From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: James Coleman <jtc331@gmail.com>
Cc: Jaime Casanova <jaime.casanova@2ndquadrant.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: enable_incremental_sort changes query behavior
Date: Fri, 30 Oct 2020 22:03:16 +0100
Message-ID: <20201030210316.cbwv4arwp4lcfgx2@development> (raw)
In-Reply-To: <CAAaqYe9P261NfjqhAkDbPGEjqf_8ZyayxTYUEdEnPOqmLRb6EA@mail.gmail.com>
References: <20201003214440.fe4674dce5q7z4bg@development>
<CAAaqYe9sBisTpRKXqOzgftPswVTp4-mrjmvOBZczcmOEoODCgw@mail.gmail.com>
<CAAaqYe-X57F0Y=BL_wm0OjPGsO-Ls0h_xAJKgoybx8LGnrK1-Q@mail.gmail.com>
<CAAaqYe8_Y7Ydfwdr_q0NWCpr+E_Hc_e=ScEhi9MUmuLT45cNDA@mail.gmail.com>
<CAAaqYe9C3f6A_tZCRfr9Dm7hPpgGwpp4i-K_=NS9GWXuNiFANg@mail.gmail.com>
<CAAaqYe80-V8q7PozVh22EbBhMDU40W286na3RahxJiZ-Pz8S2w@mail.gmail.com>
<CAJGNTeNjGZYFBjFv055aFgkeAurAOVVe2xp7XB1V4Qd3de8Fuw@mail.gmail.com>
<CAAaqYe8e9A9SC1BgWPgzyL+v8uC-=y2+yQ0z6CTXBTy1gxjvyg@mail.gmail.com>
<20201029220652.glf4xk2p7wkfisb6@development>
<CAAaqYe9P261NfjqhAkDbPGEjqf_8ZyayxTYUEdEnPOqmLRb6EA@mail.gmail.com>
On Fri, Oct 30, 2020 at 01:26:08PM -0400, James Coleman wrote:
>On Thu, Oct 29, 2020 at 6:06 PM Tomas Vondra
><tomas.vondra@2ndquadrant.com> wrote:
>>
>> 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.casanova@2ndquadrant.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.
>
>Seems fine. Do you want to commit them at the same time (just a
>separate commit)? Or have a separate patch? It seems a bit overkill to
>start a new thread just for those.
>
Probably sometime later, as a separate patch. I haven't thought very
much about those comments, it just seemed unrelated to the fix so I've
removed that for now. Let's not bother with this until after the minor
release.
>> (b) Fixes comment in expected output of incremental_sort test.
>
>Thanks.
>
>> (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.
>
>No objection.
>
>> 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.
>
>That comes from find_ec_member_for_tle which is called by
>prepare_sort_from_pathkeys which we note in the comments contains the
>set of rules we need to mirror.
>
Thanks for the pointer. I'll take a look.
>> (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?
>
>I definitely remember intentionally removing that condition. For one,
>it's not present in prepare_sort_from_pathkeys. My memory is a bit
>fuzzy on all of the whys, but isn't it the case that if we've already
>excluded const expressions, that we must have vars (and therefore
>rels) present, and therefore the relids bms must be non-empty?
>Probably more importantly, we're going to check that an actual em expr
>matches, which means the bms subset check is really just an
>optimization to avoid unnecessarily scanning the exprs for equality.
>Perhaps it's worth adding a comment saying as such?
>
>By the way, the fact that this is parallel to
>prepare_sort_from_pathkeys is the reason the XXX comment is still
>there about possibly needing to remove relabel types (since
>prepare_sort_from_pathkeys does that). I don't think it's a hard
>requirement: the worst case is that by not digging into relabel types
>we're being unnecessarily strict.
>
>Both of these I can add if desired.
>
Yeah, it'd be good to explain the reasoning why it's fine to have the
conditions like this. Thanks.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tomas.vondra@2ndquadrant.com, jtc331@gmail.com, jaime.casanova@2ndquadrant.com
Subject: Re: enable_incremental_sort changes query behavior
In-Reply-To: <20201030210316.cbwv4arwp4lcfgx2@development>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox