Re: Wrong assert in TransactionGroupUpdateXidStatus

On Thu, Dec 12, 2019 at 6:10 PM Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>
> On 2019-Dec-11, Amit Kapila 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:
>
> > > /*
> > > * 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.
>
> The more I look at both these asserts, the less sense they make. Why
> does clog.c care about PGPROC at all?
>

It is mainly for group updates. Basically, we want to piggyback the
procs that are trying to update clog at the same time on the proc
which got the CLogControlLock. This avoids taking/releasing that lock
multiple times. See TransactionGroupUpdateXidStatus.

> Looking at the callers of that
> routine, nowhere do they concern themselves with whether the overflowed
> flag has been set or not. It seems to me that the StaticAssert() should
> be near the PGPROC_MAX_CACHED_SUBXIDS definition, not the SUBTRANS
> definition (maybe as StaticAssertDecl, as in
> 201DD0641B056142AC8C6645EC1B5F62014B8E8030(at)SYD1217 )
>

Sounds reasonable. We can do that once the patch mentioned by you got
committed. For now, we are planning to just remove the Assert inside
if() condition. Do you see any problem with that?

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