Re: [HACKERS] allowing multiple PQclear() calls
2013-01-02 17:22 keltezéssel, Tom Lane írta: Boszormenyi Zoltan 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] allowing multiple PQclear() calls
2013/1/2 Heikki Linnakangas
> On 02.01.2013 17:27, Marko Kreen wrote:
>
>> On Wed, Jan 2, 2013 at 5:11 PM, Boszormenyi Zoltan
>> wrote:
>>
>>> 2012-12-11 16:09 keltezéssel, Simon Riggs írta:
>>>
>>> On 11 December 2012 12:18, Boszormenyi Zoltan 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.
Re: [HACKERS] allowing multiple PQclear() calls
Boszormenyi Zoltan 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] allowing multiple PQclear() calls
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 wrote:
2012-12-11 16:09 keltezéssel, Simon Riggs írta:
On 11 December 2012 12:18, Boszormenyi Zoltan 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/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] allowing multiple PQclear() calls
2013-01-02 16:27 keltezéssel, Marko Kreen írta:
On Wed, Jan 2, 2013 at 5:11 PM, Boszormenyi Zoltan wrote:
2012-12-11 16:09 keltezéssel, Simon Riggs írta:
On 11 December 2012 12:18, Boszormenyi Zoltan 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/
diff -durpN postgresql.3/doc/src/sgml/libpq.sgml postgresql.4/doc/src/sgml/libpq.sgml
--- postgresql.3/doc/src/sgml/libpq.sgml 2012-11-30 09:29:49.703286090 +0100
+++ postgresql.4/doc/src/sgml/libpq.sgml 2013-01-02 16:54:43.783565292 +0100
@@ -588,6 +588,27 @@ void PQfinish(PGconn *conn);
+
+ PQfinishSafePQfinishSafe
+
+
+ A macro that calls PQfinish and sets
+ the pointer to NULL afterwards.
+
+#define PQfinishSafe(conn) do { PQfinish(conn); conn = NULL; } while (0)
+
+
+
+
+ The PGconn pointer (being NULL) is safe to use
+ after this macro. Every function that deals with a
+ PGconn pointer considers a non-NULL pointer as valid.
+ Using a stale pointer may result in a crash. Setting the pointer
+ to NULL makes calling such functions a no-op instead.
+
+
+
+
PQresetPQreset
@@ -2753,6 +2774,29 @@ void PQclear(PGresult *res);
+
+
+ PQclearSafePQclearSafe
+
+
+A macro to call PQclear and set
+the pointer to NULL afterwards.
+
+#define PQclearSafe(res) do { PQclear(res); res = NULL; } while (0)
+
+
+
+
+Calling PQclear leaves a stale
+PGresult pointer. Calling
+functions that deal with PGresult
+objects afterwards may result in a crash. Setting the
+PGresult pointer to NULL makes
+calling such functions a no-op instead.
+
+
+
+
diff -durpN postgresql.3/src/interfaces/libpq/libpq-fe.h postgresql.4/src/interfaces/libpq/libpq-fe.h
--- postgresql.3/src/interfaces/libpq/libpq-fe.h 2013-01-02 09:19:03.929522614 +0100
+++ postgresql.4/src/interfaces/libpq/libpq-fe.h 2013-01-02 16:52:41.236683245 +0100
@@ -256,6 +256,9 @@ extern PGconn *PQsetdbLogin(const char *
/* close the current connection and free the PGconn data structure */
extern void PQfinish(PGconn *conn);
+/* macro to close the current connection and set the conn pointer to NULL */
+#define PQfinishSafe(conn) do { PQfinish(conn); conn = NULL; } while (0)
+
/* get info about connection options known to PQconnectdb */
extern PQconninfoOption *PQconndefaults(void);
@@ -472,6 +475,9 @@ extern int PQsendDescribePortal(PGconn *
/* Delete a PGresult */
extern void PQclear(PGresult *res);
+/* Macro to delete a PGresult and set the res pointer to NULL */
+#define PQclearSafe(res) do { PQclear(res); res = NULL; } while (0)
+
/* For freeing other alloc'd results, such as PGnotify structs */
extern void PQfreemem(void *ptr);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] allowing multiple PQclear() calls
On 02.01.2013 17:27, Marko Kreen wrote:
On Wed, Jan 2, 2013 at 5:11 PM, Boszormenyi Zoltan wrote:
2012-12-11 16:09 keltezéssel, Simon Riggs írta:
On 11 December 2012 12:18, Boszormenyi Zoltan 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] allowing multiple PQclear() calls
On Wed, Jan 2, 2013 at 5:11 PM, Boszormenyi Zoltan wrote:
> 2012-12-11 16:09 keltezéssel, Simon Riggs írta:
>
>> On 11 December 2012 12:18, Boszormenyi Zoltan 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] allowing multiple PQclear() calls
2012-12-11 16:09 keltezéssel, Simon Riggs írta:
On 11 December 2012 12:18, Boszormenyi Zoltan 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/
diff -durpN postgresql.3/src/interfaces/libpq/libpq-fe.h postgresql.4/src/interfaces/libpq/libpq-fe.h
--- postgresql.3/src/interfaces/libpq/libpq-fe.h 2013-01-02 09:19:03.929522614 +0100
+++ postgresql.4/src/interfaces/libpq/libpq-fe.h 2013-01-02 16:10:51.978974575 +0100
@@ -256,6 +256,16 @@ extern PGconn *PQsetdbLogin(const char *
/* close the current connection and free the PGconn data structure */
extern void PQfinish(PGconn *conn);
+/* macro to close the current connection and set the conn pointer to NULL */
+#define PQfinishSafe(conn) \
+ {\
+ if (conn) \
+ { \
+ PQfinish(conn); \
+ conn = NULL; \
+ } \
+ }
+
/* get info about connection options known to PQconnectdb */
extern PQconninfoOption *PQconndefaults(void);
@@ -472,6 +482,16 @@ extern int PQsendDescribePortal(PGconn *
/* Delete a PGresult */
extern void PQclear(PGresult *res);
+/* Macro to delete a PGresult and set the res pointer to NULL */
+#define PQclearSafe(res) \
+ {\
+ if (res) \
+ { \
+ PQclear(res); \
+ res = NULL; \
+ } \
+ }
+
/* For freeing other alloc'd results, such as PGnotify structs */
extern void PQfreemem(void *ptr);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] allowing multiple PQclear() calls
On 11 December 2012 12:18, Boszormenyi Zoltan 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] allowing multiple PQclear() calls
On Tue, Dec 11, 2012 at 12:43 PM, Josh Kupershmidt 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] allowing multiple PQclear() calls
On Tue, Dec 11, 2012 at 5:18 AM, Boszormenyi Zoltan wrote:
> 2012-12-11 12:45 keltezéssel, Simon Riggs írta:
>
>> On 11 December 2012 10:39, Marko Kreen wrote:
>>>
>>> On Tue, Dec 11, 2012 at 6:59 AM, Josh Kupershmidt
>>> 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] allowing multiple PQclear() calls
2012-12-11 12:45 keltezéssel, Simon Riggs írta: On 11 December 2012 10:39, Marko Kreen wrote: On Tue, Dec 11, 2012 at 6:59 AM, Josh Kupershmidt 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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] allowing multiple PQclear() calls
On 11 December 2012 10:39, Marko Kreen wrote: > On Tue, Dec 11, 2012 at 6:59 AM, Josh Kupershmidt 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] allowing multiple PQclear() calls
On Tue, Dec 11, 2012 at 6:59 AM, Josh Kupershmidt 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] allowing multiple PQclear() calls
Josh Kupershmidt 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
