Re: Multiple -t options for pg_dump - Mailing list pgsql-patches

On Tue, Sep 20, 2005 at 12:15:23PM -0400, Neil Conway wrote:
> A few minor comments are below. This is for 8.2, right?

I am hoping to make a case for inclusion in 8.1.  The code is
completely isolated in one command and does not affect anyone who
does not use the new features.

> On Tue, 2005-20-09 at 08:51 -0700, David Fetter wrote:
> >   /* obsolete as of 7.3: */
> >   static Oid    g_last_builtin_oid; /* value of the last builtin oid
> > */
> >
> > ! static char **selectTableNames = NULL;        /* name(s) of
> > specified table(s) to dump */
> > ! int           tab_max = 0, tab_idx = 0;
> > ! static char *selectSchemaName = NULL; /* name(s) of specified
> > schema(ta) to dump */
> > ! int           schem_max = 0, schem_idx = 0;
> >
> >   char          g_opaque_type[10];      /* name for the opaque type */
>
> AFAICS the schema changes are unused. However, it would be worth
> enhancing -n to allow it to be specified multiple times as well. And
> to nit-pick, "tab_max" and "tab_idx" should be static. Also, within
> the PG source the plural of "schema" is "schemas".

OK, will fix when I get home from work.

>
> > !                       case 't':                       /* Dump data
> > for th(is|ese) table(s) only */
> > !                               if (tab_idx == tab_max) {
> > !                                       tab_max += 32;
> > !                                       if ( (selectTableNames =
> > realloc(selectTableNames, tab_max*sizeof(char *))) == 0)
> > !                                               exit(-1);
> > !                               }
>
> If you're going to check for calloc failure, you may as well check for
> strdup failure as well. Also, exit(1) would be more consistent with the
> rest of pg_dump. But personally I wouldn't bother checking for calloc
> (or strdup) failure, as the rest of pg_dump doesn't.

OK :)

> > *** 2414,2420 ****
> >   {
> >         PGresult   *res;
> >         int                     ntups;
> > !       int                     i;
> >         PQExpBuffer query = createPQExpBuffer();
> >         PQExpBuffer delqry = createPQExpBuffer();
> >         PQExpBuffer lockquery = createPQExpBuffer();
> > --- 2427,2433 ----
> >   {
> >         PGresult   *res;
> >         int                     ntups;
> > !       int                     i,j;
> >         PQExpBuffer query = createPQExpBuffer();
> >         PQExpBuffer delqry = createPQExpBuffer();
> >         PQExpBuffer lockquery = createPQExpBuffer();
>
> You should move j's definition to the scope closest to its use.

OK

>
> > --- 2706,2724 ----
> >          * simplistic since we don't fully check the combination of -n
> > and -t
> >          * switches.)
> >          */
> > !       if (selectTableNames != NULL)
> >         {
> > !               for (i = 0; i < ntups; i++) {
> > !                       for (j = 0; j < tab_idx; j++) {
> > !                               if (strcmp(tblinfo[i].dobj.name,
> > selectTableNames[j]) == 0)
> > !                                       goto check_match;
> > !                       }
> > !               }
> >                 /* Didn't find a match */
> > ! check_match: if (i == ntups)
> >                 {
> >                         write_msg(NULL, "specified table \"%s\" does
> > not exist\n",
> > !                                         selectTableNames[j]);
> >                         exit_nicely();
> >                 }
> >         }
>
> AFAICS the goto is unnecessary -- just break out of the loop when you
> find a match, and test i == ntups outside the loop.

I tried that.  How do I break out of both loops without a goto?

Cheers,
D
--
David Fetter david@fetter.org http://fetter.org/
phone: +1 510 893 6100   mobile: +1 415 235 3778

Remember to vote!