From: | "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Ajin Cherian <itsajin(at)gmail(dot)com> |
Cc: | Peter Smith <smithpb2250(at)gmail(dot)com>, vignesh C <vignesh21(at)gmail(dot)com>, Zheng Li <zhengli10(at)gmail(dot)com>, 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> |
Subject: | RE: Support logical replication of DDLs |
Date: | 2023-02-19 14:54:40 |
Message-ID: | OS0PR01MB5716AFDB4C888DF630771DAB94A79@OS0PR01MB5716.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-general pgsql-hackers |
On Tues, Feb 14, 2023 at 19:02 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Feb 10, 2023 at 4:36 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Thu, Feb 9, 2023 at 3:25 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> > >
> >
> > Comments on 0001 and 0002
> > =======================
> >
>
> Few more comments on 0001 and 0003
Thanks for your comments.
> ===============================
> 1. I think having 'internal' in an exposed function
> pg_get_viewdef_internal() seems a bit odd to me. Shall we name it
> something like pg_get_viewdef_sys() as it consults the system cache?
I think it might be better to rename these to pg_get_xxxdef_string(). Because we used these style in some existing functions(e.g. pg_get_statisticsobjdef_string, pg_get_indexdef_string, pg_get_partconstrdef_string).
So renamed pg_get_viewdef_internal to pg_get_viewdef_string.
> 2. In pg_get_trigger_whenclause(), there are various things that have
> changed in the new code but the patch didn't update those. It is
> important to update those especially because it replaces the existing
> code as well. For example, it should use GET_PRETTY_FLAGS for
> prettyFlags, then some variables are not initialized, and also didn't
> use rellockmode for old and new rtes. I suggest carefully comparing
> the code with the corresponding existing code in the function
> pg_get_triggerdef_worker().
Make sense. According to the current function pg_get_triggerdef_worker, updated the function pg_get_trigger_whenclause.
> 3.
> deparse_CreateTrigStmt
> {
> ...
> +
> + if (node->deferrable)
> + list = lappend(list, new_string_object("DEFERRABLE")); if
> + (node->initdeferred) list = lappend(list,
> + new_string_object("INITIALLY DEFERRED")); append_array_object(ret,
> + "%{constraint_attrs: }s", list);
> ...
> }
>
> Is there a reason that the above part of the conditions doesn't match
> the below conditions in pg_get_triggerdef_worker()?
> pg_get_triggerdef_worker()
> {
> ...
> if (!trigrec->tgdeferrable)
> appendStringInfoString(&buf, "NOT ");
> appendStringInfoString(&buf, "DEFERRABLE INITIALLY "); if
> (trigrec->tginitdeferred) appendStringInfoString(&buf, "DEFERRED ");
> else appendStringInfoString(&buf, "IMMEDIATE "); ...
> }
Modified deparse_CreateTrigStmt to be consistent with pg_get_trigger_whenclause.
> 4. In deparse_CreateTrigStmt(), the handling for REFERENCING OLD/NEW
> Table is missing. See the corresponding code in
> pg_get_triggerdef_worker().
Added.
> 5. In deparse_CreateTrigStmt(), the function name for EXECUTE
> PROCEDURE is generated in a different way as compared to what we are
> doing in pg_get_triggerdef_worker(). Is there a reason for the same?
I think the approach used in the function pg_get_triggerdef_worker sometimes doesn't include the schema name and returns the string directly (see function generate_function_name). And I think the approach used in the function deparse_CreateTrigStmt always includes the schema name and returns the ObjTree type result we need. So I think the current approach looks fine.
> 6.
> +char *
> +pg_get_partkeydef_simple(Oid relid)
> +{
> + return pg_get_partkeydef_worker(relid, 0, false, false); }
>
> The 0 is not a valid value for prettyFlags, so not sure what is the
> intention here. I think you need to use GET_PRETTY_FLAGS() here.
>
> 7. The above comment applies to pg_get_constraintdef_command_simple()
> as well.
Change '0' to 'GET_PRETTY_FLAGS(false)'.
> 8. Can we think of better names instead of appending 'simple' in the
> above two cases?
Renamed pg_get_partkeydef_simple to pg_get_partkeydef_string.
Renamed pg_get_constraintdef_command_simple to pg_get_constraintdef_string.
Attach the new version patchset. The 0001,0002,0003,0004,0006,0008 patches
were modified, and the details are as follows:
The following changes have been made to the patch set:
1. The comments by Amit in [1] were addressed in patches 0001, 0002 and 0003.
2. The comments by Sawada and Amit in [2] were addressed in patches 0002 and 0006.
3. The comments by Alvaro in [1] were addressed in patches 0001, 0003 and 0008.
4. Removed redundant function calls (append_bool_object(tmp, "present", true);) in the function deparse_DefineStmt_Aggregate introduced in patch v70-0003-*. In addition, modify the expected results of related tests in patch 0004.
[1] - https://www.postgresql.org/message-id/CAA4eK1%2BpdyQoYB4R5rzrxZjz2dNWW1p2iqAj7J9qWeTvKDyBiQ%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/CAA4eK1J2voRVoYBB%3Dr4xtdzYTSPX7RnTcvXyYLk031YE6gWxKg%40mail.gmail.com
[3] - https://www.postgresql.org/message-id/20230216180200.4shhjmuzhdb24nh6%40alvherre.pgsql
Best Regards,
Hou zj
Attachment | Content-Type | Size |
---|---|---|
v71-0008-Allow-replicated-objects-to-have-the-same-owner-.patch | application/octet-stream | 54.2 KB |
v71-0001-Infrastructure-to-support-DDL-deparsing.patch | application/octet-stream | 44.3 KB |
v71-0002-Functions-to-deparse-Table-DDL-commands.patch | application/octet-stream | 131.2 KB |
v71-0003-Support-DDL-deparse-of-the-rest-commands.patch | application/octet-stream | 201.2 KB |
v71-0004-Introduce-the-test_ddl_deparse_regress-test-modu.patch | application/octet-stream | 47.3 KB |
v71-0005-DDL-messaging-infrastructure-for-DDL-replication.patch | application/octet-stream | 41.5 KB |
v71-0006-Support-DDL-replication.patch | application/octet-stream | 213.1 KB |
v71-0007-Document-DDL-replication-and-DDL-deparser.patch | application/octet-stream | 40.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | houzj.fnst@fujitsu.com | 2023-02-19 14:55:57 | RE: Support logical replication of DDLs |
Previous Message | Christophe Pettus | 2023-02-19 02:54:35 | Re: Who adds the "start transaction" and "commit" to the intended SQL statement in "autocommit" mode? |
From | Date | Subject | |
---|---|---|---|
Next Message | houzj.fnst@fujitsu.com | 2023-02-19 14:55:57 | RE: Support logical replication of DDLs |
Previous Message | Robert Haas | 2023-02-19 14:36:24 | Re: Weird failure with latches in curculio on v15 |