public inbox for [email protected]help / color / mirror / Atom feed
Re: BUG #19438: segfault with temp_file_limit inside cursor 5+ messages / 2 participants [nested] [flat]
* Re: BUG #19438: segfault with temp_file_limit inside cursor @ 2026-03-30 00:15 David Rowley <[email protected]> 2026-03-30 00:34 ` Re: BUG #19438: segfault with temp_file_limit inside cursor Tom Lane <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: David Rowley @ 2026-03-30 00:15 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: [email protected]; [email protected] On Mon, 30 Mar 2026 at 12:51, Tom Lane <[email protected]> wrote: > > David Rowley <[email protected]> writes: > > I don't know if that means > > it's worth deviating from the similar WARNINGs you've added and making > > that one an ERROR. There's certainly no guarantee with the other > > context that we'll not crash sometime very soon after issuing the > > warning anyway, so maybe it's fine. > > Seems like a reasonable answer. What do you think of making the > double-free cases ERRORs across the board? If we don't error out, > there will likely be cascading problems in all the mcxt types not > just this one. I think it's a good idea. It might slightly increase the chances that we get a report about an issue. I suppose the logic in deciding which elevel to make it could be applied about equally to the sentinel byte check as well. Maybe that should also be an error for the same reason. David ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: BUG #19438: segfault with temp_file_limit inside cursor 2026-03-30 00:15 Re: BUG #19438: segfault with temp_file_limit inside cursor David Rowley <[email protected]> @ 2026-03-30 00:34 ` Tom Lane <[email protected]> 2026-03-30 01:09 ` Re: BUG #19438: segfault with temp_file_limit inside cursor David Rowley <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tom Lane @ 2026-03-30 00:34 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: [email protected]; [email protected] David Rowley <[email protected]> writes: > On Mon, 30 Mar 2026 at 12:51, Tom Lane <[email protected]> wrote: >> Seems like a reasonable answer. What do you think of making the >> double-free cases ERRORs across the board? If we don't error out, >> there will likely be cascading problems in all the mcxt types not >> just this one. > I think it's a good idea. It might slightly increase the chances that > we get a report about an issue. I suppose the logic in deciding which > elevel to make it could be applied about equally to the sentinel byte > check as well. Maybe that should also be an error for the same reason. I thought about that, but it's been a WARNING for a long time and I'm hesitant to change that. We've seen many cases where scribbling one or two bytes past the end of the requested size doesn't actually cause fatal problems, because that was padding or unused space anyway. Double frees are in a different category: if we let one happen, it's pretty much guaranteed to cause hard-to-decipher problems down the road. (The fact that that didn't happen in the particular case reported here doesn't mean it's usually okay.) regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: BUG #19438: segfault with temp_file_limit inside cursor 2026-03-30 00:15 Re: BUG #19438: segfault with temp_file_limit inside cursor David Rowley <[email protected]> 2026-03-30 00:34 ` Re: BUG #19438: segfault with temp_file_limit inside cursor Tom Lane <[email protected]> @ 2026-03-30 01:09 ` David Rowley <[email protected]> 2026-03-30 02:19 ` Re: BUG #19438: segfault with temp_file_limit inside cursor Tom Lane <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: David Rowley @ 2026-03-30 01:09 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: [email protected]; [email protected] On Mon, 30 Mar 2026 at 13:34, Tom Lane <[email protected]> wrote: > > David Rowley <[email protected]> writes: > > On Mon, 30 Mar 2026 at 12:51, Tom Lane <[email protected]> wrote: > >> Seems like a reasonable answer. What do you think of making the > >> double-free cases ERRORs across the board? If we don't error out, > >> there will likely be cascading problems in all the mcxt types not > >> just this one. > > > I think it's a good idea. It might slightly increase the chances that > > we get a report about an issue. I suppose the logic in deciding which > > elevel to make it could be applied about equally to the sentinel byte > > check as well. Maybe that should also be an error for the same reason. > > I thought about that, but it's been a WARNING for a long time and I'm > hesitant to change that. We've seen many cases where scribbling one > or two bytes past the end of the requested size doesn't actually cause > fatal problems, because that was padding or unused space anyway. > Double frees are in a different category: if we let one happen, > it's pretty much guaranteed to cause hard-to-decipher problems down > the road. (The fact that that didn't happen in the particular case > reported here doesn't mean it's usually okay.) Fair. Maybe worth a short comment in the code to explain why we don't use the same elevel then? Just considering someone stumbling upon the variation in the future and reporting or asking why, and us having to dig up the reason why in the archives to answer them. Maybe something like this? /* * Test for someone scribbling on unused space in chunk. Small * overwrites are less likely to cause issues than a double-free, so * warn for this instead of erroring. */ David ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: BUG #19438: segfault with temp_file_limit inside cursor 2026-03-30 00:15 Re: BUG #19438: segfault with temp_file_limit inside cursor David Rowley <[email protected]> 2026-03-30 00:34 ` Re: BUG #19438: segfault with temp_file_limit inside cursor Tom Lane <[email protected]> 2026-03-30 01:09 ` Re: BUG #19438: segfault with temp_file_limit inside cursor David Rowley <[email protected]> @ 2026-03-30 02:19 ` Tom Lane <[email protected]> 2026-03-30 16:29 ` Re: BUG #19438: segfault with temp_file_limit inside cursor Tom Lane <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tom Lane @ 2026-03-30 02:19 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: [email protected]; [email protected] David Rowley <[email protected]> writes: > Fair. Maybe worth a short comment in the code to explain why we don't > use the same elevel then? Just considering someone stumbling upon the > variation in the future and reporting or asking why, and us having to > dig up the reason why in the archives to answer them. > Maybe something like this? Works for me. I'm done for the day but will make it so tomorrow. regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: BUG #19438: segfault with temp_file_limit inside cursor 2026-03-30 00:15 Re: BUG #19438: segfault with temp_file_limit inside cursor David Rowley <[email protected]> 2026-03-30 00:34 ` Re: BUG #19438: segfault with temp_file_limit inside cursor Tom Lane <[email protected]> 2026-03-30 01:09 ` Re: BUG #19438: segfault with temp_file_limit inside cursor David Rowley <[email protected]> 2026-03-30 02:19 ` Re: BUG #19438: segfault with temp_file_limit inside cursor Tom Lane <[email protected]> @ 2026-03-30 16:29 ` Tom Lane <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Tom Lane @ 2026-03-30 16:29 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: [email protected]; [email protected] I wrote: > Works for me. I'm done for the day but will make it so tomorrow. Pushed that, though in the event I wrote a rather longer apologia for why we use ERROR in one complaint and WARNING in the other. Getting back to the original bug ... I went through tuplestore.c and found two other places where foreseeable failures in subroutines could leave the tuplestore in an inconsistent state. I think we need to fix them all, per the attached draft against HEAD. The back branches might have more/different bugs, didn't look yet. regards, tom lane Attachments: [text/x-diff] v2-fix-tuplestore-inconsistency-after-error.patch (1.9K, 2-v2-fix-tuplestore-inconsistency-after-error.patch) download | inline diff: diff --git a/src/backend/utils/sort/tuplestore.c b/src/backend/utils/sort/tuplestore.c index caad7cad0b4..f9e2d95186a 100644 --- a/src/backend/utils/sort/tuplestore.c +++ b/src/backend/utils/sort/tuplestore.c @@ -708,10 +708,10 @@ grow_memtuples(Tuplestorestate *state) /* OK, do it */ FREEMEM(state, GetMemoryChunkSpace(state->memtuples)); - state->memtupsize = newmemtupsize; state->memtuples = (void **) repalloc_huge(state->memtuples, - state->memtupsize * sizeof(void *)); + newmemtupsize * sizeof(void *)); + state->memtupsize = newmemtupsize; USEMEM(state, GetMemoryChunkSpace(state->memtuples)); if (LACKMEM(state)) elog(ERROR, "unexpected out-of-memory situation in tuplestore"); @@ -1306,7 +1306,19 @@ dumptuples(Tuplestorestate *state) if (i >= state->memtupcount) break; WRITETUP(state, state->memtuples[i]); + + /* + * Increase memtupdeleted to track the fact that we just deleted that + * tuple. Think not to remove this on the grounds that we'll reset + * memtupdeleted to zero below. We might not reach that if some later + * WRITETUP fails (e.g. due to overrunning temp_file_limit). If so, + * we'd error out leaving an effectively-corrupt tuplestore, which + * would be quite bad if it's a persistent data structure such as a + * Portal's holdStore. + */ + state->memtupdeleted++; } + /* Now we can reset memtupdeleted along with memtupcount */ state->memtupdeleted = 0; state->memtupcount = 0; } @@ -1496,8 +1508,10 @@ tuplestore_trim(Tuplestorestate *state) FREEMEM(state, GetMemoryChunkSpace(state->memtuples[i])); pfree(state->memtuples[i]); state->memtuples[i] = NULL; + /* As in dumptuples(), increment memtupdeleted synchronously */ + state->memtupdeleted++; } - state->memtupdeleted = nremove; + Assert(state->memtupdeleted == nremove); /* mark tuplestore as truncated (used for Assert crosschecks only) */ state->truncated = true; ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2026-03-30 16:29 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-03-30 00:15 Re: BUG #19438: segfault with temp_file_limit inside cursor David Rowley <[email protected]> 2026-03-30 00:34 ` Tom Lane <[email protected]> 2026-03-30 01:09 ` David Rowley <[email protected]> 2026-03-30 02:19 ` Tom Lane <[email protected]> 2026-03-30 16:29 ` Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox