Re: Bug in pg_describe_object, patch v2 - Mailing list pgsql-hackers

Andreas Karlsson <andreas@proxel.se> writes:
> On Sat, 2011-01-15 at 10:36 -0500, Tom Lane wrote:
>> But I can read the handwriting on the wall: if I want this done right,
>> I'm going to have to do it myself.

> Do I understand you correctly if I interpret what you would like to see
> is the same format used now in these cases?

Attached is a patch that does what I would consider an acceptable job of
printing these datatypes only when they're interesting.  I still think
that this is largely a waste of code, but if people want it, this is
what to do.  Testing this in the regression database, it fires on
(a) the entries where a binary-compatible hash function is used, and
(b) all the entries associated with the GIN operator family array_ops.
The latter happens because we've more or less arbitrarily crammed a
bunch of opclasses into the same opfamily.

One other point here is that I find messages like this a mite
unreadable:

function 1 (oidvector[], oidvector[]) btoidvectorcmp(oidvector,oidvector) of operator family array_ops for access
methodgin 

If we were to go with this, I'd be strongly tempted to rearrange all
four of the messages involved to put the operator or function name
at the end, eg

function 1 (oidvector[], oidvector[]) of operator family array_ops for access method gin:
btoidvectorcmp(oidvector,oidvector)

Comments?

            regards, tom lane

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index ec8eb74954a9cc0ec3623dc42dfed462dc1a3533..d02b58dcc2933a278959727f55d93e1e9101c1f1 100644
*** a/src/backend/catalog/dependency.c
--- b/src/backend/catalog/dependency.c
*************** getObjectDescription(const ObjectAddress
*** 2337,2351 ****
                  initStringInfo(&opfam);
                  getOpFamilyDescription(&opfam, amopForm->amopfamily);

!                 /*
!                  * translator: %d is the operator strategy (a number), the
!                  * first %s is the textual form of the operator, and the
!                  * second %s is the description of the operator family.
!                  */
!                 appendStringInfo(&buffer, _("operator %d %s of %s"),
!                                  amopForm->amopstrategy,
!                                  format_operator(amopForm->amopopr),
!                                  opfam.data);
                  pfree(opfam.data);

                  systable_endscan(amscan);
--- 2337,2372 ----
                  initStringInfo(&opfam);
                  getOpFamilyDescription(&opfam, amopForm->amopfamily);

!                 if (opfamily_op_has_default_types(amopForm->amopfamily,
!                                                   amopForm->amoplefttype,
!                                                   amopForm->amoprighttype,
!                                                   amopForm->amopopr))
!                 {
!                     /*
!                      * translator: %d is the operator strategy (a number), the
!                      * first %s is the textual form of the operator, and the
!                      * second %s is the description of the operator family.
!                      */
!                     appendStringInfo(&buffer, _("operator %d %s of %s"),
!                                      amopForm->amopstrategy,
!                                      format_operator(amopForm->amopopr),
!                                      opfam.data);
!                 }
!                 else
!                 {
!                     /*
!                      * translator: %d is the operator strategy (a number), the
!                      * first two %s's are data type names, the third %s is the
!                      * textual form of the operator, and the last %s is the
!                      * description of the operator family.
!                      */
!                     appendStringInfo(&buffer, _("operator %d (%s, %s) %s of %s"),
!                                      amopForm->amopstrategy,
!                                      format_type_be(amopForm->amoplefttype),
!                                      format_type_be(amopForm->amoprighttype),
!                                      format_operator(amopForm->amopopr),
!                                      opfam.data);
!                 }
                  pfree(opfam.data);

                  systable_endscan(amscan);
*************** getObjectDescription(const ObjectAddress
*** 2384,2398 ****
                  initStringInfo(&opfam);
                  getOpFamilyDescription(&opfam, amprocForm->amprocfamily);

!                 /*
!                  * translator: %d is the function number, the first %s is the
!                  * textual form of the function with arguments, and the second
!                  * %s is the description of the operator family.
!                  */
!                 appendStringInfo(&buffer, _("function %d %s of %s"),
!                                  amprocForm->amprocnum,
!                                  format_procedure(amprocForm->amproc),
!                                  opfam.data);
                  pfree(opfam.data);

                  systable_endscan(amscan);
--- 2405,2441 ----
                  initStringInfo(&opfam);
                  getOpFamilyDescription(&opfam, amprocForm->amprocfamily);

!                 if (opfamily_proc_has_default_types(amprocForm->amprocfamily,
!                                                     amprocForm->amproclefttype,
!                                                     amprocForm->amprocrighttype,
!                                                     amprocForm->amproc))
!                 {
!                     /*
!                      * translator: %d is the function number, the first %s is
!                      * the textual form of the function with arguments, and
!                      * the second %s is the description of the operator
!                      * family.
!                      */
!                     appendStringInfo(&buffer, _("function %d %s of %s"),
!                                      amprocForm->amprocnum,
!                                      format_procedure(amprocForm->amproc),
!                                      opfam.data);
!                 }
!                 else
!                 {
!                     /*
!                      * translator: %d is the function number, the first two
!                      * %s's are data type names, the third %s is the textual
!                      * form of the function with arguments, and the last %s is
!                      * the description of the operator family.
!                      */
!                     appendStringInfo(&buffer, _("function %d (%s, %s) %s of %s"),
!                                      amprocForm->amprocnum,
!                                      format_type_be(amprocForm->amproclefttype),
!                                      format_type_be(amprocForm->amprocrighttype),
!                                      format_procedure(amprocForm->amproc),
!                                      opfam.data);
!                 }
                  pfree(opfam.data);

                  systable_endscan(amscan);
diff --git a/src/backend/commands/opclasscmds.c b/src/backend/commands/opclasscmds.c
index 662b9420387f90bf3c9b6e656ed7b90583319d98..48b641f35a1ab901c092feedc53b25abf4d329e3 100644
*** a/src/backend/commands/opclasscmds.c
--- b/src/backend/commands/opclasscmds.c
*************** static void AlterOpFamilyDrop(List *opfa
*** 69,74 ****
--- 69,75 ----
  static void processTypesSpec(List *args, Oid *lefttype, Oid *righttype);
  static void assignOperTypes(OpFamilyMember *member, Oid amoid, Oid typeoid);
  static void assignProcTypes(OpFamilyMember *member, Oid amoid, Oid typeoid);
+ static Oid    get_opfamily_input_type(Oid opfamily, Oid amoid);
  static void addFamilyMember(List **list, OpFamilyMember *member, bool isProc);
  static void storeOperators(List *opfamilyname, Oid amoid,
                 Oid opfamilyoid, Oid opclassoid,
*************** assignProcTypes(OpFamilyMember *member,
*** 1209,1214 ****
--- 1210,1374 ----
  }

  /*
+  * Test whether a pg_amop entry has the default lefttype/righttype that
+  * would be deduced from its opfamily and operator.
+  *
+  * This is here because it must match the logic in assignOperTypes.
+  */
+ bool
+ opfamily_op_has_default_types(Oid opfamily, Oid lefttype, Oid righttype,
+                               Oid operatorId)
+ {
+     bool        result = false;
+     Operator    optup;
+     Form_pg_operator opform;
+
+     /* Fetch the operator definition */
+     optup = SearchSysCache1(OPEROID, ObjectIdGetDatum(operatorId));
+     if (optup == NULL)
+         elog(ERROR, "cache lookup failed for operator %u", operatorId);
+     opform = (Form_pg_operator) GETSTRUCT(optup);
+
+     /*
+      * Default types are the operator's input types
+      */
+     if (opform->oprkind == 'b' &&
+         lefttype == opform->oprleft &&
+         righttype == opform->oprright)
+         result = true;
+
+     ReleaseSysCache(optup);
+
+     return result;
+ }
+
+ /*
+  * Test whether a pg_amproc entry has the default lefttype/righttype that
+  * would be deduced from its opfamily and procedure.
+  *
+  * This is here because it must match the logic in assignProcTypes.
+  */
+ bool
+ opfamily_proc_has_default_types(Oid opfamily, Oid lefttype, Oid righttype,
+                                 Oid procedureId)
+ {
+     bool        result = false;
+     Oid            amoid;
+     Oid            opcintype;
+     HeapTuple    opfTup;
+     Form_pg_opfamily opfForm;
+     HeapTuple    proctup;
+     Form_pg_proc procform;
+
+     /* We need to know the opfamily's AM to know which rules apply */
+     opfTup = SearchSysCache1(OPFAMILYOID, ObjectIdGetDatum(opfamily));
+     if (!HeapTupleIsValid(opfTup))
+         elog(ERROR, "cache lookup failed for opfamily %u", opfamily);
+     opfForm = (Form_pg_opfamily) GETSTRUCT(opfTup);
+
+     amoid = opfForm->opfmethod;
+
+     ReleaseSysCache(opfTup);
+
+     /* Fetch the procedure definition */
+     proctup = SearchSysCache1(PROCOID, ObjectIdGetDatum(procedureId));
+     if (proctup == NULL)
+         elog(ERROR, "cache lookup failed for function %u", procedureId);
+     procform = (Form_pg_proc) GETSTRUCT(proctup);
+
+     if (amoid == BTREE_AM_OID)
+     {
+         /* Default types are the comparison procedure's input types */
+         if (procform->pronargs == 2 &&
+             lefttype == procform->proargtypes.values[0] &&
+             righttype == procform->proargtypes.values[1])
+             result = true;
+     }
+     else if (amoid == HASH_AM_OID)
+     {
+         /* Default types are the hash procedure's (single) input type */
+         if (procform->pronargs == 1 &&
+             lefttype == procform->proargtypes.values[0] &&
+             righttype == procform->proargtypes.values[0])
+             result = true;
+     }
+     else
+     {
+         /*
+          * For GiST and GIN, the default (and actually the only useful case)
+          * is for lefttype/righttype to be the opcintype of the associated
+          * opclass.  So first we have to look that up.  The support function's
+          * signature may not contain any instance of the opcintype.
+          */
+         opcintype = get_opfamily_input_type(opfamily, amoid);
+         if (OidIsValid(opcintype) &&
+             lefttype == opcintype &&
+             righttype == opcintype)
+             result = true;
+     }
+
+     ReleaseSysCache(proctup);
+
+     return result;
+ }
+
+ /*
+  * If the specified opfamily contains exactly one opclass, return that
+  * opclass's opcintype.  Otherwise, return InvalidOid.
+  *
+  * GiST and GIN indexes have no particular use for opfamilies, since there
+  * is no expectation of common behavior across different opclasses.  So
+  * generally this can be expected to succeed on a GiST or GIN opfamily.
+  */
+ static Oid
+ get_opfamily_input_type(Oid opfamily, Oid amoid)
+ {
+     Oid            result = InvalidOid;
+     int            nmatch = 0;
+     Relation    rel;
+     ScanKeyData skey[1];
+     SysScanDesc scan;
+     HeapTuple    tup;
+
+     /*
+      * We must scan through all the opclasses available for the access method,
+      * since there's no index that would let us look at just those for the
+      * opfamily.
+      */
+     rel = heap_open(OperatorClassRelationId, AccessShareLock);
+
+     ScanKeyInit(&skey[0],
+                 Anum_pg_opclass_opcmethod,
+                 BTEqualStrategyNumber, F_OIDEQ,
+                 ObjectIdGetDatum(amoid));
+
+     scan = systable_beginscan(rel, OpclassAmNameNspIndexId, true,
+                               SnapshotNow, 1, skey);
+
+     while (HeapTupleIsValid(tup = systable_getnext(scan)))
+     {
+         Form_pg_opclass opclass = (Form_pg_opclass) GETSTRUCT(tup);
+
+         if (opclass->opcfamily == opfamily)
+         {
+             if (++nmatch > 1)
+             {
+                 /* more than one opclass in family, so fail */
+                 result = InvalidOid;
+                 break;
+             }
+             result = opclass->opcintype;
+         }
+     }
+
+     systable_endscan(scan);
+
+     heap_close(rel, AccessShareLock);
+
+     return result;
+ }
+
+ /*
   * Add a new family member to the appropriate list, after checking for
   * duplicated strategy or proc number.
   */
diff --git a/src/include/commands/defrem.h b/src/include/commands/defrem.h
index 01f271bff43f3c3d8560cb9169e85afe9545d41b..fb48530c32a9f93aea106bf9ce2147b99b8d89e4 100644
*** a/src/include/commands/defrem.h
--- b/src/include/commands/defrem.h
*************** extern void AlterOpFamilyNamespace(List
*** 106,111 ****
--- 106,115 ----
  extern Oid get_am_oid(const char *amname, bool missing_ok);
  extern Oid get_opclass_oid(Oid amID, List *opclassname, bool missing_ok);
  extern Oid get_opfamily_oid(Oid amID, List *opfamilyname, bool missing_ok);
+ extern bool opfamily_op_has_default_types(Oid opfamily, Oid lefttype, Oid righttype,
+                                           Oid operatorId);
+ extern bool opfamily_proc_has_default_types(Oid opfamily, Oid lefttype, Oid righttype,
+                                             Oid procedureId);

  /* commands/tsearchcmds.c */
  extern void DefineTSParser(List *names, List *parameters);