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

ne 19. 3. 2023 v 15:01 odesílatel Justin Pryzby <pryzby(at)telsasoft(dot)com>
napsal:

> On Thu, Mar 16, 2023 at 01:05:41PM +0100, Pavel Stehule wrote:
> > rebase + enhancing about related option from a563c24
>
> Thanks.
>
> It looks like this doesn't currently handle extensions, which were added
> at 6568cef26e.
>
> > + <literal>table_and_children</literal>: tables, works like
> > + <option>-t</option>/<option>--table</option>, except that
> > + it also includes any partitions or inheritance child
> > + tables of the table(s) matching the
> > + <replaceable class="parameter">pattern</replaceable>.
>
> Why doesn't this just say "works like --table-and-children" ?
>
> I think as you wrote log_invalid_filter_format(), the messages wouldn't
> be translated, because they're printed via %s. One option is to call
> _() on the message.
>
> > +ok($dump !=~ qr/^CREATE TABLE public\.bootab/m, "exclude dumped
> children table");
>
> !=~ is being interpretted as as numeric "!=" and throwing warnings.
> It should be a !~ b, right ?
> It'd be nice if perl warnings during the tests were less easy to miss.
>
> > + * char is not alpha. The char '_' is allowed too (exclude first
> position).
>
> Why is it treated specially? Could it be treated the same as alpha?
>
> > + log_invalid_filter_format(&fstate,
> > +
> "\"include\" table data filter is not allowed");
> > + log_invalid_filter_format(&fstate,
> > +
> "\"include\" table data and children filter is not allowed");
>
> For these, it might be better to write the literal option:
>
> > +
> "include filter for \"table_data_and_children\" is not allowed");
>
> Because the option is a literal and shouldn't be translated.
> And it's probably better to write that using %s, like:
>
> > +
> "include filter for \"%s\" is not allowed");
>
> That makes shorter and fewer strings.
>
> Find attached a bunch of other corrections as 0002.txt
>

Thank you very much - I'll recheck the mentioned points tomorrow.

>
> I also dug up what I'd started in november, trying to reduce the code
> duplication betwen pg_restore/dump/all. This isn't done, but I might
> never finish it, so I'll at least show what I have in case you think
> it's a good idea. This passes tests on CI, except for autoconf, due to
> using exit_nicely() differently.
>

Your implementation reduced 60 lines, but the interface and code is more
complex. I cannot say what is significantly better. Personally, in this
case, I prefer my variant, because I think it is a little bit more
readable, and possible modification can be more simple. But this is just my
opinion, and I have no problem accepting other opinions. I can imagine to
define some configuration array like getopt, but it looks like over
engineering

Regards

Pavel

> --
> Justin
>