sqlite: move first read into a transaction

[vrothberg]

According to an old upstream issue [1]: "If the first statement after BEGIN DEFERRED is a SELECT, then a read transaction is started. Subsequent write statements will upgrade the transaction to a write transaction if possible, or return SQLITE_BUSY."

So let's move the first SELECT under the same transaction as the table initialization.

[NO NEW TESTS NEEDED] as it's a hard to cause race.

[1] mattn/go-sqlite3#274 (comment)

Fixes: #17859

Does this PR introduce a user-facing change?

Fix a bug in the sqlite database backend where the first read access may fail.

[vrothberg]

@mheon @baude @edsantiago PTAL

[edsantiago]

I'm just having a lot of trouble with the overall structure here. I cringe a little at having functions operate on a transaction that their caller has to set up. I cringe a little at the BEGIN; migrate; init-tables; COMMIT flow. Maybe there are best-practice reasons that I don't know, but to me it seems cleaner to have:

   sqlite_state.go  --  simply runs InitTables()

   sqlite_state_internal.go:
      func InitTables() {
            BEGIN TRANSACTION
            migrate-if-necessary
            create tables as needed
            END TRANSACTION
     }

That is, keep the transaction and the schema-migration knowledge tightly clustered together instead of requiring the caller (sqlite_state.go) to synchronize the dance.

But I might just need more coffee. Stepping back, will reassess in a few hours.

[vrothberg]

but to me it seems cleaner to have

Sure, that sounds good to me as well. Waiting for feedback from @mheon, then I can wrap things up and repush.

[edsantiago]

Stepping back in with an observation: this would be a code flow that I find more natural:

    func InitTables() {
        schema := getSchema()
        if schema == CURRENT_SCHEMA
            return       // nothing to do. Don't even bother with the CREATE TABLE IF NOT EXIST

        if schema is not nil
            return migrateFromSchema(schema) // this is scary but deferred to future

        // fall through: database does not exist
        ...do the CREATE TABLE thing
    }

That fixes one concern I have: calling InitTables() after Migrate(). (that seems weird. Either Init, or Migrate, but never both). It could also speed things up a little by eliminating unnecessary CREATE TABLEs and transactions.

But it's very probable that I've missed something.

[mheon]

Current code arrangement LGTM, though Ed's point of combining the two into a single function definitely has merit (as does making InitTables only run if Migrate did not actually find and migrate a valid schema, though that seems like an optimization that can be done later).

Overall, I'm good to merge now if desired, but I would definitely at least want a TODO added about these optimizations, and I certainly would not object to seeing them in this PR.

[vrothberg]

Done ✔️ PTanotherL

[mheon]

LGTM
@containers/podman-maintainers PTAL

[edsantiago]

LGTM2. Thanks @vrothberg!

[EDIT: followup, fwiw: none of the strings 'is locked|unique constraint|constraint fail|config row|adding db|dbconfig.id' appear in the (downloaded, grep'ed) logs.]

[vrothberg]

merge me

[giuseppe]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [giuseppe,vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment