On Mon, Jul 25, 2022 at 12:58 PM Peter Smith <smithpb2...@gmail.com> wrote: > > Firstly, I have some (case-sensitivity) questions about the previous > patch which was already pushed [1]. > > Q1. create_subscription docs > > I did not understand why the docs refer to slot_name = NONE, yet the > newly added option says origin = none/any. I think that saying origin > = NONE/ANY would be more consistent with the existing usage of NONE in > this documentation. > > ~~~ > > Q2. parse_subscription_options > > Similarly, in the code (parse_subscription_options), I did not > understand why the checks for special name values are implemented > differently: > > The new 'origin' code is using pg_strcmpcase to check special values > (none/any), and the old 'slot_name' code uses case-sensitive strcmp to > check the special value (none). > > FWIW, here I thought the new origin code is the correct one. > > ====== > > Now, here are some review comments for the patch v38-0001: > > 1. src/backend/commands/subscriptioncmds.c - check_pub_table_subscribed > > @@ -1781,6 +1858,121 @@ 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 copydata is ON > and > + * the origin is local for CREATE SUBSCRIPTION and > + * ALTER SUBSCRIPTION ... REFRESH statements to avoid replicating remote data > + * from the publisher. > + * > + * 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) > > 1a. > "copydata is ON" --> "copy_data = on" (because the comment is talking > about the CREATE/ALTER statements, so it seemed a bit confusing to > refer to the copydata function param instead of the copy_data > subscription parameter)
Modified > 1b. > "the origin is local" ?? But, "local" was the old special name value. > Now it is "none", so I think this part needs minor rewording. Modified > > ~~~ > > 2. > > + if (copydata != COPY_DATA_ON || !origin || > + (pg_strcasecmp(origin, "none") != 0)) > + return; > > Should this be using the constant LOGICALREP_ORIGIN_NONE? Modified > ~~~ > > 3. > > + /* > + * Throw an error if the publisher has subscribed to the same table > + * from some other publisher. We cannot differentiate between the > + * origin and non-origin data that is present in the HEAP during the > + * initial sync. Identification of non-origin data can be done only > + * from the WAL by using the origin id. > + * > + * XXX: For simplicity, we don't check whether the table has any data > + * or not. If the table doesn't have any data then we don't need to > + * distinguish between local and non-local data so we can avoid > + * throwing an error in that case. > + */ > > 3a. > When the special origin value changed from "local" to "none" this > comment's first part seems to have got a bit lost in translation. Modified > SUGGESTION: > Throw an error if the publisher has subscribed to the same table from > some other publisher. We cannot know the origin of data during the > initial sync. Data origins can be found only from the WAL by looking > at the origin id. Modified > 3b. > I think referring to "local and non-local" data in the XXX part of > this comment also needs some minor rewording now that "local" is not a > special origin name anymore. Modified Thanks for the comments, the v39 patch shared at [1] has the changes for the same. [1] - https://www.postgresql.org/message-id/CALDaNm2POATc_jwQ-8MBJgGCVZGdUNhnTv8zkBuGzLaY03dM%3DA%40mail.gmail.com Regards, Vignesh
