Обсуждение: 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

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