sql: add SHOW TENANT name WITH REPLICATION STATUS #92628

lidorcarmel · 2022-11-28T23:05:34Z

Extending SHOW TENANT to also allow showing replication status which contains info such as the protected timestamp on the destination cluster and the source cluster name.

Command output right after the destination cluster is created:

[email protected]:26257/defaultdb> show tenant dest5 with replication status;
  id | name  | status | source_tenant_name | source_cluster_uri | replication_job_id | replicated_time | retained_time
-----+-------+--------+--------------------+--------------------+--------------------+-----------------+----------------
   7 | dest5 | ADD    | NULL               | NULL               | 819890711267737601 | NULL            | NULL
(1 row)

A bit later we have most stats (manually adjusting the source_cluster_uri):

[email protected]:26257/defaultdb> show tenant dest5 with replication status;
  id | name  | status | source_tenant_name | source_cluster_uri                                    | replication_job_id | replicated_time |       retained_time
-----+-------+--------+--------------------+-------------------------------------------------------+--------------------+-----------------+-----------------------------
   7 | dest5 | ADD    | src                | postgresql://[email protected]:26257/defaultdb?ssl...crt | 819890711267737601 | NULL            | 2022-12-05 23:00:04.516331
(1 row)

And a moment later the replication time is populated.

Fixes: #91261

Epic: CRDB-18749

Release note: None

cockroach-teamcity · 2022-11-30T15:38:28Z

This change is

adityamaru

Overall LGTM but two blocking comments:

Should this code live in CCL?
Should we start using expr for the tenant name based on our discussion last week that its better to start with expr and then become more restrictive in the future if the need be.

pkg/sql/parser/sql.y

pkg/sql/catalog/colinfo/result_columns.go

lidorcarmel · 2022-12-02T22:14:18Z

Overall LGTM but two blocking comments:

Should this code live in CCL?

So, maybe? IIUC we need SHOW TENANT to stay in BSL land and SHOW TENANT name WITH REPLICATION STATUS to move to CCL, which means that we need to break things to 2 different files. The benefit is not super clear to me because code in show_tenant.go doesn't do much.. just runs the ccl code. So, I was thinking it's fine to keep it as is and move stuff later if it adds some value? that said, if I'm missing something then I can do that in this PR.
Thanks for the offline chat about this!

Should we start using expr for the tenant name based on our discussion last week that its better to start with expr and then become more restrictive in the future if the need be.

Done, d_expr.

pkg/sql/catalog/colinfo/result_columns.go

pkg/sql/show_tenant.go

adityamaru · 2022-12-05T14:13:00Z

pkg/sql/show_tenant.go

+	replicationInfo *streampb.StreamIngestionStats
+	pts             hlc.Timestamp
+	columns         colinfo.ResultColumns
+	done            bool
 }

 func (p *planner) ShowTenant(_ context.Context, n *tree.ShowTenant) (planNode, error) {


Meta comment about the code structure, I think we should follow a pattern similar to sql/tenant_settings.go. Namely, this method should:

RequireAdminRole

Ensure we are running as the SystemTenant

Call p.analyzeExpr to get a TypedExpr from the Expr for the name. (this will give us subquery support for free, and seems to be the canonical way to evaluate expressions to specialized types).

Stash the TypedExpr in the showTenantNode

1 and 2 - Done.

re 3 and 4, let's do that later? I saw the TypedExpr thing but I prefer adding this when I can actually test subqueries, otherwise it feels like making things a bit more complicated without actual impact. if for example we would not want subqueries then I think the existing code is a bit easier to read compared to the TypedExpr thing.

It feels a little weird to me to be accepting an expression but then only using .String() when using the expression, instead of analyzing/asserting the type of the expression in a canonical manner. imo if we are using d_expr we should do the annoying work of evaluating the expression all the way through in the same diff. We can test subqueries, concat expressions etc. once we have the datadriven PR checked in, which is what we're doing for some of the other tests in the e2e file as well.

Alternatively, we can go back to using tree.Name and change this to d_expr when we change it for CREATE/DROP TENANT as well.

Done! definitely need to add tests for this, data driven.
PTAL.

adityamaru · 2022-12-05T14:15:17Z

pkg/sql/show_tenant.go

+	} else {
+		node.columns = colinfo.TenantColumns
+	}
+	return node, nil
 }

 func (n *showTenantNode) startExec(params runParams) error {


Following from the comment above, this method should then evaluate the typed Expr in a manner similar to resolveTenantID to get our hands on tree.DString which can then be converted to a roachpb.TenantName to be used below.

same as above.

pkg/sql/show_tenant.go

pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go

adityamaru · 2022-12-05T15:29:40Z

pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go

+	require.Equal(t, ingestionJobID, jobId)
+	require.Less(t, maxReplTime, timeutil.Now())
+	require.Less(t, protectedTime, timeutil.Now())
+	require.Greater(t, maxReplTime, testStartTime)


hmm I think a tighter check would be if require.Greater(t, maxReplTime, highWatermark of C2C we know we have reached)?

lidorcarmel

Thanks Aditya for the thorough review!

pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go

lidorcarmel · 2022-12-05T18:52:45Z

pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go

+	require.Equal(t, ingestionJobID, jobId)
+	require.Less(t, maxReplTime, timeutil.Now())
+	require.Less(t, protectedTime, timeutil.Now())
+	require.Greater(t, maxReplTime, testStartTime)


pkg/sql/show_tenant.go

pkg/sql/catalog/colinfo/result_columns.go

pkg/sql/show_tenant.go

lidorcarmel · 2022-12-05T21:51:04Z

pkg/sql/show_tenant.go

+	replicationInfo *streampb.StreamIngestionStats
+	pts             hlc.Timestamp
+	columns         colinfo.ResultColumns
+	done            bool
 }

 func (p *planner) ShowTenant(_ context.Context, n *tree.ShowTenant) (planNode, error) {


1 and 2 - Done.

re 3 and 4, let's do that later? I saw the TypedExpr thing but I prefer adding this when I can actually test subqueries, otherwise it feels like making things a bit more complicated without actual impact. if for example we would not want subqueries then I think the existing code is a bit easier to read compared to the TypedExpr thing.

lidorcarmel · 2022-12-05T21:52:00Z

pkg/sql/show_tenant.go

+	} else {
+		node.columns = colinfo.TenantColumns
+	}
+	return node, nil
 }

 func (n *showTenantNode) startExec(params runParams) error {


same as above.

pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go

adityamaru

Mostly LGTM, I think I only disagree about how we're doing expression evaluation currently. Even with that, if you'd rather check this in and fast follow once the datadriven framework works, I'm okay with that. I'm not worried about us never getting to this because we need to fix it for CREATE/DROP TENANT too.

adityamaru · 2022-12-06T13:56:32Z

pkg/sql/show_tenant.go

+	replicatedTimestamp := tree.DNull
+	retainedTimestamp := tree.DNull
+
+	// New replication clusters don't have replicationInfo initially.


q: what does this mean? I'm not sure I follow what case we're guarding against here.

dropped, it's not clear and repeating the comment above. See the comment and todo after calling mgr.GetStreamIngestionStats() above? hopefully this would make sense.

adityamaru · 2022-12-06T13:59:57Z

pkg/sql/show_tenant.go

+			return err
+		}
+		stats, err := mgr.GetStreamIngestionStats(params.ctx, info.TenantReplicationJobID)
+		// An error means we don't have stats but we can still present some info,


