2009/7/20 Alvaro Herrera <alvherre@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
