121 Commits (66bde49d96a9ddacc49dcbdf1b47b5bd6e31ead5)

Author	SHA1	Message	Date
Michael Paquier	c74d49d41c	Fix many typos and inconsistencies ... Author: Alexander Lakhin Discussion: https://postgr.es/m/af27d1b3-a128-9d62-46e0-88f424397f44@gmail.com	6 years ago
Etsuro Fujita	8b6da83d16	postgres_fdw: Account for triggers in non-direct remote UPDATE planning. ... Previously, in postgresPlanForeignModify, we planned an UPDATE operation on a foreign table so that we transmit only columns that were explicitly targets of the UPDATE, so as to avoid unnecessary data transmission, but if there were BEFORE ROW UPDATE triggers on the foreign table, those triggers might change values for non-target columns, in which case we would miss sending changed values for those columns. Prevent optimizing away transmitting all columns if there are BEFORE ROW UPDATE triggers on the foreign table. This is an oversight in commit 7cbe57c34 which added triggers on foreign tables, so apply the patch all the way back to 9.4 where that came in. Author: Shohei Mochizuki Reviewed-by: Amit Langote Discussion: https://postgr.es/m/201905270152.x4R1q3qi014550@toshiba.co.jp	6 years ago
Tom Lane	8cad5adb9c	Avoid postgres_fdw crash for a targetlist entry that's just a Param. ... foreign_grouping_ok() is willing to put fairly arbitrary expressions into the targetlist of a remote SELECT that's doing grouping or aggregation on the remote side, including expressions that have no foreign component to them at all. This is possibly a bit dubious from an efficiency standpoint; but it rises to the level of a crash-causing bug if the expression is just a Param or non-foreign Var. In that case, the expression will necessarily also appear in the fdw_exprs list of values we need to send to the remote server, and then setrefs.c's set_foreignscan_references will mistakenly replace the fdw_exprs entry with a Var referencing the targetlist result. The root cause of this problem is bad design in commit e7cb7ee14: it put logic into set_foreignscan_references that IMV is postgres_fdw-specific, and yet this bug shows that it isn't postgres_fdw-specific enough. The transformation being done on fdw_exprs assumes that fdw_exprs is to be evaluated with the fdw_scan_tlist as input, which is not how postgres_fdw uses it; yet it could be the right thing for some other FDW. (In the bigger picture, setrefs.c has no business assuming this for the other expression fields of a ForeignScan either.) The right fix therefore would be to expand the FDW API so that the FDW could inform setrefs.c how it intends to evaluate these various expressions. We can't change that in the back branches though, and we also can't just summarily change setrefs.c's behavior there, or we're likely to break external FDWs. As a stopgap, therefore, hack up postgres_fdw so that it won't attempt to send targetlist entries that look exactly like the fdw_exprs entries they'd produce. In most cases this actually produces a superior plan, IMO, with less data needing to be transmitted and returned; so we probably ought to think harder about whether we should ship tlist expressions at all when they don't contain any foreign Vars or Aggs. But that's an optimization not a bug fix so I left it for later. One case where this produces an inferior plan is where the expression in question is actually a GROUP BY expression: then the restriction prevents us from using remote grouping. It might be possible to work around that (since that would reduce to group-by-a-constant on the remote side); but it seems like a pretty unlikely corner case, so I'm not sure it's worth expending effort solely to improve that. In any case the right long-term answer is to fix the API as sketched above, and then revert this hack. Per bug #15781 from Sean Johnston. Back-patch to v10 where the problem was introduced. Discussion: https://postgr.es/m/15781-2601b1002bad087c@postgresql.org	6 years ago
Etsuro Fujita	5c47049180	postgres_fdw: Fix incorrect handling of row movement for remote partitions. ... Commit 3d956d9562 added support for update row movement in postgres_fdw. This patch fixes the following issues introduced by that commit: * When a remote partition chosen to insert routed rows into was also an UPDATE subplan target rel that would be updated later, the UPDATE that used a direct modification plan modified those routed rows incorrectly because those routed rows were visible to the later UPDATE command. The right fix for this would be to have some way in postgres_fdw in which the later UPDATE command ignores those routed rows, but it seems hard to do so with the current infrastructure. For now throw an error in that case. * When a remote partition chosen to insert routed rows into was also an UPDATE subplan target rel, fmstate created for the UPDATE that used a non-direct modification plan was mistakenly overridden by another fmstate created for inserting those routed rows into the partition. This caused 1) server crash when the partition would be updated later, and 2) resource leak when the partition had been already updated. To avoid that, adjust the treatment of the fmstate for the inserting. As for #1, since we would also have the incorrectness issue as mentioned above, error out in that case as well. Update the docs to mention that postgres_fdw currently does not handle the case where a remote partition chosen to insert a routed row into is also an UPDATE subplan target rel that will be updated later. Author: Amit Langote and Etsuro Fujita Reviewed-by: Amit Langote Backpatch-through: 11 where row movement in postgres_fdw was added Discussion: https://postgr.es/m/21e7eaa4-0d4d-20c2-a1f7-c7e96f4ce440@lab.ntt.co.jp	6 years ago
Michael Paquier	249d649996	Add support TCP user timeout in libpq and the backend server ... Similarly to the set of parameters for keepalive, a connection parameter for libpq is added as well as a backend GUC, called tcp_user_timeout. Increasing the TCP user timeout is useful to allow a connection to survive extended periods without end-to-end connection, and decreasing it allows application to fail faster. By default, the parameter is 0, which makes the connection use the system default, and follows a logic close to the keepalive parameters in its handling. When connecting through a Unix-socket domain, the parameters have no effect. Author: Ryohei Nagaura Reviewed-by: Fabien Coelho, Robert Haas, Kyotaro Horiguchi, Kirk Jamison, Mikalai Keida, Takayuki Tsunakawa, Andrei Yahorau Discussion: https://postgr.es/m/EDA4195584F5064680D8130B1CA91C45367328@G01JPEXMBYT04	7 years ago
Etsuro Fujita	d50d172e51	postgres_fdw: Perform the (FINAL, NULL) upperrel operations remotely. ... The upper-planner pathification allows FDWs to arrange to push down different types of upper-stage operations to the remote side. This commit teaches postgres_fdw to do it for the (FINAL, NULL) upperrel, which is responsible for doing LockRows, LIMIT, and/or ModifyTable. This provides the ability for postgres_fdw to handle SELECT commands so that it 1) skips the LockRows step (if any) (note that this is safe since it performs early locking) and 2) pushes down the LIMIT and/or OFFSET restrictions (if any) to the remote side. This doesn't handle the INSERT/UPDATE/DELETE cases. Author: Etsuro Fujita Reviewed-By: Antonin Houska and Jeff Janes Discussion: https://postgr.es/m/87pnz1aby9.fsf@news-spur.riddles.org.uk	7 years ago
Etsuro Fujita	0269edefac	postgres_fdw: Modify regression tests for EPQ-related planning problems. ... This prevents the tests added by commit 4bbf6edfbd and adjusted by commit 99f6a17dd6 from being useless by plan changes created by an upcoming commit. Author: Etsuro Fujita Discussion: https://postgr.es/m/87pnz1aby9.fsf@news-spur.riddles.org.uk	7 years ago
Etsuro Fujita	ffab494a4d	postgres_fdw: Perform the (ORDERED, NULL) upperrel operations remotely. ... The upper-planner pathification allows FDWs to arrange to push down different types of upper-stage operations to the remote side. This commit teaches postgres_fdw to do it for the (ORDERED, NULL) upperrel, which is responsible for evaluating the query's ORDER BY ordering. Since postgres_fdw is already able to evaluate that ordering remotely for foreign baserels and foreign joinrels (see commit aa09cd242f et al.), this adds support for that for foreign grouping relations. Author: Etsuro Fujita Reviewed-By: Antonin Houska and Jeff Janes Discussion: https://postgr.es/m/87pnz1aby9.fsf@news-spur.riddles.org.uk	7 years ago
Tom Lane	428b260f87	Speed up planning when partitions can be pruned at plan time. ... Previously, the planner created RangeTblEntry and RelOptInfo structs for every partition of a partitioned table, even though many of them might later be deemed uninteresting thanks to partition pruning logic. This incurred significant overhead when there are many partitions. Arrange to postpone creation of these data structures until after we've processed the query enough to identify restriction quals for the partitioned table, and then apply partition pruning before not after creation of each partition's data structures. In this way we need not open the partition relations at all for partitions that the planner has no real interest in. For queries that can be proven at plan time to access only a small number of partitions, this patch improves the practical maximum number of partitions from under 100 to perhaps a few thousand. Amit Langote, reviewed at various times by Dilip Kumar, Jesper Pedersen, Yoshikazu Imai, and David Rowley Discussion: https://postgr.es/m/9d7c5112-cb99-6a47-d3be-cf1ee6862a1d@lab.ntt.co.jp	7 years ago
Peter Eisentraut	fc22b6623b	Generated columns ... This is an SQL-standard feature that allows creating columns that are computed from expressions rather than assigned, similar to a view or materialized view but on a column basis. This implements one kind of generated column: stored (computed on write). Another kind, virtual (computed on read), is planned for the future, and some room is left for it. Reviewed-by: Michael Paquier <michael@paquier.xyz> Reviewed-by: Pavel Stehule <pavel.stehule@gmail.com> Discussion: https://www.postgresql.org/message-id/flat/b151f851-4019-bdb1-699e-ebab07d2f40a@2ndquadrant.com	7 years ago
Tom Lane	e8d5dd6be7	Get rid of duplicate child RTE for a partitioned table. ... We've been creating duplicate RTEs for partitioned tables just because we do so for regular inheritance parent tables. But unlike regular-inheritance parents which are themselves regular tables and thus need to be scanned, partitioned tables don't need the extra RTE. This makes the conditions for building a child RTE the same as those for building an AppendRelInfo, allowing minor simplification in expand_single_inheritance_child. Since the planner's actual processing is driven off the AppendRelInfo list, nothing much changes beyond that, we just have one fewer useless RTE entry. Amit Langote, reviewed and hacked a bit by me Discussion: https://postgr.es/m/9d7c5112-cb99-6a47-d3be-cf1ee6862a1d@lab.ntt.co.jp	7 years ago
Tom Lane	8edd0e7946	Suppress Append and MergeAppend plan nodes that have a single child. ... If there's only one child relation, the Append or MergeAppend isn't doing anything useful, and can be elided. It does have a purpose during planning though, which is to serve as a buffer between parent and child Var numbering. Therefore we keep it all the way through to setrefs.c, and get rid of it only after fixing references in the plan level(s) above it. This works largely the same as setrefs.c's ancient hack to get rid of no-op SubqueryScan nodes, and can even share some code with that. Note the change to make setrefs.c use apply_tlist_labeling rather than ad-hoc code. This has the effect of propagating the child's resjunk and ressortgroupref labels, which formerly weren't propagated when removing a SubqueryScan. Doing that is demonstrably necessary for the [Merge]Append cases, and seems harmless for SubqueryScan, if only because trivial_subqueryscan is afraid to collapse cases where the resjunk marking differs. (I suspect that restriction could now be removed, though it's unclear that it'd make any new matches possible, since the outer query can't have references to a child resjunk column.) David Rowley, reviewed by Alvaro Herrera and Tomas Vondra Discussion: https://postgr.es/m/CAKJS1f_7u8ATyJ1JGTMHFoKDvZdeF-iEBhs+sM_SXowOr9cArg@mail.gmail.com	7 years ago
Tom Lane	608b167f9f	Allow user control of CTE materialization, and change the default behavior. ... Historically we've always materialized the full output of a CTE query, treating WITH as an optimization fence (so that, for example, restrictions from the outer query cannot be pushed into it). This is appropriate when the CTE query is INSERT/UPDATE/DELETE, or is recursive; but when the CTE query is non-recursive and side-effect-free, there's no hazard of changing the query results by pushing restrictions down. Another argument for materialization is that it can avoid duplicate computation of an expensive WITH query --- but that only applies if the WITH query is called more than once in the outer query. Even then it could still be a net loss, if each call has restrictions that would allow just a small part of the WITH query to be computed. Hence, let's change the behavior for WITH queries that are non-recursive and side-effect-free. By default, we will inline them into the outer query (removing the optimization fence) if they are called just once. If they are called more than once, we will keep the old behavior by default, but the user can override this and force inlining by specifying NOT MATERIALIZED. Lastly, the user can force the old behavior by specifying MATERIALIZED; this would mainly be useful when the query had deliberately been employing WITH as an optimization fence to prevent a poor choice of plan. Andreas Karlsson, Andrew Gierth, David Fetter Discussion: https://postgr.es/m/87sh48ffhb.fsf@news-spur.riddles.org.uk	7 years ago
Tom Lane	34ea1ab7fd	Split create_foreignscan_path() into three functions. ... Up to now postgres_fdw has been using create_foreignscan_path() to generate not only base-relation paths, but also paths for foreign joins and foreign upperrels. This is wrong, because create_foreignscan_path() calls get_baserel_parampathinfo() which will only do the right thing for baserels. It accidentally fails to fail for unparameterized paths, which are the only ones postgres_fdw (thought it) was handling, but we really need different APIs for the baserel and join cases. In HEAD, the best thing to do seems to be to split up the baserel, joinrel, and upperrel cases into three functions so that they can have different APIs. I haven't actually given create_foreign_join_path a different API in this commit: we should spend a bit of time thinking about just what we want to do there, since perhaps FDWs would want to do something different from the build-up-a-join-pairwise approach that get_joinrel_parampathinfo expects. In the meantime, since postgres_fdw isn't prepared to generate parameterized joins anyway, just give it a defense against trying to plan joins with lateral refs. In addition (and this is what triggered this whole mess) fix bug #15613 from Srinivasan S A, by teaching file_fdw and postgres_fdw that plain baserel foreign paths still have outer refs if the relation has lateral_relids. Add some assertions in relnode.c to catch future occurrences of the same error --- in particular, to catch other FDWs doing that, but also as backstop against core-code mistakes like the one fixed by commit bdd9a99aa. Bug #15613 also needs to be fixed in the back branches, but the appropriate fix will look quite a bit different there, since we don't want to assume that existing FDWs get the word right away. Discussion: https://postgr.es/m/15613-092be1be9576c728@postgresql.org	7 years ago
Tom Lane	4be058fe9e	In the planner, replace an empty FROM clause with a dummy RTE. ... The fact that "SELECT expression" has no base relations has long been a thorn in the side of the planner. It makes it hard to flatten a sub-query that looks like that, or is a trivial VALUES() item, because the planner generally uses relid sets to identify sub-relations, and such a sub-query would have an empty relid set if we flattened it. prepjointree.c contains some baroque logic that works around this in certain special cases --- but there is a much better answer. We can replace an empty FROM clause with a dummy RTE that acts like a table of one row and no columns, and then there are no such corner cases to worry about. Instead we need some logic to get rid of useless dummy RTEs, but that's simpler and covers more cases than what was there before. For really trivial cases, where the query is just "SELECT expression" and nothing else, there's a hazard that adding the extra RTE makes for a noticeable slowdown; even though it's not much processing, there's not that much for the planner to do overall. However testing says that the penalty is very small, close to the noise level. In more complex queries, this is able to find optimizations that we could not find before. The new RTE type is called RTE_RESULT, since the "scan" plan type it gives rise to is a Result node (the same plan we produced for a "SELECT expression" query before). To avoid confusion, rename the old ResultPath path type to GroupResultPath, reflecting that it's only used in degenerate grouping cases where we know the query produces just one grouped row. (It wouldn't work to unify the two cases, because there are different rules about where the associated quals live during query_planner.) Note: although this touches readfuncs.c, I don't think a catversion bump is required, because the added case can't occur in stored rules, only plans. Patch by me, reviewed by David Rowley and Mark Dilger Discussion: https://postgr.es/m/15944.1521127664@sss.pgh.pa.us	7 years ago
Michael Paquier	bf491a9073	Disable WAL-skipping optimization for COPY on views and foreign tables ... COPY can skip writing WAL when loading data on a table which has been created in the same transaction as the one loading the data, however this cannot work on views or foreign table as this would result in trying to flush relation files which do not exist. So disable the optimization so as commands are able to work the same way with any configuration of wal_level. Tests are added to cover the different cases, which need to have wal_level set to minimal to allow the problem to show up, and that is not the default configuration. Reported-by: Luis M. Carril, Etsuro Fujita Author: Amit Langote, Michael Paquier Reviewed-by: Etsuro Fujita Discussion: https://postgr.es/m/15552-c64aa14c5c22f63c@postgresql.org Backpatch-through: 10, where support for COPY on views has been added, while v11 has added support for COPY on foreign tables.	7 years ago
Tom Lane	77d4d88afb	Repair bogus EPQ plans generated for postgres_fdw foreign joins. ... postgres_fdw's postgresGetForeignPlan() assumes without checking that the outer_plan it's given for a join relation must have a NestLoop, MergeJoin, or HashJoin node at the top. That's been wrong at least since commit 4bbf6edfb (which could cause insertion of a Sort node on top) and it seems like a pretty unsafe thing to Just Assume even without that. Through blind good fortune, this doesn't seem to have any worse consequences today than strange EXPLAIN output, but it's clearly trouble waiting to happen. To fix, test the node type explicitly before touching Join-specific fields, and avoid jamming the new tlist into a node type that can't do projection. Export a new support function from createplan.c to avoid building low-level knowledge about the latter into FDWs. Back-patch to 9.6 where the faulty coding was added. Note that the associated regression test cases don't show any changes before v11, apparently because the tests back-patched with 4bbf6edfb don't actually exercise the problem case before then (there's no top-level Sort in those plans). Discussion: https://postgr.es/m/8946.1544644803@sss.pgh.pa.us	7 years ago
Etsuro Fujita	f8f6e44676	postgres_fdw: Improve cost and size estimation for aggregate pushdown. ... In commit 7012b132d0, which added aggregate pushdown to postgres_fdw, we didn't account for the evaluation cost and the selectivity of HAVING quals attached to ForeignPaths performing aggregate pushdown, as core had never accounted for that for AggPaths and GroupPaths. And we didn't set these values of the locally-checked quals (ie, fpinfo's local_conds_cost and local_conds_sel), which were initialized to zeros, but since estimate_path_cost_size factors in these to estimate the result size and the evaluation cost of such a ForeignPath when the use_remote_estimate option is enabled, this caused it to produce underestimated results in that case. By commit 7b6c075471 core was changed so that it accounts for the evaluation cost and the selectivity of HAVING quals in aggregation paths, so change the postgres_fdw's aggregate pushdown code as well as such. This not only fixes the underestimation issue mentioned above, but improves the estimation using local statistics in that function when that option is disabled. This would be a bug fix rather than an improvement, but apply it to HEAD only to avoid destabilizing existing plan choices. Author: Etsuro Fujita Discussion: https://postgr.es/m/5BFD3EAD.2060301%40lab.ntt.co.jp	7 years ago
Andres Freund	578b229718	Remove WITH OIDS support, change oid catalog column visibility. ... Previously tables declared WITH OIDS, including a significant fraction of the catalog tables, stored the oid column not as a normal column, but as part of the tuple header. This special column was not shown by default, which was somewhat odd, as it's often (consider e.g. pg_class.oid) one of the more important parts of a row. Neither pg_dump nor COPY included the contents of the oid column by default. The fact that the oid column was not an ordinary column necessitated a significant amount of special case code to support oid columns. That already was painful for the existing, but upcoming work aiming to make table storage pluggable, would have required expanding and duplicating that "specialness" significantly. WITH OIDS has been deprecated since 2005 (commit ff02d0a05280e0). Remove it. Removing includes: - CREATE TABLE and ALTER TABLE syntax for declaring the table to be WITH OIDS has been removed (WITH (oids[ = true]) will error out) - pg_dump does not support dumping tables declared WITH OIDS and will issue a warning when dumping one (and ignore the oid column). - restoring an pg_dump archive with pg_restore will warn when restoring a table with oid contents (and ignore the oid column) - COPY will refuse to load binary dump that includes oids. - pg_upgrade will error out when encountering tables declared WITH OIDS, they have to be altered to remove the oid column first. - Functionality to access the oid of the last inserted row (like plpgsql's RESULT_OID, spi's SPI_lastoid, ...) has been removed. The syntax for declaring a table WITHOUT OIDS (or WITH (oids = false) for CREATE TABLE) is still supported. While that requires a bit of support code, it seems unnecessary to break applications / dumps that do not use oids, and are explicit about not using them. The biggest user of WITH OID columns was postgres' catalog. This commit changes all 'magic' oid columns to be columns that are normally declared and stored. To reduce unnecessary query breakage all the newly added columns are still named 'oid', even if a table's column naming scheme would indicate 'reloid' or such. This obviously requires adapting a lot code, mostly replacing oid access via HeapTupleGetOid() with access to the underlying Form_pg_*->oid column. The bootstrap process now assigns oids for all oid columns in genbki.pl that do not have an explicit value (starting at the largest oid previously used), only oids assigned later by oids will be above FirstBootstrapObjectId. As the oid column now is a normal column the special bootstrap syntax for oids has been removed. Oids are not automatically assigned during insertion anymore, all backend code explicitly assigns oids with GetNewOidWithIndex(). For the rare case that insertions into the catalog via SQL are called for the new pg_nextoid() function can be used (which only works on catalog tables). The fact that oid columns on system tables are now normal columns means that they will be included in the set of columns expanded by * (i.e. SELECT * FROM pg_class will now include the table's oid, previously it did not). It'd not technically be hard to hide oid column by default, but that'd mean confusing behavior would either have to be carried forward forever, or it'd cause breakage down the line. While it's not unlikely that further adjustments are needed, the scope/invasiveness of the patch makes it worthwhile to get merge this now. It's painful to maintain externally, too complicated to commit after the code code freeze, and a dependency of a number of other patches. Catversion bump, for obvious reasons. Author: Andres Freund, with contributions by John Naylor Discussion: https://postgr.es/m/20180930034810.ywp2c7awz7opzcfr@alap3.anarazel.de	7 years ago
Etsuro Fujita	7cfdc77023	Disable support for partitionwise joins in problematic cases. ... Commit f49842d, which added support for partitionwise joins, built the child's tlist by applying adjust_appendrel_attrs() to the parent's. So in the case where the parent's included a whole-row Var for the parent, the child's contained a ConvertRowtypeExpr. To cope with that, that commit added code to the planner, such as setrefs.c, but some code paths still assumed that the tlist for a scan (or join) rel would only include Vars and PlaceHolderVars, which was true before that commit, causing errors: * When creating an explicit sort node for an input path for a mergejoin path for a child join, prepare_sort_from_pathkeys() threw the 'could not find pathkey item to sort' error. * When deparsing a relation participating in a pushed down child join as a subquery in contrib/postgres_fdw, get_relation_column_alias_ids() threw the 'unexpected expression in subquery output' error. * When performing set_plan_references() on a local join plan generated by contrib/postgres_fdw for EvalPlanQual support for a pushed down child join, fix_join_expr() threw the 'variable not found in subplan target lists' error. To fix these, two approaches have been proposed: one by Ashutosh Bapat and one by me. While the former keeps building the child's tlist with a ConvertRowtypeExpr, the latter builds it with a whole-row Var for the child not to violate the planner assumption, and tries to fix it up later, But both approaches need more work, so refuse to generate partitionwise join paths when whole-row Vars are involved, instead. We don't need to handle ConvertRowtypeExprs in the child's tlists for now, so this commit also removes the changes to the planner. Previously, partitionwise join computed attr_needed data for each child separately, and built the child join's tlist using that data, which also required an extra step for adding PlaceHolderVars to that tlist, but it would be more efficient to build it from the parent join's tlist through the adjust_appendrel_attrs() transformation. So this commit builds that list that way, and simplifies build_joinrel_tlist() and placeholder.c as well as part of set_append_rel_size() to basically what they were before partitionwise join went in. Back-patch to PG11 where partitionwise join was introduced. Report by Rajkumar Raghuwanshi. Analysis by Ashutosh Bapat, who also provided some of regression tests. Patch by me, reviewed by Robert Haas. Discussion: https://postgr.es/m/CAKcux6ktu-8tefLWtQuuZBYFaZA83vUzuRd7c1YHC-yEWyYFpg@mail.gmail.com	7 years ago
Heikki Linnakangas	31380bc7c2	Spell "partitionwise" consistently. ... I'm not sure which spelling is better, "partitionwise" or "partition-wise", but everywhere else we spell it "partitionwise", so be consistent. Tatsuro Yamada reported the one in README, I found the other one with grep. Discussion: https://www.postgresql.org/message-id/d25ebf36-5a6d-8b2c-1ff3-d6f022a56000@lab.ntt.co.jp	7 years ago
Andres Freund	3522d0eaba	Deduplicate "invalid input syntax" messages for various types. ... Previously a lot of the error messages referenced the type in the error message itself. That requires that the message is translated separately for each type. Note that currently a few smallint cases continue to reference the integer, rather than smallint, type. A later patch will create a separate routine for 16bit input. Author: Andres Freund Discussion: https://postgr.es/m/20180707200158.wpqkd7rjr4jxq5g7@alap3.anarazel.de	7 years ago
Tom Lane	1007b0a126	Fix hashjoin costing mistake introduced with inner_unique optimization. ... In final_cost_hashjoin(), commit 9c7f5229a allowed inner_unique cases to follow a code path previously used only for SEMI/ANTI joins; but it neglected to fix an if-test within that path that assumed SEMI and ANTI were the only possible cases. This resulted in a wrong value for hashjointuples, and an ensuing bad cost estimate, for inner_unique normal joins. Fortunately, for inner_unique normal joins we can assume the number of joined tuples is the same as for a SEMI join; so there's no need for more code, we just have to invert the test to check for ANTI not SEMI. It turns out that in two contrib tests in which commit 9c7f5229a changed the plan expected for a query, the change was actually wrong and induced by this estimation error, not by any real improvement. Hence this patch also reverts those changes. Per report from RK Korlapati. Backpatch to v10 where the error was introduced. David Rowley Discussion: https://postgr.es/m/CA+SNy03bhq0fodsfOkeWDCreNjJVjsdHwUsb7AG=jpe0PtZc_g@mail.gmail.com	7 years ago
Jeff Davis	a45adc747e	Fix WITH CHECK OPTION on views referencing postgres_fdw tables. ... If a view references a foreign table, and the foreign table has a BEFORE INSERT trigger, then it's possible for a tuple inserted or updated through the view to be changed such that it violates the view's WITH CHECK OPTION constraint. Before this commit, postgres_fdw handled this case inconsistently. A RETURNING clause on the INSERT or UPDATE statement targeting the view would cause the finally-inserted tuple to be read back, and the WITH CHECK OPTION violation would throw an error. But without a RETURNING clause, postgres_fdw would not read the final tuple back, and WITH CHECK OPTION would not throw an error for the violation (or may throw an error when there is no real violation). AFTER ROW triggers on the foreign table had a similar effect as a RETURNING clause on the INSERT or UPDATE statement. To fix, this commit retrieves the attributes needed to enforce the WITH CHECK OPTION constraint along with the attributes needed for the RETURNING clause (if any) from the remote side. Thus, the WITH CHECK OPTION constraint is always evaluated against the final tuple after any triggers on the remote side. This fix may be considered inconsistent with CHECK constraints declared on foreign tables, which are not enforced locally at all (because the constraint is on a remote object). The discussion concluded that this difference is reasonable, because the WITH CHECK OPTION is a constraint on the local view (not any remote object); therefore it only makes sense to enforce its WITH CHECK OPTION constraint locally. Author: Etsuro Fujita Reviewed-by: Arthur Zakirov, Stephen Frost Discussion: https://www.postgresql.org/message-id/7eb58fab-fd3b-781b-ac33-f7cfec96021f%40lab.ntt.co.jp	7 years ago
Tom Lane	b86b7bfa3e	Improve English wording of some other getObjectDescription() messages. ... Print columns as "column C of <relation>" rather than "<relation> column C". This seems to read noticeably better in English, as evidenced by the regression test output changes, and the code change also makes it possible for translators to adjust the phrase order in other languages. Also change the output for OCLASS_DEFAULT from "default for %s" to "default value for %s". This seems to read better and is also more consistent with the output of, for instance, getObjectTypeDescription(). Kyotaro Horiguchi, per a complaint from me Discussion: https://postgr.es/m/20180522.182020.114074746.horiguchi.kyotaro@lab.ntt.co.jp	7 years ago
Robert Haas	7fc7dac1a7	Pass the correct PlannerInfo to PlanForeignModify/PlanDirectModify. ... Previously, we passed the toplevel PlannerInfo, but we actually want to pass the relevant subroot. One problem with passing the toplevel PlannerInfo is that the FDW which wants to push down an UPDATE or DELETE against a join won't find the relevant joinrel there. As of commit 1bc0100d27, postgres_fdw tries to do exactly this and can be made to fail an assertion as a result. It's possible that this should be regarded as a bug fix and back-patched to earlier releases, but for lack of a test case that fails in earlier releases, no back-patch for now. Etsuro Fujita, reviewed by Amit Langote. Discussion: http://postgr.es/m/5AF43E02.30000@lab.ntt.co.jp	7 years ago
Robert Haas	37a3058bc7	Fix interaction of foreign tuple routing with remote triggers. ... Without these fixes, changes to the inserted tuple made by remote triggers are ignored when building local RETURNING tuples. In the core code, call ExecInitRoutingInfo at a later point from within ExecInitPartitionInfo so that the FDW callback gets invoked after the returning list has been built. But move CheckValidResultRel out of ExecInitRoutingInfo so that it can happen at an earlier stage. In postgres_fdw, refactor assorted deparsing functions to work with the RTE rather than the PlannerInfo, which saves us having to construct a fake PlannerInfo in cases where we don't have a real one. Then, we can pass down a constructed RTE that yields the correct deparse result when no real one exists. Unfortunately, this necessitates a hack that understands how the core code manages RT indexes for update tuple routing, which is ugly, but we don't have a better idea right now. Original report, analysis, and patch by Etsuro Fujita. Heavily refactored by me. Then worked over some more by Amit Langote. Discussion: http://postgr.es/m/5AD4882B.10002@lab.ntt.co.jp	7 years ago
Tom Lane	2fe977712c	YA attempt to stabilize the results of the postgres_fdw regression test. ... We've made multiple attempts to stabilize the plans shown by commit 1bc0100d2, with little success so far. The reason for the remaining instability seems to be that if a transaction (such as auto-analyze) is running concurrently with the test, then get_actual_variable_range may return a maximum value for "T 1"."C 1" that's far away from the actual max, as a result of our having transiently inserted such a value earlier in the test. Because we use a non-MVCC snapshot to fetch the value (for performance reasons), the presence of other transactions can cause that function to return entries that are actually dead. To fix, use a less extreme value in the earlier transient insertion, so that whether it is visible or not won't affect the selectivity estimate. The use of 9999 there seems to have been picked with the aid of a dartboard anyway, rather than having a specific reason. Discussion: https://postgr.es/m/16962.1523551784@sss.pgh.pa.us	8 years ago
Robert Haas	3d956d9562	Allow insert and update tuple routing and COPY for foreign tables. ... Also enable this for postgres_fdw. Etsuro Fujita, based on an earlier patch by Amit Langote. The larger patch series of which this is a part has been reviewed by Amit Langote, David Fetter, Maksim Milyutin, Álvaro Herrera, Stephen Frost, and me. Minor documentation changes to the final version by me. Discussion: http://postgr.es/m/29906a26-da12-8c86-4fb9-d8f88442f2b9@lab.ntt.co.jp	8 years ago
Robert Haas	7e0d64c7a5	postgres_fdw: Push down partition-wise aggregation. ... Since commit 7012b132d0, postgres_fdw has been able to push down the toplevel aggregation operation to the remote server. Commit e2f1eb0ee3 made it possible to break down the toplevel aggregation into one aggregate per partition. This commit lets postgres_fdw push down aggregation in that case just as it does at the top level. In order to make this work, this commit adds an additional argument to the GetForeignUpperPaths FDW API. A matching argument is added to the signature for create_upper_paths_hook. Third-party code using either of these will need to be updated. Also adjust create_foreignscan_plan() so that it picks up the correct set of relids in this case. Jeevan Chalke, reviewed by Ashutosh Bapat and by me and with some adjustments by me. The larger patch series of which this patch is a part was also reviewed and tested by Antonin Houska, Rajkumar Raghuwanshi, David Rowley, Dilip Kumar, Konstantin Knizhnik, Pascal Legrand, and Rafia Sabih. Discussion: http://postgr.es/m/CAM2+6=V64_xhstVHie0Rz=KPEQnLJMZt_e314P0jaT_oJ9MR8A@mail.gmail.com Discussion: http://postgr.es/m/CAM2+6=XPWujjmj5zUaBTGDoB38CemwcPmjkRy0qOcsQj_V+2sQ@mail.gmail.com	8 years ago
Robert Haas	11cf92f6e2	Rewrite the code that applies scan/join targets to paths. ... If the toplevel scan/join target list is parallel-safe, postpone generating Gather (or Gather Merge) paths until after the toplevel has been adjusted to return it. This (correctly) makes queries with expensive functions in the target list more likely to choose a parallel plan, since the cost of the plan now reflects the fact that the evaluation will happen in the workers rather than the leader. The original complaint about this problem was from Jeff Janes. If the toplevel scan/join relation is partitioned, recursively apply the changes to all partitions. This sometimes allows us to get rid of Result nodes, because Append is not projection-capable but its children may be. It also cleans up what appears to be incorrect SRF handling from commit e2f1eb0ee30d144628ab523432320f174a2c8966: the old code had no knowledge of SRFs for child scan/join rels. Because we now use create_projection_path() in some cases where we formerly used apply_projection_to_path(), this changes the ordering of columns in some queries generated by postgres_fdw. Update regression outputs accordingly. Patch by me, reviewed by Amit Kapila and by Ashutosh Bapat. Other fixes for this problem (substantially different from this version) were reviewed by Dilip Kumar, Amit Khandekar, and Marina Polyakova. Discussion: http://postgr.es/m/CAMkU=1ycXNipvhWuweUVpKuyu6SpNjF=yHWu4c4US5JgVGxtZQ@mail.gmail.com	8 years ago
Tom Lane	feb8254518	Improve style guideline compliance of assorted error-report messages. ... Per the project style guide, details and hints should have leading capitalization and end with a period. On the other hand, errcontext should not be capitalized and should not end with a period. To support well formatted error contexts in dblink, extend dblink_res_error() to take a format+arguments rather than a hardcoded string. Daniel Gustafsson Discussion: https://postgr.es/m/B3C002C8-21A0-4F53-A06E-8CAB29FCF295@yesql.se	8 years ago
Tom Lane	04e7ecadf6	Revert "Temporarily instrument postgres_fdw test to look for statistics changes." ... This reverts commit c2c537c56d. It's now clear that whatever is going on there, it can't be blamed on unexpected ANALYZE runs, because the statistics are the same just before the failing query as they were at the start of the test.	8 years ago
Tom Lane	c2c537c56d	Temporarily instrument postgres_fdw test to look for statistics changes. ... It seems fairly hard to explain recent buildfarm failures without the theory that something is doing an ANALYZE behind our backs. Probe for this directly to see if it's true. In principle the outputs of these queries should be stable, since the table in question is small enough that ANALYZE's sample will include all rows. But even if that turns out to be wrong, we can put up with some failures for a bit. I don't intend to leave this here indefinitely. Discussion: https://postgr.es/m/25502.1520277552@sss.pgh.pa.us	8 years ago
Robert Haas	1733460f02	postgres_fdw: Fourth attempt to stabilize regression tests. ... Commit 1bc0100d27 added this test, and commits 882ea509fe, 958e20e42d, 4fa396464e tried to stabilize it. It's still not stable, so keep trying. The latest comment from Tom Lane is that disabling autovacuum seems like a good strategy, but we might need to do it on more tables, hence this patch. Etsuro Fujita Discussion: http://postgr.es/m/5A9928F1.2010206@lab.ntt.co.jp	8 years ago
Robert Haas	4fa396464e	postgres_fdw: Third attempt to stabilize regression tests. ... Commit 1bc0100d27 added this test, and commit 882ea509fe tried to stabilize it. There were still failures, so commit 958e20e42d tried again to stabilize it. That approach is still failing on jaguarundi, though, so back it out and try something else. Specifically, instead of disabling remote estimates for the table in question, let's tell autovacuum to leave it alone. Etsuro Fujita Discussion: http://postgr.es/m/5A82DCCE.3060107@lab.ntt.co.jp	8 years ago
Robert Haas	84cb51b4e2	postgres_fdw: Fix interaction of PHVs with child joins. ... Commit f49842d1ee introduced the concept of a child join, but did not update this code accordingly. Ashutosh Bapat, with cosmetic changes by me Discussion: http://postgr.es/m/CAFjFpRf=J_KPOtw+bhZeURYkbizr8ufSaXg6gPEF6DKpgH-t6g@mail.gmail.com	8 years ago
Peter Eisentraut	2fb1abaeb0	Rename enable_partition_wise_join to enable_partitionwise_join ... Discussion: https://www.postgresql.org/message-id/flat/ad24e4f4-6481-066e-e3fb-6ef4a3121882%402ndquadrant.com	8 years ago
Robert Haas	958e20e42d	postgres_fdw: Attmempt to stabilize regression tests. ... Even after commit 882ea509fe, some buildfarm members are still failing in the postgres_fdw tests. Try to fix that by disabling use of remote statistics for some test cases. Etsuro Fujita Discussion: http://postgr.es/m/5A7D76CF.8080601@lab.ntt.co.jp	8 years ago
Robert Haas	882ea509fe	postgres_fdw: Remove CTID output from some tests. ... Commit 1bc0100d27 added these tests, but they're not stable enough to survive in the buildfarm. Remove CTIDs from the output in the hopes of fixing that.	8 years ago
Robert Haas	1bc0100d27	postgres_fdw: Push down UPDATE/DELETE joins to remote servers. ... Commit 0bf3ae88af allowed direct foreign table modification; instead of fetching each row, updating it locally, and then pushing the modification back to the remote side, we would instead do all the work on the remote server via a single remote UPDATE or DELETE command. However, that commit only enabled this optimization when join tree consisted only of the target table. This change allows the same optimization when an UPDATE statement has a FROM clause or a DELETE statement has a USING clause. This works much like ordinary foreign join pushdown, in that the tables must be on the same remote server, relevant parts of the query must be pushdown-safe, and so forth. Etsuro Fujita, reviewed by Ashutosh Bapat, Rushabh Lathia, and me. Some formatting corrections by me. Discussion: http://postgr.es/m/5A57193A.2080003@lab.ntt.co.jp Discussion: http://postgr.es/m/b9cee735-62f8-6c07-7528-6364ce9347d0@lab.ntt.co.jp	8 years ago
Robert Haas	99f6a17dd6	Fix test case for 'outer pathkeys do not match mergeclauses' fix. ... Commit 4bbf6edfbd added a test case, but it turns out that the test case doesn't reliably test for the bug, and in the context of the regression test suite did not because ANALYZE had not been run. Report and patch by Etsuro Fujita. I added a comment along lines previously suggested by Tom Lane. Discussion: http://postgr.es/m/5A6195D8.8060206@lab.ntt.co.jp	8 years ago
Robert Haas	4bbf6edfbd	postgres_fdw: Avoid 'outer pathkeys do not match mergeclauses' error. ... When pushing down a join to a foreign server, postgres_fdw constructs an alternative plan to be used for any EvalPlanQual rechecks that prove to be necessary. This plan is stored as the outer subplan of the Foreign Scan implementing the pushed-down join. Previously, this alternative plan could have a different nominal sort ordering than its parent, which seemed OK since there will only be one tuple per base table anyway in the case of an EvalPlanQual recheck. Actually, though, it caused a problem if that path was used as a building block for the EvalPlanQual recheck plan of a higher-level foreign join, because we could end up with a merge join one of whose inputs was not labelled with the correct sort order. Repair by injecting an extra Sort node into the EvalPlanQual recheck plan whenever it would otherwise fail to be sorted at least as well as its parent Foreign Scan. Report by Jeff Janes. Patch by me, reviewed by Tom Lane, who also provided the test case and comment text. Discussion: http://postgr.es/m/CAMkU=1y2G8VOVBHv3iXU2TMAj7-RyBFFW1uhkr5sm9LQ2=X35g@mail.gmail.com	8 years ago
Tom Lane	e9f2703ab7	Fix postgres_fdw to cope with duplicate GROUP BY entries. ... Commit 7012b132d, which added the ability to push down aggregates and grouping to the remote server, wasn't careful to ensure that the remote server would have the same idea we do about which columns are the grouping columns, in cases where there are textually identical GROUP BY expressions. Such cases typically led to "targetlist item has multiple sortgroupref labels" errors. To fix this reliably, switch over to using "GROUP BY column-number" syntax rather than "GROUP BY expression" in transmitted queries, and adjust foreign_grouping_ok() to be more careful about duplicating the sortgroupref labeling of the local pathtarget. Per bug #14890 from Sean Johnston. Back-patch to v10 where the buggy code was introduced. Jeevan Chalke, reviewed by Ashutosh Bapat Discussion: https://postgr.es/m/20171107134948.1508.94783@wrigleys.postgresql.org	8 years ago
Robert Haas	82c5c533d1	postgres_fdw: Fix failing regression test. ... Commit ab3f008a2d broke this. Report by Stephen Frost. Discussion: http://postgr.es/m/20171205180342.GO4628@tamriel.snowman.net	8 years ago
Robert Haas	9502227805	postgres_fdw: Fix test that didn't test what it claimed. ... Antonin Houska reported that the planner does consider pushing postgres_fdw_abs() to the remote side, which happens because we make it shippable earlier in the test case file. Jeevan Chalke provided this patch, which changes the join condition to use random(), which is not shippable, instead. Antonin reviewed the patch. Discussion: http://postgr.es/m/15265.1511985971@localhost	8 years ago
Tom Lane	9a785ad573	Fix creation of resjunk tlist entries for inherited mixed UPDATE/DELETE. ... rewriteTargetListUD's processing is dependent on the relkind of the query's target table. That was fine at the time it was made to act that way, even for queries on inheritance trees, because all tables in an inheritance tree would necessarily be plain tables. However, the 9.5 feature addition allowing some members of an inheritance tree to be foreign tables broke the assumption that rewriteTargetListUD's output tlist could be applied to all child tables with nothing more than column-number mapping. This led to visible failures if foreign child tables had row-level triggers, and would also break in cases where child tables belonged to FDWs that used methods other than CTID for row identification. To fix, delay running rewriteTargetListUD until after the planner has expanded inheritance, so that it is applied separately to the (already mapped) tlist for each child table. We can conveniently call it from preprocess_targetlist. Refactor associated code slightly to avoid the need to heap_open the target relation multiple times during preprocess_targetlist. (The APIs remain a bit ugly, particularly around the point of which steps scribble on parse->targetList and which don't. But avoiding such scribbling would require a change in FDW callback APIs, which is more pain than it's worth.) Also fix ExecModifyTable to ensure that "tupleid" is reset to NULL when we transition from rows providing a CTID to rows that don't. (That's really an independent bug, but it manifests in much the same cases.) Add a regression test checking one manifestation of this problem, which was that row-level triggers on a foreign child table did not work right. Back-patch to 9.5 where the problem was introduced. Etsuro Fujita, reviewed by Ildus Kurbangaliev and Ashutosh Bapat Discussion: https://postgr.es/m/20170514150525.0346ba72@postgrespro.ru	8 years ago
Robert Haas	f49842d1ee	Basic partition-wise join functionality. ... Instead of joining two partitioned tables in their entirety we can, if it is an equi-join on the partition keys, join the matching partitions individually. This involves teaching the planner about "other join" rels, which are related to regular join rels in the same way that other member rels are related to baserels. This can use significantly more CPU time and memory than regular join planning, because there may now be a set of "other" rels not only for every base relation but also for every join relation. In most practical cases, this probably shouldn't be a problem, because (1) it's probably unusual to join many tables each with many partitions using the partition keys for all joins and (2) if you do that scenario then you probably have a big enough machine to handle the increased memory cost of planning and (3) the resulting plan is highly likely to be better, so what you spend in planning you'll make up on the execution side. All the same, for now, turn this feature off by default. Currently, we can only perform joins between two tables whose partitioning schemes are absolutely identical. It would be nice to cope with other scenarios, such as extra partitions on one side or the other with no match on the other side, but that will have to wait for a future patch. Ashutosh Bapat, reviewed and tested by Rajkumar Raghuwanshi, Amit Langote, Rafia Sabih, Thomas Munro, Dilip Kumar, Antonin Houska, Amit Khandekar, and by me. A few final adjustments by me. Discussion: http://postgr.es/m/CAFjFpRfQ8GrQvzp3jA2wnLqrHmaXna-urjm_UY9BqXj=EaDTSA@mail.gmail.com Discussion: http://postgr.es/m/CAFjFpRcitjfrULr5jfuKWRPsGUX0LQ0k8-yG0Qw2+1LBGNpMdw@mail.gmail.com	8 years ago
Robert Haas	7086be6e36	When WCOs are present, disable direct foreign table modification. ... If the user modifies a view that has CHECK OPTIONs and this gets translated into a modification to an underlying relation which happens to be a foreign table, the check options should be enforced. In the normal code path, that was happening properly, but it was not working properly for "direct" modification because the whole operation gets pushed to the remote side in that case and we never have an option to enforce the constraint against individual tuples. Fix by disabling direct modification when there is a need to enforce CHECK OPTIONs. Etsuro Fujita, reviewed by Kyotaro Horiguchi and by me. Discussion: http://postgr.es/m/f8a48f54-6f02-9c8a-5250-9791603171ee@lab.ntt.co.jp	8 years ago
Tom Lane	88f48b57fd	Stabilize postgres_fdw regression tests. ... The new test cases added in commit 8bf58c0d9 turn out to have output that can vary depending on the lc_messages setting prevailing on the test server. Hide the remote end's error messages to ensure stable output. This isn't a terribly desirable solution; we'd rather know that the connection failed for the expected reason and not some other one. But there seems little choice for the moment. Per buildfarm. Discussion: https://postgr.es/m/18419.1500658570@sss.pgh.pa.us	8 years ago