Forbid to DROP temp tables of other sessions

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]/messages/by-id/d4a68c6d-d6c4-d52a-56cb-babb8177b5fe@oss.nttdata.com, 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]/messages/by-id/CACG=ezZzMbjMzshhe+LDd4NJ0SeRPvCH9+LFS7SAPbM6Qxwe5g@mail.gmail.com.

[0]: /messages/by-id/d4a68c6d-d6c4-d52a-56cb-babb8177b5fe@oss.nttdata.com
/messages/by-id/d4a68c6d-d6c4-d52a-56cb-babb8177b5fe@oss.nttdata.com
[1]: /messages/by-id/CACG=ezZzMbjMzshhe+LDd4NJ0SeRPvCH9+LFS7SAPbM6Qxwe5g@mail.gmail.com
/messages/by-id/CACG=ezZzMbjMzshhe+LDd4NJ0SeRPvCH9+LFS7SAPbM6Qxwe5g@mail.gmail.com

--
Best regards,
Maxim Orlov.

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

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

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

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

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

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

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

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.

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

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.

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.

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 :
/messages/by-id/CAJDiXgj6TBzn=6Ezx7+9BNa9HpBitBU+Muv-N3mHeN_Zs3NBDw@mail.gmail.com

What do you think about this idea?

--
Best regards,
Daniil Davydov

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]/messages/by-id/9A1C6763-A293-4EDD-A462-EFCF93BA2909@yandex-team.ru and Andres's comment from [2]/messages/by-id/4xxau766dofbwugeyvjftra3g5f7ifaal2clgrbpr7jqotr4av@d3ige2krpoza are pending. I'm changing
the commitfest entry to Waiting on Author, please provide an updated
patch and update it to Needs review.
[1]: /messages/by-id/9A1C6763-A293-4EDD-A462-EFCF93BA2909@yandex-team.ru
[2]: /messages/by-id/4xxau766dofbwugeyvjftra3g5f7ifaal2clgrbpr7jqotr4av@d3ige2krpoza

Regards,
Vignesh

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

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.

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.

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]/messages/by-id/CAJDiXgj6TBzn=6Ezx7+9BNa9HpBitBU+Muv-N3mHeN_Zs3NBDw@mail.gmail.com. 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]: /messages/by-id/CAJDiXgj6TBzn=6Ezx7+9BNa9HpBitBU+Muv-N3mHeN_Zs3NBDw@mail.gmail.com

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