Re: get rid of RM_HEAP2_ID

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