Re: Replace known_assigned_xids_lck by memory barrier

On Wed, Aug 16, 2023 at 05:30:59PM +0200, Michail Nikolaev wrote:
> Updated version (with read barriers is attached).

Thanks for the updated patch.  I've attached v4 in which I've made a number
of cosmetic edits.

> I'll think more, but can't find something wrong here so far.

IIUC this memory barrier stuff is only applicable to KnownAssignedXidsAdd()
(without an exclusive lock) when we add entries to the end of the array and
then update the head pointer.  Otherwise, appropriate locks are taken when
reading/writing the array.  For example, say we have the following array:

              head
               |
               v
    [ 0, 1, 2, 3 ]

When adding elements, we keep the head pointer where it is:

              head
               |
               v
    [ 0, 1, 2, 3, 4, 5 ]

If another processor sees this intermediate state, it's okay because it
will only inspect elements 0 through 3.  Only at the end do we update the
head pointer:

                    head
                     |
                     v
    [ 0, 1, 2, 3, 4, 5 ]

With weak memory ordering and no barriers, another process may see this
(which is obviously no good):

                    head
                     |
                     v
    [ 0, 1, 2, 3 ]

One thing that I'm still trying to understand is this code in
KnownAssignedXidsSearch():

        /* we hold ProcArrayLock exclusively, so no need for spinlock */
        tail = pArray->tailKnownAssignedXids;
        head = pArray->headKnownAssignedXids;

It's not clear to me why holding ProcArrayLock exclusively means we don't
need to worry about the spinlock/barriers.  If KnownAssignedXidsAdd() adds
entries without a lock, holding ProcArrayLock won't protect you, and I
don't see anything else that acts as a read barrier before the array
entries are inspected.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com