Re: check for null value before looking up the hash function

Em sáb., 21 de mai. de 2022 às 12:05, Tomas Vondra <tomas.vondra@enterprisedb.com> escreveu:

On 5/21/22 15:06, Ranier Vilela wrote:
>>Zhihong Yu <zyu(at)yugabyte(dot)com> writes:
>>> I was looking at the code in hash_record()
>>> of src/backend/utils/adt/rowtypes.c
>>> It seems if nulls[i] is true, we don't need to look up the hash function.
>
>>I don't think this is worth changing. It complicates the logic,
>>rendering it unlike quite a few other functions written in the same
>>style. In cases where the performance actually matters, the hash
>>function is cached across multiple calls anyway. You might save
>>something if you have many calls in a query and not one of them
>>receives a non-null input, but how likely is that?
>
> I disagree.
> I think that is worth changing. The fact of complicating the logic
> is irrelevant.

That's a quite bold claim, and yet you haven't supported it by any
argument whatsoever. Trade-offs between complexity and efficiency are a
crucial development task, so complicating the logic clearly does matter.

What I meant is that complicating the logic in search of efficiency is worth it, and that's what everyone is looking for in this thread.
Likewise, not complicating the logic, losing a little bit of efficiency, applied to all the code, leads to a big loss of efficiency.
In other words, I never miss an opportunity to gain efficiency.

diff --git a/src/backend/utils/adt/rowtypes.c b/src/backend/utils/adt/rowtypes.c
index db843a0fbf..0bc28d1742 100644
--- a/src/backend/utils/adt/rowtypes.c
+++ b/src/backend/utils/adt/rowtypes.c
@@ -1838,6 +1838,13 @@ hash_record(PG_FUNCTION_ARGS)
        TypeCacheEntry *typentry;
        uint32      element_hash;
 
+       if (nulls[i])
+       {
+           /* see hash_array() */
+           result = (result << 5) - result + 0;
+           continue;
+       }
+
        att = TupleDescAttr(tupdesc, i);
 
        if (att->attisdropped)
@@ -1860,24 +1867,16 @@ hash_record(PG_FUNCTION_ARGS)
            my_extra->columns[i].typentry = typentry;
        }
 
-       /* Compute hash of element */
-       if (nulls[i])
-       {
-           element_hash = 0;
-       }
-       else
-       {
-           LOCAL_FCINFO(locfcinfo, 1);
+       LOCAL_FCINFO(locfcinfo, 1);
 
-           InitFunctionCallInfoData(*locfcinfo, &typentry->hash_proc_finfo, 1,
-                                    att->attcollation, NULL, NULL);
-           locfcinfo->args[0].value = values[i];
-           locfcinfo->args[0].isnull = false;
-           element_hash = DatumGetUInt32(FunctionCallInvoke(locfcinfo));
+       InitFunctionCallInfoData(*locfcinfo, &typentry->hash_proc_finfo, 1,
+                                   att->attcollation, NULL, NULL);
+       locfcinfo->args[0].value = values[i];
+       locfcinfo->args[0].isnull = false;
+       element_hash = DatumGetUInt32(FunctionCallInvoke(locfcinfo));
 
-           /* We don't expect hash support functions to return null */
-           Assert(!locfcinfo->isnull);
-       }
+       /* We don't expect hash support functions to return null */
+       Assert(!locfcinfo->isnull);
 
        /* see hash_array() */
        result = (result << 5) - result + element_hash;