Subject: Re: [PATCH] ob-sql: session

From: Ihor Radchenko <[email-masked]>
Date: Thu, 19 Jun 2025 13:13:38 +0000

Phil Estival <[email-masked]> 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 <[email-masked]>
> 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/[email-masked]/)

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