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

Here are some review comments for patch v63-0001.

======
General

1.
(This is not really a review comment - more just an observation...)

This patch seemed mostly like an assortment of random changes that
don't seem to have anything in common except that some *later* patches
of this set are apparently going to want them.

Now maybe doing it this way was the best and neatest thing to do --
I'm not sure. But my first impression was I felt this has gone too far
in some places -- e.g. perhaps some of these changes would have been
better deferred until they are *really* needed instead of just
plonking a whole lot of un-called (i.e. untested) code into patch
0001.


======
Commit message

2.
2) Some of the prototype and structures were moved from pg_publication.h
   to publicationcmds.h as one of the later patch requires inclusion of
   pg_publication.h and these prototype had references to server header
   files.

SUGGESTION (?)
2) Some prototypes and structures were moved from pg_publication.h to
publicationcmds.h. This was because one of the later patches required
the inclusion of pg_publication.h and these prototypes had references
to server header files.


======
src/backend/catalog/aclchk.c

3. ExecuteGrantStmt

+ /* Copy the grantor id needed for DDL deparsing of Grant */
+ istmt.grantor_uid = grantor;
+

SUGGESTION (comment)
Copy the grantor id to the parsetree, needed for DDL deparsing of Grant

======
src/backend/catalog/objectaddress.c

4. getObjectIdentityParts

@@ -5922,7 +5922,7 @@ getObjectIdentityParts(const ObjectAddress *object,
  transformType = format_type_be_qualified(transform->trftype);
  transformLang = get_language_name(transform->trflang, false);

- appendStringInfo(&buffer, "for %s on language %s",
+ appendStringInfo(&buffer, "for %s language %s",
  transformType,
  transformLang);

There is no clue anywhere what this change was for.

Perhaps this ought to be mentioned in the Commit Message.

Alternatively, maybe defer this change until it becomes clearer who needs it?

======
src/backend/commands/collationcmds.c

5.
+ /*
+ * Make from existing collationid available to callers for statement such as
+ * CREATE COLLATION any_name FROM any_name
+ */
+ if (from_existing_collid && OidIsValid(collid))
+ ObjectAddressSet(*from_existing_collid, CollationRelationId, collid);

"for statement such as" --> "for statements such as"

======
src/backend/commands/event_trigger.c

6.
+EventTriggerQueryState *currentEventTriggerState = NULL;

It seems overkill to make this non-static here. I didn't find anybody
using this variable from outside this source, so unless this was a
mistake I guess it's preparing the ground for some future patch.
Either way, it didn't seem like this belonged in patch 0001.

======
src/backend/commands/sequence.c

7.
+Form_pg_sequence_data
+get_sequence_values(Oid sequenceId)
+{
+ Buffer      buf;
+ SeqTable    elm;
+ Relation    seqrel;
+ HeapTupleData seqtuple;
+ Form_pg_sequence_data seq;
+ Form_pg_sequence_data retSeq;
+
+ /* 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);
+ retSeq = palloc(sizeof(FormData_pg_sequence_data));
+
+ memcpy(retSeq, seq, sizeof(FormData_pg_sequence_data));
+
+ UnlockReleaseBuffer(buf);
+ relation_close(seqrel, NoLock);
+
+ return retSeq;
+}

IMO the palloc might be better done up-front when the retSeq was declared.

======
src/backend/tcop/utility.c

8.
+/*
+ * Return the given object type as a string.
+ */
+const char *
+stringify_objtype(ObjectType objtype, bool isgrant)
+{
+ switch (objtype)
+ {
+ case OBJECT_AGGREGATE:
+ return "AGGREGATE";
+ case OBJECT_CAST:
+ return "CAST";
+ case OBJECT_COLLATION:
+ return "COLLATION";
+ case OBJECT_COLUMN:
+ return isgrant ? "TABLE" : "COLUMN";
+ case OBJECT_CONVERSION:
+ return "CONVERSION";
+ case OBJECT_DATABASE:
+ return "DATABASE";
+ case OBJECT_DOMAIN:
+ return "DOMAIN";
+ case OBJECT_EVENT_TRIGGER:
+ return "EVENT TRIGGER";
+ case OBJECT_EXTENSION:
+ return "EXTENSION";
+ case OBJECT_FDW:
+ return "FOREIGN DATA WRAPPER";
+ case OBJECT_FOREIGN_SERVER:
+ return isgrant ? "FOREIGN SERVER" : "SERVER";
+ case OBJECT_FOREIGN_TABLE:
+ return "FOREIGN TABLE";

That 'is_grant' param seemed a bit hacky.

At least some comment should be given (maybe in the function header?)
to explain why this boolean is modifying the return string.

Or maybe it is better to have another stringify_objtype_for_grant that
just wraps this?

======
src/backend/utils/adt/regproc.c

9.
+
+/*
+ * Append the parenthesized arguments of the given pg_proc row into the output
+ * buffer. force_qualify indicates whether to schema-qualify type names
+ * regardless of visibility.
+ */
+static void
+format_procedure_args_internal(Form_pg_proc procform, StringInfo buf,
+    bool force_qualify)
+{
+ int i;
+ char* (*func[2])(Oid) = {format_type_be, format_type_be_qualified};
+
+ appendStringInfoChar(buf, '(');
+ for (i = 0; i < procform->pronargs; i++)
+ {
+ Oid thisargtype = procform->proargtypes.values[i];
+ char    *argtype = NULL;
+
+ if (i > 0)
+ appendStringInfoChar(buf, ',');
+
+ argtype = func[force_qualify](thisargtype);
+ appendStringInfoString(buf, argtype);
+ pfree(argtype);
+ }
+ appendStringInfoChar(buf, ')');
+}

9a.
Assign argtype = NULL looks redundant because it will always be
overwritten anyhow.

~

9b.
I understand why this function was put here beside the other static
functions in "Support Routines" but IMO it really belongs nearby (i.e.
directly above) the only caller (format_procedure_args). Keeping both
those functional together will improve the readability of both, and
will also remove the need to have the static forward declaration.

======
src/backend/utils/adt/ruleutils.c

10.
+void
+pg_get_ruledef_detailed(Datum ev_qual, Datum ev_action,
+ char **whereClause, List **actions)
+{
+ int prettyFlags = 0;
+ char    *qualstr = TextDatumGetCString(ev_qual);
+ char    *actionstr = TextDatumGetCString(ev_action);
+ List    *actionNodeList = (List *) stringToNode(actionstr);
+ StringInfoData buf;
+
+ *whereClause = NULL;
+ *actions = NIL;
+ initStringInfo(&buf);
+ if (strlen(qualstr) > 0 && strcmp(qualstr, "<>") != 0)
+ {

If you like, that condition could have been written more simply as:

if (*qualstr && strcmp(qualstr, "<>") != 0)

~~~

11.
+/*
+ * Parse back the TriggerWhen clause of a trigger given the
pg_trigger record and
+ * the expression tree (in nodeToString() representation) from
pg_trigger.tgqual
+ * for the trigger's WHEN condition.
+ */
+char *
+pg_get_trigger_whenclause(Form_pg_trigger trigrec, Node *whenClause,
bool pretty)
+{

It seemed "Parse back" is a typo.

I assume it was meant to say something like "Passes back", or maybe
just "Returns" is better.

======
src/include/replication/logicalrelation.h

12.
@@ -14,6 +14,7 @@

 #include "access/attmap.h"
 #include "replication/logicalproto.h"
+#include "storage/lockdefs.h"

What is this needed here for? I tried without this change and
everything still builds OK.


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