On 11/17/18 9:55 AM, Alvaro Herrera wrote:
> The comment in expand_dbname_patterns is ungrammatical and mentions
> "OID" rather than "name", so I suggest
>
> /*
> * The loop below might sometimes result in duplicate entries in the
> * output name list, but we don't care.
> */
Will fix.
> I'm not sure this is grammatical either:
> exclude databases whose name matches PATTERN
> I would have written it like this:
> exclude databases whose names match PATTERN
> but I'm not sure (each database has only one name, of course, but aren't
> you talking about multiple databases there?)
I think the original is grammatical.
> Other than that, the patch seems fine to me -- I tested and it works as
> intended.
>
> Personally I would say "See also expand_table_name_patterns" instead of
> "This is similar to code in pg_dump.c for handling matching table names.",
> as well as mention this function in expand_table_name_patterns' comment.
> (No need to mention expand_schema_name_patterns, since these are
> adjacent.) But this is mostly stylistic and left to your own judgement.
>
> In the long run, I think we should add an option to processSQLNamePattern
> to use OR instead of AND, which would fix both this problem as well as
> pg_dump's. I don't think that's important enough to stall this patch.
Thanks for the review.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
