Hi Arthur,
(2018/03/03 18:51), Arthur Zakirov wrote:
On Wed, Feb 28, 2018 at 05:22:42PM +0900, Etsuro Fujita wrote:
I rebased the patch over HEAD and revised comments/docs a little bit. Please
find attached a new version of the patch.
I've reviewed the patch.
The code is good, clear and it is pretty small. There are documentation
fixes and additional regression tests.
Unfortunately the patch is outdated and it needs rebasing. Outdated
files are regression tests files.
After rebasing regression tests they pass.
I rebased the patch over HEAD. Please find attached an updated patch.
Thank you for the review!
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***************
*** 140,145 **** static void deparseSubqueryTargetList(deparse_expr_cxt *context);
--- 140,146 ----
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,
***************
*** 1645,1658 **** 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.
*/
void
deparseInsertSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
List *targetAttrs, bool doNothing,
! List *returningList, List **retrieved_attrs)
{
AttrNumber pindex;
bool first;
--- 1646,1660 ----
* deparse remote INSERT statement
*
* The statement text is appended to buf, and we also create an integer List
! * of the columns being retrieved by WITH CHECK OPTION or RETURNING (if any),
! * which is returned to *retrieved_attrs.
*/
void
deparseInsertSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
List *targetAttrs, bool doNothing,
! List *withCheckOptionList, List *returningList,
! List **retrieved_attrs)
{
AttrNumber pindex;
bool first;
***************
*** 1701,1720 **** deparseInsertSql(StringInfo buf, PlannerInfo *root,
deparseReturningList(buf, root, rtindex, rel,
rel->trigdesc && rel->trigdesc->trig_insert_after_row,
! 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.
*/
void
deparseUpdateSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
! List *targetAttrs, List *returningList,
List **retrieved_attrs)
{
AttrNumber pindex;
--- 1703,1723 ----
deparseReturningList(buf, root, rtindex, rel,
rel->trigdesc && rel->trigdesc->trig_insert_after_row,
! 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 WITH CHECK OPTION or RETURNING (if any),
! * which is returned to *retrieved_attrs.
*/
void
deparseUpdateSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
! List *targetAttrs,
! List *withCheckOptionList, List *returningList,
List **retrieved_attrs)
{
AttrNumber pindex;
***************
*** 1743,1749 **** deparseUpdateSql(StringInfo buf, PlannerInfo *root,
deparseReturningList(buf, root, rtindex, rel,
rel->trigdesc && rel->trigdesc->trig_update_after_row,
! returningList, retrieved_attrs);
}
/*
--- 1746,1752 ----
deparseReturningList(buf, root, rtindex, rel,
rel->trigdesc && rel->trigdesc->trig_update_after_row,
! withCheckOptionList, returningList, retrieved_attrs);
}
/*
***************
*** 1836,1842 **** deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
&context);
else
deparseReturningList(buf, root, rtindex, rel, false,
! returningList, retrieved_attrs);
}
/*
--- 1839,1845 ----
&context);
else
deparseReturningList(buf, root, rtindex, rel, false,
! NIL, returningList, retrieved_attrs);
}
/*
***************
*** 1858,1864 **** deparseDeleteSql(StringInfo buf, PlannerInfo *root,
deparseReturningList(buf, root, rtindex, rel,
rel->trigdesc && rel->trigdesc->trig_delete_after_row,
! returningList, retrieved_attrs);
}
/*
--- 1861,1867 ----
deparseReturningList(buf, root, rtindex, rel,
rel->trigdesc && rel->trigdesc->trig_delete_after_row,
! NIL, returningList, retrieved_attrs);
}
/*
***************
*** 1919,1925 **** deparseDirectDeleteSql(StringInfo buf, PlannerInfo *root,
&context);
else
deparseReturningList(buf, root, rtindex, rel, false,
! returningList, retrieved_attrs);
}
/*
--- 1922,1928 ----
&context);
else
deparseReturningList(buf, root, rtindex, rel, false,
! NIL, returningList, retrieved_attrs);
}
/*
***************
*** 1929,1934 **** static void
--- 1932,1938 ----
deparseReturningList(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
bool trig_after_row,
+ List *withCheckOptionList,
List *returningList,
List **retrieved_attrs)
{
***************
*** 1941,1946 **** deparseReturningList(StringInfo buf, PlannerInfo *root,
--- 1945,1960 ----
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.
+ */
+ 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
***************
*** 6175,6182 **** ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c2negative;
-- ===================================================================
CREATE TABLE base_tbl (a int, b int);
ALTER TABLE base_tbl SET (autovacuum_enabled = 'false');
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
--- 6175,6184 ----
-- ===================================================================
CREATE TABLE base_tbl (a int, b int);
ALTER TABLE base_tbl SET (autovacuum_enabled = 'false');
+ 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
***************
*** 6192,6235 **** 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)
--- 6194,6265 ----
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
***************
*** 1563,1568 **** postgresPlanForeignModify(PlannerInfo *root,
--- 1563,1569 ----
Relation rel;
StringInfoData sql;
List *targetAttrs = NIL;
+ List *withCheckOptionList = NIL;
List *returningList = NIL;
List *retrieved_attrs = NIL;
bool doNothing = false;
***************
*** 1612,1617 **** postgresPlanForeignModify(PlannerInfo *root,
--- 1613,1625 ----
}
/*
+ * Extract the relevant WITH CHECK OPTION list if any.
+ */
+ if (plan->withCheckOptionLists)
+ withCheckOptionList = (List *) list_nth(plan->withCheckOptionLists,
+ subplan_index);
+
+ /*
* Extract the relevant RETURNING list if any.
*/
if (plan->returningLists)
***************
*** 1636,1647 **** 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:
--- 1644,1657 ----
{
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
***************
*** 1264,1286 **** ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c2negative;
CREATE TABLE base_tbl (a int, b int);
ALTER TABLE base_tbl SET (autovacuum_enabled = 'false');
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;
-- ===================================================================
--- 1264,1296 ----
CREATE TABLE base_tbl (a int, b int);
ALTER TABLE base_tbl SET (autovacuum_enabled = 'false');
+ 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 <literal>WITH CHECK OPTION</literal> constraints or
! 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>WITH CHECK OPTION</literal> constraints 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 <literal>WITH CHECK OPTION</literal> constraints or
! 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>WITH CHECK OPTION</literal> constraints 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>
