On Wed, Apr 29, 2020 at 9:57 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> 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.
>
If we want to move the calculation of size for tablespaces in the
caller then I think we also need to do something about the progress
reporting related to phase
PROGRESS_BASEBACKUP_PHASE_ESTIMATE_BACKUP_SIZE.
> 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.
>
The 0002 patch looks good to me.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com