>Hi, > >On Sat, Jun 20, 2026 at 10:48?PM ZizhuanLiu X-MAN <[email protected]> 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) [email protected]
