bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved

bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved

[Jonas Bernoulli]

sqlite-select does not signal any errors.  This just returns nil for
example:

  (sqlite-select (sqlite-open nil) "SELECT * FROM no_such_table")

At least some of the other functions do signal errors when appropriate.

  (sqlite-execute db "bla")
  -error-> (error "near \"bla\": syntax error")

It would be nice if a dedicated error type were used and if the error
code was included in the error data.  Maybe it would even make sense
to use dedicated error types for all of the "primary result codes" as
per https://sqlite.org/rescode.html.

     Thanks for considering,
     Jonas

bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved

[Lars Ingebrigtsen]

Jonas Bernoulli <jonas@bernoul.li> writes:

> sqlite-select does not signal any errors.  This just returns nil for
> example:
>
>   (sqlite-select (sqlite-open nil) "SELECT * FROM no_such_table")
>
> At least some of the other functions do signal errors when appropriate.

I've now made this signal an error:

Debugger entered--Lisp error: (error "SQL logic error")

Eli, this also required changes for the Windows macrology at the start
of src/sqlite.c -- can you check whether I broke anything?

bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 58363@debbugs.gnu.org, Eli Zaretskii <eliz@gnu.org>
> Date: Sat, 08 Oct 2022 15:41:51 +0200
> 
> Eli, this also required changes for the Windows macrology at the start
> of src/sqlite.c -- can you check whether I broke anything?

Nothing's broken, your changes are fine.

Thanks.

bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Nothing's broken, your changes are fine.

Thanks for checking; I'm closing this bug report, then.

bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved

[Jonas Bernoulli]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Jonas Bernoulli <jonas@bernoul.li> writes:
>
>> sqlite-select does not signal any errors.  This just returns nil for
>> example:
>>
>>   (sqlite-select (sqlite-open nil) "SELECT * FROM no_such_table")
>>
>> At least some of the other functions do signal errors when appropriate.
>
> I've now made this signal an error:
>
> Debugger entered--Lisp error: (error "SQL logic error")

Thanks, but what about my suggestion to use a dedicated signal?
(Suggesting that different signals be used for different error
codes, may have been a bit excessive though.)

More importantly though, please also include the actual error
message from SQLite in the error data.  Currently only a string
is included, which represents the _kind_ of error that occurred,
something like "SQL logic error".

Unfortunately that only tells the user that they made a mistake
when writing their SQL.  When using pekingduck's module or EmacSQL's
custom binary, the error data includes the error code and the error
message, such as the very useful "no such table: missing".

Please include the message in the error data.

Including the error code would also be useful as that would make
it easier to look it up at https://www.sqlite.org/rescode.html.

I am adding two new SQLite backends to EmacSQL, one using the module
and the other using the new builtin support.

Currently
  (emacsql (emacsql-sqlite nil) "SELECT * FROM missing")
and
  (emacsql (emacsql-sqlite-module nil) "SELECT * FROM missing")
both signal
  (emacsql-error "no such table: nono" 1)
and I considering extending that to
  (emacsql-error "no such table: nono" 1 "SQL logic error")

However, the best I can do for
  (emacsql (emacsql-sqlite-builtin nil) "SELECT * FROM missing")
is
  (emacsql-error nil 1 "SQL logic error")

bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved

[Lars Ingebrigtsen]

Jonas Bernoulli <jonas@bernoul.li> writes:

> Thanks, but what about my suggestion to use a dedicated signal?
> (Suggesting that different signals be used for different error
> codes, may have been a bit excessive though.)

I'm not sure adding a separate signal would be valuable here.

> More importantly though, please also include the actual error
> message from SQLite in the error data.  Currently only a string
> is included, which represents the _kind_ of error that occurred,
> something like "SQL logic error".

Ah, I missed that it was possible to get more data out of sqlite about
what's wrong.

I've now added the extra error string to the value.

bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved

[Jonas Bernoulli]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Jonas Bernoulli <jonas@bernoul.li> writes:
>
>> More importantly though, please also include the actual error
>> message from SQLite in the error data.  Currently only a string
>> is included, which represents the _kind_ of error that occurred,
>> something like "SQL logic error".
>
> Ah, I missed that it was possible to get more data out of sqlite about
> what's wrong.
>
> I've now added the extra error string to the value.

Thanks!

You should probably do the same for sqlite-execute.

The functions that return error codes and messages are documented
at https://www.sqlite.org/c3ref/errcode.html and the error codes
at https://www.sqlite.org/rescode.html.

- sqlite3_errcode()
- sqlite3_extended_errcode()
  return the numeric result code or extended result code for that
  API call.
- sqlite3_errstr()
  returns the English-language text that describes the result code
- sqlite3_errmsg()
- sqlite3_errmsg16()
  return English-language text that describes error
- sqlite3_error_offset()

>> Thanks, but what about my suggestion to use a dedicated signal?
>> (Suggesting that different signals be used for different error
>> codes, may have been a bit excessive though.)
>
> I'm not sure adding a separate signal would be valuable here.

Currently sqlite.c intentionally withhold information from elisp,
needlessly making sophisticated error handling harder and less reliable.
Not using a separate signal is part of that.

I can see no benefit in withholding information.  Maybe you feel that no
user of sqlite.c would ever need/should implement more than rudimentary
error handling.  Currently my end-user packages only use rudimentary
error handling; basically they simply bail on any sql error.  (They use
sqlite.c via emacsql-sqlite-builtin.el.)

However as the new maintainer of EmacSQL, I would like to give users of
the new builtin backend the same feature sets as for the other backends,
not least because maybe some current or future users make use of that.

This is possible to an extend because EmacSQL wraps directly around the
call to sqlite-select and similar functions from different backends, so
it knows that every error it encounters there comes from the respective
backend and can then (in the case of sqlite-select) make an attempt to
decrypt the provided error data.

With the most recent change to that function it can, for example,
resignal (error "SQL logic error (no such table)") as (emacsql-error "no
such table: nono" 1).  To do this it has to extract the two pieces of
information from the one string and because we include the errcode in
the error data instead of the equivalent errstr, we have to maintain an
alist to translate from errstr to errcode.  Including the human readable
errstr is probably better than using the errcode, but changing that
would be a breaking change, so going forward I will provide both, which
actually is better than providing just either one: (emacsql-error "no
such table: nono" 1 "SQL logic error").  I think it would be a good idea
for sqlite.c to do the same.

But I am not just thinking of the needs of EmacSQL here.  In the future
I (and others) likely will use sqlite.c directly.  In order to implement
anything but rudimentary error handling, all those callers would have to
wrap sqlite.c's functions to resignal its errors.  Without doing that it
becomes impossible to reliably tell errors that originate from SQL (or
sqlite.c itself) apart from other errors.

So I would encourage you to always (i.e., not only in sqlite-select)
signal

  (sqlite-error sqlite_errstr() sqlite_errmsg() sqlite_errcode())

for any error originating from sqlite3_prepare_v2() or similar,
and e.g., 

  (sqlite-error "Invalid set object" nil nil)
or
  (sqlite-error "Invalid set object")

for errors originating from check_sqlite() and other places, where the
error doesn't originate from a call to sqlite3_prepare_v2() or similar.

By the way, for one particular error sqlite-execute already uses a
separate signal: Qsqlite_locked_error.  But only that function does it
and only for that one error.  That seems highly inconsistent.  I would
recommend removing this signal and replacing it with Qsqlite_error and
using that for every error and to always include all available data,
filling in nil when a particular piece is not available.

Actually, in the spirit of forward thinking, you might just as well
include the sqlite3_extended_errcode() and sqlite3_error_offset():

  (sqlite-error sqlite_errstr()
                sqlite_errmsg()
                sqlite_errcode()
                sqlite3_extended_errcode()
                sqlite3_error_offset())

     Thanks for considering,
     Jonas

bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved

[Lars Ingebrigtsen]

Jonas Bernoulli <jonas@bernoul.li> writes:

> You should probably do the same for sqlite-execute.

Now done.

> So I would encourage you to always (i.e., not only in sqlite-select)
> signal
>
>   (sqlite-error sqlite_errstr() sqlite_errmsg() sqlite_errcode())

Patches welcome.

> By the way, for one particular error sqlite-execute already uses a
> separate signal: Qsqlite_locked_error.  But only that function does it
> and only for that one error.  That seems highly inconsistent.  I would
> recommend removing this signal and replacing it with Qsqlite_error and
> using that for every error and to always include all available data,
> filling in nil when a particular piece is not available.

It's a retryable error, so it's nice to be able to differentiate easily.

bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved

[Jonas Bernoulli]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Patches welcome.

I'll send some.

bug#58363: [PATCH 0/3] Improve error data signaled by sqlite-execute et al.

[Jonas Bernoulli]

>> So I would encourage you to always (i.e., not only in sqlite-select)
>> signal
>>
>>   (sqlite-error sqlite_errstr() sqlite_errmsg() sqlite_errcode())
>
> Patches welcome.

I hope these changes make sense to someone more experienced.

I have included the value of sqlite3_extended_errcode() in the error
data, but have left out sqlite3_error_offset() because that function
is too new (but we could append it at a later time).

     Cheers,
     Jonas

Jonas Bernoulli (3):
  Use xsignal1 as required by argument type
  Introduce a new sqlite-error
  Improve error data signaled by sqlite-execute et al.

 src/sqlite.c | 57 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 24 deletions(-)

-- 
2.38.0

bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type

[Jonas Bernoulli]

* src/sqlite.c (sqlite-load-extension): Use xsignal1.
---
 src/sqlite.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/sqlite.c b/src/sqlite.c
index 1526e344e5..7861a699f4 100644
--- a/src/sqlite.c
+++ b/src/sqlite.c
@@ -675,7 +675,7 @@ DEFUN ("sqlite-load-extension", Fsqlite_load_extension,
     }
 
   if (!do_allow)
-    xsignal (Qerror, build_string ("Module name not on allowlist"));
+    xsignal1 (Qerror, build_string ("Module name not on allowlist"));
 
   int result = sqlite3_load_extension
 		       (XSQLITE (db)->db,
-- 
2.38.0

bug#58363: [PATCH 2/3] Introduce a new sqlite-error

[Jonas Bernoulli]

* src/sqlite.c (check_sqlite, sqlite-open, bind_values)
(sqlite-execute, sqlite-select, sqlite-load-extension)
(sqlite-next): Use it.
(syms_of_sqlite): Introduce a new error for all sqlite errors
so that we can catch that condition on higher levels.
---
 src/sqlite.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/src/sqlite.c b/src/sqlite.c
index 7861a699f4..78b261fb08 100644
--- a/src/sqlite.c
+++ b/src/sqlite.c
@@ -233,13 +233,13 @@ check_sqlite (Lisp_Object db, bool is_statement)
   init_sqlite_functions ();
   CHECK_SQLITE (db);
   if (is_statement && !XSQLITE (db)->is_statement)
-    xsignal1 (Qerror, build_string ("Invalid set object"));
+    xsignal1 (Qsqlite_error, build_string ("Invalid set object"));
   else if (!is_statement && XSQLITE (db)->is_statement)
-    xsignal1 (Qerror, build_string ("Invalid database object"));
+    xsignal1 (Qsqlite_error, build_string ("Invalid database object"));
   if (!is_statement && !XSQLITE (db)->db)
-    xsignal1 (Qerror, build_string ("Database closed"));
+    xsignal1 (Qsqlite_error, build_string ("Database closed"));
   else if (is_statement && !XSQLITE (db)->db)
-    xsignal1 (Qerror, build_string ("Statement closed"));
+    xsignal1 (Qsqlite_error, build_string ("Statement closed"));
 }
 
 static int db_count = 0;
@@ -259,7 +259,7 @@ DEFUN ("sqlite-open", Fsqlite_open, Ssqlite_open, 0, 1, 0,
 #endif
 
   if (!init_sqlite_functions ())
-    xsignal1 (Qerror, build_string ("sqlite support is not available"));
+    xsignal1 (Qsqlite_error, build_string ("sqlite support is not available"));
 
   if (!NILP (file))
     name = ENCODE_FILE (Fexpand_file_name (file, Qnil));
@@ -272,7 +272,7 @@ DEFUN ("sqlite-open", Fsqlite_open, Ssqlite_open, 0, 1, 0,
       name = CALLN (Fformat, memory_fmt, make_int (++db_count));
       flags |= SQLITE_OPEN_MEMORY;
 #else
-      xsignal1 (Qerror, build_string ("sqlite in-memory is not available"));
+      xsignal1 (Qsqlite_error, build_string ("sqlite in-memory is not available"));
 #endif
     }
 
@@ -342,7 +342,7 @@ bind_values (sqlite3 *db, sqlite3_stmt *stmt, Lisp_Object values)
 	  if (blob)
 	    {
 	      if (SBYTES (value) != SCHARS (value))
-		xsignal1 (Qerror, build_string ("BLOB values must be unibyte"));
+		xsignal1 (Qsqlite_error, build_string ("BLOB values must be unibyte"));
 	    ret = sqlite3_bind_blob (stmt, i + 1,
 				       SSDATA (value), SBYTES (value),
 				       NULL);
@@ -447,7 +447,7 @@ DEFUN ("sqlite-execute", Fsqlite_execute, Ssqlite_execute, 2, 3, 0,
   check_sqlite (db, false);
   CHECK_STRING (query);
   if (!(NILP (values) || CONSP (values) || VECTORP (values)))
-    xsignal1 (Qerror, build_string ("VALUES must be a list or a vector"));
+    xsignal1 (Qsqlite_error, build_string ("VALUES must be a list or a vector"));
 
   sqlite3 *sdb = XSQLITE (db)->db;
   Lisp_Object errmsg = Qnil,
@@ -505,7 +505,7 @@ DEFUN ("sqlite-execute", Fsqlite_execute, Ssqlite_execute, 2, 3, 0,
  exit:
   sqlite3_finalize (stmt);
   xsignal1 (ret == SQLITE_LOCKED || ret == SQLITE_BUSY?
-	    Qsqlite_locked_error: Qerror,
+	    Qsqlite_locked_error: Qsqlite_error,
 	    errmsg);
 }
 
@@ -540,7 +540,7 @@ DEFUN ("sqlite-select", Fsqlite_select, Ssqlite_select, 2, 4, 0,
   CHECK_STRING (query);
 
   if (!(NILP (values) || CONSP (values) || VECTORP (values)))
-    xsignal1 (Qerror, build_string ("VALUES must be a list or a vector"));
+    xsignal1 (Qsqlite_error, build_string ("VALUES must be a list or a vector"));
 
   sqlite3 *sdb = XSQLITE (db)->db;
   Lisp_Object retval = Qnil, errmsg = Qnil,
@@ -589,7 +589,7 @@ DEFUN ("sqlite-select", Fsqlite_select, Ssqlite_select, 2, 4, 0,
 
  exit:
   if (! NILP (errmsg))
-    xsignal1 (Qerror, errmsg);
+    xsignal1 (Qsqlite_error, errmsg);
 
   return retval;
 }
@@ -675,7 +675,7 @@ DEFUN ("sqlite-load-extension", Fsqlite_load_extension,
     }
 
   if (!do_allow)
-    xsignal1 (Qerror, build_string ("Module name not on allowlist"));
+    xsignal1 (Qsqlite_error, build_string ("Module name not on allowlist"));
 
   int result = sqlite3_load_extension
 		       (XSQLITE (db)->db,
@@ -695,7 +695,7 @@ DEFUN ("sqlite-next", Fsqlite_next, Ssqlite_next, 1, 1, 0,
 
   int ret = sqlite3_step (XSQLITE (set)->stmt);
   if (ret != SQLITE_ROW && ret != SQLITE_OK && ret != SQLITE_DONE)
-    xsignal1 (Qerror, build_string (sqlite3_errmsg (XSQLITE (set)->db)));
+    xsignal1 (Qsqlite_error, build_string (sqlite3_errmsg (XSQLITE (set)->db)));
 
   if (ret == SQLITE_DONE)
     {
@@ -794,9 +794,15 @@ syms_of_sqlite (void)
   defsubr (&Ssqlitep);
   defsubr (&Ssqlite_available_p);
 
+  DEFSYM (Qsqlite_error, "sqlite-error");
+  Fput (Qsqlite_error, Qerror_conditions,
+	Fpurecopy (list2 (Qsqlite_error, Qerror)));
+  Fput (Qsqlite_error, Qerror_message,
+	build_pure_c_string ("Database error"));
+
   DEFSYM (Qsqlite_locked_error, "sqlite-locked-error");
   Fput (Qsqlite_locked_error, Qerror_conditions,
-	Fpurecopy (list2 (Qsqlite_locked_error, Qerror)));
+	Fpurecopy (list3 (Qsqlite_locked_error, Qsqlite_error, Qerror)));
   Fput (Qsqlite_locked_error, Qerror_message,
 	build_pure_c_string ("Database locked"));
 
-- 
2.38.0

bug#58363: [PATCH 3/3] Improve error data signaled by sqlite-execute et al.

[Jonas Bernoulli]

* src/sqlite.c (load_dll_functions): Update.
(sqlite_prepare_errdata): New function.
(sqlite_prepare_errmsg): Remove function.
(sqlite-execute, sqlite-select): Use new function.
---
 src/sqlite.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/src/sqlite.c b/src/sqlite.c
index 78b261fb08..d6cb38a29a 100644
--- a/src/sqlite.c
+++ b/src/sqlite.c
@@ -50,6 +50,7 @@ DEF_DLL_FN (SQLITE_API int, sqlite3_bind_int64,
 DEF_DLL_FN (SQLITE_API int, sqlite3_bind_double, (sqlite3_stmt*, int, double));
 DEF_DLL_FN (SQLITE_API int, sqlite3_bind_null, (sqlite3_stmt*, int));
 DEF_DLL_FN (SQLITE_API int, sqlite3_bind_int, (sqlite3_stmt*, int, int));
+DEF_DLL_FN (SQLITE_API int, sqlite3_extended_errcode, (sqlite3*));
 DEF_DLL_FN (SQLITE_API const char*, sqlite3_errmsg, (sqlite3*));
 DEF_DLL_FN (SQLITE_API const char*, sqlite3_errstr, (int));
 DEF_DLL_FN (SQLITE_API int, sqlite3_step, (sqlite3_stmt*));
@@ -88,6 +89,7 @@ DEF_DLL_FN (SQLITE_API int, sqlite3_load_extension,
 # undef sqlite3_bind_double
 # undef sqlite3_bind_null
 # undef sqlite3_bind_int
+# undef sqlite3_extended_errcode
 # undef sqlite3_errmsg
 # undef sqlite3_errstr
 # undef sqlite3_step
@@ -113,6 +115,7 @@ DEF_DLL_FN (SQLITE_API int, sqlite3_load_extension,
 # define sqlite3_bind_double fn_sqlite3_bind_double
 # define sqlite3_bind_null fn_sqlite3_bind_null
 # define sqlite3_bind_int fn_sqlite3_bind_int
+# define sqlite3_extended_errcode fn_sqlite3_extended_errcode
 # define sqlite3_errmsg fn_sqlite3_errmsg
 # define sqlite3_errstr fn_sqlite3_errstr
 # define sqlite3_step fn_sqlite3_step
@@ -141,6 +144,7 @@ load_dll_functions (HMODULE library)
   LOAD_DLL_FN (library, sqlite3_bind_double);
   LOAD_DLL_FN (library, sqlite3_bind_null);
   LOAD_DLL_FN (library, sqlite3_bind_int);
+  LOAD_DLL_FN (library, sqlite3_extended_errcode);
   LOAD_DLL_FN (library, sqlite3_errmsg);
   LOAD_DLL_FN (library, sqlite3_errstr);
   LOAD_DLL_FN (library, sqlite3_step);
@@ -422,16 +426,15 @@ row_to_value (sqlite3_stmt *stmt)
 }
 
 static Lisp_Object
-sqlite_prepare_errmsg (int code, sqlite3 *sdb)
+sqlite_prepare_errdata (int code, sqlite3 *sdb)
 {
-  Lisp_Object errmsg = build_string (sqlite3_errstr (code));
+  Lisp_Object errstr = build_string (sqlite3_errstr (code));
+  Lisp_Object errcode = make_fixnum (code);
   /* More details about what went wrong.  */
-  const char *sql_error = sqlite3_errmsg (sdb);
-  if (sql_error)
-    return CALLN (Fformat, build_string ("%s (%s)"),
-		  errmsg, build_string (sql_error));
-  else
-    return errmsg;
+  Lisp_Object ext_errcode = make_fixnum (sqlite3_extended_errcode (sdb));
+  const char *errmsg = sqlite3_errmsg (sdb);
+  return list4 (errstr, errmsg ? build_string (errmsg) : Qnil,
+		errcode, ext_errcode);
 }
 
 DEFUN ("sqlite-execute", Fsqlite_execute, Ssqlite_execute, 2, 3, 0,
@@ -466,7 +469,7 @@ DEFUN ("sqlite-execute", Fsqlite_execute, Ssqlite_execute, 2, 3, 0,
 	  sqlite3_reset (stmt);
 	}
 
-      errmsg = sqlite_prepare_errmsg (ret, sdb);
+      errmsg = sqlite_prepare_errdata (ret, sdb);
       goto exit;
     }
 
@@ -553,7 +556,7 @@ DEFUN ("sqlite-select", Fsqlite_select, Ssqlite_select, 2, 4, 0,
     {
       if (stmt)
 	sqlite3_finalize (stmt);
-      errmsg = sqlite_prepare_errmsg (ret, sdb);
+      errmsg = sqlite_prepare_errdata (ret, sdb);
       goto exit;
     }
 
-- 
2.38.0

bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Date: Fri, 21 Oct 2022 23:06:34 +0200
> 
> * src/sqlite.c (sqlite-load-extension): Use xsignal1.
> ---
>  src/sqlite.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/sqlite.c b/src/sqlite.c
> index 1526e344e5..7861a699f4 100644
> --- a/src/sqlite.c
> +++ b/src/sqlite.c
> @@ -675,7 +675,7 @@ DEFUN ("sqlite-load-extension", Fsqlite_load_extension,
>      }
>  
>    if (!do_allow)
> -    xsignal (Qerror, build_string ("Module name not on allowlist"));
> +    xsignal1 (Qerror, build_string ("Module name not on allowlist"));

Why Qerror here and not Qsqlite_error?  And if the more general Qerror
is deliberate, then why not Qmodule_load_failed, for instance?

bug#58363: [PATCH 3/3] Improve error data signaled by sqlite-execute et al.

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Date: Fri, 21 Oct 2022 23:06:36 +0200
> 
> +  Lisp_Object ext_errcode = make_fixnum (sqlite3_extended_errcode (sdb));
> +  const char *errmsg = sqlite3_errmsg (sdb);
> +  return list4 (errstr, errmsg ? build_string (errmsg) : Qnil,
> +		errcode, ext_errcode);                      ^^^^

Is that Qnil really a good idea here?  What will an error message look
like in that case?  We may wish replacing Qnil with some standard
text, if it looks better.

bug#58363: [PATCH 2/3] Introduce a new sqlite-error

[Michael Albinus]

Jonas Bernoulli <jonas@bernoul.li> writes:

Hi Jonas,

> +  DEFSYM (Qsqlite_error, "sqlite-error");
> +  Fput (Qsqlite_error, Qerror_conditions,
> +	Fpurecopy (list2 (Qsqlite_error, Qerror)));
> +  Fput (Qsqlite_error, Qerror_message,
> +	build_pure_c_string ("Database error"));
> +
>    DEFSYM (Qsqlite_locked_error, "sqlite-locked-error");
>    Fput (Qsqlite_locked_error, Qerror_conditions,
> -	Fpurecopy (list2 (Qsqlite_locked_error, Qerror)));
> +	Fpurecopy (list3 (Qsqlite_locked_error, Qsqlite_error, Qerror)));
>    Fput (Qsqlite_locked_error, Qerror_message,
>  	build_pure_c_string ("Database locked"));

I'm not sure about our policy, but shouldn't error symbols in the C core
be documented in the manual, node "(elisp) Standard Errors"?

Best regards, Michael.

bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jonas Bernoulli <jonas@bernoul.li>
>> Date: Fri, 21 Oct 2022 23:06:34 +0200
>> 
>> * src/sqlite.c (sqlite-load-extension): Use xsignal1.
>> ---
>>  src/sqlite.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/src/sqlite.c b/src/sqlite.c
>> index 1526e344e5..7861a699f4 100644
>> --- a/src/sqlite.c
>> +++ b/src/sqlite.c
>> @@ -675,7 +675,7 @@ DEFUN ("sqlite-load-extension", Fsqlite_load_extension,
>>      }
>>  
>>    if (!do_allow)
>> -    xsignal (Qerror, build_string ("Module name not on allowlist"));
>> +    xsignal1 (Qerror, build_string ("Module name not on allowlist"));
>
> Why Qerror here and not Qsqlite_error?  And if the more general Qerror
> is deliberate, then why not Qmodule_load_failed, for instance?

This commit just fixes a bug.
Qsqlite_error is introduced in the next commit.

bug#58363: [PATCH 2/3] Introduce a new sqlite-error

[Jonas Bernoulli]

Michael Albinus <michael.albinus@gmx.de> writes:

> Jonas Bernoulli <jonas@bernoul.li> writes:
>
> Hi Jonas,
>
>> +  DEFSYM (Qsqlite_error, "sqlite-error");
>> +  Fput (Qsqlite_error, Qerror_conditions,
>> +	Fpurecopy (list2 (Qsqlite_error, Qerror)));
>> +  Fput (Qsqlite_error, Qerror_message,
>> +	build_pure_c_string ("Database error"));
>> +
>>    DEFSYM (Qsqlite_locked_error, "sqlite-locked-error");
>>    Fput (Qsqlite_locked_error, Qerror_conditions,
>> -	Fpurecopy (list2 (Qsqlite_locked_error, Qerror)));
>> +	Fpurecopy (list3 (Qsqlite_locked_error, Qsqlite_error, Qerror)));
>>    Fput (Qsqlite_locked_error, Qerror_message,
>>  	build_pure_c_string ("Database locked"));
>
> I'm not sure about our policy, but shouldn't error symbols in the C core
> be documented in the manual, node "(elisp) Standard Errors"?
>
> Best regards, Michael.

I don't know but that section begins with

> Here is a list of the more important error symbols in standard Emacs,

Maybe this new error symbol falls into that category.
sqlite-locked-error wasn't documented on that page,
so I didn't do it for this either, for now.

bug#58363: [PATCH 3/3] Improve error data signaled by sqlite-execute et al.

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jonas Bernoulli <jonas@bernoul.li>
>> Date: Fri, 21 Oct 2022 23:06:36 +0200
>> 
>> +  Lisp_Object ext_errcode = make_fixnum (sqlite3_extended_errcode (sdb));
>> +  const char *errmsg = sqlite3_errmsg (sdb);
>> +  return list4 (errstr, errmsg ? build_string (errmsg) : Qnil,
>> +		errcode, ext_errcode);                      ^^^^
>
> Is that Qnil really a good idea here?  What will an error message look
> like in that case?  We may wish replacing Qnil with some standard
> text, if it looks better.

Based on some data I collected from SQLite documentation and code, this
is only relevant in some rare cases:

   ((1  SQLITE_ERROR      "SQL logic error")
    (2  SQLITE_INTERNAL   nil)
    (3  SQLITE_PERM       "access permission denied")
    (4  SQLITE_ABORT      "query aborted")
    (5  SQLITE_BUSY       "database is locked")
    (6  SQLITE_LOCKED     "database table is locked")
    (7  SQLITE_NOMEM      "out of memory")
    (8  SQLITE_READONLY   "attempt to write a readonly database")
    (9  SQLITE_INTERRUPT  "interrupted")
    (10 SQLITE_IOERR      "disk I/O error")
    (11 SQLITE_CORRUPT    "database disk image is malformed")
    (12 SQLITE_NOTFOUND   "unknown operation")
    (13 SQLITE_FULL       "database or disk is full")
    (14 SQLITE_CANTOPEN   "unable to open database file")
    (15 SQLITE_PROTOCOL   "locking protocol")
    (16 SQLITE_EMPTY      nil)
    (17 SQLITE_SCHEMA     "database schema has changed")
    (18 SQLITE_TOOBIG     "string or blob too big")
    (19 SQLITE_CONSTRAINT "constraint failed")
    (20 SQLITE_MISMATCH   "datatype mismatch")
    (21 SQLITE_MISUSE     "bad parameter or other API misuse")
    (22 SQLITE_NOLFS      "large file support is disabled")
    (23 SQLITE_AUTH       "authorization denied")
    (24 SQLITE_FORMAT     nil)
    (25 SQLITE_RANGE      "column index out of range")
    (26 SQLITE_NOTADB     "file is not a database")
    (27 SQLITE_NOTICE     "notification message")
    (28 SQLITE_WARNING    "warning message"))

We would get nil for SQLITE_INTERNAL ("internal malfunction ...
application should never see this"), SQLITE_EMPTY ("not currently
used"), and SQLITE_FORMAT ("not currently used").  The comments in
parentheses are from https://www.sqlite.org/rescode.html.  We will
"never" see these error codes, and if we do see 2/SQLITE_INTERNAL,
it is IMO okay if the shown error is a bit ugly.

bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Cc: 58363@debbugs.gnu.org
> Date: Sat, 22 Oct 2022 12:45:02 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> -    xsignal (Qerror, build_string ("Module name not on allowlist"));
> >> +    xsignal1 (Qerror, build_string ("Module name not on allowlist"));
> >
> > Why Qerror here and not Qsqlite_error?  And if the more general Qerror
> > is deliberate, then why not Qmodule_load_failed, for instance?
> 
> This commit just fixes a bug.
> Qsqlite_error is introduced in the next commit.

This is one reason why I prefer a single patch to series of patches.
(I believe Lars prefers that as well.)  It avoids the need to review
patches that are superseded by the following ones, especially when
network delays cause the different parts of the series to be delivered
out of sequence.

So, unless this totally disrupts your workflows, please post patches
as a single coherent changeset, bypassing intermediate steps that are
later superseded.  TIA.

bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Jonas Bernoulli <jonas@bernoul.li>
>> Cc: 58363@debbugs.gnu.org
>> Date: Sat, 22 Oct 2022 12:45:02 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> -    xsignal (Qerror, build_string ("Module name not on allowlist"));
>> >> +    xsignal1 (Qerror, build_string ("Module name not on allowlist"));
>> >
>> > Why Qerror here and not Qsqlite_error?  And if the more general Qerror
>> > is deliberate, then why not Qmodule_load_failed, for instance?
>> 
>> This commit just fixes a bug.
>> Qsqlite_error is introduced in the next commit.
>
> This is one reason why I prefer a single patch to series of patches.
> (I believe Lars prefers that as well.)  It avoids the need to review
> patches that are superseded by the following ones, especially when
> network delays cause the different parts of the series to be delivered
> out of sequence.
>
> So, unless this totally disrupts your workflows, please post patches
> as a single coherent changeset, bypassing intermediate steps that are
> later superseded.  TIA.

I will do as you wish but I completely disagree that this is the
right thing to do.  But let's agree to disagree, and since you are
the maintainer, you get to say how it ought to be done around here.

(Is there anything you would like me to do, aside from squashing
these two (or all three?) commits?)

bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Cc: 58363@debbugs.gnu.org
> Date: Sat, 22 Oct 2022 17:32:07 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So, unless this totally disrupts your workflows, please post patches
> > as a single coherent changeset, bypassing intermediate steps that are
> > later superseded.  TIA.
> 
> I will do as you wish but I completely disagree that this is the
> right thing to do.  But let's agree to disagree, and since you are
> the maintainer, you get to say how it ought to be done around here.

If the request causes you significant inconvenience, I won't insist.

> (Is there anything you would like me to do, aside from squashing
> these two (or all three?) commits?)

For these patch series, you don't need to do anything: the series is
small enough to not produce any significant problems (and I think you
will be submitting v2 anyway?).  My request was for the future.

TIA