Re: [HACKERS] StandbyRecoverPreparedTransactions recovers subtranslinks incorrectly

Hi,

On 2017-04-23 11:05:44 +0100, Simon Riggs wrote:
> > Yikes.  This is clearly way undertested.  It's also pretty scary that
> > the code has recently been whacked out quite heavily (both 9.6 and
> > master), without hitting anything around this - doesn't seem to bode
> > well for how in-depth the testing was.
> 
> Obviously if there is a bug it's because tests didn't find it and
> therefore it is by definition undertested for that specific bug.

Sure. But there's bugs that are hard to catch, and there's bugs that are
easily testable. This seems to largely fall into the "relatively easy to
test" camp.


> I'm not sure what other conclusion you wish to draw, if any?

That the changes around whacking around twophase.c in several of the
last releases, didn't yield enough verified testing infrastructure.


> >> It's not clear to me how much potential this has to create user data
> >> corruption, but it doesn't look good at first glance.  Discuss.
> >
> > Hm. I think it can cause wrong tqual.c results in some edge cases.
> > During HS, lastOverflowedXid will be set in some cases, and then
> > XidInMVCCSnapshot etc will not find the actual toplevel xid, which'll in
> > turn cause lookups snapshot->subxip (where all HS xids reside)
> > potentially return wrong results.
> >
> > I was about to say that I don't see how it could result in persistent
> > corruption however - the subtrans lookups are only done for
> > (snapshot->xmin, snapshot->xmax] and subtrans is truncated
> > regularly. But these days CHECKPOINT_END_OF_RECOVERY isn't obligatory
> > anymore, so that might be delayed.  Hm.
> 
> I've not found any reason, yet, to believe we return wrong answers in
> any case, even though the transient data structure pg_subtrans is
> corrupted by the switched call Tom discovers.

I think I pointed out a danger above. Consider what happens if query on
a standby has a suboverflowed snapshot:
Snapshot
GetSnapshotData(Snapshot snapshot)
{
...    if (TransactionIdPrecedesOrEquals(xmin, procArray->lastOverflowedXid))        suboverflowed = true;}
..snapshot->suboverflowed = suboverflowed;
}

In that case we rely on pg_subtrans for visibility determinations:
bool
HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,                   Buffer buffer)
{
...if (!HeapTupleHeaderXminCommitted(tuple)){
...    else if (XidInMVCCSnapshot(HeapTupleHeaderGetRawXmin(tuple), snapshot))        return false;

and
static bool
XidInMVCCSnapshot(TransactionId xid, Snapshot snapshot)
{
...if (!snapshot->takenDuringRecovery){
...else{
...    if (snapshot->suboverflowed)    {        /*         * Snapshot overflowed, so convert xid to top-level.  This is
safe        * because we eliminated too-old XIDs above.         */        xid = SubTransGetTopmostTransaction(xid);
 

if the subxid->xid mapping doesn't actually exist - as it's the case
with this bug afaics - we'll not get the correct toplevel
transaction. Which'll mean the following block:    /*     * We now have either a top-level xid higher than xmin or an
 * indeterminate xid. We don't know whether it's top level or subxact     * but it doesn't matter. If it's present, the
xidis visible.     */    for (j = 0; j < snapshot->subxcnt; j++)    {        if (TransactionIdEquals(xid,
snapshot->subxip[j]))           return true;    }
 
won't work correctly if suboverflowed.

I don't see anything prevent wrong results here?


Andres Freund