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)



Reply via email to