On Tue, Aug 5, 2014 at 3:15 PM, Peter Geoghegan <p...@heroku.com> wrote:
> On Tue, Aug 5, 2014 at 12:03 PM, Robert Haas <robertmh...@gmail.com> wrote:
>> and if some opclass other than text wants to
>> provide a sortsupport shim that supplies a comparator only sometimes,
>> it will need its own copy of the logic.
>
> That's true, but my proposal to do things that way reflects the fact
> that text is a type oddly tied to the platform. I don't think it will
> come up again (note that in the 4 byte Datum case, we still use sort
> support to some degree on other platforms with patch 2 applied). It
> seemed logical to impose the obligation to deal with that on
> varlena.c.
Per your other email, here's the patch again; hopefully I've got the
right stuff in the file this time.
On this point, I'm all for confining knowledge of things to a
particular module to that module. However, in this particular case, I
don't believe that there's anything specific to varlena.c in
bttext_inject_shim(); it's a cut down version of the combination of
functions that appear in other modules, and there's nothing to
encourage someone who modifies one of those functions to also update
varlena.c. Indeed, people developing on non-Windows platforms won't
even be compiling that function, so it would be easy for most of us to
miss the need for an update. So I argue that my approach is keeping
this knowledge more localized.
I'm also not sure it won't come up again. There are certainly other
text-like datatypes out there that might want to optimize sorts; e.g.
citext.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c
index bc036a3..fdf2f4c 100644
--- a/src/backend/executor/nodeMergejoin.c
+++ b/src/backend/executor/nodeMergejoin.c
@@ -230,19 +230,19 @@ MJExamineQuals(List *mergeclauses,
qual->opno);
/* And get the matching support or comparison function */
+ Assert(clause->ssup.comparator == NULL);
sortfunc = get_opfamily_proc(opfamily,
op_lefttype,
op_righttype,
BTSORTSUPPORT_PROC);
if (OidIsValid(sortfunc))
{
- /* The sort support function should provide a comparator */
+ /* The sort support function can provide a comparator */
OidFunctionCall1(sortfunc, PointerGetDatum(&clause->ssup));
- Assert(clause->ssup.comparator != NULL);
}
- else
+ if (clause->ssup.comparator == NULL)
{
- /* opfamily doesn't provide sort support, get comparison func */
+ /* support not available, get comparison func */
sortfunc = get_opfamily_proc(opfamily,
op_lefttype,
op_righttype,
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 4b5ef99..552e498 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -246,62 +246,6 @@ get_ordering_op_properties(Oid opno,
}
/*
- * get_sort_function_for_ordering_op
- * Get the OID of the datatype-specific btree sort support function,
- * or if there is none, the btree comparison function,
- * associated with an ordering operator (a "<" or ">" operator).
- *
- * *sortfunc receives the support or comparison function OID.
- * *issupport is set TRUE if it's a support func, FALSE if a comparison func.
- * *reverse is set FALSE if the operator is "<", TRUE if it's ">"
- * (indicating that comparison results must be negated before use).
- *
- * Returns TRUE if successful, FALSE if no btree function can be found.
- * (This indicates that the operator is not a valid ordering operator.)
- */
-bool
-get_sort_function_for_ordering_op(Oid opno, Oid *sortfunc,
- bool *issupport, bool *reverse)
-{
- Oid opfamily;
- Oid opcintype;
- int16 strategy;
-
- /* Find the operator in pg_amop */
- if (get_ordering_op_properties(opno,
- &opfamily, &opcintype, &strategy))
- {
- /* Found a suitable opfamily, get matching support function */
- *sortfunc = get_opfamily_proc(opfamily,
- opcintype,
- opcintype,
- BTSORTSUPPORT_PROC);
- if (OidIsValid(*sortfunc))
- *issupport = true;
- else
- {
- /* opfamily doesn't provide sort support, get comparison func */
- *sortfunc = get_opfamily_proc(opfamily,
- opcintype,
- opcintype,
- BTORDER_PROC);
- if (!OidIsValid(*sortfunc)) /* should not happen */
- elog(ERROR, "missing support function %d(%u,%u) in opfamily %u",
- BTORDER_PROC, opcintype, opcintype, opfamily);
- *issupport = false;
- }
- *reverse = (strategy == BTGreaterStrategyNumber);
- return true;
- }
-
- /* ensure outputs are set on failure */
- *sortfunc = InvalidOid;
- *issupport = false;
- *reverse = false;
- return false;
-}
-
-/*
* get_equality_op_for_ordering_op
* Get the OID of the datatype-specific btree equality operator
* associated with an ordering operator (a "<" or ">" operator).
diff --git a/src/backend/utils/sort/sortsupport.c b/src/backend/utils/sort/sortsupport.c
index 347f448..2240fd0 100644
--- a/src/backend/utils/sort/sortsupport.c
+++ b/src/backend/utils/sort/sortsupport.c
@@ -18,6 +18,7 @@
/* See sortsupport.h */
#define SORTSUPPORT_INCLUDE_DEFINITIONS
+#include "access/nbtree.h"
#include "fmgr.h"
#include "utils/lsyscache.h"
#include "utils/sortsupport.h"
@@ -94,24 +95,43 @@ PrepareSortSupportComparisonShim(Oid cmpFunc, SortSupport ssup)
void
PrepareSortSupportFromOrderingOp(Oid orderingOp, SortSupport ssup)
{
- Oid sortFunction;
- bool issupport;
+ Oid sortSupportFunction;
+ Oid opfamily;
+ Oid opcintype;
+ int16 strategy;
- if (!get_sort_function_for_ordering_op(orderingOp,
- &sortFunction,
- &issupport,
- &ssup->ssup_reverse))
+ Assert(ssup->comparator == NULL);
+
+ /* Find the operator in pg_amop */
+ if (!get_ordering_op_properties(orderingOp, &opfamily, &opcintype,
+ &strategy))
elog(ERROR, "operator %u is not a valid ordering operator",
orderingOp);
+ ssup->ssup_reverse = (strategy == BTGreaterStrategyNumber);
- if (issupport)
+ /* Look for a sort support function */
+ sortSupportFunction = get_opfamily_proc(opfamily, opcintype, opcintype,
+ BTSORTSUPPORT_PROC);
+ if (OidIsValid(sortSupportFunction))
{
- /* The sort support function should provide a comparator */
- OidFunctionCall1(sortFunction, PointerGetDatum(ssup));
- Assert(ssup->comparator != NULL);
+ /*
+ * The sort support function can provide a comparator, but it can
+ * also choose not to so (e.g. based on the selected collation).
+ */
+ OidFunctionCall1(sortSupportFunction, PointerGetDatum(ssup));
}
- else
+
+ if (ssup->comparator == NULL)
{
+ Oid sortFunction;
+
+ sortFunction = get_opfamily_proc(opfamily, opcintype, opcintype,
+ BTORDER_PROC);
+
+ if (!OidIsValid(sortFunction))
+ elog(ERROR, "missing support function %d(%u,%u) in opfamily %u",
+ BTORDER_PROC, opcintype, opcintype, opfamily);
+
/* We'll use a shim to call the old-style btree comparator */
PrepareSortSupportComparisonShim(sortFunction, ssup);
}
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index f46460a..07d24d4 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -50,8 +50,6 @@ extern Oid get_opfamily_member(Oid opfamily, Oid lefttype, Oid righttype,
int16 strategy);
extern bool get_ordering_op_properties(Oid opno,
Oid *opfamily, Oid *opcintype, int16 *strategy);
-extern bool get_sort_function_for_ordering_op(Oid opno, Oid *sortfunc,
- bool *issupport, bool *reverse);
extern Oid get_equality_op_for_ordering_op(Oid opno, bool *reverse);
extern Oid get_ordering_op_for_equality_op(Oid opno, bool use_lhs_type);
extern List *get_mergejoin_opfamilies(Oid opno);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
