Re: [PATCH] Table sync race with REFRESH PUBLICATION - Mailing list pgsql-hackers

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