Re: Fix bug with accessing to temporary tables of other sessions - Mailing list pgsql-hackers

Hi,

On Wed, Apr 22, 2026 at 6:41 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Apr 21, 2026 at 01:54:47PM +0300, Alexander Korotkov wrote:
> > OK. I'm going to push and backpatch if no objections.
>
> Timing is interesting here.  Last week I have been doing a on-site
> patch review with a couple of colleagues and myself, and this thread
> has been chosen as part of this exercise.  I am attaching them in CC
> of this thread, and replying to this thread was an action item I had
> to act on.
>
> Here is some feedback, based on v18 posted here:
> https://www.postgresql.org/message-id/b6614954-6c71-451a-a1d7-345d255b4afb@uni-muenster.de
>

Thank you for looking into this!

> We have found that the thread does not state clearly what it aims to
> fix.  The subject states that it is a bug, perhaps it is but the
> thread does not seem to do a good job in explaining why the current
> superuser behavior is bad, while the behavior of the patch to block
> some commands is better.  This should be clearer in explaining why
> this new behavior is better.

This patch doesn't provide any new behavior. It returns consistent behavior,
which we have lost after the appearance of streaming I/O in sequential scans.
Jim wrote about it here [1]. You can also find this in the commit message of
the v19 patch.

IMHO, both thread and attached patch do the job of explaining why this fix is
necessary. But I think I understand what confused you. Please, see my comments
below.

> The test script is under-documented.  There are zero comments that
> actually explain why the patch does what it does.  The few comments
> present could be removed: they are copy-pasted of the descriptions of
> the test cases.  A much worse thing is this thing:
>
> +# DROP TEMPORARY TABLE from other session
> +$node->safe_psql('postgres', "DROP TABLE $tempschema.foo;");

Yeah, I agree. It can be (and will be) improved.

> DROP TABLE on a temporary relation for a superuser is a very useful
> behavior to unstuck autovacuum if a temp relation has been orphaned.
> It looks critical to me to explain that we want to keep this behavior
> for this reason.

Do you mean that we should add something to the documentation? Again, postgres
has always allowed DROP TABLE operation to be done for other temp tables, if
the user has enough privileges. We are disallowing everyone to look at other
temp tables' pages because there is no capability to do it. DROP TABLE doesn't
require access to backend-private data, so it is OK. Tom Lane wrote about it
here [2]. I want to say that described behavior is pretty natural. Why should
we additionally describe that the user can do operations that he is allowed
to do?

On the other hand, we have an error message that says "cannot access...", which
may look like every kind of "access" is forbidden. I bet that this is the place
that has confused you. More accurate error message would be "cannot access
pages..." or "cannot access content...". I think we can change our error
message in this way. What do you think?

We can also explicitly write in the documentation that users cannot access
*pages/content* of other temp tables. But the original patch [3] didn't do it,
so I am not sure that we should either.

> Using safe_psql() is not really adapted.  You should
> use a psql() where we check that the command does not fail, so as the
> test can generate a full report.

Fair enough.

> The test coverage actually has holes.  The three DML patterns INSERT,
> UPDATE and DELETE are covered, and it is missing MERGE.  Also, what
> about other patterns like ALTER TABLE, ALTER INDEX, ALTER FUNCTION,
> DROP FUNCTION and the like?
> There are many object patterns that can
> be schema-qualified, and none of this is covered.  What about the new
> REPACK and a bunch of other maintenance commands?  There is nothing
> about VACUUM, TRUNCATE, CLUSTER, etc.  Just to name a few.

I have concerns about it. If we try the "brute force" approach (i.e. test all
available commands), the test will increase unreasonably and will need constant
support. I guess that covering all available code paths that lead to reading
temp table's pages would be enough.

> As a whole, we were not really convinced that this is something that
> needs any kind of specific fix

It is a bug, obviously. TBH, I don't see any reason for not fixing it.

> especially not something that should
> be backpatched.

I couldn't answer for a long time, and Jim had already clearly demonstrated
why the backpatch is needed (for which I am grateful).


[1] https://www.postgresql.org/message-id/b13dc5ba-6c11-429c-b6fe-673c1c767bcf%40uni-muenster.de
[2] https://www.postgresql.org/message-id/4075754.1774378690%40sss.pgh.pa.us
[3] 948d6ec90fd35d6e1a59d0b1af8d6efd8442f0ad

--
Best regards,
Danill Davydov