Re: BEFORE ROW triggers for partitioned tables
От | Ashutosh Bapat |
---|---|
Тема | Re: BEFORE ROW triggers for partitioned tables |
Дата | |
Msg-id | CAExHW5u-FYReDC48z94S2dhDFcsgrwYPdCvSpfOGt7UKQAqP+Q@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: BEFORE ROW triggers for partitioned tables (Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com>) |
Список | pgsql-hackers |
I was expecting that documentation somewhere covered the fact that BR triggers are not supported on a partitioned table. And this patch would remove/improve that portion of the code. But I don't see any documentation changes in this patch. On Tue, Mar 17, 2020 at 10:11 PM Ashutosh Bapat <ashutosh.bapat@2ndquadrant.com> wrote: > > > > On Fri, 13 Mar 2020 at 21:55, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> >> On 2020-Mar-11, Ashutosh Bapat wrote: >> >> > On Thu, Feb 27, 2020 at 10:22 PM Alvaro Herrera >> > <alvherre@2ndquadrant.com> wrote: >> >> > > * The new function I added, ReportTriggerPartkeyChange(), contains one >> > > serious bug (namely: it doesn't map attribute numbers properly if >> > > partitions are differently defined). >> > >> > IIUC the code in your patch, it seems you are just looking at >> > partnatts. But partition key can contain expressions also which are >> > stored in partexprs. So, I think the code won't catch change in the >> > partition key values when it contains expression. Using >> > RelationGetPartitionQual() will fix this problem and also problem of >> > attribute remapping across the partition hierarchy. >> >> Oh, of course. >> >> In fact, I don't need to deal with PartitionQual directly; I can just >> use ExecPartitionCheck(), since in ExecBRInsertTriggers et al we already >> have all we need. v2 attached. > > > Thanks. > >> >> insert into parted values (1, 1, 'uno uno v2'); -- fail >> ERROR: moving row to another partition during a BEFORE trigger is not supported >> DETAIL: Before trigger "t", row was to be in partition "public.parted_1_1" >> >> Note that in this implementation I no longer know which column is the >> problematic one, but I suppose users have clue enough. Wording >> suggestions welcome. > > > When we have expression as a partition key, there may not be one particular column which causes the row to move out ofpartition. So, this should be fine. > A slight wording suggestion below. > > - * Complain if we find an unexpected trigger type. > - */ > - if (!TRIGGER_FOR_AFTER(trigForm->tgtype)) > - elog(ERROR, "unexpected trigger \"%s\" found", > - NameStr(trigForm->tgname)); > > !AFTER means INSTEAD OF and BEFORE. Do you intend to allow INSTEAD OF triggers > as well? > - */ > - if (stmt->timing != TRIGGER_TYPE_AFTER) > > Same comment as the above? > > + /* > + * After a tuple in a partition goes through a trigger, the user > + * could have changed the partition key enough that the tuple > + * no longer fits the partition. Verify that. > + */ > + if (trigger->tgisclone && > > Why do we want to restrict this check only for triggers which are cloned from > the ancestors? > > + !ExecPartitionCheck(relinfo, slot, estate, false)) > + ereport(ERROR, > + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), > + errmsg("moving row to another partition during a BEFORE trigger is not supported"), > + errdetail("Before trigger \"%s\", row was to be in partition \"%s.%s\"", > > In the error message you removed above, we are mentioning BEFORE FOR EACH ROW > trigger. Should we continue to use the same terminology? > > I was wondering whether it would be good to check the partition constraint only > once i.e. after all before row triggers have been executed. This would avoid > throwing an error in case multiple triggers together worked to keep the tuple > in the same partition when individual trigger/s caused it to move out of that > partition. But then we would loose the opportunity to point out the before row > trigger which actually caused the row to move out of the partition. Anyway, > wanted to bring that for the discussion here. > > @@ -307,7 +307,7 @@ CreatePartitionDirectory(MemoryContext mcxt) > * > * The purpose of this function is to ensure that we get the same > * PartitionDesc for each relation every time we look it up. In the > - * face of current DDL, different PartitionDescs may be constructed with > + * face of concurrent DDL, different PartitionDescs may be constructed with > > Thanks for catching it. Looks unrelated though. > > +-- Before triggers and partitions > > The test looks good. Should we add a test for partitioned table with partition > key as expression? > > The approach looks good to me. > > -- > Best Wishes, > Ashutosh -- Best Wishes, Ashutosh Bapat
В списке pgsql-hackers по дате отправления: