Re: Sloppiness around failure handling of parsePGArray in pg_dump

> On 11 Nov 2020, at 07:13, Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> I would like to propose the attached to tighten the error handling in
> the area, generating a fatal error if an array cannot be parsed.

I agree that we should fix this even if it will have quite limited impact in
production settings. Patch LGTM, +1.

> I did not see the point of changing the assumptions we use for the parsing of
> function args or such when it comes to pre-8.4 dumps.

Another thing caught my eye here (while not the fault of this patch), we ensure
to clean up array leftover in case of parsePGArray failure, but we don't clean
up the potential allocations from the previous calls. Something like the below
seems more consistent.

@@ -12105,6 +12099,8 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
nitems != nallargs)
{
pg_log_warning("could not parse proargmodes array");
+ if (allargtypes)
+ free(allargtypes);
if (argmodes)
free(argmodes);
argmodes = NULL;
@@ -12119,6 +12115,10 @@ dumpFunc(Archive *fout, FuncInfo *finfo)
nitems != nallargs)
{
pg_log_warning("could not parse proargnames array");
+ if (allargtypes)
+ free(allargtypes);
+ if (argmodes)
+ free(argmodes);
if (argnames)
free(argnames);
argnames = NULL;

> This issue is unlikely going to matter in practice, so I don't propose a
> backpatch.

Agreed, unless it's easier for dealing with backpatching other things, that
would be the only reason I reckon.

cheers ./daniel