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