From: Tom Lane <tgl@sss.pgh.pa.us>
To: Bossart, Nathan <bossartn@amazon.com>
Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Subject: Re: Improving psql's \password command
Date: Mon, 15 Nov 2021 13:12:44 -0500
Message-ID: <2141902.1636999964@sss.pgh.pa.us> (raw)
In-Reply-To: <AEA1BA57-BD21-4237-9859-1C04FE38602B@amazon.com>
References: <747443.1635536754@sss.pgh.pa.us>
<C2787D93-6871-4D93-9CF3-CD7BB489F47F@amazon.com>
<765509.1635552418@sss.pgh.pa.us>
<7BAC8A28-148B-4B03-A345-D68071388300@amazon.com>
<1553329.1636647111@sss.pgh.pa.us>
<E0C46CDF-46B2-4388-B53B-96EB2CA0F71E@amazon.com>
<1847699.1636747076@sss.pgh.pa.us>
<AEA1BA57-BD21-4237-9859-1C04FE38602B@amazon.com>
"Bossart, Nathan" <bossartn@amazon.com> writes:
> Here is an attempt at adding control-C support for \password. With
> this patch, we pass sigint_interrupt_jmp and sigint_interrupt_enabled
> all the way down to pg_get_line_append(), which is admittedly a bit
> more complicated than I think would be ideal. I see a couple of other
> calls to simple_prompt() (e.g., \prompt and \connect), and I think the
> same infrastructure could be used for those as well. I'll send some
> follow-up patches for those if this approach seems reasonable.
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.
Some other random observations (not a full review):
* 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".
* 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();
* You're failing to re-enable sigint_ctx->enabled when looping
around for another fgets call.
* Personally I'd write those assignments like
*(sigint_ctx->enabled) = true;
as that seems clearer.
regards, tom lane
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, bossartn@amazon.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Improving psql's \password command
In-Reply-To: <2141902.1636999964@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox