Hi, Thanks for the review!
On Mon, 28 Sept 2026 at 14:48, Peter Smith <[email protected]> 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
v2-0001-Recheck-table-sync-state-after-refresh.patch
Description: Binary data
v2-0002-Test-table-sync-after-a-concurrent-refresh.patch
Description: Binary data
