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

[TTL] Historical data may never expire #5131

Closed
cangfengzhs opened this issue Dec 28, 2022 · 9 comments
Closed

[TTL] Historical data may never expire #5131

cangfengzhs opened this issue Dec 28, 2022 · 9 comments
Assignees
Labels
affects/master PR/issue: this bug affects master version. doc affected PR: improvements or additions to documentation process/done Process of bug severity/minor Severity of bug type/bug Type: something is unexpected wontfix Solution: this will not be worked on recently
Milestone

Comments

@cangfengzhs
Copy link
Contributor

Please check the FAQ documentation before raising an issue

Describe the bug (required)

create tag ttl_tag1(age int);
insert vertex ttl_tag1(age) VALUES "1":(10),"2":(20),"3":(30);
alter tag ttl_tag1 add (ttl timestamp default now());
alter tag ttl_tag1 ttl_duration=10, ttl_col="ttl";
insert vertex ttl_tag1(age) VALUES "4":(40);
match (v:ttl_tag1) return v limit 10;

After the above series of operations. The ttl_tag1 of "1", "2" and "3" will never expire.

This is because there is a problem with our definition of default. In this case, the default value of ttl is now(). now(), which will be stored in the definition of ttl (it is the function now(), not the return value of it).

When we read a ttl if the property does not exist in tag1.ttl, it will get the default value from the schema definition. So for ttl_tag1 of "1", "2" and "3", the latest now() will be calculated every time when reading, so they will never expire.

Your Environments (required)

  • OS: uname -a
  • Compiler: g++ --version or clang++ --version
  • CPU: lscpu
  • Commit id (e.g. a3ffc7d8)

How To Reproduce(required)

Steps to reproduce the behavior:

  1. Step 1
  2. Step 2
  3. Step 3

Expected behavior

Additional context

@cangfengzhs cangfengzhs added the type/bug Type: something is unexpected label Dec 28, 2022
@github-actions github-actions bot added affects/none PR/issue: this bug affects none version. severity/none Severity of bug labels Dec 28, 2022
@cangfengzhs
Copy link
Contributor Author

cangfengzhs commented Dec 28, 2022

Here is a relatively reasonable repair plan. We define two default values for each property, default_read_value and default_write_value.

When we define a DEFAULT, we assign it to both default_read_value and default_write_value. However, the difference is that default_read_value must be a constant, and default_write_value is the actually defined DEFAULT.

In the above case, default_read_value stores the actual time (the return value of now()) when ttl is defined, and default_write_value stores the now() function itself.

Later, in the process of using ttl, if it is found that there is no ttl attribute when reading its value, it will be filled with default_read_value. If no ttl value is given when writing, it will be filled with default_write_value.

In addition, subsequent modifications to 'DEFAULT' will only modify the default_write_value, and default_read_value will never modify

@cangfengzhs
Copy link
Contributor Author

And, if the value of property ttlis NULL, tag also never expire.

@cangfengzhs
Copy link
Contributor Author

Another relatively simple repair method is to prohibit the use of "now()" as the default value on the ttl column, and do not allow the value to be NULL

@cangfengzhs
Copy link
Contributor Author

@critical27 @yixinglu

@cangfengzhs
Copy link
Contributor Author

@MuYiYong

@critical27
Copy link
Contributor

critical27 commented Dec 28, 2022

I suggest forbid use a default value of now, in alter tag ttl_tag1 add (ttl timestamp default now());

@Sophie-Xie Sophie-Xie added this to the v3.4.0 milestone Dec 28, 2022
@shanlai shanlai added severity/minor Severity of bug affects/master PR/issue: this bug affects master version. and removed severity/none Severity of bug affects/none PR/issue: this bug affects none version. labels Dec 28, 2022
@HarrisChu
Copy link
Contributor

prefer to mark as a known issue.
and we could re-design in v5.0

@Sophie-Xie Sophie-Xie added the wontfix Solution: this will not be worked on recently label Jan 4, 2023
@Sophie-Xie
Copy link
Contributor

@abby-cyber discussed it with @MuYiYong offline: he suggested to update doc.

@abby-cyber abby-cyber added the doc affected PR: improvements or additions to documentation label Jan 6, 2023
@abby-cyber
Copy link
Contributor

@github-actions github-actions bot added the process/fixed Process of bug label Jan 6, 2023
@shanlai shanlai added the process/done Process of bug label Jan 6, 2023
@github-actions github-actions bot removed the process/fixed Process of bug label Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects/master PR/issue: this bug affects master version. doc affected PR: improvements or additions to documentation process/done Process of bug severity/minor Severity of bug type/bug Type: something is unexpected wontfix Solution: this will not be worked on recently
Projects
None yet
Development

No branches or pull requests

7 participants