Re: [HACKERS] proposal psql \gdesc - Mailing list pgsql-hackers

From Pavel Stehule
Subject Re: [HACKERS] proposal psql \gdesc
Date
Msg-id CAFj8pRD8NM8X=drUR5NjmCZt5Ky7-7p113BdQT1_Mvv8VswHMA@mail.gmail.com
Whole thread Raw
In response to Re: [HACKERS] proposal psql \gdesc  (Fabien COELHO <coelho@cri.ensmp.fr>)
Responses Re: [HACKERS] proposal psql \gdesc  (Fabien COELHO <coelho@cri.ensmp.fr>)
List pgsql-hackers
Hi

2017-05-07 22:55 GMT+02:00 Fabien COELHO <coelho@cri.ensmp.fr>:

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

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

I don't think - now it is processed in  sendquery_cleanup and it is consistent with other flags. "if" is used only when some memory releasing is necessary.


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

yes, DESCRIBE doesn't return text type names.
 

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

done
 


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



done

 
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:

done
 

  SELECT $1 AS unknown_type \gdesc

It is not unknown type - the default placeholder type is text

 
  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

Looks like messy PostgreSQL error message. You miss "$1" placeholder

attached updated patch

Regards

Pavel



--
Fabien.

Attachment

pgsql-hackers by date:

Previous
From: amul sul
Date:
Subject: [HACKERS] Bug in pg_dump --table and --exclude-table for declarative partitiontable handling.
Next
From: Amit Langote
Date:
Subject: Re: [HACKERS] no test coverage for ALTER FOREIGN DATA WRAPPER nameHANDLER ...