At 2014-07-04 13:43:46 -0400, alvherre@2ndquadrant.com wrote:
>
> Now, I see that pg_xlogdump is checking for NULL return, but I'm not
> sure this is the best possible API.
Note that the patched pg_xlogdump will display "rm_name/0xNN" for any
records that rm_identify returns a NULL for.
Earlier, when there was the possibility that new record types could be
added without updating pg_xlogdump's identification code, this made good
sense. Now one could argue that pg_xlogdump should fully depend on every
record in WAL being properly identified.
If that's the case, rm_identify could return "UNKNOWN", and pg_xlogdump
could use the return value without checking. I considered that, but the
only thing that stopped me was the thought that if a "weird" record DOES
show up in WAL somehow, it'd be pretty handy to (a) know the value of
xl_info, and (b) see if there's more than one kind (per-rmid).
But I don't feel strongly enough about it that I'd object to displaying
just "UNKNOWN".
> I wonder if it'd be better to pass the whole record instead and have
> the rm_identify function pull out the info if it's all it needs, but
> leave the possibility open that it could read, say, some header bytes
> in the record data.
I don't *have* an XLogRecord at the point where I'm calling rm_identify.
I could call rm_identify earlier, and either store the name in the stats
structure, or hash the name and use the hash value to store that record
type's statistics.
We don't even have any other potential callers for rm_identify. Adding
it and making it significantly more difficult to use for the only code
that actually needs it… no, I pretty much hate that idea.
Personally, I think it's fine to keep rm_identify the way it is. It's
hardly very difficult code, and if the assumption that records can be
identified by xl_info ever becomes invalid, this isn't the only code
that will need to be changed either.
(Otherwise, I'd actually prefer to keep all the identification code in
pg_xlogdump, i.e. the pre-rm_identify version of my patch. At least that
would make it possible to maintain a version that could be built against
9.3, the way Marti did.)
> Also I didn't check how you handle the "init" bit in RM_HEAP and
> RM_HEAP2. Are we just saying that "insert (init)" is a different
> record type from "insert"?
Yes, that's what the patch does currently. "X" vs. "X+INIT".
-- Abhijit