Re: Support logical replication of DDLs

On Fri, 7 Apr 2023 at 08:52, houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Friday, April 7, 2023 11:13 AM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com>
> >
> > On Tuesday, April 4, 2023 7:35 PM shveta malik <shveta.malik@gmail.com>
> > wrote:
> > >
> > > On Tue, Apr 4, 2023 at 8:43 AM houzj.fnst@fujitsu.com
> > > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > > Attach the new version patch set which did the following changes:
> > > >
> > >
> > > Hello,
> > >
> > > I tried below:
> > > pubnew=# ALTER PUBLICATION mypub2 SET (ddl = 'table'); ALTER
> > > PUBLICATION
> > >
> > > pubnew=# \dRp+
> > >                                         Publication mypub2 Owner  |
> > > All tables
> > > | All DDLs | Table DDLs |
> > > --------+------------+----------+------------+---------
> > > shveta |         t          | f               | f
> > > (1 row)
> > >
> > > I still see 'Table DDLs' as false and ddl replication did not work for this case.
> >
> > Thanks for reporting.
> >
> > Attach the new version patch which include the following changes:
> > * Fix the above bug for ALTER PUBLICATION SET.
> > * Modify the corresponding event trigger when user execute ALTER
> > PUBLICATION SET to change the ddl option.
> > * Fix a miss in pg_dump's code which causes CFbot failure.
> > * Rebase the patch due to recent commit 4826759.
>
> Sorry, there was a miss when rebasing the patch which could cause the
> CFbot to fail and here is the correct patch set.

Few comments:
1) I felt is_present_flag variable can be removed by moving
"object_name = append_object_to_format_string(tree, sub_fmt);" inside
the if condition:
+static void
+append_bool_object(ObjTree *tree, char *sub_fmt, bool value)
+{
+       ObjElem    *param;
+       char       *object_name = sub_fmt;
+       bool            is_present_flag = false;
+
+       Assert(sub_fmt);
+
+       /*
+        * Check if the format string is 'present' and if yes, store the boolean
+        * value
+        */
+       if (strcmp(sub_fmt, "present") == 0)
+       {
+               is_present_flag = true;
+               tree->present = value;
+       }
+
+       if (!is_present_flag)
+               object_name = append_object_to_format_string(tree, sub_fmt);
+
+       param = new_object(ObjTypeBool, object_name);
+       param->value.boolean = value;
+       append_premade_object(tree, param);
+}

By changing it to something like below:
+static void
+append_bool_object(ObjTree *tree, char *sub_fmt, bool value)
+{
+       ObjElem    *param;
+       char       *object_name = sub_fmt;
+
+       Assert(sub_fmt);
+
+       /*
+        * Check if the format string is 'present' and if yes, store the boolean
+        * value
+        */
+       if (strcmp(sub_fmt, "present") == 0)
+       {
+               tree->present = value;
+               object_name = append_object_to_format_string(tree, sub_fmt);
+       }
+
+       param = new_object(ObjTypeBool, object_name);
+       param->value.boolean = value;
+       append_premade_object(tree, param);
+}

2) We could remove the temporary variable tmp_str here:
+       if (start_ptr != NULL && end_ptr != NULL)
+       {
+               length = end_ptr - start_ptr - 1;
+               tmp_str = (char *) palloc(length + 1);
+               strncpy(tmp_str, start_ptr + 1, length);
+               tmp_str[length] = '\0';
+               appendStringInfoString(&object_name, tmp_str);
+               pfree(tmp_str);
+       }

by changing to:
+       if (start_ptr != NULL && end_ptr != NULL)
+               appendBinaryStringInfo(&object_name, start_ptr + 1,
end_ptr - start_ptr - 1);

3) I did not see the usage of ObjTypeFloat type used anywhere, we
could remove it:
+typedef enum
+{
+       ObjTypeNull,
+       ObjTypeBool,
+       ObjTypeString,
+       ObjTypeArray,
+       ObjTypeInteger,
+       ObjTypeFloat,
+       ObjTypeObject
+} ObjType;

4) I noticed that none of the file names in src/backend/commands uses
"_" in the filenames, but in case of ddl_deparse.c and ddl_json.c we
have used "_", it might be better to be consistent with other
filenames in this directory:

diff --git a/src/backend/commands/Makefile b/src/backend/commands/Makefile
index 48f7348f91..171dfb2800 100644
--- a/src/backend/commands/Makefile
+++ b/src/backend/commands/Makefile
@@ -29,6 +29,8 @@ OBJS = \
        copyto.o \
        createas.o \
        dbcommands.o \
+       ddl_deparse.o \
+       ddl_json.o \
        define.o \
        discard.o \
        dropcmds.o \

5) The following includes are no more required in ddl_deparse.c as we
have removed support for deparsing of other objects:
#include "catalog/pg_am.h"
#include "catalog/pg_aggregate.h"
#include "catalog/pg_authid.h"
#include "catalog/pg_cast.h"
#include "catalog/pg_conversion.h"
#include "catalog/pg_depend.h"
#include "catalog/pg_extension.h"
#include "catalog/pg_foreign_data_wrapper.h"
#include "catalog/pg_foreign_server.h"
#include "catalog/pg_language.h"
#include "catalog/pg_largeobject.h"
#include "catalog/pg_opclass.h"
#include "catalog/pg_operator.h"
#include "catalog/pg_opfamily.h"
#include "catalog/pg_policy.h"
#include "catalog/pg_range.h"
#include "catalog/pg_rewrite.h"
#include "catalog/pg_sequence.h"
#include "catalog/pg_statistic_ext.h"
#include "catalog/pg_transform.h"
#include "catalog/pg_ts_config.h"
#include "catalog/pg_ts_dict.h"
#include "catalog/pg_ts_parser.h"
#include "catalog/pg_ts_template.h"
#include "catalog/pg_user_mapping.h"
#include "foreign/foreign.h"
#include "mb/pg_wchar.h"
#include "nodes/nodeFuncs.h"
#include "nodes/parsenodes.h"
#include "parser/parse_type.h"

Regards,
Vignesh