Remove unnecessary relabel stripping
Hi hackers,
Per discussion in [1]/messages/by-id/CAMbWs48HF9f=g+jSmmYBnWub9+Wyg5Xh-FoqAnvqAspue5ypAw@mail.gmail.com, we don't need to strip relabel for the expr
explicitly before calling pull_varnos() to retrieve all mentioned
relids. pull_varnos() would recurse into T_RelabelType nodes.
Add a patch to remove that and simplify the code a bit.
[1]: /messages/by-id/CAMbWs48HF9f=g+jSmmYBnWub9+Wyg5Xh-FoqAnvqAspue5ypAw@mail.gmail.com
/messages/by-id/CAMbWs48HF9f=g+jSmmYBnWub9+Wyg5Xh-FoqAnvqAspue5ypAw@mail.gmail.com
Thanks
Richard
Attachments:
v1-0001-Remove-unnecessary-relabel-stripping.patchapplication/octet-stream; name=v1-0001-Remove-unnecessary-relabel-stripping.patchDownload
From 875abbd2f2ed8fd31f30808bbfe7dada0bbd1a5a Mon Sep 17 00:00:00 2001
From: Richard Guo <riguo@pivotal.io>
Date: Wed, 29 Apr 2020 11:32:23 +0000
Subject: [PATCH v1] Remove unnecessary relabel stripping
---
src/backend/optimizer/path/costsize.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index f5f9bae..b10b14a 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -1842,9 +1842,6 @@ cost_incremental_sort(Path *path,
*/
foreach(l, pathkeys)
{
- Node *expr;
- Relids varnos;
-
PathKey *key = (PathKey *) lfirst(l);
EquivalenceMember *member = (EquivalenceMember *)
linitial(key->pk_eclass->ec_members);
@@ -1853,14 +1850,7 @@ cost_incremental_sort(Path *path,
* Check if the expression contains Var with "varno 0" so that we
* don't call estimate_num_groups in that case.
*/
- expr = (Node *) member->em_expr;
-
- if (IsA(expr, RelabelType))
- expr = (Node *) ((RelabelType *) expr)->arg;
-
- varnos = pull_varnos(expr);
-
- if (bms_is_member(0, varnos))
+ if (bms_is_member(0, pull_varnos((Node *) member->em_expr)))
{
unknown_varno = true;
break;
--
2.7.4
On Wed, Apr 29, 2020 at 03:51:40PM +0800, Richard Guo wrote:
Hi hackers,
Per discussion in [1], we don't need to strip relabel for the expr
explicitly before calling pull_varnos() to retrieve all mentioned
relids. pull_varnos() would recurse into T_RelabelType nodes.Add a patch to remove that and simplify the code a bit.
[1]
/messages/by-id/CAMbWs48HF9f=g+jSmmYBnWub9+Wyg5Xh-FoqAnvqAspue5ypAw@mail.gmail.comThanks
Richard
Thanks, I'll get this pushed (or something similar to this patch) soon.
FWIW it'd be better to send the patch to the original thread instead of
starting a new one.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Apr 30, 2020 at 8:11 AM Tomas Vondra <tomas.vondra@2ndquadrant.com>
wrote:
On Wed, Apr 29, 2020 at 03:51:40PM +0800, Richard Guo wrote:
Hi hackers,
Per discussion in [1], we don't need to strip relabel for the expr
explicitly before calling pull_varnos() to retrieve all mentioned
relids. pull_varnos() would recurse into T_RelabelType nodes.Add a patch to remove that and simplify the code a bit.
[1]
/messages/by-id/CAMbWs48HF9f=g+jSmmYBnWub9+Wyg5Xh-FoqAnvqAspue5ypAw@mail.gmail.com
Thanks
RichardThanks, I'll get this pushed (or something similar to this patch) soon.
Thanks.
FWIW it'd be better to send the patch to the original thread instead of
starting a new one.
Ah yes, you're right. Sorry for not doing so.
Thanks
Richard
On Thu, Apr 30, 2020 at 09:37:41AM +0800, Richard Guo wrote:
On Thu, Apr 30, 2020 at 8:11 AM Tomas Vondra <tomas.vondra@2ndquadrant.com>
wrote:>FWIW it'd be better to send the patch to the original thread instead of
starting a new one.Ah yes, you're right. Sorry for not doing so.
FWIW, I don't find the move from Richard completely incorrect either
as the original thread discusses about a crash in incremental sorts
with sqlsmith, and here we have a patch to remove a useless operation.
Different threads pointing to different issues help to attract
sometimes a more correct audience.
--
Michael
On Thu, Apr 30, 2020 at 01:40:11PM +0900, Michael Paquier wrote:
On Thu, Apr 30, 2020 at 09:37:41AM +0800, Richard Guo wrote:
On Thu, Apr 30, 2020 at 8:11 AM Tomas Vondra <tomas.vondra@2ndquadrant.com>
wrote:>FWIW it'd be better to send the patch to the original thread instead of
starting a new one.Ah yes, you're right. Sorry for not doing so.
FWIW, I don't find the move from Richard completely incorrect either
as the original thread discusses about a crash in incremental sorts
with sqlsmith, and here we have a patch to remove a useless operation.
Different threads pointing to different issues help to attract
sometimes a more correct audience.
Possibly. I agree it's not an entirely clear case.
Anyway, I've pushed the fix.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services