On Monday, May 9, 2022 10:51 AM wangw.fnst@fujitsu.com <wangw.fnst@fujitsu.com> 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.
(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".
(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.
(3) free of allocated memory
In the pg_get_publication_tables(),
we don't free 'elems'. Don't we need it ?
(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.
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.
Best Regards,
Takamichi Osumi
