Re: [BUGS] BUG #14808: V10-beta4, backend abort
От | Thomas Munro |
---|---|
Тема | Re: [BUGS] BUG #14808: V10-beta4, backend abort |
Дата | |
Msg-id | CAEepm=2Fu=W7MFqsTcBGQEFwv05F6DWV0mv+vnPXReUcmeZ4Kg@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [BUGS] BUG #14808: V10-beta4, backend abort (Tom Lane <tgl@sss.pgh.pa.us>) |
Ответы |
Re: [BUGS] BUG #14808: V10-beta4, backend abort
(Tom Lane <tgl@sss.pgh.pa.us>)
Re: [BUGS] BUG #14808: V10-beta4, backend abort (Tom Lane <tgl@sss.pgh.pa.us>) |
Список | pgsql-bugs |
On Fri, Sep 15, 2017 at 9:45 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Attached is a draft patch for this. Some initial feedback: Compiles cleanly and make check-world passes here. with wcte as (insert into table1 values (42)) insert into table2 values ('hello world');NOTICE: trigger = table2_trig,new table = ("hello world")NOTICE: trigger = table1_trig, new table = (42) +with wcte as (insert into table1 values (43)) + insert into table1 values (44); +NOTICE: trigger = table1_trig, new table = (43), (44) The effects of multiple ModifyTable nodes on the same table are merged. Good. I doubt anyone will notice but it might warrant a note somewhere that there is a user-visible change here: previously if you did this (without TTs) your trigger would have fired twice. +create trigger my_table_col_update_trig + after update of b on my_table referencing new table as new_table + for each statement execute procedure dump_insert(); +ERROR: transition tables cannot be specified for triggers with column lists Potential SQL standard non-compliance avoided. Good. delete from refd_table where length(b) = 3; -NOTICE: trigger = trig_table_delete_trig, old table = (2,"two a"), (2,"two b") -NOTICE: trigger = trig_table_delete_trig, old table = (11,"one a"), (11,"one b") +NOTICE: trigger = trig_table_delete_trig, old table = (2,"two a"), (2,"two b"), (11,"one a"), (11,"one b") The effects of fk cascade machinery are merged as discussed. Good, I think, but I'm a bit confused about how this works when the cascading operation also fires triggers. All the other tests show no change in behaviour. Good. What is going on here? === setup === create or replace function dump_delete() returns trigger language plpgsql as $$ begin raise notice 'trigger = %, old table = %, depth = %', TG_NAME, (select string_agg(old_table::text,', ' order by a) from old_table), pg_trigger_depth(); return null; end; $$; create table foo (a int primary key, b int references foo(a) on delete cascade); create trigger foo_s_trig after delete on foo referencing old table as old_table for each statement execute procedure dump_delete(); create trigger foo_r_trig after delete on foo referencing old table as old_table for each row execute procedure dump_delete(); insert into foo values (1, null), (2, 1); ===8<=== postgres=# delete from foo where a = 1; NOTICE: trigger = foo_r_trig, old table = (1,), depth = 1 NOTICE: trigger = foo_s_trig, old table = (1,), depth = 1 NOTICE: trigger = foo_r_trig, old table = (2,1), depth = 1 NOTICE: trigger = foo_s_trig, old table = (2,1), depth = 1 NOTICE: trigger = foo_s_trig, old table = <NULL>, depth = 1 DELETE 1 > Thomas Munro <thomas.munro@enterprisedb.com> writes: >> 1. Merging the transition tables when there are multiple wCTEs >> referencing the same table. Here's one idea: Rename >> MakeTransitionCaptureState() to GetTransitionCaptureState() and use a >> hash table keyed by table OID in >> afterTriggers.transition_capture_states[afterTriggers.query_depth] to >> find the TCS for the given TriggerDesc or create it if not found, so >> that all wCTEs find the same TransitionCaptureState object. The all >> existing callers continue to do what they're doing now, but they'll be >> sharing TCSs appropriately with other things in the plan. Note that >> TransitionCaptureState already holds tuplestores for each operation >> (INSERT, UPDATE, DELETE) so the OID of the table alone is a suitable >> key for the hash table (assuming we are ignoring the column-list part >> of the spec as you suggested). > > It seems unsafe to merge the TCS objects themselves, because the callers > assume that they can munge the tcs_map and tcs_original_insert_tuple > fields freely without regard for any other callers. So as I have it, > we still have a TCS for each caller, but the TCSes point at tuplestores > that can be shared across multiple callers for the same event type. > The tuplestores themselves are managed by the AfterTrigger data > structures. Also, because the TCS structs are just throwaway per-caller > data, it's uncool to reference them in the trigger event lists. > So I replaced ats_transition_capture with two pointers to the actual > tuplestores. Ok, that works and yeah it may be conceptually better. Also, maybe the tcs_original_insert_tuple as member of TCS is a bit clunky and could be reconsidered later: I was trying to avoid widening a bunch of function calls, but that might have been optimising for the wrong thing... > That bloats AfterTriggerSharedData a bit but I think it's > okay; we don't expect a lot of those structs in a normal query. Yeah, it was bloat avoidance that led me to find a way to use a single pointer. But at least it's in the shared part, so I don't think it matters too much. > I chose to make the persistent state (AfterTriggersTableData) independent > for each operation type. We could have done that differently perhaps, but > it seemed more complicated and less likely to match the spec's semantics. OK. Your GetAfterTriggersTableData(Oid relid, CmdType cmdType) is mirroring the spec's way of describing what happens (without the column list). + foreach(lc, qs->tables) + { + table = (AfterTriggersTableData *) lfirst(lc); + if (table->relid == relid && table->cmdType == cmdType && + !table->closed) + return table; + } Yeah, my suggestion of a hash table was overkill. (Maybe in future if we change our rules around inheritance this could finish up being searched for a lot of child tables; we can cross that bridge when we come to it.) I'm a little confused about the "closed" flag. This seems to imply that it's possible for there to be more than one separate tuplestore for a given (table, operation) in a given trigger execution context. Why is that OK? > The INSERT ON CONFLICT UPDATE mess is handled by creating two separate > TCSes with two different underlying AfterTriggersTableData structs. > The insertion tuplestore sees only the inserted tuples, the update > tuplestores see only the updated-pre-existing tuples. That adds a little > code to nodeModifyTable but it seems conceptually much cleaner. OK. The resulting behaviour is unchanged. >> 3. Merging the invocation after statement firing so that if you >> updated the same table directly and also via a wCTE and also >> indirectly via fk ON DELETE/UPDATE trigger then you still only get one >> invocation of the after statement trigger. Not sure exactly how... > > What I did here was to use the AfterTriggersTableData structs to hold > a flag saying we'd already queued statement triggers for this rel and > cmdType. There's probably more than one way to do that, but this seemed > convenient. Seems good. That implements the following (from whatever random draft spec I have): "A statement-level trigger that is considered as executed for a state change SC (in a given trigger execution context) is not subsequently executed for SC." > One thing I don't like too much about that is that it means there are > cases where the single statement trigger firing would occur before some > AFTER ROW trigger firings. Not sure if we promise anything about the > ordering in the docs. It looks quite expensive/complicated to try to > make it always happen afterwards, though, and it might well be totally > impossible if triggers cause more table updates to occur. I suppose that it might be possible for AfterTriggersTableData to record the location of a previously queued event so that you can later disable it and queue a replacement, with the effect of suppressing earlier firings rather than later ones. That might make sense if you think that after statement triggers should fire after all row triggers. I can't figure out from the spec whether that's expected and I'm not sure if it's useful. > Because MakeTransitionCaptureState now depends on the trigger query > level being active, I had to relocate the AfterTriggerBeginQuery calls > to occur before it. Right. > In passing, I refactored the AfterTriggers data structures a bit so > that we don't need to do as many palloc calls to manage them. Instead > of several independent arrays there's now one array of structs. Good improvement. -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-bugs
В списке pgsql-bugs по дате отправления: