Re: pgsql: Implement pg_wal_replay_wait() stored procedure

On Mon, Sep 02, 2024 at 02:55:50AM +0300, Alexander Korotkov wrote:
> Could you, please, check the attached patch?

The patch moving the code looks correct at quick glance.

Now, I've been staring at this line, wondering why this is required
while WaitForLSNReplay() does not return any status:
+ (void) WaitForLSNReplay(target_lsn, timeout);

And this makes me question whether you have the right semantics
regarding the SQL function itself. Could it be more useful for
WaitForLSNReplay() to report an enum state that tells you the reason
why a wait may not have happened with a text or a tuple returned by
the function? There are quite a few reasons why a wait may or may not
happen:
- Recovery has ended, target LSN has been reached.
- We're not in recovery anymore. Is an error the most useful thing
here actually?
- Timeout may have been reached, where an error is also generated.
ERRCODE_QUERY_CANCELED is not a correct error state.
- Perhaps more of these in the future?

My point is: this is a feature that can be used for monitoring as far
as I know, still it does not stick as a feature that could be most
useful for such applications. Wouldn't it be more useful for client
applications to deal with a state returned by the SQL function rather
than having to parse error strings to know what happened? What is
returned by pg_wal_replay_wal() reflects on the design of
WaitForLSNReplay() itself.
--
Michael