Re: Support logical replication of DDLs

On Fri, Apr 7, 2023 at 8:52 AM houzj.fnst@fujitsu.com
 wrote:
>

Few comments on 0001
===================
1.
+ ConstrObjDomain,
+ ConstrObjForeignTable
+} ConstraintObjType;

These both object types don't seem to be supported by the first patch.
So, I don't see why these should be part of it.

2.
+append_string_object(ObjTree *tree, char *sub_fmt, char * object_name,

Extra space before object_name.

3. Is there a reason to keep format_type_detailed() in ddl_deparse.c
instead of defining it in format_type.c where other format functions
reside? Earlier, we were doing this deparsing as an extension, so it
makes sense to define it locally but not sure if that is required now.

4.
format_type_detailed()
{
...
+ /*
+ * Check if it's a regular (variable length) array type.  As above,
+ * fixed-length array types such as "name" shouldn't get deconstructed.
+ */
+ array_base_type = typeform->typelem;

This comment gives incomplete information. I think it is better to
say: "We switch our attention to the array element type for certain
cases. See format_type_extended(). Then we can remove a similar
comment later in the function.

5.
+
+ switch (type_oid)
+ {
+ case INTERVALOID:
+ *typename = pstrdup("INTERVAL");
+ break;
+ case TIMESTAMPTZOID:
+ if (typemod < 0)
+ *typename = pstrdup("TIMESTAMP WITH TIME ZONE");
+ else
+ /* otherwise, WITH TZ is added by typmod. */
+ *typename = pstrdup("TIMESTAMP");
+ break;
+ case TIMESTAMPOID:
+ *typename = pstrdup("TIMESTAMP");
+ break;

In this switch case, use the type oid cases in the order of their value.

6.
+static inline char *
+get_type_storage(char storagetype)

We already have a function with the name storage_name() which does
exactly what this function is doing. Shall we expose that and use it?

7.
+static ObjTree *
+new_objtree(char *fmt)
+{
+ ObjTree    *params;
+
+ params = palloc0(sizeof(ObjTree));

Here, the variable name params appear a bit odd. Shall we change it to
objtree or obj?

-- 
With Regards,
Amit Kapila.