Merge #143272 #143356

143272: kvclient: serve some DeleteRequest's from write buffer r=yuzefovich a=stevendanna

In order to ensure that a DELETE returns the correct number of deleted rows, it must be integrated with the write buffer so that it observes any previous deletes or writes.

Note that all DELETE statements are not yet handled since we don't yet have DeleteRange support.

Epic: none
Release note: None

143356: sql/schemachanger: support ALTER POLICY DDL r=spilchen a=spilchen

Previously, attempts to execute ALTER POLICY DDL statements failed with an unimplemented error.

This change enables support for ALTER POLICY by leveraging the existing policy-related scpb elements, which already accounted for alteration scenarios.

Closes #136996
Closes #136997

Epic: CRDB-11724
Release note: none

Co-authored-by: Steven Danna <[email protected]>
Co-authored-by: Matt Spilchen <[email protected]>

Author: 3 people

pkg/kv/kvclient/kvcoord/txn_interceptor_write_buffer.go

@@ -396,6 +396,15 @@ func (twb *txnWriteBuffer) applyTransformations(
 			twb.addToBuffer(t.Key, t.Value, t.Sequence)
 
 		case *kvpb.DeleteRequest:
+			// To correctly populate FoundKey in the response, we need to look in our
+			// write buffer to see if there is a tombstone.
+			var foundKey bool
+			val, served := twb.maybeServeRead(t.Key, t.Sequence)
+			if served {
+				log.VEventf(ctx, 2, "serving read portion of %s on key %s from the buffer", t.Method(), t.Key)
+				foundKey = val.IsPresent()
+			}
+
 			// If MustAcquireExclusiveLock flag is set on the DeleteRequest, then we
 			// need to add a locking Get to the BatchRequest, including if the key
 			// doesn't exist.
@@ -414,10 +423,20 @@ func (twb *txnWriteBuffer) applyTransformations(
 				baRemote.Requests = append(baRemote.Requests, getReqU)
 			}
 
+			// If we found a key in our write buffer we use that
+			// result regardless of what the GetResponse that we
+			// might have sent says.
+			//
+			// NOTE(ssd): We are assuming that callers who care
+			// about an accurate value of FoundKey also set
+			// MustAcquireExclusiveLock.
 			var ru kvpb.ResponseUnion
-			ru.MustSetInner(&kvpb.DeleteResponse{
-				FoundKey: false,
-			})
+			if served || !t.MustAcquireExclusiveLock {
+				ru.MustSetInner(&kvpb.DeleteResponse{
+					FoundKey: foundKey,
+				})
+			}
+
 			ts = append(ts, transformation{
 				stripped:    !t.MustAcquireExclusiveLock,
 				index:       i,
@@ -813,13 +832,19 @@ func (t transformation) toResp(
 		ru = t.resp
 
 	case *kvpb.DeleteRequest:
-		getResp := br.GetInner().(*kvpb.GetResponse)
 		ru = t.resp
-		if log.ExpensiveLogEnabled(ctx, 2) {
-			log.Eventf(ctx, "synthesizing DeleteResponse from GetResponse: %#v", getResp)
+		// If the deletion response is already set, it means we served response from
+		// the write buffer. We can still be here because we happened to need to
+		// send a GetRequest solely for the locking behaviour.
+		if ru.GetDelete() == nil {
+			getResp := br.GetInner().(*kvpb.GetResponse)
+			if log.ExpensiveLogEnabled(ctx, 2) {
+				log.Eventf(ctx, "synthesizing DeleteResponse from GetResponse: %#v", getResp)
+			}
+			ru.MustSetInner(&kvpb.DeleteResponse{
+				FoundKey: getResp.Value.IsPresent(),
+			})
 		}
-		ru.GetDelete().FoundKey = getResp.Value.IsPresent()
-
 	case *kvpb.GetRequest:
 		// Get requests must be served from the local buffer if a transaction
 		// performed a previous write to the key being read. However, Get requests

pkg/kv/kvpb/api.proto

@@ -408,7 +408,11 @@ message DeleteRequest {
   RequestHeader header = 1 [(gogoproto.nullable) = false, (gogoproto.embed) = true];
   // MustAcquireExclusiveLock, if set, indicates that a lock with the strength
   // no lower than "exclusive" needs to be acquired on the key, even if it
-  // doesn't exist.
+  // doesn't exist. Note that when MustAcquireExclusiveLock is false, the
+  // FoundKey field in the DeleteResponse may be incorrect when the kvclient has
+  // been configured to buffer writes.
+  //
+  // TODO(ssd): Separate the behaviour of FoundKey to not depend on this flag.
   bool must_acquire_exclusive_lock = 2;
 }
 

pkg/sql/logictest/logic.go

@@ -2092,6 +2092,21 @@ func (c clusterOptIgnoreStrictGCForTenants) apply(args *base.TestServerArgs) {
 	args.Knobs.Store.(*kvserver.StoreTestingKnobs).IgnoreStrictGCEnforcement = true
 }
 
+// clusterOptDisableUseMVCCRangeTombstonesForPointDeletes corresponds
+// to the disable-mvcc-range-tombstones-for-point-deletes directive.
+type clusterOptDisableUseMVCCRangeTombstonesForPointDeletes struct{}
+
+var _ clusterOpt = clusterOptDisableUseMVCCRangeTombstonesForPointDeletes{}
+
+// apply implements the clusterOpt interface.
+func (c clusterOptDisableUseMVCCRangeTombstonesForPointDeletes) apply(args *base.TestServerArgs) {
+	_, ok := args.Knobs.Store.(*kvserver.StoreTestingKnobs)
+	if !ok {
+		args.Knobs.Store = &kvserver.StoreTestingKnobs{}
+	}
+	args.Knobs.Store.(*kvserver.StoreTestingKnobs).EvalKnobs.UseRangeTombstonesForPointDeletes = false
+}
+
 // knobOptDisableCorpusGeneration disables corpus generation for declarative
 // schema changer.
 type knobOptDisableCorpusGeneration struct{}
@@ -2236,6 +2251,8 @@ func readClusterOptions(t *testing.T, path string) []clusterOpt {
 			res = append(res, clusterOptTracingOff{})
 		case "ignore-tenant-strict-gc-enforcement":
 			res = append(res, clusterOptIgnoreStrictGCForTenants{})
+		case "disable-mvcc-range-tombstones-for-point-deletes":
+			res = append(res, clusterOptDisableUseMVCCRangeTombstonesForPointDeletes{})
 		default:
 			t.Fatalf("unrecognized cluster option: %s", opt)
 		}

pkg/sql/logictest/testdata/logic_test/buffered_writes

@@ -1,11 +1,12 @@
-
-subtest point_delete
+# cluster-opt: disable-mvcc-range-tombstones-for-point-deletes
 
 statement ok
 SET kv_transaction_buffered_writes_enabled=true
 
 statement ok
-CREATE TABLE t1 (pk int primary key, v int)
+CREATE TABLE t1 (pk int primary key, v int, FAMILY (pk, v))
+
+subtest point_delete
 
 statement ok
 INSERT INTO t1 VALUES (1,1)
@@ -21,3 +22,41 @@ DELETE FROM t1 WHERE pk = 3
 
 statement ok
 COMMIT
+
+subtest repeated_point_delete
+
+statement ok
+INSERT INTO t1 VALUES (1,1)
+
+statement ok
+BEGIN
+
+statement count 1
+DELETE FROM t1 WHERE pk = 1
+
+# The second delete should be served from the write buffer and observe
+# the buffered tombstone.
+statement count 0
+DELETE FROM t1 WHERE pk = 1
+
+statement ok
+COMMIT
+
+subtest point_delete_after_write
+
+statement ok
+BEGIN
+
+statement ok
+INSERT INTO t1 VALUES (1,1)
+
+statement count 1
+DELETE FROM t1 WHERE pk = 1
+
+# The second delete should be served from the write buffer and observe
+# the buffered tombstone.
+statement count 0
+DELETE FROM t1 WHERE pk = 1
+
+statement ok
+COMMIT

pkg/sql/logictest/testdata/logic_test/row_level_security

@@ -1280,6 +1280,124 @@ DROP FUNCTION my_non_sec_definer_reader_function;
 statement ok
 DROP TABLE sensitive_data_table CASCADE;
 
+subtest alter_policy
+
+statement ok
+CREATE TABLE alter_policy_table (c1 INT NOT NULL PRIMARY KEY, c2 TEXT, FAMILY (c1, c2));
+
+statement ok
+ALTER TABLE alter_policy_table ENABLE ROW LEVEL SECURITY, FORCE ROW LEVEL SECURITY;
+
+statement ok
+CREATE ROLE alter_policy_role;
+
+statement ok
+CREATE ROLE aux1;
+
+statement ok
+CREATE USER aux2;
+
+statement ok
+CREATE SEQUENCE seq1;
+
+statement ok
+GRANT ALL ON seq1 TO alter_policy_role;
+
+statement ok
+ALTER TABLE alter_policy_table OWNER TO alter_policy_role;
+
+statement ok
+SET ROLE alter_policy_role;
+
+statement ok
+CREATE POLICY p ON alter_policy_table FOR INSERT WITH CHECK (false);
+
+statement error pq: new row violates row-level security policy for table "alter_policy_table"
+INSERT INTO alter_policy_table VALUES (1, 'one'), (2, 'two'), (3, 'three');
+
+statement error pq: only WITH CHECK expression allowed for INSERT
+ALTER POLICY p ON alter_policy_table USING (true);
+
+statement ok
+ALTER POLICY p ON alter_policy_table WITH CHECK (nextval('seq1') < 10000);
+
+statement ok
+INSERT INTO alter_policy_table VALUES (1, 'one'), (2, 'two'), (3, 'three');
+
+query I
+SELECT c1 FROM alter_policy_table ORDER BY c1;
+----
+
+statement ok
+ALTER POLICY p ON alter_policy_table RENAME TO p_ins;
+
+statement ok
+CREATE POLICY p ON alter_policy_table FOR SELECT TO aux1 USING (c1 > 0);
+
+query I
+SELECT c1 FROM alter_policy_table ORDER BY c1;
+----
+
+statement error pq: policy "p_sel" for table "alter_policy_table" does not exist
+ALTER POLICY p_sel ON alter_policy_table WITH CHECK (true);
+
+statement error pq: WITH CHECK cannot be applied to SELECT or DELETE
+ALTER POLICY p ON alter_policy_table WITH CHECK (true);
+
+statement ok
+ALTER POLICY p ON alter_policy_table TO alter_policy_role,aux1,aux2 USING (c1 != 1);
+
+query I
+SELECT c1 FROM alter_policy_table ORDER BY c1;
+----
+2
+3
+
+statement ok
+ALTER POLICY p ON alter_policy_table RENAME TO p_sel;
+
+query TT
+SHOW CREATE TABLE alter_policy_table;
+----
+alter_policy_table  CREATE TABLE public.alter_policy_table (
+                      c1 INT8 NOT NULL,
+                      c2 STRING NULL,
+                      CONSTRAINT alter_policy_table_pkey PRIMARY KEY (c1 ASC),
+                      FAMILY fam_0_c1_c2 (c1, c2)
+                    );
+                    ALTER TABLE public.alter_policy_table ENABLE ROW LEVEL SECURITY, FORCE ROW LEVEL SECURITY;
+                    CREATE POLICY p_ins ON public.alter_policy_table AS PERMISSIVE FOR INSERT TO public WITH CHECK (nextval('public.seq1'::REGCLASS) < 10000:::INT8);
+                    CREATE POLICY p_sel ON public.alter_policy_table AS PERMISSIVE FOR SELECT TO aux1, alter_policy_role, aux2 USING (c1 != 1:::INT8)
+
+# TODO(143358): Include roles in the SHOW POLICIES output.
+query TTTTT colnames
+SELECT name,cmd,type,using_expr,with_check_expr
+FROM [SHOW POLICIES FOR alter_policy_table]
+ORDER BY name DESC;
+----
+name   cmd     type        using_expr      with_check_expr
+p_sel  SELECT  permissive  c1 != 1:::INT8  ·
+p_ins  INSERT  permissive  ·               nextval('public.seq1'::REGCLASS) < 10000:::INT8
+
+statement ok
+SET ROLE root;
+
+statement error pq: cannot drop sequence seq1 because other objects depend on it
+DROP SEQUENCE seq1;
+
+# Change the policy so there isn't a dependency on seq1 anymore.
+statement ok
+ALTER POLICY p_ins ON alter_policy_table WITH CHECK (true);
+
+statement ok
+DROP SEQUENCE seq1;
+
+statement ok
+DROP TABLE alter_policy_table;
+
+statement ok
+DROP ROLE alter_policy_role, aux1, aux2;
+
 # Verify that you need to be the table owner to do any of the RLS DDLs
 subtest table_owner_and_rls_ddl
 
@@ -1313,14 +1431,14 @@ DROP POLICY p1 on table_owner_test;
 statement ok
 CREATE POLICY new_p1 on table_owner_test;
 
-statement error pq: unimplemented: ALTER POLICY is not yet implemented
+statement ok
 ALTER POLICY new_p1 on table_owner_test RENAME TO p1;
 
-statement error pq: unimplemented: ALTER POLICY is not yet implemented
+statement ok
 ALTER POLICY p1 on table_owner_test RENAME TO new_p1;
 
-statement error pq: unimplemented: ALTER POLICY is not yet implemented
-ALTER POLICY p1 on table_owner_test USING (true);
+statement ok
+ALTER POLICY new_p1 on table_owner_test USING (true);
 
 statement ok
 SET ROLE nontab_owner;
@@ -1337,10 +1455,10 @@ CREATE POLICY p2 on table_owner_test;
 statement error pq: must be owner of relation table_owner_test
 DROP POLICY new_p1 on table_owner_test;
 
-statement error pq: unimplemented: ALTER POLICY is not yet implemented
+statement error pq: must be owner of relation table_owner_test
 ALTER POLICY new_p1 on table_owner_test WITH CHECK (true);
 
-statement error pq: unimplemented: ALTER POLICY is not yet implemented
+statement error pq: must be owner of relation table_owner_test
 ALTER POLICY new_p1 on table_owner_test RENAME TO p1;
 
 statement ok
@@ -2634,12 +2752,9 @@ query I
 UPDATE cnt SET counter = counter + 1 RETURNING counter;
 ----
 
-# Now replace the UPDATE policy with one that allows everything.
+# Now alter the UPDATE policy with one that allows everything.
 statement ok
-DROP POLICY upd1 ON cnt;
-
-statement ok
-CREATE POLICY upd1 ON cnt FOR UPDATE USING (true);
+ALTER POLICY upd1 ON cnt USING (true);
 
 query I
 UPDATE cnt SET counter = counter + 1 RETURNING counter;
@@ -2648,10 +2763,7 @@ UPDATE cnt SET counter = counter + 1 RETURNING counter;
 
 # Update the UPDATE policy so that it allows old rows but blocks all new rows.
 statement ok
-DROP POLICY upd1 ON cnt;
-
-statement ok
-CREATE POLICY upd1 ON cnt FOR UPDATE USING (true) WITH CHECK (false);
+ALTER POLICY upd1 ON cnt USING (true) WITH CHECK (false);
 
 # We are able to read the row but cannot write a new row as it violates the
 # update policy.
@@ -2673,10 +2785,7 @@ select counter from cnt;
 
 # Now change the select policy to be always false, and delete policy to be always true.
 statement ok
-DROP POLICY sel1 ON cnt;
-
-statement ok
-CREATE POLICY sel1 ON cnt FOR SELECT USING (false);
+ALTER POLICY sel1 ON cnt USING (false);
 
 statement ok
 CREATE POLICY del1 ON cnt FOR DELETE USING (true);

pkg/sql/schemachanger/dml_injection_test.go

@@ -416,6 +416,30 @@ func TestAlterTableDMLInjection(t *testing.T) {
 			setup:        []string{"CREATE INDEX idx ON tbl (val) USING HASH"},
 			schemaChange: "DROP INDEX idx",
 		},
+		{
+			desc: "alter policy name",
+			setup: []string{
+				"SET enable_row_level_security=on",
+				"CREATE POLICY p ON tbl FOR SELECT USING (val > 0)",
+				"ALTER TABLE tbl ENABLE ROW LEVEL SECURITY, FORCE ROW LEVEL SECURITY",
+				"CREATE USER foo",
+				"ALTER TABLE tbl OWNER TO foo",
+				"SET ROLE foo",
+			},
+			schemaChange: "ALTER POLICY p ON tbl RENAME TO policy_1",
+		},
+		{
+			desc: "alter policy using expression",
+			setup: []string{
+				"SET enable_row_level_security=on",
+				"CREATE POLICY p ON tbl FOR SELECT USING (val > 0)",
+				"ALTER TABLE tbl ENABLE ROW LEVEL SECURITY, FORCE ROW LEVEL SECURITY",
+				"CREATE USER foo",
+				"ALTER TABLE tbl OWNER TO foo",
+				"SET ROLE foo",
+			},
+			schemaChange: "ALTER POLICY p ON tbl USING (val > -10)",
+		},
 		{
 			desc: "drop column with index using hash cascade",
 			setup: []string{