Hi, hackers! On Fri, 29 Mar 2024 at 16:45, Alexander Korotkov <aekorot...@gmail.com> wrote:
> Hi, Euler! > > On Fri, Mar 29, 2024 at 1:38 AM Euler Taveira <eu...@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.
