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
