sql: add CHECK EXTERNAL CONNECTION command #138696

kev-cao · 2025-01-08T23:08:17Z

This patch adds the CHECK EXTERNAL CONNECTION command and replaces the old SHOW BACKUP CONNECTION syntax.

Epic: None

Release note: None

blathers-crl · 2025-01-08T23:08:21Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser.

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

cockroach-teamcity · 2025-01-08T23:08:33Z

This change is

dt · 2025-01-09T14:13:40Z

pkg/sql/export.go

@@ -146,7 +146,7 @@ func (ef *execFactory) ConstructExport(
 		panic(err)
 	}
 	// TODO(adityamaru): Ideally we'd use
-	// `cloudprivilege.CheckDestinationPrivileges privileges here, but because of
+	// `sql.CheckDestinationPrivileges privileges here, but because of


I guess we can fix this now in a followup

dt · 2025-01-09T14:35:45Z

pkg/backup/show.go

@@ -225,45 +222,6 @@ func showBackupPlanHook(
 	}
 	exprEval := p.ExprEvaluator("SHOW BACKUP")

-	// TODO(dt): find move this to its own hook.


So we're ripping out SHOW BACKUP CONNECTION here, rather than pointing it at the moved cloud check code?

I suppose this is okay since we never doc'ed it, but I think if we're doing that we can delete the case above about its header and we can delete the BackupConnectionTest parse/AST support too, right?

Yeah since it uses the planHookFn and the moved code creates a standalone planNode, trying to just have it point to it I think is more effort than it is worth. I'll remove all the old code in a follow up.

kev-cao · 2025-01-13T21:35:27Z

TFTR!

bors r=dt

137426: sql: make memo.IsStale more efficient when schema objects aren't modified r=fqazi a=fqazi This patch speeds up memo staleness checks for prepared statements by: 1. Updates the lease manager to expose a generation ID which allows you to know if any descriptors changed 2. Updates the table stats cache to add a generation ID to determine if any new stats were added 3. Updates the IsStale check to confirm if either the generation IDs have changed, zone configs, search_path, changed or current database has changed. If everything was the same in the last execution, the full CheckDependencies can be skipped. This is only used for prepared statements. 5. Updates the schema changer logic to lease new descriptors if the schema is already leased. This allows invalidation to work within a search path if new objects are made. Fixes: #105867 Numbers from sysbench (before these changes) using `rpsak.sh repeated`: ``` Before the changes (geomean TPS): 1121.75 After the changes (geomean TPS): 1165.47 Before the changes (geomean QPS): 22435.06 After the changes (geomean QPS): 23309.41 Percent improvement: 3.89% ``` Also from the sysbench workload numbers (from `@tbg)` write_only: ``` benchdiff --old lastmerge ./pkg/sql/tests -b -r 'Sysbench/SQL/3node/oltp_write_only' -d 1000x -c 10 name old time/op new time/op delta Sysbench/SQL/3node/oltp_write_only-24 4.38ms ± 1% 4.23ms ± 1% -3.38% (p=0.000 n=10+10) name old alloc/op new alloc/op delta Sysbench/SQL/3node/oltp_write_only-24 935kB ± 6% 933kB ± 6% ~ (p=0.739 n=10+10) name old allocs/op new allocs/op delta Sysbench/SQL/3node/oltp_write_only-24 6.08k ± 2% 6.04k ± 3% ~ (p=0.060 n=10+10) ``` read_only: ``` benchdiff --old lastmerge ./pkg/sql/tests -b -r 'Sysbench/SQL/3node/oltp_read_only' -d 1000x -c 10 test binaries already exist for d2bd29e: Merge #137750 test binaries already exist for 3cc7a6c: sql: add avoid_catalog_generation_for_staleness to pkg=1/1 iter=10/10 cockroachdb/cockroach/pkg/sql/tests \ name old time/op new time/op delta Sysbench/SQL/3node/oltp_read_only-24 7.88ms ± 1% 7.44ms ± 2% -5.62% (p=0.000 n=10+10) name old alloc/op new alloc/op delta Sysbench/SQL/3node/oltp_read_only-24 1.20MB ± 4% 1.19MB ± 5% ~ (p=0.353 n=10+10) name old allocs/op new allocs/op delta Sysbench/SQL/3node/oltp_read_only-24 4.71k ± 3% 4.64k ± 3% -1.48% (p=0.022 n=10+10) ``` 137929: sql: extend drop region for system db r=annrpom a=annrpom This patch ensures that references to a dropping region get cleaned up for the system database. Epic: none Fixes: #137095 Release note: None 138696: sql: add CHECK EXTERNAL CONNECTION command r=dt a=kev-cao This patch adds the `CHECK EXTERNAL CONNECTION` command and replaces the old `SHOW BACKUP CONNECTION` syntax. Epic: None Release note: None 138770: kvserver: enable leader leases for StoreRangeMergeAbandonedFollowersAutomaticallyGarbageCollected r=iskettaneh a=iskettaneh This commit enables leader leases to run with TestStoreRangeMergeAbandonedFollowersAutomaticallyGarbageCollected. The test used to move the lease and stop Raft traffic without waiting for leadership to follow it. This could cause flakes with leader leases as if the replica doesn't know who the leader is, it might not be able to determine the lease status. References: #136806 Release Note: None 138909: sql/schemachanger: Add support for storing policy command and type r=spilchen a=spilchen A previous commit introduced basic support for CREATE/DROP POLICY. This commit expands on that functionality by storing additional details in the policy descriptor. Specifically, it adds support for storing the policy type (restrictive or permissive) and the policy command (ALL, SELECT, INSERT, UPDATE, or DELETE). Since neither the policy type nor the policy command will be modifiable via ALTER POLICY, these attributes are included in the Policy element within the DSC, rather than as separate elements. Epic: CRDB-11724 Informs: #136696 Release note: None 138979: roachtest: update activerecord and ruby-pg expected failures r=rafiss a=rafiss - Mark 2 activerecord tests as flaky. - Mark 4 ruby-pg tests as passing after #138709 was merged. fixes #138886 fixes #138881 Release note: None Co-authored-by: Faizan Qazi <[email protected]> Co-authored-by: Annie Pompa <[email protected]> Co-authored-by: Kevin Cao <[email protected]> Co-authored-by: Ibrahim Kettaneh <[email protected]> Co-authored-by: Matt Spilchen <[email protected]> Co-authored-by: Rafi Shamim <[email protected]>

craig · 2025-01-13T22:07:55Z

Build failed (retrying...):

check_generated_code

craig · 2025-01-13T22:44:49Z

Canceled.

kev-cao · 2025-01-14T18:02:09Z

bors r+

137805: sql/row: use Put instead of CPut when updating value of secondary index r=yuzefovich,stevendanna a=michae2 **sql/row: use Put instead of CPut when updating value of secondary index** When an UPDATE statement changes the value but not the key of a secondary index (e.g. an update to the stored columns of a secondary index) we need to write a new version of the secondary index KV with the new value. We were using a CPutAllowingIfNotExists to do this, which verified that if the KV existed, the expected value was the value before update. But there's no need for this verification. We have other mechanisms to detect a write-write conflict with any other transaction that could have changed the value concurrently. We can simply use a Put to overwrite the previous value. This also matches what we do for the primary index when the PK doesn't change. Epic: None Release note: None --- **sql: change CPutAllowingIfNotExists with nil expValue to CPut** CPutAllowingIfNotExists with empty expValue is equivalent to CPut with empty expValue, so change a few spots to use regular CPut. This almost gets rid of CPutAllowingIfNotExists entirely, but there is at least one spot in backfill (introduced in #138707) that needs to allow for both a non-empty expValue and non-existence of the KV. Also drop "(expecting does not exist)" from CPut tracing, as CPut with empty expValue is now overwhelmingly the most common use of CPut. And this matches the tracing in #138707. Epic: None Release note: None 138243: changefeedccl: fix PTS test r=stevendanna a=asg0451 Fix failing TestPTSRecordProtectsTargetsAndSystemTables test Fixes: #135639 Fixes: #138066 Fixes: #137885 Fixes: #137505 Fixes: #136396 Fixes: #135805 Fixes: #135639 Release note: None 138696: sql: add CHECK EXTERNAL CONNECTION command r=kev-cao a=kev-cao This patch adds the `CHECK EXTERNAL CONNECTION` command and replaces the old `SHOW BACKUP CONNECTION` syntax. Epic: None Release note: None 138740: opt/bench: fix benchmark regression r=mgartner a=mgartner #### opt/bench: fix benchmark regression PR #138641 caused extra allocations for plan gist factories in optimizer benchmarks. These allocations should not be included in benchmark results, so they have been eliminated. Release note: None #### util/base64: use `bytes.Buffer` instead of `strings.Builder` in `Encoder` For our purposes in base64-encoding plan gists, using `bytes.Buffer` in `Encoder` causes fewer allocations, presumably because of a more aggressive growth algorithm. Epic: None Release note: None 138877: opt: reduce allocations when filtering histogram buckets r=mgartner a=mgartner `cat.HistogramBuckets` are now returned and passed by value in `getFilteredBucket` and `(*Histogram).addBucket`, respectively, eliminating some heap allocations. Also, two allocations when building spans from buckets via the `spanBuilder` have been combined into one. The new `(*spanBuilder).init` method simplifies the API by no longer requiring that prefix datums are passed to every invocation of `makeSpanFromBucket`. This also reduces redundant copying of the prefix. Epic: None Release note: None 139029: sql/logictest: disable column family mutations in some cases r=mgartner a=mgartner Random column family mutations are now disabled for `CREATE TABLE` statements with unique, hash-sharded indexes. This prevents the AST from being reserialized with a `UNIQUE` constraint with invalid options, instead of the original `UNIQUE INDEX`. See #65929 and #107398. Epic: None Release note: None 139039: ccl/serverccl: revise `TestTenantVars` cpu time checks r=xinhaoz a=xinhaoz Previously, this test verified that a portion of the test's user cpu time would be less than or equal to the entire tenant user cpu time up to that point. This check is flaky because there's no guarantee that the inactive tenant's cpu time will surpass the test cpu time. We now simply verify that the test cpu times are greater than or equal to the tenant metrics. The test was likely passing before 331596c because the reported tenant cpu time was accounting for the sql server prestart. A tenant's user cpu metrics are tracked from the time the `_status/load` endpoint is registered, and the commit above moved the router setup to occur just after the prestart. Epic: none Fixes: #119329 Release note: None Co-authored-by: Michael Erickson <[email protected]> Co-authored-by: Miles Frankel <[email protected]> Co-authored-by: Kevin Cao <[email protected]> Co-authored-by: Marcus Gartner <[email protected]> Co-authored-by: Xin Hao Zhang <[email protected]>

craig · 2025-01-14T19:14:48Z

Build failed (retrying...):

unit_tests

kev-cao · 2025-01-14T19:15:21Z

bors please it passed the ci checks stop being so difficult

This patch adds the `CHECK EXTERNAL CONNECTION` command and replaces the old `SHOW BACKUP CONNECTION` syntax. Epic: None Release note: None

craig · 2025-01-14T20:44:22Z

Canceled.

kev-cao · 2025-01-14T21:39:29Z

bors r+

craig · 2025-01-14T22:25:03Z

Build succeeded:

kev-cao requested review from a team as code owners January 8, 2025 23:08

kev-cao requested review from dt and mgartner and removed request for a team January 8, 2025 23:08

kev-cao changed the title ~~sql: add CHECK EXTERNAL CONNECTIONS command~~ sql: add CHECK EXTERNAL CONNECTION command Jan 8, 2025

kev-cao force-pushed the sql/check-connections branch from 2657320 to 9a92e43 Compare January 8, 2025 23:10

dt reviewed Jan 9, 2025

View reviewed changes

dt approved these changes Jan 9, 2025

View reviewed changes

kev-cao force-pushed the sql/check-connections branch from 9a92e43 to c51e3ac Compare January 9, 2025 16:52

kev-cao requested a review from msbutler January 9, 2025 16:54

kev-cao force-pushed the sql/check-connections branch 8 times, most recently from a5d986e to 99f939f Compare January 13, 2025 16:24

kev-cao force-pushed the sql/check-connections branch from 99f939f to 9097e31 Compare January 13, 2025 22:44

kev-cao force-pushed the sql/check-connections branch from 9097e31 to 44a81bf Compare January 14, 2025 15:57

sql: add CHECK EXTERNAL CONNECTION command

c9ef59a

This patch adds the `CHECK EXTERNAL CONNECTION` command and replaces the old `SHOW BACKUP CONNECTION` syntax. Epic: None Release note: None

kev-cao force-pushed the sql/check-connections branch from 44a81bf to c9ef59a Compare January 14, 2025 20:44

craig bot merged commit 7c622cd into cockroachdb:master Jan 14, 2025
21 of 22 checks passed

mohini-crl mentioned this pull request Jan 17, 2025

[Replicated] sql: add CHECK EXTERNAL CONNECTION command mohini-crl/cockroach#170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add CHECK EXTERNAL CONNECTION command #138696

sql: add CHECK EXTERNAL CONNECTION command #138696

kev-cao commented Jan 8, 2025 •

edited

Loading

blathers-crl bot commented Jan 8, 2025

cockroach-teamcity commented Jan 8, 2025

dt Jan 9, 2025

dt Jan 9, 2025

kev-cao Jan 9, 2025

kev-cao commented Jan 13, 2025

craig bot commented Jan 13, 2025

craig bot commented Jan 13, 2025

kev-cao commented Jan 14, 2025

craig bot commented Jan 14, 2025

kev-cao commented Jan 14, 2025 •

edited

Loading

craig bot commented Jan 14, 2025

kev-cao commented Jan 14, 2025

craig bot commented Jan 14, 2025

sql: add CHECK EXTERNAL CONNECTION command #138696

sql: add CHECK EXTERNAL CONNECTION command #138696

Conversation

kev-cao commented Jan 8, 2025 • edited Loading

blathers-crl bot commented Jan 8, 2025

cockroach-teamcity commented Jan 8, 2025

dt Jan 9, 2025

Choose a reason for hiding this comment

dt Jan 9, 2025

Choose a reason for hiding this comment

kev-cao Jan 9, 2025

Choose a reason for hiding this comment

kev-cao commented Jan 13, 2025

craig bot commented Jan 13, 2025

craig bot commented Jan 13, 2025

kev-cao commented Jan 14, 2025

craig bot commented Jan 14, 2025

kev-cao commented Jan 14, 2025 • edited Loading

craig bot commented Jan 14, 2025

kev-cao commented Jan 14, 2025

craig bot commented Jan 14, 2025

kev-cao commented Jan 8, 2025 •

edited

Loading

kev-cao commented Jan 14, 2025 •

edited

Loading