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@gmail.com 
> <mailto:robertmhaas@gmail.com>> a écrit :
> 
>     On Tue, Nov 13, 2018 at 3:40 PM Tomas Vondra
>     <tomas.vondra@2ndquadrant.com <mailto:tomas.vondra@2ndquadrant.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