Content-Type: multipart/alternative;
 boundary="------------00A7Q47w0GRtTfWpyJEpMMx7"
Message-ID: <0fe740dd-cd23-45a6-b116-c777349acbf1@gmail.com>
Date: Wed, 4 Sep 2024 00:28:23 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: optimizing pg_upgrade's once-in-each-database steps
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Corey Huinker <corey.huinker@gmail.com>,
 Daniel Gustafsson <daniel@yesql.se>, Robert Haas <robertmhaas@gmail.com>,
 Jeff Davis <pgsql@j-davis.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
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>
Content-Language: en-US
From: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
In-Reply-To: <ZtTXEexwmlvsgsPs@nathan>
Archived-At: 
 <https://www.postgresql.org/message-id/0fe740dd-cd23-45a6-b116-c777349acbf1%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------00A7Q47w0GRtTfWpyJEpMMx7
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


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:

diff --git a/src/bin/pg_upgrade/task.c b/src/bin/pg_upgrade/task.c
index 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.

--------------00A7Q47w0GRtTfWpyJEpMMx7
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 8bit

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p><br>
    </p>
    <div class="moz-cite-prefix">On 01.09.2024 22:05, Nathan Bossart
      wrote:<span style="white-space: pre-wrap">
</span></div>
    <blockquote type="cite" cite="mid:ZtTXEexwmlvsgsPs@nathan">
      <pre wrap="" class="moz-quote-pre">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.</pre>
    </blockquote>
    Good catch, I didn't look so closely at this.<span
    style="white-space: pre-wrap">
</span><span style="white-space: pre-wrap">
</span>
    <blockquote type="cite" cite="mid:ZtTXEexwmlvsgsPs@nathan">
      <pre wrap="" class="moz-quote-pre">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.
</pre>
    </blockquote>
    The fix looks right to me, but I got confused by the skip_wait and
    this `if`:<br>
    <br>
    +            if (PQstatus(slot-&gt;conn) != CONNECTION_OK)<br>
    +                return;<br>
    <p>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:</p>
    <p>diff --git a/src/bin/pg_upgrade/task.c
      b/src/bin/pg_upgrade/task.c<br>
      index 3618dc08ff..73e987febf 100644<br>
      --- a/src/bin/pg_upgrade/task.c<br>
      +++ b/src/bin/pg_upgrade/task.c<br>
      @@ -237,6 +237,8 @@ process_query_result(const ClusterInfo
      *cluster, UpgradeTaskSlot *slot,<br>
       static void<br>
       process_slot(const ClusterInfo *cluster, UpgradeTaskSlot *slot,
      const UpgradeTask *task)<br>
       {<br>
      +    PostgresPollingStatusType status;<br>
      +<br>
           if (!slot-&gt;ready)<br>
               return;<br>
       <br>
      @@ -260,26 +262,26 @@ process_slot(const ClusterInfo *cluster,
      UpgradeTaskSlot *slot, const UpgradeTas<br>
                   start_conn(cluster, slot);<br>
       <br>
                   return;<br>
      -<br>
               case CONNECTING:<br>
       <br>
      -            /* Check for connection failure. */<br>
      -            if (PQstatus(slot-&gt;conn) == CONNECTION_BAD)<br>
      -                pg_fatal("connection failure: %s",
      PQerrorMessage(slot-&gt;conn));<br>
      -<br>
      -            /* Check whether the connection is still
      establishing. */<br>
      -            if (PQstatus(slot-&gt;conn) != CONNECTION_OK)<br>
      -                return;<br>
      -<br>
      -            /*<br>
      -             * Move on to running/processing the queries in the
      task.<br>
      -             */<br>
      -            slot-&gt;state = RUNNING_QUERIES;<br>
      -            if (!PQsendQuery(slot-&gt;conn,
      task-&gt;queries-&gt;data))<br>
      +            status = PQconnectPoll(slot-&gt;conn);<br>
      +            if (status == PGRES_POLLING_READING)<br>
      +                slot-&gt;select_mode = true;<br>
      +            else if (status == PGRES_POLLING_WRITING)<br>
      +                slot-&gt;select_mode = false;<br>
      +            else if (status == PGRES_POLLING_FAILED)<br>
                       pg_fatal("connection failure: %s",
      PQerrorMessage(slot-&gt;conn));<br>
      +            else<br>
      +            {<br>
      +                /*<br>
      +                 * Move on to running/processing the queries in
      the task.<br>
      +                 */<br>
      +                slot-&gt;state = RUNNING_QUERIES;<br>
      +                if (!PQsendQuery(slot-&gt;conn,
      task-&gt;queries-&gt;data))<br>
      +                    pg_fatal("connection failure: %s",
      PQerrorMessage(slot-&gt;conn));<br>
       <br>
      +            }<br>
                   return;<br>
      -<br>
               case RUNNING_QUERIES:<br>
       <br>
                   /*<br>
      @@ -370,8 +372,6 @@ wait_on_slots(UpgradeTaskSlot *slots, int
      numslots)<br>
       <br>
           for (int i = 0; i &lt; numslots; i++)<br>
           {<br>
      -        PostgresPollingStatusType status;<br>
      -<br>
               switch (slots[i].state)<br>
               {<br>
                   case FREE:<br>
      @@ -386,33 +386,7 @@ wait_on_slots(UpgradeTaskSlot *slots, int
      numslots)<br>
                       continue;<br>
       <br>
                   case CONNECTING:<br>
      -<br>
      -                /*<br>
      -                 * Don't call PQconnectPoll() again for this slot
      until<br>
      -                 * select() tells us something is ready.  Be sure
      to use the<br>
      -                 * previous poll mode in this case.<br>
      -                 */<br>
      -                if (!slots[i].ready)<br>
      -                    break;<br>
      -<br>
      -                /*<br>
      -                 * If we are waiting for the connection to
      establish, choose<br>
      -                 * whether to wait for reading or for writing on
      the socket as<br>
      -                 * appropriate.  If neither apply, mark the slot
      as ready and<br>
      -                 * skip waiting so that it is handled ASAP (we
      assume this<br>
      -                 * means the connection is either bad or fully
      ready).<br>
      -                 */<br>
      -                status = PQconnectPoll(slots[i].conn);<br>
      -                if (status == PGRES_POLLING_READING)<br>
      -                    slots[i].select_mode = true;<br>
      -                else if (status == PGRES_POLLING_WRITING)<br>
      -                    slots[i].select_mode = false;<br>
      -                else<br>
      -                {<br>
      -                    slots[i].ready = true;<br>
      -                    skip_wait = true;<br>
      -                    continue;<br>
      -                }<br>
      +                /* All the slot metadata was already setup in
      process_slots() */<br>
       <br>
                       break;<br>
       <br>
      -- <br>
      2.43.0<br>
      <br>
      skip_wait can be removed in this case as well. <br>
    </p>
    <p>This is up to you, I think the v12 is good and commitable in any
      case.<br>
    </p>
  </body>
</html>

--------------00A7Q47w0GRtTfWpyJEpMMx7--