Horton

Re: [HACKERS] make async slave to wait for lsn to be replayed

Alexander Korotkov <aekorotkov@gmail.com>

View thread View on postgr.es
From: Alexander Korotkov <aekorotkov@gmail.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Kartyshov Ivan <i.kartyshov@postgrespro.ru>, Euler Taveira <euler@eulerto.com>, Thomas Munro <thomas.munro@gmail.com>, Peter Eisentraut <peter@eisentraut.org>, Amit Kapila <amit.kapila16@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, Peter Smith <smithpb2250@gmail.com>, pgsql-hackers@lists.postgresql.org
Date: 2024-04-03T18:17:48Z
Lists: pgsql-hackers 
On Wed, Apr 3, 2024 at 7:55 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2024-Apr-03, Alexander Korotkov wrote:
>
> > Regarding the shmem data structure for LSN waiters.  I didn't pick
> > LWLock or ConditionVariable, because I needed the ability to wake up
> > only those waiters whose LSN is already replayed.  In my experience
> > waking up a process is way slower than scanning a short flat array.
>
> I agree, but I think that's unrelated to what I was saying, which is
> just the patch I attach here.

Oh, sorry for the confusion.  I'd re-read your message.  Indeed you
meant this very clearly!

I'm good with the patch.  Attached revision contains a bit of a commit message.

> > However, I agree that when the number of waiters is very high and flat
> > array may become a problem.  It seems that the pairing heap is not
> > hard to use for shmem structures.  The only memory allocation call in
> > paritingheap.c is in pairingheap_allocate().  So, it's only needed to
> > be able to initialize the pairing heap in-place, and it will be fine
> > for shmem.
>
> Ok.
>
> With the code as it stands today, everything in WaitLSNState apart from
> the pairing heap is accessed without any locking.  I think this is at
> least partly OK because each backend only accesses its own entry; but it
> deserves a comment.  Or maybe something more, because WaitLSNSetLatches
> does modify the entry for other backends.  (Admittedly, this could only
> happens for backends that are already sleeping, and it only happens
> with the lock acquired, so it's probably okay.  But clearly it deserves
> a comment.)

Please, check 0002 patch attached.  I found it easier to move two
assignments we previously moved out of lock, into the lock; then claim
WaitLSNState.procInfos is also protected by WaitLSNLock.

> Don't we need to WaitLSNCleanup() during error recovery or something?

Yes, there is WaitLSNCleanup(). It's currently only called from one
place, given that waiting for LSN can't be invoked from background
workers or inside the transaction.

------
Regards,
Alexander Korotkov

Commits

  1. Ensure standby promotion point in 043_wal_replay_wait.pl

  2. Minor cleanup related to pg_wal_replay_wait() procedure

  3. Adjust pg_wal_replay_wait() procedure behavior on promoted standby

  4. pg_wal_replay_wait(): Fix typo in the doc

  5. Implement pg_wal_replay_wait() stored procedure

  6. Revert: Implement pg_wal_replay_wait() stored procedure

  7. Call WaitLSNCleanup() in AbortTransaction()

  8. Clarify what is protected by WaitLSNLock

  9. Use an LWLock instead of a spinlock in waitlsn.c

  10. Use the pairing heap instead of a flat array for LSN replay waiters

  11. Minor improvements for waitlsn.c

  12. Make the order of the header file includes consistent