Re: Logical Replication of sequences

On Fri, 20 Dec 2024 at 06:27, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh,
>
> Here are some review comments for the patch v20241211-0004.
>
> ======
> GENERAL
>
> 1.
> There are more than a dozen places where the relation (relkind) is
> checked to see if it is a SEQUENCE:
>
> e.g. + get_rel_relkind(subrel->srrelid) != RELKIND_SEQUENCE &&
> e.g. + if (get_rel_relkind(subrel->srrelid) != RELKIND_SEQUENCE)
> e.g. + if (relkind == RELKIND_SEQUENCE && !get_sequences)
> e.g. + if (relkind != RELKIND_SEQUENCE && !get_tables)
> e.g. + relkind == RELKIND_SEQUENCE ? "sequence" : "table",
> e.g. + if (relkind != RELKIND_SEQUENCE)
> e.g. + relkind == RELKIND_SEQUENCE ? "sequence" : "table",
> e.g. + if (get_rel_relkind(sub_remove_rels[off].relid) == RELKIND_SEQUENCE)
> e.g. + if (get_rel_relkind(relid) != RELKIND_SEQUENCE)
> e.g. + relkind != RELKIND_SEQUENCE)
> e.g. + Assert(get_rel_relkind(rstate->relid) == RELKIND_SEQUENCE);
> e.g. + Assert(relkind == RELKIND_SEQUENCE);
> e.g. + if (get_rel_relkind(rstate->relid) == RELKIND_SEQUENCE)
> e.g. + Assert(get_rel_relkind(rstate->relid) != RELKIND_SEQUENCE);
>
> I am wondering if the code might be improved slightly by adding one new macro:
>
> #define RELKIND_IS_SEQUENCE(relkind) ((relkind) == RELKIND_SEQUENCE)

I was not sure of this, as it is being done like that in other parts
of code like in aclchk.c, should we try to do this change as a
separate patch.

>
> 12.
> - logicalrep_worker_stop(w->subid, w->relid);
> + /* Worker might have exited because of an error */
> + if (w->type == WORKERTYPE_UNKNOWN)
> + continue;
> +
> + logicalrep_worker_stop(w->subid, w->relid, w->type);
>
> It may be better to put that special case WORKERTYPE_UNKNOWN condition
> as a quick exit within the logicalrep_worker_stop() function.

I have removed this as this will be handled by logicalrep_worker_stop
as the Assert is now removed.

> ======
> src/backend/replication/logical/launcher.c
>
> logicalrep_worker_find:
>
> 13.
> + Assert(wtype == WORKERTYPE_TABLESYNC ||
> +    wtype == WORKERTYPE_SEQUENCESYNC ||
> +    wtype == WORKERTYPE_APPLY);
> +
>
> The master version of this function checked for
> isParallelApplyWorker(w), but now if a WORKERTYPE_PARALLEL_APPLY ever
> got to here it would fail the above Assert. So, is the patch code OK,
> or do we still need to also account for a possible
> WORKERTYPE_PARALLEL_APPLY reaching here?

I have removed this assertion and kept it as in master code.

> ~~~
>
> logicalrep_worker_stop:
>
> 14.
> worker = logicalrep_worker_find(subid, relid, wtype, false);
>
> if (worker)
> {
>   Assert(!isParallelApplyWorker(worker));
>   logicalrep_worker_stop_internal(worker, SIGTERM);
> }
>
> ~
>
> This code is not changed much from before, so it first finds the
> worker, but then asserts that it must not be not a parallel apply
> worker. But now, since the wtype is known and passed to the function
> why not move the Assert up-front based on the wtype and before even
> doing the 'find'?

I prefer to keep it the way currently it is, as in_use also is
required to be checked apart from worker type.


> FetchRelationStates:
>
> 23.
> - * Note: If this function started the transaction (indicated by the parameter)
> - * then it is the caller's responsibility to commit it.
> + * Returns true if subscription has 1 or more tables, else false.
>   */
>  bool
> -FetchRelationStates(bool *started_tx)
> +FetchRelationStates(void)
>
> Partly because of the name (relations), I felt this might be better to
> be a void function and the returned value would be passed back by
> references (bool *has_tables).

I did not want to add it as a function parameter, as this value is not
required in all the callers like the caller SyncProcessRelations.
Instead I have changed the variable to has_tables in this caller
function to avoid confusion.

> ======
> src/backend/replication/logical/worker.c
>
> SetupApplyOrSyncWorker:
>
> 24.
> +
> + if (am_sequencesync_worker())
> + before_shmem_exit(logicalrep_seqsyncworker_failuretime, (Datum) 0);
>
> There should be a comment saying what this callback is for.

Function logicalrep_seqsyncworker_failuretime mentions it updates the
failure time. Function ProcessSyncingSequencesForApply mentions the
reason:
 * To prevent starting the sequencesync worker at a high frequency after a
 * failure, we store its last failure time. We start the sequencesync worker
 * again after waiting at least wal_retrieve_retry_interval.

I felt this should be enough.

The rest of the comments are fixed. The attached patch has the changes
for the same.

Regards,
Vignesh