Thread: psql: add \si, \sm, \st and \sr functions to show CREATE commands for indexes, matviews, triggers and tables

Hi,

I've attached a patch that implements \si, \sm, \st and \sr functions 
that show the CREATE command for indexes, matviews, triggers and tables. 
The functions are implemented similarly to the existing sf/sv functions 
with some modifications.

For triggers, I've decided to change input format to "table_name TRIGGER 
trigger_name", as multiple tables are allowed to have a trigger of the 
same name. Because we need to verify not only the name of the trigger, 
but also the name of the table, I've implemented a separate function 
lookup_trigger_oid that takes an additional argument.

Triggers and indexes use pg_catalog.pg_get_triggerdef() and 
pg_indexes.indexdef, while tables and matviews have separate queries for 
reconstruction. Get_create_object_cmd also runs three additional queries 
for tables, to get information on constraints, parents and columns.

There is also the question, if this functionality should be realised on 
the server instead of the client, but some people may think that changes 
to the language are "not the postgres way". However, server realisation 
may have some advantages, such as independence from the client and 
server version.

Best regards,
Alexandra Pervushina.

On 2020-07-28 20:46, a.pervushina@postgrespro.ru wrote:
> I've attached a patch that implements \si, \sm, \st and \sr functions
> that show the CREATE command for indexes, matviews, triggers and
> tables. The functions are implemented similarly to the existing sf/sv
> functions with some modifications.
> 
To me these functions seem useful.
As for adding them to server side, I don't see a big need for it. It 
feels more logical to follow the already eatablished pattern for the 
\s[...] commands.

About the patch:

1) There is some code duplication for the exec_command_[sm|si|st|sr] 
functions. Plus, it seems weird to separate sm (show matview) from sv 
(show view). Perhaps it would be more convenient to combine some of the 
code? Maybe by editing the already-existing exec_command_sf_sv() 
function.

2) Seeing how \s and \e functions were added together, I'm wondering - 
should there be \e functions too for any objects affected by this patch?

-- 
Anna Akenteva
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Anna Akenteva wrote 2020-08-11 13:37:
> About the patch:
> 
> 1) There is some code duplication for the exec_command_[sm|si|st|sr]
> functions. Plus, it seems weird to separate sm (show matview) from sv
> (show view). Perhaps it would be more convenient to combine some of
> the code? Maybe by editing the already-existing exec_command_sf_sv()
> function.

I've combined most of the functions into one, as the code was mostly 
duplicated. Had to change the argument from is_func to object type, 
because the number of values has increased. I've attached a patch with 
those changes.

a.pervushina@postgrespro.ru writes:
> [ si_st_sm_sr_v2.patch ]

I hadn't particularly noticed this thread before, but I happened to
look through this patch, and I've got to say that this proposed feature
seems like an absolute disaster from a maintenance standpoint.  There
will be no value in an \st command that is only 90% accurate; the produced
DDL has to be 100% correct.  This means that, if we accept this feature,
psql will have to know everything pg_dump knows about how to construct the
DDL describing tables, indexes, views, etc.  That is a lot of code, and
it's messy, and it changes nontrivially on a very regular basis.  I can't
accept that we want another copy in psql --- especially one that looks
nothing like what pg_dump has.

There've been repeated discussions about somehow extracting pg_dump's
knowledge into a library that would also be available to other client
programs (see e.g. the concurrent thread at [1]).  That's quite a tall
order, which is why it's not happened yet.  But I think we really need
to have something like that before we can accept this feature for psql.

BTW, as an example of why this is far more difficult than it might
seem at first glance, this patch doesn't even begin to meet the
expectation stated at the top of describe.c:

 * Support for the various \d ("describe") commands.  Note that the current
 * expectation is that all functions in this file will succeed when working
 * with servers of versions 7.4 and up.  It's okay to omit irrelevant
 * information for an old server, but not to fail outright.

It might be okay for this to cut off at 8.0 or so, as I think pg_dump
does, but not to just fail on older servers.

Another angle, which I'm not even sure how we want to think about it, is
security.  It will not do for "\et" to allow some attacker to replace
function calls appearing in the table's CHECK constraints, for instance.
So this means you've got to be very aware of CVE-2018-1058-style attacks.
Our answer to that for pg_dump has partially depended on restricting the
search_path used at both dump and restore time ... but I don't think \et
gets to override the search path that the psql user is using.  I'm not
sure what that means in practice but it certainly requires some thought
before we add the feature, not after.

Anyway, I can see the attraction of having psql commands like these,
but "write a bunch of new code that we'll have to maintain" does not
seem like a desirable way to get them.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/9df8a3d3-13d2-116d-26ab-6a273c1ed38c%402ndquadrant.com

On 18.08.2020 17:25, Tom Lane wrote:
> a.pervushina@postgrespro.ru writes:
>> [ si_st_sm_sr_v2.patch ]
> I hadn't particularly noticed this thread before, but I happened to
> look through this patch, and I've got to say that this proposed feature
> seems like an absolute disaster from a maintenance standpoint.  There
> will be no value in an \st command that is only 90% accurate; the produced
> DDL has to be 100% correct.  This means that, if we accept this feature,
> psql will have to know everything pg_dump knows about how to construct the
> DDL describing tables, indexes, views, etc.  That is a lot of code, and
> it's messy, and it changes nontrivially on a very regular basis.  I can't
> accept that we want another copy in psql --- especially one that looks
> nothing like what pg_dump has.
>
> There've been repeated discussions about somehow extracting pg_dump's
> knowledge into a library that would also be available to other client
> programs (see e.g. the concurrent thread at [1]).  That's quite a tall
> order, which is why it's not happened yet.  But I think we really need
> to have something like that before we can accept this feature for psql.
>
> BTW, as an example of why this is far more difficult than it might
> seem at first glance, this patch doesn't even begin to meet the
> expectation stated at the top of describe.c:
>
>   * Support for the various \d ("describe") commands.  Note that the current
>   * expectation is that all functions in this file will succeed when working
>   * with servers of versions 7.4 and up.  It's okay to omit irrelevant
>   * information for an old server, but not to fail outright.
>
> It might be okay for this to cut off at 8.0 or so, as I think pg_dump
> does, but not to just fail on older servers.
>
> Another angle, which I'm not even sure how we want to think about it, is
> security.  It will not do for "\et" to allow some attacker to replace
> function calls appearing in the table's CHECK constraints, for instance.
> So this means you've got to be very aware of CVE-2018-1058-style attacks.
> Our answer to that for pg_dump has partially depended on restricting the
> search_path used at both dump and restore time ... but I don't think \et
> gets to override the search path that the psql user is using.  I'm not
> sure what that means in practice but it certainly requires some thought
> before we add the feature, not after.
>
> Anyway, I can see the attraction of having psql commands like these,
> but "write a bunch of new code that we'll have to maintain" does not
> seem like a desirable way to get them.
>
>             regards, tom lane
>
> [1] https://www.postgresql.org/message-id/flat/9df8a3d3-13d2-116d-26ab-6a273c1ed38c%402ndquadrant.com
>
>

Since there has been no activity on this thread since before the CF and
no response from the author I have marked this "returned with feedback".

Alexandra, feel free to resubmit it to the next commitfest, when you 
have time to address the issues raised in the review.

-- 
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company