Horton

Re: Improving psql's \password command

Nathan Bossart <bossartn@amazon.com>

View thread View on postgr.es
From: "Bossart, Nathan" <bossartn@amazon.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Date: 2021-11-15T20:30:49Z
Lists: pgsql-hackers

Attachments

Thanks for the expeditious review.

On 11/15/21, 10:13 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> Hm.  It's not as bad as I thought it might be, but I still dislike
> importing <setjmp.h> into common/string.h --- that seems like a mighty
> ugly dependency to have there.  I guess one idea to avoid that is to
> declare SigintInterruptContext.jmpbuf as "void *".  Alternatively we
> could push those function declarations into some specialized header.

I used "void *" for v2.

> * API spec for SigintInterruptContext needs to be a bit more detailed.
> Maybe "... via an existing SIGINT signal handler that will longjmp to
> the specified place, but only when *enabled is true".

Done.

> * I don't believe that this bit is necessary, or if it is,
> the comment fails to justify it:
>
> -       initStringInfo(&buf);
> +       /* make sure buf is palloc'd so we don't lose changes after a longjmp */
> +       buf = makeStringInfo();

My main worry was that buf->data might get repalloc'd via
enlargeStringInfo(), which could cause problems after a longjmp.  We
could also declare it as volatile, but I think you'd unfortunately
have to unvolatize() a bunch.

> * You're failing to re-enable sigint_ctx->enabled when looping
> around for another fgets call.

Oops, fixed.

> * Personally I'd write those assignments like
>
>         *(sigint_ctx->enabled) = true;
>
> as that seems clearer.

Done.

Nathan

Commits

  1. pg_receivewal, pg_recvlogical: allow canceling initial password prompt.

  2. Allow psql's other uses of simple_prompt() to be interrupted by ^C.

  3. Provide a variant of simple_prompt() that can be interrupted by ^C.

  4. Make psql's \password default to CURRENT_USER, not PQuser(conn).