Thanks for your review and comments, Amit! Amit Khandekar <amit.khande...@enterprisedb.com> wrote: > On 21 June 2014 23:36, Kevin Grittner <kgri...@ymail.com> wrote: >> Kevin Grittner <kgri...@ymail.com> wrote: >> I didn't change the tuplestores to TID because it seemed to me that >> it would preclude using transition relations with FDW triggers, and >> it seemed bad not to support that. Does anyone see a way around >> that, or feel that it's OK to not support FDW triggers in this >> regard? > > I think it is ok to use tuplestores for now, but as mentioned by you > somewhere else in the thread, later on we should change this to using > tids as an optimization.
Well, the optimization would probably be to use a tuplestore of tids referencing modified tuples in the base table, rather than a tuplestore of the data itself. But I think we're in agreement. >> Does this look good otherwise, as far as it goes? > > I didn't yet extensively go through the patch, but before that, just a > few quick comments: > > I see that the tupelstores for transition tables are stored per query > depth. If the > DML involves a table that has multiple child tables, it seems as though > a single tuplestore would be shared among all these tables. That means > if we define > such triggers using transition table clause for all the child tables, then > the trigger function for a child table will see tuples from other child tables > as well. Is that true ? I don't think so. I will make a note of the concern to confirm by testing. > If it is, it does not make sense. I agree. > I tried to google some SQLs that use REFERENCING clause with triggers. > It looks like in some database systems, even the WHEN clause of CREATE TRIGGER > can refer to a transition table, just like how it refers to NEW and > OLD row variables. > > For e.g. : > CREATE TRIGGER notify_dept > AFTER UPDATE ON weather > REFERENCING NEW_TABLE AS N_TABLE > NEW AS N_ROW > FOR EACH ROW > WHEN ((SELECT AVG (temperature) FROM N_TABLE) > 10) > BEGIN > notify_department(N_ROW.temperature, N_ROW.city); > END > > Above, it is used to get an aggregate value of all the changed rows. I think > we do not currently support aggregate expressions in the where clause, but > with > transition tables, it makes more sense to support it later if not now. Interesting point; I had not thought about that. Will see if I can include support for that in the patch for the next CF; failing that; I will at least be careful to not paint myself into a corner where it is unduly hard to do later. -- Kevin Grittner EDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers