Thread: tablespace_map code cleanup

tablespace_map code cleanup

From
Robert Haas
Date:
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

Attachment

Re: tablespace_map code cleanup

From
Amit Kapila
Date:
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



Re: tablespace_map code cleanup

From
Robert Haas
Date:
On Mon, May 4, 2020 at 5:24 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> 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.

Oh, good point. v2 attached.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachment

Re: tablespace_map code cleanup

From
Robert Haas
Date:
On Wed, May 6, 2020 at 11:15 AM Robert Haas <robertmhaas@gmail.com> wrote:
> Oh, good point. v2 attached.

Here's v3, with one more small cleanup. I noticed tblspc_map_file is
initialized to NULL and then unconditionally reset to the return value
of makeStringInfo(), and then later tested to see whether it is NULL.
It can't be, because makeStringInfo() doesn't return NULL. So the
attached version deletes the superfluous initialization and the
superfluous test.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachment

Re: tablespace_map code cleanup

From
Amit Kapila
Date:
On Thu, May 7, 2020 at 9:44 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, May 6, 2020 at 11:15 AM Robert Haas <robertmhaas@gmail.com> wrote:
> > Oh, good point. v2 attached.
>

While looking at this, I noticed that caller (perform_base_backup) of
do_pg_start_backup, sets the backup phase as
PROGRESS_BASEBACKUP_PHASE_WAIT_CHECKPOINT whereas, in
do_pg_start_backup, we do collect the information about all
tablespaces after the checkpoint.  I am not sure if it is long enough
that we consider having a separate phase for it.   Without your patch,
it was covered under PROGRESS_BASEBACKUP_PHASE_ESTIMATE_BACKUP_SIZE
phase which doesn't appear to be a bad idea.

> Here's v3, with one more small cleanup. I noticed tblspc_map_file is
> initialized to NULL and then unconditionally reset to the return value
> of makeStringInfo(), and then later tested to see whether it is NULL.
> It can't be, because makeStringInfo() doesn't return NULL. So the
> attached version deletes the superfluous initialization and the
> superfluous test.
>

This change looks fine to me.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com



Re: tablespace_map code cleanup

From
Robert Haas
Date:
On Tue, May 12, 2020 at 2:23 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> While looking at this, I noticed that caller (perform_base_backup) of
> do_pg_start_backup, sets the backup phase as
> PROGRESS_BASEBACKUP_PHASE_WAIT_CHECKPOINT whereas, in
> do_pg_start_backup, we do collect the information about all
> tablespaces after the checkpoint.  I am not sure if it is long enough
> that we consider having a separate phase for it.   Without your patch,
> it was covered under PROGRESS_BASEBACKUP_PHASE_ESTIMATE_BACKUP_SIZE
> phase which doesn't appear to be a bad idea.

Maybe I'm confused here, but I think the size estimation still *is*
covered under PROGRESS_BASEBACKUP_PHASE_ESTIMATE_BACKUP_SIZE. It's
just that now that happens a bit later. I'm assuming that listing the
tablespaces is pretty cheap, but sizing them is expensive, as you'd
have to iterate over all the files and stat() each one.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: tablespace_map code cleanup

From
Amit Kapila
Date:
On Wed, May 13, 2020 at 1:54 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, May 12, 2020 at 2:23 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > While looking at this, I noticed that caller (perform_base_backup) of
> > do_pg_start_backup, sets the backup phase as
> > PROGRESS_BASEBACKUP_PHASE_WAIT_CHECKPOINT whereas, in
> > do_pg_start_backup, we do collect the information about all
> > tablespaces after the checkpoint.  I am not sure if it is long enough
> > that we consider having a separate phase for it.   Without your patch,
> > it was covered under PROGRESS_BASEBACKUP_PHASE_ESTIMATE_BACKUP_SIZE
> > phase which doesn't appear to be a bad idea.
>
> Maybe I'm confused here, but I think the size estimation still *is*
> covered under PROGRESS_BASEBACKUP_PHASE_ESTIMATE_BACKUP_SIZE. It's
> just that now that happens a bit later.
>

There is no problem with this part.

> I'm assuming that listing the
> tablespaces is pretty cheap, but sizing them is expensive, as you'd
> have to iterate over all the files and stat() each one.
>

I was trying to say that tablespace listing will happen under
PROGRESS_BASEBACKUP_PHASE_WAIT_CHECKPOINT phase which could be a
problem if it is a costly operation but as you said it is pretty cheap
so I think we don't need to bother about that.

Apart from the above point which I think we don't need to bother, both
your patches look good to me.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com



Re: tablespace_map code cleanup

From
Robert Haas
Date:
On Tue, May 12, 2020 at 10:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> I was trying to say that tablespace listing will happen under
> PROGRESS_BASEBACKUP_PHASE_WAIT_CHECKPOINT phase which could be a
> problem if it is a costly operation but as you said it is pretty cheap
> so I think we don't need to bother about that.
>
> Apart from the above point which I think we don't need to bother, both
> your patches look good to me.

OK, good. Let's see if anyone else feels differently about this issue
or wants to raise anything else. If not, I'll plan to commit these
patches after we branch. Thanks for the review.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: tablespace_map code cleanup

From
Kyotaro Horiguchi
Date:
At Wed, 13 May 2020 15:10:30 -0400, Robert Haas <robertmhaas@gmail.com> wrote in 
> On Tue, May 12, 2020 at 10:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > I was trying to say that tablespace listing will happen under
> > PROGRESS_BASEBACKUP_PHASE_WAIT_CHECKPOINT phase which could be a
> > problem if it is a costly operation but as you said it is pretty cheap
> > so I think we don't need to bother about that.
> >
> > Apart from the above point which I think we don't need to bother, both
> > your patches look good to me.
> 
> OK, good. Let's see if anyone else feels differently about this issue
> or wants to raise anything else. If not, I'll plan to commit these
> patches after we branch. Thanks for the review.

Table space listing needs only one or few 512k pages, which should be
on OS file cache, which cannot take long time unless the system is
facing a severe trouble. (I believe that is the same on Windows.)

I'm fine that WAIT_CHECKPOINT contains the time to enumerate
tablespace directories.


0001 looks good to me.  The progress information gets

About 0002,

+                bool    sendtblspclinks = true;

The boolean seems to me useless since it is always the inverse of
opt->sendtblspcmapfile when it is used.

Everything looks fine to me except the above.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: tablespace_map code cleanup

From
Robert Haas
Date:
On Wed, May 13, 2020 at 10:43 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> About 0002,
>
> +                               bool    sendtblspclinks = true;
>
> The boolean seems to me useless since it is always the inverse of
> opt->sendtblspcmapfile when it is used.

Well, I think it might have some documentation value, to clarify that
whether or not we send tablespace links is the opposite of whether we
send the tablespace map file.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: tablespace_map code cleanup

From
Kyotaro Horiguchi
Date:
At Wed, 27 May 2020 07:57:38 -0400, Robert Haas <robertmhaas@gmail.com> wrote in 
> On Wed, May 13, 2020 at 10:43 PM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> > About 0002,
> >
> > +                               bool    sendtblspclinks = true;
> >
> > The boolean seems to me useless since it is always the inverse of
> > opt->sendtblspcmapfile when it is used.
> 
> Well, I think it might have some documentation value, to clarify that
> whether or not we send tablespace links is the opposite of whether we
> send the tablespace map file.

That makes sense to me. Thanks!

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: tablespace_map code cleanup

From
Robert Haas
Date:
On Wed, May 27, 2020 at 8:11 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> That makes sense to me. Thanks!

Great. Thanks to you and Amit for reviewing. I have committed the patches.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company