Make description of heap records more talkative for flags

Started by Michael Paquieralmost 8 years ago18 messages
#1Michael Paquier
michael@paquier.xyz
1 attachment(s)

Hi all,

I was just playing with the WAL consistency issue with rows moved across
partitions when I noticed that heapdesc.c is not really talkative about
the different flag records set.

What about something like the patch attached? I found that useful for
debugging.

(One comment of heapam_xlog.h mentions xl_heap_delete instead of
xl_heap_truncate, noticed it on the way.)

Thanks,
--
Michael

Attachments:

heapdesc-flags.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/access/rmgrdesc/heapdesc.c b/src/backend/access/rmgrdesc/heapdesc.c
index 318a281d7f..f03dbca55b 100644
--- a/src/backend/access/rmgrdesc/heapdesc.c
+++ b/src/backend/access/rmgrdesc/heapdesc.c
@@ -42,12 +42,32 @@ heap_desc(StringInfo buf, XLogReaderState *record)
 	{
 		xl_heap_insert *xlrec = (xl_heap_insert *) rec;
 
+		if (xlrec->flags & XLH_INSERT_ALL_VISIBLE_CLEARED)
+			appendStringInfo(buf, "all_visible_cleared ");
+		if (xlrec->flags & XLH_INSERT_LAST_IN_MULTI)
+			appendStringInfo(buf, "last_in_multi ");
+		if (xlrec->flags & XLH_INSERT_IS_SPECULATIVE)
+			appendStringInfo(buf, "is_speculative ");
+		if (xlrec->flags & XLH_INSERT_CONTAINS_NEW_TUPLE)
+			appendStringInfo(buf, "contains_new_tuple ");
+
 		appendStringInfo(buf, "off %u", xlrec->offnum);
 	}
 	else if (info == XLOG_HEAP_DELETE)
 	{
 		xl_heap_delete *xlrec = (xl_heap_delete *) rec;
 
+		if (xlrec->flags & XLH_DELETE_ALL_VISIBLE_CLEARED)
+			appendStringInfo(buf, "all_visible_cleared ");
+		if (xlrec->flags & XLH_DELETE_CONTAINS_OLD_TUPLE)
+			appendStringInfo(buf, "contains_old_tuple ");
+		if (xlrec->flags & XLH_DELETE_CONTAINS_OLD_KEY)
+			appendStringInfo(buf, "contains_old_key ");
+		if (xlrec->flags & XLH_DELETE_IS_SUPER)
+			appendStringInfo(buf, "is_super ");
+		if (xlrec->flags & XLH_DELETE_IS_PARTITION_MOVE)
+			appendStringInfo(buf, "is_partition_move ");
+
 		appendStringInfo(buf, "off %u ", xlrec->offnum);
 		out_infobits(buf, xlrec->infobits_set);
 	}
@@ -55,6 +75,21 @@ heap_desc(StringInfo buf, XLogReaderState *record)
 	{
 		xl_heap_update *xlrec = (xl_heap_update *) rec;
 
+		if (xlrec->flags & XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED)
+			appendStringInfo(buf, "old_all_visible_cleared ");
+		if (xlrec->flags & XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED)
+			appendStringInfo(buf, "new_all_visible_cleared ");
+		if (xlrec->flags & XLH_UPDATE_CONTAINS_OLD_TUPLE)
+			appendStringInfo(buf, "contains_old_tuple ");
+		if (xlrec->flags & XLH_UPDATE_CONTAINS_OLD_KEY)
+			appendStringInfo(buf, "contains_old_key ");
+		if (xlrec->flags & XLH_UPDATE_CONTAINS_NEW_TUPLE)
+			appendStringInfo(buf, "contains_new_tuple ");
+		if (xlrec->flags & XLH_UPDATE_PREFIX_FROM_OLD)
+			appendStringInfo(buf, "prefix_from_old ");
+		if (xlrec->flags & XLH_UPDATE_SUFFIX_FROM_OLD)
+			appendStringInfo(buf, "suffix_from_old ");
+
 		appendStringInfo(buf, "off %u xmax %u ",
 						 xlrec->old_offnum,
 						 xlrec->old_xmax);
@@ -146,6 +181,15 @@ heap2_desc(StringInfo buf, XLogReaderState *record)
 	{
 		xl_heap_multi_insert *xlrec = (xl_heap_multi_insert *) rec;
 
+		if (xlrec->flags & XLH_INSERT_ALL_VISIBLE_CLEARED)
+			appendStringInfo(buf, "all_visible_cleared ");
+		if (xlrec->flags & XLH_INSERT_LAST_IN_MULTI)
+			appendStringInfo(buf, "last_in_multi ");
+		if (xlrec->flags & XLH_INSERT_IS_SPECULATIVE)
+			appendStringInfo(buf, "is_speculative ");
+		if (xlrec->flags & XLH_INSERT_CONTAINS_NEW_TUPLE)
+			appendStringInfo(buf, "contains_new_tuple ");
+
 		appendStringInfo(buf, "%d tuples", xlrec->ntuples);
 	}
 	else if (info == XLOG_HEAP2_LOCK_UPDATED)
diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.h
index cf88ff7cb4..cc85f4b3c7 100644
--- a/src/include/access/heapam_xlog.h
+++ b/src/include/access/heapam_xlog.h
@@ -111,7 +111,7 @@ typedef struct xl_heap_delete
 #define SizeOfHeapDelete	(offsetof(xl_heap_delete, flags) + sizeof(uint8))
 
 /*
- * xl_heap_delete flag values, 8 bits are available.
+ * xl_heap_truncate flag values, 8 bits are available.
  */
 #define XLH_TRUNCATE_CASCADE					(1<<0)
 #define XLH_TRUNCATE_RESTART_SEQS				(1<<1)
#2Andres Freund
andres@anarazel.de
In reply to: Michael Paquier (#1)
Re: Make description of heap records more talkative for flags

On 2018-04-13 12:47:34 +0900, Michael Paquier wrote:

Hi all,

I was just playing with the WAL consistency issue with rows moved across
partitions when I noticed that heapdesc.c is not really talkative about
the different flag records set.

What about something like the patch attached? I found that useful for
debugging.

(One comment of heapam_xlog.h mentions xl_heap_delete instead of
xl_heap_truncate, noticed it on the way.)

OTOH, that also kinda bloats the output noticeably... I'm somewhat
inclined to just put the hex value or such there?

Greetings,

Andres Freund

#3Michael Paquier
michael@paquier.xyz
In reply to: Andres Freund (#2)
Re: Make description of heap records more talkative for flags

On Thu, Apr 12, 2018 at 08:49:03PM -0700, Andres Freund wrote:

OTOH, that also kinda bloats the output noticeably... I'm somewhat
inclined to just put the hex value or such there?

That would do as well for me.
--
Michael

#4Alvaro Herrera
alvherre@alvh.no-ip.org
In reply to: Michael Paquier (#3)
Re: Make description of heap records more talkative for flags

Michael Paquier wrote:

On Thu, Apr 12, 2018 at 08:49:03PM -0700, Andres Freund wrote:

OTOH, that also kinda bloats the output noticeably... I'm somewhat
inclined to just put the hex value or such there?

That would do as well for me.

Me too. Should we consider this for pg11? My vote is yes, with no
backpatch (might break scripts reading pg_{wal,xlog}dump output.
Though, really!?).

Please submit a patch implementing this.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#5Andres Freund
andres@anarazel.de
In reply to: Alvaro Herrera (#4)
Re: Make description of heap records more talkative for flags

Hi,

On 2018-04-23 12:37:20 -0300, Alvaro Herrera wrote:

Michael Paquier wrote:

On Thu, Apr 12, 2018 at 08:49:03PM -0700, Andres Freund wrote:

OTOH, that also kinda bloats the output noticeably... I'm somewhat
inclined to just put the hex value or such there?

That would do as well for me.

Me too. Should we consider this for pg11? My vote is yes, with no
backpatch (might break scripts reading pg_{wal,xlog}dump output.
Though, really!?).

It's low risk enough, but why should we try to get it into v11? Flags
been around for years now.

Greetings,

Andres Freund

#6Alvaro Herrera
alvherre@alvh.no-ip.org
In reply to: Andres Freund (#5)
Re: Make description of heap records more talkative for flags

Hello

Andres Freund wrote:

On 2018-04-23 12:37:20 -0300, Alvaro Herrera wrote:

Michael Paquier wrote:

On Thu, Apr 12, 2018 at 08:49:03PM -0700, Andres Freund wrote:

OTOH, that also kinda bloats the output noticeably... I'm
somewhat inclined to just put the hex value or such there?

That would do as well for me.

Me too. Should we consider this for pg11? My vote is yes, with no
backpatch (might break scripts reading pg_{wal,xlog}dump output.
Though, really!?).

It's low risk enough, but why should we try to get it into v11? Flags
been around for years now.

I was thinking the newly added flags would make for possibly more
confusing scenarios, but now that I look again, the new ones are XLHL
flags not the XLOG flags being handled in this patch.

Now, frankly, this being mostly a debugging tool, I think it would be
better to have the output as complete as we can. Otherwise, when
debugging some hideous problem we find ourselves patching the tool
during an emergency in order to figure out what is going on. For this,
I would rather patch the earliest version we conveniently can. We
cannot patch pg10 because it's already out there, but not so for pg11.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#7Michael Paquier
michael@paquier.xyz
In reply to: Alvaro Herrera (#6)
1 attachment(s)
Re: Make description of heap records more talkative for flags

On Mon, Apr 23, 2018 at 01:02:16PM -0300, Alvaro Herrera wrote:

Andres Freund wrote:

On 2018-04-23 12:37:20 -0300, Alvaro Herrera wrote:

Me too. Should we consider this for pg11? My vote is yes, with no
backpatch (might break scripts reading pg_{wal,xlog}dump output.
Though, really!?).

Breaking the output format is exactly one reason to not patch v10 and
older versions.

It's low risk enough, but why should we try to get it into v11? Flags
been around for years now.

I was thinking the newly added flags would make for possibly more
confusing scenarios, but now that I look again, the new ones are XLHL
flags not the XLOG flags being handled in this patch.

Now, frankly, this being mostly a debugging tool, I think it would be
better to have the output as complete as we can. Otherwise, when
debugging some hideous problem we find ourselves patching the tool
during an emergency in order to figure out what is going on. For this,
I would rather patch the earliest version we conveniently can. We
cannot patch pg10 because it's already out there, but not so for pg11.

OK, that works for me. Attached is a patch using hex values for the
flags of all those records (the comment could be fixed further down but
let's not bother about it). There are a couple of things I noticed on
the way:
1) xl_heap_lock prints flags, but not in hexa but using %u.
2) xl_heap_visible prints flags, not in hexa but using %d.
3) xl_heap_lock_updated prints flags, not in hexa but using %u.
4) xl_hash_split_allocate_page has flags, those are handled as directly
text (see hashdesc.c).
5) Most GIN records also have flags, which are handled.
6) xl_smgr_truncate as records, which are not showed up yet.
So I have hacked the patch so as all flags show up in hexa format,
except those which are already handled and print text.
--
Michael

Attachments:

waldump-hexa-flags.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/access/rmgrdesc/heapdesc.c b/src/backend/access/rmgrdesc/heapdesc.c
index 318a281d7f..f63b8f844d 100644
--- a/src/backend/access/rmgrdesc/heapdesc.c
+++ b/src/backend/access/rmgrdesc/heapdesc.c
@@ -42,22 +42,26 @@ heap_desc(StringInfo buf, XLogReaderState *record)
 	{
 		xl_heap_insert *xlrec = (xl_heap_insert *) rec;
 
-		appendStringInfo(buf, "off %u", xlrec->offnum);
+		appendStringInfo(buf, "off %u flags %02X", xlrec->offnum,
+						 xlrec->flags);
 	}
 	else if (info == XLOG_HEAP_DELETE)
 	{
 		xl_heap_delete *xlrec = (xl_heap_delete *) rec;
 
-		appendStringInfo(buf, "off %u ", xlrec->offnum);
+		appendStringInfo(buf, "off %u flags %02X ",
+						 xlrec->offnum,
+						 xlrec->flags);
 		out_infobits(buf, xlrec->infobits_set);
 	}
 	else if (info == XLOG_HEAP_UPDATE)
 	{
 		xl_heap_update *xlrec = (xl_heap_update *) rec;
 
-		appendStringInfo(buf, "off %u xmax %u ",
+		appendStringInfo(buf, "off %u xmax %u flags %02X ",
 						 xlrec->old_offnum,
-						 xlrec->old_xmax);
+						 xlrec->old_xmax,
+						 xlrec->flags);
 		out_infobits(buf, xlrec->old_infobits_set);
 		appendStringInfo(buf, "; new off %u xmax %u",
 						 xlrec->new_offnum,
@@ -67,9 +71,10 @@ heap_desc(StringInfo buf, XLogReaderState *record)
 	{
 		xl_heap_update *xlrec = (xl_heap_update *) rec;
 
-		appendStringInfo(buf, "off %u xmax %u ",
+		appendStringInfo(buf, "off %u xmax %u flags %02X ",
 						 xlrec->old_offnum,
-						 xlrec->old_xmax);
+						 xlrec->old_xmax,
+						 xlrec->flags);
 		out_infobits(buf, xlrec->old_infobits_set);
 		appendStringInfo(buf, "; new off %u xmax %u",
 						 xlrec->new_offnum,
@@ -98,7 +103,7 @@ heap_desc(StringInfo buf, XLogReaderState *record)
 	{
 		xl_heap_lock *xlrec = (xl_heap_lock *) rec;
 
-		appendStringInfo(buf, "off %u: xid %u: flags %u ",
+		appendStringInfo(buf, "off %u: xid %u: flags %02X ",
 						 xlrec->offnum, xlrec->locking_xid, xlrec->flags);
 		out_infobits(buf, xlrec->infobits_set);
 	}
@@ -139,20 +144,21 @@ heap2_desc(StringInfo buf, XLogReaderState *record)
 	{
 		xl_heap_visible *xlrec = (xl_heap_visible *) rec;
 
-		appendStringInfo(buf, "cutoff xid %u flags %d",
+		appendStringInfo(buf, "cutoff xid %u flags %02X",
 						 xlrec->cutoff_xid, xlrec->flags);
 	}
 	else if (info == XLOG_HEAP2_MULTI_INSERT)
 	{
 		xl_heap_multi_insert *xlrec = (xl_heap_multi_insert *) rec;
 
-		appendStringInfo(buf, "%d tuples", xlrec->ntuples);
+		appendStringInfo(buf, "%d tuples flags %02X", xlrec->ntuples,
+						 xlrec->flags);
 	}
 	else if (info == XLOG_HEAP2_LOCK_UPDATED)
 	{
 		xl_heap_lock_updated *xlrec = (xl_heap_lock_updated *) rec;
 
-		appendStringInfo(buf, "off %u: xmax %u: flags %u ",
+		appendStringInfo(buf, "off %u: xmax %u: flags %02X ",
 						 xlrec->offnum, xlrec->xmax, xlrec->flags);
 		out_infobits(buf, xlrec->infobits_set);
 	}
diff --git a/src/backend/access/rmgrdesc/smgrdesc.c b/src/backend/access/rmgrdesc/smgrdesc.c
index df1ad38b5a..604604ac04 100644
--- a/src/backend/access/rmgrdesc/smgrdesc.c
+++ b/src/backend/access/rmgrdesc/smgrdesc.c
@@ -36,7 +36,7 @@ smgr_desc(StringInfo buf, XLogReaderState *record)
 		xl_smgr_truncate *xlrec = (xl_smgr_truncate *) rec;
 		char	   *path = relpathperm(xlrec->rnode, MAIN_FORKNUM);
 
-		appendStringInfo(buf, "%s to %u blocks flags %d", path,
+		appendStringInfo(buf, "%s to %u blocks flags %02X", path,
 						 xlrec->blkno, xlrec->flags);
 		pfree(path);
 	}
diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.h
index cf88ff7cb4..cc85f4b3c7 100644
--- a/src/include/access/heapam_xlog.h
+++ b/src/include/access/heapam_xlog.h
@@ -111,7 +111,7 @@ typedef struct xl_heap_delete
 #define SizeOfHeapDelete	(offsetof(xl_heap_delete, flags) + sizeof(uint8))
 
 /*
- * xl_heap_delete flag values, 8 bits are available.
+ * xl_heap_truncate flag values, 8 bits are available.
  */
 #define XLH_TRUNCATE_CASCADE					(1<<0)
 #define XLH_TRUNCATE_RESTART_SEQS				(1<<1)
In reply to: Alvaro Herrera (#4)
Re: Make description of heap records more talkative for flags

Hi all,

Bellow a 1¢ on feedback from a side project about this.

On Mon, 23 Apr 2018 12:37:20 -0300
Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Michael Paquier wrote:

On Thu, Apr 12, 2018 at 08:49:03PM -0700, Andres Freund wrote:

OTOH, that also kinda bloats the output noticeably... I'm somewhat
inclined to just put the hex value or such there?

That would do as well for me.

Me too. Should we consider this for pg11? My vote is yes, with no
backpatch (might break scripts reading pg_{wal,xlog}dump output.
Though, really!?).

Yes, we are using pg_{wal,xlog}dump in PAF project to check a controlled
switchover is safe.

We use it to check that the designated standby to promote received the shutdown
checkpoint from the old master when PAF shut it down before starting it again
as a regular standby. It allows us to safely hook the old master as a standby
on the new master without xact loss and/or using pg_rewind.

I suppose we can deal with the format output change though.

#9Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#6)
Re: Make description of heap records more talkative for flags

On 2018-Apr-23, Alvaro Herrera wrote:

Now, frankly, this being mostly a debugging tool, I think it would be
better to have the output as complete as we can. Otherwise, when
debugging some hideous problem we find ourselves patching the tool
during an emergency in order to figure out what is going on. For this,
I would rather patch the earliest version we conveniently can. We
cannot patch pg10 because it's already out there, but not so for pg11.

RMT, I would like your opinion on whether to apply a fix for this
problem to pg11 or not.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#10Andres Freund
andres@anarazel.de
In reply to: Alvaro Herrera (#9)
Re: Make description of heap records more talkative for flags

On 2018-05-15 13:44:58 -0400, Alvaro Herrera wrote:

On 2018-Apr-23, Alvaro Herrera wrote:

Now, frankly, this being mostly a debugging tool, I think it would be
better to have the output as complete as we can. Otherwise, when
debugging some hideous problem we find ourselves patching the tool
during an emergency in order to figure out what is going on. For this,
I would rather patch the earliest version we conveniently can. We
cannot patch pg10 because it's already out there, but not so for pg11.

RMT, I would like your opinion on whether to apply a fix for this
problem to pg11 or not.

-0.1. We've lived without this for years, I fail to see why this should
get an exception / has any sort of urgency. We could establish a policy
that we just exclude parts of the source tree from the code freeze, but
we haven't so far.

Greetings,

Andres Freund

#11Jonathan S. Katz
jkatz@postgresql.org
In reply to: Andres Freund (#10)
Re: Make description of heap records more talkative for flags

On May 15, 2018, at 12:50 PM, Andres Freund <andres@anarazel.de> wrote:

On 2018-05-15 13:44:58 -0400, Alvaro Herrera wrote:

On 2018-Apr-23, Alvaro Herrera wrote:

Now, frankly, this being mostly a debugging tool, I think it would be
better to have the output as complete as we can. Otherwise, when
debugging some hideous problem we find ourselves patching the tool
during an emergency in order to figure out what is going on. For this,
I would rather patch the earliest version we conveniently can. We
cannot patch pg10 because it's already out there, but not so for pg11.

RMT, I would like your opinion on whether to apply a fix for this
problem to pg11 or not.

-0.1. We've lived without this for years, I fail to see why this should
get an exception / has any sort of urgency. We could establish a policy
that we just exclude parts of the source tree from the code freeze, but
we haven't so far.

Per Alvaro’s above comment, if this is something low-risk that could prevent
the “emergency patch” scenario later on when we actually need such
debugging flags in place, I would be ok with getting this in prior to Beta 1.
After Beta 1 I would be more risk adverse and go with Andres’s statement
around urgency and wait until the PG12 commit cycle.

And of course, we should come up with a policy, too, but I think we could
kick that can just a bit farther down the road for roughly 2 more weeks.

So +1 if we can commit this prior to Beta 1.
-1 if it waits until after.

Jonathan

#12Andres Freund
andres@anarazel.de
In reply to: Jonathan S. Katz (#11)
Re: Make description of heap records more talkative for flags

On 2018-05-15 16:28:23 -0500, Jonathan S. Katz wrote:

On May 15, 2018, at 12:50 PM, Andres Freund <andres@anarazel.de> wrote:

On 2018-05-15 13:44:58 -0400, Alvaro Herrera wrote:

On 2018-Apr-23, Alvaro Herrera wrote:

Now, frankly, this being mostly a debugging tool, I think it would be
better to have the output as complete as we can. Otherwise, when
debugging some hideous problem we find ourselves patching the tool
during an emergency in order to figure out what is going on. For this,
I would rather patch the earliest version we conveniently can. We
cannot patch pg10 because it's already out there, but not so for pg11.

RMT, I would like your opinion on whether to apply a fix for this
problem to pg11 or not.

-0.1. We've lived without this for years, I fail to see why this should
get an exception / has any sort of urgency. We could establish a policy
that we just exclude parts of the source tree from the code freeze, but
we haven't so far.

Per Alvaro’s above comment, if this is something low-risk that could prevent
the “emergency patch” scenario later on when we actually need such
debugging flags in place, I would be ok with getting this in prior to
Beta 1.

I have an *extremely* hard time that'd ever be the case. And it'd not
really change much, given several years of back branch releases.
Anyway, I'm only -0.1 here.

Greetings,

Andres Freund

#13Michael Paquier
michael@paquier.xyz
In reply to: Andres Freund (#12)
Re: Make description of heap records more talkative for flags

On Tue, May 15, 2018 at 02:32:55PM -0700, Andres Freund wrote:

I have an *extremely* hard time that'd ever be the case. And it'd not
really change much, given several years of back branch releases.
Anyway, I'm only -0.1 here.

I personally don't care much if this gets in v11 or later in v12 as
generally bug fixes are not branch-specific and can be found on master.
For now, I am just letting this version of the patch sit in the CF:
/messages/by-id/20180423234928.GA1570@paquier.xyz
https://commitfest.postgresql.org/18/1633/
--
Michael

#14Nathan Bossart
bossartn@amazon.com
In reply to: Michael Paquier (#13)
Re: Make description of heap records more talkative for flags

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

-		appendStringInfo(buf, "off %u", xlrec->offnum);
+		appendStringInfo(buf, "off %u flags %02X", xlrec->offnum,
+						 xlrec->flags);

Can we add "0x" before the flags so that it is abundantly clear it's a
hex value?

Nathan

#15Dmitry Dolgov
9erthalion6@gmail.com
In reply to: Nathan Bossart (#14)
Re: Make description of heap records more talkative for flags

On Fri, 15 Jun 2018 at 17:09, Nathan Bossart <bossartn@amazon.com> wrote:

The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

-               appendStringInfo(buf, "off %u", xlrec->offnum);
+               appendStringInfo(buf, "off %u flags %02X", xlrec->offnum,
+                                                xlrec->flags);

Can we add "0x" before the flags so that it is abundantly clear it's a
hex value?

This thread is being inactive for quite some time. As far as I understand, the
agreement was to add it in the PG12 commit cycle, so probably it's a good time
to do so. Any plans about that?

#16Michael Paquier
michael@paquier.xyz
In reply to: Dmitry Dolgov (#15)
1 attachment(s)
Re: Make description of heap records more talkative for flags

On Fri, Nov 09, 2018 at 04:23:57PM +0100, Dmitry Dolgov wrote:

This thread is being inactive for quite some time. As far as I understand, the
agreement was to add it in the PG12 commit cycle, so probably it's a good time
to do so. Any plans about that?

This thread needed a new patch to answer to the previous comment from
Nathan, and it was not marked as waiting on author. Sorry I got
confused with the CF status and I have missed Nathan's update, which
mentions a good idea. So attached is an updated patch which adds "0x"
in front of each flag output.

I have committed the fix for the incorrect routine name in xlog_heapam.h
separately.
--
Michael

Attachments:

waldump-hexa-flags-v2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/access/rmgrdesc/heapdesc.c b/src/backend/access/rmgrdesc/heapdesc.c
index 318a281d7f..87852151bd 100644
--- a/src/backend/access/rmgrdesc/heapdesc.c
+++ b/src/backend/access/rmgrdesc/heapdesc.c
@@ -42,22 +42,26 @@ heap_desc(StringInfo buf, XLogReaderState *record)
 	{
 		xl_heap_insert *xlrec = (xl_heap_insert *) rec;
 
-		appendStringInfo(buf, "off %u", xlrec->offnum);
+		appendStringInfo(buf, "off %u flags 0x%02X", xlrec->offnum,
+						 xlrec->flags);
 	}
 	else if (info == XLOG_HEAP_DELETE)
 	{
 		xl_heap_delete *xlrec = (xl_heap_delete *) rec;
 
-		appendStringInfo(buf, "off %u ", xlrec->offnum);
+		appendStringInfo(buf, "off %u flags 0x%02X ",
+						 xlrec->offnum,
+						 xlrec->flags);
 		out_infobits(buf, xlrec->infobits_set);
 	}
 	else if (info == XLOG_HEAP_UPDATE)
 	{
 		xl_heap_update *xlrec = (xl_heap_update *) rec;
 
-		appendStringInfo(buf, "off %u xmax %u ",
+		appendStringInfo(buf, "off %u xmax %u flags 0x%02X ",
 						 xlrec->old_offnum,
-						 xlrec->old_xmax);
+						 xlrec->old_xmax,
+						 xlrec->flags);
 		out_infobits(buf, xlrec->old_infobits_set);
 		appendStringInfo(buf, "; new off %u xmax %u",
 						 xlrec->new_offnum,
@@ -67,9 +71,10 @@ heap_desc(StringInfo buf, XLogReaderState *record)
 	{
 		xl_heap_update *xlrec = (xl_heap_update *) rec;
 
-		appendStringInfo(buf, "off %u xmax %u ",
+		appendStringInfo(buf, "off %u xmax %u flags 0x%02X ",
 						 xlrec->old_offnum,
-						 xlrec->old_xmax);
+						 xlrec->old_xmax,
+						 xlrec->flags);
 		out_infobits(buf, xlrec->old_infobits_set);
 		appendStringInfo(buf, "; new off %u xmax %u",
 						 xlrec->new_offnum,
@@ -98,7 +103,7 @@ heap_desc(StringInfo buf, XLogReaderState *record)
 	{
 		xl_heap_lock *xlrec = (xl_heap_lock *) rec;
 
-		appendStringInfo(buf, "off %u: xid %u: flags %u ",
+		appendStringInfo(buf, "off %u: xid %u: flags 0x%02X ",
 						 xlrec->offnum, xlrec->locking_xid, xlrec->flags);
 		out_infobits(buf, xlrec->infobits_set);
 	}
@@ -139,20 +144,21 @@ heap2_desc(StringInfo buf, XLogReaderState *record)
 	{
 		xl_heap_visible *xlrec = (xl_heap_visible *) rec;
 
-		appendStringInfo(buf, "cutoff xid %u flags %d",
+		appendStringInfo(buf, "cutoff xid %u flags 0x%02X",
 						 xlrec->cutoff_xid, xlrec->flags);
 	}
 	else if (info == XLOG_HEAP2_MULTI_INSERT)
 	{
 		xl_heap_multi_insert *xlrec = (xl_heap_multi_insert *) rec;
 
-		appendStringInfo(buf, "%d tuples", xlrec->ntuples);
+		appendStringInfo(buf, "%d tuples flags 0x%02X", xlrec->ntuples,
+						 xlrec->flags);
 	}
 	else if (info == XLOG_HEAP2_LOCK_UPDATED)
 	{
 		xl_heap_lock_updated *xlrec = (xl_heap_lock_updated *) rec;
 
-		appendStringInfo(buf, "off %u: xmax %u: flags %u ",
+		appendStringInfo(buf, "off %u: xmax %u: flags 0x%02X ",
 						 xlrec->offnum, xlrec->xmax, xlrec->flags);
 		out_infobits(buf, xlrec->infobits_set);
 	}
diff --git a/src/backend/access/rmgrdesc/smgrdesc.c b/src/backend/access/rmgrdesc/smgrdesc.c
index df1ad38b5a..7ad9ec08f9 100644
--- a/src/backend/access/rmgrdesc/smgrdesc.c
+++ b/src/backend/access/rmgrdesc/smgrdesc.c
@@ -36,7 +36,7 @@ smgr_desc(StringInfo buf, XLogReaderState *record)
 		xl_smgr_truncate *xlrec = (xl_smgr_truncate *) rec;
 		char	   *path = relpathperm(xlrec->rnode, MAIN_FORKNUM);
 
-		appendStringInfo(buf, "%s to %u blocks flags %d", path,
+		appendStringInfo(buf, "%s to %u blocks flags 0x%02X", path,
 						 xlrec->blkno, xlrec->flags);
 		pfree(path);
 	}
#17Bossart, Nathan
bossartn@amazon.com
In reply to: Michael Paquier (#16)
Re: Make description of heap records more talkative for flags

On 11/9/18, 6:11 PM, "Michael Paquier" <michael@paquier.xyz> wrote:

This thread needed a new patch to answer to the previous comment from
Nathan, and it was not marked as waiting on author. Sorry I got
confused with the CF status and I have missed Nathan's update, which
mentions a good idea. So attached is an updated patch which adds "0x"
in front of each flag output.

Thanks for the updated patch. It looks good to me.

Nathan

#18Michael Paquier
michael@paquier.xyz
In reply to: Bossart, Nathan (#17)
Re: Make description of heap records more talkative for flags

On Tue, Nov 13, 2018 at 04:04:29PM +0000, Bossart, Nathan wrote:

Thanks for the updated patch. It looks good to me.

Thanks for double-checking. Committed.
--
Michael