On Tue, 6 Sept 2022 at 15:25, Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Sep 6, 2022 at 9:31 AM wangw.f...@fujitsu.com > <wangw.f...@fujitsu.com> wrote: > > > > On Tues, 6 Sept 2022 at 11:14, vignesh C <vignes...@gmail.com> wrote: > > > Thanks for the comments, the attached patch has the changes for the same. > > > > Thanks for updating the patch. > > > > Here is one comment for 0001 patch. > > 1. The query in function check_publications_origin. > > + appendStringInfoString(&cmd, > > + "SELECT DISTINCT > > P.pubname AS pubname\n" > > + "FROM pg_publication > > P,\n" > > + " LATERAL > > pg_get_publication_tables(P.pubname) GPT\n" > > + " LEFT JOIN > > pg_subscription_rel PS ON (GPT.relid = PS.srrelid),\n" > > + " pg_class C JOIN > > pg_namespace N ON (N.oid = C.relnamespace)\n" > > + "WHERE C.oid = GPT.relid > > AND PS.srrelid IS NOT NULL AND P.pubname IN ("); > > > > Since I found that we only use "PS.srrelid" in the WHERE statement by > > specifying "PS.srrelid IS NOT NULL", could we just use "[INNER] JOIN" to > > join > > the table pg_subscription_rel? > > > > I also think we can use INNER JOIN here but maybe there is a reason > why that is not used in the first place. If we change it in the code > then also let's change the same in the docs section as well.
Modified > Few minor points: > =============== > 1. > This scenario is detected and a WARNING is logged to the > + user, but the warning is only an indication of a potential problem; it is > + the user responsibility to make the necessary checks to ensure the copied > + data origins are really as wanted or not. > + </para> > > /user/user's Modified > 2. How about a commit message like: > Raise a warning if there is a possibility of data from multiple origins. > > This commit raises a warning message for a combination of options > ('copy_data = true' and 'origin = none') during CREATE/ALTER subscription > operations if the publication tables were also replicated from other > publishers. > > During replication, we can skip the data from other origins as we have that > information in WAL but that is not possible during initial sync so we raise > a warning if there is such a possibility. Yes, this looks good. Modified Thanks for the comment, the v47 patch attached at [1] has the changes for the same. [1] - https://www.postgresql.org/message-id/CALDaNm33T%3D23P-GWvy3O7cT1BfHuGV8dosAw1AVLf40MPvg2bg%40mail.gmail.com Regards, Vignesh