Re: collect_corrupt_items_vacuum.patch

On Wed, Jul 03, 2024 at 12:31:48AM +0300, Alexander Korotkov wrote:
> On Mon, Jul 1, 2024 at 2:18 AM Noah Misch <noah@leadboat.com> wrote:
> > Commit e85662d wrote:
> > > --- a/src/backend/storage/ipc/procarray.c
> > > +++ b/src/backend/storage/ipc/procarray.c
> >
> > > @@ -2740,6 +2741,8 @@ GetRunningTransactionData(void)
> > >        */
> > >       for (index = 0; index < arrayP->numProcs; index++)
> > >       {
> > > +             int                     pgprocno = arrayP->pgprocnos[index];
> > > +             PGPROC     *proc = &allProcs[pgprocno];
> > >               TransactionId xid;
> > >
> > >               /* Fetch xid just once - see GetNewTransactionId */
> > > @@ -2760,6 +2763,13 @@ GetRunningTransactionData(void)
> > >               if (TransactionIdPrecedes(xid, oldestRunningXid))
> > >                       oldestRunningXid = xid;
> > >
> > > +             /*
> > > +              * Also, update the oldest running xid within the current database.
> > > +              */
> > > +             if (proc->databaseId == MyDatabaseId &&
> > > +                     TransactionIdPrecedes(xid, oldestRunningXid))
> > > +                     oldestDatabaseRunningXid = xid;
> >
> > Shouldn't that be s/oldestRunningXid/oldestDatabaseRunningXid/?
> 
> Thank you for catching this.
> 
> > While this isn't a hot path, I likely would test TransactionIdPrecedes()
> > before fetching pgprocno and PGPROC, to reduce wasted cache misses.
> 
> And thanks for suggestion.
> 
> The patchset is attached.  0001 implements
> s/oldestRunningXid/oldestDatabaseRunningXid/.  0002 implements cache
> misses optimization.
> 
> If no objection, I'll backpatch 0001 and apply 0002 to the head.

Looks fine.  I'd drop the comment update as saying the obvious, but keeping it
is okay.