Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: update UDF references in ALTER TABLE #110130

Merged
merged 1 commit into from Sep 15, 2023

Conversation

chrisseto
Copy link
Contributor

Previously, ALTER TABLE ... ADD CONSTRAINT statements that referenced a UDF did not appropriately update UDFs' DependedOnBy field to reference the table/constraint that utilized it. This would surface as the validation error reported in #109414, relation X: depends-on function X has no corresponding depended-on-by back reference. This commit ensures that the back references are updated by copying a hook from CREATE TABLE.

Epic: None
Fixes: #109414
Release note (bug fix): ALTER TABLE ... ADD CONSTRAINT CHECK ... statements that utilize a UDF in the CHECK no longer cause a validation error.

@chrisseto chrisseto requested a review from a team as a code owner September 6, 2023 17:58
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@chrisseto chrisseto added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Sep 6, 2023
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great! just had very minor testing comments

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


pkg/sql/logictest/testdata/logic_test/udf_in_constraints line 444 at r1 (raw file):

subtest issue-109414-minimal

statement ok

very cool that we got a repro for this!

one thing which may not be obvious is that if you send all the queries at once like this, they are al run in the same transaction (it's a PG compat thing). to make this more clear, could we

  • add a BEGIN/COMMIT around the whole thing, so it's clear it's all the same transaction
  • or add a BEGIN/COMMIT around each command, so each one is forced to run in a different transaction

pkg/sql/logictest/testdata/logic_test/udf_in_constraints line 448 at r1 (raw file):

CREATE FUNCTION has_account(account_id UUID) RETURNS BOOL LANGUAGE SQL AS $$ SELECT EXISTS(SELECT * FROM accounts WHERE id = account_id) $$;
CREATE TABLE account_ref ( account_id UUID NOT NULL );
ALTER TABLE account_ref ADD CONSTRAINT diy_fk CHECK (has_account(account_id));

nit: it might be useful to throw in an assertion here, like check the output of SHOW CREATE TABLE account_ref


pkg/sql/logictest/testdata/logic_test/udf_in_constraints line 453 at r1 (raw file):

# of itself, so it's been included as it may catch other regressions that the
# minimal case wouldn't.
subtest issue-109414-full

both comments above apply to this subtest too

Copy link
Contributor Author

@chrisseto chrisseto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

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


pkg/sql/logictest/testdata/logic_test/udf_in_constraints line 444 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

very cool that we got a repro for this!

one thing which may not be obvious is that if you send all the queries at once like this, they are al run in the same transaction (it's a PG compat thing). to make this more clear, could we

  • add a BEGIN/COMMIT around the whole thing, so it's clear it's all the same transaction
  • or add a BEGIN/COMMIT around each command, so each one is forced to run in a different transaction

done and I managed to simplify the reproduction a bit further.


pkg/sql/logictest/testdata/logic_test/udf_in_constraints line 448 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: it might be useful to throw in an assertion here, like check the output of SHOW CREATE TABLE account_ref

Done.


pkg/sql/logictest/testdata/logic_test/udf_in_constraints line 453 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

both comments above apply to this subtest too

Done.

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/sql/logictest/testdata/logic_test/udf_in_constraints line 469 at r2 (raw file):

statement ok
CREATE TABLE accounts_a (id UUID NOT NULL);
CREATE TABLE accounts_b (id UUID NOT NULL);

ah, the test is failing due to something the logic tests do called column family randomization. basically, it will add non-default column families, so the output of SHOW is affected. you can fix it by making the CREATE statement be explicit about column families, like this:

CREATE TABLE accounts_a (id UUID NOT NULL, FAMILY "primary" (id, rowid))

Previously, `ALTER TABLE ... ADD CONSTRAINT` statements  that referenced
a UDF did not appropriately update UDFs' `DependedOnBy` field to
reference the table/constraint that utilized it. This would surface as
the validation error reported in cockroachdb#109414, `relation X: depends-on
function X has no corresponding depended-on-by back reference`. This
commit ensures that the back references are updated by copying a hook
from `CREATE TABLE`.

Epic: None
Fixes: cockroachdb#109414
Release note (bug fix): `ALTER TABLE ... ADD CONSTRAINT CHECK ...`
statements that utilize a UDF in the CHECK no longer cause a validation
error.
Copy link
Contributor Author

@chrisseto chrisseto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/sql/logictest/testdata/logic_test/udf_in_constraints line 469 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

ah, the test is failing due to something the logic tests do called column family randomization. basically, it will add non-default column families, so the output of SHOW is affected. you can fix it by making the CREATE statement be explicit about column families, like this:

CREATE TABLE accounts_a (id UUID NOT NULL, FAMILY "primary" (id, rowid))

aaaaah, thank you! Fixed.

@chrisseto
Copy link
Contributor Author

bors r=rafiss

@craig
Copy link
Contributor

craig bot commented Sep 15, 2023

Build succeeded:

@craig craig bot merged commit 1755d67 into cockroachdb:master Sep 15, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
3 participants