Re: Fwd: Problem with a "complex" upsert - Mailing list pgsql-bugs

Hi,

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.

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