Re: Merge compact/non compact commits, make aborts dynamically sized

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