Re: Support logical replication of DDLs
От | vignesh C |
---|---|
Тема | Re: Support logical replication of DDLs |
Дата | |
Msg-id | CALDaNm3GRSBPAWAAeLeE8tBEfKkSH7zDx3gxk9ZPaA6NgARtZw@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Support logical replication of DDLs (Peter Smith <smithpb2250@gmail.com>) |
Ответы |
Re: Support logical replication of DDLs
(vignesh C <vignesh21@gmail.com>)
|
Список | pgsql-hackers |
On Fri, 11 Nov 2022 at 11:03, Peter Smith <smithpb2250@gmail.com> wrote: > > On Fri, Nov 11, 2022 at 4:17 PM Peter Smith <smithpb2250@gmail.com> wrote: > > > > On Fri, Nov 11, 2022 at 4:09 PM Peter Smith <smithpb2250@gmail.com> wrote: > > > > > > On Fri, Nov 11, 2022 at 3:47 PM Peter Smith <smithpb2250@gmail.com> wrote: > > > > > > > > Here are more review comments for the v32-0001 file ddl_deparse.c > > > > > > > > *** NOTE - my review post became too big, so I split it into smaller parts. > > > > > > > THIS IS PART 4 OF 4. > > ======= > > src/backend/commands/ddl_deparse.c > > 99. deparse_CreateUserMappingStmt > > + /* > + * Lookup up object in the catalog, so we don't have to deal with > + * current_user and such. > + */ > > Typo "Lookup up" Modified > 100. > > + createStmt = new_objtree("CREATE USER MAPPING "); > + > + append_object_object(createStmt, "FOR %{role}R", > new_objtree_for_role_id(form->umuser)); > + > + append_string_object(createStmt, "SERVER %{server}I", server->servername); > > All this can be combined into a single VA() function call. Modified > 101. > > + /* add an OPTIONS clause, if any */ > > Uppercase comment. Modified > 102. deparse_AlterUserMappingStmt > > + /* > + * Lookup up object in the catalog, so we don't have to deal with > + * current_user and such. > + */ > + > + tp = SearchSysCache1(USERMAPPINGOID, ObjectIdGetDatum(objectId)); > > 102a. > Typo "Lookup up" Modified > 102b. > Unnecessary blank line. Modified > 103. > > + alterStmt = new_objtree("ALTER USER MAPPING"); > + > + append_object_object(alterStmt, "FOR %{role}R", > new_objtree_for_role_id(form->umuser)); > + > + append_string_object(alterStmt, "SERVER %{server}I", server->servername); > > Can be combined into a single VA() function call. Modified > 104. > + /* add an OPTIONS clause, if any */ > > Uppercase comment Modified > 105. deparse_AlterStatsStmt > > + alterStat = new_objtree("ALTER STATISTICS"); > + > + /* Lookup up object in the catalog */ > + tp = SearchSysCache1(STATEXTOID, ObjectIdGetDatum(objectId)); > + if (!HeapTupleIsValid(tp)) > + elog(ERROR, "cache lookup failed for statistic %u", objectId); > + > + statform = (Form_pg_statistic_ext) GETSTRUCT(tp); > + > + append_object_object(alterStat, "%{identity}D", > + new_objtree_for_qualname(statform->stxnamespace, > + NameStr(statform->stxname))); > + > + append_float_object(alterStat, "SET STATISTICS %{target}n", > node->stxstattarget); > > 105a. > This was a biff unusual to put the new_objtree even before the catalog lookup. Modified > 105b. > All new_objtreee and append_XXX can be combined as a single VA() > function call here. Modified > 106. deparse_RefreshMatViewStmt > > + refreshStmt = new_objtree_VA("REFRESH MATERIALIZED VIEW", 0); > + > + /* Add a CONCURRENTLY clause */ > + append_string_object(refreshStmt, "%{concurrently}s", > + node->concurrent ? "CONCURRENTLY" : ""); > + /* Add the matview name */ > + append_object_object(refreshStmt, "%{identity}D", > + new_objtree_for_qualname_id(RelationRelationId, > + objectId)); > + /* Add a WITH NO DATA clause */ > + tmp = new_objtree_VA("WITH NO DATA", 1, > + "present", ObjTypeBool, > + node->skipData ? true : false); > + append_object_object(refreshStmt, "%{with_no_data}s", tmp); > > 106a. > Don't use VA() function for 0 args. This has been fixed already > 106b. > Huh? There are 2 different implementation styles here for the optional clauses > - CONCURRENTLY just replaces with an empty string > - WITH NOT DATA - has a new ObjTree either present/not present I have not made any changes for this, we can handle together when we are taking care of present/not present consistency across all > 106c. > Most/all of this can be combined into a single VA call. Modified > 107. deparse_DefElem > > + set = new_objtree(""); > + optname = new_objtree(""); > + > + if (elem->defnamespace != NULL) > + append_string_object(optname, "%{schema}I.", elem->defnamespace); > + > + append_string_object(optname, "%{label}I", elem->defname); > + > + append_object_object(set, "%{label}s", optname); > > The set should be created *after* the optname, then it can be done > something like: > > set = new_objtree_VA("%{label}s", 1, "label", OptTyeString, optname); Modified > 108. > > + append_string_object(set, " = %{value}L", > + elem->arg ? defGetString(elem) : > + defGetBoolean(elem) ? "TRUE" : "FALSE"); > > The calling code does not need to prefix the format with spaces like > this. The append_XXX will handle space separators automatically. Modified > 109. deparse_drop_command > > + stmt = new_objtree_VA(fmt, 1, "objidentity", ObjTypeString, identity); > + stmt2 = new_objtree_VA("CASCADE", 1, > + "present", ObjTypeBool, behavior == DROP_CASCADE); > + > + append_object_object(stmt, "%{cascade}s", stmt2); > > 109a. > 'stmt2' is a poor name. "CASCADE" is not a statement. Even 'tmpobj' Modified > 109b. > The 2 lines of cascade should be grouped together -- i.e. the blank > line should be *above* the "CASCADE", not below it. Modified > 110. deparse_FunctionSet > > + obj = new_objtree("RESET"); > + append_string_object(obj, "%{set_name}I", name); > > This can be combined as a single VA() call with a format "RESET %{set_name}I". Modified > 111. > > + if (kind == VAR_RESET_ALL) > + { > + obj = new_objtree("RESET ALL"); > + } > + else if (value != NULL) > > > It seems a bit strange that the decision is judged sometimes by the > *value*. Why isn’t this just deciding according to different > VariableSetKind (e.g. VAR_SET_VALUE) Modified > 112. deparse_IndexStmt > > + indexStmt = new_objtree("CREATE"); > + > + append_string_object(indexStmt, "%{unique}s", > + node->unique ? "UNIQUE" : ""); > + > + append_format_string(indexStmt, "INDEX"); > + > + append_string_object(indexStmt, "%{concurrently}s", > + node->concurrent ? "CONCURRENTLY" : ""); > + > + append_string_object(indexStmt, "%{if_not_exists}s", > + node->if_not_exists ? "IF NOT EXISTS" : ""); > + > + append_string_object(indexStmt, "%{name}I", > + RelationGetRelationName(idxrel)); > + > + append_object_object(indexStmt, "ON %{table}D", > + new_objtree_for_qualname(heaprel->rd_rel->relnamespace, > + RelationGetRelationName(heaprel))); > + > + append_string_object(indexStmt, "USING %{index_am}s", index_am); > + > + append_string_object(indexStmt, "(%{definition}s)", definition); > > This could all be combined to a single VA() function call. Modified > 113. deparse_OnCommitClause > > + case ONCOMMIT_NOOP: > + append_null_object(oncommit, "%{on_commit_value}s"); > + append_bool_object(oncommit, "present", false); > + break; > > Why is the null object necessary when the entire "ON COMMIT" is present=false? This code was intended to generate a verbose json node for "ON COMMIT". So that user can easily modify the command by changing the value of ON COMMIT to generate a new ddl command. > 114. deparse_RenameStmt > > + renameStmt = new_objtree_VA(fmtstr, 0); > + append_string_object(renameStmt, "%{if_exists}s", > + node->missing_ok ? "IF EXISTS" : ""); > + append_object_object(renameStmt, "%{identity}D", > + new_objtree_for_qualname(schemaId, > + node->relation->relname)); > + append_string_object(renameStmt, "RENAME TO %{newname}I", > + node->newname); > > 114a. > Don't use VA() for 0 args. This was already fixed. > 114b. > Anyway, all these can be combined to a single new_objtree_VA() call. Modified > 115. > > + renameStmt = new_objtree_VA("ALTER POLICY", 0); > + append_string_object(renameStmt, "%{if_exists}s", > + node->missing_ok ? "IF EXISTS" : ""); > + append_string_object(renameStmt, "%{policyname}I", node->subname); > + append_object_object(renameStmt, "ON %{identity}D", > + new_objtree_for_qualname_id(RelationRelationId, > + polForm->polrelid)); > + append_string_object(renameStmt, "RENAME TO %{newname}I", > + node->newname); > > All these can be combined into a single VA() call. Modified > 116. > > relation_close(pg_policy, AccessShareLock); > > } > break; > > case OBJECT_ATTRIBUTE: > > Spurious blank line before the } Modified > 117. > > + objtype = stringify_objtype(node->relationType); > + fmtstr = psprintf("ALTER %s", objtype); > + renameStmt = new_objtree(fmtstr); > > The code seems over-complicated. All these temporary assignments are > not really necessary. > > Maybe better remove the psprintf anyway, as per my general comment at > top of this review post. Here psprintf cannot be removed because node->relationType is not a fixed type, variable string cannot be used in fmr argument of new_objtree_VA. However objtype can be removed, I have removed it. > 118. > > + relation_close(relation, AccessShareLock); > + > + break; > + case OBJECT_FUNCTION: > > > The misplaced blank line should be *after* the break; not before it. Modified > 119. > > + char *fmt; > + > + fmt = psprintf("ALTER %s %%{identity}D USING %%{index_method}s > RENAME TO %%{newname}I", > + stringify_objtype(node->renameType)); > > Let's be consistent about the variable naming at least within the same > function. Elsewhere was 'fmt' was 'fmtstr' so make them all the same > (pick one). Removed fmt variable and used the existing fmtstr variable > 120. > > + objtype = stringify_objtype(node->renameType); > + fmtstring = psprintf("ALTER %s", objtype); > + > + renameStmt = new_objtree_VA(fmtstring, > + 0); > + append_object_object(renameStmt, "%{identity}D", > + new_objtree_for_qualname(DatumGetObjectId(objnsp), > + strVal(llast(identity)))); > + > + append_string_object(renameStmt, "RENAME TO %{newname}I", > + node->newname); > > 120a. > Simplify this by not going the assignment to 'objtype' Modified > 120b. > All this can be combined as a single VA() call. Modified > 121. deparse_AlterDependStmt > > +deparse_AlterDependStmt(Oid objectId, Node *parsetree) > +{ > + AlterObjectDependsStmt *node = (AlterObjectDependsStmt *) parsetree; > + ObjTree *alterDependeStmt = NULL; > + > + > + if (node->objectType == OBJECT_INDEX) > > Double blank lines? Modified > 122. > > + alterDependeStmt = new_objtree("ALTER INDEX"); > + > + qualified = new_objtree_for_qualname(relation->rd_rel->relnamespace, > + node->relation->relname); > + append_object_object(alterDependeStmt, "%{identity}D", qualified); > > This could be combined into a single VA() call. > > In, fact everything could be if the code it refactored a bit better so > only the assignment for 'qualified' was within the lock. > > SUGGESTION > > qualified = new_objtree_for_qualname(relation->rd_rel->relnamespace, > node->relation->relname); > relation_close(relation, AccessShareLock); > > stmt = new_objtree_VA("ALTER INDEX %{identity}D %{no}s DEPENDS > ON EXTENSION %{newname}I", 3, > "identity", ObjTypeObject, qualified, > "no", ObjTypeString, node->remove ? "NO" : "", > "newname", strVal(node->extname)); Modified > 123. > > + append_string_object(alterDependeStmt, "%{NO}s", > + node->remove ? "NO" : ""); > > IMO it seemed more conventional for the substition marker to be > lowercase. So this should say "%{no}s" instead. Modified > 124. > > AlterObjectDependsStmt *node = (AlterObjectDependsStmt *) parsetree; > ObjTree *alterDependeStmt = NULL; > > Why 'alterDependeStmt' with the extra 'e' -- Is it a typo? Anyway, the > name seems overkill - just 'stmt' would put be fine. Modified > 125. GENERAL comments for all the deparse_Seq_XXX functions > > Comments common for: > - deparse_Seq_Cache > - deparse_Seq_Cycle > - deparse_Seq_IncrementBy > - deparse_Seq_Maxvalue > - deparse_Seq_Minvalue > - deparse_Seq_OwnedBy > - deparse_Seq_Restart > - deparse_Seq_Startwith > > 125a > Most of the deparse_Seq_XXX functions are prefixed with "SET" which is > needed for ALTER TABLE only. > > e.g. > > if (alter_table) > fmt = "SET %{no}s CYCLE"; > else > fmt = "%{no}s CYCLE"; > > IMO all these "SET" additions should be done at the point of the call > when doing the ALTER TABLE instead of polluting all these helper > functions. Remove the alter_table function parameter. In this case we have to create a format string and create an object tree, since SET is part of the format string even if we remove alter_table, we might have to pass SET as format string in that case or we might have to duplicate these deparse_XXX functions for alter table case. I preferred the existing approach unless there is an easier way. > 125b. > IMO it would be tidier with a blank line before the returns. Modified > 125c. > The function parameter *parent is unused. Modified > 126. deparse_RuleStmt > > + ruleStmt = new_objtree("CREATE RULE"); > + > + append_string_object(ruleStmt, "%{or_replace}s", > + node->replace ? "OR REPLACE" : ""); > + > + append_string_object(ruleStmt, "%{identity}I", > + node->rulename); > + > + append_string_object(ruleStmt, "AS ON %{event}s", > + node->event == CMD_SELECT ? "SELECT" : > + node->event == CMD_UPDATE ? "UPDATE" : > + node->event == CMD_DELETE ? "DELETE" : > + node->event == CMD_INSERT ? "INSERT" : "XXX"); > + append_object_object(ruleStmt, "TO %{table}D", > + new_objtree_for_qualname_id(RelationRelationId, > + rewrForm->ev_class)); > + > + append_string_object(ruleStmt, "DO %{instead}s", > + node->instead ? "INSTEAD" : "ALSO"); > > I suspect all of this can be combined to be a single VA() function call. Modified > 127. > > + append_string_object(ruleStmt, "AS ON %{event}s", > + node->event == CMD_SELECT ? "SELECT" : > + node->event == CMD_UPDATE ? "UPDATE" : > + node->event == CMD_DELETE ? "DELETE" : > + node->event == CMD_INSERT ? "INSERT" : "XXX"); > > The bogus "XXX" looks a bit dodgy. Probably it would be better to > assign this 'event_str' separately and Assert/Error if node->event is > not supported. Modified > 128. > > + tmp = new_objtree_VA("WHERE %{clause}s", 0); > + > + if (qual) > + append_string_object(tmp, "clause", qual); > + else > + { > + append_null_object(tmp, "clause"); > + append_bool_object(tmp, "present", false); > + } > + > + append_object_object(ruleStmt, "where_clause", tmp); > > This doesn't look right to me... > > 128a. > Using VA() with 0 args Modified > 128b. > Using null object to fudge substitution to "%{clause}s, is avoidable IMO This code was intended to generate a verbose json node for "where clause". So that user can easily modify the command by changing the value of where clause to generate a new ddl. > 128c. > Shouldn't there be a "%{where_clause}s" on the ruleStmt format? Modified > 129. deparse_CreateTransformStmt > > + createTransform = new_objtree("CREATE"); > + > + append_string_object(createTransform, "%{or_replace}s", > + node->replace ? "OR REPLACE" : ""); > + append_object_object(createTransform, "TRANSFORM FOR %{typename}D", > + new_objtree_for_qualname_id(TypeRelationId, > + trfForm->trftype)); > + append_string_object(createTransform, "LANGUAGE %{language}I", > + NameStr(langForm->lanname)); > > This can all be combined into a single VA() function. Modified > 130. > + /* deparse the transform_element_list */ > + if (trfForm->trffromsql != InvalidOid) > > 130a. > Uppercase comment Modified > 130b. > Use OidIsValid macro. Modified > 131. > > + /* > + * Verbose syntax > + * > + * CREATE %{or_replace}s TRANSFORM FOR %{typename}D LANGUAGE > + * %{language}I ( FROM SQL WITH FUNCTION %{signature}s, TO SQL WITH > + * FUNCTION %{signature_tof}s ) > + * > + * OR > + * > + * CREATE %{or_replace}s TRANSFORM FOR %{typename}D LANGUAGE > + * %{language}I ( TO SQL WITH FUNCTION %{signature_tof}s ) > + */ > + > > According to the PG DOCS [3] *either* part of FROM/TO SQL WITH > FUNCTION are optional. So a "FROM SQL" without a "TO SQL" is also > allowed. So the comment should say this too. Verbose syntax has been mentioned 3 times, I felt it is not required to mention again and again. I have retained it at the beginning and remained the others. > 132. > > There are multiple other places in this code where should use OidIsValid macro. > > e.g. > + if (trfForm->trftosql != InvalidOid) > > e.g. > + /* Append a ',' if trffromsql is present, else append '(' */ > + append_string_object(createTransform, "%{comma}s", > + trfForm->trffromsql != InvalidOid ? "," : "("); Modified > 133. > These strange substitutions could've just use the > append_format_string() function I think. > > 133a > + /* Append a ',' if trffromsql is present, else append '(' */ > + append_string_object(createTransform, "%{comma}s", > + trfForm->trffromsql != InvalidOid ? "," : "("); > > SUGGESTION > append_format_string(createTransform, OidIsValid( trfForm->trffromsql) > "," : "("); Modified > 133b. > + append_string_object(createTransform, "%{close_bracket}s", ")"); > > SUGGESTION > append_format_string(createTransform, ")"); Modified > 134. > + sign = new_objtree(""); > + > + append_object_object(sign, "%{identity}D", > + new_objtree_for_qualname(procForm->pronamespace, > + NameStr(procForm->proname))); > + append_array_object(sign, "(%{arguments:, }s)", params); > + > + append_object_object(createTransform, "TO SQL WITH FUNCTION > %{signature_tof}s", sign); > > 134a. > IIUC it's a bit clunky to parse out this whole fmt looking for a '{' > to extract the name "signature_tof" (maybe it works but there is a lot > of ineficiency hidden under the covers I think), when with some small > refactoring this could be done using a VA() function passing in the > known name. Modified > 134b. > Looks like 'sign' is either a typo or very misleading name. Isn't that > supposed to be the ObjTree for the "signature_tof"? Changed it to signature > 135. append_literal_or_null > > +static void > +append_literal_or_null(ObjTree *mainobj, char *elemname, char *value) > > In other functions 'mainobj' would have been called 'parent'. I think > parent is a better name. Modified > 136. > > + top = new_objtree_VA("", 0); > > Don't use VA() for 0 args. It was already fixed > 137. > > + top = new_objtree_VA("", 0); > + part = new_objtree_VA("NULL", 1, > + "present", ObjTypeBool, > + !value); > + append_object_object(top, "%{null}s", part); > + part = new_objtree_VA("", 1, > + "present", ObjTypeBool, > + !!value); > + if (value) > + append_string_object(part, "%{value}L", value); > + append_object_object(top, "%{literal}s", part); > > 137a. > Suggest to put each VA arg name/value on the same line. > e.g. > + part = new_objtree_VA("NULL", 1, > + "present", ObjTypeBool, !value); Modified > 137b. > The '!!' is cute but seems uncommon technique in PG sources. Maybe > better just say value != NULL Modified > 137c. > Suggest adding a blank line to separate the logic of the 2 parts. > (e.g. above the 2nd part = new_objtree_VA). Modified > 138. deparse_CommentOnConstraintSmt > > + comment = new_objtree("COMMENT ON CONSTRAINT"); > + > + append_string_object(comment, "%{identity}s", > pstrdup(NameStr(constrForm->conname))); > + append_format_string(comment, "ON"); > + > + if (node->objtype == OBJECT_DOMCONSTRAINT) > + append_format_string(comment, "DOMAIN"); > + > + append_string_object(comment, "%{parentobj}s", > + getObjectIdentity(&addr, false)); > > This can mostly be done as a single VA() call I think. Modified > 139. deparse_CommentStmt > > + > +static ObjTree * > +deparse_CommentStmt(ObjectAddress address, Node *parsetree) > > Missing function comment. Modified > 140. > > + comment = new_objtree("COMMENT ON"); > + append_string_object(comment, "%{objtype}s", (char *) > stringify_objtype(node->objtype)); > > A single VA() function call can do this. Modified > 141. deparse_CreateAmStmt > > + > +static ObjTree * > +deparse_CreateAmStmt(Oid objectId, Node *parsetree) > > Missing function comment. Modified > 142. > > + CreateAm = new_objtree("CREATE ACCESS METHOD"); > + append_string_object(CreateAm, "%{identity}I", > + NameStr(amForm->amname)); > + > + switch (amForm->amtype) > + { > + case 'i': > + amtype = "INDEX"; > + break; > + case 't': > + amtype = "TABLE"; > + break; > + default: > + elog(ERROR, "invalid type %c for access method", amForm->amtype); > + } > + append_string_object(CreateAm, "TYPE %{am_type}s", amtype); > + > + /* Add the HANDLER clause */ > + append_object_object(CreateAm, "HANDLER %{handler}D", > + new_objtree_for_qualname_id(ProcedureRelationId, > + amForm->amhandler)); > > This entire thing can be done as a single VA() function call. > > SUGGESTION > > switch (amForm->amtype) > { > case 'i': > amtype = "INDEX"; > break; > case 't': > amtype = "TABLE"; > break; > default: > elog(ERROR, "invalid type %c for access method", amForm->amtype); > } > > createAm = new_objtree_VA("CREATE ACCESS METHOD %{identity}I TYPE > %{am_type}s HANDLER %{handler}D", 3, > "identity", ObjTypeString, NameStr(amForm->amname), > "am_type", ObjTypeString, amtype, > "handler", ObjTypeObject, > new_objtree_for_qualname_id(ProcedureRelationId, amForm->amhandler)); Modified > 143. deparse_simple_command > > + switch (nodeTag(parsetree)) > + { > + case T_CreateSchemaStmt: > + command = deparse_CreateSchemaStmt(objectId, parsetree); > + break; > + > + case T_AlterDomainStmt: > + command = deparse_AlterDomainStmt(objectId, parsetree, > + cmd->d.simple.secondaryObject); > + break; > + > + case T_CreateStmt: > + command = deparse_CreateStmt(objectId, parsetree); > + break; > + > + case T_RefreshMatViewStmt: > + command = deparse_RefreshMatViewStmt(objectId, parsetree); > + break; > + > + case T_CreateTrigStmt: > + command = deparse_CreateTrigStmt(objectId, parsetree); > + break; > + > + case T_RuleStmt: > + command = deparse_RuleStmt(objectId, parsetree); > + break; > + > + case T_CreatePLangStmt: > + command = deparse_CreateLangStmt(objectId, parsetree); > + break; > + > + case T_CreateSeqStmt: > + command = deparse_CreateSeqStmt(objectId, parsetree); > + break; > + > + case T_CreateFdwStmt: > + command = deparse_CreateFdwStmt(objectId, parsetree); > + break; > + > + case T_CreateUserMappingStmt: > + command = deparse_CreateUserMappingStmt(objectId, parsetree); > + break; > + > + case T_AlterUserMappingStmt: > + command = deparse_AlterUserMappingStmt(objectId, parsetree); > + break; > + > + case T_AlterStatsStmt: > + command = deparse_AlterStatsStmt(objectId, parsetree); > + break; > + > + case T_AlterFdwStmt: > + command = deparse_AlterFdwStmt(objectId, parsetree); > + break; > + > + case T_AlterSeqStmt: > + command = deparse_AlterSeqStmt(objectId, parsetree); > + break; > + > + case T_DefineStmt: > + command = deparse_DefineStmt(objectId, parsetree, > + cmd->d.simple.secondaryObject); > + break; > + > + case T_CreateConversionStmt: > + command = deparse_CreateConversion(objectId, parsetree); > + break; > + > + case T_CreateDomainStmt: > + command = deparse_CreateDomain(objectId, parsetree); > + break; > + > + case T_CreateExtensionStmt: > + command = deparse_CreateExtensionStmt(objectId, parsetree); > + break; > + > + case T_AlterExtensionStmt: > + command = deparse_AlterExtensionStmt(objectId, parsetree); > + break; > + > + case T_AlterExtensionContentsStmt: > + command = deparse_AlterExtensionContentsStmt(objectId, parsetree, > + cmd->d.simple.secondaryObject); > + break; > + > + case T_CreateOpFamilyStmt: > + command = deparse_CreateOpFamily(objectId, parsetree); > + break; > + > + case T_CreatePolicyStmt: > + command = deparse_CreatePolicyStmt(objectId, parsetree); > + break; > + > + case T_IndexStmt: > + command = deparse_IndexStmt(objectId, parsetree); > + break; > + > + case T_CreateFunctionStmt: > + command = deparse_CreateFunction(objectId, parsetree); > + break; > + > + case T_AlterFunctionStmt: > + command = deparse_AlterFunction(objectId, parsetree); > + break; > + > + case T_AlterCollationStmt: > + command = deparse_AlterCollation(objectId, parsetree); > + break; > + > + case T_RenameStmt: > + command = deparse_RenameStmt(cmd->d.simple.address, parsetree); > + break; > + > + case T_AlterObjectDependsStmt: > + command = deparse_AlterDependStmt(objectId, parsetree); > + break; > + > + case T_AlterObjectSchemaStmt: > + command = deparse_AlterObjectSchemaStmt(cmd->d.simple.address, > + parsetree, > + cmd->d.simple.secondaryObject); > + break; > + > + case T_AlterOwnerStmt: > + command = deparse_AlterOwnerStmt(cmd->d.simple.address, parsetree); > + break; > + > + case T_AlterOperatorStmt: > + command = deparse_AlterOperatorStmt(objectId, parsetree); > + break; > + > + case T_AlterPolicyStmt: > + command = deparse_AlterPolicyStmt(objectId, parsetree); > + break; > + > + case T_AlterTypeStmt: > + command = deparse_AlterTypeSetStmt(objectId, parsetree); > + break; > + > + case T_CreateStatsStmt: > + command = deparse_CreateStatisticsStmt(objectId, parsetree); > + break; > + > + case T_CreateForeignServerStmt: > + command = deparse_CreateForeignServerStmt(objectId, parsetree); > + break; > + > + case T_AlterForeignServerStmt: > + command = deparse_AlterForeignServerStmt(objectId, parsetree); > + break; > + > + case T_CompositeTypeStmt: > + command = deparse_CompositeTypeStmt(objectId, parsetree); > + break; > + > + case T_CreateEnumStmt: /* CREATE TYPE AS ENUM */ > + command = deparse_CreateEnumStmt(objectId, parsetree); > + break; > + > + case T_CreateRangeStmt: /* CREATE TYPE AS RANGE */ > + command = deparse_CreateRangeStmt(objectId, parsetree); > + break; > + > + case T_AlterEnumStmt: > + command = deparse_AlterEnumStmt(objectId, parsetree); > + break; > + > + case T_CreateCastStmt: > + command = deparse_CreateCastStmt(objectId, parsetree); > + break; > + > + case T_AlterTSDictionaryStmt: > + command = deparse_AlterTSDictionaryStmt(objectId, parsetree); > + break; > + > + case T_CreateTransformStmt: > + command = deparse_CreateTransformStmt(objectId, parsetree); > + break; > + > + case T_ViewStmt: /* CREATE VIEW */ > + command = deparse_ViewStmt(objectId, parsetree); > + break; > + > + case T_CreateTableAsStmt: /* CREATE MATERIALIZED VIEW */ > + command = deparse_CreateTableAsStmt_vanilla(objectId, parsetree); > + break; > + > + case T_CommentStmt: > + command = deparse_CommentStmt(cmd->d.simple.address, parsetree); > + break; > + > + case T_CreateAmStmt: > + command = deparse_CreateAmStmt(objectId, parsetree); > + break; > > 143a. > Suggestion to put all these cases in alphabetical order. Modified > 143b. > Suggest removing the variable 'command' and for each case just return > the deparse_XXX result -- doing this will eliminate the need for > "break;" and so the function can be 50 lines shorter. Modified > 144. deparse_TableElements > > + if (tree != NULL) > + { > + ObjElem *column; > + > + column = new_object_object(tree); > + elements = lappend(elements, column); > + } > > Why do all this instead of just: > > if (tree != NULL) > elements = lappend(elements, new_object_object(tree)); Modified > 145. deparse_utility_command > > + if (tree) > + { > + Jsonb *jsonb; > + > + jsonb = objtree_to_jsonb(tree); > + command = JsonbToCString(&str, &jsonb->root, JSONB_ESTIMATED_LEN); > + } > + else > + command = NULL; > > 145a. > Since 'tree' is always assigned the result of deparse_XXX I am > wondering if tree == NULL is even possible here? If not then this > if/else should be an Assert instead. This is required as the tree can be NULL like in the below case: /* * Indexes for PRIMARY KEY and other constraints are output * separately; return empty here. */ > 145b. > Anyway, maybe assign default command = NULL in the declaration to > reduce a couple of lines of unnecessary code. Modified Thanks for the comments, the attached v39 patch has the changes for the same. Regards, Vignesh
Вложения
В списке pgsql-hackers по дате отправления: