From: Nathan Bossart <nathandbossart@gmail.com>
To: 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, 17 Jul 2024 16:32:10 -0500
Message-ID: <Zpg4WnS9BRWbgBLi@nathan> (raw)
In-Reply-To: <26742670-9539-4F30-B6FA-9C1DB743BC23@yesql.se>
References: <20240516211638.GA1688936@nathanxps13>
<3b97ab8e295ec3db22baa2dfdd49073449ae1696.camel@j-davis.com>
<20240517012408.GA1691322@nathanxps13>
<ZoMHsBasBy8LqkEU@nathan>
<CA+TgmoaBof36-ZuRWtu=+8rnhGTNQFFGNcmVB0x3aQawfc_JxA@mail.gmail.com>
<ZoMK9ZhOsWEKqxlM@nathan>
<ZowEIGd0lLKh16qe@nathan>
<ZoyveaS6a6jyhYS6@nathan>
<26742670-9539-4F30-B6FA-9C1DB743BC23@yesql.se>
On Wed, Jul 17, 2024 at 11:16:59PM +0200, Daniel Gustafsson wrote:
> First reaction after a read-through is that this seems really cool, can't wait
> to see how much v18 pg_upgrade will be over v17. I will do more testing and
> review once back from vacation, below are some comments from reading which is
> all I had time for at this point:
Thanks for taking a look!
> +static void
> +conn_failure(PGconn *conn)
> +{
> + pg_log(PG_REPORT, "%s", PQerrorMessage(conn));
> + printf(_("Failure, exiting\n"));
> + exit(1);
> +}
>
> Any particular reason this isn't using pg_fatal()?
IIRC this was to match the error in connectToServer(). We could probably
move to pg_fatal().
> +static void
> +dispatch_query(const ClusterInfo *cluster, AsyncSlot *slot,
> ....
> + pg_free(query);
> +}
>
> A minor point, perhaps fueled by me not having played around much with this
> patchset. It seems a bit odd that dispatch_query is responsible for freeing
> the query from the get_query callback. I would have expected the output from
> AsyncTaskGetQueryCB to be stored in AsyncTask and released by async_task_free.
I don't see any problem with doing it the way you suggest.
Tangentially related, I waffled a bit on whether to require the query
callbacks to put the result in allocated memory. Some queries are the same
no matter what, and some require customization at runtime. As you can see,
I ended up just requiring allocated memory. That makes the code a tad
simpler, and I doubt the extra work is noticeable.
> +static void
> +sub_process(DbInfo *dbinfo, PGresult *res, void *arg)
> +{
> ....
> + fprintf(state->script, "The table sync state \"%s\" is not allowed for database:\"%s\" subscription:\"%s\" schema:\"%s\" relation:\"%s\"\n",
> + PQgetvalue(res, i, 0),
> + dbinfo->db_name,
> + PQgetvalue(res, i, 1),
>
> With the query being in a separate place in the code from the processing it
> takes a bit of jumping around to resolve the columns in PQgetvalue calls like
> this. Using PQfnumber() calls and descriptive names would make this easier. I
> know this case is copying old behavior, but the function splits make it less
> useful than before.
Good point.
> + char *query = pg_malloc(QUERY_ALLOC);
>
> Should we convert this to a PQExpBuffer?
Seems like a good idea. I think I was trying to change as few lines as
possible for my proof-of-concept. :)
--
nathan
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: 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: <Zpg4WnS9BRWbgBLi@nathan>
* 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