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

Поиск
Список
Период
Сортировка
От Jeevan Chalke
Тема Re: [Review] Add SPI_gettypmod() to return a field's typemod from a TupleDesc
Дата
Msg-id CAM2+6=V7u6HHLhdL8xyGwxKc+Vwk5bYesLMERFbFaxBfF1r6Mg@mail.gmail.com
обсуждение исходный текст
Ответ на Re: [Review] Add SPI_gettypmod() to return a field's typemod from a TupleDesc  (Mark Wong <markwkm@gmail.com>)
Список pgsql-hackers



On Wed, Jun 26, 2013 at 7:49 AM, Mark Wong <markwkm@gmail.com> wrote:
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.

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.

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Jan Urbański
Дата:
Сообщение: Re: updated emacs configuration
Следующее
От: Ronan Dunklau
Дата:
Сообщение: Re: [PATCH] Fix conversion for Decimal arguments in plpython functions