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(at)leadboat(dot)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.