fix hard-coded index in make_partition_op_expr

Started by Jeevan Ladheover 8 years ago4 messages
#1Jeevan Ladhe
jeevan.ladhe@enterprisedb.com
1 attachment(s)

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

Attachments:

fix_indexing_make_partition_op_expr_v1.patchapplication/octet-stream; name=fix_indexing_make_partition_op_expr_v1.patchDownload
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 832049c..45d95a8 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1274,7 +1274,7 @@ make_partition_op_expr(PartitionKey key, int keynum,
 				saopexpr->opno = operoid;
 				saopexpr->opfuncid = get_opcode(operoid);
 				saopexpr->useOr = true;
-				saopexpr->inputcollid = key->partcollation[0];
+				saopexpr->inputcollid = key->partcollation[keynum];
 				saopexpr->args = list_make2(arg1, arg2);
 				saopexpr->location = -1;
 
#2Robert Haas
robertmhaas@gmail.com
In reply to: Jeevan Ladhe (#1)
Re: fix hard-coded index in make_partition_op_expr

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

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

#3Jeevan Ladhe
jeevan.ladhe@enterprisedb.com
In reply to: Robert Haas (#2)
Re: fix hard-coded index in make_partition_op_expr

On Thu, May 18, 2017 at 12:13 AM, Robert Haas <robertmhaas@gmail.com> wrote:

Agreed. Committed your patch.

Thanks Robert!

#4Amit Langote
Langote_Amit_f8@lab.ntt.co.jp
In reply to: Robert Haas (#2)
Re: fix hard-coded index in make_partition_op_expr

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

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