Re: [HACKERS] Incorrect comment for build_child_join_rel
(2017/11/11 0:58), Robert Haas wrote: On Fri, Nov 10, 2017 at 4:34 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: Here is a small patch for $Subject. Good catch. Committed. 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] Another oddity in handling of WCO constraints in postgres_fdw
(2017/11/01 11:16), Robert Haas wrote: On Wed, Oct 4, 2017 at 5:58 PM, Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> wrote: The view with WCO is local but the modification which violates WCO is being made on remote server by a trigger on remote table. Trying to control that doesn't seem to be a good idea, just like we can't control what rows get inserted on the foreign server when they violate local constraints. I think that's a fair point. For local constraints on foreign tables, it's the user's responsibility to ensure that those constraints matches the remote side, so we don't need to ensure those constraints locally. But I'm not sure if the same thing applies to WCOs on views defined on foreign tables, because in some case it's not possible to impose constraints on the remote side that match those WCOs, as I explained before. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Incorrect comment for build_child_join_rel
Hi, Here is a small patch for $Subject. Best regards, Etsuro Fujita *** a/src/backend/optimizer/util/relnode.c --- b/src/backend/optimizer/util/relnode.c *** *** 676,683 build_join_rel(PlannerInfo *root, * 'sjinfo': child-join context info * 'restrictlist': list of RestrictInfo nodes that apply to this particular *pair of joinable relations ! * 'join_appinfos': list of AppendRelInfo nodes for base child relations ! *involved in this join */ RelOptInfo * build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel, --- 676,682 * 'sjinfo': child-join context info * 'restrictlist': list of RestrictInfo nodes that apply to this particular *pair of joinable relations ! * 'jointype' is the join type (inner, left, full, etc) */ RelOptInfo * build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel, -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Reorder header files in alphabetical order
Hi, Attached is a patch to reorder header files in joinrels.c and pathnode.c in alphabetical order, removing unnecessary ones. Best regards, Etsuro Fujita *** a/src/backend/optimizer/path/joinrels.c --- b/src/backend/optimizer/path/joinrels.c *** *** 16,30 #include "miscadmin.h" #include "catalog/partition.h" - #include "nodes/relation.h" #include "optimizer/clauses.h" #include "optimizer/joininfo.h" #include "optimizer/pathnode.h" #include "optimizer/paths.h" #include "optimizer/prep.h" - #include "optimizer/cost.h" - #include "utils/memutils.h" #include "utils/lsyscache.h" static void make_rels_by_clause_joins(PlannerInfo *root, --- 16,28 #include "miscadmin.h" #include "catalog/partition.h" #include "optimizer/clauses.h" #include "optimizer/joininfo.h" #include "optimizer/pathnode.h" #include "optimizer/paths.h" #include "optimizer/prep.h" #include "utils/lsyscache.h" + #include "utils/memutils.h" static void make_rels_by_clause_joins(PlannerInfo *root, *** a/src/backend/optimizer/util/pathnode.c --- b/src/backend/optimizer/util/pathnode.c *** *** 17,24 #include #include "miscadmin.h" ! #include "nodes/nodeFuncs.h" #include "nodes/extensible.h" #include "optimizer/clauses.h" #include "optimizer/cost.h" #include "optimizer/pathnode.h" --- 17,25 #include #include "miscadmin.h" ! #include "foreign/fdwapi.h" #include "nodes/extensible.h" + #include "nodes/nodeFuncs.h" #include "optimizer/clauses.h" #include "optimizer/cost.h" #include "optimizer/pathnode.h" *** *** 29,35 #include "optimizer/tlist.h" #include "optimizer/var.h" #include "parser/parsetree.h" - #include "foreign/fdwapi.h" #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/selfuncs.h" --- 30,35 -- 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] Comment typo
On 2017/10/30 22:39, Magnus Hagander wrote: On Mon, Oct 30, 2017 at 3:49 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp <mailto:fujita.ets...@lab.ntt.co.jp>> wrote: Here is a patch to fix a typo in a comment in partition.c: s/specificiation/specification/. Applied, 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
[HACKERS] Comment typo
Here is a patch to fix a typo in a comment in partition.c: s/specificiation/specification/. Best regards, Etsuro Fujita diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index 07fdf66..e234167 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -703,7 +703,7 @@ partition_bounds_equal(int partnatts, int16 *parttyplen, bool *parttypbyval, /* * Return a copy of given PartitionBoundInfo structure. The data types of bounds - * are described by given partition key specificiation. + * are described by given partition key specification. */ extern PartitionBoundInfo partition_bounds_copy(PartitionBoundInfo src, -- 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] Typos in src/backend/optimizer/README
On 2017/10/28 18:15, Robert Haas wrote: On Fri, Oct 27, 2017 at 9:34 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: This sentence in the section of Partition-wise joins in src/backend/optimizer/README should be fixed: "This technique of breaking down a join between partition tables into join between their partitions is called partition-wise join." (1) s/a join between partition tables/a join between partitioned tables/ (2) s/join between their partitions/joins between their partitions/ It might be okay to leave #2 as-is, but I'd like to propose to change that way to make the meaning clear. I think you are right. I have committed the patch. 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] Add support for tuple routing to foreign partitions
On 2017/10/26 16:40, Etsuro Fujita wrote: Other changes I made to the executor are: (1) currently, we set the RT index for the root partitioned table to ri_RangeTableIndex of partitions' ResultRelInfos, but the proposed EXPLAIN requires that the partition's ri_RangeTableIndex is set to the RT index for that partition's RTE, to show that partition info in the output. So, I made that change. One thing I forgot to mention is: that would be also required to call BeginForeignModify, ExecForeignInsert, and EndForeignModify with the partition's ResultRelInfo. I updated docs in doc/src/sgml/ddl.sgml the same way as [1]. (I used only the ddl.sgml change proposed by [1], not all the changes.) I did some cleanup as well. Please find attached an updated version of the patch. Best regards, Etsuro Fujita [1] https://www.postgresql.org/message-id/b19a8e2b-e000-f592-3e0b-3e90ba0fa816%40lab.ntt.co.jp *** a/contrib/file_fdw/input/file_fdw.source --- b/contrib/file_fdw/input/file_fdw.source *** *** 176,182 COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ','); --- 176,188 SELECT tableoid::regclass, * FROM pt; SELECT tableoid::regclass, * FROM p1; SELECT tableoid::regclass, * FROM p2; + \t on + EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (1, 'xyzzy'); + \t off INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR + \t on + EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (2, 'xyzzy'); + \t off INSERT INTO pt VALUES (2, 'xyzzy'); SELECT tableoid::regclass, * FROM pt; SELECT tableoid::regclass, * FROM p1; *** a/contrib/file_fdw/output/file_fdw.source --- b/contrib/file_fdw/output/file_fdw.source *** *** 315,321 SELECT tableoid::regclass, * FROM p2; (0 rows) COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR ! ERROR: cannot route inserted tuples to a foreign table CONTEXT: COPY pt, line 2: "1,qux" COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ','); SELECT tableoid::regclass, * FROM pt; --- 315,321 (0 rows) COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR ! ERROR: cannot route copied tuples to a foreign table CONTEXT: COPY pt, line 2: "1,qux" COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ','); SELECT tableoid::regclass, * FROM pt; *** *** 341,348 SELECT tableoid::regclass, * FROM p2; p2 | 2 | qux (2 rows) INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR ! ERROR: cannot route inserted tuples to a foreign table INSERT INTO pt VALUES (2, 'xyzzy'); SELECT tableoid::regclass, * FROM pt; tableoid | a | b --- 341,366 p2 | 2 | qux (2 rows) + \t on + EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (1, 'xyzzy'); + Insert on public.pt +Foreign Insert on public.p1 +Insert on public.p2 +-> Result + Output: 1, 'xyzzy'::text + + \t off INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR ! ERROR: cannot route inserted tuples to foreign table "p1" ! \t on ! EXPLAIN (VERBOSE, COSTS FALSE) INSERT INTO pt VALUES (2, 'xyzzy'); ! Insert on public.pt !Foreign Insert on public.p1 !Insert on public.p2 !-> Result ! Output: 2, 'xyzzy'::text ! ! \t off INSERT INTO pt VALUES (2, 'xyzzy'); SELECT tableoid::regclass, * FROM pt; tableoid | a | b *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 7029,7034 NOTICE: drop cascades to foreign table bar2 --- 7029,7236 drop table loct1; drop table loct2; -- === + -- test tuple routing for foreign-table partitions + -- === + create table pt (a int, b int) partition by list (a); + create table locp partition of pt for values in (1); + create table locfoo (a int check (a in (2)), b int); + create foreign table remp partition of pt for values in (2) server loopback options (table_name 'locfoo'); + explain (verbose, costs off) + insert into pt values (1, 1), (2, 1); +QUERY PLAN + - + Insert on public.pt +Insert on public.locp +Foreign Insert on public.remp + Remote SQL: INSERT INTO public.locfoo(a, b) VALUES ($1, $2) +-> Values Scan on "*VALUES*" + Output: "*VALUES*".column1, "*VALUES*".column2 + (6 rows) + + insert into pt values (1, 1), (2, 1); + select tableoid::regclass, * FROM pt; + tableoid | a | b + --+---+--- + locp | 1 | 1 + remp | 2 | 1 + (2 rows) + + select tableoid::regclass, * FROM locp; + tableoid | a | b + --+---+--- + locp | 1 | 1 + (1
[HACKERS] Typos in src/backend/optimizer/README
Hi, This sentence in the section of Partition-wise joins in src/backend/optimizer/README should be fixed: "This technique of breaking down a join between partition tables into join between their partitions is called partition-wise join." (1) s/a join between partition tables/a join between partitioned tables/ (2) s/join between their partitions/joins between their partitions/ It might be okay to leave #2 as-is, but I'd like to propose to change that way to make the meaning clear. Attached is a patch for that. Best regards, Etsuro Fujita diff --git a/src/backend/optimizer/README b/src/backend/optimizer/README index 031e503..1e4084d 100644 --- a/src/backend/optimizer/README +++ b/src/backend/optimizer/README @@ -1088,8 +1088,8 @@ broken into joins between the matching partitions. The resultant join is partitioned in the same way as the joining relations, thus allowing an N-way join between similarly partitioned tables having equi-join condition between their partition keys to be broken down into N-way joins between their matching -partitions. This technique of breaking down a join between partition tables -into join between their partitions is called partition-wise join. We will use +partitions. This technique of breaking down a join between partitioned tables +into joins between their partitions is called partition-wise join. We will use term "partitioned relation" for either a partitioned table or a join between compatibly partitioned tables. -- 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] Add support for tuple routing to foreign partitions
Hi Maksim, On 2017/10/02 21:37, Maksim Milyutin wrote: On 11.09.2017 16:01, Etsuro Fujita wrote: * Query planning: the patch creates copies of Query/Plan with a foreign partition as target from the original Query/Plan for each foreign partition and invokes PlanForeignModify with those copies, to allow the FDW to do query planning for remote INSERT with the existing API. To make such Queries the similar way inheritance_planner does, I modified transformInsertStmt so that the inh flag for the partitioned table's RTE is set to true, which allows (1) expand_inherited_rtentry to build an RTE and AppendRelInfo for each partition in the partitioned table and (2) make_modifytable to build such Queries using adjust_appendrel_attrs and those AppendRelInfos. * explain.c: I modified show_modifytable_info so that we can show remote queries for foreign partitions in EXPLAIN for INSERT into a partitioned table the same way as for inherited UPDATE/DELETE cases. Here is an example: postgres=# explain verbose insert into pt values (1), (2); QUERY PLAN --- Insert on public.pt (cost=0.00..0.03 rows=2 width=4) Foreign Insert on public.fp1 Remote SQL: INSERT INTO public.t1(a) VALUES ($1) Foreign Insert on public.fp2 Remote SQL: INSERT INTO public.t2(a) VALUES ($1) -> Values Scan on "*VALUES*" (cost=0.00..0.03 rows=2 width=4) Output: "*VALUES*".column1 (7 rows) I think I should add more explanation about the patch, but I don't have time today, so I'll write additional explanation in the next email. Sorry about that. Could you update your patch, it isn't applied on the actual state of master. Namely conflict in src/backend/executor/execMain.c Attached is an updated version. * As mentioned in "Query planning", the patch builds an RTE for each partition so that the FDW can make reference to that RTE in eg, PlanForeignModify. set_plan_references also uses such RTEs to record plan dependencies for plancache.c to invalidate cached plans. See an example for that added to the regression tests in postgres_fdw. * As mentioned in "explain.c", the EXPLAIN shows all partitions beneath the ModifyTable node. One merit of that is we can show remote queries for foreign partitions in the output as shown above. Another one I can think of is when reporting execution stats for triggers. Here is an example for that: postgres=# explain analyze insert into list_parted values (1, 'hi there'), (2, 'hi there'); QUERY PLAN -- Insert on list_parted (cost=0.00..0.03 rows=2 width=36) (actual time=0.375..0.375 rows=0 loops=1) Insert on part1 Insert on part2 -> Values Scan on "*VALUES*" (cost=0.00..0.03 rows=2 width=36) (actual time=0.004..0.007 rows=2 loops=1) Planning time: 0.089 ms Trigger part1brtrig on part1: time=0.059 calls=1 Trigger part2brtrig on part2: time=0.021 calls=1 Execution time: 0.422 ms (8 rows) This would allow the user to understand easily that "part1" and "part2" in the trigger lines are the partitions of list_parted. So, I think it's useful to show partition info in the ModifyTable node even in the case where a partitioned table only contains plain tables. * The patch modifies make_modifytable and ExecInitModifyTable so that the former can allow the FDW to construct private plan data for each foreign partition and accumulate it all into a list, and that the latter can perform BeginForeignModify for each partition using that plan data stored in the list passed from make_modifytable. Other changes I made to the executor are: (1) currently, we set the RT index for the root partitioned table to ri_RangeTableIndex of partitions' ResultRelInfos, but the proposed EXPLAIN requires that the partition's ri_RangeTableIndex is set to the RT index for that partition's RTE, to show that partition info in the output. So, I made that change. Because of that, ExecPartitionCheck, ExecConstraints, and ExecWithCheckOptions are adjusted accordingly. (2) I added a new member to ResultRelInfo (ie, ri_PartitionIsValid), and modified CheckValidResultRel so that it checks a given partition without throwing an error and save the result in that flag so that ExecInsert determines using that flag whether a partition chosen by ExecFindPartition is valid for tuple-routing as proposed before. * copy.c: I still don't think it's a good idea to implement COPY tuple-routing for foreign partitions using PlanForeignModify. (I plan to propose new FDW APIs for bulkload as discussed before, to implement this feature.) So, I kept that as-is. Two things I changed there are: (1) currently, ExecSetupPartitionTupleRouti
Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw
On 2017/10/05 20:06, Kyotaro HORIGUCHI wrote: Since WCO ensures finally inserted values, we can't do other than acturally requesting for the values. I think so too. So just merging WCO columns to RETURNING in deparsed query is ok. But can't we concatenate returningList and withCheckOptionList at more higher level? Specifically, just passing calculated used_attr to deparse(Insert|Update)Sql instead of returningList and withCheckOptionList separately. Deparsed queries anyway forget the origin of requested columns. We could do that, but I think that would need a bit more code to postgresPlanForeignModify including changes to the deparseDeleteSql API in addition to the deparse(Insert|Update)Sql APIs. I prefer making high level functions simple, so I'd vote for just passing withCheckOptionList separately to deparse(Insert|Update)Sql, as proposed in the patch. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Comment typo
On 2017/10/05 21:48, Robert Haas wrote: On Thu, Oct 5, 2017 at 6:11 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: Here is a small patch to fix a typo in a comment in partition.c: s/mantain/maintain/. Committed. 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
[HACKERS] Comment typo
Here is a small patch to fix a typo in a comment in partition.c: s/mantain/maintain/. Best regards, Etsuro Fujita diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index 1ab6dba..9777d40 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -1236,7 +1236,7 @@ RelationGetPartitionDispatchInfo(Relation rel, * get_partition_dispatch_recurse * Recursively expand partition tree rooted at rel * - * As the partition tree is expanded in a depth-first manner, we mantain two + * As the partition tree is expanded in a depth-first manner, we maintain two * global lists: of PartitionDispatch objects corresponding to partitioned * tables in *pds and of the leaf partition OIDs in *leaf_part_oids. * -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw
On 2017/10/04 21:28, Ashutosh Bapat wrote: On Wed, Oct 4, 2017 at 5:32 PM, Robert Haas <robertmh...@gmail.com> wrote: On Wed, Oct 4, 2017 at 6:40 AM, Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> wrote: We can check whether a row being sent from the local server to the foreign server obeys WCO, but what foreign server does to that row is beyond local server's scope. But I think right now we're not checking the row being sent from the local server, either. We don't check the row *before* sending it to the remote server, but check the row returned by ExecForeignInsert/ExecForeignUpdate, which is allowed to have been changed by the remote server. In postgres_fdw, we currently return the data actually inserted/updated if RETURNING/AFTER TRIGGER present, but not if WCO only presents. So, for the postgres_fdw foreign table, WCO is enforced on the data that was actually inserted/updated if RETURNING/AFTER TRIGGER present and on the original data core supplied if WCO only presents, which is inconsistent behavior. Didn't 7086be6e3627c1ad797e32ebbdd232905b5f577f fix that? No. The commit addressed another issue. The WCO that is being ignored isn't a constraint on the foreign table; it's a constraint on a view which happens to reference the foreign table. It seems quite odd for the "assume constraints are valid" property of the foreign table to propagate back up into the view that references it. Agreed. The view with WCO is local but the modification which violates WCO is being made on remote server by a trigger on remote table. Trying to control that doesn't seem to be a good idea, just like we can't control what rows get inserted on the foreign server when they violate local constraints. I am using local constraints as an example of precedence where we ignore what's happening on remote side and enforce whatever we could enforce locally. Local server should make sure that any rows sent from local server to the remote server do not violate any local WCO. Seems odd (and too restrictive) to me too. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw
On 2017/10/03 18:16, Ashutosh Bapat wrote: Enforcing WCO constraints imposed by the local server on the row/DML being passed to the foreign server is fine, but trying to impose them on the row being inserted/updated at the foreign server looks odd. May be we should just leave this case as it is. I am comparing this case with the way we handle constraints on a foreign table. Hmm, I think that would be okay in the case where WCO constraints match constraints on the foreign table, but I'm not sure that would be okay even in the case where WCO constraints don't match? Consider: create table bt (a int check (a % 2 = 0)); create foreign table ft (a int check (a % 2 = 0)) server loopback options (table_name 'bt'); create view rw_view_2 as select * from ft where a % 2 = 0 with check option; In that case the WCO constraint matches the constraint on the foreign table, so there would be no need to ensure the WCO constraint locally (to make the explanation simple, we assume here that we don't have triggers on the remote end). BUT: for another auto-updatable view defined using the same foreign table like this: create view rw_view_4 as select * from ft where a % 4 = 0 with check option; how is the WCO constraint (ie, a % 4 = 0) ensured remotely, which is different from the constraint on the foreign table (ie, a % 2 = 0)? Maybe I'm missing something, though. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Another oddity in handling of WCO constraints in postgres_fdw
Hi,
Commit 7086be6e3627c1ad797e32ebbdd232905b5f577f addressed mishandling of
WCO in direct foreign table modification by disabling it when we have
WCO, but I noticed another oddity in postgres_fdw:
postgres=# create table base_tbl (a int, b int);
postgres=# create function row_before_insupd_trigfunc() returns trigger
as $$begin new.a := new.a + 10; return new; end$$ language plpgsql;
postgres=# create trigger row_before_insupd_trigger before insert or
update on base_tbl for each row execute procedure
row_before_insupd_trigfunc();
postgres=# create server loopback foreign data wrapper postgres_fdw
options (dbname 'postgres');
postgres=# create user mapping for CURRENT_USER server loopback;
postgres=# create foreign table foreign_tbl (a int, b int) server
loopback options (table_name 'base_tbl');
postgres=# create view rw_view as select * from foreign_tbl where a < b
with check option;
So, this should fail, but
postgres=# insert into rw_view values (0, 5);
INSERT 0 1
The reason for that is: this is processed using postgres_fdw's
non-direct foreign table modification (ie. ForeignModify), but unlike
the RETURNING or local after trigger case, the ForeignModify doesn't
take care that remote triggers might change the data in that case, so
the WCO is evaluated using the data supplied, not the data actually
inserted, which I think is wrong. (I should have noticed that as well
while working on the fix, though.) So, I'd propose to fix that by
modifying postgresPlanForeignModify so that it handles WCO the same way
as for the RETURNING case. Attached is a patch for that. I'll add the
patch to the next commitfest.
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 138,143 static void deparseSubqueryTargetList(deparse_expr_cxt
*context);
--- 138,144
static void deparseReturningList(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
bool trig_after_row,
+List *withCheckOptionList,
List *returningList,
List **retrieved_attrs);
static void deparseColumnRef(StringInfo buf, int varno, int varattno,
***
*** 1548,1554 void
deparseInsertSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
List *targetAttrs, bool doNothing,
!List *returningList, List **retrieved_attrs)
{
AttrNumber pindex;
boolfirst;
--- 1549,1556
deparseInsertSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
List *targetAttrs, bool doNothing,
!List *withCheckOptionList, List *returningList,
!List **retrieved_attrs)
{
AttrNumber pindex;
boolfirst;
***
*** 1597,1603 deparseInsertSql(StringInfo buf, PlannerInfo *root,
deparseReturningList(buf, root, rtindex, rel,
rel->trigdesc &&
rel->trigdesc->trig_insert_after_row,
!returningList,
retrieved_attrs);
}
/*
--- 1599,1605
deparseReturningList(buf, root, rtindex, rel,
rel->trigdesc &&
rel->trigdesc->trig_insert_after_row,
!withCheckOptionList,
returningList, retrieved_attrs);
}
/*
***
*** 1610,1616 deparseInsertSql(StringInfo buf, PlannerInfo *root,
void
deparseUpdateSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
!List *targetAttrs, List *returningList,
List **retrieved_attrs)
{
AttrNumber pindex;
--- 1612,1619
void
deparseUpdateSql(StringInfo buf, PlannerInfo *root,
Index rtindex, Relation rel,
!List *targetAttrs,
!List *withCheckOptionList, List *returningList,
List **retrieved_attrs)
{
AttrNumber pindex;
***
*** 1639,1645 deparseUpdateSql(StringInfo buf, PlannerInfo *root,
deparseReturningList(buf, root, rtindex, rel,
rel->trigdesc &&
rel->trigdesc->trig_update_after_row,
!returningList,
retrieved_attrs);
}
/*
--- 1642,1648
deparseReturningList(buf, root, rtindex, rel,
Re: [HACKERS] postgres_fdw: evaluate placeholdervars on remote server
On 2017/09/16 0:19, Robert Haas wrote: On Fri, Sep 15, 2017 at 10:15 AM, Daniel Gustafsson <dan...@yesql.se> wrote: Have you had a chance to look at this such that we can expect a rebased version of this patch during the commitfest? Frankly, I think things where there was a ping multiple weeks before the CommitFest started and no rebase before it started should be regarded as untimely submissions, and summarily marked Returned with Feedback. The CommitFest is supposed to be a time to get things that are ready before it starts committed before it ends. Some amount of back-and-forth during the CF is of course to be expected, but we don't even really have enough bandwidth to deal with the patches that are being timely updated, never mind the ones that aren't. Agreed. I marked this as RWF. 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] Add support for tuple routing to foreign partitions
On 2017/08/17 17:27, Etsuro Fujita wrote: On 2017/07/11 6:56, Robert Haas wrote: I have to admit that I'm a little bit fuzzy about why foreign insert routing requires all of these changes. I think this patch would benefit from being accompanied by several paragraphs of explanation outlining the rationale for each part of the patch. Will do. Here is an updated version of the patch. * Query planning: the patch creates copies of Query/Plan with a foreign partition as target from the original Query/Plan for each foreign partition and invokes PlanForeignModify with those copies, to allow the FDW to do query planning for remote INSERT with the existing API. To make such Queries the similar way inheritance_planner does, I modified transformInsertStmt so that the inh flag for the partitioned table's RTE is set to true, which allows (1) expand_inherited_rtentry to build an RTE and AppendRelInfo for each partition in the partitioned table and (2) make_modifytable to build such Queries using adjust_appendrel_attrs and those AppendRelInfos. * explain.c: I modified show_modifytable_info so that we can show remote queries for foreign partitions in EXPLAIN for INSERT into a partitioned table the same way as for inherited UPDATE/DELETE cases. Here is an example: postgres=# explain verbose insert into pt values (1), (2); QUERY PLAN --- Insert on public.pt (cost=0.00..0.03 rows=2 width=4) Foreign Insert on public.fp1 Remote SQL: INSERT INTO public.t1(a) VALUES ($1) Foreign Insert on public.fp2 Remote SQL: INSERT INTO public.t2(a) VALUES ($1) -> Values Scan on "*VALUES*" (cost=0.00..0.03 rows=2 width=4) Output: "*VALUES*".column1 (7 rows) I think I should add more explanation about the patch, but I don't have time today, so I'll write additional explanation in the next email. Sorry about that. Best regards, Etsuro Fujita *** a/contrib/file_fdw/output/file_fdw.source --- b/contrib/file_fdw/output/file_fdw.source *** *** 315,321 SELECT tableoid::regclass, * FROM p2; (0 rows) COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR ! ERROR: cannot route inserted tuples to a foreign table CONTEXT: COPY pt, line 2: "1,qux" COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ','); SELECT tableoid::regclass, * FROM pt; --- 315,321 (0 rows) COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR ! ERROR: cannot route copied tuples to a foreign table CONTEXT: COPY pt, line 2: "1,qux" COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ','); SELECT tableoid::regclass, * FROM pt; *** *** 342,348 SELECT tableoid::regclass, * FROM p2; (2 rows) INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR ! ERROR: cannot route inserted tuples to a foreign table INSERT INTO pt VALUES (2, 'xyzzy'); SELECT tableoid::regclass, * FROM pt; tableoid | a | b --- 342,348 (2 rows) INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR ! ERROR: cannot route inserted tuples to foreign table "p1" INSERT INTO pt VALUES (2, 'xyzzy'); SELECT tableoid::regclass, * FROM pt; tableoid | a | b *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 7029,7034 NOTICE: drop cascades to foreign table bar2 --- 7029,7172 drop table loct1; drop table loct2; -- === + -- test tuple routing to foreign-table partitions + -- === + create table pt (a int, b int) partition by list (a); + create table locp partition of pt for values in (1); + create table locfoo (a int check (a in (2)), b int); + create foreign table remp partition of pt for values in (2) server loopback options (table_name 'locfoo'); + explain (verbose, costs off) + insert into pt values (1, 1), (2, 1); +QUERY PLAN + - + Insert on public.pt +Insert on public.locp +Foreign Insert on public.remp + Remote SQL: INSERT INTO public.locfoo(a, b) VALUES ($1, $2) +-> Values Scan on "*VALUES*" + Output: "*VALUES*".column1, "*VALUES*".column2 + (6 rows) + + insert into pt values (1, 1), (2, 1); + select tableoid::regclass, * FROM pt; + tableoid | a | b + --+---+--- + locp | 1 | 1 + remp | 2 | 1 + (2 rows) + + select tableoid::regclass, * FROM locp; + tableoid | a | b + --+---+--- + locp | 1 | 1 + (1 row) + + select tableoid::regclass, * FROM remp; + tableoid | a | b + --+---+--- + r
Re: [HACKERS] Tuple-routing for certain partitioned tables not working as expected
On 2017/09/08 0:21, Robert Haas wrote: On Thu, Sep 7, 2017 at 5:13 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: On 2017/08/30 17:20, Etsuro Fujita wrote: Here is a patch to skip the CheckValidResultRel checks for a target table if it's a foreign partition to perform tuple-routing for, as proposed by Robert. In the patch, to skip the checks, I passed to CheckValidResultRel a new flag indicating whether the target relation is a partition to do tuple-routing for, but while updating another patch for tuple-routing for foreign partitions in PG11, I noticed that it would be better to pass ResultRelInfo than that flag. The reason is: (1) we can see whether the relation is such a partition by looking at the ResultRelInfo's ri_PartitionRoot, instead of that flag, and (2) this is for tuple-routing for foreign partitions, but we could extend that function with that argument to do the checks without throwing an error and save the result in a new member of ResultRelInfo (eg, ri_PartitionIsValid) so that we can use that info to determine in ExecInsert whether a foreign partition chosen by ExecFindPartition is valid for tuple-routing. So, I updated the patch that way. Attached is an updated version of the patch. OK, committed to master and v10. I am less convinced than you that this was a must-fix issue, but it's not a very invasive change the way you did it here. Thank you! I'll update the tuple-routing-for-foreign-partitions patch that way. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Tuple-routing for certain partitioned tables not working as expected
On 2017/08/30 17:20, Etsuro Fujita wrote: Here is a patch to skip the CheckValidResultRel checks for a target table if it's a foreign partition to perform tuple-routing for, as proposed by Robert. In the patch, to skip the checks, I passed to CheckValidResultRel a new flag indicating whether the target relation is a partition to do tuple-routing for, but while updating another patch for tuple-routing for foreign partitions in PG11, I noticed that it would be better to pass ResultRelInfo than that flag. The reason is: (1) we can see whether the relation is such a partition by looking at the ResultRelInfo's ri_PartitionRoot, instead of that flag, and (2) this is for tuple-routing for foreign partitions, but we could extend that function with that argument to do the checks without throwing an error and save the result in a new member of ResultRelInfo (eg, ri_PartitionIsValid) so that we can use that info to determine in ExecInsert whether a foreign partition chosen by ExecFindPartition is valid for tuple-routing. So, I updated the patch that way. Attached is an updated version of the patch. (I discussed about lazy initialization of partition ResultRelInfos before, but I changed my mind because I noticed that the change to EXPLAIN for INSERT into a partitioned table, which I'd like to propose in the tuple-routing-for-foereign-partitions patch, needs partition ResultRelInfos.) Best regards, Etsuro Fujita *** /dev/null --- b/contrib/file_fdw/data/list1.csv *** *** 0 --- 1,2 + 1,foo + 1,bar *** /dev/null --- b/contrib/file_fdw/data/list2.bad *** *** 0 --- 1,2 + 2,baz + 1,qux *** /dev/null --- b/contrib/file_fdw/data/list2.csv *** *** 0 --- 1,2 + 2,baz + 2,qux *** a/contrib/file_fdw/input/file_fdw.source --- b/contrib/file_fdw/input/file_fdw.source *** *** 162,167 SELECT tableoid::regclass, * FROM agg FOR UPDATE; --- 162,188 ALTER FOREIGN TABLE agg_csv NO INHERIT agg; DROP TABLE agg; + -- declarative partitioning tests + SET ROLE regress_file_fdw_superuser; + CREATE TABLE pt (a int, b text) partition by list (a); + CREATE FOREIGN TABLE p1 partition of pt for values in (1) SERVER file_server + OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ','); + CREATE TABLE p2 partition of pt for values in (2); + SELECT tableoid::regclass, * FROM pt; + SELECT tableoid::regclass, * FROM p1; + SELECT tableoid::regclass, * FROM p2; + COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR + COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ','); + SELECT tableoid::regclass, * FROM pt; + SELECT tableoid::regclass, * FROM p1; + SELECT tableoid::regclass, * FROM p2; + INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR + INSERT INTO pt VALUES (2, 'xyzzy'); + SELECT tableoid::regclass, * FROM pt; + SELECT tableoid::regclass, * FROM p1; + SELECT tableoid::regclass, * FROM p2; + DROP TABLE pt; + -- privilege tests SET ROLE regress_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 *** *** 289,294 SELECT tableoid::regclass, * FROM agg FOR UPDATE; --- 289,375 ALTER FOREIGN TABLE agg_csv NO INHERIT agg; DROP TABLE agg; + -- declarative partitioning tests + SET ROLE regress_file_fdw_superuser; + CREATE TABLE pt (a int, b text) partition by list (a); + CREATE FOREIGN TABLE p1 partition of pt for values in (1) SERVER file_server + OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ','); + CREATE TABLE p2 partition of pt for values in (2); + SELECT tableoid::regclass, * FROM pt; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + (2 rows) + + SELECT tableoid::regclass, * FROM p1; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + (2 rows) + + SELECT tableoid::regclass, * FROM p2; + tableoid | a | b + --+---+--- + (0 rows) + + COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR + ERROR: cannot route inserted tuples to a foreign table + CONTEXT: COPY pt, line 2: "1,qux" + COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ','); + SELECT tableoid::regclass, * FROM pt; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + p2 | 2 | baz + p2 | 2 | qux + (4 rows) + + SELECT tableoid::regclass, * FROM p1; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + (2 rows) + + SELECT tableoid::regclass, * FROM p2; + tableoid | a | b + --+---+- + p2 | 2 | baz + p2 | 2 | qux + (2 rows) + + INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR + ERROR: cannot route inserted tuples to a foreign table + INSERT INTO pt VALUES (2, 'xyzzy')
Re: [HACKERS] Tuple-routing for certain partitioned tables not working as expected
On 2017/08/30 17:20, Etsuro Fujita wrote: On 2017/08/30 9:13, Amit Langote wrote: On 2017/08/29 20:18, Etsuro Fujita wrote: On 2017/08/25 22:26, Robert Haas wrote: On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: Agreed, but I'd vote for fixing this in v10 as proposed; I agree that just ripping the CheckValidResultRel checks out entirely is not a good idea, but that seems OK to me at least as a fix just for v10. I'm still not on-board with having this be the one case where we don't do CheckValidResultRel. If we want to still call it but pass down some additional information that can selectively skip certain checks, I could probably live with that. Another idea would be to not do CheckValidResultRel for partitions in ExecSetupPartitionTupleRouting; instead, do that the first time the partition is chosen by ExecFindPartition, and if successfully checked, initialize the partition's ResultRelInfo and other stuff. (We could skip this after the first time by checking whether we already have a valid ResultRelInfo for the chosen partition.) That could allow us to not only call CheckValidResultRel the way it is, but avoid initializing useless partitions for which tuples aren't routed at all. I too have thought about the idea of lazy initialization of the partition ResultRelInfos. I think it would be a good idea, but definitely something for PG 11. Agreed. Here is a patch to skip the CheckValidResultRel checks for a target table if it's a foreign partition to perform tuple-routing for, as proposed by Robert. I'll add this to the open items list for v10. 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] postgres_fdw bug in 9.6
On 2017/09/05 13:43, Ryan Murphy wrote: The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: not tested Spec compliant: not tested Documentation:not tested This feature is obviously a pretty deep cut, and I don't understand the JOIN system enough to comment on whether the code is correct. I did not see any issues when glancing through the patch. Thank you for the review! I ran "make check", "make installcheck" and "make installcheck-world" and had ALMOST no problems: I marked it Tested and Passed because the only issue I had was a recurring issue I've had with "make installcheck-world" which I think is unrelated: *** path/to/postgres/src/interfaces/ecpg/test/expected/connect-test5.stderr 2017-02-14 09:22:25.0 -0600 --- path/to/postgres/src/interfaces/ecpg/test/results/connect-test5.stderr 2017-09-04 23:31:50.0 -0500 *** *** 36,42 [NO_PID]: sqlca: code: 0, state: 0 [NO_PID]: ECPGconnect: opening database on port for user regress_ecpg_user2 [NO_PID]: sqlca: code: 0, state: 0 ! [NO_PID]: ECPGconnect: could not open database: FATAL: database "regress_ecpg_user2" does not exist ... I tested that with HEAD, but couldn't reproduce this problem. Didn't you test that with HEAD? But this still Needs Review by someone who knows the JOIN system better. I think Tom is reviewing this patch [1]. Best regards, Etsuro Fujita [1] https://www.postgresql.org/message-id/22168.1504041271%40sss.pgh.pa.us -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Copyright in partition.h and partition.c
Here is the copyright in partition.h: * Copyright (c) 2007-2017, PostgreSQL Global Development Group I think it's reasonable that that matches the copyright in partition.c, but partition.c has: * Portions Copyright (c) 1996-2017, PostgreSQL Global Development Group * Portions Copyright (c) 1994, Regents of the University of California Is that intentional? 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] Partition-wise join for join between (declaratively) partitioned tables
On 2017/09/05 13:20, Amit Langote wrote: On 2017/09/04 21:32, Ashutosh Bapat wrote: +1. Will Fujita-san's patch also handle getting rid of partitioned_rels list? As Fujita-san mentioned, his patch won't. Actually, I suppose he didn't say that partitioned_rels itself is useless, just that its particular usage in ExecInitModifyTable is. That's right. (I thought there would probably be no need to create that list if we created AppendRelInfos even for partitioned partitions.) We still need that list for planner to tell the executor that there are some RT entries the latter would need to lock before executing a given plan. Without that dedicated list, the executor cannot know at all that certain tables in the partition tree (viz. the partitioned ones) need to be locked. I mentioned the reason - (Merge)Append.subplans, ModifyTable.resultRelations does not contain respective entries corresponding to the partitioned tables, and traditionally, the executor looks at those lists to figure out the tables to lock. I think so too. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Partition-wise join for join between (declaratively) partitioned tables
On 2017/09/04 21:32, Ashutosh Bapat wrote: On Mon, Sep 4, 2017 at 10:04 AM, Amit Langote By the way, if you want to get rid of PartitionedChildRelInfo, you can do that as long as you find some other way of putting together the partitioned_rels list to add into the ModifyTable (Append/MergeAppend) node created for the root partitioned table. Currently, PartitionedChildRelInfo (and the root->pcinfo_list) is the way for expand_inherited_rtentry() to pass that information to the planner's path-generating code. We may be able to generate that list when actually creating the path using set_append_rel_pathlist() or inheritance_planner(), without having created a PartitionedChildRelInfo node beforehand. AFAIU, the list contained RTIs of the relations, which didnt' have corresponding AppendRelInfos to lock those relations. Now that we create AppendRelInfos even for partitioned partitions, I don't think we need the list to take care of the locks. I don't think so either. (Since I haven't followed discussions on this thread in detail yet, I don't understand the idea/need of creating AppendRelInfos for partitioned partitions, though.) Though I haven't read the patch yet, I think the above code is useless. And I proposed a patch to clean it up before [1]. I'll add that patch to the next commitfest. +1. +1. Will Fujita-san's patch also handle getting rid of partitioned_rels list? No. The patch just removes the partitioned_rels list from nodeModifyTable.c. 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] Useless code in ExecInitModifyTable
On 2017/06/21 17:57, Amit Langote wrote:
On 2017/06/21 16:59, Etsuro Fujita wrote:
Commit d3cc37f1d801a6b5cad9bf179274a8d767f1ee50 added this to
ExecInitModifyTable:
+ /* The root table RT index is at the head of the partitioned_rels list */
+ if (node->partitioned_rels)
+ {
+ Index root_rti;
+ Oid root_oid;
+
+ root_rti = linitial_int(node->partitioned_rels);
+ root_oid = getrelid(root_rti, estate->es_range_table);
+ rel = heap_open(root_oid, NoLock); /* locked by InitPlan */
+ }
but I noticed that that function doesn't use the relation descriptor at
all. Since partitioned_rels is given in case of an UPDATE/DELETE on a
partitioned table, the relation is opened in that case, but the relation
descriptor isn't referenced at all in initializing WITH CHECK OPTION
constraints and/or RETURNING projections. (The mtstate->resultRelinfo
array is referenced in those initialization, instead.) So, I'd like to
propose to remove this from that function. Attached is a patch for that.
Thanks for cleaning that up. I cannot see any problem in applying the patch.
I noticed that the patch needs to be rebased. Updated patch attached.
Thanks for the review!
Best regards,
Etsuro Fujita
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***
*** 45,51
#include "foreign/fdwapi.h"
#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
- #include "parser/parsetree.h"
#include "storage/bufmgr.h"
#include "storage/lmgr.h"
#include "utils/builtins.h"
--- 45,50
***
*** 1894,1913 ExecInitModifyTable(ModifyTable *node, EState *estate, int
eflags)
estate->es_result_relation_info = saved_resultRelInfo;
- /* The root table RT index is at the head of the partitioned_rels list
*/
- if (node->partitioned_rels)
- {
- Index root_rti;
- Oid root_oid;
-
- root_rti = linitial_int(node->partitioned_rels);
- root_oid = getrelid(root_rti, estate->es_range_table);
- rel = heap_open(root_oid, NoLock); /* locked by InitPlan */
- }
- else
- rel = mtstate->resultRelInfo->ri_RelationDesc;
-
/* Build state for INSERT tuple routing */
if (operation == CMD_INSERT &&
rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
--- 1893,1900
estate->es_result_relation_info = saved_resultRelInfo;
/* Build state for INSERT tuple routing */
+ rel = mtstate->resultRelInfo->ri_RelationDesc;
if (operation == CMD_INSERT &&
rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
***
*** 2091,2100 ExecInitModifyTable(ModifyTable *node, EState *estate, int
eflags)
mtstate->ps.ps_ExprContext = NULL;
}
- /* Close the root partitioned rel if we opened it above. */
- if (rel != mtstate->resultRelInfo->ri_RelationDesc)
- heap_close(rel, NoLock);
-
/*
* If needed, Initialize target list, projection and qual for ON
CONFLICT
* DO UPDATE.
--- 2078,2083
--
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] Partition-wise join for join between (declaratively) partitioned tables
On 2017/09/02 4:10, Ashutosh Bapat wrote:
This rebase mainly changes patch 0001, which translates partition
hierarchy into inheritance hierarchy creating AppendRelInfos and
RelOptInfos for partitioned partitions. Because of that, it's not
necessary to record the partitioned partitions in a
PartitionedChildRelInfos::child_rels. The only RTI that goes in there
is the RTI of child RTE which is same as the parent RTE except inh
flag. I tried removing that with a series of changes but it seems that
following code in ExecInitModifyTable() requires it.
1897 /* The root table RT index is at the head of the
partitioned_rels list */
1898 if (node->partitioned_rels)
1899 {
1900 Index root_rti;
1901 Oid root_oid;
1902
1903 root_rti = linitial_int(node->partitioned_rels);
1904 root_oid = getrelid(root_rti, estate->es_range_table);
1905 rel = heap_open(root_oid, NoLock); /* locked by InitPlan */
1906 }
1907 else
1908 rel = mtstate->resultRelInfo->ri_RelationDesc;
I don't know whether we could change this code not to use
PartitionedChildRelInfos::child_rels.
Though I haven't read the patch yet, I think the above code is useless.
And I proposed a patch to clean it up before [1]. I'll add that patch
to the next commitfest.
Best regards,
Etsuro Fujita
[1]
https://www.postgresql.org/message-id/93cf9816-2f7d-0f67-8ed2-4a4e497a6ab8%40lab.ntt.co.jp
--
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] Minor code improvement to postgresGetForeignPlan
On 2017/04/07 13:12, Tatsuro Yamada wrote:> The declaration of postgresGetForeignPlan uses baserel, but the actual definition uses foreignrel. It would be better to sync. Agreed. Please find attached a patch. The patch looks good to me, so I'll mark this as Ready for Committer. (I'm not sure we should do the same thing to the function declaration in other places such as fdwapi.h and the documentation for consistency, but if so, I'd vote for leaving that for another patch.) Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Update comment in ExecPartitionCheck
On 2017/08/26 2:28, Robert Haas wrote:
On Tue, Jul 4, 2017 at 4:55 AM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
This comment in an error handling in ExecPartitionCheck():
if (!ExecCheck(resultRelInfo->ri_PartitionCheckExpr, econtext))
{
char *val_desc;
Relationorig_rel = rel;
/* See the comment above. */
if (resultRelInfo->ri_PartitionRoot)
should be updated because we don't have any comment on that above in the
code. Since we have a comment on that in ExecConstraints() defined just
below that function, I think the comment should be something like this: "See
the comment in ExecConstraints().". Attached is a patch for that.
Hrm. I'm not sure I understand which comment in ExecConstraints()
this is supposed to refer to. Maybe we need to think a bit harder
about how to make this clear.
The comment in ExecConstraints is this:
/*
* If the tuple has been routed, it's been converted to the
* partition's rowtype, which might differ from the root
* table's. We must convert it back to the root table's
* rowtype so that val_desc shown error message matches the
* input tuple.
*/
if (resultRelInfo->ri_PartitionRoot)
How about replacing the comment "See the comment above." in
ExecPartitionCheck with something like this: "If the tuple has been
routed, convert it from the partition's rowtype to the root table's. See
the comment in ExecConstraints().". I think that would make it easy to
specify that comment in ExecConstrains. I'd like to propose to update
the same comments in other places as well, just for consistency.
PFA an updated version of the patch.
Best regards,
Etsuro Fujita
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***
*** 1884,1890 ExecPartitionCheck(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot,
char *val_desc;
Relationorig_rel = rel;
! /* See the comment above. */
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple tuple = ExecFetchSlotTuple(slot);
--- 1884,1893
char *val_desc;
Relationorig_rel = rel;
! /*
!* If the tuple has been routed, convert it from the partition's
!* rowtype to the root table's. See the comment in
ExecConstraints().
!*/
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple tuple = ExecFetchSlotTuple(slot);
***
*** 2011,2017 ExecConstraints(ResultRelInfo *resultRelInfo,
char *val_desc;
Relationorig_rel = rel;
! /* See the comment above. */
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple tuple =
ExecFetchSlotTuple(slot);
--- 2014,2023
char *val_desc;
Relationorig_rel = rel;
! /*
!* If the tuple has been routed, convert it from the
partition's
!* rowtype to the root table's. See the comment above.
!*/
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple tuple =
ExecFetchSlotTuple(slot);
***
*** 2121,2127 ExecWithCheckOptions(WCOKind kind, ResultRelInfo
*resultRelInfo,
* USING policy.
*/
case WCO_VIEW_CHECK:
! /* See the comment in
ExecConstraints(). */
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple tuple =
ExecFetchSlotTuple(slot);
--- 2127,2137
* USING policy.
*/
case WCO_VIEW_CHECK:
! /*
!* If the tuple has been routed,
convert it from the
!* partition's rowtype to the root
table's. See the
!* comment in ExecConstraints().
!*/
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple
Re: [HACKERS] Add support for tuple routing to foreign partitions
On 2017/08/26 1:43, Robert Haas wrote: On Sun, Aug 20, 2017 at 11:25 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: I agree, but I wonder if we ought to make it work first using the existing APIs and then add these new APIs as an optimization. I'm not sure that's a good idea because that once we support INSERT tuple-routing for foreign partitions, we would have a workaround: INSERT INTO partitioned_table SELECT * from data_table where data_table is a foreign table defined for an input file using file_fdw. That's true, but I don't see how it refutes the point I was trying to raise. My concern is: the existing APIs can really work well for any FDW to do COPY tuple-routing? I know the original version of the patch showed the existing APIs would work well for postgres_fdw but nothing beyond. For example: the original version made a dummy Query/Plan only containing a leaf partition as its result relation, and passed it to PlanForeignModify, IIRC. That would work well for postgres_fdw because it doesn't look at the Query/Plan except the result relation, but might not for other FDWs that look at more stuff of the given Query/Plan and do something based on that information. Some FDW might check to see whether the Plan is to do INSERT .. VALUES with a single VALUES sublist or INSERT .. VALUES with multiple VALUES sublists, for optimizing remote INSERT, for example. 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] Tuple-routing for certain partitioned tables not working as expected
On 2017/08/30 9:13, Amit Langote wrote: On 2017/08/29 20:18, Etsuro Fujita wrote: On 2017/08/25 22:26, Robert Haas wrote: On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: Agreed, but I'd vote for fixing this in v10 as proposed; I agree that just ripping the CheckValidResultRel checks out entirely is not a good idea, but that seems OK to me at least as a fix just for v10. I'm still not on-board with having this be the one case where we don't do CheckValidResultRel. If we want to still call it but pass down some additional information that can selectively skip certain checks, I could probably live with that. Another idea would be to not do CheckValidResultRel for partitions in ExecSetupPartitionTupleRouting; instead, do that the first time the partition is chosen by ExecFindPartition, and if successfully checked, initialize the partition's ResultRelInfo and other stuff. (We could skip this after the first time by checking whether we already have a valid ResultRelInfo for the chosen partition.) That could allow us to not only call CheckValidResultRel the way it is, but avoid initializing useless partitions for which tuples aren't routed at all. I too have thought about the idea of lazy initialization of the partition ResultRelInfos. I think it would be a good idea, but definitely something for PG 11. Agreed. Here is a patch to skip the CheckValidResultRel checks for a target table if it's a foreign partition to perform tuple-routing for, as proposed by Robert. Best regards, Etsuro Fujita *** /dev/null --- b/contrib/file_fdw/data/list1.csv *** *** 0 --- 1,2 + 1,foo + 1,bar *** /dev/null --- b/contrib/file_fdw/data/list2.bad *** *** 0 --- 1,2 + 2,baz + 1,qux *** /dev/null --- b/contrib/file_fdw/data/list2.csv *** *** 0 --- 1,2 + 2,baz + 2,qux *** a/contrib/file_fdw/input/file_fdw.source --- b/contrib/file_fdw/input/file_fdw.source *** *** 162,167 SELECT tableoid::regclass, * FROM agg FOR UPDATE; --- 162,188 ALTER FOREIGN TABLE agg_csv NO INHERIT agg; DROP TABLE agg; + -- declarative partitioning tests + SET ROLE regress_file_fdw_superuser; + CREATE TABLE pt (a int, b text) partition by list (a); + CREATE FOREIGN TABLE p1 partition of pt for values in (1) SERVER file_server + OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ','); + CREATE TABLE p2 partition of pt for values in (2); + SELECT tableoid::regclass, * FROM pt; + SELECT tableoid::regclass, * FROM p1; + SELECT tableoid::regclass, * FROM p2; + COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR + COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ','); + SELECT tableoid::regclass, * FROM pt; + SELECT tableoid::regclass, * FROM p1; + SELECT tableoid::regclass, * FROM p2; + INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR + INSERT INTO pt VALUES (2, 'xyzzy'); + SELECT tableoid::regclass, * FROM pt; + SELECT tableoid::regclass, * FROM p1; + SELECT tableoid::regclass, * FROM p2; + DROP TABLE pt; + -- privilege tests SET ROLE regress_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 *** *** 289,294 SELECT tableoid::regclass, * FROM agg FOR UPDATE; --- 289,375 ALTER FOREIGN TABLE agg_csv NO INHERIT agg; DROP TABLE agg; + -- declarative partitioning tests + SET ROLE regress_file_fdw_superuser; + CREATE TABLE pt (a int, b text) partition by list (a); + CREATE FOREIGN TABLE p1 partition of pt for values in (1) SERVER file_server + OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ','); + CREATE TABLE p2 partition of pt for values in (2); + SELECT tableoid::regclass, * FROM pt; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + (2 rows) + + SELECT tableoid::regclass, * FROM p1; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + (2 rows) + + SELECT tableoid::regclass, * FROM p2; + tableoid | a | b + --+---+--- + (0 rows) + + COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR + ERROR: cannot route inserted tuples to a foreign table + CONTEXT: COPY pt, line 2: "1,qux" + COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ','); + SELECT tableoid::regclass, * FROM pt; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + p2 | 2 | baz + p2 | 2 | qux + (4 rows) + + SELECT tableoid::regclass, * FROM p1; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + (2 rows) + + SELECT tableoid::regclass, * FROM p2; + tableoid | a | b + --+---+- + p2 | 2 | baz + p2 | 2 | qux + (2 rows) + + INSERT INTO pt V
Re: [HACKERS] Tuple-routing for certain partitioned tables not working as expected
On 2017/08/25 22:26, Robert Haas wrote: On Wed, Aug 23, 2017 at 4:55 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: Agreed, but I'd vote for fixing this in v10 as proposed; I agree that just ripping the CheckValidResultRel checks out entirely is not a good idea, but that seems OK to me at least as a fix just for v10. I'm still not on-board with having this be the one case where we don't do CheckValidResultRel. If we want to still call it but pass down some additional information that can selectively skip certain checks, I could probably live with that. Another idea would be to not do CheckValidResultRel for partitions in ExecSetupPartitionTupleRouting; instead, do that the first time the partition is chosen by ExecFindPartition, and if successfully checked, initialize the partition's ResultRelInfo and other stuff. (We could skip this after the first time by checking whether we already have a valid ResultRelInfo for the chosen partition.) That could allow us to not only call CheckValidResultRel the way it is, but avoid initializing useless partitions for which tuples aren't routed at all. At some point we've got to stop developing v10 and just let it be what it is. I agree on that point, but ISTM that this is more like a bug. 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] postgres_fdw bug in 9.6
On 2017/08/21 20:37, Etsuro Fujita wrote:
Attached is an updated version of the patch.
I corrected some comments. New version attached.
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 1701,1722 SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 =
t2.c1) ORDER BY t1.c3, t
Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
Remote SQL: SELECT r1."C 1", r1.c3, CASE WHEN (r1.*)::text IS
NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8)
END, r2."C 1", CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2,
r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN
"S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY r1.c3 ASC NULLS LAST,
r1."C 1" ASC NULLS LAST FOR UPDATE OF r1
!-> Merge Join
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
! Merge Cond: (t1.c1 = t2.c1)
! -> Sort
Output: t1.c1, t1.c3, t1.*
!Sort Key: t1.c1
!-> Foreign Scan on public.ft1 t1
! Output: t1.c1, t1.c3, t1.*
! Remote SQL: SELECT "C 1", c2, c3, c4, c5,
c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
! -> Sort
Output: t2.c1, t2.*
!Sort Key: t2.c1
!-> Foreign Scan on public.ft2 t2
! Output: t2.c1, t2.*
! Remote SQL: SELECT "C 1", c2, c3, c4, c5,
c6, c7, c8 FROM "S 1"."T 1"
! (23 rows)
SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY
t1.c3, t1.c1 OFFSET 100 LIMIT 10 FOR UPDATE OF t1;
c1 | c1
--- 1701,1716
Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
Remote SQL: SELECT r1."C 1", r1.c3, CASE WHEN (r1.*)::text IS
NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8)
END, r2."C 1", CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2,
r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN
"S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY r1.c3 ASC NULLS LAST,
r1."C 1" ASC NULLS LAST FOR UPDATE OF r1
!-> Nested Loop
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
! Join Filter: (t1.c1 = t2.c1)
! -> Foreign Scan on public.ft1 t1
Output: t1.c1, t1.c3, t1.*
!Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7,
c8 FROM "S 1"."T 1" FOR UPDATE
! -> Foreign Scan on public.ft2 t2
Output: t2.c1, t2.*
!Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7,
c8 FROM "S 1"."T 1"
! (17 rows)
SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY
t1.c3, t1.c1 OFFSET 100 LIMIT 10 FOR UPDATE OF t1;
c1 | c1
***
*** 1745,1766 SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 =
t2.c1) ORDER BY t1.c3, t
Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
Remote SQL: SELECT r1."C 1", r1.c3, CASE WHEN (r1.*)::text IS
NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8)
END, r2."C 1", CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2,
r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN
"S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY r1.c3 ASC NULLS LAST,
r1."C 1" ASC NULLS LAST FOR UPDATE OF r1 FOR UPDATE OF r2
!-> Merge Join
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
! Merge Cond: (t1.c1 = t2.c1)
! -> Sort
Output: t1.c1, t1.c3, t1.*
!Sort Key: t1.c1
!-> Foreign Scan on public.ft1 t1
! Output: t1.c1, t1.c3, t1.*
! Remote SQL: SELECT "C 1", c2, c3, c4, c5,
c6, c7, c8 FROM "S 1&quo
Re: [HACKERS] Tuple-routing for certain partitioned tables not working as expected
On 2017/08/22 9:55, Amit Langote wrote: On 2017/08/22 1:08, Robert Haas wrote: On Mon, Aug 21, 2017 at 7:45 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: If there are no objections, I'll add this to the open item list for v10. This seems fairly ad-hoc to me. I mean, now you have CheckValidResultRel not being called in just this one case -- but that bypasses all the checks that function might do, not just this one. It so happens that's OK at the moment because CheckCmdReplicaIdentity() doesn't do anything in the insert case. I'm somewhat inclined to just view this as a limitation of v10 and fix it in v11. That limitation seems too restrictive to me. If you want to fix it in v10, I think we need a different approach -- just ripping the CheckValidResultRel checks out entirely doesn't seem like a good idea to me. Before 389af951552f, the relkind check that is now performed by CheckValidResultRel(), used to be done in InitResultRelInfo(). ISTM, it was separated out so that certain ResultRelInfos could be initialized without the explicit relkind check, either because it's taken care of elsewhere or the table in question is *known* to be a valid result relation. Maybe, mostly just the former of the two reasons when that commit went in. IMO, the latter case applies when initializing ResultRelInfos for partitions during tuple-routing, because the table types we allow to become partitions are fairly restricted. Agreed. Also, it seems okay to show the error messages that CheckValidResultRel() shows when the concerned table is *directly* addressed in a query, but the same error does not seem so user-friendly when emitted for one of the partitions while tuple-routing is being set up. IMHO, there should be "tuple routing" somewhere in the error message shown in that case, even if it's for the total lack of support for inserts by a FDW. Agreed, but I'd vote for fixing this in v10 as proposed; I agree that just ripping the CheckValidResultRel checks out entirely is not a good idea, but that seems OK to me at least as a fix just for v10. 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] Tuple-routing for certain partitioned tables not working as expected
On 2017/08/07 15:45, Etsuro Fujita wrote: On 2017/08/07 15:33, Amit Langote wrote: On 2017/08/07 15:22, Etsuro Fujita wrote: On 2017/08/07 13:11, Amit Langote wrote:> The patch looks good too. Although, looking at the following hunk: + Assert(partrel->rd_rel->relkind == RELKIND_RELATION || + partrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE); + /* * Verify result relation is a valid target for the current operation. */ ! if (partrel->rd_rel->relkind == RELKIND_RELATION) ! CheckValidResultRel(partrel, CMD_INSERT); makes me now wonder if we need the CheckValidResultRel check at all. The only check currently in place for RELKIND_RELATION is CheckCmdReplicaIdentity(), which is a noop (returns true) for inserts anyway. Good point! I left the verification for a plain table because that is harmless but as you mentioned, that is nothing but an overhead. So, here is a new version which removes the verification at all from ExecSetupPartitionTupleRouting. The updated patch looks good to me, thanks. Thanks for the review! If there are no objections, I'll add this to the open item list for v10. 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] postgres_fdw bug in 9.6
On 2017/04/08 4:24, Robert Haas wrote:
Looking at the code itself, I find the changes to joinpath.c rather alarming.
I missed this mail. Sorry about that, Robert.
+/* Save hashclauses for possible use by the FDW */
+if (extra->consider_foreignjoin && hashclauses)
+extra->hashclauses = hashclauses;
A minor consideration is that this is fairly far away from where
hashclauses actually gets populated, so if someone later added an
early "return" statement to this function -- after creating some paths
-- it could subtly break join pushdown. But I also think there's no
real need for this. The loop that extracts hash clauses is simple
enough that we could just refactor it into a separate function, or if
necessary duplicate the logic.
I refactored that into a new function so that we can call that function
at the top of add_paths_to_joinrel and store the result in
JoinPathExtraData.
+/* Save first mergejoin data for possible use by the FDW */
+if (extra->consider_foreignjoin && outerkeys == all_pathkeys)
+{
+extra->mergeclauses = cur_mergeclauses;
+extra->outersortkeys = outerkeys;
+extra->innersortkeys = innerkeys;
+}
Similarly here. select_outer_pathkeys_for_merge(),
find_mergeclauses_for_pathkeys(), and make_inner_pathkeys_for_merge()
are all extern, so there's nothing to keep CreateLocalJoinPath() from
just doing that work itself instead of getting help from joinpath,
which I guess seems better to me. I think it's just better if we
don't burden joinpath.c with keeping little bits of data around that
CreateLocalJoinPath() can easily get for itself.
Done that way.
There appears to be no regression test covering the case where we get
a Merge Full Join with a non-empty list of mergeclauses. Hash Full
Join is tested, as is Merge Full Join without merge clauses, but
there's no test for Merge Full Join with mergeclauses, and since that
is a separate code path it seems like it should be tested.
Done.
-/*
- * If either inner or outer path is a ForeignPath corresponding to a
- * pushed down join, replace it with the fdw_outerpath, so that we
- * maintain path for EPQ checks built entirely of local join
- * strategies.
- */
-if (IsA(joinpath->outerjoinpath, ForeignPath))
-{
-ForeignPath *foreign_path;
-
-foreign_path = (ForeignPath *) joinpath->outerjoinpath;
-if (IS_JOIN_REL(foreign_path->path.parent))
-joinpath->outerjoinpath = foreign_path->fdw_outerpath;
-}
-
-if (IsA(joinpath->innerjoinpath, ForeignPath))
-{
-ForeignPath *foreign_path;
-
-foreign_path = (ForeignPath *) joinpath->innerjoinpath;
-if (IS_JOIN_REL(foreign_path->path.parent))
-joinpath->innerjoinpath = foreign_path->fdw_outerpath;
-}
This logic is removed and not replaced with anything, but I don't see
what keeps this code...
+Path *outer_path = outerrel->cheapest_total_path;
+Path *inner_path = innerrel->cheapest_total_path;
...from picking a ForeignPath?
CreateLocalJoinPath creates an alternative local join path for a foreign
join from the cheapest total paths for the outer/inner relations. The
reason for the above is to pass these paths to that function. On second
thought, however, I think it would be convenient for the caller to just
pass outerrel/innerrel to that function. So, I modified that function's
API as such. Another change is: the previous version of that function
allowed the caller to create a parameterized local-join path
corresponding to a parameterized foreign join, but that is a feature,
not a bug fix, so I dropped that. (I'll propose that as part of the
patch in [1].)
There's probably more to think about here, but those are my question
on an initial read-through.
Thanks for the review!
Attached is an updated version of the patch.
Best regards,
Etsuro Fujita
[1] https://commitfest.postgresql.org/14/1042/
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 1701,1722 SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 =
t2.c1) ORDER BY t1.c3, t
Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
Remote SQL: SELECT r1."C 1", r1.c3, CASE WHEN (r1.*)::text IS
NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8)
END, r2."C 1", CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2,
r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN
"S 1"."T 1" r2 ON (((r1."C 1" = r2."
[HACKERS] Re: Stats for triggers on partitioned tables not shown in EXPLAIN ANALYZE
On 2017/08/21 9:18, Amit Langote wrote: On 2017/08/19 2:09, Robert Haas wrote: On Fri, Aug 18, 2017 at 1:15 AM, Noah Misch <n...@leadboat.com> wrote: The above-described topic is currently a PostgreSQL 10 open item. Robert, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com Committed and back-patched the proposed patch. 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] Add support for tuple routing to foreign partitions
On 2017/08/19 2:12, Robert Haas wrote: On Thu, Aug 17, 2017 at 4:27 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: I think that would be much more efficient than INSERTing tuples into the remote server one by one. What do you think about that? I agree, but I wonder if we ought to make it work first using the existing APIs and then add these new APIs as an optimization. I'm not sure that's a good idea because that once we support INSERT tuple-routing for foreign partitions, we would have a workaround: INSERT INTO partitioned_table SELECT * from data_table where data_table is a foreign table defined for an input file using file_fdw. postgres_fdw isn't the only FDW in the world, and it's better if getting a working implementation doesn't require too many interfaces. Agreed. My vote would be to leave the COPY part as-is until we have these new APIs. 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] Add support for tuple routing to foreign partitions
On 2017/08/18 22:41, David Fetter wrote: On Fri, Aug 18, 2017 at 05:10:29PM +0900, Etsuro Fujita wrote: On 2017/08/17 23:48, David Fetter wrote: On Thu, Aug 17, 2017 at 05:27:05PM +0900, Etsuro Fujita wrote: On 2017/07/11 6:56, Robert Haas wrote: On Thu, Jun 29, 2017 at 6:20 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: So, I dropped the COPY part. Ouch. I think we should try to figure out how the COPY part will be handled before we commit to a design. I spent some time on this. To handle that, I'd like to propose doing something similar to \copy (frontend copy): submit a COPY query "COPY ... >FROM STDIN" to the remote server and route data from a file to the remote server. For that, I'd like to add new FDW APIs called during CopyFrom that allow us to copy to foreign tables: * BeginForeignCopyIn: this would be called after creating a ResultRelInfo for the target table (or each leaf partition of the target partitioned table) if it's a foreign table, and perform any initialization needed before the remote copy can start. In the postgres_fdw case, I think this function would be a good place to send "COPY ... FROM STDIN" to the remote server. * ExecForeignCopyInOneRow: this would be called instead of heap_insert if the target is a foreign table, and route the tuple read from the file by NextCopyFrom to the remote server. In the postgres_fdw case, I think this function would convert the tuple to text format for portability, and then send the data to the remote server using PQputCopyData. * EndForeignCopyIn: this would be called at the bottom of CopyFrom, and release resources such as connections to the remote server. In the postgres_fdw case, this function would do PQputCopyEnd to terminate data transfer. These primitives look good. I know it seems unlikely at first blush, but do we know of bulk load APIs for non-PostgreSQL data stores that this would be unable to serve? Maybe I'm missing something, but I think these would allow the FDW to do the remote copy the way it would like; in ExecForeignCopyInOneRow, for example, the FDW could buffer tuples in a buffer memory and transmit the buffered data to the remote server if the data size exceeds a threshold. The naming is not so good? Suggestions are welcome. The naming seems reasonable. I was trying to figure out whether this fits well enough with the bulk load APIs for databases other than PostgreSQL. I'm guessing it's "well enough" based on checking MySQL, Oracle, and MS SQL Server. Good to know. 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] Add support for tuple routing to foreign partitions
On 2017/08/17 23:48, David Fetter wrote: On Thu, Aug 17, 2017 at 05:27:05PM +0900, Etsuro Fujita wrote: On 2017/07/11 6:56, Robert Haas wrote: On Thu, Jun 29, 2017 at 6:20 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: So, I dropped the COPY part. Ouch. I think we should try to figure out how the COPY part will be handled before we commit to a design. I spent some time on this. To handle that, I'd like to propose doing something similar to \copy (frontend copy): submit a COPY query "COPY ... FROM STDIN" to the remote server and route data from a file to the remote server. For that, I'd like to add new FDW APIs called during CopyFrom that allow us to copy to foreign tables: * BeginForeignCopyIn: this would be called after creating a ResultRelInfo for the target table (or each leaf partition of the target partitioned table) if it's a foreign table, and perform any initialization needed before the remote copy can start. In the postgres_fdw case, I think this function would be a good place to send "COPY ... FROM STDIN" to the remote server. * ExecForeignCopyInOneRow: this would be called instead of heap_insert if the target is a foreign table, and route the tuple read from the file by NextCopyFrom to the remote server. In the postgres_fdw case, I think this function would convert the tuple to text format for portability, and then send the data to the remote server using PQputCopyData. * EndForeignCopyIn: this would be called at the bottom of CopyFrom, and release resources such as connections to the remote server. In the postgres_fdw case, this function would do PQputCopyEnd to terminate data transfer. These primitives look good. I know it seems unlikely at first blush, but do we know of bulk load APIs for non-PostgreSQL data stores that this would be unable to serve? Maybe I'm missing something, but I think these would allow the FDW to do the remote copy the way it would like; in ExecForeignCopyInOneRow, for example, the FDW could buffer tuples in a buffer memory and transmit the buffered data to the remote server if the data size exceeds a threshold. The naming is not so good? Suggestions are welcome. 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] postgres_fdw bug in 9.6
On 2017/08/18 8:16, Michael Paquier wrote: On Fri, Aug 18, 2017 at 3:59 AM, Tom Lane <t...@sss.pgh.pa.us> wrote: Robert Haas <robertmh...@gmail.com> writes: On Thu, Aug 17, 2017 at 8:00 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: I rebased the patch to HEAD. PFA a new version of the patch. Tom, you were instrumental in identifying what was going wrong here initially. Any chance you'd be willing to have a look at the patch? I will, but probably not for a week or so. Going eclipse-chasing. Good luck: https://xkcd.com/1876/ And enjoy: https://xkcd.com/1877/ Enjoy! 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] Garbled comment in postgresGetForeignJoinPaths
On 2017/08/17 1:31, Tom Lane wrote: postgres_fdw.c around line 4500: /* * If there is a possibility that EvalPlanQual will be executed, we need * to be able to reconstruct the row using scans of the base relations. * GetExistingLocalJoinPath will find a suitable path for this purpose in * the path list of the joinrel, if one exists. We must be careful to * call it before adding any ForeignPath, since the ForeignPath might * dominate the only suitable local path available. We also do it before --> * reconstruct the row for EvalPlanQual(). Find an alternative local path * calling foreign_join_ok(), since that function updates fpinfo and marks * it as pushable if the join is found to be pushable. */ Should the marked line simply be deleted? If not, what correction is appropriate? In relation to this, I updated the patch for addressing the GetExistingLocalJoinPath issue [1]. Best regards, Etsuro Fujita [1] https://www.postgresql.org/message-id/6008ca34-906f-e61d-478b-8f85fde2c090%40lab.ntt.co.jp -- 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] postgres_fdw bug in 9.6
On 2017/04/04 18:01, Etsuro Fujita wrote:
I rebased the patch also. Please find attached an updated version of
the patch.
I rebased the patch to HEAD. PFA a new version of the patch.
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 1701,1722 SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 =
t2.c1) ORDER BY t1.c3, t
Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
Remote SQL: SELECT r1."C 1", r1.c3, CASE WHEN (r1.*)::text IS
NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8)
END, r2."C 1", CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2,
r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN
"S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY r1.c3 ASC NULLS LAST,
r1."C 1" ASC NULLS LAST FOR UPDATE OF r1
!-> Merge Join
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
! Merge Cond: (t1.c1 = t2.c1)
! -> Sort
Output: t1.c1, t1.c3, t1.*
!Sort Key: t1.c1
!-> Foreign Scan on public.ft1 t1
! Output: t1.c1, t1.c3, t1.*
! Remote SQL: SELECT "C 1", c2, c3, c4, c5,
c6, c7, c8 FROM "S 1"."T 1" FOR UPDATE
! -> Sort
Output: t2.c1, t2.*
!Sort Key: t2.c1
!-> Foreign Scan on public.ft2 t2
! Output: t2.c1, t2.*
! Remote SQL: SELECT "C 1", c2, c3, c4, c5,
c6, c7, c8 FROM "S 1"."T 1"
! (23 rows)
SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY
t1.c3, t1.c1 OFFSET 100 LIMIT 10 FOR UPDATE OF t1;
c1 | c1
--- 1701,1716
Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
Remote SQL: SELECT r1."C 1", r1.c3, CASE WHEN (r1.*)::text IS
NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8)
END, r2."C 1", CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2,
r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN
"S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY r1.c3 ASC NULLS LAST,
r1."C 1" ASC NULLS LAST FOR UPDATE OF r1
!-> Nested Loop
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
! Join Filter: (t1.c1 = t2.c1)
! -> Foreign Scan on public.ft1 t1
Output: t1.c1, t1.c3, t1.*
!Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7,
c8 FROM "S 1"."T 1" FOR UPDATE
! -> Foreign Scan on public.ft2 t2
Output: t2.c1, t2.*
!Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7,
c8 FROM "S 1"."T 1"
! (17 rows)
SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY
t1.c3, t1.c1 OFFSET 100 LIMIT 10 FOR UPDATE OF t1;
c1 | c1
***
*** 1745,1766 SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 =
t2.c1) ORDER BY t1.c3, t
Output: t1.c1, t2.c1, t1.c3, t1.*, t2.*
Relations: (public.ft1 t1) INNER JOIN (public.ft2 t2)
Remote SQL: SELECT r1."C 1", r1.c3, CASE WHEN (r1.*)::text IS
NOT NULL THEN ROW(r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, r1.c7, r1.c8)
END, r2."C 1", CASE WHEN (r2.*)::text IS NOT NULL THEN ROW(r2."C 1", r2.c2,
r2.c3, r2.c4, r2.c5, r2.c6, r2.c7, r2.c8) END FROM ("S 1"."T 1" r1 INNER JOIN
"S 1"."T 1" r2 ON (((r1."C 1" = r2."C 1" ORDER BY r1.c3 ASC NULLS LAST,
r1."C 1" ASC NULLS LAST FOR UPDATE OF r1 FOR UPDATE OF r2
!-> Merge Join
Output: t1.c1, t1.c3, t1.*, t2.c1, t2.*
! Merge Cond: (t1.c1 = t2.c1)
! -> Sort
Output: t1.c1, t1.c3, t1.*
!Sort Key: t1.c1
!-> Foreign Scan on public.ft1 t1
! Output: t1.c1, t1.c3, t1.*
! Remote SQL: SELECT "C 1", c2
Re: [HACKERS] Add support for tuple routing to foreign partitions
On 2017/08/17 20:37, Ashutosh Bapat wrote: On Thu, Aug 17, 2017 at 1:57 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: I spent some time on this. To handle that, I'd like to propose doing something similar to \copy (frontend copy): submit a COPY query "COPY ... FROM STDIN" to the remote server and route data from a file to the remote server. For that, I'd like to add new FDW APIs called during CopyFrom that allow us to copy to foreign tables: The description seems to support only COPY to a foreign table from a file, but probably we need the support other way round as well. This looks like a feature (support copy to and from a foreign table) to be handled by itself. Agreed. I'll consider how to handle copy-from-a-foreign-table as well. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support for tuple routing to foreign partitions
On 2017/07/11 6:56, Robert Haas wrote: On Thu, Jun 29, 2017 at 6:20 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: So, I dropped the COPY part. Ouch. I think we should try to figure out how the COPY part will be handled before we commit to a design. I spent some time on this. To handle that, I'd like to propose doing something similar to \copy (frontend copy): submit a COPY query "COPY ... FROM STDIN" to the remote server and route data from a file to the remote server. For that, I'd like to add new FDW APIs called during CopyFrom that allow us to copy to foreign tables: * BeginForeignCopyIn: this would be called after creating a ResultRelInfo for the target table (or each leaf partition of the target partitioned table) if it's a foreign table, and perform any initialization needed before the remote copy can start. In the postgres_fdw case, I think this function would be a good place to send "COPY ... FROM STDIN" to the remote server. * ExecForeignCopyInOneRow: this would be called instead of heap_insert if the target is a foreign table, and route the tuple read from the file by NextCopyFrom to the remote server. In the postgres_fdw case, I think this function would convert the tuple to text format for portability, and then send the data to the remote server using PQputCopyData. * EndForeignCopyIn: this would be called at the bottom of CopyFrom, and release resources such as connections to the remote server. In the postgres_fdw case, this function would do PQputCopyEnd to terminate data transfer. I think that would be much more efficient than INSERTing tuples into the remote server one by one. What do you think about that? I have to admit that I'm a little bit fuzzy about why foreign insert routing requires all of these changes. I think this patch would benefit from being accompanied by several paragraphs of explanation outlining the rationale for each part of the patch. Will do. 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
[HACKERS] Stats for triggers on partitioned tables not shown in EXPLAIN ANALYZE
Hi hackers,
I noticed that runtime stats for BEFORE ROW INSERT triggers on leaf
partitions of partitioned tables aren't reported in EXPLAIN ANALYZE.
Here is an example:
postgres=# create table trigger_test (a int, b text) partition by list (a);
CREATE TABLE
postgres=# create table trigger_test1 partition of trigger_test for
values in (1);
CREATE TABLE
postgres=# create trigger before_ins_row_trig BEFORE INSERT ON
trigger_test1 FOR EACH ROW EXECUTE PROCEDURE trigger_data();
CREATE TRIGGER
postgres=# create trigger after_ins_row_trig AFTER INSERT ON
trigger_test1 FOR EACH ROW EXECUTE PROCEDURE trigger_data();
CREATE TRIGGER
postgres=# explain analyze insert into trigger_test values (1, 'foo');
NOTICE: before_ins_row_trig() BEFORE ROW INSERT ON trigger_test1
NOTICE: NEW: (1,foo)
NOTICE: after_ins_row_trig() AFTER ROW INSERT ON trigger_test1
NOTICE: NEW: (1,foo)
QUERY PLAN
-
Insert on trigger_test (cost=0.00..0.01 rows=1 width=36) (actual
time=0.193..0.193 rows=0 loops=1)
-> Result (cost=0.00..0.01 rows=1 width=36) (actual
time=0.002..0.003 rows=1 loops=1)
Planning time: 0.027 ms
Trigger after_ins_row_trig on trigger_test1: time=0.075 calls=1
Execution time: 0.310 ms
(5 rows)
where trig_data() is borrowed from the regression test in postgres_fdw.
The stats for the AFTER ROW INSERT trigger after_ins_row_trig are well
shown in the output, but the stats for the BEFORE ROW INSERT trigger
before_ins_row_trig aren't at all. I think we should show the latter as
well.
Another thing I noticed is: runtime stats for BEFORE STATEMENT
UPDATE/DELETE triggers on partitioned table roots aren't reported in
EXPLAIN ANALYZE, either, as shown in a below example:
postgres=# create trigger before_upd_stmt_trig BEFORE UPDATE ON
trigger_test FOR EACH STATEMENT EXECUTE PROCEDURE trigger_func();
CREATE TRIGGER
postgres=# create trigger after_upd_stmt_trig AFTER UPDATE ON
trigger_test FOR EACH STATEMENT EXECUTE PROCEDURE trigger_func();
CREATE TRIGGER
postgres=# explain analyze update trigger_test set b = 'bar' where a = 1;
NOTICE: trigger_func() called: action = UPDATE, when = BEFORE,
level = STATEMENT
NOTICE: trigger_func() called: action = UPDATE, when = AFTER,
level = STATEMENT
QUERY PLAN
--
-
Update on trigger_test (cost=0.00..25.88 rows=6 width=42) (actual
time=0.296..0.296 rows=0 loops=1)
Update on trigger_test1
-> Seq Scan on trigger_test1 (cost=0.00..25.88 rows=6 width=42)
(actual time=0.010..0.011 rows=1 loops=1)
Filter: (a = 1)
Planning time: 0.152 ms
Trigger after_upd_stmt_trig on trigger_test: time=0.141 calls=1
Execution time: 0.476 ms
(7 rows)
where trigger_func() is borrowed from the regression test, too. The
stats for the BEFORE STATEMENT UPDATE trigger before_upd_stmt_trig
aren't shown at all in the output. I think this should also be fixed.
So here is a patch for fixing both issues. Changes I made are:
* To fix the former, I added a new List member es_leaf_result_relations
to EState, modified ExecSetupPartitionTupleRouting so that it creates
ResultRelInfos with the EState's es_instrument and then saves them in
that list, and modified ExplainPrintTriggers to show stats for BEFORE
ROW INSERT triggers on leaf partitions (if any) by looking at that list.
I also modified copy.c so that ExecSetupPartitionTupleRouting and
related things are performed in CopyFrom after its EState creation.
* To fix the latter, I modified ExplainPrintTriggers to show stats for
BEFORE STATEMENT UPDATE/DELETE triggers on partitioned table roots (if
any) by looking at the es_root_result_relations array.
* While fixing these, I noticed that AFTER ROW INSERT triggers on leaf
partitions and BEFORE STATEMENT UPDATE/DELETE triggers on partitioned
table roots re-open relations and re-create ResultRelInfos (trigger-only
ResultRelInfos!) in ExecGetTriggerResultRel when executing triggers (and
that in the above examples, the stats for AFTER ROW INSERT trigger/AFTER
STATEMENT UPDATE trigger are shown the result for
es_trig_target_relations in ExplainPrintTriggers). But that wouldn't be
efficient (and EXPLAIN ANALYZE might produce odd outputs), so I modified
ExecGetTriggerResultRel so that it searches es_leaf_result_relations and
es_root_result_relations in addition to es_result_relations.
Best regards,
Etsuro Fujita
*** a/src/backend/commands/copy.c
--- b/src/backend/commands/copy.c
***
*** 1415,1473 BeginCopy(ParseState *pstate,
(errcode(ERRCODE_UNDEFINED_COLUMN),
errmsg("table \"%s\"
Re: [HACKERS] Comment typo in plannodes.h
On 2017/08/11 2:18, Robert Haas wrote: On Thu, Aug 10, 2017 at 8:25 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: Here is a small patch for fixing a comment typo in plannodes.h: s/all the partitioned table/all the partitioned tables/. Committed. 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
[HACKERS] Comment typo in plannodes.h
Hi, Here is a small patch for fixing a comment typo in plannodes.h: s/all the partitioned table/all the partitioned tables/. Best regards, Etsuro Fujita diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index f1a1b24..7c51e7f 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -67,7 +67,7 @@ typedef struct PlannedStmt /* * rtable indexes of non-leaf target relations for UPDATE/DELETE on all -* the partitioned table mentioned in the query. +* the partitioned tables mentioned in the query. */ List *nonleafResultRelations; -- 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] Tuple-routing for certain partitioned tables not working as expected
On 2017/08/07 15:33, Amit Langote wrote: On 2017/08/07 15:22, Etsuro Fujita wrote: On 2017/08/07 13:11, Amit Langote wrote:> The patch looks good too. Although, looking at the following hunk: + Assert(partrel->rd_rel->relkind == RELKIND_RELATION || +partrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE); + /* * Verify result relation is a valid target for the current operation. */ ! if (partrel->rd_rel->relkind == RELKIND_RELATION) ! CheckValidResultRel(partrel, CMD_INSERT); makes me now wonder if we need the CheckValidResultRel check at all. The only check currently in place for RELKIND_RELATION is CheckCmdReplicaIdentity(), which is a noop (returns true) for inserts anyway. Good point! I left the verification for a plain table because that is harmless but as you mentioned, that is nothing but an overhead. So, here is a new version which removes the verification at all from ExecSetupPartitionTupleRouting. The updated patch looks good to me, thanks. 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] Tuple-routing for certain partitioned tables not working as expected
On 2017/08/07 13:11, Amit Langote wrote:> The patch looks good too. Although, looking at the following hunk: + Assert(partrel->rd_rel->relkind == RELKIND_RELATION || + partrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE); + /* * Verify result relation is a valid target for the current operation. */ ! if (partrel->rd_rel->relkind == RELKIND_RELATION) ! CheckValidResultRel(partrel, CMD_INSERT); makes me now wonder if we need the CheckValidResultRel check at all. The only check currently in place for RELKIND_RELATION is CheckCmdReplicaIdentity(), which is a noop (returns true) for inserts anyway. Good point! I left the verification for a plain table because that is harmless but as you mentioned, that is nothing but an overhead. So, here is a new version which removes the verification at all from ExecSetupPartitionTupleRouting. Best regards, Etsuro Fujita *** /dev/null --- b/contrib/file_fdw/data/list1.csv *** *** 0 --- 1,2 + 1,foo + 1,bar *** /dev/null --- b/contrib/file_fdw/data/list2.bad *** *** 0 --- 1,2 + 2,baz + 1,qux *** /dev/null --- b/contrib/file_fdw/data/list2.csv *** *** 0 --- 1,2 + 2,baz + 2,qux *** a/contrib/file_fdw/input/file_fdw.source --- b/contrib/file_fdw/input/file_fdw.source *** *** 162,167 SELECT tableoid::regclass, * FROM agg FOR UPDATE; --- 162,188 ALTER FOREIGN TABLE agg_csv NO INHERIT agg; DROP TABLE agg; + -- declarative partitioning tests + SET ROLE regress_file_fdw_superuser; + CREATE TABLE pt (a int, b text) partition by list (a); + CREATE FOREIGN TABLE p1 partition of pt for values in (1) SERVER file_server + OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ','); + CREATE TABLE p2 partition of pt for values in (2); + SELECT tableoid::regclass, * FROM pt; + SELECT tableoid::regclass, * FROM p1; + SELECT tableoid::regclass, * FROM p2; + COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR + COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ','); + SELECT tableoid::regclass, * FROM pt; + SELECT tableoid::regclass, * FROM p1; + SELECT tableoid::regclass, * FROM p2; + INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR + INSERT INTO pt VALUES (2, 'xyzzy'); + SELECT tableoid::regclass, * FROM pt; + SELECT tableoid::regclass, * FROM p1; + SELECT tableoid::regclass, * FROM p2; + DROP TABLE pt; + -- privilege tests SET ROLE regress_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 *** *** 289,294 SELECT tableoid::regclass, * FROM agg FOR UPDATE; --- 289,375 ALTER FOREIGN TABLE agg_csv NO INHERIT agg; DROP TABLE agg; + -- declarative partitioning tests + SET ROLE regress_file_fdw_superuser; + CREATE TABLE pt (a int, b text) partition by list (a); + CREATE FOREIGN TABLE p1 partition of pt for values in (1) SERVER file_server + OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ','); + CREATE TABLE p2 partition of pt for values in (2); + SELECT tableoid::regclass, * FROM pt; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + (2 rows) + + SELECT tableoid::regclass, * FROM p1; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + (2 rows) + + SELECT tableoid::regclass, * FROM p2; + tableoid | a | b + --+---+--- + (0 rows) + + COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR + ERROR: cannot route inserted tuples to a foreign table + CONTEXT: COPY pt, line 2: "1,qux" + COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ','); + SELECT tableoid::regclass, * FROM pt; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + p2 | 2 | baz + p2 | 2 | qux + (4 rows) + + SELECT tableoid::regclass, * FROM p1; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + (2 rows) + + SELECT tableoid::regclass, * FROM p2; + tableoid | a | b + --+---+- + p2 | 2 | baz + p2 | 2 | qux + (2 rows) + + INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR + ERROR: cannot route inserted tuples to a foreign table + INSERT INTO pt VALUES (2, 'xyzzy'); + SELECT tableoid::regclass, * FROM pt; + tableoid | a | b + --+---+--- + p1 | 1 | foo + p1 | 1 | bar + p2 | 2 | baz + p2 | 2 | qux + p2 | 2 | xyzzy + (5 rows) + + SELECT tableoid::regclass, * FROM p1; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + (2 rows) + + SELECT tableoid::regclass, * FROM p2; + tableoid | a | b
[HACKERS] Tuple-routing for certain partitioned tables not working as expected
Hi, I noticed that tuple-routing for partitioned tables that contain non-writable foreign partitions doesn't work as expected. Here is an example: postgres=# create extension file_fdw; postgres=# create server file_server foreign data wrapper file_fdw; postgres=# create user mapping for CURRENT_USER server file_server; postgres=# create table p (a int) partition by list (a); postgres=# create foreign table t1 partition of p for values in (1) server file_server options (format 'csv', filename '/path/to/file', delimiter ','); postgres=# create table t2 partition of p for values in (2); postgres=# insert into p values (1); ERROR: cannot insert into foreign table "t1" Looks good, but: postgres=# insert into p values (2); ERROR: cannot insert into foreign table "t1" The insert should work but doesn't. (It also seems odd to me that the error message points to t1, not t2.) The reason for that is CheckValidResultRel in ExecSetupPartitionTupleRouting aborts any insert into a partitioned table in the case where the partitioned table contains at least one foreign partition into which the FDW can't insert. I don't think that that is intentional behavior, so I'd like to propose to fix that by skipping CheckValidResultRel for foreign partitions because we can abort an insert into a foreign partition after ExecFindPartition in ExecInsert, by checking to see if resultRelInfo->ri_FdwRoutine is not NULL. Attached is a proposed patch for that. Since COPY FROM has the same issue, I added regression tests for COPY FROM as well as INSERT to file_fdw. Best regards, Etsuro Fujita *** /dev/null --- b/contrib/file_fdw/data/list1.csv *** *** 0 --- 1,2 + 1,foo + 1,bar *** /dev/null --- b/contrib/file_fdw/data/list2.bad *** *** 0 --- 1,2 + 2,baz + 1,qux *** /dev/null --- b/contrib/file_fdw/data/list2.csv *** *** 0 --- 1,2 + 2,baz + 2,qux *** a/contrib/file_fdw/input/file_fdw.source --- b/contrib/file_fdw/input/file_fdw.source *** *** 162,167 SELECT tableoid::regclass, * FROM agg FOR UPDATE; --- 162,188 ALTER FOREIGN TABLE agg_csv NO INHERIT agg; DROP TABLE agg; + -- declarative partitioning tests + SET ROLE regress_file_fdw_superuser; + CREATE TABLE pt (a int, b text) partition by list (a); + CREATE FOREIGN TABLE p1 partition of pt for values in (1) SERVER file_server + OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ','); + CREATE TABLE p2 partition of pt for values in (2); + SELECT tableoid::regclass, * FROM pt; + SELECT tableoid::regclass, * FROM p1; + SELECT tableoid::regclass, * FROM p2; + COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR + COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ','); + SELECT tableoid::regclass, * FROM pt; + SELECT tableoid::regclass, * FROM p1; + SELECT tableoid::regclass, * FROM p2; + INSERT INTO pt VALUES (1, 'xyzzy'); -- ERROR + INSERT INTO pt VALUES (2, 'xyzzy'); + SELECT tableoid::regclass, * FROM pt; + SELECT tableoid::regclass, * FROM p1; + SELECT tableoid::regclass, * FROM p2; + DROP TABLE pt; + -- privilege tests SET ROLE regress_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 *** *** 289,294 SELECT tableoid::regclass, * FROM agg FOR UPDATE; --- 289,375 ALTER FOREIGN TABLE agg_csv NO INHERIT agg; DROP TABLE agg; + -- declarative partitioning tests + SET ROLE regress_file_fdw_superuser; + CREATE TABLE pt (a int, b text) partition by list (a); + CREATE FOREIGN TABLE p1 partition of pt for values in (1) SERVER file_server + OPTIONS (format 'csv', filename '@abs_srcdir@/data/list1.csv', delimiter ','); + CREATE TABLE p2 partition of pt for values in (2); + SELECT tableoid::regclass, * FROM pt; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + (2 rows) + + SELECT tableoid::regclass, * FROM p1; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + (2 rows) + + SELECT tableoid::regclass, * FROM p2; + tableoid | a | b + --+---+--- + (0 rows) + + COPY pt FROM '@abs_srcdir@/data/list2.bad' with (format 'csv', delimiter ','); -- ERROR + ERROR: cannot route inserted tuples to a foreign table + CONTEXT: COPY pt, line 2: "1,qux" + COPY pt FROM '@abs_srcdir@/data/list2.csv' with (format 'csv', delimiter ','); + SELECT tableoid::regclass, * FROM pt; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + p2 | 2 | baz + p2 | 2 | qux + (4 rows) + + SELECT tableoid::regclass, * FROM p1; + tableoid | a | b + --+---+- + p1 | 1 | foo + p1 | 1 | bar + (2 rows) + + SELECT tableoid::regclass, * FROM p2; + tableoid | a | b + --+---+- + p2 | 2
Re: [HACKERS] Update comments in nodeModifyTable.c
On 2017/08/04 1:52, Robert Haas wrote: On Thu, Aug 3, 2017 at 5:55 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: I updated the patch that way. Attached is a new version of the patch. Committed. Thanks again. 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] map_partition_varattnos() and whole-row vars
On 2017/08/04 10:00, Amit Langote wrote: On 2017/08/04 1:06, Robert Haas wrote: On Thu, Aug 3, 2017 at 4:40 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: On 2017/08/03 17:12, Amit Langote wrote: Attached updated patches. Thanks for the patch! That looks good to me. Committed with some comment changes. Thanks a lot. 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] Update comments in nodeModifyTable.c
On 2017/08/02 4:07, Robert Haas wrote:
On Tue, Aug 1, 2017 at 12:31 AM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
Maybe I'm missing something, but I'm not sure that's a good idea because the
change says like we might have 'wholerow' only for the FDW case, but that
isn't correct because we would have 'wholerow' for a view as well. ISTM that
views should be one of the typical cases, so I'd like to propose to modify
the sentence starting with 'Typically' to something like this: "Typically,
this will be a 'ctid' or 'wholerow' attribute, but in the case of a foreign
data wrapper it might be a set of junk attributes sufficient to identify the
remote row." What do you think about that?
Works for me.
I updated the patch that way. Attached is a new version of the patch.
Best regards,
Etsuro Fujita
diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index 0dde0ed..0199c9d 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1696,7 +1696,7 @@ ExecModifyTable(PlanState *pstate)
* the old relation tuple.
*
* Foreign table updates have a wholerow
attribute when the
-* relation has an AFTER ROW trigger. Note
that the wholerow
+* relation has a row-level trigger. Note that
the wholerow
* attribute does not carry system columns.
Foreign table
* triggers miss seeing those, except that we
know enough here
* to set t_tableOid. Quite separately from
this, the FDW may
@@ -2182,8 +2182,11 @@ ExecInitModifyTable(ModifyTable *node, EState *estate,
int eflags)
/*
* Initialize the junk filter(s) if needed. INSERT queries need a
filter
* if there are any junk attrs in the tlist. UPDATE and DELETE always
-* need a filter, since there's always a junk 'ctid' or 'wholerow'
-* attribute present --- no need to look first.
+* need a filter, since there's always at least one junk attribute
present
+* --- no need to look first. Typically, this will be a 'ctid' or
+* 'wholerow' attribute, but in the case of a foreign data wrapper it
+* might be a set of junk attributes sufficient to identify the remote
+* row.
*
* If there are multiple result relations, each one needs its own junk
* filter. Note multiple rels are only possible for UPDATE/DELETE, so
we
@@ -2251,7 +2254,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate,
int eflags)
else if (relkind ==
RELKIND_FOREIGN_TABLE)
{
/*
-* When there is an AFTER
trigger, there should be a
+* When there is a row-level
trigger, there should be a
* wholerow attribute.
*/
j->jf_junkAttNo =
ExecFindJunkAttribute(j, "wholerow");
--
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] map_partition_varattnos() and whole-row vars
On 2017/08/03 17:12, Amit Langote wrote: Attached updated patches. Thanks for the patch! That looks good to me. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] map_partition_varattnos() and whole-row vars
On 2017/08/02 15:21, Amit Langote wrote:
Thanks Fuita-san and Amit for reviewing.
On 2017/08/02 1:33, Amit Khandekar wrote:
On 1 August 2017 at 15:11, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote:
On 2017/07/31 18:56, Amit Langote wrote:
Yes, that's what's needed here. So we need to teach
map_variable_attnos_mutator() to convert whole-row vars just like it's
done in adjust_appendrel_attrs_mutator().
Seems reasonable. (Though I think it might be better to do this kind of
conversion in the planner, not the executor, because that would increase the
efficiency of cached plans.)
That's a good point, although it sounds like a bigger project that, IMHO,
should be undertaken separately, because that would involve designing for
considerations of expanding inheritance even in the INSERT case.
Agreed. I think that would be definitely a material for PG11.
I think the work of shifting to planner should be taken as a different
task when we shift the preparation of DispatchInfo to the planner.
Yeah, I think it'd be a good idea to do those projects together. That is,
doing what Fujita-san suggested and expanding partitioned tables in
partition bound order in the planner.
OK
---
Few more comments :
@@ -1240,7 +1247,7 @@ map_variable_attnos_mutator(Node *node,
var->varlevelsup == context->sublevels_up)
{
/* Found a matching variable, make the substitution */
- Var*newvar = (Var *) palloc(sizeof(Var));
+ Var*newvar = copyObject(var);
int attno = var->varattno;
*newvar = *var;
Here, "*newvar = *var" should be removed.
Done.
I'm not sure this change is a good idea, because the copy by "*newvar =
*var" would be more efficient than the copyObject(). (We have this
optimization in other places as well. See eg, copyVar() in setrefs.c.)
Here are some other comments:
+ /* If the callers expects us to convert the
same, do so. */
+ if (OidIsValid(context->to_rowtype))
+ {
+ ConvertRowtypeExpr *r;
+
+ /* Var itself is converted to the
requested rowtype. */
+ newvar->vartype = context->to_rowtype;
+
+ /*
+* And a conversion step on top to
convert back to the
+* original type.
+*/
+ r = makeNode(ConvertRowtypeExpr);
+ r->arg = (Expr *) newvar;
+ r->resulttype = var->vartype;
+ r->convertformat = COERCE_IMPLICIT_CAST;
+ r->location = -1;
+
+ return (Node *) r;
+ }
Why not do this conversion if to_rowtype is valid and it's different
from the rowtype of the original whole-row Var like the previous patch?
Also, I think it's better to add an assertion that the rowtype of the
original whole-row Var is a named one. So, what I have in mind is:
if (OidIsValid(context->to_rowtype))
{
Assert(var->vartype != RECORDOID)
if (var->vartype != context->to_rowtype)
{
ConvertRowtypeExpr *r;
/* Var itself is converted to the requested rowtype. */
...
/* And a conversion step on top to convert back to the ... */
...
return (Node *) r;
}
}
Here is the modification to the map_variable_attnos()'s API:
map_variable_attnos(Node *node,
int target_varno, int sublevels_up,
const AttrNumber *attno_map,
int map_length,
- bool *found_whole_row)
+ bool *found_whole_row, Oid
to_rowtype)
This is nitpicking, but I think it would be better to put the new
argument to_rowtype right before the output argument found_whole_row.
+ * RelationGetRelType
+ * Returns the rel's pg_type OID.
+ */
+#define RelationGetRelType(relation) \
+ ((relation)->rd_rel->reltype)
This macro is used in only one place. Do we really need that? (This
macro would be useful for other places such as expand_inherited_rtentry,
but I think it's better to introduce this in a separate patch, maybe for
PG11.)
+-- check that wholerow vars in the RETUNING list work with partitioned
tables
Typo: s/RETUNING/RETURNING/
Sorry for the delay.
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] map_partition_varattnos() and whole-row vars
On 2017/08/03 12:06, Robert Haas wrote: On Wed, Aug 2, 2017 at 2:25 AM, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: On 2017/08/02 15:21, Amit Langote wrote: Thanks Fuita-san and Amit for reviewing. Sorry, I meant Fujita-san. Time is growing short here. Is there more review or coding that needs to be done here? I'll take another look at the patch from now. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Another comment typo in execMain.c
On 2017/08/01 6:09, Peter Eisentraut wrote: On 7/6/17 03:23, Etsuro Fujita wrote: Here is a comment in ExecFindPartition() in execMain.c: /* * First check the root table's partition constraint, if any. No point in * routing the tuple it if it doesn't belong in the root table itself. */ I think that in the second sentence "it" just before "if" is a typo. Attached is a small patch for fixing that. fixed 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] map_partition_varattnos() and whole-row vars
*/ + newvar->vartype = context->to_rowtype; + return (Node *) r; + } I think we could set r->resulttype to the vartype (ie, "r->resulttype = newvar->vartype" instead of "r->resulttype = context->from_rowtype"). 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] Update comments in nodeModifyTable.c
On 2017/08/01 1:03, Robert Haas wrote: On Fri, Jul 28, 2017 at 8:12 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: On 2017/07/26 22:39, Robert Haas wrote: So the first part of the change weakens the assertion that a 'ctid' or 'wholerow' attribute will always be present by saying that an FDW may instead have other attributes sufficient to identify the row. That's right. But then the additional sentence says that there will be a 'wholerow' attribute after all. So this whole change seems to me to be going around in a circle. What I mean by the additional one is: if the result table that is a foreign table has a row-level UPDATE/DELETE trigger, a 'wholerow' will also be present. So, if the result table didn't have the trigger, there wouldn't be 'whole-row', so in that case it could be possible that there would be only attributes other than 'ctid' and 'wholerow'. I think maybe something like this would be clearer, then: /* * Initialize the junk filter(s) if needed. INSERT queries need a filter * if there are any junk attrs in the tlist. UPDATE and DELETE always - * need a filter, since there's always a junk 'ctid' or 'wholerow' - * attribute present --- no need to look first. + * need a filter, since there's always at least one junk attribute present + * --- no need to look first. Typically, this will be a 'ctid' + * attribute, but in the case of a foreign data wrapper it might be a + * 'wholerow' attribute or some other set of junk attributes sufficient to + * identify the remote row. * * If there are multiple result relations, each one needs its own junk * filter. Note multiple rels are only possible for UPDATE/DELETE, so we Maybe I'm missing something, but I'm not sure that's a good idea because the change says like we might have 'wholerow' only for the FDW case, but that isn't correct because we would have 'wholerow' for a view as well. ISTM that views should be one of the typical cases, so I'd like to propose to modify the sentence starting with 'Typically' to something like this: "Typically, this will be a 'ctid' or 'wholerow' attribute, but in the case of a foreign data wrapper it might be a set of junk attributes sufficient to identify the remote row." What do you think about that? 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] [BUGS] BUG #14759: insert into foreign data partitions fail
On 2017/08/01 10:18, Amit Langote wrote: Good points; fixed in the updated patch. I should have mentioned this in an earlier mail, but one thing I noticed is this: -the remote server. +the remote server. That becomes especially important if the table is +being used in a partition hierarchy, where it is recommended to add +a constraint matching the partition constraint expression on +the remote table. I think this would apply to CHECK constraints on foreign tables when implementing partitioning with inheritance. Why do we only mention this for partition constraints? Other than that, the patch looks good to me. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Update comments in nodeModifyTable.c
On 2017/07/26 22:39, Robert Haas wrote: On Wed, Jun 14, 2017 at 10:40 PM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: Attached is an updated version of the patch. Well, now I'm confused: * Initialize the junk filter(s) if needed. INSERT queries need a filter * if there are any junk attrs in the tlist. UPDATE and DELETE always * need a filter, since there's always a junk 'ctid' or 'wholerow' - * attribute present --- no need to look first. + * attribute present if not foreign table, and if foreign table, there + * are always junk attributes present the FDW needs to identify the exact + * row to update or delete --- no need to look first. For foreign tables, + * there's also a wholerow attribute when the relation has a row-level + * trigger on UPDATE/DELETE but not on INSERT. So the first part of the change weakens the assertion that a 'ctid' or 'wholerow' attribute will always be present by saying that an FDW may instead have other attributes sufficient to identify the row. That's right. But then the additional sentence says that there will be a 'wholerow' attribute after all. So this whole change seems to me to be going around in a circle. What I mean by the additional one is: if the result table that is a foreign table has a row-level UPDATE/DELETE trigger, a 'wholerow' will also be present. So, if the result table didn't have the trigger, there wouldn't be 'whole-row', so in that case it could be possible that there would be only attributes other than 'ctid' and 'wholerow'. 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] [BUGS] BUG #14759: insert into foreign data partitions fail
On 2017/07/26 15:29, Amit Langote wrote: On 2017/07/25 9:43, David G. Johnston wrote: On Mon, Jul 24, 2017 at 5:19 PM, Amit Langote <langote_amit...@lab.ntt.co.jp wrote: On 2017/07/25 6:28, mtun...@gmail.com wrote: The following bug has been logged on the website: Bug reference: 14759 Logged by: Murat Tuncer Email address: mtun...@gmail.com PostgreSQL version: 10beta2 Operating system: Mac 10.12.6 Description: I got ERROR: cannot route inserted tuples to a foreign table Inserting tuples into a partitioned table that will route to one of its foreign table partitions is unsupported in PG 10. The limitation is mentioned on the following page: https://www.postgresql.org/docs/devel/static/ddl-partitioning.html It would be nice to also note this limitation here: https://www.postgresql.org/docs/devel/static/sql-createforeigntable.html Yeah, I thought the same when writing my previous email. Also, the ddl-partitioning.html page has a section "5.10.2.3. Limitations". Moving (or duplicating maybe) the existing comment on that page in that section would make finding out about this limitation a bit easier. Yeah, perhaps. I'd probably move (and rework) the "limitation wording" to the limitation sections and do something like the following in the main section. "Foreign Tables can be added to a partitioning structure but inserts to the partitioned table will fail if they are routed to a foreign table partition. Direct writes to the foreign table, and partition reads, work normally." Done that in the attached. I'm curious what the other limitations are... I think COPY has the same limitation as INSERT. When I first wrote that documentation line (I am assuming you're asking about "although these have some limitations that normal tables do not"), I was thinking about the fact that the core system does not enforce (locally) any constraints defined on foreign tables. Since we allow inserting data into partitions directly, it is imperative that we enforce the "partition constraint" along with the traditional constraints such as NOT NULL and CHECK constraints, which we can do for local table partitions but not for foreign table ones. Anyway, attached patch documents all these limitations about foreign table partitions more prominently. Typo: s/the they is/they are/ 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] UPDATE of partition key
On 2017/07/26 6:07, Robert Haas wrote: On Thu, Jul 13, 2017 at 1:09 PM, Amit Khandekar <amitdkhan...@gmail.com> wrote: Attached is a WIP patch (make_resultrels_ordered.patch) that generates the result rels in canonical order. This patch is kept separate from the update-partition-key patch, and can be applied on master branch. Thank you for working on this, Amit! Hmm, I like the approach you've taken here in general, +1 for the approach. Is there any real benefit in this "walker" interface? It looks to me like it might be simpler to just change things around so that it returns a list of OIDs, like find_all_inheritors, but generated differently. Then if you want bound-ordering rather than OID-ordering, you just do this: list_free(inhOids); inhOids = get_partition_oids_in_bound_order(rel); That'd remove the need for some if/then logic as you've currently got in get_next_child(). Yeah, that would make the code much simple, so +1 for Robert's idea. I think we should always expand in bound order rather than only when it's a result relation. I think for partition-wise join, we're going to want to do it this way for all relations in the query, or at least for all relations in the query that might possibly be able to participate in a partition-wise join. If there are multiple cases that are going to need this ordering, it's hard for me to accept the idea that it's worth the complexity of trying to keep track of when we expanded things in one order vs. another. There are other applications of having things in bound order too, like MergeAppend -> Append strength-reduction (which might not be legal anyway if there are list partitions with multiple, non-contiguous list bounds or if any NULL partition doesn't end up in the right place in the order, but there will be lots of cases where it can work). +1 for that as well. Another benefit from that would be EXPLAIN; we could display partitions for a partitioned table in the same order for Append and ModifyTable (ie, SELECT/UPDATE/DELETE), which I think would make the EXPLAIN result much readable. 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] Mishandling of WCO constraints in direct foreign table modification
On 2017/07/25 5:35, Robert Haas wrote: On Fri, Jul 21, 2017 at 6:21 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: I mean constraints derived from WITH CHECK OPTIONs specified for parent views. We use the words "WITH CHECK OPTION constraints" in comments in nodeModifyTable.c, so the expression "CHECK OPTION constrains" doesn't sound not that bad to me. (I used "CHECK OPTION", not "WITH CHECK OPTION", because we use "CHECK OPTION" a lot more in the documentation than "WITH CHECK OPTION".) Yeah, it seems OK to me, too; if the consensus is otherwise, we also have the option to change it later. Agreed. Committed and back-patched as you had it, but I removed a spurious comma. Thanks for that, Robert! Thanks for reviewing, Horiguchi-san! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables
On 2017/07/24 16:16, Amit Khandekar wrote: On 24 July 2017 at 12:11, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: Attached is the updated version of your patch. Good catch, Amit K. and Amit L.! Now that this is done, any particular reason it is not done in ExecPartitionCheck() ? I see that there is a do_convert_tuple() in that function, again without changing the slot descriptor. Yeah, I think we would need that in ExecPartitionCheck() as well. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables
On 2017/07/21 19:16, Etsuro Fujita wrote: On 2017/07/20 11:21, Etsuro Fujita wrote: On 2017/07/19 23:36, Tom Lane wrote: Please put the responsibility of doing const-expression simplification in these cases somewhere closer to where the problem is being created. It would be reasonable that it's the FDW's responsibility to do that const-simplification if necessary? There seems to be no objections, so I removed the const-expression simplification from the patch and I added the note to the docs for AddForeignUpdateTargets. Attached is an updated version of the patch. I cleaned up the patch a bit. PFA a new version of the patch. Best regards, Etsuro Fujita *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 6962,6967 update bar set f2 = f2 + 100 returning *; --- 6962,7026 7 | 277 (6 rows) + -- Test that UPDATE/DELETE with inherited target works with row-level triggers + CREATE TRIGGER trig_row_before + BEFORE UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + CREATE TRIGGER trig_row_after + AFTER UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + explain (verbose, costs off) + update bar set f2 = f2 + 100; + QUERY PLAN + -- + Update on public.bar +Update on public.bar +Foreign Update on public.bar2 + Remote SQL: UPDATE public.loct2 SET f2 = $2 WHERE ctid = $1 RETURNING f1, f2, f3 +-> Seq Scan on public.bar + Output: bar.f1, (bar.f2 + 100), bar.ctid +-> Foreign Scan on public.bar2 + Output: bar2.f1, (bar2.f2 + 100), bar2.f3, bar2.ctid, bar2.* + Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE + (9 rows) + + update bar set f2 = f2 + 100; + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (3,333,33),NEW: (3,433,33) + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (4,344,44),NEW: (4,444,44) + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (7,277,77),NEW: (7,377,77) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (3,333,33),NEW: (3,433,33) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (4,344,44),NEW: (4,444,44) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (7,277,77),NEW: (7,377,77) + explain (verbose, costs off) + delete from bar where f2 < 400; + QUERY PLAN + - + Delete on public.bar +Delete on public.bar +Foreign Delete on public.bar2 + Remote SQL: DELETE FROM public.loct2 WHERE ctid = $1 RETURNING f1, f2, f3 +-> Seq Scan on public.bar + Output: bar.ctid + Filter: (bar.f2 < 400) +-> Foreign Scan on public.bar2 + Output: bar2.ctid, bar2.* + Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 WHERE ((f2 < 400)) FOR UPDATE + (10 rows) + + delete from bar where f2 < 400; + NOTICE: trig_row_before(23, skidoo) BEFORE ROW DELETE ON bar2 + NOTICE: OLD: (7,377,77) + NOTICE: trig_row_after(23, skidoo) AFTER ROW DELETE ON bar2 + NOTICE: OLD: (7,377,77) + -- cleanup + DROP TRIGGER trig_row_before ON bar2; + DROP TRIGGER trig_row_after ON bar2; drop table foo cascade; NOTICE: drop cascades to foreign table foo2 drop table bar cascade; *** a/contrib/postgres_fdw/sql/postgres_fdw.sql --- b/contrib/postgres_fdw/sql/postgres_fdw.sql *** *** 1632,1637 explain (verbose, costs off) --- 1632,1657 update bar set f2 = f2 + 100 returning *; update bar set f2 = f2 + 100 returning *; + -- Test that UPDATE/DELETE with inherited target works with row-level triggers + CREATE TRIGGER trig_row_before + BEFORE UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + + CREATE TRIGGER trig_row_after + AFTER UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + + explain (verbose, costs off) + update bar set f2 = f2 + 100; + update bar set f2 = f2 + 100; + + explain (verbose, costs off) + delete from bar where f2 < 400; + delete from bar where f2 < 400; + + -- cleanup + DROP TRIGGER trig_row_before ON bar2; + DROP TRIGGER trig_row_after ON bar2; drop table foo cascade; drop table bar cascade; drop table loct1; *** a/doc/src/sgml/fdwhandler.sgml --- b/doc/src/sgml/fdwhandler.sgml *** *** 428,438 AddForeignUpdateTargets (Query *parsetree, Avoid using names matching ctidN, wholerow, or wholerowN, as the core system can ! genera
Re: [HACKERS] Mishandling of WCO constraints in direct foreign table modification
On 2017/07/21 17:18, Kyotaro HORIGUCHI wrote: At Fri, 21 Jul 2017 12:00:03 +0900, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote in <15aa9936-9bd8-c9e3-7ca1-394861073...@lab.ntt.co.jp> Attached is the second version which updated docs in postgres-fdw.sgml as well. !no local joins for the query, no row-level local BEFORE or !AFTER triggers on the target table, and no !CHECK OPTION constraints from parent views. !In UPDATE, Might be a silly question, is CHECK OPTION a "constraint"? I mean constraints derived from WITH CHECK OPTIONs specified for parent views. We use the words "WITH CHECK OPTION constraints" in comments in nodeModifyTable.c, so the expression "CHECK OPTION constrains" doesn't sound not that bad to me. (I used "CHECK OPTION", not "WITH CHECK OPTION", because we use "CHECK OPTION" a lot more in the documentation than "WITH CHECK OPTION".) 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] Bug in ExecModifyTable function and trigger issues for foreign tables
On 2017/07/20 11:21, Etsuro Fujita wrote: On 2017/07/19 23:36, Tom Lane wrote: Please put the responsibility of doing const-expression simplification in these cases somewhere closer to where the problem is being created. It would be reasonable that it's the FDW's responsibility to do that const-simplification if necessary? There seems to be no objections, so I removed the const-expression simplification from the patch and I added the note to the docs for AddForeignUpdateTargets. Attached is an updated version of the patch. Best regards, Etsuro Fujita *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 6924,6929 update bar set f2 = f2 + 100 returning *; --- 6924,6988 7 | 277 (6 rows) + -- Test that UPDATE/DELETE with inherited target works with row-level triggers + CREATE TRIGGER trig_row_before + BEFORE UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + CREATE TRIGGER trig_row_after + AFTER UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + explain (verbose, costs off) + update bar set f2 = f2 + 100; + QUERY PLAN + -- + Update on public.bar +Update on public.bar +Foreign Update on public.bar2 + Remote SQL: UPDATE public.loct2 SET f2 = $2 WHERE ctid = $1 RETURNING f1, f2, f3 +-> Seq Scan on public.bar + Output: bar.f1, (bar.f2 + 100), bar.ctid +-> Foreign Scan on public.bar2 + Output: bar2.f1, (bar2.f2 + 100), bar2.f3, bar2.ctid, bar2.* + Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE + (9 rows) + + update bar set f2 = f2 + 100; + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (3,333,33),NEW: (3,433,33) + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (4,344,44),NEW: (4,444,44) + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (7,277,77),NEW: (7,377,77) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (3,333,33),NEW: (3,433,33) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (4,344,44),NEW: (4,444,44) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (7,277,77),NEW: (7,377,77) + explain (verbose, costs off) + delete from bar where f2 < 400; + QUERY PLAN + - + Delete on public.bar +Delete on public.bar +Foreign Delete on public.bar2 + Remote SQL: DELETE FROM public.loct2 WHERE ctid = $1 RETURNING f1, f2, f3 +-> Seq Scan on public.bar + Output: bar.ctid + Filter: (bar.f2 < 400) +-> Foreign Scan on public.bar2 + Output: bar2.ctid, bar2.* + Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 WHERE ((f2 < 400)) FOR UPDATE + (10 rows) + + delete from bar where f2 < 400; + NOTICE: trig_row_before(23, skidoo) BEFORE ROW DELETE ON bar2 + NOTICE: OLD: (7,377,77) + NOTICE: trig_row_after(23, skidoo) AFTER ROW DELETE ON bar2 + NOTICE: OLD: (7,377,77) + -- cleanup + DROP TRIGGER trig_row_before ON bar2; + DROP TRIGGER trig_row_after ON bar2; drop table foo cascade; NOTICE: drop cascades to foreign table foo2 drop table bar cascade; *** a/contrib/postgres_fdw/sql/postgres_fdw.sql --- b/contrib/postgres_fdw/sql/postgres_fdw.sql *** *** 1609,1614 explain (verbose, costs off) --- 1609,1634 update bar set f2 = f2 + 100 returning *; update bar set f2 = f2 + 100 returning *; + -- Test that UPDATE/DELETE with inherited target works with row-level triggers + CREATE TRIGGER trig_row_before + BEFORE UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + + CREATE TRIGGER trig_row_after + AFTER UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + + explain (verbose, costs off) + update bar set f2 = f2 + 100; + update bar set f2 = f2 + 100; + + explain (verbose, costs off) + delete from bar where f2 < 400; + delete from bar where f2 < 400; + + -- cleanup + DROP TRIGGER trig_row_before ON bar2; + DROP TRIGGER trig_row_after ON bar2; drop table foo cascade; drop table bar cascade; drop table loct1; *** a/doc/src/sgml/fdwhandler.sgml --- b/doc/src/sgml/fdwhandler.sgml *** *** 428,438 AddForeignUpdateTargets (Query *parsetree, Avoid using names matching ctidN, wholerow, or wholerowN, as the core system can ! generate junk columns of these names. ! This function is called in the rewriter, not the planner, so th
Re: [HACKERS] Mishandling of WCO constraints in direct foreign table modification
On 2017/07/21 3:24, Robert Haas wrote: I think that's reasonable. This should be committed and back-patched to 9.6, right? Yeah, because direct modify was introduced in 9.6. Attached is the second version which updated docs in postgres-fdw.sgml as well. Best regards, Etsuro Fujita *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 5856,5861 INSERT INTO ft1(c1, c2) VALUES(, 2); --- 5856,5921 UPDATE ft1 SET c2 = c2 + 1 WHERE c1 = 1; ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c2negative; -- === + -- test WITH CHECK OPTION constraints + -- === + CREATE TABLE base_tbl (a int, b int); + CREATE FOREIGN TABLE foreign_tbl (a int, b int) + SERVER loopback OPTIONS(table_name 'base_tbl'); + CREATE VIEW rw_view AS SELECT * FROM foreign_tbl + WHERE a < b WITH CHECK OPTION; + \d+ rw_view +View "public.rw_view" + Column | Type | Collation | Nullable | Default | Storage | Description + +-+---+--+-+-+- + a | integer | | | | plain | + b | integer | | | | plain | + View definition: + SELECT foreign_tbl.a, + foreign_tbl.b +FROM foreign_tbl + WHERE foreign_tbl.a < foreign_tbl.b; + Options: check_option=cascaded + + INSERT INTO rw_view VALUES (0, 10); -- ok + INSERT INTO rw_view VALUES (10, 0); -- should fail + ERROR: new row violates check option for view "rw_view" + DETAIL: Failing row contains (10, 0). + EXPLAIN (VERBOSE, COSTS OFF) + UPDATE rw_view SET b = 20 WHERE a = 0; -- not pushed down + QUERY PLAN + -- + Update on public.foreign_tbl +Remote SQL: UPDATE public.base_tbl SET b = $2 WHERE ctid = $1 +-> Foreign Scan on public.foreign_tbl + Output: foreign_tbl.a, 20, foreign_tbl.ctid + Remote SQL: SELECT a, ctid FROM public.base_tbl WHERE ((a < b)) AND ((a = 0)) FOR UPDATE + (5 rows) + + UPDATE rw_view SET b = 20 WHERE a = 0; -- ok + EXPLAIN (VERBOSE, COSTS OFF) + UPDATE rw_view SET b = -20 WHERE a = 0; -- not pushed down + QUERY PLAN + -- + Update on public.foreign_tbl +Remote SQL: UPDATE public.base_tbl SET b = $2 WHERE ctid = $1 +-> Foreign Scan on public.foreign_tbl + Output: foreign_tbl.a, '-20'::integer, foreign_tbl.ctid + Remote SQL: SELECT a, ctid FROM public.base_tbl WHERE ((a < b)) AND ((a = 0)) FOR UPDATE + (5 rows) + + UPDATE rw_view SET b = -20 WHERE a = 0; -- should fail + ERROR: new row violates check option for view "rw_view" + DETAIL: Failing row contains (0, -20). + SELECT * FROM foreign_tbl; + a | b + ---+ + 0 | 20 + (1 row) + + DROP FOREIGN TABLE foreign_tbl CASCADE; + NOTICE: drop cascades to view rw_view + DROP TABLE base_tbl; + -- === -- 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 *** *** 1158,1163 UPDATE ft1 SET c2 = c2 + 1 WHERE c1 = 1; --- 1158,1187 ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c2negative; -- === + -- test WITH CHECK OPTION constraints + -- === + + CREATE TABLE base_tbl (a int, b int); + CREATE FOREIGN TABLE foreign_tbl (a int, b int) + SERVER loopback OPTIONS(table_name 'base_tbl'); + CREATE VIEW rw_view AS SELECT * FROM foreign_tbl + WHERE a < b WITH CHECK OPTION; + \d+ rw_view + + INSERT INTO rw_view VALUES (0, 10); -- ok + INSERT INTO rw_view VALUES (10, 0); -- should fail + EXPLAIN (VERBOSE, COSTS OFF) + UPDATE rw_view SET b = 20 WHERE a = 0; -- not pushed down + UPDATE rw_view SET b = 20 WHERE a = 0; -- ok + EXPLAIN (VERBOSE, COSTS OFF) + UPDATE rw_view SET b = -20 WHERE a = 0; -- not pushed down + UPDATE rw_view SET b = -20 WHERE a = 0; -- should fail + SELECT * FROM foreign_tbl; + + DROP FOREIGN TABLE foreign_tbl CASCADE; + DROP TABLE base_tbl; + + -- === -- test serial columns (ie, sequence-based defaults) --
[HACKERS] Mishandling of WCO constraints in direct foreign table modification
Here is an example for $subject using postgres_fdw: postgres=# create foreign table foreign_tbl (a int, b int) server loopback options (table_name 'base_tbl'); CREATE FOREIGN TABLE postgres=# create view rw_view as select * from foreign_tbl where a < b with check option; CREATE VIEW postgres=# insert into rw_view values (0, 10); INSERT 0 1 postgres=# explain verbose update rw_view set a = 20 where b = 10; QUERY PLAN -- Update on public.foreign_tbl (cost=100.00..146.21 rows=4 width=14) -> Foreign Update on public.foreign_tbl (cost=100.00..146.21 rows=4 width=14) Remote SQL: UPDATE public.base_tbl SET a = 20 WHERE ((a < b)) AND ((b = 10)) (3 rows) postgres=# update rw_view set a = 20 where b = 10; UPDATE 1 This is wrong! This should fail. The reason for that is; direct modify is overlooking checking WITH CHECK OPTION constraints from parent views. I think we could do direct modify, even if there are any WITH CHECK OPTIONs, in some way or other, but I think that is a feature. So, I'd like to propose to fix this by just giving up direct modify if there are any WITH CHECK OPTIONs. Attached is a patch for that. I'll add it to the next commitfest. Best regards, Etsuro Fujita *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 5856,5861 INSERT INTO ft1(c1, c2) VALUES(, 2); --- 5856,5921 UPDATE ft1 SET c2 = c2 + 1 WHERE c1 = 1; ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c2negative; -- === + -- test WITH CHECK OPTION constraints + -- === + CREATE TABLE base_tbl (a int, b int); + CREATE FOREIGN TABLE foreign_tbl (a int, b int) + SERVER loopback OPTIONS(table_name 'base_tbl'); + CREATE VIEW rw_view AS SELECT * FROM foreign_tbl + WHERE a < b WITH CHECK OPTION; + \d+ rw_view +View "public.rw_view" + Column | Type | Collation | Nullable | Default | Storage | Description + +-+---+--+-+-+- + a | integer | | | | plain | + b | integer | | | | plain | + View definition: + SELECT foreign_tbl.a, + foreign_tbl.b +FROM foreign_tbl + WHERE foreign_tbl.a < foreign_tbl.b; + Options: check_option=cascaded + + INSERT INTO rw_view VALUES (0, 10); -- ok + INSERT INTO rw_view VALUES (10, 0); -- should fail + ERROR: new row violates check option for view "rw_view" + DETAIL: Failing row contains (10, 0). + EXPLAIN (VERBOSE, COSTS OFF) + UPDATE rw_view SET b = 20 WHERE a = 0; -- not pushed down + QUERY PLAN + -- + Update on public.foreign_tbl +Remote SQL: UPDATE public.base_tbl SET b = $2 WHERE ctid = $1 +-> Foreign Scan on public.foreign_tbl + Output: foreign_tbl.a, 20, foreign_tbl.ctid + Remote SQL: SELECT a, ctid FROM public.base_tbl WHERE ((a < b)) AND ((a = 0)) FOR UPDATE + (5 rows) + + UPDATE rw_view SET b = 20 WHERE a = 0; -- ok + EXPLAIN (VERBOSE, COSTS OFF) + UPDATE rw_view SET b = -20 WHERE a = 0; -- not pushed down + QUERY PLAN + -- + Update on public.foreign_tbl +Remote SQL: UPDATE public.base_tbl SET b = $2 WHERE ctid = $1 +-> Foreign Scan on public.foreign_tbl + Output: foreign_tbl.a, '-20'::integer, foreign_tbl.ctid + Remote SQL: SELECT a, ctid FROM public.base_tbl WHERE ((a < b)) AND ((a = 0)) FOR UPDATE + (5 rows) + + UPDATE rw_view SET b = -20 WHERE a = 0; -- should fail + ERROR: new row violates check option for view "rw_view" + DETAIL: Failing row contains (0, -20). + SELECT * FROM foreign_tbl; + a | b + ---+ + 0 | 20 + (1 row) + + DROP FOREIGN TABLE foreign_tbl CASCADE; + NOTICE: drop cascades to view rw_view + DROP TABLE base_tbl; + -- === -- 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 *** *** 1158,1163 UPDATE ft1 SET c2 = c2 + 1 WHERE c1 = 1; --- 1158,1187 ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c2negative; -- ==
Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables
On 2017/07/19 23:36, Tom Lane wrote: Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> writes: * Modified rewrite_targetlist(), which is a new function added to preptlist.c, so that we do const-simplification to junk TLEs that AddForeignUpdateTargets() added, as that API allows the FDW to add junk TLEs containing non-Var expressions to the query's targetlist. This does not seem like a good idea to me. eval_const_expressions is not a cheap thing, and for most use-cases those cycles will be wasted, and it has never been the responsibility of preprocess_targetlist to do this sort of thing. Hm, I added that const-simplification to that function so that the existing FDWs that append junk TLEs that need const-simplification, which I don't know really exist, would work well for this fix, without any changes, but I agree on that point. Please put the responsibility of doing const-expression simplification in these cases somewhere closer to where the problem is being created. It would be reasonable that it's the FDW's responsibility to do that const-simplification if necessary? 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] Bug in ExecModifyTable function and trigger issues for foreign tables
On 2017/07/13 21:10, Etsuro Fujita wrote: Attached is an updated version of the patch. Here is an updated version of the patch. Changes are: * Modified rewrite_targetlist(), which is a new function added to preptlist.c, so that we do const-simplification to junk TLEs that AddForeignUpdateTargets() added, as that API allows the FDW to add junk TLEs containing non-Var expressions to the query's targetlist. * Updated docs in fdwhandler.sgml. Best regards, Etsuro Fujita *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 6924,6929 update bar set f2 = f2 + 100 returning *; --- 6924,6988 7 | 277 (6 rows) + -- Test that UPDATE/DELETE with inherited target works with row-level triggers + CREATE TRIGGER trig_row_before + BEFORE UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + CREATE TRIGGER trig_row_after + AFTER UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + explain (verbose, costs off) + update bar set f2 = f2 + 100; + QUERY PLAN + -- + Update on public.bar +Update on public.bar +Foreign Update on public.bar2 + Remote SQL: UPDATE public.loct2 SET f2 = $2 WHERE ctid = $1 RETURNING f1, f2, f3 +-> Seq Scan on public.bar + Output: bar.f1, (bar.f2 + 100), bar.ctid +-> Foreign Scan on public.bar2 + Output: bar2.f1, (bar2.f2 + 100), bar2.f3, bar2.ctid, bar2.* + Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE + (9 rows) + + update bar set f2 = f2 + 100; + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (3,333,33),NEW: (3,433,33) + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (4,344,44),NEW: (4,444,44) + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (7,277,77),NEW: (7,377,77) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (3,333,33),NEW: (3,433,33) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (4,344,44),NEW: (4,444,44) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (7,277,77),NEW: (7,377,77) + explain (verbose, costs off) + delete from bar where f2 < 400; + QUERY PLAN + - + Delete on public.bar +Delete on public.bar +Foreign Delete on public.bar2 + Remote SQL: DELETE FROM public.loct2 WHERE ctid = $1 RETURNING f1, f2, f3 +-> Seq Scan on public.bar + Output: bar.ctid + Filter: (bar.f2 < 400) +-> Foreign Scan on public.bar2 + Output: bar2.ctid, bar2.* + Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 WHERE ((f2 < 400)) FOR UPDATE + (10 rows) + + delete from bar where f2 < 400; + NOTICE: trig_row_before(23, skidoo) BEFORE ROW DELETE ON bar2 + NOTICE: OLD: (7,377,77) + NOTICE: trig_row_after(23, skidoo) AFTER ROW DELETE ON bar2 + NOTICE: OLD: (7,377,77) + -- cleanup + DROP TRIGGER trig_row_before ON bar2; + DROP TRIGGER trig_row_after ON bar2; drop table foo cascade; NOTICE: drop cascades to foreign table foo2 drop table bar cascade; *** a/contrib/postgres_fdw/sql/postgres_fdw.sql --- b/contrib/postgres_fdw/sql/postgres_fdw.sql *** *** 1609,1614 explain (verbose, costs off) --- 1609,1634 update bar set f2 = f2 + 100 returning *; update bar set f2 = f2 + 100 returning *; + -- Test that UPDATE/DELETE with inherited target works with row-level triggers + CREATE TRIGGER trig_row_before + BEFORE UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + + CREATE TRIGGER trig_row_after + AFTER UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + + explain (verbose, costs off) + update bar set f2 = f2 + 100; + update bar set f2 = f2 + 100; + + explain (verbose, costs off) + delete from bar where f2 < 400; + delete from bar where f2 < 400; + + -- cleanup + DROP TRIGGER trig_row_before ON bar2; + DROP TRIGGER trig_row_after ON bar2; drop table foo cascade; drop table bar cascade; drop table loct1; *** a/doc/src/sgml/fdwhandler.sgml --- b/doc/src/sgml/fdwhandler.sgml *** *** 432,438 AddForeignUpdateTargets (Query *parsetree, ! This function is called in the rewriter, not the planner, so the information available is a bit different from that available to the planning routines. parsetree is the parse tree for the UPDATE or --- 432,438 ! Although this fun
[HACKERS] Re: Oddity in error handling of constraint violation in ExecConstraints for partitioned tables
On 2017/07/18 11:03, Robert Haas wrote: On Mon, Jul 10, 2017 at 5:44 PM, Robert Haas <robertmh...@gmail.com> wrote: The posted patches look OK to me. Barring developments, I will commit them on 2017-07-17, or send another update by then. Committed them. 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] Bug in ExecModifyTable function and trigger issues for foreign tables
On 2017/06/30 18:44, Etsuro Fujita wrote:
On 2017/06/16 21:29, Etsuro Fujita wrote:
I'll have second thought about this, so I'll mark this as waiting on
author.
I spent quite a bit of time on this and came up with a solution for
addressing the concern mentioned by Ashutosh [1]. The basic idea is, as
I said before, to move rewriteTargetListUD from the rewriter to the
planner (whether the update or delete is inherited or not), except for
the view case. In case of inherited UPDATE/DELETE, the planner adds a
necessary junk column needed for the update or delete for each child
relation, without the assumption I made before about junk tlist entries,
so I think this would be more robust for future changes as mentioned in
[1]. It wouldn't work well that the planner does the same thing for the
view case (ie, add a whole-row reference to the given target relation),
unlike other cases, because what we need to do for that case is to add a
whole-row reference to the view as the source for an INSTEAD OF trigger,
not the target. So, ISTM that it's reasonable to handle that case in
the rewriter as-is, not in the planner, but one thing I'd like to
propose to simplify the code in rewriteHandler.c is to move the code for
the view case in rewriteTargetListUD to ApplyRetrieveRule. By that
change, we won't add a whole-row reference to the view in RewriteQuery,
so we don't need this annoying thing in rewriteTargetView any more:
/*
* For UPDATE/DELETE, rewriteTargetListUD will have added a
wholerow junk
* TLE for the view to the end of the targetlist, which we no
longer need.
* Remove it to avoid unnecessary work when we process the targetlist.
* Note that when we recurse through rewriteQuery a new junk TLE
will be
* added to allow the executor to find the proper row in the new target
* relation. (So, if we failed to do this, we might have multiple junk
* TLEs with the same name, which would be disastrous.)
*/
if (parsetree->commandType != CMD_INSERT)
{
TargetEntry *tle = (TargetEntry *) llast(parsetree->targetList);
Assert(tle->resjunk);
Assert(IsA(tle->expr, Var) &&
((Var *) tle->expr)->varno == parsetree->resultRelation &&
((Var *) tle->expr)->varattno == 0);
parsetree->targetList = list_delete_ptr(parsetree->targetList,
tle);
}
Attached is an updated version of the patch. Comments are welcome!
Best regards,
Etsuro Fujita
[1]
https://www.postgresql.org/message-id/CAFjFpRfTpamoUz6fNyk6gPh%3DecfBJjbUHJNKbDxscpyPJ3FfjQ%40mail.gmail.com
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 6924,6929 update bar set f2 = f2 + 100 returning *;
--- 6924,6988
7 | 277
(6 rows)
+ -- Test that UPDATE/DELETE with inherited target works with row-level triggers
+ CREATE TRIGGER trig_row_before
+ BEFORE UPDATE OR DELETE ON bar2
+ FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
+ CREATE TRIGGER trig_row_after
+ AFTER UPDATE OR DELETE ON bar2
+ FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
+ explain (verbose, costs off)
+ update bar set f2 = f2 + 100;
+ QUERY PLAN
+
--
+ Update on public.bar
+Update on public.bar
+Foreign Update on public.bar2
+ Remote SQL: UPDATE public.loct2 SET f2 = $2 WHERE ctid = $1 RETURNING
f1, f2, f3
+-> Seq Scan on public.bar
+ Output: bar.f1, (bar.f2 + 100), bar.ctid
+-> Foreign Scan on public.bar2
+ Output: bar2.f1, (bar2.f2 + 100), bar2.f3, bar2.ctid, bar2.*
+ Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE
+ (9 rows)
+
+ update bar set f2 = f2 + 100;
+ NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
+ NOTICE: OLD: (3,333,33),NEW: (3,433,33)
+ NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
+ NOTICE: OLD: (4,344,44),NEW: (4,444,44)
+ NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
+ NOTICE: OLD: (7,277,77),NEW: (7,377,77)
+ NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
+ NOTICE: OLD: (3,333,33),NEW: (3,433,33)
+ NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
+ NOTICE: OLD: (4,344,44),NEW: (4,444,44)
+ NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
+ NOTICE: OLD: (7,277,77),NEW: (7,377,77)
+ explain (verbose, costs off)
+ delete from bar where f2 < 400;
+ QUERY PLAN
+
-
+ Delete on public.bar
+Delete on public.bar
+Foreign Delete on public.bar2
+ Remote SQL: DELETE FROM public.lo
Re: [HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables
On 2017/07/07 18:47, Amit Langote wrote:
On 2017/07/06 16:06, Etsuro Fujita wrote:
I think this should be fixed. Attached is a patch for that.
Looking up the ResultRelInfo using the proposed method in
ExecFindResultRelInfo() can be unreliable sometimes, that is, the method
of using the root table OID to pin down the RangeTableEntry to get the the
modified columns set.
You are right. Thank you for pointing that out.
How about setting ri_RangeTableIndex of the partition ResultRelInfo
correctly in the first place? If you look at the way InitResultRelInfo()
is called in ExecSetupPartitionTupleRouting(), a dummy RT index of 1 is
passed. We could instead pass the correct one. I think
ModifyTable.nominalRelation is that (at least in the INSERT case.
The attached patch implements that. It modifies
ExecSetupPartitionTupleRouting to accept one more argument resultRTindex
and passes along the same to InitResultRelInfo(). Later when
ExecConstraints() wants to build the modifiedCols set, it looks up the
correct RTE using the partition ResultRelInfo, which has the appropriate
ri_RangeTableIndex set and uses the same.
Looks good to me.
The patch keeps tests that you added in your patch. Added this to the
open items list.
Another thing I noticed is the error handling in ExecWithCheckOptions;
it doesn't take any care for partition tables, so the column description
in the error message for WCO_VIEW_CHECK is built using the partition
table's rowtype, not the root table's. But I think it'd be better to
build that using the root table's rowtype, like ExecConstraints, because
that would make the column description easier to understand since the
parent view(s) (from which WithCheckOptions evaluated there are created)
would have been defined on the root table. This seems independent from
the above issue, so I created a separate patch, which I'm attaching.
What do you think about that?
Best regards,
Etsuro Fujita
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***
*** 2097,2102 ExecWithCheckOptions(WCOKind kind, ResultRelInfo
*resultRelInfo,
--- 2097,2121
* USING policy.
*/
case WCO_VIEW_CHECK:
+ /* See the comment in
ExecConstraints(). */
+ if (resultRelInfo->ri_PartitionRoot)
+ {
+ HeapTuple tuple =
ExecFetchSlotTuple(slot);
+ TupleDesc old_tupdesc =
RelationGetDescr(rel);
+ TupleConversionMap *map;
+
+ rel =
resultRelInfo->ri_PartitionRoot;
+ tupdesc = RelationGetDescr(rel);
+ /* a reverse map */
+ map =
convert_tuples_by_name(old_tupdesc, tupdesc,
+
gettext_noop("could not convert row type"));
+ if (map != NULL)
+ {
+ tuple =
do_convert_tuple(tuple, map);
+ ExecStoreTuple(tuple,
slot, InvalidBuffer, false);
+ }
+ }
+
insertedCols =
GetInsertedColumns(resultRelInfo, estate);
updatedCols =
GetUpdatedColumns(resultRelInfo, estate);
modifiedCols = bms_union(insertedCols,
updatedCols);
*** a/src/test/regress/expected/updatable_views.out
--- b/src/test/regress/expected/updatable_views.out
***
*** 2424,2429 select tableoid::regclass, * from pt;
create view ptv_wco as select * from pt where a = 0 with check option;
insert into ptv_wco values (1, 2);
ERROR: new row violates check option for view "ptv_wco"
! DETAIL: Failing row contains (2, 1).
drop view ptv, ptv_wco;
drop table pt, pt1, pt11;
--- 2424,2429
create view ptv_wco as select * from pt where a = 0 with check option;
insert into ptv_wco values (1, 2);
ERROR: new row violates check option for view "ptv_wco"
! DETAIL: Failing row contains (1, 2).
drop view ptv, ptv_wco;
drop table pt, pt1, pt11;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Add support for tuple routing to foreign partitions
On 2017/07/05 9:13, Amit Langote wrote:
On 2017/06/29 20:20, Etsuro Fujita wrote:
In relation to this, I also allowed expand_inherited_rtentry() to
build an RTE and AppendRelInfo for each partition of a partitioned table
that is an INSERT target, as mentioned by Amit in [1], by modifying
transformInsertStmt() so that we set the inh flag for the target table's
RTE to true when the target table is a partitioned table.
About this part, Robert sounded a little unsure back then [1]; his words:
"Doing more inheritance expansion early is probably not a good idea
because it likely sucks for performance, and that's especially unfortunate
if it's only needed for foreign tables."
But...
The partition
RTEs are not only referenced by FDWs, but used in selecting relation
aliases for EXPLAIN (see below) as well as extracting plan dependencies in
setref.c so that we force rebuilding of the plan on any change to
partitions. (We do replanning on FDW table-option changes to foreign
partitions, currently. See regression tests in the attached patch.)
...this part means we cannot really avoid locking at least the foreign
partitions during the planning stage, which we currently don't, as we
don't look at partitions at all before ExecSetupPartitionTupleRouting() is
called by ExecInitModifyTable().
Another case where we need inheritance expansion during the planning
stage would probably be INSERT .. ON CONFLICT into a partitioned table,
I guess. We don't support that yet, but if implementing that, I guess
we would probably need to look at each partition and do something like
infer_arbiter_indexes() for each partition during the planner stage.
Not sure.
Since we are locking *all* the partitions anyway, it may be better to
shift the cost of locking to the planner by doing the inheritance
expansion even in the insert case (for partitioned tables) and avoid
calling the lock manager again in the executor when setting up
tuple-routing data structures (that is, in ExecSetupPartitionTupleRouting).
We need the execution-time lock, anyway. See the comments from Robert
in [3].
An idea that Robert recently mentioned on the nearby "UPDATE partition
key" thread [2] seems relevant in this regard. By that idea,
expand_inherited_rtentry(), instead of reading in the partition OIDs in
the old catalog order, will instead call
RelationBuildPartitionDispatchInfo(), which locks the partitions and
returns their OIDs in the canonical order. If we store the foreign
partition RT indexes in that order in ModifyTable.partition_rels
introduced by this patch, then the following code added by the patch could
be made more efficient (as also explained in aforementioned Robert's email):
+foreach(l, node->partition_rels)
+{
+Index rti = lfirst_int(l);
+Oid relid = getrelid(rti, estate->es_range_table);
+boolfound;
+int j;
+
+/* First, find the ResultRelInfo for the partition */
+found = false;
+for (j = 0; j < mtstate->mt_num_partitions; j++)
+{
+resultRelInfo = partitions + j;
+
+if (RelationGetRelid(resultRelInfo->ri_RelationDesc) ==
relid)
+{
+Assert(mtstate->mt_partition_indexes[i] == 0);
+mtstate->mt_partition_indexes[i] = j;
+found = true;
+break;
+}
+}
+if (!found)
+elog(ERROR, "failed to find ResultRelInfo for rangetable
index %u", rti);
Agreed. I really want to improve this based on that idea.
Thank you for the comments!
Best regards,
Etsuro Fujita
[3]
https://www.postgresql.org/message-id/ca+tgmoyiwvicdri3zk+quxj1r7umu9t_kdnq+17pcwgzrbz...@mail.gmail.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Another comment typo in execMain.c
Here is a comment in ExecFindPartition() in execMain.c: /* * First check the root table's partition constraint, if any. No point in * routing the tuple it if it doesn't belong in the root table itself. */ I think that in the second sentence "it" just before "if" is a typo. Attached is a small patch for fixing that. Best regards, Etsuro Fujita diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 0f08283..06b467b 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -3310,7 +3310,7 @@ ExecFindPartition(ResultRelInfo *resultRelInfo, PartitionDispatch *pd, /* * First check the root table's partition constraint, if any. No point in -* routing the tuple it if it doesn't belong in the root table itself. +* routing the tuple if it doesn't belong in the root table itself. */ if (resultRelInfo->ri_PartitionCheck) ExecPartitionCheck(resultRelInfo, slot, estate); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Oddity in error handling of constraint violation in ExecConstraints for partitioned tables
Here is an example:
postgres=# create table col_desc (a int, b int) partition by list (a);
postgres=# create table col_desc_1 partition of col_desc for values in (1);
postgres=# alter table col_desc_1 add check (b > 0);
postgres=# create role col_desc_user;
postgres=# grant insert on col_desc to col_desc_user;
postgres=# revoke select on col_desc from col_desc_user;
postgres=# set role col_desc_user;
postgres=> insert into col_desc values (1, -1) returning 1;
ERROR: new row for relation "col_desc_1" violates check constraint
"col_desc_1_b_check"
DETAIL: Failing row contains (a, b) = (1, -1).
Looks good, but
postgres=> reset role;
postgres=# create table result (f1 text default 'foo', f2 text default
'bar', f3 int);
postgres=# grant insert on result to col_desc_user;
postgres=# set role col_desc_user;
postgres=> with t as (insert into col_desc values (1, -1) returning 1)
insert into result (f3) select * from t;
ERROR: new row for relation "col_desc_1" violates check constraint
"col_desc_1_b_check"
Looks odd to me because the error message doesn't show any DETAIL info;
since the CTE query, which produces the message, is the same as the
above query, the message should also be the same as the one for the
above query. I think the reason for that is: ExecConstraints looks at
an incorrect resultRelInfo to build the message for a violating tuple
that has been routed *in the case where the partitioned table isn't the
primary ModifyTable node*, which leads to deriving an incorrect
modifiedCols and then invoking ExecBuildSlotValueDescription with the
wrong bitmap. I think this should be fixed. Attached is a patch for that.
Best regards,
Etsuro Fujita
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***
*** 92,97 static bool ExecCheckRTEPermsModified(Oid relOid, Oid userid,
--- 92,99
Bitmapset *modifiedCols,
AclMode requiredPerms);
static void ExecCheckXactReadOnly(PlannedStmt *plannedstmt);
+ static ResultRelInfo *ExecFindResultRelInfo(EState *estate, Oid relid,
+
bool missing_ok);
static char *ExecBuildSlotValueDescription(Oid reloid,
TupleTableSlot *slot,
TupleDesc tupdesc,
***
*** 1360,1365 InitResultRelInfo(ResultRelInfo *resultRelInfo,
--- 1362,1392
}
/*
+ * ExecFindResultRelInfo -- find the ResultRelInfo struct for given relation
OID
+ *
+ * If no such struct, either return NULL or throw error depending on
missing_ok
+ */
+ static ResultRelInfo *
+ ExecFindResultRelInfo(EState *estate, Oid relid, bool missing_ok)
+ {
+ ResultRelInfo *rInfo;
+ int nr;
+
+ rInfo = estate->es_result_relations;
+ nr = estate->es_num_result_relations;
+ while (nr > 0)
+ {
+ if (RelationGetRelid(rInfo->ri_RelationDesc) == relid)
+ return rInfo;
+ rInfo++;
+ nr--;
+ }
+ if (!missing_ok)
+ elog(ERROR, "failed to find ResultRelInfo for relation %u",
relid);
+ return NULL;
+ }
+
+ /*
*ExecGetTriggerResultRel
*
* Get a ResultRelInfo for a trigger target relation. Most of the time,
***
*** 1379,1399 ResultRelInfo *
ExecGetTriggerResultRel(EState *estate, Oid relid)
{
ResultRelInfo *rInfo;
- int nr;
ListCell *l;
Relationrel;
MemoryContext oldcontext;
/* First, search through the query result relations */
! rInfo = estate->es_result_relations;
! nr = estate->es_num_result_relations;
! while (nr > 0)
! {
! if (RelationGetRelid(rInfo->ri_RelationDesc) == relid)
! return rInfo;
! rInfo++;
! nr--;
! }
/* Nope, but maybe we already made an extra ResultRelInfo for it */
foreach(l, estate->es_trig_target_relations)
{
--- 1406,1419
ExecGetTriggerResultRel(EState *estate, Oid relid)
{
ResultRelInfo *rInfo;
ListCell *l;
Relationrel;
MemoryContext oldcontext;
/* First, search through the query result relations */
! rInfo = ExecFindResultRelInfo(estate, relid, true);
! if (rInfo)
! return rInfo;
/* Nope, but maybe we already made an extra ResultRelInfo for it */
foreach(l, estate->es_trig_target_relations)
{
***
*** 1828,1833 ExecPartitionCheck(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot,
--- 1848,1854
Re: [HACKERS] Update comment in ExecPartitionCheck
On 2017/07/04 18:15, Amit Langote wrote:
On 2017/07/04 17:55, Etsuro Fujita wrote:
This comment in an error handling in ExecPartitionCheck():
if (!ExecCheck(resultRelInfo->ri_PartitionCheckExpr, econtext))
{
char *val_desc;
Relationorig_rel = rel;
/* See the comment above. */
if (resultRelInfo->ri_PartitionRoot)
should be updated because we don't have any comment on that above in the
code. Since we have a comment on that in ExecConstraints() defined just
below that function, I think the comment should be something like this:
"See the comment in ExecConstraints().". Attached is a patch for that.
Thanks for fixing that. I forgot to do the same in the patch that got
committed as 15ce775faa428 [1], which moved that code block from
ExecConstraints() to ExecPartitionCheck().
Thanks for the explanation!
In relation to this, I found odd behavior in the error handling. Since
that is another topic, I'll start a new thread.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Update comment in ExecPartitionCheck
This comment in an error handling in ExecPartitionCheck():
if (!ExecCheck(resultRelInfo->ri_PartitionCheckExpr, econtext))
{
char *val_desc;
Relationorig_rel = rel;
/* See the comment above. */
if (resultRelInfo->ri_PartitionRoot)
should be updated because we don't have any comment on that above in the
code. Since we have a comment on that in ExecConstraints() defined just
below that function, I think the comment should be something like this:
"See the comment in ExecConstraints().". Attached is a patch for that.
Best regards,
Etsuro Fujita
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 0f08283..dcf685a 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1864,7 +1864,7 @@ ExecPartitionCheck(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot,
char *val_desc;
Relationorig_rel = rel;
- /* See the comment above. */
+ /* See the comment in ExecConstraints(). */
if (resultRelInfo->ri_PartitionRoot)
{
HeapTuple tuple = ExecFetchSlotTuple(slot);
--
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] UPDATE of partition key
On 2017/07/03 18:54, Amit Langote wrote: On 2017/07/02 20:10, Robert Haas wrote: But that seems like it wouldn't be too hard to fix: let's have expand_inherited_rtentry() expand the partitioned table in the same order that will be used by ExecSetupPartitionTupleRouting(). That's really what I wanted when updating the patch for tuple-routing to foreign partitions. (I don't understand the issue discussed here, though.) That seems pretty easy to do - just have expand_inherited_rtentry() notice that it's got a partitioned table and call RelationGetPartitionDispatchInfo() instead of find_all_inheritors() to produce the list of OIDs. Seems like a good idea. Interesting idea. If we are going to do this, I think we may need to modify RelationGetPartitionDispatchInfo() a bit or invent an alternative that does not do as much work. Currently, it assumes that it's only ever called by ExecSetupPartitionTupleRouting() and hence also generates PartitionDispatchInfo objects for partitioned child tables. We don't need that if called from within the planner. Actually, it seems that RelationGetPartitionDispatchInfo() is too coupled with its usage within the executor, because there is this comment: /* * We keep the partitioned ones open until we're done using the * information being collected here (for example, see * ExecEndModifyTable). */ Yeah, we need some refactoring work. Is anyone working on that? 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] Bug in ExecModifyTable function and trigger issues for foreign tables
On 2017/06/16 21:29, Etsuro Fujita wrote: On 2017/06/16 19:26, Ashutosh Bapat wrote: That issue has not been addressed. The reason stated was that it would make code complicated. But I have not had chance to look at how complicated would be and assess myself whether that's worth the trouble. I have to admit that what I proposed upthread is a quick-and-dirty kluge. One thing I thought to address your concern was to move rewriteTargetListUD entirely from the rewriter to the planner when doing inherited UPDATE/DELETE, but I'm not sure that's a good idea, because at least I think that would need a lot more changes to the rewriter. I'll have second thought about this, so I'll mark this as waiting on author. 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] Declarative partitioning - another take
Hi Maksim, On 2017/04/07 19:52, Maksim Milyutin wrote: On 07.04.2017 13:05, Etsuro Fujita wrote: On 2016/12/09 19:46, Maksim Milyutin wrote: I would like to work on two tasks: - insert (and eventually update) tuple routing for foreign partition. - the ability to create an index on the parent and have all of the children inherit it; There seems to be no work on the first one, so I'd like to work on that. Yes, you can start to work on this, I'll join later as a reviewer. Great! I added the patch to the next commitfest: https://commitfest.postgresql.org/14/1184/ Sorry for the delay. 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
[HACKERS] Add support for tuple routing to foreign partitions
Hi, Here is a patch for $subject. This is based on Amit's original patch (patch '0007-Tuple-routing-for-partitioned-tables-15.patch' in [1]). Main changes are: * I like Amit's idea that for each partition that is a foreign table, the FDW performs query planning in the same way as in non-partition cases, but the changes to make_modifytable() in the original patch that creates a working-copy of Query to pass to PlanForeignModify() seem unacceptable. So, I modified the changes so that we create more-valid-looking copies of Query and ModifyTable with the foreign partition as target, as I said before. In relation to this, I also allowed expand_inherited_rtentry() to build an RTE and AppendRelInfo for each partition of a partitioned table that is an INSERT target, as mentioned by Amit in [1], by modifying transformInsertStmt() so that we set the inh flag for the target table's RTE to true when the target table is a partitioned table. The partition RTEs are not only referenced by FDWs, but used in selecting relation aliases for EXPLAIN (see below) as well as extracting plan dependencies in setref.c so that we force rebuilding of the plan on any change to partitions. (We do replanning on FDW table-option changes to foreign partitions, currently. See regression tests in the attached patch.) * The original patch added tuple routing to foreign partitions in COPY FROM, but I'm not sure the changes to BeginCopy() in the patch are the right way to go. For example, the patch passes a dummy empty Query to PlanForeignModify() to make FDWs perform query planning, but I think FDWs would be surprised by the Query. Currently, we don't support COPY to foreign tables, so ISTM that it's better to revisit this issue when adding support for COPY to foreign tables. So, I dropped the COPY part. * I modified explain.c so that EXPLAIN for an INSERT into a partitioned table displays partitions just below the ModifyTable node, and shows remote queries for foreign partitions in the same way as EXPLAIN for UPDATE/DELETE cases. Here is an example: postgres=# explain verbose insert into pt values (1), (2); QUERY PLAN --- Insert on public.pt (cost=0.00..0.03 rows=2 width=4) Foreign Insert on public.fp1 Remote SQL: INSERT INTO public.t1(a) VALUES ($1) Foreign Insert on public.fp2 Remote SQL: INSERT INTO public.t2(a) VALUES ($1) -> Values Scan on "*VALUES*" (cost=0.00..0.03 rows=2 width=4) Output: "*VALUES*".column1 (7 rows) Comments are welcome! Anyway, I'll add this to the next commitfest. Best regards, Etsuro Fujita [1] https://www.postgresql.org/message-id/aa874eaf-cd3b-0f75-c647-6c0ea823781e%40lab.ntt.co.jp *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 6931,6936 NOTICE: drop cascades to foreign table bar2 --- 6931,7074 drop table loct1; drop table loct2; -- === + -- test tuple routing to foreign-table partitions + -- === + create table pt (a int, b int) partition by list (a); + create table locp partition of pt for values in (1); + create table locfoo (a int check (a in (2)), b int); + create foreign table remp partition of pt for values in (2) server loopback options (table_name 'locfoo'); + explain (verbose, costs off) + insert into pt values (1, 1), (2, 1); +QUERY PLAN + - + Insert on public.pt +Insert on public.locp +Foreign Insert on public.remp + Remote SQL: INSERT INTO public.locfoo(a, b) VALUES ($1, $2) +-> Values Scan on "*VALUES*" + Output: "*VALUES*".column1, "*VALUES*".column2 + (6 rows) + + insert into pt values (1, 1), (2, 1); + select tableoid::regclass, * FROM pt; + tableoid | a | b + --+---+--- + locp | 1 | 1 + remp | 2 | 1 + (2 rows) + + select tableoid::regclass, * FROM locp; + tableoid | a | b + --+---+--- + locp | 1 | 1 + (1 row) + + select tableoid::regclass, * FROM remp; + tableoid | a | b + --+---+--- + remp | 2 | 1 + (1 row) + + explain (verbose, costs off) + insert into pt values (1, 2), (2, 2) returning *; +QUERY PLAN + + Insert on public.pt +Output: pt.a, pt.b +Insert on public.locp +Foreign Insert on public.remp + Remote SQL: INSERT INTO public.locfoo(a, b) VALUES ($1, $2) RETURNING a, b +-> Values Scan on "*VALUES*" + Output: "*VALUES*".column1, &q
Re: [HACKERS] Missing comment for create_modifytable_path
On 2017/06/23 2:53, Robert Haas wrote: On Thu, Jun 15, 2017 at 4:40 AM, Etsuro Fujita <fujita.ets...@lab.ntt.co.jp> wrote: While working on adding support for tuple routing for foreign partitions, I noticed that in create_modifytable_path, we forgot to add a comment on its new argument 'partitioned_rels'. Attached a patch for including that in the comments for that function. Committed with a slight adjustment. Thank you for committing this patch (and another one)! 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] Useless code in ExecInitModifyTable
On 2017/06/21 23:52, Robert Haas wrote: You're right that there is a comment missing from the function header, but it's not too hard to figure out what it should be. Just consult the definition of ModifyTable itself: /* RT indexes of non-leaf tables in a partition tree */ List *partitioned_rels; I agree on that point, but I think it's better to add the missing comment for the create_modifytable_path argument, as proposed in [1]. Best regards, Etsuro Fujita [1] https://www.postgresql.org/message-id/e87c4a6d-23d7-5e7c-e8db-44ed418eb5d1%40lab.ntt.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Comment typo in execMain.c
Here is a patch to fix a typo in a comment in ExecWithCheckOptions(): s/as as/as/. Best regards, Etsuro Fujita diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 7f460bd..9dbe175 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -2093,8 +2093,8 @@ ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo, * the permissions on the relation (that is, if the user * could view it directly anyway). For RLS violations, we * don't include the data since we don't know if the user -* should be able to view the tuple as as that depends on -* the USING policy. +* should be able to view the tuple as that depends on the +* USING policy. */ case WCO_VIEW_CHECK: insertedCols = GetInsertedColumns(resultRelInfo, estate); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Useless code in ExecInitModifyTable
Commit d3cc37f1d801a6b5cad9bf179274a8d767f1ee50 added this to
ExecInitModifyTable:
+ /* The root table RT index is at the head of the partitioned_rels
list */
+ if (node->partitioned_rels)
+ {
+ Index root_rti;
+ Oid root_oid;
+
+ root_rti = linitial_int(node->partitioned_rels);
+ root_oid = getrelid(root_rti, estate->es_range_table);
+ rel = heap_open(root_oid, NoLock); /* locked by InitPlan */
+ }
but I noticed that that function doesn't use the relation descriptor at
all. Since partitioned_rels is given in case of an UPDATE/DELETE on a
partitioned table, the relation is opened in that case, but the relation
descriptor isn't referenced at all in initializing WITH CHECK OPTION
constraints and/or RETURNING projections. (The mtstate->resultRelinfo
array is referenced in those initialization, instead.) So, I'd like to
propose to remove this from that function. Attached is a patch for that.
Best regards,
Etsuro Fujita
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***
*** 45,51
#include "foreign/fdwapi.h"
#include "miscadmin.h"
#include "nodes/nodeFuncs.h"
- #include "parser/parsetree.h"
#include "storage/bufmgr.h"
#include "storage/lmgr.h"
#include "utils/builtins.h"
--- 45,50
***
*** 1767,1786 ExecInitModifyTable(ModifyTable *node, EState *estate, int
eflags)
estate->es_result_relation_info = saved_resultRelInfo;
- /* The root table RT index is at the head of the partitioned_rels list
*/
- if (node->partitioned_rels)
- {
- Index root_rti;
- Oid root_oid;
-
- root_rti = linitial_int(node->partitioned_rels);
- root_oid = getrelid(root_rti, estate->es_range_table);
- rel = heap_open(root_oid, NoLock); /* locked by
InitPlan */
- }
- else
- rel = mtstate->resultRelInfo->ri_RelationDesc;
-
/* Build state for INSERT tuple routing */
if (operation == CMD_INSERT &&
rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
--- 1766,1773
estate->es_result_relation_info = saved_resultRelInfo;
/* Build state for INSERT tuple routing */
+ rel = mtstate->resultRelInfo->ri_RelationDesc;
if (operation == CMD_INSERT &&
rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
***
*** 1959,1968 ExecInitModifyTable(ModifyTable *node, EState *estate, int
eflags)
mtstate->ps.ps_ExprContext = NULL;
}
- /* Close the root partitioned rel if we opened it above. */
- if (rel != mtstate->resultRelInfo->ri_RelationDesc)
- heap_close(rel, NoLock);
-
/*
* If needed, Initialize target list, projection and qual for ON
CONFLICT
* DO UPDATE.
--- 1946,1951
--
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] Missing comment for ResultRelInfo in execnodes.h
On 2017/06/21 3:30, Peter Eisentraut wrote: On 6/20/17 05:50, Etsuro Fujita wrote: Here is a small patch to add a comment on its new member PartitionRoot. The existing comment style is kind of unusual. How about the attached to clean it up a bit? +1 for that change. 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
[HACKERS] Missing comment for ResultRelInfo in execnodes.h
Here is a small patch to add a comment on its new member PartitionRoot. Best regards, Etsuro Fujita diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index d33392f..7175a42 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -365,6 +365,7 @@ typedef struct JunkFilter * onConflictSetWhere list of ON CONFLICT DO UPDATE exprs (qual) * PartitionCheck partition check expression * PartitionCheckExpr partition check expression state + * PartitionRoot relation descriptor for root partitioned table * */ typedef struct ResultRelInfo -- 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] Bug in ExecModifyTable function and trigger issues for foreign tables
On 2017/06/16 19:26, Ashutosh Bapat wrote: On Fri, Jun 16, 2017 at 3:35 PM, Etsuro Fujita Ashutosh mentioned his concern about what I proposed above before [2], but I'm not sure we should address that. And there have been no opinions from him (or anyone else) since then. So, I'd like to leave that for committer (ie, +1 for Ready for Committer). That issue has not been addressed. The reason stated was that it would make code complicated. But I have not had chance to look at how complicated would be and assess myself whether that's worth the trouble. I have to admit that what I proposed upthread is a quick-and-dirty kluge. One thing I thought to address your concern was to move rewriteTargetListUD entirely from the rewriter to the planner when doing inherited UPDATE/DELETE, but I'm not sure that's a good idea, because at least I think that would need a lot more changes to the rewriter. 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] Bug in ExecModifyTable function and trigger issues for foreign tables
On 2017/06/16 19:26, Ashutosh Bapat wrote: Also, I don't see any discussion about my concern [3] about a parent with child from multiple foreign servers with different FDWs. So, I am not sure whether the patch really fixes the problem in its entirety. The patch would allow child tables to have different foreign servers with different FDWs since it applies rewriteTargetListUD to each child table when generating a modified query with that child table with the target in inheritance_planner. I didn't any regression tests for that, though. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables
On 2017/06/16 0:05, Ildus Kurbangaliev wrote: I wrote: One approach I came up with to fix this issue is to rewrite the targetList entries of an inherited UPDATE/DELETE properly using rewriteTargetListUD, when generating a plan for each child table in inheritance_planner. Attached is a WIP patch for that. Maybe I am missing something, though. While updating the patch, I noticed the patch rewrites the UPDATE targetList incorrectly in some cases; rewrite_inherited_tlist I added to adjust_appendrel_attrs (1) removes all junk items from the targetList and (2) adds junk items for the child table using rewriteTargetListUD, but it's wrong to drop all junk items in cases where there are junk items for some other reasons than rewriteTargetListUD. Consider junk items containing MULTIEXPR SubLink. One way I came up with to fix this is to change (1) to only remove junk items with resname; since junk items added by rewriteTargetListUD should have resname (note: we would need resname to call ExecFindJunkAttributeInTlist at execution time!) while other junk items wouldn't have resname (see transformUpdateTargetList), we could correctly replace junk items added by rewriteTargetListUD for the parent with ones for the child, by that change. I might be missing something, though. Comments welcome. I updated the patch that way. Please find attached an updated version. Other changes: * Moved the initialization for "tupleid" I added in ExecModifyTable as discussed before, which I think is essentially the same as proposed by Ildus in [1], since I think that would be more consistent with "oldtuple". * Added regression tests. Anyway I'll add this to the next commitfest. Checked the latest patch. Looks good. Shouldn't this patch be backported to 9.6 and 10beta? The bug affects them too. Thank you for the review! The bug is in foreign table inheritance, which was supported in 9.5, so I think this patch should be backported to 9.5. Ashutosh mentioned his concern about what I proposed above before [2], but I'm not sure we should address that. And there have been no opinions from him (or anyone else) since then. So, I'd like to leave that for committer (ie, +1 for Ready for Committer). Attached is a slightly-updated version; I renamed some variables used in rewrite_inherited_tlist() to match other existing code in prepunion.c and revised some comments a bit. I didn't make any functional changes, so I'll keep this Ready for Committer. Best regards, Etsuro Fujita [2] https://www.postgresql.org/message-id/CAFjFpRfTpamoUz6fNyk6gPh%3DecfBJjbUHJNKbDxscpyPJ3FfjQ%40mail.gmail.com *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 6924,6929 update bar set f2 = f2 + 100 returning *; --- 6924,7038 7 | 277 (6 rows) + -- Test that UPDATE/DELETE with inherited target works with row-level triggers + CREATE TRIGGER trig_row_before + BEFORE UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + CREATE TRIGGER trig_row_after + AFTER UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + explain (verbose, costs off) + update bar set f2 = f2 + 100; + QUERY PLAN + -- + Update on public.bar +Update on public.bar +Foreign Update on public.bar2 + Remote SQL: UPDATE public.loct2 SET f2 = $2 WHERE ctid = $1 RETURNING f1, f2, f3 +-> Seq Scan on public.bar + Output: bar.f1, (bar.f2 + 100), bar.ctid +-> Foreign Scan on public.bar2 + Output: bar2.f1, (bar2.f2 + 100), bar2.f3, bar2.ctid, bar2.* + Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE + (9 rows) + + update bar set f2 = f2 + 100; + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (3,333,33),NEW: (3,433,33) + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (4,344,44),NEW: (4,444,44) + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (7,277,77),NEW: (7,377,77) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (3,333,33),NEW: (3,433,33) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (4,344,44),NEW: (4,444,44) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (7,277,77),NEW: (7,377,77) + explain (verbose, costs off) + update bar set (f2,f1) = (select f1, f2 from bar2 where f3 = 77) where f1 = 7; + QUERY PLAN + --- + Update on public.bar +Update on public.bar +Foreign Update on public.bar2 + Remote SQ
[HACKERS] Missing comment for create_modifytable_path
While working on adding support for tuple routing for foreign partitions, I noticed that in create_modifytable_path, we forgot to add a comment on its new argument 'partitioned_rels'. Attached a patch for including that in the comments for that function. Best regards, Etsuro Fujita diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index ec4a093..46bc1df 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -3159,6 +3159,8 @@ create_lockrows_path(PlannerInfo *root, RelOptInfo *rel, * 'operation' is the operation type * 'canSetTag' is true if we set the command tag/es_processed * 'nominalRelation' is the parent RT index for use of EXPLAIN + * 'partitioned_rels' is an integer list of RT indexes of non-leaf tables in + * the partition tree if UPDATE/DELETE to a partitioned table, else NIL * 'resultRelations' is an integer list of actual RT indexes of target rel(s) * 'subpaths' is a list of Path(s) producing source data (one per rel) * 'subroots' is a list of PlannerInfo structs (one per rel) -- 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] Update comments in nodeModifyTable.c
On 2017/06/07 0:30, Robert Haas wrote:
On Mon, Jun 5, 2017 at 4:45 AM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
While working on [1], I noticed that the comment in ExecModifyTable:
* Foreign table updates have a wholerow attribute when the
* relation has an AFTER ROW trigger.
is not 100% correct because a foreign table has a wholerow attrubute when
the relation has an AFTER ROW or BEFORE ROW trigger (see
rewriteTargetListUD). So I'd propose s/an AFTER ROW trigger/a row-level
trigger/. Attached is a patch for that.
That seems better, but looking at rewriteTargetListUD, it seems that
the actual rule is that this happens when there is a row-level on
either UPDATE or DELETE. If there is only a row-level trigger on
INSERT, then it is not done. Perhaps we should try to include that
detail in the comment as well.
Agreed, but I think it's better to add that detail to this comment in
ExecInitModifyTable:
* Initialize the junk filter(s) if needed. INSERT queries need a
filter
* if there are any junk attrs in the tlist. UPDATE and DELETE always
* need a filter, since there's always a junk 'ctid' or 'wholerow'
* attribute present --- no need to look first.
I'd also like to propose to update the third sentence in this comment,
since there isn't necessarily a ctid or wholerow in the UPDATE/DELETE
tlist when the result relation is a foreign table.
Attached is an updated version of the patch.
Best regards,
Etsuro Fujita
diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index cf555fe..07bc870 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1554,7 +1554,7 @@ ExecModifyTable(ModifyTableState *node)
* the old relation tuple.
*
* Foreign table updates have a wholerow
attribute when the
-* relation has an AFTER ROW trigger. Note
that the wholerow
+* relation has a row-level trigger. Note that
the wholerow
* attribute does not carry system columns.
Foreign table
* triggers miss seeing those, except that we
know enough here
* to set t_tableOid. Quite separately from
this, the FDW may
@@ -2025,7 +2025,11 @@ ExecInitModifyTable(ModifyTable *node, EState *estate,
int eflags)
* Initialize the junk filter(s) if needed. INSERT queries need a
filter
* if there are any junk attrs in the tlist. UPDATE and DELETE always
* need a filter, since there's always a junk 'ctid' or 'wholerow'
-* attribute present --- no need to look first.
+* attribute present if not foreign table, and if foreign table, there
+* are always junk attributes present the FDW needs to identify the
exact
+* row to update or delete --- no need to look first. For foreign
tables,
+* there's also a wholerow attribute when the relation has a row-level
+* trigger on UPDATE/DELETE but not on INSERT.
*
* If there are multiple result relations, each one needs its own junk
* filter. Note multiple rels are only possible for UPDATE/DELETE, so
we
@@ -2093,7 +2097,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate,
int eflags)
else if (relkind ==
RELKIND_FOREIGN_TABLE)
{
/*
-* When there is an AFTER
trigger, there should be a
+* When there is a row-level
trigger, there should be a
* wholerow attribute.
*/
j->jf_junkAttNo =
ExecFindJunkAttribute(j, "wholerow");
--
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] Bug in ExecModifyTable function and trigger issues for foreign tables
On 2017/06/05 17:39, Ashutosh Bapat wrote: On Fri, Jun 2, 2017 at 2:40 PM, Etsuro Fujita While updating the patch, I noticed the patch rewrites the UPDATE targetList incorrectly in some cases; rewrite_inherited_tlist I added to adjust_appendrel_attrs (1) removes all junk items from the targetList and (2) adds junk items for the child table using rewriteTargetListUD, but it's wrong to drop all junk items in cases where there are junk items for some other reasons than rewriteTargetListUD. Consider junk items containing MULTIEXPR SubLink. One way I came up with to fix this is to change (1) to only remove junk items with resname; since junk items added by rewriteTargetListUD should have resname (note: we would need resname to call ExecFindJunkAttributeInTlist at execution time!) while other junk items wouldn't have resname (see transformUpdateTargetList), we could correctly replace junk items added by rewriteTargetListUD for the parent with ones for the child, by that change. I might be missing something, though. Comments welcome. I haven't looked at the patch, but that doesn't look right. In future some code path other than rewriteTargetListUD() may add junk items with resname and this fix will remove those junk items as well. Yeah, I thought that too. I think we could replace junk tlist entries added by rewriteTargetListUD() more safely, by adding a lot more code, but I'm not sure it's worth complicating the code at the current stage. 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
[HACKERS] Update comments in nodeModifyTable.c
While working on [1], I noticed that the comment in ExecModifyTable:
* Foreign table updates have a wholerow attribute when the
* relation has an AFTER ROW trigger.
is not 100% correct because a foreign table has a wholerow attrubute
when the relation has an AFTER ROW or BEFORE ROW trigger (see
rewriteTargetListUD). So I'd propose s/an AFTER ROW trigger/a row-level
trigger/. Attached is a patch for that.
Best regards,
Etsuro Fujita
[1]
https://www.postgresql.org/message-id/a31f779e-9cb8-1ea5-71a6-bca6adbfa6a4%40lab.ntt.co.jp
diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index cf555fe..5dde93c 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -1554,7 +1554,7 @@ ExecModifyTable(ModifyTableState *node)
* the old relation tuple.
*
* Foreign table updates have a wholerow
attribute when the
-* relation has an AFTER ROW trigger. Note
that the wholerow
+* relation has a row-level trigger. Note that
the wholerow
* attribute does not carry system columns.
Foreign table
* triggers miss seeing those, except that we
know enough here
* to set t_tableOid. Quite separately from
this, the FDW may
@@ -2093,7 +2093,7 @@ ExecInitModifyTable(ModifyTable *node, EState *estate,
int eflags)
else if (relkind ==
RELKIND_FOREIGN_TABLE)
{
/*
-* When there is an AFTER
trigger, there should be a
+* When there is a row-level
trigger, there should be a
* wholerow attribute.
*/
j->jf_junkAttNo =
ExecFindJunkAttribute(j, "wholerow");
--
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] Bug in ExecModifyTable function and trigger issues for foreign tables
On 2017/06/02 18:10, Etsuro Fujita wrote: On 2017/05/16 21:36, Etsuro Fujita wrote: One approach I came up with to fix this issue is to rewrite the targetList entries of an inherited UPDATE/DELETE properly using rewriteTargetListUD, when generating a plan for each child table in inheritance_planner. Attached is a WIP patch for that. Maybe I am missing something, though. While updating the patch, I noticed the patch rewrites the UPDATE targetList incorrectly in some cases; rewrite_inherited_tlist I added to adjust_appendrel_attrs (1) removes all junk items from the targetList and (2) adds junk items for the child table using rewriteTargetListUD, but it's wrong to drop all junk items in cases where there are junk items for some other reasons than rewriteTargetListUD. Consider junk items containing MULTIEXPR SubLink. One way I came up with to fix this is to change (1) to only remove junk items with resname; since junk items added by rewriteTargetListUD should have resname (note: we would need resname to call ExecFindJunkAttributeInTlist at execution time!) while other junk items wouldn't have resname (see transformUpdateTargetList), we could correctly replace junk items added by rewriteTargetListUD for the parent with ones for the child, by that change. I might be missing something, though. Comments welcome. I updated the patch that way. Please find attached an updated version. Other changes: * Moved the initialization for "tupleid" I added in ExecModifyTable as discussed before, which I think is essentially the same as proposed by Ildus in [1], since I think that would be more consistent with "oldtuple". * Added regression tests. Anyway I'll add this to the next commitfest. Best regards, Etsuro Fujita [1] https://www.postgresql.org/message-id/20170514150525.0346ba72%40postgrespro.ru *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 6924,6929 update bar set f2 = f2 + 100 returning *; --- 6924,7038 7 | 277 (6 rows) + -- Test that UPDATE/DELETE with inherited target works with row-level triggers + CREATE TRIGGER trig_row_before + BEFORE UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + CREATE TRIGGER trig_row_after + AFTER UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + explain (verbose, costs off) + update bar set f2 = f2 + 100; + QUERY PLAN + -- + Update on public.bar +Update on public.bar +Foreign Update on public.bar2 + Remote SQL: UPDATE public.loct2 SET f2 = $2 WHERE ctid = $1 RETURNING f1, f2, f3 +-> Seq Scan on public.bar + Output: bar.f1, (bar.f2 + 100), bar.ctid +-> Foreign Scan on public.bar2 + Output: bar2.f1, (bar2.f2 + 100), bar2.f3, bar2.ctid, bar2.* + Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE + (9 rows) + + update bar set f2 = f2 + 100; + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (3,333,33),NEW: (3,433,33) + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (4,344,44),NEW: (4,444,44) + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (7,277,77),NEW: (7,377,77) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (3,333,33),NEW: (3,433,33) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (4,344,44),NEW: (4,444,44) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (7,277,77),NEW: (7,377,77) + explain (verbose, costs off) + update bar set (f2,f1) = (select f1, f2 from bar2 where f3 = 77) where f1 = 7; + QUERY PLAN + --- + Update on public.bar +Update on public.bar +Foreign Update on public.bar2 + Remote SQL: UPDATE public.loct2 SET f1 = $2, f2 = $3 WHERE ctid = $1 RETURNING f1, f2, f3 +InitPlan 1 (returns $0,$1) + -> Foreign Scan on public.bar2 bar2_1 +Output: bar2_1.f1, bar2_1.f2 +Remote SQL: SELECT f1, f2 FROM public.loct2 WHERE ((f3 = 77)) +-> Seq Scan on public.bar + Output: $1, $0, NULL::record, bar.ctid + Filter: (bar.f1 = 7) +-> Foreign Scan on public.bar2 + Output: $1, $0, bar2.f3, NULL::record, bar2.ctid, bar2.* + Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 WHERE ((f1 = 7)) FOR UPDATE + (14 rows) + + update bar set (f2,f1) = (select f1, f2 from bar2 where f3 = 77) where f1 = 7; + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD:
Re: [HACKERS] Bug in ExecModifyTable function and trigger issues for foreign tables
On 2017/05/16 21:36, Etsuro Fujita wrote: One approach I came up with to fix this issue is to rewrite the targetList entries of an inherited UPDATE/DELETE properly using rewriteTargetListUD, when generating a plan for each child table in inheritance_planner. Attached is a WIP patch for that. Maybe I am missing something, though. While updating the patch, I noticed the patch rewrites the UPDATE targetList incorrectly in some cases; rewrite_inherited_tlist I added to adjust_appendrel_attrs (1) removes all junk items from the targetList and (2) adds junk items for the child table using rewriteTargetListUD, but it's wrong to drop all junk items in cases where there are junk items for some other reasons than rewriteTargetListUD. Consider junk items containing MULTIEXPR SubLink. One way I came up with to fix this is to change (1) to only remove junk items with resname; since junk items added by rewriteTargetListUD should have resname (note: we would need resname to call ExecFindJunkAttributeInTlist at execution time!) while other junk items wouldn't have resname (see transformUpdateTargetList), we could correctly replace junk items added by rewriteTargetListUD for the parent with ones for the child, by that change. I might be missing something, though. Comments welcome. 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
