Re: [PATCH] psql: tab completion for ALTER ROLE ... IN DATABASE ... - Mailing list pgsql-hackers

From VASUKI M
Subject Re: [PATCH] psql: tab completion for ALTER ROLE ... IN DATABASE ...
Date
Msg-id CAE2r8H6XaQrCd6f71GDz9mvrwEOoSqEbsFt2_cgmhyjGx24RTg@mail.gmail.com
Whole thread Raw
In response to Re: [PATCH] psql: tab completion for ALTER ROLE ... IN DATABASE ...  (VASUKI M <vasukianand0119@gmail.com>)
Responses Re: [PATCH] psql: tab completion for ALTER ROLE ... IN DATABASE ...
List pgsql-hackers

Hi all,

I tried adding TAP coverage for the new ALTER ROLE … IN DATABASE tab-completion paths in src/bin/psql/t/010_tab_completion.pl.

The tests themselves work as expected, but I ran into a limitation of the existing interactive completion harness. The new  RESET completion intentionally ends on incomplete SQL and leaves psql in continuation/readline mode (postgres-#). As a result, the interactive psql process does not terminate cleanly at the end of the test, causing  IPC::Run to time out and the test to abort, even though all completion checks pass.

Earlier completion tests never exercised this state, so the harness implicitly assumes that psql can always be exited cleanly after <TAB> using \q / clear query(); / clear line(); . This change exposes that assumption rather than introducing a regression.

Given this limitation, and to avoid relying on timeouts or fragile cleanup logic, I’m omitting TAP tests for this change for now. If there’s interest in extending or refactoring the completion test harness to better handle continuation-mode cases, I’d be happy to look into that separately.

Attaching some lines from the logfile for the reference,

[11:19:45.497](0.000s) ok 79 - complete a psql variable name
[11:19:45.497](0.000s) ok 80 - complete a psql variable value
[11:19:45.498](0.000s) ok 81 - \r works
[11:19:45.498](0.000s) ok 82 - complete an interpolated psql variable name
[11:19:45.498](0.000s) ok 83 - \r works
[11:19:45.498](0.000s) ok 84 - complete a psql variable test
[11:19:45.498](0.000s) ok 85 - \r works
[11:19:45.498](0.000s) ok 86 - check completion failure path
[11:19:45.499](0.000s) ok 87 - \r works
[11:19:45.499](0.000s) ok 88 - COPY FROM with DEFAULT completion
[11:19:45.499](0.000s) ok 89 - control-U works
IPC::Run: timeout on timer #1 at /usr/share/perl5/IPC/Run.pm line 3007.
# Postmaster PID for node "main" is 16601
### Stopping node "main" using mode immediate
# Running: pg_ctl --pgdata /home/cdac/postgres/src/bin/psql/tmp_check/t_010_tab_completion_main_data/pgdata --mode immediate stop
waiting for server to shut down.... done
server stopped
# No postmaster PID for node "main"
[11:22:46.573](181.074s) # Tests were run but no plan was declared and done_testing() was not seen.
[11:22:46.574](0.000s) # Looks like your test exited with 29 just after 89.

Regards,

Vasuki M
C-DAC,Chennai.


On Mon, Dec 22, 2025 at 9:10 PM VASUKI M <vasukianand0119@gmail.com> wrote:

Hi zeng,

Thanks for pointing this out. You’re absolutely right — PQescapeLiteral() allocates memory using libpq’s allocator, so the returned buffers must be released with PQfreemem() rather than pfree(). Using pfree() here would be incorrect, since it expects memory allocated via PostgreSQL’s memory context APIs (palloc/psprintf).

I’ll update the patch to replace pfree() with PQfreemem() for the buffers returned by PQescapeLiteral(),while continuing to use pfree() for memory allocated via psprintf().

Thanks again for catching this.

Best regards,
Vasuki M
C-DAC,Chennai


On Mon, 22 Dec 2025, 8:18 pm zengman, <zengman@halodbtech.com> wrote:
Hi

I noticed that in the code, the variables `q_role` and `q_dbname` are processed with the `PQescapeLiteral` function,
so `PQfreemem` – instead of `pfree` – should be used here to free the memory.

--
Regards,
Man Zeng
www.openhalo.org
Attachment

pgsql-hackers by date:

Previous
From: Chao Li
Date:
Subject: Re: Fixing some ancient errors in hash join costing
Next
From: Zsolt Parragi
Date:
Subject: Re: RFC: PostgreSQL Storage I/O Transformation Hooks