Re: WIP Incremental JSON Parser

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.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com