From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Jonas Bernoulli Newsgroups: gmane.emacs.bugs Subject: bug#58363: 29.0.50; sqlite-select does not signal errors and errors should be improved Date: Mon, 10 Oct 2022 12:56:38 +0200 Message-ID: <87pmf0c5g9.fsf@bernoul.li> References: <87mta7lb3o.fsf@bernoul.li> <87k05asa8w.fsf@gnus.org> <871qrivsoz.fsf@bernoul.li> <87ilktqdwe.fsf@gnus.org> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="4046"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 58363@debbugs.gnu.org, Eli Zaretskii To: Lars Ingebrigtsen Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Mon Oct 10 12:57:25 2022 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1ohqTM-0000oO-Gs for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 10 Oct 2022 12:57:24 +0200 Original-Received: from localhost ([::1]:45764 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1ohqTL-00046K-CJ for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 10 Oct 2022 06:57:23 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:44174) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1ohqT1-000456-1N for bug-gnu-emacs@gnu.org; Mon, 10 Oct 2022 06:57:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:47148) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1ohqT0-0001Su-Lr for bug-gnu-emacs@gnu.org; Mon, 10 Oct 2022 06:57:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1ohqT0-0006l7-0n for bug-gnu-emacs@gnu.org; Mon, 10 Oct 2022 06:57:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Jonas Bernoulli Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 10 Oct 2022 10:57:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 58363 X-GNU-PR-Package: emacs Original-Received: via spool by 58363-submit@debbugs.gnu.org id=B58363.166539940425956 (code B ref 58363); Mon, 10 Oct 2022 10:57:01 +0000 Original-Received: (at 58363) by debbugs.gnu.org; 10 Oct 2022 10:56:44 +0000 Original-Received: from localhost ([127.0.0.1]:46226 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ohqSh-0006ka-Sd for submit@debbugs.gnu.org; Mon, 10 Oct 2022 06:56:44 -0400 Original-Received: from mail.hostpark.net ([212.243.197.30]:40122) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1ohqSf-0006jR-DD for 58363@debbugs.gnu.org; Mon, 10 Oct 2022 06:56:42 -0400 Original-Received: from localhost (localhost [127.0.0.1]) by mail.hostpark.net (Postfix) with ESMTP id 5A421165A6; Mon, 10 Oct 2022 12:56:39 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=bernoul.li; h= content-type:content-type:mime-version:message-id:date:date :references:in-reply-to:subject:subject:from:from:received :received; s=sel2011a; t=1665399399; bh=oTdN2je9bbpoaMqfM7a8evjx WRutD8mB0kOYf7A9aI4=; b=omKDhIqNBQzKwmZiFK6Uy6d8h3W1VzrJP4/3bOmf yr7q2C0lqcuaThYj2bOwAO+9Vl/RLylhhVrBMi/f8ANAuyTrJG4uqxs+U+OqMV/6 TGKkeG2IEZttoUtA9A8NEiXzbr88DtrZuV84E8qM7rHuDCnZN/R0Qx6RgjncZ4So BY8= X-Virus-Scanned: by Hostpark/NetZone Mailprotection at hostpark.net Original-Received: from mail.hostpark.net ([127.0.0.1]) by localhost (mail0.hostpark.net [127.0.0.1]) (amavisd-new, port 10224) with ESMTP id 4ebOvrr52OOY; Mon, 10 Oct 2022 12:56:39 +0200 (CEST) Original-Received: from customer (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.hostpark.net (Postfix) with ESMTPSA id 2446716591; Mon, 10 Oct 2022 12:56:39 +0200 (CEST) In-Reply-To: <87ilktqdwe.fsf@gnus.org> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:245008 Archived-At: Lars Ingebrigtsen writes: > Jonas Bernoulli 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