Re: Assorted style changes with a tiny improvement

Thanks Michael.

Em seg., 7 de out. de 2024 às 22:53, Michael Paquier <michael(at)paquier(dot)xyz>
escreveu:

> On Tue, Jul 02, 2024 at 02:39:20PM -0300, Ranier Vilela wrote:
> > This is a series of patches to change styles, in assorted sources.
> > IMO, this improves a tiny bit and is worth trying.
> >
> > 1. Avoid dereference iss_SortSupport if it has nulls.
> > 2. Avoid dereference plan_node_id if no dsm area.
> > 3. Avoid dereference spill partitions if zero ntuples.
> > 4. Avoid possible useless palloc call with zero size.
> > 5. Avoid redundant variable initialization in foreign.
> > 6. Check the cheap test first in ExecMain.
> > 7. Check the cheap test first in pathkeys.
> > 8. Delay possible expensive bms_is_empty call in sub_select.
> > 9. Reduce some scope in execPartition.
> > 10. Reduce some scope for TupleDescAttr array dereference.
> > 11. Remove useless duplicate test in ruleutils.
> > This is already checked at line 4566.
> >
> > 12. Remove useless duplicate test in string_utils.
> > This is already checked at line 982.
>
> You have something here, but not everything is worth changing without
> a reason to do so, other than code "correctness". For example,
> bms_is_empty() is a NULL comparison, so it does not matter.

Of course, this is a tiny bit of optimization and it is something laborious
for the comitter.

I don't
> see a point in the three dereference patches, either, as these states
> are expected AFAIK so it does not matter. If we crash, it's actually
> an indication that something has gone wrong.

Sorry by sorry for the confusing name patch.
Actually, these are not null pointers being dereferenced.
The point here is to avoid touching memory (cache or RAM) when it is not
strictly necessary.

> Same comment about the
> two remove-useless patches and the two reduce-some-scope.
>
Same motivation here, avoid touching memory (cache or RAM) when it is not
strictly necessary.

> The point about group_keys_reorder_by_pathkeys() is to be proved; I
> doubt it matters.

If *ec_sortref* is nonzero.
We avoid touching memory, an expensive branch and a call to an expensive
function.

> Same for the ExecBuildSlotValueDescription()
> business to check for the acl_result before bms_is_member() does not
> really matter performance-wise.
>
Same here, if *aclresult* is ACLCHECK_OK,
we avoid an expensive subtraction and an expensive function call.

>
> The allocation in execTuplesMatchPrepare() is indeed something that
> we'd better avoid, that's minimal but memory that can be avoided is
> always less error-prone. pg_options_to_table() also, is a bit better
> this way. Applied these two, let's move on.
>
Great.

Thanks for your work.

best regards,
Ranier Vilela