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
