Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQLiteRows.Columns() returns outdated state #1284

Open
ganigeorgiev opened this issue Oct 7, 2024 · 4 comments
Open

SQLiteRows.Columns() returns outdated state #1284

ganigeorgiev opened this issue Oct 7, 2024 · 4 comments

Comments

@ganigeorgiev
Copy link

ganigeorgiev commented Oct 7, 2024

This is a little difficult to explain so I apologize if it seems confusing.

The problem is that when there 2+ live connections and 1 of the connection changes the schema, calling rows.Columns() from any of the other connections return an outdated list of columns (aka. the state before the schema change).

Below is a minimal reproducible:

package main

import (
	"database/sql"
	"log"
	"os"

	_ "github.com/mattn/go-sqlite3"
	// note: uncomment to test with the pure go port (the driver name is "sqlite")
	// _ "modernc.org/sqlite"
)

func main() {
	os.Remove("./foo.db")

	db1, err := sql.Open("sqlite3", "./foo.db")
	if err != nil {
		log.Fatal(err)
	}
	defer db1.Close()

	db2, err := sql.Open("sqlite3", "./foo.db")
	if err != nil {
		log.Fatal(err)
	}
	defer db2.Close()

	// ensure that there is a live db1 connection
	if err = db1.Ping(); err != nil {
		log.Println("failed to ensure db1 live connection", err)
		return
	}

	// create the dummy schema
	_, err = db1.Exec("CREATE TABLE foo (id INTEGER PRIMARY KEY, title TEXT)")
	if err != nil {
		log.Println("create table failure:", err)
		return
	}

	// ensure that there is a live db2 connection AFTER the table creation
	if err = db2.Ping(); err != nil {
		log.Println("failed to ensure db2 live connection", err)
		return
	}

	// rename for example the column through one of the connections
	_, err = db1.Exec("ALTER TABLE foo RENAME COLUMN title TO title_new")
	if err != nil {
		log.Println("column rename failure:", err)
		return
	}

	// apparently running PRAGMA quick_check, optimize, etc. seems to fix the sync state
	// db2.Exec("PRAGMA quick_check")

	rows, err := db2.Query("select * from foo")
	if err != nil {
		log.Println("select failure:", err)
		return
	}
	defer rows.Close()

	columns, _ := rows.Columns()

	log.Println("Expected [id, title_new], got", columns)
}
@rittneje
Copy link
Collaborator

rittneje commented Oct 7, 2024

Each time you open a database connection with sqlite3_open_v2 (which is what Ping does), SQLite itself (the C library) will parse the database schema and cache it.

When you call db.Query, that calls sqlite3_prepare_v2, which parses the query against the cached schema, but does not yet realize that the cache is stale (because you changed the schema from another connection).

When you call db.Columns, that calls sqlite3_column_name, which uses the values from the parsed query, again not yet realizing the cache is stale.

In order to resolve this, you have to call sqlite3_step, which causes it finally realize the cache is stale and automatically re-parse the schema. This is achieved by calling db.Exec, db.QueryRow, or rows.Next.

Unfortunately, the other contributing factor is the fact that this driver itself caches the column names for a given result set from the first call to rows.Columns. And database/sql calls that when you call rows.Next.

In short, you'd need to first fix this driver to stop caching the column names. But even if you fixed that, you'd still need to call rows.Next once in order to force SQLite to notice the schema change. And rows.Columns cannot be called once rows.Next returns false. So there is no general solution to this problem other than the one you found, which is to exec some arbitrary command first, in order to force SQLite to detect schema changes.

@ganigeorgiev
Copy link
Author

That's OK, we call rows.Next() internally, it is just skipped from the minimal reproducible example because there are no entries in the table (for more context about the origin of the issue you can check pocketbase/pocketbase#5623 (reply in thread)).

With that said, I don't think the current driver columns caching behavior is correct (it differs from other drivers and the above works as expected with modernc.org/sqlite).

@rittneje
Copy link
Collaborator

rittneje commented Oct 7, 2024

I'm not personally opposed to you removing the caching from this driver. Just be advised that if your query returns no rows, there is no way to get the column names with that approach.

@ganigeorgiev
Copy link
Author

ganigeorgiev commented Oct 7, 2024

@rittneje I'm not going to remove anything. I'm just reporting an issue.

And again, we don't call directly rows.Columns() or need its result if there are no rows. The stale cache in fact causes issues only when there are entries that need to be scanned. The above code sample is just a minimal reproducible example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants