Re: Support logical replication of DDLs

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
===============================
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.