On Wed, Aug 14, 2024 at 9:20 AM Amul Sul <sulamul@gmail.com> wrote:
> I agree with keeping verify_backup_file() separate, but I'm hesitant
> about doing the same for verify_backup_directory().
I don't have time today to go through your whole email or re-review
the code, but I plan to circle back to that at a later time, However,
I want to respond to this point in the meanwhile. There are two big
things that are different for a tar-format backup vs. a
directory-format backup as far as verify_backup_directory() is
concerned. One is that, for a directory format backup, we need to be
able to recurse down through subdirectories; for tar-format backups we
don't. So a version of this function that only handled tar-format
backups would be somewhat shorter and simpler, and would need one
fewer argument. The second difference is that for the tar-format
backup, you need to make a list of the files you see and then go back
and visit each one a second time, and for a directory-format backup
you don't need to do that. It seems to me that those differences are
significant enough to warrant having two separate functions. If you
unify them, I think that less than half of the resulting function is
going to be common to both cases. Yeah, a few bits of logic will be
duplicated, like the error handling for closedir(), the logic to skip
"." and "..", and the psprintf() to construct a full pathname for the
directory entry. But that isn't really very much code, and it's code
that is pretty straightforward and also present in various other
places in the PostgreSQL source tree, perhaps not in precisely the
same form. The fact that two functions both call readdir() and do
something with each file in the directory isn't enough to say that
they should be the same function, IMHO.
--
Robert Haas
EDB: http://www.enterprisedb.com
