Re: [HACKERS] inherit support for foreign tables

[Tom Lane]

Andres Freund  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

[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

[Tom Lane]

Etsuro Fujita  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

[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

[Kyotaro HORIGUCHI]

Hello,

At Thu, 16 Apr 2015 14:43:33 -0700, David Fetter  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  wrote 
in 
> 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

[Robert Haas]

On Tue, Apr 14, 2015 at 8:35 PM, Kyotaro HORIGUCHI
 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

[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

[Etsuro Fujita]

On 2015/04/16 16:05, Etsuro Fujita wrote:
> On 2015/03/23 2:57, Tom Lane wrote:
>> Etsuro Fujita  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'


 
! This controls whether CREATE TABLE and
! CREATE TABLE AS include an OID column in
  newly-created tables, if neither WITH OIDS
  nor WITHOUT OIDS is specified. It also
  determines whether OIDs will be included in tables created by
--- 6745,6753 


 
! This controls whether CREATE TABLE,
! CREATE TABLE AS and
! CREATE FOREIGN TABLE include an OID column in
  newly-created tables, if neither WITH OIDS
  nor WITHOUT OIDS 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 ( expression )
--- 293,304 
  responsibility to ensure that the constraint definition matches
  reality.
 
+ 
+   
+To add OIDs to the table created by CREATE FOREIGN TABLE,
+enable the  configuration
+variable.
+   
   
  
   
*** 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

[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  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

[Etsuro Fujita]

On 2015/03/23 2:57, Tom Lane wrote:
> Etsuro Fujita  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

[Kyotaro HORIGUCHI]

Hello,

At Thu, 16 Apr 2015 12:20:47 +0900, Etsuro Fujita  
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

[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

[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  
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

[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

[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

[Etsuro Fujita]

On 2015/03/23 2:57, Tom Lane wrote:
> Etsuro Fujita  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

[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

[Ashutosh Bapat]

On Mon, Mar 23, 2015 at 12:09 AM, Robert Haas  wrote:

> On Sun, Mar 22, 2015 at 1:57 PM, Tom Lane  wrote:
> > Etsuro Fujita  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

[Robert Haas]

On Sun, Mar 22, 2015 at 1:57 PM, Tom Lane  wrote:
> Etsuro Fujita  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

[Tom Lane]

Etsuro Fujita  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

[Tom Lane]

Etsuro Fujita  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

[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   | 

Re: [HACKERS] inherit support for foreign tables

[Michael Paquier]

On Thu, Jan 15, 2015 at 4:35 PM, Etsuro Fujita 
wrote:

> 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?
>

Patch moved to CF 2015-02 with same status, "Ready for committer".
-- 
Michael

Re: [HACKERS] inherit support for foreign tables

[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

[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   | 
+

Re: [HACKERS] inherit support for foreign tables

[Tom Lane]

Etsuro Fujita  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

[Etsuro Fujita]

On 2014/12/18 7:04, Tom Lane wrote:
> Etsuro Fujita  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 'aaa%';
+ SELE

Re: [HACKERS] inherit support for foreign tables

[Etsuro Fujita]

(2014/12/18 7:04), Tom Lane wrote:
> Etsuro Fujita  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

[Tom Lane]

Etsuro Fujita  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

[Michael Paquier]

On Thu, Dec 11, 2014 at 2:54 PM, Ashutosh Bapat
 wrote:
> On Thu, Dec 11, 2014 at 8:39 AM, Etsuro Fujita 
> 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

[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

[Ashutosh Bapat]

I marked this as ready for committer.

On Thu, Dec 11, 2014 at 8:39 AM, Etsuro Fujita 
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

[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

[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 
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
>> 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 PostgreSQL
>> tables.
>> +We will refer to the child tables as partitions, though
>> we assume
>> +that they are normal PostgreSQL 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 )
>> 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
>> +).
>>
>
>  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 wrot

Re: [HACKERS] inherit support for foreign tables

[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
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 PostgreSQL tables.
+We will refer to the child tables as partitions, though
we assume
+that they are normal PostgreSQL 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 )
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
+).



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 translated the macro into the English 
description.



2. The functio

Re: [HACKERS] inherit support for foreign tables

[Etsuro Fujita]

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

On Sat, Dec 6, 2014 at 9:21 PM, Noah Misch 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

[Ashutosh Bapat]

On Sat, Dec 6, 2014 at 9:21 PM, Noah Misch  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
> > >> 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

[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

[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
> >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  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

[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  
> wrote:
> > (2014/12/03 19:35), Ashutosh Bapat wrote:
> >> On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita
> >> 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

[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  > wrote:
>
>> (2014/12/03 19:35), Ashutosh Bapat wrote:
>>
>>> On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita
>>> 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

[Ashutosh Bapat]

On Thu, Dec 4, 2014 at 9:05 AM, Etsuro Fujita 
wrote:

> (2014/12/03 19:35), Ashutosh Bapat wrote:
>
>> On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita
>> 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

[Etsuro Fujita]

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

On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita
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)


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.


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

[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  > wrote:
>
>> (2014/11/28 18:14), Ashutosh Bapat wrote:
>>
>>> On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita
>>> 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 l

Re: [HACKERS] inherit support for foreign tables

[Ashutosh Bapat]

On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita 
wrote:

> (2014/11/28 18:14), Ashutosh Bapat wrote:
>
>> On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita
>> 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.
>>
>

Re: [HACKERS] inherit support for foreign tables

[Etsuro Fujita]

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

On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita
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

[Ashutosh Bapat]

On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita 
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 PostgreSQL tables.
>> +We will refer to the child tables as partitions, though we assume
>> +that they are normal PostgreSQL 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 ) 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
>> +).
>>
>
> 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 "table"s.


>
>  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

Re: [HACKERS] inherit support for foreign tables

[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 PostgreSQL tables.
+We will refer to the child tables as partitions, though we assume
+that they are normal PostgreSQL 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 ) 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
+).


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 the row mark is placed on a parent table and
hence 

Re: [HACKERS] inherit support for foreign tables

[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

[Ashutosh Bapat]

On Mon, Nov 17, 2014 at 1:25 PM, Etsuro Fujita 
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

[Ashutosh Bapat]

Hi Fujita-san,
Here are my review comments for patch fdw-inh-3.patch.

Sanity

1. The patch applies cleanly
2. It compiles cleanly.
3. The server regression tests and tests in contrib modules pass.

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?

Documentation

1. The change in ddl.sgml
-We will refer to the child tables as partitions, though they
-are in every way normal PostgreSQL tables.
+We will refer to the child tables as partitions, though we assume
+that they are normal PostgreSQL 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.

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
+).

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".

2. The function has_foreign() be better named has_foreign_child()? This
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 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.

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.

4. In postgresGetForeignPlan(), the code added by this patch is required to
handle the case when the row mark is placed on a parent table and hence is
required to be applied on the child table. We need a comment explaining
this. Otherwise, the three step process to get the row mark information
isn't clear for a reader.

5. In expand_inherited_rtentry() why do you need a separate variable
hasForeign? Instead of using that variable, you can actually set/reset
rte->hasForeign since existence of even a single foreign child would mark
that member as true. - After typing this, I got the answer when I looked at
the function code. Every child's RTE is initially a copy of parent's RTE
and hence hasForeign status would be inherited by every child after the
first foreign child. I think, this reasoning should be added as comment
before assignment to rte->hasForeign at the end of the function.

6. The tests in foreign_data.sql are pretty extensive. Thanks for that. I
think, we should also check the effect of each of the following command
using \d on appropriate tables.
+CREATE FOREIGN TABLE ft2 () INHERITS (pt1)
+  SERVER s0 OPTIONS (delimiter ',', quote '"', "be quoted" 'value');
+ALTER FOREIGN TABLE ft2 NO INHERIT pt1;
+DROP FOREIGN TABLE ft2;
+CREATE FOREIGN TABLE ft2 (
+   c1 integer NOT NULL,
+   c2 text,
+   c3 date
+) SERVER s0 OPTIONS (delimiter ',', quote '"', "be quoted" 'value');
+ALTER FOREIGN TABLE ft2 INHERIT pt1;

Rest of the changes look good.


On Thu, Nov 13, 2014 at 12:21 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

>
>
> On Thu, Nov 13, 2014 at 12:20 PM, Etsuro Fujita <
> fujita.ets...@lab.ntt.co.jp> wrote:
>
>> Hi Ashutosh,
>>
>> Thanks for the review!
>>
>> (201

Re: [HACKERS] inherit support for foreign tables

[Etsuro Fujita]

(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.

Thanks,

Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 62,68  CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'csv', null '
  CREATE FOREIGN TABLE tbl () SERVER file_server;  -- ERROR
  
  CREATE FOREIGN TABLE agg_text (
! 	a	int2,
  	b	float4
  ) SERVER file_server
  OPTIONS (format 'text', filename '@abs_srcdir@/data/agg.data', delimiter '	', null '\N');
--- 62,68 
  CREATE FOREIGN TABLE tbl () SERVER file_server;  -- ERROR
  
  CREATE FOREIGN TABLE agg_text (
! 	a	int2 CHECK (a >= 0),
  	b	float4
  ) SERVER file_server
  OPTIONS (format 'text', filename '@abs_srcdir@/data/agg.data', delimiter '	', null '\N');
***
*** 72,82  CREATE FOREIGN TABLE agg_csv (
--- 72,84 
  	b	float4
  ) SERVER file_server
  OPTIONS (format 'csv', filename '@abs_srcdir@/data/agg.csv', header 'true', delimiter ';', quote '@', escape '"', null '');
+ ALTER FOREIGN TABLE agg_csv ADD CHECK (a >= 0);
  CREATE FOREIGN TABLE agg_bad (
  	a	int2,
  	b	float4
  ) SERVER file_server
  OPTIONS (format 'csv', filename '@abs_srcdir@/data/agg.bad', header 'true', delimiter ';', quote '@', escape '"', null '');
+ ALTER FOREIGN TABLE agg_bad ADD CHECK (a >= 0);
  
  -- per-column options tests
  CREATE FOREIGN TABLE text_csv (
***
*** 134,139  DELETE FROM agg_csv WHERE a = 100;
--- 136,153 
  -- but this should be ignored
  SELECT * FROM agg_csv FOR UPDATE;
  
+ -- constraint exclusion tests
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0;
+ \t off
+ SELECT * FROM agg_csv WHERE a < 0;
+ SET constraint_exclusion = 'on';
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0;
+ \t off
+ SELECT * FROM agg_csv WHERE a < 0;
+ SET constraint_exclusion = 'partition';
+ 
  -- 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
***
*** 78,84  ERROR:  COPY null representation cannot use newline or carriage return
  CREATE FOREIGN TABLE tbl () SERVER file_server;  -- ERROR
  ERROR:  filename is required for file_fdw foreign tables
  CREATE FOREIGN TABLE agg_text (
! 	a	int2,
  	b	float4
  )

Re: [HACKERS] inherit support for foreign tables

[Ashutosh Bapat]

On Thu, Nov 13, 2014 at 12:20 PM, Etsuro Fujita  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

[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

[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 
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

[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 
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 subscriptio

Re: [HACKERS] inherit support for foreign tables

[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 
  (

Re: [HACKERS] inherit support for foreign tables

[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

[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

[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

[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)
  -- ===

Re: [HACKERS] inherit support for foreign tables

[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

[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

[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);
*** a/doc/sr

Re: [HACKERS] inherit support for foreign tables

[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

[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

[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

[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

[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

[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

[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

[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,16

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

[Etsuro Fujita]

(2014/08/27 11:06), Tom Lane wrote:
> Etsuro Fujita  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: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Tom Lane]

Etsuro Fujita  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)

[Etsuro Fujita]

(2014/08/27 3:27), Tom Lane wrote:
> Etsuro Fujita  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)

[Tom Lane]

Etsuro Fujita  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)

[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 :
> Hi Ashutish,
>
>
> (2014/08/14 22:30), Ashutosh Bapat wrote:
>>
>> On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
>> 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: [HACKERS] inherit support for foreign tables

[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)

[Ashutosh Bapat]

On Thu, Aug 21, 2014 at 3:00 PM, Etsuro Fujita 
wrote:

> (2014/08/21 13:21), Ashutosh Bapat wrote:
>
>> On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita
>> 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
>> > 
>> >
>> >> 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: [HACKERS] inherit support for foreign tables

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

[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

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

[Etsuro Fujita]

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

On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita
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
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: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Ashutosh Bapat]

On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita 
wrote:

> Hi Ashutish,
>
>
> (2014/08/14 22:30), Ashutosh Bapat wrote:
>
>> On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
>> 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: [HACKERS] inherit support for foreign tables

[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->Reparam

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

[Etsuro Fujita]

Hi Ashutish,

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

On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
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: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)

[Ashutosh Bapat]

Hi,



On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita  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)

[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 changi

Re: [HACKERS] inherit support for foreign tables

[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(cons

Re: [HACKERS] inherit support for foreign tables

[Etsuro Fujita]

(2014/07/01 11:10), Etsuro Fujita wrote:
> (2014/06/30 22:48), Tom Lane wrote:
>> Etsuro Fujita  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_i

Re: [HACKERS] inherit support for foreign tables

[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

[Etsuro Fujita]

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

2014-06-24 16:30 GMT+09:00 Etsuro Fujita :

(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

[Shigeru Hanada]

Hi Fujita-san,

Sorry for leaving this thread for long time.

2014-06-24 16:30 GMT+09:00 Etsuro Fujita :
> (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

[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 

Re: [HACKERS] inherit support for foreign tables

[Kyotaro HORIGUCHI]

Hi, 

At Tue, 01 Jul 2014 16:30:41 +0900, Etsuro Fujita  
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

[Etsuro Fujita]

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

On Tue, Jul 1, 2014 at 12:25 PM, Etsuro Fujita
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

[Ashutosh Bapat]

On Tue, Jul 1, 2014 at 12:25 PM, Etsuro Fujita 
wrote:

> (2014/07/01 15:13), Ashutosh Bapat wrote:
>
>> On Tue, Jul 1, 2014 at 7:39 AM, Etsuro Fujita
>> 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

[Etsuro Fujita]

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

On Tue, Jul 1, 2014 at 7:39 AM, Etsuro Fujita
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

[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

[Ashutosh Bapat]

On Tue, Jul 1, 2014 at 7:39 AM, Etsuro Fujita 
wrote:

> (2014/06/30 20:17), Ashutosh Bapat wrote:
>
>> On Mon, Jun 30, 2014 at 4:17 PM, Etsuro Fujita
>> 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