Horton

Thread

  1. allowing multiple PQclear() calls

    Josh Kupershmidt <schmiddy@gmail.com> — 2012-12-11T04:59:45Z

    The documentation for PQclear() doesn't say whether it is safe to call
PQclear() more than once on the same PGresult pointer. In fact, it is
not safe, but apparently only because of this last step:
    /* Free the PGresult structure itself */
    free(res);

The other members of PGresult which may be freed by PQclear are set to
NULL or otherwise handled so as not to not be affected by a subsequent
PQclear().

I find that accounting for whether I've already PQclear'ed a given
PGresult can be quite tedious in some cases. For example, in the
cleanup code at the end of a function where control may goto in case
of a problem, it would be much simpler to unconditionally call
PQclear() without worrying about whether this was already done. One
can see an admittedly small illustration of this headache in
pqSetenvPoll() in our own codebase, where several times PQclear(res);
is called immediately before a goto error_return;

Would it be crazy to add an "already_freed" flag to the pg_result
struct which PQclear() would set, or some equivalent safety mechanism,
to avoid this hassle for users?

Josh

  2. Re: allowing multiple PQclear() calls

    Tom Lane <tgl@sss.pgh.pa.us> — 2012-12-11T07:12:17Z

    Josh Kupershmidt <schmiddy@gmail.com> writes:
> Would it be crazy to add an "already_freed" flag to the pg_result
> struct which PQclear() would set, or some equivalent safety mechanism,
> to avoid this hassle for users?

Yes, it would.  Once the memory has been freed, malloc() is at liberty
to give it out for some other purpose.  The only way we could make that
work reliably is to permanently leak the memory occupied by the PGresult
... which I trust you will agree is a cure worse than the disease.

			regards, tom lane

  3. Re: allowing multiple PQclear() calls

    Marko Kreen <markokr@gmail.com> — 2012-12-11T10:39:42Z

    On Tue, Dec 11, 2012 at 6:59 AM, Josh Kupershmidt <schmiddy@gmail.com> wrote:
> Would it be crazy to add an "already_freed" flag to the pg_result
> struct which PQclear() would set, or some equivalent safety mechanism,
> to avoid this hassle for users?

Such mechanism already exist - you just need to set
your PGresult pointer to NULL after each PQclear().

Later you can safely call PQclear() again on that pointer.

-- 
marko

  4. Re: allowing multiple PQclear() calls

    Simon Riggs <simon@2ndquadrant.com> — 2012-12-11T11:45:13Z

    On 11 December 2012 10:39, Marko Kreen <markokr@gmail.com> wrote:
> On Tue, Dec 11, 2012 at 6:59 AM, Josh Kupershmidt <schmiddy@gmail.com> wrote:
>> Would it be crazy to add an "already_freed" flag to the pg_result
>> struct which PQclear() would set, or some equivalent safety mechanism,
>> to avoid this hassle for users?
>
> Such mechanism already exist - you just need to set
> your PGresult pointer to NULL after each PQclear().

So why doesn't PQclear() do that?

Maintaining a pointer to something that no longer exists seems strange.

Under what conditions would anybody want the old pointer value after PQclear() ?

-- 
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  5. Re: allowing multiple PQclear() calls

    Boszormenyi Zoltan <zb@cybertec.at> — 2012-12-11T12:18:27Z

    2012-12-11 12:45 keltezéssel, Simon Riggs írta:
> On 11 December 2012 10:39, Marko Kreen <markokr@gmail.com> wrote:
>> On Tue, Dec 11, 2012 at 6:59 AM, Josh Kupershmidt <schmiddy@gmail.com> wrote:
>>> Would it be crazy to add an "already_freed" flag to the pg_result
>>> struct which PQclear() would set, or some equivalent safety mechanism,
>>> to avoid this hassle for users?
>> Such mechanism already exist - you just need to set
>> your PGresult pointer to NULL after each PQclear().
> So why doesn't PQclear() do that?

Because then PQclear() would need a ** not a *. Do you want its
interface changed for 9.3 and break compatibility with previous versions?
Same can be said for e.g. PQfinish(). Calling it again crashes your client,
as I have recently discovered when I added atexit() functions that
does "if (conn) PQfinish(conn);"  and the normal flow didn't do conn = NULL;
after it was done.

>
> Maintaining a pointer to something that no longer exists seems strange.
>
> Under what conditions would anybody want the old pointer value after PQclear() ?


>


-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

  6. Re: allowing multiple PQclear() calls

    Josh Kupershmidt <schmiddy@gmail.com> — 2012-12-11T12:43:08Z

    On Tue, Dec 11, 2012 at 5:18 AM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
> 2012-12-11 12:45 keltezéssel, Simon Riggs írta:
>
>> On 11 December 2012 10:39, Marko Kreen <markokr@gmail.com> wrote:
>>>
>>> On Tue, Dec 11, 2012 at 6:59 AM, Josh Kupershmidt <schmiddy@gmail.com>
>>> wrote:
>>>>
>>>> Would it be crazy to add an "already_freed" flag to the pg_result
>>>> struct which PQclear() would set, or some equivalent safety mechanism,
>>>> to avoid this hassle for users?
>>>
>>> Such mechanism already exist - you just need to set
>>> your PGresult pointer to NULL after each PQclear().
>>
>> So why doesn't PQclear() do that?
>
>
> Because then PQclear() would need a ** not a *. Do you want its
> interface changed for 9.3 and break compatibility with previous versions?
> Same can be said for e.g. PQfinish(). Calling it again crashes your client,
> as I have recently discovered when I added atexit() functions that
> does "if (conn) PQfinish(conn);"  and the normal flow didn't do conn = NULL;
> after it was done.

Ah, well. I guess using a macro like:

#define SafeClear(res) do {PQclear(res); res = NULL;} while (0);

will suffice for me.

Josh

  7. Re: allowing multiple PQclear() calls

    Daniele Varrazzo <daniele.varrazzo@gmail.com> — 2012-12-11T13:15:27Z

    On Tue, Dec 11, 2012 at 12:43 PM, Josh Kupershmidt <schmiddy@gmail.com> wrote:

> Ah, well. I guess using a macro like:
>
> #define SafeClear(res) do {PQclear(res); res = NULL;} while (0);
>
> will suffice for me.

Psycopg uses:

#define IFCLEARPGRES(pgres)  if (pgres) {PQclear(pgres); pgres = NULL;}

-- Daniele

  8. Re: allowing multiple PQclear() calls

    Simon Riggs <simon@2ndquadrant.com> — 2012-12-11T15:09:05Z

    On 11 December 2012 12:18, Boszormenyi Zoltan <zb@cybertec.at> wrote:

>>> Such mechanism already exist - you just need to set
>>> your PGresult pointer to NULL after each PQclear().
>>
>> So why doesn't PQclear() do that?
>
>
> Because then PQclear() would need a ** not a *. Do you want its
> interface changed for 9.3 and break compatibility with previous versions?

No, but we should introduce a new public API call that is safer,
otherwise we get people continually re-inventing new private APIs that
Do the Right Thing, as the two other respondents have shown.

-- 
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  9. Re: allowing multiple PQclear() calls

    Boszormenyi Zoltan <zb@cybertec.at> — 2013-01-02T15:11:49Z

    2012-12-11 16:09 keltezéssel, Simon Riggs írta:
> On 11 December 2012 12:18, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>
>>>> Such mechanism already exist - you just need to set
>>>> your PGresult pointer to NULL after each PQclear().
>>> So why doesn't PQclear() do that?
>>
>> Because then PQclear() would need a ** not a *. Do you want its
>> interface changed for 9.3 and break compatibility with previous versions?
> No, but we should introduce a new public API call that is safer,
> otherwise we get people continually re-inventing new private APIs that
> Do the Right Thing, as the two other respondents have shown.
>

How about these macros?

Best regards,
Zoltán Böszörményi

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

  10. Re: allowing multiple PQclear() calls

    Marko Kreen <markokr@gmail.com> — 2013-01-02T15:27:15Z

    On Wed, Jan 2, 2013 at 5:11 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
> 2012-12-11 16:09 keltezéssel, Simon Riggs írta:
>
>> On 11 December 2012 12:18, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>>
>>>>> Such mechanism already exist - you just need to set
>>>>> your PGresult pointer to NULL after each PQclear().
>>>>
>>>> So why doesn't PQclear() do that?
>>>
>>>
>>> Because then PQclear() would need a ** not a *. Do you want its
>>> interface changed for 9.3 and break compatibility with previous versions?
>>
>> No, but we should introduce a new public API call that is safer,
>> otherwise we get people continually re-inventing new private APIs that
>> Do the Right Thing, as the two other respondents have shown.
>>
>
> How about these macros?

* Use do { } while (0) around the macros to get proper statement behaviour.
* The if() is not needed, both PQclear and PQfinish do it internally.
* Docs

Should the names show somehow that they are macros?
Or is it enough that it's mentioned in documentation?

-- 
marko

  11. Re: allowing multiple PQclear() calls

    Heikki Linnakangas <hlinnakangas@vmware.com> — 2013-01-02T15:52:36Z

    On 02.01.2013 17:27, Marko Kreen wrote:
> On Wed, Jan 2, 2013 at 5:11 PM, Boszormenyi Zoltan<zb@cybertec.at>  wrote:
>> 2012-12-11 16:09 keltezéssel, Simon Riggs írta:
>>
>>> On 11 December 2012 12:18, Boszormenyi Zoltan<zb@cybertec.at>  wrote:
>>>
>>>>>> Such mechanism already exist - you just need to set
>>>>>> your PGresult pointer to NULL after each PQclear().
>>>>>
>>>>> So why doesn't PQclear() do that?
>>>>
>>>>
>>>> Because then PQclear() would need a ** not a *. Do you want its
>>>> interface changed for 9.3 and break compatibility with previous versions?
>>>
>>> No, but we should introduce a new public API call that is safer,
>>> otherwise we get people continually re-inventing new private APIs that
>>> Do the Right Thing, as the two other respondents have shown.
>>>
>>
>> How about these macros?
>
> * Use do { } while (0) around the macros to get proper statement behaviour.
> * The if() is not needed, both PQclear and PQfinish do it internally.
> * Docs
>
> Should the names show somehow that they are macros?
> Or is it enough that it's mentioned in documentation?

IMHO this doesn't belong into libpq, the interface is fine as it is. 
It's the caller's responsibility to set the pointer to NULL after 
PQclear(), same as it's the caller's responsibility to set a pointer to 
NULL after calling free(), or to set the fd variable to -1 after calling 
close(fd). There's plenty of precedence for this pattern, and it 
shouldn't surprise any programmer.

- Heikki

  12. Re: allowing multiple PQclear() calls

    Boszormenyi Zoltan <zb@cybertec.at> — 2013-01-02T16:00:44Z

    2013-01-02 16:27 keltezéssel, Marko Kreen írta:
> On Wed, Jan 2, 2013 at 5:11 PM, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>> 2012-12-11 16:09 keltezéssel, Simon Riggs írta:
>>
>>> On 11 December 2012 12:18, Boszormenyi Zoltan <zb@cybertec.at> wrote:
>>>
>>>>>> Such mechanism already exist - you just need to set
>>>>>> your PGresult pointer to NULL after each PQclear().
>>>>> So why doesn't PQclear() do that?
>>>>
>>>> Because then PQclear() would need a ** not a *. Do you want its
>>>> interface changed for 9.3 and break compatibility with previous versions?
>>> No, but we should introduce a new public API call that is safer,
>>> otherwise we get people continually re-inventing new private APIs that
>>> Do the Right Thing, as the two other respondents have shown.
>>>
>> How about these macros?
> * Use do { } while (0) around the macros to get proper statement behaviour.
> * The if() is not needed, both PQclear and PQfinish do it internally.
> * Docs
>
> Should the names show somehow that they are macros?
> Or is it enough that it's mentioned in documentation?

Done. The fact that these are macros is mentioned in the docs.

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

  13. Re: allowing multiple PQclear() calls

    Boszormenyi Zoltan <zb@cybertec.at> — 2013-01-02T16:02:46Z

    2013-01-02 16:52 keltezéssel, Heikki Linnakangas írta:
> On 02.01.2013 17:27, Marko Kreen wrote:
>> On Wed, Jan 2, 2013 at 5:11 PM, Boszormenyi Zoltan<zb@cybertec.at>  wrote:
>>> 2012-12-11 16:09 keltezéssel, Simon Riggs írta:
>>>
>>>> On 11 December 2012 12:18, Boszormenyi Zoltan<zb@cybertec.at>  wrote:
>>>>
>>>>>>> Such mechanism already exist - you just need to set
>>>>>>> your PGresult pointer to NULL after each PQclear().
>>>>>>
>>>>>> So why doesn't PQclear() do that?
>>>>>
>>>>>
>>>>> Because then PQclear() would need a ** not a *. Do you want its
>>>>> interface changed for 9.3 and break compatibility with previous versions?
>>>>
>>>> No, but we should introduce a new public API call that is safer,
>>>> otherwise we get people continually re-inventing new private APIs that
>>>> Do the Right Thing, as the two other respondents have shown.
>>>>
>>>
>>> How about these macros?
>>
>> * Use do { } while (0) around the macros to get proper statement behaviour.
>> * The if() is not needed, both PQclear and PQfinish do it internally.
>> * Docs
>>
>> Should the names show somehow that they are macros?
>> Or is it enough that it's mentioned in documentation?
>
> IMHO this doesn't belong into libpq, the interface is fine as it is. It's the caller's 
> responsibility to set the pointer to NULL after PQclear(), same as it's the caller's 
> responsibility to set a pointer to NULL after calling free(), or to set the fd variable 
> to -1 after calling close(fd). There's plenty of precedence for this pattern, and it 
> shouldn't surprise any programmer.

Let me quote Simon Riggs here:
> ... we should introduce a new public API call that is safer,
> otherwise we get people continually re-inventing new private APIs that
> Do the Right Thing, as the two other respondents have shown.

Best regards,
Zoltán Böszörményi

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/

  14. Re: allowing multiple PQclear() calls

    Tom Lane <tgl@sss.pgh.pa.us> — 2013-01-02T16:22:44Z

    Boszormenyi Zoltan <zb@cybertec.at> writes:
> 2013-01-02 16:52 keltezssel, Heikki Linnakangas rta:
>> IMHO this doesn't belong into libpq, the interface is fine as it is. It's the caller's 
>> responsibility to set the pointer to NULL after PQclear(), same as it's the caller's 
>> responsibility to set a pointer to NULL after calling free(), or to set the fd variable 
>> to -1 after calling close(fd). There's plenty of precedence for this pattern, and it 
>> shouldn't surprise any programmer.

> Let me quote Simon Riggs here:
>> ... we should introduce a new public API call that is safer,
>> otherwise we get people continually re-inventing new private APIs that
>> Do the Right Thing, as the two other respondents have shown.

Heikki is right and Simon is wrong.  This is not a very helpful idea,
and anybody who wants it is probably doing it already anyway.

There might be some value in the proposed macro if using it reliably
stopped bugs of this class, but it won't; the most obvious reason being
that there is seldom only one copy of a given PGconn pointer in an
application.  If you just blindly s/PQfinish(x)/PQfinishSafe(&x)/
you will most likely be zapping some low-level function's local
parameter copy, and thus accomplishing nothing of use.  You could
possibly make it work fairly consistently if you changed your entire
application to pass around PGconn** instead of PGconn*, but that would
be tedious and somewhat error-prone in itself; and none of the rest of
libpq's API is at all friendly to the idea.

The context where this sort of thing is actually helpful is C++, where
it's possible to introduce enough low-level infrastructure that the
programmer doesn't have to think about what he's doing when using
indirect pointers like that.  You can make a C++ wrapper class that is
both guaranteed safe (unlike this) and notationally transparent.
Of course, that has its own costs, but at least the language provides
some support.  So it'd be reasonable for libpqxx to do something like
this, but it's not very helpful for libpq to do it.  As Heikki says,
there is basically zero precedent for it in libc or other C libraries.
There's a reason for that.

			regards, tom lane

  15. Re: allowing multiple PQclear() calls

    Dmitry Igrishin <dmitigr@gmail.com> — 2013-01-02T16:30:05Z

    2013/1/2 Heikki Linnakangas <hlinnakangas@vmware.com>

> On 02.01.2013 17:27, Marko Kreen wrote:
>
>> On Wed, Jan 2, 2013 at 5:11 PM, Boszormenyi Zoltan<zb@cybertec.at>
>>  wrote:
>>
>>> 2012-12-11 16:09 keltezéssel, Simon Riggs írta:
>>>
>>>  On 11 December 2012 12:18, Boszormenyi Zoltan<zb@cybertec.at>  wrote:
>>>>
>>>>  Such mechanism already exist - you just need to set
>>>>>>> your PGresult pointer to NULL after each PQclear().
>>>>>>>
>>>>>>
>>>>>> So why doesn't PQclear() do that?
>>>>>>
>>>>>
>>>>>
>>>>> Because then PQclear() would need a ** not a *. Do you want its
>>>>> interface changed for 9.3 and break compatibility with previous
>>>>> versions?
>>>>>
>>>>
>>>> No, but we should introduce a new public API call that is safer,
>>>> otherwise we get people continually re-inventing new private APIs that
>>>> Do the Right Thing, as the two other respondents have shown.
>>>>
>>>>
>>> How about these macros?
>>>
>>
>> * Use do { } while (0) around the macros to get proper statement
>> behaviour.
>> * The if() is not needed, both PQclear and PQfinish do it internally.
>> * Docs
>>
>> Should the names show somehow that they are macros?
>> Or is it enough that it's mentioned in documentation?
>>
>
> IMHO this doesn't belong into libpq, the interface is fine as it is. It's
> the caller's responsibility to set the pointer to NULL after PQclear(),
> same as it's the caller's responsibility to set a pointer to NULL after
> calling free(), or to set the fd variable to -1 after calling close(fd).
> There's plenty of precedence for this pattern, and it shouldn't surprise
> any programmer.

True. +1

-- 
// Dmitriy.

  16. Re: allowing multiple PQclear() calls

    Boszormenyi Zoltan <zb@cybertec.at> — 2013-01-02T16:36:23Z

    2013-01-02 17:22 keltezéssel, Tom Lane írta:
> Boszormenyi Zoltan <zb@cybertec.at> writes:
>> 2013-01-02 16:52 keltezéssel, Heikki Linnakangas írta:
>>> IMHO this doesn't belong into libpq, the interface is fine as it is. It's the caller's
>>> responsibility to set the pointer to NULL after PQclear(), same as it's the caller's
>>> responsibility to set a pointer to NULL after calling free(), or to set the fd variable
>>> to -1 after calling close(fd). There's plenty of precedence for this pattern, and it
>>> shouldn't surprise any programmer.
>> Let me quote Simon Riggs here:
>>> ... we should introduce a new public API call that is safer,
>>> otherwise we get people continually re-inventing new private APIs that
>>> Do the Right Thing, as the two other respondents have shown.
> Heikki is right and Simon is wrong.  This is not a very helpful idea,
> and anybody who wants it is probably doing it already anyway.
>
> There might be some value in the proposed macro if using it reliably
> stopped bugs of this class, but it won't; the most obvious reason being
> that there is seldom only one copy of a given PGconn pointer in an
> application.  If you just blindly s/PQfinish(x)/PQfinishSafe(&x)/
> you will most likely be zapping some low-level function's local
> parameter copy, and thus accomplishing nothing of use.  You could
> possibly make it work fairly consistently if you changed your entire
> application to pass around PGconn** instead of PGconn*, but that would
> be tedious and somewhat error-prone in itself; and none of the rest of
> libpq's API is at all friendly to the idea.
>
> The context where this sort of thing is actually helpful is C++, where
> it's possible to introduce enough low-level infrastructure that the
> programmer doesn't have to think about what he's doing when using
> indirect pointers like that.  You can make a C++ wrapper class that is
> both guaranteed safe (unlike this) and notationally transparent.
> Of course, that has its own costs, but at least the language provides
> some support.  So it'd be reasonable for libpqxx to do something like
> this, but it's not very helpful for libpq to do it.  As Heikki says,
> there is basically zero precedent for it in libc or other C libraries.
> There's a reason for that.
>
> 			regards, tom lane

OK, then forget about this patch.

Best regards,
Zoltán Böszörményi

-- 
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
      http://www.postgresql.at/