MEMORY DOCTOR/MALLOC-STATS in loading phase

[knggk]

The problem/use-case that the feature addresses

Sometimes I wish to investigate memory issues during the loading phase. However I get:

socket_redis> memory malloc-stats
LOADING Redis is loading the dataset in memory

Description of the feature

I would like to see the memory details provided by these commands during loading

Additional information

The check seems to be here https://github.com/valkey-io/valkey/blob/unstable/src/server.c#L4176-L4181:

    /* Loading DB? Return an error if the command has not the
     * CMD_LOADING flag. */
    if (server.loading && !server.async_loading && is_denyloading_command) {
        rejectCommand(c, shared.loadingerr);
        return C_OK;
    }

The question: Is there specific reasons those commands are disallowed during loading?

If not, is it as simple as adding CMD_LOADING flag to all the MEMORY commands, or there are considerations to take into account for making this work?

[hwware]

The command with flag "LOADING" indicates that this command has no any interaction with the data set.
During loading process, data is written into memory, thus memory malloc-stats and related commands are not allowed to run.

[knggk]

@hwware Understood about LOADING being defined as a flag for commands not interacting with the dataset.
Apart from that definition, is it not a potential valid use case to dump malloc-stats for investigation during loading?

[madolson]

I think we can allow this, it shouldn't be a breaking change, @knggk do you want to open a PR? We might also want to take a look at similar commands to see which ones we should enable so we stop flipping them one at a time :)

[knggk]

@madolson

We might also want to take a look at similar commands to see which ones we should enable so we stop flipping them one at a time :)

Agree...I was thinking doing all memory commands.

One additional question: any pointers on how to unit test this assuming we want to?

[hpatro]

One additional question: any pointers on how to unit test this assuming we want to?

@knggk I think it's fine to write a tcl test and slowdown the rdb loading via key-load-delay config and then perform the MEMORY * command operation.

[knggk]

I will look into TCL + key-load-delay. Thanks @hpatro!

[knggk]

I've moved all memory commands to be allowed during loading EXCEPT for memory usage key, because that one works on key and was always returning empty on any key during loading. It's also directly interacting with the dataset, which we want to disallow during loading as per @hwware.

Let me know your thoughts on the PR.

[hwware]

Honestly said, I have a little bit concern to allow run command "memory stats" during loading, because it costs a lot of resources and involves key issues. (key count, key used memory). Can we just allow the other 3 commands to be called during loading process?

[knggk]

Honestly said, I have a little bit concern to allow run command "memory stats" during loading, because it costs a lot of resources and involves key issues. (key count, key used memory). Can we just allow the other 3 commands to be called during loading process?

Ok, I do see

keys.count: The total number of keys stored across all databases in the server
keys.bytes-per-key: The ratio between dataset.bytes and keys.count

as part of the memory stats doc. (I don't know that it costs a lot of resources though.)

I can remove memory stats from the commands allowed during loading.

That would leave us with memory doctor, memory malloc-stats, memory purge now being allowed during loading.

Thoughts @hpatro @madolson?

[madolson]

I would also remove memory doctor. I think it might give weird results when run during loading. The other two seem fine to me.

[knggk]

Ok that makes sense. I will update the PR keeping only memory malloc-stats and memory purge, and updating the TCL. Thank you @hwware @hpatro and @madolson for the feedback.

[knggk]

I've made the requested changes to the PR as per the feedback.