Re: patch for temporary view from TODO list

From: Neil Conway <neilc(at)samurai(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Koju Iijima <koju(at)fast(dot)fujitsu(dot)com(dot)au>, pgsql-patches <pgsql-patches(at)postgresql(dot)org>
Subject: Re: patch for temporary view from TODO list
Date: 2005-02-02 01:12:41
Message-ID: 1107306761.23033.21.camel@localhost.localdomain
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers pgsql-patches

On Tue, 2005-02-01 at 01:28 -0500, Tom Lane wrote:
> The references to "CASCADED" (sic) in the patch are surely bogus.

Per SQL:2003 section 11.22, it is spelt "CASCADED". I'm not sure there's
a whole lot of value in documenting syntax we don't support, but if
we're going to do it we may as well get it right.

> "tempViewWalker" in view.c is bereft of either comments or a usefully
> descriptive name. Yeah, you find out what it is supposed to do when you
> reach the routine below, but the whole thing is poorly presented. Waste
> a static declaration forward reference so you can put the documented
> routine first, and rename the walker to something based on the calling
> routine's name.

I've added the forward declaration, but I couldn't think of a better
name for the walker routine. isViewOnTempTableWalker() seemed too
clumsy; any suggestions?

> Does the gram.y change really require breaking out OR REPLACE as a
> separate production, and if so why? That strikes me as something
> that should not be necessary ... if it is then there is some deeper
> problem to investigate.

Without a separate production, you get 8 shift/reduce conflicts because
both opt_or_replace and OptTemp can be reduced on the empty string. The
easiest workaround I can see is the one implemented in the patch, but I
won't claim to be a bison expert. Suggestions for improvement are
welcome.

> The regression tests seem a tad excessive --- I'll grant this is a
> matter of taste, but if every tiny little feature we have were tested
> like that, the regression tests would take days to run.

I've removed a few tests that didn't seem helpful. As for the tests
taking "days to run", that would be fine with me -- if and when the
tests actually _do_ take a long time to run, we can put more effort into
defining a subset of them that can be run more quickly. In the mean
time, though, I think we have far too few tests, not too many.

> [ If I were applying this I'd just editorialize on these things without
> comment, but if you want to do it then you get to do the cleanup... ]

Thanks for your comments. A revised version of the patch is attached.

-Neil

Attachment Content-Type Size
temporaryview4.patch text/x-patch 28.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message John Hansen 2005-02-02 01:36:52 Re: [NOVICE] Last ID Problem
Previous Message Mike Rylander 2005-02-02 01:12:04 Re: FunctionCallN improvement.

Browse pgsql-patches by date

  From Date Subject
Next Message Greg Sabino Mullane 2005-02-02 03:49:52 Re: Continue transactions after errors in psql
Previous Message Tom Lane 2005-02-01 06:28:01 Re: patch for temporary view from TODO list