Re: [COMMITTERS] pgsql: Fix traversal of half-frozen update chains

[Peter Geoghegan]

On Tue, Oct 17, 2017 at 3:40 AM, Alvaro Herrera  wrote:
> I also realized we can stop checking (i.e. don't compare xmin to
> frozenxid) if the XMIN_FROZEN bits are set -- because in that case the
> tuple cannot possibly come from 9.3 frozen.  So I think this should do
> it.

When are you planning on committing this?

-- 
Peter Geoghegan


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Re: [COMMITTERS] pgsql: Fix traversal of half-frozen update chains

[Peter Geoghegan]

On Tue, Oct 17, 2017 at 12:23 PM, Peter Geoghegan  wrote:
> On Tue, Oct 17, 2017 at 3:40 AM, Alvaro Herrera  
> wrote:
>> Peter Geoghegan wrote:
>>
>>> Wouldn't this last "if" test, to cover the pg_upgrade case, be better
>>> targeted by comparing *raw* xmin to FrozenTransactionId? You're using
>>> the potentially distinct xmin value returned by
>>> HeapTupleHeaderGetXmin() for the test here. I think we should be
>>> directly targeting tuples frozen on or before 9.4 (prior to
>>> pg_upgrade) instead.
>>
>> I also realized we can stop checking (i.e. don't compare xmin to
>> frozenxid) if the XMIN_FROZEN bits are set -- because in that case the
>> tuple cannot possibly come from 9.3 frozen.  So I think this should do
>> it.
>>
>> (New HeapTupleUpdateXmaxMatchesXmin() implementation)
>
> Yeah, this is what I had in mind, too.

BTW, seems worth fixing this old comment above
heap_prepare_freeze_tuple() while you're at it:

 * NB: It is not enough to set hint bits to indicate something is
 * committed/invalid -- they might not be set on a standby, or after crash
 * recovery.  We really need to remove old xids.



-- 
Peter Geoghegan


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Re: [COMMITTERS] pgsql: Fix traversal of half-frozen update chains

[Peter Geoghegan]

On Tue, Oct 17, 2017 at 3:40 AM, Alvaro Herrera  wrote:
> Peter Geoghegan wrote:
>
>> Wouldn't this last "if" test, to cover the pg_upgrade case, be better
>> targeted by comparing *raw* xmin to FrozenTransactionId? You're using
>> the potentially distinct xmin value returned by
>> HeapTupleHeaderGetXmin() for the test here. I think we should be
>> directly targeting tuples frozen on or before 9.4 (prior to
>> pg_upgrade) instead.
>
> I also realized we can stop checking (i.e. don't compare xmin to
> frozenxid) if the XMIN_FROZEN bits are set -- because in that case the
> tuple cannot possibly come from 9.3 frozen.  So I think this should do
> it.
>
> (New HeapTupleUpdateXmaxMatchesXmin() implementation)

Yeah, this is what I had in mind, too.

-- 
Peter Geoghegan


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Re: [COMMITTERS] pgsql: Fix traversal of half-frozen update chains

[Alvaro Herrera]

Peter Geoghegan wrote:

> Wouldn't this last "if" test, to cover the pg_upgrade case, be better
> targeted by comparing *raw* xmin to FrozenTransactionId? You're using
> the potentially distinct xmin value returned by
> HeapTupleHeaderGetXmin() for the test here. I think we should be
> directly targeting tuples frozen on or before 9.4 (prior to
> pg_upgrade) instead.

I also realized we can stop checking (i.e. don't compare xmin to
frozenxid) if the XMIN_FROZEN bits are set -- because in that case the
tuple cannot possibly come from 9.3 frozen.  So I think this should do
it.

/*
 * HeapTupleUpdateXmaxMatchesXmin - verify update chain xmax/xmin lineage
 *
 * Given the new version of a tuple after some update, verify whether the
 * given Xmax (corresponding to the previous version) matches the tuple's
 * Xmin, taking into account that the Xmin might have been frozen after the
 * update.
 */
bool
HeapTupleUpdateXmaxMatchesXmin(TransactionId xmax, HeapTupleHeader htup)
{
TransactionId   rawxmin = HeapTupleHeaderGetRawXmin(htup);
TransactionId   xmin = HeapTupleHeaderGetXmin(htup);

/*
 * If the xmax of the old tuple is identical to the xmin of the new one,
 * it's a match.
 */
if (TransactionIdEquals(xmax, xmin))
return true;

/*
 * If the HEAP_XMIN_FROZEN flag is set, then we know it can only be a 
match
 * if the Xmin before the freezing is identical to the Xmax -- we can 
skip
 * the fuzzy pg_upgraded check below.
 */
if (HeapTupleHeaderXminFrozen(htup))
{
if (TransactionIdEquals(rawxmin, xmax))
return true;
else
return false;
}

/*
 * When a tuple is frozen, the original Xmin is lost, but we know it's a
 * committed transaction.  So unless the Xmax is InvalidXid, we don't 
know
 * for certain that there is a match, but there may be one; and we must
 * return true so that a HOT chain that is half-frozen can be walked
 * correctly.
 *
 * We no longer freeze tuples this way, but we must keep this in order 
to
 * interpret pre-pg_upgrade pages correctly.
 */
if (TransactionIdEquals(rawxmin, FrozenTransactionId) &&
TransactionIdIsValid(xmax))
return true;

return false;
}

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers

Re: [COMMITTERS] pgsql: Fix traversal of half-frozen update chains

[Peter Geoghegan]

On Fri, Oct 6, 2017 at 8:29 AM, Alvaro Herrera  wrote:
> Fix traversal of half-frozen update chains

I have a question about this (this is taken from the master branch):

> bool
> HeapTupleUpdateXmaxMatchesXmin(TransactionId xmax, HeapTupleHeader htup)
> {
> TransactionId   xmin = HeapTupleHeaderGetXmin(htup);
>
> /*
>  * If the xmax of the old tuple is identical to the xmin of the new one,
>  * it's a match.
>  */
> if (TransactionIdEquals(xmax, xmin))
> return true;
>
> /*
>  * If the Xmin that was in effect prior to a freeze matches the Xmax,
>  * it's good too.
>  */
> if (HeapTupleHeaderXminFrozen(htup) &&
> TransactionIdEquals(HeapTupleHeaderGetRawXmin(htup), xmax))
> return true;
>
> /*
>  * When a tuple is frozen, the original Xmin is lost, but we know it's a
>  * committed transaction.  So unless the Xmax is InvalidXid, we don't know
>  * for certain that there is a match, but there may be one; and we must
>  * return true so that a HOT chain that is half-frozen can be walked
>  * correctly.
>  *
>  * We no longer freeze tuples this way, but we must keep this in order to
>  * interpret pre-pg_upgrade pages correctly.
>  */
> if (TransactionIdEquals(xmin, FrozenTransactionId) &&
> TransactionIdIsValid(xmax))
> return true;
>
> return false;
> }

Wouldn't this last "if" test, to cover the pg_upgrade case, be better
targeted by comparing *raw* xmin to FrozenTransactionId? You're using
the potentially distinct xmin value returned by
HeapTupleHeaderGetXmin() for the test here. I think we should be
directly targeting tuples frozen on or before 9.4 (prior to
pg_upgrade) instead.

Have I missed something?

-- 
Peter Geoghegan


-- 
Sent via pgsql-committers mailing list (pgsql-committers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-committers