Subject: Re: bug#65998: Sqlite-mode issue deleting records and closing database
From: Eli Zaretskii <[email-masked]>
To: Stefan Kangas <[email-masked]>
Cc: [email-masked], [email-masked], [email-masked]
Date: Sun, 17 Sep 2023 13:05:47 +0300
List-Id: <debbugs-submit.debbugs.gnu.org>

> From: Stefan Kangas <[email-masked]>
> Date: Sat, 16 Sep 2023 07:07:51 -0700
> Cc: [email-masked]
> 
> Eli Zaretskii <[email-masked]> writes:
> 
> >> - The sql query built by sqlite-mode-delete is syntactically correct,
> >>   but misinterpreted by sqlite, and eventually do nothing. The query
> >>   is of the form "REMOVE FROM table_name WHERE rowid = ? and
> >>   'column_name_1' = ? and 'column_name_2' = ? and ..."  From
> >>   https://www.sqlite.org/lang_keywords.html, if I understand
> >>   correctly, the quoted column names in the WHERE clause are
> >>   interpreted as strings from sqlite, and as a result not a single row
> >>   is ever matched and deleted.
> >
> > Thanks.  I installed on the emacs-29 branch the first part of your
> > patch, which closes the DB when the buffer is killed.  As for the
> > second part, I'd prefer that Lars or someone who knows SQL reviewed it
> > first, as I find it strange that Lars would code something so basic
> > which doesn't work at all.
> >
> > Could someone who knows SQL please review and chime in?
> 
> Something like this in an SQL "REMOVE FROM table_name WHERE {foo}" clause
> 
>     'column_name_1' = ?
> 
> will check if the '?' part is equal to the string 'column_name_1', which
> is probably not what we want.
> 
> Whereas this
> 
>     column_name_1 = ?
> 
> will instead check if the '?' part is equal to the value of the column
> column_name_1 in table_name.
> 
> (The "?" is just a placeholder that will be filled in with an actual
> value later.)
> 
> So without having tested the code or studied it in detail, the analysis
> of the problem sounds right to me.

Thanks, so I've now installed the other part on the emacs-29 branch,
and I'm closing the bug.

> Don't we have unit tests in place for this stuff, though?  Perhaps we
> should see this as an opportunity to add some...

I agree, patches are welcome.