Re: TODO Item - Return compressed length of TOAST datatypes

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