Re: proposal: possibility to read dumped table's name from file

st 10. 6. 2020 v 0:30 odesílatel Justin Pryzby <pryzby@telsasoft.com>
napsal:

> On Tue, Jun 09, 2020 at 11:46:24AM +0200, Pavel Stehule wrote:
> > po 8. 6. 2020 v 23:30 odesílatel Justin Pryzby <pryzby@telsasoft.com>
> napsal:
>
> > I still wonder if a better syntax would use a unified --filter option,
> whose
> > > argument would allow including/excluding any type of object:
> > I tried to implement simple format "[+-][tndf] objectname"
>
> Thanks.
>
> > +                                             /* ignore empty rows */
> > +                                             if (*line != '\0')
>
> Maybe: if line=='\0': continue
>

ok


> We should also support comments.
>
> > +                                                     bool
> include_filter = false;
> > +                                                     bool
> exclude_filter = false;
>
> I think we only need one bool.
> You could call it: bool is_exclude = false
>
>
ok

> +
> > +                                                     if (chars < 4)
> > +
>  invalid_filter_format(optarg, line, lineno);
>
> I think that check is too lax.
> I think it's ok if we require the first char to be [-+] and the 2nd char
> to be
> [dntf]
>

> > +                                                     objecttype =
> line[1];
>
> ... but I think this is inadequately "liberal in what it accepts"; I think
> it
> should skip spaces.  In my proposed scheme, someone might reasonably write:
>
> > +
> > +                                                     objectname =
> &line[3];
> > +
> > +                                                     /* skip initial
> spaces */
> > +                                                     while (*objectname
> == ' ')
> > +
>  objectname++;
>
> I suggest to use isspace()
>

ok


> I think we should check that *objectname != '\0', rather than chars>=4,
> above.
>

done


> > +                                                             if
> (include_filter)
> > +                                                             {
> > +
>  simple_string_list_append(&table_include_patterns, objectname);
> > +
>  dopt.include_everything = false;
> > +                                                             }
> > +                                                             else if
> (exclude_filter)
> > +
>  simple_string_list_append(&table_exclude_patterns, objectname);
>
> If you use bool is_exclude, then this becomes "else" and you don't need to
> think about checking if (!include && !exclude).
>
> > +                                                     else if
> (objecttype == 'f')
> > +                                                     {
> > +                                                             if
> (include_filter)
> > +
>  simple_string_list_append(&foreign_servers_include_patterns, objectname);
> > +                                                             else if
> (exclude_filter)
> > +
>  invalid_filter_format(optarg, line, lineno);
> > +                                                     }
>
> I would handle invalid object types as "else: invalid_filter_format()"
> here,
> rather than duplicating above as: !=ALL('d','n','t','f')
>

good idea


> > +
> > +                                     if (ferror(f))
> > +                                             fatal("could not read from
> file \"%s\": %s",
> > +                                                       f == stdin ?
> "stdin" : optarg,
> > +                                                       strerror(errno));
>
> I think we're allowed to use %m here ?
>

changed


> > +     printf(_("  --filter=FILENAME            read object names from
> file\n"));
>
> Object name filter expression, or something..
>

yes, it is not object names now


> > + * getline is originaly GNU function, and should not be everywhere
> still.
> originally
>
> > + * Use own reduced implementation.
>
> Did you "reduce" this from another implementation?  Where?
> What is its license ?
>
> Maybe a line-reader already exists in the frontend (?) .. or maybe it
> should.
>

everywhere else is used a function fgets. Currently pg_getline is used just
on only one place, so I don't think so moving it to some common part is
maybe premature.

Maybe it can be used as replacement of some fgets calls, but then it is
different topic, I think.

Thank you for comments, attached updated patch

Regards

Pavel


> --
> Justin
>