Re: [PATCH] Teach planner to further optimize sort in distinct - Mailing list pgsql-hackers

> 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