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