On Mon, 27 Nov 2023 at 06:53, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are some review comments for patch set v19*
>
> //////
>
> v19-0001.
>
> No comments
>
> ///////
>
> v19-0002.
>
> (I saw that both changes below seemed cut/paste from similar
> functions, but I will ask the questions anyway).
>
> ======
> src/backend/commands/subscriptioncmds.c
>
> 1.
> +/* Potentially set by pg_upgrade_support functions */
> +Oid binary_upgrade_next_pg_subscription_oid = InvalidOid;
> +
>
> The comment "by pg_upgrade_support functions" seemed a bit vague. IMO
> you might as well tell the name of the function that sets this.
>
> SUGGESTION
> Potentially set by the pg_upgrade_support function --
> binary_upgrade_set_next_pg_subscription_oid().
Modified
> ~~~
>
> 2. CreateSubscription
>
> + if (!OidIsValid(binary_upgrade_next_pg_subscription_oid))
> + ereport(ERROR,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("pg_subscription OID value not set when in binary upgrade mode")));
>
> Doesn't this condition mean some kind of impossible internal error
> occurred -- i.e. should this be elog instead of ereport?
This is kind of a sanity check to prevent setting the subscription id
with an invalid oid. This can happen if the server is started in
binary upgrade mode and create subscription is called without calling
binary_upgrade_set_next_pg_subscription_oid.
The comment is handled in the v20 version patch attached at:
https://www.postgresql.org/message-id/CALDaNm0ST1iSrJLD_CV6hQs%3Dw4GZRCRdftQvQA3cO8Hq3QUvYw%40mail.gmail.com
Regards,
Vignesh