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

On Wed, Jun 26, 2013 at 7:49 AM, Mark Wong <markwkm(at)gmail(dot)com> wrote:

> On Tue, Jun 25, 2013 at 1:38 AM, Jeevan Chalke
> <jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
> > Hi Mark,
> >
> > Is this the latest patch you are targeting for 9.4 CF1 ?
> >
> > I am going to review it.
> >
> > From the comment, here is one issue you need to resolve first:
> >
> > *************** exec_eval_datum(PLpgSQL_execstate *estat
> > *** 4386,4396 ****
> > errmsg("record \"%s\" has no field
> \"%s\"",
> > rec->refname,
> recfield->fieldname)));
> > *typeid = SPI_gettypeid(rec->tupdesc, fno);
> > ! /* XXX there's no SPI_gettypmod, for some reason */
> > ! if (fno > 0)
> > ! *typetypmod = rec->tupdesc->attrs[fno -
> 1]->atttypmod;
> > ! else
> > ! *typetypmod = -1;
> > *value = SPI_getbinval(rec->tup, rec->tupdesc, fno,
> > isnull);
> > break;
> > }
> > --- 4386,4392 ----
> > errmsg("record \"%s\" has no field
> \"%s\"",
> > rec->refname,
> recfield->fieldname)));
> > *typeid = SPI_gettypeid(rec->tupdesc, fno);
> > ! *typetypmod = SPI_gettypeid(rec->tupdesc, fno);
> > *value = SPI_getbinval(rec->tup, rec->tupdesc, fno,
> > isnull);
> > break;
> > }
> >
> >
> > Once you confirm, I will go ahead reviewing it.
>
> Hi Jeevan,
>
> Oopsies, I've updated the patch and attached it.
>

Here are my review points:

1. Patch is very simple and straight forward.
2. Applies well with patch command. No issues at all.
3. Regression test passes. We have good coverage for that. Also NO issues
found with my testing.
4. New function is analogous to other SPI_get* functions
5. Ready for committer

However, while walking through your changes, I see following line:
/* XXX there's no SPI_getcollation either */
It says we do need function for SPI_getcollation as well. It will be another
simple patch.

Anyway this is not part of this topic so I will go ahead and mark it as
"Ready for committer"

Thanks

Regards,
> Mark
>

--
Jeevan B Chalke
Senior Software Engineer, R&D
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 30589500

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.