Re: [PATCH] Table sync race with REFRESH PUBLICATION

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?

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?

Something like:

current_relstate = GetSubscriptionRelState(...);

if (current_relstate != SUBREL_STATE_SYNCDONE || current_lsn < statelsn)
{
char * msg = (current_relstate == SUBREL_STATE_UNKNOWN) ?
"a concurrent refresh has removed relation oid %u of
subscription \"%s\"" :
"a concurrent refresh has changed relation oid %u of
subscription \"%s\"";

ereport(LOG, errmsg(msg, rstate->relid, MySubscription->name));

/*
* Skipping is the only sensible action. It must not be treated as an error
* because when disable_on_error is true, that would disable the entire
* subscription, including unrelated tables.
*/
continue;
}

~~~

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?

======
[1] https://github.com/postgres/postgres/commit/dca73f7dd03ddfc89a88019659fc0161e0427a8c#diff-e188109d851e85cc74d6bb7543f8ddf2be5cc2764e976ed44a7c82ddcc5ad884R697

Kind Regards,
Peter Smith.
Fujitsu Australia