Re: [Postgres-xc-developers] Patch for trigger shippability

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

Hi Amit,
Here are comment on trig_shippability patch.

1. The function pgxc_trigevent_quickfind() needs a better name like
pgxc_has_trigger_for_event() to convey the functionality clearly.
 2. The prologue of function pgxc_should_exec_triggers() has all the
necessary content, but it needs to be written in a better order. The
prologue talks about a single trigger (to be executed) but in reality the
function is executed for checking whether all the triggers matching the
given criteria and belonging to a given relation are firable or not. The
prologue needs to be corrected. Also, first specify why is such a check
needed (trigger order being alphabetical makes it necessary that all or
none of the triggers execute on the same node).
3. pgxc_is_inttrigger_firable(), the function name needs to change a bit.
The int in inttrigger can be easily associated with integer instead of
internal. Can you please change the name to use internal_ or intern_
instead of int. If the name becomes too long, please use suitable prefix.
4. RemoteQueryNext() is called multiple times; the after statement triggers
too will be called those many times. Right now this does not happen since
there is no way the function will be called multiple times for a DML. But
in future, we will start supporting RETURNING with FQS, in which case
RemoteQueryNext will be called multiple times.

test xc_trigship

1. we are using same name xc_auditlog for table and function. Please use
different names.
2. For every object (table/function etc) that the test creates, please
mention the purpose of that object. That helps validating the object
definition there and there itself.
 3. I couldn't understand, how does the test check where the trigger is
being fired?



On Mon, May 13, 2013 at 2:36 PM, Ashutosh Bapat <
ash...@en...> wrote:

> Hi Amit,
> We now have Query structure in RemoteQuery. This corresponds to the query
> being fired on the datanodes. THe commandType here should tell you whether
> it's DELETE or not.
>
>
> On Mon, May 13, 2013 at 1:14 PM, Amit Khandekar <
> ami...@en...> wrote:
>
>> In case of multiple triggers of the same type for a given table, the
>> requirement is that they should be fired in alphabetical order. To do so,
>> we need to fire either all of them on coordinator, or all of them on
>> datanode. The main changes in the attached patch are related to this
>> requirement.
>>
>> All of the Exec*Trigger() functions now execute should_exec_trigger*()
>> functions that return true if the node on which they are run is the right
>> node to execute those type of triggers.
>>
>> Also for BR triggers, the additional requirement is that they should be
>> run on coordinator if AR triggers are not shippable, regardless of whether
>> BR themselves are shippable or not. This is because, if we fire BR triggers
>> on datanode, they might change the final row updated, and so we need to
>> again fetch the new row back to the coordinator. Instead, if we fire them
>> on coordinator, we already know what's the final row. Also, we would have
>> required additional changes to add RETURNING to the remote query to fetch
>> the final updated row.
>>
>> Constraint triggers are exception; we need to fire them always on
>> datanode. Once we support global constraints, these need not be specially
>> handled.
>>
>> I was trying to avoid the should_exec_trigger*() calls for each of the
>> Exec*Trigger() functions by doing this in TriggerEnabled() because it is a
>> common function called for all triggers. But the issue is, for AFTER
>> triggers, it is called with a different set of  event type values than the
>> usual  TRIGGER_TYPE* values. For AFTER triggers, TRIGGER_EVENT_* values are
>> used. Anyways, TriggerEnabled() is called for each of the trigger.
>>
>> The trigger shippability helper functions are now completely changed.
>> pgxc_find_nonshippable_row_trig() is the key function. The comments in
>> these functions should give a fair idea of their functionality.
>>
>>
>> For stmt shippability, the trigger functions are now explicitly called if
>> it's not an internally generated query. rq_internal_params field is used to
>> know that this DML is user-supplied query (that is, it would be FQS). The
>> functions are called in RemoteQueryNext().
>>
>>
>> THere is another patch (relaccess_type.patch) that you need to first
>> apply before the main patch (trig_shippability.patch) is applied. I had to
>> add another RELATION_ACCESS_DELETE in ExecNodes. To handle the stmt
>> triggers, I had to know whether the statement is DELETE or INSERT or UPDATE.
>>
>> The new test xc_trigship is added.
>>
>>
>> ------------------------------------------------------------------------------
>> Learn Graph Databases - Download FREE O'Reilly Book
>> "Graph Databases" is the definitive new guide to graph databases and
>> their applications. This 200-page book is written by three acclaimed
>> leaders in the field. The early access version is available now.
>> Download your free book today! http://p.sf.net/sfu/neotech_d2d_may
>> _______________________________________________
>> Postgres-xc-developers mailing list
>> Pos...@li...
>> https://lists.sourceforge.net/lists/listinfo/postgres-xc-developers
>>
>>
>
>
> --
> Best Wishes,
> Ashutosh Bapat
> EntepriseDB Corporation
> The Postgres Database Company
>



-- 
Best Wishes,
Ashutosh Bapat
EntepriseDB Corporation
The Postgres Database Company