Visibility bug with prepared transaction with subtransactions on standby - Mailing list pgsql-hackers

Hi,

Konstantin and I found an MVCC bug with:

- a prepared transaction,
- which has a subtransaction,
- on a hot standby,
- after starting the standby from a shutdown checkpoint.

See the test case in the attached patch to demonstrate this. The last 
query in the new test returns incorrect result on master, causing the 
test to fail.

The problem
-----------

When you shut down a primary with a prepared transaction, and start a 
hot standby server from the shutdown checkpoint, the hot standby server 
goes through this code at startup:

>             if (wasShutdown)
>                 oldestActiveXID = PrescanPreparedTransactions(&xids, &nxids);
>             else
>                 oldestActiveXID = checkPoint.oldestActiveXid;
>             Assert(TransactionIdIsValid(oldestActiveXID));
> 
>             /* Tell procarray about the range of xids it has to deal with */
>             ProcArrayInitRecovery(XidFromFullTransactionId(TransamVariables->nextXid));
> 
>             /*
>              * Startup subtrans only.  CLOG, MultiXact and commit timestamp
>              * have already been started up and other SLRUs are not maintained
>              * during recovery and need not be started yet.
>              */
>             StartupSUBTRANS(oldestActiveXID);
> 
>             /*
>              * If we're beginning at a shutdown checkpoint, we know that
>              * nothing was running on the primary at this point. So fake-up an
>              * empty running-xacts record and use that here and now. Recover
>              * additional standby state for prepared transactions.
>              */
>             if (wasShutdown)
>             {
>                 RunningTransactionsData running;
>                 TransactionId latestCompletedXid;
> 
>                 /*
>                  * Construct a RunningTransactions snapshot representing a
>                  * shut down server, with only prepared transactions still
>                  * alive. We're never overflowed at this point because all
>                  * subxids are listed with their parent prepared transactions.
>                  */
>                 running.xcnt = nxids;
>                 running.subxcnt = 0;
>                 running.subxid_overflow = false;
>                 running.nextXid = XidFromFullTransactionId(checkPoint.nextXid);
>                 running.oldestRunningXid = oldestActiveXID;
>                 latestCompletedXid = XidFromFullTransactionId(checkPoint.nextXid);
>                 TransactionIdRetreat(latestCompletedXid);
>                 Assert(TransactionIdIsNormal(latestCompletedXid));
>                 running.latestCompletedXid = latestCompletedXid;
>                 running.xids = xids;
> 
>                 ProcArrayApplyRecoveryInfo(&running);
> 
>                 StandbyRecoverPreparedTransactions();
>             }

The problem is that the RunningTransactions snapshot constructed here 
does not include subtransaction XIDs of the prepared transactions, only 
the main XIDs. Because of that, snapshots taken in the standby will 
consider the sub-XIDs as aborted rather than in-progress. That leads to 
two problems if the prepared transaction is later committed:

- We will incorrectly set hint bits on tuples inserted/deleted by the 
subtransactions, which leads to incorrect query results later if the 
prepared transaction is committed.

- If you acquire an MVCC snapshot and hold to it while the prepared 
transaction commits, the subtransactions will suddenly become visible to 
the old snapshot.

History
-------

StandbyRecoverPreparedTransactions has this comment:

>  * The lack of calls to SubTransSetParent() calls here is by design;
>  * those calls are made by RecoverPreparedTransactions() at the end of recovery
>  * for those xacts that need this.

I think that's wrong; it really should update pg_subtrans. It used to, a 
long time ago, but commit 49e92815497 changed it. Reading the 
discussions that led to that change, seems that we somehow didn't 
realize that it's important to distinguish between in-progress and 
aborted transactions in a standby. On that thread, Nikhil posted [1] a 
test case that is almost exactly the same test case that I used to find 
this, but apparently the visibility in standby in that scenario was not 
tested thoroughly back then.

[1] 
https://www.postgresql.org/message-id/CAMGcDxde4XjDyTjGvZCPVQROpXw1opfpC0vjpCkzc1pcQBqvrg%40mail.gmail.com

Fix
---

Attached is a patch to fix this, with a test case. It should be 
backpatched to all supported versions.

The patch changes a field in RunningTransactionsData from bool to an 
enum. Could that break extensions on back branches? I think it's OK, I'm 
not aware of any extensions touching RunningTransactionsData. I did not 
change the xl_running_xacts WAL record, only the in-memory struct.

Alternatively, we could add a new argument to 
ProcArrayApplyRecoveryInfo() to indicate the new case that the xids 
array in RunningTransactionsData does not include all the subxids but 
they have all been marked in pg_subtrans already. But I think the 
attached is better, as the enum makes the three different states more clear.

-- 
Heikki Linnakangas
Neon (https://neon.tech)