Horton

Re: Support logical replication of DDLs

Amit Kapila <amit.kapila16@gmail.com>

View thread
From: Amit Kapila <amit.kapila16@gmail.com>
To: Ajin Cherian <itsajin@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>, vignesh C <vignesh21@gmail.com>, Zheng Li <zhengli10@gmail.com>, li jie <ggysxcq@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>, "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Japin Li <japinli@hotmail.com>, rajesh singarapu <rajesh.rs0541@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2023-02-14T11:02:13Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add a run_as_owner option to subscriptions.

  2. Refactor pgoutput_change().

  3. Print the correct aliases for DML target tables in ruleutils.

  4. Fix object identity string for transforms

  5. Add grantable MAINTAIN privilege and pg_maintain role.

  6. Get rid of recursion-marker values in enum AlterTableType

  7. Release cache tuple when no longer needed

  8. Empty search_path in logical replication apply worker and walsender.

  9. Refactor format_type APIs to be more modular

  10. Use wrappers of PG_DETOAST_DATUM_PACKED() more.

On Fri, Feb 10, 2023 at 4:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Feb 9, 2023 at 3:25 PM Ajin Cherian <itsajin@gmail.com> wrote:
> >
>
> Comments on 0001 and 0002
> =======================
>

Few more comments on 0001 and 0003
===============================
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?

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().

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 ");
...
}

4. In deparse_CreateTrigStmt(), the handling for REFERENCING OLD/NEW
Table is missing. See the corresponding code in
pg_get_triggerdef_worker().

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?

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.

8. Can we think of better names instead of appending 'simple' in the
above two cases?

-- 
With Regards,
Amit Kapila.