Re: Support logical replication of DDLs

On Fri, 7 Apr 2023 at 08:52, houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Friday, April 7, 2023 11:13 AM houzj(dot)fnst(at)fujitsu(dot)com <houzj(dot)fnst(at)fujitsu(dot)com>
> >
> > On Tuesday, April 4, 2023 7:35 PM shveta malik <shveta(dot)malik(at)gmail(dot)com>
> > wrote:
> > >
> > > On Tue, Apr 4, 2023 at 8:43 AM houzj(dot)fnst(at)fujitsu(dot)com
> > > <houzj(dot)fnst(at)fujitsu(dot)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