Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. pg_basebackup: Add missing PQclear in error path

  1. Forbid to DROP temp tables of other sessions

    Daniil Davydov <3danissimo@gmail.com> — 2024-10-25T08:02:37Z

    Hi,
I noticed that TRUNCATE and ALTER commands on temporary tables of other
sessions produce an error "cannot truncate/alter temporary tables of other
sessions". But are there any reasons to allow us to DROP such tables?
It seems to me that the only case when we may need it is the removal of
orphan tables. But the autovacuum is responsible for this and it uses a
different functionality. I'm wondering if there are any other cases. If
not, can we just handle it for example in ExecDropStmt and produce an error
like "cannot drop temporary tables of other sessions"?

--
Best regards,
Daniil Davydov

  2. Re: Forbid to DROP temp tables of other sessions

    Maxim Orlov <orlovmg@gmail.com> — 2024-10-25T10:55:24Z

    On Fri, 25 Oct 2024 at 11:02, Daniil Davydov <3danissimo@gmail.com> wrote:

> But are there any reasons to allow us to DROP such tables?
>
Hi!

This topic has already been discussed in [0], I believe. I'm not sure how
it all ended and if there were any changes made in the end. But from the
user's perspective, temporary tables are expected to be isolated within
sessions, I think. This is an ideal solution, but does it feasible or not
is a question.

BTW, if we can "isolate" temp relations, we'll be one step close to get rid
of temp relations locking [1].

[0]
https://www.postgresql.org/message-id/flat/d4a68c6d-d6c4-d52a-56cb-babb8177b5fe%40oss.nttdata.com
[1]
https://www.postgresql.org/message-id/flat/CACG%3DezZzMbjMzshhe%2BLDd4NJ0SeRPvCH9%2BLFS7SAPbM6Qxwe5g%40mail.gmail.com

-- 
Best regards,
Maxim Orlov.

  3. Re: Forbid to DROP temp tables of other sessions

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-10-25T18:01:23Z

    Daniil Davydov <3danissimo@gmail.com> writes:
> I noticed that TRUNCATE and ALTER commands on temporary tables of other
> sessions produce an error "cannot truncate/alter temporary tables of other
> sessions". But are there any reasons to allow us to DROP such tables?
> It seems to me that the only case when we may need it is the removal of
> orphan tables. But the autovacuum is responsible for this and it uses a
> different functionality. I'm wondering if there are any other cases. If
> not, can we just handle it for example in ExecDropStmt and produce an error
> like "cannot drop temporary tables of other sessions"?

If autovacuum can do it, I don't see a reason to prevent superusers
from doing it manually.

			regards, tom lane

  4. Re: Forbid to DROP temp tables of other sessions

    Michael Paquier <michael@paquier.xyz> — 2024-10-28T01:26:04Z

    On Fri, Oct 25, 2024 at 02:01:23PM -0400, Tom Lane wrote:
> If autovacuum can do it, I don't see a reason to prevent superusers
> from doing it manually.

Being able to do a DROP of a temporary table in a controlled way can
also be very handy when working on a cluster with a corrupted catalog
state, so it is a feature if superusers are able to do that.  I'm
pretty sure that we have this argument once every few years on
pgsql-hackers..
--
Michael

  5. Re: Forbid to DROP temp tables of other sessions

    Daniil Davydov <3danissimo@gmail.com> — 2024-10-29T06:21:40Z

    Hi,
Thanks for your comments, I appreciate them.

As I continued to deal with the topic of working with temp tables of
other sessions, I noticed something like a bug. For example
(REL_17_STABLE):
Session 1:
=# CREATE TEMP TABLE test(id int);

Session 2:
=# INSERT INTO pg_temp_0.test VALUES (1);
=# INSERT INTO pg_temp_0.test VALUES (2);

Second INSERT command will end with an error "cannot access temporary
tables of other sessions". I checked why this is happening and found
errors in several places.
So, I attach two files to this email :
1) Isolation test, that shows an error in REL_17_STABLE (iso_1.patch)
2) Patch that fixes code that mistakenly considered temporary tables
to be permanent (I will be glad to receive feedback on these fixes) +
isolation test, which shows that now any action with temp table of
other session leads to error (temp_tbl_fix.patch)

Tests look kinda ugly, but I think it's inevitable, given that we
don't know exactly what the name of the temporary schema of other
session will be.

--
Best regards,
Daniil Davydov

  6. Re: Forbid to DROP temp tables of other sessions

    Stepan Neretin <sndcppg@gmail.com> — 2024-10-29T10:37:48Z

    Hi, looks good for me, but please fix formatting in temp_tbl_fix.patch!

  7. Re: Forbid to DROP temp tables of other sessions

    Rafia Sabih <rafia.pghackers@gmail.com> — 2024-10-30T12:32:32Z

    On Tue, 29 Oct 2024 at 07:22, Daniil Davydov <3danissimo@gmail.com> wrote:

> Hi,
> Thanks for your comments, I appreciate them.
>
> As I continued to deal with the topic of working with temp tables of
> other sessions, I noticed something like a bug. For example
> (REL_17_STABLE):
> Session 1:
> =# CREATE TEMP TABLE test(id int);
>
> Session 2:
> =# INSERT INTO pg_temp_0.test VALUES (1);
> =# INSERT INTO pg_temp_0.test VALUES (2);
>
> Second INSERT command will end with an error "cannot access temporary
> tables of other sessions". I checked why this is happening and found
> errors in several places.
>

Good catch. I agree with this being an unwarranted behaviour.
A minor comment from my end is the wording of the error message.
Based on the Postgresql error message style huide, something like this
could be better,
"could not access temporary relations of other sessions".


> So, I attach two files to this email :
> 1) Isolation test, that shows an error in REL_17_STABLE (iso_1.patch)
> 2) Patch that fixes code that mistakenly considered temporary tables
> to be permanent (I will be glad to receive feedback on these fixes) +
> isolation test, which shows that now any action with temp table of
> other session leads to error (temp_tbl_fix.patch)
>
> Tests look kinda ugly, but I think it's inevitable, given that we
> don't know exactly what the name of the temporary schema of other
> session will be.
>
> --
> Best regards,
> Daniil Davydov
>


-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

  8. Re: Forbid to DROP temp tables of other sessions

    Daniil Davydov <3danissimo@gmail.com> — 2024-11-14T08:55:41Z

    On Wed, Oct 30, 2024 at 7:32 PM Rafia Sabih <rafia.pghackers@gmail.com> wrote:

> Good catch. I agree with this being an unwarranted behaviour.
> A minor comment from my end is the wording of the error message.
> Based on the Postgresql error message style huide, something like this could be better,
> "could not access temporary relations of other sessions".
> --
> Regards,
> Rafia Sabih
> CYBERTEC PostgreSQL International GmbH
>
Thanks for your comment. I attach a patch with a fixed error message.
Also you can find it in commit fest
(https://commitfest.postgresql.org/51/5379/)
--
Best Regards,
Daniil Davydov

  9. Re: Forbid to DROP temp tables of other sessions

    Rafia Sabih <rafia.pghackers@gmail.com> — 2024-11-18T11:49:23Z

    On Thu, 14 Nov 2024 at 09:55, Daniil Davydov <3danissimo@gmail.com> wrote:

> On Wed, Oct 30, 2024 at 7:32 PM Rafia Sabih <rafia.pghackers@gmail.com>
> wrote:
>
> > Good catch. I agree with this being an unwarranted behaviour.
> > A minor comment from my end is the wording of the error message.
> > Based on the Postgresql error message style huide, something like this
> could be better,
> > "could not access temporary relations of other sessions".
> > --
> > Regards,
> > Rafia Sabih
> > CYBERTEC PostgreSQL International GmbH
> >
> Thanks for your comment. I attach a patch with a fixed error message.
> Also you can find it in commit fest
> (https://commitfest.postgresql.org/51/5379/)
>
Looks good to me.


> --
> Best Regards,
> Daniil Davydov
>


-- 
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH

  10. Re: Forbid to DROP temp tables of other sessions

    Andrey Borodin <x4mmm@yandex-team.ru> — 2024-11-21T20:52:52Z

    
> On 14 Nov 2024, at 11:55, Daniil Davydov <3danissimo@gmail.com> wrote:
> 
> On Wed, Oct 30, 2024 at 7:32 PM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
> 
>> Good catch. I agree with this being an unwarranted behaviour.
>> A minor comment from my end is the wording of the error message.
>> Based on the Postgresql error message style huide, something like this could be better,
>> "could not access temporary relations of other sessions".
>> --
>> Regards,
>> Rafia Sabih
>> CYBERTEC PostgreSQL International GmbH
>> 
> Thanks for your comment. I attach a patch with a fixed error message.
> Also you can find it in commit fest
> (https://commitfest.postgresql.org/51/5379/)

I suspect that protection of temp tables was broken by 00d1e02be249. And I'd suggest fixing it in a line with how it worked before. Changes to locking mechanism is kind of a super subtle matters, it is really hard to bring this checks here without breaking something else. Maybe not immidiately. but still. I'd suggest fixing somewhere around RelationAddBlocks(). But be sure to check all code pathes that lead to this check.

Also, having an isolation test is nice. But do we actually do isolation tests with PL\pgSQL?

Thanks!


Best regards, Andrey Borodin.

  11. Re: Forbid to DROP temp tables of other sessions

    Andres Freund <andres@anarazel.de> — 2024-11-21T22:02:12Z

    Hi,

On 2024-11-21 23:52:52 +0300, Andrey M. Borodin wrote:
> I suspect that protection of temp tables was broken by 00d1e02be249.

I think we might have broken this in multiple ways in recent releases. In 17
one can even read the data from the other relation when using a sequential
scan, because that'll go through a streaming read and from there directly to
StartReadBuffers(), bypassing the check in ReadBufferExtended().


> And I'd suggest fixing it in a line with how it worked before. Changes to
> locking mechanism is kind of a super subtle matters, it is really hard to
> bring this checks here without breaking something else. Maybe not
> immidiately. but still. I'd suggest fixing somewhere around
> RelationAddBlocks(). But be sure to check all code pathes that lead to this
> check.

Yea, I don't think the lock approach would work that well.  However, I don't
love having to put RELATION_IS_OTHER_TEMP() checks everywhere either. After
all we seem to have introduced two independent oversights related to this...

I wonder if we could handle this by having a few locations explicitly opt-in
to accessing another database's temp table and erroring out everywhere else -
there's not that many places we need to do so.


> Also, having an isolation test is nice. But do we actually do isolation
> tests with PL\pgSQL?

There are several other tests creating functions. But I think this one goes a
bit too far...


Greetings,

Andres Freund

  12. Re: Forbid to DROP temp tables of other sessions

    Andrey Borodin <x4mmm@yandex-team.ru> — 2024-11-23T18:13:02Z

    
> On 22 Nov 2024, at 03:02, Andres Freund <andres@anarazel.de> wrote:
> 
> I don't
> love having to put RELATION_IS_OTHER_TEMP() checks everywhere either.

+1. I do not understand why this restriction (protecting temp tables from access) is a responsibility of the buffer manager.

Actually, I like the idea that superuser knows better what to do.
What if we say it's not a bug, but a feature. Will it break some contracts with user or some functionality?

It seems that protection of temp tables should belong to ACL stuff. And in a logic of this subsystem would be natural to just allow superuser do whatever they want with.

Is it some lunatic idea? Or does it make some sense?


Best regards, Andrey Borodin.

  13. Re: Forbid to DROP temp tables of other sessions

    Maxim Orlov <orlovmg@gmail.com> — 2024-11-25T07:44:55Z

    On Sat, 23 Nov 2024 at 21:13, Andrey M. Borodin <x4mmm@yandex-team.ru>
wrote:

> What if we say it's not a bug, but a feature. Will it break some contracts
> with user or some functionality?


An important thing to note here. We have to trade off an opportunity to
significantly improve temp tables performance by removing locks for a "not
a bug, but a feature". This seems odd to me. Arguably, the number of people
who need faster temp relations is greater than the number of people who
want to have access to temp relations of other backends.

-- 
Best regards,
Maxim Orlov.

  14. Re: Forbid to DROP temp tables of other sessions

    Daniil Davydov <3danissimo@gmail.com> — 2025-02-19T06:13:07Z

    Hi,
I'm sorry for the long lull.
Considering that it is still important for some users to be able to
clean other sessions' temporary directories, I suggest adding a GUC
that will allow superuser to do this.
We keep only one option - to drop tables, because only this feature
works properly in postgres by now.

The ability to read and modify other session's temp tables is broken
(I mean INSERT, UPDATE, DELETE, SELECT queries) and needs to be
disabled.
I wrote about broken INSERTs here :
https://www.postgresql.org/message-id/CAJDiXgj6TBzn%3D6Ezx7%2B9BNa9HpBitBU%2BMuv-N3mHeN_Zs3NBDw%40mail.gmail.com

What do you think about this idea?

--
Best regards,
Daniil Davydov

  15. Re: Forbid to DROP temp tables of other sessions

    vignesh C <vignesh21@gmail.com> — 2025-03-16T12:53:31Z

    On Thu, 14 Nov 2024 at 14:26, Daniil Davydov <3danissimo@gmail.com> wrote:
>
> On Wed, Oct 30, 2024 at 7:32 PM Rafia Sabih <rafia.pghackers@gmail.com> wrote:
>
> > Good catch. I agree with this being an unwarranted behaviour.
> > A minor comment from my end is the wording of the error message.
> > Based on the Postgresql error message style huide, something like this could be better,
> > "could not access temporary relations of other sessions".
> > --
> > Regards,
> > Rafia Sabih
> > CYBERTEC PostgreSQL International GmbH
> >
> Thanks for your comment. I attach a patch with a fixed error message.
> Also you can find it in commit fest
> (https://commitfest.postgresql.org/51/5379/)

I noticed that the following Andrey's comment regarding the isolation
test from [1] and Andres's comment  from [2] are pending. I'm changing
the commitfest entry to Waiting on Author, please provide an updated
patch and update it to Needs review.
[1] - https://www.postgresql.org/message-id/9A1C6763-A293-4EDD-A462-EFCF93BA2909%40yandex-team.ru
[2] - https://www.postgresql.org/message-id/4xxau766dofbwugeyvjftra3g5f7ifaal2clgrbpr7jqotr4av%40d3ige2krpoza

Regards,
Vignesh

  16. Re: Forbid to DROP temp tables of other sessions

    Daniil Davydov <3danissimo@gmail.com> — 2025-03-17T05:11:46Z

    Hi,

On Sun, Mar 16, 2025 at 7:53 PM vignesh C <vignesh21@gmail.com> wrote:
> I noticed that the following Andrey's comment regarding the isolation
> test from [1] and Andres's comment  from [2] are pending. I'm changing
> the commitfest entry to Waiting on Author, please provide an updated
> patch and update it to Needs review.

Thanks for reading it.
I saw [2] and introduced a possible solution in my last letter. In
short : we can have a GUC variable that will permit superuser to drop
temp tables of other sessions. Thus, we have a single location in code
that will check whether we can perform operations with other temp
tables. As far as I understand, this is exactly what Andres wrote
about.
Also, it is difficult for me to express my opinion on [1] at the
moment. I can say for sure that the tests will change when we agree on
the behavior of the code. Therefore, I suggest postponing the
resolution of this issue.

> I suggest adding a GUC that will allow superuser to do this
Waiting for your feedback on this issue :)

--
Best regards,
Daniil Davydov

  17. Re: Forbid to DROP temp tables of other sessions

    David G. Johnston <david.g.johnston@gmail.com> — 2025-03-17T06:15:51Z

    On Sunday, March 16, 2025, Daniil Davydov <3danissimo@gmail.com> wrote:

> Hi,
>
> On Sun, Mar 16, 2025 at 7:53 PM vignesh C <vignesh21@gmail.com> wrote:
> > I noticed that the following Andrey's comment regarding the isolation
> > test from [1] and Andres's comment  from [2] are pending. I'm changing
> > the commitfest entry to Waiting on Author, please provide an updated
> > patch and update it to Needs review.
>
> Thanks for reading it.
> I saw [2] and introduced a possible solution in my last letter. In
> short : we can have a GUC variable that will permit superuser to drop
> temp tables of other sessions. Thus, we have a single location in code
> that will check whether we can perform operations with other temp
> tables. As far as I understand, this is exactly what Andres wrote
> about.
>

It’s seems like the bug “session A can read and write to session B’s
tables” has gotten lost in this thread that pertains to something related
but different.  I strongly suggest you break out a new thread for this bug
with an attention-getting subject line.  Seems we should be fixing that
without regards to how to refactor this area of the code (maybe we did, I
haven’t followed or am able to experiment right now…just reading this
thread).

Solving the bug is not going to involve adding a new GUC.  I don’t really
see how a GUC helps here at all - the superuser shouldn’t need to opt-in
they could always do before.  If they specify the precise pg_temp schema to
affect they likely didn’t make a mistake.  The feature is a no-go if it
only applies to superuser anyway.  If instead we are discussing the owner
of the temporary table there is probably a discussion to be had and
decision to be documented somewhere - maybe that central place of testing
being wished for.

David J.

  18. Re: Forbid to DROP temp tables of other sessions

    David G. Johnston <david.g.johnston@gmail.com> — 2025-03-17T06:52:31Z

    On Saturday, November 23, 2024, Andrey M. Borodin <x4mmm@yandex-team.ru>
wrote:
>
>
> It seems that protection of temp tables should belong to ACL stuff. And in
> a logic of this subsystem would be natural to just allow superuser do
> whatever they want with.
>

My understanding is the limitation of an owner of a temporary relation in
one session being disallowed to alter its contents from another session is
an implementation consequence, and not some fundamental model restriction.
ACL doesn’t interact with Sessions or Transactions.  Nor should it.

Minimally informed thinking, associate the specific pg_temp namespace with
a procid.  Where this limitation exists, which seems like middle
management, compare the proc of the namespace to the executor.  Pass the
role and also an enum of action type (CRUD, drop, truncate, lock, etc…).
If the procs match all good.  Superuser cannot bypass CRUD and similar as
that is the limitation being implemented here.  And the owner cannot bypass
anything (exceptions could be added as desired).

Centralizing things a bit though…maybe something like the relcache (for
namespaces…) so you cannot even get a handle on the namespace if you don’t
supply the info and pass the checks.  Don’t really know enough to say
where/how to implement “if you forget to call this check all commands that
can reference tables will fail”.

David J.

  19. Re: Forbid to DROP temp tables of other sessions

    Daniil Davydov <3danissimo@gmail.com> — 2025-03-17T08:00:45Z

    Hi,

On Mon, Mar 17, 2025 at 1:15 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> It’s seems like the bug “session A can read and write to session B’s tables” has gotten lost in this thread that pertains to something related but different. .... Solving the bug is not going to involve adding a new GUC.
I don't think it's worth putting this in a separate discussion. Now
everything is moving towards the fact that the superuser will be
prohibited from changing the temporary tables of other sessions. In
fact, he can't do it anyway (except for DROP command) - see [1]. But
now the error for INSERT, UPDATE, DELETE and SELECT commands may not
surface immediately due to errors in the code. The only question now
is whether superuser should be allowed to DROP these other temp
tables. Since opinions are divided, I suggested adding a GUC that will
only control this feature.

> If they specify the precise pg_temp schema to affect they likely didn’t make a mistake.
Yep, I agree. However, the features of the postgres kernel do not
allow the superuser to correctly perform INSERT, UPDATE, DELETE,
SELECT operations, because temporary table's pages cannot be stored in
shared buffers.

> If instead we are discussing the owner of the temporary table there is probably a discussion to be had and decision to be documented somewhere - maybe that central place of testing being wished for.
As far as I understand, only superuser and table's owner (within
session) can access the temp tables of session. We want CRUD
operations to be performed only by the owner.

--
Best regards,
Daniil Davydov

[1] https://www.postgresql.org/message-id/CAJDiXgj6TBzn=6Ezx7+9BNa9HpBitBU+Muv-N3mHeN_Zs3NBDw@mail.gmail.com

  20. Re: Forbid to DROP temp tables of other sessions

    Daniil Davydov <3danissimo@gmail.com> — 2025-03-17T08:31:39Z

    Hi,

On Mon, Mar 17, 2025 at 1:52 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> My understanding is the limitation of an owner of a temporary relation in one session being disallowed to alter its contents from another session is an implementation consequence, and not some fundamental model restriction.
I would say this: working with temporary tables (in the postgres
kernel) is so very different from working with regular heap tables
that such a limitation can be considered fundamental. We simply will
not be able to organize coordinated work on the temporary table from
different OS processes (for INSERT, UPDATE, DELETE).

> Minimally informed thinking, associate the specific pg_temp namespace with a procid.  Where this limitation exists, which seems like middle management, compare the proc of the namespace to the executor.  Pass the role and also an enum of action type (CRUD, drop, truncate, lock, etc…).  If the procs match all good.  Superuser cannot bypass CRUD and similar as that is the limitation being implemented here.  And the owner cannot bypass anything (exceptions could be added as desired).
>
> Centralizing things a bit though…maybe something like the relcache (for namespaces…) so you cannot even get a handle on the namespace if you don’t supply the info and pass the checks.  Don’t really know enough to say where/how to implement “if you forget to call this check all commands that can reference tables will fail”.
I'm sorry, I probably don't quite understand what this is about, so
I'll just describe how it works now. If a superuser wants to access
other temp table, he must specify schema_name in request (for example
: INSERT INTO pg_temp_N.test .....). N is the id of the owner process.
Thus, postgres will call RangeVarGetRelidExtended to find the given
relation's oid. It is at this point that we can step in and check
whether the caller can work with the specified schema. It is
elementary to understand that the schema does belong to another
session. Right now, there is a bug in the code that mistakenly
recognizes the table in such a case as permanent (not temporary), so
we cannot do what I just described. So, we have to get rid of this bug
and decide whether we reserve the right for the superuser to DROP such
tables.

I hope this remark will be useful.

--
Best regards,
Daniil Davydov

  21. Re: Forbid to DROP temp tables of other sessions

    Daniil Davydov <3danissimo@gmail.com> — 2025-03-17T09:03:55Z

    Hi,
I see that the presence of isolation tests in the patch is
controversial. First things first, let's concentrate on fixing the
bug.
I attach a new version of patch (for `master` branch) to this letter.
It contains better comments and a few small improvements.

P.S.
Sorry for bad formatting in previous letter (idk how to fix it in gmail client)

--
Best regards,
Daniil Davydov

  22. Re: Forbid to DROP temp tables of other sessions

    Steven Niu <niushiji@gmail.com> — 2025-03-17T09:48:06Z

    Hi,

I have some comments:

1. namespace.c, if relation->schemaname is pg_temp but myTempNamespace 
isn't set, the error information might be misleading. Consider checking 
OidIsValid(myTempNamespace) first.

2."you have not any temporary relations" --> "you have no any temporary 
relations"

3. Regarding to the code "strncmp(nspname, "pg_temp", 7)", is it ok when 
the nspname contains something like "pg_temp_1234"? I think we should 
use strcmp instead of strncmp for exact matching.

Thanks,
Steven

在 2025/3/17 17:03, Daniil Davydov 写道:
> Hi,
> I see that the presence of isolation tests in the patch is
> controversial. First things first, let's concentrate on fixing the
> bug.
> I attach a new version of patch (for `master` branch) to this letter.
> It contains better comments and a few small improvements.
> 
> P.S.
> Sorry for bad formatting in previous letter (idk how to fix it in gmail client)
> 
> --
> Best regards,
> Daniil Davydov

  23. Re: Forbid to DROP temp tables of other sessions

    Daniil Davydov <3danissimo@gmail.com> — 2025-03-17T10:13:27Z

    Hi,

On Mon, Mar 17, 2025 at 4:48 PM Steven Niu <niushiji@gmail.com> wrote:
>
> 1. namespace.c, if relation->schemaname is pg_temp but myTempNamespace
> isn't set, the error information might be misleading. Consider checking
> OidIsValid(myTempNamespace) first.
Could you please clarify exactly which place in the code we are
talking about? I think we handle this case in the
LookupExplicitNamespace call (with all appropriate error information).

>
> 2."you have not any temporary relations" --> "you have no any temporary
> relations"
I am not an English speaker, but it seems that "have not" would be
more correct. Someone has to judge us :)

>
> 3. Regarding to the code "strncmp(nspname, "pg_temp", 7)", is it ok when
> the nspname contains something like "pg_temp_1234"? I think we should
> use strcmp instead of strncmp for exact matching.
Great catch! I'll fix it. Please, see v3 patch.

--
Best regards,
Daniil Davydov

  24. Re: Forbid to DROP temp tables of other sessions

    Steven Niu <niushiji@gmail.com> — 2025-03-17T10:32:53Z

    
在 2025/3/17 18:13, Daniil Davydov 写道:
> Hi,
> 
> On Mon, Mar 17, 2025 at 4:48 PM Steven Niu <niushiji@gmail.com> wrote:
>>
>> 1. namespace.c, if relation->schemaname is pg_temp but myTempNamespace
>> isn't set, the error information might be misleading. Consider checking
>> OidIsValid(myTempNamespace) first.
> Could you please clarify exactly which place in the code we are
> talking about? I think we handle this case in the
> LookupExplicitNamespace call (with all appropriate error information).
> 

I mean RangeVarGetRelidExtended(), you deleted the following code:

if (!OidIsValid(myTempNamespace))
            relId = InvalidOid; /* this probably can't happen? */


>>
>> 2."you have not any temporary relations" --> "you have no any temporary
>> relations"
> I am not an English speaker, but it seems that "have not" would be
> more correct. Someone has to judge us :)
> 
>>
>> 3. Regarding to the code "strncmp(nspname, "pg_temp", 7)", is it ok when
>> the nspname contains something like "pg_temp_1234"? I think we should
>> use strcmp instead of strncmp for exact matching.
> Great catch! I'll fix it. Please, see v3 patch.
> 
> --
> Best regards,
> Daniil Davydov

  25. Re: Forbid to DROP temp tables of other sessions

    Daniil Davydov <3danissimo@gmail.com> — 2025-03-17T10:56:04Z

    Hi,

On Mon, Mar 17, 2025 at 5:33 PM Steven Niu <niushiji@gmail.com> wrote:
>
> I mean RangeVarGetRelidExtended(), you deleted the following code:
>
> if (!OidIsValid(myTempNamespace))
>             relId = InvalidOid; /* this probably can't happen? */

Hm, I got it. Let's start with the fact that the comment "this
probably can't happen?" is incorrect. Even if we don't have a
temporary namespace in our session, we still can specify "pg_temp_N"
in the psql query.
Next, if relation->schemaname is pg_temp, then we firstly call
LookupExplicitNamespace, where you can find if
(!OidIsValid(myTempNamespace)) check.

--
Best regards,
Daniil Davydov

  26. Re: Forbid to DROP temp tables of other sessions

    Steven Niu <niushiji@gmail.com> — 2025-03-17T13:28:50Z

    
在 2025/3/17 18:56, Daniil Davydov 写道:
> Hi,
> 
> On Mon, Mar 17, 2025 at 5:33 PM Steven Niu <niushiji@gmail.com> wrote:
>>
>> I mean RangeVarGetRelidExtended(), you deleted the following code:
>>
>> if (!OidIsValid(myTempNamespace))
>>              relId = InvalidOid; /* this probably can't happen? */
> 
> Hm, I got it. Let's start with the fact that the comment "this
> probably can't happen?" is incorrect. Even if we don't have a
> temporary namespace in our session, we still can specify "pg_temp_N"
> in the psql query.
> Next, if relation->schemaname is pg_temp, then we firstly call
> LookupExplicitNamespace, where you can find if
> (!OidIsValid(myTempNamespace)) check.
> 
> --
> Best regards,
> Daniil Davydov

If the (relation->relpersistence == RELPERSISTENCE_TEMP) can ensure the 
myTempNamespace is always valid, then my comment can be ignored. 
Otherwise I think the modified RangeVarGetRelidExtended() should keep 
check of myTempNamespace, like this:

if (relation->relpersistence == RELPERSISTENCE_TEMP)
{
   Oid namespaceId;

  if (!OidIsValid(myTempNamespace))
      relId = InvalidOid; /* this probably can't happen? */
  else
  {
    if (relation->schemaname)
    {
      namespaceId = LookupExplicitNamespace(relation->schemaname, 
missing_ok);
      if (namespaceId != myTempNamespace)
      {
          ereport(ERROR,
                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                   errmsg("could not access temporary relations of other 
sessions")));
      }
   }
   else
   {
      namespaceId = myTempNamespace;
      Assert(OidIsValid(namespaceId));
   }
   if (missing_ok && !OidIsValid(namespaceId))
      relId = InvalidOid;
   else
      relId = get_relname_relid(relation->relname, namespaceId);
  }
...
...

Thanks,
Steven

  27. Re: Forbid to DROP temp tables of other sessions

    David G. Johnston <david.g.johnston@gmail.com> — 2025-03-17T15:09:12Z

    On Monday, March 17, 2025, Daniil Davydov <3danissimo@gmail.com> wrote:

>
> >
> > 2."you have not any temporary relations" --> "you have no any temporary
> > relations"
> I am not an English speaker, but it seems that "have not" would be
> more correct. Someone has to judge us :)
>
>
Both are not good.

“pg_temp was specified but it contains no relations” [1]

But why are we promoting this situation to an error?  It should be a
relation not found error just like any other and not its own special case.
The fact we create pg_temp only as it is needed is an implementation detail
that should not be visible to the user.  Either by saying pg_temp not found
(possibly at this point) or pretending it does and letting the relation
name lookup produce the error.

David J.

[1] It isn’t part of the style guide but I don’t think we use “you” to
directly refer to the query author in error messages.

  28. Re: Forbid to DROP temp tables of other sessions

    Daniil Davydov <3danissimo@gmail.com> — 2025-03-17T17:04:15Z

    Hi,

On Mon, Mar 17, 2025 at 8:29 PM Steven Niu <niushiji@gmail.com> wrote:
>
> If the (relation->relpersistence == RELPERSISTENCE_TEMP) can ensure the
> myTempNamespace is always valid, then my comment can be ignored.

No, even if we see a temporary table in RangeVarGetRelidExtended,
MyTempNamespace still can be `InvalidOid` (I mentioned it in the
previous letter).
Thus, comment like "this probably can't happen?" should be removed.

> Otherwise I think the modified RangeVarGetRelidExtended() should keep
> check of myTempNamespace, like this:
>
> if (relation->relpersistence == RELPERSISTENCE_TEMP)
> {
>    Oid namespaceId;
>
>   if (!OidIsValid(myTempNamespace))
>       relId = InvalidOid; /* this probably can't happen? */
>  ...

OK, let's assume that MyTempNamespace == InvalidOid and caller
specified "pg_temp_N" in his query. In this case we want to throw an
error, because access to the other temp tables is prohibited.
If we keep code like "if (!OidIsValid(myTempNamespace)) => relId =
InvalidOid", eventually the caller will get an error "relation ...
does not exist".
Yes, we have saved the caller from making mistakes, but we are giving
the wrong error message.

In my realization the caller will get the correct error message, and I
still think that we should keep v3 patch logic.

--
Best regards,
Daniil Davydov

  29. Re: Forbid to DROP temp tables of other sessions

    Daniil Davydov <3danissimo@gmail.com> — 2025-03-17T17:58:45Z

    Hi,

On Mon, Mar 17, 2025 at 10:09 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> On Monday, March 17, 2025, Daniil Davydov <3danissimo@gmail.com> wrote:
>>
>>
>> >
>> > 2."you have not any temporary relations" --> "you have no any temporary
>> > relations"
>> I am not an English speaker, but it seems that "have not" would be
>> more correct. Someone has to judge us :)
>>
>
> Both are not good.
>
> “pg_temp was specified but it contains no relations” [1]

That sounds reasonable. I'll fix it. Thanks!

> But why are we promoting this situation to an error?  It should be a relation not found error just like any other and not its own special case.
> The fact we create pg_temp only as it is needed is an implementation detail that should not be visible to the user.

1)
Error message has no mention of a non-existent schema. "Schema has no
relations" => "Given relation not found in empty schema". It seems to
me that these are equivalent statements.

2)
Is this really the implementation detail that we want to hide from the
user? User can just run "select pg_my_temp_schema();" and see that
there is no temp schema in the current session.
Don't get me wrong - I can agree with that, but for now it seems odd to me...
Steven Niu also mentioned this issue, but IMO we must give the most
accurate description of the problem - tell "relation not found" only
if we have temp namespace, but not specified relation in it.

Please see v4 patch (only comment change).

--
Best regards,
Daniil Davydov

  30. Re: Forbid to DROP temp tables of other sessions

    David G. Johnston <david.g.johnston@gmail.com> — 2025-03-17T19:36:04Z

    On Mon, Mar 17, 2025 at 10:58 AM Daniil Davydov <3danissimo@gmail.com>
wrote:

> 2)

Is this really the implementation detail that we want to hide from the
> user? User can just run "select pg_my_temp_schema();" and see that
> there is no temp schema in the current session.
>

No ordinary user uses that function; it serves no everyday usage need.

Don't get me wrong - I can agree with that, but for now it seems odd to
> me...
> Steven Niu also mentioned this issue, but IMO we must give the most
> accurate description of the problem - tell "relation not found" only
> if we have temp namespace, but not specified relation in it.
>
>
"I want to give a better error message" is not a good enough reason to
change this long-standing behavior in a back-patchable bug fix.

IOW, you don't get to change:

postgres=# select * from pg_temp.temp_table;
ERROR:  relation "pg_temp.temp_table" does not exist
LINE 1: select * from pg_temp.temp_table;

to

postgres=# select * from pg_temp.tmptable;
ERROR:  pg_temp was specified but it contains no relations
LINE 1: select * from pg_temp.tmptable;

In a released branch; and I do not agree that it is an improvement worth
making in HEAD.

David J.

  31. Re: Forbid to DROP temp tables of other sessions

    David G. Johnston <david.g.johnston@gmail.com> — 2025-03-17T20:30:16Z

    On Mon, Mar 17, 2025 at 10:58 AM Daniil Davydov <3danissimo@gmail.com>
wrote:

>
> Please see v4 patch (only comment change).
>
>
 /*
- * For missing_ok, allow a non-existent schema name to
- * return InvalidOid.
+ * We don't allow users to access temp tables of other
+ * sessions (consider adding GUC for to allow to DROP such
+ * tables?).
  */

I sound like a broken record but this is a bug fix; introducing a GUC and
changing unrelated behaviors is not acceptable in a back-patch.

Superusers have been able to drop the temporary tables of other sessions
for a long time - now is not the place to change that behavior.

Separately:

Is the general logic here that we assume r->relpersistence is
RELPERSISTENCE_PERMANENT until and unless we can prove it to be
RELPERSISTENCE_TEMP?

I'm trying to figure out whether there is still an issue when dealing with
an unqualified name that would be found in the temporary schema.  We've
obviously already marked it permanent since we didn't have pg_temp in the
query to inform us otherwise.  But if we are changing permanent to
temporary later because of search_path resolution then why did we have to
get it right in the case where pg_temp is specified?

I question whether "persistence" is something that gram.y should be dealing
with at all.  Shouldn't that property be solely determined in post-parse
analysis via catalog lookup?  The best you can do in the grammar is break
separation of concerns by programming to pg_temp as you are here, and
deduce it is temporary, or not, so long as a schema is listed.

IOW, the original code here seems incorrect if this is the definitive place
to determine relpersistence.  in "case 1:", where there is no schema name,
one cannot deduce relpersistence and it should remain NULL, IMO (not
knowing what that might break...).  The code this patch replaces is wrong
for the same reason.

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 271ae26cbaf..f68948d475c 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -19424,7 +19424,11 @@ makeRangeVarFromAnyName(List *names, int position,
core_yyscan_t yyscanner)
  break;
  }

- r->relpersistence = RELPERSISTENCE_PERMANENT;
+ if (r->schemaname && strncmp(r->schemaname, "pg_temp", 7) == 0)
+ r->relpersistence = RELPERSISTENCE_TEMP;
+ else
+ r->relpersistence = RELPERSISTENCE_PERMANENT;
+
  r->location = position;

The qualified name variant is fine since r->schemaname must be present by
definition.  Passing through the same if block, once with schemaname null
(in makeRangeVar) and once with it populated (end of
makeRangeVarFromQualifiedName) is a bit annoying.

makeRangeVar has the same problem, assuming permanent when the schemaname
argument is null.

I guess if others more knowledgeable agree existing assertions are OK this
patch is strictly improving things.  Whether it is enough in the case of a
non-schema qualified relation in the query text I cannot say.

David J.

  32. Re: Forbid to DROP temp tables of other sessions

    Daniil Davydov <3danissimo@gmail.com> — 2025-03-18T04:26:49Z

    Hi,

On Tue, Mar 18, 2025 at 2:36 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> "I want to give a better error message" is not a good enough reason to change this long-standing behavior in a back-patchable bug fix.
>.... and I do not agree that it is an improvement worth making in HEAD.

OK, In this case I'd rather agree.


On Tue, Mar 18, 2025 at 3:30 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> I sound like a broken record but this is a bug fix; introducing a GUC and changing unrelated behaviors is not acceptable in a back-patch.
>
> Superusers have been able to drop the temporary tables of other sessions for a long time - now is not the place to change that behavior.

Well, we can keep this option for the superuser, but we need to
explicitly indicate that we are inside the DROP operation.
Please see v5 patch. It is a bit experimental :
1) The original behavior in the LookupExplicitNamespace function has
been returned.
2) New RVROption has been added to indicate that we are inside the
DROP operation. Thus, superuser can drop other temp tables.

>
> Separately:
>
> Is the general logic here that we assume r->relpersistence is RELPERSISTENCE_PERMANENT until and unless we can prove it to be RELPERSISTENCE_TEMP?
>
> I'm trying to figure out whether there is still an issue when dealing with an unqualified name that would be found in the temporary schema.
> We've obviously already marked it permanent since we didn't have pg_temp in the query to inform us otherwise.
> But if we are changing permanent to temporary later because of search_path resolution then why did we have to get it right in the case where pg_temp is specified?
> I question whether "persistence" is something that gram.y should be dealing with at all.  Shouldn't that property be solely determined in post-parse analysis via catalog lookup?

Hm, let's dive into this question. Why do I consider the original
behavior to be incorrect?

1)
If schema is not specified, then gram.y marks the table as PERMANENT.
1.1)
If we are looking for our temporary table, then "pg_temp_N" is present
in search_path, and RelnameGetRelid will return us this table.
1.2)
If we are looking for other temporary table, then RelnameGetRelid will
return `InvalidOid` and we will get a "relation not found" error.

2)
If schema is specified, then gram.y ALSO marks the table as PERMANENT.
2.1)
If we are looking for our temporary table, then
LookupExplicitNamespace will return us MyTempNamespace and we can get
our table without search_path lookup.
2.2)
If we are looking for other temporary table, then
LookupExplicitNamespace will return some valid oid, and we can get
other temp table without search_path lookup. Then, we will perform any
specified operation, assuming that we are working with a PERMANENT
table. This is a bug.

Let's summarize. If schema is not specified, we can safely mark the
table as PERMANENT, because we always can get the table from
search_path (and if the schema is not specified, then it is obvious
that the user wants to refer specifically to his table).
BUT, if schema is specified, we must know that the given relation is
TEMP before calling RangeVarGetRelidExtended (otherwise, there will be
an error, which I wrote about in paragraph 2.2).

>
> IOW, the original code here seems incorrect if this is the definitive place to determine relpersistence.  in "case 1:", where there is no schema name, one cannot deduce relpersistence and it should remain NULL, IMO (not knowing what that might break...).  The code this patch replaces is wrong for the same reason.

I hope that I managed to clarify this issue. As far as "pg_temp_"
prefix is reserved by the postgres kernel, we can definitely say that
we have encountered a temporary table when we see this prefix. IMO
there is no problem, that gram.y will do it.

>
> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
> index 271ae26cbaf..f68948d475c 100644
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
> @@ -19424,7 +19424,11 @@ makeRangeVarFromAnyName(List *names, int position, core_yyscan_t yyscanner)
>   break;
>   }
>
> - r->relpersistence = RELPERSISTENCE_PERMANENT;
> + if (r->schemaname && strncmp(r->schemaname, "pg_temp", 7) == 0)
> + r->relpersistence = RELPERSISTENCE_TEMP;
> + else
> + r->relpersistence = RELPERSISTENCE_PERMANENT;
> +
>   r->location = position;
>
> The qualified name variant is fine since r->schemaname must be present by definition.  Passing through the same if block, once with schemaname null (in makeRangeVar) and once with it populated (end of makeRangeVarFromQualifiedName) is a bit annoying.
>
> makeRangeVar has the same problem, assuming permanent when the schemaname argument is null.

Thank you for noticing it. I suggest we first confirm that the logic
(with persistence definition) remains in gram.y and then fix this
problem.

--
Best regards,
Daniil Davydov

  33. Re: Forbid to DROP temp tables of other sessions

    David G. Johnston <david.g.johnston@gmail.com> — 2025-03-18T23:32:19Z

    On Mon, Mar 17, 2025 at 9:27 PM Daniil Davydov <3danissimo@gmail.com> wrote:

> Hi,
>
> On Tue, Mar 18, 2025 at 2:36 AM David G. Johnston
> <david.g.johnston@gmail.com> wrote:
> >
> > "I want to give a better error message" is not a good enough reason to
> change this long-standing behavior in a back-patchable bug fix.
> >.... and I do not agree that it is an improvement worth making in HEAD.
>
> OK, In this case I'd rather agree.
>

I'm probably done trying to help with this one - it's beyond my ability and
desire to contribute to meaningfully.  It seems to need to be escalated if
the regression has a chance of being understood and fixed.  Some good
candidates for helping out are copied already so I'm just hoping one or
more of them chimes in.  I'll repeat my suggestion to start a new thread
and let this one die.  The answer to $subject is no.

David J.

  34. Re: Forbid to DROP temp tables of other sessions

    Daniil Davydov <3danissimo@gmail.com> — 2025-03-19T03:12:20Z

    Hi,

On Wed, Mar 19, 2025 at 6:32 AM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> I'm probably done trying to help with this one - it's beyond my ability and desire to contribute to meaningfully.  It seems to need to be escalated if the regression has a chance of being understood and fixed.
> Some good candidates for helping out are copied already so I'm just hoping one or more of them chimes in.  I'll repeat my suggestion to start a new thread and let this one die.  The answer to $subject is no.

Well, the name of the thread has stopped reflecting what is being
discussed here. I may start a new discussion in the near future, but
as part of this discussion I would like to make sure that people agree
with the statements from my previous letter.

Thanks for your help.

--
Best regards,
Daniil Davydov

  35. Add missing PQclear for StreamLogicalLog function

    Steven Niu <niushiji@gmail.com> — 2025-03-19T05:38:26Z

    Hi, hackers,

During browsing the code, I found one missing PQclear in function 
StreamLogicalLog(). It's a very small problem as it only happens in 
error condition. However since another similar patch was accepted,
maybe we should also fix this one.

https://www.postgresql.org/message-id/flat/3DA7CECD-5A05-416D-8527-ABD794AEFE8B%40yesql.se#c5d662ba7bdb07e56ddbd9aaa90dea5d 


Regards,
Steven

  36. Re: Add missing PQclear for StreamLogicalLog function

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-19T10:03:42Z

    > On 19 Mar 2025, at 06:38, Steven Niu <niushiji@gmail.com> wrote:

> During browsing the code, I found one missing PQclear in function StreamLogicalLog(). It's a very small problem as it only happens in error condition. However since another similar patch was accepted,
> maybe we should also fix this one.

Thanks for the report, the patch looks reasonable so I'll have a look at
applying it once back from travelling.

--
Daniel Gustafsson

  37. Re: Add missing PQclear for StreamLogicalLog function

    Daniel Gustafsson <daniel@yesql.se> — 2025-03-25T16:46:51Z

    > On 19 Mar 2025, at 11:03, Daniel Gustafsson <daniel@yesql.se> wrote:
> 
>> On 19 Mar 2025, at 06:38, Steven Niu <niushiji@gmail.com> wrote:
> 
>> During browsing the code, I found one missing PQclear in function StreamLogicalLog(). It's a very small problem as it only happens in error condition. However since another similar patch was accepted,
>> maybe we should also fix this one.
> 
> Thanks for the report, the patch looks reasonable so I'll have a look at
> applying it once back from travelling.

Applied to head, thanks!

--
Daniel Gustafsson