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

Additional callouts for row-level TTL #13703

Closed
5 tasks
vy-ton opened this issue Apr 25, 2022 · 9 comments
Closed
5 tasks

Additional callouts for row-level TTL #13703

vy-ton opened this issue Apr 25, 2022 · 9 comments
Assignees

Comments

@vy-ton
Copy link
Contributor

vy-ton commented Apr 25, 2022

Vy Ton (vy-ton) commented:

Vy Ton (vy-ton) commented:

Vy Ton (vy-ton) commented:

This issue is a meta-issue of additional callout and enhancements to row-level TTL docs:

Jira Issue: DOC-3545

@exalate-issue-sync
Copy link

Richard Loveland (rmloveland) commented:
Another one from Oliver Tan via cockroachdb/cockroach#82140 (comment)

{quote}it may be worth documenting on the TTL docs that tables with multiple indexes may benefit from a lower delete_batch_size, as secondary indexes touch multiple ranges and a larger batch size means potentially more ranges being touched which potentially impacts cluster-wide latency. let me know if you need clarification!{quote}

@exalate-issue-sync
Copy link

Richard Loveland (rmloveland) commented:
Vy Ton (and/or Oliver Tan )

couple questions:

  1. I don’t understand “schema changes do not update {{crdb_internal_expiration}} “ - are they supposed to? what schema changes are being referred to? I would not expect that column to be updated unless I (1) set it directly, or (2) changed the {{ttl_expire_after}} on a table. Do you mind explaining what is the behavior that is surprising to users? (update: or is it related to the below?)

  2. what is meant by the comment “what value {{ttl_expire_after}} is set to during backfill”? presumably the user set the expiration themself when they added TTL to the table (per this slide) so the value is set to the value of ~ {{now() + ttl_expire_after}} from the point in time when they issued the schema change? or is that not the behavior, and hence the surprise?

  3. Oliver if you can tell me the required perms that would be awesome. Is it the same as for CREATE TABLE?

@exalate-issue-sync
Copy link

Vy Ton (vy-ton) commented:
{quote}I don’t understand “schema changes do not update {{crdb_internal_expiration}} “ - are they supposed to? what schema changes are being referred to?{quote}

For schema changes with backfills, there was concern that the schema change itself would be treated as a row update and therefore would push {{crdb_internal_expiration}}. But this is not the case, so schema changes do not change crdb_internal_expiration values

{quote}what is meant by the comment “what value {{ttl_expire_after}} is set to during backfill”? presumably the user set the expiration themself when they added TTL to the table (per this slide) so the value is set to the value of ~ {{now() + ttl_expire_after}} from the point in time when they issued the schema change? or is that not the behavior, and hence the surprise?{quote}

Setting TTL on an existing table requires a schema change with backfill to create the {{crdb_internal_expiration}} column - that’s what we want users to note. The default behavior sets the {{expiration}} column to {{now() + ttl_expire_after}} like you noted. A user adding TTL to existing tables prob has some other logic to control deletion in which case they need to wait for cockroachdb/cockroach#76916

@exalate-issue-sync
Copy link

Vy Ton (vy-ton) commented:
Richard Loveland Do you have a PR in progress for this? I’d like to get the callout for migration of existing tables in the next 2w.

@exalate-issue-sync
Copy link

Richard Loveland (rmloveland) commented:
Vy Ton Yes! just merged #14481 30 seconds before seeing your comment

By “migration of existing tables” do you mean the info about how the schema change when you add TTL to a table does not change {{crdb_internal_expiration}} ? If so that info is in the PR AFAICT and should be showing up soon on https://www.cockroachlabs.com/docs/v22.1/row-level-ttl.html

Let me know if I should have tagged you on the PR. I did not because it was not really product positioning stuff, only some technical details

PS thanks for answering my questions before, not sure why I forgot to respond.

@exalate-issue-sync
Copy link

Richard Loveland (rmloveland) commented:
{quote}I did not because it was not really product positioning stuff, only some technical details{quote}

or at least that was my understanding, please let me know if that was incorrect. happy to update my understanding / do something different

@exalate-issue-sync
Copy link

Vy Ton (vy-ton) commented:
I didn’t need to be tagged for review 🙂

{quote}By “migration of existing tables” do you mean the info about how the schema change when you add TTL to a table does not change {{crdb_internal_expiration}} ?{quote}

No, I want to highlight the unfulfilled user story for taking an existing large table and adding TTL to it. Right now, the UX is non-ideal, details here. In 22.1, we need to backfill a new column and the expiration timestamp is set to {{now() + ttl_expire_after}}

@exalate-issue-sync
Copy link

Richard Loveland (rmloveland) commented:
{quote}No, I want to highlight the unfulfilled user story for taking an existing large table and adding TTL to it. Right now, the UX is non-ideal, details here. In 22.1, we need to backfill a new column and the expiration timestamp is set to {{now() + ttl_expire_after}}{quote}

Ah ok, right now the docs on adding TTL to an existing table say “Adding or changing the Row-Level TTL settings for an existing table will result in a schema change that sets the {{crdb_internal_expiration}} column for all rows. Depending on table size, this could be an expensive operation.”

Are you saying that you would like to add more detail to that info? specifically, add the detail that CockroachDB needs to backfill a new column and the expiration timestamp is set to {{now() + ttl_expire_after }} ?

If yes, I can make a PR to add that ASAP

If no, please advise 🙂

@exalate-issue-sync
Copy link

Vy Ton (vy-ton) commented:
{quote}Are you saying that you would like to add more detail to that info? specifically, add the detail that CockroachDB needs to backfill a new column and the expiration timestamp is set to {{now() + ttl_expire_after }}?{quote}

Yes and can you mention it in the {{Limitation}} section as well. It relates to the note about “The TTL cannot be customized based on the values of other columns in the row. cockroachdb/cockroach#76916” because for an existing table you might already have a column that TTL should use.

My goal here is for users to understand that creating new tables with TTL is well supported. Adding TTL to existing tables, especially when they are large, will have better user experience in 22.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants