Message-ID: <60cbc507-4c36-4037-9af7-028b1c8455b5@gmail.com>
Date: Sun, 4 Aug 2024 19:19:57 +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: Daniel Gustafsson <daniel@yesql.se>, Robert Haas <robertmhaas@gmail.com>,
 Jeff Davis <pgsql@j-davis.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
References: <ZoMK9ZhOsWEKqxlM@nathan> <ZowEIGd0lLKh16qe@nathan>
 <ZoyveaS6a6jyhYS6@nathan> <26742670-9539-4F30-B6FA-9C1DB743BC23@yesql.se>
 <Zpg4WnS9BRWbgBLi@nathan> <4493D323-9726-48F7-A702-E71FB991F76B@yesql.se>
 <ZprY4e23vR95Nc51@nathan> <Zp677slGLwhPhVWy@nathan>
 <10c1c8dd-4685-46d4-80be-56bdfca8659a@gmail.com> <ZqvJg_lTTTt82BfQ@nathan>
 <ZqwA_qngQM25FrjK@nathan>
Content-Language: en-US
From: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
In-Reply-To: <ZqwA_qngQM25FrjK@nathan>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/60cbc507-4c36-4037-9af7-028b1c8455b5%40gmail.com>
Precedence: bulk


On 01.08.2024 22:41, Nathan Bossart wrote:
> Here is a new patch set.  Besides rebasing, I've added the recursive call
> to process_slot() mentioned in the quoted text, and I've added quite a bit
> of commentary to async.c.
That's much better now, thanks! Here's my code review, note that I 
haven't tested the patches yet:

+void
+async_task_add_step(AsyncTask *task,
+                    AsyncTaskGetQueryCB query_cb,
+                    AsyncTaskProcessCB process_cb, bool free_result,
+                    void *arg)

Is there any reason to have query as a callback function instead of char 
*? From what I see right now, it doesn't give any extra flexibility, as 
the query has to be static anyway (can't be customized on a per-database 
basis) and will be created once before all the callbacks are run. While 
passing in char * makes the API simpler, excludes any potential error of 
making the query dependent on the current database and removes the 
unnecessary malloc/free of the static strings.

+static void
+dispatch_query(const ClusterInfo *cluster, AsyncSlot *slot,
+               const AsyncTask *task)
+{
+    ...
+    if (!PQsendQuery(slot->conn, cbs->query))
+        conn_failure(slot->conn);
+}

This will print "connection failure: connection pointer is NULL", which 
I don't think makes a lot of sense to the end user. I'd prefer something 
like pg_fatal("failed to allocate a new connection").

      if (found)
-        pg_fatal("Data type checks failed: %s", report.data);
+    {
+        pg_fatal("Data type checks failed: %s", 
data_type_check_report.data);
+        termPQExpBuffer(&data_type_check_report);
+    }

`found` should be removed and replaced with `data_type_check_failed`, as 
it's not set anymore. Also the termPQExpBuffer after pg_fatal looks 
unnecessary.

+static bool *data_type_check_results;
+static bool data_type_check_failed;
+static PQExpBufferData data_type_check_report;

IMO, it would be nicer to have these as a local state, that's passed in 
as an arg* to the AsyncTaskProcessCB, which aligns with how the other 
checks do it.

-- End of review --

Regarding keeping the connections, the way I envisioned it entailed 
passing a list of connections from one check to the next one (or keeping 
a global state with connections?). I didn't concretely look at the code 
to verify this, so it's just an abstract idea.