[YSQL][#1199][#2496][#2842] FOR [NO KEY] UPDATE

Summary:
Enable `FOR UPDATE` and `FOR NO KEY UPDATE` row locking statements with
the caveat that `FOR NO KEY UPDATE` behaves like `FOR UPDATE` (see
follow-up issue #2922).  Also, fix conflict detection such that `FOR
SHARE` locks do not conflict with each other.

Implement the fix as follows:

1. Pass down `FOR UPDATE` and `FOR NO KEY UPDATE` row locks through
   Protobufs.

   1. Pass them through `PgsqlReadRequestPB.row_mark_type`.  (Also,
      change the field from `repeated` to `optional`.)
   1. Pass them through `KeyValueWriteBatchPB.row_mark_type`.  (Also,
      change the field from `repeated` to `optional`.)

1. Add a row lock parameter to `docdb::GetStrongIntentTypeSet`, and
   permeate row lock information throughout the affected areas (fix
   issue #2842).  Remove the isolation level hack in
   `tablet::Tablet::PrepareTransactionWriteBatch` in favor of using row
   lock information (fix issue #2496).
1. Create a new value type `kRowLock` to be placed in the value of
   intents for row locking.  Handle this value type in
   `docdb::(anonymous namespace)::DecodeStrongWriteIntent` (for
   in-transaction reads) and `docdb::IntentToWriteRequest` (for
   transaction commits).
1. Create tests, specified in the test plan.

Also, do the following:

* Create new files defining helper functions related to row locks (row
  marks).

  * `src/yb/common/row_mark.cc`
  * `src/yb/common/row_mark.h`

* Prevent `ROW_MARK_REFERENCE` and `ROW_MARK_COPY` from getting passed
  down as the `rowmark` execution parameter.
* Update regress test `yb_pg_privileges` (java test
  `TestPgRegressAuthorization`) to uncomment `FOR UPDATE` row locking
  statements.

Test Plan:
* Jenkins
* `./yb_build.sh`

  * `--cxx-test pgwrapper_pg_mini-test --gtest_filter
    PgMiniTest.RowLockConflictMatrix`
  * `--cxx-test pgwrapper_pg_mini-test --gtest_filter
    PgMiniTest.SerializableDeleteForNoKeyUpdate`
  * `--cxx-test pgwrapper_pg_mini-test --gtest_filter
    PgMiniTest.SerializableDeleteForUpdate`
  * `--cxx-test pgwrapper_pg_mini-test --gtest_filter
    PgMiniTest.SerializableInsertForNoKeyUpdate`
  * `--cxx-test pgwrapper_pg_mini-test --gtest_filter
    PgMiniTest.SerializableInsertForUpdate`
  * `--cxx-test pgwrapper_pg_mini-test --gtest_filter
    PgMiniTest.SnapshotDeleteForNoKeyUpdate`
  * `--cxx-test pgwrapper_pg_mini-test --gtest_filter
    PgMiniTest.SnapshotDeleteForUpdate`
  * `--cxx-test pgwrapper_pg_mini-test --gtest_filter
    PgMiniTest.SnapshotInsertForNoKeyUpdate`
  * `--cxx-test pgwrapper_pg_mini-test --gtest_filter
    PgMiniTest.SnapshotInsertForUpdate`

Reviewers: hector, sergei, mikhail

Reviewed By: mikhail

Subscribers: yql, bogdan

Differential Revision: https://phabricator.dev.yugabyte.com/D7453

src/postgres/src/backend/executor/execMain.c

@@ -959,13 +959,6 @@ InitPlan(QueryDesc *queryDesc, int eflags)
 		{
 			case ROW_MARK_EXCLUSIVE:
 			case ROW_MARK_NOKEYEXCLUSIVE:
-				relation = heap_open(relid, RowShareLock);
-				if (IsYBRelation(relation))
-				{
-					YBRaiseNotSupported("SELECT locking option only supported for temporary tables",
-										1199);
-				}
-				break;
 			case ROW_MARK_SHARE:
 			case ROW_MARK_KEYSHARE:
 				relation = heap_open(relid, RowShareLock);

src/postgres/src/backend/executor/nodeIndexscan.c

@@ -138,7 +138,10 @@ IndexNext(IndexScanState *node)
 		ListCell   *l;
 		foreach(l, estate->es_rowMarks) {
 			ExecRowMark *erm = (ExecRowMark *) lfirst(l);
-			scandesc->yb_exec_params->rowmark = erm->markType;
+			// Do not propogate non-row-locking row marks.
+			if (erm->markType != ROW_MARK_REFERENCE &&
+				erm->markType != ROW_MARK_COPY)
+				scandesc->yb_exec_params->rowmark = erm->markType;
 			break;
 		}
 	}

src/postgres/src/backend/executor/ybc_fdw.c

@@ -275,7 +275,10 @@ ybcBeginForeignScan(ForeignScanState *node, int eflags)
 	ListCell   *l;
 	foreach(l, estate->es_rowMarks) {
 		ExecRowMark *erm = (ExecRowMark *) lfirst(l);
-		ybc_state->exec_params->rowmark = erm->markType;
+		// Do not propogate non-row-locking row marks.
+		if (erm->markType != ROW_MARK_REFERENCE &&
+			erm->markType != ROW_MARK_COPY)
+			ybc_state->exec_params->rowmark = erm->markType;
 		break;
 	}
 

src/postgres/src/test/regress/expected/yb_pg_privileges.out

@@ -94,9 +94,15 @@ INSERT INTO atest1 SELECT 1, b FROM atest1; -- ok
 UPDATE atest1 SET a = 1 WHERE a = 2; -- ok
 UPDATE atest2 SET col2 = NOT col2; -- fail
 ERROR:  permission denied for table atest2
--- TODO(jason): try uncommenting after issue #1199 is resolved.
--- SELECT * FROM atest1 FOR UPDATE; -- ok
--- SELECT * FROM atest2 FOR UPDATE; -- fail
+SELECT * FROM atest1 FOR UPDATE; -- ok
+ a |  b  
+---+-----
+ 1 | two
+ 1 | two
+(2 rows)
+
+SELECT * FROM atest2 FOR UPDATE; -- fail
+ERROR:  permission denied for table atest2
 DELETE FROM atest2; -- fail
 ERROR:  permission denied for table atest2
 TRUNCATE atest2; -- fail
@@ -146,9 +152,10 @@ ERROR:  permission denied for table atest2
 UPDATE atest2 SET col2 = true FROM atest1 WHERE atest1.a = 5; -- ok
 ERROR:  FROM clause in UPDATE not supported yet
 HINT:  See https://github.com/YugaByte/yugabyte-db/issues/738. Click '+' on the description to raise its priority
--- TODO(jason): try uncommenting after issue #1199 is resolved.
--- SELECT * FROM atest1 FOR UPDATE; -- fail
--- SELECT * FROM atest2 FOR UPDATE; -- fail
+SELECT * FROM atest1 FOR UPDATE; -- fail
+ERROR:  permission denied for table atest1
+SELECT * FROM atest2 FOR UPDATE; -- fail
+ERROR:  permission denied for table atest2
 DELETE FROM atest2; -- fail
 ERROR:  permission denied for table atest2
 TRUNCATE atest2; -- fail

src/postgres/src/test/regress/sql/yb_pg_privileges.sql

@@ -76,9 +76,8 @@ INSERT INTO atest2 VALUES ('foo', true); -- fail
 INSERT INTO atest1 SELECT 1, b FROM atest1; -- ok
 UPDATE atest1 SET a = 1 WHERE a = 2; -- ok
 UPDATE atest2 SET col2 = NOT col2; -- fail
--- TODO(jason): try uncommenting after issue #1199 is resolved.
--- SELECT * FROM atest1 FOR UPDATE; -- ok
--- SELECT * FROM atest2 FOR UPDATE; -- fail
+SELECT * FROM atest1 FOR UPDATE; -- ok
+SELECT * FROM atest2 FOR UPDATE; -- fail
 DELETE FROM atest2; -- fail
 TRUNCATE atest2; -- fail
 COPY atest2 FROM stdin; -- fail
@@ -101,9 +100,8 @@ UPDATE atest1 SET a = 1 WHERE a = 2; -- fail
 UPDATE atest2 SET col2 = NULL; -- ok
 UPDATE atest2 SET col2 = NOT col2; -- fails; requires SELECT on atest2
 UPDATE atest2 SET col2 = true FROM atest1 WHERE atest1.a = 5; -- ok
--- TODO(jason): try uncommenting after issue #1199 is resolved.
--- SELECT * FROM atest1 FOR UPDATE; -- fail
--- SELECT * FROM atest2 FOR UPDATE; -- fail
+SELECT * FROM atest1 FOR UPDATE; -- fail
+SELECT * FROM atest2 FOR UPDATE; -- fail
 DELETE FROM atest2; -- fail
 TRUNCATE atest2; -- fail
 COPY atest2 FROM stdin; -- fail

src/yb/common/CMakeLists.txt

@@ -99,6 +99,7 @@ set(COMMON_SRCS
   partial_row.cc
   partition.cc
   row_key-util.cc
+  row_mark.cc
   schema.cc
   index.cc
   jsonb.cc

src/yb/common/common.proto

@@ -368,12 +368,13 @@ enum IsolationLevel {
 }
 
 // This enum matches enum RowMarkType defined in src/include/nodes/plannodes.h.
+// The exception is ROW_MARK_ABSENT, which signifies the absence of a row mark.
 enum RowMarkType
 {
-  // Obtain exclusive tuple lock. Not supported yet.
+  // Obtain exclusive tuple lock.
   ROW_MARK_EXCLUSIVE = 0;
 
-  // Obtain no-key exclusive tuple lock. Not supported yet.
+  // Obtain no-key exclusive tuple lock.
   ROW_MARK_NOKEYEXCLUSIVE = 1;
 
   // Obtain shared tuple lock.
@@ -387,6 +388,10 @@ enum RowMarkType
 
   // Not supported. Used for postgres compatibility.
   ROW_MARK_COPY = 5;
+
+  // Obtain no tuple lock (this should never sent be on the wire).  The value
+  // should be high for convenient comparisons with the other row lock types.
+  ROW_MARK_ABSENT = 15;
 }
 
 enum TransactionStatus {

src/yb/common/pgsql_protocol.proto

@@ -290,13 +290,8 @@ message PgsqlReadRequestPB {
   // The version of the ysql system catalog this query was prepared against.
   optional uint64 ysql_catalog_version = 16;
 
-  // Row mark as used by postgres for row locking. As of 10/12/19 only ROW_MARK_SHARE and
-  // ROW_MARK_KEYSHARE are supported.
-  // TODO https://github.com/yugabyte/yugabyte-db/issues/2495:
-  // As of 10/12/19 whenever a read request has an element in this list, the whole read batch is
-  // assumed to need the same row locks. This is not a problem because we are not batching
-  // read requests, but we will need to change this behavior if we start batching requests.
-  repeated RowMarkType row_mark_type = 23;
+  // Row mark as used by postgres for row locking.
+  optional RowMarkType row_mark_type = 23;
 }
 
 //--------------------------------------------------------------------------------------------------

src/yb/common/row_mark.cc

@@ -0,0 +1,86 @@
+//
+// Copyright (c) YugaByte, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+// in compliance with the License.  You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software distributed under the License
+// is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+// or implied.  See the License for the specific language governing permissions and limitations
+// under the License.
+//
+
+#include "yb/common/row_mark.h"
+
+#include <glog/logging.h>
+
+#include "yb/common/common.pb.h"
+#include "yb/gutil/macros.h"
+
+namespace yb {
+
+bool AreConflictingRowMarkTypes(
+    const RowMarkType row_mark_type_a,
+    const RowMarkType row_mark_type_b) {
+  constexpr int kConflictThreshold = 4;
+  const unsigned int value_a = static_cast<unsigned int>(row_mark_type_a);
+  const unsigned int value_b = static_cast<unsigned int>(row_mark_type_b);
+
+  // TODO: remove this when issue #2922 is fixed.
+  if ((row_mark_type_a == RowMarkType::ROW_MARK_NOKEYEXCLUSIVE &&
+       row_mark_type_b == RowMarkType::ROW_MARK_KEYSHARE) ||
+      (row_mark_type_a == RowMarkType::ROW_MARK_KEYSHARE &&
+       row_mark_type_b == RowMarkType::ROW_MARK_NOKEYEXCLUSIVE)) {
+    return true;
+  }
+
+  return (value_a + value_b < kConflictThreshold);
+}
+
+RowMarkType GetStrongestRowMarkType(std::initializer_list<RowMarkType> row_mark_types) {
+  RowMarkType strongest_row_mark_type = RowMarkType::ROW_MARK_ABSENT;
+  for (RowMarkType row_mark_type : row_mark_types) {
+    strongest_row_mark_type = std::min(row_mark_type, strongest_row_mark_type);
+  }
+  return strongest_row_mark_type;
+}
+
+bool IsValidRowMarkType(RowMarkType row_mark_type) {
+  switch (row_mark_type) {
+    case RowMarkType::ROW_MARK_EXCLUSIVE: FALLTHROUGH_INTENDED;
+    case RowMarkType::ROW_MARK_NOKEYEXCLUSIVE: FALLTHROUGH_INTENDED;
+    case RowMarkType::ROW_MARK_SHARE: FALLTHROUGH_INTENDED;
+    case RowMarkType::ROW_MARK_KEYSHARE:
+      return true;
+      break;
+    default:
+      return false;
+      break;
+  }
+}
+
+std::string RowMarkTypeToPgsqlString(const RowMarkType row_mark_type) {
+  switch (row_mark_type) {
+    case RowMarkType::ROW_MARK_EXCLUSIVE:
+      return "UPDATE";
+      break;
+    case RowMarkType::ROW_MARK_NOKEYEXCLUSIVE:
+      return "NO KEY UPDATE";
+      break;
+    case RowMarkType::ROW_MARK_SHARE:
+      return "SHARE";
+      break;
+    case RowMarkType::ROW_MARK_KEYSHARE:
+      return "KEY SHARE";
+      break;
+    default:
+      // We shouldn't get here because other row lock types are disabled at the postgres level.
+      LOG(DFATAL) << "Unsupported row lock of type " << RowMarkType_Name(row_mark_type);
+      return "";
+      break;
+  }
+}
+
+} // namespace yb

src/yb/common/row_mark.h

@@ -0,0 +1,50 @@
+//
+// Copyright (c) YugaByte, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
+// in compliance with the License.  You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software distributed under the License
+// is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+// or implied.  See the License for the specific language governing permissions and limitations
+// under the License.
+//
+#ifndef YB_COMMON_ROW_MARK_H
+#define YB_COMMON_ROW_MARK_H
+
+#include <glog/logging.h>
+
+#include "yb/common/common.pb.h"
+
+namespace yb {
+
+// Determine whether two row mark types conflict.
+bool AreConflictingRowMarkTypes(RowMarkType row_mark_type_a, RowMarkType row_mark_type_b);
+
+template <typename PB>
+RowMarkType GetRowMarkTypeFromPB(const PB& pb) {
+  if (pb.has_row_mark_type()) {
+    if (IsValidRowMarkType(pb.row_mark_type())) {
+      return pb.row_mark_type();
+    } else {
+      // We shouldn't get here because other row lock types are disabled at the postgres level.
+      LOG(DFATAL) << "Unsupported row lock of type " << RowMarkType_Name(pb.row_mark_type());
+    }
+  }
+  return RowMarkType::ROW_MARK_ABSENT;
+}
+
+// Get the most restrictive row mark type from a list of row mark types.
+RowMarkType GetStrongestRowMarkType(std::initializer_list<RowMarkType> row_mark_types);
+
+// Determine whether a row mark type is valid.
+bool IsValidRowMarkType(RowMarkType row_mark_type);
+
+// Convert a row mark type to a string to use in a PostgreSQL query.
+std::string RowMarkTypeToPgsqlString(const RowMarkType row_mark_type);
+
+} // namespace yb
+
+#endif // YB_COMMON_ROW_MARK_H

src/yb/docdb/conflict_resolution.cc

@@ -15,6 +15,7 @@
 
 #include "yb/common/hybrid_time.h"
 #include "yb/common/pgsql_error.h"
+#include "yb/common/row_mark.h"
 #include "yb/common/transaction.h"
 #include "yb/common/transaction_error.h"
 
@@ -399,9 +400,11 @@ class TransactionConflictResolverContext : public ConflictResolverContext {
     boost::container::small_vector<RefCntPrefix, 8> paths;
 
     KeyBytes encoded_key_buffer;
+    RowMarkType row_mark = GetRowMarkTypeFromPB(write_batch_);
     EnumerateIntentsCallback callback = std::bind(
         &TransactionConflictResolverContext::ProcessIntent, this, resolver,
-        GetStrongIntentTypeSet(metadata_.isolation, docdb::OperationKind::kWrite), _1, _3);
+        GetStrongIntentTypeSet(metadata_.isolation, docdb::OperationKind::kWrite, row_mark), _1,
+        _3);
     for (const auto& doc_op : doc_ops_) {
       paths.clear();
       IsolationLevel ignored_isolation_level;
@@ -429,10 +432,11 @@ class TransactionConflictResolverContext : public ConflictResolverContext {
       return Status::OK();
     }
 
+    RowMarkType row_mark = GetRowMarkTypeFromPB(write_batch_);
     return EnumerateIntents(
         pairs,
         std::bind(&TransactionConflictResolverContext::ProcessIntent, this, resolver,
-                  GetStrongIntentTypeSet(metadata_.isolation, kind), _1, _3),
+                  GetStrongIntentTypeSet(metadata_.isolation, kind, row_mark), _1, _3),
         resolver->partial_range_key_intents());
   }
 
@@ -610,7 +614,8 @@ class OperationConflictResolverContext : public ConflictResolverContext {
       IsolationLevel isolation;
       RETURN_NOT_OK(doc_op->GetDocPaths(GetDocPathsMode::kIntents, &doc_paths, &isolation));
 
-      strong_intent_types = GetStrongIntentTypeSet(isolation, OperationKind::kWrite);
+      strong_intent_types = GetStrongIntentTypeSet(isolation, OperationKind::kWrite,
+                                                   RowMarkType::ROW_MARK_ABSENT);
 
       for (const auto& doc_path : doc_paths) {
         VLOG(4) << "Doc path: " << SubDocKey::DebugSliceToString(doc_path.as_slice());