summaryrefslogtreecommitdiff
path: root/src/backend/optimizer
Commit message (Collapse)AuthorAgeFilesLines
* Track tlist_vinfo.varnullingrels even in non-Assert builds.Tom Lane2023-05-171-6/+0
| | | | | Oversight in commit 867be9c07 (which should get reverted along with that, if we ever do revert it). Per buildfarm.
* Fix some issues with improper placement of outer join clauses.Tom Lane2023-05-179-79/+244
| | | | | | | | | | | | | | | | | | | | | | | | | | After applying outer-join identity 3 in the forward direction, it was possible for the planner to mistakenly apply a qual clause from above the two outer joins at the now-lower join level. This can give the wrong answer, since a value that would get nulled by the now-upper join might not yet be null. To fix, when we perform such a transformation, consider that the now-lower join hasn't really completed the outer join it's nominally responsible for and thus its relid set should not include that OJ's relid (nor should its output Vars have that nullingrel bit set). Instead we add those bits when the now-upper join is performed. The existing rules for qual placement then suffice to prevent higher qual clauses from dropping below the now-upper join. There are a few complications from needing to consider transitive closures in case multiple pushdowns have happened, but all in all it's not a very complex patch. This is all new logic (from 2489d76c4) so no need to back-patch. The added test cases all have the same results as in v15. Tom Lane and Richard Guo Discussion: https://postgr.es/m/0b819232-4b50-f245-1c7d-c8c61bf41827@postgrespro.ru
* Convert nullingrels match checks from Asserts to test-and-elog.Tom Lane2023-05-171-13/+21
| | | | | | | | | | | | It seems like the code that these checks are backstopping may have a few bugs left in it. Use a test-and-elog so that the tests are performed even in non-assert builds, and so that we get something more informative than "server closed the connection" on failure. Committed separately with the idea that eventually we'll revert this. It might be awhile though. Discussion: https://postgr.es/m/3014965.1684293045@sss.pgh.pa.us
* Add back SQLValueFunction for SQL keywordsMichael Paquier2023-05-172-7/+33
| | | | | | | | | | | | | | | | | | | | | | | | This is equivalent to a revert of f193883 and fb32748, with the addition that the declaration of the SQLValueFunction node needs to gain a couple of node_attr for query jumbling. The performance impact of removing the function call inlining is proving to be too huge for some workloads where these are used. A worst-case test case of involving only simple SELECT queries with a SQL keyword is proving to lead to a reduction of 10% in TPS via pgbench and prepared queries on a high-end machine. None of the tests I ran back for this set of changes saw such a huge gap, but Alexander Lakhin and Andres Freund have found that this can be noticeable. Keeping the older performance would mean to do more inlining in the executor when using COERCE_SQL_SYNTAX for a function expression, similarly to what SQLValueFunction does. This requires more redesign work and there is little time until 16beta1 is released, so for now reverting the change is the best way forward, bringing back the previous performance. Bump catalog version. Reported-by: Alexander Lakhin Discussion: https://postgr.es/m/b32bed1b-0746-9b20-1472-4bdc9ca66d52@gmail.com
* Undo faulty attempt at not relying on RINFO_IS_PUSHED_DOWN.Tom Lane2023-05-111-6/+1
| | | | | | | | | | | | | | I've had a bee in my bonnet for some time about getting rid of RestrictInfo.is_pushed_down, because it's squishily defined and requires not-inexpensive extra tests to use (cf RINFO_IS_PUSHED_DOWN). In commit 2489d76c4, I tried to make remove_rel_from_query() not depend on that macro; but the replacement test is buggy, as exposed by a report from Rushabh Lathia and Robert Haas. That change was pretty incidental to the main goal of 2489d76c4, so let's just revert it for now. (Getting rid of is_pushed_down is still far away, anyway.) Discussion: https://postgr.es/m/CA+TgmoYco=hmg+iX1CW9Y1_CzNoSL81J03wUG-d2_3=rue+L2A@mail.gmail.com
* Handle RLS dependencies in inlined set-returning functions properly.Tom Lane2023-05-081-0/+7
| | | | | | | | | | | | | | | If an SRF in the FROM clause references a table having row-level security policies, and we inline that SRF into the calling query, we neglected to mark the plan as potentially dependent on which role is executing it. This could lead to later executions in the same session returning or hiding rows that should have been hidden or returned instead. Our thanks to Wolfgang Walther for reporting this problem. Stephen Frost and Tom Lane Security: CVE-2023-2455
* Revert "Move PartitionPruneInfo out of plan nodes into PlannedStmt"Alvaro Herrera2023-05-043-72/+41
| | | | | | | | | | | | | This reverts commit ec386948948c and its fixup 589bb816499e. This change was intended to support query planning avoiding acquisition of locks on partitions that were going to be pruned; however, the overall project took a different direction at [1] and this bit is no longer needed. Put things back the way they were as agreed in [2], to avoid unnecessary complexity. Discussion: [1] https://postgr.es/m/4191508.1674157166@sss.pgh.pa.us Discussion: [2] https://postgr.es/m/20230502175409.kcoirxczpdha26wt@alvherre.pgsql
* Fix typos in commentsMichael Paquier2023-05-021-1/+1
| | | | | | | | | The changes done in this commit impact comments with no direct user-visible changes, with fixes for incorrect function, variable or structure names. Author: Alexander Lakhin Discussion: https://postgr.es/m/e8c38840-596a-83d6-bd8d-cebc51111572@gmail.com
* Fix various typos and incorrect/outdated name referencesDavid Rowley2023-04-191-1/+1
| | | | | Author: Alexander Lakhin Discussion: https://postgr.es/m/699beab4-a6ca-92c9-f152-f559caf6dc25@gmail.com
* Fix some typos and some incorrectly duplicated wordsDavid Rowley2023-04-182-2/+2
| | | | | | Author: Justin Pryzby Reviewed-by: David Rowley Discussion: https://postgr.es/m/ZD3D1QxoccnN8A1V@telsasoft.com
* Revert "Catalog NOT NULL constraints" and falloutAlvaro Herrera2023-04-121-2/+0
| | | | | | | | | | | This reverts commit e056c557aef4 and minor later fixes thereof. There's a few problems in this new feature -- most notably regarding pg_upgrade behavior, but others as well. This new feature is not in any way critical on its own, so instead of scrambling to fix it we revert it and try again in early 17 with these issues in mind. Discussion: https://postgr.es/m/3801207.1681057430@sss.pgh.pa.us
* Fix parallel-safety marking when moving initplans to another node.Tom Lane2023-04-124-10/+22
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | Our policy since commit ab77a5a45 has been that a plan node having any initplans is automatically not parallel-safe. (This could be relaxed, but not today.) clean_up_removed_plan_level neglected this, and could attach initplans to a parallel-safe child plan node without clearing the plan's parallel-safe flag. That could lead to "subplan was not initialized" errors at runtime, in case an initplan referenced another one and only the referencing one got transmitted to parallel workers. The fix in clean_up_removed_plan_level is trivial enough. materialize_finished_plan also moves initplans from one node to another, but it's okay because it already copies the source node's parallel_safe flag. The other place that does this kind of thing is standard_planner's hack to inject a top-level Gather when debug_parallel_query is active. But that's actually dead code given that we're correctly enforcing the "initplans aren't parallel safe" rule, so just replace it with an Assert that there are no initplans. Also improve some related comments. Normally we'd add a regression test case for this sort of bug. The mistake itself is already reached by existing tests, but there is accidentally no visible problem. The only known test case that creates an actual failure seems too indirect and fragile to justify keeping it as a regression test (not least because it fails to fail in v11, though the bug is clearly present there too). Per report from Justin Pryzby. Back-patch to all supported branches. Discussion: https://postgr.es/m/ZDVt6MaNWkRDO1LQ@telsasoft.com
* Catalog NOT NULL constraintsAlvaro Herrera2023-04-071-0/+2
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | We now create pg_constaint rows for NOT NULL constraints with contype='n'. We propagate these constraints during operations such as adding inheritance relationships, creating and attaching partitions, creating tables LIKE other tables. We mostly follow the well-known rules of conislocal and coninhcount that we have for CHECK constraints, with some adaptations; for example, as opposed to CHECK constraints, we don't match NOT NULL ones by name when descending a hierarchy to alter it; instead we match by column number. This means we don't require the constraint names to be identical across a hierarchy. For now, we omit them from system catalogs. Maybe this is worth reconsidering. We don't support NOT VALID nor DEFERRABLE clauses either; these can be added as separate features later (this patch is already large and complicated enough.) This has been very long in the making. The first patch was written by Bernd Helmle in 2010 to add a new pg_constraint.contype value ('n'), which I (Álvaro) then hijacked in 2011 and 2012, until that one was killed by the realization that we ought to use contype='c' instead: manufactured CHECK constraints. However, later SQL standard development, as well as nonobvious emergent properties of that design (mostly, failure to distinguish them from "normal" CHECK constraints as well as the performance implication of having to test the CHECK expression) led us to reconsider this choice, so now the current implementation uses contype='n' again. In 2016 Vitaly Burovoy also worked on this feature[1] but found no consensus for his proposed approach, which was claimed to be closer to the letter of the standard, requiring additional pg_attribute columns to track the OID of the NOT NULL constraint for that column. [1] https://postgr.es/m/CAKOSWNkN6HSyatuys8xZxzRCR-KL1OkHS5-b9qd9bf1Rad3PLA@mail.gmail.com Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Author: Bernd Helmle <mailings@oopsware.de> Reviewed-by: Justin Pryzby <pryzby@telsasoft.com> Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com> Discussion: https://postgr.es/m/CACA0E642A0267EDA387AF2B%40%5B172.26.14.62%5D Discussion: https://postgr.es/m/AANLkTinLXMOEMz+0J29tf1POokKi4XDkWJ6-DDR9BKgU@mail.gmail.com Discussion: https://postgr.es/m/20110707213401.GA27098@alvh.no-ip.org Discussion: https://postgr.es/m/1343682669-sup-2532@alvh.no-ip.org Discussion: https://postgr.es/m/CAKOSWNkN6HSyatuys8xZxzRCR-KL1OkHS5-b9qd9bf1Rad3PLA@mail.gmail.com Discussion: https://postgr.es/m/20220817181249.q7qvj3okywctra3c@alvherre.pgsql
* Support "Right Anti Join" plan shapes.Tom Lane2023-04-055-32/+47
| | | | | | | | | | | | | Merge and hash joins can support antijoin with the non-nullable input on the right, using very simple combinations of their existing logic for right join and anti join. This gives the planner more freedom about how to order the join. It's particularly useful for hash join, since we may now have the option to hash the smaller table instead of the larger. Richard Guo, reviewed by Ronan Dunklau and myself Discussion: https://postgr.es/m/CAMbWs48xh9hMzXzSy3VaPzGAz+fkxXXTUbCLohX1_L8THFRm2Q@mail.gmail.com
* Remove comment obsoleted by 11c2d6fd.Thomas Munro2023-04-051-3/+2
| | | | | Reported-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/1604497.1680637072%40sss.pgh.pa.us
* Pass down table relation into more index relation functionsAndres Freund2023-04-011-1/+1
| | | | | | | | | | | | This is done in preparation for logical decoding on standby, which needs to include whether visibility affecting WAL records are about a (user) catalog table. Which is only known for the table, not the indexes. It's also nice to be able to pass the heap relation to GlobalVisTestFor() in vacuumRedirectAndPlaceholder(). Author: "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.com> Discussion: https://postgr.es/m/21b700c3-eecf-2e05-a699-f8c78dd31ec7@gmail.com
* Parallel Hash Full Join.Thomas Munro2023-03-311-8/+6
| | | | | | | | | | | | | | | | | | | | | | | | | | Full and right outer joins were not supported in the initial implementation of Parallel Hash Join because of deadlock hazards (see discussion). Therefore FULL JOIN inhibited parallelism, as the other join strategies can't do that in parallel either. Add a new PHJ phase PHJ_BATCH_SCAN that scans for unmatched tuples on the inner side of one batch's hash table. For now, sidestep the deadlock problem by terminating parallelism there. The last process to arrive at that phase emits the unmatched tuples, while others detach and are free to go and work on other batches, if there are any, but otherwise they finish the join early. That unfairness is considered acceptable for now, because it's better than no parallelism at all. The build and probe phases are run in parallel, and the new scan-for-unmatched phase, while serial, is usually applied to the smaller of the two relations and is either limited by some multiple of work_mem, or it's too big and is partitioned into batches and then the situation is improved by batch-level parallelism. Author: Melanie Plageman <melanieplageman@gmail.com> Author: Thomas Munro <thomas.munro@gmail.com> Reviewed-by: Thomas Munro <thomas.munro@gmail.com> Discussion: https://postgr.es/m/CA%2BhUKG%2BA6ftXPz4oe92%2Bx8Er%2BxpGZqto70-Q_ERwRaSyA%3DafNg%40mail.gmail.com
* Fix setrefs.c code for adjusting partPruneInfosAlvaro Herrera2023-03-301-31/+52
| | | | | | | | | | | | | | | | | | | | | We were transferring partPruneInfos from PlannerInfo into PlannerGlobal wrong, essentially relying on all of them being transferred, and adjusting their list indexes based on that. But apparently it's possible that some of them are skipped, so that strategy leads to a corrupted execution tree. Instead, adjust each Append/MergeAppend's partpruneinfo index as we copy from one list to the other, which seems safer anyway. This requires adjusting the RT offset of the RTE referenced in each partPruneInfo ahead of actually adjusting the RTE itself, which seems a bit too ad-hoc. This problem was introduced by commit ec386948948c. However, it may be that we no longer require the change introduced there, so perhaps we should revert both the present commit and that one. Problem noticed by sqlsmith. Author: Amit Langote <amitlangote09@gmail.com Discussion: https://postgr.es/m/CA+HiwqG6tbc2oadsbyyy24b2AL295XHQgyLRWghmA7u_SL1K8A@mail.gmail.com
* SQL/JSON: add standard JSON constructor functionsAlvaro Herrera2023-03-291-0/+55
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | This commit introduces the SQL/JSON standard-conforming constructors for JSON types: JSON_ARRAY() JSON_ARRAYAGG() JSON_OBJECT() JSON_OBJECTAGG() Most of the functionality was already present in PostgreSQL-specific functions, but these include some new functionality such as the ability to skip or include NULL values, and to allow duplicate keys or throw error when they are found, as well as the standard specified syntax to specify output type and format. Author: Nikita Glukhov <n.gluhov@postgrespro.ru> Author: Teodor Sigaev <teodor@sigaev.ru> Author: Oleg Bartunov <obartunov@gmail.com> Author: Alexander Korotkov <aekorotkov@gmail.com> Author: Amit Langote <amitlangote09@gmail.com> Reviewers have included (in no particular order) Andres Freund, Alexander Korotkov, Pavel Stehule, Andrew Alsup, Erik Rijkers, Zihong Yu, Himanshu Upadhyaya, Daniel Gustafsson, Justin Pryzby. Discussion: https://postgr.es/m/CAF4Au4w2x-5LTnN_bxky-mq4=WOqsGsxSpENCzHRAzSnEd8+WQ@mail.gmail.com Discussion: https://postgr.es/m/cd0bb935-0158-78a7-08b5-904886deac4b@postgrespro.ru Discussion: https://postgr.es/m/20220616233130.rparivafipt6doj3@alap3.anarazel.de Discussion: https://postgr.es/m/abd9b83b-aa66-f230-3d6d-734817f0995d%40postgresql.org
* Fix corner-case planner failure for MERGE.Tom Lane2023-03-281-3/+7
| | | | | | | | | | | | | | | | | | | | | MERGE planning could fail with "variable not found in subplan target list" if the target table is partitioned and all its partitions are excluded at plan time, or in the case where it has no partitions but used to have some. This happened because distribute_row_identity_vars thought it didn't need to make the target table's reltarget list fully valid; but if we generate a join plan then that is required because the dummy Result node's tlist will be made from the reltarget. The same logic appears in distribute_row_identity_vars in v14, but AFAICS the problem is unreachable in that branch for lack of MERGE. In other updating statements, the target table is always inner-joined to any other tables, so if the target is known dummy then the whole plan reduces to dummy, so no join nodes are created. So I'll refrain from back-patching this code change to v14 for now. Per report from Alvaro Herrera. Discussion: https://postgr.es/m/20230328112248.6as34mlx5sr4kltg@alvherre.pgsql
* Add SysCacheGetAttrNotNull for guaranteed not-null attrsDaniel Gustafsson2023-03-251-19/+4
| | | | | | | | | | | | | When extracting an attr from a cached tuple in the syscache with SysCacheGetAttr the isnull parameter must be checked in case the attr cannot be NULL. For cases when this is known beforehand, a wrapper is introduced which perform the errorhandling internally on behalf of the caller, invoking an elog in case of a NULL attr. Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com> Reviewed-by: David Rowley <dgrowleyml@gmail.com> Discussion: https://postgr.es/m/AD76405E-DB45-46B6-941F-17B1EB3A9076@yesql.se
* Fix incorrect comment in preptlist.cDavid Rowley2023-03-221-1/+1
| | | | | | Author: Etsuro Fujita Reviewed-by: Richard Guo, Tom Lane Discussion: https://postgr.es/m/CAPmGK15V8dcVxL9vcgVWPHV6pw1qzM42LzoUkQDB7-e+1onnJw@mail.gmail.com
* Correct Memoize's estimated cache hit ratio calculationDavid Rowley2023-03-221-4/+3
| | | | | | | | | | | | | | | | | | | | | | | As demonstrated by David Johnston, the Memoize cache hit ratio calculation wasn't quite correct. This change only affects the estimated hit ratio when the estimated number of entries to cache is estimated not to fit inside the cache. For example, if we expect 2000 distinct cache key values and only expect to be able to cache 1000 of those at once due to memory constraints, with an estimate of 10000 calls, if we could store all entries then the hit ratio should be 80% to account for the first 2000 of the 10000 calls to be a cache miss due to the value not being cached yet. If we can only store 1000 entries for each of the 2000 distinct possible values at once then the 80% should be reduced by half to make the final estimate of 40%. Previously, the calculation would have produced an estimated hit ratio of 30%, which wasn't correct. Apply to master only so as not to destabilize plans in the back branches. Reported-by: David G. Johnston Discussion: https://postgr.es/m/CAKFQuwZEmcNk3YQo2Xj4EDUOdY6qakad31rOD1Vc4q1_s68-Ew@mail.gmail.com Discussion: https://postgr.es/m/CAApHDvrV44LwiF4W_qf_RpbGYWSgp1kF=cZr+kTRRaALUfmXqw@mail.gmail.com
* Have the planner account for the Memoize cache key memoryDavid Rowley2023-03-201-46/+58
| | | | | | | | | | | | | | | | | | | | | The Memoize executor node stores the cache key values along with the tuple(s) which were found in the outer node which match each key value, however, when the planner tried to estimate how many entries could be stored in the cache, it didn't take into account that the cache key must also be stored. In many cases, this won't make a large difference as the key is likely small in comparison to the tuple(s) being stored, however, it's not impossible to craft cases where the key could take more memory than the tuple(s) stored for it. Here we adjust the planner so it takes into account the estimated amount of memory to store the cache key. Effectively, this change will reduce the estimated cache hit ratio when it's thought that not all items will fit in the cache, thus Memoize will become more expensive in such cases. The executor already takes into account the memory consumed by the cache key, so here we only need to adjust the planner. Discussion: https://postgr.es/m/CAApHDvqGErGuyBfQvBQrTCHDbzLTqoiW=_G9sOzeFxWEc_7auA@mail.gmail.com
* Fix incorrect logic for determining safe WindowAgg run conditionsDavid Rowley2023-03-171-66/+119
| | | | | | | | | | | | | | | The logic added in 9d9c02ccd to determine when a qual can be used as a WindowClause run condition failed to correctly check for subqueries in the qual. This was being done correctly for normal subquery qual pushdowns, it's just that 9d9c02ccd failed to follow the lead on that. This also fixes various other cases where transforming the qual into a WindowClause run condition in the subquery should have been disallowed. Bug: #17826 Reported-by: Anban Company Discussion: https://postgr.es/m/17826-7d8750952f19a5f5@postgresql.org Backpatch-through: 15, where 9d9c02ccd was introduced.
* Enable use of Memoize atop an Append that came from UNION ALL.Tom Lane2023-03-162-18/+12
| | | | | | | | | | | | | | | | | create_append_path() would only apply get_baserel_parampathinfo when the path is for a partitioned table, but it's also potentially useful for paths for UNION ALL appendrels. Specifically, that supports building a Memoize path atop this one. While we're in the vicinity, delete some dead code in create_merge_append_plan(): there's no need for it to support parameterized MergeAppend paths, and it doesn't look like that is going to change anytime soon. It'll be easy enough to undo this when/if it becomes useful. Richard Guo Discussion: https://postgr.es/m/CAMbWs4_ABSu4PWG2rE1q10tJugEXHWgru3U8dAgkoFvgrb6aEA@mail.gmail.com
* Support PlaceHolderVars in MERGE actions.Tom Lane2023-03-152-14/+10
| | | | | | | | | | | | | preprocess_targetlist thought PHVs couldn't appear here. It was mistaken, as per report from Önder Kalacı. Surveying other pull_var_clause calls, I noted no similar errors, but I did notice that qual_is_pushdown_safe's assertion about !contain_window_function was pointless, because the following pull_var_clause call would complain about them anyway. In HEAD only, remove the redundant Assert and improve the commentary. Discussion: https://postgr.es/m/CACawEhUuum-gC_2S3sXLTcsk7bUSPSHOD+g1ZpfKaDK-KKPPWA@mail.gmail.com
* Work around implementation restriction in adjust_appendrel_attrs.Tom Lane2023-03-121-4/+24
| | | | | | | | | | | | | | | | | | | | | | | adjust_appendrel_attrs can't transfer nullingrel labeling to a non-Var translation expression (mainly because it's too late to wrap such an expression in a PlaceHolderVar). I'd supposed in commit 2489d76c4 that that restriction was unreachable because we'd not attempt to push problematic clauses down to an appendrel child relation. I forgot that set_append_rel_size blindly converts all the parent rel's joininfo clauses to child clauses, and that list could well contain clauses from above a nulling outer join. We might eventually have to devise a direct fix for this implementation restriction, but for now it seems enough to filter out troublesome clauses while constructing the child's joininfo list. Such clauses are certainly not useful while constructing paths for the child rel; they'll have to be applied later when we join the completed appendrel to something else. So we don't need them here, and omitting them from the list should save a few cycles while processing the child rel. Per bug #17832 from Marko Tiikkaja. Discussion: https://postgr.es/m/17832-d0a8106cdf1b722e@postgresql.org
* Remove local optimizations of empty Bitmapsets into null pointers.Tom Lane2023-03-026-35/+2
| | | | | | | | These are all dead code now that it's done centrally. Patch by me; thanks to Nathan Bossart and Richard Guo for review. Discussion: https://postgr.es/m/1159933.1677621588@sss.pgh.pa.us
* Remove bms_first_member().Tom Lane2023-03-021-1/+2
| | | | | | | | | | | | | | This function has been semi-deprecated ever since we invented bms_next_member(). Its habit of scribbling on the input bitmapset isn't great, plus for sufficiently large bitmapsets it would take O(N^2) time to complete a loop. Now we have the additional problem that reducing the input to empty while leaving it still accessible would violate a planned invariant. So let's just get rid of it, after updating the few extant callers to use bms_next_member(). Patch by me; thanks to Nathan Bossart and Richard Guo for review. Discussion: https://postgr.es/m/1159933.1677621588@sss.pgh.pa.us
* Fix mis-handling of outer join quals generated by EquivalenceClasses.Tom Lane2023-02-234-17/+64
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | It's possible, in admittedly-rather-contrived cases, for an eclass to generate a derived "join" qual that constrains the post-outer-join value(s) of some RHS variable(s) without mentioning the LHS at all. While the mechanisms were set up to work for this, we fell foul of the "get_common_eclass_indexes" filter installed by commit 3373c7155: it could decide that such an eclass wasn't relevant to the join, so that the required qual clause wouldn't get emitted there or anywhere else. To fix, apply get_common_eclass_indexes only at inner joins, where its rule is still valid. At an outer join, fall back to examining all eclasses that mention either input (or the OJ relid, though it should be impossible for an eclass to mention that without mentioning either input). Perhaps we can improve on that later, but the cost/benefit of adding more complexity to skip some irrelevant eclasses is dubious. To allow cheaply distinguishing outer from inner joins, pass the ojrelid to generate_join_implied_equalities as a separate argument. This also allows cleaning up some sloppiness that had crept into the definition of its join_relids argument, and it allows accurate calculation of nominal_join_relids for a child outer join. (The latter oversight seems not to have been a live bug, but it certainly could have caused problems in future.) Also fix what might be a live bug in check_index_predicates: it was being sloppy about what it passed to generate_join_implied_equalities. Per report from Richard Guo. Discussion: https://postgr.es/m/CAMbWs4-DsTBfOvXuw64GdFss2=M5cwtEhY=0DCS7t2gT7P6hSA@mail.gmail.com
* Fix some issues with wrong placement of pseudo-constant quals.Tom Lane2023-02-222-9/+69
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | initsplan.c figured that it could push Var-free qual clauses to the top of the current JoinDomain, which is okay in the abstract. But if the current domain is inside some outer join, and we later commute an inside-the-domain outer join with one outside it, we end up placing the pushed-up qual clause incorrectly. In distribute_qual_to_rels, avoid this by using the syntactic scope of the qual clause; with the exception that if we're in the top-level join domain we can still use the full query relid set, ensuring the resulting gating Result node goes to the top of the plan. (This is approximately as smart as the pre-v16 code was. Perhaps we can do better later, but it's not clear that such cases are worth a lot of sweat.) In process_implied_equality, we don't have a clear notion of syntactic scope, but we do have the results of SpecialJoinInfo construction. Thumb through those and remove any lower outer joins that might get commuted to above the join domain. Again, we can make an exception for the top-level join domain. It'd be possible to work harder here (for example, by keeping outer joins that aren't shown as potentially commutable), but I'm going to stop here for the moment. This issue has convinced me that the current representation of join domains probably needs further refinement, so I'm disinclined to write inessential dependent logic just yet. In passing, tighten the qualscope passed to process_implied_equality by generate_base_implied_equalities_no_const; there's no need for it to be larger than the rel we are currently considering. Tom Lane and Richard Guo, per report from Tender Wang. Discussion: https://postgr.es/m/CAHewXNk9eJ35ru5xATWioTV4+xZPHptjy9etdcNPjUfY9RQ+uQ@mail.gmail.com
* Prevent join removal from removing the query's result relation.Tom Lane2023-02-201-0/+8
| | | | | | | | | | | | | | | | | | | | | | | | | | | | This was not something that required consideration before MERGE was invented; but MERGE builds a join tree that left-joins to the result relation, meaning that remove_useless_joins will consider removing it. That should generally be stopped by the query's use of output variables from the result relation. However, if the result relation is inherited (e.g. a partitioned table) then we don't add any row identity variables to the query until expand_inherited_rtentry, which happens after join removal. This was exposed as of commit 3c569049b, which made it possible to deduce that a partitioned table could contain at most one row matching a join key, enabling removal of the not-yet-expanded result relation. Ooops. To fix, let's just teach join_is_removable that the query result rel is never removable. It's a cheap enough test in any case, and it'll save some cycles that we'd otherwise expend in proving that it's not removable, even in the cases we got right. Back-patch to v15 where MERGE was added. Although I think the case cannot be reached in v15, this seems like cheap insurance. Per investigation of a report from Alexander Lakhin. Discussion: https://postgr.es/m/36bee393-b351-16ac-93b2-d46d83637e45@gmail.com
* Remove gratuitous assumptions about what make_modifytable can see.Tom Lane2023-02-201-22/+20
| | | | | | | | | | | | | | | For no clearly good reason, make_modifytable assumed that it could not reach its get-the-FDW-info-the-hard-way path in MERGE. It's currently possible to demonstrate that assertion failing, which seems to be due to an upstream planner bug; but there's no good reason to do it like this at all. Let's apply the principle of separation of concerns and make the MERGE check separately, after getting or not getting the fdwroutine pointer. Per report from Alexander Lakhin. No test case, since I think the potential test condition will go away soon. Discussion: https://postgr.es/m/36bee393-b351-16ac-93b2-d46d83637e45@gmail.com
* Correctly set userid of subquery relations' child relsAlvaro Herrera2023-02-201-4/+14
| | | | | | | | | | | | | | | | | The RelOptInfo->userid field (the user ID to check permissions as) of an "otherrel" relation was being copied from its parent relation, which is correct in most cases but wrong when the parent is a subquery. In that case, using the value from the RTEPermissionInfo of the child itself is the appropriate thing to do. Coming up with a test case where user-visible behavior changes proves hard enough, so we don't add one here. Bug introduced by a61b1f74823c, discovered by Amit while reviewing nearby code. Author: Amit Langote <amitlangote09@gmail.com> Discussion: https://postgr.es/m/CA+HiwqE0WY_AhLnGtTsY7eYebG212XWbM-D8gr2A_ToOHyCywQ@mail.gmail.com
* Optimize generate_orderedappend_pathsDavid Rowley2023-02-201-23/+34
| | | | | | | | | | | | | | | | | | In generate_orderedappend_paths(), when match_partition_order_desc was true, we would lcons() items to various lists in a loop over each live partition. When the number of live partitions was large, the lcons() could show up in profiles due to it having to perform memmove() to make way for the new list item. Here we adjust things so that we just perform the loop over the live partitions backwards when match_partition_order_desc is true. This allows us to simplify the logic in the loop. Now, as far as the guts of the loop knows, there's no difference between match_partition_order and match_partition_order_desc. We can just set match_partition_order to true so that we build the correct list of paths for the asc and desc case. Per idea from Andres Freund. Discussion: https://postgr.es/m/20230217002351.nyt4y5tdzg6hugdt@awork3.anarazel.de
* Rename force_parallel_mode to debug_parallel_queryDavid Rowley2023-02-152-12/+12
| | | | | | | | | | | | | | | | | | | | force_parallel_mode is meant to be used to allow us to exercise the parallel query infrastructure to ensure that it's working as we expect. It seems some users think this GUC is for forcing the query planner into picking a parallel plan regardless of the costs. A quick look at the documentation would have made them realize that they were wrong, but the GUC is likely too conveniently named which, evidently, seems to often result in users expecting that it forces the planner into usefully parallelizing queries. Here we rename the GUC to something which casual users are less likely to mistakenly think is what they need to make their query run more quickly. For now, the old name can still be used. We'll revisit if the old name mapping can be removed once the buildfarm configs are all updated. Reviewed-by: John Naylor Discussion: https://postgr.es/m/CAApHDvrsOi92_uA7PEaHZMH-S4Xv+MGhQWA+GrP8b1kjpS1HjQ@mail.gmail.com
* When removing a relation from the query, drop its RelOptInfo.Tom Lane2023-02-133-12/+15
| | | | | | | | | | | | | | | In commit b78f6264e I opined that it was "too risky" to delete a relation's RelOptInfo from the planner's data structures when we have realized that we don't need to join to it; so instead we just marked it as a dead relation. In hindsight that judgment seems flawed: any subsequent access to such a dead relation is arguably a bug in itself, so leaving the RelOptInfo present just helps to mask bugs. Let's delete it instead, allowing removal of the whole notion of a "dead relation". So far as the regression tests can find, this requires no other code changes, except for one Assert in equivclass.c that was very dubiously not complaining about access to a dead rel. Discussion: https://postgr.es/m/229905.1676062220@sss.pgh.pa.us
* Fix buggy recursion in flatten_rtes_walker().Tom Lane2023-02-131-4/+9
| | | | | | | | | | Must save-and-restore the context we are modifying. Oversight in commit a61b1f748. Tender Wang Discussion: https://postgr.es/m/CAHewXNnnNySD_YcKNuFpQDV2gxWA7_YLWqHmYVcyoOYxn8kY2A@mail.gmail.com Discussion: https://postgr.es/m/20230212233711.GA1316@telsasoft.com
* Fix thinkos in have_unsafe_outer_join_ref; reduce to Assert check.Tom Lane2023-02-131-15/+14
| | | | | | | | | | | | | | | | | Late in the development of commit 2489d76c4, I (tgl) incorrectly concluded that the new function have_unsafe_outer_join_ref couldn't ever reach its inner loop. That should be the case if the inner rel's parameterization is based on just one Var, but it could be based on Vars from several relations, and then not only is the inner loop reachable but it's wrongly coded. Despite those errors, it still appears that the whole thing is redundant given previous join_is_legal checks, so let's arrange to only run it in assert-enabled builds. Diagnosis and patch by Richard Guo, per fuzz testing by Justin Pryzby. Discussion: https://postgr.es/m/20230212235823.GW1653@telsasoft.com
* Fix join removal logic to clean up sub-RestrictInfos of OR clauses.Tom Lane2023-02-101-15/+62
| | | | | | | | | | | | | | | | | analyzejoins.c took care to clean out removed relids from the clause_relids and required_relids of RestrictInfos associated with the doomed rel ... but it paid no attention to the fact that if such a RestrictInfo contains an OR clause, there will be sub-RestrictInfos containing similar fields. I'm more than a bit surprised that this oversight hasn't caused visible problems before. In any case, it's certainly broken now, so add logic to clean out the sub-RestrictInfos recursively. We might need to back-patch this someday. Per bug #17786 from Robins Tharakan. Discussion: https://postgr.es/m/17786-f1ea7fbdab97daec@postgresql.org
* Further fixes in qual nullingrel adjustment for outer join commutation.Tom Lane2023-02-101-5/+16
| | | | | | | | | | | | | | | | | | | | | | One of the add_nulling_relids calls in deconstruct_distribute_oj_quals added an OJ relid to too few Vars, while the other added it to too many. We should consider the syntactic structure not min_left/righthand while deciding which Vars to decorate, and when considering pushing up a lower outer join pursuant to transforming the second form of OJ identity 3 to the first form, we only want to decorate Vars coming from its LHS. In a related bug, I realized that make_outerjoininfo was failing to check a very basic property that's needed to apply OJ identity 3: the syntactically-upper outer join clause can't refer to the lower join's LHS. This didn't break the join order restriction logic, but it led to setting bogus commute_xxx bits, possibly resulting in bogus nullingrel markings in modified quals. Richard Guo and Tom Lane Discussion: https://postgr.es/m/CAMbWs497CmBruMx1SOjepWEz+T5NWa4scqbdE9v7ZzSXqH_gQw@mail.gmail.com Discussion: https://postgr.es/m/CAEP4nAx9C5gXNBfEA0JBfz7B+5f1Bawt-RWQWyhev-wdps8BZA@mail.gmail.com
* Further tighten nullingrel marking rules in build_joinrel_tlist().Tom Lane2023-02-081-9/+11
| | | | | | | | | | The code I added in fee7b77b9 could misbehave if commute_above_r contains multiple relids. While adding too many relids here is probably harmless (pre-fee7b77b9, we did it all the time), it's not very expensive to be accurate: we just have to intersect commute_above_r with the join's relids. Discussion: https://postgr.es/m/17781-c0405c8b3cd5e072@postgresql.org
* remove_rel_from_query() must clean up PlaceHolderVar.phrels fields.Tom Lane2023-02-081-7/+10
| | | | | | | | | | While we got away with this sloppiness before, it's not okay now that fee7b77b9 caused build_joinrel_tlist() to make use of phrels. Per report from Robins Tharakan. Richard Guo (some cosmetic tweaks by me) Discussion: https://postgr.es/m/CAMbWs4_ngw9sKxpTE8hqk=-ooVX_CQP3DarA4HzkRMz_JKpTrA@mail.gmail.com
* Rethink nullingrel marking rules in build_joinrel_tlist().Tom Lane2023-02-071-19/+24
| | | | | | | | | | | | | | | | | | The logic for when to add the current outer join's own relid to the nullingrels sets of output Vars and PHVs was overly complicated and underly correct. Not sure why I didn't think of this before, but since what we want is marking per the syntactic structure, we can just consult our records about the syntactic structure, ie syn_righthand/syn_lefthand. Also, tighten the rule about when to add the commute_above_r bits, in hopes of eliminating some squishy reasoning. I do not know of a reason to think that that's broken as-is, but this way seems better. Per bug #17781 from Robins Tharakan. Discussion: https://postgr.es/m/17781-c0405c8b3cd5e072@postgresql.org
* Remove leftover code in deconstruct_distribute_oj_quals().Tom Lane2023-02-071-7/+0
| | | | | | | | | | The initial "put back OJ relids" adjustment of ojscope was incorrect and unnecessary; it seems to be a leftover from when I (tgl) was trying to get this function to work at all. Richard Guo Discussion: https://postgr.es/m/CAMbWs4-L2C47ZGZPabBAi5oDZsKmsbvhYcGCy5o=gCjsaG_ZQA@mail.gmail.com
* Fix up join removal's interaction with PlaceHolderVars.Tom Lane2023-02-061-22/+30
| | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | The portion of join_is_removable() that checks PlaceHolderVars can be made a little more accurate and intelligible than it was. The key point is that we can allow join removal even if a PHV mentions the target rel in ph_eval_at, if that mention was only added as a consequence of forcing the PHV up to a join level that's at/above the outer join we're trying to get rid of. We can check that by testing for the OJ's relid appearing in ph_eval_at, indicating that it's supposed to be evaluated after the outer join, plus the existing test that the contained expression doesn't actually mention the target rel. While here, add an explicit check that there'll be something left in ph_eval_at after we remove the target rel and OJ relid. There is an Assert later on about that, and I'm not too sure that the case could happen for a PHV satisfying the other constraints, but let's just check. (There was previously a bms_is_subset test that meant to cover this risk, but it's broken now because it doesn't account for the fact that we'll also remove the OJ relid.) The real reason for revisiting this code though is that the Assert I left behind in 8538519db turns out to be easily reachable, because if a PHV of this sort appears in an upper-level qual clause then that clause's clause_relids will include the PHV's ph_eval_at relids. This is a mirage though: we have or soon will remove these relids from the PHV's ph_eval_at, and therefore they no longer belong in qual clauses' clause_relids either. Remove that Assert in join_is_removable, and replace the similar one in remove_rel_from_query with code to remove the deleted relids from clause_relids. Per bug #17773 from Robins Tharakan. Discussion: https://postgr.es/m/17773-a592e6cedbc7bac5@postgresql.org
* Remove useless casts to (void *) in hash_search() callsPeter Eisentraut2023-02-061-1/+1
| | | | | | | | | | | Some of these appear to be leftovers from when hash_search() took a char * argument (changed in 5999e78fc45dcb91784b64b6e9ae43f4e4f68ca2). Since after this there is some more horizontal space available, do some light reformatting where suitable. Reviewed-by: Corey Huinker <corey.huinker@gmail.com> Discussion: https://www.postgresql.org/message-id/flat/fd9adf5d-b1aa-e82f-e4c7-263c30145807%40enterprisedb.com
* Fix over-optimistic updating of info about commutable outer joins.Tom Lane2023-02-051-16/+58
| | | | | | | | | | | | | | | | | | | | | | | | | | make_outerjoininfo was set up to update SpecialJoinInfo's commute_below, commute_above_l, commute_above_r fields as soon as it found a pair of outer joins that look like they can commute. However, this decision could be negated later in the same loop due to finding an intermediate outer join that prevents commutation. That left us with commute_xxx fields that were contradictory to the join order restrictions expressed in min_lefthand/min_righthand. The latter fields would keep us from actually choosing a bad join order; but the inconsistent commute_xxx fields could bollix details such as the varnullingrels values created for intermediate join relation targetlists, ending in an assertion failure in setrefs.c. To fix, wait till the end of make_outerjoininfo where we have accurate values for min_lefthand/min_righthand, and then insert only relids not present in those sets into the commute_xxx fields. Per SQLSmith testing by Robins Tharakan. Note that while Robins bisected the failure to commit b448f1c8d, it's really the fault of 2489d76c4. The outerjoin_delayed logic removed in the later commit was keeping us from deciding that troublesome join pairs commute, at least in the specific example seen here. Discussion: https://postgr.es/m/CAEP4nAyAORgE8K_RHSmvWbE9UaChhjbEL1RrDU3neePwwRUB=A@mail.gmail.com
* Fix thinko in qual distribution.Tom Lane2023-02-041-15/+14
| | | | | | | | | | | | | | | | deconstruct_distribute tweaks the outer join scope (ojscope) it passes to distribute_qual_to_rels when considering an outer join qual that's above potentially-commutable outer joins. However, if the current join is *not* potentially commutable, we shouldn't do that. The argument that distribute_qual_to_rels will not do something wrong with the bogus ojscope falls flat if we don't pass it non-null postponed_oj_qual_list. Moreover, there's no need to play games in this case since we aren't going to commute anything. Per SQLSmith testing by Robins Tharakan. Discussion: https://postgr.es/m/CAEP4nAw74k4b-=93gmfCNX3MOY3y4uPxqbk_MnCVEpdsqHJVsg@mail.gmail.com
View Lorry Controller Status