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: handle crdb_internal computed cols in colIsRefByComputed #126967

Closed
annrpom opened this issue Jul 10, 2024 · 2 comments · Fixed by #131797
Closed

workload/schemachange: handle crdb_internal computed cols in colIsRefByComputed #126967

annrpom opened this issue Jul 10, 2024 · 2 comments · Fixed by #131797
Assignees
Labels
C-test-failure Broken test (automatically or manually discovered). P-2 Issues/test failures with a fix SLA of 3 months T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@annrpom
Copy link
Contributor

annrpom commented Jul 10, 2024

Our query for colIsRefByComputed cannot handle the following case (human-readable ops generated by schemachange workload):

[email protected]:26257/demoapp/movr> CREATE TABLE public.table_w0_1 (                                        
                                ->     "co\\u58A1l1 _w0_8" INTERVAL NOT NULL,                              
                                ->     INDEX (                                                             
                                ->         (CASE                                                           
                                ->             WHEN "co\\u58A1l1 _w0_8" IS NULL THEN ')':::STRING          
                                ->             ELSE e'-s\\x12M\\bps':::STRING                              
                                ->         END) DESC                                                       
                                ->     )                                                                   
                                -> );                                                                      
CREATE TABLE

Time: 10ms total (execution 10ms / network 0ms)

[email protected]:26257/demoapp/movr> ALTER TABLE public.table_w0_1                                           
                                -> DROP COLUMN "co\\u58A1l1 _w0_8";                                        
ERROR: column "co\\\\u58A1l1 _w0_8" is referenced by computed column "crdb_internal_idx_expr"
SQLSTATE: 42P10

Jira issue: CRDB-40208

Epic CRDB-19168

@annrpom annrpom added C-test-failure Broken test (automatically or manually discovered). T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) labels Jul 10, 2024
@annrpom
Copy link
Contributor Author

annrpom commented Jul 18, 2024

From Faizan:

I think this would be similar to: getUniqueConstraintsForTable and the whole unique constraint logic that detects column references.

@exalate-issue-sync exalate-issue-sync bot added the P-2 Issues/test failures with a fix SLA of 3 months label Jul 30, 2024
@rafiss
Copy link
Collaborator

rafiss commented Aug 6, 2024

Let's fix this as part of enabling DROP COLUMN (#127286):

// TODO(#126967): We need to add a check for the column being in an expression
// to an index. In the case where the expression does not already exist for
// us to use, we add an internal crdb_internal_idx_expr prefixed column to
// the table.
stmt.potentialExecErrors.add(pgcode.InvalidColumnReference)

craig bot pushed a commit that referenced this issue Oct 4, 2024
131797: workload/schemachanger: address flakes and stabilize this test r=fqazi a=fqazi

This patch will address the following flakes:

1. Allow constraint violations on add column / alter pk, since concurrent inserts can cause these errors on commit
2. Fix aborted txn error inside ADD FOREIGN KEY, which was because one of the intropection queries was never run inside a child txn.
3. Address command is too large error in insert, which could cause inserts to fail when a large number of columns existed on a table.
4. Address a bug inside INSERT foreign key validation when multiple rows were inserted in the same batch
5. Handle crdb_internal computed cols in colIsRefByComputed, which removes a potential error from DROP COLUMN

Fixes: #131345
Fixes: #130923
Fixes: #126967

131845: kvserver: deflake TestNewRangefeedForceLeaseRetry r=iskettaneh a=iskettaneh

This commit lets the test waits for N1's view of N2's lease expiration to match N2's view. This is important in the rare case where N1 tries to increase N2's epoch, but it has a stale view of the lease expiration time.

Fixes: #131808

Release note: None

131905: cli: add --user flag to client cmds r=tbg a=tbg

I originally sent #130827 in reaction to finding that `./cockroach gen
haproxy` didn't work with SQL urls that used client certs for a non-root
user (it would erroneously expect to be pointed at the root client certs).

That PR caused problems too; now one needed to specify `--certs` even
though the certs were in the URL. There's a fix for that too (#131894)
but it all seems pretty tangled up.

This PR takes a more straightforward route: we revert #130827 and add
the `--user` flag to all client commands (who all already get the
`--sql` flag.

This should have fewer unintended consequences, and solves the problem.

Fixes #131802.
Fixes #131812.
Fixes #131814.
Fixes #131815.
Fixes #131816.
Fixes #131817.

Epic: none
Release note: None



Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Ibrahim Kettaneh <[email protected]>
Co-authored-by: Tobias Grieger <[email protected]>
@craig craig bot closed this as completed in 586ea79 Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-test-failure Broken test (automatically or manually discovered). P-2 Issues/test failures with a fix SLA of 3 months T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
No open projects
Status: Triage
Development

Successfully merging a pull request may close this issue.

3 participants