Обсуждение: tablespace_map code cleanup
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
Вложения
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
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
Вложения
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
Вложения
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
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
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
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
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
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
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
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