From: movead.li@highgo.ca <movead.li@highgo.ca>
To: Horiguchi <horikyota.ntt@gmail.com>
Cc: andres <andres@anarazel.de>
Cc: michael <michael@paquier.xyz>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Cc: ahsan.hadi <ahsan.hadi@highgo.ca>
Subject: Re: Wrong statistics for size of XLOG_SWITCH during pg_waldump.
Date: Fri, 16 Oct 2020 16:21:47 +0800
Message-ID: <2020101616214467451338@highgo.ca> (raw)
References: <20201014204613.lxts3w6lc4lsv7zh@alap3.anarazel.de>
<20201015.114443.2231235644531652368.horikyota.ntt@gmail.com>
<2020101512560020200319@highgo.ca>
<20201015.173210.1595235318844221199.horikyota.ntt@gmail.com>
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
Attachments:fix_waldump_size_for_wal_switch_v4.patch (application/octet-stream, 4.5K) downloaddiff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.cindex 3200f777f5..2edc02fb72 100644--- a/src/backend/access/rmgrdesc/xlogdesc.c+++ b/src/backend/access/rmgrdesc/xlogdesc.c@@ -74,6 +74,13 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
memcpy(&nextOid, rec, sizeof(Oid));
appendStringInfo(buf, "%u", nextOid);
}
+ else if (info == XLOG_SWITCH)+ {+ uint32 junk_len;+ junk_len = xlog_switch_junk_len(record);++ appendStringInfo(buf, "trailing-bytes: %u", junk_len);+ }
else if (info == XLOG_RESTORE_POINT)
{
xl_restore_point *xlrec = (xl_restore_point *) rec;
@@ -142,6 +149,31 @@ xlog_desc(StringInfo buf, XLogReaderState *record)
}
}
+uint32+xlog_switch_junk_len(XLogReaderState *record)+{+ uint32 junk_len;+ XLogSegNo startSegNo;+ XLogSegNo endSegNo;++ XLByteToSeg(record->ReadRecPtr, startSegNo, record->segcxt.ws_segsize);+ XLByteToPrevSeg(record->EndRecPtr, endSegNo, record->segcxt.ws_segsize);++ junk_len = record->EndRecPtr - record->ReadRecPtr -+ XLogRecGetTotalLen(record);+ /*+ * If the wal switch record spread on two segments, we should extra minus+ * the long page head. I mean the case when the remain size of a wal+ * segment can not afford a XLogRecord struct for XLOG_SWITCH.+ */+ if(startSegNo != endSegNo)+ junk_len -= SizeOfXLogLongPHD;++ Assert(junk_len >= 0);++ return junk_len;+}+
const char *
xlog_identify(uint8 info)
{
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.cindex 31e99c2a6d..c6db93ff36 100644--- a/src/bin/pg_waldump/pg_waldump.c+++ b/src/bin/pg_waldump/pg_waldump.c@@ -24,6 +24,7 @@
#include "common/logging.h"
#include "getopt_long.h"
#include "rmgrdesc.h"
+#include "catalog/pg_control.h"
static const char *progname;
@@ -66,6 +67,7 @@ typedef struct Stats
typedef struct XLogDumpStats
{
uint64 count;
+ uint32 junk_size;
Stats rmgr_stats[RM_NEXT_ID];
Stats record_stats[RM_NEXT_ID][MAX_XLINFO_TYPES];
} XLogDumpStats;
@@ -416,12 +418,21 @@ XLogDumpCountRecord(XLogDumpConfig *config, XLogDumpStats *stats,
uint8 recid;
uint32 rec_len;
uint32 fpi_len;
+ uint8 info;
stats->count++;
rmid = XLogRecGetRmid(record);
+ info = XLogRecGetInfo(record) & ~XLR_INFO_MASK;
XLogDumpRecordLen(record, &rec_len, &fpi_len);
+ /* Add junk-space stats for XLOG_SWITCH */+ if(rmid == RM_XLOG_ID && info == XLOG_SWITCH)+ {+ uint32 junk_len = xlog_switch_junk_len(record);++ stats->junk_size += junk_len;+ }
/* Update per-rmgr statistics */
@@ -622,6 +633,7 @@ XLogDumpDisplayStats(XLogDumpConfig *config, XLogDumpStats *stats)
total_rec_len += stats->rmgr_stats[ri].rec_len;
total_fpi_len += stats->rmgr_stats[ri].fpi_len;
}
+ total_rec_len += stats->junk_size;
total_len = total_rec_len + total_fpi_len;
/*
@@ -652,12 +664,20 @@ XLogDumpDisplayStats(XLogDumpConfig *config, XLogDumpStats *stats)
XLogDumpStatsRow(desc->rm_name,
count, total_count, rec_len, total_rec_len,
fpi_len, total_fpi_len, tot_len, total_len);
+ if(ri == RM_XLOG_ID)+ {+ XLogDumpStatsRow(psprintf("XLOGSwitchJunk"),+ 0, total_count, stats->junk_size, total_rec_len,+ 0, total_fpi_len, stats->junk_size, total_len);+ }+
}
else
{
for (rj = 0; rj < MAX_XLINFO_TYPES; rj++)
{
const char *id;
+ uint8 info;
count = stats->record_stats[ri][rj].count;
rec_len = stats->record_stats[ri][rj].rec_len;
@@ -676,6 +696,12 @@ XLogDumpDisplayStats(XLogDumpConfig *config, XLogDumpStats *stats)
XLogDumpStatsRow(psprintf("%s/%s", desc->rm_name, id),
count, total_count, rec_len, total_rec_len,
fpi_len, total_fpi_len, tot_len, total_len);
+ 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);+
}
}
}
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.hindex 4146753d47..8cbc309108 100644--- a/src/include/access/xlog_internal.h+++ b/src/include/access/xlog_internal.h@@ -315,6 +315,7 @@ extern XLogRecPtr RequestXLogSwitch(bool mark_unimportant);
extern void GetOldestRestartPoint(XLogRecPtr *oldrecptr, TimeLineID *oldtli);
+extern uint32 xlog_switch_junk_len(XLogReaderState *record);
/*
* Exported for the functions in timeline.c and xlogarchive.c. Only valid
* in the startup process.
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: movead.li@highgo.ca, horikyota.ntt@gmail.com, andres@anarazel.de, michael@paquier.xyz, ahsan.hadi@highgo.ca
Subject: Re: Wrong statistics for size of XLOG_SWITCH during pg_waldump.
In-Reply-To: <2020101616214467451338@highgo.ca>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox