[Review] Add SPI_gettypmod() to return a field's typemod from a TupleDesc - Mailing list pgsql-hackers

<div class="WordSection1"><p class="MsoNormal"><span lang="EN" style="font-size:13.0pt;font-family:"Courier
New";color:black">OnWed, Feb 1, 2012 at 5:23 AM, Chetan Suttraway</span><p class="MsoNormal"><span lang="EN"
style="font-size:13.0pt;font-family:"CourierNew";color:black"><chetan(dot)suttraway(at)enterprisedb(dot)com>
wrote:</span><pclass="MsoNormal"><span lang="EN" style="font-size:13.0pt;font-family:"Courier New";color:black">> Hi
All,</span><pclass="MsoNormal"><span lang="EN" style="font-size:13.0pt;font-family:"Courier
New";color:black">> </span><pclass="MsoNormal"><span lang="EN" style="font-size:13.0pt;font-family:"Courier
New";color:black">>This is regarding the TODO item :</span><p class="MsoNormal"><span lang="EN"
style="font-size:13.0pt;font-family:"CourierNew";color:black">> "Add SPI_gettypmod() to return a field's typemod
froma TupleDesc"</span><p class="MsoNormal"><span lang="EN" style="font-size:13.0pt;font-family:"Courier
New";color:black">> </span><pclass="MsoNormal"><span lang="EN" style="font-size:13.0pt;font-family:"Courier
New";color:black">>The related message is:</span><p class="MsoNormal"><span lang="EN"
style="font-size:13.0pt;font-family:"CourierNew";color:black">> <a
href="http://archives.postgresql.org/pgsql-hackers/2005-11/msg00250.php">http://archives.postgresql.org/pgsql-hackers/2005-11/msg00250.php</a></span><p
class="MsoNormal"><spanlang="EN" style="font-size:13.0pt;font-family:"Courier New";color:black">> </span><p
class="MsoNormal"><spanlang="EN" style="font-size:13.0pt;font-family:"Courier New";color:black">> This basically
talksabout having an SPI_gettypemod() which returns the</span><p class="MsoNormal"><span lang="EN"
style="font-size:13.0pt;font-family:"CourierNew";color:black">> typmod of a field of tupdesc</span><p
class="MsoNormal"><spanlang="EN" style="font-size:13.0pt;font-family:"Courier New";color:black">> </span><p
class="MsoNormal"><spanlang="EN" style="font-size:13.0pt;font-family:"Courier New";color:black">> Please refer the
attachedpatch based on the suggested implementation.</span><p class="MsoNormal"><span lang="EN"
style="font-size:13.0pt;font-family:"CourierNew";color:black"> </span><p class="MsoNormal"><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Here'smy review of this patch.</span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Basicstuff:</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">------------</span><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-Patch applies OK </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-Compiles cleanly with no warnings</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-Regression tests pass.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-Documentation changes missing</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   - "43.2. Interface Support Functions" need to add
informationabout "SPI_gettypmod".</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">      as
wellas need to add the Description about "SPI_gettypmod".</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Whatit does:</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">----------------------</span><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   New SPI interface function is exposed. </span><p
class="MsoNormal"><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">    It is used for returning the
atttypmodof the specified column. </span><p class="MsoNormal"><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">    Ifattribute number is out of range then set the
SPI_resultto "SPI_ERROR_NOATTRIBUTE" and returns -1 else returns attributes typmod </span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">Testing:</span><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-------------------</span><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   Created C function and validated type mode for </span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">        - output basic data types</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">       - numeric, varchar with valid typmod</span><br /><br
/><spanstyle="font-size:7.5pt">=================C-Function=============================</span><br /><span
style="font-size:7.5pt">       PG_FUNCTION_INFO_V1(test_SPI_gettypmod);</span><br /><span style="font-size:7.5pt">     
 Datum</span><br /><span style="font-size:7.5pt">        test_SPI_gettypmod(PG_FUNCTION_ARGS)</span><br /><span
style="font-size:7.5pt">       {</span><br /><span style="font-size:7.5pt">                Oid                      
 relid= PG_GETARG_OID(0);</span><br /><span style="font-size:7.5pt">                Oid                        AttidNo
=PG_GETARG_OID(1);</span><br /><span style="font-size:7.5pt">                Relation        rel;</span><br /><span
style="font-size:7.5pt">               TupleDesc        tupdesc;                /* tuple description */</span><br
/><spanstyle="font-size:7.5pt">                int4                retval; </span><br /><br /><span
style="font-size:7.5pt">               rel = relation_open(relid, NoLock);</span><br /><span style="font-size:7.5pt"> 
             if (!rel)</span><br /><span style="font-size:7.5pt">                {</span><br /><span
style="font-size:7.5pt">                       PG_RETURN_INT32(0);</span><br /><span style="font-size:7.5pt">         
     }</span><br /><span style="font-size:7.5pt">        </span><br /><span style="font-size:7.5pt">               
tupdesc= rel->rd_att;</span><br /><span style="font-size:7.5pt">                retval = SPI_gettypmod(tupdesc,
AttidNo);</span><br/><span style="font-size:7.5pt">                relation_close(rel, NoLock);</span><br /><span
style="font-size:7.5pt">               PG_RETURN_INT32(retval);</span><br /><span style="font-size:7.5pt">       
}</span><br/><br /><span style="font-size:7.5pt">==============Function
creation==================================</span><br/><span style="font-size:7.5pt">        CREATE FUNCTION
test_spi_gettypmod(oid, int) RETURNS int AS '/lib/mytest.so','test_SPI_gettypmod' LANGUAGE C STRICT;</span><br /><br
/><spanstyle="font-size:7.5pt">===============Output=============================================</span><br /><span
style="font-size:7.5pt">postgres=#\d tbl</span><br /><span style="font-size:7.5pt">                Table
"public.tbl"</span><br/><span style="font-size:7.5pt"> Column |            Type             | Modifiers</span><br
/><spanstyle="font-size:7.5pt">--------+-----------------------------+-----------</span><br /><span
style="font-size:7.5pt"> a     | integer                     |</span><br /><span style="font-size:7.5pt"> b      |
charactervarying(100)      |</span><br /><span style="font-size:7.5pt"> c      | numeric(10,5)              
|</span><br/><span style="font-size:7.5pt"> d      | numeric                     |</span><br /><span
style="font-size:7.5pt"> e     | character varying           |</span><br /><span style="font-size:7.5pt"> f      |
timestampwithout time zone |</span><br /><br /><span style="font-size:7.5pt">postgres=# \d tt1</span><br /><span
style="font-size:7.5pt">            Table "public.tt1"</span><br /><span style="font-size:7.5pt"> Column |        
 Type         | Modifiers</span><br /><span
style="font-size:7.5pt">--------+------------------------+-----------</span><br/><span style="font-size:7.5pt"> t    
 |tbl                    |</span><br /><span style="font-size:7.5pt"> b      | character varying(200) |</span><br /><br
/><spanstyle="font-size:7.5pt">postgres=# select atttypmod, attname from pg_attribute where attrelid = 24577;</span><br
/><spanstyle="font-size:7.5pt"> atttypmod | attname</span><br /><span
style="font-size:7.5pt">-----------+----------</span><br/><span style="font-size:7.5pt">        -1 | tableoid</span><br
/><spanstyle="font-size:7.5pt">        -1 | cmax</span><br /><span style="font-size:7.5pt">        -1 | xmax</span><br
/><spanstyle="font-size:7.5pt">        -1 | cmin</span><br /><span style="font-size:7.5pt">        -1 | xmin</span><br
/><spanstyle="font-size:7.5pt">        -1 | ctid</span><br /><span style="font-size:7.5pt">        -1 | a</span><br
/><spanstyle="font-size:7.5pt">       104 | b</span><br /><span style="font-size:7.5pt">    655369 | c</span><br
/><spanstyle="font-size:7.5pt">(9 rows)</span><br /><br /><span style="font-size:7.5pt">postgres=# select
test_spi_gettypmod(24577,3);</span><br /><span style="font-size:7.5pt"> test_spi_gettypmod</span><br /><span
style="font-size:7.5pt">--------------------</span><br/><span style="font-size:7.5pt">             655369</span><br
/><spanstyle="font-size:7.5pt">(1 row)</span><br /><br /><span style="font-size:7.5pt">postgres=# alter table tbl add
columnd numeric;</span><br /><span style="font-size:7.5pt">ALTER TABLE</span><br /><span
style="font-size:7.5pt">postgres=#alter table tbl add column e varchar;</span><br /><span style="font-size:7.5pt">ALTER
TABLE</span><br/><span style="font-size:7.5pt">postgres=# select test_spi_gettypmod(24577, 4);</span><br /><span
style="font-size:7.5pt"> test_spi_gettypmod</span><br/><span style="font-size:7.5pt">--------------------</span><br
/><spanstyle="font-size:7.5pt">                 -1</span><br /><span style="font-size:7.5pt">(1 row)</span><br /><br
/><spanstyle="font-size:7.5pt">postgres=# select test_spi_gettypmod(24577, 5);</span><br /><span
style="font-size:7.5pt"> test_spi_gettypmod</span><br/><span style="font-size:7.5pt">--------------------</span><br
/><spanstyle="font-size:7.5pt">                 -1</span><br /><span style="font-size:7.5pt">(1 row)</span><br /><br
/><spanstyle="font-size:7.5pt">postgres=# select test_spi_gettypmod( 24592, 1);</span><br /><span
style="font-size:7.5pt"> test_spi_gettypmod</span><br/><span style="font-size:7.5pt">--------------------</span><br
/><spanstyle="font-size:7.5pt">                 -1</span><br /><span style="font-size:7.5pt">(1 row)</span><br /><br
/><br/><span style="font-size:7.5pt">postgres=# alter table tt1 add column b varchar(200);</span><br /><span
style="font-size:7.5pt">ALTERTABLE</span><br /><span style="font-size:7.5pt">postgres=# select test_spi_gettypmod(
24592,2);</span><br /><span style="font-size:7.5pt"> test_spi_gettypmod</span><br /><span
style="font-size:7.5pt">--------------------</span><br/><span style="font-size:7.5pt">                204</span><br
/><spanstyle="font-size:7.5pt">(1 row)</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">============================================================</span><br
/><br/><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">Conclusion:</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">-------------------</span><br/><span
style="font-size:10.0pt;font-family:"Arial","sans-serif""> Thepatch changes are okay but needs documentation update, So
Iam marking it as Waiting On Author</span><p class="MsoNormal"><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">   1. For such kind of patch, even if new regression test is
notadded, it is okay.</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    2. Documentation
needto be updated.</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">    3. In case attribute
numberis not in valid range, currently it return -1, but -1 is a valid typmod. </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">       Committer can verify if this is appropriate.</span><br
/><spanstyle="font-size:10.0pt;font-family:"Arial","sans-serif"">    4. In functions exec_eval_datum &
exec_get_datum_type_infoaccording to me if we use SPI_gettypmod() to get the </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">       attribute typmod, it is better as </span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">       a. there is comment  /* XXX there's no SPI_gettypmod,
forsome reason */" and it uses SPI_gettypeid to get the typeid.</span><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">       b. it will maintain consistency of nearby code such as
itwill use functions to get typeid and binval.</span><br /><br /><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">       However there are other places in code which has
similarinconsistency.</span><br /><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">        Committer can
suggestif these changes are required.    </span><p class="MsoNormal"><span
style="font-size:10.0pt;font-family:"Arial","sans-serif""> </span><pclass="MsoNormal"><span
style="font-size:10.0pt;font-family:"Arial","sans-serif""> </span><pclass="MsoNormal"><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">WithRegards,</span><p class="MsoNormal"><span
style="font-size:10.0pt;font-family:"Arial","sans-serif"">AmitKapila.</span></div>