Re: Wrong assert in TransactionGroupUpdateXidStatus

On Wed, Dec 11, 2019 at 11:00 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 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.
>

I am able to reproduce the issue by reducing the values of
PGPROC_MAX_CACHED_SUBXIDS and THRESHOLD_SUBTRANS_CLOG_OPT to 2. Below
is what I did after reducing the values:
Session-1
--------------
postgres=# begin;
BEGIN
postgres=# insert into t1 values(1);
INSERT 0 1
postgres=# savepoint s1;
SAVEPOINT
postgres=# insert into t1 values(2);
INSERT 0 1
postgres=# savepoint s2;
SAVEPOINT
postgres=# insert into t1 values(3);
INSERT 0 1
postgres=# savepoint s3;
SAVEPOINT
postgres=# insert into t1 values(4);
INSERT 0 1
postgres=# rollback to s2;
ROLLBACK

Session-2
---------------
insert into t1 values(4); -- attach debugger and stop in
TransactionIdSetPageStatus after acquiring CLogControlLock

Session-1
---------------
Commit; -- This will wait to acquire CLogControlLock in a group
update path (TransactionGroupUpdateXidStatus). Now, continue in the
session-2 debugger. After that continue in session-1's debugger and
it will hit the Assert.

The attached patch fixes it by changing the Assert. I have
additionally removed the redundant Assert in
TransactionIdSetPageStatus as pointed out by Andres. I am planning to
commit and backpatch this early next week (Monday) unless someone
wants to review it further or has objections to it.

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