Thread: psql :: support for \ev viewname and \sv viewname

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

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
(withoutadditional parentheses). 


> On 05 May 2015, at 16:42, Robert Haas <robertmhaas@gmail.com> wrote:
>
> 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

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

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

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

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

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

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

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

[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

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

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

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

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

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

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

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