maxpert / marmot

A distributed SQLite replicator built on top of NATS
https://maxpert.github.io/marmot/
MIT License
1.81k stars 41 forks source link

sql: Scan error on column index 4, name \"pk\": sql/driver: couldn't convert 2 into type bool #48

Closed disarticulate closed 1 year ago

disarticulate commented 1 year ago

Trying to setup a example with a sufficiently complex DB: https://github.com/disarticulate/marmot/tree/master/example/single-leader to get a better idea of current NATS configuration.

Ran into the topic error. Not sure what the error is, but looks like pk refers to IsPrimaryKey booldb:"pk"in the queryquery := "SELECT name, type, notnull, dflt_value, pk FROM pragma_table_info(?)"` from here: https://github.com/maxpert/marmot/blob/master/db/sqlite.go

    if !hasPrimaryKey {
        tableInfo = append(tableInfo, &ColumnInfo{
            Name:         "rowid",
            IsPrimaryKey: true,
            Type:         "INT",
            NotNull:      true,
            DefaultValue: nil,
        })
    }

this stackoverflow: https://stackoverflow.com/questions/10472103/sqlite-query-to-find-primary-keys indicates the references pk column is not a bool, but a count of the number of keys in the primary key.

I assume the fix is to evaluate (unfortunately) IsPrimary as an array rather than bool.

maxpert commented 1 year ago

If I am understanding the question correctly. This particular case is for the tables when table doesn't have any primary key. So the IsPrimaryKey is the meta information for the column information stored in tableInfo. In case there are multiple primary keys, tableInfo will have multiple ColumnInfo with IsPrimaryKey set to true.

disarticulate commented 1 year ago

I think the error is assuming that IsPrimaryKey: true, can be converted to a boolean when SELECT name, type, notnull, dflt_value, pk FROM pragma_table_info(?) returns for pk a integer which can be interpreted as 0 for no pk, 1 for 1 pk (true) or 2+ for the number of primary keys. I did not look at how isPrimary key is used, but I would suspect there's logic that tracks the pk and if there's multiple pk one would need to account for that in the design.

maxpert commented 1 year ago

I might need a detailed repro or side-effect this is causing. Can you help me do that?

disarticulate commented 1 year ago

if you have docker compose, you should be able to just build this: https://github.com/disarticulate/marmot/tree/master/example/single-leader

That should pull the latest source. and the sqlite file is there.

thetooth commented 1 year ago

I managed to hit this on the first attempt at using marmot. The issue comes up when a table is configured with multiple primary keys or the primary key is not the first column. According to this pragma_table_info's pk value is a index of the column containing the primary key not a boolean. The following schema will cause the error and prevent marmot from starting:

CREATE TABLE "playlist_track" (
    "PlaylistId"    INTEGER NOT NULL,
    "TrackId"   INTEGER NOT NULL,
    FOREIGN KEY("PlaylistId") REFERENCES "playlists"("PlaylistId") ON DELETE NO ACTION ON UPDATE NO ACTION,
    FOREIGN KEY("TrackId") REFERENCES "tracks"("TrackId") ON DELETE NO ACTION ON UPDATE NO ACTION,
    CONSTRAINT "PK_PlaylistTrack" PRIMARY KEY("PlaylistId","TrackId")
);

I tested changing the type to an int and doing >0 checks. This seems to be sufficient to workaround the crash but someone who has used this software for more than a few hours should probably have a closer look.

maxpert commented 1 year ago

Perfect I will try it locally, and try to get fix out.

maxpert commented 1 year ago

I see how SQLite is putting index numbers instead of booleans, and I can reproduce it already. I have got a fix in place that should fix it let me put that up once a beta build once I've tested it locally.

maxpert commented 1 year ago

I have a build ready to test these changes out here. @thetooth can you try it and let me know if the changes fix the issue. I tried on my side and it works.

maxpert commented 1 year ago

@thetooth did you get a chance to test it on your side? I've been able to get it up and running without any issues.

thetooth commented 1 year ago

@maxpert not yet but changes look identical to my local branch, should be ready to go as I've not encountered any problems so far. Will resync onto tagged release next week and let you know if any problems come up.

maxpert commented 1 year ago

@thetooth the latest master has the changes now, feel free to use binaries from build or compile yourself and try.