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

Hi,

On 2015-02-24 20:51:42 +0200, Heikki Linnakangas wrote:
> On 02/20/2015 05:21 PM, Andres Freund wrote:
> >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.

Yea, the whole thing needs a bit more comments. I really wanted some
feedback before sinking even more time into this...

> The comment in xl_xact_commit should mention that it's a uint32. But
> actually, why is it 32 bits wide?

The first reason is that it's essentially just the old 'xinfo' field
moved to a different place ;). I think I'll add a xl_xact_flags or so,
and document it there.

> 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.

But yes, I didn't think about changing it because of alignment. Given
that commit and abort records can be fairly large, it seems better to
waste three bytes in a few scenarios (not the minimal one!) than
allocate another large chunk of memory for the copied data.

Even in the current code there's some rather underdocumented assumptions
about alignment; namely that RelFileNode+, TransactionId+ always result
in a acceptable alignment for the data after them. That happens to be
the case, but is worth a comment and/or a MAXALIGN.

> >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.

pg_xlogdump outputs subxacts and such; I don't forsee other
usages. Sure, we could copy the code around, but I think that's worse
than having it in xactdesc.c. Needs a comment explaining why it's there
if I haven't added one already.

> 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.

Hm, that's an idea. And rename it to xaxt_commit/abort_data?

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services