Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Update development docs referencing versions of sqlite we no longer support #15498

Merged
merged 4 commits into from
Apr 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/15498.doc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Update outdated development docs that mention restrictions in versions of SQLite that we no longer support.
34 changes: 1 addition & 33 deletions docs/development/database_schema.md
Original file line number Diff line number Diff line change
Expand Up @@ -155,43 +155,11 @@ def run_upgrade(
Boolean columns require special treatment, since SQLite treats booleans the
same as integers.

There are three separate aspects to this:

* Any new boolean column must be added to the `BOOLEAN_COLUMNS` list in
Any new boolean column must be added to the `BOOLEAN_COLUMNS` list in
`synapse/_scripts/synapse_port_db.py`. This tells the port script to cast
the integer value from SQLite to a boolean before writing the value to the
postgres database.

* Before SQLite 3.23, `TRUE` and `FALSE` were not recognised as constants by
SQLite, and the `IS [NOT] TRUE`/`IS [NOT] FALSE` operators were not
supported. This makes it necessary to avoid using `TRUE` and `FALSE`
constants in SQL commands.

For example, to insert a `TRUE` value into the database, write:

```python
txn.execute("INSERT INTO tbl(col) VALUES (?)", (True, ))
```
Comment on lines -165 to -174
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we file an issue to track doing this for any current queries? I don't know how easy that would be to find, but I definitely know I've written a few where I purposefully pass in True or False as parameters...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean open an issue to covert queries like this:

txn.execute(
"UPDATE events SET outlier = ?"
" WHERE event_id IN ("
" SELECT event_id FROM events_to_purge "
" WHERE NOT should_delete"
")",
(True,),
)

currently in the code base to use the IS [NOT] TRUE/IS [NOT] FALSE operators (presumably because it's easier to understand that way)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what I mean.

I feel less strongly about converting = to IS than just inlining the variables: I think reducing the number of placeholders makes the queries clearer.

Copy link
Contributor Author

@H-Shay H-Shay May 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right on I'll open an issue for that. Edit: this is #15515


* Default values for new boolean columns present a particular
difficulty. Generally it is best to create separate schema files for
Postgres and SQLite. For example:

```sql
# in 00delta.sql.postgres:
ALTER TABLE tbl ADD COLUMN col BOOLEAN DEFAULT FALSE;
```

```sql
# in 00delta.sql.sqlite:
ALTER TABLE tbl ADD COLUMN col BOOLEAN DEFAULT 0;
```

Note that there is a particularly insidious failure mode here: the Postgres
flavour will be accepted by SQLite 3.22, but will give a column whose
default value is the **string** `"FALSE"` - which, when cast back to a boolean
in Python, evaluates to `True`.


## `event_id` global uniqueness

Expand Down