Hi
On 02/05/2026 18:34, Alexander Korotkov wrote:
> On Sat, May 2, 2026 at 6:37 PM Daniil Davydov <3danissimo@gmail.com> wrote:
>> On Sat, May 2, 2026 at 9:16 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>>>
>>> Thank you for your feedback. I think the scope of this patch is well
>>> described in [1]. We don't want to restrict the superuser from
>>> something, but our buffer manager just technically can't access the
>>> local buffers of other sessions. Read streams introduced a new code
>>> path for reading relations, which was lacking of the proper check for
>>> local buffers of other sessions. And this patch attempts to fix that.
>>> DROP TABLE is an exclusion. It actually don't need to read contents
>>> of buffers, just drop them. And DropRelationBuffers() have a special
>>> exclusion for this case. So, DROP TABLE appears to be the only
>>> operation that makes sense, it's a conscious exclusion, and there is
>>> no intention to forbid it.
>>
>> Yep, exactly.
+1
>>> I've revised the patch. 0001 contains tests and states the current
>>> behavior. 0002 contains fix and the corresponding changes in the
>>> tests. I made a change in 0001: removed the check in
>>> ReadBufferExtended(). We added the same check to ReadBuffer_common(),
>>> and I don't think it makes sense to do this check twice in the row.
>>
>> Thank you! But I'm afraid that you forgot to attach the patches..
>
> Here they are.
Thanks for the comprehensive additional tests!
In addition to the DROP TABLE exception:
It is also possible to LOCK temporary tables from other sessions:
postgres=# BEGIN;
BEGIN
postgres=*# LOCK TABLE pg_temp_91.t IN ACCESS SHARE MODE ;
LOCK TABLE
pg_temp_91.t lives as long the transaction is open -- even after the
origin session closes, which is totally expected. I'd say it falls into
the same category of DROP TABLE, where the table contents are never
read, so I'd argue it's ok.
>
>> BTW, what do you think about this proposal? :
>>> On the other hand, we have an error message that says "cannot access...", which
>>> may look like every kind of "access" is forbidden. I bet that this is the place
>>> 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 easily include it in the first patch.
>
> This is possible, but I would keep that in a separate patch. We now
> have clear scope for both patches: 0001 includes additional tests,
> 0002 fixes the bug and restores old behavior.
+1 for a separate patch. I think the scope of the current patch is good
as-is.
Thanks!
Best, Jim
