Thread: Re: A assert failure when initdb with track_commit_timestamp=on
On 2025/07/03 22:31, Andy Fan wrote:
> "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com> writes:
>
>> Dear Michael, Fujii-san,
>>
>>> Ah, indeed, so it was reported a couple of months ago. I am not sure
>>> that the argument about all the other GUCs potentially impacted holds
>>> much value; we are talking about a specific code path.
>>
>> Yeah, I did report but sadly it was missed by others :-(. To clarify,
>> The current patch looks good to me.
>
> Then I'd thank Michael to watch the maillist closely this time.
>
> I checked the fix suggested by Hayato, I think his patch is better than
> me because his patch checks at the startup time while my patch checks at
> each time of RecordTransactionCommit. So v3 takes his patch. v3 also
> added the testcase suggested by Michael for test coverage, it clearly
> proves the bug is fixed now.
The patch looks good to me.
By the way, although it's a separate issue, I noticed that running
initdb -c transaction_timeout=1 causes an assertion failure:
running bootstrap script ... TRAP: failed Assert("all_timeouts_initialized"), File: "timeout.c", Line: 164, PID: 22057
0 postgres 0x00000001105d9d02 ExceptionalCondition + 178
1 postgres 0x0000000110612af7 enable_timeout + 55
2 postgres 0x0000000110612aa9 enable_timeout_after + 73
3 postgres 0x000000010fead8e0 StartTransaction + 816
4 postgres 0x000000010fead4a1 StartTransactionCommand + 65
5 postgres 0x000000010fef01de BootstrapModeMain + 1518
6 postgres 0x0000000110167ef4 main + 676
7 dyld 0x00007ff805092530 start + 3056
child process was terminated by signal 6: Abort trap: 6
This happens because enable_timeout() tries to activate the transaction
timeout before InitializeTimeouts() has been called — in other words,
the timeout system hasn't been initialized yet. To fix this, we might
need to forcibly set transaction_timeout to 0 during bootstrap.
Regards,
--
Fujii Masao
NTT DATA Japan Corporation
On Fri, Jul 04, 2025 at 01:26:19AM +0900, Fujii Masao wrote: > On 2025/07/03 22:31, Andy Fan wrote: >> I checked the fix suggested by Hayato, I think his patch is better than >> me because his patch checks at the startup time while my patch checks at >> each time of RecordTransactionCommit. So v3 takes his patch. v3 also >> added the testcase suggested by Michael for test coverage, it clearly >> proves the bug is fixed now. > > The patch looks good to me. I was wondering if we should backpatch that, and decided towards a yes here as it can be annoying. 3e51b278db6a has introduced the -c switch in initdb in v16, but that could be reached as well if one touches at some initialization path, perhaps in a fork. Done, down to v13. Let's tackle the rest separately. -- Michael
Attachment
Fujii Masao <masao.fujii@oss.nttdata.com> writes:
> On 2025/07/04 16:29, Hayato Kuroda (Fujitsu) wrote:
>> If more GUCs were found which cannot be set during the bootstrap mode, how about
>> introducing a new flag like GUC_DEFAULT_WHILE_BOOTSTRAPPING for GUC variables?
>> If the flag is set all setting can be ignored when
>> IsBootstrapProcessingMode() = true.
> If there are many GUCs that behave incorrectly during bootstrap,
> a general mechanism like that might be worth considering. But if
> only a few GUCs are affected, as I believe is the case,
> then such a mechanism may be overkill.
As I remarked in the other thread, I don't like inventing a different
solution for each GUC. So if there are even two that need something
done, I think Hayato-san's idea has merit.
The core of the patch could be as little as
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 667df448732..43f289924e6 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3464,6 +3464,15 @@ set_config_with_handle(const char *name, config_handle *handle,
return 0;
}
+ /*
+ * Certain GUCs aren't safe to enable during bootstrap mode. Silently
+ * ignore attempts to set them to non-default values.
+ */
+ if (unlikely(IsBootstrapProcessingMode()) &&
+ (record->flags & GUC_IGNORE_IN_BOOTSTRAP) &&
+ source != PGC_S_DEFAULT)
+ changeVal = false;
+
/*
* Check if the option can be set at this time. See guc.h for the precise
* rules.
If we went this way, we'd presumably revert 5a6c39b6d in favor
of marking track_commit_timestamp with this flag.
regards, tom lane
On 2025/07/05 0:30, Tom Lane wrote: > Fujii Masao <masao.fujii@oss.nttdata.com> writes: >> On 2025/07/04 16:29, Hayato Kuroda (Fujitsu) wrote: >>> If more GUCs were found which cannot be set during the bootstrap mode, how about >>> introducing a new flag like GUC_DEFAULT_WHILE_BOOTSTRAPPING for GUC variables? >>> If the flag is set all setting can be ignored when >>> IsBootstrapProcessingMode() = true. > >> If there are many GUCs that behave incorrectly during bootstrap, >> a general mechanism like that might be worth considering. But if >> only a few GUCs are affected, as I believe is the case, >> then such a mechanism may be overkill. > > As I remarked in the other thread, I don't like inventing a different > solution for each GUC. So if there are even two that need something > done, I think Hayato-san's idea has merit. > > The core of the patch could be as little as > > diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c > index 667df448732..43f289924e6 100644 > --- a/src/backend/utils/misc/guc.c > +++ b/src/backend/utils/misc/guc.c > @@ -3464,6 +3464,15 @@ set_config_with_handle(const char *name, config_handle *handle, > return 0; > } > > + /* > + * Certain GUCs aren't safe to enable during bootstrap mode. Silently > + * ignore attempts to set them to non-default values. > + */ > + if (unlikely(IsBootstrapProcessingMode()) && > + (record->flags & GUC_IGNORE_IN_BOOTSTRAP) && > + source != PGC_S_DEFAULT) > + changeVal = false; > + > /* > * Check if the option can be set at this time. See guc.h for the precise > * rules. This code seems to assume that the processing mode is switched to bootstrap before GUC parameters are processed. But is that actually the case? Regards, -- Fujii Masao NTT DATA Japan Corporation