Re: Handle infinite recursion in logical replication setup
От | vignesh C |
---|---|
Тема | Re: Handle infinite recursion in logical replication setup |
Дата | |
Msg-id | CALDaNm2-D860yULtcmZAzDbdiof-Dg6Y_YaY4owbO6Rj=XEHMw@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Handle infinite recursion in logical replication setup (Peter Smith <smithpb2250@gmail.com>) |
Ответы |
Re: Handle infinite recursion in logical replication setup
(Amit Kapila <amit.kapila16@gmail.com>)
|
Список | pgsql-hackers |
On Mon, Jul 11, 2022 at 9:11 AM Peter Smith <smithpb2250@gmail.com> wrote: > > Here are my review comments for the v30* patches: > > ======== > v30-0001 > ======== > > 1.1 <general> > > I was wondering if it is better to implement a new defGetOrigin method > now instead of just using the defGetString to process the 'origin', > since you may need to do that in future anyway if the 'origin' name is > planned to become specifiable by the user. OTOH maybe you prefer to > change this code later when the time comes. I am not sure what way is > best. I preferred to do that change when the feature is getting extended. > ~~~ > > 1.2. src/include/replication/walreceiver.h > > @@ -183,6 +183,8 @@ typedef struct > bool streaming; /* Streaming of large transactions */ > bool twophase; /* Streaming of two-phase transactions at > * prepare time */ > + char *origin; /* Only publish data originating from the > + * specified origin */ > } logical; > } proto; > } WalRcvStreamOptions; > > Should the new comments be aligned with the other ones? I kept it like this as pgindent also is aligning it as the current code. > ======== > v30-0002 > ======== > > 2.1 doc/src/sgml/ref/alter_subscription.sgml > > + <para> > + Refer <xref linkend="sql-createsubscription-notes"/> for the usage of > + <literal>force</literal> for <literal>copy_data</literal> parameter > + and its interaction with the <literal>origin</literal> parameter. > </para> > > IMO it's better to say "Refer to the Notes" or "Refer to CREATE > SUBSCRIPTION Notes" instead of just "Refer Notes" Modified to "Refer to the Notes" > ~~~ > > 2.2 doc/src/sgml/ref/create_subscription.sgml > > 2.2.a > + <para> > + Refer <xref linkend="sql-createsubscription-notes"/> for the usage of > + <literal>force</literal> for <literal>copy_data</literal> parameter > + and its interaction with the <literal>origin</literal> parameter. > + </para> > > IMO it's better to say "Refer to the Notes" (same as other xref on > this page) instead of "Refer Notes" Modified to "Refer to the Notes" > 2.2.b > @@ -316,6 +324,11 @@ CREATE SUBSCRIPTION <replaceable > class="parameter">subscription_name</replaceabl > publisher sends changes regardless of their origin. The default is > <literal>any</literal>. > </para> > + <para> > + Refer <xref linkend="sql-createsubscription-notes"/> for the usage of > + <literal>force</literal> for <literal>copy_data</literal> parameter > + and its interaction with the <literal>origin</literal> parameter. > + </para> > > Ditto Modified to "Refer to the Notes" > ~~~ > > 2.3 src/backend/commands/subscriptioncmds.c - DefGetCopyData > > +/* > + * Validate the value specified for copy_data parameter. > + */ > +static CopyData > +DefGetCopyData(DefElem *def) Changed it to defGetCopyData to keep the naming similar to others > ~~~ > > 2.4 > > + /* > + * If no parameter given, assume "true" is meant. > + */ > > Please modify the comment to match the recent push [1]. Modified > ~~~ > > 2.5 src/test/subscription/t/032_localonly.pl > > 2.5.a > +# Check Alter subscription ... refresh publication when there is a new > +# table that is subscribing data from a different publication > +$node_A->safe_psql('postgres', "CREATE TABLE tab_full1 (a int PRIMARY KEY)"); > +$node_B->safe_psql('postgres', "CREATE TABLE tab_full1 (a int PRIMARY KEY)"); > > Unfortunately, I think tab_full1 is a terrible table name because in > my screen font the 'l' and the '1' look exactly the same so it just > looks like a typo. Maybe change it to "tab_new" or something? Modified to tab_new > 2.5b > What exactly is the purpose of "full" in all these test table names? > AFAIK "full" is just some name baggage inherited from completely > different tests which were doing full versus partial table > replication. I'm not sure it is relevant here. Modified to tab Thanks for the comments, the v31 patch attached has the changes for the same. Regards, Vignesh
Вложения
В списке pgsql-hackers по дате отправления: