Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Robert Haas]

On Mon, Jul 24, 2017 at 6:21 AM, Amit Langote
 wrote:
> Yes, we need that there too.
>
> Done that in the attached v3 (including the test where
> ExecPartitionCheck() would have crashed without the patch).

Committed.  Thanks to all of you.

-- 
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] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Amit Langote]

On 2017/07/24 17:30, Etsuro Fujita wrote:
> On 2017/07/24 16:16, Amit Khandekar wrote:
>> On 24 July 2017 at 12:11, Amit Langote 
>> wrote:
>>> Attached is the updated version of your patch.
> 
> Good catch, Amit K. and Amit L.!
> 
>> Now that this is done, any particular reason it is not done in
>> ExecPartitionCheck() ? I see that there is a do_convert_tuple() in
>> that function, again without changing the slot descriptor.
> 
> Yeah, I think we would need that in ExecPartitionCheck() as well.

Yes, we need that there too.

Done that in the attached v3 (including the test where
ExecPartitionCheck() would have crashed without the patch).

Thanks,
Amit
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index b22de78516..78cbcd1a32 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1879,6 +1879,7 @@ ExecPartitionCheck(ResultRelInfo *resultRelInfo, 
TupleTableSlot *slot,
if (map != NULL)
{
tuple = do_convert_tuple(tuple, map);
+   ExecSetSlotDescriptor(slot, tupdesc);
ExecStoreTuple(tuple, slot, InvalidBuffer, 
false);
}
}
@@ -1956,6 +1957,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
if (map != NULL)
{
tuple = do_convert_tuple(tuple, 
map);
+   ExecSetSlotDescriptor(slot, 
tupdesc);
ExecStoreTuple(tuple, slot, 
InvalidBuffer, false);
}
}
@@ -2003,6 +2005,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
if (map != NULL)
{
tuple = do_convert_tuple(tuple, map);
+   ExecSetSlotDescriptor(slot, tupdesc);
ExecStoreTuple(tuple, slot, 
InvalidBuffer, false);
}
}
@@ -2112,6 +2115,7 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo 
*resultRelInfo,
if (map != NULL)
{
tuple = 
do_convert_tuple(tuple, map);
+   
ExecSetSlotDescriptor(slot, tupdesc);
ExecStoreTuple(tuple, 
slot, InvalidBuffer, false);
}
}
diff --git a/src/test/regress/expected/insert.out 
b/src/test/regress/expected/insert.out
index c608ce377f..0dcc86fef4 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -410,6 +410,21 @@ with ins (a, b, c) as
  mlparted4  | 1 |  30 |  39
 (5 rows)
 
+alter table mlparted add c text;
+create table mlparted5 (c text, a int not null, b int not null) partition by 
list (c);
+create table mlparted5a (a int not null, c text, b int not null);
+alter table mlparted5 attach partition mlparted5a for values in ('a');
+alter table mlparted attach partition mlparted5 for values from (1, 40) to (1, 
50);
+alter table mlparted add constraint check_b check (a = 1 and b < 45);
+insert into mlparted values (1, 45, 'a');
+ERROR:  new row for relation "mlparted5a" violates check constraint "check_b"
+DETAIL:  Failing row contains (1, 45, a).
+create function mlparted5abrtrig_func() returns trigger as $$ begin new.c = 
'b'; return new; end; $$ language plpgsql;
+create trigger mlparted5abrtrig before insert on mlparted5a for each row 
execute procedure mlparted5abrtrig_func();
+insert into mlparted5 (a, b, c) values (1, 40, 'a');
+ERROR:  new row for relation "mlparted5a" violates partition constraint
+DETAIL:  Failing row contains (b, 1, 40).
+drop table mlparted5;
 -- check that message shown after failure to find a partition shows the
 -- appropriate key description (or none) in various situations
 create table key_desc (a int, b int) partition by list ((a+0));
diff --git a/src/test/regress/expected/updatable_views.out 
b/src/test/regress/expected/updatable_views.out
index caca81a70b..eab5c0334c 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -2368,8 +2368,8 @@ DETAIL:  Failing row contains (-1, invalid).
 DROP VIEW v1;
 DROP TABLE t1;
 -- check that an auto-updatable view on a partitioned table works correctly
-create table pt (a int, b int) partition by range (a, b);
-create table pt1 (b int not null, a int not null) partition by range (b);
+create table pt (a int, b int, v varchar) 

Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Etsuro Fujita]

On 2017/07/24 16:16, Amit Khandekar wrote:

On 24 July 2017 at 12:11, Amit Langote  wrote:

Attached is the updated version of your patch.


Good catch, Amit K. and Amit L.!


Now that this is done, any particular reason it is not done in
ExecPartitionCheck() ? I see that there is a do_convert_tuple() in
that function, again without changing the slot descriptor.


Yeah, I think we would need that in ExecPartitionCheck() as well.

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] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Amit Khandekar]

On 24 July 2017 at 12:11, Amit Langote  wrote:
> Hi Amit,
>
> On 2017/07/24 14:09, Amit Khandekar wrote:
 On 2017/07/10 14:15, Etsuro Fujita wrote:
 Another thing I noticed is the error handling in ExecWithCheckOptions; it
 doesn't take any care for partition tables, so the column description in
 the error message for WCO_VIEW_CHECK is built using the partition table's
 rowtype, not the root table's.  But I think it'd be better to build that
 using the root table's rowtype, like ExecConstraints, because that would
 make the column description easier to understand since the parent view(s)
 (from which WithCheckOptions evaluated there are created) would have been
 defined on the root table.  This seems independent from the above issue,
 so I created a separate patch, which I'm attaching. What do you think
 about that?
>>
>> + if (map != NULL)
>> + {
>> + tuple = do_convert_tuple(tuple, map);
>> + ExecStoreTuple(tuple, slot, InvalidBuffer, false);
>> + }
>>
>> Above, the tuple descriptor also needs to be set, since the parent and
>> child partitions can have different column ordering.
>>
>> Due to this, the following testcase crashes :
>>
>> CREATE TABLE range_parted (a text,b int, c int) partition by range (b);
>> CREATE VIEW upview AS SELECT * FROM range_parted WHERE (select c >
>> 120) WITH CHECK OPTION;
>> create table part_a_1_a_10(b int, c int, a text);
>> alter table range_parted attach partition part_a_1_a_10 for values
>> from (1) to (10);
>> insert into upview values ('a', 2, 100);
>
> Good catch.  Thanks for creating the patch.
>
> There are other places with similar code viz. those in ExecConstraints()
> that would need fixing.

Ah ok. I should have noticed that. Thanks.

> Attached is the updated version of your patch.
Now that this is done, any particular reason it is not done in
ExecPartitionCheck() ? I see that there is a do_convert_tuple() in
that function, again without changing the slot descriptor.


-- 
Thanks,
-Amit Khandekar
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] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Amit Langote]

Hi Amit,

On 2017/07/24 14:09, Amit Khandekar wrote:
>>> On 2017/07/10 14:15, Etsuro Fujita wrote:
>>> Another thing I noticed is the error handling in ExecWithCheckOptions; it
>>> doesn't take any care for partition tables, so the column description in
>>> the error message for WCO_VIEW_CHECK is built using the partition table's
>>> rowtype, not the root table's.  But I think it'd be better to build that
>>> using the root table's rowtype, like ExecConstraints, because that would
>>> make the column description easier to understand since the parent view(s)
>>> (from which WithCheckOptions evaluated there are created) would have been
>>> defined on the root table.  This seems independent from the above issue,
>>> so I created a separate patch, which I'm attaching. What do you think
>>> about that?
> 
> + if (map != NULL)
> + {
> + tuple = do_convert_tuple(tuple, map);
> + ExecStoreTuple(tuple, slot, InvalidBuffer, false);
> + }
> 
> Above, the tuple descriptor also needs to be set, since the parent and
> child partitions can have different column ordering.
> 
> Due to this, the following testcase crashes :
> 
> CREATE TABLE range_parted (a text,b int, c int) partition by range (b);
> CREATE VIEW upview AS SELECT * FROM range_parted WHERE (select c >
> 120) WITH CHECK OPTION;
> create table part_a_1_a_10(b int, c int, a text);
> alter table range_parted attach partition part_a_1_a_10 for values
> from (1) to (10);
> insert into upview values ('a', 2, 100);

Good catch.  Thanks for creating the patch.

There are other places with similar code viz. those in ExecConstraints()
that would need fixing.  Without the same, the following causes a crash
(building on your example):

alter table range_parted add constraint check_c check (c > 120);
insert into range_parted values ('a', 2, 100);
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

Attached is the updated version of your patch.  A test is also added in
insert.sql on lines of the above example.

Will add this to the PG 10 open items list.

Thanks,
Amit
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index b22de78516..040e9a916a 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1956,6 +1956,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
if (map != NULL)
{
tuple = do_convert_tuple(tuple, 
map);
+   ExecSetSlotDescriptor(slot, 
tupdesc);
ExecStoreTuple(tuple, slot, 
InvalidBuffer, false);
}
}
@@ -2003,6 +2004,7 @@ ExecConstraints(ResultRelInfo *resultRelInfo,
if (map != NULL)
{
tuple = do_convert_tuple(tuple, map);
+   ExecSetSlotDescriptor(slot, tupdesc);
ExecStoreTuple(tuple, slot, 
InvalidBuffer, false);
}
}
@@ -2112,6 +2114,7 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo 
*resultRelInfo,
if (map != NULL)
{
tuple = 
do_convert_tuple(tuple, map);
+   
ExecSetSlotDescriptor(slot, tupdesc);
ExecStoreTuple(tuple, 
slot, InvalidBuffer, false);
}
}
diff --git a/src/test/regress/expected/insert.out 
b/src/test/regress/expected/insert.out
index c608ce377f..0eaa47fb2b 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -410,6 +410,14 @@ with ins (a, b, c) as
  mlparted4  | 1 |  30 |  39
 (5 rows)
 
+alter table mlparted add c text;
+create table mlparted5 (c text, a int not null, b int not null);
+alter table mlparted attach partition mlparted5 for values from (1, 40) to (1, 
50);
+alter table mlparted add constraint check_b check (a = 1 and b < 45);
+insert into mlparted values (1, 45, 'bah');
+ERROR:  new row for relation "mlparted5" violates check constraint "check_b"
+DETAIL:  Failing row contains (1, 45, bah).
+drop table mlparted5;
 -- check that message shown after failure to find a partition shows the
 -- appropriate key description (or none) in various situations
 create table key_desc (a int, b int) partition by list ((a+0));
diff --git a/src/test/regress/expected/updatable_views.out 

Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Amit Khandekar]

>> On 2017/07/10 14:15, Etsuro Fujita wrote:
>> Another thing I noticed is the error handling in ExecWithCheckOptions; it
>> doesn't take any care for partition tables, so the column description in
>> the error message for WCO_VIEW_CHECK is built using the partition table's
>> rowtype, not the root table's.  But I think it'd be better to build that
>> using the root table's rowtype, like ExecConstraints, because that would
>> make the column description easier to understand since the parent view(s)
>> (from which WithCheckOptions evaluated there are created) would have been
>> defined on the root table.  This seems independent from the above issue,
>> so I created a separate patch, which I'm attaching. What do you think
>> about that?

+ if (map != NULL)
+ {
+ tuple = do_convert_tuple(tuple, map);
+ ExecStoreTuple(tuple, slot, InvalidBuffer, false);
+ }

Above, the tuple descriptor also needs to be set, since the parent and
child partitions can have different column ordering.

Due to this, the following testcase crashes :

CREATE TABLE range_parted (a text,b int, c int) partition by range (b);
CREATE VIEW upview AS SELECT * FROM range_parted WHERE (select c >
120) WITH CHECK OPTION;
create table part_a_1_a_10(b int, c int, a text);
alter table range_parted attach partition part_a_1_a_10 for values
from (1) to (10);
insert into upview values ('a', 2, 100);

Attached is a patch that sets the tuple descriptor.
Also in the patch, in test updatable_view.sql, I have added a varchar
column in one of the partitioned tables used in updatable_views.sql,
so as to cover this scenario. Without setting the tuple descriptor,
the output of the patched updatable_views.sql  shows junk value in one
of the columns of the row in the error message emitted for the
WithCheckOption violation.

Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company


set_slot_descriptor.patch
Description: Binary data

-- 
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] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Amit Langote]

Fujita-san,

On 2017/07/10 14:15, Etsuro Fujita wrote:
> On 2017/07/07 18:47, Amit Langote wrote:
>> On 2017/07/06 16:06, Etsuro Fujita wrote:
>>> I think this should be fixed.  Attached is a patch for that.
> 
>> How about setting ri_RangeTableIndex of the partition ResultRelInfo
>> correctly in the first place?  If you look at the way InitResultRelInfo()
>> is called in ExecSetupPartitionTupleRouting(), a dummy RT index of 1 is
>> passed.  We could instead pass the correct one.  I think
>> ModifyTable.nominalRelation is that (at least in the INSERT case.
>>
>> The attached patch implements that.  It modifies
>> ExecSetupPartitionTupleRouting to accept one more argument resultRTindex
>> and passes along the same to InitResultRelInfo().  Later when
>> ExecConstraints() wants to build the modifiedCols set, it looks up the
>> correct RTE using the partition ResultRelInfo, which has the appropriate
>> ri_RangeTableIndex set and uses the same.
> 
> Looks good to me.

Thanks for the review.

>> The patch keeps tests that you added in your patch.  Added this to the
>> open items list.
> 
> Another thing I noticed is the error handling in ExecWithCheckOptions; it
> doesn't take any care for partition tables, so the column description in
> the error message for WCO_VIEW_CHECK is built using the partition table's
> rowtype, not the root table's.  But I think it'd be better to build that
> using the root table's rowtype, like ExecConstraints, because that would
> make the column description easier to understand since the parent view(s)
> (from which WithCheckOptions evaluated there are created) would have been
> defined on the root table.  This seems independent from the above issue,
> so I created a separate patch, which I'm attaching. What do you think
> about that?

Good catch.  The patch looks spot on to me.  To keep both the patches
together, I'm attaching them here as 0001 which fixes the original issue
you reported on this thread and 0002 which is your patch to fix error
reporting in ExecWithCheckOptions.

Thanks,
Amit
From f9f4cc93d962ff433fe98b36a84953ba4048a725 Mon Sep 17 00:00:00 2001
From: amit 
Date: Fri, 7 Jul 2017 17:24:44 +0900
Subject: [PATCH 1/2] Properly set ri_RangeTableIndex of partition result rels

Previously it was set to a "dummy" value of 1, which would cause
certain code, such as ExecConstraint()'s error handling code, to
look at the unintended range table entry.  Instead set it to the
index of the RT entry corresponding to root partitioned table.
---
 src/backend/commands/copy.c|  1 +
 src/backend/executor/execMain.c|  3 ++-
 src/backend/executor/nodeModifyTable.c |  1 +
 src/include/executor/executor.h|  1 +
 src/test/regress/expected/insert.out   | 18 ++
 src/test/regress/sql/insert.sql| 18 ++
 6 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f391828e74..51693791cc 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1433,6 +1433,7 @@ BeginCopy(ParseState *pstate,
num_partitions;
 
ExecSetupPartitionTupleRouting(rel,
+   
   1,

   _dispatch_info,

   ,

   _tupconv_maps,
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 0f08283f81..c36b5b7392 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -3213,6 +3213,7 @@ EvalPlanQualEnd(EPQState *epqstate)
  */
 void
 ExecSetupPartitionTupleRouting(Relation rel,
+  Index resultRTindex,
   PartitionDispatch 
**pd,
   ResultRelInfo 
**partitions,
   TupleConversionMap 
***tup_conv_maps,
@@ -3271,7 +3272,7 @@ ExecSetupPartitionTupleRouting(Relation rel,
 
InitResultRelInfo(leaf_part_rri,
  partrel,
- 1,/* dummy */
+ resultRTindex,/* 
dummy */
  rel,
  0);
 
diff --git a/src/backend/executor/nodeModifyTable.c 
b/src/backend/executor/nodeModifyTable.c
index 8d17425abe..77ba15dd90 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1914,6 +1914,7 @@ 

Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Etsuro Fujita]

On 2017/07/07 18:47, Amit Langote wrote:

On 2017/07/06 16:06, Etsuro Fujita wrote:

I think this should be fixed.  Attached is a patch for that.


Looking up the ResultRelInfo using the proposed method in
ExecFindResultRelInfo() can be unreliable sometimes, that is, the method
of using the root table OID to pin down the RangeTableEntry to get the the
modified columns set.


You are right.  Thank you for pointing that out.


How about setting ri_RangeTableIndex of the partition ResultRelInfo
correctly in the first place?  If you look at the way InitResultRelInfo()
is called in ExecSetupPartitionTupleRouting(), a dummy RT index of 1 is
passed.  We could instead pass the correct one.  I think
ModifyTable.nominalRelation is that (at least in the INSERT case.

The attached patch implements that.  It modifies
ExecSetupPartitionTupleRouting to accept one more argument resultRTindex
and passes along the same to InitResultRelInfo().  Later when
ExecConstraints() wants to build the modifiedCols set, it looks up the
correct RTE using the partition ResultRelInfo, which has the appropriate
ri_RangeTableIndex set and uses the same.


Looks good to me.


The patch keeps tests that you added in your patch.  Added this to the
open items list.


Another thing I noticed is the error handling in ExecWithCheckOptions; 
it doesn't take any care for partition tables, so the column description 
in the error message for WCO_VIEW_CHECK is built using the partition 
table's rowtype, not the root table's.  But I think it'd be better to 
build that using the root table's rowtype, like ExecConstraints, because 
that would make the column description easier to understand since the 
parent view(s) (from which WithCheckOptions evaluated there are created) 
would have been defined on the root table.  This seems independent from 
the above issue, so I created a separate patch, which I'm attaching. 
What do you think about that?


Best regards,
Etsuro Fujita
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***
*** 2097,2102  ExecWithCheckOptions(WCOKind kind, ResultRelInfo 
*resultRelInfo,
--- 2097,2121 
 * USING policy.
 */
case WCO_VIEW_CHECK:
+   /* See the comment in 
ExecConstraints(). */
+   if (resultRelInfo->ri_PartitionRoot)
+   {
+   HeapTuple   tuple = 
ExecFetchSlotTuple(slot);
+   TupleDesc   old_tupdesc = 
RelationGetDescr(rel);
+   TupleConversionMap *map;
+ 
+   rel = 
resultRelInfo->ri_PartitionRoot;
+   tupdesc = RelationGetDescr(rel);
+   /* a reverse map */
+   map = 
convert_tuples_by_name(old_tupdesc, tupdesc,
+   
 gettext_noop("could not convert row type"));
+   if (map != NULL)
+   {
+   tuple = 
do_convert_tuple(tuple, map);
+   ExecStoreTuple(tuple, 
slot, InvalidBuffer, false);
+   }
+   }
+ 
insertedCols = 
GetInsertedColumns(resultRelInfo, estate);
updatedCols = 
GetUpdatedColumns(resultRelInfo, estate);
modifiedCols = bms_union(insertedCols, 
updatedCols);
*** a/src/test/regress/expected/updatable_views.out
--- b/src/test/regress/expected/updatable_views.out
***
*** 2424,2429  select tableoid::regclass, * from pt;
  create view ptv_wco as select * from pt where a = 0 with check option;
  insert into ptv_wco values (1, 2);
  ERROR:  new row violates check option for view "ptv_wco"
! DETAIL:  Failing row contains (2, 1).
  drop view ptv, ptv_wco;
  drop table pt, pt1, pt11;
--- 2424,2429 
  create view ptv_wco as select * from pt where a = 0 with check option;
  insert into ptv_wco values (1, 2);
  ERROR:  new row violates check option for view "ptv_wco"
! DETAIL:  Failing row contains (1, 2).
  drop view ptv, ptv_wco;
  drop table pt, pt1, pt11;

-- 
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] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables

[Amit Langote]

Fujita-san,

On 2017/07/06 16:06, Etsuro Fujita wrote:
> Here is an example:
> 
> postgres=# create table col_desc (a int, b int) partition by list (a);
> postgres=# create table col_desc_1 partition of col_desc for values in (1);
> postgres=# alter table col_desc_1 add check (b > 0);
> postgres=# create role col_desc_user;
> postgres=# grant insert on col_desc to col_desc_user;
> postgres=# revoke select on col_desc from col_desc_user;
> postgres=# set role col_desc_user;
> postgres=> insert into col_desc values (1, -1) returning 1;
> ERROR:  new row for relation "col_desc_1" violates check constraint
> "col_desc_1_b_check"
> DETAIL:  Failing row contains (a, b) = (1, -1).
> 
> Looks good, but
> 
> postgres=> reset role;
> postgres=# create table result (f1 text default 'foo', f2 text default
> 'bar', f3 int);
> postgres=# grant insert on result to col_desc_user;
> postgres=# set role col_desc_user;
> postgres=> with t as (insert into col_desc values (1, -1) returning 1)
> insert into result (f3) select * from t;
> ERROR:  new row for relation "col_desc_1" violates check constraint
> "col_desc_1_b_check"
> 
> Looks odd to me because the error message doesn't show any DETAIL info;
> since the CTE query, which produces the message, is the same as the above
> query, the message should also be the same as the one for the above
> query.

I agree that the DETAIL should be shown.

> I think the reason for that is: ExecConstraints looks at an
> incorrect resultRelInfo to build the message for a violating tuple that
> has been routed *in the case where the partitioned table isn't the primary
> ModifyTable node*, which leads to deriving an incorrect modifiedCols and
> then invoking ExecBuildSlotValueDescription with the wrong bitmap.

That's true.  Choosing the appropriate ResultRelInfo will lead us to
looking at the correct RangeTblEntry (via ri_RangeTableIndex) and hence
get the desired modifiedCols bitmap.  But...

> I think this should be fixed.  Attached is a patch for that.

Looking up the ResultRelInfo using the proposed method in
ExecFindResultRelInfo() can be unreliable sometimes, that is, the method
of using the root table OID to pin down the RangeTableEntry to get the the
modified columns set.  If the same table is a target in the main query
with different, the corresponding RTE will appear earlier in the
es_range_table list and will get returned.  For example (slightly altered
version of the example in your email):

alter table col_desc add c int;
set session authorization col_desc_user ;
with t (a) as (insert into col_desc values (1, -1) returning 1)
  insert into col_desc (c) select * from t;
ERROR:  new row for relation "col_desc_1" violates check constraint
"col_desc_1_b_check"
DETAIL:  Failing row contains (c) = (null).

Note that the patched code ends up matching the main query RTE of col_desc
instead of that in the CTE query.  And the columns shown in the DETAIL
part reflects which RTE got used to compute the modifiedCols set.

How about setting ri_RangeTableIndex of the partition ResultRelInfo
correctly in the first place?  If you look at the way InitResultRelInfo()
is called in ExecSetupPartitionTupleRouting(), a dummy RT index of 1 is
passed.  We could instead pass the correct one.  I think
ModifyTable.nominalRelation is that (at least in the INSERT case.

The attached patch implements that.  It modifies
ExecSetupPartitionTupleRouting to accept one more argument resultRTindex
and passes along the same to InitResultRelInfo().  Later when
ExecConstraints() wants to build the modifiedCols set, it looks up the
correct RTE using the partition ResultRelInfo, which has the appropriate
ri_RangeTableIndex set and uses the same.  With the patch:

with t (a) as (insert into col_desc values (1, -1) returning 1)
   insert into col_desc (c) select * from t;
ERROR:  new row for relation "col_desc_1" violates check constraint
"col_desc_1_b_check"
DETAIL:  Failing row contains (a, b) = (1, -1).

The patch keeps tests that you added in your patch.  Added this to the
open items list.

Thoughts?

Thanks,
Amit
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f391828e74..51693791cc 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1433,6 +1433,7 @@ BeginCopy(ParseState *pstate,
num_partitions;
 
ExecSetupPartitionTupleRouting(rel,
+   
   1,

   _dispatch_info,

   ,

   _tupconv_maps,
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 0f08283f81..df9302896c 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ 

Re: [HACKERS] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/08/25 1:08, Robert Haas wrote:

On Tue, Aug 23, 2016 at 11:18 PM, Etsuro Fujita
 wrote:

OK, I think we should fix the issue that postgres_fdw produces incorrect
aliases for joining relations shown in the Relations line in EXPLAIN for a
join pushdown query like the above) in advance of the 9.6 release, so I'll
add this to the 9.6 open items.



Isn't it a bit late for that?

I'm not eager to have 9.6 get further delayed while we work on this
issue, and I definitely don't believe that this is a sufficiently
important issue to justify reverting join pushdown in its entirety.
We're talking about a minor detail of the EXPLAIN output that we'd
like to fix, but for which we have no consensus on exactly how to fix
it, and the fix may involve some redesign.  That does not seem like a
good thing to the week before rc1.  Let's just leave this well enough
alone and work on it for v10.


That's fine with me.

You said upthread, "I don't want to change it at all, neither in 10 or 
any later version."  That was the reason why I proposed to fix this in 9.6.


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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Robert Haas]

On Tue, Aug 23, 2016 at 11:18 PM, Etsuro Fujita
 wrote:
>> Yes, it seems what we are doing now is not consistent with what
>> happens for plain tables; that should probably be fixed.
>
> OK, I think we should fix the issue that postgres_fdw produces incorrect
> aliases for joining relations shown in the Relations line in EXPLAIN for a
> join pushdown query like the above) in advance of the 9.6 release, so I'll
> add this to the 9.6 open items.

Isn't it a bit late for that?

I'm not eager to have 9.6 get further delayed while we work on this
issue, and I definitely don't believe that this is a sufficiently
important issue to justify reverting join pushdown in its entirety.
We're talking about a minor detail of the EXPLAIN output that we'd
like to fix, but for which we have no consensus on exactly how to fix
it, and the fix may involve some redesign.  That does not seem like a
good thing to the week before rc1.  Let's just leave this well enough
alone and work on it for v10.

-- 
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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/08/10 5:19, Robert Haas wrote:

On Mon, Aug 8, 2016 at 12:22 AM, Etsuro Fujita
 wrote:

One thing we need to do to leave that as is would be to fix a bug that I
pointed out upthred.  Let me explain about that again.  The EXPLAIN command
selects relation aliases to be used in printing a query so that each
selected alias is unique, but postgres_fdw wouldn't consider the uniqueness.
Here is an example:

postgres=# explain verbose select * from (select t1.a, t2.a from ft1 t1, ft2
t2 where t1.a = t2.a union select t1.a, t2.a from ft1 t1, ft2 t2 where t1.a
= t2.a) as t(t1a, t2a);
 QUERY PLAN

 Unique  (cost=204.12..204.13 rows=2 width=8)
   Output: t1.a, t2.a
   ->  Sort  (cost=204.12..204.12 rows=2 width=8)
 Output: t1.a, t2.a
 Sort Key: t1.a, t2.a
 ->  Append  (cost=100.00..204.11 rows=2 width=8)
   ->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
 Output: t1.a, t2.a
 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
 Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1 INNER
JOIN public.t2 r2 ON (((r1.a = r2.a
   ->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
 Output: t1_1.a, t2_1.a
 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
 Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1 INNER
JOIN public.t2 r2 ON (((r1.a = r2.a
(14 rows)

The relation aliases in the Relations line in the second Foreign Scan, t1
and t2 for ft1 and ft2, are not unique; they should be t1_1 and t2_1
(compare the aliases in the Relations line with ones in the Output line
directly above that, created by core).  The reason for that is because
postgres_fdw has created the Relations info by using rte->eref->aliasname as
relation aliases as is at path-creation time. Probably it would be a little
bit too early for postgers_fdw to do that.  Couldn't postgres_fdw create
that info after query planning, for example, during ExplainForeignScan?



Yes, it seems what we are doing now is not consistent with what
happens for plain tables; that should probably be fixed.


OK, I think we should fix the issue that postgres_fdw produces incorrect 
aliases for joining relations shown in the Relations line in EXPLAIN for 
a join pushdown query like the above) in advance of the 9.6 release, so 
I'll add this to the 9.6 open items.


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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Robert Haas]

On Mon, Aug 8, 2016 at 12:22 AM, Etsuro Fujita
 wrote:
>>> I noticed that currently the core doesn't show any information on the
>>> target
>>> relations involved in a foreign/custom join in EXPLAIN, by itself.
>> I think that's a feature, not a bug.
> I agree with you.  I'd leave that for 10.0.

I don't want to change it at all, neither in 10 or any later version.

>> I disagree with that.  Currently, when we say that something is a join
>> (Merge Join, Hash Join, Nested Loop) we mean that the executor is
>> performing a join, but that's not the case here.  The executor is
>> performing a scan.  The remote side, we suppose, is performing a join
>> for us, but we are not performing a join: we are performing a scan.
>> So I think the fact that it shows up in the plan as "Foreign Scan" is
>> exactly right.  We are scanning some foreign thing, and that thing may
>> internally be doing other stuff, like joins and aggregates, but all
>> we're doing is scanning it.
>
> Hmm.  One thing I'm concerned about would be the case where direct
> modification is implemented by using GetForeignUpperPaths, not
> PlanDirectModify.  In that case, the way things are now, we would have
> "Foreign Scan" followed by an INSERT/UPDATE/DELETE query, but that seems odd
> to me.

I don't think there's really any problem there.  But if there is,
let's solve it when someone submits that patch, not now.

> One thing we need to do to leave that as is would be to fix a bug that I
> pointed out upthred.  Let me explain about that again.  The EXPLAIN command
> selects relation aliases to be used in printing a query so that each
> selected alias is unique, but postgres_fdw wouldn't consider the uniqueness.
> Here is an example:
>
> postgres=# explain verbose select * from (select t1.a, t2.a from ft1 t1, ft2
> t2 where t1.a = t2.a union select t1.a, t2.a from ft1 t1, ft2 t2 where t1.a
> = t2.a) as t(t1a, t2a);
>  QUERY PLAN
> 
>  Unique  (cost=204.12..204.13 rows=2 width=8)
>Output: t1.a, t2.a
>->  Sort  (cost=204.12..204.12 rows=2 width=8)
>  Output: t1.a, t2.a
>  Sort Key: t1.a, t2.a
>  ->  Append  (cost=100.00..204.11 rows=2 width=8)
>->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
>  Output: t1.a, t2.a
>  Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
>  Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1 INNER
> JOIN public.t2 r2 ON (((r1.a = r2.a
>->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
>  Output: t1_1.a, t2_1.a
>  Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
>  Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1 INNER
> JOIN public.t2 r2 ON (((r1.a = r2.a
> (14 rows)
>
> The relation aliases in the Relations line in the second Foreign Scan, t1
> and t2 for ft1 and ft2, are not unique; they should be t1_1 and t2_1
> (compare the aliases in the Relations line with ones in the Output line
> directly above that, created by core).  The reason for that is because
> postgres_fdw has created the Relations info by using rte->eref->aliasname as
> relation aliases as is at path-creation time. Probably it would be a little
> bit too early for postgers_fdw to do that.  Couldn't postgres_fdw create
> that info after query planning, for example, during ExplainForeignScan?

Yes, it seems what we are doing now is not consistent with what
happens for plain tables; that should probably be fixed.

-- 
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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/08/05 21:47, Robert Haas wrote:

On Tue, Jul 26, 2016 at 11:20 PM, Etsuro Fujita
 wrote:

I noticed that currently the core doesn't show any information on the target
relations involved in a foreign/custom join in EXPLAIN, by itself.



I think that's a feature, not a bug.


I agree with you.  I'd leave that for 10.0.


postgres_fdw shows the target relations in the Relations line, as shown
above, but I think that the core should show such information independently
of FDWs; in the above example replace "Foreign Scan" with "Foreign Join on
public.ft1 t1, public.ft2 t2".



I disagree with that.  Currently, when we say that something is a join
(Merge Join, Hash Join, Nested Loop) we mean that the executor is
performing a join, but that's not the case here.  The executor is
performing a scan.  The remote side, we suppose, is performing a join
for us, but we are not performing a join: we are performing a scan.
So I think the fact that it shows up in the plan as "Foreign Scan" is
exactly right.  We are scanning some foreign thing, and that thing may
internally be doing other stuff, like joins and aggregates, but all
we're doing is scanning it.


Hmm.  One thing I'm concerned about would be the case where direct 
modification is implemented by using GetForeignUpperPaths, not 
PlanDirectModify.  In that case, the way things are now, we would have 
"Foreign Scan" followed by an INSERT/UPDATE/DELETE query, but that seems 
odd to me.



Also, I don't really see the point of moving this from postgres_fdw to
core.  If, at some point in time, there are many FDWs that implement
sophisticated pushdown operations and we figure out that they are all
duplicating the code to do the EXPLAIN printout, and they're all
printing basically the same thing, perhaps not in an entirely
consistent way, then we could try to unify all of them into one
implementation in core.  But that's certainly not where we are right
now.  I don't see any harm at all in leaving this under the control of
the FDW, and in fact, I think it's better.  Neither the postgres_fdw
format nor what you want to replace it with are so unambiguously
awesome that some other FDW author might not come up with something
better.

I think we should leave this the way it is.


One thing we need to do to leave that as is would be to fix a bug that I 
pointed out upthred.  Let me explain about that again.  The EXPLAIN 
command selects relation aliases to be used in printing a query so that 
each selected alias is unique, but postgres_fdw wouldn't consider the 
uniqueness.  Here is an example:


postgres=# explain verbose select * from (select t1.a, t2.a from ft1 t1, 
ft2 t2 where t1.a = t2.a union select t1.a, t2.a from ft1 t1, ft2 t2 
where t1.a = t2.a) as t(t1a, t2a);

 QUERY PLAN

 Unique  (cost=204.12..204.13 rows=2 width=8)
   Output: t1.a, t2.a
   ->  Sort  (cost=204.12..204.12 rows=2 width=8)
 Output: t1.a, t2.a
 Sort Key: t1.a, t2.a
 ->  Append  (cost=100.00..204.11 rows=2 width=8)
   ->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
 Output: t1.a, t2.a
 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
 Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1 
INNER JOIN public.t2 r2 ON (((r1.a = r2.a

   ->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
 Output: t1_1.a, t2_1.a
 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
 Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1 
INNER JOIN public.t2 r2 ON (((r1.a = r2.a

(14 rows)

The relation aliases in the Relations line in the second Foreign Scan, 
t1 and t2 for ft1 and ft2, are not unique; they should be t1_1 and t2_1 
(compare the aliases in the Relations line with ones in the Output line 
directly above that, created by core).  The reason for that is because 
postgres_fdw has created the Relations info by using 
rte->eref->aliasname as relation aliases as is at path-creation time. 
Probably it would be a little bit too early for postgers_fdw to do that. 
 Couldn't postgres_fdw create that info after query planning, for 
example, during ExplainForeignScan?


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] Oddity with NOT IN

[Corey Huinker]

On Sat, Aug 6, 2016 at 2:13 PM, Jim Nasby  wrote:

> On 8/6/16 12:57 PM, Andrew Gierth wrote:
>
>> The easy to catch case, I think, is when the targetlist of the IN or NOT
>> IN subquery contains vars of the outer query level but no vars of the
>> inner one and no volatile functions. This can be checked for with a
>> handful of lines in the parser or a couple of dozen lines in a plugin
>> module (though one would have to invent an error code, none of the
>> existing WARNING sqlstates would do).
>>
>
> I would still like to warn on any outer vars show up in the target list
> (other than as function params), because it's still very likely to be a
> bug. But I agree that what you describe is even more certain to be one.
>
> Maybe David Fetter's suggested module for catching missing WHERE clauses
>> could be expanded into a more general SQL-'Lint' module?
>>
>
> Possibly, though I hadn't really considered treating this condition as an
> error.
>
> Also, there's some other common gotchas that we could better warn users
> about, some of which involve DDL. One example is accidentally defining
> duplicate indexes.
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
> 855-TREBLE2 (855-873-2532)   mobile: 512-569-9461
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

If we are contemplating a setting wherein we issue debug/notice/warning
messages for potentially erroneous SQL, I would suggest a simple test would
be any reference to a column without the a corresponding table/alias.

This is fine:
SELECT a.x, b.y FROM table_that_has_x a JOIN table_that_has_y b ON a.id
= b.foreign_id
This gives the notice/warning:
SELECT x, b.y FROM table_that_has_x a JOIN table_that_has_y b ON a.id =
b.foreign_id

We'd have to suppress the warning in cases where no tables are mentioned
(no table to alias, i.e. "SELECT 'a_constant' as config_var"), and I could
see a reason for suppressing it where only one table is mentioned, though I
often urge table aliasing and full references because it makes it easier
when you modify the query to add another table.

Some setting name suggestions:

notify_vague_column_reference = (on,off)
pedantic_column_identifiers = (off,debug,notice,warn,error)

Re: [HACKERS] Oddity with NOT IN

[Jim Nasby]

On 8/6/16 12:57 PM, Andrew Gierth wrote:

The easy to catch case, I think, is when the targetlist of the IN or NOT
IN subquery contains vars of the outer query level but no vars of the
inner one and no volatile functions. This can be checked for with a
handful of lines in the parser or a couple of dozen lines in a plugin
module (though one would have to invent an error code, none of the
existing WARNING sqlstates would do).


I would still like to warn on any outer vars show up in the target list 
(other than as function params), because it's still very likely to be a 
bug. But I agree that what you describe is even more certain to be one.



Maybe David Fetter's suggested module for catching missing WHERE clauses
could be expanded into a more general SQL-'Lint' module?


Possibly, though I hadn't really considered treating this condition as 
an error.


Also, there's some other common gotchas that we could better warn users 
about, some of which involve DDL. One example is accidentally defining 
duplicate indexes.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


--
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] Oddity with NOT IN

[Andrew Gierth]

> "Andrew" == Andrew Gierth  writes:

 Andrew> The easy to catch case, I think, is when the targetlist of the
 Andrew> IN or NOT IN subquery contains vars of the outer query level
 Andrew> but no vars of the inner one and no volatile functions. This
 Andrew> can be checked for with a handful of lines in the parser or a
 Andrew> couple of dozen lines in a plugin module (though one would have
 Andrew> to invent an error code, none of the existing WARNING sqlstates
 Andrew> would do).

Actually thinking about this, if you did it in a module, you'd probably
want to make it an ERROR not a WARNING, because you'd want to actually
stop queries like

delete from t1 where x in (select x from table_with_no_column_x);

rather than let them run.

-- 
Andrew (irc:RhodiumToad)


-- 
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] Oddity with NOT IN

[Pavel Stehule]

2016-08-06 20:01 GMT+02:00 Jim Nasby :

> On 8/6/16 12:03 PM, Pavel Stehule wrote:
>
>> It would be very useful if we had some way to warn users about stuff
>> like this. Emitting a NOTICE comes to mind.
>>
>>
>> This can be valid query
>>
>
> Right, but in my experience it's an extremely uncommon pattern and much
> more likely to be a mistake (that ends up being very time consuming to
> debug). That's why I think something like a NOTICE or even a WARNING would
> be useful. The only thing I don't like about that idea is if you ever did
> actually want this behavior you'd have to do something to squash the
> ereport. Though, that's a problem we already have in some places, so
> perhaps not worth worrying about.


I worked for company where they generated sets of SQL queries as result of
transformation from multidimensional query language. Some similar queries
are possible there.

I don't thing so using NOTICE or WARNING for any valid query is good idea.

I like the idea of some special extension than can block or raises warning
for some strange plans like this or with Cartesian product ...

Regards

Pavel


>
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
> 855-TREBLE2 (855-873-2532)   mobile: 512-569-9461
>

Re: [HACKERS] Oddity with NOT IN

[Jim Nasby]

On 8/6/16 12:03 PM, Pavel Stehule wrote:

It would be very useful if we had some way to warn users about stuff
like this. Emitting a NOTICE comes to mind.


This can be valid query


Right, but in my experience it's an extremely uncommon pattern and much 
more likely to be a mistake (that ends up being very time consuming to 
debug). That's why I think something like a NOTICE or even a WARNING 
would be useful. The only thing I don't like about that idea is if you 
ever did actually want this behavior you'd have to do something to 
squash the ereport. Though, that's a problem we already have in some 
places, so perhaps not worth worrying about.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


--
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] Oddity with NOT IN

[Andrew Gierth]

> "Pavel" == Pavel Stehule  writes:

 >> Well now I feel dumb...
 >> 
 >> It would be very useful if we had some way to warn users about stuff
 >> like this. Emitting a NOTICE comes to mind.

 Pavel> This can be valid query

It can be, but it essentially never is. The cases where you genuinely
want a correlated IN query are rare, but even then there would be
something in the targetlist that referenced the inner query.

The easy to catch case, I think, is when the targetlist of the IN or NOT
IN subquery contains vars of the outer query level but no vars of the
inner one and no volatile functions. This can be checked for with a
handful of lines in the parser or a couple of dozen lines in a plugin
module (though one would have to invent an error code, none of the
existing WARNING sqlstates would do).

Maybe David Fetter's suggested module for catching missing WHERE clauses
could be expanded into a more general SQL-'Lint' module?

-- 
Andrew (irc:RhodiumToad)


-- 
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] Oddity with NOT IN

[Pavel Stehule]

2016-08-06 18:53 GMT+02:00 Jim Nasby :

> On 8/4/16 4:53 PM, Marko Tiikkaja wrote:
>
>> On 2016-08-04 11:23 PM, Jim Nasby wrote:
>>
>>> I've got a customer that discovered something odd...
>>>
>>> SELECT f1 FROM v1 WHERE f2 not in (SELECT bad FROM v2 WHERE f3 = 1);
>>>
>>> does not error, even though bad doesn't exist, but
>>>
>>
>> I'm guessing there's a v1.bad?
>>
>> This is a common mistake, and also why I recommend always table
>> qualifying column references when there's more than one table in scope.
>>
>
> Well now I feel dumb...
>
> It would be very useful if we had some way to warn users about stuff like
> this. Emitting a NOTICE comes to mind.
>

This can be valid query

Regards

Pavel


> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
> 855-TREBLE2 (855-873-2532)   mobile: 512-569-9461
>
>
> --
> 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] Oddity with NOT IN

[Jim Nasby]

On 8/4/16 4:53 PM, Marko Tiikkaja wrote:

On 2016-08-04 11:23 PM, Jim Nasby wrote:

I've got a customer that discovered something odd...

SELECT f1 FROM v1 WHERE f2 not in (SELECT bad FROM v2 WHERE f3 = 1);

does not error, even though bad doesn't exist, but


I'm guessing there's a v1.bad?

This is a common mistake, and also why I recommend always table
qualifying column references when there's more than one table in scope.


Well now I feel dumb...

It would be very useful if we had some way to warn users about stuff 
like this. Emitting a NOTICE comes to mind.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


--
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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Robert Haas]

On Tue, Jul 26, 2016 at 11:20 PM, Etsuro Fujita
 wrote:
> I noticed that currently the core doesn't show any information on the target
> relations involved in a foreign/custom join in EXPLAIN, by itself.

I think that's a feature, not a bug.

> postgres_fdw shows the target relations in the Relations line, as shown
> above, but I think that the core should show such information independently
> of FDWs; in the above example replace "Foreign Scan" with "Foreign Join on
> public.ft1 t1, public.ft2 t2".

I disagree with that.  Currently, when we say that something is a join
(Merge Join, Hash Join, Nested Loop) we mean that the executor is
performing a join, but that's not the case here.  The executor is
performing a scan.  The remote side, we suppose, is performing a join
for us, but we are not performing a join: we are performing a scan.
So I think the fact that it shows up in the plan as "Foreign Scan" is
exactly right.  We are scanning some foreign thing, and that thing may
internally be doing other stuff, like joins and aggregates, but all
we're doing is scanning it.

Also, I don't really see the point of moving this from postgres_fdw to
core.  If, at some point in time, there are many FDWs that implement
sophisticated pushdown operations and we figure out that they are all
duplicating the code to do the EXPLAIN printout, and they're all
printing basically the same thing, perhaps not in an entirely
consistent way, then we could try to unify all of them into one
implementation in core.  But that's certainly not where we are right
now.  I don't see any harm at all in leaving this under the control of
the FDW, and in fact, I think it's better.  Neither the postgres_fdw
format nor what you want to replace it with are so unambiguously
awesome that some other FDW author might not come up with something
better.

I think we should leave this the way it is.

-- 
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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/08/02 21:35, Etsuro Fujita wrote:

I removed the Relations line.  Here is an updated version of the patch.


I revised code and comments a bit.  Attached is an updated version of  
the patch.


Best regards,
Etsuro Fujita


explain-for-foreign-join-pushdown-v2.patch
Description: binary/octet-stream

-- 
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] Oddity with NOT IN

[Marko Tiikkaja]

On 2016-08-04 11:23 PM, Jim Nasby wrote:

I've got a customer that discovered something odd...

SELECT f1 FROM v1 WHERE f2 not in (SELECT bad FROM v2 WHERE f3 = 1);

does not error, even though bad doesn't exist, but


I'm guessing there's a v1.bad?

This is a common mistake, and also why I recommend always table 
qualifying column references when there's more than one table in scope.



.m


--
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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/08/04 18:03, Kouhei Kaigai wrote:

Kaigai-san wrote:

Also, the logic to print "Foreign (Scan|Insert|Update|Delete)" is different
from what I suggested. I'm suggesting to allow extension giving a label
to fill up "Foreign %s" format.

Please explain why your choice is better than my proposition.


I wrote:

No, I haven't done anything about that yet.  I kept the behavior as-is.



At least, my proposition is available to apply on both of foreign-scan and
custom-scan, and no need to future maintenance if and when FDW gets support
remote Aggregation, Sort or others.



I'd like to discuss this issue separately, maybe in a new thread.



Why do you try to re-invent a similar infrastructure twice and separately?


As I said above, I haven't changed the behavior of EXPLAIN for *upper  
relation processing* such as aggregation or sorting in a ForeignScan or  
CustomScan node.



What I proposed perfectly covers what you want to do, and has more benefits.
- A common manner for both of ForeignScan and CustomScan
- Flexibility to control "Foreign XXX" label and relation names to be printed.


That may be so or not, but more importantly, this is more like a user  
interface problem, so each person would have different opinions about that.



Even if it is sufficient for the current usage of FDW, I've been saying your
proposition is not sufficient for CustomScan nowadays, and ForeignScan in the
near future.


Again I haven't done anything about the EXPLAIN for upper relation  
processing in both ForeignScan and CustomScan cases.  I kept the  
behavior as-is, but I don't think the behavior as-is is OK, either.



It is not an answer to ignore the CustomScan side, because we have to enhanced
the infrastructure of CustomScan side to follow up FDW sooner or later.
However, we will have to apply a different manner on CustomScan side, or 
overwrite
what you proposed on FDW side, at that time.
It is not a desirable future.


I agree on that point that it's better to handle both ForeignScan and  
CustomScan cases the same way.


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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Kouhei Kaigai]

> -Original Message-
> From: Etsuro Fujita [mailto:fujita.ets...@lab.ntt.co.jp]
> Sent: Thursday, August 04, 2016 4:42 PM
> To: Kaigai Kouhei(海外 浩平); pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] Oddity in EXPLAIN for foreign/custom join pushdown 
> plans
> 
> On 2016/08/02 22:02, Kouhei Kaigai wrote:
> 
> I wrote:
> >> I removed the Relations line.  Here is an updated version of the patch.
> >>
> >> * As I said upthread, I left the upper-relation handling for another
> >> patch.  Currently, the patch prints "Foreign Scan" with no relations in
> >> that case.
> >>
> >> * I kept custom joins as-is.  We would need discussions about how to
> >> choose relations we print in EXPLAIN, so I'd also like to leave that for
> >> yet another patch.
> 
> > Please don't rely on fs_relids bitmap to pick up relations to be printed.
> > It just hold a set of underlying relations, but it does not mean all of
> > these relations are actually scanned inside of the ForeignScan.
> 
> Firstly, I'd like to discuss about what the relations printed after
> "Foreign join" would mean.  I think they would mean the relations
> involved in that foreign join (in other words, the ones that participate
> in that foreign join) rather than the relations to be scanned by a
> Foreign Scan representing that foreign join.  Wouldn't that make sense?
> 
> In that sense I think it's reasonable to print all relations specified
> in fs_relids after "Foreign Join".  (And in that sense I was thinking we
> could handle the custom join case the same way as the foreign join case.)
> 
> > You didn't answer the following scenario I pointed out in the upthread.
> >
> > | Please assume an enhancement of postgres_fdw that reads a small local 
> > table (tbl_1)
> > | and parse them as VALUES() clause within a remote query to execute remote 
> > join
> > | with foreign tables (ftbl_2, ftbl_3).
> > | This ForeignScan node has three underlying relations; tbl_1, ftbl_2 and 
> > ftbl_3.
> > | Likely, tbl_1 will be scanned by SeqScan, not ForeignScan itself.
> > | In this case, which relations shall be printed around ForeignScan?
> > | Is it possible to choose proper relation names without hint by the 
> > extension?
> > |
> >
> > To care about these FDW usage, you should add an alternative bitmap rather
> > than fs_relids/custom_relids. My suggestion is, having explain_relids for
> > the purpose.
> 
> We currently don't allow such a join to be performed as a foreign join,
> because we allow a join to be performed so if the relations of the join
> are all foreign tables belonging to the same remote server, as you know.
> 
> So, as I said upthread, I think it would make sense to introduce such a
> bitmap when we extend the existing foreign-join pushdown infrastructure
> so that we can do such a thing as proposed by you.  I guess that that
> would need to extend the concept of foreign joins, though.
>
OK, right now, FDW does not support to take arbitrary child nodes, unlike
CustomScan. However, it implies your proposed infrastructure also has to
be revised once FDW gets enhanced in the near future.

> > Also, the logic to print "Foreign (Scan|Insert|Update|Delete)" is different
> > from what I suggested. I'm suggesting to allow extension giving a label
> > to fill up "Foreign %s" format.
> >
> > Please explain why your choice is better than my proposition.
> 
> No, I haven't done anything about that yet.  I kept the behavior as-is.
> 
> > At least, my proposition is available to apply on both of foreign-scan and
> > custom-scan, and no need to future maintenance if and when FDW gets support
> > remote Aggregation, Sort or others.
> 
> I'd like to discuss this issue separately, maybe in a new thread.
>
Why do you try to re-invent a similar infrastructure twice and separately?

What I proposed perfectly covers what you want to do, and has more benefits.
- A common manner for both of ForeignScan and CustomScan
- Flexibility to control "Foreign XXX" label and relation names to be printed.

Even if it is sufficient for the current usage of FDW, I've been saying your
proposition is not sufficient for CustomScan nowadays, and ForeignScan in the
near future.
It is not an answer to ignore the CustomScan side, because we have to enhanced
the infrastructure of CustomScan side to follow up FDW sooner or later.
However, we will have to apply a different manner on CustomScan side, or 
overwrite
what you proposed on FDW side, at that time.
It is not a desirable future.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kai...@ak.jp.nec.com>



-- 
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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/08/02 22:02, Kouhei Kaigai wrote:

I wrote:

I removed the Relations line.  Here is an updated version of the patch.

* As I said upthread, I left the upper-relation handling for another
patch.  Currently, the patch prints "Foreign Scan" with no relations in
that case.

* I kept custom joins as-is.  We would need discussions about how to
choose relations we print in EXPLAIN, so I'd also like to leave that for
yet another patch.



Please don't rely on fs_relids bitmap to pick up relations to be printed.
It just hold a set of underlying relations, but it does not mean all of
these relations are actually scanned inside of the ForeignScan.


Firstly, I'd like to discuss about what the relations printed after  
"Foreign join" would mean.  I think they would mean the relations  
involved in that foreign join (in other words, the ones that participate  
in that foreign join) rather than the relations to be scanned by a  
Foreign Scan representing that foreign join.  Wouldn't that make sense?


In that sense I think it's reasonable to print all relations specified  
in fs_relids after "Foreign Join".  (And in that sense I was thinking we  
could handle the custom join case the same way as the foreign join case.)



You didn't answer the following scenario I pointed out in the upthread.

| Please assume an enhancement of postgres_fdw that reads a small local table 
(tbl_1)
| and parse them as VALUES() clause within a remote query to execute remote join
| with foreign tables (ftbl_2, ftbl_3).
| This ForeignScan node has three underlying relations; tbl_1, ftbl_2 and 
ftbl_3.
| Likely, tbl_1 will be scanned by SeqScan, not ForeignScan itself.
| In this case, which relations shall be printed around ForeignScan?
| Is it possible to choose proper relation names without hint by the extension?
|

To care about these FDW usage, you should add an alternative bitmap rather
than fs_relids/custom_relids. My suggestion is, having explain_relids for
the purpose.


We currently don't allow such a join to be performed as a foreign join,  
because we allow a join to be performed so if the relations of the join  
are all foreign tables belonging to the same remote server, as you know.


So, as I said upthread, I think it would make sense to introduce such a  
bitmap when we extend the existing foreign-join pushdown infrastructure  
so that we can do such a thing as proposed by you.  I guess that that  
would need to extend the concept of foreign joins, though.



Also, the logic to print "Foreign (Scan|Insert|Update|Delete)" is different
from what I suggested. I'm suggesting to allow extension giving a label
to fill up "Foreign %s" format.

Please explain why your choice is better than my proposition.


No, I haven't done anything about that yet.  I kept the behavior as-is.


At least, my proposition is available to apply on both of foreign-scan and
custom-scan, and no need to future maintenance if and when FDW gets support
remote Aggregation, Sort or others.


I'd like to discuss this issue separately, maybe in a new thread.

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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Kouhei Kaigai]

> -Original Message-
> From: Etsuro Fujita [mailto:fujita.ets...@lab.ntt.co.jp]
> Sent: Tuesday, August 02, 2016 9:36 PM
> To: Kaigai Kouhei(海外 浩平); pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] Oddity in EXPLAIN for foreign/custom join pushdown 
> plans
> 
> On 2016/08/01 20:15, Etsuro Fujita wrote:
> > I thought about the Relations line a bit more and noticed that there are
> > cases where the table reference names for joining relations in the
> > Relations line are printed incorrectly.  Here is an example:
> >
> > postgres=# explain verbose select * from (select t1.a, t2.a from ft1 t1,
> > ft2 t2 where t1.a = t2.a union select t1.a, t2.a from ft1 t1, ft2 t2
> > where t1.a = t2.a) as t(t1a, t2a);
> >  QUERY PLAN
> >
> --
> --
> >
> >  Unique  (cost=204.12..204.13 rows=2 width=8)
> >Output: t1.a, t2.a
> >->  Sort  (cost=204.12..204.12 rows=2 width=8)
> >  Output: t1.a, t2.a
> >  Sort Key: t1.a, t2.a
> >  ->  Append  (cost=100.00..204.11 rows=2 width=8)
> >->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
> >  Output: t1.a, t2.a
> >  Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
> >  Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1
> > INNER JOIN public.t2 r2 ON (((r1.a = r2.a
> >->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
> >  Output: t1_1.a, t2_1.a
> >  Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
> >  Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1
> > INNER JOIN public.t2 r2 ON (((r1.a = r2.a
> > (14 rows)
> >
> > The table reference names for ft1 and ft2 in the Relations line for the
> > second Foreign Scan should be t1_1 and t2_1 respectively.
> >
> > Another concern about the Relations line is, that represents just an
> > internal representation of a pushed-down join, so that would not
> > necessarily match a deparsed query shown in the Remote SQL line.  Here
> > is an example, which I found when working on supporting pushing down
> > full outer join a lot more, by improving the deparsing logic so that
> > postgres_fdw can build a remote query that involves subqueries [1],
> > which I'll work on for 10.0:
> >
> > + -- full outer join with restrictions on the joining relations
> > + EXPLAIN (COSTS false, VERBOSE)
> > + SELECT t1.c1, t2.c1 FROM (SELECT c1 FROM ft4 WHERE c1 BETWEEN 50 AND
> > 60) t1 FULL JOIN (SELECT c1 FROM ft5 WHERE c1 BETWEEN 50 AND 60) t2 ON
> > (t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1;
> > + QUERY
> > PLAN
> > +
> >
> --
> --
> --
> -
> >
> > +  Foreign Scan
> > +Output: ft4.c1, ft5.c1
> > +Relations: (public.ft4) FULL JOIN (public.ft5)
> > +Remote SQL: SELECT ss1.c1, ss2.c1 FROM ((SELECT c1 FROM "S 1"."T 3"
> > WHERE ((c1 >= 50)) AND ((c1 <= 60))) ss1(c1) FULL JOIN (SELECT c1 FROM
> > "S 1"."T 4" WHERE ((c1 >= 50)) AND ((c1 <= 60))) ss2(c1) ON (((ss1.c1 =
> > ss2.c1 ORDER BY ss1.c1 ASC NULLS LAST, ss2.c1 ASC NULLS LAST
> > + (4 rows)
> >
> > "(public.ft4) FULL JOIN (public.ft5)" in the Relations line does not
> > exactly match the deparsed query in the Remote SQL line, which I think
> > would be rather confusing for users.  (We may be able to print more
> > exact information in the Relations line so as to match the depaserd
> > query, but I think that that would make the Relations line redundant.)
> >
> > Would we really need the Relations line?  If joining relations are
> > printed by core like "Foreign Join on public.ft1 t1_1, public.ft2 t2_1"
> > as proposed upthread, we can see those relations from that, not the
> > Relations line.  Also we can see the join tree structure from the
> > deparsed query in the Remote SQL line.  The Relations line seems to be
> > not that useful anymore, then.  What do you think about that?
> 
> I removed the Relations line.  Here is an updated version of the pa

Re: [HACKERS] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/08/01 20:15, Etsuro Fujita wrote:

I thought about the Relations line a bit more and noticed that there are
cases where the table reference names for joining relations in the
Relations line are printed incorrectly.  Here is an example:

postgres=# explain verbose select * from (select t1.a, t2.a from ft1 t1,
ft2 t2 where t1.a = t2.a union select t1.a, t2.a from ft1 t1, ft2 t2
where t1.a = t2.a) as t(t1a, t2a);
 QUERY PLAN


 Unique  (cost=204.12..204.13 rows=2 width=8)
   Output: t1.a, t2.a
   ->  Sort  (cost=204.12..204.12 rows=2 width=8)
 Output: t1.a, t2.a
 Sort Key: t1.a, t2.a
 ->  Append  (cost=100.00..204.11 rows=2 width=8)
   ->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
 Output: t1.a, t2.a
 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
 Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1
INNER JOIN public.t2 r2 ON (((r1.a = r2.a
   ->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
 Output: t1_1.a, t2_1.a
 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
 Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1
INNER JOIN public.t2 r2 ON (((r1.a = r2.a
(14 rows)

The table reference names for ft1 and ft2 in the Relations line for the
second Foreign Scan should be t1_1 and t2_1 respectively.

Another concern about the Relations line is, that represents just an
internal representation of a pushed-down join, so that would not
necessarily match a deparsed query shown in the Remote SQL line.  Here
is an example, which I found when working on supporting pushing down
full outer join a lot more, by improving the deparsing logic so that
postgres_fdw can build a remote query that involves subqueries [1],
which I'll work on for 10.0:

+ -- full outer join with restrictions on the joining relations
+ EXPLAIN (COSTS false, VERBOSE)
+ SELECT t1.c1, t2.c1 FROM (SELECT c1 FROM ft4 WHERE c1 BETWEEN 50 AND
60) t1 FULL JOIN (SELECT c1 FROM ft5 WHERE c1 BETWEEN 50 AND 60) t2 ON
(t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1;
+ QUERY
PLAN
+
---

+  Foreign Scan
+Output: ft4.c1, ft5.c1
+Relations: (public.ft4) FULL JOIN (public.ft5)
+Remote SQL: SELECT ss1.c1, ss2.c1 FROM ((SELECT c1 FROM "S 1"."T 3"
WHERE ((c1 >= 50)) AND ((c1 <= 60))) ss1(c1) FULL JOIN (SELECT c1 FROM
"S 1"."T 4" WHERE ((c1 >= 50)) AND ((c1 <= 60))) ss2(c1) ON (((ss1.c1 =
ss2.c1 ORDER BY ss1.c1 ASC NULLS LAST, ss2.c1 ASC NULLS LAST
+ (4 rows)

"(public.ft4) FULL JOIN (public.ft5)" in the Relations line does not
exactly match the deparsed query in the Remote SQL line, which I think
would be rather confusing for users.  (We may be able to print more
exact information in the Relations line so as to match the depaserd
query, but I think that that would make the Relations line redundant.)

Would we really need the Relations line?  If joining relations are
printed by core like "Foreign Join on public.ft1 t1_1, public.ft2 t2_1"
as proposed upthread, we can see those relations from that, not the
Relations line.  Also we can see the join tree structure from the
deparsed query in the Remote SQL line.  The Relations line seems to be
not that useful anymore, then.  What do you think about that?


I removed the Relations line.  Here is an updated version of the patch.

* As I said upthread, I left the upper-relation handling for another  
patch.  Currently, the patch prints "Foreign Scan" with no relations in  
that case.


* I kept custom joins as-is.  We would need discussions about how to  
choose relations we print in EXPLAIN, so I'd also like to leave that for  
yet another patch.


Best regards,
Etsuro Fujita


explain-for-foreign-join-pushdown.patch
Description: binary/octet-stream

-- 
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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Kouhei Kaigai]

> -Original Message-
> From: Etsuro Fujita [mailto:fujita.ets...@lab.ntt.co.jp]
> Sent: Tuesday, August 02, 2016 2:45 PM
> To: Kaigai Kouhei(海外 浩平); Ashutosh Bapat
> Cc: pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] Oddity in EXPLAIN for foreign/custom join pushdown 
> plans
> 
> On 2016/08/02 13:32, Kouhei Kaigai wrote:
> 
> I wrote:
> >> My concern here is EXPLAIN for foreign joins.  I think it's another
> >> problem how we handle Foreign Scan plan nodes representing
> >> post-scan/join operations in EXPLAIN, so I'd like to leave that for
> >> another patch.
> 
> > What is the post-scan/join operations? Are you saying EPQ recheck and
> > alternative local join plan?
> 
> No.  I mean e.g., aggregation, window functions, sorting, or table
> modification.  In other words, Foreign Scan plan nodes created from
> ForeignPath paths created from GetForeignUpperPaths.
>
Why do you need to handle these upper paths individually?
FDW driver knows the remote query contains aggregation, window functions,
sorting, or table modification. It can give proper label.

If remote query contains partial aggregation and relations join, for
example, "Partial Agg + Scan" will be a proper label that shall be
followed by the "Foreign %s".

All you need to do are the two enhancements:
- Add "const char *explain_label" on the ForeignScanState or somewhere
  extension can set. It gives a label to be printed.
  If NULL string is set, EXPLAIN shows "Foreign Scan" as a default.
- Add "Bitmapset explain_rels" on the ForeignScanState or somewhere
  extension can set. It indicates the relations to be printed after
  the "Foreign %s" token. If you want to print all the relations names
  underlying this ForeignScan node, just copy scanrelids bitmap.
  If NULL bitmap is set, EXPLAIN shows nothing as a default.

Please note that the default does not change the existing behavior.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kai...@ak.jp.nec.com>


-- 
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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/08/02 13:32, Kouhei Kaigai wrote:

I wrote:

My concern here is EXPLAIN for foreign joins.  I think it's another
problem how we handle Foreign Scan plan nodes representing
post-scan/join operations in EXPLAIN, so I'd like to leave that for
another patch.



What is the post-scan/join operations? Are you saying EPQ recheck and
alternative local join plan?


No.  I mean e.g., aggregation, window functions, sorting, or table 
modification.  In other words, Foreign Scan plan nodes created from 
ForeignPath paths created from GetForeignUpperPaths.


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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Kouhei Kaigai]

> On 2016/08/01 22:25, Kouhei Kaigai wrote:
> 
> I wrote:
> >>> a broader
> >>> word like "Processing" seems to work well because we allow various
> >>> kinds of operations to the remote side, in addition to scans/joins,
> >>> to be performed in that one Foreign Scan node indicated by "Foreign
> >>> Processing", such as aggregation, window functions, distinct, order
> >>> by, row locking, table modification, or combinations of them.
> 
>  >> On 2016/07/29 13:28, Ashutosh Bapat wrote:
> >>> "Scan" is a better word than "Processing". From plan's perspective it's
> >>> ultimately a Scan (on the data produced by the foreign server) and not
> >>> processing.
> 
> I wrote:
> >> Exactly, but one thing I'm concerned about is the table modification
> >> case; the EXPLAIN output would be something like this:
> >>
> >>Foreign Scan
> >>  Remote SQL: INSERT INTO remote_table VALUES ...
> >>
> >> That would be strange, so I think a more broader word is better.  I
> >> don't think "Foreign Processing" is best.  "Foreign Upper" might be much
> >> better because the corresponding path is created by calling
> >> GetForeignUpperPaths.
> 
> > Be "Foreign %s", and gives "Insert" label by postgres_fdw; which knows
> > the ForeignScan node actually insert tuples.
> > From the standpoint of users, it looks "Foreign Insert".
> 
> My concern here is EXPLAIN for foreign joins.  I think it's another
> problem how we handle Foreign Scan plan nodes representing
> post-scan/join operations in EXPLAIN, so I'd like to leave that for
> another patch.
>
What is the post-scan/join operations? Are you saying EPQ recheck and
alternative local join plan?

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


-- 
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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/08/01 22:25, Kouhei Kaigai wrote:

I wrote:

a broader
word like "Processing" seems to work well because we allow various
kinds of operations to the remote side, in addition to scans/joins,
to be performed in that one Foreign Scan node indicated by "Foreign
Processing", such as aggregation, window functions, distinct, order
by, row locking, table modification, or combinations of them.


>> On 2016/07/29 13:28, Ashutosh Bapat wrote:

"Scan" is a better word than "Processing". From plan's perspective it's
ultimately a Scan (on the data produced by the foreign server) and not
processing.


I wrote:

Exactly, but one thing I'm concerned about is the table modification
case; the EXPLAIN output would be something like this:

   Foreign Scan
 Remote SQL: INSERT INTO remote_table VALUES ...

That would be strange, so I think a more broader word is better.  I
don't think "Foreign Processing" is best.  "Foreign Upper" might be much
better because the corresponding path is created by calling
GetForeignUpperPaths.



Be "Foreign %s", and gives "Insert" label by postgres_fdw; which knows
the ForeignScan node actually insert tuples.
From the standpoint of users, it looks "Foreign Insert".


My concern here is EXPLAIN for foreign joins.  I think it's another 
problem how we handle Foreign Scan plan nodes representing 
post-scan/join operations in EXPLAIN, so I'd like to leave that for 
another patch.


I wrote:

Also for a Foreign Scan representing a foreign join, I think "Foreign
Join" is better than "Foreign Scan".  Here is an example:

   Foreign Join on foreign_table1, foreign_table2
 Remote SQL: SELECT ... FROM remote_table1 INNER JOIN remote_table2
WHERE ...

I think "Foreign Join" better indicates that foreign tables listed after
that (ie, foreign_table1 and foreign_table2 in the example) are joining
(or component) tables of this join, than "Foreign Scan".



Postgres_fdw knows it is remote join. It is quite easy to tell the core
this ForeignScan node is "Join". Also, it knows which tables are involved in.


Yeah, we can do that.


We can add hint information to control relations name to be printed.


For foreign joins, it would make sense to add such a functionality when 
necessary, for example when we extend the pushdown feature so that we 
can do what you proposed upthread.


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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Kouhei Kaigai]

> -Original Message-
> From: Etsuro Fujita [mailto:fujita.ets...@lab.ntt.co.jp]
> Sent: Monday, August 01, 2016 8:26 PM
> To: Ashutosh Bapat
> Cc: Kaigai Kouhei(海外 浩平); pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] Oddity in EXPLAIN for foreign/custom join pushdown 
> plans
> 
> On 2016/07/29 13:28, Ashutosh Bapat wrote:
> 
> I wrote:
> > Probably something like this:
> >
> >Foreign Processing
> >  Remote Operations: ...
> >
> > In the Remote Operations line, the FDW/extension could print
> > any info
> > about remote operations, eg, "Scan/Join + Aggregate".
> 
> I wrote:
> > I intentionally chose that word and thought we could leave detailed
> > descriptions about remote operations to the FDW/extension; a broader
> > word like "Processing" seems to work well because we allow various
> > kinds of operations to the remote side, in addition to scans/joins,
> > to be performed in that one Foreign Scan node indicated by "Foreign
> > Processing", such as aggregation, window functions, distinct, order
> > by, row locking, table modification, or combinations of them.
> 
> > "Scan" is a better word than "Processing". From plan's perspective it's
> > ultimately a Scan (on the data produced by the foreign server) and not
> > processing.
> 
> Exactly, but one thing I'm concerned about is the table modification
> case; the EXPLAIN output would be something like this:
> 
>Foreign Scan
>  Remote SQL: INSERT INTO remote_table VALUES ...
> 
> That would be strange, so I think a more broader word is better.  I
> don't think "Foreign Processing" is best.  "Foreign Upper" might be much
> better because the corresponding path is created by calling
> GetForeignUpperPaths.
>
Be "Foreign %s", and gives "Insert" label by postgres_fdw; which knows
the ForeignScan node actually insert tuples.
From the standpoint of users, it looks "Foreign Insert".

> Also for a Foreign Scan representing a foreign join, I think "Foreign
> Join" is better than "Foreign Scan".  Here is an example:
> 
>Foreign Join on foreign_table1, foreign_table2
>  Remote SQL: SELECT ... FROM remote_table1 INNER JOIN remote_table2
> WHERE ...
> 
> I think "Foreign Join" better indicates that foreign tables listed after
> that (ie, foreign_table1 and foreign_table2 in the example) are joining
> (or component) tables of this join, than "Foreign Scan".
>
Postgres_fdw knows it is remote join. It is quite easy to tell the core
this ForeignScan node is "Join". Also, it knows which tables are involved in.
We can add hint information to control relations name to be printed.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kai...@ak.jp.nec.com>


-- 
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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/08/01 20:31, Ashutosh Bapat wrote:

I thought about the Relations line a bit more and noticed that there
are cases where the table reference names for joining relations in
the Relations line are printed incorrectly.  Here is an example:

postgres=# explain verbose select * from (select t1.a, t2.a from ft1
t1, ft2 t2 where t1.a = t2.a union select t1.a, t2.a from ft1 t1,
ft2 t2 where t1.a = t2.a) as t(t1a, t2a);
 QUERY PLAN


 Unique  (cost=204.12..204.13 rows=2 width=8)
   Output: t1.a, t2.a
   ->  Sort  (cost=204.12..204.12 rows=2 width=8)
 Output: t1.a, t2.a
 Sort Key: t1.a, t2.a
 ->  Append  (cost=100.00..204.11 rows=2 width=8)
   ->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
 Output: t1.a, t2.a
 Relations: (public.ft1 t1) INNER JOIN
(public.ft2 t2)
 Remote SQL: SELECT r1.a, r2.a FROM (public.t1
r1 INNER JOIN public.t2 r2 ON (((r1.a = r2.a
   ->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
 Output: t1_1.a, t2_1.a
 Relations: (public.ft1 t1) INNER JOIN
(public.ft2 t2)
 Remote SQL: SELECT r1.a, r2.a FROM (public.t1
r1 INNER JOIN public.t2 r2 ON (((r1.a = r2.a
(14 rows)

The table reference names for ft1 and ft2 in the Relations line for
the second Foreign Scan should be t1_1 and t2_1 respectively.



Relations line prints the names of foreign tables that are being joined
and the type of join. I find t1_1 and t2_1 more confusing since the
query that user has provided does not mention t1_1 and t2_1.


Please look at the Output line for the second Foreign Scan in the 
EXPLAIN.  (The reason for that is because postgres_fdw gets table 
reference names directly from rte->eref->aliasname, while EXPLAIN gets 
those through select_rtable_names_for_explain.)



Would we really need the Relations line?  If joining relations are
printed by core like "Foreign Join on public.ft1 t1_1, public.ft2
t2_1" as proposed upthread, we can see those relations from that,
not the Relations line.



The join type is missing in that description.



Also we can see the join tree structure from the deparsed query in
the Remote SQL line.



The remote SQL has the names of the table on the foreign server. It does
not help to identify the local names.


We can see the remote names of the foreign tables from the catalogs, so 
we would easily identify the local names of foreign tables in the remote 
SQL and thus the join type (or join tree structure) from the SQL.


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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Ashutosh Bapat]

> I thought about the Relations line a bit more and noticed that there are
> cases where the table reference names for joining relations in the
> Relations line are printed incorrectly.  Here is an example:
>
> postgres=# explain verbose select * from (select t1.a, t2.a from ft1 t1,
> ft2 t2 where t1.a = t2.a union select t1.a, t2.a from ft1 t1, ft2 t2 where
> t1.a = t2.a) as t(t1a, t2a);
>  QUERY PLAN
>
> 
>  Unique  (cost=204.12..204.13 rows=2 width=8)
>Output: t1.a, t2.a
>->  Sort  (cost=204.12..204.12 rows=2 width=8)
>  Output: t1.a, t2.a
>  Sort Key: t1.a, t2.a
>  ->  Append  (cost=100.00..204.11 rows=2 width=8)
>->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
>  Output: t1.a, t2.a
>  Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
>  Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1
> INNER JOIN public.t2 r2 ON (((r1.a = r2.a
>->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
>  Output: t1_1.a, t2_1.a
>  Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
>  Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1
> INNER JOIN public.t2 r2 ON (((r1.a = r2.a
> (14 rows)
>
> The table reference names for ft1 and ft2 in the Relations line for the
> second Foreign Scan should be t1_1 and t2_1 respectively.
>

Relations line prints the names of foreign tables that are being joined and
the type of join. I find t1_1 and t2_1 more confusing since the query that
user has provided does not mention t1_1 and t2_1.

>
>
> Would we really need the Relations line?  If joining relations are printed
> by core like "Foreign Join on public.ft1 t1_1, public.ft2 t2_1" as proposed
> upthread, we can see those relations from that, not the Relations line.


The join type is missing in that description.


> Also we can see the join tree structure from the deparsed query in the
> Remote SQL line.


The remote SQL has the names of the table on the foreign server. It does
not help to identify the local names.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/07/29 13:28, Ashutosh Bapat wrote:

I wrote:

Probably something like this:

   Foreign Processing
 Remote Operations: ...

In the Remote Operations line, the FDW/extension could print
any info
about remote operations, eg, "Scan/Join + Aggregate".


I wrote:

I intentionally chose that word and thought we could leave detailed
descriptions about remote operations to the FDW/extension; a broader
word like "Processing" seems to work well because we allow various
kinds of operations to the remote side, in addition to scans/joins,
to be performed in that one Foreign Scan node indicated by "Foreign
Processing", such as aggregation, window functions, distinct, order
by, row locking, table modification, or combinations of them.



"Scan" is a better word than "Processing". From plan's perspective it's
ultimately a Scan (on the data produced by the foreign server) and not
processing.


Exactly, but one thing I'm concerned about is the table modification 
case; the EXPLAIN output would be something like this:


  Foreign Scan
Remote SQL: INSERT INTO remote_table VALUES ...

That would be strange, so I think a more broader word is better.  I 
don't think "Foreign Processing" is best.  "Foreign Upper" might be much 
better because the corresponding path is created by calling 
GetForeignUpperPaths.


Also for a Foreign Scan representing a foreign join, I think "Foreign 
Join" is better than "Foreign Scan".  Here is an example:


  Foreign Join on foreign_table1, foreign_table2
Remote SQL: SELECT ... FROM remote_table1 INNER JOIN remote_table2 
WHERE ...


I think "Foreign Join" better indicates that foreign tables listed after 
that (ie, foreign_table1 and foreign_table2 in the example) are joining 
(or component) tables of this join, than "Foreign Scan".


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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/07/29 13:05, Etsuro Fujita wrote:

In a foreign-join case,
however, we can't see such relations from the EXPLAIN printed *by core*.
 postgres_fdw avoids this issue by adding such relations to the EXPLAIN
using ExplainForeignScan as shown in the below example, but since such
relations are essential, I think that information should be shown by
core itself.

postgres=# explain select * from (select ft1.a from ft1 left join ft2 on
ft1.a = ft2.a where ft1.b = 1) ss1(a) full join (select ft3.a from ft3
left join ft4 on ft3.a = ft4.a where ft3.b = 1) ss2(a) on ss1.a = ss2.a;
   QUERY PLAN

 Hash Full Join  (cost=202.06..204.12 rows=1 width=8)
   Hash Cond: (ft1.a = ft3.a)
   ->  Foreign Scan  (cost=100.00..102.05 rows=1 width=4)
 Relations: (public.ft1) LEFT JOIN (public.ft2)
   ->  Hash  (cost=102.05..102.05 rows=1 width=4)
 ->  Foreign Scan  (cost=100.00..102.05 rows=1 width=4)
   Relations: (public.ft3) LEFT JOIN (public.ft4)
(7 rows)

From the Relations line shown by postgres_fdw, we can see which foreign
join joins which foreign tables, but if no such lines, we couldn't.


I thought about the Relations line a bit more and noticed that there are  
cases where the table reference names for joining relations in the  
Relations line are printed incorrectly.  Here is an example:


postgres=# explain verbose select * from (select t1.a, t2.a from ft1 t1,  
ft2 t2 where t1.a = t2.a union select t1.a, t2.a from ft1 t1, ft2 t2  
where t1.a = t2.a) as t(t1a, t2a);

 QUERY PLAN

 Unique  (cost=204.12..204.13 rows=2 width=8)
   Output: t1.a, t2.a
   ->  Sort  (cost=204.12..204.12 rows=2 width=8)
 Output: t1.a, t2.a
 Sort Key: t1.a, t2.a
 ->  Append  (cost=100.00..204.11 rows=2 width=8)
   ->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
 Output: t1.a, t2.a
 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
 Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1  
INNER JOIN public.t2 r2 ON (((r1.a = r2.a

   ->  Foreign Scan  (cost=100.00..102.04 rows=1 width=8)
 Output: t1_1.a, t2_1.a
 Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
 Remote SQL: SELECT r1.a, r2.a FROM (public.t1 r1  
INNER JOIN public.t2 r2 ON (((r1.a = r2.a

(14 rows)

The table reference names for ft1 and ft2 in the Relations line for the  
second Foreign Scan should be t1_1 and t2_1 respectively.


Another concern about the Relations line is, that represents just an  
internal representation of a pushed-down join, so that would not  
necessarily match a deparsed query shown in the Remote SQL line.  Here  
is an example, which I found when working on supporting pushing down  
full outer join a lot more, by improving the deparsing logic so that  
postgres_fdw can build a remote query that involves subqueries [1],  
which I'll work on for 10.0:


+ -- full outer join with restrictions on the joining relations
+ EXPLAIN (COSTS false, VERBOSE)
+ SELECT t1.c1, t2.c1 FROM (SELECT c1 FROM ft4 WHERE c1 BETWEEN 50 AND  
60) t1 FULL JOIN (SELECT c1 FROM ft5 WHERE c1 BETWEEN 50 AND 60) t2 ON  
(t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1;
+  
QUERY  
PLAN  

+  
---

+  Foreign Scan
+Output: ft4.c1, ft5.c1
+Relations: (public.ft4) FULL JOIN (public.ft5)
+Remote SQL: SELECT ss1.c1, ss2.c1 FROM ((SELECT c1 FROM "S 1"."T 3"  
WHERE ((c1 >= 50)) AND ((c1 <= 60))) ss1(c1) FULL JOIN (SELECT c1 FROM  
"S 1"."T 4" WHERE ((c1 >= 50)) AND ((c1 <= 60))) ss2(c1) ON (((ss1.c1 =  
ss2.c1 ORDER BY ss1.c1 ASC NULLS LAST, ss2.c1 ASC NULLS LAST

+ (4 rows)

"(public.ft4) FULL JOIN (public.ft5)" in the Relations line does not  
exactly match the deparsed query in the Remote SQL line, which I think  
would be rather confusing for users.  (We may be able to print more  
exact information in the Relations line so as to match the depaserd  
query, but I think that that would make the Relations line redundant.)


Would we really need the Relations line?  If joining relations are  
printed by core like "Foreign Join on public.ft1 t1_1, public.ft2 t2_1"  
as proposed upthread, we can see those relations from that, not the  
Relations line.  Also we can see the join tree structure from the  
deparsed query in the Remote SQL line.  The Relations line seems to be  
not that 

Re: [HACKERS] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Kouhei Kaigai]

> I wrote:
> >> That may be so, but my point is that the target relations involved in
> >> the foreign join (ie, ft1 and ft2) should be printed somewhere in the
> >> EXPLAIN output by core, as in EXPLAIN for a simple foreign table scan.
> 
> > Why? According to your rule, Hash Join should take "on t0,t1,t2".
> >
> > postgres=# explain select id from t0 natural join t1 natural join t2;
> >  QUERY PLAN
> > -
> >  Hash Join  (cost=6370.00..4560826.24 rows=98784048 width=4)
> >Hash Cond: (t0.aid = t1.aid)
> >->  Hash Join  (cost=3185.00..3199360.58 rows=98784048 width=8)
> >  Hash Cond: (t0.bid = t2.bid)
> >  ->  Seq Scan on t0  (cost=0.00..184.80 rows=10080 width=12)
> >  ->  Hash  (cost=1935.00..1935.00 rows=10 width=4)
> >->  Seq Scan on t2  (cost=0.00..1935.00 rows=10 width=4)
> >->  Hash  (cost=1935.00..1935.00 rows=10 width=4)
> >  ->  Seq Scan on t1  (cost=0.00..1935.00 rows=10 width=4)
> > (9 rows)
> 
> I don't think it needs "on t0,t1,t2", because we can see joining
> relations from inner/outer plans in that case.  In a foreign-join case,
> however, we can't see such relations from the EXPLAIN printed *by core*.
>   postgres_fdw avoids this issue by adding such relations to the EXPLAIN
> using ExplainForeignScan as shown in the below example, but since such
> relations are essential, I think that information should be shown by
> core itself.
>
I never opposed to print the relation names by the core, however, it has
to be controllable by the extension which provides ForeignScan/CustomScan
because only extension can know how underlying relation shall be scanned
exactly.

> postgres=# explain select * from (select ft1.a from ft1 left join ft2 on
> ft1.a = ft2.a where ft1.b = 1) ss1(a) full join (select ft3.a from ft3
> left join ft4 on ft3.a = ft4.a where ft3.b = 1) ss2(a) on ss1.a = ss2.a;
> QUERY PLAN
> 
>   Hash Full Join  (cost=202.06..204.12 rows=1 width=8)
> Hash Cond: (ft1.a = ft3.a)
> ->  Foreign Scan  (cost=100.00..102.05 rows=1 width=4)
>   Relations: (public.ft1) LEFT JOIN (public.ft2)
> ->  Hash  (cost=102.05..102.05 rows=1 width=4)
>   ->  Foreign Scan  (cost=100.00..102.05 rows=1 width=4)
> Relations: (public.ft3) LEFT JOIN (public.ft4)
> (7 rows)
> 
>  From the Relations line shown by postgres_fdw, we can see which foreign
> join joins which foreign tables, but if no such lines, we couldn't.
>
In case of postgres_fdw, if extension can indicate the core EXPLAIN to
print all of its underlying relations, the core EXPLAIN routine will
print name of the relations. Here is no problem.

> >>> postgres=# explain select id from t0 natural join t1 natural join t2;
> >>> QUERY PLAN
> >>> ---
> >>>  Custom Scan (GpuJoin)  (cost=12385.67..291245.35 rows=9815465 width=4)
> >>>GPU Projection: t0.id
> >>>Depth 1: GpuHashJoin, HashKeys: (t0.bid)
> >>> JoinQuals: (t0.bid = t2.bid)
> >>> Nrows (in/out: 98.15%), KDS-Hash (size: 13.47MB, nbatches: 1)
> >>>Depth 2: GpuHashJoin, HashKeys: (t0.aid)
> >>> JoinQuals: (t0.aid = t1.aid)
> >>> Nrows (in/out: 100.00%), KDS-Hash (size: 13.47MB, nbatches: 1)
> >>>->  Seq Scan on t0  (cost=0.00..18.96 rows=996 width=12)
> >>>->  Seq Scan on t2  (cost=0.00..1935.00 rows=10 width=4)
> >>>->  Seq Scan on t1  (cost=0.00..1935.00 rows=10 width=4)
> >>> (11 rows)
> 
> > My largest concern for you proposition is, ForeignScan/CustomScan node is
> > enforced to print name of underlying relations, regardless of its actual
> > behavior. The above GpuJoin never scans tables at least, thus, it mislead
> > users if we have no choice to print underlying relation names.
> 
> OK, I understand we would need special handling for such custom joins.
>
It is not a case only for CustomScan.

Please assume an enhancement of postgres_fdw that reads a small local table 
(tbl_1)
and parse them as VALUES() clause within a remote query to execute remote join
with foreign tables (ftbl_2, ftbl_3).
This ForeignScan node has three underlying relations; tbl_1, ftbl_2 and ftbl_3.
Likely, tbl_1 will be scanned by SeqScan, not ForeignScan itself.
In this case, which relations shall be printed around ForeignScan?
Is it possible to choose proper relation names without hint by the extension?
   
Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Re: [HACKERS] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Ashutosh Bapat]

I wrote:
>
>> Probably something like this:
>>>
>>>Foreign Processing
>>>  Remote Operations: ...
>>>
>>> In the Remote Operations line, the FDW/extension could print any info
>>> about remote operations, eg, "Scan/Join + Aggregate".
>>>
>>
> "Foreign" implies this node is processed by FDW, but "Procesing" gives us
>> no extra information; seems to me redundant.
>>
>
> I intentionally chose that word and thought we could leave detailed
> descriptions about remote operations to the FDW/extension; a broader word
> like "Processing" seems to work well because we allow various kinds of
> operations to the remote side, in addition to scans/joins, to be performed
> in that one Foreign Scan node indicated by "Foreign Processing", such as
> aggregation, window functions, distinct, order by, row locking, table
> modification, or combinations of them.
>
> "Scan" is a better word than "Processing". From plan's perspective it's
ultimately a Scan (on the data produced by the foreign server) and not
processing.
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/07/28 22:11, Kouhei Kaigai wrote:

I wrote:

That may be so, but my point is that the target relations involved in
the foreign join (ie, ft1 and ft2) should be printed somewhere in the
EXPLAIN output by core, as in EXPLAIN for a simple foreign table scan.



Why? According to your rule, Hash Join should take "on t0,t1,t2".

postgres=# explain select id from t0 natural join t1 natural join t2;
 QUERY PLAN
-
 Hash Join  (cost=6370.00..4560826.24 rows=98784048 width=4)
   Hash Cond: (t0.aid = t1.aid)
   ->  Hash Join  (cost=3185.00..3199360.58 rows=98784048 width=8)
 Hash Cond: (t0.bid = t2.bid)
 ->  Seq Scan on t0  (cost=0.00..184.80 rows=10080 width=12)
 ->  Hash  (cost=1935.00..1935.00 rows=10 width=4)
   ->  Seq Scan on t2  (cost=0.00..1935.00 rows=10 width=4)
   ->  Hash  (cost=1935.00..1935.00 rows=10 width=4)
 ->  Seq Scan on t1  (cost=0.00..1935.00 rows=10 width=4)
(9 rows)


I don't think it needs "on t0,t1,t2", because we can see joining  
relations from inner/outer plans in that case.  In a foreign-join case,  
however, we can't see such relations from the EXPLAIN printed *by core*.  
 postgres_fdw avoids this issue by adding such relations to the EXPLAIN  
using ExplainForeignScan as shown in the below example, but since such  
relations are essential, I think that information should be shown by  
core itself.


postgres=# explain select * from (select ft1.a from ft1 left join ft2 on  
ft1.a = ft2.a where ft1.b = 1) ss1(a) full join (select ft3.a from ft3  
left join ft4 on ft3.a = ft4.a where ft3.b = 1) ss2(a) on ss1.a = ss2.a;

   QUERY PLAN

 Hash Full Join  (cost=202.06..204.12 rows=1 width=8)
   Hash Cond: (ft1.a = ft3.a)
   ->  Foreign Scan  (cost=100.00..102.05 rows=1 width=4)
 Relations: (public.ft1) LEFT JOIN (public.ft2)
   ->  Hash  (cost=102.05..102.05 rows=1 width=4)
 ->  Foreign Scan  (cost=100.00..102.05 rows=1 width=4)
   Relations: (public.ft3) LEFT JOIN (public.ft4)
(7 rows)

From the Relations line shown by postgres_fdw, we can see which foreign  
join joins which foreign tables, but if no such lines, we couldn't.


I wrote:

Probably something like this:

   Foreign Processing
 Remote Operations: ...

In the Remote Operations line, the FDW/extension could print any info
about remote operations, eg, "Scan/Join + Aggregate".



"Foreign" implies this node is processed by FDW, but "Procesing" gives us
no extra information; seems to me redundant.


I intentionally chose that word and thought we could leave detailed  
descriptions about remote operations to the FDW/extension; a broader  
word like "Processing" seems to work well because we allow various kinds  
of operations to the remote side, in addition to scans/joins, to be  
performed in that one Foreign Scan node indicated by "Foreign  
Processing", such as aggregation, window functions, distinct, order by,  
row locking, table modification, or combinations of them.



Prior to the new invention, please explain why you don't want to by my
suggestion first? Annoying is a feel of you, but not a logic to persuade
others.


I'm not saying that the idea I proposed is better than your suggestion.  
 Just brain storming.  I want to know what options we have and the pros  
and cons of each approach.



postgres=# explain select id from t0 natural join t1 natural join t2;
QUERY PLAN
---
 Custom Scan (GpuJoin)  (cost=12385.67..291245.35 rows=9815465 width=4)
   GPU Projection: t0.id
   Depth 1: GpuHashJoin, HashKeys: (t0.bid)
JoinQuals: (t0.bid = t2.bid)
Nrows (in/out: 98.15%), KDS-Hash (size: 13.47MB, nbatches: 1)
   Depth 2: GpuHashJoin, HashKeys: (t0.aid)
JoinQuals: (t0.aid = t1.aid)
Nrows (in/out: 100.00%), KDS-Hash (size: 13.47MB, nbatches: 1)
   ->  Seq Scan on t0  (cost=0.00..18.96 rows=996 width=12)
   ->  Seq Scan on t2  (cost=0.00..1935.00 rows=10 width=4)
   ->  Seq Scan on t1  (cost=0.00..1935.00 rows=10 width=4)
(11 rows)



My largest concern for you proposition is, ForeignScan/CustomScan node is
enforced to print name of underlying relations, regardless of its actual
behavior. The above GpuJoin never scans tables at least, thus, it mislead
users if we have no choice to print underlying relation names.


OK, I understand we would need special handling for such custom joins.

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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Kouhei Kaigai]

> On 2016/07/28 10:01, Kouhei Kaigai wrote:
> > What I'm saying is here:
> 
> > EXPLAIN (COSTS false, VERBOSE)
> > SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY
> > t1.c3, t1.c1 OFFSET 100 LIMIT 10;
> >   QUERY PLAN
> > ---
> > Limit
> >   Output: t1.c1, t2.c1, t1.c3
> >   ->  Foreign Scan
> >   
> > Output: t1.c1, t2.c1, t1.c3
> > Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
> > Remote SQL: SELECT r1."C 1", r1.c3, r2."C 1" FROM ("S 1"."T
> > 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY
> > r1.c3 ASC N\
> > ULLS LAST, r1."C 1" ASC NULLS LAST
> > (6 rows)
> 
> > Your concern is that EXPLAIN shows ForeignScan node as "Foreign Scan"
> > although it actually works as "Foreign Join".
> 
> That may be so, but my point is that the target relations involved in
> the foreign join (ie, ft1 and ft2) should be printed somewhere in the
> EXPLAIN output by core, as in EXPLAIN for a simple foreign table scan.
>
Why? According to your rule, Hash Join should take "on t0,t1,t2".

postgres=# explain select id from t0 natural join t1 natural join t2;
 QUERY PLAN
-
 Hash Join  (cost=6370.00..4560826.24 rows=98784048 width=4)
   Hash Cond: (t0.aid = t1.aid)
   ->  Hash Join  (cost=3185.00..3199360.58 rows=98784048 width=8)
 Hash Cond: (t0.bid = t2.bid)
 ->  Seq Scan on t0  (cost=0.00..184.80 rows=10080 width=12)
 ->  Hash  (cost=1935.00..1935.00 rows=10 width=4)
   ->  Seq Scan on t2  (cost=0.00..1935.00 rows=10 width=4)
   ->  Hash  (cost=1935.00..1935.00 rows=10 width=4)
 ->  Seq Scan on t1  (cost=0.00..1935.00 rows=10 width=4)
(9 rows)

> > My suggestion makes EXPLAIN print "Foreign %s" according to the label
> > assigned by the extension. Only FDW driver knows how this ForeignScan
> > node actually performs on, thus, only FDW driver can set meaningful label.
> >
> > This label may be "Join", may be "Partial Aggregate + Join", or something
> > others according to the actual job of this node.
> 
> OK, thanks for that.  But sorry, I'm not sure that's a good idea.  One
> reason for that is that FDWs/extentions might give different labels to
> the same upper-planner action, which would lead to confusing EXPLAINs.
>
If extension put a label hard to understand, it is a problem of the extension.

> Another is that labeling might be annoying, so some FDWs/extensions may
> not want to do that.
>
I'm happy with arbitrary label, not annoying. :-)

> Couldn't we print EXPLAINs in a more unified way?  A simple idea is to
> introduce a broad label, eg, "Foreign Processing", as a unified label;
> if the FDW/extension performs any upper-planner actions remotely, then
> the label "Foreign Processing" will be printed by core, and following
> that, the FDW/extension would print what they want, using
> ExplainForeignScan/ExplainCustomScan.  Probably something like this:
> 
>Foreign Processing
>  Remote Operations: ...
> 
> In the Remote Operations line, the FDW/extension could print any info
> about remote operations, eg, "Scan/Join + Aggregate".  Different data
> sources have different concepts/terms, so it seems reasonable to me that
> there would be different descriptions by different data sources, to the
> same plan actions done remotely.
>
"Foreign" implies this node is processed by FDW, but "Procesing" gives us
no extra information; seems to me redundant.
Prior to the new invention, please explain why you don't want to by my
suggestion first? Annoying is a feel of you, but not a logic to persuade
others.

> >>>  - A flag to turn on/off printing relation(s) name
> >>
> >> ISTM that the relation information should be always printed even in the
> >> case of scanrelid=0 by the core, because that would be essential for
> >> analyzing EXPLAIN results.
> 
> > We have no information which relations are actually scanned by ForeignScan
> > and CustomScan. Your patch abuses fs_relids and custom_relids, however,
> > these fields don't indicate the relations actually scan by these nodes.
> > It just tracks relations processed by this node or its children.
> 
> Maybe my explanation was not enough, but I just mean that *joining
> relations* should be printed somewhere in the EXPLAIN output for a
> foreign/custom join as well, by core.
> 
> > postgres=# explain select id from t0 natural join t1 natural join t2;
> > QUERY PLAN
> > ---
> >  Custom Scan (GpuJoin)  (cost=12385.67..291245.35 rows=9815465 width=4)
> >GPU Projection: t0.id
> >Depth 1: GpuHashJoin, HashKeys: (t0.bid)
> > JoinQuals: (t0.bid = t2.bid)
> > Nrows (in/out: 98.15%), KDS-Hash (size: 

Re: [HACKERS] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/07/28 10:01, Kouhei Kaigai wrote:

What I'm saying is here:



EXPLAIN (COSTS false, VERBOSE)
SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY
t1.c3, t1.c1 OFFSET 100 LIMIT 10;
  QUERY PLAN
---
Limit
  Output: t1.c1, t2.c1, t1.c3
  ->  Foreign Scan
  
Output: t1.c1, t2.c1, t1.c3
Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
Remote SQL: SELECT r1."C 1", r1.c3, r2."C 1" FROM ("S 1"."T
1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY
r1.c3 ASC N\
ULLS LAST, r1."C 1" ASC NULLS LAST
(6 rows)



Your concern is that EXPLAIN shows ForeignScan node as "Foreign Scan"
although it actually works as "Foreign Join".


That may be so, but my point is that the target relations involved in  
the foreign join (ie, ft1 and ft2) should be printed somewhere in the  
EXPLAIN output by core, as in EXPLAIN for a simple foreign table scan.



My suggestion makes EXPLAIN print "Foreign %s" according to the label
assigned by the extension. Only FDW driver knows how this ForeignScan
node actually performs on, thus, only FDW driver can set meaningful label.

This label may be "Join", may be "Partial Aggregate + Join", or something
others according to the actual job of this node.


OK, thanks for that.  But sorry, I'm not sure that's a good idea.  One  
reason for that is that FDWs/extentions might give different labels to  
the same upper-planner action, which would lead to confusing EXPLAINs.  
Another is that labeling might be annoying, so some FDWs/extensions may  
not want to do that.


Couldn't we print EXPLAINs in a more unified way?  A simple idea is to  
introduce a broad label, eg, "Foreign Processing", as a unified label;  
if the FDW/extension performs any upper-planner actions remotely, then  
the label "Foreign Processing" will be printed by core, and following  
that, the FDW/extension would print what they want, using  
ExplainForeignScan/ExplainCustomScan.  Probably something like this:


  Foreign Processing
Remote Operations: ...

In the Remote Operations line, the FDW/extension could print any info  
about remote operations, eg, "Scan/Join + Aggregate".  Different data  
sources have different concepts/terms, so it seems reasonable to me that  
there would be different descriptions by different data sources, to the  
same plan actions done remotely.



 - A flag to turn on/off printing relation(s) name


ISTM that the relation information should be always printed even in the
case of scanrelid=0 by the core, because that would be essential for
analyzing EXPLAIN results.



We have no information which relations are actually scanned by ForeignScan
and CustomScan. Your patch abuses fs_relids and custom_relids, however,
these fields don't indicate the relations actually scan by these nodes.
It just tracks relations processed by this node or its children.


Maybe my explanation was not enough, but I just mean that *joining  
relations* should be printed somewhere in the EXPLAIN output for a  
foreign/custom join as well, by core.



postgres=# explain select id from t0 natural join t1 natural join t2;
QUERY PLAN
---
 Custom Scan (GpuJoin)  (cost=12385.67..291245.35 rows=9815465 width=4)
   GPU Projection: t0.id
   Depth 1: GpuHashJoin, HashKeys: (t0.bid)
JoinQuals: (t0.bid = t2.bid)
Nrows (in/out: 98.15%), KDS-Hash (size: 13.47MB, nbatches: 1)
   Depth 2: GpuHashJoin, HashKeys: (t0.aid)
JoinQuals: (t0.aid = t1.aid)
Nrows (in/out: 100.00%), KDS-Hash (size: 13.47MB, nbatches: 1)
   ->  Seq Scan on t0  (cost=0.00..18.96 rows=996 width=12)
   ->  Seq Scan on t2  (cost=0.00..1935.00 rows=10 width=4)
   ->  Seq Scan on t1  (cost=0.00..1935.00 rows=10 width=4)
(11 rows)



If EXPLAIN command attaches something like "on t0,t1,t2" after the GpuJoin
*with no choice*, it is problematic and misleading.
It shall be controllable by the extension which knows what tables are
actually scanned. (Please note that I never says extension makes the string.
It is a job of core explain. I suggest it needs to be controllable.)


Again, I just mean that the info "on t0,t1,t2" should be printed after  
the GpuJoin label, as its joining relations.


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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Kouhei Kaigai]

> On 2016/07/27 13:51, Kouhei Kaigai wrote:
> > This output is, at least, not incorrect.
> > This ForeignScan actually scan a relation that consists of two joined
> > tables on the remote side. So, not incorrect, but may not convenient for
> > better understanding by users who don't have deep internal knowledge.
> 
> Hmm.
> 
> > On the other hands, I cannot be happy with your patch because it concludes
> > the role of ForeignScan/CustomScan with scanrelid==0 is always join.
> > However, it does not cover all the case.
> >
> > When postgres_fdw gets support of remote partial aggregate, we can implement
> > the feature using ForeignScan with scanrelid==0. Is it a join? No.
> 
> Yeah, I think that that could be implemented in both cases (scanrelid>0
> and scanrelid=0).
> 
> > Probably, the core cannot know exact purpose of ForeignScan/CustomScan with
> > scanrelid==0. It can be a remote partial aggregation. It can be an 
> > alternative
> > sort logic by GPU. It can be something others.
> > So, I think extension needs to inform the core more proper label to show;
> > including a flag to control whether the relation(s) name shall be attached
> > next to the ForeignScan/CustomScan line.
> >
> > I'd like you to suggest to add two fields below:
> >  - An alternative label extension wants to show instead of the default one
> 
> How about adding something like the "Additional Operations" line as
> proposed in a previous email, instead?  As you know, FDWs/Extensions
> could add such information through ExplainForeignScan/ExplainCustomScan.
>
No. What I'm saying is here:


EXPLAIN (COSTS false, VERBOSE)
SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY  
t1.c3, t1.c1 OFFSET 100 LIMIT 10;
  QUERY PLAN   
---
Limit
  Output: t1.c1, t2.c1, t1.c3
  ->  Foreign Scan
  
Output: t1.c1, t2.c1, t1.c3
Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
Remote SQL: SELECT r1."C 1", r1.c3, r2."C 1" FROM ("S 1"."T  
1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY  
r1.c3 ASC N\
ULLS LAST, r1."C 1" ASC NULLS LAST
(6 rows)


Your concern is that EXPLAIN shows ForeignScan node as "Foreign Scan"
although it actually works as "Foreign Join".

My suggestion makes EXPLAIN print "Foreign %s" according to the label 
assigned by the extension. Only FDW driver knows how this ForeignScan
node actually performs on, thus, only FDW driver can set meaningful label.

This label may be "Join", may be "Partial Aggregate + Join", or something
others according to the actual job of this node.

> >  - A flag to turn on/off printing relation(s) name
> 
> ISTM that the relation information should be always printed even in the
> case of scanrelid=0 by the core, because that would be essential for
> analyzing EXPLAIN results.
>
We have no information which relations are actually scanned by ForeignScan
and CustomScan. Your patch abuses fs_relids and custom_relids, however,
these fields don't indicate the relations actually scan by these nodes.
It just tracks relations processed by this node or its children.

See the example below. This GpuJoin on CustomScan takes three SeqScan nodes
as inner/outer source of relations joins. GpuJoin never scans the tables
actually. It picks up the records generated by underlying SeqScan nodes.


postgres=# explain select id from t0 natural join t1 natural join t2;
QUERY PLAN
---
 Custom Scan (GpuJoin)  (cost=12385.67..291245.35 rows=9815465 width=4)
   GPU Projection: t0.id
   Depth 1: GpuHashJoin, HashKeys: (t0.bid)
JoinQuals: (t0.bid = t2.bid)
Nrows (in/out: 98.15%), KDS-Hash (size: 13.47MB, nbatches: 1)
   Depth 2: GpuHashJoin, HashKeys: (t0.aid)
JoinQuals: (t0.aid = t1.aid)
Nrows (in/out: 100.00%), KDS-Hash (size: 13.47MB, nbatches: 1)
   ->  Seq Scan on t0  (cost=0.00..18.96 rows=996 width=12)
   ->  Seq Scan on t2  (cost=0.00..1935.00 rows=10 width=4)
   ->  Seq Scan on t1  (cost=0.00..1935.00 rows=10 width=4)
(11 rows)


If EXPLAIN command attaches something like "on t0,t1,t2" after the GpuJoin
*with no choice*, it is problematic and misleading.
It shall be controllable by the extension which knows what tables are
actually scanned. (Please note that I never says extension makes the string.
It is a job of core explain. I suggest it needs to be controllable.)


Even though I recommended a boolean flag to turn on/off this print, it may
need to be a bitmap to indicate which underlying relations should be printed
by the explain command. Because we can easily assume ForeignScan/CustomScan
node that scans only a part of child tables. For example, it is available to
implement GpuJoin scans the t1 and t2 by itself but takes SeqScan on t0.

> >  - A flag to turn on/off printing 

Re: [HACKERS] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/07/27 13:51, Kouhei Kaigai wrote:

This output is, at least, not incorrect.
This ForeignScan actually scan a relation that consists of two joined
tables on the remote side. So, not incorrect, but may not convenient for
better understanding by users who don't have deep internal knowledge.


Hmm.


On the other hands, I cannot be happy with your patch because it concludes
the role of ForeignScan/CustomScan with scanrelid==0 is always join.
However, it does not cover all the case.

When postgres_fdw gets support of remote partial aggregate, we can implement
the feature using ForeignScan with scanrelid==0. Is it a join? No.


Yeah, I think that that could be implemented in both cases (scanrelid>0  
and scanrelid=0).



Probably, the core cannot know exact purpose of ForeignScan/CustomScan with
scanrelid==0. It can be a remote partial aggregation. It can be an alternative
sort logic by GPU. It can be something others.
So, I think extension needs to inform the core more proper label to show;
including a flag to control whether the relation(s) name shall be attached
next to the ForeignScan/CustomScan line.

I'd like you to suggest to add two fields below:
 - An alternative label extension wants to show instead of the default one


How about adding something like the "Additional Operations" line as  
proposed in a previous email, instead?  As you know, FDWs/Extensions  
could add such information through ExplainForeignScan/ExplainCustomScan.



 - A flag to turn on/off printing relation(s) name


ISTM that the relation information should be always printed even in the  
case of scanrelid=0 by the core, because that would be essential for  
analyzing EXPLAIN results.


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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Etsuro Fujita]

On 2016/07/27 13:09, Ashutosh Bapat wrote:

The patch always prints ForeignJoin when scanrelid <= 0, which would be
odd considering that FDWs can now push down post-join operations. We
need to device a better way to convey post-join operations. May be
something like
Foreign Grouping, aggregation on ...
Foreign Join on ...


Good point!

The point of the proposed patch is to print "Foreign Join"/"Custom 
Join", to show, in every text/xml/json/yaml format, that there are 
multiple target relations involved in that join and to print those 
relations, following the "Foreign Join"/"Custom Join" words, which I 
think would be more machine-readable.



But then the question is a foreign scan node can be pushing down many
operations together e.g. join, aggregation, sort OR join aggregation and
windowing OR join and insert. How would we clearly convey this? May be
we say
Foreign Scan
operations: join on ..., aggregation, ...

That wouldn't be so great and might be clumsy for many operations. Any
better idea?


How about: when doing post scan/join operations remotely, print such 
additional operations in EXPLAIN the same way as in the "Relations" or 
"Remote SQL" info printed by postgres_fdw.  Maybe something like this:


  Foreign Scan/Join on target relation(s)
Output: ...
Relations: ...
Additional Operations: grouping, aggregate, ...
Remote SQL: ...

That seems not that clumsy to me.

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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Kouhei Kaigai]

> I noticed that currently the core doesn't show any information on the
> target relations involved in a foreign/custom join in EXPLAIN, by
> itself.  Here is an example:
> 
> -- join two tables
> EXPLAIN (COSTS false, VERBOSE)
> SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY
> t1.c3, t1.c1 OFFSET 100 LIMIT 10;
>   QUERY PLAN   \
> 
> --
> -\
> ---
> Limit
>   Output: t1.c1, t2.c1, t1.c3
>   ->  Foreign Scan
> Output: t1.c1, t2.c1, t1.c3
> Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
> Remote SQL: SELECT r1."C 1", r1.c3, r2."C 1" FROM ("S 1"."T
> 1" r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY
> r1.c3 ASC N\
> ULLS LAST, r1."C 1" ASC NULLS LAST
> (6 rows)
> 
> postgres_fdw shows the target relations in the Relations line, as shown
> above, but I think that the core should show such information
> independently of FDWs; in the above example replace "Foreign Scan" with
> "Foreign Join on public.ft1 t1, public.ft2 t2".  Please find attached a
> patch for that.  Comments welcome!
>
This output is, at least, not incorrect.
This ForeignScan actually scan a relation that consists of two joined
tables on the remote side. So, not incorrect, but may not convenient for
better understanding by users who don't have deep internal knowledge.

On the other hands, I cannot be happy with your patch because it concludes
the role of ForeignScan/CustomScan with scanrelid==0 is always join.
However, it does not cover all the case.

When postgres_fdw gets support of remote partial aggregate, we can implement
the feature using ForeignScan with scanrelid==0. Is it a join? No.

Probably, the core cannot know exact purpose of ForeignScan/CustomScan with
scanrelid==0. It can be a remote partial aggregation. It can be an alternative
sort logic by GPU. It can be something others.
So, I think extension needs to inform the core more proper label to show;
including a flag to control whether the relation(s) name shall be attached
next to the ForeignScan/CustomScan line.

I'd like you to suggest to add two fields below:
 - An alternative label extension wants to show instead of the default one
 - A flag to turn on/off printing relation(s) name

EXPLAIN can print proper information according to these requirements.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


-- 
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] Oddity in EXPLAIN for foreign/custom join pushdown plans

[Ashutosh Bapat]

On Wed, Jul 27, 2016 at 8:50 AM, Etsuro Fujita 
wrote:

> Hi,
>
> I noticed that currently the core doesn't show any information on the
> target relations involved in a foreign/custom join in EXPLAIN, by itself.
> Here is an example:
>
> -- join two tables
> EXPLAIN (COSTS false, VERBOSE)
> SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY
> t1.c3, t1.c1 OFFSET 100 LIMIT 10;
>  QUERY PLAN   \
>
>
> ---\
> ---
>Limit
>  Output: t1.c1, t2.c1, t1.c3
>  ->  Foreign Scan
>Output: t1.c1, t2.c1, t1.c3
>Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
>Remote SQL: SELECT r1."C 1", r1.c3, r2."C 1" FROM ("S 1"."T 1"
> r1 INNER JOIN "S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY r1.c3
> ASC N\
> ULLS LAST, r1."C 1" ASC NULLS LAST
> (6 rows)
>
> postgres_fdw shows the target relations in the Relations line, as shown
> above, but I think that the core should show such information independently
> of FDWs; in the above example replace "Foreign Scan" with "Foreign Join on
> public.ft1 t1, public.ft2 t2".  Please find attached a patch for that.
> Comments welcome!
>


The patch always prints ForeignJoin when scanrelid <= 0, which would be odd
considering that FDWs can now push down post-join operations. We need to
device a better way to convey post-join operations. May be something like
Foreign Grouping, aggregation on ...
Foreign Join on ...

But then the question is a foreign scan node can be pushing down many
operations together e.g. join, aggregation, sort OR join aggregation and
windowing OR join and insert. How would we clearly convey this? May be we
say
Foreign Scan
operations: join on ..., aggregation, ...

That wouldn't be so great and might be clumsy for many operations. Any
better idea?
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] Oddity in handling of cached plans for FDW queries

[Tom Lane]

Etsuro Fujita  writes:
> On 2016/07/21 16:30, Etsuro Fujita wrote:
>> One thing I'd like to discuss is GetUserMappingById/GetUserMappingId.
>> Though those functions aren't used in any places, I didn't take them
>> out, because I thought somebody else would need them someday.  But
>> considering that user mapping OIDs now aren't available in RelOptInfos,
>> at least the former might be rather useless.  Should we keep them in core?

> I thought a bit more about this.  ISTM that there isn't much value in  
> the latter either, because we can use GetUserMapping and get that OID  
> from the result, instead of using the latter, so I'd vote for removing  
> those functions.  Attached is a patch for that.

Agreed - pushed.

regards, tom lane


-- 
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] Oddity in handling of cached plans for FDW queries

[Etsuro Fujita]

On 2016/07/21 16:30, Etsuro Fujita wrote:

One thing I'd like to discuss is GetUserMappingById/GetUserMappingId.
Though those functions aren't used in any places, I didn't take them
out, because I thought somebody else would need them someday.  But
considering that user mapping OIDs now aren't available in RelOptInfos,
at least the former might be rather useless.  Should we keep them in core?


I thought a bit more about this.  ISTM that there isn't much value in  
the latter either, because we can use GetUserMapping and get that OID  
from the result, instead of using the latter, so I'd vote for removing  
those functions.  Attached is a patch for that.


Best regards,
Etsuro Fujita


remove-usermapping-helper-funcs.patch
Description: binary/octet-stream

-- 
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] Oddity in handling of cached plans for FDW queries

[Etsuro Fujita]

On 2016/07/16 6:25, Tom Lane wrote:

Pushed, after fooling around with it some more so that it would cover the
case we discussed where the join could be allowed if we restrict the plan
to be executed by the owner of a view used in the query.


I left that for future work, but I'm happy to have that in 9.6.  Thanks  
for improving and committing the patch!


One thing I'd like to discuss is GetUserMappingById/GetUserMappingId.  
Though those functions aren't used in any places, I didn't take them  
out, because I thought somebody else would need them someday.  But  
considering that user mapping OIDs now aren't available in RelOptInfos,  
at least the former might be rather useless.  Should we keep them in core?


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] Oddity in handling of cached plans for FDW queries

[Robert Haas]

On Wed, Jul 20, 2016 at 10:15 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> Mumble.  Why, exactly, was this a good idea?  The upside of commit
>> 45639a0525a58a2700cf46d4c934d6de78349dac is only that you do fewer
>> plan invalidations, but surely that's not a significant benefit for
>> most people: user mappings don't change that often.  On the downside,
>> there are now cases where joins would have gotten pushed down
>> previously and now they won't.  In other words, you've saved some
>> replanning activity at the cost of delivering worse plans.  That seems
>> pretty suspect to me, although I grant that the scenarios where either
>> the old or the new behavior is actually a problem are all somewhat off
>> the beaten path.
>
> I think that you are undervaluing the removal of user-mapping-based plan
> invalidation.  That was never more than a kluge, and here is the reason:
> we have no way to lock user mappings.  The system whereby we invalidate
> plans as a consequence of table DDL changes is bulletproof, because we
> (re) acquire locks on the tables used in the plan, then check for
> invalidation signals, before deciding whether the plan is stale.  The
> corresponding scenario where a user mapping changes between that check
> and execution time is unprotected, so that we could end up using a plan
> that is insecure for the mappings selected at execution.

OK, that's a fair point.  Thanks for explaining.

> Another way we could have removed the race condition is the suggestion
> made upthread of embedding the user mapping details right into the plan
> instead of looking them up afresh at execution.  But I didn't much like
> that approach, per upthread discussion.

OK.

-- 
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] Oddity in handling of cached plans for FDW queries

[Tom Lane]

Robert Haas  writes:
> Mumble.  Why, exactly, was this a good idea?  The upside of commit
> 45639a0525a58a2700cf46d4c934d6de78349dac is only that you do fewer
> plan invalidations, but surely that's not a significant benefit for
> most people: user mappings don't change that often.  On the downside,
> there are now cases where joins would have gotten pushed down
> previously and now they won't.  In other words, you've saved some
> replanning activity at the cost of delivering worse plans.  That seems
> pretty suspect to me, although I grant that the scenarios where either
> the old or the new behavior is actually a problem are all somewhat off
> the beaten path.

I think that you are undervaluing the removal of user-mapping-based plan
invalidation.  That was never more than a kluge, and here is the reason:
we have no way to lock user mappings.  The system whereby we invalidate
plans as a consequence of table DDL changes is bulletproof, because we
(re) acquire locks on the tables used in the plan, then check for
invalidation signals, before deciding whether the plan is stale.  The
corresponding scenario where a user mapping changes between that check
and execution time is unprotected, so that we could end up using a plan
that is insecure for the mappings selected at execution.

Another way we could have removed the race condition is the suggestion
made upthread of embedding the user mapping details right into the plan
instead of looking them up afresh at execution.  But I didn't much like
that approach, per upthread discussion.

regards, tom lane


-- 
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] Oddity in handling of cached plans for FDW queries

[Robert Haas]

On Fri, Jul 15, 2016 at 5:25 PM, Tom Lane  wrote:
> I wrote:
>> Etsuro Fujita  writes:
>>> Here is a patch for that redesign proposed by you; reverts commits
>>> fbe5a3fb73102c2cfec114a67943f4474383 and
>>> 5d4171d1c70edfe3e9be1de9e66603af28e3afe1, adds changes for that redesign
>>> to the core, and adjusts the postgres_fdw code to that changes.  Also, I
>>> rearranged the postgres_fdw regression tests to match that changes.
>
>> OK, I'll review this later today.
>
> Pushed, after fooling around with it some more so that it would cover the
> case we discussed where the join could be allowed if we restrict the plan
> to be executed by the owner of a view used in the query.

Mumble.  Why, exactly, was this a good idea?  The upside of commit
45639a0525a58a2700cf46d4c934d6de78349dac is only that you do fewer
plan invalidations, but surely that's not a significant benefit for
most people: user mappings don't change that often.  On the downside,
there are now cases where joins would have gotten pushed down
previously and now they won't.  In other words, you've saved some
replanning activity at the cost of delivering worse plans.  That seems
pretty suspect to me, although I grant that the scenarios where either
the old or the new behavior is actually a problem are all somewhat off
the beaten path.

-- 
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] Oddity in handling of cached plans for FDW queries

[Tom Lane]

I wrote:
> Etsuro Fujita  writes:
>> Here is a patch for that redesign proposed by you; reverts commits  
>> fbe5a3fb73102c2cfec114a67943f4474383 and  
>> 5d4171d1c70edfe3e9be1de9e66603af28e3afe1, adds changes for that redesign  
>> to the core, and adjusts the postgres_fdw code to that changes.  Also, I  
>> rearranged the postgres_fdw regression tests to match that changes.

> OK, I'll review this later today.

Pushed, after fooling around with it some more so that it would cover the
case we discussed where the join could be allowed if we restrict the plan
to be executed by the owner of a view used in the query.

regards, tom lane


-- 
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] Oddity in handling of cached plans for FDW queries

[Tom Lane]

Etsuro Fujita  writes:
> On 2016/07/15 11:48, Tom Lane wrote:
>> If we add a mechanism to let us know that the FDW doesn't care, we could
>> relax the requirement for such cases.  I don't have a strong opinion on
>> whether that's worthwhile.  It'd depend in part on how many FDWs there
>> are that don't care, versus those that do; and I have no idea about that.

> So, I'd vote for leaving that for future work if necessary.

Makes sense to me.

> Here is a patch for that redesign proposed by you; reverts commits  
> fbe5a3fb73102c2cfec114a67943f4474383 and  
> 5d4171d1c70edfe3e9be1de9e66603af28e3afe1, adds changes for that redesign  
> to the core, and adjusts the postgres_fdw code to that changes.  Also, I  
> rearranged the postgres_fdw regression tests to match that changes.

OK, I'll review this later today.

regards, tom lane


-- 
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] Oddity in handling of cached plans for FDW queries

[Etsuro Fujita]

On 2016/07/15 11:48, Tom Lane wrote:

Etsuro Fujita  writes:

One thing I'm not sure about is: should we insist that a join can be
pushed down only if the checkAsUser fields of the relevant RTEs are
equal in the case where user mappings are meaningless to the FDW, like
file_fdw?



If we add a mechanism to let us know that the FDW doesn't care, we could
relax the requirement for such cases.  I don't have a strong opinion on
whether that's worthwhile.  It'd depend in part on how many FDWs there
are that don't care, versus those that do; and I have no idea about that.


So, I'd vote for leaving that for future work if necessary.

Here is a patch for that redesign proposed by you; reverts commits  
fbe5a3fb73102c2cfec114a67943f4474383 and  
5d4171d1c70edfe3e9be1de9e66603af28e3afe1, adds changes for that redesign  
to the core, and adjusts the postgres_fdw code to that changes.  Also, I  
rearranged the postgres_fdw regression tests to match that changes.


Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 2053,2207  SELECT t1."C 1" FROM "S 1"."T 1" t1, LATERAL (SELECT DISTINCT t2.c1, t3.c1 FROM
 1
  (10 rows)
  
- -- create another user for permission, user mapping, effective user tests
- CREATE USER view_owner;
- -- grant privileges on ft4 and ft5 to view_owner
- GRANT ALL ON ft4 TO view_owner;
- GRANT ALL ON ft5 TO view_owner;
- -- prepare statement with current session user
- PREPARE join_stmt AS SELECT t1.c1, t2.c1 FROM ft4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1 OFFSET 10 LIMIT 10;
- EXPLAIN (COSTS OFF, VERBOSE) EXECUTE join_stmt;
- QUERY PLAN 
- ---
-  Limit
-Output: t1.c1, t2.c1
-->  Foreign Scan
-  Output: t1.c1, t2.c1
-  Relations: (public.ft4 t1) LEFT JOIN (public.ft5 t2)
-  Remote SQL: SELECT r1.c1, r2.c1 FROM ("S 1"."T 3" r1 LEFT JOIN "S 1"."T 4" r2 ON (((r1.c1 = r2.c1 ORDER BY r1.c1 ASC NULLS LAST, r2.c1 ASC NULLS LAST
- (6 rows)
- 
- EXECUTE join_stmt;
-  c1 | c1 
- +
-  22 |   
-  24 | 24
-  26 |   
-  28 |   
-  30 | 30
-  32 |   
-  34 |   
-  36 | 36
-  38 |   
-  40 |   
- (10 rows)
- 
- -- change the session user to view_owner and execute the statement. Because of
- -- change in session user, the plan should get invalidated and created again.
- -- The join will not be pushed down since the joining relations do not have a
- -- valid user mapping.
- SET SESSION ROLE view_owner;
- EXPLAIN (COSTS OFF, VERBOSE) EXECUTE join_stmt;
- QUERY PLAN
- --
-  Limit
-Output: t1.c1, t2.c1
-->  Sort
-  Output: t1.c1, t2.c1
-  Sort Key: t1.c1, t2.c1
-  ->  Hash Left Join
-Output: t1.c1, t2.c1
-Hash Cond: (t1.c1 = t2.c1)
-->  Foreign Scan on public.ft4 t1
-  Output: t1.c1, t1.c2, t1.c3
-  Remote SQL: SELECT c1 FROM "S 1"."T 3"
-->  Hash
-  Output: t2.c1
-  ->  Foreign Scan on public.ft5 t2
-Output: t2.c1
-Remote SQL: SELECT c1 FROM "S 1"."T 4"
- (16 rows)
- 
- RESET ROLE;
- DEALLOCATE join_stmt;
- CREATE VIEW v_ft5 AS SELECT * FROM ft5;
- -- change owner of v_ft5 to view_owner so that the effective user for scan on
- -- ft5 is view_owner and not the current user.
- ALTER VIEW v_ft5 OWNER TO view_owner;
- -- create a public user mapping for loopback server
- -- drop user mapping for current_user.
- DROP USER MAPPING FOR CURRENT_USER SERVER loopback;
- CREATE USER MAPPING FOR PUBLIC SERVER loopback;
- -- different effective user for permission check, but same user mapping for the
- -- joining sides, join pushed down, no result expected.
- PREPARE join_stmt AS SELECT t1.c1, t2.c1 FROM ft5 t1 JOIN v_ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1 OFFSET 100 LIMIT 10;
- EXPLAIN (COSTS false, VERBOSE) EXECUTE join_stmt;
-   QUERY PLAN  
- --
-  Limit
-Output: t1.c1, ft5.c1
-->  Foreign Scan
-  Output: t1.c1, ft5.c1
-  Relations: (public.ft5 t1) INNER JOIN (public.ft5)
-  Remote SQL: SELECT r1.c1, r6.c1 FROM ("S 1"."T 4" r1 INNER JOIN "S 1"."T 4" r6 ON (((r1.c1 

Re: [HACKERS] Oddity in handling of cached plans for FDW queries

[Ashutosh Bapat]

On Fri, Jul 15, 2016 at 12:19 AM, Tom Lane  wrote:

> I wrote:
> > I concur with Etsuro-san's dislike for hasForeignJoin; that flag is
> > underspecified and doesn't convey nearly enough information.  I do not
> > think a uses_user_mapping flag is much better.  ISTM what should happen
> is
> > that any time we decide to push down a foreign join, we should record the
> > identity of the common user mapping that made that pushdown possible in
> > the plan's invalItems list.  That would make it possible to invalidate
> > only the relevant plans when a user mapping is changed.
>
> I thought a bit more about this and realized that the above doesn't work
> too well.  Initially, a join might have been pushed down on the strength
> of both userids mapping to a PUBLIC user mapping for the server.  If now
> someone does CREATE USER MAPPING to install a new mapping for one of
> those userids, we should invalidate the plan --- but there is certainly
> not going to be anything in the plan matching the new user mapping.
>

I replied to your earlier mail before reading this. Ok, so we agree there.


>
> > Another way we could attack it would be to record the foreign server OID
> > as an invalItem for any query that has more than one foreign table
> > belonging to the same foreign server.  Then, invalidate whenever any user
> > mapping for that server changes.
>
> And that doesn't work so well either, because the most that the plan inval
> code is going to have its hands on is (a hash of) the OID of the user
> mapping that changed.  We can't tell which server that's for.
>

I assumed that there is a way to get server's oid from user mapping or we
record it to be passed to the invalidation logic. Looks like there's no
easy way to do that.


>
> On reflection, it seems to me that we've gone wrong by tying planning to
> equality of user mappings at all, and the best way to get out of this is
> to not do that.  Instead, let's insist that a join can be pushed down only
> if the checkAsUser fields of the relevant RTEs are equal.  If they are,
> then the same user mapping must apply to both at runtime, whatever it is
> --- and we don't need to predetermine that.  With this approach, the need
> for plan invalidation due to user mapping changes goes away entirely.
>

I have already explained in my earlier mail, that the problem you described
doesn't exist. With the invalidation logic we are able to also support
pushing down joins between table with different effective user.


>
> This doesn't cost us anything at all in simple cases such as direct
> execution of a query, because all the checkAsUser fields will be equal
> (i.e., zero).  And it also doesn't hurt if the potential foreign join is
> encapsulated in a view, where all the checkAsUser fields would contain
> the OID of the view owner.
>

> The situation where we potentially lose something is a case like
> Etsuro-san's original example, where the query contains one foreign table
> reference coming from a view and one coming from the outer query, or maybe
> from a different view.  In the two-views case we would have to not push
> down the join if the views have different owners, even though perhaps both
> owners will use the PUBLIC mapping at runtime.  I think that's a narrow
> enough case that we can just live with not optimizing it.  In the
> view-and-outer-query case, the simplest answer is that we can't push down
> because zero is not equal to the view owner's OID.  We could make that a
> little better if we know that the query will be executed as the view
> owner, so that the relevant user IDs will be the same at runtime.  There
> is already some mechanism in the plan cache to track whether a plan
> depends on the identity of the user running it (for RLS), so we could use
> that to enforce that a plan containing such a pushed-down join is only run
> by the same user that owns the view.
>
>
Join between views on foreign tables or between foreign tables and views
containing foreign tables won't be rare. This feature is yet to be
released, so we don't know if PostgreSQL users would find it useful. But I
do see Oracle users joining views on dblink tables. I would guess same
would be the case in PostgreSQL. But I would like to hear from other
PostgreSQL FDW users. In such cases, being able to push down a join between
foreign tables across view boundaries will be useful.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] Oddity in handling of cached plans for FDW queries

[Ashutosh Bapat]

On Thu, Jul 14, 2016 at 10:02 PM, Tom Lane  wrote:

> Ashutosh Bapat  writes:
> > Exactly, for a rare scenario, should we be penalizing large number of
> plans
> > or just continue to use a previously prepared plan when an optimal plan
> has
> > become available because of changed condition. I would choose second over
> > the first as it doesn't make things worse than they are.
>
> You seem to be arguing from a premise that the worst consequence is use of
> an inefficient plan.  This is false; the worst consequence is use of a
> *wrong* plan, specifically one where a join has been pushed down but doing
> so is no longer legal because of a user mapping change.  That is, it was
> previously correct to access the two remote tables under the same remote
> userID but now they should be accessed under different userIDs.  The
> result will be that one or the other remote table is accessed under a
> userID that is not what the current user mappings say should be used.
> That could lead to either unexpected permissions failures (if the
> actually-used userID lacks needed permissions) or security breakdowns
> (if the actually-used userID has permissions to do something but the
> query should not have been allowed to do it).
>

> I do not think fixing this is optional.
>

There is confusion here. The current situation is this:
If at the time of preparing a statement a join can be pushed down to
foreign server, we mark that plan as hasForeignJoin. Before execution, if
user mapping changes (add/modify/drop), all plans with hasForeignJoin are
invalidated. This means the situation you are describing above does not
exist in the current code. So, nothing needs to be fixed.


>
> I concur with Etsuro-san's dislike for hasForeignJoin; that flag is
> underspecified and doesn't convey nearly enough information.  I do not
> think a uses_user_mapping flag is much better.  ISTM what should happen is
> that any time we decide to push down a foreign join, we should record the
> identity of the common user mapping that made that pushdown possible in
> the plan's invalItems list.  That would make it possible to invalidate
> only the relevant plans when a user mapping is changed.
>

This, as you have proposed, does not solve the problem either. Consider a
case when a public user mapping was associated with the foreign tables
being joined and the join was pushed down while preparing statement and
plan. Before execution, a user mapping was added which changed one of the
table's associated user mapping from public to the new one. Now the join is
not pushable, but the user mapping that was added/changed was not recorded
in invalItems, which contains the public user mapping. An improvement to
your proposal would be to invalidate plans related to a public user
mapping, when any user mapping is added/changed/dropped. But I guess, there
are also some problems there too. Adding/dropping/changing a user mapping
affects other user mappings as well, unlike say doing that to tables or
views. When implementing this, we debated whether it's worth to add that
much complexity. We favoured not adding the complexity that time. The code
as is not optimistic and might lead to using already created suboptimal
plans, but it doesn't have any known bugs there (assuming, I have explained
why the above situation doesn't lead to a bug).


>
> I believe what Ashutosh is focusing on is whether, when some user mapping
> changes, we should invalidate all plans that could potentially now use a
> pushed-down join but did not before.  I tend to agree that that's probably
> not something we want to do, as it would be very hard to identify just the
> plans likely to benefit.  So we would cause replanning of a lot of queries
> that won't actually benefit, and in this direction it is indeed only an
> optimization not a correctness matter.
>

Right, that's my argument.


>
> Another way we could attack it would be to record the foreign server OID
> as an invalItem for any query that has more than one foreign table
> belonging to the same foreign server.  Then, invalidate whenever any user
> mapping for that server changes.  This would take care of both the case
> where a join pushdown becomes possible and where it stops becoming
> possible.  But I'm not sure that the extra invalidations would be
> worthwhile.
>

Yes. That wouldn't have the problem I described above. Again, I am not sure
whether it's worth making code complex for that case. User mappings are not
something added/dropped/modified very frequently.


>
> I'm not excited about Etsuro-san's proposal to record user mapping info
> in the plan and skip execution-time mapping lookups altogether.  I think
> (1) that's solving a problem that hasn't been proven to be a problem,
> (2) it doesn't help unless the FDW code is changed to take advantage of
> it, which is unlikely to happen for third-party FDWs, and (3) it opens
> the door to a class of new bugs, as any 

Re: [HACKERS] Oddity in handling of cached plans for FDW queries

[Tom Lane]

Etsuro Fujita  writes:
> One thing I'm not sure about is: should we insist that a join can be  
> pushed down only if the checkAsUser fields of the relevant RTEs are  
> equal in the case where user mappings are meaningless to the FDW, like  
> file_fdw?

If we add a mechanism to let us know that the FDW doesn't care, we could
relax the requirement for such cases.  I don't have a strong opinion on
whether that's worthwhile.  It'd depend in part on how many FDWs there
are that don't care, versus those that do; and I have no idea about that.

regards, tom lane


-- 
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] Oddity in handling of cached plans for FDW queries

[Etsuro Fujita]

On 2016/07/15 3:49, Tom Lane wrote:

On reflection, it seems to me that we've gone wrong by tying planning to
equality of user mappings at all, and the best way to get out of this is
to not do that.  Instead, let's insist that a join can be pushed down only
if the checkAsUser fields of the relevant RTEs are equal.  If they are,
then the same user mapping must apply to both at runtime, whatever it is
--- and we don't need to predetermine that.  With this approach, the need
for plan invalidation due to user mapping changes goes away entirely.


+1


The situation where we potentially lose something is a case like
Etsuro-san's original example, where the query contains one foreign table
reference coming from a view and one coming from the outer query, or maybe
from a different view.  In the two-views case we would have to not push
down the join if the views have different owners, even though perhaps both
owners will use the PUBLIC mapping at runtime.  I think that's a narrow
enough case that we can just live with not optimizing it.  In the
view-and-outer-query case, the simplest answer is that we can't push down
because zero is not equal to the view owner's OID.  We could make that a
little better if we know that the query will be executed as the view
owner, so that the relevant user IDs will be the same at runtime.  There
is already some mechanism in the plan cache to track whether a plan
depends on the identity of the user running it (for RLS), so we could use
that to enforce that a plan containing such a pushed-down join is only run
by the same user that owns the view.


Seems reasonable to me.

One thing I'm not sure about is: should we insist that a join can be  
pushed down only if the checkAsUser fields of the relevant RTEs are  
equal in the case where user mappings are meaningless to the FDW, like  
file_fdw?


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] Oddity in handling of cached plans for FDW queries

[Tom Lane]

I wrote:
> I concur with Etsuro-san's dislike for hasForeignJoin; that flag is
> underspecified and doesn't convey nearly enough information.  I do not
> think a uses_user_mapping flag is much better.  ISTM what should happen is
> that any time we decide to push down a foreign join, we should record the
> identity of the common user mapping that made that pushdown possible in
> the plan's invalItems list.  That would make it possible to invalidate
> only the relevant plans when a user mapping is changed.

I thought a bit more about this and realized that the above doesn't work
too well.  Initially, a join might have been pushed down on the strength
of both userids mapping to a PUBLIC user mapping for the server.  If now
someone does CREATE USER MAPPING to install a new mapping for one of
those userids, we should invalidate the plan --- but there is certainly
not going to be anything in the plan matching the new user mapping.

> Another way we could attack it would be to record the foreign server OID
> as an invalItem for any query that has more than one foreign table
> belonging to the same foreign server.  Then, invalidate whenever any user
> mapping for that server changes.

And that doesn't work so well either, because the most that the plan inval
code is going to have its hands on is (a hash of) the OID of the user
mapping that changed.  We can't tell which server that's for.

On reflection, it seems to me that we've gone wrong by tying planning to
equality of user mappings at all, and the best way to get out of this is
to not do that.  Instead, let's insist that a join can be pushed down only
if the checkAsUser fields of the relevant RTEs are equal.  If they are,
then the same user mapping must apply to both at runtime, whatever it is
--- and we don't need to predetermine that.  With this approach, the need
for plan invalidation due to user mapping changes goes away entirely.

This doesn't cost us anything at all in simple cases such as direct
execution of a query, because all the checkAsUser fields will be equal
(i.e., zero).  And it also doesn't hurt if the potential foreign join is
encapsulated in a view, where all the checkAsUser fields would contain
the OID of the view owner.

The situation where we potentially lose something is a case like
Etsuro-san's original example, where the query contains one foreign table
reference coming from a view and one coming from the outer query, or maybe
from a different view.  In the two-views case we would have to not push
down the join if the views have different owners, even though perhaps both
owners will use the PUBLIC mapping at runtime.  I think that's a narrow
enough case that we can just live with not optimizing it.  In the
view-and-outer-query case, the simplest answer is that we can't push down
because zero is not equal to the view owner's OID.  We could make that a
little better if we know that the query will be executed as the view
owner, so that the relevant user IDs will be the same at runtime.  There
is already some mechanism in the plan cache to track whether a plan
depends on the identity of the user running it (for RLS), so we could use
that to enforce that a plan containing such a pushed-down join is only run
by the same user that owns the view.

Comments?

regards, tom lane


-- 
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] Oddity in handling of cached plans for FDW queries

[Tom Lane]

Ashutosh Bapat  writes:
> Exactly, for a rare scenario, should we be penalizing large number of plans
> or just continue to use a previously prepared plan when an optimal plan has
> become available because of changed condition. I would choose second over
> the first as it doesn't make things worse than they are.

You seem to be arguing from a premise that the worst consequence is use of
an inefficient plan.  This is false; the worst consequence is use of a
*wrong* plan, specifically one where a join has been pushed down but doing
so is no longer legal because of a user mapping change.  That is, it was
previously correct to access the two remote tables under the same remote
userID but now they should be accessed under different userIDs.  The
result will be that one or the other remote table is accessed under a
userID that is not what the current user mappings say should be used.
That could lead to either unexpected permissions failures (if the
actually-used userID lacks needed permissions) or security breakdowns
(if the actually-used userID has permissions to do something but the
query should not have been allowed to do it).

I do not think fixing this is optional.

I concur with Etsuro-san's dislike for hasForeignJoin; that flag is
underspecified and doesn't convey nearly enough information.  I do not
think a uses_user_mapping flag is much better.  ISTM what should happen is
that any time we decide to push down a foreign join, we should record the
identity of the common user mapping that made that pushdown possible in
the plan's invalItems list.  That would make it possible to invalidate
only the relevant plans when a user mapping is changed.

I believe what Ashutosh is focusing on is whether, when some user mapping
changes, we should invalidate all plans that could potentially now use a
pushed-down join but did not before.  I tend to agree that that's probably
not something we want to do, as it would be very hard to identify just the
plans likely to benefit.  So we would cause replanning of a lot of queries
that won't actually benefit, and in this direction it is indeed only an
optimization not a correctness matter.

Another way we could attack it would be to record the foreign server OID
as an invalItem for any query that has more than one foreign table
belonging to the same foreign server.  Then, invalidate whenever any user
mapping for that server changes.  This would take care of both the case
where a join pushdown becomes possible and where it stops becoming
possible.  But I'm not sure that the extra invalidations would be
worthwhile.

I'm not excited about Etsuro-san's proposal to record user mapping info
in the plan and skip execution-time mapping lookups altogether.  I think
(1) that's solving a problem that hasn't been proven to be a problem,
(2) it doesn't help unless the FDW code is changed to take advantage of
it, which is unlikely to happen for third-party FDWs, and (3) it opens
the door to a class of new bugs, as any failure to invalidate after a
mapping change would become a security fail even for non-join situations.

regards, tom lane


-- 
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] Oddity in handling of cached plans for FDW queries

[Ashutosh Bapat]

On Thu, Jul 14, 2016 at 5:10 PM, Etsuro Fujita 
wrote:

> On 2016/07/13 18:00, Ashutosh Bapat wrote:
>
>> To fix the first, I'd like to propose (1) replacing the existing
>> has_foreign_join flag in the CachedPlan data structure with a new
>> flag, say uses_user_mapping, that indicates whether a cached plan
>> uses any user mapping regardless of whether the cached plan has
>> foreign joins or not (likewise, replace the hasForeignJoin flag in
>> PlannedStmt), and (2) invalidating the cached plan if the
>> uses_user_mapping flag is true.
>>
>
> That way we will have plan cache invalidations even when simple foreign
>> tables scans (not join) are involved, which means all the plans
>> involving any reference to a foreign table with valid user mapping
>> associated with it. That can be a huge cost as compared to the current
>> solution where sub-optimal plan will be used only when a user mapping is
>> changed while a statement has been prepared. That's a rare scenario and
>> somebody can work around that by preparing the statement again.
>>
>
> I'm not sure that's a good workaround.  ISTM that people often don't pay
> much attention to plan changes, so they would execute the inefficient plan
> without realizing the plan change, it would take long, they would start
> thinking what's happening there, and finally, they would find that the
> reason for that is due to the plan change.  I think we should prevent such
> a trouble.
>

The case you described is other way round. When the statement was prepared
the join was not pushed down. A change in user mapping afterwards may allow
the join to be pushed down. But right now it won't be, so a user wouldn't
see any difference, right?

>
> IIRC, we
>> had discussed this situation when implementing the cache invalidation
>> logic.
>>
>
> I didn't know that.  Sorry for speaking up late.
>
> But there's no workaround for your solution.
>>
>
> As you said, this is a rare scenario; in many cases, people define user
> mappings properly beforehand.  So, just invalidating all relevant plans on
> the syscache invalidation events would be fine.  (I thought one possible
> improvement might be to track exactly the dependencies of plans on user
> mappings and invalidate just those plans that depend on the user mapping
> being modified the same way for user-defined functions, but I'm not sure
> it's worth complicating the code.)
>

Exactly, for a rare scenario, should we be penalizing large number of plans
or just continue to use a previously prepared plan when an optimal plan has
become available because of changed condition. I would choose second over
the first as it doesn't make things worse than they are.


> I don't think the above change is sufficient to fix the second.  The
>> root reason for that is that since currently, we allow the user
>> mapping OID (rel->umid) to be InvalidOid in two cases: (1) user
>> mappings mean something to the FDW but it can't get any user mapping
>> at planning time and (2) user mappings are meaningless to the FDW,
>> we cannot distinguish these two cases.
>>
>
> The way to differentiate between these two is to look at the serverid.
>> If server id is invalid it's the case 1,
>>
>
> Really?  Maybe my explanation was not good, but consider a foreign join
> plan created through GetForeignJoinPaths, by an FDW to which user mappings
> are meaningless, like file_fdw.  In that plan, the corresponding server id
> would be valid, not invalid.  No?
>

While planning join, we invalidate user mapping id if the user mappings do
not match (see build_join_rel()). In such case, the joinrel will have
invalid user mapping (and invalid server id) even though user mapping is
associated with the joining tables. The way to differentiate between this
case and the case when an FDW doesn't need user mappings and the join is
shippable is through valid serverid (see build_join_rel()).
Non-availability of a user mapping for a table whose FDW requires user
mappings should end up in an error (in FDW code), so we shouldn't add
complexity for that case.


>
> So, I'd like to introduce a new callback routine to specify that
>> user mappings mean something to the FDW as proposed by Tom [2], and
>> use that to reject the former case, which allows us to set the above
>> uses_user_mapping flag appropriately, ie, set the flag to true only
>> if user mapping changes require forcing a replan.
>>
>
> This routine is meaningless unless the core (or FDW) does not allow a
>> user mapping to be created for such FDWs. Without that, core code would
>> get confused as to what it should do when it sees a user mapping for an
>> FDW which says user mappings are meaningless.
>>
>
> The core wouldn't care about such a user mapping for the FDW; the core
> would just ignore the user mapping.  No?
>

See build_join_rel(). I would like to see, how do you change the conditions
below in that function with 

Re: [HACKERS] Oddity in handling of cached plans for FDW queries

[Etsuro Fujita]

On 2016/07/13 18:00, Ashutosh Bapat wrote:

To fix the first, I'd like to propose (1) replacing the existing
has_foreign_join flag in the CachedPlan data structure with a new
flag, say uses_user_mapping, that indicates whether a cached plan
uses any user mapping regardless of whether the cached plan has
foreign joins or not (likewise, replace the hasForeignJoin flag in
PlannedStmt), and (2) invalidating the cached plan if the
uses_user_mapping flag is true.



That way we will have plan cache invalidations even when simple foreign
tables scans (not join) are involved, which means all the plans
involving any reference to a foreign table with valid user mapping
associated with it. That can be a huge cost as compared to the current
solution where sub-optimal plan will be used only when a user mapping is
changed while a statement has been prepared. That's a rare scenario and
somebody can work around that by preparing the statement again.


I'm not sure that's a good workaround.  ISTM that people often don't pay 
much attention to plan changes, so they would execute the inefficient 
plan without realizing the plan change, it would take long, they would 
start thinking what's happening there, and finally, they would find that 
the reason for that is due to the plan change.  I think we should 
prevent such a trouble.



IIRC, we
had discussed this situation when implementing the cache invalidation
logic.


I didn't know that.  Sorry for speaking up late.


But there's no workaround for your solution.


As you said, this is a rare scenario; in many cases, people define user 
mappings properly beforehand.  So, just invalidating all relevant plans 
on the syscache invalidation events would be fine.  (I thought one 
possible improvement might be to track exactly the dependencies of plans 
on user mappings and invalidate just those plans that depend on the user 
mapping being modified the same way for user-defined functions, but I'm 
not sure it's worth complicating the code.)



One benefit of using the proposed approach is that we could make the
FDW's handling of user mappings in BeginForeignScan more efficient;
currently, there is additional overhead caused by catalog re-lookups
to obtain the user mapping information for the
simple-foreign-table-scan case where user mappings mean something to
the FDW as in postgres_fdw (probably, updates on the catalogs are
infrequent, though), but we could improve the efficiency by using
the validated user mapping information created at planning time for
that case as well as for the foreign-join case.



postgres_fdw to fetches user mapping in some cases but never remembers
it. If what you are describing is a better way, it should have been
implemented before join pushdown was implemented. Refetching a user
mapping is not that expensive given that there is a high chance that it
will be found in the syscache, because it was accessed at the time of
planning.


That assumption is reasonably valid if execution is done immediately 
after planning, but that doesn't necessarily follow.



Effect of plan cache invalidation is probably worse than
fetching the value from a sys cache again.


As I said above, we could expect updates on pg_user_mapping to be 
infrequent, so the effect of the plan cache invalidation would be more 
limited than that of re-fetching user mappings during BeginForeignScan.



I don't think the above change is sufficient to fix the second.  The
root reason for that is that since currently, we allow the user
mapping OID (rel->umid) to be InvalidOid in two cases: (1) user
mappings mean something to the FDW but it can't get any user mapping
at planning time and (2) user mappings are meaningless to the FDW,
we cannot distinguish these two cases.



The way to differentiate between these two is to look at the serverid.
If server id is invalid it's the case 1,


Really?  Maybe my explanation was not good, but consider a foreign join 
plan created through GetForeignJoinPaths, by an FDW to which user 
mappings are meaningless, like file_fdw.  In that plan, the 
corresponding server id would be valid, not invalid.  No?



So, I'd like to introduce a new callback routine to specify that
user mappings mean something to the FDW as proposed by Tom [2], and
use that to reject the former case, which allows us to set the above
uses_user_mapping flag appropriately, ie, set the flag to true only
if user mapping changes require forcing a replan.



This routine is meaningless unless the core (or FDW) does not allow a
user mapping to be created for such FDWs. Without that, core code would
get confused as to what it should do when it sees a user mapping for an
FDW which says user mappings are meaningless.


The core wouldn't care about such a user mapping for the FDW; the core 
would just ignore the user mapping.  No?


Best regards,
Etsuro Fujita




--
Sent via pgsql-hackers 

Re: [HACKERS] Oddity in handling of cached plans for FDW queries

[Ashutosh Bapat]

> Seems odd to me.  Both relations use the same user mapping as before, so
> the join should be pushed down.
>
> Another thing I noticed is that the code in plancache.c would invalidate a
> cached plan with a foreign join due to user mapping changes even in the
> case where user mappings are meaningless to the FDW.
>
> To fix the first, I'd like to propose (1) replacing the existing
> has_foreign_join flag in the CachedPlan data structure with a new flag, say
> uses_user_mapping, that indicates whether a cached plan uses any user
> mapping regardless of whether the cached plan has foreign joins or not
> (likewise, replace the hasForeignJoin flag in PlannedStmt), and (2)
> invalidating the cached plan if the uses_user_mapping flag is true.  I
> thought that we invalidate the cached plan if the flag is true and there is
> a possibility of performing foreign joins, but it seems to me that updates
> on the corresponding catalog are infrequent enough that such a detailed
> tracking is not worth the effort.


That way we will have plan cache invalidations even when simple foreign
tables scans (not join) are involved, which means all the plans involving
any reference to a foreign table with valid user mapping associated with
it. That can be a huge cost as compared to the current solution where
sub-optimal plan will be used only when a user mapping is changed while a
statement has been prepared. That's a rare scenario and somebody can work
around that by preparing the statement again. IIRC, we had discussed this
situation when implementing the cache invalidation logic. But there's no
workaround for your solution.


> One benefit of using the proposed approach is that we could make the FDW's
> handling of user mappings in BeginForeignScan more efficient; currently,
> there is additional overhead caused by catalog re-lookups to obtain the
> user mapping information for the simple-foreign-table-scan case where user
> mappings mean something to the FDW as in postgres_fdw (probably, updates on
> the catalogs are infrequent, though), but we could improve the efficiency
> by using the validated user mapping information created at planning time
> for that case as well as for the foreign-join case.  For that, I'd like to
> propose storing the validated user mapping information into ForeignScan,
> not fdw_private.


postgres_fdw to fetches user mapping in some cases but never remembers it.
If what you are describing is a better way, it should have been implemented
before join pushdown was implemented. Refetching a user mapping is not that
expensive given that there is a high chance that it will be found in the
syscache, because it was accessed at the time of planning. Effect of plan
cache invalidation is probably worse than fetching the value from a sys
cache again.


> There is a discussion about using an ExtensibleNode [1] for that, but the
> proposed approach would make the FDW's job much simpler.
>
> I don't think the above change is sufficient to fix the second.  The root
> reason for that is that since currently, we allow the user mapping OID
> (rel->umid) to be InvalidOid in two cases: (1) user mappings mean something
> to the FDW but it can't get any user mapping at planning time and (2) user
> mappings are meaningless to the FDW, we cannot distinguish these two cases.


The way to differentiate between these two is to look at the serverid. If
server id is invalid it's the case 1, otherwise 2. For a simple table,
there will always be a serverid associated with it. A user mapping will
always be associated with a simple table if there is one i.e. FDW requires
it. Albeit, there will be a case when FDW requires a user mapping but it's
not created, in which case the table will be useless for fetching remote
data anyway. I don't think we should be programming for that case.


> So, I'd like to introduce a new callback routine to specify that user
> mappings mean something to the FDW as proposed by Tom [2], and use that to
> reject the former case, which allows us to set the above uses_user_mapping
> flag appropriately, ie, set the flag to true only if user mapping changes
> require forcing a replan.  This would change the postgres_fdw's behavior
> that it allows to prepare statements involving foreign tables without any
> user mappings as long as those aren't required at planning time, but I'm
> not sure that it's a good idea to keep that behavior because ISTM that such
> a behavior would make people sloppy about creating user mappings, which
> could lead to latent security problems [2].
>
> Attached is a proposed patch for that.
>
>
This routine is meaningless unless the core (or FDW) does not allow a user
mapping to be created for such FDWs. Without that, core code would get
confused as to what it should do when it sees a user mapping for an FDW
which says user mappings are meaningless. If we do that, we might not set
hasForeignJoin flag in create_foreignscan_plan() when the user mapping for
pushed down join is 

Re: [HACKERS] oddity in initdb probing of max_connections/shared_buffers

[Tom Lane]

Greg Stark  writes:
> I happened to notice a bit of an inconsistency in the way initdb
> probes max_connections and shared_buffers.

> This line in the shared_buffers test:

> /* Use same amount of memory, independent of BLCKSZ */
> test_buffs = (trial_bufs[i] * 8192) / BLCKSZ;

> has no equivalent in the max_connections test. As a result
> max_connections is tested with 10 buffers per connection regardless of
> BLCKSZ.

> Is this intentional? Is the idea that Postgres can't function properly
> without being able to read from 10 files concurrently regardless of
> block size? Or is it an unintentional holdover from before the line
> above was added for the shared_buffers tests?

I think it's intentional; the minimum number of buffers needed per
session doesn't really vary with BLCKSZ, but rather with code structure
(ie, how many buffer pins a query might take at once).  Still, some
comments documenting that a little better wouldn't be a bad thing.

regards, tom lane


-- 
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] Oddity with psql \d and pg_table_is_visible

[Decibel!]

On Wed, Sep 05, 2007 at 03:27:50PM -0400, Tom Lane wrote:
 Decibel! [EMAIL PROTECTED] writes:
  While this is correct on a per-relation level, I'm thinking that it's
  not what we'd really like to have happen in psql. What I'd like \d to do
  is show me everything in any schema that's in my search_path, even if
  there's something higher in the search_path that would over-ride it.
  ISTM that's what most people would expect out of \d.
 
 I don't agree with that reasoning in the least, particularly not if you
 intend to fix it by redefining pg_table_is_visible() ...
 
No, pg_table_is_visible is correct as-is.

 What will happen if we change \d to work that way is that it will show
 you a table, and you'll try to access it, and you'll get the wrong table
 because the access will go to the one that really is visible.

That's why I was suggesting that any table showing up in \d that in-fact
wasn't visible be marked somehow, either with a separate field, or by
sticking an * after it's name.

This is confusing because when using \d you generally think in terms of
what schemas are in your search path, not if an individual object has
been superseded by something further up the chain.
-- 
Decibel!, aka Jim Nasby[EMAIL PROTECTED]
EnterpriseDB  http://enterprisedb.com  512.569.9461 (cell)


pgpSd6sbfMLD2.pgp
Description: PGP signature

Re: [HACKERS] Oddity with psql \d and pg_table_is_visible

[Tom Lane]

Decibel! [EMAIL PROTECTED] writes:
 While this is correct on a per-relation level, I'm thinking that it's
 not what we'd really like to have happen in psql. What I'd like \d to do
 is show me everything in any schema that's in my search_path, even if
 there's something higher in the search_path that would over-ride it.
 ISTM that's what most people would expect out of \d.

I don't agree with that reasoning in the least, particularly not if you
intend to fix it by redefining pg_table_is_visible() ...

What will happen if we change \d to work that way is that it will show
you a table, and you'll try to access it, and you'll get the wrong table
because the access will go to the one that really is visible.

regards, tom lane

---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster

Re: [HACKERS] Oddity with extract microseconds?

[Harald Fuchs]

In article [EMAIL PROTECTED],
Christopher Kings-Lynne [EMAIL PROTECTED] writes:

mysql SELECT EXTRACT(MICROSECOND FROM '2003-01-02 10:30:01.00123');
 +---+
 | EXTRACT(MICROSECOND FROM '2003-01-02 10:30:01.00123') |
 +---+
 |  1230 |
 +---+
 1 row in set (0.00 sec)
 Does contrary behavior from MySQL count as evidence that PostgreSQL's
 behavior is correct? :-)

 No...I happen to think that their way is more consistent though.  Pity
 it's not in the spec.

I'd say the comparison with MySQL is useless because MySQL is unable
to store microseconds in a DATETIME or TIMESTAMP column, although you
can extract microseconds from a date/time literal.


---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster

Re: [HACKERS] Oddity with extract microseconds?

[Andrew Dunstan]

Christopher Kings-Lynne wrote:

Why aren't 'minutes' considered too?  Because they aren't 'seconds'. 
Well, seconds aren't microseconds either.



Yeah, they are: it's just one field.  The other way of looking at it
(that everything is seconds) is served by extract(epoch).



Well, it's different in MySQL unfortunately - what does the standard 
say?  Out of interest, can someone try this for me in MySQL 5:


SELECT EXTRACT (MICROSECOND FROM '2003-01-02 10:30:00.00123');
SELECT EXTRACT (MICROSECOND FROM '2003-01-02 10:30:10.00123');



mysql 4.1.5 gives back 123 in both cases. I assume they haven't changed 
that, although anything is possible.


cheers

andrew



---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

  http://www.postgresql.org/docs/faq

Re: [HACKERS] Oddity with extract microseconds?

[Christopher Kings-Lynne]

OK, AndrewSN just pointed out that it's documented to work like that...

...still seems bizarre...

Chris

Christopher Kings-Lynne wrote:

Does anyone else find this odd:

mysql=# select extract(microseconds from timestamp '2005-01-01
00:00:00.123');
 date_part
---
123000
(1 row)

mysql=# select extract(microseconds from timestamp '2005-01-01
00:00:01.123');
 date_part
---
   1123000
(1 row)

No other extracts include other fields.  eg, minutes:

mysql=# select extract(minutes from timestamp '2005-01-01 00:10:00');
 date_part
---
10
(1 row)

mysql=# select extract(minutes from timestamp '2005-01-01 10:10:00');
 date_part
---
10

So how come microseconds includes the microseconds from the 'seconds'
field and not just after the '.'?  And if it's supposed to include
'seconds', then why doesn't it include minutes, hours, etc.?

Chris


---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings


---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster

Re: [HACKERS] Oddity with extract microseconds?

[Alvaro Herrera]

Christopher Kings-Lynne wrote:
 OK, AndrewSN just pointed out that it's documented to work like that...
 
 ...still seems bizarre...

So it's a gotcha!

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

---(end of broadcast)---
TIP 6: explain analyze is your friend

Re: [HACKERS] Oddity with extract microseconds?

[Tom Lane]

Christopher Kings-Lynne [EMAIL PROTECTED] writes:
 OK, AndrewSN just pointed out that it's documented to work like that...
 ...still seems bizarre...

It seems reasonably consistent to me.  extract() doesn't consider
seconds and fractional seconds to be distinct fields: it's all one
value.  The milliseconds and microseconds options just shift the
decimal place for you.

regards, tom lane

---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings

Re: [HACKERS] Oddity with extract microseconds?

[Bruce Momjian]

Tom Lane wrote:
 Christopher Kings-Lynne [EMAIL PROTECTED] writes:
  OK, AndrewSN just pointed out that it's documented to work like that...
  ...still seems bizarre...
 
 It seems reasonably consistent to me.  extract() doesn't consider
 seconds and fractional seconds to be distinct fields: it's all one
 value.  The milliseconds and microseconds options just shift the
 decimal place for you.

I think this illustrates the issue:

test= SELECT date_part('microseconds', '00:00:01.33'::time);
 date_part
---
   133
(1 row)

test= SELECT date_part('microseconds', '00:03:01.33'::time);
 date_part
---
   133
(1 row)

Why aren't 'minutes' considered too?  Because they aren't 'seconds'. 
Well, seconds aren't microseconds either.

-- 
  Bruce Momjian|  http://candle.pha.pa.us
  pgman@candle.pha.pa.us   |  (610) 359-1001
  +  If your life is a hard drive, |  13 Roberts Road
  +  Christ can be your backup.|  Newtown Square, Pennsylvania 19073

---(end of broadcast)---
TIP 4: Have you searched our list archives?

   http://archives.postgresql.org

Re: [HACKERS] Oddity with extract microseconds?

[Tom Lane]

Bruce Momjian pgman@candle.pha.pa.us writes:
 Why aren't 'minutes' considered too?  Because they aren't 'seconds'. 
 Well, seconds aren't microseconds either.

Yeah, they are: it's just one field.  The other way of looking at it
(that everything is seconds) is served by extract(epoch).

regards, tom lane

---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

   http://www.postgresql.org/docs/faq

Re: [HACKERS] Oddity with extract microseconds?

[Christopher Kings-Lynne]

Why aren't 'minutes' considered too?  Because they aren't 'seconds'. 
Well, seconds aren't microseconds either.


Yeah, they are: it's just one field.  The other way of looking at it
(that everything is seconds) is served by extract(epoch).


Well, it's different in MySQL unfortunately - what does the standard 
say?  Out of interest, can someone try this for me in MySQL 5:


SELECT EXTRACT (MICROSECOND FROM '2003-01-02 10:30:00.00123');
SELECT EXTRACT (MICROSECOND FROM '2003-01-02 10:30:10.00123');

Chris


---(end of broadcast)---
TIP 4: Have you searched our list archives?

  http://archives.postgresql.org

Re: [HACKERS] Oddity with extract microseconds?

[Michael Fuhr]

On Wed, Dec 07, 2005 at 09:43:30AM +0800, Christopher Kings-Lynne wrote:
 Why aren't 'minutes' considered too?  Because they aren't 'seconds'. 
 Well, seconds aren't microseconds either.
 
 Yeah, they are: it's just one field.  The other way of looking at it
 (that everything is seconds) is served by extract(epoch).
 
 Well, it's different in MySQL unfortunately - what does the standard 
 say?

I don't see microseconds as a possible field in SQL:2003 (draft copy).

 Out of interest, can someone try this for me in MySQL 5:
 
 SELECT EXTRACT (MICROSECOND FROM '2003-01-02 10:30:00.00123');
 SELECT EXTRACT (MICROSECOND FROM '2003-01-02 10:30:10.00123');

MySQL 5.0.16 gives an error:

mysql SELECT EXTRACT (MICROSECOND FROM '2003-01-02 10:30:00.00123');
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual
that corresponds to your MySQL server version for the right syntax to use
near 'FROM '2003-01-02 10:30:00.00123')' at line 1

-- 
Michael Fuhr

---(end of broadcast)---
TIP 2: Don't 'kill -9' the postmaster

Re: [HACKERS] Oddity with extract microseconds?

[Christopher Kings-Lynne]

MySQL 5.0.16 gives an error:

mysql SELECT EXTRACT (MICROSECOND FROM '2003-01-02 10:30:00.00123');
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual
that corresponds to your MySQL server version for the right syntax to use
near 'FROM '2003-01-02 10:30:00.00123')' at line 1


Odd, that example is straight from the MySQL 5 manual:

http://dev.mysql.com/doc/refman/5.0/en/date-and-time-functions.html

Chris


---(end of broadcast)---
TIP 5: don't forget to increase your free space map settings

Re: [HACKERS] Oddity with extract microseconds?

[Christopher Kings-Lynne]

Looks like MySQL doesn't allow a space before the open parenthesis
(there isn't one in the manual's example):

mysql SELECT EXTRACT(MICROSECOND FROM '2003-01-02 10:30:00.00123');
+---+
| EXTRACT(MICROSECOND FROM '2003-01-02 10:30:00.00123') |
+---+
|  1230 |
+---+
1 row in set (0.01 sec)


Ok, and what does this give:

SELECT EXTRACT(MICROSECOND FROM '2003-01-02 10:30:01.00123');

Chris


---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

  http://www.postgresql.org/docs/faq

Re: [HACKERS] Oddity with extract microseconds?

[Michael Fuhr]

On Wed, Dec 07, 2005 at 10:32:20AM +0800, Christopher Kings-Lynne wrote:
 MySQL 5.0.16 gives an error:
 
 mysql SELECT EXTRACT (MICROSECOND FROM '2003-01-02 10:30:00.00123');
 ERROR 1064 (42000): You have an error in your SQL syntax; check the manual
 that corresponds to your MySQL server version for the right syntax to use
 near 'FROM '2003-01-02 10:30:00.00123')' at line 1
 
 Odd, that example is straight from the MySQL 5 manual:
 
 http://dev.mysql.com/doc/refman/5.0/en/date-and-time-functions.html

Looks like MySQL doesn't allow a space before the open parenthesis
(there isn't one in the manual's example):

mysql SELECT EXTRACT(MICROSECOND FROM '2003-01-02 10:30:00.00123');
+---+
| EXTRACT(MICROSECOND FROM '2003-01-02 10:30:00.00123') |
+---+
|  1230 |
+---+
1 row in set (0.01 sec)

-- 
Michael Fuhr

---(end of broadcast)---
TIP 3: Have you checked our extensive FAQ?

   http://www.postgresql.org/docs/faq

Re: [HACKERS] Oddity with extract microseconds?

[Michael Fuhr]

On Wed, Dec 07, 2005 at 10:47:45AM +0800, Christopher Kings-Lynne wrote:
 Ok, and what does this give:
 
 SELECT EXTRACT(MICROSECOND FROM '2003-01-02 10:30:01.00123');

mysql SELECT EXTRACT(MICROSECOND FROM '2003-01-02 10:30:01.00123');
+---+
| EXTRACT(MICROSECOND FROM '2003-01-02 10:30:01.00123') |
+---+
|  1230 |
+---+
1 row in set (0.00 sec)

Does contrary behavior from MySQL count as evidence that PostgreSQL's
behavior is correct? :-)

-- 
Michael Fuhr

---(end of broadcast)---
TIP 6: explain analyze is your friend

Re: [HACKERS] Oddity with extract microseconds?

[Christopher Kings-Lynne]

mysql SELECT EXTRACT(MICROSECOND FROM '2003-01-02 10:30:01.00123');
+---+
| EXTRACT(MICROSECOND FROM '2003-01-02 10:30:01.00123') |
+---+
|  1230 |
+---+
1 row in set (0.00 sec)

Does contrary behavior from MySQL count as evidence that PostgreSQL's
behavior is correct? :-)


No...I happen to think that their way is more consistent though.  Pity 
it's not in the spec.


At least PostgreSQL is consistent with seconds/microseconds:

mysql=# select extract(seconds from timestamp '2005-01-01 00:00:01.01');
 date_part
---
  1.01
(1 row)

Chris


---(end of broadcast)---
TIP 9: In versions below 8.0, the planner will ignore your desire to
  choose an index scan if your joining column's datatypes do not
  match