Thread: Make description of heap records more talkative for flags
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
Attachment
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
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
Attachment
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
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
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
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
Attachment
Re: Make description of heap records more talkative for flags
From
Jehan-Guillaume de Rorthais
Date:
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.
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
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
> 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
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
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: https://www.postgresql.org/message-id/20180423234928.GA1570%40paquier.xyz https://commitfest.postgresql.org/18/1633/ -- Michael
Attachment
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
> 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?
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
Attachment
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
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