Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CASE-SENSITIVITY: Error importing data from SQL file: cannot start a transaction within a transaction #1764

Closed
jsejcksn opened this Issue Feb 24, 2019 · 16 comments

Comments

Projects
None yet
4 participants
@jsejcksn
Copy link

jsejcksn commented Feb 24, 2019

Details for the issue

What did you do?

  1. File > New In-Memory Database
  2. Modal: Edit table definition > Cancel
  3. File > Import... > Database from SQL file...
  4. Modal: Create new database > No

export.sql:

begin transaction;
create table names(
  id integer primary key,
  name text unique
);
commit;

What did you expect to see?

My SQL file imported into the in-memory database

What did you see instead?

Alert:

Error importing data: Error in statement #1: cannot start a transaction within a transaction. Aborting execution and rolling back

Other notes

Using this file instead produces no error:

export-case-sensitive.sql:

BEGIN TRANSACTION;
CREATE TABLE names(
  id integer PRIMARY KEY,
  name text UNIQUE
);
COMMIT;

Useful extra information

macOS: 10.14.3
DB4S: 3.11.1 r2

Did you also

Possibly related to #568, #675

@jsejcksn

This comment has been minimized.

Copy link
Author

jsejcksn commented Feb 24, 2019

Not sure why there is a case-sensitivity rule in place. Is this a trivial fix or big refactor?

@justinclift justinclift added the bug label Feb 24, 2019

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Feb 24, 2019

Not sure why there is a case-sensitivity rule in place.

From rough memory, we use a regex to detect & remove existing transaction statements inside .sql files (so we can do the start/end of transaction in-application). It's probably just a matter of making the regex case insensitive. 😄

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Feb 24, 2019

This might have been easy if this issue had been opened before #1045 😄. Then the replacement was used with a regexp, and it'd have been easy to add the case-insensitivity flag. But after the optimizations done in d783d36 it is no longer possible, because a QByteArray is used instead of a QString, and the former lacks regexp versions for contains and replace. @MKleusberg was aware of the shortcommings, according to his TODO comments 😄 So we have to decide now between performance and reliability. As a minimal solution, we could replace "begin transaction" as well as "BEGIN TRANSACTION".

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Feb 24, 2019

As a minimal solution, we could replace "begin transaction" as well as "BEGIN TRANSACTION".

That's probably good enough for now. It's unlikely people will (in the real world 😉) use stuff like 'BeGiN traNsACtion`. Hopefully. 😁

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Feb 25, 2019

I have seen Begin transaction; or even Begin Transaction; too 😉

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Feb 25, 2019

By the way, some more problems are:

BEGIN     TRANSACTION ; -- Multiple spaces here. Not detected.
END TRANSACTION; -- We're not even looking for this.
BEGIN DEFERRED TRANSACTION; -- We're not expecting any qualifiers between the two words.
INSERT INTO sql_log VALUES('BEGIN TRANSACTION;'); -- Statement gets modified

MKleusberg added a commit that referenced this issue Feb 25, 2019

Fix transaction detection in SQL import
Fix a couple of problems in the detection of transaction statements
during the SQL import. Before this the detection was case-dependent as
well as dependent on the number of spaces you use. Also it did not
detect 'END TRANSACTION', 'COMMIT TRANSACTION', or 'BEGIN <qualifier>
TRANSACTION' at all. Finally, it could modify your statements if you
embed string in them which look like transaction statements. All of this
is, hopefully, fixed in this commit.

See issue #1764.
@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Feb 25, 2019

I think the issue should be fixed now. The other problems I mentioned in my last comment should be fixed too 😉

@jsejcksn Can you check if it is working for you with one of the next nightly builds?

@jsejcksn

This comment has been minimized.

Copy link
Author

jsejcksn commented Feb 25, 2019

rm -r /Applications/DB\ Browser\ for\ SQLite.app
rm ~/Library/Preferences/net.sourceforge.sqlitebrowser.plist
rm -r ~/Library/Saved\ Application\ State/net.sourceforge.sqlitebrowser.savedState
shasum -a 256 DB.Browser.for.SQLite.dmg
16ed0353ed67d233a5614b86ab1474fbd0fd73de00b77eb3b82e5e4b4fe7e460  DB.Browser.for.SQLite.dmg
cat example-upper.sql
BEGIN TRANSACTION;
CREATE TABLE names(
  id integer PRIMARY KEY,
  name text UNIQUE
);
COMMIT;
cat example-lower.sql
begin transaction;
create table names(
  id integer primary key,
  name text unique
);
commit;

@MKleusberg Same results as before when following the same testing procedure with the nightly.

However, I found it interesting that even after deleting my preferences plist and savedState, some (or perhaps all?) of my preferences were preserved upon launching the nightly. I don't recall all of the changes I made from the defaults, but I know that I set a custom font and font size, and those variables were persisted. Is the app storing information in a nonstandard location for macOS? Should I be looking in other locations?

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Feb 26, 2019

Preferences are split over two files (legacy thing). Running this should show them both:

$ ls -1 ~/Library/Preferences/*sqliteb*
/Users/jc/Library/Preferences/com.sqlitebrowser.sqlitebrowser.plist
/Users/jc/Library/Preferences/net.sourceforge.sqlitebrowser.plist

Looking at their names, it's fairly ironic neither of those are .org.

If we change them now, then everyones saved preferences will stop working. Thus leaving them alone. 😉

@jsejcksn

This comment has been minimized.

Copy link
Author

jsejcksn commented Feb 26, 2019

@justinclift You could update it to reflect the current namespace org.sqlitebrowser.sqlitebrowser.plist and direct all writes to that location, but still read from all three locations (in a cascade, I imagine). Then, on a future major semver, you can stop reading from the legacy locations.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Feb 26, 2019

Yeah, that's a decent idea. Probably super low priority though, compared to other stuff. 😄

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Feb 27, 2019

@jsejcksn Sorry about that! There was a minor error in there - I somehow thought 'COMMIT' was five characters long instead of six. So the fix didn't work for that statement. Should be fixed now 😉 Are you ok to check again with the next nightly build?

@jsejcksn

This comment has been minimized.

Copy link
Author

jsejcksn commented Feb 27, 2019

@MKleusberg Sure—just send me a link when it's ready.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Feb 28, 2019

The nightly builds ran successfully (yay) and are online now: 😄

    https://nightlies.sqlitebrowser.org/latest/

@jsejcksn

This comment has been minimized.

Copy link
Author

jsejcksn commented Feb 28, 2019

env:

rm -r /Applications/DB\ Browser\ for\ SQLite.app
rm ~/Library/Preferences/com.sqlitebrowser.sqlitebrowser.plist
rm ~/Library/Preferences/net.sourceforge.sqlitebrowser.plist
# rm: ~/Library/Preferences/net.sourceforge.sqlitebrowser.plist: No such file or directory
rm -r ~/Library/Saved\ Application\ State/net.sourceforge.sqlitebrowser.savedState

shasum -a 256 DB.Browser.for.SQLite.dmg
# ae5fbfcfc7d4a10201ed23d90b62ba236386c46d5c6924215b4d55f4a6f0d541  DB.Browser.for.SQLite.dmg

@MKleusberg Repeated last test. Success.

🐛 -> 🦋

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Feb 28, 2019

Awesome! Thanks for reporting and testing this, @jsejcksn 👍

MKleusberg added a commit that referenced this issue Mar 30, 2019

Fix transaction detection in SQL import
Fix a couple of problems in the detection of transaction statements
during the SQL import. Before this the detection was case-dependent as
well as dependent on the number of spaces you use. Also it did not
detect 'END TRANSACTION', 'COMMIT TRANSACTION', or 'BEGIN <qualifier>
TRANSACTION' at all. Finally, it could modify your statements if you
embed string in them which look like transaction statements. All of this
is, hopefully, fixed in this commit.

See issue #1764.

@MKleusberg MKleusberg referenced this issue Mar 30, 2019

Closed

3.11.2 - outstanding pieces #1773

13 of 13 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.