Menu
▾
▴
Re: [Postgres-xc-developers] Patch for trigger shippability
|
From: Amit K. <ami...@en...> - 2013-05-20 06:01:07
|
On 17 May 2013 17:16, Ashutosh Bapat <ash...@en...>wrote: > > > > 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. >> >> I verified using Abbas's patch that the with.sql passes with both mine and Abbas's patch applied. I am committing my patch. > > 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 > |
