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(dot)kapila(at)huawei(dot)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(at)gmail(dot)com> wrote:
> On Mon, Jun 18, 2012 at 3:29 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)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(at)gmail(dot)com> wrote:
> On Thu, Jun 28, 2012 at 9:49 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Mon, Jun 18, 2012 at 3:29 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)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

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.

Thanks

On Sat, Feb 9, 2013 at 10:37 PM, Mark Wong <markwkm(at)gmail(dot)com> wrote:

> On Tue, Jul 3, 2012 at 8:33 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Thu, Jun 28, 2012 at 9:49 AM, Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
> >> On Mon, Jun 18, 2012 at 3:29 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)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
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

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

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.

Regards,
Mark

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.

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(at)leadboat(dot)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