Thread: [PATCH] Refactor: Extract XLogRecord info
Hi,
I refactored the code of extracting XLogRecord info.
In XLogRecord, the high 4 bits in xl_info is used by rmgr.
typedef struct XLogRecord
{
uint32 xl_tot_len; /* total len of entire record */
TransactionId xl_xid; /* xact id */
XLogRecPtr xl_prev; /* ptr to previous record in log */
uint8 xl_info; /* flag bits, see below */
RmgrId xl_rmid; /* resource manager for this record */
/* 2 bytes of padding here, initialize to zero */
pg_crc32c xl_crc; /* CRC for this record */
/* XLogRecordBlockHeaders and XLogRecordDataHeader follow, no padding */
} XLogRecord;
{
uint32 xl_tot_len; /* total len of entire record */
TransactionId xl_xid; /* xact id */
XLogRecPtr xl_prev; /* ptr to previous record in log */
uint8 xl_info; /* flag bits, see below */
RmgrId xl_rmid; /* resource manager for this record */
/* 2 bytes of padding here, initialize to zero */
pg_crc32c xl_crc; /* CRC for this record */
/* XLogRecordBlockHeaders and XLogRecordDataHeader follow, no padding */
} XLogRecord;
I found lots of the code to get the info as below
XLogRecGetInfo(record) & ~XLR_INFO_MASK
Actually, we can directly use XLR_RMGR_INFO_MASK(0xF0)
instead of XLR_INFO_MASK(0x0F), which is easier to understand.
Remove XLR_INFO_MASK as it is not used any more.
Remove XLR_INFO_MASK as it is not used any more.
Best regards !
Xiaoran Wang
Attachment
Hi, I like the idea of your change as it saves me out of converting-in-my-mind. And I suggest to create a macro to do this job. #define getRmgrInfo(info) (info & XLR_RMGR_INFO_MASK) Then the code can become: XLogRecGetInfo(record) & ~XLR_INFO_MASK; --> getRmgrInfo(XLogRecGetInfo(record)); Thanks, Steven 在 2025/6/9 14:23, Xiaoran Wang 写道: > Hi, > I refactored the code of extracting XLogRecord info. > In XLogRecord, the high 4 bits in xl_info is used by rmgr. > > typedef struct XLogRecord > { > uint32 xl_tot_len; /* total len of entire record */ > TransactionId xl_xid; /* xact id */ > XLogRecPtr xl_prev; /* ptr to previous record in log */ > uint8 xl_info; /* flag bits, see below */ > RmgrId xl_rmid; /* resource manager for this record */ > /* 2 bytes of padding here, initialize to zero */ > pg_crc32c xl_crc; /* CRC for this record */ > > /* XLogRecordBlockHeaders and XLogRecordDataHeader follow, no > padding */ > > } XLogRecord; > > I found lots of the code to get the info as below > > XLogRecGetInfo(record) & ~XLR_INFO_MASK > > Actually, we can directly use XLR_RMGR_INFO_MASK(0xF0) > instead of XLR_INFO_MASK(0x0F), which is easier to understand. > Remove XLR_INFO_MASK as it is not used any more. > > > -- > Best regards ! > Xiaoran Wang
HI
> And I suggest to create a macro to do this job.
> #define getRmgrInfo(info) (info & XLR_RMGR_INFO_MASK)
>
> Then the code can become:
> XLogRecGetInfo(record) & ~XLR_INFO_MASK;
> -->
> getRmgrInfo(XLogRecGetInfo(record))
> #define getRmgrInfo(info) (info & XLR_RMGR_INFO_MASK)
>
> Then the code can become:
> XLogRecGetInfo(record) & ~XLR_INFO_MASK;
> -->
> getRmgrInfo(XLogRecGetInfo(record))
+1 Agreed, this makes the code more readable.
Thanks
On Mon, Jun 9, 2025 at 2:46 PM Steven Niu <niushiji@gmail.com> wrote:
Hi,
I like the idea of your change as it saves me out of converting-in-my-mind.
And I suggest to create a macro to do this job.
#define getRmgrInfo(info) (info & XLR_RMGR_INFO_MASK)
Then the code can become:
XLogRecGetInfo(record) & ~XLR_INFO_MASK;
-->
getRmgrInfo(XLogRecGetInfo(record));
Thanks,
Steven
在 2025/6/9 14:23, Xiaoran Wang 写道:
> Hi,
> I refactored the code of extracting XLogRecord info.
> In XLogRecord, the high 4 bits in xl_info is used by rmgr.
>
> typedef struct XLogRecord
> {
> uint32 xl_tot_len; /* total len of entire record */
> TransactionId xl_xid; /* xact id */
> XLogRecPtr xl_prev; /* ptr to previous record in log */
> uint8 xl_info; /* flag bits, see below */
> RmgrId xl_rmid; /* resource manager for this record */
> /* 2 bytes of padding here, initialize to zero */
> pg_crc32c xl_crc; /* CRC for this record */
>
> /* XLogRecordBlockHeaders and XLogRecordDataHeader follow, no
> padding */
>
> } XLogRecord;
>
> I found lots of the code to get the info as below
>
> XLogRecGetInfo(record) & ~XLR_INFO_MASK
>
> Actually, we can directly use XLR_RMGR_INFO_MASK(0xF0)
> instead of XLR_INFO_MASK(0x0F), which is easier to understand.
> Remove XLR_INFO_MASK as it is not used any more.
>
>
> --
> Best regards !
> Xiaoran Wang
Steven Niu <niushiji@gmail.com> 于2025年6月9日周一 14:46写道:
Hi,
I like the idea of your change as it saves me out of converting-in-my-mind.
And I suggest to create a macro to do this job.
#define getRmgrInfo(info) (info & XLR_RMGR_INFO_MASK)
Then the code can become:
XLogRecGetInfo(record) & ~XLR_INFO_MASK;
-->
getRmgrInfo(XLogRecGetInfo(record));
Good idea, found lots of 'XLogRecGetInfo(record) & ~XLR_INFO_MASK;'
in the code.
I add a macro XLogRecRmgrGetInfo(record) in patch 0002.
Thanks,
Steven
在 2025/6/9 14:23, Xiaoran Wang 写道:
> Hi,
> I refactored the code of extracting XLogRecord info.
> In XLogRecord, the high 4 bits in xl_info is used by rmgr.
>
> typedef struct XLogRecord
> {
> uint32 xl_tot_len; /* total len of entire record */
> TransactionId xl_xid; /* xact id */
> XLogRecPtr xl_prev; /* ptr to previous record in log */
> uint8 xl_info; /* flag bits, see below */
> RmgrId xl_rmid; /* resource manager for this record */
> /* 2 bytes of padding here, initialize to zero */
> pg_crc32c xl_crc; /* CRC for this record */
>
> /* XLogRecordBlockHeaders and XLogRecordDataHeader follow, no
> padding */
>
> } XLogRecord;
>
> I found lots of the code to get the info as below
>
> XLogRecGetInfo(record) & ~XLR_INFO_MASK
>
> Actually, we can directly use XLR_RMGR_INFO_MASK(0xF0)
> instead of XLR_INFO_MASK(0x0F), which is easier to understand.
> Remove XLR_INFO_MASK as it is not used any more.
>
>
> --
> Best regards !
> Xiaoran Wang
Best regards !
Xiaoran Wang
Attachment
Just upload all the patches together.
Xiaoran Wang <fanfuxiaoran@gmail.com> 于2025年6月9日周一 18:25写道:
Steven Niu <niushiji@gmail.com> 于2025年6月9日周一 14:46写道:Hi,
I like the idea of your change as it saves me out of converting-in-my-mind.
And I suggest to create a macro to do this job.
#define getRmgrInfo(info) (info & XLR_RMGR_INFO_MASK)
Then the code can become:
XLogRecGetInfo(record) & ~XLR_INFO_MASK;
-->
getRmgrInfo(XLogRecGetInfo(record));Good idea, found lots of 'XLogRecGetInfo(record) & ~XLR_INFO_MASK;'in the code.I add a macro XLogRecRmgrGetInfo(record) in patch 0002.
Thanks,
Steven
在 2025/6/9 14:23, Xiaoran Wang 写道:
> Hi,
> I refactored the code of extracting XLogRecord info.
> In XLogRecord, the high 4 bits in xl_info is used by rmgr.
>
> typedef struct XLogRecord
> {
> uint32 xl_tot_len; /* total len of entire record */
> TransactionId xl_xid; /* xact id */
> XLogRecPtr xl_prev; /* ptr to previous record in log */
> uint8 xl_info; /* flag bits, see below */
> RmgrId xl_rmid; /* resource manager for this record */
> /* 2 bytes of padding here, initialize to zero */
> pg_crc32c xl_crc; /* CRC for this record */
>
> /* XLogRecordBlockHeaders and XLogRecordDataHeader follow, no
> padding */
>
> } XLogRecord;
>
> I found lots of the code to get the info as below
>
> XLogRecGetInfo(record) & ~XLR_INFO_MASK
>
> Actually, we can directly use XLR_RMGR_INFO_MASK(0xF0)
> instead of XLR_INFO_MASK(0x0F), which is easier to understand.
> Remove XLR_INFO_MASK as it is not used any more.
>
>
> --
> Best regards !
> Xiaoran Wang--Best regards !Xiaoran Wang
Best regards !
Xiaoran Wang
Attachment
LGTM. I have no more comments.
Regards,
Steven
Xiaoran Wang <fanfuxiaoran@gmail.com> 于2025年6月9日周一 18:31写道:
Just upload all the patches together.Xiaoran Wang <fanfuxiaoran@gmail.com> 于2025年6月9日周一 18:25写道:Steven Niu <niushiji@gmail.com> 于2025年6月9日周一 14:46写道:Hi,
I like the idea of your change as it saves me out of converting-in-my-mind.
And I suggest to create a macro to do this job.
#define getRmgrInfo(info) (info & XLR_RMGR_INFO_MASK)
Then the code can become:
XLogRecGetInfo(record) & ~XLR_INFO_MASK;
-->
getRmgrInfo(XLogRecGetInfo(record));Good idea, found lots of 'XLogRecGetInfo(record) & ~XLR_INFO_MASK;'in the code.I add a macro XLogRecRmgrGetInfo(record) in patch 0002.
Thanks,
Steven
在 2025/6/9 14:23, Xiaoran Wang 写道:
> Hi,
> I refactored the code of extracting XLogRecord info.
> In XLogRecord, the high 4 bits in xl_info is used by rmgr.
>
> typedef struct XLogRecord
> {
> uint32 xl_tot_len; /* total len of entire record */
> TransactionId xl_xid; /* xact id */
> XLogRecPtr xl_prev; /* ptr to previous record in log */
> uint8 xl_info; /* flag bits, see below */
> RmgrId xl_rmid; /* resource manager for this record */
> /* 2 bytes of padding here, initialize to zero */
> pg_crc32c xl_crc; /* CRC for this record */
>
> /* XLogRecordBlockHeaders and XLogRecordDataHeader follow, no
> padding */
>
> } XLogRecord;
>
> I found lots of the code to get the info as below
>
> XLogRecGetInfo(record) & ~XLR_INFO_MASK
>
> Actually, we can directly use XLR_RMGR_INFO_MASK(0xF0)
> instead of XLR_INFO_MASK(0x0F), which is easier to understand.
> Remove XLR_INFO_MASK as it is not used any more.
>
>
> --
> Best regards !
> Xiaoran Wang--Best regards !Xiaoran Wang--Best regards !Xiaoran Wang
On Mon, 9 Jun 2025 at 22:40 Steven Niu <niushiji@gmail.com> wrote:
LGTM. I have no more comments.
The refactoring LGTM but do we really need two patches? IMHO you can just merge everything into a single patch.
Fabrízio de Royes Mello
On Mon, Jun 09, 2025 at 10:54:43PM -0300, Fabrízio de Royes Mello wrote: > The refactoring LGTM but do we really need two patches? IMHO you can just > merge everything into a single patch. FWIW, I'm not sure what's the benefit of the proposal which comes down to the removal of a bitwise NOT, except more code conflicts with back branches. -- Michael
Attachment
HI
> FWIW, I'm not sure what's the benefit of the proposal which comes down
> to the removal of a bitwise NOT, except more code conflicts with back
> branches.
> to the removal of a bitwise NOT, except more code conflicts with back
> branches.
Agree
On Tue, Jun 10, 2025 at 3:37 PM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Jun 09, 2025 at 10:54:43PM -0300, Fabrízio de Royes Mello wrote:
> The refactoring LGTM but do we really need two patches? IMHO you can just
> merge everything into a single patch.
FWIW, I'm not sure what's the benefit of the proposal which comes down
to the removal of a bitwise NOT, except more code conflicts with back
branches.
--
Michael
I'm confused by the code of XLR_RMGR_INFO_MASK and XLR_INFO_MASK.
According to the definition of masks, the high 4 bits are for rmgr.
/*
* The high 4 bits in xl_info may be used freely by rmgr. The
* XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCY bits can be passed by
* XLogInsert caller. The rest are set internally by XLogInsert.
*/
#define XLR_INFO_MASK 0x0F
#define XLR_RMGR_INFO_MASK 0xF0
However, in function XLogInsert(), there is code:
/*
* The caller can set rmgr bits, XLR_SPECIAL_REL_UPDATE and
* XLR_CHECK_CONSISTENCY; the rest are reserved for use by me.
*/
if ((info & ~(XLR_RMGR_INFO_MASK |
XLR_SPECIAL_REL_UPDATE |
XLR_CHECK_CONSISTENCY)) != 0)
elog(PANIC, "invalid xlog info mask %02X", info);
#define XLR_SPECIAL_REL_UPDATE 0x01
#define XLR_CHECK_CONSISTENCY 0x02
As the XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCY are of the low 4 bits,
the above code is indicating the low 4 bits are for rmgr too?
Did I misunderstand something?
According to the definition of masks, the high 4 bits are for rmgr.
/*
* The high 4 bits in xl_info may be used freely by rmgr. The
* XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCY bits can be passed by
* XLogInsert caller. The rest are set internally by XLogInsert.
*/
#define XLR_INFO_MASK 0x0F
#define XLR_RMGR_INFO_MASK 0xF0
However, in function XLogInsert(), there is code:
/*
* The caller can set rmgr bits, XLR_SPECIAL_REL_UPDATE and
* XLR_CHECK_CONSISTENCY; the rest are reserved for use by me.
*/
if ((info & ~(XLR_RMGR_INFO_MASK |
XLR_SPECIAL_REL_UPDATE |
XLR_CHECK_CONSISTENCY)) != 0)
elog(PANIC, "invalid xlog info mask %02X", info);
#define XLR_SPECIAL_REL_UPDATE 0x01
#define XLR_CHECK_CONSISTENCY 0x02
As the XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCY are of the low 4 bits,
the above code is indicating the low 4 bits are for rmgr too?
Did I misunderstand something?
Thanks,
Steven
wenhui qiu <qiuwenhuifx@gmail.com> 于2025年6月10日周二 16:00写道:
HI> FWIW, I'm not sure what's the benefit of the proposal which comes down
> to the removal of a bitwise NOT, except more code conflicts with back
> branches.AgreeOn Tue, Jun 10, 2025 at 3:37 PM Michael Paquier <michael@paquier.xyz> wrote:On Mon, Jun 09, 2025 at 10:54:43PM -0300, Fabrízio de Royes Mello wrote:
> The refactoring LGTM but do we really need two patches? IMHO you can just
> merge everything into a single patch.
FWIW, I'm not sure what's the benefit of the proposal which comes down
to the removal of a bitwise NOT, except more code conflicts with back
branches.
--
Michael
Steven Niu <niushiji@gmail.com> 于2025年6月10日周二 17:56写道:
I'm confused by the code of XLR_RMGR_INFO_MASK and XLR_INFO_MASK.
According to the definition of masks, the high 4 bits are for rmgr.
/*
* The high 4 bits in xl_info may be used freely by rmgr. The
* XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCY bits can be passed by
* XLogInsert caller. The rest are set internally by XLogInsert.
*/
#define XLR_INFO_MASK 0x0F
#define XLR_RMGR_INFO_MASK 0xF0
However, in function XLogInsert(), there is code:
/*
* The caller can set rmgr bits, XLR_SPECIAL_REL_UPDATE and
* XLR_CHECK_CONSISTENCY; the rest are reserved for use by me.
*/
if ((info & ~(XLR_RMGR_INFO_MASK |
XLR_SPECIAL_REL_UPDATE |
XLR_CHECK_CONSISTENCY)) != 0)
elog(PANIC, "invalid xlog info mask %02X", info);
XLogInsert only allows the rmgr ,XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCY
set in the info.
#define XLR_SPECIAL_REL_UPDATE 0x01
#define XLR_CHECK_CONSISTENCY 0x02
As the XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCY are of the low 4 bits,
the above code is indicating the low 4 bits are for rmgr too?
No, only the high 4 bits are used for RMGR, see the code under directory 'src/backend/access/rmgrdesc'
'XLR_SPECIAL_REL_UPDATE' and 'XLR_CHECK_CONSISTENCY' are not RMGR info, but they
can be passed by XLogInsert caller.
Did I misunderstand something?Thanks,Steven
Best regards !
Xiaoran Wang
Hi, Xiaoran,
I see. The code is checking if the bits other than rmgr bits, XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCY are used.
Thanks for the explanation.
Steven
I see. The code is checking if the bits other than rmgr bits, XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCY are used.
Thanks for the explanation.
Steven
Xiaoran Wang <fanfuxiaoran@gmail.com> 于2025年6月11日周三 10:13写道:
Steven Niu <niushiji@gmail.com> 于2025年6月10日周二 17:56写道:I'm confused by the code of XLR_RMGR_INFO_MASK and XLR_INFO_MASK.
According to the definition of masks, the high 4 bits are for rmgr.
/*
* The high 4 bits in xl_info may be used freely by rmgr. The
* XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCY bits can be passed by
* XLogInsert caller. The rest are set internally by XLogInsert.
*/
#define XLR_INFO_MASK 0x0F
#define XLR_RMGR_INFO_MASK 0xF0
However, in function XLogInsert(), there is code:
/*
* The caller can set rmgr bits, XLR_SPECIAL_REL_UPDATE and
* XLR_CHECK_CONSISTENCY; the rest are reserved for use by me.
*/
if ((info & ~(XLR_RMGR_INFO_MASK |
XLR_SPECIAL_REL_UPDATE |
XLR_CHECK_CONSISTENCY)) != 0)
elog(PANIC, "invalid xlog info mask %02X", info);XLogInsert only allows the rmgr ,XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCYset in the info.#define XLR_SPECIAL_REL_UPDATE 0x01
#define XLR_CHECK_CONSISTENCY 0x02
As the XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCY are of the low 4 bits,
the above code is indicating the low 4 bits are for rmgr too?No, only the high 4 bits are used for RMGR, see the code under directory 'src/backend/access/rmgrdesc''XLR_SPECIAL_REL_UPDATE' and 'XLR_CHECK_CONSISTENCY' are not RMGR info, but theycan be passed by XLogInsert caller.
Did I misunderstand something?Thanks,Steven--Best regards !Xiaoran Wang
Hi, Xiaoran, I see. The code is checking if the bits other than rmgr bits, XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCY are used. Thanks for explanation. Steven 在 2025/6/11 10:13, Xiaoran Wang 写道: > > > Steven Niu <niushiji@gmail.com <mailto:niushiji@gmail.com>> 于2025年6月 > 10日周二 17:56写道: > > I'm confused by the code of XLR_RMGR_INFO_MASK and XLR_INFO_MASK. > > According to the definition of masks, the high 4 bits are for rmgr. > > /* > * The high 4 bits in xl_info may be used freely by rmgr. The > * XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCY bits can be > passed by > * XLogInsert caller. The rest are set internally by XLogInsert. > */ > #define XLR_INFO_MASK 0x0F > #define XLR_RMGR_INFO_MASK 0xF0 > > > However, in function XLogInsert(), there is code: > > /* > * The caller can set rmgr bits, XLR_SPECIAL_REL_UPDATE and > * XLR_CHECK_CONSISTENCY; the rest are reserved for use by me. > */ > if ((info & ~(XLR_RMGR_INFO_MASK | > XLR_SPECIAL_REL_UPDATE | > XLR_CHECK_CONSISTENCY)) != 0) > elog(PANIC, "invalid xlog info mask %02X", info); > > XLogInsert only allows the rmgr ,XLR_SPECIAL_REL_UPDATE and > XLR_CHECK_CONSISTENCY > set in the info. > > #define XLR_SPECIAL_REL_UPDATE 0x01 > #define XLR_CHECK_CONSISTENCY 0x02 > > As the XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCY are of the > low 4 bits, > the above code is indicating the low 4 bits are for rmgr too? > > > No, only the high 4 bits are used for RMGR, see the code under directory > 'src/backend/access/rmgrdesc' > > 'XLR_SPECIAL_REL_UPDATE' and 'XLR_CHECK_CONSISTENCY' are not RMGR > info, but they > can be passed by XLogInsert caller. > > > > Did I misunderstand something? > > Thanks, > Steven > > > -- > Best regards ! > Xiaoran Wang