From: | shveta malik <shveta(dot)malik(at)gmail(dot)com> |
---|---|
To: | "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Ajin Cherian <itsajin(at)gmail(dot)com>, "Wei Wang (Fujitsu)" <wangw(dot)fnst(at)fujitsu(dot)com>, Runqi Tian <runqidev(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, li jie <ggysxcq(at)gmail(dot)com>, Dilip Kumar <dilipbalaut(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Japin Li <japinli(at)hotmail(dot)com>, rajesh singarapu <rajesh(dot)rs0541(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Zheng Li <zhengli10(at)gmail(dot)com> |
Subject: | Re: Support logical replication of DDLs |
Date: | 2023-04-20 12:39:21 |
Message-ID: | CAJpy0uB7f2GxPNor5iTT-30JuD-p-gvnsMZG9tiiHN+DHJj0RQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-general pgsql-hackers |
On Thu, Apr 20, 2023 at 2:28 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Thu, Apr 20, 2023 at 9:11 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>>
>> On Mon, Apr 17, 2023 at 5:32 PM Zhijie Hou (Fujitsu)
>> <houzj(dot)fnst(at)fujitsu(dot)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 = 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 == OBJECT_TABLE)
+ {
+ parent = currentEventTriggerState->currentCommand->parent;
+ pfree(currentEventTriggerState->currentCommand);
+ currentEventTriggerState->currentCommand = parent;
+ }
+ else
+ {
+ MemoryContext oldcxt;
+ oldcxt = MemoryContextSwitchTo(currentEventTriggerState->cxt);
+ currentEventTriggerState->currentCommand->d.simple.address = address;
+ currentEventTriggerState->commandList =
+ 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 != 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 != 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
From | Date | Subject | |
---|---|---|---|
Next Message | Bryn Llewellyn | 2023-04-20 16:17:49 | Re: What happened to the tip "It is good practice to create a role that has the CREATEDB and CREATEROLE privileges..." |
Previous Message | Amit Kapila | 2023-04-20 10:10:44 | Re: Support logical replication of DDLs |
From | Date | Subject | |
---|---|---|---|
Next Message | David Rowley | 2023-04-20 12:40:48 | Re: Should we put command options in alphabetical order in the doc? |
Previous Message | Melanie Plageman | 2023-04-20 12:37:52 | Re: Should we put command options in alphabetical order in the doc? |