improve PQexec documentation

Started by Fabien COELHOalmost 7 years ago5 messages
#1Fabien COELHO
coelho@cri.ensmp.fr
1 attachment(s)

Hello devs,

I'm looking at psql's use of PQexec for implementing some feature.

When running with multiple SQL commands, the doc is not very helpful.

From the source code I gathered that PQexec returns the first COPY results
if any, and if not the last non-empty results, unless all is empty in
which case an empty result is returned. So * marks the returned result
in the following examples:

INSERT ... \; * COPY ... \; SELECT ... \; \;
SELECT ... \; UPDATE ... \; * SELECT ... \; \;
\; \; * ;

The attached patch tries to improve the documentation based on my
understanding.

IMVHO, psql's code is kind of a mess to work around this strange behavior,
as there is a loop over results within PQexec, then another one after
PQexec if there were some COPY.

--
Fabien.

Attachments:

libpq-pqexec-doc-1.patchtext/x-diff; name=libpq-pqexec-doc-1.patchDownload
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 7f01fcc148..de9a0652a1 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2488,9 +2488,14 @@ PGresult *PQexec(PGconn *conn, const char *command);
     for more details about how the server handles multi-query strings.)
     Note however that the returned
     <structname>PGresult</structname> structure describes only the result
-    of the last command executed from the string.  Should one of the
-    commands fail, processing of the string stops with it and the returned
-    <structname>PGresult</structname> describes the error condition.
+    of the last command executed from the string.
+    If the strings contains <command>COPY</command> commands, the result of the
+    first of those is returned and further results should be fetched with
+    <function>PQgetResult</function>.
+    If the string contains only empty commands, an empty result is returned.
+    Otherwise, the result of the last non-empty command is returned.
+    Should one of the commands fail, processing of the string stops with it and
+    the returned <structname>PGresult</structname> describes the error condition.
    </para>
 
    <para>
#2Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Fabien COELHO (#1)
Re: improve PQexec documentation

On 2019-Apr-12, Fabien COELHO wrote:

I'm looking at psql's use of PQexec for implementing some feature.

When running with multiple SQL commands, the doc is not very helpful.

From the source code I gathered that PQexec returns the first COPY results
if any, and if not the last non-empty results, unless all is empty in which
case an empty result is returned.

I'm not sure we necessarily want to document this behavior. If it was
super helpful for some reason, or if we thought we would never change
it, then it would make sense to document it in minute detail. But
otherwise I think documenting it sets a promise that we would (try to)
never change it in the future, which I don't necessarily agree with --
particularly since it's somewhat awkward to use.

I'm inclined to reject this patch.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#3Fabien COELHO
coelho@cri.ensmp.fr
In reply to: Alvaro Herrera (#2)
Re: improve PQexec documentation

Hello Alvaro,

I'm looking at psql's use of PQexec for implementing some feature.

When running with multiple SQL commands, the doc is not very helpful.

From the source code I gathered that PQexec returns the first COPY results
if any, and if not the last non-empty results, unless all is empty in which
case an empty result is returned.

I'm not sure we necessarily want to document this behavior. If it was
super helpful for some reason, or if we thought we would never change
it, then it would make sense to document it in minute detail. But
otherwise I think documenting it sets a promise that we would (try to)
never change it in the future, which I don't necessarily agree with --
particularly since it's somewhat awkward to use.

I'm inclined to reject this patch.

Hmmm. I obviously agree that PQexec is beyond awkward.

Now I'm not sure how anyone is expected to guess the actual function
working from the available documentation, and without this knowledge I
cannot see how to write meaningful code for the multiple query case.

Basically it seems to have been designed for simple queries, and then
accomodated somehow for the multiple case but with a strange non
systematic approach.

I think it would have been much simpler and straightforward to always
return the first result and let the client do whatever it wants
afterwards. However, as it has existed for quite some time, I'm unsure how
likely it is to change as it would break existing code, so documenting its
behavior seems logical. I'd be all in favor of changing the behavior, but
I'm pessimistic that it could pass. Keeping the current status (not really
documented & awkward behavior) seems rather strange.

--
Fabien.

#4Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Fabien COELHO (#3)
Re: improve PQexec documentation

On 2019-04-12 17:51, Fabien COELHO wrote:

Hmmm. I obviously agree that PQexec is beyond awkward.

Now I'm not sure how anyone is expected to guess the actual function
working from the available documentation, and without this knowledge I
cannot see how to write meaningful code for the multiple query case.

But you're not really supposed to use it for multiple queries or
multiple result sets anyway. There are other functions for this.

If a source code comment in libpq or psql would help explaining some of
the current code, then we could add that. But I am also not sure that
enshrining the current behavior on the API documentation is desirable.

Basically it seems to have been designed for simple queries, and then
accomodated somehow for the multiple case but with a strange non
systematic approach.

probably

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

#5Thomas Munro
thomas.munro@gmail.com
In reply to: Peter Eisentraut (#4)
Re: improve PQexec documentation

On Sat, Apr 13, 2019 at 1:12 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

I'm inclined to reject this patch.

On Fri, Jul 5, 2019 at 6:47 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

But you're not really supposed to use it for multiple queries or
multiple result sets anyway. There are other functions for this.

If a source code comment in libpq or psql would help explaining some of
the current code, then we could add that. But I am also not sure that
enshrining the current behavior on the API documentation is desirable.

Hi Fabien,

Based on the above, I have marked this as "Returned with feedback".

--
Thomas Munro
https://enterprisedb.com