libpq: Remove redundant null pointer checks before free()

On Thu, Jun 16, 2022 at 10:07:33PM +0200, Peter Eisentraut wrote:

calls, where the "if" part is unnecessary. This is of course pretty
harmless, but some functions like scram_free() and freePGconn() have become
so bulky that it becomes annoying. So while I was doing some work in that
area I undertook to simplify this.

Seems fine. Would some of the buildfarm dinosaurs hiccup on that?
gaur is one that comes into mind.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Thu, Jun 16, 2022 at 10:07:33PM +0200, Peter Eisentraut wrote:

calls, where the "if" part is unnecessary. This is of course pretty
harmless, but some functions like scram_free() and freePGconn() have become
so bulky that it becomes annoying. So while I was doing some work in that
area I undertook to simplify this.

Seems fine. Would some of the buildfarm dinosaurs hiccup on that?
gaur is one that comes into mind.

Doubt it. (In any case, gaur/pademelon are unlikely to be seen
again after a hardware failure --- I'm working on resurrecting that
machine using modern NetBSD on an external drive, but its HPUX
installation probably isn't coming back.)

POSIX has required free(NULL) to be a no-op since at least SUSv2 (1997).
Even back then, the machines that failed on it were legacy devices,
like then-decade-old SunOS versions. So I don't think that Peter's
proposal has any portability risk today.

Having said that, the pattern "if (x) free(x);" is absolutely
ubiquitous across our code, and so I'm not sure that I'm on
board with undoing it only in libpq. I'd be happier if we made
a push to get rid of it everywhere. Notably, I think the choice
that pfree(NULL) is disallowed traces directly to worries about
coding-pattern-compatibility with pre-POSIX free(). Should we
revisit that?

Independently of that concern, how much of a back-patch hazard
might we create with such changes?

regards, tom lane

On 17.06.22 05:25, Michael Paquier wrote:

On Thu, Jun 16, 2022 at 10:07:33PM +0200, Peter Eisentraut wrote:

calls, where the "if" part is unnecessary. This is of course pretty
harmless, but some functions like scram_free() and freePGconn() have become
so bulky that it becomes annoying. So while I was doing some work in that
area I undertook to simplify this.

Seems fine. Would some of the buildfarm dinosaurs hiccup on that?
gaur is one that comes into mind.

I'm pretty sure PostgreSQL code already depends on this behavior anyway.
The code just isn't consistent about it.

On 17.06.22 07:11, Tom Lane wrote:

Having said that, the pattern "if (x) free(x);" is absolutely
ubiquitous across our code, and so I'm not sure that I'm on
board with undoing it only in libpq. I'd be happier if we made
a push to get rid of it everywhere.

Sure, here is a more comprehensive patch set. (It still looks like
libpq is the largest chunk.)

Notably, I think the choice
that pfree(NULL) is disallowed traces directly to worries about
coding-pattern-compatibility with pre-POSIX free(). Should we
revisit that?

Yes please, and also repalloc().

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:

On 17.06.22 07:11, Tom Lane wrote:

Notably, I think the choice
that pfree(NULL) is disallowed traces directly to worries about
coding-pattern-compatibility with pre-POSIX free(). Should we
revisit that?

Yes please, and also repalloc().

repalloc no, because you wouldn't know which context to do the
allocation in.

regards, tom lane

On Fri, Jun 17, 2022 at 09:03:23PM +0200, Peter Eisentraut wrote:

I'm pretty sure PostgreSQL code already depends on this behavior anyway.
The code just isn't consistent about it.

In the frontend, I'd like to think that you are right and that we have
already some places doing that. The backend is a different story,
like in GetMemoryChunkContext(). That should be easy enough to check
with some LD_PRELOAD wizardry, at least.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Fri, Jun 17, 2022 at 09:03:23PM +0200, Peter Eisentraut wrote:

I'm pretty sure PostgreSQL code already depends on this behavior anyway.
The code just isn't consistent about it.

In the frontend, I'd like to think that you are right and that we have
already some places doing that.

We do, indeed.

The backend is a different story,
like in GetMemoryChunkContext(). That should be easy enough to check
with some LD_PRELOAD wizardry, at least.

Huh? The proposal is to accept the fact that free() tolerates NULL,
and then maybe make pfree() tolerate it as well. I don't think that
that needs to encompass any other functions.

regards, tom lane

On 2022-Jun-17, Peter Eisentraut wrote:

From 355ef1a68be690d9bb8ee0524226abd648733ce0 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 17 Jun 2022 12:09:32 +0200
Subject: [PATCH v2 3/3] Remove redundant null pointer checks before PQclear
and PQconninfofree

These functions already had the free()-like behavior of handling NULL
pointers as a no-op. But it wasn't documented, so add it explicitly
to the documentation, too.

For PQclear() specifically, one thing that I thought a few days ago
would be useful would to have it return (PGresult *) NULL. Then the
very common pattern of doing "PQclear(res); res = NULL;" could be
simplified to "res = PQclear(res);", which is nicely compact and is
learned instantly.

I've not seen this convention used anywhere else though, and I'm not
sure I'd advocate it for other functions where we use similar patterns
such as pfree/pg_free, so perhaps it'd become too much of a special
case.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

For PQclear() specifically, one thing that I thought a few days ago
would be useful would to have it return (PGresult *) NULL. Then the
very common pattern of doing "PQclear(res); res = NULL;" could be
simplified to "res = PQclear(res);", which is nicely compact and is
learned instantly.

That's a public API unfortunately, and so some people would demand
a libpq.so major version bump if we changed it.

regards, tom lane