[YSQL][Wait-on-conflict] : Parallel inserts with same primary key don't throw error

[Karvy-yb]

Jira Link: DB-8199

Description

Steps to repro:

1. Create universe with read committed, deadlock detection and wait on conflict enabled on 2.21.0.0-b12 DB version
2. create table tb (k int primary key, v int);
3. insert into tb values (1,1), (2,2);
4. Session-1: begin; select; insert into tb values (3,3);
5. Session-2: insert into tb values (3,9); -- This will wait for txn in session-1
6. Session-1: commit;

Notice that both the inserts will go through without throwing any error and table will have (3,9) row.
I don't see this happening with wait-queues disabled and read committed enabled.
This issue is also observed for RR isolation level.

Issue Type

kind/bug

Warning: Please confirm that this issue does not contain any sensitive information

• I confirm this issue does not contain any sensitive information.

[rthallamko3]

@Karvy-yb , Can you confirm if the isolation level is set to read_committed on session 2? Or is the isolation level set at the universe level? cc @basavaraj29

[Karvy-yb]

@rthallamko3 the isolation level is set as read committed at universe level (by setting yb_enable_read_committed_isolation to true) and session 2 uses the default. But note that the behaviour is same when yb_enable_read_committed_isolation is set to false (in which case repeatable read becomes the default isolation level).

[pkj415]

Root cause:

This is happening because, for the single shard INSERT in session 2, we have this order for the following steps:
(1) pick a read time on PgClientSession
(2) do conflict resolution in intents db -> wait in wait queue -> wake up and repeat conflict resolution in intents db
(3) check for a duplicate row among committed data in regular db as of the read time picked in step (1) (see ApplyInsert() in pgsql_operation.cc)

Because the read time was picked on PgClientSession before conflict resolution, the INSERT ends up not seeing the duplicate row.

While we discuss the fix, another thing to note: single shard UPDATEs and DELETEs do not pick a read time on PgClientSession. Instead they pick a read time on the tserver. This happens because a single shard INSERT ends up buffering the operation and then flushing in pg_session.cc as compared to UPDATE and DELETE which directly flush without buffering. Because of this, EnsureReadTimeIsSet is set as true for a single shard INSERT.

Perhaps we should do the same (i.e., pick a read time on docdb) for a single shard INSERT because picking the read time on docdb has some advantages: i) no need to wait for safe time to catch up with a read time picked on PgClientSession ii) kReadRestart errors can be handled on docdb itself without going back to the query layer for retries.

Moreover, for single shard operations, there might be other correctness issue if we pick a read time before conflict resolution but don't use it in the conflict resolution itself.