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

schemachanger: refactor subzone config elements #133879

Merged
merged 4 commits into from
Nov 12, 2024

Conversation

annrpom
Copy link
Contributor

@annrpom annrpom commented Oct 30, 2024

config/zonepb: add subzoneSpan-related methods

This patch adds subzoneSpan-related methods that aid in configuring
subzone zcs in the declarative schemachanger.

Epic: none

Release note: None


schemachanger: extend Catalog interface to support subzone writes

This patch extends the Catalog interface to support getting and
writing a zone config. This is necessary for us to be able to get
the parent zone config, apply all subzone changes, and then write
to kv in immediateState.

Epic: none

Release note: None


schemachanger: refactor subzone config elements

This patch refactors how subzoneSpans are handled in the declarative schema changer. We now filter out subzoneSpans that do not relate to the index/partition zone config element it is nested within. This makes discarding these subzone elements feasible.

In order to support zone config changes where the target's subzone spans -- along with other targets' subzone spans -- are modified, we make those changes a side-effect. That is:

...
ALTER PARTITION default OF TABLE db.person CONFIGURE ZONE USING gc.ttlseconds = 1;
-> PartitionZoneConfig{default}

ALTER PARTITION australia OF TABLE db.person CONFIGURE ZONE USING gc.ttlseconds = 2;
-> PartitionZoneConfig{default}
and
-> PartitionZoneConfig{australia}

Informs: #133158
Epic: none

Release note: None


schemachanger: refactor add/drop zc elements

This patch separates marking zc elements as dropped from
the add direction in the builder as subzone configs will have
a mixed state of drop subzone + add side effects.

Epic: none

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@annrpom
Copy link
Contributor Author

annrpom commented Oct 30, 2024

TODO

  • sort spans correctly in updatesubzoneconfig
  • gather all subzone spans related to an idx/partition (forgot that it's not a 1:1 relation)

@annrpom annrpom force-pushed the dsc-subzone-refactor branch 2 times, most recently from d797557 to a3a76f2 Compare October 31, 2024 15:46
@annrpom
Copy link
Contributor Author

annrpom commented Oct 31, 2024

woof ok -- so this is almost there. the problem now is from a situation like

CREATE DATABASE db;
CREATE TABLE db.person (
name STRING,
country STRING,
birth_date DATE,
PRIMARY KEY (country, birth_date, name)
)
PARTITION BY LIST (country) (
PARTITION australia
VALUES IN ('AU', 'NZ')
PARTITION BY RANGE (birth_date)
(
PARTITION old_au VALUES FROM (minvalue) TO ('1995-01-01'),
PARTITION yung_au VALUES FROM ('1995-01-01') TO (maxvalue)
),
PARTITION north_america
VALUES IN ('US', 'CA')
PARTITION BY RANGE (birth_date)
(
PARTITION old_na VALUES FROM (minvalue) TO ('1995-01-01'),
PARTITION yung_na VALUES FROM ('1995-01-01') TO (maxvalue)
),
PARTITION default
VALUES IN (default)
);
----
translate database=db table=person
----
/Table/10{6-7} range default
exec-sql
ALTER PARTITION default OF TABLE db.person CONFIGURE ZONE USING gc.ttlseconds = 1;
----
exec-sql
ALTER PARTITION australia OF TABLE db.person CONFIGURE ZONE USING gc.ttlseconds = 2;
----

where just updating australia's subzone spans is not going to work since default's subzone spans will need to get reconfigured to cover the keyspace(?) that australia doesnt cover; we need to add support to configure pre-existing subzone configs' subzone spans as a side effect

so:

ALTER PARTITION default OF TABLE db.person CONFIGURE ZONE USING gc.ttlseconds = 1; will have

  • PartitionZoneConfig{default}

ALTER PARTITION australia OF TABLE db.person CONFIGURE ZONE USING gc.ttlseconds = 2; will have

  • PartitionZoneConfig{australia}
    and
  • PartitionZoneConfig{default} (just to update its subzone spans accordingly)

or something of the sort

pkg/config/zonepb/zone.go Outdated Show resolved Hide resolved
@annrpom annrpom force-pushed the dsc-subzone-refactor branch 5 times, most recently from 9a651ba to 566a80a Compare November 2, 2024 00:45
Copy link

blathers-crl bot commented Nov 2, 2024

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

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

@annrpom
Copy link
Contributor Author

annrpom commented Nov 2, 2024

arg i tried to do an optimization https://gist.github.com/annrpom/8a5012562c8e0682b8f995e6e3eb46c8

and it's ok, but not in transactions for some reason -- i left a TODO and put ^ gist up as a future task

@annrpom annrpom force-pushed the dsc-subzone-refactor branch 2 times, most recently from 28dde09 to 7e9a204 Compare November 4, 2024 00:38
@annrpom annrpom marked this pull request as ready for review November 4, 2024 14:43
@annrpom annrpom requested review from a team as code owners November 4, 2024 14:43
@annrpom annrpom requested a review from fqazi November 4, 2024 14:43
Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @annrpom, this is coming along nicely.

Reviewed 6 of 14 files at r1, 23 of 23 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @annrpom)


