Change pfree to accept NULL argument

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

Per discussion in [0], here is a patch set that allows pfree() to accept
a NULL argument, like free() does.

So the question is, is this actually a good thing to do?

If we were starting in a green field, I'd be fine with defining
pfree(NULL) as okay. But we're not, so there are a couple of big
objections:

* Code developed to this standard will be unsafe to back-patch

* The sheer number of places touched will create back-patching
hazards.

I'm not very convinced that the benefits of making pfree() more
like free() are worth those costs.

We could ameliorate the first objection if we wanted to back-patch
0002, I guess.

(FWIW, no objection to your 0001. 0004 and 0005 seem okay too;
they don't touch enough places to create much back-patching risk.)

regards, tom lane

Hi,

On 2022-08-22 14:30:22 -0400, Tom Lane wrote:

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

Per discussion in [0], here is a patch set that allows pfree() to accept
a NULL argument, like free() does.

So the question is, is this actually a good thing to do?

If we were starting in a green field, I'd be fine with defining
pfree(NULL) as okay. But we're not, so there are a couple of big
objections:

* Code developed to this standard will be unsafe to back-patch

* The sheer number of places touched will create back-patching
hazards.

I'm not very convinced that the benefits of making pfree() more
like free() are worth those costs.

It's probably also not entirely cost free due to the added branches in place
we are certain that the pointer is non-null. That could partially be
ameliorated by moving the NULL pointer check into the callers.

If we don't want to go this route it might be worth adding a
pg_attribute_nonnull() or such to pfree().

(FWIW, no objection to your 0001. 0004 and 0005 seem okay too;
they don't touch enough places to create much back-patching risk.)

I like 0001, not sure I find 0004, 0005 an improvement.

Semi-related note: I've sometimes wished for a pfreep(void **p) that'd do
something like

if (*p)
{
pfree(*p);
*p = NULL;
}

so there's no dangling pointers after the pfree(), which often enoughis
important (e.g. because the code could be reached again if there's an error)
and is also helpful when debugging. The explicit form does bulk up code
sufficiently to be annoying.

Greetings,

Andres Freund

Per discussion in [0], here is a patch set that allows pfree() to accept
a NULL argument, like free() does.

Also, a patch that removes the now-unnecessary null pointer checks
before calling pfree(). And a few patches that do the same for some
other functions that I found around. (The one with FreeDir() is perhaps
a bit arguable, since FreeDir() wraps closedir() which does *not* accept
NULL arguments. Also, neither FreeFile() nor the underlying fclose()
accept NULL.)

Hi Peter,

+1

However, after a quick review, I noticed some cases of PQ freemen missing.
I took the liberty of making a v1, attached.

regards,

Ranier Vilela

On Tue, 23 Aug 2022 at 06:30, Tom Lane <tgl@sss.pgh.pa.us> wrote:

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

Per discussion in [0], here is a patch set that allows pfree() to accept
a NULL argument, like free() does.

So the question is, is this actually a good thing to do?

I think making pfree() accept NULL is a bad idea. The vast majority
of cases the pointer will never be NULL, so we're effectively just
burdening those with the additional overhead of checking for NULL.

We know from [1]/messages/by-id/CAApHDvr6qFw3jLBL9d4zUpo3A2Cb6hoZsUnWD0vF1OGsd67v=w@mail.gmail.com that adding branching in the memory management code
can be costly.

I'm measuring about a 2.6% slowdown from the 0002 patch using a
function that I wrote [2]/messages/by-id/attachment/136801/pg_allocate_memory_test.patch.txt to hammer palloc/pfree.

master
postgres=# select pg_allocate_memory_test(64, 1024*1024,
10::bigint*1024*1024*1024,'aset');
Time: 2007.527 ms (00:02.008)
Time: 1991.574 ms (00:01.992)
Time: 2008.945 ms (00:02.009)
Time: 2011.410 ms (00:02.011)
Time: 2019.317 ms (00:02.019)
Time: 2060.832 ms (00:02.061)
Time: 2003.066 ms (00:02.003)
Time: 2025.039 ms (00:02.025)
Time: 2039.744 ms (00:02.040)
Time: 2090.384 ms (00:02.090)

master + pfree modifed to check for NULLs
postgres=# select pg_allocate_memory_test(64, 1024*1024,
10::bigint*1024*1024*1024,'aset');
Time: 2057.625 ms (00:02.058)
Time: 2074.699 ms (00:02.075)
Time: 2075.629 ms (00:02.076)
Time: 2104.581 ms (00:02.105)
Time: 2072.620 ms (00:02.073)
Time: 2066.916 ms (00:02.067)
Time: 2071.962 ms (00:02.072)
Time: 2097.520 ms (00:02.098)
Time: 2087.421 ms (00:02.087)
Time: 2078.695 ms (00:02.079)

(~2.62% slowdown)

If the aim here is to remove a bunch of ugly if (ptr) pfree(ptr);
code, then why don't we just have a[n inline] function or a macro for
that and only use it when we need to?

David

[1]: /messages/by-id/CAApHDvr6qFw3jLBL9d4zUpo3A2Cb6hoZsUnWD0vF1OGsd67v=w@mail.gmail.com
[2]: /messages/by-id/attachment/136801/pg_allocate_memory_test.patch.txt

On Tue, 23 Aug 2022 at 13:17, David Rowley <dgrowleyml@gmail.com> wrote:

I think making pfree() accept NULL is a bad idea.

One counter argument to that is for cases like list_free_deep().
Right now if I'm not mistaken there's a bug (which I just noticed) in
list_free_private() that would trigger if you have a List of Lists and
one of the inner Lists is NIL. The code in list_free_private() just
seems to go off and pfree() whatever is stored in the element, which I
think would crash if it found a NIL List. If pfree() was to handle
NULLs at least that wouldn't have been a crash, but in reality, we
should probably fix that with recursion if we detect the element IsA
List type. If we don't use recursion, then the "free" does not seem
very "deep". (Or maybe it's too late to make it go deeper as it might
break existing code.)

David

On Wed, 24 Aug 2022 at 23:07, David Rowley <dgrowleyml@gmail.com> wrote:

One counter argument to that is for cases like list_free_deep().
Right now if I'm not mistaken there's a bug (which I just noticed) in
list_free_private() that would trigger if you have a List of Lists and
one of the inner Lists is NIL. The code in list_free_private() just
seems to go off and pfree() whatever is stored in the element, which I
think would crash if it found a NIL List. If pfree() was to handle
NULLs at least that wouldn't have been a crash, but in reality, we
should probably fix that with recursion if we detect the element IsA
List type. If we don't use recursion, then the "free" does not seem
very "deep". (Or maybe it's too late to make it go deeper as it might
break existing code.)

Hmm, that was a false alarm. It seems list_free_deep() can't really
handle freeing sublists as the list elements might be non-Node types,
which of course have no node tag, so we can't check for sub-Lists.

David

On 22.08.22 20:30, Tom Lane wrote:

I'm not very convinced that the benefits of making pfree() more
like free() are worth those costs.

We could ameliorate the first objection if we wanted to back-patch
0002, I guess.

(FWIW, no objection to your 0001. 0004 and 0005 seem okay too;
they don't touch enough places to create much back-patching risk.)

To conclude this, I have committed those secondary patches and updated
the utils/mmgr/README with some information from this discussion.