Increase value of OUTER_VAR

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

Increase value of OUTER_VAR

Andrey V. Lepikhov
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

0001-Set-values-of-special-varnos-to-the-upper-bound-of-t.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

David Rowley
On Wed, 3 Mar 2021 at 21:29, Andrey Lepikhov <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

Amit Langote
On Wed, Mar 3, 2021 at 5:52 PM David Rowley <[hidden email]> wrote:

> On Wed, 3 Mar 2021 at 21:29, Andrey Lepikhov <[hidden email]> 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:

https://www.postgresql.org/message-id/flat/16302-e45634e2c0e34e97%40postgresql.org

--
Amit Langote
EDB: http://www.enterprisedb.com


Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

Julien Rouhaud
On Wed, Mar 3, 2021 at 4:57 PM Amit Langote <[hidden email]> wrote:

>
> On Wed, Mar 3, 2021 at 5:52 PM David Rowley <[hidden email]> 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:
>
> https://www.postgresql.org/message-id/flat/16302-e45634e2c0e34e97%40postgresql.org

+1


Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

Tom Lane-2
In reply to this post by Amit Langote
Amit Langote <[hidden email]> writes:
> Also, I got reminded of this discussion from not so long ago:

> https://www.postgresql.org/message-id/flat/16302-e45634e2c0e34e97%40postgresql.org

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


Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

Andrey V. Lepikhov
In reply to this post by Julien Rouhaud
On 3/3/21 12:52, Julien Rouhaud wrote:

> On Wed, Mar 3, 2021 at 4:57 PM Amit Langote <[hidden email]> wrote:
>>
>> On Wed, Mar 3, 2021 at 5:52 PM David Rowley <[hidden email]> 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:
>>
>> https://www.postgresql.org/message-id/flat/16302-e45634e2c0e34e97%40postgresql.org
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

0001-Increase-values-of-special-varnos-to-1-million.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

Tomas Vondra-6


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 <[hidden email]>
>> wrote:
>>>
>>> On Wed, Mar 3, 2021 at 5:52 PM David Rowley <[hidden email]>
>>> 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:
>>>
>>> https://www.postgresql.org/message-id/flat/16302-e45634e2c0e34e97%40postgresql.org
>>>
> 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


Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

Tom Lane-2
Tomas Vondra <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

Tomas Vondra-6
On 3/4/21 4:16 PM, Tom Lane wrote:

> Tomas Vondra <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

Tom Lane-2
Tomas Vondra <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

Tom Lane-2
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


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") */
Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

Peter Eisentraut-7
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.


Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

Tom Lane-2
Peter Eisentraut <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

Peter Eisentraut-7
On 06.03.21 15:59, Tom Lane wrote:

> Peter Eisentraut <[hidden email]> 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.

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?




Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

Tom Lane-2
Peter Eisentraut <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

Tom Lane-2
I wrote:
> Peter Eisentraut <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Increase value of OUTER_VAR

Andrey V. Lepikhov
On 4/8/21 8:13 AM, Tom Lane wrote:
> I wrote:
>> Peter Eisentraut <[hidden email]> 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

0001-remove-64k-rangetable-limit-wip.patch (8K) Download Attachment