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: Euler Taveira <euler@eulerto.com>
Cc: Thomas Munro <thomas.munro@gmail.com>, Kartyshov Ivan <i.kartyshov@postgrespro.ru>, Bharath Rupireddy <bharath.rupireddyforpostgres@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-03-30T14:14:28Z
Lists: pgsql-hackers

Attachments

On Fri, Mar 29, 2024 at 6:50 PM Euler Taveira <euler@eulerto.com> wrote:
> On Fri, Mar 29, 2024, at 9:44 AM, Alexander Korotkov wrote:
>
> This generally makes sense, but I'm not sure about this.  The
> milliseconds timeout was used initially but received critics in [1].
>
>
> Alexander, I see why you changed the patch.
>
> Peter suggested to use an interval but you proposed another data type:
> float. The advantage of the interval data type is that you don't need to
> carefully think about the unit, however, if you use the integer data
> type you have to propose one. (If that's the case, milliseconds is a
> good granularity for this feature.) I don't have a strong preference
> between integer and interval data types but I don't like the float for
> this case. The 2 main reasons are (a) that we treat time units (hours,
> minutes, seconds, ...) as integers so it seems natural for a human being
> to use a unit time as integer and (b) depending on the number of digits
> after the decimal separator you still don't have an integer in the
> internal unit, hence, you have to round it to integer.
>
> We already have functions that use integer (such as pg_terminate_backend)
> and interval (such as pg_sleep_for) and if i searched correctly it will
> be the first timeout argument as float.

Thank you for the detailed explanation.  Float seconds are used in
pg_sleep() just similar to the interval in pg_sleep_for().  However,
that's a delay, not exactly a timeout.  Given the precedent of
milliseconds timeout in pg_terminate_backend(), your and Pavel's
points, I've switched back to integer milliseconds timeout.

Some fixes spotted off-list by Alexander Lakhin.
1) We don't need an explicit check for the postmaster being alive as
soon as we pass WL_EXIT_ON_PM_DEATH to WaitLatch().
2) When testing for unreachable LSN, we need to select LSN well in
advance so that autovacuum couldn't affect that.

I'm going to push this if no objections.

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