Обсуждение: Fixing memory leak in pg_upgrade

According to Coverity, there's a memory leak bug in transfer_all_new_dbs().
    mappings = gen_db_file_maps(old_db, new_db, &n_maps, old_pgdata,                                new_pgdata);
    if (n_maps)    {        print_maps(mappings, n_maps, new_db->db_name);

#ifdef PAGE_CONVERSION        pageConverter = setupPageConverter();
#endif        transfer_single_new_db(pageConverter, mappings, n_maps,                               old_tablespace);
        pg_free(mappings);    }
-----> leaks "mappings"}return;
}

This is because gen_db_file_maps() allocates memory even if n_maps == 0.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On Fri, Jan 9, 2015 at 9:23 PM, Tatsuo Ishii <ishii@postgresql.org> wrote:
> This is because gen_db_file_maps() allocates memory even if n_maps == 0.
Purely cosmetic: the initialization n_maps = 0 before the call of
gen_db_file_maps is unnecessary ;)
-- 
Michael

Tatsuo Ishii <ishii@postgresql.org> writes:
> According to Coverity, there's a memory leak bug in transfer_all_new_dbs().

It's pretty difficult to get excited about that; how many table-free
databases is pg_upgrade likely to see in one run?  But surely we could
just move the pg_free call to after the if-block.
        regards, tom lane

On Fri, Jan  9, 2015 at 11:34:24AM -0500, Tom Lane wrote:
> Tatsuo Ishii <ishii@postgresql.org> writes:
> > According to Coverity, there's a memory leak bug in transfer_all_new_dbs().
>
> It's pretty difficult to get excited about that; how many table-free
> databases is pg_upgrade likely to see in one run?  But surely we could
> just move the pg_free call to after the if-block.

I have fixed this with the attached, applied patch.  I thought malloc(0)
would return null, but our src/common pg_malloc() has:

    /* Avoid unportable behavior of malloc(0) */
    if (size == 0)
        size = 1;

so some memory is allocated, and has to be freed.  I looked at avoiding
the call to gen_db_file_maps() for old_db->rel_arr.nrels == 0, but there
are checks in there comparing the old/new relation counts, so it can't
be skipped.  I also removed the unnecessary memory initialization.

--
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + Everyone has their own god. +

> On Fri, Jan 9, 2015 at 9:23 PM, Tatsuo Ishii <ishii@postgresql.org> wrote:
>> This is because gen_db_file_maps() allocates memory even if n_maps == 0.
> Purely cosmetic: the initialization n_maps = 0 before the call of
> gen_db_file_maps is unnecessary ;)

Of course. n_maps is written by calling gen_db_file_maps() anyway. I
was talking about the case after calling gen_db_file_maps().

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On Sat, Jan 10, 2015 at 2:27 AM, Bruce Momjian <bruce@momjian.us> wrote:
> so some memory is allocated, and has to be freed.  I looked at avoiding
> the call to gen_db_file_maps() for old_db->rel_arr.nrels == 0, but there
> are checks in there comparing the old/new relation counts, so it can't
> be skipped.  I also removed the unnecessary memory initialization.
Patch is fine IMO for its purpose.
-- 
Michael