284 Commits (585e31fcb6dfcb1d88cfee2371f565574db24869)

Author	SHA1	Message	Date
Michael Paquier	ff8ca5fadd	Add connection parameters to control SSL protocol min/max in libpq ... These two new parameters, named sslminprotocolversion and sslmaxprotocolversion, allow to respectively control the minimum and the maximum version of the SSL protocol used for the SSL connection attempt. The default setting is to allow any version for both the minimum and the maximum bounds, causing libpq to rely on the bounds set by the backend when negotiating the protocol to use for an SSL connection. The bounds are checked when the values are set at the earliest stage possible as this makes the checks independent of any SSL implementation. Author: Daniel Gustafsson Reviewed-by: Michael Paquier, Cary Huang Discussion: https://postgr.es/m/4F246AE3-A7AE-471E-BD3D-C799D3748E03@yesql.se	6 years ago
Tom Lane	215824f918	In postgres_fdw, don't try to ship MULTIEXPR updates to remote server. ... In a statement like "UPDATE remote_tab SET (x,y) = (SELECT ...)", we'd conclude that the statement could be directly executed remotely, because the sub-SELECT is in a resjunk tlist item that's not examined for shippability. Currently that ends up crashing if the sub-SELECT contains any remote Vars. Prevent the crash by deeming MULTIEXEC Params to be unshippable. This is a bit of a brute-force solution, since if the sub-SELECT *doesn't* contain any remote Vars, the current execution technology would work; but that's not a terribly common use-case for this syntax, I think. In any case, we generally don't try to ship sub-SELECTs, so it won't surprise anybody that this doesn't end up as a remote direct update. I'd be inclined to see if that general limitation can be fixed before worrying about this case further. Per report from Lukáš Sobotka. Back-patch to 9.6. 9.5 had MULTIEXPR, but we didn't try to perform remote direct updates then, so the case didn't arise anyway. Discussion: https://postgr.es/m/CAJif3k+iA_ekBB5Zw2hDBaE1wtiQa4LH4_JUXrrMGwTrH0J01Q@mail.gmail.com	6 years ago
Andrew Dunstan	cebf9d6e6e	Only superuser can set sslcert/sslkey in postgres_fdw user mappings ... Othrwise there is a security risk. Discussion: https://postgr.es/m/20200109103014.GA4192@msg.df7cb.de	6 years ago
Andrew Dunstan	f5fd995a1a	Allow 'sslkey' and 'sslcert' in postgres_fdw user mappings ... This allows different users to authenticate with different certificates. Author: Craig Ringer	6 years ago
Tom Lane	0af0504da9	Adjust test case added by commit 6136e94dc. ... Per project policy, transient roles created by regression test cases should be named "regress_something", to reduce the risks of running such cases against installed servers. And no such role should ever be left behind after running a test. Discussion: https://postgr.es/m/11297.1576868677@sss.pgh.pa.us	6 years ago
Tom Lane	e60b480d39	libpq should expose GSS-related parameters even when not implemented. ... We realized years ago that it's better for libpq to accept all connection parameters syntactically, even if some are ignored or restricted due to lack of the feature in a particular build. However, that lesson from the SSL support was for some reason never applied to the GSSAPI support. This is causing various buildfarm members to have problems with a test case added by commit 6136e94dc, and it's just a bad idea from a user-experience standpoint anyway, so fix it. While at it, fix some places where parameter-related infrastructure was added with the aid of a dartboard, or perhaps with the aid of the anti-pattern "add new stuff at the end". It should be safe to rearrange the contents of struct pg_conn even in released branches, since that's private to libpq (and we'd have to move some fields in some builds to fix this, anyway). Back-patch to all supported branches. Discussion: https://postgr.es/m/11297.1576868677@sss.pgh.pa.us	6 years ago
Andrew Dunstan	6136e94dcb	Superuser can permit passwordless connections on postgres_fdw ... Currently postgres_fdw doesn't permit a non-superuser to connect to a foreign server without specifying a password, or to use an authentication mechanism that doesn't use the password. This is to avoid using the settings and identity of the user running Postgres. However, this doesn't make sense for all authentication methods. We therefore allow a superuser to set "password_required 'false'" for user mappings for the postgres_fdw. The superuser must ensure that the foreign server won't try to rely solely on the server identity (e.g. trust, peer, ident) or use an authentication mechanism that relies on the password settings (e.g. md5, scram-sha-256). This feature is a prelude to better support for sslcert and sslkey settings in user mappings. Author: Craig Ringer. Discussion: https://postgr.es/m/075135da-545c-f958-fed0-5dcb462d6dae@2ndQuadrant.com	6 years ago
Tom Lane	6ef77cf46e	Further adjust EXPLAIN's choices of table alias names. ... This patch causes EXPLAIN to always assign a separate table alias to the parent RTE of an append relation (inheritance set); before, such RTEs were ignored if not actually scanned by the plan. Since the child RTEs now always have that same alias to start with (cf. commit 55a1954da), the net effect is that the parent RTE usually gets the alias used or implied by the query text, and the children all get that alias with "_N" appended. (The exception to "usually" is if there are duplicate aliases in different subtrees of the original query; then some of those original RTEs will also have "_N" appended.) This results in more uniform output for partitioned-table plans than we had before: the partitioned table itself gets the original alias, and all child tables have aliases with "_N", rather than the previous behavior where one of the children would get an alias without "_N". The reason for giving the parent RTE an alias, even if it isn't scanned by the plan, is that we now use the parent's alias to qualify Vars that refer to an appendrel output column and appear above the Append or MergeAppend that computes the appendrel. But below the append, Vars refer to some one of the child relations, and are displayed that way. This seems clearer than the old behavior where a Var that could carry values from any child relation was displayed as if it referred to only one of them. While at it, change ruleutils.c so that the code paths used by EXPLAIN deal in Plan trees not PlanState trees. This effectively reverts a decision made in commit 1cc29fe7c, which seemed like a good idea at the time to make ruleutils.c consistent with explain.c. However, it's problematic because we'd really like to allow executor startup pruning to remove all the children of an append node when possible, leaving no child PlanState to resolve Vars against. (That's not done here, but will be in the next patch.) This requires different handling of subplans and initplans than before, but is otherwise a pretty straightforward change. Discussion: https://postgr.es/m/001001d4f44b$2a2cca50$7e865ef0$@lab.ntt.co.jp	6 years ago
Etsuro Fujita	5a20b0219e	Fix handling of multiple AFTER ROW triggers on a foreign table. ... AfterTriggerExecute() retrieves a fresh tuple or pair of tuples from a tuplestore and then stores the tuple(s) in the passed-in slot(s) if AFTER_TRIGGER_FDW_FETCH, while it uses the most-recently-retrieved tuple(s) stored in the slot(s) if AFTER_TRIGGER_FDW_REUSE. This was done correctly before 12, but commit ff11e7f4b broke it by mistakenly clearing the tuple(s) stored in the slot(s) in that function, leading to an assertion failure as reported in bug #16139 from Alexander Lakhin. Also, fix some other issues with the aforementioned commit in passing: * For tg_newslot, which is a slot added to the TriggerData struct by the commit to store new updated tuples, it didn't ensure the slot was NULL if there was no such tuple. * The commit failed to update the documentation about the trigger interface. Author: Etsuro Fujita Backpatch-through: 12 Discussion: https://postgr.es/m/16139-94f9ccf0db6119ec%40postgresql.org	6 years ago
Tom Lane	bf39b3af6a	Further sync postgres_fdw's "Relations" output with the rest of EXPLAIN. ... EXPLAIN generally only adds schema qualifications to table names when VERBOSE is specified. In postgres_fdw's "Relations" output, table names were always so qualified, but that was an implementation restriction: in the original coding, we didn't have access to the verbose flag at the time the string was generated. After the code rearrangement of commit 4526951d5, we do have that info available at the right time, so make this output follow the normal rule. Discussion: https://postgr.es/m/12424.1575168015@sss.pgh.pa.us	6 years ago
Tom Lane	55a1954da1	Fix EXPLAIN's column alias output for mismatched child tables. ... If an inheritance/partitioning parent table is assigned some column alias names in the query, EXPLAIN mapped those aliases onto the child tables' columns by physical position, resulting in bogus output if a child table's columns aren't one-for-one with the parent's. To fix, make expand_single_inheritance_child() generate a correctly re-mapped column alias list, rather than just copying the parent RTE's alias node. (We have to fill the alias field, not just adjust the eref field, because ruleutils.c will ignore eref in favor of looking at the real column names.) This means that child tables will now always have alias fields in plan rtables, where before they might not have. That results in a rather substantial set of regression test output changes: EXPLAIN will now always show child tables with aliases that match the parent table (usually with "_N" appended for uniqueness). But that seems like a net positive for understandability, since the parent alias corresponds to something that actually appeared in the original query, while the child table names didn't. (Note that this does not change anything for cases where an explicit table alias was written in the query for the parent table; it just makes cases without such aliases behave similarly to that.) Hence, while we could avoid these subsidiary changes if we made inherit.c more complicated, we choose not to. Discussion: https://postgr.es/m/12424.1575168015@sss.pgh.pa.us	6 years ago
Tom Lane	4526951d56	Make postgres_fdw's "Relations" output agree with the rest of EXPLAIN. ... The relation aliases shown in the "Relations" line for a foreign scan didn't always agree with those used in the rest of EXPLAIN's output. The regression test result changes appearing here provide examples. It's really impossible for postgres_fdw to duplicate EXPLAIN's alias assignment logic during postgresGetForeignRelSize(), because of the de-duplication that EXPLAIN does on a global basis --- and anyway, trying to duplicate that would be unmaintainable. Instead, just put numeric rangetable indexes into the string, and convert those to table names/aliases in postgresExplainForeignScan, which does have access to the results of ruleutils.c's alias assignment logic. Aside from being more reliable, this shifts some work from planning to EXPLAIN, which is a good tradeoff for performance. (I also changed from using StringInfo to using psprintf, which makes the code slightly simpler and reduces its memory consumption.) A kluge required by this solution is that we have to reverse-engineer the rtoffset applied by setrefs.c. If that logic ever fails (presumably because the member tables of a join got offset by different amounts), we'll need some more cooperation with setrefs.c to keep things straight. But for now, there's no need for that. Arguably this is a back-patchable bug fix, but since this is a mostly cosmetic issue and there have been no field complaints, I'll refrain for now. Discussion: https://postgr.es/m/12424.1575168015@sss.pgh.pa.us	6 years ago
Michael Paquier	94fec48516	Add regression test for two-phase transaction in postgres_fdw ... postgres_fdw does not support two-phase transactions, so let's add a small negative test case to check after it. Note that this is checked using an end-of-xact callback to ensure a proper connection cleanup with the foreign server, which is called before checking if a server is able to handle 2PC with max_prepared_xacts, so this test does not need an alternate output file. Author: Gilles Darold Discussion: https://postgr.es/m/20191108090507.GC1768@paquier.xyz	6 years ago
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