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
