Re: extension patch of CREATE OR REPLACE TRIGGER

On Mon, Aug 24, 2020 at 9:33 PM osumi(dot)takamichi(at)fujitsu(dot)com
<osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> I've fixed my v06 and created v07.

Hi Osumi-san.

I have reviewed the source code of the v07 patch.

(I also reviewed the test cases but I will share those comments as a
separate post).

Below are my comments - sorry, many of them are very minor.

====

COMMENT pg_constraint.c (wrong comment?)

- * The new constraint's OID is returned.
+ * The new constraint's OID is returned. (This will be the same as
+ * "conOid" if that is specified as nonzero.)

Shouldn't that comment say:
(This will be the same as "existing_constraint_oid" if that is other
than InvalidOid)

====

COMMENT pg_constraint.c (declarations)

@@ -91,6 +93,11 @@ CreateConstraintEntry(const char *constraintName,
NameData cname;
int i;
ObjectAddress conobject;
+ SysScanDesc conscan;
+ ScanKeyData skey[2];
+ HeapTuple tuple;
+ bool replaces[Natts_pg_constraint];
+ Form_pg_constraint constrForm;

Maybe it is more convenient/readable to declare these in the scope
where they are actually used.

====

COMMENT pg_constraint.c (oid checking)

- conOid = GetNewOidWithIndex(conDesc, ConstraintOidIndexId,
- Anum_pg_constraint_oid);
- values[Anum_pg_constraint_oid - 1] = ObjectIdGetDatum(conOid);
+ if (!existing_constraint_oid)
+ {
+ conOid = GetNewOidWithIndex(conDesc, ConstraintOidIndexId,
+ Anum_pg_constraint_oid);

Maybe better to use if (!OidIsValid(existing_constraint_oid)) here.

====

COMMENT tablecmds.c (unrelated change)

- false); /* is_internal */
+ false); /* is_internal */

Some whitespace which has nothing to do with the patch was changed.

====

COMMENT trigger.c (declarations / whitespace)

+ bool is_update = false;
+ HeapTuple newtup;
+ TupleDesc tupDesc;
+ bool replaces[Natts_pg_trigger];
+ Oid existing_constraint_oid = InvalidOid;
+ bool trigger_exists = false;
+ bool trigger_deferrable = false;

1. One of those variables is misaligned with tabbing.

2. Maybe it is more convenient/readable to declare some of these in
the scope where they are actually used.
e.g. newtup, tupDesc, replaces.

====

COMMENT trigger.c (error messages)

+ /*
+ * If this trigger has pending event, throw an error.
+ */
+ if (stmt->replace && !isInternal && trigger_deferrable &&
+ AfterTriggerPendingOnRel(RelationGetRelid(rel)))
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_IN_USE),
+ errmsg("cannot replace \"%s\" on \"%s\" because it has pending
trigger events",
+ stmt->trigname, RelationGetRelationName(rel))));
+ /*
+ * without OR REPLACE clause, can't override the trigger with the same name.
+ */
+ if (!stmt->replace && !isInternal)
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("trigger \"%s\" for relation \"%s\" already exists",
+ stmt->trigname, RelationGetRelationName(rel))));
+ /*
+ * CREATE OR REPLACE CONSTRAINT TRIGGER command can't replace
non-constraint trigger.
+ */
+ if (stmt->replace && stmt->isconstraint &&
!OidIsValid(existing_constraint_oid))
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("Trigger \"%s\" for relation \"%s\" cannot be replaced with
constraint trigger",
+ stmt->trigname, RelationGetRelationName(rel))));
+ /*
+ * CREATE OR REPLACE TRIGGER command can't replace constraint trigger.
+ */
+ if (stmt->replace && !stmt->isconstraint &&
OidIsValid(existing_constraint_oid))
+ ereport(ERROR,
+ (errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("Constraint trigger \"%s\" for relation \"%s\" cannot be
replaced with non-constraint trigger",
+ stmt->trigname, RelationGetRelationName(rel))));

1. The order of these new errors is confusing. Maybe do the "already
exists" check first so that all the REPLACE errors can be grouped
together.

2. There is inconsistent message text capitalising the 1st word.
Should all be lower [1]
[1] - https://www.postgresql.org/docs/current/error-style-guide.html

3. That "already exists" error might benefit from a message hint. e.g.
---
ereport(ERROR, (errcode(ERRCODE_DUPLICATE_OBJECT),
errmsg("trigger \"%s\" for relation \"%s\" already exists", ...),
errhint("use CREATE OR REPLACE to replace it")));
---

4. Those last two errors seem like a complicated way just to say the
trigger types are not compatible. Maybe these messages can be
simplified, and some message hints added. e.g.
---
ereport(ERROR, (errcode(ERRCODE_DUPLICATE_OBJECT),
errmsg("trigger \"%s\" for relation \"%s\" is a regular trigger", ...),
errhint("use CREATE OR REPLACE TRIGGER to replace a regular trigger")));

ereport(ERROR, (errcode(ERRCODE_DUPLICATE_OBJECT),
errmsg("trigger \"%s\" for relation \"%s\" is a constraint trigger", ...),
errhint("use CREATE OR REPLACE CONSTRAINT to replace a constraint
trigger")));
---

====

COMMENT trigger.c (comment wording)

+ * In case of replace trigger, trigger should no-more dependent on old
+ * referenced objects. Always remove the old dependencies and then

Needs re-wording.

====

Kind Regards,
Peter Smith.
Fujitsu Australia