On 1/25/13 1:00 AM, Kevin Grittner wrote:
> New patch rebased, fixes issues raised by Thom Brown, and addresses
> some of your points.
This patch doesn't apply anymore, so I just took a superficial look. I
think the intended functionality and the interfaces look pretty good.
Documentation looks complete, tests are there.
I have a couple of notes:
* What you call WITH [NO] DATA, Oracle calls BUILD IMMEDIATE/DEFERRED.
It might be better to use that as well then.
* You use fields named relkind in the parse nodes, but they don't
actually contain relkind values, which is confusing. I'd just name the
field is_matview or something.
* More generally, I wouldn't be so fond of combining the parse handling
of CREATE TABLE AS and CREATE MATERIALIZED VIEW. They are similar, but
then again so are a lot of other things.
* Some of the terminology is inconsistent. A materialized view is
sometimes called valid, populated, or built, with approximately the same
meaning. Personally, I would settle on "built", as per above, but it
should be one term only.
* I find the name of the relisvalid column a bit confusing. Especially
because it only applies to materialized views, and there is already a
meaning of "valid" for indexes. (Recall that indexes are also stored in
pg_class, but they are concerned about indisvalid.) I would name it
something like relmvbuilt.
Btw., half of the patch seems to consist of updating places referring to
relkind. Is something wrong with the meaning of relkind that this sort
of thing is required? Maybe these places should be operating in terms
of features, not accessing relkind directly.