Horton

Thread

Commits

  1. Fix psql's \ev and \sv commands so that they handle view reloptions.

  2. Move and rename fmtReloptionsArray().

  3. pgindent run for 9.5

  1. psql :: support for \ev viewname and \sv viewname

    Petr Korobeinikov <pkorobeinikov@gmail.com> — 2015-05-04T09:21:43Z

    Hackers!

I'm proposing to add two new subcommands in psql:
1. \ev viewname - edit view definition with external editor (like \ef for
function)
2. \sv viewname - show view definition (like \sf for function, for
consistency)

What's inside:
1. review-ready implementation of \ev and \sv psql subcommands for editing
and viewing view's definition.
2. psql's doc update with new subcommands description.
3. a bit of extracting common source code parts into separate functions.
4. psql internal help update.
5. tab completion update.

There is one narrow place in this patch: current implementation doesn't
support "create" statement formatting for recursive views.

Any comments? Suggestions?

  2. Re: psql :: support for \ev viewname and \sv viewname

    Pavel Stehule <pavel.stehule@gmail.com> — 2015-05-04T09:26:18Z

    2015-05-04 11:21 GMT+02:00 Petr Korobeinikov <pkorobeinikov@gmail.com>:

> Hackers!
>
> I'm proposing to add two new subcommands in psql:
> 1. \ev viewname - edit view definition with external editor (like \ef for
> function)
> 2. \sv viewname - show view definition (like \sf for function, for
> consistency)
>

+1

Pavel


>
> What's inside:
> 1. review-ready implementation of \ev and \sv psql subcommands for editing
> and viewing view's definition.
> 2. psql's doc update with new subcommands description.
> 3. a bit of extracting common source code parts into separate functions.
> 4. psql internal help update.
> 5. tab completion update.
>
> There is one narrow place in this patch: current implementation doesn't
> support "create" statement formatting for recursive views.
>
> Any comments? Suggestions?
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

  3. Re: psql :: support for \ev viewname and \sv viewname

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2015-05-04T17:33:16Z

    On Mon, May 4, 2015 at 6:26 AM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

>
>
> 2015-05-04 11:21 GMT+02:00 Petr Korobeinikov <pkorobeinikov@gmail.com>:
>
>> Hackers!
>>
>> I'm proposing to add two new subcommands in psql:
>> 1. \ev viewname - edit view definition with external editor (like \ef for
>> function)
>> 2. \sv viewname - show view definition (like \sf for function, for
>> consistency)
>>
>
> +1
>
>
+1

During the FISL13 [1] (year 2012) me and other friends (Leonardo César,
Dickson Guedes and Fernando Ike) implemented a very WIP patch [2] to
support the "\ev" subcommand in psql. Unfortunately we didn't go ahead with
this work. :-(

I'll do some reviews.

Regards,


[1] http://softwarelivre.org/fisl13?lang=en
[2]
https://github.com/lhcezar/postgres/commit/b4bfc3b17b4a32850d6035165209b2b82746190a

-- 
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

  4. Re: psql :: support for \ev viewname and \sv viewname

    Robert Haas <robertmhaas@gmail.com> — 2015-05-05T13:42:45Z

    On Mon, May 4, 2015 at 5:21 AM, Petr Korobeinikov
<pkorobeinikov@gmail.com> wrote:
> I'm proposing to add two new subcommands in psql:
> 1. \ev viewname - edit view definition with external editor (like \ef for
> function)
> 2. \sv viewname - show view definition (like \sf for function, for
> consistency)

Sounds nice.  Make sure to add your patch to the open CommitFest so we
don't forget about it.

https://commitfest.postgresql.org/action/commitfest_view/open

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  5. Re: psql :: support for \ev viewname and \sv viewname

    Petr Korobeinikov <pkorobeinikov@gmail.com> — 2015-05-14T08:04:30Z

    This version contains one little change.
In order to be consistent with “\d+ viewname” it uses pg_get_viewdef(oid, /* pretty */ true) to produce “pretty” output (without additional parentheses).

  6. Re: psql :: support for \ev viewname and \sv viewname

    Petr Korobeinikov <pkorobeinikov@gmail.com> — 2015-05-24T06:08:41Z

    Just a merge after pgindent run (807b9e0dff663c5da875af7907a5106c0ff90673).

  7. Re: psql :: support for \ev viewname and \sv viewname

    Jeevan Chalke <jeevan.chalke@gmail.com> — 2015-06-01T10:08:55Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, failed

I have reviewed this patch. Most of the code is just rearranged.
Since this is based upon, \ef and \sf, code is almost similar.

However hare are my review comments:

1.
make failed with docs

2.
> \ev vw1 3

This syntax is supported. But documentation only says:
\ev [ viewname ] 
Missing optional line_number clause

3.
> strip_lineno_from_objdesc(char *func)

Can we have parameter name as obj instead of func.
You have renamed the function name, as it is now called in case of views as
well. Better rename the parameter names as well.

4.
Also please update the comments above strip_lineno_from_objdesc().
It is specific to functions which is not the case now.

5.
> print_with_linenumbers(FILE *output,
>					   char *lines,
>					   const char *header_cmp_keyword,
>					   size_t header_cmp_sz)

Can't we calculate the length of header (header_cmp_sz) inside function?
This will avoid any sloppy changes like, change in the keyword but forgot to
change the size.
Lets just accept the keyword and calculate the size within the function.

6.
>				 *
>				 * Note that this loop scribbles on func_buf.
>				 */

These lines at commands.c:1357, looks NO more valid now as there is NO loop
there.

7.
I see few comment lines explaining which is line 1 in case of function, for
which "AS " is used. Similarly, for view "SELECT " is used.
Can you add similar kind of explanation there?

8.
> get_create_object_cmd_internal
> get_create_function_cmd
> get_create_view_cmd

Can these three functions grouped together in just get_create_object_cmd(). 
This function will take an extra parameter to indicate the object type.
Say O_FUNC and O_VIEW for example.

For distinct part, just have a switch case over this type.

This will add a flexibility that if we add another such \e and \s options, we
don't need new functions, rather just need new enum like O_new and a new case
in this switch statement.
Also it will look good to read the code as well.

similarly you can do it for
> lookup_object_oid_internal
> get_create_function_cmd
> lookup_function_oid

9.
> static int count_lines_in_buf(PQExpBuffer buf)
> static void print_with_linenumbers(FILE *output, .. )
> static bool lookup_view_oid(const char *desc, Oid *view_oid)
> static bool lookup_object_oid_internal(PQExpBuffer query, Oid *obj_oid)

Can we have smaller description, explaining what's the function doing for
these functions at the definition?

10.
> +		"\\e", "\\echo", "\\ef", "\\ev", "\\encoding",

Can you keep this sorted?
It will be good if it sorted, but I see no such restriction as I see few out
of order options. But better keep it ordered.
Ignore if you dis-agree.


The new status of this patch is: Waiting on Author

  8. Re: psql :: support for \ev viewname and \sv viewname

    Petr Korobeinikov <pkorobeinikov@gmail.com> — 2015-06-06T19:59:21Z

    > 1.
> make failed with docs
>
Fixed.


> 2.
> > \ev vw1 3
>
> This syntax is supported. But documentation only says:
> \ev [ viewname ]
> Missing optional line_number clause
>
Fixed. Documented.

3.
> > strip_lineno_from_objdesc(char *func)
>
> Can we have parameter name as obj instead of func.
> You have renamed the function name, as it is now called in case of views as
> well. Better rename the parameter names as well.

Renamed.

4.
> Also please update the comments above strip_lineno_from_objdesc().
> It is specific to functions which is not the case now.
>
 Comments updated.


> 5.
> > print_with_linenumbers(FILE *output,
> >                                          char *lines,
> >                                          const char *header_cmp_keyword,
> >                                          size_t header_cmp_sz)
>
> Can't we calculate the length of header (header_cmp_sz) inside function?
> This will avoid any sloppy changes like, change in the keyword but forgot
> to
> change the size.
> Lets just accept the keyword and calculate the size within the function.
>
Now header_cmp_sz calculated inside function.

6.
> >                                *
> >                                * Note that this loop scribbles on
> func_buf.
> >                                */
>
> These lines at commands.c:1357, looks NO more valid now as there is NO loop
> there.
>
Removed.


>
> 7.
> I see few comment lines explaining which is line 1 in case of function, for
> which "AS " is used. Similarly, for view "SELECT " is used.
> Can you add similar kind of explanation there?
>
Explanation added.

8.
> > get_create_object_cmd_internal
> > get_create_function_cmd
> > get_create_view_cmd
>
> Can these three functions grouped together in just get_create_object_cmd().
> This function will take an extra parameter to indicate the object type.
> Say O_FUNC and O_VIEW for example.
>
> For distinct part, just have a switch case over this type.
>
> This will add a flexibility that if we add another such \e and \s options,
> we
> don't need new functions, rather just need new enum like O_new and a new
> case
> in this switch statement.
> Also it will look good to read the code as well.
>
> similarly you can do it for
> > lookup_object_oid_internal
> > get_create_function_cmd
> > lookup_function_oid
>
Reworked.
New enum PgObjType introduced.


>
> 9.
> > static int count_lines_in_buf(PQExpBuffer buf)
> > static void print_with_linenumbers(FILE *output, .. )
> > static bool lookup_view_oid(const char *desc, Oid *view_oid)
> > static bool lookup_object_oid_internal(PQExpBuffer query, Oid *obj_oid)
>
> Can we have smaller description, explaining what's the function doing for
> these functions at the definition?
>
Description added.


>
> 10.
> > +             "\\e", "\\echo", "\\ef", "\\ev", "\\encoding",
>
> Can you keep this sorted?
> It will be good if it sorted, but I see no such restriction as I see few
> out
> of order options. But better keep it ordered.
> Ignore if you dis-agree.
>
Hmm, sorted now.
Sort is based on my feelings.

  9. Re: psql :: support for \ev viewname and \sv viewname

    Jeevan Chalke <jeevan.chalke@enterprisedb.com> — 2015-06-09T12:29:52Z

    Hi

Patch looks excellent now. No issues.

Found a typo which I have fixed in the attached patch.

Thanks

-- 
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

  10. Re: psql :: support for \ev viewname and \sv viewname

    Jeevan Chalke <jeevan.chalke@gmail.com> — 2015-06-09T12:39:57Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

Patch looks good to pass to committer.

The new status of this patch is: Ready for Committer

  11. Re: psql :: support for \ev viewname and \sv viewname

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-07-03T16:30:52Z

    Jeevan Chalke <jeevan.chalke@enterprisedb.com> writes:
> Patch looks excellent now. No issues.
> Found a typo which I have fixed in the attached patch.

Starting to look at this ...

The business with numbering lines from SELECT seems to me to be completely
nonsensical.  In the first place, it fails to allow for views containing
WITH clauses.  But really it looks like it was cargo-culted over from
\ef/\sf without understanding why those commands number lines the way
they do.  The reason they do that is that for errors occurring inside a
function definition, the PL will typically report a line number relative
to the function body text, and so we're trying to be helpful about
interpreting line numbers of that kind.  But there's no comparable
behavior in the case of a view.  If you fat-finger a view, you'll get
a line number relative to the text of the whole CREATE command, eg

regression=# create or replace view z as
regression-# select 1/col
regression-# from bar;
ERROR:  relation "bar" does not exist
LINE 3: from bar;
             ^

So AFAICS, \ev and \sv should just number lines straightforwardly, with
"1" being the first line of the CREATE command text.  Am I missing
something?

			regards, tom lane

  12. Re: psql :: support for \ev viewname and \sv viewname

    Petr Korobeinikov <pkorobeinikov@gmail.com> — 2015-07-03T18:24:19Z

    пт, 3 июля 2015 г. в 19:30, Tom Lane <tgl@sss.pgh.pa.us>:

> So AFAICS, \ev and \sv should just number lines straightforwardly, with
> "1" being the first line of the CREATE command text.  Am I missing
> something?
>

Fixed. Now both \ev and \sv  numbering lines starting with "1". New version
attached.

As I've already noticed that pg_get_viewdef() does not support full syntax
of creating or replacing views. In my opinion, psql source code isn't the
place where some formatting hacks should be. So, can you give me an idea
how to produce already formatted output supporting "WITH" statement without
breaking backward compatibility of pg_get_viewdef() internals?

  13. Re: psql :: support for \ev viewname and \sv viewname

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-07-03T19:50:43Z

    Petr Korobeinikov <pkorobeinikov@gmail.com> writes:
> Fixed. Now both \ev and \sv  numbering lines starting with "1". New version
> attached.

Applied with a fair amount of mostly-cosmetic adjustment.

> As I've already noticed that pg_get_viewdef() does not support full syntax
> of creating or replacing views.

Oh?  If that were true, pg_dump wouldn't work on such views.  It is kind
of a PITA for this purpose that it doesn't include the CREATE text for
you, but we're surely not changing that behavior now.

			regards, tom lane

  14. Re: psql :: support for \ev viewname and \sv viewname

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2015-07-22T20:32:38Z

    On 3 July 2015 at 20:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Petr Korobeinikov <pkorobeinikov@gmail.com> writes:
>> Fixed. Now both \ev and \sv  numbering lines starting with "1". New version
>> attached.
>
> Applied with a fair amount of mostly-cosmetic adjustment.
>
>> As I've already noticed that pg_get_viewdef() does not support full syntax
>> of creating or replacing views.
>
> Oh?  If that were true, pg_dump wouldn't work on such views.  It is kind
> of a PITA for this purpose that it doesn't include the CREATE text for
> you, but we're surely not changing that behavior now.
>

This appears to be missing support for view options (WITH CHECK OPTION
and security_barrier), so editing a view with either of those options
will cause them to be stripped off.

Regards,
Dean

  15. Re: psql :: support for \ev viewname and \sv viewname

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-07-22T20:37:08Z

    Dean Rasheed <dean.a.rasheed@gmail.com> writes:
> On 3 July 2015 at 20:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Oh?  If that were true, pg_dump wouldn't work on such views.  It is kind
>> of a PITA for this purpose that it doesn't include the CREATE text for
>> you, but we're surely not changing that behavior now.

> This appears to be missing support for view options (WITH CHECK OPTION
> and security_barrier), so editing a view with either of those options
> will cause them to be stripped off.

Hm.  Why exactly were those not implemented as part of pg_get_viewdef?

			regards, tom lane

  16. Re: psql :: support for \ev viewname and \sv viewname

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2015-07-22T21:00:54Z

    On 22 July 2015 at 21:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Dean Rasheed <dean.a.rasheed@gmail.com> writes:
>> On 3 July 2015 at 20:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Oh?  If that were true, pg_dump wouldn't work on such views.  It is kind
>>> of a PITA for this purpose that it doesn't include the CREATE text for
>>> you, but we're surely not changing that behavior now.
>
>> This appears to be missing support for view options (WITH CHECK OPTION
>> and security_barrier), so editing a view with either of those options
>> will cause them to be stripped off.
>
> Hm.  Why exactly were those not implemented as part of pg_get_viewdef?
>

pg_get_viewdef in its current form is needed for the
information_schema "views" view, which has separate columns for the
view's query and its CHECK OPTIONs.

Arguably another function could be added. However, given the need for
psql to support older server versions, a new function wouldn't
actually help much, since psql would still need to be able to do it
the hard way in the absence of that new function on the server.

Regards,
Dean

  17. Re: psql :: support for \ev viewname and \sv viewname

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2016-04-27T07:36:10Z

    [Looking back over old threads]

On 22 July 2015 at 22:00, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> This appears to be missing support for view options (WITH CHECK OPTION
> and security_barrier), so editing a view with either of those options
> will cause them to be stripped off.

It seems like this issue was never addressed, and it needs to be fixed for 9.6.

Here is a rough patch based on the way pg_dump handles this. It still
needs a bit of polishing -- in particular I think fmtReloptionsArray()
(copied from pg_dump) should probably be moved to string_utils.c so
that it can be shared between pg_dump and psql. Also, I'm not sure
that's the best name for it -- I think appendReloptionsArray() is a
more accurate description of what is does.

Regards,
Dean

  18. Re: psql :: support for \ev viewname and \sv viewname

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2016-05-02T12:53:00Z

    On 27 April 2016 at 08:36, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
> Here is a rough patch based on the way pg_dump handles this. It still
> needs a bit of polishing -- in particular I think fmtReloptionsArray()
> (copied from pg_dump) should probably be moved to string_utils.c so
> that it can be shared between pg_dump and psql. Also, I'm not sure
> that's the best name for it -- I think appendReloptionsArray() is a
> more accurate description of what is does.
>

Here are updated patches doing that --- the first moves
fmtReloptionsArray() from pg_dump.c to fe_utils/string_utils.c so that
it can be shared by pg_dump and psql, and renames it to
appendReloptionsArray(). The second patch fixes the actual psql bug.

Regards,
Dean

  19. Re: psql :: support for \ev viewname and \sv viewname

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2016-05-03T15:52:23Z

    On 5/2/16 8:53 AM, Dean Rasheed wrote:
> Here are updated patches doing that --- the first moves
> fmtReloptionsArray() from pg_dump.c to fe_utils/string_utils.c so that
> it can be shared by pg_dump and psql, and renames it to
> appendReloptionsArray(). The second patch fixes the actual psql bug.

This looks reasonable.

I would change appendReloptionsArrayAH() to a function and keep AH as 
the first argument (similar to other functions that take such a handle).

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  20. Re: psql :: support for \ev viewname and \sv viewname

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2016-05-03T19:10:13Z

    On 3 May 2016 at 16:52, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> I would change appendReloptionsArrayAH() to a function and keep AH as the
> first argument (similar to other functions that take such a handle).

I can understand changing it to a function, but I don't think AH
should be the first argument. All other append*() functions that
append to a buffer have the buffer as the first argument, including
the appendStringLiteralAH() macro on which this is based.

Regards,
Dean

  21. Re: psql :: support for \ev viewname and \sv viewname

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2016-05-04T00:35:43Z

    
On 5/3/16 3:10 PM, Dean Rasheed wrote:
> On 3 May 2016 at 16:52, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> I would change appendReloptionsArrayAH() to a function and keep AH as the
>> first argument (similar to other functions that take such a handle).
>
> I can understand changing it to a function, but I don't think AH
> should be the first argument. All other append*() functions that
> append to a buffer have the buffer as the first argument, including
> the appendStringLiteralAH() macro on which this is based.

Well, all the functions that take archive handles have that as the first 
argument, so how do we consolidate that?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  22. Re: psql :: support for \ev viewname and \sv viewname

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2016-05-04T07:21:06Z

    On 4 May 2016 at 01:35, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 5/3/16 3:10 PM, Dean Rasheed wrote:
>> On 3 May 2016 at 16:52, Peter Eisentraut
>>>
>>> I would change appendReloptionsArrayAH() to a function and keep AH as the
>>> first argument (similar to other functions that take such a handle).
>>
>> I can understand changing it to a function, but I don't think AH
>> should be the first argument. All other append*() functions that
>> append to a buffer have the buffer as the first argument, including
>> the appendStringLiteralAH() macro on which this is based.
>
> Well, all the functions that take archive handles have that as the first
> argument, so how do we consolidate that?
>

Well, appendStringLiteralAH() takes both, so that sets a precedent.

And I think that makes sense too. The functions that take an archive
handle as their first argument are mostly functions whose primary
concern is to operate on the archive in some way. All the append*()
functions that take a buffer as their first argument are primarily
concerned with operating on the buffer. I'd say
appendStringLiteralAH() and appendReloptionsArrayAH() fall very much
into that second category. They only take an archive handle to get the
encoding and std_strings settings controlling *how* they operate on
the buffer. The main purpose of those append*() functions is to append
to a buffer, so it makes sense that that is their first argument.

All the append*() functions are consistent in their argument ordering,
including those that also take an archive handle, so I think
appendReloptionsArrayAH() should follow that pattern.

Regards,
Dean

  23. Re: psql :: support for \ev viewname and \sv viewname

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2016-05-04T12:23:38Z

    On 5/4/16 3:21 AM, Dean Rasheed wrote:
> Well, appendStringLiteralAH() takes both, so that sets a precedent.

Works for me.  Could you supply an updated patch with a static function 
instead of a macro?  Then I think this is good to go.

(bonus points for some tests)

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  24. Re: psql :: support for \ev viewname and \sv viewname

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2016-05-06T12:30:31Z

    On 4 May 2016 at 13:23, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 5/4/16 3:21 AM, Dean Rasheed wrote:
>> Well, appendStringLiteralAH() takes both, so that sets a precedent.
> Works for me.  Could you supply an updated patch with a static function
> instead of a macro?  Then I think this is good to go.
>
> (bonus points for some tests)
>

OK, pushed that way.

I didn't get round to adding any tests though. I strikes me that the
most likely bugs in this area are bugs of omission, like this and the
missing PARALLEL SAFE recently fixed for functions. Ideally tests
would be able to spot those kinds of issues, but it's not obvious how
to write such tests.

Regards,
Dean