Re: Dimension limit in contrib/cube (dump/restore hazard?)
On Fri, Aug 31, 2018 at 6:18 PM Alvaro Herrera wrote: > On 2018-Aug-30, Alexander Korotkov wrote: > > > Personally I get tired with cube.out > > and cube_2.out. They are different with only few checks involving > > scientific notation. But all the patches touching cube regression > > tests should update both cube.out and cube_2.out. I propose to split > > scientific notation checks into separate test. > > Good idea. Thank you for the feedback! Pushed. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Dimension limit in contrib/cube (dump/restore hazard?)
On 2018-Aug-30, Alexander Korotkov wrote: > Personally I get tired with cube.out > and cube_2.out. They are different with only few checks involving > scientific notation. But all the patches touching cube regression > tests should update both cube.out and cube_2.out. I propose to split > scientific notation checks into separate test. Good idea. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Dimension limit in contrib/cube (dump/restore hazard?)
On Thu, Aug 30, 2018 at 02:28:20PM +0300, Alexander Korotkov wrote: > In general looks good for me. Personally I get tired with cube.out > and cube_2.out. They are different with only few checks involving > scientific notation. But all the patches touching cube regression > tests should update both cube.out and cube_2.out. I propose to split > scientific notation checks into separate test. +1. -- Michael signature.asc Description: PGP signature
Re: Dimension limit in contrib/cube (dump/restore hazard?)
On Thu, Aug 30, 2018 at 4:59 PM Tom Lane wrote: > Alexander Korotkov writes: > > I'm going to check this patchset on Windows and commit if no objections. > > These error messages do not conform to our message style guidelines: > you've copied an errdetail message as primary error message, but the > rules are different for that (no complete sentences, no initial cap, > no period). > > Using ERRCODE_ARRAY_ELEMENT_ERROR seems pretty random as well --- so far > as I can see, that's generally used for cases like "this array has the > wrong type of data elements". Perhaps ERRCODE_PROGRAM_LIMIT_EXCEEDED > would be the best choice. Thank you for catching this! I'll be more careful about error messages. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company commit 173eb698ffc0dcc69cb63a7a8f5fa9965acc0e8e Author: Alexander Korotkov Date: Thu Aug 30 14:09:25 2018 +0300 Split contrib/cube platform-depended checks into separate test We're currently maintaining two outputs for cube regression test. But that appears to be unsuitable, because these outputs are different in out few checks involving scientific notation. So, split checks involving scientific notation into separate test, making contrib/cube easier to maintain. diff --git a/contrib/cube/Makefile b/contrib/cube/Makefile index a679ac626ee..5e7b524dc22 100644 --- a/contrib/cube/Makefile +++ b/contrib/cube/Makefile @@ -11,7 +11,7 @@ PGFILEDESC = "cube - multidimensional cube data type" HEADERS = cubedata.h -REGRESS = cube +REGRESS = cube cube_sci EXTRA_CLEAN = y.tab.c y.tab.h diff --git a/contrib/cube/expected/cube.out b/contrib/cube/expected/cube.out index 6378db3004e..ac5f0bf7a8d 100644 --- a/contrib/cube/expected/cube.out +++ b/contrib/cube/expected/cube.out @@ -62,90 +62,6 @@ SELECT '-1.0'::cube AS cube; (-1) (1 row) -SELECT '1e27'::cube AS cube; - cube -- - (1e+27) -(1 row) - -SELECT '-1e27'::cube AS cube; - cube --- - (-1e+27) -(1 row) - -SELECT '1.0e27'::cube AS cube; - cube -- - (1e+27) -(1 row) - -SELECT '-1.0e27'::cube AS cube; - cube --- - (-1e+27) -(1 row) - -SELECT '1e+27'::cube AS cube; - cube -- - (1e+27) -(1 row) - -SELECT '-1e+27'::cube AS cube; - cube --- - (-1e+27) -(1 row) - -SELECT '1.0e+27'::cube AS cube; - cube -- - (1e+27) -(1 row) - -SELECT '-1.0e+27'::cube AS cube; - cube --- - (-1e+27) -(1 row) - -SELECT '1e-7'::cube AS cube; - cube -- - (1e-07) -(1 row) - -SELECT '-1e-7'::cube AS cube; - cube --- - (-1e-07) -(1 row) - -SELECT '1.0e-7'::cube AS cube; - cube -- - (1e-07) -(1 row) - -SELECT '-1.0e-7'::cube AS cube; - cube --- - (-1e-07) -(1 row) - -SELECT '1e-300'::cube AS cube; - cube --- - (1e-300) -(1 row) - -SELECT '-1e-300'::cube AS cube; - cube - (-1e-300) -(1 row) - SELECT 'infinity'::cube AS cube; cube @@ -164,24 +80,6 @@ SELECT 'NaN'::cube AS cube; (NaN) (1 row) -SELECT '1234567890123456'::cube AS cube; - cube - - (1.23456789012346e+15) -(1 row) - -SELECT '+1234567890123456'::cube AS cube; - cube - - (1.23456789012346e+15) -(1 row) - -SELECT '-1234567890123456'::cube AS cube; - cube -- - (-1.23456789012346e+15) -(1 row) - SELECT '.1234567890123456'::cube AS cube; cube - diff --git a/contrib/cube/expected/cube_2.out b/contrib/cube/expected/cube_2.out deleted file mode 100644 index 75fe405c497..000 --- a/contrib/cube/expected/cube_2.out +++ /dev/null @@ -1,2006 +0,0 @@ --- --- Test cube datatype --- -CREATE EXTENSION cube; --- Check whether any of our opclasses fail amvalidate -SELECT amname, opcname -FROM pg_opclass opc LEFT JOIN pg_am am ON am.oid = opcmethod -WHERE opc.oid >= 16384 AND NOT amvalidate(opc.oid); - amname | opcname -+- -(0 rows) - --- --- testing the input and output functions --- --- Any number (a one-dimensional point) -SELECT '1'::cube AS cube; - cube --- - (1) -(1 row) - -SELECT '-1'::cube AS cube; - cube --- - (-1) -(1 row) - -SELECT '1.'::cube AS cube; - cube --- - (1) -(1 row) - -SELECT '-1.'::cube AS cube; - cube --- - (-1) -(1 row) - -SELECT '.1'::cube AS cube; - cube - (0.1) -(1 row) - -SELECT '-.1'::cube AS cube; - cube - - (-0.1) -(1 row) - -SELECT '1.0'::cube AS cube; - cube --- - (1) -(1 row) - -SELECT '-1.0'::cube AS cube; - cube --- - (-1) -(1 row) - -SELECT '1e27'::cube AS cube; - cube --- - (1e+027) -(1 row) - -SELECT '-1e27'::cube AS cube; - cube - (-1e+027) -(1 row) - -SELECT '1.0e27'::cube AS cube; - cube --- - (1e+027) -(1 row) - -SELECT '-1.0e27'::cube AS cube; - cube -
Re: Dimension limit in contrib/cube (dump/restore hazard?)
Alexander Korotkov writes: > I'm going to check this patchset on Windows and commit if no objections. These error messages do not conform to our message style guidelines: you've copied an errdetail message as primary error message, but the rules are different for that (no complete sentences, no initial cap, no period). Using ERRCODE_ARRAY_ELEMENT_ERROR seems pretty random as well --- so far as I can see, that's generally used for cases like "this array has the wrong type of data elements". Perhaps ERRCODE_PROGRAM_LIMIT_EXCEEDED would be the best choice. regards, tom lane
Re: Dimension limit in contrib/cube (dump/restore hazard?)
Hi! On Tue, Aug 28, 2018 at 10:30 PM Andrey Borodin wrote: > > 28 авг. 2018 г., в 14:18, Alexander Korotkov > > написал(а): > > > > OK, but I think cube_c_f8() and cube_c_f8_f8() also need to be > > revised. Also, I think this behavior should be covered by regression > > tests. > True. Also there's one case in cube_subset. In general looks good for me. Personally I get tired with cube.out and cube_2.out. They are different with only few checks involving scientific notation. But all the patches touching cube regression tests should update both cube.out and cube_2.out. I propose to split scientific notation checks into separate test. I've also add check for sube_subset(). I'm going to check this patchset on Windows and commit if no objections. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company 0001-cube-split-scientific-notation-test-v1.patch Description: Binary data 0002-cube-enforce-dimension-checks-v1.patch Description: Binary data
Re: Dimension limit in contrib/cube (dump/restore hazard?)
> 28 авг. 2018 г., в 14:18, Alexander Korotkov > написал(а): > > OK, but I think cube_c_f8() and cube_c_f8_f8() also need to be > revised. Also, I think this behavior should be covered by regression > tests. True. Also there's one case in cube_subset. Best regards, Andrey Borodin. cube_check_dim.diff Description: Binary data
Re: Dimension limit in contrib/cube (dump/restore hazard?)
Hi! On Tue, Aug 28, 2018 at 6:21 PM Andrey Borodin wrote: > I belive cube construction from array\arrays should check size of arrays. Makes sense to me. > Also there are some unexpected cube dimensionality reduction like in > cube_enlarge > if (n > CUBE_MAX_DIM) > n = CUBE_MAX_DIM; > You wanted larger cube, but got cube of another dimension. > > I think we should something like this OK, but I think cube_c_f8() and cube_c_f8_f8() also need to be revised. Also, I think this behavior should be covered by regression tests. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: Dimension limit in contrib/cube (dump/restore hazard?)
Hi!
> 28 авг. 2018 г., в 8:29, Andrew Gierth
> написал(а):
>
> contrib/cube has an arbitrary limit of 100 on the number of dimensions
> in a cube, but it actually enforces that only in cube_in and
> cube_enlarge, with the other cube creation functions happy to create
> cubes of more dimensions.
>
> I haven't actually tested, but this implies that one can create cubes
> that will break dump/restore.
>
> Should this limit be kept, and if so what should it be? (There's
> obviously a limit on the size of indexable cubes)
>
> (Noticed because an irc user was trying to use cubes with 512
> dimensions with partial success)
+1
This can cause very unpleasant fails like
postgres=# create table y as select cube(array(SELECT random() as a FROM
generate_series(1,1000))) from generate_series(1,1e3,1);
SELECT 1000
postgres=# create index on y using gist(cube );
ERROR: index row size 8016 exceeds maximum 8152 for index "y_cube_idx"
postgres=# create table y as select cube(array(SELECT random() as a FROM
generate_series(1,800))) from generate_series(1,1e3,1);
SELECT 1000
postgres=# create index on y using gist(cube );
ERROR: failed to add item to index page in "y_cube_idx"
I belive cube construction from array\arrays should check size of arrays.
Also there are some unexpected cube dimensionality reduction like in
cube_enlarge
if (n > CUBE_MAX_DIM)
n = CUBE_MAX_DIM;
You wanted larger cube, but got cube of another dimension.
I think we should something like this
diff --git a/contrib/cube/cube.c b/contrib/cube/cube.c
index dfa8465d74..38739b1df2 100644
--- a/contrib/cube/cube.c
+++ b/contrib/cube/cube.c
@@ -151,6 +151,12 @@ cube_a_f8_f8(PG_FUNCTION_ARGS)
errmsg("cannot work with arrays containing
NULLs")));
dim = ARRNELEMS(ur);
+ if (dim > CUBE_MAX_DIM)
+ ereport(ERROR,
+ (errcode(ERRCODE_ARRAY_ELEMENT_ERROR),
+errmsg("A cube cannot have more than %d
dimensions.",
+ CUBE_MAX_DIM)));
+
if (ARRNELEMS(ll) != dim)
ereport(ERROR,
(errcode(ERRCODE_ARRAY_ELEMENT_ERROR),
@@ -208,6 +214,11 @@ cube_a_f8(PG_FUNCTION_ARGS)
errmsg("cannot work with arrays containing
NULLs")));
dim = ARRNELEMS(ur);
+ if (dim > CUBE_MAX_DIM)
+ ereport(ERROR,
+ (errcode(ERRCODE_ARRAY_ELEMENT_ERROR),
+errmsg("A cube cannot have more than %d
dimensions.",
+ CUBE_MAX_DIM)));
dur = ARRPTR(ur);
Best regards, Andrey Borodin.
Dimension limit in contrib/cube (dump/restore hazard?)
contrib/cube has an arbitrary limit of 100 on the number of dimensions in a cube, but it actually enforces that only in cube_in and cube_enlarge, with the other cube creation functions happy to create cubes of more dimensions. I haven't actually tested, but this implies that one can create cubes that will break dump/restore. Should this limit be kept, and if so what should it be? (There's obviously a limit on the size of indexable cubes) (Noticed because an irc user was trying to use cubes with 512 dimensions with partial success) -- Andrew (irc:RhodiumToad)
