Re: BEFORE ROW triggers for partitioned tables

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(dot)bapat(at)2ndquadrant(dot)com> wrote:
>
>
>
> On Fri, 13 Mar 2020 at 21:55, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>>
>> On 2020-Mar-11, Ashutosh Bapat wrote:
>>
>> > On Thu, Feb 27, 2020 at 10:22 PM Alvaro Herrera
>> > <alvherre(at)2ndquadrant(dot)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 of partition. 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