Thread: Re: A assert failure when initdb with track_commit_timestamp=on

Hi,

On Wed, Jul 02, 2025 at 01:38:18AM +0000, Andy Fan wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> 
> Hi,
> 
> > On Wed, Jul 02, 2025 at 12:38:01AM +0000, Andy Fan wrote:
> >> However this is not true in BootstrapMode, this failure is masked by
> >> default because TransactionTreeSetCommitTsData returns fast when
> >> track_commit_timestamp is off.
> >
> > Agreed that there is no point in registering a commit timestamp in
> > the cases of a frozen and bootstrap XIDs.  I would recommend to keep
> > the assertion in TransactionIdSetCommitTs(), though, that still looks
> > useful to me for the many callers of this routine, at least as a
> > sanity check.
> 
> Yes, The assert also guard the InvalidTransactionId. So I removed this
> solution in v2. Another reason for this is: if we allowed
> BooststrapTransactionId in the commit_ts, it introduces something new to
> this module when initdb with track_commit_timestamp=on. This risk might
> be very low, but it can be avoided easily with the another solution. 
> 
> >
> > I did not check, but usually we apply filters based on
> > IsBootstrapProcessingMode() for code paths that we do not want to
> > reach while in bootstrap mode.  Could the same be done here?
> 
> I think you are right. so I used IsBootstrapProcessingMode in v2.

Thanks for the report and the patch.

Yeah, I also think that making use of IsBootstrapProcessingMode() is the
right thing to do here.

=== 1

+        * Don't bother to record commit_ts for Booststrap mode.

typo: s/Booststrap/Bootstrap/

Also, grep on "Bootstrap mode" and "bootstrap mode" gives much more occurrences
for the later, so maybe use "bootstrap mode" instead?

=== 2

The FrozenTransactionId case is missing in v2, is that on purpose?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Jul 02, 2025 at 09:03:35AM +0000, Bertrand Drouvot wrote:
> +        * Don't bother to record commit_ts for Booststrap mode.
>
> typo: s/Booststrap/Bootstrap/
>
> Also, grep on "Bootstrap mode" and "bootstrap mode" gives much more occurrences
> for the later, so maybe use "bootstrap mode" instead?
>
> === 2
>
> The FrozenTransactionId case is missing in v2, is that on purpose?

There may be an argument about bypassing the frozen case as well, but
I cannot get excited about that because it can also be very useful to
detect problems as calling this routine with a frozen XID would be
really wrong.  The assert in TransactionIdSetCommitTs() is enough for
this purpose, no need to add something else.  As of now
TransactionTreeSetCommitTsData() is only called with an active XID at
commit, but if somebody has too much imagination in a module..

As a whole I think that the addition of IsBootstrapProcessingMode() in
TransactionTreeSetCommitTsData() is enough.  There may be an argument
about backpatching this change.  However, nobody has complained about
the idea of enabling the commit_ts module while bootstrapping, so I'd
keep it as a HEAD-only thing and be conservative.

A second thing I would suggest is some test coverage, with the
addition of extra => [ '-c', "track_commit_timestamp=on=on" ] in an
init() method called in one of the script tests of
src/test/modules/commit_ts/t.  Just changing 001_base.pl should be
enough to trigger your original failure, making sure that this never
breaks again.
--
Michael