2009/7/20 Alvaro Herrera <alvhe...@commandprompt.com>: > Dean Rasheed wrote: >> Thanks for the thorough review. I attach an new version of the patch, >> updated to HEAD, and with the index AM change discussed. > > Wow, this is a large patch. > > I didn't do a thorough review, but some quickies I noticed: > > * Please move the code that says that it should be in a new file to a > new file. >
Ah yes, I forgot about that. Will do. utils/adt/constraints.c ? I'm thinking other deferred constraints (eg. CHECK) might get added to it one day. > * Please don't mimic the silliness of RI_FKey_check of an unused return > value. Just make it return void OK. > and have the caller use the proper > PG_RETURN_FOO macro. (Unless there's a specific reason to not do > things that way) > It looks like I can't use PG_RETURN_NULL() because that will break the trigger calling code, but I could use PG_RETURN_VOID(). But is that the recommended style? Section 35.4 of the docs suggests using PointerGetDatum(NULL). > * I'm not sure about the changes in trigger.h and related elsewhere. > Seems related to the new list in AfterTriggerSaveEvent, which is > used in ways that seem to conflict with its header comment ... I > wonder if we should be doing that in the executor itself instead. Yes I suppose the comment is a bit misleading. I put the check there because that's where the similar RI checks are, which already conflict with the header comment. The simplest solution is to just update the comment. This seemed like the least intrusive way to do this. It is also called from copy.c which duplicates some of the executor code. > In any case it's inconsistent that the list is added to > ExecARInsertTriggers but not to ExecARUpdateTrigger et al ... > Yes it is added to ExecARUpdateTriggers(). - Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers