Hi,
On 2013-11-12 22:35:31 +0400, Teodor Sigaev wrote:
> Attatched patch adds nesting feature, types (string, boll and numeric
> values), arrays and scalar to hstore type.
I took a quick peek at this:
* You cannot simply catch and ignore errors by doing
+ PG_TRY();
+ {
+ n = DatumGetNumeric(DirectFunctionCall3(numeric_in, CStringGetDatum(s->val), 0, -1));
+ }
+ PG_CATCH();
+ {
+ n = NULL;
+ }
+ PG_END_TRY();That skips cleanup and might ignore some errors (think memory allocationfailures). But why do you
evenwant to silently ignore errors there?
* Shouldn't the checks for v->size be done before filling the datastructures in makeHStoreValueArray() and
makeHStoreValuePairs()?
* could you make ORDER_PAIRS() a function instead of a macro? It's pretty long and there's no reason not to use a
function.
* You call numeric_recv via recvHStoreValue via recvHStore without checks on the input length. That seems - without
havingchecked it in detail - a good way to read unrelated memory. Generally ISTM the input needs to be more carefully
checkedin the whole recv function.
* There's quite some new new, completely uncommented, code. Especially in hstore_op.c.
* the _PG_init you added should probably do a EmitWarningsOnPlaceholders().
* why does hstore need it's own atoi?
* shouldn't all the function prototypes be marked as externs?
* Lots of trailing whitespaces, quite some long lines, cuddly braces, ...
* I think hstore_compat.c's header should be updated.
Greetings,
Andres Freund
-- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services