Re: Support logical replication of DDLs - Mailing list pgsql-hackers

Here are some more comments for the patch v32-0001, file:
src/backend/commands/ddl_deparse.c

This is a WIP, it being such a large file...

======

1. General - comments

For better consistency, I suggest using uppercase for all the
single-line comments in the function bodies.

There are multiple of them - I am not going to itemize them all in
this post. Please just search/replace all of them

e.g.
/* add the "ON table" clause */
/* add the USING clause, if any */
/* add the USING clause, if any */

~~~

2. General - object names

There is a bit of inconsistency with the param object names where
there are multi-words.

Some have underscore (e.g. "is_public", "subtype_diff", "if_not_exists", etc)...
Many others do not (e.g. "schemaname", "objname", "rolename", etc)...

IMO it would be better to use a consistent naming convention - e,g,
maybe use '_' *everywhere*


~~~

3. ObjTree

+typedef struct ObjTree
+{
+ slist_head params; /* Object tree parameters */
+ int numParams; /* Number of parameters in the object tree */
+ StringInfo fmtinfo; /* Format string of the ObjTree */
+ bool present; /* Indicates if boolean value should be stored */
+} ObjTree;

It seems that this member is called "parameters" in the sense that
each of these params are destined to be substition-params of for the
format string part of this struct.

OK. That seems sensible here, but this 'parameter' terminology infests
this whole source file. IIUC really much of the code is dealing with
just JSON objects -- they don't become parameters until those objects
get added into the params list of this structure. Basically, I felt
the word 'parameter' in comments and the variables called 'param' in
functions seemed a bit overused...

~~~

4. ObjElem

+ slist_node node; /* Used in converting back to ObjElem
+ * structure */
+} ObjElem;

At face value (and without yet seeing the usage), that comment about
'node' does not mean much. e.g. this is already an 'ObjElem' struct...
(??)

~~~

5. verbose

+/*
+ * Reduce some unncessary string from the output json stuff when verbose
+ * and "present" member is false. This means these strings won't be merged into
+ * the last DDL command.
+ */
+bool verbose = true;

The comment needs some rewording to explain what this is about more
clearly and without the typos

"Reduce some unncessary string from the output json stuff" ???

~~~

6. add_policy_clauses

+ else
+ {
+ append_bool_object(policyStmt, "present", false);
+ }

Something seems strange. Probably I'm wrong but just by code
inspection it looks like there is potential for there to be multiple
param {present:false} JSON objects:

{"present" :false},
{"present" :false},
{"present" :false},

Shouldn't those all be array elements or something? IIUC apart from
just DDL, the JSON idea was going to (in future) allow potential
machine manipulation of the values prior to the replication, but
having all these ambiguous-looking objects does not seem to lend
itself to that idea readily. How to know what are each of those params
representing?

~~~

7. append_array_object


+ }
+
+ }

Spurious blank line

~~

8.

+ /* Extract the ObjElems whose present flag is true */
+ foreach(lc, array)
+ {
+ ObjElem    *elem = (ObjElem *) lfirst(lc);
+
+ Assert(elem->objtype == ObjTypeObject ||
+    elem->objtype == ObjTypeString);
+
+ if (!elem->value.object->present &&
+ elem->objtype == ObjTypeObject)
+ array = foreach_delete_current(array, lc);
+ }
+
+ }

8a.
Is that comment correct? Or should it say more like "remove elements
where present flag is false" ??

8b.
It's not clear to me what is going to be the result of deleting the
array elements that are determined not present. Will this affect the
length of the array written to JSON? What if there is nothing left at
all - the top of this function return if the array length is zero, but
the bottom(after the loop) has not got similar logic.

~~~

9. append_bool_object

+ /*
+ * Check if the present is part of the format string and store the boolean
+ * value
+ */
+ if (strcmp(sub_fmt, "present") == 0)

The comment seems not right. Looks like not testing "present" is PART
of the format string - it is testing it IS the ENTIRE format string.

~~~

10. append_object_to_format_string

+ initStringInfo(&object_name);
+ end_ptr = sub_fmt + strlen(sub_fmt);
+
+ for (cp = sub_fmt; cp < end_ptr; cp++)
+ {
+ if (*cp == '{')
+ {
+ start_copy = true;
+ continue;
+ }
+
+ if (!start_copy)
+ continue;
+
+ if (*cp == ':' || *cp == '}')
+ break;
+
+ appendStringInfoCharMacro(&object_name, *cp);
+ }

Instead of this little loop why doesn't the code just look for the
name delimiters?

e.g.
pstart = strch(sub_fmt, '{');
pend = strbrk(pstart, ":}");

then the 'name' is what lies in between...

~~~

11.

format_type_detailed(Oid type_oid, int32 typemod,
                     Oid *nspid, char **typname, char **typemodstr,
                     bool *typarray)


There seems a bit mixture of param prefixes of both 'typ' and 'type'.
Is it correct? If these are changed, check also in the function
comment.

~~~

12.

+ /*
+ * Special-case crock for types with strange typmod rules where we put
+ * typmod at the middle of name(e.g. TIME(6) with time zone ). We cannot
+ * schema-qualify nor add quotes to the type name in these cases.
+ */

Missing space before '(e.g.'. Extra space before ').'.

~~~

13. FunctionGetDefaults

/*
 * Return the defaults values of arguments to a function, as a list of
 * deparsed expressions.
 */

"defaults values" -> "default values"

------
Kind Regards,
Peter Smith.
Fujitsu Australia