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

Alexander Korotkov <aekorotkov@gmail.com>

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-29T12:44:56Z
Lists: pgsql-hackers

Attachments

Hi, Euler!

On Fri, Mar 29, 2024 at 1:38 AM Euler Taveira <euler@eulerto.com> wrote:
> On Thu, Mar 28, 2024, at 9:39 AM, Alexander Korotkov wrote:
>
> Fixed along with other issues spotted by Alexander Lakhin.
>
>
> [I didn't read the whole thread. I'm sorry if I missed something ...]
>
> You renamed the function in a previous version but let me suggest another one:
> pg_wal_replay_wait. It uses the same pattern as the other recovery control
> functions [1]. I think "for" doesn't add much for the function name and "lsn" is
> used in functions that return an LSN (that's not the case here).
>
> postgres=# \df pg_wal_replay*
> List of functions
> -[ RECORD 1 ]-------+---------------------
> Schema              | pg_catalog
> Name                | pg_wal_replay_pause
> Result data type    | void
> Argument data types |
> Type                | func
> -[ RECORD 2 ]-------+---------------------
> Schema              | pg_catalog
> Name                | pg_wal_replay_resume
> Result data type    | void
> Argument data types |
> Type                | func

Makes sense to me.  I tried to make a new procedure name consistent
with functions acquiring various WAL positions.  But you're right,
it's better to be consistent with other functions controlling wal
replay.

> Regarding the arguments, I think the timeout should be bigint. There is at least
> another function that implements a timeout that uses bigint.
>
> postgres=# \df pg_terminate_backend
> List of functions
> -[ RECORD 1 ]-------+--------------------------------------
> Schema              | pg_catalog
> Name                | pg_terminate_backend
> Result data type    | boolean
> Argument data types | pid integer, timeout bigint DEFAULT 0
> Type                | func
>
> I also suggests that the timeout unit should be milliseconds, hence, using
> bigint is perfectly fine for the timeout argument.
>
> +       <para>
> +        Throws an ERROR if the target <acronym>lsn</acronym> was not replayed
> +        on standby within given timeout.  Parameter <parameter>timeout</parameter>
> +        is the time in seconds to wait for the <parameter>target_lsn</parameter>
> +        replay. When <parameter>timeout</parameter> value equals to zero no
> +        timeout is applied.
> +       </para></entry>

This generally makes sense, but I'm not sure about this.  The
milliseconds timeout was used initially but received critics in [1].

Links.
1. https://www.postgresql.org/message-id/b45ff979-9d12-4828-a22a-e4cb327e115c%40eisentraut.org

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