Re: Support logical replication of DDLs - Mailing list pgsql-hackers

The patches here are quite large, so for this review post, I have only
done a quick check for cosmetic stuff in the comments of patch
v24-0001.

~

I did this mostly just by cutting/pasting the whole patch text into a
grammar/spell-checker to see what it reported. Please use the same
strategy prior to posting future patch versions, because it will be
way more efficient/easier for the author to spend a few minutes to fix
everything like this up-front before posting, rather than getting a
flood of review comments to deal with (like this post) about such
stuff.

(BTW, most of these suggestions are just verbatim what my
grammar/spell-checker told me)

======

1. Commit comment

(Note #2) Note that, for ATTACH/DETACH PARTITION, we haven't added
extra logic on
subscriber to handle the case where the table on publisher is a PARTITIONED
TABLE while the target table on subcriber side is NORMAL table. We will
research this more and improve this later.

SUGGESTION (minor changes + fix typo)
(Note #2) Note that for ATTACH/DETACH PARTITION we haven't added extra logic on
the subscriber to handle the case where the table on the publisher is
a PARTITIONED
TABLE while the target table on the subscriber is a NORMAL table. We will
research this more and improve it later.

======

2. GENERAL - uppercase the comments

Please make all your single-line comments start with uppercase for consistency.

Here are some examples to fix:

Line  303: + /* add the "ON table" clause */
Line  331: + /* add the USING clause, if any */
Line  349: + /* add the WITH CHECK clause, if any */
Line  653: + /* otherwise, WITH TZ is added by typmod. */
Line  663: + /* otherwise, WITH TZ is added by typmode. */
Line 1946: + /* build list of privileges to grant/revoke */
Line 2017: + /* target objects.  We use object identities here */
Line 2041: + /* list of grantees */
Line 2053: + /* the wording of the grant option is variable ... */
Line 2632: + /* skip this one; we add one unconditionally below */
Line 2660: + /* done */
Line 2768: + /* add HANDLER clause */
Line 2780: + /* add VALIDATOR clause */
Line 2792: + /* add an OPTIONS clause, if any */
Line 2845: + /* add HANDLER clause */
Line 2859: + /* add VALIDATOR clause */
Line 2877: + /* add an OPTIONS clause, if any */
Line 5024: + /* add the rest of the stuff */
Line 5040: + /* add the rest of the stuff */
Line 5185: + /* a new constraint has a name and definition */
Line 5211: + /* done */
Line 6124: + /* add a CONCURRENTLY clause */
Line 6127: + /* add the matview name */
Line 6131: + /* add a WITH NO DATA clause */
Line 6302: + /* reloptions */
Line 6310: + /* tablespace */
Line 6592: + /* deconstruct the name list */
Line 6636: + /* deconstruct the name list */
Line 6725: + /* obtain object tuple */
Line 6729: + /* obtain namespace */

------

3.  Grammar/Spelling

3a. - format_type_detailed

+ * - nspid is the schema OID.  For certain SQL-standard types which have weird
+ *   typmod rules, we return InvalidOid; caller is expected to not schema-
+ *   qualify the name nor add quotes to the type name in this case.

"caller" -> "the caller"

~

3b. - format_type_detailed

+ * - typemodstr is set to the typemod, if any, as a string with parens

"parens" -> "parentheses"

~

3c. - format_type_detailed

+ else
+ /* otherwise, WITH TZ is added by typmode. */
+ *typname = pstrdup("TIME");

"typmode" -> "typmod" ?

~

3d. - new_objtree_for_qualname

+ * A helper routine to setup %{}D and %{}O elements.

"setup" -> "set up"

~

3e. - new_objtree_for_type

+/*
+ * A helper routine to setup %{}T elements.
+ */
+static ObjTree *
+new_objtree_for_type(Oid typeId, int32 typmod)

"setup" -> "set up"

~

3f. - pg_get_indexdef_detailed
+/*
+ * Return an index definition, split in several pieces.
+ *
+ * A large amount of code is duplicated from  pg_get_indexdef_worker, but
+ * control flow is different enough that it doesn't seem worth keeping them
+ * together.
+ */
+static void
+pg_get_indexdef_detailed(Oid indexrelid,

"split in" -> "split into"

~

3g. - ddl_deparse_to_json

+ * The command is expanded fully, so that there are no ambiguities even in the
+ * face of search_path changes.
+ */
+Datum
+ddl_deparse_to_json(PG_FUNCTION_ARGS)

"fully, so" -> "fully so"

~

3h. -deparse_AlterFunction

+ * Given a function OID and the parsetree that created it, return the JSON
+ * blob representing the alter command.
+ */

"the parsetree" -> "the parse tree"

~

3i. - deparse_AlterObjectSchemaStmt

+/*
+ * deparse an ALTER ... SET SCHEMA command.
+ */
+static ObjTree *
+deparse_AlterObjectSchemaStmt(ObjectAddress address, Node *parsetree,

"deparse" -> "Deparse"

~

3j. deparse_AlterObjectSchemaStmt

+ /*
+ * Since the command has already taken place from the point of view of
+ * catalogs, getObjectIdentity returns the object name with the already
+ * changed schema.  The output of our deparsing must return the original
+ * schema name however, so we chop the schema name off the identity string
+ * and then prepend the quoted schema name.

"name however," -> "name, however,"

~

3k. - deparse_GrantStmt

+ /* build list of privileges to grant/revoke */
+ if (istmt->all_privs)

"build list" -> "build a list"

~

3l. - deparse_AlterTypeSetStmt

+ * Deparse an AlterTypeStmt.
+ *
+ * Given a type OID and a parsetree that modified it, return an ObjTree
+ * representing the alter type.
+ */
+static ObjTree *
+deparse_AlterTypeSetStmt(Oid objectId, Node *cmd)

"parsetree" -> "parse tree"

~

3m. - deparse_CompositeTypeStmt

+ * Deparse a CompositeTypeStmt (CREATE TYPE AS)
+ *
+ * Given a type OID and the parsetree that created it, return an ObjTree
+ * representing the creation command.
+ */

"parsetree" -> "parse tree"

~

3n. - deparse_CreateEnumStmt

+/*
+ * Deparse a CreateEnumStmt (CREATE TYPE AS ENUM)
+ *
+ * Given a type OID and the parsetree that created it, return an ObjTree
+ * representing the creation command.
+ */

"parsetree" -> "parse tree"

~

3o. - deparse_CreateExtensionStmt

+/*
+ * deparse_CreateExtensionStmt
+ * deparse a CreateExtensionStmt
+ *
+ * Given an extension OID and the parsetree that created it, return the JSON
+ * blob representing the creation command.
+ *
+ */

"parsetree" -> "parse tree"

~

3p. - deparse_CreateFdwStmt

+/*
+ * deparse_CreateFdwStmt
+ *   Deparse a CreateFdwStmt (CREATE FOREIGN DATA WRAPPER)
+ *
+ * Given a trigger OID and the parsetree that created it,
+ * return an ObjTree representing the creation command.
+ */

"parsetree" -> "parse tree"

~

3q. - deparse_AlterFdwStmt

+/*
+ * deparse_AlterFdwStmt
+ *  Deparse an AlterFdwStmt (ALTER FOREIGN DATA WRAPPER)
+ *
+ * Given a function OID and the parsetree that create it, return the
+ * JSON blob representing the alter command.
+ */

"parsetree" -> "parse tree"

"create it" -> "created it"

3r.

+ /*
+ * Iterate through options, to see what changed, but use catalog as basis
+ * for new values.
+ */

"as basis" -> "as a basis"

~

3s.

+/*
+ * Deparse a CREATE TYPE AS RANGE statement
+ *
+ * Given a type OID and the parsetree that created it, return an ObjTree
+ * representing the creation command.
+ */

"parsetree" -> "parse tree"

~

3t. - deparse_AlterEnumStmt

+/*
+ * Deparse an AlterEnumStmt.
+ *
+ * Given a type OID and a parsetree that modified it, return an ObjTree
+ * representing the alter type.
+ */

"parsetree" -> "parse tree"

~

3u. - deparse_AlterTableStmt

+ /*
+ * We don't support replicating ALTER TABLE which contains volatile
+ * functions because It's possible the functions contain DDL/DML in
+ * which case these opertions will be executed twice and cause
+ * duplicate data. In addition, we don't know whether the tables being
+ * accessed by these DDL/DML are published or not. So blindly allowing
+ * such functions can allow unintended clauses like the tables accessed
+ * in those functions may not even exist on the subscriber-side.
+ */

"opertions" -> "operations"

"subscriber-side." -> "subscriber."

~

3v. - deparse_ColumnDef

+ * Deparse a ColumnDef node within a regular (non typed) table creation.

"non typed" -> "non-typed"

~

3w. - deparse_ColumnDef

+ /*
+ * Emit a NOT NULL declaration if necessary.  Note that we cannot trust
+ * pg_attribute.attnotnull here, because that bit is also set when
+ * primary keys are specified; and we must not emit a NOT NULL
+ * constraint in that case, unless explicitely specified.  Therefore,
+ * we scan the list of constraints attached to this column to determine
+ * whether we need to emit anything.
+ * (Fortunately, NOT NULL constraints cannot be table constraints.)
+ *
+ * In the ALTER TABLE cases, we also add a NOT NULL if the colDef is
+ * marked is_not_null.
+ */

"specified; and we" -> "specified; we"

"explicitely" -> "explicitly"


~

3x. - deparse_CreateDomain

+/*
+ * Deparse the CREATE DOMAIN
+ *
+ * Given a function OID and the parsetree that created it, return the JSON
+ * blob representing the creation command.
+ */

"parsetree" -> "parse tree"

~

3y. - deparse_CreateFunction

+/*
+ * Deparse a CreateFunctionStmt (CREATE FUNCTION)
+ *
+ * Given a function OID and the parsetree that created it, return the JSON
+ * blob representing the creation command.
+ */

"parsetree" -> "parse tree"

~

3z. - deparse_CreateFunction

+ /*
+ * Now iterate over each parameter in the parsetree to create the
+ * parameters array.
+ */

"parsetree" -> "parse tree"

~

4a. - deparse_CreateFunction

+ /*
+ * A PARAM_TABLE parameter indicates end of input arguments; the
+ * following parameters are part of the return type.  We ignore them
+ * here, but keep track of the current position in the list so that
+ * we can easily produce the return type below.
+ */

"end" -> "the end"

~

4b. - deparse_CreateOpClassStmt

+ /* Don't deparse sql commands generated while creating extension */
+ if (cmd->in_extension)
+ return NULL;

"sql" -> "SQL"

~

4c. - deparse_CreateOpClassStmt

/*
+ * Add the FAMILY clause; but if it has the same name and namespace as the
+ * opclass, then have it expand to empty, because it would cause a failure
+ * if the opfamily was created internally.
+ */

"clause; " -> "clause, "

"empty," -> "empty"

~

4d. - deparse_CreateOpFamily

+/*
+ * Deparse a CreateTrigStmt (CREATE OPERATOR FAMILY)
+ *
+ * Given a trigger OID and the parsetree that created it, return an ObjTree
+ * representing the creation command.
+ */

"parsetree" -> "parse tree"

~

4e. - deparse_CreateSchemaStmt

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

"parsetree" -> "parse tree"

~

4f. - deparse_CreateSeqStmt

+/*
+ * Deparse a CreateSeqStmt.
+ *
+ * Given a sequence OID and the parsetree that create it, return an ObjTree
+ * representing the creation command.
+ */

"parsetree" -> "parse tree"

"create it" -> "created it"

~

4g. - deparse_CreateStmt

+/*
+ * Deparse a CreateStmt (CREATE TABLE).
+ *
+ * Given a table OID and the parsetree that created it, return an ObjTree
+ * representing the creation command.
+ */

"parsetree" -> "parse tree"

~

4h.

+ /*
+ * Typed tables and partitions use a slightly different format string: we
+ * must not put table_elements with parents directly in the fmt string,
+ * because if there are no options the parens must not be emitted; and
+ * also, typed tables do not allow for inheritance.
+ */

"parens" -> "parentheses"

~

4i. - deparse_CreateStmt

+ /*
+ * We can't put table elements directly in the fmt string as an array
+ * surrounded by parens here, because an empty clause would cause a
+ * syntax error.  Therefore, we use an indirection element and set
+ * present=false when there are no elements.
+ */

"parens" -> "parentheses"

~

4j. - deparse_CreateStmt

+ /*
+ * Get pg_class.relpartbound. We cannot use partbound in the
+ * parsetree directly as it's the original partbound expression
+ * which haven't been transformed.
+ */

"parsetree" -> "parse tree" ? maybe this one if ok if it referring to
the parameter with this name.

~

4k. - deparse_DefineStmt_Operator

+/*
+ * Deparse a DefineStmt (CREATE OPERATOR)
+ *
+ * Given a trigger OID and the parsetree that created it, return an ObjTree
+ * representing the creation command.
+ */

"parsetree" -> "parse tree"

~

4l. - deparse_CreateTrigStmt

+/*
+ * Deparse a CreateTrigStmt (CREATE TRIGGER)
+ *
+ * Given a trigger OID and the parsetree that created it, return an ObjTree
+ * representing the creation command.
+ */

"parsetree" -> "parsetree"

~

4m. - deparse_RefreshMatViewStmt

+/*
+ * Deparse a RefreshMatViewStmt (REFRESH MATERIALIZED VIEW)
+ *
+ * Given a materialized view OID and the parsetree that created it, return an
+ * ObjTree representing the refresh command.
+ */

"parseree" -> "parse tree"

~

4n. - deparse_IndexStmt

+/*
+ * Deparse an IndexStmt.
+ *
+ * Given an index OID and the parsetree that created it, return an ObjTree
+ * representing the creation command.
+ *
+ * If the index corresponds to a constraint, NULL is returned.
+ */

"parsetree" -> "parse tree"

~

4o. - deparse_InhRelations

+/*
+ * Deparse the INHERITS relations.
+ *
+ * Given a table OID, return a schema qualified table list representing
+ * the parent tables.
+ */
+static List *
+deparse_InhRelations(Oid objectId)

"schema qualified" -> "schema-qualified"

~

4p. - deparse_OnCommitClause

+/*
+ * Deparse the ON COMMMIT ... clause for CREATE ... TEMPORARY ...
+ */
+static ObjTree *
+deparse_OnCommitClause(OnCommitAction option)

"COMMMIT" -> "COMMIT"

~

4q. - deparse_RuleStmt

+/*
+ * Deparse a RuleStmt (CREATE RULE).
+ *
+ * Given a rule OID and the parsetree that created it, return an ObjTree
+ * representing the rule command.
+ */

"parsetree" -> "parse tree"

~

4r. - deparse_utility_command

+ /*
+ * Allocate everything done by the deparsing routines into a temp context,
+ * to avoid having to sprinkle them with memory handling code; but allocate
+ * the output StringInfo before switching.
+ */

"code; " -> "code, "

~

4s. - deparse_utility_command

+ /*
+ * Many routines underlying this one will invoke ruleutils.c functionality
+ * in order to obtain deparsed versions of expressions.  In such results,
+ * we want all object names to be qualified, so that results are "portable"
+ * to environments with different search_path settings.  Rather than inject
+ * what would be repetitive calls to override search path all over the
+ * place, we do it centrally here.
+ */

"in order to" => "to"

~

4t. - convSpecifier

+/*
+ * Conversion specifier, it determines how we expand the json element into
+ * string.
+ */

SUGGESTION
Conversion specifier which determines how we expand the json element
into a string.

~

4u. - json_trivalue

+/*
+ * A ternary value which represents a boolean type JsonbValue.
+ */

"which represents" -> "that represents"

~

4v. - expand_fmt_recursive

+ /*
+ * Scan the mandatory element name.  Allow for an array separator
+ * (which may be the empty string) to be specified after colon.
+ */

"after colon" -> "after a colon"

~

4w. - 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.
+ *
+ * Returns false if no actual expansion was made due to the "present" flag
+ * being set to "false".
+ */

"latter case" -> "latter case,"

~

4x. - format_procedure_args_internal

+/*
+ * Append the parenthised arguments of the given pg_proc row into the output
+ * buffer.  force_qualify indicates whether to schema-qualify type names
+ * regardless of visibility.
+ */

"parenthised" -> "parenthesized "

~

4y.

+/*
+ * Internal version that returns definition of a CONSTRAINT command
+ */

"definition" -> "the definition"

======

5. Function comment inconsistencies

5a.

Sometimes the function name is repeated in the comment and sometimes it is not.

e.g. compare deparse_CreateEnumStmt() versus deparse_CreateExtensionStmt(), etc.

(IMO there is no need to repeat the function name)

~

5b.

Sometimes the deparse function comments are verbose and say like:

+ * Given a type OID and a parsetree that modified it, return an ObjTree
+ * representing the alter type.

but sometimes - like deparse_AlterExtensionStmt() etc. - they don't
bother to say anything at all.

e.g.
+/*
+ * Deparse ALTER EXTENSION .. UPDATE TO VERSION
+ */
+static ObjTree *
+deparse_AlterExtensionStmt(Oid objectId, Node *parsetree)

Either it is useful, so say it always, or it is not useful, so say it
never. Pick one.

------

6. GENERAL - json

IMO change "json" -> "JSON" everywhere.

Here are some examples:

Line 7605: + * Conversion specifier, it determines how we expand the
json element into
Line 7699: + errmsg("missing element \"%s\" in json object", keyname)));
Line 7857: + * Expand a json value as a quoted identifier.  The value
must be of type string.
Line 7872: + * Expand a json value as a dot-separated-name.  The value
must be of type
Line 7908: + * Expand a json value as a type name.
Line 7966: + * Expand a json value as an operator name
Line 7993: + * Expand a json value as a string.  The value must be of
type string or of
Line 8031: + * Expand a json value as a string literal.
Line 8070: + * Expand a json value as an integer quantity.
Line 8083: + * Expand a json value as a role name.  If the is_public
element is set to
Line 8111: + * Expand one json element into the output StringInfo
according to the
Line 8807: +{ oid => '4642', descr => 'deparse the DDL command into
json format string',
Line 8810: +{ oid => '4643', descr => 'expand json format DDL to a
plain DDL command',

------
Kind Regards,
Peter Smith.
Fujitsu Australia