From: Ilya Gladyshev <ilya.v.gladyshev@gmail.com>
To: Nathan Bossart <nathandbossart@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: Sun, 4 Aug 2024 19:19:57 +0100
Message-ID: <60cbc507-4c36-4037-9af7-028b1c8455b5@gmail.com> (raw)
In-Reply-To: <ZqwA_qngQM25FrjK@nathan>
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>
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.
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, daniel@yesql.se, robertmhaas@gmail.com, pgsql@j-davis.com
Subject: Re: optimizing pg_upgrade's once-in-each-database steps
In-Reply-To: <60cbc507-4c36-4037-9af7-028b1c8455b5@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