On Mon, Oct 24, 2022 at 11:41 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> Here are my review comments for v40-0001.
>
> ======
>
> src/backend/replication/logical/worker.c
>
>
> 1. should_apply_changes_for_rel
>
> + else if (am_parallel_apply_worker())
> + {
> + if (rel->state != SUBREL_STATE_READY)
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("logical replication parallel apply worker for subscription
> \"%s\" will stop",
> + MySubscription->name),
> + errdetail("Cannot handle streamed replication transaction using parallel "
> + "apply workers until all tables are synchronized.")));
>
> 1a.
> "transaction" -> "transactions"
>
> 1b.
> "are synchronized" -> "have been synchronized."
>
> e.g. "Cannot handle streamed replication transactions using parallel
> apply workers until all tables have been synchronized."
>
> ~~~
>
> 2. maybe_reread_subscription
>
> + if (am_parallel_apply_worker())
> + ereport(LOG,
> + (errmsg("logical replication parallel apply worker for subscription
> \"%s\" will "
> + "stop because the subscription was removed",
> + MySubscription->name)));
> + else
> + ereport(LOG,
> + (errmsg("logical replication apply worker for subscription \"%s\" will "
> + "stop because the subscription was removed",
> + MySubscription->name)));
>
> Maybe there is an easier way to code this instead of if/else and
> cut/paste message text:
>
> SUGGESTION
>
> ereport(LOG,
> (errmsg("logical replication %s for subscription \"%s\" will stop
> because the subscription was removed",
> am_parallel_apply_worker() ? "parallel apply worker" : "apply worker",
> MySubscription->name)));
> ~~~
>
If we want to go this way then it may be better to record the
appropriate string beforehand and use that here.
--
With Regards,
Amit Kapila.
