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