all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved
@ 2022-10-07 18:52 Jonas Bernoulli
  2022-10-08 13:41 ` Lars Ingebrigtsen
  0 siblings, 1 reply; 22+ messages in thread
From: Jonas Bernoulli @ 2022-10-07 18:52 UTC (permalink / raw)
  To: 58363

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





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved
  2022-10-07 18:52 bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved Jonas Bernoulli
@ 2022-10-08 13:41 ` Lars Ingebrigtsen
  2022-10-08 14:35   ` Eli Zaretskii
  2022-10-08 22:47   ` Jonas Bernoulli
  0 siblings, 2 replies; 22+ messages in thread
From: Lars Ingebrigtsen @ 2022-10-08 13:41 UTC (permalink / raw)
  To: Jonas Bernoulli; +Cc: 58363, Eli Zaretskii

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?





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved
  2022-10-08 13:41 ` Lars Ingebrigtsen
@ 2022-10-08 14:35   ` Eli Zaretskii
  2022-10-08 14:51     ` Lars Ingebrigtsen
  2022-10-08 22:47   ` Jonas Bernoulli
  1 sibling, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2022-10-08 14:35 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 58363, jonas

> 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.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved
  2022-10-08 14:35   ` Eli Zaretskii
@ 2022-10-08 14:51     ` Lars Ingebrigtsen
  0 siblings, 0 replies; 22+ messages in thread
From: Lars Ingebrigtsen @ 2022-10-08 14:51 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 58363, jonas

Eli Zaretskii <eliz@gnu.org> writes:

> Nothing's broken, your changes are fine.

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





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved
  2022-10-08 13:41 ` Lars Ingebrigtsen
  2022-10-08 14:35   ` Eli Zaretskii
@ 2022-10-08 22:47   ` Jonas Bernoulli
  2022-10-09 14:18     ` Lars Ingebrigtsen
  1 sibling, 1 reply; 22+ messages in thread
From: Jonas Bernoulli @ 2022-10-08 22:47 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 58363, Eli Zaretskii

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")





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved
  2022-10-08 22:47   ` Jonas Bernoulli
@ 2022-10-09 14:18     ` Lars Ingebrigtsen
  2022-10-10 10:56       ` Jonas Bernoulli
  0 siblings, 1 reply; 22+ messages in thread
From: Lars Ingebrigtsen @ 2022-10-09 14:18 UTC (permalink / raw)
  To: Jonas Bernoulli; +Cc: 58363, Eli Zaretskii

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.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved
  2022-10-09 14:18     ` Lars Ingebrigtsen
@ 2022-10-10 10:56       ` Jonas Bernoulli
  2022-10-11  0:23         ` Lars Ingebrigtsen
  0 siblings, 1 reply; 22+ messages in thread
From: Jonas Bernoulli @ 2022-10-10 10:56 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 58363, Eli Zaretskii

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





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved
  2022-10-10 10:56       ` Jonas Bernoulli
@ 2022-10-11  0:23         ` Lars Ingebrigtsen
  2022-10-14 17:52           ` Jonas Bernoulli
  2022-10-21 21:06           ` bug#58363: [PATCH 0/3] Improve error data signaled by sqlite-execute et al Jonas Bernoulli
  0 siblings, 2 replies; 22+ messages in thread
From: Lars Ingebrigtsen @ 2022-10-11  0:23 UTC (permalink / raw)
  To: Jonas Bernoulli; +Cc: 58363, Eli Zaretskii

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.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved
  2022-10-11  0:23         ` Lars Ingebrigtsen
@ 2022-10-14 17:52           ` Jonas Bernoulli
  2022-10-21 21:06           ` bug#58363: [PATCH 0/3] Improve error data signaled by sqlite-execute et al Jonas Bernoulli
  1 sibling, 0 replies; 22+ messages in thread
From: Jonas Bernoulli @ 2022-10-14 17:52 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 58363, Eli Zaretskii

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Patches welcome.

I'll send some.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: [PATCH 0/3] Improve error data signaled by sqlite-execute et al.
  2022-10-11  0:23         ` Lars Ingebrigtsen
  2022-10-14 17:52           ` Jonas Bernoulli
@ 2022-10-21 21:06           ` Jonas Bernoulli
  2022-10-21 21:06             ` bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type Jonas Bernoulli
                               ` (2 more replies)
  1 sibling, 3 replies; 22+ messages in thread
From: Jonas Bernoulli @ 2022-10-21 21:06 UTC (permalink / raw)
  To: 58363

>> 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






^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type
  2022-10-21 21:06           ` bug#58363: [PATCH 0/3] Improve error data signaled by sqlite-execute et al Jonas Bernoulli
@ 2022-10-21 21:06             ` Jonas Bernoulli
  2022-10-22  6:45               ` Eli Zaretskii
  2022-10-21 21:06             ` bug#58363: [PATCH 2/3] Introduce a new sqlite-error Jonas Bernoulli
  2022-10-21 21:06             ` bug#58363: [PATCH 3/3] Improve error data signaled by sqlite-execute et al Jonas Bernoulli
  2 siblings, 1 reply; 22+ messages in thread
From: Jonas Bernoulli @ 2022-10-21 21:06 UTC (permalink / raw)
  To: 58363

* 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






^ permalink raw reply related	[flat|nested] 22+ messages in thread

* bug#58363: [PATCH 2/3] Introduce a new sqlite-error
  2022-10-21 21:06           ` bug#58363: [PATCH 0/3] Improve error data signaled by sqlite-execute et al Jonas Bernoulli
  2022-10-21 21:06             ` bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type Jonas Bernoulli
@ 2022-10-21 21:06             ` Jonas Bernoulli
  2022-10-22  9:14               ` Michael Albinus
  2022-10-21 21:06             ` bug#58363: [PATCH 3/3] Improve error data signaled by sqlite-execute et al Jonas Bernoulli
  2 siblings, 1 reply; 22+ messages in thread
From: Jonas Bernoulli @ 2022-10-21 21:06 UTC (permalink / raw)
  To: 58363

* 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






^ permalink raw reply related	[flat|nested] 22+ messages in thread

* bug#58363: [PATCH 3/3] Improve error data signaled by sqlite-execute et al.
  2022-10-21 21:06           ` bug#58363: [PATCH 0/3] Improve error data signaled by sqlite-execute et al Jonas Bernoulli
  2022-10-21 21:06             ` bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type Jonas Bernoulli
  2022-10-21 21:06             ` bug#58363: [PATCH 2/3] Introduce a new sqlite-error Jonas Bernoulli
@ 2022-10-21 21:06             ` Jonas Bernoulli
  2022-10-22  6:49               ` Eli Zaretskii
  2 siblings, 1 reply; 22+ messages in thread
From: Jonas Bernoulli @ 2022-10-21 21:06 UTC (permalink / raw)
  To: 58363

* 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






^ permalink raw reply related	[flat|nested] 22+ messages in thread

* bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type
  2022-10-21 21:06             ` bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type Jonas Bernoulli
@ 2022-10-22  6:45               ` Eli Zaretskii
  2022-10-22 10:45                 ` Jonas Bernoulli
  0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2022-10-22  6:45 UTC (permalink / raw)
  To: Jonas Bernoulli; +Cc: 58363

> 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?





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: [PATCH 3/3] Improve error data signaled by sqlite-execute et al.
  2022-10-21 21:06             ` bug#58363: [PATCH 3/3] Improve error data signaled by sqlite-execute et al Jonas Bernoulli
@ 2022-10-22  6:49               ` Eli Zaretskii
  2022-10-22 11:07                 ` Jonas Bernoulli
  0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2022-10-22  6:49 UTC (permalink / raw)
  To: Jonas Bernoulli; +Cc: 58363

> 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.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: [PATCH 2/3] Introduce a new sqlite-error
  2022-10-21 21:06             ` bug#58363: [PATCH 2/3] Introduce a new sqlite-error Jonas Bernoulli
@ 2022-10-22  9:14               ` Michael Albinus
  2022-10-22 10:47                 ` Jonas Bernoulli
  0 siblings, 1 reply; 22+ messages in thread
From: Michael Albinus @ 2022-10-22  9:14 UTC (permalink / raw)
  To: Jonas Bernoulli; +Cc: 58363

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.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type
  2022-10-22  6:45               ` Eli Zaretskii
@ 2022-10-22 10:45                 ` Jonas Bernoulli
  2022-10-22 11:45                   ` Eli Zaretskii
  0 siblings, 1 reply; 22+ messages in thread
From: Jonas Bernoulli @ 2022-10-22 10:45 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 58363

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.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: [PATCH 2/3] Introduce a new sqlite-error
  2022-10-22  9:14               ` Michael Albinus
@ 2022-10-22 10:47                 ` Jonas Bernoulli
  0 siblings, 0 replies; 22+ messages in thread
From: Jonas Bernoulli @ 2022-10-22 10:47 UTC (permalink / raw)
  To: Michael Albinus; +Cc: 58363

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.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: [PATCH 3/3] Improve error data signaled by sqlite-execute et al.
  2022-10-22  6:49               ` Eli Zaretskii
@ 2022-10-22 11:07                 ` Jonas Bernoulli
  0 siblings, 0 replies; 22+ messages in thread
From: Jonas Bernoulli @ 2022-10-22 11:07 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 58363

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.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type
  2022-10-22 10:45                 ` Jonas Bernoulli
@ 2022-10-22 11:45                   ` Eli Zaretskii
  2022-10-22 15:32                     ` Jonas Bernoulli
  0 siblings, 1 reply; 22+ messages in thread
From: Eli Zaretskii @ 2022-10-22 11:45 UTC (permalink / raw)
  To: Jonas Bernoulli; +Cc: 58363

> 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.





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type
  2022-10-22 11:45                   ` Eli Zaretskii
@ 2022-10-22 15:32                     ` Jonas Bernoulli
  2022-10-22 15:59                       ` Eli Zaretskii
  0 siblings, 1 reply; 22+ messages in thread
From: Jonas Bernoulli @ 2022-10-22 15:32 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 58363

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?)





^ permalink raw reply	[flat|nested] 22+ messages in thread

* bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type
  2022-10-22 15:32                     ` Jonas Bernoulli
@ 2022-10-22 15:59                       ` Eli Zaretskii
  0 siblings, 0 replies; 22+ messages in thread
From: Eli Zaretskii @ 2022-10-22 15:59 UTC (permalink / raw)
  To: Jonas Bernoulli; +Cc: 58363

> 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





^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2022-10-22 15:59 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-10-07 18:52 bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved Jonas Bernoulli
2022-10-08 13:41 ` Lars Ingebrigtsen
2022-10-08 14:35   ` Eli Zaretskii
2022-10-08 14:51     ` Lars Ingebrigtsen
2022-10-08 22:47   ` Jonas Bernoulli
2022-10-09 14:18     ` Lars Ingebrigtsen
2022-10-10 10:56       ` Jonas Bernoulli
2022-10-11  0:23         ` Lars Ingebrigtsen
2022-10-14 17:52           ` Jonas Bernoulli
2022-10-21 21:06           ` bug#58363: [PATCH 0/3] Improve error data signaled by sqlite-execute et al Jonas Bernoulli
2022-10-21 21:06             ` bug#58363: [PATCH 1/3] Use xsignal1 as required by argument type Jonas Bernoulli
2022-10-22  6:45               ` Eli Zaretskii
2022-10-22 10:45                 ` Jonas Bernoulli
2022-10-22 11:45                   ` Eli Zaretskii
2022-10-22 15:32                     ` Jonas Bernoulli
2022-10-22 15:59                       ` Eli Zaretskii
2022-10-21 21:06             ` bug#58363: [PATCH 2/3] Introduce a new sqlite-error Jonas Bernoulli
2022-10-22  9:14               ` Michael Albinus
2022-10-22 10:47                 ` Jonas Bernoulli
2022-10-21 21:06             ` bug#58363: [PATCH 3/3] Improve error data signaled by sqlite-execute et al Jonas Bernoulli
2022-10-22  6:49               ` Eli Zaretskii
2022-10-22 11:07                 ` Jonas Bernoulli

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.