Re: Support logical replication of DDLs

On Fri, Nov 11, 2022 at 4:09 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Fri, Nov 11, 2022 at 3:47 PM Peter Smith <smithpb2250(at)gmail(dot)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 3 OF 4.

=======

src/backend/commands/ddl_deparse.c

50. deparse_DefineStmt_Operator

+/*
+ * Deparse a DefineStmt (CREATE OPERATOR)
+ *
+ * Given a trigger OID and the parse tree that created it, return an ObjTree
+ * representing the creation command.
+ */
+static ObjTree *
+deparse_DefineStmt_Operator(Oid objectId, DefineStmt *define)

"trigger OID" ?? Is that right?

~

51.

/* MERGES */
tmpobj = new_objtree_VA("MERGES", 1,
"clause", ObjTypeString, "merges");
if (!oprForm->oprcanmerge)
append_bool_object(tmpobj, "present", false);
list = lappend(list, new_object_object(tmpobj));

/* HASHES */
tmpobj = new_objtree_VA("HASHES", 1,
"clause", ObjTypeString, "hashes");
if (!oprForm->oprcanhash)
append_bool_object(tmpobj, "present", false);
list = lappend(list, new_object_object(tmpobj));

Maybe HASHES and MERGES should be done in a different order, just to
be consistent with the PG documentation [2].

------

52. deparse_DefineStmt_Type

+ /* Shortcut processing for shell types. */
+ if (!typForm->typisdefined)
+ {
+ stmt = new_objtree_VA("CREATE TYPE", 0);
+ append_object_object(stmt, "%{identity}D",
+ new_objtree_for_qualname(typForm->typnamespace,
+ NameStr(typForm->typname)));
+ append_bool_object(stmt, "present", true);
+ ReleaseSysCache(typTup);
+ return stmt;
+ }
+
+ stmt = new_objtree_VA("CREATE TYPE", 0);
+ append_object_object(stmt, "%{identity}D",
+ new_objtree_for_qualname(typForm->typnamespace,
+ NameStr(typForm->typname)));
+ append_bool_object(stmt, "present", true);

52a.
This code looked strange because everything is the same except the
Release/return, so IMO it should be refactored to use the common code.

~

52b.
The VA(0 args) should be combined with the subsequent appends to use
fewer append_XXX calls.

~

53.
Is it necessary to say append_bool_object(stmt, "present", true); ? --
I'd assumed that is the default unless it explicitly says false.

~

54.

/* INPUT */
tmp = new_objtree_VA("(INPUT=", 1,
"clause", ObjTypeString, "input");
append_object_object(tmp, "%{procedure}D",
new_objtree_for_qualname_id(ProcedureRelationId,
typForm->typinput));
list = lappend(list, new_object_object(tmp));

/* OUTPUT */
tmp = new_objtree_VA("OUTPUT=", 1,
"clause", ObjTypeString, "output");
append_object_object(tmp, "%{procedure}D",
new_objtree_for_qualname_id(ProcedureRelationId,
typForm->typoutput));
list = lappend(list, new_object_object(tmp));

These could each be simplified into single VA() function calls, the
same as was done in deparse_DefineStmt_Operator PROCEDURE.

And the same comment applies to other parts. e.g.:
- /* CATEGORY */
- /* ALIGNMENT */
- STORAGE

~

55.

+ tmp = new_objtree_VA("STORAGE=", 1,
+ "clause", ObjTypeString, "storage");

Missing comment above this to say /* STORAGE */

~

56.

+ /* INTERNALLENGTH */
+ if (typForm->typlen == -1)
+ {
+ tmp = new_objtree_VA("INTERNALLENGTH=VARIABLE", 0);
+ }
+ else
+ {
+ tmp = new_objtree_VA("INTERNALLENGTH=%{typlen}n", 1,
+ "typlen", ObjTypeInteger, typForm->typlen);
+ }

56a.
The VA(args = 0) does not need to be a VA function.

~

56b.
The { } blocks are unnecessary

------

57. deparse_DefineStmt_TSConfig

+
+static ObjTree *
+deparse_DefineStmt_TSConfig(Oid objectId, DefineStmt *define,
+ ObjectAddress copied)

Missing function comment.

~

58.

+ stmt = new_objtree("CREATE");
+
+ append_object_object(stmt, "TEXT SEARCH CONFIGURATION %{identity}D",
+ new_objtree_for_qualname(tscForm->cfgnamespace,
+ NameStr(tscForm->cfgname)));

Why not combine these using VA() function?

~

59.

+ list = NIL;
+ /* COPY */

Just assign NIL when declared.

~

60.

+ if (copied.objectId != InvalidOid)

Use OidIsValid macro.

------

61. deparse_DefineStmt_TSParser

+
+static ObjTree *
+deparse_DefineStmt_TSParser(Oid objectId, DefineStmt *define)

Missing function comment.

~

62.

+ stmt = new_objtree("CREATE");
+
+ append_object_object(stmt, "TEXT SEARCH PARSER %{identity}D",
+ new_objtree_for_qualname(tspForm->prsnamespace,
+ NameStr(tspForm->prsname)));

Why not combine as a single VA() function call?

~

63.

+ list = NIL;

Just assign NIL when declared

~

64.

tmp = new_objtree_VA("START=", 1,
"clause", ObjTypeString, "start");
append_object_object(tmp, "%{procedure}D",
new_objtree_for_qualname_id(ProcedureRelationId,
tspForm->prsstart));

Easily combined to be a single VA() function call.

The same comment applies for
- /* GETTOKEN */
- /* END */
- /* LEXTYPES */

------

65. deparse_DefineStmt_TSDictionary

+static ObjTree *
+deparse_DefineStmt_TSDictionary(Oid objectId, DefineStmt *define)

Missing function comment.

~

66.

+ stmt = new_objtree("CREATE");
+
+ append_object_object(stmt, "TEXT SEARCH DICTIONARY %{identity}D",
+ new_objtree_for_qualname(tsdForm->dictnamespace,
+ NameStr(tsdForm->dictname)));

Why not combine this as a single VA() function call?

~

67.

+ list = NIL;

Just assign NIL when declared

~

68.

+ tmp = new_objtree_VA("", 0);

Don't need VA() function for 0 args.

------

69. deparse_DefineStmt_TSTemplate

+static ObjTree *
+deparse_DefineStmt_TSTemplate(Oid objectId, DefineStmt *define)

Missing function comment.

~

70.

+ stmt = new_objtree("CREATE");
+
+ append_object_object(stmt, "TEXT SEARCH TEMPLATE %{identity}D",
+ new_objtree_for_qualname(tstForm->tmplnamespace,
+ NameStr(tstForm->tmplname)));

Combine this to be a single VA() function call.

~

71.

+ list = NIL;

Just assign NIL when declared

~

72.

+ tmp = new_objtree_VA("LEXIZE=", 1,
+ "clause", ObjTypeString, "lexize");
+ append_object_object(tmp, "%{procedure}D",
+ new_objtree_for_qualname_id(ProcedureRelationId,
+ tstForm->tmpllexize));

Combine this to be a single VA() function call.

------

73. deparse_AlterTSConfigurationStmt

+static ObjTree *
+deparse_AlterTSConfigurationStmt(CollectedCommand *cmd)

Missing function comment.

~

74.

+ /* determine the format string appropriate to each subcommand */
+ switch (node->kind)

Uppercase comment

~

75.

+ tmp = new_objtree_VA("IF EXISTS", 0);

Should not use a VA() function with 0 args.

~

76.

+ case ALTER_TSCONFIG_ALTER_MAPPING_FOR_TOKEN:
+ append_object_object(config, "%{identity}D ALTER MAPPING",
+ new_objtree_for_qualname_id(cmd->d.atscfg.address.classId,
+ cmd->d.atscfg.address.objectId));
+ break;
+
+ case ALTER_TSCONFIG_REPLACE_DICT:
+ append_object_object(config, "%{identity}D ALTER MAPPING",
+ new_objtree_for_qualname_id(cmd->d.atscfg.address.classId,
+ cmd->d.atscfg.address.objectId));
+ break;
+
+ case ALTER_TSCONFIG_REPLACE_DICT_FOR_TOKEN:
+ append_object_object(config, "%{identity}D ALTER MAPPING",
+ new_objtree_for_qualname_id(cmd->d.atscfg.address.classId,
+ cmd->d.atscfg.address.objectId));
+ break;

If all these 3 cases have identical code then why repeat it three times?

~

77.

+ /* add further subcommand-specific elements */

Uppercase comment

~

78.

+ /* the REPLACE forms want old and new dictionaries */
+ Assert(cmd->d.atscfg.ndicts == 2);

Uppercase comment.

------

79. deparse_AlterTSDictionaryStmt

+
+static ObjTree *
+deparse_AlterTSDictionaryStmt(Oid objectId, Node *parsetree)

Missing function comment

~

80.

+ alterTSD = new_objtree("ALTER TEXT SEARCH DICTIONARY");
+
+ append_object_object(alterTSD, "%{identity}D",
+ new_objtree_for_qualname(tsdForm->dictnamespace,
+ NameStr(tsdForm->dictname)));

Combine this as a sing VA() function call

~

81.

+ tmp = new_objtree_VA("", 0);

Don't use the VA() function for 0 args.

------

82. deparse_RelSetOptions

+ if (is_reset)
+ fmt = "RESET ";
+ else
+ fmt = "SET ";
+
+ relset = new_objtree(fmt);

82a.
Those format trailing spaces are a bit unusual. The append_XXX will
take care of space separators anyhow so it is not needed like this.

~

82b.
This can all be simplified to one line:

relset = new_objtree(is_reset ? "RESET" : "SET");

------

83. deparse_ViewStmt

+ * Given a view OID and the parsetree that created it, return an ObjTree
+ * representing the creation command.
+ */

Be consistent with other function headers:

"parsetree" -> "parse tree".

~

84.

+ viewStmt = new_objtree("CREATE");
+
+ append_string_object(viewStmt, "%{or_replace}s",
+ node->replace ? "OR REPLACE" : "");
+
+ append_string_object(viewStmt, "%{persistence}s",
+ get_persistence_str(relation->rd_rel->relpersistence));
+
+ tmp = new_objtree_for_qualname(relation->rd_rel->relnamespace,
+ RelationGetRelationName(relation));
+
+ append_object_object(viewStmt, "VIEW %{identity}D", tmp);
+
+ append_string_object(viewStmt, "AS %{query}s",
+ pg_get_viewdef_internal(objectId));

IMO all of this can be combined in a single VA() function call.

------

85. deparse_CreateTableAsStmt_vanilla

+/*
+ * Deparse CREATE Materialized View statement, it is a variant of
CreateTableAsStmt
+ *
+ * Note that CREATE TABLE AS SELECT INTO can also be deparsed by
+ * deparse_CreateTableAsStmt to remove the SELECT INTO clause.
+ */
+static ObjTree *
+deparse_CreateTableAsStmt_vanilla(Oid objectId, Node *parsetree)

The function comment refers to 'deparse_CreateTableAsStmt' but I don't
see any such function. Maybe this was renamed causing the comment
became stale?

~

86.

+ /* Add identity */
+ append_object_object(createStmt, "%{identity}D",
+ new_objtree_for_qualname_id(RelationRelationId,
+ objectId));

This could be included as another arg of the preceding VA() call/

~

87.

+ /* COLLUMNS clause */
+ if (node->into->colNames == NIL)
+ tmp = new_objtree_VA("", 1,
+ "present", ObjTypeBool, false);
+ else

87a.
Typo "COLLUMNS"

~

87b.
It might be more usual/natural to reverse this if/else to check the
list is NOT empty. e.g.

if (node->into->colNames)
...
else
tmp = new_objtree_VA("", 1,
"present", ObjTypeBool, false);

~

88.

+ tmp = new_objtree("USING");
+ if (node->into->accessMethod)
+ append_string_object(tmp, "%{access_method}I", node->into->accessMethod);
+ else
+ {
+ append_null_object(tmp, "%{access_method}I");
+ append_bool_object(tmp, "present", false);
+ }

I'm not sure why a null object is necessary when present = false.

~

89.

+ /* WITH clause */
+ tmp = new_objtree_VA("WITH", 0);

VA() function call is not needed when there are 0 args.

~

90.

+ /* TABLESPACE clause */
+ tmp = new_objtree_VA("TABLESPACE %{tablespace}I", 0);

VA() function call nor needed when there are 0 args.

~

91.

+ else
+ {
+ append_null_object(tmp, "%{tablespace}I");
+ append_bool_object(tmp, "present", false);
+ }

I'm not sure why a null object is necessary when present = false.

~

92.

+ /* add a WITH NO DATA clause */
+ tmp = new_objtree_VA("WITH NO DATA", 1,
+ "present", ObjTypeBool,
+ node->into->skipData ? true : false);
+ append_object_object(createStmt, "%{with_no_data}s", tmp);

92a.
Uppercase comment.

~

92b.
It is a bit confusing that this style of specifying empty tree (just
saying present/not present) is used here. But elsewhere in this patch
for similar syntax it just adds text or an empty string.
e.g.
+ append_string_object(renameStmt, "%{if_exists}s",
+ node->missing_ok ? "IF EXISTS" : "");

IMO it's better to apply a consistent deparse approach for everything.
But without documentation of the deparse structure, it is kind of
impossible to know even what *are* the rules?

------

93. deparse_CreateTrigStmt

+ trigger = new_objtree("CREATE");
+
+ append_string_object(trigger, "%{constraint}s",
+ node->isconstraint ? "CONSTRAINT" : "");
+
+ append_string_object(trigger, "TRIGGER %{name}I", node->trigname);

All this can be combined into a single VA() call.

~

94.

+ if (node->timing == TRIGGER_TYPE_BEFORE)
+ append_string_object(trigger, "%{time}s", "BEFORE");
+ else if (node->timing == TRIGGER_TYPE_AFTER)
+ append_string_object(trigger, "%{time}s", "AFTER");
+ else if (node->timing == TRIGGER_TYPE_INSTEAD)
+ append_string_object(trigger, "%{time}s", "INSTEAD OF");
+ else
+ elog(ERROR, "unrecognized trigger timing type %d", node->timing);

It might be better to assign the value to a char* and then just have
only a single append_string_object() call.

char *tval =
node->timing == TRIGGER_TYPE_BEFORE ? "BEFORE" :
node->timing == TRIGGER_TYPE_AFTER ? "AFTER" :
node->timing == TRIGGER_TYPE_INSTEAD ? "INSTEAD OF" :
NULL;
if (tval == NULL)
elog(ERROR, "unrecognized trigger timing type %d", node->timing);
append_string_object(trigger, "%{time}s", tval);

~

95.

+ tmpobj = new_objtree_VA("FROM", 0);

VA() function call is not needed for 0 args.

~

96.

+ tmpobj = new_objtree_VA("WHEN", 0);

VA() function call is not needed for 0 args.

~

97.

Should use consistent wording for unexpected nulls.

e.g.1
+ if (isnull)
+ elog(ERROR, "bogus NULL tgqual");

e.g.2
+ if (isnull)
+ elog(ERROR, "invalid NULL tgargs");

~

98.

+ append_format_string(tmpobj, "(");
+ append_array_object(tmpobj, "%{args:, }L", list); /* might be NIL */
+ append_format_string(tmpobj, ")");

IMO probably that can be a single call to append_array_object which
includes the enclosing parens.

------
[2] CREATE OPERATOR -
/docs/current/sql-createoperator.html

Kind Regards,
Peter Smith.
Fujitsu Australia