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
