Thread: Re: pgsql: Implement pg_wal_replay_wait() stored procedure
On Mon, Oct 28, 2024 at 11:36 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 25/10/2024 14:56, Alexander Korotkov wrote:
> > I see that pg_wal_replay_wait_status() might look weird, but it seems
> > to me like the best of feasible solutions.
>
> I haven't written many procedures, but our docs say:
>
> > Procedures do not return a function value; hence CREATE PROCEDURE
> lacks a RETURNS clause. However, procedures can instead return data to
> their callers via output parameters.
>
> Did you consider using an output parameter?
Yes I did consider them and found two issues.
1) You still need to pass something to them. And that couldn't be
default values. That's a bit awkward.
2) Usage of them causes extra snapshot to be held.
I'll recheck if it's possible to workaround any of these two.
> > Given that
> > pg_wal_replay_wait() procedure can't work concurrently to a query
> > involving pg_wal_replay_wait_status() function, I think
> > pg_wal_replay_wait_status() should be stable and parallel safe.
>
> If you call pg_wal_replay_wait() in the backend process, and
> pg_wal_replay_wait_status() in a parallel worker process, it won't
> return the result of the wait. Probably not what you'd expect. So I'd
> argue that it should be parallel unsafe.
Oh, sorry. You're absolutely correct. That should be parallel unsafe.
> > This is the brief answer. I will be able to come back with more
> > details on Monday.
>
> Thanks. A few more minor issues I spotted while playing with this:
>
> - If you pass a very high value as the timeout, e.g. INT_MAX-1, it wraps
> around and doesn't wait at all
> - You can pass NULLs as arguments. That should probably not be allowed,
> or we need to document what it means.
>
> This is disappointing:
>
> > postgres=# set default_transaction_isolation ='repeatable read';
> > SET
> > postgres=# call pg_wal_replay_wait('0/55DA24F');
> > ERROR: pg_wal_replay_wait() must be only called without an active or registered snapshot
> > DETAIL: Make sure pg_wal_replay_wait() isn't called within a transaction with an isolation level higher than READ
COMMITTED,another procedure, or a function.
>
> Is there any way we could make that work? Otherwise, the feature just
> basically doesn't work if you use repeatable read.
Thank you for catching this. The last one is really disappointing.
I'm exploring on what could be done there.
------
Regards,
Alexander Korotkov
Supabase
On Sun, Nov 3, 2024 at 10:54 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > > On Mon, Oct 28, 2024 at 9:42 PM Alexander Korotkov <aekorotkov@gmail.com> wrote: > > > > On Mon, Oct 28, 2024 at 11:36 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > > > > > > On 25/10/2024 14:56, Alexander Korotkov wrote: > > > > I see that pg_wal_replay_wait_status() might look weird, but it seems > > > > to me like the best of feasible solutions. > > > > > > I haven't written many procedures, but our docs say: > > > > > > > Procedures do not return a function value; hence CREATE PROCEDURE > > > lacks a RETURNS clause. However, procedures can instead return data to > > > their callers via output parameters. > > > > > > Did you consider using an output parameter? > > > > Yes I did consider them and found two issues. > > 1) You still need to pass something to them. And that couldn't be > > default values. That's a bit awkward. > > 2) Usage of them causes extra snapshot to be held. > > I'll recheck if it's possible to workaround any of these two. > > I've rechecked the output parameters for stored procedures. And I think the behavior I previously discovered is an anomaly. > > CREATE PROCEDURE test_proc(a integer, out b integer) > LANGUAGE plpgsql > AS $$ > BEGIN > b := a; > END; > $$; > > # call test_proc(1); > ERROR: procedure test_proc(integer) does not exist > LINE 1: call test_proc(1); > ^ > HINT: No procedure matches the given name and argument types. You might need to add explicit type casts. > > # call test_proc(1,2); > b > --- > 1 > (1 row) > > Looks weird that we have to pass in some (ignored?) values for output parameters. In contrast, functions don't requirethis. > > CREATE FUNCTION test_func(a integer, out b integer) > LANGUAGE plpgsql > AS $$ > BEGIN > b := a; > END; > $$; > > # select test_func(1); > test_func > ----------- > 1 > (1 row) > > This makes me think we have an issue with stored procedures here. I'll try to investigate it further. Oh, this seems to be intentional [1] and seems to be part of standard [2]. Links 1. https://www.postgresql.org/docs/devel/xfunc-sql.html#XFUNC-OUTPUT-PARAMETERS-PROC 2. https://www.postgresql.org/message-id/2b8490fe-51af-e671-c504-47359dc453c5%402ndquadrant.com ------ Regards, Alexander Korotkov Supabase
On Mon, Nov 4, 2024 at 8:04 AM Michael Paquier <michael@paquier.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
On 2024-Nov-04, Alexander Korotkov wrote: > On Mon, Nov 4, 2024 at 8:04 AM Michael Paquier <michael@paquier.xyz> wrote: > > 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 wonder if it would be better to go back to the original idea of using special DDL syntax rather than a procedure. It seems we've been piling up hacks to get around the behavior of procedures, and we seem to have grown one more to handle repeatable read transactions. It's looking to me like there's just too much cruft in the quest to avoid having to reach consensus on new syntax. This might be a mistake. Is it possible to backtrack on that decision? -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Linux transformó mi computadora, de una `máquina para hacer cosas', en un aparato realmente entretenido, sobre el cual cada día aprendo algo nuevo" (Jaime Salinas)
On Mon, Nov 4, 2024 at 9:19 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > It's looking to me like there's just too much cruft in the quest to > avoid having to reach consensus on new syntax. This might be a mistake. > Is it possible to backtrack on that decision? There's also the patch that Heikki posted to wait using a protocol-level facility. Maybe that's just a better fit and we don't need either a procedure or new syntax. -- Robert Haas EDB: http://www.enterprisedb.com
On 04/11/2024 17:53, Robert Haas wrote: > On Mon, Nov 4, 2024 at 9:19 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: >> It's looking to me like there's just too much cruft in the quest to >> avoid having to reach consensus on new syntax. This might be a mistake. >> Is it possible to backtrack on that decision? > > There's also the patch that Heikki posted to wait using a > protocol-level facility. It was Peter E > Maybe that's just a better fit and we don't need either a procedure > or new syntax. I think it would still be good to expose the feature at SQL level too. Makes it easier to test and makes it usable without client library changes, for example. -- Heikki Linnakangas Neon (https://neon.tech)
On Mon, Nov 4, 2024 at 10:57 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > > There's also the patch that Heikki posted to wait using a > > protocol-level facility. > > It was Peter E Oops. > > Maybe that's just a better fit and we don't need either a procedure > > or new syntax. > I think it would still be good to expose the feature at SQL level too. > Makes it easier to test and makes it usable without client library > changes, for example. OK. Well then I agree with Álvaro. If using a procedure isn't working out nicely, it's better to consider alternatives than to force a round peg into a square hole (with due regard for the fact that you can fit anything through the square hole if you make the square hole big enough...). -- Robert Haas EDB: http://www.enterprisedb.com
On 04/11/2024 18:53, Robert Haas wrote: > On Mon, Nov 4, 2024 at 10:57 AM Heikki Linnakangas <hlinnaka@iki.fi> wrote: >>> Maybe that's just a better fit and we don't need either a procedure >>> or new syntax. >> I think it would still be good to expose the feature at SQL level too. >> Makes it easier to test and makes it usable without client library >> changes, for example. > > OK. Well then I agree with Álvaro. If using a procedure isn't working > out nicely, it's better to consider alternatives than to force a round > peg into a square hole (with due regard for the fact that you can fit > anything through the square hole if you make the square hole big > enough...). Agreed. Off the top of my, summary of issues I'm aware of: - Missing documentation. The function itself is documented, but it's clearly intended for implementing read-after-write consistency in a cluster setup, so we should document how to do that. - Not clear to me what the use case for the 'timeout' and 'noerror' arguments is. Why do we need it at all? (See first point on missing docs). - Separate pg_wal_replay_wait_status() function to get result status is awkward - Inconsistencies in behavior if server is not in recovery (Alexander's latest patch addresses this, but I didn't review it yet) - pg_wal_replay_wait_status() is incorrectly marked as STABLE and PARALLEL SAFE - Broken with default_transaction_isolation ='repeatable read' Let's revert commit e546989a26 and rethink. I think the basic pg_wal_replay_wait() interface before e546989a26 was fine. Some of the above are issues above were present before e546989a26 already and need to be addressed in any case, but I believe they can be fixed without changing the interface. -- Heikki Linnakangas Neon (https://neon.tech)
On Mon, Nov 4, 2024 at 4:19 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > On 2024-Nov-04, Alexander Korotkov wrote: > > > On Mon, Nov 4, 2024 at 8:04 AM Michael Paquier <michael@paquier.xyz> wrote: > > > > 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 wonder if it would be better to go back to the original idea of using > special DDL syntax rather than a procedure. It seems we've been piling > up hacks to get around the behavior of procedures, and we seem to have > grown one more to handle repeatable read transactions. > > It's looking to me like there's just too much cruft in the quest to > avoid having to reach consensus on new syntax. This might be a mistake. > Is it possible to backtrack on that decision? Before committing pg_wal_replay_wait() I was under impression that stored procedure would require the same amount of efforts than utility statement to make backend "snapshot-less". However, it appears that if we have implicit REPEATABLE READ transaction, stored procedure needs more efforts. That makes the whole decision, at least, questionable. ------ Regards, Alexander Korotkov Supabase
On Mon, Nov 4, 2024 at 8:04 AM Michael Paquier <michael@paquier.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? > > 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?). On second thought, status function is probably not so silly idea. Unlike result of utility statement/stored procedure, it could be used inside another function, that is one may implement custom result handling logic on postgres server side. On contrast, if you get something out of utility statement/stored procedure, it takes an extra roundtrip to push it back to postgres backend. > 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. Reverted. ------ Regards, Alexander Korotkov Supabase
On Mon, Nov 4, 2024 at 5:57 PM Heikki Linnakangas <hlinnaka@iki.fi> wrote: > On 04/11/2024 17:53, Robert Haas wrote: > > On Mon, Nov 4, 2024 at 9:19 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > >> It's looking to me like there's just too much cruft in the quest to > >> avoid having to reach consensus on new syntax. This might be a mistake. > >> Is it possible to backtrack on that decision? > > > > There's also the patch that Heikki posted to wait using a > > protocol-level facility. > > It was Peter E > > > Maybe that's just a better fit and we don't need either a procedure > > or new syntax. > I think it would still be good to expose the feature at SQL level too. > Makes it easier to test and makes it usable without client library > changes, for example. +1, Also, it could potentially has wider use cases. For example, waiting for replay of not latest changes, but some intermediate changes. Or issuing pg_wal_replay_wait() from another process than one which made the changes. ------ Regards, Alexander Korotkov Supabase