On Thur, May 12, 2022 9:48 AM [email protected] <[email protected]> wrote: > On Wednesday, May 11, 2022 11:33 AM I wrote: > > On Monday, May 9, 2022 10:51 AM [email protected] > > <[email protected]> wrote: > > > Attach new patches. > > > The patch for HEAD: > > > 1. Modify the approach. Enhance the API of function > > > pg_get_publication_tables to handle one publication or an array of > > > publications. > > > The patch for REL14: > > > 1. Improve the table sync SQL. [suggestions by Shi yu] > > Hi, thank you for updating the patch ! > > > > Minor comments on your patch for HEAD v2. Thanks for your comments.
> > (1) commit message sentence
> >
> > I suggest below sentence.
> >
> > Kindly change from
> > "... when subscribing to both publications using one subscription, the data
> > is
> > replicated twice in inital copy"
> > to "subscribing to both publications from one subscription causes initial
> > copy
> > twice".
Improve it according to your suggestion.
> > (2) unused variable
> >
> > pg_publication.c: In function ‘pg_get_publication_tables’:
> > pg_publication.c:1091:11: warning: unused variable ‘pubname’
> > [-Wunused-variable]
> > char *pubname;
> >
> > We can remove this.
Fix it.
> > (3) free of allocated memory
> >
> > In the pg_get_publication_tables(),
> > we don't free 'elems'. Don't we need it ?
Improve it according to your suggestion. Free 'elems'.
> > (4) some coding alignments
> >
> > 4-1.
> >
> > + List *tables_viaroot = NIL,
> > ...
> > + *current_table = NIL;
> >
> > I suggest we can put some variables
> > into the condition for the first time call of this function, like
> > tables_viaroot and
> > current_table.
> > When you agree, kindly change it.
Improve these according to your suggestions.
Also, I put the code for getting publication(s) into the condition for the
first time call of this function.
> > 4-2.
> >
> > + /*
> > + * Publications support partitioned tables, although
> > all changes
> > + * are replicated using leaf partition identity and
> > schema, so we
> > + * only need those.
> > + */
> > + if (publication->alltables)
> > + {
> > + current_table =
> > GetAllTablesPublicationRelations(publication->pubviaroot);
> > + }
> >
> > This is not related to the change itself and now we are inheriting the
> > previous
> > curly brackets, but I think there's no harm in removing it, since it's only
> > for one
> > statement.
Improve these according to your suggestions.
> Hi,
>
> One more thing I'd like to add is that
> we don't hit the below code by tests.
> In the HEAD v2, we add a new filtering logic in pg_get_publication_tables.
> Although I'm not sure if this is related to the bug fix itself,
> when we want to include it in this patch, then
> I feel it's better to add some simple test for this part,
> to cover all the new main paths and check if
> new logic works correctly.
>
>
> + /*
> + * If a partition table is published in a publication with
> viaroot,
> + * and its parent or child table is published in another
> publication
> + * without viaroot. Then we need to move the parent or child
> table
> + * from tables to tables_viaroot.
> + *
> + * If all publication(s)'s viaroot are the same, then skip
> this part.
> + */
>
> ....
> if (ancestor_viaroot == ancestor)
> + {
> + tables =
> foreach_delete_current(tables, lc2);
> + change_tables =
> list_append_unique_oid(change_tables,
> +
> relid);
> + }
Yes, I agree.
But when I was adding the test, I found we could improve this part.
So, I removed this part of the code.
Also rebase it because the change in HEAD(23e7b38).
Attach the patches.(Only changed the patch for HEAD.).
1. Improve the commit message. [suggestions by Osumi-san]
2. Improve coding alignments and the usage for SRFs. [suggestions by Osumi-san
and I]
3. Simplify the modifications in function pg_get_publication_tables.
Regards,
Wang wei
HEAD_v3-0001-Fix-data-replicated-twice-when-specifying-PUBLISH.patch
Description: HEAD_v3-0001-Fix-data-replicated-twice-when-specifying-PUBLISH.patch
REL14_v4-0001-Fix-data-replicated-twice-when-specifying-PUBLISH.patch
Description: REL14_v4-0001-Fix-data-replicated-twice-when-specifying-PUBLISH.patch
