From: | Andrew Dunstan <andrew(at)dunslane(dot)net> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Nathan Bossart <nathandbossart(at)gmail(dot)com>, Jelte Fennema-Nio <postgres(at)jeltef(dot)nl>, Amit Langote <amitlangote09(at)gmail(dot)com>, Jacob Champion <jacob(dot)champion(at)enterprisedb(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Peter Smith <smithpb2250(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: WIP Incremental JSON Parser |
Date: | 2024-04-08 22:20:13 |
Message-ID: | bb00a338-9027-4dea-8fd8-dca7e8ee785c@dunslane.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2024-04-08 Mo 09:29, Andrew Dunstan wrote:
>
> On 2024-04-07 Su 20:58, Tom Lane wrote:
>> Coverity complained that this patch leaks memory:
>>
>> /srv/coverity/git/pgsql-git/postgresql/src/bin/pg_combinebackup/load_manifest.c:
>> 212 in load_backup_manifest()
>> 206 bytes_left -= rc;
>> 207 json_parse_manifest_incremental_chunk(
>> 208 inc_state, buffer, rc, bytes_left == 0);
>> 209 }
>> 210
>> 211 close(fd);
>>>>> CID 1596259: (RESOURCE_LEAK)
>>>>> Variable "inc_state" going out of scope leaks the storage it
>>>>> points to.
>> 212 }
>> 213
>> 214 /* All done. */
>> 215 pfree(buffer);
>> 216 return result;
>> 217 }
>>
>> /srv/coverity/git/pgsql-git/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c:
>> 488 in parse_manifest_file()
>> 482 bytes_left -= rc;
>> 483 json_parse_manifest_incremental_chunk(
>> 484 inc_state, buffer, rc, bytes_left == 0);
>> 485 }
>> 486
>> 487 close(fd);
>>>>> CID 1596257: (RESOURCE_LEAK)
>>>>> Variable "inc_state" going out of scope leaks the storage it
>>>>> points to.
>> 488 }
>> 489
>> 490 /* Done with the buffer. */
>> 491 pfree(buffer);
>> 492
>> 493 return result;
>>
>> It's right about that AFAICS, and not only is the "inc_state" itself
>> leaked but so is its assorted infrastructure. Perhaps we don't care
>> too much about that in the existing applications, but ISTM that
>> isn't going to be a tenable assumption across the board. Shouldn't
>> there be a "json_parse_manifest_incremental_shutdown()" or the like
>> to deallocate all the storage allocated by the parser?
>
>
>
> yeah, probably. Will work on it.
>
>
>
Here's a patch. In addition to the leaks Coverity found, there was
another site in the backend code that should call the shutdown function,
and a probably memory leak from a logic bug in the incremental json
parser code. All these are fixed.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Attachment | Content-Type | Size |
---|---|---|
fix-incremental-manifest-parser-memory-leak.patch | text/x-patch | 3.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | David Rowley | 2024-04-08 22:23:28 | Re: enhance the efficiency of migrating particularly large tables |
Previous Message | David Zhang | 2024-04-08 21:52:13 | enhance the efficiency of migrating particularly large tables |