Why would we not have stats if we have an active replication job as we have asserted above by checking `TenantReplicationJobID?

good question, we have most stats but not all because we fail here:

cockroach/pkg/ccl/streamingccl/streamingest/stream_ingestion_job.go

Line 106 in 721cc76

client, err := streamclient.GetFirstActiveClient(ctx, progress.GetStreamIngest().StreamAddresses)

I think it will be nice to refactor and return what we can.

ahh I see, lets log the error if its non-nil?

adityamaru · 2022-12-06T14:04:13Z

pkg/sql/logictest/testdata/logic_test/tenant

@@ -60,6 +60,9 @@ id  name   status
 statement error tenant "seven" does not exist
 SHOW TENANT seven

+statement error tenant two does not have replication info


hmm I would expect the error we hit would indicate there is no active replication job rather than info, wdyt?

yep, that failed in ci, fixed now.

pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go

adityamaru · 2022-12-06T14:12:35Z

pkg/sql/show_tenant.go

+	replicationInfo *streampb.StreamIngestionStats
+	pts             hlc.Timestamp
+	columns         colinfo.ResultColumns
+	done            bool
 }

 func (p *planner) ShowTenant(_ context.Context, n *tree.ShowTenant) (planNode, error) {


It feels a little weird to me to be accepting an expression but then only using .String() when using the expression, instead of analyzing/asserting the type of the expression in a canonical manner. imo if we are using d_expr we should do the annoying work of evaluating the expression all the way through in the same diff. We can test subqueries, concat expressions etc. once we have the datadriven PR checked in, which is what we're doing for some of the other tests in the e2e file as well.

Alternatively, we can go back to using tree.Name and change this to d_expr when we change it for CREATE/DROP TENANT as well.

pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go

lidorcarmel

Thanks! PTAL.

pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go

lidorcarmel · 2022-12-06T23:42:31Z

pkg/sql/show_tenant.go

+	replicationInfo *streampb.StreamIngestionStats
+	pts             hlc.Timestamp
+	columns         colinfo.ResultColumns
+	done            bool
 }

 func (p *planner) ShowTenant(_ context.Context, n *tree.ShowTenant) (planNode, error) {


Done! definitely need to add tests for this, data driven.
PTAL.

pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go

lidorcarmel · 2022-12-06T23:45:37Z

pkg/sql/logictest/testdata/logic_test/tenant

@@ -60,6 +60,9 @@ id  name   status
 statement error tenant "seven" does not exist
 SHOW TENANT seven

+statement error tenant two does not have replication info


yep, that failed in ci, fixed now.

lidorcarmel · 2022-12-06T23:50:12Z

pkg/sql/show_tenant.go

+			return err
+		}
+		stats, err := mgr.GetStreamIngestionStats(params.ctx, info.TenantReplicationJobID)
+		// An error means we don't have stats but we can still present some info,


good question, we have most stats but not all because we fail here:

cockroach/pkg/ccl/streamingccl/streamingest/stream_ingestion_job.go

Line 106 in 721cc76

client, err := streamclient.GetFirstActiveClient(ctx, progress.GetStreamIngest().StreamAddresses)

I think it will be nice to refactor and return what we can.

adityamaru

Thanks for working through this! LGTM, we can add more tests soon.

I might be missing it, but I don't see a test with a hyphenated name.

adityamaru · 2022-12-07T16:19:17Z

pkg/sql/show_tenant.go

+			return err
+		}
+		stats, err := mgr.GetStreamIngestionStats(params.ctx, info.TenantReplicationJobID)
+		// An error means we don't have stats but we can still present some info,


ahh I see, lets log the error if its non-nil?

adityamaru · 2022-12-07T16:26:47Z

pkg/sql/show_tenant.go

+	}
+
+	var dummyHelper tree.IndexedVarHelper
+	strName := paramparse.UnresolvedNameToStrVal(n.Name)


do you know why we need this call to UnresolvedNameToStrVal instead of just passing in n.Name to analyzeExpr below? My understanding is that this method is only called in the context of SET clauses.

this one was indeed an annoying one, without this I get:

logic.go:3182: /private/var/tmp/_bazel_lidorcarmel/8fa5a2c4db0103c8ea8fcdb54466d4b1/sandbox/darwin-sandbox/11989/execroot/com_github_cockroachdb_cockroach/bazel-out/darwin-fastbuild/bin/pkg/sql/logictest/tests/fakedist-vec-off/fakedist-vec-off_test_/fakedist-vec-off_test.runfiles/com_github_cockroachdb_cockroach/pkg/sql/logictest/testdata/logic_test/tenant:30: SHOW TENANT system expected success, but found (42703) column "system" does not exist type_check.go:1433: in TypeCheck() logic.go:2856: pq: column "system" does not exist

is there a better way around this? I poked around a bit and other than calling tree.NewStrVal(tree.AsStringWithFlags(s, tree.FmtBareIdentifiers)) directly I'm not sure what is more appropriate.

Oh hmm good point, I think SHOW TENANT 'system' will work but I'm not sure what we should strive for here. SHOW TENANT system or SHOW TENANT 'system'. I was going with the latter in ALTER TENANT which also uses a d_expr.

interesting, I was definitely assuming we need SHOW TENANT system, like SHOW CREATE TABLE system.tenants (maybe there are better examples?).

lidorcarmel

for some reason I didn't push the hyphen test, it's pushed now.

lidorcarmel · 2022-12-07T17:20:52Z

pkg/sql/show_tenant.go

+			return err
+		}
+		stats, err := mgr.GetStreamIngestionStats(params.ctx, info.TenantReplicationJobID)
+		// An error means we don't have stats but we can still present some info,


lidorcarmel · 2022-12-07T17:48:22Z

pkg/sql/show_tenant.go

+	}
+
+	var dummyHelper tree.IndexedVarHelper
+	strName := paramparse.UnresolvedNameToStrVal(n.Name)


this one was indeed an annoying one, without this I get:

logic.go:3182: /private/var/tmp/_bazel_lidorcarmel/8fa5a2c4db0103c8ea8fcdb54466d4b1/sandbox/darwin-sandbox/11989/execroot/com_github_cockroachdb_cockroach/bazel-out/darwin-fastbuild/bin/pkg/sql/logictest/tests/fakedist-vec-off/fakedist-vec-off_test_/fakedist-vec-off_test.runfiles/com_github_cockroachdb_cockroach/pkg/sql/logictest/testdata/logic_test/tenant:30: SHOW TENANT system expected success, but found (42703) column "system" does not exist type_check.go:1433: in TypeCheck() logic.go:2856: pq: column "system" does not exist

is there a better way around this? I poked around a bit and other than calling tree.NewStrVal(tree.AsStringWithFlags(s, tree.FmtBareIdentifiers)) directly I'm not sure what is more appropriate.

pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go

Extending SHOW TENANT to also allow showing replication status which contains info such as the protected timestamp on the destination cluster and the source cluster name. Command output right after the destination cluster is created: ``` [email protected]:26257/defaultdb> show tenant dest5 with replication status; id | name | status | source_tenant_name | source_cluster_uri | replication_job_id | replicated_time | retained_time -----+-------+--------+--------------------+--------------------+--------------------+-----------------+---------------- 7 | dest5 | ADD | NULL | NULL | 819890711267737601 | NULL | NULL (1 row) ``` A bit later we have most stats (manually adjusting the source_cluster_uri): ``` [email protected]:26257/defaultdb> show tenant dest5 with replication status; id | name | status | source_tenant_name | source_cluster_uri | replication_job_id | replicated_time | retained_time -----+-------+--------+--------------------+-------------------------------------------------------+--------------------+-----------------+----------------------------- 7 | dest5 | ADD | src | postgresql://[email protected]:26257/defaultdb?ssl...crt | 819890711267737601 | NULL | 2022-12-05 23:00:04.516331 (1 row) ``` And a moment later the replication time is populated. Informs: cockroachdb#91261 Epic: CRDB-18749 Release note: None

