Re: Improving psql's \password command - Mailing list pgsql-hackers

"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.