At 2014-07-04 13:43:46 -0400, alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
