Re: executor relation handling

Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> writes:
> I've rebased the remaining patches. I broke down one of the patches into
> 2 and re-ordered the patches as follows:

> 0001: introduces a function that opens range table relations and maintains
> them in an array indexes by RT index

> 0002: introduces a new field in EState that's an array of RangeTblEntry
> pointers and revises macros used in the executor that access RTEs to
> return them from the array (David Rowley co-authored this one)

I've pushed 0001 and 0002 with mostly cosmetic changes. One thing I
wanted to point out explicitly, though, is that I found this bit of 0002
to be a seriously bad idea:

--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -20,6 +20,7 @@
#include "executor/instrument.h"
#include "lib/pairingheap.h"
#include "nodes/params.h"
+#include "nodes/parsenodes.h"
#include "nodes/plannodes.h"
#include "utils/hsearch.h"
#include "utils/queryenvironment.h"

Please do not add #includes of fundamental headers to other fundamental
headers without clearing it with somebody. There's little enough
structure to our header collection now. I don't want to end up in a
situation where effectively the entire header set gets pulled into
every .c file, or worse that we have actual reference loops in the
headers. (This is not an academic problem; somebody actually created
such a loop awhile back. Cleaning it up, by the time we'd recognized
the problem, was really painful.)

> 0003: moves result relation and ExecRowMark initialization out of InitPlan
> and into ExecInit* routines of respective nodes

I am finding myself pretty unconvinced by this one; it seems like mostly
a random reallocation of responsibility with little advantage. The
particular thing that brought me to a screeching halt was seeing that
the patch removed ExecFindRowMark, despite the fact that that's part
of our advertised FDW API (see fdwhandler.sgml), and it didn't provide
any alternative way for an FDW to find out at runtime whether it's
subject to a row locking requirement.

I thought for a minute about just leaving the function in place, but
that wouldn't work because both nodeLockRows and nodeModifyTable are
written so that they find^H^H^Hbuild their rowmarks only after recursing
to initialize their child plan nodes; so a child node that tried to use
ExecFindRowMark during ExecInitNode would get the wrong answer. Of
course, we could consider changing the order of operations during
initialization of those node types, but I'm not really seeing a compelling
reason why we should whack things around that much.

So I'm inclined to just omit 0003. AFAICS this would only mean that
we couldn't drop the global PlanRowMarks list from PlannedStmt, which
does not bother me much.

> 0005: teaches planner to remove PlanRowMarks corresponding to dummy relations

I'm not entirely sold on the value of that either?

regards, tom lane