Horton

Thread

Commits

  1. Refactor ChangeVarNodesExtended() using the custom callback

  2. Fixes for ChangeVarNodes_walker()

  3. Restore comments in ChangeVarNodesExtended()

  1. Some problems regarding the self-join elimination code

    Richard Guo <guofenglinux@gmail.com> — 2025-04-02T13:26:44Z

    While working on another patch, I looked at ChangeVarNodes() and found
some issues introduced by the self-join elimination commit.  I think
it'd better to fix or improve them.

* ChangeVarNodes_walker includes special handling for RestrictInfo
nodes.  And it adjusts RestrictInfo.orclause only if the rt_index of
the relation to be removed is contained in RestrictInfo.clause_relids.

I don't think this is correct.  Even if the to-be-removed relation is
not present in clause_relids, it can still be found somewhere within
the orclause.  As an example, consider:

create table t (a int primary key, b int, c int);

explain (costs off)
select * from t t1
   inner join t t2 on t1.a = t2.a
    left join t t3 on t1.c = 1 and (t2.b = t3.b or t2.b = 1);
server closed the connection unexpectedly

This query triggers an Assert failure in match_orclause_to_indexcol,
and the root cause is that the removed relation is still present
in the outer_relids of the OR-clauses.

To fix, I think we may need to run ChangeVarNodes_walker on the
orclause in all cases.


* After the adjustment, the num_base_rels is recalculated as:

+      rinfo->num_base_rels = bms_num_members(rinfo->clause_relids);

I don't think this is correct either.  num_base_rels is the number of
base rels in clause_relids and should not count outer-join relids.
And we have no guarantee that clause_relids does not include any
outer-join relids.

To fix, I think we should exclude all outer-join relids.  Perhaps we
can leverage root->outer_join_rels to achieve this.


*  I'm not sure why the self-join elimination commit removed all the
comments in ChangeVarNodes(Extended).  I think these comments are very
helpful for understanding the code.

-       /*
-        * Must be prepared to start with a Query or a bare expression tree; if
-        * it's a Query, go straight to query_tree_walker to make sure that
-        * sublevels_up doesn't get incremented prematurely.
-        */
        if (node && IsA(node, Query))
        {
                Query      *qry = (Query *) node;

-               /*
-                * If we are starting at a Query, and sublevels_up is
zero, then we
-                * must also fix rangetable indexes in the Query
itself --- namely
-                * resultRelation, mergeTargetRelation, exclRelIndex
and rowMarks
-                * entries.  sublevels_up cannot be zero when recursing into a
-                * subquery, so there's no need to have the same logic inside
-                * ChangeVarNodes_walker.
-                */
                if (sublevels_up == 0)
                {
                        ListCell   *l;
@@ -701,7 +772,6 @@ ChangeVarNodes(Node *node, int rt_index, int
new_index, int sublevels_up)
                        if (qry->mergeTargetRelation == rt_index)
                                qry->mergeTargetRelation = new_index;

-                       /* this is unlikely to ever be used, but ... */


* Speaking of comments, I’ve noticed that some changes are lacking
comments.  For example, there are almost no comments regarding the
special handling of RestrictInfo nodes in ChangeVarNodes_walker,
except for an explanation about adding the NullTest.

BTW, there's a typo in that explanation: qual() should be equal().

This is not a thorough review of the code; I just randomly looked at
ChangeVarNodes().  It's possible that there are other issues that
haven't been discovered, but I think we should at least fix these.
I'll provide a patch for the fixes later.  Apologies for being so
nitpicky.

Thanks
Richard

  2. Re: Some problems regarding the self-join elimination code

    Richard Guo <guofenglinux@gmail.com> — 2025-04-04T07:37:06Z

    On Wed, Apr 2, 2025 at 10:26 PM Richard Guo <guofenglinux@gmail.com> wrote:
> This is not a thorough review of the code; I just randomly looked at
> ChangeVarNodes().  It's possible that there are other issues that
> haven't been discovered, but I think we should at least fix these.
> I'll provide a patch for the fixes later.  Apologies for being so
> nitpicky.

With more exposure to the changes in ChangeVarNodes(), I have some
more concerns.  As I understand it, ChangeVarNodes() is designed to
update the varno fields of Var nodes in the given tree that belong to
the specific relation; neat and clear.  However, now, within this
function, we also create new NullTest quals for SJE, which doesn't
seem like something this function should be handling.  It makes this
function look a bit like a kludge.

Actually, while working on the reduce-NullTest-during-constant-folding
patch, I tried to figure out where the new NOT NULL quals from SJE are
added.  I was a bit surprised to find that they are added in
ChangeVarNodes.  I didn't expect this function to have this extra
responsibility.

Additionally, I have a minor suggestion regarding the new boolean
parameter, "change_RangeTblRef".  AFAIU, by convention, we typically
use flags to control the behavior of walker functions, like in
pull_var_clause.  While it's fine to use a boolean parameter here
given that we currently only have one flag, for the sake of future
extensibility, I think using flags might be a better choice.

Any ideas on how we could improve this function?  Apologies again for
being nitpicky.

Thanks
Richard

  3. Re: Some problems regarding the self-join elimination code

    Andrei Lepikhov <lepihov@gmail.com> — 2025-04-08T09:44:32Z

    On 4/2/25 15:26, Richard Guo wrote:
> While working on another patch, I looked at ChangeVarNodes() and found
> some issues introduced by the self-join elimination commit.  I think
> it'd better to fix or improve them.
> 
> * ChangeVarNodes_walker includes special handling for RestrictInfo
> nodes.  And it adjusts RestrictInfo.orclause only if the rt_index of
> the relation to be removed is contained in RestrictInfo.clause_relids.
> 
> I don't think this is correct.  Even if the to-be-removed relation is
> not present in clause_relids, it can still be found somewhere within
> the orclause.  As an example, consider:
Yeah, discovering how we tolerated such a gaffe I found that the comment 
on the clause_relids field is quite vague here:

"The relids (varnos+varnullingrels) actually referenced in the clause:"

and only in the RestrictInfo description you can find some additional 
information. I think we should modify the check, uniting clause_relids 
and required_relids before searching for the removing relid.

> +      rinfo->num_base_rels = bms_num_members(rinfo->clause_relids);
> 
> I don't think this is correct either.  num_base_rels is the number of
> base rels in clause_relids and should not count outer-join relids.
> And we have no guarantee that clause_relids does not include any
> outer-join relids.
It is a clear mistake, no apologies to me.
> 
> To fix, I think we should exclude all outer-join relids.  Perhaps we
> can leverage root->outer_join_rels to achieve this.
I think we may use a difference in size of rinfo->clause_relids before 
and after adjustion. If something were removed, just decrease num_base_rels.

> * Speaking of comments, I’ve noticed that some changes are lacking
> comments.  For example, there are almost no comments regarding the
> special handling of RestrictInfo nodes in ChangeVarNodes_walker,
> except for an explanation about adding the NullTest.
Ok, it seems that comments were removed too hastily. Not addressed it yet.

I also added little code refactoring to make it more readable. See 
fix.diff in attachment as my proposal for future discussion.

-- 
regards, Andrei Lepikhov

  4. Re: Some problems regarding the self-join elimination code

    Andrei Lepikhov <lepihov@gmail.com> — 2025-04-08T11:31:45Z

    On 4/4/25 09:37, Richard Guo wrote:
> On Wed, Apr 2, 2025 at 10:26 PM Richard Guo <guofenglinux@gmail.com> wrote:
> With more exposure to the changes in ChangeVarNodes(), I have some
> more concerns.  As I understand it, ChangeVarNodes() is designed to
> update the varno fields of Var nodes in the given tree that belong to
> the specific relation; neat and clear.  However, now, within this
> function, we also create new NullTest quals for SJE, which doesn't
> seem like something this function should be handling.  It makes this
> function look a bit like a kludge.
To be precise, inside the function we replace old clause with the 
NullTest, because relid replacement action made it degenerate. It seems 
essential to me and I think it may be ok to add a comment at the top of 
ChangeVarNodes, describing this minor optimisation.

> Additionally, I have a minor suggestion regarding the new boolean
> parameter, "change_RangeTblRef".  AFAIU, by convention, we typically
> use flags to control the behavior of walker functions, like in
> pull_var_clause.  While it's fine to use a boolean parameter here
> given that we currently only have one flag, for the sake of future
> extensibility, I think using flags might be a better choice.
That was exactly why I wasn't happy with replacing the change_relid 
routine with ChangeVarNodes.
But variant with a flag seems non-trivial to implement. Do you have any 
proposal about the code?

-- 
regards, Andrei Lepikhov

  5. Re: Some problems regarding the self-join elimination code

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2025-04-08T14:12:14Z

    On Tue, 8 Apr 2025 at 12:31, Andrei Lepikhov <lepihov@gmail.com> wrote:
>
> On 4/4/25 09:37, Richard Guo wrote:
> > On Wed, Apr 2, 2025 at 10:26 PM Richard Guo <guofenglinux@gmail.com> wrote:
> > With more exposure to the changes in ChangeVarNodes(), I have some
> > more concerns.  As I understand it, ChangeVarNodes() is designed to
> > update the varno fields of Var nodes in the given tree that belong to
> > the specific relation; neat and clear.  However, now, within this
> > function, we also create new NullTest quals for SJE, which doesn't
> > seem like something this function should be handling.  It makes this
> > function look a bit like a kludge.
> To be precise, inside the function we replace old clause with the
> NullTest, because relid replacement action made it degenerate. It seems
> essential to me and I think it may be ok to add a comment at the top of
> ChangeVarNodes, describing this minor optimisation.
>

I recall raising the same concerns when I originally reviewed the
patch. I don't think this code belongs in the rewriter, because it's
very specific to how SJE wants to handle these kinds of nodes.

Note also that RestrictInfo is defined in nodes/pathnodes.h, which
describes its contents as internal data structures for the planner,
suggesting that the rewriter has no business processing those kinds of
nodes.

I don't think simply adding a comment addresses those concerns.

> > Additionally, I have a minor suggestion regarding the new boolean
> > parameter, "change_RangeTblRef".  AFAIU, by convention, we typically
> > use flags to control the behavior of walker functions, like in
> > pull_var_clause.  While it's fine to use a boolean parameter here
> > given that we currently only have one flag, for the sake of future
> > extensibility, I think using flags might be a better choice.
> That was exactly why I wasn't happy with replacing the change_relid
> routine with ChangeVarNodes.
> But variant with a flag seems non-trivial to implement. Do you have any
> proposal about the code?
>

Perhaps the way to do it is to make ChangeVarNodesExtended() take a
callback function to be invoked for each node visited. The callback
(which would then be in the planner, with the other SJE code) would do
any special handling it needed (for RangeTblRef and RestrictInfo
nodes), and call ChangeVarNodes_walker() for any other types of node,
to get the default behaviour.

Regards,
Dean

  6. Re: Some problems regarding the self-join elimination code

    Richard Guo <guofenglinux@gmail.com> — 2025-04-09T02:05:00Z

    On Tue, Apr 8, 2025 at 11:12 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> On Tue, 8 Apr 2025 at 12:31, Andrei Lepikhov <lepihov@gmail.com> wrote:
> > On 4/4/25 09:37, Richard Guo wrote:
> > > With more exposure to the changes in ChangeVarNodes(), I have some
> > > more concerns.  As I understand it, ChangeVarNodes() is designed to
> > > update the varno fields of Var nodes in the given tree that belong to
> > > the specific relation; neat and clear.  However, now, within this
> > > function, we also create new NullTest quals for SJE, which doesn't
> > > seem like something this function should be handling.  It makes this
> > > function look a bit like a kludge.

> > To be precise, inside the function we replace old clause with the
> > NullTest, because relid replacement action made it degenerate. It seems
> > essential to me and I think it may be ok to add a comment at the top of
> > ChangeVarNodes, describing this minor optimisation.

> I recall raising the same concerns when I originally reviewed the
> patch. I don't think this code belongs in the rewriter, because it's
> very specific to how SJE wants to handle these kinds of nodes.

Correct.  And I don't think it's this function's responsibility to
create new NullTest quals for SJE.

> Note also that RestrictInfo is defined in nodes/pathnodes.h, which
> describes its contents as internal data structures for the planner,
> suggesting that the rewriter has no business processing those kinds of
> nodes.

> I don't think simply adding a comment addresses those concerns.

Agreed.  We may need more than just a comment change.

> > > Additionally, I have a minor suggestion regarding the new boolean
> > > parameter, "change_RangeTblRef".  AFAIU, by convention, we typically
> > > use flags to control the behavior of walker functions, like in
> > > pull_var_clause.  While it's fine to use a boolean parameter here
> > > given that we currently only have one flag, for the sake of future
> > > extensibility, I think using flags might be a better choice.

> > That was exactly why I wasn't happy with replacing the change_relid
> > routine with ChangeVarNodes.
> > But variant with a flag seems non-trivial to implement. Do you have any
> > proposal about the code?

> Perhaps the way to do it is to make ChangeVarNodesExtended() take a
> callback function to be invoked for each node visited. The callback
> (which would then be in the planner, with the other SJE code) would do
> any special handling it needed (for RangeTblRef and RestrictInfo
> nodes), and call ChangeVarNodes_walker() for any other types of node,
> to get the default behaviour.

Yeah, this might be a better approach.  Perhaps we can borrow some
ideas from replace_rte_variables.

Thanks
Richard

  7. Re: Some problems regarding the self-join elimination code

    Andrei Lepikhov <lepihov@gmail.com> — 2025-04-09T07:38:53Z

    On 4/9/25 04:05, Richard Guo wrote:
> On Tue, Apr 8, 2025 at 11:12 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>> On Tue, 8 Apr 2025 at 12:31, Andrei Lepikhov <lepihov@gmail.com> wrote:
>> Perhaps the way to do it is to make ChangeVarNodesExtended() take a
>> callback function to be invoked for each node visited. The callback
>> (which would then be in the planner, with the other SJE code) would do
>> any special handling it needed (for RangeTblRef and RestrictInfo
>> nodes), and call ChangeVarNodes_walker() for any other types of node,
>> to get the default behaviour.
> 
> Yeah, this might be a better approach.  Perhaps we can borrow some
> ideas from replace_rte_variables.
It seems we are coming to the conclusion that join removal optimisation 
may do something out of ChangeVarNodes resposibility. Before further 
complicating of this function code I would like to know opinion of Tom, 
who initially proposed [1] to use this routine. May be better a) return 
to more specialised change_relid / sje_walker machinery or b) move 
ChangeVarNodes out of rewriteManip and make it multi-purpose routine, 
allowing to transform expression that may happen after a Var node change?

[1] https://www.postgresql.org/message-id/3622801.1715010885%40sss.pgh.pa.us

-- 
regards, Andrei Lepikhov

  8. Re: Some problems regarding the self-join elimination code

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-04-10T11:36:57Z

    On Wed, Apr 9, 2025 at 10:39 AM Andrei Lepikhov <lepihov@gmail.com> wrote:
> On 4/9/25 04:05, Richard Guo wrote:
> > On Tue, Apr 8, 2025 at 11:12 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> >> On Tue, 8 Apr 2025 at 12:31, Andrei Lepikhov <lepihov@gmail.com> wrote:
> >> Perhaps the way to do it is to make ChangeVarNodesExtended() take a
> >> callback function to be invoked for each node visited. The callback
> >> (which would then be in the planner, with the other SJE code) would do
> >> any special handling it needed (for RangeTblRef and RestrictInfo
> >> nodes), and call ChangeVarNodes_walker() for any other types of node,
> >> to get the default behaviour.
> >
> > Yeah, this might be a better approach.  Perhaps we can borrow some
> > ideas from replace_rte_variables.
> It seems we are coming to the conclusion that join removal optimisation
> may do something out of ChangeVarNodes resposibility. Before further
> complicating of this function code I would like to know opinion of Tom,
> who initially proposed [1] to use this routine. May be better a) return
> to more specialised change_relid / sje_walker machinery or b) move
> ChangeVarNodes out of rewriteManip and make it multi-purpose routine,
> allowing to transform expression that may happen after a Var node change?

What about adding a callback to ChangeVarNodes_context that would
called for each RestrictInfo after changing varnodes itself?  SJE
could use a callback that replaces OpExpr with NullTest when needed.

------
Regards,
Alexander Korotkov
Supabase

  9. Re: Some problems regarding the self-join elimination code

    Andrei Lepikhov <lepihov@gmail.com> — 2025-04-10T12:39:04Z

    On 4/10/25 13:36, Alexander Korotkov wrote:
> On Wed, Apr 9, 2025 at 10:39 AM Andrei Lepikhov <lepihov@gmail.com> wrote:
>> It seems we are coming to the conclusion that join removal optimisation
>> may do something out of ChangeVarNodes resposibility. Before further
>> complicating of this function code I would like to know opinion of Tom,
>> who initially proposed [1] to use this routine. May be better a) return
>> to more specialised change_relid / sje_walker machinery or b) move
>> ChangeVarNodes out of rewriteManip and make it multi-purpose routine,
>> allowing to transform expression that may happen after a Var node change?
> 
> What about adding a callback to ChangeVarNodes_context that would
> called for each RestrictInfo after changing varnodes itself?  SJE
> could use a callback that replaces OpExpr with NullTest when needed.
I think it is doable, of course. Just looking forward a little, it may 
need more complication in the future (SJE definitely should be widened 
to partitioned tables) and it may be simpler to have two different 
routines for two different stages of planning.

-- 
regards, Andrei Lepikhov

  10. Re: Some problems regarding the self-join elimination code

    Andrei Lepikhov <lepihov@gmail.com> — 2025-04-11T14:46:33Z

    On 4/10/25 14:39, Andrei Lepikhov wrote:
> On 4/10/25 13:36, Alexander Korotkov wrote:
>> On Wed, Apr 9, 2025 at 10:39 AM Andrei Lepikhov <lepihov@gmail.com> 
>> wrote:
>>> It seems we are coming to the conclusion that join removal optimisation
>>> may do something out of ChangeVarNodes resposibility. Before further
>>> complicating of this function code I would like to know opinion of Tom,
>>> who initially proposed [1] to use this routine. May be better a) return
>>> to more specialised change_relid / sje_walker machinery or b) move
>>> ChangeVarNodes out of rewriteManip and make it multi-purpose routine,
>>> allowing to transform expression that may happen after a Var node 
>>> change?
>>
>> What about adding a callback to ChangeVarNodes_context that would
>> called for each RestrictInfo after changing varnodes itself?  SJE
>> could use a callback that replaces OpExpr with NullTest when needed.
> I think it is doable, of course. Just looking forward a little, it may 
> need more complication in the future (SJE definitely should be widened 
> to partitioned tables) and it may be simpler to have two different 
> routines for two different stages of planning. 
To provide some food for thought, here is a draft in attachment which 
addresses both issues: RestrictInfo relid replacement and move 
SJE-specific code out of the ChangeVarNodes routine (callback approach).

-- 
regards, Andrei Lepikhov

  11. Re: Some problems regarding the self-join elimination code

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-04-27T11:02:45Z

    On Fri, Apr 11, 2025 at 5:46 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
> On 4/10/25 14:39, Andrei Lepikhov wrote:
> > On 4/10/25 13:36, Alexander Korotkov wrote:
> >> On Wed, Apr 9, 2025 at 10:39 AM Andrei Lepikhov <lepihov@gmail.com>
> >> wrote:
> >>> It seems we are coming to the conclusion that join removal optimisation
> >>> may do something out of ChangeVarNodes resposibility. Before further
> >>> complicating of this function code I would like to know opinion of Tom,
> >>> who initially proposed [1] to use this routine. May be better a) return
> >>> to more specialised change_relid / sje_walker machinery or b) move
> >>> ChangeVarNodes out of rewriteManip and make it multi-purpose routine,
> >>> allowing to transform expression that may happen after a Var node
> >>> change?
> >>
> >> What about adding a callback to ChangeVarNodes_context that would
> >> called for each RestrictInfo after changing varnodes itself?  SJE
> >> could use a callback that replaces OpExpr with NullTest when needed.
> > I think it is doable, of course. Just looking forward a little, it may
> > need more complication in the future (SJE definitely should be widened
> > to partitioned tables) and it may be simpler to have two different
> > routines for two different stages of planning.
> To provide some food for thought, here is a draft in attachment which
> addresses both issues: RestrictInfo relid replacement and move
> SJE-specific code out of the ChangeVarNodes routine (callback approach).

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'm going to further work on improvement of these patches.

------
Regards,
Alexander Korotkov
Supabase

  12. Re: Some problems regarding the self-join elimination code

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-04-30T11:22:19Z

    On Sun, Apr 27, 2025 at 2:02 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> On Fri, Apr 11, 2025 at 5:46 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
> > On 4/10/25 14:39, Andrei Lepikhov wrote:
> > > On 4/10/25 13:36, Alexander Korotkov wrote:
> > >> On Wed, Apr 9, 2025 at 10:39 AM Andrei Lepikhov <lepihov@gmail.com>
> > >> wrote:
> > >>> It seems we are coming to the conclusion that join removal optimisation
> > >>> may do something out of ChangeVarNodes resposibility. Before further
> > >>> complicating of this function code I would like to know opinion of Tom,
> > >>> who initially proposed [1] to use this routine. May be better a) return
> > >>> to more specialised change_relid / sje_walker machinery or b) move
> > >>> ChangeVarNodes out of rewriteManip and make it multi-purpose routine,
> > >>> allowing to transform expression that may happen after a Var node
> > >>> change?
> > >>
> > >> What about adding a callback to ChangeVarNodes_context that would
> > >> called for each RestrictInfo after changing varnodes itself?  SJE
> > >> could use a callback that replaces OpExpr with NullTest when needed.
> > > I think it is doable, of course. Just looking forward a little, it may
> > > need more complication in the future (SJE definitely should be widened
> > > to partitioned tables) and it may be simpler to have two different
> > > routines for two different stages of planning.
> > To provide some food for thought, here is a draft in attachment which
> > addresses both issues: RestrictInfo relid replacement and move
> > SJE-specific code out of the ChangeVarNodes routine (callback approach).
>
> 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'm going to further work on improvement of these patches.

Sorry, I accidentally sent some messages off-list.  So, now 0001 and
0002 are pushed.

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?

------
Regards,
Alexander Korotkov
Supabase

  13. Re: Some problems regarding the self-join elimination code

    Andrei Lepikhov <lepihov@gmail.com> — 2025-04-30T13:34:28Z

    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

  14. Re: Some problems regarding the self-join elimination code

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-05-01T12:11:41Z

    Hi, Andrei!

Thank you for your review!

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?
>

Done.

------
Regards,
Alexander Korotkov
Supabase

  15. Re: Some problems regarding the self-join elimination code

    Andrei Lepikhov <lepihov@gmail.com> — 2025-05-01T17:22:20Z

    On 1/5/2025 14:11, Alexander Korotkov wrote:
>     3. Should the ChangeVarNodesWalkExpression function return the walker's
>     returning value?
> 
> 
> Done.
Thanks for your efforts! Looks good to me.

-- 
regards, Andrei Lepikhov

  16. Re: Some problems regarding the self-join elimination code

    Alexander Korotkov <aekorotkov@gmail.com> — 2025-05-01T19:01:07Z

    On Thu, May 1, 2025 at 8:22 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
> On 1/5/2025 14:11, Alexander Korotkov wrote:
> >     3. Should the ChangeVarNodesWalkExpression function return the walker's
> >     returning value?
> >
> >
> > Done.
> Thanks for your efforts! Looks good to me.

Thank you.  I'm going to push this if there is no objections.

------
Regards,
Alexander Korotkov
Supabase