Re: Support logical replication of DDLs - Mailing list pgsql-hackers
From | Peter Smith |
---|---|
Subject | Re: Support logical replication of DDLs |
Date | |
Msg-id | CAHut+PvBzyJuKqMDUOs4AZPSKqNaNfQBLXsFn-O_OZqyXeLv+A@mail.gmail.com Whole thread Raw |
In response to | Re: Support logical replication of DDLs (Peter Smith <smithpb2250@gmail.com>) |
Responses |
Re: Support logical replication of DDLs
|
List | pgsql-hackers |
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" ~ 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. ~ 101. + /* add an OPTIONS clause, if any */ Uppercase comment. ------ 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" ~ 102b. Unnecessary blank line. ~ 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. ~ 104. + /* add an OPTIONS clause, if any */ Uppercase comment ------ 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. ~ 105b. All new_objtreee and append_XXX can be combined as a single VA() function call here. ------ 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. ~ 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 ~ 106c. Most/all of this can be combined into a single VA call. ------ 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); ~ 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. ------ 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' would be better here. ~ 109b. The 2 lines of cascade should be grouped together -- i.e. the blank line should be *above* the "CASCADE", not below it. ------ 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". ~ 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) ------ 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. ------ 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? ------ 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. ~ 114b. Anyway, all these can be combined to a single new_objtree_VA() call. ~ 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. ~ 116. relation_close(pg_policy, AccessShareLock); } break; case OBJECT_ATTRIBUTE: Spurious blank line before the } ~ 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. ~ 118. + relation_close(relation, AccessShareLock); + + break; + case OBJECT_FUNCTION: The misplaced blank line should be *after* the break; not before it. ~ 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). ~ 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' ~ 120b. All this can be combined as a single VA() call. ------ 121. deparse_AlterDependStmt +deparse_AlterDependStmt(Oid objectId, Node *parsetree) +{ + AlterObjectDependsStmt *node = (AlterObjectDependsStmt *) parsetree; + ObjTree *alterDependeStmt = NULL; + + + if (node->objectType == OBJECT_INDEX) Double blank lines? ~ 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)); ~ 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. ~ 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. ------ 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. ~ 125b. IMO it would be tidier with a blank line before the returns. ~ 125c. The function parameter *parent is unused. ------ 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. ~ 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. ~ 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 ~ 128b. Using null object to fudge substitution to "%{clause}s, is avoidable IMO ~ 128c. Shouldn't there be a "%{where_clause}s" on the ruleStmt format? ------ 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. ~ 130. + /* deparse the transform_element_list */ + if (trfForm->trffromsql != InvalidOid) 130a. Uppercase comment ~ 130b. Use OidIsValid macro. ~ 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. ~ 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 ? "," : "("); ~ 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) "," : "("); ~ 133b. + append_string_object(createTransform, "%{close_bracket}s", ")"); SUGGESTION append_format_string(createTransform, ")"); ~ 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. ~ 134b. Looks like 'sign' is either a typo or very misleading name. Isn't that supposed to be the ObjTree for the "signature_tof"? ------ 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. ~ 136. + top = new_objtree_VA("", 0); Don't use VA() for 0 args. ~ 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); ~ 137b. The '!!' is cute but seems uncommon technique in PG sources. Maybe better just say value != NULL ~ 137c. Suggest adding a blank line to separate the logic of the 2 parts. (e.g. above the 2nd part = new_objtree_VA). ------ 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. ------ 139. deparse_CommentStmt + +static ObjTree * +deparse_CommentStmt(ObjectAddress address, Node *parsetree) Missing function comment. ~ 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. ------ 141. deparse_CreateAmStmt + +static ObjTree * +deparse_CreateAmStmt(Oid objectId, Node *parsetree) Missing function comment. ~ 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)); ------ 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. ~ 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. ------ 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)); ------ 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. ~ 145b. Anyway, maybe assign default command = NULL in the declaration to reduce a couple of lines of unnecessary code. ------ [3] CREATE TRANSFORM - https://www.postgresql.org/docs/current/sql-createtransform.html Kind Regards, Peter Smith. Fujitsu Australia
pgsql-hackers by date: