MIME-Version: 1.0
References: <e58a9580-7bbf-4d70-ad99-833fab77cf88@uni-muenster.de>
 <CAJDiXghBO_TqvHOSui8MOxiFmwLT20+SAnH5nW1rpWHk7Jwffg@mail.gmail.com>
 <402bbc8d-728b-4467-8024-31c2bc101ead@uni-muenster.de>
 <CAJDiXgi0JFk0f2KWWQkzLBC5P7erX9WP18qqnbi-rjZ-K-P=3w@mail.gmail.com>
 <CAMtXxw_hBNuAWQUdSRMpoeRVRYr+5+S7p0bSzuqtHxfpzJPd3w@mail.gmail.com>
 <b6614954-6c71-451a-a1d7-345d255b4afb@uni-muenster.de>
 <CAMtXxw_2VnP5U7U+2ObOCC2voEkAzUqAgH=rPTQBXrb_8K=pyA@mail.gmail.com>
 <CAPpHfdtsGSywUExfKMakAuRz8-+61d-4XHkVf=pNQeYTZTVYoA@mail.gmail.com>
 <CAMtXxw8J7L0r8aXt9NBFfqqSnaNyU=-_Uq4tW4aO3PHyVeXLAQ@mail.gmail.com>
 <CAPpHfdvLCgXgdhSvuPCkek+r=P4E-WiNceWfh7UQztiMzfFcvg@mail.gmail.com>
 <aegLPbRmc95UbOZG@paquier.xyz>
In-Reply-To: <aegLPbRmc95UbOZG@paquier.xyz>
From: Daniil Davydov <3danissimo@gmail.com>
Date: Sat, 25 Apr 2026 15:34:22 +0700
Message-ID: 
 <CAJDiXgim8OjxuWZ2EYs2nV+1kTcACH2cNURgzyCye8301PmR0w@mail.gmail.com>
Subject: Re: Fix bug with accessing to temporary tables of other sessions
To: Michael Paquier <michael@paquier.xyz>
Cc: Alexander Korotkov <aekorotkov@gmail.com>,
 Soumya S Murali <soumyamurali.work@gmail.com>,
	Jim Jones <jim.jones@uni-muenster.de>, Tom Lane <tgl@sss.pgh.pa.us>,
	Stepan Neretin <slpmcf@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
	Mohamed Ali <moali.pg@gmail.com>, Nazneen Jafri <jafrinazneen@gmail.com>,
	Shawn McCoy <shawn.the.mccoy@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAJDiXgim8OjxuWZ2EYs2nV%2B1kTcACH2cNURgzyCye8301PmR0w%40mail.gmail.com>
Precedence: bulk

Hi,

On Wed, Apr 22, 2026 at 6:41=E2=80=AFAM Michael Paquier <michael@paquier.xy=
z> 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-345d255b4af=
b@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 scan=
s.
Jim wrote about it here [1]. You can also find this in the commit message o=
f
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 comme=
nts
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, postg=
res
has always allowed DROP TABLE operation to be done for other temp tables, i=
f
the user has enough privileges. We are disallowing everyone to look at othe=
r
temp tables' pages because there is no capability to do it. DROP TABLE does=
n't
require access to backend-private data, so it is OK. Tom Lane wrote about i=
t
here [2]. I want to say that described behavior is pretty natural. Why shou=
ld
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...", w=
hich
may look like every kind of "access" is forbidden. I bet that this is the p=
lace
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 a=
ll
available commands), the test will increase unreasonably and will need cons=
tant
support. I guess that covering all available code paths that lead to readin=
g
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-673c1c767=
bcf%40uni-muenster.de
[2] https://www.postgresql.org/message-id/4075754.1774378690%40sss.pgh.pa.u=
s
[3] 948d6ec90fd35d6e1a59d0b1af8d6efd8442f0ad

--
Best regards,
Danill Davydov