Re: [PATCH] Silence Valgrind about SelectConfigFiles()

From: Aleksander Alekseev <aleksander(at)tigerdata(dot)com>
To: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: [PATCH] Silence Valgrind about SelectConfigFiles()
Date: 2025-08-14 11:09:35
Message-ID: CAJ7c6TMFgp5p9Erdh5PODXbwc2MT8ogiZQx3mcN0O3TXuOpoAQ@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Tom,

> Yeah, I noticed that too, and it does offend my inner neatnik.
>
> Instead of what you did, I'd be inclined to add
>
> free(configdir);
>
> return true;
> +
> +fail:
> + free(configdir);
> +
> + return false;
> }
>
> and then s/return false/goto fail/ throughout, so as to avoid
> duplicating the free() calls. It's a minor point as things stand,
> but if more cleanup gets added to the function I think it'd be
> easier to maintain this way.

Makes sense. Here is the corrected patch v2.

> Huh ... don't quite see where in that recipe we'd reach a
> SelectConfigFiles error exit.

How exactly we reach this code patch is a good question. I tried to
understand the exact conditions by using my steps to reproduce and an
ancient debugging technique with sleep(), elog() and `watch` - see
trick.txt. Unfortunately I was not able to reproduce it again nor
under Valgrind nor without it. I guess it means that either I did
something differently before or the right conditions are met under
rare circumstances.

Attachment Content-Type Size
trick.txt text/plain 1.5 KB
v2-0001-Fix-memory-leaks-in-SelectConfigFiles.patch text/x-patch 2.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Shinya Kato 2025-08-14 11:12:55 Add mode column to pg_stat_progress_vacuum
Previous Message Alastair Turner 2025-08-14 10:56:26 Re: Proposal: Conflict log history table for Logical Replication