Conversation
|
Can you write a test? |
hifi
force-pushed
the
feature/opt-dbpage
branch
2 times, most recently
from
ba67c76 to
98169e3
Compare
|
Added a test that makes a full copy of a database using sqlite_dbpage table and checks the copy is consistent and has the expected data. |
|
@mattn Is there still something that needs to be done here? Thanks. |
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in build tag to compile SQLite with the SQLITE_ENABLE_DBPAGE_VTAB option, exposing the sqlite_dbpage virtual table for raw page access (useful for snapshot/rsync-style workflows).
Changes:
- Add
sqlite_dbpagebuild tag that enablesSQLITE_ENABLE_DBPAGE_VTABat compile time. - Add a test that exercises reading pages via
sqlite_dbpageand validating the copied DB. - Document the new tag in the README and include it in the CI “full tags” job.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
sqlite3_opt_dbpage.go |
Introduces sqlite_dbpage build tag and enables SQLITE_ENABLE_DBPAGE_VTAB. |
sqlite3_opt_dbpage_test.go |
Adds a test that reads sqlite_dbpage output and writes a copied DB for integrity verification. |
README.md |
Adds sqlite_dbpage to the feature/build-tag list with a link to SQLite docs. |
.github/workflows/go.yaml |
Includes sqlite_dbpage in the “full” tag set for CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if _, err := destFile.Write(page); err != nil { | ||
| t.Fatal("Unable to write page to file:", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The loop over rows.Next() should check rows.Err() after iteration to catch any I/O/scan errors that occur during row streaming.
| } | |
| } | |
| if err := rows.Err(); err != nil { | |
| t.Fatal("Error iterating over rows:", err) | |
| } |
sqlite3_opt_dbpage_test.go
Outdated
| } | ||
| defer rows.Close() | ||
|
|
||
| destFile, err := os.OpenFile(destFilename, os.O_CREATE|os.O_WRONLY, 0700) |
There was a problem hiding this comment.
destFilename is created by TempFilename already; opening with os.O_CREATE|os.O_WRONLY won't truncate if the file is non-empty. Consider using os.Create or include os.O_TRUNC, and use a more typical permission like 0600 for a test artifact (0700 sets the executable bit).
| destFile, err := os.OpenFile(destFilename, os.O_CREATE|os.O_WRONLY, 0700) | |
| destFile, err := os.OpenFile(destFilename, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0600) |
| if err := rows.Close(); err != nil { | ||
| t.Fatal("Unable to close rows:", err) | ||
| } else if err := db.Close(); err != nil { | ||
| t.Fatal("Unable to close database:", err) | ||
| } else if err := destFile.Close(); err != nil { | ||
| t.Fatal("Unable to close file:", err) | ||
| } |
There was a problem hiding this comment.
This test mixes defer Close() with explicit Close() calls later. That results in double-closes (especially for the file), and the deferred closes ignore any error. Prefer a single close strategy (typically defers that check errors in a deferred func) to avoid masking issues and reduce confusion.
| | Tracing / Debug | sqlite_trace | Activate trace functions | | ||
| | User Authentication | sqlite_userauth | SQLite User Authentication see [User Authentication](#user-authentication) for more information. | | ||
| | Virtual Tables | sqlite_vtable | SQLite Virtual Tables see [SQLite Official VTABLE Documentation](https://www.sqlite.org/vtab.html) for more information, and a [full example here](https://github.com/mattn/go-sqlite3/tree/master/_example/vtable) | | ||
| | SQLITE_DBPAGE Virtual Table | sqlite_dbpage | SQLITE_DBPAGE Virtual Table see [SQLite Official SQLITE_DBPAGE Documentation](https://www.sqlite.org/dbpage.html) for more information. | |
There was a problem hiding this comment.
Enabling SQLITE_ENABLE_DBPAGE_VTAB allows reading and (on newer SQLite versions) writing/deleting raw pages via SQL, which is unsafe when executing untrusted SQL. Consider adding a short warning in this README entry about the security implications and that it should only be enabled for trusted use-cases.
| for rows.Next() { | ||
| var page []byte | ||
| if err := rows.Scan(&page); err != nil { | ||
| t.Fatal("Unable to scan results:", err) | ||
| } | ||
|
|
||
| if _, err := destFile.Write(page); err != nil { | ||
| t.Fatal("Unable to write page to file:", err) | ||
| } |
There was a problem hiding this comment.
Appending each page to the output file assumes pages are returned contiguously starting at page 1. To reliably reconstruct a database, capture pgno and write each page at offset (pgno-1)*pageSize (and truncate the file first), rather than using a plain sequential Write.
sqlite3_opt_dbpage_test.go
Outdated
| rows, err := db.Query("SELECT data FROM sqlite_dbpage") | ||
| if err != nil && err.Error() == "no such table: sqlite_dbpage" { | ||
| t.Skip("sqlite_dbpage not supported") | ||
| } else if err != nil { |
There was a problem hiding this comment.
The error check err.Error() == "no such table: sqlite_dbpage" is brittle (message text can vary). Prefer using errors.As to sqlite3.Error and/or a substring match for the specific condition you're skipping on.
Provides raw access to database pages. Useful for taking consistent live snapshots of WAL databases without causing write locks by reading out all of the pages and writing them either to a file or some external storage.
The sqlite3_rsync utility that is bundled with sqlite3 source is one way to use it to somewhat efficiently synchronize a remote database.
Fixes #1288