On 02/20/2015 05:21 PM, Andres Freund wrote:
In the attached patch I've merged compact/noncompact code, made aborts use similar logic to avoid including useless bytes and used both for the 2pc equivalents.
+1 for this approach in general.
To avoid using more space in the compact case the 'xinfo' field indicating the presence of further data is only included when a byte in the xl_info flag is set (similar to what heap rmgr does). That means that transactions without subtransactions and other things are four bytes smaller than before, but ones with a subtransaction but no other complications 4 byte larger. database info, nsubxacts, nrels, nmsgs et al are also only included if required. I think that's a overall win, even disregarding wal_level = logical.
xinfo is a bit underdocumented now. The comment in xl_xact_commit should mention that it's a uint32. But actually, why is it 32 bits wide? Only 6 bits are used, so it would in a single byte. I guess that's because of alignment: now that it's 32 bits wide, that ensures that all the other structs are 4 byte aligned. That should be documented. Alternatively, store them unaligned to save the few bytes and use memcpy.
There's one bit that I'm not so sure about though: To avoid duplication I've added Parse(Commit/Abort)Record(), but unfortunately that has to be available both in front and backend code - so it's currently living in xactdesc.c. I think we can live with that, but it's certainly not pretty.
Yeah, that's ugly. Why does frontend code need that? The old format isn't exactly trivial for frontend code to decode either.
Regarding XactEmitCommitRecord and XactEmitAbortRecord, I wonder if you could pass an xl_xact_parsed/abort_commit struct to them, instead of the individual fields? You could then also avoid the static variables inside it, passing pointers to that struct to XLogRegisterData() instead.
- Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
