Обсуждение: allowing multiple PQclear() calls

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

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

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

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

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/

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

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

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

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/

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

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

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/

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/

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

<div dir="ltr"><br /><div class="gmail_extra"><br /><br /><div class="gmail_quote">2013/1/2 Heikki Linnakangas <span
dir="ltr"><<ahref="mailto:hlinnakangas@vmware.com" target="_blank">hlinnakangas@vmware.com</a>></span><br
/><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div
class="im">On02.01.2013 17:27, Marko Kreen wrote:<br /><blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px#ccc solid;padding-left:1ex"> On Wed, Jan 2, 2013 at 5:11 PM, Boszormenyi Zoltan<<a
href="mailto:zb@cybertec.at"target="_blank">zb@cybertec.at</a>>  wrote:<br /><blockquote class="gmail_quote"
style="margin:00 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 2012-12-11 16:09 keltezéssel, Simon Riggs
írta:<br/><br /><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On11 December 2012 12:18, Boszormenyi Zoltan<<a href="mailto:zb@cybertec.at" target="_blank">zb@cybertec.at</a>>
 wrote:<br/><br /><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc
solid;padding-left:1ex"><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc
solid;padding-left:1ex"><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc
solid;padding-left:1ex">Such mechanism already exist - you just need to set<br /> your PGresult pointer to NULL after
eachPQclear().<br /></blockquote><br /> So why doesn't PQclear() do that?<br /></blockquote><br /><br /> Because then
PQclear()would need a ** not a *. Do you want its<br /> interface changed for 9.3 and break compatibility with previous
versions?<br/></blockquote><br /> No, but we should introduce a new public API call that is safer,<br /> otherwise we
getpeople continually re-inventing new private APIs that<br /> Do the Right Thing, as the two other respondents have
shown.<br/><br /></blockquote><br /> How about these macros?<br /></blockquote><br /> * Use do { } while (0) around the
macrosto get proper statement behaviour.<br /> * The if() is not needed, both PQclear and PQfinish do it internally.<br
/>* Docs<br /><br /> Should the names show somehow that they are macros?<br /> Or is it enough that it's mentioned in
documentation?<br/></blockquote><br /></div> IMHO this doesn't belong into libpq, the interface is fine as it is. It's
thecaller's responsibility to set the pointer to NULL after PQclear(), same as it's the caller's responsibility to set
apointer to NULL after calling free(), or to set the fd variable to -1 after calling close(fd). There's plenty of
precedencefor this pattern, and it shouldn't surprise any programmer.</blockquote><div style="style">True. +1</div><div
style="style"><br/></div></div>-- <br />// Dmitriy.<br /><br /></div></div> 

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/