Re: Support logical replication of DDLs

Here are some review comments for the patch v11-0001:

======

1. Commit message

This provides base for logical replication of DDL statements. Currently,
this provides support for:

SUGGESTION
This provides a base for logical replication of DDL statements. Currently,
the patch has support for:

======

2. src/backend/commands/ddl_deparse.c - <general>

I noticed that some of these function header comments have periods and
so do not. Please add a period to every one of them for consistency.

~~~

3. src/backend/commands/ddl_deparse.c - <general>

There are quite a few functions in this file with no function comment.
Probably every function should have a comment.

List includes at least all these:

- deparse_ColumnIdentity
- RelationGetPartitionBound
- deparse_AlterOwnerStmt
- deparse_RenameStmt
- deparse_Seq_Cache
- deparse_Seq_Cycle
- deparse_Seq_IncrementBy
- deparse_Seq_Minvalue
- deparse_Seq_Maxvalue
- deparse_Seq_Startwith
- deparse_Seq_Restart
- deparse_Seq_OwnedBy
- deparse_AlterTableStmt
- deparse_CreateTableAsStmt
- deparse_drop_sequence
- deparse_drop_schema
- deparse_drop_index
- deparse_drop_table

~~~

4. src/backend/commands/ddl_deparse.c - <general>

Lots of places are making calls to the new_objtree_VA function but
some of them are a bit messy. I think the wrapping of the args to that
function needs to be revisited and made consistent indentation
everywhere to make them all easier to read. IMO it is easier when the
number of arg-groups is clear and each arg-group is on a new line.
Like this example:

column = new_objtree_VA("%{name}I WITH OPTIONS %{default}s %{not_null}s",
                                           2,
                                          "type", ObjTypeString, "column",
                                          "name", ObjTypeString,
coldef->colname);

~~~

5. src/backend/commands/ddl_deparse.c - <general>

Lots of the function comments are giving the function name again. It
does not seem necessary.

e.g (there many more are like this)

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

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

~~~

6. src/backend/commands/ddl_deparse.c - typedefs

6a.
+} ObjType;

Shouldn't this be added to typedefs.list?

~

6b.
+typedef struct ObjTree

Ditto.

~

6c.
+typedef struct ObjElem

Ditto

~~~

7. src/backend/commands/ddl_deparse.c - format_type_detailed

+ }
+ *nspid = InvalidOid;
+
+ if (typemod >= 0)
+ *typemodstr = printTypmod("", typemod, typeform->typmodout);
+ else
+ *typemodstr = pstrdup("");
+
+ ReleaseSysCache(tuple);
+ return;
+ }
+
+ /*
+ * No additional processing is required for other types, so get the type
+ * name and schema directly from the catalog.
+ */
+ *nspid = typeform->typnamespace;
+ *typname = pstrdup(NameStr(typeform->typname));
+
+ if (typemod >= 0)
+ *typemodstr = printTypmod("", typemod, typeform->typmodout);
+ else
+ *typemodstr = pstrdup("");
+
+ ReleaseSysCache(tuple);
+}

The code can be simplified a bit by using if/else because the part:

+ if (typemod >= 0)
+ *typemodstr = printTypmod("", typemod, typeform->typmodout);
+ else
+ *typemodstr = pstrdup("");
+
+ ReleaseSysCache(tuple);

is common code.

~~~

8. src/backend/commands/ddl_deparse.c - append_bool_object

Why this function has no assert like the others do?

+ Assert(name);

~~~

9. src/backend/commands/ddl_deparse.c - append_array_object

Why this function has no assert like the others do?

+ Assert(name);

~~~

10. src/backend/commands/ddl_deparse.c - new_objtree_for_type

+ if (!OidIsValid(typnspid))
+ typnsp = pstrdup("");
+ else
+ typnsp = get_namespace_name_or_temp(typnspid);

Reversing this if/else will give slight simpler code

~~~

11. src/backend/commands/ddl_deparse.c - deparse_ColumnIdentity

+ ObjTree    *tmp;

"tmp" doesn’t seem a very good variable name since that is also what
the function is returning.

~~~

12.

+ /* definition elemets */

Uppercase comment.

~~~

13.

+ /* we purposefully do not emit OWNED BY here */

Uppercase comment.

~~~

14. src/backend/commands/ddl_deparse.c - deparse_ColumnDef

+ /*
+ * Inherited columns without local definitions must not be emitted. XXX --
+ * maybe it is useful to have them with "present = false" or some such?
+ */

I think the XXX should be on a newline otherwise the note just gets
lost in the comment.

~~~

15.

+ if (saw_notnull)
+ append_string_object(column, "not_null", "NOT NULL");
+ else
+ append_string_object(column, "not_null", "");

Perhaps simple code like this is more neatly written as:

append_string_object(column, "not_null", saw_notnull ? "NOT NULL" : "");

~~~

16. src/backend/commands/ddl_deparse.c - deparse_ColumnDef_typed

+ if (saw_notnull)
+ append_string_object(column, "not_null", "NOT NULL");
+ else
+ append_string_object(column, "not_null", "");

Ditto previous comment #15.

~~~

17. src/backend/commands/ddl_deparse.c - deparseTableElements

Should the function name be "deparse_TableElements" to match the
pattern of all the others?

~~~

18. src/backend/commands/ddl_deparse.c - deparse_OnCommitClause

+ ObjTree    *tmp;

I don’t think "tmp" is a good name here because the function is returning it.

~~~

19. src/backend/commands/ddl_deparse.c - deparse_DefElem

+ set = new_objtree_VA("%{label}s = %{value}L", 1,
+ "value", ObjTypeString,
+ elem->arg ? defGetString(elem) :
+ defGetBoolean(elem) ? "TRUE" : "FALSE");

The double ternary operators here are a bit hard to read. Maybe add
some extra parens just to improve the readability?

~~~

20. src/backend/commands/ddl_deparse.c - deparse_ColumnSetOptions

+ ObjTree *tmp;

I don’t think "tmp" is a very good name here because the function is
returning this.

~~~

21. src/backend/commands/ddl_deparse.c - deparse_RelSetOptions

+ ObjTree *tmp;

I don’t think "tmp" is a very good name here because the function is
returning this.

~~~

22 src/backend/commands/ddl_deparse.c - pg_get_indexdef_detailed

+
+
+ /*
+ * Fetch the pg_am tuple of the index' access method
+ */

Spurious whitespace line.

~~~

23.

+ /* output index AM */

Uppercase comment

~~~

24.

+ sep = "";
+ for (keyno = 0; keyno < idxrec->indnatts; keyno++)
+ {
+ AttrNumber attnum = idxrec->indkey.values[keyno];
+ int16 opt = indoption->values[keyno];
+ Oid keycoltype;
+ Oid keycolcollation;
+ Oid indcoll;
+
+ appendStringInfoString(&definitionBuf, sep);
+ sep = ", ";

This assignment of "sep" seemed a bit strange. Can't it be more
easilly written like:

appendStringInfoString(&definitionBuf, keyno == 0 ? "" : ", ");

~~~

25.

+ /* expressional index */

Uppercase comment.

~~~

26.

+ /* if it supports sort ordering, report DESC and NULLS opts */

Uppercase comment

~~~

27.

+ /* output tablespace */

Uppercase comment

~~~

28.

+ /* report index predicate, if any */

Uppercase comment

~~~

29.

+
+ /* all done */

Kind of redundant/meaningless. Consider removing this comment.

~~~

30. src/backend/commands/ddl_deparse.c - deparse_RenameStmt

+ switch (node->renameType)
+ {
+ case OBJECT_SCHEMA:
+ {
+ renameStmt =
+ new_objtree_VA("ALTER SCHEMA %{identity}I RENAME TO %{newname}I",
+    0);
+ append_string_object(renameStmt, "identity", node->subname);
+ }
+ break;
+ default:
+ elog(ERROR, "unsupported object type %d", node->renameType);
+ }

The switch with single case seems a bit overkill here. Wouldn’t just
"if" be more appropriate?

~~~

31. src/backend/commands/ddl_deparse.c - deparse_CreateSeqStmt

This function looked very similar to the other function
deparse_ColumnIdentity. Is it worth trying to combine these or have
one of them just delegate to the other to reduce the cut/paste code?

~~~

32. src/backend/commands/ddl_deparse.c - deparse_AlterTableStmt

+ default:
+ elog(ERROR, "unexpected relkind %d", rel->rd_rel->relkind);
+ reltype = NULL;;

Does the assignment after the elog achieve anything?

~~~

33.

+ /* if no constraint was altered, silently skip it */

Uppercase comment

~~~

34.

+ /* add the TYPE clause */

Uppercase comment

~~~

35.

+ /* add a COLLATE clause, if needed */

Uppercase comment

~~~

36.

+ /* if not a composite type, add the USING clause */

Uppercase comment

~~~

37.

+ /* if it's a composite type, add the CASCADE clause */

Uppercase comment

~~~

38. src/backend/commands/ddl_deparse.c - deparse_drop_sequence

38a.

+ command = JsonbToCString(&str, &jsonb->root, 128);

Is there some more appropriate constant to use here instead of the
hardwired 128?

~~

38b. deparse_drop_schema
ditto

~~

38c. deparse_drop_index
ditto

~~

38d. deparse_drop_table
ditto


~~~

39. src/backend/commands/ddl_deparse.c - ddl_deparse_to_json

+ if (command)
+ PG_RETURN_TEXT_P(CStringGetTextDatum(command));
+ else
+ PG_RETURN_NULL();

The 'else’ keyword is not needed. Just do PG_RETURN_NULL();

======

40. src/backend/commands/ddl_json.c - <general>

Many (but not all) of these comments (particularly the function header
comments) seem to have double blank spaces in them after periods. I
don’t think it is normal. Please remove the extra spaces

~~~

41. src/backend/commands/ddl_json.c - <general>

All the function header comment sentences should start with uppercase. E.g. in
many places:

"expand" -> "Expand"

~~~

42. src/backend/commands/ddl_json.c -

42.a
+typedef enum
+{
+ SpecTypename,
+ SpecOperatorname,
+ SpecDottedName,
+ SpecString,
+ SpecNumber,
+ SpecStringLiteral,
+ SpecIdentifier,
+ SpecRole
+} convSpecifier;

Add this to typedefs.list?

~~

42.b
+typedef enum
+{
+ tv_absent,
+ tv_true,
+ tv_false
+} trivalue;

Add this to typedefs.list?

~~~

43. src/backend/commands/ddl_json.c - find_string_in_jsonbcontainer

+/*
+ * Given a JsonbContainer, find the JsonbValue with the given key name in it.
+ * If it's of a type other than jbvString, an error is raised.  If it doesn't
+ * exist, an error is raised if missing_ok; otherwise return NULL.
+ *
+ * If it exists and is a string, a freshly palloc'ed copy is returned.
+ *
+ * If *length is not NULL, it is set to the length of the string.
+ */
+static char *
+find_string_in_jsonbcontainer(JsonbContainer *container, char *keyname,
+   bool missing_ok, int *length)


"an error is raised if missing_ok" --> I think this should say an
error is raised *unless* missing_ok

~~~

44.

+ if (value == NULL)
+ {
+ if (missing_ok)
+ return NULL;
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("missing element \"%s\" in json object", keyname)));
+ }

For some reason, it seems more intuitive IMO to handle the error case
first here. YMMV.

if (value == NULL)
{
    if (!missing_ok)
        ereport(...)

    return NULL;
}

~~~

45. src/backend/commands/ddl_json.c - expand_fmt_recursive

+ /*
+ * found array separator delimiter; element name is now
+ * complete, start filling the separator.
+ */

Uppercase comment

~~~

46.

+ /* Validate that we got an array if the format string specified one. */
+
+ /* And finally print out the data */
+ if (is_array)

Something seems strange for these comments to be separated like this.

~~~

47 src/backend/commands/ddl_json.c - expand_jsonval_identifier

+ str = pnstrdup(jsonval->val.string.val,
+    jsonval->val.string.len);

Unnecessary wrapping

~~~

48. src/backend/commands/ddl_json.c - expand_jsonval_dottedname

+/*
+ * Expand a json value as a dot-separated-name.  The value must be of type
+ * object and must contain elements "schemaname" (optional), "objname"
+ * (mandatory), "attrname" (optional).  Double quotes are added to each element
+ * as necessary, and dot separators where needed.
+ *
+ * One day we might need a "catalog" element as well, but no current use case
+ * needs that.
+ */

Does it make sense to say "must" contain elements that are optional elements?

~~~

49. src/backend/commands/ddl_json.c - expand_jsonval_strlit

+ /* easy case: if there are no ' and no \, just use a single quote */

Uppercase comment

~~~

50.

+ dqnextchar %= sizeof(dqsuffixes) - 1;

This statement looks quite confusing. May add some parens for better reability?

~~~

51.

+ /* add trailing $ */

Uppercase comment.

~~~

52. src/backend/commands/ddl_json.c - expand_one_jsonb_element

