On 20/01/23 06:07, David Rowley wrote:
Looking at the patch, you've not added any additional tests. If the
existing tests are all passing then that just tells me that either the
code is not functioning as intended or we have no tests that look at
the EXPLAIN output which can make use of this optimization. If the
former is true, then the patch needs to be fixed. If it's the latter
then you need to write new tests.
I definitely need to add tests because this scenario is missing.
I don't know all the repercussions. If you look at add_path() then
you'll see we do a pathkey comparison when the costs are not fuzzily
different from an existing path so that we try to keep a path with the
best pathkeys. If we start keeping paths around with other weird
pathkeys that are not along the lines of the query_pathkeys requires,
then add_path might start throwing away paths that are actually good
for something. It seems probable that could cause some regressions.
Okay, in that case I think it is better idea to store original pathkeys
(apart from what get assigned by useful_pathkeys). I need to dig deeper for
this.
Does this patch actually work? I tried:
I don't see that you're
adjusting IndexPath's pathkeys anywhere.
I had removed the changes for indexPath (it was in v1) because I hadn't
investigated
repercussions. But I failed to mention this.
The nested loop in
pathkeys_count_contained_in_unordered() seems to be inside out. The
reordered_pathkeys must have the common pathkeys in the order they
appear in keys2. In your patch, they'll be ordered according to the
keys1 list, which is wrong. Testing would tell you this, so all the
more reason to make the patch work and write some queries to ensure it
does actually work, then some tests to ensure that remains in a
working state.
Feel free to take the proper time to write a working patch which
contains new tests to ensure it's functioning as intended. It's
disheartening to review patches that don't seem to work. If it wasn't
meant to work, then you didn't make that clear.
Please don't rush out the next patch. Take your time and study the code
and see if you can build up your own picture for what the repercussions
might be of IndexPaths having additional pathkeys when they were previously empty.
If you're uncertain of aspects of the patch you've written feel free to leave
XXX type comments to indicate this. That way the reviewer will know
you might need more guidance on that and you'll not forget yourself
when you come back and look again after a few weeks.
I deeply regret this. I will be mindful of my patches and ensure that they are
complete by themselves.
Thanks for your pointers as well, I can see errors in my approach which I will
address.
I'll likely not be
able to do any further reviews until the March commitfest, so it might
be better to only post if you're stuck.
Yes sure, I will work on patches and limit posts to discussion only (if
blocked).
Thanks,
Ankit
