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

wallet/db.c, wallet/wallet.c: Add a partial index to speed up startup. #4925

Merged
merged 1 commit into from
Dec 2, 2021

Conversation

ZmnSCPxj
Copy link
Collaborator

Closes: #4901

Tested by EXPLAIN QUERY PLAN on sqlite3; #4901 shows the result from
@whitslack doing a similar partial index on PostgreSQL on his ~1000 chan
node.

@ZmnSCPxj ZmnSCPxj force-pushed the speedup-4901 branch 2 times, most recently from dedc985 to b15f8a4 Compare November 23, 2021 05:44
wallet/db.c Outdated
" ON channel_htlcs(channel_id, direction)"
" WHERE hstate NOT IN (9, 19);"),
/* 9 = RCVD_REMOVE_ACK_REVOCATION
* 19 = SENT_REMOVE_ACK_REVOCATION
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be really nice if we could put in the constants here instead of hardcoding the constant values 9 and 19. Any ideas how to do this? Could use a function for this migration so we can db_bind_int, but in principle a function is not necessary and would be too heavyweight...

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe it is something stupid and I'm missing the point, but we could define a #define RCVD_REMOVE_ACK_REVOCATION 9 somewhere?

Just worry if it is a valid type of db_bind_int

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They are already defined here:

RCVD_REMOVE_ACK_REVOCATION,
SENT_REMOVE_ACK_REVOCATION,

The #define would duplicate the above declaration, and if not carefully ordered relative to the above, would cause a syntax error. We also prefer to use the above enum declaration if possible.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than asserting the values of these constants in a comment, you could use a compile-time assertion. I see you have a BUILD_ASSERT macro presumably for that purpose. wallet/wallet.h does contain assertions of the numeric values of these enumerators, but you could duplicate those assertions here in wallet/db.c at zero cost to make absolutely certain that the enumerators retain the values you're expecting.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is not really about asserting the values, it is more that stylistically, I would prefer not to see hardcoded numbers in code. Ideally I would be able to do something like tal_fmt("CREATE INDEX blah blah WHERE hstate NOT IN (%d, %d);;", RCVD_REMOVE_ACK_REVOCATION, SENT_REMOVE_ACK_REVOCATION) but that cannot work as this is a static data structure and this is C, sigh. C++ initializers...

I think BUILD_ASSERT would have to be outside the db_migrations array, too. And the point is not to check the numbers (it is already done elsewhere, as you note) but as a reminder to future readers of the code what these hardcoded 9 and 19 are. If BUILD_ASSERT has to be done outside of db_migrations, then as db_migrations grows, the explanation of the hardcoded 9 and 19 becomes more distant from the query. In that respect a comment is superior as it can sit right next to that query in perpetuam (and the check is already done elsewhere).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Then you should at least put a comment adjacent to the assertions in wallet/wallet.h to help future coders understand what they will be breaking if they violate the assertions.

We already have a warning comment in common/htlc_state.h for this, which is more important since this is where the constants are actually defined:

/*
* /!\ The generated enum values are used in the database, DO NOT
* reorder or insert new values (appending at the end is ok) /!\
*/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BUILD_ASSERT_OR_ZERO works:

/* example.c */
#include<stddef.h>

#define BUILD_ASSERT_OR_ZERO(cond) \
        (sizeof(char [1 - 2*!(cond)]) - 1)

enum example {
        EXAMPLE_ZERO,
        EXAMPLE_ONE
};

struct db_entry {
        const char *query;
        const char *foo;
};

struct db_entry db_migrations[] = {
        { "string"
        + BUILD_ASSERT_OR_ZERO(EXAMPLE_ZERO == 0)
        + BUILD_ASSERT_OR_ZERO(EXAMPLE_ONE == 1),
          NULL
        },
};

int main() {
        return 0;
}
$ gcc -c example.c

Still suboptimal though since we do not really want to assert, we want to not have to use hardcoded numbers in the source code...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Except clang hates it...

wallet/db.c:868:2: error: adding 'unsigned long' to a string does not append to the string [-Werror,-Wstring-plus-int]
        + BUILD_ASSERT_OR_ZERO( 9 == RCVD_REMOVE_ACK_REVOCATION)
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
wallet/db.c:868:2: note: use array indexing to silence this warning
        + BUILD_ASSERT_OR_ZERO( 9 == RCVD_REMOVE_ACK_REVOCATION)
        ^
        [                                                       ]

bleah.....

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about &"string"[BUILD_ASSERT_OR_ZERO(…)]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Current version already does that.

@ZmnSCPxj
Copy link
Collaborator Author

Modified to use BUILD_ASSERT_OR_ZERO Still non-ideal since ideally the numbers 9 and 19 should never appear in the source code.

Closes: ElementsProject#4901

Tested by `EXPLAIN QUERY PLAN` on sqlite3; ElementsProject#4901 shows the result from
@whitslack doing a similar partial index on PostgreSQL on his ~1000 chan
node.

ChangeLog-Added: db: Speed up loading of pending HTLCs during startup by using a partial index.
@cdecker
Copy link
Member

cdecker commented Dec 2, 2021

ACK ZmnSCPxj@7c63ce4

@cdecker cdecker merged commit 3433ff5 into ElementsProject:master Dec 2, 2021
@ZmnSCPxj ZmnSCPxj deleted the speedup-4901 branch December 2, 2021 12:29
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.

Adding partial index of unresolved HTLCs drastically reduces startup time
4 participants