Skip to content

Conversation

nobiot
Copy link
Contributor

@nobiot nobiot commented Dec 27, 2021

This commit is follow-up for PR #2009 should fix issues #1927 & #1910.

Currently, [:pragma (= foreign_keys ON)] is present only in function
org-roam-db--init. This means the foreign_keys is turned on only when the db
file is created for the first time. Subsequent Emacs sessions won't turn it on
as the db file is already present and does not evaluate org-roam-db--init.

This PRAGMA needs to be turned on each database connection at runtime according
to sqlite's documentation (https://sqlite.org/foreignkeys.html#fk_enable)

Foreign key constraints are disabled by default (for backwards compatibility),
so must be enabled separately for each database connection

I have observed that on Windows only the Emacs session that initially creates
the Org-roam db file works correctly with the sqlite3 option. Subsequent Emacs
sessions have the same "Unique constraint failed" error messages.

I have tested this patch on Windows and Ubuntu; both seem to work in the first
and subsequent Emacs sessions.

I am not 100% sure if the location of the PRAGMA is the best; use it as a
proof-of-cocenpt for the fix. Thank you.

Motivation for this change

This commit is follow-up for PR org-roam#2009 should fix issues org-roam#1927 & org-roam#1910.

Currently, `[:pragma (= foreign_keys ON)]` is present only in function
`org-roam-db--init`. This means the `foreign_keys` is turned on only when the db
file is created for the first time. Subsequent Emacs sessions won't turn it on
as the db file is already present and does not evaluate `org-roam-db--init`.

This PRAGMA needs to be turned on each database connection at runtime according
to sqlite's documentation (https://sqlite.org/foreignkeys.html#fk_enable)

```
Foreign key constraints are disabled by default (for backwards compatibility),
so must be enabled separately for each database connection
```

I have observed that on Windows only the Emacs session that initially creates
the Org-roam db file works correctly with the `sqlite3` option. Subsequent Emacs
sessions have the same "Unique constraint failed" error messages.

I have tested this patch on Windows and Ubuntu; both seem to work in the first
and subsequent Emacs sessions.

I am not 100% sure if the location of the PRAGMA is the best; use it as a
proof-of-cocenpt for the fix. Thank you.
@jethrokuan
Copy link
Member

@nobiot thanks for investigating. I found it strange from a design perspective, but I found a forum post that describes a little on how this particular pragma came to be a per connection rather than per db file setting: https://sqlite.org/forum/info/c5dc50f61b88c587

Your fix is correct. We would then want to remove the other pragma call here:

(emacsql db [:pragma (= foreign_keys ON)])

Then this should be good to merge.

@nobiot
Copy link
Contributor Author

nobiot commented Dec 30, 2021

@jethrokuan
Thank you for this. Removed, commit pushed.
Tested OK with Windows and Ubuntu :)

When `org-roam-database-connector` is not `sqlite`, it outputs an unnecessary
error when Org-roam starts up as `defconst` evaluates
`(emacsql-sqlite-ensure-binary)`.

```
Org-roam initialization: (error "No EmacSQL SQLite binary available, aborting")
```

For other database connectors, this is not relevant.
@nobiot
Copy link
Contributor Author

nobiot commented Dec 30, 2021

@jethrokuan

Started using sqlite3 and noticed this error (see my third commit).

org-roam--sqlite-available-p does not seem to be used at all. Do you want to keep it for some reason?
In any case, the error message is not relevant and with "aborting" (it does not abort) it's misleading.

@jethrokuan
Copy link
Member

org-roam--sqlite-available-p does not seem to be used at all. Do you want to keep it for some reason?

No, it can be removed

As per our conversation, org-roam--sqlite-available-p not needed.
@nobiot
Copy link
Contributor Author

nobiot commented Jan 1, 2022

done :)

@jethrokuan jethrokuan merged commit 679ef6e into org-roam:master Jan 2, 2022
@nobiot nobiot deleted the fix/sqlite3 branch January 2, 2022 08:28
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants