sqlite: add optional ConnLogger #111

alisdair · 2025-10-22T22:55:08Z

ConnLogger can be used to log executed statements for a connection. The interface captures events for Begin, Exec, Commit, and Rollback calls.

Updates tailscale/corp#33577

bradfitz · 2025-10-22T23:49:02Z

sqlite.go

+// ConnLogger is implemented by the caller to support statement-level logging for
+// write transactions. Only Exec calls are logged, not Query calls, as this is
+// intended as a mechanism to replay failed transactions.
+type ConnLogger interface {
+	// Begin is called when a writable transaction is opened.
+	Begin()
+
+	// Statement is called with evaluated SQL when a statement is executed.
+	Statement(sql string)
+
+	// Commit is called when a transaction successfully commits.
+	Commit()
+
+	// Rollback is called when a transaction is rolled back.
+	Rollback()
+}


I'm not sure this gives us enough information for what we'd want to log.

I want to see separate log events for beginning to commit vs commit done and its result, possibly including the WAL growth. Well, we track that per-Tx WAL growth already in cfgdb, so I guess we have enough to stich that in, but I think we'd at least need here for the Commit method above to take an error parameter for what we returned to the caller, and then update the docs to say it's not just about successful commits. Or Rollback needs an error perhaps, where a nil error means it was an explicit Rollback from user code, as opposed to a failed commit resulting in a rollback?

Other misc thought: maybe Begin should return some comparable TxID that the other 3 methods also receive, for lining stuff up. (but maybe not needed if we in practice serialize all writes... but maybe we're not?)

I am not against having more debugging data, but I don't think I understand why this API is insufficient for logging all committed write transactions? If a logger resets its state on Begin and only logs accumulated statements upon receiving Commit, do we care about unsuccessful commits or rollbacks at all? For the task at hand, I am not sure we even need the Rollback callback.

Same thoughts about TxID - I don't mind more details, but if a single conn can only have one transaction at a time, the logger itself can assign an id when Begin is called, and attribute all subsequent statements to that ID, right?

I've added an error parameter to Commit.

icio · 2025-10-23T09:38:44Z

sqlite.go

+// ConnLogger is implemented by the caller to support statement-level logging for
+// write transactions. Only Exec calls are logged, not Query calls, as this is
+// intended as a mechanism to replay failed transactions.
+type ConnLogger interface {


We probably want some kind of Close() method to ensure the file is flushed to disk when we exit.

I don't think we want Close here, although of course you're right that we need to flush the logs to disk on exit. In the corresponding PR, I've added a Close handler which is attached to the database shutdown process. Thanks for spotting it!

sqlite_test.go

knyar · 2025-10-23T11:13:39Z

sqlite.go

+// ConnLogger is implemented by the caller to support statement-level logging for
+// write transactions. Only Exec calls are logged, not Query calls, as this is


As implemented, I think this would log read transactions too? I took a stab at skipping the logger for readonly transactions: 112a07d

Thanks! Merged your commit into mine.

knyar · 2025-10-23T11:32:55Z

sqlite.go

+// ConnLogger is implemented by the caller to support statement-level logging for
+// write transactions. Only Exec calls are logged, not Query calls, as this is
+// intended as a mechanism to replay failed transactions.
+type ConnLogger interface {
+	// Begin is called when a writable transaction is opened.
+	Begin()
+
+	// Statement is called with evaluated SQL when a statement is executed.
+	Statement(sql string)
+
+	// Commit is called when a transaction successfully commits.
+	Commit()
+
+	// Rollback is called when a transaction is rolled back.
+	Rollback()
+}


I am not against having more debugging data, but I don't think I understand why this API is insufficient for logging all committed write transactions? If a logger resets its state on Begin and only logs accumulated statements upon receiving Commit, do we care about unsuccessful commits or rollbacks at all? For the task at hand, I am not sure we even need the Rollback callback.

Same thoughts about TxID - I don't mind more details, but if a single conn can only have one transaction at a time, the logger itself can assign an id when Begin is called, and attribute all subsequent statements to that ID, right?

bradfitz · 2025-10-24T14:46:36Z

sqlite.go

 type connector struct {
 	name         string
 	tracer       sqliteh.Tracer
+	makeLogger   func() ConnLogger


bradfitz · 2025-10-24T14:50:02Z

sqlite.go

 	if tx.conn.tracer != nil {
 		tx.conn.tracer.Commit(tx.conn.id, err)
 	}


This Tracer interface is what I was remembering with its type TraceConnID it plumbs through.

It's kinda weird having both Tracer and Logger that are such overlaps. Oh well.

I guess Tracer has the SQL with placeholders and Logger has it without? Might be worth documenting on both the relationship between the two.

dsnet · 2025-10-24T22:29:51Z

sqlite.go

 	}
 }

+func ConnectorWithLogger(sqliteURI string, connInitFunc ConnInitFunc, tracer sqliteh.Tracer, makeLogger func() ConnLogger) driver.Connector {


Document when makeLogger is called (i.e., "when Connect is called")?

ConnLogger can be used to log executed statements for a connection. The interface captures events for Begin, Exec, Commit, and Rollback calls. Updates tailscale/corp#33577 Co-authored-by: Anton Tolchanov <[email protected]>

alisdair force-pushed the alisdair/connlogger branch from c5f7361 to 7572674 Compare October 22, 2025 22:58

alisdair requested review from andrew-d, bradfitz, codekitchen and raggi October 22, 2025 23:15

bradfitz reviewed Oct 22, 2025

View reviewed changes

icio reviewed Oct 23, 2025

View reviewed changes

knyar reviewed Oct 23, 2025

View reviewed changes

alisdair force-pushed the alisdair/connlogger branch 2 times, most recently from 2ebf665 to b07a594 Compare October 23, 2025 23:00

knyar approved these changes Oct 24, 2025

View reviewed changes

bradfitz reviewed Oct 24, 2025

View reviewed changes

alisdair force-pushed the alisdair/connlogger branch from b07a594 to 94eb142 Compare October 24, 2025 16:28

dsnet approved these changes Oct 24, 2025

View reviewed changes

sqlite: add optional ConnLogger

731f626

ConnLogger can be used to log executed statements for a connection. The interface captures events for Begin, Exec, Commit, and Rollback calls. Updates tailscale/corp#33577 Co-authored-by: Anton Tolchanov <[email protected]>

alisdair force-pushed the alisdair/connlogger branch from 94eb142 to 731f626 Compare October 27, 2025 13:19

alisdair merged commit 35d6745 into main Oct 27, 2025
2 checks passed

alisdair deleted the alisdair/connlogger branch October 27, 2025 13:35

		// ConnLogger is implemented by the caller to support statement-level logging for
		// write transactions. Only Exec calls are logged, not Query calls, as this is

sqlite: add optional ConnLogger #111

sqlite: add optional ConnLogger #111

Uh oh!

Conversation

alisdair commented Oct 22, 2025

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants