Re: pgsql: Implement pg_wal_replay_wait() stored procedure

On Mon, Nov 4, 2024 at 8:04 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Mon, Nov 04, 2024 at 06:29:42AM +0200, Alexander Korotkov wrote:
> > The attached patchset contains patch 0001, which improves handling of
> > not in recovery state by usage of PromoteIsTriggered(). When
> > (PromoteIsTriggered() == false), last replay LSN is not accepted and
> > not reported in errdetail().
> >
> > 0002 contains patch finishing implicit transaction in default
> > isolation level REPEATABLE READ or higher with revised commit message.
>
> I was just catching up with this thread, and I'm still confused about
> the state of things. There are two things that are out of order for
> me, at least, after skimming through the code on HEAD (I suspect there
> is more):
> - WaitForLSNReplay() uses an initial set of checks that are duplicated
> in the main loop. This is still overcomplicated, no? Wouldn't it be
> simpler to eliminate the first of checks or just have a goto block
> with addLSNWaiter() called after the first round of checks?
> - pg_wal_replay_wait_status() returns a status based on a static
> variable that can only be accessed with the same backend as the one
> that has called the wait function. That's.. Err.. Unfriendly. Why
> being sticky with one backend for the job?

That's a bit awkward, but I think generally valid. Some backend can
wait for LSN, then the result could be read from the same backend.
Difference backends could wait for different LSNs with different
results, I definitely don't feel like something should be shared
across backends.

> Using output parameters in a procedure is something I did not recall.
> Based on your point about not using a function due your argument based
> on the snapshots, let's just use that and forget about the status
> function entirely (please?).

Please, check [1]. Usage of output parameters is a bit awkward too,
because you need to pass some value in there. And more importantly,
usage of output parameters also causes snapshot problem, as it causes
another snapshot to be held.

I'm tending to think we don't necessarily need to have ability to get
the waiting status in the initial version of this feature. The work
on this feature started in 2016. It's already very long for a simple
feature of just waiting for LSN (simple in general design, while our
snapshot reality makes one full of trouble). Having in-core
implementation for this seems like breakthrough already, even if it
doesn't have all the abilities it could potentially have.

> Based on the latest set of issues reported, it does not feel like this
> is really baked and ready for prime day, either. Perhaps it would be
> less confusing to just revert the whole and repost a more complete and
> structured implementation with an extra round of reviews? There's
> still time in this release cycle.

Yes, this makes sense. Will revert.

Links.
1. https://www.postgresql.org/message-id/CAPpHfdvRmTzGJw5rQdSMkTxUPZkjwtbQ%3DLJE2u9Jqh9gFXHpmg%40mail.gmail.com

------
Regards,
Alexander Korotkov
Supabase