Re: get rid of RM_HEAP2_ID - Mailing list pgsql-hackers

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