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.

Sign up for GitHub

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

Jump to bottom

importccl: IMPORT INTO should support gateway_region builtin default expression #69616

Closed
adityamaru opened this issue Aug 30, 2021 · 3 comments · Fixed by #69903
Closed

importccl: IMPORT INTO should support gateway_region builtin default expression #69616

adityamaru opened this issue Aug 30, 2021 · 3 comments · Fixed by #69903
Assignees
@adityamaru
Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker T-disaster-recovery

Comments

@adityamaru
Copy link
Contributor

adityamaru commented Aug 30, 2021 •
edited by exalate-issue-sync bot
Loading

To unlock IMPORT INTO for regional by row tables, it needs to be able to handle the default expression on the crdb_region column which is a call to the crdb builtin gateway_region. This builtin is volatile depending on the gateway node the import job is running on, and thus we do cannot support it as is, since it breaks the idempotency invariant of import execution.

To work around this volatility we can override the gateway_region default expression to always return the primary region of the database that was resolved during import planning. This is similar to what we do during the backfill of the column when altering a table's locality to RBR. Evaluating the default expr to the primary region also seems like the "more correct" thing to do since an import job is not necessarily associated with a gateway node, it is a background process ingesting data.

Today, an IMPORT INTO will fail during type resolution of the underlying UDT of crdb_region, but once #61133 (comment) is addressed we will not run into that particular hurdle.

Epic CRDB-7074
@adityamaru adityamaru added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-disaster-recovery labels Aug 30, 2021
@adityamaru adityamaru changed the title importccl: IMPORT INTO should support gateway_region builtin default expressions importccl: IMPORT INTO should support gateway_region builtin default expression Aug 30, 2021
@adityamaru adityamaru mentioned this issue Sep 1, 2021
Merged
adityamaru added a commit to adityamaru/cockroach that referenced this issue Sep 7, 2021
@adityamaru
importccl: add support for IMPORT INTO RBR table
d3ac3fc 
This change overrides the `default_to_database_primary_region`
and `gateway_region` to always return the primary region of the
database of the table being imported into. This allows for
IMPORT INTO an RBR table.

Since IMPORT is a job, it does not have an associated session data
and so it cannot rely on the planners' implementation of the regional
operator. This change also implements the relevant methods in the
`importRegionOperator` to allow resolution of the primary region
of the database being imported into.

Fixes: cockroachdb#69616

Release note (sql change): IMPORT INTO regional by row tables
is supported.

Release justification: fixes for high-priority or high-severity bugs in existing functionality
@adityamaru adityamaru mentioned this issue Sep 7, 2021
Merged
adityamaru added a commit to adityamaru/cockroach that referenced this issue Sep 10, 2021
@adityamaru
importccl: add support for IMPORT INTO RBR table
d0d70dc 
This change overrides the `default_to_database_primary_region`
and `gateway_region` to always return the primary region of the
database of the table being imported into. This allows for
IMPORT INTO an RBR table.

Since IMPORT is a job, it does not have an associated session data
and so it cannot rely on the planners' implementation of the regional
operator. This change also implements the relevant methods in the
`importRegionOperator` to allow resolution of the primary region
of the database being imported into.

Fixes: cockroachdb#69616

Release note (sql change): IMPORT INTO regional by row tables
is supported.

Release justification: fixes for high-priority or high-severity bugs in existing functionality
adityamaru added a commit to adityamaru/cockroach that referenced this issue Sep 10, 2021
@adityamaru
importccl: add support for IMPORT INTO RBR table
b78d0c5 
This change overrides the `default_to_database_primary_region`
and `gateway_region` to always return the primary region of the
database of the table being imported into. This allows for
IMPORT INTO an RBR table.

Since IMPORT is a job, it does not have an associated session data
and so it cannot rely on the planners' implementation of the regional
operator. This change also implements the relevant methods in the
`importRegionOperator` to allow resolution of the primary region
of the database being imported into.

Fixes: cockroachdb#69616

Release note (sql change): IMPORT INTO regional by row tables
is supported.

Release justification: fixes for high-priority or high-severity bugs in existing functionality
adityamaru added a commit to adityamaru/cockroach that referenced this issue Sep 10, 2021
@adityamaru
importccl: add support for IMPORT INTO RBR table
7e152cb 
This change overrides the `default_to_database_primary_region`
and `gateway_region` to always return the primary region of the
database of the table being imported into. This allows for
IMPORT INTO an RBR table.

To ensure that the import is idempotent across resumptions, we cache
the primary region of the database being imported into, during planning.
This information is store in the job details and flow spec to be used
when evaluating the relevant default expr/computed column

Since IMPORT is a job, it does not have an associated session data
and so it cannot rely on the planners' implementation of the regional
operator. This change also implements the relevant methods in the
`importRegionOperator` to allow resolution of the primary region
of the database being imported into.

Fixes: cockroachdb#69616

Release note (sql change): IMPORT INTO regional by row tables
is supported.

Release justification: fixes for high-priority or high-severity bugs in existing functionality
@adityamaru adityamaru added the release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. label Sep 13, 2021
@blathers-crl
Copy link

blathers-crl bot commented Sep 13, 2021

Hi @adityamaru, please add branch-* labels to identify which branch(es) this release-blocker affects.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

adityamaru added a commit to adityamaru/cockroach that referenced this issue Sep 15, 2021
@adityamaru
importccl: add support for IMPORT INTO RBR table
d20c0e7 
This change overrides the `default_to_database_primary_region`
and `gateway_region` to always return the primary region of the
database of the table being imported into. This allows for
IMPORT INTO an RBR table.

To ensure that the import is idempotent across resumptions, we cache
the primary region of the database being imported into, during planning.
This information is store in the job details and flow spec to be used
when evaluating the relevant default expr/computed column

Since IMPORT is a job, it does not have an associated session data
and so it cannot rely on the planners' implementation of the regional
operator. This change also implements the relevant methods in the
`importRegionOperator` to allow resolution of the primary region
of the database being imported into.

Fixes: cockroachdb#69616

Release note (sql change): IMPORT INTO regional by row tables
is supported.

Release justification: fixes for high-priority or high-severity bugs in existing functionality
@adityamaru adityamaru mentioned this issue Sep 15, 2021
Merged
@adityamaru adityamaru added GA-blocker and removed release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. labels Sep 15, 2021
craig bot pushed a commit that referenced this issue Sep 15, 2021
@ajwerner @adityamaru
@Santamaura @maryliag @irfansharif
Merge #68282 #69903 #70150 #70226 #70229 #70262 #70268
f278e64 
68282: cli,log: allow use of `debug merge-logs` on older logs r=knz a=ajwerner

Fixes [#68278](#68278).

Log parsers require the flag `--format` when parsing older logs (because 
they do not contain format specification). With this patch, this is no longer 
a requirement as the log format is now inferred based on the structure of 
the log if no log format specification exists.

Release justification: bug fix

Release note (bug fix): The debug merge-logs command no longer returns an error 
when the log decoder attempts to parse older logs.

69903: importccl: add support for IMPORT INTO RBR table r=arulajmani,ajstorm,dt a=adityamaru

This change overrides the `default_to_database_primary_region`
and `gateway_region` to always return the primary region of the
database of the table being imported into. This allows for
IMPORT INTO an RBR table.

To ensure that the import is idempotent across resumptions, we cache
the primary region of the database being imported into, during planning.
This information is store in the job details and flow spec to be used
when evaluating the relevant default expr/computed column.

Since IMPORT is a job, it does not have an associated session data
and so it cannot rely on the planners' implementation of the regional
operator. This change also implements the relevant methods in the
`importRegionOperator` to allow resolution of the primary region
of the database being imported into.

Fixes: #69616

Release note (sql change): IMPORT INTO regional by row tables
is supported.

Release justification: fixes for high-priority or high-severity bugs in existing functionality

70150: server: fix TestAdminAPIJobs failure r=knz a=adityamaru

This change sorts the expected job IDs before ensuring
that they are equal.

Fixes: #69401

Release note: None

70226: changefeedccl: updated retryable error warning message r=wongio123 a=wongio123

Retryable error warning message contained the word "error"
Confusing to users because warning message had the word "error" in it
Prefaced warning message with "WARNING"

Release note (enterprise change): updated retyable error warning message to begin with "WARNING"

Closes #69677 

70229: [CRDB-9016] ui: fix drag to zoom on custom charts r=Santamaura a=Santamaura

This PR addresses the issue where a user creates a custom chart and selects an area to zoom into which leaves the grey highlight after the graph zooms in. This was due to the history prop not being passed into the linegraph component and caused an error to throw when updating the url params. This was resolved by passing in the history to propagate to the
linegraph component.

Release note (ui change): fix drag to zoom on custom charts

https://user-images.githubusercontent.com/17861665/133342585-d7b37e9b-7eb8-4a48-b2c5-814fed62556a.mp4



70262: ui: add column selector to transation page r=maryliag a=maryliag

Add column selector to Transaction Page

Fixes #70148

<img width="414" alt="Screen Shot 2021-09-15 at 11 28 56 AM" src="https://user-images.githubusercontent.com/1017486/133463202-7ed7ac3a-9614-4101-ad76-8f431defe688.png">


Release justification: Category 4
Release note (ui change): Add column selector to transaction page

70268: clusterversion: remove Start21_1 (no longer applicable) r=irfansharif a=irfansharif

Fixes #65200. The last remaining 21.1 version (V21_1) can be removed as
part of #69828.

Release note: None

Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Aditya Maru <[email protected]>
Co-authored-by: Alex Wong <[email protected]>
Co-authored-by: Santamaura <[email protected]>
Co-authored-by: Marylia Gutierrez <[email protected]>
Co-authored-by: irfan sharif <[email protected]>
@craig craig bot closed this as completed in fb180a9 Sep 15, 2021
@adityamaru
Copy link
Contributor Author

Reopening until the backport is merged.

@adityamaru adityamaru reopened this Sep 15, 2021
@adityamaru
Copy link
Contributor Author

Backport has been merged.

@livlobo livlobo mentioned this issue Oct 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@adityamaru adityamaru

Labels
C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. GA-blocker T-disaster-recovery
Projects
No open projects
Disaster Recovery Backlog
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

importccl: add support for IMPORT INTO RBR table adityamaru/cockroach
1 participant
@adityamaru