On Fri, Aug 16, 2019 at 3:24 PM Jeevan Chalke < jeevan.cha...@enterprisedb.com> wrote:
> > > On Fri, Aug 2, 2019 at 6:43 PM vignesh C <vignes...@gmail.com> wrote: > >> Some comments: >> 1) There will be some link files created for tablespace, we might >> require some special handling for it >> > > Yep. I have that in my ToDo. > Will start working on that soon. > > >> 2) >> Retry functionality is hanlded only for copying of full files, should >> we handle retry for copying of partial files >> 3) >> we can have some range for maxretries similar to sleeptime >> > > I took help from pg_standby code related to maxentries and sleeptime. > > However, as we don't want to use system() call now, I have > removed all this kludge and just used fread/fwrite as discussed. > > >> 4) >> Should we check for malloc failure >> > > Used pg_malloc() instead. Same is also suggested by Ibrar. > > >> >> 5) Should we add display of progress as backup may take some time, >> this can be added as enhancement. We can get other's opinion on this. >> > > Can be done afterward once we have the functionality in place. > > >> >> 6) >> If the backup count increases providing the input may be difficult, >> Shall user provide all the incremental backups from a parent folder >> and can we handle the ordering of incremental backup internally >> > > I am not sure of this yet. We need to provide the tablespace mapping too. > But thanks for putting a point here. Will keep that in mind when I revisit > this. > > >> >> 7) >> Add verbose for copying whole file >> > Done > > >> >> 8) We can also check if approximate space is available in disk before >> starting combine backup, this can be added as enhancement. We can get >> other's opinion on this. >> > > Hmm... will leave it for now. User will get an error anyway. > > >> >> 9) >> Combine backup into directory can be combine backup directory >> > Done > > >> >> 10) >> MAX_INCR_BK_COUNT can be increased little, some applications use 1 >> full backup at the beginning of the month and use 30 incremental >> backups rest of the days in the month >> > > Yeah, agree. But using any number here is debatable. > Let's see others opinion too. > Why not use a list? > > >> Regards, >> Vignesh >> EnterpriseDB: http://www.enterprisedb.com >> > > > Attached new sets of patches with refactoring done separately. > Incremental backup patch became small now and hopefully more > readable than the first version. > > -- > Jeevan Chalke > Technical Architect, Product Development > EnterpriseDB Corporation > The Enterprise PostgreSQL Company > > + buf = (char *) malloc(statbuf->st_size); + if (buf == NULL) + ereport(ERROR, + (errcode(ERRCODE_OUT_OF_MEMORY), + errmsg("out of memory"))); Why are you using malloc, you can use palloc here. -- Ibrar Ahmed
