From: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
---|---|
To: | James Coleman <jtc331(at)gmail(dot)com> |
Cc: | Jaime Casanova <jaime(dot)casanova(at)2ndquadrant(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: enable_incremental_sort changes query behavior |
Date: | 2020-10-29 22:06:52 |
Message-ID: | 20201029220652.glf4xk2p7wkfisb6@development |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Oct 06, 2020 at 09:37:31AM -0400, James Coleman wrote:
>On Tue, Oct 6, 2020 at 9:28 AM Jaime Casanova
><jaime(dot)casanova(at)2ndquadrant(dot)com> wrote:
>> Can you please create an entry in the commitfest for this one so we
>> don't lose track of it?
>
We're not too far from the next minor release, so I've been looking at
this fix again and I intend to get it committed shortly (on Monday or
so). Attached is a slightly modified version of the v4 patch that:
(a) Removes comments about projections added to code that is not
directly related to the fix. I'm not against adding such comments
separately.
(b) Fixes comment in expected output of incremental_sort test.
(c) Removes else branch from find_em_expr_usable_for_sorting_rel. It's
not quite needed thanks to the "return" in the "if" branch. IMO this
makes it more elegant.
I do have two questions about find_em_expr_usable_for_sorting_rel.
(a) Where does the em_is_const check come from? The comment claims we
should not be trying to sort by equivalence class members, but I can't
convince myself it's actually true and I don't see it discussed in this
thread.
(b) In find_em_expr_for_rel, which was what was used before, the
condition on relids was this:
if (bms_is_subset(em->em_relids, rel->relids) &&
!bms_is_empty(em->em_relids))
{
return em->em_expr;
}
but here we're using only
if (!bms_is_subset(em->em_relids, rel->relids))
continue;
Isn't this missing the second bms_is_empty condition?
Of course, an alternative to this fix would be reverting ba3e76cc571
(completely or just the part introducing generate_useful_gather_paths).
If anyone thinks that's what we should do, please speak now.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment | Content-Type | Size |
---|---|---|
v5-0001-Fix-get_useful_pathkeys_for_relation-for-volatile.patch | text/plain | 11.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Daniel Gustafsson | 2020-10-29 22:48:57 | contrib/sslinfo cleanup and OpenSSL errorhandling |
Previous Message | Victor Yegorov | 2020-10-29 22:05:28 | Re: Deleting older versions in unique indexes to avoid page splits |