New wal format distorts pg_xlogdump --stats
Hi,
The new WAL format calculates FPI vs plain record data like:
rec_len = XLogRecGetDataLen(record) + SizeOfXLogRecord;
fpi_len = record->decoded_record->xl_tot_len - rec_len;
Due to the amount of data now handled outside the main data portion ,
that doesn't seem correct to me. As an example, a couple thousand
inserts with full_page_writes=off now yields:
Type N (%) Record size (%) FPI size (%) Combined size (%)
---- - --- ----------- --- -------- --- ------------- ---
Heap/INSERT 30167 ( 99.53) 814509 ( 99.50) 965856 ( 99.54) 1780365 ( 99.52)
I think fpi_len now needs to only count the sum of the of the actual
length of block images, and all the rest needs to be rec_len.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 11/25/2014 05:36 AM, Andres Freund wrote:
Hi,
The new WAL format calculates FPI vs plain record data like:
rec_len = XLogRecGetDataLen(record) + SizeOfXLogRecord;
fpi_len = record->decoded_record->xl_tot_len - rec_len;Due to the amount of data now handled outside the main data portion ,
that doesn't seem correct to me. As an example, a couple thousand
inserts with full_page_writes=off now yields:Type N (%) Record size (%) FPI size (%) Combined size (%)
---- - --- ----------- --- -------- --- ------------- ---
Heap/INSERT 30167 ( 99.53) 814509 ( 99.50) 965856 ( 99.54) 1780365 ( 99.52)I think fpi_len now needs to only count the sum of the of the actual
length of block images, and all the rest needs to be rec_len.
Yeah, that's broken.
I propose the attached. Or does anyone want to argue for adding an
XLogRecGetFPILen() accessor macro for the hole_length in xlogreader.h.
It's not something a redo routine would need, nor most XLOG-reading
applications, so I thought it's better to just have pg_xlogdump peek
into the DecodedBkpBlock struct directly.
- Heikki
Attachments:
fix-fpi-calculation-1.patchtext/x-diff; name=fix-fpi-calculation-1.patchDownload
diff --git a/contrib/pg_xlogdump/pg_xlogdump.c b/contrib/pg_xlogdump/pg_xlogdump.c
index 26556dc..9f05e25 100644
--- a/contrib/pg_xlogdump/pg_xlogdump.c
+++ b/contrib/pg_xlogdump/pg_xlogdump.c
@@ -351,14 +351,29 @@ XLogDumpCountRecord(XLogDumpConfig *config, XLogDumpStats *stats,
uint8 recid;
uint32 rec_len;
uint32 fpi_len;
+ int block_id;
stats->count++;
- /* Update per-rmgr statistics */
-
rmid = XLogRecGetRmid(record);
rec_len = XLogRecGetDataLen(record) + SizeOfXLogRecord;
- fpi_len = record->decoded_record->xl_tot_len - rec_len;
+
+ /*
+ * Calculate the amount of FPI data in the record. Each backup block
+ * takes up BLCKSZ bytes, minus the "hole" length.
+ *
+ * XXX: We peek into xlogreader's private decoded backup blocks for the
+ * hole_length. 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 += BLCKSZ - record->blocks[block_id].hole_length;
+ }
+
+ /* Update per-rmgr statistics */
stats->rmgr_stats[rmid].count++;
stats->rmgr_stats[rmid].rec_len += rec_len;
On 2014-12-04 16:26:02 +0200, Heikki Linnakangas wrote:
Yeah, that's broken.
I propose the attached. Or does anyone want to argue for adding an
XLogRecGetFPILen() accessor macro for the hole_length in xlogreader.h. It's
not something a redo routine would need, nor most XLOG-reading applications,
so I thought it's better to just have pg_xlogdump peek into the
DecodedBkpBlock struct directly.
I think both would be justifyable, so I don't really care for now. One
slight reason for wrapping it in xlogreader.h is that we might add
compression of some form or another soon and it'd possibly be better to
have it in xlogreader.h so pg_xlogdump doesn't have to be changed. But
that's really rather minor.
Thanks,
Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Dec 5, 2014 at 8:09 AM, Andres Freund <andres@2ndquadrant.com>
wrote:
On 2014-12-04 16:26:02 +0200, Heikki Linnakangas wrote:
Yeah, that's broken.
I propose the attached. Or does anyone want to argue for adding an
XLogRecGetFPILen() accessor macro for the hole_length in xlogreader.h.It's
not something a redo routine would need, nor most XLOG-reading
applications,
so I thought it's better to just have pg_xlogdump peek into the
DecodedBkpBlock struct directly.I think both would be justifyable, so I don't really care for now. One
slight reason for wrapping it in xlogreader.h is that we might add
compression of some form or another soon and it'd possibly be better to
have it in xlogreader.h so pg_xlogdump doesn't have to be changed. But
that's really rather minor.
If we go this road and want to be complete, you may as well add access
macros for the image offset, the block image and for the block data stuff.
That would be handy and consistent with the rest, now both approaches are
fine as long as DecodedBkpBlock is in xlogreader.h.
Regards,
--
Michael
On 2014-12-05 08:58:33 +0900, Michael Paquier wrote:
On Fri, Dec 5, 2014 at 8:09 AM, Andres Freund <andres@2ndquadrant.com>
wrote:On 2014-12-04 16:26:02 +0200, Heikki Linnakangas wrote:
Yeah, that's broken.
I propose the attached. Or does anyone want to argue for adding an
XLogRecGetFPILen() accessor macro for the hole_length in xlogreader.h.It's
not something a redo routine would need, nor most XLOG-reading
applications,
so I thought it's better to just have pg_xlogdump peek into the
DecodedBkpBlock struct directly.I think both would be justifyable, so I don't really care for now. One
slight reason for wrapping it in xlogreader.h is that we might add
compression of some form or another soon and it'd possibly be better to
have it in xlogreader.h so pg_xlogdump doesn't have to be changed. But
that's really rather minor.If we go this road and want to be complete, you may as well add access
macros for the image offset, the block image and for the block data stuff.
That would be handy and consistent with the rest, now both approaches are
fine as long as DecodedBkpBlock is in xlogreader.h.
I don't see the point. Let's introduce that if (which I doubt a bit)
there's a user.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 12/05/2014 02:31 AM, Andres Freund wrote:
On 2014-12-05 08:58:33 +0900, Michael Paquier wrote:
On Fri, Dec 5, 2014 at 8:09 AM, Andres Freund <andres@2ndquadrant.com>
wrote:On 2014-12-04 16:26:02 +0200, Heikki Linnakangas wrote:
Yeah, that's broken.
I propose the attached. Or does anyone want to argue for adding an
XLogRecGetFPILen() accessor macro for the hole_length in xlogreader.h.It's
not something a redo routine would need, nor most XLOG-reading
applications,
so I thought it's better to just have pg_xlogdump peek into the
DecodedBkpBlock struct directly.I think both would be justifyable, so I don't really care for now. One
slight reason for wrapping it in xlogreader.h is that we might add
compression of some form or another soon and it'd possibly be better to
have it in xlogreader.h so pg_xlogdump doesn't have to be changed. But
that's really rather minor.
Good point about the compression patch.
I committed this as posted. It's not yet clear how the compression patch
is going to do the compression. If we just compress all records above a
certain size, pg_xlogdump should display the compressed and uncompressed
sizes separately, and an accessor macro for the backup block's size
would help much. The compression patch will almost certainly need to
modify pg_xlogdump --stats anyway, so there's not much point trying to
guess now what it's going to need.
If we go this road and want to be complete, you may as well add access
macros for the image offset, the block image and for the block data stuff.
That would be handy and consistent with the rest, now both approaches are
fine as long as DecodedBkpBlock is in xlogreader.h.I don't see the point. Let's introduce that if (which I doubt a bit)
there's a user.
Agreed.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers