Re: wal_dump output on CREATE DATABASE

Le jeu. 15 nov. 2018 à 19:44, Robert Haas <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> 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.
If the A/B/C notation is to be generalized, it would be worth document it
in the SGML file.
If not, the first patch provided should be enough.

Regards

--
Jean-Christophe Arnu