DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma;
	h=message-id:date:from:to:subject:cc:in-reply-to:mime-version
	:content-type:content-transfer-encoding:content-disposition
	:references;
	b=Iwg7C42WPBmZFzFyi6ePCYAFZc+GrDtGSAtjpoYVo6Nl53L0X1XKTYgdBny7EzfogE
	G5/Q6L/sRL9ZlePlVO1xjnRk+wiaTsWLBaCXYkQxEdO6jqXpr7FjHTG/pPD94KqGSyed
	nx7RUxS4T+1xmQbT22p2B3hGR5amF3r9EAZEQ=
Message-ID: <603c8f070811112006q906b3a6o59a66228b7fc997@mail.gmail.com>
Date: Tue, 11 Nov 2008 23:06:08 -0500
From: "Robert Haas" <robertmhaas@gmail.com>
To: "Bernd Helmle" <mailings@oopsware.de>
Subject: Re: WIP: Automatic view update rules
Cc: pgsql-hackers@postgresql.org
In-Reply-To: <94CF655A8D0DC7D5B4B81D8B@imhotep.credativ.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Content-Disposition: inline
References: <2849137C693B65CC8E86411C@imhotep.credativ.de>
	<94CF655A8D0DC7D5B4B81D8B@imhotep.credativ.de>

I haven't done a full review of this patch, but here are some thoughts
based on a quick read-through:

- "make check" fails 16 of 118 tests for me with this patch applied.

- There are some unnecessary hunks in this diff.  For example, some of
the gram.y changes appear to move curly braces around, adjust spacing,
and replace true and false with TRUE and FALSE (I'm not 100% sure that
the last of these isn't substantive... but I hope it's not).   The
changes to rewriteDefine.c contain some commented-out declarations
that need to be cleaned up, and at least one place where a blank line
has been added.

- The doc changes for ev_kind describe 'e' as meaning "rule was
created by user", which confused me because why would you pick "e" for
that?  Then I realized that "e" was probably intended to mean
"explicit"; it would probably be good to work that word into the
documentation of that value somehow.

- Should this be an optional behavior?  What if I don't WANT my view
to be updateable?

- I am wondering if the old_tup_is_implicit variable started off its
life as a boolean and morphed into a char.  It seems misnamed, now, at
any rate.

- The capitalization of deleteImplicitRulesOnEvent is inconsistent
with the functions that immediately precede it in rewriteRemove.h.  I
think the "d" should be capitalized.  checkTree() also uses this style
of capitalization, which I haven't seen elsewhere in the source tree.

- This:

rhaas=# create table baz (a integer, b integer);
CREATE TABLE
rhaas=# create or replace view bar as select * from baz;
NOTICE:  CREATE VIEW will create implicit INSERT/UPDATE/DELETE rules
CREATE VIEW

Generates this update rule:

ON UPDATE TO bar DO INSTEAD  UPDATE ONLY foo SET a = new.a, b = new.b
  WHERE
        CASE
            WHEN old.a IS NOT NULL THEN old.a = foo.a
            ELSE foo.a IS NULL
        END AND
        CASE
            WHEN old.b IS NOT NULL THEN old.b = foo.b
            ELSE foo.b IS NULL
        END
  RETURNING new.a, new.b

It seems like this could be simplified using IS NOT DISTINCT FROM.

...Robert