From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: luis.roberto@siscobra.com.br, Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: 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: Tue, 17 Nov 2020 05:23:46 +0100
Message-ID: <a833e667-333d-580c-a600-b01ced234184@enterprisedb.com> (raw)
In-Reply-To: <991884329.69403168.1605565987239.JavaMail.zimbra@siscobra.com.br>
References: <CAAaqYe8_Y7Ydfwdr_q0NWCpr+E_Hc_e=ScEhi9MUmuLT45cNDA@mail.gmail.com>
<CAAaqYe8e9A9SC1BgWPgzyL+v8uC-=y2+yQ0z6CTXBTy1gxjvyg@mail.gmail.com>
<20201029220652.glf4xk2p7wkfisb6@development>
<CAAaqYe9P261NfjqhAkDbPGEjqf_8ZyayxTYUEdEnPOqmLRb6EA@mail.gmail.com>
<20201030210316.cbwv4arwp4lcfgx2@development>
<CAAaqYe_izhFTq1FLgUxa+J+_bY8FGBNiHjTj=EzrSA72tvacEw@mail.gmail.com>
<20201103023743.sswkl6h74dkbcfmp@development>
<20201103213942.sj3qohewdshrw4oz@development>
<991884329.69403168.1605565987239.JavaMail.zimbra@siscobra.com.br>
Hmm, I missed that other thread. That indeed seems like a bug in the
same area already tweaked by ebb7ae839d033d0f2 for similar cases.
The attached patch fixes this simply by adding is_parallel_safe to
get_useful_pathkeys_for_relation - that does fix the reproducer, but I'm
not entirely sure that's the right place. Maybe it should be done in
find_em_expr_usable_for_sorting_rel (which might make a difference if an
EC clause can contain a mix of parallel safe and unsafe expressions). Or
maybe we should do it in the caller (which would allow using
get_useful_pathkeys_for_relation in contexts not requiring parallel
safety in the future).
Anyway, while this is not an "incremental sort" bug, it seems like the
root cause is the same as for ebb7ae839d033d0f2 - one of the incremental
sort patches started considering sorting below gather nodes, not
realizing these possible consequences.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
[text/x-patch] incremental-sort-fix-parallel-safe.patch (1.0K, ../a833e667-333d-580c-a600-b01ced234184@enterprisedb.com/2-incremental-sort-fix-parallel-safe.patch)
download | inline diff:diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.cindex 84a69b064a..93db261011 100644--- a/src/backend/optimizer/path/allpaths.c+++ b/src/backend/optimizer/path/allpaths.c@@ -2826,6 +2826,7 @@ get_useful_pathkeys_for_relation(PlannerInfo *root, RelOptInfo *rel)
foreach(lc, root->query_pathkeys)
{
+ Expr *expr;
PathKey *pathkey = (PathKey *) lfirst(lc);
EquivalenceClass *pathkey_ec = pathkey->pk_eclass;
@@ -2840,7 +2841,14 @@ get_useful_pathkeys_for_relation(PlannerInfo *root, RelOptInfo *rel)
* enable not just an incremental sort on the entirety of
* query_pathkeys but also incremental sort below a JOIN.
*/
- if (!find_em_expr_usable_for_sorting_rel(pathkey_ec, rel))+ if (!(expr = find_em_expr_usable_for_sorting_rel(pathkey_ec, rel)))+ break;++ /*+ * Also stop when the expression is not parallel-safe. We plan+ * to add all of this under a Gather node.+ */+ if (!is_parallel_safe(root, (Node *) expr))
break;
npathkeys++;
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@enterprisedb.com, tomas.vondra@2ndquadrant.com, jtc331@gmail.com, jaime.casanova@2ndquadrant.com
Subject: Re: enable_incremental_sort changes query behavior
In-Reply-To: <a833e667-333d-580c-a600-b01ced234184@enterprisedb.com>
* 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