[Postgres-xc-developers] BEFORE and AFTER row triggers review

[Ashutosh B. <ash...@en...>]

Hi Amit
Here are comments on commit

commit 1dc081ebe097e63009bab219231e55679db6fae0
Author: Amit Khandekar <ami...@en...>
Date:   Thu Apr 4 11:39:46 2013 +0545

A few helper functions needed for subsequent trigger-support related
commits.

In prologue of pgxc_check_triggers_shippability_ex() can you please use
word "caller" instead of "user". "user" is confusing.

Generally we keep all shippability related code in pgxcship.c. But I see
trigger shippability code in trigger.c. Should we move it to pgxcship.c

We may want to give a different name to function
"pgxc_check_triggers_shippability_ex", since it's doing more than looking
for shippability. The function is looking at all triggers for given command
type to check if each of them is shippable. It also returns has_quals and
trigger descriptor if requested. May be we want to call it as
pgxc_relation_has_shippable_triggers() or something like that.

BTW, mixing three functionalities 1. checking shippability of triggers on
relation 2. returning a trigger descriptor for a relation 3. checking
whether a trigger has quals, may not be a good idea (esp. putting 2
along-with 1 and 3). I can understand that it might improve performance a
bit, because we save on lookup, but it will hamper readability, since we
will be calling this function to get trigger descriptor even if we don't
want shippability information. Can we please separate these functionalities
into separate functions. A good example of misuse is function
"pgxc_triggers_getdesc() which is a misnomer. This function gets trigger
descriptor only if the triggers are unshippable and also returns this
shippability status.

Please add prologue to functions  pgxc_form_trigger_tuple() and
pgxc_get_trigger_tuple() to explain, what these functions do and where they
can be used.
-- 
Best Wishes,
Ashutosh Bapat
EntepriseDB Corporation
The Enterprise Postgres Company