Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw

[Etsuro Fujita]

(2017/11/01 11:16), Robert Haas wrote:

On Wed, Oct 4, 2017 at 5:58 PM, Ashutosh Bapat
  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

Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw

[Robert Haas]

On Wed, Oct 4, 2017 at 5:58 PM, Ashutosh Bapat
 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

Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw

[Etsuro Fujita]

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

Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw

[Kyotaro HORIGUCHI]

At Thu, 5 Oct 2017 18:08:50 +0900, Etsuro Fujita  
wrote in <60e94494-4e5d-afed-e482-b9ad1986b...@lab.ntt.co.jp>
> On 2017/10/04 21:28, Ashutosh Bapat wrote:
> > On Wed, Oct 4, 2017 at 5:32 PM, Robert Haas 
> > wrote:
> >> On Wed, Oct 4, 2017 at 6:40 AM, Ashutosh Bapat
> >>  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

Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw

[Etsuro Fujita]

On 2017/10/04 21:28, Ashutosh Bapat wrote:

On Wed, Oct 4, 2017 at 5:32 PM, Robert Haas  wrote:

On Wed, Oct 4, 2017 at 6:40 AM, Ashutosh Bapat
 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

Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw

[Ashutosh Bapat]

On Wed, Oct 4, 2017 at 5:32 PM, Robert Haas  wrote:
> On Wed, Oct 4, 2017 at 6:40 AM, Ashutosh Bapat
>  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

Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw

[Robert Haas]

On Wed, Oct 4, 2017 at 6:40 AM, Ashutosh Bapat
 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

Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw

[Ashutosh Bapat]

On Wed, Oct 4, 2017 at 3:45 PM, Etsuro Fujita
 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

Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw

[Etsuro Fujita]

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

Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw

[Ashutosh Bapat]

On Thu, Sep 28, 2017 at 5:45 PM, Etsuro Fujita
 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 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.
>

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

[HACKERS] Another oddity in handling of WCO constraints in postgres_fdw

[Etsuro Fujita]

Hi,

Commit 7086be6e3627c1ad797e32ebbdd232905b5f577f addressed mishandling of 
WCO in direct foreign table modification by disabling it when we have 
WCO, but I noticed another oddity in postgres_fdw:


postgres=# create table base_tbl (a int, b int);
postgres=# create function row_before_insupd_trigfunc() returns trigger 
as $$begin new.a := new.a + 10; return new; end$$ language plpgsql;
postgres=# create trigger row_before_insupd_trigger before insert or 
update on base_tbl for each row execute procedure 
row_before_insupd_trigfunc();
postgres=# create server loopback foreign data wrapper postgres_fdw 
options (dbname 'postgres');

postgres=# create user mapping for CURRENT_USER server loopback;
postgres=# create foreign table foreign_tbl (a int, b int) server 
loopback options (table_name 'base_tbl');
postgres=# create view rw_view as select * from foreign_tbl where a < b 
with check option;


So, this should fail, but

postgres=# insert into rw_view values (0, 5);
INSERT 0 1

The reason for that is: this is processed using postgres_fdw's 
non-direct foreign table modification (ie. ForeignModify), but unlike 
the RETURNING or local after trigger case, the ForeignModify doesn't 
take care that remote triggers might change the data in that case, so 
the WCO is evaluated using the data supplied, not the data actually 
inserted, which I think is wrong.  (I should have noticed that as well 
while working on the fix, though.)  So, I'd propose to fix that by 
modifying postgresPlanForeignModify so that it handles WCO the same way 
as for the RETURNING case.  Attached is a patch for that.  I'll add the 
patch to the next commitfest.


Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 138,143  static void deparseSubqueryTargetList(deparse_expr_cxt 
*context);
--- 138,144 
  static void deparseReturningList(StringInfo buf, PlannerInfo *root,
 Index rtindex, Relation rel,
 bool trig_after_row,
+List *withCheckOptionList,
 List *returningList,
 List **retrieved_attrs);
  static void deparseColumnRef(StringInfo buf, int varno, int varattno,
***
*** 1548,1554  void
  deparseInsertSql(StringInfo buf, PlannerInfo *root,
 Index rtindex, Relation rel,
 List *targetAttrs, bool doNothing,
!List *returningList, List **retrieved_attrs)
  {
AttrNumber  pindex;
boolfirst;
--- 1549,1556 
  deparseInsertSql(StringInfo buf, PlannerInfo *root,
 Index rtindex, Relation rel,
 List *targetAttrs, bool doNothing,
!List *withCheckOptionList, List *returningList,
!List **retrieved_attrs)
  {
AttrNumber  pindex;
boolfirst;
***
*** 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 &&