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.

* [2025-05-17 18:05 +0000] Ihor Radchenko <yantar92@posteo.net>:
> Phil Estival <pe@7d.nz> writes:
>
>>> May you please update your latest patch for ob-sql.el, converting it
>>> into (1) patch to sql.el; (2) patch for ob-sql.el that assumes changes
>>> to sql.el?
>>
>> Here they are
>> 1) the patch of sql.el
>> 2) the list of changes in ob-sql.el against release_9.7.30.
>>    The patch that assumes the change in sql.el is n°13.
>
> Thanks! I will comment on the Org-related part.
>
>> Subject: [PATCH 01/14] ob-sql: session support. Introduces new variables and
>>  functions
>>
>> * lisp/ob-sql.el: new variables and functions for session support for
>> postgres and sqlite. Requires sql.el for a connection to a session.
>> SQL clients are configured by a preamble of commands given to the SQL
>> shell.  Custom variables are declared in a new sub-group ob-babel-sql.
>> The echo of an SQL ANSI comment is to be appended to the source block
>> of SQL commands for comint to detect when the commands terminate,l
>> instead of relying on prompt detection.
>
> This is not a complete changelog of your changes. Please check out
> https://orgmode.org/worg/org-contribute.html#commit-messages and follow
> the format.
>
> In general, because you plan to be the maintainer, please familiarize
> yourself with all the conventions discussed in
> https://orgmode.org/worg/org-contribute.html and
> https://orgmode.org/worg/org-maintenance.html
>
>>  ;; Author: Eric Schulte
>>  ;; Maintainer: Daniel Kraus <daniel@kraus.my>
>> +;; Maintainer: Philippe Estival <pe@7d.nz>
>>  ;; Keywords: literate programming, reproducible research
>>  ;; URL: https://orgmode.org
>
> I think adding you as a maintainer should be a separate patch to make
> the change distinct.

Patch follows

>
>>  (require 'ob)
>> +(require 'sql)
>
> Do we need to load sql.el early? May we do it dynamically instead?
>

I see that calling `org-require-package' when establishing a new
connection is more rational. Changed in the following patch.

>> +(defvar org-babel-sql-session-start-time)
>
> This variable is unused. What is it for?
>

Starting time of one session block execution, used to exit on timeout.
Fixed in the following patch.

What naming convention to opt for ?  org-babel-[lang]-var ?
or ob-[lang]-var ?

>> +(defvar org-sql-session-preamble
>> +  (list
>> +   'postgres "\\set ON_ERROR_STOP 1
>> +\\pset footer off
>> +\\pset pager off
>> +\\pset format unaligned"	 )
>> +  "Command preamble to run upon shell start.")
>
> Should it be a defcustom?
>

Yes. It is now in the next patch.

>> +(defvar org-sql-session-command-terminated nil)
>
> AFAIU, this is some kind of flag to be used inside sql comit buffer.
> Should it be buffer-local?

Yes, it is now too.

> Also, please add a docstring.
>
>> +(defvar org-sql-session--batch-terminate  "---#"  "To print at the end of a command batch.")
>
> This is a bit weird formatting. In Elisp, the docstring is usually
> placed on a separate line. Also, please check out
> https://www.gnu.org/software/emacs/manual/html_node/elisp/Documentation-Tips.html
> Your docstring here reads confusing.

I hope the formatting and style of the others docstrings is now fine.

>
>> +(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.")
>
> This implies that you treat `org-sql-session--batch-terminate' as a
> constant. If so, maybe declare it as such.
>
> Also, should it be a defcustom?
>
>> +(defvar org-sql-terminal-command-prefix
>> +  (list 'sqlite "\\."
>> +        'postgres "\\\\")
>> +  "Identify a command for the SQL shell.")
>
> This variable is unused.
>

Right, it is no longer in the present mileage.

>> +(defvar org-sql-environment
>> +  (list 'postgres '(("PGPASSWORD" sql-password))))
>
> Also unused.
>

removed.

>> +(defvar org-sql-session-clean-output nil
>> +  "Store the regexp used to clear output (prompt1|termination|prompt2).")
>
> Should it be buffer-local in session buffer?
>

Yes.

>> +(defvar org-sql-session-start-time)
>> +(defvar org-sql-session-command-terminated nil)
>
> Same question.
>
>> +(defvar org-sql-session--batch-terminate  "---#"  "To print at the end of a command batch.")
>
> Duplicate defvar.
>
>> +(defcustom org-sql-run-comint-p 'nil
>
> There is no need to quote nil and numbers.
> They are self-quoting.
>
>> +  "Run non-session SQL commands through comint if not nil."
>> +  :type '(boolean)
>
> :type 'boolean
>
>> +(defcustom org-sql-timeout '5.0
>> +  "Abort on timeout."
>> +  :type '(number)
>
> :type 'number
>
>> +(defcustom org-sql-close-out-temp-buffer-p 'nil
>> +  "To automatically close sql-out-temp buffer."
>
> The docstring is not very clear.
>

Changed to "When non-nil, close the temporary buffer holding output results."

>> +  :type '(boolean)
>
> :type 'boolean
>
>> +(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"
>> +
>> +  ;; Inserting a result in the sql process buffer (to read it as a
>> +  ;; regular prompt log) inserts it to the terminal, and as a result the
>> +  ;; ouput would get passed as input onto the next command line; See
>> +  ;; `comint-redirect-setup' to possibly fix that,
>> +  ;; (with-current-buffer (process-buffer proc) (insert output))
>> +
>> +  (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 org-sql-session-command-terminated t))
>> +
>> +  (with-current-buffer (get-buffer-create "*ob-sql-result*")
>> +    (insert string)))
>
> Using global `org-sql-session-command-terminated',
> `org-sql-session-start-time', and "*ob-sql-result*" buffer will not be
> reliable when there are multiple comint sessions. It will also make it
> impossible to implement async sessions.
> Please use some other approach (for example, buffer-local or
> process-local variables)
>


>> * lisp/ob-sql.el: removal of org-assert-version makes org-macs no
>> longer needed.
>> ...
>> -
>> -(require 'org-macs)
>> -(org-assert-version)
>> -
>
> (org-assert-version) is needed to detect mixed Org versions.
> (require 'org-macs) there is mostly to provide `org-assert-version'.
>

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?

>> Subject: [PATCH 04/14] ob-sql: realign variables for improved readability.
>
> We do not allow whitespace-only commits. See
> https://orgmode.org/worg/org-contribute.html#orgbbd6fd6
>

>> +	  (when colnames-p (with-temp-buffer
>
> nitpick: It is unusual to place
> (when condition body-line1
>                ...)
>
> The common practice is
> (when condition
>   body)
>

Ok, fixed.

>> +        (setq 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 "\\)"))))))
>
> This will be broken when multiple sessions for different engines are
> used in parallel. Please avoid global state.
>

Set as local now.

>> +	     (let ((sql--buffer
>> +		    (org-babel-sql-session-connect in-engine params session)))
>
> 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?

>> +	       (with-current-buffer (get-buffer-create "*ob-sql-result*")
>> +		 (erase-buffer))
>> +	       (setq org-sql-session-start-time (current-time))
>> +	       (setq org-sql-session-command-terminated nil)
>> +
>> +	       (with-current-buffer (get-buffer sql--buffer)
>> +		 (process-send-string (current-buffer)
>> +				      (org-sql-session-format-query
>> +                                       (org-babel-expand-body:sql body params)
>> +                                       in-engine))
>> +		 (while (or (not org-sql-session-command-terminated)
>> +                            (> (time-to-seconds
>> +                                (time-subtract (current-time)
>> +                                               org-sql-session-start-time))
>> +                               org-sql-timeout))
>> +		   (sleep-for 0.03))
>> +		 ;; command finished, remove filter
>> +		 (set-process-filter (get-buffer-process sql--buffer) nil)
>
> This looks repetitive with the code in
> `org-sql-session-comint-output-filter'. Could it be possible to avoid
> code duplication?
>

True, the evaluation of the timeout was dup.

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

>> +                   (insert-for-yank
>
> Why do you need `insert-for-yank'?

No need. It's rather insert instead.

Cheers,

--
Phil