Skip to content

allow union of several retention policies#3766

Merged
Ivansete-status merged 13 commits intorelease/v0.37from
union-retention-policy
Mar 19, 2026
Merged

allow union of several retention policies#3766
Ivansete-status merged 13 commits intorelease/v0.37from
union-retention-policy

Conversation

@Ivansete-status
Copy link
Copy Markdown
Collaborator

@Ivansete-status Ivansete-status commented Mar 18, 2026

Description

  • Fix the time retention logic. The logic could cause all the partitions to get removed because we checked if certain time threshold belongs to any partition. Instead, <-only approach is being used.
  • Allow setting several retention policies simultaneously by a semicolon-separated string. For example: size: 50GB;time:86400. The node applies both and then, the most restrictive remains.

Validation done

I've validated that the proposed solution works well in waku.test.
By applying the following:

store-message-retention-policy = 'size:41GB;time:15724800'

... the database is kept up to the lowest retention policy threshold (either of them.)

In the following pic I share evidence how a retention policy is applied depending on which is more restrictive:

image

Related Issue

This PR aims to help mitigate the following issue as much as possible but it doesn't fixes it.

@github-actions
Copy link
Copy Markdown

This PR may contain changes to database schema of one of the drivers.

If you are introducing any changes to the schema, make sure the upgrade from the latest release to this change passes without any errors/issues.

Please make sure the label release-notes is added to make sure upgrade instructions properly highlight this change.

@Ivansete-status Ivansete-status self-assigned this Mar 18, 2026
@Ivansete-status Ivansete-status changed the base branch from master to release/v0.37 March 18, 2026 20:34
partitionLastMoment = oldestPartition.getLastMoment(),
tsInSec

while oldestPartition.getFirstMoment() < tsInSec:
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous while implementation caused the deletion of all the partitions when using time retention policy.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this oldestPartition.getLastMoment() < tsInSec?

That is, this whole partition is dead if the entire timestamp range in it is obsolete.

first = time 10
last = time 20
oldest msg we want = 15
we have to keep this partition, but first (10) < 15 and it gets deleted

(Maybe I'm just being dumb here, please check the math!)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I have the same impression that it should be

Suggested change
while oldestPartition.getFirstMoment() < tsInSec:
while oldestPartition.getLastMoment() < tsInSec:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Thank you both! You see that I'm nothing without a calculator xD
Addressed in fde1723

@fcecin
Copy link
Copy Markdown
Contributor

fcecin commented Mar 18, 2026

I'm wondering whether we can't just delete this query inside method deleteMessagesOlderThanTimestamp*:

  (
    await s.writeConnPool.pgQuery(
      "DELETE FROM messages WHERE timestamp < " & $tsNanoSec
    )
  ).isOkOr:
    return err("error in deleteMessagesOlderThanTimestamp: " & $error)

Just dropping whole partitions is faster than running a query to collect the last expired messages inside the oldest partition (because, well, it's just less work). Maybe we can get away with some fuzziness in the time retention logic and just drop whole partitions at once? Sticking to data expiration in whole buckets is usually safer; it has a more predictable cost.

Copy link
Copy Markdown
Contributor

@fcecin fcecin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!!
LGTM!

Copy link
Copy Markdown
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve with the time check needs attention.

storeMessageRetentionPolicy* {.
desc:
"Message store retention policy. Time retention policy: 'time:<seconds>'. Capacity retention policy: 'capacity:<count>'. Size retention policy: 'size:<xMB/xGB>'. Set to 'none' to disable.",
"Message store retention policy. Multiple policies may be provided as a semicolon-separated string and are applied as a union. Time retention policy: 'time:<seconds>'. Capacity retention policy: 'capacity:<count>'. Size retention policy: 'size:<xMB/xGB>'. Set to 'none' to disable. Example: 'time:3600;size:1GB;capacity:100'.",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please note what happens if duplicated options are found (I'd just fail to start in this case, as it's obviously a misconfiguration).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed in this 2eb14b6

@Ivansete-status
Copy link
Copy Markdown
Collaborator Author

I'm wondering whether we can't just delete this query inside method deleteMessagesOlderThanTimestamp*:
... ( I'm cutting to shorten the comment )

Yes good point. In this particular case, we are dropping partitions at the beginning of the proc.
For now I would leave it because that way the implementation is more accurate with the proc's name.
Thanks!

@Ivansete-status Ivansete-status merged commit 11461ae into release/v0.37 Mar 19, 2026
1 check passed
@Ivansete-status Ivansete-status deleted the union-retention-policy branch March 19, 2026 20:37
Ivansete-status added a commit that referenced this pull request Mar 24, 2026
* refactor retention policy to allow union of several retention policies
* bug fix time retention policy
* add removal of orphan partitions if any
* use nim-http-utils 0.4.1
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

Successfully merging this pull request may close these issues.

4 participants