Date: Mon, 24 Jan 2022 11:41:17 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Bruce Momjian <bruce@momjian.us>
Cc: Michael Paquier <michael@paquier.xyz>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	Tom Lane <tgl@sss.pgh.pa.us>, Daniel Gustafsson <daniel@yesql.se>,
	Andrew Dunstan <andrew@dunslane.net>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: pg_upgrade should truncate/remove its logs before running
Message-ID: <20220124174117.GW23027@telsasoft.com>
References: <20220108184857.GI14051@telsasoft.com>
 <Yd00xmV+rdffAQxf@paquier.xyz>
 <20220111200307.GA14051@telsasoft.com>
 <Yd5SOupY+FHbiXLD@paquier.xyz>
 <20220112040813.GD14051@telsasoft.com>
 <YefIHrNYTNH/PML4@paquier.xyz>
 <45e7680a-b617-1397-afa0-35a793aacefe@enterprisedb.com>
 <Yek+uQp/kMn2O3aK@paquier.xyz>
 <Ye4IDNnbMQ5IRxv9@paquier.xyz>
 <Ye7kUuo/+we1sipG@momjian.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Ye7kUuo/+we1sipG@momjian.us>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220124174117.GW23027%40telsasoft.com>
Precedence: bulk

On Mon, Jan 24, 2022 at 12:39:30PM -0500, Bruce Momjian wrote:
> On Mon, Jan 24, 2022 at 10:59:40AM +0900, Michael Paquier wrote:
> > On Thu, Jan 20, 2022 at 07:51:37PM +0900, Michael Paquier wrote:
> > > Neat idea.  That would work fine for my case.  So I am fine to stick
> > > with this suggestion. 
> > 
> > I have been looking at this idea, and the result is quite nice, being
> > simpler than anything that has been proposed on this thread yet.  We
> > get a simpler removal logic, and there is no need to perform any kind
> > of sanity checks with the output path provided as long as we generate
> > the paths and the dirs after adjust_data_dir().
> ...
> >  
> >    <para>
> >     <application>pg_upgrade</application> creates various working files, such
> > -   as schema dumps, in the current working directory.  For security, be sure
> > -   that that directory is not readable or writable by any other users.
> > +   as schema dumps, stored within <literal>pg_upgrade_output.d</literal> in
> > +   the directory of the new cluster.
> >    </para>
> 
> Uh, how are we instructing people to delete that pg_upgrade output
> directory?  If pg_upgrade completes cleanly, would it be removed
> automatically?

Clearly.

@@ -689,28 +751,5 @@ cleanup(void)                                                                                                                  
                                                                                                                                                    
        /* Remove dump and log files? */                                                                                                            
        if (!log_opts.retain)                                                                                                                       
-       {                                                                                                                                           
-               int                     dbnum;                                                                                                      
-               char      **filename;                                                                                                               
-                                                                                                                                                   
-               for (filename = output_files; *filename != NULL; filename++)                                                                        
-                       unlink(*filename);                                                                                                          
-                                                                                                                                                   
-               /* remove dump files */                                                                                                             
-               unlink(GLOBALS_DUMP_FILE);                                                                                                          
-                                                                                                                                                   
-               if (old_cluster.dbarr.dbs)                                                                                                          
-                       for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)                                                                    
-                       {                                                                                                                           
-                               char            sql_file_name[MAXPGPATH],                                                                           
-                                                       log_file_name[MAXPGPATH];                                                                   
-                               DbInfo     *old_db = &old_cluster.dbarr.dbs[dbnum];                                                                 
-                                                                                                                                                   
-                               snprintf(sql_file_name, sizeof(sql_file_name), DB_DUMP_FILE_MASK, old_db->db_oid);                                  
-                               unlink(sql_file_name);                                                                                              
-                                                                                                                                                   
-                               snprintf(log_file_name, sizeof(log_file_name), DB_DUMP_LOG_FILE_MASK, old_db->db_oid);                              
-                               unlink(log_file_name);                                                                                              
-                       }                                                                                                                           
-       }                                                                                                                                           
+               rmtree(log_opts.basedir, true);                                                                                                     
 }