Horton

Re: optimizing pg_upgrade's once-in-each-database steps

Daniel Gustafsson <daniel@yesql.se>

View thread View on postgr.es
From: Daniel Gustafsson <daniel@yesql.se>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Jeff Davis <pgsql@j-davis.com>, PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Date: 2024-07-17T21:16:59Z
Lists: pgsql-hackers 
> On 9 Jul 2024, at 05:33, Nathan Bossart <nathandbossart@gmail.com> wrote:

> The code is still very rough and nowhere near committable, but this at
> least gets the patch set into the editing phase.

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:

+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()?


+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.


+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.


+   char       *query = pg_malloc(QUERY_ALLOC);

Should we convert this to a PQExpBuffer?

--
Daniel Gustafsson

Commits

  1. Introduce framework for parallelizing various pg_upgrade tasks.

  2. pg_upgrade: Parallelize WITH OIDS check.

  3. pg_upgrade: Parallelize contrib/isn check.

  4. pg_upgrade: Parallelize data type checks.

  5. pg_upgrade: Parallelize encoding conversion check.

  6. pg_upgrade: Parallelize incompatible polymorphics check.

  7. pg_upgrade: Parallelize postfix operator check.

  8. pg_upgrade: Parallelize retrieving extension updates.

  9. pg_upgrade: Parallelize retrieving loadable libraries.

  10. pg_upgrade: Parallelize retrieving relation information.

  11. pg_upgrade: Parallelize subscription check.

  12. pg_upgrade: Move live_check variable to user_opts.

  13. pg_upgrade: run all data type checks per connection