Re: WIP Incremental JSON Parser

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

In response to

Browse pgsql-hackers by date

  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