lidorcarmel · 2022-12-08T18:45:01Z

TFTR Aditya!
bors r+

craig · 2022-12-08T20:15:39Z

Build succeeded:

Bazel Essential CI (Cockroach)

lidorcarmel requested review from stevendanna, adityamaru and a team November 28, 2022 23:05

lidorcarmel requested a review from a team as a code owner November 28, 2022 23:05

lidorcarmel force-pushed the lidor_show_tenant_with branch from a62463f to 4112186 Compare November 29, 2022 22:18

adityamaru suggested changes Dec 1, 2022

View reviewed changes

pkg/sql/parser/sql.y Outdated Show resolved Hide resolved

adityamaru self-requested a review December 1, 2022 17:24

adityamaru reviewed Dec 2, 2022

View reviewed changes

pkg/sql/catalog/colinfo/result_columns.go Outdated Show resolved Hide resolved

lidorcarmel force-pushed the lidor_show_tenant_with branch from 4112186 to f16a876 Compare December 2, 2022 22:04

adityamaru self-requested a review December 5, 2022 14:01

adityamaru suggested changes Dec 5, 2022

View reviewed changes

lidorcarmel force-pushed the lidor_show_tenant_with branch from f16a876 to ea05406 Compare December 6, 2022 05:08

lidorcarmel commented Dec 6, 2022

View reviewed changes

adityamaru suggested changes Dec 6, 2022

View reviewed changes

adityamaru self-requested a review December 6, 2022 14:14

adityamaru reviewed Dec 6, 2022

View reviewed changes

pkg/ccl/streamingccl/streamingest/stream_replication_e2e_test.go Outdated Show resolved Hide resolved

adityamaru self-requested a review December 6, 2022 14:17

lidorcarmel force-pushed the lidor_show_tenant_with branch from ea05406 to 029e8da Compare December 6, 2022 23:40

lidorcarmel commented Dec 6, 2022

View reviewed changes

adityamaru approved these changes Dec 7, 2022

View reviewed changes

lidorcarmel force-pushed the lidor_show_tenant_with branch from 029e8da to 4036a9c Compare December 7, 2022 17:50

lidorcarmel commented Dec 7, 2022

View reviewed changes

lidorcarmel force-pushed the lidor_show_tenant_with branch 2 times, most recently from 7a3319b to eeceee6 Compare December 8, 2022 18:38

lidorcarmel force-pushed the lidor_show_tenant_with branch from eeceee6 to dcd183c Compare December 8, 2022 18:44

craig bot merged commit fe683e3 into cockroachdb:master Dec 8, 2022

lidorcarmel deleted the lidor_show_tenant_with branch January 24, 2023 20:07

shermanCRL mentioned this pull request Jan 26, 2023

c2c: implement syntax #96022

Closed

sql: add SHOW TENANT name WITH REPLICATION STATUS #92628

sql: add SHOW TENANT name WITH REPLICATION STATUS #92628

Conversation

lidorcarmel commented Nov 28, 2022 • edited Loading

cockroach-teamcity commented Nov 30, 2022

adityamaru left a comment

Choose a reason for hiding this comment

lidorcarmel commented Dec 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidorcarmel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityamaru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidorcarmel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adityamaru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidorcarmel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidorcarmel commented Dec 8, 2022

craig bot commented Dec 8, 2022

lidorcarmel commented Nov 28, 2022 •

edited

Loading