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

On 2018-03-06 14:50, Simon Riggs wrote:
> On 6 March 2018 at 11:24, Dmitry Ivanov <d(dot)ivanov(at)postgrespro(dot)ru>
> wrote:
>>> In PG11, I propose the following command, sticking mostly to Ants'
>>> syntax, and allowing to wait for multiple events before it returns.
>>> It
>>> doesn't hold snapshot and will not get cancelled by Hot Standby.
>>>
>>> WAIT FOR event [, event ...] options
>>>
>>> event is
>>> LSN value
>>> TIMESTAMP value
>>>
>>> options
>>> TIMEOUT delay
>>> UNTIL TIMESTAMP timestamp
>>> (we have both, so people don't need to do math, they can use
>>> whichever
>>> they have)
>>
>>
>> I have a (possibly) dumb question: if we have specified several
>> events,
>> should WAIT finish if only one of them triggered? It's not immediately
>> obvious if we're waiting for ALL of them to trigger, or just one will
>> suffice (ANY). IMO the syntax could be extended to something like:
>>
>> WAIT FOR [ANY | ALL] event [, event ...] options,
>>
>> with ANY being the default variant.
>
> +1

Here I made new patch of feature, discussed above.

WAIT FOR - wait statement to pause beneath statements
==========

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

and options is:
TIMEOUT delay
UNTIL TIMESTAMP timestamp
Description
==========
WAIT FOR - make to wait statements (that are beneath) on sleep until
event happens (Don’t process new queries until an event happens).

How to use it
==========
WAIT FOR LSN ‘LSN’ [, timeout in ms];

#Wait until LSN 0/303EC60 will be replayed, or 10 second passed.
WAIT FOR ANY LSN ‘0/303EC60’, TIMEOUT 10000;

#Or same without timeout.
WAIT FOR LSN ‘0/303EC60’;

#Or wait for some timestamp.
WAIT FOR TIMESTAMP '2020-01-02 17:20:19.028161+03';

#Wait until ALL events occur: LSN to be replayed and timestamp
passed.
WAIT FOR ALL LSN ‘0/303EC60’, TIMESTAMP '2020-01-28 11:10:39.021341+03';

Notice: WAIT FOR will release on PostmasterDeath or Interruption events
if they come earlier then LSN or timeout.

Testing the implementation
======================
The implementation was tested with src/test/recovery/t/018_waitfor.pl

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

Hello.

I looked this briefly but not tested.

At Fri, 06 Mar 2020 00:24:01 +0300, Kartyshov Ivan <i(dot)kartyshov(at)postgrespro(dot)ru> wrote in
> On 2018-03-06 14:50, Simon Riggs wrote:
> > On 6 March 2018 at 11:24, Dmitry Ivanov <d(dot)ivanov(at)postgrespro(dot)ru>
> > wrote:
> >>> In PG11, I propose the following command, sticking mostly to Ants'
> >>> syntax, and allowing to wait for multiple events before it returns. It
> >>> doesn't hold snapshot and will not get cancelled by Hot Standby.
> >>> WAIT FOR event [, event ...] options
> >>> event is
> >>> LSN value
> >>> TIMESTAMP value
> >>> options
> >>> TIMEOUT delay
> >>> UNTIL TIMESTAMP timestamp
> >>> (we have both, so people don't need to do math, they can use whichever
> >>> they have)
> >> I have a (possibly) dumb question: if we have specified several
> >> events,
> >> should WAIT finish if only one of them triggered? It's not immediately
> >> obvious if we're waiting for ALL of them to trigger, or just one will
> >> suffice (ANY). IMO the syntax could be extended to something like:
> >> WAIT FOR [ANY | ALL] event [, event ...] options,
> >> with ANY being the default variant.
> > +1
>
> Here I made new patch of feature, discussed above.
>
> WAIT FOR - wait statement to pause beneath statements
> ==========
>
> Synopsis
> ==========
> WAIT FOR [ANY | SOME | ALL] event [, event ...] options
> and event is:
> LSN value
> TIMESTAMP value
>
> and options is:
> TIMEOUT delay
> UNTIL TIMESTAMP timestamp

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.

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?

> 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?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

As it was discussed earlier, I added wait for statement into begin/start
statement.

Synopsis
==========
BEGIN [ WORK | TRANSACTION ] [ transaction_mode[, ...] ] wait_for_event
where transaction_mode is one of:
ISOLATION LEVEL { SERIALIZABLE | REPEATABLE READ | READ
COMMITTED | READ UNCOMMITTED }
READ WRITE | READ ONLY
[ NOT ] DEFERRABLE

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

and options is:
TIMEOUT delay
UNTIL TIMESTAMP timestamp

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

On 2020-03-21 14:16, Kartyshov Ivan wrote:
> As it was discussed earlier, I added wait for statement into
> begin/start statement.
Thanks! To address the discussion: I like the idea of having WAIT as a
part of BEGIN statement rather than a separate command, as Thomas Munro
suggested. That way, the syntax itself enforces that WAIT FOR LSN will
actually take effect, even for single-snapshot transactions. It seems
more convenient for the user, who won't have to remember the details
about how WAIT interacts with isolation levels.

> BEGIN [ WORK | TRANSACTION ] [ transaction_mode[, ...] ] wait_for_event
Not sure about this, but could we add "WAIT FOR .." as another
transaction_mode rather than a separate thing? That way, user won't have
to worry about the order. As of now, one should remember to always put
WAIT FOR as the Last parameter in the BEGIN statement.

> and event is:
> LSN value [options]
> TIMESTAMP value
I would maybe remove WAIT FOR TIMESTAMP. As Robert Haas has pointed out,
it seems a lot like pg_sleep_until(). Besides, it doesn't necessarily
need to be connected to transaction start, which makes it different from
WAIT FOR LSN - so I wouldn't mix them together.

I had another look at the code:

===
In WaitShmemSize() you might want to use functions that check for
overflow - add_size()/mul_size(). They're used in similar situations,
for example in BTreeShmemSize().

===
This is how WaitUtility() is called - note that time_val will always be
> 0:
+    if (time_val <= 0)
+        time_val = 1;
+...
+    res = WaitUtility(lsn, (int)(time_val * 1000), dest);

(time_val * 1000) is passed to WaitUtility() as the delay argument. And
inside WaitUtility() we have this:

+if (delay > 0)
+    latch_events = WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH;
+else
+    latch_events = WL_LATCH_SET | WL_POSTMASTER_DEATH;

Since we always pass a delay value greater than 0, we'll never get to
the "else" clause here and we'll never be ready to wait for LSN forever.
Perhaps due to that, the current test outputs this after a simple WAIT
FOR LSN command:
psql:<stdin>:1: NOTICE:  LSN is not reached.

===
Speaking of tests,

When I tried to test BEGIN TRANSACTION WAIT FOR LSN, I got a segfault:
LOG: statement: BEGIN TRANSACTION WAIT FOR LSN '0/3002808'
LOG: server process (PID 10385) was terminated by signal 11:
Segmentation fault
DETAIL: Failed process was running: COMMIT

Could you add some more tests to the patch when this is fixed? With WAIT
as part of BEGIN statement + with things such as WAIT FOR ALL ... / WAIT
FOR ANY ... / WAIT FOR LSN ... UNTIL TIMESTAMP ...

===
In WaitSetLatch() we should probably check backend for NULL before
calling SetLatch(&backend->procLatch)

We might also need to check wait statement for NULL in these two cases:
+  case T_TransactionStmt:
+  {...
+      result = transformWaitForStmt(pstate, (WaitStmt *) stmt->wait);

case TRANS_STMT_START:
{...
+ WaitStmt *waitstmt = (WaitStmt *) stmt->wait;
+ res = WaitMain(waitstmt, dest);

===
After we added the "wait" attribute to TransactionStmt struct, do we
perhaps need to add something to _copyTransactionStmt() /
_equalTransactionStmt()?

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

Anna, thank you for your review.

On 2020-03-25 21:10, Anna Akenteva wrote:
> On 2020-03-21 14:16, Kartyshov Ivan wrote:
>> and event is:
>> LSN value [options]
>> TIMESTAMP value
> I would maybe remove WAIT FOR TIMESTAMP. As Robert Haas has pointed
> out, it seems a lot like pg_sleep_until(). Besides, it doesn't
> necessarily need to be connected to transaction start, which makes it
> different from WAIT FOR LSN - so I wouldn't mix them together.
I don't mind.
But I think we should get one more opinions on this point.

> ===
> This is how WaitUtility() is called - note that time_val will always be
> > 0:
> +    if (time_val <= 0)
> +        time_val = 1;
> +...
> +    res = WaitUtility(lsn, (int)(time_val * 1000), dest);
>
> (time_val * 1000) is passed to WaitUtility() as the delay argument.
> And inside WaitUtility() we have this:
>
> +if (delay > 0)
> +    latch_events = WL_LATCH_SET | WL_TIMEOUT | WL_POSTMASTER_DEATH;
> +else
> +    latch_events = WL_LATCH_SET | WL_POSTMASTER_DEATH;
>
> Since we always pass a delay value greater than 0, we'll never get to
> the "else" clause here and we'll never be ready to wait for LSN
> forever. Perhaps due to that, the current test outputs this after a
> simple WAIT FOR LSN command:
> psql:<stdin>:1: NOTICE:  LSN is not reached.
I fix it, and Interruptions in last patch.

Anna, feel free to work on this patch.

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

On 2020-03-27 04:15, Kartyshov Ivan wrote:
> Anna, feel free to work on this patch.

Ivan and I worked on this patch a bit more. We fixed the bugs that we
could find and cleaned up the code. For now, we've kept both options:
WAIT as a standalone statement and WAIT as a part of BEGIN. The new
patch is attached.

The syntax looks a bit different now:

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

Now, one event cannot contain both an LSN and a TIMEOUT. With such
syntax, the behaviour seems to make sense. For the (default) WAIT FOR
ALL strategy, we pick the maximum LSN and maximum allowed timeout, and
wait for the LSN till the timeout is over. If no timeout is specified,
we wait forever. If no LSN is specified, we just wait for the time to
pass. For the WAIT FOR ANY strategy, it's the same but we pick minimum
LSN and timeout.

There are still some questions left:
1) Should we only keep the BEGIN option, or does the standalone command
have potential after all?
2) Should we change the grammar so that WAIT can be in any position of
the BEGIN statement, not necessarily in the end? Ivan and I haven't come
to a consensus about this, so more opinions would be helpful.
3) Since we added the "wait" attribute to TransactionStmt struct, do we
need to add something to _copyTransactionStmt() /
_equalTransactionStmt()?

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

On 2020-04-01 02:26, Anna Akenteva wrote:
> On 2020-03-27 04:15, Kartyshov Ivan wrote:
>> Anna, feel free to work on this patch.
>
> Ivan and I worked on this patch a bit more. We fixed the bugs that we
> could find and cleaned up the code. For now, we've kept both options:
> WAIT as a standalone statement and WAIT as a part of BEGIN. The new
> patch is attached.
>
> The syntax looks a bit different now:
>
> - WAIT FOR [ANY | ALL] event [, ...]
> - BEGIN [ WORK | TRANSACTION ] [ transaction_mode [, ...] ] [ WAIT FOR
> [ANY | ALL] event [, ...]]
> where event is one of:
> LSN value
> TIMEOUT number_of_milliseconds
> timestamp
>
> Now, one event cannot contain both an LSN and a TIMEOUT.
>

In my understanding the whole idea of having TIMEOUT was to do something
like 'Do wait for this LSN to be replicated, but no longer than TIMEOUT
milliseconds'. What is the point of having plain TIMEOUT? It seems to be
equivalent to pg_sleep, doesn't it?

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

I did some code cleanup and added tests - both for the standalone WAIT
FOR statement and for WAIT FOR as a part of BEGIN. The new patch is
attached.

On 2020-04-03 17:29, Alexey Kondratov wrote:
> On 2020-04-01 02:26, Anna Akenteva wrote:
>>
>> - WAIT FOR [ANY | ALL] event [, ...]
>> - BEGIN [ WORK | TRANSACTION ] [ transaction_mode [, ...] ] [ WAIT FOR
>> [ANY | ALL] event [, ...]]
>> where event is one of:
>> LSN value
>> TIMEOUT number_of_milliseconds
>> timestamp
>>
>> Now, one event cannot contain both an LSN and a TIMEOUT.
>>
>
> In my understanding the whole idea of having TIMEOUT was to do
> something like 'Do wait for this LSN to be replicated, but no longer
> than TIMEOUT milliseconds'. What is the point of having plain TIMEOUT?
> It seems to be equivalent to pg_sleep, doesn't it?
>

In the patch that I reviewed, you could do things like:
WAIT FOR
LSN lsn0,
LSN lsn1 TIMEOUT time1,
LSN lsn2 TIMEOUT time2;
and such a statement was in practice equivalent to
WAIT FOR LSN(max(lsn0, lsn1, lsn2)) TIMEOUT (max(time1, time2))

As you can see, even though grammatically lsn1 is grouped with time1 and
lsn2 is grouped with time2, both timeouts that we specified are not
connected to their respective LSN-s, and instead they kinda act like
global timeouts. Therefore, I didn't see a point in keeping TIMEOUT
necessarily grammatically connected to LSN.

In the new syntax our statement would look like this:
WAIT FOR LSN lsn0, LSN lsn1, LSN lsn2, TIMEOUT time1, TIMEOUT time2;
TIMEOUT-s are not forced to be grouped with LSN-s anymore, which makes
it more clear that all specified TIMEOUTs will be global and will apply
to all LSN-s at once.

The point of having TIMEOUT is still to let us limit the time of waiting
for LSNs. It's just that with the new syntax, we can also use TIMEOUT
without an LSN. You are right, such a case is equivalent to pg_sleep.
One way to avoid that is to prohibit waiting for TIMEOUT without
specifying an LSN. Do you think we should do that?

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

Hi!

On Fri, Apr 3, 2020 at 9:51 PM Anna Akenteva <a(dot)akenteva(at)postgrespro(dot)ru> wrote:
> In the patch that I reviewed, you could do things like:
> WAIT FOR
> LSN lsn0,
> LSN lsn1 TIMEOUT time1,
> LSN lsn2 TIMEOUT time2;
> and such a statement was in practice equivalent to
> WAIT FOR LSN(max(lsn0, lsn1, lsn2)) TIMEOUT (max(time1, time2))
>
> As you can see, even though grammatically lsn1 is grouped with time1 and
> lsn2 is grouped with time2, both timeouts that we specified are not
> connected to their respective LSN-s, and instead they kinda act like
> global timeouts. Therefore, I didn't see a point in keeping TIMEOUT
> necessarily grammatically connected to LSN.
>
> In the new syntax our statement would look like this:
> WAIT FOR LSN lsn0, LSN lsn1, LSN lsn2, TIMEOUT time1, TIMEOUT time2;
> TIMEOUT-s are not forced to be grouped with LSN-s anymore, which makes
> it more clear that all specified TIMEOUTs will be global and will apply
> to all LSN-s at once.
>
> The point of having TIMEOUT is still to let us limit the time of waiting
> for LSNs. It's just that with the new syntax, we can also use TIMEOUT
> without an LSN. You are right, such a case is equivalent to pg_sleep.
> One way to avoid that is to prohibit waiting for TIMEOUT without
> specifying an LSN. Do you think we should do that?

I think specifying multiple LSNs/TIMEOUTs is kind of ridiculous. We
can assume that client application is smart enough to calculate
minimum/maximum on its side. When multiple LSNs/TIMEOUTs are
specified, what should we wait for? Reaching all the LSNs? Reaching
any of LSNs? Are timeouts independent from LSNs or sticked together?
So if we didn't manage to reach LSN1 in TIMEOUT1, then we don't wait
for LSN2 with TIMEOUT2 (or not)?

I think that now we would be fine with single LSN and single TIMEOUT.
In future we may add multiple LSNs/TIMEOUTs or/and support for
expressions as LSNs/TIMEOUTs if we figure out it's necessary.

I also think it's good to couple waiting for lsn with beginning of
transaction is good idea. Separate WAIT FOR LSN statement called in
the middle of transaction looks problematic for me. Imagine we have RR
isolation and already acquired the snapshot. Then out snapshot can
block applying wal records, which we are waiting for. That would be
implicit deadlock. It would be nice to evade such deadlocks by
design.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2020-04-03 21:51, Anna Akenteva wrote:
> I did some code cleanup and added tests - both for the standalone WAIT
> FOR statement and for WAIT FOR as a part of BEGIN. The new patch is
> attached.

I did more cleanup and code optimization on waiting events on latch.
And rebase patch.

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

On 2020-04-04 03:14, Alexander Korotkov wrote:
> I think that now we would be fine with single LSN and single TIMEOUT.
> In future we may add multiple LSNs/TIMEOUTs or/and support for
> expressions as LSNs/TIMEOUTs if we figure out it's necessary.
>
> I also think it's good to couple waiting for lsn with beginning of
> transaction is good idea. Separate WAIT FOR LSN statement called in
> the middle of transaction looks problematic for me. Imagine we have RR
> isolation and already acquired the snapshot. Then out snapshot can
> block applying wal records, which we are waiting for. That would be
> implicit deadlock. It would be nice to evade such deadlocks by
> design.
Ok, here is a new version of patch with single LSN and TIMEOUT.

Synopsis
==========
BEGIN [ WORK | TRANSACTION ] [ transaction_mode [, ...] ] [WAIT FOR LSN
'lsn' [ TIMEOUT 'value']]
and
START TRANSACTION [ transaction_mode [, ...] ] [WAIT FOR LSN 'lsn' [
TIMEOUT 'value']]
where lsn is result of pg_current_wal_flush_lsn on master.
and value is uint time interval in milliseconds.
Description
==========
BEGIN/START...WAIT FOR - pause the start of transaction until a
specified LSN has
been replayed. (Don’t open transaction if lsn is not reached on
timeout).

How to use it
==========
WAIT FOR LSN ‘LSN’ [, timeout in ms];

# Before starting transaction, wait until LSN 0/84832E8 is replayed.
Wait time is
not limited here because a timeout was not specified
BEGIN WAIT FOR LSN '0/84832E8';

# Before starting transaction, wait until LSN 0/84832E8 is replayed.
Limit the wait
time with 10 seconds, and if LSN is not reached by then, don't start the
transaction.
START TRANSACTION WAIT FOR LSN '0/8DFFB88' TIMEOUT 10000;

# Same as previous, but with transaction isolation level = REPEATABLE
READ
BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ WAIT FOR LSN
'0/815C0F1' TIMEOUT 10000;

Notice: WAIT FOR will release on PostmasterDeath or Interruption events
if they come earlier than LSN or timeout.

Testing the implementation
======================
The implementation was tested with src/test/recovery/t/020_begin_wait.pl

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

On Tue, Apr 7, 2020 at 12:58 AM Kartyshov Ivan
<i(dot)kartyshov(at)postgrespro(dot)ru> wrote:
> On 2020-04-04 03:14, Alexander Korotkov wrote:
> > I think that now we would be fine with single LSN and single TIMEOUT.
> > In future we may add multiple LSNs/TIMEOUTs or/and support for
> > expressions as LSNs/TIMEOUTs if we figure out it's necessary.
> >
> > I also think it's good to couple waiting for lsn with beginning of
> > transaction is good idea. Separate WAIT FOR LSN statement called in
> > the middle of transaction looks problematic for me. Imagine we have RR
> > isolation and already acquired the snapshot. Then out snapshot can
> > block applying wal records, which we are waiting for. That would be
> > implicit deadlock. It would be nice to evade such deadlocks by
> > design.
> Ok, here is a new version of patch with single LSN and TIMEOUT.

I think this quite small feature, which already received quite amount
of review. The last version is very pinched. But I think it would be
good to commit some very basic version, which is at least some
progress in the area and could be extended in future. I'm going to
pass trough the code tomorrow and commit this unless I found major
issues or somebody objects.

------
Alexander Korotkov

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

On 2020-04-07 00:58, Kartyshov Ivan wrote:
> Ok, here is a new version of patch with single LSN and TIMEOUT.

I had a look at the code and did some more code cleanup, with Ivan's
permission.
This is what I did:
- Removed "WAIT FOR" command tag from cmdtaglist.h and renamed WaitStmt
to WaitClause (since there's no standalone WAIT FOR command anymore)
- Added _copyWaitClause() and _equalWaitClause()
- Removed unused #include-s from utility.c
- Adjusted tests and documentation
- Fixed/added some code comments

I have a couple of questions about WaitUtility() though:
- When waiting forever (due to not specifying a timeout), isn't 60
seconds too long of an interval to check for interrupts?
- If we did specify a timeout, it might be a very long one. In this
case, shouldn't we also make sure to wake up sometimes to check for
interrupts?
- Is it OK that specifying timeout = 0 (BEGIN WAIT FOR LSN ... TIMEOUT
0) is the same as not specifying timeout at all?

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

On Tue, Apr 7, 2020 at 7:56 AM Anna Akenteva <a(dot)akenteva(at)postgrespro(dot)ru> wrote:
>
> On 2020-04-07 00:58, Kartyshov Ivan wrote:
> > Ok, here is a new version of patch with single LSN and TIMEOUT.
>
> I had a look at the code and did some more code cleanup, with Ivan's
> permission.
> This is what I did:
> - Removed "WAIT FOR" command tag from cmdtaglist.h and renamed WaitStmt
> to WaitClause (since there's no standalone WAIT FOR command anymore)
> - Added _copyWaitClause() and _equalWaitClause()
> - Removed unused #include-s from utility.c
> - Adjusted tests and documentation
> - Fixed/added some code comments
>
> I have a couple of questions about WaitUtility() though:
> - When waiting forever (due to not specifying a timeout), isn't 60
> seconds too long of an interval to check for interrupts?
> - If we did specify a timeout, it might be a very long one. In this
> case, shouldn't we also make sure to wake up sometimes to check for
> interrupts?
>

Right, we should probably wait for 100ms before checking the
interrupts. See the similar logic in pg_promote where we wait for
specified number of seconds.

> - Is it OK that specifying timeout = 0 (BEGIN WAIT FOR LSN ... TIMEOUT
> 0) is the same as not specifying timeout at all?
>

Yes that sounds reasonable to me.

Review comments:
--------------------------
1.
+/*
+ * Delete wait event of the current backend from the shared memory array.
+ *
+ * TODO: Consider state cleanup on backend failure.
+ * Check:
+ * 1) nomal|smart|fast|immediate stop
+ * 2) SIGKILL and SIGTERM
+ */
+static void
+DeleteEvent(void)

I don't see how this is implemented or called to handle any errors.
For example in function WaitUtility if the WaitLatch errors out due to
any error, then the event won't be deleted. I think we can't assume
WaitLatch or any other code in this code path will never error out.
For ex. WaitLatch---->WaitEventSetWaitBlock() can error out. Also, in
future we can add more code which can error out.

2.
+ /*
+ * If received an interruption from CHECK_FOR_INTERRUPTS,
+ * then delete the current event from array.
+ */
+ if (InterruptPending)
+ {
+ DeleteEvent();
+ ProcessInterrupts();
+ }

We generally do this type of handling via CHECK_FOR_INTERRUPTS. One
reason is that it behaves slightly differently in Windows. I am not
sure why we want to do differently here? This looks quite adhoc to me
and may not be correct. If we handle this event in error path, then
we might not need to do some special handling.

3.
+/*
+ * On WAIT use a latch to wait till LSN is replayed,
+ * postmaster dies or timeout happens.
+ * Returns 1 if LSN was reached and 0 otherwise.
+ */
+int
+WaitUtility(XLogRecPtr target_lsn, const float8 secs)

Isn't it better to have a return value as bool? IOW, why this
function need int as its return value?

4.
+#define GetNowFloat() ((float8) GetCurrentTimestamp() / 1000000.0)

This same define is used elsewhere in the code as well, may be we can
define it in some central place and use it.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Apr 7, 2020 at 5:56 AM Alexander Korotkov
<a(dot)korotkov(at)postgrespro(dot)ru> wrote:
>
> On Tue, Apr 7, 2020 at 12:58 AM Kartyshov Ivan
> <i(dot)kartyshov(at)postgrespro(dot)ru> wrote:
> > On 2020-04-04 03:14, Alexander Korotkov wrote:
> > > I think that now we would be fine with single LSN and single TIMEOUT.
> > > In future we may add multiple LSNs/TIMEOUTs or/and support for
> > > expressions as LSNs/TIMEOUTs if we figure out it's necessary.
> > >
> > > I also think it's good to couple waiting for lsn with beginning of
> > > transaction is good idea. Separate WAIT FOR LSN statement called in
> > > the middle of transaction looks problematic for me. Imagine we have RR
> > > isolation and already acquired the snapshot. Then out snapshot can
> > > block applying wal records, which we are waiting for. That would be
> > > implicit deadlock. It would be nice to evade such deadlocks by
> > > design.
> > Ok, here is a new version of patch with single LSN and TIMEOUT.
>
> I think this quite small feature, which already received quite amount
> of review. The last version is very pinched. But I think it would be
> good to commit some very basic version, which is at least some
> progress in the area and could be extended in future. I'm going to
> pass trough the code tomorrow and commit this unless I found major
> issues or somebody objects.
>

I have gone through this thread and skimmed through the patch and I am
not sure if we can say that this patch is ready to go. First, I don't
think we have a consensus on the syntax being used in the patch
(various people didn't agree to LSN specific syntax). They wanted a
more generic syntax and I see that we tried to implement it and it
turns out to be a bit complex but that doesn't mean we just give up on
the idea and take the simplest approach and that too without a broader
agreement. Second, on my quick review, it seems there are a few
things like error handling, interrupt checking which need more work.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 2020-04-07 13:32, Amit Kapila wrote:
> First, I don't
> think we have a consensus on the syntax being used in the patch
> (various people didn't agree to LSN specific syntax). They wanted a
> more generic syntax and I see that we tried to implement it and it
> turns out to be a bit complex but that doesn't mean we just give up on
> the idea and take the simplest approach and that too without a broader
> agreement.

Thank you for your comments!

Initially, the syntax used to be "WAITLSN", which confined us with only
waiting for LSN-s and not anything else. So we switched to "WAIT FOR
LSN", which would allow us to add variations like "WAIT FOR XID" or
"WAIT FOR COMMIT TOKEN" in the future if we wanted. A few people seemed
to imply that this kind of syntax is expandable enough:

On 2018-02-01 14:47, Simon Riggs wrote:
> I agree that WAIT LSN is good syntax because this allows us to wait
> for something else in future.

On 2017-10-31 12:42:56, Ants Aasma wrote:
> For lack of a better proposal I would like something along the lines
> of:
> WAIT FOR state_id[, state_id] [ OPTIONS (..)]

As for giving up waiting for multiple events: we can only wait for LSN-s
at the moment, and there seems to be no point in waiting for multiple
LSN-s at once, because it's equivalent to waiting for the biggest LSN.
So we opted for simpler grammar for now, only letting the user specify
one LSN and one TIMEOUT. If in the future we allow waiting for something
else, like XID-s, we can expand the grammar as needed.

What are your own thoughts on the syntax?

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

On Tue, Apr 7, 2020 at 3:07 PM Anna Akenteva <a(dot)akenteva(at)postgrespro(dot)ru> wrote:
> On 2017-10-31 12:42:56, Ants Aasma wrote:
> > For lack of a better proposal I would like something along the lines
> > of:
> > WAIT FOR state_id[, state_id] [ OPTIONS (..)]
>
> As for giving up waiting for multiple events: we can only wait for LSN-s
> at the moment, and there seems to be no point in waiting for multiple
> LSN-s at once, because it's equivalent to waiting for the biggest LSN.
> So we opted for simpler grammar for now, only letting the user specify
> one LSN and one TIMEOUT. If in the future we allow waiting for something
> else, like XID-s, we can expand the grammar as needed.

+1
In the latest version of patch we have very brief and simple syntax
allowing to wait for given LSN with given timeout. In future we can
expand this syntax in different ways. I don't see that current syntax
is limiting us from something.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Tue, Apr 7, 2020 at 5:37 PM Anna Akenteva <a(dot)akenteva(at)postgrespro(dot)ru> wrote:
>
> On 2020-04-07 13:32, Amit Kapila wrote:
> > First, I don't
> > think we have a consensus on the syntax being used in the patch
> > (various people didn't agree to LSN specific syntax). They wanted a
> > more generic syntax and I see that we tried to implement it and it
> > turns out to be a bit complex but that doesn't mean we just give up on
> > the idea and take the simplest approach and that too without a broader
> > agreement.
>
> Thank you for your comments!
>
> Initially, the syntax used to be "WAITLSN", which confined us with only
> waiting for LSN-s and not anything else. So we switched to "WAIT FOR
> LSN", which would allow us to add variations like "WAIT FOR XID" or
> "WAIT FOR COMMIT TOKEN" in the future if we wanted. A few people seemed
> to imply that this kind of syntax is expandable enough:
>
> On 2018-02-01 14:47, Simon Riggs wrote:
> > I agree that WAIT LSN is good syntax because this allows us to wait
> > for something else in future.
>
> On 2017-10-31 12:42:56, Ants Aasma wrote:
> > For lack of a better proposal I would like something along the lines
> > of:
> > WAIT FOR state_id[, state_id] [ OPTIONS (..)]
>
> As for giving up waiting for multiple events: we can only wait for LSN-s
> at the moment, and there seems to be no point in waiting for multiple
> LSN-s at once, because it's equivalent to waiting for the biggest LSN.
> So we opted for simpler grammar for now, only letting the user specify
> one LSN and one TIMEOUT. If in the future we allow waiting for something
> else, like XID-s, we can expand the grammar as needed.
>
> What are your own thoughts on the syntax?
>

I don't know how users can specify the LSN value but OTOH I could see
if users can somehow provide the correct value of commit LSN for which
they want to wait, then it could work out. It is possible that I
misread and we have a consensus on WAIT FOR LSN [option] because I saw
what Simon and Ants have proposed includes multiple state/events and
it might be fine to have just one event for now.

Anyone else wants to share an opinion on syntax?

I think even if we are good with syntax, I could see the code is not
completely ready to go as mentioned in few comments raised by me. I
am not sure if we want to commit it in the current form and then
improve after feature freeze. If it is possible to fix the loose ends
quickly and there are no more comments by anyone then probably we
might be able to commit it.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 2020-04-07 12:58, Amit Kapila wrote:
>
> Review comments:
> 1.
> +static void
> +DeleteEvent(void)
> I don't see how this is implemented or called to handle any errors.
>
> 2.
> + if (InterruptPending)
> + {
> + DeleteEvent();
> + ProcessInterrupts();
> + }
> We generally do this type of handling via CHECK_FOR_INTERRUPTS.
>
> 3.
> +int
> +WaitUtility(XLogRecPtr target_lsn, const float8 secs)
> Isn't it better to have a return value as bool?
>
> 4.
> +#define GetNowFloat() ((float8) GetCurrentTimestamp() / 1000000.0)
> This same define is used elsewhere in the code as well, may be we can
> define it in some central place and use it.

Thank you for your review!
Ivan and I have worked on the patch and tried to address your comments:

0. Now we wake up at least every 100ms to check for interrupts.
1. Now we call DeleteWaitedLSN() from
ProcessInterrupts()=>LockErrorCleanup(). It seems that we can only exit
the WAIT cycle improperly due to interrupts, so this should be enough
(?)
2. Now we use CHECK_FOR_INTERRUPTS() instead of ProcessInterrupts()
3. Now WaitUtility() returns bool rather than int
4. Now GetNowFloat() is only defined at one place in the code

What we changed additionally:
- Prohibited using WAIT FOR LSN on master
- Added more tests
- Checked the code with pgindent and adjusted pgindent/typedefs.list
- Changed min_lsn's type to pg_atomic_uint64 + fixed how we work with
mutex
- Code cleanup in wait.[c|h]: cleaned up #include-s, gave better names
to functions, changed elog() to ereport()

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

On Tue, Apr 7, 2020 at 10:58 PM Anna Akenteva <a(dot)akenteva(at)postgrespro(dot)ru> wrote:
> Thank you for your review!
> Ivan and I have worked on the patch and tried to address your comments:

I've pushed this. I promise to do careful post-commit review and
resolve any issues arising.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 2020-04-08 00:27, Tom Lane wrote:
> Alexander Korotkov <akorotkov(at)postgresql(dot)org> writes:
» WAIT FOR LSN lsn [ TIMEOUT timeout ]
>
> This seems like a really carelessly chosen syntax —- *three* new
> keywords, when you probably didn't need any. Are you not aware that
> there is distributed overhead in the grammar for every keyword?
> Plus, each new keyword carries the risk of breaking existing
> applications, since it no longer works as an alias-not-preceded-by-AS.
>

To avoid creating new keywords, we could change syntax in the following
way:
WAIT FOR => DEPENDS ON
LSN => EVENT
TIMEOUT => WITH INTERVAL

So
START TRANSACTION WAIT FOR LSN '0/3F07A6B1' TIMEOUT 5000;
would instead look as
START TRANSACTION DEPENDS ON EVENT '0/3F07A6B1' WITH INTERVAL '5
seconds';

[1]
/message-id/28209.1586294824%40sss.pgh.pa.us

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

On Wed, Apr 8, 2020 at 2:14 AM Kartyshov Ivan
<i(dot)kartyshov(at)postgrespro(dot)ru> wrote:
> On 2020-04-08 00:27, Tom Lane wrote:
> > Alexander Korotkov <akorotkov(at)postgresql(dot)org> writes:
> » WAIT FOR LSN lsn [ TIMEOUT timeout ]
> >
> > This seems like a really carelessly chosen syntax —- *three* new
> > keywords, when you probably didn't need any. Are you not aware that
> > there is distributed overhead in the grammar for every keyword?
> > Plus, each new keyword carries the risk of breaking existing
> > applications, since it no longer works as an alias-not-preceded-by-AS.
> >
>
> To avoid creating new keywords, we could change syntax in the following
> way:
> WAIT FOR => DEPENDS ON

Looks OK for me.

> LSN => EVENT

I think it's too generic. Not every event is lsn. TBH, lsn is not
event at all :)

I wonder is we can still use word lsn, but don't use keyword for that.
Can we take arbitrary non-quoted literal there and check it later?

> TIMEOUT => WITH INTERVAL

I'm not yet sure about this. Probably there are better options.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

At Wed, 8 Apr 2020 02:52:55 +0300, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> wrote in
> On Wed, Apr 8, 2020 at 2:14 AM Kartyshov Ivan
> <i(dot)kartyshov(at)postgrespro(dot)ru> wrote:
> > On 2020-04-08 00:27, Tom Lane wrote:
> > > Alexander Korotkov <akorotkov(at)postgresql(dot)org> writes:
> > » WAIT FOR LSN lsn [ TIMEOUT timeout ]
> > >
> > > This seems like a really carelessly chosen syntax —- *three* new
> > > keywords, when you probably didn't need any. Are you not aware that
> > > there is distributed overhead in the grammar for every keyword?
> > > Plus, each new keyword carries the risk of breaking existing
> > > applications, since it no longer works as an alias-not-preceded-by-AS.
> > >
> >
> > To avoid creating new keywords, we could change syntax in the following
> > way:
> > WAIT FOR => DEPENDS ON
>
> Looks OK for me.
>
> > LSN => EVENT
>
> I think it's too generic. Not every event is lsn. TBH, lsn is not
> event at all :)
>
> I wonder is we can still use word lsn, but don't use keyword for that.
> Can we take arbitrary non-quoted literal there and check it later?
>
> > TIMEOUT => WITH INTERVAL
>
> I'm not yet sure about this. Probably there are better options.

How about something like the follows.

BEGIN AFTER ColId Sconst
BEGIN FOLOWING ColId Sconst

UNTIL <absolute time>;
LIMIT BY <interval>;
WITHIN Iconst;

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 2020-04-08 04:09, Kyotaro Horiguchi wrote:
> How about something like the follows.
>
> BEGIN AFTER ColId Sconst
> BEGIN FOLOWING ColId Sconst
>
> UNTIL <absolute time>;
> LIMIT BY <interval>;
> WITHIN Iconst;
>
> regards.

I like your suggested keywords! I think that "AFTER" + "WITHIN" sound
the most natural. We could completely give up the LSN keyword for now.
The final command could look something like:

BEGIN AFTER ‘0/303EC60’ WITHIN '5 seconds';
or
BEGIN AFTER ‘0/303EC60’ WITHIN 5000;

I'd like to hear others' opinions on the syntax as well.

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

Anna Akenteva <a(dot)akenteva(at)postgrespro(dot)ru> writes:
> I'd like to hear others' opinions on the syntax as well.

Pardon me for coming very late to the party, but it seems like there are
other questions that ought to be answered before we worry about any of
this. Why is this getting grafted onto BEGIN/START TRANSACTION in the
first place? It seems like it would be just as useful as a separate
command, if not more so. You could always start a transaction just
after waiting. Conversely, there might be reasons to want to wait
within an already-started transaction.

If it could survive as a separate command, then I'd humbly suggest
that it requires no grammar work at all. You could just invent one
or more functions that take suitable parameters.

regards, tom lane

At Wed, 08 Apr 2020 16:35:46 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in
> Anna Akenteva <a(dot)akenteva(at)postgrespro(dot)ru> writes:
> > I'd like to hear others' opinions on the syntax as well.
>
> Pardon me for coming very late to the party, but it seems like there are
> other questions that ought to be answered before we worry about any of
> this. Why is this getting grafted onto BEGIN/START TRANSACTION in the
> first place? It seems like it would be just as useful as a separate
> command, if not more so. You could always start a transaction just
> after waiting. Conversely, there might be reasons to want to wait
> within an already-started transaction.
>
> If it could survive as a separate command, then I'd humbly suggest
> that it requires no grammar work at all. You could just invent one
> or more functions that take suitable parameters.

The rationale for not being a fmgr function is stated in the following
comments.

/message-id/CAEepm%3D0V74EApmfv%3DMGZa24Ac_pV1vGrp3Ovnv-3rUXwxu9epg%40mail.gmail.com
| because it doesn't work for our 2 higher isolation levels as
| mentioned."

/message-id/CA%2BTgmob-aG3Lqh6OpvMDYTNR5eyq94VugyEejyk7pLhE9uwnyA%40mail.gmail.com

| IMHO, trying to do this using a function-based interface is a really
| bad idea for exactly the reasons you mention. I don't see why we'd
| resist the idea of core syntax here; transactions are a core part of
| PostgreSQL.

It seemed to me that they were suggested it to be in a part of BEGIN
command, but the next proposed patch implemented "WAIT FOR" command
for uncertain reasons to me. I don't object to the isolate command if
it is useful than a part of BEGIN command.

By the way, for example, pg_current_wal_lsn() is a volatile function
and repeated calls within a SERIALIZABLE transaction can return
different values.

If there's no necessity for this feature to be a core command, I think
I would like to be it a function.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On 2020/04/09 16:11, Kyotaro Horiguchi wrote:
> At Wed, 08 Apr 2020 16:35:46 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in
>> Anna Akenteva <a(dot)akenteva(at)postgrespro(dot)ru> writes:
>>> I'd like to hear others' opinions on the syntax as well.
>>
>> Pardon me for coming very late to the party, but it seems like there are
>> other questions that ought to be answered before we worry about any of
>> this. Why is this getting grafted onto BEGIN/START TRANSACTION in the
>> first place? It seems like it would be just as useful as a separate
>> command, if not more so. You could always start a transaction just
>> after waiting. Conversely, there might be reasons to want to wait
>> within an already-started transaction.
>>
>> If it could survive as a separate command, then I'd humbly suggest
>> that it requires no grammar work at all. You could just invent one
>> or more functions that take suitable parameters.
>
> The rationale for not being a fmgr function is stated in the following
> comments.

This issue happens because the function is executed after BEGIN? If yes,
what about executing the function (i.e., as separate transaction) before BEGIN?
If so, the snapshot taken in the function doesn't affect the subsequent
transaction whatever its isolation level is.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> writes:
> On 2020/04/09 16:11, Kyotaro Horiguchi wrote:
>> At Wed, 08 Apr 2020 16:35:46 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in
>>> Why is this getting grafted onto BEGIN/START TRANSACTION in the
>>> first place?

>> The rationale for not being a fmgr function is stated in the following
>> comments. [...]

> This issue happens because the function is executed after BEGIN? If yes,
> what about executing the function (i.e., as separate transaction) before BEGIN?
> If so, the snapshot taken in the function doesn't affect the subsequent
> transaction whatever its isolation level is.

I wonder whether making it a procedure, rather than a plain function,
would help any.

regards, tom lane

On 2020-04-09 16:33, Tom Lane wrote:
> Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> writes:
>> On 2020/04/09 16:11, Kyotaro Horiguchi wrote:
>>> At Wed, 08 Apr 2020 16:35:46 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
>>> wrote in
>>>> Why is this getting grafted onto BEGIN/START TRANSACTION in the
>>>> first place?
>
>>> The rationale for not being a fmgr function is stated in the
>>> following
>>> comments. [...]
>
>> This issue happens because the function is executed after BEGIN? If
>> yes,
>> what about executing the function (i.e., as separate transaction)
>> before BEGIN?
>> If so, the snapshot taken in the function doesn't affect the
>> subsequent
>> transaction whatever its isolation level is.
>
> I wonder whether making it a procedure, rather than a plain function,
> would help any.
>

Just another idea in case if one will still decide to go with a separate
statement + BEGIN integration instead of a function. We could use
parenthesized options list here. This is already implemented for VACUUM,
REINDEX, etc. There was an idea to allow CONCURRENTLY in REINDEX there
[1] and recently this was proposed again for new options [2], since it
is much more extensible from the grammar perspective.

That way, the whole feature may look like:

WAIT (LSN '16/B374D848', TIMEOUT 100);

and/or

BEGIN
WAIT (LSN '16/B374D848', WHATEVER_OPTION_YOU_WANT);
...
COMMIT;

It requires only one reserved keyword 'WAIT'. The advantage of this
approach is that it can be extended to support xid, timestamp, csn or
anything else, that may be invented in the future, without affecting the
grammar.

What do you think?

Personally, I find this syntax to be more convenient and human-readable
compared with function call:

SELECT pg_wait_for_lsn('16/B374D848');
BEGIN;

[1]
/message-id/aad2ec49-5142-7356-ffb2-a9b2649cdd1f%402ndquadrant.com

[2]
/message-id/20200401060334.GB142683%40paquier.xyz

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

On 2020/04/10 3:16, Alexey Kondratov wrote:
> On 2020-04-09 16:33, Tom Lane wrote:
>> Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> writes:
>>> On 2020/04/09 16:11, Kyotaro Horiguchi wrote:
>>>> At Wed, 08 Apr 2020 16:35:46 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in
>>>>> Why is this getting grafted onto BEGIN/START TRANSACTION in the
>>>>> first place?
>>
>>>> The rationale for not being a fmgr function is stated in the following
>>>> comments. [...]
>>
>>> This issue happens because the function is executed after BEGIN? If yes,
>>> what about executing the function (i.e., as separate transaction) before BEGIN?
>>> If so, the snapshot taken in the function doesn't affect the subsequent
>>> transaction whatever its isolation level is.
>>
>> I wonder whether making it a procedure, rather than a plain function,
>> would help any.
>>
>
> Just another idea in case if one will still decide to go with a separate statement + BEGIN integration instead of a function. We could use parenthesized options list here. This is already implemented for VACUUM, REINDEX, etc. There was an idea to allow CONCURRENTLY in REINDEX there [1] and recently this was proposed again for new options [2], since it is much more extensible from the grammar perspective.
>
> That way, the whole feature may look like:
>
> WAIT (LSN '16/B374D848', TIMEOUT 100);
>
> and/or
>
> BEGIN
> WAIT (LSN '16/B374D848', WHATEVER_OPTION_YOU_WANT);
> ...
> COMMIT;
>
> It requires only one reserved keyword 'WAIT'. The advantage of this approach is that it can be extended to support xid, timestamp, csn or anything else, that may be invented in the future, without affecting the grammar.
>
> What do you think?
>
> Personally, I find this syntax to be more convenient and human-readable compared with function call:
>
> SELECT pg_wait_for_lsn('16/B374D848');
> BEGIN;

I can imagine that some users want to specify the LSN to wait for,
from the result of another query, for example,
SELECT pg_wait_for_lsn(lsn) FROM xxx. If this is valid use case,
isn't the function better?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2020-04-10 05:25, Fujii Masao wrote:
> On 2020/04/10 3:16, Alexey Kondratov wrote:
>> Just another idea in case if one will still decide to go with a
>> separate statement + BEGIN integration instead of a function. We could
>> use parenthesized options list here. This is already implemented for
>> VACUUM, REINDEX, etc. There was an idea to allow CONCURRENTLY in
>> REINDEX there [1] and recently this was proposed again for new options
>> [2], since it is much more extensible from the grammar perspective.
>>
>> That way, the whole feature may look like:
>>
>> WAIT (LSN '16/B374D848', TIMEOUT 100);
>>
>> and/or
>>
>> BEGIN
>> WAIT (LSN '16/B374D848', WHATEVER_OPTION_YOU_WANT);
>> ...
>> COMMIT;
>>
>> It requires only one reserved keyword 'WAIT'. The advantage of this
>> approach is that it can be extended to support xid, timestamp, csn or
>> anything else, that may be invented in the future, without affecting
>> the grammar.
>>
>> What do you think?
>>
>> Personally, I find this syntax to be more convenient and
>> human-readable compared with function call:
>>
>> SELECT pg_wait_for_lsn('16/B374D848');
>> BEGIN;
>
> I can imagine that some users want to specify the LSN to wait for,
> from the result of another query, for example,
> SELECT pg_wait_for_lsn(lsn) FROM xxx. If this is valid use case,
> isn't the function better?
>

I think that the main purpose of the feature is to achieve
read-your-writes-consistency, while using async replica for reads. In
that case lsn of last modification is stored inside application, so
there is no need to do any query for that. Moreover, you cannot store
this lsn inside database, since reads are distributed across all
replicas (+ primary).

Thus, I could imagine that 'xxx' in your example states for some kind of
stored procedure, that fetches lsn from the off-postgres storage, but it
looks like very narrow case to count on it, doesn't it?

Anyway, I am not against implementing this as a function. That was just
another option to consider.

Just realized that the last patch I have seen does not allow usage of
wait on primary. It may be a problem if reads are pooled not only across
replicas, but on primary as well, which should be quite usual I guess.
In that case application does not know either request will be processed
on replica, or on primary. I think it should be allowed without any
warning, or just saying some LOG/DEBUG at most, that there was no
waiting performed.

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

Hi,

On 2020-04-10 11:25:02 +0900, Fujii Masao wrote:
> > BEGIN
> > WAIT (LSN '16/B374D848', WHATEVER_OPTION_YOU_WANT);
> > ...
> > COMMIT;
> >
> > It requires only one reserved keyword 'WAIT'. The advantage of this approach is that it can be extended to support xid, timestamp, csn or anything else, that may be invented in the future, without affecting the grammar.
> >
> > What do you think?
> >
> > Personally, I find this syntax to be more convenient and human-readable compared with function call:
> >
> > SELECT pg_wait_for_lsn('16/B374D848');
> > BEGIN;
>
> I can imagine that some users want to specify the LSN to wait for,
> from the result of another query, for example,
> SELECT pg_wait_for_lsn(lsn) FROM xxx. If this is valid use case,
> isn't the function better?

I don't think a function is a good idea - it'll cause a snapshot to be
held while waiting. Which in turn will cause hot_standby_feedback to not
be able to report an increased xmin up. And it will possibly hit
snapshot recovery conflicts.

Whereas explicit syntax, especially if a transaction control statement,
won't have that problem.

I'd personally look at 'AFTER' instead of 'WAIT'. Upthread you talked
about a reserved keyword - why does it have to be reserved?

FWIW, I'm not really convinced there needs to be bespoke timeout syntax
for this feature. I can see reasons why you'd not just want to rely on
statement_timeout, but at least that should be discussed.

Greetings,

Andres Freund

Andres Freund <andres(at)anarazel(dot)de> writes:
> I don't think a function is a good idea - it'll cause a snapshot to be
> held while waiting. Which in turn will cause hot_standby_feedback to not
> be able to report an increased xmin up. And it will possibly hit
> snapshot recovery conflicts.

Good point, but we could address that by making it a procedure no?

regards, tom lane

Hi,

On 2020-04-10 16:29:39 -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > I don't think a function is a good idea - it'll cause a snapshot to be
> > held while waiting. Which in turn will cause hot_standby_feedback to not
> > be able to report an increased xmin up. And it will possibly hit
> > snapshot recovery conflicts.
>
> Good point, but we could address that by making it a procedure no?

Probably. Don't think we have great infrastructure for builtin
procedures yet though? We'd presumably not want to use plpgsql.

ISTM that we can make it BEGIN AFTER 'xx/xx' or such, which'd not
require any keywords, it'd be easier to use than a procedure.

With a separate procedure, you'd likely need more roundtrips / complex
logic at the client. You either need to check first if the procedure
errored ou, and then send the BEGIN, or send both together and separate
out potential errors.

Greetings,

Andres Freund

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2020-04-10 16:29:39 -0400, Tom Lane wrote:
>> Good point, but we could address that by making it a procedure no?

> Probably. Don't think we have great infrastructure for builtin
> procedures yet though? We'd presumably not want to use plpgsql.

Don't think anyone's tried yet. It's not instantly clear that the
amount of code needed would be more than comes along with new
syntax, though.

> ISTM that we can make it BEGIN AFTER 'xx/xx' or such, which'd not
> require any keywords, it'd be easier to use than a procedure.

I still don't see a good argument for tying this to BEGIN. If it
has to be a statement, why not a standalone statement?

(I also have a lurking suspicion that this shouldn't be SQL at all
but part of the replication command set.)

regards, tom lane

Hi,

On 2020-04-10 17:17:10 -0400, Tom Lane wrote:
> > ISTM that we can make it BEGIN AFTER 'xx/xx' or such, which'd not
> > require any keywords, it'd be easier to use than a procedure.
>
> I still don't see a good argument for tying this to BEGIN. If it
> has to be a statement, why not a standalone statement?

Because the goal is to start a transaction where a certain action from
the primary is visible.

I think there's also some advantages of having it in a single statement
for poolers. If a pooler analyzes BEGIN AFTER 'xx/xx' it could
e.g. redirect the transaction to a node that's caught up far enough,
instead of blocking. But that can't work even close to as easily if it's
something that has to be executed before transaction begin.

> (I also have a lurking suspicion that this shouldn't be SQL at all
> but part of the replication command set.)

Hm? I'm not quite following. The feature is useful to achieve
read-your-own-writes consistency. Consider

Primary: INSERT INTO app_users ...; SELECT pg_current_wal_lsn();
Standby: BEGIN AFTER 'returned/lsn';
Standby: SELECT i_am_a_set_of_very_expensive_queries FROM ..., app_users;

without the AFTER/WAIT whatnot, you cannot rely on the insert having
been replicated to the standby.

Offloading queries from the write node to replicas is a pretty standard
technique for scaling out databases (including PG). We just make it
harder than necessary.

How would this be part of the replication command set? This shouldn't
require replication permissions for the user executing the queries.
While I'm in favor of merging the replication protocol entirely with the
normal protocol, I've so far received very little support for that
proposition...

Greetings,

Andres Freund

On 2020-04-11 00:44, Andres Freund wrote:
> I think there's also some advantages of having it in a single statement
> for poolers. If a pooler analyzes BEGIN AFTER 'xx/xx' it could
> e.g. redirect the transaction to a node that's caught up far enough,
> instead of blocking. But that can't work even close to as easily if
> it's
> something that has to be executed before transaction begin.
>

I think that's a good point.

Also, I'm not sure how we'd expect a wait-for-LSN procedure to work
inside a single-snapshot transaction. Would it throw an error inside a
RR transaction block? Would it give a warning?

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

This patch require some rewording of documentation/comments and variable names
after the language change introduced by 229f8c219f8f..a9a4a7ad565b, the thread
below can be used as reference for how to change:

/message-id/flat/20200615182235.x7lch5n6kcjq4aue%40alap3.anarazel.de

cheers ./daniel

On 2020-07-13 14:21, Daniel Gustafsson wrote:
> This patch require some rewording of documentation/comments and
> variable names
> after the language change introduced by 229f8c219f8f..a9a4a7ad565b, the
> thread
> below can be used as reference for how to change:
>
> /message-id/flat/20200615182235.x7lch5n6kcjq4aue%40alap3.anarazel.de
>

Thank you for the heads up!

I updated the most recent patch and removed the use of "master" from it,
replacing it with "primary".

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

On Tue, Aug 18, 2020 at 01:12:51PM +0300, Anna Akenteva wrote:
> I updated the most recent patch and removed the use of "master" from it,
> replacing it with "primary".

This is failing to apply lately, causing the CF bot to complain:
http://cfbot.cputube.org/patch_29_772.log
--
Michael

Anna Akenteva писал 2020-04-08 22:36:
> On 2020-04-08 04:09, Kyotaro Horiguchi wrote:
>
> I like your suggested keywords! I think that "AFTER" + "WITHIN" sound
> the most natural. We could completely give up the LSN keyword for now.
> The final command could look something like:
>
> BEGIN AFTER ‘0/303EC60’ WITHIN '5 seconds';
> or
> BEGIN AFTER ‘0/303EC60’ WITHIN 5000;

Hello,

I've changed the syntax of the command from BEGIN [ WAIT FOR LSN value [
TIMEOUT delay ]] to BEGIN [ AFTER value [ WITHIN delay ]] and removed
all the unnecessary keywords.

Best regards,
Alexandra Pervushina.

Hello,

I've changed the BEGIN WAIT FOR LSN statement to core functions
pg_waitlsn, pg_waitlsn_infinite and pg_waitlsn_no_wait.
Currently the functions work inside repeatable read transactions, but
waitlsn creates a snapshot if called first in a transaction block, which
can possibly lead the transaction to working incorrectly, so the
function gives a warning.

Usage examples
==========
select pg_waitlsn(‘LSN’, timeout);
select pg_waitlsn_infinite(‘LSN’);
select pg_waitlsn_no_wait(‘LSN’);

Hello.

At Wed, 18 Nov 2020 15:05:00 +0300, a(dot)pervushina(at)postgrespro(dot)ru wrote in
> I've changed the BEGIN WAIT FOR LSN statement to core functions
> pg_waitlsn, pg_waitlsn_infinite and pg_waitlsn_no_wait.
> Currently the functions work inside repeatable read transactions, but
> waitlsn creates a snapshot if called first in a transaction block,
> which can possibly lead the transaction to working incorrectly, so the
> function gives a warning.

According to the discuttion here, implementing as functions is not
optimal. As a Poc, I made it as a procedure. However I'm not sure it
is the correct implement as a native procedure but it seems working as
expected.

> Usage examples
> ==========
> select pg_waitlsn(‘LSN’, timeout);
> select pg_waitlsn_infinite(‘LSN’);
> select pg_waitlsn_no_wait(‘LSN’);

The first and second usage is coverd by a single procedure. The last
function is equivalent to pg_last_wal_replay_lsn(). As the result, the
following procedure is provided in the attached.

pg_waitlsn(wait_lsn pg_lsn, timeout integer DEFAULT -1)

Any opinions mainly compared to implementation as a command?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Jan 21, 2021 at 1:30 PM Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
wrote:

> Hello.
>
> At Wed, 18 Nov 2020 15:05:00 +0300, a(dot)pervushina(at)postgrespro(dot)ru wrote in
> > I've changed the BEGIN WAIT FOR LSN statement to core functions
> > pg_waitlsn, pg_waitlsn_infinite and pg_waitlsn_no_wait.
> > Currently the functions work inside repeatable read transactions, but
> > waitlsn creates a snapshot if called first in a transaction block,
> > which can possibly lead the transaction to working incorrectly, so the
> > function gives a warning.
>
> According to the discuttion here, implementing as functions is not
> optimal. As a Poc, I made it as a procedure. However I'm not sure it
> is the correct implement as a native procedure but it seems working as
> expected.
>
> > Usage examples
> > ==========
> > select pg_waitlsn(‘LSN’, timeout);
> > select pg_waitlsn_infinite(‘LSN’);
> > select pg_waitlsn_no_wait(‘LSN’);
>
> The first and second usage is coverd by a single procedure. The last
> function is equivalent to pg_last_wal_replay_lsn(). As the result, the
> following procedure is provided in the attached.
>
> pg_waitlsn(wait_lsn pg_lsn, timeout integer DEFAULT -1)
>
> Any opinions mainly compared to implementation as a command?
>
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>

The patch (pg_waitlsn_v10_2_kh.patch) does not compile successfully and has
compilation errors. Can you please take a look?

https://cirrus-ci.com/task/6241565996744704

xlog.c:45:10: fatal error: commands/wait.h: No such file or directory
#include "commands/wait.h"
^~~~~~~~~~~~~~~~~
compilation terminated.
make[4]: *** [<builtin>: xlog.o] Error 1
make[4]: *** Waiting for unfinished jobs....
make[3]: *** [../../../src/backend/common.mk:39: transam-recursive] Error 2
make[2]: *** [common.mk:39: access-recursive] Error 2
make[1]: *** [Makefile:42: all-backend-recurse] Error 2
make: *** [GNUmakefile:11: all-src-recurse] Error 2

I am changing the status to "Waiting on Author"

--
Ibrar Ahmed

At Thu, 18 Mar 2021 18:57:15 +0500, Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com> wrote in
> On Thu, Jan 21, 2021 at 1:30 PM Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>
> wrote:
>
> > Hello.
> >
> > At Wed, 18 Nov 2020 15:05:00 +0300, a(dot)pervushina(at)postgrespro(dot)ru wrote in
> > > I've changed the BEGIN WAIT FOR LSN statement to core functions
> > > pg_waitlsn, pg_waitlsn_infinite and pg_waitlsn_no_wait.
> > > Currently the functions work inside repeatable read transactions, but
> > > waitlsn creates a snapshot if called first in a transaction block,
> > > which can possibly lead the transaction to working incorrectly, so the
> > > function gives a warning.
> >
> > According to the discuttion here, implementing as functions is not
> > optimal. As a Poc, I made it as a procedure. However I'm not sure it
> > is the correct implement as a native procedure but it seems working as
> > expected.
> >
> > > Usage examples
> > > ==========
> > > select pg_waitlsn(‘LSN’, timeout);
> > > select pg_waitlsn_infinite(‘LSN’);
> > > select pg_waitlsn_no_wait(‘LSN’);
> >
> > The first and second usage is coverd by a single procedure. The last
> > function is equivalent to pg_last_wal_replay_lsn(). As the result, the
> > following procedure is provided in the attached.
> >
> > pg_waitlsn(wait_lsn pg_lsn, timeout integer DEFAULT -1)
> >
> > Any opinions mainly compared to implementation as a command?
> >
> > regards.
> >
> > --
> > Kyotaro Horiguchi
> > NTT Open Source Software Center
> >
>
> The patch (pg_waitlsn_v10_2_kh.patch) does not compile successfully and has
> compilation errors. Can you please take a look?
>
> https://cirrus-ci.com/task/6241565996744704
>
> xlog.c:45:10: fatal error: commands/wait.h: No such file or directory
> #include "commands/wait.h"
> ^~~~~~~~~~~~~~~~~
> compilation terminated.
> make[4]: *** [<builtin>: xlog.o] Error 1
> make[4]: *** Waiting for unfinished jobs....
> make[3]: *** [../../../src/backend/common.mk:39: transam-recursive] Error 2
> make[2]: *** [common.mk:39: access-recursive] Error 2
> make[1]: *** [Makefile:42: all-backend-recurse] Error 2
> make: *** [GNUmakefile:11: all-src-recurse] Error 2
>
> I am changing the status to "Waiting on Author"

Anna is the autor. The "patch" was just to show how we can implement
the feature as a procedure. (Sorry for the bad mistake I made.)

The patch still applies to the master. So I resend just rebased
version as v10_2, and attached the "PoC" as *.txt which applies on top
of the patch.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center