Re: [HACKERS] make async slave to wait for lsn to be replayed

On Tue, 28 Feb 2023 at 05:13, Kartyshov Ivan <i.kartyshov@postgrespro.ru> wrote:

Below I provide the implementation of patches for the first three types.
I propose to discuss this feature again/

Oof, that doesn't really work with the cfbot. It tries to apply all
three patches and of course the second and third fail to apply.

In any case this seems like a lot of effort to me. I would suggest you
just pick one avenue and provide that patch for discussion and just
ask whether people would prefer any of the alternative syntaxes.

Fwiw I prefer the functions approach. I do like me some nice syntax
but I don't see any particular advantage of the special syntax in this
case. They don't seem to provide any additional expressiveness.

That said, I'm not a fan of the specific function names. Remember that
we have polymorphic functions so you could probably just have an
option argument:

pg_lsn_wait('LSN', [timeout]) returns boolean

(just call it with a timeout of 0 to do a no-wait)

I'll set the patch to "Waiting on Author" for now. If you feel you're
still looking for more opinions from others maybe set it back to Needs
Review but honestly there are a lot of patches so you probably won't
see much this commitfest unless you have a patch that shows in
cfbot.cputube.org as applying and which looks ready to commit.

--
greg

On Wed, Mar 01, 2023 at 03:31:06PM -0500, Greg Stark wrote:

Fwiw I prefer the functions approach. I do like me some nice syntax
but I don't see any particular advantage of the special syntax in this
case. They don't seem to provide any additional expressiveness.

So do I, eventhough I saw a point that sticking to a function or a
procedure approach makes the wait stick with more MVCC rules, like the
fact that the wait may be holding a snapshot for longer than
necessary. The grammar can be more extensible without more keywords
with DefElems, still I'd like to think that we should not introduce
more restrictions in the parser if we have ways to work around it.
Using a procedure or function approach is more extensible in its own
ways, and it also depends on the data waiting for (there could be more
than one field as well for a single wait pattern?).

I'll set the patch to "Waiting on Author" for now. If you feel you're
still looking for more opinions from others maybe set it back to Needs
Review but honestly there are a lot of patches so you probably won't
see much this commitfest unless you have a patch that shows in
cfbot.cputube.org as applying and which looks ready to commit.

While looking at all the patches proposed, I have noticed that all the
approaches proposed force a wakeup of the waiters in the redo loop of
the startup process for each record, before reading the next record.
It strikes me that there is some interaction with custom resource
managers here, where it is possible to poke at the waiters not for
each record, but after reading some specific records. Something
out-of-core would not be as responsive as the per-record approach,
still responsive enough that the waiters wait on input for an
acceptable amount of time, depending on the frequency of the records
generated by a primary to wake them up. Just something that popped
into my mind while looking a bit at the threads.
--
Michael

On 28.02.23 11:10, Kartyshov Ivan wrote:

3) Procedure style: Tom Lane and Kyotaro (wait_proc_v1.patch)
/messages/by-id/27171.1586439221@sss.pgh.pa.us
/messages/by-id/20210121.173009.235021120161403875.horikyota.ntt@gmail.com
==========
advantages: no new words in grammar,like it made in
pg_last_wal_replay_lsn, no snapshots need
disadvantages: a little harder to remember names
SELECT pg_waitlsn(‘LSN’, timeout);
SELECT pg_waitlsn_infinite(‘LSN’);
SELECT pg_waitlsn_no_wait(‘LSN’);

Of the presented options, I prefer this one. (Maybe with a "_" between
"wait" and "lsn".)

But I wonder how a client is going to get the LSN. How would all of
this be used by a client? I can think of a scenarios where you have an
application that issues a bunch of SQL commands and you have some kind
of pooler in the middle that redirects those commands to different
hosts, and what you really want is to have it transparently behave as if
it's just a single host. Do we want to inject a bunch of "SELECT
pg_get_lsn()", "SELECT pg_wait_lsn()" calls into that?

I'm tempted to think this could be a protocol-layer facility. Every
query automatically returns the current LSN, and every query can also
send along an LSN to wait for, and the client library would just keep
track of the LSN for (what it thinks of as) the connection. So you get
some automatic serialization without having to modify your client code.

That said, exposing this functionality using functions could be a valid
step in that direction, so that you can at least build out the actual
internals of the functionality and test it out.

Here I made new patch of feature, discussed above.

WAIT FOR procedure - waits for certain lsn on pause
==========
Synopsis
==========
SELECT pg_wait_lsn(‘LSN’, timeout) returns boolean

Where timeout = 0, will wait infinite without timeout
And if timeout = 1, then just check if lsn was replayed

How to use it
==========

Greg Stark wrote:

That said, I'm not a fan of the specific function names. Remember that
we have polymorphic functions so you could probably just have an
option argument:

If you have any example, I will be glade to see them. Ьy searches have
not been fruitful.

Michael Paquier wrote:

While looking at all the patches proposed, I have noticed that all the
approaches proposed force a wakeup of the waiters in the redo loop of
the startup process for each record, before reading the next record.
It strikes me that there is some interaction with custom resource
managers here, where it is possible to poke at the waiters not for
each record, but after reading some specific records. Something
out-of-core would not be as responsive as the per-record approach,
still responsive enough that the waiters wait on input for an
acceptable amount of time, depending on the frequency of the records
generated by a primary to wake them up. Just something that popped
into my mind while looking a bit at the threads.

I`ll work on this idea to have less impact on the redo system.

On 2023-03-02 13:33, Peter Eisentraut wrote:

But I wonder how a client is going to get the LSN. How would all of
this be used by a client?

As I wrote earlier main purpose of the feature is to achieve
read-your-writes-consistency, while using async replica for reads and
primary for writes. In that case lsn of last modification is stored
inside application.

I'm tempted to think this could be a protocol-layer facility. Every
query automatically returns the current LSN, and every query can also
send along an LSN to wait for, and the client library would just keep
track of the LSN for (what it thinks of as) the connection. So you
get some automatic serialization without having to modify your client
code.

Yes it sounds very tempted. But I think community will be against it.

--
Ivan Kartyshov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Update patch to fix conflict with master
--
Ivan Kartyshov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Fix build.meson troubles

--
Ivan Kartyshov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

All rebased and tested

--
Ivan Kartyshov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company@postgrespro.ru>
 

Hi, Ivan!

On Fri, Jun 30, 2023 at 11:32 AM Картышов Иван <i.kartyshov@postgrespro.ru>
wrote:

All rebased and tested

Thank you for continuing to work on this patch.

I see you're concentrating on the procedural version of this feature. But
when you're calling a procedure within a normal SQL statement, the executor
gets a snapshot and holds it until the procedure finishes. In the case the
WAL record conflicts with this snapshot, the query will be canceled.
Alternatively, when hot_standby_feedback = on, the query and WAL replayer
will be in a deadlock (WAL replayer will wait for the query to finish, and
the query will wait for WAL replayed). Do you see this issue? Or do you
think I'm missing something?

XLogRecPtr
GetMinWaitedLSN(void)
{
return state->min_lsn.value;
}

You definitely shouldn't access directly the fields
inside pg_atomic_uint64. In this particular case, you should
use pg_atomic_read_u64().

Also, I think there is a race condition.

/* Check if we already reached the needed LSN */
if (cur_lsn >= target_lsn)
return true;

AddWaitedLSN(target_lsn);

Imagine, PerformWalRecovery() will replay a record after the check, but
before AddWaitedLSN(). This code will start the waiting cycle even if the
LSN is already achieved. Surely this cycle will end soon because it
rechecks LSN value each 100 ms. But anyway, I think there should be
another check after AddWaitedLSN() for the sake of consistency.

------
Regards,
Alexander Korotkov

Hi!

On Wed, Oct 4, 2023 at 1:22 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:

I see you're concentrating on the procedural version of this feature. But when you're calling a procedure within a normal SQL statement, the executor gets a snapshot and holds it until the procedure finishes. In the case the WAL record conflicts with this snapshot, the query will be canceled. Alternatively, when hot_standby_feedback = on, the query and WAL replayer will be in a deadlock (WAL replayer will wait for the query to finish, and the query will wait for WAL replayed). Do you see this issue? Or do you think I'm missing something?

I'm sorry, I actually meant hot_standby_feedback = off
(hot_standby_feedback = on actually avoids query conflicts). I
managed to reproduce this problem.

master: create table test as (select i from generate_series(1,10000) i);
slave conn1: select pg_wal_replay_pause();
master: delete from test;
master: vacuum test;
master: select pg_current_wal_lsn();
slave conn2: select pg_wait_lsn('the value from previous query'::pg_lsn, 0);
slave conn1: select pg_wal_replay_resume();
slave conn2: ERROR: canceling statement due to conflict with recovery
DETAIL: User query might have needed to see row versions that must be removed.

Needless to say, this is very undesirable behavior. This happens
because pg_wait_lsn() has to run within a snapshot as any other
function. This is why I think this functionality should be
implemented as a separate statement.

Another issue I found is that pg_wait_lsn() hangs on the primary. I
think an error should be reported instead.

------
Regards,
Alexander Korotkov

Alexander, thank you for your review and pointing this issues. According to
them I made some fixes and rebase all patch.

But I can`t repeat your ERROR. Not with hot_standby_feedback = on nor 
hot_standby_feedback = off.master: create table test as (select i from generate_series(1,10000) i);
slave conn1: select pg_wal_replay_pause();
master: delete from test;
master: vacuum test;
master: select pg_current_wal_lsn();
slave conn2: select pg_wait_lsn('the value from previous query'::pg_lsn, 0);
slave conn1: select pg_wal_replay_resume();
slave conn2: ERROR: canceling statement due to conflict with recovery
DETAIL: User query might have needed to see row versions that must be removed.Also I use little hack to work out of snapshot similar to SnapshotResetXmin.

