Re: BEFORE ROW triggers for partitioned tables

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