"databases" misreported

[mgravell]

Describe the bug

The following is unexpected:

> config get databases
* 2
 [0]$ databases
 [1]$ 16
> select 1
- ERR invalid database index.

If select isn't fully implemented, databases should probably report 1. Reporting 16 but erroring if trying to change the database is... weird.

Steps to reproduce the bug

select 1 (or anything else greater than zero and less than the reported databases)

note that select 0 works fine and as expected

I suspect the real "bug" here is in config get, in particular databases

Expected behavior

No response

Screenshots

No response

Release version

No response

IDE

No response

OS version

No response

Additional context

No response

[Vijay-Nirmal]

Looks like if the cluster mode is enabled config get databases return as 16 else 1. Since Garnet doesn't support multiple database (#61), I think we should always return 1 . I can raise a PR if Garnet team is fine with this change. Just a small change.

garnet/libs/server/ServerConfig.cs

Line 83 in 9230a44

ServerConfigType.DATABASES => storeWrapper.serverOptions.EnableCluster ? "$9\r\ndatabases\r\n$1\r\n1\r\n"u8 : "$9\r\ndatabases\r\n$2\r\n16\r\n"u8,

Edit: Looks like if the cluster mode is enabled then we should get an error even in SELECT 0. I will dig deeper soon

garnet/libs/server/Resp/ArrayCommands.cs

Lines 240 to 245 in 9230a44

	if (storeWrapper.serverOptions.EnableCluster)
	{
	// Cluster mode does not allow DBID
	while (!RespWriteUtils.WriteError(CmdStrings.RESP_ERR_GENERIC_SELECT_CLUSTER_MODE, ref dcurr, dend))
	SendAndReset();
	}

[vazois]

For cluster mode, I think right now we are on par with Redis. We are basically returning the following. I guess the only difference is that Redis executes successfully for SELECT 0, so that might need to be fixed.

For the standalone, I guess it makes sense to advertise databases = 1. I am not sure though if there is any expectation from the clients to observe a value of 16. @mgravell, is there such thing? If not, then should be fine to return databases = 1.
Let me know if you are going to create a PR for this or else I can do it. It should be simple fix.

[rnz]

@vazois
May be better implement namespacing - #415 ?

Also redis support not only 16 databases, this parameter can be configured https://github.com/valkey-io/valkey/blob/45d596e1216472e49b9f950a4b9a040b6e87add6/valkey.conf#L396C1-L396C13

[mgravell]

Re select 0: from memory (not at PC), that already worked fine Re Redis; I'll check what they return, good point; if they also return 16, then: it is equally misleading, and "check the cluster mode first, before even considering the databases setting" is totally valid.

…

On Sat, 9 Nov 2024, 19:19 RNZ, ***@***.***> wrote: May be better implement namespacing - #415 <#415> ? — Reply to this email directly, view it on GitHub <#775 (comment)> or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEHMBRHHGKXRMF6BUYVGLZ7ZN2PBFKMF2HI4TJMJ2XIZLTSOBKK5TBNR2WLJDUOJ2WLJDOMFWWLO3UNBZGKYLEL5YGC4TUNFRWS4DBNZ2F6YLDORUXM2LUPGBKK5TBNR2WLJDUOJ2WLJDOMFWWLLTXMF2GG2C7MFRXI2LWNF2HTAVFOZQWY5LFUVUXG43VMWSG4YLNMWVXI2DSMVQWIX3UPFYGLLDTOVRGUZLDORPXI6LQMWWES43TOVSUG33NNVSW45FGORXXA2LDOOJIFJDUPFYGLKTSMVYG643JORXXE6NFOZQWY5LFVE3DQMJTG4ZDQNZRQKSHI6LQMWSWS43TOVS2K5TBNR2WLKRSGYZTQNBRGMZTQNNHORZGSZ3HMVZKMY3SMVQXIZI> . You are receiving this email because you authored the thread. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub> .

[vazois]

@vazois May be better implement namespacing - #415 ?

Also redis support not only 16 databases, this parameter can be configured https://github.com/valkey-io/valkey/blob/45d596e1216472e49b9f950a4b9a040b6e87add6/valkey.conf#L396C1-L396C13

We do not have plans to support namespaces. Our initial assessment is that it will be detrimental to performance. An alternative to namespaces (if that is the functionality you are looking for) could be using the ACL functionality to add permissions based on key pattern. Though this could also potentially be detrimental to performance and is currently not supported also. What is your exact use case of namespaces?

[vazois]

I have posted PR #800 to fix the above issue. @mgravell let me know if you have any issues with the proposed fix.