tursodatabase / libsql

libSQL is a fork of SQLite that is both Open Source, and Open Contributions.
https://turso.tech/libsql
MIT License
9.35k stars 244 forks source link

Setting `legacy_alter_table` works in shell but not in sqld #1547

Open simolus3 opened 2 months ago

simolus3 commented 2 months ago

The legacy_alter_table pragma is required to reliably implement the suggested procedure for other kinds of schema changes without manually tracking views. Unfortunately, setting this pragma fails in Turso. I thought that this may perhaps be caused by compile-time options disabling legacy pragmas, but it's available in libsql-sqlite3:

$ ./libsql-sqlite3/libsql
-- Loading resources from /home/simon/.sqliterc
libSQL version 0.2.3 (based on SQLite version 3.44.0) 2023-11-01 11:23:50
Enter ".help" for usage hints.
Connected to a transient in-memory database.
Use ".open FILENAME" to reopen on a persistent database.
libsql> pragma legacy_alter_table = 1;
libsql> .q

With a sqld build that should use the same sqlite3 build, it doesn't work:

$ ./target/debug/sqld &
$ turso db shell http://127.0.0.1:8080
Connected to http://127.0.0.1:8080

Welcome to Turso SQL shell!

Type ".quit" to exit the shell and ".help" to list all available commands.

→  pragma legacy_alter_table = 1;
Error: failed to execute SQL: pragma legacy_alter_table = 1;
SQL string could not be parsed: unsupported statement: pragma legacy_alter_table = 1
avinassh commented 1 month ago

hey, it disabled by purpose in libsql server - https://github.com/tursodatabase/libsql/blob/b769c6c/libsql/src/parser.rs#L155

let me find out why we have done this in the first place and if we can allow it

avinassh commented 1 month ago

@simolus3 do you really this need though?

They don't recommend this:

This pragma is provided as a work-around for older programs that contain code that expect the incomplete behavior of ALTER TABLE RENAME found in older versions of SQLite. New applications should leave this flag turned off.

https://www.sqlite.org/pragma.html#pragma_legacy_alter_table

is foreign_keys=OFF not enough: https://www.sqlite.org/lang_altertable.html#otheralter

simolus3 commented 1 month ago

So for context, I maintain a sqlite3 library for Dart that, among other things, offers help implementing the 12-step procedure suggested to emulate arbitrary ALTER TABLE statements by re-creating tables and copying data over. As long as these steps as outlined there are followed in a transaction and legacy_alter_table is only enabled as short as possible, it's not unsafe.

Without legacy_alter_table, some structures like views would have to be destroyed and re-created manually as the schema is temporarily inconsistent during the 12 steps. I can change my code to do that and avoid legacy_alter_table, but since this is a library, breaking migrations even in a very subtle way that some users may depend on is horrible. With support for legacy_alter_table, I could support sqld & Turso much easier. I also understand that supporting legacy pragmas can be undesirable. If this feature is not supposed to be working, I'm happy to close this issue and find a way to work around this.