Re: wal_dump output on CREATE DATABASE

On 11/16/18 12:05 PM, Jean-Christophe Arnu wrote:
>
>
> Le jeu. 15 nov. 2018 à 19:44, Robert Haas <robertmhaas(at)gmail(dot)com
> <mailto:robertmhaas(at)gmail(dot)com>> a écrit :
>
> On Tue, Nov 13, 2018 at 3:40 PM Tomas Vondra
> <tomas(dot)vondra(at)2ndquadrant(dot)com <mailto:tomas(dot)vondra(at)2ndquadrant(dot)com>>
> wrote:
> > People reading pg_waldump output quickly learn to read the A/B/C
> format
> > and what those fields mean. Breaking that into ts=A db=B
> relfilenode=C
> > does not make that particularly clearer or easier to read. I'd
> say it'd
> > also makes it harder to parse, and it increases the size of the
> output
> > (both in terms of line length and data size).
>
> I agree.
>
>
> First, thank you all for your reviews.
>
> I also agree that the A/B/C format is right (and it may be a good thing
> to document it, maybe by adding some changes in the
> doc/src/sgml/ref/pg_waldump.sgml file to this patch).
>
> To reply to Andres, I agree we should not change things for a target
> format that would not fit clearly defined syntax. In that way, I agree
> with Tomas on the fact that people reading
> pg_waldump output are quickly familiar with the A/B/C notation.
>
> My first use case was to decode the ids with a processing script to
> identify each id in A/B/C or pg_waldump output with a "human readable"
> item. For this, my processing script connects the cluster and tries
> resolve the ids with simple queries (and building a local cache for
> this). Then it replaces each looked up id item with its corresponding text.
> In some cases, this could be useful for DBA to find more easily when a
> specific relation was modified (searching for DELETE BTW). But that's
> only my use case and my little script.
>
> Going back to the code :
>
> As I can figure by crawling the source tree (and discovering it) there
> are messages with :
>   * A/B/C notation which seems to be the one we should adopt ( meaning
> ts/db/refilenode )
> some are only
>   * A/B for the COPY message we discussed later
>
> On the other hand, and I don't know if it's relevant, I've pointed some
> examples such as XLOG_RELMAP_UPDATE in relmapdesc.c which could benefit
> of that "notation" :
>
> appendStringInfo(buf, "database %u tablespace %u size %u",
>                          xlrec->dbid, xlrec->tsid, xlrec->nbytes);
>
> could be written like this :
>
> appendStringInfo(buf, "%u/%u size %u",
>                          xlrec->tsid, xlrec->dbid, xlrec->nbytes);
>
> In that case ts and db should also be switched. In that case the message
> would only by B/C which is confusing, but we have other place where
> "base/" is put in prefix.
>
> The same transform may be also applied to standbydesc.c in
> standby_desc() function.
>
> appendStringInfo(buf, "xid %u db %u rel %u ",
>                              xlrec->locks[i].xid, xlrec->locks[i].dbOid,
>                              xlrec->locks[i].relOid);
>
> may be  changed to
>
> appendStringInfo(buf, "xid %u (db/rel) %u/%u ",
>                              xlrec->locks[i].xid, xlrec->locks[i].dbOid,
>                              xlrec->locks[i].relOid);
>
>
> As I said, I don't know whether it's relevant to perform these changes
> or not.

Maybe, I'm not against doing that. But if we do that, I don't think we
need to add the "(db/rel)" bit - we don't do that elsewhere, so why
here? And if we adopt the same format, should that also include the
tablespace? Although, maybe for locks that doesn't make much sense.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services