Re: Add last commit LSN to pg_last_committed_xact()

Hi,

On 2022-01-18 20:58:01 -0500, James Coleman wrote:
> Is something roughly like the attached what you'd envisioned?

Roughly, yea.


> I think we need a shared ProcArrayLock to read the array, correct?

You could perhaps get away without it, but it'd come at the price of needing
to look at all procs, rather than the connected procs. And I don't think it's
needed.


> We also need to do the global updating under lock, but given it's when a
> proc is removed, that shouldn't be a performance issue if I'm following what
> you are saying.

Yup.


> +    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...

Greetings,

Andres Freund