Re: Add last commit LSN to pg_last_committed_xact()

On Tue, Jan 18, 2022 at 9:19 PM Andres Freund <andres@anarazel.de> wrote:
>
> > +     LWLockAcquire(ProcArrayLock, LW_SHARED);
> > +     lsn = ShmemVariableCache->finishedProcsLastCommitLSN;
> > +     for (index = 0; index < ProcGlobal->allProcCount; index++)
> > +     {
> > +             XLogRecPtr procLSN = ProcGlobal->allProcs[index].lastCommitLSN;
> > +             if (procLSN > lsn)
> > +                     lsn = procLSN;
> > +     }
> > +     LWLockRelease(ProcArrayLock);
>
> I think it'd be better to go through the pgprocnos infrastructure, so that
> only connected procs need to be checked.
>
>     LWLockAcquire(ProcArrayLock, LW_SHARED);
>     for (i = 0; i < arrayP->numProcs; i++)
>     {
>         int         pgprocno = arrayP->pgprocnos[i];
>         PGPROC     *proc = &allProcs[pgprocno];
>
>         if (proc->lastCommitLSN > lsn)
>            lsn =proc->lastCommitLSN;
>    }
>
>
> > diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
> > index a58888f9e9..2a026b0844 100644
> > --- a/src/include/storage/proc.h
> > +++ b/src/include/storage/proc.h
> > @@ -258,6 +258,11 @@ struct PGPROC
> >       PGPROC     *lockGroupLeader;    /* lock group leader, if I'm a member */
> >       dlist_head      lockGroupMembers;       /* list of members, if I'm a leader */
> >       dlist_node      lockGroupLink;  /* my member link, if I'm a member */
> > +
> > +     /*
> > +      * Last transaction metadata.
> > +      */
> > +     XLogRecPtr      lastCommitLSN;          /* cache of last committed LSN */
> >  };
>
> We do not rely on 64bit integers to be read/written atomically, just 32bit
> ones. To make this work for older platforms you'd have to use a
> pg_atomic_uint64. On new-ish platforms pg_atomic_read_u64/pg_atomic_write_u64
> end up as plain read/writes, but on older ones they'd do the necessarily
> locking to make that safe...

All right, here's an updated patch.

The final interface (new function or refactor the existing not to rely
on commit_ts) is still TBD (and I'd appreciate input on that from
Alvaro and others).

Thanks,
James Coleman