On Thu, Feb 17, 2022 at 12:08 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > I wrote: > > Justin Pryzby <pry...@telsasoft.com> writes: > >> pg_basebackup.c:1261:35: warning: storing the address of local variable > >> archive_filename in progress_filename [-Wdangling-pointer=] > >> => new in 23a1c6578 - looks like a real error > > > I saw that one a few days ago but didn't get around to looking > > more closely yet. It does look fishy, but it might be okay > > depending on when the global variable can be accessed. > > I got around to looking at this, and it absolutely is a bug. > The test scripts don't reach it, because they don't exercise -P > mode, let alone -P -v which is what you need to get progress_report() > to try to print the filename. However, if you modify the test > to do so, then you find that the output differs depending on > whether or not you add "progress_filename = NULL;" at the bottom > of CreateBackupStreamer(). So the variable is continuing to be > referenced, not only after it goes out of scope within that > function, but even after the function returns entirely. > (Interestingly, the output isn't obvious garbage, at least not > on my machine; so somehow the array's part of the stack is not > getting overwritten very soon.) > > A quick-and-dirty fix is > > - progress_filename = archive_filename; > + progress_filename = pg_strdup(archive_filename); > > but perhaps this needs more thought. How long is that filename > actually reasonable to show in the progress reports?
Man, you couldn't have asked a question that made me any happier. Preserving that behavior was one of the most annoying parts of this whole refactoring. The server always sends a series of tar files, but the pre-existing behavior is that progress reports show the archive name with -Ft and the name of the archive member with -Fp. I think that's sort of useful, but it's certainly annoying from an implementation perspective because we only know the name of the archive member if we're extracting the tarfile, possibly after decompressing it, whereas the name of the archive itself is easily known. This results in annoying gymnastics to try to update the global variable that we use to store this information from very different places depending on what the user requested, and evidently I messed that up, and also, why in the world does this code think that "more global variables" is the right answer to every problem? I'm not totally convinced that it's OK to just hit this progress reporting stuff with a hammer and remove some functionality in the interest of simplifying the code. But I will shed no tears if that's the direction other people would like to go. If not, I think that your quick-and-dirty fix is about right, except that we probably need to do it every place where we set progress_filename, and we should arrange to free the memory later. With -Ft, you won't have enough archives to matter, but with -Fp, you might have enough archive members to matter. -- Robert Haas EDB: http://www.enterprisedb.com
