Thread: patch for temporary view from TODO list
Dear community, I have implemented temporary view related items as I proposed few days ago. http://archives.postgresql.org/pgsql-hackers/2004-09/msg00682.php About test queries for RT (create_view.sql), all queries I added were to find any errors that may be caused by temporary view related operations. Please trim or change those queries according to the community's style. *Patch against CVS HEAD. Thanks koju This is an email from Fujitsu Australia Software Technology Pty Ltd, ABN 27 003 693 481. It is confidential to the ordinaryuser of the email address to which it was addressed and may contain copyright and/or legally privileged information.No one else may read, print, store, copy or forward all or any of it or its attachments. If you receive thisemail in error, please return to sender. Thank you. If you do not wish to receive commercial email messages from Fujitsu Australia Software Technology Pty Ltd, please emailunsubscribe@fast.fujitsu.com.au
Attachment
This has been saved for the 8.1 release:
http:/momjian.postgresql.org/cgi-bin/pgpatches2
---------------------------------------------------------------------------
Koju Iijima wrote:
> Dear community,
>
> I have implemented temporary view related items as I proposed few days ago.
> http://archives.postgresql.org/pgsql-hackers/2004-09/msg00682.php
>
> About test queries for RT (create_view.sql), all queries I added were to
> find any errors that may be caused by temporary view related operations.
> Please trim or change those queries according to the community's style.
>
> *Patch against CVS HEAD.
>
> Thanks
>
> koju
>
> This is an email from Fujitsu Australia Software Technology Pty Ltd, ABN 27 003 693 481. It is confidential to the
ordinaryuser of the email address to which it was addressed and may contain copyright and/or legally privileged
information.No one else may read, print, store, copy or forward all or any of it or its attachments. If you receive
thisemail in error, please return to sender. Thank you.
>
> If you do not wish to receive commercial email messages from Fujitsu Australia Software Technology Pty Ltd, please
emailunsubscribe@fast.fujitsu.com.au
>
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 8: explain analyze is your friend
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
On Mon, 2004-09-27 at 09:39 +1000, Koju Iijima wrote: > I have implemented temporary view related items as I proposed few days ago. > http://archives.postgresql.org/pgsql-hackers/2004-09/msg00682.php Barring any objections, I'll apply this patch tomorrow. -Neil
Neil Conway <neilc@samurai.com> writes: > On Mon, 2004-09-27 at 09:39 +1000, Koju Iijima wrote: >> I have implemented temporary view related items as I proposed few days ago. >> http://archives.postgresql.org/pgsql-hackers/2004-09/msg00682.php > Barring any objections, I'll apply this patch tomorrow. Some minor quibbles: The references to "CASCADED" (sic) in the patch are surely bogus. "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. "explicitely" and "implicitely" are not wel speled. + ereport(NOTICE, + (errmsg("view \"%s\" will be created in a temporary schema", + view->relname))); This seems to me to be exposing an implementation issue without need, ie, the average user does not care that we use temp schemas to implement temp tables. Why not + (errmsg("view \"%s\" will be a temporary view", (Possibly the documentation patches should be adjusted similarly.) 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. 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. [ 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... ] regards, tom lane
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
Neil Conway <neilc@samurai.com> writes:
> 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".
Maybe I'm reading the wrong thing, but the only uses of CASCADED-with-
a-D that I see in the spec are in the context of WITH CHECK OPTION,
which this patch does not implement. I am not sure why we are
documenting stuff we don't implement.
>> "tempViewWalker" in view.c is bereft of either comments or a usefully
>> descriptive 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?
isViewOnTempTable_walker. If that isn't euphonious to you, then change
the name of isViewOnTempTable. Everywhere else that we have walker
subroutines, foo() invokes foo_walker(). You need a seriously good reason
to deviate from that convention.
>> Does the gram.y change really require breaking out OR REPLACE as a
>> separate production, and if so why?
> Without a separate production, you get 8 shift/reduce conflicts because
> both opt_or_replace and OptTemp can be reduced on the empty string.
[ scratches head... ] Seems like it ought to work anyway, since there
are no lookahead keywords for which the parse is ambiguous. I still
think there's something odd here.
If we do have to do it this way, a minor readability improvement would
be to write the CREATE case first and CREATE OR REPLACE second.
regards, tom lane
On Wed, 2005-02-02 at 00:39 -0500, Tom Lane wrote:
> Maybe I'm reading the wrong thing, but the only uses of CASCADED-with-
> a-D that I see in the spec are in the context of WITH CHECK OPTION,
> which this patch does not implement.
Precisely; prior to the patch, WITH CHECK OPTION was documented in the
CREATE VIEW reference page, it was merely done incorrectly ("CASCADE",
not "CASCADED"). This patch corrects that documentation. Whether it is
worth documenting the syntax of unsupported features in the first place
is questionable, but if we're going to do it, we may as well do it
right.
-Neil
On Wed, 2005-02-02 at 00:39 -0500, Tom Lane wrote: > isViewOnTempTable_walker. If that isn't euphonious to you, then change > the name of isViewOnTempTable. Everywhere else that we have walker > subroutines, foo() invokes foo_walker(). You need a seriously good reason > to deviate from that convention. [...] > If we do have to do it this way, a minor readability improvement would > be to write the CREATE case first and CREATE OR REPLACE second. Patch applied to HEAD, including the above suggested changes. Thanks for the patch, Koju. -Neil