Thread: [HACKERS] fix hard-coded index in make_partition_op_expr
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
Attachment
On Wed, May 17, 2017 at 5:38 AM, Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> wrote: > 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'. Agreed. Committed your patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, May 18, 2017 at 12:13 AM, Robert Haas <robertmhaas@gmail.com> wrote:
Agreed. Committed your patch.
Thanks Robert!
On 2017/05/18 3:43, Robert Haas wrote: > On Wed, May 17, 2017 at 5:38 AM, Jeevan Ladhe > <jeevan.ladhe@enterprisedb.com> wrote: >> 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'. > > Agreed. Committed your patch. Me too, thanks both. Regards, Amit