Re: POC: converting Lists into arrays - Mailing list pgsql-hackers

David Rowley <david.rowley@2ndquadrant.com> writes:
> On Fri, 9 Aug 2019 at 09:55, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I still have hopes for getting rid of es_range_table_array though,
>> and will look at that tomorrow or so.

> Yes, please. I've measured that to be quite an overhead with large
> partitioning setups. However, that was with some additional code which
> didn't lock partitions until it was ... well .... too late... as it
> turned out. But it seems pretty good to remove code that could be a
> future bottleneck if we ever manage to do something else with the
> locking of all partitions during UPDATE/DELETE.

I poked at this, and attached is a patch, but again I'm not seeing
that there's any real performance-based argument for it.  So far
as I can tell, if we've got a lot of RTEs in an executable plan,
the bulk of the startup time is going into lock (re) acquisition in
AcquirePlannerLocks, and/or permissions scanning in ExecCheckRTPerms;
both of those have to do work for every RTE including ones that
run-time pruning drops later on.  ExecInitRangeTable just isn't on
the radar.

If we wanted to try to improve things further, it seems like we'd
have to find a way to not lock unreferenced partitions at all,
as you suggest above.  But combining that with run-time pruning seems
like it'd be pretty horrid from a system structural standpoint: if we
acquire locks only during execution, what happens if we find we must
invalidate the query plan?

Anyway, the attached might be worth committing just on cleanliness
grounds, to avoid two-sources-of-truth issues in the executor.
But it seems like there's no additional performance win here
after all ... unless you've got a test case that shows differently?

            regards, tom lane

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index dbd7dd9..7f494ab 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -2790,7 +2790,6 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree)
     estate->es_snapshot = parentestate->es_snapshot;
     estate->es_crosscheck_snapshot = parentestate->es_crosscheck_snapshot;
     estate->es_range_table = parentestate->es_range_table;
-    estate->es_range_table_array = parentestate->es_range_table_array;
     estate->es_range_table_size = parentestate->es_range_table_size;
     estate->es_relations = parentestate->es_relations;
     estate->es_queryEnv = parentestate->es_queryEnv;
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index c1fc0d5..afd9beb 100644
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -113,7 +113,6 @@ CreateExecutorState(void)
     estate->es_snapshot = InvalidSnapshot;    /* caller must initialize this */
     estate->es_crosscheck_snapshot = InvalidSnapshot;    /* no crosscheck */
     estate->es_range_table = NIL;
-    estate->es_range_table_array = NULL;
     estate->es_range_table_size = 0;
     estate->es_relations = NULL;
     estate->es_rowmarks = NULL;
@@ -720,29 +719,17 @@ ExecOpenScanRelation(EState *estate, Index scanrelid, int eflags)
  * ExecInitRangeTable
  *        Set up executor's range-table-related data
  *
- * We build an array from the range table list to allow faster lookup by RTI.
- * (The es_range_table field is now somewhat redundant, but we keep it to
- * avoid breaking external code unnecessarily.)
- * This is also a convenient place to set up the parallel es_relations array.
+ * In addition to the range table proper, initialize arrays that are
+ * indexed by rangetable index.
  */
 void
 ExecInitRangeTable(EState *estate, List *rangeTable)
 {
-    Index        rti;
-    ListCell   *lc;
-
     /* Remember the range table List as-is */
     estate->es_range_table = rangeTable;

-    /* Set up the equivalent array representation */
+    /* Set size of associated arrays */
     estate->es_range_table_size = list_length(rangeTable);
-    estate->es_range_table_array = (RangeTblEntry **)
-        palloc(estate->es_range_table_size * sizeof(RangeTblEntry *));
-    rti = 0;
-    foreach(lc, rangeTable)
-    {
-        estate->es_range_table_array[rti++] = lfirst_node(RangeTblEntry, lc);
-    }

     /*
      * Allocate an array to store an open Relation corresponding to each
@@ -753,8 +740,8 @@ ExecInitRangeTable(EState *estate, List *rangeTable)
         palloc0(estate->es_range_table_size * sizeof(Relation));

     /*
-     * es_rowmarks is also parallel to the es_range_table_array, but it's
-     * allocated only if needed.
+     * es_rowmarks is also parallel to the es_range_table, but it's allocated
+     * only if needed.
      */
     estate->es_rowmarks = NULL;
 }
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 1fb28b4..39c8b3b 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -535,8 +535,7 @@ extern void ExecInitRangeTable(EState *estate, List *rangeTable);
 static inline RangeTblEntry *
 exec_rt_fetch(Index rti, EState *estate)
 {
-    Assert(rti > 0 && rti <= estate->es_range_table_size);
-    return estate->es_range_table_array[rti - 1];
+    return (RangeTblEntry *) list_nth(estate->es_range_table, rti - 1);
 }

 extern Relation ExecGetRangeTableRelation(EState *estate, Index rti);
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 4ec7849..063b490 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -502,7 +502,6 @@ typedef struct EState
     Snapshot    es_snapshot;    /* time qual to use */
     Snapshot    es_crosscheck_snapshot; /* crosscheck time qual for RI */
     List       *es_range_table; /* List of RangeTblEntry */
-    struct RangeTblEntry **es_range_table_array;    /* equivalent array */
     Index        es_range_table_size;    /* size of the range table arrays */
     Relation   *es_relations;    /* Array of per-range-table-entry Relation
                                  * pointers, or NULL if not yet opened */