Thread: ON CONFLICT DO UPDATE using EXCLUDED.column gives an error about mismatched types

On Mon, Aug 3, 2015 at 8:53 AM, Geoff Winkless <pgsqladmin@geoff.dj> wrote:
> the new table does not exhibit the same problem (so I'm assuming it's not
> easily reproducible and giving you a creation script isn't going to help).
>
> VACUUM FULL on the table makes no difference.
>
> Is there anything you guys can suggest that I can do to help narrow down the
> problem?

Yes. You should enable all of the following settings:

debug_print_parse (boolean)
debug_print_rewritten (boolean)
debug_print_plan (boolean)
debug_pretty_print (boolean)

which you can do dynamically from psql.

Then post the output for both the failing version (on that table where
you can reproduce the issue), and the other table where it seems that
you ought to be able to reproduce the problem, but you can't. Maybe
that'll show where the problem is.

Thanks
-- 
Peter Geoghegan

On Mon, Aug 3, 2015 at 8:53 AM, Geoff Winkless <pgsqladmin@geoff.dj> wrote:
> If I create a copy of the table using
>
> CREATE mytab (LIKE brokentab INCLUDING ALL);
> INSERT INTO mytab SELECT * FROM brokentab;

Also, did you drop any columns from the original "brokentab" table
where the bug can be reproduced?

-- 
Peter Geoghegan

On 2015-08-04 AM 02:57, Peter Geoghegan wrote:
> On Mon, Aug 3, 2015 at 8:53 AM, Geoff Winkless <pgsqladmin@geoff.dj> wrote:
>> If I create a copy of the table using
>>
>> CREATE mytab (LIKE brokentab INCLUDING ALL);
>> INSERT INTO mytab SELECT * FROM brokentab;
> 
> Also, did you drop any columns from the original "brokentab" table
> where the bug can be reproduced?
> 

This seem to be the case. I could reproduce the reported problem:

test=# CREATE TABLE upsert_fail_test(a int, b int, c smallint);
CREATE TABLE

test=# ALTER TABLE upsert_fail_test DROP c;
ALTER TABLE

test=# ALTER TABLE upsert_fail_test ADD c smallint;
ALTER TABLE

test=# ALTER TABLE upsert_fail_test ADD PRIMARY KEY (a, b, c);
ALTER TABLE

test=# INSERT INTO  upsert_fail_test(a, b, c) VALUES (1, 2, 0) ON CONFLICT
(a, b, c) DO UPDATE SET c = EXCLUDED.c;
INSERT 0 1

test=# INSERT INTO  upsert_fail_test(a, b, c) VALUES (1, 2, 0) ON CONFLICT
(a, b, c) DO UPDATE SET b = EXCLUDED.b;
INSERT 0 1

test=# INSERT INTO  upsert_fail_test(a, b, c) VALUES (1, 2, 0) ON CONFLICT
(a, b, c) DO UPDATE SET c = EXCLUDED.c;
ERROR:  attribute 3 has wrong type
DETAIL:  Table has type integer, but query expects smallint.

FWIW, I tried to look why that happens. It seems during set_plan_refs(),
fix_join_expr on the splan->onConflictSet targetlist using EXCLUDE
pseudo-rel's targetlist as inner targetlist causes columns c's varattno to
be changed to 3, whereas in the actual tuple descriptor it's 4 (dropped
and added). Eventually, ExecEvalScalarVar() complains when it finds attno
3 is a dropped attribute.

Thanks,
Amit

On 2015-08-04 PM 05:58, Geoff Winkless wrote:
> 
> ​Although it seems Amit has defined the problem better than I could, so
> this is a bit late to the party (!), yes, the table had been ALTERed after
> it was created (looking back through the history, that modification
> included at least one DROP COLUMN).
> 

It seems using any columns that used to be after a dropped columns cause
EXCLUDE pseudo-relation to misbehave. For example, I observed another symptom:

test=# CREATE TABLE upsert_fail_test(a int, b int, c int, d smallint);
CREATE TABLE

test=# ALTER TABLE upsert_fail_test DROP b;
ALTER TABLE

test=# ALTER TABLE upsert_fail_test ADD PRIMARY KEY (a, c, d);
ALTER TABLE

test=# INSERT INTO  upsert_fail_test(a, c, d) VALUES (1, 2, 3) ON CONFLICT
(a, c, d) DO UPDATE SET c = EXCLUDED.c;
INSERT 0 1

test=# INSERT INTO  upsert_fail_test(a, c, d) VALUES (1, 2, 3) ON CONFLICT
(a, c, d) DO UPDATE SET c = EXCLUDED.c;
ERROR:  null value in column "c" violates not-null constraint
DETAIL:  Failing row contains (1, null, 3).

Or, the EXCLUDED pseudo-rel failed to deliver '2' produced by the subplan
and instead produced a 'null' which I guess was caused by the dropped
column 'b'.

Perhaps, it may have to do with how EXCLUDED pseudo-rel's targetlist is
manipulated through parse-plan stage?

Thanks,
Amit

On Tue, Aug 4, 2015 at 2:29 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Perhaps, it may have to do with how EXCLUDED pseudo-rel's targetlist is
> manipulated through parse-plan stage?

I think so, yes.

I'll look into writing a fix for this later in the week.

Thanks for the report, Geoff, and thanks for the analysis Amit.

-- 
Peter Geoghegan

On 2015-08-05 AM 06:44, Peter Geoghegan wrote:
> On Tue, Aug 4, 2015 at 2:29 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Perhaps, it may have to do with how EXCLUDED pseudo-rel's targetlist is
>> manipulated through parse-plan stage?
>
> I think so, yes.
>
> I'll look into writing a fix for this later in the week.
>

Just a heads-up.

I forgot mentioning one thing later yesterday. The way exclRelTlist is
initialized, all the way in the beginning (transformOnConflictClause), is
most probably to blame. It uses expandRelAttrs() for other valid reasons;
but within it, expandRTE() is called with 'false' for include_dropped
(columns). I applied the attached (perhaps ugly) fix, and it seemed to fix
the issue. But, I guess you'll be able to come up with some better fix.

Thanks,
Amit

On Wed, Aug 5, 2015 at 12:58 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> I forgot mentioning one thing later yesterday. The way exclRelTlist is
> initialized, all the way in the beginning (transformOnConflictClause), is
> most probably to blame. It uses expandRelAttrs() for other valid reasons;
> but within it, expandRTE() is called with 'false' for include_dropped
> (columns). I applied the attached (perhaps ugly) fix, and it seemed to fix
> the issue. But, I guess you'll be able to come up with some better fix.

Thanks for working on this. I think you have this right, but I also
don't think we should discuss this fix outside of the context of a
wider discussion about the excluded targetlist.

What I'm going to do is roll this into my own pending patch to fix the
issue with wholerow vars, which is also down to a problem with the
excluded targetlist initially generated by calling expandRelAttrs().
Andres might want to take an alternative approach with that, so a
consolidation makes sense.

-- 
Peter Geoghegan

Peter,

On 2015-08-11 AM 07:37, Peter Geoghegan wrote:
> What I'm going to do is roll this into my own pending patch to fix the
> issue with wholerow vars, which is also down to a problem with the
> excluded targetlist initially generated by calling expandRelAttrs().
> Andres might want to take an alternative approach with that, so a
> consolidation makes sense.

Did you get around to making a patch for this?

Thanks,
Amit

On Wed, Sep 2, 2015 at 1:18 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Did you get around to making a patch for this?

I've worked on it inconsistently. I'll pick this up again soon. I may
take the opportunity to talk this over with Andres in person when we
meet at Postgres Open shortly.

-- 
Peter Geoghegan

Hi,

> db=# INSERT INTO brokentab(id, k1,k2,k3,k4,k5,k6,k7, smallval) VALUES
> (5,0,0,0,1,0,1,0, 0) ON CONFLICT (id, k1,k2,k3,k4,k5,k6,k7) DO UPDATE SET
> smallval=EXCLUDED.smallval;
> ERROR:  attribute 29 has wrong type
> DETAIL:  Table has type integer, but query expects smallint.

I pushed a fix for the issue. Could you verify that your original
problem does not exist anymore?

Thanks for testing Geoff, thanks for helping to nail this down Amit and
Peter.

Regards,

Andres