[Review] Add SPI_gettypmod() to return a field's typemod from a TupleDesc

On Wed, Feb 1, 2012 at 5:23 AM, Chetan Suttraway

<chetan(dot)suttraway(at)enterprisedb(dot)com> wrote:

> Hi All,

>

> This is regarding the TODO item :

> "Add SPI_gettypmod() to return a field's typemod from a TupleDesc"

>

> The related message is:

> http://archives.postgresql.org/pgsql-hackers/2005-11/msg00250.php

>

> This basically talks about having an SPI_gettypemod() which returns the

> typmod of a field of tupdesc

>

> Please refer the attached patch based on the suggested implementation.

Here's my review of this patch.

Basic stuff:
------------
- Patch applies OK
- Compiles cleanly with no warnings
- Regression tests pass.
- Documentation changes missing
- "43.2. Interface Support Functions" need to add information about
"SPI_gettypmod".
as well as need to add the Description about "SPI_gettypmod".

What it does:
----------------------
New SPI interface function is exposed.

It is used for returning the atttypmod of the specified column.

If attribute number is out of range then set the SPI_result to
"SPI_ERROR_NOATTRIBUTE" and returns -1 else returns attributes typmod

Testing:
-------------------
Created C function and validated type mode for
- output basic data types
- numeric, varchar with valid typmod

=================C-Function=============================
PG_FUNCTION_INFO_V1(test_SPI_gettypmod);
Datum
test_SPI_gettypmod(PG_FUNCTION_ARGS)
{
Oid relid = PG_GETARG_OID(0);
Oid AttidNo = PG_GETARG_OID(1);
Relation rel;
TupleDesc tupdesc; /* tuple
description */
int4 retval;

rel = relation_open(relid, NoLock);
if (!rel)
{
PG_RETURN_INT32(0);
}

tupdesc = rel->rd_att;
retval = SPI_gettypmod(tupdesc, AttidNo);
relation_close(rel, NoLock);
PG_RETURN_INT32(retval);
}

==============Function creation==================================
CREATE FUNCTION test_spi_gettypmod (oid, int) RETURNS int AS
'/lib/mytest.so','test_SPI_gettypmod' LANGUAGE C STRICT;

===============Output=============================================
postgres=# \d tbl
Table "public.tbl"
Column | Type | Modifiers
--------+-----------------------------+-----------
a | integer |
b | character varying(100) |
c | numeric(10,5) |
d | numeric |
e | character varying |
f | timestamp without time zone |

postgres=# \d tt1
Table "public.tt1"
Column | Type | Modifiers
--------+------------------------+-----------
t | tbl |
b | character varying(200) |

postgres=# select atttypmod, attname from pg_attribute where attrelid =
24577;
atttypmod | attname
-----------+----------
-1 | tableoid
-1 | cmax
-1 | xmax
-1 | cmin
-1 | xmin
-1 | ctid
-1 | a
104 | b
655369 | c
(9 rows)

postgres=# select test_spi_gettypmod(24577, 3);
test_spi_gettypmod
--------------------
655369
(1 row)

postgres=# alter table tbl add column d numeric;
ALTER TABLE
postgres=# alter table tbl add column e varchar;
ALTER TABLE
postgres=# select test_spi_gettypmod(24577, 4);
test_spi_gettypmod
--------------------
-1
(1 row)

postgres=# select test_spi_gettypmod(24577, 5);
test_spi_gettypmod
--------------------
-1
(1 row)

postgres=# select test_spi_gettypmod( 24592, 1);
test_spi_gettypmod
--------------------
-1
(1 row)

postgres=# alter table tt1 add column b varchar(200);
ALTER TABLE
postgres=# select test_spi_gettypmod( 24592, 2);
test_spi_gettypmod
--------------------
204
(1 row)
============================================================

Conclusion:
-------------------
The patch changes are okay but needs documentation update, So I am marking
it as Waiting On Author

1. For such kind of patch, even if new regression test is not added, it
is okay.
2. Documentation need to be updated.
3. In case attribute number is not in valid range, currently it return
-1, but -1 is a valid typmod.
Committer can verify if this is appropriate.
4. In functions exec_eval_datum & exec_get_datum_type_info according to
me if we use SPI_gettypmod() to get the
attribute typmod, it is better as
a. there is comment /* XXX there's no SPI_gettypmod, for some
reason */" and it uses SPI_gettypeid to get the typeid.
b. it will maintain consistency of nearby code such as it will use
functions to get typeid and binval.

However there are other places in code which has similar
inconsistency.
Committer can suggest if these changes are required.

With Regards,

Amit Kapila.