Conversation
ExecTx commits inside a deferred closure that assigns to err. With an
unnamed result the assignment lands after `return err` has copied the
value, so a failed COMMIT is reported as success. The reconciler then
records a grant, schema, default privilege or role setting that the
server rolled back.
Commit at the end and defer only Rollback, the shape from the
database/sql docs. Nothing assigns to the result in a defer, so the bug
cannot recur. Move the transaction body into execTx, which takes a
*sql.DB, so sqlmock can drive it.
Closing the *sql.DB moves in front of BeginTx. Before, Close lived in
the closure deferred only after BeginTx succeeded, so a failed BeginTx
(bad credentials, unreachable host, cancelled context) leaked the pool
on every reconcile.
Testing: TestExecTx drives execTx with sqlmock through Begin, Exec,
later-Exec and Commit failures. The Commit case is the regression:
m.ExpectBegin()
m.ExpectExec("GRANT USAGE").WillReturnResult(ok)
m.ExpectCommit().WillReturnError(errBoom)
// want errBoom; the previous code returned nil
Reproduced on postgres:18 through the e2e harness. A GRANT cannot fail
at COMMIT by itself, so an event trigger writes a row whose deferred
constraint trigger raises when the transaction commits:
CREATE EVENT TRIGGER audit_ddl ON ddl_command_end
WHEN TAG IN ('GRANT') EXECUTE FUNCTION audit_ddl();
-- audit_ddl(): INSERT INTO ddl_audit(tag) VALUES (tg_tag)
CREATE CONSTRAINT TRIGGER reject_at_commit AFTER INSERT ON ddl_audit
DEFERRABLE INITIALLY DEFERRED FOR EACH ROW
EXECUTE FUNCTION reject_at_commit();
-- reject_at_commit(): RAISE EXCEPTION 'extx: COMMIT rejected'
A Grant of USAGE on a schema in that database then reconciles as:
BEGIN; REVOKE ...; GRANT USAGE ON SCHEMA extx_schema TO extx_role;
COMMIT; -- ERROR: extx: COMMIT rejected
Before: ExecTx returned nil, Synced=True, the privilege was absent and
Create re-ran every poll. After: Synced=False with reason ReconcileError
and the COMMIT error in the message, on both the cluster-scoped and the
namespaced pass.
Signed-off-by: Carl Henrik Lunde <chlunde@ifi.uio.no>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
There were two subtle bugs in ExecTx, the return value from Commit was assigned to err, which would only work if err was a named return variable. Additionally, the *sql.Conn handle including a goroutine would leak if BeginTx failed, like if there's an issue with the providerconfig.
To add testing we extract the main code to execTx and test using sqlmock. To remove the bug and make the code easier to understand and more standard, follow the code structure more like used in stdlib docs: https://pkg.go.dev/database/sql#Tx.Prepare
Fixes issue found by agent inspection.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Unit test, and also manual testing in e2e (not added here to keep runtime low) with a trigger causing the COMMIT to fail. It didn't return any error without this fix, synced became true.