Refactoring of connection with password prompt loop for frontends

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Refactoring of connection with password prompt loop for frontends
Date: 2019-08-22 07:45:58
Message-ID: 20190822074558.GG1683@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi all,

In six places of the code tree (+ one in psql which is a bit
different), we have the following pattern for frontend tools to
connect to a backend with a password prompt, roughly like that:
do
{
[...]
conn = PQconnectdbParams(keywords, values, true);

[...]

if (PQstatus(conn) == CONNECTION_BAD &&
PQconnectionNeedsPassword(conn) &&
!have_password)
{
PQfinish(conn);
simple_prompt("Password: ", password, sizeof(password), false);
have_password = true;
new_pass = true;
}
} while (new_pass);

Attached is a tentative of patch to consolidate this logic across the
tree. The patch is far from being in a committable state, and there
are a couple of gotchas:
- pg_dumpall merges connection string parameters, so it is much harder
to plugin that the others, and I left it out on purpose.
- Some code paths spawn a password prompt before the first connection
attempt. For now I have left this first attempt out of the refactored
logic, but I think that it is possible to consolidate that a bit more
by enforcing a password prompt before doing the first connection
attempt (somewhat related to the XXX portion in the patch). At the
end it would be nice to not have to have a 100-byte-long field for the
password buffer we have here and there. Unfortunately this comes with
its limitations in pg_dump as savedPassword needs to be moved around
and may be reused in the context.
- I don't like the routine name connect_with_password_prompt() I
introduced. Suggestions of better names are welcome :)
- This also brings the point that some of our tools are not able to
handle tri-values for passwords, so we may want to consolidate that as
well.

Among the positive points, this brings a lot of consolidation in terms
of error handling, and this shaves a bit of code:
13 files changed, 190 insertions(+), 280 deletions(-)

This moves the logic into src/fe_utils, which is natural as that's
aimed only for frontends and because this also links to libpq.

Please note that this links a bit with the refactoring of vacuumlo and
oid2name logging I proposed a couple of days back (applying one patch
or the other results in conflicts) because we need to have frontends
initialized for logging in order to be able to log errors in the
refactored routine:
https://www.postgresql.org/message-id/20190820012819.GA8326@paquier.xyz
This one should be merged first IMO, but that's a story for the other
thread.

This compiles, and passes all regression tests so it is possible to
play with it easily, still it needs much more testing and love.

Any thoughts? I am adding that to the next commit fest.

Thanks,
--
Michael

Attachment Content-Type Size
refactor-password-prompt-v1.patch text/x-diff 18.6 KB

Browse pgsql-hackers by date

  From Date Subject
Next Message Heikki Linnakangas 2019-08-22 08:14:39 Comment in ginpostinglist.c doesn't match code
Previous Message Surafel Temesgen 2019-08-22 07:40:30 Re: FETCH FIRST clause PERCENT option