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

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

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]http://softwarelivre.org/fisl13?lang=en (year 2012) me and other friends (Leonardo César,
Dickson Guedes and Fernando Ike) implemented a very WIP patch [2]https://github.com/lhcezar/postgres/commit/b4bfc3b17b4a32850d6035165209b2b82746190a 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
https://github.com/lhcezar/postgres/commit/b4bfc3b17b4a32850d6035165209b2b82746190a

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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).

Just a merge after pgindent run (807b9e0dff663c5da875af7907a5106c0ff90673).

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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.

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

пт, 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?

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers