Re: Wrong statistics for size of XLOG_SWITCH during pg_waldump.

At Thu, 15 Oct 2020 12:56:02 +0800, "movead.li@highgo.ca" <movead.li@highgo.ca> wrote in 
> Thanks for all the suggestions.
> 
> >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 have change the implement, move some code into xlog_desc().

Andres suggested that we don't need that description with per-record
basis. Do you have a reason to do that?  (For clarity, I'm not
suggesting that you should reving it.)

> >On top of that, it seems to me that the calculation used in the patch
> >is wrong in two aspects at quick glance:
> >1) startSegNo and endSegNo point always to two different segments with
> >a XLOG_SWITCH record, so you should check that ReadRecPtr is not at a
> >segment border instead before extracting SizeOfXLogLongPHD, no?
> Yes you are right, and I use 'record->EndRecPtr - 1' instead.

+	XLByteToSeg(record->EndRecPtr - 1, endSegNo, record->segcxt.ws_segsize);

We use XLByteToPrevSeg instead for this purpose.

> >2) This stuff should also check after the case of a WAL *page* border
> >where you'd need to adjust based on SizeOfXLogShortPHD instead.
> The 'junk_len -= SizeOfXLogLongPHD' code is considered for when the
> remain size of a wal segment can not afford a XLogRecord struct for
> XLOG_SWITCH, it needn't care *page* border.
> 
> >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.
> Yes agree, and now it looks like below as new patch:

You forgot to add a correction for short headers.

> movead@movead-PC:/h2/pg/bin$ ./pg_waldump -p ../walbk/ -s 0/3000000 -e 0/6000000 -z
> Type                                           N      (%)          Record size      (%)             FPI size      (%)        Combined size      (%)
> ----                                           -      ---          -----------      ---             --------      ---        -------------      ---
> XLOG                                           5 ( 31.25)                  300 (  0.00)                    0 (  0.00)                  300 (  0.00)
> XLOGSwitchJunk                                 3 ( 18.75)             50330512 (100.00)                    0 (  0.00)             50330512 (100.00)
> 
> 
> movead@movead-PC:/h2/pg/bin$ ./pg_waldump -p ../walbk/ -s 0/3000000 -e 0/6000000 --stat=record
> XLOG/SWITCH                                    3 ( 18.75)                   72 (  0.00)                    0 (  0.00)                   72 (  0.00)
> XLOG/SWITCH_JUNK                               3 ( 18.75)             50330512 (100.00)                    0 (  0.00)             50330512 (100.00)
> 
> The shortcoming now is I do not know how to handle the 'count' of SWITCH_JUNK
> in pg_waldump results. Currently I regard SWITCH_JUNK as one count.


+	if(RM_XLOG_ID == rmid && XLOG_SWITCH == info)

We need a comment for the special code path.
We don't follow this kind of convension. Rather use "variable =
constant".

+	{
+		junk_len = xlog_switch_junk_len(record);
+		stats->count_xlog_switch++;
+		stats->junk_size += junk_len;

junk_len is used only the last line above.  We don't need that
temporary variable.

+	 * If the wal switch record spread on two segments, we should extra minus the

This comment is sticking out of 80-column border.  However, I'm not
sure if we have reached a conclustion about the target console-width.

+				info = (rj << 4) & ~XLR_INFO_MASK;
+				switch_junk_id = "XLOG/SWITCH_JUNK";
+				if( XLOG_SWITCH == info && stats->count_xlog_switch > 0)

This line order is strange. At least switch_junk_id is used only in
the if-then block.

I'm not confindent on which is better, but I feel that this is not a
work for display side, but for the recorder side like attached.

> >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.
> Thanks,I didn't notice it before, and your patch added into v3 patch attached.

Sorry for the confusion, but it would be a separate topic even if we
are going to fix that..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center