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

sql: sequence owner dependencies are poorly handled in BACKUP/RESTORE #50781

Closed
solongordon opened this issue Jun 29, 2020 · 0 comments · Fixed by #50949
Closed

sql: sequence owner dependencies are poorly handled in BACKUP/RESTORE #50781

solongordon opened this issue Jun 29, 2020 · 0 comments · Fixed by #50949
Assignees
@arulajmani
Labels
A-disaster-recovery C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@solongordon
Copy link
Contributor

It looks like restoring tables or sequences with OWNED BY relationship can lead to broken dependencies. For example, if you restore a table which is a sequence owner but its owned sequence no longer exists, this prevents dropping the table.

root@:26257/defaultdb> create database d1;
root@:26257/defaultdb> create database d2;
root@:26257/defaultdb> create table d1.t (x int);
root@:26257/defaultdb> create sequence d2.s owned by d1.t.x;
root@:26257/defaultdb> backup database d1 to 'nodelocal://self/d1';
root@:26257/defaultdb> drop database d1 cascade;
root@:26257/defaultdb> drop sequence d2.s;
root@:26257/defaultdb> restore database d1 from 'nodelocal://self/d1';
root@:26257/defaultdb> drop table d1.t;
ERROR: table is being dropped

I think we need to handle this the same way we do other dependencies like foreign keys: if a user tries to restore a sequence owner and the owned sequence no longer exists, the restore should error out. We can also add a skip_missing_sequence_owners flag which lets the user skip adding the missing dependency.
@solongordon solongordon added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-disaster-recovery labels Jun 29, 2020
arulajmani added a commit to arulajmani/cockroach that referenced this issue Jul 2, 2020
@arulajmani
backupccl: handle sequence ownership remapping logic for restore
1fc943e 
Previously, we would restore sequence descriptors/table descriptors as
is, without remapping the ownership dependency to the new IDs. This
meant sequence ownership was broken post restore. This PR fixes that.

When sequences with owners or tables that have columns that own
sequences are restored:
- If both the owned sequence and the owning table are being restored,
the ownership is remapped.
- If just the sequence is restored, the user receives an error to use
the `skip_missing_sequence_owners` flag. If the flag is provided, the
sequence is restored without any owner.
- If just the table is restored, the user receives an error to use the
`skip_missing_sequence_owners` flag. If the flag is provided, the table
is restored with the table column that previously owned the sequence no
longer owning that sequence.

Fixes cockroachdb#50781

Release note (enterprise change): Restore now has a new option
`skip_missing_sequence_owners` that must be supplied when restoring
only the table/sequence that was previously
a sequence owner/owned by a table.
Additionally, this fixes a bug where ownership relationships would not
be remapped after a restore.
@arulajmani arulajmani mentioned this issue Jul 2, 2020
Merged
arulajmani added a commit to arulajmani/cockroach that referenced this issue Jul 6, 2020
@arulajmani
backupccl: handle sequence ownership remapping logic for restore
106bd1d 
Previously, we would restore sequence descriptors/table descriptors as
is, without remapping the ownership dependency to the new IDs. This
meant sequence ownership was broken post restore. This PR fixes that.

When sequences with owners or tables that have columns that own
sequences are restored:
- If both the owned sequence and the owning table are being restored,
the ownership is remapped.
- If just the sequence is restored, the user receives an error to use
the `skip_missing_sequence_owners` flag. If the flag is provided, the
sequence is restored without any owner.
- If just the table is restored, the user receives an error to use the
`skip_missing_sequence_owners` flag. If the flag is provided, the table
is restored with the table column that previously owned the sequence no
longer owning that sequence.

Fixes cockroachdb#50781

Release note (enterprise change): Restore now has a new option
`skip_missing_sequence_owners` that must be supplied when restoring
only the table/sequence that was previously
a sequence owner/owned by a table.
Additionally, this fixes a bug where ownership relationships would not
be remapped after a restore.
craig bot pushed a commit that referenced this issue Jul 7, 2020
@arulajmani @tbg
@mgartner @darinpp
Merge #50949 #51005 #51037 #51071
6893785 
50949: backupccl: handle sequence ownership remapping logic for restore r=pbardea a=arulajmani

Previously, we would restore sequence descriptors/table descriptors as
is, without remapping the ownership dependency to the new IDs. This
meant sequence ownership was broken post restore. This PR fixes that.

When sequences with owners or tables that have columns that own
sequences are restored:
- If both the owned sequence and the owning table are being restored,
the ownership is remapped.
- If just the sequence is restored, the user receives an error to use
the `skip_missing_sequence_owners` flag. If the flag is provided, the
sequence is restored without any owner.
- If just the table is restored, the user receives an error to use the
`skip_missing_sequence_owners` flag. If the flag is provided, the table
is restored with the table column that previously owned the sequence no
longer owning that sequence.

Fixes #50781

Release note (enterprise change): Restore now has a new option
`skip_missing_sequence_owners` that must be supplied when restoring
only the table/sequence that was previously
a sequence owner/owned by a table.
Additionally, this fixes a bug where ownership relationships would not
be remapped after a restore.

51005: build: fix package reported by acceptance and compose r=otan a=tbg

The acceptance and compose tests were previously reporting `emptypkg` as
their package, which would be used to create issues.

We need to pass the "real" package to go so that it shows up in the
JSON output.

Closes #51003.

Release note: None

51037: sql: change volatility of timezone with TIME and TIMETZ to stable r=mgartner a=mgartner

sql: change volatility of timezone with TIME and TIMETZ to stable

The `timezone(STRING, TIME)` and `timezone(STRING, TIMETZ)` function
overloads were originally marked as volatile to copy Postgres's
volatility for the `timezone(STRING, TIMETZ)` overload (Postgres does
not implement `timezone (STRING, TIME)`.

Postgres's volatility settings for all overloads of `timezone` are
below.

       Name   |          Argument data types          | Volatility
    ----------+---------------------------------------+------------------
     timezone | interval, time with time zone         | immutable   zone
     timezone | interval, timestamp with time zone    | immutable
     timezone | interval, timestamp without time zone | immutable
     timezone | text, time with time zone             | volatile    zone
     timezone | text, timestamp with time zone        | immutable
     timezone | text, timestamp without time zone     | immutable

This single overload is singled out as volatile by Postgres because it
is dependent on C's `time(NULL)`. See the Postgres commit originally
marking this overload volatility, and the lines of code showing the
dependence:

postgres/postgres@35979e6
https://github.com/postgres/postgres/blob/ac3a4866c0bf1d7a14009f18d3b42ffcb063a7e9/src/backend/utils/adt/date.c#L2816

CRDB does not have the same issue. All `timezone` overloads have the
same volatility, stable. This commit marks them as such.

Additional context: #48756 (comment)

This commit also moves the `IgnoreVolatilityCheck` field from function
definitions to overloads. This allows overloads of the same function to
be ignored when asserting that the volalitity of an overload matches
Postgres's assigned volatility.

Release note: None


51071: roachtest: fix acceptance/decommission r=darinpp a=darinpp

The added --self flag makes the test fail with 20.1 and 19.2

With 20.1 the flag is not supported and so the expected
result should reflect that the step where a node decommissions self
will not happen.
With 19.2 the match that --self is not supported doesn't work
as the error message capitalization differs.

The fix changes the expected result and corrects the match of --self not
supported to work with 19.2 as well

Release note: None

Co-authored-by: arulajmani <[email protected]>
Co-authored-by: Tobias Schottdorf <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Darin <[email protected]>
@craig craig bot closed this as completed in d00c92f Jul 7, 2020
@arulajmani arulajmani mentioned this issue Jul 8, 2020
Closed
arulajmani added a commit to arulajmani/cockroach that referenced this issue Jul 21, 2020
@arulajmani
backupccl: handle sequence ownership remapping logic for restore
3fd4ec3 
Previously, we would restore sequence descriptors/table descriptors as
is, without remapping the ownership dependency to the new IDs. This
meant sequence ownership was broken post restore. This PR fixes that.

When sequences with owners or tables that have columns that own
sequences are restored:
- If both the owned sequence and the owning table are being restored,
the ownership is remapped.
- If just the sequence is restored, the user receives an error to use
the `skip_missing_sequence_owners` flag. If the flag is provided, the
sequence is restored without any owner.
- If just the table is restored, the user receives an error to use the
`skip_missing_sequence_owners` flag. If the flag is provided, the table
is restored with the table column that previously owned the sequence no
longer owning that sequence.

Fixes cockroachdb#50781

Release note (enterprise change): Restore now has a new option
`skip_missing_sequence_owners` that must be supplied when restoring
only the table/sequence that was previously
a sequence owner/owned by a table.
Additionally, this fixes a bug where ownership relationships would not
be remapped after a restore.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@arulajmani arulajmani

Labels
A-disaster-recovery C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

backupccl: handle sequence ownership remapping logic for restore arulajmani/cockroach
2 participants
@solongordon @arulajmani