Re: SLRU optimization - configurable buffer pool and partitioning the SLRU lock

On Fri, Nov 17, 2023 at 7:28 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2023-Nov-17, Dilip Kumar wrote:

I think I need some more clarification for some of the review comments

> > On Thu, Nov 16, 2023 at 3:11 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > >
> > > I just noticed that 0003 does some changes to
> > > TransactionGroupUpdateXidStatus() that haven't been adequately
> > > explained AFAICS.  How do you know that these changes are safe?
> >
> > IMHO this is safe as well as logical to do w.r.t. performance.  It's
> > safe because whenever we are updating any page in a group we are
> > acquiring the respective bank lock in exclusive mode and in extreme
> > cases if there are pages from different banks then we do switch the
> > lock as well before updating the pages from different groups.
>
> Looking at the coverage for this code,
> https://coverage.postgresql.org/src/backend/access/transam/clog.c.gcov.html#413
> it seems in our test suites we never hit the case where there is
> anything in the "nextidx" field for commit groups.

1)
I was looking into your coverage report and I have attached a
screenshot from the same, it seems we do hit the block where nextidx
is not INVALID_PGPROCNO, which means there is some other process other
than the group leader.  Although I have already started exploring the
injection point but just wanted to be sure what is your main concern
point about the coverage so though of checking that first.

470             :     /*
     471             :      * If the list was not empty, the leader
will update the status of our
     472             :      * XID. It is impossible to have followers
without a leader because the
     473             :      * first process that has added itself to
the list will always have
     474             :      * nextidx as INVALID_PGPROCNO.
     475             :      */
     476          98 :     if (nextidx != INVALID_PGPROCNO)
     477             :     {
     478           2 :         int         extraWaits = 0;
     479             :
     480             :         /* Sleep until the leader updates our
XID status. */
     481           2 :
pgstat_report_wait_start(WAIT_EVENT_XACT_GROUP_UPDATE);
     482             :         for (;;)
     483             :         {
     484             :             /* acts as a read barrier */
     485           2 :             PGSemaphoreLock(proc->sem);
     486           2 :             if (!proc->clogGroupMember)
     487           2 :                 break;
     488           0 :             extraWaits++;
     489             :         }

2) Do we really need one separate lwlock tranche for each SLRU?

IMHO if we use the same lwlock tranche then the wait event will show
the same wait event name, right? And that would be confusing for the
user, whether we are waiting for Subtransaction or Multixact or
anything else.  Is my understanding no correct here?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com