| From: | ZizhuanLiu X-MAN <44973863(at)qq(dot)com> |
|---|---|
| To: | Daniil Davydov <3danissimo(at)gmail(dot)com> |
| Cc: | Jim Jones <jim(dot)jones(at)uni-muenster(dot)de>, pgsql-hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: BUG with accessing to temporary tables of other sessions still exists |
| Date: | 2026-06-21 08:08:55 |
| Message-ID: | tencent_9CF0C26DD2B44713FCDF6C9634D4158D9908@qq.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
>Hi,
>
>On Sat, Jun 20, 2026 at 10:48?PM ZizhuanLiu X-MAN <44973863(at)qq(dot)com> wrote:
>>
>> Just as described in the comment of RELATION_IS_OTHER_TEMP macro in rel.h,
>> existing buffer manager routines including ReadBuffer_common(), StartReadBuffersImpl()
>> and PrefetchBuffer() have already invoked this macro to check cross-session temporary
>> table access. All these functions are located in bufmgr.c.
>>
>> For code consistency, I suggest adding the same RELATION_IS_OTHER_TEMP check in
>> ExtendBufferedRelCommon() (also in bufmgr.c), right before calling ExtendBufferedRelLocal().
>
>Thank you for your feedback!
>
>Do you mean that this check should be moved to "ExtendBufferedRelCommon"
>because this function is in the bufmgr.c file? If so, I don't see the point in
>that. Buffer manager is not only the bufmgr.c file, but also all the files in
>the "storage/buffer" folder. So we can say that localbuf.c also contains logic
>related to the buffer manager.
>
>Moreover, ExtendBufferedRelCommon should contain only the logic that is really
>common for both ordinary and temp tables. All other specific code is
>encapsulated inside the ExtendBufferedRelLocal and ExtendBufferedRelShared. It
>allows us to keep the ExtendBufferedRelCommon function pretty short.
>
>So, I suggest leaving RELATION_IS_OTHER_TEMP as it is.
>
>> Meanwhile, we should also update the comment of RELATION_IS_OTHER_TEMP
>> accordingly to keep the documentation synchronized with code changes.
>
>This comment is already updated - it now mentions the ExtendBufferedRelLocal
>function.
>
>--
>Best regards,
>Daniil Davydov
Hi,Danii
Thank you very much for your feedback, as well as the original patch and test cases.
I have learned a lot about the internals of temporary tables from this discussion.
First, I would like to clarify that the buffer manager is not limited to `bufmgr.c` alone,
but covers all source files under the `storage/buffer` directory.
I’ve analyzed the existing architecture and the relationship between `bufmgr.c`,
the `RELATION_IS_OTHER_TEMP` macro and `localbuf.c`, and my observations
are summarised below:
1. Within the `storage/buffer` module, `RELATION_IS_OTHER_TEMP` is currently referenced
in three places, all of which reside in `bufmgr.c`; there are no usages in `localbuf.c` at present.
2. Looking at the function call graph between `bufmgr.c` and `localbuf.c`, most routines in
`localbuf.c` serve as helper functions invoked from `bufmgr.c`. (A tiny number of calls exist
within `src/test/modules/test_io.c`, which I believe can be disregarded for this discussion.)
3. Under the current design, `localbuf.c` acts as a callee layer focused solely on implementing
core logic without introducing `RELATION_IS_OTHER_TEMP` checks, keeping this module clean.
All access validation decisions using `RELATION_IS_OTHER_TEMP` are handled upstream in
`bufmgr.c` as the main caller.
4. Placing the `RELATION_IS_OTHER_TEMP` check inside `ExtendBufferedRelCommon()` in `bufmgr.c`
(rather than `ExtendBufferedRelLocal()` in `localbuf.c`) aligns with this existing architectural principle:
validation rules are enforced by the caller layer. Admittedly, this approach has a downside — if new
caller entrypoints are added in the future, each would need to remember to perform this security check.
Fortunately, all relevant call paths currently originate from `bufmgr.c`.
Please feel free to point out any flaws or oversights in my reasoning.
Below is a partial summary of the call relationships among `bufmgr.c`, `RELATION_IS_OTHER_TEMP`
and `localbuf.c` (this is not an exhaustive list):
PrefetchBuffer()
--RELATION_IS_OTHER_TEMP + PrefetchLocalBuffer() /* pass it off to localbuf.c */
--PrefetchSharedBuffer()
ReadBuffer_common()
RELATION_IS_OTHER_TEMP ExtendBufferedRel()==>ExtendBufferedRel()===>ExtendBufferedRelBy()==>ExtendBufferedRelCommon()==>ExtendBufferedRelCommon()
PinBufferForBlock()
StartReadBuffer()==>StartReadBuffersImpl()
WaitReadBuffers()
static ExtendBufferedRelCommon()
--if bmr.relpersistence == RELPERSISTENCE_TEMP call ExtendBufferedRelLocal() /* @localbuf.c */
-- else ExtendBufferedRelShared()
static StartReadBuffersImpl()
RELATION_IS_OTHER_TEMP
static PinBufferForBlock()
if (persistence == RELPERSISTENCE_TEMP) LocalBufferAlloc() /* @localbuf.c */
else BufferAlloc()
FlushRelationBuffers()
PinLocalBuffer() /* @localbuf.c */
FlushLocalBuffer() /* @localbuf.c */
UnpinLocalBuffer /* @localbuf.c */
MarkBufferDirty()
MarkLocalBufferDirty() /* @localbuf.c */
MarkBufferDirtyHint()
MarkLocalBufferDirty() /* @localbuf.c */
BufferSetHintBits16()
MarkLocalBufferDirty() /* @localbuf.c */
ZeroAndLockBuffer()
StartLocalBufferIO() /* @localbuf.c */
TerminateLocalBufferIO() /* @localbuf.c */
StartBufferIO()
StartLocalBufferIO() /* @localbuf.c */
buffer_readv_complete_one()
TerminateLocalBufferIO() /* @localbuf.c */
regards,
--
ZizhuanLiu (X-MAN)
44973863(at)qq(dot)com
| From | Date | Subject | |
|---|---|---|---|
| Next Message | ZizhuanLiu X-MAN | 2026-06-21 08:25:46 | Re: BUG with accessing to temporary tables of other sessions still exists |
| Previous Message | Zsolt Parragi | 2026-06-21 07:57:45 | Re: PG20 Minimum Dependency Thread |