On Wed, Apr 30, 2025 at 4:34 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
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.
I didn't have any better ideas and picked the name you proposed. Then I renamed ChangeVarNodes_callback_type to just ChangeVarNodes_callback. Now it seems consitent with other type names like ChangeVarNodes_context (which is not ChangeVarNodes_context_type).
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.
I've added a couple sentences to ChangeVarNodesExtended().
3. Should the ChangeVarNodesWalkExpression function return the walker's returning value?
