Re: TODO Item - Return compressed length of TOAST datatypes (WIP)

I thought I would have a look at:

(Datatypes) Add function to return compressed length of TOAST data values.

A WIP patch is attached for comment (wanted to check I hadn't bitten off
more than I could chew *before* asking questions!).

A few questions come to mind:

1) The name - I have called it 'toast_compressed_length'. Seems longish
- I'm wondering if just 'compressed_length' is ok?

2) What should be returned for toasted data that is not compressed (or
plain stored data for that matter)? The WIP patch just gives the
uncompressed size (I notice I may need to subtract VARHDRSZ in some cases).

3) What should be returned for non-varlena types? The WIP patch is
treating everything as a varlena, so is returning incorrect information
for that case.

4) The builtin is declared as immutable - I am not so sure about that (I
am wondering if altering a column's storage from MAIN -> EXTENDED and
then updating the column to be itself will fool it).

5) Any multi-byte locale considerations?

regards

Mark

Mark Kirkwood <markir(at)paradise(dot)net(dot)nz> writes:
> I thought I would have a look at:
> (Datatypes) Add function to return compressed length of TOAST data values.

My recollection of that discussion is that we just wanted something
that would return the actual VARSIZE() of the datum. You're building
something way too confusing ...

A more interesting point is that the way you've declared the function,
it will only work on text values, which is pretty limiting. Ideally
we'd define this thing as "pg_datum_length(any-varlena-type) returns int"
but there is no pseudotype corresponding to "any varlena type".

I was thinking about this the other day in connection with my proposal
to make something that could return the TOAST value OID of an
out-of-line datum. I think the only non-restrictive way to do it would
be to declare the function as taking "any", and then add a runtime check
using the get_fn_expr_argtype() mechanism to test that what you've been
given is in fact a varlena datatype.

regards, tom lane

Tom Lane wrote:
> Mark Kirkwood <markir(at)paradise(dot)net(dot)nz> writes:
>
>>I thought I would have a look at:
>>(Datatypes) Add function to return compressed length of TOAST data values.
>
>
> My recollection of that discussion is that we just wanted something
> that would return the actual VARSIZE() of the datum. You're building
> something way too confusing ...
>

I was guessing a little about exactly what was wanted - for some reason
I couldn't access the mail archives for the last few days (however,
can now).

> A more interesting point is that the way you've declared the function,
> it will only work on text values, which is pretty limiting. Ideally
> we'd define this thing as "pg_datum_length(any-varlena-type) returns int"
> but there is no pseudotype corresponding to "any varlena type".
>
> I was thinking about this the other day in connection with my proposal
> to make something that could return the TOAST value OID of an
> out-of-line datum. I think the only non-restrictive way to do it would
> be to declare the function as taking "any", and then add a runtime check
> using the get_fn_expr_argtype() mechanism to test that what you've been
> given is in fact a varlena datatype.
>

Yes - was thinking I needed to check if the Datum was a varlena (and
didn't know how to do it...), so thanks for the pointer!

With these thoughts in mind, I'll have another go :-)

Cheers

Mark

The next iteration -

Hopefully I have got the idea basically right.

I wonder if I have done the "am I a varlena" the long way.., pls advise
if so!

Cheers

Mark

Tom Lane wrote:
>
> My recollection of that discussion is that we just wanted something
> that would return the actual VARSIZE() of the datum. You're building
> something way too confusing ...
>
> A more interesting point is that the way you've declared the function,
> it will only work on text values, which is pretty limiting. Ideally
> we'd define this thing as "pg_datum_length(any-varlena-type) returns int"
> but there is no pseudotype corresponding to "any varlena type".
>
> I was thinking about this the other day in connection with my proposal
> to make something that could return the TOAST value OID of an
> out-of-line datum. I think the only non-restrictive way to do it would
> be to declare the function as taking "any", and then add a runtime check
> using the get_fn_expr_argtype() mechanism to test that what you've been
> given is in fact a varlena datatype.
>
>

I did a few cleanups on the last patch. Please examine this one instead.
The changes are:

1. Add documentation for pg_datum_length builtin.
2. Correct some typos in the code comments.
3. Move the code in toastfuncs.c to varlena.c as it is probably the
correct place.
4. Use ereport instead of elog.
5 Quiet compiler warning in pg_datum_length.

Best wishes

Mark

Mark Kirkwood wrote:
> The next iteration -
>
> Hopefully I have got the idea basically right.
>
> I wonder if I have done the "am I a varlena" the long way.., pls advise
> if so!
>

Mark Kirkwood wrote:
> I did a few cleanups on the last patch. Please examine this one instead.
> The changes are:
>
> 1. Add documentation for pg_datum_length builtin.
> 2. Correct some typos in the code comments.
> 3. Move the code in toastfuncs.c to varlena.c as it is probably the
> correct place.
> 4. Use ereport instead of elog.
> 5 Quiet compiler warning in pg_datum_length.

I have modified your patch to simplify the logic, and renamed it to
pg_column_size(), to be consistent with our soon-to-be-added
pg_relation/tablespace/database functions from dbsize.

Here is a sample usage:

test=> CREATE TABLE test (x INT, y TEXT);
CREATE TABLE
test=> INSERT INTO test VALUES (4, repeat('x', 10000));
INSERT 0 1
test=> INSERT INTO test VALUES (4, repeat('x', 100000));
INSERT 0 1
test=> SELECT pg_column_size(x), pg_column_size(y) FROM test;
pg_column_size | pg_column_size
----------------+----------------
4 | 121
4 | 1152
(2 rows)

Interesting the 10-times larger column is 10-times larger in storage.
Do we have some limit on how many repeated values we can record?

Patch attached and applied.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

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?

> + 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.

-Neil

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

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

Well, the syscache already sits in front of the catalogs themselves. I'd
be curious to see what the performance difference actually is...

-Neil

On Thu, Jul 07, 2005 at 03:01:46PM +1200, Mark Kirkwood wrote:
> Neil Conway wrote:

> >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.
>
> 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.

I am, because it gives useless messages to the translators to work on.
elog parameters are not marked for translation, ereport are (errmsg and
friends, really). So please don't do that.

--
Alvaro Herrera (<alvherre[a]alvh.no-ip.org>)
"¿Cómo puedes confiar en algo que pagas y que no ves,
y no confiar en algo que te dan y te lo muestran?" (Germán Poo)

Alvaro Herrera wrote:
> On Thu, Jul 07, 2005 at 03:01:46PM +1200, Mark Kirkwood wrote:
>
>>Neil Conway wrote:
>
>
>>>elog(ERROR) is usually used for "can't happen" errors.
>>
>>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.
>
>
> I am, because it gives useless messages to the translators to work on.
> elog parameters are not marked for translation, ereport are (errmsg and
> friends, really). So please don't do that.
>

Ok, didn't realize the difference! Revised patch attached that uses elog.

Neil, I will runs some tests to see if there is any performance saving
with the first-call-only business.

Cheers

Mark

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
> On Thu, Jul 07, 2005 at 03:01:46PM +1200, Mark Kirkwood wrote:
>> 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.

> I am, because it gives useless messages to the translators to work on.

Exactly. I had already made a private note to fix that patch ---
inventing your own random wording and punctuation for an extremely
standard error message is simply Not Acceptable.

regards, tom lane

Neil Conway wrote:
> Mark Kirkwood wrote:
>
>> I didn't performance test it, but the idea of hammering the catalogs for
>> each value to be processed seemed a bad thing.
>
>
> Well, the syscache already sits in front of the catalogs themselves. I'd
> be curious to see what the performance difference actually is...
>
>

I did some tests with a two tables, one small and one large, I am seeing
a consistent difference favoring the first-call-only type checking:

Table "public.dim1"
Column | Type | Modifiers
--------+------------------------+-----------
d1key | integer | not null
dat | character varying(100) | not null
dattyp | character varying(20) | not null
dfill | character varying(100) | not null

INFO: "dim1": scanned 24 of 24 pages, containing 1001 live rows and 0
dead rows; 1001 rows in sample, 1001 estimated total rows

SELECT max(pg_column_size(dfill)) FROM dim1

Run 1st call only check? elapsed(ms)
1 y 5.5
2 y 3.1
3 n 11.3
4 n 4.1

Now try a bigger table:

Table "public.fact0"
Column | Type | Modifiers
--------+------------------------+-----------
d0key | integer | not null
d1key | integer | not null
d2key | integer | not null
fval | integer | not null
ffill | character varying(100) | not null

INFO: "fact0": scanned 3000 of 18182 pages, containing 165000 live rows
and 0 dead rows; 3000 rows in sample, 1000010 estimated total rows

SELECT max(pg_column_size(ffill)) FROM fact0

Run 1st call only check? elapsed(ms)
1 y 2497
2 y 2484
3 y 2496
4 y 2480
5 n 3572
6 n 3570
7 n 3558
8 n 3457

Mark Kirkwood <markir(at)paradise(dot)net(dot)nz> writes:
> I did some tests with a two tables, one small and one large, I am seeing
> a consistent difference favoring the first-call-only type checking:

We had come to a similar conclusion in regards to making array-related
functions cache lookup info across calls. It's worth checking the
assumption every so often, but the result surprises me not at all.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> writes:
>
>>On Thu, Jul 07, 2005 at 03:01:46PM +1200, Mark Kirkwood wrote:
>>
>>>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.
>
>
>>I am, because it gives useless messages to the translators to work on.
>
>
> Exactly. I had already made a private note to fix that patch ---
> inventing your own random wording and punctuation for an extremely
> standard error message is simply Not Acceptable.
>

Apologies all, my "not fussed about it" was highly misleading - I had
already changed the error message as per Neil's suggestion in the
revised patch, but that was not obvious from my comment :-(

regards

Mark

Patch applied. Thanks.

---------------------------------------------------------------------------

Mark Kirkwood wrote:
> Alvaro Herrera wrote:
> > On Thu, Jul 07, 2005 at 03:01:46PM +1200, Mark Kirkwood wrote:
> >
> >>Neil Conway wrote:
> >
> >
> >>>elog(ERROR) is usually used for "can't happen" errors.
> >>
> >>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.
> >
> >
> > I am, because it gives useless messages to the translators to work on.
> > elog parameters are not marked for translation, ereport are (errmsg and
> > friends, really). So please don't do that.
> >
>
> Ok, didn't realize the difference! Revised patch attached that uses elog.
>
> Neil, I will runs some tests to see if there is any performance saving
> with the first-call-only business.
>
> Cheers
>
> 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 03:40:44 -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,2377 ----
> {
> /* 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. */
> ! elog(ERROR, "cache lookup failed for type %u", argtypeid);
> }
> !
> fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
> sizeof(int));
> *(int *)fcinfo->flinfo->fn_extra = typlen;
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073