Skip site navigation (1) Skip section navigation (2)

Re: nested hstore patch

From: Andres Freund <andres(at)2ndquadrant(dot)com>
To: Teodor Sigaev <teodor(at)sigaev(dot)ru>
Cc: Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: nested hstore patch
Date: 2013-11-18 16:36:33
Message-ID: 20131118163633.GE20305@awork2.anarazel.de (view raw or flat)
Thread:
Lists: pgsql-hackers
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?
* 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 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.
* 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


In response to

Responses

pgsql-hackers by date

Next:From: Heikki LinnakangasDate: 2013-11-18 16:37:14
Subject: Re: appendPQExpBufferVA vs appendStringInfoVA
Previous:From: Andres FreundDate: 2013-11-18 15:53:26
Subject: Re: Freezing without write I/O

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group