Re: proposal: possibility to read dumped table's name from file - Mailing list pgsql-hackers

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

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

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

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

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

> +
> +                    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 ?

> +    printf(_("  --filter=FILENAME            read object names from file\n"));

Object name filter expression, or something..

> + * 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.

-- 
Justin