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
