On Mon, Sep 23, 2013 at 4:15 PM, Tomas Vondra <tv@fuzzy.cz> wrote: > Seems "ready for commiter" to me. I'll wait a few days for others to > comment, and then I'll update the commitfest page.
Some thoughts:
The documentation doesn't reflect the restriction to type internal. On a related note, why restrict this to type internal?
Now, for almost all types Postgres well estimate size of state value. Only arrays with unknown size can be a different. When we enable this value for all types, then users can specify some bad values for scalar buildin types. Next argument is simply and bad - I don't see a good use case for customization this value for other than types than internal type.
I have no strong position here - prefer joining with internal type due little bit higher robustness.
Project style is not to use curly-braces for single statements. Also, the changes to numeric.c add blank lines before and after function header comments, which is not the usual style.
I think this should just say PG_RETURN_POINTER(state). PG_RETURN_NULL is for returning an SQL NULL, not (void *) 0. Is there some reason why we need an SQL NULL here, rather than a NULL pointer?
fixed
On the whole this looks fairly solid on a first read-through.