Re: WIP/PoC for parallel backup

On Thu, Oct 17, 2019 at 1:33 AM Jeevan Ladhe <jeevan(dot)ladhe(at)enterprisedb(dot)com>
wrote:

> I quickly tried to have a look at your 0001-refactor patch.
> Here are some comments:
>
> 1. The patch fails to compile.
>
> Sorry if I am missing something, but am not able to understand why in new
> function collectTablespaces() you have added an extra parameter NULL while
> calling sendTablespace(), it fails the compilation :
>
> + ti->size = infotbssize ? sendTablespace(fullpath, true, NULL) : -1;
>
>
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Werror=vla -Wendif-labels
> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv
> -Wno-unused-command-line-argument -g -g -O0 -Wall -Werror
> -I../../../../src/include -c -o xlog.o xlog.c -MMD -MP -MF .deps/xlog.Po
> xlog.c:12253:59: error: too many arguments to function call, expected 2,
> have 3
> ti->size = infotbssize ? sendTablespace(fullpath, true,
> NULL) : -1;
> ~~~~~~~~~~~~~~
> ^~~~
>
> 2. I think the patch needs to run via pg_indent. It does not follow 80
> column
> width.
> e.g.
>
> +void
> +collectTablespaces(List **tablespaces, StringInfo tblspcmapfile, bool
> infotbssize, bool needtblspcmapfile)
> +{
>
> 3.
> The comments in re-factored code appear to be redundant. example:
> Following comment:
> /* Setup and activate network throttling, if client requested it */
> appears thrice in the code, before calling setup_throttle(), in the
> prologue of
> the function setup_throttle(), and above the if() in that function.
> Similarly - the comment:
> /* Collect information about all tablespaces */
> in collectTablespaces().
>
> 4.
> In function include_wal_files() why is the parameter TimeLineID i.e. endtli
> needed. I don't see it being used in the function at all. I think you can
> safely
> get rid of it.
>
> +include_wal_files(XLogRecPtr endptr, TimeLineID endtli)
>
>
Thanks Jeevan. Some changes that should be part of 2nd patch were left in
the 1st. I have fixed that and the above mentioned issues as well.
Attached are the updated patches.

Thanks,

--
Asif Rehman
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca