MIME-Version: 1.0
References: 
 <CAAD30ULCxqOJp0sffm_y9jNC4BVPYv7Q7_va_JE8qyfRXkfu+g@mail.gmail.com>
 <CAFPTHDaaewvYUznZD1YjUQnvHycgvgMKNK4w=V8Q-8MKTpVDrw@mail.gmail.com>
 <OS3PR01MB6275025C397CC22E446B1F589EBC9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <OS0PR01MB5716765B2D38786DA3943E0294809@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAFPTHDbAJPccMcZnOraBy14hM6qBJxYqcRwK6iV6=gtL6VZQwQ@mail.gmail.com>
 <CALDaNm3NUO8ofK64N7HMtNmUP=52R8_jWzrekqAm7m7wqZjwaQ@mail.gmail.com>
 <CALDaNm3XUKfD+nD1AVvSuZyUY_zRk_eyz+Pt9t13N8WXViR6pw@mail.gmail.com>
 <3032112.1679865718@sss.pgh.pa.us>
 <CAA4eK1K3VXfTWXbLADcH81J==7ussvNdqLFHN68sEokDPueu7w@mail.gmail.com>
 <CAA4eK1++Y7a2SQq55DXT6neghZgj3j+pQ74=8zfT3k8Tkdj0ZA@mail.gmail.com>
 <OS0PR01MB571638B29E37EB9A48507DAE94889@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB571636F0F1307AB1A868B06A948E9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB57169AB2355A90C9FA87DE75948E9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CALDaNm2vBN8oMv-7G=DH5rR-u40JGbR9aP4B6nwr71qw17rPFA@mail.gmail.com>
 <OS0PR01MB571614EE7EEDBF7049DA4EAE94939@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAJpy0uCSCtWwb_LyjXQT65c-UksYhp-4U0QpimtEho_wxjzkog@mail.gmail.com>
 <OS0PR01MB57168840ABFAB9E9BB28A54B94969@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB571643602A8B7226B558DBAA94969@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAA4eK1LyEqEq0XCeCsd9ZmTHyeshHbv=BiRTD=1mmk+YtxwpkQ@mail.gmail.com>
 <OS0PR01MB5716A9E1433DA1FD3EE38069949C9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAJpy0uDb2mDJtLNFXzUY4911qRZOvj6Q8pu4xFh4BMYBeOSPow@mail.gmail.com>
 <CAJpy0uAA0SQ0kPA5bXmrW=32p0bwFCifoKb5OSgteTjGggEkLA@mail.gmail.com>
In-Reply-To: 
 <CAJpy0uAA0SQ0kPA5bXmrW=32p0bwFCifoKb5OSgteTjGggEkLA@mail.gmail.com>
From: shveta malik <shveta.malik@gmail.com>
Date: Thu, 20 Apr 2023 18:09:21 +0530
Message-ID: 
 <CAJpy0uB7f2GxPNor5iTT-30JuD-p-gvnsMZG9tiiHN+DHJj0RQ@mail.gmail.com>
Subject: Re: Support logical replication of DDLs
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, vignesh C <vignesh21@gmail.com>,
	Ajin Cherian <itsajin@gmail.com>,
 "Wei Wang (Fujitsu)" <wangw.fnst@fujitsu.com>,
	Runqi Tian <runqidev@gmail.com>, Peter Smith <smithpb2250@gmail.com>,
 Tom Lane <tgl@sss.pgh.pa.us>,
	li jie <ggysxcq@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
 Masahiko Sawada <sawada.mshk@gmail.com>,
	Japin Li <japinli@hotmail.com>, rajesh singarapu <rajesh.rs0541@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Zheng Li <zhengli10@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAJpy0uB7f2GxPNor5iTT-30JuD-p-gvnsMZG9tiiHN%2BDHJj0RQ%40mail.gmail.com>
Precedence: bulk

On Thu, Apr 20, 2023 at 2:28=E2=80=AFPM shveta malik <shveta.malik@gmail.co=
m> wrote:
>
> On Thu, Apr 20, 2023 at 9:11=E2=80=AFAM shveta malik <shveta.malik@gmail.=
com> wrote:
>>
>> On Mon, Apr 17, 2023 at 5:32=E2=80=AFPM Zhijie Hou (Fujitsu)
>> <houzj.fnst@fujitsu.com> wrote:
>> >
>> > Attach the new version patch set which include the following changes:
>> Few comments for ddl_deparse.c in patch dated April17:
>>
>  Few comments for ddl_json.c in the patch dated April17:
>

Few more comments, mainly for event_trigger.c  in the patch dated April17:

1)EventTriggerCommonSetup()
+    pub_funcoid =3D LookupFuncName(pubfuncname, 0, NULL, true);

This is the code where we have special handling for ddl-triggers. Here
we are passing 'missing_ok' as true, so shouldn't we check pub_funcoid
against 'InvalidOid' ?


2) EventTriggerTableInitWriteEnd()

+ if (stmt->objtype =3D=3D OBJECT_TABLE)
+ {
+    parent =3D currentEventTriggerState->currentCommand->parent;
+    pfree(currentEventTriggerState->currentCommand);
+    currentEventTriggerState->currentCommand =3D parent;
+ }
+ else
+ {
+    MemoryContext oldcxt;
+    oldcxt =3D MemoryContextSwitchTo(currentEventTriggerState->cxt);
+    currentEventTriggerState->currentCommand->d.simple.address =3D address=
;
+    currentEventTriggerState->commandList =3D
+         lappend(currentEventTriggerState->commandList,
+     currentEventTriggerState->currentCommand);
+
+     MemoryContextSwitchTo(oldcxt);
+ }
+}

It will be good to add some comments in the 'else' part. It is not
very much clear what exactly we are doing here and for which scenario.


3) EventTriggerTableInitWrite()

+ if (!currentEventTriggerState)
+     return;
+
+ /* Do nothing if no command was collected. */
+ if (!currentEventTriggerState->currentCommand)
+      return;

Is it possible that when we reach here no command is collected yet? I
think this can happen only for the case when
commandCollectionInhibited is true. If so, above can be modified to:

        if (!currentEventTriggerState ||
                currentEventTriggerState->commandCollectionInhibited)
                return;
        Assert(currentEventTriggerState->currentCommand !=3D NULL);

This will make the behaviour and checks consistent across similar
functions in this file.


4) EventTriggerTableInitWriteEnd()
Here as well, we can have the same assert as below:
 Assert(currentEventTriggerState->currentCommand !=3D NULL);
'currentEventTriggerState' and 'commandCollectionInhibited' checks are
already present.

5) logical_replication.sgml:
 +  'This is especially useful if you have lots schema changes over
time that need replication.'

 lots schema --> lots of schema

thanks
Shveta