Re: Support logical replication of DDLs - Mailing list pgsql-hackers
From | Zheng Li |
---|---|
Subject | Re: Support logical replication of DDLs |
Date | |
Msg-id | CAAD30UJ2MmA7vM1H2b20L_SMHS0-76raROqZELs-GDGk3Pet5A@mail.gmail.com Whole thread Raw |
In response to | Re: Support logical replication of DDLs (Peter Smith <smithpb2250@gmail.com>) |
List | pgsql-hackers |
On Fri, Oct 28, 2022 at 2:50 AM Peter Smith <smithpb2250@gmail.com> wrote: > > Here are some review comments for patch v32-0001. > > This is a WIP - I have not yet looked at the largest file of this > patch (src/backend/commands/ddl_deparse.c) > > ====== > > Commit Message > > 1. > > The list of the supported statements should be in alphabetical order > to make it easier to read Updated the list in the commit message. > 2. > > The "Notes" are obviously notes, so the text does not need to say > "Note that..." etc again > > "(Note #1) Note that some..." -> "(Note #1) Some..." > > "(Note #2) Note that, for..." -> "(Note #2) For..." > > "(Note #4) Note that, for..." -> "(Note #4) For..." Modified. > 3. > > For "Note #3", use uppercase for the SQL keywords in the example. Modified. > 4. > > For "Note #4": > > "We created" -> "we created" Modified. > ====== > > src/backend/catalog/aclchk.c > > 5. ExecuteGrantStmt > > @@ -385,7 +385,11 @@ ExecuteGrantStmt(GrantStmt *stmt) > ereport(ERROR, > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > errmsg("grantor must be current user"))); > + > + istmt.grantor_uid = grantor; > } > + else > + istmt.grantor_uid = InvalidOid; > > This code can be simpler by just declaring the 'grantor' variable at > function scope, then assigning the istmt.grantor_uid along with the > other grantor assignments. > > SUGGESTION > Oid grantor = InvalidOid; > ... > istmt.grantor_uid = grantor; > istmt.is_grant = stmt->is_grant; > istmt.objtype = stmt->objtype; Modified. > ====== > > src/backend/commands/collationcmds.c > > 6. DefineCollation > > + /* make from existing collationid available to callers */ > + if (from_collid && OidIsValid(collid)) > + ObjectAddressSet(*from_collid, > + CollationRelationId, > + collid); > > 6a. > Maybe the param can be made 'from_existing_colid', then the above code > comment can be made more readable? Modified. > 6b. > Seems some unnecessary wrapping here Modified. > 7. convSpecifier > > typedef enum > { > SpecTypename, > SpecOperatorname, > SpecDottedName, > SpecString, > SpecNumber, > SpecStringLiteral, > SpecIdentifier, > SpecRole > } convSpecifier; > > Inconsistent case. Some of these say "name" and some say "Name" Modified. > 8. Forward declarations > > char *ddl_deparse_json_to_string(char *jsonb); > > Is this needed here? I thought this was already declared extern in > ddl_deparse.h. It is needed. We get the following warning without it: ddl_json.c:704:1: warning: no previous prototype for ‘ddl_deparse_json_to_string’ [-Wmissing-prototypes] ddl_deparse_json_to_string(char *json_str) > 9. find_string_in_jsonbcontainer > > The function comment says "If it's of a type other than jbvString, an > error is raised.", but I do not see this check in the function code. Modified. > 10. expand_fmt_recursive > > /* > * Recursive helper for pg_event_trigger_expand_command > * > * Find the "fmt" element in the given container, and expand it into the > * provided StringInfo. > */ > > > 10a. > I am not sure if the mention of "pg_event_trigger_expand_command" is > stale or is not relevant anymore, because that caller is not in this > module. Modified. > 10b. > The first sentence is missing a period. Modified. > 11. > > value = findJsonbValueFromContainer(container, JB_FOBJECT, &key); > > Should this be checking is value is NULL? The null checking for value is done in the upcoming call of expand_one_jsonb_element(). > 12. expand_jsonval_dottedname > > * Expand a json value as a dot-separated-name. The value must be of type > * object and may contain elements "schemaname" (optional), "objname" > * (mandatory), "attrname" (optional). Double quotes are added to each element > * as necessary, and dot separators where needed. > > The comment says "The value must be of type object" but I don't see > any check/assert for that in the code. The value must be of type binary, updated comment and added Assert(jsonval→type == jbvBinary); > 13. expand_jsonval_typename > > In other code (e.g. expand_jsonval_dottedname) there are lots of > pfree(str) so why not similar here? > > e.g. Shouldn’t the end of the function have like shown below: > pfree(schema); > pfree(typename); > pfree(typmodstr); Modified. > 14. expand_jsonval_operator > > The function comment is missing a period. Modified. > 15. expand_jsonval_string > > /* > * Expand a JSON value as a string. The value must be of type string or of > * type object. In the latter case, it must contain a "fmt" element which will > * be recursively expanded; also, if the object contains an element "present" > * and it is set to false, the expansion is the empty string. > > 15a. > Although the comment says "The value must be of type string or of type > object" the code is checking for jbvString and jbvBinary (??) Updated the comment to “The value must be of type string or of type binary” > 15b. > else > return false; > > Is that OK to just return false, or should this in fact be throwing an > error if the wrong type? The caller checks the type is either jbvString or jbvBinary. Added comment “The caller is responsible to check jsonval is of type jbvString or jbvBinary”. > 16. expand_jsonval_strlit > > /* Easy case: if there are no ' and no \, just use a single quote */ > if (strchr(str, '\'') == NULL && > strchr(str, '\\') == NULL) > > That could be simplified as: > > if ((strpbk(str, "\'\\") == NULL) Modified. > 17. expand_jsonval_number > > strdatum = DatumGetCString(DirectFunctionCall1(numeric_out, > > NumericGetDatum(jsonval->val.numeric))); > appendStringInfoString(buf, strdatum); > > Shouldn't this function do pfree(strdatum) at the end? Modified. > 18. expand_jsonval_role > > /* > * Expand a JSON value as a role name. If the is_public element is set to > * true, PUBLIC is expanded (no quotes); otherwise, expand the given role name, > * quoting as an identifier. > */ > > > Maybe better to quote that element name -> 'If the "is_public" element > is set to true...' I think we need to quote the non-public roles just in case they contain special characters. > 19. expand_one_jsonb_element > > The enum jbvType definition says that jbvBinary is a combination of > array/object, so I am not sure if that should be reflected in the > errmsg text (multiple places in this function body) instead of only > saying "JSON object". Updated errmsg texts to “JSON struct”. > 20. ddl_deparse_expand_command > > * % expand to a literal %. > > > Remove the period from that line (because not of the other specifier > descriptions have one). Modified. > ====== > > src/backend/utils/adt/regproc.c > > 21. format_procedure_args_internal > > +static void > +format_procedure_args_internal(Form_pg_proc procform, StringInfo buf, > + bool force_qualify) > +{ > + int i; > + int nargs = procform->pronargs; > > The 'nargs' var is used one time only, so hardly seems worth having it. Modified. > 22. > > + appendStringInfoString(buf, > + force_qualify ? > + format_type_be_qualified(thisargtype) : > + format_type_be(thisargtype)); > > 22a. > Should these function results be assigned to a char* ptr so that they > can be pfree(ptr) AFTER being appended to the 'buf'? Modified. > 22b. > It's not really nececessary to check the force_qualify at every > iteration. More effient to asign a function pointer outside this loop > and just call that here. IIRC something like this: > > char * (*func[2])(Oid) = { format_type_be, format_type_be_qualified }; > > ... > > then > appendStringInfoString(buf, func[force_qualify](thisargtype)) Modified. > src/backend/utils/adt/ruleutils.c > > 23. pg_get_ruledef_detailed > > Instead of the multiple if/else it might be easier to just assignup-front: > *whereClause = NULL; > *actions = NIL; > > Then the if blocks can just overwrite them. > > Also, if you do that, then I expect probably the 'output' temp list > var is not needed at all. Modified. > 24. pg_get_viewdef_internal > > /* > * In the case that the CREATE VIEW command execution is still in progress, > * we would need to search the system cache RULERELNAME to get the rewrite > * rule of the view as oppose to querying pg_rewrite as in > pg_get_viewdef_worker(), > * the latter will return empty result. > */ > > 24a. > I'm not quite sure of the context of this function call. Maybe the > comment was supposed to be worded more like below? > > "Because this function is called when CREATE VIEW command execution is > still in progress, we need to search..." Improved comment. > 24b. > "as oppose" -> "as opposed" Modified. > 25. pg_get_triggerdef_worker > > if (!isnull) > { > Node *qual; > char *qualstr; > > qual = stringToNode(TextDatumGetCString(value)); > qualstr = pg_get_trigger_whenclause(trigrec, qual, pretty); > > appendStringInfo(&buf, "WHEN (%s) ", qualstr); > } > > After appending the qualstr to buf, should there be a pfree(qualstr)? I think we should skip pfree(qualstr) here since the memory is allocated by initStringInfo in pg_get_trigger_whenclause, to avoid double free when the StringInfoData in pg_get_trigger_whenclause gets freed. > 26. pg_get_trigger_whenclause > > Missing function comment. Added comment. > 27. print_function_sqlbody > > -static void > +void > print_function_sqlbody(StringInfo buf, HeapTuple proctup) > { > > Missing function comment. Probably having a function comment is more > important now that this is not static? Added comment. > src/include/tcop/ddl_deparse.h > > 28. > > +extern char *deparse_utility_command(CollectedCommand *cmd, bool verbose_mode); > +extern char *ddl_deparse_json_to_string(char *jsonb); > +extern char *deparse_drop_command(const char *objidentity, const char > *objecttype, > + DropBehavior behavior); > > Function naming seems inconsistent. ('ddl_deparse_XXX' versus 'deparse_XXX'). modified. Regards, Zheng
Attachment
pgsql-hackers by date: