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

Hi, hackers!

On Fri, 29 Mar 2024 at 16:45, Alexander Korotkov <aekorotkov@gmail.com>
wrote:

> 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].
>
I see in Postgres we already have different units for timeouts:

e.g in guc's:
wal_receiver_status_interval in seconds
tcp_keepalives_idle in seconds

commit_delay in microseconds

deadlock_timeout in milliseconds
max_standby_archive_delay in milliseconds
vacuum_cost_delay in milliseconds
autovacuum_vacuum_cost_delay in milliseconds
etc..

I haven't counted precisely, but I feel that milliseconds are the most
often used in both guc's and functions. So I'd propose using milliseconds
for the patch as it was proposed originally.

Regards,
Pavel Borisov
Supabase.