Обсуждение: Re: Pathify RHS unique-ification for semijoin planning

On Tue, Jul 1, 2025 at 11:57 AM Richard Guo <guofenglinux@gmail.com> wrote:
> On Tue, Jun 3, 2025 at 4:52 PM Richard Guo <guofenglinux@gmail.com> wrote:
> > Here is an updated version of the patch, which is ready for review.
> > I've fixed a cost estimation issue, improved some comments, and added
> > a commit message.  Nothing essential has changed.

> This patch does not apply anymore, and here is a new rebase.

This patch does not apply again, so here is a new rebase.

This version also fixes an issue related to parameterized paths: if
the RHS has LATERAL references to the LHS, unique-ification becomes
meaningless because the RHS depends on the LHS, and such paths should
not be generated.

Thanks
Richard

On Thu, Jul 3, 2025 at 7:06 PM Richard Guo <guofenglinux@gmail.com> wrote:
> This patch does not apply again, so here is a new rebase.
>
> This version also fixes an issue related to parameterized paths: if
> the RHS has LATERAL references to the LHS, unique-ification becomes
> meaningless because the RHS depends on the LHS, and such paths should
> not be generated.

(The cc list is somehow lost; re-ccing.)

FWIW, I noticed that the row/cost estimates for the unique-ification
node on master can be very wrong.  For example:

create table t(a int, b int);
insert into t select i%100, i from generate_series(1,10000)i;
vacuum analyze t;
set enable_hashagg to off;

explain (costs on)
select * from t t1, t t2 where (t1.a, t2.b) in
    (select a, b from t t3 where t1.b is not null offset 0);

And look at the snippet from the plan:

(on master)
->  Unique  (cost=934.39..1009.39 rows=10000 width=8)
      ->  Sort  (cost=271.41..271.54 rows=50 width=8)
            Sort Key: "ANY_subquery".a, "ANY_subquery".b
            ->  Subquery Scan on "ANY_subquery"  (cost=0.00..270.00
rows=50 width=8)

The row estimate for the subpath is 50, but it increases to 10000
after unique-ification.  How does that make sense?

This issue does not occur with this patch:

(on patched)
->  Unique  (cost=271.41..271.79 rows=50 width=8)
      ->  Sort  (cost=271.41..271.54 rows=50 width=8)
            Sort Key: "ANY_subquery".a, "ANY_subquery".b
            ->  Subquery Scan on "ANY_subquery"  (cost=0.00..270.00
rows=50 width=8)

Thanks
Richard

On Fri, Jul 4, 2025 at 10:41 AM Richard Guo <guofenglinux@gmail.com> wrote:
> On Thu, Jul 3, 2025 at 7:06 PM Richard Guo <guofenglinux@gmail.com> wrote:
> > This patch does not apply again, so here is a new rebase.
> >
> > This version also fixes an issue related to parameterized paths: if
> > the RHS has LATERAL references to the LHS, unique-ification becomes
> > meaningless because the RHS depends on the LHS, and such paths should
> > not be generated.

> (The cc list is somehow lost; re-ccing.)

The CI reports a test failure related to this patch, although I'm
quite confident it's unrelated to the changes introduced here.

The failure is: recovery/009_twophase time out (After 1000 seconds)

In any case, here's a freshly rebased version.

Hi Tom, I wonder if you've had a chance to look at this patch.  It
would be great to have your input.

Thanks
Richard

Hello,

As a very trivial test on this patch, I ran the query in your opening
email, both with and without the patch, scaling up the size of the table
a little bit.  So I did this

    drop table if exists t;
    create table t(a int, b int);
    insert into t select i % 100000, i from generate_series(1,1e7) i;
    create index on t(a);
    vacuum analyze t;

    set enable_hashagg to off;

    explain (costs off, analyze, buffers)
    select * from t t1 where t1.a in
      (select a from t t2 where a < 10000)
    order by t1.a;


This is the plan without the patch:

                                                    QUERY PLAN                                                     
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────
 Merge Join (actual time=289.262..700.761 rows=1000000.00 loops=1)
   Merge Cond: (t1.a = t2.a)
   Buffers: shared hit=1017728 read=3945 written=3361, temp read=1471 written=1476
   ->  Index Scan using t_a_idx on t t1 (actual time=0.011..320.747 rows=1000001.00 loops=1)
         Index Searches: 1
         Buffers: shared hit=997725 read=3112 written=2664
   ->  Sort (actual time=219.273..219.771 rows=10000.00 loops=1)
         Sort Key: t2.a
         Sort Method: quicksort  Memory: 385kB
         Buffers: shared hit=20003 read=833 written=697, temp read=1471 written=1476
         ->  Unique (actual time=128.173..218.708 rows=10000.00 loops=1)
               Buffers: shared hit=20003 read=833 written=697, temp read=1471 written=1476
               ->  Sort (actual time=128.170..185.461 rows=1000000.00 loops=1)
                     Sort Key: t2.a
                     Sort Method: external merge  Disk: 11768kB
                     Buffers: shared hit=20003 read=833 written=697, temp read=1471 written=1476
                     ->  Index Only Scan using t_a_idx on t t2 (actual time=0.024..74.171 rows=1000000.00 loops=1)
                           Index Cond: (a < 10000)
                           Heap Fetches: 0
                           Index Searches: 1
                           Buffers: shared hit=20003 read=833 written=697
 Planning:
   Buffers: shared hit=28 read=7
 Planning Time: 0.212 ms
 Execution Time: 732.840 ms


and this is the plan with the patch:

                                              QUERY PLAN                                               
───────────────────────────────────────────────────────────────────────────────────────────────────────
 Merge Join (actual time=70.310..595.116 rows=1000000.00 loops=1)
   Merge Cond: (t1.a = t2.a)
   Buffers: shared hit=1017750 read=3923 written=3586
   ->  Index Scan using t_a_idx on t t1 (actual time=0.020..341.257 rows=1000001.00 loops=1)
         Index Searches: 1
         Buffers: shared hit=996914 read=3923 written=3586
   ->  Unique (actual time=0.028..99.074 rows=10000.00 loops=1)
         Buffers: shared hit=20836
         ->  Index Only Scan using t_a_idx on t t2 (actual time=0.026..66.219 rows=1000000.00 loops=1)
               Index Cond: (a < 10000)
               Heap Fetches: 0
               Index Searches: 1
               Buffers: shared hit=20836
 Planning:
   Buffers: shared hit=55 read=15 written=14
 Planning Time: 0.391 ms
 Execution Time: 621.377 ms


This is a really nice improvement.  I think we could find queries that
are arbitrarily faster, by feeding enough tuples to the unnecessary Sort
nodes.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"No necesitamos banderas
 No reconocemos fronteras"                  (Jorge González)

On Wed, Jul 23, 2025 at 5:11 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> As a very trivial test on this patch, I ran the query in your opening
> email, both with and without the patch, scaling up the size of the table
> a little bit.

> This is a really nice improvement.  I think we could find queries that
> are arbitrarily faster, by feeding enough tuples to the unnecessary Sort
> nodes.

Thank you for testing this patch!

In addition to eliminating unnecessary sort nodes, this patch also
allows us to exploit pre-sorted paths that aren't necessarily the
cheapest total during the unique-ification step.  It also allows the
use of parallel plans for that step on large tables.  I think we could
also find queries that become faster as a result of these improvements.

Thanks
Richard

On Thu, Jul 31, 2025 at 10:33 AM Alexandra Wang
<alexandra.wang.oss@gmail.com> wrote:
> Thanks for the patch! I applied your patch and played around with it.

Thanks for trying out this patch.

> I have a question about the following test case you added in
> subselect.sql:

> I was under the impression that we wanted Unique on top of a sorted
> node for the inner of the SIMI join. However, the expected output uses
> a HashAgg instead. Is this expected?

Hmm, I don't think we have such expectation that "Sort+Unique" should
be used for the unique-ification step in this query.  This patch
considers both hash-based and sort-based implementations, and lets the
cost comparison determine which one is preferable.  So I wouldn't be
surprised if the planner ends up choosing the hash-based
implementation in the final plan.

> While looking at the code, I also had a question about the following
> changes in costsize.c:
>
> --- a/src/backend/optimizer/path/costsize.c
> +++ b/src/backend/optimizer/path/costsize.c
> @@ -3963,7 +3963,9 @@ final_cost_mergejoin(PlannerInfo *root, MergePath *path,
>   * The whole issue is moot if we are working from a unique-ified outer
>   * input, or if we know we don't need to mark/restore at all.
>   */
> - if (IsA(outer_path, UniquePath) || path->skip_mark_restore)
> + if (IsA(outer_path, UniquePath) ||
> + IsA(outer_path, AggPath) ||
> + path->skip_mark_restore)
>
> and
>
> @@ -4358,7 +4360,7 @@ final_cost_hashjoin(PlannerInfo *root, HashPath *path,
>   * because we avoid contaminating the cache with a value that's wrong for
>   * non-unique-ified paths.
>   */
> - if (IsA(inner_path, UniquePath))
> + if (IsA(inner_path, UniquePath) || IsA(inner_path, AggPath))
>
> I'm curious why AggPath was added in these two cases.

Well, in final_cost_hashjoin() and final_cost_mergejoin(), we have
some special cases when the inner or outer relation has been
unique-ified.  Previously, it was sufficient to check whether the path
was a UniquePath, since both hash-based and sort-based implementations
were represented that way.  However, with this patch, UniquePath now
only represents the sort-based implementation, so we also need to
check for AggPath to account for the hash-based case.

> The second diff looks fine to me. However, the first diff in the
> subselect test happened to be the test that I asked about.
>
> Here's the comparison of the two EXPLAIN ANALYZE results:

> While both runs are fast (28ms vs. 17ms), the plan generated by the v5
> patch is slower in this case.

This is a very interesting observation.  In fact, with the original v5
patch, you can produce both plans by setting enable_hashagg on and
off.

set enable_hashagg to on;
 Incremental Sort  (cost=91.95..277.59 rows=2500 width=16)
                   (actual time=31.960..147.040 rows=90000.00 loops=1)

set enable_hashagg to off;
 Merge Join  (cost=70.14..294.34 rows=2500 width=16)
             (actual time=4.303..71.891 rows=90000.00 loops=1)

This seems to be another case where the planner chooses a suboptimal
plan due to inaccurate cost estimates.

Thanks
Richard

On Thu, Jul 31, 2025 at 1:08 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Thu, Jul 31, 2025 at 10:33 AM Alexandra Wang
> <alexandra.wang.oss@gmail.com> wrote:
> > While looking at the code, I also had a question about the following
> > changes in costsize.c:
> >
> > --- a/src/backend/optimizer/path/costsize.c
> > +++ b/src/backend/optimizer/path/costsize.c
> > @@ -3963,7 +3963,9 @@ final_cost_mergejoin(PlannerInfo *root, MergePath *path,
> >   * The whole issue is moot if we are working from a unique-ified outer
> >   * input, or if we know we don't need to mark/restore at all.
> >   */
> > - if (IsA(outer_path, UniquePath) || path->skip_mark_restore)
> > + if (IsA(outer_path, UniquePath) ||
> > + IsA(outer_path, AggPath) ||
> > + path->skip_mark_restore)
> >
> > and
> >
> > @@ -4358,7 +4360,7 @@ final_cost_hashjoin(PlannerInfo *root, HashPath *path,
> >   * because we avoid contaminating the cache with a value that's wrong for
> >   * non-unique-ified paths.
> >   */
> > - if (IsA(inner_path, UniquePath))
> > + if (IsA(inner_path, UniquePath) || IsA(inner_path, AggPath))
> >
> > I'm curious why AggPath was added in these two cases.

> Well, in final_cost_hashjoin() and final_cost_mergejoin(), we have
> some special cases when the inner or outer relation has been
> unique-ified.  Previously, it was sufficient to check whether the path
> was a UniquePath, since both hash-based and sort-based implementations
> were represented that way.  However, with this patch, UniquePath now
> only represents the sort-based implementation, so we also need to
> check for AggPath to account for the hash-based case.

BTW, maybe a better way to determine whether a relation has been
unique-ified is to check that the nominal jointype is JOIN_INNER and
sjinfo->jointype is JOIN_SEMI, and the relation is exactly the RHS of
the semijoin.  This approach is mentioned in a comment in joinpath.c:

 * Path cost estimation code may need to recognize that it's
 * dealing with such a case --- the combination of nominal jointype INNER
 * with sjinfo->jointype == JOIN_SEMI indicates that.

... but it seems we don't currently apply it in costsize.c.

To be concrete, I'm imagining a check like the following:

#define IS_UNIQUEIFIED_REL(rel, sjinfo, nominal_jointype) \
    ((nominal_jointype) == JOIN_INNER && (sjinfo)->jointype == JOIN_SEMI && \
     bms_equal((sjinfo)->syn_righthand, (rel)->relids))

... and then the check in final_cost_hashjoin() becomes:

    if (IS_UNIQUEIFIED_REL(inner_path->parent, extra->sjinfo,
                           path->jpath.jointype))
    {
        innerbucketsize = 1.0 / virtualbuckets;
        innermcvfreq = 0.0;
    }

Would this be a better approach?  Any thoughts?

Thanks
Richard

On Thu, Jul 31, 2025 at 9:49 PM wenhui qiu <qiuwenhuifx@gmail.com> wrote:

> > This seems to be another case where the planner chooses a suboptimal
> >  plan due to inaccurate cost estimates.

> Agree ,I increased some rows , set enable_hashagg to on and off ,There's no difference in execution time. The
executionplan is the same. 

Yeah, I found that if you increase the total number of rows in the
table from 1000 to 1083 or more, you consistently get the more
efficient plan -- regardless of whether enable_hashagg is on or off.

> > #define IS_UNIQUEIFIED_REL(rel, sjinfo, nominal_jointype) \
>  >   ((nominal_jointype) == JOIN_INNER && (sjinfo)->jointype == JOIN_SEMI && \
> >     bms_equal((sjinfo)->syn_righthand, (rel)->relids))
> >
> >... and then the check in final_cost_hashjoin() becomes:
> >
>  >   if (IS_UNIQUEIFIED_REL(inner_path->parent, extra->sjinfo,
> >                           path->jpath.jointype))
> >    {
> >        innerbucketsize = 1.0 / virtualbuckets;
>  >       innermcvfreq = 0.0;
> >    }
> >
> >Would this be a better approach?  Any thoughts?

> This approach does indeed more accurately capture the fact that the relation has been unique-ified, especially in
caseswhere a semi join has been transformed into an inner join. Compared to the current heuristic checks in costsize.c
thatrely on inner_path->rows, this method is more semantically meaningful and robust. 

The current check in costsize.c relies on the path type, not the path
rows as you mentioned.  However, I agree that the check I proposed is
more robust and extensible: if additional path types are introduced to
represent unique-ification, this check wouldn't need to change.  So I
plan to go this way, unless there are any objections.

Thanks
Richard

On Fri, Aug 1, 2025 at 11:58 PM Alexandra Wang
<alexandra.wang.oss@gmail.com> wrote:
> While reviewing the code, the following diff concerns me:

>   if (joinrel->consider_parallel &&
> - save_jointype != JOIN_UNIQUE_OUTER &&
> - save_jointype != JOIN_FULL &&
> - save_jointype != JOIN_RIGHT &&
> - save_jointype != JOIN_RIGHT_ANTI &&
> + jointype != JOIN_FULL &&
> + jointype != JOIN_RIGHT &&
> + jointype != JOIN_RIGHT_ANTI &&
>   outerrel->partial_pathlist != NIL &&
>   bms_is_empty(joinrel->lateral_relids))
>
> What has changed that enabled JOIN_UNIQUE_OUTER to handle partial
> paths? Or is it no longer possible for the outer path to be partial?

It's the latter, as indicated by the Assert in create_unique_paths():

+       /*
+        * There shouldn't be any partial paths for the unique relation;
+        * otherwise, we won't be able to properly guarantee uniqueness.
+        */
+       Assert(unique_rel->partial_pathlist == NIL);

Thanks
Richard

The v5 patch does not apply anymore, and here is a new rebase.  There
are two main changes in v6:

* I choose to use the check I proposed earlier to determine whether a
relation has been unique-ified in costsize.c.

* Now that the only call to relation_has_unique_index_for() that
supplied an exprlist and oprlist has been removed, the loop handling
those lists is effectively dead code.  0002 removes that loop and
simplifies the function accordingly.

Thanks
Richard

On Thu, Aug 7, 2025 at 6:04 PM wenhui qiu <qiuwenhuifx@gmail.com> wrote:
> In light of this commit (https://github.com/postgres/postgres/commit/e035863c9a04beeecc254c3bfe48dab58e389e10), I
alsorecommend changing the macro to a static inline function. Macros are harder to debug and lack type safety. 

I'm inclined not to do that.  We already have other macros for
checking whether a relation is of a certain kind, and I'd prefer to
keep the new check consistent with those.

Thanks
Richard

On Mon, Aug 4, 2025 at 11:08 AM Richard Guo <guofenglinux@gmail.com> wrote:
> The v5 patch does not apply anymore, and here is a new rebase.  There
> are two main changes in v6:
>
> * I choose to use the check I proposed earlier to determine whether a
> relation has been unique-ified in costsize.c.
>
> * Now that the only call to relation_has_unique_index_for() that
> supplied an exprlist and oprlist has been removed, the loop handling
> those lists is effectively dead code.  0002 removes that loop and
> simplifies the function accordingly.

Does anyone plan to review this patch further?  I intend to push it in
two weeks unless there are any objections or additional comments.

Thanks
Richard

On 2025-Aug-12, Richard Guo wrote:

> Does anyone plan to review this patch further?  I intend to push it in
> two weeks unless there are any objections or additional comments.

No review, but apparently "uniquify" is more widely accepted than
"uniqueify".  No dictionary lists either words AFAICS, except that
Wiktionary lists the former:
https://en.wiktionary.org/wiki/uniquify
but apparently adjectives ending in "-e" are prone to lose it when a
verb is formed from them with "-ify", such as

https://www.merriam-webster.com/dictionary/falsify
https://www.merriam-webster.com/dictionary/intensify
https://www.merriam-webster.com/dictionary/simplify

There aren't many though.  Most in this list don't end in -e:
https://en.wiktionary.org/w/index.php?title=Category:English_terms_suffixed_with_-ify

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"El destino baraja y nosotros jugamos" (A. Schopenhauer)

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> No review, but apparently "uniquify" is more widely accepted than
> "uniqueify".

Personally I'd write "unique-ify", seeing that neither of the forms
without the dash are considered good English.  Of course, if you
need to make identifiers out of this, that solution doesn't work;
but you could just avoid the construction --- say, "make_path_unique"
rather than "uniquify_path".

            regards, tom lane

On Wed, Aug 13, 2025 at 1:38 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> > No review, but apparently "uniquify" is more widely accepted than
> > "uniqueify".

> Personally I'd write "unique-ify", seeing that neither of the forms
> without the dash are considered good English.  Of course, if you
> need to make identifiers out of this, that solution doesn't work;
> but you could just avoid the construction --- say, "make_path_unique"
> rather than "uniquify_path".

Some 'git grep' work shows that, currently on master, we commonly use
the form "unique-ify" (with the dash) and its variants, such as:
unique-ify, unique-ified, unique-ification, and unique-ifying.

$ git grep -in 'unique-if' | wc -l
50

There is one instance of the form "uniquify":
planner.c:5107: * check).  We can uniquify these tuples simply by just taking

And one instance of "uniqueify" (without the dash):
jsonb_util.c:65:static void uniqueifyJsonbObject()

Given this, I'd prefer to stick with "unique-ify", for consistency
with the majority usage in the codebase.

In this patch, the only instance that doesn't follow the "unique-ify"
form is the macro IS_UNIQUEIFIED_REL, as dashes are not allowed in C
identifiers.  Maybe a better alternative is IS_RELATION_UNIQUE?  Any
suggestions?

Thanks
Richard

Richard Guo <guofenglinux@gmail.com> writes:
> Given this, I'd prefer to stick with "unique-ify", for consistency
> with the majority usage in the codebase.

+1.  (Not but what I might've been responsible for many of the
existing usages, so my opinion is perhaps counting twice here.)

> In this patch, the only instance that doesn't follow the "unique-ify"
> form is the macro IS_UNIQUEIFIED_REL, as dashes are not allowed in C
> identifiers.  Maybe a better alternative is IS_RELATION_UNIQUE?  Any
> suggestions?

Hm ... to my ear, "unique-ified" implies that we took some positive
action to make the path's output unique, such as running it through
a hashagg or Unique node.  IS_RELATION_UNIQUE only implies that the
output is unique, so for example a scan of a primary key should
satisfy such a predicate.  Not having read the patch (I do hope
to get to that), I'm not sure which connotation you have in mind.
If it's the latter, IS_RELATION_UNIQUE seems like a fine name.
If it's the former, maybe something like "RELATION_WAS_MADE_UNIQUE"?
That's not very pretty though ...

            regards, tom lane

On Wed, Aug 13, 2025 at 11:27 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Richard Guo <guofenglinux@gmail.com> writes:
> > In this patch, the only instance that doesn't follow the "unique-ify"
> > form is the macro IS_UNIQUEIFIED_REL, as dashes are not allowed in C
> > identifiers.  Maybe a better alternative is IS_RELATION_UNIQUE?  Any
> > suggestions?

> Hm ... to my ear, "unique-ified" implies that we took some positive
> action to make the path's output unique, such as running it through
> a hashagg or Unique node.  IS_RELATION_UNIQUE only implies that the
> output is unique, so for example a scan of a primary key should
> satisfy such a predicate.  Not having read the patch (I do hope
> to get to that), I'm not sure which connotation you have in mind.
> If it's the latter, IS_RELATION_UNIQUE seems like a fine name.
> If it's the former, maybe something like "RELATION_WAS_MADE_UNIQUE"?
> That's not very pretty though ...

It's the former: this macro is to signal that we've explicitly taken
steps to make the output of the relation unique.  IMO, "unique-ified"
best describes this, but we cannot use it directly in the macro name
because of the dash.

Hmm, I think "RELATION_WAS_MADE_UNIQUE" works well because it clearly
conveys that the relation has been explicitly unique-ified.  It's a
bit verbose, but I found that we have similar names in our codebase,
such as VAC_BLK_WAS_EAGER_SCANNED.

Thanks
Richard

On Tue, Aug 12, 2025 at 10:43 AM Richard Guo <guofenglinux@gmail.com> wrote:
> On Mon, Aug 4, 2025 at 11:08 AM Richard Guo <guofenglinux@gmail.com> wrote:
> > The v5 patch does not apply anymore, and here is a new rebase.  There
> > are two main changes in v6:
> >
> > * I choose to use the check I proposed earlier to determine whether a
> > relation has been unique-ified in costsize.c.
> >
> > * Now that the only call to relation_has_unique_index_for() that
> > supplied an exprlist and oprlist has been removed, the loop handling
> > those lists is effectively dead code.  0002 removes that loop and
> > simplifies the function accordingly.

> Does anyone plan to review this patch further?  I intend to push it in
> two weeks unless there are any objections or additional comments.

Here's the updated version of the patch, which renames the macro
IS_UNIQUEIFIED_REL to RELATION_WAS_MADE_UNIQUE, and includes some
comment updates as well.  I plan to push it soon, barring any
objections.

This patch removes the last call to make_sort_from_sortclauses(), so
I'm wondering if we can safely remove the function itself.  Or should
we keep it around in case it's used by extensions or might be needed
in the future?

Thanks
Richard

On Mon, Aug 18, 2025 at 3:07 PM Richard Guo <guofenglinux@gmail.com> wrote:
> Here's the updated version of the patch, which renames the macro
> IS_UNIQUEIFIED_REL to RELATION_WAS_MADE_UNIQUE, and includes some
> comment updates as well.  I plan to push it soon, barring any
> objections.

Pushed.

> This patch removes the last call to make_sort_from_sortclauses(), so
> I'm wondering if we can safely remove the function itself.  Or should
> we keep it around in case it's used by extensions or might be needed
> in the future?

This function, along with two other make_xxx() functions from
createplan.c, was exported in 570be1f73 because CitusDB was using
them.

commit 570be1f73f385abb557bda15b718d7aac616cc15
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   Sat Mar 12 12:12:59 2016 -0500

    Re-export a few of createplan.c's make_xxx() functions.

    CitusDB is using these and don't wish to redesign their code right now.
    I am not on board with this being a good idea, or a good precedent,
    but I lack the energy to fight about it.

I actually agree with Tom that it's not a good idea to create Plan
nodes outside of createplan.c; instead, one should construct a Path
tree and let create_plan() convert it into Plan nodes.

I'm not sure whether CitusDB has redesigned their code in this way,
but for now, I prefer not to remove make_sort_from_sortclauses() just
to be safe.

Thanks
Richard

On Wed, Jul 23, 2025 at 5:11 PM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> As a very trivial test on this patch, I ran the query in your opening
> email, both with and without the patch, scaling up the size of the table
> a little bit.

> This is a really nice improvement.  I think we could find queries that
> are arbitrarily faster, by feeding enough tuples to the unnecessary Sort
> nodes.

FWIW, I'm looking for a query to better showcase the performance
improvement from this patch.  Here is one I found.

create table t (a int, b int);
insert into t select i%10, i%10 from generate_series(1,50000) i;
create index on t (a, b);
analyze t;

explain (analyze, costs on)
select * from t t1, t t2 where (t1.a, t2.b) in (select a, b from t t3)
order by t1.a, t2.b;

Here are the planning and execution time on my snail-paced machine
(best of 3), without and with this patch.

-- without this patch
 Planning Time: 0.850 ms
 Execution Time: 108149.907 ms

-- with this patch
 Planning Time: 0.728 ms
 Execution Time: 29229.748 ms

So this specific case runs about 3.7 times faster, which is really
nice.

- Richard

On 2/9/2025 12:10, Richard Guo wrote:
> So this specific case runs about 3.7 times faster, which is really
> nice.No questions, it is good enough optimisation. I'm worried only about 
implementation: It creates one more RelOptInfo that may look like a 
baserel, but we can't find it by find_base_rel or even find_join_rel. It 
seems a little inconsistent to me.
Don't think it is critical - just complicates life for extension 
developers in some cases.

-- 
regards, Andrei Lepikhov

On Tue, Sep 2, 2025 at 7:56 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
> No questions, it is good enough optimisation. I'm worried only about
> implementation: It creates one more RelOptInfo that may look like a
> baserel, but we can't find it by find_base_rel or even find_join_rel. It
> seems a little inconsistent to me.
> Don't think it is critical - just complicates life for extension
> developers in some cases.

The RelOptInfo representing the unique-ified rel is intended to be
used only internally during path generation for semi-joins, and should
be opaque outside of that.  I don't think extensions should know about
it.

- Richard

On 3/9/2025 11:12, Richard Guo wrote:
> On Tue, Sep 2, 2025 at 7:56 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
>> No questions, it is good enough optimisation. I'm worried only about
>> implementation: It creates one more RelOptInfo that may look like a
>> baserel, but we can't find it by find_base_rel or even find_join_rel. It
>> seems a little inconsistent to me.
>> Don't think it is critical - just complicates life for extension
>> developers in some cases.
> 
> The RelOptInfo representing the unique-ified rel is intended to be
> used only internally during path generation for semi-joins, and should
> be opaque outside of that.  I don't think extensions should know about
> it.
I just stated the fact - it is not for debate ;).
To understand how deeply developers utilise the core, take a look at the 
pg_hint_plan. The extensibility of the Postgres planner isn't flexible 
enough (and will never be) for the developers' purposes. So, they 
exploit every exported function, variable and type.
Every feature intended to be hidden from extensions should be wrapped 
into an internal type, not exposed in .h files.
It leads us to a discussion about the voice of extension developers in 
the core decisions. I think it is worth debating at one of the 
conferences in the near future.

-- 
regards, Andrei Lepikhov