Re: Wrong assert in TransactionGroupUpdateXidStatus

On Wed, Dec 11, 2019 at 4:02 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2019-12-10 13:55:40 +0530, Dilip Kumar wrote:
>
> > /*
> > * 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.
>

AFAICS, the MyProc->subxids is maintained properly if the number of
subtransactions is lesser than PGPROC_MAX_CACHED_SUBXIDS (64). Can
you explain the case where that won't be true? Also, even if what you
are saying is true, I think the memcmp in TransactionIdSetPageStatus
should avoid taking us a wrong decision.

>
> 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?
>

I don't remember exactly the reason for this, but now I don't find the
Assert within if () meaningful. I think we should remove the Assert
inside if() unless Robert or someone see any use of it.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com