Re: [HACKERS] Default Partition for Range
| От | Rahila Syed |
|---|---|
| Тема | Re: [HACKERS] Default Partition for Range |
| Дата | |
| Msg-id | CAH2L28v_k+_s=O1SWpm-OHuK=+60vP5R8ARNUKt+V0FBxF6RXg@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: [HACKERS] Default Partition for Range (Dilip Kumar <dilipbalaut@gmail.com>) |
| Ответы |
Re: [HACKERS] Default Partition for Range
(Beena Emerson <memissemerson@gmail.com>)
|
| Список | pgsql-hackers |
Hello Beena,
> /* 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
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 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
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)
+ {
+ /*
В списке pgsql-hackers по дате отправления: