On 12/25/18 3:36 AM, Fabien COELHO wrote:
>
> Hello Andrew,
>
>> Rebased and updated patch attached.
>
> Here is a review of v5, sorry for the delay.
>
> Patch applies cleanly, compiles, "make check" is ok.
>
> I do not see Michaël's issue, and do not see how it could be so, for
> me the whole database-specific section generated by the underlying
> "pg_dump" call is removed, as expected.
>
> All is well for me, I turned the patch as ready.
OK, thanks.
>
>
> While poking around the dump output, I noticed some unrelated points:
>
> * Command "pg_dump" could tell which database is dumped in the output
> at the start of the section, eg:
>
> --
> -- PostgreSQL database "foo" dump
> --
>
> Or "pg_dumpall" could issue a comment line in the output telling which
> database is being considered.
>
> * The database dumps should have an introductory comment, like there
> is one for roles, eg:
>
> --
> -- Databases
> --
>
I agree these are unrelated but would be nice to have. Probably having
pg_dumpall do it would be better. Do you want to do a patch for that?
> * On extensions, the dump creates both the extension and the extension
> comment. However, ISTM that the extension comment is already created
> by the extension, so this is redundant:
>
> --
> -- Name: pg_dirtyread; Type: EXTENSION; Schema: -; Owner:
> --
> CREATE EXTENSION IF NOT EXISTS pg_dirtyread WITH SCHEMA public;
>
> --
> -- Name: EXTENSION pg_dirtyread; Type: COMMENT; Schema: -; Owner:
> --
> COMMENT ON EXTENSION pg_dirtyread IS 'Read dead but unvacuumed rows
> from table';
>
> Maybe it should notice that the comment belongs to the extension and
> need not be updated?
What if the owner had updated the comment after installing the extension?
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services