Patch rebased and ready for review.

Hi,

I used the latest code and found some conflicts while applying. Which PG
version did you rebase?

Regards
Bowen Shi

On Thu, Nov 23, 2023 at 5:52 AM Bowen Shi <zxwsbg12138@gmail.com> wrote:

I used the latest code and found some conflicts while applying. Which PG version did you rebase?

I've successfully applied the patch on bc3c8db8ae. But I've used
"patch -p1 < wait_proc_v6.patch", git am doesn't work.

------
Regards,
Alexander Korotkov

On Mon, Nov 20, 2023 at 1:10 PM Картышов Иван
<i.kartyshov@postgrespro.ru> wrote:

Alexander, thank you for your review and pointing this issues. According to
them I made some fixes and rebase all patch.

But I can`t repeat your ERROR. Not with hot_standby_feedback = on nor
hot_standby_feedback = off.

master: create table test as (select i from generate_series(1,10000) i);
slave conn1: select pg_wal_replay_pause();
master: delete from test;
master: vacuum test;
master: select pg_current_wal_lsn();
slave conn2: select pg_wait_lsn('the value from previous query'::pg_lsn, 0);
slave conn1: select pg_wal_replay_resume();
slave conn2: ERROR: canceling statement due to conflict with recovery
DETAIL: User query might have needed to see row versions that must be removed.

Also I use little hack to work out of snapshot similar to SnapshotResetXmin.

Patch rebased and ready for review.

I've retried my case with v6 and it doesn't fail anymore. But I
wonder how safe it is to reset xmin within the user-visible function?
We have no guarantee that the function is not called inside the
complex query. Then how will the rest of the query work with xmin
reset? Separate utility statement still looks like more safe option
for me.

------
Regards,
Alexander Korotkov

On 2023-11-27 03:08, Alexander Korotkov wrote:

I've retried my case with v6 and it doesn't fail anymore. But I
wonder how safe it is to reset xmin within the user-visible function?
We have no guarantee that the function is not called inside the
complex query. Then how will the rest of the query work with xmin
reset? Separate utility statement still looks like more safe option
for me.

As you mentioned, we can`t guarantee that the function is not called
inside the complex query, but we can return the xmin after waiting.
But you are right and separate utility statement still looks more safe.
So I want to bring up the discussion on separate utility statement
again.

--
Ivan Kartyshov
Postgres Professional: www.postgrespro.com

Should rise disscusion on separate utility statement or find
case where procedure version is failed.

1) Classic (wait_classic_v3.patch)
/messages/by-id/3cc883048264c2e9af022033925ff8db@postgrespro.ru
==========
advantages: multiple wait events, separate WAIT FOR statement
disadvantages: new words in grammar

WAIT FOR [ANY | ALL] event [, ...]
BEGIN [ WORK | TRANSACTION ] [ transaction_mode [, ...] ]
[ WAIT FOR [ANY | ALL] event [, ...]]
event:
LSN value
TIMEOUT number_of_milliseconds
timestamp

2) After style: Kyotaro and Freund (wait_after_within_v2.patch)
/messages/by-id/d3ff2e363af60b345f82396992595a03@postgrespro.ru
==========
advantages: no new words in grammar
disadvantages: a little harder to understand

AFTER lsn_event [ WITHIN delay_milliseconds ] [, ...]
BEGIN [ WORK | TRANSACTION ] [ transaction_mode [, ...] ]
[ AFTER lsn_event [ WITHIN delay_milliseconds ]]
START [ WORK | TRANSACTION ] [ transaction_mode [, ...] ]
[ AFTER lsn_event [ WITHIN delay_milliseconds ]]

3) Procedure style: Tom Lane and Kyotaro (wait_proc_v7.patch)
/messages/by-id/27171.1586439221@sss.pgh.pa.us
/messages/by-id/20210121.173009.235021120161403875.horikyota.ntt@gmail.com
==========
advantages: no new words in grammar,like it made in
pg_last_wal_replay_lsn
disadvantages: use snapshot xmin trick
SELECT pg_waitlsn(‘LSN’, timeout);
SELECT pg_waitlsn_infinite(‘LSN’);
SELECT pg_waitlsn_no_wait(‘LSN’);

Regards
--
Ivan Kartyshov
Postgres Professional: www.postgrespro.com

On Fri, Dec 8, 2023 at 11:20 AM Kartyshov Ivan
<i.kartyshov@postgrespro.ru> wrote:

On 2023-11-27 03:08, Alexander Korotkov wrote:

I've retried my case with v6 and it doesn't fail anymore. But I
wonder how safe it is to reset xmin within the user-visible function?
We have no guarantee that the function is not called inside the
complex query. Then how will the rest of the query work with xmin
reset? Separate utility statement still looks like more safe option
for me.

As you mentioned, we can`t guarantee that the function is not called
inside the complex query, but we can return the xmin after waiting.

Returning xmin back isn't safe. Especially after potentially long
waiting. The snapshot could be no longer valid, because the
corresponding tuples could be VACUUM'ed.

------
Regards,
Alexander Korotkov

On Fri, Dec 8, 2023 at 11:46 AM Kartyshov Ivan
<i.kartyshov@postgrespro.ru> wrote:

Should rise disscusion on separate utility statement or find
case where procedure version is failed.

1) Classic (wait_classic_v3.patch)
/messages/by-id/3cc883048264c2e9af022033925ff8db@postgrespro.ru
==========
advantages: multiple wait events, separate WAIT FOR statement
disadvantages: new words in grammar

WAIT FOR [ANY | ALL] event [, ...]
BEGIN [ WORK | TRANSACTION ] [ transaction_mode [, ...] ]
[ WAIT FOR [ANY | ALL] event [, ...]]
event:
LSN value
TIMEOUT number_of_milliseconds
timestamp

Nice, but as you stated requires new keywords.

2) After style: Kyotaro and Freund (wait_after_within_v2.patch)
/messages/by-id/d3ff2e363af60b345f82396992595a03@postgrespro.ru
==========
advantages: no new words in grammar
disadvantages: a little harder to understand

AFTER lsn_event [ WITHIN delay_milliseconds ] [, ...]
BEGIN [ WORK | TRANSACTION ] [ transaction_mode [, ...] ]
[ AFTER lsn_event [ WITHIN delay_milliseconds ]]
START [ WORK | TRANSACTION ] [ transaction_mode [, ...] ]
[ AFTER lsn_event [ WITHIN delay_milliseconds ]]

+1 from me

3) Procedure style: Tom Lane and Kyotaro (wait_proc_v7.patch)
/messages/by-id/27171.1586439221@sss.pgh.pa.us
/messages/by-id/20210121.173009.235021120161403875.horikyota.ntt@gmail.com
==========
advantages: no new words in grammar,like it made in
pg_last_wal_replay_lsn
disadvantages: use snapshot xmin trick
SELECT pg_waitlsn(‘LSN’, timeout);
SELECT pg_waitlsn_infinite(‘LSN’);
SELECT pg_waitlsn_no_wait(‘LSN’);

Nice, because simplicity. But only safe if called within the simple
query containing nothing else. Validating this from the function
kills the simplicity.

------
Regards,
Alexander Korotkov

On Fri, 8 Dec 2023 at 15:17, Kartyshov Ivan <i.kartyshov@postgrespro.ru> wrote:

Should rise disscusion on separate utility statement or find
case where procedure version is failed.

1) Classic (wait_classic_v3.patch)
/messages/by-id/3cc883048264c2e9af022033925ff8db@postgrespro.ru
==========
advantages: multiple wait events, separate WAIT FOR statement
disadvantages: new words in grammar

WAIT FOR [ANY | ALL] event [, ...]
BEGIN [ WORK | TRANSACTION ] [ transaction_mode [, ...] ]
[ WAIT FOR [ANY | ALL] event [, ...]]
event:
LSN value
TIMEOUT number_of_milliseconds
timestamp

2) After style: Kyotaro and Freund (wait_after_within_v2.patch)
/messages/by-id/d3ff2e363af60b345f82396992595a03@postgrespro.ru
==========
advantages: no new words in grammar
disadvantages: a little harder to understand

AFTER lsn_event [ WITHIN delay_milliseconds ] [, ...]
BEGIN [ WORK | TRANSACTION ] [ transaction_mode [, ...] ]
[ AFTER lsn_event [ WITHIN delay_milliseconds ]]
START [ WORK | TRANSACTION ] [ transaction_mode [, ...] ]
[ AFTER lsn_event [ WITHIN delay_milliseconds ]]

3) Procedure style: Tom Lane and Kyotaro (wait_proc_v7.patch)
/messages/by-id/27171.1586439221@sss.pgh.pa.us
/messages/by-id/20210121.173009.235021120161403875.horikyota.ntt@gmail.com
==========
advantages: no new words in grammar,like it made in
pg_last_wal_replay_lsn
disadvantages: use snapshot xmin trick
SELECT pg_waitlsn(‘LSN’, timeout);
SELECT pg_waitlsn_infinite(‘LSN’);
SELECT pg_waitlsn_no_wait(‘LSN’);

Few of the tests have aborted at [1]https://cirrus-ci.com/task/5618308515364864 in CFBot with:
0000058`9c7ff550 00007ff6`5bdff1f4
postgres!pg_atomic_compare_exchange_u64_impl(
struct pg_atomic_uint64 * ptr = 0x00000000`00000008,
unsigned int64 * expected = 0x00000058`9c7ff5a0,
unsigned int64 newval = 0)+0x34
[c:\cirrus\src\include\port\atomics\generic-msvc.h @ 83]
00000058`9c7ff580 00007ff6`5bdff256 postgres!pg_atomic_read_u64_impl(
struct pg_atomic_uint64 * ptr = 0x00000000`00000008)+0x24
[c:\cirrus\src\include\port\atomics\generic.h @ 323]
00000058`9c7ff5c0 00007ff6`5bdfef67 postgres!pg_atomic_read_u64(
struct pg_atomic_uint64 * ptr = 0x00000000`00000008)+0x46
[c:\cirrus\src\include\port\atomics.h @ 430]
00000058`9c7ff5f0 00007ff6`5bc98fc3
postgres!GetMinWaitedLSN(void)+0x17
[c:\cirrus\src\backend\commands\wait.c @ 176]
00000058`9c7ff620 00007ff6`5bc82fb9
postgres!PerformWalRecovery(void)+0x4c3
[c:\cirrus\src\backend\access\transam\xlogrecovery.c @ 1788]
00000058`9c7ff6e0 00007ff6`5bffc651
postgres!StartupXLOG(void)+0x989
[c:\cirrus\src\backend\access\transam\xlog.c @ 5562]
00000058`9c7ff870 00007ff6`5bfed38b
postgres!StartupProcessMain(void)+0xd1
[c:\cirrus\src\backend\postmaster\startup.c @ 288]
00000058`9c7ff8a0 00007ff6`5bff49fd postgres!AuxiliaryProcessMain(
AuxProcType auxtype = StartupProcess (0n0))+0x1fb
[c:\cirrus\src\backend\postmaster\auxprocess.c @ 139]
00000058`9c7ff8e0 00007ff6`5beb7674 postgres!SubPostmasterMain(

More details are available at [2]https://api.cirrus-ci.com/v1/artifact/task/5618308515364864/crashlog/crashlog-postgres.exe_0008_2023-12-08_07-48-37-722.txt.

[1]: https://cirrus-ci.com/task/5618308515364864
[2]: https://api.cirrus-ci.com/v1/artifact/task/5618308515364864/crashlog/crashlog-postgres.exe_0008_2023-12-08_07-48-37-722.txt

Regards,
Vignesh

Rebased and ready for review.
I left only versions (due to irreparable problems)

1) Classic (wait_classic_v4.patch)
/messages/by-id/3cc883048264c2e9af022033925ff8db@postgrespro.ru
==========
advantages: multiple wait events, separate WAIT FOR statement
disadvantages: new words in grammar

WAIT FOR [ANY | ALL] event [, ...]
BEGIN [ WORK | TRANSACTION ] [ transaction_mode [, ...] ]
[ WAIT FOR [ANY | ALL] event [, ...]]
event:
LSN value
TIMEOUT number_of_milliseconds
timestamp

2) After style: Kyotaro and Freund (wait_after_within_v3.patch)
/messages/by-id/d3ff2e363af60b345f82396992595a03@postgrespro.ru
==========
advantages: no new words in grammar
disadvantages: a little harder to understand

AFTER lsn_event [ WITHIN delay_milliseconds ] [, ...]
BEGIN [ WORK | TRANSACTION ] [ transaction_mode [, ...] ]
[ AFTER lsn_event [ WITHIN delay_milliseconds ]]
START [ WORK | TRANSACTION ] [ transaction_mode [, ...] ]
[ AFTER lsn_event [ WITHIN delay_milliseconds ]]

--
Ivan Kartyshov
Postgres Professional: www.postgrespro.com