Skip to content

Commit

Permalink
config: propagate zone config info for non-gossiped system tables
Browse files Browse the repository at this point in the history
Before this PR we would allow setting zone configurations on system tables but
they would not propagate because the GetZoneConfig function would return the
zone config for the entire system database. This leaves us in a still weird
situation where some system tables will ignore their zone configs but not
others.

Fixes #39605.

Release note (bug fix): Propagate zone configuration to non-gossiped system
tables.
  • Loading branch information
ajwerner committed Aug 15, 2019
1 parent 02f239d commit ea56965
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 6 deletions.
1 change: 1 addition & 0 deletions pkg/cmd/roachtest/gossip.go
Original file line number Diff line number Diff line change
Expand Up @@ -350,6 +350,7 @@ func runGossipRestartNodeOne(ctx context.Context, t *test, c *cluster) {
run(`ALTER DATABASE system %[1]s CONFIGURE ZONE %[2]s 'constraints: {"-rack=0"}'`)
run(`ALTER RANGE meta %[1]s CONFIGURE ZONE %[2]s 'constraints: {"-rack=0"}'`)
run(`ALTER RANGE liveness %[1]s CONFIGURE ZONE %[2]s 'constraints: {"-rack=0"}'`)
run(`ALTER TABLE system.jobs %[1]s CONFIGURE ZONE %[2]s 'constraints: {"-rack=0"}'`)

This comment has been minimized.

Copy link
@andreimatei

andreimatei Sep 10, 2019

Contributor

Do you remember why this became necessary? How come system.jobs doesn't inherit from system (which is altered above)?
Asking because I just ran into this with other system tables.

This comment has been minimized.

Copy link
@ajwerner

ajwerner Sep 10, 2019

Author Contributor

Maybe see the GetZoneConfigForKey line 269 and below?

Which system tables in particular?


var lastReplCount int
if err := retry.ForDuration(2*time.Minute, func() error {
Expand Down
19 changes: 16 additions & 3 deletions pkg/config/system.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,11 +255,14 @@ func (s *SystemConfig) GetZoneConfigForKey(key roachpb.RKey) (*ZoneConfig, error
if !ok {
// Not in the structured data namespace.
objectID = keys.RootNamespaceID
} else if objectID <= keys.MaxReservedDescID {
// For now, you can only set a zone config on the system database as a whole,
// not on any of its constituent tables. This is largely because all the
} else if objectID <= keys.MaxSystemConfigDescID || isPseudoTableID(objectID) {
// For now, you cannot set the zone config on gossiped tables. The only
// way to set a zone config on these tables is to modify config for the
// system database as a whole. This is largely because all the
// "system config" tables are colocated in the same range by default and
// thus couldn't be managed separately.
// Furthermore pseudo-table ids should be considered to be a part of the
// system database as they aren't real tables.
objectID = keys.SystemDatabaseID
}

Expand All @@ -279,6 +282,16 @@ func (s *SystemConfig) GetZoneConfigForKey(key roachpb.RKey) (*ZoneConfig, error
return s.getZoneConfigForKey(objectID, keySuffix)
}

// isPseudoTableID returns true if id is in keys.PseudoTableIDs.
func isPseudoTableID(id uint32) bool {
for _, pseudoTableID := range keys.PseudoTableIDs {
if id == pseudoTableID {
return true
}
}
return false
}

// GetZoneConfigForObject returns the combined zone config for the given object
// identifier.
// NOTE: any subzones from the zone placeholder will be automatically merged
Expand Down
18 changes: 15 additions & 3 deletions pkg/config/system_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -402,11 +402,23 @@ func TestGetZoneConfigForKey(t *testing.T) {
{roachpb.RKey(keys.TimeseriesPrefix.PrefixEnd()), keys.SystemRangesID},
{roachpb.RKey(keys.TableDataMin), keys.SystemDatabaseID},
{roachpb.RKey(keys.SystemConfigSplitKey), keys.SystemDatabaseID},

// Gossiped system tables should refer to the SystemDatabaseID.
{tkey(keys.NamespaceTableID), keys.SystemDatabaseID},
{tkey(keys.ZonesTableID), keys.SystemDatabaseID},
{tkey(keys.LeaseTableID), keys.SystemDatabaseID},
{tkey(keys.JobsTableID), keys.SystemDatabaseID},
{tkey(keys.LocationsTableID), keys.SystemDatabaseID},

// Non-gossiped system tables should refer to themselves.
{tkey(keys.LeaseTableID), keys.LeaseTableID},
{tkey(keys.JobsTableID), keys.JobsTableID},
{tkey(keys.LocationsTableID), keys.LocationsTableID},

// Pseudo-tables should refer to the SystemDatabaseID.
{tkey(keys.MetaRangesID), keys.SystemDatabaseID},
{tkey(keys.SystemRangesID), keys.SystemDatabaseID},
{tkey(keys.TimeseriesRangesID), keys.SystemDatabaseID},
{tkey(keys.LivenessRangesID), keys.SystemDatabaseID},

// User tables should refer to themselves.
{tkey(keys.MinUserDescID), keys.MinUserDescID},
{tkey(keys.MinUserDescID + 22), keys.MinUserDescID + 22},
{roachpb.RKeyMax, keys.RootNamespaceID},
Expand Down
7 changes: 7 additions & 0 deletions pkg/storage/client_replica_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1783,6 +1783,13 @@ func TestSystemZoneConfigs(t *testing.T) {
testutils.SucceedsSoon(t, waitForReplicas)
log.Info(ctx, "TestSystemZoneConfig: down-replication of timeseries ranges succeeded")

// Up-replicate the system.jobs table to demonstrate that it is configured
// independently from the system database.
sqlutils.SetZoneConfig(t, sqlDB, "TABLE system.jobs", "num_replicas: 7")
expectedReplicas += 2
testutils.SucceedsSoon(t, waitForReplicas)
log.Info(ctx, "TestSystemZoneConfig: up-replication of jobs table succeeded")

// Finally, verify the system ranges. Note that in a new cluster there are
// two system ranges, which we have to take into account here.
sqlutils.SetZoneConfig(t, sqlDB, "RANGE system", "num_replicas: 7")
Expand Down

0 comments on commit ea56965

Please sign in to comment.