catalog: add support for replicating catalogs for reader virtual clusters #130184

fqazi · 2024-09-05T18:47:50Z

This patch adds logic for extracting and creating a PCR reader catalog. This patch will do the following:

Introduce a new catalog function catalog.SetupOrAdvanceStandbyReaderCatalog which can be used to advanced or setup a reader catalog from a source tenant.
Add logic to allow read only sequence access, so that SQL behaves correctly

Fixes: #129439

Note: The first commit in this PR can be ignored, and will be merged via a separate PR #130183

cockroach-teamcity · 2024-09-05T18:48:03Z

This change is

msbutler

this looks great! probably want a foundations reviewer on this as well.

msbutler · 2024-09-06T12:36:50Z

pkg/sql/catalog/replication/reader_catalog.go

+)
+
+// SetupOrAdvanceStandbyReaderCatalog sets up a reader catalog that reads from
+// that tenant as of the passed timestamp -- potentially replacing the current


i think this docstring could be clearer. The function scrapes the descriptors AOST from the passed in fromID, and updates the descriptors of the calling tenant.

msbutler · 2024-09-06T12:39:52Z

pkg/sql/catalog/replication/reader_catalog.go

+	}
+	return descsCol.DescsTxn(
+		ctx, func(ctx context.Context, txn descs.Txn) error {
+			// Descriptors that no longer exist


stray comment?

msbutler · 2024-09-06T13:42:12Z

pkg/sql/catalog/replication/reader_catalog.go

+					if existingRawBytes != nil {
+						t.Table.Version = existingDesc.GetVersion()
+						mutBuilder = existingDesc.NewBuilder().(tabledesc.TableDescriptorBuilder)
+						newDescBytes, err := protoutil.Marshal(t.Table)


it's kinda sad that we have to marshal then unmarshall t.Table just to place it into the mutBuilder's Table Descriptor. I don't know if i can suggest a better solution, as you're the expert on this stuff.

msbutler · 2024-09-06T13:47:54Z

pkg/sql/catalog/replication/reader_catalog.go

+					// view definition will be wiped.
+					if mutTbl.IsPhysicalTable() {
+						mutTbl.ViewQuery = ""
+						mutTbl.SetExternalRowData(&descpb.ExternalRowData{TenantID: fromID, TableID: desc.GetID(), AsOf: asOf})


why don't we set external row data for non physical tables? will reads to those naturally reroute to physical tables which have external row data set?

msbutler · 2024-09-06T13:55:06Z

pkg/sql/catalog/replication/reader_catalog.go

+			// Resolve any existing descriptors within the tenant, which
+			// will be use to compute old values for writing.
+			b := txn.KV().NewBatch()
+			if err := extracted.ForEachDescriptor(func(desc catalog.Descriptor) error {


nit: i think the logic would be easier to follow if desc were renamed fromDesc, given that you also call ForEachDescriptor on allExistingDescs below

pkg/sql/catalog/replication/reader_catalog.go

pkg/sql/catalog/replication/reader_catalog_test.go

fqazi

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @azhu-crl, @msbutler, and @stevendanna)

pkg/sql/catalog/replication/reader_catalog.go line 35 at r2 (raw file):

Previously, msbutler (Michael Butler) wrote…

i think this docstring could be clearer. The function scrapes the descriptors AOST from the passed in fromID, and updates the descriptors of the calling tenant.

Good point, updated this comment to:
// SetupOrAdvanceStandbyReaderCatalog when invoked inside the reader
// tenant will replicate the descriptors from the tenant specified
// by fromID. The replicated descriptors will be setup such that they
// will access data from fromID. If the descriptors are already replicated
// then this function will advance the timestamp.

pkg/sql/catalog/replication/reader_catalog.go line 56 at r2 (raw file):

Previously, msbutler (Michael Butler) wrote…

stray comment?

More of an incomplete thought:

		// Track which descriptors / namespaces that have been updated, 
		// the difference between any existing tenant in the reader
		// catalog will be deleted (i.e. this are descriptors that exist
		// in the reader tenant, but not in the from tenant which we are 
		// replicating).

pkg/sql/catalog/replication/reader_catalog.go line 66 at r2 (raw file):

Previously, msbutler (Michael Butler) wrote…

nit: i think the logic would be easier to follow if desc were renamed fromDesc, given that you also call ForEachDescriptor on allExistingDescs below

Good point, let me change this.

Done.

pkg/sql/catalog/replication/reader_catalog.go line 83 at r2 (raw file):

Previously, msbutler (Michael Butler) wrote…

^ actually, my check above :
existingDesc.GetParentID()  == desc.GetParentID()
could lead to false positives if the system database id is not hard coded at start up

Done.

Good point. System database ID is fixed and hard coded. So, we can just confirm that it matches.

pkg/sql/catalog/replication/reader_catalog.go line 100 at r2 (raw file):

Previously, msbutler (Michael Butler) wrote…

it's kinda sad that we have to marshal then unmarshall t.Table just to place it into the mutBuilder's Table Descriptor. I don't know if i can suggest a better solution, as you're the expert on this stuff.

Done.

Could just do protoutil.Clone instead, let me do that.

pkg/sql/catalog/replication/reader_catalog.go line 118 at r2 (raw file):

Previously, msbutler (Michael Butler) wrote…

why don't we set external row data for non physical tables? will reads to those naturally reroute to physical tables which have external row data set?

Yeah, for views the reads will go through the descriptors selected by those tables.

pkg/sql/catalog/replication/reader_catalog.go line 188 at r2 (raw file):

Previously, msbutler (Michael Butler) wrote…

perhaps we default error on an unrecognized type?

Done.

pkg/sql/catalog/replication/reader_catalog_test.go line 91 at r2 (raw file):

Previously, msbutler (Michael Butler) wrote…

could you add a few test cases, where you rerun SetupOrAdvanceStandbyReaderCatalog with a new AOST after:

insert into t1

create a new table and insert data into it

conduct some schema change on it (e.g. add column in it)

drop that table new table

I have more tests in the follow on PR...but let me just add these here.

msbutler

Looks awesome! I'll let you decide if you want a pair of sql-foundations eyes on this before you merge.

msbutler · 2024-09-10T17:12:58Z

pkg/sql/catalog/replication/reader_catalog.go

+		ctx, func(ctx context.Context, txn descs.Txn) error {
+			// Track which descriptors / namespaces that have been updated,
+			// the difference between any existing tenant in the reader
+			// catalog will be deleted (i.e. this are descriptors that exist


s/this/these/r

msbutler · 2024-09-10T17:22:51Z

pkg/sql/catalog/replication/reader_catalog_test.go

+	compareConn("SELECT * FROM v1 ORDER BY 1")
+	compareConn("SELECT * FROM t2 ORDER BY n")
+
+	// Validate that systme tables are synced


nit: s/systme/system/r

To add support for replicating catalogs between tenants to support PCR this patch will introduce a new public API SetupOrAdvanceStandbyReaderCatalog, which will copy descriptors and set external data as needed. These schema objects then can be used for read only operations. Additionally, tables will be turned into materialized vies with this change, so that DML cannot be executed on them. Fixes: cockroachdb#129439 Release note: None

Previously, the only physical table types that were read only were materialized viewed. As a part of the PCR work to support reading from a tenant we added support for replicating catalogs and inserting them w ith external data rows. We apply the same idea to sequences, however we are lacking runtime support. To address this, this patch makes sequence opertions read only if external data rows exists. Release note: None

fqazi

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @azhu-crl, @msbutler, and @stevendanna)

pkg/sql/catalog/replication/reader_catalog.go line 54 at r7 (raw file):

Previously, msbutler (Michael Butler) wrote…

s/this/these/r

Done.

fqazi · 2024-09-10T20:04:14Z

@msbutler TFTR!

bors r+

craig · 2024-09-10T20:39:51Z

Build succeeded:

fqazi requested review from a team as code owners September 5, 2024 18:47

fqazi requested review from a team and stevendanna and removed request for a team September 5, 2024 18:51

fqazi force-pushed the fixCrossTenantSharedInitial branch from 3d5550b to 4e03571 Compare September 5, 2024 18:55

msbutler requested review from msbutler and azhu-crl September 5, 2024 19:00

fqazi force-pushed the fixCrossTenantSharedInitial branch from 4e03571 to f7f8b25 Compare September 5, 2024 19:38

msbutler reviewed Sep 6, 2024

View reviewed changes

fqazi force-pushed the fixCrossTenantSharedInitial branch from f7f8b25 to dbd9523 Compare September 10, 2024 14:17

fqazi commented Sep 10, 2024

View reviewed changes

fqazi force-pushed the fixCrossTenantSharedInitial branch from dbd9523 to ba833e9 Compare September 10, 2024 14:47

msbutler approved these changes Sep 10, 2024

View reviewed changes

fqazi added 2 commits September 10, 2024 18:29

fqazi force-pushed the fixCrossTenantSharedInitial branch from ba833e9 to db05204 Compare September 10, 2024 18:29

fqazi commented Sep 10, 2024

View reviewed changes

craig bot merged commit c63a63b into cockroachdb:master Sep 10, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

catalog: add support for replicating catalogs for reader virtual clusters #130184

catalog: add support for replicating catalogs for reader virtual clusters #130184

fqazi commented Sep 5, 2024

cockroach-teamcity commented Sep 5, 2024

msbutler left a comment

msbutler Sep 6, 2024

msbutler Sep 6, 2024

msbutler Sep 6, 2024

msbutler Sep 6, 2024

msbutler Sep 6, 2024

fqazi left a comment

msbutler left a comment

msbutler Sep 10, 2024

msbutler Sep 10, 2024

fqazi left a comment

fqazi commented Sep 10, 2024

craig bot commented Sep 10, 2024

catalog: add support for replicating catalogs for reader virtual clusters #130184

catalog: add support for replicating catalogs for reader virtual clusters #130184

Conversation

fqazi commented Sep 5, 2024

cockroach-teamcity commented Sep 5, 2024

msbutler left a comment

Choose a reason for hiding this comment

msbutler Sep 6, 2024

Choose a reason for hiding this comment

msbutler Sep 6, 2024

Choose a reason for hiding this comment

msbutler Sep 6, 2024

Choose a reason for hiding this comment

msbutler Sep 6, 2024

Choose a reason for hiding this comment

msbutler Sep 6, 2024

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

msbutler left a comment

Choose a reason for hiding this comment

msbutler Sep 10, 2024

Choose a reason for hiding this comment

msbutler Sep 10, 2024

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

fqazi commented Sep 10, 2024

craig bot commented Sep 10, 2024