[sql39]: Actions schemas, queries and SQL store impl #1072

ellemouton · 2025-05-13T12:59:14Z

This is the final PR for part 1 of #917 🎉

In this PR, we introduce the schema for the actions table. See the diagram in #917 for how this fits in with regard
to our other tables (namely sessions & accounts).

We then implement the SQL version of the Actions store using the new queries.

Finally, our dev build is updated to use the SQL version of the actions table.

ViktorTigerstrom

Generally this looks great, really awesome work 🔥🚀!!

Leaving some comments below, where I think the most substantial comment is in regards to the actual usage of the indexes.

db/sqlc/migrations/000005_actions.up.sql

ViktorTigerstrom · 2025-05-23T18:28:42Z

db/sqlc/queries/actions.sql

+OFFSET @num_offset;
+
+-- name: ListActions :many
+SELECT a.*


Hmmm I'm not sure that the indexes will end up getting with these queries, due to the "OR IS NULL" clauses for all of the index filter params. It requires some more investigation if it's the case or not, but if this is an issue, we should consider if it's worth creating separate queries that's being used when for example the sessionID is not nil at the call site. That way, we'd ensure that the indexes actually gets used.

Just for some reference, I copied the table + this query into GPT and asked if the indexes will gets used. I'm posting the response below:

gpt convo

Because each filter is written as

(column = :param OR :param IS NULL)

PostgreSQL’s planner can only turn that into an index‐based lookup when it can prove, at plan‐time, that :param is non‐NULL and that the condition simplifies to a simple column = constant. As soon as you wrap it in an OR … IS NULL, you introduce uncertainty:

For a single invocation with literal parameters

If you prepare the statement and bind, say, session_id = 42, then

(a.session_id = 42 OR 42 IS NULL)

is semantically equivalent to a.session_id = 42, so in principle the actions_session_id_idx can be used for that clause.

When the planner builds a “generic” plan

PostgreSQL distinguishes custom plans (where it knows your parameter values at plan‐time) from generic plans (where it has to pick one plan for all future binds).

By default, after a few executions it switches to a generic plan, where it doesn’t know whether each :param IS NULL will turn on or off the filter. In that case it almost always chooses a sequential scan, because it can’t assume you’ll eliminate most rows.

Multiple optional filters multiply the uncertainty

You have six of these (… OR … IS NULL) filters all ANDed together. Even if one of them would by itself use an index, the planner sees the whole WHERE clause as “maybe trivial, maybe selective” and still tends to fall back to seq‐scan.

Dynamic ordering makes it worse

The CASE WHEN :reversed … END DESC, CASE WHEN NOT :reversed … END ASC trick also prevents an “index‐only scan in index‐order” on created_at_idx; PostgreSQL won’t match that to a simple ORDER BY created_at ASC/DESC.

Bottom line

In custom plans (i.e. if the planner does re-optimize on each bind, or if you force plan_cache_mode = force_custom_plan), then when you supply non-NULL parameters each (col = constant OR constant IS NULL) really does reduce to col = constant, and the corresponding single-column index can be used.

In generic plans (the default after a few executions of a prepared statement), the planner assumes the worst (that your filters might all be turned off), so it chooses a sequential scan and then a regular sort.

How to guarantee index usage

Generate the SQL dynamically so you only include WHERE session_id = … clauses for the parameters you actually passed. No ORs, no uncertainty — the planner sees plain equality and happily uses the index.

Use a stored procedure with EXECUTE format(). Inside PL/pgSQL you can build the WHERE list piecewise and then EXECUTE the resulting text; the final plan is always custom.

Split into two code‐paths:

One static SQL with all parameters NOT NULL (index usage guaranteed).

One static SQL with no filters (trivial SELECT * … ORDER BY …; index usage isn’t needed).

Partial indexes or multicolumn indexes won’t help here unless you know in advance which combinations of columns you’ll filter on.

TL;DR

Will the indexes actually end up getting used?
Not reliably. With the (col = :param OR :param IS NULL) pattern inside a prepared statement, Postgres will usually fall back to a sequential scan and a regular sort—unless you force it to rebuild a custom plan each time or rewrite the query to eliminate the OR … IS NULL out of your WHERE clause.

(edited your comment to put the gpt response in a drop down fyi)

ok so turns out: postgres correctly uses the index here, but sqlite does not - so good catch!

went with creating these queries manually instead

db/sqlc/migrations/000005_actions.up.sql

firewalldb/actions_sql.go

firewalldb/test_sql.go

ellemouton

Thanks @ViktorTigerstrom 🙏

db/sqlc/migrations/000005_actions.up.sql

ellemouton · 2025-05-27T07:32:38Z

db/sqlc/queries/actions.sql

+OFFSET @num_offset;
+
+-- name: ListActions :many
+SELECT a.*


(edited your comment to put the gpt response in a drop down fyi)

ellemouton · 2025-05-27T13:24:51Z

db/sqlc/queries/actions.sql

+OFFSET @num_offset;
+
+-- name: ListActions :many
+SELECT a.*


ok so turns out: postgres correctly uses the index here, but sqlite does not - so good catch!

went with creating these queries manually instead

db/sqlc/migrations/000005_actions.up.sql

bitromortac

Very cool and impressive work! So happy to see all schemas implemented and running against unit tests 🎉.

db/sqlc/migrations/000005_actions.up.sql

db/sqlc/migrations/000005_actions.down.sql

db/sqlc/actions_custom.go

bitromortac · 2025-05-28T09:20:55Z

db/sqlc/actions_custom.go

+	SessionID   sql.NullInt64
+	AccountID   sql.NullInt64
+	FeatureName sql.NullString
+	ActorName   sql.NullString
+	RpcMethod   sql.NullString
+	State       sql.NullInt16
+	EndTime     sql.NullTime
+	StartTime   sql.NullTime
+	GroupID     sql.NullInt64


something that could be cool would be to add a unit test that uses reflection and field tagging to check that field names and types are a subset of and consistent with models.Action, such that any changes in the schema could be detected. one could tag EndTime, StartTime, and GroupID as not being part of Action

existing unit tests would break no?

I'm not sure every field is covered in unit tests, sqlc does all the renaming automatically if a field's name changes, but we would have to manually change the representations in the query and would only fail once we add a unit test. Was just an idea, but not a blocker :)

firewalldb/kvstores_sql.go

firewalldb/actions_sql.go

firewalldb/db.go

In this commit we define the schema for the `actions` table along with various queries we will need for interacting with the table. NOTE: we will also add some of our own queries manually in commits to follow.

bitromortac

LGTM 🚀

db/sqlc/actions_custom.go

firewalldb/actions_sql.go

Here, we manually define some queries for the actions store. We do this so that we can manually build the "SELECT" and only add "WHERE" clauses that are actually needed for the query and hence ensure that available indexes are used.

In this commit we let the firewalldb.SQLDB implement the ActionsDB. We also ensure that all the action unit tests now run against the SQL impl.

ViktorTigerstrom

Very nice changes!! Awesome job 🎉🔥

ViktorTigerstrom · 2025-06-02T10:10:01Z

db/sqlc/actions_custom.go

@@ -0,0 +1,210 @@
+package sqlc


Very nice approach with the addition of this file 🎉🔥!

ViktorTigerstrom · 2025-06-02T10:15:10Z

firewalldb/actions_sql.go

+	if errReason != "" && state != ActionStateError {
+		return fmt.Errorf("error reason should only be set for " +
+			"ActionStateError")
+	}


Nit: As a follow-up to @bitromortac's feedback below, we could potentially also check that an errReason is actually set when state == ActionStateError, though that may be a bit dangerous to require that the middleware interceptor sets an error for all responses marked as "errors". So therefore I think it's probably safer to leave it as is.

ellemouton self-assigned this May 13, 2025

ellemouton added the sql-ize label May 13, 2025

ellemouton marked this pull request as draft May 13, 2025 12:59

ellemouton added the no-changelog This PR is does not require a release notes entry label May 13, 2025

ellemouton force-pushed the sql39 branch from c7a0b45 to 00c061f Compare May 13, 2025 13:33

ellemouton force-pushed the sql39 branch 2 times, most recently from 00a6ae4 to 7324726 Compare May 22, 2025 11:54

ellemouton marked this pull request as ready for review May 22, 2025 11:54

ellemouton requested review from bitromortac and ViktorTigerstrom May 22, 2025 12:13

ViktorTigerstrom reviewed May 23, 2025

View reviewed changes

ellemouton mentioned this pull request May 27, 2025

[sql-40] firewalldb: action store prep commits #1078

Merged

ellemouton changed the base branch from master to sql40 May 27, 2025 13:38

ellemouton force-pushed the sql39 branch from 7324726 to 8892c49 Compare May 27, 2025 13:39

ellemouton commented May 27, 2025

View reviewed changes

ellemouton requested a review from ViktorTigerstrom May 27, 2025 16:05

bitromortac reviewed May 28, 2025

View reviewed changes

ellemouton force-pushed the sql39 branch from 8892c49 to 7bb6383 Compare May 28, 2025 12:14

ellemouton requested a review from bitromortac May 28, 2025 12:14

ellemouton force-pushed the sql39 branch from 7bb6383 to be2f8aa Compare May 28, 2025 12:29

ellemouton changed the base branch from sql40 to master May 29, 2025 05:04

db: add actions schemas and queries

65e4309

In this commit we define the schema for the `actions` table along with various queries we will need for interacting with the table. NOTE: we will also add some of our own queries manually in commits to follow.

ellemouton force-pushed the sql39 branch from be2f8aa to 7ef1699 Compare May 29, 2025 05:04

bitromortac approved these changes May 30, 2025

View reviewed changes

ellemouton added 3 commits May 30, 2025 10:54

db: define manual action SQL queries

1674490

Here, we manually define some queries for the actions store. We do this so that we can manually build the "SELECT" and only add "WHERE" clauses that are actually needed for the query and hence ensure that available indexes are used.

firewalldb: add SQL impl of Actions store

3f7ae5b

In this commit we let the firewalldb.SQLDB implement the ActionsDB. We also ensure that all the action unit tests now run against the SQL impl.

lit: plug the SQL actions DB into the dev build

89b807c

ellemouton force-pushed the sql39 branch from 7ef1699 to 89b807c Compare May 30, 2025 08:57

ViktorTigerstrom approved these changes Jun 2, 2025

View reviewed changes

ellemouton merged commit cad67fc into lightninglabs:master Jun 2, 2025
22 checks passed

ellemouton deleted the sql39 branch June 2, 2025 11:34

[sql39]: Actions schemas, queries and SQL store impl #1072

[sql39]: Actions schemas, queries and SQL store impl #1072

Conversation

ellemouton commented May 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ViktorTigerstrom May 23, 2025 • edited by ellemouton Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Bottom line

How to guarantee index usage

TL;DR

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ellemouton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bitromortac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ViktorTigerstrom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ellemouton commented May 13, 2025 •

edited

Loading

ViktorTigerstrom May 23, 2025 •

edited by ellemouton

Loading