On Tuesday, February 22, 2022 3:03 PM Peter Smith <smithpb2250@gmail.com> wrote:
> Here are a couple more review comments for v21.
>
> ~~~
>
> 1. worker.c - comment
>
> + subform = (Form_pg_subscription) GETSTRUCT(tup);
> +
> + /*
> + * We would not be here unless this subscription's disableonerr field
> + was
> + * true, but check whether that field has changed in the interim.
> + */
> + if (!subform->subdisableonerr)
> + {
> + heap_freetuple(tup);
> + table_close(rel, RowExclusiveLock);
> + CommitTransactionCommand();
> + return false;
> + }
>
> I felt that comment belongs above the subform assignment because that is the
> only reason we are getting the subform again.
This part has been removed along with the modification
that we just disable the subscription in the main processing
when we get an error.
> ~~
>
> 2. worker.c - subform->oid
>
> + /* Notify the subscription will be no longer valid */ ereport(LOG,
> + errmsg("logical replication subscription \"%s\" will be disabled due
> to an error",
> + MySubscription->name));
> +
> + LockSharedObject(SubscriptionRelationId, subform->oid, 0,
> AccessExclusiveLock);
>
> Can't we just use MySubscription->oid here? We really only needed that
> subform to get new option values.
Fixed.
> ~~
>
> 3. worker.c - whitespace
>
> Your pg_indent has also changed some whitespace for parts of worker.c that
> are completely unrelated to this patch. You might want to revert those changes.
Fixed.
Kindly have a look at v22 that took in all your comments.
It's shared in [1].
[1] -
https://www.postgresql.org/message-id/TYCPR01MB8373D9B26F988307B0D3FE20ED3E9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
