Обсуждение: [HACKERS] PartitionSchemaData & partcollation (Re: [COMMITTERS] pgsql:Associate partitioning information with each RelOptInfo.)

Поиск
Список
Период
Сортировка
On 2017/09/21 12:42, Robert Haas wrote:
> Associate partitioning information with each RelOptInfo.
> 
> This is not used for anything yet, but it is necessary infrastructure
> for partition-wise join and for partition pruning without constraint
> exclusion.
> 
> Ashutosh Bapat, reviewed by Amit Langote and with quite a few changes,
> mostly cosmetic, by me.  Additional review and testing of this patch
> series by Antonin Houska, Amit Khandekar, Rafia Sabih, Rajkumar
> Raghuwanshi, Thomas Munro, and Dilip Kumar.

I noticed that this commit does not add partcollation field to
PartitionSchemeData, while it adds parttypcoll.  I think it'd be necessary
to have partcollation too, because partitioning would have used the same,
not parttypcoll.  For, example see the following code in
partition_rbound_datum_cmp:

        cmpval = DatumGetInt32(FunctionCall2Coll(&key->partsupfunc[i],
                                                 key->partcollation[i],
                                                 rb_datums[i],
                                                 tuple_datums[i]));

So, it would be wrong to use parttypcoll, if we are to use the collation
to match a clause with the partition bounds when doing partition-pruning.
Concretely, a clause's inputcollid should match partcollation for the
corresponding column, not the column's parttypcoll.

Attached is a patch that adds the same.  I first thought of including it
in the partition-pruning patch set [1], but thought we could independently
fix this.

Thoughts?

Thanks,
Amit

[1] https://commitfest.postgresql.org/14/1272/

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

Вложения
Sorry, I meant to say PartitionSchem"e"Data in subject.

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

On Thu, Sep 28, 2017 at 11:47 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/09/21 12:42, Robert Haas wrote:
>> Associate partitioning information with each RelOptInfo.
>>
>> This is not used for anything yet, but it is necessary infrastructure
>> for partition-wise join and for partition pruning without constraint
>> exclusion.
>>
>> Ashutosh Bapat, reviewed by Amit Langote and with quite a few changes,
>> mostly cosmetic, by me.  Additional review and testing of this patch
>> series by Antonin Houska, Amit Khandekar, Rafia Sabih, Rajkumar
>> Raghuwanshi, Thomas Munro, and Dilip Kumar.
>
> I noticed that this commit does not add partcollation field to
> PartitionSchemeData, while it adds parttypcoll.  I think it'd be necessary
> to have partcollation too, because partitioning would have used the same,
> not parttypcoll.  For, example see the following code in
> partition_rbound_datum_cmp:
>
>         cmpval = DatumGetInt32(FunctionCall2Coll(&key->partsupfunc[i],
>                                                  key->partcollation[i],
>                                                  rb_datums[i],
>                                                  tuple_datums[i]));
>
> So, it would be wrong to use parttypcoll, if we are to use the collation
> to match a clause with the partition bounds when doing partition-pruning.
> Concretely, a clause's inputcollid should match partcollation for the
> corresponding column, not the column's parttypcoll.
>
> Attached is a patch that adds the same.  I first thought of including it
> in the partition-pruning patch set [1], but thought we could independently
> fix this.
>

I think PartitionSchemeData structure will grow as we need more
information about partition key for various things. E.g. partsupfunc
is not part of this structure right now, but it would be required to
compare partition bound datums. Similarly partcollation. Please add
this to partition pruning patchset. May be parttypcoll won't be used
at all and we may want to remove it altogether.

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

On 2017/09/28 16:13, Ashutosh Bapat wrote:
> On Thu, Sep 28, 2017 at 11:47 AM, Amit Langote wrote:
>> On 2017/09/21 12:42, Robert Haas wrote:
>>> Associate partitioning information with each RelOptInfo.
>>>
>>> This is not used for anything yet, but it is necessary infrastructure
>>> for partition-wise join and for partition pruning without constraint
>>> exclusion.
>>>
>>> Ashutosh Bapat, reviewed by Amit Langote and with quite a few changes,
>>> mostly cosmetic, by me.  Additional review and testing of this patch
>>> series by Antonin Houska, Amit Khandekar, Rafia Sabih, Rajkumar
>>> Raghuwanshi, Thomas Munro, and Dilip Kumar.
>>
>> I noticed that this commit does not add partcollation field to
>> PartitionSchemeData, while it adds parttypcoll.  I think it'd be necessary
>> to have partcollation too, because partitioning would have used the same,
>> not parttypcoll.  For, example see the following code in
>> partition_rbound_datum_cmp:
>>
>>         cmpval = DatumGetInt32(FunctionCall2Coll(&key->partsupfunc[i],
>>                                                  key->partcollation[i],
>>                                                  rb_datums[i],
>>                                                  tuple_datums[i]));
>>
>> So, it would be wrong to use parttypcoll, if we are to use the collation
>> to match a clause with the partition bounds when doing partition-pruning.
>> Concretely, a clause's inputcollid should match partcollation for the
>> corresponding column, not the column's parttypcoll.
>>
>> Attached is a patch that adds the same.  I first thought of including it
>> in the partition-pruning patch set [1], but thought we could independently
>> fix this.
>>
> 
> I think PartitionSchemeData structure will grow as we need more
> information about partition key for various things. E.g. partsupfunc
> is not part of this structure right now, but it would be required to
> compare partition bound datums. Similarly partcollation. Please add
> this to partition pruning patchset. May be parttypcoll won't be used
> at all and we may want to remove it altogether.

Okay, I will post it with the partition-pruning patch set.

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