Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Tue, Aug 24, 2021 at 5:38 PM Amit Kapila wrote: > > On Tue, Aug 24, 2021 at 7:13 AM Masahiko Sawada wrote: > > > > On Tue, Aug 24, 2021 at 10:01 AM houzj.f...@fujitsu.com > > wrote: > > > > > > > > > > > > > -Original Message- > > > > From: Masahiko Sawada > > > > Sent: Tuesday, August 24, 2021 8:52 AM > > > > > > > > Thanks. The patch for HEAD looks good to me. Regarding the patch for > > > > PG14, I > > > > think we need to update the comment as well: > > > > > > > > @@ -987,7 +986,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool > > > > isTopLevel) > > > > PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with > > > > refresh"); > > > > > > > > /* Only refresh the added/dropped list of publications. */ > > > > - sub->publications = stmt->publication; > > > > + sub->publications = publist; > > > > > > > > > > Thanks for the comment, attach new version patch which fixed it. > > > > Thank you for updating the patch! Looks good to me. > > > > Pushed! Thanks! I've marked this open item as resolved. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Tue, Aug 24, 2021 at 7:13 AM Masahiko Sawada wrote: > > On Tue, Aug 24, 2021 at 10:01 AM houzj.f...@fujitsu.com > wrote: > > > > > > > > > -Original Message- > > > From: Masahiko Sawada > > > Sent: Tuesday, August 24, 2021 8:52 AM > > > > > > Thanks. The patch for HEAD looks good to me. Regarding the patch for > > > PG14, I > > > think we need to update the comment as well: > > > > > > @@ -987,7 +986,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool > > > isTopLevel) > > > PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with > > > refresh"); > > > > > > /* Only refresh the added/dropped list of publications. */ > > > - sub->publications = stmt->publication; > > > + sub->publications = publist; > > > > > > > Thanks for the comment, attach new version patch which fixed it. > > Thank you for updating the patch! Looks good to me. > Pushed! -- With Regards, Amit Kapila.
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Tue, Aug 24, 2021 at 10:01 AM houzj.f...@fujitsu.com wrote: > > > > > -Original Message- > > From: Masahiko Sawada > > Sent: Tuesday, August 24, 2021 8:52 AM > > > > Thanks. The patch for HEAD looks good to me. Regarding the patch for PG14, I > > think we need to update the comment as well: > > > > @@ -987,7 +986,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool > > isTopLevel) > > PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with > > refresh"); > > > > /* Only refresh the added/dropped list of publications. */ > > - sub->publications = stmt->publication; > > + sub->publications = publist; > > > > Thanks for the comment, attach new version patch which fixed it. Thank you for updating the patch! Looks good to me. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
RE: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
> -Original Message- > From: Masahiko Sawada > Sent: Tuesday, August 24, 2021 8:52 AM > > Thanks. The patch for HEAD looks good to me. Regarding the patch for PG14, I > think we need to update the comment as well: > > @@ -987,7 +986,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool > isTopLevel) > PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with > refresh"); > > /* Only refresh the added/dropped list of publications. */ > - sub->publications = stmt->publication; > + sub->publications = publist; > Thanks for the comment, attach new version patch which fixed it. Best regards, Hou zj v8-0001-PG15-Fix-Alter-Subscription-Add-Drop-Publication-refre.patch Description: v8-0001-PG15-Fix-Alter-Subscription-Add-Drop-Publication-refre.patch v8-0001-PG14-Fix-Alter-Subscription-Add-Drop-Publication-refre_patch Description: v8-0001-PG14-Fix-Alter-Subscription-Add-Drop-Publication-refre_patch
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Mon, Aug 23, 2021 at 11:05 PM houzj.f...@fujitsu.com wrote: > > On Mon, Aug 23, 2021 8:01 PM Amit Kapila wrote: > > On Mon, Aug 23, 2021 at 2:45 PM > > houzj.f...@fujitsu.com wrote: > > > > > > On Mon, Aug 23, 2021 12:59 PM Amit Kapila wrote: > > > > > > > > On Sat, Aug 7, 2021 at 6:53 PM houzj.f...@fujitsu.com > > > > wrote: > > > > > > > > > > Personally, I also think it will be better to make the behavior > > > > > consistent. > > > > > Attach the new version patch make both ADD and DROP behave the > > > > > same as SET PUBLICATION which refresh all the publications. > > > > > > > > > > > > > I think we can have tests in the separate test file > > > > (alter_sub_pub.pl) like you earlier had in one of the versions. Use > > > > some meaningful names for tables instead of temp1, temp2 as you had in > > > > the > > previous version. > > > > Otherwise, the code changes look good to me. > > > > > > Thanks for the comment. > > > Attach new version patch which did the following changes. > > > > > > * move the tests to a separate test file (024_alter_sub_pub.pl) > > > * adjust the document of copy_data option > > > * add tab-complete for copy_data option when ALTER DROP publication. > > > > > > > Thanks, the patch looks good to me. I have made some cosmetic changes in the > > attached version. It makes the test cases easier to understand. > > I am planning to push this tomorrow unless there are more comments or > > suggestions. > > Thanks ! The patch looks good to me. > > I noticed that the patch cannot be applied to PG14-stable, > so I attach a separate patch for back-patch(I didn’t change the patch for > HEAD branch). Thanks. The patch for HEAD looks good to me. Regarding the patch for PG14, I think we need to update the comment as well: @@ -987,7 +986,7 @@ AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel) PreventInTransactionBlock(isTopLevel, "ALTER SUBSCRIPTION with refresh"); /* Only refresh the added/dropped list of publications. */ - sub->publications = stmt->publication; + sub->publications = publist; Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
RE: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Mon, Aug 23, 2021 8:01 PM Amit Kapila wrote: > On Mon, Aug 23, 2021 at 2:45 PM > houzj.f...@fujitsu.com wrote: > > > > On Mon, Aug 23, 2021 12:59 PM Amit Kapila wrote: > > > > > > On Sat, Aug 7, 2021 at 6:53 PM houzj.f...@fujitsu.com > > > wrote: > > > > > > > > Personally, I also think it will be better to make the behavior > > > > consistent. > > > > Attach the new version patch make both ADD and DROP behave the > > > > same as SET PUBLICATION which refresh all the publications. > > > > > > > > > > I think we can have tests in the separate test file > > > (alter_sub_pub.pl) like you earlier had in one of the versions. Use > > > some meaningful names for tables instead of temp1, temp2 as you had in the > previous version. > > > Otherwise, the code changes look good to me. > > > > Thanks for the comment. > > Attach new version patch which did the following changes. > > > > * move the tests to a separate test file (024_alter_sub_pub.pl) > > * adjust the document of copy_data option > > * add tab-complete for copy_data option when ALTER DROP publication. > > > > Thanks, the patch looks good to me. I have made some cosmetic changes in the > attached version. It makes the test cases easier to understand. > I am planning to push this tomorrow unless there are more comments or > suggestions. Thanks ! The patch looks good to me. I noticed that the patch cannot be applied to PG14-stable, so I attach a separate patch for back-patch(I didn’t change the patch for HEAD branch). Best regards, Hou zj v7-0001-PG14-Fix-Alter-Subscription-Add-Drop-Publication-refre_patch Description: v7-0001-PG14-Fix-Alter-Subscription-Add-Drop-Publication-refre_patch v7-0001-PG15-Fix-Alter-Subscription-Add-Drop-Publication-refre.patch Description: v7-0001-PG15-Fix-Alter-Subscription-Add-Drop-Publication-refre.patch
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Mon, Aug 23, 2021 at 2:45 PM houzj.f...@fujitsu.com wrote: > > On Mon, Aug 23, 2021 12:59 PM Amit Kapila wrote: > > > > On Sat, Aug 7, 2021 at 6:53 PM houzj.f...@fujitsu.com > > wrote: > > > > > > Personally, I also think it will be better to make the behavior > > > consistent. > > > Attach the new version patch make both ADD and DROP behave the same as > > > SET PUBLICATION which refresh all the publications. > > > > > > > I think we can have tests in the separate test file (alter_sub_pub.pl) like > > you > > earlier had in one of the versions. Use some meaningful names for tables > > instead of temp1, temp2 as you had in the previous version. > > Otherwise, the code changes look good to me. > > Thanks for the comment. > Attach new version patch which did the following changes. > > * move the tests to a separate test file (024_alter_sub_pub.pl) > * adjust the document of copy_data option > * add tab-complete for copy_data option when ALTER DROP publication. > Thanks, the patch looks good to me. I have made some cosmetic changes in the attached version. It makes the test cases easier to understand. I am planning to push this tomorrow unless there are more comments or suggestions. -- With Regards, Amit Kapila. v6-0001-Fix-Alter-Subscription-Add-Drop-Publication-refre.patch Description: Binary data
RE: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Mon, Aug 23, 2021 12:59 PM Amit Kapila wrote: > > On Sat, Aug 7, 2021 at 6:53 PM houzj.f...@fujitsu.com > wrote: > > > > Personally, I also think it will be better to make the behavior consistent. > > Attach the new version patch make both ADD and DROP behave the same as > > SET PUBLICATION which refresh all the publications. > > > > I think we can have tests in the separate test file (alter_sub_pub.pl) like > you > earlier had in one of the versions. Use some meaningful names for tables > instead of temp1, temp2 as you had in the previous version. > Otherwise, the code changes look good to me. Thanks for the comment. Attach new version patch which did the following changes. * move the tests to a separate test file (024_alter_sub_pub.pl) * adjust the document of copy_data option * add tab-complete for copy_data option when ALTER DROP publication. Best regards, Hou zj v5-0001-Fix-Alter-Subscription-Add-Drop-Publication-refresh-.patch Description: v5-0001-Fix-Alter-Subscription-Add-Drop-Publication-refresh-.patch
RE: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Mon, Aug 23, 2021 1:18 PM Masahiko Sawada wrote: > On Mon, Aug 23, 2021 at 1:59 PM Amit Kapila > wrote: > > > > On Sat, Aug 7, 2021 at 6:53 PM houzj.f...@fujitsu.com > > wrote: > > > > > > Personally, I also think it will be better to make the behavior > > > consistent. > > > Attach the new version patch make both ADD and DROP behave the same > > > as SET PUBLICATION which refresh all the publications. > > > > > > > I think we can have tests in the separate test file (alter_sub_pub.pl) > > like you earlier had in one of the versions. Use some meaningful names > > for tables instead of temp1, temp2 as you had in the previous version. > > Otherwise, the code changes look good to me. > > - supported_opts = SUBOPT_REFRESH; > - if (isadd) > - supported_opts |= SUBOPT_COPY_DATA; > + supported_opts = SUBOPT_REFRESH | SUBOPT_COPY_DATA; > > I think that the currently the doc says copy_data option can be specified > except > in DROP PUBLICATION case, which needs to be fixed corresponding the above > change. Thanks for the comment. Fixed in the new version patch. Best regards, Hou zj
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Mon, Aug 23, 2021 at 1:59 PM Amit Kapila wrote: > > On Sat, Aug 7, 2021 at 6:53 PM houzj.f...@fujitsu.com > wrote: > > > > Personally, I also think it will be better to make the behavior consistent. > > Attach the new version patch make both ADD and DROP behave the same as SET > > PUBLICATION > > which refresh all the publications. > > > > I think we can have tests in the separate test file (alter_sub_pub.pl) > like you earlier had in one of the versions. Use some meaningful names > for tables instead of temp1, temp2 as you had in the previous version. > Otherwise, the code changes look good to me. - supported_opts = SUBOPT_REFRESH; - if (isadd) - supported_opts |= SUBOPT_COPY_DATA; + supported_opts = SUBOPT_REFRESH | SUBOPT_COPY_DATA; I think that the currently the doc says copy_data option can be specified except in DROP PUBLICATION case, which needs to be fixed corresponding the above change. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Wed, Aug 18, 2021 at 7:54 PM Peter Eisentraut wrote: > > On 10.08.21 05:22, Amit Kapila wrote: > > Yeah, unless we change design drastically we might not be able to do a > > refresh for dropped publications, for add it is possible. It seems > > most of the people responded on this thread that we can be consistent > > in terms of refreshing for add/drop at this stage but we can still go > > with another option where we can refresh only newly added publications > > for add but for drop refresh all publications. > > > > Peter E., do you have any opinion on this matter? > > Refresh everything for both ADD and DROP makes sense to me. > Thanks for your suggestion. -- With Regards, Amit Kapila.
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Sat, Aug 7, 2021 at 6:53 PM houzj.f...@fujitsu.com wrote: > > Personally, I also think it will be better to make the behavior consistent. > Attach the new version patch make both ADD and DROP behave the same as SET > PUBLICATION > which refresh all the publications. > I think we can have tests in the separate test file (alter_sub_pub.pl) like you earlier had in one of the versions. Use some meaningful names for tables instead of temp1, temp2 as you had in the previous version. Otherwise, the code changes look good to me. -- With Regards, Amit Kapila.
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On 10.08.21 05:22, Amit Kapila wrote: Yeah, unless we change design drastically we might not be able to do a refresh for dropped publications, for add it is possible. It seems most of the people responded on this thread that we can be consistent in terms of refreshing for add/drop at this stage but we can still go with another option where we can refresh only newly added publications for add but for drop refresh all publications. Peter E., do you have any opinion on this matter? Refresh everything for both ADD and DROP makes sense to me.
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Tue, Aug 10, 2021 at 8:05 AM Masahiko Sawada wrote: > > On Sat, Aug 7, 2021 at 2:36 PM Amit Kapila wrote: > > > > On Fri, Aug 6, 2021 at 9:57 PM Japin Li wrote: > > > > > > > > > > > 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. > > > > > > > Actually, doing it both will make the behavior consistent but doing it > > just for Drop might be preferable by some users. > > I agree that ADD/DROP PUBLICATION is convenient to just add/drop > publications instead of providing the complete publication list. But > I'm concerned that during developing this feature most people didn't > like the behavior of refreshing new and existing publications. Also, > there was pointing out that there will be an overhead of a SQL with > more publications when AlterSubscription_refresh() is called with all > the existing publications. > True, but I think at that time we didn't know this design problem with 'drop publication'. > If we feel this behavior is unnatural, the > users will feel the same. I personally think there is a benefit only > in terms of syntax. > Right, and I think in this particular case, the new syntax is much easier to use for users. > And we can work on the part of refreshing only > publications being added/dropped on v15 development. > Yeah, unless we change design drastically we might not be able to do a refresh for dropped publications, for add it is possible. It seems most of the people responded on this thread that we can be consistent in terms of refreshing for add/drop at this stage but we can still go with another option where we can refresh only newly added publications for add but for drop refresh all publications. Peter E., do you have any opinion on this matter? -- With Regards, Amit Kapila.
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Sat, Aug 7, 2021 at 2:36 PM Amit Kapila wrote: > > On Fri, Aug 6, 2021 at 9:57 PM Japin Li wrote: > > > > > > > > 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. > > > > Actually, doing it both will make the behavior consistent but doing it > just for Drop might be preferable by some users. I agree that ADD/DROP PUBLICATION is convenient to just add/drop publications instead of providing the complete publication list. But I'm concerned that during developing this feature most people didn't like the behavior of refreshing new and existing publications. Also, there was pointing out that there will be an overhead of a SQL with more publications when AlterSubscription_refresh() is called with all the existing publications. If we feel this behavior is unnatural, the users will feel the same. I personally think there is a benefit only in terms of syntax. And we can work on the part of refreshing only publications being added/dropped on v15 development. But it might be better to consider also if there will be use cases for users to add/remove publications in the subscription even with such downsides. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
RE: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Monday, August 9, 2021 11:10 AM Amit Kapila wrote: > > On Sat, Aug 7, 2021 at 6:53 PM houzj.f...@fujitsu.com > wrote: > > > > On Sat, Aug 7, 2021 1:36 PM Amit Kapila wrote: > > > On Fri, Aug 6, 2021 at 9:57 PM Japin Li wrote: > > > > > > Do you mean to say that do it for both Add and Drop or just for Drop? > > > Actually, doing it both will make the behavior consistent but doing > > > it just for Drop might be preferable by some users. I think it is > > > better to be consistent here but I am fine if you and others feel it > > > is better to refresh all publications only in Drop case. > > > > Personally, I also think it will be better to make the behavior consistent. > > Attach the new version patch make both ADD and DROP behave the same as > > SET PUBLICATION which refresh all the publications. > > > > There is a bug reported on pgsql-bugs [1] which seems to be happening due to > the same reason. Can you please test that the other bug is also fixed with > your > patch? > > [1] - > https://www.postgresql.org/message-id/17132-6a702189086c6243%40postgres > ql.org Thanks for the reminder, I have checked and tested the reported bug. The reported bug is that when drop and then re-add a publication on subscriber side, the table in the publication wasn't synced. And after applying the patch here, the table will be synced as expected if re-added(behaves the same as SET PUBLICATION). Best regards, Hou zj
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Sat, Aug 7, 2021 at 6:53 PM houzj.f...@fujitsu.com wrote: > > On Sat, Aug 7, 2021 1:36 PM Amit Kapila wrote: > > On Fri, Aug 6, 2021 at 9:57 PM Japin Li wrote: > > > > Do you mean to say that do it for both Add and Drop or just for Drop? > > Actually, doing it both will make the behavior consistent but doing it just > > for > > Drop might be preferable by some users. I think it is better to be > > consistent here > > but I am fine if you and others feel it is better to refresh all > > publications only in > > Drop case. > > Personally, I also think it will be better to make the behavior consistent. > Attach the new version patch make both ADD and DROP behave the same as SET > PUBLICATION > which refresh all the publications. > There is a bug reported on pgsql-bugs [1] which seems to be happening due to the same reason. Can you please test that the other bug is also fixed with your patch? [1] - https://www.postgresql.org/message-id/17132-6a702189086c6243%40postgresql.org -- With Regards, Amit Kapila.
RE: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Sat, Aug 7, 2021 1:36 PM Amit Kapila wrote: > On Fri, Aug 6, 2021 at 9:57 PM Japin Li wrote: > > > > > > > > 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. > > > > Do you mean to say that do it for both Add and Drop or just for Drop? > Actually, doing it both will make the behavior consistent but doing it just > for > Drop might be preferable by some users. I think it is better to be consistent > here > but I am fine if you and others feel it is better to refresh all publications > only in > Drop case. Personally, I also think it will be better to make the behavior consistent. Attach the new version patch make both ADD and DROP behave the same as SET PUBLICATION which refresh all the publications. Best regards, houzj v4-0001-fix-ALTER-SUBSCRIPTION-ADD-DROP-PUBLICATION.patch Description: v4-0001-fix-ALTER-SUBSCRIPTION-ADD-DROP-PUBLICATION.patch
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
> On Aug 7, 2021, at 1:35 PM, Amit Kapila wrote: > > On Fri, Aug 6, 2021 at 9:57 PM Japin Li wrote: >> >>> >>> 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. >> > > Do you mean to say that do it for both Add and Drop or just for Drop? > Actually, doing it both will make the behavior consistent but doing it > just for Drop might be preferable by some users. I think it is better > to be consistent here but I am fine if you and others feel it is > better to refresh all publications only in Drop case. > I prefer to refresh all PUBLICATIONS for both ADD and DROP operations, I think this is more consistent and makes the code simple. -- Best regards Japin Li
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Fri, Aug 6, 2021 at 9:57 PM Japin Li wrote: > > > > > 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. > Do you mean to say that do it for both Add and Drop or just for Drop? Actually, doing it both will make the behavior consistent but doing it just for Drop might be preferable by some users. I think it is better to be consistent here but I am fine if you and others feel it is better to refresh all publications only in Drop case. -- With Regards, Amit Kapila.
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Fri, Aug 6, 2021 at 2:50 PM Amit Kapila wrote: > > On Fri, Aug 6, 2021 at 10:09 AM Amit Kapila wrote: > > > > On Fri, Aug 6, 2021 at 5:09 AM Masahiko Sawada > > wrote: > > > > > > > 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? 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? > > > > The other options could be that (a) for drop publication, we refresh > all the publications, (b) for both add/drop publication, we refresh > all the publications. > > Any better ideas? > > As this is introduced in PG-14 via the below commit, I am adding > authors and committer to see if they have any better ideas. > > commit 82ed7748b710e3ddce3f7ebc74af80fe4869492f > Author: Peter Eisentraut > Date: Tue Apr 6 10:44:26 2021 +0200 > > ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION > > Author: Japin Li > Author: Bharath Rupireddy > I've added this to Open Items. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
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 wrote:
> On Fri, Aug 6, 2021 at 1:39 PM Amit Kapila wrote:
>>
>> On Fri, Aug 6, 2021 at 5:09 AM Masahiko Sawada wrote:
>> >
>> > On Thu, Aug 5, 2021 at 11:40 PM houzj.f...@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.
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Fri, Aug 6, 2021 at 1:39 PM Amit Kapila wrote:
>
> On Fri, Aug 6, 2021 at 5:09 AM Masahiko Sawada wrote:
> >
> > On Thu, Aug 5, 2021 at 11:40 PM houzj.f...@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.
>
> 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
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Fri, Aug 6, 2021 at 10:09 AM Amit Kapila wrote: > > On Fri, Aug 6, 2021 at 5:09 AM Masahiko Sawada wrote: > > > > 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? 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? > The other options could be that (a) for drop publication, we refresh all the publications, (b) for both add/drop publication, we refresh all the publications. Any better ideas? As this is introduced in PG-14 via the below commit, I am adding authors and committer to see if they have any better ideas. commit 82ed7748b710e3ddce3f7ebc74af80fe4869492f Author: Peter Eisentraut Date: Tue Apr 6 10:44:26 2021 +0200 ALTER SUBSCRIPTION ... ADD/DROP PUBLICATION Author: Japin Li Author: Bharath Rupireddy -- With Regards, Amit Kapila.
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Fri, Aug 6, 2021 at 5:09 AM Masahiko Sawada wrote:
>
> On Thu, Aug 5, 2021 at 11:40 PM houzj.f...@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? 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?
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.
--
With Regards,
Amit Kapila.
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Thu, Aug 5, 2021 at 11:40 PM houzj.f...@fujitsu.com
wrote:
>
> On Thursday, August 5, 2021 1:09 PM Masahiko Sawada
> wrote
> > I've reviewed v2 patch. Here are some comments:
> >
> > + if (type == ALTER_SUBSCRIPTION_SET_PUBLICATION ||
> > + type == ALTER_SUBSCRIPTION_REFRESH)
> > + drop_table = !bsearch(, pubrel_local_oids,
> > + list_length(pubrel_names),
> > + sizeof(Oid), oid_cmp);
> > + else if (type == ALTER_SUBSCRIPTION_DROP_PUBLICATION)
> > + drop_table = bsearch(, pubrel_local_oids,
> > +list_length(pubrel_names),
> > +sizeof(Oid), oid_cmp);
> >
> > IIUC, in DROP PUBLICATION cases, we don't need to refer to the tables in the
> > publication on the publisher that we're dropping in order to check whether
> > to
> > remove the relation. If we drop a table from the publication on the
> > publisher
> > before dropping the publication from the subscription on the subscriber, the
> > pg_subscription_rel entry for the table remains. For example, 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 pub12 drop table t2;
> >
> > 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 | t2 | r | 0/1707CE0
> >
> > With v2 patch, the pg_subscription_rel has the entry for 't2' but it should
> > not
> > exist.
> >
> > But that doesn't mean that drop_table should always be true in DROP
> > PULIBCATION cases. Since the tables associated with two publications can
> > overlap, we cannot remove the relation from pg_subscription_rel if it is
> > also
> > associated with the remaining publication. For example:
> >
> > 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 pub13 for
> > table t1, t3;
> >
> > On subscriber:
> > create subscription sub connection 'dbname=postgres' publication pub12,
> > pub13;
> >
> > 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/17080B0
> > (1 row)
> >
> > With v2 patch, the pg_subscription_rel has only one entry for t3 but it
> > should
> > have also the one for t1 since it's still subscribing to pub13.
> >
> > 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
RE: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Thursday, August 5, 2021 1:09 PM Masahiko Sawada
wrote
> I've reviewed v2 patch. Here are some comments:
>
> + if (type == ALTER_SUBSCRIPTION_SET_PUBLICATION ||
> + type == ALTER_SUBSCRIPTION_REFRESH)
> + drop_table = !bsearch(, pubrel_local_oids,
> + list_length(pubrel_names),
> + sizeof(Oid), oid_cmp);
> + else if (type == ALTER_SUBSCRIPTION_DROP_PUBLICATION)
> + drop_table = bsearch(, pubrel_local_oids,
> +list_length(pubrel_names),
> +sizeof(Oid), oid_cmp);
>
> IIUC, in DROP PUBLICATION cases, we don't need to refer to the tables in the
> publication on the publisher that we're dropping in order to check whether to
> remove the relation. If we drop a table from the publication on the publisher
> before dropping the publication from the subscription on the subscriber, the
> pg_subscription_rel entry for the table remains. For example, 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 pub12 drop table t2;
>
> 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 | t2 | r | 0/1707CE0
>
> With v2 patch, the pg_subscription_rel has the entry for 't2' but it should
> not
> exist.
>
> But that doesn't mean that drop_table should always be true in DROP
> PULIBCATION cases. Since the tables associated with two publications can
> overlap, we cannot remove the relation from pg_subscription_rel if it is also
> associated with the remaining publication. For example:
>
> 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 pub13 for table
> t1, t3;
>
> On subscriber:
> create subscription sub connection 'dbname=postgres' publication pub12,
> pub13;
>
> 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/17080B0
> (1 row)
>
> With v2 patch, the pg_subscription_rel has only one entry for t3 but it should
> have also the one for t1 since it's still subscribing to pub13.
>
> 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).
Best regards,
Houzj
v3-0001-fix-ALTER-SUB-ADD-DROP-PUBLICATION.patch
Description: v3-0001-fix-ALTER-SUB-ADD-DROP-PUBLICATION.patch
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Thu, Aug 5, 2021 at 2:08 PM Masahiko Sawada wrote: > > On Wed, Aug 4, 2021 at 9:19 PM houzj.f...@fujitsu.com > wrote: > > > > On Wednesday, August 4, 2021 7:00 PM Masahiko Sawada > > wrote > > > On Wed, Aug 4, 2021 at 5:06 PM houzj.f...@fujitsu.com > > > wrote: > > > > > > > > On Wednesday, August 4, 2021 1:47 PM Masahiko Sawada > > > > > > > > > > > > > > I've not looked at the patch deeply yet but I think that the > > > > > following one line change seems to fix the cases you reported, > > > > > although I migit be missing > > > > > something: > > > > > > > > > > diff --git a/src/backend/commands/subscriptioncmds.c > > > > > b/src/backend/commands/subscriptioncmds.c > > > > > index 22ae982328..c74d6d51d6 100644 > > > > > --- a/src/backend/commands/subscriptioncmds.c > > > > > +++ b/src/backend/commands/subscriptioncmds.c > > > > > @@ -1068,7 +1068,7 @@ AlterSubscription(ParseState *pstate, > > > > > AlterSubscriptionStmt *stmt, > > > > > PreventInTransactionBlock(isTopLevel, "ALTER > > > > > SUBSCRIPTION with refresh"); > > > > > > > > > > /* Only refresh the added/dropped list of > > > > > publications. > > > */ > > > > > - sub->publications = stmt->publication; > > > > > + sub->publications = publist; > > > > > > > > > > AlterSubscription_refresh(sub, opts.copy_data); > > > > > } > > > > > > > > I considered the same fix before, but with the above fix, it seems > > > > refresh both existsing publications and new publication, which most of > > > > people didn't like in previous discussion[1]. From the doc[2], IIRC, > > > > Currently, the ADD/DROP PUBLICATION is supposed to only refresh the new > > > publication. > > > > > > What do you mean by refreshing both existing and new publications? I > > > dropped > > > and created one publication out of three publications that the > > > subscription is > > > subscribing to but the corresponding entries in pg_subscription_rel for > > > tables > > > associated with the other two publications were not changed and the table > > > sync > > > workers also were not started for them. > > > > > > > + sub->publications = publist; > > - sub->publications = stmt->publication; > > With the above fix, When ADD/DROP PUBLICATION, I meant the table that > > doesn't > > belong to the dropped or added publication could be updated in > > pg_subscription_rel. > > > > I can see the effect in the following example: > > > > 1)-publisher- > > CREATE TABLE test,test2,test3 > > CREATE PUBLICATION pub FOR TABLE test; > > CREATE PUBLICATION pub2 FOR TABLE test2; > > 2)-subscriber- > > CREATE TABLE test,test2,test3 > > CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres port=1' PUBLICATION > > pub,pub2; > > select * from pg_subscription_rel; > > -[ RECORD 1 ]- > > srsubid| 16393 > > srrelid| 16387 > > srsubstate | r > > srsublsn | 0/150A2D8 > > -[ RECORD 2 ]- > > srsubid| 16393 > > srrelid| 16384 > > srsubstate | r > > srsublsn | 0/150A310 > > > > 3)-publisher- > > alter publication pub add table test3; > > 4)-subscriber- > > ALTER SUBSCRIPTION sub drop PUBLICATION pub2; > > select * from pg_subscription_rel; > > -[ RECORD 1 ]- > > srsubid| 16393 > > srrelid| 16384 > > srsubstate | r > > srsublsn | 0/150A310 > > -[ RECORD 2 ]- > > srsubid| 16393 > > srrelid| 16390 > > srsubstate | r > > srsublsn | > > > > I can see the [RECORD 2] which is the new registered table in 'pub2' (see > > step 3) > > has been added to the pg_subscription_rel. Based pervious discussion and > > document, that table seems shouldn't be refreshed when drop another > > publication. > > Thanks for the explanation! I understood the problem. > > I've reviewed v2 patch. Here are some comments: Regarding regression tests, since ADD/DROP PUBLICATION logic could be complicated it seems to me that we can add tests to a new file, say alter_sub_pub.pl, that includes some scenarios as I mentioned in the previous message. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Wed, Aug 4, 2021 at 9:19 PM houzj.f...@fujitsu.com wrote: > > On Wednesday, August 4, 2021 7:00 PM Masahiko Sawada > wrote > > On Wed, Aug 4, 2021 at 5:06 PM houzj.f...@fujitsu.com > > wrote: > > > > > > On Wednesday, August 4, 2021 1:47 PM Masahiko Sawada > > > > > > > > > > > I've not looked at the patch deeply yet but I think that the > > > > following one line change seems to fix the cases you reported, > > > > although I migit be missing > > > > something: > > > > > > > > diff --git a/src/backend/commands/subscriptioncmds.c > > > > b/src/backend/commands/subscriptioncmds.c > > > > index 22ae982328..c74d6d51d6 100644 > > > > --- a/src/backend/commands/subscriptioncmds.c > > > > +++ b/src/backend/commands/subscriptioncmds.c > > > > @@ -1068,7 +1068,7 @@ AlterSubscription(ParseState *pstate, > > > > AlterSubscriptionStmt *stmt, > > > > PreventInTransactionBlock(isTopLevel, "ALTER > > > > SUBSCRIPTION with refresh"); > > > > > > > > /* Only refresh the added/dropped list of > > > > publications. > > */ > > > > - sub->publications = stmt->publication; > > > > + sub->publications = publist; > > > > > > > > AlterSubscription_refresh(sub, opts.copy_data); > > > > } > > > > > > I considered the same fix before, but with the above fix, it seems > > > refresh both existsing publications and new publication, which most of > > > people didn't like in previous discussion[1]. From the doc[2], IIRC, > > > Currently, the ADD/DROP PUBLICATION is supposed to only refresh the new > > publication. > > > > What do you mean by refreshing both existing and new publications? I dropped > > and created one publication out of three publications that the subscription > > is > > subscribing to but the corresponding entries in pg_subscription_rel for > > tables > > associated with the other two publications were not changed and the table > > sync > > workers also were not started for them. > > > > + sub->publications = publist; > - sub->publications = stmt->publication; > With the above fix, When ADD/DROP PUBLICATION, I meant the table that doesn't > belong to the dropped or added publication could be updated in > pg_subscription_rel. > > I can see the effect in the following example: > > 1)-publisher- > CREATE TABLE test,test2,test3 > CREATE PUBLICATION pub FOR TABLE test; > CREATE PUBLICATION pub2 FOR TABLE test2; > 2)-subscriber- > CREATE TABLE test,test2,test3 > CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres port=1' PUBLICATION > pub,pub2; > select * from pg_subscription_rel; > -[ RECORD 1 ]- > srsubid| 16393 > srrelid| 16387 > srsubstate | r > srsublsn | 0/150A2D8 > -[ RECORD 2 ]- > srsubid| 16393 > srrelid| 16384 > srsubstate | r > srsublsn | 0/150A310 > > 3)-publisher- > alter publication pub add table test3; > 4)-subscriber- > ALTER SUBSCRIPTION sub drop PUBLICATION pub2; > select * from pg_subscription_rel; > -[ RECORD 1 ]- > srsubid| 16393 > srrelid| 16384 > srsubstate | r > srsublsn | 0/150A310 > -[ RECORD 2 ]- > srsubid| 16393 > srrelid| 16390 > srsubstate | r > srsublsn | > > I can see the [RECORD 2] which is the new registered table in 'pub2' (see > step 3) > has been added to the pg_subscription_rel. Based pervious discussion and > document, that table seems shouldn't be refreshed when drop another > publication. Thanks for the explanation! I understood the problem. I've reviewed v2 patch. Here are some comments: + if (type == ALTER_SUBSCRIPTION_SET_PUBLICATION || + type == ALTER_SUBSCRIPTION_REFRESH) + drop_table = !bsearch(, pubrel_local_oids, + list_length(pubrel_names), + sizeof(Oid), oid_cmp); + else if (type == ALTER_SUBSCRIPTION_DROP_PUBLICATION) + drop_table = bsearch(, pubrel_local_oids, +list_length(pubrel_names), +sizeof(Oid), oid_cmp); IIUC, in DROP PUBLICATION cases, we don't need to refer to the tables in the publication on the publisher that we're dropping in order to check whether to remove the relation. If we drop a table from the publication on the publisher before dropping the publication from the subscription on the subscriber, the pg_subscription_rel entry for the table remains. For example, 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 pub12 drop table t2; On subscriber: alter subscription sub drop publication pub12; select srsubid,
RE: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Wednesday, August 4, 2021 7:00 PM Masahiko Sawada wrote > On Wed, Aug 4, 2021 at 5:06 PM houzj.f...@fujitsu.com > wrote: > > > > On Wednesday, August 4, 2021 1:47 PM Masahiko Sawada > > > > > > I've not looked at the patch deeply yet but I think that the > > > following one line change seems to fix the cases you reported, > > > although I migit be missing > > > something: > > > > > > diff --git a/src/backend/commands/subscriptioncmds.c > > > b/src/backend/commands/subscriptioncmds.c > > > index 22ae982328..c74d6d51d6 100644 > > > --- a/src/backend/commands/subscriptioncmds.c > > > +++ b/src/backend/commands/subscriptioncmds.c > > > @@ -1068,7 +1068,7 @@ AlterSubscription(ParseState *pstate, > > > AlterSubscriptionStmt *stmt, > > > PreventInTransactionBlock(isTopLevel, "ALTER > > > SUBSCRIPTION with refresh"); > > > > > > /* Only refresh the added/dropped list of > > > publications. > */ > > > - sub->publications = stmt->publication; > > > + sub->publications = publist; > > > > > > AlterSubscription_refresh(sub, opts.copy_data); > > > } > > > > I considered the same fix before, but with the above fix, it seems > > refresh both existsing publications and new publication, which most of > > people didn't like in previous discussion[1]. From the doc[2], IIRC, > > Currently, the ADD/DROP PUBLICATION is supposed to only refresh the new > publication. > > What do you mean by refreshing both existing and new publications? I dropped > and created one publication out of three publications that the subscription is > subscribing to but the corresponding entries in pg_subscription_rel for tables > associated with the other two publications were not changed and the table sync > workers also were not started for them. > + sub->publications = publist; - sub->publications = stmt->publication; With the above fix, When ADD/DROP PUBLICATION, I meant the table that doesn't belong to the dropped or added publication could be updated in pg_subscription_rel. I can see the effect in the following example: 1)-publisher- CREATE TABLE test,test2,test3 CREATE PUBLICATION pub FOR TABLE test; CREATE PUBLICATION pub2 FOR TABLE test2; 2)-subscriber- CREATE TABLE test,test2,test3 CREATE SUBSCRIPTION sub CONNECTION 'dbname=postgres port=1' PUBLICATION pub,pub2; select * from pg_subscription_rel; -[ RECORD 1 ]- srsubid| 16393 srrelid| 16387 srsubstate | r srsublsn | 0/150A2D8 -[ RECORD 2 ]- srsubid| 16393 srrelid| 16384 srsubstate | r srsublsn | 0/150A310 3)-publisher- alter publication pub add table test3; 4)-subscriber- ALTER SUBSCRIPTION sub drop PUBLICATION pub2; select * from pg_subscription_rel; -[ RECORD 1 ]- srsubid| 16393 srrelid| 16384 srsubstate | r srsublsn | 0/150A310 -[ RECORD 2 ]- srsubid| 16393 srrelid| 16390 srsubstate | r srsublsn | I can see the [RECORD 2] which is the new registered table in 'pub2' (see step 3) has been added to the pg_subscription_rel. Based pervious discussion and document, that table seems shouldn't be refreshed when drop another publication. Best regards, houzj
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Wed, Aug 4, 2021 at 5:06 PM houzj.f...@fujitsu.com wrote: > > On Wednesday, August 4, 2021 1:47 PM Masahiko Sawada > > On Mon, Aug 2, 2021 at 10:52 PM houzj.f...@fujitsu.com > > wrote: > > > > > > > > > Hi hackers, > > > > > > When testing some other logical replication related patches, I found > > > two unexpected behaviours about ALTER SUBSCRIPTION ADD/DROP > > PUBLICATION. > > > > > > (1) > > > when I execute the following sqls[1], the data of tables that > > > registered to 'pub' wasn't copied to the subscriber side which means > > > tablesync worker didn't start. > > > > > > -sub originally had 2 pub nodes(pub,pub2) ALTER SUBSCRIPTION sub > > > drop PUBLICATION pub; ALTER SUBSCRIPTION sub add PUBLICATION pub; > > > - > > > > > > (2) > > > And when I execute the following sqls, the data of table registered to > > > 'pub2' > > > are synced again. > > > > > > -sub originally had 2 pub nodes(pub,pub2) ALTER SUBSCRIPTION sub > > > drop PUBLICATION pub; ALTER SUBSCRIPTION sub REFRESH PUBLICATION; > > > - > > > > I could reproduce this issue by the above steps. > > Thanks for looking into this problem. > > > > > I've not looked at the patch deeply yet but I think that the following one > > line > > change seems to fix the cases you reported, although I migit be missing > > something: > > > > diff --git a/src/backend/commands/subscriptioncmds.c > > b/src/backend/commands/subscriptioncmds.c > > index 22ae982328..c74d6d51d6 100644 > > --- a/src/backend/commands/subscriptioncmds.c > > +++ b/src/backend/commands/subscriptioncmds.c > > @@ -1068,7 +1068,7 @@ AlterSubscription(ParseState *pstate, > > AlterSubscriptionStmt *stmt, > > PreventInTransactionBlock(isTopLevel, "ALTER > > SUBSCRIPTION with refresh"); > > > > /* Only refresh the added/dropped list of publications. > > */ > > - sub->publications = stmt->publication; > > + sub->publications = publist; > > > > AlterSubscription_refresh(sub, opts.copy_data); > > } > > I considered the same fix before, but with the above fix, it seems refresh > both > existsing publications and new publication, which most of people didn't like > in > previous discussion[1]. From the doc[2], IIRC, Currently, the ADD/DROP > PUBLICATION is supposed to only refresh the new publication. What do you mean by refreshing both existing and new publications? I dropped and created one publication out of three publications that the subscription is subscribing to but the corresponding entries in pg_subscription_rel for tables associated with the other two publications were not changed and the table sync workers also were not started for them. > > > Also, I think we need to add some regression tests for this. > > Currently, subscription.sql has tests for ADD/DROP PUBLICATION but they > > don't > > check pg_subscription_rel. > > Currently, the subscription.sql doesn't have actual tables to > be subscribed which means the pg_subscription_rel is empty. I am not sure > where > to place the testcases, temporarily placed in 001_rep_changes.pl. Right. Adding tests to 001_rep_changes.pl seems good to me. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
RE: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
On Wednesday, August 4, 2021 1:47 PM Masahiko Sawada > On Mon, Aug 2, 2021 at 10:52 PM houzj.f...@fujitsu.com > wrote: > > > > > > Hi hackers, > > > > When testing some other logical replication related patches, I found > > two unexpected behaviours about ALTER SUBSCRIPTION ADD/DROP > PUBLICATION. > > > > (1) > > when I execute the following sqls[1], the data of tables that > > registered to 'pub' wasn't copied to the subscriber side which means > > tablesync worker didn't start. > > > > -sub originally had 2 pub nodes(pub,pub2) ALTER SUBSCRIPTION sub > > drop PUBLICATION pub; ALTER SUBSCRIPTION sub add PUBLICATION pub; > > - > > > > (2) > > And when I execute the following sqls, the data of table registered to > > 'pub2' > > are synced again. > > > > -sub originally had 2 pub nodes(pub,pub2) ALTER SUBSCRIPTION sub > > drop PUBLICATION pub; ALTER SUBSCRIPTION sub REFRESH PUBLICATION; > > - > > I could reproduce this issue by the above steps. Thanks for looking into this problem. > > I've not looked at the patch deeply yet but I think that the following one > line > change seems to fix the cases you reported, although I migit be missing > something: > > diff --git a/src/backend/commands/subscriptioncmds.c > b/src/backend/commands/subscriptioncmds.c > index 22ae982328..c74d6d51d6 100644 > --- a/src/backend/commands/subscriptioncmds.c > +++ b/src/backend/commands/subscriptioncmds.c > @@ -1068,7 +1068,7 @@ AlterSubscription(ParseState *pstate, > AlterSubscriptionStmt *stmt, > PreventInTransactionBlock(isTopLevel, "ALTER > SUBSCRIPTION with refresh"); > > /* Only refresh the added/dropped list of publications. */ > - sub->publications = stmt->publication; > + sub->publications = publist; > > AlterSubscription_refresh(sub, opts.copy_data); > } I considered the same fix before, but with the above fix, it seems refresh both existsing publications and new publication, which most of people didn't like in previous discussion[1]. From the doc[2], IIRC, Currently, the ADD/DROP PUBLICATION is supposed to only refresh the new publication. [1] https://www.postgresql.org/message-id/MEYP282MB166921C8622675A5C0701C31B6BB0%40MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM [2] https://www.postgresql.org/docs/14/sql-altersubscription.html By default, this command will also act like REFRESH PUBLICATION, except that in case of ADD or DROP, only the added or dropped publications are refreshed. > Also, I think we need to add some regression tests for this. > Currently, subscription.sql has tests for ADD/DROP PUBLICATION but they don't > check pg_subscription_rel. Currently, the subscription.sql doesn't have actual tables to be subscribed which means the pg_subscription_rel is empty. I am not sure where to place the testcases, temporarily placed in 001_rep_changes.pl. Best regards, houzj v2-0001-fix-ALTER-SUB-ADD-DROP-PUBLICATION.patch Description: v2-0001-fix-ALTER-SUB-ADD-DROP-PUBLICATION.patch
Re: [BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
Hi,
On Mon, Aug 2, 2021 at 10:52 PM houzj.f...@fujitsu.com
wrote:
>
>
> Hi hackers,
>
> When testing some other logical replication related patches, I found two
> unexpected behaviours about ALTER SUBSCRIPTION ADD/DROP PUBLICATION.
>
> (1)
> when I execute the following sqls[1], the data of tables that registered to
> 'pub' wasn't copied to the subscriber side which means tablesync worker didn't
> start.
>
> -sub originally had 2 pub nodes(pub,pub2)
> ALTER SUBSCRIPTION sub drop PUBLICATION pub;
> ALTER SUBSCRIPTION sub add PUBLICATION pub;
> -
>
> (2)
> And when I execute the following sqls, the data of table registered to 'pub2'
> are synced again.
>
> -sub originally had 2 pub nodes(pub,pub2)
> ALTER SUBSCRIPTION sub drop PUBLICATION pub;
> ALTER SUBSCRIPTION sub REFRESH PUBLICATION;
> -
I could reproduce this issue by the above steps.
>
> After looking into this problem, I think the reason is the
> [alter sub add/drop publication] misused the function
> AlterSubscription_refresh().
>
> For DROP cases:
>
> Currently, in function AlterSubscription_refresh(), it will first fetch the
> target tables from the target publication, and also fetch the tables in
> subscriber side from pg_subscription_rel. Then it will check each table from
> local pg_subscription_rel, if the table does not exists in the tables fetched
> from the target publication then drop it.
>
> The logic above only works for SET PUBLICATION. However, When DROP
> PUBLICATION,
> the tables fetched from target publication is actually the tables that need to
> be dropped. If reuse the above logic, it will drop the wrong table which
> result
> in unexpected behavioud in (1) and (2).(ADD PUBLICATION have similar problem).
Yes. For example, suppose that the publisher has two publications pub1
and pub2 for table1 and table2, respecitively. And we create a
subscription subscribing to those two publications.
postgres(1:74454)=# \dRs sub
List of subscriptions
Name | Owner | Enabled | Publication
--+--+-+-
sub | masahiko | t | {pub1,pub2}
(1 row)
postgres(1:74454)=# select srsubid, srrelid::regclass::text,
srsubstate, srsublsn from pg_subscription_rel;
srsubid | srrelid | srsubstate | srsublsn
-+-++---
16394 | table1 | r | 0/170F388
16394 | table2 | r | 0/170F388
(2 rows)
After dropping pub1 from the subscription, 'table2' associated with
'pub2' is removed:
postgres(1:74454)=# alter subscription sub drop publication pub1;
ALTER SUBSCRIPTION
postgres(1:74454)=# select srsubid, srrelid::regclass::text,
srsubstate, srsublsn from pg_subscription_rel;
srsubid | srrelid | srsubstate | srsublsn
-+-++---
16394 | table1 | r | 0/170F388
(1 row)
> So, I think we'd better fix this problem. I tried add some additional check in
> AlterSubscription_refresh() which can avoid the problem like the attached
> patch. Not sure do we need to further refactor.
I've not looked at the patch deeply yet but I think that the following
one line change seems to fix the cases you reported, although I migit
be missing something:
diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index 22ae982328..c74d6d51d6 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1068,7 +1068,7 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
PreventInTransactionBlock(isTopLevel, "ALTER
SUBSCRIPTION with refresh");
/* Only refresh the added/dropped list of publications. */
- sub->publications = stmt->publication;
+ sub->publications = publist;
AlterSubscription_refresh(sub, opts.copy_data);
}
Also, I think we need to add some regression tests for this.
Currently, subscription.sql has tests for ADD/DROP PUBLICATION but
they don't check pg_subscription_rel.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
[BUG] wrong refresh when ALTER SUBSCRIPTION ADD/DROP PUBLICATION
Hi hackers, When testing some other logical replication related patches, I found two unexpected behaviours about ALTER SUBSCRIPTION ADD/DROP PUBLICATION. (1) when I execute the following sqls[1], the data of tables that registered to 'pub' wasn't copied to the subscriber side which means tablesync worker didn't start. -sub originally had 2 pub nodes(pub,pub2) ALTER SUBSCRIPTION sub drop PUBLICATION pub; ALTER SUBSCRIPTION sub add PUBLICATION pub; - (2) And when I execute the following sqls, the data of table registered to 'pub2' are synced again. -sub originally had 2 pub nodes(pub,pub2) ALTER SUBSCRIPTION sub drop PUBLICATION pub; ALTER SUBSCRIPTION sub REFRESH PUBLICATION; - After looking into this problem, I think the reason is the [alter sub add/drop publication] misused the function AlterSubscription_refresh(). For DROP cases: Currently, in function AlterSubscription_refresh(), it will first fetch the target tables from the target publication, and also fetch the tables in subscriber side from pg_subscription_rel. Then it will check each table from local pg_subscription_rel, if the table does not exists in the tables fetched from the target publication then drop it. The logic above only works for SET PUBLICATION. However, When DROP PUBLICATION, the tables fetched from target publication is actually the tables that need to be dropped. If reuse the above logic, it will drop the wrong table which result in unexpected behavioud in (1) and (2).(ADD PUBLICATION have similar problem). So, I think we'd better fix this problem. I tried add some additional check in AlterSubscription_refresh() which can avoid the problem like the attached patch. Not sure do we need to further refactor. Best regards, houzj 0001-fix-ALTER-SUB-ADD-DROP-PUBLICATION.patch Description: 0001-fix-ALTER-SUB-ADD-DROP-PUBLICATION.patch
