Re: [HACKERS] inherit support for foreign tables
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
Andres Freund and...@anarazel.de writes: FWIW, I think we're getting pretty close to the point, or are there even, where we can remove default_with_oids. So not adding complications because of it sounds good to me. Well, pg_dump uses it --- so the argument about not breaking old dump scripts would apply to any attempt to remove it entirely. But I don't have a problem with saying that its semantics are frozen. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: On 2015/04/16 16:05, Etsuro Fujita wrote: I agree with you on this point. However, ISTM there is a bug in handling OIDs on foreign tables; while we now allow for ALTER SET WITH/WITHOUT OIDS, we still don't allow the default_with_oids parameter for foreign tables. I think that since CREATE FOREIGN TABLE should be consistent with ALTER FOREIGN TABLE, we should also allow the parameter for foreign tables. Attached is a patch for that. I also updated docs. Attached is an updated version of the patch. I believe that we intentionally did not do this, and here is why not: existing pg_dump files assume that default_with_oids doesn't affect any relation type except plain tables. pg_backup_archiver.c only bothers to change the GUC when about to dump a plain table, and otherwise leaves it at its previous value. That means if we apply a patch like this, it's entirely possible that pg_dump/pg_restore will result in foreign tables accidentally acquiring OID columns. Since default_with_oids is really only meant as a backwards-compatibility hack, I don't personally have a problem with restricting it to act only on plain tables. However, it might be a good idea to explicitly document this interaction in a code comment to prevent anyone from re-inventing this idea... I'll go do that. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
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
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
Hello, At Thu, 16 Apr 2015 14:43:33 -0700, David Fetter da...@fetter.org wrote in 20150416214333.ga...@fetter.org On Wed, Apr 15, 2015 at 09:35:05AM +0900, Kyotaro HORIGUCHI wrote: Hi, Before suppressing the symptom, I doubt the necessity and/or validity of giving foreign tables an ability to be a parent. Is there any reasonable usage for the ability? ... I have a use case for having foreign tables as non-leaf nodes in a partitioning hierarchy, namely geographic. Ah, I see. I understood the case of intermediate nodes. I agree that it is quite natural. One might have a table at HQ called foo_world, then partitions under it called foo_jp, foo_us, etc., in one level, foo_us_ca, foo_us_pa, etc. in the next level, and on down, each in general in a separate data center. Is there something essential about having non-leaf nodes as foreign tables that's a problem here? No. I'm convinced of the necessity. Sorry for the noise. At Wed, 22 Apr 2015 17:00:10 -0400, Robert Haas robertmh...@gmail.com wrote in CA+TgmobZVHp3D9wWCV8QJc+qGDu7=tekncbxowijzkhjucm...@mail.gmail.com Gee, I don't see why that would be unreasonable or invalid Hmm. Yes, as mentioned above, there's no reason to refuse non-leaf foregin tables. I didn't understood the real cause of the problem and thought that not allowing foreign *root* tables seem better than tweaking elsewhere. But that thought found to be totally a garbage :( regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On Tue, Apr 14, 2015 at 8:35 PM, Kyotaro HORIGUCHI horiguchi.kyot...@lab.ntt.co.jp wrote: Before suppressing the symptom, I doubt the necessity and/or validity of giving foreign tables an ability to be a parent. Is there any reasonable usage for the ability? Gee, I don't see why that would be unreasonable or invalid -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On 2015/04/16 16:05, Etsuro Fujita wrote: On 2015/03/23 2:57, Tom Lane wrote: Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: [ fdw-inh-8.patch ] I've committed this with some substantial rearrangements, notably: * As I mentioned earlier, I got rid of a few unnecessary restrictions on foreign tables so as to avoid introducing warts into inheritance behavior. In particular, we now allow NOT VALID CHECK constraints (and hence ALTER ... VALIDATE CONSTRAINT), ALTER SET STORAGE, and ALTER SET WITH/WITHOUT OIDS. These are probably no-ops anyway for foreign tables, though conceivably an FDW might choose to implement some behavior for STORAGE or OIDs. I agree with you on this point. However, ISTM there is a bug in handling OIDs on foreign tables; while we now allow for ALTER SET WITH/WITHOUT OIDS, we still don't allow the default_with_oids parameter for foreign tables. I think that since CREATE FOREIGN TABLE should be consistent with ALTER FOREIGN TABLE, we should also allow the parameter for foreign tables. Attached is a patch for that. I also updated docs. Attached is an updated version of the patch. Best regards, Etsuro Fujita *** a/doc/src/sgml/config.sgml --- b/doc/src/sgml/config.sgml *** *** 6745,6752 dynamic_library_path = 'C:\tools\postgresql;H:\my_project\lib;$libdir' /term listitem para ! This controls whether commandCREATE TABLE/command and ! commandCREATE TABLE AS/command include an OID column in newly-created tables, if neither literalWITH OIDS/literal nor literalWITHOUT OIDS/literal is specified. It also determines whether OIDs will be included in tables created by --- 6745,6753 /term listitem para ! This controls whether commandCREATE TABLE/command, ! commandCREATE TABLE AS/command and ! commandCREATE FOREIGN TABLE/command include an OID column in newly-created tables, if neither literalWITH OIDS/literal nor literalWITHOUT OIDS/literal is specified. It also determines whether OIDs will be included in tables created by *** a/doc/src/sgml/ref/create_foreign_table.sgml --- b/doc/src/sgml/ref/create_foreign_table.sgml *** *** 293,298 CHECK ( replaceable class=PARAMETERexpression/replaceable ) --- 293,304 responsibility to ensure that the constraint definition matches reality. /para + + para +To add OIDs to the table created by commandCREATE FOREIGN TABLE/command, +enable the xref linkend=guc-default-with-oids configuration +variable. + /para /refsect1 refsect1 id=SQL-CREATEFOREIGNTABLE-examples *** a/src/backend/commands/tablecmds.c --- b/src/backend/commands/tablecmds.c *** *** 580,586 DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId, descriptor = BuildDescForRelation(schema); localHasOids = interpretOidsOption(stmt-options, ! (relkind == RELKIND_RELATION)); descriptor-tdhasoid = (localHasOids || parentOidCount 0); /* --- 580,587 descriptor = BuildDescForRelation(schema); localHasOids = interpretOidsOption(stmt-options, ! (relkind == RELKIND_RELATION || ! relkind == RELKIND_FOREIGN_TABLE)); descriptor-tdhasoid = (localHasOids || parentOidCount 0); /* -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On 2015/03/23 2:57, Tom Lane wrote: Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: [ fdw-inh-8.patch ] I've committed this with some substantial rearrangements, notably: * As I mentioned earlier, I got rid of a few unnecessary restrictions on foreign tables so as to avoid introducing warts into inheritance behavior. In particular, we now allow NOT VALID CHECK constraints (and hence ALTER ... VALIDATE CONSTRAINT), ALTER SET STORAGE, and ALTER SET WITH/WITHOUT OIDS. These are probably no-ops anyway for foreign tables, though conceivably an FDW might choose to implement some behavior for STORAGE or OIDs. I agree with you on this point. However, ISTM there is a bug in handling OIDs on foreign tables; while we now allow for ALTER SET WITH/WITHOUT OIDS, we still don't allow the default_with_oids parameter for foreign tables. I think that since CREATE FOREIGN TABLE should be consistent with ALTER FOREIGN TABLE, we should also allow the parameter for foreign tables. Attached is a patch for that. Best regards, Etsuro Fujita *** a/src/backend/commands/tablecmds.c --- b/src/backend/commands/tablecmds.c *** *** 580,586 DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId, descriptor = BuildDescForRelation(schema); localHasOids = interpretOidsOption(stmt-options, ! (relkind == RELKIND_RELATION)); descriptor-tdhasoid = (localHasOids || parentOidCount 0); /* --- 580,587 descriptor = BuildDescForRelation(schema); localHasOids = interpretOidsOption(stmt-options, ! (relkind == RELKIND_RELATION || ! relkind == RELKIND_FOREIGN_TABLE)); descriptor-tdhasoid = (localHasOids || parentOidCount 0); /* -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
Hello, At Thu, 16 Apr 2015 12:20:47 +0900, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote in 552f2a8f.2090...@lab.ntt.co.jp On 2015/04/15 3:52, Alvaro Herrera wrote: On 4/14/15 5:49 AM, Etsuro Fujita wrote: postgres=# create foreign table ft1 (c1 int) server myserver options (table_name 't1'); CREATE FOREIGN TABLE postgres=# create foreign table ft2 (c1 int) server myserver options (table_name 't2'); CREATE FOREIGN TABLE postgres=# alter foreign table ft2 inherit ft1; ALTER FOREIGN TABLE postgres=# select * from ft1 for update; ERROR: could not find junk tableoid1 column I think this is a bug. Attached is a patch fixing this issue. I think the real question is whether we're now (I mean after this patch) emitting useless tableoid columns that we didn't previously have. I think the answer is yes, and if so I think we need a smaller comb to fix the problem. This one seems rather large. My answer for that would be *no* because I think tableoid would be needed for EvalPlanQual checking in more complex SELECT FOR UPDATE on the inheritance or UPDATE/DELETE involving the inheritance as a source table. Also, if we allow the FDW to change the behavior of SELECT FOR UPDATE so as to match the local semantics exactly, which I'm working on in another thread, I think tableoid would also be needed for the actual row locking. Given the parent foreign talbes, surely they need tableoids for such usage. The patch preserves the condition rc-isParent so it newly affects exactly only parent foreign tables for now. Before the parent foreign tables introduced, ROW_MARK_COPY and RTE_RELATION are mutually exclusive so didn't need, or cannot have tableoid. But now it intorduces an rte with ROW_MARK_COPY RTE_RELATION and there seems no reason for parent tables in any kind not to have tableoid. After such consideration, I came to think that the patch is a reasonable fix, not mere a workaround. Thoughts? regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On Wed, Apr 15, 2015 at 09:35:05AM +0900, Kyotaro HORIGUCHI wrote: Hi, Before suppressing the symptom, I doubt the necessity and/or validity of giving foreign tables an ability to be a parent. Is there any reasonable usage for the ability? I think we should choose to inhibit foreign tables from becoming a parent rather than leaving it allowed then taking measures for the consequent symptom. I have a use case for having foreign tables as non-leaf nodes in a partitioning hierarchy, namely geographic. One might have a table at HQ called foo_world, then partitions under it called foo_jp, foo_us, etc., in one level, foo_us_ca, foo_us_pa, etc. in the next level, and on down, each in general in a separate data center. Is there something essential about having non-leaf nodes as foreign tables that's a problem here? Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
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
On 2015/03/23 2:57, Tom Lane wrote: Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: [ fdw-inh-8.patch ] I've committed this with some substantial rearrangements, notably: * I thought that if we were doing this at all, we should go all the way and allow foreign tables to be both inheritance parents and children. I found that when setting a foreign table to be the parent of an inheritance set that only contains foreign tables, SELECT FOR UPDATE on the inheritance parent fails with a can't-happen error condition. Here is an example: $ createdb mydb $ psql mydb psql (9.5devel) Type help for help. mydb=# create table t1 (c1 int); CREATE TABLE mydb=# create table t2 (c1 int); CREATE TABLE $ psql postgres psql (9.5devel) Type help for help. postgres=# create extension postgres_fdw; CREATE EXTENSION postgres=# create server myserver foreign data wrapper postgres_fdw options (dbname 'mydb'); CREATE SERVER postgres=# create user mapping for current_user server myserver; CREATE USER MAPPING postgres=# create foreign table ft1 (c1 int) server myserver options (table_name 't1'); CREATE FOREIGN TABLE postgres=# create foreign table ft2 (c1 int) server myserver options (table_name 't2'); CREATE FOREIGN TABLE postgres=# alter foreign table ft2 inherit ft1; ALTER FOREIGN TABLE postgres=# select * from ft1 for update; ERROR: could not find junk tableoid1 column I think this is a bug. Attached is a patch fixing this issue. Best regards, Etsuro Fujita *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 3193,3218 select * from bar where f1 in (select f1 from foo) for update; QUERY PLAN -- LockRows !Output: bar.f1, bar.f2, bar.ctid, bar.tableoid, bar.*, foo.ctid, foo.tableoid, foo.* - Hash Join ! Output: bar.f1, bar.f2, bar.ctid, bar.tableoid, bar.*, foo.ctid, foo.tableoid, foo.* Hash Cond: (bar.f1 = foo.f1) - Append - Seq Scan on public.bar ! Output: bar.f1, bar.f2, bar.ctid, bar.tableoid, bar.* - Foreign Scan on public.bar2 ! Output: bar2.f1, bar2.f2, bar2.ctid, bar2.tableoid, bar2.* Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE - Hash !Output: foo.ctid, foo.tableoid, foo.*, foo.f1 - HashAggregate ! Output: foo.ctid, foo.tableoid, foo.*, foo.f1 Group Key: foo.f1 - Append - Seq Scan on public.foo ! Output: foo.ctid, foo.tableoid, foo.*, foo.f1 - Foreign Scan on public.foo2 ! Output: foo2.ctid, foo2.tableoid, foo2.*, foo2.f1 Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct1 (22 rows) --- 3193,3218 QUERY PLAN -- LockRows !Output: bar.f1, bar.f2, bar.ctid, bar.*, bar.tableoid, foo.ctid, foo.*, foo.tableoid - Hash Join ! Output: bar.f1, bar.f2, bar.ctid, bar.*, bar.tableoid, foo.ctid, foo.*, foo.tableoid Hash Cond: (bar.f1 = foo.f1) - Append - Seq Scan on public.bar ! Output: bar.f1, bar.f2, bar.ctid, bar.*, bar.tableoid - Foreign Scan on public.bar2 ! Output: bar2.f1, bar2.f2, bar2.ctid, bar2.*, bar2.tableoid Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE - Hash !Output: foo.ctid, foo.*, foo.tableoid, foo.f1 - HashAggregate ! Output: foo.ctid, foo.*, foo.tableoid, foo.f1 Group Key: foo.f1 - Append - Seq Scan on public.foo ! Output: foo.ctid, foo.*, foo.tableoid, foo.f1 - Foreign Scan on public.foo2 ! Output: foo2.ctid, foo2.*, foo2.tableoid, foo2.f1 Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct1 (22 rows) *** *** 3230,3255 select * from bar where f1 in (select f1 from foo) for share; QUERY PLAN -- LockRows !Output: bar.f1, bar.f2, bar.ctid, bar.tableoid, bar.*, foo.ctid, foo.tableoid, foo.*
Re: [HACKERS] inherit support for foreign tables
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
Hi, Before suppressing the symptom, I doubt the necessity and/or validity of giving foreign tables an ability to be a parent. Is there any reasonable usage for the ability? I think we should choose to inhibit foreign tables from becoming a parent rather than leaving it allowed then taking measures for the consequent symptom. regards, At Tue, 14 Apr 2015 15:52:18 -0300, Alvaro Herrera alvhe...@2ndquadrant.com wrote in 20150414185218.gx4...@alvh.no-ip.org Jim Nasby wrote: On 4/14/15 5:49 AM, Etsuro Fujita wrote: postgres=# create foreign table ft1 (c1 int) server myserver options (table_name 't1'); CREATE FOREIGN TABLE postgres=# create foreign table ft2 (c1 int) server myserver options (table_name 't2'); CREATE FOREIGN TABLE postgres=# alter foreign table ft2 inherit ft1; ALTER FOREIGN TABLE postgres=# select * from ft1 for update; ERROR: could not find junk tableoid1 column I think this is a bug. Attached is a patch fixing this issue. What happens when the foreign side breaks the inheritance? Does the FDW somehow know to check that fact for each query? This is a meaningless question. The remote tables don't have to have an inheritance relationship already; only the local side sees them as connected. I think the real question is whether we're now (I mean after this patch) emitting useless tableoid columns that we didn't previously have. I think the answer is yes, and if so I think we need a smaller comb to fix the problem. This one seems rather large. -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
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
On Mon, Mar 23, 2015 at 12:09 AM, Robert Haas robertmh...@gmail.com wrote: On Sun, Mar 22, 2015 at 1:57 PM, Tom Lane t...@sss.pgh.pa.us wrote: Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: [ fdw-inh-8.patch ] I've committed this with some substantial rearrangements, notably: I'm really glad this is going in! Thanks to to Shigeru Hanada and Etsuro Fujita for working on this, to you (Tom) for putting in the time to get it committed, and of course to the reviewers Ashutosh Bapat and Kyotaro Horiguchi for their time and effort. In a way, I believe we can think of this as the beginnings of a sharding story for PostgreSQL. A lot more work is needed, of course -- join and aggregate pushdown are high on my personal list -- but it's a start. +1. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] inherit support for foreign tables
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
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: [ fdw-inh-8.patch ] I've committed this with some substantial rearrangements, notably: * I thought that if we were doing this at all, we should go all the way and allow foreign tables to be both inheritance parents and children. * As I mentioned earlier, I got rid of a few unnecessary restrictions on foreign tables so as to avoid introducing warts into inheritance behavior. In particular, we now allow NOT VALID CHECK constraints (and hence ALTER ... VALIDATE CONSTRAINT), ALTER SET STORAGE, and ALTER SET WITH/WITHOUT OIDS. These are probably no-ops anyway for foreign tables, though conceivably an FDW might choose to implement some behavior for STORAGE or OIDs. * I did not like the EXPLAIN changes at all; in the first place they resulted in invalid JSON output (there could be multiple fields of the Update plan object with identical labels), and in the second place it seemed like a bad idea to rely on FDWs to change the behavior of their ExplainModifyTarget functions. I've refactored that so that explain.c remains responsible for getting the grouping right. Also, as I said earlier, it seemed like a good idea to produce subgroups identifying all the target tables not only the foreign ones. * I fooled around with the PlanRowMark changes some more, mainly with the idea that we might soon allow FDWs to use rowmark methods other than ROW_MARK_COPY. The planner now has just one place where a rel's rowmark method is chosen, so as to centralize anything we need to do there. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On Sun, Mar 22, 2015 at 1:57 PM, Tom Lane t...@sss.pgh.pa.us wrote: Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: [ fdw-inh-8.patch ] I've committed this with some substantial rearrangements, notably: I'm really glad this is going in! Thanks to to Shigeru Hanada and Etsuro Fujita for working on this, to you (Tom) for putting in the time to get it committed, and of course to the reviewers Ashutosh Bapat and Kyotaro Horiguchi for their time and effort. In a way, I believe we can think of this as the beginnings of a sharding story for PostgreSQL. A lot more work is needed, of course -- join and aggregate pushdown are high on my personal list -- but it's a start. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: I noticed that the latter disallows TRUNCATE on inheritance trees that contain at least one child foreign table. But I think it would be better to allow it, with the semantics that we quietly ignore the child foreign tables and apply the operation to the child plain tables, which is the same semantics as ALTER COLUMN SET STORAGE on such inheritance trees. Comments welcome. I've been working through the foreign table inheritance patch, and found the code that makes the above happen. I don't think this is a good idea at all. In the first place, successful TRUNCATE should leave the table empty, not well, we'll make it empty if we feel up to that. In the second place, someday we might want to make TRUNCATE actually work for foreign tables (at least for FDWs that want to support it). If we did, we would have a backwards-compatibility hazard, because suddenly a TRUNCATE on an inheritance tree that includes a foreign table would have different non-error effects than before. I think we should just throw error in this case. BTW, the SET STORAGE comparison is bogus as well. I see no reason that we shouldn't just allow SET STORAGE on foreign tables. It's probably not going to have any effect, but so what? And again, if we did ever find a use for that, we'd have a compatibility problem if inherited SET STORAGE has a pre-existing behavior that it skips foreign children. In the same vein, I'm planning to take out the existing prohibition on marking CHECK constraints on foreign tables NOT VALID. That likewise creates a corner case for inheritance trees for no obviously good reason. It was reasonable to be conservative about whether to allow that so long as there were no side-effects; but putting warts into the behavior of inheritance trees to preserve the prohibition is not a good outcome. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On 2015/01/15 16:35, Etsuro Fujita wrote:
On 2014/12/23 0:36, Tom Lane wrote:
Yeah, we need to do something about the PlanRowMark data structure.
Aside from the pre-existing issue in postgres_fdw, we need to fix it
to support inheritance trees in which more than one rowmark method
is being used. That rte.hasForeignChildren thing is a crock,
The idea I'd had about that was to convert the markType field into a
bitmask, so that a parent node's markType could represent the logical
OR of the rowmark methods being used by all its children.
As I said before, that seems to me like a good idea. So I'll update the
patch based on that if you're okey with it.
Done based on your ideas: (a) add a field to PlanRowMark to record the
original lock strength to fix the postgres_fdw issue and (b) convert its
markType field into a bitmask to support the inheritance trees. I think
that both work well and that (a) is useful for the other places.
Patch attached, which has been created on top of [1].
Best regards,
Etsuro Fujita
[1] http://www.postgresql.org/message-id/54bcbbf8.3020...@lab.ntt.co.jp
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 148,153 EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a 0;
--- 148,167
SELECT * FROM agg_csv WHERE a 0;
RESET constraint_exclusion;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+ SELECT tableoid::regclass, * FROM agg_csv;
+ SELECT tableoid::regclass, * FROM ONLY agg;
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ DELETE FROM agg WHERE a = 100;
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
+
-- privilege tests
SET ROLE file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 249,254 SELECT * FROM agg_csv WHERE a 0;
--- 249,294
(0 rows)
RESET constraint_exclusion;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ SELECT tableoid::regclass, * FROM agg_csv;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ SELECT tableoid::regclass, * FROM ONLY agg;
+ tableoid | a | b
+ --+---+---
+ (0 rows)
+
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ ERROR: cannot update foreign table agg_csv
+ DELETE FROM agg WHERE a = 100;
+ ERROR: cannot delete from foreign table agg_csv
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
-- privilege tests
SET ROLE file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 3027,3032 NOTICE: NEW: (13,test triggered !)
--- 3027,3544
(1 row)
-- ===
+ -- test inheritance features
+ -- ===
+ CREATE TABLE a (aa TEXT);
+ CREATE TABLE loct (aa TEXT, bb TEXT);
+ CREATE FOREIGN TABLE b (bb TEXT) INHERITS (a)
+ SERVER loopback OPTIONS (table_name 'loct');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO a(aa) VALUES('a');
+ INSERT INTO a(aa) VALUES('aa');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('b');
+ INSERT INTO b(aa) VALUES('bb');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ SELECT relname, a.* FROM a, pg_class where a.tableoid = pg_class.oid;
+ relname |aa
+ -+--
+ a | aaa
+ a |
+ a | a
+ a | aa
+ a | aaa
+ a |
+ b | bbb
+ b |
+ b | b
+ b | bb
+ b | bbb
+ b |
+ (12 rows)
+
+ SELECT relname, b.* FROM b, pg_class where b.tableoid = pg_class.oid;
+ relname |aa| bb
+ -+--+
+ b | bbb |
+ b | |
+ b | b|
+ b | bb |
+ b | bbb |
+ b |
Re: [HACKERS] inherit support for foreign tables
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
On 2014/12/23 0:36, Tom Lane wrote:
Yeah, we need to do something about the PlanRowMark data structure.
Aside from the pre-existing issue in postgres_fdw, we need to fix it
to support inheritance trees in which more than one rowmark method
is being used. That rte.hasForeignChildren thing is a crock, and
would still be a crock even if it were correctly documented as being
a planner temporary variable (rather than the current implication that
it's always valid). RangeTblEntry is no place for planner temporaries.
Agreed.
The idea I'd had about that was to convert the markType field into a
bitmask, so that a parent node's markType could represent the logical
OR of the rowmark methods being used by all its children. I've not
attempted to code this up though, and probably won't get to it until
after Christmas. One thing that's not clear is what should happen
with ExecRowMark.
That seems like a good idea, as parent PlanRowMarks are ignored at runtime.
Aside from the above, I noticed that the patch has a bug in handling
ExecRowMarks/ExecAuxRowMarks for foreign tables in inheritance trees
during the EPQ processing.:-( Attached is an updated version of the
patch to fix that, which has been created on top of [1], as said before.
Thanks,
[1] http://www.postgresql.org/message-id/5497bf4c.6080...@lab.ntt.co.jp
Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 148,153 EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a 0;
--- 148,167
SELECT * FROM agg_csv WHERE a 0;
RESET constraint_exclusion;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+ SELECT tableoid::regclass, * FROM agg_csv;
+ SELECT tableoid::regclass, * FROM ONLY agg;
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ DELETE FROM agg WHERE a = 100;
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
+
-- privilege tests
SET ROLE file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 249,254 SELECT * FROM agg_csv WHERE a 0;
--- 249,294
(0 rows)
RESET constraint_exclusion;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ SELECT tableoid::regclass, * FROM agg_csv;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ SELECT tableoid::regclass, * FROM ONLY agg;
+ tableoid | a | b
+ --+---+---
+ (0 rows)
+
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ ERROR: cannot update foreign table agg_csv
+ DELETE FROM agg WHERE a = 100;
+ ERROR: cannot delete from foreign table agg_csv
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
-- privilege tests
SET ROLE file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 3027,3032 NOTICE: NEW: (13,test triggered !)
--- 3027,3544
(1 row)
-- ===
+ -- test inheritance features
+ -- ===
+ CREATE TABLE a (aa TEXT);
+ CREATE TABLE loct (aa TEXT, bb TEXT);
+ CREATE FOREIGN TABLE b (bb TEXT) INHERITS (a)
+ SERVER loopback OPTIONS (table_name 'loct');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO a(aa) VALUES('a');
+ INSERT INTO a(aa) VALUES('aa');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('b');
+ INSERT INTO b(aa) VALUES('bb');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ SELECT relname, a.* FROM a, pg_class where a.tableoid = pg_class.oid;
+ relname |aa
+ -+--
+ a | aaa
+ a |
+ a | a
+ a | aa
+ a | aaa
+ a |
+ b | bbb
+ b |
+ b | b
+ b | bb
+ b | bbb
+ b |
+ (12 rows)
+
+
Re: [HACKERS] inherit support for foreign tables
On 2014/12/18 7:04, Tom Lane wrote:
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
Attached are updated patches. Patch fdw-inh-5.patch has been created on
top of patch fdw-chk-5.patch.
I've committed fdw-chk-5.patch with some minor further adjustments;
Have not looked at the other patch yet.
I updated the remaining patch correspondingly to the fix [1]. Please
find attached a patch (the patch has been created on top of the patch in
[1]).
I haven't done anything about the issue that postgresGetForeignPlan() is
using get_parse_rowmark(), discussed in eg, [2]. Tom, will you work on
that?
Thanks,
[1] http://www.postgresql.org/message-id/5497bf4c.6080...@lab.ntt.co.jp
[2] http://www.postgresql.org/message-id/18256.1418401...@sss.pgh.pa.us
Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 148,153 EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a 0;
--- 148,167
SELECT * FROM agg_csv WHERE a 0;
RESET constraint_exclusion;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+ SELECT tableoid::regclass, * FROM agg_csv;
+ SELECT tableoid::regclass, * FROM ONLY agg;
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ DELETE FROM agg WHERE a = 100;
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
+
-- privilege tests
SET ROLE file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 249,254 SELECT * FROM agg_csv WHERE a 0;
--- 249,294
(0 rows)
RESET constraint_exclusion;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ SELECT tableoid::regclass, * FROM agg_csv;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ SELECT tableoid::regclass, * FROM ONLY agg;
+ tableoid | a | b
+ --+---+---
+ (0 rows)
+
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ ERROR: cannot update foreign table agg_csv
+ DELETE FROM agg WHERE a = 100;
+ ERROR: cannot delete from foreign table agg_csv
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
-- privilege tests
SET ROLE file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 3027,3032 NOTICE: NEW: (13,test triggered !)
--- 3027,3544
(1 row)
-- ===
+ -- test inheritance features
+ -- ===
+ CREATE TABLE a (aa TEXT);
+ CREATE TABLE loct (aa TEXT, bb TEXT);
+ CREATE FOREIGN TABLE b (bb TEXT) INHERITS (a)
+ SERVER loopback OPTIONS (table_name 'loct');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO a(aa) VALUES('a');
+ INSERT INTO a(aa) VALUES('aa');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('b');
+ INSERT INTO b(aa) VALUES('bb');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ SELECT relname, a.* FROM a, pg_class where a.tableoid = pg_class.oid;
+ relname |aa
+ -+--
+ a | aaa
+ a |
+ a | a
+ a | aa
+ a | aaa
+ a |
+ b | bbb
+ b |
+ b | b
+ b | bb
+ b | bbb
+ b |
+ (12 rows)
+
+ SELECT relname, b.* FROM b, pg_class where b.tableoid = pg_class.oid;
+ relname |aa| bb
+ -+--+
+ b | bbb |
+ b | |
+ b | b|
+ b | bb |
+ b | bbb |
+ b | |
+ (6 rows)
+
+ SELECT relname, a.* FROM ONLY a, pg_class where a.tableoid = pg_class.oid;
+ relname |aa
+ -+--
+ a | aaa
+ a |
+ a | a
+ a | aa
+ a | aaa
+ a |
+ (6 rows)
+
+ UPDATE a SET aa='zz' WHERE aa LIKE
Re: [HACKERS] inherit support for foreign tables
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: I haven't done anything about the issue that postgresGetForeignPlan() is using get_parse_rowmark(), discussed in eg, [2]. Tom, will you work on that? Yeah, we need to do something about the PlanRowMark data structure. Aside from the pre-existing issue in postgres_fdw, we need to fix it to support inheritance trees in which more than one rowmark method is being used. That rte.hasForeignChildren thing is a crock, and would still be a crock even if it were correctly documented as being a planner temporary variable (rather than the current implication that it's always valid). RangeTblEntry is no place for planner temporaries. The idea I'd had about that was to convert the markType field into a bitmask, so that a parent node's markType could represent the logical OR of the rowmark methods being used by all its children. I've not attempted to code this up though, and probably won't get to it until after Christmas. One thing that's not clear is what should happen with ExecRowMark. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: Attached are updated patches. Patch fdw-inh-5.patch has been created on top of patch fdw-chk-5.patch. Patch fdw-chk-5.patch is basically the same as the previous one fdw-chk-4.patch, but I slightly modified that one. The changes are the following. * added to foreign_data.sql more tests for your comments. * revised docs on ALTER FOREIGN TABLE a bit further. I've committed fdw-chk-5.patch with some minor further adjustments; the most notable one was that I got rid of the error check prohibiting NO INHERIT, which did not seem to me to have any value. Attaching such a clause won't have any effect, but so what? Have not looked at the other patch yet. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/12/18 7:04), Tom Lane wrote: Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: Attached are updated patches. Patch fdw-inh-5.patch has been created on top of patch fdw-chk-5.patch. Patch fdw-chk-5.patch is basically the same as the previous one fdw-chk-4.patch, but I slightly modified that one. The changes are the following. * added to foreign_data.sql more tests for your comments. * revised docs on ALTER FOREIGN TABLE a bit further. I've committed fdw-chk-5.patch with some minor further adjustments; the most notable one was that I got rid of the error check prohibiting NO INHERIT, which did not seem to me to have any value. Attaching such a clause won't have any effect, but so what? Have not looked at the other patch yet. Thanks! I added the error check because the other patch, fdw-inh-5.patch, doesn't allow foreign tables to be inherited and so it seems more consistent at least to me to do so. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On Thu, Dec 11, 2014 at 2:54 PM, Ashutosh Bapat ashutosh.ba...@enterprisedb.com wrote: On Thu, Dec 11, 2014 at 8:39 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: Hi Ashutosh, Thanks for the review! (2014/12/10 14:47), Ashutosh Bapat wrote: We haven't heard anything from Horiguchi-san and Hanada-san for almost a week. So, I am fine marking it as ready for committer. What do you say? Moving this patch to CF 2014-12 with the same status. Let's get a committer having a look at it. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/12/11 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
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
I marked this as ready for committer. On Thu, Dec 11, 2014 at 8:39 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: Hi Ashutosh, Thanks for the review! (2014/12/10 14:47), Ashutosh Bapat wrote: We haven't heard anything from Horiguchi-san and Hanada-san for almost a week. So, I am fine marking it as ready for committer. What do you say? ISTM that both of them are not against us, so let's ask for the committer's review! Thanks, Best regards, Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] inherit support for foreign tables
Hi Ashutosh,
Thanks for the review!
(2014/11/28 18:14), Ashutosh Bapat wrote:
On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
(2014/11/17 17:55), Ashutosh Bapat wrote:
Here are my review comments for patch fdw-inh-3.patch.
Tests
---
1. It seems like you have copied from testcase inherit.sql to
postgres_fdw testcase. That's a good thing, but it makes the
test quite
long. May be we should have two tests in postgres_fdw contrib
module,
one for simple cases, and other for inheritance. What do you say?
IMO, the test is not so time-consuming, so it doesn't seem worth
splitting it into two.
I am not worried about the timing but I am worried about the length of
the file and hence ease of debugging in case we find any issues there.
We will leave that for the commiter to decide.
OK
Documentation
1. The change in ddl.sgml
-We will refer to the child tables as partitions, though
they
-are in every way normal productnamePostgreSQL/ tables.
+We will refer to the child tables as partitions, though
we assume
+that they are normal productnamePostgreSQL/ tables.
adds phrase we assume that, which confuses the intention
behind the
sentence. The original text is intended to highlight the equivalence
between partition and normal table, where as the addition
esp. the
word assume weakens that equivalence. Instead now we have to
highlight
the equivalence between partition and normal or foreign
table. The
wording similar to though they are either normal or foreign tables
should be used there.
You are right, but I feel that there is something out of place in
saying that there (5.10.2. Implementing Partitioning) because the
procedure there has been written based on normal tables only. Put
another way, if we say that, I think we'd need to add more docs,
describing the syntax and/or the corresponding examples for
foreign-table cases. But I'd like to leave that for another patch.
So, how about the wording we assume *here* that, instead of we
assume that, as I added the following notice in the previous
section (5.10.1. Overview)?
@@ -2650,7 +2669,10 @@ VALUES ('Albany', NULL, NULL, 'NY');
table of a single parent table. The parent table itself is
normally
empty; it exists just to represent the entire data set. You
should be
familiar with inheritance (see xref linkend=ddl-inherit)
before
-attempting to set up partitioning.
+attempting to set up partitioning. (The setup and management of
+partitioned tables illustrated in this section assume that each
+partition is a normal table. However, you can do that in a
similar way
+for cases where some or all partitions are foreign tables.)
This looks ok, though, I would like to see final version of the
document. But I think, we will leave that for committer to handle.
OK
2. The wording some kind of optimization gives vague picture.
May be
it can be worded as Since the constraints are assumed to be
true, they
are used in constraint-based query optimization like constraint
exclusion for partitioned tables..
+Those constraints are used in some kind of query
optimization such
+as constraint exclusion for partitioned tables (see
+xref linkend=ddl-partitioning).
Will follow your revision.
Done.
Code
---
1. In the following change
+/*
* acquire_inherited_sample_rows -- acquire sample rows from
inheritance tree
*
* This has the same API as acquire_sample_rows, except that
rows are
* collected from all inheritance children as well as the
specified table.
- * We fail and return zero if there are no inheritance children.
+ * We fail and return zero if there are no inheritance children or
there are
+ * inheritance children that foreign tables.
The addition should be there are inheritance children that *are all
*foreign tables. Note the addition are all.
Sorry, I incorrectly wrote the comment. What I tried to write is
We fail and return zero if there are no inheritance children or if
we are not in VAC_MODE_SINGLE case and inheritance tree contains at
least one foreign table..
You might want to use English description of VAC_MODE_SINGLE instead
of that macro in the comment, so that reader doesn't have to look up
VAC_MODE_SINGLE. But I think, we will leave this for the committer.
I corrected the comments and
Re: [HACKERS] inherit support for foreign tables
We haven't heard anything from Horiguchi-san and Hanada-san for almost a
week. So, I am fine marking it as ready for committer. What do you say?
On Wed, Dec 10, 2014 at 8:48 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:
Hi Ashutosh,
Thanks for the review!
(2014/11/28 18:14), Ashutosh Bapat wrote:
On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
(2014/11/17 17:55), Ashutosh Bapat wrote:
Here are my review comments for patch fdw-inh-3.patch.
Tests
---
1. It seems like you have copied from testcase inherit.sql to
postgres_fdw testcase. That's a good thing, but it makes the
test quite
long. May be we should have two tests in postgres_fdw contrib
module,
one for simple cases, and other for inheritance. What do you say?
IMO, the test is not so time-consuming, so it doesn't seem worth
splitting it into two.
I am not worried about the timing but I am worried about the length of
the file and hence ease of debugging in case we find any issues there.
We will leave that for the commiter to decide.
OK
Documentation
1. The change in ddl.sgml
-We will refer to the child tables as partitions, though
they
-are in every way normal productnamePostgreSQL/
tables.
+We will refer to the child tables as partitions, though
we assume
+that they are normal productnamePostgreSQL/ tables.
adds phrase we assume that, which confuses the intention
behind the
sentence. The original text is intended to highlight the
equivalence
between partition and normal table, where as the addition
esp. the
word assume weakens that equivalence. Instead now we have to
highlight
the equivalence between partition and normal or foreign
table. The
wording similar to though they are either normal or foreign
tables
should be used there.
You are right, but I feel that there is something out of place in
saying that there (5.10.2. Implementing Partitioning) because the
procedure there has been written based on normal tables only. Put
another way, if we say that, I think we'd need to add more docs,
describing the syntax and/or the corresponding examples for
foreign-table cases. But I'd like to leave that for another patch.
So, how about the wording we assume *here* that, instead of we
assume that, as I added the following notice in the previous
section (5.10.1. Overview)?
@@ -2650,7 +2669,10 @@ VALUES ('Albany', NULL, NULL, 'NY');
table of a single parent table. The parent table itself is
normally
empty; it exists just to represent the entire data set. You
should be
familiar with inheritance (see xref linkend=ddl-inherit)
before
-attempting to set up partitioning.
+attempting to set up partitioning. (The setup and management of
+partitioned tables illustrated in this section assume that each
+partition is a normal table. However, you can do that in a
similar way
+for cases where some or all partitions are foreign tables.)
This looks ok, though, I would like to see final version of the
document. But I think, we will leave that for committer to handle.
OK
2. The wording some kind of optimization gives vague picture.
May be
it can be worded as Since the constraints are assumed to be
true, they
are used in constraint-based query optimization like constraint
exclusion for partitioned tables..
+Those constraints are used in some kind of query
optimization such
+as constraint exclusion for partitioned tables (see
+xref linkend=ddl-partitioning).
Will follow your revision.
Done.
Code
---
1. In the following change
+/*
* acquire_inherited_sample_rows -- acquire sample rows from
inheritance tree
*
* This has the same API as acquire_sample_rows, except that
rows are
* collected from all inheritance children as well as the
specified table.
- * We fail and return zero if there are no inheritance children.
+ * We fail and return zero if there are no inheritance children
or
there are
+ * inheritance children that foreign tables.
The addition should be there are inheritance children that *are
all
*foreign tables. Note the addition are all.
Sorry, I incorrectly wrote the comment. What I tried to write is
We fail and return zero if there are no inheritance children or if
we are
Re: [HACKERS] inherit support for foreign tables
(2014/12/08 15:17), Ashutosh Bapat wrote: On Sat, Dec 6, 2014 at 9:21 PM, Noah Misch n...@leadboat.com mailto:n...@leadboat.com wrote: Does this inheritance patch add any atomicity problem that goes away when one breaks up the inheritance hierarchy and UPDATEs each table separately? If not, this limitation is okay. If the UPDATES crafted after breaking up the inheritance hierarchy are needed to be run within the same transaction (as the UPDATE on inheritance hierarchy would do), yes, there is atomicity problem. ISTM that your concern would basically a known problem. Consider the following transaction. BEGIN TRANSACTION; UPDATE foo SET a = 100; -- updates on table foo in remote server1 UPDATE bar SET a = 100; -- updates on table bar in remote server2 COMMIT TRANSACTION; This transaction would cause the atomicity problem if pgfdw_xact_callback() for XACT_EVENT_PRE_COMMIT for foo succeeded and then that for bar failed during CommitTransaction(). Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/12/07 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
On Sat, Dec 6, 2014 at 9:21 PM, Noah Misch n...@leadboat.com wrote: On Thu, Dec 04, 2014 at 10:00:14AM +0530, Ashutosh Bapat wrote: On Thu, Dec 4, 2014 at 9:05 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/12/03 19:35), Ashutosh Bapat wrote: On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: IIUC, even the transactions over the local and the *single* remote server are not guaranteed to be executed atomically in the current form. It is possible that the remote transaction succeeds and the local one fails, for example, resulting in data inconsistency between the local and the remote. IIUC, while committing transactions involving a single remote server, the steps taken are as follows 1. the local changes are brought to PRE-COMMIT stage, which means that the transaction *will* succeed locally after successful completion of this phase, 2. COMMIT message is sent to the foreign server 3. If step two succeeds, local changes are committed and successful commit is conveyed to the client 4. if step two fails, local changes are rolled back and abort status is conveyed to the client 5. If step 1 itself fails, the remote changes are rolled back. This is as per one phase commit protocol which guarantees ACID for single foreign data source. So, the changes involving local and a single foreign server seem to be atomic and consistent. Really? Maybe I'm missing something, but I don't think the current implementation for committing transactions has such a mechanism stated in step 1. So, I think it's possible that the local transaction fails in step3 while the remote transaction succeeds, as mentioned above. PFA a script attached which shows this. You may want to check the code in pgfdw_xact_callback() for actions taken by postgres_fdw on various events. CommitTransaction() for how those events are generated. The code there complies with the sequence above. While postgres_fdw delays remote commit to eliminate many causes for having one server commit while another aborts, other causes remain. The local transaction could still fail due to WAL I/O problems or a system crash. 2PC is the reliable answer, but that was explicitly out of scope for the initial postgres_fdw write support. Does this inheritance patch add any atomicity problem that goes away when one breaks up the inheritance hierarchy and UPDATEs each table separately? If not, this limitation is okay. If the UPDATES crafted after breaking up the inheritance hierarchy are needed to be run within the same transaction (as the UPDATE on inheritance hierarchy would do), yes, there is atomicity problem. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] inherit support for foreign tables
On Thu, Dec 04, 2014 at 10:00:14AM +0530, Ashutosh Bapat wrote: On Thu, Dec 4, 2014 at 9:05 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/12/03 19:35), Ashutosh Bapat wrote: On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: IIUC, even the transactions over the local and the *single* remote server are not guaranteed to be executed atomically in the current form. It is possible that the remote transaction succeeds and the local one fails, for example, resulting in data inconsistency between the local and the remote. IIUC, while committing transactions involving a single remote server, the steps taken are as follows 1. the local changes are brought to PRE-COMMIT stage, which means that the transaction *will* succeed locally after successful completion of this phase, 2. COMMIT message is sent to the foreign server 3. If step two succeeds, local changes are committed and successful commit is conveyed to the client 4. if step two fails, local changes are rolled back and abort status is conveyed to the client 5. If step 1 itself fails, the remote changes are rolled back. This is as per one phase commit protocol which guarantees ACID for single foreign data source. So, the changes involving local and a single foreign server seem to be atomic and consistent. Really? Maybe I'm missing something, but I don't think the current implementation for committing transactions has such a mechanism stated in step 1. So, I think it's possible that the local transaction fails in step3 while the remote transaction succeeds, as mentioned above. PFA a script attached which shows this. You may want to check the code in pgfdw_xact_callback() for actions taken by postgres_fdw on various events. CommitTransaction() for how those events are generated. The code there complies with the sequence above. While postgres_fdw delays remote commit to eliminate many causes for having one server commit while another aborts, other causes remain. The local transaction could still fail due to WAL I/O problems or a system crash. 2PC is the reliable answer, but that was explicitly out of scope for the initial postgres_fdw write support. Does this inheritance patch add any atomicity problem that goes away when one breaks up the inheritance hierarchy and UPDATEs each table separately? If not, this limitation is okay. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On Thu, Dec 04, 2014 at 12:35:54PM +0900, Etsuro Fujita wrote: (2014/12/03 19:35), Ashutosh Bapat wrote: On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: This is not exactly extension of non-inheritance case. non-inheritance case doesn't show two remote SQLs under the same plan node. May be you can rename the label Remote SQL as Remote UPDATE/INSERT/DELETE (or something to that effect) for the DML command and the Foreign plan node should be renamed to Foreign access node or something to indicate that it does both the scan as well as DML. I am not keen about the actual terminology, but I think a reader of plan shouldn't get confused. We can leave this for committer's judgement. Thanks for the proposal! I think that would be a good idea. But I think there would be another idea. An example will be shown below. We show the update commands below the ModifyTable node, not above the corresponding ForeignScan nodes, so maybe less confusing. If there are no objections of you and others, I'll update the patch this way. postgres=# explain verbose update parent set a = a * 2 where a = 5; QUERY PLAN - Update on public.parent (cost=0.00..280.77 rows=25 width=10) On public.ft1 Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1 ^^ It occurs to me that the command generated by the FDW might well not be SQL at all, as is the case with file_fdw and anything else that talks to a NoSQL engine. Would it be reasonable to call this Remote command or something similarly generic? Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/11/28 18:14), Ashutosh Bapat wrote: On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: Apart from the above, I noticed that the patch doesn't consider to call ExplainForeignModify during EXPLAIN for an inherited UPDATE/DELETE, as shown below (note that there are no UPDATE remote queries displayed): So, I'd like to modify explain.c to show those queries like this: postgres=# explain verbose update parent set a = a * 2 where a = 5; QUERY PLAN --__ --__- Update on public.parent (cost=0.00..280.77 rows=25 width=10) - Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10) Output: (parent.a * 2), parent.ctid Filter: (parent.a = 5) Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1 - Foreign Scan on public.ft1 (cost=100.00..140.38 rows=12 width=10) Output: (ft1.a * 2), ft1.ctid Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5)) FOR UPDATE Remote SQL: UPDATE public.mytable_2 SET a = $2 WHERE ctid = $1 - Foreign Scan on public.ft2 (cost=100.00..140.38 rows=12 width=10) Output: (ft2.a * 2), ft2.ctid Remote SQL: SELECT a, ctid FROM public.mytable_2 WHERE ((a = 5)) FOR UPDATE (12 rows) Two remote SQL under a single node would be confusing. Also the node is labelled as Foreign Scan. It would be confusing to show an UPDATE command under this scan node. I thought this as an extention of the existing (ie, non-inherited) case (see the below example) to the inherited case. postgres=# explain verbose update ft1 set a = a * 2 where a = 5; QUERY PLAN - Update on public.ft1 (cost=100.00..140.38 rows=12 width=10) Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1 - Foreign Scan on public.ft1 (cost=100.00..140.38 rows=12 width=10) Output: (a * 2), ctid Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5)) FOR UPDATE (5 rows) I think we should show update commands somewhere for the inherited case as for the non-inherited case. Alternatives to this are welcome. This is not exactly extension of non-inheritance case. non-inheritance case doesn't show two remote SQLs under the same plan node. May be you can rename the label Remote SQL as Remote UPDATE/INSERT/DELETE (or something to that effect) for the DML command and the Foreign plan node should be renamed to Foreign access node or something to indicate that it does both the scan as well as DML. I am not keen about the actual terminology, but I think a reader of plan shouldn't get confused. We can leave this for committer's judgement. BTW, I was experimenting with DMLs being executed on multiple FDW server under same transaction and found that the transactions may not be atomic (and may be inconsistent), if one or more of the server fails to commit while rest of them commit the transaction. The reason for this is, we do not rollback the already committed changes to the foreign server, if one or more of them fail to commit a transaction. With foreign tables under inheritance hierarchy a single DML can span across multiple servers and the result may not be atomic (and may be inconsistent). So, IIUC, even the transactions over the local and the *single* remote server are not guaranteed to be executed atomically in the current form. It is possible that the remote transaction succeeds and the local one fails, for example, resulting in data inconsistency between the local and the remote. IIUC, while committing transactions involving a single remote server, the steps taken are as follows 1. the local changes are brought to PRE-COMMIT stage, which means that the transaction *will* succeed locally after successful completion of this phase, 2. COMMIT message is sent to the foreign server 3. If step two succeeds, local changes are committed and successful commit is conveyed to the client 4. if step two fails, local changes are rolled back and abort status is conveyed to the client 5. If step 1 itself fails, the remote changes are rolled back. This is as per one phase commit protocol which guarantees ACID for single foreign data source. So, the changes involving local and a single foreign server seem to be atomic and consistent. either we have to disable DMLs on an inheritance hierarchy which spans multiple servers. OR make sure that such transactions follow 2PC norms. -1 for disabling update queries on such an inheritance hierarchy because I think we
Re: [HACKERS] inherit support for foreign tables
On Wed, Dec 3, 2014 at 4:05 PM, Ashutosh Bapat ashutosh.ba...@enterprisedb.com wrote: On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/11/28 18:14), Ashutosh Bapat wrote: On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: Apart from the above, I noticed that the patch doesn't consider to call ExplainForeignModify during EXPLAIN for an inherited UPDATE/DELETE, as shown below (note that there are no UPDATE remote queries displayed): So, I'd like to modify explain.c to show those queries like this: postgres=# explain verbose update parent set a = a * 2 where a = 5; QUERY PLAN --__ --__- Update on public.parent (cost=0.00..280.77 rows=25 width=10) - Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10) Output: (parent.a * 2), parent.ctid Filter: (parent.a = 5) Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1 - Foreign Scan on public.ft1 (cost=100.00..140.38 rows=12 width=10) Output: (ft1.a * 2), ft1.ctid Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5)) FOR UPDATE Remote SQL: UPDATE public.mytable_2 SET a = $2 WHERE ctid = $1 - Foreign Scan on public.ft2 (cost=100.00..140.38 rows=12 width=10) Output: (ft2.a * 2), ft2.ctid Remote SQL: SELECT a, ctid FROM public.mytable_2 WHERE ((a = 5)) FOR UPDATE (12 rows) Two remote SQL under a single node would be confusing. Also the node is labelled as Foreign Scan. It would be confusing to show an UPDATE command under this scan node. I thought this as an extention of the existing (ie, non-inherited) case (see the below example) to the inherited case. postgres=# explain verbose update ft1 set a = a * 2 where a = 5; QUERY PLAN - Update on public.ft1 (cost=100.00..140.38 rows=12 width=10) Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1 - Foreign Scan on public.ft1 (cost=100.00..140.38 rows=12 width=10) Output: (a * 2), ctid Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5)) FOR UPDATE (5 rows) I think we should show update commands somewhere for the inherited case as for the non-inherited case. Alternatives to this are welcome. This is not exactly extension of non-inheritance case. non-inheritance case doesn't show two remote SQLs under the same plan node. May be you can rename the label Remote SQL as Remote UPDATE/INSERT/DELETE (or something to that effect) for the DML command and the Foreign plan node should be renamed to Foreign access node or something to indicate that it does both the scan as well as DML. I am not keen about the actual terminology, but I think a reader of plan shouldn't get confused. We can leave this for committer's judgement. BTW, I was experimenting with DMLs being executed on multiple FDW server under same transaction and found that the transactions may not be atomic (and may be inconsistent), if one or more of the server fails to commit while rest of them commit the transaction. The reason for this is, we do not rollback the already committed changes to the foreign server, if one or more of them fail to commit a transaction. With foreign tables under inheritance hierarchy a single DML can span across multiple servers and the result may not be atomic (and may be inconsistent). So, IIUC, even the transactions over the local and the *single* remote server are not guaranteed to be executed atomically in the current form. It is possible that the remote transaction succeeds and the local one fails, for example, resulting in data inconsistency between the local and the remote. IIUC, while committing transactions involving a single remote server, the steps taken are as follows 1. the local changes are brought to PRE-COMMIT stage, which means that the transaction *will* succeed locally after successful completion of this phase, 2. COMMIT message is sent to the foreign server 3. If step two succeeds, local changes are committed and successful commit is conveyed to the client 4. if step two fails, local changes are rolled back and abort status is conveyed to the client 5. If step 1 itself fails, the remote changes are rolled back. This is as per one phase commit protocol which guarantees ACID for single foreign data source. So, the changes involving local and a single foreign server seem to be atomic and consistent. either we have to disable DMLs on an inheritance hierarchy which spans multiple servers. OR make sure that such
Re: [HACKERS] inherit support for foreign tables
On Thu, Dec 4, 2014 at 9:05 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/12/03 19:35), Ashutosh Bapat wrote: On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: This is not exactly extension of non-inheritance case. non-inheritance case doesn't show two remote SQLs under the same plan node. May be you can rename the label Remote SQL as Remote UPDATE/INSERT/DELETE (or something to that effect) for the DML command and the Foreign plan node should be renamed to Foreign access node or something to indicate that it does both the scan as well as DML. I am not keen about the actual terminology, but I think a reader of plan shouldn't get confused. We can leave this for committer's judgement. Thanks for the proposal! I think that would be a good idea. But I think there would be another idea. An example will be shown below. We show the update commands below the ModifyTable node, not above the corresponding ForeignScan nodes, so maybe less confusing. If there are no objections of you and others, I'll update the patch this way. postgres=# explain verbose update parent set a = a * 2 where a = 5; QUERY PLAN - Update on public.parent (cost=0.00..280.77 rows=25 width=10) On public.ft1 Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1 On public.ft2 Remote SQL: UPDATE public.mytable_2 SET a = $2 WHERE ctid = $1 - Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10) Output: (parent.a * 2), parent.ctid Filter: (parent.a = 5) - Foreign Scan on public.ft1 (cost=100.00..140.38 rows=12 width=10) Output: (ft1.a * 2), ft1.ctid Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5)) FOR UPDATE - Foreign Scan on public.ft2 (cost=100.00..140.38 rows=12 width=10) Output: (ft2.a * 2), ft2.ctid Remote SQL: SELECT a, ctid FROM public.mytable_2 WHERE ((a = 5)) FOR UPDATE (12 rows) Looks better. IIUC, even the transactions over the local and the *single* remote server are not guaranteed to be executed atomically in the current form. It is possible that the remote transaction succeeds and the local one fails, for example, resulting in data inconsistency between the local and the remote. IIUC, while committing transactions involving a single remote server, the steps taken are as follows 1. the local changes are brought to PRE-COMMIT stage, which means that the transaction *will* succeed locally after successful completion of this phase, 2. COMMIT message is sent to the foreign server 3. If step two succeeds, local changes are committed and successful commit is conveyed to the client 4. if step two fails, local changes are rolled back and abort status is conveyed to the client 5. If step 1 itself fails, the remote changes are rolled back. This is as per one phase commit protocol which guarantees ACID for single foreign data source. So, the changes involving local and a single foreign server seem to be atomic and consistent. Really? Maybe I'm missing something, but I don't think the current implementation for committing transactions has such a mechanism stated in step 1. So, I think it's possible that the local transaction fails in step3 while the remote transaction succeeds, as mentioned above. PFA a script attached which shows this. You may want to check the code in pgfdw_xact_callback() for actions taken by postgres_fdw on various events. CommitTransaction() for how those events are generated. The code there complies with the sequence above. Thanks, Best regards, Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] inherit support for foreign tables
Sorry, here's the script. On Thu, Dec 4, 2014 at 10:00 AM, Ashutosh Bapat ashutosh.ba...@enterprisedb.com wrote: On Thu, Dec 4, 2014 at 9:05 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/12/03 19:35), Ashutosh Bapat wrote: On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: This is not exactly extension of non-inheritance case. non-inheritance case doesn't show two remote SQLs under the same plan node. May be you can rename the label Remote SQL as Remote UPDATE/INSERT/DELETE (or something to that effect) for the DML command and the Foreign plan node should be renamed to Foreign access node or something to indicate that it does both the scan as well as DML. I am not keen about the actual terminology, but I think a reader of plan shouldn't get confused. We can leave this for committer's judgement. Thanks for the proposal! I think that would be a good idea. But I think there would be another idea. An example will be shown below. We show the update commands below the ModifyTable node, not above the corresponding ForeignScan nodes, so maybe less confusing. If there are no objections of you and others, I'll update the patch this way. postgres=# explain verbose update parent set a = a * 2 where a = 5; QUERY PLAN - Update on public.parent (cost=0.00..280.77 rows=25 width=10) On public.ft1 Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1 On public.ft2 Remote SQL: UPDATE public.mytable_2 SET a = $2 WHERE ctid = $1 - Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10) Output: (parent.a * 2), parent.ctid Filter: (parent.a = 5) - Foreign Scan on public.ft1 (cost=100.00..140.38 rows=12 width=10) Output: (ft1.a * 2), ft1.ctid Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5)) FOR UPDATE - Foreign Scan on public.ft2 (cost=100.00..140.38 rows=12 width=10) Output: (ft2.a * 2), ft2.ctid Remote SQL: SELECT a, ctid FROM public.mytable_2 WHERE ((a = 5)) FOR UPDATE (12 rows) Looks better. IIUC, even the transactions over the local and the *single* remote server are not guaranteed to be executed atomically in the current form. It is possible that the remote transaction succeeds and the local one fails, for example, resulting in data inconsistency between the local and the remote. IIUC, while committing transactions involving a single remote server, the steps taken are as follows 1. the local changes are brought to PRE-COMMIT stage, which means that the transaction *will* succeed locally after successful completion of this phase, 2. COMMIT message is sent to the foreign server 3. If step two succeeds, local changes are committed and successful commit is conveyed to the client 4. if step two fails, local changes are rolled back and abort status is conveyed to the client 5. If step 1 itself fails, the remote changes are rolled back. This is as per one phase commit protocol which guarantees ACID for single foreign data source. So, the changes involving local and a single foreign server seem to be atomic and consistent. Really? Maybe I'm missing something, but I don't think the current implementation for committing transactions has such a mechanism stated in step 1. So, I think it's possible that the local transaction fails in step3 while the remote transaction succeeds, as mentioned above. PFA a script attached which shows this. You may want to check the code in pgfdw_xact_callback() for actions taken by postgres_fdw on various events. CommitTransaction() for how those events are generated. The code there complies with the sequence above. Thanks, Best regards, Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company tran_inconsistency.sql Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/11/28 18:14), Ashutosh Bapat wrote: On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: Apart from the above, I noticed that the patch doesn't consider to call ExplainForeignModify during EXPLAIN for an inherited UPDATE/DELETE, as shown below (note that there are no UPDATE remote queries displayed): So, I'd like to modify explain.c to show those queries like this: postgres=# explain verbose update parent set a = a * 2 where a = 5; QUERY PLAN --__--__- Update on public.parent (cost=0.00..280.77 rows=25 width=10) - Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10) Output: (parent.a * 2), parent.ctid Filter: (parent.a = 5) Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1 - Foreign Scan on public.ft1 (cost=100.00..140.38 rows=12 width=10) Output: (ft1.a * 2), ft1.ctid Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5)) FOR UPDATE Remote SQL: UPDATE public.mytable_2 SET a = $2 WHERE ctid = $1 - Foreign Scan on public.ft2 (cost=100.00..140.38 rows=12 width=10) Output: (ft2.a * 2), ft2.ctid Remote SQL: SELECT a, ctid FROM public.mytable_2 WHERE ((a = 5)) FOR UPDATE (12 rows) Two remote SQL under a single node would be confusing. Also the node is labelled as Foreign Scan. It would be confusing to show an UPDATE command under this scan node. I thought this as an extention of the existing (ie, non-inherited) case (see the below example) to the inherited case. postgres=# explain verbose update ft1 set a = a * 2 where a = 5; QUERY PLAN - Update on public.ft1 (cost=100.00..140.38 rows=12 width=10) Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1 - Foreign Scan on public.ft1 (cost=100.00..140.38 rows=12 width=10) Output: (a * 2), ctid Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5)) FOR UPDATE (5 rows) I think we should show update commands somewhere for the inherited case as for the non-inherited case. Alternatives to this are welcome. BTW, I was experimenting with DMLs being executed on multiple FDW server under same transaction and found that the transactions may not be atomic (and may be inconsistent), if one or more of the server fails to commit while rest of them commit the transaction. The reason for this is, we do not rollback the already committed changes to the foreign server, if one or more of them fail to commit a transaction. With foreign tables under inheritance hierarchy a single DML can span across multiple servers and the result may not be atomic (and may be inconsistent). So, IIUC, even the transactions over the local and the *single* remote server are not guaranteed to be executed atomically in the current form. It is possible that the remote transaction succeeds and the local one fails, for example, resulting in data inconsistency between the local and the remote. either we have to disable DMLs on an inheritance hierarchy which spans multiple servers. OR make sure that such transactions follow 2PC norms. -1 for disabling update queries on such an inheritance hierarchy because I think we should leave that to the user's judgment. And I think 2PC is definitely a separate patch. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:
(2014/11/17 17:55), Ashutosh Bapat wrote:
Here are my review comments for patch fdw-inh-3.patch.
Thanks for the review!
Tests
---
1. It seems like you have copied from testcase inherit.sql to
postgres_fdw testcase. That's a good thing, but it makes the test quite
long. May be we should have two tests in postgres_fdw contrib module,
one for simple cases, and other for inheritance. What do you say?
IMO, the test is not so time-consuming, so it doesn't seem worth splitting
it into two.
I am not worried about the timing but I am worried about the length of the
file and hence ease of debugging in case we find any issues there. We will
leave that for the commiter to decide.
Documentation
1. The change in ddl.sgml
-We will refer to the child tables as partitions, though they
-are in every way normal productnamePostgreSQL/ tables.
+We will refer to the child tables as partitions, though we assume
+that they are normal productnamePostgreSQL/ tables.
adds phrase we assume that, which confuses the intention behind the
sentence. The original text is intended to highlight the equivalence
between partition and normal table, where as the addition esp. the
word assume weakens that equivalence. Instead now we have to highlight
the equivalence between partition and normal or foreign table. The
wording similar to though they are either normal or foreign tables
should be used there.
You are right, but I feel that there is something out of place in saying
that there (5.10.2. Implementing Partitioning) because the procedure there
has been written based on normal tables only. Put another way, if we say
that, I think we'd need to add more docs, describing the syntax and/or the
corresponding examples for foreign-table cases. But I'd like to leave that
for another patch. So, how about the wording we assume *here* that,
instead of we assume that, as I added the following notice in the
previous section (5.10.1. Overview)?
@@ -2650,7 +2669,10 @@ VALUES ('Albany', NULL, NULL, 'NY');
table of a single parent table. The parent table itself is normally
empty; it exists just to represent the entire data set. You should be
familiar with inheritance (see xref linkend=ddl-inherit) before
-attempting to set up partitioning.
+attempting to set up partitioning. (The setup and management of
+partitioned tables illustrated in this section assume that each
+partition is a normal table. However, you can do that in a similar
way
+for cases where some or all partitions are foreign tables.)
This looks ok, though, I would like to see final version of the document.
But I think, we will leave that for committer to handle.
2. The wording some kind of optimization gives vague picture. May be
it can be worded as Since the constraints are assumed to be true, they
are used in constraint-based query optimization like constraint
exclusion for partitioned tables..
+Those constraints are used in some kind of query optimization such
+as constraint exclusion for partitioned tables (see
+xref linkend=ddl-partitioning).
Will follow your revision.
Thanks.
Code
---
1. In the following change
+/*
* acquire_inherited_sample_rows -- acquire sample rows from
inheritance tree
*
* This has the same API as acquire_sample_rows, except that rows are
* collected from all inheritance children as well as the specified
table.
- * We fail and return zero if there are no inheritance children.
+ * We fail and return zero if there are no inheritance children or
there are
+ * inheritance children that foreign tables.
The addition should be there are inheritance children that *are all
*foreign tables. Note the addition are all.
Sorry, I incorrectly wrote the comment. What I tried to write is We fail
and return zero if there are no inheritance children or if we are not in
VAC_MODE_SINGLE case and inheritance tree contains at least one foreign
table..
You might want to use English description of VAC_MODE_SINGLE instead of
that macro in the comment, so that reader doesn't have to look up
VAC_MODE_SINGLE. But I think, we will leave this for the committer.
2. The function has_foreign() be better named has_foreign_child()? This
How about has_foreign_table?
has_foreign_child() would be better, since these are children in the
inheritance hierarchy and not mere tables.
function loops through all the tableoids passed even after it has found
a foreign table. Why can't we return true immediately after finding the
first foreign table, unless the side effects of heap_open() on all the
table are required. But I don't see that to be the case, since these
tables are locked already through a previous call to heap_open(). In the
Good catch! Will fix.
same function
Re: [HACKERS] inherit support for foreign tables
(2014/11/17 17:55), Ashutosh Bapat wrote:
Here are my review comments for patch fdw-inh-3.patch.
Thanks for the review!
Tests
---
1. It seems like you have copied from testcase inherit.sql to
postgres_fdw testcase. That's a good thing, but it makes the test quite
long. May be we should have two tests in postgres_fdw contrib module,
one for simple cases, and other for inheritance. What do you say?
IMO, the test is not so time-consuming, so it doesn't seem worth
splitting it into two.
Documentation
1. The change in ddl.sgml
-We will refer to the child tables as partitions, though they
-are in every way normal productnamePostgreSQL/ tables.
+We will refer to the child tables as partitions, though we assume
+that they are normal productnamePostgreSQL/ tables.
adds phrase we assume that, which confuses the intention behind the
sentence. The original text is intended to highlight the equivalence
between partition and normal table, where as the addition esp. the
word assume weakens that equivalence. Instead now we have to highlight
the equivalence between partition and normal or foreign table. The
wording similar to though they are either normal or foreign tables
should be used there.
You are right, but I feel that there is something out of place in saying
that there (5.10.2. Implementing Partitioning) because the procedure
there has been written based on normal tables only. Put another way, if
we say that, I think we'd need to add more docs, describing the syntax
and/or the corresponding examples for foreign-table cases. But I'd like
to leave that for another patch. So, how about the wording we assume
*here* that, instead of we assume that, as I added the following
notice in the previous section (5.10.1. Overview)?
@@ -2650,7 +2669,10 @@ VALUES ('Albany', NULL, NULL, 'NY');
table of a single parent table. The parent table itself is normally
empty; it exists just to represent the entire data set. You should be
familiar with inheritance (see xref linkend=ddl-inherit) before
-attempting to set up partitioning.
+attempting to set up partitioning. (The setup and management of
+partitioned tables illustrated in this section assume that each
+partition is a normal table. However, you can do that in a similar way
+for cases where some or all partitions are foreign tables.)
2. The wording some kind of optimization gives vague picture. May be
it can be worded as Since the constraints are assumed to be true, they
are used in constraint-based query optimization like constraint
exclusion for partitioned tables..
+Those constraints are used in some kind of query optimization such
+as constraint exclusion for partitioned tables (see
+xref linkend=ddl-partitioning).
Will follow your revision.
Code
---
1. In the following change
+/*
* acquire_inherited_sample_rows -- acquire sample rows from
inheritance tree
*
* This has the same API as acquire_sample_rows, except that rows are
* collected from all inheritance children as well as the specified table.
- * We fail and return zero if there are no inheritance children.
+ * We fail and return zero if there are no inheritance children or
there are
+ * inheritance children that foreign tables.
The addition should be there are inheritance children that *are all
*foreign tables. Note the addition are all.
Sorry, I incorrectly wrote the comment. What I tried to write is We
fail and return zero if there are no inheritance children or if we are
not in VAC_MODE_SINGLE case and inheritance tree contains at least one
foreign table..
2. The function has_foreign() be better named has_foreign_child()? This
How about has_foreign_table?
function loops through all the tableoids passed even after it has found
a foreign table. Why can't we return true immediately after finding the
first foreign table, unless the side effects of heap_open() on all the
table are required. But I don't see that to be the case, since these
tables are locked already through a previous call to heap_open(). In the
Good catch! Will fix.
same function instead of argument name parentrelId may be we should use
name parent_oid, so that we use same notation for parent and child table
OIDs.
Will fix.
3. Regarding enum VacuumMode - it's being used only in case of
acquire_inherited_sample_rows() and that too, to check only a single
value of the three defined there. May be we should just infer that value
inside acquire_inherited_sample_rows() or pass a boolean true or false
from do_analyse_rel() based on the VacuumStmt. I do not see need for a
separate three value enum of which only one value is used and also to
pass it down from vacuum() by changing signatures of the minion functions.
I introduced that for possible future use. See the discussion in [1].
4. In postgresGetForeignPlan(), the code added by this patch is required
to handle the case when
Re: [HACKERS] inherit support for foreign tables
On Mon, Nov 17, 2014 at 1:25 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/11/12 20:04), Ashutosh Bapat wrote: I reviewed fdw-chk-3 patch. Here are my comments Thanks for the review! Tests --- 1. The tests added in file_fdw module look good. We should add tests for CREATE TABLE with CHECK CONSTRAINT also. Agreed. I added the tests, and also changed the proposed tests a bit. 2. For postgres_fdw we need tests to check the behaviour in case the constraints mismatch between the remote table and its local foreign table declaration in case of INSERT, UPDATE and SELECT. Done. 3. In the testcases for postgres_fdw it seems that you have forgotten to add statement after SET constraint_exclusion to 'partition' I added the statement. 4. In test foreign_data there are changes to fix the diffs caused by these changes like below ALTER FOREIGN TABLE ft1 DROP CONSTRAINT no_const; -- ERROR -ERROR: ft1 is not a table +ERROR: constraint no_const of relation ft1 does not exist ALTER FOREIGN TABLE ft1 DROP CONSTRAINT IF EXISTS no_const; -ERROR: ft1 is not a table +NOTICE: constraint no_const of relation ft1 does not exist, skipping ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c1_check; -ERROR: ft1 is not a table +ERROR: constraint ft1_c1_check of relation ft1 does not exist Earlier when constraints were not supported for FOREIGN TABLE, these tests made sure the same functionality. So, even though the corresponding constraints were not created on the table (in fact it didn't allow the creation as well). Now that the constraints are allowed, I think the tests for no_const (without IF EXISTS) and ft1_c1_check are duplicating the same testcase. May be we should review this set of statement in the light of new functionality. Agreed. I removed the DROP CONSTRAINT ft1_c1_check test, and added a new test for ALTER CONSTRAINT. Code and implementation -- The usage of NO INHERIT and NOT VALID with CONSTRAINT on foreign table is blocked, but corresponding documentation entry doesn't mention so. Since foreign tables can not be inherited NO INHERIT option isn't applicable to foreign tables and the constraints on the foreign tables are declarative, hence NOT VALID option is also not applicable. So, I agree with what the code is doing, but that should be reflected in documentation with this explanation. Rest of the code modifies the condition checks for CHECK CONSTRAINTs on foreign tables, and it looks good to me. Done. Other changes: * Modified one error message that I added in AddRelationNewConstraints, to match the other message there. * Revised other docs a little bit. Attached is an updated version of the patch. I looked at the patch. It looks good now. Once we have the inh patch ready, we can mark this item as ready for commiter. Thanks, Best regards, Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] inherit support for foreign tables
(2014/11/18 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
Hi Fujita-san, I reviewed fdw-chk-3 patch. Here are my comments Sanity 1. The patch applies on the latest master using patch but not by git apply 2. it compiles clean 3. Regression run is clean, including the contrib module regressions Tests --- 1. The tests added in file_fdw module look good. We should add tests for CREATE TABLE with CHECK CONSTRAINT also. 2. For postgres_fdw we need tests to check the behaviour in case the constraints mismatch between the remote table and its local foreign table declaration in case of INSERT, UPDATE and SELECT. 3. In the testcases for postgres_fdw it seems that you have forgotten to add statement after SET constraint_exclusion to 'partition' 4. In test foreign_data there are changes to fix the diffs caused by these changes like below ALTER FOREIGN TABLE ft1 DROP CONSTRAINT no_const; -- ERROR -ERROR: ft1 is not a table +ERROR: constraint no_const of relation ft1 does not exist ALTER FOREIGN TABLE ft1 DROP CONSTRAINT IF EXISTS no_const; -ERROR: ft1 is not a table +NOTICE: constraint no_const of relation ft1 does not exist, skipping ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c1_check; -ERROR: ft1 is not a table +ERROR: constraint ft1_c1_check of relation ft1 does not exist Earlier when constraints were not supported for FOREIGN TABLE, these tests made sure the same functionality. So, even though the corresponding constraints were not created on the table (in fact it didn't allow the creation as well). Now that the constraints are allowed, I think the tests for no_const (without IF EXISTS) and ft1_c1_check are duplicating the same testcase. May be we should review this set of statement in the light of new functionality. Code and implementation -- The usage of NO INHERIT and NOT VALID with CONSTRAINT on foreign table is blocked, but corresponding documentation entry doesn't mention so. Since foreign tables can not be inherited NO INHERIT option isn't applicable to foreign tables and the constraints on the foreign tables are declarative, hence NOT VALID option is also not applicable. So, I agree with what the code is doing, but that should be reflected in documentation with this explanation. Rest of the code modifies the condition checks for CHECK CONSTRAINTs on foreign tables, and it looks good to me. On Fri, Nov 7, 2014 at 5:31 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/11/07 14:57), Kyotaro HORIGUCHI wrote: Here are separated patches. fdw-chk.patch - CHECK constraints on foreign tables fdw-inh.patch - table inheritance with foreign tables The latter has been created on top of [1]. [1] http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp To be exact, it has been created on top of [1] and fdw-chk.patch. I tried both patches on the current head, the newly added parameter to analyze_rel() hampered them from applying but it is easy to fix seemingly and almost all the other part was applied cleanly. Thanks for the review! By the way, are these the result of simply splitting of your last patch, foreign_inherit-v15.patch? http://www.postgresql.org/message-id/53feef94.6040...@lab.ntt.co.jp The answer is no. The result of apllying whole-in-one version and this splitted version seem to have many differences. Did you added even other changes? Or do I understand this patch wrongly? As I said before, I splitted the whole-in-one version into three: 1) CHECK constraint patch (ie fdw-chk.patch), 2) table inheritance patch (ie fdw-inh.patch) and 3) path reparameterization patch (not posted). In addition to that, I slightly modified #1 and #2. IIUC, #3 would be useful not only for the inheritance cases but for union all cases. So, I plan to propose it independently in the next CF. I noticed that the latter disallows TRUNCATE on inheritance trees that contain at least one child foreign table. But I think it would be better to allow it, with the semantics that we quietly ignore the child foreign tables and apply the operation to the child plain tables, which is the same semantics as ALTER COLUMN SET STORAGE on such inheritance trees. Comments welcome. Done. And I've also a bit revised regression tests for both patches. Patches attached. I rebased the patches to the latest head. Here are updated patches. Other changes: * fdw-chk-3.patch: the updated patch revises some ereport messages a little bit. * fdw-inh-3.patch: I noticed that there is a doc bug in the previous patch. The updated patch fixes that, adds a bit more docs, and revises regression tests in foreign_data.sql a bit further. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] inherit support for foreign tables
Hi Fujita-san, I tried to apply fdw-inh-3.patch on the latest head from master branch. It failed to apply using both patch and git apply. patch failed to apply because of rejections in contrib/file_fdw/output/file_fdw.source and doc/src/sgml/ref/create_foreign_table.sgml On Fri, Nov 7, 2014 at 5:31 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/11/07 14:57), Kyotaro HORIGUCHI wrote: Here are separated patches. fdw-chk.patch - CHECK constraints on foreign tables fdw-inh.patch - table inheritance with foreign tables The latter has been created on top of [1]. [1] http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp To be exact, it has been created on top of [1] and fdw-chk.patch. I tried both patches on the current head, the newly added parameter to analyze_rel() hampered them from applying but it is easy to fix seemingly and almost all the other part was applied cleanly. Thanks for the review! By the way, are these the result of simply splitting of your last patch, foreign_inherit-v15.patch? http://www.postgresql.org/message-id/53feef94.6040...@lab.ntt.co.jp The answer is no. The result of apllying whole-in-one version and this splitted version seem to have many differences. Did you added even other changes? Or do I understand this patch wrongly? As I said before, I splitted the whole-in-one version into three: 1) CHECK constraint patch (ie fdw-chk.patch), 2) table inheritance patch (ie fdw-inh.patch) and 3) path reparameterization patch (not posted). In addition to that, I slightly modified #1 and #2. IIUC, #3 would be useful not only for the inheritance cases but for union all cases. So, I plan to propose it independently in the next CF. I noticed that the latter disallows TRUNCATE on inheritance trees that contain at least one child foreign table. But I think it would be better to allow it, with the semantics that we quietly ignore the child foreign tables and apply the operation to the child plain tables, which is the same semantics as ALTER COLUMN SET STORAGE on such inheritance trees. Comments welcome. Done. And I've also a bit revised regression tests for both patches. Patches attached. I rebased the patches to the latest head. Here are updated patches. Other changes: * fdw-chk-3.patch: the updated patch revises some ereport messages a little bit. * fdw-inh-3.patch: I noticed that there is a doc bug in the previous patch. The updated patch fixes that, adds a bit more docs, and revises regression tests in foreign_data.sql a bit further. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] inherit support for foreign tables
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
On Thu, Nov 13, 2014 at 12:20 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: Hi Ashutosh, Thanks for the review! (2014/11/13 15:23), Ashutosh Bapat wrote: I tried to apply fdw-inh-3.patch on the latest head from master branch. It failed to apply using both patch and git apply. patch failed to apply because of rejections in contrib/file_fdw/output/file_fdw.source and doc/src/sgml/ref/create_foreign_table.sgml As I said upthread, fdw-inh-3.patch has been created on top of [1] and fdw-chk-3.patch. Did you apply these patche first? Oh, sorry, I didn't pay attention to that. I will apply both the patches and review the inheritance patch. Thanks for pointing that out. [1] https://commitfest.postgresql.org/action/patch_view?id=1599 Best regards, Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] inherit support for foreign tables
Hi Furuya-san, (2014/11/07 16:54), furu...@pm.nttdata.co.jp wrote: (2014/08/28 18:00), Etsuro Fujita wrote: (2014/08/22 11:51), Noah Misch wrote: Today's ANALYZE VERBOSE messaging for former inheritance parents (tables with relhassubclass = true but no pg_inherits.inhparent links) is deceptive, and I welcome a fix to omit the spurious message. As defects go, this is quite minor. There's fundamentally no value in collecting inheritance tree statistics for such a parent, and no PostgreSQL command will do so. A credible alternative is to emit a second message indicating that we skipped the inheritance tree statistics after all, and why we skipped them. I'd like to address this by emitting the second message as shown below: A separate patch (analyze.patch) handles the former case in a similar way. I'll add to the upcoming CF, the analyze.patch as an independent item, which emits a second message indicating that we skipped the inheritance tree statistics and why we skipped them. I did a review of the patch. There was no problem. I confirmed the following. 1. applied cleanly and compilation was without warnings and errors 2. all regress tests was passed ok 3. The message output from ANALYZE VERBOSE. Following are the SQL which I used to check messages. create table parent (id serial); create table child (name text) inherits (parent); ANALYZE VERBOSE parent ; drop table child ; ANALYZE VERBOSE parent ; I think that that is a correct test for this patch. Thanks for the review! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/11/07 14:57), Kyotaro HORIGUCHI wrote: Here are separated patches. fdw-chk.patch - CHECK constraints on foreign tables fdw-inh.patch - table inheritance with foreign tables The latter has been created on top of [1]. [1] http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp To be exact, it has been created on top of [1] and fdw-chk.patch. I tried both patches on the current head, the newly added parameter to analyze_rel() hampered them from applying but it is easy to fix seemingly and almost all the other part was applied cleanly. Thanks for the review! By the way, are these the result of simply splitting of your last patch, foreign_inherit-v15.patch? http://www.postgresql.org/message-id/53feef94.6040...@lab.ntt.co.jp The answer is no. The result of apllying whole-in-one version and this splitted version seem to have many differences. Did you added even other changes? Or do I understand this patch wrongly? As I said before, I splitted the whole-in-one version into three: 1) CHECK constraint patch (ie fdw-chk.patch), 2) table inheritance patch (ie fdw-inh.patch) and 3) path reparameterization patch (not posted). In addition to that, I slightly modified #1 and #2. IIUC, #3 would be useful not only for the inheritance cases but for union all cases. So, I plan to propose it independently in the next CF. I noticed that the latter disallows TRUNCATE on inheritance trees that contain at least one child foreign table. But I think it would be better to allow it, with the semantics that we quietly ignore the child foreign tables and apply the operation to the child plain tables, which is the same semantics as ALTER COLUMN SET STORAGE on such inheritance trees. Comments welcome. Done. And I've also a bit revised regression tests for both patches. Patches attached. I rebased the patches to the latest head. Here are updated patches. Other changes: * fdw-chk-3.patch: the updated patch revises some ereport messages a little bit. * fdw-inh-3.patch: I noticed that there is a doc bug in the previous patch. The updated patch fixes that, adds a bit more docs, and revises regression tests in foreign_data.sql a bit further. Thanks, Best regards, Etsuro Fujita *** a/contrib/file_fdw/input/file_fdw.source --- b/contrib/file_fdw/input/file_fdw.source *** *** 144,149 SET constraint_exclusion = 'partition'; --- 144,166 \t off ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check; + -- table inheritance tests + CREATE TABLE agg (a int2, b float4); + ALTER FOREIGN TABLE agg_csv INHERIT agg; + ALTER FOREIGN TABLE agg_text INHERIT agg; + SELECT * FROM agg WHERE b 10.0 ORDER BY a, b; + SELECT * FROM ONLY agg WHERE b 10.0 ORDER BY a, b; + SELECT * FROM agg_csv WHERE b 10.0 ORDER BY a, b; + SELECT * FROM agg_text WHERE b 10.0 ORDER BY a, b; + -- updates aren't supported + UPDATE agg SET a = 1; + DELETE FROM agg WHERE a = 100; + -- but this should be ignored + SELECT * FROM agg WHERE b 10.0 ORDER BY a, b FOR UPDATE; + ALTER FOREIGN TABLE agg_csv NO INHERIT agg; + ALTER FOREIGN TABLE agg_text NO INHERIT agg; + DROP TABLE agg; + -- privilege tests SET ROLE file_fdw_superuser; SELECT * FROM agg_text ORDER BY a; *** a/contrib/file_fdw/output/file_fdw.source --- b/contrib/file_fdw/output/file_fdw.source *** *** 237,242 EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a 0; --- 237,289 SET constraint_exclusion = 'partition'; \t off ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check; + -- table inheritance tests + CREATE TABLE agg (a int2, b float4); + ALTER FOREIGN TABLE agg_csv INHERIT agg; + ALTER FOREIGN TABLE agg_text INHERIT agg; + SELECT * FROM agg WHERE b 10.0 ORDER BY a, b; + a |b + +- + 0 | 0.09561 + 0 | 0.09561 + 56 | 7.8 + (3 rows) + + SELECT * FROM ONLY agg WHERE b 10.0 ORDER BY a, b; + a | b + ---+--- + (0 rows) + + SELECT * FROM agg_csv WHERE b 10.0 ORDER BY a, b; + a |b + ---+- + 0 | 0.09561 + (1 row) + + SELECT * FROM agg_text WHERE b 10.0 ORDER BY a, b; + a |b + +- + 0 | 0.09561 + 56 | 7.8 + (2 rows) + + -- updates aren't supported + UPDATE agg SET a = 1; + ERROR: cannot update foreign table agg_text + DELETE FROM agg WHERE a = 100; + ERROR: cannot delete from foreign table agg_text + -- but this should be ignored + SELECT * FROM agg WHERE b 10.0 ORDER BY a, b FOR UPDATE; + a |b + +- + 0 | 0.09561 + 0 | 0.09561 + 56 | 7.8 + (3 rows) + + ALTER FOREIGN TABLE agg_csv NO INHERIT agg; + ALTER FOREIGN TABLE agg_text NO INHERIT agg; + DROP TABLE agg; -- privilege tests SET ROLE file_fdw_superuser; SELECT * FROM agg_text ORDER BY a; *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 2863,2868 NOTICE: NEW: (13,test triggered !) --- 2863,3376 (1 row) --
Re: [HACKERS] inherit support for foreign tables
Hello, I don't fully catch up this topic but tried this one. Here are separated patches. fdw-chk.patch - CHECK constraints on foreign tables fdw-inh.patch - table inheritance with foreign tables The latter has been created on top of [1]. [1] http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp To be exact, it has been created on top of [1] and fdw-chk.patch. I tried both patches on the current head, the newly added parameter to analyze_rel() hampered them from applying but it is easy to fix seemingly and almost all the other part was applied cleanly. By the way, are these the result of simply splitting of your last patch, foreign_inherit-v15.patch? http://www.postgresql.org/message-id/53feef94.6040...@lab.ntt.co.jp The result of apllying whole-in-one version and this splitted version seem to have many differences. Did you added even other changes? Or do I understand this patch wrongly? git diff --numstat 0_foreign_inherit_one 0_foreign_inherit_two 5 51 contrib/file_fdw/file_fdw.c 10 19 contrib/file_fdw/input/file_fdw.source 18 71 contrib/file_fdw/output/file_fdw.source 19 70 contrib/postgres_fdw/expected/postgres_fdw.out 9 66 contrib/postgres_fdw/postgres_fdw.c 12 35 contrib/postgres_fdw/sql/postgres_fdw.sql 13 48 doc/src/sgml/fdwhandler.sgml 39 39 doc/src/sgml/ref/alter_foreign_table.sgml 4 3 doc/src/sgml/ref/create_foreign_table.sgml 8 0 src/backend/catalog/heap.c 7 3 src/backend/commands/analyze.c 0 7 src/backend/commands/tablecmds.c 1 22 src/backend/optimizer/plan/createplan.c 7 7 src/backend/optimizer/prep/prepunion.c 0 26 src/backend/optimizer/util/pathnode.c 1 1 src/include/commands/vacuum.h 0 7 src/include/foreign/fdwapi.h 19 1 src/test/regress/expected/foreign_data.out 9 2 src/test/regress/sql/foreign_data.sql I noticed that the latter disallows TRUNCATE on inheritance trees that contain at least one child foreign table. But I think it would be better to allow it, with the semantics that we quietly ignore the child foreign tables and apply the operation to the child plain tables, which is the same semantics as ALTER COLUMN SET STORAGE on such inheritance trees. Comments welcome. Done. And I've also a bit revised regression tests for both patches. Patches attached. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/08/28 18:00), Etsuro Fujita wrote: (2014/08/22 11:51), Noah Misch wrote: Today's ANALYZE VERBOSE messaging for former inheritance parents (tables with relhassubclass = true but no pg_inherits.inhparent links) is deceptive, and I welcome a fix to omit the spurious message. As defects go, this is quite minor. There's fundamentally no value in collecting inheritance tree statistics for such a parent, and no PostgreSQL command will do so. A credible alternative is to emit a second message indicating that we skipped the inheritance tree statistics after all, and why we skipped them. I'd like to address this by emitting the second message as shown below: A separate patch (analyze.patch) handles the former case in a similar way. I'll add to the upcoming CF, the analyze.patch as an independent item, which emits a second message indicating that we skipped the inheritance tree statistics and why we skipped them. I did a review of the patch. There was no problem. I confirmed the following. 1. applied cleanly and compilation was without warnings and errors 2. all regress tests was passed ok 3. The message output from ANALYZE VERBOSE. Following are the SQL which I used to check messages. create table parent (id serial); create table child (name text) inherits (parent); ANALYZE VERBOSE parent ; drop table child ; ANALYZE VERBOSE parent ; Regards, -- Furuya Osamu -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/10/21 17:40), Etsuro Fujita wrote: (2014/10/14 20:00), Etsuro Fujita wrote: Here are separated patches. fdw-chk.patch - CHECK constraints on foreign tables fdw-inh.patch - table inheritance with foreign tables The latter has been created on top of [1]. [1] http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp To be exact, it has been created on top of [1] and fdw-chk.patch. I noticed that the latter disallows TRUNCATE on inheritance trees that contain at least one child foreign table. But I think it would be better to allow it, with the semantics that we quietly ignore the child foreign tables and apply the operation to the child plain tables, which is the same semantics as ALTER COLUMN SET STORAGE on such inheritance trees. Comments welcome. Done. And I've also a bit revised regression tests for both patches. Patches attached. Thanks, Best regards, Etsuro Fujita *** a/contrib/file_fdw/input/file_fdw.source --- b/contrib/file_fdw/input/file_fdw.source *** *** 134,139 DELETE FROM agg_csv WHERE a = 100; --- 134,149 -- but this should be ignored SELECT * FROM agg_csv FOR UPDATE; + -- constraint exclusion tests + ALTER FOREIGN TABLE agg_csv ADD CONSTRAINT agg_csv_a_check CHECK (a = 0); + \t on + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a 0; + SET constraint_exclusion = 'on'; + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a 0; + SET constraint_exclusion = 'partition'; + \t off + ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check; + -- privilege tests SET ROLE file_fdw_superuser; SELECT * FROM agg_text ORDER BY a; *** a/contrib/file_fdw/output/file_fdw.source --- b/contrib/file_fdw/output/file_fdw.source *** *** 219,224 SELECT * FROM agg_csv FOR UPDATE; --- 219,242 42 | 324.78 (3 rows) + -- constraint exclusion tests + ALTER FOREIGN TABLE agg_csv ADD CONSTRAINT agg_csv_a_check CHECK (a = 0); + \t on + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a 0; + Foreign Scan on public.agg_csv +Output: a, b +Filter: (agg_csv.a 0) +Foreign File: @abs_srcdir@/data/agg.csv + + SET constraint_exclusion = 'on'; + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a 0; + Result +Output: a, b +One-Time Filter: false + + SET constraint_exclusion = 'partition'; + \t off + ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check; -- privilege tests SET ROLE file_fdw_superuser; SELECT * FROM agg_text ORDER BY a; *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 2488,2493 select c2, count(*) from S 1.T 1 where c2 500 group by 1 order by 1; --- 2488,2515 (13 rows) -- === + -- test constraint exclusion stuff + -- === + ALTER FOREIGN TABLE ft1 ADD CONSTRAINT ft1_c2positive CHECK (c2 = 0); + EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 WHERE c2 0; + QUERY PLAN + -- + Foreign Scan on public.ft1 +Output: c1, c2, c3, c4, c5, c6, c7, c8 +Remote SQL: SELECT C 1, c2, c3, c4, c5, c6, c7, c8 FROM S 1.T 1 WHERE ((c2 0)) + (3 rows) + + SET constraint_exclusion = 'on'; + EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 WHERE c2 0; + QUERY PLAN + -- + Result +Output: c1, c2, c3, c4, c5, c6, c7, c8 +One-Time Filter: false + (3 rows) + + SET constraint_exclusion = 'partition'; + -- === -- test serial columns (ie, sequence-based defaults) -- === create table loc1 (f1 serial, f2 text); *** a/contrib/postgres_fdw/sql/postgres_fdw.sql --- b/contrib/postgres_fdw/sql/postgres_fdw.sql *** *** 387,392 select c2, count(*) from ft2 where c2 500 group by 1 order by 1; --- 387,401 select c2, count(*) from S 1.T 1 where c2 500 group by 1 order by 1; -- === + -- test constraint exclusion stuff + -- === + ALTER FOREIGN TABLE ft1 ADD CONSTRAINT ft1_c2positive CHECK (c2 = 0); + EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 WHERE c2 0; + SET constraint_exclusion = 'on'; + EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 WHERE c2 0; + SET constraint_exclusion = 'partition'; + + -- === -- test serial columns (ie, sequence-based defaults) -- === create table
Re: [HACKERS] inherit support for foreign tables
(2014/10/14 20:00), Etsuro Fujita wrote: Here are separated patches. fdw-chk.patch - CHECK constraints on foreign tables fdw-inh.patch - table inheritance with foreign tables The latter has been created on top of [1]. [1] http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp To be exact, it has been created on top of [1] and fdw-chk.patch. I noticed that the latter disallows TRUNCATE on inheritance trees that contain at least one child foreign table. But I think it would be better to allow it, with the semantics that we quietly ignore the child foreign tables and apply the operation to the child plain tables, which is the same semantics as ALTER COLUMN SET STORAGE on such inheritance trees. Comments welcome. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/08/28 18:00), Etsuro Fujita wrote: (2014/08/22 11:51), Noah Misch wrote: Today's ANALYZE VERBOSE messaging for former inheritance parents (tables with relhassubclass = true but no pg_inherits.inhparent links) is deceptive, and I welcome a fix to omit the spurious message. As defects go, this is quite minor. There's fundamentally no value in collecting inheritance tree statistics for such a parent, and no PostgreSQL command will do so. A credible alternative is to emit a second message indicating that we skipped the inheritance tree statistics after all, and why we skipped them. I'd like to address this by emitting the second message as shown below: A separate patch (analyze.patch) handles the former case in a similar way. I'll add to the upcoming CF, the analyze.patch as an independent item, which emits a second message indicating that we skipped the inheritance tree statistics and why we skipped them. Thanks, Best regards, Etsuro Fujita *** a/src/backend/commands/analyze.c --- b/src/backend/commands/analyze.c *** *** 1478,1483 acquire_inherited_sample_rows(Relation onerel, int elevel, --- 1478,1487 /* CCI because we already updated the pg_class row in this command */ CommandCounterIncrement(); SetRelationHasSubclass(RelationGetRelid(onerel), false); + ereport(elevel, + (errmsg(skipping analyze of \%s.%s\ inheritance tree --- this inheritance tree contains no child tables, + get_namespace_name(RelationGetNamespace(onerel)), + RelationGetRelationName(onerel; return 0; } -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/09/12 16:30), Etsuro Fujita wrote: (2014/09/11 20:51), Heikki Linnakangas wrote: On 09/11/2014 02:30 PM, Etsuro Fujita wrote: So, should I split the patch into the two? Yeah, please do. OK, Will do. Here are separated patches. fdw-chk.patch - CHECK constraints on foreign tables fdw-inh.patch - table inheritance with foreign tables The latter has been created on top of [1]. I've addressed the comment from Horiguchi-san [2] in it also, though I've slightly modified his proposal. There is the functionality for path reparameterization for ForeignScan in the previous patch, but since the functionality would be useful not only for such table inheritance cases but for UNION ALL cases, I'd add the functionality as an independent feature maybe to CF 2014-12. Thanks, [1] http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp [2] http://www.postgresql.org/message-id/20140902.142218.253402812.horiguchi.kyot...@lab.ntt.co.jp Best regards, Etsuro Fujita *** a/contrib/file_fdw/input/file_fdw.source --- b/contrib/file_fdw/input/file_fdw.source *** *** 134,139 DELETE FROM agg_csv WHERE a = 100; --- 134,149 -- but this should be ignored SELECT * FROM agg_csv FOR UPDATE; + -- constraint exclusion tests + ALTER FOREIGN TABLE agg_csv ADD CONSTRAINT agg_csv_a_check CHECK (a = 0); + \t on + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a 0; + SET constraint_exclusion = 'on'; + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a 0; + SET constraint_exclusion = 'partition'; + \t off + ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check; + -- privilege tests SET ROLE file_fdw_superuser; SELECT * FROM agg_text ORDER BY a; *** a/contrib/file_fdw/output/file_fdw.source --- b/contrib/file_fdw/output/file_fdw.source *** *** 219,224 SELECT * FROM agg_csv FOR UPDATE; --- 219,242 42 | 324.78 (3 rows) + -- constraint exclusion tests + ALTER FOREIGN TABLE agg_csv ADD CONSTRAINT agg_csv_a_check CHECK (a = 0); + \t on + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a 0; + Foreign Scan on public.agg_csv +Output: a, b +Filter: (agg_csv.a 0) +Foreign File: @abs_srcdir@/data/agg.csv + + SET constraint_exclusion = 'on'; + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a 0; + Result +Output: a, b +One-Time Filter: false + + SET constraint_exclusion = 'partition'; + \t off + ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check; -- privilege tests SET ROLE file_fdw_superuser; SELECT * FROM agg_text ORDER BY a; *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 2488,2493 select c2, count(*) from S 1.T 1 where c2 500 group by 1 order by 1; --- 2488,2512 (13 rows) -- === + -- test constraint exclusion stuff + -- === + ALTER FOREIGN TABLE ft1 ADD CONSTRAINT ft1_c1_check CHECK (c1 0); + \t on + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM ft1 WHERE c1 = 0; + Foreign Scan on public.ft1 +Output: c1, c2, c3, c4, c5, c6, c7, c8 +Remote SQL: SELECT C 1, c2, c3, c4, c5, c6, c7, c8 FROM S 1.T 1 WHERE ((C 1 = 0)) + + SET constraint_exclusion = 'on'; + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM ft1 WHERE c1 = 0; + Result +Output: c1, c2, c3, c4, c5, c6, c7, c8 +One-Time Filter: false + + SET constraint_exclusion = 'partition'; + \t off + ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c1_check; + -- === -- test serial columns (ie, sequence-based defaults) -- === create table loc1 (f1 serial, f2 text); *** a/contrib/postgres_fdw/sql/postgres_fdw.sql --- b/contrib/postgres_fdw/sql/postgres_fdw.sql *** *** 387,392 select c2, count(*) from ft2 where c2 500 group by 1 order by 1; --- 387,404 select c2, count(*) from S 1.T 1 where c2 500 group by 1 order by 1; -- === + -- test constraint exclusion stuff + -- === + ALTER FOREIGN TABLE ft1 ADD CONSTRAINT ft1_c1_check CHECK (c1 0); + \t on + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM ft1 WHERE c1 = 0; + SET constraint_exclusion = 'on'; + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM ft1 WHERE c1 = 0; + SET constraint_exclusion = 'partition'; + \t off + ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c1_check; + + -- === -- test serial columns (ie, sequence-based defaults) -- === create table loc1 (f1 serial, f2 text); ***
Re: [HACKERS] inherit support for foreign tables
(2014/09/11 20:51), Heikki Linnakangas wrote: On 09/11/2014 02:30 PM, Etsuro Fujita wrote: So, should I split the patch into the two? Yeah, please do. OK, Will do. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/09/11 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
On 09/11/2014 12:22 PM, Etsuro Fujita wrote: (2014/09/11 4:32), Heikki Linnakangas wrote: I had a cursory look at this patch and the discussions around this. Thank you! ISTM there are actually two new features in this: 1) allow CHECK constraints on foreign tables, and 2) support inheritance for foreign tables. How about splitting it into two? That's right. There are the two in this patch. I'm not sure if I should split the patch into the two, because 1) won't make sense without 2). As described in the following note added to the docs on CREATE FOREIGN TABLE, CHECK constraints on foreign tables are intended to support constraint exclusion for partitioned foreign tables: + Constraints on foreign tables are not enforced on insert or update. + Those definitions simply declare the constraints hold for all rows + in the foreign tables. It is the user's responsibility to ensure + that those definitions match the remote side. Such constraints are + used for some kind of query optimization such as constraint exclusion + for partitioned tables The planner can do constraint exclusion based on CHECK constraints even without inheritance. It's not enabled by default because it can increase planning time, but if you set constraint_exclusion=on, it will work. For example: postgres=# create table foo (i int4 CHECK (i 0)); CREATE TABLE postgres=# explain select * from foo WHERE i 0; QUERY PLAN -- Seq Scan on foo (cost=0.00..40.00 rows=800 width=4) Filter: (i 0) Planning time: 0.335 ms (3 rows) postgres=# show constraint_exclusion ; constraint_exclusion -- partition (1 row) postgres=# set constraint_exclusion ='on'; SET postgres=# explain select * from foo WHERE i 0; QUERY PLAN -- Result (cost=0.00..0.01 rows=1 width=0) One-Time Filter: false Planning time: 0.254 ms (3 rows) postgres=# - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/09/11 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
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
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
Hello, I have a request with slight significance for the messages. I'd like to address this by emitting the second message as shown below: INFO: analyzing public.parent INFO: parent: scanned 0 of 0 pages, containing 0 live rows and 0 dead rows; 0 rows in sample, 0 estimated total rows INFO: analyzing public.parent inheritance tree INFO: skipping analyze of public.parent inheritance tree --- this inheritance tree contains foreign tables In acquire_inherited_sample_rows(), the message below is emitted when the parent explicitly specified in analyze command has at least one foreign tables. skipping analyze of \%s.%s\ inheritance tree --- this inheritance tree contains foreign tables This message implicitly asserts (for me) that A inheritance tree containing at least one foreign tables *always* cannot be analyzed but in reality, we can let it go by specifying the parent table explicitly. For example, the additional HINT or DETAIL message would clarify that. INFO: analyzing public.parent inheritance tree INFO: skipping analyze of public.parent inheritance tree --- this inheritance tree contains foreign tables + HINT: You can analyze this inheritance tree by specifying public.parent to analze command -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/08/22 11:51), Noah Misch wrote:
On Wed, Aug 20, 2014 at 08:11:01PM +0900, Etsuro Fujita wrote:
(2014/07/02 11:23), Noah Misch wrote:
Your chosen ANALYZE behavior is fair, but the messaging from a database-wide
ANALYZE VERBOSE needs work:
INFO: analyzing test_foreign_inherit.parent
INFO: parent: scanned 1 of 1 pages, containing 1 live rows and 0 dead rows;
1 rows in sample, 1 estimated total rows
INFO: analyzing test_foreign_inherit.parent inheritance tree
Please arrange to omit the 'analyzing tablename inheritance tree' message,
since this ANALYZE actually skipped that task.
I think it would be better that this is handled in the same way as
an inheritance tree that turns out to be a singe table that doesn't
have any descendants in acquire_inherited_sample_rows(). That would
still output the message as shown below, but I think that that would
be more consistent with the existing code. The patch works so.
Today's ANALYZE VERBOSE messaging for former inheritance parents (tables with
relhassubclass = true but no pg_inherits.inhparent links) is deceptive, and I
welcome a fix to omit the spurious message. As defects go, this is quite
minor. There's fundamentally no value in collecting inheritance tree
statistics for such a parent, and no PostgreSQL command will do so.
Your proposed behavior for inheritance parents having at least one foreign
table child is more likely to mislead DBAs in practice. An inheritance tree
genuinely exists, and a different ANALYZE command is quite capable of
collecting statistics on that inheritance tree. Messaging should reflect the
difference between ANALYZE invocations that do such work and ANALYZE
invocations that skip it. I'm anticipating a bug report along these lines:
I saw poor estimates involving a child foreign table, so I ran ANALYZE
VERBOSE, which reported 'INFO: analyzing public.parent inheritance
tree'. Estimates remained poor, so I scratched my head for awhile before
noticing that pg_stats didn't report such statistics. I then ran ANALYZE
VERBOSE parent, saw the same 'INFO: analyzing public.parent inheritance
tree', but this time saw relevant rows in pg_stats. The message doesn't
reflect the actual behavior.
I'll sympathize with that complaint. It's a minor point overall, but the code
impact is, I predict, small enough that we may as well get it right. A
credible alternative is to emit a second message indicating that we skipped
the inheritance tree statistics after all, and why we skipped them.
I'd like to address this by emitting the second message as shown below:
INFO: analyzing public.parent
INFO: parent: scanned 0 of 0 pages, containing 0 live rows and 0 dead
rows; 0 rows in sample, 0 estimated total rows
INFO: analyzing public.parent inheritance tree
INFO: skipping analyze of public.parent inheritance tree --- this
inheritance tree contains foreign tables
Attached is the update version of the patch. Based on the result of
discussions about attr_needed upthread, I've changed the patch so that
create_foreignscan_plan makes reference to reltargetlist, not to
attr_needed. (So, the patch in [1] isn't required, anymore.)
Other changes:
* Revise code/comments/docs a bit
* Add more regression tests
A separate patch (analyze.patch) handles the former case in a similar way.
[1] http://www.postgresql.org/message-id/53f4707c.8030...@lab.ntt.co.jp
Thanks,
Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***
*** 115,120 static void fileGetForeignRelSize(PlannerInfo *root,
--- 115,125
static void fileGetForeignPaths(PlannerInfo *root,
RelOptInfo *baserel,
Oid foreigntableid);
+ static ForeignPath *fileReparameterizeForeignPath(PlannerInfo *root,
+ RelOptInfo *baserel,
+ Oid foreigntableid,
+ ForeignPath *path,
+ Relids required_outer);
static ForeignScan *fileGetForeignPlan(PlannerInfo *root,
RelOptInfo *baserel,
Oid foreigntableid,
***
*** 143,149 static bool check_selective_binary_conversion(RelOptInfo *baserel,
static void estimate_size(PlannerInfo *root, RelOptInfo *baserel,
FileFdwPlanState *fdw_private);
static void estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
! FileFdwPlanState *fdw_private,
Cost *startup_cost, Cost *total_cost);
static int file_acquire_sample_rows(Relation onerel, int elevel,
HeapTuple *rows, int targrows,
--- 148,154
static void estimate_size(PlannerInfo *root, RelOptInfo *baserel,
FileFdwPlanState *fdw_private);
static void estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
! FileFdwPlanState *fdw_private, List *join_conds,
Cost *startup_cost, Cost *total_cost);
static int file_acquire_sample_rows(Relation onerel, int elevel,
HeapTuple *rows, int targrows,
***
*** 161,166
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
Hi Fujita-san,
I reviewed the v4 patch, and here are some comments from me.
* After applying this patch, pull_varattnos() should not called in
unnecessary places. For developers who want list of
columns-to-be-processed for some purpose, it would be nice to mention
when they should use pull_varattnos() and when they should not, maybe
at the comments of pull_varattnos() implementation.
* deparseReturningList() and postgresGetForeignRelSize() in
contrib/postgres_fdw/ also call pull_varattnos() to determine which
column to be in the SELECT clause of remote query. Shouldn't these be
replaced in the same manner? Other pull_varattnos() calls are for
restrictions, so IIUC they can't be replaced.
* Through this review I thought up that lazy evaluation approach might
fit attr_needed. I mean that we leave attr_needed for child relations
empty, and fill it at the first request for it. This would avoid
useless computation of attr_needed for child relations, though this
idea has been considered but thrown away before...
2014-08-20 18:55 GMT+09:00 Etsuro Fujita fujita.ets...@lab.ntt.co.jp:
Hi Ashutish,
(2014/08/14 22:30), Ashutosh Bapat wrote:
On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
(2014/08/08 18:51), Etsuro Fujita wrote:
(2014/06/30 22:48), Tom Lane wrote:
I wonder whether it isn't time to change that. It was coded
like that
originally only because calculating the values would've been a
waste of
cycles at the time. But this is at least the third place
where it'd be
useful to have attr_needed for child rels.
I've revised the patch.
There was a problem with the previous patch, which will be described
below. Attached is the updated version of the patch addressing that.
Here are some more comments
Thank you for the further review!
attr_needed also has the attributes used in the restriction clauses as
seen in distribute_qual_to_rels(), so, it looks unnecessary to call
pull_varattnos() on the clauses in baserestrictinfo in functions
check_selective_binary_conversion(), remove_unused_subquery_outputs(),
check_index_only().
IIUC, I think it's *necessary* to do that in those functions since the
attributes used in the restriction clauses in baserestrictinfo are not added
to attr_needed due the following code in distribute_qual_to_rels.
/*
* If it's a join clause (either naturally, or because delayed by
* outer-join rules), add vars used in the clause to targetlists of
their
* relations, so that they will be emitted by the plan nodes that scan
* those relations (else they won't be available at the join node!).
*
* Note: if the clause gets absorbed into an EquivalenceClass then this
* may be unnecessary, but for now we have to do it to cover the case
* where the EC becomes ec_broken and we end up reinserting the original
* clauses into the plan.
*/
if (bms_membership(relids) == BMS_MULTIPLE)
{
List *vars = pull_var_clause(clause,
PVC_RECURSE_AGGREGATES,
PVC_INCLUDE_PLACEHOLDERS);
add_vars_to_targetlist(root, vars, relids, false);
list_free(vars);
}
Although in case of RTE_RELATION, the 0th entry in attr_needed
corresponds to FirstLowInvalidHeapAttributeNumber + 1, it's always safer
to use it is RelOptInfo::min_attr, in case get_relation_info() wants to
change assumption or somewhere down the line some other part of code
wants to change attr_needed information. It may be unlikely, that it
would change, but it will be better to stick to comments in RelOptInfo
443 AttrNumber min_attr; /* smallest attrno of rel (often
0) */
444 AttrNumber max_attr; /* largest attrno of rel */
445 Relids *attr_needed;/* array indexed [min_attr ..
max_attr] */
Good point! Attached is the revised version of the patch.
Thanks,
Best regards,
Etsuro Fujita
--
Shigeru HANADA
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
[ attr_needed-v4.patch ]
I looked this over, and TBH I'm rather disappointed. The patch adds
150 lines of dubiously-correct code in order to save ... uh, well,
actually it *adds* code, because the places that are supposedly getting
a benefit are changed like this:
*** 799,806 check_selective_binary_conversion(RelOptInfo *baserel,
}
/* Collect all the attributes needed for joins or final output. */
! pull_varattnos((Node *) baserel-reltargetlist, baserel-relid,
! attrs_used);
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)
--- 799,810
}
/* Collect all the attributes needed for joins or final output. */
! for (i = baserel-min_attr; i = baserel-max_attr; i++)
! {
! if (!bms_is_empty(baserel-attr_needed[i - baserel-min_attr]))
! attrs_used = bms_add_member(attrs_used,
!
i - FirstLowInvalidHeapAttributeNumber);
! }
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)
That's not simpler, it's not easier to understand, and it's probably not
faster either. We could address some of those complaints by encapsulating
the above loop into a utility function, but still, I come away with the
feeling that it's not worth changing this. Considering that all the
places that are doing this then proceed to use pull_varattnos to add on
attnos from the restriction clauses, it seems like using pull_varattnos
on the reltargetlist isn't such a bad thing after all.
So I'm inclined to reject this. It seemed like a good idea in the
abstract, but the concrete result isn't very attractive, and doesn't
seem like an improvement over what we have.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
(2014/08/27 3:27), Tom Lane wrote: Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: [ attr_needed-v4.patch ] I looked this over, and TBH I'm rather disappointed. The patch adds 150 lines of dubiously-correct code in order to save ... uh, well, Just for my study, could you tell me why you think that the code is dubiously-correct? Considering that all the places that are doing this then proceed to use pull_varattnos to add on attnos from the restriction clauses, it seems like using pull_varattnos on the reltargetlist isn't such a bad thing after all. I agree with you on that point. So I'm inclined to reject this. It seemed like a good idea in the abstract, but the concrete result isn't very attractive, and doesn't seem like an improvement over what we have. Okay. I'll withdraw the patch. Thank you for taking the time to review the patch! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: (2014/08/27 3:27), Tom Lane wrote: I looked this over, and TBH I'm rather disappointed. The patch adds 150 lines of dubiously-correct code in order to save ... uh, well, Just for my study, could you tell me why you think that the code is dubiously-correct? It might be fine; I did not actually review the new adjust_appendrel_attr_needed code in any detail. What's scaring me off it is (1) it's a lot longer and more complicated than I'd thought it would be, and (2) you already made several bug fixes in it, which is often an indicator that additional problems lurk. It's possible there's some other, simpler, way to compute child attr_needed arrays that would resolve (1) and (2). However, even if we had a simple and obviously-correct way to do that, it still seems like there's not very much benefit to be had after all. So my thought that this would be worth doing seems wrong, and I must apologize to you for sending you chasing down a dead end :-( regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
(2014/08/27 11:06), Tom Lane wrote: Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: (2014/08/27 3:27), Tom Lane wrote: I looked this over, and TBH I'm rather disappointed. The patch adds 150 lines of dubiously-correct code in order to save ... uh, well, Just for my study, could you tell me why you think that the code is dubiously-correct? It might be fine; I did not actually review the new adjust_appendrel_attr_needed code in any detail. What's scaring me off it is (1) it's a lot longer and more complicated than I'd thought it would be, and (2) you already made several bug fixes in it, which is often an indicator that additional problems lurk. Okay. It's possible there's some other, simpler, way to compute child attr_needed arrays that would resolve (1) and (2). However, even if we had a simple and obviously-correct way to do that, it still seems like there's not very much benefit to be had after all. So my thought that this would be worth doing seems wrong, and I must apologize to you for sending you chasing down a dead end :-( Please don't worry yourself about that! Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/08/22 12:58), Alvaro Herrera wrote: Noah Misch wrote: I'm anticipating a bug report along these lines: I saw poor estimates involving a child foreign table, so I ran ANALYZE VERBOSE, which reported 'INFO: analyzing public.parent inheritance tree'. Estimates remained poor, so I scratched my head for awhile before noticing that pg_stats didn't report such statistics. I then ran ANALYZE VERBOSE parent, saw the same 'INFO: analyzing public.parent inheritance tree', but this time saw relevant rows in pg_stats. The message doesn't reflect the actual behavior. I'll sympathize with that complaint. Agreed on that. I've got the point. Will fix. Thanks for the comment! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
(2014/08/21 13:21), Ashutosh Bapat wrote: On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: Hi Ashutish, I am sorry that I mistook your name's spelling. (2014/08/14 22:30), Ashutosh Bapat wrote: On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.__co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: (2014/08/08 18:51), Etsuro Fujita wrote: (2014/06/30 22:48), Tom Lane wrote: I wonder whether it isn't time to change that. It was coded like that originally only because calculating the values would've been a waste of cycles at the time. But this is at least the third place where it'd be useful to have attr_needed for child rels. There was a problem with the previous patch, which will be described below. Attached is the updated version of the patch addressing that. Here are some more comments attr_needed also has the attributes used in the restriction clauses as seen in distribute_qual_to_rels(), so, it looks unnecessary to call pull_varattnos() on the clauses in baserestrictinfo in functions check_selective_binary___conversion(), remove_unused_subquery___outputs(), check_index_only(). IIUC, I think it's *necessary* to do that in those functions since the attributes used in the restriction clauses in baserestrictinfo are not added to attr_needed due the following code in distribute_qual_to_rels. That's right. Thanks for pointing that out. Although in case of RTE_RELATION, the 0th entry in attr_needed corresponds to FirstLowInvalidHeapAttributeNu__mber + 1, it's always safer to use it is RelOptInfo::min_attr, in case get_relation_info() wants to change assumption or somewhere down the line some other part of code wants to change attr_needed information. It may be unlikely, that it would change, but it will be better to stick to comments in RelOptInfo 443 AttrNumber min_attr; /* smallest attrno of rel (often 0) */ 444 AttrNumber max_attr; /* largest attrno of rel */ 445 Relids *attr_needed;/* array indexed [min_attr .. max_attr] */ Good point! Attached is the revised version of the patch. If the patch is not in the commit-fest, can you please add it there? I've already done that: https://commitfest.postgresql.org/action/patch_view?id=1529 From my side, the review is done, it should be marked ready for committer, unless somebody else wants to review. Many thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/07/02 11:23), Noah Misch wrote: On Fri, Jun 20, 2014 at 05:04:06PM +0900, Kyotaro HORIGUCHI wrote: Attached is the rebased patch of v11 up to the current master. The rest of these review comments are strictly observations; they're not requests for you to change the patch, but they may deserve more discussion. We use the term child table in many messages. Should that change to something more inclusive, now that the child may be a foreign table? Perhaps one of child relation, plain child, or child foreign table/child table depending on the actual object? child relation is robust technically, but it might add more confusion than it removes. Varying the message depending on the actual object feels like a waste. Opinions? IMHO, I think that child table would not confusing users terribly. LOCK TABLE on the inheritance parent locks child foreign tables, but LOCK TABLE fails when given a foreign table directly. That's odd, but I see no cause to change it. I agree wth that. The longstanding behavior of CREATE TABLE INHERITS is to reorder local columns to match the order found in parents. That is, both of these tables actually have columns in the order (a,b): create table parent (a int, b int); create table child (b int, a int) inherits (parent); Ordinary dump/reload always uses CREATE TABLE INHERITS, thereby changing column order in this way. (pg_dump --binary-upgrade uses ALTER TABLE INHERIT and some catalog hacks to avoid doing so.) I've never liked that dump/reload can change column order, but it's tolerable if you follow the best practice of always writing out column lists. The stakes rise for foreign tables, because column order is inherently significant to file_fdw and probably to certain other non-RDBMS FDWs. If pg_dump changes column order in your file_fdw foreign table, the table breaks. I would heartily support making pg_dump preserve column order for all inheritance children. That doesn't rise to the level of being a blocker for this patch, though. I agree with that, too. I think it would be better to add docs for now. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
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
Noah Misch wrote: I'm anticipating a bug report along these lines: I saw poor estimates involving a child foreign table, so I ran ANALYZE VERBOSE, which reported 'INFO: analyzing public.parent inheritance tree'. Estimates remained poor, so I scratched my head for awhile before noticing that pg_stats didn't report such statistics. I then ran ANALYZE VERBOSE parent, saw the same 'INFO: analyzing public.parent inheritance tree', but this time saw relevant rows in pg_stats. The message doesn't reflect the actual behavior. I'll sympathize with that complaint. Agreed on that. A credible alternative is to emit a second message indicating that we skipped the inheritance tree statistics after all, and why we skipped them. That'd be similar to Xorg emitting messages such as [53.772] (II) intel(0): RandR 1.2 enabled, ignore the following RandR disabled message. [53.800] (--) RandR disabled I find this very poor. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
On Thu, Aug 21, 2014 at 3:00 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/08/21 13:21), Ashutosh Bapat wrote: On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: Hi Ashutish, I am sorry that I mistook your name's spelling. (2014/08/14 22:30), Ashutosh Bapat wrote: On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.__co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: (2014/08/08 18:51), Etsuro Fujita wrote: (2014/06/30 22:48), Tom Lane wrote: I wonder whether it isn't time to change that. It was coded like that originally only because calculating the values would've been a waste of cycles at the time. But this is at least the third place where it'd be useful to have attr_needed for child rels. There was a problem with the previous patch, which will be described below. Attached is the updated version of the patch addressing that. Here are some more comments attr_needed also has the attributes used in the restriction clauses as seen in distribute_qual_to_rels(), so, it looks unnecessary to call pull_varattnos() on the clauses in baserestrictinfo in functions check_selective_binary___conversion(), remove_unused_subquery___outputs(), check_index_only(). IIUC, I think it's *necessary* to do that in those functions since the attributes used in the restriction clauses in baserestrictinfo are not added to attr_needed due the following code in distribute_qual_to_rels. That's right. Thanks for pointing that out. Although in case of RTE_RELATION, the 0th entry in attr_needed corresponds to FirstLowInvalidHeapAttributeNu__mber + 1, it's always safer to use it is RelOptInfo::min_attr, in case get_relation_info() wants to change assumption or somewhere down the line some other part of code wants to change attr_needed information. It may be unlikely, that it would change, but it will be better to stick to comments in RelOptInfo 443 AttrNumber min_attr; /* smallest attrno of rel (often 0) */ 444 AttrNumber max_attr; /* largest attrno of rel */ 445 Relids *attr_needed;/* array indexed [min_attr .. max_attr] */ Good point! Attached is the revised version of the patch. If the patch is not in the commit-fest, can you please add it there? I've already done that: https://commitfest.postgresql.org/action/patch_view?id=1529 From my side, the review is done, it should be marked ready for committer, unless somebody else wants to review. Many thanks! Thanks. Since, I haven't seen anybody else commenting here and I do not have any further comments to make, I have marked it as ready for committer. Best regards, Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
Hi Ashutish,
(2014/08/14 22:30), Ashutosh Bapat wrote:
On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
(2014/08/08 18:51), Etsuro Fujita wrote:
(2014/06/30 22:48), Tom Lane wrote:
I wonder whether it isn't time to change that. It was coded
like that
originally only because calculating the values would've been a
waste of
cycles at the time. But this is at least the third place
where it'd be
useful to have attr_needed for child rels.
I've revised the patch.
There was a problem with the previous patch, which will be described
below. Attached is the updated version of the patch addressing that.
Here are some more comments
Thank you for the further review!
attr_needed also has the attributes used in the restriction clauses as
seen in distribute_qual_to_rels(), so, it looks unnecessary to call
pull_varattnos() on the clauses in baserestrictinfo in functions
check_selective_binary_conversion(), remove_unused_subquery_outputs(),
check_index_only().
IIUC, I think it's *necessary* to do that in those functions since the
attributes used in the restriction clauses in baserestrictinfo are not
added to attr_needed due the following code in distribute_qual_to_rels.
/*
* If it's a join clause (either naturally, or because delayed by
* outer-join rules), add vars used in the clause to targetlists of
their
* relations, so that they will be emitted by the plan nodes that scan
* those relations (else they won't be available at the join node!).
*
* Note: if the clause gets absorbed into an EquivalenceClass then this
* may be unnecessary, but for now we have to do it to cover the case
* where the EC becomes ec_broken and we end up reinserting the
original
* clauses into the plan.
*/
if (bms_membership(relids) == BMS_MULTIPLE)
{
List *vars = pull_var_clause(clause,
PVC_RECURSE_AGGREGATES,
PVC_INCLUDE_PLACEHOLDERS);
add_vars_to_targetlist(root, vars, relids, false);
list_free(vars);
}
Although in case of RTE_RELATION, the 0th entry in attr_needed
corresponds to FirstLowInvalidHeapAttributeNumber + 1, it's always safer
to use it is RelOptInfo::min_attr, in case get_relation_info() wants to
change assumption or somewhere down the line some other part of code
wants to change attr_needed information. It may be unlikely, that it
would change, but it will be better to stick to comments in RelOptInfo
443 AttrNumber min_attr; /* smallest attrno of rel (often
0) */
444 AttrNumber max_attr; /* largest attrno of rel */
445 Relids *attr_needed;/* array indexed [min_attr ..
max_attr] */
Good point! Attached is the revised version of the patch.
Thanks,
Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***
*** 799,806 check_selective_binary_conversion(RelOptInfo *baserel,
}
/* Collect all the attributes needed for joins or final output. */
! pull_varattnos((Node *) baserel-reltargetlist, baserel-relid,
! attrs_used);
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)
--- 799,810
}
/* Collect all the attributes needed for joins or final output. */
! for (i = baserel-min_attr; i = baserel-max_attr; i++)
! {
! if (!bms_is_empty(baserel-attr_needed[i - baserel-min_attr]))
! attrs_used = bms_add_member(attrs_used,
! i - FirstLowInvalidHeapAttributeNumber);
! }
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
***
*** 577,588 set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
childrel-has_eclass_joins = rel-has_eclass_joins;
/*
! * Note: we could compute appropriate attr_needed data for the child's
! * variables, by transforming the parent's attr_needed through the
! * translated_vars mapping. However, currently there's no need
! * because attr_needed is only examined for base relations not
! * otherrels. So we just leave the child's attr_needed empty.
*/
/*
* Compute the child's size.
--- 577,585
childrel-has_eclass_joins = rel-has_eclass_joins;
/*
! * Compute the child's attr_needed.
*/
+ adjust_appendrel_attr_needed(rel, childrel, appinfo);
/*
* Compute the child's size.
***
*** 2173,2178 remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
--- 2170,2176
{
Bitmapset *attrs_used = NULL;
ListCell *lc;
+ int i;
/*
* Do nothing if subquery has UNION/INTERSECT/EXCEPT: in principle we
***
***
Re: [HACKERS] inherit support for foreign tables
Hi Noah,
Thank you for the review!
(2014/07/02 11:23), Noah Misch wrote:
On Fri, Jun 20, 2014 at 05:04:06PM +0900, Kyotaro HORIGUCHI wrote:
Attached is the rebased patch of v11 up to the current master.
I've been studying this patch.
SELECT FOR UPDATE on the inheritance parent fails with a can't-happen error
condition, even when SELECT FOR UPDATE on the child foreign table alone would
have succeeded.
To fix this, I've modified the planner and executor so that the planner
adds wholerow as well as ctid and tableoid as resjunk output columns to
the plan for an inheritance tree that contains foreign tables, and that
while the executor uses the ctid and tableoid in the EPQ processing for
child regular tables as before, the executor uses the wholerow and
tableoid for child foreign tables. Patch attached. This is based on
the patch [1].
The patch adds zero tests. Add principal tests to postgres_fdw.sql. Also,
create a child foreign table in foreign_data.sql; this will make dump/reload
tests of the regression database exercise an inheritance tree that includes a
foreign table.
Done. I used your tests as reference. Thanks!
The inheritance section of ddl.sgml should mention child foreign tables, at
least briefly. Consider mentioning it in the partitioning section, too.
Done.
Your chosen ANALYZE behavior is fair, but the messaging from a database-wide
ANALYZE VERBOSE needs work:
INFO: analyzing test_foreign_inherit.parent
INFO: parent: scanned 1 of 1 pages, containing 1 live rows and 0 dead rows;
1 rows in sample, 1 estimated total rows
INFO: analyzing test_foreign_inherit.parent inheritance tree
WARNING: relcache reference leak: relation child not closed
WARNING: relcache reference leak: relation tchild not closed
WARNING: relcache reference leak: relation parent not closed
Please arrange to omit the 'analyzing tablename inheritance tree' message,
since this ANALYZE actually skipped that task. (The warnings obviously need a
fix, too.) I do find it awkward that adding a foreign table to an inheritance
tree will make autovacuum stop collecting statistics on that inheritance tree,
but I can't think of a better policy.
I think it would be better that this is handled in the same way as an
inheritance tree that turns out to be a singe table that doesn't have
any descendants in acquire_inherited_sample_rows(). That would still
output the message as shown below, but I think that that would be more
consistent with the existing code. The patch works so. (The warnings
are also fixed.)
INFO: analyzing public.parent
INFO: parent: scanned 0 of 0 pages, containing 0 live rows and 0 dead
rows; 0 rows in sample, 0 estimated total rows
INFO: analyzing public.parent inheritance tree
INFO: analyzing pg_catalog.pg_authid
INFO: pg_authid: scanned 1 of 1 pages, containing 1 live rows and 0
dead rows; 1 rows in sample, 1 estimated total rows
The rest of these review comments are strictly observations; they're not
requests for you to change the patch, but they may deserve more discussion.
I'd like to give my opinions on those things later on.
Sorry for the long delay.
[1] http://www.postgresql.org/message-id/53f4707c.8030...@lab.ntt.co.jp
Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***
*** 115,120 static void fileGetForeignRelSize(PlannerInfo *root,
--- 115,125
static void fileGetForeignPaths(PlannerInfo *root,
RelOptInfo *baserel,
Oid foreigntableid);
+ static ForeignPath *fileReparameterizeForeignPath(PlannerInfo *root,
+ RelOptInfo *baserel,
+ Oid foreigntableid,
+ ForeignPath *path,
+ Relids required_outer);
static ForeignScan *fileGetForeignPlan(PlannerInfo *root,
RelOptInfo *baserel,
Oid foreigntableid,
***
*** 143,149 static bool check_selective_binary_conversion(RelOptInfo *baserel,
static void estimate_size(PlannerInfo *root, RelOptInfo *baserel,
FileFdwPlanState *fdw_private);
static void estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
! FileFdwPlanState *fdw_private,
Cost *startup_cost, Cost *total_cost);
static int file_acquire_sample_rows(Relation onerel, int elevel,
HeapTuple *rows, int targrows,
--- 148,154
static void estimate_size(PlannerInfo *root, RelOptInfo *baserel,
FileFdwPlanState *fdw_private);
static void estimate_costs(PlannerInfo *root, RelOptInfo *baserel,
! FileFdwPlanState *fdw_private, List *join_conds,
Cost *startup_cost, Cost *total_cost);
static int file_acquire_sample_rows(Relation onerel, int elevel,
HeapTuple *rows, int targrows,
***
*** 161,166 file_fdw_handler(PG_FUNCTION_ARGS)
--- 166,172
fdwroutine-GetForeignRelSize = fileGetForeignRelSize;
fdwroutine-GetForeignPaths = fileGetForeignPaths;
+ fdwroutine-ReparameterizeForeignPath =
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
On Wed, Aug 20, 2014 at 3:25 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp
wrote:
Hi Ashutish,
(2014/08/14 22:30), Ashutosh Bapat wrote:
On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote:
(2014/08/08 18:51), Etsuro Fujita wrote:
(2014/06/30 22:48), Tom Lane wrote:
I wonder whether it isn't time to change that. It was coded
like that
originally only because calculating the values would've been a
waste of
cycles at the time. But this is at least the third place
where it'd be
useful to have attr_needed for child rels.
I've revised the patch.
There was a problem with the previous patch, which will be described
below. Attached is the updated version of the patch addressing that.
Here are some more comments
Thank you for the further review!
attr_needed also has the attributes used in the restriction clauses as
seen in distribute_qual_to_rels(), so, it looks unnecessary to call
pull_varattnos() on the clauses in baserestrictinfo in functions
check_selective_binary_conversion(), remove_unused_subquery_outputs(),
check_index_only().
IIUC, I think it's *necessary* to do that in those functions since the
attributes used in the restriction clauses in baserestrictinfo are not
added to attr_needed due the following code in distribute_qual_to_rels.
That's right. Thanks for pointing that out.
/*
* If it's a join clause (either naturally, or because delayed by
* outer-join rules), add vars used in the clause to targetlists of
their
* relations, so that they will be emitted by the plan nodes that scan
* those relations (else they won't be available at the join node!).
*
* Note: if the clause gets absorbed into an EquivalenceClass then this
* may be unnecessary, but for now we have to do it to cover the case
* where the EC becomes ec_broken and we end up reinserting the
original
* clauses into the plan.
*/
if (bms_membership(relids) == BMS_MULTIPLE)
{
List *vars = pull_var_clause(clause,
PVC_RECURSE_AGGREGATES,
PVC_INCLUDE_PLACEHOLDERS);
add_vars_to_targetlist(root, vars, relids, false);
list_free(vars);
}
Although in case of RTE_RELATION, the 0th entry in attr_needed
corresponds to FirstLowInvalidHeapAttributeNumber + 1, it's always safer
to use it is RelOptInfo::min_attr, in case get_relation_info() wants to
change assumption or somewhere down the line some other part of code
wants to change attr_needed information. It may be unlikely, that it
would change, but it will be better to stick to comments in RelOptInfo
443 AttrNumber min_attr; /* smallest attrno of rel (often
0) */
444 AttrNumber max_attr; /* largest attrno of rel */
445 Relids *attr_needed;/* array indexed [min_attr ..
max_attr] */
Good point! Attached is the revised version of the patch.
If the patch is not in the commit-fest, can you please add it there? From
my side, the review is done, it should be marked ready for committer,
unless somebody else wants to review.
Thanks,
Best regards,
Etsuro Fujita
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Re: Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
Hi, On Thu, Aug 14, 2014 at 10:05 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/08/08 18:51), Etsuro Fujita wrote: (2014/06/30 22:48), Tom Lane wrote: I wonder whether it isn't time to change that. It was coded like that originally only because calculating the values would've been a waste of cycles at the time. But this is at least the third place where it'd be useful to have attr_needed for child rels. I've revised the patch. There was a problem with the previous patch, which will be described below. Attached is the updated version of the patch addressing that. The previous patch doesn't cope with some UNION ALL cases properly. So, e.g., the server will crash for the following query: postgres=# create table ta1 (f1 int); CREATE TABLE postgres=# create table ta2 (f2 int primary key, f3 int); CREATE TABLE postgres=# create table tb1 (f1 int); CREATE TABLE postgres=# create table tb2 (f2 int primary key, f3 int); CREATE TABLE postgres=# explain verbose select f1 from ((select f1, f2 from (select f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all (select f1, f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1) ssb)) ss; With the updated version, we get the right result: postgres=# explain verbose select f1 from ((select f1, f2 from (select f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all (select f1, f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1) ssb)) ss; QUERY PLAN Append (cost=0.00..0.05 rows=2 width=4) - Subquery Scan on ssa (cost=0.00..0.02 rows=1 width=4) Output: ssa.f1 - Limit (cost=0.00..0.01 rows=1 width=4) Output: ta1.f1, (NULL::integer), (NULL::integer) - Seq Scan on public.ta1 (cost=0.00..34.00 rows=2400 width=4) Output: ta1.f1, NULL::integer, NULL::integer - Subquery Scan on ssb (cost=0.00..0.02 rows=1 width=4) Output: ssb.f1 - Limit (cost=0.00..0.01 rows=1 width=4) Output: tb1.f1, (NULL::integer), (NULL::integer) - Seq Scan on public.tb1 (cost=0.00..34.00 rows=2400 width=4) Output: tb1.f1, NULL::integer, NULL::integer Planning time: 0.453 ms (14 rows) While thinking to address this problem, Ashutosh also expressed concern about the UNION ALL handling in the previous patch in a private email. Thank you for the review, Ashutosh! Thanks for taking care of this one. Here are some more comments attr_needed also has the attributes used in the restriction clauses as seen in distribute_qual_to_rels(), so, it looks unnecessary to call pull_varattnos() on the clauses in baserestrictinfo in functions check_selective_binary_conversion(), remove_unused_subquery_outputs(), check_index_only(). Although in case of RTE_RELATION, the 0th entry in attr_needed corresponds to FirstLowInvalidHeapAttributeNumber + 1, it's always safer to use it is RelOptInfo::min_attr, in case get_relation_info() wants to change assumption or somewhere down the line some other part of code wants to change attr_needed information. It may be unlikely, that it would change, but it will be better to stick to comments in RelOptInfo 443 AttrNumber min_attr; /* smallest attrno of rel (often 0) */ 444 AttrNumber max_attr; /* largest attrno of rel */ 445 Relids *attr_needed;/* array indexed [min_attr .. max_attr] */ Thanks, Best regards, Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Compute attr_needed for child relations (was Re: [HACKERS] inherit support for foreign tables)
(2014/08/08 18:51), Etsuro Fujita wrote:
(2014/06/30 22:48), Tom Lane wrote:
I wonder whether it isn't time to change that. It was coded like that
originally only because calculating the values would've been a waste of
cycles at the time. But this is at least the third place where it'd be
useful to have attr_needed for child rels.
I've revised the patch.
There was a problem with the previous patch, which will be described
below. Attached is the updated version of the patch addressing that.
The previous patch doesn't cope with some UNION ALL cases properly. So,
e.g., the server will crash for the following query:
postgres=# create table ta1 (f1 int);
CREATE TABLE
postgres=# create table ta2 (f2 int primary key, f3 int);
CREATE TABLE
postgres=# create table tb1 (f1 int);
CREATE TABLE
postgres=# create table tb2 (f2 int primary key, f3 int);
CREATE TABLE
postgres=# explain verbose select f1 from ((select f1, f2 from (select
f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all
(select f1,
f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1)
ssb)) ss;
With the updated version, we get the right result:
postgres=# explain verbose select f1 from ((select f1, f2 from (select
f1, f2, f3 from ta1 left join ta2 on f1 = f2 limit 1) ssa) union all
(select f1,
f2 from (select f1, f2, f3 from tb1 left join tb2 on f1 = f2 limit 1)
ssb)) ss;
QUERY PLAN
Append (cost=0.00..0.05 rows=2 width=4)
- Subquery Scan on ssa (cost=0.00..0.02 rows=1 width=4)
Output: ssa.f1
- Limit (cost=0.00..0.01 rows=1 width=4)
Output: ta1.f1, (NULL::integer), (NULL::integer)
- Seq Scan on public.ta1 (cost=0.00..34.00 rows=2400
width=4)
Output: ta1.f1, NULL::integer, NULL::integer
- Subquery Scan on ssb (cost=0.00..0.02 rows=1 width=4)
Output: ssb.f1
- Limit (cost=0.00..0.01 rows=1 width=4)
Output: tb1.f1, (NULL::integer), (NULL::integer)
- Seq Scan on public.tb1 (cost=0.00..34.00 rows=2400
width=4)
Output: tb1.f1, NULL::integer, NULL::integer
Planning time: 0.453 ms
(14 rows)
While thinking to address this problem, Ashutosh also expressed concern
about the UNION ALL handling in the previous patch in a private email.
Thank you for the review, Ashutosh!
Thanks,
Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***
*** 799,806 check_selective_binary_conversion(RelOptInfo *baserel,
}
/* Collect all the attributes needed for joins or final output. */
! pull_varattnos((Node *) baserel-reltargetlist, baserel-relid,
! attrs_used);
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)
--- 799,810
}
/* Collect all the attributes needed for joins or final output. */
! for (i = baserel-min_attr; i = baserel-max_attr; i++)
! {
! if (!bms_is_empty(baserel-attr_needed[i - baserel-min_attr]))
! attrs_used = bms_add_member(attrs_used,
! i - FirstLowInvalidHeapAttributeNumber);
! }
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
***
*** 577,588 set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
childrel-has_eclass_joins = rel-has_eclass_joins;
/*
! * Note: we could compute appropriate attr_needed data for the child's
! * variables, by transforming the parent's attr_needed through the
! * translated_vars mapping. However, currently there's no need
! * because attr_needed is only examined for base relations not
! * otherrels. So we just leave the child's attr_needed empty.
*/
/*
* Compute the child's size.
--- 577,585
childrel-has_eclass_joins = rel-has_eclass_joins;
/*
! * Compute the child's attr_needed.
*/
+ adjust_appendrel_attr_needed(rel, childrel, appinfo);
/*
* Compute the child's size.
***
*** 2173,2178 remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
--- 2170,2176
{
Bitmapset *attrs_used = NULL;
ListCell *lc;
+ int i;
/*
* Do nothing if subquery has UNION/INTERSECT/EXCEPT: in principle we
***
*** 2193,2204 remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
* Collect a bitmap of all the output column numbers used by the upper
* query.
*
! * Add all the attributes needed for joins or final output. Note: we must
! * look at reltargetlist, not the attr_needed data, because attr_needed
! * isn't computed for inheritance child rels, cf set_append_rel_size().
! * (XXX might be worth changing that sometime.)
*/
!
Re: [HACKERS] inherit support for foreign tables
(2014/08/06 20:43), Etsuro Fujita wrote:
(2014/06/30 22:48), Tom Lane wrote:
I wonder whether it isn't time to change that. It was coded like that
originally only because calculating the values would've been a waste of
cycles at the time. But this is at least the third place where it'd be
useful to have attr_needed for child rels.
Attached is a WIP patch for that.
I've revised the patch.
Changes:
* Make the code more readable
* Revise the comments
* Cleanup
Please find attached an updated version of the patch.
Thanks,
Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***
*** 799,806 check_selective_binary_conversion(RelOptInfo *baserel,
}
/* Collect all the attributes needed for joins or final output. */
! pull_varattnos((Node *) baserel-reltargetlist, baserel-relid,
! attrs_used);
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)
--- 799,811
}
/* Collect all the attributes needed for joins or final output. */
! for (i = baserel-min_attr; i = baserel-max_attr; i++)
! {
! if (!bms_is_empty(baserel-attr_needed[i - baserel-min_attr]))
! attrs_used =
! bms_add_member(attrs_used,
! i - FirstLowInvalidHeapAttributeNumber);
! }
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
***
*** 577,588 set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
childrel-has_eclass_joins = rel-has_eclass_joins;
/*
! * Note: we could compute appropriate attr_needed data for the child's
! * variables, by transforming the parent's attr_needed through the
! * translated_vars mapping. However, currently there's no need
! * because attr_needed is only examined for base relations not
! * otherrels. So we just leave the child's attr_needed empty.
*/
/*
* Compute the child's size.
--- 577,585
childrel-has_eclass_joins = rel-has_eclass_joins;
/*
! * Compute the child's attr_needed.
*/
+ adjust_appendrel_attr_needed(rel, childrel, childRTE, appinfo);
/*
* Compute the child's size.
***
*** 2173,2178 remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
--- 2170,2176
{
Bitmapset *attrs_used = NULL;
ListCell *lc;
+ int i;
/*
* Do nothing if subquery has UNION/INTERSECT/EXCEPT: in principle we
***
*** 2193,2204 remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
* Collect a bitmap of all the output column numbers used by the upper
* query.
*
! * Add all the attributes needed for joins or final output. Note: we must
! * look at reltargetlist, not the attr_needed data, because attr_needed
! * isn't computed for inheritance child rels, cf set_append_rel_size().
! * (XXX might be worth changing that sometime.)
*/
! pull_varattnos((Node *) rel-reltargetlist, rel-relid, attrs_used);
/* Add all the attributes used by un-pushed-down restriction clauses. */
foreach(lc, rel-baserestrictinfo)
--- 2191,2205
* Collect a bitmap of all the output column numbers used by the upper
* query.
*
! * Add all the attributes needed for joins or final output.
*/
! for (i = rel-min_attr; i = rel-max_attr; i++)
! {
! if (!bms_is_empty(rel-attr_needed[i - rel-min_attr]))
! attrs_used =
! bms_add_member(attrs_used,
! i - FirstLowInvalidHeapAttributeNumber);
! }
/* Add all the attributes used by un-pushed-down restriction clauses. */
foreach(lc, rel-baserestrictinfo)
*** a/src/backend/optimizer/path/indxpath.c
--- b/src/backend/optimizer/path/indxpath.c
***
*** 1768,1779 check_index_only(RelOptInfo *rel, IndexOptInfo *index)
* way out.
*/
! /*
! * Add all the attributes needed for joins or final output. Note: we must
! * look at reltargetlist, not the attr_needed data, because attr_needed
! * isn't computed for inheritance child rels.
! */
! pull_varattnos((Node *) rel-reltargetlist, rel-relid, attrs_used);
/* Add all the attributes used by restriction clauses. */
foreach(lc, rel-baserestrictinfo)
--- 1768,1781
* way out.
*/
! /* Add all the attributes needed for joins or final output. */
! for (i = rel-min_attr; i = rel-max_attr; i++)
! {
! if (!bms_is_empty(rel-attr_needed[i - rel-min_attr]))
! attrs_used =
! bms_add_member(attrs_used,
! i - FirstLowInvalidHeapAttributeNumber);
! }
/* Add all the attributes used by restriction clauses. */
foreach(lc, rel-baserestrictinfo)
*** a/src/backend/optimizer/prep/prepunion.c
--- b/src/backend/optimizer/prep/prepunion.c
***
*** 109,114 static Bitmapset *translate_col_privs(const Bitmapset *parent_privs,
--- 109,122
Re: [HACKERS] inherit support for foreign tables
(2014/07/01 11:10), Etsuro Fujita wrote:
(2014/06/30 22:48), Tom Lane wrote:
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes:
Done. I think this is because create_foreignscan_plan() makes reference
to attr_needed, which isn't computed for inheritance children.
I wonder whether it isn't time to change that. It was coded like that
originally only because calculating the values would've been a waste of
cycles at the time. But this is at least the third place where it'd be
useful to have attr_needed for child rels.
+1 for calculating attr_needed for child rels. (I was wondering too.)
I'll create a separate patch for it.
Attached is a WIP patch for that. The following functions have been
changed to refer to attr_needed:
* check_index_only()
* remove_unused_subquery_outputs()
* check_selective_binary_conversion()
I'll add this to the upcoming commitfest. If anyone has any time to
glance at it before then, that would be a great help.
Thanks,
Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/file_fdw.c
--- b/contrib/file_fdw/file_fdw.c
***
*** 799,806 check_selective_binary_conversion(RelOptInfo *baserel,
}
/* Collect all the attributes needed for joins or final output. */
! pull_varattnos((Node *) baserel-reltargetlist, baserel-relid,
! attrs_used);
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)
--- 799,811
}
/* Collect all the attributes needed for joins or final output. */
! for (i = baserel-min_attr; i = baserel-max_attr; i++)
! {
! if (!bms_is_empty(baserel-attr_needed[i - baserel-min_attr]))
! attrs_used =
! bms_add_member(attrs_used,
! i - FirstLowInvalidHeapAttributeNumber);
! }
/* Add all the attributes used by restriction clauses. */
foreach(lc, baserel-baserestrictinfo)
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
***
*** 577,588 set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
childrel-has_eclass_joins = rel-has_eclass_joins;
/*
! * Note: we could compute appropriate attr_needed data for the child's
! * variables, by transforming the parent's attr_needed through the
! * translated_vars mapping. However, currently there's no need
! * because attr_needed is only examined for base relations not
! * otherrels. So we just leave the child's attr_needed empty.
*/
/*
* Compute the child's size.
--- 577,585
childrel-has_eclass_joins = rel-has_eclass_joins;
/*
! * Compute the child's attr_needed.
*/
+ adjust_appendrel_attr_needed(rel, childrel, childRTE, appinfo);
/*
* Compute the child's size.
***
*** 2173,2178 remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
--- 2170,2176
{
Bitmapset *attrs_used = NULL;
ListCell *lc;
+ int i;
/*
* Do nothing if subquery has UNION/INTERSECT/EXCEPT: in principle we
***
*** 2193,2204 remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
* Collect a bitmap of all the output column numbers used by the upper
* query.
*
! * Add all the attributes needed for joins or final output. Note: we must
! * look at reltargetlist, not the attr_needed data, because attr_needed
! * isn't computed for inheritance child rels, cf set_append_rel_size().
! * (XXX might be worth changing that sometime.)
*/
! pull_varattnos((Node *) rel-reltargetlist, rel-relid, attrs_used);
/* Add all the attributes used by un-pushed-down restriction clauses. */
foreach(lc, rel-baserestrictinfo)
--- 2191,2205
* Collect a bitmap of all the output column numbers used by the upper
* query.
*
! * Add all the attributes needed for joins or final output.
*/
! for (i = rel-min_attr; i = rel-max_attr; i++)
! {
! if (!bms_is_empty(rel-attr_needed[i - rel-min_attr]))
! attrs_used =
! bms_add_member(attrs_used,
! i - FirstLowInvalidHeapAttributeNumber);
! }
/* Add all the attributes used by un-pushed-down restriction clauses. */
foreach(lc, rel-baserestrictinfo)
*** a/src/backend/optimizer/path/indxpath.c
--- b/src/backend/optimizer/path/indxpath.c
***
*** 1768,1779 check_index_only(RelOptInfo *rel, IndexOptInfo *index)
* way out.
*/
! /*
! * Add all the attributes needed for joins or final output. Note: we must
! * look at reltargetlist, not the attr_needed data, because attr_needed
! * isn't computed for inheritance child rels.
! */
! pull_varattnos((Node *) rel-reltargetlist, rel-relid, attrs_used);
/* Add all the attributes used by restriction clauses. */
foreach(lc, rel-baserestrictinfo)
--- 1768,1781
* way out.
*/
! /* Collect all the attributes needed for joins or final output. */
! for (i = rel-min_attr; i = rel-max_attr; i++)
! {
! if (!bms_is_empty(rel-attr_needed[i
Re: [HACKERS] inherit support for foreign tables
(2014/07/10 18:12), Shigeru Hanada wrote: 2014-06-24 16:30 GMT+09:00 Etsuro Fujita fujita.ets...@lab.ntt.co.jp: (2014/06/23 18:35), Ashutosh Bapat wrote: Selecting tableoid on parent causes an error, ERROR: cannot extract system attribute from virtual tuple. The foreign table has an OID which can be reported as tableoid for the rows coming from that foreign table. Do we want to do that? No. I think it's a bug. I'll fix it. IIUC, you mean that tableoid can't be retrieved when a foreign table is accessed via parent table, No. What I want to say is that tableoid *can* be retrieved when a foreign table is accessed via the parent table. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/07/11 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
Hi Fujita-san, Sorry for leaving this thread for long time. 2014-06-24 16:30 GMT+09:00 Etsuro Fujita fujita.ets...@lab.ntt.co.jp: (2014/06/23 18:35), Ashutosh Bapat wrote: Hi, Selecting tableoid on parent causes an error, ERROR: cannot extract system attribute from virtual tuple. The foreign table has an OID which can be reported as tableoid for the rows coming from that foreign table. Do we want to do that? No. I think it's a bug. I'll fix it. IIUC, you mean that tableoid can't be retrieved when a foreign table is accessed via parent table, but it sounds strange to me, because one of main purposes of tableoid is determine actual source table in appended results. Am I missing the point? -- Shigeru HANADA -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
If we are going to change that portion of the code, we may as well go a bit forward and allow any expressions to be fetched from a foreign server (obviously, if that server is capable of doing so). It will help, when we come to aggregate push-down or whole query push-down (whenever that happens). So, instead of attr_needed, which restricts only the attributes to be fetched, why not to targetlist itself? On Mon, Jun 30, 2014 at 7:18 PM, Tom Lane t...@sss.pgh.pa.us wrote: Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: Done. I think this is because create_foreignscan_plan() makes reference to attr_needed, which isn't computed for inheritance children. I wonder whether it isn't time to change that. It was coded like that originally only because calculating the values would've been a waste of cycles at the time. But this is at least the third place where it'd be useful to have attr_needed for child rels. regards, tom lane -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] inherit support for foreign tables
On Tue, Jul 1, 2014 at 7:39 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/06/30 20:17), Ashutosh Bapat wrote: On Mon, Jun 30, 2014 at 4:17 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: (2014/06/30 17:47), Ashutosh Bapat wrote: BTW, why aren't you using the tlist passed to this function? I guess create_scan_plan() passes tlist after processing it, so that should be used rather than rel-reltargetlist. I think that that would be maybe OK, but I think that it would not be efficient to use the list to compute attrs_used, because the tlist would have more information than rel-reltargetlist in cases where the tlist is build through build_physical_tlist(). In that case, we can call build_relation_tlist() for foreign tables. Do you mean build_physical_tlist()? Sorry, I meant build_path_tlist() or disuse_physical_tlist() whichever is appropriate. We may want to modify use_physical_tlist(), to return false, in case of foreign tables. BTW, it does return false for inheritance trees. 486 /* 487 * Can't do it with inheritance cases either (mainly because Append 488 * doesn't project). 489 */ 490 if (rel-reloptkind != RELOPT_BASEREL) 491 return false; Yeah, we can call build_physical_tlist() (and do that in some cases), but if we call the function, it would generate a tlist that contains all Vars in the relation, not only those Vars actually needed by the query (ie, Vars in reltargetlist), and thus it would take more cycles to compute attr_used from the tlist than from reltargetlist. That' what I wanted to say. Thanks, Best regards, Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] inherit support for foreign tables
Hello,
Sorry, this was no relation with this patch.
ForeignNext materializes the slot, which would be any of physical
and virtual tuple, when system column was requested. If it was a
virtual one, file_fdw makes this, heap_form_tuple generates the
tuple as DatumTuple. The result is a jumble of virtual and
physical. But the returning slot has tts_tuple so the caller
interprets it as a physical one.
Anyway the requests for xmin/xmax could not be prevented
beforehand in current framework, I did rewrite the physical tuple
header so as to really be a physical one.
This would be another patch, so I will put this into next CF if
this don't get any immediate objection.
By the way, I tried xmin and xmax for the file_fdw tables.
postgres=# select tableoid, xmin, xmax, * from passwd1;
tableoid | xmin |xmax| uname | pass | uid | gid | ..
16396 | 244 | 4294967295 | root | x| 0 | 0 | root...
16396 | 252 | 4294967295 | bin | x| 1 | 1 | bin...
16396 | 284 | 4294967295 | daemon| x| 2 | 2 |
daemon...
The xmin and xmax apparently doesn't look sane. After some
investigation, I found that they came from the following place in
heap_form_tuple(), (call stach is show below)
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c
index 9cc5345..728db14 100644
--- a/src/backend/executor/nodeForeignscan.c
+++ b/src/backend/executor/nodeForeignscan.c
@@ -22,6 +22,8 @@
*/
#include postgres.h
+#include access/transam.h
+#include access/htup_details.h
#include executor/executor.h
#include executor/nodeForeignscan.h
#include foreign/fdwapi.h
@@ -58,8 +60,21 @@ ForeignNext(ForeignScanState *node)
*/
if (plan-fsSystemCol !TupIsNull(slot))
{
+ bool was_virtual_tuple = (slot-tts_tuple == NULL);
HeapTuple tup = ExecMaterializeSlot(slot);
+ if (was_virtual_tuple)
+ {
+ /*
+ * ExecMaterializeSlot fills the tuple header as a
+ * DatumTupleFields if the slot was a virtual tuple, but a
+ * physical one is needed here. So rewrite the tuple header as
+ * HeapTupleFirelds.
+ */
+ HeapTupleHeaderSetXmin(tup-t_data, FrozenTransactionId);
+ HeapTupleHeaderSetXmax(tup-t_data, InvalidTransactionId);
+ HeapTupleHeaderSetCmin(tup-t_data, FirstCommandId);
+ }
tup-t_tableOid = RelationGetRelid(node-ss.ss_currentRelation);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/07/01 15:13), Ashutosh Bapat wrote: On Tue, Jul 1, 2014 at 7:39 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: We may want to modify use_physical_tlist(), to return false, in case of foreign tables. BTW, it does return false for inheritance trees. Yeah, but please consider cases where foreign tables are not inheritance child rels (and any system columns are requested). 486 /* 487 * Can't do it with inheritance cases either (mainly because Append 488 * doesn't project). 489 */ 490 if (rel-reloptkind != RELOPT_BASEREL) 491 return false; Yeah, we can call build_physical_tlist() (and do that in some cases), but if we call the function, it would generate a tlist that contains all Vars in the relation, not only those Vars actually needed by the query (ie, Vars in reltargetlist), and thus it would take more cycles to compute attr_used from the tlist than from reltargetlist. That' what I wanted to say. Maybe I'm missing something, but what's the point of using the tlist, not reltargetlist? Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On Tue, Jul 1, 2014 at 12:25 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/07/01 15:13), Ashutosh Bapat wrote: On Tue, Jul 1, 2014 at 7:39 AM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: We may want to modify use_physical_tlist(), to return false, in case of foreign tables. BTW, it does return false for inheritance trees. Yeah, but please consider cases where foreign tables are not inheritance child rels (and any system columns are requested). 486 /* 487 * Can't do it with inheritance cases either (mainly because Append 488 * doesn't project). 489 */ 490 if (rel-reloptkind != RELOPT_BASEREL) 491 return false; Yeah, we can call build_physical_tlist() (and do that in some cases), but if we call the function, it would generate a tlist that contains all Vars in the relation, not only those Vars actually needed by the query (ie, Vars in reltargetlist), and thus it would take more cycles to compute attr_used from the tlist than from reltargetlist. That' what I wanted to say. Maybe I'm missing something, but what's the point of using the tlist, not reltargetlist? Compliance with other create_*scan_plan() functions. The tlist passed to those functions is sometimes preprocessed in create_scan_plan() and some of the function it calls. If we use reltargetlist directly, we loose that preprocessing. I have not see any of create_*scan_plan() fetch the targetlist directly from RelOptInfo. It is always the one supplied by build_path_tlist() or disuse_physical_tlist() (which in turn calls build_path_tlist()) or build_physical_tlist(). Thanks, Best regards, Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] inherit support for foreign tables
(2014/07/01 16:04), Ashutosh Bapat wrote: On Tue, Jul 1, 2014 at 12:25 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: Maybe I'm missing something, but what's the point of using the tlist, not reltargetlist? Compliance with other create_*scan_plan() functions. The tlist passed to those functions is sometimes preprocessed in create_scan_plan() and some of the function it calls. If we use reltargetlist directly, we loose that preprocessing. I have not see any of create_*scan_plan() fetch the targetlist directly from RelOptInfo. It is always the one supplied by build_path_tlist() or disuse_physical_tlist() (which in turn calls build_path_tlist()) or build_physical_tlist(). I've got the point. As I said upthread, I'll work on calculating attr_needed for child rels, and I hope that that will eliminate your concern. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
Hi, At Tue, 01 Jul 2014 16:30:41 +0900, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote in 53b263a1.3060...@lab.ntt.co.jp I've got the point. As I said upthread, I'll work on calculating attr_needed for child rels, and I hope that that will eliminate your concern. Inheritance tree is expanded far after where attr_needed for the parent built, in set_base_rel_sizes() in make_one_rel(). I'm afraid that building all attr_needed for whole inheritance tree altogether is a bit suffering. I have wanted the point of inheritance expansion earlier for another patch. Do you think that rearranging there? Or generate them individually in crete_foreign_plan()? Anyway, I'm lookin forward to your next patch. So no answer needed. regards, -- Kyotaro Horiguchi NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On Fri, Jun 20, 2014 at 05:04:06PM +0900, Kyotaro HORIGUCHI wrote: Attached is the rebased patch of v11 up to the current master. I've been studying this patch. SELECT FOR UPDATE on the inheritance parent fails with a can't-happen error condition, even when SELECT FOR UPDATE on the child foreign table alone would have succeeded. The patch adds zero tests. Add principal tests to postgres_fdw.sql. Also, create a child foreign table in foreign_data.sql; this will make dump/reload tests of the regression database exercise an inheritance tree that includes a foreign table. The inheritance section of ddl.sgml should mention child foreign tables, at least briefly. Consider mentioning it in the partitioning section, too. Your chosen ANALYZE behavior is fair, but the messaging from a database-wide ANALYZE VERBOSE needs work: INFO: analyzing test_foreign_inherit.parent INFO: parent: scanned 1 of 1 pages, containing 1 live rows and 0 dead rows; 1 rows in sample, 1 estimated total rows INFO: analyzing test_foreign_inherit.parent inheritance tree WARNING: relcache reference leak: relation child not closed WARNING: relcache reference leak: relation tchild not closed WARNING: relcache reference leak: relation parent not closed Please arrange to omit the 'analyzing tablename inheritance tree' message, since this ANALYZE actually skipped that task. (The warnings obviously need a fix, too.) I do find it awkward that adding a foreign table to an inheritance tree will make autovacuum stop collecting statistics on that inheritance tree, but I can't think of a better policy. The rest of these review comments are strictly observations; they're not requests for you to change the patch, but they may deserve more discussion. We use the term child table in many messages. Should that change to something more inclusive, now that the child may be a foreign table? Perhaps one of child relation, plain child, or child foreign table/child table depending on the actual object? child relation is robust technically, but it might add more confusion than it removes. Varying the message depending on the actual object feels like a waste. Opinions? LOCK TABLE on the inheritance parent locks child foreign tables, but LOCK TABLE fails when given a foreign table directly. That's odd, but I see no cause to change it. A partition root only accepts an UPDATE command if every child is updatable. Similarly, UPDATE ... WHERE CURRENT OF cursor_name fails if any child does not support it. That seems fine. Incidentally, does anyone have a FDW that supports WHERE CURRENT OF? The longstanding behavior of CREATE TABLE INHERITS is to reorder local columns to match the order found in parents. That is, both of these tables actually have columns in the order (a,b): create table parent (a int, b int); create table child (b int, a int) inherits (parent); Ordinary dump/reload always uses CREATE TABLE INHERITS, thereby changing column order in this way. (pg_dump --binary-upgrade uses ALTER TABLE INHERIT and some catalog hacks to avoid doing so.) I've never liked that dump/reload can change column order, but it's tolerable if you follow the best practice of always writing out column lists. The stakes rise for foreign tables, because column order is inherently significant to file_fdw and probably to certain other non-RDBMS FDWs. If pg_dump changes column order in your file_fdw foreign table, the table breaks. I would heartily support making pg_dump preserve column order for all inheritance children. That doesn't rise to the level of being a blocker for this patch, though. I am attaching rough-hewn tests I used during this review. Thanks, nm -- Noah Misch EnterpriseDB http://www.enterprisedb.com drop schema test_foreign_inherit cascade; create schema test_foreign_inherit; set search_path = test_foreign_inherit; create extension postgres_fdw; CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw OPTIONS (dbname 'test'); CREATE USER MAPPING FOR CURRENT_USER SERVER loopback; create extension file_fdw; CREATE SERVER f FOREIGN DATA WRAPPER file_fdw; create table parent ( a int, b text, c date, d path ); insert into parent values (4, 'foo', current_date, '0,0,1,2'); -- regular child create table tchild () inherits (parent); insert into tchild values (5, 'oof', current_date - 2, '-1,-1,0,0'); -- cursor update works begin; declare c cursor for select 1 from parent where b = 'oof'; fetch from c; explain analyze update parent set b = null where current of c; select * from parent; rollback; -- foreign side create table _child ( c date, b text, extra numeric, d path, a int ); -- Foreign child. No column reorder. create foreign table child ( c date, b text, extra numeric, d path, a int ) SERVER LOOPBACK OPTIONS (table_name '_child'); alter table child inherit parent; insert into
Re: [HACKERS] inherit support for foreign tables
I checked that it's reporting the right tableoid now. BTW, why aren't you using the tlist passed to this function? I guess create_scan_plan() passes tlist after processing it, so that should be used rather than rel-reltargetlist. On Mon, Jun 30, 2014 at 12:22 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/06/24 16:30), Etsuro Fujita wrote: (2014/06/23 18:35), Ashutosh Bapat wrote: Selecting tableoid on parent causes an error, ERROR: cannot extract system attribute from virtual tuple. The foreign table has an OID which can be reported as tableoid for the rows coming from that foreign table. Do we want to do that? No. I think it's a bug. I'll fix it. Done. I think this is because create_foreignscan_plan() makes reference to attr_needed, which isn't computed for inheritance children. To aboid this, I've modified create_foreignscan_plan() to see reltargetlist and baserestrictinfo, instead of attr_needed. Please find attached an updated version of the patch. Sorry for the delay. Best regards, Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: [HACKERS] inherit support for foreign tables
(2014/06/30 17:47), Ashutosh Bapat wrote: I checked that it's reporting the right tableoid now. Thank you for the check. BTW, why aren't you using the tlist passed to this function? I guess create_scan_plan() passes tlist after processing it, so that should be used rather than rel-reltargetlist. I think that that would be maybe OK, but I think that it would not be efficient to use the list to compute attrs_used, because the tlist would have more information than rel-reltargetlist in cases where the tlist is build through build_physical_tlist(). Thanks, Best regards, Etsuro Fujita On Mon, Jun 30, 2014 at 12:22 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: (2014/06/24 16:30), Etsuro Fujita wrote: (2014/06/23 18:35), Ashutosh Bapat wrote: Selecting tableoid on parent causes an error, ERROR: cannot extract system attribute from virtual tuple. The foreign table has an OID which can be reported as tableoid for the rows coming from that foreign table. Do we want to do that? No. I think it's a bug. I'll fix it. Done. I think this is because create_foreignscan_plan() makes reference to attr_needed, which isn't computed for inheritance children. To aboid this, I've modified create_foreignscan_plan() to see reltargetlist and baserestrictinfo, instead of attr_needed. Please find attached an updated version of the patch. Sorry for the delay. Best regards, Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On Mon, Jun 30, 2014 at 4:17 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp wrote: (2014/06/30 17:47), Ashutosh Bapat wrote: I checked that it's reporting the right tableoid now. Thank you for the check. BTW, why aren't you using the tlist passed to this function? I guess create_scan_plan() passes tlist after processing it, so that should be used rather than rel-reltargetlist. I think that that would be maybe OK, but I think that it would not be efficient to use the list to compute attrs_used, because the tlist would have more information than rel-reltargetlist in cases where the tlist is build through build_physical_tlist(). In that case, we can call build_relation_tlist() for foreign tables. Thanks, Best regards, Etsuro Fujita On Mon, Jun 30, 2014 at 12:22 PM, Etsuro Fujita fujita.ets...@lab.ntt.co.jp mailto:fujita.ets...@lab.ntt.co.jp wrote: (2014/06/24 16:30), Etsuro Fujita wrote: (2014/06/23 18:35), Ashutosh Bapat wrote: Selecting tableoid on parent causes an error, ERROR: cannot extract system attribute from virtual tuple. The foreign table has an OID which can be reported as tableoid for the rows coming from that foreign table. Do we want to do that? No. I think it's a bug. I'll fix it. Done. I think this is because create_foreignscan_plan() makes reference to attr_needed, which isn't computed for inheritance children. To aboid this, I've modified create_foreignscan_plan() to see reltargetlist and baserestrictinfo, instead of attr_needed. Please find attached an updated version of the patch. Sorry for the delay. Best regards, Etsuro Fujita -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
