Re: Store FullTransactionId in TwoPhaseFileHeader/GlobalTransactionData

On Mon, Aug 5, 2019 at 8:31 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2019-08-05 14:44:37 +1200, Thomas Munro wrote:
> > 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)
>
> Hm. Is that actually hard? Can't we just use the current logic to
> compute the horizons in 32bit, and then extend that to 64bit by
> comparing to nextFullXid or something like that? That shouldn't be more
> than a few instructions, outside of the contended locks?
>
>
> > 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 don't think we should start to change existing wal contents to 64bit
> xids before there's a benefit from doing so (that obviously doesn't mean
> that records for a hypothetical AM employing 64bit xids shouldn't
> contain them, if they are actually long-term values, rather than just
> about the current transaction). I think that's just going to be
> confusing, without providing much in the way of benefits.
>
>
> > > > 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
>
> Right, but I think that can just be done the suggestion from above.
>
>
> > I'm trying to stop people from supplying a general purpose footgun
> > that looks like "GuessFullTransactionId(xid)".
>
> I think you're right - but I think we should be able to provide
> functions that ensure safety for most if not all of these. For WAL
> replay routines we can reference xlogrecord, for GetOldestXmin() we can
> just expand internally, by referencing a 64bit xid in ShmemVariableCache
> or such.
>
>
> > 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.
>
> I'm against doing this just for one part of the record. That seems
> supremely confusing. And I don't buy that it meaningfully helps us in
> the first place, given the multitude of other records containing 32bit
> xids.
>
Going by the discussion shall we conclude that we don't need to
convert the subxids into fxid's as part of this fix.
Let me know if any further changes need to be done.

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com