Re: proposal psql \gdesc

Hi

2017-05-07 22:55 GMT+02:00 Fabien COELHO <coelho(at)cri(dot)ensmp(dot)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.