On Fri, 1 Aug 2025 at 16:18, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Vignesh,
>
> I played with your patch and found something.
>
> 01.
> In LogicalRepSyncSequences() and GetSubscriptionRelations(), there is a possibility
> that the sequence on the subscriber could be dropped before opens that.
> This can cause `could not open relation with OID %u` error, which is not user-friendly.
> Can we avoid that? Even if it is difficult we should add ereport().
Addressed this by taking RowExclusiveLock on the sequence while
preparing the list
> 02.
> ```
> /*
> * Check that our sequencesync worker has permission to insert into
> * the target sequence.
> */
> aclresult = pg_class_aclcheck(RelationGetRelid(sequence_rel), GetUserId(),
> ACL_INSERT);
> if (aclresult != ACLCHECK_OK)
> aclcheck_error(aclresult,
> get_relkind_objtype(sequence_rel->rd_rel->relkind),
> seqname);
> ```
>
> Hmm, but upcoming SetSequence() needs UPDATE privilege. I feel it should be checked.
Modified
> 03.
> Similar with 1, sequences can be dropped just before entering copy_sequences().
> This can cause `cache lookup failed for sequence` error, which cannot be translated.
> Can we avoid that or change the error-function to erport()?
Changed it to log this sequence concurrently dropped
> 04.
> ```
> if (message_level_is_interesting(DEBUG1))
> ereport(DEBUG1,
> errmsg_internal("logical replication synchronization for
subscription\"%s\", sequence \"%s.%s\" has finished",
> MySubscription->name,
> seqinfo->nspname,
> seqinfo->seqname));
> ```
>
> I feel no need to add if-statement because we do not touch additional data here.
Modified
> 05.
> ```
> list_free_deep(sequences_to_copy);
> ```
>
> IIUC, this function free's each elements and list itself, but they do no-op for
> attributes of elements. Can we pfree() for seqname and nspname?
Modified
The attached v20250806 version patch has the changes for the same.
Regards,
Vignesh
