Обсуждение: Improving psql's \password command
The attached patch responds to the discussion at [1]. 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] https://www.postgresql.org/message-id/flat/B340250F-A0E3-43BF-A1FB-2AE36003F68D%40gmail.com
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
Вложения
"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
Вложения
"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
Вложения
"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
Вложения
"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
Вложения
"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
Вложения
"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.
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