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

At Tue, 12 Dec 2023 18:11:44 +0100, Alvaro Herrera <alvherre@alvh.no-ip.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