Hi,
Commit 7086be6e3627c1ad797e32ebbdd232905b5f577f addressed mishandling of
WCO in direct foreign table modification by disabling it when we have
WCO, but I noticed another oddity in postgres_fdw:
postgres=# create table base_tbl (a int, b int);
postgres=# create function row_before_insupd_trigfunc() returns trigger
as $$begin new.a := new.a + 10; return new; end$$ language plpgsql;
postgres=# create trigger row_before_insupd_trigger before insert or
update on base_tbl for each row execute procedure
row_before_insupd_trigfunc();
postgres=# create server loopback foreign data wrapper postgres_fdw
options (dbname 'postgres');
postgres=# create user mapping for CURRENT_USER server loopback;
postgres=# create foreign table foreign_tbl (a int, b int) server
loopback options (table_name 'base_tbl');
postgres=# create view rw_view as select * from foreign_tbl where a < b
with check option;
So, this should fail, but
postgres=# insert into rw_view values (0, 5);
INSERT 0 1
The reason for that is: this is processed using postgres_fdw's
non-direct foreign table modification (ie. ForeignModify), but unlike
the RETURNING or local after trigger case, the ForeignModify doesn't
take care that remote triggers might change the data in that case, so
the WCO is evaluated using the data supplied, not the data actually
inserted, which I think is wrong. (I should have noticed that as well
while working on the fix, though.) So, I'd propose to fix that by
modifying postgresPlanForeignModify so that it handles WCO the same way
as for the RETURNING case. Attached is a patch for that. I'll add the
patch to the next commitfest.
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
***************
*** 5897,5904 **** 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
--- 5897,5906 ----
-- 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
***************
*** 5914,5957 **** 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)
--- 5916,5987 ----
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
***************
*** 1185,1207 **** 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;
-- ===================================================================
--- 1185,1217 ----
-- ===================================================================
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
***************
*** 565,576 **** ExecForeignInsert(EState *estate,
<para>
The data in the returned slot is used only if the <command>INSERT</>
! query has a <literal>RETURNING</> clause or the foreign table has
! an <literal>AFTER ROW</> 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</> clause. Regardless, some
! slot must be returned to indicate success, or the query's reported row
! count will be wrong.
</para>
<para>
--- 565,577 ----
<para>
The data in the returned slot is used only if the <command>INSERT</>
! query has a <literal>WITH CHECK OPTION</> or <literal>RETURNING</>
! clause or the foreign table has an <literal>AFTER ROW</> 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</> or <literal>RETURNING</> clause.
! Regardless, some slot must be returned to indicate success, or the
! query's reported row count will be wrong.
</para>
<para>
***************
*** 611,622 **** ExecForeignUpdate(EState *estate,
<para>
The data in the returned slot is used only if the <command>UPDATE</>
! query has a <literal>RETURNING</> clause or the foreign table has
! an <literal>AFTER ROW</> 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</> clause. Regardless, some
! slot must be returned to indicate success, or the query's reported row
! count will be wrong.
</para>
<para>
--- 612,624 ----
<para>
The data in the returned slot is used only if the <command>UPDATE</>
! query has a <literal>WITH CHECK OPTION</> or <literal>RETURNING</>
! clause or the foreign table has an <literal>AFTER ROW</> 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</> or <literal>RETURNING</> clause.
! Regardless, some slot must be returned to indicate success, or the
! query's reported row count will be wrong.
</para>
<para>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
