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

sql: create an admin owned trash database for dropped tables #26476

Closed
benesch opened this issue Jun 6, 2018 · 10 comments · Fixed by #96217
Closed

sql: create an admin owned trash database for dropped tables #26476

benesch opened this issue Jun 6, 2018 · 10 comments · Fixed by #96217
Assignees
Labels
A-schema-changes C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) GA-blocker O-postmortem Originated from a Postmortem action item. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@benesch
Copy link
Contributor

benesch commented Jun 6, 2018

If you forget to change a table's GC TTL before you drop it, e.g via.

ALTER TABLE foo EXPERIMENTAL CONFIGURE ZONE '{gc: {ttlseconds: 30}}'

you won't ever be able to change it after DROP TABLE foo. You just can't name the table.

ALTER TABLE foo EXPERIMENTAL CONFIGURE ZONE '{gc: {ttlseconds: 30}}'
pq: relation "foo" does not exist

🤦‍♂️

I think @knz might have plans to fix this by allowing you to target tables by ID in DDL statements. Regardless, we'll need to do some work even after we have the syntax to make sure ALTER TABLE ... EXPERIMENTAL CONFIGURE ZONE doesn't panic if it loads a dropped table desc.

Jira issue: CRDB-5008

@benesch benesch added A-bulkio-schema-changes A-schema-descriptors Relating to SQL table/db descriptor handling. labels Jun 6, 2018
@bdarnell
Copy link
Contributor

bdarnell commented Jun 6, 2018

As an alternative to targeting by ID in ALTER TABLE, we could have an undelete/undrop command to bring the table back from its "trash" state and give it a name again.

Also, in either case, we'll need to make sure there's some way to discover the ID to use.

@vivekmenezes
Copy link
Contributor

good catch @benesch . The TestDropTableDeleteData test uses

if _, err := sqlDB.Exec(`INSERT INTO system.zones VALUES ($1, $2)`, descs[i].ID, buf); err != nil {
			t.Fatal(err)
		}

but that facility is only available to root.

@knz
Copy link
Contributor

knz commented Jun 8, 2018

My personal preference would be to have DROP of anything actually move the thing to a root-owned "trash" database, where it can be renamed from, until the data deletion actually completes (then the descriptor can be removed).

@knz
Copy link
Contributor

knz commented Jun 8, 2018

Filed #26543 for per-ID access.

@vivekmenezes vivekmenezes added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-schema-changes and removed A-schema-descriptors Relating to SQL table/db descriptor handling. A-bulkio-schema-changes labels Jul 23, 2018
@vivekmenezes vivekmenezes changed the title sql: can't change table's GC TTL after its dropped sql: create an admin owned trash database for dropped tables Oct 31, 2018
@vivekmenezes
Copy link
Contributor

The goal here is to change DROP to rename the table to trash/id where id is the descriptor id of the table. The table will inherit the zone config of the original table. A DROP DATABASE will move all the underlying tables to trash without the database. A TRUNCATE will leave the original table as empty with the data under trash/id

An admin can use ALTER TABLE DROP|RENAME to drop or bring back a table.

Once this is complete, we will stop allowing AS OF SYSTEM TIME queries over the original table before the DROP timestamp.

@knz knz added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) and removed C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. labels Jan 2, 2019
@postamar
Copy link
Contributor

Some of what's covered by this longstanding issue came to the forefront in a recent incident. As a user, I should be able to change the GC TTL of a table after it's dropped, but I can't. We need to fix this.

@ajwerner
Copy link
Contributor

ajwerner commented Jan 24, 2023

The proposal of the moment is to create a vtable which discovers all dropped descriptors which can hold zone configs (tables, materialized views, sequences, databases), and it lists

  • ID
  • names (at the time, unqualified)
  • parent IDs
  • drop time
  • current TTL.

Then, additionally, we create a builtin which can be used by an admin to inject a new TTL for a given ID which is dropped.

Taken together, this will allow users to find and update the TTL for dropped things.


In terms of implementation details, the code which goes to write the zone config entry should make sure that the descriptor exists and is dropped in the same transaction that writes the new zone config entry.

@ajwerner ajwerner added GA-blocker branch-release-23.1 Used to mark GA and release blockers, technical advisories, and bugs for 23.1 labels Jan 24, 2023
@postamar postamar removed the branch-release-23.1 Used to mark GA and release blockers, technical advisories, and bugs for 23.1 label Jan 24, 2023
postamar pushed a commit to postamar/cockroach that referenced this issue Jan 30, 2023
This commit adds a new virtual view which exposes which relations are
currently being dropped and are waiting for garbage collection.

Informs cockroachdb#26476.

Release note (sql change): users can query
crdb_internal.kv_dropped_relation to see which tables, materialized
views and sequences are currently already dropped but have not yet
been garbage collected, along with the garbage collection TTL setting
that's currently in force. This setting originates from the table's own
zone configuration, or from its parent database which it inherits, or in
turn from the default zone configuration. These settings are typically
set using ALTER TABLE ... CONFIGURE ZONE USING gc.ttlseconds = ...;
postamar pushed a commit to postamar/cockroach that referenced this issue Jan 30, 2023
This new builtin can be used to set the GC TTL for tables, sequences and
materialized views which have already been dropped.

Fixes cockroachdb#26476.

Release note (sql change): administrators may now call a new builtin
function crdb_internal.upsert_dropped_relation_gc_ttl to retroactively
set the garbage collection TTL on a table, materialized view or sequence
which has already been dropped. Effectively, this retroactively performs
ALTER TABLE ... CONFIGURE ZONE USING gc.ttlseconds = ...
Note that this statement is prevented from being executed on dropped
tables because they can no longer be referenced by name at that point.
Usage of this builtin is typically in conjunction with the recently
added crdb_internal.kv_dropped_relations virtual table. For example,
garbage collection can be triggered ASAP for all dropped relations by
querying:
  SELECT crdb_internal.upsert_dropped_relation_gc_ttl(id, '1 second')
    FROM crdb_internal.kv_dropped_relations;
Doing so for all tables in a dropped database requires filtering on the
parent_id column, the database name being lost at that point.
postamar pushed a commit to postamar/cockroach that referenced this issue Feb 1, 2023
This commit adds a new virtual view which exposes which relations are
currently being dropped and are waiting for garbage collection.

Informs cockroachdb#26476.

Release note (sql change): users can query
crdb_internal.kv_dropped_relation to see which tables, materialized
views and sequences are currently already dropped but have not yet
been garbage collected, along with the garbage collection TTL setting
that's currently in force. This setting originates from the table's own
zone configuration, or from its parent database which it inherits, or in
turn from the default zone configuration. These settings are typically
set using ALTER TABLE ... CONFIGURE ZONE USING gc.ttlseconds = ...;
craig bot pushed a commit that referenced this issue Feb 2, 2023
96217: sql: add crdb_internal.upsert_dropped_relation_gc_ttl builtin r=postamar a=postamar

sql: add crdb_internal.upsert_dropped_relation_gc_ttl builtin

This new builtin can be used to set the GC TTL for tables, sequences and
materialized views which have already been dropped.

Fixes #26476.

Release note (sql change): administrators may now call a new builtin
function crdb_internal.upsert_dropped_relation_gc_ttl to retroactively
set the garbage collection TTL on a table, materialized view or sequence
which has already been dropped. Effectively, this retroactively performs
ALTER TABLE ... CONFIGURE ZONE USING gc.ttlseconds = ...
Note that this statement is prevented from being executed on dropped
tables because they can no longer be referenced by name at that point.
Usage of this builtin is typically in conjunction with the recently
added crdb_internal.kv_dropped_relations virtual table. For example,
garbage collection can be triggered ASAP for all dropped relations by
querying:
  SELECT crdb_internal.upsert_dropped_relation_gc_ttl(id, '1 second')
    FROM crdb_internal.kv_dropped_relations;
Doing so for all tables in a dropped database requires filtering on the
parent_id column, the database name being lost at that point.

----

sql: add crdb_internal.kv_dropped_relations virtual view

This commit adds a new virtual view which exposes which relations are
currently being dropped and are waiting for garbage collection.

Informs #26476.

Release note (sql change): users can query
crdb_internal.kv_dropped_relation to see which tables, materialized
views and sequences are currently already dropped but have not yet
been garbage collected, along with the garbage collection TTL setting
that's currently in force. This setting originates from the table's own
zone configuration, or from its parent database which it inherits, or in
turn from the default zone configuration. These settings are typically
set using ALTER TABLE ... CONFIGURE ZONE USING gc.ttlseconds = ...;

----

sql: enable comments for virtual views

This commit makes it possible to define hardcoded comments for virtual
views, the same way it was already possible to do so for virtual tables.

Fixes #95427.

Release note: None


Co-authored-by: Marius Posta <marius@cockroachlabs.com>
@craig craig bot closed this as completed in 04028e6 Feb 2, 2023
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-schema-changes C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) GA-blocker O-postmortem Originated from a Postmortem action item. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants