Subject: Re: Use generation memory context for tuplestore.c

From: Alexander Lakhin <[email-masked]>
Date: Fri, 5 Jul 2024 17:00:00 +0300

05.07.2024 07:57, David Rowley wrote:
> Thanks for the report. I've just pushed a fix in 53abb1e0e.

Thank you, David!

Please look at another anomaly introduced with 590b045c3:
echo "
CREATE TABLE t(f int, t int);
INSERT INTO t VALUES (1, 1);

WITH RECURSIVE sg(f, t) AS (
SELECT * FROM t t1
UNION ALL
SELECT t2.* FROM t t2, sg WHERE t2.f = sg.t
) SEARCH DEPTH FIRST BY f, t SET seq
SELECT * FROM sg;
" | timeout 60 psql

triggers
TRAP: failed Assert("chunk->requested_size < oldsize"), File: "generation.c", Line: 842, PID: 830294

Best regards,
Alexander

---

From: David Rowley <[email-masked]>
Date: Sat, 6 Jul 2024 12:08:50 +1200

> TRAP: failed Assert("chunk->requested_size < oldsize"), File: "generation.c", Line: 842, PID: 830294

This seems to be a bug in GenerationRealloc().

What's happening is when we palloc(4) for the files array in
makeBufFile(), that palloc uses GenerationAlloc() and since we have
MEMORY_CONTEXT_CHECKING, the code does:

/* ensure there's always space for the sentinel byte */
chunk_size = MAXALIGN(size + 1);

resulting in chunk_size == 8.  When extendBufFile() effectively does
the repalloc(file->files, 8), we call GenerationRealloc() with those 8
bytes and go into the "if (oldsize >= size)" path thinking we have
enough space already.  Here both values are 8, which would be fine on
non-MEMORY_CONTEXT_CHECKING builds, but there's no space for the
sentinel byte here. set_sentinel(pointer, size) stomps on some memory,
but no crash from that.  It's only a problem when extendBufFile() asks
for 12 bytes that we come back into GenerationRealloc() and trigger
the Assert(chunk->requested_size < oldsize). Both of these values are
8. The oldsize should have been large enough to store the sentinel
byte, it isn't due to the problem caused during GenerationRealloc with
8 bytes.

I also had not intended that the buffile.c stuff would use the
generation context.  I'll need to fix that too, but I think I'll fix
the GenerationRealloc() first.

The attached fixes the issue. I'll stare at it a bit more and try to
decide if that's the best way to fix it.

David

---

From: David Rowley <[email-masked]>
Date: Sat, 6 Jul 2024 14:05:46 +1200

> decide if that's the best way to fix it.

I did that staring work and thought about arranging the code to reduce
the number of #ifdefs. In the end, I decided it was better to keep the
two "if" checks close together so that it's easy to see that one does
> and the other >=. Someone else's tastes may vary.

I adjusted the comment to remove the >= mention and pushed the
resulting patch and backpatched to 16, where I broke this in
0e480385e.

Thanks for the report.

David

David

---

From: David Rowley <[email-masked]>
Date: Sat, 6 Jul 2024 17:41:21 +1200

> the GenerationRealloc() first.

I've pushed a fix for that now too.

David