Re: [HACKERS] inherit support for foreign tables

2015-04-26 Thread Andres Freund
On 2015-04-25 20:47:04 -0400, Tom Lane wrote:
 Since default_with_oids is really only meant as a backwards-compatibility
 hack, I don't personally have a problem with restricting it to act only on
 plain tables.

FWIW, I think we're getting pretty close to the point, or are there
even, where we can remove default_with_oids. So not adding complications
because of it sounds good to me.

Greetings,

Andres Freund


-- 
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] inherit support for foreign tables

2015-04-26 Thread Tom Lane
Andres Freund and...@anarazel.de writes:
 FWIW, I think we're getting pretty close to the point, or are there
 even, where we can remove default_with_oids. So not adding complications
 because of it sounds good to me.

Well, pg_dump uses it --- so the argument about not breaking old dump
scripts would apply to any attempt to remove it entirely.  But I don't
have a problem with saying that its semantics are frozen.

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] inherit support for foreign tables

2015-04-25 Thread Tom Lane
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 On 2015/04/16 16:05, Etsuro Fujita wrote:
 I agree with you on this point.  However, ISTM there is a bug in
 handling OIDs on foreign tables; while we now allow for ALTER SET
 WITH/WITHOUT OIDS, we still don't allow the default_with_oids parameter
 for foreign tables.  I think that since CREATE FOREIGN TABLE should be
 consistent with ALTER FOREIGN TABLE, we should also allow the parameter
 for foreign tables.  Attached is a patch for that.

 I also updated docs.  Attached is an updated version of the patch.

I believe that we intentionally did not do this, and here is why not:
existing pg_dump files assume that default_with_oids doesn't affect any
relation type except plain tables.  pg_backup_archiver.c only bothers
to change the GUC when about to dump a plain table, and otherwise leaves
it at its previous value.  That means if we apply a patch like this, it's
entirely possible that pg_dump/pg_restore will result in foreign tables
accidentally acquiring OID columns.

Since default_with_oids is really only meant as a backwards-compatibility
hack, I don't personally have a problem with restricting it to act only on
plain tables.  However, it might be a good idea to explicitly document
this interaction in a code comment to prevent anyone from re-inventing
this idea...  I'll go do 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] inherit support for foreign tables

2015-04-22 Thread Stephen Frost
Etsuro,

* Etsuro Fujita (fujita.ets...@lab.ntt.co.jp) wrote:
 postgres=# select * from ft1 for update;
 ERROR:  could not find junk tableoid1 column
 
 I think this is a bug.  Attached is a patch fixing this issue.

Pushed, thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] inherit support for foreign tables

2015-04-22 Thread Etsuro Fujita

On 2015/04/23 0:34, Stephen Frost wrote:

* Etsuro Fujita (fujita.ets...@lab.ntt.co.jp) wrote:

postgres=# select * from ft1 for update;
ERROR:  could not find junk tableoid1 column

I think this is a bug.  Attached is a patch fixing this issue.


Pushed, thanks!


Thank you.

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] inherit support for foreign tables

2015-04-22 Thread Kyotaro HORIGUCHI
Hello,

At Thu, 16 Apr 2015 14:43:33 -0700, David Fetter da...@fetter.org wrote in 
20150416214333.ga...@fetter.org
 On Wed, Apr 15, 2015 at 09:35:05AM +0900, Kyotaro HORIGUCHI wrote:
  Hi,
  
  Before suppressing the symptom, I doubt the necessity and/or
  validity of giving foreign tables an ability to be a parent. Is
  there any reasonable usage for the ability?
...
 I have a use case for having foreign tables as non-leaf nodes in a
 partitioning hierarchy, namely geographic.

Ah, I see. I understood the case of intermediate nodes. I agree
that it is quite natural.

  One might have a table at
 HQ called foo_world, then partitions under it called foo_jp, foo_us,
 etc., in one level, foo_us_ca, foo_us_pa, etc. in the next level, and
 on down, each in general in a separate data center.
 
 Is there something essential about having non-leaf nodes as foreign
 tables that's a problem here?

No. I'm convinced of the necessity. Sorry for the noise.


At Wed, 22 Apr 2015 17:00:10 -0400, Robert Haas robertmh...@gmail.com wrote 
in CA+TgmobZVHp3D9wWCV8QJc+qGDu7=tekncbxowijzkhjucm...@mail.gmail.com
 Gee, I don't see why that would be unreasonable or invalid

Hmm. Yes, as mentioned above, there's no reason to refuse
non-leaf foregin tables. I didn't understood the real cause of
the problem and thought that not allowing foreign *root* tables
seem better than tweaking elsewhere. But that thought found to be
totally a garbage :(

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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


Re: [HACKERS] inherit support for foreign tables

2015-04-22 Thread Robert Haas
On Tue, Apr 14, 2015 at 8:35 PM, Kyotaro HORIGUCHI
horiguchi.kyot...@lab.ntt.co.jp wrote:
 Before suppressing the symptom, I doubt the necessity and/or
 validity of giving foreign tables an ability to be a parent. Is
 there any reasonable usage for the ability?

Gee, I don't see why that would be unreasonable or invalid

-- 
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] inherit support for foreign tables

2015-04-20 Thread Etsuro Fujita
On 2015/04/16 16:05, Etsuro Fujita wrote:
 On 2015/03/23 2:57, Tom Lane wrote:
 Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 [ fdw-inh-8.patch ]

 I've committed this with some substantial rearrangements, notably:
 
 * As I mentioned earlier, I got rid of a few unnecessary restrictions on
 foreign tables so as to avoid introducing warts into inheritance behavior.
 In particular, we now allow NOT VALID CHECK constraints (and hence ALTER
 ... VALIDATE CONSTRAINT), ALTER SET STORAGE, and ALTER SET WITH/WITHOUT
 OIDS.  These are probably no-ops anyway for foreign tables, though
 conceivably an FDW might choose to implement some behavior for STORAGE
 or OIDs.
 
 I agree with you on this point.  However, ISTM there is a bug in
 handling OIDs on foreign tables; while we now allow for ALTER SET
 WITH/WITHOUT OIDS, we still don't allow the default_with_oids parameter
 for foreign tables.  I think that since CREATE FOREIGN TABLE should be
 consistent with ALTER FOREIGN TABLE, we should also allow the parameter
 for foreign tables.  Attached is a patch for that.

I also updated docs.  Attached is an updated version of the patch.

Best regards,
Etsuro Fujita
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***
*** 6745,6752  dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir'
/term
listitem
 para
! This controls whether commandCREATE TABLE/command and
! commandCREATE TABLE AS/command include an OID column in
  newly-created tables, if neither literalWITH OIDS/literal
  nor literalWITHOUT OIDS/literal is specified. It also
  determines whether OIDs will be included in tables created by
--- 6745,6753 
/term
listitem
 para
! This controls whether commandCREATE TABLE/command,
! commandCREATE TABLE AS/command and
! commandCREATE FOREIGN TABLE/command include an OID column in
  newly-created tables, if neither literalWITH OIDS/literal
  nor literalWITHOUT OIDS/literal is specified. It also
  determines whether OIDs will be included in tables created by
*** a/doc/src/sgml/ref/create_foreign_table.sgml
--- b/doc/src/sgml/ref/create_foreign_table.sgml
***
*** 293,298  CHECK ( replaceable class=PARAMETERexpression/replaceable )
--- 293,304 
  responsibility to ensure that the constraint definition matches
  reality.
 /para
+ 
+   para
+To add OIDs to the table created by commandCREATE FOREIGN TABLE/command,
+enable the xref linkend=guc-default-with-oids configuration
+variable.
+   /para
   /refsect1
  
   refsect1 id=SQL-CREATEFOREIGNTABLE-examples
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 580,586  DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
  	descriptor = BuildDescForRelation(schema);
  
  	localHasOids = interpretOidsOption(stmt-options,
! 	   (relkind == RELKIND_RELATION));
  	descriptor-tdhasoid = (localHasOids || parentOidCount  0);
  
  	/*
--- 580,587 
  	descriptor = BuildDescForRelation(schema);
  
  	localHasOids = interpretOidsOption(stmt-options,
! 	   (relkind == RELKIND_RELATION ||
! 		relkind == RELKIND_FOREIGN_TABLE));
  	descriptor-tdhasoid = (localHasOids || parentOidCount  0);
  
  	/*

-- 
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] inherit support for foreign tables

2015-04-16 Thread Etsuro Fujita
On 2015/03/23 2:57, Tom Lane wrote:
 Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 [ fdw-inh-8.patch ]
 
 I've committed this with some substantial rearrangements, notably:

 * As I mentioned earlier, I got rid of a few unnecessary restrictions on
 foreign tables so as to avoid introducing warts into inheritance behavior.
 In particular, we now allow NOT VALID CHECK constraints (and hence ALTER
 ... VALIDATE CONSTRAINT), ALTER SET STORAGE, and ALTER SET WITH/WITHOUT
 OIDS.  These are probably no-ops anyway for foreign tables, though
 conceivably an FDW might choose to implement some behavior for STORAGE
 or OIDs.

I agree with you on this point.  However, ISTM there is a bug in
handling OIDs on foreign tables; while we now allow for ALTER SET
WITH/WITHOUT OIDS, we still don't allow the default_with_oids parameter
for foreign tables.  I think that since CREATE FOREIGN TABLE should be
consistent with ALTER FOREIGN TABLE, we should also allow the parameter
for foreign tables.  Attached is a patch for that.

Best regards,
Etsuro Fujita
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 580,586  DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
  	descriptor = BuildDescForRelation(schema);
  
  	localHasOids = interpretOidsOption(stmt-options,
! 	   (relkind == RELKIND_RELATION));
  	descriptor-tdhasoid = (localHasOids || parentOidCount  0);
  
  	/*
--- 580,587 
  	descriptor = BuildDescForRelation(schema);
  
  	localHasOids = interpretOidsOption(stmt-options,
! 	   (relkind == RELKIND_RELATION ||
! 		relkind == RELKIND_FOREIGN_TABLE));
  	descriptor-tdhasoid = (localHasOids || parentOidCount  0);
  
  	/*

-- 
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] inherit support for foreign tables

2015-04-16 Thread Kyotaro HORIGUCHI
Hello,

At Thu, 16 Apr 2015 12:20:47 +0900, Etsuro Fujita fujita.ets...@lab.ntt.co.jp 
wrote in 552f2a8f.2090...@lab.ntt.co.jp
 On 2015/04/15 3:52, Alvaro Herrera wrote:
  On 4/14/15 5:49 AM, Etsuro Fujita wrote:
  postgres=# create foreign table ft1 (c1 int) server myserver options
  (table_name 't1');
  CREATE FOREIGN TABLE
  postgres=# create foreign table ft2 (c1 int) server myserver options
  (table_name 't2');
  CREATE FOREIGN TABLE
  postgres=# alter foreign table ft2 inherit ft1;
  ALTER FOREIGN TABLE
  postgres=# select * from ft1 for update;
  ERROR:  could not find junk tableoid1 column
 
  I think this is a bug.  Attached is a patch fixing this issue.
 
  I think the real question is whether we're now (I mean after this
  patch)
  emitting useless tableoid columns that we didn't previously have.  I
  think the answer is yes, and if so I think we need a smaller comb to
  fix
  the problem.  This one seems rather large.
 
 My answer for that would be *no* because I think tableoid would be
 needed for EvalPlanQual checking in more complex SELECT FOR UPDATE on
 the inheritance or UPDATE/DELETE involving the inheritance as a source
 table.  Also, if we allow the FDW to change the behavior of SELECT FOR
 UPDATE so as to match the local semantics exactly, which I'm working
 on in another thread, I think tableoid would also be needed for the
 actual row locking.

Given the parent foreign talbes, surely they need tableoids for
such usage. The patch preserves the condition rc-isParent so it
newly affects exactly only parent foreign tables for now.

Before the parent foreign tables introduced, ROW_MARK_COPY and
RTE_RELATION are mutually exclusive so didn't need, or cannot
have tableoid.  But now it intorduces an rte with ROW_MARK_COPY 
RTE_RELATION and there seems no reason for parent tables in any
kind not to have tableoid. After such consideration, I came to
think that the patch is a reasonable fix, not mere a workaround.

Thoughts?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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


Re: [HACKERS] inherit support for foreign tables

2015-04-16 Thread David Fetter
On Wed, Apr 15, 2015 at 09:35:05AM +0900, Kyotaro HORIGUCHI wrote:
 Hi,
 
 Before suppressing the symptom, I doubt the necessity and/or
 validity of giving foreign tables an ability to be a parent. Is
 there any reasonable usage for the ability?
 
 I think we should choose to inhibit foreign tables from becoming
 a parent rather than leaving it allowed then taking measures for
 the consequent symptom.

I have a use case for having foreign tables as non-leaf nodes in a
partitioning hierarchy, namely geographic.  One might have a table at
HQ called foo_world, then partitions under it called foo_jp, foo_us,
etc., in one level, foo_us_ca, foo_us_pa, etc. in the next level, and
on down, each in general in a separate data center.

Is there something essential about having non-leaf nodes as foreign
tables that's a problem here?

Cheers,
David.
-- 
David Fetter da...@fetter.org http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] inherit support for foreign tables

2015-04-15 Thread Etsuro Fujita

On 2015/04/15 3:52, Alvaro Herrera wrote:

On 4/14/15 5:49 AM, Etsuro Fujita wrote:

postgres=# create foreign table ft1 (c1 int) server myserver options
(table_name 't1');
CREATE FOREIGN TABLE
postgres=# create foreign table ft2 (c1 int) server myserver options
(table_name 't2');
CREATE FOREIGN TABLE
postgres=# alter foreign table ft2 inherit ft1;
ALTER FOREIGN TABLE
postgres=# select * from ft1 for update;
ERROR:  could not find junk tableoid1 column

I think this is a bug.  Attached is a patch fixing this issue.



I think the real question is whether we're now (I mean after this patch)
emitting useless tableoid columns that we didn't previously have.  I
think the answer is yes, and if so I think we need a smaller comb to fix
the problem.  This one seems rather large.


My answer for that would be *no* because I think tableoid would be 
needed for EvalPlanQual checking in more complex SELECT FOR UPDATE on 
the inheritance or UPDATE/DELETE involving the inheritance as a source 
table.  Also, if we allow the FDW to change the behavior of SELECT FOR 
UPDATE so as to match the local semantics exactly, which I'm working on 
in another thread, I think tableoid would also be needed for the actual 
row locking.


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] inherit support for foreign tables

2015-04-14 Thread Etsuro Fujita
On 2015/03/23 2:57, Tom Lane wrote:
 Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 [ fdw-inh-8.patch ]
 
 I've committed this with some substantial rearrangements, notably:
 
 * I thought that if we were doing this at all, we should go all the way
 and allow foreign tables to be both inheritance parents and children.

I found that when setting a foreign table to be the parent of an
inheritance set that only contains foreign tables, SELECT FOR UPDATE on
the inheritance parent fails with a can't-happen error condition.  Here
is an example:

$ createdb mydb
$ psql mydb
psql (9.5devel)
Type help for help.

mydb=# create table t1 (c1 int);
CREATE TABLE
mydb=# create table t2 (c1 int);
CREATE TABLE

$ psql postgres
psql (9.5devel)
Type help for help.

postgres=# create extension postgres_fdw;
CREATE EXTENSION
postgres=# create server myserver foreign data wrapper postgres_fdw
options (dbname 'mydb');
CREATE SERVER
postgres=# create user mapping for current_user server myserver;
CREATE USER MAPPING
postgres=# create foreign table ft1 (c1 int) server myserver options
(table_name 't1');
CREATE FOREIGN TABLE
postgres=# create foreign table ft2 (c1 int) server myserver options
(table_name 't2');
CREATE FOREIGN TABLE
postgres=# alter foreign table ft2 inherit ft1;
ALTER FOREIGN TABLE
postgres=# select * from ft1 for update;
ERROR:  could not find junk tableoid1 column

I think this is a bug.  Attached is a patch fixing this issue.

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 3193,3218  select * from bar where f1 in (select f1 from foo) for update;
QUERY PLAN  
  --
   LockRows
!Output: bar.f1, bar.f2, bar.ctid, bar.tableoid, bar.*, foo.ctid, foo.tableoid, foo.*
 -  Hash Join
!  Output: bar.f1, bar.f2, bar.ctid, bar.tableoid, bar.*, foo.ctid, foo.tableoid, foo.*
   Hash Cond: (bar.f1 = foo.f1)
   -  Append
 -  Seq Scan on public.bar
!  Output: bar.f1, bar.f2, bar.ctid, bar.tableoid, bar.*
 -  Foreign Scan on public.bar2
!  Output: bar2.f1, bar2.f2, bar2.ctid, bar2.tableoid, bar2.*
   Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE
   -  Hash
!Output: foo.ctid, foo.tableoid, foo.*, foo.f1
 -  HashAggregate
!  Output: foo.ctid, foo.tableoid, foo.*, foo.f1
   Group Key: foo.f1
   -  Append
 -  Seq Scan on public.foo
!  Output: foo.ctid, foo.tableoid, foo.*, foo.f1
 -  Foreign Scan on public.foo2
!  Output: foo2.ctid, foo2.tableoid, foo2.*, foo2.f1
   Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct1
  (22 rows)
  
--- 3193,3218 
QUERY PLAN  
  --
   LockRows
!Output: bar.f1, bar.f2, bar.ctid, bar.*, bar.tableoid, foo.ctid, foo.*, foo.tableoid
 -  Hash Join
!  Output: bar.f1, bar.f2, bar.ctid, bar.*, bar.tableoid, foo.ctid, foo.*, foo.tableoid
   Hash Cond: (bar.f1 = foo.f1)
   -  Append
 -  Seq Scan on public.bar
!  Output: bar.f1, bar.f2, bar.ctid, bar.*, bar.tableoid
 -  Foreign Scan on public.bar2
!  Output: bar2.f1, bar2.f2, bar2.ctid, bar2.*, bar2.tableoid
   Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE
   -  Hash
!Output: foo.ctid, foo.*, foo.tableoid, foo.f1
 -  HashAggregate
!  Output: foo.ctid, foo.*, foo.tableoid, foo.f1
   Group Key: foo.f1
   -  Append
 -  Seq Scan on public.foo
!  Output: foo.ctid, foo.*, foo.tableoid, foo.f1
 -  Foreign Scan on public.foo2
!  Output: foo2.ctid, foo2.*, foo2.tableoid, foo2.f1
   Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct1
  (22 rows)
  
***
*** 3230,3255  select * from bar where f1 in (select f1 from foo) for share;
QUERY PLAN  
  --
   LockRows
!Output: bar.f1, bar.f2, bar.ctid, bar.tableoid, bar.*, foo.ctid, foo.tableoid, foo.*

Re: [HACKERS] inherit support for foreign tables

2015-04-14 Thread Jim Nasby
On 4/14/15 5:49 AM, Etsuro Fujita wrote:
 postgres=# create foreign table ft1 (c1 int) server myserver options
 (table_name 't1');
 CREATE FOREIGN TABLE
 postgres=# create foreign table ft2 (c1 int) server myserver options
 (table_name 't2');
 CREATE FOREIGN TABLE
 postgres=# alter foreign table ft2 inherit ft1;
 ALTER FOREIGN TABLE
 postgres=# select * from ft1 for update;
 ERROR:  could not find junk tableoid1 column
 
 I think this is a bug.  Attached is a patch fixing this issue.

What happens when the foreign side breaks the inheritance? Does the FDW
somehow know to check that fact for each query?

What do you gain from having the local table have inheritance?

Basically, I think we have to be very careful about implementing
features that imply the local side knows something about the persisted
state of the remote side (in this case, whether there's inheritance).
Anything like that sets us up for synchronization problems.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.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] inherit support for foreign tables

2015-04-14 Thread Kyotaro HORIGUCHI
Hi,

Before suppressing the symptom, I doubt the necessity and/or
validity of giving foreign tables an ability to be a parent. Is
there any reasonable usage for the ability?

I think we should choose to inhibit foreign tables from becoming
a parent rather than leaving it allowed then taking measures for
the consequent symptom.


regards,

At Tue, 14 Apr 2015 15:52:18 -0300, Alvaro Herrera alvhe...@2ndquadrant.com 
wrote in 20150414185218.gx4...@alvh.no-ip.org
 Jim Nasby wrote:
  On 4/14/15 5:49 AM, Etsuro Fujita wrote:
   postgres=# create foreign table ft1 (c1 int) server myserver options
   (table_name 't1');
   CREATE FOREIGN TABLE
   postgres=# create foreign table ft2 (c1 int) server myserver options
   (table_name 't2');
   CREATE FOREIGN TABLE
   postgres=# alter foreign table ft2 inherit ft1;
   ALTER FOREIGN TABLE
   postgres=# select * from ft1 for update;
   ERROR:  could not find junk tableoid1 column
   
   I think this is a bug.  Attached is a patch fixing this issue.
  
  What happens when the foreign side breaks the inheritance? Does the FDW
  somehow know to check that fact for each query?
 
 This is a meaningless question.  The remote tables don't have to have an
 inheritance relationship already; only the local side sees them as
 connected.
 
 I think the real question is whether we're now (I mean after this patch)
 emitting useless tableoid columns that we didn't previously have.  I
 think the answer is yes, and if so I think we need a smaller comb to fix
 the problem.  This one seems rather large.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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


Re: [HACKERS] inherit support for foreign tables

2015-04-14 Thread Alvaro Herrera
Jim Nasby wrote:
 On 4/14/15 5:49 AM, Etsuro Fujita wrote:
  postgres=# create foreign table ft1 (c1 int) server myserver options
  (table_name 't1');
  CREATE FOREIGN TABLE
  postgres=# create foreign table ft2 (c1 int) server myserver options
  (table_name 't2');
  CREATE FOREIGN TABLE
  postgres=# alter foreign table ft2 inherit ft1;
  ALTER FOREIGN TABLE
  postgres=# select * from ft1 for update;
  ERROR:  could not find junk tableoid1 column
  
  I think this is a bug.  Attached is a patch fixing this issue.
 
 What happens when the foreign side breaks the inheritance? Does the FDW
 somehow know to check that fact for each query?

This is a meaningless question.  The remote tables don't have to have an
inheritance relationship already; only the local side sees them as
connected.

I think the real question is whether we're now (I mean after this patch)
emitting useless tableoid columns that we didn't previously have.  I
think the answer is yes, and if so I think we need a smaller comb to fix
the problem.  This one seems rather large.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training  Services


-- 
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] inherit support for foreign tables

2015-03-23 Thread Ashutosh Bapat
On Mon, Mar 23, 2015 at 12:09 AM, Robert Haas robertmh...@gmail.com wrote:

 On Sun, Mar 22, 2015 at 1:57 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
  [ fdw-inh-8.patch ]
 
  I've committed this with some substantial rearrangements, notably:

 I'm really glad this is going in!  Thanks to to Shigeru Hanada and
 Etsuro Fujita for working on this, to you (Tom) for putting in the
 time to get it committed, and of course to the reviewers Ashutosh
 Bapat and Kyotaro Horiguchi for their time and effort.

 In a way, I believe we can think of this as the beginnings of a
 sharding story for PostgreSQL.  A lot more work is needed, of course
 -- join and aggregate pushdown are high on my personal list -- but
 it's a start.


+1.


 --
 Robert Haas
 EnterpriseDB: http://www.enterprisedb.com
 The Enterprise PostgreSQL Company




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


Re: [HACKERS] inherit support for foreign tables

2015-03-23 Thread Etsuro Fujita
On 2015/03/23 2:57, Tom Lane wrote:
 I've committed this with some substantial rearrangements, notably:

Thanks for taking the time to committing the patch!

Thanks for the work, Hanada-san!  And thank you everyone for the reviews
and comments, especially Ashutosh, Horiguchi-san and Noah!

 * I fooled around with the PlanRowMark changes some more, mainly with
 the idea that we might soon allow FDWs to use rowmark methods other than
 ROW_MARK_COPY.  The planner now has just one place where a rel's rowmark
 method is chosen, so as to centralize anything we need to do there.

Will work on this issue.

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] inherit support for foreign tables

2015-03-22 Thread Tom Lane
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 [ fdw-inh-8.patch ]

I've committed this with some substantial rearrangements, notably:

* I thought that if we were doing this at all, we should go all the way
and allow foreign tables to be both inheritance parents and children.

* As I mentioned earlier, I got rid of a few unnecessary restrictions on
foreign tables so as to avoid introducing warts into inheritance behavior.
In particular, we now allow NOT VALID CHECK constraints (and hence ALTER
... VALIDATE CONSTRAINT), ALTER SET STORAGE, and ALTER SET WITH/WITHOUT
OIDS.  These are probably no-ops anyway for foreign tables, though
conceivably an FDW might choose to implement some behavior for STORAGE
or OIDs.

* I did not like the EXPLAIN changes at all; in the first place they
resulted in invalid JSON output (there could be multiple fields of the
Update plan object with identical labels), and in the second place it
seemed like a bad idea to rely on FDWs to change the behavior of their
ExplainModifyTarget functions.  I've refactored that so that explain.c
remains responsible for getting the grouping right.  Also, as I said
earlier, it seemed like a good idea to produce subgroups identifying
all the target tables not only the foreign ones.

* I fooled around with the PlanRowMark changes some more, mainly with
the idea that we might soon allow FDWs to use rowmark methods other than
ROW_MARK_COPY.  The planner now has just one place where a rel's rowmark
method is chosen, so as to centralize anything we need to do there.

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] inherit support for foreign tables

2015-03-22 Thread Robert Haas
On Sun, Mar 22, 2015 at 1:57 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 [ fdw-inh-8.patch ]

 I've committed this with some substantial rearrangements, notably:

I'm really glad this is going in!  Thanks to to Shigeru Hanada and
Etsuro Fujita for working on this, to you (Tom) for putting in the
time to get it committed, and of course to the reviewers Ashutosh
Bapat and Kyotaro Horiguchi for their time and effort.

In a way, I believe we can think of this as the beginnings of a
sharding story for PostgreSQL.  A lot more work is needed, of course
-- join and aggregate pushdown are high on my personal list -- but
it's a start.

-- 
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] inherit support for foreign tables

2015-03-20 Thread Tom Lane
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 I noticed that the latter disallows TRUNCATE on inheritance trees that 
 contain at least one child foreign table.  But I think it would be 
 better to allow it, with the semantics that we quietly ignore the child 
 foreign tables and apply the operation to the child plain tables, which 
 is the same semantics as ALTER COLUMN SET STORAGE on such inheritance 
 trees.  Comments welcome.

I've been working through the foreign table inheritance patch, and found
the code that makes the above happen.  I don't think this is a good idea
at all.  In the first place, successful TRUNCATE should leave the table
empty, not well, we'll make it empty if we feel up to that.  In the
second place, someday we might want to make TRUNCATE actually work for
foreign tables (at least for FDWs that want to support it).  If we did,
we would have a backwards-compatibility hazard, because suddenly a
TRUNCATE on an inheritance tree that includes a foreign table would have
different non-error effects than before.

I think we should just throw error in this case.

BTW, the SET STORAGE comparison is bogus as well.  I see no reason that
we shouldn't just allow SET STORAGE on foreign tables.  It's probably
not going to have any effect, but so what?  And again, if we did ever
find a use for that, we'd have a compatibility problem if inherited SET
STORAGE has a pre-existing behavior that it skips foreign children.

In the same vein, I'm planning to take out the existing prohibition on
marking CHECK constraints on foreign tables NOT VALID.  That likewise
creates a corner case for inheritance trees for no obviously good reason.
It was reasonable to be conservative about whether to allow that so long
as there were no side-effects; but putting warts into the behavior of
inheritance trees to preserve the prohibition is not a good outcome.

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] inherit support for foreign tables

2015-02-19 Thread Etsuro Fujita
On 2015/01/15 16:35, Etsuro Fujita wrote:
 On 2014/12/23 0:36, Tom Lane wrote:
 Yeah, we need to do something about the PlanRowMark data structure.
 Aside from the pre-existing issue in postgres_fdw, we need to fix it
 to support inheritance trees in which more than one rowmark method
 is being used.  That rte.hasForeignChildren thing is a crock,
 
 The idea I'd had about that was to convert the markType field into a
 bitmask, so that a parent node's markType could represent the logical
 OR of the rowmark methods being used by all its children.

 As I said before, that seems to me like a good idea.  So I'll update the
 patch based on that if you're okey with it.

Done based on your ideas: (a) add a field to PlanRowMark to record the
original lock strength to fix the postgres_fdw issue and (b) convert its
markType field into a bitmask to support the inheritance trees.  I think
that both work well and that (a) is useful for the other places.

Patch attached, which has been created on top of [1].

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/54bcbbf8.3020...@lab.ntt.co.jp
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 148,153  EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a  0;
--- 148,167 
  SELECT * FROM agg_csv WHERE a  0;
  RESET constraint_exclusion;
  
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+ SELECT tableoid::regclass, * FROM agg_csv;
+ SELECT tableoid::regclass, * FROM ONLY agg;
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ DELETE FROM agg WHERE a = 100;
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
+ 
  -- privilege tests
  SET ROLE file_fdw_superuser;
  SELECT * FROM agg_text ORDER BY a;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 249,254  SELECT * FROM agg_csv WHERE a  0;
--- 249,294 
  (0 rows)
  
  RESET constraint_exclusion;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+  tableoid |  a  |b
+ --+-+-
+  agg_csv  | 100 |  99.097
+  agg_csv  |   0 | 0.09561
+  agg_csv  |  42 |  324.78
+ (3 rows)
+ 
+ SELECT tableoid::regclass, * FROM agg_csv;
+  tableoid |  a  |b
+ --+-+-
+  agg_csv  | 100 |  99.097
+  agg_csv  |   0 | 0.09561
+  agg_csv  |  42 |  324.78
+ (3 rows)
+ 
+ SELECT tableoid::regclass, * FROM ONLY agg;
+  tableoid | a | b 
+ --+---+---
+ (0 rows)
+ 
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ ERROR:  cannot update foreign table agg_csv
+ DELETE FROM agg WHERE a = 100;
+ ERROR:  cannot delete from foreign table agg_csv
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+  tableoid |  a  |b
+ --+-+-
+  agg_csv  | 100 |  99.097
+  agg_csv  |   0 | 0.09561
+  agg_csv  |  42 |  324.78
+ (3 rows)
+ 
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
  -- privilege tests
  SET ROLE file_fdw_superuser;
  SELECT * FROM agg_text ORDER BY a;
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 3027,3032  NOTICE:  NEW: (13,test triggered !)
--- 3027,3544 
  (1 row)
  
  -- ===
+ -- test inheritance features
+ -- ===
+ CREATE TABLE a (aa TEXT);
+ CREATE TABLE loct (aa TEXT, bb TEXT);
+ CREATE FOREIGN TABLE b (bb TEXT) INHERITS (a)
+   SERVER loopback OPTIONS (table_name 'loct');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO a(aa) VALUES('a');
+ INSERT INTO a(aa) VALUES('aa');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('b');
+ INSERT INTO b(aa) VALUES('bb');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ SELECT relname, a.* FROM a, pg_class where a.tableoid = pg_class.oid;
+  relname |aa
+ -+--
+  a   | aaa
+  a   | 
+  a   | a
+  a   | aa
+  a   | aaa
+  a   | 
+  b   | bbb
+  b   | 
+  b   | b
+  b   | bb
+  b   | bbb
+  b   | 
+ (12 rows)
+ 
+ SELECT relname, b.* FROM b, pg_class where b.tableoid = pg_class.oid;
+  relname |aa| bb 
+ -+--+
+  b   | bbb  | 
+  b   |  | 
+  b   | b| 
+  b   | bb   | 
+  b   | bbb  | 
+  b   | 

Re: [HACKERS] inherit support for foreign tables

2015-01-14 Thread Etsuro Fujita
On 2014/12/23 0:36, Tom Lane wrote:
 Yeah, we need to do something about the PlanRowMark data structure.
 Aside from the pre-existing issue in postgres_fdw, we need to fix it
 to support inheritance trees in which more than one rowmark method
 is being used.  That rte.hasForeignChildren thing is a crock,

 The idea I'd had about that was to convert the markType field into a
 bitmask, so that a parent node's markType could represent the logical
 OR of the rowmark methods being used by all its children.  I've not
 attempted to code this up though, and probably won't get to it until
 after Christmas.  One thing that's not clear is what should happen
 with ExecRowMark.

As I said before, that seems to me like a good idea.  So I'll update the
patch based on that if you're okey with it.  Or you've found any problem
concerning the above idea?

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] inherit support for foreign tables

2014-12-25 Thread Etsuro Fujita
On 2014/12/23 0:36, Tom Lane wrote:
 Yeah, we need to do something about the PlanRowMark data structure.
 Aside from the pre-existing issue in postgres_fdw, we need to fix it
 to support inheritance trees in which more than one rowmark method
 is being used.  That rte.hasForeignChildren thing is a crock, and
 would still be a crock even if it were correctly documented as being
 a planner temporary variable (rather than the current implication that
 it's always valid).  RangeTblEntry is no place for planner temporaries.

Agreed.

 The idea I'd had about that was to convert the markType field into a
 bitmask, so that a parent node's markType could represent the logical
 OR of the rowmark methods being used by all its children.  I've not
 attempted to code this up though, and probably won't get to it until
 after Christmas.  One thing that's not clear is what should happen
 with ExecRowMark.

That seems like a good idea, as parent PlanRowMarks are ignored at runtime.

Aside from the above, I noticed that the patch has a bug in handling
ExecRowMarks/ExecAuxRowMarks for foreign tables in inheritance trees
during the EPQ processing.:-(  Attached is an updated version of the
patch to fix that, which has been created on top of [1], as said before.

Thanks,

[1] http://www.postgresql.org/message-id/5497bf4c.6080...@lab.ntt.co.jp

Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 148,153  EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a  0;
--- 148,167 
  SELECT * FROM agg_csv WHERE a  0;
  RESET constraint_exclusion;
  
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+ SELECT tableoid::regclass, * FROM agg_csv;
+ SELECT tableoid::regclass, * FROM ONLY agg;
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ DELETE FROM agg WHERE a = 100;
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
+ 
  -- privilege tests
  SET ROLE file_fdw_superuser;
  SELECT * FROM agg_text ORDER BY a;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 249,254  SELECT * FROM agg_csv WHERE a  0;
--- 249,294 
  (0 rows)
  
  RESET constraint_exclusion;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+  tableoid |  a  |b
+ --+-+-
+  agg_csv  | 100 |  99.097
+  agg_csv  |   0 | 0.09561
+  agg_csv  |  42 |  324.78
+ (3 rows)
+ 
+ SELECT tableoid::regclass, * FROM agg_csv;
+  tableoid |  a  |b
+ --+-+-
+  agg_csv  | 100 |  99.097
+  agg_csv  |   0 | 0.09561
+  agg_csv  |  42 |  324.78
+ (3 rows)
+ 
+ SELECT tableoid::regclass, * FROM ONLY agg;
+  tableoid | a | b 
+ --+---+---
+ (0 rows)
+ 
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ ERROR:  cannot update foreign table agg_csv
+ DELETE FROM agg WHERE a = 100;
+ ERROR:  cannot delete from foreign table agg_csv
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+  tableoid |  a  |b
+ --+-+-
+  agg_csv  | 100 |  99.097
+  agg_csv  |   0 | 0.09561
+  agg_csv  |  42 |  324.78
+ (3 rows)
+ 
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
  -- privilege tests
  SET ROLE file_fdw_superuser;
  SELECT * FROM agg_text ORDER BY a;
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 3027,3032  NOTICE:  NEW: (13,test triggered !)
--- 3027,3544 
  (1 row)
  
  -- ===
+ -- test inheritance features
+ -- ===
+ CREATE TABLE a (aa TEXT);
+ CREATE TABLE loct (aa TEXT, bb TEXT);
+ CREATE FOREIGN TABLE b (bb TEXT) INHERITS (a)
+   SERVER loopback OPTIONS (table_name 'loct');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO a(aa) VALUES('a');
+ INSERT INTO a(aa) VALUES('aa');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('b');
+ INSERT INTO b(aa) VALUES('bb');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ SELECT relname, a.* FROM a, pg_class where a.tableoid = pg_class.oid;
+  relname |aa
+ -+--
+  a   | aaa
+  a   | 
+  a   | a
+  a   | aa
+  a   | aaa
+  a   | 
+  b   | bbb
+  b   | 
+  b   | b
+  b   | bb
+  b   | bbb
+  b   | 
+ (12 rows)
+ 
+ 

Re: [HACKERS] inherit support for foreign tables

2014-12-22 Thread Etsuro Fujita
On 2014/12/18 7:04, Tom Lane wrote:
 Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 Attached are updated patches.  Patch fdw-inh-5.patch has been created on
 top of patch fdw-chk-5.patch.

 I've committed fdw-chk-5.patch with some minor further adjustments;

 Have not looked at the other patch yet.

I updated the remaining patch correspondingly to the fix [1].  Please
find attached a patch (the patch has been created on top of the patch in
[1]).

I haven't done anything about the issue that postgresGetForeignPlan() is
using get_parse_rowmark(), discussed in eg, [2].  Tom, will you work on
that?

Thanks,

[1] http://www.postgresql.org/message-id/5497bf4c.6080...@lab.ntt.co.jp
[2] http://www.postgresql.org/message-id/18256.1418401...@sss.pgh.pa.us

Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 148,153  EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a  0;
--- 148,167 
  SELECT * FROM agg_csv WHERE a  0;
  RESET constraint_exclusion;
  
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+ SELECT tableoid::regclass, * FROM agg_csv;
+ SELECT tableoid::regclass, * FROM ONLY agg;
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ DELETE FROM agg WHERE a = 100;
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
+ 
  -- privilege tests
  SET ROLE file_fdw_superuser;
  SELECT * FROM agg_text ORDER BY a;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 249,254  SELECT * FROM agg_csv WHERE a  0;
--- 249,294 
  (0 rows)
  
  RESET constraint_exclusion;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+  tableoid |  a  |b
+ --+-+-
+  agg_csv  | 100 |  99.097
+  agg_csv  |   0 | 0.09561
+  agg_csv  |  42 |  324.78
+ (3 rows)
+ 
+ SELECT tableoid::regclass, * FROM agg_csv;
+  tableoid |  a  |b
+ --+-+-
+  agg_csv  | 100 |  99.097
+  agg_csv  |   0 | 0.09561
+  agg_csv  |  42 |  324.78
+ (3 rows)
+ 
+ SELECT tableoid::regclass, * FROM ONLY agg;
+  tableoid | a | b 
+ --+---+---
+ (0 rows)
+ 
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ ERROR:  cannot update foreign table agg_csv
+ DELETE FROM agg WHERE a = 100;
+ ERROR:  cannot delete from foreign table agg_csv
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+  tableoid |  a  |b
+ --+-+-
+  agg_csv  | 100 |  99.097
+  agg_csv  |   0 | 0.09561
+  agg_csv  |  42 |  324.78
+ (3 rows)
+ 
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
  -- privilege tests
  SET ROLE file_fdw_superuser;
  SELECT * FROM agg_text ORDER BY a;
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 3027,3032  NOTICE:  NEW: (13,test triggered !)
--- 3027,3544 
  (1 row)
  
  -- ===
+ -- test inheritance features
+ -- ===
+ CREATE TABLE a (aa TEXT);
+ CREATE TABLE loct (aa TEXT, bb TEXT);
+ CREATE FOREIGN TABLE b (bb TEXT) INHERITS (a)
+   SERVER loopback OPTIONS (table_name 'loct');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO a(aa) VALUES('a');
+ INSERT INTO a(aa) VALUES('aa');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('b');
+ INSERT INTO b(aa) VALUES('bb');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ SELECT relname, a.* FROM a, pg_class where a.tableoid = pg_class.oid;
+  relname |aa
+ -+--
+  a   | aaa
+  a   | 
+  a   | a
+  a   | aa
+  a   | aaa
+  a   | 
+  b   | bbb
+  b   | 
+  b   | b
+  b   | bb
+  b   | bbb
+  b   | 
+ (12 rows)
+ 
+ SELECT relname, b.* FROM b, pg_class where b.tableoid = pg_class.oid;
+  relname |aa| bb 
+ -+--+
+  b   | bbb  | 
+  b   |  | 
+  b   | b| 
+  b   | bb   | 
+  b   | bbb  | 
+  b   |  | 
+ (6 rows)
+ 
+ SELECT relname, a.* FROM ONLY a, pg_class where a.tableoid = pg_class.oid;
+  relname |aa
+ -+--
+  a   | aaa
+  a   | 
+  a   | a
+  a   | aa
+  a   | aaa
+  a   | 
+ (6 rows)
+ 
+ UPDATE a SET aa='zz' WHERE aa LIKE 

Re: [HACKERS] inherit support for foreign tables

2014-12-22 Thread Tom Lane
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 I haven't done anything about the issue that postgresGetForeignPlan() is
 using get_parse_rowmark(), discussed in eg, [2].  Tom, will you work on
 that?

Yeah, we need to do something about the PlanRowMark data structure.
Aside from the pre-existing issue in postgres_fdw, we need to fix it
to support inheritance trees in which more than one rowmark method
is being used.  That rte.hasForeignChildren thing is a crock, and
would still be a crock even if it were correctly documented as being
a planner temporary variable (rather than the current implication that
it's always valid).  RangeTblEntry is no place for planner temporaries.

The idea I'd had about that was to convert the markType field into a
bitmask, so that a parent node's markType could represent the logical
OR of the rowmark methods being used by all its children.  I've not
attempted to code this up though, and probably won't get to it until
after Christmas.  One thing that's not clear is what should happen
with ExecRowMark.

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] inherit support for foreign tables

2014-12-17 Thread Tom Lane
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 Attached are updated patches.  Patch fdw-inh-5.patch has been created on 
 top of patch fdw-chk-5.patch.  Patch fdw-chk-5.patch is basically the 
 same as the previous one fdw-chk-4.patch, but I slightly modified that 
 one.  The changes are the following.
 * added to foreign_data.sql more tests for your comments.
 * revised docs on ALTER FOREIGN TABLE a bit further.

I've committed fdw-chk-5.patch with some minor further adjustments;
the most notable one was that I got rid of the error check prohibiting
NO INHERIT, which did not seem to me to have any value.  Attaching such
a clause won't have any effect, but so what?

Have not looked at the other patch yet.

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] inherit support for foreign tables

2014-12-17 Thread Etsuro Fujita
(2014/12/18 7:04), Tom Lane wrote:
 Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 Attached are updated patches.  Patch fdw-inh-5.patch has been created on
 top of patch fdw-chk-5.patch.  Patch fdw-chk-5.patch is basically the
 same as the previous one fdw-chk-4.patch, but I slightly modified that
 one.  The changes are the following.
 * added to foreign_data.sql more tests for your comments.
 * revised docs on ALTER FOREIGN TABLE a bit further.
 
 I've committed fdw-chk-5.patch with some minor further adjustments;
 the most notable one was that I got rid of the error check prohibiting
 NO INHERIT, which did not seem to me to have any value.  Attaching such
 a clause won't have any effect, but so what?
 
 Have not looked at the other patch yet.

Thanks!

I added the error check because the other patch, fdw-inh-5.patch,
doesn't allow foreign tables to be inherited and so it seems more
consistent at least to me to do so.

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] inherit support for foreign tables

2014-12-14 Thread Michael Paquier
On Thu, Dec 11, 2014 at 2:54 PM, Ashutosh Bapat
ashutosh.ba...@enterprisedb.com wrote:
 On Thu, Dec 11, 2014 at 8:39 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
 wrote:

 Hi Ashutosh,

 Thanks for the review!

 (2014/12/10 14:47), Ashutosh Bapat wrote:

 We haven't heard anything from Horiguchi-san and Hanada-san for almost a
 week. So, I am fine marking it as ready for committer. What do you say?
Moving this patch to CF 2014-12 with the same status. Let's get a
committer having a look at it.
-- 
Michael


-- 
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] inherit support for foreign tables

2014-12-11 Thread Etsuro Fujita

(2014/12/11 14:54), Ashutosh Bapat wrote:

I marked this as ready for committer.


Many thanks!

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] inherit support for foreign tables

2014-12-10 Thread Etsuro Fujita

Hi Ashutosh,

Thanks for the review!

(2014/12/10 14:47), Ashutosh Bapat wrote:

We haven't heard anything from Horiguchi-san and Hanada-san for almost a
week. So, I am fine marking it as ready for committer. What do you say?


ISTM that both of them are not against us, so let's ask for the 
committer's review!


Thanks,

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] inherit support for foreign tables

2014-12-10 Thread Ashutosh Bapat
I marked this as ready for committer.

On Thu, Dec 11, 2014 at 8:39 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 Hi Ashutosh,

 Thanks for the review!

 (2014/12/10 14:47), Ashutosh Bapat wrote:

 We haven't heard anything from Horiguchi-san and Hanada-san for almost a
 week. So, I am fine marking it as ready for committer. What do you say?


 ISTM that both of them are not against us, so let's ask for the
 committer's review!


 Thanks,

 Best regards,
 Etsuro Fujita




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


Re: [HACKERS] inherit support for foreign tables

2014-12-09 Thread Etsuro Fujita

Hi Ashutosh,

Thanks for the review!

(2014/11/28 18:14), Ashutosh Bapat wrote:

On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
(2014/11/17 17:55), Ashutosh Bapat wrote:
Here are my review comments for patch fdw-inh-3.patch.



Tests
---
1. It seems like you have copied from testcase inherit.sql to
postgres_fdw testcase. That's a good thing, but it makes the
test quite
long. May be we should have two tests in postgres_fdw contrib
module,
one for simple cases, and other for inheritance. What do you say?



IMO, the test is not so time-consuming, so it doesn't seem worth
splitting it into two.



I am not worried about the timing but I am worried about the length of
the file and hence ease of debugging in case we find any issues there.
We will leave that for the commiter to decide.


OK


Documentation

1. The change in ddl.sgml
-We will refer to the child tables as partitions, though
they
-are in every way normal productnamePostgreSQL/ tables.
+We will refer to the child tables as partitions, though
we assume
+that they are normal productnamePostgreSQL/ tables.

adds phrase we assume that, which confuses the intention
behind the
sentence. The original text is intended to highlight the equivalence
between partition and normal table, where as the addition
esp. the
word assume weakens that equivalence. Instead now we have to
highlight
the equivalence between partition and normal or foreign
table. The
wording similar to though they are either normal or foreign tables
should be used there.



You are right, but I feel that there is something out of place in
saying that there (5.10.2. Implementing Partitioning) because the
procedure there has been written based on normal tables only.  Put
another way, if we say that, I think we'd need to add more docs,
describing the syntax and/or the corresponding examples for
foreign-table cases.  But I'd like to leave that for another patch.
So, how about the wording we assume *here* that, instead of we
assume that, as I added the following notice in the previous
section (5.10.1. Overview)?

@@ -2650,7 +2669,10 @@ VALUES ('Albany', NULL, NULL, 'NY');
  table of a single parent table.  The parent table itself is
normally
  empty; it exists just to represent the entire data set.  You
should be
  familiar with inheritance (see xref linkend=ddl-inherit)
before
-attempting to set up partitioning.
+attempting to set up partitioning.  (The setup and management of
+partitioned tables illustrated in this section assume that each
+partition is a normal table.  However, you can do that in a
similar way
+for cases where some or all partitions are foreign tables.)



This looks ok, though, I would like to see final version of the
document. But I think, we will leave that for committer to handle.


OK


2. The wording some kind of optimization gives vague picture.
May be
it can be worded as Since the constraints are assumed to be
true, they
are used in constraint-based query optimization like constraint
exclusion for partitioned tables..
+Those constraints are used in some kind of query
optimization such
+as constraint exclusion for partitioned tables (see
+xref linkend=ddl-partitioning).



Will follow your revision.


Done.


Code
---
1. In the following change
+/*
* acquire_inherited_sample_rows -- acquire sample rows from
inheritance tree
*
* This has the same API as acquire_sample_rows, except that
rows are
* collected from all inheritance children as well as the
specified table.
- * We fail and return zero if there are no inheritance children.
+ * We fail and return zero if there are no inheritance children or
there are
+ * inheritance children that foreign tables.

The addition should be there are inheritance children that *are all
*foreign tables. Note the addition are all.



Sorry, I incorrectly wrote the comment.  What I tried to write is
We fail and return zero if there are no inheritance children or if
we are not in VAC_MODE_SINGLE case and inheritance tree contains at
least one foreign table..



You might want to use English description of VAC_MODE_SINGLE instead
of that macro in the comment, so that reader doesn't have to look up
VAC_MODE_SINGLE. But I think, we will leave this for the committer.


I corrected the comments and 

Re: [HACKERS] inherit support for foreign tables

2014-12-09 Thread Ashutosh Bapat
We haven't heard anything from Horiguchi-san and Hanada-san for almost a
week. So, I am fine marking it as ready for committer. What do you say?


On Wed, Dec 10, 2014 at 8:48 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 Hi Ashutosh,

 Thanks for the review!

 (2014/11/28 18:14), Ashutosh Bapat wrote:

 On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
 (2014/11/17 17:55), Ashutosh Bapat wrote:
 Here are my review comments for patch fdw-inh-3.patch.


  Tests
 ---
 1. It seems like you have copied from testcase inherit.sql to
 postgres_fdw testcase. That's a good thing, but it makes the
 test quite
 long. May be we should have two tests in postgres_fdw contrib
 module,
 one for simple cases, and other for inheritance. What do you say?


  IMO, the test is not so time-consuming, so it doesn't seem worth
 splitting it into two.


  I am not worried about the timing but I am worried about the length of
 the file and hence ease of debugging in case we find any issues there.
 We will leave that for the commiter to decide.


 OK


  Documentation
 
 1. The change in ddl.sgml
 -We will refer to the child tables as partitions, though
 they
 -are in every way normal productnamePostgreSQL/
 tables.
 +We will refer to the child tables as partitions, though
 we assume
 +that they are normal productnamePostgreSQL/ tables.

 adds phrase we assume that, which confuses the intention
 behind the
 sentence. The original text is intended to highlight the
 equivalence
 between partition and normal table, where as the addition
 esp. the
 word assume weakens that equivalence. Instead now we have to
 highlight
 the equivalence between partition and normal or foreign
 table. The
 wording similar to though they are either normal or foreign
 tables
 should be used there.


  You are right, but I feel that there is something out of place in
 saying that there (5.10.2. Implementing Partitioning) because the
 procedure there has been written based on normal tables only.  Put
 another way, if we say that, I think we'd need to add more docs,
 describing the syntax and/or the corresponding examples for
 foreign-table cases.  But I'd like to leave that for another patch.
 So, how about the wording we assume *here* that, instead of we
 assume that, as I added the following notice in the previous
 section (5.10.1. Overview)?

 @@ -2650,7 +2669,10 @@ VALUES ('Albany', NULL, NULL, 'NY');
   table of a single parent table.  The parent table itself is
 normally
   empty; it exists just to represent the entire data set.  You
 should be
   familiar with inheritance (see xref linkend=ddl-inherit)
 before
 -attempting to set up partitioning.
 +attempting to set up partitioning.  (The setup and management of
 +partitioned tables illustrated in this section assume that each
 +partition is a normal table.  However, you can do that in a
 similar way
 +for cases where some or all partitions are foreign tables.)


  This looks ok, though, I would like to see final version of the
 document. But I think, we will leave that for committer to handle.


 OK

  2. The wording some kind of optimization gives vague picture.
 May be
 it can be worded as Since the constraints are assumed to be
 true, they
 are used in constraint-based query optimization like constraint
 exclusion for partitioned tables..
 +Those constraints are used in some kind of query
 optimization such
 +as constraint exclusion for partitioned tables (see
 +xref linkend=ddl-partitioning).


  Will follow your revision.


 Done.

  Code
 ---
 1. In the following change
 +/*
 * acquire_inherited_sample_rows -- acquire sample rows from
 inheritance tree
 *
 * This has the same API as acquire_sample_rows, except that
 rows are
 * collected from all inheritance children as well as the
 specified table.
 - * We fail and return zero if there are no inheritance children.
 + * We fail and return zero if there are no inheritance children
 or
 there are
 + * inheritance children that foreign tables.

 The addition should be there are inheritance children that *are
 all
 *foreign tables. Note the addition are all.


  Sorry, I incorrectly wrote the comment.  What I tried to write is
 We fail and return zero if there are no inheritance children or if
 we are 

Re: [HACKERS] inherit support for foreign tables

2014-12-08 Thread Etsuro Fujita

(2014/12/08 15:17), Ashutosh Bapat wrote:

On Sat, Dec 6, 2014 at 9:21 PM, Noah Misch n...@leadboat.com
mailto:n...@leadboat.com wrote:
Does this inheritance patch add any
atomicity
problem that goes away when one breaks up the inheritance hierarchy and
UPDATEs each table separately?  If not, this limitation is okay.



If the UPDATES crafted after breaking up the inheritance hierarchy are
needed to be run within the same transaction (as the UPDATE on
inheritance hierarchy would do), yes, there is atomicity problem.


ISTM that your concern would basically a known problem.  Consider the 
following transaction.


BEGIN TRANSACTION;
UPDATE foo SET a = 100;  -- updates on table foo in remote server1
UPDATE bar SET a = 100;  -- updates on table bar in remote server2
COMMIT TRANSACTION;

This transaction would cause the atomicity problem if 
pgfdw_xact_callback() for XACT_EVENT_PRE_COMMIT for foo succeeded and 
then that for bar failed during CommitTransaction().


Thanks,

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] inherit support for foreign tables

2014-12-07 Thread Etsuro Fujita

(2014/12/07 2:02), David Fetter wrote:

On Thu, Dec 04, 2014 at 12:35:54PM +0900, Etsuro Fujita wrote:

But I think
there would be another idea.  An example will be shown below.  We show the
update commands below the ModifyTable node, not above the corresponding
ForeignScan nodes, so maybe less confusing.  If there are no objections of
you and others, I'll update the patch this way.

postgres=# explain verbose update parent set a = a * 2 where a = 5;
  QUERY PLAN
-
  Update on public.parent  (cost=0.00..280.77 rows=25 width=10)
On public.ft1
  Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1

^^
It occurs to me that the command generated by the FDW might well not
be SQL at all, as is the case with file_fdw and anything else that
talks to a NoSQL engine.

Would it be reasonable to call this Remote command or something
similarly generic?


Yeah, but I'd like to propose that this line is shown by the FDW API 
(ie, ExplainForeignModify) as in non-inherited update cases, so that the 
FDW developer can choose the right name.  As for On public.ft1, I'd 
like to propose that the FDW API also show that by calling a function 
for that introduced into the PG core (Would it be better to use For 
rather than On?).


Sorry, my explanation was not enough.

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] inherit support for foreign tables

2014-12-07 Thread Ashutosh Bapat
On Sat, Dec 6, 2014 at 9:21 PM, Noah Misch n...@leadboat.com wrote:

 On Thu, Dec 04, 2014 at 10:00:14AM +0530, Ashutosh Bapat wrote:
  On Thu, Dec 4, 2014 at 9:05 AM, Etsuro Fujita 
 fujita.ets...@lab.ntt.co.jp wrote:
   (2014/12/03 19:35), Ashutosh Bapat wrote:
   On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita
   fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp
 wrote:
IIUC, even the transactions over the local and the *single* remote
   server are not guaranteed to be executed atomically in the current
   form.  It is possible that the remote transaction succeeds and the
   local one fails, for example, resulting in data inconsistency
   between the local and the remote.
  
  
IIUC, while committing transactions involving a single remote server,
   the steps taken are as follows
   1. the local changes are brought to PRE-COMMIT stage, which means that
   the transaction *will* succeed locally after successful completion of
   this phase,
   2. COMMIT message is sent to the foreign server
   3. If step two succeeds, local changes are committed and successful
   commit is conveyed to the client
   4. if step two fails, local changes are rolled back and abort status
 is
   conveyed to the client
   5. If step 1 itself fails, the remote changes are rolled back.
   This is as per one phase commit protocol which guarantees ACID for
   single foreign data source. So, the changes involving local and a
 single
   foreign server seem to be atomic and consistent.
  
  
   Really?  Maybe I'm missing something, but I don't think the current
   implementation for committing transactions has such a mechanism stated
 in
   step 1.  So, I think it's possible that the local transaction fails in
   step3 while the remote transaction succeeds, as mentioned above.
  
  
  PFA a script attached which shows this. You may want to check the code in
  pgfdw_xact_callback() for actions taken by postgres_fdw on various
 events.
  CommitTransaction() for how those events are generated. The code there
  complies with the sequence above.

 While postgres_fdw delays remote commit to eliminate many causes for having
 one server commit while another aborts, other causes remain.  The local
 transaction could still fail due to WAL I/O problems or a system crash.
 2PC
 is the reliable answer, but that was explicitly out of scope for the
 initial
 postgres_fdw write support.  Does this inheritance patch add any atomicity
 problem that goes away when one breaks up the inheritance hierarchy and
 UPDATEs each table separately?  If not, this limitation is okay.


If the UPDATES crafted after breaking up the inheritance hierarchy are
needed to be run within the same transaction (as the UPDATE on inheritance
hierarchy would do), yes, there is atomicity problem.

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


Re: [HACKERS] inherit support for foreign tables

2014-12-06 Thread Noah Misch
On Thu, Dec 04, 2014 at 10:00:14AM +0530, Ashutosh Bapat wrote:
 On Thu, Dec 4, 2014 at 9:05 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp 
 wrote:
  (2014/12/03 19:35), Ashutosh Bapat wrote:
  On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita
  fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
   IIUC, even the transactions over the local and the *single* remote
  server are not guaranteed to be executed atomically in the current
  form.  It is possible that the remote transaction succeeds and the
  local one fails, for example, resulting in data inconsistency
  between the local and the remote.
 
 
   IIUC, while committing transactions involving a single remote server,
  the steps taken are as follows
  1. the local changes are brought to PRE-COMMIT stage, which means that
  the transaction *will* succeed locally after successful completion of
  this phase,
  2. COMMIT message is sent to the foreign server
  3. If step two succeeds, local changes are committed and successful
  commit is conveyed to the client
  4. if step two fails, local changes are rolled back and abort status is
  conveyed to the client
  5. If step 1 itself fails, the remote changes are rolled back.
  This is as per one phase commit protocol which guarantees ACID for
  single foreign data source. So, the changes involving local and a single
  foreign server seem to be atomic and consistent.
 
 
  Really?  Maybe I'm missing something, but I don't think the current
  implementation for committing transactions has such a mechanism stated in
  step 1.  So, I think it's possible that the local transaction fails in
  step3 while the remote transaction succeeds, as mentioned above.
 
 
 PFA a script attached which shows this. You may want to check the code in
 pgfdw_xact_callback() for actions taken by postgres_fdw on various events.
 CommitTransaction() for how those events are generated. The code there
 complies with the sequence above.

While postgres_fdw delays remote commit to eliminate many causes for having
one server commit while another aborts, other causes remain.  The local
transaction could still fail due to WAL I/O problems or a system crash.  2PC
is the reliable answer, but that was explicitly out of scope for the initial
postgres_fdw write support.  Does this inheritance patch add any atomicity
problem that goes away when one breaks up the inheritance hierarchy and
UPDATEs each table separately?  If not, this limitation is okay.


-- 
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] inherit support for foreign tables

2014-12-06 Thread David Fetter
On Thu, Dec 04, 2014 at 12:35:54PM +0900, Etsuro Fujita wrote:
 (2014/12/03 19:35), Ashutosh Bapat wrote:
 On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
 
 This is not exactly extension of non-inheritance case. non-inheritance
 case doesn't show two remote SQLs under the same plan node. May be you
 can rename the label Remote SQL as Remote UPDATE/INSERT/DELETE (or
 something to that effect) for the DML command and the Foreign plan node
 should be renamed to Foreign access node or something to indicate that
 it does both the scan as well as DML. I am not keen about the actual
 terminology, but I think a reader of plan shouldn't get confused.
 
 We can leave this for committer's judgement.
 
 Thanks for the proposal!  I think that would be a good idea.  But I think
 there would be another idea.  An example will be shown below.  We show the
 update commands below the ModifyTable node, not above the corresponding
 ForeignScan nodes, so maybe less confusing.  If there are no objections of
 you and others, I'll update the patch this way.
 
 postgres=# explain verbose update parent set a = a * 2 where a = 5;
  QUERY PLAN
 -
  Update on public.parent  (cost=0.00..280.77 rows=25 width=10)
On public.ft1
  Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1
   ^^
It occurs to me that the command generated by the FDW might well not
be SQL at all, as is the case with file_fdw and anything else that
talks to a NoSQL engine.

Would it be reasonable to call this Remote command or something
similarly generic?

Cheers,
David.
-- 
David Fetter da...@fetter.org http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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] inherit support for foreign tables

2014-12-03 Thread Ashutosh Bapat
On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 (2014/11/28 18:14), Ashutosh Bapat wrote:

 On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
 Apart from the above, I noticed that the patch doesn't consider to
 call ExplainForeignModify during EXPLAIN for an inherited
 UPDATE/DELETE, as shown below (note that there are no UPDATE remote
 queries displayed):


  So, I'd like to modify explain.c to show those queries like this:


  postgres=# explain verbose update parent set a = a * 2 where a = 5;
   QUERY PLAN
 --__
 --__-
   Update on public.parent  (cost=0.00..280.77 rows=25 width=10)
 -  Seq Scan on public.parent  (cost=0.00..0.00 rows=1 width=10)
   Output: (parent.a * 2), parent.ctid
   Filter: (parent.a = 5)
 Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1
 -  Foreign Scan on public.ft1  (cost=100.00..140.38 rows=12
 width=10)
   Output: (ft1.a * 2), ft1.ctid
   Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a
 = 5)) FOR UPDATE
 Remote SQL: UPDATE public.mytable_2 SET a = $2 WHERE ctid = $1
 -  Foreign Scan on public.ft2  (cost=100.00..140.38 rows=12
 width=10)
   Output: (ft2.a * 2), ft2.ctid
   Remote SQL: SELECT a, ctid FROM public.mytable_2 WHERE ((a
 = 5)) FOR UPDATE
 (12 rows)


  Two remote SQL under a single node would be confusing. Also the node
 is labelled as Foreign Scan. It would be confusing to show an UPDATE
 command under this scan node.


 I thought this as an extention of the existing (ie, non-inherited) case
 (see the below example) to the inherited case.

 postgres=# explain verbose update ft1 set a = a * 2 where a = 5;
  QUERY PLAN
 
 -
  Update on public.ft1  (cost=100.00..140.38 rows=12 width=10)
Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1
-  Foreign Scan on public.ft1  (cost=100.00..140.38 rows=12 width=10)
  Output: (a * 2), ctid
  Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5))
 FOR UPDATE
 (5 rows)

 I think we should show update commands somewhere for the inherited case as
 for the non-inherited case.  Alternatives to this are welcome.

This is not exactly extension of non-inheritance case. non-inheritance case
doesn't show two remote SQLs under the same plan node. May be you can
rename the label Remote SQL as Remote UPDATE/INSERT/DELETE (or something to
that effect) for the DML command and the Foreign plan node should be
renamed to Foreign access node or something to indicate that it does both
the scan as well as DML. I am not keen about the actual terminology, but I
think a reader of plan shouldn't get confused.

We can leave this for committer's judgement.


  BTW, I was experimenting with DMLs being executed on multiple FDW server
 under same transaction and found that the transactions may not be atomic
 (and may be inconsistent), if one or more of the server fails to commit
 while rest of them commit the transaction. The reason for this is, we do
 not rollback the already committed changes to the foreign server, if
 one or more of them fail to commit a transaction. With foreign tables
 under inheritance hierarchy a single DML can span across multiple
 servers and the result may not be atomic (and may be inconsistent). So,


 IIUC, even the transactions over the local and the *single* remote server
 are not guaranteed to be executed atomically in the current form.  It is
 possible that the remote transaction succeeds and the local one fails, for
 example, resulting in data inconsistency between the local and the remote.


IIUC, while committing transactions involving a single remote server, the
steps taken are as follows
1. the local changes are brought to PRE-COMMIT stage, which means that the
transaction *will* succeed locally after successful completion of this
phase,
2. COMMIT message is sent to the foreign server
3. If step two succeeds, local changes are committed and successful commit
is conveyed to the client
4. if step two fails, local changes are rolled back and abort status is
conveyed to the client
5. If step 1 itself fails, the remote changes are rolled back.
This is as per one phase commit protocol which guarantees ACID for single
foreign data source. So, the changes involving local and a single foreign
server seem to be atomic and consistent.


  either we have to disable DMLs on an inheritance hierarchy which spans
 multiple servers. OR make sure that such transactions follow 2PC norms.


 -1 for disabling update queries on such an inheritance hierarchy because I
 think we 

Re: [HACKERS] inherit support for foreign tables

2014-12-03 Thread Ashutosh Bapat
On Wed, Dec 3, 2014 at 4:05 PM, Ashutosh Bapat 
ashutosh.ba...@enterprisedb.com wrote:



 On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
  wrote:

 (2014/11/28 18:14), Ashutosh Bapat wrote:

 On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp
 wrote:
 Apart from the above, I noticed that the patch doesn't consider to
 call ExplainForeignModify during EXPLAIN for an inherited
 UPDATE/DELETE, as shown below (note that there are no UPDATE remote
 queries displayed):


  So, I'd like to modify explain.c to show those queries like this:


  postgres=# explain verbose update parent set a = a * 2 where a = 5;
   QUERY PLAN
 --__
 --__-
   Update on public.parent  (cost=0.00..280.77 rows=25 width=10)
 -  Seq Scan on public.parent  (cost=0.00..0.00 rows=1 width=10)
   Output: (parent.a * 2), parent.ctid
   Filter: (parent.a = 5)
 Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1
 -  Foreign Scan on public.ft1  (cost=100.00..140.38 rows=12
 width=10)
   Output: (ft1.a * 2), ft1.ctid
   Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a
 = 5)) FOR UPDATE
 Remote SQL: UPDATE public.mytable_2 SET a = $2 WHERE ctid = $1
 -  Foreign Scan on public.ft2  (cost=100.00..140.38 rows=12
 width=10)
   Output: (ft2.a * 2), ft2.ctid
   Remote SQL: SELECT a, ctid FROM public.mytable_2 WHERE ((a
 = 5)) FOR UPDATE
 (12 rows)


  Two remote SQL under a single node would be confusing. Also the node
 is labelled as Foreign Scan. It would be confusing to show an UPDATE
 command under this scan node.


 I thought this as an extention of the existing (ie, non-inherited) case
 (see the below example) to the inherited case.

 postgres=# explain verbose update ft1 set a = a * 2 where a = 5;
  QUERY PLAN
 
 -
  Update on public.ft1  (cost=100.00..140.38 rows=12 width=10)
Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1
-  Foreign Scan on public.ft1  (cost=100.00..140.38 rows=12 width=10)
  Output: (a * 2), ctid
  Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5))
 FOR UPDATE
 (5 rows)

 I think we should show update commands somewhere for the inherited case
 as for the non-inherited case.  Alternatives to this are welcome.

 This is not exactly extension of non-inheritance case. non-inheritance
 case doesn't show two remote SQLs under the same plan node. May be you can
 rename the label Remote SQL as Remote UPDATE/INSERT/DELETE (or something to
 that effect) for the DML command and the Foreign plan node should be
 renamed to Foreign access node or something to indicate that it does both
 the scan as well as DML. I am not keen about the actual terminology, but I
 think a reader of plan shouldn't get confused.

 We can leave this for committer's judgement.


  BTW, I was experimenting with DMLs being executed on multiple FDW server
 under same transaction and found that the transactions may not be atomic
 (and may be inconsistent), if one or more of the server fails to commit
 while rest of them commit the transaction. The reason for this is, we do
 not rollback the already committed changes to the foreign server, if
 one or more of them fail to commit a transaction. With foreign tables
 under inheritance hierarchy a single DML can span across multiple
 servers and the result may not be atomic (and may be inconsistent). So,


 IIUC, even the transactions over the local and the *single* remote server
 are not guaranteed to be executed atomically in the current form.  It is
 possible that the remote transaction succeeds and the local one fails, for
 example, resulting in data inconsistency between the local and the remote.


 IIUC, while committing transactions involving a single remote server, the
 steps taken are as follows
 1. the local changes are brought to PRE-COMMIT stage, which means that the
 transaction *will* succeed locally after successful completion of this
 phase,
 2. COMMIT message is sent to the foreign server
 3. If step two succeeds, local changes are committed and successful commit
 is conveyed to the client
 4. if step two fails, local changes are rolled back and abort status is
 conveyed to the client
 5. If step 1 itself fails, the remote changes are rolled back.
 This is as per one phase commit protocol which guarantees ACID for single
 foreign data source. So, the changes involving local and a single foreign
 server seem to be atomic and consistent.


  either we have to disable DMLs on an inheritance hierarchy which spans
 multiple servers. OR make sure that such 

Re: [HACKERS] inherit support for foreign tables

2014-12-03 Thread Ashutosh Bapat
On Thu, Dec 4, 2014 at 9:05 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 (2014/12/03 19:35), Ashutosh Bapat wrote:

 On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:


  This is not exactly extension of non-inheritance case. non-inheritance
 case doesn't show two remote SQLs under the same plan node. May be you
 can rename the label Remote SQL as Remote UPDATE/INSERT/DELETE (or
 something to that effect) for the DML command and the Foreign plan node
 should be renamed to Foreign access node or something to indicate that
 it does both the scan as well as DML. I am not keen about the actual
 terminology, but I think a reader of plan shouldn't get confused.

 We can leave this for committer's judgement.


 Thanks for the proposal!  I think that would be a good idea.  But I think
 there would be another idea.  An example will be shown below.  We show the
 update commands below the ModifyTable node, not above the corresponding
 ForeignScan nodes, so maybe less confusing.  If there are no objections of
 you and others, I'll update the patch this way.

 postgres=# explain verbose update parent set a = a * 2 where a = 5;
  QUERY PLAN
 
 -
  Update on public.parent  (cost=0.00..280.77 rows=25 width=10)
On public.ft1
  Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1
On public.ft2
  Remote SQL: UPDATE public.mytable_2 SET a = $2 WHERE ctid = $1
-  Seq Scan on public.parent  (cost=0.00..0.00 rows=1 width=10)
  Output: (parent.a * 2), parent.ctid
  Filter: (parent.a = 5)
-  Foreign Scan on public.ft1  (cost=100.00..140.38 rows=12 width=10)
  Output: (ft1.a * 2), ft1.ctid
  Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5))
 FOR UPDATE
-  Foreign Scan on public.ft2  (cost=100.00..140.38 rows=12 width=10)
  Output: (ft2.a * 2), ft2.ctid
  Remote SQL: SELECT a, ctid FROM public.mytable_2 WHERE ((a = 5))
 FOR UPDATE
 (12 rows)


Looks better.


  IIUC, even the transactions over the local and the *single* remote
 server are not guaranteed to be executed atomically in the current
 form.  It is possible that the remote transaction succeeds and the
 local one fails, for example, resulting in data inconsistency
 between the local and the remote.


  IIUC, while committing transactions involving a single remote server,
 the steps taken are as follows
 1. the local changes are brought to PRE-COMMIT stage, which means that
 the transaction *will* succeed locally after successful completion of
 this phase,
 2. COMMIT message is sent to the foreign server
 3. If step two succeeds, local changes are committed and successful
 commit is conveyed to the client
 4. if step two fails, local changes are rolled back and abort status is
 conveyed to the client
 5. If step 1 itself fails, the remote changes are rolled back.
 This is as per one phase commit protocol which guarantees ACID for
 single foreign data source. So, the changes involving local and a single
 foreign server seem to be atomic and consistent.


 Really?  Maybe I'm missing something, but I don't think the current
 implementation for committing transactions has such a mechanism stated in
 step 1.  So, I think it's possible that the local transaction fails in
 step3 while the remote transaction succeeds, as mentioned above.


PFA a script attached which shows this. You may want to check the code in
pgfdw_xact_callback() for actions taken by postgres_fdw on various events.
CommitTransaction() for how those events are generated. The code there
complies with the sequence above.



 Thanks,

 Best regards,
 Etsuro Fujita




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


Re: [HACKERS] inherit support for foreign tables

2014-12-03 Thread Ashutosh Bapat
Sorry, here's the script.

On Thu, Dec 4, 2014 at 10:00 AM, Ashutosh Bapat 
ashutosh.ba...@enterprisedb.com wrote:



 On Thu, Dec 4, 2014 at 9:05 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
  wrote:

 (2014/12/03 19:35), Ashutosh Bapat wrote:

 On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp
 wrote:


  This is not exactly extension of non-inheritance case. non-inheritance
 case doesn't show two remote SQLs under the same plan node. May be you
 can rename the label Remote SQL as Remote UPDATE/INSERT/DELETE (or
 something to that effect) for the DML command and the Foreign plan node
 should be renamed to Foreign access node or something to indicate that
 it does both the scan as well as DML. I am not keen about the actual
 terminology, but I think a reader of plan shouldn't get confused.

 We can leave this for committer's judgement.


 Thanks for the proposal!  I think that would be a good idea.  But I think
 there would be another idea.  An example will be shown below.  We show the
 update commands below the ModifyTable node, not above the corresponding
 ForeignScan nodes, so maybe less confusing.  If there are no objections of
 you and others, I'll update the patch this way.

 postgres=# explain verbose update parent set a = a * 2 where a = 5;
  QUERY PLAN
 
 -
  Update on public.parent  (cost=0.00..280.77 rows=25 width=10)
On public.ft1
  Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1
On public.ft2
  Remote SQL: UPDATE public.mytable_2 SET a = $2 WHERE ctid = $1
-  Seq Scan on public.parent  (cost=0.00..0.00 rows=1 width=10)
  Output: (parent.a * 2), parent.ctid
  Filter: (parent.a = 5)
-  Foreign Scan on public.ft1  (cost=100.00..140.38 rows=12 width=10)
  Output: (ft1.a * 2), ft1.ctid
  Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5))
 FOR UPDATE
-  Foreign Scan on public.ft2  (cost=100.00..140.38 rows=12 width=10)
  Output: (ft2.a * 2), ft2.ctid
  Remote SQL: SELECT a, ctid FROM public.mytable_2 WHERE ((a = 5))
 FOR UPDATE
 (12 rows)


 Looks better.


  IIUC, even the transactions over the local and the *single* remote
 server are not guaranteed to be executed atomically in the current
 form.  It is possible that the remote transaction succeeds and the
 local one fails, for example, resulting in data inconsistency
 between the local and the remote.


  IIUC, while committing transactions involving a single remote server,
 the steps taken are as follows
 1. the local changes are brought to PRE-COMMIT stage, which means that
 the transaction *will* succeed locally after successful completion of
 this phase,
 2. COMMIT message is sent to the foreign server
 3. If step two succeeds, local changes are committed and successful
 commit is conveyed to the client
 4. if step two fails, local changes are rolled back and abort status is
 conveyed to the client
 5. If step 1 itself fails, the remote changes are rolled back.
 This is as per one phase commit protocol which guarantees ACID for
 single foreign data source. So, the changes involving local and a single
 foreign server seem to be atomic and consistent.


 Really?  Maybe I'm missing something, but I don't think the current
 implementation for committing transactions has such a mechanism stated in
 step 1.  So, I think it's possible that the local transaction fails in
 step3 while the remote transaction succeeds, as mentioned above.


 PFA a script attached which shows this. You may want to check the code in
 pgfdw_xact_callback() for actions taken by postgres_fdw on various events.
 CommitTransaction() for how those events are generated. The code there
 complies with the sequence above.



 Thanks,

 Best regards,
 Etsuro Fujita




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




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


tran_inconsistency.sql
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] inherit support for foreign tables

2014-12-01 Thread Etsuro Fujita

(2014/11/28 18:14), Ashutosh Bapat wrote:

On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
Apart from the above, I noticed that the patch doesn't consider to
call ExplainForeignModify during EXPLAIN for an inherited
UPDATE/DELETE, as shown below (note that there are no UPDATE remote
queries displayed):



So, I'd like to modify explain.c to show those queries like this:



postgres=# explain verbose update parent set a = a * 2 where a = 5;
  QUERY PLAN

--__--__-
  Update on public.parent  (cost=0.00..280.77 rows=25 width=10)
-  Seq Scan on public.parent  (cost=0.00..0.00 rows=1 width=10)
  Output: (parent.a * 2), parent.ctid
  Filter: (parent.a = 5)
Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1
-  Foreign Scan on public.ft1  (cost=100.00..140.38 rows=12
width=10)
  Output: (ft1.a * 2), ft1.ctid
  Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a
= 5)) FOR UPDATE
Remote SQL: UPDATE public.mytable_2 SET a = $2 WHERE ctid = $1
-  Foreign Scan on public.ft2  (cost=100.00..140.38 rows=12
width=10)
  Output: (ft2.a * 2), ft2.ctid
  Remote SQL: SELECT a, ctid FROM public.mytable_2 WHERE ((a
= 5)) FOR UPDATE
(12 rows)



Two remote SQL under a single node would be confusing. Also the node
is labelled as Foreign Scan. It would be confusing to show an UPDATE
command under this scan node.


I thought this as an extention of the existing (ie, non-inherited) case 
(see the below example) to the inherited case.


postgres=# explain verbose update ft1 set a = a * 2 where a = 5;
 QUERY PLAN
-
 Update on public.ft1  (cost=100.00..140.38 rows=12 width=10)
   Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1
   -  Foreign Scan on public.ft1  (cost=100.00..140.38 rows=12 width=10)
 Output: (a * 2), ctid
 Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 
5)) FOR UPDATE

(5 rows)

I think we should show update commands somewhere for the inherited case 
as for the non-inherited case.  Alternatives to this are welcome.



BTW, I was experimenting with DMLs being executed on multiple FDW server
under same transaction and found that the transactions may not be atomic
(and may be inconsistent), if one or more of the server fails to commit
while rest of them commit the transaction. The reason for this is, we do
not rollback the already committed changes to the foreign server, if
one or more of them fail to commit a transaction. With foreign tables
under inheritance hierarchy a single DML can span across multiple
servers and the result may not be atomic (and may be inconsistent). So,


IIUC, even the transactions over the local and the *single* remote 
server are not guaranteed to be executed atomically in the current form. 
 It is possible that the remote transaction succeeds and the local one 
fails, for example, resulting in data inconsistency between the local 
and the remote.



either we have to disable DMLs on an inheritance hierarchy which spans
multiple servers. OR make sure that such transactions follow 2PC norms.


-1 for disabling update queries on such an inheritance hierarchy because 
I think we should leave that to the user's judgment.  And I think 2PC is 
definitely a separate patch.


Thanks,

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] inherit support for foreign tables

2014-11-28 Thread Ashutosh Bapat
On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 (2014/11/17 17:55), Ashutosh Bapat wrote:

 Here are my review comments for patch fdw-inh-3.patch.


 Thanks for the review!

  Tests
 ---
 1. It seems like you have copied from testcase inherit.sql to
 postgres_fdw testcase. That's a good thing, but it makes the test quite
 long. May be we should have two tests in postgres_fdw contrib module,
 one for simple cases, and other for inheritance. What do you say?


 IMO, the test is not so time-consuming, so it doesn't seem worth splitting
 it into two.


I am not worried about the timing but I am worried about the length of the
file and hence ease of debugging in case we find any issues there. We will
leave that for the commiter to decide.



  Documentation
 
 1. The change in ddl.sgml
 -We will refer to the child tables as partitions, though they
 -are in every way normal productnamePostgreSQL/ tables.
 +We will refer to the child tables as partitions, though we assume
 +that they are normal productnamePostgreSQL/ tables.

 adds phrase we assume that, which confuses the intention behind the
 sentence. The original text is intended to highlight the equivalence
 between partition and normal table, where as the addition esp. the
 word assume weakens that equivalence. Instead now we have to highlight
 the equivalence between partition and normal or foreign table. The
 wording similar to though they are either normal or foreign tables
 should be used there.


 You are right, but I feel that there is something out of place in saying
 that there (5.10.2. Implementing Partitioning) because the procedure there
 has been written based on normal tables only.  Put another way, if we say
 that, I think we'd need to add more docs, describing the syntax and/or the
 corresponding examples for foreign-table cases.  But I'd like to leave that
 for another patch.  So, how about the wording we assume *here* that,
 instead of we assume that, as I added the following notice in the
 previous section (5.10.1. Overview)?

 @@ -2650,7 +2669,10 @@ VALUES ('Albany', NULL, NULL, 'NY');
  table of a single parent table.  The parent table itself is normally
  empty; it exists just to represent the entire data set.  You should be
  familiar with inheritance (see xref linkend=ddl-inherit) before
 -attempting to set up partitioning.
 +attempting to set up partitioning.  (The setup and management of
 +partitioned tables illustrated in this section assume that each
 +partition is a normal table.  However, you can do that in a similar
 way
 +for cases where some or all partitions are foreign tables.)


This looks ok, though, I would like to see final version of the document.
But I think, we will leave that for committer to handle.


  2. The wording some kind of optimization gives vague picture. May be
 it can be worded as Since the constraints are assumed to be true, they
 are used in constraint-based query optimization like constraint
 exclusion for partitioned tables..
 +Those constraints are used in some kind of query optimization such
 +as constraint exclusion for partitioned tables (see
 +xref linkend=ddl-partitioning).


 Will follow your revision.


Thanks.



  Code
 ---
 1. In the following change
 +/*
* acquire_inherited_sample_rows -- acquire sample rows from
 inheritance tree
*
* This has the same API as acquire_sample_rows, except that rows are
* collected from all inheritance children as well as the specified
 table.
 - * We fail and return zero if there are no inheritance children.
 + * We fail and return zero if there are no inheritance children or
 there are
 + * inheritance children that foreign tables.

 The addition should be there are inheritance children that *are all
 *foreign tables. Note the addition are all.


 Sorry, I incorrectly wrote the comment.  What I tried to write is We fail
 and return zero if there are no inheritance children or if we are not in
 VAC_MODE_SINGLE case and inheritance tree contains at least one foreign
 table..


You might want to use English description of VAC_MODE_SINGLE instead of
that macro in the comment, so that reader doesn't have to look up
VAC_MODE_SINGLE. But I think, we will leave this for the committer.


  2. The function has_foreign() be better named has_foreign_child()? This


 How about has_foreign_table?


has_foreign_child() would be better, since these are children in the
inheritance hierarchy and not mere tables.



  function loops through all the tableoids passed even after it has found
 a foreign table. Why can't we return true immediately after finding the
 first foreign table, unless the side effects of heap_open() on all the
 table are required. But I don't see that to be the case, since these
 tables are locked already through a previous call to heap_open(). In the


 Good catch!  Will fix.

  same function 

Re: [HACKERS] inherit support for foreign tables

2014-11-27 Thread Etsuro Fujita

(2014/11/17 17:55), Ashutosh Bapat wrote:

Here are my review comments for patch fdw-inh-3.patch.


Thanks for the review!


Tests
---
1. It seems like you have copied from testcase inherit.sql to
postgres_fdw testcase. That's a good thing, but it makes the test quite
long. May be we should have two tests in postgres_fdw contrib module,
one for simple cases, and other for inheritance. What do you say?


IMO, the test is not so time-consuming, so it doesn't seem worth 
splitting it into two.



Documentation

1. The change in ddl.sgml
-We will refer to the child tables as partitions, though they
-are in every way normal productnamePostgreSQL/ tables.
+We will refer to the child tables as partitions, though we assume
+that they are normal productnamePostgreSQL/ tables.

adds phrase we assume that, which confuses the intention behind the
sentence. The original text is intended to highlight the equivalence
between partition and normal table, where as the addition esp. the
word assume weakens that equivalence. Instead now we have to highlight
the equivalence between partition and normal or foreign table. The
wording similar to though they are either normal or foreign tables
should be used there.


You are right, but I feel that there is something out of place in saying 
that there (5.10.2. Implementing Partitioning) because the procedure 
there has been written based on normal tables only.  Put another way, if 
we say that, I think we'd need to add more docs, describing the syntax 
and/or the corresponding examples for foreign-table cases.  But I'd like 
to leave that for another patch.  So, how about the wording we assume 
*here* that, instead of we assume that, as I added the following 
notice in the previous section (5.10.1. Overview)?


@@ -2650,7 +2669,10 @@ VALUES ('Albany', NULL, NULL, 'NY');
 table of a single parent table.  The parent table itself is normally
 empty; it exists just to represent the entire data set.  You should be
 familiar with inheritance (see xref linkend=ddl-inherit) before
-attempting to set up partitioning.
+attempting to set up partitioning.  (The setup and management of
+partitioned tables illustrated in this section assume that each
+partition is a normal table.  However, you can do that in a similar way
+for cases where some or all partitions are foreign tables.)


2. The wording some kind of optimization gives vague picture. May be
it can be worded as Since the constraints are assumed to be true, they
are used in constraint-based query optimization like constraint
exclusion for partitioned tables..
+Those constraints are used in some kind of query optimization such
+as constraint exclusion for partitioned tables (see
+xref linkend=ddl-partitioning).


Will follow your revision.


Code
---
1. In the following change
+/*
   * acquire_inherited_sample_rows -- acquire sample rows from
inheritance tree
   *
   * This has the same API as acquire_sample_rows, except that rows are
   * collected from all inheritance children as well as the specified table.
- * We fail and return zero if there are no inheritance children.
+ * We fail and return zero if there are no inheritance children or
there are
+ * inheritance children that foreign tables.

The addition should be there are inheritance children that *are all
*foreign tables. Note the addition are all.


Sorry, I incorrectly wrote the comment.  What I tried to write is We 
fail and return zero if there are no inheritance children or if we are 
not in VAC_MODE_SINGLE case and inheritance tree contains at least one 
foreign table..



2. The function has_foreign() be better named has_foreign_child()? This


How about has_foreign_table?


function loops through all the tableoids passed even after it has found
a foreign table. Why can't we return true immediately after finding the
first foreign table, unless the side effects of heap_open() on all the
table are required. But I don't see that to be the case, since these
tables are locked already through a previous call to heap_open(). In the


Good catch!  Will fix.


same function instead of argument name parentrelId may be we should use
name parent_oid, so that we use same notation for parent and child table
OIDs.


Will fix.


3.  Regarding enum VacuumMode - it's being used only in case of
acquire_inherited_sample_rows() and that too, to check only a single
value of the three defined there. May be we should just infer that value
inside acquire_inherited_sample_rows() or pass a boolean true or false
from do_analyse_rel() based on the VacuumStmt. I do not see need for a
separate three value enum of which only one value is used and also to
pass it down from vacuum() by changing signatures of the minion functions.


I introduced that for possible future use.  See the discussion in [1].


4. In postgresGetForeignPlan(), the code added by this patch is required
to handle the case when 

Re: [HACKERS] inherit support for foreign tables

2014-11-18 Thread Ashutosh Bapat
On Mon, Nov 17, 2014 at 1:25 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 (2014/11/12 20:04), Ashutosh Bapat wrote:

 I reviewed fdw-chk-3 patch. Here are my comments


 Thanks for the review!

  Tests
 ---
 1. The tests added in file_fdw module look good. We should add tests for
 CREATE TABLE with CHECK CONSTRAINT also.


 Agreed.  I added the tests, and also changed the proposed tests a bit.

  2.  For postgres_fdw we need tests to check the behaviour in case the
 constraints mismatch between the remote table and its local foreign
 table declaration in case of INSERT, UPDATE and SELECT.


 Done.

  3. In the testcases for postgres_fdw it seems that you have forgotten to
 add statement after SET constraint_exclusion to 'partition'


 I added the statement.

  4. In test foreign_data there are changes to fix the diffs caused by
 these changes like below
   ALTER FOREIGN TABLE ft1 DROP CONSTRAINT no_const;   -- ERROR
 -ERROR:  ft1 is not a table
 +ERROR:  constraint no_const of relation ft1 does not exist
   ALTER FOREIGN TABLE ft1 DROP CONSTRAINT IF EXISTS no_const;
 -ERROR:  ft1 is not a table
 +NOTICE:  constraint no_const of relation ft1 does not exist, skipping
   ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c1_check;
 -ERROR:  ft1 is not a table
 +ERROR:  constraint ft1_c1_check of relation ft1 does not exist


  Earlier when constraints were not supported for FOREIGN TABLE, these
 tests made sure the same functionality. So, even though the
 corresponding constraints were not created on the table (in fact it
 didn't allow the creation as well). Now that the constraints are
 allowed, I think the tests for no_const (without IF EXISTS) and
 ft1_c1_check are duplicating the same testcase. May be we should
 review this set of statement in the light of new functionality.


 Agreed.  I removed the DROP CONSTRAINT ft1_c1_check test, and added a
 new test for ALTER CONSTRAINT.

  Code and implementation
 --
 The usage of NO INHERIT and NOT VALID with CONSTRAINT on foreign table
 is blocked, but corresponding documentation entry doesn't mention so.
 Since foreign tables can not be inherited NO INHERIT option isn't
 applicable to foreign tables and the constraints on the foreign tables
 are declarative, hence NOT VALID option is also not applicable. So, I
 agree with what the code is doing, but that should be reflected in
 documentation with this explanation.
 Rest of the code modifies the condition checks for CHECK CONSTRAINTs on
 foreign tables, and it looks good to me.


 Done.

 Other changes:
 * Modified one error message that I added in AddRelationNewConstraints, to
 match the other message there.
 * Revised other docs a little bit.

 Attached is an updated version of the patch.


I looked at the patch. It looks good now. Once we have the inh patch ready,
we can mark this item as ready for commiter.


 Thanks,

 Best regards,
 Etsuro Fujita




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


Re: [HACKERS] inherit support for foreign tables

2014-11-18 Thread Etsuro Fujita

(2014/11/18 18:09), Ashutosh Bapat wrote:

I looked at the patch. It looks good now. Once we have the inh patch
ready, we can mark this item as ready for commiter.


Thanks for the review!

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] inherit support for foreign tables

2014-11-12 Thread Ashutosh Bapat
Hi Fujita-san,
I reviewed fdw-chk-3 patch. Here are my comments

Sanity

1. The patch applies on the latest master using patch but not by git apply
2. it compiles clean
3. Regression run is clean, including the contrib module regressions

Tests
---
1. The tests added in file_fdw module look good. We should add tests for
CREATE TABLE with CHECK CONSTRAINT also.
2.  For postgres_fdw we need tests to check the behaviour in case the
constraints mismatch between the remote table and its local foreign table
declaration in case of INSERT, UPDATE and SELECT.
3. In the testcases for postgres_fdw it seems that you have forgotten to
add statement after SET constraint_exclusion to 'partition'
4. In test foreign_data there are changes to fix the diffs caused by these
changes like below
 ALTER FOREIGN TABLE ft1 DROP CONSTRAINT no_const;   -- ERROR
-ERROR:  ft1 is not a table
+ERROR:  constraint no_const of relation ft1 does not exist
 ALTER FOREIGN TABLE ft1 DROP CONSTRAINT IF EXISTS no_const;
-ERROR:  ft1 is not a table
+NOTICE:  constraint no_const of relation ft1 does not exist, skipping
 ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c1_check;
-ERROR:  ft1 is not a table
+ERROR:  constraint ft1_c1_check of relation ft1 does not exist


Earlier when constraints were not supported for FOREIGN TABLE, these tests
made sure the same functionality. So, even though the corresponding
constraints were not created on the table (in fact it didn't allow the
creation as well). Now that the constraints are allowed, I think the tests
for no_const (without IF EXISTS) and ft1_c1_check are duplicating the
same testcase. May be we should review this set of statement in the light
of new functionality.

Code and implementation
--
The usage of NO INHERIT and NOT VALID with CONSTRAINT on foreign table is
blocked, but corresponding documentation entry doesn't mention so. Since
foreign tables can not be inherited NO INHERIT option isn't applicable to
foreign tables and the constraints on the foreign tables are declarative,
hence NOT VALID option is also not applicable. So, I agree with what the
code is doing, but that should be reflected in documentation with this
explanation.
Rest of the code modifies the condition checks for CHECK CONSTRAINTs on
foreign tables, and it looks good to me.



On Fri, Nov 7, 2014 at 5:31 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 (2014/11/07 14:57), Kyotaro HORIGUCHI wrote:

 Here are separated patches.

 fdw-chk.patch  - CHECK constraints on foreign tables
 fdw-inh.patch  - table inheritance with foreign tables

 The latter has been created on top of [1].


  [1]
 http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp


  To be exact, it has been created on top of [1] and fdw-chk.patch.


 I tried both patches on the current head, the newly added
 parameter to analyze_rel() hampered them from applying but it is
 easy to fix seemingly and almost all the other part was applied
 cleanly.


 Thanks for the review!

  By the way, are these the result of simply splitting of your last
 patch, foreign_inherit-v15.patch?

 http://www.postgresql.org/message-id/53feef94.6040...@lab.ntt.co.jp


 The answer is no.

  The result of apllying whole-in-one version and this splitted
 version seem to have many differences. Did you added even other
 changes? Or do I understand this patch wrongly?


 As I said before, I splitted the whole-in-one version into three: 1) CHECK
 constraint patch (ie fdw-chk.patch), 2) table inheritance patch (ie
 fdw-inh.patch) and 3) path reparameterization patch (not posted). In
 addition to that, I slightly modified #1 and #2.

 IIUC, #3 would be useful not only for the inheritance cases but for union
 all cases.  So, I plan to propose it independently in the next CF.

  I noticed that the latter disallows TRUNCATE on inheritance trees that
 contain at least one child foreign table.  But I think it would be
 better to allow it, with the semantics that we quietly ignore the
 child
 foreign tables and apply the operation to the child plain tables,
 which
 is the same semantics as ALTER COLUMN SET STORAGE on such inheritance
 trees.  Comments welcome.


 Done.  And I've also a bit revised regression tests for both
 patches. Patches attached.


 I rebased the patches to the latest head.  Here are updated patches.

 Other changes:

 * fdw-chk-3.patch: the updated patch revises some ereport messages a
 little bit.

 * fdw-inh-3.patch: I noticed that there is a doc bug in the previous
 patch.  The updated patch fixes that, adds a bit more docs, and revises
 regression tests in foreign_data.sql a bit further.


 Thanks,

 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




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


Re: [HACKERS] inherit support for foreign tables

2014-11-12 Thread Ashutosh Bapat
Hi Fujita-san,
I tried to apply fdw-inh-3.patch on the latest head from master branch. It
failed to apply using both patch and git apply.

patch failed to apply because of rejections in
contrib/file_fdw/output/file_fdw.source and
doc/src/sgml/ref/create_foreign_table.sgml


On Fri, Nov 7, 2014 at 5:31 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 (2014/11/07 14:57), Kyotaro HORIGUCHI wrote:

 Here are separated patches.

 fdw-chk.patch  - CHECK constraints on foreign tables
 fdw-inh.patch  - table inheritance with foreign tables

 The latter has been created on top of [1].


  [1]
 http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp


  To be exact, it has been created on top of [1] and fdw-chk.patch.


 I tried both patches on the current head, the newly added
 parameter to analyze_rel() hampered them from applying but it is
 easy to fix seemingly and almost all the other part was applied
 cleanly.


 Thanks for the review!

  By the way, are these the result of simply splitting of your last
 patch, foreign_inherit-v15.patch?

 http://www.postgresql.org/message-id/53feef94.6040...@lab.ntt.co.jp


 The answer is no.

  The result of apllying whole-in-one version and this splitted
 version seem to have many differences. Did you added even other
 changes? Or do I understand this patch wrongly?


 As I said before, I splitted the whole-in-one version into three: 1) CHECK
 constraint patch (ie fdw-chk.patch), 2) table inheritance patch (ie
 fdw-inh.patch) and 3) path reparameterization patch (not posted). In
 addition to that, I slightly modified #1 and #2.

 IIUC, #3 would be useful not only for the inheritance cases but for union
 all cases.  So, I plan to propose it independently in the next CF.

  I noticed that the latter disallows TRUNCATE on inheritance trees that
 contain at least one child foreign table.  But I think it would be
 better to allow it, with the semantics that we quietly ignore the
 child
 foreign tables and apply the operation to the child plain tables,
 which
 is the same semantics as ALTER COLUMN SET STORAGE on such inheritance
 trees.  Comments welcome.


 Done.  And I've also a bit revised regression tests for both
 patches. Patches attached.


 I rebased the patches to the latest head.  Here are updated patches.

 Other changes:

 * fdw-chk-3.patch: the updated patch revises some ereport messages a
 little bit.

 * fdw-inh-3.patch: I noticed that there is a doc bug in the previous
 patch.  The updated patch fixes that, adds a bit more docs, and revises
 regression tests in foreign_data.sql a bit further.


 Thanks,

 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




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


Re: [HACKERS] inherit support for foreign tables

2014-11-12 Thread Etsuro Fujita

Hi Ashutosh,

Thanks for the review!

(2014/11/13 15:23), Ashutosh Bapat wrote:

I tried to apply fdw-inh-3.patch on the latest head from master branch.
It failed to apply using both patch and git apply.

patch failed to apply because of rejections in
contrib/file_fdw/output/file_fdw.source and
doc/src/sgml/ref/create_foreign_table.sgml


As I said upthread, fdw-inh-3.patch has been created on top of [1] and 
fdw-chk-3.patch.  Did you apply these patche first?


[1] https://commitfest.postgresql.org/action/patch_view?id=1599

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] inherit support for foreign tables

2014-11-12 Thread Ashutosh Bapat
On Thu, Nov 13, 2014 at 12:20 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
 wrote:

 Hi Ashutosh,

 Thanks for the review!

 (2014/11/13 15:23), Ashutosh Bapat wrote:

 I tried to apply fdw-inh-3.patch on the latest head from master branch.
 It failed to apply using both patch and git apply.

 patch failed to apply because of rejections in
 contrib/file_fdw/output/file_fdw.source and
 doc/src/sgml/ref/create_foreign_table.sgml


 As I said upthread, fdw-inh-3.patch has been created on top of [1] and
 fdw-chk-3.patch.  Did you apply these patche first?

 Oh, sorry, I didn't pay attention to that. I will apply both the patches
and review the inheritance patch. Thanks for pointing that out.


 [1] https://commitfest.postgresql.org/action/patch_view?id=1599

 Best regards,
 Etsuro Fujita




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


Re: [HACKERS] inherit support for foreign tables

2014-11-07 Thread Etsuro Fujita
Hi Furuya-san,

(2014/11/07 16:54), furu...@pm.nttdata.co.jp wrote:
 (2014/08/28 18:00), Etsuro Fujita wrote:
 (2014/08/22 11:51), Noah Misch wrote:
 Today's ANALYZE VERBOSE messaging for former inheritance parents
 (tables with relhassubclass = true but no pg_inherits.inhparent
 links) is deceptive, and I welcome a fix to omit the spurious
 message.  As defects go, this is quite minor.  There's fundamentally
 no value in collecting inheritance tree statistics for such a parent,
 and no PostgreSQL command will do so.

 A
 credible alternative is to emit a second message indicating that we
 skipped the inheritance tree statistics after all, and why we skipped
 them.

 I'd like to address this by emitting the second message as shown below:

 A separate patch (analyze.patch) handles the former case in a similar
 way.

 I'll add to the upcoming CF, the analyze.patch as an independent item,
 which emits a second message indicating that we skipped the inheritance
 tree statistics and why we skipped them.
 
 I did a review of the patch.
 There was no problem.
 I confirmed the following.
 
 1. applied cleanly and compilation was without warnings and errors
 2. all regress tests was passed ok
 3. The message output from ANALYZE VERBOSE.
 
 Following are the SQL which I used to check messages.
 
 create table parent (id serial);
 create table child (name text) inherits (parent);
 ANALYZE VERBOSE parent ;
 drop table child ;
 ANALYZE VERBOSE parent ;

I think that that is a correct test for this patch.

Thanks for the review!

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] inherit support for foreign tables

2014-11-07 Thread Etsuro Fujita

(2014/11/07 14:57), Kyotaro HORIGUCHI wrote:

Here are separated patches.

fdw-chk.patch  - CHECK constraints on foreign tables
fdw-inh.patch  - table inheritance with foreign tables

The latter has been created on top of [1].



[1]
http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp



To be exact, it has been created on top of [1] and fdw-chk.patch.


I tried both patches on the current head, the newly added
parameter to analyze_rel() hampered them from applying but it is
easy to fix seemingly and almost all the other part was applied
cleanly.


Thanks for the review!


By the way, are these the result of simply splitting of your last
patch, foreign_inherit-v15.patch?

http://www.postgresql.org/message-id/53feef94.6040...@lab.ntt.co.jp


The answer is no.


The result of apllying whole-in-one version and this splitted
version seem to have many differences. Did you added even other
changes? Or do I understand this patch wrongly?


As I said before, I splitted the whole-in-one version into three: 1) 
CHECK constraint patch (ie fdw-chk.patch), 2) table inheritance patch 
(ie fdw-inh.patch) and 3) path reparameterization patch (not posted). 
In addition to that, I slightly modified #1 and #2.


IIUC, #3 would be useful not only for the inheritance cases but for 
union all cases.  So, I plan to propose it independently in the next CF.



I noticed that the latter disallows TRUNCATE on inheritance trees that
contain at least one child foreign table.  But I think it would be
better to allow it, with the semantics that we quietly ignore the
child
foreign tables and apply the operation to the child plain tables,
which
is the same semantics as ALTER COLUMN SET STORAGE on such inheritance
trees.  Comments welcome.


Done.  And I've also a bit revised regression tests for both
patches. Patches attached.


I rebased the patches to the latest head.  Here are updated patches.

Other changes:

* fdw-chk-3.patch: the updated patch revises some ereport messages a 
little bit.


* fdw-inh-3.patch: I noticed that there is a doc bug in the previous 
patch.  The updated patch fixes that, adds a bit more docs, and revises 
regression tests in foreign_data.sql a bit further.


Thanks,

Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 144,149  SET constraint_exclusion = 'partition';
--- 144,166 
  \t off
  ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check;
  
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ ALTER FOREIGN TABLE agg_text INHERIT agg;
+ SELECT * FROM agg WHERE b  10.0 ORDER BY a, b;
+ SELECT * FROM ONLY agg WHERE b  10.0 ORDER BY a, b;
+ SELECT * FROM agg_csv WHERE b  10.0 ORDER BY a, b;
+ SELECT * FROM agg_text WHERE b  10.0 ORDER BY a, b;
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ DELETE FROM agg WHERE a = 100;
+ -- but this should be ignored
+ SELECT * FROM agg WHERE b  10.0 ORDER BY a, b FOR UPDATE;
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ ALTER FOREIGN TABLE agg_text NO INHERIT agg;
+ DROP TABLE agg;
+ 
  -- privilege tests
  SET ROLE file_fdw_superuser;
  SELECT * FROM agg_text ORDER BY a;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 237,242  EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a  0;
--- 237,289 
  SET constraint_exclusion = 'partition';
  \t off
  ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ ALTER FOREIGN TABLE agg_text INHERIT agg;
+ SELECT * FROM agg WHERE b  10.0 ORDER BY a, b;
+  a  |b
+ +-
+   0 | 0.09561
+   0 | 0.09561
+  56 | 7.8
+ (3 rows)
+ 
+ SELECT * FROM ONLY agg WHERE b  10.0 ORDER BY a, b;
+  a | b 
+ ---+---
+ (0 rows)
+ 
+ SELECT * FROM agg_csv WHERE b  10.0 ORDER BY a, b;
+  a |b
+ ---+-
+  0 | 0.09561
+ (1 row)
+ 
+ SELECT * FROM agg_text WHERE b  10.0 ORDER BY a, b;
+  a  |b
+ +-
+   0 | 0.09561
+  56 | 7.8
+ (2 rows)
+ 
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ ERROR:  cannot update foreign table agg_text
+ DELETE FROM agg WHERE a = 100;
+ ERROR:  cannot delete from foreign table agg_text
+ -- but this should be ignored
+ SELECT * FROM agg WHERE b  10.0 ORDER BY a, b FOR UPDATE;
+  a  |b
+ +-
+   0 | 0.09561
+   0 | 0.09561
+  56 | 7.8
+ (3 rows)
+ 
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ ALTER FOREIGN TABLE agg_text NO INHERIT agg;
+ DROP TABLE agg;
  -- privilege tests
  SET ROLE file_fdw_superuser;
  SELECT * FROM agg_text ORDER BY a;
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 2863,2868  NOTICE:  NEW: (13,test triggered !)
--- 2863,3376 
  (1 row)
  
  -- 

Re: [HACKERS] inherit support for foreign tables

2014-11-06 Thread Kyotaro HORIGUCHI
Hello, I don't fully catch up this topic but tried this one.

  Here are separated patches.
 
  fdw-chk.patch  - CHECK constraints on foreign tables
  fdw-inh.patch  - table inheritance with foreign tables
 
  The latter has been created on top of [1].
 
  [1]
  http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp
 
  To be exact, it has been created on top of [1] and fdw-chk.patch.

I tried both patches on the current head, the newly added
parameter to analyze_rel() hampered them from applying but it is
easy to fix seemingly and almost all the other part was applied
cleanly.

By the way, are these the result of simply splitting of your last
patch, foreign_inherit-v15.patch?

http://www.postgresql.org/message-id/53feef94.6040...@lab.ntt.co.jp

The result of apllying whole-in-one version and this splitted
version seem to have many differences. Did you added even other
changes? Or do I understand this patch wrongly?

git diff --numstat 0_foreign_inherit_one 0_foreign_inherit_two 
5   51  contrib/file_fdw/file_fdw.c
10  19  contrib/file_fdw/input/file_fdw.source
18  71  contrib/file_fdw/output/file_fdw.source
19  70  contrib/postgres_fdw/expected/postgres_fdw.out
9   66  contrib/postgres_fdw/postgres_fdw.c
12  35  contrib/postgres_fdw/sql/postgres_fdw.sql
13  48  doc/src/sgml/fdwhandler.sgml
39  39  doc/src/sgml/ref/alter_foreign_table.sgml
4   3   doc/src/sgml/ref/create_foreign_table.sgml
8   0   src/backend/catalog/heap.c
7   3   src/backend/commands/analyze.c
0   7   src/backend/commands/tablecmds.c
1   22  src/backend/optimizer/plan/createplan.c
7   7   src/backend/optimizer/prep/prepunion.c
0   26  src/backend/optimizer/util/pathnode.c
1   1   src/include/commands/vacuum.h
0   7   src/include/foreign/fdwapi.h
19  1   src/test/regress/expected/foreign_data.out
9   2   src/test/regress/sql/foreign_data.sql


  I noticed that the latter disallows TRUNCATE on inheritance trees that
  contain at least one child foreign table.  But I think it would be
  better to allow it, with the semantics that we quietly ignore the
  child
  foreign tables and apply the operation to the child plain tables,
  which
  is the same semantics as ALTER COLUMN SET STORAGE on such inheritance
  trees.  Comments welcome.
 
 Done.  And I've also a bit revised regression tests for both
 patches. Patches attached.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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


Re: [HACKERS] inherit support for foreign tables

2014-11-06 Thread furuyao
 (2014/08/28 18:00), Etsuro Fujita wrote:
  (2014/08/22 11:51), Noah Misch wrote:
  Today's ANALYZE VERBOSE messaging for former inheritance parents
  (tables with relhassubclass = true but no pg_inherits.inhparent
  links) is deceptive, and I welcome a fix to omit the spurious
  message.  As defects go, this is quite minor.  There's fundamentally
  no value in collecting inheritance tree statistics for such a parent,
  and no PostgreSQL command will do so.
 
  A
  credible alternative is to emit a second message indicating that we
  skipped the inheritance tree statistics after all, and why we skipped
  them.
 
  I'd like to address this by emitting the second message as shown below:
 
  A separate patch (analyze.patch) handles the former case in a similar
 way.
 
 I'll add to the upcoming CF, the analyze.patch as an independent item,
 which emits a second message indicating that we skipped the inheritance
 tree statistics and why we skipped them.

I did a review of the patch. 
There was no problem.
I confirmed the following.

1. applied cleanly and compilation was without warnings and errors
2. all regress tests was passed ok
3. The message output from ANALYZE VERBOSE.

Following are the SQL which I used to check messages.

create table parent (id serial);
create table child (name text) inherits (parent);
ANALYZE VERBOSE parent ;
drop table child ;
ANALYZE VERBOSE parent ;

Regards,

--
Furuya Osamu


-- 
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] inherit support for foreign tables

2014-10-24 Thread Etsuro Fujita

(2014/10/21 17:40), Etsuro Fujita wrote:

(2014/10/14 20:00), Etsuro Fujita wrote:

Here are separated patches.

fdw-chk.patch  - CHECK constraints on foreign tables
fdw-inh.patch  - table inheritance with foreign tables

The latter has been created on top of [1].



[1] http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp


To be exact, it has been created on top of [1] and fdw-chk.patch.

I noticed that the latter disallows TRUNCATE on inheritance trees that
contain at least one child foreign table.  But I think it would be
better to allow it, with the semantics that we quietly ignore the child
foreign tables and apply the operation to the child plain tables, which
is the same semantics as ALTER COLUMN SET STORAGE on such inheritance
trees.  Comments welcome.


Done.  And I've also a bit revised regression tests for both patches. 
Patches attached.


Thanks,

Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 134,139  DELETE FROM agg_csv WHERE a = 100;
--- 134,149 
  -- but this should be ignored
  SELECT * FROM agg_csv FOR UPDATE;
  
+ -- constraint exclusion tests
+ ALTER FOREIGN TABLE agg_csv ADD CONSTRAINT agg_csv_a_check CHECK (a = 0);
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a  0;
+ SET constraint_exclusion = 'on';
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a  0;
+ SET constraint_exclusion = 'partition';
+ \t off
+ ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check;
+ 
  -- privilege tests
  SET ROLE file_fdw_superuser;
  SELECT * FROM agg_text ORDER BY a;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 219,224  SELECT * FROM agg_csv FOR UPDATE;
--- 219,242 
42 |  324.78
  (3 rows)
  
+ -- constraint exclusion tests
+ ALTER FOREIGN TABLE agg_csv ADD CONSTRAINT agg_csv_a_check CHECK (a = 0);
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a  0;
+  Foreign Scan on public.agg_csv
+Output: a, b
+Filter: (agg_csv.a  0)
+Foreign File: @abs_srcdir@/data/agg.csv
+ 
+ SET constraint_exclusion = 'on';
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a  0;
+  Result
+Output: a, b
+One-Time Filter: false
+ 
+ SET constraint_exclusion = 'partition';
+ \t off
+ ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check;
  -- privilege tests
  SET ROLE file_fdw_superuser;
  SELECT * FROM agg_text ORDER BY a;
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 2488,2493  select c2, count(*) from S 1.T 1 where c2  500 group by 1 order by 1;
--- 2488,2515 
  (13 rows)
  
  -- ===
+ -- test constraint exclusion stuff
+ -- ===
+ ALTER FOREIGN TABLE ft1 ADD CONSTRAINT ft1_c2positive CHECK (c2 = 0);
+ EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 WHERE c2  0;
+ QUERY PLAN
+ --
+  Foreign Scan on public.ft1
+Output: c1, c2, c3, c4, c5, c6, c7, c8
+Remote SQL: SELECT C 1, c2, c3, c4, c5, c6, c7, c8 FROM S 1.T 1 WHERE ((c2  0))
+ (3 rows)
+ 
+ SET constraint_exclusion = 'on';
+ EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 WHERE c2  0;
+ QUERY PLAN
+ --
+  Result
+Output: c1, c2, c3, c4, c5, c6, c7, c8
+One-Time Filter: false
+ (3 rows)
+ 
+ SET constraint_exclusion = 'partition';
+ -- ===
  -- test serial columns (ie, sequence-based defaults)
  -- ===
  create table loc1 (f1 serial, f2 text);
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***
*** 387,392  select c2, count(*) from ft2 where c2  500 group by 1 order by 1;
--- 387,401 
  select c2, count(*) from S 1.T 1 where c2  500 group by 1 order by 1;
  
  -- ===
+ -- test constraint exclusion stuff
+ -- ===
+ ALTER FOREIGN TABLE ft1 ADD CONSTRAINT ft1_c2positive CHECK (c2 = 0);
+ EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 WHERE c2  0;
+ SET constraint_exclusion = 'on';
+ EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 WHERE c2  0;
+ SET constraint_exclusion = 'partition';
+ 
+ -- ===
  -- test serial columns (ie, sequence-based defaults)
  -- ===
  create table 

Re: [HACKERS] inherit support for foreign tables

2014-10-21 Thread Etsuro Fujita

(2014/10/14 20:00), Etsuro Fujita wrote:

Here are separated patches.

fdw-chk.patch  - CHECK constraints on foreign tables
fdw-inh.patch  - table inheritance with foreign tables

The latter has been created on top of [1].



[1] http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp


To be exact, it has been created on top of [1] and fdw-chk.patch.

I noticed that the latter disallows TRUNCATE on inheritance trees that 
contain at least one child foreign table.  But I think it would be 
better to allow it, with the semantics that we quietly ignore the child 
foreign tables and apply the operation to the child plain tables, which 
is the same semantics as ALTER COLUMN SET STORAGE on such inheritance 
trees.  Comments welcome.


Thanks,

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] inherit support for foreign tables

2014-10-16 Thread Etsuro Fujita

(2014/08/28 18:00), Etsuro Fujita wrote:

(2014/08/22 11:51), Noah Misch wrote:

Today's ANALYZE VERBOSE messaging for former inheritance parents
(tables with
relhassubclass = true but no pg_inherits.inhparent links) is
deceptive, and I
welcome a fix to omit the spurious message.  As defects go, this is quite
minor.  There's fundamentally no value in collecting inheritance tree
statistics for such a parent, and no PostgreSQL command will do so.



A
credible alternative is to emit a second message indicating that we
skipped
the inheritance tree statistics after all, and why we skipped them.



I'd like to address this by emitting the second message as shown below:



A separate patch (analyze.patch) handles the former case in a similar way.


I'll add to the upcoming CF, the analyze.patch as an independent item, 
which emits a second message indicating that we skipped the inheritance 
tree statistics and why we skipped them.


Thanks,

Best regards,
Etsuro Fujita
*** a/src/backend/commands/analyze.c
--- b/src/backend/commands/analyze.c
***
*** 1478,1483  acquire_inherited_sample_rows(Relation onerel, int elevel,
--- 1478,1487 
  		/* CCI because we already updated the pg_class row in this command */
  		CommandCounterIncrement();
  		SetRelationHasSubclass(RelationGetRelid(onerel), false);
+ 		ereport(elevel,
+ (errmsg(skipping analyze of \%s.%s\ inheritance tree --- this inheritance tree contains no child tables,
+ 		get_namespace_name(RelationGetNamespace(onerel)),
+ 		RelationGetRelationName(onerel;
  		return 0;
  	}
  

-- 
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] inherit support for foreign tables

2014-10-14 Thread Etsuro Fujita

(2014/09/12 16:30), Etsuro Fujita wrote:

(2014/09/11 20:51), Heikki Linnakangas wrote:

On 09/11/2014 02:30 PM, Etsuro Fujita wrote:

So, should I split the patch into the two?


Yeah, please do.


OK, Will do.


Here are separated patches.

fdw-chk.patch  - CHECK constraints on foreign tables
fdw-inh.patch  - table inheritance with foreign tables

The latter has been created on top of [1].  I've addressed the comment 
from Horiguchi-san [2] in it also, though I've slightly modified his 
proposal.


There is the functionality for path reparameterization for ForeignScan 
in the previous patch, but since the functionality would be useful not 
only for such table inheritance cases but for UNION ALL cases, I'd add 
the functionality as an independent feature maybe to CF 2014-12.


Thanks,

[1] http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp
[2] 
http://www.postgresql.org/message-id/20140902.142218.253402812.horiguchi.kyot...@lab.ntt.co.jp


Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 134,139  DELETE FROM agg_csv WHERE a = 100;
--- 134,149 
  -- but this should be ignored
  SELECT * FROM agg_csv FOR UPDATE;
  
+ -- constraint exclusion tests
+ ALTER FOREIGN TABLE agg_csv ADD CONSTRAINT agg_csv_a_check CHECK (a = 0);
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a  0;
+ SET constraint_exclusion = 'on';
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a  0;
+ SET constraint_exclusion = 'partition';
+ \t off
+ ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check;
+ 
  -- privilege tests
  SET ROLE file_fdw_superuser;
  SELECT * FROM agg_text ORDER BY a;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 219,224  SELECT * FROM agg_csv FOR UPDATE;
--- 219,242 
42 |  324.78
  (3 rows)
  
+ -- constraint exclusion tests
+ ALTER FOREIGN TABLE agg_csv ADD CONSTRAINT agg_csv_a_check CHECK (a = 0);
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a  0;
+  Foreign Scan on public.agg_csv
+Output: a, b
+Filter: (agg_csv.a  0)
+Foreign File: @abs_srcdir@/data/agg.csv
+ 
+ SET constraint_exclusion = 'on';
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a  0;
+  Result
+Output: a, b
+One-Time Filter: false
+ 
+ SET constraint_exclusion = 'partition';
+ \t off
+ ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check;
  -- privilege tests
  SET ROLE file_fdw_superuser;
  SELECT * FROM agg_text ORDER BY a;
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 2488,2493  select c2, count(*) from S 1.T 1 where c2  500 group by 1 order by 1;
--- 2488,2512 
  (13 rows)
  
  -- ===
+ -- test constraint exclusion stuff
+ -- ===
+ ALTER FOREIGN TABLE ft1 ADD CONSTRAINT ft1_c1_check CHECK (c1  0);
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM ft1 WHERE c1 = 0;
+  Foreign Scan on public.ft1
+Output: c1, c2, c3, c4, c5, c6, c7, c8
+Remote SQL: SELECT C 1, c2, c3, c4, c5, c6, c7, c8 FROM S 1.T 1 WHERE ((C 1 = 0))
+ 
+ SET constraint_exclusion = 'on';
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM ft1 WHERE c1 = 0;
+  Result
+Output: c1, c2, c3, c4, c5, c6, c7, c8
+One-Time Filter: false
+ 
+ SET constraint_exclusion = 'partition';
+ \t off
+ ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c1_check;
+ -- ===
  -- test serial columns (ie, sequence-based defaults)
  -- ===
  create table loc1 (f1 serial, f2 text);
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***
*** 387,392  select c2, count(*) from ft2 where c2  500 group by 1 order by 1;
--- 387,404 
  select c2, count(*) from S 1.T 1 where c2  500 group by 1 order by 1;
  
  -- ===
+ -- test constraint exclusion stuff
+ -- ===
+ ALTER FOREIGN TABLE ft1 ADD CONSTRAINT ft1_c1_check CHECK (c1  0);
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM ft1 WHERE c1 = 0;
+ SET constraint_exclusion = 'on';
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM ft1 WHERE c1 = 0;
+ SET constraint_exclusion = 'partition';
+ \t off
+ ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c1_check;
+ 
+ -- ===
  -- test serial columns (ie, sequence-based defaults)
  -- ===
  create table loc1 (f1 serial, f2 text);
*** 

Re: [HACKERS] inherit support for foreign tables

2014-09-12 Thread Etsuro Fujita

(2014/09/11 20:51), Heikki Linnakangas wrote:

On 09/11/2014 02:30 PM, Etsuro Fujita wrote:

So, should I split the patch into the two?


Yeah, please do.


OK, Will do.

Thanks,

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] inherit support for foreign tables

2014-09-11 Thread Etsuro Fujita

(2014/09/11 4:32), Heikki Linnakangas wrote:

I had a cursory look at this patch and the discussions around this.


Thank you!


ISTM there are actually two new features in this: 1) allow CHECK
constraints on foreign tables, and 2) support inheritance for foreign
tables. How about splitting it into two?


That's right.  There are the two in this patch.

I'm not sure if I should split the patch into the two, because 1) won't 
make sense without 2).  As described in the following note added to the 
docs on CREATE FOREIGN TABLE, CHECK constraints on foreign tables are 
intended to support constraint exclusion for partitioned foreign tables:


+ Constraints on foreign tables are not enforced on insert or update.
+ Those definitions simply declare the constraints hold for all rows
+ in the foreign tables.  It is the user's responsibility to ensure
+ that those definitions match the remote side.  Such constraints are
+ used for some kind of query optimization such as constraint exclusion
+ for partitioned tables

Thanks,

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] inherit support for foreign tables

2014-09-11 Thread Heikki Linnakangas

On 09/11/2014 12:22 PM, Etsuro Fujita wrote:

(2014/09/11 4:32), Heikki Linnakangas wrote:

I had a cursory look at this patch and the discussions around this.


Thank you!


ISTM there are actually two new features in this: 1) allow CHECK
constraints on foreign tables, and 2) support inheritance for foreign
tables. How about splitting it into two?


That's right.  There are the two in this patch.

I'm not sure if I should split the patch into the two, because 1) won't
make sense without 2).  As described in the following note added to the
docs on CREATE FOREIGN TABLE, CHECK constraints on foreign tables are
intended to support constraint exclusion for partitioned foreign tables:

+ Constraints on foreign tables are not enforced on insert or update.
+ Those definitions simply declare the constraints hold for all rows
+ in the foreign tables.  It is the user's responsibility to ensure
+ that those definitions match the remote side.  Such constraints are
+ used for some kind of query optimization such as constraint exclusion
+ for partitioned tables


The planner can do constraint exclusion based on CHECK constraints even 
without inheritance. It's not enabled by default because it can increase 
planning time, but if you set constraint_exclusion=on, it will work.


For example:

postgres=# create table foo (i int4 CHECK (i  0));
CREATE TABLE
postgres=# explain select * from foo WHERE i  0;
  QUERY PLAN
--
 Seq Scan on foo  (cost=0.00..40.00 rows=800 width=4)
   Filter: (i  0)
 Planning time: 0.335 ms
(3 rows)

postgres=# show constraint_exclusion ;
 constraint_exclusion
--
 partition
(1 row)

postgres=# set constraint_exclusion ='on';
SET
postgres=# explain select * from foo WHERE i  0;
QUERY PLAN
--
 Result  (cost=0.00..0.01 rows=1 width=0)
   One-Time Filter: false
 Planning time: 0.254 ms
(3 rows)

postgres=#

- Heikki



--
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] inherit support for foreign tables

2014-09-11 Thread Etsuro Fujita

(2014/09/11 19:46), Heikki Linnakangas wrote:

On 09/11/2014 12:22 PM, Etsuro Fujita wrote:

(2014/09/11 4:32), Heikki Linnakangas wrote:

I had a cursory look at this patch and the discussions around this.


Thank you!


ISTM there are actually two new features in this: 1) allow CHECK
constraints on foreign tables, and 2) support inheritance for foreign
tables. How about splitting it into two?


That's right.  There are the two in this patch.

I'm not sure if I should split the patch into the two, because 1) won't
make sense without 2).  As described in the following note added to the
docs on CREATE FOREIGN TABLE, CHECK constraints on foreign tables are
intended to support constraint exclusion for partitioned foreign tables:

+ Constraints on foreign tables are not enforced on insert or update.
+ Those definitions simply declare the constraints hold for all rows
+ in the foreign tables.  It is the user's responsibility to ensure
+ that those definitions match the remote side.  Such constraints are
+ used for some kind of query optimization such as constraint
exclusion
+ for partitioned tables


The planner can do constraint exclusion based on CHECK constraints even
without inheritance. It's not enabled by default because it can increase
planning time, but if you set constraint_exclusion=on, it will work.


Exactly!


For example:

postgres=# create table foo (i int4 CHECK (i  0));
CREATE TABLE
postgres=# explain select * from foo WHERE i  0;
   QUERY PLAN
--
  Seq Scan on foo  (cost=0.00..40.00 rows=800 width=4)
Filter: (i  0)
  Planning time: 0.335 ms
(3 rows)

postgres=# show constraint_exclusion ;
  constraint_exclusion
--
  partition
(1 row)

postgres=# set constraint_exclusion ='on';
SET
postgres=# explain select * from foo WHERE i  0;
 QUERY PLAN
--
  Result  (cost=0.00..0.01 rows=1 width=0)
One-Time Filter: false
  Planning time: 0.254 ms
(3 rows)

postgres=#


Actually, this patch allows the exact same thing to apply to foreign 
tables.  My explanation was insufficient about that.  Sorry for that.


So, should I split the patch into the two?

Thanks,

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] inherit support for foreign tables

2014-09-11 Thread Heikki Linnakangas

On 09/11/2014 02:30 PM, Etsuro Fujita wrote:

Actually, this patch allows the exact same thing to apply to foreign
tables.  My explanation was insufficient about that.  Sorry for that.


Great, that's what I thought.


So, should I split the patch into the two?


Yeah, please do.

- Heikki


--
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] inherit support for foreign tables

2014-09-10 Thread Heikki Linnakangas

I had a cursory look at this patch and the discussions around this.

ISTM there are actually two new features in this: 1) allow CHECK 
constraints on foreign tables, and 2) support inheritance for foreign 
tables. How about splitting it into two?


- Heikki



--
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] inherit support for foreign tables

2014-09-01 Thread Kyotaro HORIGUCHI
Hello, I have a request with slight significance for the messages.

 I'd like to address this by emitting the second message as shown below:
 
 INFO:  analyzing public.parent
 INFO:  parent: scanned 0 of 0 pages, containing 0 live rows and 0 dead 
 rows; 0 rows in sample, 0 estimated total rows
 INFO:  analyzing public.parent inheritance tree
 INFO:  skipping analyze of public.parent inheritance tree --- this 
 inheritance tree contains foreign tables

In acquire_inherited_sample_rows(), the message below is emitted
when the parent explicitly specified in analyze command has at
least one foreign tables.

skipping analyze of \%s.%s\ inheritance tree --- this
 inheritance tree contains foreign tables

This message implicitly asserts (for me) that A inheritance tree
containing at least one foreign tables *always* cannot be
analyzed but in reality, we can let it go by specifying the
parent table explicitly. For example, the additional HINT or
DETAIL message would clarify that.

 INFO:  analyzing public.parent inheritance tree
 INFO:  skipping analyze of public.parent inheritance tree --- this 
 inheritance tree contains foreign tables
+ HINT:  You can analyze this inheritance tree by specifying public.parent to 
analze command

-- 
Kyotaro Horiguchi
NTT Open Source Software Center


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


Re: [HACKERS] inherit support for foreign tables

2014-08-28 Thread Etsuro Fujita

(2014/08/22 11:51), Noah Misch wrote:

On Wed, Aug 20, 2014 at 08:11:01PM +0900, Etsuro Fujita wrote:

(2014/07/02 11:23), Noah Misch wrote:

Your chosen ANALYZE behavior is fair, but the messaging from a database-wide
ANALYZE VERBOSE needs work:

INFO:  analyzing test_foreign_inherit.parent
INFO:  parent: scanned 1 of 1 pages, containing 1 live rows and 0 dead rows; 
1 rows in sample, 1 estimated total rows
INFO:  analyzing test_foreign_inherit.parent inheritance tree



Please arrange to omit the 'analyzing tablename inheritance tree' message,
since this ANALYZE actually skipped that task.



I think it would be better that this is handled in the same way as
an inheritance tree that turns out to be a singe table that doesn't
have any descendants in acquire_inherited_sample_rows().  That would
still output the message as shown below, but I think that that would
be more consistent with the existing code.  The patch works so.



Today's ANALYZE VERBOSE messaging for former inheritance parents (tables with
relhassubclass = true but no pg_inherits.inhparent links) is deceptive, and I
welcome a fix to omit the spurious message.  As defects go, this is quite
minor.  There's fundamentally no value in collecting inheritance tree
statistics for such a parent, and no PostgreSQL command will do so.

Your proposed behavior for inheritance parents having at least one foreign
table child is more likely to mislead DBAs in practice.  An inheritance tree
genuinely exists, and a different ANALYZE command is quite capable of
collecting statistics on that inheritance tree.  Messaging should reflect the
difference between ANALYZE invocations that do such work and ANALYZE
invocations that skip it.  I'm anticipating a bug report along these lines:

   I saw poor estimates involving a child foreign table, so I ran ANALYZE
   VERBOSE, which reported 'INFO:  analyzing public.parent inheritance
   tree'.  Estimates remained poor, so I scratched my head for awhile before
   noticing that pg_stats didn't report such statistics.  I then ran ANALYZE
   VERBOSE parent, saw the same 'INFO:  analyzing public.parent inheritance
   tree', but this time saw relevant rows in pg_stats.  The message doesn't
   reflect the actual behavior.

I'll sympathize with that complaint.  It's a minor point overall, but the code
impact is, I predict, small enough that we may as well get it right.  A
credible alternative is to emit a second message indicating that we skipped
the inheritance tree statistics after all, and why we skipped them.


I'd like to address this by emitting the second message as shown below:

INFO:  analyzing public.parent
INFO:  parent: scanned 0 of 0 pages, containing 0 live rows and 0 dead 
rows; 0 rows in sample, 0 estimated total rows

INFO:  analyzing public.parent inheritance tree
INFO:  skipping analyze of public.parent inheritance tree --- this 
inheritance tree contains foreign tables


Attached is the update version of the patch.  Based on the result of 
discussions about attr_needed upthread, I've changed the patch so that 
create_foreignscan_plan makes reference to reltargetlist, not to 
attr_needed.  (So, the patch in [1] isn't required, anymore.)


Other changes:
* Revise code/comments/docs a bit
* Add more regression tests

A separate patch (analyze.patch) handles the former case in a similar way.

[1] http://www.postgresql.org/message-id/53f4707c.8030...@lab.ntt.co.jp

Thanks,

Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***
*** 115,120  static void fileGetForeignRelSize(PlannerInfo *root,
--- 115,125 
  static void fileGetForeignPaths(PlannerInfo *root,
  	RelOptInfo *baserel,
  	Oid foreigntableid);
+ static ForeignPath *fileReparameterizeForeignPath(PlannerInfo *root,
+   RelOptInfo *baserel,
+   Oid foreigntableid,
+   ForeignPath *path,
+   Relids required_outer);
  static ForeignScan *fileGetForeignPlan(PlannerInfo *root,
     RelOptInfo *baserel,
     Oid foreigntableid,
***
*** 143,149  static bool check_selective_binary_conversion(RelOptInfo *baserel,
  static void estimate_size(PlannerInfo *root, RelOptInfo *baserel,
  			  FileFdwPlanState *fdw_private);
  static void estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
! 			   FileFdwPlanState *fdw_private,
  			   Cost *startup_cost, Cost *total_cost);
  static int file_acquire_sample_rows(Relation onerel, int elevel,
  		 HeapTuple *rows, int targrows,
--- 148,154 
  static void estimate_size(PlannerInfo *root, RelOptInfo *baserel,
  			  FileFdwPlanState *fdw_private);
  static void estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
! 			   FileFdwPlanState *fdw_private, List *join_conds,
  			   Cost *startup_cost, Cost *total_cost);
  static int file_acquire_sample_rows(Relation onerel, int elevel,
  		 HeapTuple *rows, int targrows,
***
*** 161,166  

Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

2014-08-26 Thread Shigeru Hanada
Hi Fujita-san,

I reviewed the v4 patch, and here are some comments from me.

* After applying this patch, pull_varattnos() should not called in
unnecessary places.  For developers who want list of
columns-to-be-processed for some purpose, it would be nice to mention
when they should use pull_varattnos() and when they should not, maybe
at the comments of pull_varattnos() implementation.

* deparseReturningList() and postgresGetForeignRelSize() in
contrib/postgres_fdw/ also call pull_varattnos() to determine which
column to be in the SELECT clause of remote query.  Shouldn't these be
replaced in the same manner?  Other pull_varattnos() calls are for
restrictions, so IIUC they can't be replaced.

* Through this review I thought up that lazy evaluation approach might
fit attr_needed.  I mean that we leave attr_needed for child relations
empty, and fill it at the first request for it.  This would avoid
useless computation of attr_needed for child relations, though this
idea has been considered but thrown away before...


2014-08-20 18:55 GMT+09:00 Etsuro Fujita fujita.ets...@lab.ntt.co.jp:
 Hi Ashutish,


 (2014/08/14 22:30), Ashutosh Bapat wrote:

 On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:

 (2014/08/08 18:51), Etsuro Fujita wrote:
   (2014/06/30 22:48), Tom Lane wrote:
   I wonder whether it isn't time to change that.  It was coded
 like that
   originally only because calculating the values would've been a
 waste of
   cycles at the time.  But this is at least the third place
 where it'd be
   useful to have attr_needed for child rels.

   I've revised the patch.

 There was a problem with the previous patch, which will be described
 below.  Attached is the updated version of the patch addressing that.


 Here are some more comments


 Thank you for the further review!


 attr_needed also has the attributes used in the restriction clauses as
 seen in distribute_qual_to_rels(), so, it looks unnecessary to call
 pull_varattnos() on the clauses in baserestrictinfo in functions
 check_selective_binary_conversion(), remove_unused_subquery_outputs(),
 check_index_only().


 IIUC, I think it's *necessary* to do that in those functions since the
 attributes used in the restriction clauses in baserestrictinfo are not added
 to attr_needed due the following code in distribute_qual_to_rels.

 /*
  * If it's a join clause (either naturally, or because delayed by
  * outer-join rules), add vars used in the clause to targetlists of
 their
  * relations, so that they will be emitted by the plan nodes that scan
  * those relations (else they won't be available at the join node!).
  *
  * Note: if the clause gets absorbed into an EquivalenceClass then this
  * may be unnecessary, but for now we have to do it to cover the case
  * where the EC becomes ec_broken and we end up reinserting the original
  * clauses into the plan.
  */
 if (bms_membership(relids) == BMS_MULTIPLE)
 {
 List   *vars = pull_var_clause(clause,
PVC_RECURSE_AGGREGATES,
PVC_INCLUDE_PLACEHOLDERS);

 add_vars_to_targetlist(root, vars, relids, false);
 list_free(vars);

 }

 Although in case of RTE_RELATION, the 0th entry in attr_needed
 corresponds to FirstLowInvalidHeapAttributeNumber + 1, it's always safer
 to use it is RelOptInfo::min_attr, in case get_relation_info() wants to
 change assumption or somewhere down the line some other part of code
 wants to change attr_needed information. It may be unlikely, that it
 would change, but it will be better to stick to comments in RelOptInfo
   443 AttrNumber  min_attr;   /* smallest attrno of rel (often
 0) */
   444 AttrNumber  max_attr;   /* largest attrno of rel */
   445 Relids *attr_needed;/* array indexed [min_attr ..
 max_attr] */


 Good point!  Attached is the revised version of the patch.


 Thanks,

 Best regards,
 Etsuro Fujita



-- 
Shigeru HANADA


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


Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

2014-08-26 Thread Tom Lane
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 [ attr_needed-v4.patch ]

I looked this over, and TBH I'm rather disappointed.  The patch adds
150 lines of dubiously-correct code in order to save ... uh, well,
actually it *adds* code, because the places that are supposedly getting
a benefit are changed like this:

*** 799,806  check_selective_binary_conversion(RelOptInfo *baserel,
}
  
/* Collect all the attributes needed for joins or final output. */
!   pull_varattnos((Node *) baserel-reltargetlist, baserel-relid,
!  attrs_used);
  
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)
--- 799,810 
}
  
/* Collect all the attributes needed for joins or final output. */
!   for (i = baserel-min_attr; i = baserel-max_attr; i++)
!   {
!   if (!bms_is_empty(baserel-attr_needed[i - baserel-min_attr]))
!   attrs_used = bms_add_member(attrs_used,
!   
i - FirstLowInvalidHeapAttributeNumber);
!   }
  
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)

That's not simpler, it's not easier to understand, and it's probably not
faster either.  We could address some of those complaints by encapsulating
the above loop into a utility function, but still, I come away with the
feeling that it's not worth changing this.  Considering that all the
places that are doing this then proceed to use pull_varattnos to add on
attnos from the restriction clauses, it seems like using pull_varattnos
on the reltargetlist isn't such a bad thing after all.

So I'm inclined to reject this.  It seemed like a good idea in the
abstract, but the concrete result isn't very attractive, and doesn't
seem like an improvement over what we have.

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: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

2014-08-26 Thread Etsuro Fujita
(2014/08/27 3:27), Tom Lane wrote:
 Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 [ attr_needed-v4.patch ]
 
 I looked this over, and TBH I'm rather disappointed.  The patch adds
 150 lines of dubiously-correct code in order to save ... uh, well,

Just for my study, could you tell me why you think that the code is
dubiously-correct?

 Considering that all the
 places that are doing this then proceed to use pull_varattnos to add on
 attnos from the restriction clauses, it seems like using pull_varattnos
 on the reltargetlist isn't such a bad thing after all.

I agree with you on that point.

 So I'm inclined to reject this.  It seemed like a good idea in the
 abstract, but the concrete result isn't very attractive, and doesn't
 seem like an improvement over what we have.

Okay.  I'll withdraw the patch.

Thank you for taking the time to review the patch!

Best regards,
Etsuro Fujita


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


Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

2014-08-26 Thread Tom Lane
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 (2014/08/27 3:27), Tom Lane wrote:
 I looked this over, and TBH I'm rather disappointed.  The patch adds
 150 lines of dubiously-correct code in order to save ... uh, well,

 Just for my study, could you tell me why you think that the code is
 dubiously-correct?

It might be fine; I did not actually review the new
adjust_appendrel_attr_needed code in any detail.  What's scaring me off it
is (1) it's a lot longer and more complicated than I'd thought it would
be, and (2) you already made several bug fixes in it, which is often an
indicator that additional problems lurk.

It's possible there's some other, simpler, way to compute child
attr_needed arrays that would resolve (1) and (2).  However, even if we
had a simple and obviously-correct way to do that, it still seems like
there's not very much benefit to be had after all.  So my thought that
this would be worth doing seems wrong, and I must apologize to you for
sending you chasing down a dead end :-(

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: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

2014-08-26 Thread Etsuro Fujita
(2014/08/27 11:06), Tom Lane wrote:
 Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 (2014/08/27 3:27), Tom Lane wrote:
 I looked this over, and TBH I'm rather disappointed.  The patch adds
 150 lines of dubiously-correct code in order to save ... uh, well,
 
 Just for my study, could you tell me why you think that the code is
 dubiously-correct?
 
 It might be fine; I did not actually review the new
 adjust_appendrel_attr_needed code in any detail.  What's scaring me off it
 is (1) it's a lot longer and more complicated than I'd thought it would
 be, and (2) you already made several bug fixes in it, which is often an
 indicator that additional problems lurk.

Okay.

 It's possible there's some other, simpler, way to compute child
 attr_needed arrays that would resolve (1) and (2).  However, even if we
 had a simple and obviously-correct way to do that, it still seems like
 there's not very much benefit to be had after all.  So my thought that
 this would be worth doing seems wrong, and I must apologize to you for
 sending you chasing down a dead end :-(

Please don't worry yourself about that!

Thanks,

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] inherit support for foreign tables

2014-08-22 Thread Etsuro Fujita

(2014/08/22 12:58), Alvaro Herrera wrote:

Noah Misch wrote:


I'm anticipating a bug report along these lines:

   I saw poor estimates involving a child foreign table, so I ran ANALYZE
   VERBOSE, which reported 'INFO:  analyzing public.parent inheritance
   tree'.  Estimates remained poor, so I scratched my head for awhile before
   noticing that pg_stats didn't report such statistics.  I then ran ANALYZE
   VERBOSE parent, saw the same 'INFO:  analyzing public.parent inheritance
   tree', but this time saw relevant rows in pg_stats.  The message doesn't
   reflect the actual behavior.

I'll sympathize with that complaint.


Agreed on that.


I've got the point.  Will fix.

Thanks for the comment!

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: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

2014-08-21 Thread Etsuro Fujita

(2014/08/21 13:21), Ashutosh Bapat wrote:

On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:



Hi Ashutish,


I am sorry that I mistook your name's spelling.


(2014/08/14 22:30), Ashutosh Bapat wrote:

On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp
mailto:fujita.ets...@lab.ntt.co.jp
mailto:fujita.ets...@lab.ntt.__co.jp
mailto:fujita.ets...@lab.ntt.co.jp wrote:

 (2014/08/08 18:51), Etsuro Fujita wrote:
   (2014/06/30 22:48), Tom Lane wrote:
   I wonder whether it isn't time to change that.  It
was coded
 like that
   originally only because calculating the values
would've been a
 waste of
   cycles at the time.  But this is at least the third place
 where it'd be
   useful to have attr_needed for child rels.



 There was a problem with the previous patch, which will be
described
 below.  Attached is the updated version of the patch
addressing that.



Here are some more comments



attr_needed also has the attributes used in the restriction
clauses as
seen in distribute_qual_to_rels(), so, it looks unnecessary to call
pull_varattnos() on the clauses in baserestrictinfo in functions
check_selective_binary___conversion(),
remove_unused_subquery___outputs(),
check_index_only().



IIUC, I think it's *necessary* to do that in those functions since
the attributes used in the restriction clauses in baserestrictinfo
are not added to attr_needed due the following code in
distribute_qual_to_rels.



That's right. Thanks for pointing that out.



Although in case of RTE_RELATION, the 0th entry in attr_needed
corresponds to FirstLowInvalidHeapAttributeNu__mber + 1, it's
always safer
to use it is RelOptInfo::min_attr, in case get_relation_info()
wants to
change assumption or somewhere down the line some other part of code
wants to change attr_needed information. It may be unlikely, that it
would change, but it will be better to stick to comments in
RelOptInfo
   443 AttrNumber  min_attr;   /* smallest attrno of rel
(often
0) */
   444 AttrNumber  max_attr;   /* largest attrno of rel */
   445 Relids *attr_needed;/* array indexed [min_attr ..
max_attr] */



Good point!  Attached is the revised version of the patch.



If the patch is not in the commit-fest, can you please add it there?


I've already done that:

https://commitfest.postgresql.org/action/patch_view?id=1529


 From my side, the review is done, it should be marked ready for
committer, unless somebody else wants to review.


Many thanks!

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] inherit support for foreign tables

2014-08-21 Thread Etsuro Fujita

(2014/07/02 11:23), Noah Misch wrote:

On Fri, Jun 20, 2014 at 05:04:06PM +0900, Kyotaro HORIGUCHI wrote:

Attached is the rebased patch of v11 up to the current master.



The rest of these review comments are strictly observations; they're not
requests for you to change the patch, but they may deserve more discussion.

We use the term child table in many messages.  Should that change to
something more inclusive, now that the child may be a foreign table?  Perhaps
one of child relation, plain child, or child foreign table/child table
depending on the actual object?  child relation is robust technically, but
it might add more confusion than it removes.  Varying the message depending on
the actual object feels like a waste.  Opinions?


IMHO, I think that child table would not confusing users terribly.


LOCK TABLE on the inheritance parent locks child foreign tables, but LOCK
TABLE fails when given a foreign table directly.  That's odd, but I see no
cause to change it.


I agree wth that.


The longstanding behavior of CREATE TABLE INHERITS is to reorder local columns
to match the order found in parents.  That is, both of these tables actually
have columns in the order (a,b):

create table parent (a int, b int);
create table child (b int, a int) inherits (parent);

Ordinary dump/reload always uses CREATE TABLE INHERITS, thereby changing
column order in this way.  (pg_dump --binary-upgrade uses ALTER TABLE INHERIT
and some catalog hacks to avoid doing so.)  I've never liked that dump/reload
can change column order, but it's tolerable if you follow the best practice of
always writing out column lists.  The stakes rise for foreign tables, because
column order is inherently significant to file_fdw and probably to certain
other non-RDBMS FDWs.  If pg_dump changes column order in your file_fdw
foreign table, the table breaks.  I would heartily support making pg_dump
preserve column order for all inheritance children.  That doesn't rise to the
level of being a blocker for this patch, though.


I agree with that, too.  I think it would be better to add docs for now.

Thanks,

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] inherit support for foreign tables

2014-08-21 Thread Noah Misch
On Wed, Aug 20, 2014 at 08:11:01PM +0900, Etsuro Fujita wrote:
 (2014/07/02 11:23), Noah Misch wrote:
 Your chosen ANALYZE behavior is fair, but the messaging from a database-wide
 ANALYZE VERBOSE needs work:
 
 INFO:  analyzing test_foreign_inherit.parent
 INFO:  parent: scanned 1 of 1 pages, containing 1 live rows and 0 dead 
 rows; 1 rows in sample, 1 estimated total rows
 INFO:  analyzing test_foreign_inherit.parent inheritance tree
 WARNING:  relcache reference leak: relation child not closed
 WARNING:  relcache reference leak: relation tchild not closed
 WARNING:  relcache reference leak: relation parent not closed
 
 Please arrange to omit the 'analyzing tablename inheritance tree' message,
 since this ANALYZE actually skipped that task.  (The warnings obviously need 
 a
 fix, too.)  I do find it awkward that adding a foreign table to an 
 inheritance
 tree will make autovacuum stop collecting statistics on that inheritance 
 tree,
 but I can't think of a better policy.
 
 I think it would be better that this is handled in the same way as
 an inheritance tree that turns out to be a singe table that doesn't
 have any descendants in acquire_inherited_sample_rows().  That would
 still output the message as shown below, but I think that that would
 be more consistent with the existing code.  The patch works so.
 (The warnings are also fixed.)
 
 INFO:  analyzing public.parent
 INFO:  parent: scanned 0 of 0 pages, containing 0 live rows and 0
 dead rows; 0 rows in sample, 0 estimated total rows
 INFO:  analyzing public.parent inheritance tree
 INFO:  analyzing pg_catalog.pg_authid
 INFO:  pg_authid: scanned 1 of 1 pages, containing 1 live rows and
 0 dead rows; 1 rows in sample, 1 estimated total rows

Today's ANALYZE VERBOSE messaging for former inheritance parents (tables with
relhassubclass = true but no pg_inherits.inhparent links) is deceptive, and I
welcome a fix to omit the spurious message.  As defects go, this is quite
minor.  There's fundamentally no value in collecting inheritance tree
statistics for such a parent, and no PostgreSQL command will do so.

Your proposed behavior for inheritance parents having at least one foreign
table child is more likely to mislead DBAs in practice.  An inheritance tree
genuinely exists, and a different ANALYZE command is quite capable of
collecting statistics on that inheritance tree.  Messaging should reflect the
difference between ANALYZE invocations that do such work and ANALYZE
invocations that skip it.  I'm anticipating a bug report along these lines:

  I saw poor estimates involving a child foreign table, so I ran ANALYZE
  VERBOSE, which reported 'INFO:  analyzing public.parent inheritance
  tree'.  Estimates remained poor, so I scratched my head for awhile before
  noticing that pg_stats didn't report such statistics.  I then ran ANALYZE
  VERBOSE parent, saw the same 'INFO:  analyzing public.parent inheritance
  tree', but this time saw relevant rows in pg_stats.  The message doesn't
  reflect the actual behavior.

I'll sympathize with that complaint.  It's a minor point overall, but the code
impact is, I predict, small enough that we may as well get it right.  A
credible alternative is to emit a second message indicating that we skipped
the inheritance tree statistics after all, and why we skipped them.


-- 
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] inherit support for foreign tables

2014-08-21 Thread Alvaro Herrera
Noah Misch wrote:

 I'm anticipating a bug report along these lines:
 
   I saw poor estimates involving a child foreign table, so I ran ANALYZE
   VERBOSE, which reported 'INFO:  analyzing public.parent inheritance
   tree'.  Estimates remained poor, so I scratched my head for awhile before
   noticing that pg_stats didn't report such statistics.  I then ran ANALYZE
   VERBOSE parent, saw the same 'INFO:  analyzing public.parent inheritance
   tree', but this time saw relevant rows in pg_stats.  The message doesn't
   reflect the actual behavior.
 
 I'll sympathize with that complaint.

Agreed on that.

 A credible alternative is to emit a second message indicating that we
 skipped the inheritance tree statistics after all, and why we skipped
 them.

That'd be similar to Xorg emitting messages such as

[53.772] (II) intel(0): RandR 1.2 enabled, ignore the following RandR 
disabled message.
[53.800] (--) RandR disabled

I find this very poor.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training  Services


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


Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

2014-08-21 Thread Ashutosh Bapat
On Thu, Aug 21, 2014 at 3:00 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 (2014/08/21 13:21), Ashutosh Bapat wrote:

 On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:


  Hi Ashutish,


 I am sorry that I mistook your name's spelling.

  (2014/08/14 22:30), Ashutosh Bapat wrote:

 On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp
 mailto:fujita.ets...@lab.ntt.co.jp
 mailto:fujita.ets...@lab.ntt.__co.jp

 mailto:fujita.ets...@lab.ntt.co.jp wrote:

  (2014/08/08 18:51), Etsuro Fujita wrote:
(2014/06/30 22:48), Tom Lane wrote:
I wonder whether it isn't time to change that.  It
 was coded
  like that
originally only because calculating the values
 would've been a
  waste of
cycles at the time.  But this is at least the third
 place
  where it'd be
useful to have attr_needed for child rels.


   There was a problem with the previous patch, which will be
 described
  below.  Attached is the updated version of the patch
 addressing that.


  Here are some more comments


  attr_needed also has the attributes used in the restriction
 clauses as
 seen in distribute_qual_to_rels(), so, it looks unnecessary to
 call
 pull_varattnos() on the clauses in baserestrictinfo in functions
 check_selective_binary___conversion(),
 remove_unused_subquery___outputs(),
 check_index_only().


  IIUC, I think it's *necessary* to do that in those functions since
 the attributes used in the restriction clauses in baserestrictinfo
 are not added to attr_needed due the following code in
 distribute_qual_to_rels.


  That's right. Thanks for pointing that out.


  Although in case of RTE_RELATION, the 0th entry in attr_needed
 corresponds to FirstLowInvalidHeapAttributeNu__mber + 1, it's

 always safer
 to use it is RelOptInfo::min_attr, in case get_relation_info()
 wants to
 change assumption or somewhere down the line some other part of
 code
 wants to change attr_needed information. It may be unlikely, that
 it
 would change, but it will be better to stick to comments in
 RelOptInfo
443 AttrNumber  min_attr;   /* smallest attrno of rel
 (often
 0) */
444 AttrNumber  max_attr;   /* largest attrno of rel */
445 Relids *attr_needed;/* array indexed [min_attr
 ..
 max_attr] */


  Good point!  Attached is the revised version of the patch.


  If the patch is not in the commit-fest, can you please add it there?


 I've already done that:

 https://commitfest.postgresql.org/action/patch_view?id=1529


   From my side, the review is done, it should be marked ready for
 committer, unless somebody else wants to review.


 Many thanks!


Thanks. Since, I haven't seen anybody else commenting here and I do not
have any further comments to make, I have marked it as ready for
committer.


 Best regards,
 Etsuro Fujita




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


Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

2014-08-20 Thread Etsuro Fujita

Hi Ashutish,

(2014/08/14 22:30), Ashutosh Bapat wrote:

On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:

(2014/08/08 18:51), Etsuro Fujita wrote:
  (2014/06/30 22:48), Tom Lane wrote:
  I wonder whether it isn't time to change that.  It was coded
like that
  originally only because calculating the values would've been a
waste of
  cycles at the time.  But this is at least the third place
where it'd be
  useful to have attr_needed for child rels.

  I've revised the patch.

There was a problem with the previous patch, which will be described
below.  Attached is the updated version of the patch addressing that.



Here are some more comments


Thank you for the further review!


attr_needed also has the attributes used in the restriction clauses as
seen in distribute_qual_to_rels(), so, it looks unnecessary to call
pull_varattnos() on the clauses in baserestrictinfo in functions
check_selective_binary_conversion(), remove_unused_subquery_outputs(),
check_index_only().


IIUC, I think it's *necessary* to do that in those functions since the 
attributes used in the restriction clauses in baserestrictinfo are not 
added to attr_needed due the following code in distribute_qual_to_rels.


/*
 * If it's a join clause (either naturally, or because delayed by
 * outer-join rules), add vars used in the clause to targetlists of 
their

 * relations, so that they will be emitted by the plan nodes that scan
 * those relations (else they won't be available at the join node!).
 *
 * Note: if the clause gets absorbed into an EquivalenceClass then this
 * may be unnecessary, but for now we have to do it to cover the case
 * where the EC becomes ec_broken and we end up reinserting the 
original

 * clauses into the plan.
 */
if (bms_membership(relids) == BMS_MULTIPLE)
{
List   *vars = pull_var_clause(clause,
   PVC_RECURSE_AGGREGATES,
   PVC_INCLUDE_PLACEHOLDERS);

add_vars_to_targetlist(root, vars, relids, false);
list_free(vars);
}


Although in case of RTE_RELATION, the 0th entry in attr_needed
corresponds to FirstLowInvalidHeapAttributeNumber + 1, it's always safer
to use it is RelOptInfo::min_attr, in case get_relation_info() wants to
change assumption or somewhere down the line some other part of code
wants to change attr_needed information. It may be unlikely, that it
would change, but it will be better to stick to comments in RelOptInfo
  443 AttrNumber  min_attr;   /* smallest attrno of rel (often
0) */
  444 AttrNumber  max_attr;   /* largest attrno of rel */
  445 Relids *attr_needed;/* array indexed [min_attr ..
max_attr] */


Good point!  Attached is the revised version of the patch.

Thanks,

Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***
*** 799,806  check_selective_binary_conversion(RelOptInfo *baserel,
  	}
  
  	/* Collect all the attributes needed for joins or final output. */
! 	pull_varattnos((Node *) baserel-reltargetlist, baserel-relid,
!    attrs_used);
  
  	/* Add all the attributes used by restriction clauses. */
  	foreach(lc, baserel-baserestrictinfo)
--- 799,810 
  	}
  
  	/* Collect all the attributes needed for joins or final output. */
! 	for (i = baserel-min_attr; i = baserel-max_attr; i++)
! 	{
! 		if (!bms_is_empty(baserel-attr_needed[i - baserel-min_attr]))
! 			attrs_used = bms_add_member(attrs_used,
! 		i - FirstLowInvalidHeapAttributeNumber);
! 	}
  
  	/* Add all the attributes used by restriction clauses. */
  	foreach(lc, baserel-baserestrictinfo)
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
***
*** 577,588  set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
  		childrel-has_eclass_joins = rel-has_eclass_joins;
  
  		/*
! 		 * Note: we could compute appropriate attr_needed data for the child's
! 		 * variables, by transforming the parent's attr_needed through the
! 		 * translated_vars mapping.  However, currently there's no need
! 		 * because attr_needed is only examined for base relations not
! 		 * otherrels.  So we just leave the child's attr_needed empty.
  		 */
  
  		/*
  		 * Compute the child's size.
--- 577,585 
  		childrel-has_eclass_joins = rel-has_eclass_joins;
  
  		/*
! 		 * Compute the child's attr_needed.
  		 */
+ 		adjust_appendrel_attr_needed(rel, childrel, appinfo);
  
  		/*
  		 * Compute the child's size.
***
*** 2173,2178  remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
--- 2170,2176 
  {
  	Bitmapset  *attrs_used = NULL;
  	ListCell   *lc;
+ 	int			i;
  
  	/*
  	 * Do nothing if subquery has UNION/INTERSECT/EXCEPT: in principle we
***
*** 

Re: [HACKERS] inherit support for foreign tables

2014-08-20 Thread Etsuro Fujita

Hi Noah,

Thank you for the review!

(2014/07/02 11:23), Noah Misch wrote:

On Fri, Jun 20, 2014 at 05:04:06PM +0900, Kyotaro HORIGUCHI wrote:

Attached is the rebased patch of v11 up to the current master.


I've been studying this patch.

SELECT FOR UPDATE on the inheritance parent fails with a can't-happen error
condition, even when SELECT FOR UPDATE on the child foreign table alone would
have succeeded.


To fix this, I've modified the planner and executor so that the planner 
adds wholerow as well as ctid and tableoid as resjunk output columns to 
the plan for an inheritance tree that contains foreign tables, and that
while the executor uses the ctid and tableoid in the EPQ processing for 
child regular tables as before, the executor uses the wholerow and 
tableoid for child foreign tables.  Patch attached.  This is based on 
the patch [1].



The patch adds zero tests.  Add principal tests to postgres_fdw.sql.  Also,
create a child foreign table in foreign_data.sql; this will make dump/reload
tests of the regression database exercise an inheritance tree that includes a
foreign table.


Done.  I used your tests as reference.  Thanks!


The inheritance section of ddl.sgml should mention child foreign tables, at
least briefly.  Consider mentioning it in the partitioning section, too.


Done.


Your chosen ANALYZE behavior is fair, but the messaging from a database-wide
ANALYZE VERBOSE needs work:

INFO:  analyzing test_foreign_inherit.parent
INFO:  parent: scanned 1 of 1 pages, containing 1 live rows and 0 dead rows; 
1 rows in sample, 1 estimated total rows
INFO:  analyzing test_foreign_inherit.parent inheritance tree
WARNING:  relcache reference leak: relation child not closed
WARNING:  relcache reference leak: relation tchild not closed
WARNING:  relcache reference leak: relation parent not closed

Please arrange to omit the 'analyzing tablename inheritance tree' message,
since this ANALYZE actually skipped that task.  (The warnings obviously need a
fix, too.)  I do find it awkward that adding a foreign table to an inheritance
tree will make autovacuum stop collecting statistics on that inheritance tree,
but I can't think of a better policy.


I think it would be better that this is handled in the same way as an 
inheritance tree that turns out to be a singe table that doesn't have 
any descendants in acquire_inherited_sample_rows().  That would still 
output the message as shown below, but I think that that would be more 
consistent with the existing code.  The patch works so.  (The warnings 
are also fixed.)


INFO:  analyzing public.parent
INFO:  parent: scanned 0 of 0 pages, containing 0 live rows and 0 dead 
rows; 0 rows in sample, 0 estimated total rows

INFO:  analyzing public.parent inheritance tree
INFO:  analyzing pg_catalog.pg_authid
INFO:  pg_authid: scanned 1 of 1 pages, containing 1 live rows and 0 
dead rows; 1 rows in sample, 1 estimated total rows



The rest of these review comments are strictly observations; they're not
requests for you to change the patch, but they may deserve more discussion.


I'd like to give my opinions on those things later on.

Sorry for the long delay.

[1] http://www.postgresql.org/message-id/53f4707c.8030...@lab.ntt.co.jp

Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***
*** 115,120  static void fileGetForeignRelSize(PlannerInfo *root,
--- 115,125 
  static void fileGetForeignPaths(PlannerInfo *root,
  	RelOptInfo *baserel,
  	Oid foreigntableid);
+ static ForeignPath *fileReparameterizeForeignPath(PlannerInfo *root,
+   RelOptInfo *baserel,
+   Oid foreigntableid,
+   ForeignPath *path,
+   Relids required_outer);
  static ForeignScan *fileGetForeignPlan(PlannerInfo *root,
     RelOptInfo *baserel,
     Oid foreigntableid,
***
*** 143,149  static bool check_selective_binary_conversion(RelOptInfo *baserel,
  static void estimate_size(PlannerInfo *root, RelOptInfo *baserel,
  			  FileFdwPlanState *fdw_private);
  static void estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
! 			   FileFdwPlanState *fdw_private,
  			   Cost *startup_cost, Cost *total_cost);
  static int file_acquire_sample_rows(Relation onerel, int elevel,
  		 HeapTuple *rows, int targrows,
--- 148,154 
  static void estimate_size(PlannerInfo *root, RelOptInfo *baserel,
  			  FileFdwPlanState *fdw_private);
  static void estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
! 			   FileFdwPlanState *fdw_private, List *join_conds,
  			   Cost *startup_cost, Cost *total_cost);
  static int file_acquire_sample_rows(Relation onerel, int elevel,
  		 HeapTuple *rows, int targrows,
***
*** 161,166  file_fdw_handler(PG_FUNCTION_ARGS)
--- 166,172 
  
  	fdwroutine-GetForeignRelSize = fileGetForeignRelSize;
  	fdwroutine-GetForeignPaths = fileGetForeignPaths;
+ 	fdwroutine-ReparameterizeForeignPath = 

Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

2014-08-20 Thread Ashutosh Bapat
On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 Hi Ashutish,


 (2014/08/14 22:30), Ashutosh Bapat wrote:

 On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:

 (2014/08/08 18:51), Etsuro Fujita wrote:
   (2014/06/30 22:48), Tom Lane wrote:
   I wonder whether it isn't time to change that.  It was coded
 like that
   originally only because calculating the values would've been a
 waste of
   cycles at the time.  But this is at least the third place
 where it'd be
   useful to have attr_needed for child rels.

   I've revised the patch.

 There was a problem with the previous patch, which will be described
 below.  Attached is the updated version of the patch addressing that.


  Here are some more comments


 Thank you for the further review!


  attr_needed also has the attributes used in the restriction clauses as
 seen in distribute_qual_to_rels(), so, it looks unnecessary to call
 pull_varattnos() on the clauses in baserestrictinfo in functions
 check_selective_binary_conversion(), remove_unused_subquery_outputs(),
 check_index_only().


 IIUC, I think it's *necessary* to do that in those functions since the
 attributes used in the restriction clauses in baserestrictinfo are not
 added to attr_needed due the following code in distribute_qual_to_rels.


That's right. Thanks for pointing that out.


 /*
  * If it's a join clause (either naturally, or because delayed by
  * outer-join rules), add vars used in the clause to targetlists of
 their
  * relations, so that they will be emitted by the plan nodes that scan
  * those relations (else they won't be available at the join node!).
  *
  * Note: if the clause gets absorbed into an EquivalenceClass then this
  * may be unnecessary, but for now we have to do it to cover the case
  * where the EC becomes ec_broken and we end up reinserting the
 original
  * clauses into the plan.
  */
 if (bms_membership(relids) == BMS_MULTIPLE)
 {
 List   *vars = pull_var_clause(clause,
PVC_RECURSE_AGGREGATES,
PVC_INCLUDE_PLACEHOLDERS);

 add_vars_to_targetlist(root, vars, relids, false);
 list_free(vars);

 }

  Although in case of RTE_RELATION, the 0th entry in attr_needed
 corresponds to FirstLowInvalidHeapAttributeNumber + 1, it's always safer
 to use it is RelOptInfo::min_attr, in case get_relation_info() wants to
 change assumption or somewhere down the line some other part of code
 wants to change attr_needed information. It may be unlikely, that it
 would change, but it will be better to stick to comments in RelOptInfo
   443 AttrNumber  min_attr;   /* smallest attrno of rel (often
 0) */
   444 AttrNumber  max_attr;   /* largest attrno of rel */
   445 Relids *attr_needed;/* array indexed [min_attr ..
 max_attr] */


 Good point!  Attached is the revised version of the patch.


If the patch is not in the commit-fest, can you please add it there? From
my side, the review is done, it should be marked ready for committer,
unless somebody else wants to review.



 Thanks,

 Best regards,
 Etsuro Fujita




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


Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

2014-08-14 Thread Ashutosh Bapat
Hi,



On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
 wrote:

 (2014/08/08 18:51), Etsuro Fujita wrote:
  (2014/06/30 22:48), Tom Lane wrote:
  I wonder whether it isn't time to change that.  It was coded like that
  originally only because calculating the values would've been a waste
 of
  cycles at the time.  But this is at least the third place where it'd
 be
  useful to have attr_needed for child rels.

  I've revised the patch.

 There was a problem with the previous patch, which will be described
 below.  Attached is the updated version of the patch addressing that.

 The previous patch doesn't cope with some UNION ALL cases properly.  So,
 e.g., the server will crash for the following query:

 postgres=# create table ta1 (f1 int);
 CREATE TABLE
 postgres=# create table ta2 (f2 int primary key, f3 int);
 CREATE TABLE
 postgres=# create table tb1 (f1 int);
 CREATE TABLE
 postgres=# create table tb2 (f2 int primary key, f3 int);
 CREATE TABLE
 postgres=# explain verbose select f1 from ((select f1, f2 from (select
 f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all
 (select f1,
 f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1)
 ssb)) ss;

 With the updated version, we get the right result:

 postgres=# explain verbose select f1 from ((select f1, f2 from (select
 f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all
 (select f1,
 f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1)
 ssb)) ss;
QUERY PLAN

 
  Append  (cost=0.00..0.05 rows=2 width=4)
-  Subquery Scan on ssa  (cost=0.00..0.02 rows=1 width=4)
  Output: ssa.f1
  -  Limit  (cost=0.00..0.01 rows=1 width=4)
Output: ta1.f1, (NULL::integer), (NULL::integer)
-  Seq Scan on public.ta1  (cost=0.00..34.00 rows=2400
 width=4)
  Output: ta1.f1, NULL::integer, NULL::integer
-  Subquery Scan on ssb  (cost=0.00..0.02 rows=1 width=4)
  Output: ssb.f1
  -  Limit  (cost=0.00..0.01 rows=1 width=4)
Output: tb1.f1, (NULL::integer), (NULL::integer)
-  Seq Scan on public.tb1  (cost=0.00..34.00 rows=2400
 width=4)
  Output: tb1.f1, NULL::integer, NULL::integer
  Planning time: 0.453 ms
 (14 rows)

 While thinking to address this problem, Ashutosh also expressed concern
 about the UNION ALL handling in the previous patch in a private email.
 Thank you for the review, Ashutosh!


Thanks for taking care of this one.

Here are some more comments

attr_needed also has the attributes used in the restriction clauses as seen
in distribute_qual_to_rels(), so, it looks unnecessary to call
pull_varattnos() on the clauses in baserestrictinfo in functions
check_selective_binary_conversion(), remove_unused_subquery_outputs(),
check_index_only().

Although in case of RTE_RELATION, the 0th entry in attr_needed corresponds
to FirstLowInvalidHeapAttributeNumber + 1, it's always safer to use it is
RelOptInfo::min_attr, in case get_relation_info() wants to change
assumption or somewhere down the line some other part of code wants to
change attr_needed information. It may be unlikely, that it would change,
but it will be better to stick to comments in RelOptInfo
 443 AttrNumber  min_attr;   /* smallest attrno of rel (often 0) */
 444 AttrNumber  max_attr;   /* largest attrno of rel */
 445 Relids *attr_needed;/* array indexed [min_attr ..
max_attr] */


 Thanks,

 Best regards,
 Etsuro Fujita




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


Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

2014-08-13 Thread Etsuro Fujita
(2014/08/08 18:51), Etsuro Fujita wrote:
 (2014/06/30 22:48), Tom Lane wrote:
 I wonder whether it isn't time to change that.  It was coded like that
 originally only because calculating the values would've been a waste of
 cycles at the time.  But this is at least the third place where it'd be
 useful to have attr_needed for child rels.

 I've revised the patch.

There was a problem with the previous patch, which will be described
below.  Attached is the updated version of the patch addressing that.

The previous patch doesn't cope with some UNION ALL cases properly.  So,
e.g., the server will crash for the following query:

postgres=# create table ta1 (f1 int);
CREATE TABLE
postgres=# create table ta2 (f2 int primary key, f3 int);
CREATE TABLE
postgres=# create table tb1 (f1 int);
CREATE TABLE
postgres=# create table tb2 (f2 int primary key, f3 int);
CREATE TABLE
postgres=# explain verbose select f1 from ((select f1, f2 from (select
f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all
(select f1,
f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1)
ssb)) ss;

With the updated version, we get the right result:

postgres=# explain verbose select f1 from ((select f1, f2 from (select
f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all
(select f1,
f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1)
ssb)) ss;
   QUERY PLAN

 Append  (cost=0.00..0.05 rows=2 width=4)
   -  Subquery Scan on ssa  (cost=0.00..0.02 rows=1 width=4)
 Output: ssa.f1
 -  Limit  (cost=0.00..0.01 rows=1 width=4)
   Output: ta1.f1, (NULL::integer), (NULL::integer)
   -  Seq Scan on public.ta1  (cost=0.00..34.00 rows=2400
width=4)
 Output: ta1.f1, NULL::integer, NULL::integer
   -  Subquery Scan on ssb  (cost=0.00..0.02 rows=1 width=4)
 Output: ssb.f1
 -  Limit  (cost=0.00..0.01 rows=1 width=4)
   Output: tb1.f1, (NULL::integer), (NULL::integer)
   -  Seq Scan on public.tb1  (cost=0.00..34.00 rows=2400
width=4)
 Output: tb1.f1, NULL::integer, NULL::integer
 Planning time: 0.453 ms
(14 rows)

While thinking to address this problem, Ashutosh also expressed concern
about the UNION ALL handling in the previous patch in a private email.
Thank you for the review, Ashutosh!

Thanks,

Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***
*** 799,806  check_selective_binary_conversion(RelOptInfo *baserel,
  	}
  
  	/* Collect all the attributes needed for joins or final output. */
! 	pull_varattnos((Node *) baserel-reltargetlist, baserel-relid,
!    attrs_used);
  
  	/* Add all the attributes used by restriction clauses. */
  	foreach(lc, baserel-baserestrictinfo)
--- 799,810 
  	}
  
  	/* Collect all the attributes needed for joins or final output. */
! 	for (i = baserel-min_attr; i = baserel-max_attr; i++)
! 	{
! 		if (!bms_is_empty(baserel-attr_needed[i - baserel-min_attr]))
! 			attrs_used = bms_add_member(attrs_used,
! 		i - FirstLowInvalidHeapAttributeNumber);
! 	}
  
  	/* Add all the attributes used by restriction clauses. */
  	foreach(lc, baserel-baserestrictinfo)
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
***
*** 577,588  set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
  		childrel-has_eclass_joins = rel-has_eclass_joins;
  
  		/*
! 		 * Note: we could compute appropriate attr_needed data for the child's
! 		 * variables, by transforming the parent's attr_needed through the
! 		 * translated_vars mapping.  However, currently there's no need
! 		 * because attr_needed is only examined for base relations not
! 		 * otherrels.  So we just leave the child's attr_needed empty.
  		 */
  
  		/*
  		 * Compute the child's size.
--- 577,585 
  		childrel-has_eclass_joins = rel-has_eclass_joins;
  
  		/*
! 		 * Compute the child's attr_needed.
  		 */
+ 		adjust_appendrel_attr_needed(rel, childrel, appinfo);
  
  		/*
  		 * Compute the child's size.
***
*** 2173,2178  remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
--- 2170,2176 
  {
  	Bitmapset  *attrs_used = NULL;
  	ListCell   *lc;
+ 	int			i;
  
  	/*
  	 * Do nothing if subquery has UNION/INTERSECT/EXCEPT: in principle we
***
*** 2193,2204  remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
  	 * Collect a bitmap of all the output column numbers used by the upper
  	 * query.
  	 *
! 	 * Add all the attributes needed for joins or final output.  Note: we must
! 	 * look at reltargetlist, not the attr_needed data, because attr_needed
! 	 * isn't computed for inheritance child rels, cf set_append_rel_size().
! 	 * (XXX might be worth changing that sometime.)
  	 */
! 	

Re: [HACKERS] inherit support for foreign tables

2014-08-08 Thread Etsuro Fujita
(2014/08/06 20:43), Etsuro Fujita wrote:
 (2014/06/30 22:48), Tom Lane wrote:
 I wonder whether it isn't time to change that.  It was coded like that
 originally only because calculating the values would've been a waste of
 cycles at the time.  But this is at least the third place where it'd be
 useful to have attr_needed for child rels.

 Attached is a WIP patch for that.

I've revised the patch.

Changes:

* Make the code more readable
* Revise the comments
* Cleanup

Please find attached an updated version of the patch.

Thanks,

Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***
*** 799,806  check_selective_binary_conversion(RelOptInfo *baserel,
  	}
  
  	/* Collect all the attributes needed for joins or final output. */
! 	pull_varattnos((Node *) baserel-reltargetlist, baserel-relid,
!    attrs_used);
  
  	/* Add all the attributes used by restriction clauses. */
  	foreach(lc, baserel-baserestrictinfo)
--- 799,811 
  	}
  
  	/* Collect all the attributes needed for joins or final output. */
! 	for (i = baserel-min_attr; i = baserel-max_attr; i++)
! 	{
! 		if (!bms_is_empty(baserel-attr_needed[i - baserel-min_attr]))
! 			attrs_used =
! bms_add_member(attrs_used,
! 			   i - FirstLowInvalidHeapAttributeNumber);
! 	}
  
  	/* Add all the attributes used by restriction clauses. */
  	foreach(lc, baserel-baserestrictinfo)
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
***
*** 577,588  set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
  		childrel-has_eclass_joins = rel-has_eclass_joins;
  
  		/*
! 		 * Note: we could compute appropriate attr_needed data for the child's
! 		 * variables, by transforming the parent's attr_needed through the
! 		 * translated_vars mapping.  However, currently there's no need
! 		 * because attr_needed is only examined for base relations not
! 		 * otherrels.  So we just leave the child's attr_needed empty.
  		 */
  
  		/*
  		 * Compute the child's size.
--- 577,585 
  		childrel-has_eclass_joins = rel-has_eclass_joins;
  
  		/*
! 		 * Compute the child's attr_needed.
  		 */
+ 		adjust_appendrel_attr_needed(rel, childrel, childRTE, appinfo);
  
  		/*
  		 * Compute the child's size.
***
*** 2173,2178  remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
--- 2170,2176 
  {
  	Bitmapset  *attrs_used = NULL;
  	ListCell   *lc;
+ 	int			i;
  
  	/*
  	 * Do nothing if subquery has UNION/INTERSECT/EXCEPT: in principle we
***
*** 2193,2204  remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
  	 * Collect a bitmap of all the output column numbers used by the upper
  	 * query.
  	 *
! 	 * Add all the attributes needed for joins or final output.  Note: we must
! 	 * look at reltargetlist, not the attr_needed data, because attr_needed
! 	 * isn't computed for inheritance child rels, cf set_append_rel_size().
! 	 * (XXX might be worth changing that sometime.)
  	 */
! 	pull_varattnos((Node *) rel-reltargetlist, rel-relid, attrs_used);
  
  	/* Add all the attributes used by un-pushed-down restriction clauses. */
  	foreach(lc, rel-baserestrictinfo)
--- 2191,2205 
  	 * Collect a bitmap of all the output column numbers used by the upper
  	 * query.
  	 *
! 	 * Add all the attributes needed for joins or final output.
  	 */
! 	for (i = rel-min_attr; i = rel-max_attr; i++)
! 	{
! 		if (!bms_is_empty(rel-attr_needed[i - rel-min_attr]))
! 			attrs_used =
! bms_add_member(attrs_used,
! 			   i - FirstLowInvalidHeapAttributeNumber);
! 	}
  
  	/* Add all the attributes used by un-pushed-down restriction clauses. */
  	foreach(lc, rel-baserestrictinfo)
*** a/src/backend/optimizer/path/indxpath.c
--- b/src/backend/optimizer/path/indxpath.c
***
*** 1768,1779  check_index_only(RelOptInfo *rel, IndexOptInfo *index)
  	 * way out.
  	 */
  
! 	/*
! 	 * Add all the attributes needed for joins or final output.  Note: we must
! 	 * look at reltargetlist, not the attr_needed data, because attr_needed
! 	 * isn't computed for inheritance child rels.
! 	 */
! 	pull_varattnos((Node *) rel-reltargetlist, rel-relid, attrs_used);
  
  	/* Add all the attributes used by restriction clauses. */
  	foreach(lc, rel-baserestrictinfo)
--- 1768,1781 
  	 * way out.
  	 */
  
! 	/* Add all the attributes needed for joins or final output. */
! 	for (i = rel-min_attr; i = rel-max_attr; i++)
! 	{
! 		if (!bms_is_empty(rel-attr_needed[i - rel-min_attr]))
! 			attrs_used =
! bms_add_member(attrs_used,
! 			   i - FirstLowInvalidHeapAttributeNumber);
! 	}
  
  	/* Add all the attributes used by restriction clauses. */
  	foreach(lc, rel-baserestrictinfo)
*** a/src/backend/optimizer/prep/prepunion.c
--- b/src/backend/optimizer/prep/prepunion.c
***
*** 109,114  static Bitmapset *translate_col_privs(const Bitmapset *parent_privs,
--- 109,122 
  			

Re: [HACKERS] inherit support for foreign tables

2014-08-06 Thread Etsuro Fujita
(2014/07/01 11:10), Etsuro Fujita wrote:
 (2014/06/30 22:48), Tom Lane wrote:
 Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
 Done.  I think this is because create_foreignscan_plan() makes reference
 to attr_needed, which isn't computed for inheritance children.

 I wonder whether it isn't time to change that.  It was coded like that
 originally only because calculating the values would've been a waste of
 cycles at the time.  But this is at least the third place where it'd be
 useful to have attr_needed for child rels.
 
 +1 for calculating attr_needed for child rels.  (I was wondering too.)
 
 I'll create a separate patch for it.

Attached is a WIP patch for that.  The following functions have been
changed to refer to attr_needed:

* check_index_only()
* remove_unused_subquery_outputs()
* check_selective_binary_conversion()

I'll add this to the upcoming commitfest.  If anyone has any time to
glance at it before then, that would be a great help.

Thanks,

Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***
*** 799,806  check_selective_binary_conversion(RelOptInfo *baserel,
  	}
  
  	/* Collect all the attributes needed for joins or final output. */
! 	pull_varattnos((Node *) baserel-reltargetlist, baserel-relid,
!    attrs_used);
  
  	/* Add all the attributes used by restriction clauses. */
  	foreach(lc, baserel-baserestrictinfo)
--- 799,811 
  	}
  
  	/* Collect all the attributes needed for joins or final output. */
! 	for (i = baserel-min_attr; i = baserel-max_attr; i++)
! 	{
! 		if (!bms_is_empty(baserel-attr_needed[i - baserel-min_attr]))
! 			attrs_used =
! bms_add_member(attrs_used,
! 			   i - FirstLowInvalidHeapAttributeNumber);
! 	}
  
  	/* Add all the attributes used by restriction clauses. */
  	foreach(lc, baserel-baserestrictinfo)
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
***
*** 577,588  set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
  		childrel-has_eclass_joins = rel-has_eclass_joins;
  
  		/*
! 		 * Note: we could compute appropriate attr_needed data for the child's
! 		 * variables, by transforming the parent's attr_needed through the
! 		 * translated_vars mapping.  However, currently there's no need
! 		 * because attr_needed is only examined for base relations not
! 		 * otherrels.  So we just leave the child's attr_needed empty.
  		 */
  
  		/*
  		 * Compute the child's size.
--- 577,585 
  		childrel-has_eclass_joins = rel-has_eclass_joins;
  
  		/*
! 		 * Compute the child's attr_needed.
  		 */
+ 		adjust_appendrel_attr_needed(rel, childrel, childRTE, appinfo);
  
  		/*
  		 * Compute the child's size.
***
*** 2173,2178  remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
--- 2170,2176 
  {
  	Bitmapset  *attrs_used = NULL;
  	ListCell   *lc;
+ 	int			i;
  
  	/*
  	 * Do nothing if subquery has UNION/INTERSECT/EXCEPT: in principle we
***
*** 2193,2204  remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
  	 * Collect a bitmap of all the output column numbers used by the upper
  	 * query.
  	 *
! 	 * Add all the attributes needed for joins or final output.  Note: we must
! 	 * look at reltargetlist, not the attr_needed data, because attr_needed
! 	 * isn't computed for inheritance child rels, cf set_append_rel_size().
! 	 * (XXX might be worth changing that sometime.)
  	 */
! 	pull_varattnos((Node *) rel-reltargetlist, rel-relid, attrs_used);
  
  	/* Add all the attributes used by un-pushed-down restriction clauses. */
  	foreach(lc, rel-baserestrictinfo)
--- 2191,2205 
  	 * Collect a bitmap of all the output column numbers used by the upper
  	 * query.
  	 *
! 	 * Add all the attributes needed for joins or final output.
  	 */
! 	for (i = rel-min_attr; i = rel-max_attr; i++)
! 	{
! 		if (!bms_is_empty(rel-attr_needed[i - rel-min_attr]))
! 			attrs_used =
! bms_add_member(attrs_used,
! 			   i - FirstLowInvalidHeapAttributeNumber);
! 	}
  
  	/* Add all the attributes used by un-pushed-down restriction clauses. */
  	foreach(lc, rel-baserestrictinfo)
*** a/src/backend/optimizer/path/indxpath.c
--- b/src/backend/optimizer/path/indxpath.c
***
*** 1768,1779  check_index_only(RelOptInfo *rel, IndexOptInfo *index)
  	 * way out.
  	 */
  
! 	/*
! 	 * Add all the attributes needed for joins or final output.  Note: we must
! 	 * look at reltargetlist, not the attr_needed data, because attr_needed
! 	 * isn't computed for inheritance child rels.
! 	 */
! 	pull_varattnos((Node *) rel-reltargetlist, rel-relid, attrs_used);
  
  	/* Add all the attributes used by restriction clauses. */
  	foreach(lc, rel-baserestrictinfo)
--- 1768,1781 
  	 * way out.
  	 */
  
! 	/* Collect all the attributes needed for joins or final output. */
! 	for (i = rel-min_attr; i = rel-max_attr; i++)
! 	{
! 		if (!bms_is_empty(rel-attr_needed[i 

Re: [HACKERS] inherit support for foreign tables

2014-07-11 Thread Etsuro Fujita

(2014/07/10 18:12), Shigeru Hanada wrote:

2014-06-24 16:30 GMT+09:00 Etsuro Fujita fujita.ets...@lab.ntt.co.jp:

(2014/06/23 18:35), Ashutosh Bapat wrote:



Selecting tableoid on parent causes an error, ERROR:  cannot extract
system attribute from virtual tuple. The foreign table has an OID which
can be reported as tableoid for the rows coming from that foreign table.
Do we want to do that?



No.  I think it's a bug.  I'll fix it.



IIUC, you mean that tableoid can't be retrieved when a foreign table
is accessed via parent table,


No.  What I want to say is that tableoid *can* be retrieved when a 
foreign table is accessed via the parent table.


Thanks,

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] inherit support for foreign tables

2014-07-11 Thread Etsuro Fujita

(2014/07/11 15:50), Etsuro Fujita wrote:

(2014/07/10 18:12), Shigeru Hanada wrote:



IIUC, you mean that tableoid can't be retrieved when a foreign table
is accessed via parent table,


No.  What I want to say is that tableoid *can* be retrieved when a
foreign table is accessed via the parent table.


In fact, you can do that with v13 [1], but I plan to change the way of 
fixing (see [2]).


Thanks,

[1] http://www.postgresql.org/message-id/53b10914.2010...@lab.ntt.co.jp
[2] http://www.postgresql.org/message-id/53b2188b.4090...@lab.ntt.co.jp

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] inherit support for foreign tables

2014-07-10 Thread Shigeru Hanada
Hi Fujita-san,

Sorry for leaving this thread for long time.

2014-06-24 16:30 GMT+09:00 Etsuro Fujita fujita.ets...@lab.ntt.co.jp:
 (2014/06/23 18:35), Ashutosh Bapat wrote:

 Hi,
 Selecting tableoid on parent causes an error, ERROR:  cannot extract
 system attribute from virtual tuple. The foreign table has an OID which
 can be reported as tableoid for the rows coming from that foreign table.
 Do we want to do that?


 No.  I think it's a bug.  I'll fix it.

IIUC, you mean that tableoid can't be retrieved when a foreign table
is accessed via parent table, but it sounds strange to me, because one
of main purposes of tableoid is determine actual source table in
appended results.

Am I missing the point?

-- 
Shigeru HANADA


-- 
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] inherit support for foreign tables

2014-07-01 Thread Ashutosh Bapat
If we are going to change that portion of the code, we may as well go a bit
forward and allow any expressions to be fetched from a foreign server
(obviously, if that server is capable of doing so). It will help, when we
come to aggregate push-down or whole query push-down (whenever that
happens). So, instead of attr_needed, which restricts only the attributes
to be fetched, why not to targetlist itself?


On Mon, Jun 30, 2014 at 7:18 PM, Tom Lane t...@sss.pgh.pa.us wrote:

 Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
  Done.  I think this is because create_foreignscan_plan() makes reference
  to attr_needed, which isn't computed for inheritance children.

 I wonder whether it isn't time to change that.  It was coded like that
 originally only because calculating the values would've been a waste of
 cycles at the time.  But this is at least the third place where it'd be
 useful to have attr_needed for child rels.

 regards, tom lane




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


Re: [HACKERS] inherit support for foreign tables

2014-07-01 Thread Ashutosh Bapat
On Tue, Jul 1, 2014 at 7:39 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 (2014/06/30 20:17), Ashutosh Bapat wrote:

 On Mon, Jun 30, 2014 at 4:17 PM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:


  (2014/06/30 17:47), Ashutosh Bapat wrote:


  BTW, why aren't you using the tlist passed to this function? I
 guess
 create_scan_plan() passes tlist after processing it, so that
 should be
 used rather than rel-reltargetlist.


  I think that that would be maybe OK, but I think that it would not
 be efficient to use the list to compute attrs_used, because the
 tlist would have more information than rel-reltargetlist in cases
 where the tlist is build through build_physical_tlist().


  In that case, we can call build_relation_tlist() for foreign tables.


 Do you mean build_physical_tlist()?


Sorry, I meant build_path_tlist() or disuse_physical_tlist() whichever is
appropriate.

We may want to modify use_physical_tlist(), to return false, in case of
foreign tables. BTW, it does return false for inheritance trees.

 486 /*
 487  * Can't do it with inheritance cases either (mainly because Append
 488  * doesn't project).
 489  */
 490 if (rel-reloptkind != RELOPT_BASEREL)
 491 return false;

Yeah, we can call build_physical_tlist() (and do that in some cases), but
 if we call the function, it would generate a tlist that contains all Vars
 in the relation, not only those Vars actually needed by the query (ie, Vars
 in reltargetlist), and thus it would take more cycles to compute attr_used
 from the tlist than from reltargetlist.  That' what I wanted to say.


 Thanks,

 Best regards,
 Etsuro Fujita




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


Re: [HACKERS] inherit support for foreign tables

2014-07-01 Thread Kyotaro HORIGUCHI
Hello,

Sorry, this was no relation with this patch.

ForeignNext materializes the slot, which would be any of physical
and virtual tuple, when system column was requested. If it was a
virtual one, file_fdw makes this, heap_form_tuple generates the
tuple as DatumTuple. The result is a jumble of virtual and
physical. But the returning slot has tts_tuple so the caller
interprets it as a physical one.

Anyway the requests for xmin/xmax could not be prevented
beforehand in current framework, I did rewrite the physical tuple
header so as to really be a physical one.

This would be another patch, so I will put this into next CF if
this don't get any immediate objection.


 By the way, I tried xmin and xmax for the file_fdw tables.
 
 postgres=# select tableoid, xmin, xmax, * from passwd1;
 tableoid | xmin |xmax| uname | pass |  uid  |  gid  | .. 
16396 |  244 | 4294967295 | root  | x| 0 | 0 | root...
16396 |  252 | 4294967295 | bin   | x| 1 | 1 | bin...
16396 |  284 | 4294967295 | daemon| x| 2 | 2 | 
 daemon...
 
 The xmin and xmax apparently doesn't look sane. After some
 investigation, I found that they came from the following place in
 heap_form_tuple(), (call stach is show below)


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c
index 9cc5345..728db14 100644
--- a/src/backend/executor/nodeForeignscan.c
+++ b/src/backend/executor/nodeForeignscan.c
@@ -22,6 +22,8 @@
  */
 #include postgres.h
 
+#include access/transam.h
+#include access/htup_details.h
 #include executor/executor.h
 #include executor/nodeForeignscan.h
 #include foreign/fdwapi.h
@@ -58,8 +60,21 @@ ForeignNext(ForeignScanState *node)
 	 */
 	if (plan-fsSystemCol  !TupIsNull(slot))
 	{
+		bool		was_virtual_tuple = (slot-tts_tuple == NULL);
 		HeapTuple	tup = ExecMaterializeSlot(slot);
 
+		if (was_virtual_tuple)
+		{
+			/*
+			 * ExecMaterializeSlot fills the tuple header as a
+			 * DatumTupleFields if the slot was a virtual tuple, but a
+			 * physical one is needed here. So rewrite the tuple header as
+			 * HeapTupleFirelds.
+			 */
+			HeapTupleHeaderSetXmin(tup-t_data, FrozenTransactionId);
+			HeapTupleHeaderSetXmax(tup-t_data, InvalidTransactionId);
+			HeapTupleHeaderSetCmin(tup-t_data, FirstCommandId);
+		}
 		tup-t_tableOid = RelationGetRelid(node-ss.ss_currentRelation);
 	}
 

-- 
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] inherit support for foreign tables

2014-07-01 Thread Etsuro Fujita

(2014/07/01 15:13), Ashutosh Bapat wrote:

On Tue, Jul 1, 2014 at 7:39 AM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:



We may want to modify use_physical_tlist(), to return false, in case of
foreign tables. BTW, it does return false for inheritance trees.


Yeah, but please consider cases where foreign tables are not inheritance 
child rels (and any system columns are requested).



  486 /*
  487  * Can't do it with inheritance cases either (mainly because
Append
  488  * doesn't project).
  489  */
  490 if (rel-reloptkind != RELOPT_BASEREL)
  491 return false;

Yeah, we can call build_physical_tlist() (and do that in some
cases), but if we call the function, it would generate a tlist that
contains all Vars in the relation, not only those Vars actually
needed by the query (ie, Vars in reltargetlist), and thus it would
take more cycles to compute attr_used from the tlist than from
reltargetlist.  That' what I wanted to say.


Maybe I'm missing something, but what's the point of using the tlist, 
not reltargetlist?


Thanks,

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] inherit support for foreign tables

2014-07-01 Thread Ashutosh Bapat
On Tue, Jul 1, 2014 at 12:25 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 (2014/07/01 15:13), Ashutosh Bapat wrote:

 On Tue, Jul 1, 2014 at 7:39 AM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:


  We may want to modify use_physical_tlist(), to return false, in case of
 foreign tables. BTW, it does return false for inheritance trees.


 Yeah, but please consider cases where foreign tables are not inheritance
 child rels (and any system columns are requested).


486 /*
   487  * Can't do it with inheritance cases either (mainly because
 Append
   488  * doesn't project).
   489  */
   490 if (rel-reloptkind != RELOPT_BASEREL)
   491 return false;

 Yeah, we can call build_physical_tlist() (and do that in some
 cases), but if we call the function, it would generate a tlist that
 contains all Vars in the relation, not only those Vars actually
 needed by the query (ie, Vars in reltargetlist), and thus it would
 take more cycles to compute attr_used from the tlist than from
 reltargetlist.  That' what I wanted to say.


 Maybe I'm missing something, but what's the point of using the tlist, not
 reltargetlist?


Compliance with other create_*scan_plan() functions. The tlist passed to
those functions is sometimes preprocessed in create_scan_plan() and some of
the function it calls. If we use reltargetlist directly, we loose that
preprocessing. I have not see any of create_*scan_plan() fetch the
targetlist directly from RelOptInfo. It is always the one supplied by
build_path_tlist() or disuse_physical_tlist() (which in turn calls
build_path_tlist()) or build_physical_tlist().



 Thanks,

 Best regards,
 Etsuro Fujita




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


Re: [HACKERS] inherit support for foreign tables

2014-07-01 Thread Etsuro Fujita

(2014/07/01 16:04), Ashutosh Bapat wrote:

On Tue, Jul 1, 2014 at 12:25 PM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:



Maybe I'm missing something, but what's the point of using the
tlist, not reltargetlist?



Compliance with other create_*scan_plan() functions. The tlist passed to
those functions is sometimes preprocessed in create_scan_plan() and some
of the function it calls. If we use reltargetlist directly, we loose
that preprocessing. I have not see any of create_*scan_plan() fetch the
targetlist directly from RelOptInfo. It is always the one supplied by
build_path_tlist() or disuse_physical_tlist() (which in turn calls
build_path_tlist()) or build_physical_tlist().


I've got the point.

As I said upthread, I'll work on calculating attr_needed for child rels, 
and I hope that that will eliminate your concern.


Thanks,

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] inherit support for foreign tables

2014-07-01 Thread Kyotaro HORIGUCHI
Hi, 

At Tue, 01 Jul 2014 16:30:41 +0900, Etsuro Fujita fujita.ets...@lab.ntt.co.jp 
wrote in 53b263a1.3060...@lab.ntt.co.jp
 I've got the point.
 
 As I said upthread, I'll work on calculating attr_needed for child
 rels, and I hope that that will eliminate your concern.

Inheritance tree is expanded far after where attr_needed for the
parent built, in set_base_rel_sizes() in make_one_rel(). I'm
afraid that building all attr_needed for whole inheritance tree
altogether is a bit suffering.

I have wanted the point of inheritance expansion earlier for
another patch. Do you think that rearranging there? Or generate
them individually in crete_foreign_plan()?

Anyway, I'm lookin forward to your next patch. So no answer
needed.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center


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


Re: [HACKERS] inherit support for foreign tables

2014-07-01 Thread Noah Misch
On Fri, Jun 20, 2014 at 05:04:06PM +0900, Kyotaro HORIGUCHI wrote:
 Attached is the rebased patch of v11 up to the current master.

I've been studying this patch.

SELECT FOR UPDATE on the inheritance parent fails with a can't-happen error
condition, even when SELECT FOR UPDATE on the child foreign table alone would
have succeeded.

The patch adds zero tests.  Add principal tests to postgres_fdw.sql.  Also,
create a child foreign table in foreign_data.sql; this will make dump/reload
tests of the regression database exercise an inheritance tree that includes a
foreign table.

The inheritance section of ddl.sgml should mention child foreign tables, at
least briefly.  Consider mentioning it in the partitioning section, too.


Your chosen ANALYZE behavior is fair, but the messaging from a database-wide
ANALYZE VERBOSE needs work:

INFO:  analyzing test_foreign_inherit.parent
INFO:  parent: scanned 1 of 1 pages, containing 1 live rows and 0 dead rows; 
1 rows in sample, 1 estimated total rows
INFO:  analyzing test_foreign_inherit.parent inheritance tree
WARNING:  relcache reference leak: relation child not closed
WARNING:  relcache reference leak: relation tchild not closed
WARNING:  relcache reference leak: relation parent not closed

Please arrange to omit the 'analyzing tablename inheritance tree' message,
since this ANALYZE actually skipped that task.  (The warnings obviously need a
fix, too.)  I do find it awkward that adding a foreign table to an inheritance
tree will make autovacuum stop collecting statistics on that inheritance tree,
but I can't think of a better policy.


The rest of these review comments are strictly observations; they're not
requests for you to change the patch, but they may deserve more discussion.

We use the term child table in many messages.  Should that change to
something more inclusive, now that the child may be a foreign table?  Perhaps
one of child relation, plain child, or child foreign table/child table
depending on the actual object?  child relation is robust technically, but
it might add more confusion than it removes.  Varying the message depending on
the actual object feels like a waste.  Opinions?

LOCK TABLE on the inheritance parent locks child foreign tables, but LOCK
TABLE fails when given a foreign table directly.  That's odd, but I see no
cause to change it.

A partition root only accepts an UPDATE command if every child is updatable.
Similarly, UPDATE ... WHERE CURRENT OF cursor_name fails if any child does
not support it.  That seems fine.  Incidentally, does anyone have a FDW that
supports WHERE CURRENT OF?


The longstanding behavior of CREATE TABLE INHERITS is to reorder local columns
to match the order found in parents.  That is, both of these tables actually
have columns in the order (a,b):

create table parent (a int, b int);
create table child (b int, a int) inherits (parent);

Ordinary dump/reload always uses CREATE TABLE INHERITS, thereby changing
column order in this way.  (pg_dump --binary-upgrade uses ALTER TABLE INHERIT
and some catalog hacks to avoid doing so.)  I've never liked that dump/reload
can change column order, but it's tolerable if you follow the best practice of
always writing out column lists.  The stakes rise for foreign tables, because
column order is inherently significant to file_fdw and probably to certain
other non-RDBMS FDWs.  If pg_dump changes column order in your file_fdw
foreign table, the table breaks.  I would heartily support making pg_dump
preserve column order for all inheritance children.  That doesn't rise to the
level of being a blocker for this patch, though.


I am attaching rough-hewn tests I used during this review.

Thanks,
nm

-- 
Noah Misch
EnterpriseDB http://www.enterprisedb.com
drop schema test_foreign_inherit cascade;
create schema test_foreign_inherit;
set search_path = test_foreign_inherit;

create extension postgres_fdw;
CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw
  OPTIONS (dbname 'test');
CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;

create extension file_fdw;
CREATE SERVER f FOREIGN DATA WRAPPER file_fdw;

create table parent (
a int,
b text,
c date,
d path
);
insert into parent values (4, 'foo', current_date, '0,0,1,2');

-- regular child
create table tchild () inherits (parent);
insert into tchild values (5, 'oof', current_date - 2, '-1,-1,0,0');

-- cursor update works
begin;
declare c cursor for select 1 from parent where b = 'oof';
fetch from c;
explain analyze update parent set b = null where current of c;
select * from parent;
rollback;

-- foreign side
create table _child (
c date,
b text,
extra numeric,
d path,
a int
);

-- Foreign child.  No column reorder.
create foreign table child (
c date,
b text,
extra numeric,
d path,
a int
) SERVER LOOPBACK OPTIONS (table_name '_child');
alter table child inherit parent;
insert into 

Re: [HACKERS] inherit support for foreign tables

2014-06-30 Thread Ashutosh Bapat
I checked that it's reporting the right tableoid now.

BTW, why aren't you using the tlist passed to this function? I guess
create_scan_plan() passes tlist after processing it, so that should be used
rather than rel-reltargetlist.


On Mon, Jun 30, 2014 at 12:22 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
 wrote:

 (2014/06/24 16:30), Etsuro Fujita wrote:

 (2014/06/23 18:35), Ashutosh Bapat wrote:


  Selecting tableoid on parent causes an error, ERROR:  cannot extract
 system attribute from virtual tuple. The foreign table has an OID which
 can be reported as tableoid for the rows coming from that foreign table.
 Do we want to do that?


 No.  I think it's a bug.  I'll fix it.


 Done.  I think this is because create_foreignscan_plan() makes reference
 to attr_needed, which isn't computed for inheritance children.  To aboid
 this, I've modified create_foreignscan_plan() to see reltargetlist and
 baserestrictinfo, instead of attr_needed.  Please find attached an updated
 version of the patch.


 Sorry for the delay.

 Best regards,
 Etsuro Fujita




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


Re: [HACKERS] inherit support for foreign tables

2014-06-30 Thread Etsuro Fujita

(2014/06/30 17:47), Ashutosh Bapat wrote:

I checked that it's reporting the right tableoid now.


Thank you for the check.


BTW, why aren't you using the tlist passed to this function? I guess
create_scan_plan() passes tlist after processing it, so that should be
used rather than rel-reltargetlist.


I think that that would be maybe OK, but I think that it would not be 
efficient to use the list to compute attrs_used, because the tlist would 
have more information than rel-reltargetlist in cases where the tlist 
is build through build_physical_tlist().


Thanks,

Best regards,
Etsuro Fujita



On Mon, Jun 30, 2014 at 12:22 PM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:

(2014/06/24 16:30), Etsuro Fujita wrote:

(2014/06/23 18:35), Ashutosh Bapat wrote:


Selecting tableoid on parent causes an error, ERROR:
  cannot extract
system attribute from virtual tuple. The foreign table has
an OID which
can be reported as tableoid for the rows coming from that
foreign table.
Do we want to do that?


No.  I think it's a bug.  I'll fix it.


Done.  I think this is because create_foreignscan_plan() makes
reference to attr_needed, which isn't computed for inheritance
children.  To aboid this, I've modified create_foreignscan_plan() to
see reltargetlist and baserestrictinfo, instead of attr_needed.
  Please find attached an updated version of the patch.


Sorry for the delay.

Best regards,
Etsuro Fujita




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



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


Re: [HACKERS] inherit support for foreign tables

2014-06-30 Thread Ashutosh Bapat
On Mon, Jun 30, 2014 at 4:17 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:

 (2014/06/30 17:47), Ashutosh Bapat wrote:

 I checked that it's reporting the right tableoid now.


 Thank you for the check.


  BTW, why aren't you using the tlist passed to this function? I guess
 create_scan_plan() passes tlist after processing it, so that should be
 used rather than rel-reltargetlist.


 I think that that would be maybe OK, but I think that it would not be
 efficient to use the list to compute attrs_used, because the tlist would
 have more information than rel-reltargetlist in cases where the tlist is
 build through build_physical_tlist().


In that case, we can call build_relation_tlist() for foreign tables.



 Thanks,

 Best regards,
 Etsuro Fujita


 On Mon, Jun 30, 2014 at 12:22 PM, Etsuro Fujita
 fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:

 (2014/06/24 16:30), Etsuro Fujita wrote:

 (2014/06/23 18:35), Ashutosh Bapat wrote:


 Selecting tableoid on parent causes an error, ERROR:
   cannot extract
 system attribute from virtual tuple. The foreign table has
 an OID which
 can be reported as tableoid for the rows coming from that
 foreign table.
 Do we want to do that?


 No.  I think it's a bug.  I'll fix it.


 Done.  I think this is because create_foreignscan_plan() makes
 reference to attr_needed, which isn't computed for inheritance
 children.  To aboid this, I've modified create_foreignscan_plan() to
 see reltargetlist and baserestrictinfo, instead of attr_needed.
   Please find attached an updated version of the patch.


 Sorry for the delay.

 Best regards,
 Etsuro Fujita




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




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


  1   2   >