Improving psql's \password command
The attached patch responds to the discussion at [1]/messages/by-id/B340250F-A0E3-43BF-A1FB-2AE36003F68D@gmail.com. That thread
pointed out that if you issue "\password" with no argument, psql tries
to set the password for the role identified by PQuser(conn), that is,
the originally requested login name. That's problematic because
if you've done SET SESSION AUTHORIZATION or SET ROLE, you may no
longer have permissions to set the password for the original role.
Even if you do, that might not be what you were expecting, especially
since the psql documentation specifically says it sets the password
for "the current user".
The solution adopted here is to do "SELECT CURRENT_USER" to acquire
the correct role name, and to include the role name in the password
prompt so as to make it very clear which password is to be set.
While testing that, I noticed another bit of user-unfriendliness:
there's no obvious way to get out of it if you realize you are
setting the wrong user's password. simple_prompt() ignores
control-C, and when you give up and press return, you'll just
get the prompt to enter the password again. If at this point
you have the presence of mind to enter a deliberately different
string, you'll be out of the woods. If you don't, and just hit
return again, you will get this response from the backend:
NOTICE: empty string is not a valid password, clearing password
which is just about the worst default behavior I can think of.
If you're superuser, and you meant to set the password for user1
but typed user2 instead, you just clobbered user2's password,
and you have no easy way to undo that.
What I propose here is that we just refuse to let you set an
empty password this way:
postgres=# \password joe
Enter new password for user "joe":
Empty string is not a valid password.
postgres=#
If you actually want to clear the password, I don't see anything
wrong with entering "alter user joe password null" explicitly.
It's not clear to me whether we ought to back-patch some or all
of this. In the other thread I expressed doubt about back-patching
security-relevant behavior changes, and that still seems like a
serious concern; but on the other hand, the behaviors enumerated
above are pretty bad. Since \password is probably only used
interactively, it seems like we could get away with making these
changes: the addition of the user name to the prompt should be
enough to cue anyone about what's really going to happen.
A compromise position could be to keep PQuser() as the default
target role name in the back branches, but back-patch the other
aspects (the prompt addition and the exit on empty password).
Thoughts?
regards, tom lane
[1]: /messages/by-id/B340250F-A0E3-43BF-A1FB-2AE36003F68D@gmail.com
Attachments:
improve-psql-password-command-1.patchtext/x-diff; charset=us-ascii; name=improve-psql-password-command-1.patchDownload
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 49d4c0e3ce..fac4b1e87f 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2023,28 +2023,53 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
if (active_branch)
{
- char *opt0 = psql_scan_slash_option(scan_state,
+ char *user = psql_scan_slash_option(scan_state,
OT_SQLID, NULL, true);
- char *pw1;
- char *pw2;
+ char *pw1 = NULL;
+ char *pw2 = NULL;
+ PQExpBufferData buf;
- pw1 = simple_prompt("Enter new password: ", false);
- pw2 = simple_prompt("Enter it again: ", false);
+ initPQExpBuffer(&buf);
- if (strcmp(pw1, pw2) != 0)
+ if (user == NULL)
{
- pg_log_error("Passwords didn't match.");
- success = false;
+ /* Fetch current user so we can report whose PW will be changed */
+ PGresult *res;
+
+ res = PSQLexec("SELECT CURRENT_USER");
+ if (!res)
+ success = false;
+ else
+ {
+ user = pg_strdup(PQgetvalue(res, 0, 0));
+ PQclear(res);
+ }
}
- else
- {
- char *user;
- char *encrypted_password;
- if (opt0)
- user = opt0;
+ if (success)
+ {
+ printfPQExpBuffer(&buf, _("Enter new password for user \"%s\": "),
+ user);
+ pw1 = simple_prompt(buf.data, false);
+ if (*pw1 == '\0')
+ {
+ pg_log_error("Empty string is not a valid password.");
+ success = false;
+ }
else
- user = PQuser(pset.db);
+ {
+ pw2 = simple_prompt("Enter it again: ", false);
+ if (strcmp(pw1, pw2) != 0)
+ {
+ pg_log_error("Passwords didn't match.");
+ success = false;
+ }
+ }
+ }
+
+ if (success)
+ {
+ char *encrypted_password;
encrypted_password = PQencryptPasswordConn(pset.db, pw1, user, NULL);
@@ -2055,15 +2080,12 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
}
else
{
- PQExpBufferData buf;
PGresult *res;
- initPQExpBuffer(&buf);
printfPQExpBuffer(&buf, "ALTER USER %s PASSWORD ",
fmtId(user));
appendStringLiteralConn(&buf, encrypted_password, pset.db);
res = PSQLexec(buf.data);
- termPQExpBuffer(&buf);
if (!res)
success = false;
else
@@ -2072,10 +2094,13 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
}
}
- if (opt0)
- free(opt0);
- free(pw1);
- free(pw2);
+ if (user)
+ free(user);
+ if (pw1)
+ free(pw1);
+ if (pw2)
+ free(pw2);
+ termPQExpBuffer(&buf);
}
else
ignore_slash_options(scan_state);
On 10/29/21, 12:47 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
While testing that, I noticed another bit of user-unfriendliness:
there's no obvious way to get out of it if you realize you are
setting the wrong user's password. simple_prompt() ignores
control-C, and when you give up and press return, you'll just
get the prompt to enter the password again. If at this point
you have the presence of mind to enter a deliberately different
string, you'll be out of the woods. If you don't, and just hit
return again, you will get this response from the backend:NOTICE: empty string is not a valid password, clearing password
which is just about the worst default behavior I can think of.
If you're superuser, and you meant to set the password for user1
but typed user2 instead, you just clobbered user2's password,
and you have no easy way to undo that.
Well, as of bf6b9e9, "ALTER ROLE nathan PASSWORD ''" is effectively
the same as "ALTER ROLE nathan PASSWORD NULL". I agree about the
user-unfriendliness, but maybe simple_prompt() ignoring control-C is
the root-cause of the user-unfriendliness. I'm not sure that it's
totally unreasonable to expect the password to be cleared if you don't
enter a new one in the prompts.
A compromise position could be to keep PQuser() as the default
target role name in the back branches, but back-patch the other
aspects (the prompt addition and the exit on empty password).
I think it would be okay to back-patch the PQuser() fix. I would
argue that it's clearly a bug because the docs say it uses the current
user.
Nathan
"Bossart, Nathan" <bossartn@amazon.com> writes:
On 10/29/21, 12:47 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
While testing that, I noticed another bit of user-unfriendliness:
there's no obvious way to get out of it if you realize you are
setting the wrong user's password. simple_prompt() ignores
control-C, and when you give up and press return, you'll just
get the prompt to enter the password again.
Well, as of bf6b9e9, "ALTER ROLE nathan PASSWORD ''" is effectively
the same as "ALTER ROLE nathan PASSWORD NULL". I agree about the
user-unfriendliness, but maybe simple_prompt() ignoring control-C is
the root-cause of the user-unfriendliness.
I was afraid somebody would say that. I have looked at it, but AFAICS
we'd have to duplicate all of sprompt.c and nearly all of pg_get_line.c
in order to tie it into psql's SIGINT infrastructure, since we wouldn't
dare enable the signal handler except during the innermost fgets() call,
and if we did get a signal we'd still need to clean up the terminal echo
state, so we couldn't just longjmp out of simple_prompt(). The
cost/benefit ratio of that doesn't look very good.
(Note that most callers of simple_prompt() don't need to sweat over
this because they haven't moved SIGINT handling off the default:
they're OK with just terminating on control-C.)
regards, tom lane
On 10/29/21, 5:07 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
"Bossart, Nathan" <bossartn@amazon.com> writes:
Well, as of bf6b9e9, "ALTER ROLE nathan PASSWORD ''" is effectively
the same as "ALTER ROLE nathan PASSWORD NULL". I agree about the
user-unfriendliness, but maybe simple_prompt() ignoring control-C is
the root-cause of the user-unfriendliness.I was afraid somebody would say that. I have looked at it, but AFAICS
we'd have to duplicate all of sprompt.c and nearly all of pg_get_line.c
in order to tie it into psql's SIGINT infrastructure, since we wouldn't
dare enable the signal handler except during the innermost fgets() call,
and if we did get a signal we'd still need to clean up the terminal echo
state, so we couldn't just longjmp out of simple_prompt(). The
cost/benefit ratio of that doesn't look very good.
Hm. Is it really necessary to duplicate all of sprompt.c and
pg_get_line.c? Would it be possible to teach the existing functions
how to optionally enable SIGINT handling instead? I wouldn't mind
trying my hand at this if it seems like a reasonable approach.
Nathan
"Bossart, Nathan" <bossartn@amazon.com> writes:
On 10/29/21, 5:07 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
I was afraid somebody would say that. I have looked at it, but AFAICS
we'd have to duplicate all of sprompt.c and nearly all of pg_get_line.c
in order to tie it into psql's SIGINT infrastructure, since we wouldn't
dare enable the signal handler except during the innermost fgets() call,
and if we did get a signal we'd still need to clean up the terminal echo
state, so we couldn't just longjmp out of simple_prompt(). The
cost/benefit ratio of that doesn't look very good.
Hm. Is it really necessary to duplicate all of sprompt.c and
pg_get_line.c? Would it be possible to teach the existing functions
how to optionally enable SIGINT handling instead? I wouldn't mind
trying my hand at this if it seems like a reasonable approach.
It seems to me it'd overcomplicate simple_prompt's API for one use-case
... but if you want to try it, step right up. (I suppose some of that
objection could be overcome by making simple_prompt into a wrapper
around another function not_so_simple_prompt.)
regards, tom lane
On 11/11/21, 8:12 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
It seems to me it'd overcomplicate simple_prompt's API for one use-case
... but if you want to try it, step right up. (I suppose some of that
objection could be overcome by making simple_prompt into a wrapper
around another function not_so_simple_prompt.)
I haven't started on the SIGINT stuff yet, but I did extract the
PQuser() fix so that we can hopefully get that one out of the way. I
made some small adjustments in an attempt to keep the branching in
this function from getting too complicated.
Nathan
Attachments:
v2-0001-Adjust-password-to-use-current-user-instead-of-au.patchapplication/octet-stream; name=v2-0001-Adjust-password-to-use-current-user-instead-of-au.patchDownload
From 02459bbd107d7bf04e97184a00d823c860688a51 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Fri, 12 Nov 2021 16:57:36 +0000
Subject: [PATCH v2 1/2] Adjust \password to use current user instead of
authenticated user by default.
---
src/bin/psql/command.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 49d4c0e3ce..5619ed685a 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2023,11 +2023,24 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
if (active_branch)
{
- char *opt0 = psql_scan_slash_option(scan_state,
+ char *user = psql_scan_slash_option(scan_state,
OT_SQLID, NULL, true);
char *pw1;
char *pw2;
+ if (user == NULL)
+ {
+ /* Fetch current user so we can report whose PW will be changed */
+ PGresult *res;
+
+ res = PSQLexec("SELECT CURRENT_USER");
+ if (!res)
+ return PSQL_CMD_ERROR;
+
+ user = pg_strdup(PQgetvalue(res, 0, 0));
+ PQclear(res);
+ }
+
pw1 = simple_prompt("Enter new password: ", false);
pw2 = simple_prompt("Enter it again: ", false);
@@ -2038,14 +2051,8 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
}
else
{
- char *user;
char *encrypted_password;
- if (opt0)
- user = opt0;
- else
- user = PQuser(pset.db);
-
encrypted_password = PQencryptPasswordConn(pset.db, pw1, user, NULL);
if (!encrypted_password)
@@ -2072,8 +2079,8 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
}
}
- if (opt0)
- free(opt0);
+ if (user)
+ free(user);
free(pw1);
free(pw2);
}
--
2.16.6
v2-0002-Add-role-name-to-password-prompt.patchapplication/octet-stream; name=v2-0002-Add-role-name-to-password-prompt.patchDownload
From 0f92b7b1d992c6174b49358f337d84ba719b9576 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Fri, 12 Nov 2021 17:10:51 +0000
Subject: [PATCH v2 2/2] Add role name to \password prompt.
---
src/bin/psql/command.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 5619ed685a..eedc6243d3 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2027,6 +2027,7 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
OT_SQLID, NULL, true);
char *pw1;
char *pw2;
+ PQExpBufferData buf;
if (user == NULL)
{
@@ -2041,7 +2042,10 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
PQclear(res);
}
- pw1 = simple_prompt("Enter new password: ", false);
+ initPQExpBuffer(&buf);
+ printfPQExpBuffer(&buf, _("Enter new password for user \"%s\": "), user);
+
+ pw1 = simple_prompt(buf.data, false);
pw2 = simple_prompt("Enter it again: ", false);
if (strcmp(pw1, pw2) != 0)
@@ -2062,15 +2066,12 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
}
else
{
- PQExpBufferData buf;
PGresult *res;
- initPQExpBuffer(&buf);
printfPQExpBuffer(&buf, "ALTER USER %s PASSWORD ",
fmtId(user));
appendStringLiteralConn(&buf, encrypted_password, pset.db);
res = PSQLexec(buf.data);
- termPQExpBuffer(&buf);
if (!res)
success = false;
else
@@ -2083,6 +2084,7 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
free(user);
free(pw1);
free(pw2);
+ termPQExpBuffer(&buf);
}
else
ignore_slash_options(scan_state);
--
2.16.6
"Bossart, Nathan" <bossartn@amazon.com> writes:
I haven't started on the SIGINT stuff yet, but I did extract the
PQuser() fix so that we can hopefully get that one out of the way. I
made some small adjustments in an attempt to keep the branching in
this function from getting too complicated.
Right, it makes sense to consider just this much as the back-patchable
part, and leave the more complicated messing with simple_prompt()
for HEAD only. I made a couple further cosmetic tweaks and pushed it.
regards, tom lane
On 11/12/21, 11:58 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
"Bossart, Nathan" <bossartn@amazon.com> writes:
I haven't started on the SIGINT stuff yet, but I did extract the
PQuser() fix so that we can hopefully get that one out of the way. I
made some small adjustments in an attempt to keep the branching in
this function from getting too complicated.Right, it makes sense to consider just this much as the back-patchable
part, and leave the more complicated messing with simple_prompt()
for HEAD only. I made a couple further cosmetic tweaks and pushed it.
Thanks!
Nathan
On 11/12/21, 11:58 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
Right, it makes sense to consider just this much as the back-patchable
part, and leave the more complicated messing with simple_prompt()
for HEAD only. I made a couple further cosmetic tweaks and pushed it.
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.
Nathan
Attachments:
v1-0001-Add-control-C-handling-for-psql-s-password-comman.patchapplication/octet-stream; name=v1-0001-Add-control-C-handling-for-psql-s-password-comman.patchDownload
From 457d72df44e177774c72049345649e6a63544bb9 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Sat, 13 Nov 2021 22:56:08 +0000
Subject: [PATCH v1 1/1] Add control-C handling for psql's \password command.
---
src/backend/libpq/hba.c | 2 +-
src/bin/initdb/initdb.c | 2 +-
src/bin/psql/command.c | 24 +++++++++++++++--------
src/common/pg_get_line.c | 47 +++++++++++++++++++++++++++++++++++++--------
src/common/sprompt.c | 17 +++++++++++++++-
src/include/common/string.h | 15 +++++++++++++--
6 files changed, 86 insertions(+), 21 deletions(-)
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 3be8778d21..4328eb74fe 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -500,7 +500,7 @@ tokenize_file(const char *filename, FILE *file, List **tok_lines, int elevel)
/* Collect the next input line, handling backslash continuations */
resetStringInfo(&buf);
- while (pg_get_line_append(file, &buf))
+ while (pg_get_line_append(file, &buf, NULL))
{
/* Strip trailing newline, including \r in case we're on Windows */
buf.len = pg_strip_crlf(buf.data);
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 31839c1a19..3c61c789e4 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -1497,7 +1497,7 @@ get_su_pwd(void)
pwfilename);
exit(1);
}
- pwd1 = pg_get_line(pwf);
+ pwd1 = pg_get_line(pwf, NULL);
if (!pwd1)
{
if (ferror(pwf))
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 3de9d096fd..142c51aa15 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2025,9 +2025,10 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
{
char *user = psql_scan_slash_option(scan_state,
OT_SQLID, NULL, true);
- char *pw1;
- char *pw2;
+ char *pw1 = NULL;
+ char *pw2 = NULL;
PQExpBufferData buf;
+ SigintInterruptContext sigint_ctx;
if (user == NULL)
{
@@ -2042,18 +2043,23 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
PQclear(res);
}
+ sigint_ctx.jmpbuf = &sigint_interrupt_jmp;
+ sigint_ctx.enabled = &sigint_interrupt_enabled;
+ sigint_ctx.canceled = false;
+
initPQExpBuffer(&buf);
printfPQExpBuffer(&buf, _("Enter new password for user \"%s\": "), user);
- pw1 = simple_prompt(buf.data, false);
- pw2 = simple_prompt("Enter it again: ", false);
+ pw1 = simple_prompt_extended(buf.data, false, &sigint_ctx);
+ if (!sigint_ctx.canceled)
+ pw2 = simple_prompt_extended("Enter it again: ", false, &sigint_ctx);
- if (strcmp(pw1, pw2) != 0)
+ if (!sigint_ctx.canceled && strcmp(pw1, pw2) != 0)
{
pg_log_error("Passwords didn't match.");
success = false;
}
- else
+ else if (!sigint_ctx.canceled)
{
char *encrypted_password;
@@ -2081,8 +2087,10 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
}
free(user);
- free(pw1);
- free(pw2);
+ if (pw1)
+ free(pw1);
+ if (pw2)
+ free(pw2);
termPQExpBuffer(&buf);
}
else
diff --git a/src/common/pg_get_line.c b/src/common/pg_get_line.c
index a80d196156..bd2f813d82 100644
--- a/src/common/pg_get_line.c
+++ b/src/common/pg_get_line.c
@@ -47,25 +47,34 @@
* to collect lots of long-lived data. A less memory-hungry option
* is to use pg_get_line_buf() or pg_get_line_append() in a loop,
* then pstrdup() each line.
+ *
+ * sigint_ctx can optionally be provided to allow this function to be
+ * canceled via an existing SIGINT signal handler. If canceled, this
+ * function returns NULL, and sigint_ctx->canceled is set to true.
*/
char *
-pg_get_line(FILE *stream)
+pg_get_line(FILE *stream, SigintInterruptContext *sigint_ctx)
{
- StringInfoData buf;
+ StringInfo buf;
+ char *ret;
- initStringInfo(&buf);
+ /* make sure buf is palloc'd so we don't lose changes after a longjmp */
+ buf = makeStringInfo();
- if (!pg_get_line_append(stream, &buf))
+ if (!pg_get_line_append(stream, buf, sigint_ctx))
{
/* ensure that free() doesn't mess up errno */
int save_errno = errno;
- pfree(buf.data);
+ pfree(buf->data);
+ pfree(buf);
errno = save_errno;
return NULL;
}
- return buf.data;
+ ret = buf->data;
+ pfree(buf);
+ return ret;;
}
/*
@@ -89,7 +98,7 @@ pg_get_line_buf(FILE *stream, StringInfo buf)
{
/* We just need to drop any data from the previous call */
resetStringInfo(buf);
- return pg_get_line_append(stream, buf);
+ return pg_get_line_append(stream, buf, NULL);
}
/*
@@ -107,15 +116,34 @@ pg_get_line_buf(FILE *stream, StringInfo buf)
*
* In the false-result case, the contents of *buf are logically unmodified,
* though it's possible that the buffer has been resized.
+ *
+ * sigint_ctx can optionally be provided to allow this function to be
+ * canceled via an existing SIGINT signal handler. If canceled, this
+ * function returns false, and sigint_ctx->canceled is set to true.
*/
bool
-pg_get_line_append(FILE *stream, StringInfo buf)
+pg_get_line_append(FILE *stream, StringInfo buf,
+ SigintInterruptContext *sigint_ctx)
{
int orig_len = buf->len;
+ if (sigint_ctx && sigsetjmp(*sigint_ctx->jmpbuf, 1) != 0)
+ {
+ /* got here with longjmp */
+ sigint_ctx->canceled = true;
+ return false;
+ }
+
+ /* enable longjmp while waiting for input */
+ if (sigint_ctx)
+ *sigint_ctx->enabled = true;
+
/* Read some data, appending it to whatever we already have */
while (fgets(buf->data + buf->len, buf->maxlen - buf->len, stream) != NULL)
{
+ if (sigint_ctx)
+ *sigint_ctx->enabled = false;
+
buf->len += strlen(buf->data + buf->len);
/* Done if we have collected a newline */
@@ -126,6 +154,9 @@ pg_get_line_append(FILE *stream, StringInfo buf)
enlargeStringInfo(buf, 128);
}
+ if (sigint_ctx)
+ *sigint_ctx->enabled = false;
+
/* Check for I/O errors and EOF */
if (ferror(stream) || buf->len == orig_len)
{
diff --git a/src/common/sprompt.c b/src/common/sprompt.c
index f3a891a260..6fb133114a 100644
--- a/src/common/sprompt.c
+++ b/src/common/sprompt.c
@@ -36,6 +36,21 @@
*/
char *
simple_prompt(const char *prompt, bool echo)
+{
+ return simple_prompt_extended(prompt, echo, NULL);
+}
+
+/*
+ * simple_prompt_extended
+ *
+ * This is the same as simple_prompt(), except that sigint_ctx can
+ * optionally be provided to allow this function to be canceled via an
+ * existing SIGINT signal handler. If canceled, this function returns an
+ * empty string, and sigint_ctx->canceled is set to true.
+ */
+char *
+simple_prompt_extended(const char *prompt, bool echo,
+ SigintInterruptContext *sigint_ctx)
{
char *result;
FILE *termin,
@@ -126,7 +141,7 @@ simple_prompt(const char *prompt, bool echo)
fflush(termout);
}
- result = pg_get_line(termin);
+ result = pg_get_line(termin, sigint_ctx);
/* If we failed to read anything, just return an empty string */
if (result == NULL)
diff --git a/src/include/common/string.h b/src/include/common/string.h
index 686c158efe..331f23ef33 100644
--- a/src/include/common/string.h
+++ b/src/include/common/string.h
@@ -10,8 +10,16 @@
#ifndef COMMON_STRING_H
#define COMMON_STRING_H
+#include <setjmp.h>
+
struct StringInfoData; /* avoid including stringinfo.h here */
+typedef struct SigintInterruptContext {
+ sigjmp_buf *jmpbuf; /* preexisting longjmp buffer */
+ volatile bool *enabled; /* used to enable/disable SIGINT handling */
+ bool canceled; /* indicates whether cancellation occurred */
+} SigintInterruptContext;
+
/* functions in src/common/string.c */
extern bool pg_str_endswith(const char *str, const char *end);
extern int strtoint(const char *pg_restrict str, char **pg_restrict endptr,
@@ -21,11 +29,14 @@ extern int pg_strip_crlf(char *str);
extern bool pg_is_ascii(const char *str);
/* functions in src/common/pg_get_line.c */
-extern char *pg_get_line(FILE *stream);
+extern char *pg_get_line(FILE *stream, SigintInterruptContext *sigint_ctx);
extern bool pg_get_line_buf(FILE *stream, struct StringInfoData *buf);
-extern bool pg_get_line_append(FILE *stream, struct StringInfoData *buf);
+extern bool pg_get_line_append(FILE *stream, struct StringInfoData *buf,
+ SigintInterruptContext *sigint_ctx);
/* functions in src/common/sprompt.c */
extern char *simple_prompt(const char *prompt, bool echo);
+extern char *simple_prompt_extended(const char *prompt, bool echo,
+ SigintInterruptContext *sigint_ctx);
#endif /* COMMON_STRING_H */
--
2.16.6
"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
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
Attachments:
v2-0001-Add-control-C-handling-for-psql-s-password-comman.patchapplication/octet-stream; name=v2-0001-Add-control-C-handling-for-psql-s-password-comman.patchDownload
From c337f517e90588fa924e7422363b743632fa935f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Mon, 15 Nov 2021 20:25:39 +0000
Subject: [PATCH v2 1/1] Add control-C handling for psql's \password command.
---
src/backend/libpq/hba.c | 2 +-
src/bin/initdb/initdb.c | 2 +-
src/bin/psql/command.c | 24 ++++++++++++-------
src/common/pg_get_line.c | 58 ++++++++++++++++++++++++++++++++++++++-------
src/common/sprompt.c | 18 +++++++++++++-
src/include/common/string.h | 13 ++++++++--
6 files changed, 96 insertions(+), 21 deletions(-)
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 3be8778d21..4328eb74fe 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -500,7 +500,7 @@ tokenize_file(const char *filename, FILE *file, List **tok_lines, int elevel)
/* Collect the next input line, handling backslash continuations */
resetStringInfo(&buf);
- while (pg_get_line_append(file, &buf))
+ while (pg_get_line_append(file, &buf, NULL))
{
/* Strip trailing newline, including \r in case we're on Windows */
buf.len = pg_strip_crlf(buf.data);
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 31839c1a19..3c61c789e4 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -1497,7 +1497,7 @@ get_su_pwd(void)
pwfilename);
exit(1);
}
- pwd1 = pg_get_line(pwf);
+ pwd1 = pg_get_line(pwf, NULL);
if (!pwd1)
{
if (ferror(pwf))
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 3de9d096fd..142c51aa15 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2025,9 +2025,10 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
{
char *user = psql_scan_slash_option(scan_state,
OT_SQLID, NULL, true);
- char *pw1;
- char *pw2;
+ char *pw1 = NULL;
+ char *pw2 = NULL;
PQExpBufferData buf;
+ SigintInterruptContext sigint_ctx;
if (user == NULL)
{
@@ -2042,18 +2043,23 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
PQclear(res);
}
+ sigint_ctx.jmpbuf = &sigint_interrupt_jmp;
+ sigint_ctx.enabled = &sigint_interrupt_enabled;
+ sigint_ctx.canceled = false;
+
initPQExpBuffer(&buf);
printfPQExpBuffer(&buf, _("Enter new password for user \"%s\": "), user);
- pw1 = simple_prompt(buf.data, false);
- pw2 = simple_prompt("Enter it again: ", false);
+ pw1 = simple_prompt_extended(buf.data, false, &sigint_ctx);
+ if (!sigint_ctx.canceled)
+ pw2 = simple_prompt_extended("Enter it again: ", false, &sigint_ctx);
- if (strcmp(pw1, pw2) != 0)
+ if (!sigint_ctx.canceled && strcmp(pw1, pw2) != 0)
{
pg_log_error("Passwords didn't match.");
success = false;
}
- else
+ else if (!sigint_ctx.canceled)
{
char *encrypted_password;
@@ -2081,8 +2087,10 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
}
free(user);
- free(pw1);
- free(pw2);
+ if (pw1)
+ free(pw1);
+ if (pw2)
+ free(pw2);
termPQExpBuffer(&buf);
}
else
diff --git a/src/common/pg_get_line.c b/src/common/pg_get_line.c
index a80d196156..cb684dc714 100644
--- a/src/common/pg_get_line.c
+++ b/src/common/pg_get_line.c
@@ -18,6 +18,8 @@
#include "postgres_fe.h"
#endif
+#include <setjmp.h>
+
#include "common/string.h"
#include "lib/stringinfo.h"
@@ -47,25 +49,38 @@
* to collect lots of long-lived data. A less memory-hungry option
* is to use pg_get_line_buf() or pg_get_line_append() in a loop,
* then pstrdup() each line.
+ *
+ * sigint_ctx can optionally be provided to allow this function to be
+ * canceled via an existing SIGINT signal handler that will longjmp to the
+ * specified place only when *(sigint_ctx->enabled) is true. If canceled,
+ * this function returns NULL, and sigint_ctx->canceled is set to true.
*/
char *
-pg_get_line(FILE *stream)
+pg_get_line(FILE *stream, SigintInterruptContext *sigint_ctx)
{
- StringInfoData buf;
+ StringInfo buf;
+ char *ret;
- initStringInfo(&buf);
+ /*
+ * Make sure buf is palloc'd to avoid using local variables that may
+ * change between sigsetjmp() and siglongjmp().
+ */
+ buf = makeStringInfo();
- if (!pg_get_line_append(stream, &buf))
+ if (!pg_get_line_append(stream, buf, sigint_ctx))
{
/* ensure that free() doesn't mess up errno */
int save_errno = errno;
- pfree(buf.data);
+ pfree(buf->data);
+ pfree(buf);
errno = save_errno;
return NULL;
}
- return buf.data;
+ ret = buf->data;
+ pfree(buf);
+ return ret;
}
/*
@@ -89,7 +104,7 @@ pg_get_line_buf(FILE *stream, StringInfo buf)
{
/* We just need to drop any data from the previous call */
resetStringInfo(buf);
- return pg_get_line_append(stream, buf);
+ return pg_get_line_append(stream, buf, NULL);
}
/*
@@ -107,15 +122,35 @@ pg_get_line_buf(FILE *stream, StringInfo buf)
*
* In the false-result case, the contents of *buf are logically unmodified,
* though it's possible that the buffer has been resized.
+ *
+ * sigint_ctx can optionally be provided to allow this function to be
+ * canceled via an existing SIGINT signal handler that will longjmp to the
+ * specified place only when *(sigint_ctx->enabled) is true. If canceled,
+ * this function returns false, and sigint_ctx->canceled is set to true.
*/
bool
-pg_get_line_append(FILE *stream, StringInfo buf)
+pg_get_line_append(FILE *stream, StringInfo buf,
+ SigintInterruptContext *sigint_ctx)
{
int orig_len = buf->len;
+ if (sigint_ctx && sigsetjmp(*((sigjmp_buf *) sigint_ctx->jmpbuf), 1) != 0)
+ {
+ /* got here with longjmp */
+ sigint_ctx->canceled = true;
+ return false;
+ }
+
+ /* enable longjmp while waiting for input */
+ if (sigint_ctx)
+ *(sigint_ctx->enabled) = true;
+
/* Read some data, appending it to whatever we already have */
while (fgets(buf->data + buf->len, buf->maxlen - buf->len, stream) != NULL)
{
+ if (sigint_ctx)
+ *(sigint_ctx->enabled) = false;
+
buf->len += strlen(buf->data + buf->len);
/* Done if we have collected a newline */
@@ -124,8 +159,15 @@ pg_get_line_append(FILE *stream, StringInfo buf)
/* Make some more room in the buffer, and loop to read more data */
enlargeStringInfo(buf, 128);
+
+ /* enable longjmp while waiting for input */
+ if (sigint_ctx)
+ *(sigint_ctx->enabled) = true;
}
+ if (sigint_ctx)
+ *(sigint_ctx->enabled) = false;
+
/* Check for I/O errors and EOF */
if (ferror(stream) || buf->len == orig_len)
{
diff --git a/src/common/sprompt.c b/src/common/sprompt.c
index f3a891a260..2a86bced38 100644
--- a/src/common/sprompt.c
+++ b/src/common/sprompt.c
@@ -36,6 +36,22 @@
*/
char *
simple_prompt(const char *prompt, bool echo)
+{
+ return simple_prompt_extended(prompt, echo, NULL);
+}
+
+/*
+ * simple_prompt_extended
+ *
+ * This is the same as simple_prompt(), except that sigint_ctx can
+ * optionally be provided to allow this function to be canceled via an
+ * existing SIGINT signal handler that will longjmp to the specified place
+ * only when *(sigint_ctx->enabled) is true. If canceled, this function
+ * returns an empty string, and sigint_ctx->canceled is set to true.
+ */
+char *
+simple_prompt_extended(const char *prompt, bool echo,
+ SigintInterruptContext *sigint_ctx)
{
char *result;
FILE *termin,
@@ -126,7 +142,7 @@ simple_prompt(const char *prompt, bool echo)
fflush(termout);
}
- result = pg_get_line(termin);
+ result = pg_get_line(termin, sigint_ctx);
/* If we failed to read anything, just return an empty string */
if (result == NULL)
diff --git a/src/include/common/string.h b/src/include/common/string.h
index 686c158efe..238c3dc895 100644
--- a/src/include/common/string.h
+++ b/src/include/common/string.h
@@ -12,6 +12,12 @@
struct StringInfoData; /* avoid including stringinfo.h here */
+typedef struct SigintInterruptContext {
+ void *jmpbuf; /* existing longjmp buffer (really a sigjmp_buf *) */
+ volatile bool *enabled; /* used to enable/disable SIGINT handling */
+ bool canceled; /* indicates whether cancellation occurred */
+} SigintInterruptContext;
+
/* functions in src/common/string.c */
extern bool pg_str_endswith(const char *str, const char *end);
extern int strtoint(const char *pg_restrict str, char **pg_restrict endptr,
@@ -21,11 +27,14 @@ extern int pg_strip_crlf(char *str);
extern bool pg_is_ascii(const char *str);
/* functions in src/common/pg_get_line.c */
-extern char *pg_get_line(FILE *stream);
+extern char *pg_get_line(FILE *stream, SigintInterruptContext *sigint_ctx);
extern bool pg_get_line_buf(FILE *stream, struct StringInfoData *buf);
-extern bool pg_get_line_append(FILE *stream, struct StringInfoData *buf);
+extern bool pg_get_line_append(FILE *stream, struct StringInfoData *buf,
+ SigintInterruptContext *sigint_ctx);
/* functions in src/common/sprompt.c */
extern char *simple_prompt(const char *prompt, bool echo);
+extern char *simple_prompt_extended(const char *prompt, bool echo,
+ SigintInterruptContext *sigint_ctx);
#endif /* COMMON_STRING_H */
--
2.16.6
"Bossart, Nathan" <bossartn@amazon.com> writes:
On 11/15/21, 10:13 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
* 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.
So what? That has nothing to do with whether the buf struct itself
is alloc'd or not. Besides which, no longjmp is going to happen
during any reallocation. I'm not entirely sure what scenario
you're worried about, but I don't see how alloc'ing the
StringInfoData struct would make it any safer. If anything it'd
be less safe, since the StringInfoData is certain to be on the
stack, while a buf pointer variable is likely to be kept in a
register. But really that doesn't matter anyhow, since this
is a stack level below where the sigsetjmp call is. AFAIK the
only longjmp clobber risk is to pg_get_line_append's mutable
local variables, of which there are none.
regards, tom lane
On 11/15/21, 1:04 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
register. But really that doesn't matter anyhow, since this
is a stack level below where the sigsetjmp call is. AFAIK the
only longjmp clobber risk is to pg_get_line_append's mutable
local variables, of which there are none.
*facepalm*
You are right. I'm not sure what I was thinking. Attached a v3
with that part removed.
Nathan
Attachments:
v3-0001-Add-control-C-handling-for-psql-s-password-comman.patchapplication/octet-stream; name=v3-0001-Add-control-C-handling-for-psql-s-password-comman.patchDownload
From 05a8fe52db98330aab54179e2062960e927e32da Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Mon, 15 Nov 2021 21:43:11 +0000
Subject: [PATCH v3 1/1] Add control-C handling for psql's \password command.
---
src/backend/libpq/hba.c | 2 +-
src/bin/initdb/initdb.c | 2 +-
src/bin/psql/command.c | 24 ++++++++++++++++--------
src/common/pg_get_line.c | 42 ++++++++++++++++++++++++++++++++++++++----
src/common/sprompt.c | 18 +++++++++++++++++-
src/include/common/string.h | 13 +++++++++++--
6 files changed, 84 insertions(+), 17 deletions(-)
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 3be8778d21..4328eb74fe 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -500,7 +500,7 @@ tokenize_file(const char *filename, FILE *file, List **tok_lines, int elevel)
/* Collect the next input line, handling backslash continuations */
resetStringInfo(&buf);
- while (pg_get_line_append(file, &buf))
+ while (pg_get_line_append(file, &buf, NULL))
{
/* Strip trailing newline, including \r in case we're on Windows */
buf.len = pg_strip_crlf(buf.data);
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 31839c1a19..3c61c789e4 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -1497,7 +1497,7 @@ get_su_pwd(void)
pwfilename);
exit(1);
}
- pwd1 = pg_get_line(pwf);
+ pwd1 = pg_get_line(pwf, NULL);
if (!pwd1)
{
if (ferror(pwf))
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 3de9d096fd..142c51aa15 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2025,9 +2025,10 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
{
char *user = psql_scan_slash_option(scan_state,
OT_SQLID, NULL, true);
- char *pw1;
- char *pw2;
+ char *pw1 = NULL;
+ char *pw2 = NULL;
PQExpBufferData buf;
+ SigintInterruptContext sigint_ctx;
if (user == NULL)
{
@@ -2042,18 +2043,23 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
PQclear(res);
}
+ sigint_ctx.jmpbuf = &sigint_interrupt_jmp;
+ sigint_ctx.enabled = &sigint_interrupt_enabled;
+ sigint_ctx.canceled = false;
+
initPQExpBuffer(&buf);
printfPQExpBuffer(&buf, _("Enter new password for user \"%s\": "), user);
- pw1 = simple_prompt(buf.data, false);
- pw2 = simple_prompt("Enter it again: ", false);
+ pw1 = simple_prompt_extended(buf.data, false, &sigint_ctx);
+ if (!sigint_ctx.canceled)
+ pw2 = simple_prompt_extended("Enter it again: ", false, &sigint_ctx);
- if (strcmp(pw1, pw2) != 0)
+ if (!sigint_ctx.canceled && strcmp(pw1, pw2) != 0)
{
pg_log_error("Passwords didn't match.");
success = false;
}
- else
+ else if (!sigint_ctx.canceled)
{
char *encrypted_password;
@@ -2081,8 +2087,10 @@ exec_command_password(PsqlScanState scan_state, bool active_branch)
}
free(user);
- free(pw1);
- free(pw2);
+ if (pw1)
+ free(pw1);
+ if (pw2)
+ free(pw2);
termPQExpBuffer(&buf);
}
else
diff --git a/src/common/pg_get_line.c b/src/common/pg_get_line.c
index a80d196156..49e18eec82 100644
--- a/src/common/pg_get_line.c
+++ b/src/common/pg_get_line.c
@@ -18,6 +18,8 @@
#include "postgres_fe.h"
#endif
+#include <setjmp.h>
+
#include "common/string.h"
#include "lib/stringinfo.h"
@@ -47,15 +49,20 @@
* to collect lots of long-lived data. A less memory-hungry option
* is to use pg_get_line_buf() or pg_get_line_append() in a loop,
* then pstrdup() each line.
+ *
+ * sigint_ctx can optionally be provided to allow this function to be
+ * canceled via an existing SIGINT signal handler that will longjmp to the
+ * specified place only when *(sigint_ctx->enabled) is true. If canceled,
+ * this function returns NULL, and sigint_ctx->canceled is set to true.
*/
char *
-pg_get_line(FILE *stream)
+pg_get_line(FILE *stream, SigintInterruptContext *sigint_ctx)
{
StringInfoData buf;
initStringInfo(&buf);
- if (!pg_get_line_append(stream, &buf))
+ if (!pg_get_line_append(stream, &buf, sigint_ctx))
{
/* ensure that free() doesn't mess up errno */
int save_errno = errno;
@@ -89,7 +96,7 @@ pg_get_line_buf(FILE *stream, StringInfo buf)
{
/* We just need to drop any data from the previous call */
resetStringInfo(buf);
- return pg_get_line_append(stream, buf);
+ return pg_get_line_append(stream, buf, NULL);
}
/*
@@ -107,15 +114,35 @@ pg_get_line_buf(FILE *stream, StringInfo buf)
*
* In the false-result case, the contents of *buf are logically unmodified,
* though it's possible that the buffer has been resized.
+ *
+ * sigint_ctx can optionally be provided to allow this function to be
+ * canceled via an existing SIGINT signal handler that will longjmp to the
+ * specified place only when *(sigint_ctx->enabled) is true. If canceled,
+ * this function returns false, and sigint_ctx->canceled is set to true.
*/
bool
-pg_get_line_append(FILE *stream, StringInfo buf)
+pg_get_line_append(FILE *stream, StringInfo buf,
+ SigintInterruptContext *sigint_ctx)
{
int orig_len = buf->len;
+ if (sigint_ctx && sigsetjmp(*((sigjmp_buf *) sigint_ctx->jmpbuf), 1) != 0)
+ {
+ /* got here with longjmp */
+ sigint_ctx->canceled = true;
+ return false;
+ }
+
+ /* enable longjmp while waiting for input */
+ if (sigint_ctx)
+ *(sigint_ctx->enabled) = true;
+
/* Read some data, appending it to whatever we already have */
while (fgets(buf->data + buf->len, buf->maxlen - buf->len, stream) != NULL)
{
+ if (sigint_ctx)
+ *(sigint_ctx->enabled) = false;
+
buf->len += strlen(buf->data + buf->len);
/* Done if we have collected a newline */
@@ -124,8 +151,15 @@ pg_get_line_append(FILE *stream, StringInfo buf)
/* Make some more room in the buffer, and loop to read more data */
enlargeStringInfo(buf, 128);
+
+ /* enable longjmp while waiting for input */
+ if (sigint_ctx)
+ *(sigint_ctx->enabled) = true;
}
+ if (sigint_ctx)
+ *(sigint_ctx->enabled) = false;
+
/* Check for I/O errors and EOF */
if (ferror(stream) || buf->len == orig_len)
{
diff --git a/src/common/sprompt.c b/src/common/sprompt.c
index f3a891a260..2a86bced38 100644
--- a/src/common/sprompt.c
+++ b/src/common/sprompt.c
@@ -36,6 +36,22 @@
*/
char *
simple_prompt(const char *prompt, bool echo)
+{
+ return simple_prompt_extended(prompt, echo, NULL);
+}
+
+/*
+ * simple_prompt_extended
+ *
+ * This is the same as simple_prompt(), except that sigint_ctx can
+ * optionally be provided to allow this function to be canceled via an
+ * existing SIGINT signal handler that will longjmp to the specified place
+ * only when *(sigint_ctx->enabled) is true. If canceled, this function
+ * returns an empty string, and sigint_ctx->canceled is set to true.
+ */
+char *
+simple_prompt_extended(const char *prompt, bool echo,
+ SigintInterruptContext *sigint_ctx)
{
char *result;
FILE *termin,
@@ -126,7 +142,7 @@ simple_prompt(const char *prompt, bool echo)
fflush(termout);
}
- result = pg_get_line(termin);
+ result = pg_get_line(termin, sigint_ctx);
/* If we failed to read anything, just return an empty string */
if (result == NULL)
diff --git a/src/include/common/string.h b/src/include/common/string.h
index 686c158efe..238c3dc895 100644
--- a/src/include/common/string.h
+++ b/src/include/common/string.h
@@ -12,6 +12,12 @@
struct StringInfoData; /* avoid including stringinfo.h here */
+typedef struct SigintInterruptContext {
+ void *jmpbuf; /* existing longjmp buffer (really a sigjmp_buf *) */
+ volatile bool *enabled; /* used to enable/disable SIGINT handling */
+ bool canceled; /* indicates whether cancellation occurred */
+} SigintInterruptContext;
+
/* functions in src/common/string.c */
extern bool pg_str_endswith(const char *str, const char *end);
extern int strtoint(const char *pg_restrict str, char **pg_restrict endptr,
@@ -21,11 +27,14 @@ extern int pg_strip_crlf(char *str);
extern bool pg_is_ascii(const char *str);
/* functions in src/common/pg_get_line.c */
-extern char *pg_get_line(FILE *stream);
+extern char *pg_get_line(FILE *stream, SigintInterruptContext *sigint_ctx);
extern bool pg_get_line_buf(FILE *stream, struct StringInfoData *buf);
-extern bool pg_get_line_append(FILE *stream, struct StringInfoData *buf);
+extern bool pg_get_line_append(FILE *stream, struct StringInfoData *buf,
+ SigintInterruptContext *sigint_ctx);
/* functions in src/common/sprompt.c */
extern char *simple_prompt(const char *prompt, bool echo);
+extern char *simple_prompt_extended(const char *prompt, bool echo,
+ SigintInterruptContext *sigint_ctx);
#endif /* COMMON_STRING_H */
--
2.16.6
"Bossart, Nathan" <bossartn@amazon.com> writes:
You are right. I'm not sure what I was thinking. Attached a v3
with that part removed.
Pushed with a little further tweaking --- mostly, I felt that
explicitly referring to SIGINT in the API names was too
implementation-specific, so I renamed things.
As you mentioned, there are several other simple_prompt() calls
that could usefully be improved. (I think the one in startup.c
may be OK because we've not set up the SIGINT handler yet,
though.) I wondered whether it's worth refactoring some more
to have just one function that sets up the context mechanism.
I was also of two minds about whether to add a context option
to pg_get_line_buf(). I stuck with your choice not to, but
it does look a bit inconsistent.
regards, tom lane
On 11/17/21, 4:15 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
Pushed with a little further tweaking --- mostly, I felt that
explicitly referring to SIGINT in the API names was too
implementation-specific, so I renamed things.
Thanks!
As you mentioned, there are several other simple_prompt() calls
that could usefully be improved. (I think the one in startup.c
may be OK because we've not set up the SIGINT handler yet,
though.) I wondered whether it's worth refactoring some more
to have just one function that sets up the context mechanism.
I'll get started on these.
I was also of two minds about whether to add a context option
to pg_get_line_buf(). I stuck with your choice not to, but
it does look a bit inconsistent.
Yeah, I figured it'd be simple enough to add that if/when it's needed.
Nathan
On 11/17/21, 4:38 PM, "Bossart, Nathan" <bossartn@amazon.com> wrote:
On 11/17/21, 4:15 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
As you mentioned, there are several other simple_prompt() calls
that could usefully be improved. (I think the one in startup.c
may be OK because we've not set up the SIGINT handler yet,
though.) I wondered whether it's worth refactoring some more
to have just one function that sets up the context mechanism.I'll get started on these.
Okay, here is an attempt at adding control-C support for \prompt and
\connect. It was reasonably straightforward. I did have to teach
simple_prompt_extended() to add a newline after a cancellation when
"echo" is true.
I think that's it for psql. After a quick glance, I didn't see any
other obvious candidates for control-C support, but I'll look a little
closer to be sure.
Nathan
Attachments:
0001-Add-control-C-support-for-psql-s-prompt-and-connect-.patchapplication/octet-stream; name=0001-Add-control-C-support-for-psql-s-prompt-and-connect-.patchDownload
From a7657d5fc232d9e16a9714751977df58bea6c11b Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Fri, 19 Nov 2021 00:36:57 +0000
Subject: [PATCH 1/1] Add control-C support for psql's \prompt and \connect
meta-commands.
---
src/bin/psql/command.c | 42 +++++++++++++++++++++++++++++++++---------
src/common/sprompt.c | 6 ++++++
2 files changed, 39 insertions(+), 9 deletions(-)
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 102bc5956b..941be1f51e 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -2132,6 +2132,12 @@ exec_command_prompt(PsqlScanState scan_state, bool active_branch,
else
{
char *result;
+ PromptInterruptContext prompt_ctx;
+
+ /* Set up to let SIGINT cancel simple_prompt_extended() */
+ prompt_ctx.jmpbuf = sigint_interrupt_jmp;
+ prompt_ctx.enabled = &sigint_interrupt_enabled;
+ prompt_ctx.canceled = false;
if (arg2)
{
@@ -2143,7 +2149,7 @@ exec_command_prompt(PsqlScanState scan_state, bool active_branch,
if (!pset.inputfile)
{
- result = simple_prompt(prompt_text, true);
+ result = simple_prompt_extended(prompt_text, true, &prompt_ctx);
}
else
{
@@ -2161,8 +2167,8 @@ exec_command_prompt(PsqlScanState scan_state, bool active_branch,
}
}
- if (result &&
- !SetVariable(pset.vars, opt, result))
+ if (prompt_ctx.canceled ||
+ (result && !SetVariable(pset.vars, opt, result)))
success = false;
if (result)
@@ -3059,23 +3065,34 @@ copy_previous_query(PQExpBuffer query_buf, PQExpBuffer previous_buf)
/*
* Ask the user for a password; 'username' is the username the
* password is for, if one has been explicitly specified. Returns a
- * malloc'd string.
+ * malloc'd string. If 'canceled' is provided, it will be set to true
+ * if the prompt is canceled via SIGINT and to false otherwise.
*/
static char *
-prompt_for_password(const char *username)
+prompt_for_password(const char *username, bool *canceled)
{
char *result;
+ PromptInterruptContext prompt_ctx;
+
+ /* Set up to let SIGINT cancel simple_prompt_extended() */
+ prompt_ctx.jmpbuf = sigint_interrupt_jmp;
+ prompt_ctx.enabled = &sigint_interrupt_enabled;
+ prompt_ctx.canceled = false;
if (username == NULL || username[0] == '\0')
- result = simple_prompt("Password: ", false);
+ result = simple_prompt_extended("Password: ", false, &prompt_ctx);
else
{
char *prompt_text;
prompt_text = psprintf(_("Password for user %s: "), username);
- result = simple_prompt(prompt_text, false);
+ result = simple_prompt_extended(prompt_text, false, &prompt_ctx);
free(prompt_text);
}
+
+ if (canceled)
+ *canceled = prompt_ctx.canceled;
+
return result;
}
@@ -3331,6 +3348,8 @@ do_connect(enum trivalue reuse_previous_specification,
*/
if (pset.getPassword == TRI_YES && success)
{
+ bool canceled = false;
+
/*
* If a connstring or URI is provided, we don't know which username
* will be used, since we haven't dug that out of the connstring.
@@ -3338,7 +3357,9 @@ do_connect(enum trivalue reuse_previous_specification,
* not seem worth working harder, since this getPassword setting is
* normally only used in noninteractive cases.
*/
- password = prompt_for_password(has_connection_string ? NULL : user);
+ password = prompt_for_password(has_connection_string ? NULL : user,
+ &canceled);
+ success = !canceled;
}
/*
@@ -3417,13 +3438,16 @@ do_connect(enum trivalue reuse_previous_specification,
*/
if (!password && PQconnectionNeedsPassword(n_conn) && pset.getPassword != TRI_NO)
{
+ bool canceled = false;
+
/*
* Prompt for password using the username we actually connected
* with --- it might've come out of "dbname" rather than "user".
*/
- password = prompt_for_password(PQuser(n_conn));
+ password = prompt_for_password(PQuser(n_conn), &canceled);
PQfinish(n_conn);
n_conn = NULL;
+ success = !canceled;
continue;
}
diff --git a/src/common/sprompt.c b/src/common/sprompt.c
index 917676b58c..8ffed303f7 100644
--- a/src/common/sprompt.c
+++ b/src/common/sprompt.c
@@ -164,6 +164,12 @@ simple_prompt_extended(const char *prompt, bool echo,
fflush(termout);
#endif
}
+ else if (prompt_ctx && prompt_ctx->canceled)
+ {
+ /* echo \n if prompt was canceled */
+ fputs("\n", termout);
+ fflush(termout);
+ }
if (termin != stdin)
{
--
2.16.6
"Bossart, Nathan" <bossartn@amazon.com> writes:
Okay, here is an attempt at adding control-C support for \prompt and
\connect. It was reasonably straightforward. I did have to teach
simple_prompt_extended() to add a newline after a cancellation when
"echo" is true.
LGTM, pushed after very minor fooling with the comments.
I think that's it for psql. After a quick glance, I didn't see any
other obvious candidates for control-C support, but I'll look a little
closer to be sure.
Hmm, initdb's prompt-for-superuser-password might need it.
I think the rest of our frontend programs don't trap SIGINT,
or at least don't do so while requesting user input.
regards, tom lane
On 11/19/21, 9:17 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
"Bossart, Nathan" <bossartn@amazon.com> writes:
Okay, here is an attempt at adding control-C support for \prompt and
\connect. It was reasonably straightforward. I did have to teach
simple_prompt_extended() to add a newline after a cancellation when
"echo" is true.LGTM, pushed after very minor fooling with the comments.
Thanks!
I think that's it for psql. After a quick glance, I didn't see any
other obvious candidates for control-C support, but I'll look a little
closer to be sure.Hmm, initdb's prompt-for-superuser-password might need it.
I think the rest of our frontend programs don't trap SIGINT,
or at least don't do so while requesting user input.
I'm able to cancel the superuser password prompt in initdb already.
It looks like the signal handlers aren't set up until after
get_su_pwd(). I did find some missing control-C handling in
pg_receivewal/pg_recvlogical, though. Attached is a patch for those.
Nathan
Attachments:
v1-0001-Add-control-C-handling-for-password-prompts-in-pg.patchapplication/octet-stream; name=v1-0001-Add-control-C-handling-for-password-prompts-in-pg.patchDownload
From edd0839967129f7ac8965ebccd9bfe18a7c8967a Mon Sep 17 00:00:00 2001
From: Nathan Bossart <bossartn@amazon.com>
Date: Fri, 19 Nov 2021 22:56:13 +0000
Subject: [PATCH v1 1/1] Add control-C handling for password prompts in
pg_receivewal and pg_recvlogical.
---
src/bin/pg_basebackup/nls.mk | 2 +-
src/bin/pg_basebackup/pg_receivewal.c | 11 ++++++++++-
src/bin/pg_basebackup/pg_recvlogical.c | 11 ++++++++++-
src/bin/pg_basebackup/streamutil.c | 36 ++++++++++++++++++++++++++++------
src/bin/pg_basebackup/streamutil.h | 5 +++++
5 files changed, 56 insertions(+), 9 deletions(-)
diff --git a/src/bin/pg_basebackup/nls.mk b/src/bin/pg_basebackup/nls.mk
index 2d521f0683..338048583c 100644
--- a/src/bin/pg_basebackup/nls.mk
+++ b/src/bin/pg_basebackup/nls.mk
@@ -2,5 +2,5 @@
CATALOG_NAME = pg_basebackup
AVAIL_LANGUAGES = cs de es fr he it ja ko pl pt_BR ru sv tr uk vi zh_CN
GETTEXT_FILES = $(FRONTEND_COMMON_GETTEXT_FILES) pg_basebackup.c pg_receivewal.c pg_recvlogical.c receivelog.c streamutil.c walmethods.c ../../common/fe_memutils.c ../../common/file_utils.c ../../fe_utils/recovery_gen.c
-GETTEXT_TRIGGERS = $(FRONTEND_COMMON_GETTEXT_TRIGGERS) simple_prompt tar_set_error
+GETTEXT_TRIGGERS = $(FRONTEND_COMMON_GETTEXT_TRIGGERS) simple_prompt simple_prompt_extended tar_set_error
GETTEXT_FLAGS = $(FRONTEND_COMMON_GETTEXT_FLAGS)
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 23cf5f8ec7..ecc068fd9b 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -649,7 +649,9 @@ StreamLog(void)
/*
* When sigint is called, just tell the system to exit at the next possible
- * moment.
+ * moment. Since this won't work when blocked on user input (e.g., fgets()),
+ * we also provide a mechanism to longjmp out of user prompts. While blocked,
+ * sigint_interrupt_enabled should be set to true.
*/
#ifndef WIN32
@@ -657,6 +659,13 @@ static void
sigint_handler(int signum)
{
time_to_stop = true;
+
+ /* if we are waiting for input, longjmp out of it */
+ if (sigint_interrupt_enabled)
+ {
+ sigint_interrupt_enabled = false;
+ siglongjmp(sigint_interrupt_jmp, 1);
+ }
}
#endif
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index f235d6fecf..8c23abf1f4 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -657,12 +657,21 @@ error:
/*
* When sigint is called, just tell the system to exit at the next possible
- * moment.
+ * moment. Since this won't work when blocked on user input (e.g., fgets()),
+ * we also provide a mechanism to longjmp out of user prompts. While blocked,
+ * sigint_interrupt_enabled should be set to true.
*/
static void
sigint_handler(int signum)
{
time_to_abort = true;
+
+ /* if we are waiting for input, longjmp out of it */
+ if (sigint_interrupt_enabled)
+ {
+ sigint_interrupt_enabled = false;
+ siglongjmp(sigint_interrupt_jmp, 1);
+ }
}
/*
diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c
index 2a3e0c688f..c0adb256b0 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -52,16 +52,21 @@ char *dbname = NULL;
int dbgetpassword = 0; /* 0=auto, -1=never, 1=always */
static char *password = NULL;
PGconn *conn = NULL;
+volatile bool sigint_interrupt_enabled = false;
+sigjmp_buf sigint_interrupt_jmp;
/*
* Connect to the server. Returns a valid PGconn pointer if connected,
* or NULL on non-permanent error. On permanent error, the function will
- * call exit(1) directly.
+ * call exit(1) directly. The password prompt can be canceled via an
+ * existing SIGINT handler that will longjmp to the place specified by
+ * sigint_interrupt_jmp only when sigint_interrupt_enabled is true. When
+ * canceled, this function returns NULL.
*/
PGconn *
GetConnection(void)
{
- PGconn *tmpconn;
+ PGconn *tmpconn = NULL;
int argcount = 7; /* dbname, replication, fallback_app_name,
* host, user, port, password */
int i;
@@ -157,10 +162,20 @@ GetConnection(void)
/* Get a new password if appropriate */
if (need_password)
{
+ PromptInterruptContext prompt_ctx;
+
+ /* Set up to let SIGINT cancel simple_prompt_extended() */
+ prompt_ctx.jmpbuf = sigint_interrupt_jmp;
+ prompt_ctx.enabled = &sigint_interrupt_enabled;
+ prompt_ctx.canceled = false;
+
if (password)
free(password);
- password = simple_prompt("Password: ", false);
+ password = simple_prompt_extended("Password: ", false, &prompt_ctx);
need_password = false;
+
+ if (prompt_ctx.canceled)
+ break;
}
/* Use (or reuse, on a subsequent connection) password if we have it */
@@ -193,15 +208,24 @@ GetConnection(void)
dbgetpassword != -1)
{
PQfinish(tmpconn);
+ tmpconn = NULL;
need_password = true;
}
}
while (need_password);
- if (PQstatus(tmpconn) != CONNECTION_OK)
+ if (tmpconn == NULL || PQstatus(tmpconn) != CONNECTION_OK)
{
- pg_log_error("%s", PQerrorMessage(tmpconn));
- PQfinish(tmpconn);
+ /*
+ * If tmpconn is NULL, the password prompt was canceled via SIGINT,
+ * and we don't bother logging an error message.
+ */
+ if (tmpconn != NULL)
+ {
+ pg_log_error("%s", PQerrorMessage(tmpconn));
+ PQfinish(tmpconn);
+ }
+
free(values);
free(keywords);
if (conn_opts)
diff --git a/src/bin/pg_basebackup/streamutil.h b/src/bin/pg_basebackup/streamutil.h
index 7918935cb3..2c45fdace4 100644
--- a/src/bin/pg_basebackup/streamutil.h
+++ b/src/bin/pg_basebackup/streamutil.h
@@ -12,6 +12,8 @@
#ifndef STREAMUTIL_H
#define STREAMUTIL_H
+#include <setjmp.h>
+
#include "access/xlogdefs.h"
#include "datatype/timestamp.h"
#include "libpq-fe.h"
@@ -65,4 +67,7 @@ extern bool feTimestampDifferenceExceeds(TimestampTz start_time, TimestampTz sto
extern void fe_sendint64(int64 i, char *buf);
extern int64 fe_recvint64(char *buf);
+extern volatile bool sigint_interrupt_enabled;
+extern sigjmp_buf sigint_interrupt_jmp;
+
#endif /* STREAMUTIL_H */
--
2.16.6
"Bossart, Nathan" <bossartn@amazon.com> writes:
On 11/19/21, 9:17 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
Hmm, initdb's prompt-for-superuser-password might need it.
I'm able to cancel the superuser password prompt in initdb already.
It looks like the signal handlers aren't set up until after
get_su_pwd().
Right; I misread that code in an overly hasty scan.
I did find some missing control-C handling in
pg_receivewal/pg_recvlogical, though. Attached is a patch for those.
Meh ... I'm inclined to fix those programs by just moving their pqsignal
calls down to after their initial GetConnection calls, as attached.
This'd be simple enough to back-patch, for one thing.
It could be argued that this doesn't provide a nice experience if
(a) somebody changes your password mid-run and (b) you actually
need to make a new connection for some reason and (c) you want
to give up at that point instead of putting in the new password.
But I doubt it's worth so much extra complication to address that
edge case. We've had about zero field complaints about the existing
behavior in those programs, so the cost/benefit ratio seems poor.
regards, tom lane
PS: I noticed that StreamLogicalLog leaks its query buffer if
GetConnection fails. Probably not very exciting, but we might
as well fix that, as included below.
Attachments:
delay-setting-up-signal-handlers.patchtext/x-diff; charset=us-ascii; name=delay-setting-up-signal-handlers.patchDownload
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 23cf5f8ec7..4b1439be90 100644
--- a/src/bin/pg_basebackup/pg_receivewal.c
+++ b/src/bin/pg_basebackup/pg_receivewal.c
@@ -917,10 +917,6 @@ main(int argc, char **argv)
close_destination_dir(dir, basedir);
}
-#ifndef WIN32
- pqsignal(SIGINT, sigint_handler);
-#endif
-
/*
* Obtain a connection before doing anything.
*/
@@ -930,6 +926,14 @@ main(int argc, char **argv)
exit(1);
atexit(disconnect_atexit);
+ /*
+ * Trap signals. (Don't do this until after the initial password prompt,
+ * if one is needed, in GetConnection.)
+ */
+#ifndef WIN32
+ pqsignal(SIGINT, sigint_handler);
+#endif
+
/*
* Run IDENTIFY_SYSTEM to make sure we've successfully have established a
* replication connection and haven't connected using a database specific
diff --git a/src/bin/pg_basebackup/pg_recvlogical.c b/src/bin/pg_basebackup/pg_recvlogical.c
index f235d6fecf..13319cf0d3 100644
--- a/src/bin/pg_basebackup/pg_recvlogical.c
+++ b/src/bin/pg_basebackup/pg_recvlogical.c
@@ -216,8 +216,6 @@ StreamLogicalLog(void)
output_written_lsn = InvalidXLogRecPtr;
output_fsync_lsn = InvalidXLogRecPtr;
- query = createPQExpBuffer();
-
/*
* Connect in replication mode to the server
*/
@@ -236,6 +234,7 @@ StreamLogicalLog(void)
replication_slot);
/* Initiate the replication stream at specified location */
+ query = createPQExpBuffer();
appendPQExpBuffer(query, "START_REPLICATION SLOT \"%s\" LOGICAL %X/%X",
replication_slot, LSN_FORMAT_ARGS(startpos));
@@ -932,16 +931,9 @@ main(int argc, char **argv)
exit(1);
}
-
-#ifndef WIN32
- pqsignal(SIGINT, sigint_handler);
- pqsignal(SIGHUP, sighup_handler);
-#endif
-
/*
- * Obtain a connection to server. This is not really necessary but it
- * helps to get more precise error messages about authentication, required
- * GUC parameters and such.
+ * Obtain a connection to server. Notably, if we need a password, we want
+ * to collect it from the user immediately.
*/
conn = GetConnection();
if (!conn)
@@ -949,6 +941,15 @@ main(int argc, char **argv)
exit(1);
atexit(disconnect_atexit);
+ /*
+ * Trap signals. (Don't do this until after the initial password prompt,
+ * if one is needed, in GetConnection.)
+ */
+#ifndef WIN32
+ pqsignal(SIGINT, sigint_handler);
+ pqsignal(SIGHUP, sighup_handler);
+#endif
+
/*
* Run IDENTIFY_SYSTEM to make sure we connected using a database specific
* replication connection.
On 11/20/21, 1:58 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
"Bossart, Nathan" <bossartn@amazon.com> writes:
I did find some missing control-C handling in
pg_receivewal/pg_recvlogical, though. Attached is a patch for those.Meh ... I'm inclined to fix those programs by just moving their pqsignal
calls down to after their initial GetConnection calls, as attached.
This'd be simple enough to back-patch, for one thing.It could be argued that this doesn't provide a nice experience if
(a) somebody changes your password mid-run and (b) you actually
need to make a new connection for some reason and (c) you want
to give up at that point instead of putting in the new password.
But I doubt it's worth so much extra complication to address that
edge case. We've had about zero field complaints about the existing
behavior in those programs, so the cost/benefit ratio seems poor.
Yeah, I was looking for a way to simplify this, too. Your approach
seems reasonable enough to me.
Nathan
"Bossart, Nathan" <bossartn@amazon.com> writes:
On 11/20/21, 1:58 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
Meh ... I'm inclined to fix those programs by just moving their pqsignal
calls down to after their initial GetConnection calls, as attached.
This'd be simple enough to back-patch, for one thing.
Yeah, I was looking for a way to simplify this, too. Your approach
seems reasonable enough to me.
Hearing no contrary opinions, pushed that way.
regards, tom lane
On 11/21/21, 11:15 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
"Bossart, Nathan" <bossartn@amazon.com> writes:
Yeah, I was looking for a way to simplify this, too. Your approach
seems reasonable enough to me.Hearing no contrary opinions, pushed that way.
Thanks!
Nathan