Hi,
Thanks for the review!
On Mon, 28 Sept 2026 at 14:48, Peter Smith <smithpb2250@gmail.com> wrote:
>
> A couple of comments for 0001.
>
> ======
>
> 1.
> + if (GetSubscriptionRelState(MyLogicalRepWorker->subid,
> + rstate->relid, &statelsn) != SUBREL_STATE_SYNCDONE ||
> + current_lsn < statelsn)
> + continue;
>
> 1.
> Would a local variable simplify the condition?
Yes, done.
> Also, the related SEQUENCE code [1] had
> i) logging if COPYSEQ_NOT_SUBSCRIBED was detected. Should this do
> something similar?
> ii) a comment saying the error must be avoided. Should this do
> something similar?
I added a DEBUG1 message. I didn't use LOG because, unlike the
sequence case, the copy has already finished by this point. We only
skip the READY update for a row that's gone (or now belongs to a new
sync), so there isn't really anything for the user to act on?
For the comment, I kept it to one line saying the table may have been
removed or re-added while we were waiting for the lock.
> 2.
> IIUC,
>
> i) If the concurrent REFRESH removed the relation, then the new
> relstate will be SUBREL_STATE_UNKNOWN.
> ii) If there were multiple concurrent REFRESHes and the same relation
> got re-added, then the state would be SUBREL_STATE_INIT.
>
> Either way, the state is not SUBREL_STATE_SYNCDONE.
>
> AFAIK, there is no way for a newly added same relation to get back to
> SUBREL_STATE_SYNCDONE while we are still blocked on this lock. IOW,
> was that extra LSN check (current_lsn < statelsn) really needed? Is
> just checking the state enough?
Yeah, I think you're right, got rid of it.
Attached is v2 with those changes. Thoughts?
Regards,
Ayush
