execbuilder: enforce_home_region should only apply to DML

[msirek]

Fixes #89875
Fixes #88789

This fixes a problem where the enforce_home_region session flag might
cause non-DML statements to error out, such as SHOW CREATE, if those
statements utilize scans or joins of multiregion tables.

This also fixes issues with proper erroring out of mutation DML like
UPDATE AND DELETE. For example, the following previously did not error:

CREATE TABLE messages_rbr (
    account_id INT NOT NULL,
    message_id   UUID DEFAULT gen_random_uuid(),
    message    STRING NOT NULL,
    PRIMARY KEY (account_id),
    INDEX msg_idx(message)
)
LOCALITY REGIONAL BY ROW;

SET enforce_home_region = true;

DELETE FROM messages_rbr WHERE message = 'Hello World!'
ERROR: Query has no home region. Try adding a filter on messages_rbr.crdb_region and/or on key column (messages_rbr.account_id).
SQLSTATE: XCHR2

Release note (bug fix): This patch fixes an issue with the
enforce_home_region session setting which may cause SHOW CREATE TABLE or
other non-DML statements to error out if the optimizer plan for the
statement involves accessing a multiregion table.

[cockroach-teamcity]

This change is 

[yuzefovich]

Does this also fix #88789?

[msirek]

Does this also fix #88789?

Yes, thanks!

[rytaft]

Reviewed 23 of 23 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @msirek and @yuzefovich)

---

-- commits line 11 at r1:
Can you give some more details?

---

pkg/sql/region_util.go line 2364 at r1 (raw file):

// IsANSIDML is part of the eval.Planner interface.
func (p *planner) IsANSIDML() bool {

nit: Consider changing this here and everywhere else to just IsDML. IsANSIDML seems overly verbose...

---

pkg/sql/opt/exec/execbuilder/relational.go line 1843 at r1 (raw file):

		inputTable := inputTableMeta.Table
		if inputTable.ID() == skipID {
			continue

why is this needed?

---

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 419 at r1 (raw file):

AND rbr.crdb_region = 'ap-southeast-2'
----
project

why does this have an extra project now?

[msirek]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rytaft and @yuzefovich)

---

pkg/sql/region_util.go line 2364 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: Consider changing this here and everywhere else to just IsDML. IsANSIDML seems overly verbose...

The reason I named it this is because DML has a side meaning of covering only mutation operations, e.g. https://cloud.google.com/bigquery/docs/reference/standard-sql/data-manipulation-language

,but formally in ANSI, SELECT is included. Just trying to clarify this in case someone is confused by SELECT being included. Also, our code base already tags statements which aren't true DML, as TypeDML:

cockroach/pkg/sql/sem/tree/stmt.go

Lines 1183 to 1184 in a84835a

	// StatementType implements the Statement interface.
	func (*RelocateRange) StatementType() StatementType { return TypeDML }

So, by naming this ANSIDML, perhaps it will be more clear that this covers only the statements considered by ANSI to be DML.

[msirek]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @rytaft and @yuzefovich)

---

-- commits line 11 at r1:

Previously, rytaft (Rebecca Taft) wrote…

Can you give some more details?

Added an example.

---

pkg/sql/opt/exec/execbuilder/relational.go line 1843 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

why is this needed?

Added the comment:

		// Mutation DML errors out with additional information via a call to
		// `filterSuggestionError`, handled by the caller, so skip the target of
		// the mutations if encountered here.

Code quote:

if inputTable.ID() == skipID {

---

pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 419 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

why does this have an extra project now?

This is due to removal of the explicit non-hidden crdb_region column in the messages_rbr table. Since it's now hidden, it's no longer part of the * column expansion and is projected away.

Code quote:

EXPLAIN(OPT) SELECT * FROM  messages_rbt rbt INNER LOOKUP JOIN messages_rbr rbr ON rbr.account_id = rbt.account_id

[rytaft]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @msirek and @yuzefovich)

---

pkg/sql/region_util.go line 2364 at r1 (raw file):

Previously, msirek (Mark Sirek) wrote…

The reason I named it this is because DML has a side meaning of covering only mutation operations, e.g. https://cloud.google.com/bigquery/docs/reference/standard-sql/data-manipulation-language

,but formally in ANSI, SELECT is included. Just trying to clarify this in case someone is confused by SELECT being included. Also, our code base already tags statements which aren't true DML, as TypeDML:

cockroach/pkg/sql/sem/tree/stmt.go

Lines 1183 to 1184 in a84835a

	// StatementType implements the Statement interface.
	func (*RelocateRange) StatementType() StatementType { return TypeDML }

So, by naming this ANSIDML, perhaps it will be more clear that this covers only the statements considered by ANSI to be DML.

ok thanks for the explanation!

[msirek]

TFTR!
bors r=rytaft

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

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 51c220d to blathers/backport-release-22.2-90007: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}