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.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