Re: BEFORE ROW triggers for partitioned tables - Mailing list pgsql-hackers

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.

 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.

- * 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