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

Changing Valkey RDB magic for Valkey 8 #645

Closed
madolson opened this issue Jun 13, 2024 · 14 comments · Fixed by #665
Closed

Changing Valkey RDB magic for Valkey 8 #645

madolson opened this issue Jun 13, 2024 · 14 comments · Fixed by #665
Labels
major-decision-pending Major decision pending by TSC team

Comments

@madolson
Copy link
Member

Redis likely bumped their RDB OP code to 12 for a 7.4, and we have bumped our RDB OP code to 12 as well, however our two versions are no longer compatible. We should consider changing our magic to be "VALKEYXXX" instead. It keeps the same 10 character alignment, and I assume we don't ever need more than 999 RDB versions.

@madolson
Copy link
Member Author

madolson commented Jun 13, 2024

More broadly we have the topic of migrations. Once we diverge, how will we support migrations from Redis <-> Valkey. We could implement logic to also support their opcodes for loading. There will be some special logic in Redis 7.4 to support the new hash field expiration, which we won't be able to load.

@PingXie
Copy link
Member

PingXie commented Jun 13, 2024

I think we should draw a line between Redis 7.2 and 7.4. Any RDB generated by Redis 7.2 and prior versions should be able to load on any Valkey versions (without modules). Post 7.4, inclusive, it is going to be really hard to stay compatible for the reasons you mentioned above. Vice versa, we also cant guarantee anything about loading Valkey persistence files on newer Redis versions. And lastly, "downgrading" RDB is never supported in the Redis world regardless so not being able to load newer Valkey RDB on the older Redis/Valkey versions is not news.

@hwware
Copy link
Member

hwware commented Jun 13, 2024

Since Valkey was born, it support compatible with Redis 7.2.4 and its previous version. Thus it natively supports RDB version 10 format. And because RDB version 12 and new hash field expiration are introduced in Redis 7.4, it makes sense for Valkey 8 RDB not to load them.

But my concern is if we really need to upgrade to version 12, because 2 years ago, many users were suffering the incompatible between Redis 7 and Redis 6.2, do we have chance to postpone this plan to later version?

Briefly summarize:

  1. Valkey 8 not support Redis 7.4 RDB load
  2. Suggest Valkey 8 do not upgrade to 12, make more clients confortable
  3. If Valkey 8 upgrade to 12, do not support downgrade from Valkey 8 to Valkey 7.2 and Redis OSS 7.2

@madolson
Copy link
Member Author

Suggest Valkey 8 do not upgrade to 12, make more clients confortable

I was talking about this with another AWS engineer. The only thing we're introducing is optional metadata related to per-slot loading to make it faster. We don't need to introduce this, and could change it be an optional AUX field. There is a second option, and want to tie it into Ping's comment:

And lastly, "downgrading" RDB is never supported in the Redis world regardless so not being able to load newer Valkey RDB on the older Redis/Valkey versions is not news.

It's worth at least mentioning this came up in the summit, and is honestly not that hard to implement. We could optionally have the replica indicate it's preferred version, and have the primary take that into consideration when generating the output. If the replica doesn't indicate any version (like it would for 7.2 and early) we assume it's on 7.2 and send it with older compatibility.

@soloestoy
Copy link
Member

Agree to change the RDB magic in Valkey 8, and be compatible with redis's RDB magic thus we can load data from redis before 7.4.

About the RDB version, after magic changed, it can be arbitrary even start from 1 : )

It's worth at least mentioning this came up in the summit, and is honestly not that hard to implement. We could optionally have the replica indicate it's preferred version, and have the primary take that into consideration when generating the output. If the replica doesn't indicate any version (like it would for 7.2 and early) we assume it's on 7.2 and send it with older compatibility.

Interesting, but I think it can only be used for BGSAVE to generate old version RDB. I don't think it's good for replication, since master run in new major version could have new commands, these commands cannot be executed in old version replica.

@zuiderkwast
Copy link
Contributor

For replication to replicas of different versions, we have a shared replication buffer, so which version to pick?

What if someone dumps to disk, then downgrades and loads the dump with an older version?

For maximum compatibility, we could dump using the lowest version possible for the data currently stored. We'd need to keep track of the features currenty used though, so for example we the first time a new feature is used (assume timestamp per hash field) we set the minimum RDB version for dumping the data set. If the new features are not used, an older format is used when dumping/replicating.

@zuiderkwast
Copy link
Contributor

zuiderkwast commented Jun 14, 2024

To answer the actual questions:

Yes, changing magic to "VALKEYXXX" (and maybe start version number from 1, I don't mind, but also I don't care) is a good idea. I like this. 👍

Support possible future Redis dumps sounds like a hypothetical thing. I think we can defer this to external tools, to convert the RDB files between different formats. Or it could be something for the "Redis" module someone mentioned in the summit.

And lastly, "downgrading" RDB is never supported in the Redis world regardless so not being able to load newer Valkey RDB on the older Redis/Valkey versions is not news.'

True, but I do think that downgrading, i.e. forward compatibility, would be a good feature to have though. Valli from the summit talked about it in the summit. One version forward compat would be enough. It can happen that problems are found after upgrading (performance or other problems) so users want to revert the upgrade, i.e. downgrade. That's what my previous comment was all about. It's good to allow primaries to be upgraded before replicas too, although we don't recommend it.

@madolson
Copy link
Member Author

So, for Valkey 8, I think Wen's opinion makes sense. I think we should do the following:

  1. Make it so that Valkey 8 still generates a Redis 7.2 compatible snapshot.
  2. Introduce the logic to handle Valkey magic in the snapshots.

@valkey-io/core-team Does this seem reasonable?

@soloestoy
Copy link
Member

soloestoy commented Jun 17, 2024

I'm a bit confused, it means that Valkey 8 still use Redis' magic and 7.2's version to generate an RDB file? Then how to handle Valkey magic (no one would generate an RDB file contains Valkey magic)?

@zuiderkwast
Copy link
Contributor

1. Make it so that Valkey 8 still generates a Redis 7.2 compatible snapshot.

Makes sense to me. We only add aux fields that are ignored in older versions.

2. Introduce the logic to handle Valkey magic in the snapshots.

Like Zhao, I'm a bit confused. Maybe Valkey 9 can generate an RDB file that contains Valkey magic? But Valkey 8 can only load it if it knows the new features of Valkey 9.

Valkey 8 cannot know the new features the future RDB version, so I think it doesn't make sense to handle Valkey magic now.

My preference is that we should only bump the magic and the version when we need to make breaking changes (like adding a new datatype or encoding), so we can do it in Valkey 9, but even in Valkey 9 if no keys use the new features, Valkey 9 can still generate dumps with the old format, to make it possible to downgrade to Valkey 8 as long as no new features are used.

@hpatro
Copy link
Contributor

hpatro commented Jun 17, 2024

So, for Valkey 8, I think Wen's opinion makes sense. I think we should do the following:

  1. Make it so that Valkey 8 still generates a Redis 7.2 compatible snapshot.

What is the behavior with the parsers regarding aux fields? Are AUX fields skipped over if they don't understand it?

@madolson
Copy link
Member Author

What is the behavior with the parsers regarding aux fields? Are AUX fields skipped over if they don't understand it?

Yes, if the server doesn't understand the aux field, it's skipped.

@zuiderkwast
Copy link
Contributor

Rename this issue to "revert bumbing the RDB version for Valkey 8" or create a separate issue for that?

ranshid added a commit to ranshid/valkey that referenced this issue Jun 18, 2024
This PR makes our current RDB format compatible with the Redis 7.2.4 RDB format.
there are 2 changes introduced in this PR:
1. Move back the RDB version to 11
2. Make slot info section persist as AUX data instead of dedicated
   section.

We have introduced slot-info as part of the work to replace cluster metadata with slot specific dictionaries.
This caused us to bump the RDB version and thus we prevent downgrade
(which is conceptualy O.K but better be prevented). We do not require
the slot-info section to exist, so making it an AUX section will help
suppport version downgrade from Valkey 8.

fixes: [valkey-io#645](valkey-io#645)

Signed-off-by: ranshid <ranshid@amazon.com>
@ranshid
Copy link
Member

ranshid commented Jun 18, 2024

Rename this issue to "revert bumbing the RDB version for Valkey 8" or create a separate issue for that?

@valkey-io/core-team I created #665 I think it will also support extending slot-info in the future and I tested compatibility with Redis 7.24

madolson pushed a commit that referenced this issue Jun 18, 2024
This PR makes our current RDB format compatible with the Redis 7.2.4 RDB
format. there are 2 changes introduced in this PR:
1. Move back the RDB version to 11
2. Make slot info section persist as AUX data instead of dedicated
section.

We have introduced slot-info as part of the work to replace cluster
metadata with slot specific dictionaries. This caused us to bump the RDB
version and thus we prevent downgrade (which is conceptualy O.K but
better be prevented). We do not require the slot-info section to exist,
so making it an AUX section will help suppport version downgrade from
Valkey 8.

fixes: [#645](#645)

NOTE: tested manually by:
1. connecting Redis 7.2.4 replica to a Valkey 8(RC) 
2. upgrade/downgrade Redis 7.2.4 cluster and Valkey 8(RC) cluster

---------

Signed-off-by: ranshid <ranshid@amazon.com>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-decision-pending Major decision pending by TSC team
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

7 participants