On 2017/01/14 6:24, Robert Haas wrote: > On Tue, Jan 10, 2017 at 6:06 AM, Amit Langote wrote: >> >> Thanks! I realized however that the approach I used in 0002 of passing >> the original slot to ExecConstraints() fails in certain situations. For >> example, if a BR trigger changes the tuple, the original slot would not >> receive those changes, so it will be wrong to use such a tuple anymore. >> In attached 0001, I switched back to the approach of converting the >> partition-tupdesc-based tuple back to the root partitioned table's format. >> The converted tuple contains the changes by BR triggers, if any. Sorry >> about some unnecessary work. > > Hmm. Even with this patch, I wonder if this is really correct. I > mean, isn't the root of the problem here that ExecConstraints() is > expecting that resultRelInfo matches slot, and it doesn't?
The problem is that whereas the SlotValueDescription that we build to show in the error message should be based on the tuple that was passed to ExecInsert() or whatever NextCopyFrom() returned for CopyFrom() to process, it might fail to be the case if the tuple was needed to be converted after tuple routing. slot (the tuple in it and its tuple descriptor) and resultRelInfo that ExecConstraint() receives *do* correspond with each other, even after possible tuple conversion following tuple-routing, and hence constraint checking itself works fine (since commit 2ac3ef7a01 ). As said, it's the val_desc built to show in the error message being based on the converted-for-partition tuple that could be seen as a problem - is it acceptable if we showed in the error message whatever the converted-for-partition tuple looks like which might have columns ordered differently from the root table? If so, we could simply forget the whole thing, including reverting f1b4c771 . An example: create table p (a int, b char, c int) partition by list (a); create table p1 (b char, c int, a int); -- note the column order alter table p attach partition p1 for values in (1); alter table p add constraint check_b check (b = 'x'); insert into p values (1, 'y', 1); ERROR: new row for relation "p1" violates check constraint "check_b" DETAIL: Failing row contains (y, 1, 1). Note that "(y, 1, 1)" results from using p1's descriptor on the converted tuple. As long that's clear and acceptable, I think we need not worry about this patch and revert the previously committed patch for this "problem". > And why > isn't that also a problem for the things that get passed resultRelInfo > and slot after tuple routing and before ExecConstraints? In > particular, in copy.c, ExecBRInsertTriggers. If I explained the problem (as I'm seeing it) well enough above, you may see why that's not an issue in other cases. Other sub-routines viz. ExecBRInsertTriggers, ExecWithCheckOptions for RLS INSERT WITH CHECK policies, ExecARInsertTriggers, etc. do receive the correct slot and resultRelInfo for whatever they do with a given tuple and the relation (partition) it is being inserted into. However, if we do consider the above to be a problem, then we would need to fix ExecWithCheckOptions() to do whatever we decide ExecConstraints() should do, because it can also report WITH CHECK violation for a tuple. > Committed 0002. Thanks, Amit  https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2ac3ef7  https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=f1b4c77 -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers