sql: Add RLS support to crdb_internal.create_statements table

[Dedej-Bergin]

This PR adds a new column policy_statements to the crdb_internal.create_statements table, and populates this column with the appropriate row level security statements.

Epic: CRDB-11724
Fixes: #136753
Release note: None

[cockroach-teamcity]

This change is 

[spilchen]

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Dedej-Bergin)

---

pkg/sql/show_create.go line 61 at r1 (raw file):

	RedactableValues bool

	IgnoreCreatePolicies bool

Does anyone set this field? It seems like it will always be false. I thought we were going to use it for the create_nofks column. Also, maybe a better name would be IgnoreRLSStatements? We should include a comment for it too to be consistent with the other fields in this struct.

---

pkg/sql/show_create.go line 232 at r1 (raw file):

func showRLSAlterStatements(
	tn *tree.TableName, table catalog.TableDescriptor, addNewLine bool,
) (error, string) {

It's usually standard for the error output parm to come last.

---

pkg/sql/crdb_internal.go line 3970 at r1 (raw file):

  state                         STRING NOT NULL,
  create_nofks                  STRING NOT NULL,
  policy_statements       			STRING[] NOT NULL,

nit: the alignment seems to be off with this new column. Can you fix that?

---

pkg/sql/crdb_internal.go line 3971 at r1 (raw file):

  create_nofks                  STRING NOT NULL,
  policy_statements       			STRING[] NOT NULL,
  alter_statements              STRING[] NOT NULL,

I wonder if we should put the ALTER TABLE ... * ROW LEVEL SECURITY statement into this column. That way the policy_statements column would just have the CREATE POLICY statements. Otherwise, I think the name of policy_statements is misnamed.

---

pkg/sql/crdb_internal.go line 4030 at r1 (raw file):

			// Add the RLS ALTER statements to the policy_statements column.
			if err, alterRLSStatements := showRLSAlterStatements(&name, table, false); err != nil {

I think we should move this call into showAlterStatement.

---

pkg/sql/crdb_internal.go line 4040 at r1 (raw file):

			// Add the RLS policy statements to the policy_statements column.
			for _, policy := range table.GetPolicies() {
				if err, policyStatement := showPolicyStatements(ctx, &name, table, p.EvalContext(), &p.semaCtx, p.SessionData(), policy, false); err != nil {

It seems like this code is the wrong level abstraction. For the other columns we have a single function for each column. So perhaps, we do the same and move the policy loop into it's own function. A way to do this is to rename showPolicyStatements to showPolicyStatement (singular), then create a new function called showPolicyStatements that handles the looping that we can call here.

[Dedej-Bergin]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @spilchen)

---

pkg/sql/crdb_internal.go line 3970 at r1 (raw file):

Previously, spilchen wrote…

nit: the alignment seems to be off with this new column. Can you fix that?

Yes, just fixed it, not sure why my auto formatting wasn't able to do this. Thanks Matt!

---

pkg/sql/crdb_internal.go line 3971 at r1 (raw file):

Previously, spilchen wrote…

I wonder if we should put the ALTER TABLE ... * ROW LEVEL SECURITY statement into this column. That way the policy_statements column would just have the CREATE POLICY statements. Otherwise, I think the name of policy_statements is misnamed.

Yes, I agree, since it is an ALTER statement it should go in the alter_statements column. I just made this change, thanks Matt!

---

pkg/sql/crdb_internal.go line 4030 at r1 (raw file):

Previously, spilchen wrote…

I think we should move this call into showAlterStatement.

Yes, I just moved it, thanks Matt!

---

pkg/sql/crdb_internal.go line 4040 at r1 (raw file):

Previously, spilchen wrote…

It seems like this code is the wrong level abstraction. For the other columns we have a single function for each column. So perhaps, we do the same and move the policy loop into it's own function. A way to do this is to rename showPolicyStatements to showPolicyStatement (singular), then create a new function called showPolicyStatements that handles the looping that we can call here.

I like it, just made this change, thanks for the explanation Matt!

---

pkg/sql/show_create.go line 61 at r1 (raw file):

Previously, spilchen wrote…

Does anyone set this field? It seems like it will always be false. I thought we were going to use it for the create_nofks column. Also, maybe a better name would be IgnoreRLSStatements? We should include a comment for it too to be consistent with the other fields in this struct.

Ah yes, I forgot about this part. I added the comment and a test for create_nofks in row_level_security. I also just made some changes for the create_nofks code but I'm unsure about them, let me know if these are the code changes we wanted and thanks!

---

pkg/sql/show_create.go line 232 at r1 (raw file):

Previously, spilchen wrote…

It's usually standard for the error output parm to come last.

I'm still learning the standards, thanks for pointing it out, just made this change.

[spilchen]

Thanks for making the revisions.

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Dedej-Bergin)

[Dedej-Bergin]

TFTR!

bors r+

[craig]

Build succeeded:

• lint
• check_generated_code
• docker_image_amd64
• unit_tests
• linux_amd64_fips_build
• local_roachtest
• local_roachtest_fips
• examples_orms
• acceptance
• linux_amd64_build