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

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