Обсуждение: ExecBuildGroupingEqual versus collations

In pursuit of places that might not be on board with non-default
collations, I tried sticking

    Assert(PG_GET_COLLATION() == C_COLLATION_OID);

into nameeq() and the other name comparison functions, in my build with
type name's typcollation changed to "C".  I'm down to one place in the
core regression tests that falls over because of that:
ExecBuildGroupingEqual() figures it can get away with

        InitFunctionCallInfoData(*fcinfo, finfo, 2,
                                 InvalidOid, NULL, NULL);

i.e. passing collation = InvalidOid to the type-specific equality
functions.

Now, it's certainly true that nameeq() doesn't need a collation spec
today, any more than texteq() does, because both types legislate that
equality is bitwise.  But if we leave ExecBuildGroupingEqual like this,
we're mandating that no type anywhere, ever, can have a
collation-dependent notion of equality.  Is that really a restriction
we're comfortable with?  citext is sort of the poster child here,
because it already wishes it could have collation-dependent equality.

We'd have to do some work to pass a reasonable collation choice through
to this code from wherever we have the info available, but I don't
think it'd be a huge amount of work.

BTW, this case is perhaps a bit of a counterexample to Peter's idea about
doing collation lookups earlier: if we change this, we'd presumably have
to do collation lookup right here, even though we know very well that
common types' equality functions won't use the information.

In any case, I think we need a policy decision about whether it's our
intent to allow collation-dependent equality or not.

Comments?

            regards, tom lane

Isaac Morland <isaac.morland@gmail.com> writes:
> On Fri, 14 Dec 2018 at 14:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Now, it's certainly true that nameeq() doesn't need a collation spec
>> today, any more than texteq() does, because both types legislate that
>> equality is bitwise.  But if we leave ExecBuildGroupingEqual like this,
>> we're mandating that no type anywhere, ever, can have a
>> collation-dependent notion of equality.  Is that really a restriction
>> we're comfortable with?  citext is sort of the poster child here,
>> because it already wishes it could have collation-dependent equality.

> There already seems to be a policy that individual types are not allowed to
> have their own concepts of equality:
> postgres=> select 'NaN'::float = 'NaN'::float;
>  ?column?
> ----------
>  t
> (1 row)

> According to the IEEE floating point specification, this should be f not t.

I don't think that's a particularly relevant counter-example.  The problem
with following the IEEE spec for that is that it breaks pretty much
everybody's notion of equality, in particular the reflexive axiom (A=A for
any A), which is one of the axioms needed for btrees to work.  That does
*not* preclude allowing non-bitwise-equal values to be considered equal,
which is the case that citext would like --- and, indeed, a case that IEEE
math also requires (0 = -0), and which we do support for floats.

            regards, tom lane

Hi,

On 2018-12-14 14:25:30 -0500, Tom Lane wrote:
> In pursuit of places that might not be on board with non-default
> collations, I tried sticking
> 
>     Assert(PG_GET_COLLATION() == C_COLLATION_OID);
> 
> into nameeq() and the other name comparison functions, in my build with
> type name's typcollation changed to "C".  I'm down to one place in the
> core regression tests that falls over because of that:
> ExecBuildGroupingEqual() figures it can get away with
> 
>         InitFunctionCallInfoData(*fcinfo, finfo, 2,
>                                  InvalidOid, NULL, NULL);
> 
> i.e. passing collation = InvalidOid to the type-specific equality
> functions.

Yea, I just cargo-culted that behaviour forward, the code previously
did:

bool
execTuplesMatch(TupleTableSlot *slot1,
                TupleTableSlot *slot2,
                int numCols,
                AttrNumber *matchColIdx,
                FmgrInfo *eqfunctions,
                MemoryContext evalContext)
...
        /* Apply the type-specific equality function */

        if (!DatumGetBool(FunctionCall2(&eqfunctions[i],
                                        attr1, attr2)))
        {
            result = false;     /* they aren't equal */
            break;
        }


> Now, it's certainly true that nameeq() doesn't need a collation spec
> today, any more than texteq() does, because both types legislate that
> equality is bitwise.  But if we leave ExecBuildGroupingEqual like this,
> we're mandating that no type anywhere, ever, can have a
> collation-dependent notion of equality.  Is that really a restriction
> we're comfortable with?  citext is sort of the poster child here,
> because it already wishes it could have collation-dependent equality.

Don't we rely on that in other places too?


> We'd have to do some work to pass a reasonable collation choice through
> to this code from wherever we have the info available, but I don't
> think it'd be a huge amount of work.

Yea, I think it shouldn't be too hard for most callers.

I think one issue here is that it's not clear how it'd be sensible to
have collation dependent equality for cases where we don't have a real
way to override the collation for an operation.  It's not too hard to
imagine adding something to GROUP BY, but set operations seem harder.


> BTW, this case is perhaps a bit of a counterexample to Peter's idea about
> doing collation lookups earlier: if we change this, we'd presumably have
> to do collation lookup right here, even though we know very well that
> common types' equality functions won't use the information.

Hm.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2018-12-14 14:25:30 -0500, Tom Lane wrote:
>> Now, it's certainly true that nameeq() doesn't need a collation spec
>> today, any more than texteq() does, because both types legislate that
>> equality is bitwise.  But if we leave ExecBuildGroupingEqual like this,
>> we're mandating that no type anywhere, ever, can have a
>> collation-dependent notion of equality.  Is that really a restriction
>> we're comfortable with?  citext is sort of the poster child here,
>> because it already wishes it could have collation-dependent equality.

> Don't we rely on that in other places too?

Possibly; the regression tests probably don't stress type "name" too hard,
so my Assert might well not be finding some other code paths that do
likewise.  The question at hand is whether we're going to say those are
bugs to be fixed, or that it's OK as-is.

> I think one issue here is that it's not clear how it'd be sensible to
> have collation dependent equality for cases where we don't have a real
> way to override the collation for an operation.  It's not too hard to
> imagine adding something to GROUP BY, but set operations seem harder.

Yeah, fair point.  But we've got comparable issues with respect to
which equality operator to use in the first place, for a lot of these
constructs, cf
https://www.postgresql.org/message-id/10492.1531515255@sss.pgh.pa.us
(which I've not made any further progress on).  Unless you want to
throw up your hands and say that *all* equality is bitwise, we need
to think about ways to deal with that.

            regards, tom lane

I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2018-12-14 14:25:30 -0500, Tom Lane wrote:
>>> Now, it's certainly true that nameeq() doesn't need a collation spec
>>> today, any more than texteq() does, because both types legislate that
>>> equality is bitwise.  But if we leave ExecBuildGroupingEqual like this,
>>> we're mandating that no type anywhere, ever, can have a
>>> collation-dependent notion of equality.  Is that really a restriction
>>> we're comfortable with?  citext is sort of the poster child here,
>>> because it already wishes it could have collation-dependent equality.

>> Don't we rely on that in other places too?

> Possibly; the regression tests probably don't stress type "name" too hard,
> so my Assert might well not be finding some other code paths that do
> likewise.

To investigate this a bit more, I added a similar Assert in texteq(),
and also grepped for places that were using FunctionCall2 to call
equality functions.  The attached patch shows what I had to change
to get check-world to pass.  It's possible that there are one or two
more places I missed --- two of these changes were found by grepping
but were *not* exposed by regression testing.  But this ought to be
a pretty good indication of the scope of the problem.

There are a couple of places touched here that know they are invoking
texteq specifically, so we wouldn't really need to change them to have
a working feature.  In all I found six places that we'd need to deal with
if we want to support collation-dependent equality.

A possible medium-term compromise is to pass DEFAULT_COLLATION_OID,
as I've done here, so that at least a collation-examining equality
function won't fall over.  A variant of that is to pass C_COLLATION_OID,
which presumably would be faster to execute in many cases; it would
also have the advantage of being database-independent, which might
be a good thing in execReplication.c for instance.

Or we could just decide that collation-dependent equality is a bridge
too far for today.  I'm not hugely excited about pursuing it myself,
I just wanted to get a handle on how badly broken it is.

            regards, tom lane

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index d9087ca..fa00a36 100644
*** a/src/backend/executor/execExpr.c
--- b/src/backend/executor/execExpr.c
***************
*** 32,37 ****
--- 32,38 ----

  #include "access/nbtree.h"
  #include "catalog/objectaccess.h"
+ #include "catalog/pg_collation.h"
  #include "catalog/pg_type.h"
  #include "executor/execExpr.h"
  #include "executor/nodeSubplan.h"
*************** ExecBuildGroupingEqual(TupleDesc ldesc,
*** 3393,3399 ****
          fmgr_info(foid, finfo);
          fmgr_info_set_expr(NULL, finfo);
          InitFunctionCallInfoData(*fcinfo, finfo, 2,
!                                  InvalidOid, NULL, NULL);

          /* left arg */
          scratch.opcode = EEOP_INNER_VAR;
--- 3394,3400 ----
          fmgr_info(foid, finfo);
          fmgr_info_set_expr(NULL, finfo);
          InitFunctionCallInfoData(*fcinfo, finfo, 2,
!                                  DEFAULT_COLLATION_OID, NULL, NULL);

          /* left arg */
          scratch.opcode = EEOP_INNER_VAR;
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 5bd3bbc..cd40071 100644
*** a/src/backend/executor/execReplication.c
--- b/src/backend/executor/execReplication.c
***************
*** 17,22 ****
--- 17,23 ----
  #include "access/relscan.h"
  #include "access/transam.h"
  #include "access/xact.h"
+ #include "catalog/pg_collation.h"
  #include "commands/trigger.h"
  #include "executor/executor.h"
  #include "nodes/nodeFuncs.h"
*************** tuple_equals_slot(TupleDesc desc, HeapTu
*** 265,273 ****
                       errmsg("could not identify an equality operator for type %s",
                              format_type_be(att->atttypid))));

!         if (!DatumGetBool(FunctionCall2(&typentry->eq_opr_finfo,
!                                         values[attrnum],
!                                         slot->tts_values[attrnum])))
              return false;
      }

--- 266,275 ----
                       errmsg("could not identify an equality operator for type %s",
                              format_type_be(att->atttypid))));

!         if (!DatumGetBool(FunctionCall2Coll(&typentry->eq_opr_finfo,
!                                             DEFAULT_COLLATION_OID,
!                                             values[attrnum],
!                                             slot->tts_values[attrnum])))
              return false;
      }

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index daf56cd..50e0f58 100644
*** a/src/backend/executor/nodeAgg.c
--- b/src/backend/executor/nodeAgg.c
***************
*** 219,224 ****
--- 219,225 ----
  #include "access/htup_details.h"
  #include "catalog/objectaccess.h"
  #include "catalog/pg_aggregate.h"
+ #include "catalog/pg_collation.h"
  #include "catalog/pg_proc.h"
  #include "catalog/pg_type.h"
  #include "executor/executor.h"
*************** process_ordered_aggregate_single(AggStat
*** 748,762 ****
          /*
           * If DISTINCT mode, and not distinct from prior, skip it.
           *
!          * Note: we assume equality functions don't care about collation.
           */
          if (isDistinct &&
              haveOldVal &&
              ((oldIsNull && *isNull) ||
               (!oldIsNull && !*isNull &&
                oldAbbrevVal == newAbbrevVal &&
!               DatumGetBool(FunctionCall2(&pertrans->equalfnOne,
!                                          oldVal, *newVal)))))
          {
              /* equal to prior, so forget this one */
              if (!pertrans->inputtypeByVal && !*isNull)
--- 749,765 ----
          /*
           * If DISTINCT mode, and not distinct from prior, skip it.
           *
!          * Note: most equality functions don't care about collation.  For
!          * those that do, just select DEFAULT_COLLATION_OID.
           */
          if (isDistinct &&
              haveOldVal &&
              ((oldIsNull && *isNull) ||
               (!oldIsNull && !*isNull &&
                oldAbbrevVal == newAbbrevVal &&
!               DatumGetBool(FunctionCall2Coll(&pertrans->equalfnOne,
!                                              DEFAULT_COLLATION_OID,
!                                              oldVal, *newVal)))))
          {
              /* equal to prior, so forget this one */
              if (!pertrans->inputtypeByVal && !*isNull)
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 84a1a91..b136650 100644
*** a/src/backend/executor/nodeSubplan.c
--- b/src/backend/executor/nodeSubplan.c
***************
*** 30,35 ****
--- 30,36 ----
  #include <math.h>

  #include "access/htup_details.h"
+ #include "catalog/pg_collation.h"
  #include "executor/executor.h"
  #include "executor/nodeSubplan.h"
  #include "nodes/makefuncs.h"
*************** execTuplesUnequal(TupleTableSlot *slot1,
*** 671,678 ****

          /* Apply the type-specific equality function */

!         if (!DatumGetBool(FunctionCall2(&eqfunctions[i],
!                                         attr1, attr2)))
          {
              result = true;        /* they are unequal */
              break;
--- 672,680 ----

          /* Apply the type-specific equality function */

!         if (!DatumGetBool(FunctionCall2Coll(&eqfunctions[i],
!                                             DEFAULT_COLLATION_OID,
!                                             attr1, attr2)))
          {
              result = true;        /* they are unequal */
              break;
diff --git a/src/backend/utils/adt/orderedsetaggs.c b/src/backend/utils/adt/orderedsetaggs.c
index 1b21da8..5fed920 100644
*** a/src/backend/utils/adt/orderedsetaggs.c
--- b/src/backend/utils/adt/orderedsetaggs.c
***************
*** 17,22 ****
--- 17,23 ----
  #include <math.h>

  #include "catalog/pg_aggregate.h"
+ #include "catalog/pg_collation.h"
  #include "catalog/pg_operator.h"
  #include "catalog/pg_type.h"
  #include "executor/executor.h"
*************** mode_final(PG_FUNCTION_ARGS)
*** 1084,1090 ****
              last_abbrev_val = abbrev_val;
          }
          else if (abbrev_val == last_abbrev_val &&
!                  DatumGetBool(FunctionCall2(equalfn, val, last_val)))
          {
              /* value equal to previous value, count it */
              if (last_val_is_mode)
--- 1085,1092 ----
              last_abbrev_val = abbrev_val;
          }
          else if (abbrev_val == last_abbrev_val &&
!                  DatumGetBool(FunctionCall2Coll(equalfn, DEFAULT_COLLATION_OID,
!                                                 val, last_val)))
          {
              /* value equal to previous value, count it */
              if (last_val_is_mode)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index cdda860..a77d610 100644
*** a/src/backend/utils/adt/ri_triggers.c
--- b/src/backend/utils/adt/ri_triggers.c
*************** ri_AttributesEqual(Oid eq_opr, Oid typei
*** 2975,2985 ****
      }

      /*
!      * Apply the comparison operator.  We assume it doesn't care about
!      * collations.
       */
!     return DatumGetBool(FunctionCall2(&entry->eq_opr_finfo,
!                                       oldvalue, newvalue));
  }

  /* ----------
--- 2975,2985 ----
      }

      /*
!      * Apply the comparison operator.
       */
!     return DatumGetBool(FunctionCall2Coll(&entry->eq_opr_finfo,
!                                           DEFAULT_COLLATION_OID,
!                                           oldvalue, newvalue));
  }

  /* ----------
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 0fd3b15..c3c89cf 100644
*** a/src/backend/utils/adt/varlena.c
--- b/src/backend/utils/adt/varlena.c
*************** texteq(PG_FUNCTION_ARGS)
*** 1646,1651 ****
--- 1646,1653 ----
      Size        len1,
                  len2;

+     Assert(PG_GET_COLLATION() != 0);
+
      /*
       * Since we only care about equality or not-equality, we can avoid all the
       * expense of strcoll() here, and just do bitwise comparison.  In fact, we
*************** split_text(PG_FUNCTION_ARGS)
*** 4281,4289 ****
  static bool
  text_isequal(text *txt1, text *txt2)
  {
!     return DatumGetBool(DirectFunctionCall2(texteq,
!                                             PointerGetDatum(txt1),
!                                             PointerGetDatum(txt2)));
  }

  /*
--- 4283,4292 ----
  static bool
  text_isequal(text *txt1, text *txt2)
  {
!     return DatumGetBool(DirectFunctionCall2Coll(texteq,
!                                                 DEFAULT_COLLATION_OID,
!                                                 PointerGetDatum(txt1),
!                                                 PointerGetDatum(txt2)));
  }

  /*
diff --git a/src/backend/utils/cache/catcache.c b/src/backend/utils/cache/catcache.c
index b31fd5a..2ac6dd6 100644
*** a/src/backend/utils/cache/catcache.c
--- b/src/backend/utils/cache/catcache.c
***************
*** 22,27 ****
--- 22,28 ----
  #include "access/tuptoaster.h"
  #include "access/valid.h"
  #include "access/xact.h"
+ #include "catalog/pg_collation.h"
  #include "catalog/pg_operator.h"
  #include "catalog/pg_type.h"
  #include "miscadmin.h"
*************** int4hashfast(Datum datum)
*** 181,187 ****
  static bool
  texteqfast(Datum a, Datum b)
  {
!     return DatumGetBool(DirectFunctionCall2(texteq, a, b));
  }

  static uint32
--- 182,188 ----
  static bool
  texteqfast(Datum a, Datum b)
  {
!     return DatumGetBool(DirectFunctionCall2Coll(texteq, DEFAULT_COLLATION_OID, a, b));
  }

  static uint32
*************** CatalogCacheInitializeCache(CatCache *ca
*** 1014,1021 ****
          /* Fill in sk_strategy as well --- always standard equality */
          cache->cc_skey[i].sk_strategy = BTEqualStrategyNumber;
          cache->cc_skey[i].sk_subtype = InvalidOid;
!         /* Currently, there are no catcaches on collation-aware data types */
!         cache->cc_skey[i].sk_collation = InvalidOid;

          CACHE4_elog(DEBUG2, "CatalogCacheInitializeCache %s %d %p",
                      cache->cc_relname,
--- 1015,1022 ----
          /* Fill in sk_strategy as well --- always standard equality */
          cache->cc_skey[i].sk_strategy = BTEqualStrategyNumber;
          cache->cc_skey[i].sk_subtype = InvalidOid;
!         /* If a catcache key requires a collation, it must be C collation */
!         cache->cc_skey[i].sk_collation = C_COLLATION_OID;

          CACHE4_elog(DEBUG2, "CatalogCacheInitializeCache %s %d %p",
                      cache->cc_relname,

On 14/12/2018 20:25, Tom Lane wrote:
> Now, it's certainly true that nameeq() doesn't need a collation spec
> today, any more than texteq() does, because both types legislate that
> equality is bitwise.  But if we leave ExecBuildGroupingEqual like this,
> we're mandating that no type anywhere, ever, can have a
> collation-dependent notion of equality.  Is that really a restriction
> we're comfortable with?  citext is sort of the poster child here,
> because it already wishes it could have collation-dependent equality.

I have just posted my "insensitive collations" patch that contains code
to fix this.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services