Re: enable_incremental_sort changes query behavior

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-11-03 02:37:43
Message-ID: 20201103023743.sswkl6h74dkbcfmp@development
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Fri, Oct 30, 2020 at 07:37:33PM -0400, James Coleman wrote:
> ...
>I was looking at this some more, and I'm still convinced that this is
>correct, but I don't think a comment about it being an optimization
>(though I suspect that that its major benefit), since it came from,
>and must parallel, find_ec_member_for_tle, and there is no such
>explanation there. I don't want to backfill reasoning onto that
>decision, but I do think it's important to parallel it since it's
>ultimately what is going to cause errors if we're out of sync with it.
>As to why find_em_expr_for_rel is different, I think the answer is
>that it's used by the FDW code, and it seems to me to make sense that
>in that case we'd only send sort conditions to the foreign server if
>the em actually contains rels.
>The new series attached includes the primary fix for this issue, the
>additional comments that could either go in at the same time or not,
>and my patch with semi-related questions (which isn't intended to be
>committed, but to keep track of the questions).

Thanks. Attached are two patches that I plan to get committed

0001 is what you sent, with slightly reworded commit message. This is
the actual fix.

I'm still thinking about Robert's comment that perhaps we should be
considering only expressions already in the relation's target, which
would imply checking for volatile expressions is not enough. But I've
been unable to convince myself it's correct or incorrect. In any case
0001 is a clear improvement - in the worst case we'll have to make it
stricter in the future.

0002 partially reverts ba3e76cc571 by moving find_em_expr_for_rel back
to postgres_fdw.c. We've moved it to equivclass.c so that it could be
called from both the FDW and allpaths.c, but now that the functions
diverged it's only called from the FDW again. So maybe we should move
it back, but I guess that's not a good thing in a minor release.


Tomas Vondra
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachment Content-Type Size
v7-0001-Fix-get_useful_pathkeys_for_relation-for-volatile-ex.patch text/plain 12.5 KB
v7-0002-partially-revert-ba3e76cc571eba3dea19c9465ff15ac3ac1.patch text/plain 3.3 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Tomas Vondra 2020-11-03 02:53:53 Re: Use of "long" in incremental sort code
Previous Message David Rowley 2020-11-03 02:15:27 Re: Split copy.c