Re: pg_dumpall --exclude-database option

On 08/03/2018 05:08 PM, Fabien COELHO wrote:
>
>> Among other use cases, this is useful where a database name is
>> visible but the database is not dumpable by the user. Examples of
>> this occur in some managed Postgres services.
>
> This looks like a reasonable feature.

Thanks for the review.

>
>> I will add this to the September CF.
>
> My 0.02€:
>
> Patch applies cleanly, compiles, and works for me.
>
> A question: would it makes sense to have a symmetrical
> --include-database=PATTERN option as well?

I don't think so. If you only want a few databases, just use pg_dump.
The premise of pg_dumpall is that you want all of them and this switch
provides for exceptions to that.

>
> Somehow the option does not make much sense when under -g/-r/-t...
> maybe it should complain, like it does when the others are used together?

Added an error check.

>
> ISTM that it would have been better to issue just one query with an OR
> list, but that would require to extend "processSQLNamePattern" a
> little bit. Not sure whether it is worth it.

I don't think it is. This uses the same pattern that is used in
pg_dump.c for similar switches.

>
> Function "database_excluded": I'd suggest to consider reusing the
> "simple_string_list_member" function instead of reimplementing it in a
> special case.

done.

>
> XML doc: "--exclude-database=dbname", ISTM that
> "--exclude-database=pattern" would be closer to what it is? "Multiple
> database can be matched by writing multiple switches". Sure, but it
> can also be done with a pattern. The documentation seems to assume
> that the argument is one database name, and then changes this
> afterwards. I'd suggest to start by saying that a pattern like psql is
> expected, and then proceed to simply tell that the option can be
> repeated, instead of implying that it is a dbname and then telling
> that it is a pattern.

docco revised.

>
> The simple list is not freed. Ok, it seems to be part of the design of
> the data structure.

I don't see much point in freeing it.

revised patch attached.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services