Re: Support logical replication of DDLs

On Fri, Feb 3, 2023 at 11:41 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 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.
>
>

Alvaro has replied to this.

> ======
> 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.
>

Changed.

>
> ======
> 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
>

didn't change this, as Alvaro said this was not a parsetree.

> ======
> 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.
>

added this in the commit message.

>
> ======
> 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"
>

changed.

> ======
> 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.
>

The idea is to use this as a preparatory patch.

> ======
> 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.
>

changed.

> ======
> 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.
>

added comment in the function header.

> ======
> 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.
>

changed this.

> ~
>
> 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)
>

fixed.

> ~~~
>
> 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.

fixed.

>
> ======
> 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.
>

fixed.

regards,
Ajin Cherian
Fujitsu Australia