Problem with a "complex" upsert

I'm trying to do an upsert to an updatable view with the following SQL
query:

INSERT INTO "acs2014_5yr"."b01003" (geoid, b01003001)

SELECT (left(acs.geoid, 7) || bi.blockid) geoid, (b01003001 *
(percentage*100.0)) b01003001
FROM "tiger2015".blocks_interpolation bi
INNER JOIN "acs2014_5yr"."b01003" acs ON bi.blockgroupid =
substr(acs.geoid,8)
WHERE acs.geoid = '15000US020200001013' AND char_length(substr(acs.geoid,
8)) = 12
ON CONFLICT (geoid) DO UPDATE SET (b01003001) = ROW(EXCLUDED.b01003001);

The View is:

View "acs2014_5yr.b01003"
Column | Type | Collation | Nullable | Default |
Storage | Description
-----------+-----------------------+-----------+----------+-
--------+----------+-------------
geoid | character varying(40) | | | |
extended |
b01003001 | double precision | | | |
plain |
View definition:
SELECT seq0003.geoid,
seq0003.b01003001
FROM acs2014_5yr.seq0003;

If I don't get any conflict everything works as intended but if we hit a
conflict then I get the following error message:

ERROR: attribute 2 of type record has the wrong type
DETAIL: Table has type character varying, but query expects double
precision.

Looks like it's trying to use the geoid value in the b01003001 field.

I've tried using the source insert table data but the server crashes:

INSERT INTO "acs2014_5yr"."b01003" (geoid, b01003001)

SELECT (left(acs.geoid, 7) || bi.blockid) geoid, (b01003001 *
(percentage*100.0))::float b01003001
FROM "tiger2015".blocks_interpolation bi
INNER JOIN "acs2014_5yr"."b01003" acs ON bi.blockgroupid =
substr(acs.geoid,8)
WHERE acs.geoid = '15000US020200001013' AND char_length(substr(acs.geoid,
8)) = 12
ON CONFLICT (geoid) DO UPDATE SET (b01003001) = ROW("acs2014_5yr"."b01003".
b01003001);

server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

Any clues? Could be a bug? I see something similar here
/messages/by-id/CAEzk6fdzJ3xYQZGbcuYM2rBd2BuDk
UksmK=mY9UYYDugg_GgZg@mail.gmail.com and it was a bug

On Thu, 21 Jun 2018 at 15:07, Mario De Frutos Dieguez <
mariodefrutos@gmail.com> wrote:

ON CONFLICT (geoid) DO UPDATE SET (b01003001) = ROW(EXCLUDED.b01003001);

​
At first glance, shouldn't the query simply be

SET b01003001 = EXCLUDED.b01003001;

?

The second part of your email does suggest a bug though, syntax shouldn't
cause a crash.

Geoff

In this case is just one column but in other queries I'm updating
multiple columns thats why I set the ROW thing

2018-06-21 16:40 GMT+02:00 Geoff Winkless <pgsqladmin@geoff.dj>:

On Thu, 21 Jun 2018 at 15:46, Mario de Frutos Dieguez
<mariodefrutos@gmail.com> wrote:

In this case is just one column but in other queries I'm updating
multiple columns thats why I set the ROW thing

Hmm. The documentation isn't explicit that that's valid syntax. Choices are
({expression|DEFAULT} [,...] |
( sub-SELECT )

and although the docs do say row constructors are valid as sub-SELECT
for comparisons I'm not sure that it fits here.

It does seem like it's trying to work anyway but my guess is that
"Table has type character varying". doesn't mean it's trying to use
the geoid value, but rather that it's implying that the ROW() (which
returns an anonymous type) is the same type as the target table of the
main INSERT query (rather than the type of the column in brackets).
Whether it counts as a bug or not (given that it's not explicitly
defined as allowed syntax) is probably an esoteric argument.

Are you using the ROW() syntax just because it's easier to build the
query programmatically? Otherwise I can't see why you wouldn't just
use SET col1=EXCLUDED.col1, col2=EXCLUDED.col2 [, ....]

Does
SET (b01003001) = (SELECT b01003001 FROM EXCLUDED)
work?

Geoff

On Thu, Jun 21, 2018 at 7:11 AM, Mario De Frutos Dieguez
<mariodefrutos@gmail.com> wrote:

I've tried using the source insert table data but the server crashes:

INSERT INTO "acs2014_5yr"."b01003" (geoid, b01003001)
SELECT (left(acs.geoid, 7) || bi.blockid) geoid, (b01003001 *
(percentage*100.0))::float b01003001
FROM "tiger2015".blocks_interpolation bi
INNER JOIN "acs2014_5yr"."b01003" acs ON bi.blockgroupid =
substr(acs.geoid,8)
WHERE acs.geoid = '15000US020200001013' AND char_length(substr(acs.geoid,
8)) = 12
ON CONFLICT (geoid) DO UPDATE SET (b01003001) =
ROW("acs2014_5yr"."b01003".b01003001);

server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

Any clues? Could be a bug? I see something similar here

It would be very helpful if you could get a stack trace of the crashing backend:

https://wiki.postgresql.org/wiki/Getting_a_stack_trace_of_a_running_PostgreSQL_backend_on_Linux/BSD#Getting_a_trace_from_a_randomly_crashing_backend

--
Peter Geoghegan

Mario De Frutos Dieguez <mariodefrutos@gmail.com> writes:

I'm trying to do an upsert to an updatable view with the following SQL
query:
...
If I don't get any conflict everything works as intended but if we hit a
conflict then I get the following error message:
ERROR: attribute 2 of type record has the wrong type
DETAIL: Table has type character varying, but query expects double
precision.

When filing a bug report, it's a good idea to provide both a self-
contained test case and a mention of what PG version you're using.

I guess from the ROW() syntax you used here, which isn't accepted pre-v10,
that you're using 10.0 or later, but that's not specific enough.

I tried to duplicate this problem using the attached script, but it
works for me.

FWIW, that error message definitely looks like a bug, but I can't
tell whether it's an already-fixed bug or there's some triggering
detail you didn't mention.

regards, tom lane

On 2018/06/22 2:05, Tom Lane wrote:

Mario De Frutos Dieguez <mariodefrutos@gmail.com> writes:

I'm trying to do an upsert to an updatable view with the following SQL
query:
...
If I don't get any conflict everything works as intended but if we hit a
conflict then I get the following error message:
ERROR: attribute 2 of type record has the wrong type
DETAIL: Table has type character varying, but query expects double
precision.

When filing a bug report, it's a good idea to provide both a self-
contained test case and a mention of what PG version you're using.

I guess from the ROW() syntax you used here, which isn't accepted pre-v10,
that you're using 10.0 or later, but that's not specific enough.

I tried to duplicate this problem using the attached script, but it
works for me.

FWIW, that error message definitely looks like a bug, but I can't
tell whether it's an already-fixed bug or there's some triggering
detail you didn't mention.

Having worked a little bit on the ON CONFLICT code recently, I was able to
guess at the triggering detail. At least, I was able to reproduce the
error and crash seen in the OP's report. Here's a minimal example:

create table foo (a text unique, b float);
insert into foo values ('xyxyxy', 1);

-- note the different order of columns in the view
create view foo_view as select b, a from foo;

insert into foo_view values (1, 'xyxyxy')
on conflict (a) do update set b = excluded.b;
ERROR: attribute 1 of type record has wrong type
DETAIL: Table has type text, but query expects double precision.

-- crash occurs if, like OP, I change EXCLUDED reference to target table
insert into foo_view values (1, 'xyxyxy') on conflict (a) do update set b
= foo_view.b;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

I tried debugging why that happens and concluded that rewriteTargetView
fails to *completely* account for the fact that the view's column's may
have different attribute numbers than the underlying table that the DO
UPDATE action will be applied to. Especially, even if the view's Vars are
replaced with those corresponding underlying base table's columns, the
TargetEntry's into which those Vars are contained are not refreshed, that
is, their resnos don't match varattnos.

I created a patch that seems to fix the issue, which also adds a
representative test in updatable_view.sql.

Thanks,
Amit

Wow, that's amazing news. Sorry for not being doing this in a proper way,
it was my first time guessing if I'm confronting a bug or not. For the next
time, I'll provide a more prepared answer :)

