From: Andy Fan <zhihuifan1213@163.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: A assert failure when initdb with track_commit_timestamp=on
In-Reply-To: <aGSBcvlXY9FDkcsr@paquier.xyz> (Michael Paquier's message of
	"Wed, 2 Jul 2025 09:46:42 +0900")
References: <87plejmnpy.fsf@163.com> <aGSBcvlXY9FDkcsr@paquier.xyz>
Date: Wed, 02 Jul 2025 01:38:18 +0000
Message-ID: <87ikkbmkxh.fsf@163.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="=-=-="
Archived-At: <https://www.postgresql.org/message-id/87ikkbmkxh.fsf%40163.com>
Precedence: bulk

--=-=-=
Content-Type: text/plain

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.


-- 
Best Regards
Andy Fan


--=-=-=
Content-Type: text/x-diff
Content-Disposition: attachment;
 filename=v2-0001-Don-t-record-commit_ts-in-bootstarp-mode.patch

From def54f2c6a73aec74aebde9b687612f65a8bcdd5 Mon Sep 17 00:00:00 2001
From: Andy Fan <zhihuifan1213@163.com>
Date: Wed, 2 Jul 2025 01:23:34 +0000
Subject: [PATCH v2 1/1] Don't record commit_ts in bootstarp mode.

This case would raise Assert failure in TransactionIdSetCommitTs  when
initdb with track_commit_timestamp=on.

Another option might be allowing BooststrapTransactionId in
TransactionIdSetCommitTs, but this adds a new change in
commit_ts module, which risk could be avoided easily with current
solution.
---
 src/backend/access/transam/commit_ts.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index 113fae1437a..1aa2dc450f5 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -157,6 +157,12 @@ TransactionTreeSetCommitTsData(TransactionId xid, int nsubxids,
 	if (!commitTsShared->commitTsActive)
 		return;
 
+	/*
+	 * Don't bother to record commit_ts for Booststrap mode.
+	 */
+	if (IsBootstrapProcessingMode())
+		return;
+
 	/*
 	 * Figure out the latest Xid in this batch: either the last subxid if
 	 * there's any, otherwise the parent xid.
-- 
2.45.1


--=-=-=--