From: Tom Lane <tgl@sss.pgh.pa.us>
To: Bossart, Nathan <bossartn@amazon.com>
Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Subject: Re: Improving psql's \password command
Date: Sat, 20 Nov 2021 16:57:56 -0500
Message-ID: <3349562.1637445476@sss.pgh.pa.us> (raw)
In-Reply-To: <9D3BFA94-DEF2-4B43-B240-8ADC595ECA7A@amazon.com>
References: <747443.1635536754@sss.pgh.pa.us>
<C2787D93-6871-4D93-9CF3-CD7BB489F47F@amazon.com>
<765509.1635552418@sss.pgh.pa.us>
<7BAC8A28-148B-4B03-A345-D68071388300@amazon.com>
<1553329.1636647111@sss.pgh.pa.us>
<E0C46CDF-46B2-4388-B53B-96EB2CA0F71E@amazon.com>
<1847699.1636747076@sss.pgh.pa.us>
<AEA1BA57-BD21-4237-9859-1C04FE38602B@amazon.com>
<2141902.1636999964@sss.pgh.pa.us>
<5EDA38EF-C411-4C7D-A9BC-10B1D4584E5E@amazon.com>
<2152988.1637010233@sss.pgh.pa.us>
<E4840C66-2FD5-4A0B-8CC9-A09F8B9126B7@amazon.com>
<2850852.1637194468@sss.pgh.pa.us>
<450C0AD1-CF21-4056-A9EE-6201E9763FC9@amazon.com>
<3085404.1637342157@sss.pgh.pa.us>
<9D3BFA94-DEF2-4B43-B240-8ADC595ECA7A@amazon.com>
"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.patch (text/x-diff, 2.5K) downloaddiff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.cindex 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.cindex 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.
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, bossartn@amazon.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Improving psql's \password command
In-Reply-To: <3349562.1637445476@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox