Re: Default Partition for Range

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:
- Oid defid;
+ 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 <memissemerson(at)gmail(dot)com>
wrote:

> Hello,
>
> On Mon, May 22, 2017 at 11:29 AM, Ashutosh Bapat <
> ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>
>> On Mon, May 22, 2017 at 7:27 AM, Beena Emerson <memissemerson(at)gmail(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>