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

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

On Fri, May 17, 2013 at 4:15 PM, Amit Khandekar <
ami...@en...> wrote:

>
>
> On 15 May 2013 12:16, Ashutosh Bapat <ash...@en...>wrote:
>
>> 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.
>>
>
> Done all the above.
>
>
>> 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.
>>
>
> True. Right now it works but once RemoteQueryNExt would be called multiple
> times for FQS, this won't work. I have added a check of TupIsNull() before
> firing AS trigger. BS trigger was ok.
>
>
>
This looks fine.


> Also, I have removed the relation_access_type field check for statement
> triggers. Instead, for FQS updated the remote_query field with the Query
> structure, and used it's command tag.
>
>
Good.


>
> There was one more redundancy discovered. When we have non-shippable AR
> triggers but BR triggers don't even exist, we are currently using all
> columns in the SET clause in the remote update statement. This is not
> necessary. The function should_exec_br_triggers() is used to determine
> whether we should do this. But this function returns true even if there are
> no BR triggers and one or more non-shippable AR triggers exist. In the
> updated patch, I have added a check to see if the BR triggers exist in the
> first place, and if not, return false.
>
>
Ok, thanks for noticing this one.


>
> Before checking in the patch, I want to check one more thing. The with.sql
> shows a different output for statements like:
>   WITH wcte AS ( INSERT INTO child1 VALUES ( 42, 'new' ) RETURNING id AS
> newid )
>
> Although the current results also fail for the same statements, my patch
> results in further diffs for the same statements, so I am not sure what's
> going on. When Abbas checks in the with.sql , I will re-run the regression.
> Abbas, do you have any rough patch ready in order for me to test that my
> patch does not create any new with.sql failures ? Just one that passes
> with.sql should suffice.
>
>
Once you check this, it's fine to commit the patch.


>
>> 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.
>>
>
> Done.
>
>
>>  3. I couldn't understand, how does the test check where the trigger is
>> being fired?
>>
>
> The last column of xc_auditlog is node type.
>
>
>>
>>
>>
>> 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
>>
>
>


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