Re: Increase value of OUTER_VAR - Mailing list pgsql-hackers

Aleksander Alekseev <aleksander@timescale.com> writes:
> +1. The proposed changes will be beneficial in the long term. They
> will affect existing extensions. However, the scale of the problem
> seems to be exaggerated.

Yeah, after thinking more about this I agree we should just do it.
I do not say that David's concerns about effects on extensions are
without merit, but I do think he's overblown it a bit.  Most of
the patch is s/Index/int/ for various variables, and as I mentioned
before, that's basically cosmetic; there's no strong reason why
extensions have to follow suit.  (In the attached v2, I modified
IS_SPECIAL_VARNO() as discussed, so it will do the right thing
even if the input is declared as Index.)  There may be a few
places where extensions need to add explicit IS_SPECIAL_VARNO()
calls, but not many, and I doubt they'll be hard to find.

The alternative of increasing the values of OUTER_VAR et al
is not without risk to extensions either, so on the whole
I don't think this patch is any more problematic than many
other things we commit with little debate.

In any case, since it's still very early in the v15 cycle,
there is plenty of time for people to find problems.  If I'm
wrong and there are serious consequences, we can always revert
this and do it the other way.

(v2 below is a rebase up to HEAD; no actual code changes except
for adjusting the definition of IS_SPECIAL_VARNO.)

            regards, tom lane

diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c
index 69ab34573e..9f1d8b6d1e 100644
--- a/src/backend/executor/execScan.c
+++ b/src/backend/executor/execScan.c
@@ -282,7 +282,7 @@ ExecAssignScanProjectionInfo(ScanState *node)
  *        As above, but caller can specify varno expected in Vars in the tlist.
  */
 void
-ExecAssignScanProjectionInfoWithVarno(ScanState *node, Index varno)
+ExecAssignScanProjectionInfoWithVarno(ScanState *node, int varno)
 {
     TupleDesc    tupdesc = node->ss_ScanTupleSlot->tts_tupleDescriptor;

diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index ad11392b99..4ab1302313 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -65,7 +65,7 @@
 #include "utils/typcache.h"


-static bool tlist_matches_tupdesc(PlanState *ps, List *tlist, Index varno, TupleDesc tupdesc);
+static bool tlist_matches_tupdesc(PlanState *ps, List *tlist, int varno, TupleDesc tupdesc);
 static void ShutdownExprContext(ExprContext *econtext, bool isCommit);


@@ -553,7 +553,7 @@ ExecAssignProjectionInfo(PlanState *planstate,
  */
 void
 ExecConditionalAssignProjectionInfo(PlanState *planstate, TupleDesc inputDesc,
-                                    Index varno)
+                                    int varno)
 {
     if (tlist_matches_tupdesc(planstate,
                               planstate->plan->targetlist,
@@ -579,7 +579,7 @@ ExecConditionalAssignProjectionInfo(PlanState *planstate, TupleDesc inputDesc,
 }

 static bool
-tlist_matches_tupdesc(PlanState *ps, List *tlist, Index varno, TupleDesc tupdesc)
+tlist_matches_tupdesc(PlanState *ps, List *tlist, int varno, TupleDesc tupdesc)
 {
     int            numattrs = tupdesc->natts;
     int            attrno;
diff --git a/src/backend/executor/nodeCustom.c b/src/backend/executor/nodeCustom.c
index c82060e6d1..1dfa53c381 100644
--- a/src/backend/executor/nodeCustom.c
+++ b/src/backend/executor/nodeCustom.c
@@ -31,7 +31,7 @@ ExecInitCustomScan(CustomScan *cscan, EState *estate, int eflags)
     CustomScanState *css;
     Relation    scan_rel = NULL;
     Index        scanrelid = cscan->scan.scanrelid;
-    Index        tlistvarno;
+    int            tlistvarno;

     /*
      * Allocate the CustomScanState object.  We let the custom scan provider
diff --git a/src/backend/executor/nodeForeignscan.c b/src/backend/executor/nodeForeignscan.c
index 1abd906b16..0a549afcc0 100644
--- a/src/backend/executor/nodeForeignscan.c
+++ b/src/backend/executor/nodeForeignscan.c
@@ -138,7 +138,7 @@ ExecInitForeignScan(ForeignScan *node, EState *estate, int eflags)
     ForeignScanState *scanstate;
     Relation    currentRelation = NULL;
     Index        scanrelid = node->scan.scanrelid;
-    Index        tlistvarno;
+    int            tlistvarno;
     FdwRoutine *fdwroutine;

     /* check for unsupported flags */
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index 01c110cd2f..7d1a01d1ed 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -63,7 +63,7 @@ makeSimpleA_Expr(A_Expr_Kind kind, char *name,
  *      creates a Var node
  */
 Var *
-makeVar(Index varno,
+makeVar(int varno,
         AttrNumber varattno,
         Oid vartype,
         int32 vartypmod,
@@ -85,7 +85,7 @@ makeVar(Index varno,
      * them, but just initialize them to the given varno/varattno.  This
      * reduces code clutter and chance of error for most callers.
      */
-    var->varnosyn = varno;
+    var->varnosyn = (Index) varno;
     var->varattnosyn = varattno;

     /* Likewise, we just set location to "unknown" here */
@@ -100,7 +100,7 @@ makeVar(Index varno,
  *        TargetEntry
  */
 Var *
-makeVarFromTargetEntry(Index varno,
+makeVarFromTargetEntry(int varno,
                        TargetEntry *tle)
 {
     return makeVar(varno,
@@ -131,7 +131,7 @@ makeVarFromTargetEntry(Index varno,
  */
 Var *
 makeWholeRowVar(RangeTblEntry *rte,
-                Index varno,
+                int varno,
                 Index varlevelsup,
                 bool allowScalar)
 {
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 36e618611f..464a7f1da5 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1116,7 +1116,7 @@ _outVar(StringInfo str, const Var *node)
 {
     WRITE_NODE_TYPE("VAR");

-    WRITE_UINT_FIELD(varno);
+    WRITE_INT_FIELD(varno);
     WRITE_INT_FIELD(varattno);
     WRITE_OID_FIELD(vartype);
     WRITE_INT_FIELD(vartypmod);
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 0dd1ad7dfc..559cf47d7f 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -577,7 +577,7 @@ _readVar(void)
 {
     READ_LOCALS(Var);

-    READ_UINT_FIELD(varno);
+    READ_INT_FIELD(varno);
     READ_INT_FIELD(varattno);
     READ_OID_FIELD(vartype);
     READ_INT_FIELD(vartypmod);
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 1fd53b40bb..1e4d404f02 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -5961,7 +5961,8 @@ set_pathtarget_cost_width(PlannerInfo *root, PathTarget *target)
             Assert(var->varlevelsup == 0);

             /* Try to get data from RelOptInfo cache */
-            if (var->varno < root->simple_rel_array_size)
+            if (!IS_SPECIAL_VARNO(var->varno) &&
+                var->varno < root->simple_rel_array_size)
             {
                 RelOptInfo *rel = root->simple_rel_array[var->varno];

diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index a5f6d678cc..3dc0176a51 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -4805,7 +4805,8 @@ replace_nestloop_params_mutator(Node *node, PlannerInfo *root)
         /* Upper-level Vars should be long gone at this point */
         Assert(var->varlevelsup == 0);
         /* If not to be replaced, we can just return the Var unmodified */
-        if (!bms_is_member(var->varno, root->curOuterRels))
+        if (IS_SPECIAL_VARNO(var->varno) ||
+            !bms_is_member(var->varno, root->curOuterRels))
             return node;
         /* Replace the Var with a nestloop Param */
         return (Node *) replace_nestloop_param_var(root, var);
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index e50624c465..ca22c340a7 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -31,7 +31,7 @@

 typedef struct
 {
-    Index        varno;            /* RT index of Var */
+    int            varno;            /* RT index of Var */
     AttrNumber    varattno;        /* attr number of Var */
     AttrNumber    resno;            /* TLE position of Var */
 } tlist_vinfo;
@@ -66,7 +66,7 @@ typedef struct
 {
     PlannerInfo *root;
     indexed_tlist *subplan_itlist;
-    Index        newvarno;
+    int            newvarno;
     int            rtoffset;
     double        num_exec;
 } fix_upper_expr_context;
@@ -143,15 +143,15 @@ static void set_dummy_tlist_references(Plan *plan, int rtoffset);
 static indexed_tlist *build_tlist_index(List *tlist);
 static Var *search_indexed_tlist_for_var(Var *var,
                                          indexed_tlist *itlist,
-                                         Index newvarno,
+                                         int newvarno,
                                          int rtoffset);
 static Var *search_indexed_tlist_for_non_var(Expr *node,
                                              indexed_tlist *itlist,
-                                             Index newvarno);
+                                             int newvarno);
 static Var *search_indexed_tlist_for_sortgroupref(Expr *node,
                                                   Index sortgroupref,
                                                   indexed_tlist *itlist,
-                                                  Index newvarno);
+                                                  int newvarno);
 static List *fix_join_expr(PlannerInfo *root,
                            List *clauses,
                            indexed_tlist *outer_itlist,
@@ -163,7 +163,7 @@ static Node *fix_join_expr_mutator(Node *node,
 static Node *fix_upper_expr(PlannerInfo *root,
                             Node *node,
                             indexed_tlist *subplan_itlist,
-                            Index newvarno,
+                            int newvarno,
                             int rtoffset, double num_exec);
 static Node *fix_upper_expr_mutator(Node *node,
                                     fix_upper_expr_context *context);
@@ -468,16 +468,6 @@ add_rte_to_flat_rtable(PlannerGlobal *glob, RangeTblEntry *rte)

     glob->finalrtable = lappend(glob->finalrtable, newrte);

-    /*
-     * Check for RT index overflow; it's very unlikely, but if it did happen,
-     * the executor would get confused by varnos that match the special varno
-     * values.
-     */
-    if (IS_SPECIAL_VARNO(list_length(glob->finalrtable)))
-        ereport(ERROR,
-                (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
-                 errmsg("too many range table entries")));
-
     /*
      * If it's a plain relation RTE, add the table to relationOids.
      *
@@ -1924,10 +1914,8 @@ fix_scan_expr_mutator(Node *node, fix_scan_expr_context *context)
     {
         CurrentOfExpr *cexpr = (CurrentOfExpr *) copyObject(node);

-        Assert(cexpr->cvarno != INNER_VAR);
-        Assert(cexpr->cvarno != OUTER_VAR);
-        if (!IS_SPECIAL_VARNO(cexpr->cvarno))
-            cexpr->cvarno += context->rtoffset;
+        Assert(!IS_SPECIAL_VARNO((int) cexpr->cvarno));
+        cexpr->cvarno += context->rtoffset;
         return (Node *) cexpr;
     }
     if (IsA(node, PlaceHolderVar))
@@ -2424,7 +2412,7 @@ build_tlist_index(List *tlist)
  * (so nothing other than Vars and PlaceHolderVars can be matched).
  */
 static indexed_tlist *
-build_tlist_index_other_vars(List *tlist, Index ignore_rel)
+build_tlist_index_other_vars(List *tlist, int ignore_rel)
 {
     indexed_tlist *itlist;
     tlist_vinfo *vinfo;
@@ -2476,9 +2464,9 @@ build_tlist_index_other_vars(List *tlist, Index ignore_rel)
  */
 static Var *
 search_indexed_tlist_for_var(Var *var, indexed_tlist *itlist,
-                             Index newvarno, int rtoffset)
+                             int newvarno, int rtoffset)
 {
-    Index        varno = var->varno;
+    int            varno = var->varno;
     AttrNumber    varattno = var->varattno;
     tlist_vinfo *vinfo;
     int            i;
@@ -2516,7 +2504,7 @@ search_indexed_tlist_for_var(Var *var, indexed_tlist *itlist,
  */
 static Var *
 search_indexed_tlist_for_non_var(Expr *node,
-                                 indexed_tlist *itlist, Index newvarno)
+                                 indexed_tlist *itlist, int newvarno)
 {
     TargetEntry *tle;

@@ -2558,7 +2546,7 @@ static Var *
 search_indexed_tlist_for_sortgroupref(Expr *node,
                                       Index sortgroupref,
                                       indexed_tlist *itlist,
-                                      Index newvarno)
+                                      int newvarno)
 {
     ListCell   *lc;

@@ -2776,7 +2764,7 @@ static Node *
 fix_upper_expr(PlannerInfo *root,
                Node *node,
                indexed_tlist *subplan_itlist,
-               Index newvarno,
+               int newvarno,
                int rtoffset,
                double num_exec)
 {
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 224c5153b1..387a35e112 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -80,7 +80,7 @@ static Node *pull_up_simple_union_all(PlannerInfo *root, Node *jtnode,
 static void pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root,
                                        int parentRTindex, Query *setOpQuery,
                                        int childRToffset);
-static void make_setop_translation_list(Query *query, Index newvarno,
+static void make_setop_translation_list(Query *query, int newvarno,
                                         AppendRelInfo *appinfo);
 static bool is_simple_subquery(PlannerInfo *root, Query *subquery,
                                RangeTblEntry *rte,
@@ -1372,7 +1372,7 @@ pull_up_union_leaf_queries(Node *setOp, PlannerInfo *root, int parentRTindex,
  *      Also create the rather trivial reverse-translation array.
  */
 static void
-make_setop_translation_list(Query *query, Index newvarno,
+make_setop_translation_list(Query *query, int newvarno,
                             AppendRelInfo *appinfo)
 {
     List       *vars = NIL;
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index ccd2835c22..b932a83827 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -6949,7 +6949,7 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context)
     AttrNumber    attnum;
     int            netlevelsup;
     deparse_namespace *dpns;
-    Index        varno;
+    int            varno;
     AttrNumber    varattno;
     deparse_columns *colinfo;
     char       *refname;
@@ -6995,7 +6995,7 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context)
          */
         if (context->appendparents && dpns->appendrels)
         {
-            Index        pvarno = varno;
+            int            pvarno = varno;
             AttrNumber    pvarattno = varattno;
             AppendRelInfo *appinfo = dpns->appendrels[pvarno];
             bool        found = false;
@@ -7305,7 +7305,7 @@ get_name_for_var_field(Var *var, int fieldno,
     AttrNumber    attnum;
     int            netlevelsup;
     deparse_namespace *dpns;
-    Index        varno;
+    int            varno;
     AttrNumber    varattno;
     TupleDesc    tupleDesc;
     Node       *expr;
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 3dc03c913e..cd57a704ad 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -459,7 +459,7 @@ typedef bool (*ExecScanRecheckMtd) (ScanState *node, TupleTableSlot *slot);
 extern TupleTableSlot *ExecScan(ScanState *node, ExecScanAccessMtd accessMtd,
                                 ExecScanRecheckMtd recheckMtd);
 extern void ExecAssignScanProjectionInfo(ScanState *node);
-extern void ExecAssignScanProjectionInfoWithVarno(ScanState *node, Index varno);
+extern void ExecAssignScanProjectionInfoWithVarno(ScanState *node, int varno);
 extern void ExecScanReScan(ScanState *node);

 /*
@@ -552,7 +552,7 @@ extern const TupleTableSlotOps *ExecGetResultSlotOps(PlanState *planstate,
 extern void ExecAssignProjectionInfo(PlanState *planstate,
                                      TupleDesc inputDesc);
 extern void ExecConditionalAssignProjectionInfo(PlanState *planstate,
-                                                TupleDesc inputDesc, Index varno);
+                                                TupleDesc inputDesc, int varno);
 extern void ExecFreeExprContext(PlanState *planstate);
 extern void ExecAssignScanType(ScanState *scanstate, TupleDesc tupDesc);
 extern void ExecCreateScanSlotFromOuterPlan(EState *estate,
diff --git a/src/include/nodes/makefuncs.h b/src/include/nodes/makefuncs.h
index 48a7ebfe45..eea87f847d 100644
--- a/src/include/nodes/makefuncs.h
+++ b/src/include/nodes/makefuncs.h
@@ -24,18 +24,18 @@ extern A_Expr *makeA_Expr(A_Expr_Kind kind, List *name,
 extern A_Expr *makeSimpleA_Expr(A_Expr_Kind kind, char *name,
                                 Node *lexpr, Node *rexpr, int location);

-extern Var *makeVar(Index varno,
+extern Var *makeVar(int varno,
                     AttrNumber varattno,
                     Oid vartype,
                     int32 vartypmod,
                     Oid varcollid,
                     Index varlevelsup);

-extern Var *makeVarFromTargetEntry(Index varno,
+extern Var *makeVarFromTargetEntry(int varno,
                                    TargetEntry *tle);

 extern Var *makeWholeRowVar(RangeTblEntry *rte,
-                            Index varno,
+                            int varno,
                             Index varlevelsup,
                             bool allowScalar);

diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 7b125904b4..433437643e 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -172,12 +172,12 @@ typedef struct Expr
  * in the planner and doesn't correspond to any simple relation column may
  * have varnosyn = varattnosyn = 0.
  */
-#define    INNER_VAR        65000    /* reference to inner subplan */
-#define    OUTER_VAR        65001    /* reference to outer subplan */
-#define    INDEX_VAR        65002    /* reference to index column */
-#define    ROWID_VAR        65003    /* row identity column during planning */
+#define    INNER_VAR        (-1)    /* reference to inner subplan */
+#define    OUTER_VAR        (-2)    /* reference to outer subplan */
+#define    INDEX_VAR        (-3)    /* reference to index column */
+#define    ROWID_VAR        (-4)    /* row identity column during planning */

-#define IS_SPECIAL_VARNO(varno)        ((varno) >= INNER_VAR)
+#define IS_SPECIAL_VARNO(varno)        ((int) (varno) < 0)

 /* Symbols for the indexes of the special RTE entries in rules */
 #define    PRS2_OLD_VARNO            1
@@ -186,8 +186,8 @@ typedef struct Expr
 typedef struct Var
 {
     Expr        xpr;
-    Index        varno;            /* index of this var's relation in the range
-                                 * table, or INNER_VAR/OUTER_VAR/INDEX_VAR */
+    int            varno;            /* index of this var's relation in the range
+                                 * table, or INNER_VAR/OUTER_VAR/etc */
     AttrNumber    varattno;        /* attribute number of this var, or zero for
                                  * all attrs ("whole-row Var") */
     Oid            vartype;        /* pg_type OID for the type of this var */
@@ -1351,6 +1351,7 @@ typedef struct SetToDefault
  * of the target relation being constrained; this aids placing the expression
  * correctly during planning.  We can assume however that its "levelsup" is
  * always zero, due to the syntactic constraints on where it can appear.
+ * Also, cvarno will always be a true RT index, never INNER_VAR etc.
  *
  * The referenced cursor can be represented either as a hardwired string
  * or as a reference to a run-time parameter of type REFCURSOR.  The latter