On 11/16/18 12:05 PM, Jean-Christophe Arnu wrote:
Le jeu. 15 nov. 2018 à 19:44, Robert Haas <robertmh...@gmail.com
<mailto:robertmh...@gmail.com>> a écrit :
On Tue, Nov 13, 2018 at 3:40 PM Tomas Vondra
<tomas.von...@2ndquadrant.com <mailto:tomas.von...@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
