Date: Sun, 4 Jun 2017 19:25:11 -0700
From: Andres Freund <andres@anarazel.de>
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: pgsql-bugs@postgresql.org, chjischj@163.com
Subject: Re: BUG #14687: pg_xlogdump does only count "main data" for
 record length and leading to incorrect statistics
Message-ID: <20170605022511.imow2t2ojsbauk72@alap3.anarazel.de>
References: <20170603165939.1436.58887@wrigleys.postgresql.org>
 <20170604231012.rkhlpf5hqiiszwmb@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="s7h4lh7oaiz2itqc"
Content-Disposition: inline
In-Reply-To: <20170604231012.rkhlpf5hqiiszwmb@alap3.anarazel.de>
Precedence: bulk
Sender: pgsql-bugs-owner@postgresql.org


--s7h4lh7oaiz2itqc
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi Heikki,

On 2017-06-04 16:10:12 -0700, Andres Freund wrote:
> On 2017-06-03 16:59:39 +0000, chjischj@163.com wrote:
> > Currently, record length in pg_xlogdump/pg_waldump's output only count
> > length of "main data",
> 
> Good catch!  Will fix.

I notice that this seems to originate in
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ebc2b681b8ffa597137aa7a19619e6a21176e928
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2c03216d831160bedd72d45f712601b6f7d03f1c
and that it's not just stats, but also plain xlogdump displaying wrong
data.

Do you see any better way to get at the !fpi len than to iterate over
the blocks and sum up the image length?  It's not really accurate to
display len (rec/tot) as total-fpi, total, but I don't have a better
backward-compatible idea?  Actually, I don't even have a better idea for
master, so maybe it's ok as proposed?

- Andres

--s7h4lh7oaiz2itqc
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="waldump.diff"

diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 6bc27f4be3..1fcce53379 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -363,6 +363,37 @@ XLogDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
 }
 
 /*
+ * Calculate the size of a record, split into !FPI and FPI parts.
+ */
+static void
+XLogDumpRecordLen(XLogReaderState *record, uint32 *rec_len, uint32 *fpi_len)
+{
+	int			block_id;
+
+	/*
+	 * Calculate the amount of FPI data in the record.
+	 *
+	 * XXX: We peek into xlogreader's private decoded backup blocks for the
+	 * bimg_len indicating the length of FPI data. It doesn't seem worth it to
+	 * add an accessor macro for this.
+	 */
+	*fpi_len = 0;
+	for (block_id = 0; block_id <= record->max_block_id; block_id++)
+	{
+		if (XLogRecHasBlockImage(record, block_id))
+			*fpi_len += record->blocks[block_id].bimg_len;
+	}
+
+	/*
+	 * Calculate the length of the record as the total length - the length of
+	 * all the block images.  That's commonly more accurate than just using
+	 * XLogRecGetDataLen(), because that's only the main record, not per-block
+	 * data registered wity XLogRegisterBufData().
+	 */
+	*rec_len = XLogRecGetTotalLen(record) - *fpi_len;
+}
+
+/*
  * Store per-rmgr and per-record statistics for a given record.
  */
 static void
@@ -373,26 +404,12 @@ XLogDumpCountRecord(XLogDumpConfig *config, XLogDumpStats *stats,
 	uint8		recid;
 	uint32		rec_len;
 	uint32		fpi_len;
-	int			block_id;
 
 	stats->count++;
 
 	rmid = XLogRecGetRmid(record);
-	rec_len = XLogRecGetDataLen(record) + SizeOfXLogRecord;
 
-	/*
-	 * Calculate the amount of FPI data in the record.
-	 *
-	 * XXX: We peek into xlogreader's private decoded backup blocks for the
-	 * bimg_len indicating the length of FPI data. It doesn't seem worth it to
-	 * add an accessor macro for this.
-	 */
-	fpi_len = 0;
-	for (block_id = 0; block_id <= record->max_block_id; block_id++)
-	{
-		if (XLogRecHasBlockImage(record, block_id))
-			fpi_len += record->blocks[block_id].bimg_len;
-	}
+	XLogDumpRecordLen(record, &rec_len, &fpi_len);
 
 	/* Update per-rmgr statistics */
 
@@ -422,6 +439,8 @@ XLogDumpDisplayRecord(XLogDumpConfig *config, XLogReaderState *record)
 {
 	const char *id;
 	const RmgrDescData *desc = &RmgrDescTable[XLogRecGetRmid(record)];
+	uint32		rec_len;
+	uint32		fpi_len;
 	RelFileNode rnode;
 	ForkNumber	forknum;
 	BlockNumber blk;
@@ -429,13 +448,15 @@ XLogDumpDisplayRecord(XLogDumpConfig *config, XLogReaderState *record)
 	uint8		info = XLogRecGetInfo(record);
 	XLogRecPtr	xl_prev = XLogRecGetPrev(record);
 
+	XLogDumpRecordLen(record, &rec_len, &fpi_len);
+
 	id = desc->rm_identify(info);
 	if (id == NULL)
 		id = psprintf("UNKNOWN (%x)", info & ~XLR_INFO_MASK);
 
 	printf("rmgr: %-11s len (rec/tot): %6u/%6u, tx: %10u, lsn: %X/%08X, prev %X/%08X, ",
 		   desc->rm_name,
-		   XLogRecGetDataLen(record), XLogRecGetTotalLen(record),
+		   rec_len, XLogRecGetTotalLen(record),
 		   XLogRecGetXid(record),
 		   (uint32) (record->ReadRecPtr >> 32), (uint32) record->ReadRecPtr,
 		   (uint32) (xl_prev >> 32), (uint32) xl_prev);

--s7h4lh7oaiz2itqc
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs

--s7h4lh7oaiz2itqc--