Hello,
On Fri, May 12, 2017 at 5:30 PM, Rahila Syed <rahilasye...@gmail.com> wrote: > Hello, > > >(1) With the new patch, we allow new partitions when there is overlapping > data with default partition. The entries in default are ignored when > running queries satisfying the new partition. > This was introduced in latest version. We are not allowing adding a > partition when entries with same key value exist in default partition. So > this scenario should not > come in picture. Please find attached an updated patch which corrects this. > Thank you for the updated patch. However, now I cannot create a partition after default. CREATE TABLE list1 ( a int, b int ) PARTITION BY LIST (a); CREATE TABLE list1_1 (LIKE list1); ALTER TABLE list1 ATTACH PARTITION list1_1 FOR VALUES IN (1); CREATE TABLE list1_def PARTITION OF list1 DEFAULT; CREATE TABLE list1_5 PARTITION OF list1 FOR VALUES IN (3); server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. The connection to the server was lost. Attempting reset: Failed. !> > > > >(2) I get the following warning: > > >partition.c: In function ‘check_new_partition_bound’: > >partition.c:882:15: warning: ‘boundinfo’ may be used uninitialized in > this function [-Wmaybe-uninitialized] > > && boundinfo->has_default) > ^ > >preproc.y:3250.2-8: warning: type clash on default action: <str> != <> > I failed to notice this warning. I will look into it. > > >This is incredibly ugly. I don't know exactly what should be done > >about it, but I think PARTITION_DEFAULT is a bad idea and has got to > >go. Maybe add a separate isDefault flag to PartitionBoundSpec > Will look at other ways to do it. > > >Doesn't it strike you as a bit strange that get_qual_for_default() > >doesn't return a qual? Functions should generally have names that > >describe what they do. > Will fix this. > > >There's an existing function that you can use to concatenate two lists > >instead of open-coding it. > Will check this. > > >you should really add the documentation and > >regression tests which you mentioned as a TODO. And run the code > >through pgindent > I will also update the next version with documentation and regression tests > and run pgindent > > Thank you, > Rahila Syed > > On Fri, May 12, 2017 at 4:33 PM, Beena Emerson <memissemer...@gmail.com> > wrote: > >> >> >> On Thu, May 11, 2017 at 7:37 PM, Rahila Syed <rahilasye...@gmail.com> >> wrote: >> >>> Hello, >>> >>> Please find attached an updated patch with review comments and bugs >>> reported till date implemented. >>> >> >> Hello Rahila, >> >> Tested on "efa2c18 Doc fix: scale(numeric) returns integer, not numeric." >> >> (1) With the new patch, we allow new partitions when there is overlapping >> data with default partition. The entries in default are ignored when >> running queries satisfying the new partition. >> >> DROP TABLE list1; >> CREATE TABLE list1 ( >> a int, >> b int >> ) PARTITION BY LIST (a); >> CREATE TABLE list1_1 (LIKE list1); >> ALTER TABLE list1 ATTACH PARTITION list1_1 FOR VALUES IN (1); >> CREATE TABLE list1_def PARTITION OF list1 DEFAULT; >> INSERT INTO list1 SELECT generate_series(1,2),1; >> -- Partition overlapping with DEF >> CREATE TABLE list1_2 PARTITION OF list1 FOR VALUES IN (2); >> INSERT INTO list1 SELECT generate_series(2,3),2; >> >> postgres=# SELECT * FROM list1 ORDER BY a,b; >> a | b >> ---+--- >> 1 | 1 >> 2 | 1 >> 2 | 2 >> 3 | 2 >> (4 rows) >> >> postgres=# SELECT * FROM list1 WHERE a=2; >> a | b >> ---+--- >> 2 | 2 >> (1 row) >> >> This ignores the a=2 entries in the DEFAULT. >> >> postgres=# SELECT * FROM list1_def; >> a | b >> ---+--- >> 2 | 1 >> 3 | 2 >> (2 rows) >> >> >> (2) I get the following warning: >> >> partition.c: In function ‘check_new_partition_bound’: >> partition.c:882:15: warning: ‘boundinfo’ may be used uninitialized in >> this function [-Wmaybe-uninitialized] >> && boundinfo->has_default) >> ^ >> preproc.y:3250.2-8: warning: type clash on default action: <str> != <> >> >> >>> >1. >>> >In following block, we can just do with def_index, and we do not need >>> found_def >>> >flag. We can check if def_index is -1 or not to decide if default >>> partition is >>> >present. >>> found_def is used to set boundinfo->has_default which is used at couple >>> of other places to check if default partition exists. The implementation >>> is similar >>> to has_null. >>> >>> >3. >>> >In following function isDefaultPartitionBound, first statement "return >>> false" >>> >is not needed. >>> It is needed to return false if the node is not DefElem. >>> >>> Todo: >>> Add regression tests >>> Documentation >>> >>> Thank you, >>> Rahila Syed >>> >>> >>> > -- Beena Emerson EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company