Re: Wrong statistics for size of XLOG_SWITCH during pg_waldump.

At Wed, 14 Oct 2020 13:46:13 -0700, Andres Freund <andres@anarazel.de> wrote in 
> Hi,
> 
> On 2020-10-14 15:52:43 +0900, Michael Paquier wrote:
> > Yeah.  In its current shape, it means that only pg_waldump would be
> > able to know this information.  If you make this information part of
> > xlogdesc.c, any consumer of the WAL record descriptions would be able
> > to show this information, so it would provide a consistent output for
> > any kind of tools.
> 
> I'm not convinced by this argument. The only case where accounting for
> the "wasted" length seems really interesting is for --stats=record - and
> for that including it in the record description is useless. When looking
> at plain records the length is sufficiently deducable by looking at the
> next record's LSN.

I'm not sure the exact motive of this proposal, but if we show the
wasted length in the stats result, I think it should be other than
existing record types.

  XLOG/CHECKPOINT_SHUTDOWN    1 (  0.50) ..
  ...
  Btree/INSERT_LEAF          63 ( 31.19) ..
+ EMPTY                       1 ( xx.xx) ..
----------------------------------------
  Total                      ...


By the way, I noticed that --stats=record shows two lines for
Transaction/COMMIT. The cause is that XLogDumpCountRecord assumes the
all four bits of xl_info is used to identify record id.

The fourth bit of xl_info of XLOG records is used to signal the
existence of record has 'xinfo' field or not. So an XLOG record with
recid == 8 actually exists but it is really a record that recid == 0
and has xinfo field.

I didn't come up with a cleaner solution but the attached fixes that.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 31e99c2a6d..c544b90d88 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -438,6 +438,13 @@ XLogDumpCountRecord(XLogDumpConfig *config, XLogDumpStats *stats,
 
     recid = XLogRecGetInfo(record) >> 4;
 
+    /*
+     * XXX: There is a special case for XACT records. Those records use the MSB
+     * of recid for another purpose. Ignore that bit in that case.
+     */
+    if (rmid == RM_XACT_ID)
+        recid &= 0x07;
+
     stats->record_stats[rmid][recid].count++;
     stats->record_stats[rmid][recid].rec_len += rec_len;
     stats->record_stats[rmid][recid].fpi_len += fpi_len;