Re: ResourceOwner refactoring

On 02/02/2024 11:00, Alexander Lakhin wrote:
> Please try the following script:
> mkdir /tmp/50m
> sudo mount -t tmpfs -o size=50M tmpfs /tmp/50m
> export PGDATA=/tmp/50m/tmpdb
> 
> initdb
> pg_ctl -l server.log start
> 
> cat << 'EOF' | psql
> CREATE TEMP TABLE t (a name, b name, c name, d name);
> INSERT INTO t SELECT 'a', 'b', 'c', 'd' FROM generate_series(1, 1000) g;
> 
> COPY t TO '/tmp/t.data';
> SELECT 'COPY t FROM ''/tmp/t.data''' FROM generate_series(1, 100)
> \gexec
> EOF
> 
> which produces an unexpected error, a warning, and an assertion failure,
> starting from b8bff07da:

Fixed, thanks for the report!

Comparing ExtendBufferedRelLocal() and ExtendBufferedRelShared(), it's 
easy to see that ExtendBufferedRelLocal() was missing a 
ResourceOwnerEnlarge() call in the loop. But it's actually a bit more 
subtle: it was correct without the ResourceOwnerEnlarge() call until 
commit b8bff07da, because ExtendBufferedRelLocal() unpins the old buffer 
pinning the new one, while ExtendBufferedRelShared() does it the other 
way 'round. The implicit assumption was that unpinning the old buffer 
ensures that you can pin a new one. That no longer holds with commit 
b8bff07da. Remembering a new resource expects there to be a free slot in 
the fixed-size array, but if the forgotten resource was in the hash, 
rather than the array, forgetting it doesn't make space in the array.

We also make that assumption here in BufferAlloc:

>         /*
>          * Got a collision. Someone has already done what we were about to do.
>          * We'll just handle this as if it were found in the buffer pool in
>          * the first place.  First, give up the buffer we were planning to
>          * use.
>          *
>          * We could do this after releasing the partition lock, but then we'd
>          * have to call ResourceOwnerEnlarge() & ReservePrivateRefCountEntry()
>          * before acquiring the lock, for the rare case of such a collision.
>          */
>         UnpinBuffer(victim_buf_hdr);

It turns out to be OK in that case, because it unpins the buffer that 
was the last one pinned. That does ensure that you have one free slot in 
the array, but forgetting anything other than the most recently 
remembered resource does not.

I've added a note to that in ResourceOwnerForget. I read through the 
other callers of ResourceOwnerRemember and PinBuffer, but didn't find 
any other unsafe uses. I'm not too happy with this subtlety, but at 
least it's documented now.

-- 
Heikki Linnakangas
Neon (https://neon.tech)