Re: Some problems regarding the self-join elimination code

Hi, Andrei!

Thank you for your review!

On Wed, Apr 30, 2025 at 4:34 PM Andrei Lepikhov <lepihov(at)gmail(dot)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