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

On Mon, Jun 18, 2012 at 3:29 PM, Amit Kapila <amit.kapila@huawei.com> wrote:
> [ review ]

Chetan, this patch is waiting for an update from you.  If you'd like
this to get committed this CommitFest, we'll need an updated patch
soon.

Thanks,

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jun 28, 2012 at 9:49 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Jun 18, 2012 at 3:29 PM, Amit Kapila <amit.kapila@huawei.com> wrote:
>> [ review ]
>
> Chetan, this patch is waiting for an update from you.  If you'd like
> this to get committed this CommitFest, we'll need an updated patch
> soon.

Hearing no response, I've marked this patch Returned with Feedback.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Jul 3, 2012 at 8:33 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Jun 28, 2012 at 9:49 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Mon, Jun 18, 2012 at 3:29 PM, Amit Kapila <amit.kapila@huawei.com> wrote:
>>> [ review ]
>>
>> Chetan, this patch is waiting for an update from you.  If you'd like
>> this to get committed this CommitFest, we'll need an updated patch
>> soon.
>
> Hearing no response, I've marked this patch Returned with Feedback.

Hello everyone,

I thought I'd take a stab at helping finish this patch.  I have made
an attempt at adding documentation and replacing the couple of XXX
comments.  I'll add it to the next commitfest.

Regards,
Mark

On Tue, Jun 25, 2013 at 1:38 AM, Jeevan Chalke
<jeevan.chalke@enterprisedb.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.

Regards,
Mark

I find the SPI "interface support functions" quaint.  They're thin wrappers,
of ancient origin, around standard backend coding patterns.  They have the
anti-feature of communicating certain programming errors through return
value/SPI_result rather than elog()/Assert().  The chance that we could
substantially refactor the underlying primary backend APIs and data structures
while keeping these SPI wrappers unchanged seems slight.

On Tue, Jun 25, 2013 at 07:19:58PM -0700, Mark Wong wrote:
> +    <function>SPI_gettypmod</function> returns the type-specific data supplied
> +    at table creation time.  For example: the max length of a varchar field.

SPI callers typically have no business interpreting the value, that being the
distinct purview of each type implementation.  The text type does set its
typmod to 4 + max length, but other code should not know that.  SPI callers
can use this to convey a typmod for later use, though.

> +   <para>
> +    The type-specific data supplied at table creation time of the specified
> +    column or <symbol>InvalidOid</symbol> on error.  On error,
> +    <varname>SPI_result</varname> is set to
> +    <symbol>SPI_ERROR_NOATTRIBUTE</symbol>.
> +   </para>

You have it returning -1, not InvalidOid.  Per Amit's review last year, I'm
wary of returning -1 in the error case.  But I suspect most callers will, like
the two callers you add, make a point of never passing an invalid argument and
then not bother checking for error.  So, no big deal.


I mildly recommend we reject this patch as such, remove the TODO item, remove
the XXX comments this patch removes, and plan not to add more trivial SPI
wrappers.  If consensus goes otherwise, I think we should at least introduce
SPI_getcollation() at the same time.  Code that needs to transfer one of them
very often needs to transfer the other.  Having API coverage for just one
makes it easier for hackers to miss that.

Thanks,
nm

-- 
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com

On Sun, 2013-07-07 at 20:15 -0400, Noah Misch wrote:
> I mildly recommend we reject this patch as such, remove the TODO item,
> remove
> the XXX comments this patch removes, and plan not to add more trivial
> SPI
> wrappers.  If consensus goes otherwise, I think we should at least
> introduce
> SPI_getcollation() at the same time.  Code that needs to transfer one
> of them
> very often needs to transfer the other.  Having API coverage for just
> one
> makes it easier for hackers to miss that. 

The question is, what would one do with those values?  It's hard to see
when you would need the typmod and the collation of a result set.  There
might be cases, but enough to provide a special API for it?

On Sun, Jul 07, 2013 at 08:55:01PM -0400, Peter Eisentraut wrote:
> On Sun, 2013-07-07 at 20:15 -0400, Noah Misch wrote:
> > I mildly recommend we reject this patch as such, remove the TODO item,
> > remove
> > the XXX comments this patch removes, and plan not to add more trivial
> > SPI
> > wrappers.  If consensus goes otherwise, I think we should at least
> > introduce
> > SPI_getcollation() at the same time.  Code that needs to transfer one
> > of them
> > very often needs to transfer the other.  Having API coverage for just
> > one
> > makes it easier for hackers to miss that. 
> 
> The question is, what would one do with those values?  It's hard to see
> when you would need the typmod and the collation of a result set.  There
> might be cases, but enough to provide a special API for it?

Good point.  One of the ways PL/pgSQL uses it is to feed a result datum back
into a future query as a Param node.

-- 
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com

On Sun, Jul 07, 2013 at 08:15:00PM -0400, Noah Misch wrote:
> I mildly recommend we reject this patch as such, remove the TODO item, remove
> the XXX comments this patch removes, and plan not to add more trivial SPI
> wrappers.

Seeing just the one response consistent with that view, done.

-- 
Noah Misch
EnterpriseDB                                 http://www.enterprisedb.com

On Jul 12, 2013, at 4:29 PM, Noah Misch <noah@leadboat.com> wrote:

> On Sun, Jul 07, 2013 at 08:15:00PM -0400, Noah Misch wrote:
>> I mildly recommend we reject this patch as such, remove the TODO item, remove
>> the XXX comments this patch removes, and plan not to add more trivial SPI
>> wrappers.
>
> Seeing just the one response consistent with that view, done.

Shucks.  :) Thanks for reviewing everyone.

Regards,
Mark