Thread: Additional minor pg_dump cleanups
Re-reading Nathans recent 8213df9effaf I noticed a few more small things which can be cleaned up. In two of the get<Object> functions we lack a fast-path for when no tuples are found which leads to pg_malloc(0) calls. Another thing is that we in one place reset the PQExpBuffer immediately after creating it which isn't required. -- Daniel Gustafsson
Attachment
Em qua., 3 de jul. de 2024 às 04:37, Daniel Gustafsson <daniel@yesql.se> escreveu:
Re-reading Nathans recent 8213df9effaf I noticed a few more small things which
can be cleaned up. In two of the get<Object> functions we lack a fast-path for
when no tuples are found which leads to pg_malloc(0) calls. Another thing is
that we in one place reset the PQExpBuffer immediately after creating it which
isn't required.
0001 Looks good to me.
0002:
With the function *getPublications* I think it would be good to free up the allocated memory?
}
+ pg_free(pubinfo);
+ pg_free(pubinfo);
+cleanup:
PQclear(res);
With the function *getExtensions* I think it would be good to return NULL in case ntups = 0?
Otherwise we may end up with an uninitialized variable.
- ExtensionInfo *extinfo;
+ ExtensionInfo *extinfo = NULL;
Funny, the function *getExtensionMembership* does not use the parameter ExtensionInfo extinfo.
getExtensions does not have another caller, Is it really necessary?
best regards,
Ranier Vilela
> On 3 Jul 2024, at 13:29, Ranier Vilela <ranier.vf@gmail.com> wrote: > With the function *getPublications* I think it would be good to free up the allocated memory? > > } > + pg_free(pubinfo); > +cleanup: > PQclear(res); Since the pubinfo is recorded in the DumpableObject and is responsible for keeping track of which publications to dump, it would be quite incorrect to free it here. > With the function *getExtensions* I think it would be good to return NULL in case ntups = 0? > Otherwise we may end up with an uninitialized variable. > > - ExtensionInfo *extinfo; > + ExtensionInfo *extinfo = NULL; I guess that won't hurt, though any code inspecting extinfo when numExtensions is returned as zero is flat-out wrong. It may however silence a static analyzer so there is that. > Funny, the function *getExtensionMembership* does not use the parameter ExtensionInfo extinfo. > getExtensions does not have another caller, Is it really necessary? Yes, see processExtensionTables(). -- Daniel Gustafsson
Attachment
Em qui., 4 de jul. de 2024 às 05:18, Daniel Gustafsson <daniel@yesql.se> escreveu:
> On 3 Jul 2024, at 13:29, Ranier Vilela <ranier.vf@gmail.com> wrote:
> With the function *getPublications* I think it would be good to free up the allocated memory?
>
> }
> + pg_free(pubinfo);
> +cleanup:
> PQclear(res);
Since the pubinfo is recorded in the DumpableObject and is responsible for
keeping track of which publications to dump, it would be quite incorrect to
free it here.
> With the function *getExtensions* I think it would be good to return NULL in case ntups = 0?
> Otherwise we may end up with an uninitialized variable.
>
> - ExtensionInfo *extinfo;
> + ExtensionInfo *extinfo = NULL;
I guess that won't hurt, though any code inspecting extinfo when numExtensions
is returned as zero is flat-out wrong. It may however silence a static
analyzer so there is that.
> Funny, the function *getExtensionMembership* does not use the parameter ExtensionInfo extinfo.
> getExtensions does not have another caller, Is it really necessary?
Yes, see processExtensionTables().
I saw, thanks.
LGTM.
best regards,
Ranier Vilela