On Fri, Oct 3, 2025 at 4:05 PM Richard Guo <guofenglinux@gmail.com> wrote:
> Sorry, I haven't had a chance to review it yet, but it's on my to-do
> list. I'll get to it as soon as I can.
I've had some time to review this patch, but I have a few concerns.
From the changes in the test cases, it seems that this patch
encourages using MergeAppend+Sort over Sort+Append. However, I'm not
sure MergeAppend+Sort is always more efficient than Sort+Append.
While it is in some cases, there are likely cases where it isn't.
I also noticed the hack you added to avoid using MergeAppend+Sort when
none of the chosen subpaths are ordered. It seems to me that this
contradicts the idea of this patch. If MergeAppend+Sort is indeed a
better plan, why wouldn't it apply in cases where no chosen subpaths
are ordered?
For example, imagine a table with 1000 child tables, where only one
child has a chosen subpath that is ordered, and the other 999 do not.
In this case, this patch would consider using MergeAppend+Sort, but I
don't think there's much practical difference between this case and
one where none of the chosen subpaths are ordered.
Moreover, I think this hack may cause us to miss some paths that the
current master is able to explore. When child pathkeys exist, the
master can generate MergeAppend paths. However, with the hack in
this patch, if none of the chosen subpaths for the child tables are
ordered, the MergeAppend paths will be missed. I think this is a
regression.
Regarding the code, for the newly added function
get_cheapest_path_for_pathkeys_ext(), I think it's a reasonable
expectation from the function name that the returned path satisfies
the given pathkeys. However, this function can return a path that is
not ordered according to those pathkeys, which I think is not a good
design choice.
Also, I'm not sure about this coding style:
+ if (path == NULL)
+
+ /*
+ * Current pathlist doesn't fit the pathkeys. No need to check extra
+ * sort path ways.
+ */
+ return base_path;
On one hand, I don't see this style often in our codebase. On the
other hand, I have noticed commits that try to fix this style by
adding braces (cf. commit aadf7db66). So I wonder if we can avoid
this style altogether from the start.
The commit message states that "To arrange the cost model, change the
merge cost multiplier". However, I didn't find any related changes in
the patch. Am I missing something? Additionally, if you did change
some cost model multiplier, I think it's better to support this change
with benchmark results.
- Richard
