Fix WITH CHECK OPTION on views referencing postgres_fdw tables.

If a view references a foreign table, and the foreign table has a
BEFORE INSERT trigger, then it's possible for a tuple inserted or
updated through the view to be changed such that it violates the
view's WITH CHECK OPTION constraint.

Before this commit, postgres_fdw handled this case inconsistently. A
RETURNING clause on the INSERT or UPDATE statement targeting the view
would cause the finally-inserted tuple to be read back, and the WITH
CHECK OPTION violation would throw an error. But without a RETURNING
clause, postgres_fdw would not read the final tuple back, and WITH
CHECK OPTION would not throw an error for the violation (or may throw
an error when there is no real violation). AFTER ROW triggers on the
foreign table had a similar effect as a RETURNING clause on the INSERT
or UPDATE statement.

To fix, this commit retrieves the attributes needed to enforce the
WITH CHECK OPTION constraint along with the attributes needed for the
RETURNING clause (if any) from the remote side. Thus, the WITH CHECK
OPTION constraint is always evaluated against the final tuple after
any triggers on the remote side.

This fix may be considered inconsistent with CHECK constraints
declared on foreign tables, which are not enforced locally at all
(because the constraint is on a remote object). The discussion
concluded that this difference is reasonable, because the WITH CHECK
OPTION is a constraint on the local view (not any remote object);
therefore it only makes sense to enforce its WITH CHECK OPTION
constraint locally.

Author: Etsuro Fujita
Reviewed-by: Arthur Zakirov, Stephen Frost
Discussion: https://www.postgresql.org/message-id/7eb58fab-fd3b-781b-ac33-f7cfec96021f%40lab.ntt.co.jp

1 files changed, 30 insertions, 11 deletions

diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 8068e28184c..6001f4d25ef 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -140,6 +140,7 @@ static void deparseSubqueryTargetList(deparse_expr_cxt *context);
 static void deparseReturningList(StringInfo buf, RangeTblEntry *rte,
 					 Index rtindex, Relation rel,
 					 bool trig_after_row,
+					 List *withCheckOptionList,
 					 List *returningList,
 					 List **retrieved_attrs);
 static void deparseColumnRef(StringInfo buf, int varno, int varattno,
@@ -1645,14 +1646,15 @@ deparseRangeTblRef(StringInfo buf, PlannerInfo *root, RelOptInfo *foreignrel,
  * deparse remote INSERT statement
  *
  * The statement text is appended to buf, and we also create an integer List
- * of the columns being retrieved by RETURNING (if any), which is returned
- * to *retrieved_attrs.
+ * of the columns being retrieved by WITH CHECK OPTION or RETURNING (if any),
+ * which is returned to *retrieved_attrs.
  */
 void
 deparseInsertSql(StringInfo buf, RangeTblEntry *rte,
 				 Index rtindex, Relation rel,
 				 List *targetAttrs, bool doNothing,
-				 List *returningList, List **retrieved_attrs)
+				 List *withCheckOptionList, List *returningList,
+				 List **retrieved_attrs)
 {
 	AttrNumber	pindex;
 	bool		first;
@@ -1701,20 +1703,21 @@ deparseInsertSql(StringInfo buf, RangeTblEntry *rte,
 
 	deparseReturningList(buf, rte, rtindex, rel,
 						 rel->trigdesc && rel->trigdesc->trig_insert_after_row,
-						 returningList, retrieved_attrs);
+						 withCheckOptionList, returningList, retrieved_attrs);
 }
 
 /*
  * deparse remote UPDATE statement
  *
  * The statement text is appended to buf, and we also create an integer List
- * of the columns being retrieved by RETURNING (if any), which is returned
- * to *retrieved_attrs.
+ * of the columns being retrieved by WITH CHECK OPTION or RETURNING (if any),
+ * which is returned to *retrieved_attrs.
  */
 void
 deparseUpdateSql(StringInfo buf, RangeTblEntry *rte,
 				 Index rtindex, Relation rel,
-				 List *targetAttrs, List *returningList,
+				 List *targetAttrs,
+				 List *withCheckOptionList, List *returningList,
 				 List **retrieved_attrs)
 {
 	AttrNumber	pindex;
@@ -1743,7 +1746,7 @@ deparseUpdateSql(StringInfo buf, RangeTblEntry *rte,
 
 	deparseReturningList(buf, rte, rtindex, rel,
 						 rel->trigdesc && rel->trigdesc->trig_update_after_row,
-						 returningList, retrieved_attrs);
+						 withCheckOptionList, returningList, retrieved_attrs);
 }
 
 /*
@@ -1837,7 +1840,7 @@ deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
 								  &context);
 	else
 		deparseReturningList(buf, rte, rtindex, rel, false,
-							 returningList, retrieved_attrs);
+							 NIL, returningList, retrieved_attrs);
 }
 
 /*
@@ -1859,7 +1862,7 @@ deparseDeleteSql(StringInfo buf, RangeTblEntry *rte,
 
 	deparseReturningList(buf, rte, rtindex, rel,
 						 rel->trigdesc && rel->trigdesc->trig_delete_after_row,
-						 returningList, retrieved_attrs);
+						 NIL, returningList, retrieved_attrs);
 }
 
 /*
@@ -1921,7 +1924,7 @@ deparseDirectDeleteSql(StringInfo buf, PlannerInfo *root,
 	else
 		deparseReturningList(buf, planner_rt_fetch(rtindex, root),
 							 rtindex, rel, false,
-							 returningList, retrieved_attrs);
+							 NIL, returningList, retrieved_attrs);
 }
 
 /*
@@ -1931,6 +1934,7 @@ static void
 deparseReturningList(StringInfo buf, RangeTblEntry *rte,
 					 Index rtindex, Relation rel,
 					 bool trig_after_row,
+					 List *withCheckOptionList,
 					 List *returningList,
 					 List **retrieved_attrs)
 {
@@ -1943,6 +1947,21 @@ deparseReturningList(StringInfo buf, RangeTblEntry *rte,
 			bms_make_singleton(0 - FirstLowInvalidHeapAttributeNumber);
 	}
 
+	if (withCheckOptionList != NIL)
+	{
+		/*
+		 * We need the attrs, non-system and system, mentioned in the local
+		 * query's WITH CHECK OPTION list.
+		 *
+		 * Note: we do this to ensure that WCO constraints will be evaluated
+		 * on the data actually inserted/updated on the remote side, which
+		 * might differ from the data supplied by the core code, for example
+		 * as a result of remote triggers.
+		 */
+		pull_varattnos((Node *) withCheckOptionList, rtindex,
+					   &attrs_used);
+	}
+
 	if (returningList != NIL)
 	{
 		/*