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

workload/schemachange: correct error code for referencing dropping enum #120865

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

annrpom
Copy link
Contributor

@annrpom annrpom commented Mar 22, 2024

This patch changes the error code expected for referencing an enum that's in the process of being dropped to the proper one.

Epic: none

Release note: None

@annrpom annrpom requested a review from a team as a code owner March 22, 2024 00:06
@annrpom annrpom requested review from DarrylWong and renatolabs and removed request for a team March 22, 2024 00:06
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@annrpom annrpom changed the title workload/schemachange: correct error code for referencing enum being dropped workload/schemachange: correct error code for referencing dropping enum Mar 22, 2024
@annrpom annrpom requested a review from a team March 22, 2024 00:07
@annrpom
Copy link
Contributor Author

annrpom commented Mar 22, 2024

whoops - we let this one fly under the radar for a bit

i also found a new trick i could try out here (as opposed to reading code (also - thank you, faizan for the cluster setting pointer)):

CREATE TYPE mood AS ENUM ('sad', 'ok', 'happy');
SET CLUSTER SETTING jobs.debug.pausepoints = 'schemachanger.before.exec';
DROP TYPE IF EXISTS mood;
-- other session

in the other session:

root@localhost:26257/defaultdb> CREATE FUNCTION ah (IN p1 mood) RETURNS VOID LANGUAGE SQL AS $$ SELECT NULL $$;       
ERROR: type "mood" does not exist
SQLSTATE: 42704

@rafiss
Copy link
Collaborator

rafiss commented Mar 22, 2024

ah interesting. have you tested with ./dev test pkg/ccl/testccl/workload/schemachange/ --stress --count=100 --ignore-cache -f=TestWorkload and altering the weights? just asking because there are at least two places where we do use UndefinedTable for types:

return nil, sqlerrors.NewUndefinedRelationError(
tree.NewUnqualifiedTableName(tree.Name(fmt.Sprintf("[%d]", id))))

return nil, sqlerrors.NewUndefinedRelationError(&tn)

So since the workload hasn't failed so far with this looking for the wrong error code, i wonder if it's actually fine?

@annrpom
Copy link
Contributor Author

annrpom commented Mar 22, 2024

oh interesting! i just tested with ./dev test pkg/ccl/testccl/workload/schemachange/ --stress --ignore-cache -f=TestWorkload & fwiw, i have stressed this workload a bunch this way and this is the first time i have encountered this error

i got:

 "clientTimestamp": "23:46:55.734682",
 "ops": [
  "BEGIN",
  {
   "sql": "ALTER TYPE schema_w1_6.enum_w0_8 DROP VALUE 'd'"
  },
  {
   "sql": "CREATE SCHEMA IF NOT EXISTS schema_w1_6 AUTHORIZATION root"
  },
  {
   "sql": "DROP FUNCTION IF EXISTS \"NoSuchFunction\""
  },
  {
   "sql": "ALTER FUNCTION public.udf_w0_7() RENAME TO udf_w1_15"
  },
  {
   "sql": "CREATE FUNCTION udf_w1_16(IN p1 schema_w1_6.enum_w0_8)\n\tRETURNS VOID\n\tLANGUAGE SQL\n\tAS $$ SELECT NULL $$",
   "expectedExecErr": "42P01"
  }
 ],
 "expectedExecErrors": "42P01",
 "expectedCommitErrors": "",
 "message": "***FAIL; Failed to receive an execution error when errors were expected",

maybe my triage of this was wrong - let me see

@rafiss
Copy link
Collaborator

rafiss commented Mar 22, 2024

it might be best to go with your change here, and also adjust the places i linked to to use UndefinedObject. as long as the tests pass!

This patch changes the error code expected for referencing
an enum that's in the process of being dropped to the proper
one. In addition, we ensure that we are returning the proper
error code whenever we get/lookup type descriptors

Epic: none

Release note: None
@annrpom annrpom force-pushed the create-fn-wrong-err branch from f1beb49 to 3038c13 Compare March 26, 2024 16:55
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.

lgtm!

@annrpom
Copy link
Contributor Author

annrpom commented Mar 26, 2024

TFTR! ('-')7

bors r=rafiss

@craig
Copy link
Contributor

craig bot commented Mar 26, 2024

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 26, 2024

This PR was included in a batch that was canceled, it will be automatically retried

@craig craig bot merged commit 6cf97ea into cockroachdb:master Mar 27, 2024
21 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants