Обсуждение: Store FullTransactionId in TwoPhaseFileHeader/GlobalTransactionData

Hi Vignesh,

On Thu, Aug 1, 2019 at 9:32 PM vignesh C <vignesh21@gmail.com> wrote:
> In the undo system, we use full-transaction-id for transactions.  For
> rollback of prepared transactions, we were planning to use
> FullTransactionId by combining TransactionId and epoch, but as
> suggested by multiple people in that email chain [1][2], the better
> idea is to store Full-transactionid in TwoPhaseFileHeader

+1

> Backward compatibility need not be handled for this scnario as upgrade
> does not support having open prepared transactions.

+1

> There is also one more comment which is yet to be concluded. The
> comment discusses about changing subxids which are of TransactionId
> type to FullTransactionId type being written in two phase transaction
> file. We could not conclude this as the data is similarly stored in
> TransactionStateData.

No comment on that question or the patch yet but could you please add
this to the next Commitfest so that cfbot starts testing it?

-- 
Thomas Munro
https://enterprisedb.com

On Thu, Aug 1, 2019 at 5:36 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>
> Hi Vignesh,
>
> On Thu, Aug 1, 2019 at 9:32 PM vignesh C <vignesh21@gmail.com> wrote:
> > In the undo system, we use full-transaction-id for transactions.  For
> > rollback of prepared transactions, we were planning to use
> > FullTransactionId by combining TransactionId and epoch, but as
> > suggested by multiple people in that email chain [1][2], the better
> > idea is to store Full-transactionid in TwoPhaseFileHeader
>
> +1
>
> > Backward compatibility need not be handled for this scnario as upgrade
> > does not support having open prepared transactions.
>
> +1
>
> > There is also one more comment which is yet to be concluded. The
> > comment discusses about changing subxids which are of TransactionId
> > type to FullTransactionId type being written in two phase transaction
> > file. We could not conclude this as the data is similarly stored in
> > TransactionStateData.
>
> No comment on that question or the patch yet but could you please add
> this to the next Commitfest so that cfbot starts testing it?
>
I have added it to the commitfest.

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

On Fri, Aug 2, 2019 at 1:38 AM vignesh C <vignesh21@gmail.com> wrote:
> On Thu, Aug 1, 2019 at 5:36 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> > On Thu, Aug 1, 2019 at 9:32 PM vignesh C <vignesh21@gmail.com> wrote:
> > > There is also one more comment which is yet to be concluded. The
> > > comment discusses about changing subxids which are of TransactionId
> > > type to FullTransactionId type being written in two phase transaction
> > > file. We could not conclude this as the data is similarly stored in
> > > TransactionStateData.
> >
> > No comment on that question or the patch yet but could you please add
> > this to the next Commitfest so that cfbot starts testing it?
> >
> I have added it to the commitfest.

Thanks.  This looks pretty reasonable to me, and I don't think we need
to worry about the subxid list for now.  Here's a version I ran though
pgindent to fix some whitespace problems.

The pg_prepared_xacts view seems like as good a place as any to start
showing FullTransactionId values to users.  Here's an experimental
patch on top to do that, introducing a new "xid8" type.  After
pg_resetwal -e 1000000000 -D pgdata you can make transactions that
look like this:

postgres=# select transaction, gid from pg_prepared_xacts ;
     transaction     | gid
---------------------+-----
 4294967296000000496 | tx1
(1 row)

-- 
Thomas Munro
https://enterprisedb.com

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?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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.  Vignesh's patch achieves something important
on its own: it avoids the needs for zheap to do a 32->64 conversion.

-- 
Thomas Munro
https://enterprisedb.com

Hi,

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?

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.


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

Greetings,

Andres Freund

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

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.

Greetings,

Andres Freund

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

On Wed, Aug 7, 2019 at 6:56 AM vignesh C <vignesh21@gmail.com> wrote:
> 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.

I'm not sure, but I think the prior question is whether we want this
patch at all, and I'm not sure we've achieved consensus on that.
Thomas's point, at least as I understand it, is that if we start doing
32-bit => 64-bit XID conversions all over the place, it's not going to
be long before some incautious developer inserts one that is not
actually safe. On the other hand, Andres's point, at least as I
understand it, is that putting information that we don't really need
into the twophase state file because of some overly rigid coding rule
is not smart.  Both of those arguments sound right to me, but they
lead to opposite conclusions.

I am somewhat inclined to Andres's conclusion on balance.  I think
that we can probably define a set of policies about 32 => 64 bit XID
conversions both in terms of when you can do them and what comments
you have to include justifying them and how the API actually works
that makes it safe. It might help to think about defining the API in
terms of a reference FullTransactionId that must be OLDER than the XID
you're promoting to an FXID.  For instance, we know that all of the
relfrozenxid and datfrozenxids are from the current era because we've
got freezing machinery to enforce that. So if you got a tuple from the
heap, it's XID has got to be new, at least modulo bugs. In other
cases, we may be able to say, hey, look, this XID can't be from before
the CLOG cutoff.  I'm not sure of all the details here, but I'm
tentatively inclined to think that trying to lay down policies for
when promotion can be done safely is more promising than holding our
breath and saying we're never going to promote.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company