Hi Michael,
On Thu, Dec 18, 2025 at 12:53 PM Michael Paquier <[email protected]> wrote:
>
> On Thu, Nov 13, 2025 at 08:51:32AM +0900, Michael Paquier wrote:
> > Err, no. It's the opposite here: code cleanups and file splits are
> > cleaner if they happen first, not after the implementations as these
> > lead to less code churn overall. Splitting the sequence "core" logic
> > and WAL logic make sense in the long-term to me anyway, as a separate
> > refactoring piece. It's true that 0002 could be slightly different,
> > though, we could for example keep sequence.c where it is now in
> > src/backend/commands/ without forcing the use of the term "AM" in the
> > file names, and extract the WAL pieces of it into a new file (aka the
> > redo and marking routines). Then it's only a game of moving the files
> > around depending on the follow-up pieces. I should probably post a
> > patch for that separately, this has been bugging me a bit in terms of
> > code separation clarity for the sequence RMGR.
>
> Since this update, the code related to the sequence RMGR has been
> moved around with a87987cafca6. This makes the rebased version of the
> patch leaner, with a cleaner split between the WAL and "core"
> computation logic, as v24 and older patch sets submitted on this
> thread were splitting this code already.
>
> Anyway. Rebased. v25. Attached.
Thanks for working on this. I tried to review patch set v25, but I
wasn’t able to apply it cleanly on HEAD.
On an initial look, here are a few minor comments on patches 1 and 2.
1. Duplicate macro definition in seqlocalam.c:
+#define SEQ_LOCAL_LOG_VALS 32
...
+#define SEQ_LOCAL_LOG_VALS 32
We have two macros the same here.
2. Duplicate stmt->tableElts = NIL; in sequence.c:
*/
stmt->tableElts = NIL;
+ /*
+ * Initial relation has no attributes, these can be added later via the
+ * "init" AM callback.
+ */
+ stmt->tableElts = NIL;
The same assignment appears twice - the second seems to be redundant.
3. Potential error message inconsistency in seqlocalxlog.c:
if (info != XLOG_SEQ_LOCAL_LOG)
elog(PANIC, "seq_redo: unknown op code %u", info); // Says
"seq_redo" not "seq_local_redo"
Should we update this to "seq_local_redo: unknown op code %u”?
--
Best,
Xuneng
