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