sql: set locality to GLOBAL for materialized views #62122
Conversation
otan
requested review from
ajstorm,
a team and
miretskiy
and removed request for
a team
|
This change is |
|
Nice speedy work! |
There was a problem hiding this comment.
Reviewed 13 of 13 files at r1, 3 of 3 files at r2, 2 of 2 files at r3.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @miretskiy and @otan)
pkg/sql/catalog/tabledesc/validate.go, line 1253 at r2 (raw file):
// Check non-table items have a correctly set locality. if desc.IsSequence() { if !desc.IsLocalityRegionalByTable() {
Nit: Does it make sense here to create IsDefaultLocalitySequence and tree.DefaultLocalitySequence, so that we only have to change this in one place down the road?
There was a problem hiding this comment.
Reviewable status:
pkg/sql/create_sequence.go, line 91 at r1 (raw file):
func doCreateSequence( params runParams, context string,
mind removing this context? Looks greyed out
pkg/sql/create_sequence.go, line 98 at r1 (raw file):
opts tree.SequenceOptions, jobDesc string, isMultiRegion bool,
nit: you can get rid of this param and construct isMultiRegion in this function as opposed to funnelling it through to here.
pkg/sql/create_table.go, line 2419 at r1 (raw file):
} _, dbDesc, err := params.p.Descriptors().GetImmutableDatabaseByID(
Don't you need an AvoidCached version here?
There was a problem hiding this comment.
suggestions changed on base PR
Reviewable status:
pkg/sql/create_sequence.go, line 91 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
mind removing this context? Looks greyed out
Done.
pkg/sql/create_table.go, line 2419 at r1 (raw file):
Previously, arulajmani (Arul Ajmani) wrote…
Don't you need an
AvoidCachedversion here?
Done.
pkg/sql/catalog/tabledesc/validate.go, line 1253 at r2 (raw file):
Previously, ajstorm (Adam Storm) wrote…
Nit: Does it make sense here to create IsDefaultLocalitySequence and tree.DefaultLocalitySequence, so that we only have to change this in one place down the road?
hmm, i don't see the value of it so much right now as i imagine the way we go forwards from here is allowing users to configure it.
depends how strongly you feel. happy to do this in a follow up.
otan
force-pushed
the
materialised_views_mr
branch
from
dd1f652 to
45757fb
Compare
Release note (sql change): Materialized views in multi-region databases will now have a GLOBAL locality.
Release note (sql change): Materialized views which are in a database before the first ADD REGION will become GLOBAL on ADD REGION, in line with the behavior of CREATE MATERIALIZED VIEW on a multi-region database.
otan
force-pushed
the
materialised_views_mr
branch
from
45757fb to
f7bf627
Compare
|
bors r=ajstorm |
|
Build succeeded: |
Only last two commits matter.
See individual commits for details.
Refs: #61382