allowing multiple PQclear() calls

Started by Josh Kupershmidtabout 13 years ago16 messages
#1Josh Kupershmidt
schmiddy@gmail.com

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

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Josh Kupershmidt (#1)
Re: allowing multiple PQclear() calls

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Marko Kreen
markokr@gmail.com
In reply to: Josh Kupershmidt (#1)
Re: allowing multiple PQclear() calls

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Simon Riggs
simon@2ndQuadrant.com
In reply to: Marko Kreen (#3)
Re: allowing multiple PQclear() calls

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Boszormenyi Zoltan
zb@cybertec.at
In reply to: Simon Riggs (#4)
Re: allowing multiple PQclear() calls

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/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Josh Kupershmidt
schmiddy@gmail.com
In reply to: Boszormenyi Zoltan (#5)
Re: allowing multiple PQclear() calls

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7Daniele Varrazzo
daniele.varrazzo@gmail.com
In reply to: Josh Kupershmidt (#6)
Re: allowing multiple PQclear() calls

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#8Simon Riggs
simon@2ndQuadrant.com
In reply to: Boszormenyi Zoltan (#5)
Re: allowing multiple PQclear() calls

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#9Boszormenyi Zoltan
zb@cybertec.at
In reply to: Simon Riggs (#8)
1 attachment(s)
Re: allowing multiple PQclear() calls

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/

Attachments:

pqclear-pqfinish-set-null.patchtext/x-patch; name=pqclear-pqfinish-set-null.patchDownload
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);
 
#10Marko Kreen
markokr@gmail.com
In reply to: Boszormenyi Zoltan (#9)
Re: allowing multiple PQclear() calls

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11Heikki Linnakangas
hlinnakangas@vmware.com
In reply to: Marko Kreen (#10)
Re: allowing multiple PQclear() calls

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12Boszormenyi Zoltan
zb@cybertec.at
In reply to: Marko Kreen (#10)
1 attachment(s)
Re: 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 <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/

Attachments:

pqclear-pqfinish-set-null-v2.patchtext/x-patch; name=pqclear-pqfinish-set-null-v2.patchDownload
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);
      </listitem>
     </varlistentry>
 
+    <varlistentry id="libpq-pqfinishsafe">
+     <term><function>PQfinishSafe</function><indexterm><primary>PQfinishSafe</></></term>
+     <listitem>
+      <para>
+       A macro that calls <function>PQfinish</function> and sets
+       the pointer to NULL afterwards.
+<synopsis>
+#define PQfinishSafe(conn) do { PQfinish(conn); conn = NULL; } while (0)
+</synopsis>
+      </para>
+
+      <para>
+       The <structname>PGconn</> pointer (being NULL) is safe to use
+       after this macro. Every function that deals with a
+       <structname>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.
+      </para>
+     </listitem>
+    </varlistentry>
+
     <varlistentry id="libpq-pqreset">
      <term><function>PQreset</function><indexterm><primary>PQreset</></></term>
      <listitem>
@@ -2753,6 +2774,29 @@ void PQclear(PGresult *res);
        </para>
       </listitem>
      </varlistentry>
+
+     <varlistentry id="libpq-pqclearsafe">
+      <term><function>PQclearSafe</function><indexterm><primary>PQclearSafe</></></term>
+      <listitem>
+       <para>
+        A macro to call <function>PQclear</function> and set
+        the pointer to NULL afterwards.
+<synopsis>
+#define PQclearSafe(res) do { PQclear(res); res = NULL; } while (0)
+</synopsis>
+       </para>
+
+       <para>
+        Calling <function>PQclear</function> leaves a stale
+        <structname>PGresult</structname> pointer. Calling
+        functions that deal with <structname>PGresult</structname>
+        objects afterwards may result in a crash. Setting the
+        <structname>PGresult</structname> pointer to NULL makes
+        calling such functions a no-op instead.
+       </para>
+      </listitem>
+     </varlistentry>
+
     </variablelist>
    </para>
   </sect2>
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);
 
#13Boszormenyi Zoltan
zb@cybertec.at
In reply to: Heikki Linnakangas (#11)
Re: 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<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/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#14Tom Lane
tgl@sss.pgh.pa.us
In reply to: Boszormenyi Zoltan (#13)
Re: allowing multiple PQclear() calls

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#15Dmitriy Igrishin
dmitigr@gmail.com
In reply to: Heikki Linnakangas (#11)
Re: allowing multiple PQclear() calls

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.

#16Boszormenyi Zoltan
zb@cybertec.at
In reply to: Tom Lane (#14)
Re: allowing multiple PQclear() calls

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/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers