Re: TODO Item - Return compressed length of TOAST datatypes - Mailing list pgsql-patches

Neil Conway wrote:
> Bruce Momjian wrote:
>
>> + /* +  * Return the length of a datum, possibly compressed
>> +  */
>> + Datum
>> + pg_column_size(PG_FUNCTION_ARGS)
>> + {
>> +     Datum            value = PG_GETARG_DATUM(0);
>> +     int                result;
>> + +     /*    fn_extra stores the fixed column length, or -1 for
>> varlena. */
>> +     if (fcinfo->flinfo->fn_extra == NULL)    /* first call? */
>> +     {
>> +         /* On the first call lookup the datatype of the supplied
>> argument */
>
>
> [...]
>
> Is this optimization worth the code complexity?
>

I didn't performance test it, but the idea of hammering the catalogs for
each value to be processed seemed a bad thing.

>> +         tp = SearchSysCache(TYPEOID,
>> +                             ObjectIdGetDatum(argtypeid),
>> +                             0, 0, 0);
>> +         if (!HeapTupleIsValid(tp))
>> +         {
>> +             /* Oid not in pg_type, should never happen. */
>> +             ereport(ERROR,
>> +                     (errcode(ERRCODE_INTERNAL_ERROR),
>> +                      errmsg("invalid typid: %u", argtypeid)));
>> +         }
>
>
> elog(ERROR) is usually used for "can't happen" errors; also, the usual
> error message in this scenario is "cache lookup failed [...]". Perhaps
> better to use get_typlen() here, anyway.
>

Ahem,..ah yes, get_typlen()! (ducks)- that is clearly much better!

I have attached a little change to varlena.c that uses it. I left the
ereport as it was, but am not fussed about it either way.

BTW - Bruce, I like the changes, makes the beast more friendly to use!

Best wishes

Mark


Index: src/backend/utils/adt/varlena.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/varlena.c,v
retrieving revision 1.125
diff -c -r1.125 varlena.c
*** src/backend/utils/adt/varlena.c    6 Jul 2005 19:02:52 -0000    1.125
--- src/backend/utils/adt/varlena.c    7 Jul 2005 02:52:50 -0000
***************
*** 28,34 ****
  #include "utils/builtins.h"
  #include "utils/lsyscache.h"
  #include "utils/pg_locale.h"
- #include "utils/syscache.h"


  typedef struct varlena unknown;
--- 28,33 ----
***************
*** 2364,2385 ****
      {
          /* On the first call lookup the datatype of the supplied argument */
          Oid                argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
!         HeapTuple        tp;
!         int                typlen;

!         tp = SearchSysCache(TYPEOID,
!                             ObjectIdGetDatum(argtypeid),
!                             0, 0, 0);
!         if (!HeapTupleIsValid(tp))
          {
              /* Oid not in pg_type, should never happen. */
              ereport(ERROR,
                      (errcode(ERRCODE_INTERNAL_ERROR),
!                      errmsg("invalid typid: %u", argtypeid)));
          }
!
!         typlen = ((Form_pg_type)GETSTRUCT(tp))->typlen;
!         ReleaseSysCache(tp);
          fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
                                                        sizeof(int));
          *(int *)fcinfo->flinfo->fn_extra = typlen;
--- 2363,2379 ----
      {
          /* On the first call lookup the datatype of the supplied argument */
          Oid                argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
!         int                typlen    = get_typlen(argtypeid);

!
!         if (typlen == 0)
          {
              /* Oid not in pg_type, should never happen. */
              ereport(ERROR,
                      (errcode(ERRCODE_INTERNAL_ERROR),
!                      errmsg("cache lookup failed for type %u", argtypeid)));
          }
!
          fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
                                                        sizeof(int));
          *(int *)fcinfo->flinfo->fn_extra = typlen;