Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Sat, Sep 9, 2017 at 3:05 AM, Robert Haas  wrote:

> On Fri, Sep 8, 2017 at 10:08 AM, Jeevan Ladhe
>  wrote:
> > Thanks Robert for taking care of this.
> > My V29 patch series[1] is based on this commit now.
>
> Committed 0001-0003, 0005 with assorted modifications, mostly
> cosmetic, but with some actual changes to describeOneTableDetails and
> ATExecAttachPartition and minor additions to the regression tests.
>
>
Thanks Robert!!

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Fri, Sep 8, 2017 at 10:08 AM, Jeevan Ladhe
 wrote:
> Thanks Robert for taking care of this.
> My V29 patch series[1] is based on this commit now.

Committed 0001-0003, 0005 with assorted modifications, mostly
cosmetic, but with some actual changes to describeOneTableDetails and
ATExecAttachPartition and minor additions to the regression tests.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Fri, Sep 8, 2017 at 6:46 AM, Robert Haas  wrote:

> On Thu, Sep 7, 2017 at 8:13 AM, Jeevan Ladhe
>  wrote:
> > The fix would be much easier if the refactoring patch 0001 by Amul in
> hash
> > partitioning thread[2] is committed.
>
> Done.
>

Thanks Robert for taking care of this.
My V29 patch series[1] is based on this commit now.

[1]
https://www.postgresql.org/message-id/CAOgcT0PCM5mJPCOyj3c0D1mLxwaVz6DJLO=nmz5j-5jgywx...@mail.gmail.com

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Thu, Sep 7, 2017 at 8:13 AM, Jeevan Ladhe
 wrote:
> The fix would be much easier if the refactoring patch 0001 by Amul in hash
> partitioning thread[2] is committed.

Done.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Wed, Sep 6, 2017 at 5:50 PM, Jeevan Ladhe
 wrote:
>
>>
>> I am wondering whether we could avoid call to get_default_partition_oid()
>> in
>> the above block, thus avoiding a sys cache lookup. The sys cache search
>> shouldn't be expensive since the cache should already have that entry, but
>> still if we can avoid it, we save some CPU cycles. The default partition
>> OID is
>> stored in pg_partition_table catalog, which is looked up in
>> RelationGetPartitionKey(), a function which precedes
>> RelationGetPartitionDesc()
>> everywhere. What if that RelationGetPartitionKey() also returns the
>> default
>> partition OID and the common caller passes it to
>> RelationGetPartitionDesc()?.
>
>
> The purpose here is to cross check the relid with partdefid stored in
> catalog
> pg_partitioned_table, though its going to be the same in the parents cache,
> I
> think its better that we retrieve it from the catalog syscache.
> Further, RelationGetPartitionKey() is a macro and not a function, so
> modifying
> the existing simple macro for this reason does not sound a good idea to me.
> Having said this I am open to ideas here.

Sorry, I meant RelationBuildPartitionKey() and
RelationBuildPartitionDesc() instead of RelationGetPartitionKey() and
RelationGetPartitionDesc() resp.

>
>>
>> +/* A partition cannot be attached if there exists a default partition
>> */
>> +defaultPartOid = get_default_partition_oid(RelationGetRelid(rel));
>> +if (OidIsValid(defaultPartOid))
>> +ereport(ERROR,
>> +(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
>> + errmsg("cannot attach a new partition to table
>> \"%s\" having a default partition",
>> +RelationGetRelationName(rel;
>> get_default_partition_oid() searches the catalogs, which is not needed
>> when we
>> have relation descriptor of the partitioned table (to which a new
>> partition is
>> being attached). You should get the default partition OID from partition
>> descriptor. That will be cheaper.
>
>
> Something like following can be done here:
> /* A partition cannot be attached if there exists a default partition */
> if (partition_bound_has_default(rel->partdesc->boundinfo))
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
>  errmsg("cannot attach a new partition to table \"%s\"
> having a default partition",
> RelationGetRelationName(rel;
>
> But, partition_bound_has_default() is defined in partition.c and not in
> partition.h. This is done that way because boundinfo is not available in
> partition.h. Further, this piece of code is removed in next patch where we
> extend default partition support to add/attach partition even when default
> partition exists. So, to me I don’t see much of the correction issue here.

If the code is being removed, I don't think we should sweat too much
about it. Sorry for the noise.

>
> Another way to get around this is, we can define another version of
> get_default_partition_oid() something like
> get_default_partition_oid_from_parent_rel()
> in partition.c which looks around in relcache instead of catalog and returns
> the
> oid of default partition, or get_default_partition_oid() accepts both parent
> OID,
> and parent ‘Relation’ rel, if rel is not null look into relcahce and return,
> else search from catalog using OID.

I think we should define a function to return default partition OID
from partition descriptor and make it extern. Define a wrapper which
accepts Relation and returns calls this function to get default
partition OID from partition descriptor. The wrapper will be called
only on an open Relation, wherever it's available.


>
>> I haven't gone through the full patch yet, so there may be more
>> comments here. There is some duplication of code in
>> check_default_allows_bound() and ValidatePartitionConstraints() to
>> scan the children of partition being validated. The difference is that
>> the first one scans the relations in the same function and the second
>> adds them to work queue. May be we could use
>> ValidatePartitionConstraints() to scan the relation or add to the
>> queue based on some input flag may be wqueue argument itself. But I
>> haven't thought through this completely. Any thoughts?
>
>
> check_default_allows_bound() is called only from DefineRelation(),
> and not for alter command. I am not really sure how can we use
> work queue for create command.


No, we shouldn't use work queue for CREATE command. We should extract
the common code into a function to be called from
check_default_allows_bound() and ValidatePartitionConstraints(). To
that function we pass a flag (or the work queue argument itself),
which decides whether to add a work queue item or scan the relation
directly.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list 

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Thu, Sep 7, 2017 at 3:15 PM, Rajkumar Raghuwanshi <
rajkumar.raghuwan...@enterprisedb.com> wrote:

> On Wed, Sep 6, 2017 at 5:25 PM, Jeevan Ladhe <
> jeevan.la...@enterprisedb.com> wrote:
>
>> Hi,
>>
>> Attached is the rebased set of patches.
>> Robert has committed[1] patch 0001 in V26 series, hence the patch
>> numbering
>> in V27 is now decreased by 1 for each patch as compared to V26.
>>
>
> Hi,
>
> I have applied v27 patches and while testing got below observation.
>
> Observation: in below partition table, d1 constraints not allowing NULL to
> be inserted in b column
> but I am able to insert it.
>
> steps to reproduce:
> create table d0 (a int, b int) partition by range(a,b);
> create table d1 partition of d0 for values from (0,0) to
> (maxvalue,maxvalue);
>
> postgres=# insert into d0 values (0,null);
> INSERT 0 1
> postgres=# \d+ d1
> Table "public.d1"
>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target | Description
> +-+---+--+-+
> -+--+-
>  a  | integer |   |  | | plain
> |  |
>  b  | integer |   |  | | plain
> |  |
> Partition of: d0 FOR VALUES FROM (0, 0) TO (MAXVALUE, MAXVALUE)
> Partition constraint: ((a IS NOT NULL) AND *(b IS NOT NULL) *AND ((a > 0)
> OR ((a = 0) AND (b >= 0
>
> postgres=# select tableoid::regclass,* from d0;
>  tableoid | a | b
> --+---+---
>  *d1   | 0 |  *
> (1 row)
>

Good catch. Thanks Rajkumar.
This seems to be happening because of the following changes made in
get_partition_for_tuple() for default range partition support as part of
patch 0002.

@@ -1971,27 +2204,10 @@ get_partition_for_tuple(PartitionDispatch *pd,
  ecxt->ecxt_scantuple = slot;
  FormPartitionKeyDatum(parent, slot, estate, values, isnull);

- if (key->strategy == PARTITION_STRATEGY_RANGE)
- {
- /*
- * Since we cannot route tuples with NULL partition keys through a
- * range-partitioned table, simply return that no partition exists
- */
- for (i = 0; i < key->partnatts; i++)
- {
- if (isnull[i])
- {
- *failed_at = parent;
- *failed_slot = slot;
- result = -1;
- goto error_exit;
- }
- }
- }

Instead of getting rid of this. If there isn't a default partition then
we still do not have any range partition to route a null partition
key and the routing should fail.

I will work on a fix and send a patch shortly.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Rajkumar Raghuwanshi]

On Wed, Sep 6, 2017 at 5:25 PM, Jeevan Ladhe 
wrote:

> Hi,
>
> Attached is the rebased set of patches.
> Robert has committed[1] patch 0001 in V26 series, hence the patch numbering
> in V27 is now decreased by 1 for each patch as compared to V26.
>

Hi,

I have applied v27 patches and while testing got below observation.

Observation: in below partition table, d1 constraints not allowing NULL to
be inserted in b column
but I am able to insert it.

steps to reproduce:
create table d0 (a int, b int) partition by range(a,b);
create table d1 partition of d0 for values from (0,0) to
(maxvalue,maxvalue);

postgres=# insert into d0 values (0,null);
INSERT 0 1
postgres=# \d+ d1
Table "public.d1"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
+-+---+--+-+-+--+-
 a  | integer |   |  | | plain   |
|
 b  | integer |   |  | | plain   |
|
Partition of: d0 FOR VALUES FROM (0, 0) TO (MAXVALUE, MAXVALUE)
Partition constraint: ((a IS NOT NULL) AND *(b IS NOT NULL) *AND ((a > 0)
OR ((a = 0) AND (b >= 0

postgres=# select tableoid::regclass,* from d0;
 tableoid | a | b
--+---+---
 *d1   | 0 |  *
(1 row)


Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Ashutosh,

I have tried to address your comments in V27 patch series[1].
Please find my comments inlined.


> >>
> >> The current set of patches contains 6 patches as below:
> >>
> >> 0001:
> >> Refactoring existing ATExecAttachPartition  code so that it can be used
> >> for
> >> default partitioning as well
>

If I understand correctly these comments refer to patch 0001 of V25_rebase
series, which is related to "Fix assumptions that get_qual_from_partbound()"
and not refactoring. This is patch 0001 in V27 now.

  * Returns an expression tree describing the passed-in relation's partition
> - * constraint.
> + * constraint. If there are no partition constraints returns NULL e.g. in
> case
> + * default partition is the only partition.
> The first sentence uses singular constraint. The second uses plural. Given
> that
> partition bounds together form a single constraint we should use singular
> constraint in the second sentence as well.
>

I have changed the wording now.


>
> Do we want to add a similar comment in the prologue of
> generate_partition_qual(). The current wording there seems to cover this
> case,
> but do we want to explicitly mention this case?
>

I have added a comment there.


>
> +if (!and_args)
> +result = NULL;
> While this is correct, I am increasingly seeing (and_args != NIL) usage.
>

Changed this to:
+   if (and_args == NIL)
+   result = NULL;


> get_partition_qual_relid() is called from pg_get_partition_
> constraintdef(),
> constr_expr = get_partition_qual_relid(relationId);
>
> /* Quick exit if not a partition */
> if (constr_expr == NULL)
> PG_RETURN_NULL();
> The comment is now wrong since a default partition may have no
> constraints. May
> be rewrite it as simply, "Quick exit if no partition constraint."
>

Fixed.


>
> generate_partition_qual() has three callers and all of them are capable of
> handling NIL partition constraint for default partition. May be it's
> better to
> mention in the commit message that we have checked that the callers of
> this function
> can handle NIL partition constraint.
>

Added in commit message.


> >>
> >> 0002:
> >> This patch teaches the partitioning code to handle the NIL returned by
> >> get_qual_for_list().
> >> This is needed because a default partition will not have any constraints
> >> in case
> >> it is the only partition of its parent.
>

Comments below refer to patch 0002 in V25_rebase(0003 in V25), which
adds basic support for default partition, which is now 0002 in V27.


> If the partition being dropped is the default partition,
> heap_drop_with_catalog() locks default partition twice, once as the default
> partition and the second time as the partition being dropped. So, it will
> be
> counted as locked twice. There doesn't seem to be any harm in this, since
> the
> lock will be help till the transaction ends, by when all the locks will be
> released.
>

 Fixed.


> Same is the case with cache invalidation message. If we are dropping
> default
> partition, the cache invalidation message on "default partition" is not
> required. Again this might be harmless, but better to avoid it.


Fixed.


> Similar problems exists with ATExecDetachPartition(), default partition
> will be
> locked twice if it's being detached.
>

In ATExecDetachPartition() we do not have OID of the relation being
detached
available at the time we lock default partition. Moreover, here we are
taking an
exclusive lock on default OID and an share lock on partition being detached.
As you correctly said in your earlier comment that it will be counted as
locked
twice, which to me also seems harmless. As these locks are to be held till
commit of the transaction nobody else is supposed to be releasing these
locks in
between. I am not able to visualize a problem here, but still I have tried
to
avoid locking the default partition table twice, please review the changes
and
let me know your thoughts.


> +/*
> + * If this is a default partition, pg_partitioned_table must have
> it's
> + * OID as value of 'partdefid' for it's parent (i.e. rel) entry.
> + */
> +if (castNode(PartitionBoundSpec, boundspec)->is_default)
> +{
> +Oidpartdefid;
> +
> +partdefid = get_default_partition_oid(RelationGetRelid(rel));
> +Assert(partdefid == inhrelid);
> +}
> Since an accidental change or database corruption may change the default
> partition OID in pg_partition_table. An Assert won't help in such a case.
> May
> be we should throw an error or at least report a warning. If we throw an
> error,
> the table will become useless (or even the database will become useless
> RelationBuildPartitionDesc is called from RelationCacheInitializePhase3()
> on
> such a corrupted table). To avoid that we may raise a warning.
>
> I have added a warning here instead of Assert.


> I am wondering whether we could avoid call to 

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Sat, Sep 2, 2017 at 7:03 AM, Robert Haas  wrote:

> On Fri, Sep 1, 2017 at 3:19 PM, Robert Haas  wrote:
> > On Thu, Aug 31, 2017 at 8:53 AM, Jeevan Ladhe
> >  wrote:
> >> 0001:
> >> This patch refactors RelationBuildPartitionDesc(), basically this is
> patch
> >> 0001 of default range partition[1].
> >
> > I spent a while studying this; it seems to be simpler and there's no
> > real downside.  So, committed.
>
>
Thanks Robert for taking care of this.


> BTW, the rest of this series seems to need a rebase.  The changes to
> insert.sql conflicted with 30833ba154e0c1106d61e3270242dc5999a3e4f3.


Will rebase the patches.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Fri, Sep 1, 2017 at 3:19 PM, Robert Haas  wrote:
> On Thu, Aug 31, 2017 at 8:53 AM, Jeevan Ladhe
>  wrote:
>> 0001:
>> This patch refactors RelationBuildPartitionDesc(), basically this is patch
>> 0001 of default range partition[1].
>
> I spent a while studying this; it seems to be simpler and there's no
> real downside.  So, committed.

BTW, the rest of this series seems to need a rebase.  The changes to
insert.sql conflicted with 30833ba154e0c1106d61e3270242dc5999a3e4f3.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Thu, Aug 31, 2017 at 8:53 AM, Jeevan Ladhe
 wrote:
> 0001:
> This patch refactors RelationBuildPartitionDesc(), basically this is patch
> 0001 of default range partition[1].

I spent a while studying this; it seems to be simpler and there's no
real downside.  So, committed.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Mon, Aug 21, 2017 at 4:47 PM, Jeevan Ladhe
 wrote:
>
> Hi,
>
> On Thu, Aug 17, 2017 at 3:29 PM, Jeevan Ladhe
>  wrote:
>>
>> Hi,
>>
>> On Tue, Aug 15, 2017 at 7:20 PM, Robert Haas 
>> wrote:
>>>
>>> On Wed, Jul 26, 2017 at 8:14 AM, Jeevan Ladhe
>>>  wrote:
>>> > I have rebased the patches on the latest commit.
>>>
>>> This needs another rebase.
>>
>>
>> I have rebased the patch and addressed your and Ashutosh comments on last
>> set of patches.

Thanks for the rebased patches.

>>
>> The current set of patches contains 6 patches as below:
>>
>> 0001:
>> Refactoring existing ATExecAttachPartition  code so that it can be used
>> for
>> default partitioning as well

  * Returns an expression tree describing the passed-in relation's partition
- * constraint.
+ * constraint. If there are no partition constraints returns NULL e.g. in case
+ * default partition is the only partition.
The first sentence uses singular constraint. The second uses plural. Given that
partition bounds together form a single constraint we should use singular
constraint in the second sentence as well.

Do we want to add a similar comment in the prologue of
generate_partition_qual(). The current wording there seems to cover this case,
but do we want to explicitly mention this case?

+if (!and_args)
+result = NULL;
While this is correct, I am increasingly seeing (and_args != NIL) usage.

get_partition_qual_relid() is called from pg_get_partition_constraintdef(),
constr_expr = get_partition_qual_relid(relationId);

/* Quick exit if not a partition */
if (constr_expr == NULL)
PG_RETURN_NULL();
The comment is now wrong since a default partition may have no constraints. May
be rewrite it as simply, "Quick exit if no partition constraint."

generate_partition_qual() has three callers and all of them are capable of
handling NIL partition constraint for default partition. May be it's better to
mention in the commit message that we have checked that the callers of
this function
can handle NIL partition constraint.

>>
>> 0002:
>> This patch teaches the partitioning code to handle the NIL returned by
>> get_qual_for_list().
>> This is needed because a default partition will not have any constraints
>> in case
>> it is the only partition of its parent.

If the partition being dropped is the default partition,
heap_drop_with_catalog() locks default partition twice, once as the default
partition and the second time as the partition being dropped. So, it will be
counted as locked twice. There doesn't seem to be any harm in this, since the
lock will be help till the transaction ends, by when all the locks will be
released.

Same is the case with cache invalidation message. If we are dropping default
partition, the cache invalidation message on "default partition" is not
required. Again this might be harmless, but better to avoid it.

Similar problems exists with ATExecDetachPartition(), default partition will be
locked twice if it's being detached.

+/*
+ * If this is a default partition, pg_partitioned_table must have it's
+ * OID as value of 'partdefid' for it's parent (i.e. rel) entry.
+ */
+if (castNode(PartitionBoundSpec, boundspec)->is_default)
+{
+Oidpartdefid;
+
+partdefid = get_default_partition_oid(RelationGetRelid(rel));
+Assert(partdefid == inhrelid);
+}
Since an accidental change or database corruption may change the default
partition OID in pg_partition_table. An Assert won't help in such a case. May
be we should throw an error or at least report a warning. If we throw an error,
the table will become useless (or even the database will become useless
RelationBuildPartitionDesc is called from RelationCacheInitializePhase3() on
such a corrupted table). To avoid that we may raise a warning.

I am wondering whether we could avoid call to get_default_partition_oid() in
the above block, thus avoiding a sys cache lookup. The sys cache search
shouldn't be expensive since the cache should already have that entry, but
still if we can avoid it, we save some CPU cycles. The default partition OID is
stored in pg_partition_table catalog, which is looked up in
RelationGetPartitionKey(), a function which precedes RelationGetPartitionDesc()
everywhere. What if that RelationGetPartitionKey() also returns the default
partition OID and the common caller passes it to RelationGetPartitionDesc()?.

+/* A partition cannot be attached if there exists a default partition */
+defaultPartOid = get_default_partition_oid(RelationGetRelid(rel));
+if (OidIsValid(defaultPartOid))
+ereport(ERROR,
+(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("cannot attach a new partition to table
\"%s\" having a default partition",
+

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Fri, Aug 18, 2017 at 12:25 AM, Robert Haas  wrote:

> On Thu, Aug 17, 2017 at 6:24 AM, Jeevan Ladhe
>  wrote:
> > I have addressed following comments in V25 patch[1].
>
> Committed 0001.  Since that code seems to be changing about every 10
> minutes, it seems best to get this refactoring out of the way before
> it changes again.


Thanks Robert for taking care of this.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Thu, Aug 17, 2017 at 6:24 AM, Jeevan Ladhe
 wrote:
> I have addressed following comments in V25 patch[1].

Committed 0001.  Since that code seems to be changing about every 10
minutes, it seems best to get this refactoring out of the way before
it changes again.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Thom Brown]

On 17 August 2017 at 10:59, Jeevan Ladhe  wrote:
> Hi,
>
> On Tue, Aug 15, 2017 at 7:20 PM, Robert Haas  wrote:
>>
>> On Wed, Jul 26, 2017 at 8:14 AM, Jeevan Ladhe
>>  wrote:
>> > I have rebased the patches on the latest commit.
>>
>> This needs another rebase.
>
>
> I have rebased the patch and addressed your and Ashutosh comments on last
> set of patches.
>
> The current set of patches contains 6 patches as below:
>
> 0001:
> Refactoring existing ATExecAttachPartition  code so that it can be used for
> default partitioning as well
>
> 0002:
> This patch teaches the partitioning code to handle the NIL returned by
> get_qual_for_list().
> This is needed because a default partition will not have any constraints in
> case
> it is the only partition of its parent.
>
> 0003:
> Support for default partition with the restriction of preventing addition of
> any
> new partition after default partition. This is a merge of 0003 and 0004 in
> V24 series.
>
> 0004:
> Extend default partitioning support to allow addition of new partitions
> after
> default partition is created/attached. This patch is a merge of patches
> 0005 and 0006 in V24 series to simplify the review process. The
> commit message has more details regarding what all is included.
>
> 0005:
> This patch introduces code to check if the scanning of default partition
> child
> can be skipped if it's constraints are proven.
>
> 0006:
> Documentation.
>
>
> PFA, and let me know in case of any comments.

Thanks.  Applies fine, and I've been exercising the patch and it is
doing everything it's supposed to do.

I am, however, curious to know why the planner can't optimise the following:

SELECT * FROM mystuff WHERE mystuff = (1::int,'JP'::text,'blue'::text);

This exhaustively checks all partitions, but if I change it to:

SELECT * FROM mystuff WHERE (id, country, content) =
(1::int,'JP'::text,'blue'::text);

It works fine.

The former filters like so: ((mystuff_default_1.*)::mystuff = ROW(1,
'JP'::text, 'blue'::text))

Shouldn't it instead do:

((mystuff_default_1.id, mystuff_default_1.country,
mystuff_default_1.content)::mystuff = ROW(1, 'JP'::text,
'blue'::text))

So it's not really to do with this patch; it's just something I
noticed while testing.

Thom


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Robert,

>> > 0007:
> >> > This patch introduces code to check if the scanning of default
> partition
> >> > child
> >> > can be skipped if it's constraints are proven.
> >>
> >> If I understand correctly, this is actually a completely separate
> >> feature not intrinsically related to default partitioning.
> >
> > I don't see this as a new feature, since scanning the default partition
> > will be introduced by this series of patches only, and rather than a
> > feature this can be classified as a completeness of default skip
> > validation logic. Your thoughts?
>
> Currently, when a partitioned table is attached, we check whether all
> the scans can be checked but not whether scans on some partitions can
> be attached.  So there are two separate things:
>
> 1. When we introduce default partitioning, we need scan the default
> partition either when (a) any partition is attached or (b) any
> partition is created.
>
> 2. In any situation where scans are needed (scanning the partition
> when it's attached, scanning the default partition when some other
> partition is attached, scanning the default when a new partition is
> created), we can run predicate_implied_by for each partition to see
> whether the scan of that partition can be skipped.
>
> Those two changes are independent. We could do (1) without doing (2)
> or (2) without doing (1) or we could do both.  So they are separate
> features.
>
>
In my V25 series(patch 0005) I have only addressed half of (2) above
i.e. code to check whether the scan of a partition of default partition
can be skipped when other partition is being added. Amit Langote
has submitted[1] a separate patch(0003)  to address skipping the scan
of the children of relation when it's being attached as a partition.

[1]
https://www.postgresql.org/message-id/4cd13b03-846d-dc65-89de-1fd9743a3869%40lab.ntt.co.jp

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Robert,

Please find my feedback inlined.
I have addressed following comments in V25 patch[1].


> > 0002:
> > This patch teaches the partitioning code to handle the NIL returned by
> > get_qual_for_list().
> > This is needed because a default partition will not have any constraints
> in
> > case
> > it is the only partition of its parent.
>
> Perhaps it would be better to make validatePartConstraint() a no-op
> when the constraint is empty rather than putting the logic in the
> caller.  Otherwise, every place that calls validatePartConstraint()
> has to think about whether or not the constraint-is-NULL case needs to
> be handled.
>
> I have now added a check in ValidatePartConstraint(). This change is made
in 0001 patch.



> > 0003:
> > Support for default partition with the restriction of preventing
> addition of
> > any
> > new partition after default partition.
>
> This looks generally reasonable, but can't really be committed without
> the later patches, because it might break pg_dump, which won't know
> that the DEFAULT partition must be dumped last and might therefore get
> the dump ordering wrong, and of course also because it reverts commit
> c1e0e7e1d790bf18c913e6a452dea811e858b554.
>
>
Thanks Robert for looking into this. The purpose of having different
patches is
just to ease the review process and the basic patch of introducing the
default
partition support and extending support for addition of new partition
should go
together.


> > 0004:
> > Store the default partition OID in pg_partition_table, this will help us
> to
> > retrieve the OID of default relation when we don't have the relation
> cache
> > available. This was also suggested by Amit Langote here[1].
>
> I looked this over and I think this is the right approach.  An
> alternative way to avoid needing a relcache entry in
> heap_drop_with_catalog() would be for get_default_partition_oid() to
> call find_inheritance_children() here and then use a syscache lookup
> to get the partition bound for each one, but that's still going to
> cause some syscache bloat.
>

To safeguard future development from missing this and leaving it out of
sync, I
have added an Assert in RelationBuildPartitionDesc() to cross check the
partdefid.


>
> > 0005:
> > Extend default partitioning support to allow addition of new partitions.
>
> +   if (spec->is_default)
> +   {
> +   /* Default partition cannot be added if there already
> exists one. */
> +   if (partdesc->nparts > 0 &&
> partition_bound_has_default(boundinfo))
> +   {
> +   with = boundinfo->default_index;
> +   ereport(ERROR,
> +
> (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> +errmsg("partition \"%s\"
> conflicts with existing default partition \"%s\"",
> +   relname,
> get_rel_name(partdesc->oids[with])),
> +parser_errposition(pstate,
> spec->location)));
> +   }
> +
> +   return;
> +   }
>
> I generally think it's good to structure the code so as to minimize
> the indentation level.  In this case, if you did if (partdesc->nparts
> == 0 || !partition_bound_has_default(boundinfo)) return; first, then
> the rest of it could be one level less indented.  Also, perhaps it
> would be clearer to test boundinfo == NULL rather than
> partdesc->nparts == 0, assuming they are equivalent.
>

Fixed.

> 0006:
> > Extend default partitioning validation code to reuse the refactored code
> in
> > patch 0001.
>
> I'm having a very hard time understanding what's going on with this
> patch.  It certainly doesn't seem to be just refactoring things to use
> the code from 0001.  For example:
>
> -   if (ExecCheck(partqualstate, econtext))
> +   if (!ExecCheck(partqualstate, econtext))
>
> It seems hard to believe that refactoring things to use the code from
> 0001 would involve inverting the value of this test.
>
> +* derived from the bounds(the partition constraint
> never evaluates to
> +* NULL, so negating it like this is safe).
>
> I don't see it being negated.
>
> I think this patch needs a better explanation of what it's trying to
> do, and better comments.  I gather that at least part of the point
> here is to skip validation scans on default partitions if the default
> partition has been constrained not to contain any values that would
> fall in the new partition, but neither the commit message for 0006 nor
> your description here make that very clear.
>

In V25 series, this is now part of patch 0004, and should avoid any
confusion as above. I have tried to add verbose comment in commit
message as well as I have improved the code comments in this code
area.

> [0008 documentation]
>
> -  attached is marked NO INHERIT, the command will
> fail;
> -  such a constraint must be recreated 

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Ashutosh,

On Thu, Aug 17, 2017 at 3:41 PM, Jeevan Ladhe  wrote:

> Hi Ashutosh,
>
> Please find my feedback inlined.
>
> On Fri, Jul 28, 2017 at 7:00 PM, Ashutosh Bapat <
> ashutosh.ba...@enterprisedb.com> wrote:
>
>> On Wed, Jul 26, 2017 at 5:44 PM, Jeevan Ladhe
>>  wrote:
>> > Hi,
>> >
>> > I have rebased the patches on the latest commit.
>> >
>>
>> Thanks for rebasing the patches. The patches apply and compile
>> cleanly. make check passes.
>>
>> Here are some review comments
>> 0001 patch
>> Most of this patch is same as 0002 patch posted in thread [1]. I have
>> extensively reviewed that patch for Amit Langote. Can you please compare
>> these
>> two patches and try to address those comments OR just use patch from that
>> thread? For example, canSkipPartConstraintValidation() is named as
>> PartConstraintImpliedByRelConstraint() in that patch. OR
>> +if (scanRel_constr == NULL)
>> +return false;
>> +
>> is not there in that patch since returning false is wrong when
>> partConstraint
>> is NULL. I think this patch needs those fixes. Also, this patch set would
>> need
>> a rebase when 0001 from that thread gets committed.
>>
>>
> I have renamed the canSkipPartConstraintValidation() to
> PartConstraintImpliedByRelConstraint() and made other changes applicable
> per
> Amit’s patch. This patch also refactors the scanning logic in
> ATExecAttachPartition()
> and adds it into a function ValidatePartitionConstraints(), hence I could
> not use
> Amit’s patch as it is. Please have a look into the new patch and let me
> know if it
> looks fine to you.
>
>
>> 0002 patch
>> +if (!and_args)
>> +result = NULL;
>> Add "NULL, if there are not partition constraints e.g. in case of default
>> partition as the only partition.".
>
>
> Added. Please check.
>
>
>> This patch avoids calling
>> validatePartitionConstraints() and hence canSkipPartConstraintValidation()
>> when
>> partConstraint is NULL, but patches in [1] introduce more callers of
>> canSkipPartConstraintValidation() which may pass NULL. So, it's better
>> that we
>> handle that case.
>>
>
> Following code added in patch 0001 now should take care of this.
> +   num_check = (constr != NULL) ? constr->num_check : 0;
>
>
>> 0003 patch
>> +parentRel = heap_open(parentOid, AccessExclusiveLock);
>> In [2], Amit Langote has given a reason as to why heap_drop_with_catalog()
>> should not heap_open() the parent relation. But this patch still calls
>> heap_open() without giving any counter argument. Also I don't see
>> get_default_partition_oid() using Relation anywhere. If you remove that
>> heap_open() please remove following heap_close().
>> +heap_close(parentRel, NoLock);
>>
>
> As clarified earlier this was addressed in 0004 patch of V24 series. In
> current set of patches this is now addressed in patch 0003 itself.
>
>
>>
>> +/*
>> + * The default partition accepts any
>> non-specified
>> + * value, hence it should not get a mapped index
>> while
>> + * assigning those for non-null datums.
>> + */
>> Instead of "any non-specified value", you may want to use "any value not
>> specified in the lists of other partitions" or something like that.
>>
>
> Changed the comment.
>
>
>>
>> + * If this is a NULL, route it to the null-accepting partition.
>> + * Otherwise, route by searching the array of partition bounds.
>> You may want to write it as "If this is a null partition key, ..." to
>> clarify
>> what's NULL.
>>
>
> Changed the comment.
>
>
>>
>> + * cur_index < 0 means we could not find a non-default partition
>> of
>> + * this parent. cur_index >= 0 means we either found the leaf
>> + * partition, or the next parent to find a partition of.
>> + *
>> + * If we couldn't find a non-default partition check if the
>> default
>> + * partition exists, if it does, get its index.
>> In order to avoid repeating "we couldn't find a ..."; you may want to add
>> ",
>> try default partition if one exists." in the first sentence itself.
>>
>
> Sorry, but I am not really sure how this change would make the comment
> more readable than the current one.
>
>
>> get_default_partition_oid() is defined in this patch and then redefined in
>> 0004. Let's define it only once, mostly in or before 0003 patch.
>>
>
> get_default_partition_oid() is now defined only once in patch 0003.
>
>
>>
>> + * partition strategy. Assign the parent strategy to the default
>> s/parent/parent's/
>>
>
> Fixed.
>
>
>>
>> +-- attaching default partition overlaps if the default partition already
>> exists
>> +CREATE TABLE def_part PARTITION OF list_parted DEFAULT;
>> +CREATE TABLE fail_def_part (LIKE part_1 INCLUDING CONSTRAINTS);
>> +ALTER TABLE list_parted ATTACH PARTITION fail_def_part DEFAULT;
>> +ERROR:  

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Ashutosh,

Please find my feedback inlined.

On Fri, Jul 28, 2017 at 7:00 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> On Wed, Jul 26, 2017 at 5:44 PM, Jeevan Ladhe
>  wrote:
> > Hi,
> >
> > I have rebased the patches on the latest commit.
> >
>
> Thanks for rebasing the patches. The patches apply and compile
> cleanly. make check passes.
>
> Here are some review comments
> 0001 patch
> Most of this patch is same as 0002 patch posted in thread [1]. I have
> extensively reviewed that patch for Amit Langote. Can you please compare
> these
> two patches and try to address those comments OR just use patch from that
> thread? For example, canSkipPartConstraintValidation() is named as
> PartConstraintImpliedByRelConstraint() in that patch. OR
> +if (scanRel_constr == NULL)
> +return false;
> +
> is not there in that patch since returning false is wrong when
> partConstraint
> is NULL. I think this patch needs those fixes. Also, this patch set would
> need
> a rebase when 0001 from that thread gets committed.
>
>
I have renamed the canSkipPartConstraintValidation() to
PartConstraintImpliedByRelConstraint() and made other changes applicable per
Amit’s patch. This patch also refactors the scanning logic in
ATExecAttachPartition()
and adds it into a function ValidatePartitionConstraints(), hence I could
not use
Amit’s patch as it is. Please have a look into the new patch and let me
know if it
looks fine to you.


> 0002 patch
> +if (!and_args)
> +result = NULL;
> Add "NULL, if there are not partition constraints e.g. in case of default
> partition as the only partition.".


Added. Please check.


> This patch avoids calling
> validatePartitionConstraints() and hence canSkipPartConstraintValidation()
> when
> partConstraint is NULL, but patches in [1] introduce more callers of
> canSkipPartConstraintValidation() which may pass NULL. So, it's better
> that we
> handle that case.
>

Following code added in patch 0001 now should take care of this.
+   num_check = (constr != NULL) ? constr->num_check : 0;


> 0003 patch
> +parentRel = heap_open(parentOid, AccessExclusiveLock);
> In [2], Amit Langote has given a reason as to why heap_drop_with_catalog()
> should not heap_open() the parent relation. But this patch still calls
> heap_open() without giving any counter argument. Also I don't see
> get_default_partition_oid() using Relation anywhere. If you remove that
> heap_open() please remove following heap_close().
> +heap_close(parentRel, NoLock);
>

As clarified earlier this was addressed in 0004 patch of V24 series. In
current set of patches this is now addressed in patch 0003 itself.


>
> +/*
> + * The default partition accepts any non-specified
> + * value, hence it should not get a mapped index
> while
> + * assigning those for non-null datums.
> + */
> Instead of "any non-specified value", you may want to use "any value not
> specified in the lists of other partitions" or something like that.
>

Changed the comment.


>
> + * If this is a NULL, route it to the null-accepting partition.
> + * Otherwise, route by searching the array of partition bounds.
> You may want to write it as "If this is a null partition key, ..." to
> clarify
> what's NULL.
>

Changed the comment.


>
> + * cur_index < 0 means we could not find a non-default partition
> of
> + * this parent. cur_index >= 0 means we either found the leaf
> + * partition, or the next parent to find a partition of.
> + *
> + * If we couldn't find a non-default partition check if the
> default
> + * partition exists, if it does, get its index.
> In order to avoid repeating "we couldn't find a ..."; you may want to add
> ",
> try default partition if one exists." in the first sentence itself.
>

Sorry, but I am not really sure how this change would make the comment
more readable than the current one.


> get_default_partition_oid() is defined in this patch and then redefined in
> 0004. Let's define it only once, mostly in or before 0003 patch.
>

get_default_partition_oid() is now defined only once in patch 0003.


>
> + * partition strategy. Assign the parent strategy to the default
> s/parent/parent's/
>

Fixed.


>
> +-- attaching default partition overlaps if the default partition already
> exists
> +CREATE TABLE def_part PARTITION OF list_parted DEFAULT;
> +CREATE TABLE fail_def_part (LIKE part_1 INCLUDING CONSTRAINTS);
> +ALTER TABLE list_parted ATTACH PARTITION fail_def_part DEFAULT;
> +ERROR:  cannot attach a new partition to table "list_parted" having a
> default partition
> For 0003 patch this testcase is same as the testcase in the next hunk; no
> new
> partition can be added after default partition. Please add this testcase in
> next set of patches.
>

Though the 

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Wed, Jul 26, 2017 at 8:14 AM, Jeevan Ladhe
 wrote:
> I have rebased the patches on the latest commit.

This needs another rebase.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Mon, Aug 14, 2017 at 7:51 AM, Jeevan Ladhe
 wrote:
> I think even with this change there will be one level of indentation
> needed for throwing the error, as the error is to be thrown only if
> there exists a default partition.

That's true, but we don't need two levels.

>> > 0007:
>> > This patch introduces code to check if the scanning of default partition
>> > child
>> > can be skipped if it's constraints are proven.
>>
>> If I understand correctly, this is actually a completely separate
>> feature not intrinsically related to default partitioning.
>
> I don't see this as a new feature, since scanning the default partition
> will be introduced by this series of patches only, and rather than a
> feature this can be classified as a completeness of default skip
> validation logic. Your thoughts?

Currently, when a partitioned table is attached, we check whether all
the scans can be checked but not whether scans on some partitions can
be attached.  So there are two separate things:

1. When we introduce default partitioning, we need scan the default
partition either when (a) any partition is attached or (b) any
partition is created.

2. In any situation where scans are needed (scanning the partition
when it's attached, scanning the default partition when some other
partition is attached, scanning the default when a new partition is
created), we can run predicate_implied_by for each partition to see
whether the scan of that partition can be skipped.

Those two changes are independent. We could do (1) without doing (2)
or (2) without doing (1) or we could do both.  So they are separate
features.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Robert,


> 0005:
> > Extend default partitioning support to allow addition of new partitions.
>
> +   if (spec->is_default)
> +   {
> +   /* Default partition cannot be added if there already
> exists one. */
> +   if (partdesc->nparts > 0 &&
> partition_bound_has_default(boundinfo))
> +   {
> +   with = boundinfo->default_index;
> +   ereport(ERROR,
> +
> (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> +errmsg("partition \"%s\"
> conflicts with existing default partition \"%s\"",
> +   relname,
> get_rel_name(partdesc->oids[with])),
> +parser_errposition(pstate,
> spec->location)));
> +   }
> +
> +   return;
> +   }
>
> I generally think it's good to structure the code so as to minimize
> the indentation level.  In this case, if you did if (partdesc->nparts
> == 0 || !partition_bound_has_default(boundinfo)) return; first, then
> the rest of it could be one level less indented.  Also, perhaps it
> would be clearer to test boundinfo == NULL rather than
> partdesc->nparts == 0, assuming they are equivalent.


I think even with this change there will be one level of indentation
needed for throwing the error, as the error is to be thrown only if
there exists a default partition.


>
>
-* We must also lock the default partition, for the same
> reasons explained
> -* in heap_drop_with_catalog().
> +* We must lock the default partition, for the same reasons
> explained in
> +* DefineRelation().
>
> I don't really see the point of this change.  Whichever earlier patch
> adds this code could include or omit the word "also" as appropriate,
> and then this patch wouldn't need to change it.
>
>
Actually the change is made because if the difference in the function name.
I will remove ‘also’ from the first patch itself.


> > 0007:
> > This patch introduces code to check if the scanning of default partition
> > child
> > can be skipped if it's constraints are proven.
>
> If I understand correctly, this is actually a completely separate
> feature not intrinsically related to default partitioning.


I don't see this as a new feature, since scanning the default partition
will be introduced by this series of patches only, and rather than a
feature this can be classified as a completeness of default skip
validation logic. Your thoughts?

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Wed, Jul 12, 2017 at 3:31 PM, Jeevan Ladhe
 wrote:
> 0001:
> Refactoring existing ATExecAttachPartition  code so that it can be used for
> default partitioning as well

Boring refactoring.  Seems fine.

> 0002:
> This patch teaches the partitioning code to handle the NIL returned by
> get_qual_for_list().
> This is needed because a default partition will not have any constraints in
> case
> it is the only partition of its parent.

Perhaps it would be better to make validatePartConstraint() a no-op
when the constraint is empty rather than putting the logic in the
caller.  Otherwise, every place that calls validatePartConstraint()
has to think about whether or not the constraint-is-NULL case needs to
be handled.

> 0003:
> Support for default partition with the restriction of preventing addition of
> any
> new partition after default partition.

This looks generally reasonable, but can't really be committed without
the later patches, because it might break pg_dump, which won't know
that the DEFAULT partition must be dumped last and might therefore get
the dump ordering wrong, and of course also because it reverts commit
c1e0e7e1d790bf18c913e6a452dea811e858b554.

> 0004:
> Store the default partition OID in pg_partition_table, this will help us to
> retrieve the OID of default relation when we don't have the relation cache
> available. This was also suggested by Amit Langote here[1].

I looked this over and I think this is the right approach.  An
alternative way to avoid needing a relcache entry in
heap_drop_with_catalog() would be for get_default_partition_oid() to
call find_inheritance_children() here and then use a syscache lookup
to get the partition bound for each one, but that's still going to
cause some syscache bloat.

> 0005:
> Extend default partitioning support to allow addition of new partitions.

+   if (spec->is_default)
+   {
+   /* Default partition cannot be added if there already
exists one. */
+   if (partdesc->nparts > 0 &&
partition_bound_has_default(boundinfo))
+   {
+   with = boundinfo->default_index;
+   ereport(ERROR,
+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+errmsg("partition \"%s\"
conflicts with existing default partition \"%s\"",
+   relname,
get_rel_name(partdesc->oids[with])),
+parser_errposition(pstate,
spec->location)));
+   }
+
+   return;
+   }

I generally think it's good to structure the code so as to minimize
the indentation level.  In this case, if you did if (partdesc->nparts
== 0 || !partition_bound_has_default(boundinfo)) return; first, then
the rest of it could be one level less indented.  Also, perhaps it
would be clearer to test boundinfo == NULL rather than
partdesc->nparts == 0, assuming they are equivalent.

-* We must also lock the default partition, for the same
reasons explained
-* in heap_drop_with_catalog().
+* We must lock the default partition, for the same reasons explained in
+* DefineRelation().

I don't really see the point of this change.  Whichever earlier patch
adds this code could include or omit the word "also" as appropriate,
and then this patch wouldn't need to change it.

> 0006:
> Extend default partitioning validation code to reuse the refactored code in
> patch 0001.

I'm having a very hard time understanding what's going on with this
patch.  It certainly doesn't seem to be just refactoring things to use
the code from 0001.  For example:

-   if (ExecCheck(partqualstate, econtext))
+   if (!ExecCheck(partqualstate, econtext))

It seems hard to believe that refactoring things to use the code from
0001 would involve inverting the value of this test.

+* derived from the bounds(the partition constraint
never evaluates to
+* NULL, so negating it like this is safe).

I don't see it being negated.

I think this patch needs a better explanation of what it's trying to
do, and better comments.  I gather that at least part of the point
here is to skip validation scans on default partitions if the default
partition has been constrained not to contain any values that would
fall in the new partition, but neither the commit message for 0006 nor
your description here make that very clear.

> 0007:
> This patch introduces code to check if the scanning of default partition
> child
> can be skipped if it's constraints are proven.

If I understand correctly, this is actually a completely separate
feature not intrinsically related to default partitioning.

> [0008 documentation]

-  attached is marked NO INHERIT, the command will fail;
-  such a constraint must be recreated without the NO
INHERIT
-  clause.
+  target table.
+ 

I don't favor inserting a 

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Sun, Jul 30, 2017 at 8:07 AM, Jeevan Ladhe
 wrote:
> Hi Ashutosh,
>
> 0003 patch
>>
>> +parentRel = heap_open(parentOid, AccessExclusiveLock);
>> In [2], Amit Langote has given a reason as to why heap_drop_with_catalog()
>> should not heap_open() the parent relation. But this patch still calls
>> heap_open() without giving any counter argument. Also I don't see
>> get_default_partition_oid() using Relation anywhere. If you remove that
>> heap_open() please remove following heap_close().
>
>
> I think the patch 0004 exactly does what you have said here, i.e. it gets
> rid of the heap_open() and heap_close().
> The question might be why I kept the patch 0004 a separate one, and the
> answer is I wanted to make it easier for review, and also keeping it that
> way would make it bit easy to work on a different approach if needed.
>

The reviewer has to review two different set of changes to the same
portion of the code. That just doubles the work. I didn't find that
simplifying review. As I have suggested earlier, let's define
get_default_partition_oid() only once, mostly in or before 0003 patch.
Having it in a separate patch would allow you to change its
implementation if needed.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Sat, Jul 29, 2017 at 2:55 AM, Robert Haas  wrote:
> On Fri, Jul 28, 2017 at 9:30 AM, Ashutosh Bapat
>  wrote:
>> 0004 patch
>> The patch adds another column partdefid to catalog pg_partitioned_table. The
>> column gives OID of the default partition for a given partitioned table. This
>> means that the default partition's OID is stored at two places 1. in the
>> default partition table's pg_class entry and in pg_partitioned_table. There 
>> is
>> no way to detect when these two go out of sync. Keeping those two in sync is
>> also a maintenance burdern. Given that default partition's OID is required 
>> only
>> while adding/dropping a partition, which is a less frequent operation, it 
>> won't
>> hurt to join a few catalogs (pg_inherits and pg_class in this case) to find 
>> out
>> the default partition's OID. That will be occasional performance hit
>> worth the otherwise maintenance burden.
>
> Performance isn't the only consideration here.  We also need to think
> about locking and concurrency.  I think that most operations that
> involve locking the parent will also involve locking the default
> partition.  However, we can't safely build a relcache entry for a
> relation before we've got some kind of lock on it.  We can't assume
> that there is no concurrent DDL going on before we take some lock.  We
> can't assume invalidation messages are processed before we have taken
> some lock.  If we read multiple catalog tuples, they may be from
> different points in time.  If we can figure out everything we need to
> know from one or two syscache lookups, it may be easier to verify that
> the code is bug-free vs. having to do something more complicated.
>

The code takes a lock on the parent relation. While that function
holds that lock nobody else would change partitions of that relation
and hence nobody changes the default partition.
heap_drop_with_catalog() has code to lock the parent. Looking up
pg_inherits catalog for its partitions followed by identifying the
partition which has default partition bounds specification while
holding the lock on the parent should be safe. Any changes to
partition bounds, or partitions would require lock on the parent. In
order to prevent any buggy code changing the default partition without
sufficient locks, we should lock the default partition after it's
found and check the default partition bound specification again. Will
that work?

> Now that having been said, I'm not taking the position that Jeevan's
> patch (based on Amit Langote's idea) has definitely got the right
> idea, just that you should think twice before shooting down the
> approach.
>

If we can avoid the problems specified by Amit Langote, I am fine with
the approach of reading the default partition OID from the Relcache as
well. But I am not able to device a solution to those problems.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Ashutosh,

0003 patch

> +parentRel = heap_open(parentOid, AccessExclusiveLock);
> In [2], Amit Langote has given a reason as to why heap_drop_with_catalog()
> should not heap_open() the parent relation. But this patch still calls
> heap_open() without giving any counter argument. Also I don't see
> get_default_partition_oid() using Relation anywhere. If you remove that
> heap_open() please remove following heap_close().
>

I think the patch 0004 exactly does what you have said here, i.e. it gets
rid of the heap_open() and heap_close().
The question might be why I kept the patch 0004 a separate one, and the
answer is I wanted to make it easier for review, and also keeping it that
way would make it bit easy to work on a different approach if needed.

About this: *"Also I don't see get_default_partition_oid() using Relation
anywhere."*
The get_default_partition_oid() uses parent relation to
retrieve PartitionDesc
from parent.

Kindly let me know if you think I am still missing anything.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Fri, Jul 28, 2017 at 9:30 AM, Ashutosh Bapat
 wrote:
> 0004 patch
> The patch adds another column partdefid to catalog pg_partitioned_table. The
> column gives OID of the default partition for a given partitioned table. This
> means that the default partition's OID is stored at two places 1. in the
> default partition table's pg_class entry and in pg_partitioned_table. There is
> no way to detect when these two go out of sync. Keeping those two in sync is
> also a maintenance burdern. Given that default partition's OID is required 
> only
> while adding/dropping a partition, which is a less frequent operation, it 
> won't
> hurt to join a few catalogs (pg_inherits and pg_class in this case) to find 
> out
> the default partition's OID. That will be occasional performance hit
> worth the otherwise maintenance burden.

Performance isn't the only consideration here.  We also need to think
about locking and concurrency.  I think that most operations that
involve locking the parent will also involve locking the default
partition.  However, we can't safely build a relcache entry for a
relation before we've got some kind of lock on it.  We can't assume
that there is no concurrent DDL going on before we take some lock.  We
can't assume invalidation messages are processed before we have taken
some lock.  If we read multiple catalog tuples, they may be from
different points in time.  If we can figure out everything we need to
know from one or two syscache lookups, it may be easier to verify that
the code is bug-free vs. having to do something more complicated.

Now that having been said, I'm not taking the position that Jeevan's
patch (based on Amit Langote's idea) has definitely got the right
idea, just that you should think twice before shooting down the
approach.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Wed, Jul 26, 2017 at 5:44 PM, Jeevan Ladhe
 wrote:
> Hi,
>
> I have rebased the patches on the latest commit.
>

Thanks for rebasing the patches. The patches apply and compile
cleanly. make check passes.

Here are some review comments
0001 patch
Most of this patch is same as 0002 patch posted in thread [1]. I have
extensively reviewed that patch for Amit Langote. Can you please compare these
two patches and try to address those comments OR just use patch from that
thread? For example, canSkipPartConstraintValidation() is named as
PartConstraintImpliedByRelConstraint() in that patch. OR
+if (scanRel_constr == NULL)
+return false;
+
is not there in that patch since returning false is wrong when partConstraint
is NULL. I think this patch needs those fixes. Also, this patch set would need
a rebase when 0001 from that thread gets committed.

0002 patch
+if (!and_args)
+result = NULL;
Add "NULL, if there are not partition constraints e.g. in case of default
partition as the only partition.". This patch avoids calling
validatePartitionConstraints() and hence canSkipPartConstraintValidation() when
partConstraint is NULL, but patches in [1] introduce more callers of
canSkipPartConstraintValidation() which may pass NULL. So, it's better that we
handle that case.

0003 patch
+parentRel = heap_open(parentOid, AccessExclusiveLock);
In [2], Amit Langote has given a reason as to why heap_drop_with_catalog()
should not heap_open() the parent relation. But this patch still calls
heap_open() without giving any counter argument. Also I don't see
get_default_partition_oid() using Relation anywhere. If you remove that
heap_open() please remove following heap_close().
+heap_close(parentRel, NoLock);

+/*
+ * The default partition accepts any non-specified
+ * value, hence it should not get a mapped index while
+ * assigning those for non-null datums.
+ */
Instead of "any non-specified value", you may want to use "any value not
specified in the lists of other partitions" or something like that.

+ * If this is a NULL, route it to the null-accepting partition.
+ * Otherwise, route by searching the array of partition bounds.
You may want to write it as "If this is a null partition key, ..." to clarify
what's NULL.

+ * cur_index < 0 means we could not find a non-default partition of
+ * this parent. cur_index >= 0 means we either found the leaf
+ * partition, or the next parent to find a partition of.
+ *
+ * If we couldn't find a non-default partition check if the default
+ * partition exists, if it does, get its index.
In order to avoid repeating "we couldn't find a ..."; you may want to add ",
try default partition if one exists." in the first sentence itself.

get_default_partition_oid() is defined in this patch and then redefined in
0004. Let's define it only once, mostly in or before 0003 patch.

+ * partition strategy. Assign the parent strategy to the default
s/parent/parent's/

+-- attaching default partition overlaps if the default partition already exists
+CREATE TABLE def_part PARTITION OF list_parted DEFAULT;
+CREATE TABLE fail_def_part (LIKE part_1 INCLUDING CONSTRAINTS);
+ALTER TABLE list_parted ATTACH PARTITION fail_def_part DEFAULT;
+ERROR:  cannot attach a new partition to table "list_parted" having a
default partition
For 0003 patch this testcase is same as the testcase in the next hunk; no new
partition can be added after default partition. Please add this testcase in
next set of patches.

+-- fail
+insert into part_default values ('aa', 2);
May be explain why the insert should fail. "A row, which would fit
other partition, does not fit default partition, even when inserted directly"
or something like that. I see that many of the tests in that file do not
explain why something should "fail" or be "ok", but may be it's better to
document the reason for better readability and future reference.

+-- check in case of multi-level default partitioned table
s/in/the/ ?. Or you may want to reword it as "default partitioned partition in
multi-level partitioned table" as there is nothing like "default partitioned
table". May be we need a testcase where every level of a multi-level
partitioned table has a default partition.

+-- drop default, as we need to add some more partitions to test tuple routing
Should be clubbed with the actual DROP statement?

+-- Check that addition or removal of any partition is correctly dealt with by
+-- default partition table when it is being used in cached plan.
Plan of a prepared statement gets cached only after it's executed 5 times.
Before that the statement gets invalidated but there's not cached plan that
gets invalidated. The test is fine here, but in order to test the cached plan
as mentioned in the comment, you 

Re: [HACKERS] Adding support for Default partition in partitioning

[Beena Emerson]

Hello,

On Thu, Jul 13, 2017 at 1:22 AM, Jeevan Ladhe
 wrote:
>
>> - Should probably be merged with the patch to add default partitioning
>> for ranges.
>
>
> Beena is already rebasing her patch on my latest patches, so I think getting
> them merged here won't be an issue, mostly will be just like one more patch
> on top my patches.
>

I have posted the updated patch which can be applied over the v22
patches submitted here.
https://www.postgresql.org/message-id/CAOG9ApGEZxSQD-ZD3icj_CwTmprSGG7sZ_r3k9m4rmcc6ozr%3Dg%40mail.gmail.com

Thank you,

Beena Emerson

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi,

I have tried to address your comments in the V22 set of patches[1].
Please find my feedback inlined on your comments.

On Fri, Jun 16, 2017 at 1:46 PM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:

> Hello, I'd like to review this but it doesn't fit the master, as
> Robert said. Especially the interface of predicate_implied_by is
> changed by the suggested commit.
>
> Anyway I have some comment on this patch with fresh eyes.  I
> believe the basic design so my comment below are from a rather
> micro viewpoint.
>
> - Considering on how canSkipPartConstraintValidation is called, I
>   *think* that RelationProvenValid() would be better.  (But this
>   would be disappear by rebasing..)
>
> I think RelationProvenValid() is bit confusing in the sense that, it does
not
imply the meaning that some constraint is being checke


> - 0002 changes the interface of get_qual_for_list, but left
>   get_qual_for_range alone.  Anyway get_qual_for_range will have
>   to do the similar thing soon.
>
> Yes, this will be taken care with default partition for range.


> - In check_new_partition_bound, "overlap" and "with" is
>   completely correlated with each other. "with > -1" means
>   "overlap = true". So overlap is not useless. ("with" would be
>   better to be "overlap_with" or somehting if we remove
>   "overlap")
>
> Agree, probably this can be taken as a separate refactoring patch.
Currently,
for in case of default I have got rid of "overlap", and now use of "with"
and
that is also used just for code simplification.


> - The error message of check_default_allows_bound is below.
>
>   "updated partition constraint for default partition \"%s\"
>would be violated by some row"
>
>   This looks an analog of validateCheckConstraint but as my
>   understanding this function is called only when new partition
>   is added. This would be difficult to recognize in the
>   situation.
>
>   "the default partition contains rows that should be in
>the new partition: \"%s\""
>
>   or something?
>
> I think the current error message is more clearer. Agree that there might
be
sort of confusion if it's due to addition or attach partition, but we have
already stretched the message longer. I am open to suggestions here.


> - In check_default_allows_bound, the iteration over partitions is
>   quite similar to what validateCheckConstraint does. Can we
>   somehow share validateCheckConstraint with this function?
>
> May be we can, but I think again this can also be categorized as
refactoring
patch and done later maybe. Your thoughts?


> - In the same function, skipping RELKIND_PARTITIONED_TABLE is
>   okay, but silently ignoring RELKIND_FOREIGN_TABLE doesn't seem
>   good. I think at least some warning should be emitted.
>
>   "Skipping foreign tables in the defalut partition. It might
>contain rows that should be in the new partition."  (Needs
>preventing multple warnings in single call, maybe)
>
> Currently we do not emit any warning when attaching a foreign table as a
non-default partition having rows that do not match its partition
constraints
and we still let attach the partition.
But, I agree that we should emit such a warning, I added a code to do this.


> - In the same function, the following condition seems somewhat
>   strange in comparison to validateCheckConstraint.
>
> > if (partqualstate && ExecCheck(partqualstate, econtext))
>
>   partqualstate won't be null as long as partition_constraint is
>   valid. Anyway (I'm believing that) an invalid constraint
>   results in error by ExecPrepareExpr. Therefore 'if
>   (partqualstate' is useless.
>
> Removed the check for partqualstate.


> - In gram.y, the nonterminal for list spec clause is still
>   "ForValues". It seems somewhat strange. partition_spec or
>   something would be better.
>
> Done.
Thanks for catching this, I agree with you.
I have changed the name to PartitionBoundSpec.


> - This is not a part of this patch, but in ruleutils.c, the error
>   for unknown paritioning strategy is emitted as following.
>
> >   elog(ERROR, "unrecognized partition strategy: %d",
> >(int) strategy);
>
>   The cast is added because the strategy is a char. I suppose
>   this is because strategy can be an unprintable. I'd like to see
>   a comment if it is correct.
>
> I think this should be taken separately.

Thanks,
Jeevan Ladhe

Refs:
[1] https://www.postgresql.org/message-id/CAOgcT0OARciE2X%
2BU0rjSKp9VuC279dYcCGkc3nCWKhHQ1_m2rw%40mail.gmail.com

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Ashutosh,

I have tried to address your comments in the V22 set of patches[1].
Please find my feedback inlined on your comments.

On Thu, Jun 15, 2017 at 10:24 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> Some more comments on the latest set of patches.
>
> In heap_drop_with_catalog(), we heap_open() the parent table to get the
> default partition OID, if any. If the relcache doesn't have an entry for
> the
> parent, this means that the entry will be created, only to be invalidated
> at
> the end of the function. If there is no default partition, this all is
> completely unnecessary. We should avoid heap_open() in this case. This also
> means that get_default_partition_oid() should not rely on the relcache
> entry,
> but should growl through pg_inherit to find the default partition.
>

Instead of reading the defaultOid from cache, as suggested by Amit here[2],
now
I have stored this in pg_partition_table, and reading it from there.


> In get_qual_for_list(), if the table has only default partition, it won't
> have
> any boundinfo. In such a case the default partition's constraint would
> come out
> as (NOT ((a IS NOT NULL) AND (a = ANY (ARRAY[]::integer[]. The empty
> array
> looks odd and may be we spend a few CPU cycles executing ANY on an empty
> array.
> We have the same problem with a partition containing only NULL value. So,
> may
> be this one is not that bad.
>
> Fixed.


> Please add a testcase to test addition of default partition as the first
> partition.
>
> Added this in insert.sql rather than create_table.sql, as the purpose here
is to test if default being a first partition accepts any values for the key
including null.


> get_qual_for_list() allocates the constant expressions corresponding to the
> datums in CacheMemoryContext while constructing constraints for a default
> partition. We do not do this for other partitions. We may not be
> constructing
> the constraints for saving in the cache. For example, ATExecAttachPartition
> constructs the constraints for validation. In such a case, this code will
> unnecessarily clobber the cache memory. generate_partition_qual() copies
> the
> partition constraint in the CacheMemoryContext.
>

Removed CacheMemoryContext.
I thought once the partition qual is generated, it should be in remain in
the memory context, but when it is needed, it is indirectly taken care by
generate_partition_qual() in following code:

/* Save a copy in the relcache */
oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
rel->rd_partcheck = copyObject(result);
MemoryContextSwitchTo(oldcxt);


>
> +   if (spec->is_default)
> +   {
> +   result = list_make1(make_ands_explicit(result));
> +   result = list_make1(makeBoolExpr(NOT_EXPR, result, -1));
> +   }
>
> If the "result" is an OR expression, calling make_ands_explicit() on it
> would
> create AND(OR(...)) expression, with an unnecessary AND. We want to avoid
> that?
>
>
Actually the OR expression here is generated using a call to makeBoolExpr(),
which returns a single expression node, and if this is passed to
make_ands_explicit(), it checks if the list length is node, returns the
initial
node itself, and hence AND(OR(...)) kind of expression is not generated
here.


> +   if (cur_index < 0 && (partition_bound_has_default(
> partdesc->boundinfo)))
> +   cur_index = partdesc->boundinfo->default_index;
> +
> The partition_bound_has_default() check is unnecessary since we check for
> cur_index < 0 next anyway.
>
> Done.


> + *
> + * Given the parent relation checks if it has default partition, and if it
> + * does exist returns its oid, otherwise returns InvalidOid.
> + */
> May be reworded as "If the given relation has a default partition, this
> function returns the OID of the default partition. Otherwise it returns
> InvalidOid."
>
> I have reworded this to:
"If the given relation has a default partition return the OID of the default
partition, otherwise return InvalidOid."


> +Oid
> +get_default_partition_oid(Relation parent)
> +{
> +   PartitionDesc partdesc = RelationGetPartitionDesc(parent);
> +
> +   if (partdesc->boundinfo && partition_bound_has_default(
> partdesc->boundinfo))
> +   return partdesc->oids[partdesc->boundinfo->default_index];
> +
> +   return InvalidOid;
> +}
> An unpartitioned table would not have partdesc set set. So, this function
> will
> segfault if we pass an unpartitioned table. Either Assert that partdesc
> should
> exist or check for its NULL-ness.
>

Fixed.


>
>
> +defaultPartOid = get_default_partition_oid(rel);
> +if (OidIsValid(defaultPartOid))
> +ereport(ERROR,
> +(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> + errmsg("there exists a default partition for table
> \"%s\", cannot attach a new partition",
> +RelationGetRelationName(rel;
> +
> Should be done before heap_open on the table being attached. If we are not
> going to attach the partition, there's no 

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Robert,

I have tried to address your comments in the V22 set of patches[1].
Please find my feedback inlined on your comments.


On Thu, Jun 15, 2017 at 1:21 AM, Robert Haas  wrote:
>
> - Needs to be rebased over b08df9cab777427fdafe633ca7b8abf29817aa55.
>

Rebased on master latest commit: ca793c59a51e94cedf8cbea5c29668bf8fa298f3


> - Still no documentation.
>
Yes, this is long pending, and I will make this is a priority to get it
included
in next set of my patches.

- Should probably be merged with the patch to add default partitioning
> for ranges.


Beena is already rebasing her patch on my latest patches, so I think getting
them merged here won't be an issue, mostly will be just like one more patch
on top my patches.


> Other stuff I noticed:
>
> - The regression tests don't seem to check that the scan-skipping
> logic works as expected.  We have regression tests for that case for
> attaching regular partitions, and it seems like it would be worth
> testing the default-partition case as well.
>

Added a test case for default in alter_table.sql.

- check_default_allows_bound() assumes that if
> canSkipPartConstraintValidation() fails for the default partition, it
> will also fail for every subpartition of the default partition.  That
> is, once we commit to scanning one child partition, we're committed to
> scanning them all.  In practice, that's probably not a huge
> limitation, but if it's not too much code, we could keep the top-level
> check but also check each partitioning individually as we reach it,
> and skip the scan for any individual partitions for which the
> constraint can be proven.  For example, suppose the top-level table is
> list-partitioned with a partition for each of the most common values,
> and then we range-partition the default partition.
>

I have tried to address this in patch 0007, please let me know your views on
that patch.

- The changes to the regression test results in 0004 make the error
> messages slightly worse.  The old message names the default partition,
> whereas the new one does not.  Maybe that's worth avoiding.


The only way for this, I can think of to achieve this is to store the name
of
the default relation in AlteredTableInfo, currently I am using a flag for
realizing if the scanned table is a default partition to throw specific
error.
But, IMO storing a string in AlteredTableInfo just for error purpose might
be
overkill. Your suggestions?


> Specific comments:
>
> + * Also, invalidate the parent's and a sibling default partition's
> relcache,
> + * so that the next rebuild will load the new partition's info into
> parent's
> + * partition descriptor and default partition constraints(which are
> dependent
> + * on other partition bounds) are built anew.
>
> I find this a bit unclear, and it also repeats the comment further
> down.  Maybe something like: Also, invalidate the parent's relcache
> entry, so that the next rebuild will load he new partition's info into
> its partition descriptor.  If there is a default partition, we must
> invalidate its relcache entry as well.
>
> Done.


> +/*
> + * The default partition constraints depend upon the partition bounds
> of
> + * other partitions. Adding a new(or even removing existing) partition
> + * would invalidate the default partition constraints. Invalidate the
> + * default partition's relcache so that the constraints are built
> anew and
> + * any plans dependent on those constraints are invalidated as well.
> + */
>
> Here, I'd write: The partition constraint for the default partition
> depends on the partition bounds of every other partition, so we must
> invalidate the relcache entry for that partition every time a
> partition is added or removed.
>
> Done.


> +/*
> + * Default partition cannot be added if there already
> + * exists one.
> + */
> +if (spec->is_default)
> +{
> +overlap = partition_bound_has_default(boundinfo);
> +with = boundinfo->default_index;
> +break;
> +}
>
> To support default partitioning for range, this is going to have to be
> moved above the switch rather than done inside of it.  And there's
> really no downside to putting it there.
>
> Done.


> + * constraint, by *proving* that the existing constraints of the table
> + * *imply* the given constraints.  We include the table's check
> constraints and
>
> Both the comma and the asterisks are unnecessary.
>
> Done.


> + * Check whether all rows in the given table (scanRel) obey given
> partition
>
> obey the given
>
> I think the larger comment block could be tightened up a bit, like
> this:  Check whether all rows in the given table obey the given
> partition constraint; if so, it can be attached as a partition.  We do
> this by scanning the table (or all of its 

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi,

On Mon, Jun 19, 2017 at 12:34 PM, Amit Langote <
langote_amit...@lab.ntt.co.jp> wrote:

> On 2017/06/16 14:16, Ashutosh Bapat wrote:
> > On Fri, Jun 16, 2017 at 12:48 AM, Robert Haas 
> wrote:
> >> On Thu, Jun 15, 2017 at 12:54 PM, Ashutosh Bapat
> >>  wrote:
> >>> Some more comments on the latest set of patches.
> >> or looking up the OID in the
> >> relcache multiple times.
> >
> > I am not able to understand this in the context of default partition.
> > After that nobody else is going to change its partitions and their
> > bounds (since both of those require heap_open on parent which would be
> > stuck on the lock we hold.). So, we have to check only once if the
> > table has a default partition. If it doesn't, it's not going to
> > acquire one unless we release the lock on the parent i.e at the end of
> > transaction. If it has one, it's not going to get dropped till the end
> > of the transaction for the same reason. I don't see where we are
> > looking up OIDs multiple times.
>
> Without heap_opening the parent, the only way is to look up parentOid's
> children in pg_inherits and for each child looking up its pg_class tuple
> in the syscache to see if its relpartbound indicates that it's a default
> partition.  That seems like it won't be inexpensive either.
>
> It would be nice if could get that information (that is - is a given
> relation being heap_drop_with_catalog'd a partition of the parent that
> happens to have default partition) in less number of steps than that.
> Having that information in relcache is one way, but as mentioned, that
> turns out be expensive.
>
> Has anyone considered the idea of putting the default partition OID in the
> pg_partitioned_table catalog?  Looking the above information up would
> amount to one syscache lookup.  Default partition seems to be special
> enough object to receive a place in the pg_partitioned_table tuple of the
> parent.  Thoughts?
>

I liked this suggestion. Having an entry in pg_partitioned_table would avoid
both expensive methods, i.e. 1. opening the parent or 2. lookup for
each of the children first in pg_inherits and then its corresponding entry
in
pg_class.
Unless anybody has any other suggestions/comments here, I am going to
implement this suggestion.

Thanks,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Wed, Jun 21, 2017 at 8:47 PM, Amit Langote
 wrote:
> It's the comma inside the error message that suggests to me that it's a
> style that I haven't seen elsewhere in the backend code.

Exactly.  Avoid that.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Amit Langote]

On 2017/06/21 21:37, Jeevan Ladhe wrote:
> Hi Amit,
> 
> On Thu, Jun 15, 2017 at 12:31 PM, Amit Langote <
> langote_amit...@lab.ntt.co.jp> wrote:
> 
>> Oops, I meant to send one more comment.
>>
>> On 2017/06/15 15:48, Amit Langote wrote:
>>> BTW, I noticed the following in 0002
>> +errmsg("there exists a default
>> partition for table \"%s\", cannot
>> add a new partition",
>>
>> This error message style seems novel to me.  I'm not sure about the best
>> message text here, but maybe: "cannot add new partition to table \"%s\"
>> with default partition"
>>
> 
> This sounds confusing to me, what about something like:
> "\"%s\" has a default partition, cannot add a new partition."

It's the comma inside the error message that suggests to me that it's a
style that I haven't seen elsewhere in the backend code.  The primary
error message here is that the new partition cannot be created.  "%s has
default partition" seems to me to belong in errdetail() (see "What Goes
Where" in [1].)

Or write the sentence such that the comma is not required.  Anyway, we can
leave this for the committer to decide.

> Note that this comment belongs to patch 0002, and it will go away
> in case we are going to have extended functionality i.e. patch 0003,
> as in that patch we allow user to create a new partition even in the
> cases when there exists a default partition.

Oh, that'd be great.  It's always better to get rid of the error
conditions that are hard to communicate to users. :)  (Although, this
one's not that ambiguous.)

Thanks,
Amit

[1] https://www.postgresql.org/docs/devel/static/error-style-guide.html



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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Thanks Ashutosh and Kyotaro for reviewing further.
I shall address your comments in next version of my patch.

Regards,
Jeevan Ladhe

On Fri, Jun 16, 2017 at 1:46 PM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:

> Hello, I'd like to review this but it doesn't fit the master, as
> Robert said. Especially the interface of predicate_implied_by is
> changed by the suggested commit.
>
> Anyway I have some comment on this patch with fresh eyes.  I
> believe the basic design so my comment below are from a rather
> micro viewpoint.
>
> At Thu, 15 Jun 2017 16:01:53 +0900, Amit Langote <
> langote_amit...@lab.ntt.co.jp> wrote in  34ff801bf...@lab.ntt.co.jp>
> > Oops, I meant to send one more comment.
> >
> > On 2017/06/15 15:48, Amit Langote wrote:
> > > BTW, I noticed the following in 0002
> > +  errmsg("there exists a default
> partition for table \"%s\", cannot
> > add a new partition",
> >
> > This error message style seems novel to me.  I'm not sure about the best
> > message text here, but maybe: "cannot add new partition to table \"%s\"
> > with default partition"
> >
> > Note that the comment applies to both DefineRelation and
> > ATExecAttachPartition.
>
> - Considering on how canSkipPartConstraintValidation is called, I
>   *think* that RelationProvenValid() would be better.  (But this
>   would be disappear by rebasing..)
>
> - 0002 changes the interface of get_qual_for_list, but left
>   get_qual_for_range alone.  Anyway get_qual_for_range will have
>   to do the similar thing soon.
>
> - In check_new_partition_bound, "overlap" and "with" is
>   completely correlated with each other. "with > -1" means
>   "overlap = true". So overlap is not useless. ("with" would be
>   better to be "overlap_with" or somehting if we remove
>   "overlap")
>
> - The error message of check_default_allows_bound is below.
>
>   "updated partition constraint for default partition \"%s\"
>would be violated by some row"
>
>   This looks an analog of validateCheckConstraint but as my
>   understanding this function is called only when new partition
>   is added. This would be difficult to recognize in the
>   situation.
>
>   "the default partition contains rows that should be in
>the new partition: \"%s\""
>
>   or something?
>
> - In check_default_allows_bound, the iteration over partitions is
>   quite similar to what validateCheckConstraint does. Can we
>   somehow share validateCheckConstraint with this function?
>
> - In the same function, skipping RELKIND_PARTITIONED_TABLE is
>   okay, but silently ignoring RELKIND_FOREIGN_TABLE doesn't seem
>   good. I think at least some warning should be emitted.
>
>   "Skipping foreign tables in the defalut partition. It might
>contain rows that should be in the new partition."  (Needs
>preventing multple warnings in single call, maybe)
>
> - In the same function, the following condition seems somewhat
>   strange in comparison to validateCheckConstraint.
>
> > if (partqualstate && ExecCheck(partqualstate, econtext))
>
>   partqualstate won't be null as long as partition_constraint is
>   valid. Anyway (I'm believing that) an invalid constraint
>   results in error by ExecPrepareExpr. Therefore 'if
>   (partqualstate' is useless.
>
> - In gram.y, the nonterminal for list spec clause is still
>   "ForValues". It seems somewhat strange. partition_spec or
>   something would be better.
>
> - This is not a part of this patch, but in ruleutils.c, the error
>   for unknown paritioning strategy is emitted as following.
>
> >   elog(ERROR, "unrecognized partition strategy: %d",
> >(int) strategy);
>
>   The cast is added because the strategy is a char. I suppose
>   this is because strategy can be an unprintable. I'd like to see
>   a comment if it is correct.
>
>
> regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
>

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Amit,

On Thu, Jun 15, 2017 at 12:31 PM, Amit Langote <
langote_amit...@lab.ntt.co.jp> wrote:

> Oops, I meant to send one more comment.
>
> On 2017/06/15 15:48, Amit Langote wrote:
> > BTW, I noticed the following in 0002
> +errmsg("there exists a default
> partition for table \"%s\", cannot
> add a new partition",
>
> This error message style seems novel to me.  I'm not sure about the best
> message text here, but maybe: "cannot add new partition to table \"%s\"
> with default partition"
>

This sounds confusing to me, what about something like:
"\"%s\" has a default partition, cannot add a new partition."

Note that this comment belongs to patch 0002, and it will go away
in case we are going to have extended functionality i.e. patch 0003,
as in that patch we allow user to create a new partition even in the
cases when there exists a default partition.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi,

Sorry for being away from here.
I had some issues with my laptop, and I have resumed working on this.

On Thu, Jun 15, 2017 at 1:21 AM, Robert Haas  wrote:

> On Wed, Jun 14, 2017 at 8:02 AM, Jeevan Ladhe
>  wrote:
> > Here are the details of the patches in attached zip.
> > 0001. refactoring existing ATExecAttachPartition  code so that it can be
> > used for
> > default partitioning as well
> > 0002. support for default partition with the restriction of preventing
> > addition
> > of any new partition after default partition.
> > 0003. extend default partitioning support to allow addition of new
> > partitions.
> > 0004. extend default partitioning validation code to reuse the refactored
> > code
> > in patch 0001.
>
> I think the core ideas of this patch are pretty solid now.  It's come
> a long way in the last month.  High-level comments:
>

Thanks Robert for looking into this.


> - Needs to be rebased over b08df9cab777427fdafe633ca7b8abf29817aa55.
>

Will rebase.


> - Still no documentation.
> - Should probably be merged with the patch to add default partitioning
> for ranges.
>
Will try to get this soon.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Amit Langote]

On 2017/06/16 14:16, Ashutosh Bapat wrote:
> On Fri, Jun 16, 2017 at 12:48 AM, Robert Haas  wrote:
>> On Thu, Jun 15, 2017 at 12:54 PM, Ashutosh Bapat
>>  wrote:
>>> Some more comments on the latest set of patches.
>>>
>>> In heap_drop_with_catalog(), we heap_open() the parent table to get the
>>> default partition OID, if any. If the relcache doesn't have an entry for the
>>> parent, this means that the entry will be created, only to be invalidated at
>>> the end of the function. If there is no default partition, this all is
>>> completely unnecessary. We should avoid heap_open() in this case. This also
>>> means that get_default_partition_oid() should not rely on the relcache 
>>> entry,
>>> but should growl through pg_inherit to find the default partition.
>>
>> I am *entirely* unconvinced by this line of argument.  I think we want
>> to open the relation the first time we touch it and pass the Relation
>> around thereafter.
> 
> If this would be correct, why heap_drop_with_catalog() without this
> patch just locks the parent and doesn't call a heap_open(). I am
> missing something.

As of commit c1e0e7e1d790bf, we avoid creating relcache entry for the
parent.  Before that commit, drop table
partitioned_table_with_many_partitions used to take too long and consumed
quite some memory as result of relcache invalidation requested at the end
on the parent table for every partition.

If this patch reintroduces the heap_open() on the parent table, that's
going to bring back the problem fixed by that commit.

>> Anything else is prone to accidentally failing to
>> have the relation locked early enough,
> 
> We are locking the parent relation even without this patch, so this
> isn't an issue.

Yes.

>> or looking up the OID in the
>> relcache multiple times.
> 
> I am not able to understand this in the context of default partition.
> After that nobody else is going to change its partitions and their
> bounds (since both of those require heap_open on parent which would be
> stuck on the lock we hold.). So, we have to check only once if the
> table has a default partition. If it doesn't, it's not going to
> acquire one unless we release the lock on the parent i.e at the end of
> transaction. If it has one, it's not going to get dropped till the end
> of the transaction for the same reason. I don't see where we are
> looking up OIDs multiple times.

Without heap_opening the parent, the only way is to look up parentOid's
children in pg_inherits and for each child looking up its pg_class tuple
in the syscache to see if its relpartbound indicates that it's a default
partition.  That seems like it won't be inexpensive either.

It would be nice if could get that information (that is - is a given
relation being heap_drop_with_catalog'd a partition of the parent that
happens to have default partition) in less number of steps than that.
Having that information in relcache is one way, but as mentioned, that
turns out be expensive.

Has anyone considered the idea of putting the default partition OID in the
pg_partitioned_table catalog?  Looking the above information up would
amount to one syscache lookup.  Default partition seems to be special
enough object to receive a place in the pg_partitioned_table tuple of the
parent.  Thoughts?

>>> +defaultPartOid = get_default_partition_oid(rel);
>>> +if (OidIsValid(defaultPartOid))
>>> +ereport(ERROR,
>>> +(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
>>> + errmsg("there exists a default partition for table
>>> \"%s\", cannot attach a new partition",
>>> +RelationGetRelationName(rel;
>>> +
>>> Should be done before heap_open on the table being attached. If we are not
>>> going to attach the partition, there's no point in instantiating its 
>>> relcache.
>>
>> No, because we should take the lock before examining any properties of
>> the table.
> 
> There are three tables involved here. "rel" which is the partitioned
> table. "attachrel" is the table being attached as a partition to "rel"
> and defaultrel, which is the default partition table. If there exists
> a default partition in "rel" we are not allowing "attachrel" to be
> attached to "rel". If that's the case, we don't need to examine any
> properties of "attachrel" and hence we don't need to instantiate
> relcache of "attachrel". That's what the comment is about.
> ATExecAttachPartition() receives "rel" as an argument which has been
> already locked and opened. So, we can check the existence of default
> partition right at the beginning of the function.

It seems that we are examining the properties of the parent table here
(whether it has default partition), which as Ashutosh mentions, is already
locked before we got to ATExecAttachPartition().  Another place where we
are ereporting before locking the table to be attached (actually even
before looking it up by name), based just on the properties 

Re: [HACKERS] Adding support for Default partition in partitioning

[Kyotaro HORIGUCHI]

Hello, I'd like to review this but it doesn't fit the master, as
Robert said. Especially the interface of predicate_implied_by is
changed by the suggested commit.

Anyway I have some comment on this patch with fresh eyes.  I
believe the basic design so my comment below are from a rather
micro viewpoint.

At Thu, 15 Jun 2017 16:01:53 +0900, Amit Langote 
 wrote in 

> Oops, I meant to send one more comment.
> 
> On 2017/06/15 15:48, Amit Langote wrote:
> > BTW, I noticed the following in 0002
> +  errmsg("there exists a default 
> partition for table \"%s\", cannot
> add a new partition",
> 
> This error message style seems novel to me.  I'm not sure about the best
> message text here, but maybe: "cannot add new partition to table \"%s\"
> with default partition"
> 
> Note that the comment applies to both DefineRelation and
> ATExecAttachPartition.

- Considering on how canSkipPartConstraintValidation is called, I
  *think* that RelationProvenValid() would be better.  (But this
  would be disappear by rebasing..)

- 0002 changes the interface of get_qual_for_list, but left
  get_qual_for_range alone.  Anyway get_qual_for_range will have
  to do the similar thing soon.

- In check_new_partition_bound, "overlap" and "with" is
  completely correlated with each other. "with > -1" means
  "overlap = true". So overlap is not useless. ("with" would be
  better to be "overlap_with" or somehting if we remove
  "overlap")

- The error message of check_default_allows_bound is below.

  "updated partition constraint for default partition \"%s\"
   would be violated by some row"

  This looks an analog of validateCheckConstraint but as my
  understanding this function is called only when new partition
  is added. This would be difficult to recognize in the
  situation.

  "the default partition contains rows that should be in
   the new partition: \"%s\""

  or something?

- In check_default_allows_bound, the iteration over partitions is
  quite similar to what validateCheckConstraint does. Can we
  somehow share validateCheckConstraint with this function?

- In the same function, skipping RELKIND_PARTITIONED_TABLE is
  okay, but silently ignoring RELKIND_FOREIGN_TABLE doesn't seem
  good. I think at least some warning should be emitted.

  "Skipping foreign tables in the defalut partition. It might
   contain rows that should be in the new partition."  (Needs
   preventing multple warnings in single call, maybe)

- In the same function, the following condition seems somewhat
  strange in comparison to validateCheckConstraint.

> if (partqualstate && ExecCheck(partqualstate, econtext))

  partqualstate won't be null as long as partition_constraint is
  valid. Anyway (I'm believing that) an invalid constraint
  results in error by ExecPrepareExpr. Therefore 'if
  (partqualstate' is useless.

- In gram.y, the nonterminal for list spec clause is still
  "ForValues". It seems somewhat strange. partition_spec or
  something would be better.

- This is not a part of this patch, but in ruleutils.c, the error
  for unknown paritioning strategy is emitted as following.

>   elog(ERROR, "unrecognized partition strategy: %d",
>(int) strategy);

  The cast is added because the strategy is a char. I suppose
  this is because strategy can be an unprintable. I'd like to see
  a comment if it is correct.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Fri, Jun 16, 2017 at 12:48 AM, Robert Haas  wrote:
> On Thu, Jun 15, 2017 at 12:54 PM, Ashutosh Bapat
>  wrote:
>> Some more comments on the latest set of patches.
>>
>> In heap_drop_with_catalog(), we heap_open() the parent table to get the
>> default partition OID, if any. If the relcache doesn't have an entry for the
>> parent, this means that the entry will be created, only to be invalidated at
>> the end of the function. If there is no default partition, this all is
>> completely unnecessary. We should avoid heap_open() in this case. This also
>> means that get_default_partition_oid() should not rely on the relcache entry,
>> but should growl through pg_inherit to find the default partition.
>
> I am *entirely* unconvinced by this line of argument.  I think we want
> to open the relation the first time we touch it and pass the Relation
> around thereafter.

If this would be correct, why heap_drop_with_catalog() without this
patch just locks the parent and doesn't call a heap_open(). I am
missing something.

> Anything else is prone to accidentally failing to
> have the relation locked early enough,

We are locking the parent relation even without this patch, so this
isn't an issue.

> or looking up the OID in the
> relcache multiple times.

I am not able to understand this in the context of default partition.
After that nobody else is going to change its partitions and their
bounds (since both of those require heap_open on parent which would be
stuck on the lock we hold.). So, we have to check only once if the
table has a default partition. If it doesn't, it's not going to
acquire one unless we release the lock on the parent i.e at the end of
transaction. If it has one, it's not going to get dropped till the end
of the transaction for the same reason. I don't see where we are
looking up OIDs multiple times.


>
>> +defaultPartOid = get_default_partition_oid(rel);
>> +if (OidIsValid(defaultPartOid))
>> +ereport(ERROR,
>> +(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
>> + errmsg("there exists a default partition for table
>> \"%s\", cannot attach a new partition",
>> +RelationGetRelationName(rel;
>> +
>> Should be done before heap_open on the table being attached. If we are not
>> going to attach the partition, there's no point in instantiating its 
>> relcache.
>
> No, because we should take the lock before examining any properties of
> the table.

There are three tables involved here. "rel" which is the partitioned
table. "attachrel" is the table being attached as a partition to "rel"
and defaultrel, which is the default partition table. If there exists
a default partition in "rel" we are not allowing "attachrel" to be
attached to "rel". If that's the case, we don't need to examine any
properties of "attachrel" and hence we don't need to instantiate
relcache of "attachrel". That's what the comment is about.
ATExecAttachPartition() receives "rel" as an argument which has been
already locked and opened. So, we can check the existence of default
partition right at the beginning of the function.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Thu, Jun 15, 2017 at 12:54 PM, Ashutosh Bapat
 wrote:
> Some more comments on the latest set of patches.
>
> In heap_drop_with_catalog(), we heap_open() the parent table to get the
> default partition OID, if any. If the relcache doesn't have an entry for the
> parent, this means that the entry will be created, only to be invalidated at
> the end of the function. If there is no default partition, this all is
> completely unnecessary. We should avoid heap_open() in this case. This also
> means that get_default_partition_oid() should not rely on the relcache entry,
> but should growl through pg_inherit to find the default partition.

I am *entirely* unconvinced by this line of argument.  I think we want
to open the relation the first time we touch it and pass the Relation
around thereafter.  Anything else is prone to accidentally failing to
have the relation locked early enough, or looking up the OID in the
relcache multiple times.

> In get_qual_for_list(), if the table has only default partition, it won't have
> any boundinfo. In such a case the default partition's constraint would come 
> out
> as (NOT ((a IS NOT NULL) AND (a = ANY (ARRAY[]::integer[]. The empty array
> looks odd and may be we spend a few CPU cycles executing ANY on an empty 
> array.
> We have the same problem with a partition containing only NULL value. So, may
> be this one is not that bad.

I think that one is probably worth fixing.

> Please add a testcase to test addition of default partition as the first
> partition.

That seems like a good idea, too.

> get_qual_for_list() allocates the constant expressions corresponding to the
> datums in CacheMemoryContext while constructing constraints for a default
> partition. We do not do this for other partitions. We may not be constructing
> the constraints for saving in the cache. For example, ATExecAttachPartition
> constructs the constraints for validation. In such a case, this code will
> unnecessarily clobber the cache memory. generate_partition_qual() copies the
> partition constraint in the CacheMemoryContext.
>
> +   if (spec->is_default)
> +   {
> +   result = list_make1(make_ands_explicit(result));
> +   result = list_make1(makeBoolExpr(NOT_EXPR, result, -1));
> +   }

Clearly we do not want things to end up across multiple contexts.  We
should ensure that anything linked from the relcache entry ends up in
CacheMemoryContext, but we must be careful not to allocate anything
else in there, because CacheMemoryContext is never reset.

> If the "result" is an OR expression, calling make_ands_explicit() on it would
> create AND(OR(...)) expression, with an unnecessary AND. We want to avoid 
> that?

I'm not sure it's worth the trouble.

> +defaultPartOid = get_default_partition_oid(rel);
> +if (OidIsValid(defaultPartOid))
> +ereport(ERROR,
> +(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> + errmsg("there exists a default partition for table
> \"%s\", cannot attach a new partition",
> +RelationGetRelationName(rel;
> +
> Should be done before heap_open on the table being attached. If we are not
> going to attach the partition, there's no point in instantiating its relcache.

No, because we should take the lock before examining any properties of
the table.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

Some more comments on the latest set of patches.

In heap_drop_with_catalog(), we heap_open() the parent table to get the
default partition OID, if any. If the relcache doesn't have an entry for the
parent, this means that the entry will be created, only to be invalidated at
the end of the function. If there is no default partition, this all is
completely unnecessary. We should avoid heap_open() in this case. This also
means that get_default_partition_oid() should not rely on the relcache entry,
but should growl through pg_inherit to find the default partition.

In get_qual_for_list(), if the table has only default partition, it won't have
any boundinfo. In such a case the default partition's constraint would come out
as (NOT ((a IS NOT NULL) AND (a = ANY (ARRAY[]::integer[]. The empty array
looks odd and may be we spend a few CPU cycles executing ANY on an empty array.
We have the same problem with a partition containing only NULL value. So, may
be this one is not that bad.

Please add a testcase to test addition of default partition as the first
partition.

get_qual_for_list() allocates the constant expressions corresponding to the
datums in CacheMemoryContext while constructing constraints for a default
partition. We do not do this for other partitions. We may not be constructing
the constraints for saving in the cache. For example, ATExecAttachPartition
constructs the constraints for validation. In such a case, this code will
unnecessarily clobber the cache memory. generate_partition_qual() copies the
partition constraint in the CacheMemoryContext.

+   if (spec->is_default)
+   {
+   result = list_make1(make_ands_explicit(result));
+   result = list_make1(makeBoolExpr(NOT_EXPR, result, -1));
+   }

If the "result" is an OR expression, calling make_ands_explicit() on it would
create AND(OR(...)) expression, with an unnecessary AND. We want to avoid that?

+   if (cur_index < 0 && (partition_bound_has_default(partdesc->boundinfo)))
+   cur_index = partdesc->boundinfo->default_index;
+
The partition_bound_has_default() check is unnecessary since we check for
cur_index < 0 next anyway.

+ *
+ * Given the parent relation checks if it has default partition, and if it
+ * does exist returns its oid, otherwise returns InvalidOid.
+ */
May be reworded as "If the given relation has a default partition, this
function returns the OID of the default partition. Otherwise it returns
InvalidOid."

+Oid
+get_default_partition_oid(Relation parent)
+{
+   PartitionDesc partdesc = RelationGetPartitionDesc(parent);
+
+   if (partdesc->boundinfo && partition_bound_has_default(partdesc->boundinfo))
+   return partdesc->oids[partdesc->boundinfo->default_index];
+
+   return InvalidOid;
+}
An unpartitioned table would not have partdesc set set. So, this function will
segfault if we pass an unpartitioned table. Either Assert that partdesc should
exist or check for its NULL-ness.


+defaultPartOid = get_default_partition_oid(rel);
+if (OidIsValid(defaultPartOid))
+ereport(ERROR,
+(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("there exists a default partition for table
\"%s\", cannot attach a new partition",
+RelationGetRelationName(rel;
+
Should be done before heap_open on the table being attached. If we are not
going to attach the partition, there's no point in instantiating its relcache.

The comment in heap_drop_with_catalog() should mention why we lock the default
partition before locking the table being dropped.

 extern List *preprune_pg_partitions(PlannerInfo *root, RangeTblEntry *rte,
Index rti, Node *quals, LOCKMODE lockmode);
-
 #endif   /* PARTITION_H */
Unnecessary hunk.

On Thu, Jun 15, 2017 at 12:31 PM, Amit Langote
 wrote:
> Oops, I meant to send one more comment.
>
> On 2017/06/15 15:48, Amit Langote wrote:
>> BTW, I noticed the following in 0002
> +errmsg("there exists a default 
> partition for table \"%s\", cannot
> add a new partition",
>
> This error message style seems novel to me.  I'm not sure about the best
> message text here, but maybe: "cannot add new partition to table \"%s\"
> with default partition"
>
> Note that the comment applies to both DefineRelation and
> ATExecAttachPartition.
>
> Thanks,
> Amit
>



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Amit Langote]

Oops, I meant to send one more comment.

On 2017/06/15 15:48, Amit Langote wrote:
> BTW, I noticed the following in 0002
+errmsg("there exists a default 
partition for table \"%s\", cannot
add a new partition",

This error message style seems novel to me.  I'm not sure about the best
message text here, but maybe: "cannot add new partition to table \"%s\"
with default partition"

Note that the comment applies to both DefineRelation and
ATExecAttachPartition.

Thanks,
Amit



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

Re: [HACKERS] Adding support for Default partition in partitioning

[Amit Langote]

On 2017/06/15 4:51, Robert Haas wrote:
> On Wed, Jun 14, 2017 at 8:02 AM, Jeevan Ladhe
>  wrote:
>> Here are the details of the patches in attached zip.
>> 0001. refactoring existing ATExecAttachPartition  code so that it can be
>> used for
>> default partitioning as well
>> 0002. support for default partition with the restriction of preventing
>> addition
>> of any new partition after default partition.
>> 0003. extend default partitioning support to allow addition of new
>> partitions.
>> 0004. extend default partitioning validation code to reuse the refactored
>> code
>> in patch 0001.
> 
> I think the core ideas of this patch are pretty solid now.  It's come
> a long way in the last month.

+1


BTW, I noticed the following in 0002:

@@ -1322,15 +1357,59 @@ get_qual_for_list(PartitionKey key,
PartitionBoundSpec *spec)

[ ... ]

+   oldcxt = MemoryContextSwitchTo(CacheMemoryContext);

I'm not sure if we need to do that.  Can you explain?

Thanks,
Amit



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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Wed, Jun 14, 2017 at 8:02 AM, Jeevan Ladhe
 wrote:
> Here are the details of the patches in attached zip.
> 0001. refactoring existing ATExecAttachPartition  code so that it can be
> used for
> default partitioning as well
> 0002. support for default partition with the restriction of preventing
> addition
> of any new partition after default partition.
> 0003. extend default partitioning support to allow addition of new
> partitions.
> 0004. extend default partitioning validation code to reuse the refactored
> code
> in patch 0001.

I think the core ideas of this patch are pretty solid now.  It's come
a long way in the last month.  High-level comments:

- Needs to be rebased over b08df9cab777427fdafe633ca7b8abf29817aa55.
- Still no documentation.
- Should probably be merged with the patch to add default partitioning
for ranges.

Other stuff I noticed:

- The regression tests don't seem to check that the scan-skipping
logic works as expected.  We have regression tests for that case for
attaching regular partitions, and it seems like it would be worth
testing the default-partition case as well.

- check_default_allows_bound() assumes that if
canSkipPartConstraintValidation() fails for the default partition, it
will also fail for every subpartition of the default partition.  That
is, once we commit to scanning one child partition, we're committed to
scanning them all.  In practice, that's probably not a huge
limitation, but if it's not too much code, we could keep the top-level
check but also check each partitioning individually as we reach it,
and skip the scan for any individual partitions for which the
constraint can be proven.  For example, suppose the top-level table is
list-partitioned with a partition for each of the most common values,
and then we range-partition the default partition.

- The changes to the regression test results in 0004 make the error
messages slightly worse.  The old message names the default partition,
whereas the new one does not.  Maybe that's worth avoiding.

Specific comments:

+ * Also, invalidate the parent's and a sibling default partition's relcache,
+ * so that the next rebuild will load the new partition's info into parent's
+ * partition descriptor and default partition constraints(which are dependent
+ * on other partition bounds) are built anew.

I find this a bit unclear, and it also repeats the comment further
down.  Maybe something like: Also, invalidate the parent's relcache
entry, so that the next rebuild will load he new partition's info into
its partition descriptor.  If there is a default partition, we must
invalidate its relcache entry as well.

+/*
+ * The default partition constraints depend upon the partition bounds of
+ * other partitions. Adding a new(or even removing existing) partition
+ * would invalidate the default partition constraints. Invalidate the
+ * default partition's relcache so that the constraints are built anew and
+ * any plans dependent on those constraints are invalidated as well.
+ */

Here, I'd write: The partition constraint for the default partition
depends on the partition bounds of every other partition, so we must
invalidate the relcache entry for that partition every time a
partition is added or removed.

+/*
+ * Default partition cannot be added if there already
+ * exists one.
+ */
+if (spec->is_default)
+{
+overlap = partition_bound_has_default(boundinfo);
+with = boundinfo->default_index;
+break;
+}

To support default partitioning for range, this is going to have to be
moved above the switch rather than done inside of it.  And there's
really no downside to putting it there.

+ * constraint, by *proving* that the existing constraints of the table
+ * *imply* the given constraints.  We include the table's check constraints and

Both the comma and the asterisks are unnecessary.

+ * Check whether all rows in the given table (scanRel) obey given partition

obey the given

I think the larger comment block could be tightened up a bit, like
this:  Check whether all rows in the given table obey the given
partition constraint; if so, it can be attached as a partition.  We do
this by scanning the table (or all of its leaf partitions) row by row,
except when the existing constraints are sufficient to prove that the
new partitioning constraint must already hold.

+/* Check if we can do away with having to scan the table being attached. */

If possible, skip the validation scan.

+ * Set up to have the table be scanned to validate the partition
+ * constraint If it's a partitioned table, we instead schedule its leaf
+ * partitions to be scanned.

I suggest: Prepare to scan the default partition (or, if it is itself
partitioned, all of its leaf 

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

While rebasing the current set of patches to the latest source, I realized
that it might be a good idea to split the default partitioning support
patch further
into two incremental patches, where the first patch for default partition
support would prevent addition of any new partition if there exists a
default
partition, and then an incremental patch which allows to create/attach a
new partition even if there exists a default partition provided the default
partition does not have any rows satisfying the bounds of the new partition
being added. This would be easier for review.

Here are the details of the patches in attached zip.
0001. refactoring existing ATExecAttachPartition  code so that it can be
used for
default partitioning as well
0002. support for default partition with the restriction of preventing
addition
of any new partition after default partition.
0003. extend default partitioning support to allow addition of new
partitions.
0004. extend default partitioning validation code to reuse the refactored
code
in patch 0001.

PFA

Regards,
Jeevan Ladhe

On Mon, Jun 12, 2017 at 11:49 AM, Jeevan Ladhe <
jeevan.la...@enterprisedb.com> wrote:

>
> On Mon, Jun 12, 2017 at 9:39 AM, Ashutosh Bapat <
> ashutosh.ba...@enterprisedb.com> wrote:
>
>> While the refactoring seems a reasonable way to re-use existing code,
>> that may change based on the discussion in [1]. Till then please keep
>> the refactoring patches separate from the main patch. In the final
>> version, I think the refactoring changes to ATAttachPartition and the
>> default partition support should be committed separately. So, please
>> provide three different patches. That also makes review easy.
>>
>
> Sure Ashutosh,
>
> PFA.
>


default_partition_splits_v21.tar.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Mon, Jun 12, 2017 at 9:39 AM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> While the refactoring seems a reasonable way to re-use existing code,
> that may change based on the discussion in [1]. Till then please keep
> the refactoring patches separate from the main patch. In the final
> version, I think the refactoring changes to ATAttachPartition and the
> default partition support should be committed separately. So, please
> provide three different patches. That also makes review easy.
>

Sure Ashutosh,

PFA.


default_partition_v20_patches.tar.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

While the refactoring seems a reasonable way to re-use existing code,
that may change based on the discussion in [1]. Till then please keep
the refactoring patches separate from the main patch. In the final
version, I think the refactoring changes to ATAttachPartition and the
default partition support should be committed separately. So, please
provide three different patches. That also makes review easy.

On Mon, Jun 12, 2017 at 8:29 AM, Jeevan Ladhe
 wrote:
> Hi Ashutosh,
>
> I tried to look into your refactoring code.
> When applied all 3 patches, I got some regression failures, I have fixed all
> of
> them now in attached patches, attached the regression.diffs.
>
> Moving further, I have also made following changes in attached patches:
>
> 1. 0001-Refactor-ATExecAttachPartition.patch
>
> + * There is a case in which we cannot rely on just the result of the
> + * proof
> This comment seems to also exist in current code, and I am not able to
> follow
> which case this refers to. But, IIUC, this comment is for the case where we
> are
> handling the 'key IS NOT NULL' part separately, and if that is the case it
> is
> not needed here in the prologue of the function.
>
> attachPartCanSkipValidation
> +static bool
> +ATCheckValidationSkippable(Relation scanRel, List *partConstraint,
> +  PartitionKey key)
> The function name ATCheckValidationSkippable does not sound very intuitive
> to me,
> and also I think prefix AT is something does not fit here as the function is
> not
> really directly related to alter table command, instead is an auxiliary
> function.
> How about changing it to "attachPartitionRequiresScan" or
> "canSkipPartConstraintValidation"
>
> +   List   *existConstraint = NIL;
> Needs to be moved to inside if block instead.
>
> +   boolskip_validate;
> Needs to be initialized to false, otherwise it can be returned without
> initialization when scanRel_constr is NULL.
>
> +   if (scanRel_constr != NULL)
> instead of this may be we can simply have:
> +   if (scanRel_constr == NULL)
> + return false;
> This can prevent further indentation.
>
> +static void
> +ATValidatePartitionConstraints(List **wqueue, Relation scanRel,
> +  List *partConstraint, Relation rel)
> What about just validatePartitionConstraints()
>
> +   boolskip_validate = false;
> +
> +   /* Check if we can do away with having to scan the table being attached.
> */
> +   skip_validate = ATCheckValidationSkippable(scanRel, partConstraint,
> key);
>
> First assignment is unnecessary here.
>
> Instead of:
> /* Check if we can do away with having to scan the table being attached. */
> skip_validate = ATCheckValidationSkippable(scanRel, partConstraint, key);
>
> /* It's safe to skip the validation scan after all */
> if (skip_validate)
> ereport(INFO,
> (errmsg("partition constraint for table \"%s\" is implied by existing
> constraints",
> RelationGetRelationName(scanRel;
>
> Following change can prevent further indentation:
> if (ATCheckValidationSkippable(scanRel, partConstraint, key))
> {
> ereport(INFO,
> (errmsg("partition constraint for table \"%s\" is implied by existing
> constraints",
> RelationGetRelationName(scanRel;
> return;
> }
> This way variable skip_validate will not be needed.
>
> Apart from this, I see that the patch will need change depending on how the
> fix
> for validating partition constraints in case of embedded NOT-NULL[1] shapes
> up.
>
> 2. 0003-Refactor-default-partitioning-patch-to-re-used-code.patch
>
> + * In case the new partition bound being checked itself is a DEFAULT
> + * bound, this check shouldn't be triggered as there won't already exists
> + * the default partition in such a case.
> I think above comment in DefineRelation() is not applicable, as
> check_default_allows_bound() is called unconditional, and the check for
> existence
> of default partition is now done inside the check_default_allows_bound()
> function.
>
>   * This function checks if there exists a row in the default partition that
>   * fits in the new partition and throws an error if it finds one.
>   */
> Above comment for check_default_allows_bound() needs a change now, may be
> something like this:
>   * This function checks if a default partition already exists and if it
> does
>   * it checks if there exists a row in the default partition that fits in
> the
>   * new partition and throws an error if it finds one.
>   */
>
> List   *new_part_constraints = NIL;
> List   *def_part_constraints = NIL;
> I think above initialization is not needed, as the further assignments are
> unconditional.
>
> + if (OidIsValid(default_oid))
> + {
> + Relation default_rel = heap_open(default_oid, AccessExclusiveLock);
> We already have taken a lock on default and here we should be using a NoLock
> instead.
>
> + def_part_constraints =
> get_default_part_validation_constraint(new_part_constraints);
> exceeds 80 columns.
>
> + 

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Ashutosh,

I tried to look into your refactoring code.
When applied all 3 patches, I got some regression failures, I have fixed
all of
them now in attached patches, attached the regression.diffs.

Moving further, I have also made following changes in attached patches:

*1. 0001-Refactor-ATExecAttachPartition.patch*

+ * There is a case in which we cannot rely on just the result of the
+ * proof
This comment seems to also exist in current code, and I am not able to
follow
which case this refers to. But, IIUC, this comment is for the case where we
are
handling the 'key IS NOT NULL' part separately, and if that is the case it
is
not needed here in the prologue of the function.

attachPartCanSkipValidation
+static bool
+ATCheckValidationSkippable(Relation scanRel, List *partConstraint,
+  PartitionKey key)
The function name ATCheckValidationSkippable does not sound very intuitive
to me,
and also I think prefix AT is something does not fit here as the function
is not
really directly related to alter table command, instead is an auxiliary
function.
How about changing it to "attachPartitionRequiresScan" or
"canSkipPartConstraintValidation"

+   List   *existConstraint = NIL;
Needs to be moved to inside if block instead.

+   boolskip_validate;
Needs to be initialized to false, otherwise it can be returned without
initialization when scanRel_constr is NULL.

+   if (scanRel_constr != NULL)
instead of this may be we can simply have:
+   if (scanRel_constr == NULL)
+ return false;
This can prevent further indentation.

+static void
+ATValidatePartitionConstraints(List **wqueue, Relation scanRel,
+  List *partConstraint, Relation rel)
What about just validatePartitionConstraints()

+   boolskip_validate = false;
+
+   /* Check if we can do away with having to scan the table being
attached. */
+   skip_validate = ATCheckValidationSkippable(scanRel, partConstraint,
key);

First assignment is unnecessary here.

Instead of:
/* Check if we can do away with having to scan the table being attached. */
skip_validate = ATCheckValidationSkippable(scanRel, partConstraint, key);

/* It's safe to skip the validation scan after all */
if (skip_validate)
ereport(INFO,
(errmsg("partition constraint for table \"%s\" is implied by existing
constraints",
RelationGetRelationName(scanRel;

Following change can prevent further indentation:
if (ATCheckValidationSkippable(scanRel, partConstraint, key))
{
ereport(INFO,
(errmsg("partition constraint for table \"%s\" is implied by existing
constraints",
RelationGetRelationName(scanRel;
return;
}
This way variable skip_validate will not be needed.

Apart from this, I see that the patch will need change depending on how the
fix
for validating partition constraints in case of embedded NOT-NULL[1] shapes
up.

*2. 0003-Refactor-default-partitioning-patch-to-re-used-code.patch*

+ * In case the new partition bound being checked itself is a DEFAULT
+ * bound, this check shouldn't be triggered as there won't already exists
+ * the default partition in such a case.
I think above comment in DefineRelation() is not applicable, as
check_default_allows_bound() is called unconditional, and the check for
existence
of default partition is now done inside the check_default_allows_bound()
function.

  * This function checks if there exists a row in the default partition that
  * fits in the new partition and throws an error if it finds one.
  */
Above comment for check_default_allows_bound() needs a change now, may be
something like this:
  * This function checks if a default partition already exists and if it
does
  * it checks if there exists a row in the default partition that fits in
the
  * new partition and throws an error if it finds one.
  */

List   *new_part_constraints = NIL;
List   *def_part_constraints = NIL;
I think above initialization is not needed, as the further assignments are
unconditional.

+ if (OidIsValid(default_oid))
+ {
+ Relation default_rel = heap_open(default_oid, AccessExclusiveLock);
We already have taken a lock on default and here we should be using a NoLock
instead.

+ def_part_constraints =
get_default_part_validation_constraint(new_part_constraints);
exceeds 80 columns.

+ defPartConstraint =
get_default_part_validation_constraint(partBoundConstraint);
similarly, needs indentation.

+
+List *
+get_default_part_validation_constraint(List *new_part_constraints)
+{
Needs some comment. What about:
/*
 * get_default_part_validation_constraint
 *
 * Given partition constraints, this function returns *would be* default
 * partition constraint.
 */

Apart from this, I tried to address the differences in error shown in case
of
attache and create partition when rows in default partition would violate
the
updated constraints, basically I have taken a flag in AlteredTableInfo to
indicate if the relation being scanned is a default partition or a child of
default partition(which I dint like much, but I don't 

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Wed, Jun 7, 2017 at 5:47 AM, Ashutosh Bapat
 wrote:
> On Sat, Jun 3, 2017 at 2:11 AM, Robert Haas  wrote:
>>
>> + errmsg("default partition contains row(s)
>> that would overlap with partition being created")));
>>
>> It doesn't really sound right to talk about rows overlapping with a
>> partition.  Partitions can overlap with each other, but not rows.
>> Also, it's not really project style to use ambiguously plural forms
>> like "row(s)" in error messages.  Maybe something like:
>>
>> new partition constraint for default partition \"%s\" would be
>> violated by some row
>
> Partition constraint is implementation detail here. We enforce
> partition bounds through constraints and we call such constraints as
> partition constraints. But a user may not necessarily understand this
> term or may interpret it different. Adding "new" adds to the confusion
> as the default partition is not new.

I see your point.  We could say "updated partition constraint" instead
of "new partition constraint" to address that to some degree.

> My suggestion in an earlier mail
> was ""default partition contains rows that conflict with the partition
> bounds of "part_xyz"", with a note that we should use a better word
> than "conflict". So, Jeevan seems to have used overlap, which again is
> not correct. How about "default partition contains row/s which would
> fit the partition "part_xyz" being created or attached." with a hint
> to move those rows to the new partition's table in case of attach. I
> don't think hint would be so straight forward i.e. to create the table
> with SELECT INTO and then ATTACH.

The problem is that none of these actually sound very good.  Neither
conflict nor overlap nor fit actually express the underlying idea very
clearly, at least IMHO.  I'm not opposed to using some wording along
these lines if we can think of a clear way to word it, but I think my
wording is better than using some unclear word for this concept.  I
can't immediately think of a way to adjust your wording so that it
seems completely clear.

> Also, the error code ERRCODE_CHECK_VIOLATION, which is an "integrity
> constraint violation" code, seems misleading. We aren't violating any
> integrity here. In fact I am not able to understand, how could adding
> an object violate integrity constraint. The nearest errorcode seems to
> be ERRCODE_INVALID_OBJECT_DEFINITION, which is also used for
> partitions with overlapping bounds.

I think that calling a constraint failure a check violation is not too
much of a stretch, even if it's technically a partition constraint
rather than a CHECK constraint.  However, your proposal also seems
reasonable.  I'm happy to go with whatever most people like best.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Wed, Jun 7, 2017 at 1:59 AM, amul sul  wrote:
> But Ashutosh's suggestion make sense, we might have constraints other
> than that partitioning constraint on default partition.  If those
> constraints refutes the new partition's constraints, we should skip
> the scan.

Right.  If the user adds a constraint to the default partition that is
identical to the new partition constraint, that should cause the scan
to be skipped.

Ideally, we could do even better.  For example, if the user is
creating a new partition FOR VALUES IN (7), and the default partition
has CHECK (key != 7), we could perhaps deduce that the combination of
the existing partition constraint (which must certainly hold) and the
additional CHECK constraint (which must also hold, at least assuming
it's not marked NOT VALID) are sufficient to prove the new check
constraint.  But I'm not sure whether predicate_refuted_by() is smart
enough to figure that out.  However, it should definitely be smart
enough to figure out that if somebody's added the new partitioning
constraint as a CHECK constraint on the default partition, we don't
need to scan it.

The reason somebody might want to do that, just to be clear, is that
they could do this in multiple steps: first, add the new CHECK
constraint as NOT VALID.  Then VALIDATE CONSTRAINT.  Then add the new
non-default partition.  This would result in holding an exclusive lock
for a lesser period of time than if they did it all together as one
operation.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Thanks Ashutosh,

On Thu, Jun 8, 2017 at 4:04 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> On Thu, Jun 8, 2017 at 2:54 PM, Ashutosh Bapat
>  wrote:
> > On Wed, Jun 7, 2017 at 2:08 AM, Jeevan Ladhe
> >  wrote:
> >
> >>
> >>>
> >>> The code in check_default_allows_bound() to check whether the default
> >>> partition
> >>> has any rows that would fit new partition looks quite similar to the
> code
> >>> in
> >>> ATExecAttachPartition() checking whether all rows in the table being
> >>> attached
> >>> as a partition fit the partition bounds. One thing that
> >>> check_default_allows_bound() misses is, if there's already a
> constraint on
> >>> the
> >>> default partition refutes the partition constraint on the new
> partition,
> >>> we can
> >>> skip the scan of the default partition since it can not have rows that
> >>> would
> >>> fit the new partition. ATExecAttachPartition() has code to deal with a
> >>> similar
> >>> case i.e. the table being attached has a constraint which implies the
> >>> partition
> >>> constraint. There may be more cases which check_default_allows_bound()
> >>> does not
> >>> handle but ATExecAttachPartition() handles. So, I am wondering whether
> >>> it's
> >>> better to somehow take out the common code into a function and use it.
> We
> >>> will
> >>> have to deal with a difference through. The first one would throw an
> error
> >>> when
> >>> finding a row that satisfies partition constraints whereas the second
> one
> >>> would
> >>> throw an error when it doesn't find such a row. But this difference
> can be
> >>> handled through a flag or by negating the constraint. This would also
> take
> >>> care
> >>> of Amit Langote's complaint about foreign partitions. There's also
> another
> >>> difference that the ATExecAttachPartition() queues the table for scan
> and
> >>> the
> >>> actual scan takes place in ATRewriteTable(), but there is not such
> queue
> >>> while
> >>> creating a table as a partition. But we should check if we can reuse
> the
> >>> code to
> >>> scan the heap for checking a constraint.
> >>>
> >>> In case of ATTACH PARTITION, probably we should schedule scan of
> default
> >>> partition in the alter table's work queue like what
> >>> ATExecAttachPartition() is
> >>> doing for the table being attached. That would fit in the way alter
> table
> >>> works.
> >>
> >
> > I tried refactoring existing code so that it can be used for default
> > partitioning as well. The code to validate the partition constraints
> > against the table being attached in ATExecAttachPartition() is
> > extracted out into a set of functions. For default partition we reuse
> > those functions to check whether it contains any row that would fit
> > the partition being attached. While creating a new partition, the
> > function to skip validation is reused but the scan portion is
> > duplicated from ATRewriteTable since we are not in ALTER TABLE
> > context. The names of the functions, their declaration will require
> > some thought.
> >
> > There's one test failing because for ATTACH partition the error comes
> > from ATRewriteTable instead of check_default_allows_bounds(). May be
> > we want to use same message in both places or some make ATRewriteTable
> > give a different message while validating default partition.
> >
> > Please review the patch and let me know if the changes look good.
>
> From the discussion on thread [1], that having a NOT NULL constraint
> embedded within an expression may cause a scan to be skipped when it
> shouldn't be. For default partitions such a case may arise. If an
> existing partition accepts NULL and we try to attach a default
> partition, it would get a NOT NULL partition constraint but it will be
> buried within an expression like !(key = any(array[1, 2, 3]) OR key is
> null) where the existing partition/s accept values 1, 2, 3 and null.
> We need to check whether the refactored code handles this case
> correctly. v19 patch does not have this problem since it doesn't try
> to skip the scan based on the constraints of the table being attached.
> Please try following cases 1. a default partition accepting nulls
> exists and we attach a partition to accept NULL values 2. a NULL
> accepting partition exists and we try to attach a table as default
> partition. In both the cases default partition should be checked for
> rows with NULL partition keys. In both the cases, if the default
> partition table has a NOT NULL constraint we should be able to skip
> the scan and should scan the table when such a constraint does not
> exist.
>

I will review your refactoring patch as well test above cases.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Thu, Jun 8, 2017 at 2:54 PM, Ashutosh Bapat
 wrote:
> On Wed, Jun 7, 2017 at 2:08 AM, Jeevan Ladhe
>  wrote:
>
>>
>>>
>>> The code in check_default_allows_bound() to check whether the default
>>> partition
>>> has any rows that would fit new partition looks quite similar to the code
>>> in
>>> ATExecAttachPartition() checking whether all rows in the table being
>>> attached
>>> as a partition fit the partition bounds. One thing that
>>> check_default_allows_bound() misses is, if there's already a constraint on
>>> the
>>> default partition refutes the partition constraint on the new partition,
>>> we can
>>> skip the scan of the default partition since it can not have rows that
>>> would
>>> fit the new partition. ATExecAttachPartition() has code to deal with a
>>> similar
>>> case i.e. the table being attached has a constraint which implies the
>>> partition
>>> constraint. There may be more cases which check_default_allows_bound()
>>> does not
>>> handle but ATExecAttachPartition() handles. So, I am wondering whether
>>> it's
>>> better to somehow take out the common code into a function and use it. We
>>> will
>>> have to deal with a difference through. The first one would throw an error
>>> when
>>> finding a row that satisfies partition constraints whereas the second one
>>> would
>>> throw an error when it doesn't find such a row. But this difference can be
>>> handled through a flag or by negating the constraint. This would also take
>>> care
>>> of Amit Langote's complaint about foreign partitions. There's also another
>>> difference that the ATExecAttachPartition() queues the table for scan and
>>> the
>>> actual scan takes place in ATRewriteTable(), but there is not such queue
>>> while
>>> creating a table as a partition. But we should check if we can reuse the
>>> code to
>>> scan the heap for checking a constraint.
>>>
>>> In case of ATTACH PARTITION, probably we should schedule scan of default
>>> partition in the alter table's work queue like what
>>> ATExecAttachPartition() is
>>> doing for the table being attached. That would fit in the way alter table
>>> works.
>>
>
> I tried refactoring existing code so that it can be used for default
> partitioning as well. The code to validate the partition constraints
> against the table being attached in ATExecAttachPartition() is
> extracted out into a set of functions. For default partition we reuse
> those functions to check whether it contains any row that would fit
> the partition being attached. While creating a new partition, the
> function to skip validation is reused but the scan portion is
> duplicated from ATRewriteTable since we are not in ALTER TABLE
> context. The names of the functions, their declaration will require
> some thought.
>
> There's one test failing because for ATTACH partition the error comes
> from ATRewriteTable instead of check_default_allows_bounds(). May be
> we want to use same message in both places or some make ATRewriteTable
> give a different message while validating default partition.
>
> Please review the patch and let me know if the changes look good.

>From the discussion on thread [1], that having a NOT NULL constraint
embedded within an expression may cause a scan to be skipped when it
shouldn't be. For default partitions such a case may arise. If an
existing partition accepts NULL and we try to attach a default
partition, it would get a NOT NULL partition constraint but it will be
buried within an expression like !(key = any(array[1, 2, 3]) OR key is
null) where the existing partition/s accept values 1, 2, 3 and null.
We need to check whether the refactored code handles this case
correctly. v19 patch does not have this problem since it doesn't try
to skip the scan based on the constraints of the table being attached.
Please try following cases 1. a default partition accepting nulls
exists and we attach a partition to accept NULL values 2. a NULL
accepting partition exists and we try to attach a table as default
partition. In both the cases default partition should be checked for
rows with NULL partition keys. In both the cases, if the default
partition table has a NOT NULL constraint we should be able to skip
the scan and should scan the table when such a constraint does not
exist.

[1] 
http://www.postgresql-archive.org/A-bug-in-mapping-attributes-in-ATExecAttachPartition-td5965298.html

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Wed, Jun 7, 2017 at 2:08 AM, Jeevan Ladhe
 wrote:

>
>>
>> The code in check_default_allows_bound() to check whether the default
>> partition
>> has any rows that would fit new partition looks quite similar to the code
>> in
>> ATExecAttachPartition() checking whether all rows in the table being
>> attached
>> as a partition fit the partition bounds. One thing that
>> check_default_allows_bound() misses is, if there's already a constraint on
>> the
>> default partition refutes the partition constraint on the new partition,
>> we can
>> skip the scan of the default partition since it can not have rows that
>> would
>> fit the new partition. ATExecAttachPartition() has code to deal with a
>> similar
>> case i.e. the table being attached has a constraint which implies the
>> partition
>> constraint. There may be more cases which check_default_allows_bound()
>> does not
>> handle but ATExecAttachPartition() handles. So, I am wondering whether
>> it's
>> better to somehow take out the common code into a function and use it. We
>> will
>> have to deal with a difference through. The first one would throw an error
>> when
>> finding a row that satisfies partition constraints whereas the second one
>> would
>> throw an error when it doesn't find such a row. But this difference can be
>> handled through a flag or by negating the constraint. This would also take
>> care
>> of Amit Langote's complaint about foreign partitions. There's also another
>> difference that the ATExecAttachPartition() queues the table for scan and
>> the
>> actual scan takes place in ATRewriteTable(), but there is not such queue
>> while
>> creating a table as a partition. But we should check if we can reuse the
>> code to
>> scan the heap for checking a constraint.
>>
>> In case of ATTACH PARTITION, probably we should schedule scan of default
>> partition in the alter table's work queue like what
>> ATExecAttachPartition() is
>> doing for the table being attached. That would fit in the way alter table
>> works.
>

I tried refactoring existing code so that it can be used for default
partitioning as well. The code to validate the partition constraints
against the table being attached in ATExecAttachPartition() is
extracted out into a set of functions. For default partition we reuse
those functions to check whether it contains any row that would fit
the partition being attached. While creating a new partition, the
function to skip validation is reused but the scan portion is
duplicated from ATRewriteTable since we are not in ALTER TABLE
context. The names of the functions, their declaration will require
some thought.

There's one test failing because for ATTACH partition the error comes
from ATRewriteTable instead of check_default_allows_bounds(). May be
we want to use same message in both places or some make ATRewriteTable
give a different message while validating default partition.

Please review the patch and let me know if the changes look good.


default_partition_refactor_v20.tar.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Sat, Jun 3, 2017 at 2:11 AM, Robert Haas  wrote:
>
> + errmsg("default partition contains row(s)
> that would overlap with partition being created")));
>
> It doesn't really sound right to talk about rows overlapping with a
> partition.  Partitions can overlap with each other, but not rows.
> Also, it's not really project style to use ambiguously plural forms
> like "row(s)" in error messages.  Maybe something like:
>
> new partition constraint for default partition \"%s\" would be
> violated by some row
>

Partition constraint is implementation detail here. We enforce
partition bounds through constraints and we call such constraints as
partition constraints. But a user may not necessarily understand this
term or may interpret it different. Adding "new" adds to the confusion
as the default partition is not new. My suggestion in an earlier mail
was ""default partition contains rows that conflict with the partition
bounds of "part_xyz"", with a note that we should use a better word
than "conflict". So, Jeevan seems to have used overlap, which again is
not correct. How about "default partition contains row/s which would
fit the partition "part_xyz" being created or attached." with a hint
to move those rows to the new partition's table in case of attach. I
don't think hint would be so straight forward i.e. to create the table
with SELECT INTO and then ATTACH.

What do you think?

Also, the error code ERRCODE_CHECK_VIOLATION, which is an "integrity
constraint violation" code, seems misleading. We aren't violating any
integrity here. In fact I am not able to understand, how could adding
an object violate integrity constraint. The nearest errorcode seems to
be ERRCODE_INVALID_OBJECT_DEFINITION, which is also used for
partitions with overlapping bounds.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Wed, Jun 7, 2017 at 2:08 AM, Jeevan Ladhe
 wrote:
>>
>> This also means that we have to test PREPARED statements involving
>> default partition. Any addition/deletion/attach/detach of other partition
>> should invalidate those cached statements.
>
>
> Will add this in next version of patch.

My earlier statement requires a clarification. By "PREPARED statements
involving default partition." I mean PREPAREd statements with direct
access to the default partition, without going through the partitioned
table.

>
>>
>> The code in check_default_allows_bound() to check whether the default
>> partition
>> has any rows that would fit new partition looks quite similar to the code
>> in
>> ATExecAttachPartition() checking whether all rows in the table being
>> attached
>> as a partition fit the partition bounds. One thing that
>> check_default_allows_bound() misses is, if there's already a constraint on
>> the
>> default partition refutes the partition constraint on the new partition,
>> we can
>> skip the scan of the default partition since it can not have rows that
>> would
>> fit the new partition. ATExecAttachPartition() has code to deal with a
>> similar
>> case i.e. the table being attached has a constraint which implies the
>> partition
>> constraint. There may be more cases which check_default_allows_bound()
>> does not
>> handle but ATExecAttachPartition() handles. So, I am wondering whether
>> it's
>> better to somehow take out the common code into a function and use it. We
>> will
>> have to deal with a difference through. The first one would throw an error
>> when
>> finding a row that satisfies partition constraints whereas the second one
>> would
>> throw an error when it doesn't find such a row. But this difference can be
>> handled through a flag or by negating the constraint. This would also take
>> care
>> of Amit Langote's complaint about foreign partitions. There's also another
>> difference that the ATExecAttachPartition() queues the table for scan and
>> the
>> actual scan takes place in ATRewriteTable(), but there is not such queue
>> while
>> creating a table as a partition. But we should check if we can reuse the
>> code to
>> scan the heap for checking a constraint.
>>
>> In case of ATTACH PARTITION, probably we should schedule scan of default
>> partition in the alter table's work queue like what
>> ATExecAttachPartition() is
>> doing for the table being attached. That would fit in the way alter table
>> works.
>
>
> I am still working on this.
> But, about your comment here:
> "if there's already a constraint on the default partition refutes the
> partition
> constraint on the new partition, we can skip the scan":
> I am so far not able to imagine such a case, since default partition
> constraint
> can be imagined something like "minus infinity to positive infinity with
> some finite set elimination", and any new non-default partition being added
> would simply be a set of finite values(at-least in case of list, but I think
> range
> should not also differ here). Hence one cannot imply the other here.
> Possibly,
> I might be missing something that you had visioned when you raised the flag,
> please correct me if I am missing something.

I am hoping that this has been clarified in other mails in this thread
between you and Amul.

>
>>
>>  /* Generate the main expression, i.e., keyCol = ANY (arr) */
>>  opexpr = make_partition_op_expr(key, 0, BTEqualStrategyNumber,
>> -keyCol, (Expr *) arr);
>> +keyCol, (Expr *) arr,
>> spec->is_default);
>>  /* Build leftop = ANY (rightop) */
>>  saopexpr = makeNode(ScalarArrayOpExpr);
>> The comments in both the places need correction, as for default partition
>> the
>> expression will be keyCol <> ALL(arr).
>
>
> Done.

Please note that this changes, if you construct the constraint as
!(keycol = ANY[]).

>
>> We have RelationGetPartitionDesc() for
>> that. Probably we should also add Asserts to check that every pointer in
>> the
>> long pointer chain is Non-null.
>
>
> I am sorry, but I did not understand which chain you are trying to point
> here.

The chain of pointers: a->b->c->d is a chain of pointers.

>
>>
>> @@ -2044,7 +2044,7 @@ psql_completion(const char *text, int start, int
>> end)
>>  COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, "");
>>  /* Limited completion support for partition bound specification */
>>  else if (TailMatches3("ATTACH", "PARTITION", MatchAny))
>> -COMPLETE_WITH_CONST("FOR VALUES");
>> +COMPLETE_WITH_LIST2("FOR VALUES", "DEFAULT");
>>  else if (TailMatches2("FOR", "VALUES"))
>>  COMPLETE_WITH_LIST2("FROM (", "IN (");
>>
>> @@ -2483,7 +2483,7 @@ psql_completion(const char *text, int start, int
>> end)
>>  COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_partitioned_tables,
>> "");
>>  /* Limited completion support for 

Re: [HACKERS] Adding support for Default partition in partitioning

[amul sul]

On Wed, Jun 7, 2017 at 10:30 AM, Jeevan Ladhe
 wrote:
>
>
>> IIUC, default partition constraints is simply NOT IN (> other sibling partitions>).
>> If constraint on the default partition refutes the new partition's
>> constraints that means we have overlapping partition, and perhaps
>> error.
>
>
> You are correct Amul, but this error will be thrown before we try to
> check for the default partition data. So, in such cases I think we really
> do not need to have logic to check if default partition refutes the new
> partition contraints.
>

But Ashutosh's suggestion make sense, we might have constraints other
than that partitioning constraint on default partition.  If those
constraints refutes the new partition's constraints, we should skip
the scan.

Regards,
Amul


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

IIUC, default partition constraints is simply NOT IN ( other sibling partitions>).
> If constraint on the default partition refutes the new partition's
> constraints that means we have overlapping partition, and perhaps
> error.
>

You are correct Amul, but this error will be thrown before we try to
check for the default partition data. So, in such cases I think we really
do not need to have logic to check if default partition refutes the new
partition contraints.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[amul sul]

On Wed, Jun 7, 2017 at 2:08 AM, Jeevan Ladhe
 wrote:
[...]
>>
>> The code in check_default_allows_bound() to check whether the default
>> partition
>> has any rows that would fit new partition looks quite similar to the code
>> in
>> ATExecAttachPartition() checking whether all rows in the table being
>> attached
>> as a partition fit the partition bounds. One thing that
>> check_default_allows_bound() misses is, if there's already a constraint on
>> the
>> default partition refutes the partition constraint on the new partition,
>> we can
>> skip the scan of the default partition since it can not have rows that
>> would
>> fit the new partition. ATExecAttachPartition() has code to deal with a
>> similar
>> case i.e. the table being attached has a constraint which implies the
>> partition
>> constraint. There may be more cases which check_default_allows_bound()
>> does not
>> handle but ATExecAttachPartition() handles. So, I am wondering whether
>> it's
>> better to somehow take out the common code into a function and use it. We
>> will
>> have to deal with a difference through. The first one would throw an error
>> when
>> finding a row that satisfies partition constraints whereas the second one
>> would
>> throw an error when it doesn't find such a row. But this difference can be
>> handled through a flag or by negating the constraint. This would also take
>> care
>> of Amit Langote's complaint about foreign partitions. There's also another
>> difference that the ATExecAttachPartition() queues the table for scan and
>> the
>> actual scan takes place in ATRewriteTable(), but there is not such queue
>> while
>> creating a table as a partition. But we should check if we can reuse the
>> code to
>> scan the heap for checking a constraint.
>>
>> In case of ATTACH PARTITION, probably we should schedule scan of default
>> partition in the alter table's work queue like what
>> ATExecAttachPartition() is
>> doing for the table being attached. That would fit in the way alter table
>> works.
>
>
> I am still working on this.
> But, about your comment here:
> "if there's already a constraint on the default partition refutes the
> partition
> constraint on the new partition, we can skip the scan":
> I am so far not able to imagine such a case, since default partition
> constraint
> can be imagined something like "minus infinity to positive infinity with
> some finite set elimination", and any new non-default partition being added
> would simply be a set of finite values(at-least in case of list, but I think
> range
> should not also differ here). Hence one cannot imply the other here.
> Possibly,
> I might be missing something that you had visioned when you raised the flag,
> please correct me if I am missing something.
>

IIUC, default partition constraints is simply NOT IN ().
If constraint on the default partition refutes the new partition's
constraints that means we have overlapping partition, and perhaps
error.


Regards,
Amul


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Ashutosh,

Thanks for the detailed review.

Also, please find my feedback on your comments in-lined, I also addressed
the comments given by Robert in attached patch:

On Sat, Jun 3, 2017 at 5:13 PM, Ashutosh Bapat  wrote:

> Here's some detailed review of the code.
>
> @@ -1883,6 +1883,15 @@ heap_drop_with_catalog(Oid relid)
>  if (OidIsValid(parentOid))
>  {
>  /*
> + * Default partition constraints are constructed run-time from the
> + * constraints of its siblings(basically by negating them), so any
> + * change in the siblings needs to rebuild the constraints of the
> + * default partition. So, invalidate the sibling default
> partition's
> + * relcache.
> + */
> +InvalidateDefaultPartitionRelcache(parentOid);
> +
> Do we need a lock on the default partition for doing this? A query might be
> scanning the default partition directly and we will invalidate the relcache
> underneath it. What if two partitions are being dropped simultaneously and
> change default constraints simultaneously. Probably the lock on the parent
> helps there, but need to check it. What if the default partition cache is
> invalidated because partition gets added/dropped to the default partition
> itself. If we need a lock on the default partition, we will need to
> check the order in which we should be obtaining the locks so as to avoid
> deadlocks.


Done. I have taken a lock on default partition after acquiring a lock on
parent
relation where ever applicable.


> This also means that we have to test PREPARED statements involving
> default partition. Any addition/deletion/attach/detach of other partition
> should invalidate those cached statements.
>

Will add this in next version of patch.


> +if (partition_bound_has_default(boundinfo))
> +{
> +overlap = true;
> +with = boundinfo->default_index;
> +}
> You could possibly rewrite this as
> overlap = partition_bound_has_default(boundinfo);
> with = boundinfo->default_index;
> that would save one indentation and a conditional jump.


Done


> +if (partdesc->nparts > 0 && partition_bound_has_default(boundinfo))
> +check_default_allows_bound(parent, spec);
> If the table has a default partition, nparts > 0, nparts > 0 check looks
> redundant. The comments above should also explain that this check doesn't
> trigger when a default partition is added since we don't expect an existing
> default partition in such a case.
>

The check nparts > 0, is needed to make sure that the boundinfo is non-null,
i.e. to confirm that there exists at least one partition so that
partition_bound_has_default() does not result in segmentation fault.
I have changed the condition as below to make it more intuitive:
if (boundinfo && partition_bound_has_default(boundinfo))
Also, I have updated the comment.


> + * Checks if there exists any row in the default partition that passes the
> + * check for constraints of new partition, if any reports an error.
> grammar two conflicting ifs in the same statement. You may want to rephrase
> this as "This function checks if there exists a row in the default
> partition that fits in the new
> partition and throws an error if it finds one."
>

Done


> +if (new_spec->strategy != PARTITION_STRATEGY_LIST)
> +return;
> This should probably be an Assert. When default range partition is
> supported
> this function would silently return, meaning there is no row in the default
> partition which fits the new partition. We don't want that behavior.
>

Agreed, changed.


> The code in check_default_allows_bound() to check whether the default
> partition
> has any rows that would fit new partition looks quite similar to the code
> in
> ATExecAttachPartition() checking whether all rows in the table being
> attached
> as a partition fit the partition bounds. One thing that
> check_default_allows_bound() misses is, if there's already a constraint on
> the
> default partition refutes the partition constraint on the new partition,
> we can
> skip the scan of the default partition since it can not have rows that
> would
> fit the new partition. ATExecAttachPartition() has code to deal with a
> similar
> case i.e. the table being attached has a constraint which implies the
> partition
> constraint. There may be more cases which check_default_allows_bound()
> does not
> handle but ATExecAttachPartition() handles. So, I am wondering whether it's
> better to somehow take out the common code into a function and use it. We
> will
> have to deal with a difference through. The first one would throw an error
> when
> finding a row that satisfies partition constraints whereas the second one
> would
> throw an error when it doesn't find such a row. But this difference can be
> handled through a flag or by negating the constraint. This would 

Re: [HACKERS] Adding support for Default partition in partitioning

[Beena Emerson]

On Mon, Jun 5, 2017 at 12:14 AM, Jeevan Ladhe
 wrote:
>
>
>>
>> What is the reason the new patch does not mention of violating rows
>> when a new partition overlaps with default?
>> Is it because more than one row could be violating the condition?
>
>
> This is because, for reporting the violating error, I had to function
> ExecBuildSlotValueDescription() public. Per Amit's comment I have
> removed this change and let the overlapping error without row contains.
> I think this is analogus to other functions that are throwing violation
> error
> but are not local to execMain.c.
>

ok thanks.


-- 

Beena Emerson

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

> What is the reason the new patch does not mention of violating rows
> when a new partition overlaps with default?
> Is it because more than one row could be violating the condition?
>

This is because, for reporting the violating error, I had to function
ExecBuildSlotValueDescription() public. Per Amit's comment I have
removed this change and let the overlapping error without row contains.
I think this is analogus to other functions that are throwing violation
error
but are not local to execMain.c.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Beena Emerson]

Hello,

On Fri, Jun 2, 2017 at 1:05 AM, Jeevan Ladhe
 wrote:
> Hi,
>
> I have addressed Ashutosh's and Amit's comments in the attached patch.
>
> Please let me know if I have missed anything and any further comments.
>
> PFA.
>
> Regards,
> Jeevan Ladhe
>

What is the reason the new patch does not mention of violating rows
when a new partition overlaps with default?
Is it because more than one row could be violating the condition?

-- 

Beena Emerson

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Robert,

Thanks for your comments:


> If DETACH PARTITION and DROP PARTITION require this, why not ATTACH
> PARTITION and CREATE TABLE .. PARTITION OF?
>
>
For CREATE and ATTACH parition the invalidation of default relation is taken
care by the following clean-up part in check_default_allows_bound():

+ ResetExprContext(econtext);
+ CHECK_FOR_INTERRUPTS();
+ }
+
+ CacheInvalidateRelcache(part_rel);
+ MemoryContextSwitchTo(oldCxt);

However, post your comment I carefully looked in the code I wrote here, and
I
see that this still explicitly needs cache invalidation in ATTACH and CREATE
command, because the above invalidation call will not happen in case the
default partition is further partitioned. Plus, I think the call to
CacheInvalidateRelcache() in check_default_allows_bound() can be completely
removed.

This code however will be rearranged, as I plan to address Ashutosh's one
of the
comment to write a function for common code of ATExecAttachPartition() and
check_default_allows_bound().

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

Here's some detailed review of the code.

@@ -1883,6 +1883,15 @@ heap_drop_with_catalog(Oid relid)
 if (OidIsValid(parentOid))
 {
 /*
+ * Default partition constraints are constructed run-time from the
+ * constraints of its siblings(basically by negating them), so any
+ * change in the siblings needs to rebuild the constraints of the
+ * default partition. So, invalidate the sibling default partition's
+ * relcache.
+ */
+InvalidateDefaultPartitionRelcache(parentOid);
+
Do we need a lock on the default partition for doing this? A query might be
scanning the default partition directly and we will invalidate the relcache
underneath it. What if two partitions are being dropped simultaneously and
change default constraints simultaneously. Probably the lock on the parent
helps there, but need to check it. What if the default partition cache is
invalidated because partition gets added/dropped to the default partition
itself. If we need a lock on the default partition, we will need to
check the order in which we should be obtaining the locks so as to avoid
deadlocks. This also means that we have to test PREPARED statements involving
default partition. Any addition/deletion/attach/detach of other partition
should invalidate those cached statements.

+if (partition_bound_has_default(boundinfo))
+{
+overlap = true;
+with = boundinfo->default_index;
+}
You could possibly rewrite this as
overlap = partition_bound_has_default(boundinfo);
with = boundinfo->default_index;
that would save one indentation and a conditional jump.

+if (partdesc->nparts > 0 && partition_bound_has_default(boundinfo))
+check_default_allows_bound(parent, spec);
If the table has a default partition, nparts > 0, nparts > 0 check looks
redundant. The comments above should also explain that this check doesn't
trigger when a default partition is added since we don't expect an existing
default partition in such a case.

+ * Checks if there exists any row in the default partition that passes the
+ * check for constraints of new partition, if any reports an error.
grammar two conflicting ifs in the same statement. You may want to rephrase
this as "This function checks if there exists a row in the default
partition that fits in the new
partition and throws an error if it finds one."

+if (new_spec->strategy != PARTITION_STRATEGY_LIST)
+return;
This should probably be an Assert. When default range partition is supported
this function would silently return, meaning there is no row in the default
partition which fits the new partition. We don't want that behavior.

The code in check_default_allows_bound() to check whether the default partition
has any rows that would fit new partition looks quite similar to the code in
ATExecAttachPartition() checking whether all rows in the table being attached
as a partition fit the partition bounds. One thing that
check_default_allows_bound() misses is, if there's already a constraint on the
default partition refutes the partition constraint on the new partition, we can
skip the scan of the default partition since it can not have rows that would
fit the new partition. ATExecAttachPartition() has code to deal with a similar
case i.e. the table being attached has a constraint which implies the partition
constraint. There may be more cases which check_default_allows_bound() does not
handle but ATExecAttachPartition() handles. So, I am wondering whether it's
better to somehow take out the common code into a function and use it. We will
have to deal with a difference through. The first one would throw an error when
finding a row that satisfies partition constraints whereas the second one would
throw an error when it doesn't find such a row. But this difference can be
handled through a flag or by negating the constraint. This would also take care
of Amit Langote's complaint about foreign partitions. There's also another
difference that the ATExecAttachPartition() queues the table for scan and the
actual scan takes place in ATRewriteTable(), but there is not such queue while
creating a table as a partition. But we should check if we can reuse the code to
scan the heap for checking a constraint.

In case of ATTACH PARTITION, probably we should schedule scan of default
partition in the alter table's work queue like what ATExecAttachPartition() is
doing for the table being attached. That would fit in the way alter table
works.

 make_partition_op_expr(PartitionKey key, int keynum,
-   uint16 strategy, Expr *arg1, Expr *arg2)
+uint16 strategy, Expr *arg1, Expr *arg2, bool is_default)
Indentation

+if (is_default &&
+((operoid = get_negator(operoid)) == InvalidOid))
+ereport(ERROR, 

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Thu, Jun 1, 2017 at 3:35 PM, Jeevan Ladhe
 wrote:
> Please let me know if I have missed anything and any further comments.

+ errmsg("a default partition \"%s\" already exists",

I suggest: partition \"%s\" conflicts with existing default partition \"%s\"

The point is that's more similar to the message you get when overlap
&& !spec->is_default.

+ * If the default partition exists, it's partition constraint will change

it's -> its

+ errmsg("default partition contains row(s)
that would overlap with partition being created")));

It doesn't really sound right to talk about rows overlapping with a
partition.  Partitions can overlap with each other, but not rows.
Also, it's not really project style to use ambiguously plural forms
like "row(s)" in error messages.  Maybe something like:

new partition constraint for default partition \"%s\" would be
violated by some row

+/*
+ * InvalidateDefaultPartitionRelcache
+ *
+ * Given a parent oid, this function checks if there exists a default partition
+ * and invalidates it's relcache if it exists.
+ */
+void
+InvalidateDefaultPartitionRelcache(Oid parentOid)
+{
+Relation parent = heap_open(parentOid, AccessShareLock);
+Oid default_relid =
parent->rd_partdesc->oids[DEFAULT_PARTITION_INDEX(parent)];
+
+if (partition_bound_has_default(parent->rd_partdesc->boundinfo))
+CacheInvalidateRelcacheByRelid(default_relid);
+
+heap_close(parent, AccessShareLock);
+}

It does not seem like a good idea to put the heap_open() call inside
this function.  One of the two callers already *has* the Relation, and
we definitely want to avoid pulling the Oid out of the Relation only
to reopen it to get the Relation back.  And I think
heap_drop_with_catalog could open the parent relation instead of
calling LockRelationOid().

If DETACH PARTITION and DROP PARTITION require this, why not ATTACH
PARTITION and CREATE TABLE .. PARTITION OF?

The indentation of the changes in gram.y doesn't appear to match the
nearby code.  I'd remove this comment:

+ * Currently this is supported only for LIST partition.

Since nothing here is dependent on this working only for LIST
partitions, and since this will probably change, I think it would be
more future-proof to leave this out, lest somebody forget to update it
later.

-switch (spec->strategy)
+if (spec->is_default && (strategy == PARTITION_STRATEGY_LIST ||
+ strategy == PARTITION_STRATEGY_RANGE))

Checking strategy here appears pointless.

This is not a full review, but I'm out of time for today.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi,

I have addressed Ashutosh's and Amit's comments in the attached patch.

Please let me know if I have missed anything and any further comments.

PFA.

Regards,
Jeevan Ladhe

On Wed, May 31, 2017 at 9:50 AM, Beena Emerson 
wrote:

> On Wed, May 31, 2017 at 8:13 AM, Amit Langote
>  wrote:
> > On 2017/05/31 9:33, Amit Langote wrote:
> >
> >
> > In get_rule_expr():
> >
> >  case PARTITION_STRATEGY_LIST:
> >  Assert(spec->listdatums != NIL);
> >
> > +/*
> > + * If the boundspec is of Default partition, it
> does
> > + * not have list of datums, but has only one
> node to
> > + * indicate its a default partition.
> > + */
> > +if (isDefaultPartitionBound(
> > +(Node *)
> linitial(spec->listdatums)))
> > +{
> > +appendStringInfoString(buf, "DEFAULT");
> > +break;
> > +}
> > +
> >
> > How about adding this part before the switch (key->strategy)?  That way,
> > we won't have to come back and add this again when we add range default
> > partitions.
>
> I think it is best that we add a bool is_default to PartitionBoundSpec
> and then have a general check for both list and range. Though
> listdatums, upperdatums and lowerdatums are set to default for a
> DEFAULt partition, it does not seem proper that we check listdatums
> for range as well.
>
>
>
>
> --
>
> Beena Emerson
>
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>


default_partition_v18.patch
Description: Binary data

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

Re: [HACKERS] Adding support for Default partition in partitioning

[Beena Emerson]

On Wed, May 31, 2017 at 8:13 AM, Amit Langote
 wrote:
> On 2017/05/31 9:33, Amit Langote wrote:
>
>
> In get_rule_expr():
>
>  case PARTITION_STRATEGY_LIST:
>  Assert(spec->listdatums != NIL);
>
> +/*
> + * If the boundspec is of Default partition, it does
> + * not have list of datums, but has only one node to
> + * indicate its a default partition.
> + */
> +if (isDefaultPartitionBound(
> +(Node *) linitial(spec->listdatums)))
> +{
> +appendStringInfoString(buf, "DEFAULT");
> +break;
> +}
> +
>
> How about adding this part before the switch (key->strategy)?  That way,
> we won't have to come back and add this again when we add range default
> partitions.

I think it is best that we add a bool is_default to PartitionBoundSpec
and then have a general check for both list and range. Though
listdatums, upperdatums and lowerdatums are set to default for a
DEFAULt partition, it does not seem proper that we check listdatums
for range as well.




-- 

Beena Emerson

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Amit Langote]

On 2017/05/31 9:33, Amit Langote wrote:
> On 2017/05/30 16:38, Jeevan Ladhe wrote:
>> I have rebased the patch on the latest commit.
>> PFA.
> 
> Was looking at the patch

I tried creating default partition of a range-partitioned table and got
the following error:

ERROR:  invalid bound specification for a range partition

I thought it would give:

ERROR: creating default partition is not supported for range partitioned
tables

Which means transformPartitionBound() should perform this check more
carefully.  As I suggested in my previous email, if there were a
is_default field in the PartitionBoundSpec, then one could add the
following block of code at the beginning of transformPartitionBound:


  if (spec->is_default && spec->strategy != PARTITION_STRATEGY_LIST)
  ereport(ERROR,
  (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
   errmsg("creating default partition is not supported for %s
partitioned tables", get_partition_strategy_name(key->strategy;


Some more comments on the patch:

+ errmsg("new default partition constraint is
violated by some row"),

"new default partition constraint" may sound a bit confusing to users.
That we recompute the default partition's constraint and check the "new
constraint" against the rows it contains seems to me to be the description
of internal details.  How about:

ERROR: default partition contains rows that belong to partition being created

+char *ExecBuildSlotValueDescription(Oid reloid,
+  TupleTableSlot *slot,
+  TupleDesc tupdesc,
+  Bitmapset *modifiedCols,
+  int maxfieldlen);

It seems that you made the above public to use it in
check_default_allows_bound(), which while harmless, I'm not sure if
needed.  ATRewriteTable() in tablecmds.c, for example, emits the following
error messages:

errmsg("check constraint \"%s\" is violated by some row",

errmsg("partition constraint is violated by some row")));

but neither outputs the DETAIL part showing exactly what row.  I think
it's fine for check_default_allows_bound() not to show the row itself and
hence no need to make ExecBuildSlotValueDescription public.


In get_rule_expr():

 case PARTITION_STRATEGY_LIST:
 Assert(spec->listdatums != NIL);

+/*
+ * If the boundspec is of Default partition, it does
+ * not have list of datums, but has only one node to
+ * indicate its a default partition.
+ */
+if (isDefaultPartitionBound(
+(Node *) linitial(spec->listdatums)))
+{
+appendStringInfoString(buf, "DEFAULT");
+break;
+}
+

How about adding this part before the switch (key->strategy)?  That way,
we won't have to come back and add this again when we add range default
partitions.

Gotta go; will provide more comments later.

Thanks,
Amit



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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Thanks Amit for your comments.

On 31-May-2017 6:03 AM, "Amit Langote" 
wrote:

Hi Jeevan,

On 2017/05/30 16:38, Jeevan Ladhe wrote:
> I have rebased the patch on the latest commit.
> PFA.

Was looking at the patch and felt that the parse node representation of
default partition bound could be slightly different.  Can you explain the
motivation behind implementing it without adding a new member to the
PartitionBoundSpec struct?

I would suggest instead adding a bool named is_default and be done with
it.  It will help get rid of the public isDefaultPartitionBound() in the
proposed patch whose interface isn't quite clear and instead simply check
if (spec->is_default) in places where it's called by passing it (Node *)
linitial(spec->listdatums).


I thought of reusing the existing members of PartitionBoundSpec, but I
agree that having a bool could simplify the code. Will do the receptive
change.

Further looking into the patch, I found a tiny problem in
check_default_allows_bound().  If the default partition that will be
scanned by it is a foreign table or a partitioned table with a foreign
leaf partition, you will get a failure like:

-- default partition is a foreign table
alter table p attach partition fp default;

-- adding a new partition will try to scan fp above
alter table p attach partition p12 for values in (1, 2);
ERROR:  could not open file "base/13158/16456": No such file or directory

I think the foreign tables should be ignored here to avoid the error.  The
fact that foreign default partition may contain data that satisfies the
new partition's constraint is something we cannot do much about.  Also,
see the note in ATTACH PARTITION description regarding foreign tables [1]
and the discussion at [2].


Will look into this.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Amit Langote]

Hi Jeevan,

On 2017/05/30 16:38, Jeevan Ladhe wrote:
> I have rebased the patch on the latest commit.
> PFA.

Was looking at the patch and felt that the parse node representation of
default partition bound could be slightly different.  Can you explain the
motivation behind implementing it without adding a new member to the
PartitionBoundSpec struct?

I would suggest instead adding a bool named is_default and be done with
it.  It will help get rid of the public isDefaultPartitionBound() in the
proposed patch whose interface isn't quite clear and instead simply check
if (spec->is_default) in places where it's called by passing it (Node *)
linitial(spec->listdatums).

Further looking into the patch, I found a tiny problem in
check_default_allows_bound().  If the default partition that will be
scanned by it is a foreign table or a partitioned table with a foreign
leaf partition, you will get a failure like:

-- default partition is a foreign table
alter table p attach partition fp default;

-- adding a new partition will try to scan fp above
alter table p attach partition p12 for values in (1, 2);
ERROR:  could not open file "base/13158/16456": No such file or directory

I think the foreign tables should be ignored here to avoid the error.  The
fact that foreign default partition may contain data that satisfies the
new partition's constraint is something we cannot do much about.  Also,
see the note in ATTACH PARTITION description regarding foreign tables [1]
and the discussion at [2].

Thanks,
Amit

[1] https://www.postgresql.org/docs/devel/static/sql-altertable.html
[2]
https://www.postgresql.org/message-id/flat/8f89dcb2-bd15-d8dc-5f54-3e11dc6c9463%40lab.ntt.co.jp



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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Tue, May 30, 2017 at 1:08 PM, Jeevan Ladhe
 wrote:
> Hi,
>
> I have rebased the patch on the latest commit.
> PFA.
>

Thanks for rebasing the patch. Here are some review comments.
+/*
+ * In case of default partition, just note the index, we do not
+ * add this to non_null_values list.
+ */
We may want to rephrase it like
"Note the index of the partition bound spec for the default partition. There's
no datum to add to the list of non-null datums for this partition."

/* Assign mapping index for default partition. */
"mapping index" should be "mapped index". May be we want to use "the" before
default partition everywhere, there's only one specific default partition.

Assert(default_index >= 0 &&
   mapping[default_index] == -1);
Needs some explanation for asserting mapping[default_index] == -1. Since
default partition accepts any non-specified value, it should not get a mapped
index while assigning those for non-null datums.

+ * Currently range partition do not have default partition
May be rephrased as "As of now, we do not support default range partition."

+ * ArrayExpr, which would return an negated expression for default
a negated instead of an negated.

+cur_index = -1;
 /*
- * A null partition key is only acceptable if null-accepting list
- * partition exists.
+ * A null partition key is acceptable if null-accepting list partition
+ * or a default partition exists. Check if there exists a null
+ * accepting partition, else this will be handled later by default
+ * partition if it exists.
  */
-cur_index = -1;
Why do we need to move assignment to cur_index before the comment.
The comment should probably change to "Handle NULL partition key here
if there's a
null-accepting list partition. Else it will routed to a default partition if
one exists."

+-- attaching default partition overlaps if a default partition already exists
+ERROR:  partition "part_def2" would overlap partition "part_def1"
Saying a default partition overlaps is misleading here. A default partition is
not exepected to overlap with anything. It's expected to "adjust" with the rest
of the partitions. It can "conflict" with another default partition. So the
right error message here is "a default partition "part_def1" already exists."

+CREATE TABLE part_def1 PARTITION OF list_parted DEFAULT;
+CREATE TABLE part_def2 (LIKE part_1 INCLUDING CONSTRAINTS);
+ALTER TABLE list_parted ATTACH PARTITION part_def2 DEFAULT;
May be you want to name part_def1 as def_part and part_def2 as fail_def_part to
be consistent with other names in the file. May be you want to test to
consecutive CREATE TABLE ... DEFAULT.

+ALTER TABLE list_parted2 ATTACH PARTITION part_3 FOR VALUES IN (11);
+ERROR:  new default partition constraint is violated by some row
+DETAIL:  Violating row contains (11, z).
The error message seems to be misleading. The default partition is not new. May
be we should say, "default partition contains rows that conflict with the
partition bounds of "part_3"". I think we should use a better word instead of
"conflict", but I am not able to find one right now.

+-- check that leaf partitons of default partition are scanned when
s/partitons/partitions/

-ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IN (5)), ALTER a
SET NOT NULL;
-ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5);
+ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IN (5, 55)), ALTER
a SET NOT NULL;
+ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5, 55);
Why do we want to change partition bounds of this one? The test is for children
of part_5 right?

+drop table part_default;
I think this is premature drop. Down the file there's a SELECT from
list_parted, which won't list the rows inserted to the default partition and we
will miss to check whether the tuples were routed to the right partition or
not.

+update list_part1 set a = 'c' where a = 'a';
+ERROR:  new row for relation "list_part1" violates partition constraint
+DETAIL:  Failing row contains (c, 1).
Why do we need this test here? It's not dealing with the default partition and
partition row movement is not in there. So the updated row may not move to the
default partition, even if it's there.

This isn't a complete review. I will continue to review this patch further.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi,

I have fixed the issue related to default partition constraints not getting
updated
after detaching a partition.

PFA.

Regards,
Jeevan Ladhe

On Tue, May 30, 2017 at 1:08 PM, Jeevan Ladhe  wrote:

> Hi,
>
> I have rebased the patch on the latest commit.
> PFA.
>
> There exists one issue reported by Rajkumar[1] off-line as following, where
> describing the default partition after deleting null partition, does not
> show
> updated constraints. I am working on fixing this issue.
>
> create table t1 (c1 int) partition by list (c1);
> create table t11 partition of t1 for values in (1,2);
> create table t12 partition of t1 default;
> create table t13 partition of t1 for values in (10,11);
> create table t14 partition of t1 for values in (null);
>
> postgres=# \d+ t12
> Table "public.t12"
>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target | Description
> +-+---+--+-+
> -+--+-
>  c1 | integer |   |  | | plain   |
>  |
> Partition of: t1 DEFAULT
> Partition constraint: ((c1 IS NOT NULL) AND (c1 <> ALL (ARRAY[1, 2, 10,
> 11])))
>
> postgres=# alter table t1 detach partition t14;
> ALTER TABLE
> postgres=# \d+ t12
> Table "public.t12"
>  Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target | Description
> +-+---+--+-+
> -+--+-
>  c1 | integer |   |  | | plain   |
>  |
> Partition of: t1 DEFAULT
> Partition constraint: ((c1 IS NOT NULL) AND (c1 <> ALL (ARRAY[1, 2, 10,
> 11])))
>
> postgres=# insert into t1 values(null);
> INSERT 0 1
>
> Note that the parent correctly allows the nulls to be inserted.
>
> [1] rajkumar.raghuwan...@enterprisedb.com
>
> Regards,
> Jeevan Ladhe
>
> On Tue, May 30, 2017 at 10:59 AM, Beena Emerson 
> wrote:
>
>> On Mon, May 29, 2017 at 9:33 PM, Jeevan Ladhe
>>  wrote:
>> > Hi,
>> >
>> > I have rebased the patch on latest commit with few cosmetic changes.
>> >
>> > The patch fix_listdatums_get_qual_for_list_v3.patch [1]  needs to be
>> applied
>> > before applying this patch.
>> >
>> > [1] http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg
>> 315490.html
>> >
>>
>>
>> This needs a rebase again.
>>
>> --
>>
>> Beena Emerson
>>
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>


default_partition_v17.patch
Description: Binary data

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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi,

I have rebased the patch on the latest commit.
PFA.

There exists one issue reported by Rajkumar[1] off-line as following, where
describing the default partition after deleting null partition, does not
show
updated constraints. I am working on fixing this issue.

create table t1 (c1 int) partition by list (c1);
create table t11 partition of t1 for values in (1,2);
create table t12 partition of t1 default;
create table t13 partition of t1 for values in (10,11);
create table t14 partition of t1 for values in (null);

postgres=# \d+ t12
Table "public.t12"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
+-+---+--+-+-+--+-
 c1 | integer |   |  | | plain   |
 |
Partition of: t1 DEFAULT
Partition constraint: ((c1 IS NOT NULL) AND (c1 <> ALL (ARRAY[1, 2, 10,
11])))

postgres=# alter table t1 detach partition t14;
ALTER TABLE
postgres=# \d+ t12
Table "public.t12"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
+-+---+--+-+-+--+-
 c1 | integer |   |  | | plain   |
 |
Partition of: t1 DEFAULT
Partition constraint: ((c1 IS NOT NULL) AND (c1 <> ALL (ARRAY[1, 2, 10,
11])))

postgres=# insert into t1 values(null);
INSERT 0 1

Note that the parent correctly allows the nulls to be inserted.

[1] rajkumar.raghuwan...@enterprisedb.com

Regards,
Jeevan Ladhe

On Tue, May 30, 2017 at 10:59 AM, Beena Emerson 
wrote:

> On Mon, May 29, 2017 at 9:33 PM, Jeevan Ladhe
>  wrote:
> > Hi,
> >
> > I have rebased the patch on latest commit with few cosmetic changes.
> >
> > The patch fix_listdatums_get_qual_for_list_v3.patch [1]  needs to be
> applied
> > before applying this patch.
> >
> > [1] http://www.mail-archive.com/pgsql-hackers@postgresql.org/
> msg315490.html
> >
>
>
> This needs a rebase again.
>
> --
>
> Beena Emerson
>
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>


default_partition_v16.patch
Description: Binary data

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

Re: [HACKERS] Adding support for Default partition in partitioning

[Beena Emerson]

On Mon, May 29, 2017 at 9:33 PM, Jeevan Ladhe
 wrote:
> Hi,
>
> I have rebased the patch on latest commit with few cosmetic changes.
>
> The patch fix_listdatums_get_qual_for_list_v3.patch [1]  needs to be applied
> before applying this patch.
>
> [1] http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg315490.html
>


This needs a rebase again.

-- 

Beena Emerson

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi,

I have rebased the patch on latest commit with few cosmetic changes.

The patch fix_listdatums_get_qual_for_list_v3.patch [1]

 needs to be applied
before applying this patch.

[1] http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg315490.html

Regards,
Jeevan Ladhe


On Mon, May 29, 2017 at 2:28 PM, Jeevan Ladhe  wrote:

>
>
>> The existing comment is not valid
>> /*
>>  * A null partition key is only acceptable if null-accepting
>> list
>>  * partition exists.
>>  */
>> as we allow NULL to be stored in default. It should be updated.
>>
>
> Sure Beena, as stated earlier will update this on my next version of patch.
>
>
> Regards,
> Jeevan Ladhe
>


default_partition_v15.patch
Description: Binary data

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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

>
> The existing comment is not valid
> /*
>  * A null partition key is only acceptable if null-accepting
> list
>  * partition exists.
>  */
> as we allow NULL to be stored in default. It should be updated.
>

Sure Beena, as stated earlier will update this on my next version of patch.


Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

This patch needs a rebase on recent commits, and also a fix[1] that is
posted for get_qual_for_list().

I am working on both of these tasks. Will update the patch once I am done
with this.


Regards,

Jeevan Ladhe

On Mon, May 29, 2017 at 12:25 PM, Beena Emerson 
wrote:

> On Thu, May 25, 2017 at 3:03 PM, Jeevan Ladhe
>  wrote:
> >
> > Forgot to attach the patch.
> > PFA.
> >
> > On Thu, May 25, 2017 at 3:02 PM, Jeevan Ladhe <
> jeevan.la...@enterprisedb.com> wrote:
> >>
> >> Hi Rajkumar,
> >>
> >>> postgres=# CREATE TEMP TABLE temp_list_part (a int) PARTITION BY LIST
> (a);
> >>> CREATE TABLE
> >>> postgres=# CREATE TEMP TABLE temp_def_part (a int);
> >>> CREATE TABLE
> >>> postgres=# ALTER TABLE temp_list_part ATTACH PARTITION temp_def_part
> DEFAULT;
> >>> 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 for reporting.
> >> PFA patch that fixes above issue.
> >>
>
>
> The existing comment is not valid
> /*
>  * A null partition key is only acceptable if null-accepting
> list
>  * partition exists.
>  */
> as we allow NULL to be stored in default. It should be updated.
>
> DROP TABLE list1;
> CREATE TABLE list1 (a int) PARTITION BY LIST (a);
> CREATE TABLE list1_1 (LIKE list1);
> ALTER TABLE  list1 ATTACH PARTITION list1_1 FOR VALUES IN (2);
> CREATE TABLE list1_def PARTITION OF list1 DEFAULT;
> INSERT INTO list1 VALUES (NULL);
> SELECT * FROM list1_def;
>  a
> ---
>
> (1 row)
>
>
> --
>
> Beena Emerson
>
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [HACKERS] Adding support for Default partition in partitioning

[Beena Emerson]

On Thu, May 25, 2017 at 3:03 PM, Jeevan Ladhe
 wrote:
>
> Forgot to attach the patch.
> PFA.
>
> On Thu, May 25, 2017 at 3:02 PM, Jeevan Ladhe  
> wrote:
>>
>> Hi Rajkumar,
>>
>>> postgres=# CREATE TEMP TABLE temp_list_part (a int) PARTITION BY LIST (a);
>>> CREATE TABLE
>>> postgres=# CREATE TEMP TABLE temp_def_part (a int);
>>> CREATE TABLE
>>> postgres=# ALTER TABLE temp_list_part ATTACH PARTITION temp_def_part 
>>> DEFAULT;
>>> 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 for reporting.
>> PFA patch that fixes above issue.
>>


The existing comment is not valid
/*
 * A null partition key is only acceptable if null-accepting list
 * partition exists.
 */
as we allow NULL to be stored in default. It should be updated.

DROP TABLE list1;
CREATE TABLE list1 (a int) PARTITION BY LIST (a);
CREATE TABLE list1_1 (LIKE list1);
ALTER TABLE  list1 ATTACH PARTITION list1_1 FOR VALUES IN (2);
CREATE TABLE list1_def PARTITION OF list1 DEFAULT;
INSERT INTO list1 VALUES (NULL);
SELECT * FROM list1_def;
 a
---

(1 row)


-- 

Beena Emerson

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Forgot to attach the patch.
PFA.

On Thu, May 25, 2017 at 3:02 PM, Jeevan Ladhe  wrote:

> Hi Rajkumar,
>
> postgres=# CREATE TEMP TABLE temp_list_part (a int) PARTITION BY LIST (a);
>> CREATE TABLE
>> postgres=# CREATE TEMP TABLE temp_def_part (a int);
>> CREATE TABLE
>> postgres=# ALTER TABLE temp_list_part ATTACH PARTITION temp_def_part
>> DEFAULT;
>> 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 for reporting.
> PFA patch that fixes above issue.
>
> Regards,
> Jeevan Ladhe
>


default_partition_v14.patch
Description: Binary data

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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Rajkumar,

postgres=# CREATE TEMP TABLE temp_list_part (a int) PARTITION BY LIST (a);
> CREATE TABLE
> postgres=# CREATE TEMP TABLE temp_def_part (a int);
> CREATE TABLE
> postgres=# ALTER TABLE temp_list_part ATTACH PARTITION temp_def_part
> DEFAULT;
> 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 for reporting.
PFA patch that fixes above issue.

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Rajkumar Raghuwanshi]

On Thu, May 25, 2017 at 12:10 PM, Jeevan Ladhe <
jeevan.la...@enterprisedb.com> wrote:

> PFA.
>

Hi

I have applied v13 patch, got a crash when trying to attach default temp
partition.

postgres=# CREATE TEMP TABLE temp_list_part (a int) PARTITION BY LIST (a);
CREATE TABLE
postgres=# CREATE TEMP TABLE temp_def_part (a int);
CREATE TABLE
postgres=# ALTER TABLE temp_list_part ATTACH PARTITION temp_def_part
DEFAULT;
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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi,

I started looking into Rahila's default_partition_v11.patch, and reworked on
few things as below:

- I tried to cover all the review comments posted on the thread. Do let
me know if something is missing.

- Got rid of the functions get_qual_for_default() and
generate_qual_for_defaultpart().
There is no need of collecting boundspecs of all the partitions in case of
list
partition, the list is available in boundinfo->ndatums, an expression for
default can be created from the information that is available in boundinfo.

- Got rid of variable has_default, and added a macro for it.

- Changed the logic of checking the overlapping of existing rows in default
partition. Earlier version of patch used to build new constraints for
default
partition table and then was checking if any of existing rows violate those
constraints. However, current version of patch just checks if any of the
rows in
default partition satisfy the new partition's constraint and fail if there
exists any.
This logic can also be used as it is for default partition in case of RANGE
partitioning.

- Simplified grammar rule.

- Got rid of PARTITION_DEFAULT since DEFAULT is not a different partition
strategy, the applicable logic is also revised:

- There are few other code adjustments like: indentation, commenting, code
simplification etc.

- Added regression tests.

TODO:
Documentation, I am working on it. Will updated the patch soon.

PFA.

Regards,
Jeevan

On Mon, May 22, 2017 at 7:31 AM, Beena Emerson 
wrote:

> Hello,
>
> Patch for default range partition has been added. PFA the rebased v12
> patch for the same.
> I have not removed the has_default variable yet.
>
> Default range partition: https://www.postgresql.org/message-id/
> CAOG9ApEYj34fWMcvBMBQ-YtqR9fTdXhdN82QEKG0SVZ6zeL1xg%40mail.gmail.com
> --
>
> Beena Emerson
>
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>


default_partition_v13.patch
Description: Binary data

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

Re: [HACKERS] Adding support for Default partition in partitioning

[Beena Emerson]

Hello,

Patch for default range partition has been added. PFA the rebased v12 patch
for the same.
I have not removed the has_default variable yet.

Default range partition:
https://www.postgresql.org/message-id/CAOG9ApEYj34fWMcvBMBQ-YtqR9fTdXhdN82QEKG0SVZ6zeL1xg%40mail.gmail.com
-- 

Beena Emerson

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


default_partition_v12_rebase.patch
Description: Binary data

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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Wed, May 17, 2017 at 2:28 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> On Tue, May 16, 2017 at 9:01 PM, Robert Haas 
> wrote:
> > On Tue, May 16, 2017 at 8:57 AM, Jeevan Ladhe
> >  wrote:
> >> I have fixed the crash in attached patch.
> >> Also the patch needed bit of adjustments due to recent commit.
> >> I have re-based the patch on latest commit.
> >
> > +boolhas_default;/* Is there a default partition?
> > Currently false
> > + * for a range partitioned table */
> > +intdefault_index;/* Index of the default list
> > partition. -1 for
> > + * range partitioned tables */
> >
>
> We have has_null and null_index for list partitioning. There
> null_index == -1 = has_null. May be Rahila and/or Jeevan just copied
> that style. Probably we should change that as well?
>
>
I agree with Ashutosh.
I had given similar comment on earlier version of patch[1]
,
and  Rahila reverted
with above reasoning, hence did not change the logic she introduced.

Probably its a good idea to have a separate patch that removes has_null
logic,
in a separate thread.

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

Regards,
Jeevan Ladhe.

Re: [HACKERS] Adding support for Default partition in partitioning

[Amit Langote]

On 2017/05/17 17:58, Ashutosh Bapat wrote:
> On Tue, May 16, 2017 at 9:01 PM, Robert Haas  wrote:
>> On Tue, May 16, 2017 at 8:57 AM, Jeevan Ladhe
>>  wrote:
>>> I have fixed the crash in attached patch.
>>> Also the patch needed bit of adjustments due to recent commit.
>>> I have re-based the patch on latest commit.
>>
>> +boolhas_default;/* Is there a default partition?
>> Currently false
>> + * for a range partitioned table */
>> +intdefault_index;/* Index of the default list
>> partition. -1 for
>> + * range partitioned tables */
>>
> 
> We have has_null and null_index for list partitioning. There
> null_index == -1 = has_null. May be Rahila and/or Jeevan just copied
> that style. Probably we should change that as well?

Probably a good idea.

Thanks,
Amit



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

Re: [HACKERS] Adding support for Default partition in partitioning

[Ashutosh Bapat]

On Tue, May 16, 2017 at 9:01 PM, Robert Haas  wrote:
> On Tue, May 16, 2017 at 8:57 AM, Jeevan Ladhe
>  wrote:
>> I have fixed the crash in attached patch.
>> Also the patch needed bit of adjustments due to recent commit.
>> I have re-based the patch on latest commit.
>
> +boolhas_default;/* Is there a default partition?
> Currently false
> + * for a range partitioned table */
> +intdefault_index;/* Index of the default list
> partition. -1 for
> + * range partitioned tables */
>

We have has_null and null_index for list partitioning. There
null_index == -1 = has_null. May be Rahila and/or Jeevan just copied
that style. Probably we should change that as well?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Tue, May 16, 2017 at 8:57 AM, Jeevan Ladhe
 wrote:
> I have fixed the crash in attached patch.
> Also the patch needed bit of adjustments due to recent commit.
> I have re-based the patch on latest commit.

+boolhas_default;/* Is there a default partition?
Currently false
+ * for a range partitioned table */
+intdefault_index;/* Index of the default list
partition. -1 for
+ * range partitioned tables */

Why do we need both has_default and default_index?  If default_index
== -1 means that there is no default, we don't also need a separate
bool to record the same thing, do we?

get_qual_for_default() still returns a list of things that are not
quals.  I think that this logic is all pretty poorly organized.  The
logic to create a partitioning constraint for a list partition should
be part of get_qual_for_list(), whether or not it is a default.  And
when we have range partitions, the logic to create a default range
partitioning constraint should be part of get_qual_for_range().  The
code the way it's organized today makes it look like there are three
kinds of partitions: list, range, and default.  But that's not right
at all.  There are two kinds: list and range.  And a list partition
might or might not be a default partition, and similarly for range.

+ereport(ERROR, (errcode(ERRCODE_CHECK_VIOLATION),
+errmsg("DEFAULT partition cannot be used"
+   " without negator of operator  %s",
+   get_opname(operoid;

I don't think ERRCODE_CHECK_VIOLATION is the right error code here,
and we have a policy against splitting message strings like this.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Fri, May 12, 2017 at 7:34 PM, Beena Emerson 
wrote:

>
> 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.
> !>
>

Hi,

I have fixed the crash in attached patch.
Also the patch needed bit of adjustments due to recent commit.
I have re-based the patch on latest commit.

PFA.

Regards,
Jeevan Ladhe


default_partition_v12.patch
Description: Binary data

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

Re: [HACKERS] Adding support for Default partition in partitioning

[Beena Emerson]

Hello,


On Fri, May 12, 2017 at 5:30 PM, Rahila Syed  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:  != <>
> 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 
> wrote:
>
>>
>>
>> On Thu, May 11, 2017 at 7:37 PM, Rahila Syed 
>> 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:  != <>
>>
>>
>>> >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

Re: [HACKERS] Adding support for Default partition in partitioning

[Rahila Syed]

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.

>(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:  != <>
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 
wrote:

>
>
> On Thu, May 11, 2017 at 7:37 PM, Rahila Syed 
> 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:  != <>
>
>
>> >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
>>
>>
>>


default_partition_v11.patch
Description: application/download

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

Re: [HACKERS] Adding support for Default partition in partitioning

[Beena Emerson]

On Thu, May 11, 2017 at 7:37 PM, Rahila Syed  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:  != <>


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Rahila,

On Thu, May 11, 2017 at 7:37 PM, Rahila Syed  wrote:
>
> >3.
> >In following function isDefaultPartitionBound, first statement "return
> false"
> >is not needed.
> It is needed to return false if the node is not DefElem.
>

Please have a look at following code:

+ * Returns true if the partition bound is default
+ */
+bool
+isDefaultPartitionBound(Node *value)
+{
+ if (IsA(value, DefElem))
+ {
+ DefElem defvalue = (DefElem ) value;
+ if(!strcmp(defvalue->defname, "DEFAULT"))
+ return true;
+ return false;
+ }
+ return false;
+}

By first return false, I mean to say the return statement inside the
if block "if (IsA(value, DefElem))":

+ if(!strcmp(defvalue->defname, "DEFAULT"))
+ return true;
+ return false;

Even if this "return false" is not present, the control is anyway going to
fall through and will return false from the outermost return statement.

I leave this decision to you, but further this block could be rewritten as
below and also can be defined as a macro:

bool
isDefaultPartitionBound(Node *value)
{
return (IsA(value, DefElem) &&
!strcmp(((DefElem) value)->defname, "DEFAULT"));
}

Regards,
Jeevan Ladhe

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Thu, May 11, 2017 at 10:07 AM, Rahila Syed  wrote:
> Please find attached an updated patch with review comments and bugs reported
> till date implemented.

You haven't done anything about the repeated suggestion that this
should also cover range partitioning.

+/*
+ * If the partition is the default partition switch
+ * back to PARTITION_STRATEGY_LIST
+ */
+if (spec->strategy == PARTITION_DEFAULT)
+result_spec->strategy = PARTITION_STRATEGY_LIST;
+else
+ereport(ERROR,
+(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+ errmsg("invalid bound specification for a list
partition"),
  parser_errposition(pstate, exprLocation(bound;

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.

+/*
+ * Skip if it's a partitioned table.  Only RELKIND_RELATION
+ * relations (ie, leaf partitions) need to be scanned.
+ */
+if (part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)

What about foreign table partitions?

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.

+bound_datums = list_copy(spec->listdatums);
+
+boundspecs = get_qual_for_default(parent, defid);
+
+foreach(cell, bound_datums)
+{
+Node *value = lfirst(cell);
+boundspecs = lappend(boundspecs, value);
+}

There's an existing function that you can use to concatenate two lists
instead of open-coding it.

Also, I think that before you ask anyone to spend too much more time
and energy reviewing this, you should really add the documentation and
regression tests which you mentioned as a TODO.  And run the code
through pgindent.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Rahila Syed]

Hello,

Please find attached an updated patch with review comments and bugs
reported till date implemented.

>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



On Fri, May 5, 2017 at 1:30 AM, Jeevan Ladhe 
wrote:

> Hi Rahila,
>
> I have started reviewing your latest patch, and here are my initial
> comments:
>
> 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.
>
> @@ -166,6 +172,8 @@ RelationBuildPartitionDesc(Relation rel)
>   /* List partitioning specific */
>   PartitionListValue **all_values = NULL;
>   bool found_null = false;
> + bool found_def = false;
> + int def_index = -1;
>   int null_index = -1;
>
> 2.
> In check_new_partition_bound, in case of PARTITION_STRATEGY_LIST, remove
> following duplicate declaration of boundinfo, because it is confusing and
> after
> your changes it is not needed as its not getting overridden in the if block
> locally.
> if (partdesc->nparts > 0)
> {
> PartitionBoundInfo boundinfo = partdesc->boundinfo;
> ListCell   *cell;
>
>
> 3.
> In following function isDefaultPartitionBound, first statement "return
> false"
> is not needed.
>
> + * Returns true if the partition bound is default
> + */
> +bool
> +isDefaultPartitionBound(Node *value)
> +{
> + if (IsA(value, DefElem))
> + {
> + DefElem *defvalue = (DefElem *) value;
> + if(!strcmp(defvalue->defname, "DEFAULT"))
> + return true;
> + return false;
> + }
> + return false;
> +}
>
> 4.
> As mentioned in my previous set of comments, following if block inside a
> loop
> in get_qual_for_default needs a break:
>
> + foreach(cell1, bspec->listdatums)
> + {
> + Node *value = lfirst(cell1);
> + if (isDefaultPartitionBound(value))
> + {
> + def_elem = true;
> + *defid  = inhrelid;
> + }
> + }
>
> 5.
> In the grammar the rule default_part_list is not needed:
>
> +default_partition:
> + DEFAULT  { $$ = (Node *)makeDefElem("DEFAULT", NULL, @1); }
> +
> +default_part_list:
> + default_partition { $$ = list_make1($1); }
> + ;
> +
>
> Instead you can simply declare default_partition as a list and write it as:
>
> default_partition:
> DEFAULT
> {
> Node *def = (Node *)makeDefElem("DEFAULT", NULL, @1);
> $$ = list_make1(def);
> }
>
> 6.
> You need to change the output of the describe command, which is currently
> as below: postgres=# \d+ test; Table "public.test" Column | Type |
> Collation | Nullable | Default | Storage | Stats target | Description
> +-+---+--+-+-+--+-
> a | integer | | | | plain | | b | date | | | | plain | | Partition key:
> LIST (a) Partitions: pd FOR VALUES IN (DEFAULT), test_p1 FOR VALUES IN (4,
> 5) What about changing the Paritions output as below: Partitions: *pd
> DEFAULT,* test_p1 FOR VALUES IN (4, 5)
>
> 7.
> You need to handle tab completion for DEFAULT.
> e.g.
> If I partially type following command:
> CREATE TABLE pd PARTITION OF test DEFA
> and then press tab, I get following completion:
> CREATE TABLE pd PARTITION OF test FOR VALUES
>
> I did some primary testing and did not find any problem so far.
>
> I will review and test further and let you know my comments.
>
> Regards,
> Jeevan Ladhe
>
> On Thu, May 4, 2017 at 6:09 PM, Rajkumar Raghuwanshi <
> rajkumar.raghuwan...@enterprisedb.com> wrote:
>
>> On Thu, May 4, 2017 at 5:14 PM, Rahila Syed 
>> wrote:
>>
>>> The syntax implemented in this patch is as follows,
>>>
>>> CREATE TABLE p11 PARTITION OF p1 DEFAULT;
>>>
>>> Applied v9 patches, table description still showing old pattern of
>> default partition. Is it expected?
>>
>> create table lpd (a int, b int, c varchar) partition by list(a);
>> create table lpd_d partition of lpd DEFAULT;
>>
>> \d+ lpd
>>  Table "public.lpd"
>>  Column |   Type| Collation | Nullable | Default | Storage  |
>> Stats target | Description
>> +---+---+--+
>> -+--+--+-
>>  a  | integer   |   |  | | plain
>> |  |
>>  b  | integer   |   |  | | plain
>> |  |
>>  c  | character varying |   |  | | extended
>> |  |
>> Partition key: LIST (a)
>> Partitions: lpd_d FOR VALUES IN (DEFAULT)
>>
>
>

Re: [HACKERS] Adding support for Default partition in partitioning

[Sven R. Kunze]

On 10.05.2017 17:59, Robert Haas wrote:

Well, I don't think it would be a HUGE problem, but I think the fact
that Amit chose to implement this with syntax similar to that of
Oracle is probably not a coincidence, but rather a goal, and I think
the readability problem that you're worrying about is really pretty
minor.  I think most people aren't going to subpartition their default
partition, and I think those who do will probably find the syntax
clear enough anyway.


I agree here.


Optional keywords may not be the root of ALL evil, but they're pretty
evil.  See my posting earlier on this same thread on this topic:

http://postgr.es/m/CA+TgmoZGHgd3vKZvyQ1Qx3e0L3n=voxy57mz9ttncvet-al...@mail.gmail.com

The issues here are more or less the same.


Ah, I see. I didn't draw the conclusion from the optionality of a 
keyword the other day but after re-reading your post, it's exactly the 
same issue.

Let's avoid optional keywords!

Sven

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Wed, May 10, 2017 at 12:12 PM, Alvaro Herrera
 wrote:
> I'm surprised that there is so much activity in this thread.  Is this
> patch being considered for pg10?

Of course not.  Feature freeze was a month ago.

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


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Alvaro Herrera]

I'm surprised that there is so much activity in this thread.  Is this
patch being considered for pg10?

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Wed, May 10, 2017 at 10:59 AM, Sven R. Kunze  wrote:
> You are definitely right. Changing it here would require to change it
> everywhere AND thus to loose syntax parity with Oracle.

Right.

> I am not in a position to judge this properly whether this would be a huge
> problem. Personally, I don't have an issue with that. But don't count me as
> most important opion on this.

Well, I don't think it would be a HUGE problem, but I think the fact
that Amit chose to implement this with syntax similar to that of
Oracle is probably not a coincidence, but rather a goal, and I think
the readability problem that you're worrying about is really pretty
minor.  I think most people aren't going to subpartition their default
partition, and I think those who do will probably find the syntax
clear enough anyway.   So I don't favor changing it.  Now, if there's
an outcry of support for your position then I'll stand aside but I
don't anticipate that.

>> So I guess I'm still in favor of the CREATE TABLE p1 PARTITION OF test
>> DEFAULT syntax, but if it ends up being AS DEFAULT instead, I can live
>> with that.
>
> Is to make it optional an option?

Optional keywords may not be the root of ALL evil, but they're pretty
evil.  See my posting earlier on this same thread on this topic:

http://postgr.es/m/CA+TgmoZGHgd3vKZvyQ1Qx3e0L3n=voxy57mz9ttncvet-al...@mail.gmail.com

The issues here are more or less the same.

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


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