Re: [HACKERS] Fixing target-column indirection in INSERT with multiple VALUES

[Tom Lane]

Kevin Grittner  writes:
> On Wed, Jul 27, 2016 at 1:47 PM, Tom Lane  wrote:
>> this would require an initdb because it changes the
>> representation of stored rules for cases like this,

> So pg_upgrade would not work at all for the version this goes into,

No, pg_upgrade wouldn't have a problem.  The entire point of pg_upgrade
is to cope with system catalog changes.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fixing target-column indirection in INSERT with multiple VALUES

[Kevin Grittner]

On Wed, Jul 27, 2016 at 1:47 PM, Tom Lane  wrote:

> I looked into the problem reported in bug #14265,

> attached is a proposed patch that fixes this bug

> this would require an initdb because it changes the
> representation of stored rules for cases like this,

So pg_upgrade would not work at all for the version this goes into,
or do you see us testing for such cases and allowing pg_upgrade if
none are found?  Or do you see a way to allow pg_upgrade in the
face of such stored rules?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Fixing target-column indirection in INSERT with multiple VALUES

[Tom Lane]

I looked into the problem reported in bug #14265,
https://www.postgresql.org/message-id/20160727005725.7438.26...@wrigleys.postgresql.org

The core of the problem is a design decision that seems pretty bad in
hindsight: if we have array-subscripting or field-selection decoration
in the target column list of an INSERT with multiple VALUES rows, then
we apply transformAssignedExpr() to each element of each VALUES row
independently.  So for example in
INSERT INTO foo (col[1]) VALUES (1), (2), (3)
there are going to be three identical ArrayRef nodes (with different
input expressions) inside the VALUES RTE, and then just a Var referencing
the VALUES RTE in the top-level targetlist.  While that works, it's not
so good when we have
INSERT INTO foo (col[1], col[2]) VALUES (1, 2), (3, 4)
because the rewriter needs to merge the assignments to "col" by nesting
the ArrayRefs together, but it fails to find the ArrayRefs in the
top-level targetlist, leading to the complained-of error.

Other reasons not to like this design are the space taken by the redundant
copies of the ArrayRef nodes, and the ugly logic needed in ruleutils.c
to deal with this situation, which is unlike either the INSERT-with-
single-VALUES-row or the INSERT-SELECT cases.

So attached is a proposed patch that fixes this bug by moving the ArrayRef
and/or FieldStore nodes associated with the column target list into the
top-level targetlist.  I still have it calling transformAssignedExpr
for each VALUES expression, which means that the parser still generates
(and then throws away) ArrayRef/FieldStore nodes for each VALUES row when
there is indirection decoration in the target list.  It would probably be
possible to avoid that, but it would take very invasive refactoring of
transformAssignmentIndirection and allied routines, and I'm dubious that
the case comes up often enough to be worth complicating that code even
more.

Another issue is that this would require an initdb because it changes the
representation of stored rules for cases like this, which means we could
only fix it in HEAD and there wouldn't be a back-patch.  Given that it's
been like this forever and we've not had complaints before, I think that's
acceptable.

Conceivably we could fix this without initdb-forcing changes in the
parser's output, if we taught the rewriter to apply rewriteTargetListIU
to each row of the VALUES RTE rather than to the top-level targetlist.
But that would be a complex mess for multiple reasons --- for instance,
it would change the output column set for the RTE.  So I'm not at all
excited about pursuing that path.

In short, I propose applying the attached to HEAD but not attempting
to fix the issue in the back branches.

regards, tom lane

diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 29c8c4e..eac86cc 100644
*** a/src/backend/parser/analyze.c
--- b/src/backend/parser/analyze.c
*** post_parse_analyze_hook_type post_parse_
*** 51,57 
  static Query *transformDeleteStmt(ParseState *pstate, DeleteStmt *stmt);
  static Query *transformInsertStmt(ParseState *pstate, InsertStmt *stmt);
  static List *transformInsertRow(ParseState *pstate, List *exprlist,
!    List *stmtcols, List *icolumns, List *attrnos);
  static OnConflictExpr *transformOnConflictClause(ParseState *pstate,
  		  OnConflictClause *onConflictClause);
  static int	count_rowexpr_columns(ParseState *pstate, Node *expr);
--- 51,58 
  static Query *transformDeleteStmt(ParseState *pstate, DeleteStmt *stmt);
  static Query *transformInsertStmt(ParseState *pstate, InsertStmt *stmt);
  static List *transformInsertRow(ParseState *pstate, List *exprlist,
!    List *stmtcols, List *icolumns, List *attrnos,
!    bool strip_indirection);
  static OnConflictExpr *transformOnConflictClause(ParseState *pstate,
  		  OnConflictClause *onConflictClause);
  static int	count_rowexpr_columns(ParseState *pstate, Node *expr);
*** transformInsertStmt(ParseState *pstate, 
*** 619,625 
  		/* Prepare row for assignment to target table */
  		exprList = transformInsertRow(pstate, exprList,
  	  stmt->cols,
! 	  icolumns, attrnos);
  	}
  	else if (list_length(selectStmt->valuesLists) > 1)
  	{
--- 620,627 
  		/* Prepare row for assignment to target table */
  		exprList = transformInsertRow(pstate, exprList,
  	  stmt->cols,
! 	  icolumns, attrnos,
! 	  false);
  	}
  	else if (list_length(selectStmt->valuesLists) > 1)
  	{
*** transformInsertStmt(ParseState *pstate, 
*** 663,672 
  			exprLocation((Node *) sublist;
  			}
  
! 			/* Prepare row for assignment to target table */
  			sublist = transformInsertRow(pstate, sublist,
  		 stmt->cols,
! 		 icolumns, attrnos);
  
  			/*
  			 * We must assign collations now because assign_query_collations
--- 665,684 
  			exprLocation((Node *) sublist;