Re: [PoC] pg_upgrade: allow to upgrade publisher node
От | Amit Kapila |
---|---|
Тема | Re: [PoC] pg_upgrade: allow to upgrade publisher node |
Дата | |
Msg-id | CAA4eK1LhEwxQmK2ZepYTYDOKp6F8JCFbiBcw5EoQFbs-CjmY7Q@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [PoC] pg_upgrade: allow to upgrade publisher node (Peter Smith <smithpb2250@gmail.com>) |
Ответы |
Re: [PoC] pg_upgrade: allow to upgrade publisher node
(Peter Smith <smithpb2250@gmail.com>)
|
Список | pgsql-hackers |
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.
В списке pgsql-hackers по дате отправления: