Re: nested hstore patch

Attached is a new version of patch, which addresses most issues raised
by Andres.

It's long holidays in Russia now and it happened that Teodor is
traveling with family, so Teodor asked me to reply. Comments in code
will be added asap.

Oleg

On Mon, Nov 18, 2013 at 8:36 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> 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 allocation
>  failures). But why do you even want to silently ignore errors there?

Fixed: now it's ignore only ERRCODE_INVALID_TEXT_REPRESENTATION error.
Our stringIsNumber() could be too optimistic.

> * Shouldn't the checks for v->size be done before filling the
>   datastructures in makeHStoreValueArray() and makeHStoreValuePairs()?

Why check before filling? The result will be the same (throwing an
error) but it saves a loop over array/hash. String values aren't
copied during parse process.


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

fixed - macro and delaction argument was used for development.

> * You call numeric_recv via recvHStoreValue via recvHStore without
>   checks on the input length. That seems - without having checked it in
>   detail - a good way to read unrelated memory. Generally ISTM the input
>   needs to be more carefully checked in the whole recv function.

numeric_recv checks this,  otherwise we can say that numeric_recv
could not check its input. numeric_recv() gets a StringInfo buffer.

> * There's quite some new new, completely uncommented, code. Especially
>   in hstore_op.c.

We'll comment asap

> * the _PG_init you added should probably do a EmitWarningsOnPlaceholders().

fixed

> * why does hstore need it's own atoi?

because:
  * our pg_atoi should work with non-C-string (not finished with \0)
  * doesn't throw an exception on error

> * shouldn't all the function prototypes be marked as externs?

fixed

> * Lots of trailing whitespaces, quite some long lines, cuddly braces,

tried to fix

>   ...
> * I think hstore_compat.c's header should be updated.

yes, we'll do with

>
> Greetings,
>
> Andres Freund
>
> --
>  Andres Freund                     http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers