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

SQLite UPDATE...RETURNING sets NOT NULL column as nullable #2939

Open
genderquery opened this issue Dec 16, 2023 · 2 comments
Open

SQLite UPDATE...RETURNING sets NOT NULL column as nullable #2939

genderquery opened this issue Dec 16, 2023 · 2 comments
Labels

Comments

@genderquery
Copy link

Bug Description

If I take the following test from main and change the WHERE clause to match on id instead of name, it fails. The returned id column should not be nullable, regardless.

async fn it_describes_update_with_returning() -> anyhow::Result<()> {
let mut conn = new::<Sqlite>().await?;
let d = conn
.describe("UPDATE accounts SET is_active=true WHERE name=?1 RETURNING id")
.await?;
assert_eq!(d.columns().len(), 1);
assert_eq!(d.column(0).type_info().name(), "INTEGER");
assert_eq!(d.nullable(0), Some(false));
Ok(())
}

Minimal Reproduction

let d = conn
    .describe("UPDATE accounts SET is_active=true WHERE id=?1 RETURNING id")
    .await?;

assert_eq!(d.columns().len(), 1);
assert_eq!(d.column(0).type_info().name(), "INTEGER");
assert_eq!(d.nullable(0), Some(false));
assertion `left == right` failed
  left: Some(true)
 right: Some(false)

From setup.sql:

CREATE TABLE accounts (
    id INTEGER NOT NULL PRIMARY KEY,
    name TEXT NOT NULL,
    is_active BOOLEAN
);
sqlite> EXPLAIN UPDATE accounts SET is_active=true WHERE name=?1 RETURNING id;
addr  opcode         p1    p2    p3    p4             p5  comment
----  -------------  ----  ----  ----  -------------  --  -------------
0     Init           0     35    0                    0   Start at 35
1     Null           0     1     2                    0   r[1..2]=NULL
2     OpenEphemeral  1     0     1                    0   nColumn=0
3     OpenRead       0     4     0     2              0   root=4 iDb=0; accounts
4     Rewind         0     10    0                    0
5       Column         0     1     10                   0   r[10]=accounts.name
6       Ne             11    9     10    BINARY-8       82  if r[10]!=r[11] goto 9
7       Rowid          0     2     0                    0   r[2]=rowid
8       Insert         1     1     2                    0   intkey=r[2] data=r[1]
9     Next           0     5     0                    1
10    OpenWrite      0     4     0     3              0   root=4 iDb=0; accounts
11    Rewind         1     30    0                    0
12      Rowid          1     2     0                    0   r[2]=rowid
13      NotExists      0     29    2                    0   intkey=r[2]
14      Rowid          0     3     0                    0   r[3]=rowid
15      Column         0     1     4                    0   r[4]=accounts.name
16      Column         0     2     5                    0   r[5]=accounts.is_active
17      Copy           2     6     0                    0   r[6]=r[2]
18      Null           0     7     0                    0   r[7]=NULL
19      Column         0     1     8                    0   r[8]=accounts.name
20      Integer        1     9     0                    0   r[9]=1
21      HaltIfNull     1299  2     9     accounts.is_active  1   if r[9]=null halt
22      MakeRecord     7     3     12    DBC            0   r[12]=mkrec(r[7..9])
23      Delete         0     68    6     accounts       0
24      Insert         0     12    6     accounts       5   intkey=r[6] data=r[12]
25      SCopy          6     13    0                    0   r[13]=r[6]
26      MakeRecord     13    1     14                   0   r[14]=mkrec(r[13])
27      NewRowid       3     15    0                    0   r[15]=rowid
28      Insert         3     14    15                   0   intkey=r[15] data=r[14]
29    Next           1     12    0                    0
30    Rewind         3     34    0                    0
31      Column         3     0     13                   0   r[13]=
32      ResultRow      13    1     0                    0   output=r[13]
33    Next           3     31    0                    0
34    Halt           0     0     0                    0
35    Transaction    0     1     3     0              1   usesStmtJournal=1
36    Variable       1     11    0     ?1             0   r[11]=parameter(1,?1)
37    OpenEphemeral  3     1     0                    0   nColumn=1
38    Goto           0     1     0                    0
sqlite> EXPLAIN UPDATE accounts SET is_active=true WHERE id=?1 RETURNING id;
addr  opcode         p1    p2    p3    p4             p5  comment
----  -------------  ----  ----  ----  -------------  --  -------------
0     Init           0     28    0                    0   Start at 28
1     Null           0     1     2                    0   r[1..2]=NULL
2     Noop           1     0     1                    0
3     OpenWrite      0     4     0     3              0   root=4 iDb=0; accounts
4     Variable       1     10    0     ?1             0   r[10]=parameter(1,?1)
5     SeekRowid      0     7     10                   0   intkey=r[10]
6     Rowid          0     2     0                    0   r[2]=rowid
7     IsNull         2     23    0                    0   if r[2]==NULL goto 23
8     Rowid          0     3     0                    0   r[3]=rowid
9     Column         0     1     4                    0   r[4]=accounts.name
10    Column         0     2     5                    0   r[5]=accounts.is_active
11    Copy           2     6     0                    0   r[6]=r[2]
12    Null           0     7     0                    0   r[7]=NULL
13    Column         0     1     8                    0   r[8]=accounts.name
14    Integer        1     9     0                    0   r[9]=1
15    HaltIfNull     1299  2     9     accounts.is_active  1   if r[9]=null halt
16    MakeRecord     7     3     1     DBC            0   r[1]=mkrec(r[7..9])
17    Delete         0     68    6     accounts       0
18    Insert         0     1     6     accounts       5   intkey=r[6] data=r[1]
19    SCopy          6     11    0                    0   r[11]=r[6]
20    MakeRecord     11    1     12                   0   r[12]=mkrec(r[11])
21    NewRowid       3     13    0                    0   r[13]=rowid
22    Insert         3     12    13                   0   intkey=r[13] data=r[12]
23    Rewind         3     27    0                    0
24      Column         3     0     11                   0   r[11]=
25      ResultRow      11    1     0                    0   output=r[11]
26    Next           3     24    0                    0
27    Halt           0     0     0                    0
28    Transaction    0     1     3     0              1   usesStmtJournal=1
29    OpenEphemeral  3     1     0                    0   nColumn=1
30    Goto           0     1     0                    0

Info

  • SQLx version: 0.7.3
  • SQLx features enabled: "sqlite", "migrate", "runtime-tokio", "tls-rustls"
  • Database server and version: SQLite 3.37.2
  • Operating system: Ubuntu 22.04.3 LTS
  • rustc --version: rustc 1.74.0 (79e9716c9 2023-11-13)
@genderquery
Copy link
Author

Likely related to issues #2542 and #2407.

tyrelr added a commit to tyrelr/sqlx that referenced this issue Feb 18, 2024
tyrelr added a commit to tyrelr/sqlx that referenced this issue Feb 18, 2024
@arlyon
Copy link
Contributor

arlyon commented May 24, 2024

I am the author and fixer of #2407 which is related but slightly different. That fix handles proving that is_active is not null which is enough for inserts but in the case of updates, as you observed, misses the id field.

> 15    HaltIfNull     1299  2     9     accounts.is_active  1   if r[9]=null halt

There is not a heuristic in the code to prove that id is not null but I think it is possible from these ops.

5     SeekRowid      0     7     10                   0   intkey=r[10]
6     Rowid          0     2     0                    0   r[2]=rowid
7     IsNull         2     23    0                    0   if r[2]==NULL goto 23

abonander pushed a commit that referenced this issue May 31, 2024
* convert logger to output a query graph

* avoid duplicating branch paths to shrink output graph

* separate different branching paths

* include all branches which found unique states

* track the reason for ending each branches execution

* track the result type of each branch

* make edges rely on history index instead of program_id, to avoid errors when looping

* add state diff to query graph

* drop redundant table info

* rework graph to show state changes, rework logger to store state snapshots

* show state on the previous operation

* gather duplicate state changes into clusters to reduce repetition

* draw invisible connections between unknown instructions by program_i

* clean up dot format string escaping

* add test case from #1960 (update returning all columns)

* add tests for #2939 (update returning only the PK column)

* allow inserting into a table using only the index

* improve null handling of IfNull, fix output type of NewRowId

* add NoResult nodes for branches which don't log a result, as a sanity check

* add short-circuit to all logging operations

* remove duplicate logging checks, and make logging enabled/disabled consistently depend on sqlx::explain instead of sqlx for capture & sqlx::explain for output

* add failing test for awkwardly nested/filtered count subquery

* handle special case of return operation to fix failing test

* require trace log level instead of using whatever log level  statement logging was configured to use
jayy-lmao pushed a commit to jayy-lmao/sqlx that referenced this issue Jun 6, 2024
* convert logger to output a query graph

* avoid duplicating branch paths to shrink output graph

* separate different branching paths

* include all branches which found unique states

* track the reason for ending each branches execution

* track the result type of each branch

* make edges rely on history index instead of program_id, to avoid errors when looping

* add state diff to query graph

* drop redundant table info

* rework graph to show state changes, rework logger to store state snapshots

* show state on the previous operation

* gather duplicate state changes into clusters to reduce repetition

* draw invisible connections between unknown instructions by program_i

* clean up dot format string escaping

* add test case from launchbadge#1960 (update returning all columns)

* add tests for launchbadge#2939 (update returning only the PK column)

* allow inserting into a table using only the index

* improve null handling of IfNull, fix output type of NewRowId

* add NoResult nodes for branches which don't log a result, as a sanity check

* add short-circuit to all logging operations

* remove duplicate logging checks, and make logging enabled/disabled consistently depend on sqlx::explain instead of sqlx for capture & sqlx::explain for output

* add failing test for awkwardly nested/filtered count subquery

* handle special case of return operation to fix failing test

* require trace log level instead of using whatever log level  statement logging was configured to use
jrasanen pushed a commit to jrasanen/sqlx that referenced this issue Oct 14, 2024
* convert logger to output a query graph

* avoid duplicating branch paths to shrink output graph

* separate different branching paths

* include all branches which found unique states

* track the reason for ending each branches execution

* track the result type of each branch

* make edges rely on history index instead of program_id, to avoid errors when looping

* add state diff to query graph

* drop redundant table info

* rework graph to show state changes, rework logger to store state snapshots

* show state on the previous operation

* gather duplicate state changes into clusters to reduce repetition

* draw invisible connections between unknown instructions by program_i

* clean up dot format string escaping

* add test case from launchbadge#1960 (update returning all columns)

* add tests for launchbadge#2939 (update returning only the PK column)

* allow inserting into a table using only the index

* improve null handling of IfNull, fix output type of NewRowId

* add NoResult nodes for branches which don't log a result, as a sanity check

* add short-circuit to all logging operations

* remove duplicate logging checks, and make logging enabled/disabled consistently depend on sqlx::explain instead of sqlx for capture & sqlx::explain for output

* add failing test for awkwardly nested/filtered count subquery

* handle special case of return operation to fix failing test

* require trace log level instead of using whatever log level  statement logging was configured to use
jrasanen pushed a commit to jrasanen/sqlx that referenced this issue Oct 14, 2024
* convert logger to output a query graph

* avoid duplicating branch paths to shrink output graph

* separate different branching paths

* include all branches which found unique states

* track the reason for ending each branches execution

* track the result type of each branch

* make edges rely on history index instead of program_id, to avoid errors when looping

* add state diff to query graph

* drop redundant table info

* rework graph to show state changes, rework logger to store state snapshots

* show state on the previous operation

* gather duplicate state changes into clusters to reduce repetition

* draw invisible connections between unknown instructions by program_i

* clean up dot format string escaping

* add test case from launchbadge#1960 (update returning all columns)

* add tests for launchbadge#2939 (update returning only the PK column)

* allow inserting into a table using only the index

* improve null handling of IfNull, fix output type of NewRowId

* add NoResult nodes for branches which don't log a result, as a sanity check

* add short-circuit to all logging operations

* remove duplicate logging checks, and make logging enabled/disabled consistently depend on sqlx::explain instead of sqlx for capture & sqlx::explain for output

* add failing test for awkwardly nested/filtered count subquery

* handle special case of return operation to fix failing test

* require trace log level instead of using whatever log level  statement logging was configured to use
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants