Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Robert Haas]

On Thu, Sep 14, 2017 at 12:59 PM, David G. Johnston
 wrote:
> I think we are being consistent as a project by enforcing strictness of
> input in this situation so I'll toss my +0.5/+1 here as well.

All right, since all three new votes are going the same direction with
this, committed the patch for that.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[David G. Johnston]

On Thu, Sep 14, 2017 at 8:41 AM, Stephen Frost  wrote:

> Robert, all,
>
> * Robert Haas (robertmh...@gmail.com) wrote:
>
> > >
> > > I vote for rejecting it.  DDL compatibility is less valuable than other
> > > compatibility.  The hypothetical affected application can change its
> DDL to
> > > placate PostgreSQL and use that modified DDL for all other databases,
> too.
> >
> > OK.  Any other votes?
>
> I haven't been as close to this as others, so perhaps my vote is only
> 0.5 on this specific case, but that's my feeling on it.
>

I think we are being consistent as a project by enforcing strictness of
input in this situation so I'll toss my +0.5/+1​ here as well.

David J.

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Stephen Frost]

Robert, all,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Wed, Sep 13, 2017 at 10:49 PM, Noah Misch  wrote:
> >> > Both Oracle and MySQL allow finite values after MAXVALUE (usually
> >> > listed as "0" in code examples, e.g. see [1]). Oracle explicitly
> >> > documents the fact that values after MAXVALUE are irrelevant in [1].
> >> > I'm not sure if MySQL explicitly documents that, but it does behave
> >> > the same.
> >> >
> >> > Also, both Oracle and MySQL store what the user entered (they do not
> >> > canonicalise), as can be seen by looking at ALL_TAB_PARTITIONS in
> >> > Oracle, or "show create table" in MySQL.
> >>
> >> OK, thanks.  I still don't really like allowing this, but I can see
> >> that compatibility with other systems has some value here, and if
> >> nobody else is rejecting these cases, maybe we shouldn't either.  So
> >> I'll hold my nose and change my vote to canonicalizing rather than
> >> rejecting outright.
> >
> > I vote for rejecting it.  DDL compatibility is less valuable than other
> > compatibility.  The hypothetical affected application can change its DDL to
> > placate PostgreSQL and use that modified DDL for all other databases, too.
> 
> OK.  Any other votes?

I think I side with Noah on this one.  I agree that DDL compatibility is
less valuable than other compatibility.  I had also been thinking that
perhaps it would be good to still be compatible for the sake of DBAs not
being confused if they got an error, but this seems straight-forward
enough that it wouldn't take much for a DBA who is building such
partitions to understand their mistake and to fix it.

I haven't been as close to this as others, so perhaps my vote is only
0.5 on this specific case, but that's my feeling on it.

Also, I don't think we should be adding compatibility for the sake of
compatibility alone.  If there's more than one way to do something and
they're all correct and reasonable, then I could see us choosing the
route that matches what others in the industry do, but I don't see
simply ignoring user input in this specific case as really correct and
therefore it's better to reject it.

Basically, for my 2c, the reason Oracle does this is something more of a
historical artifact than because it was deemed sensible, and everyone
else just followed suit, but I don't believe we need to or should.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Robert Haas]

On Wed, Sep 13, 2017 at 10:49 PM, Noah Misch  wrote:
>> > Both Oracle and MySQL allow finite values after MAXVALUE (usually
>> > listed as "0" in code examples, e.g. see [1]). Oracle explicitly
>> > documents the fact that values after MAXVALUE are irrelevant in [1].
>> > I'm not sure if MySQL explicitly documents that, but it does behave
>> > the same.
>> >
>> > Also, both Oracle and MySQL store what the user entered (they do not
>> > canonicalise), as can be seen by looking at ALL_TAB_PARTITIONS in
>> > Oracle, or "show create table" in MySQL.
>>
>> OK, thanks.  I still don't really like allowing this, but I can see
>> that compatibility with other systems has some value here, and if
>> nobody else is rejecting these cases, maybe we shouldn't either.  So
>> I'll hold my nose and change my vote to canonicalizing rather than
>> rejecting outright.
>
> I vote for rejecting it.  DDL compatibility is less valuable than other
> compatibility.  The hypothetical affected application can change its DDL to
> placate PostgreSQL and use that modified DDL for all other databases, too.

OK.  Any other votes?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Amit Langote]

On 2017/09/14 16:53, Dean Rasheed wrote:
> On 13 September 2017 at 10:05, Amit Langote
>  wrote:
>> Coincidentally, I just wrote the patch for canonicalizing stored values,
>> instead of erroring out.  Please see attached if that's what you were
>> thinking too.
>>
> 
> Looks reasonable to me, if we decide to go this way.>
> One minor review comment --

Thanks for the review.

> it isn't really necessary to have the
> separate new boolean local variables as well as the datum kind
> variables. Just tracking the datum kind is sufficient and slightly
> simpler. That would also address a slight worry I have that your
> coding might result in a compiler warning about the kind variables
> being used uninitialised with some less intelligent compilers, not
> smart enough to work out that it can't actually happen.

Got rid of the boolean variables in the updated patch.

Thanks,
Amit
From 61bfaeb46623572f5adba0b624d00dfecb6ed495 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 13 Sep 2017 17:51:38 +0900
Subject: [PATCH] Canonicalize catalog representation of range partition bounds

Since the system effectively ignores values of the bound for
partition key columns following the first unbounded key, it's pointless
to accurately remember them in the system catalog.  Instead store
minvalue or maxvalue for all such columns, matching what the first
unbounded key is.  This makes \d output of range partitions look
more canonical.
---
 doc/src/sgml/ref/create_table.sgml |  6 +-
 src/backend/parser/parse_utilcmd.c | 30 --
 src/test/regress/expected/create_table.out | 20 +---
 src/test/regress/expected/insert.out   |  8 
 src/test/regress/sql/create_table.sql  |  6 ++
 5 files changed, 60 insertions(+), 10 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index 824253de40..7d2eb30722 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -328,7 +328,11 @@ FROM ( { numeric_literal | MAXVALUE in a partition bound are ignored; so the bound
   (10, MINVALUE, 0) is equivalent to
   (10, MINVALUE, 10) and (10, MINVALUE, MINVALUE)
-  and (10, MINVALUE, MAXVALUE).
+  and (10, MINVALUE, MAXVALUE).  Morever, the system will
+  store (10, MINVALUE, MINVALUE) into the system catalog if
+  the user specifies (10, MINVALUE, 0), and
+  (10, MAXVALUE, MAXVALUE) if the user specifies
+  (10, MAXVALUE, 0).
  
 
  
diff --git a/src/backend/parser/parse_utilcmd.c 
b/src/backend/parser/parse_utilcmd.c
index 655da02c10..ca983105cc 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -3381,6 +3381,8 @@ transformPartitionBound(ParseState *pstate, Relation 
parent,
   *cell2;
int i,
j;
+   PartitionRangeDatumKind lower_kind = 
PARTITION_RANGE_DATUM_VALUE,
+   
upper_kind = PARTITION_RANGE_DATUM_VALUE;
 
if (spec->strategy != PARTITION_STRATEGY_RANGE)
ereport(ERROR,
@@ -3426,7 +3428,17 @@ transformPartitionBound(ParseState *pstate, Relation 
parent,
coltype = get_partition_col_typid(key, i);
coltypmod = get_partition_col_typmod(key, i);
 
-   if (ldatum->value)
+   /*
+* If we've found the first infinite bound, use the 
same for
+* subsequent columns.
+*/
+   if (lower_kind != PARTITION_RANGE_DATUM_VALUE)
+   {
+   ldatum = copyObject(ldatum);/* don't 
scribble on input */
+   ldatum->kind = lower_kind;
+   ldatum->value = NULL;
+   }
+   else if (ldatum->value)
{
con = castNode(A_Const, ldatum->value);
value = transformPartitionBoundValue(pstate, 
con,
@@ -3439,8 +3451,20 @@ transformPartitionBound(ParseState *pstate, Relation 
parent,
ldatum = copyObject(ldatum);/* don't 
scribble on input */
ldatum->value = (Node *) value;
}
+   else
+   lower_kind = ldatum->kind;
 
-   if (rdatum->value)
+   /*
+* If we've found the first infinite bound, use the 
same for
+* subsequent columns.
+*/
+   if (upper_kind != PARTITION_RANGE_DATUM_VALUE)
+  

[HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Dean Rasheed]

On 14 September 2017 at 03:49, Noah Misch  wrote:
> On Wed, Sep 13, 2017 at 12:06:40PM -0400, Robert Haas wrote:
>> OK, thanks.  I still don't really like allowing this, but I can see
>> that compatibility with other systems has some value here, and if
>> nobody else is rejecting these cases, maybe we shouldn't either.  So
>> I'll hold my nose and change my vote to canonicalizing rather than
>> rejecting outright.
>
> I vote for rejecting it.  DDL compatibility is less valuable than other
> compatibility.  The hypothetical affected application can change its DDL to
> placate PostgreSQL and use that modified DDL for all other databases, too.

So we have 3 options:

1. Do nothing, allowing and keeping any values after a MINVALUE/MAXVALUE.

2. Allow the such values, but canonicalise what we store.

3. Reject anything other than MINVALUE/MAXVALUE after MINVALUE/MAXVALUE.


My order of preference is still (1), (2) then (3) because:

- Compatibility.
- Less verbose / easier to type.
- We allow multiple syntaxes for equivalent constraints in other places,
  without canonicalising what the user enters.
- Regarding Robert's coding argument, code in general should not be
  looking at and basing decisions on any values it sees after a
  MINVALUE/MAXVALUE. If it does, at the very least it's doing more work
  than it needs to, and at worst, there's a potential bug there which
  would be more readily exposed by allowing arbitrary values there. Code
  that only worked because because of earlier canonicalisation would
  strike me as being somewhat fragile.

However, it seems that there is a clear consensus towards (2) or (3)
and we have viable patches for each, although I'm not sure which of
those the consensus is really leaning towards.

Robert, since partitioning was originally your commit, and you know
the wider codebase better, I'm happy to go with whatever you decide.

Regards,
Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Dean Rasheed]

On 13 September 2017 at 14:51, Robert Haas  wrote:
> Coincidentally, I wrote a patch for this too, but mine goes back to
> rejecting MINVALUE or MAXVALUE followed by anything else.
>

LGTM, if we decide to go this way.

One minor review comment -- you missed an example code snippet using
(MINVALUE, 0) further down the page in create_table.sgml, which needs
updating.

Regards,
Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Dean Rasheed]

On 13 September 2017 at 10:05, Amit Langote
 wrote:
> Coincidentally, I just wrote the patch for canonicalizing stored values,
> instead of erroring out.  Please see attached if that's what you were
> thinking too.
>

Looks reasonable to me, if we decide to go this way.

One minor review comment -- it isn't really necessary to have the
separate new boolean local variables as well as the datum kind
variables. Just tracking the datum kind is sufficient and slightly
simpler. That would also address a slight worry I have that your
coding might result in a compiler warning about the kind variables
being used uninitialised with some less intelligent compilers, not
smart enough to work out that it can't actually happen.

Regards,
Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Noah Misch]

On Wed, Sep 13, 2017 at 12:06:40PM -0400, Robert Haas wrote:
> On Wed, Sep 13, 2017 at 10:54 AM, Dean Rasheed  
> wrote:
> > Oracle, MySQL and DB2 all use MINVALUE/MAXVALUE. Actually, Oracle and
> > MySQL only use MAXVALUE, not MINVALUE, because they don't allow gaps
> > between partitions and the first partition implicitly starts at
> > MINVALUE, so the bounds that we currently support are a strict
> > superset of those supported by Oracle and MySQL.
> >
> > Both Oracle and MySQL allow finite values after MAXVALUE (usually
> > listed as "0" in code examples, e.g. see [1]). Oracle explicitly
> > documents the fact that values after MAXVALUE are irrelevant in [1].
> > I'm not sure if MySQL explicitly documents that, but it does behave
> > the same.
> >
> > Also, both Oracle and MySQL store what the user entered (they do not
> > canonicalise), as can be seen by looking at ALL_TAB_PARTITIONS in
> > Oracle, or "show create table" in MySQL.
> 
> OK, thanks.  I still don't really like allowing this, but I can see
> that compatibility with other systems has some value here, and if
> nobody else is rejecting these cases, maybe we shouldn't either.  So
> I'll hold my nose and change my vote to canonicalizing rather than
> rejecting outright.

I vote for rejecting it.  DDL compatibility is less valuable than other
compatibility.  The hypothetical affected application can change its DDL to
placate PostgreSQL and use that modified DDL for all other databases, too.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Robert Haas]

On Wed, Sep 13, 2017 at 10:54 AM, Dean Rasheed  wrote:
> Oracle, MySQL and DB2 all use MINVALUE/MAXVALUE. Actually, Oracle and
> MySQL only use MAXVALUE, not MINVALUE, because they don't allow gaps
> between partitions and the first partition implicitly starts at
> MINVALUE, so the bounds that we currently support are a strict
> superset of those supported by Oracle and MySQL.
>
> Both Oracle and MySQL allow finite values after MAXVALUE (usually
> listed as "0" in code examples, e.g. see [1]). Oracle explicitly
> documents the fact that values after MAXVALUE are irrelevant in [1].
> I'm not sure if MySQL explicitly documents that, but it does behave
> the same.
>
> Also, both Oracle and MySQL store what the user entered (they do not
> canonicalise), as can be seen by looking at ALL_TAB_PARTITIONS in
> Oracle, or "show create table" in MySQL.

OK, thanks.  I still don't really like allowing this, but I can see
that compatibility with other systems has some value here, and if
nobody else is rejecting these cases, maybe we shouldn't either.  So
I'll hold my nose and change my vote to canonicalizing rather than
rejecting outright.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Dean Rasheed]

On 13 September 2017 at 14:53, Robert Haas  wrote:
> On Wed, Sep 13, 2017 at 4:51 AM, Dean Rasheed  
> wrote:
>> A drawback to doing this is that we lose compatibility with syntaxes
>> supported by other databases, which was part of the reason for
>> choosing the terms MINVALUE and MAXVALUE in the first place.
>>
>> So thinking about this afresh, my preference would actually be to just
>> canonicalise the values stored rather than erroring out.
>
> Can you be more specific about what other databases do here?  Which
> other systems support MINVALUE/MAXVALUE, and what are their respective
> behaviors in this situation?
>

Oracle, MySQL and DB2 all use MINVALUE/MAXVALUE. Actually, Oracle and
MySQL only use MAXVALUE, not MINVALUE, because they don't allow gaps
between partitions and the first partition implicitly starts at
MINVALUE, so the bounds that we currently support are a strict
superset of those supported by Oracle and MySQL.

Both Oracle and MySQL allow finite values after MAXVALUE (usually
listed as "0" in code examples, e.g. see [1]). Oracle explicitly
documents the fact that values after MAXVALUE are irrelevant in [1].
I'm not sure if MySQL explicitly documents that, but it does behave
the same.

Also, both Oracle and MySQL store what the user entered (they do not
canonicalise), as can be seen by looking at ALL_TAB_PARTITIONS in
Oracle, or "show create table" in MySQL.

I have not used DB2.

Regards,
Dean

[1] https://docs.oracle.com/cd/E18283_01/server.112/e16541/part_admin001.htm


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Robert Haas]

On Wed, Sep 13, 2017 at 5:05 AM, Amit Langote
 wrote:
>> So thinking about this afresh, my preference would actually be to just
>> canonicalise the values stored rather than erroring out.
>
> Coincidentally, I just wrote the patch for canonicalizing stored values,
> instead of erroring out.  Please see attached if that's what you were
> thinking too.

Coincidentally, I wrote a patch for this too, but mine goes back to
rejecting MINVALUE or MAXVALUE followed by anything else.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


enforce-minmaxvalue-consistency.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Robert Haas]

On Wed, Sep 13, 2017 at 4:51 AM, Dean Rasheed  wrote:
> A drawback to doing this is that we lose compatibility with syntaxes
> supported by other databases, which was part of the reason for
> choosing the terms MINVALUE and MAXVALUE in the first place.
>
> So thinking about this afresh, my preference would actually be to just
> canonicalise the values stored rather than erroring out.

Can you be more specific about what other databases do here?  Which
other systems support MINVALUE/MAXVALUE, and what are their respective
behaviors in this situation?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Amit Langote]

Hi Dean,

On 2017/09/13 17:51, Dean Rasheed wrote:
> Robert Haas  writes:
>> On Tue, Sep 12, 2017 at 9:58 AM, Alvaro Herrera  
>> wrote:
>>> Did anything happen on this, or did we just forget it completely?
>>
>> I forgot it.  :-(
>>
>> I really think we should fix this.
> 
> Ah, sorry. This was for me to follow up, and I dropped the ball.
> 
> Here's a patch restoring the original error checks (actually not the
> same coding as used in previous versions of the patch, because that
> would have allowed a MINVALUE after a MAXVALUE and vice versa).
> 
> A drawback to doing this is that we lose compatibility with syntaxes
> supported by other databases, which was part of the reason for
> choosing the terms MINVALUE and MAXVALUE in the first place.
> 
> So thinking about this afresh, my preference would actually be to just
> canonicalise the values stored rather than erroring out.
> 
> Thoughts?

Coincidentally, I just wrote the patch for canonicalizing stored values,
instead of erroring out.  Please see attached if that's what you were
thinking too.

Thanks,
Amit
From e5fc04c915cb98b0c56b234372ece4b9b1043033 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 13 Sep 2017 17:51:38 +0900
Subject: [PATCH] Canonicalize catalog representation of range partition bounds

Since the system effectively ignores values of the bound for
partition key columns following the first unbounded key, it's pointless
to accurately remember them in the system catalog.  Instead store
minvalue or maxvalue for all such columns, matching what the first
unbounded key is.  This makes \d output of range partitions look
more canonical.
---
 doc/src/sgml/ref/create_table.sgml |  6 +-
 src/backend/parser/parse_utilcmd.c | 30 --
 src/test/regress/expected/create_table.out |  6 +++---
 src/test/regress/expected/insert.out   |  8 
 4 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index 824253de40..7d2eb30722 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -328,7 +328,11 @@ FROM ( { numeric_literal | MAXVALUE in a partition bound are ignored; so the bound
   (10, MINVALUE, 0) is equivalent to
   (10, MINVALUE, 10) and (10, MINVALUE, MINVALUE)
-  and (10, MINVALUE, MAXVALUE).
+  and (10, MINVALUE, MAXVALUE).  Morever, the system will
+  store (10, MINVALUE, MINVALUE) into the system catalog if
+  the user specifies (10, MINVALUE, 0), and
+  (10, MAXVALUE, MAXVALUE) if the user specifies
+  (10, MAXVALUE, 0).
  
 
  
diff --git a/src/backend/parser/parse_utilcmd.c 
b/src/backend/parser/parse_utilcmd.c
index 655da02c10..9086ac90ef 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -3381,6 +3381,10 @@ transformPartitionBound(ParseState *pstate, Relation 
parent,
   *cell2;
int i,
j;
+   PartitionRangeDatumKind first_lower_unbounded_kind,
+   
first_upper_unbounded_kind;
+   boollower_unbounded_found = false,
+   upper_unbounded_found = false;
 
if (spec->strategy != PARTITION_STRATEGY_RANGE)
ereport(ERROR,
@@ -3426,7 +3430,13 @@ transformPartitionBound(ParseState *pstate, Relation 
parent,
coltype = get_partition_col_typid(key, i);
coltypmod = get_partition_col_typmod(key, i);
 
-   if (ldatum->value)
+   if (lower_unbounded_found)
+   {
+   ldatum = copyObject(ldatum);
+   ldatum->kind = first_lower_unbounded_kind;
+   ldatum->value = NULL;
+   }
+   else if (ldatum->value)
{
con = castNode(A_Const, ldatum->value);
value = transformPartitionBoundValue(pstate, 
con,
@@ -3439,8 +3449,19 @@ transformPartitionBound(ParseState *pstate, Relation 
parent,
ldatum = copyObject(ldatum);/* don't 
scribble on input */
ldatum->value = (Node *) value;
}
+   else
+   {
+   lower_unbounded_found = true;
+   first_lower_unbounded_kind = ldatum->kind;
+   }
 
-   if (rdatum->value)
+   if (upper_unbounded_found)
+   {
+   rdatum = 

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Dean Rasheed]

Robert Haas  writes:
> On Tue, Sep 12, 2017 at 9:58 AM, Alvaro Herrera  
> wrote:
>> Did anything happen on this, or did we just forget it completely?
>
> I forgot it.  :-(
>
> I really think we should fix this.

Ah, sorry. This was for me to follow up, and I dropped the ball.

Here's a patch restoring the original error checks (actually not the
same coding as used in previous versions of the patch, because that
would have allowed a MINVALUE after a MAXVALUE and vice versa).

A drawback to doing this is that we lose compatibility with syntaxes
supported by other databases, which was part of the reason for
choosing the terms MINVALUE and MAXVALUE in the first place.

So thinking about this afresh, my preference would actually be to just
canonicalise the values stored rather than erroring out.

Thoughts?

Regards,
Dean


restore-minvalue-maxvalue-error-checks.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Tom Lane]

Robert Haas  writes:
> On Tue, Sep 12, 2017 at 9:58 AM, Alvaro Herrera  
> wrote:
>> Did anything happen on this, or did we just forget it completely?

> I forgot it.  :-(

> I really think we should fix this.

+1.  You've got the rest of the week ...

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Robert Haas]

On Tue, Sep 12, 2017 at 9:58 AM, Alvaro Herrera  wrote:
> Robert Haas wrote:
>> I just don't understand why you think there should be multiple
>> spellings of the same bound.  Generally, canonicalization is good.
>> One of my fears here is that at least some people will get confused
>> and think a bound from (minvalue, 0) to (maxvalue, 10) allows any
>> value for the first column and a 0-9 value for the second column which
>> is wrong.
>>
>> My other fear is that, since you've not only allowed this into the
>> syntax but into the catalog, this will become a source of bugs for
>> years to come.  Every future patch that deals with partition bounds
>> will now have to worry about testing
>> unbounded-followed-by-non-unbounded.  If we're not going to put back
>> those error checks completely - and I think we should - we should at
>> least canonicalize what actually gets stored.
>
> Did anything happen on this, or did we just forget it completely?

I forgot it.  :-(

I really think we should fix this.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Alvaro Herrera]

Robert Haas wrote:

> I just don't understand why you think there should be multiple
> spellings of the same bound.  Generally, canonicalization is good.
> One of my fears here is that at least some people will get confused
> and think a bound from (minvalue, 0) to (maxvalue, 10) allows any
> value for the first column and a 0-9 value for the second column which
> is wrong.
> 
> My other fear is that, since you've not only allowed this into the
> syntax but into the catalog, this will become a source of bugs for
> years to come.  Every future patch that deals with partition bounds
> will now have to worry about testing
> unbounded-followed-by-non-unbounded.  If we're not going to put back
> those error checks completely - and I think we should - we should at
> least canonicalize what actually gets stored.

Did anything happen on this, or did we just forget it completely?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Robert Haas]

On Tue, Aug 8, 2017 at 7:33 PM, Dean Rasheed  wrote:
> Well perhaps verbosity-reduction isn't sufficient justification but I
> still think this is correct because logically any values in the bound
> after MINVALUE/MAXVALUE are irrelevant, so it seems overly restrictive
> to force all later values to be MINVALUE/MAXVALUE when the code will
> just ignore those values.

I just don't understand why you think there should be multiple
spellings of the same bound.  Generally, canonicalization is good.
One of my fears here is that at least some people will get confused
and think a bound from (minvalue, 0) to (maxvalue, 10) allows any
value for the first column and a 0-9 value for the second column which
is wrong.

My other fear is that, since you've not only allowed this into the
syntax but into the catalog, this will become a source of bugs for
years to come.  Every future patch that deals with partition bounds
will now have to worry about testing
unbounded-followed-by-non-unbounded.  If we're not going to put back
those error checks completely - and I think we should - we should at
least canonicalize what actually gets stored.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Amit Langote]

On 2017/08/09 9:03, David G. Johnston wrote:
> On Tue, Aug 8, 2017 at 4:33 PM, Dean Rasheed wrote:
>> Well perhaps verbosity-reduction isn't sufficient justification but I
>> still think this is correct because logically any values in the bound
>> after MINVALUE/MAXVALUE are irrelevant, so it seems overly restrictive
>> to force all later values to be MINVALUE/MAXVALUE when the code will
>> just ignore those values.
>>
>>
> But semantically using ​unbounded
> is correct - and referencing the principle of "write once, read many"
> people are probably going to care a lot more about the \d display than the
> original SQL - and \d showing some randomly chosen value and mentally doing
> the gymnastics to think "oh, wait, it doesn't actually mater what this
> value is)" compared to it showing "unbounded" and it being obvious that
> "any value" will work, seems like a win.
> 
>> I don't think we should allow values after MINVALUE/MAXVALUE to be
>> omitted altogether because we document multi-column ranges as being
>> most easily understood according to the rules of row-wise comparisons,
>> and they require equal numbers of values in each row.
>>
> 
> I wouldn't change the underlying representation but from a UX perspective
> being able to ?omit the explicit specification and let the system default
> appropriately would be worth considering - though probably not worth the
> effort.
> 
> ​The complaint regarding \d could be solved by figuring out on-the-fly
> whether the particular column is presently bounded or not and diplaying
> "unbounded" for the later ones regardless of what value is stored in the
> catalog.  "Unbounded (0)" would communicate both aspects.​  In the "be
> liberal in what you accept" department that would seem to be a win.

One of the patches posted on the thread where this development occurred
[1] implemented more or less this behavior.  That is, we would let the
users omit inconsequential values in the FROM and TO lists, but internally
store minvalue in the columns for which no value was specified. \d could
show those values as the catalog had it (minvalue).

Consider following example with the current syntax:

create table rp (a int, b int) partition by range (a, b);
create table rp0 partition of rp for values from (minvalue, minvalue) to
(1, minvalue);

\d+ rp0 shows:

Partition of: rp FOR VALUES FROM (MINVALUE, MINVALUE) TO (1, MINVALUE)
Partition constraint: ((a IS NOT NULL) AND (b IS NOT NULL) AND (a < 1))

With the aforementioned patch, the same partition could be created as:

create table rp0 partition of rp for values from (minvalue) to (1, minvalue);

or:

create table rp0 partition of rp for values from (minvalue) to (1);

Consider one more partition:

create table rp1 partition of rp for values from (1, minvalue) to (1, 1);

\d+ rp1 shows:

Partition of: rp FOR VALUES FROM (1, MINVALUE) TO (1, 1)
Partition constraint: ((a IS NOT NULL) AND (b IS NOT NULL) AND (a = 1) AND
(b < 1))

Same could be created with following alternative command:

create table rp1 partition of rp for values from (1) to (1, 1);

Regarding Dean's argument that it's hard to make sense of that syntax when
one considers that row-comparison logic is used which requires equal
number of columns to be present on both sides, since users are always able
to reveal what ROW expression looks like internally by describing a
partition, they can see what values are being used in the row comparisons,
including those of the columns for which they didn't specify any value
when creating the table.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/a6d6b752-3d08-ded8-3c8f-5cc9f090ec20%40lab.ntt.co.jp



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[David G. Johnston]

On Tue, Aug 8, 2017 at 4:33 PM, Dean Rasheed 
wrote:

> On 8 August 2017 at 19:22, Robert Haas  wrote:
> > On Fri, Jul 21, 2017 at 4:24 AM, Dean Rasheed 
> wrote:
> >> Also drop the constraint prohibiting finite values after an unbounded
> >> column, and just document the fact that any values after MINVALUE or
> >> MAXVALUE are ignored. Previously it was necessary to repeat UNBOUNDED
> >> multiple times, which was needlessly verbose.
> >
> > I would like to (belatedly) complain about this part of this commit.
> > Now you can do stuff like this:
> >
> > rhaas=# create table foo (a int, b text) partition by range (a, b);
> > CREATE TABLE
> > rhaas=# create table foo1 partition of foo for values from (minvalue,
> > 0) to (maxvalue, 0);
> > CREATE TABLE
> >
> > The inclusion of 0 has no effect, but it is still stored in the
> > catalog, shows up in \d foo1, and somebody might think it does
> > something.  I think we should go back to requiring bounds after
> > minvalue or maxvalue to be minvalue or maxvalue.
>

​The commit message indicates one would just specify "unbounded", not
min/maxvalue​, which indeed seems to be a better word for it.

Can we, presently, stick "null" in place of the "0"?

Well perhaps verbosity-reduction isn't sufficient justification but I
> still think this is correct because logically any values in the bound
> after MINVALUE/MAXVALUE are irrelevant, so it seems overly restrictive
> to force all later values to be MINVALUE/MAXVALUE when the code will
> just ignore those values.
>
>
But semantically using
​unbounded
 is correct - and referencing the principle of "write once, read many"
people are probably going to care a lot more about the \d display than the
original SQL - and \d showing some randomly chosen value and mentally doing
the gymnastics to think "oh, wait, it doesn't actually mater what this
value is)" compared to it showing "unbounded" and it being obvious that
"any value" will work, seems like a win.


> I don't think we should allow values after MINVALUE/MAXVALUE to be
> omitted altogether because we document multi-column ranges as being
> most easily understood according to the rules of row-wise comparisons,
> and they require equal numbers of values in each row.
>

I wouldn't change the underlying representation but from a UX perspective
being able to ?omit the explicit specification and let the system default
appropriately would be worth considering - though probably not worth the
effort.

​The complaint regarding \d could be solved by figuring out on-the-fly
whether the particular column is presently bounded or not and diplaying
"unbounded" for the later ones regardless of what value is stored in the
catalog.  "Unbounded (0)" would communicate both aspects.​  In the "be
liberal in what you accept" department that would seem to be a win.

​David J.​

[HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Dean Rasheed]

On 8 August 2017 at 19:22, Robert Haas  wrote:
> On Fri, Jul 21, 2017 at 4:24 AM, Dean Rasheed  
> wrote:
>> Also drop the constraint prohibiting finite values after an unbounded
>> column, and just document the fact that any values after MINVALUE or
>> MAXVALUE are ignored. Previously it was necessary to repeat UNBOUNDED
>> multiple times, which was needlessly verbose.
>
> I would like to (belatedly) complain about this part of this commit.
> Now you can do stuff like this:
>
> rhaas=# create table foo (a int, b text) partition by range (a, b);
> CREATE TABLE
> rhaas=# create table foo1 partition of foo for values from (minvalue,
> 0) to (maxvalue, 0);
> CREATE TABLE
>
> The inclusion of 0 has no effect, but it is still stored in the
> catalog, shows up in \d foo1, and somebody might think it does
> something.  I think we should go back to requiring bounds after
> minvalue or maxvalue to be minvalue or maxvalue.  I thought maybe the
> idea here was that you were going to allow something like this to
> work, which actually would have saved some typing:
>
> create table foo2 partition of foo for values from (minvalue) to (maxvalue);
>
> But no:
>
> ERROR:  FROM must specify exactly one value per partitioning column
>
> So the only way that this has made things less verbose is by letting
> you put in a meaningless value of the data type which takes fewer than
> 8 characters to type.  That doesn't seem to me to be a defensible way
> of reducing verbosity.
>

Well perhaps verbosity-reduction isn't sufficient justification but I
still think this is correct because logically any values in the bound
after MINVALUE/MAXVALUE are irrelevant, so it seems overly restrictive
to force all later values to be MINVALUE/MAXVALUE when the code will
just ignore those values.

I don't think we should allow values after MINVALUE/MAXVALUE to be
omitted altogether because we document multi-column ranges as being
most easily understood according to the rules of row-wise comparisons,
and they require equal numbers of values in each row.

It's also worth noting that this choice is consistent with other
databases, so at least some people will be used to being able to do
this.

Regards,
Dean


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Re: [COMMITTERS] pgsql: Use MINVALUE/MAXVALUE instead of UNBOUNDED for range partition b

[Robert Haas]

On Fri, Jul 21, 2017 at 4:24 AM, Dean Rasheed  wrote:
> Also drop the constraint prohibiting finite values after an unbounded
> column, and just document the fact that any values after MINVALUE or
> MAXVALUE are ignored. Previously it was necessary to repeat UNBOUNDED
> multiple times, which was needlessly verbose.

I would like to (belatedly) complain about this part of this commit.
Now you can do stuff like this:

rhaas=# create table foo (a int, b text) partition by range (a, b);
CREATE TABLE
rhaas=# create table foo1 partition of foo for values from (minvalue,
0) to (maxvalue, 0);
CREATE TABLE

The inclusion of 0 has no effect, but it is still stored in the
catalog, shows up in \d foo1, and somebody might think it does
something.  I think we should go back to requiring bounds after
minvalue or maxvalue to be minvalue or maxvalue.  I thought maybe the
idea here was that you were going to allow something like this to
work, which actually would have saved some typing:

create table foo2 partition of foo for values from (minvalue) to (maxvalue);

But no:

ERROR:  FROM must specify exactly one value per partitioning column

So the only way that this has made things less verbose is by letting
you put in a meaningless value of the data type which takes fewer than
8 characters to type.  That doesn't seem to me to be a defensible way
of reducing verbosity.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers