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
signature.asc
Description: PGP signature
