(2018/01/17 22:00), Stephen Frost wrote:
Reviewing this thread, I tend to agree with Etsuro and I'm not sure I
see where there's a good argument for having a foreign table under a
view behave differently than a local table under a view for WCO (which
is an option of the view- not about the table underneath it or if it's
local or remote). I've not done a detailed review of the patch but it
seems pretty reasonable and pretty small.
Thanks for the comments!
I noticed the patch doesn't apply. Attached is a rebased patch.
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***************
*** 138,143 **** static void deparseSubqueryTargetList(deparse_expr_cxt *context);
--- 138,144 ----
static void deparseReturningList(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
bool trig_after_row,
+ List *withCheckOptionList,
List *returningList,
List **retrieved_attrs);
static void deparseColumnRef(StringInfo buf, int varno, int varattno,
***************
*** 1548,1554 **** void
deparseInsertSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
List *targetAttrs, bool doNothing,
! List *returningList, List **retrieved_attrs)
{
AttrNumber pindex;
bool first;
--- 1549,1556 ----
deparseInsertSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
List *targetAttrs, bool doNothing,
! List *withCheckOptionList, List *returningList,
! List **retrieved_attrs)
{
AttrNumber pindex;
bool first;
***************
*** 1597,1603 **** deparseInsertSql(StringInfo buf, PlannerInfo *root,
deparseReturningList(buf, root, rtindex, rel,
rel->trigdesc && rel->trigdesc->trig_insert_after_row,
! returningList, retrieved_attrs);
}
/*
--- 1599,1605 ----
deparseReturningList(buf, root, rtindex, rel,
rel->trigdesc && rel->trigdesc->trig_insert_after_row,
! withCheckOptionList, returningList, retrieved_attrs);
}
/*
***************
*** 1610,1616 **** deparseInsertSql(StringInfo buf, PlannerInfo *root,
void
deparseUpdateSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
! List *targetAttrs, List *returningList,
List **retrieved_attrs)
{
AttrNumber pindex;
--- 1612,1619 ----
void
deparseUpdateSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
! List *targetAttrs,
! List *withCheckOptionList, List *returningList,
List **retrieved_attrs)
{
AttrNumber pindex;
***************
*** 1639,1645 **** deparseUpdateSql(StringInfo buf, PlannerInfo *root,
deparseReturningList(buf, root, rtindex, rel,
rel->trigdesc && rel->trigdesc->trig_update_after_row,
! returningList, retrieved_attrs);
}
/*
--- 1642,1648 ----
deparseReturningList(buf, root, rtindex, rel,
rel->trigdesc && rel->trigdesc->trig_update_after_row,
! withCheckOptionList, returningList, retrieved_attrs);
}
/*
***************
*** 1707,1713 **** deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
}
deparseReturningList(buf, root, rtindex, rel, false,
! returningList, retrieved_attrs);
}
/*
--- 1710,1716 ----
}
deparseReturningList(buf, root, rtindex, rel, false,
! NIL, returningList, retrieved_attrs);
}
/*
***************
*** 1729,1735 **** deparseDeleteSql(StringInfo buf, PlannerInfo *root,
deparseReturningList(buf, root, rtindex, rel,
rel->trigdesc && rel->trigdesc->trig_delete_after_row,
! returningList, retrieved_attrs);
}
/*
--- 1732,1738 ----
deparseReturningList(buf, root, rtindex, rel,
rel->trigdesc && rel->trigdesc->trig_delete_after_row,
! NIL, returningList, retrieved_attrs);
}
/*
***************
*** 1767,1773 **** deparseDirectDeleteSql(StringInfo buf, PlannerInfo *root,
}
deparseReturningList(buf, root, rtindex, rel, false,
! returningList, retrieved_attrs);
}
/*
--- 1770,1776 ----
}
deparseReturningList(buf, root, rtindex, rel, false,
! NIL, returningList, retrieved_attrs);
}
/*
***************
*** 1777,1782 **** static void
--- 1780,1786 ----
deparseReturningList(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
bool trig_after_row,
+ List *withCheckOptionList,
List *returningList,
List **retrieved_attrs)
{
***************
*** 1789,1794 **** deparseReturningList(StringInfo buf, PlannerInfo *root,
--- 1793,1808 ----
bms_make_singleton(0 - FirstLowInvalidHeapAttributeNumber);
}
+ if (withCheckOptionList != NIL)
+ {
+ /*
+ * We need the attrs, non-system and system, mentioned in the local
+ * query's WITH CHECK OPTIONS list.
+ */
+ pull_varattnos((Node *) withCheckOptionList, rtindex,
+ &attrs_used);
+ }
+
if (returningList != NIL)
{
/*
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 6006,6013 **** ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c2negative;
-- test WITH CHECK OPTION constraints
-- ===================================================================
CREATE TABLE base_tbl (a int, b int);
CREATE FOREIGN TABLE foreign_tbl (a int, b int)
! SERVER loopback OPTIONS(table_name 'base_tbl');
CREATE VIEW rw_view AS SELECT * FROM foreign_tbl
WHERE a < b WITH CHECK OPTION;
\d+ rw_view
--- 6006,6015 ----
-- test WITH CHECK OPTION constraints
-- ===================================================================
CREATE TABLE base_tbl (a int, b int);
+ CREATE FUNCTION row_before_insupd_trigfunc() RETURNS trigger AS $$BEGIN NEW.a := NEW.a + 10; RETURN NEW; END$$ LANGUAGE plpgsql;
+ CREATE TRIGGER row_before_insupd_trigger BEFORE INSERT OR UPDATE ON base_tbl FOR EACH ROW EXECUTE PROCEDURE row_before_insupd_trigfunc();
CREATE FOREIGN TABLE foreign_tbl (a int, b int)
! SERVER loopback OPTIONS (table_name 'base_tbl');
CREATE VIEW rw_view AS SELECT * FROM foreign_tbl
WHERE a < b WITH CHECK OPTION;
\d+ rw_view
***************
*** 6023,6066 **** View definition:
WHERE foreign_tbl.a < foreign_tbl.b;
Options: check_option=cascaded
! INSERT INTO rw_view VALUES (0, 10); -- ok
! INSERT INTO rw_view VALUES (10, 0); -- should fail
ERROR: new row violates check option for view "rw_view"
! DETAIL: Failing row contains (10, 0).
EXPLAIN (VERBOSE, COSTS OFF)
! UPDATE rw_view SET b = 20 WHERE a = 0; -- not pushed down
! QUERY PLAN
! --------------------------------------------------------------------------------------------------
Update on public.foreign_tbl
! Remote SQL: UPDATE public.base_tbl SET b = $2 WHERE ctid = $1
-> Foreign Scan on public.foreign_tbl
! Output: foreign_tbl.a, 20, foreign_tbl.ctid
! Remote SQL: SELECT a, ctid FROM public.base_tbl WHERE ((a < b)) AND ((a = 0)) FOR UPDATE
(5 rows)
! UPDATE rw_view SET b = 20 WHERE a = 0; -- ok
EXPLAIN (VERBOSE, COSTS OFF)
! UPDATE rw_view SET b = -20 WHERE a = 0; -- not pushed down
! QUERY PLAN
! --------------------------------------------------------------------------------------------------
Update on public.foreign_tbl
! Remote SQL: UPDATE public.base_tbl SET b = $2 WHERE ctid = $1
-> Foreign Scan on public.foreign_tbl
! Output: foreign_tbl.a, '-20'::integer, foreign_tbl.ctid
! Remote SQL: SELECT a, ctid FROM public.base_tbl WHERE ((a < b)) AND ((a = 0)) FOR UPDATE
(5 rows)
! UPDATE rw_view SET b = -20 WHERE a = 0; -- should fail
! ERROR: new row violates check option for view "rw_view"
! DETAIL: Failing row contains (0, -20).
SELECT * FROM foreign_tbl;
! a | b
! ---+----
! 0 | 20
(1 row)
DROP FOREIGN TABLE foreign_tbl CASCADE;
NOTICE: drop cascades to view rw_view
DROP TABLE base_tbl;
-- ===================================================================
-- test serial columns (ie, sequence-based defaults)
--- 6025,6096 ----
WHERE foreign_tbl.a < foreign_tbl.b;
Options: check_option=cascaded
! EXPLAIN (VERBOSE, COSTS OFF)
! INSERT INTO rw_view VALUES (0, 5);
! QUERY PLAN
! --------------------------------------------------------------------------------
! Insert on public.foreign_tbl
! Remote SQL: INSERT INTO public.base_tbl(a, b) VALUES ($1, $2) RETURNING a, b
! -> Result
! Output: 0, 5
! (4 rows)
!
! INSERT INTO rw_view VALUES (0, 5); -- should fail
ERROR: new row violates check option for view "rw_view"
! DETAIL: Failing row contains (10, 5).
EXPLAIN (VERBOSE, COSTS OFF)
! INSERT INTO rw_view VALUES (0, 15);
! QUERY PLAN
! --------------------------------------------------------------------------------
! Insert on public.foreign_tbl
! Remote SQL: INSERT INTO public.base_tbl(a, b) VALUES ($1, $2) RETURNING a, b
! -> Result
! Output: 0, 15
! (4 rows)
!
! INSERT INTO rw_view VALUES (0, 15); -- ok
! SELECT * FROM foreign_tbl;
! a | b
! ----+----
! 10 | 15
! (1 row)
!
! EXPLAIN (VERBOSE, COSTS OFF)
! UPDATE rw_view SET b = b + 5;
! QUERY PLAN
! ---------------------------------------------------------------------------------------
Update on public.foreign_tbl
! Remote SQL: UPDATE public.base_tbl SET b = $2 WHERE ctid = $1 RETURNING a, b
-> Foreign Scan on public.foreign_tbl
! Output: foreign_tbl.a, (foreign_tbl.b + 5), foreign_tbl.ctid
! Remote SQL: SELECT a, b, ctid FROM public.base_tbl WHERE ((a < b)) FOR UPDATE
(5 rows)
! UPDATE rw_view SET b = b + 5; -- should fail
! ERROR: new row violates check option for view "rw_view"
! DETAIL: Failing row contains (20, 20).
EXPLAIN (VERBOSE, COSTS OFF)
! UPDATE rw_view SET b = b + 15;
! QUERY PLAN
! ---------------------------------------------------------------------------------------
Update on public.foreign_tbl
! Remote SQL: UPDATE public.base_tbl SET b = $2 WHERE ctid = $1 RETURNING a, b
-> Foreign Scan on public.foreign_tbl
! Output: foreign_tbl.a, (foreign_tbl.b + 15), foreign_tbl.ctid
! Remote SQL: SELECT a, b, ctid FROM public.base_tbl WHERE ((a < b)) FOR UPDATE
(5 rows)
! UPDATE rw_view SET b = b + 15; -- ok
SELECT * FROM foreign_tbl;
! a | b
! ----+----
! 20 | 30
(1 row)
DROP FOREIGN TABLE foreign_tbl CASCADE;
NOTICE: drop cascades to view rw_view
+ DROP TRIGGER row_before_insupd_trigger ON base_tbl;
+ DROP FUNCTION row_before_insupd_trigfunc;
DROP TABLE base_tbl;
-- ===================================================================
-- test serial columns (ie, sequence-based defaults)
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***************
*** 1549,1554 **** postgresPlanForeignModify(PlannerInfo *root,
--- 1549,1555 ----
Relation rel;
StringInfoData sql;
List *targetAttrs = NIL;
+ List *withCheckOptionList = NIL;
List *returningList = NIL;
List *retrieved_attrs = NIL;
bool doNothing = false;
***************
*** 1598,1603 **** postgresPlanForeignModify(PlannerInfo *root,
--- 1599,1611 ----
}
/*
+ * Extract the relevant WITH CHECK OPTIONS list if any.
+ */
+ if (plan->withCheckOptionLists)
+ withCheckOptionList = (List *) list_nth(plan->withCheckOptionLists,
+ subplan_index);
+
+ /*
* Extract the relevant RETURNING list if any.
*/
if (plan->returningLists)
***************
*** 1622,1633 **** postgresPlanForeignModify(PlannerInfo *root,
{
case CMD_INSERT:
deparseInsertSql(&sql, root, resultRelation, rel,
! targetAttrs, doNothing, returningList,
&retrieved_attrs);
break;
case CMD_UPDATE:
deparseUpdateSql(&sql, root, resultRelation, rel,
! targetAttrs, returningList,
&retrieved_attrs);
break;
case CMD_DELETE:
--- 1630,1643 ----
{
case CMD_INSERT:
deparseInsertSql(&sql, root, resultRelation, rel,
! targetAttrs, doNothing,
! withCheckOptionList, returningList,
&retrieved_attrs);
break;
case CMD_UPDATE:
deparseUpdateSql(&sql, root, resultRelation, rel,
! targetAttrs,
! withCheckOptionList, returningList,
&retrieved_attrs);
break;
case CMD_DELETE:
*** a/contrib/postgres_fdw/postgres_fdw.h
--- b/contrib/postgres_fdw/postgres_fdw.h
***************
*** 142,152 **** extern bool is_foreign_expr(PlannerInfo *root,
Expr *expr);
extern void deparseInsertSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
! List *targetAttrs, bool doNothing, List *returningList,
List **retrieved_attrs);
extern void deparseUpdateSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
! List *targetAttrs, List *returningList,
List **retrieved_attrs);
extern void deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
--- 142,154 ----
Expr *expr);
extern void deparseInsertSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
! List *targetAttrs, bool doNothing,
! List *withCheckOptionList, List *returningList,
List **retrieved_attrs);
extern void deparseUpdateSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
! List *targetAttrs,
! List *withCheckOptionList, List *returningList,
List **retrieved_attrs);
extern void deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***************
*** 1198,1220 **** ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c2negative;
-- ===================================================================
CREATE TABLE base_tbl (a int, b int);
CREATE FOREIGN TABLE foreign_tbl (a int, b int)
! SERVER loopback OPTIONS(table_name 'base_tbl');
CREATE VIEW rw_view AS SELECT * FROM foreign_tbl
WHERE a < b WITH CHECK OPTION;
\d+ rw_view
- INSERT INTO rw_view VALUES (0, 10); -- ok
- INSERT INTO rw_view VALUES (10, 0); -- should fail
EXPLAIN (VERBOSE, COSTS OFF)
! UPDATE rw_view SET b = 20 WHERE a = 0; -- not pushed down
! UPDATE rw_view SET b = 20 WHERE a = 0; -- ok
EXPLAIN (VERBOSE, COSTS OFF)
! UPDATE rw_view SET b = -20 WHERE a = 0; -- not pushed down
! UPDATE rw_view SET b = -20 WHERE a = 0; -- should fail
SELECT * FROM foreign_tbl;
DROP FOREIGN TABLE foreign_tbl CASCADE;
DROP TABLE base_tbl;
-- ===================================================================
--- 1198,1230 ----
-- ===================================================================
CREATE TABLE base_tbl (a int, b int);
+ CREATE FUNCTION row_before_insupd_trigfunc() RETURNS trigger AS $$BEGIN NEW.a := NEW.a + 10; RETURN NEW; END$$ LANGUAGE plpgsql;
+ CREATE TRIGGER row_before_insupd_trigger BEFORE INSERT OR UPDATE ON base_tbl FOR EACH ROW EXECUTE PROCEDURE row_before_insupd_trigfunc();
CREATE FOREIGN TABLE foreign_tbl (a int, b int)
! SERVER loopback OPTIONS (table_name 'base_tbl');
CREATE VIEW rw_view AS SELECT * FROM foreign_tbl
WHERE a < b WITH CHECK OPTION;
\d+ rw_view
EXPLAIN (VERBOSE, COSTS OFF)
! INSERT INTO rw_view VALUES (0, 5);
! INSERT INTO rw_view VALUES (0, 5); -- should fail
EXPLAIN (VERBOSE, COSTS OFF)
! INSERT INTO rw_view VALUES (0, 15);
! INSERT INTO rw_view VALUES (0, 15); -- ok
! SELECT * FROM foreign_tbl;
!
! EXPLAIN (VERBOSE, COSTS OFF)
! UPDATE rw_view SET b = b + 5;
! UPDATE rw_view SET b = b + 5; -- should fail
! EXPLAIN (VERBOSE, COSTS OFF)
! UPDATE rw_view SET b = b + 15;
! UPDATE rw_view SET b = b + 15; -- ok
SELECT * FROM foreign_tbl;
DROP FOREIGN TABLE foreign_tbl CASCADE;
+ DROP TRIGGER row_before_insupd_trigger ON base_tbl;
+ DROP FUNCTION row_before_insupd_trigfunc;
DROP TABLE base_tbl;
-- ===================================================================
*** a/doc/src/sgml/fdwhandler.sgml
--- b/doc/src/sgml/fdwhandler.sgml
***************
*** 568,579 **** ExecForeignInsert(EState *estate,
<para>
The data in the returned slot is used only if the <command>INSERT</command>
! query has a <literal>RETURNING</literal> clause or the foreign table has
! an <literal>AFTER ROW</literal> trigger. Triggers require all columns, but the
! FDW could choose to optimize away returning some or all columns depending
! on the contents of the <literal>RETURNING</literal> clause. Regardless, some
! slot must be returned to indicate success, or the query's reported row
! count will be wrong.
</para>
<para>
--- 568,580 ----
<para>
The data in the returned slot is used only if the <command>INSERT</command>
! query has a <literal>WITH CHECK OPTION</literal> or <literal>RETURNING</literal>
! clause or the foreign table has an <literal>AFTER ROW</literal> trigger.
! Triggers require all columns, but the FDW could choose to optimize away
! returning some or all columns depending on the contents of the
! <literal>WITH CHECK OPTION</literal> or <literal>RETURNING</literal> clause.
! Regardless, some slot must be returned to indicate success, or the query's
! reported row count will be wrong.
</para>
<para>
***************
*** 614,625 **** ExecForeignUpdate(EState *estate,
<para>
The data in the returned slot is used only if the <command>UPDATE</command>
! query has a <literal>RETURNING</literal> clause or the foreign table has
! an <literal>AFTER ROW</literal> trigger. Triggers require all columns, but the
! FDW could choose to optimize away returning some or all columns depending
! on the contents of the <literal>RETURNING</literal> clause. Regardless, some
! slot must be returned to indicate success, or the query's reported row
! count will be wrong.
</para>
<para>
--- 615,627 ----
<para>
The data in the returned slot is used only if the <command>UPDATE</command>
! query has a <literal>WITH CHECK OPTION</literal> or <literal>RETURNING</literal>
! clause or the foreign table has an <literal>AFTER ROW</literal> trigger.
! Triggers require all columns, but the FDW could choose to optimize away
! returning some or all columns depending on the contents of the
! <literal>WITH CHECK OPTION</literal> or <literal>RETURNING</literal> clause.
! Regardless, some slot must be returned to indicate success, or the query's
! reported row count will be wrong.
</para>
<para>
