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

On 2020-03-06 08:54, Kyotaro Horiguchi wrote:
> The syntax seems getting confused. What happens if we typed in the
> command "WAIT FOR TIMESTAMP '...' UNTIL TIMESTAMP '....'"? It seems
> to me the options is useles. Couldn't the TIMEOUT option be a part of
> event? I know gram.y doesn't accept that syntax but it is not
> apparent from the description above.

I`ll fix the doc file.

Synopsis
==========
WAIT FOR [ANY | SOME | ALL] event [, event ...]
and event is:
LSN value [options]
TIMESTAMP value

and options is:
TIMEOUT delay
UNTIL TIMESTAMP timestamp

> As I read through the previous thread, one of the reason for this
> feature implemented as a syntax is it was inteded to be combined into
> BEGIN statement. If there is not any use case for the feature midst
> of a transaction, why don't you turn it into a part of BEGIN command?

It`s seem to have some limitations on hot standbys. I`ll take few days
to make a prototype.

>> Description
>> ==========
>> WAIT FOR - make to wait statements (that are beneath) on sleep until
>> event happens (Don’t process new queries until an event happens).
> ...
>> Notice: WAIT FOR will release on PostmasterDeath or Interruption
>> events
>> if they come earlier then LSN or timeout.
>
> I think interrupts ought to result in ERROR.
>
> wait.c adds a fair amount of code and uses proc-array based
> approach. But Thomas suggested queue-based approach and I also think
> it is better. We already have a queue-based mechanism that behaves
> almost the same with this feature in the comit code on master-side. It
> avoids spurious backend wakeups. Couldn't we extend SyncRepWaitForLSN
> or share a part of the code/infrastructures so that this feature can
> share the code?

I`ll take a look on.

Thank you for your review.

Rebased patch is attached.
--
Ivan Kartyshov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

I just wanted to express my excitement that this is being picked up again.
I was very much looking forward to this years ago, and the use case for me
is still there, so I am excited to see this moving again.

Sorry, I have some troubles on email sending.
On 2020-03-06 08:54, Kyotaro Horiguchi wrote:
> The syntax seems getting confused. What happens if we typed in the
> command "WAIT FOR TIMESTAMP '...' UNTIL TIMESTAMP '....'"? It seems
> to me the options is useles. Couldn't the TIMEOUT option be a part of
> event? I know gram.y doesn't accept that syntax but it is not
> apparent from the description above.

I`ll fix the doc file.

Synopsis
==========
WAIT FOR [ANY | SOME | ALL] event [, event ...]
and event is:
LSN value [options]
TIMESTAMP value

and options is:
TIMEOUT delay
UNTIL TIMESTAMP timestamp

> As I read through the previous thread, one of the reason for this
> feature implemented as a syntax is it was inteded to be combined into
> BEGIN statement. If there is not any use case for the feature midst
> of a transaction, why don't you turn it into a part of BEGIN command?

It`s seem to have some limitations on hot standbys. I`ll take few days
to make a prototype.

>> Description
>> ==========
>> WAIT FOR - make to wait statements (that are beneath) on sleep until
>> event happens (Don’t process new queries until an event happens).
> ...
>> Notice: WAIT FOR will release on PostmasterDeath or Interruption
>> events
>> if they come earlier then LSN or timeout.
>
> I think interrupts ought to result in ERROR.
>
> wait.c adds a fair amount of code and uses proc-array based
> approach. But Thomas suggested queue-based approach and I also think
> it is better. We already have a queue-based mechanism that behaves
> almost the same with this feature in the comit code on master-side. It
> avoids spurious backend wakeups. Couldn't we extend SyncRepWaitForLSN
> or share a part of the code/infrastructures so that this feature can
> share the code?

I`ll take a look on.

Thank you for your review.

Rebased patch is attached.

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

I made some improvements over old implementation WAIT FOR.

Synopsis
==========
WAIT FOR [ANY | SOME | ALL] event [, event ...]
and event is:
LSN value options
TIMESTAMP value

and options is:
TIMEOUT delay
UNTIL TIMESTAMP timestamp

ALL - option used by default.

P.S. Now I testing BEGIN base WAIT prototype as discussed earlier.

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

On 2020-03-17 15:47, Kartyshov Ivan wrote:
> Synopsis
> ==========
> WAIT FOR [ANY | SOME | ALL] event [, event ...]
I'm confused as to what SOME would mean in this
command's syntax, but I can see you removed it
from gram.y since the last patch. Did you
decide to not implement it after all?

Also, I had a look at the code and tested it a bit.

================
If I specify many events, here's what happens:

For WAIT_FOR_ALL strategy, it chooses
- maximum LSN
- maximum delay
and waits for the resulting event.

For WAIT_FOR_ANY strategy - same, but it uses
minimal LSN/delay.

In other words, statements
(1) WAIT FOR ALL
LSN '7F97208' TIMEOUT 11,
LSN '3002808' TIMEOUT 50;
(2) WAIT FOR ANY
LSN '7F97208' TIMEOUT 11,
LSN '3002808' TIMEOUT 50;
are essentially equivalent to:
(1) WAIT FOR LSN '7F97208' TIMEOUT 50;
(2) WAIT FOR LSN '3002808' TIMEOUT 11;

It seems a bit counter-intuitive to me, because
I expected events to be treated independently.
Is this the expected behaviour?

================
In utility.c:
if (event->delay < time_val)
time_val = event->delay / 1000;

Since event->delay is an int, the result will
be zero for any delay value less than 1000.
I suggest either dividing by 1000.0 or
explicitly converting int to float.

Also, shouldn't event->delay be divided
by 1000 in the 'if' part as well?

================
You compare two LSN-s using pg_lsn_cmp():
res = DatumGetUInt32(
DirectFunctionCall2(pg_lsn_cmp,
lsn, trg_lsn));

As far as I understand, it'd be enough to use
operators such as "<=", as you do in wait.c:
/* If LSN has been replayed */
if (trg_lsn <= cur_lsn)

--
Anna Akenteva
Postgres Professional:
The Russian Postgres Company
http://www.postgrespro.com