pkg/sql/schemachanger/scbuild/internal/scbuildstmt/zone_config_helpers.go line 1220 at r2 (raw file):

// for a lookup of which subzone spans a particular subzone is referred to by.
func getAffectedSubzoneSpansWithIdx(
	subzonesSlice int, newSubzoneSpans []zonepb.SubzoneSpan,

Nit: maybe call subzoneSlices something like numSubZones (or something more descriptive)


pkg/sql/schemachanger/scexec/exec_immediate_mutation.go line 239 at r2 (raw file):

	for _, zcToUpdate := range s.modifiedZoneConfigs {
		if zcToUpdate.isSubzoneConfig {
			// TODO(before merge): unsatisfied with this; would it be better to

Yeah it might be cleaner to eliminate the overload

@annrpom annrpom force-pushed the dsc-subzone-refactor branch 2 times, most recently from b2f189e to c9b4a45 Compare November 4, 2024 16:13
Copy link
Contributor Author

@annrpom annrpom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)


pkg/sql/schemachanger/scbuild/internal/scbuildstmt/zone_config_helpers.go line 1220 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Nit: maybe call subzoneSlices something like numSubZones (or something more descriptive)

done -- the naming of the method was also a remnant of something i tried doing that didn't pan out; so, i changed that as well


pkg/sql/schemachanger/scexec/exec_immediate_mutation.go line 239 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Yeah it might be cleaner to eliminate the overload

done oki

Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This look great @annrpom. Some very minor comments, but all of this much neater.

Reviewed 29 of 29 files at r4, 29 of 29 files at r5, 7 of 7 files at r6, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @annrpom)


pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go line 91 at r6 (raw file):

			return
		}
		toDrop, sideEffects := zco.getZoneConfigElemForDrop(b)

Nit: Use a more informative name here for sideEffects


pkg/sql/schemachanger/scbuild/internal/scbuildstmt/partition_zone_config.go line 104 at r4 (raw file):

		zc, err = pzo.getInheritedDefaultZoneConfig(b)
	} else {
		//zc, err = pzo.tableZoneConfigObj.getZoneConfig(b, false /* inheritDefaultRange */)

Nit: Clean up


pkg/config/zonepb/zone.go line 1186 at r4 (raw file):

// DeleteSubzoneSpans deletes all given SubzoneSpan from z.
func (z *ZoneConfig) DeleteSubzoneSpans(spansToDelete []SubzoneSpan) {
	toDeleteMap := make(map[string]bool, len(spansToDelete))

Nit: You could just make map[string]struct{}

This patch adds subzoneSpan-related methods that aid in configuring
subzone zcs in the declarative schemachanger.

Epic: none

Release note: None
This patch extends the `Catalog` interface to support getting and
writing a zone config. This is necessary for us to be able to get
the parent zone config, apply all subzone changes, and then write
to kv in `immediateState`.

Epic: none

Release note: None
This patch refactors how subzoneSpans are handled in the declarative schema
changer. We now filter out subzoneSpans that do not relate to the
index/partition zone config element it is nested within. This makes discarding
these subzone elements feasible.

In order to support zone config changes where the target's subzone spans --
along with other targets' subzone spans -- are modified, we make those changes
a side-effect. That is:

```
...
ALTER PARTITION default OF TABLE db.person CONFIGURE ZONE USING gc.ttlseconds = 1;
-> PartitionZoneConfig{default}

ALTER PARTITION australia OF TABLE db.person CONFIGURE ZONE USING gc.ttlseconds = 2;
-> PartitionZoneConfig{default}
and
-> PartitionZoneConfig{australia}
```

Informs: cockroachdb#133158
Epic: none

Release note: None
This patch separates marking zc elements as dropped from
the add direction in the builder as subzone configs will have
a mixed state of drop subzone + add side effects.

Epic: none

Release note: None
Copy link
Contributor Author

@annrpom annrpom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed (offline), I added 1 more change: we now gather all subzone config changes on a table zone config before writing it to kv. this prevents the span config watcher from observing partial states of subzone configurations (in particular: partial states of subzone span changes), which could lead to a panic in the span config translator

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)


pkg/config/zonepb/zone.go line 1186 at r4 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Nit: You could just make map[string]struct{}

done oop yeah save mem; ty


pkg/sql/schemachanger/scbuild/internal/scbuildstmt/configure_zone.go line 91 at r6 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Nit: Use a more informative name here for sideEffects

done -- added a blurb as well


pkg/sql/schemachanger/scbuild/internal/scbuildstmt/partition_zone_config.go line 104 at r4 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Nit: Clean up

done oop ty

@annrpom
Copy link
Contributor Author

annrpom commented Nov 12, 2024

TFTR! ('-')7

bors r+

@craig craig bot merged commit deb2445 into cockroachdb:master Nov 12, 2024
23 checks passed
craig bot pushed a commit that referenced this pull request Nov 12, 2024
134724: metric: truncate help text in prom output r=arjunmahishi a=dhartunian

We have some long multiline help text defined for metrics that makes the prometheus output quite large. This change automatically truncates help text at the first newline during prometheus output. We can still access the full text in our docs and code.

Resolves: CRDB-43497
Epic: None

Release note (ops change): the metrics scrape HTTP endpoint at `/ _status/vars` will now truncate HELP text at the first newline, reducing the metadata for metrics with large descriptions. Customers can still access these descriptions via our docs.

134902: tablemetadatacache,ui: change error messaging for failed tmj update r=kyle-a-wong a=kyle-a-wong

Previously, error messages from the table metaddata update job were surfaced in the UI on the table overview page. These errors are usually overly verbose and don't provide any actionable info to the end user.

In addition to this, error messages can be very big if the error is a result of a span stats fanout failure, which can contain errors for each fanned out request.

Now, db-console will simply report that an error occured in the job and that the data is stale. The error returned from failed SpanStats requests will now have a generic "An error has occurred while fetching span stats."

Epic: None
Release note: None

Old error messaging:
<img width="338" alt="image" src="https://github.com/user-attachments/assets/382ce140-3778-48f4-a300-fe5fcaed5faa">

New error messaging:
<img width="434" alt="image" src="https://github.com/user-attachments/assets/21df4f80-dd0e-4d95-87a1-a64752b0d867">


134955: kvserver/rangefeed: introduce Disconnector interface  r=stevendanna a=wenyihu6

This patch introduces the `Disconnector` interface, implemented by the
`registration` interface. Although currently unused, future commits will pass this
interface to `node.MuxRangefeed`, allowing disconnection of individual
registrations at the node level.

Part of: #110432
Release note: none

Co-authored-by: Steven Danna danna@cockroachlabs.com

134979: sql: avoid role existence check in DROP ROLE when it's not necessary r=rafiss a=rafiss

We only need to make the RoleExists call when the `IF EXISTS` clause is used.

This check was recently added in 696869a.

informs: #134538
Release note: None

135000: Revert "schemachanger: refactor subzone config elements" r=annrpom a=annrpom

Reverts #133879

Epic: none

Release note: None

Co-authored-by: David Hartunian <davidh@cockroachlabs.com>
Co-authored-by: Kyle Wong <37189875+kyle-a-wong@users.noreply.github.com>
Co-authored-by: Wenyi Hu <wenyi@cockroachlabs.com>
Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Co-authored-by: Annie Pompa <annie@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants