temporary view from TODO list

Dear Hackers,

I would like to implement and contribute two TODO items:
"Have views on temporary tables exist in the temporary namespace" and
"Allow temporary views on non-temporary tables".

I would like to discuss several points with community.

*Synopsis

CREATE [ OR REPLACE ] [ TEMP | TEMPORARY ] VIEW ..........

*Description
If [TEMP | TEMPORARY] is specified, the view is created in a temporary
namespace.

If any of the base tables referenced by the view are temporary, the view is
created in a temporary namespace, even if the [TEMP | TEMPORARY] is not
specified (in this case, I'm planning to give "NOTICE" to the client).

*test items for RT
I would like to know amount of test items and what the intention of those
items should be.

1. few items that should test only all the codes we modified or added, to
find any errors from those codes.

2. various items, possibly all the cases that CREATE VIEW operation
involves, to find any errors from temporary view related operations.

Look forward to your comments.

Thanks

koju

----------------------------------------------------------------------------
---
Koju Iijima

Software Engineer
Fujitsu Australia Software Technology
Address: 14 Rodborough Road, Frenchs Forest NSW 2086
Tel: +61 2 9452 9076
Fax: +61 2 9975 2899
Email: koju(at)fast(dot)fujitsu(dot)com(dot)au
Web site: www.fastware.com
----------------------------------------------------------------------------
---

This is an email from Fujitsu Australia Software Technology Pty Ltd, ABN 27 003 693 481. It is confidential to the ordinary user 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 this email 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 email unsubscribe(at)fast(dot)fujitsu(dot)com(dot)au

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 ordinary user 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 this email 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 email unsubscribe(at)fast(dot)fujitsu(dot)com(dot)au

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 ordinary user 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 this email 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 email unsubscribe(at)fast(dot)fujitsu(dot)com(dot)au
>

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 8: explain analyze is your friend

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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(at)samurai(dot)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

Neil Conway <neilc(at)samurai(dot)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