Doc/improve confusing, inefficient tests to locate CTID variable.

The IsCTIDVar() tests in nodeTidscan.c and nodeTidrangescan.c
look buggy at first sight: they aren't checking that the varno
matches the table to be scanned.  Actually they're safe because
any Var in a scan-level qual must be for the correct table ...
but if we're depending on that, it's pretty pointless to verify
varlevelsup.  (Besides which, varlevelsup is *always* zero at
execution, since we've flattened the rangetable long since.)

Remove the useless varlevelsup check, and instead add some
commentary explaining why we don't need to check varno.

Noted while fooling with a planner change that causes the order
of "t1.ctid = t2.ctid" to change in some tidscan.sql tests;
I was briefly fooled into thinking there was a live bug here.

diff --git a/src/backend/executor/nodeTidrangescan.c b/src/backend/executor/nodeTidrangescan.c
index d5bf1be787fcb0a4cb2bb7dfc9db4e40bc28ff5e..0455819e73cd7c332c67cd8842fbca66865f921c 100644 (file)
--- a/src/backend/executor/nodeTidrangescan.c
+++ b/src/backend/executor/nodeTidrangescan.c
@@ -25,11 +25,16 @@
 #include "utils/rel.h"
 
 
+/*
+ * It's sufficient to check varattno to identify the CTID variable, as any
+ * Var in the relation scan qual must be for our table.  (Even if it's a
+ * parameterized scan referencing some other table's CTID, the other table's
+ * Var would have become a Param by the time it gets here.)
+ */
 #define IsCTIDVar(node)  \
    ((node) != NULL && \
     IsA((node), Var) && \
-    ((Var *) (node))->varattno == SelfItemPointerAttributeNumber && \
-    ((Var *) (node))->varlevelsup == 0)
+    ((Var *) (node))->varattno == SelfItemPointerAttributeNumber)
 
 typedef enum
 {

diff --git a/src/backend/executor/nodeTidscan.c b/src/backend/executor/nodeTidscan.c
index a1c6325d64956a2d3a6cc5d404706d0a0e31f9f1..ee4c0d1ea01c1e971020d25827d9c8f7352b3f06 100644 (file)
--- a/src/backend/executor/nodeTidscan.c
+++ b/src/backend/executor/nodeTidscan.c
@@ -35,11 +35,16 @@
 #include "utils/rel.h"
 
 
+/*
+ * It's sufficient to check varattno to identify the CTID variable, as any
+ * Var in the relation scan qual must be for our table.  (Even if it's a
+ * parameterized scan referencing some other table's CTID, the other table's
+ * Var would have become a Param by the time it gets here.)
+ */
 #define IsCTIDVar(node)  \
    ((node) != NULL && \
     IsA((node), Var) && \
-    ((Var *) (node))->varattno == SelfItemPointerAttributeNumber && \
-    ((Var *) (node))->varlevelsup == 0)
+    ((Var *) (node))->varattno == SelfItemPointerAttributeNumber)
 
 /* one element in tss_tidexprs */
 typedef struct TidExpr