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
