Re: Store FullTransactionId in TwoPhaseFileHeader/GlobalTransactionData

On Mon, Aug 5, 2019 at 1:44 PM Andres Freund <andres@anarazel.de> wrote:
> On 2019-08-05 13:15:19 +1200, Thomas Munro wrote:
> > On Sat, Aug 3, 2019 at 12:06 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > > On Fri, Aug 2, 2019 at 6:37 AM Thomas Munro <thomas.munro@gmail.com> wrote:
> > > > Thanks.  This looks pretty reasonable to me, and I don't think we need
> > > > to worry about the subxid list for now.
> > >
> > > Why not just do them all at once?
> >
> > [tries for a couple of hours and abandons for now]
> >
> > It's a bit of a can of worms.  To do it properly, I think
> > TransactionStateData::childXids needs to become a pointer to a
> > FullTransactionId array called childFxids, so that
> > xactGetCommittedChildren() can return it, and that causes knock on
> > effects all over the tree, at least xactdesc.c, clog.c, commit_ts.c,
> > transam.c, twophase.c, xact.c need adjusting and you finish up writing
> > the subxact array into various places in the WAL in 64 bit format (but
> > not yet the main xid).  Alternatively you need to convert the array of
> > FullTransactionId into an array of TransactionId in various places, or
> > convert TransactionId into FullTransactionId just for the 2PC stuff,
> > but both of those are cop outs and require allocating extra copies.
> > Of course I am in favour of moving more things to 64 bit format, but I
> > don't want to do them all at once, and there are a number of policy
> > decisions hiding in there, and it's not strictly needed for the change
> > that Vignesh proposes.
>
> Hm. Maybe I'm missing something, but what's the point of changing this?
> We're not anytime soon going to allow transactions that are old enough
> that 32bit isn't enough to reference them. Nor do I think it's likely
> we're going to convert the procarray to 64bit xids - keeping the size
> down is too important for cache efficiency.  And as most of the data
> we're talking about here references *live* transactions, rather than
> information that's needed for longer (as e.g. a row's xmin/xmax would),
> I don't see why it'd be useful to convert to 64bit xids?

Yeah. I think we're agreed for now that we don't want to change
procarray (though we still need to figure out how to compute the 64
bit horizons correctly and efficiently), and we probably don't want to
change any high volume WAL contents, so maybe I was starting down the
wrong path there (I was thinking of the subxid list for 2pc as
infrequent, but obviously it isn't for some people).  I don't have
time to look into that any more right now, but today's experiment made
me feel more certain about my earlier statement, that we shouldn't
worry about the subxid list for now if we don't actually have to.

> I do see a point in converting these 32bit xids to 64bit xids when
> viewing them, i.e. have pg_prepared_xacts.transaction,
> pg_stat_activity.backend_{xid,xmin}, ... return a 64bit xid.

Yep, I agree, hence xid8.

> > Vignesh's patch achieves something important on its own: it avoids the
> > needs for zheap to do a 32->64 conversion.
>
> Hm, is that an actual problem?

It creates a place in the undo worker patch set that wants to do an
xid -> fxid translation, as discussed here:

https://www.postgresql.org/message-id/CAA4eK1L9BhvnQfa_RJCTpKQf9QZ15pyUW7s32BH78iBC3KbV0g%40mail.gmail.com

I'm trying to stop people from supplying a general purpose footgun
that looks like "GuessFullTransactionId(xid)".  I suspect that any
time you think you want to do that, there is probably a better way
that doesn't involve having to convince everyone that we didn't mess
up the epoch part in some unlikely race, which probably involves
holding onto an fxid that you had somewhere earlier that came
ultimately from the next fxid generator, or deriving it with reference
to the next fxid or a known older-but-still-running fxid with the
right interlocking, or something like that.  I and others said, well,
why don't we just put the fxid in the 2pc file.  That's what Vignesh
has proposed, and AFAIK it solves that immediate problem.

That caused people to ask -- entirely reasonably -- why we don't
change ALL the xids in there to fxids, which brings us here.  I think
it includes lots of tricky decisions that I don't want to make right
now, hence inclination to defer that question for now.

-- 
Thomas Munro
https://enterprisedb.com