GreptimeTeam / greptimedb

An open-source, cloud-native, unified time series database for metrics, logs and events with SQL/PromQL supported. Available on GreptimeCloud.
https://greptime.com/
Apache License 2.0
4.19k stars 299 forks source link

Alter database option #4394

Open killme2008 opened 1 month ago

killme2008 commented 1 month ago

What problem does the new feature solve?

Since v0.8, GreptimeDB supports database TTL just like:

CREATE DATABASE test with(ttl='7d');

which means all the tables in this database will inherit this option if they don't have their own TTL setting.

It does not support alerting the options after they are created.

What does the feature do?

Alter the database options:

Alter database test set ttl = '14d`;

Implementation challenges

No response

NiwakaDev commented 1 month ago

I'm interested in this issue. I guess we can implement this feature with the following high level steps.

  1. Allow parser to handle sql like ALTER DATABASE test SET ttl = '14d'.
  2. Implement AlterDatabaseProcedure. (I guess that greptimedb doesn't support this yet.) This procedure should alter the ttl for every inherited table(region) in a database, and then also alter the database ttl.
killme2008 commented 1 month ago

Cool. @NiwakaDev

Allow parser to handle sql like ALTER DATABASE test SET ttl = '14d'.

I recommend designing the statement to use set OPTIONS(ttl='14d') since we might need to SET other things in the future. Introducing the OPTIONS keyword is more beneficial. The use of parentheses () allows for batch setting of options, for example:

alter database set options ('ttl'='14d', 'compaction.time_window'='1d');

Implement AlterDatabaseProcedure. (I guess that greptimedb doesn't support this yet.) This procedure should alter the ttl > for every inherited table(region) in a database, and then also alter the database ttl.

Yes, you have to implement a procedure, which needs to change the database metadata. I am not quite sure if we need to alter all the inherited table options too, because:

  1. We don't have a value to represent if the option is inherited or defined by table DDL.
  2. It requires the table to support alter table set options, but it doesn't #1084

What do you think? @evenyag @fengjiachun

evenyag commented 1 month ago

alter database test options ('ttl'='14d', 'compaction.time_window'='1d');

Looks good.

It requires the table to support alter table set options, but it doesn't

Should table options have higher priority than database options?

fengjiachun commented 1 month ago

It requires the table to support alter table set options, but it doesn't

I think alter database relying on table options might not be a good idea. Because synchronizing and modifying all table options when executing alter database options could involve a large number of table modifications.

My idea is that alter database options should only update the database metadata. When the region GC worker operates, it retrieves the TTL of the database from DatabaseMetadataManager, merges it with the region's TTL to get the minimum value, and then continues its task.

Right now, we don't have a DatabaseMetadataManager, but that's not a big issue. We just need to wrap the KvBackend, and then we can get the TTL value of database from metasrv.

NiwakaDev commented 1 month ago

@fengjiachun

When the region GC worker operates, it retrieves the TTL of the database from DatabaseMetadataManager, merges it with the region's TTL to get the minimum value, and then continues its task.

Aren't there any situations where we want to prioritize the table ttl even if the database ttl is smaller?

evenyag commented 1 month ago

@fengjiachun

When the region GC worker operates, it retrieves the TTL of the database from DatabaseMetadataManager, merges it with the region's TTL to get the minimum value, and then continues its task.

Aren't there any situations where we want to prioritize the table ttl even if the database ttl is smaller?

I think the region's TTL option should have higher priority. It overwrites the database's TTL if we set the table TTL. @fengjiachun What do you think?

killme2008 commented 1 month ago

Of course, the table's custom TTL must be prioritized over the database TTL.

However, the table TTL is set to the database TTL when creating a table without the ttl table option.

https://github.com/GreptimeTeam/greptimedb/blob/b81d3a28e6223fb3cdcc53f8fed91dc73f99a335/src/operator/src/statement/ddl.rs#L1501

We don't know if a table is inherited or custom the TTL at all. There is one solution is that we can add a flag to figure out that the table TTL is merged from database TTL, and then while altering database TTL, we can change these tables(regions) TTL too.

Another choice is as @fengjiachun said, let's not change the region's TTL but just the database options. I prefer the second solution. It's much simpler, and altering many tables is expensive.

NiwakaDev commented 1 month ago

Another choice is as @fengjiachun said, let's not change the region's TTL but just the database options. I prefer the second solution. It's much simpler, and altering many tables is expensive.

Even if we choose the choice, I think we still need a flag indicates whether or not the table is inherited. I guess that adding the flag isn't expensive.

When the region GC works, if the flag is set, we prioritize the table TTL. Otherwise, we select the database TTL from the DatabaseMetadataManager.

killme2008 commented 1 month ago

Another choice is as @fengjiachun said, let's not change the region's TTL but just the database options. I prefer the second solution. It's much simpler, and altering many tables is expensive.

Even if we choose the choice, I think we still need a flag indicates whether or not the table is inherited. I guess that adding the flag isn't expensive.

Agree, let's add a flag to indicate whether or not the table option is inherited. Maybe it's an option set like inherited_options.

When the region GC works, if the flag is set, we prioritize the table TTL. Otherwise, we select the database TTL from the DatabaseMetadataManager.

Sound good. Then it doesn't need to alter all the table options, but just retrieving the database TTL while gc.

NiwakaDev commented 1 month ago

I'll implement this feature with the following high level steps: