Thread: minor nodeIndexScan tweak

Per EDB's Coverity analysis, "runtimeKeyInfo" is only non-NULL if
"econtext" is also non-NULL, so we can eliminate a conditional on the
former by moving it inside an "if" block for the latter. Per discussion
of earlier similar changes, this is not for performance reasons but for
code clarity.

Barring any objections I'll apply this tonight or tomorrow.

-Neil
Index: src/backend/executor/nodeIndexscan.c
===================================================================
RCS file: /Users/neilc/local/cvs/pgsql/src/backend/executor/nodeIndexscan.c,v
retrieving revision 1.103
diff -c -r1.103 nodeIndexscan.c
*** src/backend/executor/nodeIndexscan.c    6 May 2005 17:24:54 -0000    1.103
--- src/backend/executor/nodeIndexscan.c    8 Jul 2005 05:06:41 -0000
***************
*** 208,221 ****
           * recalculate *all* runtime keys on each call.
           */
          ResetExprContext(econtext);
-     }

!     /*
!      * If we are doing runtime key calculations (ie, the index keys depend
!      * on data from an outer scan), compute the new key values
!      */
!     if (runtimeKeyInfo)
!     {
          ExecIndexEvalRuntimeKeys(econtext,
                                   runtimeKeyInfo,
                                   scanKeys,
--- 208,219 ----
           * recalculate *all* runtime keys on each call.
           */
          ResetExprContext(econtext);

!         /*
!          * If we are doing runtime key calculations (ie, the index
!          * keys depend on data from an outer scan), compute the new
!          * key values
!          */
          ExecIndexEvalRuntimeKeys(econtext,
                                   runtimeKeyInfo,
                                   scanKeys,
***************
*** 603,610 ****

          Assert(leftop != NULL);

!         if (!(IsA(leftop, Var) &&
!               var_is_rel((Var *) leftop)))
              elog(ERROR, "indexqual doesn't have key on left side");

          varattno = ((Var *) leftop)->varattno;
--- 601,607 ----

          Assert(leftop != NULL);

!         if (!(IsA(leftop, Var) && var_is_rel((Var *) leftop)))
              elog(ERROR, "indexqual doesn't have key on left side");

          varattno = ((Var *) leftop)->varattno;

Neil Conway <neilc@samurai.com> writes:
> Per EDB's Coverity analysis, "runtimeKeyInfo" is only non-NULL if
> "econtext" is also non-NULL, so we can eliminate a conditional on the
> former by moving it inside an "if" block for the latter.

This is premature optimization par excellance --- it saves one if-test,
in a not very performance-critical routine.  I disagree that it improves
the code clarity: it's far from obvious that being-passed-an-outer-tuple
is the same condition as has-runtime-keys, and I can think of numerous
possible future changes that would render the conditions distinct again.
So please, don't do it.

> !         if (!(IsA(leftop, Var) &&
> !               var_is_rel((Var *) leftop)))
>               elog(ERROR, "indexqual doesn't have key on left side");

> !         if (!(IsA(leftop, Var) && var_is_rel((Var *) leftop)))
>               elog(ERROR, "indexqual doesn't have key on left side");

Just FYI, the reason for the line break is that pgindent will make it
look uglier if it's all on one line.  The IsA construct seems to confuse
pgindent somehow ...

            regards, tom lane