RE: Support logical replication of DDLs

On Tues, Feb 14, 2023 at 19:02 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 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

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