Re: Fwd: Problem with a "complex" upsert

Hi,

On 2018-08-03 10:40:50 +0100, Dean Rasheed wrote:
> On 3 August 2018 at 07:52, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> > This doesn't look right to me. It breaks the following case ...
>
> Here's an updated patch that fixes this.

Looking through the patch.

> diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
> new file mode 100644
> index 05f5759..87e084b
> --- a/src/backend/parser/analyze.c
> +++ b/src/backend/parser/analyze.c
> @@ -1022,9 +1022,6 @@ transformOnConflictClause(ParseState *ps
> if (onConflictClause->action == ONCONFLICT_UPDATE)
> {
> Relation targetrel = pstate->p_target_relation;
> - Var *var;
> - TargetEntry *te;
> - int attno;
>
> /*
> * All INSERT expressions have been parsed, get ready for potentially
> @@ -1043,56 +1040,8 @@ transformOnConflictClause(ParseState *ps
> false, false);
> exclRte->relkind = RELKIND_COMPOSITE_TYPE;
> exclRelIndex = list_length(pstate->p_rtable);
> -
> - /*
> - * Build a targetlist representing the columns of the EXCLUDED pseudo
> - * relation. Have to be careful to use resnos that correspond to
> - * attnos of the underlying relation.
> - */
> - for (attno = 0; attno < RelationGetNumberOfAttributes(targetrel); attno++)
> - {
> - Form_pg_attribute attr = TupleDescAttr(targetrel->rd_att, attno);
> - char *name;
> -
> - if (attr->attisdropped)
> - {
> - /*
> - * can't use atttypid here, but it doesn't really matter what
> - * type the Const claims to be.
> - */
> - var = (Var *) makeNullConst(INT4OID, -1, InvalidOid);
> - name = "";
> - }
> - else
> - {
> - var = makeVar(exclRelIndex, attno + 1,
> - attr->atttypid, attr->atttypmod,
> - attr->attcollation,
> - 0);
> - name = pstrdup(NameStr(attr->attname));
> - }
> -
> - te = makeTargetEntry((Expr *) var,
> - attno + 1,
> - name,
> - false);
> -
> - /* don't require select access yet */
> - exclRelTlist = lappend(exclRelTlist, te);
> - }
> -
> - /*
> - * Add a whole-row-Var entry to support references to "EXCLUDED.*".
> - * Like the other entries in exclRelTlist, its resno must match the
> - * Var's varattno, else the wrong things happen while resolving
> - * references in setrefs.c. This is against normal conventions for
> - * targetlists, but it's okay since we don't use this as a real tlist.
> - */
> - var = makeVar(exclRelIndex, InvalidAttrNumber,
> - targetrel->rd_rel->reltype,
> - -1, InvalidOid, 0);
> - te = makeTargetEntry((Expr *) var, InvalidAttrNumber, NULL, true);
> - exclRelTlist = lappend(exclRelTlist, te);
> + exclRelTlist = BuildOnConflictExcludedTargetlist(targetrel,
> + exclRelIndex);
>
> /*
> * Add EXCLUDED and the target RTE to the namespace, so that they can
> @@ -1124,6 +1073,75 @@ transformOnConflictClause(ParseState *ps
>
> return result;
> }
> +
> +
> +/*
> + * BuildOnConflictExcludedTargetlist
> + * Create the target list of EXCLUDED pseudo-relation of ON CONFLICT
> + *
> + * Note: Exported for use in the rewriter.
> + */
> +List *
> +BuildOnConflictExcludedTargetlist(Relation targetrel,
> + Index exclRelIndex)
> +{
> + List *result = NIL;
> + int attno;
> + Var *var;
> + TargetEntry *te;
> +
> + /*
> + * Build a targetlist representing the columns of the EXCLUDED pseudo
> + * relation. Have to be careful to use resnos that correspond to attnos
> + * of the underlying relation.
> + */
> + for (attno = 0; attno < RelationGetNumberOfAttributes(targetrel); attno++)
> + {
> + Form_pg_attribute attr = TupleDescAttr(targetrel->rd_att, attno);
> + char *name;
> +
> + if (attr->attisdropped)
> + {
> + /*
> + * can't use atttypid here, but it doesn't really matter what type
> + * the Const claims to be.
> + */
> + var = (Var *) makeNullConst(INT4OID, -1, InvalidOid);
> + name = "";
> + }
> + else
> + {
> + var = makeVar(exclRelIndex, attno + 1,
> + attr->atttypid, attr->atttypmod,
> + attr->attcollation,
> + 0);
> + name = pstrdup(NameStr(attr->attname));
> + }
> +
> + te = makeTargetEntry((Expr *) var,
> + attno + 1,
> + name,
> + false);
> +
> + /* don't require select access yet */
> + result = lappend(result, te);
> + }
> +
> + /*
> + * Add a whole-row-Var entry to support references to "EXCLUDED.*". Like
> + * the other entries in exclRelTlist, its resno must match the Var's
> + * varattno, else the wrong things happen while resolving references in
> + * setrefs.c. This is against normal conventions for targetlists, but
> + * it's okay since we don't use this as a real tlist.
> + */
> + var = makeVar(exclRelIndex, InvalidAttrNumber,
> + targetrel->rd_rel->reltype,
> + -1, InvalidOid, 0);
> + te = makeTargetEntry((Expr *) var, InvalidAttrNumber, NULL, true);
> + result = lappend(result, te);
> +
> + return result;
> +}

On a skim this purely is moving code around - no functional changes, right?

> +-- Check INSERT .. ON CONFLICT DO UPDATE works correctly when the view's
> +-- columns are named and ordered differently than the underlying table's.
> +create table uv_iocu_tab (a text unique, b float);
> +insert into uv_iocu_tab values ('xyxyxy', 1);
> +create view uv_iocu_view as select b, b+1 as c, a from uv_iocu_tab;
> +insert into uv_iocu_view (a, b) values ('xyxyxy', 2)
> + on conflict (a) do update set b = uv_iocu_view.b;
> +
> +-- OK to access view columns that are not present in underlying base
> +-- relation in the ON CONFLICT portion of the query
> +explain (costs off)
> +insert into uv_iocu_view (a, b) values ('xyxyxy', 3)
> + on conflict (a) do update set b = excluded.b where excluded.c > 0;
> +
> +insert into uv_iocu_view (a, b) values ('xyxyxy', 3)
> + on conflict (a) do update set b = excluded.b where excluded.c > 0;
> +-- should display 'xyxyxy, 3'
> +select * from uv_iocu_tab;
> +drop view uv_iocu_view;
> +drop table uv_iocu_tab;
> +
> +-- Example with whole-row references to the view
> +create table uv_iocu_tab (a int unique, b text);
> +create view uv_iocu_view as
> + select b as bb, a as aa, uv_iocu_tab::text as cc from uv_iocu_tab;
> +
> +insert into uv_iocu_view (aa,bb) values (1,'x');
> +explain (costs off)
> +insert into uv_iocu_view (aa,bb) values (1,'y')
> + on conflict (aa) do update set bb = 'Rejected: '||excluded.*
> + where excluded.aa > 0
> + and excluded.bb != ''
> + and excluded.cc is not null;
> +insert into uv_iocu_view (aa,bb) values (1,'y')
> + on conflict (aa) do update set bb = 'Rejected: '||excluded.*
> + where excluded.aa > 0
> + and excluded.bb != ''
> + and excluded.cc is not null;
> +select * from uv_iocu_view;
> +
> +-- Test omiting a column of the base relation
> +delete from uv_iocu_view;
> +insert into uv_iocu_view (aa,bb) values (1,'x');
> +insert into uv_iocu_view (aa) values (1)
> + on conflict (aa) do update set bb = 'Rejected: '||excluded.*;
> +select * from uv_iocu_view;
> +
> +-- Should fail to update non-updatable columns
> +insert into uv_iocu_view (aa) values (1)
> + on conflict (aa) do update set cc = 'XXX';
> +
> +drop view uv_iocu_view;
> +drop table uv_iocu_tab;

Could you add a column that's just a const and one that that's now() or
something? And based on those add a test that makes sure we don't do
stupid thinks when they're referred to via EXCLUDED? AFAICS that
currently should work correctly, but it'd be good to test that.

Peter, I think, independent of this bug, I think it'd be good to add a
few tests around arbiter expression for ON CONFLICT over a view.

Greetings,

Andres Freund