Thank you very much to all :)

2018-06-22 10:11 GMT+02:00 Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>:

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:

Having worked a little bit on the ON CONFLICT code recently, I was able to
guess at the triggering detail. At least, I was able to reproduce the
error and crash seen in the OP's report. Here's a minimal example:

create table foo (a text unique, b float);
insert into foo values ('xyxyxy', 1);

-- note the different order of columns in the view
create view foo_view as select b, a from foo;

Ah-hah.

I tried debugging why that happens and concluded that rewriteTargetView
fails to *completely* account for the fact that the view's column's may
have different attribute numbers than the underlying table that the DO
UPDATE action will be applied to. Especially, even if the view's Vars are
replaced with those corresponding underlying base table's columns, the
TargetEntry's into which those Vars are contained are not refreshed, that
is, their resnos don't match varattnos.

I created a patch that seems to fix the issue, which also adds a
representative test in updatable_view.sql.

Hm. I looked at this patch a bit. While the onConflictSet change looks
reasonable, I find the exclRelTlist change fishy. Shouldn't those resnos
correspond to the exclRelTlist's *own* vars, independently of what is or
isn't in the view_targetlist? And why is it OK to ignore failure to find
a match?

The provided test case doesn't seem to me to prove that that code is OK.
AFAICS, exclRelTlist only gets used by EXPLAIN, and there's no EXPLAIN
output in the test case.

regards, tom lane

On Tue, Jul 10, 2018 at 1:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I tried debugging why that happens and concluded that rewriteTargetView
fails to *completely* account for the fact that the view's column's may
have different attribute numbers than the underlying table that the DO
UPDATE action will be applied to. Especially, even if the view's Vars are
replaced with those corresponding underlying base table's columns, the
TargetEntry's into which those Vars are contained are not refreshed, that
is, their resnos don't match varattnos.

I created a patch that seems to fix the issue, which also adds a
representative test in updatable_view.sql.

Hm. I looked at this patch a bit. While the onConflictSet change looks
reasonable, I find the exclRelTlist change fishy. Shouldn't those resnos
correspond to the exclRelTlist's *own* vars, independently of what is or
isn't in the view_targetlist? And why is it OK to ignore failure to find
a match?

Any update on this, Amit? I would like to get this one out of the way soon.

Thanks
--
Peter Geoghegan

On 2018/07/31 7:53, Peter Geoghegan wrote:

On Tue, Jul 10, 2018 at 1:59 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I tried debugging why that happens and concluded that rewriteTargetView
fails to *completely* account for the fact that the view's column's may
have different attribute numbers than the underlying table that the DO
UPDATE action will be applied to. Especially, even if the view's Vars are
replaced with those corresponding underlying base table's columns, the
TargetEntry's into which those Vars are contained are not refreshed, that
is, their resnos don't match varattnos.

I created a patch that seems to fix the issue, which also adds a
representative test in updatable_view.sql.

Hm. I looked at this patch a bit. While the onConflictSet change looks
reasonable, I find the exclRelTlist change fishy. Shouldn't those resnos
correspond to the exclRelTlist's *own* vars, independently of what is or
isn't in the view_targetlist? And why is it OK to ignore failure to find
a match?

Any update on this, Amit? I would like to get this one out of the way soon.

This has slipped my mind. I will look into updating the patch today.

Thanks,
Amit

Thanks for looking at the patch and sorry I couldn't reply sooner.

On 2018/07/11 5:59, Tom Lane wrote:

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:

Having worked a little bit on the ON CONFLICT code recently, I was able to
guess at the triggering detail. At least, I was able to reproduce the
error and crash seen in the OP's report. Here's a minimal example:

create table foo (a text unique, b float);
insert into foo values ('xyxyxy', 1);

-- note the different order of columns in the view
create view foo_view as select b, a from foo;

Ah-hah.

I tried debugging why that happens and concluded that rewriteTargetView
fails to *completely* account for the fact that the view's column's may
have different attribute numbers than the underlying table that the DO
UPDATE action will be applied to. Especially, even if the view's Vars are
replaced with those corresponding underlying base table's columns, the
TargetEntry's into which those Vars are contained are not refreshed, that
is, their resnos don't match varattnos.

I created a patch that seems to fix the issue, which also adds a
representative test in updatable_view.sql.

Hm. I looked at this patch a bit. While the onConflictSet change looks
reasonable, I find the exclRelTlist change fishy. Shouldn't those resnos
correspond to the exclRelTlist's *own* vars, independently of what is or
isn't in the view_targetlist? And why is it OK to ignore failure to find
a match?

The provided test case doesn't seem to me to prove that that code is OK.
AFAICS, exclRelTlist only gets used by EXPLAIN, and there's no EXPLAIN
output in the test case.

On further study, I have concluded that EXCLUDED pseudo-relation and any
references to it in the sub-expressions of OnConflictExpr need to be
revised after the rewriter has substituted target view relation with its
underlying base relation.

As things stand today, transformOnConflictClause creates the EXCLUDED
pseudo-relation based on the original target relation of the query, which
in this case is the view relation. rewriteTargetView replaces the view
relation with its underlying base relation, subject to various
restrictions on what the query can then do, such as not being able to
update columns that are not present in the underlying base relation.

On the same lines, I think OnConflictExpr shouldn't be allowed to contain
references to view's own columns via the EXCLUDED pseudo-relation. That's
because ON CONFLICT's execution machinery would be able to access only
those columns of the EXCLUDED tuple that are present in the underlying
physical relation. Hence, to account for the view relation's substitution
with its underlying physical relation, we should discard the original
EXCLUDED range table entry and target list (exclRelTlist) that parser
created based on the view relation and recreate both based on the
substituted base rel. Furthermore, any Vars contained in OnConflictExpr's
sub-expressions that reference original EXCLUDED rte will need to be
substituted with Vars based on the revised rte.

Attaching the updated patch, which is quite heavily revised from the
earlier patch, given the above findings.

Thanks,
Amit

On 2 August 2018 at 11:38, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:

Attaching the updated patch, which is quite heavily revised from the
earlier patch, given the above findings.

This doesn't look right to me. It breaks the following case which
currently works in HEAD:

--
drop table if exists foo cascade;
create table foo (a int unique, b text);
create view foo_view as select a as aa, b as bb from foo;

insert into foo_view (aa,bb) values (1,'x');
insert into foo_view (aa,bb) values (1,'y')
on conflict (aa) do update set bb = excluded.bb;
select * from foo_view;
--

I also don't see why it should reject columns from the view that
aren't in the base relation. Such columns need to remain unchanged in
the UPDATE because they're non-updatable, but they're still logically
part of the new excluded view tuple, and so it may still be useful to
refer to them in other parts of the auxiliary UPDATE.

Regards,
Dean

On 3 August 2018 at 07:52, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

This doesn't look right to me. It breaks the following case ...

Here's an updated patch that fixes this.

I also don't see why it should reject columns from the view that
aren't in the base relation.

This patch also allows access to view columns that aren't in the
underlying base relation. The rationale for the result in the new test
case where it attempts to insert (1,'y') into columns (aa,bb) of the
view is that the new view row that would have resulted if the insert
had succeeded is ('y',1,(1,'y')), hence that's what excluded.* should
be for the view in the "on conflict" action, and there should be no
problem referring to any part of that excluded view tuple.

Regards,
Dean

Thanks Dean for taking a look.

On 2018/08/03 18:40, Dean Rasheed wrote:

On 3 August 2018 at 07:52, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

This doesn't look right to me. It breaks the following case ...

Hmm yeah, matching view and base relation names like that was silly.

Here's an updated patch that fixes this.

I also don't see why it should reject columns from the view that
aren't in the base relation.

This patch also allows access to view columns that aren't in the
underlying base relation. The rationale for the result in the new test
case where it attempts to insert (1,'y') into columns (aa,bb) of the
view is that the new view row that would have resulted if the insert
had succeeded is ('y',1,(1,'y')), hence that's what excluded.* should
be for the view in the "on conflict" action, and there should be no
problem referring to any part of that excluded view tuple.

Ah, I see what you did there with converting EXCLUDED column references.
I had tried to do the coversion from view attnos to base rel attnos using
tupconvert.c. When fiddling with that, I had to install that restriction
of not allowing accessing view's own columns via EXCLUDED, *because of*
trying to convert using tupconvert.c. Somehow, I had also became
convinced that restricting it like that made sense semantically, which as
you've shown, it doesn't.

After seeing your first email, I had started replacing the tupconvert.c
based conversion (which wouldn't even be readily backpatchable to 9.5) to
ReplaceVarsFromTargetList one, but you beat me to it.

Your updated version looks good and also nice that it has more tests. One
thing that stood out to me was how column references via EXCLUDED now
refer to base rel column names, but I guess that's fine.

create table foo (a int unique, b int);
create view foo_view as select b as bb, a + 1 as cc, a as aa from foo;

explain insert into foo_view (aa, bb) select 1, 1 on conflict (aa) do
update set bb = excluded.bb where excluded.cc > 1;
QUERY PLAN
─────────────────────────────────────────────────
Insert on foo (cost=0.00..0.01 rows=1 width=8)
Conflict Resolution: UPDATE
Conflict Arbiter Indexes: foo_a_key
Conflict Filter: ((excluded.a + 1) > 1)
-> Result (cost=0.00..0.01 rows=1 width=8)
(5 rows)

explain insert into foo_view (aa, bb) select 1, 1 on conflict (aa) do
update set bb = excluded.bb where excluded.aa > 1;
QUERY PLAN
─────────────────────────────────────────────────
Insert on foo (cost=0.00..0.01 rows=1 width=8)
Conflict Resolution: UPDATE
Conflict Arbiter Indexes: foo_a_key
Conflict Filter: (excluded.a > 1)
-> Result (cost=0.00..0.01 rows=1 width=8)
(5 rows)

Thanks again.

Regards,
Amit

Hi Dean,

On 2018-08-03 10:40:50 +0100, Dean Rasheed wrote:

On 3 August 2018 at 07:52, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

This doesn't look right to me. It breaks the following case ...

Here's an updated patch that fixes this.

Are you planning to push a version of this soon? It'd be good to
get this included in the next set of releases...

- Andres

On 3 August 2018 at 19:46, Andres Freund <andres@anarazel.de> wrote:

Are you planning to push a version of this soon? It'd be good to
get this included in the next set of releases...

Yes, agreed. I'll try to do it this weekend.

Regards,
Dean

Dean Rasheed <dean.a.rasheed@gmail.com> writes:

On 3 August 2018 at 19:46, Andres Freund <andres@anarazel.de> wrote:

Are you planning to push a version of this soon? It'd be good to
get this included in the next set of releases...

Yes, agreed. I'll try to do it this weekend.

Keep in mind that we are hard up against the release deadline.
This is a bad weekend to be pushing anything you are not 100.00%
sure of; the later, the more so, as by Sunday you will probably not
get a complete set of buildfarm reports before the wrap happens.

Balance the risks of shipping a broken release vs. waiting one
more quarter to ship the fix. After a quick look at the size
of the patch, my own inclination if I were the committer would
be to wait till after the releases are out. Or you might
consider pushing only to 11+HEAD, with the expectation of
back-patching later after we've gotten some beta results.

regards, tom lane

On 2018-08-03 18:32:57 -0400, Tom Lane wrote:

Dean Rasheed <dean.a.rasheed@gmail.com> writes:

On 3 August 2018 at 19:46, Andres Freund <andres@anarazel.de> wrote:

Are you planning to push a version of this soon? It'd be good to
get this included in the next set of releases...

Yes, agreed. I'll try to do it this weekend.

Keep in mind that we are hard up against the release deadline.

Right.

This is a bad weekend to be pushing anything you are not 100.00%
sure of; the later, the more so, as by Sunday you will probably not
get a complete set of buildfarm reports before the wrap happens.

Balance the risks of shipping a broken release vs. waiting one
more quarter to ship the fix. After a quick look at the size
of the patch, my own inclination if I were the committer would
be to wait till after the releases are out. Or you might
consider pushing only to 11+HEAD, with the expectation of
back-patching later after we've gotten some beta results.

This results in clearly inserting wrong data and/or crashing the
server. And it's not a huge effect outside of already broken scenarios.
I think we definitely should try to get this in.

Greetings,

Andres Freund