Thread: [HACKERS] proposal psql \gdesc

Hello Pavel,

>> Sometimes I have to solve the result types of some query. It is invisible
>> in psql.

Indeed. My 0.02€ about this patch:


About the feature:

It looks useful to allow to show the resulting types of a query.


About the code:

Patch applies cleanly and compiles.

I'm afraid that re-using the "results" variable multiple times results in 
memory leaks... ISTM that new variables should be used when going down the 
nested conditions, and all cleanup should be done where and when 
necessary.

Also, maybe it would be better if the statement is cleaned up server side 
at the end of the execution. Not sure how to achieve that, though, libpq 
seems to lack the relevant function:-(
  """although there is no libpq function for deleting a prepared  statement, the SQL DEALLOCATE statement can be used
forthat purpose."""
 

Hmmm... I have not found how to use DEALLOCATE to cleanup an unnamed 
statement, it does not allow a "zero-length" name. Maybe it could be 
extended somehow, or a function could be provided for the purpose, eg
by passing a NULL query to PQprepare...

Resetting "gdesc flag" could be done only when the flag was true, at the 
end of the if, rather than on each query.

I understand that the second level query is to retrieve the type names in 
place of the Oid returned by QPftype?

The pg_malloc length computation looks a little bit arbitrary. Would it 
make sense to use PQescapeLiteral instead?


About the documentation:

I would suggest some rewording, maybe:

"Show the description of the result of the current query buffer without 
actually executing it, by considering it a prepared statement."


About tests:

There should be some non-regression tests added, maybe something like:
  SELECT    NULL AS zero,    1 AS one,    2.0 AS two,    'three' AS three,    $1 AS four,    CURRENT_DATE AS now    --
...   \gdesc
 

And also case which trigger an error, eg:
  SELECT $1 AS unknown_type \gdesc  SELECT 1 + \gdesc

Some fun:
  PREPARE test AS SELECT 1;  EXECUTE test \gdesc  ...

I'm unsure about some error messages, but it may be unrelated to this 
patch:
 calvin=# SELECT '1 km'::unit AS foo, $2 as boo \gdesc ERROR:  could not determine data type of parameter $1

-- 
Fabien.

Hello Pavel,

A complement to my previous comments:

> Also, maybe it would be better if the statement is cleaned up server side at 
> the end of the execution. Not sure how to achieve that, though, libpq seems 
> to lack the relevant function:-(
>
>  """although there is no libpq function for deleting a prepared
>  statement, the SQL DEALLOCATE statement can be used for that purpose."""
>
> Hmmm... I have not found how to use DEALLOCATE to cleanup an unnamed 
> statement, it does not allow a "zero-length" name. Maybe it could be extended 
> somehow, or a function could be provided for the purpose, eg
> by passing a NULL query to PQprepare...

After giving it some thoughts, I see three possible solutions:

0. Do nothing about it.   I would prefer the prepare is cleaned up.

1. assign a special name, eg "_psql_gdesc_", so that   DEALLOCATE "_psql_gdesc_" can be issued afterwards.

2. allow executing DEALLOCATE "";

3. add the missing PQdeallocate function to libpq?

Version 2 is server side, so it would not be compatible when connected 
to server running previous versions. Not desirable.

Version 3 may have implication at the protocol level and server side, if 
so it does not seem desirable to introduce such a change.

So maybe only version 1 is possible.

-- 
Fabien.

Hello Pavel,

>> After giving it some thoughts, I see three possible solutions:
>>
>> 0. Do nothing about it.
>>    I would prefer the prepare is cleaned up.
>>
>> 1. assign a special name, eg "_psql_gdesc_", so that
>>    DEALLOCATE "_psql_gdesc_" can be issued afterwards.
>>
>> [...]
>
> The doc says about unnamed prepared statements - any new unnamed prepared
> statement deallocates previous by self. From psql environment is not
> possible to create unnamed prepared statement.

That is a good point. It seems that it is not possible to execute it 
either.

> I prefer @0 agaisnt @1 because workflow is simple and robust. If unnamed PP
> doesn't exists, then it will be created, else it will be replaced. @1 has
> little bit more complex workflow, because there is not command like
> DEALLOCATE IF EXISTS, so I have to ensure deallocation in all possible
> ways.

ISTM That it is only of the PQprepare succeeded, so there should be only 
one point, at the end of the corresponding OK condition?

> Another reason for @0 is not necessity to generate some auxiliary
> name.

Yes. I do not like special names either. But I do not like keeping objects 
lives if they are dead. Not sure which is worst.

> My opinion in this case is not too strong - just I see the advantages of @0
> (robust and simple) nice. The question is about cost of unwanted allocated
> PP to end of session.

My opinion is not strong either, it is more the principle that I do not 
like to let things forever live in the server while they are known dead.

Hmmm. Strange. For some obscure reason, the unnamed prepared statement 
does not show in pg_prepared_statements:
  calvin=# PREPARE test AS SELECT 2;  calvin=# EXECUTE test;    -- 2  calvin=# SELECT 1 AS one \gdesc    -- one |
integer calvin=# SELECT * FROM  pg_prepared_statements ;    -- just one row:    -- test | PREPARE test AS SELECT 2;
│7..


Conclusion: Maybe let it as solution 0 for the time being, with a comment 
telling that it will be cleaned on the next unnamed PQprepare, and the 
committer will have its opinion.

-- 
Fabien.

Hello Pavel,

>> Patch applies cleanly and compiles.

Idem for v2. "make check" ok. Tests look good.

>> I would suggest some rewording, maybe:
>>
>> "Show the description of the result of the current query buffer without
>> actually executing it, by considering it a prepared statement."
>>
> done

Ok. If some native English speaker can clarify the sentence further, or 
imprive it anyway, thanks in advance!

>>   SELECT $1 AS unknown_type \gdesc
>
> It is not unknown type - the default placeholder type is text

Indeed. I really meant something like:
  calvin=# SELECT $1 + $2 \gdesc  ERROR:  operator is not unique: unknown + unknown  ...

More comments:

I propose that the help message could be "describe result of query without 
executing it".

I found an issue. \gdesk fails when the command does not return a result:
 calvin=# TRUNCATE pgbench_history \gdesc ERROR:  syntax error at or near ")" LINE 2:  (VALUES ) s (name, tp, tpm)

I guess the issue is that PQdescribePrepared returns an empty description, 
which is fine, but then the second query should be skipped, and some 
message should be output instead, like "no result" or whatever...

This need fixing, and a corresponding test should be added.

Also I would suggest to add a \g after the first test, which would execute 
the current buffer after its description, to show that the current buffer 
does indeed hold the query:
 calvin=# SELECT 1 as one, ... \gdesc \g -- one | int -- ... -- 1 | ...

-- 
Fabien.

Hello Pavel,

> [...] it is little bit worse. I cannot to distinguish between 
> SELECT\gdesc and TRUNCATE xxx\gdesc . All are valid commands and produce 
> empty result, so result of \gdesc command should be empty result too.
>
> postgres=# truncate table xx\gdesc
> ┌──────┬──────┐
> │ Name │ Type │
> ╞══════╪══════╡
> └──────┴──────┘
> (0 rows)

Hmmm. At least it is better than the previous error.

What about detecting the empty result (eg PQntuples()==0?) and writing 
"Empty result" instead of the strange looking empty table above? That 
would just mean skipping the PrintQueryResult call in this case?

-- 
Fabien.

On 5/3/17 02:56, Pavel Stehule wrote:
>     Sometimes I have to solve the result types of some query. It is
>     invisible in psql. You have to materialize table or you have to
>     create view. Now, when we can enhance \g command, we can introduce
>     query describing
> 
>     some like
> 
>     select a, b from foo
>     \gdesc
> 
>          |   type     | length | collation | ....
>     ------------------------------------------------
>      a  | varchar  |     30  |
>      b  | numeric |      20 | 
> 
> 
> here is the patch. It is based on PQdescribePrepared result.

I have often wished for functionality like this, so I'm in favor of
investigating this.

I don't think you need a separate call to prepare the query.  You can
get the result column types using PQftype().  (Hmm, you can get the
typmod that way, but not the collation.)

My thinking in the past has been to put the column types either in the
column headers, like "colname (coltype)", or in the footer, along with
the actual query result.

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

>> What about detecting the empty result (eg PQntuples()==0?) and writing
>> "Empty result" instead of the strange looking empty table above? That would
>> just mean skipping the PrintQueryResult call in this case?
>
> PQntuples == 0 every time - the query is not executed.

I meant to test the query which collects type names, which is executed?

Or check that PQnfields() == 0 on the PQdescribePrepared() result, so 
that there is no need to execute the type name collection query?

> For the case "SELECT;" the empty table is correct.

Ok. Then write "Empty table"?

> For TRUNCATE and similar command I am not sure. The empty table is maybe 
> unusual, but it is valid - like "SELECT;".

I would partly disagree:

"SELECT;" does indeed return an empty relation, so I agree that an empty 
table is valid whether spelled out as "Empty table" or explicitly.

However, ISTM that "TRUNCATE stuff;" does *NOT* return a relation, so 
maybe "No table" would be ok, but not an empty table... ?!

So I could be okay with both:
  SELECT \gdesc  -- "Empty table" or some other string
Or  -- Name | Type

Although I prefer the first one, because the second looks like a bug 
somehow: I asked for a description, but nothing is described... even if 
the answer is somehow valid, it looks pretty strange.

The same results do not realy suit "TRUNCATE Foo \gdesc", where "No table"
would seem more appropriate?

In both case, "Empty result" is kind of neutral, it does not promise a 
table or not. Hmmm. At least not too much. Or maybe some other string such 
as "Nothing" or "No result"?

Now I wonder whether the No vs Empty cases can be distinguished?

-- 
Fabien.

Hello Pavel,

> I am sending a variant with message instead empty result.

Thanks. Patch looks good, applies, make check ok, code is neat.

> Personally I prefer empty result instead message.

Hmm. I think that this version is less likely to raise questions from 
users, especially compared to having a somehow correct but strangely 
looking description.

> It is hard to choose some good text of this message. Empty result is 
> just empty result for all cases.

I'd suggest a very minor change: "No columns or command has no result" 
(not -> no). If some English native speaker has a better suggestion, 
fine with me.

Another good point of this version is that the type name query is 
simplified because it does not need to handle an empty result, thus the 
code is easier to understand.

A few other suggestions:
 - could you update the comment on the type name query?   Maybe the comment can be simply removed?
 - I'm wondering whether the Name & Type columns names should be   translatable. What do you think?
 - Maybe tests could also exercise unnamed columns, eg:    SELECT 1, 2, 3 \gdesc \g

-- 
Fabien.

Hello Pavel,

>>  - Maybe tests could also exercise unnamed columns, eg:
>>     SELECT 1, 2, 3 \gdesc \g
>
> done

Can't see it. No big deal, but if you put it it did not get through, and 
there is a warning with git apply on the very last line of the patch which 
may be linked to that:
  psql-gdesc-05.patch:328: new blank line at EOF.  +  warning: 1 line adds whitespace errors.

... especially as the two last tests are nearly the same now. I'm fine 
with a "one line" test, could be with some unnamed columns so that it is 
more different?

-- 
Fabien.

Hello Pavel,

v6 patch applies cleanly; make check ok; code, comments, doc & tests ok; 
various interactive tests I did ok.

Thanks for this useful little feature!

Let's see what committers think about it.

-- 
Fabien.

> ok - look on  new version, please

The patch needs a rebase after Tom's reindentation of tab-complete.

-- 
Fabien.

Hello Brent,

> Regarding the error message earlier

> 'No columns or command has no result',

> it might be clearer with the slightly longer

> 'The result has no columns or the command has no result'.

I agree that a better phrasing may be possible.

I'm hesitating about this one because word "result" appears twice, but 
this is the underlying issue, maybe there is no result, or there is a 
result but it is empty... so somehow this might be unavoidable. On 
rereading it, I think that your sentence is better balance as the two 
cases have both a verb and a structured the same, so it seems better.

Another terser version could be: 'No or empty result' or 'Empty or no 
result', but maybe it is too terse.

> I didn't read the patch though, just the email so that might not make 
> sense in context.

Thanks for the suggestion!

-- 
Fabien.

> new update - rebase, changed message

Thanks. New patch applies cleanly, make check still ok.

-- 
Fabien.

Hello Pavel,

> new update - rebase, changed message

I did yet another rebase of your patch after Tom alphabetically ordered 
backslash commands. Here is the result.

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

Fabien COELHO <coelho@cri.ensmp.fr> writes:
> I did yet another rebase of your patch after Tom alphabetically ordered 
> backslash commands. Here is the result.

Pushed with some massaging.
        regards, tom lane