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

st 10. 6. 2020 v 0:30 odesílatel Justin Pryzby <pryzby(at)telsasoft(dot)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(at)telsasoft(dot)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
>