Move WAL/RMGR sequence code into its own file and header
Hi all,
Something that has been bugging me in the sequence code, while doing
some recent work (cough), is the fact that it is possible to cleanly
split the RMGR sequence code from the main sequence.c.
Please find attached a patch doing that, cleaning a bit sequence.c by
removing some of the WAL bits in it, as of:
- Addition of a new file sequence_xlog.c.
- Addition of a new header, sequence_xlog.h.
SEQ_MAGIC and sequence_magic need to be moved from sequence.c to the
new sequence_xlog.h, separating the WAL insert code with the backend
redo parts of it, which is more consistent with other RMGRs.
I have been looking at some other parts of the backend (like the
tablespace part), but these don't really bring a clear gain like the
change in the attached does.
Thanks,
--
Michael
Attachments:
0001-Move-WAL-sequence-code-into-its-own-file.patchtext/x-diff; charset=us-asciiDownload
From 00ce1d2bf53744125e4f8bd900de234150fb8a9d Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 27 Nov 2025 12:50:01 +0900
Subject: [PATCH] Move WAL sequence code into its own file
This split exists for most of the other RMGRs, and makes cleaner the
separation between WAL code, redo code and record description code
(already in its own file).
---
src/include/commands/sequence.h | 18 ------
src/include/commands/sequence_xlog.h | 45 +++++++++++++++
src/backend/access/rmgrdesc/seqdesc.c | 2 +-
src/backend/access/transam/rmgr.c | 2 +-
src/backend/commands/Makefile | 1 +
src/backend/commands/meson.build | 1 +
src/backend/commands/sequence.c | 76 +------------------------
src/backend/commands/sequence_xlog.c | 80 +++++++++++++++++++++++++++
src/bin/pg_waldump/rmgrdesc.c | 2 +-
9 files changed, 132 insertions(+), 95 deletions(-)
create mode 100644 src/include/commands/sequence_xlog.h
create mode 100644 src/backend/commands/sequence_xlog.c
diff --git a/src/include/commands/sequence.h b/src/include/commands/sequence.h
index 46b4d89dd6ea..3f8d353c49e7 100644
--- a/src/include/commands/sequence.h
+++ b/src/include/commands/sequence.h
@@ -13,14 +13,10 @@
#ifndef SEQUENCE_H
#define SEQUENCE_H
-#include "access/xlogreader.h"
#include "catalog/objectaddress.h"
#include "fmgr.h"
-#include "lib/stringinfo.h"
#include "nodes/parsenodes.h"
#include "parser/parse_node.h"
-#include "storage/relfilelocator.h"
-
typedef struct FormData_pg_sequence_data
{
@@ -42,15 +38,6 @@ typedef FormData_pg_sequence_data *Form_pg_sequence_data;
#define SEQ_COL_FIRSTCOL SEQ_COL_LASTVAL
#define SEQ_COL_LASTCOL SEQ_COL_CALLED
-/* XLOG stuff */
-#define XLOG_SEQ_LOG 0x00
-
-typedef struct xl_seq_rec
-{
- RelFileLocator locator;
- /* SEQUENCE TUPLE DATA FOLLOWS AT THE END */
-} xl_seq_rec;
-
extern int64 nextval_internal(Oid relid, bool check_permissions);
extern Datum nextval(PG_FUNCTION_ARGS);
extern List *sequence_options(Oid relid);
@@ -63,9 +50,4 @@ extern void ResetSequence(Oid seq_relid);
extern void SetSequence(Oid relid, int64 next, bool is_called);
extern void ResetSequenceCaches(void);
-extern void seq_redo(XLogReaderState *record);
-extern void seq_desc(StringInfo buf, XLogReaderState *record);
-extern const char *seq_identify(uint8 info);
-extern void seq_mask(char *page, BlockNumber blkno);
-
#endif /* SEQUENCE_H */
diff --git a/src/include/commands/sequence_xlog.h b/src/include/commands/sequence_xlog.h
new file mode 100644
index 000000000000..88453b382fb0
--- /dev/null
+++ b/src/include/commands/sequence_xlog.h
@@ -0,0 +1,45 @@
+/*-------------------------------------------------------------------------
+ *
+ * sequence_xlog.h
+ * Sequence WAL definitions.
+ *
+ * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/commands/sequence_xlog.h
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#ifndef SEQUENCE_XLOG_H
+#define SEQUENCE_XLOG_H
+
+#include "access/xlogreader.h"
+#include "lib/stringinfo.h"
+
+/* Record identifier */
+#define XLOG_SEQ_LOG 0x00
+
+/*
+ * The "special area" of a local sequence's buffer page looks like this.
+ */
+#define SEQ_MAGIC 0x1717
+
+typedef struct sequence_magic
+{
+ uint32 magic;
+} sequence_magic;
+
+/* Sequence WAL record */
+typedef struct xl_seq_rec
+{
+ RelFileLocator locator;
+ /* SEQUENCE TUPLE DATA FOLLOWS AT THE END */
+} xl_seq_rec;
+
+extern void seq_redo(XLogReaderState *record);
+extern void seq_desc(StringInfo buf, XLogReaderState *record);
+extern const char *seq_identify(uint8 info);
+extern void seq_mask(char *page, BlockNumber blkno);
+
+#endif /* SEQUENCE_XLOG_H */
diff --git a/src/backend/access/rmgrdesc/seqdesc.c b/src/backend/access/rmgrdesc/seqdesc.c
index 0d289d77fcf7..a0edb78856bd 100644
--- a/src/backend/access/rmgrdesc/seqdesc.c
+++ b/src/backend/access/rmgrdesc/seqdesc.c
@@ -14,7 +14,7 @@
*/
#include "postgres.h"
-#include "commands/sequence.h"
+#include "commands/sequence_xlog.h"
void
diff --git a/src/backend/access/transam/rmgr.c b/src/backend/access/transam/rmgr.c
index 1b7499726eb0..4fda03a3cfcc 100644
--- a/src/backend/access/transam/rmgr.c
+++ b/src/backend/access/transam/rmgr.c
@@ -33,7 +33,7 @@
#include "access/xact.h"
#include "catalog/storage_xlog.h"
#include "commands/dbcommands_xlog.h"
-#include "commands/sequence.h"
+#include "commands/sequence_xlog.h"
#include "commands/tablespace.h"
#include "replication/decode.h"
#include "replication/message.h"
diff --git a/src/backend/commands/Makefile b/src/backend/commands/Makefile
index f99acfd2b4bb..64cb6278409f 100644
--- a/src/backend/commands/Makefile
+++ b/src/backend/commands/Makefile
@@ -53,6 +53,7 @@ OBJS = \
schemacmds.o \
seclabel.o \
sequence.o \
+ sequence_xlog.o \
statscmds.o \
subscriptioncmds.o \
tablecmds.o \
diff --git a/src/backend/commands/meson.build b/src/backend/commands/meson.build
index 9f640ad48104..5fc35826b1cc 100644
--- a/src/backend/commands/meson.build
+++ b/src/backend/commands/meson.build
@@ -41,6 +41,7 @@ backend_sources += files(
'schemacmds.c',
'seclabel.c',
'sequence.c',
+ 'sequence_xlog.c',
'statscmds.c',
'subscriptioncmds.c',
'tablecmds.c',
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 8d671b7a29d6..51567994126f 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -14,7 +14,6 @@
*/
#include "postgres.h"
-#include "access/bufmask.h"
#include "access/htup_details.h"
#include "access/multixact.h"
#include "access/relation.h"
@@ -22,9 +21,7 @@
#include "access/table.h"
#include "access/transam.h"
#include "access/xact.h"
-#include "access/xlog.h"
#include "access/xloginsert.h"
-#include "access/xlogutils.h"
#include "catalog/dependency.h"
#include "catalog/indexing.h"
#include "catalog/namespace.h"
@@ -34,11 +31,13 @@
#include "catalog/storage_xlog.h"
#include "commands/defrem.h"
#include "commands/sequence.h"
+#include "commands/sequence_xlog.h"
#include "commands/tablecmds.h"
#include "funcapi.h"
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "parser/parse_type.h"
+#include "storage/bufmgr.h"
#include "storage/lmgr.h"
#include "storage/proc.h"
#include "storage/smgr.h"
@@ -58,16 +57,6 @@
*/
#define SEQ_LOG_VALS 32
-/*
- * The "special area" of a sequence's buffer page looks like this.
- */
-#define SEQ_MAGIC 0x1717
-
-typedef struct sequence_magic
-{
- uint32 magic;
-} sequence_magic;
-
/*
* We store a SeqTable item for every sequence we have touched in the current
* session. This is needed to hold onto nextval/currval state. (We can't
@@ -1907,56 +1896,6 @@ pg_sequence_last_value(PG_FUNCTION_ARGS)
PG_RETURN_NULL();
}
-
-void
-seq_redo(XLogReaderState *record)
-{
- XLogRecPtr lsn = record->EndRecPtr;
- uint8 info = XLogRecGetInfo(record) & ~XLR_INFO_MASK;
- Buffer buffer;
- Page page;
- Page localpage;
- char *item;
- Size itemsz;
- xl_seq_rec *xlrec = (xl_seq_rec *) XLogRecGetData(record);
- sequence_magic *sm;
-
- if (info != XLOG_SEQ_LOG)
- elog(PANIC, "seq_redo: unknown op code %u", info);
-
- buffer = XLogInitBufferForRedo(record, 0);
- page = BufferGetPage(buffer);
-
- /*
- * We always reinit the page. However, since this WAL record type is also
- * used for updating sequences, it's possible that a hot-standby backend
- * is examining the page concurrently; so we mustn't transiently trash the
- * buffer. The solution is to build the correct new page contents in
- * local workspace and then memcpy into the buffer. Then only bytes that
- * are supposed to change will change, even transiently. We must palloc
- * the local page for alignment reasons.
- */
- localpage = (Page) palloc(BufferGetPageSize(buffer));
-
- PageInit(localpage, BufferGetPageSize(buffer), sizeof(sequence_magic));
- sm = (sequence_magic *) PageGetSpecialPointer(localpage);
- sm->magic = SEQ_MAGIC;
-
- item = (char *) xlrec + sizeof(xl_seq_rec);
- itemsz = XLogRecGetDataLen(record) - sizeof(xl_seq_rec);
-
- if (PageAddItem(localpage, item, itemsz, FirstOffsetNumber, false, false) == InvalidOffsetNumber)
- elog(PANIC, "seq_redo: failed to add item to page");
-
- PageSetLSN(localpage, lsn);
-
- memcpy(page, localpage, BufferGetPageSize(buffer));
- MarkBufferDirty(buffer);
- UnlockReleaseBuffer(buffer);
-
- pfree(localpage);
-}
-
/*
* Flush cached sequence information.
*/
@@ -1971,14 +1910,3 @@ ResetSequenceCaches(void)
last_used_seq = NULL;
}
-
-/*
- * Mask a Sequence page before performing consistency checks on it.
- */
-void
-seq_mask(char *page, BlockNumber blkno)
-{
- mask_page_lsn_and_checksum(page);
-
- mask_unused_space(page);
-}
diff --git a/src/backend/commands/sequence_xlog.c b/src/backend/commands/sequence_xlog.c
new file mode 100644
index 000000000000..ffbd9820416a
--- /dev/null
+++ b/src/backend/commands/sequence_xlog.c
@@ -0,0 +1,80 @@
+/*-------------------------------------------------------------------------
+ *
+ * sequence.c
+ * RMGR WAL routines for sequences.
+ *
+ * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ *
+ * IDENTIFICATION
+ * src/backend/commands/sequence_xlog.c
+ *
+ *-------------------------------------------------------------------------
+ */
+#include "postgres.h"
+
+#include "access/bufmask.h"
+#include "access/xlogutils.h"
+#include "commands/sequence_xlog.h"
+#include "storage/bufmgr.h"
+
+void
+seq_redo(XLogReaderState *record)
+{
+ XLogRecPtr lsn = record->EndRecPtr;
+ uint8 info = XLogRecGetInfo(record) & ~XLR_INFO_MASK;
+ Buffer buffer;
+ Page page;
+ Page localpage;
+ char *item;
+ Size itemsz;
+ xl_seq_rec *xlrec = (xl_seq_rec *) XLogRecGetData(record);
+ sequence_magic *sm;
+
+ if (info != XLOG_SEQ_LOG)
+ elog(PANIC, "seq_redo: unknown op code %u", info);
+
+ buffer = XLogInitBufferForRedo(record, 0);
+ page = BufferGetPage(buffer);
+
+ /*
+ * We always reinit the page. However, since this WAL record type is also
+ * used for updating sequences, it's possible that a hot-standby backend
+ * is examining the page concurrently; so we mustn't transiently trash the
+ * buffer. The solution is to build the correct new page contents in
+ * local workspace and then memcpy into the buffer. Then only bytes that
+ * are supposed to change will change, even transiently. We must palloc
+ * the local page for alignment reasons.
+ */
+ localpage = (Page) palloc(BufferGetPageSize(buffer));
+
+ PageInit(localpage, BufferGetPageSize(buffer), sizeof(sequence_magic));
+ sm = (sequence_magic *) PageGetSpecialPointer(localpage);
+ sm->magic = SEQ_MAGIC;
+
+ item = (char *) xlrec + sizeof(xl_seq_rec);
+ itemsz = XLogRecGetDataLen(record) - sizeof(xl_seq_rec);
+
+ if (PageAddItem(localpage, item, itemsz, FirstOffsetNumber, false, false) == InvalidOffsetNumber)
+ elog(PANIC, "seq_redo: failed to add item to page");
+
+ PageSetLSN(localpage, lsn);
+
+ memcpy(page, localpage, BufferGetPageSize(buffer));
+ MarkBufferDirty(buffer);
+ UnlockReleaseBuffer(buffer);
+
+ pfree(localpage);
+}
+
+/*
+ * Mask a Sequence page before performing consistency checks on it.
+ */
+void
+seq_mask(char *page, BlockNumber blkno)
+{
+ mask_page_lsn_and_checksum(page);
+
+ mask_unused_space(page);
+}
diff --git a/src/bin/pg_waldump/rmgrdesc.c b/src/bin/pg_waldump/rmgrdesc.c
index fac509ed134e..931ab8b979e2 100644
--- a/src/bin/pg_waldump/rmgrdesc.c
+++ b/src/bin/pg_waldump/rmgrdesc.c
@@ -24,7 +24,7 @@
#include "access/xlog_internal.h"
#include "catalog/storage_xlog.h"
#include "commands/dbcommands_xlog.h"
-#include "commands/sequence.h"
+#include "commands/sequence_xlog.h"
#include "commands/tablespace.h"
#include "replication/message.h"
#include "replication/origin.h"
--
2.51.0
On 27/11/2025 06:29, Michael Paquier wrote:
Hi all,
Something that has been bugging me in the sequence code, while doing
some recent work (cough), is the fact that it is possible to cleanly
split the RMGR sequence code from the main sequence.c.Please find attached a patch doing that, cleaning a bit sequence.c by
removing some of the WAL bits in it, as of:
- Addition of a new file sequence_xlog.c.
- Addition of a new header, sequence_xlog.h.SEQ_MAGIC and sequence_magic need to be moved from sequence.c to the
new sequence_xlog.h, separating the WAL insert code with the backend
redo parts of it, which is more consistent with other RMGRs.I have been looking at some other parts of the backend (like the
tablespace part), but these don't really bring a clear gain like the
change in the attached does.
This doesn't really feel like an improvement to me, sequence.c is small
enough as is it is. If this helps with the other work you're doing
though, no objections.
- Heikki
On Thu, Nov 27, 2025 at 09:35:14AM +0200, Heikki Linnakangas wrote:
This doesn't really feel like an improvement to me, sequence.c is small
enough as is it is. If this helps with the other work you're doing though,
no objections.
Yes, it does for the sequence AM patch where I'm splitting the
sequence WAL APIs from the "core" sequence computation logic.
--
Michael
On Thu, 27 Nov 2025 at 10:38, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Nov 27, 2025 at 09:35:14AM +0200, Heikki Linnakangas wrote:
This doesn't really feel like an improvement to me, sequence.c is small
enough as is it is. If this helps with the other work you're doing though,
no objections.Yes, it does for the sequence AM patch where I'm splitting the
sequence WAL APIs from the "core" sequence computation logic.
--
Michael
Hi!
Shouldnt `SEQ_LOG_VALS` be moved to sequnce_xlog.c ?
Also, while on it, maybe it is worth to rename xl_seq_rec struct to
something. It will be more convietinet to make a name in sync with the
XLOG_SEQ_LOG WAL record (like we do in heapxlog). Maybe struct
xl_seq_log ?
--
Best regards,
Kirill Reshke
On Thu, Nov 27, 2025 at 12:00:30PM +0300, Kirill Reshke wrote:
Shouldnt `SEQ_LOG_VALS` be moved to sequnce_xlog.c ?
I am not sure to follow this one. This controls the frequency of the
records inserted, which has nothing to do with the redo path.
Also, while on it, maybe it is worth to rename xl_seq_rec struct to
something. It will be more convenient to make a name in sync with the
XLOG_SEQ_LOG WAL record (like we do in heapxlog). Maybe struct
xl_seq_log ?
I am not sure to see the value in a rename for the scope of this
patch, sequence.h already published them.
--
Michael
On Fri, Nov 28, 2025 at 01:05:55PM +0900, Michael Paquier wrote:
On Thu, Nov 27, 2025 at 12:00:30PM +0300, Kirill Reshke wrote:
Shouldnt `SEQ_LOG_VALS` be moved to sequnce_xlog.c ?
I am not sure to follow this one. This controls the frequency of the
records inserted, which has nothing to do with the redo path.Also, while on it, maybe it is worth to rename xl_seq_rec struct to
something. It will be more convenient to make a name in sync with the
XLOG_SEQ_LOG WAL record (like we do in heapxlog). Maybe struct
xl_seq_log ?I am not sure to see the value in a rename for the scope of this
patch, sequence.h already published them.
On a second look, I cannot get behind these two arguments. So I have
applied the patch as-is, after fixing a comment. Now onto some more
interesting work..
--
Michael