Re: BUG #18241: PushTransaction may cause Standby to execute ItemIdMarkDead

At Tue, 12 Dec 2023 18:11:44 +0100, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote in
> On 2023-Dec-12, Michael Paquier wrote:
>
> > An issue if that this could cause problems if you do catalog
> > scans, which may explain a few bugs I recall seeing over the years,
> > even if these did not directly mention the use of ssavepoints. I'd
> > need to double-check the archives. If going through a driver layer
> > like the ODBC driver that enforces savepoints for each query, that
> > would be bad.
>
> What a horrible bug. Thanks for pushing the fix. It looks OK to me:
> nowhere else do we create a TransactionStateData that would need the
> initialization.
>
> I wonder if this can cause data corruption, if you happen to have a
> broken standby that improperly kills some index entry and is later
> promoted. Maybe this bug explains these mysterious cases where an index
> seems to lack a pointer to some heap tuple for no known reason --
> especially notorious when you try to REINDEX a unique index and that
> turns out not to work due to duplicates.
>
> I would be happier if the order of initialization of the struct member
> followed the order to the struct, with comments to note the missing
> members. That might make it more visible to future patches that would
> add more members. Something like this:

The thought briefly crossed my mind; perhaps we could also comment out
parallelModeLevel (the same can be done for topXidLogged, but I'm not
sure it's worthwhile).

> Also, the way this function checks for overflow is strange. Why
> increment then check, leading to a forced free and decrement, instead of
> just testing and throwing error right away? It's less code:

The current code seems to primarily focus on reducing the number of
add operations in the normal path.
>
> diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
> index 8442c5e6a7..fac9141b16 100644
> --- a/src/backend/access/transam/xact.c
> +++ b/src/backend/access/transam/xact.c
> @@ -5272,18 +5272,14 @@ PushTransaction(void)
> MemoryContextAllocZero(TopTransactionContext,
> sizeof(TransactionStateData));
>
> - /*
> - * Assign a subtransaction ID, watching out for counter wraparound.
> - */
> - currentSubTransactionId += 1;
> - if (currentSubTransactionId == InvalidSubTransactionId)
> - {
> - currentSubTransactionId -= 1;
> - pfree(s);
> + /* Check for overflow before incrementing the counter */
> + if (currentSubTransactionId + 1 == InvalidSubTransactionId)
> ereport(ERROR,
> (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
> errmsg("cannot have more than 2^32-1 subtransactions in a transaction")));
> - }
> +
> + /* Now we can increment it without fear */
> + currentSubTransactionId += 1;
>
> /*
> * We can now stack a minimally valid subtransaction without fear of
>
> --

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center