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_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);

+   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

Reply via email to