Home > mailing lists

Re: [HACKERS] Default Partition for Range - Mailing list pgsql-hackers

From	Rahila Syed
Subject	Re: [HACKERS] Default Partition for Range
Date	July 4, 2017 16:51:33
Msg-id	CAH2L28v_k+_s=O1SWpm-OHuK=+60vP5R8ARNUKt+V0FBxF6RXg@mail.gmail.com Whole thread Raw
In response to	Re: [HACKERS] Default Partition for Range (Dilip Kumar <dilipbalaut@gmail.com>)
Responses	Re: [HACKERS] Default Partition for Range
List	pgsql-hackers

Tree view

Hello Beena,

Thanks for the updated patch. It passes the basic tests which I performed.

Few comments:

1. In check_new_partition_bound(),

> /* Default case is not handled here */
>                if (spec->is_default)
>                    break;
The default partition check here can be performed in common for both range and list partition.

2.     RANGE_DATUM_FINITE = 0,     /* actual datum stored elsewhere */
+   RANGE_DATUM_DEFAULT,        /* default */

More elaborative comment?

3. Inside make_one_range_bound(),

>+
>+   /* datums are NULL for default */
>+   if (datums == NULL)
>+       for (i = 0; i < key->partnatts; i++)
>+           bound->content[i] = RANGE_DATUM_DEFAULT;
>+
IMO, returning bound at this stage will make code clearer as further processing
does not happen for default partition.

4.
@@ -549,6 +568,7 @@ RelationBuildPartitionDesc(Relation rel)
                                                sizeof(RangeDatumContent *));
                    boundinfo->indexes = (int *) palloc((ndatums + 1) *
                                                        sizeof(int));
+                   boundinfo->default_index = -1;

This should also be done commonly for both default list and range partition.

5.
              if (!spec->is_default && partdesc->nparts > 0)
                {
                    ListCell   *cell;

                    Assert(boundinfo &&
                           boundinfo->strategy == PARTITION_STRATEGY_LIST &&
                           (boundinfo->ndatums > 0 ||
                            partition_bound_accepts_nulls(boundinfo) ||
                            partition_bound_has_default(boundinfo)));
The assertion condition partition_bound_has_default(boundinfo) is rendered useless

because of if condition change and can be removed perhaps.

6. I think its better to have following regression tests coverage

--Testing with inserting one NULL and other non NULL values for multicolumn range partitioned table.

postgres=# CREATE TABLE range_parted (
postgres(#     a int,
postgres(#     b int
postgres(# ) PARTITION BY RANGE (a, b);
CREATE TABLE
postgres=#
postgres=# CREATE TABLE part1 (
postgres(#     a int NOT NULL CHECK (a = 1),
postgres(#     b int NOT NULL CHECK (b >= 1 AND b <= 10)
postgres(# );
CREATE TABLE

postgres=# ALTER TABLE range_parted ATTACH PARTITION part1 FOR VALUES FROM (1, 1) TO (1, 10);
ALTER TABLE
postgres=# CREATE TABLE partr_def1 PARTITION OF range_parted DEFAULT;
CREATE TABLE

postgres=# insert into range_parted values (1,NULL);
INSERT 0 1
postgres=# insert into range_parted values (NULL,9);
INSERT 0 1

--Testing the boundary conditions like in the above example

following should pass.

postgres=# insert into partr_def1 values (1,10);
INSERT 0 1

Thank you,

Rahila Syed

On Mon, Jul 3, 2017 at 8:00 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:

On Fri, Jun 30, 2017 at 11:56 AM, Beena Emerson <memissemerson@gmail.com> wrote:
> Hello Dilip,
>
> On Wed, Jun 21, 2017 at 6:27 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>> On Tue, Jun 20, 2017 at 6:57 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>>> This is basically crashing in RelationBuildPartitionDesc, so I think

>
> Thank you for your review and analysis.
>
> I have updated the patch.
> - bound->content is set to RANGE_DATUM_DEFAULT for each of the keys
> and not just the first one.
> - Improve the way of handling DEFAULT bounds in RelationBuildPartitionDesc,
>
> There is a test for multiple column range in alter_table.sql

Thanks for update patch, After this fix segmentation fault is solved.

I have some more comments.

1.

lower = make_one_range_bound(key, -1, spec->lowerdatums, true);
upper = make_one_range_bound(key, -1, spec->upperdatums, false);

+ /* Default case is not handled here */
+ if (spec->is_default)
+ break;
+

I think we can move default check just above the make range bound it
will avoid unnecessary processing.

2.
RelationBuildPartitionDesc
{
....

/*
* If either of them has infinite element, we can't equate
* them. Even when both are infinite, they'd have
* opposite signs, because only one of cur and prev is a
* lower bound).
*/
if (cur->content[j] != RANGE_DATUM_FINITE ||
prev->content[j] != RANGE_DATUM_FINITE)
{
is_distinct = true;
break;
}
After default range partitioning patch the above comment doesn't sound
very accurate and
can lead to the confusion, now here we are not only considering
infinite bounds but
there is also a possibility that prev bound can be DEFAULT, so
comments should clearly
mention that.

3.

+ bound->datums = datums ? (Datum *) palloc0(key->partnatts * sizeof(Datum))
+ : NULL;
bound->content = (RangeDatumContent *) palloc0(key->partnatts *
sizeof(RangeDatumContent));
bound->lower = lower;

i = 0;
+
+ /* datums are NULL for default */
+ if (datums == NULL)
+ for (i = 0; i < key->partnatts; i++)
+ bound->content[i] = RANGE_DATUM_DEFAULT;

To me, it will look cleaner if we keep bound->content=NULL for
default, instead of allocating memory
and initializing each element, But it's just a suggestions and you
can decide whichever way
seems better to you. Then the other places e.g.
+ if (cur->content[i] == RANGE_DATUM_DEFAULT)
+ {
+ /*

will change like

+ if (cur->content == NULL)
+ {
+ /*

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

pgsql-hackers by date:

From: Amit Khandekar
Date: 04 July 2017, 15:53:05
Subject: Re: [HACKERS] UPDATE of partition key

From: AP
Date: 04 July 2017, 16:57:28
Subject: [HACKERS] pgsql 10: hash indexes testing

Re: [HACKERS] Default Partition for Range - Mailing list pgsql-hackers

Previous

Next