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