Wrong statistics for size of XLOG_SWITCH during pg_waldump.

At Fri, 9 Oct 2020 13:41:25 +0800, "movead.li@highgo.ca" <movead.li@highgo.ca> wrote in

Hello hackers,

We know that pg_waldump can statistics size for every kind of records. When I use
the feature I find it misses some size for XLOG_SWITCH records. When a user does
a pg_wal_switch(), then postgres will discard the remaining size in the current wal
segment, and the pg_waldump tool misses the discard size.

I think it will be better if pg_waldump can show the matter, so I make a patch
which regards the discard size as a part of XLOG_SWITCH record, it works if we
want to display the detail of wal records or the statistics, and patch attached.

What's your opinion?

I think that the length of the XLOG_SWITCH record is no other than 24
bytes. Just adding the padding? garbage bytes to that length doesn't
seem the right thing to me.

If we want pg_waldump to show that length somewhere, it could be shown
at the end of that record explicitly:

rmgr: XLOG len (rec/tot): 24/16776848, tx: 0, lsn: 0/02000148, prev 0/02000110, desc: SWITCH, trailing-bytes: 16776944

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

I think that the length of the XLOG_SWITCH record is no other than 24
bytes. Just adding the padding? garbage bytes to that length doesn't
seem the right thing to me.

If we want pg_waldump to show that length somewhere, it could be shown
at the end of that record explicitly:

rmgr: XLOG len (rec/tot): 24/16776848, tx: 0, lsn: 0/02000148, prev 0/02000110, desc: SWITCH, trailing-bytes: 16776944

Thanks, I think it's good idea, and new patch attached.

Here's the lookes:
rmgr: XLOG len (rec/tot): 24/ 24, tx: 0, lsn: 0/030000D8, prev 0/03000060, desc: SWITCH, trailing-bytes: 16776936

Regards,
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca

On Sat, Oct 10, 2020 at 09:50:02AM +0800, movead.li@highgo.ca wrote:

I think that the length of the XLOG_SWITCH record is no other than 24
bytes. Just adding the padding? garbage bytes to that length doesn't
seem the right thing to me.

Here's the lookes:
rmgr: XLOG len (rec/tot): 24/ 24, tx: 0, lsn: 0/030000D8, prev 0/03000060, desc: SWITCH, trailing-bytes: 16776936

 static void
-XLogDumpRecordLen(XLogReaderState *record, uint32 *rec_len, uint32 *fpi_len)
+XLogDumpRecordLen(XLogReaderState *record, uint32 *rec_len, uint32 *fpi_len, uint32 *junk_len)
 {
If you wish to add more information about a XLOG_SWITCH record, I
don't think that changing the signature of XLogDumpRecordLen() is
adapted because the record length of this record is defined as
Horiguchi-san mentioned upthread, and the meaning of junk_len is
confusing here.  It seems to me that any extra information should be
added to xlog_desc() where there should be an extra code path for
(info == XLOG_SWITCH).  XLogReaderState should have all the
information you are lookng for.
--
Michael

Thanks for reply.

If you wish to add more information about a XLOG_SWITCH record, I
don't think that changing the signature of XLogDumpRecordLen() is
adapted because the record length of this record is defined as
Horiguchi-san mentioned upthread, and the meaning of junk_len is
confusing here. It seems to me that any extra information should be
added to xlog_desc() where there should be an extra code path for
(info == XLOG_SWITCH). XLogReaderState should have all the
information you are lookng for.

We have two places to use the 'junk_len', one is when we show the
detail record information, another is when we statistics the percent
of all kind of wal record kinds(by --stat=record). The second place
will not run the xlog_desc(), so it's not a good chance to do the thing.

I am still can not understand why it can't adapted to change the
signature of XLogDumpRecordLen(), maybe we can add a new function
to caculate the 'junk_len' and rename the 'junk_len' as 'skiped_size' or
'switched_size'?

Regards,
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca

At Mon, 12 Oct 2020 09:46:37 +0800, "movead.li@highgo.ca" <movead.li@highgo.ca> wrote in

Thanks for reply.

If you wish to add more information about a XLOG_SWITCH record, I
don't think that changing the signature of XLogDumpRecordLen() is
adapted because the record length of this record is defined as
Horiguchi-san mentioned upthread, and the meaning of junk_len is
confusing here. It seems to me that any extra information should be
added to xlog_desc() where there should be an extra code path for
(info == XLOG_SWITCH). XLogReaderState should have all the
information you are lookng for.

We have two places to use the 'junk_len', one is when we show the
detail record information, another is when we statistics the percent
of all kind of wal record kinds(by --stat=record). The second place
will not run the xlog_desc(), so it's not a good chance to do the thing.

I am still can not understand why it can't adapted to change the
signature of XLogDumpRecordLen(), maybe we can add a new function
to caculate the 'junk_len' and rename the 'junk_len' as 'skiped_size' or
'switched_size'?

The reason is the function XLogDumpRecordLen is a common function
among all kind of LOG records, not belongs only to XLOG_SWICH. And the
junk_len is not useful for other than XLOG_SWITCH. Descriptions
specifc to XLOG_SWITCH is provided by xlog_desc().

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Wed, Oct 14, 2020 at 10:29:44AM +0900, Kyotaro Horiguchi wrote:

The reason is the function XLogDumpRecordLen is a common function
among all kind of LOG records, not belongs only to XLOG_SWICH. And the
junk_len is not useful for other than XLOG_SWITCH. Descriptions
specifc to XLOG_SWITCH is provided by xlog_desc().

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.

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?
2) This stuff should also check after the case of a WAL *page* border
where you'd need to adjust based on SizeOfXLogShortPHD instead.
--
Michael

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.

Greetings,

Andres Freund

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

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().

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.

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:

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.

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.

Regards,
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca

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

At Thu, 15 Oct 2020 17:32:10 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in

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.)

Sorry. Maybe I deleted wrong letters in the "reving" above.

====
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 remove it.)
====

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Thanks for all the suggestion, and new patch attached.

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.)

I think Andres is saying if we just log it in xlog_desc() then we can not get
the result for '--stats=record' case. And the patch solve the problem.

+ XLByteToSeg(record->EndRecPtr - 1, endSegNo, record->segcxt.ws_segsize);
We use XLByteToPrevSeg instead for this purpose.

Thanks and follow the suggestion.

You forgot to add a correction for short headers.

Infact, we need to consider this matter when the remain size of a wal
segment can not afford a XLogRecord struct for XLOG_SWITCH.
It's mean that if record->ReadRecPtr is on A wal segment, then
record->EndRecPtr is on (A+2) wal segment. So we need to minus
the longpagehead size on (A+1) wal segment.
In my thought we need not to care the short header, if my understand
is wrong?

+ 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.

Thanks and follow the suggestions.

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.

The patch really seems more clearly, but the new 'OTHERS' may confuse
the users and we hard to handle it with '--rmgr=RMGR' option. So I have
not use this design in this patch, let's see other's opinion.

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

Sorry, I remove the code, make sense we should discuss it in a
separate topic.

Regards,
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca

At Fri, 16 Oct 2020 16:21:47 +0800, "movead.li@highgo.ca" <movead.li@highgo.ca> wrote in

Thanks for all the suggestion, and new patch attached.

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.)

I think Andres is saying if we just log it in xlog_desc() then we can not get
the result for '--stats=record' case. And the patch solve the problem.

Mmm.

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.

It looks to me "We can know that length by subtracting the LSN of
XLOG_SWITCH from the next record's LSN so it doesn't add any
information."

+ XLByteToSeg(record->EndRecPtr - 1, endSegNo, record->segcxt.ws_segsize);
We use XLByteToPrevSeg instead for this purpose.

Thanks and follow the suggestion.

You forgot to add a correction for short headers.

Infact, we need to consider this matter when the remain size of a wal
segment can not afford a XLogRecord struct for XLOG_SWITCH.
It's mean that if record->ReadRecPtr is on A wal segment, then
record->EndRecPtr is on (A+2) wal segment. So we need to minus
the longpagehead size on (A+1) wal segment.
In my thought we need not to care the short header, if my understand
is wrong?

Maybe.

When a page doesn't have sufficient space for a record, the record is
split into to pieces and the last half is recorded after the header of
the next page. If it next page is in the next segment, the header is a
long header and a short header otherwise.

If it were the last page of a segment,

ReadRecPtr
v
<--- SEG A ------->|<---------- SEG A+1 ----------------->|<-SEG A+2
<XLOG_SWITCH_FIRST>|<LONG HEADER><XLOG_SWTICH_LAST><EMPTY>|<LONG HEADER>

So the length of <EMPTY> is:

LOC(SEG A+2) - ReadRecPtr - LEN(long header) - LEN(XLOG_SWITCH)

If not, that is, it were not the last page of a segment.

<-------------------- SEG A ---------------------------->|<-SEG A+1
< page n ->|<-- page n + 1 ---------->|....|<-last page->|<-first page
<X_S_FIRST>|<SHORT HEADER><X_S_LAST><EMPTY..............>|<LONG HEADER>

So the length of <EMPTY> in this case is:

LOC(SEG A+1) - ReadRecPtr - LEN(short header) - LEN(XLOG_SWITCH)

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.

The patch really seems more clearly, but the new 'OTHERS' may confuse
the users and we hard to handle it with '--rmgr=RMGR' option. So I have
not use this design in this patch, let's see other's opinion.

Yeah, I don't like the "OTHERS", too.

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

Sorry, I remove the code, make sense we should discuss it in a
separate topic.

Agreed.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

It looks to me "We can know that length by subtracting the LSN of
XLOG_SWITCH from the next record's LSN so it doesn't add any
information."

Sorry,maybe I miss this before.
But I think it will be better to show it clearly.

So the length of <EMPTY> in this case is:

LOC(SEG A+1) - ReadRecPtr - LEN(short header) - LEN(XLOG_SWITCH)

Thanks, I should not have missed this and fixed.

Regards,
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca

When I execute pg_waldump, I found that XLOG/SWITCH_JUNK appears twice.
Is this problem solved by the way of correcting the previously discussed Transaction/COMMIT?

$ ../bin/pg_waldump --stats=record ../data/pg_wal/000000010000000000000001
Type N (%) Record size (%) FPI size (%) Combined size (%)
---- - --- ----------- --- -------- --- ------------- ---
XLOG/CHECKPOINT_SHUTDOWN 5 ( 0.01) 570 ( 0.01) 0 ( 0.00) 570 ( 0.01)
XLOG/CHECKPOINT_ONLINE 6 ( 0.02) 684 ( 0.02) 0 ( 0.00) 684 ( 0.01)
XLOG/NEXTOID 3 ( 0.01) 90 ( 0.00) 0 ( 0.00) 90 ( 0.00)
XLOG/FPI 290 ( 0.80) 14210 ( 0.34) 638216 ( 40.72) 652426 ( 11.30)
Transaction/COMMIT 12 ( 0.03) 408 ( 0.01) 0 ( 0.00) 408 ( 0.01)
Transaction/COMMIT 496 ( 1.36) 134497 ( 3.20) 0 ( 0.00) 134497 ( 2.33)
Storage/CREATE 13 ( 0.04) 546 ( 0.01) 0 ( 0.00) 546 ( 0.01)
CLOG/ZEROPAGE 1 ( 0.00) 30 ( 0.00) 0 ( 0.00) 30 ( 0.00)
Database/CREATE 2 ( 0.01) 84 ( 0.00) 0 ( 0.00) 84 ( 0.00)
Standby/LOCK 142 ( 0.39) 5964 ( 0.14) 0 ( 0.00) 5964 ( 0.10)
Standby/RUNNING_XACTS 13 ( 0.04) 666 ( 0.02) 0 ( 0.00) 666 ( 0.01)
Standby/INVALIDATIONS 136 ( 0.37) 12416 ( 0.30) 0 ( 0.00) 12416 ( 0.22)
Heap2/CLEAN 132 ( 0.36) 8994 ( 0.21) 0 ( 0.00) 8994 ( 0.16)
Heap2/FREEZE_PAGE 245 ( 0.67) 168704 ( 4.01) 0 ( 0.00) 168704 ( 2.92)
Heap2/CLEANUP_INFO 2 ( 0.01) 84 ( 0.00) 0 ( 0.00) 84 ( 0.00)
Heap2/VISIBLE 424 ( 1.16) 25231 ( 0.60) 352256 ( 22.48) 377487 ( 6.54)
XLOG/SWITCH_JUNK 0 ( 0.00) 0 ( 0.00) 0 ( 0.00) 0 ( 0.00)
Heap2/MULTI_INSERT 1511 ( 4.15) 287727 ( 6.84) 12872 ( 0.82) 300599 ( 5.21)
Heap2/MULTI_INSERT+INIT 46 ( 0.13) 71910 ( 1.71) 0 ( 0.00) 71910 ( 1.25)
Heap/INSERT 8849 ( 24.31) 1288414 ( 30.62) 25648 ( 1.64) 1314062 ( 22.76)
Heap/DELETE 25 ( 0.07) 1350 ( 0.03) 0 ( 0.00) 1350 ( 0.02)
Heap/UPDATE 173 ( 0.48) 55238 ( 1.31) 5964 ( 0.38) 61202 ( 1.06)
Heap/HOT_UPDATE 257 ( 0.71) 27585 ( 0.66) 1300 ( 0.08) 28885 ( 0.50)
XLOG/SWITCH_JUNK 0 ( 0.00) 0 ( 0.00) 0 ( 0.00) 0 ( 0.00)
Heap/LOCK 180 ( 0.49) 9800 ( 0.23) 129812 ( 8.28) 139612 ( 2.42)
Heap/INPLACE 214 ( 0.59) 44520 ( 1.06) 40792 ( 2.60) 85312 ( 1.48)
Heap/INSERT+INIT 171 ( 0.47) 171318 ( 4.07) 0 ( 0.00) 171318 ( 2.97)

Regards,
Shinya Kato

Thanks for taking a look on this.

At Fri, 4 Dec 2020 04:20:47 +0000, <Shinya11.Kato@nttdata.com> wrote in

When I execute pg_waldump, I found that XLOG/SWITCH_JUNK appears twice.
Is this problem solved by the way of correcting the previously discussed Transaction/COMMIT?

$ ../bin/pg_waldump --stats=record ../data/pg_wal/000000010000000000000001
Type N (%) Record size (%) FPI size (%) Combined size (%)
---- - --- ----------- --- -------- --- ------------- ---

..

XLOG/SWITCH_JUNK 0 ( 0.00) 0 ( 0.00) 0 ( 0.00) 0 ( 0.00)

...

XLOG/SWITCH_JUNK 0 ( 0.00) 0 ( 0.00) 0 ( 0.00) 0 ( 0.00)

Yeah, that's because of XLogDumpDisplayStats forgets to consider ri
(rmgr id) when showing the lines. If there's a record with info = 0x04
for other resources than RM_XLOG_ID, the spurious line is shown.

The first one is for XLOG_HEAP2_VISIBLE and the latter is for
XLOG_HEAP_HOT_UPDATE, that is, both of which are not for XLOG_SWITCH..

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

Thanks for the reply. > Mr.Horiguchi.

I reviewed the patch and found some problems.

+ if(startSegNo != endSegNo)
+ else if(record->ReadRecPtr / XLOG_BLCKSZ !=
+ if(rmid == RM_XLOG_ID && info == XLOG_SWITCH)
+ if(ri == RM_XLOG_ID)
+ if(info == XLOG_SWITCH)

You need to put a space after the "if".

@@ -24,6 +24,7 @@
#include "common/logging.h"
#include "getopt_long.h"
#include "rmgrdesc.h"
+#include "catalog/pg_control.h"

I think the include statements should be arranged in alphabetical order.

+ info = (rj << 4) & ~XLR_INFO_MASK;
+ if(info == XLOG_SWITCH)
+ XLogDumpStatsRow(psprintf("XLOG/SWITCH_JUNK"),
+ 0, total_count, stats->junk_size, total_rec_len,
+ 0, total_fpi_len, stats->junk_size, total_len);

Can't be described in the same way as "XLogDumpStatsRow(psprintf("%s/%s", desc->rm_name, id)..."?
Only this part looks strange.

Why are the "count" and "fpi_len" fields 0?

I think you need to improve the duplicate output in column "XLOG/SWITCH_JUNK".

Regards,
Shinya Kato

Thanks for review, and sorry for reply so later.

I reviewed the patch and found some problems.

+ if(startSegNo != endSegNo)
+ else if(record->ReadRecPtr / XLOG_BLCKSZ !=
+ if(rmid == RM_XLOG_ID && info == XLOG_SWITCH)
+ if(ri == RM_XLOG_ID)
+ if(info == XLOG_SWITCH)

You need to put a space after the "if".

All fix and thanks for point the issue.

@@ -24,6 +24,7 @@
#include "common/logging.h"
#include "getopt_long.h"
#include "rmgrdesc.h"
+#include "catalog/pg_control.h"

I think the include statements should be arranged in alphabetical order.

Fix.

+ info = (rj << 4) & ~XLR_INFO_MASK;
+ if(info == XLOG_SWITCH)
+ XLogDumpStatsRow(psprintf("XLOG/SWITCH_JUNK"),
+ 0, total_count, stats->junk_size, total_rec_len,
+ 0, total_fpi_len, stats->junk_size, total_len);

Can't be described in the same way as "XLogDumpStatsRow(psprintf("%s/%s", desc->rm_name, id)..."?
Only this part looks strange.
Why are the "count" and "fpi_len" fields 0?

The 'SWITCH_JUNK' is not a real record and it relys on 'XLOG_SWITCH' record, so I think we can't count
'SWITCH_JUNK', so the "count" is 0. And it never contain FPI, so the "fpi_len" is 0.

But 0 value maybe looks strange, so in current version I show it like below:
Type N (%) Record size (%) FPI size (%) Combined size (%)
---- - --- ----------- --- -------- --- ------------- ---
...
XLOG/SWITCH_JUNK - ( -) 11006248 ( 72.26) - ( -) 11006248 ( 65.78)
Transaction/COMMIT 10 ( 0.03) 340 ( 0.00) 0 ( 0.00) 340 ( 0.00)

I think you need to improve the duplicate output in column "XLOG/SWITCH_JUNK".

Yes it's a bug and fixed.

Regards,
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
EMAIL: mailto:movead(dot)li(at)highgo(dot)ca

Thanks for review, and sorry for reply so later.

I reviewed the patch and found some problems.

+ if(startSegNo != endSegNo)
+ else if(record->ReadRecPtr / XLOG_BLCKSZ !=
+ if(rmid == RM_XLOG_ID && info == XLOG_SWITCH)
+ if(ri == RM_XLOG_ID)
+ if(info == XLOG_SWITCH)

You need to put a space after the "if".

All fix and thanks for point the issue.

@@ -24,6 +24,7 @@
#include "common/logging.h"
#include "getopt_long.h"
#include "rmgrdesc.h"
+#include "catalog/pg_control.h"

I think the include statements should be arranged in alphabetical order.

Fix.

Thank you for your revision!

+ info = (rj << 4) & ~XLR_INFO_MASK;
+ if(info == XLOG_SWITCH)
+ XLogDumpStatsRow(psprintf("XLOG/SWITCH_JUNK"),
+ 0, total_count, stats->junk_size, total_rec_len,
+ 0, total_fpi_len, stats->junk_size, total_len);

Can't be described in the same way as "XLogDumpStatsRow(psprintf("%s/%s", desc->rm_name, id)..."?
Only this part looks strange.
Why are the "count" and "fpi_len" fields 0?

The 'SWITCH_JUNK' is not a real record and it relys on 'XLOG_SWITCH' record, so I think we can't count
'SWITCH_JUNK', so the "count" is 0. And it never contain FPI, so the "fpi_len" is 0.

But 0 value maybe looks strange, so in current version I show it like below:
Type N (%) Record size (%) FPI size (%) Combined size (%)
---- - --- ----------- --- -------- --- ------------- ---
...
XLOG/SWITCH_JUNK - ( -) 11006248 ( 72.26) - ( -) 11006248 ( 65.78)
Transaction/COMMIT 10 ( 0.03) 340 ( 0.00) 0 ( 0.00) 340 ( 0.00)

I just wanted to know why the "count" and "fpi_len" fields 0 are.
So, it would be nice to have 0 values. Sorry for confusing you.

Regards,
Shinya Kato

On 1/7/21 2:55 AM, Shinya11.Kato@nttdata.com wrote:

But 0 value maybe looks strange, so in current version I show it like below:
Type N (%) Record size (%) FPI size (%) Combined size (%)
---- - --- ----------- --- -------- --- ------------- ---
...
XLOG/SWITCH_JUNK - ( -) 11006248 ( 72.26) - ( -) 11006248 ( 65.78)
Transaction/COMMIT 10 ( 0.03) 340 ( 0.00) 0 ( 0.00) 340 ( 0.00)

I just wanted to know why the "count" and "fpi_len" fields 0 are.
So, it would be nice to have 0 values. Sorry for confusing you.

Kato, it's not clear to me if you were asking for - to be changed back to 0?

You marked the patch as Ready for Committer so I assume not, but it
would be better to say clearly that you think this patch is ready for a
committer to look at.

Regards,
--
-David
david@pgmasters.net

But 0 value maybe looks strange, so in current version I show it like >below:
Type N (%) Record size (%) FPI size (%) Combined size (%)
---- - --- ----------- --- -------- --- ------------- --- ...
XLOG/SWITCH_JUNK - ( -) 11006248 ( 72.26) - ( -) 11006248 ( 65.78)
Transaction/COMMIT 10 ( 0.03) 340 ( 0.00) 0 ( 0.00) 340 ( 0.00)

I just wanted to know why the "count" and "fpi_len" fields 0 are.
So, it would be nice to have 0 values. Sorry for confusing you.

Kato, it's not clear to me if you were asking for - to be changed back to 0?

You marked the patch as Ready for Committer so I assume not, but it would be
better to say clearly that you think this patch is ready for a committer to look at.

Yes, I don't think it needs to be changed back to 0.
I think this patch is ready for a committer to look at.

Regards,
Shinya Kato

On 2021/03/19 15:06, Shinya11.Kato@nttdata.com wrote:

But 0 value maybe looks strange, so in current version I show it like >below:
Type N (%) Record size (%) FPI size (%) Combined size (%)
---- - --- ----------- --- -------- --- ------------- --- ...
XLOG/SWITCH_JUNK - ( -) 11006248 ( 72.26) - ( -) 11006248 ( 65.78)
Transaction/COMMIT 10 ( 0.03) 340 ( 0.00) 0 ( 0.00) 340 ( 0.00)

I just wanted to know why the "count" and "fpi_len" fields 0 are.
So, it would be nice to have 0 values. Sorry for confusing you.

Kato, it's not clear to me if you were asking for - to be changed back to 0?

You marked the patch as Ready for Committer so I assume not, but it would be
better to say clearly that you think this patch is ready for a committer to look at.

Yes, I don't think it needs to be changed back to 0.
I think this patch is ready for a committer to look at.

What's the use case of this feature? I read through this thread briefly,
but I'm still not sure how useful this feature is.

Horiguchi-san reported one issue upthread; --stats=record shows
two lines for Transaction/COMMIT record. Probably this should be
fixed separately.

Horiguchi-san,
Do you have updated version of that bug-fix patch?
Or you started another thread for that issue?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2021/03/19 18:27, Fujii Masao wrote:

On 2021/03/19 15:06, Shinya11.Kato@nttdata.com wrote:

But 0 value maybe looks strange, so in current version I show it like >below:
Type N (%) Record size (%) FPI size (%) Combined size (%)
---- - --- ----------- --- -------- --- ------------- --- ...
XLOG/SWITCH_JUNK - ( -) 11006248 ( 72.26) - ( -) 11006248 ( 65.78)
Transaction/COMMIT 10 ( 0.03) 340 ( 0.00) 0 ( 0.00) 340 ( 0.00)

I just wanted to know why the "count" and "fpi_len" fields 0 are.
So, it would be nice to have 0 values. Sorry for confusing you.

Kato, it's not clear to me if you were asking for - to be changed back to 0?

You marked the patch as Ready for Committer so I assume not, but it would be
better to say clearly that you think this patch is ready for a committer to look at.

Yes, I don't think it needs to be changed back to 0.
I think this patch is ready for a committer to look at.

What's the use case of this feature? I read through this thread briefly,
but I'm still not sure how useful this feature is.

Horiguchi-san reported one issue upthread; --stats=record shows
two lines for Transaction/COMMIT record. Probably this should be
fixed separately.

Horiguchi-san,
Do you have updated version of that bug-fix patch?
Or you started another thread for that issue?

I confirmed that only XACT records need to be processed differently.
So the patch that Horiguchi-san posted upthread looks good and enough
to me. I added a bit more detail information into the comment in the patch.
Attached is the updated version of the patch. Since this issue looks like
a bug, I'm thinking to back-patch that. Thought?

Barring any objection, I will commit this.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

-----Original Message-----
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Sent: Monday, March 22, 2021 11:22 AM
To: Shinya11.Kato@nttdata.com; david@pgmasters.net; movead.li@highgo.ca
Cc: pgsql-hackers@postgresql.org; andres@anarazel.de; michael@paquier.xyz;
ahsan.hadi@highgo.ca; horikyota.ntt@gmail.com
Subject: Re: Wrong statistics for size of XLOG_SWITCH during pg_waldump.

On 2021/03/19 18:27, Fujii Masao wrote:

On 2021/03/19 15:06, Shinya11.Kato@nttdata.com wrote:

But 0 value maybe looks strange, so in current version I show it like

below:

Type N (%) Record size (%) FPI size (%) Combined size (%)
---- - --- ----------- --- -------- --- ------------- --- ...
XLOG/SWITCH_JUNK - ( -) 11006248 ( 72.26) - ( -) 11006248 ( 65.78)
Transaction/COMMIT 10 ( 0.03) 340 ( 0.00) 0 ( 0.00) 340 ( 0.00)

I just wanted to know why the "count" and "fpi_len" fields 0 are.
So, it would be nice to have 0 values. Sorry for confusing you.

Kato, it's not clear to me if you were asking for - to be changed back to 0?

You marked the patch as Ready for Committer so I assume not, but it
would be better to say clearly that you think this patch is ready for a

committer to look at.

Yes, I don't think it needs to be changed back to 0.
I think this patch is ready for a committer to look at.

What's the use case of this feature? I read through this thread
briefly, but I'm still not sure how useful this feature is.

Horiguchi-san reported one issue upthread; --stats=record shows two
lines for Transaction/COMMIT record. Probably this should be fixed
separately.

Horiguchi-san,
Do you have updated version of that bug-fix patch?
Or you started another thread for that issue?

I confirmed that only XACT records need to be processed differently.
So the patch that Horiguchi-san posted upthread looks good and enough to me.
I added a bit more detail information into the comment in the patch.
Attached is the updated version of the patch. Since this issue looks like a bug,
I'm thinking to back-patch that. Thought?

Barring any objection, I will commit this.

I think it's good except for a typo "thoes four bits"

Regards,
Shinya Kato

On 2021/03/22 14:03, Shinya11.Kato@nttdata.com wrote:

Barring any objection, I will commit this.

I think it's good except for a typo "thoes four bits"

Thanks for the review! Attached is the updated version of the patch.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

At Mon, 22 Mar 2021 14:07:43 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in

On 2021/03/22 14:03, Shinya11.Kato@nttdata.com wrote:

Barring any objection, I will commit this.

I think it's good except for a typo "thoes four bits"

Thanks for the review! Attached is the updated version of the patch.

Thanks for picking it up. LGTM and applies cleanly.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 2021/03/22 17:49, Kyotaro Horiguchi wrote:

At Mon, 22 Mar 2021 14:07:43 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in

On 2021/03/22 14:03, Shinya11.Kato@nttdata.com wrote:

Barring any objection, I will commit this.

I think it's good except for a typo "thoes four bits"

Thanks for the review! Attached is the updated version of the patch.

Thanks for picking it up. LGTM and applies cleanly.

Pushed. Thanks!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION