From: | Beena Emerson <memissemerson(at)gmail(dot)com> |
---|---|
To: | Jeevan Ladhe <jeevan(dot)ladhe(at)enterprisedb(dot)com> |
Cc: | Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Default Partition for Range |
Date: | 2017-05-31 12:23:43 |
Message-ID: | CAOG9ApFZXKRRivXyfDq7hgNBvk6fFFFiBT0amqcEhkFOLq3ckQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hello,
PFA the updated patch.
Dependent patch default_partition_v17.patch [1]
This patch adds regression tests and removes the separate function to
get default partition qual.
On Mon, May 29, 2017 at 3:22 PM, Jeevan Ladhe
<jeevan(dot)ladhe(at)enterprisedb(dot)com> wrote:
> 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.
>
Unlike list, range partition can be for multiple columns and the
expressions get complicated. I have used the same logic of looping
through different partitions and negating the ORed expr. However, this
is now done under get_qual_for_range.
> - Wrong comment:
> + int default_index; /* Index of the default list partition. */
Comment is made generic in the dependent patch.
>
> - Suggested by Robert earlier on default list partitioning thread, instead
> of
> abbreviating is_def/found_def use is(found)_default etc.
corrected.
>
> - unrelated change:
> - List *all_parts;
> + List *all_parts;
>
undone.
> - typo: partiton should be partition, similar typo at other places too.
> + * A non-default range partiton table does not currently allow partition
> keys
>
rectified.
> - Useless hunk for this patch:
> - Oid defid;
> + Oid defid;
undone.
>
> - better to use IsA here, instead of doing explicit check:
> + bound->content[i] = (datum->type == T_DefElem)
> + ? RANGE_DATUM_DEFAULT
Modified
>
> - 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.
>
modified this part as necessary.
> - 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.
> + *
>
Updated.
[1] http://www.mail-archive.com/pgsql-hackers(at)postgresql(dot)org/msg315573.html
--
Beena Emerson
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachment | Content-Type | Size |
---|---|---|
default_range_partition_v2.patch | application/octet-stream | 28.6 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Bruce Momjian | 2017-05-31 13:04:54 | Re: Use of non-restart-safe storage by temp_tablespaces |
Previous Message | tushar | 2017-05-31 11:54:35 | Error while creating subscription when server is running in single user mode |