Subject: Re: Default Partition for Range

From: Dilip Kumar <[email-masked]>
Date: Mon, 3 Jul 2017 20:00:21 +0530

On Fri, Jun 30, 2017 at 11:56 AM, Beena Emerson <[email-masked]> wrote:
> Hello Dilip,
>
> On Wed, Jun 21, 2017 at 6:27 PM, Dilip Kumar <[email-masked]> wrote:
>> On Tue, Jun 20, 2017 at 6:57 PM, Dilip Kumar <[email-masked]> 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

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

---

From: Rahila Syed <[email-masked]>
Date: Tue, 4 Jul 2017 16:21:33 +0530

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(),

>+           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

---

From: Beena Emerson <[email-masked]>
Date: Fri, 14 Jul 2017 15:28:18 +0530

> will avoid unnecessary processing.

Removed the check here. Default is checked beforehand.

> there is also a possibility that prev bound can be DEFAULT, so

Modified.

> + if (cur->content == NULL)

I disagree. We use the content value RANGE_DATUM_DEFAULT during
comparing in partition_rbound_cmp and the code will not be very
intuiative if we use NULL instead of DEFAULT.

-- 

Beena Emerson

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

---

From: Beena Emerson <[email-masked]>
Date: Fri, 14 Jul 2017 15:28:33 +0530

> The default partition check here can be performed in common for both range

Removed the check here.

> More elaborative comment?

I am not sure what to add. Do you have something in mind?

> does not happen for default partition.

Done.

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

Removed the line here. Common allocation is done by Jeevan's patch.

> because of if condition change and can be removed perhaps.

This cannot be removed.
This check is required when this code is run for a non-default
partition and default is the only existing partition.

> postgres=# insert into range_parted values (NULL,9);

added.

-- 

Beena Emerson

---

From: Jeevan Ladhe <[email-masked]>
Date: Wed, 26 Jul 2017 19:05:09 +0530

Hi Beena,

I have posted the rebased patches[1] for default list partition.
Your patch also needs a rebase.

[1]
https://www.postgresql.org/message-id/CAOgcT0OVwDu%2BbeChWb5R5s6rfKLCiWcZT5617hqu7T3GdA1hAw%40mail.gmail.com

Regards,
Jeevan Ladhe

---

From: Beena Emerson <[email-masked]>
Date: Mon, 31 Jul 2017 17:58:51 +0530

> https://www.postgresql.org/message-id/CAOgcT0OVwDu%2BbeChWb5R5s6rfKLCiWcZT5617hqu7T3GdA1hAw%40mail.gmail.com

Thanks for informing.
PFA the updated patch.
I have changed the numbering of enum PartitionRangeDatumKind since I
have to include DEFAULT as well. If you have better ideas, let me
know.

-- 

Beena Emerson

---

From: Robert Haas <[email-masked]>
Date: Fri, 4 Aug 2017 10:18:45 -0400

> have to include DEFAULT as well. If you have better ideas, let me

Why do we need to introduce PARTITION_RANGE_DATUM_DEFAULT at all?  It
seems to me that the handling of default range partitions ought to be
similar to the way a null-accepting list partition is handled -
namely, it wouldn't show up in the "datums" or "kind" array at all,
instead just showing up in PartitionBoundInfoData's default_index
field.

-- 
Robert Haas

---

From: Beena Emerson <[email-masked]>
Date: Wed, 9 Aug 2017 08:26:00 +0530

> instead just showing up in PartitionBoundInfoData's default_index

I have updated the patch to make it similar to the way default/null is
handled in list partition, removing the PARTITION_RANGE_DATUM_DEFAULT.
This is to be applied over v24 patches shared by Jeevan [1] which
applies on commit id 5ff3d73813ebcc3ff80be77c30b458d728951036.

The RelationBuildPartitionDesc has been modified a lot, especially the
way all_bounds, ndatums and rbounds are set.

[1] https://www.postgresql.org/message-id/CAOgcT0OVwDu%2BbeChWb5R5s6rfKLCiWcZT5617hqu7T3GdA1hAw%40mail.gmail.com

-- 

Beena Emerson

---

From: Rajkumar Raghuwanshi <[email-masked]>
Date: Wed, 9 Aug 2017 12:37:43 +0530

On Wed, Aug 9, 2017 at 8:26 AM, Beena Emerson <[email-masked]>
wrote:

> [1] https://www.postgresql.org/message-id/CAOgcT0OVwDu%
> 2BbeChWb5R5s6rfKLCiWcZT5617hqu7T3GdA1hAw%40mail.gmail.com
>
>
Hi Beena,

I have applied Jeevan's v24 patches and then your v9 patch over commit
5ff3d73813ebcc3ff80be77c30b458d728951036.
and while testing I got a server crash. below is sql to reproduce it.

postgres=# CREATE TABLE rp (a int, b int) PARTITION by range (a);
CREATE TABLE
postgres=# CREATE TABLE rp_p1 PARTITION OF rp DEFAULT partition by range(a);
CREATE TABLE
postgres=# CREATE TABLE rp_p11 PARTITION OF rp_p1 FOR VALUES FROM (1) TO
(15);
CREATE TABLE
postgres=# CREATE TABLE rp_p12 PARTITION OF rp_p1 DEFAULT;
CREATE TABLE
postgres=# insert into rp select i,i from generate_series(1,15) i;
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.

Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation

---

From: Beena Emerson <[email-masked]>
Date: Wed, 9 Aug 2017 13:54:32 +0530

Hello Rajkumar,

> The connection to the server was lost. Attempting reset: Failed.

Thank you for testing. It seems I made a mistake in the assert
condition. I have corrected it in this patch.

Thank you,

Beena Emerson

---

From: Rajkumar Raghuwanshi <[email-masked]>
Date: Wed, 9 Aug 2017 17:48:48 +0530

On Wed, Aug 9, 2017 at 1:54 PM, Beena Emerson <[email-masked]>
wrote:

> Thanks Beena, I have tested new patch, crash got fixed now, got two
observations, please check if these are expected?

--difference in the description of default partition in case of list vs
range

create table lp (a int) partition by list(a);
create table lp_d partition of lp DEFAULT;
postgres=# \d+ lp_d
                                   Table "public.lp_d"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
 a      | integer |           |          |         | plain   |
|
Partition of: lp DEFAULT

*Partition constraint:*
create table rp (a int) partition by range(a);
create table rp_d partition of rp DEFAULT;
postgres=# \d+ rp_d
                                   Table "public.rp_d"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
--------+---------+-----------+----------+---------+---------+--------------+-------------
 a      | integer |           |          |         | plain   |
|
Partition of: rp DEFAULT

*Partition constraint: true*
--getting warning WARNING:  skipped scanning foreign table...which is a
partition of default partition...
--when adding new partition after adding default as foreign partition

CREATE EXTENSION postgres_fdw;
CREATE SERVER def_server FOREIGN DATA WRAPPER postgres_fdw OPTIONS (dbname
'postgres', port '5432', use_remote_estimate 'true');
CREATE USER MAPPING FOR PUBLIC SERVER def_server;

postgres=# CREATE TABLE frp (a int, b int) PARTITION BY RANGE(a);
CREATE TABLE
postgres=# CREATE TABLE frp_d PARTITION OF frp DEFAULT partition by
RANGE(b);
CREATE TABLE
postgres=# CREATE TABLE ffrp_d_d (like frp);
CREATE TABLE
postgres=# CREATE FOREIGN TABLE ftfrp_d_d PARTITION OF frp_d DEFAULT SERVER
def_server OPTIONS (TABLE_NAME 'ffrp_d_d');
CREATE FOREIGN TABLE
postgres=# CREATE TABLE frp_p2 PARTITION OF frp FOR VALUES FROM (10) TO
(12);
WARNING:  skipped scanning foreign table "ftfrp_d_d" which is a partition
of default partition "frp_d"
CREATE TABLE

Thanks & Regards,
Rajkumar Raghuwanshi

---

From: Robert Haas <[email-masked]>
Date: Wed, 9 Aug 2017 08:23:48 -0400

On Wed, Aug 9, 2017 at 8:18 AM, Rajkumar Raghuwanshi
<[email-masked]> wrote:
> Partition constraint:
> Partition constraint: true

This looks like a problem with the default list partitioning patch.  I
think "true" is what we expect to see here in both cases.

---

From: Jeevan Ladhe <[email-masked]>
Date: Thu, 10 Aug 2017 16:26:12 +0530

Hi Robert, Beena,

> >  Column |  Type   | Collation | Nullable | Default | Storage | Stats

In case of list partition if there is only default partition, then there is
no
specific value set that can be excluded from default partition, so in
such a case DEFAULT partition for a list partitioned table simply will
not have any constraint on it, and hence while the describe output is
printed it does not have any constraints to be printed.

Whereas, in case of default partition for a range partitioned table, in
get_qual_for_range() it creates true boolean expression if default is the
only partition. Here is the relevent code from Beena's patch:

+ else /* default is the only partition */
+ result = list_make1(makeBoolConst(true, false));

I think, it is unnecessary to create a constraint that is going to be always
true. Instead, if we have no constraints we can avoid some extra cpu
cycles which would otherwise be wasted in evaluating an expression
that is going to be always true.

Having said this, I see a point that in such a case we should have
some neat meaningful content to be printed for "Partition constraint: ".
I think we can address this when we construct describe output string.

Some ideas that come to my mind are:
Partition constraint: NIL
Partition constraint: no constraints
No partition constraint.
Partition constraint: true

Please let me know your thoughts.

Regards,
Jeevan Ladhe

---

From: Robert Haas <[email-masked]>
Date: Thu, 10 Aug 2017 13:59:29 -0400

> that is going to be always true.

That's a reasonable argument.  I'm not sure whether having no
partition constraint at all is likely to be a source of bugs, but
certainly, not needing to check it is appealing.

> Please let me know your thoughts.

I like "No partition constraint." of those options.

---

From: Beena Emerson <[email-masked]>
Date: Thu, 17 Aug 2017 14:04:25 +0530

Hello,

PFA the updated patch which returns NULL instead of true when the
default partition has no constraints and also have modified the output
as discussed above.

This applies over v24 patch [1] of default list partition. I will
rebase over the next version when it is updated.

Beena Emerson

---

From: Beena Emerson <[email-masked]>
Date: Thu, 17 Aug 2017 16:59:14 +0530

PFA the patch rebased over v25 patches of default list partition [1]

[1] https://www.postgresql.org/message-id/CAOgcT0NwqnavYtu-QM-DAZ6N%3DwTiqKgy83WwtO2x94LSLZ1-Sw%40mail.gmail.com

-- 

Beena Emerson

---

From: Jeevan Ladhe <[email-masked]>
Date: Tue, 22 Aug 2017 07:53:18 +0530

Hi Beena,

> PFA the patch rebased over v25 patches of default list partition [1]

Thanks for rebasing.

Range partition review:

1.
There are lot of changes in RelationBuildPartitionDesc(). It was hard to
understand why these changes are needed for default partition. I did not
find
any explanation for the same, may be I am missing some discussion? Only
when I looked into it carefully I could understand that these changes are
nothing but optimization for identifying the distinct bounds.
I think it is better to keep the patch for code optimisation/simplification
in a
separate patch. I have separated the patch(0001) in attached patches for
this
purpose.

2.
+ * For default partition, it returns the negation of the constraints of all
+ * the other partitions.
+ *
+ * If we end up with an empty result list, we return NULL.

We can rephrase the comment as below:

"For default partition, function returns the negation of the constraints of
all
the other partitions. If default is the only partition then returns NULL."

3.
@@ -2396,6 +2456,8 @@ make_one_range_bound(PartitionKey key, int index,
List *datums, bool lower)
    ListCell   *lc;
    int         i;

+   Assert(datums != NULL);
+
    bound = (PartitionRangeBound *) palloc0(sizeof(PartitionRangeBound));
    bound->index = index;
    bound->datums = (Datum *) palloc0(key->partnatts * sizeof(Datum));

I am not really convinced, why should we have above Assert.

4.
 static List *
-get_qual_for_range(PartitionKey key, PartitionBoundSpec *spec)
+get_qual_for_range(Relation parent, PartitionBoundSpec *spec,
+                  bool for_default)
 {

The addition and the way flag ‘for_default’ is being used is very confusing.
At this moment I am not able to think about a alternate solution to the
way you have used this flag. But will try and see if I can come up with
an alternate approach.

I still need to look at the test, and need to do some manual testing. Will
update here with any findings.

Patches:
0001: Refactoring/simplification of code in RelationBuildPartitionDesc(),
0002: implementation of default range partition by Beena.

Regards,
Jeevan

---

From: Beena Emerson <[email-masked]>
Date: Tue, 22 Aug 2017 16:20:20 +0530

Hi Jeevan,

>> PFA the patch rebased over v25 patches of default list partition [1]

Thank you for your review.

>
> 3.
> @@ -2396,6 +2456,8 @@ make_one_range_bound(PartitionKey key, int index, List
> *datums, bool lower)
>     ListCell   *lc;
>     int         i;
>

The function should never be called for default partition where datums = NULL.

> way you have used this flag. But will try and see if I can come up with

Well, we need to skip the get_range_nulltest while collecting the qual
of other partition to make one for default. We could skip this flag
and remove the NullTest from the qual returned for each partition but
I am not sure if thats a good way to go about it.

-- 

Beena Emerson

---

From: Beena Emerson <[email-masked]>
Date: Tue, 22 Aug 2017 17:22:00 +0530

> I am not sure if thats a good way to go about it.

Please find attached a patch which removes the for_default flag from
the get_qual_for_range function. This patch is just to show an idea
and should be applied over my earlier patch. There could be a better
way to do this. Let me know your opinion.

-- 

Beena Emerson

---

From: Jeevan Ladhe <[email-masked]>
Date: Thu, 24 Aug 2017 15:08:01 +0530

Hi Beena,

> way to do this. Let me know your opinion.
+
+ foreach (lc, list_tmp)
+ {
+ Node *n = (Node *) lfirst(lc);
+
+ if (IsA(n, NullTest))
+ {
+ list_delete(part_qual, n);
+ count++;
+ }
+ }
+

I think its an unnecessary overhead to have the nulltest prepared first
and then search for it and remove it from the partition qual. This is very
ugly.
I think the original idea is still better compared to this. May be we can
rename
the 'for_default' flag to something like 'part_of_default_qual'.

Also, as discussed offline I will merge this with default list partitioning
patch.

Regards,
Jeevan Ladhe

---

From: Beena Emerson <[email-masked]>
Date: Thu, 24 Aug 2017 15:43:23 +0530

Hello,

> and then search for it and remove it from the partition qual. This is very

Yes, I felt the same but just put it out there to see if someone can
improve on this.

> the 'for_default' flag to something like 'part_of_default_qual'.

Ya. I think that would work.

-- 

Beena Emerson

---

From: Beena Emerson <[email-masked]>
Date: Tue, 5 Sep 2017 12:31:03 +0530

Hello all,

This is to inform all that this patch has been merged with default
list partition patch [1]. There will be no further updates here. The
status of this will be updated on the commitfest according to progres
on that thread.

[1] https://www.postgresql.org/message-id/CAOgcT0ONgwajdtkoq%2BAuYkdTPY9cLWWLjxt_k4SXue3eieAr%2Bg%40mail.gmail.com

Thank you,

Beena Emerson