Thread: libpq: PQcmdStatus, PQcmdTuples signatures can be painlessly improved

Compare:
 int PQntuples(const PGresult *res)

Reasonable: doesn't modify 'res'.

With:
 char *PQcmdStatus(PGresult *res); char *PQcmdTuples(PGresult *res);

Unreasonable:
 a. What, these two can modify 'res' I pass in?..
 b. Oh, yes, because they return 'char *' pointing to    'res->cmdStatus+n', so, a libpq user may write:
       char *s = PQcmdStatus(res);       *s = 'x';
    and have 'res' modified.  (Would be the user's fault, of course.) 
The non-const-ness of 'PGresult *' for these two functions seems to
stand out among the functions covered in the "30.3.2. Retrieving Query
Result Information" manual section and inhibits writing the strict
client code.

I would suggest to change the signatures by applying this trivial
patch (and changing the documentation):

============================================================
== diff orig/postgresql-9.1.1/src/interfaces/libpq/libpq-fe.h ./postgresql-9.1.1/src/interfaces/libpq/libpq-fe.h
450c450
< extern char *PQcmdStatus(PGresult *res);
---
> extern const char *PQcmdStatus(const PGresult *res);
453c453
< extern char *PQcmdTuples(PGresult *res);
---
> extern const char *PQcmdTuples(const PGresult *res);
== diff orig/postgresql-9.1.1/src/interfaces/libpq/fe-exec.c ./postgresql-9.1.1/src/interfaces/libpq/fe-exec.c
2665,2666c2665,2666
< char *
< PQcmdStatus(PGresult *res)
---
> const char *
> PQcmdStatus(const PGresult *res)
2736,2737c2736,2737
< char *
< PQcmdTuples(PGresult *res)
---
> const char *
> PQcmdTuples(const PGresult *res)
2739,2740c2739
<       char       *p,
<                          *c;
---
>       const char *p, *c;
============================================================

(The above was obtained in 9.1.1; the subsequent build with GCC 4.1.2
succeeds without warnings.)

If the above change causes a warning in a client code, so much the
better: the client code is doing something unreasonable like the "*s"
assignment in my example above.

-- Alex -- alex-goncharov@comcast.net --

On Tue, Dec 13, 2011 at 7:55 AM, Alex Goncharov
<alex-goncharov@comcast.net> wrote:
> If the above change causes a warning in a client code, so much the
> better: the client code is doing something unreasonable like the "*s"
> assignment in my example above.

Or they just haven't bothered to decorate their entire code-base with
const declarations.

I suppose it's probably worth doing this, but I reserve the right to
be unexcited about it.  At a minimum, I dispute the application of the
term "painless" to any change involving const.

If you want this patch to be considered for application, you should
post an updated patch which includes the necessary doc changes and add
a link to it here:

https://commitfest.postgresql.org/action/commitfest_view/open

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Excerpts from Robert Haas's message of mar dic 13 12:51:54 -0300 2011:

> If you want this patch to be considered for application, you should
> post an updated patch which includes the necessary doc changes and add
> a link to it here:
>
> https://commitfest.postgresql.org/action/commitfest_view/open

Do we really need a 100% complete patch just to discuss whether we're
open to doing it?  IMHO it makes sense to see a WIP patch and then
accept or reject based on that; if we accept the general idea, then a
complete patch would presumably be submitted.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

,--- I/Alex (Tue, 13 Dec 2011 07:55:45 -0500) ----*
| If the above change causes a warning in a client code, so much the
| better: the client code is doing something unreasonable like the "*s"
| assignment in my example above.
,--- Robert Haas (Tue, 13 Dec 2011 10:51:54 -0500) ----*
| Or they just haven't bothered to decorate their entire code-base with
| const declarations.

They don't have to, for the conceptually correct code.  I.e. one can
write (with the old and new code):
 /* no: const */ PGresult *res; const char *readout; readout = PQxxx(res,...); /* no: *readout = 'x'; */

all right and have no compilation warnings.  

But one can also (reasonably) const-qualify the 'res' above
(const-correct and const-consistent code is a good thing.)
| If you want this patch to be considered for application, you should
| post an updated patch which includes the necessary doc changes and add
| a link to it here:
| 
| https://commitfest.postgresql.org/action/commitfest_view/open

OK, I could do it...

,--- Alvaro Herrera (Tue, 13 Dec 2011 13:01:13 -0300) ----*
| Do we really need a 100% complete patch just to discuss whether we're
| open to doing it?  IMHO it makes sense to see a WIP patch and then
| accept or reject based on that; if we accept the general idea, then a
| complete patch would presumably be submitted.
`---------------------------------------------------------*

... but I like  this more.

I.e., can one tell me to bother or not with the complete patch, based
on the general idea, which wouldn't change for the complete patch?

-- Alex -- alex-goncharov@comcast.net --

On Tue, Dec 13, 2011 at 11:01 AM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
> Excerpts from Robert Haas's message of mar dic 13 12:51:54 -0300 2011:
>> If you want this patch to be considered for application, you should
>> post an updated patch which includes the necessary doc changes and add
>> a link to it here:
>>
>> https://commitfest.postgresql.org/action/commitfest_view/open
>
> Do we really need a 100% complete patch just to discuss whether we're
> open to doing it?

Of course not.  You may notice that I also offered an opinion on the
substance of the patch.  In the course of doing that, I don't see why
I shouldn't point out that it's the patch author's responsibility to
provide docs.  YMMV, of course.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Excerpts from Alex Goncharov's message of mar dic 13 13:43:35 -0300 2011:

> I.e., can one tell me to bother or not with the complete patch, based
> on the general idea, which wouldn't change for the complete patch?

So let's see the patch.  In context diff format please, and also include
in-tree changes to the callers of those functions, if any are necessary.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On tis, 2011-12-13 at 07:55 -0500, Alex Goncharov wrote:
>   char *PQcmdStatus(PGresult *res);
>   char *PQcmdTuples(PGresult *res);
> 
> Unreasonable:
> 
>   a. What, these two can modify 'res' I pass in?..
> 
>   b. Oh, yes, because they return 'char *' pointing to
>      'res->cmdStatus+n', so, a libpq user may write:
> 
>         char *s = PQcmdStatus(res);
>         *s = 'x';
> 
>      and have 'res' modified.  (Would be the user's fault, of course.)
> 
Note that const PGresult * would only warn against changing the fields
of the PGresult struct.  It doesn't do anything about changing the data
pointed to by pointers in the PGresult struct.  So what you are saying
doesn't follow.

,--- You/Peter (Tue, 10 Jan 2012 19:13:42 +0200) ----*
| On tis, 2011-12-13 at 07:55 -0500, Alex Goncharov wrote:
| >   char *PQcmdStatus(PGresult *res);
| >   char *PQcmdTuples(PGresult *res);
| > 
| > Unreasonable:
| > 
| >   a. What, these two can modify 'res' I pass in?..
| > 
| >   b. Oh, yes, because they return 'char *' pointing to
| >      'res->cmdStatus+n', so, a libpq user may write:
| > 
| >         char *s = PQcmdStatus(res);
| >         *s = 'x';
| > 
| >      and have 'res' modified.  (Would be the user's fault, of course.)
| > 
| Note that const PGresult * would only warn against changing the
| fields

It would not warn, it would err (the compilation should fail).

| of the PGresult struct.  It doesn't do anything about changing the data
| pointed to by pointers in the PGresult struct.  So what you are saying
| doesn't follow.

By this logic, passing 'const struct foo *' doesn't have any point and
value, for any function.  But we know that this is done (and thank you
for that) in many cases -- a good style, self-documentation and some
protection.

E.g. here:

,--- I/Alex (Tue, 13 Dec 2011 07:55:45 -0500) ----*
| Compare:
| 
|   int PQntuples(const PGresult *res)
| 
| Reasonable: doesn't modify 'res'.
`-------------------------------------------------*

BTW, I have not submitted the context differences, as suggested, only
because of extreme overload at work and the need to do a careful
caller and documentation analysis. I still hope to be able to do it in
a reasonably near future.

-- Alex -- alex-goncharov@comcast.net --

On tis, 2012-01-10 at 14:10 -0500, Alex Goncharov wrote:
> | Note that const PGresult * would only warn against changing the
> | fields
> 
> It would not warn, it would err (the compilation should fail).

No, const violations generally only produce warnings.

> 
> | of the PGresult struct.  It doesn't do anything about changing the data
> | pointed to by pointers in the PGresult struct.  So what you are saying
> | doesn't follow.
> 
> By this logic, passing 'const struct foo *' doesn't have any point and
> value, for any function.

It pretty much doesn't.

> But we know that this is done (and thank you
> for that) in many cases -- a good style, self-documentation and some
> protection.

I'm not disagreeing, but I'm pointing out that it won't do what you
expect.