Re: Improving psql's \password command
On 11/21/21, 11:15 AM, "Tom Lane" wrote: > "Bossart, Nathan" 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
Re: Improving psql's \password command
"Bossart, Nathan" writes: > On 11/20/21, 1:58 PM, "Tom Lane" 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
Re: Improving psql's \password command
On 11/20/21, 1:58 PM, "Tom Lane" wrote: > "Bossart, Nathan" 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
Re: Improving psql's \password command
"Bossart, Nathan" writes: > On 11/19/21, 9:17 AM, "Tom Lane" 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.
Re: Improving psql's \password command
"Bossart, Nathan" 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
Re: Improving psql's \password command
On 11/17/21, 4:15 PM, "Tom Lane" 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
Re: Improving psql's \password command
"Bossart, Nathan" 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
Re: Improving psql's \password command
"Bossart, Nathan" writes: > On 11/15/21, 10:13 AM, "Tom Lane" wrote: >> * I don't believe that this bit is necessary, or if it is, >> the comment fails to justify it: >> >> - initStringInfo(); >> + /* 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
Re: Improving psql's \password command
"Bossart, Nathan" 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 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(); + /* 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
Re: Improving psql's \password command
On 11/12/21, 11:58 AM, "Tom Lane" wrote: > "Bossart, Nathan" 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
Re: Improving psql's \password command
"Bossart, Nathan" 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
Re: Improving psql's \password command
"Bossart, Nathan" writes: > On 10/29/21, 5:07 PM, "Tom Lane" 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
Re: Improving psql's \password command
On 10/29/21, 5:07 PM, "Tom Lane" wrote: > "Bossart, Nathan" 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
Re: Improving psql's \password command
"Bossart, Nathan" writes: > On 10/29/21, 12:47 PM, "Tom Lane" 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
Re: Improving psql's \password command
On 10/29/21, 12:47 PM, "Tom Lane" 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
