Package: emacs;
Reported by: Phil Estival <pe <at> 7d.nz>
Date: Mon, 3 Feb 2025 05:20:02 UTC
Severity: wishlist
Tags: patch
View this message in rfc822 format
From: Ihor Radchenko <yantar92 <at> posteo.net> To: Phil Estival <pe <at> 7d.nz> Cc: Michael Mauger <mmauger <at> protonmail.com>, Org Mode List <emacs-orgmode <at> gnu.org>, 76025 <at> debbugs.gnu.org Subject: bug#76025: [PATCH] ob-sql: session Date: Thu, 19 Jun 2025 13:13:38 +0000
Phil Estival <pe <at> 7d.nz> writes:
> I'm submitting again the patches for ob-sql.el, taking into
> considerations the previous review, commented below. I also took good
> note of Michael remarks. The attached series of patches will work
> without any modification to sql.el. I'll introduce later a local
> `sql-connection' bound to the session name. In the absence of it,
> sessions will work, but will ask to confirm provided connection
> parameter upon establishing connection.
Thanks!
> What naming convention to opt for ? org-babel-[lang]-var ?
> or ob-[lang]-var ?
org-babel-[lang]-var
> I reintroduced `org-macs to provide `org-require-package in the following
> patches. But shouldn't `org-assert-version be checked only once when
> org core loads instead of subsequent modules?
The purpose of org-assert-version is recording Org version information
in the compiled .elc file (it is a macro expanded at compile time). That
information is later compared with the dynamically determined Org
version, warning about mismatch. This approach allows us catching a
common problem when parts of Org libraries are loaded from built-in Org
mode (inside Emacs installation) and other parts are loaded from
external Org version (e.g. from ELPA). So, it should be in each Org
library - to make sure that every Org library is from the same Org version.
>> Why `sql--bufer'? (double slash) It is not a variable sql.el uses.
>>
>
> Replaced by `ob-sql-buffer'.
> IIUC, a double dash indicates a variable used by the prefixed package?
The usual convention is
1. <package>-... is a variable that belongs to <package>
2. <package>--... is an internal variable that belongs to <package>
So, your `sql--buffer' can be read as a variable that belongs to sql.el
itself. We may even accidentally shadow sql.el if it ever decided to
introduce such variable.
>>> + (when (not session-p)
>>> + (comint-quit-subjob)
>>> + ;; despite this quit signal, the process may not be finished yet
>>> + (let ((kill-buffer-query-functions nil))
>>> + (kill-this-buffer))))
>>
>> Maybe we should wait until it is finished then? You can check
>> `process-status'.
>
> To be fixed next time
Ok. Leaving it in this message for easier review next time.
> From e27f8231c2f97e6f7f84b6acb793eec7060b8396 Mon Sep 17 00:00:00 2001
> From: Phil Estival <pe <at> 7d.nz>
> Date: Wed, 13 June 2025 17:00:00 +0200
> Subject: [PATCH 01/08] ob-sql: add session support
>
> lisp/ob-sql.el: add session support to ob-sql
>
> * ob-sql.el: declare the maintainer of the following changes
There is no notion of maintainer for specific changes.
Simply say: ob-sql: Add Philippe Estival as a new maintainer
Daniel is not active on the list since 2023, so we should probably
remove him. Will need to ping him later to confirm that he is really
AWOL.
> +(defvar org-sql-batch-terminate
> + (list 'sqlite (format ".print %s\n" org-sql-session--batch-terminate)
> + 'postgres (format "\\echo %s\n" org-sql-session--batch-terminate))
> + "Print the command batch termination as last command.")
"Print the command batch termination as last command." sounds like this
is a function that prints the command ...
Maybe
"Alist of SQL commands used to print end marker after a command batch.
Keys of the alist are symbols like `sqlite', `postgress', etc.
Values of the alist are the SQL commands."
> +(defvar-local org-sql-session-command-terminated nil
> + "Non-nil when a command batch has completed.")
Maybe
"Non-nil when a command batch has completed in current SQL session buffer."
> +(defvar-local org-sql-session-start-time nil
> + "Starting time of a session code block execution, used to exit on timeout.")
Similar here. I believe that specifying that the variable is about
session buffer will make things more clear.
> +(defvar-local org-sql-session-clean-output nil
> + "Store the regexp used to clear output (prompt1|termination|prompt2).")
This also sounds like a function "store" (action).
Maybe
"Regexp to filter out from SQL command output.
This variable is set buffer-local in SQL session buffer to
filter out prompts and termination strings."
> +(defcustom org-babel-default-header-args:sql '((:engine . "unset"))
I think "none" will be a bit more idiomatic for Org babel.
> + "Default header args."
> + :type '(alist :key-type symbol :value-type string
> + :options ("dbi" "sqlite" "mysql" "postgres"
> + "sqsh" "mssql" "vertica" "oracle" "saphana" ))
> + :group 'org-babel-sql
> + :safe t)
I do not think that we should make this particular variable a defcustom.
Simply because none of other babel backends do it.
Also, the list of SQL engines may probably be stored in a constant, so
that we can reuse it.
> +(defcustom org-sql-session-preamble
> + (list
> + 'postgres "
> +\\set ON_ERROR_STOP 1
> +\\pset footer off
> +\\pset pager off
> +\\pset format unaligned"
> + 'sqlite "
> +.header off")
> + "Command preamble to run upon shell start."
> +
> + :type '(plist :key-type symbol :value-type string
> + :options ('postgres 'sqlite 'dbi 'mysql
> + 'sqsh 'mssql 'vertica 'oracle 'saphana ))
> + :group 'org-babel-sql
> + :safe t)
I do not think that :safe t is appropriate for the code that will be
injected into every SQL block.
> +(defcustom org-sql-run-comint-p nil
> + "Run non-session SQL commands through comint if not nil."
The idiomatic way is "If non-nil, ..."
> + :type 'boolean
> + :group 'org-babel-sql
> + :safe t)
> +
> +(defcustom org-sql-timeout 5.0
> + "Abort on timeout."
Action is always confusing at the beginning of variable docstring.
I suggest
"Timeout, in seconds, before ob-sql aborts code evaluation."
> (defun org-babel-sql-dbstring-mysql (host port user password database)
> "Make MySQL cmd line args for database connection. Pass nil to omit that arg."
> - (mapconcat
> - #'identity
> + (combine-and-quote-strings
`combine-and-quote-string' is _not_ the same as `shell-quote-argument'.
In fact, it has nothing to do with shell
quoting. `combine-and-quote-string' is about Elisp string quoting.
I recommend looking into the source code of these two functions. The
difference will be obvious.
> Subject: [PATCH 06/08] ob-sql: add session support
(I used the version of the patch you posted
https://list.orgmode.org/orgmode/87wm9b6ffe.fsf <at> 7d.nz/)
> + (setq-local org-sql-session-clean-output
> + (plist-put org-sql-session-clean-output in-engine
> + (concat "\\(" prompt-regexp "\\)"
> + "\\|\\(" org-sql-session--batch-terminate "\n\\)"
> + (when prompt-cont-regexp
> + (concat "\\|\\(" prompt-cont-regexp "\\)"))))))
Since the variable is buffer-local, do we really need to store
per-engine values? Can be there multiple engines in the same SLQ comint
buffer?
> +(defun org-sql-session-comint-output-filter (_proc string)
> + "Process output STRING of PROC gets redirected to a temporary buffer.
> +It is called several times consecutively as the shell outputs and flush
> +its message buffer"
> ...
> +
> + (when (or (string-match org-sql-session--batch-terminate string)
> + (> (time-to-seconds
> + (time-subtract (current-time)
> + org-sql-session-start-time))
> + org-sql-timeout))
> + (setq-local org-sql-session-command-terminated t))
But that won't actually terminate the command when we are over
`org-sql-timeout', right? So, if something hangs, we will be unable to
send any new src blocks to this SQL buffer (with :session).
> + (format "%s\n;\n%s"
> + (org-babel-expand-body:sql
> + (replace-regexp-in-string "[\t]" "" body) params)
Why do you need an additional cleanup of the BODY? Can't it be done
inside `org-babel-expand-body:sql' itself?
--
Ihor Radchenko // yantar92,
Org mode maintainer,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.