Subject: Remove XLogRecGetFullXid() in xlogreader.c?

From: Michael Paquier <[email-masked]>
Date: Fri, 17 Jan 2025 14:00:49 +0900

Hi all,

XLogRecGetFullXid() has been introduced in 67b9b3ca3283 back in 2019,
but as far as I can see this has never been used in the code and this
is used nowhere in the core code.

I have looked at Debian's codesearch and also looked at traces of it
on github without seeing it being used anywhere.  Knowing that this
was originally intended for a hypothetical undo log patch back then,
for which no work has been done for years, is there any point in
keeping this function in core?

This issue has been raised on a separate thread, where Noah has sent a
patch to consolidate a bit some epoch calculations for
FullTransactionIds, around here:
https://www.postgresql.org/message-id/[email-masked]

Removing it would have the benefit to do a bit less refactoring for
some of the work of the other thread, and this removes all traces of
-DFRONTEND in xlogreader.h.  ;)

Thoughts or comments?
--
Michael
From 101b8469dff3f35c9c3bcd24f1bcfff740f3a622 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Fri, 17 Jan 2025 13:56:40 +0900
Subject: [PATCH] Remove XLogRecGetFullXid()

---
 src/include/access/xlogreader.h         |  8 ------
 src/include/storage/standby.h           |  1 +
 src/backend/access/transam/xlogreader.c | 34 -------------------------
 3 files changed, 1 insertion(+), 42 deletions(-)

diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index 9738462d3c..18a67acf42 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -34,10 +34,6 @@
 #ifndef XLOGREADER_H
 #define XLOGREADER_H
 
-#ifndef FRONTEND
-#include "access/transam.h"
-#endif
-
 #include "access/xlogrecord.h"
 #include "storage/buf.h"
 
@@ -427,10 +423,6 @@ extern bool DecodeXLogRecord(XLogReaderState *state,
 #define XLogRecHasBlockData(decoder, block_id)		\
 	((decoder)->record->blocks[block_id].has_data)
 
-#ifndef FRONTEND
-extern FullTransactionId XLogRecGetFullXid(XLogReaderState *record);
-#endif
-
 extern bool RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page);
 extern char *XLogRecGetBlockData(XLogReaderState *record, uint8 block_id, Size *len);
 extern void XLogRecGetBlockTag(XLogReaderState *record, uint8 block_id,
diff --git a/src/include/storage/standby.h b/src/include/storage/standby.h
index 24e2f5082b..995d3c365d 100644
--- a/src/include/storage/standby.h
+++ b/src/include/storage/standby.h
@@ -14,6 +14,7 @@
 #ifndef STANDBY_H
 #define STANDBY_H
 
+#include "access/transam.h"
 #include "datatype/timestamp.h"
 #include "storage/lock.h"
 #include "storage/procsignal.h"
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 3596af0617..12d5175dc3 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -2157,37 +2157,3 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page)
 
 	return true;
 }
-
-#ifndef FRONTEND
-
-/*
- * Extract the FullTransactionId from a WAL record.
- */
-FullTransactionId
-XLogRecGetFullXid(XLogReaderState *record)
-{
-	TransactionId xid,
-				next_xid;
-	uint32		epoch;
-
-	/*
-	 * This function is only safe during replay, because it depends on the
-	 * replay state.  See AdvanceNextFullTransactionIdPastXid() for more.
-	 */
-	Assert(AmStartupProcess() || !IsUnderPostmaster);
-
-	xid = XLogRecGetXid(record);
-	next_xid = XidFromFullTransactionId(TransamVariables->nextXid);
-	epoch = EpochFromFullTransactionId(TransamVariables->nextXid);
-
-	/*
-	 * If xid is numerically greater than next_xid, it has to be from the last
-	 * epoch.
-	 */
-	if (unlikely(xid > next_xid))
-		--epoch;
-
-	return FullTransactionIdFromEpochAndXid(epoch, xid);
-}
-
-#endif
-- 
2.47.1

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 17 Jan 2025 09:39:16 -0600

(I've added Thomas Munro to the thread.)

> -DFRONTEND in xlogreader.h.  ;)

Seems reasonable to me.  I think the counterargument is that folks
developing new AMs should use this [0], but if no such users have
materialized in several years, then maybe that's no longer a concern.

[0] https://postgr.es/m/CA%2BhUKG%2BmLmuDjMi6o1dxkKvGRL56Y2Rz%2BiXAcrZV03G9ZuFQ8Q%40mail.gmail.com

-- 
nathan

---

From: Andres Freund <[email-masked]>
Date: Fri, 17 Jan 2025 10:45:28 -0500

Hi,

> -DFRONTEND in xlogreader.h.  ;)

-0.5

I think it is sensible infrastructure and we'll eventually end up using
it. The cost of having it fairly low. If it were a legacy thing that we
shouldn't introduce new users for it'd be a different story, but it's the
opposite.

Greetings,

Andres

---

From: Michael Paquier <[email-masked]>
Date: Sat, 18 Jan 2025 08:21:43 +0900

> shouldn't introduce new users for it'd be a different story, but it's the

The last 6 years are showing that we are still looking for such users,
so honestly, I am not sure if it's worth keeping.  Let's see if others
have an opinion.  I'm OK if the consensus is to let things be, as
well.
--
Michael