Increase value of OUTER_VAR
Hi,
Playing with a large value of partitions I caught the limit with 65000
table entries in a query plan:
if (IS_SPECIAL_VARNO(list_length(glob->finalrtable)))
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("too many range table entries")));
Postgres works well with so many partitions.
The constants INNER_VAR, OUTER_VAR, INDEX_VAR are used as values of the
variable 'var->varno' of integer type. As I see, they were introduced
with commit 1054097464 authored by Marc G. Fournier, in 1996.
Value 65000 was relevant to the size of the int type at that time.
Maybe we will change these values to INT_MAX? (See the patch in attachment).
--
regards,
Andrey Lepikhov
Postgres Professional
Attachments:
0001-Set-values-of-special-varnos-to-the-upper-bound-of-t.patchtext/plain; charset=UTF-8; name=0001-Set-values-of-special-varnos-to-the-upper-bound-of-t.patch; x-mac-creator=0; x-mac-type=0Download
From 98da77cdefd53dbf4ce0c740d1a0f356da970648 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>
Date: Wed, 3 Mar 2021 11:22:32 +0300
Subject: [PATCH] Set values of special varnos to the upper bound of the int
type
---
src/include/nodes/primnodes.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index d4ce037088..9038d97886 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -168,9 +168,9 @@ 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 INNER_VAR (INT_MAX - 2) /* reference to inner subplan */
+#define OUTER_VAR (INT_MAX - 1) /* reference to outer subplan */
+#define INDEX_VAR (INT_MAX) /* reference to index column */
#define IS_SPECIAL_VARNO(varno) ((varno) >= INNER_VAR)
--
2.29.2
On Wed, 3 Mar 2021 at 21:29, Andrey Lepikhov <a.lepikhov@postgrespro.ru> wrote:
Playing with a large value of partitions I caught the limit with 65000
table entries in a query plan:if (IS_SPECIAL_VARNO(list_length(glob->finalrtable)))
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("too many range table entries")));Postgres works well with so many partitions.
The constants INNER_VAR, OUTER_VAR, INDEX_VAR are used as values of the
variable 'var->varno' of integer type. As I see, they were introduced
with commit 1054097464 authored by Marc G. Fournier, in 1996.
Value 65000 was relevant to the size of the int type at that time.Maybe we will change these values to INT_MAX? (See the patch in attachment).
I don't really see any reason not to increase these a bit, but I'd
rather we kept them at some realistic maximum rather than all-out went
to INT_MAX.
I imagine a gap was left between 65535 and 65000 to allow space for
more special varno in the future. We did get INDEX_VAR since then, so
it seems like it was probably a good idea to leave a gap.
The problem I see what going close to INT_MAX is that the ERROR you
mention is unlikely to work correctly since a list_length() will never
get close to having INT_MAX elements before palloc() would exceed
MaxAllocSize for the elements array.
Something like 1 million seems like a more realistic limit to me.
That might still be on the high side, but it'll likely mean we'd not
need to revisit this for quite a while.
David
On Wed, Mar 3, 2021 at 5:52 PM David Rowley <dgrowleyml@gmail.com> wrote:
On Wed, 3 Mar 2021 at 21:29, Andrey Lepikhov <a.lepikhov@postgrespro.ru> wrote:
Playing with a large value of partitions I caught the limit with 65000
table entries in a query plan:if (IS_SPECIAL_VARNO(list_length(glob->finalrtable)))
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("too many range table entries")));Postgres works well with so many partitions.
The constants INNER_VAR, OUTER_VAR, INDEX_VAR are used as values of the
variable 'var->varno' of integer type. As I see, they were introduced
with commit 1054097464 authored by Marc G. Fournier, in 1996.
Value 65000 was relevant to the size of the int type at that time.Maybe we will change these values to INT_MAX? (See the patch in attachment).
I don't really see any reason not to increase these a bit, but I'd
rather we kept them at some realistic maximum rather than all-out went
to INT_MAX.I imagine a gap was left between 65535 and 65000 to allow space for
more special varno in the future. We did get INDEX_VAR since then, so
it seems like it was probably a good idea to leave a gap.The problem I see what going close to INT_MAX is that the ERROR you
mention is unlikely to work correctly since a list_length() will never
get close to having INT_MAX elements before palloc() would exceed
MaxAllocSize for the elements array.Something like 1 million seems like a more realistic limit to me.
That might still be on the high side, but it'll likely mean we'd not
need to revisit this for quite a while.
+1
Also, I got reminded of this discussion from not so long ago:
/messages/by-id/16302-e45634e2c0e34e97@postgresql.org
--
Amit Langote
EDB: http://www.enterprisedb.com
On Wed, Mar 3, 2021 at 4:57 PM Amit Langote <amitlangote09@gmail.com> wrote:
On Wed, Mar 3, 2021 at 5:52 PM David Rowley <dgrowleyml@gmail.com> wrote:
Something like 1 million seems like a more realistic limit to me.
That might still be on the high side, but it'll likely mean we'd not
need to revisit this for quite a while.+1
Also, I got reminded of this discussion from not so long ago:
+1
Amit Langote <amitlangote09@gmail.com> writes:
Also, I got reminded of this discussion from not so long ago:
Yeah. Nobody seems to have pursued Peter's idea of changing the magic
values to small negative ones, but that seems like a nicer idea than
arguing over what large positive value is large enough.
(Having said that, I remain pretty dubious that we're anywhere near
getting any real-world use out of such a change.)
regards, tom lane
On 3/3/21 12:52, Julien Rouhaud wrote:
On Wed, Mar 3, 2021 at 4:57 PM Amit Langote <amitlangote09@gmail.com> wrote:
On Wed, Mar 3, 2021 at 5:52 PM David Rowley <dgrowleyml@gmail.com> wrote:
Something like 1 million seems like a more realistic limit to me.
That might still be on the high side, but it'll likely mean we'd not
need to revisit this for quite a while.+1
Also, I got reminded of this discussion from not so long ago:
Thank you
+1
Ok. I changed the value to 1 million and explained this decision in the
comment.
This issue caused by two cases:
1. Range partitioning on a timestamp column.
2. Hash partitioning.
Users use range distribution by timestamp because they want to insert
new data quickly and analyze entire set of data.
Also, in some discussions, I see Oracle users discussing issues with
more than 1e5 partitions.
--
regards,
Andrey Lepikhov
Postgres Professional
Attachments:
0001-Increase-values-of-special-varnos-to-1-million.patchtext/plain; charset=UTF-8; name=0001-Increase-values-of-special-varnos-to-1-million.patch; x-mac-creator=0; x-mac-type=0Download
From a3c1ee9d2e197dee40aed81cb6a08695a8fa2917 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>
Date: Wed, 3 Mar 2021 11:22:32 +0300
Subject: [PATCH] Increase values of special varnos to 1 million. Use this
value as a realistic limit for number of range table entries. Restrict it to
detect possible errors which can cause exceeding of MaxAllocSize in palloc()
on the elements of the array. This value can be changed later to suit the
needs of the real world.
---
src/include/nodes/primnodes.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index d4ce037088..06016340a3 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -168,9 +168,9 @@ 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 INNER_VAR 1000000 /* reference to inner subplan */
+#define OUTER_VAR 1000001 /* reference to outer subplan */
+#define INDEX_VAR 1000002 /* reference to index column */
#define IS_SPECIAL_VARNO(varno) ((varno) >= INNER_VAR)
--
2.29.2
On 3/4/21 8:43 AM, Andrey Lepikhov wrote:
On 3/3/21 12:52, Julien Rouhaud wrote:
On Wed, Mar 3, 2021 at 4:57 PM Amit Langote <amitlangote09@gmail.com>
wrote:On Wed, Mar 3, 2021 at 5:52 PM David Rowley <dgrowleyml@gmail.com>
wrote:Something like 1 million seems like a more realistic limit to me.
That might still be on the high side, but it'll likely mean we'd not
need to revisit this for quite a while.+1
Also, I got reminded of this discussion from not so long ago:
Thank you
+1
Ok. I changed the value to 1 million and explained this decision in the
comment.
IMO just bumping up the constants from ~65k to 1M is a net loss, for
most users. We add this to bitmapsets, which means we're using ~8kB with
the current values, but this jumps to 128kB with this higher value. This
also means bms_next_member etc. have to walk much more memory, which is
bound to have some performance impact for everyone.
Switching to small negative values is a much better idea, but it's going
to be more invasive - we'll have to offset the values in the bitmapsets,
or we'll have to invent a new bitmapset variant that can store negative
values directly (e.g. by keeping two separate bitmaps internally, one
for negative and one for positive values). But that complicates other
stuff too (e.g. bms_next_member now returns -1 to signal "end").
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
IMO just bumping up the constants from ~65k to 1M is a net loss, for
most users. We add this to bitmapsets, which means we're using ~8kB with
the current values, but this jumps to 128kB with this higher value. This
also means bms_next_member etc. have to walk much more memory, which is
bound to have some performance impact for everyone.
Hmm, do we really have any places that include OUTER_VAR etc in
bitmapsets? They shouldn't appear in relid sets, for sure.
I agree though that if they did, this would have bad performance
consequences.
I still think the negative-special-values approach is better.
If there are any places that that would break, we'd find out about
it in short order, rather than having a silent performance lossage.
regards, tom lane
On 3/4/21 4:16 PM, Tom Lane wrote:
Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
IMO just bumping up the constants from ~65k to 1M is a net loss, for
most users. We add this to bitmapsets, which means we're using ~8kB with
the current values, but this jumps to 128kB with this higher value. This
also means bms_next_member etc. have to walk much more memory, which is
bound to have some performance impact for everyone.Hmm, do we really have any places that include OUTER_VAR etc in
bitmapsets? They shouldn't appear in relid sets, for sure.
I agree though that if they did, this would have bad performance
consequences.
Hmmm, I don't know. I mostly assumed that if I do pull_varnos() it would
include those values. But maybe that's not supposed to happen.
I still think the negative-special-values approach is better.
If there are any places that that would break, we'd find out about
it in short order, rather than having a silent performance lossage.
OK
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
On 3/4/21 4:16 PM, Tom Lane wrote:
Hmm, do we really have any places that include OUTER_VAR etc in
bitmapsets? They shouldn't appear in relid sets, for sure.
I agree though that if they did, this would have bad performance
consequences.
Hmmm, I don't know. I mostly assumed that if I do pull_varnos() it would
include those values. But maybe that's not supposed to happen.
But (IIRC) those varnos are never used till setrefs.c fixes up the plan
to replace normal Vars with references to lower plan nodes' outputs.
I'm not sure why anyone would be doing pull_varnos() after that;
it would not give very meaningful results.
regards, tom lane
Just as a proof of concept, I tried the attached, and it passes
check-world. So if there's anyplace trying to stuff OUTER_VAR and
friends into bitmapsets, it's pretty far off the beaten track.
The main loose ends that'd have to be settled seem to be:
(1) What data type do we want Var.varno to be declared as? In the
previous thread, Robert opined that plain "int" isn't a good choice,
but I'm not sure I agree. There's enough "int" for rangetable indexes
all over the place that it'd be a fool's errand to try to make it
uniformly something different.
(2) Does that datatype change need to propagate anywhere besides
what I touched here? I did not make any effort to search for
other places.
regards, tom lane
Attachments:
remove-64k-rangetable-limit-wip.patchtext/x-diff; charset=us-ascii; name=remove-64k-rangetable-limit-wip.patchDownload
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 8fc432bfe1..d44d84804e 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1098,7 +1098,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 718fb58e86..ff94c10b8d 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -575,7 +575,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/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 42f088ad71..f9267d329e 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -467,16 +467,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.
*
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index d4ce037088..43d8100424 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -168,11 +168,11 @@ 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 INNER_VAR (-1) /* reference to inner subplan */
+#define OUTER_VAR (-2) /* reference to outer subplan */
+#define INDEX_VAR (-3) /* reference to index column */
-#define IS_SPECIAL_VARNO(varno) ((varno) >= INNER_VAR)
+#define IS_SPECIAL_VARNO(varno) ((varno) < 0)
/* Symbols for the indexes of the special RTE entries in rules */
#define PRS2_OLD_VARNO 1
@@ -181,7 +181,7 @@ typedef struct Expr
typedef struct Var
{
Expr xpr;
- Index varno; /* index of this var's relation in the range
+ int varno; /* index of this var's relation in the range
* table, or INNER_VAR/OUTER_VAR/INDEX_VAR */
AttrNumber varattno; /* attribute number of this var, or zero for
* all attrs ("whole-row Var") */
On 04.03.21 20:01, Tom Lane wrote:
Just as a proof of concept, I tried the attached, and it passes
check-world. So if there's anyplace trying to stuff OUTER_VAR and
friends into bitmapsets, it's pretty far off the beaten track.The main loose ends that'd have to be settled seem to be:
(1) What data type do we want Var.varno to be declared as? In the
previous thread, Robert opined that plain "int" isn't a good choice,
but I'm not sure I agree. There's enough "int" for rangetable indexes
all over the place that it'd be a fool's errand to try to make it
uniformly something different.
int seems fine.
(2) Does that datatype change need to propagate anywhere besides
what I touched here? I did not make any effort to search for
other places.
I think
Var.varnosyn
CurrentOfExpr.cvarno
should also have their type changed.
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
On 04.03.21 20:01, Tom Lane wrote:
(2) Does that datatype change need to propagate anywhere besides
what I touched here? I did not make any effort to search for
other places.
I think
Var.varnosyn
CurrentOfExpr.cvarno
should also have their type changed.
Agreed as to CurrentOfExpr.cvarno. But I think the entire point of
varnosyn is that it saves the original rangetable reference and
*doesn't* get overwritten with OUTER_VAR etc. So that one is a
different animal, and I'm inclined to leave it as Index.
regards, tom lane
On 06.03.21 15:59, Tom Lane wrote:
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
On 04.03.21 20:01, Tom Lane wrote:
(2) Does that datatype change need to propagate anywhere besides
what I touched here? I did not make any effort to search for
other places.I think
Var.varnosyn
CurrentOfExpr.cvarnoshould also have their type changed.
Agreed as to CurrentOfExpr.cvarno. But I think the entire point of
varnosyn is that it saves the original rangetable reference and
*doesn't* get overwritten with OUTER_VAR etc. So that one is a
different animal, and I'm inclined to leave it as Index.
Can we move forward with this?
I suppose there was still some uncertainty about whether all the places
that need changing have been identified, but do we have a better idea
how to find them?
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
Can we move forward with this?
I suppose there was still some uncertainty about whether all the places
that need changing have been identified, but do we have a better idea
how to find them?
We could just push the change and see what happens. But I was thinking
more in terms of doing that early in the v15 cycle. I remain skeptical
that we need a near-term fix.
regards, tom lane
I wrote:
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
Can we move forward with this?
We could just push the change and see what happens. But I was thinking
more in terms of doing that early in the v15 cycle. I remain skeptical
that we need a near-term fix.
To make sure we don't forget, I added an entry to the next CF for this.
regards, tom lane
On 4/8/21 8:13 AM, Tom Lane wrote:
I wrote:
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
Can we move forward with this?
We could just push the change and see what happens. But I was thinking
more in terms of doing that early in the v15 cycle. I remain skeptical
that we need a near-term fix.To make sure we don't forget, I added an entry to the next CF for this.
Thanks for your efforts.
I tried to dive deeper: replace ROWID_VAR with -4 and explicitly change
types of varnos in the description of functions that can only work with
special varnos.
Use cases of OUTER_VAR looks simple (i guess). Use cases of INNER_VAR is
more complex because of the map_variable_attnos(). It is needed to
analyze how negative value of INNER_VAR can affect on this function.
INDEX_VAR causes potential problem:
in ExecInitForeignScan() and ExecInitForeignScan() we do
tlistvarno = INDEX_VAR;
here tlistvarno has non-negative type.
ROWID_VAR caused two problems in the check-world tests:
set_pathtarget_cost_width():
if (var->varno < root->simple_rel_array_size)
{
RelOptInfo *rel = root->simple_rel_array[var->varno];
...
and
replace_nestloop_params_mutator():
if (!bms_is_member(var->varno, root->curOuterRels))
I skipped this problems to see other weak points, but check-world
couldn't find another.
--
regards,
Andrey Lepikhov
Postgres Professional
Attachments:
0001-remove-64k-rangetable-limit-wip.patchtext/x-patch; charset=UTF-8; name=0001-remove-64k-rangetable-limit-wip.patchDownload
From 6ba9441cc43a2ccf868ca271494bf5b9950692e6 Mon Sep 17 00:00:00 2001
From: Andrey Lepikhov <a.lepikhov@postgrespro.ru>
Date: Thu, 8 Apr 2021 08:43:04 +0500
Subject: [PATCH] Remove 64k rangetable limit.
---
src/backend/nodes/bitmapset.c | 1 +
src/backend/nodes/outfuncs.c | 2 +-
src/backend/nodes/readfuncs.c | 2 +-
src/backend/optimizer/path/costsize.c | 3 ++-
src/backend/optimizer/plan/createplan.c | 3 ++-
src/backend/optimizer/plan/setrefs.c | 30 +++++++++----------------
src/include/nodes/primnodes.h | 12 +++++-----
7 files changed, 23 insertions(+), 30 deletions(-)
diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c
index 649478b0d4..c0d50c85da 100644
--- a/src/backend/nodes/bitmapset.c
+++ b/src/backend/nodes/bitmapset.c
@@ -23,6 +23,7 @@
#include "common/hashfn.h"
#include "nodes/bitmapset.h"
#include "nodes/pg_list.h"
+#include "nodes/primnodes.h"
#include "port/pg_bitutils.h"
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 4a8dc2d86d..4c3de615d1 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1115,7 +1115,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 9924727851..d084eee6ef 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 05686d0194..ac72bddfae 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -5934,7 +5934,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 22f10fa339..defd179bca 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -4809,7 +4809,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 70c0fa07e6..6009eabaf2 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -66,7 +66,7 @@ typedef struct
{
PlannerInfo *root;
indexed_tlist *subplan_itlist;
- Index newvarno;
+ int refvarno;
int rtoffset;
double num_exec;
} fix_upper_expr_context;
@@ -147,7 +147,7 @@ static Var *search_indexed_tlist_for_var(Var *var,
int rtoffset);
static Var *search_indexed_tlist_for_non_var(Expr *node,
indexed_tlist *itlist,
- Index newvarno);
+ int refvarno);
static Var *search_indexed_tlist_for_sortgroupref(Expr *node,
Index sortgroupref,
indexed_tlist *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 refvarno,
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.
*
@@ -2503,7 +2493,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 refvarno)
{
TargetEntry *tle;
@@ -2523,7 +2513,7 @@ search_indexed_tlist_for_non_var(Expr *node,
/* Found a matching subplan output expression */
Var *newvar;
- newvar = makeVarFromTargetEntry(newvarno, tle);
+ newvar = makeVarFromTargetEntry(refvarno, tle);
newvar->varnosyn = 0; /* wasn't ever a plain Var */
newvar->varattnosyn = 0;
return newvar;
@@ -2763,7 +2753,7 @@ static Node *
fix_upper_expr(PlannerInfo *root,
Node *node,
indexed_tlist *subplan_itlist,
- Index newvarno,
+ int refvarno,
int rtoffset,
double num_exec)
{
@@ -2771,7 +2761,7 @@ fix_upper_expr(PlannerInfo *root,
context.root = root;
context.subplan_itlist = subplan_itlist;
- context.newvarno = newvarno;
+ context.refvarno = refvarno;
context.rtoffset = rtoffset;
context.num_exec = num_exec;
return fix_upper_expr_mutator(node, &context);
@@ -2790,7 +2780,7 @@ fix_upper_expr_mutator(Node *node, fix_upper_expr_context *context)
newvar = search_indexed_tlist_for_var(var,
context->subplan_itlist,
- context->newvarno,
+ context->refvarno,
context->rtoffset);
if (!newvar)
elog(ERROR, "variable not found in subplan target list");
@@ -2805,7 +2795,7 @@ fix_upper_expr_mutator(Node *node, fix_upper_expr_context *context)
{
newvar = search_indexed_tlist_for_non_var((Expr *) phv,
context->subplan_itlist,
- context->newvarno);
+ context->refvarno);
if (newvar)
return (Node *) newvar;
}
@@ -2817,7 +2807,7 @@ fix_upper_expr_mutator(Node *node, fix_upper_expr_context *context)
{
newvar = search_indexed_tlist_for_non_var((Expr *) node,
context->subplan_itlist,
- context->newvarno);
+ context->refvarno);
if (newvar)
return (Node *) newvar;
}
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index f2ac4e51f1..34effe0f44 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) ((varno) < 0)
/* Symbols for the indexes of the special RTE entries in rules */
#define PRS2_OLD_VARNO 1
@@ -186,7 +186,7 @@ typedef struct Expr
typedef struct Var
{
Expr xpr;
- Index varno; /* index of this var's relation in the range
+ int varno; /* index of this var's relation in the range
* table, or INNER_VAR/OUTER_VAR/INDEX_VAR */
AttrNumber varattno; /* attribute number of this var, or zero for
* all attrs ("whole-row Var") */
--
2.25.1
Here's a more fleshed-out version of this patch. I ran around and
fixed all the places where INNER_VAR etc. were being assigned directly to
a variable or parameter of type Index, and also grepped for 'Index.*varno'
to find suspicious declarations. (I didn't change every last instance
of the latter though; just places that could possibly be looking at
post-setrefs.c Vars.)
I concluded that we don't really need to change the type of
CurrentOfExpr.cvarno, because that's never set to a special value.
The main thing I remain concerned about is whether there are more
places like set_pathtarget_cost_width(), where we could be making
an inequality comparison on "varno" that would now be wrong.
I tried to catch this by enabling -Wsign-compare and -Wsign-conversion,
but that produced so many thousands of uninteresting warnings that
I soon gave up. I'm not sure there's any good way to catch remaining
places like that except to commit the patch and wait for trouble
reports.
So I'm inclined to propose pushing this and seeing what happens.
regards, tom lane
Attachments:
remove-64k-rangetable-limit-1.patchtext/x-diff; charset=us-ascii; name=remove-64k-rangetable-limit-1.patchDownload
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 9dc38d47ea..cd93d1d768 100644
--- a/src/backend/executor/nodeForeignscan.c
+++ b/src/backend/executor/nodeForeignscan.c
@@ -128,7 +128,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 e32b92e299..bcb116ae8d 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 f0b34ecfac..4c3ffa482c 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 8577c7b138..fd9566bcc7 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -5963,7 +5963,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 439e6b6426..1244723ffe 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -4808,7 +4808,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 61ccfd300b..5e4a1a856b 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.
*
@@ -1921,10 +1911,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))
@@ -2421,7 +2409,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;
@@ -2473,9 +2461,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;
@@ -2513,7 +2501,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;
@@ -2555,7 +2543,7 @@ static Var *
search_indexed_tlist_for_sortgroupref(Expr *node,
Index sortgroupref,
indexed_tlist *itlist,
- Index newvarno)
+ int newvarno)
{
ListCell *lc;
@@ -2773,7 +2761,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 62a1668796..2bf2459395 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,
@@ -1365,7 +1365,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 3719755a0d..12d92fd7a0 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -6941,7 +6941,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;
@@ -6987,7 +6987,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;
@@ -7297,7 +7297,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 9ae851d847..3a3d9e7669 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) ((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 */
@@ -1341,6 +1341,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
On Sat, 3 Jul 2021 at 06:23, Tom Lane <tgl@sss.pgh.pa.us> wrote:
So I'm inclined to propose pushing this and seeing what happens.
Is this really sane?
As much as I would like to see the 65k limit removed, I just have
reservations about fixing it in this way. Even if we get all the
cases fixed in core, there's likely a whole bunch of extensions
that'll have bugs as a result of this for many years to come.
"git grep \sIndex\s -- *.[ch] | wc -l" is showing me 77 matches in the
Citus code. That's not the only extension that uses the planner hook.
I'm really just not sure it's worth all the dev hours fixing the
fallout. To me, it seems much safer to jump bump 65k up to 1m. It'll
be a while before anyone complains about that.
It's also not that great to see the number of locations that you
needed to add run-time checks for negative varnos. That's not going to
come for free.
David
David Rowley <dgrowleyml@gmail.com> writes:
Is this really sane?
As much as I would like to see the 65k limit removed, I just have
reservations about fixing it in this way. Even if we get all the
cases fixed in core, there's likely a whole bunch of extensions
that'll have bugs as a result of this for many years to come.
Maybe. I'm not that concerned about planner hacking: almost all of
the planner is only concerned with pre-setrefs.c representations and
will never see these values. Still, the fact that we had to inject
a couple of explicit IS_SPECIAL_VARNO tests is a bit worrisome.
(I'm more surprised really that noplace in the executor needed it.)
FWIW, experience with those places says that such bugs will be
exposed immediately; it's not like they'd lurk undetected "for years".
You might argue that the int-vs-Index declaration changes are
something that would be much harder to get right, but in reality
those are almost entirely cosmetic. We could make them completely
so by changing the macro to
#define IS_SPECIAL_VARNO(varno) ((int) (varno) < 0)
so that it'd still do the right thing when applied to a variable
declared as Index. (In the light of morning, I'm not sure why
I didn't do that already.) But we've always been extremely
cavalier about whether RT indexes should be declared as int or
Index, so I felt that standardizing on the former was actually
a good side-effect of the patch.
Anyway, to address your point more directly: as I recall, the main
objection to just increasing the values of these constants was the
fear that it'd bloat bitmapsets containing these values. Now on
the one hand, this patch has proven that noplace in the core code
does that today. On the other hand, there's no certainty that
someone might not try to do that tomorrow (if we don't fix it as
per this patch); or extensions might be doing so.
I'm really just not sure it's worth all the dev hours fixing the
fallout. To me, it seems much safer to jump bump 65k up to 1m. It'll
be a while before anyone complains about that.
TBH, if we're to approach it that way, I'd be inclined to go for
broke and raise the values to ~2B. Then (a) we'll be shut of the
problem pretty much permanently, and (b) if someone does try to
make a bitmapset containing these values, hopefully they'll see
performance bad enough to expose the issue immediately.
It's also not that great to see the number of locations that you
needed to add run-time checks for negative varnos. That's not going to
come for free.
Since the test is just "< 0", I pretty much disbelieve that argument.
There are only two such places in the patch, and neither of them
are *that* performance-sensitive.
Anyway, the raise-the-values solution does have the advantage of
being a four-liner, so I can live with it if that's the consensus.
But I do think this way is cleaner in the long run, and I doubt
the argument that it'll create any hard-to-detect bugs.
regards, tom lane
On 2/7/21 21:23, Tom Lane wrote:
So I'm inclined to propose pushing this and seeing what happens.
+1
But why the Index type still uses for indexing of range table entries?
For example:
- we give int resultRelation value to create_modifytable_path() as Index
nominalRelation value.
- exec_rt_fetch(Index) calls list_nth(int).
- generate_subquery_vars() accepts an 'Index varno' valueIt looks sloppy. Do you plan to change this in the next commits?
--
regards,
Andrey Lepikhov
Postgres Professional
Hi hackers,
So I'm inclined to propose pushing this and seeing what happens.
+1
+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.
I can confirm that the patch passes installcheck-world. After some
searching through the code, I was unable to identify any places where
the logic will break. Although this only proves my inattention, the
easiest way to make any further progress seems to apply the patch.
--
Best regards,
Aleksander Alekseev
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
Attachments:
remove-64k-rangetable-limit-2.patchtext/x-diff; charset=us-ascii; name=remove-64k-rangetable-limit-2.patchDownload
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
Andrey Lepikhov <a.lepikhov@postgrespro.ru> writes:
But why the Index type still uses for indexing of range table entries?
For example:
- we give int resultRelation value to create_modifytable_path() as Index
nominalRelation value.
- exec_rt_fetch(Index) calls list_nth(int).
- generate_subquery_vars() accepts an 'Index varno' value
As I mentioned, the patch only intends to touch code that's possibly
used with post-setrefs Vars. In the parser and most of the planner,
there's little need to do anything because only positive varno values
will appear. So touching that code would just make the patch more
invasive without accomplishing much.
If we'd had any strong convention about whether RT indexes should be
int or Index, I might be worried about maintaining consistency.
But it's always been a horrid mishmash of both ways. Cleaning that
up completely is a task I don't care to undertake right now.
regards, tom lane
On 9/11/21 10:37 PM, Tom Lane wrote:
Aleksander Alekseev <aleksander@timescale.com> writes:
(v2 below is a rebase up to HEAD; no actual code changes except
for adjusting the definition of IS_SPECIAL_VARNO.)
I have looked at this code. No problems found.
Also, as a test, I used two tables with 1E5 partitions each. I tried to
do plain SELECT, JOIN, join with plain table. No errors found, only
performance issues. But it is a subject for another research.
--
regards,
Andrey Lepikhov
Postgres Professional
Hi Andrey,
only performance issues
That's interesting. Any chance you could share the hardware
description, the configuration file, and steps to reproduce with us?
--
Best regards,
Aleksander Alekseev
"Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru> writes:
Also, as a test, I used two tables with 1E5 partitions each. I tried to
do plain SELECT, JOIN, join with plain table. No errors found, only
performance issues. But it is a subject for another research.
Yeah, there's no expectation that the performance would be any
good yet ;-)
regards, tom lane
On 14/9/21 16:37, Aleksander Alekseev wrote:
Hi Andrey,
only performance issues
That's interesting. Any chance you could share the hardware
description, the configuration file, and steps to reproduce with us?
I didn't control execution time exactly. Because it is a join of two
empty tables. As I see, this join used most part of 48GB RAM memory,
planned all day on a typical 6 amd cores computer.
I guess this is caused by sequental traversal of the partition list in
some places in the optimizer.
If it makes practical sense, I could investigate reasons for such poor
performance.
--
regards,
Andrey Lepikhov
Postgres Professional
Hi Andrey,
only performance issues
That's interesting. Any chance you could share the hardware
description, the configuration file, and steps to reproduce with us?I didn't control execution time exactly. Because it is a join of two
empty tables. As I see, this join used most part of 48GB RAM memory,
planned all day on a typical 6 amd cores computer.
I guess this is caused by sequental traversal of the partition list in
some places in the optimizer.
If it makes practical sense, I could investigate reasons for such poor
performance.
Let's say, any information regarding bottlenecks that affect real users
with real queries is of interest. Artificially created queries that are
unlikely to be ever executed by anyone are not.
--
Best regards,
Aleksander Alekseev