Thread: Adding \ev view editor?
All, I've grown to adore the new \ef function editor. It doesn't seem like it would be that difficult to add a view editor as \ev. While editors would also be good for other objects, I don't think we can do \et or \er etc. because those objects don't support CREATE OR REPLACE. Opinions? Other objects which could take \e? -- Josh Berkus PostgreSQL Experts Inc. www.pgexperts.com
On Tue, Sep 01, 2009 at 11:41:31AM -0700, Josh Berkus wrote: > All, > > I've grown to adore the new \ef function editor. > > It doesn't seem like it would be that difficult to add a view editor as > \ev. While editors would also be good for other objects, I don't think > we can do \et or \er etc. because those objects don't support CREATE OR > REPLACE. > > Opinions? Other objects which could take \e? For objects that don't take CREATE OR REPLACE, \e could include the object definition at the top in a multi-line comment, then start with ALTER [OBJECT] Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Josh Berkus wrote: > All, > > I've grown to adore the new \ef function editor. > > It doesn't seem like it would be that difficult to add a view editor as > \ev. While editors would also be good for other objects, I don't think > we can do \et or \er etc. because those objects don't support CREATE OR > REPLACE. > > Opinions? Other objects which could take \e? > > That would make it even more desirable to make my proposed change to pg_get_viewdef to put line breaks between each target item. cheers andrew
On tis, 2009-09-01 at 11:41 -0700, Josh Berkus wrote: > Opinions? Other objects which could take \e? All of them.
Hi, Peter Eisentraut <peter_e@gmx.net> writes: > On tis, 2009-09-01 at 11:41 -0700, Josh Berkus wrote: >> Opinions? Other objects which could take \e? > > All of them. Well, I'd vote against \e table. Are you going to propose the CREATE TABLE statement and have magics to produce the ALTER TABLE that would resort? I hope not, because in some cases I fail to see how you'd do it (CREATE TABLE AS SELECT ... comes to mind, and CREATE TABLE ... (LIKE ...) INHERITS ...; too, let alone where to put the column and table constraints: separate commands or embedded ones?). So you'd propose ALTER TABLE ... and nothing more, and I don't see any gain in that, but maybe it's just me. Editing an index is interesting too, do you generate a DROP INDEX then the new CREATE INDEX, do you add a CREATE INDEX CONCURRENTLY somewhere in the mix, etc... More generally, as said upthread, \e should certainly only be available for objects we can CREATE OR REPLACE... Regards, -- dim
On Tue, Sep 1, 2009 at 2:41 PM, Josh Berkus<josh@agliodbs.com> wrote: > All, > > I've grown to adore the new \ef function editor. > > It doesn't seem like it would be that difficult to add a view editor as > \ev. While editors would also be good for other objects, I don't think > we can do \et or \er etc. because those objects don't support CREATE OR > REPLACE. > > Opinions? Other objects which could take \e? One reason I don't like this proposal is that postgresql does not preserve the original 'create view' statement for editing. The resulting sql that is given to you to edit is hopelessly mangled and I think it's not good to encourage people to modify views in this fashion. Then again, we are only reproducing what pgadmin can already do, so why not? :-) merlin
On Sep 2, 2009, at 5:07 AM, Merlin Moncure wrote: > One reason I don't like this proposal is that postgresql does not > preserve the original 'create view' statement for editing. The > resulting sql that is given to you to edit is hopelessly mangled and I > think it's not good to encourage people to modify views in this > fashion. > > Then again, we are only reproducing what pgadmin can already do, so > why not? :-) Is there any reason that the CREATE VIEW output to pg_dump and, presumably, \ev, couldn't be refactored to pretty-print the VIEW declaration? Best, David
David E. Wheeler wrote: > On Sep 2, 2009, at 5:07 AM, Merlin Moncure wrote: > >> One reason I don't like this proposal is that postgresql does not >> preserve the original 'create view' statement for editing. The >> resulting sql that is given to you to edit is hopelessly mangled and I >> think it's not good to encourage people to modify views in this >> fashion. >> >> Then again, we are only reproducing what pgadmin can already do, so >> why not? :-) > > Is there any reason that the CREATE VIEW output to pg_dump and, > presumably, \ev, couldn't be refactored to pretty-print the VIEW > declaration? > > Tom just said a day or two ago that he doesn't want pg_dump using any pretty printing, and I think there is some sense in that. As discussed last week, the current viewdef pretty printing doesn't put each target in the select list on it own line (or lines) - hence my proposal to make it do so. I think this would be even more desirable if we have \ev. Without that I think \ev is likely to be fairly useless. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> David E. Wheeler wrote:
>> Is there any reason that the CREATE VIEW output to pg_dump and,
>> presumably, \ev, couldn't be refactored to pretty-print the VIEW
>> declaration?
> Tom just said a day or two ago that he doesn't want pg_dump using any
> pretty printing, and I think there is some sense in that.
It might be worth pointing out that what I don't want pg_dump doing
is suppressing "useless" parentheses. Adding whitespace ought to be
safe enough. So if anyone wanted to do the work of decoupling those
two effects of the prettyprint option, we could have "semi pretty
printed" output in pg_dump. Dunno if it's worth it.
I don't see any particular reason to say that \ev shouldn't use fully
pretty printed output, though. It doesn't have the same sorts of
future-proofing worries that pg_dump does.
regards, tom lane
Tom Lane wrote:
> It might be worth pointing out that what I don't want pg_dump doing
> is suppressing "useless" parentheses. Adding whitespace ought to be
> safe enough. So if anyone wanted to do the work of decoupling those
> two effects of the prettyprint option, we could have "semi pretty
> printed" output in pg_dump. Dunno if it's worth it.
>
>
>
The attached patch goes part of the way towards doing this, by adding
white space unconditionally to the target list of a viewdef.
The nice thing about that is that it's the part of a viewdef which
basically isn't pretty-printed now, even if you ask for pretty printing.
That would certainly
make editing the view with a \ev command a lot nicer.
Along the way it suppresses putting a newline before a CASE expression
in pretty printing mode, which seems to be an odd thing to do and is
inconsistent with the way other expressions are treated.
Sample output:
andrew=# select pg_get_viewdef('foo',true);
pg_get_viewdef
------------------------------
SELECT 'a'::text AS b,
( SELECT 1
FROM dual) AS x,
random() AS y,
CASE
WHEN true THEN 1
ELSE 0
END AS c,
1 AS d
FROM dual;
(1 row)
cheers
andrew
Index: src/backend/utils/adt/ruleutils.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/utils/adt/ruleutils.c,v
retrieving revision 1.306
diff -c -r1.306 ruleutils.c
*** src/backend/utils/adt/ruleutils.c 1 Aug 2009 19:59:41 -0000 1.306
--- src/backend/utils/adt/ruleutils.c 21 Sep 2009 18:22:01 -0000
***************
*** 2648,2659 ****
TupleDesc resultDesc)
{
StringInfo buf = context->buf;
char *sep;
! int colno;
ListCell *l;
sep = " ";
colno = 0;
foreach(l, targetList)
{
TargetEntry *tle = (TargetEntry *) lfirst(l);
--- 2648,2671 ----
TupleDesc resultDesc)
{
StringInfo buf = context->buf;
+ StringInfoData sep_indent;
char *sep;
! int colno, len;
ListCell *l;
sep = " ";
colno = 0;
+
+ /*
+ * Separator to make each target appear on its own line, regardless
+ * of pretty printing.
+ * Try to make them all line up at the same line position.
+ */
+ initStringInfo(&sep_indent);
+ appendStringInfoString(&sep_indent,",\n ");
+ for (len = buf->len -1; len >= 0 && buf->data[len]!= '\n'; len--)
+ appendStringInfoChar(&sep_indent,' ');
+
foreach(l, targetList)
{
TargetEntry *tle = (TargetEntry *) lfirst(l);
***************
*** 2664,2670 ****
continue; /* ignore junk entries */
appendStringInfoString(buf, sep);
! sep = ", ";
colno++;
/*
--- 2676,2682 ----
continue; /* ignore junk entries */
appendStringInfoString(buf, sep);
! sep = sep_indent.data;
colno++;
/*
***************
*** 2704,2709 ****
--- 2716,2726 ----
appendStringInfo(buf, " AS %s", quote_identifier(colname));
}
}
+
+ if (colno > 1 && ! PRETTY_INDENT(context))
+ appendStringInfoChar(buf,'\n');
+
+ pfree(sep_indent.data);
}
static void
***************
*** 4595,4602 ****
CaseExpr *caseexpr = (CaseExpr *) node;
ListCell *temp;
! appendContextKeyword(context, "CASE",
! 0, PRETTYINDENT_VAR, 0);
if (caseexpr->arg)
{
appendStringInfoChar(buf, ' ');
--- 4612,4620 ----
CaseExpr *caseexpr = (CaseExpr *) node;
ListCell *temp;
! appendStringInfoString(buf,"CASE");
! if (PRETTY_INDENT(context))
! context->indentLevel += PRETTYINDENT_VAR;
if (caseexpr->arg)
{
appendStringInfoChar(buf, ' ');
Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> It might be worth pointing out that what I don't want pg_dump doing
>> is suppressing "useless" parentheses. Adding whitespace ought to be
>> safe enough. So if anyone wanted to do the work of decoupling those
>> two effects of the prettyprint option, we could have "semi pretty
>> printed" output in pg_dump. Dunno if it's worth it.
> The attached patch goes part of the way towards doing this, by adding
> white space unconditionally to the target list of a viewdef.
That's not what I had in mind by "decoupling" the option's effects.
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> Tom Lane wrote: >> >>> It might be worth pointing out that what I don't want pg_dump doing >>> is suppressing "useless" parentheses. Adding whitespace ought to be >>> safe enough. So if anyone wanted to do the work of decoupling those >>> two effects of the prettyprint option, we could have "semi pretty >>> printed" output in pg_dump. Dunno if it's worth it. >>> > > >> The attached patch goes part of the way towards doing this, by adding >> white space unconditionally to the target list of a viewdef. >> > I'd rather do that than add another pg_get_viewdef variant or option. > That's not what I had in mind by "decoupling" the option's effects. > > > Well, regardless of that it does what I want, and with a fairly small amount of code. I can make it work only in the pretty print case, if that's your objection. Like you I doubt that fully decoupling pretty printing parentheses and whitespace is going to be worth the trouble. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> Tom Lane wrote:
>> That's not what I had in mind by "decoupling" the option's effects.
> Well, regardless of that it does what I want, and with a fairly small
> amount of code.
Well, yeah, because you are paying no mind to what anyone else might
want.
> I can make it work only in the pretty print case, if that's your objection.
I think that's back where we started in this thread. I can live with
it, but I thought some other people were unhappy with the idea of many
lines of targetlist...
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > >> Tom Lane wrote: >> >>> That's not what I had in mind by "decoupling" the option's effects. >>> > > >> Well, regardless of that it does what I want, and with a fairly small >> amount of code. >> > > Well, yeah, because you are paying no mind to what anyone else might > want. > > Umm, no. I have listened but it's not clear what people want. I don't hear too many people actually defending the status quo. Any viewdef with any substantial number of columns is just about unreadable. >> I can make it work only in the pretty print case, if that's your objection. >> > > I think that's back where we started in this thread. I can live with > it, but I thought some other people were unhappy with the idea of many > lines of targetlist... > > > Well, some were yes, but nobody actually came up with anything better. IIRC the code I tried (in response to what people said) that limited newlines by only wrapping lines at some limit got more objections than my original proposal. At one stage you said: Why don't you have the flag just driving your original two-line addition? The patch I sent today is more or less the equivalent of that two-line addition, but removes some ugliness. It's a few more lines, but not many. So that suggestion of mine above to make it work just in the pretty print case was actually intended to be responsive to your previous suggestion. cheers andrew
On Mon, Sep 21, 2009 at 02:26:05PM -0400, Andrew Dunstan wrote:
> andrew=# select pg_get_viewdef('foo',true);
> pg_get_viewdef
> ------------------------------
> SELECT 'a'::text AS b,
> ( SELECT 1
> FROM dual) AS x,
> random() AS y,
> CASE
> WHEN true THEN 1
> ELSE 0
> END AS c,
> 1 AS d
> FROM dual;
> (1 row)
+1
-dg
--
David Gould daveg@sonic.net 510 536 1443 510 282 0869
If simplicity worked, the world would be overrun with insects.
Is this a TODO? --------------------------------------------------------------------------- Josh Berkus wrote: > All, > > I've grown to adore the new \ef function editor. > > It doesn't seem like it would be that difficult to add a view editor as > \ev. While editors would also be good for other objects, I don't think > we can do \et or \er etc. because those objects don't support CREATE OR > REPLACE. > > Opinions? Other objects which could take \e? > > -- > Josh Berkus > PostgreSQL Experts Inc. > www.pgexperts.com > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.comPG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do + If your life is a hard drive,Christ can be your backup. +
On Tue, Feb 23, 2010 at 12:43 AM, Bruce Momjian <bruce@momjian.us> wrote: > Is this a TODO? Sounds good to me. >> It doesn't seem like it would be that difficult to add a view editor as >> \ev. ...Robert
Robert Haas wrote: > On Tue, Feb 23, 2010 at 12:43 AM, Bruce Momjian <bruce@momjian.us> wrote: > >> Is this a TODO? >> > > Sounds good to me. > > I think it would be useful if we put line breaks in the column list it gets, otherwise not so much for any view with more than a handful of columns. That's where we came in on the pretty-printing of viewdefs ... cheers andrew
Andrew Dunstan wrote: > > > Robert Haas wrote: > > On Tue, Feb 23, 2010 at 12:43 AM, Bruce Momjian <bruce@momjian.us> wrote: > > > >> Is this a TODO? > >> > > > > Sounds good to me. > > > > > > I think it would be useful if we put line breaks in the column list it > gets, otherwise not so much for any view with more than a handful of > columns. That's where we came in on the pretty-printing of viewdefs ... Added to TODO: |Add ability to edit views with \ev -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.comPG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do + If your life is a hard drive,Christ can be your backup. +