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

On Sat, Aug 31, 2024 at 01:18:10AM +0100, Ilya Gladyshev wrote:
> LGTM in general, but here are some final nitpicks:

Thanks for reviewing.

> + if (maxFd != 0)
> + (void) select(maxFd + 1, &input_mask, &output_mask, &except_mask, NULL);
>
> It´s a good idea to check for the return value of select, in case it
> returns any errors.

Done.

> + dbs_complete++;
> + (void) PQgetResult(slot->conn);
> + PQfinish(slot->conn);
>
> Perhaps it´s rather for me to understand, what does PQgetResult call do
> here?

I believe I was trying to follow the guidance that you should always call
PQgetResult() until it returns NULL, but looking again, I don't see any
need to call it since we free the connection immediately afterwards.

> + /* Check for connection failure. */
> + if (PQconnectPoll(slot->conn) == PGRES_POLLING_FAILED)
> + pg_fatal("connection failure: %s", PQerrorMessage(slot->conn));
> +
> + /* Check whether the connection is still establishing. */
> + if (PQconnectPoll(slot->conn) != PGRES_POLLING_OK)
> + return;
>
> Are the two consecutive calls of PQconnectPoll intended here? Seems a bit
> wasteful, but maybe that´s ok.

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.

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.

> We should probably mention this change in the docs as well, I found these
> two places that I think can be improved:

I've adjusted the documentation in v11.

--
nathan