Re: pg_upgrade and logical replication - Mailing list pgsql-hackers

On Thu, 16 Nov 2023 at 18:25, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Vignesh,
>
> Thanks for updating the patch! Here are some comments.
> They are mainly cosmetic because I have not read yours these days.
>
> 01. binary_upgrade_add_sub_rel_state()
>
> ```
> +    /* We must check these things before dereferencing the arguments */
> +    if (PG_ARGISNULL(0) || PG_ARGISNULL(1) || PG_ARGISNULL(2))
> +        elog(ERROR, "null argument to binary_upgrade_add_sub_rel_state is not allowed")
> ```
>
> But fourth argument can be NULL, right? I know you copied from other functions,
> but they do not accept for all arguments. One approach is that pg_dump explicitly
> writes InvalidXLogRecPtr as the fourth argument.

I did not find any problem with this approach, if the lsn is valid
like in ready state, we will send a valid lsn, if lsn is not valid
like in init state we will pass as NULL. This approach was also
suggested at [1].

> 02. binary_upgrade_add_sub_rel_state()
>
> ```
> +    if (!OidIsValid(relid))
> +        ereport(ERROR,
> +                errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +                errmsg("invalid relation identifier used: %u", relid));
> +
> +    tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
> +    if (!HeapTupleIsValid(tup))
> +        ereport(ERROR,
> +                errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +                errmsg("relation %u does not exist", relid))
> ```
>
> I'm not sure they should be ereport(). Isn't it that they will be never occurred?
> Other upgrade funcs do not have ereport(), and I think it does not have to be
> translated.

I have removed the first check and retained the second one for a sanity check.

> 03. binary_upgrade_replorigin_advance()
>
> IIUC this function is very similar to pg_replication_origin_advance(). Can we
> extract a common part of them? I think pg_replication_origin_advance() will be
> just a wrapper, and binary_upgrade_replorigin_advance() will get the name of
> origin and pass to it.

We will be able to reduce hardly 4 lines, I felt the existing is better.

> 04. binary_upgrade_replorigin_advance()
>
> Even if you do not accept 03, some variable name can be follow the function.

Modified

> 05. getSubscriptions()
>
> ```
> +    appendPQExpBufferStr(query, "o.remote_lsn AS suboriginremotelsn\n")
> ```
>
> Hmm, this value is taken anyway, but will be dumed only when the cluster is PG17+.
> Should we avoid getting the value like subrunasowner and subpasswordrequired?
> Not sure...

Modified

> 06. dumpSubscriptionTable()
>
> Can we assert that remote version is PG17+?

Modified

> 07. check_for_subscription_state()
>
> IIUC, this function is used only for old cluster. Should we follow
> check_old_cluster_for_valid_slots()?

Modified

> 08. check_for_subscription_state()
>
> ```
> +            fprintf(script, "database:%s subscription:%s schema:%s relation:%s state:%s not in required state\n",
> +                    active_db->db_name,
> +                    PQgetvalue(res, i, 0),
> +                    PQgetvalue(res, i, 1),
> +                    PQgetvalue(res, i, 2),
> +                    PQgetvalue(res, i, 3));
> ```
>
> IIRC, format strings should be double-quoted.

Modified

> 09. check_new_cluster_logical_replication_slots()
>
> Checks for replication origin were added in check_new_cluster_logical_replication_slots(),
> but I felt it became a super function. Can we devide?

Modified

> 10. check_new_cluster_logical_replication_slots()
>
> Even if you reject above, it should be renamed.

Since the previous is handled, this is not valid.

> 11. pg_upgrade.h
>
> ```
> +    int         subscription_count; /* number of subscriptions */
> ```
>
> Based on other struct, it should be "nsubscriptions".

Modified

> 12. 004_subscription.pl
>
> ```
> +use File::Path qw(rmtree);
> ```
>
> I think this is not used.

Modified

> 13. 004_subscription.pl
>
> ```
> +my $bindir = $new_sub->config_data('--bindir');
> ```
> For extensibility, it might be better to separate for old/new bindir.

Modified

> 14. 004_subscription.pl
>
> ```
> +my $synced_query =
> +  "SELECT count(1) = 1 FROM pg_subscription_rel WHERE srsubstate = 'r'";
> +$old_sub->poll_query_until('postgres', $synced_query)
> +  or die "Timed out while waiting for subscriber to synchronize data";
> ```
>
> Actually, I'm not sure it is really needed. wait_for_subscription_sync() in line 163
> ensures that sync are done? Are there any holes around here?

wait_for_subscription_sync will check if table is in syndone or in
ready state, since we are allowing sycndone state, I have removed this
part.

> 15. 004_subscription.pl
>
> ```
> +# Check the number of rows for each table on each server
> +my $result =
> +  $publisher->safe_psql('postgres', "SELECT count(*) FROM tab_upgraded");
> +is($result, qq(50), "check initial tab_upgraded table data on publisher");
> +$result =
> +  $publisher->safe_psql('postgres', "SELECT count(*) FROM tab_not_upgraded");
> +is($result, qq(1), "check initial tab_upgraded table data on publisher");
> +$result =
> +  $old_sub->safe_psql('postgres', "SELECT count(*) FROM tab_upgraded");
> +is($result, qq(50),
> +    "check initial tab_upgraded table data on the old subscriber");
> +$result =
> +  $old_sub->safe_psql('postgres', "SELECT count(*) FROM tab_not_upgraded");
> +is($result, qq(0),
> +    "check initial tab_not_upgraded table data on the old subscriber");
> ```
>
> I'm not sure they are really needed. At that time pg_upgrade --check is called,
> this won't change the state of clusters.

In the newer version, the check has been removed now. So these are required.

> 16. pg_proc.dat
>
> ```
> +{ oid => '8404', descr => 'for use by pg_upgrade (relation for pg_subscription_rel)',
> +  proname => 'binary_upgrade_add_sub_rel_state', proisstrict => 'f',
> +  provolatile => 'v', proparallel => 'u', prorettype => 'void',
> +  proargtypes => 'text oid char pg_lsn',
> +  prosrc => 'binary_upgrade_add_sub_rel_state' },
> +{ oid => '8405', descr => 'for use by pg_upgrade (remote_lsn for origin)',
> +  proname => 'binary_upgrade_replorigin_advance', proisstrict => 'f',
> +  provolatile => 'v', proparallel => 'u', prorettype => 'void',
> +  proargtypes => 'text pg_lsn',
> +  prosrc => 'binary_upgrade_replorigin_advance' },
> ```
>
> Based on other function, descr just should be "for use by pg_upgrade".

This was improvised based on one of earlier comments at [1]
The v15 version attached at [2] has the changes for the comments.

[1] - https://www.postgresql.org/message-id/ZQvbV2sdzBY6WEBl%40paquier.xyz
[2] - https://www.postgresql.org/message-id/CALDaNm2ssmSFs4bjpfxbkfUbPE%3DxFSGqxFoip87kF259FG%3DX2g%40mail.gmail.com

Regards,
Vignesh