Skip to content

Commit 7f2338b

Browse files
craig[bot]katmaybjeffswenson
committed
147215: docgen: update check external connection sql diagram r=kev-cao a=katmayb ![image](https://github.com/user-attachments/assets/40de8004-98fa-445e-8b17-45de54cb1657) Epic: none Release note: None Release justification: non-production code change 147366: crosscluster: add batch handler test support for sql writers r=jeffswenson a=jeffswenson This extends the batch handler tests so that they can be used to validate the SQL writers. The new tests are skipped because the SQL writers do not correctly handle tombstones that should win LWW. Release note: none Informs: #146117 Co-authored-by: katmayb <[email protected]> Co-authored-by: Jeff Swenson <[email protected]>
3 parents 8960c37 + 413f019 + b0e35da commit 7f2338b

File tree

10 files changed

+89
-17
lines changed

10 files changed

+89
-17
lines changed

‎docs/generated/sql/bnf/BUILD.bazel‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ FILES = [
8888
"cancel_session",
8989
"cancel_stmt",
9090
"check_column_level",
91+
"check_external_connection",
9192
"check_table_level",
9293
"close_cursor_stmt",
9394
"col_qualification",
@@ -103,7 +104,7 @@ FILES = [
103104
"create_database_stmt",
104105
"create_ddl_stmt",
105106
"create_extension_stmt",
106-
"create_external_connection_stmt",
107+
"create_external_connection",
107108
"create_func",
108109
"create_index_stmt",
109110
"create_index_with_storage_param",
@@ -126,7 +127,6 @@ FILES = [
126127
"create_type",
127128
"create_view_stmt",
128129
"check_stmt",
129-
"check_external_connection_stmt",
130130
"deallocate_stmt",
131131
"declare_cursor_stmt",
132132
"default_value_column_level",

‎docs/generated/sql/bnf/check_external_connection.bnf‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
check_external_connection_stmt ::=
2+
'CHECK' 'EXTERNAL' 'CONNECTION' connection_uri 'WITH' check_external_connection_options_list
3+
| 'CHECK' 'EXTERNAL' 'CONNECTION' connection_uri

‎docs/generated/sql/bnf/check_external_connection_stmt.bnf‎

Lines changed: 0 additions & 2 deletions
This file was deleted.

‎docs/generated/sql/bnf/create_external_connection_stmt.bnf‎ renamed to ‎docs/generated/sql/bnf/create_external_connection.bnf‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,2 @@
11
create_external_connection_stmt ::=
2-
'CREATE' 'EXTERNAL' 'CONNECTION' connection_name 'AS' connection_URI
2+
'CREATE' 'EXTERNAL' 'CONNECTION' connection_name 'AS' connection_uri

‎pkg/cmd/docgen/diagrams.go‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -676,6 +676,16 @@ var specs = []stmtSpec{
676676
replace: map[string]string{" stmt": " 'CREATE' 'TABLE' table_name '(' column_name column_type 'CHECK' '(' check_expr ')' ( column_constraints | ) ( ',' ( column_table_def ( ',' column_table_def )* ) | ) ( table_constraints | ) ')' ')'"},
677677
unlink: []string{"table_name", "column_name", "column_type", "check_expr", "column_constraints", "table_constraints"},
678678
},
679+
{
680+
name: "check_external_connection",
681+
stmt: "check_external_connection_stmt",
682+
inline: []string{"opt_with_check_external_connection_options_list"},
683+
exclude: []*regexp.Regexp{
684+
regexp.MustCompile("'WITH' 'OPTIONS'"),
685+
},
686+
replace: map[string]string{"string_or_placeholder": "connection_uri"},
687+
unlink: []string{"connection_uri"},
688+
},
679689
{
680690
name: "check_table_level",
681691
stmt: "stmt_block",
@@ -786,9 +796,10 @@ var specs = []stmtSpec{
786796
unlink: []string{"table_name", "sink", "option", "value"},
787797
},
788798
{
789-
name: "create_external_connection_stmt",
790-
replace: map[string]string{"label_spec": "connection_name", "string_or_placeholder": "connection_URI"},
791-
unlink: []string{"connection_name", "connection_URI"},
799+
name: "create_external_connection",
800+
stmt: "create_external_connection_stmt",
801+
replace: map[string]string{"label_spec": "connection_name", "string_or_placeholder": "connection_uri"},
802+
unlink: []string{"connection_name", "connection_uri"},
792803
},
793804
{
794805
name: "create_index_stmt",

‎pkg/crosscluster/logical/batch_handler_test.go‎

Lines changed: 63 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,21 +13,25 @@ import (
1313

1414
"github.com/cockroachdb/cockroach/pkg/base"
1515
"github.com/cockroachdb/cockroach/pkg/ccl/changefeedccl/cdctest"
16+
"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
1617
"github.com/cockroachdb/cockroach/pkg/keys"
1718
"github.com/cockroachdb/cockroach/pkg/repstream/streampb"
1819
"github.com/cockroachdb/cockroach/pkg/roachpb"
1920
"github.com/cockroachdb/cockroach/pkg/sql"
2021
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
2122
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
2223
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
24+
"github.com/cockroachdb/cockroach/pkg/sql/catalog/lease"
2325
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
2426
"github.com/cockroachdb/cockroach/pkg/sql/execinfra"
2527
"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
28+
"github.com/cockroachdb/cockroach/pkg/sql/isql"
2629
"github.com/cockroachdb/cockroach/pkg/sql/rowenc"
2730
"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
2831
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
2932
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
3033
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
34+
"github.com/cockroachdb/cockroach/pkg/testutils/skip"
3135
"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
3236
"github.com/cockroachdb/cockroach/pkg/util/hlc"
3337
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -116,7 +120,7 @@ func (b *EventBuilder) deleteEvent(
116120
// newKvBatchHandler creates a new batch handler for testing.
117121
func newKvBatchHandler(
118122
t *testing.T, s serverutils.ApplicationLayerInterface, tableName string,
119-
) (*kvRowProcessor, catalog.TableDescriptor) {
123+
) (BatchHandler, catalog.TableDescriptor) {
120124
ctx := context.Background()
121125
desc := cdctest.GetHydratedTableDescriptor(t, s.ExecutorConfig(), tree.Name(tableName))
122126
sd := sql.NewInternalSessionData(ctx, s.ClusterSettings(), "" /* opName */)
@@ -143,6 +147,38 @@ func newKvBatchHandler(
143147
return handler, desc
144148
}
145149

150+
// newSqlBatchHandler creates a new SQL-based batch handler for testing.
151+
func newSqlBatchHandler(
152+
t *testing.T, s serverutils.ApplicationLayerInterface, tableName string,
153+
) (BatchHandler, catalog.TableDescriptor) {
154+
ctx := context.Background()
155+
desc := cdctest.GetHydratedTableDescriptor(t, s.ExecutorConfig(), tree.Name(tableName))
156+
sd := sql.NewInternalSessionData(ctx, s.ClusterSettings(), "" /* opName */)
157+
codec := s.Codec()
158+
handler, err := makeSQLProcessor(
159+
ctx,
160+
s.ClusterSettings(),
161+
map[descpb.ID]sqlProcessorTableConfig{
162+
desc.GetID(): {
163+
srcDesc: desc,
164+
},
165+
},
166+
0, // jobID
167+
s.InternalDB().(descs.DB),
168+
s.InternalDB().(isql.DB).Executor(isql.WithSessionData(sd)),
169+
sd,
170+
execinfrapb.LogicalReplicationWriterSpec{
171+
Mode: jobspb.LogicalReplicationDetails_Immediate,
172+
},
173+
codec,
174+
s.LeaseManager().(*lease.Manager),
175+
)
176+
require.NoError(t, err)
177+
return handler, desc
178+
}
179+
180+
type batchHandlerFactory func(t *testing.T, s serverutils.ApplicationLayerInterface, tableName string) (BatchHandler, catalog.TableDescriptor)
181+
146182
// addAndRemoveColumn is a metamorphic test option that randomly decides whether
147183
// to add and remove a column to trigger a rebuild of the primary key.
148184
var addAndRemoveColumn = metamorphic.ConstantWithTestBool("batch-handler-exhaustive-rebuild-primary-index", false)
@@ -151,10 +187,34 @@ var addAndRemoveColumn = metamorphic.ConstantWithTestBool("batch-handler-exhaust
151187
// to add a unique constraint to the table.
152188
var uniqueConstraint = metamorphic.ConstantWithTestBool("batch-handler-exhaustive-unique-constraint", false)
153189

154-
func TestBatchHandlerExhaustive(t *testing.T) {
190+
func TestBatchHandlerExhaustiveKV(t *testing.T) {
191+
defer leaktest.AfterTest(t)()
192+
defer log.Scope(t).Close(t)
193+
194+
testBatchHandlerExhaustive(t, newKvBatchHandler)
195+
}
196+
197+
func TestBatchHandlerExhaustiveSQL(t *testing.T) {
198+
defer leaktest.AfterTest(t)()
199+
defer log.Scope(t).Close(t)
200+
201+
skip.WithIssue(t, 146117)
202+
203+
testBatchHandlerExhaustive(t, newSqlBatchHandler)
204+
}
205+
206+
func TestBatchHandlerExhaustiveCrud(t *testing.T) {
155207
defer leaktest.AfterTest(t)()
156208
defer log.Scope(t).Close(t)
157209

210+
skip.WithIssue(t, 146117)
211+
212+
testBatchHandlerExhaustive(t, newCrudBatchHandler)
213+
}
214+
215+
func testBatchHandlerExhaustive(t *testing.T, factory batchHandlerFactory) {
216+
t.Helper()
217+
158218
// This test is an "exhaustive" test of the batch handler. It tries to test
159219
// cross product of every possible (replication event type, local value,
160220
// previous value, lww win).
@@ -188,7 +248,7 @@ func TestBatchHandlerExhaustive(t *testing.T) {
188248
runner.Exec(t, `ALTER TABLE test_table DROP COLUMN temp_col`)
189249
}
190250

191-
handler, desc := newKvBatchHandler(t, s, "test_table")
251+
handler, desc := factory(t, s, "test_table")
192252
defer handler.ReleaseLeases(ctx)
193253

194254
// TODO(jeffswenson): test the other handler types.

‎pkg/crosscluster/logical/sql_crud_writer.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ func newCrudSqlWriter(
4545
discard jobspb.LogicalReplicationDetails_Discard,
4646
procConfigByDestID map[descpb.ID]sqlProcessorTableConfig,
4747
jobID jobspb.JobID,
48-
) (*sqlCrudWriter, error) {
48+
) (BatchHandler, error) {
4949
decoder, err := newEventDecoder(ctx, cfg.DB, evalCtx.Settings, procConfigByDestID)
5050
if err != nil {
5151
return nil, err

‎pkg/crosscluster/logical/table_batch_handler_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ import (
3030

3131
func newCrudBatchHandler(
3232
t *testing.T, s serverutils.ApplicationLayerInterface, tableName string,
33-
) (*sqlCrudWriter, catalog.TableDescriptor) {
33+
) (BatchHandler, catalog.TableDescriptor) {
3434
ctx := context.Background()
3535
desc := cdctest.GetHydratedTableDescriptor(t, s.ExecutorConfig(), tree.Name(tableName))
3636
sd := sql.NewInternalSessionData(ctx, s.ClusterSettings(), "" /* opName */)

‎pkg/gen/bnf.bzl‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ BNF_SRCS = [
8888
"//docs/generated/sql/bnf:cancel_session.bnf",
8989
"//docs/generated/sql/bnf:cancel_stmt.bnf",
9090
"//docs/generated/sql/bnf:check_column_level.bnf",
91-
"//docs/generated/sql/bnf:check_external_connection_stmt.bnf",
91+
"//docs/generated/sql/bnf:check_external_connection.bnf",
9292
"//docs/generated/sql/bnf:check_stmt.bnf",
9393
"//docs/generated/sql/bnf:check_table_level.bnf",
9494
"//docs/generated/sql/bnf:close_cursor_stmt.bnf",
@@ -105,7 +105,7 @@ BNF_SRCS = [
105105
"//docs/generated/sql/bnf:create_database_stmt.bnf",
106106
"//docs/generated/sql/bnf:create_ddl_stmt.bnf",
107107
"//docs/generated/sql/bnf:create_extension_stmt.bnf",
108-
"//docs/generated/sql/bnf:create_external_connection_stmt.bnf",
108+
"//docs/generated/sql/bnf:create_external_connection.bnf",
109109
"//docs/generated/sql/bnf:create_func.bnf",
110110
"//docs/generated/sql/bnf:create_index_stmt.bnf",
111111
"//docs/generated/sql/bnf:create_index_with_storage_param.bnf",

‎pkg/gen/docs.bzl‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ DOCS_SRCS = [
9898
"//docs/generated/sql/bnf:cancel_session.bnf",
9999
"//docs/generated/sql/bnf:cancel_stmt.bnf",
100100
"//docs/generated/sql/bnf:check_column_level.bnf",
101-
"//docs/generated/sql/bnf:check_external_connection_stmt.bnf",
101+
"//docs/generated/sql/bnf:check_external_connection.bnf",
102102
"//docs/generated/sql/bnf:check_stmt.bnf",
103103
"//docs/generated/sql/bnf:check_table_level.bnf",
104104
"//docs/generated/sql/bnf:close_cursor_stmt.bnf",
@@ -115,7 +115,7 @@ DOCS_SRCS = [
115115
"//docs/generated/sql/bnf:create_database_stmt.bnf",
116116
"//docs/generated/sql/bnf:create_ddl_stmt.bnf",
117117
"//docs/generated/sql/bnf:create_extension_stmt.bnf",
118-
"//docs/generated/sql/bnf:create_external_connection_stmt.bnf",
118+
"//docs/generated/sql/bnf:create_external_connection.bnf",
119119
"//docs/generated/sql/bnf:create_func.bnf",
120120
"//docs/generated/sql/bnf:create_index_stmt.bnf",
121121
"//docs/generated/sql/bnf:create_index_with_storage_param.bnf",

0 commit comments

Comments
 (0)