From: | shveta malik <shveta(dot)malik(at)gmail(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | "Yu Shi (Fujitsu)" <shiy(dot)fnst(at)fujitsu(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(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>, shveta malik <shveta(dot)malik(at)gmail(dot)com> |
Subject: | Re: Support logical replication of DDLs |
Date: | 2023-06-08 12:07:58 |
Message-ID: | CAJpy0uDPgqTYqeKyXHyxEi0Rr9UMfTP3eZtzv+J3bkUgAxUm6A@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-general pgsql-hackers |
On Tue, Jun 6, 2023 at 4:26 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Mon, Jun 5, 2023 at 3:00 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
>
> Few assorted comments:
Hi Amit, thanks for the feedback. Addressed these in recent patch
posted (*2023_06_08.patch)
> ===================
> 1. I see the following text in 0005 patch: "It supports most of ALTER
> TABLE command except some commands(DDL related to PARTITIONED TABLE
> ...) that are recently introduced but are not yet supported by the
> current ddl_deparser, we will support that later.". Is this still
> correct?
>
Removed this from the commit message.
> 2. I think the commit message of 0008
> (0008-ObjTree-Removal-for-multiple-commands-2023_05_22) should be
> expanded to explain why ObjTree is not required and or how you have
> accomplished the same with jsonb functions.
>
Done.
> 3. 0005* patch has the following changes in docs:
> + <table id="ddl-replication-by-command-tag">
> + <title>DDL Replication Support by Command Tag</title>
> + <tgroup cols="3">
> + <colspec colname="col1" colwidth="2*"/>
> + <colspec colname="col2" colwidth="1*"/>
> + <colspec colname="col3" colwidth="1*"/>
> + <thead>
> + <row>
> + <entry>Command Tag</entry>
> + <entry>For Replication</entry>
> + <entry>Notes</entry>
> + </row>
> + </thead>
> + <tbody>
> + <row>
> + <entry align="left"><literal>ALTER AGGREGATE</literal></entry>
> + <entry align="center"><literal>-</literal></entry>
> + <entry align="left"></entry>
> + </row>
> + <row>
> + <entry align="left"><literal>ALTER CAST</literal></entry>
> + <entry align="center"><literal>-</literal></entry>
> + <entry align="left"></entry>
> ...
> ...
>
> If the patch's scope is to only support replication of table DDLs, why
> such other commands are mentioned?
>
Removed the other commands and made some adjustments.
> 4. Can you write some comments about the deparsing format in one of
> the file headers or in README?
>
Added atop ddljson.c as this file takes care of expansion based on
fmt-string added.
> 5.
> + <para>
> + The <literal>table_init_write</literal> event occurs just after
> the creation of
> + table while execution of <literal>CREATE TABLE AS</literal> and
> + <literal>SELECT INTO</literal> commands.
>
> Patch 0001 has multiple references to table_init_write trigger but it
> is introduced in the 0002 patch, so those changes either belong to
> 0002 or one of the later patches. I think that may reduce most of the
> changes in event-trigger.sgml.
>
Moved it to 0002 patch.
> 6.
> + if (relpersist == RELPERSISTENCE_PERMANENT)
> + {
> + ddl_deparse_context context;
> + char *json_string;
> +
> + context.verbose_mode = false;
> + context.func_volatile = PROVOLATILE_IMMUTABLE;
>
> Can we write some comments as to why PROVOLATILE_IMMUTABLE is chosen here?
>
Added some comments and modified the variable name to make it more
appropriate.
> 7.
> diff --git a/src/test/modules/test_ddl_deparse_regress/t/001_compare_dumped_results.pl.orig
> b/src/test/modules/test_ddl_deparse_regress/t/001_compare_dumped_results.pl.orig
> new file mode 100644
> index 0000000000..58cf7cdf33
> --- /dev/null
> +++ b/src/test/modules/test_ddl_deparse_regress/t/001_compare_dumped_results.pl.orig
>
> Will this file require for the 0008 patch? Or is this just a leftover?
>
Sorry, added by mistake. Removed it now.
thanks
Shveta
From | Date | Subject | |
---|---|---|---|
Next Message | shveta malik | 2023-06-08 12:18:14 | Re: Support logical replication of DDLs |
Previous Message | shveta malik | 2023-06-08 12:01:49 | Re: Support logical replication of DDLs |
From | Date | Subject | |
---|---|---|---|
Next Message | Matthias van de Meent | 2023-06-08 12:15:33 | Re: Let's make PostgreSQL multi-threaded |
Previous Message | Tomas Vondra | 2023-06-08 12:03:47 | Re: BRIN indexes vs. SK_SEARCHARRAY (and preprocessing scan keys) |