Skip to content

database/sql: Strange Errors when Closing a Tx's Prepared Statement after Commit #4459

Closed
@gopherbot

Description

@gopherbot
Contributor

by johnkgallagher:

What steps will reproduce the problem?

This test function demonstrates the problem (note that this won't actually run on
play.golang.org because of the dependency on go-sqlite3):
http://play.golang.org/p/mKL22tAoxR

The code is wrong, but the errors it produces are very confusing and don't at all point
to the actual problem. On line 19, a transaction's prepared statement's Close() is
deferred. However, the transaction is committed before returning from the function,
which means the Close() happens after the Commit. Depending on the driver, this can
cause:

1. Silent lockup (observed with both go-sqlite3 and github.com/bmizerany/pq)
2. Very odd driver messages (pq reported nonsensical errors, like "expected 4
arguments, got 1" on a query that had 1 argument)
3. Memory corruption panic (observed with go-sqlite3 (cgo))

What is the expected output?

Nothing (success)

What do you see instead?

Sometimes success, more often either a lockup or "panic: runtime error: invalid
memory address or nil pointer dereference". Commenting out line 19 and uncommenting
line 29 fixes the problem.

Which compiler are you using (5g, 6g, 8g, gccgo)?

6g

Which operating system are you using?

Mac OS X, Linux

Which version are you using?  (run 'go version')

1.0.3

Please provide any additional information below.

I'm not sure this is necessarily a bug in database/sql, since the documentation does say
that a prepared statement can't be used after a transaction is committed/rolled back.
However, it would be nicer if it coped with this more gracefully, as it seemed
"natural" (although clearly wrong) to defer the Stmt.Close(). Any of the
following would have been preferable (to me, at least) than the debugging I did:

1. Transaction closes its open statements inside Commit/Rollback
2. Stmt.Close() panics if it's called outside the transaction
3. Stmt.Close() does nothing if it's called outside the transaction

Activity

bradfitz

bradfitz commented on Nov 29, 2012

@bradfitz
Contributor

Comment 1:

Owner changed to @bradfitz.

Status changed to Accepted.

rsc

rsc commented on Dec 10, 2012

@rsc
Contributor

Comment 2:

Labels changed: added size-m.

rsc

rsc commented on Dec 10, 2012

@rsc
Contributor

Comment 3:

Labels changed: added suggested.

rsc

rsc commented on Dec 30, 2012

@rsc
Contributor

Comment 4:

Labels changed: added priority-later, removed priority-triage.

gwenn

gwenn commented on Jan 12, 2013

@gwenn

Comment 5:

Hello,
For the sqlite driver, I've found where the issue comes from:
The prepared statements are closed twice:
 - when the Tx is committed, the underlying conn is closed and the sqlite driver tries to close dangling statements,
 - with the deferred Stmt.Close.
Just comment out the following block in the sqlite driver to do a quick and dirty fix:
func (c *SQLiteConn) Close() error {
    /*s := C.sqlite3_next_stmt(c.db, nil)
    for s != nil {
        C.sqlite3_finalize(s)
        s = C.sqlite3_next_stmt(c.db, s)
    }*/
    rv := C.sqlite3_close(c.db)
I will attempt to fix the driver.
Regards.
gwenn

gwenn commented on Jan 13, 2013

@gwenn

Comment 6:

Ok,
I've just submitted a patch to mattn that fix the issue on the driver side:
mattn/go-sqlite3#37
For postgresql, I don't have it installed.
If you ask me, I can install it and investigate...
Regards.
rsc

rsc commented on Mar 12, 2013

@rsc
Contributor

Comment 7:

Labels changed: added go1.1maybe, removed go1.1.

robpike

robpike commented on May 18, 2013

@robpike
Contributor

Comment 8:

Labels changed: added go1.2maybe, removed go1.1maybe.

rsc

rsc commented on Sep 9, 2013

@rsc
Contributor

Comment 9:

Labels changed: added go1.3maybe, removed go1.2maybe.

rsc

rsc commented on Dec 4, 2013

@rsc
Contributor

Comment 10:

Labels changed: added release-none, removed go1.3maybe.

rsc

rsc commented on Dec 4, 2013

@rsc
Contributor

Comment 11:

Labels changed: added repo-main.

gopherbot

gopherbot commented on Nov 2, 2014

@gopherbot
ContributorAuthor

Comment 12 by marko@joh.to:

This should be fixed in 1.4 by https://code.google.com/p/go/source/detail?r=50ce4ec65c4a.
added
SuggestedIssues that may be good for new contributors looking for work to do.
on Nov 2, 2014

6 remaining items

s7v7nislands

s7v7nislands commented on Nov 2, 2016

@s7v7nislands
removed their assignment
on Nov 8, 2016
bradfitz

bradfitz commented on Nov 8, 2016

@bradfitz
Contributor
kardianos

kardianos commented on Nov 8, 2016

@kardianos
Contributor

I'll look into it.

kardianos

kardianos commented on Nov 16, 2016

@kardianos
Contributor

Yes, this could be closed.

locked and limited conversation to collaborators on Nov 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    FrozenDueToAgeSuggestedIssues that may be good for new contributors looking for work to do.

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @bradfitz@gwenn@rsc@s7v7nislands@kardianos

        Issue actions

          database/sql: Strange Errors when Closing a Tx's Prepared Statement after Commit · Issue #4459 · golang/go