Re: Some problems regarding the self-join elimination code

On 4/30/25 13:22, Alexander Korotkov wrote:
>> Thank you, Andrei.  I've put it all together.
>> 0001 Fixes material bugs in ChangeVarNodes_walker() including regression test
>> 0002 Puts back comments which got accidentally removed
>> 0003 Refactors ChangeVarNodesExtended() with custom user-defined callback
> I've revised the remaining refactoring patch.  I've made a callback an
> additional callback, but not the replacement to the walker.  It looks
> better for me now.  Also, I've written some comments and the commit
> message.  Any thoughts?
It seems quite elegant to me.
I have not precisely examined the part with the RestrictInfo replacement 
logic - I guess you just copied it - I reviewed the rest of the patch.

Generally, it looks good, but let me be a little picky.
1. Looking into the callback-related code, I suggest changing the name 
of ChangeVarNodes_callback to something less general and more specific - 
like replace_relid_callback. My variant doesn't seem the best, but 
general-purposed name seems worse.
2. This callback doesn't modify query replacement logic's behaviour- it 
only affects expressions. It makes sense to write about that in the 
description of the ChangeVarNodesExtended.
3. Should the ChangeVarNodesWalkExpression function return the walker's 
returning value?

-- 
regards, Andrei Lepikhov