public inbox for pgsql-general@postgresql.org
help / color / mirror / Atom feedFrom: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
To: shveta malik <shveta.malik@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>
Cc: Ajin Cherian <itsajin@gmail.com>
Cc: Wei Wang (Fujitsu) <wangw.fnst@fujitsu.com>
Cc: Runqi Tian <runqidev@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: li jie <ggysxcq@gmail.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Japin Li <japinli@hotmail.com>
Cc: rajesh singarapu <rajesh.rs0541@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Zheng Li <zhengli10@gmail.com>
Subject: RE: Support logical replication of DDLs
Date: Tue, 25 Apr 2023 03:57:59 +0000
Message-ID: <OS0PR01MB57167AB450D6B3C5FF2C90EF94649@OS0PR01MB5716.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAJpy0uB7f2GxPNor5iTT-30JuD-p-gvnsMZG9tiiHN+DHJj0RQ@mail.gmail.com>
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>
<CAJpy0uB7f2GxPNor5iTT-30JuD-p-gvnsMZG9tiiHN+DHJj0RQ@mail.gmail.com>
On Thursday, April 20, 2023 8:39 PM shveta malik <shveta.malik@gmail.com> wrote:
> On Thu, Apr 20, 2023 at 2:28 PM shveta malik <shveta.malik@gmail.com>
> wrote:
> >
> > On Thu, Apr 20, 2023 at 9:11 AM shveta malik <shveta.malik@gmail.com>
> wrote:
> >>
> >> On Mon, Apr 17, 2023 at 5:32 PM 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:
> >
Comments from [1]
> 1) append_format_string()
> I think we need to have 'Assert(sub_fmt)' here like we have it in all
> other similar functions (append_bool_object, append_object_object,
> ...)
Added.
>
> 2) append_object_to_format_string()
> here we have code piece :
> if (sub_fmt == NULL || tree->fmtinfo == NULL)
> return sub_fmt;
> but sub_fmt will never be null when we reach this function as all its
> callers assert for null sub_fmt. So that means when tree->fmtinfo is
> null, we end up returning sub_fmt as it is, instead of extracting
> object name from that. Is this intended?
No, removed this check and added an Assert for tree->fmtinfo as the caller
should not pass a NULL fmtinfo when using this function.
>
> 3) We can remove extra spaces after full-stop in the comment below
> /*
> * Deparse a ColumnDef node within a typed table creation. This is simpler
> * than the regular case, because we don't have to emit the type declaration,
> * collation, or default. Here we only return something if the column is being
> * declared NOT NULL.
> ...
> deparse_ColumnDef_typed()
Removed.
>
> 5) deparse_AlterRelation()
> We have below variable initialized to false in the beginning
> 'bool istype = false;'
> And then we have many conditional codes using the above, eg: istype ?
> "ATTRIBUTE" : "COLUMN". We are not changing 'istype' anywhere and it
> is hard-coded in the beginning. It means there are parts of code in
> this function which will never be htt (written for 'istype=true' case)
> , so why do we need this variable and conditional code around it?
Removed.
>
> 6) There are plenty of places where we use 'append_not_present'
> without using 'append_null_object'.
> Do we need to have 'append_null_object' along with
> 'append_not_present' at these places?
I think we can remove append_null_object and replace it with a
append_format_string as the null object is unnecessary. And I moved these logic
to a separate patch and extended the append_not_present to automatically add a
format string.
>
>
> 7) deparse_utility_command():
> Rather than inject --> Rather than injecting
Fixed
~~~~
Comments from [2]
> 1) expand_jsonval_string()
> I think we need to assert if jsonval is neither jbvString nor jbvBinary.
Added.
> 2) expand_jsonval_strlit()
> same here, assert if not jbvString (like we do in expand_jsonval_number and expand_jsonval_identifier > etc)
>
Added.
> 6)expand_fmt_recursive():
> value = findJsonbValueFromContainer(container, JB_FOBJECT, &key);
> Should this 'value' be freed at the end like we do at all other places in this file?
Added.
~~~~
Comments from [3]
>
> 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.
I moved these codes to different patches but haven’t addressed comments.
I will address this in next version.
>
>
> 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.
I am not sure if we should check commandCollectionInhibited at this function,
because normally this was only checked at command collection
function(EventTriggerCollectSimpleCommand, EventTriggerAlterTableStart).
>
>
> 4) EventTriggerTableInitWriteEnd()
> Here as well, we can have the same assert as below:
> Assert(currentEventTriggerState->currentCommand != NULL);
> 'currentEventTriggerState' and 'commandCollectionInhibited' checks are
> already present.
Added.
>
> 5) logical_replication.sgml:
> + 'This is especially useful if you have lots schema changes over time that
> need replication.'
>
> lots schema --> lots of schema
Fixed.
Thanks Shveta for helping address comments.
Aport from above comments, I splitted the code related to verbose
mode to a separate patch. And here is the new version patch set.
[1] https://www.postgresql.org/message-id/CAJpy0uDb2mDJtLNFXzUY4911qRZOvj6Q8pu4xFh4BMYBeOSPow%40mail.gma...
[2] https://www.postgresql.org/message-id/CAJpy0uAA0SQ0kPA5bXmrW%3D32p0bwFCifoKb5OSgteTjGggEkLA%40mail.g...
[3] https://www.postgresql.org/message-id/CAJpy0uB7f2GxPNor5iTT-30JuD-p-gvnsMZG9tiiHN%2BDHJj0RQ%40mail.g...
Best Regards,
Hou zj
Attachments:
[application/x-gzip] ddl-replication-2023_04_25.tar.gz (145.2K, 2-ddl-replication-2023_04_25.tar.gz)
download
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-general@postgresql.org
Cc: houzj.fnst@fujitsu.com, shveta.malik@gmail.com, amit.kapila16@gmail.com, vignesh21@gmail.com, itsajin@gmail.com, wangw.fnst@fujitsu.com, runqidev@gmail.com, smithpb2250@gmail.com, tgl@sss.pgh.pa.us, ggysxcq@gmail.com, dilipbalaut@gmail.com, alvherre@alvh.no-ip.org, sawada.mshk@gmail.com, japinli@hotmail.com, rajesh.rs0541@gmail.com, pgsql-hackers@lists.postgresql.org, zhengli10@gmail.com
Subject: RE: Support logical replication of DDLs
In-Reply-To: <OS0PR01MB57167AB450D6B3C5FF2C90EF94649@OS0PR01MB5716.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox