Re: [HACKERS] Binary search in fmgr_isbuiltin() is a bottleneck.

[Jeevan Ladhe]

Hi Andres,

Another idea would be to have an array of FmgrBuiltin*, that we index by
> oid. That'd not be super small though, given that the space for function
> oids is sparse.
>
>
I totally agree here, as the oids are very much scattered having an
array is not feasible here.


> Thus what I've instead done is replacing the binary search in
> fmgr_isbuiltin() with a simplehash.h style hashtable. After that the
> lookup is still visible in the profile, but far less prominent.
>
> I'd like to move the hash creation out of fmgr_isbuiltin (to avoid
> having to check whether it's already created), but there's currently no
> convenient place to create the hash from.   Now that we don't rely on
> the sortedness of fmgrtab.c we could remove a few lines from
> Gen_fmgrtab.pl, but I don't quite see the advantage. If we were
> interested in a faster by-name lookup we could sort it by name, but
> that'd be better solved by another hashtable...
>

I looked into these patches.
Seems patch 004 is already committed, commit id:
791961f59b792fbd4f0a992d3ccab47298e79103

About patch 0005:
The patch still applies cleanly.
There are no failures in ‘make check’

+ /* TODO: it'd be better if this were done separately */
+ if (unlikely(oid2builtins == NULL))
  {
- int i = (high + low) / 2;
- const FmgrBuiltin *ptr = _builtins[i];
+ int i;

- if (id == ptr->foid)
- return ptr;
- else if (id > ptr->foid)
- low = i + 1;
- else
- high = i - 1;
+ oid2builtins = oid2builtins_create(TopMemoryContext,
+   fmgr_nbuiltins,
+   NULL);
+ for (i = 0; i < fmgr_nbuiltins; i++)
+ {
+ const FmgrBuiltin *ptr = _builtins[i];
+ bool found;
+
+ entry = oid2builtins_insert(oid2builtins, ptr->foid, );
+ Assert(!found);
+ entry->builtin = ptr;
+ }

As Andres has already pointed, may be we want to move above code in a
separate
function, and just call that function here in case the hash is not already
built.

Further I am thinking about doing some performance testing, Andres can you
please
point me how did you test it and what perf numbers you saw for this
particular patch(0005).

Regards,
Jeevan Ladhe

Re: [HACKERS] Optimise default partition scanning while adding new partition

[Jeevan Ladhe]

Thanks Amit for reviewing.

Patch looks fine to me.  By the way, why don't we just say "Can we skip
> scanning part_rel?" in the comment before the newly added call to
> PartConstraintImpliedByRelConstraint()?  We don't need to repeat the
> explanation of what it does at the every place we call it.
>

I have changed the comment.
PFA.

Regards,
Jeevan Ladhe


0001-Check-default-partitition-child-validation-scan-is.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

[HACKERS] Optimise default partition scanning while adding new partition

[Jeevan Ladhe]

Hi,

Commit 6f6b99d1335be8ea1b74581fc489a97b109dd08a introduced default
partitioning support. This commit added a new function
check_default_allows_bound(),
which checks if there exists a row in the default partition that would
belong to
the new partition being added. If it finds one, it throws an error. Before
taking
the decision to scan the default partition, this function checks if there
are
existing constraints on default partition that would imply the new partition
constraints, if yes it skips scanning the default partition, otherwise it
scans the
default partition and its children(if any). But, while doing so the current
code
misses the fact that there can be constraints on the child of default
partition
such that they would imply the constraints of the new partition being added,
and hence individual child scan can also be skipped.
Attached is the patch which does this.

This is previously discussed in default partitioning thread[1], and decision
was made that we can take this a separate patch rather than as a part of the
default partitioning support.

Amit Langote has a similar patch[2] for scanning the children of a
partitioned
table which is being attached as partition of another partitioned table.

[1]
https://www.postgresql.org/message-id/CA+TgmoZ8-q=2oahoxmvzbdnxi9g6i1idi4ozfkb67mk242d...@mail.gmail.com
[2]
https://www.postgresql.org/message-id/4cd13b03-846d-dc65-89de-1fd9743a3...@lab.ntt.co.jp

Regards,
Jeevan Ladhe


0001-Check-default-partitition-child-validation-scan-is.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 Sat, Sep 9, 2017 at 3:05 AM, Robert Haas <robertmh...@gmail.com> wrote:

> On Fri, Sep 8, 2017 at 10:08 AM, Jeevan Ladhe
> <jeevan.la...@enterprisedb.com> 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

[Jeevan Ladhe]

On Fri, Sep 8, 2017 at 6:46 AM, Robert Haas <robertmh...@gmail.com> wrote:

> On Thu, Sep 7, 2017 at 8:13 AM, Jeevan Ladhe
> <jeevan.la...@enterprisedb.com> 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

[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

[Jeevan Ladhe]

eries.
>
Comments below rather seem to be for the patch that extends default
partition
such that new partition can be added even when default partition exists.
This
is 0003 patch in V27.


>
> The commit message of this patch has following line, which no more applies
> to
> patch 0001. May be you want to remove this line or update the patch number.
> 3. This patch uses the refactored functions created in patch 0001
> in this series.
> Similarly the credit line refers to patch 0001. That too needs correction.
>

Fixed commit message.


>
> - * Also, invalidate the parent's relcache, so that the next rebuild will
> load
> - * the new partition's info into its partition descriptor.
> + * 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.
> Replacing "relcache" with "relcache entry" in the first sentence  may be a
> good
> idea, but is unrelated to this patch. I would leave that change aside and
> just
> add comment about default partition.
>

Agree. Fixed.


> +/*
> + * 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.
> + */
> +defaultPartOid = get_default_partition_oid(RelationGetRelid(parent));
> +if (OidIsValid(defaultPartOid))
> +CacheInvalidateRelcacheByRelid(defaultPartOid);
> Again, since we have access to the parent's relcache, we should get the
> default
> partition OID from relcache rather than catalogs.
>
>
This change is in heap.c, as I said above we would need to have a
different version of get_default_partition_oid() to address this.
Your thoughts?

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.


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

Regards,
Jeevan Ladhe

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

[Jeevan Ladhe]

On Sat, Sep 2, 2017 at 7:03 AM, Robert Haas <robertmh...@gmail.com> wrote:

> On Fri, Sep 1, 2017 at 3:19 PM, Robert Haas <robertmh...@gmail.com> wrote:
> > On Thu, Aug 31, 2017 at 8:53 AM, Jeevan Ladhe
> > <jeevan.la...@enterprisedb.com> 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] Default Partition for Range

[Jeevan Ladhe]

Hi Beena,


On Tue, Aug 22, 2017 at 5:22 PM, Beena Emerson <memissemer...@gmail.com>
wrote:

> On Tue, Aug 22, 2017 at 4:20 PM, Beena Emerson <memissemer...@gmail.com>
> wrote:
> > Hi Jeevan,
> >
> > On Tue, Aug 22, 2017 at 7:53 AM, Jeevan Ladhe
> > <jeevan.la...@enterprisedb.com> wrote:
> >>
> >>
> >> 4.
> >>  static List *
> >> -get_qual_for_range(PartitionKey key, PartitionBoundSpec *spec)
> >> +get_qual_for_range(Relation parent, PartitionBoundSpec *spec,
> >> +  bool for_default)
> >>  {
> >>
> >> The addition and the way flag ‘for_default’ is being used is very
> confusing.
> >> At this moment I am not able to think about a alternate solution to the
> >> way you have used this flag. But will try and see if I can come up with
> >> an alternate approach.
> >
> > Well, we need to skip the get_range_nulltest while collecting the qual
> > of other partition to make one for default. We could skip this flag
> > and remove the NullTest from the qual returned for each partition but
> > I am not sure if thats a good way to go about it.
> >
> >
>
> Please find attached a patch which removes the for_default flag from
> the get_qual_for_range function. This patch is just to show an idea
> and should be applied over my earlier patch. There could be a better
> way to do this. Let me know your opinion.
>
>
+
+ foreach (lc, list_tmp)
+ {
+ Node *n = (Node *) lfirst(lc);
+
+ if (IsA(n, NullTest))
+ {
+ list_delete(part_qual, n);
+ count++;
+ }
+ }
+

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

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

Regards,
Jeevan Ladhe

Re: [HACKERS] Default Partition for Range

[Jeevan Ladhe]

Hi Beena,

On Thu, Aug 17, 2017 at 4:59 PM, Beena Emerson 
wrote:

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

Thanks for rebasing.

Range partition review:

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

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

We can rephrase the comment as below:

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

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

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

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

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

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

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

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

Regards,
Jeevan


0001-Refactor-RelationBuildPartitionDesc.patch
Description: Binary data


0002-Add-support-for-default-partition-for-range.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 Fri, Aug 18, 2017 at 12:25 AM, Robert Haas <robertmh...@gmail.com> wrote:

> On Thu, Aug 17, 2017 at 6:24 AM, Jeevan Ladhe
> <jeevan.la...@enterprisedb.com> 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

[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 <jeevan.la...@enterprisedb.com
> 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
>> <jeevan.la...@enterprisedb.com> 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 

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
> <jeevan.la...@enterprisedb.com> 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 INCLUDI

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] Default Partition for Range

[Jeevan Ladhe]

Hi Robert, Beena,

On Wed, Aug 9, 2017 at 5:53 PM, Robert Haas <robertmh...@gmail.com> wrote:

> On Wed, Aug 9, 2017 at 8:18 AM, Rajkumar Raghuwanshi
> <rajkumar.raghuwan...@enterprisedb.com> wrote:
> > --difference in the description of default partition in case of list vs
> > range
> >
> > create table lp (a int) partition by list(a);
> > create table lp_d partition of lp DEFAULT;
> > postgres=# \d+ lp_d
> >Table "public.lp_d"
> >  Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target
> > | Description
> > +-+---+--+-+
> -+--+-
> >  a  | integer |   |  | | plain   |
> > |
> > Partition of: lp DEFAULT
> > Partition constraint:
> >
> > create table rp (a int) partition by range(a);
> > create table rp_d partition of rp DEFAULT;
> > postgres=# \d+ rp_d
> >Table "public.rp_d"
> >  Column |  Type   | Collation | Nullable | Default | Storage | Stats
> target
> > | Description
> > +-+---+--+-+
> -+--+-
> >  a  | integer |   |  | | plain   |
> > |
> > Partition of: rp DEFAULT
> > Partition constraint: true
>
> This looks like a problem with the default list partitioning patch.  I
> think "true" is what we expect to see here in both cases.
>

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

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

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

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

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

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

Please let me know your thoughts.

Regards,
Jeevan Ladhe

Re: [HACKERS] INSERT ON CONFLICT and partitioned tables

[Jeevan Ladhe]

Thanks Amit for addressing the comment.

The patch looks good to me. I have no more comments.
Verified that v2 patch applies cleanly and make check passes.

Thanks,
Jeevan Ladhe

Re: [HACKERS] INSERT ON CONFLICT and partitioned tables

[Jeevan Ladhe]

I applied the patch on latest master sources and the patch applies cleanly.
The documentation is built without errors.

We do not support following syntax for 'do nothing':

postgres=# insert into parted_conflict_test values (1, 'a') on conflict (b)
do nothing;
ERROR:  there is no unique or exclusion constraint matching the ON CONFLICT
specification

This limitation is because we do not support unique index on partitioned
table.
But, in that sense the following snippet of the documentation seems
misleading:

+   will cause an error if the conflict target is specified (see
+for more details).  That means it's not
+   possible to specify DO UPDATE as the alternative
+   action, because it requires the conflict target to be specified.
+   On the other hand, specifying DO NOTHING as the
+   alternative action works fine.
May be the last sentence can be rephrased as below:

"On the other hand, specifying DO NOTHING without target
as
an alternative action works fine."

Other than this patch looks good to me.

Regards,
Jeevan Ladhe



On Wed, Aug 2, 2017 at 10:26 AM, Amit Langote <langote_amit...@lab.ntt.co.jp
> wrote:

> Starting a new thread for a patch I posted earlier [1] to handle ON
> CONFLICT DO NOTHING when inserting into a partitioned table.  It's
> intended for PG 11 and so registered in the upcoming CF.
>
> Summary of the previous discussion and the patch for anyone interested:
>
> Currently, if an INSERT statement for a partitioned table mentions the ON
> CONFLICT clause, we error out immediately.  It was implemented that way,
> because it was thought that it could not be handled with zero support for
> defining indexes on partitioned tables.  Peter Geoghegan pointed out [2]
> that it's too restrictive a view.
>
> He pointed out that planner doesn't *always* expect indexes to be present
> on the table when ON CONFLICT is specified.  They must be present though
> if DO UPDATE action is requested, because one would need to also specify
> the exact columns on which conflict will be checked and those must covered
> by the appropriate indexes.  So, if the table is partitioned and DO UPDATE
> is specified, lack of indexes will result in an error saying that a
> suitable index is absent.  DO UPDATE action cannot be supported until we
> implement the feature to define indexes on partitioned tables.
>
> OTOH, the DO NOTHING case should go through the planner without error,
> because neither any columns need to be specified nor any indexes need to
> be present covering them.  So, DO NOTHING on partitioned tables might work
> after all.  Conflict can only be determined using indexes, which
> partitioned tables don't allow, so how?  Leaf partitions into which tuples
> are ultimately stored can have indexes defined on them, which can be used
> to check for the conflict.
>
> The patch's job is simple:
>
> - Remove the check in the parser that causes an error the moment the
>   ON CONFLICT clause is found.
>
> - Fix leaf partition ResultRelInfo initialization code so that the call
>   ExecOpenIndices() specifies 'true' for speculative, so that the
>   information necessary for conflict checking will be initialized in the
>   leaf partition's ResultRelInfo
>
> Thanks,
> Amit
>
> [1]
> https://www.postgresql.org/message-id/62be3d7a-08f6-5dcb-
> f5c8-a5b764ca96df%40lab.ntt.co.jp
>
> [2]
> https://www.postgresql.org/message-id/CAH2-Wzm10T%2B_PWVM4XO5zaknVbAXkOH9-
> JW3gRVPm1njLHck_w%40mail.gmail.com
>
>
> --
> 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] Default Partition for Range

[Jeevan Ladhe]

Hi Beena,

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

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

Regards,
Jeevan Ladhe

On Fri, Jul 14, 2017 at 3:28 PM, Beena Emerson <memissemer...@gmail.com>
wrote:

> On Tue, Jul 4, 2017 at 4:21 PM, Rahila Syed <rahilasye...@gmail.com>
> wrote:
> >
> > Hello Beena,
> >
> > Thanks for the updated patch. It passes the basic tests which I
> performed.
> >
> > Few comments:
> > 1. In   check_new_partition_bound(),
> >
> >> /* Default case is not handled here */
> >>if (spec->is_default)
> >>break;
> > The default partition check here can be performed in common for both
> range
> > and list partition.
>
> Removed the check here.
>
> >
> > 2. RANGE_DATUM_FINITE = 0, /* actual datum stored elsewhere */
> > +   RANGE_DATUM_DEFAULT,/* default */
> >
> > More elaborative comment?
>
> I am not sure what to add. Do you have something in mind?
>
> >
> > 3.  Inside make_one_range_bound(),
> >
> >>+
> >>+   /* datums are NULL for default */
> >>+   if (datums == NULL)
> >>+   for (i = 0; i < key->partnatts; i++)
> >>+   bound->content[i] = RANGE_DATUM_DEFAULT;
> >>+
> > IMO, returning bound at this stage will make code clearer as further
> > processing
> > does not happen for default partition.
>
> Done.
>
> >
> > 4.
> > @@ -549,6 +568,7 @@ RelationBuildPartitionDesc(Relation rel)
> > sizeof(RangeDatumContent
> > *));
> > boundinfo->indexes = (int *) palloc((ndatums + 1) *
> > sizeof(int));
> > +   boundinfo->default_index = -1;
> > This should also be done commonly for both default list and range
> partition.
>
> Removed the line here. Common allocation is done by Jeevan's patch.
>
> >
> > 5.
> >   if (!spec->is_default && partdesc->nparts > 0)
> > {
> > ListCell   *cell;
> >
> > Assert(boundinfo &&
> >boundinfo->strategy ==
> PARTITION_STRATEGY_LIST &&
> >(boundinfo->ndatums > 0 ||
> > partition_bound_accepts_nulls(boundinfo) ||
> > partition_bound_has_default(boundinfo)));
> > The assertion condition partition_bound_has_default(boundinfo) is
> rendered
> > useless
> > because of if condition change and can be removed perhaps.
>
> This cannot be removed.
> This check is required when this code is run for a non-default
> partition and default is the only existing partition.
>
> >
> > 6. I think its better to have following  regression tests coverage
> >
> > --Testing with inserting one NULL and other non NULL values for
> multicolumn
> > range partitioned table.
> >
> > postgres=# CREATE TABLE range_parted (
> > postgres(# a int,
> > postgres(# b int
> > postgres(# ) PARTITION BY RANGE (a, b);
> > CREATE TABLE
> > postgres=#
> > postgres=# CREATE TABLE part1 (
> > postgres(# a int NOT NULL CHECK (a = 1),
> > postgres(# b int NOT NULL CHECK (b >= 1 AND b <= 10)
> > postgres(# );
> > CREATE TABLE
> >
> > postgres=# ALTER TABLE range_parted ATTACH PARTITION part1 FOR VALUES
> FROM
> > (1, 1) TO (1, 10);
> > ALTER TABLE
> > postgres=# CREATE TABLE partr_def1 PARTITION OF range_parted DEFAULT;
> > CREATE TABLE
> >
> > postgres=# insert into range_parted values (1,NULL);
> > INSERT 0 1
> > postgres=# insert into range_parted values (NULL,9);
> > INSERT 0 1
>
> added.
>
>
>
>
>
> --
>
> Beena Emerson
>
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

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]

RROR,
> +(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.
>

Fixed.


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


I could not locate this hunk.

Regards,
Jeevan Ladhe

Refs:
[1] https://www.postgresql.org/message-id/CAOgcT0OARciE2X%
2BU0rjSKp9VuC279dYcCGkc3nCWKhHQ1_m2rw%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/35d68d49-555f-421a-99f8-185a44d085a4%40lab.ntt.co.jp

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

[Jeevan Ladhe]

changed this in
check_default_allows_bound() too.
I agree that there is still a room for debate here after this change too,
and
also this change reverts the suggestion by Ashutosh.

+ * previously cached default partition constraints; those
> constraints
> + * won't stand correct after addition(or even removal) of a
> partition.
>
> won't be correct after addition or removal
>

Done.


>
> + * allow any row that qualifies for this new partition. So, check
> if
> + * the existing data in the default partition satisfies this
> *would be*
> + * default partition constraint.
>
> check that the existing data in the default partition satisfies the
> constraint as it will exist after adding this partition
>

Done.


>
> + * Need to take a lock on the default partition, refer comment for
> locking
> + * the default partition in DefineRelation().
>
> I'd say: We must also lock the default partition, for the same reasons
> explained in DefineRelation().
>
> And similarly in the other places that refer to that same comment.
>

Done.


>
> +/*
> + * In case of the default partition, the constraint is of the form
> + * "!(result)" i.e. one of the following two forms:
> + * 1. NOT ((keycol IS NULL) OR (keycol = ANY (arr)))
> + * 2. NOT ((keycol IS NOT NULL) AND (keycol = ANY (arr))), where arr
> is an
> + * array of datums in boundinfo->datums.
> + */
>
> Does this survive pgindent?  You might need to surround the comment
> with dashes to preserve formatting.
>
> Yes, this din't survive pg_indent, but even adding dashes '--' did not make
the deal(may be I misunderstood the workaround), I have instead added
blank line in the bullets.

I think it would be worth adding a little more text this comment,
> something like this: Note that, in general, applying NOT to a
> constraint expression doesn't necessarily invert the set of rows it
> accepts, because NOT NULL is NULL.  However, the partition constraints
> we construct here never evaluate to NULL, so applying NOT works as
> intended.
>
> Added.


> + * Check whether default partition has a row that would fit the
> partition
> + * being attached by negating the partition constraint derived from
> the
> + * bounds. Since default partition is already part of the partitioned
> + * table, we don't need to validate the constraints on the partitioned
> + * table.
>
> Here again, I'd add to the end of the first sentence a parenthetical
> note, like this: ...from the bounds (the partition constraint never
> evaluates to NULL, so negating it like this is safe).
>
> Done.


> I don't understand the second sentence.  It seems to contradict the first
> one.
>

Fixed, I removed the second sentence.


> +extern List *get_default_part_validation_constraint(List
> *new_part_constaints);
>  #endif   /* PARTITION_H */
>
> There should be a blank line after the last prototype and before the
> #endif.
>
> +-- default partition table when it is being used in cahced plan.
>
> Typo.
>

Fixed.

Thanks,
Jeevan Ladhe

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

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 <robertmh...@gmail.com>
> wrote:
> >> On Thu, Jun 15, 2017 at 12:54 PM, Ashutosh Bapat
> >> <ashutosh.ba...@enterprisedb.com> 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] fix empty array expression in get_qual_for_list()

[Jeevan Ladhe]

On Mon, Jun 26, 2017 at 8:14 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Jeevan Ladhe <jeevan.la...@enterprisedb.com> writes:
> > In case of list partitioned table:
> > 1. If there is a partition accepting only null values and nothing else,
> then
> > currently the partition constraints for such a partition are constructed
> as
> > "((a IS NULL) OR (a = ANY (ARRAY[]::integer[])))".
> > I think there it is better to avoid constructing an empty array to avoid
> > executing ANY expression.
>
> Looks like a good idea, pushed with trivial stylistic adjustments.
>

Thanks Tom for taking care of this.


Regards,
Jeevan Ladhe

Re: [HACKERS] fix empty array expression in get_qual_for_list()

[Jeevan Ladhe]

Here is an example:

create table t1 ( a int) partition by list (a);
create table t11 partition of t1 for values in (null);

*Current constraints:*
postgres=# \d+ t11;
Table "public.t11"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
+-+---+--+-+-+--+-
 a  | integer |   |  | | plain   |
 |
Partition of: t1 FOR VALUES IN (NULL)
Partition constraint: ((a IS NULL) OR (a = ANY (ARRAY[]::integer[])))

*Constraints after fix:*
postgres=# \d+ t11;
Table "public.t11"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
+-+---+--+-+-+--+-
 a  | integer |   |  | | plain   |
 |
Partition of: t1 FOR VALUES IN (NULL)
Partition constraint: (a IS NULL)


Regards,
Jeevan Ladhe

On Mon, Jun 26, 2017 at 4:53 PM, Jeevan Ladhe <jeevan.la...@enterprisedb.com
> wrote:

> Hi,
>
> In case of list partitioned table:
> 1. If there is a partition accepting only null values and nothing else,
> then
> currently the partition constraints for such a partition are constructed as
> "((a IS NULL) OR (a = ANY (ARRAY[]::integer[])))".
> I think there it is better to avoid constructing an empty array to avoid
> executing ANY expression.
>
> 2.Also, we are constructing an expression using index 0 of arrays in
> PartitionKey since currently we have only one column for list partition in
> key,
> added an assert for this.
>
> PFA.
>
> Regards,
> Jeevan Ladhe
>

[HACKERS] fix empty array expression in get_qual_for_list()

[Jeevan Ladhe]

Hi,

In case of list partitioned table:
1. If there is a partition accepting only null values and nothing else, then
currently the partition constraints for such a partition are constructed as
"((a IS NULL) OR (a = ANY (ARRAY[]::integer[])))".
I think there it is better to avoid constructing an empty array to avoid
executing ANY expression.

2.Also, we are constructing an expression using index 0 of arrays in
PartitionKey since currently we have only one column for list partition in
key,
added an assert for this.

PFA.

Regards,
Jeevan Ladhe


fix_empty_arry_get_qual_for_list.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]

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 <robertmh...@gmail.com> wrote:

> On Wed, Jun 14, 2017 at 8:02 AM, Jeevan Ladhe
> <jeevan.la...@enterprisedb.com> 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

[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] fix possible optimizations in ATExecAttachPartition()

[Jeevan Ladhe]

On Wed, Jun 14, 2017 at 2:12 AM, Robert Haas <robertmh...@gmail.com> wrote:

> On Tue, Jun 13, 2017 at 5:22 AM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
> > Yeah, I was thinking the same while writing the patch posted on the
> thread
> > "A bug in mapping attributes in ATExecAttachPartition()" [1].  That patch
> > adds the break you mention in 2, but didn't do anything about point 1.
> >
> > In any case, +1 to your patch.  I'll rebase if someone decides to commit
> > it first.
>
> If the patch I posted in
> https://www.postgresql.org/message-id/CA%2BTgmoYmW9VwCWDpe7eXUxeKmAKOxm
> g8itgFkB5UTQTq4SnTjQ%40mail.gmail.com
> gets committed, all of this code will be gone entirely, so this will
> be moot.  If we decide to repair the existing broken logic rather than
> ripping it out entirely then this is probably a good idea, but I hope
> that's not what happens.
>

That seems a better option to me too.
+1

Regards,
Jeevan Ladhe

[HACKERS] fix possible optimizations in ATExecAttachPartition()

[Jeevan Ladhe]

Hi,

I was doing some testing for my default partitioning work, and I realized
that
there seem to be a some optimization possible (and I think we should really
have
this optimization) for logic around handling the "key IS NOT NULL"
constraints
followed by predicate_implied_by() call.

1. Consider, if predicate_implied_by() returns false, in that case
skip_validate
is left set to false, and we really do not want to do lots of stuff followed
this decision just to prove that the scan cannot be avoided(which is already
decided in this case) if there is no NOT NULL constraint on the partition
key
column.

2. Further, if we have already decided not to skip the scan, we should
really have
a break in following if block:

if (!partition_accepts_null &&
(partattno == 0 ||
!bms_is_member(partattno, not_null_attrs)))
skip_validate = false;


I have made above changes in the attached patch.
PFA, and let me know if I am missing something here.

Regards,
Jeevan Ladhe


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

[Jeevan Ladhe]

ild of
default partition(which I dint like much, but I don't see a way out here).
Still
the error message does not display the default partition name in error as of
check_default_allows_bound(). May be to address this and keep the messages
exactly similar we can copy the name of parent default partition in a field
in
AlteredTableInfo structure, which looks very ugly to me. I am open to
suggestions here.

*3. changes to default_partition_v19.patch:*

The default partition constraint are no more built using the negator of the
operator, instead it is formed simply as NOT of the existing partitions:
e.g.:
if a null accepting partition already exists:
NOT ((keycol IS NULL) OR (keycol = ANY (arr)))
if a null accepting partition does not exists:
NOT ((keycol IS NOT NULL) AND (keycol = ANY (arr))), where arr is an array
of
datums in boundinfo->datums.

Added tests for prepared statment.

Renamed RelationGetDefaultPartitionOid() to get_default_partition_oid().

+ if (partqualstate && ExecCheck(partqualstate, econtext))
+ ereport(ERROR,
+ (errcode(ERRCODE_CHECK_VIOLATION),
+ errmsg("new partition constraint for default partition \"%s\" would be
violated by some row",
+   RelationGetRelationName(default_rel;
Per Ashutosh's suggestion[2], changed the error code to
ERRCODE_INVALID_OBJECT_DEFINITION.
Also, per Robert's suggestion[3], changed following message:
"new partition constraint for default partition \"%s\" would be violated by
some row"
to
"updated partition constraint for default partition \"%s\" would be
violated by some row"

Some other cosmetic changes.

Apart from this, I am exploring the tests in relation with NOT NULL
constraint
embedded within an expression. Will update on that shortly.

[1]
http://www.postgresql-archive.org/A-bug-in-mapping-attributes-in-ATExecAttachPartition-td5965298.html
[2]
http://www.postgresql-archive.org/Adding-support-for-Default-partition-in-partitioning-td5946868i120.html#a5965277
[3]
http://www.postgresql-archive.org/Adding-support-for-Default-partition-in-partitioning-tp5946868p5965599.html

Regards,
Jeevan Ladhe


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

> On Wed, Jun 7, 2017 at 2:08 AM, Jeevan Ladhe
> <jeevan.la...@enterprisedb.com> 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 

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
> <ashutosh.ba...@enterprisedb.com> wrote:
> > On Wed, Jun 7, 2017 at 2:08 AM, Jeevan Ladhe
> > <jeevan.la...@enterprisedb.com> 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

[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

[Jeevan Ladhe]

t.

 }
> +
>  ;
> unnecessary extra line.
>

Removed.


> +/*
> + * The default partition bound does not have any datums to be
> + * transformed, return the new bound.
> + */
> Probably not needed.
>

Removed.


>
> +if (spec->is_default && (strategy ==
> PARTITION_STRATEGY_LIST ||
> + strategy ==
> PARTITION_STRATEGY_RANGE))
> +{
> +appendStringInfoString(buf, "DEFAULT");
> +break;
> +}
> +
> What happens if strategy is something other than RANGE or LIST. For that
> matter
> why not just LIST? Possibly you could write this as
> +if (spec->is_default)
> +{
> +Assert(strategy == PARTITION_STRATEGY_LIST);
> +appendStringInfoString(buf, "DEFAULT");
> +break;
> +}
>

Done.


> @@ -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 partition bound specification */
>  else if (TailMatches3("PARTITION", "OF", MatchAny))
> -COMPLETE_WITH_CONST("FOR VALUES");
> +COMPLETE_WITH_LIST2("FOR VALUES", "DEFAULT");
> Do we include psql tab completion in the main feature patch? I have not
> seen
> this earlier. But appreciate taking care of these defails.
>

I am not sure about this. If needed I can submit a patch to take care of
this later, but
as of now I have not removed this from the patch.

+char *ExecBuildSlotValueDescription(Oid reloid,
> needs an "extern" declaration.
>

Per one of the comment[1] given by Amit Langote, I have removed a call to
ExecBuildSlotValueDescription(), and this was a leftover, I cleaned it up.

[1]https://www.postgresql.org/message-id/7c758a6b-107e-7c82-
0d3c-3af7965cad3f%40lab.ntt.co.jp

Regards,
Jeevan Ladhe


default_partition_v19.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]

> 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

[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] BEFORE trigger can cause undetected partition constraint violation

[Jeevan Ladhe]

I tried to debug this, and I see that while the target partition index is
correctly
found in ExecInsert(), somehow the resultRelInfo->ri_PartitionCheck is NIL,
this
is extracted from array mstate->mt_partitions.

This prevents execution of constraints in following code snippet in
ExecInsert():

/*
* Check the constraints of the tuple
*/
if (resultRelationDesc->rd_att->constr || resultRelInfo->ri_PartitionCheck)
ExecConstraints(resultRelInfo, slot, estate);

I couldn't debug it further today.

Regards,
Jeevan Ladhe

On Fri, Jun 2, 2017 at 1:21 AM, Robert Haas <robertmh...@gmail.com> wrote:

> I just discovered that a BEFORE trigger can allow data into a
> partition that violates the relevant partition constraint.  This is
> bad.
>
> Here is an example:
>
> rhaas=# create or replace function t() returns trigger as $$begin
> new.a := 2; return new; end$$ language plpgsql;
> CREATE FUNCTION
> rhaas=# create table foo (a int, b text) partition by list (a);
> CREATE TABLE
> rhaas=# create table foo1 partition of foo for values in (1);
> CREATE TABLE
> rhaas=# create trigger x before insert on foo1 for each row execute
> procedure t();
> CREATE TRIGGER
> rhaas=# insert into foo values (1, 'hi there');
> INSERT 0 1
> rhaas=# select tableoid::regclass, * from foo;
>  tableoid | a |b
> --+---+--
>  foo1 | 2 | hi there
> (1 row)
>
> That row violates the partition constraint, which requires that a = 1.
> You can see that by trying to insert the same row into the partition
> directly:
>
> rhaas=# insert into foo1 values (2, 'hi there');
> ERROR:  new row for relation "foo1" violates partition constraint
> DETAIL:  Failing row contains (2, hi there).
>
> --
> 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 <memissemer...@gmail.com>
wrote:

> On Wed, May 31, 2017 at 8:13 AM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> 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

[Jeevan Ladhe]

Thanks Amit for your comments.

On 31-May-2017 6:03 AM, "Amit Langote" <langote_amit...@lab.ntt.co.jp>
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

[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 <jeevan.la...@enterprisedb.com
> 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 <memissemer...@gmail.com>
> wrote:
>
>> On Mon, May 29, 2017 at 9:33 PM, Jeevan Ladhe
>> <jeevan.la...@enterprisedb.com> 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 <memissemer...@gmail.com>
wrote:

> On Mon, May 29, 2017 at 9:33 PM, Jeevan Ladhe
> <jeevan.la...@enterprisedb.com> 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] fix side-effect in get_qual_for_list()

[Jeevan Ladhe]

On Mon, May 29, 2017 at 11:55 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:

> Jeevan Ladhe <jeevan.la...@enterprisedb.com> writes:
> > I have rebased the patch on recent commit.
>
> Pushed with some further tweaking.
>

Thanks Tom for taking care of this.

Regards,
Jeevan Ladhe

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]
<http://www.mail-archive.com/pgsql-hackers@postgresql.org/msg315490.html>
 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 <jeevan.la...@enterprisedb.com
> 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] fix side-effect in get_qual_for_list()

[Jeevan Ladhe]

Hi,

I have rebased the patch on recent commit.

With recent commits, some of the hunks in the v2 patch related to
castNode, are not needed.

PFA.

Regards,
Jeevan Ladhe

On Sat, May 27, 2017 at 1:16 AM, Jeevan Ladhe <jeevan.la...@enterprisedb.com
> wrote:

> Hi Ashutosh,
>
> Thanks for catching this. For now this isn't a problem since
>> generate_partition_qual() is crafting PartitionBoundInfo which it
>> doesn't use anywhere else. But if the function gets used where the
>> PartitionBoundSpec is being used somewhere else as well.
>
>
> Yes, this behavior currently does not affect adversely, but I think this
> function is quite useful for future enhancements and should be fixed.
>
> While you are
>> at it, can we use castNode() in place of
>> PartitionBoundSpec *spec = (PartitionBoundSpec *) bound; Or do you
>> think it should be done separately?
>>
>
> I have made this change at couple of places applicable.
>
> PFA.
>
> Regards,
> Jeevan Ladhe
>


fix_listdatums_get_qual_for_list_v3.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] Default Partition for Range

[Jeevan Ladhe]

Hi Beena,

I went through your patch, and here are some of my comments:

- For generating a qual for default range partition, instead of scanning
for all
the children and collecting all the boundspecs, how about creating and
negating
an expression from the lists of lowerdatums and upperdatums in boundinfo.

- Wrong comment:
+ int default_index; /* Index of the default list partition. */

- Suggested by Robert earlier on default list partitioning thread, instead
of
abbreviating is_def/found_def use is(found)_default etc.

- unrelated change:
- List   *all_parts;
+ List   *all_parts;

- typo: partiton should be partition, similar typo at other places too.
+  * A non-default range partiton table does not currently allow partition
keys

- Useless hunk for this patch:
- Oiddefid;
+ Oid defid;

- better to use IsA here, instead of doing explicit check:
+ bound->content[i] = (datum->type == T_DefElem)
+ ? RANGE_DATUM_DEFAULT

- It is better to use head of either lowerdatums or upperdatums list to
verify
if its a default partition and get rid of the constant PARTITION_DEFAULT
altogether.

+ {
+ /*
+ * If the partition is the default partition switch back to
+ * PARTITION_STRATEGY_RANGE
+ */
+ if (spec->strategy == PARTITION_DEFAULT)
+ {
+ is_def = true;
+ result_spec->strategy = PARTITION_STRATEGY_RANGE;
+ }

- I am sorry, but I could not understand following hunk. Does this change
really
belongs to this patch? If not, it will be better to handle it separately.

@@ -2242,33 +2387,16 @@ get_partition_for_tuple(PartitionDispatch *pd,
  ecxt->ecxt_scantuple = slot;
  FormPartitionKeyDatum(parent, slot, estate, values, isnull);

- if (key->strategy == PARTITION_STRATEGY_RANGE)
+ if (key->strategy == PARTITION_STRATEGY_LIST && isnull[0])
  {
  /*
- * Since we cannot route tuples with NULL partition keys through
- * a range-partitioned table, simply return that no partition
- * exists
+ * A null partition key is only acceptable if null-accepting list
+ * partition exists.
  */
- for (i = 0; i < key->partnatts; i++)
- {
- if (isnull[i])
- {
- *failed_at = parent;
- *failed_slot = slot;
- result = -1;
- goto error_exit;
- }
- }
+ if (partition_bound_accepts_nulls(partdesc->boundinfo))
+ cur_index = partdesc->boundinfo->null_index;

- Change not related to this patch:
- List   *all_parts;
+ List   *all_parts;

- In the function get_qual_from_partbound() is_def is always going to be
false
for range partition, the function get_qual_for_range can be directly passed
false instead.

- Following comment for function get_qual_for_range_default() implies that
this
function returns bool, but the actually it returns a List.
+ *
+ * If DEFAULT is the only partiton for the table then this returns TRUE.
+ *


Regards,
Jeevan Ladhe

On Wed, May 24, 2017 at 12:52 AM, Beena Emerson <memissemer...@gmail.com>
wrote:

> Hello,
>
> On Mon, May 22, 2017 at 11:29 AM, Ashutosh Bapat <
> ashutosh.ba...@enterprisedb.com> wrote:
>
>> On Mon, May 22, 2017 at 7:27 AM, Beena Emerson <memissemer...@gmail.com>
>> wrote:
>> Would it be more readable if this reads as NOT
>> (constraint_for_partition_1 || constraint_for_partition_2 ||
>> constraint_for_partition_3)? That way, the code to create
>> constraint_for_partition_n can be reused and there's high chance that
>> we will end up with consistent constraints?
>>
>
> PFA the patch which gives the default partition constraint as you have
> suggested.
>
>
>>
>> >
>> > It still needs more work:
>> > 1. Handling addition of new partition after default, insertion of data,
>> few
>> > more bugs
>> > 2. Documentation
>> > 3. Regression tests
>> >
>>
>> I think, the default partition support for all the strategies
>> supporting it should be a single patch since most of the code will be
>> shared?
>>
>>
> Dependency on list default patch:
> gram.y : adding the syntax
> partition.c:
> - default_index member in PartitionBoundInfoData;
> - check_new_partition_bound : the code for adding a partition after
> default has been completely reused.
> - isDefaultPartitionBound function is used.
>
> The structures are same  but the handling of list and range is very
> different and the code mostly has  the switch case construct to handle the
> 2 separately. I feel it could be taken separately.
>
> As suggested in the default list thread, I have created
> a partition_bound_has_default macro and avoided usage of has_default in
> range code. This has to be used for list as well.
> Another suggestion for list which has to be implemented in this patch in
> removal of PARTITION_DEFAULT. Ii have not done this in this version.
>
> --
>
> 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]

>
> 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 <memissemer...@gmail.com>
wrote:

> On Thu, May 25, 2017 at 3:03 PM, Jeevan Ladhe
> <jeevan.la...@enterprisedb.com> 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] fix side-effect in get_qual_for_list()

[Jeevan Ladhe]

Hi Ashutosh,

Thanks for catching this. For now this isn't a problem since
> generate_partition_qual() is crafting PartitionBoundInfo which it
> doesn't use anywhere else. But if the function gets used where the
> PartitionBoundSpec is being used somewhere else as well.


Yes, this behavior currently does not affect adversely, but I think this
function is quite useful for future enhancements and should be fixed.

While you are
> at it, can we use castNode() in place of
> PartitionBoundSpec *spec = (PartitionBoundSpec *) bound; Or do you
> think it should be done separately?
>

I have made this change at couple of places applicable.

PFA.

Regards,
Jeevan Ladhe


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

[HACKERS] fix side-effect in get_qual_for_list()

[Jeevan Ladhe]

Hi,

While working on one of the crash reported on default partition for list
partitioning table[1] I found some strange behavior in get_qual_for_list()
while
I tried to call it from the new code I wrote for default partition.

After debugging, I noticed that the function get_qual_for_list() is
implicitly
manipulating the (PartitionBoundSpec) spec->listdatums list. AFAICU, this
manipulation is needed just to construct a list of datums to be passed to
ArrayExpr, and this should be done without manipulating the original list.
The function name is get_qual_for_list(), which implies that this function
returns something and does not modify anything.

I have made this change in attached patch, as I think this is useful for
future
developments, as there may be a need in future to call get_qual_for_list()
from
other places, and the caller might not expect that PartitionBoundSpec gets
modified.

PFA.

[1] 
*https://www.postgresql.org/message-id/flat/CAOgcT0PLPge%3D5U6%3DGU5SnC3_8yutCbWWOiUva3Cw94M9zpbvgQ%40mail.gmail.com
<https://www.postgresql.org/message-id/flat/CAOgcT0PLPge%3D5U6%3DGU5SnC3_8yutCbWWOiUva3Cw94M9zpbvgQ%40mail.gmail.com>*

Regards,
Jeevan Ladhe


fix_listdatums_get_qual_for_list.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]

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

[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

[HACKERS] unreachable code in partition.c

[Jeevan Ladhe]

Hi,

Following code in function get_qual_for_list(partition.c) is not reachable.

else
result = list_make1(opexpr);

Attached is the patch that removes this dead code.

Regards,
Jeevan Ladhe


partition_remove_dead_code.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] fix hard-coded index in make_partition_op_expr

[Jeevan Ladhe]

On Thu, May 18, 2017 at 12:13 AM, Robert Haas  wrote:
>
> Agreed.  Committed your patch.
>

 Thanks Robert!

Re: [HACKERS] remove unnecessary flag has_null from PartitionBoundInfoData

[Jeevan Ladhe]

> I committed this with fixes for those issues, plus I renamed the macro
> to partition_bound_accepts_nulls, which I think is more clear.


Thanks Robert.

[HACKERS] remove unnecessary flag has_null from PartitionBoundInfoData

[Jeevan Ladhe]

Hi,

As discussed in default partition thread[1]
<https://www.postgresql.org/message-id/CAFjFpRcvD-jf6z6-5mjx4tm-8NSj8Wnh%2B6%3DL6OB9Qy2yKX0HCg%40mail.gmail.com>,
here is the patch to remove
has_null from PartitionBoundInfoData structure.
Basically flag has_null is not needed and null_index can be checked if the
current bound is having a null value or not.

For simplicity of future use, in attached patch I have introduced a macro
that
would return TRUE if the given bound has null.

PFA.

[1].
https://www.postgresql.org/message-id/CAFjFpRcvD-jf6z6-5mjx4tm-8NSj8Wnh%2B6%3DL6OB9Qy2yKX0HCg%40mail.gmail.com

Regards,
Jeevan Ladhe


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

[HACKERS] fix hard-coded index in make_partition_op_expr

[Jeevan Ladhe]

Hi,

While browsing through the partitioning code, I noticed that a recent commit
f8bffe9e6d700fd34759a92e47930ce9ba7dcbd5, which fixes multi-column range
partitioning constraints, introduced a function make_partition_op_expr, that
takes keynum as a input parameter to identify the index of the partition
key.
In case of range partition we can have multiple partition keys but for list
partition we have only one. Considering that, I think following code does
not
cause any side-effect logically(and may be a oversight while moving the code
from function get_qual_for_list to this function):

saopexpr->inputcollid = key->partcollation[0];
saopexpr->args = list_make2(arg1, arg2);

But as we have keynum now, should we be using it to index
key->partcollation,
instead of a hard coded '0'.

I tried to look around in list partitioning and hard coded '0' is used at
all
the places, but that code is either list specific or does not have
availability
of partitioned key index.

Attached patch does this small change in make_partition_op_expr.
Another way is to, have an Assert in case of PARTITION_STRATEGY_LIST:
Assert(keynum != 0);

PFA.

Regards,
Jeevan Ladhe


fix_indexing_make_partition_op_expr_v1.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 <robertmh...@gmail.com>
> wrote:
> > On Tue, May 16, 2017 at 8:57 AM, Jeevan Ladhe
> > <jeevan.la...@enterprisedb.com> 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]
<https://www.postgresql.org/message-id/CAOgcT0NUUQXWRXmeVKkYTDQvWoKLYRMiPbc89ua6i_gG8FPDmA%40mail.gmail.com>,
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

[Jeevan Ladhe]

On Fri, May 12, 2017 at 7:34 PM, Beena Emerson <memissemer...@gmail.com>
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

[Jeevan Ladhe]

Hi Rahila,

On Thu, May 11, 2017 at 7:37 PM, Rahila Syed <rahilasye...@gmail.com> 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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.

[Jeevan Ladhe]

On Thu, May 11, 2017 at 4:47 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

>
> We add PARTITION OF clause for a table which is partition, so if the
> parent is not present while restoring, the restore is going to fail.


+1
But, similarly for inheritance if we dump a child table, it's dump is
broken as
of today. When we dump a child table we append "inherits(parenttab)" clause
at
the end of the DDL. Later when we try to restore this table on a database
not
having the parenttab, the restore fails.
So, I consider this as a bug.

Consider following example:

postgres=# create table tab1(a int, b int);
CREATE TABLE
postgres=# create table tab2(c int, d char) inherits(tab1);
CREATE TABLE
postgres=# \! pg_dump postgres -t tab2 > a.sql
postgres=# create database bkdb;
CREATE DATABASE
postgres=# \! psql bkdb < a.sql
SET
SET
SET
SET
SET
SET
SET
SET
SET
SET
SET
ERROR:  relation "tab1" does not exist
ERROR:  relation "tab2" does not exist
ERROR:  relation "tab2" does not exist
invalid command \.
postgres=#

Regards,
Jeevan Ladhe

Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.

[Jeevan Ladhe]

Hi Amit, Ashutosh,

On Tue, May 9, 2017 at 3:29 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> On Tue, May 9, 2017 at 3:13 PM, Amit Langote
> <langote_amit...@lab.ntt.co.jp> wrote:
> > On 2017/05/09 17:21, Jeevan Ladhe wrote:
> >> On Tue, May 9, 2017 at 12:43 PM, amul sul <sula...@gmail.com> wrote:
> >>> Current pg_dump --exclude-table option excludes partitioned relation
> >>> and dumps all its child relations and vice versa for --table option,
> which
> >>> I think is incorrect.
> >>>
> >>> In this case we might need to explore all partitions and exclude or
> include
> >>> from dump according to given pg_dump option, attaching WIP patch
> proposing
> >>> same fix.   Thoughts/Comments?
> >>
> >> Also, I can see similar issue exists with inheritance.
> >> In attached patch, I have extended Amul's original patch to address the
> >> inheritance dumping issue.
> >
> > Perhaps, it will be better not to touch the regular inheritance tables in
> > this patch.
>
> Yeah, I think it's fine if parent gets dumped without one or more of
> its children, that's user's choice when it used a certain pattern.
> Problematic case might be when we dump a child without its parent and
> have INHERITS clause there. pg_restore would throw an error. But in
> case that problem exists it's very old and should be fixed separately.


I agree that this should be taken as a separate fix, rather than taking it
with
partition.

Regards,
Jeevan Ladhe

Re: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partition table handling.

[Jeevan Ladhe]

In my last email by mistake I attached Amul's patch itself.
Please find attached patch extending the fix to inheritance relations.

Regards,
Jeevan Ladhe

On Tue, May 9, 2017 at 1:51 PM, Jeevan Ladhe <jeevan.la...@enterprisedb.com>
wrote:

> Hi,
>
> On Tue, May 9, 2017 at 12:43 PM, amul sul <sula...@gmail.com> wrote:
>
>> Hi,
>>
>> Current pg_dump --exclude-table option excludes partitioned relation
>> and dumps all its child relations and vice versa for --table option, which
>> I think is incorrect.
>>
>> In this case we might need to explore all partitions and exclude or
>> include
>> from dump according to given pg_dump option, attaching WIP patch proposing
>> same fix.   Thoughts/Comments?
>>
>> Regards,
>> Amul
>>
>
> +1.
>
> Also, I can see similar issue exists with inheritance.
> In attached patch, I have extended Amul's original patch to address the
> inheritance dumping issue.
>
> Regards,
> Jeevan Ladhe
>
>


pg_dump_fix_for_partition_and_inheritance_wip.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] Bug in pg_dump --table and --exclude-table for declarative partition table handling.

[Jeevan Ladhe]

Hi,

On Tue, May 9, 2017 at 12:43 PM, amul sul <sula...@gmail.com> wrote:

> Hi,
>
> Current pg_dump --exclude-table option excludes partitioned relation
> and dumps all its child relations and vice versa for --table option, which
> I think is incorrect.
>
> In this case we might need to explore all partitions and exclude or include
> from dump according to given pg_dump option, attaching WIP patch proposing
> same fix.   Thoughts/Comments?
>
> Regards,
> Amul
>

+1.

Also, I can see similar issue exists with inheritance.
In attached patch, I have extended Amul's original patch to address the
inheritance dumping issue.

Regards,
Jeevan Ladhe


pg_dump_fix_for_partition_and_inheritance.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 Robert,

Thanks for your explnation.

On Mon, May 8, 2017 at 9:56 PM, Robert Haas <robertmh...@gmail.com> wrote:

> On Thu, May 4, 2017 at 4:28 PM, Jeevan Ladhe
> <jeevan.la...@enterprisedb.com> wrote:
> > While reviewing the code I was trying to explore more cases, and I here
> > comes an
> > open question to my mind:
> > should we allow the default partition table to be partitioned further?
>
> I think yes.  In general, you are allowed to partition a partition,
> and I can't see any justification for restricting that for default
> partitions when we allow it everywhere else.
>
> > If we allow it(as in the current case) then observe following case,
> where I
> > have defined a default partitioned which is further partitioned on a
> > different
> > column.
> >
> > postgres=# CREATE TABLE test ( a int, b int, c int) PARTITION BY LIST
> (a);
> > CREATE TABLE
> > postgres=# CREATE TABLE test_p1 PARTITION OF test FOR VALUES IN(4, 5, 6,
> 7,
> > 8);
> > CREATE TABLE
> > postgres=# CREATE TABLE test_pd PARTITION OF test DEFAULT PARTITION BY
> > LIST(b);
> > CREATE TABLE
> > postgres=# INSERT INTO test VALUES (20, 24, 12);
> > ERROR:  no partition of relation "test_pd" found for row
> > DETAIL:  Partition key of the failing row contains (b) = (24).
> >
> > Note, that it does not allow inserting the tuple(20, 24, 12) because
> though
> > a=20
> > would fall in default partition i.e. test_pd, table test_pd itself is
> > further
> > partitioned and does not have any partition satisfying b=24.
>
> Right, that looks like correct behavior.  You would have gotten the
> same result if you had tried to insert into test_pd directly.
>
> > Further if I define a default partition for table test_pd, the the tuple
> > gets inserted.
>
> That also sounds correct.
>
> > Doesn't this sound like the whole purpose of having DEFAULT partition on
> > test
> > table is defeated?
>
> Not to me.  It's possible to do lots of silly things with partitioned
> tables.  For example, one case that we talked about before is that you
> can define a range partition for, say, VALUES (0) TO (100), and then
> subpartition it and give the subpartitions bounds which are outside
> the range 0-100.  That's obviously silly and will lead to failures
> inserting tuples, but we chose not to try to prohibit it because it's
> not really broken, just useless.  There are lots of similar cases
> involving other features.  For example, you can apply an inherited
> CHECK (false) constraint to a table, which makes it impossible for
> that table or any of its children to ever contain any rows; that is
> probably a dumb configuration.  You can create two unique indexes with
> exactly the same definition; unless you're creating a new one with the
> intent of dropping the old one, that doesn't make sense.  You can
> define a trigger that always throws an ERROR and then another trigger
> which runs later that modifies the tuple; the second will never be run
> because the first one will always kill the transaction before we get
> there.  Those things are all legal, but often unuseful.  Similarly
> here.  Defining a default list partition and then subpartitioning it
> by list is not likely to be a good schema design, but it doesn't mean
> we should try to disallow it.  It is important to distinguish between
> things that are actually *broken* (like a partitioning configuration
> where the tuples that can be inserted into a partition manually differ
> from the ones that are routed to it automatically) and things that are
> merely *lame* (like creating a multi-level partitioning hierarchy when
> a single level would have done the job just as well).  The former
> should be prevented by the code, while the latter is at most a
> documentation issue.


I agree with you that it is a user perspective on how he decides to do
partitions of already partitioned table, and also we should have a
demarcation between things to be handled by code and things to be
left as common-sense or ability to define a good schema.

I am ok with current behavior, provided we have atleast one-lineer in
documentation alerting the user that partitioning the default partition will
limit the ability of routing the tuples that do not fit in any other
partitions.

Regards,
Jeevan Ladhe

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

[Jeevan Ladhe]

Hi Rahila,

I am not able add a new partition if default partition is further
partitioned
with default partition.

Consider example below:

postgres=# CREATE TABLE test ( a int, b int, c int) PARTITION BY LIST (a);
CREATE TABLE
postgres=# CREATE TABLE test_p1 PARTITION OF test FOR VALUES IN(4, 5, 6, 7,
8);
CREATE TABLE
postgres=# CREATE TABLE test_pd PARTITION OF test DEFAULT PARTITION BY
LIST(b);
CREATE TABLE
postgres=# CREATE TABLE test_pd_pd PARTITION OF test_pd DEFAULT;
CREATE TABLE
postgres=# INSERT INTO test VALUES (20, 24, 12);
INSERT 0 1
*postgres=# CREATE TABLE test_p2 PARTITION OF test FOR VALUES IN(15);*
*ERROR:  could not open file "base/12335/16420": No such file or directory*


Thanks,
Jeevan Ladhe

On Fri, May 5, 2017 at 11:55 AM, Rajkumar Raghuwanshi <
rajkumar.raghuwan...@enterprisedb.com> wrote:

> Hi Rahila,
>
> pg_restore is failing for default partition, dump file still storing old
> syntax of default partition.
>
> create table lpd (a int, b int, c varchar) partition by list(a);
> create table lpd_d partition of lpd DEFAULT;
>
> create database bkp owner 'edb';
> grant all on DATABASE bkp to edb;
>
> --take plain dump of existing database
> \! ./pg_dump -f lpd_test.sql -Fp -d postgres
>
> --restore plain backup to new database bkp
> \! ./psql -f lpd_test.sql -d bkp
>
> psql:lpd_test.sql:63: ERROR:  syntax error at or near "DEFAULT"
> LINE 2: FOR VALUES IN (DEFAULT);
>^
>
>
> vi lpd_test.sql
>
> --
> -- Name: lpd; Type: TABLE; Schema: public; Owner: edb
> --
>
> CREATE TABLE lpd (
> a integer,
> b integer,
> c character varying
> )
> PARTITION BY LIST (a);
>
>
> ALTER TABLE lpd OWNER TO edb;
>
> --
> -- Name: lpd_d; Type: TABLE; Schema: public; Owner: edb
> --
>
> CREATE TABLE lpd_d PARTITION OF lpd
> FOR VALUES IN (DEFAULT);
>
>
> ALTER TABLE lpd_d OWNER TO edb;
>
>
> Thanks,
> Rajkumar
>

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

[Jeevan Ladhe]

While reviewing the code I was trying to explore more cases, and I here
comes an
open question to my mind:
should we allow the default partition table to be partitioned further?

If we allow it(as in the current case) then observe following case, where I
have defined a default partitioned which is further partitioned on a
different
column.

postgres=# CREATE TABLE test ( a int, b int, c int) PARTITION BY LIST (a);
CREATE TABLE
postgres=# CREATE TABLE test_p1 PARTITION OF test FOR VALUES IN(4, 5, 6, 7,
8);
CREATE TABLE
postgres=# CREATE TABLE test_pd PARTITION OF test DEFAULT PARTITION BY
LIST(b);
CREATE TABLE
postgres=# INSERT INTO test VALUES (20, 24, 12);
ERROR:  no partition of relation "test_pd" found for row
DETAIL:  Partition key of the failing row contains (b) = (24).

Note, that it does not allow inserting the tuple(20, 24, 12) because though
a=20
would fall in default partition i.e. test_pd, table test_pd itself is
further
partitioned and does not have any partition satisfying b=24.
Further if I define a default partition for table test_pd, the the tuple
gets inserted.

Doesn't this sound like the whole purpose of having DEFAULT partition on
test
table is defeated?

Any views?

Regards,
Jeevan Ladhe

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

[Jeevan Ladhe]

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 <rahilasye...@gmail.com>
> 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] warning in twophase.c

[Jeevan Ladhe]

I have also been seeing this warning lately, I had tried to check git log,
and
seems like following commit 546c13e11b29 by Simon has introduced it.

But per the commit log, this seems to be a temporary arrangement:

commit 546c13e11b29a5408b9d6a6e3cca301380b47f7f
Author: Simon Riggs <si...@2ndquadrant.com>
Date:   Sun Apr 23 22:12:01 2017 +0100

Workaround for RecoverPreparedTransactions()

Force overwriteOK = true while we investigate deeper fix

Proposed by Tom Lane as temporary measure, accepted by me

With latest update on following thread, it seems like Simon has proposed a
fix
that gets rid of flag overwriteOK:

http://www.postgresql-archive.org/StandbyRecoverPreparedTransactions-recovers-subtrans-links-incorrectly-td5957751.html#a5957853

Regards,
Jeevan Ladhe

On Tue, Apr 25, 2017 at 6:36 AM, Amit Langote <langote_amit...@lab.ntt.co.jp
> wrote:

> Been seeing this warning recently:
>
> twophase.c: In function ‘RecoverPreparedTransactions’:
> twophase.c:1916:9: warning: variable ‘overwriteOK’ set but not used
> [-Wunused-but-set-variable]
>bool  overwriteOK = false;
>  ^~~
>
> As the message says, the value of overwriteOK is not used anywhere in
> RecoverPreparedTransactions:
>
>  booloverwriteOK = false;
>
> /*
>  * It's possible that SubTransSetParent has been set before, if
>  * the prepared transaction generated xid assignment records. Test
>  * here must match one used in AssignTransactionId().
>  */
> if (InHotStandby && (hdr->nsubxacts >= PGPROC_MAX_CACHED_SUBXIDS ||
>  XLogLogicalInfoActive()))
> overwriteOK = true;
>
> Couldn't we get rid of it?
>
> 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] DELETE and UPDATE with LIMIT and ORDER BY

[Jeevan Ladhe]

Hi Surafel,

IIUC, the requirement of the feature also had one of the consideration where
one needs to delete large data and that takes long time, and adding LIMIT
should reduce the overhead by allowing to delete the data in batches.

I did a quick performance test, and in following example you can see the
conventional delete taking "355.288 ms" VS "1137.248 ms" with new LIMIT
syntax.

postgres=# create table mytab(a int, b varchar(50));
CREATE TABLE
postgres=# insert into mytab(a, b)
select i,md5(random()::text) from generate_series(1, 100) s(i);
INSERT 0 100
postgres=# \timing
Timing is on.
postgres=# delete from mytab order by a limit 20 offset 0;
DELETE 20
*Time: 1137.248 ms (00:01.137)*
postgres=# truncate mytab;
TRUNCATE TABLE
Time: 21.717 ms
postgres=# insert into mytab(a, b)
select i,md5(random()::text) from generate_series(1, 100) s(i);
INSERT 0 100
Time: 3166.445 ms (00:03.166)
postgres=# delete from mytab where a < 21;
DELETE 20
*Time: 355.288 ms*

Am I missing something here?

Regards,
Jeevan Ladhe

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

[Jeevan Ladhe]

On Mon, Apr 24, 2017 at 5:44 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas 
> wrote:
> > On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed 
> wrote:
> >> Following can also be considered as it specifies more clearly that the
> >> partition holds default values.
> >>
> >> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
> >
> > Yes, that could be done.  But I don't think it's correct to say that
> > the partition holds default values.  Let's back up and ask what the
> > word "default" means.  The relevant definition (according to Google or
> > whoever they stole it from) is:
> >
> > a preselected option adopted by a computer program or other mechanism
> > when no alternative is specified by the user or programmer.
> >
> > So, a default *value* is the value that is used when no alternative is
> > specified by the user or programmer. We have that concept, but it's
> > not what we're talking about here: that's configured by applying the
> > DEFAULT property to a column.  Here, we're talking about the default
> > *partition*, or in other words the *partition* that is used when no
> > alternative is specified by the user or programmer.  So, that's why I
> > proposed the syntax I did.  The partition doesn't contain default
> > values; it is itself a default.
>
> Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more
> natural.
>

+1

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

[Jeevan Ladhe]

 constraint"

10.
Additionally, I did test your given sample test in first post and the one
mentioned by Keith; both of them are passing without errors.
Also, I did a pg_dump test and it is dumping the partitions and data
correctly.
But as mentioned earlier, it would be good if you have them in your patch.

I will do further review and let you know comments if any.

Regards,
Jeevan Ladhe

On Mon, Apr 24, 2017 at 5:44 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas <robertmh...@gmail.com>
> wrote:
> > On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed <rahilasye...@gmail.com>
> wrote:
> >> Following can also be considered as it specifies more clearly that the
> >> partition holds default values.
> >>
> >> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
> >
> > Yes, that could be done.  But I don't think it's correct to say that
> > the partition holds default values.  Let's back up and ask what the
> > word "default" means.  The relevant definition (according to Google or
> > whoever they stole it from) is:
> >
> > a preselected option adopted by a computer program or other mechanism
> > when no alternative is specified by the user or programmer.
> >
> > So, a default *value* is the value that is used when no alternative is
> > specified by the user or programmer. We have that concept, but it's
> > not what we're talking about here: that's configured by applying the
> > DEFAULT property to a column.  Here, we're talking about the default
> > *partition*, or in other words the *partition* that is used when no
> > alternative is specified by the user or programmer.  So, that's why I
> > proposed the syntax I did.  The partition doesn't contain default
> > values; it is itself a default.
>
> Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more
> natural.
>
>
>
> --
> 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,


On Tue, Apr 11, 2017 at 6:02 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> On Mon, Apr 10, 2017 at 8:12 PM, Jeevan Ladhe
> <jeevan.la...@enterprisedb.com> wrote:
> > Hi Rahila,
> >
> >
> > With your latest patch:
> >
> > Consider a case when a table is partitioned on a boolean key.
> >
> > Even when there are existing separate partitions for 'true' and
> >
> > 'false', still default partition can be created.
> >
> >
> > I think this should not be allowed.
>
> Well, boolean columns can have "NULL" values which will go into
> default partition if no NULL partition exists. So, probably we should
> add check for NULL partition there.


I have checked for NULLs too, and the default partition can be created even
when there are partitions for each TRUE, FALSE and NULL.

Consider the example below:

postgres=# CREATE TABLE list_partitioned (
a bool
) PARTITION BY LIST (a);
CREATE TABLE
postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
('false');
CREATE TABLE
postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN
('true');
CREATE TABLE
postgres=# CREATE TABLE part_3 PARTITION OF list_partitioned FOR VALUES IN
(null);
CREATE TABLE
postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
VALUES IN (DEFAULT);
CREATE TABLE

Regards,
Jeevan Ladhe

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

[Jeevan Ladhe]

Hi Rahila,

I tried to review the code, and here are some of my early comments:

1.
When I configure using "-Werror", I see unused variable in function
DefineRelation:

tablecmds.c: In function ‘DefineRelation’:
tablecmds.c:761:17: error: unused variable ‘partdesc’
[-Werror=unused-variable]
   PartitionDesc partdesc;
 ^

2.
Typo in comment:
+ /*
+ * When adding a list partition after default partition, scan the
+ * default partiton for rows satisfying the new partition
+ * constraint. If found dont allow addition of a new partition.
+ * Otherwise continue with the creation of new  partition.
+ */

partition
don't

3.
I think instead of a period '.', it will be good if you can use semicolon
';'
in following declaration similar to the comment for 'null_index'.

+ int def_index; /* Index of the default list partition. -1 for
+ * range partitioned tables */

4.
You may want to consider 80 column alignment for changes done in function
get_qual_from_partbound, and other places as applicable.

5.
It would be good if the patch has some test coverage that explains what is
being achieved, what kind of error handling is done etc.

6.
There are some places having code like following:

+ Node *value = lfirst(c);
  Const   *val = lfirst(c);
  PartitionListValue *list_value = NULL;

+ if (IsA(value, DefElem))

The additional variable is not needed and you can call IsA on val itself.

7.
Also, in places like below where you are just trying to check for node is a
DefaultElem, you can avoid an extra variable:

+ foreach(cell1, bspec->listdatums)
+ {
+ Node *value = lfirst(cell1);
+ if (IsA(value, DefElem))
+ {
+ def_elem = true;
+ *defid = inhrelid;
+ }

Can be written as:
+ foreach(cell1, bspec->listdatums)
+ {
+ if (IsA(lfirst(cell1), DefElem))
+ {
+ def_elem = true;
+ *defid = inhrelid;
+ }
+ }


Regards,
Jeevan Ladhe



On Mon, Apr 10, 2017 at 8:12 PM, Jeevan Ladhe <jeevan.la...@enterprisedb.com
> wrote:

> Hi Rahila,
>
>
> With your latest patch:
>
> Consider a case when a table is partitioned on a boolean key.
>
> Even when there are existing separate partitions for 'true' and
>
> 'false', still default partition can be created.
>
>
> I think this should not be allowed.
>
>
> Consider following case:
>
>
> postgres=# CREATE TABLE list_partitioned (
>
> a bool
>
> ) PARTITION BY LIST (a);
>
> CREATE TABLE
>
> postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
> ('false');
>
> CREATE TABLE
>
> postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN
> ('true');
>
> CREATE TABLE
>
> postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
> VALUES IN (DEFAULT);
>
> CREATE TABLE
>
>
> The creation of table part_default should have failed instead.
>
>
> Thanks,
>
> Jeevan Ladhe
>
>
>
> On Thu, Apr 6, 2017 at 9:37 PM, Keith Fiske <ke...@omniti.com> wrote:
>
>>
>> On Thu, Apr 6, 2017 at 7:30 AM, Rahila Syed <rahilasye...@gmail.com>
>> wrote:
>>
>>> Hello,
>>>
>>> Thanks a lot for testing and reporting this. Please find attached an
>>> updated patch with the fix. The patch also contains a fix
>>> regarding operator used at the time of creating expression as default
>>> partition constraint. This was notified offlist by Amit Langote.
>>>
>>> Thank you,
>>> Rahila Syed
>>>
>>>
>> Could probably use some more extensive testing, but all examples I had on
>> my previously mentioned blog post are now working.
>>
>> Keith
>>
>>
>

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

[Jeevan Ladhe]

Hi Rahila,


With your latest patch:

Consider a case when a table is partitioned on a boolean key.

Even when there are existing separate partitions for 'true' and

'false', still default partition can be created.


I think this should not be allowed.


Consider following case:


postgres=# CREATE TABLE list_partitioned (

a bool

) PARTITION BY LIST (a);

CREATE TABLE

postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
('false');

CREATE TABLE

postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN
('true');

CREATE TABLE

postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
VALUES IN (DEFAULT);

CREATE TABLE


The creation of table part_default should have failed instead.


Thanks,

Jeevan Ladhe



On Thu, Apr 6, 2017 at 9:37 PM, Keith Fiske <ke...@omniti.com> wrote:

>
> On Thu, Apr 6, 2017 at 7:30 AM, Rahila Syed <rahilasye...@gmail.com>
> wrote:
>
>> Hello,
>>
>> Thanks a lot for testing and reporting this. Please find attached an
>> updated patch with the fix. The patch also contains a fix
>> regarding operator used at the time of creating expression as default
>> partition constraint. This was notified offlist by Amit Langote.
>>
>> Thank you,
>> Rahila Syed
>>
>>
> Could probably use some more extensive testing, but all examples I had on
> my previously mentioned blog post are now working.
>
> Keith
>
>

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

[Jeevan Ladhe]

Hi Rahila,

IIUC, your default_partition_v3.patch is trying to implement an error if new
partition is added to a table already having a default partition.

I too tried to run the test and similar to Rushabh, I see the server is
crashing
with the given test.

However, if I reverse the order of creating the partitions, i.e. if I
create a
partition with list first and later create the default partition.

The reason is, while defining new relation DefineRelation() checks for
overlapping partitions by calling check_new_partition_bound(). Where in case
of list partitions it assumes that the ndatums should be > 0, but in case of
default partition that is 0.

The crash here seems to be coming because, following assertion getting
failed in
function check_new_partition_bound():


Assert(boundinfo &&
  boundinfo->strategy == PARTITION_STRATEGY_LIST &&
  (boundinfo->ndatums > 0 || boundinfo->has_null));


So, I think the error you have added needs to be moved before this
assertion:


@@ -690,6 +715,12 @@ check_new_partition_bound(char *relname, Relation
parent, Node *bound)
boundinfo->strategy == PARTITION_STRATEGY_LIST &&
(boundinfo->ndatums > 0 || boundinfo->has_null));

+ if (boundinfo->has_def)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("parent table \"%s\" has a default partition",
+ RelationGetRelationName(parent;


If I do so, the server does not run into crash, and instead throws an error:

postgres=# CREATE TABLE list_partitioned (
a int
) PARTITION BY LIST (a);
CREATE TABLE
postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
VALUES IN (DEFAULT);
CREATE TABLE
postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
(4,5);
ERROR:  parent table "list_partitioned" has a default partition

Regards,
Jeevan Ladhe

On Mon, Mar 27, 2017 at 12:10 PM, Rushabh Lathia <rushabh.lat...@gmail.com>
wrote:

> I applied the patch and was trying to perform some testing, but its
> ending up with server crash with the test shared by you in your starting
> mail:
>
> postgres=# CREATE TABLE list_partitioned (
> postgres(# a int
> postgres(# ) PARTITION BY LIST (a);
> CREATE TABLE
> postgres=#
> postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
> VALUES IN (DEFAULT);
> CREATE TABLE
>
> postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
> (4,5);
> 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.
>
> Apart from this, few more explanation in the patch is needed to explain the
> changes for the DEFAULT partition. Like I am not quite sure what exactly
> the
> latest version of patch supports, like does that support the tuple row
> movement,
> or adding new partition will be allowed having partition table having
> DEFAULT
> partition, which is quite difficult to understand through the code changes.
>
> Another part which is missing in the patch is the test coverage, adding
> proper test coverage, which explain what is supported and what's not.
>
> Thanks,
>
> On Fri, Mar 24, 2017 at 3:25 PM, Rahila Syed <rahilasye...@gmail.com>
> wrote:
>
>> Hello Rushabh,
>>
>> Thank you for reviewing.
>> Have addressed all your comments in the attached patch. The attached
>> patch currently throws an
>> error if a new partition is added after default partition.
>>
>> >Rather then adding check for default here, I think this should be
>> handle inside
>> >get_qual_for_list().
>> Have moved the check inside get_qual_for_partbound() as needed to do some
>> operations
>> before calling get_qual_for_list() for default partitions.
>>
>> Thank you,
>> Rahila Syed
>>
>> On Tue, Mar 21, 2017 at 11:36 AM, Rushabh Lathia <
>> rushabh.lat...@gmail.com> wrote:
>>
>>> I picked this for review and noticed that patch is not getting
>>> cleanly complied on my environment.
>>>
>>> partition.c: In function ‘RelationBuildPartitionDesc’:
>>> partition.c:269:6: error: ISO C90 forbids mixed declarations and code
>>> [-Werror=declaration-after-statement]
>>>   Const*val = lfirst(c);
>>>   ^
>>> partition.c: In function ‘generate_partition_qual’:
>>> partition.c:1590:2: error: ISO C90 forbids mixed declarations and code
>>> [-Werror=declaration-after-statement]
>>>   PartitionBoundSpec *spec = (PartitionBoundSpec *)bound;
>>>   ^
>>> partition.c: In function ‘get_partition_for_tuple’:
>>> partition

Re: [HACKERS] Bug in get_partition_for_tuple

[Jeevan Ladhe]

Hi Amit,

I was able to reproduce the crash, and with the attached patch the crash
goes
away. Also, "make check-world" passes clean.

Patch looks good to me. However, In following comment in your test:

-- check routing error through a list partitioned table when they key is
null

I think you want to say:

-- check routing error through a list partitioned table when the key is null


Thanks,
Jeevan Ladhe


On Fri, Mar 10, 2017 at 8:26 AM, Amit Langote <langote_amit...@lab.ntt.co.jp
> wrote:

> Just observed a crash due to thinko in the logic that handles NULL
> partition key.  Absence of null-accepting partition in this case should
> have caused an error, instead the current code proceeds with comparison
> resulting in crash.
>
> create table p (a int, b char) partition by list (b);
> create table p1 partition of p for values in ('a');
> insert into p values (1);   -- crashes
>
> Attached patch fixes that and adds a test.
>
> 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] pgbench more operators & functions

[Jeevan Ladhe]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:tested, passed

The patch looks good to me now.
Passing this to committer.

The new status of this patch is: Ready for Committer

-- 
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] pgbench more operators & functions

[Jeevan Ladhe]

Hi,

The patch has correct precedence now.

Further minor comments:

1. About documentation, I think it will be good idea to arrange the
operators
table with the precedence and add a line at top: "In decreasing order of
precedence".

2. You may want to remove the comment:
+   /* should it do a lazy evaluation of the branch? */

Regards,
Jeevan Ladhe

Re: [HACKERS] pgbench more operators & functions

[Jeevan Ladhe]

On Sat, Jul 9, 2016 at 12:14 PM, Fabien COELHO <coe...@cri.ensmp.fr> wrote:
>
>> Here is a simple patch which adds a bunch of operators (bitwise: & | ^ ~,
>> comparisons: =/== <>/!= < <= > >=, logical: and/&& or/|| xor/^^ not/!) and
>> functions (exp ln if) to pgbench. I've tried to be pg's SQL compatible where
>> appropriate.
>>
>> Also attached is a simple test script.
>
>
> Here is a sightly updated version.
>

Hi Fabien,

I did the review of your patch and here are my views on your patch.


Purpose of the patch:
=

This patch introduces extensive list of new operators and functions that can be
used in expressions in pgbench transaction scripts(given with -f option).

Here is the list of operators and functions introduced by this patch:

New operators:
--
bitwise: <<, >>, &, |, ^/#, ~
comparisons: =/==, <>/!=, <, <=, >, >=
logical: and/&&, or/||, xor/^^, not, !

New functions:
--
exp, ln, if

I see there had been a discussion about timing for submission of patch, but not
much about what the patch is doing, so I believe the purpose of patch is quite
acceptable.
Currently there are limited number of operators available in pgbench. So, I
think these new operators definitely make a value addition towards custom
scripts.

Documentation:
=
I could build the documentation without any errors.
New operators and functions are well categorized and added in proper sections
of existing pgbench documentation.

I have a small suggestion here: Earlier we had only few number of operators, so
it was OK to have the operators embedded in the description of '\set' command
itself, but now as we have much more number of operators will it be a good idea
to have a table of operators similar to that of functions. We need not have
several columns here like description, example etc., but a short table just
categorizing the operators would be sufficient.

Initial Run:

I was able to apply patch with 'patch -p1'.
The testcase file(functions.sql) given along the patch gives an expected output.

Further testing and review:
===
1. Postgres treats '^' as exponentiation rather than XOR, and '#' as XOR.
Personally, I think it can cause confusion, so it will be better if we can stick
to the behavior of Postgres mathematical operators.

2. I could not see any tests for bitwise operators in the functions.sql file
that you have attached.

3. Precedence:
a. Unary operators have more precedence than binary operators. So, UNOT and
UINV should have precedence next to UMINUS.
I tried couple of test expressions using C Vs your patch(pgbench)

expression result_in_C result_in_pgbench
(~14-14+2) -27 -3
(!14-14+2) -12 0

b. Similarly shift operators should take more precedence over other bitwise
operators:

expression result_in_C result_in_pgbench
(4|1<<1) 6 10
(4^5&3) 5 1

c. Also, comparison would take more precedence over bitwise operators(&,|,^)
but shift operators.

expression result_in_C result_in_pgbench
(2&1<3) 1 0

In backend/parser/gram.y, I see that unary operators are given higher precedence
than other operators, but it too does not have explicit precedence defined for
bitwise operators.
I tried to check Postgres documentation for operator precedence, but in
'Lexical Structure'[1] the documentation does not mention anything about bitwise
operators. Also, SQL standards 99 does not talk much about operator precedence.
I may be wrong while trying to understand the precedence you defined here and
you might have picked this per some standard, but do you have any reference
which you considered for this?

4. If we are going to stick to current precedence, I think it will be good idea
to document it.

5. Sorry, I was not able to understand the "should exist" comment in following
snippet.

+"xor" { return XOR_OP; } /* should exist */
+"^^" { return XOR_OP; } /* should exist */

7. You may want to reword following comment:

+ else /* cannot get there */

To

+ else /* cannot get here */

8.
+ case PGBENCH_IF:
+ /* should it do a lazy evaluation of the branch? */
+ Assert(nargs == 3);
+ *retval = coerceToBool([0]) ? vargs[1] : vargs[2];

I believe ternary operator does the lazy evaluation internally, but to be sure
you may consider rewriting this as following:

if (coerceToBool([0]))
*retval = vargs[1];
else
*retval = vargs[2];

Conclusion:
===
I have tested the patch and each of the operator is implemented correctly.
The only concern I have is precedence, otherwise the patch seems to be doing
what it is supposed to do.

[1]https://www.postgresql.org/docs/9.6/static/sql-syntax-lexical.html

Regards,
Jeevan Ladhe.


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