Re: [PATCH] Support for Array ELEMENT Foreign Keys

On Tue, Sep 18, 2012 at 05:52:51PM +0200, Marco Nenciarini wrote:
>     please find attached the refreshed v1 patch.

I perused this version in comparison to the last version I reviewed, finding
minor problems.  First, a warning:

tablecmds.c: In function `ATExecAddConstraint':
tablecmds.c:5898: warning: `fk_element_type' may be used uninitialized in this function
tablecmds.c:5898: note: `fk_element_type' was declared here

I don't see an actual bug; add a dead store to silence the compiler.

> *** a/src/test/regress/parallel_schedule
> --- b/src/test/regress/parallel_schedule
> *************** test: event_trigger
> *** 94,100 ****
>   # ----------
>   # Another group of parallel tests
>   # ----------
> ! test: select_views portals_p2 foreign_key cluster dependency guc bitmapops combocid tsearch tsdicts foreign_data window xmlmap functional_deps advisory_lock json
>   
>   # ----------
>   # Another group of parallel tests
> --- 94,100 ----
>   # ----------
>   # Another group of parallel tests
>   # ----------
> ! test: select_views portals_p2 foreign_key cluster dependency guc bitmapops combocid tsearch tsdicts foreign_data window xmlmap functional_deps advisory_lock element_foreign_key

Keep that json test.

> + 				errmsg("array ELEMENT foreign keys only support NO ACTION "
> + 					   "and RESTRICT actions")));

Project style is not to break message literals; instead, let the line run
long.  There are a few more examples of this in your patch.


Those problems are isolated and do not impugn design, so committer time would
be just as well-spent on the latest version.  As such, I'm marking the patch
Ready for Committer.  Thanks to Rafal Pietrak for his helpful review.

nm