fix(waku-store): added an index to improve messages query time #1120
Conversation
| @@ -76,7 +76,7 @@ proc createTable*(db: SqliteDatabase): DatabaseResult[void] {.inline.} = | |||
| ## Create index | |||
|
|
|||
| template createIndexQuery(table: string): SqlQueryStr = | |||
| "CREATE INDEX IF NOT EXISTS i_rt ON " & table & " (receiverTimestamp);" | |||
| "CREATE INDEX IF NOT EXISTS i_msg ON " & table & " (contentTopic, pubsubTopic, senderTimestamp, id);" | |||
| @@ -76,7 +76,7 @@ proc createTable*(db: SqliteDatabase): DatabaseResult[void] {.inline.} = | |||
| ## Create index | |||
|
|
|||
| template createIndexQuery(table: string): SqlQueryStr = | |||
| "CREATE INDEX IF NOT EXISTS i_rt ON " & table & " (receiverTimestamp);" | |||
| "CREATE INDEX IF NOT EXISTS i_msg ON " & table & " (contentTopic, pubsubTopic, senderTimestamp, id);" | |||
There was a problem hiding this comment.
Non-blocking:
The queried node MUST sort the WakuMessages based on their Index, where the senderTime constitutes the most significant part and the digest comes next, and then perform pagination on the sorted result. As such, the retrieved page contains an ordered list of WakuMessages from the oldest message to the most recent one.
ReceiverTimestampis guaranteed to be set, while senders could omit settingsenderTimestamp.
- Since the field is optional, where would messages without it end up in the resulting list? At the end?
- In case of a null value, isn't it worth fallbacking to
receiverTimestampwhen sorting, thus keeping it in the index? - Or shouldn't
senderTimestampbe required?
There was a problem hiding this comment.
- Since the field is optional, where would messages like that end up in the resulting list? At the end?
I am not sure about that. I need to review the current implementation.
- In case of a null value, isn't worth fallbacking to receiverTimestamp, thus keeping it in the index?
What I am thinking is to simplify the current implementation by keeping a single column with a timestamp.
The new column will be named storeTimestamp or storedAt. This timestamp will be set to senderTimesamp if present and to the epoch time value at the moment of message insertion. The message will be dropped and not inserted into the store if the message is "too old" (older than the current time minus ~10s (?)) or "from the future" (current time plus more than ~10s (?)).
Optionally we can keep the senderTimestamp column just in case we want to use it to extract metrics of the message propagation. But the reference timestamp column will be the storeTimestamp column for query purposes (and store restore functionality).
- Or shouldn't senderTimestamp be required?
IIRC For privacy-preserving reasons, the sender timestamp can be omitted. So makes no sense to make it mandatory.
|
As the load tests' results indicate a substantial improvement in the query times, I merge these changes into the |
This PR contains surgical changes to unblock Status's waku store testing. Some deficiencies have been identified during the analysis and will be addressed shortly.
The existing SQLite index of the messages table,
i_rt, was not helpful for the store queries.i_rtindexi_msg, that contains the fields relevant to the store query