truncate() on SQLite does not reset the id value in the sqlite_sequence any longer

[lehni]

Upon upgrading to 0.14.0, I've realised that my primary keys don't get reset anymore when I use truncate() with the SQLite dialect.

It looks like the code that's supposed to do the reseting in truncate() is actually producing an error:

https://github.com/tgriesser/knex/blob/bf8aa4db100ed34756d1d77268e3b391d5d2b528/src/dialects/sqlite3/query/compiler.js#L89-L99

If you replace noop there with (err) => { console.log(err) }, you will see the errors logged:

{ Error: SQLITE_ERROR: no such column: dummy errno: 1, code: 'SQLITE_ERROR’ }

It looks like the table-name should be quoted in that SQL statement. If I manually add quotes, then the ids get reset again:

sql: `delete from sqlite_sequence where name = "${table}"`

I'm not able to say if that's the correct fix for this issue.

[elhigu]

Thanks for report, looks like there are various places where identifiers are not wrapped properly.

[richraid21]

This works on previous versions?

[elhigu]

@richraid21 No. Those plain string replacements has been always there.

[lehni]

Well... truncate() actually did work for me before, as in it was resetting the ids without problems until I upgraded to 0.14.0. How it achieved this I cannot tell, but I didn't become aware of this problem until the upgrade.

[elhigu]

Ah, ok it might have something to do with the change where sqlite identifier quoting was changed from "to backtick `. Old " (#2087).

with " -style quotes, non existent column "iDontExist" was interpret as string, instead of throwing error. (#1048).

[elhigu]

Did you check the query (sql string) was throwing the { Error: SQLITE_ERROR: no such column: dummy errno: 1, code: 'SQLITE_ERROR’ } error? I was not able to reproduce this.

I mean it really seems to set quotes correctly https://runkit.com/embed/g4idd055djit (I suppose this.tableName already had quotes around it).

Maybe the code throws an error because some column / table which actually didn't exist earlier, now throws an error (check last comment in #1048)? And if you add "${tableName}" then sqlite doesn't throw an error anymore because "`unknownIdentifier`" actually is interpret as string `unknownIdentifier`.

Does this make sense?

[lehni]

Hmm I'm not sure I understand you correctly. Back-ticks aren't the right quotes for this statement. The table names are strings in the `sqlite_sequence` table's `name` column, so running this doesn't work:

delete from sqlite_sequence where name = `table_name`

This works as expected:

delete from sqlite_sequence where name = "table_name"

Using back-ticks makes SQLite think that `table_name` is a column name to match against within the same `sqlite_sequence` table, while we really want to match against the actual name of the table, as it's stored in the `sqlite_sequence` table's `name` column, e.g.:

select * from sqlite_sequence:

name	seq
knex_migrations	15
table_name	5

And regardless of this, there is no guarantee really that this query is executed before we use the table again in another query, because nothing is waiting for it t complete, therefore the reset might currently occur too late. Even though that's rather unlikely, this issue should also be addressed with a fix that waits for the 2nd sql statement to complete too.

[lehni]

I think the right thing to do is:

• Use "${this.single.table}" instead of ${this.tableName}
• Find a way to wait for this 2nd query to complete

[elhigu]

@lehni You are right, I didn't thought that table name as column value. Shouldn't ' quotes be the correct one in this case then like is used normally with string literals? But yeah, now I understand why it fails with knex 0.14.

[lehni]

I'm not familiar enough with it to say which quotes are best. Maybe it's better to pass the value as an actual binding to the sql statement?

[elhigu]

Yep, that would be the best option 👌

[lehni]

I can give this a go but I have no idea what to do about the two queries and the promise of the 2nd.

Could the queries simply be joined by ;?

[lehni]

I also can't tell if this.single.table is correct in every situation. It works for me, but I'm not using schemas (not even sure if SQLite supports them). And I'm by no means enough aware of Knex' internals.

[lehni]

Also, shouldn't these two queries really run in one transaction, so that if something goes wrong, both are undone and an error is produced instead of just swallowed?

[elhigu]

@lehni they shouldn't be forced to transaction unless it is initialized by the user. Both queries should be able to be done by .with query. As far as I know it should be supported by sqlite. So first one does delete query in .with part and then in real query part just reset the sequence.

[lehni]

Hmm but I don't think I can call .with() there?

truncate() needs to return an objection with a sql property which is a string. So do mean something like this?

with `truncate` as (delete from `${table_name}`) delete from sqlite_sequence where name = '${table_name}'

[lehni]

Also, I don't think the statement in the parentheses of a with statement can be a delete command. I think it needs to be a select statement:

near "delete": syntax error: with `truncate` as (delete

[lehni]

And lastly, isn't truncate() supposed to return the amount of deleted rows?

[elhigu]

I was trying out some sqlite CTEs and indeed it seems that with sqlite one cannot do any updates in those... I need to look more to it. About the returned value, I suppose count if fine, probably most of the dialects just return something truthy.

[lehni]

I'd like to suggest two measures here:

1. Roll out a quick fix for 0.14.1 0.14.2 that restores 0.13.0 behavior, by fixing the delete statement, without fixing the issue of the 2nd SQL statement not being waited on. This wasn't an issue for anyone so far, so it seems to be fine, perhaps simply a question of timing working out in our favor.
2. Find a fix for this more complex problem with a bit more time, and a proper plan

[elhigu]

That would be good start. I got no problem with that.

[lehni]

Ok, I will tackle this now... First a simple commit to address 1), and I have some ideas for 2) as well:

What if sql in the return value of truncate() was allowed to either be a string or an array of strings? And the code that runs the sql always converts to an array, and uses Promise.all() to wait for the results, plus a convention that only the result of the first query is returned?

[lehni]

It also looks like the SQLite binding doesn't currently support schema, which they could / should? That would be a separate issue: https://www.sqlite.org/lang_attach.html

[lehni]

Here the proposed fix for 1) now: 9b724fb

[lehni]

🎉