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

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.

Thanks,
--
Michael