sql/opt: Generate synthetic check constraint to enforce RLS policies for new rows

[spilchen]

With row-level security, policies include a WITH CHECK expression to enforce constraints on new rows. This commit begins adding support for enforcing these policies by modifying the optbuilder to construct the check constraint, evaluate the expression, and pass the result to the execution engine. A future commit will integrate the execution engine to fully enforce these policies.

Since the expression for the synthetic check constraint is determined at INSERT or UPDATE time, a placeholder check constraint is added when building the optimizer table catalog. The check constraint is then finalized in the mutationBuilder.

Because the check constraint is constructed late in the process, a function is needed to look up the column ordinal for a given column ID. To facilitate this, the previously internal function lookupColumnOrdinal has been made external as LookupColumnOrdinal.

Epic: CRDB-45203
Release note: None
Informs: #136704

[cockroach-teamcity]

This change is 

[DrewKimball]

Reviewed 3 of 11 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @michae2)

---

pkg/sql/opt_catalog.go line 1146 at r1 (raw file):

		synthesizedChecks = append(synthesizedChecks, optCheckConstraint{
			isRLSConstraint: true,
		})

It seems unfortunate to introduce a new type of check constraint to the list that has to be handled differently and requires new plumbing. Would it be possible to make mutationBuilder.addCheckConstraintCols directly check for RLS policies rather than adding a dummy constraint?

---

pkg/sql/opt/optbuilder/mutation_builder.go line 894 at r1 (raw file):

					tabMeta:  mb.md.TableMeta(mb.tabID),
					oc:       mb.b.catalog,
					user:     mb.checkPrivilegeUser,

nit: it looks like this is the only place where mutationBuilder.checkPrivilegeUser is used. Can we get rid of it and just do mb.b.checkPrivilegeUser here instead?

[spilchen]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @michae2)

---

pkg/sql/opt_catalog.go line 1146 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

It seems unfortunate to introduce a new type of check constraint to the list that has to be handled differently and requires new plumbing. Would it be possible to make mutationBuilder.addCheckConstraintCols directly check for RLS policies rather than adding a dummy constraint?

Yes, it’s possible. In an earlier revision, I considered this approach. However, I ultimately decided to include a dummy check constraint in the Table object for several reasons. Handling it differently would have required adding special cases in multiple places:

• In mutationBuilder.init, the logic for computing slice sizes had to be adjusted by adding +1 for checks if the table had RLS enabled.
• Refactoring mutationBuilder.addCheckConstraintCols. This function needed modifications to properly handle the RLS constraint. Since we only perform actions when mb.tab.CheckCount() != 0, the logic had to be expanded to include RLS tables. Additionally, the per-constraint logic (the inner for loop) had to be modularized into a separate function/closure so it could be executed one additional time for RLS tables.
• Correcting the mutationBuilder.checkColIDs comment. The existing comment states, "Its length is always equal to the number of check constraints on the table (see opt.Table.CheckCount)." While changing this comment wouldn’t be a major issue, I felt that the original description was simple and intuitive. Keeping it unchanged avoided unnecessary complexity.

Since we were already treating it as a check constraint in multiple places, it made sense to explicitly add a constraint for it when constructing the Table object. Plus, we already had precedence for adding synthetic check constraints in the Table object for UDTs, so incorporating one for RLS felt like a natural fit.

[michae2]

Nice! Just a couple questions.

Reviewed 10 of 11 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @mgartner)

---

pkg/sql/opt/testutils/testcat/test_catalog.go line 1075 at r2 (raw file):

		}
	}
	return 0, nil

Should this be an error?

---

pkg/sql/opt/optbuilder/row_level_security.go line 132 at r2 (raw file):

		constraint:  expr,
		columnCount: len(colIDs),
		lookupColumnOrdinal: func(i int) (int, error) {

nit: Would it be simpler to save r.tab and colIDs in the struct and do the call to LookupColumnOrdinal directly in ColumnOrdinal?

---

pkg/sql/opt/optbuilder/testdata/row_level_security line 546 at r2 (raw file):

build
INSERT INTO t1_explicit_pk VALUES (2, 'second', '2008-11-19') ON CONFLICT DO NOTHING;

Should SELECT policies apply to the read part of INSERT ON CONFLICT?

---

pkg/sql/opt/optbuilder/mutation_builder.go line 903 at r2 (raw file):

			colName := scopeColName("")
			if check.IsRLSConstraint() {
				colName = colName.WithMetadataName("rls")

Is there an assertion somewhere that IsRLSConstraint is true for only one of the check constraints?

---

pkg/sql/opt/optbuilder/mutation_builder.go line 923 at r2 (raw file):

			//   expressions can exist for read and write operations. This means it's
			//   possible to read a row whose column values would violate the write
			//   expression.

Is there a test case that validates this?

[spilchen]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @michae2)

---

pkg/sql/opt/optbuilder/mutation_builder.go line 903 at r2 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Is there an assertion somewhere that IsRLSConstraint is true for only one of the check constraints?

Not yet, but that's a good suggestion. I'll add one to mutationBuilder.addCheckConstraintCols when we walk all of the constraints.

---

pkg/sql/opt/optbuilder/mutation_builder.go line 923 at r2 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Is there a test case that validates this?

There is a test for this in opt/optbuilder/testdata/row_level_security. Search for this comment:

# Verify that an update fetches the column if a policy references it, even when
# the column itself is not being modified.

The test verifies it by including the columns fetched in the plan. When I do the execution engine side of the changes, I was going to have a logictest for this too.

---

pkg/sql/opt/testutils/testcat/test_catalog.go line 1075 at r2 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Should this be an error?

Yes, good catch!

---

pkg/sql/opt/optbuilder/testdata/row_level_security line 546 at r2 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Should SELECT policies apply to the read part of INSERT ON CONFLICT?

Yes! And we need to apply the INSERT and UPDATE policies if we are going to follow the postgres model. I created this test to understand the behaviour when I realized that there is a bunch more work needed to support upsert. I ended up creating an issue (#141998) to deal with upsert rather than add it here. I'll add a comment to this testcase to indicate the support for it isn't complete.