On Tue, Jul 5, 2022 at 9:33 PM vignesh C <vignes...@gmail.com> wrote: > > Since the existing test is already handling the verification of this > scenario, I felt no need to add the test. Updated v29 patch removes > the 0001 patch which had the test case. >
I am not able to apply 0001. patching file src/bin/psql/tab-complete.c Hunk #1 FAILED at 1873. Hunk #2 FAILED at 3152. Few comments on 0002 ===================== 1. + <xref linkend="sql-createsubscription-notes" /> for interaction Is there a need for space before / in above? If not, please remove it with similar extra space from other similar usages. 2. + <para> + There is some interaction between the <literal>origin</literal> + parameter and the <literal>copy_data</literal> parameter. Refer to + the <command>CREATE SUBSCRIPTION</command> + <xref linkend="sql-createsubscription-notes" /> for interaction + details and usage of <literal>force</literal> for + <literal>copy_data</literal> parameter. </para> I think this is bit long. We can try to reduce this by something like: Refer <xref linkend="sql-createsubscription-notes"/> for the usage of <literal>force</literal> option and its interaction with the <literal>origin</literal> parameter. Also, adopt the same other places if you agree with the above change. 4. @@ -601,16 +549,28 @@ GetSubscriptionNotReadyRelations(Oid subid) SysScanDesc scan; rel = table_open(SubscriptionRelRelationId, AccessShareLock); - ScanKeyInit(&skey[nkeys++], Anum_pg_subscription_rel_srsubid, Spurious line removal. 5. +static void +check_pub_table_subscribed(WalReceiverConn *wrconn, List *publications, + CopyData copydata, char *origin, Oid subid) { ... ... + /* + * Get the ready relations for the subscription. The subid will be valid + * only for ALTER SUBSCRIPTION ... REFRESH because there will be no + * relations in ready state while the subscription is created. + */ + if (OidIsValid(subid)) + subreadyrels = GetSubscriptionRelations(subid, READY_STATE); Why do we want to consider only READY_STATE relations here? If you see its caller AlterSubscription_refresh(), we don't consider copying the relation if it exists on subscribers in any state. If my observation is correct then you probably don't need to introduce SubRelStateType. -- With Regards, Amit Kapila.