On Fri, Dec 1, 2023 at 10:57 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> Here are review comments for patch v21-0001
>
>
> 2.
> In this function there are a couple of errors written to the
> "subs_invalid.txt" file:
>
> + fprintf(script, "replication origin is missing for database:\"%s\"
> subscription:\"%s\"\n",
> + PQgetvalue(res, i, 0),
> + PQgetvalue(res, i, 1));
>
> and
>
> + 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));
>
> The format of those messages is not consistent. It could be improved
> in a number of ways to make them more similar. e.g. below.
>
> SUGGESTION #1
> the replication origin is missing for database:\"%s\" subscription:\"%s\"\n
> the table sync state \"%s\" is not allowed for database:\"%s\"
> subscription:\"%s\" schema:\"%s\" relation:\"%s\"\n
>
+1. Shall we keep 'the' as 'The' in the message? Few other messages in
the same file start with capital letter.
>
> 4.
> +# Create a subscription in enabled state before upgrade
> +$publisher->safe_psql('postgres', "CREATE PUBLICATION regress_pub1");
> +$old_sub->safe_psql('postgres',
> + "CREATE SUBSCRIPTION regress_sub1 CONNECTION '$connstr' PUBLICATION
> regress_pub1"
> +);
> +$old_sub->wait_for_subscription_sync($publisher, 'regress_sub1');
>
> That publication has an empty set of tables. Should there be some
> comment to explain why it is OK like this?
>
I think we can add a comment to state the intention of overall test
which this is part of.
>
> 10.
> +# The subscription's running status should be preserved
> +$result =
> + $new_sub->safe_psql('postgres',
> + "SELECT subenabled FROM pg_subscription ORDER BY subname");
> +is($result, qq(t
> +f),
> + "check that the subscription's running status are preserved"
> +);
>
> I felt this was a bit too tricky. It might be more readable to do 2
> separate SELECTs with explicit subnames. Alternatively, leave the code
> as-is but improve the comment to explicitly say something like:
>
> # old subscription regress_sub was enabled
> # old subscription regress_sub1 was disabled
>
I don't see the need to have separate queries though adding comments
is a good idea.
>
> 15.
> extern void AddSubscriptionRelState(Oid subid, Oid relid, char state,
> - XLogRecPtr sublsn);
> + XLogRecPtr sublsn, bool upgrade);
>
> Shouldn't this 'upgrade' really be 'binary_upgrade' so it better
> matches the comment you added in that function?
>
It is better to name this parameter as retain_lock and then explain it
in the function header. The bigger problem with change is that we
should release the other lock
(LockSharedObject(SubscriptionRelationId, subid, 0, AccessShareLock);)
taken in the function as well.
--
With Regards,
Amit Kapila.
