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

Hi, Michael!

On Wed, Apr 22, 2026 at 2: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
>
> 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.
>
> 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;");
>
> 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.  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.
>
> 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.
>
> Another question is how do we make sure that new command patterns
> follow the same rule as what is enforced here.  It looks like this
> should be at least documented somewhere to be less surprising, but the
> patch does nothing of the kind.
>
> As a whole the patch lacks documentation, comments, and explanations,
> making it difficult to act on.
>
> As a whole, we were not really convinced that this is something that
> needs any kind of specific fix, especially not something that should
> be backpatched.
>
> After saying all that, there is some value in what you are doing here:
> it is true that we lack test coverage in terms of interactions of
> temporary objects across multiple sessions, and that we should have
> some.  TAP is adapted for this purpose, isolation tests could be an
> extra one but the schema names make that unpredictible in output.  The
> patch unfortunately does a poor job in showing what it wants to
> change.  One thing that I would suggest is to *reverse* the order of
> the patches:
> - First have a patch that introduces new tests, that shows the
> original behavior.  This needs to be more complete in terms of command
> patterns.  The DROP TABLE is one case that we want to keep.  This
> should be kept as-is, and it is critical to document the reason why we
> want to keep things this way (aka autovacuum and orphaned tables,
> AFAIK).
> - Then implement the second patch that updates the tests introduced in
> the first patch, so as one can track *what* has changed, and so as one
> does not have to test manually what the original behavior was.
>
> As a whole, this patch needs more work, based on what has been
> currently posted on the lists.  That's not ready yet.  The backpatch
> question is a separate matter.

Thank you for your feedback.  I think the scope of this patch is well
described in [1].  We don't want to restrict the superuser from
something, but our buffer manager just technically can't access the
local buffers of other sessions.  Read streams introduced a new code
path for reading relations, which was lacking of the proper check for
local buffers of other sessions.  And this patch attempts to fix that.
DROP TABLE is an exclusion.  It actually don't need to read contents
of buffers, just drop them.  And DropRelationBuffers() have a special
exclusion for this case.  So, DROP TABLE appears to be the only
operation that makes sense, it's a conscious exclusion, and there is
no intention to forbid it.

I've revised the patch.  0001 contains tests and states the current
behavior.  0002 contains fix and the corresponding changes in the
tests.  I made a change in 0001: removed the check in
ReadBufferExtended().  We added the same check to ReadBuffer_common(),
and I don't think it makes sense to do this check twice in the row.

Links.
1. https://www.postgresql.org/message-id/3529398.1774273446%40sss.pgh.pa.us

------
Regards,
Alexander Korotkov
Supabase