Fix bug with accessing to temporary tables of other sessions

First seen: 2025-04-14 05:36:15+00:00 · Messages: 54 · Participants: 8

Latest Update

2026-05-06 · opus 4.7

Fix bug with accessing to temporary tables of other sessions — Deep Analysis

1. The Core Problem

PostgreSQL has historically guarded an invariant rooted in a physical implementation restriction of the buffer manager: a backend cannot read another backend's temporary-table buffers. Temporary relations use local buffers (backend-private memory in localbuf.c), not shared buffers. There is simply no mechanism by which backend A can inspect backend B's local buffer pool, nor should there be — temp tables also rely on backend-local XIDs for MVCC visibility, so even if the pages were readable the answers would be meaningless/bogus.

For decades this was enforced by an explicit check in ReadBufferExtended() (now ReadBuffer_common()) that raised:

ERROR: cannot access temporary tables of other sessions

whenever a backend tried to read a page from a relation whose relpersistence == RELPERSISTENCE_TEMP but which did not belong to it (RELATION_IS_OTHER_TEMP).

The regression

Jim Jones traced the silent breakage to commit b7b0f3f2724 (sequential scans switched from ReadBufferExtended() to read_stream_next_buffer()), built on 210622c60e1 (StartReadBuffers / PinBufferForBlock) and b5a9b18cd0b (read_stream_begin_relation). The new streaming-I/O path bypasses the longstanding ReadBufferExtended() check. As a result, starting in PG17, queries such as

SELECT * FROM pg_temp_NN.t;   -- NN belongs to another backend
DELETE FROM pg_temp_NN.t;
UPDATE pg_temp_NN.t SET ...;
COPY  pg_temp_NN.t TO '...';

silently return 0 rows / 0 affected — the scan runs against an empty local buffer pool for a relation this backend knows nothing about. That is a correctness regression: users get results that are not just wrong but invisibly wrong (no error). Empty foreign temp tables had already lost the error in earlier versions (the check only fires when there are pages to read), but the regression made the silent-succeed case universal.

2. Solution Space Explored

Three distinct design axes were considered, and the debate between them is the intellectually interesting part of the thread.

Axis A — Parser-level fix (Davydov's original v1–v12)

Davydov's initial approach tagged the RangeVar with relpersistence in gram.y based on the pg_temp/pg_temp_NN schema prefix, then used the relpersistence field downstream to reject disallowed operations, with a new RVR_OTHER_TEMP_OK flag carving out DROP.

Axis B — ACL / permissions-level fix (Jim Jones' experimental patch)

Jim proposed handling this in aclchk.c as an access-control issue.

Axis C — Buffer-manager-level fix (Tom Lane's position, ultimately adopted)

Reinstate and extend the historical check at the lowest layer — where pages are actually read — so that every code path (including future ones) is covered.

Tom Lane's decisive intervention

Tom rejected both A and B in a single, sharply argued message (2026-03-23), and this effectively settled the design:

  1. Against ACL-level enforcement: "we do not enforce permissions checks against superusers." Bolting this onto aclchk.c would require contorting the ACL system and would inevitably leave bypass paths because superusers skip so many checks.
  2. Against parser-level enforcement: the relpersistence field on a RangeVar is fundamentally unreliable because a bare (unqualified) name cannot be classified as temp/permanent without a catalog lookup. Jim had already demonstrated the leak: SET search_path = pg_temp_81, public; SELECT * FROM t; bypasses the schema-prefix heuristic entirely. Tom went further: RangeVar.relpersistence arguably shouldn't exist; at minimum it means something different from relpersistence elsewhere, inviting bugs.
  3. The real cause is architectural, not policy-based: "If someday someone were to reimplement buffer management in a way that didn't have this implementation restriction, we would surely not arbitrarily restrict superusers from looking at tables that they then would physically be able to look at." The restriction exists because of a physical inability, not a policy decision. It must be enforced where the physical inability lives — the buffer manager.
  4. Defense-in-depth requirement: even C code can attempt to access another session's temp relation (e.g., extensions). The ACL system cannot police C-level callers of ReadBuffer; the bufmgr can and must.
  5. Back-patchability: a fix for v17/v18 must be small and non-invasive. The parser/ACL approaches were neither.

Tom's committer status plus the architectural clarity of the argument carried the day. Davydov explicitly conceded: "only operations with heap pages access must be forbidden, and the buffer manager layer is an appropriate place for it."

3. The Final Fix (v16→v19)

The accepted patch restores the invariant at two layered entry points into buffer reads:

3.1 read_stream_begin_relation() (new check)

if (RELATION_IS_OTHER_TEMP(rel))
    ereport(ERROR, ...
            errmsg("cannot access temporary relations of other sessions"));

This catches the streaming-I/O path that regressed in b7b0f3f2724 — covering heap sequential scans, COPY, and other read-stream consumers.

3.2 StartReadBuffersImpl() / ReadBuffer_common() (deeper check)

Because StartReadBuffers is a public vectored-read API that can be called without going through read_stream_begin_relation(), the same check is duplicated one layer deeper. This is the defense-in-depth point: third-party or future callers of the vectored buffer API can't inadvertently bypass the guard.

Davydov's rationale for the duplication: "if someone calls it bypassing read_stream_begin_relation (it is OK to do so), then our restriction will be violated again." Jim initially pushed back on double-checking but was persuaded once he saw that read_stream_begin_smgr_relation() can legitimately pass rel == NULL (and thus cannot do the check at the stream level for all callers).

3.3 What is not forbidden

Critically, the fix preserves three useful cross-session operations that do not read page contents:

This is exactly the "physical capability, not policy" framing Tom advocated: the line is drawn at heap-page access, nothing more.

4. Test Strategy and Michael Paquier's Review

The patch series was reorganized on Michael Paquier's advice (after his offsite review with colleagues) into two ordered patches:

This diff-based structure makes the behavioral change auditable at commit time. Korotkov accepted this restructuring and produced the final v19-style split, also removing the redundant ReadBufferExtended() check (kept only in ReadBuffer_common() to avoid double-checking the same buffer read).

Michael's broader concerns — lack of MERGE/ALTER/VACUUM/CLUSTER/TRUNCATE coverage, insufficient explanation of why DROP is intentionally preserved, and skepticism about back-patching — were partially addressed:

5. Key Technical Insights

5.1 relpersistence on RangeVar is semantically fraught

Tom's observation that a RangeVar's relpersistence cannot be truthful before catalog lookup is a subtle but important point. gram.y can set it based on a schema-qualification heuristic, but SET search_path = pg_temp_NN defeats any parser-side inference. Relying on this field for security-relevant decisions is structurally unsound. A future cleanup (out of scope here) could remove the field entirely from RangeVar.

5.2 Streaming I/O refactors need invariant audits

The b7b0f3f2724 regression is a cautionary tale: the bufmgr invariant "no cross-session temp reads" was enforced at ReadBufferExtended, and silently moving seqscans to a new code path lost it. Whenever a core read primitive is replaced with a new abstraction layer (read_stream_*, StartReadBuffers), invariants from the old primitive must be explicitly migrated or re-asserted. Adding the check at both read_stream_begin_relation and StartReadBuffersImpl reflects this lesson — it anchors the invariant at the primitive that will still be there no matter what higher-level abstraction comes next.

5.3 The "physical vs. policy" distinction

The conceptual crux is Tom's: this is not a permission. Superusers are not being told "you can't look"; they are being told "there is nothing for you to look at, and the backend that has it has its own private window." Modeling this at the ACL layer would wrongly suggest policy flexibility where none exists. Modeling it at the parser layer would wrongly suggest the question is answerable pre-catalog-lookup. Only the buffer manager sits where the actual physical limitation lives.

5.4 Why DROP is a genuine exception, not an inconsistency

DropRelationBuffers() does not read pages; it invalidates/forgets them. The operational case for keeping cross-session DROP is concrete: autovacuum must be able to remove orphaned temp tables left behind by crashed backends, and human operators occasionally need the same hammer. The fix draws the correct line — reads forbidden, metadata/drop/lock allowed — because those are the operations that physically work.

5.5 Empty-table edge case

Jim noted an interesting wrinkle: even pre-regression PG14/15/16 returned 0 rows (no error) for an empty foreign temp table, because with zero pages there was no ReadBuffer call to trigger the check. Arguably the error should fire at scan setup for consistency, but the fix does not address this (it would require a check outside the buffer read path). The accepted scope is "restore the pre-b7b0f3f2724 behavior", not "improve upon it".

6. Resolution

Korotkov (committer) announced intent to push and back-patch, with the final patch structure being:

The error message wording ("tables" vs "relations") was deliberately kept as the legacy "cannot access temporary tables of other sessions" for back-patchability and translator burden, with any rewording deferred to a separate follow-up.