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
