Re: We broke the defense against accessing other sessions' temp tables

On Mon, Sep 22, 2025 at 5:33 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> In session 1:
>
> regression=# create temp table tt(f1 int);
> CREATE TABLE
> regression=# insert into tt values(44);
> INSERT 0 1
> regression=# \d tt
>                Table "pg_temp_77.tt"
>  Column |  Type   | Collation | Nullable | Default
> --------+---------+-----------+----------+---------
>  f1     | integer |           |          |
>
> In session 2:
>
> regression=# select * from pg_temp_77.tt; -- use the schema shown in session 1
>  f1
> ----
> (0 rows)
>
> Branches before 17 show the expected failure
>
> ERROR:  cannot access temporary tables of other sessions
>
> but that's gone missing :-(.  Apparently, we refactored things enough
> that ReadBufferExtended is not used for a simple seqscan, and since
> that's where the check for an other-session temp table is, we get
> no error and instead bogus results.  I did not bother to bisect to
> find a culprit commit, but given that the failure manifests in 17,
> I'm betting the stream I/O stuff is at fault.

That was me, and it looks like I managed to miss the earlier reports
and analysis.  Oops.

> I experimented with moving the check into PinBufferForBlock, and
> that seems to work correctly, but I wonder if someone who's messed
> with this code more recently than I would prefer a different
> solution.

This looks logically correct at first glance, but I guess it would be
nice to perform the check just once for a stream instead of once per
block.  Each stream only accesses one relation for its whole life*.

That leads to the idea of leaving the check where it is in
ReadBufferExtended() for non-stream code paths, but (1) duplicating it
in read_stream_begin_relation(), (2) documenting in a comment above
StartReadBuffersImpl() that it's the caller's responsibility not to
try to read buffers in other backends' temporary tables, and (3)
turning that proposed check in PinBufferForBlock() into an assertion
just to catch programming errors.  I can try that soon if you think
that makes sense, unless someone else wants to.

An alternative would be to perform the check somewhere in
StartReadBuffers(), so that at least it's performed only once over
sequential ranges up to io_combine_limit.  I don't like that idea: I
don't currently expect other users of StartReadBuffers() to sprout, so
I'm not sure an ereport defence is warranted at that level, at the
cost of extra code size and cycles in this path.  We could always
reconsider that if I turn out to be wrong in future development, but
for now StartReadBuffers() is basically plumbing that allows a
modularity border between buffer manager concerns and streaming
concerns, and it seems OK to write down what the contract is and check
it with an assertion, and then perform user-face ereport checks at the
most efficient spot.

Thinking bigger about the scope of the check, I wondered if it could
make sense to block other backends' relations from being opened in the
first place.  Reading the older threads, I see that that was already
being discussed[1] by Andres and Andrey (well they didn't say "open"
but I guess that's what they meant?) in the context of cross-process
DROP, which would presumably need a special I-know-what-I'm-doing flag
to escape that check.  Then I guess if we had the above-mentioned
assertion, if a caller passed the escape hatch flag but doesn't in
fact know what it's doing, at least that'd fail.  Something to think
about for master maybe.

*FWIW, one of my prototypes for true AIO in recovery can switch
between (smgr) relations in one stream, but there are no temporary
relations in the picture in that context.

[1]
https://www.postgresql.org/message-id/flat/4xxau766dofbwugeyvjftra3g5f7ifaal2clgrbpr7jqotr4av%40d3ige2krpoza#3c0ef03be42942dcb7e4c305abb20ad7