On Fri, Jul 29, 2022 at 10:51 AM vignesh C <vignes...@gmail.com> wrote: > > On Fri, Jul 29, 2022 at 8:31 AM Peter Smith <smithpb2...@gmail.com> wrote: > > > > Here are some comments for the patch v40-0001: > > > > ====== > > > > 1. Commit message > > > > It might be better to always use 'copy_data = true' in favour of > > 'copy_data = on' just for consistency with all the docs and the error > > messages. > > > > ====== > > Modified > > > 2. doc/src/sgml/ref/create_subscription.sgml > > > > @@ -386,6 +401,15 @@ CREATE SUBSCRIPTION <replaceable > > class="parameter">subscription_name</replaceabl > > can have non-existent publications. > > </para> > > > > + <para> > > + If the subscription is created with <literal>origin = NONE</literal> and > > + <literal>copy_data = true</literal>, it will check if the publisher has > > + subscribed to the same table from other publishers and, if so, throw an > > + error to prevent possible non-local data from being copied. The user can > > + override this check and continue with the copy operation by specifying > > + <literal>copy_data = force</literal>. > > + </para> > > > > 2a. > > It is interesting that you changed the note to say origin = NONE. > > Personally, I prefer it written as you did, but I think maybe this > > change does not belong in this patch. The suggestion for changing from > > "none" to NONE is being discussed elsewhere in this thread and > > probably all such changes should be done together (if at all) as a > > separate patch. Until then I think this patch 0001 should just stay > > consistent with whatever is already pushed on HEAD. > > Modified > > > 2b. > > "possible no-local data". Maybe the terminology "local/non-local" is a > > hangover from back when the subscription parameter was called local > > instead of origin. I'm not sure if you want to change this or not, and > > anyway I didn't have any better suggestions – so this comment is just > > to bring it to your attention. > > > > ====== > > Modified > > > 3. src/backend/commands/subscriptioncmds.c - DefGetCopyData > > > > + /* > > + * The set of strings accepted here should match up with the > > + * grammar's opt_boolean_or_string production. > > + */ > > + if (pg_strcasecmp(sval, "false") == 0 || > > + pg_strcasecmp(sval, "off") == 0) > > + return COPY_DATA_OFF; > > + if (pg_strcasecmp(sval, "true") == 0 || > > + pg_strcasecmp(sval, "on") == 0) > > + return COPY_DATA_ON; > > + if (pg_strcasecmp(sval, "force") == 0) > > + return COPY_DATA_FORCE; > > > > I understand the intention of the comment, but it is not strictly > > correct to say "should match up" because "force" is a new value. > > Perhaps the comment should be as suggested below. > > > > SUGGESTION > > The set of strings accepted here must include all those accepted by > > the grammar's opt_boolean_or_string production. > > > > ~~ > > Modified > > > > > 4. src/backend/commands/subscriptioncmds.c - check_pub_table_subscribed > > > > @@ -1781,6 +1858,122 @@ AlterSubscriptionOwner_oid(Oid subid, Oid > > newOwnerId) > > table_close(rel, RowExclusiveLock); > > } > > > > +/* > > + * Check and throw an error if the publisher has subscribed to the same > > table > > + * from some other publisher. This check is required only if "copy_data = > > on" > > + * and "origin = NONE" for CREATE SUBSCRIPTION and > > + * ALTER SUBSCRIPTION ... REFRESH statements to avoid the publisher from > > + * replicating data that has an origin. > > + * > > + * This check need not be performed on the tables that are already added as > > + * incremental sync for such tables will happen through WAL and the origin > > of > > + * the data can be identified from the WAL records. > > + * > > + * subrel_local_oids contains the list of relation oids that are already > > + * present on the subscriber. > > + */ > > +static void > > +check_pub_table_subscribed(WalReceiverConn *wrconn, List *publications, > > + CopyData copydata, char *origin, > > + Oid *subrel_local_oids, int subrel_count) > > > > 4a. > > "copy_data = on" -> "copy_data = true" (for consistency with the docs > > and the error messages) > > Modified > > > 4b. > > The same NONE/none review comment from #2a applies here too. Probably > > it should be written as none for now unless/until *everything* changes > > to NONE. > > Modified > > > 4c. > > "to avoid the publisher from replicating data that has an origin." -> > > "to avoid replicating data that has an origin." > > Modified > > > 4d. > > + * This check need not be performed on the tables that are already added as > > + * incremental sync for such tables will happen through WAL and the origin > > of > > + * the data can be identified from the WAL records. > > > > SUGGESTION (maybe?) > > This check need not be performed on the tables that are already added > > because incremental sync for those tables will happen through WAL and > > the origin of the data can be identified from the WAL records. > > > > ====== > > Modified > > > > > 5. src/test/subscription/t/030_origin.pl > > > > + "Refresh publication when the publisher has subscribed for the new table" > > > > SUGGESTION (Just to mention origin = none somehow. Maybe you can > > reword it better than this) > > Refresh publication when the publisher has subscribed for the new > > table, but the subscriber-side wants origin=none > > Modified > > Thanks for the comments, the attached v41 patch has the changes for the same.
The patch does not apply on Head because of the commit "0c20dd33db1607d6a85ffce24238c1e55e384b49", I have attached a rebased v41 patch on top of the HEAD. I have not yet done the changes to change the error to warning in the first patch, I will change it in the next version once it is concluded. Regards, Vignesh
v41-0001-Check-and-throw-an-error-if-publication-tables-w.patch
Description: Binary data
v41-0002-Document-the-steps-for-replication-between-prima.patch
Description: Binary data
