Re: psql single-step mode woes

Hi Heikki,

Thanks very much for making single-step mode safer. Rejecting unexpected
input instead of executing the query is a useful improvement.

I found a Ctrl-C crash in the new singlestep_verify() function and two
remaining problems with Ctrl-D. Tracing the crash led me to the same use
of sigint_interrupt_jmp in nested calls on master. The Ctrl-D problems
also occur on master. I attached separate Ctrl-C and Ctrl-D fixes for
master (0001 and 0002). 0003 is your patch rebased on those fixes, with
my changes to singlestep_verify() and the tests included.

1. Ctrl-C in a later \prompt can crash psql.

I used the prompt.sql case from t/042_singlestep.pl, containing one
line:

SELECT 111; \prompt 'ANSWER: ' answer

Then, in an interactive psql session:

\set SINGLESTEP on
\i prompt.sql

I pressed Enter at the single-step confirmation to execute SELECT 111,
then Ctrl-C at the ANSWER: input requested by \prompt. With your
original patch, psql aborts with:

double free or corruption (out)

Without your patch, the same sequence cancels the script and leaves the
interactive psql session ready for another command.

The reason is that MainLoop() calls singlestep_verify() through
SendQuery(). MainLoop() and singlestep_verify() both call
sigsetjmp(sigint_interrupt_jmp, 1), using the same global variable.
singlestep_verify() overwrites MainLoop()'s saved context and leaves its
own context in the buffer when it returns.

MainLoop() is still processing the same input line, so it has not
reached the next iteration's sigsetjmp(). The following \prompt calls
gets_fromFile(stdin), which sets sigint_interrupt_enabled to true. On
Ctrl-C, psql_cancel_callback() calls siglongjmp() using the context
saved by singlestep_verify(), but that function has already returned. In
GDB, psql aborts at pg_free(first_prompt) in singlestep_verify().

I initially fixed only singlestep_verify(), using a local sigjmp_buf
variable and a global pointer to select the sigjmp_buf for siglongjmp().
The function saves the previous pointer and restores it before
returning. That fixed this case. When reviewing the other calls to
sigsetjmp() in src/bin/psql/, I noticed that nested MainLoop() calls
also use the same global variable. To check this on unpatched master, I
used an interactive session with SINGLESTEP off. This is the nested-script
case in t/040_interrupt.pl: inner.sql contains SELECT 111;, and outer.sql
has this single line:

\i inner.sql \prompt 'OUTER: ' answer

I ran \i outer.sql, then pressed Ctrl-C at OUTER:.

Both MainLoop() calls use sigsetjmp(sigint_interrupt_jmp, 1). The inner
call overwrites the outer call's saved context and then returns. Ctrl-C
at OUTER: makes siglongjmp() use the inner call's context after that
call has returned, and psql exits unexpectedly.

0001 replaces the global sigjmp_buf with local sigjmp_buf variables and
a global PG_exception_stack pointer, as with nested PG_TRY() blocks.
Each function saves the previous pointer, sets it to its local variable,
and restores it before returning. The SIGINT callback calls
siglongjmp(*PG_exception_stack, 1). A later Ctrl-C therefore uses the
calling function's saved context, not the context of a function that has
returned. This covers MainLoop(), handleCopyIn(), and the callers of
simple_prompt_extended(). 0003 applies the same change to
singlestep_verify().

This nested-script case dates back to script-mode \prompt in 8.3
(7031dd68691); I reproduced it with 8.3.0 and current stable-branch
clients from 13 through 19. These were client tests against a
current-version server.

2. Ctrl-D still executes later backslash commands and, without readline,
exits psql.

With your original patch, Ctrl-D skips the SQL, but I still found two
problems: later backslash commands on the same line execute with or
without readline, and disabling readline also makes the interactive
session exit. I used the same_line.sql case from
t/041_singlestep_eof.pl:

INSERT INTO step_log VALUES (1); \set eof_marker changed \echo :eof_marker
INSERT INTO step_log VALUES (2);

In psql -X, with readline enabled:

CREATE TABLE step_log (id int);
\set ON_ERROR_STOP off
\set SINGLESTEP on
\set eof_marker unchanged
\i same_line.sql

When psql displayed INSERT INTO step_log VALUES (1); and waited for
confirmation, I pressed Ctrl-D instead of Enter. Neither INSERT ran, but
the script printed changed: the \set and \echo after the canceled query
still executed. The session remained usable. Repeating the test with
psql -X -n also printed changed without inserting rows, but then psql
exited instead of accepting another command.

On unpatched master, both effects also occur, but the SQL is not
canceled either: both INSERTs and the later \set and \echo execute
without further confirmation. With readline enabled the session remains
usable; with -n it exits. I reproduced these results with each of the 13
through 19 clients, testing both with and without readline.

0002 therefore fixes this on master. SendQuery() currently ignores a
NULL return from fgets() and proceeds to execute the query. The patch
instead calls clearerr(stdin), sets cancel_pressed, and returns without
sending the query. The clearerr(stdin) call prevents MainLoop() from
immediately reading EOF again and exiting the interactive session.
MainLoop() already checks cancel_pressed before reading the next line;
0002 also checks it before processing each command on the same line, so
the later backslash commands are skipped too.

0003 retains this behavior when moving confirmation into
singlestep_verify(). Your function already sets cancel_pressed on EOF; I
added clearerr(stdin). The MainLoop() check and EOF tests come from 0002
and are reused unchanged.

The three attachments apply in order on master. 0003 includes your patch
with the SendQuery() conflict resolved, the local sigjmp_buf changes,
and the EOF fix in singlestep_verify().

As for backpatching 0001 and 0002, I expect these specific interactive
workflows to affect relatively few users, so I suggest fixing master
only.

The core regression suite and psql TAP tests passed both with the fixes
alone and with your single-step changes applied on top. Since the
changes to input.c and tab-complete.h also need to work when readline
support is not compiled in, I built the complete series with
--without-readline and ran the psql TAP tests; both the build and tests
passed. The Ctrl-C tests cover COPY, \prompt, \password, and \connect
with password authentication. The new tests fail without the
corresponding fixes.

Regards,
Rui