On 14/10/2025 11:13, John Naylor wrote:
Okay, v2 gets rid of the general info mask (split out into 0002 for
readability) and leaves alone the RMGR-specific masks (i.e. leaves out
v1 0002/3). It runs fine with installcheck while streaming to a
standby with wal_consistency_checking. I've also left out the removal
of HEAP2 for now. Giving existing RMGRs more breathing room seems like
a better motivator, going by Nathan's comment and yours.
It's worth thinking about backward compatibility if we did go as far
as a 2-byte xl_info (upthread: to allow more RMGR-specific flags, so
e.g. XACT wouldn't need xl_xact_info) In that case, we'd probably
still want a convention that only the lowest byte can contain the
record type. XLogStats could simply assume that in most cases. For
XACT 8 flags in the upper byte still won't be enough, and it will
still need to have its own opcode mask, but that's no worse than the
situation we have already.
First let me say that I'm not objecting to this patch. It makes the code
a little bit more clear, which is good, so I think I'm +0.5 on this
overall. With that said:
I'm not sure I agree with the premise that we should try to get rid of
RM_HEAP2_ID. There's nothing wrong with that scheme as such. As an
alternative, we could easily teach e.g pg_waldump to treat RM_HEAP_ID
and RM_HEAP2_ID the same for statistics purposes.
This patch consumes one of the padding bytes. That's not entirely free,
as there is an opportunity cost: we could squeeze out the padding bytes
and save 2 bytes on every WAL record instead.
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; /* RMGR-specific info */
RmgrId xl_rmid; /* resource manager for this
record */
uint8 xl_geninfo; /* flag bits, see below */
/* 1 byte of padding here, initialize to zero */
pg_crc32c xl_crc; /* CRC for this record */
/* XLogRecordBlockHeaders and XLogRecordDataHeader follow, no padding */
} XLogRecord;
I'd suggest some minor reordering and renaming:
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_flags; /* flag bits, see below */
RmgrId xl_rmid; /* resource manager for this record */
uint8 xl_rminfo; /* RMGR-specific info */
/* 1 byte of padding here, initialize to zero */
pg_crc32c xl_crc; /* CRC for this record */
/* XLogRecordBlockHeaders and XLogRecordDataHeader follow, no
padding */
} XLogRecord;
In summary:
- Rename 'xl_info' to 'xl_rminfo' to make it more clear that it's
RMGR-specific.
- Rename 'xl_geninfo' to 'xl_flags'. I guess the 'gen' meant 'generic'
or 'general' to distinguish from the rmgr-specific field. But we don't
use a 'gen' prefix like that for any of the other non-rmgr-specific
fields. We could keep it under the old 'xl_info' name instead, but it's
nice to rename it to avoid confusion with the old xl_info field. It now
only contains flags, so 'xl_flags' seems appropriate.
- Reorder the fields so that 'xl_rmid' comes before 'xl_rminfo'. I find
that more intuitive, because the contents of 'xl_rminfo' depends on
'xl_rmid'.
While we're at it, I wonder if it would be more clear to have explicit
'xl_padding' field for the unused byte.
- Heikki
