Handle missing fields in dbSwapDatabases and swapMainDbWithTempDb #12763
Conversation
The change in dbSwapDatabases seems harmless. Because in non-clustered mode, dbBuckets calculations are strictly accurate and in cluster mode, we only have one DB. Modify it for uniformity (just like resize_cursor). The change in swapMainDbWithTempDb is needed in case we swap with the temp db, otherwise the overhead memory usage of db can be miscalculated. Introduced in redis#12697.
|
should we also swap the rehashing? |
hpatro
left a comment
There was a problem hiding this comment.
bucket_count and rehashing are only used in cluster-enabled and it allows only one 1 db. If you see dictRehashingStarted, we do an early exit at the start of the method.
So, I believe this change is not required.
|
Currently, it is indeed not necessary, but if unsharded-cluster needs to support multiple databases, then it would be required. However, unsharded-cluster also has many other tasks to accomplish. |
|
yean, the changes is no needed indeed. since we also swap resize_cursor (it is also cluster only), i think it will be fine to do the swap, and in addition, i personally think that swapping is good in generally, although it only used in the cluster. Let’s see the final decision of oran and soloestoy |
zuiderkwast
left a comment
There was a problem hiding this comment.
I think it is good to merge this.
It's better not to rely on the two facts that bucket count is only used in cluster mode and that SWAPDB can't be used in cluster mode. This function can have less assumptions about the world if we just swap the bucket count too, like we swap everything else.
|
when cluster mode will support multiple dbs, it'll be unsharded. these two things go together. i.e. as soon as cluster is sharded, it has just one db. maybe it's easier to just place some comment that clarifies this var and other structures are only present when redis is sharded? |
|
in the original PR, we swapped everything, including resize_cursor (cluster only). do we still want to swap everything (dbDictState)? otherwise let's close it. |
|
I think it's good to swap everything. The code doesn't have to rely on assumptions that certain features cannot be used together. Those assumptions add to the complexity. Better let the code just swap everything IMO. But I don't mind if we close it if others don't agree. |
|
ok, the CI report a rehashing leak in discardTempDb. Not sure if i should continue |
|
i think you should continue. |
The change in dbSwapDatabases seems harmless. Because in non-clustered
mode, dbBuckets calculations are strictly accurate and in cluster mode,
we only have one DB. Modify it for uniformity (just like resize_cursor).
The change in swapMainDbWithTempDb is needed in case we swap with the
temp db, otherwise the overhead memory usage of db can be miscalculated.
In addition we will swap all fields (including rehashing list), just for
completeness (and reduce the chance of surprises in the future).
Introduced in #12697.