public inbox for [email protected]  

help / color / mirror / Atom feed

From: Bharath Rupireddy <[email protected]>
To: Andres Freund <[email protected]>
Cc: Nathan Bossart <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: WAL Insertion Lock Improvements (was: Re: Avoid LWLockWaitForVar() for currently held WAL insertion lock in WaitXLogInsertionsToFinish())
Date: Thu, 8 Dec 2022 12:29:54 +0530
Message-ID: <CALj2ACU5GCj1S9Wra34uQ1g5EwHqhKBgGPcvD0U+cUET_Lm8JA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <CALj2ACVF+6jLvqKe6xhDzCCkr=rfd6upaGc3477Pji1Ke9G7Bg@mail.gmail.com>
	<[email protected]>
	<CALj2ACWkWbheFhkPwMw83CUpzHFGXSV_HXTBxG9+-PZ3ufHE=Q@mail.gmail.com>
	<[email protected]>
	<CALj2ACXYpYDboe4i+yXDYoNQj9-G0584hP5x_b=W6hj693qcaA@mail.gmail.com>
	<20221203003158.GA2518778@nathanxps13>
	<[email protected]>

On Tue, Dec 6, 2022 at 12:00 AM Andres Freund <[email protected]> wrote:
>
> FWIW, I don't see an advantage in 0003. If it allows us to make something else
> simpler / faster, cool, but on its own it doesn't seem worthwhile.

Thanks. I will discard it.

> I think it'd be safe to optimize LWLockConflictsWithVar(), due to some
> pre-existing, quite crufty, code. LWLockConflictsWithVar() says:
>
>          * Test first to see if it the slot is free right now.
>          *
>          * XXX: the caller uses a spinlock before this, so we don't need a memory
>          * barrier here as far as the current usage is concerned.  But that might
>          * not be safe in general.
>
> which happens to be true in the single, indirect, caller:
>
>         /* Read the current insert position */
>         SpinLockAcquire(&Insert->insertpos_lck);
>         bytepos = Insert->CurrBytePos;
>         SpinLockRelease(&Insert->insertpos_lck);
>         reservedUpto = XLogBytePosToEndRecPtr(bytepos);
>
> I think at the very least we ought to have a comment in
> WaitXLogInsertionsToFinish() highlighting this.

So, using a spinlock ensures no memory ordering occurs while reading
lock->state in LWLockConflictsWithVar()? How does spinlock that gets
acquired and released in the caller WaitXLogInsertionsToFinish()
itself and the memory barrier in the called function
LWLockConflictsWithVar() relate here? Can you please help me
understand this a bit?

> It's not at all clear to me that the proposed fast-path for LWLockUpdateVar()
> is safe. I think at the very least we could end up missing waiters that we
> should have woken up.
>
> I think it ought to be safe to do something like
>
> pg_atomic_exchange_u64()..
> if (!(pg_atomic_read_u32(&lock->state) & LW_FLAG_HAS_WAITERS))
>   return;

pg_atomic_exchange_u64(&lock->state, exchange_with_what_?. Exchange
will change the value no?

> because the pg_atomic_exchange_u64() will provide the necessary memory
> barrier.

I'm reading some comments [1], are these also true for 64-bit atomic
CAS? Does it mean that an atomic CAS operation inherently provides a
memory barrier? Can you please point me if it's described better
somewhere else?

[1]
 * Full barrier semantics.
 */
static inline uint32
pg_atomic_exchange_u32(volatile pg_atomic_uint32 *ptr,

    /*
     * Get and clear the flags that are set for this backend. Note that
     * pg_atomic_exchange_u32 is a full barrier, so we're guaranteed that the
     * read of the barrier generation above happens before we atomically
     * extract the flags, and that any subsequent state changes happen
     * afterward.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

view thread (5+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: WAL Insertion Lock Improvements (was: Re: Avoid LWLockWaitForVar() for currently held WAL insertion lock in WaitXLogInsertionsToFinish())
  In-Reply-To: <CALj2ACU5GCj1S9Wra34uQ1g5EwHqhKBgGPcvD0U+cUET_Lm8JA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox