Another oddity in handling of WCO constraints in postgres_fdw
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
Attachments:
fix-wco-handling-in-postgres-fdw.patchtext/plain; charset=UTF-8; name=fix-wco-handling-in-postgres-fdw.patchDownload
*** 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>
On Thu, Sep 28, 2017 at 5:45 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
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 1The 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.
Enforcing WCO constraints imposed by the local server on the row/DML
being passed to the foreign server is fine, but trying to impose them
on the row being inserted/updated at the foreign server looks odd. May
be we should just leave this case as it is. I am comparing this case
with the way we handle constraints on a foreign table.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/10/03 18:16, Ashutosh Bapat wrote:
Enforcing WCO constraints imposed by the local server on the row/DML
being passed to the foreign server is fine, but trying to impose them
on the row being inserted/updated at the foreign server looks odd. May
be we should just leave this case as it is. I am comparing this case
with the way we handle constraints on a foreign table.
Hmm, I think that would be okay in the case where WCO constraints match
constraints on the foreign table, but I'm not sure that would be okay
even in the case where WCO constraints don't match? Consider:
create table bt (a int check (a % 2 = 0));
create foreign table ft (a int check (a % 2 = 0)) server loopback
options (table_name 'bt');
create view rw_view_2 as select * from ft where a % 2 = 0 with check option;
In that case the WCO constraint matches the constraint on the foreign
table, so there would be no need to ensure the WCO constraint locally
(to make the explanation simple, we assume here that we don't have
triggers on the remote end). BUT: for another auto-updatable view
defined using the same foreign table like this:
create view rw_view_4 as select * from ft where a % 4 = 0 with check option;
how is the WCO constraint (ie, a % 4 = 0) ensured remotely, which is
different from the constraint on the foreign table (ie, a % 2 = 0)?
Maybe I'm missing something, though.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Oct 4, 2017 at 3:45 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
On 2017/10/03 18:16, Ashutosh Bapat wrote:
Enforcing WCO constraints imposed by the local server on the row/DML
being passed to the foreign server is fine, but trying to impose them
on the row being inserted/updated at the foreign server looks odd. May
be we should just leave this case as it is. I am comparing this case
with the way we handle constraints on a foreign table.Hmm, I think that would be okay in the case where WCO constraints match
constraints on the foreign table, but I'm not sure that would be okay even
in the case where WCO constraints don't match? Consider:create table bt (a int check (a % 2 = 0));
create foreign table ft (a int check (a % 2 = 0)) server loopback options
(table_name 'bt');
create view rw_view_2 as select * from ft where a % 2 = 0 with check option;In that case the WCO constraint matches the constraint on the foreign table,
so there would be no need to ensure the WCO constraint locally (to make the
explanation simple, we assume here that we don't have triggers on the remote
end). BUT: for another auto-updatable view defined using the same foreign
table like this:create view rw_view_4 as select * from ft where a % 4 = 0 with check option;
how is the WCO constraint (ie, a % 4 = 0) ensured remotely, which is
different from the constraint on the foreign table (ie, a % 2 = 0)? Maybe
I'm missing something, though.
Just like the local constraints on a foreign table are not ensured on
remote table (unless user takes steps to make that sure), WCO defined
locally need not be (and probably can not be) ensured remotely. We can
check whether a row being sent from the local server to the foreign
server obeys WCO, but what foreign server does to that row is beyond
local server's scope.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Oct 4, 2017 at 6:40 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
Just like the local constraints on a foreign table are not ensured on
remote table (unless user takes steps to make that sure), WCO defined
locally need not be (and probably can not be) ensured remotely. We can
check whether a row being sent from the local server to the foreign
server obeys WCO, but what foreign server does to that row is beyond
local server's scope.
But I think right now we're not checking the row being sent from the
local server, either. The WCO that is being ignored isn't a
constraint on the foreign table; it's a constraint on a view which
happens to reference the foreign table. It seems quite odd for the
"assume constraints are valid" property of the foreign table to
propagate back up into the view that references it.
--
Robert Haas
EnterpriseDB: 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
On Wed, Oct 4, 2017 at 5:32 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Oct 4, 2017 at 6:40 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:Just like the local constraints on a foreign table are not ensured on
remote table (unless user takes steps to make that sure), WCO defined
locally need not be (and probably can not be) ensured remotely. We can
check whether a row being sent from the local server to the foreign
server obeys WCO, but what foreign server does to that row is beyond
local server's scope.But I think right now we're not checking the row being sent from the
local server, either.
Didn't 7086be6e3627c1ad797e32ebbdd232905b5f577f fix that?
The WCO that is being ignored isn't a
constraint on the foreign table; it's a constraint on a view which
happens to reference the foreign table. It seems quite odd for the
"assume constraints are valid" property of the foreign table to
propagate back up into the view that references it.
The view with WCO is local but the modification which violates WCO is
being made on remote server by a trigger on remote table. Trying to
control that doesn't seem to be a good idea, just like we can't
control what rows get inserted on the foreign server when they violate
local constraints. I am using local constraints as an example of
precedence where we ignore what's happening on remote side and enforce
whatever we could enforce locally. Local server should make sure that
any rows sent from local server to the remote server do not violate
any local WCO. But once it's handed over to the foreign server, we
shouldn't worry about what happens there. That behaviour is ensured by
the above commit, isn't it? I am not suggesting that we use local
constraints to enforce WCO or something like that.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/10/04 21:28, Ashutosh Bapat wrote:
On Wed, Oct 4, 2017 at 5:32 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Oct 4, 2017 at 6:40 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:We can
check whether a row being sent from the local server to the foreign
server obeys WCO, but what foreign server does to that row is beyond
local server's scope.But I think right now we're not checking the row being sent from the
local server, either.
We don't check the row *before* sending it to the remote server, but
check the row returned by ExecForeignInsert/ExecForeignUpdate, which is
allowed to have been changed by the remote server. In postgres_fdw, we
currently return the data actually inserted/updated if RETURNING/AFTER
TRIGGER present, but not if WCO only presents. So, for the postgres_fdw
foreign table, WCO is enforced on the data that was actually
inserted/updated if RETURNING/AFTER TRIGGER present and on the original
data core supplied if WCO only presents, which is inconsistent behavior.
Didn't 7086be6e3627c1ad797e32ebbdd232905b5f577f fix that?
No. The commit addressed another issue.
The WCO that is being ignored isn't a
constraint on the foreign table; it's a constraint on a view which
happens to reference the foreign table. It seems quite odd for the
"assume constraints are valid" property of the foreign table to
propagate back up into the view that references it.
Agreed.
The view with WCO is local but the modification which violates WCO is
being made on remote server by a trigger on remote table. Trying to
control that doesn't seem to be a good idea, just like we can't
control what rows get inserted on the foreign server when they violate
local constraints. I am using local constraints as an example of
precedence where we ignore what's happening on remote side and enforce
whatever we could enforce locally. Local server should make sure that
any rows sent from local server to the remote server do not violate
any local WCO.
Seems odd (and too restrictive) to me too.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
At Thu, 5 Oct 2017 18:08:50 +0900, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote in <60e94494-4e5d-afed-e482-b9ad1986bbf6@lab.ntt.co.jp>
On 2017/10/04 21:28, Ashutosh Bapat wrote:
On Wed, Oct 4, 2017 at 5:32 PM, Robert Haas <robertmhaas@gmail.com>
wrote:On Wed, Oct 4, 2017 at 6:40 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:We can
check whether a row being sent from the local server to the foreign
server obeys WCO, but what foreign server does to that row is beyond
local server's scope.But I think right now we're not checking the row being sent from the
local server, either.We don't check the row *before* sending it to the remote server, but
check the row returned by ExecForeignInsert/ExecForeignUpdate, which
is allowed to have been changed by the remote server. In
postgres_fdw, we currently return the data actually inserted/updated
if RETURNING/AFTER TRIGGER present, but not if WCO only presents. So,
for the postgres_fdw foreign table, WCO is enforced on the data that
was actually inserted/updated if RETURNING/AFTER TRIGGER present and
on the original data core supplied if WCO only presents, which is
inconsistent behavior.Didn't 7086be6e3627c1ad797e32ebbdd232905b5f577f fix that?
No. The commit addressed another issue.
The WCO that is being ignored isn't a
constraint on the foreign table; it's a constraint on a view which
happens to reference the foreign table. It seems quite odd for the
"assume constraints are valid" property of the foreign table to
propagate back up into the view that references it.Agreed.
The view with WCO is local but the modification which violates WCO is
being made on remote server by a trigger on remote table. Trying to
control that doesn't seem to be a good idea, just like we can't
control what rows get inserted on the foreign server when they violate
local constraints. I am using local constraints as an example of
precedence where we ignore what's happening on remote side and enforce
whatever we could enforce locally. Local server should make sure that
any rows sent from local server to the remote server do not violate
any local WCO.Seems odd (and too restrictive) to me too.
Since WCO ensures finally inserted values, we can't do other than
acturally requesting for the values. So just merging WCO columns
to RETURNING in deparsed query is ok. But can't we concatenate
returningList and withCheckOptionList at more higher level?
Specifically, just passing calculated used_attr to
deparse(Insert|Update)Sql instead of returningList and
withCheckOptionList separately. Deparsed queries anyway forget
the origin of requested columns.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2017/10/05 20:06, Kyotaro HORIGUCHI wrote:
Since WCO ensures finally inserted values, we can't do other than
acturally requesting for the values.
I think so too.
So just merging WCO columns
to RETURNING in deparsed query is ok. But can't we concatenate
returningList and withCheckOptionList at more higher level?
Specifically, just passing calculated used_attr to
deparse(Insert|Update)Sql instead of returningList and
withCheckOptionList separately. Deparsed queries anyway forget
the origin of requested columns.
We could do that, but I think that would need a bit more code to
postgresPlanForeignModify including changes to the deparseDeleteSql API
in addition to the deparse(Insert|Update)Sql APIs. I prefer making high
level functions simple, so I'd vote for just passing withCheckOptionList
separately to deparse(Insert|Update)Sql, as proposed in the patch.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Oct 4, 2017 at 5:58 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
The view with WCO is local but the modification which violates WCO is
being made on remote server by a trigger on remote table. Trying to
control that doesn't seem to be a good idea, just like we can't
control what rows get inserted on the foreign server when they violate
local constraints.
I think that's a fair point.
--
Robert Haas
EnterpriseDB: 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
(2017/11/01 11:16), Robert Haas wrote:
On Wed, Oct 4, 2017 at 5:58 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:The view with WCO is local but the modification which violates WCO is
being made on remote server by a trigger on remote table. Trying to
control that doesn't seem to be a good idea, just like we can't
control what rows get inserted on the foreign server when they violate
local constraints.I think that's a fair point.
For local constraints on foreign tables, it's the user's responsibility
to ensure that those constraints matches the remote side, so we don't
need to ensure those constraints locally. But I'm not sure if the same
thing applies to WCOs on views defined on foreign tables, because in
some case it's not possible to impose constraints on the remote side
that match those WCOs, as I explained before.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Nov 10, 2017 at 8:36 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
For local constraints on foreign tables, it's the user's responsibility to
ensure that those constraints matches the remote side, so we don't need to
ensure those constraints locally. But I'm not sure if the same thing
applies to WCOs on views defined on foreign tables, because in some case
it's not possible to impose constraints on the remote side that match those
WCOs, as I explained before.
Moved to CF 2018-01.
--
Michael
Greetings Etsuro, Robert, all,
* Etsuro Fujita (fujita.etsuro@lab.ntt.co.jp) wrote:
(2017/11/01 11:16), Robert Haas wrote:
On Wed, Oct 4, 2017 at 5:58 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:The view with WCO is local but the modification which violates WCO is
being made on remote server by a trigger on remote table. Trying to
control that doesn't seem to be a good idea, just like we can't
control what rows get inserted on the foreign server when they violate
local constraints.I think that's a fair point.
For local constraints on foreign tables, it's the user's responsibility to
ensure that those constraints matches the remote side, so we don't need to
ensure those constraints locally. But I'm not sure if the same thing
applies to WCOs on views defined on foreign tables, because in some case
it's not possible to impose constraints on the remote side that match those
WCOs, as I explained before.
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!
Stephen
(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
Attachments:
fix-wco-handling-in-postgres-fdw-v2.patchtext/x-diff; name=fix-wco-handling-in-postgres-fdw-v2.patchDownload
*** 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>
(2018/01/18 16:16), Etsuro Fujita wrote:
Attached is a rebased patch.
I rebased the patch over HEAD and revised comments/docs a little bit.
Please find attached a new version of the patch.
Best regards,
Etsuro Fujita
Attachments:
fix-wco-handling-in-postgres-fdw-v3.patchtext/x-diff; name=fix-wco-handling-in-postgres-fdw-v3.patchDownload
*** 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;
-- 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
--- 6175,6184 ----
-- 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
***************
*** 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);
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);
+ 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>
Hello,
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.
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
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
Attachments:
fix-wco-handling-in-postgres-fdw-v4.patchtext/x-diff; name=fix-wco-handling-in-postgres-fdw-v4.patchDownload
*** 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>
On Mon, Mar 05, 2018 at 09:44:37PM +0900, Etsuro Fujita wrote:
I rebased the patch over HEAD. Please find attached an updated patch.
Thank you!
IMHO, it is worth to add more explaining comment into
deparseReturningList, why it is necessary to merge WCO attributes to
RETURNING clause. You already noted it in the thread. I think it could
confuse someone who not very familiar how RETURNING is related with WITH
CHECK OPTION.
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
(2018/03/06 1:57), Arthur Zakirov wrote:
IMHO, it is worth to add more explaining comment into
deparseReturningList, why it is necessary to merge WCO attributes to
RETURNING clause. You already noted it in the thread. I think it could
confuse someone who not very familiar how RETURNING is related with WITH
CHECK OPTION.
Agreed. I added a comment to that function. I think that that comment
in combination with changes to the FDW docs in the patch would help FDW
authors understand why that is needed. Please find attached an updated
version of the patch.
Thanks for the comments!
Best regards,
Etsuro Fujita
Attachments:
fix-wco-handling-in-postgres-fdw-v5.patchtext/x-diff; name=fix-wco-handling-in-postgres-fdw-v5.patchDownload
*** 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,1965 ----
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)
{
/*
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***************
*** 6206,6213 **** 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
--- 6206,6215 ----
-- ===================================================================
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
***************
*** 6223,6266 **** 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)
--- 6225,6296 ----
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
***************
*** 1277,1299 **** 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;
-- ===================================================================
--- 1277,1309 ----
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>
On Tue, Mar 06, 2018 at 08:09:50PM +0900, Etsuro Fujita wrote:
Agreed. I added a comment to that function. I think that that comment in
combination with changes to the FDW docs in the patch would help FDW authors
understand why that is needed. Please find attached an updated version of
the patch.
Thank you.
All tests pass, the documentation builds. There was the suggestion [1]
of different approach. But the patch fix the issue in much more simple
way.
Marked as "Ready for Commiter".
1 - /messages/by-id/20171005.200631.134118679.horiguchi.kyotaro@lab.ntt.co.jp
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Greetings Robert, Ashutosh, Arthur, Etsuro, all,
* Arthur Zakirov (a.zakirov@postgrespro.ru) wrote:
On Tue, Mar 06, 2018 at 08:09:50PM +0900, Etsuro Fujita wrote:
Agreed. I added a comment to that function. I think that that comment in
combination with changes to the FDW docs in the patch would help FDW authors
understand why that is needed. Please find attached an updated version of
the patch.Thank you.
All tests pass, the documentation builds. There was the suggestion [1]
of different approach. But the patch fix the issue in much more simple
way.Marked as "Ready for Commiter".
1 - /messages/by-id/20171005.200631.134118679.horiguchi.kyotaro@lab.ntt.co.jp
Thanks, I've looked through this patch and thread again and continue to
feel that this is both a good and sensible improvment and that the patch
is in pretty good shape.
The remaining question is if the subsequent discussion has swayed the
opinion of Robert and Ashutosh. If we can get agreement that these
semantics are acceptable and an improvement over the status quo then I'm
happy to try and drive this patch to commit.
Robert, Ashutosh?
Thanks!
Stephen
On Wed, Mar 7, 2018 at 8:55 AM, Stephen Frost <sfrost@snowman.net> wrote:
Greetings Robert, Ashutosh, Arthur, Etsuro, all,
* Arthur Zakirov (a.zakirov@postgrespro.ru) wrote:
On Tue, Mar 06, 2018 at 08:09:50PM +0900, Etsuro Fujita wrote:
Agreed. I added a comment to that function. I think that that comment in
combination with changes to the FDW docs in the patch would help FDW authors
understand why that is needed. Please find attached an updated version of
the patch.Thank you.
All tests pass, the documentation builds. There was the suggestion [1]
of different approach. But the patch fix the issue in much more simple
way.Marked as "Ready for Commiter".
1 - /messages/by-id/20171005.200631.134118679.horiguchi.kyotaro@lab.ntt.co.jp
Thanks, I've looked through this patch and thread again and continue to
feel that this is both a good and sensible improvment and that the patch
is in pretty good shape.The remaining question is if the subsequent discussion has swayed the
opinion of Robert and Ashutosh. If we can get agreement that these
semantics are acceptable and an improvement over the status quo then I'm
happy to try and drive this patch to commit.Robert, Ashutosh?
If there is a local constraint on the foreign table, we don't check
it. So, a row that was inserted through this foreign table may not
show up when selected from the foreign table. Apply same logic to the
WCO on a view on the foreign table, it should be fine if a row
inserted through the view doesn't show up in the view. Somebody who
created the view, knew that it's a foreign table underneath.
Stephen said [1]/messages/by-id/20180117130021.GC2416@tamriel.snowman.net that the view is local and irrespective of what's
underneath it, it should obey WCO. Which seems to be a fair point when
considered alone, but with the above context, it doesn't look any
fair.
Etsuro said [2]/messages/by-id/5A058F21.2040201@lab.ntt.co.jp that WCO constraints can not be implemented on foreign
server and normal check constraints can be, and for that he provides
an example in [3]/messages/by-id/a3955a1d-ad07-5b0a-7618-b6ef5ff0e1c5@lab.ntt.co.jp. But I think that example is going the wrong
direction. For local constraints to be enforced, we use remote
constraints. For local WCO we need to use remote WCO. That means we
create many foreign tables pointing to same local table on the foreign
server through many views, but it's not impossible.
[1]: /messages/by-id/20180117130021.GC2416@tamriel.snowman.net
[2]: /messages/by-id/5A058F21.2040201@lab.ntt.co.jp
[3]: /messages/by-id/a3955a1d-ad07-5b0a-7618-b6ef5ff0e1c5@lab.ntt.co.jp
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Hi Ashutosh,
(2018/03/08 14:24), Ashutosh Bapat wrote:
Etsuro said [2] that WCO constraints can not be implemented on foreign
server and normal check constraints can be, and for that he provides
an example in [3]. But I think that example is going the wrong
direction.
More precisely, what I'm saying there is: for WCO constraints created by
an auto-updatable view over a foreign table, we cannot always implement
constraints on the remote side that match with those WCO constraints.
For local constraints to be enforced, we use remote
constraints. For local WCO we need to use remote WCO. That means we
create many foreign tables pointing to same local table on the foreign
server through many views, but it's not impossible.
Maybe I don't understand this correctly, but I guess that it would be
the user's responsibility to not create foreign tables in such a way.
Best regards,
Etsuro Fujita
(2018/03/09 20:55), Etsuro Fujita wrote:
(2018/03/08 14:24), Ashutosh Bapat wrote:
For local constraints to be enforced, we use remote
constraints. For local WCO we need to use remote WCO. That means we
create many foreign tables pointing to same local table on the foreign
server through many views, but it's not impossible.Maybe I don't understand this correctly, but I guess that it would be
the user's responsibility to not create foreign tables in such a way.
I think I misunderstood your words. Sorry for that. I think what you
proposed would be a solution for this issue, but I'm not sure that's a
good one because that wouldn't work for the data sources that don't
support views with WCO options.
Best regards,
Etsuro Fujita
On Mon, Mar 12, 2018 at 1:25 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
(2018/03/09 20:55), Etsuro Fujita wrote:
(2018/03/08 14:24), Ashutosh Bapat wrote:
For local constraints to be enforced, we use remote
constraints. For local WCO we need to use remote WCO. That means we
create many foreign tables pointing to same local table on the foreign
server through many views, but it's not impossible.Maybe I don't understand this correctly, but I guess that it would be
the user's responsibility to not create foreign tables in such a way.I think I misunderstood your words. Sorry for that. I think what you
proposed would be a solution for this issue, but I'm not sure that's a good
one because that wouldn't work for the data sources that don't support views
with WCO options.
Our solution for the constraints doesn't work with the data sources
(like flat files) which don't support constraints. So, that argument
doesn't help.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Greetings,
* Ashutosh Bapat (ashutosh.bapat@enterprisedb.com) wrote:
On Mon, Mar 12, 2018 at 1:25 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:(2018/03/09 20:55), Etsuro Fujita wrote:
(2018/03/08 14:24), Ashutosh Bapat wrote:
For local constraints to be enforced, we use remote
constraints. For local WCO we need to use remote WCO. That means we
create many foreign tables pointing to same local table on the foreign
server through many views, but it's not impossible.Maybe I don't understand this correctly, but I guess that it would be
the user's responsibility to not create foreign tables in such a way.I think I misunderstood your words. Sorry for that. I think what you
proposed would be a solution for this issue, but I'm not sure that's a good
one because that wouldn't work for the data sources that don't support views
with WCO options.Our solution for the constraints doesn't work with the data sources
(like flat files) which don't support constraints. So, that argument
doesn't help.
It would really help to have some examples of exactly what is being
proposed here wrt solutions.
WCO is defined at a view level, so I'm not following the notion that
we're going to depend on something remote to enforce the WCO when the
remote object is just a regular table that you can't define a WCO on top
of. That's not the case when we're talking about foreign tables vs.
local tables, so it's not the same. I certainly don't think we should
require a remote view to exist to perform the WCO check. If the remote
WCO is a view itself then I would expect it to operate in the same
manner as WCO on local views does- you can have them defined as being
cascaded or not.
In other words, there is no case where we have a "foreign view." Views
are always local. A "foreign table" could actually be a view, in which
case everything we treat it as a table in the local database, but WCO
doesn't come up in that case at all- there's no way to define WCO on a
table, foreign or not. If WCO is defined on the view on the remote
server, then it should operate properly and not require anything from the
local side.
Thanks!
Stephen
On Tue, Mar 13, 2018 at 8:34 PM, Stephen Frost <sfrost@snowman.net> wrote:
It would really help to have some examples of exactly what is being
proposed here wrt solutions.WCO is defined at a view level, so I'm not following the notion that
we're going to depend on something remote to enforce the WCO when the
remote object is just a regular table that you can't define a WCO on top
of. That's not the case when we're talking about foreign tables vs.
local tables, so it's not the same. I certainly don't think we should
require a remote view to exist to perform the WCO check. If the remote
WCO is a view itself then I would expect it to operate in the same
manner as WCO on local views does- you can have them defined as being
cascaded or not.In other words, there is no case where we have a "foreign view." Views
are always local. A "foreign table" could actually be a view, in which
case everything we treat it as a table in the local database, but WCO
doesn't come up in that case at all- there's no way to define WCO on a
table, foreign or not. If WCO is defined on the view on the remote
server, then it should operate properly and not require anything from the
local side.
I agree with this analysis. I have no objection about the patch anymore.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
On Tue, 2018-03-06 at 20:09 +0900, Etsuro Fujita wrote:
Agreed. I added a comment to that function. I think that that
comment
in combination with changes to the FDW docs in the patch would help
FDW
authors understand why that is needed. Please find attached an
updated
version of the patch.Thanks for the comments!
Committed.
I made some small modifications and added a test for the case where the
foreign table is a partition of a local table, which follows a
different code path after commit 3d956d95.
Thank you!
Regards,
Jeff Davis
(2018/07/09 9:00), Jeff Davis wrote:
Committed.
I made some small modifications and added a test for the case where the
foreign table is a partition of a local table, which follows a
different code path after commit 3d956d95.
Great! Thanks for revising and committing, Jeff. Thanks for reviewing,
Arthur and Stephen. Thanks for commenting on this, Ashutosh and Robert.
Best regards,
Etsuro Fujita