Subject: Re: enable_incremental_sort changes query behavior
To: luis.roberto@siscobra.com.br, Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: James Coleman <jtc331@gmail.com>,
 Jaime Casanova <jaime.casanova@2ndquadrant.com>,
 Pg Hackers <pgsql-hackers@postgresql.org>
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>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
Message-ID: <a833e667-333d-580c-a600-b01ced234184@enterprisedb.com>
Date: Tue, 17 Nov 2020 05:23:46 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.0
MIME-Version: 1.0
In-Reply-To: 
 <991884329.69403168.1605565987239.JavaMail.zimbra@siscobra.com.br>
Content-Type: multipart/mixed;
 boundary="------------E3B5FF6AABA2B4FDE5D80D87"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------E3B5FF6AABA2B4FDE5D80D87
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit

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

--------------E3B5FF6AABA2B4FDE5D80D87
Content-Type: text/x-patch; charset=UTF-8;
 name="incremental-sort-fix-parallel-safe.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="incremental-sort-fix-parallel-safe.patch"

diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 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++;

--------------E3B5FF6AABA2B4FDE5D80D87--