Re: [PoC] pg_upgrade: allow to upgrade publisher node

On Thu, Aug 17, 2023 at 2:10 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for the first 2 patches.
>
>
> 3.
>
> +    <para>
> +     Before you start upgrading the publisher node, ensure that the
> +     subscription is temporarily disabled. After the upgrade is complete,
> +     execute the
> +     <link linkend="sql-altersubscription"><command>ALTER
> SUBSCRIPTION ... DISABLE</command></link>
> +     command to update the connection string, and then re-enable the
> +     subscription.
> +    </para>
>
> 3a.
> That link made no sense in this context.
>
> Don't you mean to say:
> <command>ALTER SUBSCRIPTION ... CONNECTION ...</command>
>

I think the command is correct here but the wording should mention
about disabling the subscription.

>
>   /*
> - * Fetch all libraries containing non-built-in C functions in this DB.
> + * Fetch all libraries containing non-built-in C functions and
> + * output plugins in this DB.
>   */
>   ress[dbnum] = executeQueryOrDie(conn,
>   "SELECT DISTINCT probin "
>   "FROM pg_catalog.pg_proc "
>   "WHERE prolang = %u AND "
>   "probin IS NOT NULL AND "
> - "oid >= %u;",
> + "oid >= %u "
> + "UNION "
> + "SELECT DISTINCT plugin "
> + "FROM pg_catalog.pg_replication_slots "
> + "WHERE wal_status <> 'lost' AND "
> + "database = current_database() AND "
> + "temporary IS FALSE;",
>   ClanguageId,
>   FirstNormalObjectId);
>   totaltups += PQntuples(ress[dbnum]);
>
> ~
>
> Maybe it is OK, but it somehow seems like the new logic has been
> jammed into the get_loadable_libraries() function for coding
> convenience. For example, all the names (function names, variable
> names, structure field names) are referring to "libraries", so the
> plugin seems a bit out of place.
>

But the same name library (as plugin) should exist for the upgrade of
slots. I feel doing it separately could either lead to a redundant
code or a different way to achieve the same thing. Do you envision any
problem which we are not seeing?

> ~~~
> 10. get_loadable_libraries
>
> + "UNION "
> + "SELECT DISTINCT plugin "
> + "FROM pg_catalog.pg_replication_slots "
> + "WHERE wal_status <> 'lost' AND "
> + "database = current_database() AND "
> + "temporary IS FALSE;",
>
> IMO this SQL might be more readable if it uses an alias (like 'rs')
> for the catalog. Then rs.wal_status, rs.database, rs.temporary etc.
>

Then it will become inconsistent with the existing query which doesn't
use any alias. So, I think we should either change the existing query
to use an alias or not use it at all as the patch does. I would prefer
later.

>
> 16. create_logical_replication_slots
>
> + appendPQExpBuffer(query, "SELECT
> pg_catalog.pg_create_logical_replication_slot(");
> + appendStringLiteral(query, slot_arr->slots[slotnum].slotname,
> + slot_arr->encoding, slot_arr->std_strings);
> + appendPQExpBuffer(query, ", ");
> + appendStringLiteral(query, slot_arr->slots[slotnum].plugin,
> + slot_arr->encoding, slot_arr->std_strings);
> + appendPQExpBuffer(query, ", false, %s);",
> +   slot_arr->slots[slotnum].two_phase ? "true" : "false");
>
> I noticed that the function comment for appendStringLiteral() says:
> "We need it in situations where we do not have a PGconn available.
> Where we do, appendStringLiteralConn is a better choice.".
>
> But in this code, we *do* have PGconn available. So, shouldn't we be
> following the advice of the appendStringLiteral() function comment and
> use the other API instead?
>

I think that will avoid maintaining encoding and std_strings in the
slot's array. So, this sounds like a good idea to me.

-- 
With Regards,
Amit Kapila.