+static bool
+expand_one_jsonb_element(StringInfo out, char *param, JsonbValue *jsonval,
+ convSpecifier specifier, const char *fmt)

In the other function the StringInfo buffer was called "buf" instead
of "out". Either one is OK but I think you should use consistent
naming for all the functions like this. Check all the function – not
just this one.


~~~

53. src/backend/commands/ddl_json.c - expand_jsonb_array

+ if ((container->header & JB_FARRAY) == 0)

I think there is a macro you should use designed exactly for this –
see JsonContainerIsArray(jc)

~~~

54.

+ switch (type)
+ {
+ case WJB_ELEM:
+ if (!first)
+ appendStringInfoString(out, arraysep);
+
+ if (expand_one_jsonb_element(out, param, &v, specifier, NULL))
+ first = false;
+ else
+ {
+ if (!first)
+ {
+ /* remove the array separator */
+ out->len -= arrayseplen;
+ out->data[out->len] = '\0';
+ }
+ }
+ break;
+ }

Why have a switch with just a single case?

~~~

55.

+ if (!first)
+ appendStringInfoString(out, arraysep);
+
+ if (expand_one_jsonb_element(out, param, &v, specifier, NULL))
+ first = false;
+ else
+ {
+ if (!first)
+ {
+ /* remove the array separator */
+ out->len -= arrayseplen;
+ out->data[out->len] = '\0';
+ }
+ }

It looks a bit strange to first write the separator to the buffer then
find that you shouldn't have written it so have to undo it. Is this
the best way that this can be handled? Maybe there is no choice.

~~~

56.

+ /* remove the array separator */

Uppercase comment

~~~~

57. src/backend/commands/ddl_json.c - ddl_deparse_json_to_string

+char *
+ddl_deparse_json_to_string(char *json_str)

Missing function comment


~~~

58. src/backend/commands/ddl_json.c - ddl_deparse_expand_command

+Datum
+ddl_deparse_expand_command(PG_FUNCTION_ARGS)

This function name seems a bit strange - see also comment #62

======

59. src/backend/commands/sequence.c - get_sequence_values

+ /* open and AccessShareLock sequence */

Uppercase comment

~~~

60.

+ retSeq = palloc(sizeof(FormData_pg_sequence_data));
+
+ /* open and AccessShareLock sequence */
+ init_sequence(sequenceId, &elm, &seqrel);
+
+ if (pg_class_aclcheck(sequenceId, GetUserId(),
+ ACL_SELECT | ACL_UPDATE | ACL_USAGE) != ACLCHECK_OK)
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied for sequence %s",
+ RelationGetRelationName(seqrel))));
+
+ seq = read_seq_tuple(seqrel, &buf, &seqtuple);
+
+ memcpy(retSeq, seq, sizeof(FormData_pg_sequence_data));

I thought maybe better if the palloc could be done later, paired with
the memcpy, since it is not needed before then.

======

61. src/backend/utils/adt/ruleutils.c - pg_get_partkeydef_simple

+char *
+pg_get_partkeydef_simple(Oid relid)

Missing function comment

======

62. src/include/catalog/pg_proc.dat

+{ oid => '4642', descr => 'ddl deparse',
+  proname => 'ddl_deparse_to_json', prorettype => 'text',
+  proargtypes => 'pg_ddl_command', prosrc => 'ddl_deparse_to_json' },
+{ oid => '4643', descr => 'json to string',
+  proname => 'ddl_deparse_expand_command', prorettype => 'text',
+  proargtypes => 'text', prosrc => 'ddl_deparse_expand_command' },

My 1st impressions was the name "ddl_deparse_expand_command" does not
see to reflext the description very well...

Maybe calling it something like "ddl_json_to_command" is more accurate?

======

63. src/include/utils/builtins.h

@@ -118,6 +118,7 @@ extern char *format_type_extended(Oid type_oid,
int32 typemod, bits16 flags);
 extern char *format_type_be(Oid type_oid);
 extern char *format_type_be_qualified(Oid type_oid);
 extern char *format_type_with_typemod(Oid type_oid, int32 typemod);
+extern char *printTypmod(const char *typname, int32 typmod, Oid typmodout);

Notice that every of the format_type function name looks like
format_type_XXX. Not that you have change the printTypmod to be extern
then I woinder should the name also be changed (e.g.
format_type_printmod) to have the consistent function naming.

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