fix: no set/hmap expiration during serialization

[BorysTheDev]

fixes: #5299

problem: we do expiration during serialization and save 0-size hmap or set that is incorrect
fix: avoid expiration during serialization and filter out expired hmap and set during loading

[romange]

1. The original issue was the correctness and not the performance problem during SAVE.
2. The trade off with eliminating the SlowSize call is that we may serialize expired items.

There are counter examples towards both approaches, and the problem is not black and white, so it's important to identify "not well defined" issues and avoid arguing around them too much because it's not productive. Instead, once a not well defined issue is identified with cons and pros, the best course of actions is to list them in the PR in an objective way and ask the product owner (which is me) what's the best course of action.

I prefer to limit the scope of this PR to the original issue and fix only the loader part. In the long run we probably should fix the algorithmics behind the expired set and introduce sorted queue of expired times but it's not a priority.

[BorysTheDev]

1. The original issue was the correctness and not the performance problem during SAVE.
2. The trade off with eliminating the SlowSize call is that we may serialize expired items.

There are counter examples towards both approaches, and the problem is not black and white, so it's important to identify "not well defined" issues and avoid arguing around them too much because it's not productive. Instead, once a not well defined issue is identified with cons and pros, the best course of actions is to list them in the PR in an objective way and ask the product owner (which is me) what's the best course of action.

I prefer to limit the scope of this PR to the original issue and fix only the loader part. In the long run we probably should fix the algorithmics behind the expired set and introduce sorted queue of expired times but it's not a priority.

If we decide to leave expiration, I need to remove a key (if set/hmap is zero size) during the serialization process (to me it looks quite strange)

[romange]

Talked to @BorysTheDev offline and it seems his solution is the only one possible at the moment without doing too much changes in the saving code. The reason for that is that we assume immutability of the entries at lease at the interface level (see SliceSnapshot::SerializeEntry).

[adiholden]

why do you set it to false? only to check if to return in line 2588? instead can you check if append_res is valid there?

[BorysTheDev]

This flag is also checked in CreateHMap and CreateSet. Unfortunately, the loading logic isn't straightforward, and it is hard to do changes.

[adiholden]

I am not sure what is this check error, is it to detect internal error flow ?
anyway I think a better way to do this is instead of checking the rdb type is checking if items this item went through the errc::value_expired flow, so you can add to item a field to be set in the case where you have the expired items which is not error flow and than check if this filed is not set in this condition

[BorysTheDev]

I've tried this approach, but for one entity we can create a lot of items so this approach doesn;t work. This check helps detect the bugs, because during loading we shouldn't expire any streaming objects execpt hmap and sset

[romange]

1. The condition is very complicated. lets factor out it to two variables for readability:
   bool key_not_expired = item->expire_ms == 0 || db_cntx.time_now_ms < item->expire_ms
bool set_not_expired = item->val.rdb_type != RDB_TYPE_SET_WITH_EXPIRY && ...
   then using key_not_expired && set_not_expired is more clear
2. overriding item->load_config.append does not justify mutability. Create a temporary LoadConfig with overridden field and pass it to FromOpaque

[romange]

And add a comment of why you need to override it (i.e. to create a new key because the previous batches of items for that key were fully expired).

[romange]

1. The condition is very complicated. lets factor out it to two variables for readability:
   bool key_not_expired = item->expire_ms == 0 || db_cntx.time_now_ms < item->expire_ms
bool set_not_expired = item->val.rdb_type != RDB_TYPE_SET_WITH_EXPIRY && ...
   then using key_not_expired && set_not_expired is more clear
2. overriding item->load_config.append does not justify mutability. Create a temporary LoadConfig with overridden field and pass it to FromOpaque

[romange]

And add a comment of why you need to override it (i.e. to create a new key because the previous batches of items for that key were fully expired).