@zuiderkwast I see you commented on the previous thread, are you aligned with this? This seems like a good idea.

@madolson Yes, the original PR first just removed the validation and Oran complained that there may be corrupt data from unvalidated RDB or RESTORE. I think it's better to always validate on insert rather than on lookup, even though it may affect RDB loading time.

@ashtul do you want to make a flamegraph of loading a dump with and without sanitizing? It would be good to have a view on this difference as well, just for reference.

Here are some stats from when deep sanitization were introduced: redis/redis#7807 (comment). RESTORE deep is 80% slower than RESTORE shallow.

If we always sanitize listpacks on load and no longer on lookup, then we shall also do the same for intset and stream.

For zipmap and ziplist (no longer used) I think we always convert them when we load them from an old dump. Let's check that santitization is done in this case too.

Then we should remove the config and ACL rules added in redis/redis#7807. (We can't remove them immediately but we can make them have no effect.)

@madolson WDYT?

@madolson @zuiderkwast

The stats Oran showed might be skewed due to other improvements he has done as he writes Note that initially LPOS and HGET showed severe (-25%) degradation, and after some optimizations effort (last commit) i was able to re-gain the performance loss and even improve..

What do you suggest validating an insert? Maybe validating prev, next and the following next should be EOF.

As for stats, I will try to create them after the holiday.

BTW, how hard would it be to add mechanism which will load data without sanitation but won't add the key until it is sanitized, possibly on a thread?

What do you suggest validating an insert? Maybe validating prev, next and the following next should be EOF.

We only need to validate when we're loading a dump that can contain corrupt data (RESTORE or RDB). A normal insert can't add invalid data.

As for stats, I will try to create them after the holiday.

Sounds good. Anyway, I can accept a little slower RDB loading. It's more important to avoid validate on lookup. Also, the system is simpler if we never load corrupt data.

BTW, how hard would it be to add mechanism which will load data without sanitation but won't add the key until it is sanitized, possibly on a thread?

I think that would be too complex. Are you suggesting using a background thread for that? Like #356? We can do that later maybe, if you have a good idea about it, but not in the same PR.

It seems like all of this is the classic trade-off between security and speed. Since we can't have both, and there are situations that justify either choice, it appears to me that we should be giving the solution architect the ability to choose via a new configuration property.

@daniel-house There is already a config for that. Just the possibility of having invalid data is what prevents us from removing these asserts.

More important than load speed vs lookup speed IMO is the complexity aspect. The possibility of having invalid data in memory is a tech dept with a high maintenance cost IMO.

@valkey-io/core-team Shall we remove the possibility to load potentially corrupt data? Yes 👍 or no 👎 (This is a core team vote, so non-core-team members, please don't vote on this comment. Feel free to comment in the thread though.)

There is already a config for that. Just the possibility of having invalid data is what prevents us from removing these asserts.

It seems I was unclear. I meant to put a config around removing these asserts. Allow the asserts to be disabled via a test performed inside the in-lined functions.

Before I vote, to make sure I understand, the plan is to change the code so we always sanitize the load on RDB Restore and RDB Load so that we don't need to do these checks during runtime. I'm OK with that decision as long as we have performance numbers that we aren't dramatically (>25%) increased execution time on the load.

@ashtul Sorry for the delay. Do you have any interest in updating this PR to perform the validation on the restore path. This would improve the normal runtime performance, and only slightly degrade the restore case which should happen less often.

The decision was approved if we add the validation on the restore path.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.07%. Comparing base (04d76d8) to head (1d43af8).
Report is 101 commits behind head on unstable.

@@             Coverage Diff              @@
##           unstable     #399      +/-   ##
============================================
- Coverage     70.43%   70.07%   -0.37%     
============================================
  Files           113      114       +1     
  Lines         61728    61705      -23     
============================================
- Hits          43479    43237     -242     
- Misses        18249    18468     +219     

@madolson @zuiderkwast
This is great. Removing this unnecessary assert will speed up valkey for many use cases.

I need help with the tests. I am unfamiliar with tcl and my solution of removing whole tests is not likely to be the right solution. Is there is anyone I can get help from? I plan to come to Vienna for the conference, would it be better to discuss it then?

Hi @zuiderkwast,
I have rolled back some changes I have made since I don't have the knowledge in tcl and the attention span to fix the tests correctly.
The change in this PR is important and I do not want to drag committing it. I'm pretty sure some users will notice an increase in ops per second and lower latency as soon as we push it into the unstable branch. It actually gives valkey an advantage over redis.
Can someone takes the PR from here?

The change in this PR is important and I do not want to drag committing it.

We are releasing 8.0 very soon. We are busy fixing the stability of this release, so this PR will have to wait until after 8.0 is released.

What is missing in this PR is to make the config sanitize-dump-payload no behave just like sanitize-dump-payload yes, so we always sanitize the payload when we load it.

CI tests do not run and are awaiting your approval. Is that a bug?

If you haven't merged anything yet, it won't run automatically. I did run it and there is a format error, can you fix it?

Now I looked at the diff. It still just removes validation.

If we remove validation on lookup, we must do validation on insert instead.

Valkey's listpack is well tested and we do not fear a mistake during insertion.
Loading external data was the root issue for adding the assert.
Can you please remove the 'required changes' flag?

Now I looked at the diff. It still just removes validation.
If we remove validation on lookup, we must do validation on insert instead.

Valkey's listpack is well tested and we do not fear a mistake during insertion. Loading external data was the root issue for adding the assert. Can you please remove the 'required changes' flag?

Yes, I meant when loading external data. "On insert" was unclear, sorry. You still didn't ensure we always do validation when loading external data. I'm waiting for that change.

Currently, it is still possible to load corrupt data.

@zuiderkwast I see your point.

@madolson suggested to ignore the SANITIZE_DUMP_NO and always perform a check on insert. As a second stage, to sanitize incoming listpacks on a second thread as to not block the main thread. We how to agree how to treat listpacks that are found to be corrupted.

@madolson suggested to ignore the SANITIZE_DUMP_NO and always perform a check on insert.

Yes, that sounds right.

As a second stage, to sanitize incoming listpacks on a second thread as to not block the main thread. We how to agree how to treat listpacks that are found to be corrupted.

We'll do this while/before loading it? We offload to a thread and then wait for all such jobs to complete before we actually load them? If we do it this way, then we can also return an error just like we do for SANITIZE_DUMP_YES, right?

I don't see how we can do it after loading it, because then we will already have corrupt data in the system and maybe already accessing it.

Reminder:

The decision was approved if we add the validation on the restore path.

This is the decision we made long ago, as noted when the approved label was added: #399 (comment)

This PR is not relevant without the missing validation.

Replaced by #3721.