Hi, I think it's not good that do_pg_start_backup() takes a flag which tells it to call back into basebackup.c's sendTablespace(). This means that details which ought to be private to basebackup.c leak out and become visible to other parts of the code. This seems to have originated in commit 72d422a5227ef6f76f412486a395aba9f53bf3f0, and it looks like there was some discussion of the issue at the time. I think that patch was right to want only a single iteration over the tablespace list; if not, the list of tablespaces returned by the backup could be different from the list that is included in the tablespace map, which does seem like a good thing to avoid.
However, it doesn't follow that sendTablespace() needs to be called from do_pg_start_backup(). It's not actually sending the tablespace at that point, just calculating the size, because the sizeonly argument is passed as true. And, there's no reason that I can see why that needs to be done from within do_pg_start_backup(). It can equally well be done after that function returns, as in the attached 0001. I believe that this is functionally equivalent but more elegant, although there is a notable behavior difference: today, sendTablespaces() is called in sizeonly mode with "fullpath" as the argument, which I think is pg_tblspc/$OID, and in non-sizeonly mode with ti->path as an argument, which seems to be the path to which the symlink points. With the patch, it would be called with the latter in both cases. It looks to me like that should be OK, and it definitely seems more consistent. While I was poking around in this area, I found some other code which I thought could stand a bit of improvement also. The attached 0002 slightly modifies some tablespace_map related code and comments in perform_base_backup(), so that instead of having two very similar calls to sendDir() right next to each other that differ only in the value passed for the fifth argument, we have just one call with the fifth argument being a variable. Although this is a minor change I think it's a good cleanup that reduces the chances of future mistakes in this area. Comments? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
0001-Don-t-export-basebackup.c-s-sendTablespace.patch
Description: Binary data
0002-Minor-code-cleanup-for-perform_base_backup.patch
Description: Binary data
