On 2018/03/01 17:16, Amit Langote wrote:
> Added this to CF (actually moved to the September one after first having
> added it to the CF that is just getting started).
>
> It seems to me that we don't need to go with my originally proposed
> approach to teach predtest.c to strip RelabelType nodes. Apparently, it's
> only partition.c that's adding the RelableType node around partition key
> nodes in such cases. That is, in the case of having an array, record,
> enum, and range type as the partition key. No other part of the system
> ends up adding one as far as I can see. Parser's make_op(), for example,
> that is used to generate an OpExpr for a qual involving the partition key,
> won't put RelabelType around the partition key node if the operator in
> question has "pseudo"-types listed as declared types of its left and right
> arguments.
>
> So I revised the patch to drop all the predtest.c changes and instead
> modify get_partition_operator() to avoid generating RelabelType in such
> cases. Please find it attached.
I would like to revisit this as a bug fix for get_partition_operator() to
be applied to both PG 10 and HEAD. In the former case, it fixes the bug
that constraint exclusion code will fail to prune correctly when partition
key is of array, enum, range, or record type due to the structural
mismatch between the OpExpr that partitioning code generates and one that
the parser generates for WHERE clauses involving partition key columns.
In HEAD, since we already fixed that case in e5dcbb88a15d [1] which is a
different piece of code anyway, the patch only serves to improve the
deparse output emitted by ruleutils.c for partition constraint expressions
where pseudo-type partition key is involved. The change can be seen in
the updated test output for create_table test.
Attached are patches for PG 10 and HEAD, which implement a slightly
different approach to fixing this. Now, instead of passing the partition
key's type as lefttype and righttype to look up the operator, the operator
class declared type is passed. Also, we now decide whether to create a
RelabelType node on top based on whether the partition key's type is
different from the selected operator's input type, with exception for
pseudo-type types.
Thanks,
Amit
[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=e5dcbb88a15d
From 9f0dff4e6f3c7e8204a73ef0734e38056de08571 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 8 May 2018 14:31:37 +0900
Subject: [PATCH v1] Fix how get_partition_operator looks up the operator
Instead of looking up using the underlying partition key's type as the
operator's lefttype and righttype, use the partition operator class
declared input type, which works reliably even in the cases where
pseudo-types are involved. Also, make its decision whether a
RelableType is needed depend on a different criteria than what's
currently there. That is, only add a RelabelType if the partition
key's type is different from the selected operator's input types.
---
src/backend/catalog/partition.c | 49 +++++++++++++-----------------
src/test/regress/expected/create_table.out | 2 +-
2 files changed, 22 insertions(+), 29 deletions(-)
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 17704f36b9..5603a5d992 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1176,40 +1176,33 @@ get_partition_operator(PartitionKey key, int col,
StrategyNumber strategy,
Oid operoid;
/*
- * First check if there exists an operator of the given strategy, with
- * this column's type as both its lefttype and righttype, in the
- * partitioning operator family specified for the column.
+ * Get the operator in the partitioning operator family using the
operator
+ * class declared input type as both its lefttype and righttype.
*/
operoid = get_opfamily_member(key->partopfamily[col],
-
key->parttypid[col],
-
key->parttypid[col],
+
key->partopcintype[col],
+
key->partopcintype[col],
strategy);
+ if (!OidIsValid(operoid))
+ elog(ERROR, "missing operator %d(%u,%u) in partition opfamily
%u",
+ strategy, key->partopcintype[col],
key->partopcintype[col],
+ key->partopfamily[col]);
/*
- * If one doesn't exist, we must resort to using an operator in the same
- * operator family but with the operator class declared input type. It
is
- * OK to do so, because the column's type is known to be
binary-coercible
- * with the operator class input type (otherwise, the operator class in
- * question would not have been accepted as the partitioning operator
- * class). We must however inform the caller to wrap the non-Const
- * expression with a RelabelType node to denote the implicit coercion.
It
- * ensures that the resulting expression structurally matches similarly
- * processed expressions within the optimizer.
+ * If the partition key column is not of the same type as what the
+ * operator expects, we'll need to tell the caller to wrap the non-Const
+ * expression with a RelableType node in most cases, because that's what
+ * the parser would do for similar expressions (such as WHERE clauses
+ * involving this column and operator.) The intention is to make the
+ * expression that we generate here structurally match with expression
+ * generated elsewhere. We don't need the RelableType node for certain
+ * types though, because parse_coerce.c won't emit one either.
*/
- if (!OidIsValid(operoid))
- {
- operoid = get_opfamily_member(key->partopfamily[col],
-
key->partopcintype[col],
-
key->partopcintype[col],
-
strategy);
- if (!OidIsValid(operoid))
- elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
- strategy, key->partopcintype[col],
key->partopcintype[col],
- key->partopfamily[col]);
- *need_relabel = true;
- }
- else
- *need_relabel = false;
+ *need_relabel = (key->parttypid[col] != key->partopcintype[col] &&
+ !(key->partopcintype[col] ==
ANYARRAYOID ||
+ key->partopcintype[col] ==
ANYENUMOID ||
+ key->partopcintype[col] ==
ANYRANGEOID ||
+ key->partopcintype[col] ==
RECORDOID));
return operoid;
}
diff --git a/src/test/regress/expected/create_table.out
b/src/test/regress/expected/create_table.out
index 68dda4c0db..1caa3afbbf 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -808,7 +808,7 @@ CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN
('{1}', '{2}');
--------+-----------+-----------+----------+---------+----------+--------------+-------------
a | integer[] | | | | extended |
|
Partition of: arrlp FOR VALUES IN ('{1}', '{2}')
-Partition constraint: ((a IS NOT NULL) AND (((a)::anyarray
OPERATOR(pg_catalog.=) '{1}'::integer[]) OR ((a)::anyarray
OPERATOR(pg_catalog.=) '{2}'::integer[])))
+Partition constraint: ((a IS NOT NULL) AND ((a = '{1}'::integer[]) OR (a =
'{2}'::integer[])))
DROP TABLE arrlp;
-- partition on boolean column
--
2.11.0
From a4af5f05e6c3b79e0adfd99925cf9eba93a0f390 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Tue, 8 May 2018 14:57:47 +0900
Subject: [PATCH v4] Fix how get_partition_operator looks up the operator
Instead of looking up using the underlying partition key's type as the
operator's lefttype and righttype, use the partition operator class
declared input type, which works reliably even in the cases where
pseudo-types are involved. Also, make its decision whether a
RelableType is needed depend on a different criteria than what's
currently there. That is, only add a RelabelType if the partition
key's type is different from the selected operator's input types.
---
src/backend/partitioning/partbounds.c | 49 +++++++++++++-----------------
src/test/regress/expected/create_table.out | 2 +-
2 files changed, 22 insertions(+), 29 deletions(-)
diff --git a/src/backend/partitioning/partbounds.c
b/src/backend/partitioning/partbounds.c
index 09c7c3e252..3b988fb1cf 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -1154,40 +1154,33 @@ get_partition_operator(PartitionKey key, int col,
StrategyNumber strategy,
Oid operoid;
/*
- * First check if there exists an operator of the given strategy, with
- * this column's type as both its lefttype and righttype, in the
- * partitioning operator family specified for the column.
+ * Get the operator in the partitioning operator family using the
operator
+ * class declared input type as both its lefttype and righttype.
*/
operoid = get_opfamily_member(key->partopfamily[col],
-
key->parttypid[col],
-
key->parttypid[col],
+
key->partopcintype[col],
+
key->partopcintype[col],
strategy);
+ if (!OidIsValid(operoid))
+ elog(ERROR, "missing operator %d(%u,%u) in partition opfamily
%u",
+ strategy, key->partopcintype[col],
key->partopcintype[col],
+ key->partopfamily[col]);
/*
- * If one doesn't exist, we must resort to using an operator in the same
- * operator family but with the operator class declared input type. It
is
- * OK to do so, because the column's type is known to be
binary-coercible
- * with the operator class input type (otherwise, the operator class in
- * question would not have been accepted as the partitioning operator
- * class). We must however inform the caller to wrap the non-Const
- * expression with a RelabelType node to denote the implicit coercion.
It
- * ensures that the resulting expression structurally matches similarly
- * processed expressions within the optimizer.
+ * If the partition key column is not of the same type as what the
+ * operator expects, we'll need to tell the caller to wrap the non-Const
+ * expression with a RelableType node in most cases, because that's what
+ * the parser would do for similar expressions (such as WHERE clauses
+ * involving this column and operator.) The intention is to make the
+ * expression that we generate here structurally match with expression
+ * generated elsewhere. We don't need the RelableType node for certain
+ * types though, because parse_coerce.c won't emit one either.
*/
- if (!OidIsValid(operoid))
- {
- operoid = get_opfamily_member(key->partopfamily[col],
-
key->partopcintype[col],
-
key->partopcintype[col],
-
strategy);
- if (!OidIsValid(operoid))
- elog(ERROR, "missing operator %d(%u,%u) in opfamily %u",
- strategy, key->partopcintype[col],
key->partopcintype[col],
- key->partopfamily[col]);
- *need_relabel = true;
- }
- else
- *need_relabel = false;
+ *need_relabel = (key->parttypid[col] != key->partopcintype[col] &&
+ !(key->partopcintype[col] ==
ANYARRAYOID ||
+ key->partopcintype[col] ==
ANYENUMOID ||
+ key->partopcintype[col] ==
ANYRANGEOID ||
+ key->partopcintype[col] ==
RECORDOID));
return operoid;
}
diff --git a/src/test/regress/expected/create_table.out
b/src/test/regress/expected/create_table.out
index 470fca0cab..6c945a8d49 100644
--- a/src/test/regress/expected/create_table.out
+++ b/src/test/regress/expected/create_table.out
@@ -882,7 +882,7 @@ CREATE TABLE arrlp12 PARTITION OF arrlp FOR VALUES IN
('{1}', '{2}');
--------+-----------+-----------+----------+---------+----------+--------------+-------------
a | integer[] | | | | extended |
|
Partition of: arrlp FOR VALUES IN ('{1}', '{2}')
-Partition constraint: ((a IS NOT NULL) AND (((a)::anyarray
OPERATOR(pg_catalog.=) '{1}'::integer[]) OR ((a)::anyarray
OPERATOR(pg_catalog.=) '{2}'::integer[])))
+Partition constraint: ((a IS NOT NULL) AND ((a = '{1}'::integer[]) OR (a =
'{2}'::integer[])))
DROP TABLE arrlp;
-- partition on boolean column
--
2.11.0
