Re: Wrong assert in TransactionGroupUpdateXidStatus

Hi,

Amit, Robert, IIRC that's mostly your feature?

On 2019-12-10 13:55:40 +0530, Dilip Kumar wrote:
> While testing, my colleague Vignesh has hit an assert in
> TransactionGroupUpdateXidStatus. But that is not reproducible. After
> some analysis and code review, I have found the reason for the same.
>
> As shown in the below code, there is an assert in
> TransactionGroupUpdateXidStatus, which assumes that an overflowed
> transaction can never get registered for the group update. But,
> actually, that is not true because while registering the transaction
> for group update, we only check how many committed children this
> transaction has because all aborted sub-transaction would have already
> updated their status. So if the transaction once overflowed but later
> all its children are aborted (i.e remaining committed children are <=
> THRESHOLD_SUBTRANS_CLOG_OPT) then it will be registered for the group
> update.

> /*
> * Overflowed transactions should not use group XID status update
> * mechanism.
> */
> Assert(!pgxact->overflowed);
>
> A solution could be either we remove this assert or change this assert
> to Assert(pgxact->nxids <= THRESHOLD_SUBTRANS_CLOG_OPT);

Maybe I'm missing something, but isn't this a bug then? IIRC We can't
rely on MyProc->subxids once we overflowed, even if since then the
remaining number of children has become low enough. It seems to me that
the actual fix here is to correct the condition in
TransactionIdSetPageStatus() checking whether group updates are possible
- it seems it'd need to verify that the transaction isn't
overflowed.

Also, it's somewhat odd that TransactionIdSetPageStatus() first has

/* Can't use group update when PGPROC overflows. */
StaticAssertStmt(THRESHOLD_SUBTRANS_CLOG_OPT <= PGPROC_MAX_CACHED_SUBXIDS,
"group clog threshold less than PGPROC cached subxids");

and then, within an if():

/*
* We don't try to do group update optimization if a process has
* overflowed the subxids array in its PGPROC, since in that case we
* don't have a complete list of XIDs for it.
*/
Assert(THRESHOLD_SUBTRANS_CLOG_OPT <= PGPROC_MAX_CACHED_SUBXIDS);

Even if these weren't redundant, it can't make sense to test such a
static condition only within an if? Is it possible this was actually
intended to test something different?

Greetings,

Andres Freund