Hi, Andrei! Thank you for your review!
On Wed, Apr 30, 2025 at 4:34 PM Andrei Lepikhov <lepi...@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? > Done. ------ Regards, Alexander Korotkov Supabase
v5-0001-Refactor-ChangeVarNodesExtended-using-the-custom-.patch
Description: Binary data
