Horton

Thread

Commits

  1. Add fastpaths for when no objects are found

  2. Remove superfluous PQExpBuffer resetting

  1. Additional minor pg_dump cleanups

    Daniel Gustafsson <daniel@yesql.se> — 2024-07-03T07:37:32Z

    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

  2. Re: Additional minor pg_dump cleanups

    Ranier Vilela <ranier.vf@gmail.com> — 2024-07-03T11:29:15Z

    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);
+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

  3. Re: Additional minor pg_dump cleanups

    Daniel Gustafsson <daniel@yesql.se> — 2024-07-04T08:17:49Z

    > 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

  4. Re: Additional minor pg_dump cleanups

    Ranier Vilela <ranier.vf@gmail.com> — 2024-07-04T12:49:53Z

    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