From: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Corey Huinker <corey.huinker@gmail.com>
Cc: Daniel Gustafsson <daniel@yesql.se>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Jeff Davis <pgsql@j-davis.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: optimizing pg_upgrade's once-in-each-database steps
Date: Wed, 4 Sep 2024 00:28:23 +0100
Message-ID: <0fe740dd-cd23-45a6-b116-c777349acbf1@gmail.com> (raw)
In-Reply-To: <ZtTXEexwmlvsgsPs@nathan>
References: <60cbc507-4c36-4037-9af7-028b1c8455b5@gmail.com>
<ZrJ3bu4og_Uj_9qj@nathan>
<CADkLM=e0KxKJH0XSG6x2oGK7-W2Eet4AwwCqz5pxAG7QthX4Xw@mail.gmail.com>
<ZrYrL8-LQ2LhSPQ4@nathan>
<CADkLM=fc-wujxNtGpLSVrBAtWQgsW-8RYrMxVyQ1buJfQX9CHw@mail.gmail.com>
<ZreEh54Xr6D3Qy8k@nathan>
<ZreI0vpZoQfoocDq@nathan>
<Zr4mycERiqJx09y_@nathan>
<Zs-awIHJu_GGahdD@nathan>
<3A3553B1-123D-4F11-9426-277F2E920AD7@gmail.com>
<ZtTXEexwmlvsgsPs@nathan>
On 01.09.2024 22:05, Nathan Bossart wrote:
> I think we can actually just use PQstatus() here. But furthermore, I think
> the way I was initiating connections was completely bogus. IIUC before
> calling PQconnectPoll() the first time, we should wait for a write
> indicator from select(), and then we should only call PQconnectPoll() after
> subsequent indicators from select(). After spending quite a bit of time
> staring at the PQconnectPoll() code, I'm quite surprised I haven't seen any
> problems thus far. If I had to guess, I'd say that either PQconnectPoll()
> is more resilient than I think it is, or I've just gotten lucky because
> pg_upgrade establishes connections quickly.
Good catch, I didn't look so closely at this.
> Anyway, to fix this, I've added some more fields to the slot struct to
> keep track of the information we need to properly establish connections,
> and we now pay careful attention to the return value of select() so that we
> know which slots are ready for processing. This seemed like a nice little
> optimization independent of fixing connection establishment. I was worried
> this was going to require a lot more code, but I think it only added ~50
> lines or so.
The fix looks right to me, but I got confused by the skip_wait and this
`if`:
+ if (PQstatus(slot->conn) != CONNECTION_OK)
+ return;
This branch checks connection status that hasn't been refreshed after
the select. When we go back to wait_slots after this, PQconnectPoll will
refresh the connection status and run select with skip_wait=true, I
believe, we could simplify this by moving the PQconnectPoll back to the
process_slots, so that we can process connection right after polling, if
it's ready. Something like this:
inline patch 1: src/bin/pg_upgrade/task.c (+0/-0 lines) download inline-1.patchdiff --git a/src/bin/pg_upgrade/task.c b/src/bin/pg_upgrade/task.cindex 3618dc08ff..73e987febf 100644--- a/src/bin/pg_upgrade/task.c+++ b/src/bin/pg_upgrade/task.c@@ -237,6 +237,8 @@ process_query_result(const ClusterInfo *cluster, UpgradeTaskSlot *slot,
static void
process_slot(const ClusterInfo *cluster, UpgradeTaskSlot *slot, const
UpgradeTask *task)
{
+ PostgresPollingStatusType status;
+
if (!slot->ready)
return;
@@ -260,26 +262,26 @@ process_slot(const ClusterInfo *cluster,
UpgradeTaskSlot *slot, const UpgradeTas
start_conn(cluster, slot);
return;
-
case CONNECTING:
- /* Check for connection failure. */
- if (PQstatus(slot->conn) == CONNECTION_BAD)
- pg_fatal("connection failure: %s",
PQerrorMessage(slot->conn));
-
- /* Check whether the connection is still establishing. */
- if (PQstatus(slot->conn) != CONNECTION_OK)
- return;
-
- /*
- * Move on to running/processing the queries in the task.
- */
- slot->state = RUNNING_QUERIES;
- if (!PQsendQuery(slot->conn, task->queries->data))
+ status = PQconnectPoll(slot->conn);
+ if (status == PGRES_POLLING_READING)
+ slot->select_mode = true;
+ else if (status == PGRES_POLLING_WRITING)
+ slot->select_mode = false;
+ else if (status == PGRES_POLLING_FAILED)
pg_fatal("connection failure: %s",
PQerrorMessage(slot->conn));
+ else
+ {
+ /*
+ * Move on to running/processing the queries in the task.
+ */
+ slot->state = RUNNING_QUERIES;
+ if (!PQsendQuery(slot->conn, task->queries->data))
+ pg_fatal("connection failure: %s",
PQerrorMessage(slot->conn));
+ }
return;
-
case RUNNING_QUERIES:
/*
@@ -370,8 +372,6 @@ wait_on_slots(UpgradeTaskSlot *slots, int numslots)
for (int i = 0; i < numslots; i++)
{
- PostgresPollingStatusType status;
-
switch (slots[i].state)
{
case FREE:
@@ -386,33 +386,7 @@ wait_on_slots(UpgradeTaskSlot *slots, int numslots)
continue;
case CONNECTING:
-
- /*
- * Don't call PQconnectPoll() again for this slot until
- * select() tells us something is ready. Be sure to
use the
- * previous poll mode in this case.
- */
- if (!slots[i].ready)
- break;
-
- /*
- * If we are waiting for the connection to establish,
choose
- * whether to wait for reading or for writing on the
socket as
- * appropriate. If neither apply, mark the slot as
ready and
- * skip waiting so that it is handled ASAP (we assume this
- * means the connection is either bad or fully ready).
- */
- status = PQconnectPoll(slots[i].conn);
- if (status == PGRES_POLLING_READING)
- slots[i].select_mode = true;
- else if (status == PGRES_POLLING_WRITING)
- slots[i].select_mode = false;
- else
- {
- slots[i].ready = true;
- skip_wait = true;
- continue;
- }
+ /* All the slot metadata was already setup in
process_slots() */
break;
--
2.43.0
skip_wait can be removed in this case as well.
This is up to you, I think the v12 is good and commitable in any case.
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: ilya.v.gladyshev@gmail.com, nathandbossart@gmail.com, corey.huinker@gmail.com, daniel@yesql.se, robertmhaas@gmail.com, pgsql@j-davis.com
Subject: Re: optimizing pg_upgrade's once-in-each-database steps
In-Reply-To: <0fe740dd-cd23-45a6-b116-c777349acbf1@gmail.com>
* 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