RE: extension patch of CREATE OR REPLACE TRIGGER

Hi, Peter


You gave me two posts for my patch review.
Thank you so much. I'll write all my replies into this post.

> ====
> 
> 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)
Thanks. I corrected this part.

> 
> ====
> 
> 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.
You're right. Fixed.


> ====
> 
> 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.
Got it. I replaced that part by OidIsValid ().


> ====
> 
> COMMENT tablecmds.c (unrelated change)
> 
> -   false); /* is_internal */
> +   false); /* is_internal */
> 
> Some whitespace which has nothing to do with the patch was changed.
Yeah. Fixed.

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

> 
> 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.
I cannot do this because those variables are used
at the top level in this function. Anyway, thanks for the comment.

> ====
> 
> 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.
OK. I sorted out the order and conditions for this part.

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


> 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")));
I simplified those messages.

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


> ====
> 
> COMMENT (confusing functions)
> 
> +create function before_replacement() returns trigger as $$ begin raise
> +notice 'function replaced by another function'; return null; end; $$
> +language plpgsql; create function after_replacement() returns trigger
> +as $$ begin raise notice 'function to replace the initial function';
> +return null; end; $$ language plpgsql;
> 
> Why have function names with a hard-wired dependency on how you expect
> they will be called.
> I think just call them "funcA" and "funcB" is much easier and works just as well.
> e.g.
> ---
> create function funcA() returns trigger as $$ begin raise notice 'hello from
> funcA'; return null; end; $$ language plpgsql;
> 
> create function funcB() returns trigger as $$ begin raise notice 'hello from
> funcB'; return null; end; $$ language plpgsql;
> ---
Got it. I simplified such kind of confusing names. Thanks.


> ====
> 
> COMMENT (drops)
> 
> +-- setup for another test of CREATE OR REPLACE TRIGGER drop table if
> +exists parted_trig;
> +NOTICE:  table "parted_trig" does not exist, skipping drop trigger if
> +exists my_trig on parted_trig;
> +NOTICE:  relation "parted_trig" does not exist, skipping drop function
> +if exists before_replacement;
> +NOTICE:  function before_replacement() does not exist, skipping drop
> +function if exists after_replacement;
> +NOTICE:  function after_replacement() does not exist, skipping
> 
> Was it deliberate to attempt to drop the trigger after dropping the table?
> Also this seems to be dropping functions which were already dropped just
> several lines earlier.
This wasn't necessary. I deleted those.

> ====
> 
> COMMENT (typos)
> 
> There are a couple of typos in the test comments. e.g.
> "vefify" -> "verify"
> "child parition" -> "child partition"
Fixed.

> ====
> 
> COMMENT (partition table inserts)
> 
> 1. Was it deliberate to insert explicitly into each partition table?
> Why not insert everything into the top table and let the partitions take care of
> themselves?
Actually, yes. I wanted readers of this code to easily identify which partitioned is used.
But, I fixed those because it was redundant and not smart. Your suggestion sounds better.

> 
> 2. The choice of values to insert also seemed strange. Inserting 1 and
> 1 and 10 is going to all end up in the "parted_trig_1_1".
> 
> To summarise, I thought all subsequent partition tests maybe should be
> inserting more like this:
> ---
> insert into parted_trig (a) values (50); -- into parted_trig_1_1 insert into
> parted_trig (a) values (1500); -- into parted_trig_2 insert into parted_trig (a)
> values (2500); -- into default_parted_trig
This makes sense. I adopted your idea.

> ====
> 
> COMMENT (missing error test cases)
> 
> There should be some more test cases to cover the new error messages that
> were added to trigger.c:
> 
> e.g. test for "can't create regular trigger because already exists"
> e.g. test for "can't create constraint trigger because already exists"
> e.g. test for "can't replace regular trigger with constraint trigger""
> e.g. test for "can't replace constraint trigger with regular trigger"
> etc.
I've added those tests additionally.

> ====
> 
> COMMENT (trigger with pending events)
> 
> This is another test where the complexity of the functions ("not_replaced", and
> "fail_to_replace") seemed excessive.
> I think just calling these "funcA" and "funcB" as mentioned above would be
> easier, and would serve just as well.
I modified the names of functions.


Best,
    Takamichi Osumi