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

Allow Longer keys upto 600 chars #25

Merged
merged 1 commit into from
Sep 28, 2023
Merged

Conversation

G8XSU
Copy link
Collaborator

@G8XSU G8XSU commented Sep 26, 2023

Motivation: Earlier we were conservative about our key-size since it is easier to increase but difficult to increase in backward compatible fashion. Now with introduction of sub_namespace and directly allowing upto 360char keys in LDK, we need support for longer key lengths.

Earlier it was 120 based on uuid length.

@G8XSU G8XSU requested a review from jkczyz September 26, 2023 04:54
@jkczyz
Copy link

jkczyz commented Sep 26, 2023

Motivation: Earlier we were conservative about our key-size since it is easier to increase but difficult to increase in backward compatible fashion. Now with introduction of sub_namespace and directly allowing upto 360char keys in LDK, we need support for longer key lengths.

Earlier it was 120 based on uuid length.

What's the math for 360 characters? I'm getting a significantly less number (112), e.g., monitor_updates/deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef_4294967295/18446744073709551615

@tnull
Copy link

tnull commented Sep 26, 2023

Motivation: Earlier we were conservative about our key-size since it is easier to increase but difficult to increase in backward compatible fashion. Now with introduction of sub_namespace and directly allowing upto 360char keys in LDK, we need support for longer key lengths.
Earlier it was 120 based on uuid length.

What's the math for 360 characters? I'm getting a significantly less number (112), e.g., monitor_updates/deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef_4294967295/18446744073709551615

We guarantee a max length of 120 chars for each of namespace, sub-namespace, and key (cf. https://github.com/lightningdevkit/rust-lightning/blob/ce7463486ee1ae61e9af439c3d34d00244248ee9/lightning/src/util/persist.rs#L35).

@G8XSU
Copy link
Collaborator Author

G8XSU commented Sep 27, 2023

Yes each of namespace subnamespace key can be up to 120 char each.
so it would be around 120*3 + delimiters, but we allow for longer than that since vss is not only meant for ldk.

But I don't think anyone else will use keys longer than ldk. So i will be fine with tighter bound as well.

Current number is from 120*5 = 600 (considering some buffer and if some other application wants to use longer keys.)

We could do around 400, as well, shouldn't matter much.

@jkczyz
Copy link

jkczyz commented Sep 27, 2023

Ah, thanks for explaining. Couple questions:

@jkczyz
Copy link

jkczyz commented Sep 27, 2023

Answering one of my own questions, ALTER COLUMN can update the type and would cast the data.

https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-DESC-SET-DATA-TYPE

I don't have a strong preference on text vs character varying, but we should document the choice of the value in the commit message.

@G8XSU
Copy link
Collaborator Author

G8XSU commented Sep 28, 2023

text vs character varying

I think it is basic sql that text is for unlimited size strings, and varchar is for limiting size of column.
It would be wrong usage of sql itself if a primary key column is text.

If we were using text, we owe an explanation. by default everything should be varchar if possible.

@jkczyz
Copy link

jkczyz commented Sep 28, 2023

FWIW, I'm seeing opposite advice but it doesn't touch on primary keys, FWIW.

https://wiki.postgresql.org/wiki/Don't_Do_This#Text_storage

Could you update the commit message to say why 600 was chosen?

Motivation: Earlier we were conservative about our key-size since
it is easier to increase but difficult to increase in backward
compatible fashion. Now with introduction of sub_namespace and
directly allowing upto 360char keys in LDK, we need support for
longer key lengths.
* We use varchar over text as it is sql standard
and can be indexed in all sql-implementations while limiting
key size.
* LDK can have keys of length up to 362(120*3+2), we use
600 as a safe limit with some buffer and considering applications
other than LDK.
@G8XSU
Copy link
Collaborator Author

G8XSU commented Sep 28, 2023

Added explanation for both in description.

Mainly:

  • We use varchar over text as it is sql standard
    and can be indexed in all sql-implementations while limiting
    key size.

For example MySQL doesn't allow indexes on text, postgres i will have to check. Hence we shouldn't use them as primary keys.
Text is also a non-standard across different sql implementations.
And depending on size of value in text field, it can get stored in separate file other than main database file. (Similar to blob).

Varchar and a text with constraint have similar characterstics apart from storage, indexing and sql-standardness.

Copy link

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it makes sense to use a power of two for any efficiency gains, but otherwise LGTM.

@G8XSU
Copy link
Collaborator Author

G8XSU commented Sep 28, 2023

Acc. to stackoverflow it doesnt matter. In reality, even much longer string lengths have comparable performance.

https://stackoverflow.com/questions/593364/varchar-fields-is-a-power-of-two-more-efficient
https://stackoverflow.com/questions/13233495/using-power-of-two-numbers-for-length-of-database-columns

@G8XSU G8XSU merged commit 0a185d8 into lightningdevkit:main Sep 28, 2023
1 check passed
G8XSU added a commit to G8XSU/vss-server that referenced this pull request Jul 29, 2024
G8XSU added a commit to G8XSU/vss-server that referenced this pull request Jul 29, 2024
This is to reflect changes from lightningdevkit#25 in tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants