Re: set_cheapest without checking pathlist

2024-01-31 Thread Richard Guo
On Thu, Feb 1, 2024 at 10:04 AM James Coleman wrote: > I don't see any inherent reason why we must always assume that > gather_grouping_paths will always result in having at least one entry > in pathlist. If, for example, we've disabled incremental sort and the > cheapest partial path happens to

Re: Oversight in reparameterize_path_by_child leading to executor crash

2024-01-31 Thread Richard Guo
On Wed, Jan 31, 2024 at 11:21 PM Tom Lane wrote: > Richard Guo writes: > > On Wed, Jan 31, 2024 at 5:12 AM Tom Lane wrote: > >> * Why is it okay to just use pull_varnos on a tablesample expression, > >> when contain_references_to() does something different?

Re: Apply the "LIMIT 1" optimization to partial DISTINCT

2024-01-30 Thread Richard Guo
On Wed, Jan 31, 2024 at 12:26 PM David Rowley wrote: > On Fri, 26 Jan 2024 at 21:14, David Rowley wrote: > > However, having said that. Parallel plans are often picked when there > > is some highly selective qual as parallel_tuple_cost has to be applied > > to fewer tuples for such plans, so

Re: Some revises in adding sorting path

2024-01-30 Thread Richard Guo
On Wed, Jan 31, 2024 at 5:13 AM David Rowley wrote: > On Wed, 31 Jan 2024 at 00:44, Richard Guo wrote: > > This patchset does not aim to introduce anything new; it simply > > refactors the existing code. The newly added tests are used to show > > that the code

Re: Oversight in reparameterize_path_by_child leading to executor crash

2024-01-30 Thread Richard Guo
On Wed, Jan 31, 2024 at 5:12 AM Tom Lane wrote: > Richard Guo writes: > > On Wed, Jan 17, 2024 at 5:01 PM Richard Guo > wrote: > >> Sure, here it is: > >> v10-0001-Avoid-reparameterizing-Paths-when-it-s-not-suitable.patch > > > I forgot to mention th

Re: Some revises in adding sorting path

2024-01-30 Thread Richard Guo
On Tue, Jan 30, 2024 at 7:00 PM David Rowley wrote: > On Mon, 29 Jan 2024 at 22:39, Richard Guo wrote: > > So in the v3 patch I've brought back the logic that considers > > incremental sort on partial paths in gather_grouping_paths(). However, > > I failed t

Re: Support run-time partition pruning for hash join

2024-01-29 Thread Richard Guo
On Sat, Jan 27, 2024 at 11:29 AM vignesh C wrote: > CFBot shows that the patch does not apply anymore as in [1]: > > Please post an updated version for the same. Attached is an updated patch. Nothing else has changed. Thanks Richard

Re: Retiring is_pushed_down

2024-01-29 Thread Richard Guo
On Sun, Jan 21, 2024 at 8:37 PM vignesh C wrote: > I'm seeing that there has been no activity in this thread for nearly 6 > months, I'm planning to close this in the current commitfest unless > someone is planning to take it forward. It can be opened again when > there is more interest. I'm

Re: Some revises in adding sorting path

2024-01-29 Thread Richard Guo
On Mon, Jul 17, 2023 at 4:55 PM Richard Guo wrote: > But I did not find a query that makes an incremental sort in this case. > After trying for a while it seems to me that we do not need to consider > incremental sort in this case, because for a partial path of a grouped > or parti

Re: Wrong rows estimations with joins of CTEs slows queries by more than factor 500

2024-01-28 Thread Richard Guo
On Mon, Jan 29, 2024 at 11:20 AM vignesh C wrote: > On Mon, 29 Jan 2024 at 08:01, Richard Guo wrote: > > On Sat, Jan 27, 2024 at 10:08 AM vignesh C wrote: > >> I have changed the status of the commitfest entry to "Committed" as I > >> noticed the patch has

Propagate pathkeys from CTEs up to the outer query

2024-01-28 Thread Richard Guo
In [1] we've reached a conclusion that for a MATERIALIZED CTE it's okay to 'allow our statistics or guesses for the sub-query to subsequently influence what the upper planner does'. Commit f7816aec23 exposes column statistics to the upper planner. In the light of that, here is a patch that

Re: Wrong rows estimations with joins of CTEs slows queries by more than factor 500

2024-01-28 Thread Richard Guo
On Sat, Jan 27, 2024 at 10:08 AM vignesh C wrote: > On Mon, 8 Jan 2024 at 22:21, Tom Lane wrote: > > > > Richard Guo writes: > > > On Sun, Jan 7, 2024 at 6:41 AM Tom Lane wrote: > > >> Thanks for the report! I guess we need something like the attached.

Re: Reordering DISTINCT keys to match input path's pathkeys

2024-01-26 Thread Richard Guo
On Tue, Jan 23, 2024 at 5:03 PM David Rowley wrote: > I've not caught up on the specifics of 0452b461b, but I just wanted to > highlight that there was some work done in [1] in this area. It seems > Ankit didn't ever add that to a CF, so that might explain why it's > been lost. > > Anyway, just

Apply the "LIMIT 1" optimization to partial DISTINCT

2024-01-25 Thread Richard Guo
In 5543677ec9 we introduced an optimization that uses Limit instead of Unique to implement DISTINCT when all the DISTINCT pathkeys have been marked as redundant. I happened to notice that this optimization was not applied to partial DISTINCT, which I think should be. This can improve plans in

Re: A performance issue with Memoize

2024-01-25 Thread Richard Guo
On Fri, Jan 26, 2024 at 12:18 PM David Rowley wrote: > On Fri, 26 Jan 2024 at 16:51, Tom Lane wrote: > > >> However ... it seems like we're not out of the woods yet. Why > > >> is Richard's proposed test case still showing > > >> + -> Memoize (actual rows=5000 loops=N) > > >> +

Re: A performance issue with Memoize

2024-01-25 Thread Richard Guo
On Fri, Jan 26, 2024 at 1:22 AM Tom Lane wrote: > Apologies for not having noticed this thread before. I'm taking > a look at it now. However, while sniffing around this I found > what seems like an oversight in paramassign.c's > assign_param_for_var(): it says it should compare all the same >

Re: A performance issue with Memoize

2024-01-25 Thread Richard Guo
On Fri, Jan 26, 2024 at 2:32 AM Tom Lane wrote: > I'm fairly sure I thought it wouldn't matter because of the Param > de-duplication done in paramassign.c. However, Richard's example > shows that's not so, because process_subquery_nestloop_params is > picky about the param ID assigned to a

Re: A compiling warning in jsonb_populate_record_valid

2024-01-24 Thread Richard Guo
On Thu, Jan 25, 2024 at 2:28 PM Amit Langote wrote: > On Thu, Jan 25, 2024 at 2:59 PM Richard Guo > wrote: > > I came across a warning when building master (a044e61f1b) on old GCC > > (4.8.5). > > > > jsonfuncs.c: In function ‘jsonb_populate_record_valid’: >

Re: Trivial revise for the check of parameterized partial paths

2024-01-24 Thread Richard Guo
On Sun, Jan 21, 2024 at 8:36 PM vignesh C wrote: > I'm seeing that there has been no activity in this thread for nearly 7 > months, I'm planning to close this in the current commitfest unless > someone is planning to take it forward. This patch fixes the wrong comments in

A compiling warning in jsonb_populate_record_valid

2024-01-24 Thread Richard Guo
I came across a warning when building master (a044e61f1b) on old GCC (4.8.5). jsonfuncs.c: In function ‘jsonb_populate_record_valid’: ../../../../src/include/nodes/miscnodes.h:53:15: warning: the comparison will always evaluate as ‘true’ for the address of ‘escontext’ will never be NULL

Re: Removing const-false IS NULL quals and redundant IS NOT NULL quals

2024-01-22 Thread Richard Guo
On Tue, Jan 23, 2024 at 1:11 PM David Rowley wrote: > I went over this again. I did a little more work adjusting comments > and pushed it. > > Thanks for all your assistance with this, Richard. Thanks for pushing! This is really great. Thanks Richard

Reordering DISTINCT keys to match input path's pathkeys

2024-01-22 Thread Richard Guo
Similar to what we did to GROUP BY keys in 0452b461bc, I think we can do the same to DISTINCT keys, i.e. reordering DISTINCT keys to match input path's pathkeys, which can help reduce cost by avoiding unnecessary re-sort, or allowing us to use incremental-sort to save efforts. For instance,

Re: Strange Bitmapset manipulation in DiscreteKnapsack()

2024-01-17 Thread Richard Guo
On Thu, Jan 18, 2024 at 8:35 AM David Rowley wrote: > The functions's header comment mentions "The bitmapsets are all > pre-initialized with an unused high bit so that memory allocation is > done only once.". Ah, I neglected to notice this when reviewing the v1 patch. I guess it's implemented

Re: Oversight in reparameterize_path_by_child leading to executor crash

2024-01-17 Thread Richard Guo
On Wed, Jan 17, 2024 at 5:01 PM Richard Guo wrote: > On Tue, Jan 16, 2024 at 2:30 AM Robert Haas wrote: > >> On Mon, Jan 8, 2024 at 3:32 AM Richard Guo >> wrote: > > > Fair point. I think we can back-patch a more minimal fix, as Tom >> > proposed in [1], wh

Re: Oversight in reparameterize_path_by_child leading to executor crash

2024-01-17 Thread Richard Guo
On Tue, Jan 16, 2024 at 2:30 AM Robert Haas wrote: > On Mon, Jan 8, 2024 at 3:32 AM Richard Guo wrote: > > Thanks for the suggestion. Attached is an updated patch which is added > > with a commit message that tries to explain the problem and the fix. > > This is

Re: Fix a typo of func DecodeInsert()

2024-01-16 Thread Richard Guo
On Wed, Jan 17, 2024 at 8:47 AM Yongtao Huang wrote: > Hi all, > I think the comment above the function DecodeInsert() > in src/backend/replication/logical/decode.c should be > + * *Inserts *can contain the new tuple. > , rather than > - * *Deletes *can contain the new tuple. > Nice catch. +1.

Re: Strange Bitmapset manipulation in DiscreteKnapsack()

2024-01-16 Thread Richard Guo
On Tue, Jan 16, 2024 at 11:32 AM David Rowley wrote: > While working on [1], I noticed some strange code in > DiscreteKnapsack() which seems to be aiming to copy the Bitmapset. > > It's not that obvious at a casual glance, but: > > sets[j] = bms_del_members(sets[j], sets[j]); > > this is aiming

Re: Revise the Asserts added to bimapset manipulation functions

2024-01-16 Thread Richard Guo
On Tue, Jan 16, 2024 at 11:08 AM David Rowley wrote: > I've now adjusted the patch to have all modifications to Bitmapsets > return a newly allocated set. There are a few cases missing in master > that need to be fixed (items 6-10 below): > > The patch now includes changes for the following: > >

Re: POC: GROUP BY optimization

2024-01-15 Thread Richard Guo
On Mon, Jan 15, 2024 at 3:56 PM Alexander Korotkov wrote: > On Mon, Jan 15, 2024 at 8:42 AM Richard Guo > wrote: > > On Mon, Jan 15, 2024 at 8:20 AM Alexander Korotkov > wrote: > >> > >> Thank you for providing the test case relevant for this code change. >

Re: POC: GROUP BY optimization

2024-01-14 Thread Richard Guo
On Mon, Jan 15, 2024 at 8:20 AM Alexander Korotkov wrote: > Thank you for providing the test case relevant for this code change. > The revised patch incorporating this change is attached. Now the > patchset looks good to me. I'm going to push it if there are no > objections. Seems I'm late

Re: An inefficient query caused by unnecessary PlaceHolderVar

2024-01-14 Thread Richard Guo
Updated this patch over 29f114b6ff, which indicates that we should apply the same rules for PHVs. Thanks Richard v3-0001-Avoid-unnecessary-PlaceHolderVars-for-Vars-PHVs.patch Description: Binary data

Re: Can we rely on the ordering of paths in pathlist?

2024-01-09 Thread Richard Guo
On Tue, Apr 11, 2023 at 11:43 AM Andy Fan wrote: > On Tue, Apr 11, 2023 at 11:03 AM Richard Guo > wrote: > >> As the comment above add_path() says, 'The pathlist is kept sorted by >> total_cost, with cheaper paths at the front.' And it seems that >> get_cheapes

Re: Support "Right Semi Join" plan shapes

2024-01-09 Thread Richard Guo
On Sun, Jan 7, 2024 at 3:03 PM vignesh C wrote: > One of the tests in CFBot has failed at [1] with: > - Relations: (public.ft1 t1) SEMI JOIN (public.ft2 t2) > - Remote SQL: SELECT r1."C 1", r1.c2, r1.c3, r1.c4, r1.c5, r1.c6, > r1.c7, r1.c8 FROM "S 1"."T 1" r1 WHERE ((r1."C 1" < 20)) AND

Re: Wrong rows estimations with joins of CTEs slows queries by more than factor 500

2024-01-08 Thread Richard Guo
On Sun, Jan 7, 2024 at 6:41 AM Tom Lane wrote: > Alexander Lakhin writes: > > Please look at the following query: > > CREATE TABLE t(i int); > > INSERT INTO t VALUES (1); > > VACUUM ANALYZE t; > > > WITH ir AS (INSERT INTO t VALUES (2) RETURNING i) > > SELECT * FROM ir WHERE i = 2; > > > which

Re: Oversight in reparameterize_path_by_child leading to executor crash

2024-01-08 Thread Richard Guo
Thanks for the review! On Sat, Jan 6, 2024 at 2:36 AM Robert Haas wrote: > Richard, I think it could be useful to put a better commit message > into the patch file, describing both what problem is being fixed and > what the design of the fix is. I gather that the problem is that we > crash if

Re: Revise the Asserts added to bimapset manipulation functions

2024-01-01 Thread Richard Guo
On Sun, Dec 31, 2023 at 6:44 AM David Rowley wrote: > On Fri, 29 Dec 2023 at 23:38, Richard Guo wrote: > > After applying this process to the first RestrictInfo, the bitmapset now > > becomes {t2, t3}. Consequently, the second RestrictInfo also perceives > > {t2, t3} a

Re: Revise the Asserts added to bimapset manipulation functions

2023-12-29 Thread Richard Guo
On Fri, Dec 29, 2023 at 5:22 PM David Rowley wrote: > On Fri, 29 Dec 2023 at 21:07, Richard Guo wrote: > > It seems to me that the former scenario also makes sense in some cases. > > For instance, let's say there are two pointers in two structs, s1->p and > > s2->p

Re: Revise the Asserts added to bimapset manipulation functions

2023-12-29 Thread Richard Guo
On Fri, Dec 29, 2023 at 9:15 AM David Rowley wrote: > I looked into this a bit more and I just can't see why the current > code feels like it must do the reallocation in functions such as > bms_del_members(). We don't repalloc the set there, ever, so why do > we need to do it when building with

Re: Failed assertion in joininfo.c, remove_join_clause_from_rels

2023-12-27 Thread Richard Guo
On Wed, Dec 27, 2023 at 8:00 PM Alexander Korotkov wrote: > On Wed, Dec 27, 2023 at 1:54 PM Andreas Seltenreich > wrote: > > SQLsmith found a failing Assertion in joininfo.c on master. I can > > reproduce it on an assertion-enabled build like this: > > > > create table t1(a int primary key); >

Re: Removing const-false IS NULL quals and redundant IS NOT NULL quals

2023-12-27 Thread Richard Guo
On Wed, Dec 27, 2023 at 7:38 PM Andy Fan wrote: > I also want to add notnullattnums for the UniqueKey stuff as well, by > comparing your implementation with mine, I found you didn't consider > the NOT NULL generated by filter. After apply your patch: > > create table a(a int); > explain (costs

Revise the Asserts added to bimapset manipulation functions

2023-12-27 Thread Richard Guo
The Asserts added to bitmapset.c by commits 71a3e8c43b and 7d58f2342b contain some duplicates, such as in bms_difference, bms_is_subset, bms_subset_compare, bms_int_members and bms_join. For instance, @@ -953,6 +1033,15 @@ bms_int_members(Bitmapset *a, const Bitmapset *b) int

Re: Update docs for default value of fdw_tuple_cost

2023-12-26 Thread Richard Guo
On Wed, Dec 27, 2023 at 12:27 AM Umair Shahid wrote: > Commit cac169d686eddb277880a0d8a760ac3007b4846a updated the default value > of fdw_tuple_cost from 0.01 to 0.2. The attached patch updates the docs to > reflect this change. > +1. Nice catch. Thanks Richard >

An improvement on parallel DISTINCT

2023-12-26 Thread Richard Guo
While reviewing Heikki's Omit-junk-columns patchset[1], I noticed that root->upper_targets[] is used to set target for partial_distinct_rel, which is not great because root->upper_targets[] is not supposed to be used by the core code. The comment in grouping_planner() says: * Save the various

Re: Avoid computing ORDER BY junk columns unnecessarily

2023-12-25 Thread Richard Guo
On Fri, Dec 22, 2023 at 2:38 AM Heikki Linnakangas wrote: > v1-0004-Omit-columns-from-final-tlist-that-were-only-need.patch > > The main patch in this series. This patch filters out the junk columns created for ORDER BY/GROUP BY, and retains the junk columns created for RowLocks. I'm afraid

Re: Avoid computing ORDER BY junk columns unnecessarily

2023-12-25 Thread Richard Guo
On Sat, Dec 23, 2023 at 1:32 AM Tom Lane wrote: > Heikki Linnakangas writes: > > On 22/12/2023 17:24, Tom Lane wrote: > >> How much of your patchset still makes sense if we assume that we > >> can always extract the ORDER BY column values from the index? > > > That would make it much less

Re: Check lateral references within PHVs for memoize cache keys

2023-12-24 Thread Richard Guo
On Thu, Jul 13, 2023 at 3:12 PM Richard Guo wrote: > So I'm wondering if it'd be better that we move all this logic of > computing additional lateral references within PHVs to get_memoize_path, > where we can examine only PHVs that are evaluated at innerrel. And > considering that t

Erroneous -Werror=missing-braces on old GCC

2023-12-24 Thread Richard Guo
I came across the 'missing braces' warning again when building master (0a93f803f4) on old GCC (4.8.5). blkreftable.c: In function ‘BlockRefTableSetLimitBlock’: blkreftable.c:268:2: warning: missing braces around initializer [-Wmissing-braces] BlockRefTableKey key = {0}; /* make sure any padding

Re: Update the comment in nodes.h to cover Cardinality

2023-12-19 Thread Richard Guo
On Tue, Dec 19, 2023 at 10:50 PM Peter Eisentraut wrote: > On 19.12.23 07:23, Richard Guo wrote: > > By chance I discovered that the comment for the typedefs of "double"s > > does not cover Cardinality. Should we update that comment accordingly, >

Update the comment in nodes.h to cover Cardinality

2023-12-18 Thread Richard Guo
By chance I discovered that the comment for the typedefs of "double"s does not cover Cardinality. Should we update that comment accordingly, maybe something like below? - * Typedefs for identifying qualifier selectivities and plan costs as such. - * These are just plain "double"s, but declaring

Re: planner chooses incremental but not the best one

2023-12-18 Thread Richard Guo
On Mon, Dec 18, 2023 at 7:31 AM Tomas Vondra wrote: > Oh! Now I see what you meant by using the new formula in 84f9a35e3 > depending on how we sum tuples. I agree that seems like the right thing. > > I'm not sure it'll actually help with the issue, though - if I apply the > patch, the plan does

Re: planner chooses incremental but not the best one

2023-12-15 Thread Richard Guo
On Thu, Dec 14, 2023 at 6:02 PM Richard Guo wrote: > It seems that we need to improve estimate of distinct values in > estimate_num_groups() when taking the selectivity of restrictions into > account. > > In 84f9a35e3 we changed to a new formula to perform such estimation.

Re: planner chooses incremental but not the best one

2023-12-14 Thread Richard Guo
On Tue, Dec 12, 2023 at 4:40 PM Nicolas Lutic wrote: > I've come across a behaviour of the planner I can't explain. > After a migration from 11 to 15 (on RDS) we noticed a degradation in > response time on a query, it went from a few seconds to ten minutes. > A vacuum(analyze) has been realized

Re: Oversight in reparameterize_path_by_child leading to executor crash

2023-12-10 Thread Richard Guo
On Fri, Dec 8, 2023 at 5:39 PM Alena Rybakina wrote: > On 06.12.2023 10:30, Richard Guo wrote: > > I've self-reviewed this patch again and I think it's now in a > > committable state. I'm wondering if we can mark it as 'Ready for > > Committer' now, or we need more revi

Re: Assert failure on 'list_member_ptr(rel->joininfo, restrictinfo)'

2023-12-10 Thread Richard Guo
On Fri, Dec 8, 2023 at 3:13 PM Alexander Pyhalov wrote: > Andrei Lepikhov писал(а) 2023-12-08 07:37: > > I'd already clashed with Tom on copying the required_relids field and > > voluntarily made unnecessary copies in the project [1]. > > And ... stuck into huge memory consumption. The reason

Re: Wrong results with grouping sets

2023-12-07 Thread Richard Guo
On Mon, Sep 25, 2023 at 3:11 PM Richard Guo wrote: > If the grouping expression is a Var or PHV, we can just set its > nullingrels, very straightforward. For an expression that is neither a > Var nor a PHV, I'm not quite sure how to set the nullingrels. I tried > the idea

Re: Oversight in reparameterize_path_by_child leading to executor crash

2023-12-05 Thread Richard Guo
I've self-reviewed this patch again and I think it's now in a committable state. I'm wondering if we can mark it as 'Ready for Committer' now, or we need more review comments/feedbacks. To recap, this patch postpones reparameterization of paths until createplan.c, which would help avoid building

Re: Oversight in reparameterize_path_by_child leading to executor crash

2023-12-05 Thread Richard Guo
On Fri, Oct 20, 2023 at 2:52 AM Alena Rybakina wrote: > Thank you for your work on the subject. > Thanks for taking an interest in this patch. > During review your patch I didn't understand why are you checking that the > variable is path and not new_path of type T_SamplePath (I highlighted

Re: Rename ShmemVariableCache and initialize it in more standard way

2023-12-04 Thread Richard Guo
On Tue, Dec 5, 2023 at 12:31 AM Tristan Partin wrote: > On Mon Dec 4, 2023 at 6:49 AM CST, Heikki Linnakangas wrote: > > This came up in the "Refactoring backend fork+exec code" thread recently > > [0], but is independent of that work: > > > > Here's a patch to allocate and initialize it with a

Re: A wrong comment about search_indexed_tlist_for_var

2023-12-04 Thread Richard Guo
On Sat, Dec 2, 2023 at 2:27 AM Tom Lane wrote: > Alvaro Herrera writes: > > On 2023-Dec-01, Richard Guo wrote: > >> However, this cross-check will also be performed in non-debug builds > >> ever since commit 867be9c07, which converts this check from Asserts to >

Re: Fix a wrong comment in setrefs.c

2023-12-01 Thread Richard Guo
On Tue, Nov 28, 2023 at 8:19 PM Heikki Linnakangas wrote: > On 03/11/2023 08:10, Richard Guo wrote: > > On Tue, Sep 26, 2023 at 9:51 AM Richard Guo > <mailto:guofengli...@gmail.com>> wrote: > > On Tue, Sep 26, 2023 at 5:45 AM Tom Lane > <

A wrong comment about search_indexed_tlist_for_var

2023-12-01 Thread Richard Guo
The comment of search_indexed_tlist_for_var says: * In debugging builds, we cross-check the varnullingrels of the subplan * output Var based on nrm_match. However, this cross-check will also be performed in non-debug builds ever since commit 867be9c07, which converts this check from Asserts to

Re: brininsert optimization opportunity

2023-11-26 Thread Richard Guo
On Mon, Nov 27, 2023 at 1:53 PM Soumyadeep Chakraborty < soumyadeep2...@gmail.com> wrote: > On Sun, Nov 26, 2023 at 9:28 PM Richard Guo > wrote: > > It seems that we have an oversight in this commit. If there is no tuple > > that has been inserted, we wouldn't have a

Re: Incorrect comment in tableam.h regarding GetHeapamTableAmRoutine()

2023-11-26 Thread Richard Guo
On Mon, Nov 27, 2023 at 1:50 PM Michael Paquier wrote: > I have noticed that GetHeapamTableAmRoutine() is listed as being a > member of tableamapi.c but it is a convenience routine located in > heapam_handler.c. Shouldn't the header be fixed with something like > the attached? +1. Nice catch.

Re: brininsert optimization opportunity

2023-11-26 Thread Richard Guo
On Sun, Nov 26, 2023 at 4:06 AM Tomas Vondra wrote: > I've done a bit more cleanup on the last version of the patch (renamed > the fields to start with bis_ as agreed, rephrased the comments / docs / > commit message a bit) and pushed. It seems that we have an oversight in this commit. If

Re: Don't use bms_membership in places where it's not needed

2023-11-23 Thread Richard Guo
On Fri, Nov 24, 2023 at 12:06 PM David Rowley wrote: > In the attached, I've adjusted the code to use the latter of the two > above methods in 3 places. In examine_variable() this reduces the > complexity of the logic quite a bit and saves calling bms_is_member() > in addition to

Re: Assert failure on 'list_member_ptr(rel->joininfo, restrictinfo)'

2023-11-22 Thread Richard Guo
On Sun, Nov 19, 2023 at 9:17 AM Alexander Korotkov wrote: > It's here. New REALLOCATE_BITMAPSETS forces bitmapset reallocation on > each modification. +1 to the idea of introducing a reallocation mode to Bitmapset. > I had the feeling of falling into a rabbit hole while debugging all > the

Re: Wrong rows estimations with joins of CTEs slows queries by more than factor 500

2023-11-20 Thread Richard Guo
On Tue, Nov 21, 2023 at 1:46 AM Tom Lane wrote: > * Do we really need to use make_tlist_from_pathtarget? Why isn't > the tlist of the cteplan good enough (indeed, more so)? I think you are right. The cteplan->targetlist is built for the CTE's best path by build_path_tlist(), which is almost

Re: Wrong rows estimations with joins of CTEs slows queries by more than factor 500

2023-11-19 Thread Richard Guo
On Fri, Nov 17, 2023 at 11:38 AM Tom Lane wrote: > That line of argument also leads to the conclusion that it'd be > okay to expose info about the ordering of the CTE result to the > upper planner. This patch doesn't do that, and I'm not sufficiently > excited about the issue to go write some

Re: Wrong rows estimations with joins of CTEs slows queries by more than factor 500

2023-11-16 Thread Richard Guo
On Fri, Nov 17, 2023 at 11:38 AM Tom Lane wrote: > That line of argument also leads to the conclusion that it'd be > okay to expose info about the ordering of the CTE result to the > upper planner. This patch doesn't do that, and I'm not sufficiently > excited about the issue to go write some

Re: Wrong results with grouping sets

2023-11-16 Thread Richard Guo
On Thu, Nov 16, 2023 at 11:25 PM Alena Rybakina wrote: > I noticed that this query worked correctly in the main branch with the > inequality operator: > > postgres=# select distinct on (a, b) a, b from (values (3, 1), (2, 2)) as > t (a, b) where a > b group by grouping sets((a, b), (a)); a | b

Re: Wrong rows estimations with joins of CTEs slows queries by more than factor 500

2023-11-16 Thread Richard Guo
On Fri, Nov 17, 2023 at 2:16 AM Tom Lane wrote: > So you could argue that there's more to do here, but I'm hesitant > to go further. Part of the point of MATERIALIZED is to be an > optimization fence, so breaking down that fence is something to be > wary of. Maybe we shouldn't even take this

Re: Wrong rows estimations with joins of CTEs slows queries by more than factor 500

2023-11-16 Thread Richard Guo
On Thu, Nov 9, 2023 at 6:45 AM Tom Lane wrote: > The existing RTE_SUBQUERY stanza has most of what we need for this, > so I experimented with extending that to also handle RTE_CTE. It > seems to work, though I soon found out that it needed tweaking for > the case where the CTE is

Assert failure on 'list_member_ptr(rel->joininfo, restrictinfo)'

2023-11-14 Thread Richard Guo
While working on BUG #18187 [1], I noticed that we also have issues with how SJE replaces join clauses involving the removed rel. As an example, consider the query below, which would trigger an Assert. create table t (a int primary key, b int); explain (costs off) select * from t t1 inner

Re: Support run-time partition pruning for hash join

2023-11-06 Thread Richard Guo
On Mon, Nov 6, 2023 at 11:00 PM Alexander Lakhin wrote: > Please look at a warning and an assertion failure triggered by the > following script: > set parallel_setup_cost = 0; > set parallel_tuple_cost = 0; > set min_parallel_table_scan_size = '1kB'; > > create table t1 (i int) partition by

Re: Compiling warnings on old GCC

2023-11-05 Thread Richard Guo
On Mon, Nov 6, 2023 at 2:51 PM David Rowley wrote: > On Mon, 6 Nov 2023 at 19:14, Richard Guo wrote: > > I came across the following compiling warnings on GCC (Red Hat 4.8.5-44) > > 4.8.5 with 'CFLAGS=-Og' > > > I wonder if this is worth fixing, maybe by a trivial

Compiling warnings on old GCC

2023-11-05 Thread Richard Guo
I came across the following compiling warnings on GCC (Red Hat 4.8.5-44) 4.8.5 with 'CFLAGS=-Og' be-fsstubs.c: In function ‘be_lo_export’: be-fsstubs.c:537:24: warning: ‘fd’ may be used uninitialized in this function [-Wmaybe-uninitialized] if (CloseTransientFile(fd) != 0)

Re: Support run-time partition pruning for hash join

2023-11-05 Thread Richard Guo
On Sat, Nov 4, 2023 at 6:00 PM Alexander Lakhin wrote: > 02.11.2023 14:19, Richard Guo wrote: > > However, the cfbot indicates that there are test cases that fail on > FreeBSD [1] (no failure on other platforms). So I set up a FreeBSD-13 > locally but just cannot reproduce the f

Re: Fix bogus Asserts in calc_non_nestloop_required_outer

2023-11-03 Thread Richard Guo
On Thu, Sep 7, 2023 at 11:09 PM Aleksander Alekseev < aleksan...@timescale.com> wrote: > Probably it's just because of my limited experience with the optimizer > but I don't find the proposed change particularly straightforward. I > would suggest adding a comment before the Assert's and/or a

Re: Fix a wrong comment in setrefs.c

2023-11-03 Thread Richard Guo
On Tue, Sep 26, 2023 at 9:51 AM Richard Guo wrote: > On Tue, Sep 26, 2023 at 5:45 AM Tom Lane wrote: > >> I'm inclined to write the comment more like "Usually the equal() >> check is redundant, but in setop plans it may not be, since >> prepunion.c assigns ressor

Re: Why is DEFAULT_FDW_TUPLE_COST so insanely low?

2023-11-02 Thread Richard Guo
On Thu, Nov 2, 2023 at 3:19 PM David Rowley wrote: > I'm not yet seeing any failures in the buildfarm, so don't really want > to push a fix for this one if there are going to be a few more > unstable ones to fix. I may just hold off a while to see. It seems that the test is still not stable

Re: Support run-time partition pruning for hash join

2023-11-02 Thread Richard Guo
On Tue, Aug 29, 2023 at 6:41 PM Richard Guo wrote: > So it seems that the new costing logic is quite crude and tends to be > very conservative, but it can help avoid the large overhead in the worst > cases. I think this might be a good start to push this patch forward. > &g

Re: Support "Right Semi Join" plan shapes

2023-10-31 Thread Richard Guo
On Thu, Aug 10, 2023 at 3:24 PM Richard Guo wrote: > The cfbot reminds that this patch does not apply any more, so rebase it > to v2. > Attached is another rebase over the latest master. Any feedback is appreciated. Thanks Richard v3-0001-Support-Right-Semi-Join-plan-sha

Re: A performance issue with Memoize

2023-10-31 Thread Richard Guo
On Tue, Oct 31, 2023 at 1:36 PM Andrei Lepikhov wrote: > On 30/10/2023 14:55, Richard Guo wrote: > > > > On Thu, Oct 26, 2023 at 12:07 PM Andrei Lepikhov > > mailto:a.lepik...@postgrespro.ru>> wrote: > > > > Do you've thought about the case, fixed wi

Re: Not deleted mentions of the cleared path

2023-10-30 Thread Richard Guo
On Mon, Oct 30, 2023 at 7:31 PM Alena Rybakina wrote: > I have already written about the problem of InvalidPath [0] appearing. I > investigated this and found an error in the add_path() function, when we > reject a path, we free up the memory of the path, but do not delete various > mentions of

Re: A performance issue with Memoize

2023-10-30 Thread Richard Guo
On Thu, Oct 26, 2023 at 12:07 PM Andrei Lepikhov wrote: > Do you've thought about the case, fixed with the commit 1db5667? As I > see, that bugfix still isn't covered by regression tests. Could your > approach of a PARAM_EXEC slot reusing break that case? Hm, I don't think so. The issue fixed

Re: Simplify create_merge_append_path a bit for clarity

2023-10-25 Thread Richard Guo
On Tue, Oct 24, 2023 at 6:00 PM Alena Rybakina wrote: > I agree with you, and we can indeed directly set the param_info value to > NULL, and there are enough comments here to explain. > > I didn't find anything else to add in your patch. Thanks for reviewing this patch! Thanks Richard

Re: A performance issue with Memoize

2023-10-25 Thread Richard Guo
On Fri, Oct 20, 2023 at 7:43 PM Pavel Stehule wrote: > +1 > > it would be great to fix this problem - I've seen this issue a few times. > Thanks for the input. I guess this is not rare in the real world. If the subquery contains lateral reference to a Var that also appears in the subquery's

Re: A performance issue with Memoize

2023-10-25 Thread Richard Guo
On Fri, Oct 20, 2023 at 6:40 PM Richard Guo wrote: > I haven't thought thoroughly about the fix yet. But one way I'm > thinking is that in create_subqueryscan_plan() we can first add the > subquery's subplan_params to root->curOuterParams, and then replace > outer-relation Vars

A performance issue with Memoize

2023-10-20 Thread Richard Guo
I noticed $subject with the query below. set enable_memoize to off; explain (analyze, costs off) select * from tenk1 t1 left join lateral (select t1.two as t1two, * from tenk1 t2 offset 0) s on t1.two = s.two; QUERY PLAN

Re: boolin comment not moved when code was refactored

2023-10-18 Thread Richard Guo
On Thu, Oct 19, 2023 at 10:35 AM Peter Smith wrote: > Hi. > > I happened upon a function comment referring to non-existent code > (that code was moved to another location many years ago). > > Probably better to move that comment too. Thoughts? Agreed. +1 to move that comment. Thanks Richard

Re: Oversight in reparameterize_path_by_child leading to executor crash

2023-10-18 Thread Richard Guo
On Fri, Oct 13, 2023 at 6:18 PM Andrei Lepikhov wrote: > On 23/8/2023 12:37, Richard Guo wrote: > > To fix it we may need to modify RelOptInfos for Path, BitmapHeapPath, > > ForeignPath and CustomPath, and modify IndexOptInfos for IndexPath. It > > seems that that is no

Re: Check each of base restriction clauses for constant-FALSE-or-NULL

2023-10-15 Thread Richard Guo
On Thu, Oct 12, 2023 at 1:09 AM Tom Lane wrote: > Pushed after a bit of fiddling with the comment. Thanks for pushing! Thanks Richard

Re: Retire has_multiple_baserels()

2023-10-10 Thread Richard Guo
On Wed, Oct 11, 2023 at 1:13 AM Tom Lane wrote: > I thought this test wasn't too complete, because has_multiple_baserels > isn't reached at all in many cases thanks to the way the calling if() > is coded. I tried testing like this instead: > > diff --git a/src/backend/optimizer/path/allpaths.c

Re: Fix typo in psql zh_CN.po

2023-10-10 Thread Richard Guo
On Wed, Oct 11, 2023 at 4:30 AM jinser wrote: > Hi, > I found a typo here while using psql. I think this should be a trivial > patch. > The typo is that there is an extra `l` before `列出所有事件触发器`. +1. Thanks Richard

Re: Retire has_multiple_baserels()

2023-10-10 Thread Richard Guo
On Tue, Oct 10, 2023 at 5:43 PM Aleksander Alekseev < aleksan...@timescale.com> wrote: > I used the following patch to double check that nothing was missed: > > ``` > --- a/src/backend/optimizer/path/allpaths.c > +++ b/src/backend/optimizer/path/allpaths.c > @@ -2207,8 +2207,13 @@

Re: Check each of base restriction clauses for constant-FALSE-or-NULL

2023-10-10 Thread Richard Guo
On Tue, Oct 10, 2023 at 5:10 PM David Rowley wrote: > On Sat, 7 Oct 2023 at 22:44, Richard Guo wrote: > > > > In relation_excluded_by_constraints() when we're trying to figure out > > whether the relation need not be scanned, one of the checks we do is to > > de

Re: Check each of base restriction clauses for constant-FALSE-or-NULL

2023-10-10 Thread Richard Guo
On Tue, Oct 10, 2023 at 1:45 PM Ashutosh Bapat wrote: > On Tue, Oct 10, 2023 at 11:09 AM Richard Guo > wrote: > > Hm, I don't think so. get_gating_quals is called in createplan.c, where > > we've selected the best path, while the optimization with my code > > happens m

Retire has_multiple_baserels()

2023-10-10 Thread Richard Guo
The function has_multiple_baserels() is used in set_subquery_pathlist() to check and see if there are more than 1 base rel, by looping through simple_rel_array[]. I think one simpler way to do that is to leverage root->all_baserels by bms_membership(root->all_baserels) == BMS_MULTIPLE

Re: Crash in add_paths_to_append_rel

2023-10-09 Thread Richard Guo
On Tue, Oct 10, 2023 at 11:52 AM David Rowley wrote: > On Mon, 9 Oct 2023 at 22:49, Richard Guo wrote: > > I came across a crash in add_paths_to_append_rel() with the query below. > > > It was introduced by commit a8a968a8 where we referenced > > cheapest_startup_path-&

Re: Crash in add_paths_to_append_rel

2023-10-09 Thread Richard Guo
On Mon, Oct 9, 2023 at 7:35 PM David Rowley wrote: > Since you've managed to attribute this to a specific commit, for the > future, I think instead of opening a new thread, it would be more > useful to communicate the issue on the thread that's linked in the > commit message. Ah, yes, I should

<    1   2   3   4   5   6   7   >