On Fri, Aug 12, 2022 at 10:52 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Aug 12, 2022 at 12:44 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
>
> ISTM that the "This" in the first sentence in the second paragraph
> still indicates the cleanup of the origin tracking and table sync
> slot. How about not having the common comment for both like the patch
> I've attached? In addition to this change, I moved the code to drop
> the origin before stopping the streaming so that it can be close to
> the slot drop. But feel free to revert this change.
>
After this change, won't the code in AlterSubscription_refresh()
related to origin drop [1] needs to be updated? Firstly, now even if
the state is SYNCDONE, we would have already dropped the origin. Then,
I think the race mentioned in the comments no longer remains true
because before dropping the origin in tablesync worker, we would have
taken a conflicting lock on pg_subscription_rel.
[1] -
/*
* For READY state, we would have already dropped the
* tablesync origin.
*/
if (state != SUBREL_STATE_READY)
{
char originname[NAMEDATALEN];
/*
* Drop the tablesync's origin tracking if exists.
*
* It is possible that the origin is not yet created for
* tablesync worker, this can happen for the states before
* SUBREL_STATE_FINISHEDCOPY. The apply worker can also
* concurrently try to drop the origin and by this time
* the origin might be already removed. For these reasons,
* passing missing_ok = true.
*/
ReplicationOriginNameForTablesync(sub->oid, relid, originname,
sizeof(originname));
replorigin_drop_by_name(originname, true, false);
}
--
With Regards,
Amit Kapila.