[Postgres-xc-developers] Review for commit badf4c31edfd3d72a38a348523f5e05730374700

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

Hi Amit,
Here are comments for commit
commit badf4c31edfd3d72a38a348523f5e05730374700
Author: Amit Khandekar <ami...@en...>
Date:   Thu Apr 4 12:17:09 2013 +0545

    The core BEFORE ROW trigger-support related changes.

    Because there is no tupleId parameter in the trigger functions is not
valid,
    we need to accept the OLD row through  oldtuple parameter of the
trigger functions.
    And then we craft NEW row using modified values slot, plus the OLD
values in OLD tuple.

Comments
---------------
In ExecBRDeleteTriggers() and all the functions executing before row
triggers, I see this code
2210 #ifdef PGXC
2211     if ((IS_PGXC_COORDINATOR && !IsConnFromCoord()))
2212         trigtuple = pgxc_get_trigger_tuple(datanode_tuphead);
2213     else /* On datanode, do the usual way */
2214     {

This code will be applicable to all the deletes/modifications at the
coordinator. The updates to catalogs happen locally to the coordinator and
thus should call GetTupleForTrigger instead of pgxc_get_trigger_tuple(). I
am not sure if there can be triggers on catalog tables, (mostly not), but
it will be better to check whether the tuple is local or global.

Right now we are passing both the old tuple and tupleid both to
ExecBRDeleteTriggers() function. Depending upon the tuplestore
implementation we do for storing the old tuples, we may be able to retrieve
the old tuple from the tuplestore given some index into it. Can we use the
ItemPointer to store this index?

I am coming back to functions pgxc_form_trigger_tuple() and
pgxc_get_trigger_tuple(). In these functions there is nothing XC specific,
so my first thought was whether there are functions already in PG code,
which would serve the functionality that these two functions serve. I tried
to search but without success. Can you find any PG function which has this
functionality? Should these functions be in heaptuple.c or such file
instead of trigger.c? Also, there is nothing specific for triggers in these
functions, so better their names not contain trigger.

Regarding trigger firing, I am assuming that, all the BR triggers are fired
on a single node either the coordinator (if there at least one nonshippable
trigger) or datanode (if all the triggers are shippable). We can not fire
some at coordinator and some at the datanode because that might change the
order of firing the triggers. PG has documented that order of firing
triggers is alphabetical. With this context,
in function pgxc_is_trigger_firable(), what happens if a triggers is
shippable but needs to be fired on coordinator. From the code it looks like
we won't fire the trigger. At line 5037, you have used IsConnFromCoord()
which is true even for a coordinator - coordinator connection. Do you want
to specifically check datanode here? It will be helpful if the function
prologue contains a truth table with shippability and node where the firing
will happen and the connection origination. This is not your change but
somebody who implemented triggers last time has left a serious bug here.

Everywhere I see that we have used the checks for coordinator to execute
the things XC way or PG way. I think we need better have PG way for local
modifications and XC way for remote modifications.

The declaration for function fill_slot_with_oldvals(), should have the
return type, function name everything on the same line, as per PG standards.

The function fill_slots_with_oldvals() is being called before the actual
update or in fact any of the trigger processing. What's the purpose of this
function? The comment there tell what the function does, but not WHY.

I think I will need to revisit this commit again at the time of reviewing
after row trigger implementation, since this commit has modified some of
that area as well.
-- 
Best Wishes,
Ashutosh Bapat
EntepriseDB Corporation
The Enterprise Postgres Company