Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION - Mailing list pgsql-hackers

Hi,

Sorry for the late reply.  Having read this thread, the problem is caused by
misunderstanding the AlterSubscription_refresh().  My apologies.

On Fri, 06 Aug 2021 at 14:12, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> On Fri, Aug 6, 2021 at 1:39 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> On Fri, Aug 6, 2021 at 5:09 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>> >
>> > On Thu, Aug 5, 2021 at 11:40 PM houzj.fnst@fujitsu.com
>> > <houzj.fnst@fujitsu.com> wrote:
>> > >
>> > > > To summary, I think that what we want to do in DROP SUBSCRIPTION cases is to
>> > > > drop relations from pg_subscription_rel that are no longer included in the set of
>> > > > after-calculated publications. In the latter example, when ALTER
>> > > > SUBSCRIPTION ... DROP PUBLICATION pub12, we should compare the current
>> > > > set of relations associated with {pub12, pub13} to the set of relcations associated
>> > > > with pub13, not pub12. Then we can find out t2 is no longer associated with the
>> > > > after-calculated publication (i.g., pub13) so remove it.
>> > >
>> > > Thanks for the review. You are right, I missed this point.
>> > > Attach new version patch which fix these problems(Also add a new pl-test).
>> > >
>> >
>> > Thank you for updating the patch!
>> >
>> > Here are some comments:
>> >
>> > I think that it still could happen that DROP PULIBCATION misses
>> > dropping relations. Please consider the following case:
>> >
>> > On publisher and subscriber:
>> > create table t1 (a int);
>> > create table t2 (a int);
>> > create table t3 (a int);
>> >
>> > On publisher:
>> > create publication pub12 for table t1, t2;
>> > create publication pub3 for table t3;
>> >
>> > On subscriber:
>> > create subscription sub connection 'dbname=postgres' publication pub12, pub3;
>> >
>> > On publisher:
>> > alter publication pub3 add table t1;
>> >
>> > On subscriber:
>> > alter subscription sub drop publication pub12;
>> > select srsubid, srrelid::regclass::text, srsubstate, srsublsn from
>> > pg_subscription_rel;
>> >  srsubid | srrelid | srsubstate | srsublsn
>> > ---------+---------+------------+-----------
>> >    16393 | t3      | r          | 0/1707CE0
>> >    16393 | t1      | r          | 0/1707D18
>> >
>> > With v3 patch, pg_subscription_rel has entries for t3 and t1. But if
>> > ALTER SUBSCRIPTION ... DROP PUBLICATION pub12 refreshes only relations
>> > associated with pub12, t1 should be removed and be added when we
>> > refresh pub3.
>> >
>>
>> But, isn't this happening because of your suggestion to compare the
>> current set of relations with relations from publications that doesn't
>> need to be removed?
>
> Hmm yes, it cannot cover all cases. I had somehow misunderstood that
> the subscriber knows which relations are associated with which
> publications. Given that the subscriber doesn’t know which relations
> are associated with which publications (and the set of subscribed
> relations on the subscriber becomes out of date once the publication
> is updated), I think it's impossible to achieve that DROP PUBLICATION
> drops relations that are associated with only the publication being
> dropped.
>
>> Do we have better ideas to fix or shall we just
>> say that we will refresh based on whatever current set of relations
>> are present in publication to be dropped?
>
> I don't have better ideas. It seems to me that it's prudent that we
> accept the approach in v3 patch and refresh all publications in DROP
> PUBLICATION cases.
>

Maybe refreshing all publications in PUBLICATION cases is simple way to
fix the problem.

>>
>> One more thing, I think it would be better to write a separate refresh
>> function for add/drop rather than adding checks in the current refresh
>> functionality. We can extract common functionality code which can be
>> called from the different refresh functions.
>
> +1
>

Agreed.

> Regards,


--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.