Thread

Re: A little cosmetic to convert_VALUES_to_ANY()

Tender Wang <tndrwang@gmail.com> — 2025-08-04T03:41:35Z

Chao Li <li.evan.chao@gmail.com> 于2025年8月4日周一 11:11写道：

> I tested this patch locally and it works for me.
>
> However I doubt if it really improve performance. The original code
> "contain_volatile_functions((Node *) rte->values_lists)" recursively work
> through rte-values_list, and this patch move contain_volatile_functions()
> into the for loop, and checks against every item of rte->values_list. As
> entries of values_list could just be Const, contain_volatile_functions()
> will then return immediately, so that this changes reduces total calls to
> contain_volatile_functions(). From this perspective, this patch may improve
> the performance.
>
> On the other side, let's say a case where a values_list contains 10 items,
> and the last item is volatile. With the original code, it won't enter the
> for loop, nothing will be built; with this patch, operations have been
> performed on the first 9 items, but eventually it returns NULL when hit the
> last item. So in this case, performance could be worse.
>

Yes, in this case, the original code scans the values_list, finds the
volatile functions, and returns from convert_VALUES_to_ANY().
My patch will perform unnecessary convert_testexpr
and eval_const_expressions work.

Overall, I am afraid the burden could beat the gain.
>

It's not easy to evaluate performance gain.
I'm not sure it's worth doing the change. If someone doesn't like the
patch. I will withdraw it.


> The new status of this patch is: Waiting on Author
>
Thanks for your review.

Actually, the purpose of convert_VALUES_to_ANY() is only to convert the
sublink that includes the Values expression to SAOP.
I have a question: Can we move the convert_VALUES_to_ANY() to another
place? For example:preprocess_qual_conditions().
In pull_up_sublinks_jointree_recurse(), what we want to do is to pull up
the sublinks.

-- 
Thanks,
Tender Wang