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