WIP/PoC for parallel backup

Hi Hackers,

I have been looking into adding parallel backup feature in pg_basebackup.
Currently pg_basebackup sends BASE_BACKUP command for taking full backup,
server scans the PGDATA and sends the files to pg_basebackup. In general,
server takes the following steps on BASE_BACKUP command:

- do pg_start_backup
- scans PGDATA, creates and send header containing information of
tablespaces.
- sends each tablespace to pg_basebackup.
- and then do pg_stop_backup

All these steps are executed sequentially by a single process. The idea I
am working on is to separate these steps into multiple commands in
replication grammer. Add worker processes to the pg_basebackup where they
can copy the contents of PGDATA in parallel.

The command line interface syntax would be like:
pg_basebackup --jobs=WORKERS

Replication commands:

- BASE_BACKUP [PARALLEL] - returns a list of files in PGDATA
If the parallel option is there, then it will only do pg_start_backup,
scans PGDATA and sends a list of file names.

- SEND_FILES_CONTENTS (file1, file2,...) - returns the files in given list.
pg_basebackup will then send back a list of filenames in this command. This
commands will be send by each worker and that worker will be getting the
said files.

- STOP_BACKUP
when all workers finish then, pg_basebackup will send STOP_BACKUP command.

The pg_basebackup can start by sending "BASE_BACKUP PARALLEL" command and
getting a list of filenames from the server in response. It should then
divide this list as per --jobs parameter. (This division can be based on
file sizes). Each of the worker process will issue a SEND_FILES_CONTENTS
(file1, file2,...) command. In response, the server will send the files
mentioned in the list back to the requesting worker process.

Once all the files are copied, then pg_basebackup will send the STOP_BACKUP
command. Similar idea has been been discussed by Robert, on the incremental
backup thread a while ago. This is similar to that but instead of
START_BACKUP and SEND_FILE_LIST, I have combined them into BASE_BACKUP
PARALLEL.

I have done a basic proof of concenpt (POC), which is also attached. I
would appreciate some input on this. So far, I am simply dividing the list
equally and assigning them to worker processes. I intend to fine tune this
by taking into consideration file sizes. Further to add tar format support,
I am considering that each worker process, processes all files belonging to
a tablespace in its list (i.e. creates and copies tar file), before it
processes the next tablespace. As a result, this will create tar files that
are disjointed with respect tablespace data. For example:

Say, tablespace t1 has 20 files and we have 5 worker processes and
tablespace t2 has 10. Ignoring all other factors for the sake of this
example, each worker process will get a group of 4 files of t1 and 2 files
of t2. Each process will create 2 tar files, one for t1 containing 4 files
and another for t2 containing 2 files.

Regards,
Asif

Hi Asif

Interesting proposal. Bulk of the work in a backup is transferring files
from source data directory to destination. Your patch is breaking this
task down in multiple sets of files and transferring each set in parallel.
This seems correct, however, your patch is also creating a new process to
handle each set. Is that necessary? I think we should try to achieve this
using multiple asynchronous libpq connections from a single basebackup
process. That is to use PQconnectStartParams() interface instead of
PQconnectdbParams(), wich is currently used by basebackup. On the server
side, it may still result in multiple backend processes per connection, and
an attempt should be made to avoid that as well, but it seems complicated.

What do you think?

Asim

On Fri, Aug 23, 2019 at 3:18 PM Asim R P <apraveen(at)pivotal(dot)io> wrote:

> Hi Asif
>
> Interesting proposal. Bulk of the work in a backup is transferring files
> from source data directory to destination. Your patch is breaking this
> task down in multiple sets of files and transferring each set in parallel.
> This seems correct, however, your patch is also creating a new process to
> handle each set. Is that necessary? I think we should try to achieve this
> using multiple asynchronous libpq connections from a single basebackup
> process. That is to use PQconnectStartParams() interface instead of
> PQconnectdbParams(), wich is currently used by basebackup. On the server
> side, it may still result in multiple backend processes per connection, and
> an attempt should be made to avoid that as well, but it seems complicated.
>
> What do you think?
>
> The main question is what we really want to solve here. What is the
bottleneck? and which HW want to saturate?. Why I am saying that because
there are multiple H/W involve while taking the backup (Network/CPU/Disk).
If we
already saturated the disk then there is no need to add parallelism because
we will be blocked on disk I/O anyway. I implemented the parallel backup
in a sperate
application and has wonderful results. I just skim through the code and have
some reservation that creating a separate process only for copying data is
overkill.
There are two options, one is non-blocking calls or you can have some
worker threads.
But before doing that need to see the pg_basebackup bottleneck, after that,
we
can see what is the best way to solve that. Some numbers may help to
understand the
actual benefit.

--
Ibrar Ahmed

On Fri, Aug 23, 2019 at 3:18 PM Asim R P <apraveen(at)pivotal(dot)io> wrote:

> Hi Asif
>
> Interesting proposal. Bulk of the work in a backup is transferring files
> from source data directory to destination. Your patch is breaking this
> task down in multiple sets of files and transferring each set in parallel.
> This seems correct, however, your patch is also creating a new process to
> handle each set. Is that necessary? I think we should try to achieve this
> using multiple asynchronous libpq connections from a single basebackup
> process. That is to use PQconnectStartParams() interface instead of
> PQconnectdbParams(), wich is currently used by basebackup. On the server
> side, it may still result in multiple backend processes per connection, and
> an attempt should be made to avoid that as well, but it seems complicated.
>
> What do you think?
>
> Asim
>

Thanks Asim for the feedback. This is a good suggestion. The main idea I
wanted to discuss is the design where we can open multiple backend
connections to get the data instead of a single connection.
On the client side we can have multiple approaches, One is to use
asynchronous APIs ( as suggested by you) and other could be to decide
between multi-process and multi-thread. The main point was we can extract
lot of performance benefit by using the multiple connections and I built
this POC to float the idea of how the parallel backup can work, since the
core logic of getting the files using multiple connections will remain the
same, wether we use asynchronous, multi-process or multi-threaded.

I am going to address the division of files to be distributed evenly among
multiple workers based on file sizes, that would allow to get some concrete
numbers as well as it will also us to gauge some benefits between async and
multiprocess/thread approach on client side.

Regards,
Asif

Greetings,

* Asif Rehman (asifr(dot)rehman(at)gmail(dot)com) wrote:
> On Fri, Aug 23, 2019 at 3:18 PM Asim R P <apraveen(at)pivotal(dot)io> wrote:
> > Interesting proposal. Bulk of the work in a backup is transferring files
> > from source data directory to destination. Your patch is breaking this
> > task down in multiple sets of files and transferring each set in parallel.
> > This seems correct, however, your patch is also creating a new process to
> > handle each set. Is that necessary? I think we should try to achieve this
> > using multiple asynchronous libpq connections from a single basebackup
> > process. That is to use PQconnectStartParams() interface instead of
> > PQconnectdbParams(), wich is currently used by basebackup. On the server
> > side, it may still result in multiple backend processes per connection, and
> > an attempt should be made to avoid that as well, but it seems complicated.
>
> Thanks Asim for the feedback. This is a good suggestion. The main idea I
> wanted to discuss is the design where we can open multiple backend
> connections to get the data instead of a single connection.
> On the client side we can have multiple approaches, One is to use
> asynchronous APIs ( as suggested by you) and other could be to decide
> between multi-process and multi-thread. The main point was we can extract
> lot of performance benefit by using the multiple connections and I built
> this POC to float the idea of how the parallel backup can work, since the
> core logic of getting the files using multiple connections will remain the
> same, wether we use asynchronous, multi-process or multi-threaded.
>
> I am going to address the division of files to be distributed evenly among
> multiple workers based on file sizes, that would allow to get some concrete
> numbers as well as it will also us to gauge some benefits between async and
> multiprocess/thread approach on client side.

I would expect you to quickly want to support compression on the server
side, before the data is sent across the network, and possibly
encryption, and so it'd likely make sense to just have independent
processes and connections through which to do that.

Thanks,

Stephen

On Fri, Aug 23, 2019 at 10:26 PM Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> Greetings,
>
> * Asif Rehman (asifr(dot)rehman(at)gmail(dot)com) wrote:
> > On Fri, Aug 23, 2019 at 3:18 PM Asim R P <apraveen(at)pivotal(dot)io> wrote:
> > > Interesting proposal. Bulk of the work in a backup is transferring
> files
> > > from source data directory to destination. Your patch is breaking this
> > > task down in multiple sets of files and transferring each set in
> parallel.
> > > This seems correct, however, your patch is also creating a new process
> to
> > > handle each set. Is that necessary? I think we should try to achieve
> this
> > > using multiple asynchronous libpq connections from a single basebackup
> > > process. That is to use PQconnectStartParams() interface instead of
> > > PQconnectdbParams(), wich is currently used by basebackup. On the
> server
> > > side, it may still result in multiple backend processes per
> connection, and
> > > an attempt should be made to avoid that as well, but it seems
> complicated.
> >
> > Thanks Asim for the feedback. This is a good suggestion. The main idea I
> > wanted to discuss is the design where we can open multiple backend
> > connections to get the data instead of a single connection.
> > On the client side we can have multiple approaches, One is to use
> > asynchronous APIs ( as suggested by you) and other could be to decide
> > between multi-process and multi-thread. The main point was we can extract
> > lot of performance benefit by using the multiple connections and I built
> > this POC to float the idea of how the parallel backup can work, since the
> > core logic of getting the files using multiple connections will remain
> the
> > same, wether we use asynchronous, multi-process or multi-threaded.
> >
> > I am going to address the division of files to be distributed evenly
> among
> > multiple workers based on file sizes, that would allow to get some
> concrete
> > numbers as well as it will also us to gauge some benefits between async
> and
> > multiprocess/thread approach on client side.
>
> I would expect you to quickly want to support compression on the server
> side, before the data is sent across the network, and possibly
> encryption, and so it'd likely make sense to just have independent
> processes and connections through which to do that.
>
> +1 for compression and encryption, but I think parallelism will give us
the benefit with and without the compression.

Thanks,
>
> Stephen
>

--
Ibrar Ahmed

On Fri, 23 Aug 2019 at 10:26 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> Greetings,
>
> * Asif Rehman (asifr(dot)rehman(at)gmail(dot)com) wrote:
> > On Fri, Aug 23, 2019 at 3:18 PM Asim R P <apraveen(at)pivotal(dot)io> wrote:
> > > Interesting proposal. Bulk of the work in a backup is transferring
> files
> > > from source data directory to destination. Your patch is breaking this
> > > task down in multiple sets of files and transferring each set in
> parallel.
> > > This seems correct, however, your patch is also creating a new process
> to
> > > handle each set. Is that necessary? I think we should try to achieve
> this
> > > using multiple asynchronous libpq connections from a single basebackup
> > > process. That is to use PQconnectStartParams() interface instead of
> > > PQconnectdbParams(), wich is currently used by basebackup. On the
> server
> > > side, it may still result in multiple backend processes per
> connection, and
> > > an attempt should be made to avoid that as well, but it seems
> complicated.
> >
> > Thanks Asim for the feedback. This is a good suggestion. The main idea I
> > wanted to discuss is the design where we can open multiple backend
> > connections to get the data instead of a single connection.
> > On the client side we can have multiple approaches, One is to use
> > asynchronous APIs ( as suggested by you) and other could be to decide
> > between multi-process and multi-thread. The main point was we can extract
> > lot of performance benefit by using the multiple connections and I built
> > this POC to float the idea of how the parallel backup can work, since the
> > core logic of getting the files using multiple connections will remain
> the
> > same, wether we use asynchronous, multi-process or multi-threaded.
> >
> > I am going to address the division of files to be distributed evenly
> among
> > multiple workers based on file sizes, that would allow to get some
> concrete
> > numbers as well as it will also us to gauge some benefits between async
> and
> > multiprocess/thread approach on client side.
>
> I would expect you to quickly want to support compression on the server
> side, before the data is sent across the network, and possibly
> encryption, and so it'd likely make sense to just have independent
> processes and connections through which to do that.

It would be interesting to see the benefits of compression (before the data
is transferred over the network) on top of parallelism. Since there is also
some overhead associated with performing the compression. I agree with your
suggestion of trying to add parallelism first and then try compression
before the data is sent across the network.

>
> Thanks,
>
> Stephen
>

Greetings,

* Ahsan Hadi (ahsan(dot)hadi(at)gmail(dot)com) wrote:
> On Fri, 23 Aug 2019 at 10:26 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > I would expect you to quickly want to support compression on the server
> > side, before the data is sent across the network, and possibly
> > encryption, and so it'd likely make sense to just have independent
> > processes and connections through which to do that.
>
> It would be interesting to see the benefits of compression (before the data
> is transferred over the network) on top of parallelism. Since there is also
> some overhead associated with performing the compression. I agree with your
> suggestion of trying to add parallelism first and then try compression
> before the data is sent across the network.

You're welcome to take a look at pgbackrest for insight and to play with
regarding compression-before-transfer, how best to split up the files
and order them, encryption, et al. We've put quite a bit of effort into
figuring all of that out.

Thanks!

Stephen

On Wed, Aug 21, 2019 at 9:53 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:
> - BASE_BACKUP [PARALLEL] - returns a list of files in PGDATA
> If the parallel option is there, then it will only do pg_start_backup, scans PGDATA and sends a list of file names.

So IIUC, this would mean that BASE_BACKUP without PARALLEL returns
tarfiles, and BASE_BACKUP with PARALLEL returns a result set with a
list of file names. I don't think that's a good approach. It's too
confusing to have one replication command that returns totally
different things depending on whether some option is given.

> - SEND_FILES_CONTENTS (file1, file2,...) - returns the files in given list.
> pg_basebackup will then send back a list of filenames in this command. This commands will be send by each worker and that worker will be getting the said files.

Seems reasonable, but I think you should just pass one file name and
use the command multiple times, once per file.

> - STOP_BACKUP
> when all workers finish then, pg_basebackup will send STOP_BACKUP command.

This also seems reasonable, but surely the matching command should
then be called START_BACKUP, not BASEBACKUP PARALLEL.

> I have done a basic proof of concenpt (POC), which is also attached. I would appreciate some input on this. So far, I am simply dividing the list equally and assigning them to worker processes. I intend to fine tune this by taking into consideration file sizes. Further to add tar format support, I am considering that each worker process, processes all files belonging to a tablespace in its list (i.e. creates and copies tar file), before it processes the next tablespace. As a result, this will create tar files that are disjointed with respect tablespace data. For example:

Instead of doing this, I suggest that you should just maintain a list
of all the files that need to be fetched and have each worker pull a
file from the head of the list and fetch it when it finishes receiving
the previous file. That way, if some connections go faster or slower
than others, the distribution of work ends up fairly even. If you
instead pre-distribute the work, you're guessing what's going to
happen in the future instead of just waiting to see what actually does
happen. Guessing isn't intrinsically bad, but guessing when you could
be sure of doing the right thing *is* bad.

If you want to be really fancy, you could start by sorting the files
in descending order of size, so that big files are fetched before
small ones. Since the largest possible file is 1GB and any database
where this feature is important is probably hundreds or thousands of
GB, this may not be very important. I suggest not worrying about it
for v1.

> Say, tablespace t1 has 20 files and we have 5 worker processes and tablespace t2 has 10. Ignoring all other factors for the sake of this example, each worker process will get a group of 4 files of t1 and 2 files of t2. Each process will create 2 tar files, one for t1 containing 4 files and another for t2 containing 2 files.

This is one of several possible approaches. If we're doing a
plain-format backup in parallel, we can just write each file where it
needs to go and call it good. But, with a tar-format backup, what
should we do? I can see three options:

1. Error! Tar format parallel backups are not supported.

2. Write multiple tar files. The user might reasonably expect that
they're going to end up with the same files at the end of the backup
regardless of whether they do it in parallel. A user with this
expectation will be disappointed.

3. Write one tar file. In this design, the workers have to take turns
writing to the tar file, so you need some synchronization around that.
Perhaps you'd have N threads that read and buffer a file, and N+1
buffers. Then you have one additional thread that reads the complete
files from the buffers and writes them to the tar file. There's
obviously some possibility that the writer won't be able to keep up
and writing the backup will therefore be slower than it would be with
approach (2).

There's probably also a possibility that approach (2) would thrash the
disk head back and forth between multiple files that are all being
written at the same time, and approach (3) will therefore win by not
thrashing the disk head. But, since spinning media are becoming less
and less popular and are likely to have multiple disk heads under the
hood when they are used, this is probably not too likely.

I think your choice to go with approach (2) is probably reasonable,
but I'm not sure whether everyone will agree.

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

Hi Robert,

Thanks for the feedback. Please see the comments below:

On Tue, Sep 24, 2019 at 10:53 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Wed, Aug 21, 2019 at 9:53 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
> wrote:
> > - BASE_BACKUP [PARALLEL] - returns a list of files in PGDATA
> > If the parallel option is there, then it will only do pg_start_backup,
> scans PGDATA and sends a list of file names.
>
> So IIUC, this would mean that BASE_BACKUP without PARALLEL returns
> tarfiles, and BASE_BACKUP with PARALLEL returns a result set with a
> list of file names. I don't think that's a good approach. It's too
> confusing to have one replication command that returns totally
> different things depending on whether some option is given.
>

Sure. I will add a separate command (START_BACKUP) for parallel.

> > - SEND_FILES_CONTENTS (file1, file2,...) - returns the files in given
> list.
> > pg_basebackup will then send back a list of filenames in this command.
> This commands will be send by each worker and that worker will be getting
> the said files.
>
> Seems reasonable, but I think you should just pass one file name and
> use the command multiple times, once per file.
>

I considered this approach initially, however, I adopted the current
strategy to avoid multiple round trips between the server and clients and
save on query processing time by issuing a single command rather than
multiple ones. Further fetching multiple files at once will also aid in
supporting the tar format by utilising the existing ReceiveTarFile()
function and will be able to create a tarball for per tablespace per worker.

>
> > - STOP_BACKUP
> > when all workers finish then, pg_basebackup will send STOP_BACKUP
> command.
>
> This also seems reasonable, but surely the matching command should
> then be called START_BACKUP, not BASEBACKUP PARALLEL.
>
> > I have done a basic proof of concenpt (POC), which is also attached. I
> would appreciate some input on this. So far, I am simply dividing the list
> equally and assigning them to worker processes. I intend to fine tune this
> by taking into consideration file sizes. Further to add tar format support,
> I am considering that each worker process, processes all files belonging to
> a tablespace in its list (i.e. creates and copies tar file), before it
> processes the next tablespace. As a result, this will create tar files that
> are disjointed with respect tablespace data. For example:
>
> Instead of doing this, I suggest that you should just maintain a list
> of all the files that need to be fetched and have each worker pull a
> file from the head of the list and fetch it when it finishes receiving
> the previous file. That way, if some connections go faster or slower
> than others, the distribution of work ends up fairly even. If you
> instead pre-distribute the work, you're guessing what's going to
> happen in the future instead of just waiting to see what actually does
> happen. Guessing isn't intrinsically bad, but guessing when you could
> be sure of doing the right thing *is* bad.
>
> If you want to be really fancy, you could start by sorting the files
> in descending order of size, so that big files are fetched before
> small ones. Since the largest possible file is 1GB and any database
> where this feature is important is probably hundreds or thousands of
> GB, this may not be very important. I suggest not worrying about it
> for v1.
>

Ideally, I would like to support the tar format as well, which would be
much easier to implement when fetching multiple files at once since that
would enable using the existent functionality to be used without much
change.

Your idea of sorting the files in descending order of size seems very
appealing. I think we can do this and have the file divided among the
workers one by one i.e. the first file in the list goes to worker 1, the
second to process 2, and so on and so forth.

>
> > Say, tablespace t1 has 20 files and we have 5 worker processes and
> tablespace t2 has 10. Ignoring all other factors for the sake of this
> example, each worker process will get a group of 4 files of t1 and 2 files
> of t2. Each process will create 2 tar files, one for t1 containing 4 files
> and another for t2 containing 2 files.
>
> This is one of several possible approaches. If we're doing a
> plain-format backup in parallel, we can just write each file where it
> needs to go and call it good. But, with a tar-format backup, what
> should we do? I can see three options:
>
> 1. Error! Tar format parallel backups are not supported.
>
> 2. Write multiple tar files. The user might reasonably expect that
> they're going to end up with the same files at the end of the backup
> regardless of whether they do it in parallel. A user with this
> expectation will be disappointed.
>
> 3. Write one tar file. In this design, the workers have to take turns
> writing to the tar file, so you need some synchronization around that.
> Perhaps you'd have N threads that read and buffer a file, and N+1
> buffers. Then you have one additional thread that reads the complete
> files from the buffers and writes them to the tar file. There's
> obviously some possibility that the writer won't be able to keep up
> and writing the backup will therefore be slower than it would be with
> approach (2).
>
> There's probably also a possibility that approach (2) would thrash the
> disk head back and forth between multiple files that are all being
> written at the same time, and approach (3) will therefore win by not
> thrashing the disk head. But, since spinning media are becoming less
> and less popular and are likely to have multiple disk heads under the
> hood when they are used, this is probably not too likely.
>
> I think your choice to go with approach (2) is probably reasonable,
> but I'm not sure whether everyone will agree.
>

Yes for the tar format support, approach (2) is what I had in
mind. Currently I'm working on the implementation and will share the patch
in a couple of days.

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

Hi Asif,

I was looking at the patch and tried comipling it. However, got few errors
and warnings.

Fixed those in the attached patch.

On Fri, Sep 27, 2019 at 9:30 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

> Hi Robert,
>
> Thanks for the feedback. Please see the comments below:
>
> On Tue, Sep 24, 2019 at 10:53 PM Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
>
>> On Wed, Aug 21, 2019 at 9:53 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
>> wrote:
>> > - BASE_BACKUP [PARALLEL] - returns a list of files in PGDATA
>> > If the parallel option is there, then it will only do pg_start_backup,
>> scans PGDATA and sends a list of file names.
>>
>> So IIUC, this would mean that BASE_BACKUP without PARALLEL returns
>> tarfiles, and BASE_BACKUP with PARALLEL returns a result set with a
>> list of file names. I don't think that's a good approach. It's too
>> confusing to have one replication command that returns totally
>> different things depending on whether some option is given.
>>
>
> Sure. I will add a separate command (START_BACKUP) for parallel.
>
>
>> > - SEND_FILES_CONTENTS (file1, file2,...) - returns the files in given
>> list.
>> > pg_basebackup will then send back a list of filenames in this command.
>> This commands will be send by each worker and that worker will be getting
>> the said files.
>>
>> Seems reasonable, but I think you should just pass one file name and
>> use the command multiple times, once per file.
>>
>
> I considered this approach initially, however, I adopted the current
> strategy to avoid multiple round trips between the server and clients and
> save on query processing time by issuing a single command rather than
> multiple ones. Further fetching multiple files at once will also aid in
> supporting the tar format by utilising the existing ReceiveTarFile()
> function and will be able to create a tarball for per tablespace per worker.
>
>
>>
>> > - STOP_BACKUP
>> > when all workers finish then, pg_basebackup will send STOP_BACKUP
>> command.
>>
>> This also seems reasonable, but surely the matching command should
>> then be called START_BACKUP, not BASEBACKUP PARALLEL.
>>
>> > I have done a basic proof of concenpt (POC), which is also attached. I
>> would appreciate some input on this. So far, I am simply dividing the list
>> equally and assigning them to worker processes. I intend to fine tune this
>> by taking into consideration file sizes. Further to add tar format support,
>> I am considering that each worker process, processes all files belonging to
>> a tablespace in its list (i.e. creates and copies tar file), before it
>> processes the next tablespace. As a result, this will create tar files that
>> are disjointed with respect tablespace data. For example:
>>
>> Instead of doing this, I suggest that you should just maintain a list
>> of all the files that need to be fetched and have each worker pull a
>> file from the head of the list and fetch it when it finishes receiving
>> the previous file. That way, if some connections go faster or slower
>> than others, the distribution of work ends up fairly even. If you
>> instead pre-distribute the work, you're guessing what's going to
>> happen in the future instead of just waiting to see what actually does
>> happen. Guessing isn't intrinsically bad, but guessing when you could
>> be sure of doing the right thing *is* bad.
>>
>> If you want to be really fancy, you could start by sorting the files
>> in descending order of size, so that big files are fetched before
>> small ones. Since the largest possible file is 1GB and any database
>> where this feature is important is probably hundreds or thousands of
>> GB, this may not be very important. I suggest not worrying about it
>> for v1.
>>
>
> Ideally, I would like to support the tar format as well, which would be
> much easier to implement when fetching multiple files at once since that
> would enable using the existent functionality to be used without much
> change.
>
> Your idea of sorting the files in descending order of size seems very
> appealing. I think we can do this and have the file divided among the
> workers one by one i.e. the first file in the list goes to worker 1, the
> second to process 2, and so on and so forth.
>
>
>>
>> > Say, tablespace t1 has 20 files and we have 5 worker processes and
>> tablespace t2 has 10. Ignoring all other factors for the sake of this
>> example, each worker process will get a group of 4 files of t1 and 2 files
>> of t2. Each process will create 2 tar files, one for t1 containing 4 files
>> and another for t2 containing 2 files.
>>
>> This is one of several possible approaches. If we're doing a
>> plain-format backup in parallel, we can just write each file where it
>> needs to go and call it good. But, with a tar-format backup, what
>> should we do? I can see three options:
>>
>> 1. Error! Tar format parallel backups are not supported.
>>
>> 2. Write multiple tar files. The user might reasonably expect that
>> they're going to end up with the same files at the end of the backup
>> regardless of whether they do it in parallel. A user with this
>> expectation will be disappointed.
>>
>> 3. Write one tar file. In this design, the workers have to take turns
>> writing to the tar file, so you need some synchronization around that.
>> Perhaps you'd have N threads that read and buffer a file, and N+1
>> buffers. Then you have one additional thread that reads the complete
>> files from the buffers and writes them to the tar file. There's
>> obviously some possibility that the writer won't be able to keep up
>> and writing the backup will therefore be slower than it would be with
>> approach (2).
>>
>> There's probably also a possibility that approach (2) would thrash the
>> disk head back and forth between multiple files that are all being
>> written at the same time, and approach (3) will therefore win by not
>> thrashing the disk head. But, since spinning media are becoming less
>> and less popular and are likely to have multiple disk heads under the
>> hood when they are used, this is probably not too likely.
>>
>> I think your choice to go with approach (2) is probably reasonable,
>> but I'm not sure whether everyone will agree.
>>
>
> Yes for the tar format support, approach (2) is what I had in
> mind. Currently I'm working on the implementation and will share the patch
> in a couple of days.
>
>
> --
> Asif Rehman
> Highgo Software (Canada/China/Pakistan)
> URL : www.highgo.ca
>

--
Jeevan Chalke
Associate Database Architect & Team Lead, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

On Fri, Sep 27, 2019 at 12:00 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:
>> > - SEND_FILES_CONTENTS (file1, file2,...) - returns the files in given list.
>> > pg_basebackup will then send back a list of filenames in this command. This commands will be send by each worker and that worker will be getting the said files.
>>
>> Seems reasonable, but I think you should just pass one file name and
>> use the command multiple times, once per file.
>
> I considered this approach initially, however, I adopted the current strategy to avoid multiple round trips between the server and clients and save on query processing time by issuing a single command rather than multiple ones. Further fetching multiple files at once will also aid in supporting the tar format by utilising the existing ReceiveTarFile() function and will be able to create a tarball for per tablespace per worker.

I think that sending multiple filenames on a line could save some time
when there are lots of very small files, because then the round-trip
overhead could be significant.

However, if you've got mostly big files, I think this is going to be a
loser. It'll be fine if you're able to divide the work exactly evenly,
but that's pretty hard to do, because some workers may succeed in
copying the data faster than others for a variety of reasons: some
data is in memory, some data has to be read from disk, different data
may need to be read from different disks that run at different speeds,
not all the network connections may run at the same speed. Remember
that the backup's not done until the last worker finishes, and so
there may well be a significant advantage in terms of overall speed in
putting some energy into making sure that they finish as close to each
other in time as possible.

To put that another way, the first time all the workers except one get
done while the last one still has 10GB of data to copy, somebody's
going to be unhappy.

> Ideally, I would like to support the tar format as well, which would be much easier to implement when fetching multiple files at once since that would enable using the existent functionality to be used without much change.

I think we should just have the client generate the tarfile. It'll
require duplicating some code, but it's not actually that much code or
that complicated from what I can see.

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

On Thu, Oct 3, 2019 at 6:40 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Fri, Sep 27, 2019 at 12:00 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
> wrote:
> >> > - SEND_FILES_CONTENTS (file1, file2,...) - returns the files in given
> list.
> >> > pg_basebackup will then send back a list of filenames in this
> command. This commands will be send by each worker and that worker will be
> getting the said files.
> >>
> >> Seems reasonable, but I think you should just pass one file name and
> >> use the command multiple times, once per file.
> >
> > I considered this approach initially, however, I adopted the current
> strategy to avoid multiple round trips between the server and clients and
> save on query processing time by issuing a single command rather than
> multiple ones. Further fetching multiple files at once will also aid in
> supporting the tar format by utilising the existing ReceiveTarFile()
> function and will be able to create a tarball for per tablespace per worker.
>
> I think that sending multiple filenames on a line could save some time
> when there are lots of very small files, because then the round-trip
> overhead could be significant.
>
> However, if you've got mostly big files, I think this is going to be a
> loser. It'll be fine if you're able to divide the work exactly evenly,
> but that's pretty hard to do, because some workers may succeed in
> copying the data faster than others for a variety of reasons: some
> data is in memory, some data has to be read from disk, different data
> may need to be read from different disks that run at different speeds,
> not all the network connections may run at the same speed. Remember
> that the backup's not done until the last worker finishes, and so
> there may well be a significant advantage in terms of overall speed in
> putting some energy into making sure that they finish as close to each
> other in time as possible.
>
> To put that another way, the first time all the workers except one get
> done while the last one still has 10GB of data to copy, somebody's
> going to be unhappy.
>

I have updated the patch (see the attached patch) to include tablespace
support, tar format support and all other backup base backup options to
work in parallel mode as well. As previously suggested, I have removed
BASE_BACKUP [PARALLEL] and have added START_BACKUP instead to start the
backup. The tar format will write multiple tar files depending upon the
number of workers specified. Also made all commands
(START_BACKUP/SEND_FILES_CONTENT/STOP_BACKUP) to accept the
base_backup_opt_list. This way the command-line options can also be
provided to these commands. Since the command-line options don't change
once the backup initiates, I went this way instead of storing them in
shared state.

The START_BACKUP command will now return a sorted list of files in
descending order based on file sizes. This way, the larger files will be on
top of the list. hence these files will be assigned to workers one by one,
making it so that the larger files will be copied before other files.

Based on my understanding your main concern is that the files won't be
distributed fairly i.e one worker might get a big file and take more time
while others get done early with smaller files? In this approach I have
created a list of files in descending order based on there sizes so all the
big size files will come at the top. The maximum file size in PG is 1GB so
if we have four workers who are picking up file from the list one by one,
the worst case scenario is that one worker gets a file of 1GB to process
while others get files of smaller size. However with this approach of
descending files based on size and handing it out to workers one by one,
there is a very high likelihood of workers getting work evenly. does this
address your concerns?

Furthermore the patch also includes the regression test. As t/
010_pg_basebackup.pl test-case is testing base backup comprehensively, so I
have duplicated it to "t/040_pg_basebackup_parallel.pl" and added parallel
option in all of its tests, to make sure parallel mode works expectantly.
The one thing that differs from base backup is the file checksum reporting.
In parallel mode, the total number of checksum failures are not reported
correctly however it will abort the backup whenever a checksum failure
occurs. This is because processes are not maintaining any shared state. I
assume that it's not much important to report total number of failures vs
noticing the failure and aborting.

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

On Fri, Oct 4, 2019 at 7:02 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:
> Based on my understanding your main concern is that the files won't be distributed fairly i.e one worker might get a big file and take more time while others get done early with smaller files? In this approach I have created a list of files in descending order based on there sizes so all the big size files will come at the top. The maximum file size in PG is 1GB so if we have four workers who are picking up file from the list one by one, the worst case scenario is that one worker gets a file of 1GB to process while others get files of smaller size. However with this approach of descending files based on size and handing it out to workers one by one, there is a very high likelihood of workers getting work evenly. does this address your concerns?

Somewhat, but I'm not sure it's good enough. There are lots of reasons
why two processes that are started at the same time with the same
amount of work might not finish at the same time.

I'm also not particularly excited about having the server do the
sorting based on file size. Seems like that ought to be the client's
job, if the client needs the sorting.

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

Thanks Asif for the patch. I am opting this for a review. Patch is
bit big, so here are very initial comments to make the review process
easier.

1) Patch seems doing lot of code shuffling, I think it would be easy
to review if you can break the clean up patch separately.

Example:
a: setup_throttle
b: include_wal_files

2) As I can see this patch basically have three major phase.

a) Introducing new commands like START_BACKUP, SEND_FILES_CONTENT and
STOP_BACKUP.
b) Implementation of actual parallel backup.
c) Testcase

I would suggest, if you can break out in three as a separate patch that
would be nice. It will benefit in reviewing the patch.

3) In your patch you are preparing the backup manifest (file which
giving the information about the data files). Robert Haas, submitted
the backup manifests patch on another thread [1], and I think we
should use that patch to get the backup manifests for parallel backup.

Further, I will continue to review patch but meanwhile if you can
break the patches - so that review process be easier.

[1]
https://www.postgresql.org/message-id/CA+TgmoZV8dw1H2bzZ9xkKwdrk8+XYa+DC9H=F7heO2zna5T6qg@mail.gmail.com

Thanks,

On Fri, Oct 4, 2019 at 4:32 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

>
>
> On Thu, Oct 3, 2019 at 6:40 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> On Fri, Sep 27, 2019 at 12:00 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
>> wrote:
>> >> > - SEND_FILES_CONTENTS (file1, file2,...) - returns the files in
>> given list.
>> >> > pg_basebackup will then send back a list of filenames in this
>> command. This commands will be send by each worker and that worker will be
>> getting the said files.
>> >>
>> >> Seems reasonable, but I think you should just pass one file name and
>> >> use the command multiple times, once per file.
>> >
>> > I considered this approach initially, however, I adopted the current
>> strategy to avoid multiple round trips between the server and clients and
>> save on query processing time by issuing a single command rather than
>> multiple ones. Further fetching multiple files at once will also aid in
>> supporting the tar format by utilising the existing ReceiveTarFile()
>> function and will be able to create a tarball for per tablespace per worker.
>>
>> I think that sending multiple filenames on a line could save some time
>> when there are lots of very small files, because then the round-trip
>> overhead could be significant.
>>
>> However, if you've got mostly big files, I think this is going to be a
>> loser. It'll be fine if you're able to divide the work exactly evenly,
>> but that's pretty hard to do, because some workers may succeed in
>> copying the data faster than others for a variety of reasons: some
>> data is in memory, some data has to be read from disk, different data
>> may need to be read from different disks that run at different speeds,
>> not all the network connections may run at the same speed. Remember
>> that the backup's not done until the last worker finishes, and so
>> there may well be a significant advantage in terms of overall speed in
>> putting some energy into making sure that they finish as close to each
>> other in time as possible.
>>
>> To put that another way, the first time all the workers except one get
>> done while the last one still has 10GB of data to copy, somebody's
>> going to be unhappy.
>>
>
> I have updated the patch (see the attached patch) to include tablespace
> support, tar format support and all other backup base backup options to
> work in parallel mode as well. As previously suggested, I have removed
> BASE_BACKUP [PARALLEL] and have added START_BACKUP instead to start the
> backup. The tar format will write multiple tar files depending upon the
> number of workers specified. Also made all commands
> (START_BACKUP/SEND_FILES_CONTENT/STOP_BACKUP) to accept the
> base_backup_opt_list. This way the command-line options can also be
> provided to these commands. Since the command-line options don't change
> once the backup initiates, I went this way instead of storing them in
> shared state.
>
> The START_BACKUP command will now return a sorted list of files in
> descending order based on file sizes. This way, the larger files will be on
> top of the list. hence these files will be assigned to workers one by one,
> making it so that the larger files will be copied before other files.
>
> Based on my understanding your main concern is that the files won't be
> distributed fairly i.e one worker might get a big file and take more time
> while others get done early with smaller files? In this approach I have
> created a list of files in descending order based on there sizes so all the
> big size files will come at the top. The maximum file size in PG is 1GB so
> if we have four workers who are picking up file from the list one by one,
> the worst case scenario is that one worker gets a file of 1GB to process
> while others get files of smaller size. However with this approach of
> descending files based on size and handing it out to workers one by one,
> there is a very high likelihood of workers getting work evenly. does this
> address your concerns?
>
> Furthermore the patch also includes the regression test. As t/
> 010_pg_basebackup.pl test-case is testing base backup comprehensively, so
> I have duplicated it to "t/040_pg_basebackup_parallel.pl" and added
> parallel option in all of its tests, to make sure parallel mode works
> expectantly. The one thing that differs from base backup is the file
> checksum reporting. In parallel mode, the total number of checksum failures
> are not reported correctly however it will abort the backup whenever a
> checksum failure occurs. This is because processes are not maintaining any
> shared state. I assume that it's not much important to report total number
> of failures vs noticing the failure and aborting.
>
>
> --
> Asif Rehman
> Highgo Software (Canada/China/Pakistan)
> URL : www.highgo.ca
>
>

--
Rushabh Lathia

On Mon, Oct 7, 2019 at 1:52 PM Rushabh Lathia <rushabh(dot)lathia(at)gmail(dot)com>
wrote:

> Thanks Asif for the patch. I am opting this for a review. Patch is
> bit big, so here are very initial comments to make the review process
> easier.
>

Thanks Rushabh for reviewing the patch.

> 1) Patch seems doing lot of code shuffling, I think it would be easy
> to review if you can break the clean up patch separately.
>
> Example:
> a: setup_throttle
> b: include_wal_files
>
> 2) As I can see this patch basically have three major phase.
>
> a) Introducing new commands like START_BACKUP, SEND_FILES_CONTENT and
> STOP_BACKUP.
> b) Implementation of actual parallel backup.
> c) Testcase
>
> I would suggest, if you can break out in three as a separate patch that
> would be nice. It will benefit in reviewing the patch.
>

Sure, why not. I will break them into multiple patches.

>
> 3) In your patch you are preparing the backup manifest (file which
> giving the information about the data files). Robert Haas, submitted
> the backup manifests patch on another thread [1], and I think we
> should use that patch to get the backup manifests for parallel backup.
>

Sure. Though the backup manifest patch calculates and includes the checksum
of backup files and is done
while the file is being transferred to the frontend-end. The manifest file
itself is copied at the
very end of the backup. In parallel backup, I need the list of filenames
before file contents are transferred, in
order to divide them into multiple workers. For that, the manifest file has
to be available when START_BACKUP
is called.

That means, backup manifest should support its creation while excluding the
checksum during START_BACKUP().
I also need the directory information as well for two reasons:

- In plain format, base path has to exist before we can write the file. we
can extract the base path from the file
but doing that for all files does not seem a good idea.
- base backup does not include the content of some directories but those
directories although empty, are still
expected in PGDATA.

I can make these changes part of parallel backup (which would be on top of
backup manifest patch) or
these changes can be done as part of manifest patch and then parallel can
use them.

Robert what do you suggest?

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

On Mon, Oct 7, 2019 at 8:48 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:
> Sure. Though the backup manifest patch calculates and includes the checksum of backup files and is done
> while the file is being transferred to the frontend-end. The manifest file itself is copied at the
> very end of the backup. In parallel backup, I need the list of filenames before file contents are transferred, in
> order to divide them into multiple workers. For that, the manifest file has to be available when START_BACKUP
> is called.
>
> That means, backup manifest should support its creation while excluding the checksum during START_BACKUP().
> I also need the directory information as well for two reasons:
>
> - In plain format, base path has to exist before we can write the file. we can extract the base path from the file
> but doing that for all files does not seem a good idea.
> - base backup does not include the content of some directories but those directories although empty, are still
> expected in PGDATA.
>
> I can make these changes part of parallel backup (which would be on top of backup manifest patch) or
> these changes can be done as part of manifest patch and then parallel can use them.
>
> Robert what do you suggest?

I think we should probably not use backup manifests here, actually. I
initially thought that would be a good idea, but after further thought
it seems like it just complicates the code to no real benefit. I
suggest that the START_BACKUP command just return a result set, like a
query, with perhaps four columns: file name, file type ('d' for
directory or 'f' for file), file size, file mtime. pg_basebackup will
ignore the mtime, but some other tools might find that useful
information.

I wonder if we should also split START_BACKUP (which should enter
non-exclusive backup mode) from GET_FILE_LIST, in case some other
client program wants to use one of those but not the other. I think
that's probably a good idea, but not sure.

I still think that the files should be requested one at a time, not a
huge long list in a single command.

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

On Mon, Oct 7, 2019 at 6:05 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Mon, Oct 7, 2019 at 8:48 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:
> > Sure. Though the backup manifest patch calculates and includes the
> checksum of backup files and is done
> > while the file is being transferred to the frontend-end. The manifest
> file itself is copied at the
> > very end of the backup. In parallel backup, I need the list of filenames
> before file contents are transferred, in
> > order to divide them into multiple workers. For that, the manifest file
> has to be available when START_BACKUP
> > is called.
> >
> > That means, backup manifest should support its creation while excluding
> the checksum during START_BACKUP().
> > I also need the directory information as well for two reasons:
> >
> > - In plain format, base path has to exist before we can write the file.
> we can extract the base path from the file
> > but doing that for all files does not seem a good idea.
> > - base backup does not include the content of some directories but those
> directories although empty, are still
> > expected in PGDATA.
> >
> > I can make these changes part of parallel backup (which would be on top
> of backup manifest patch) or
> > these changes can be done as part of manifest patch and then parallel
> can use them.
> >
> > Robert what do you suggest?
>
> I think we should probably not use backup manifests here, actually. I
> initially thought that would be a good idea, but after further thought
> it seems like it just complicates the code to no real benefit.

Okay.

> I
> suggest that the START_BACKUP command just return a result set, like a
> query, with perhaps four columns: file name, file type ('d' for
> directory or 'f' for file), file size, file mtime. pg_basebackup will
> ignore the mtime, but some other tools might find that useful
> information.
>
yes current patch already returns the result set. will add the additional
information.

> I wonder if we should also split START_BACKUP (which should enter
> non-exclusive backup mode) from GET_FILE_LIST, in case some other
> client program wants to use one of those but not the other. I think
> that's probably a good idea, but not sure.
>

Currently pg_basebackup does not enter in exclusive backup mode and other
tools have to
use pg_start_backup() and pg_stop_backup() functions to achieve that. Since
we are breaking
backup into multiple command, I believe it would be a good idea to have
this option. I will include
it in next revision of this patch.

>
> I still think that the files should be requested one at a time, not a
> huge long list in a single command.
>
sure, will make the change.

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

On Mon, Oct 7, 2019 at 6:06 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Mon, Oct 7, 2019 at 8:48 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:
> > Sure. Though the backup manifest patch calculates and includes the
> checksum of backup files and is done
> > while the file is being transferred to the frontend-end. The manifest
> file itself is copied at the
> > very end of the backup. In parallel backup, I need the list of filenames
> before file contents are transferred, in
> > order to divide them into multiple workers. For that, the manifest file
> has to be available when START_BACKUP
> > is called.
> >
> > That means, backup manifest should support its creation while excluding
> the checksum during START_BACKUP().
> > I also need the directory information as well for two reasons:
> >
> > - In plain format, base path has to exist before we can write the file.
> we can extract the base path from the file
> > but doing that for all files does not seem a good idea.
> > - base backup does not include the content of some directories but those
> directories although empty, are still
> > expected in PGDATA.
> >
> > I can make these changes part of parallel backup (which would be on top
> of backup manifest patch) or
> > these changes can be done as part of manifest patch and then parallel
> can use them.
> >
> > Robert what do you suggest?
>
> I think we should probably not use backup manifests here, actually. I
> initially thought that would be a good idea, but after further thought
> it seems like it just complicates the code to no real benefit. I
> suggest that the START_BACKUP command just return a result set, like a
> query, with perhaps four columns: file name, file type ('d' for
> directory or 'f' for file), file size, file mtime. pg_basebackup will
> ignore the mtime, but some other tools might find that useful
> information.
>
> I wonder if we should also split START_BACKUP (which should enter
> non-exclusive backup mode) from GET_FILE_LIST, in case some other
> client program wants to use one of those but not the other. I think
> that's probably a good idea, but not sure.
>
> I still think that the files should be requested one at a time, not a
> huge long list in a single command.
>

What about have an API to get the single file or list of files? We will use
a single file in
our application and other tools can get the benefit of list of files.

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

--
Ibrar Ahmed

On Mon, Oct 7, 2019 at 9:43 AM Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com> wrote:
> What about have an API to get the single file or list of files? We will use a single file in
> our application and other tools can get the benefit of list of files.

That sounds a bit speculative to me. Who is to say that anyone will
find that useful? I mean, I think it's fine and good to build the
functionality that we need in a way that maximizes the likelihood that
other tools can reuse that functionality, and I think we should do
that. But I don't think it's smart to build functionality that we
don't really need in the hope that somebody else will find it useful
unless we're pretty sure that they actually will. I don't see that as
being the case here; YMMV.

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

On Mon, Oct 7, 2019 at 6:35 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

>
>
> On Mon, Oct 7, 2019 at 6:05 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> On Mon, Oct 7, 2019 at 8:48 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
>> wrote:
>> > Sure. Though the backup manifest patch calculates and includes the
>> checksum of backup files and is done
>> > while the file is being transferred to the frontend-end. The manifest
>> file itself is copied at the
>> > very end of the backup. In parallel backup, I need the list of
>> filenames before file contents are transferred, in
>> > order to divide them into multiple workers. For that, the manifest file
>> has to be available when START_BACKUP
>> > is called.
>> >
>> > That means, backup manifest should support its creation while excluding
>> the checksum during START_BACKUP().
>> > I also need the directory information as well for two reasons:
>> >
>> > - In plain format, base path has to exist before we can write the file.
>> we can extract the base path from the file
>> > but doing that for all files does not seem a good idea.
>> > - base backup does not include the content of some directories but
>> those directories although empty, are still
>> > expected in PGDATA.
>> >
>> > I can make these changes part of parallel backup (which would be on top
>> of backup manifest patch) or
>> > these changes can be done as part of manifest patch and then parallel
>> can use them.
>> >
>> > Robert what do you suggest?
>>
>> I think we should probably not use backup manifests here, actually. I
>> initially thought that would be a good idea, but after further thought
>> it seems like it just complicates the code to no real benefit.
>
>
> Okay.
>
>
>> I
>> suggest that the START_BACKUP command just return a result set, like a
>> query, with perhaps four columns: file name, file type ('d' for
>> directory or 'f' for file), file size, file mtime. pg_basebackup will
>> ignore the mtime, but some other tools might find that useful
>> information.
>>
> yes current patch already returns the result set. will add the additional
> information.
>
>
>> I wonder if we should also split START_BACKUP (which should enter
>> non-exclusive backup mode) from GET_FILE_LIST, in case some other
>> client program wants to use one of those but not the other. I think
>> that's probably a good idea, but not sure.
>>
>
> Currently pg_basebackup does not enter in exclusive backup mode and other
> tools have to
> use pg_start_backup() and pg_stop_backup() functions to achieve that.
> Since we are breaking
> backup into multiple command, I believe it would be a good idea to have
> this option. I will include
> it in next revision of this patch.
>
>
>>
>> I still think that the files should be requested one at a time, not a
>> huge long list in a single command.
>>
> sure, will make the change.
>
>
>

I have refactored the functionality into multiple smaller patches in order
to make the review process easier. I have divided the code into backend
changes and pg_basebackup changes. The
backend replication system now supports the following commands:

- START_BACKUP
- SEND_FILE_LIST
- SEND_FILES_CONTENT
- STOP_BACKUP

The START_BACKUP will not return the list of files, instead SEND_FILE_LIST
is used for that. The START_BACKUP
now calls pg_start_backup and returns starting WAL position, tablespace
header information and content of backup label file.
Initially I was using tmp files to store the backup_label content but that
turns out to be bad idea, because there can be multiple
non-exclusive backups running. The backup label information is needed by
stop_backup so pg_basebackup will send it as part
of STOP_BACKUP.

The SEND_FILE_LIST will return the list of files. It will be returned as
resultset having four columns (filename, type, size, mtime).
The SEND_FILES_CONTENT can now return the single file or multiple files as
required. There is not much change required to
support both, so I believe it will be much useable this way if other tools
want to utilise it.

As per suggestion from Robert, I am currently working on making changes in
pg_basebackup to fetch files one by one. However that's not complete and
the attach patch
is still using the old method of multi-file fetching to test the backend
commands. I will send an updated patch which will contain the changes on
fetching file one by one.

I wanted to share the backend patch to get some feedback in the mean time.

Thanks,

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

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)

Regards,
Jeevan Ladhe

On Wed, Oct 16, 2019 at 6:49 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

>
>
> On Mon, Oct 7, 2019 at 6:35 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:
>
>>
>>
>> On Mon, Oct 7, 2019 at 6:05 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>
>>> On Mon, Oct 7, 2019 at 8:48 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
>>> wrote:
>>> > Sure. Though the backup manifest patch calculates and includes the
>>> checksum of backup files and is done
>>> > while the file is being transferred to the frontend-end. The manifest
>>> file itself is copied at the
>>> > very end of the backup. In parallel backup, I need the list of
>>> filenames before file contents are transferred, in
>>> > order to divide them into multiple workers. For that, the manifest
>>> file has to be available when START_BACKUP
>>> > is called.
>>> >
>>> > That means, backup manifest should support its creation while
>>> excluding the checksum during START_BACKUP().
>>> > I also need the directory information as well for two reasons:
>>> >
>>> > - In plain format, base path has to exist before we can write the
>>> file. we can extract the base path from the file
>>> > but doing that for all files does not seem a good idea.
>>> > - base backup does not include the content of some directories but
>>> those directories although empty, are still
>>> > expected in PGDATA.
>>> >
>>> > I can make these changes part of parallel backup (which would be on
>>> top of backup manifest patch) or
>>> > these changes can be done as part of manifest patch and then parallel
>>> can use them.
>>> >
>>> > Robert what do you suggest?
>>>
>>> I think we should probably not use backup manifests here, actually. I
>>> initially thought that would be a good idea, but after further thought
>>> it seems like it just complicates the code to no real benefit.
>>
>>
>> Okay.
>>
>>
>>> I
>>> suggest that the START_BACKUP command just return a result set, like a
>>> query, with perhaps four columns: file name, file type ('d' for
>>> directory or 'f' for file), file size, file mtime. pg_basebackup will
>>> ignore the mtime, but some other tools might find that useful
>>> information.
>>>
>> yes current patch already returns the result set. will add the additional
>> information.
>>
>>
>>> I wonder if we should also split START_BACKUP (which should enter
>>> non-exclusive backup mode) from GET_FILE_LIST, in case some other
>>> client program wants to use one of those but not the other. I think
>>> that's probably a good idea, but not sure.
>>>
>>
>> Currently pg_basebackup does not enter in exclusive backup mode and other
>> tools have to
>> use pg_start_backup() and pg_stop_backup() functions to achieve that.
>> Since we are breaking
>> backup into multiple command, I believe it would be a good idea to have
>> this option. I will include
>> it in next revision of this patch.
>>
>>
>>>
>>> I still think that the files should be requested one at a time, not a
>>> huge long list in a single command.
>>>
>> sure, will make the change.
>>
>>
>>
>
> I have refactored the functionality into multiple smaller patches in order
> to make the review process easier. I have divided the code into backend
> changes and pg_basebackup changes. The
> backend replication system now supports the following commands:
>
> - START_BACKUP
> - SEND_FILE_LIST
> - SEND_FILES_CONTENT
> - STOP_BACKUP
>
> The START_BACKUP will not return the list of files, instead SEND_FILE_LIST
> is used for that. The START_BACKUP
> now calls pg_start_backup and returns starting WAL position, tablespace
> header information and content of backup label file.
> Initially I was using tmp files to store the backup_label content but that
> turns out to be bad idea, because there can be multiple
> non-exclusive backups running. The backup label information is needed by
> stop_backup so pg_basebackup will send it as part
> of STOP_BACKUP.
>
> The SEND_FILE_LIST will return the list of files. It will be returned as
> resultset having four columns (filename, type, size, mtime).
> The SEND_FILES_CONTENT can now return the single file or multiple files as
> required. There is not much change required to
> support both, so I believe it will be much useable this way if other tools
> want to utilise it.
>
> As per suggestion from Robert, I am currently working on making changes in
> pg_basebackup to fetch files one by one. However that's not complete and
> the attach patch
> is still using the old method of multi-file fetching to test the backend
> commands. I will send an updated patch which will contain the changes on
> fetching file one by one.
>
> I wanted to share the backend patch to get some feedback in the mean time.
>
> Thanks,
>
> --
> Asif Rehman
> Highgo Software (Canada/China/Pakistan)
> URL : www.highgo.ca
>
>

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

On Thu, Oct 17, 2019 at 10:51 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

>
> Attached are the updated patches.
>

I had a quick look over these changes and they look good overall.
However, here are my few review comments I caught while glancing the patches
0002 and 0003.

--- 0002 patch

1.
Can lsn option be renamed to start-wal-location? This will be more clear
too.

2.
+typedef struct
+{
+ char name[MAXPGPATH];
+ char type;
+ int32 size;
+ time_t mtime;
+} BackupFile;

I think it will be good if we keep this structure in a common place so that
the client can also use it.

3.
+ SEND_FILE_LIST,
+ SEND_FILES_CONTENT,
Can above two commands renamed to SEND_BACKUP_MANIFEST and SEND_BACKUP_FILE
respectively?
The reason behind the first name change is, we are not getting only file
lists
here instead we are getting a few more details with that too. And for
others,
it will be inline with START_BACKUP/STOP_BACKUP/SEND_BACKUP_MANIFEST.

4.
Typos:
non-exlusive => non-exclusive
retured => returned
optionaly => optionally
nessery => necessary
totoal => total

--- 0003 patch

1.
+static int
+simple_list_length(SimpleStringList *list)
+{
+ int len = 0;
+ SimpleStringListCell *cell;
+
+ for (cell = list->head; cell; cell = cell->next, len++)
+ ;
+
+ return len;
+}

I think it will be good if it goes to simple_list.c. That will help in other
usages as well.

2.
Please revert these unnecessary changes:

@@ -1475,6 +1575,7 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res,
int rownum)
*/
snprintf(filename, sizeof(filename), "%s/%s", current_path,
copybuf);
+
if (filename[strlen(filename) - 1] == '/')
{
/*

@@ -1528,8 +1622,8 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res,
int rownum)
* can map them too.)
*/
filename[strlen(filename) - 1] = '\0'; /* Remove
trailing slash */
-
mapped_tblspc_path =
get_tablespace_mapping(&copybuf[157]);
+
if (symlink(mapped_tblspc_path, filename) != 0)
{
pg_log_error("could not create symbolic link from
\"%s\" to \"%s\": %m",

3.
Typos:
retrive => retrieve
takecare => take care
tablespae => tablespace

4.
ParallelBackupEnd() function does not do anything for parallelism. Will it
be
better to just rename it as EndBackup()?

5.
To pass a tablespace path to the server in SEND_FILES_CONTENT, you are
reusing
a LABEL option, that seems odd. How about adding a new option for that?

6.
It will be good if we have some comments explaining what the function is
actually doing in its prologue. For functions like:
GetBackupFilesList()
ReceiveFiles()
create_workers_and_fetch()

Thanks

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

--
Jeevan Chalke
Associate Database Architect & Team Lead, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

On Fri, Oct 18, 2019 at 4:12 PM Jeevan Chalke <
jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:

>
>
> On Thu, Oct 17, 2019 at 10:51 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
> wrote:
>
>>
>> Attached are the updated patches.
>>
>
> I had a quick look over these changes and they look good overall.
> However, here are my few review comments I caught while glancing the
> patches
> 0002 and 0003.
>
>
> --- 0002 patch
>
> 1.
> Can lsn option be renamed to start-wal-location? This will be more clear
> too.
>
> 2.
> +typedef struct
> +{
> + char name[MAXPGPATH];
> + char type;
> + int32 size;
> + time_t mtime;
> +} BackupFile;
>
> I think it will be good if we keep this structure in a common place so that
> the client can also use it.
>
> 3.
> + SEND_FILE_LIST,
> + SEND_FILES_CONTENT,
> Can above two commands renamed to SEND_BACKUP_MANIFEST and SEND_BACKUP_FILE
> respectively?
> The reason behind the first name change is, we are not getting only file
> lists
> here instead we are getting a few more details with that too. And for
> others,
> it will be inline with START_BACKUP/STOP_BACKUP/SEND_BACKUP_MANIFEST.
>
> 4.
> Typos:
> non-exlusive => non-exclusive
> retured => returned
> optionaly => optionally
> nessery => necessary
> totoal => total
>
>
> --- 0003 patch
>
> 1.
> +static int
> +simple_list_length(SimpleStringList *list)
> +{
> + int len = 0;
> + SimpleStringListCell *cell;
> +
> + for (cell = list->head; cell; cell = cell->next, len++)
> + ;
> +
> + return len;
> +}
>
> I think it will be good if it goes to simple_list.c. That will help in
> other
> usages as well.
>
> 2.
> Please revert these unnecessary changes:
>
> @@ -1475,6 +1575,7 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res,
> int rownum)
> */
> snprintf(filename, sizeof(filename), "%s/%s", current_path,
> copybuf);
> +
> if (filename[strlen(filename) - 1] == '/')
> {
> /*
>
> @@ -1528,8 +1622,8 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult *res,
> int rownum)
> * can map them too.)
> */
> filename[strlen(filename) - 1] = '\0'; /* Remove
> trailing slash */
> -
> mapped_tblspc_path =
> get_tablespace_mapping(&copybuf[157]);
> +
> if (symlink(mapped_tblspc_path, filename) != 0)
> {
> pg_log_error("could not create symbolic link from
> \"%s\" to \"%s\": %m",
>
> 3.
> Typos:
> retrive => retrieve
> takecare => take care
> tablespae => tablespace
>
> 4.
> ParallelBackupEnd() function does not do anything for parallelism. Will it
> be
> better to just rename it as EndBackup()?
>
> 5.
> To pass a tablespace path to the server in SEND_FILES_CONTENT, you are
> reusing
> a LABEL option, that seems odd. How about adding a new option for that?
>
> 6.
> It will be good if we have some comments explaining what the function is
> actually doing in its prologue. For functions like:
> GetBackupFilesList()
> ReceiveFiles()
> create_workers_and_fetch()
>
>
> Thanks
>
>
>>
>> Thanks,
>>
>> --
>> Asif Rehman
>> Highgo Software (Canada/China/Pakistan)
>> URL : www.highgo.ca
>>
>>
>
> --
> Jeevan Chalke
> Associate Database Architect & Team Lead, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
>
I had a detailed discussion with Robert Haas at PostgreConf Europe about
parallel backup.
We discussed the current state of the patch and what needs to be done to
get the patch committed.

- The current patch uses a process to implement parallelism. There are many
reasons we need to use threads instead of processes. To start with, as this
is a client utility it makes
more sense to use threads. The data needs to be shared amongst different
threads and the main process,
handling that is simpler as compared to interprocess communication.

- Fetching a single file or multiple files was also discussed. We concluded
in our discussion that we
need to benchmark to see if disk I/O is a bottleneck or not and if parallel
writing gives us
any benefit. This benchmark needs to be done on different hardware and
different
network to identify which are the real bottlenecks. In general, we agreed
that we could start with fetching
one file at a time but that will be revisited after the benchmarks are done.

- There is also an ongoing debate in this thread that we should have one
single tar file for all files or one
TAR file per thread. I really want to have a single tar file because the
main purpose of the TAR file is to
reduce the management of multiple files, but in case of one file per
thread, we end up with many tar
files. Therefore we need to have one master thread which is responsible for
writing on tar file and all
the other threads will receive the data from the network and stream to the
master thread. This also
supports the idea of using a thread-based model rather than a process-based
approach because it
requires too much data sharing between processes. If we cannot achieve
this, then we can disable the
TAR option for parallel backup in the first version.

- In the case of data sharing, we need to try to avoid unnecessary locking
and more suitable algorithm to
solve the reader-writer problem is required.

--
Ibrar Ahmed

On Thu, Oct 24, 2019 at 3:21 PM Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com> wrote:

>
>
> On Fri, Oct 18, 2019 at 4:12 PM Jeevan Chalke <
> jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>
>>
>>
>> On Thu, Oct 17, 2019 at 10:51 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
>> wrote:
>>
>>>
>>> Attached are the updated patches.
>>>
>>
>> I had a quick look over these changes and they look good overall.
>> However, here are my few review comments I caught while glancing the
>> patches
>> 0002 and 0003.
>>
>>
>> --- 0002 patch
>>
>> 1.
>> Can lsn option be renamed to start-wal-location? This will be more clear
>> too.
>>
>> 2.
>> +typedef struct
>> +{
>> + char name[MAXPGPATH];
>> + char type;
>> + int32 size;
>> + time_t mtime;
>> +} BackupFile;
>>
>> I think it will be good if we keep this structure in a common place so
>> that
>> the client can also use it.
>>
>> 3.
>> + SEND_FILE_LIST,
>> + SEND_FILES_CONTENT,
>> Can above two commands renamed to SEND_BACKUP_MANIFEST and
>> SEND_BACKUP_FILE
>> respectively?
>> The reason behind the first name change is, we are not getting only file
>> lists
>> here instead we are getting a few more details with that too. And for
>> others,
>> it will be inline with START_BACKUP/STOP_BACKUP/SEND_BACKUP_MANIFEST.
>>
>> 4.
>> Typos:
>> non-exlusive => non-exclusive
>> retured => returned
>> optionaly => optionally
>> nessery => necessary
>> totoal => total
>>
>>
>> --- 0003 patch
>>
>> 1.
>> +static int
>> +simple_list_length(SimpleStringList *list)
>> +{
>> + int len = 0;
>> + SimpleStringListCell *cell;
>> +
>> + for (cell = list->head; cell; cell = cell->next, len++)
>> + ;
>> +
>> + return len;
>> +}
>>
>> I think it will be good if it goes to simple_list.c. That will help in
>> other
>> usages as well.
>>
>> 2.
>> Please revert these unnecessary changes:
>>
>> @@ -1475,6 +1575,7 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult
>> *res, int rownum)
>> */
>> snprintf(filename, sizeof(filename), "%s/%s", current_path,
>> copybuf);
>> +
>> if (filename[strlen(filename) - 1] == '/')
>> {
>> /*
>>
>> @@ -1528,8 +1622,8 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult
>> *res, int rownum)
>> * can map them too.)
>> */
>> filename[strlen(filename) - 1] = '\0'; /* Remove
>> trailing slash */
>> -
>> mapped_tblspc_path =
>> get_tablespace_mapping(&copybuf[157]);
>> +
>> if (symlink(mapped_tblspc_path, filename) != 0)
>> {
>> pg_log_error("could not create symbolic link
>> from \"%s\" to \"%s\": %m",
>>
>> 3.
>> Typos:
>> retrive => retrieve
>> takecare => take care
>> tablespae => tablespace
>>
>> 4.
>> ParallelBackupEnd() function does not do anything for parallelism. Will
>> it be
>> better to just rename it as EndBackup()?
>>
>> 5.
>> To pass a tablespace path to the server in SEND_FILES_CONTENT, you are
>> reusing
>> a LABEL option, that seems odd. How about adding a new option for that?
>>
>> 6.
>> It will be good if we have some comments explaining what the function is
>> actually doing in its prologue. For functions like:
>> GetBackupFilesList()
>> ReceiveFiles()
>> create_workers_and_fetch()
>>
>>
>> Thanks
>>
>>
>>>
>>> Thanks,
>>>
>>> --
>>> Asif Rehman
>>> Highgo Software (Canada/China/Pakistan)
>>> URL : www.highgo.ca
>>>
>>>
>>
>> --
>> Jeevan Chalke
>> Associate Database Architect & Team Lead, Product Development
>> EnterpriseDB Corporation
>> The Enterprise PostgreSQL Company
>>
>>
> I had a detailed discussion with Robert Haas at PostgreConf Europe about
> parallel backup.
> We discussed the current state of the patch and what needs to be done to
> get the patch committed.
>
> - The current patch uses a process to implement parallelism. There are many
> reasons we need to use threads instead of processes. To start with, as
> this is a client utility it makes
> more sense to use threads. The data needs to be shared amongst different
> threads and the main process,
> handling that is simpler as compared to interprocess communication.
>

Yes I agree. I have already converted the code to use threads instead of
processes. This avoids the overhead
of interprocess communication.

With a single file fetching strategy, this requires communication between
competing threads/processes. To handle
that in a multiprocess application, it requires IPC. The current approach
of multiple threads instead of processes
avoids this overhead.

> - Fetching a single file or multiple files was also discussed. We
> concluded in our discussion that we
> need to benchmark to see if disk I/O is a bottleneck or not and if
> parallel writing gives us
> any benefit. This benchmark needs to be done on different hardware and
> different
> network to identify which are the real bottlenecks. In general, we agreed
> that we could start with fetching
> one file at a time but that will be revisited after the benchmarks are
> done.
>

I'll share the updated patch in the next couple of days. After that, I'll
work on benchmarking that in
different environments that I have.

>
> - There is also an ongoing debate in this thread that we should have one
> single tar file for all files or one
> TAR file per thread. I really want to have a single tar file because the
> main purpose of the TAR file is to
> reduce the management of multiple files, but in case of one file per
> thread, we end up with many tar
> files. Therefore we need to have one master thread which is responsible
> for writing on tar file and all
> the other threads will receive the data from the network and stream to the
> master thread. This also
> supports the idea of using a thread-based model rather than a
> process-based approach because it
> requires too much data sharing between processes. If we cannot achieve
> this, then we can disable the
> TAR option for parallel backup in the first version.
>

I am in favour of disabling the tar format for the first version of
parallel backup.

> - In the case of data sharing, we need to try to avoid unnecessary locking
> and more suitable algorithm to
> solve the reader-writer problem is required.
>
> --
> Ibrar Ahmed
>

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

On Thu, Oct 24, 2019 at 4:24 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

>
>
> On Thu, Oct 24, 2019 at 3:21 PM Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com> wrote:
>
>>
>>
>> On Fri, Oct 18, 2019 at 4:12 PM Jeevan Chalke <
>> jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>>
>>>
>>>
>>> On Thu, Oct 17, 2019 at 10:51 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
>>> wrote:
>>>
>>>>
>>>> Attached are the updated patches.
>>>>
>>>
>>> I had a quick look over these changes and they look good overall.
>>> However, here are my few review comments I caught while glancing the
>>> patches
>>> 0002 and 0003.
>>>
>>>
>>> --- 0002 patch
>>>
>>> 1.
>>> Can lsn option be renamed to start-wal-location? This will be more clear
>>> too.
>>>
>>> 2.
>>> +typedef struct
>>> +{
>>> + char name[MAXPGPATH];
>>> + char type;
>>> + int32 size;
>>> + time_t mtime;
>>> +} BackupFile;
>>>
>>> I think it will be good if we keep this structure in a common place so
>>> that
>>> the client can also use it.
>>>
>>> 3.
>>> + SEND_FILE_LIST,
>>> + SEND_FILES_CONTENT,
>>> Can above two commands renamed to SEND_BACKUP_MANIFEST and
>>> SEND_BACKUP_FILE
>>> respectively?
>>> The reason behind the first name change is, we are not getting only file
>>> lists
>>> here instead we are getting a few more details with that too. And for
>>> others,
>>> it will be inline with START_BACKUP/STOP_BACKUP/SEND_BACKUP_MANIFEST.
>>>
>>> 4.
>>> Typos:
>>> non-exlusive => non-exclusive
>>> retured => returned
>>> optionaly => optionally
>>> nessery => necessary
>>> totoal => total
>>>
>>>
>>> --- 0003 patch
>>>
>>> 1.
>>> +static int
>>> +simple_list_length(SimpleStringList *list)
>>> +{
>>> + int len = 0;
>>> + SimpleStringListCell *cell;
>>> +
>>> + for (cell = list->head; cell; cell = cell->next, len++)
>>> + ;
>>> +
>>> + return len;
>>> +}
>>>
>>> I think it will be good if it goes to simple_list.c. That will help in
>>> other
>>> usages as well.
>>>
>>> 2.
>>> Please revert these unnecessary changes:
>>>
>>> @@ -1475,6 +1575,7 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult
>>> *res, int rownum)
>>> */
>>> snprintf(filename, sizeof(filename), "%s/%s", current_path,
>>> copybuf);
>>> +
>>> if (filename[strlen(filename) - 1] == '/')
>>> {
>>> /*
>>>
>>> @@ -1528,8 +1622,8 @@ ReceiveAndUnpackTarFile(PGconn *conn, PGresult
>>> *res, int rownum)
>>> * can map them too.)
>>> */
>>> filename[strlen(filename) - 1] = '\0'; /* Remove
>>> trailing slash */
>>> -
>>> mapped_tblspc_path =
>>> get_tablespace_mapping(&copybuf[157]);
>>> +
>>> if (symlink(mapped_tblspc_path, filename) != 0)
>>> {
>>> pg_log_error("could not create symbolic link
>>> from \"%s\" to \"%s\": %m",
>>>
>>> 3.
>>> Typos:
>>> retrive => retrieve
>>> takecare => take care
>>> tablespae => tablespace
>>>
>>> 4.
>>> ParallelBackupEnd() function does not do anything for parallelism. Will
>>> it be
>>> better to just rename it as EndBackup()?
>>>
>>> 5.
>>> To pass a tablespace path to the server in SEND_FILES_CONTENT, you are
>>> reusing
>>> a LABEL option, that seems odd. How about adding a new option for that?
>>>
>>> 6.
>>> It will be good if we have some comments explaining what the function is
>>> actually doing in its prologue. For functions like:
>>> GetBackupFilesList()
>>> ReceiveFiles()
>>> create_workers_and_fetch()
>>>
>>>
>>> Thanks
>>>
>>>
>>>>
>>>> Thanks,
>>>>
>>>> --
>>>> Asif Rehman
>>>> Highgo Software (Canada/China/Pakistan)
>>>> URL : www.highgo.ca
>>>>
>>>>
>>>
>>> --
>>> Jeevan Chalke
>>> Associate Database Architect & Team Lead, Product Development
>>> EnterpriseDB Corporation
>>> The Enterprise PostgreSQL Company
>>>
>>>
>> I had a detailed discussion with Robert Haas at PostgreConf Europe about
>> parallel backup.
>> We discussed the current state of the patch and what needs to be done to
>> get the patch committed.
>>
>> - The current patch uses a process to implement parallelism. There are
>> many
>> reasons we need to use threads instead of processes. To start with, as
>> this is a client utility it makes
>> more sense to use threads. The data needs to be shared amongst different
>> threads and the main process,
>> handling that is simpler as compared to interprocess communication.
>>
>
> Yes I agree. I have already converted the code to use threads instead of
> processes. This avoids the overhead
> of interprocess communication.
>
> With a single file fetching strategy, this requires communication between
> competing threads/processes. To handle
> that in a multiprocess application, it requires IPC. The current approach
> of multiple threads instead of processes
> avoids this overhead.
>
>
>> - Fetching a single file or multiple files was also discussed. We
>> concluded in our discussion that we
>> need to benchmark to see if disk I/O is a bottleneck or not and if
>> parallel writing gives us
>> any benefit. This benchmark needs to be done on different hardware and
>> different
>> network to identify which are the real bottlenecks. In general, we agreed
>> that we could start with fetching
>> one file at a time but that will be revisited after the benchmarks are
>> done.
>>
>
> I'll share the updated patch in the next couple of days. After that, I'll
> work on benchmarking that in
> different environments that I have.
>
>
>>
>> - There is also an ongoing debate in this thread that we should have one
>> single tar file for all files or one
>> TAR file per thread. I really want to have a single tar file because the
>> main purpose of the TAR file is to
>> reduce the management of multiple files, but in case of one file per
>> thread, we end up with many tar
>> files. Therefore we need to have one master thread which is responsible
>> for writing on tar file and all
>> the other threads will receive the data from the network and stream to
>> the master thread. This also
>> supports the idea of using a thread-based model rather than a
>> process-based approach because it
>> requires too much data sharing between processes. If we cannot achieve
>> this, then we can disable the
>> TAR option for parallel backup in the first version.
>>
>
> I am in favour of disabling the tar format for the first version of
> parallel backup.
>
>
>> - In the case of data sharing, we need to try to avoid unnecessary
>> locking and more suitable algorithm to
>> solve the reader-writer problem is required.
>>
>> --
>> Ibrar Ahmed
>>
>
>
> --
> Asif Rehman
> Highgo Software (Canada/China/Pakistan)
> URL : www.highgo.ca
>
>
I have updated the patch to include the changes suggested by Jeevan. This
patch also implements the thread workers instead of
processes and fetches a single file at a time. The tar format has been
disabled for first version of parallel backup.

Conversion from the previous process based application to the current
thread based one required slight modification in data structure,
addition of a few new functions and progress reporting functionality.

The core data structure remains in tact where table space based file
listing is maintained, however, we are now maintaining a list of all
files (maintaining pointers to FileInfo structure; so no duplication of
data), so that we can sequentially access these without adding too
much processing in critical section. The current scope of the critical
section for thread workers is limited to incrementing the file index
within the list of files.

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

On Mon, Oct 28, 2019 at 10:03 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:
> I have updated the patch to include the changes suggested by Jeevan. This patch also implements the thread workers instead of
> processes and fetches a single file at a time. The tar format has been disabled for first version of parallel backup.

Looking at 0001-0003:

It's not clear to me what the purpose of the start WAL location is
supposed to be. As far as I can see, SendBackupFiles() stores it in a
variable which is then used for exactly nothing, and nothing else uses
it. It seems like that would be part of a potential incremental
backup feature, but I don't see what it's got to do with parallel full
backup.

The tablespace_path option appears entirely unused, and I don't know
why that should be necessary here, either.

STORE_BACKUPFILE() seems like maybe it should be a function rather
than a macro, and also probably be renamed, because it doesn't store
files and the argument's not necessarily a file.

SendBackupManifest() does not send a backup manifest in the sense
contemplated by the email thread on that subject. It sends a file
list. That seems like the right idea - IMHO, anyway - but you need to
do a thorough renaming.

I think it would be fine to decide that this facility won't support
exclusive-mode backup.

I don't think much of having both sendDir() and sendDir_(). The latter
name is inconsistent with any naming convention we have, and there
seems to be no reason not to just add an argument to sendDir() and
change the callers.

I think we should rename - perhaps as a preparatory patch - the
sizeonly flag to dryrun, or something like that.

The resource cleanup does not look right. You've included calls to
PG_ENSURE_ERROR_CLEANUP(base_backup_cleanup, 0) in both StartBackup()
and StopBackup(), but what happens if there is an error or even a
clean shutdown of the connection in between? I think that there needs
to be some change here to ensure that a walsender will always call
base_backup_cleanup() when it exits; I think that'd probably remove
the need for any PG_ENSURE_ERROR_CLEANUP calls at all, including ones
we have already. This might also be something that could be done as a
separate, prepatory refactoring patch.

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

On Mon, Oct 28, 2019 at 8:29 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Mon, Oct 28, 2019 at 10:03 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
> wrote:
> > I have updated the patch to include the changes suggested by Jeevan.
> This patch also implements the thread workers instead of
> > processes and fetches a single file at a time. The tar format has been
> disabled for first version of parallel backup.
>
> Looking at 0001-0003:
>
> It's not clear to me what the purpose of the start WAL location is
> supposed to be. As far as I can see, SendBackupFiles() stores it in a
> variable which is then used for exactly nothing, and nothing else uses
> it. It seems like that would be part of a potential incremental
> backup feature, but I don't see what it's got to do with parallel full
> backup.
>

'startptr' is used by sendFile() during checksum verification. Since
SendBackupFiles() is using sendFIle we have to set a valid WAL location.

> The tablespace_path option appears entirely unused, and I don't know
> why that should be necessary here, either.
>

This is to calculate the basepathlen. We need to exclude the tablespace
location (or
base path) from the filename before it is sent to the client with sendFile
call. I added
this option primarily to avoid performing string manipulation on filename
to extract the
tablespace location and then calculate the basepathlen.

Alternatively we can do it by extracting the base path from the received
filename. What
do you suggest?

>
> STORE_BACKUPFILE() seems like maybe it should be a function rather
> than a macro, and also probably be renamed, because it doesn't store
> files and the argument's not necessarily a file.
>
Sure.

>
> SendBackupManifest() does not send a backup manifest in the sense
> contemplated by the email thread on that subject. It sends a file
> list. That seems like the right idea - IMHO, anyway - but you need to
> do a thorough renaming.
>

I'm considering the following command names:
START_BACKUP
- Starts the backup process

SEND_BACKUP_FILELIST (Instead of SEND_BACKUP_MANIFEST)
- Sends the list of all files (along with file information such as
filename, file type (directory/file/link),
file size and file mtime for each file) to be backed up.

SEND_BACKUP_FILES
- Sends one or more files to the client.

STOP_BACKUP
- Stops the backup process.

I'll update the function names accordingly after your confirmation. Of
course, suggestions for
better names are welcome.

>
> I think it would be fine to decide that this facility won't support
> exclusive-mode backup.
>

Sure. Will drop this patch.

>
> I don't think much of having both sendDir() and sendDir_(). The latter
> name is inconsistent with any naming convention we have, and there
> seems to be no reason not to just add an argument to sendDir() and
> change the callers.

> I think we should rename - perhaps as a preparatory patch - the
> sizeonly flag to dryrun, or something like that.
>

Sure, will take care of it.

> The resource cleanup does not look right. You've included calls to
> PG_ENSURE_ERROR_CLEANUP(base_backup_cleanup, 0) in both StartBackup()
> and StopBackup(), but what happens if there is an error or even a
> clean shutdown of the connection in between? I think that there needs

to be some change here to ensure that a walsender will always call
> base_backup_cleanup() when it exits; I think that'd probably remove
> the need for any PG_ENSURE_ERROR_CLEANUP calls at all, including ones
> we have already. This might also be something that could be done as a
> separate, prepatory refactoring patch.
>

You're right. I didn't handle this case properly. I will removed
PG_ENSURE_ERROR_CLEANUP
calls and replace it with before_shmem_exit handler. This way
whenever backend process exits,
base_backup_cleanup will be called:
- If it exists before calling the do_pg_stop_backup, base_backup_cleanup
will take care of cleanup.
- otherwise in case of a clean shutdown (after calling do_pg_stop_backup)
then base_backup_cleanup
will simply return without doing anything.

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

On Wed, Oct 30, 2019 at 7:16 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

>
>
> On Mon, Oct 28, 2019 at 8:29 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> On Mon, Oct 28, 2019 at 10:03 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
>> wrote:
>> > I have updated the patch to include the changes suggested by Jeevan.
>> This patch also implements the thread workers instead of
>> > processes and fetches a single file at a time. The tar format has been
>> disabled for first version of parallel backup.
>>
>> Looking at 0001-0003:
>>
>> It's not clear to me what the purpose of the start WAL location is
>> supposed to be. As far as I can see, SendBackupFiles() stores it in a
>> variable which is then used for exactly nothing, and nothing else uses
>> it. It seems like that would be part of a potential incremental
>> backup feature, but I don't see what it's got to do with parallel full
>> backup.
>>
>
> 'startptr' is used by sendFile() during checksum verification. Since
> SendBackupFiles() is using sendFIle we have to set a valid WAL location.
>
>
>> The tablespace_path option appears entirely unused, and I don't know
>> why that should be necessary here, either.
>>
>
> This is to calculate the basepathlen. We need to exclude the tablespace
> location (or
> base path) from the filename before it is sent to the client with sendFile
> call. I added
> this option primarily to avoid performing string manipulation on filename
> to extract the
> tablespace location and then calculate the basepathlen.
>
> Alternatively we can do it by extracting the base path from the received
> filename. What
> do you suggest?
>
>
>>
>> STORE_BACKUPFILE() seems like maybe it should be a function rather
>> than a macro, and also probably be renamed, because it doesn't store
>> files and the argument's not necessarily a file.
>>
> Sure.
>
>
>>
>> SendBackupManifest() does not send a backup manifest in the sense
>> contemplated by the email thread on that subject. It sends a file
>> list. That seems like the right idea - IMHO, anyway - but you need to
>> do a thorough renaming.
>>
>
> I'm considering the following command names:
> START_BACKUP
> - Starts the backup process
>
> SEND_BACKUP_FILELIST (Instead of SEND_BACKUP_MANIFEST)
> - Sends the list of all files (along with file information such as
> filename, file type (directory/file/link),
> file size and file mtime for each file) to be backed up.
>
> SEND_BACKUP_FILES
> - Sends one or more files to the client.
>
> STOP_BACKUP
> - Stops the backup process.
>
> I'll update the function names accordingly after your confirmation. Of
> course, suggestions for
> better names are welcome.
>
>
>>
>> I think it would be fine to decide that this facility won't support
>> exclusive-mode backup.
>>
>
> Sure. Will drop this patch.
>
>
>>
>> I don't think much of having both sendDir() and sendDir_(). The latter
>> name is inconsistent with any naming convention we have, and there
>> seems to be no reason not to just add an argument to sendDir() and
>> change the callers.
>
>
>> I think we should rename - perhaps as a preparatory patch - the
>> sizeonly flag to dryrun, or something like that.
>>
>
> Sure, will take care of it.
>
>
>> The resource cleanup does not look right. You've included calls to
>> PG_ENSURE_ERROR_CLEANUP(base_backup_cleanup, 0) in both StartBackup()
>> and StopBackup(), but what happens if there is an error or even a
>> clean shutdown of the connection in between? I think that there needs
>
> to be some change here to ensure that a walsender will always call
>> base_backup_cleanup() when it exits; I think that'd probably remove
>> the need for any PG_ENSURE_ERROR_CLEANUP calls at all, including ones
>> we have already. This might also be something that could be done as a
>> separate, prepatory refactoring patch.
>>
>
> You're right. I didn't handle this case properly. I will removed
> PG_ENSURE_ERROR_CLEANUP
> calls and replace it with before_shmem_exit handler. This way
> whenever backend process exits,
> base_backup_cleanup will be called:
> - If it exists before calling the do_pg_stop_backup, base_backup_cleanup
> will take care of cleanup.
> - otherwise in case of a clean shutdown (after calling do_pg_stop_backup)
> then base_backup_cleanup
> will simply return without doing anything.
>
>
>
The updated patches are attached.

Thanks,

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

On Wed, Oct 30, 2019 at 10:16 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:
> 'startptr' is used by sendFile() during checksum verification. Since
> SendBackupFiles() is using sendFIle we have to set a valid WAL location.

Ugh, global variables.

Why are START_BACKUP, SEND_BACKUP_FILELIST, SEND_BACKUP_FILES, and
STOP_BACKUP all using the same base_backup_opt_list production as
BASE_BACKUP? Presumably most of those options are not applicable to
most of those commands, and the productions should therefore be
separated.

You should add docs, too. I wouldn't have to guess what some of this
stuff was for if you wrote documentation explaining what this stuff
was for. :-)

>> The tablespace_path option appears entirely unused, and I don't know
>> why that should be necessary here, either.
>
> This is to calculate the basepathlen. We need to exclude the tablespace location (or
> base path) from the filename before it is sent to the client with sendFile call. I added
> this option primarily to avoid performing string manipulation on filename to extract the
> tablespace location and then calculate the basepathlen.
>
> Alternatively we can do it by extracting the base path from the received filename. What
> do you suggest?

I don't think the server needs any information from the client in
order to be able to exclude the tablespace location from the pathname.
Whatever it needs to know, it should be able to figure out, just as it
would in a non-parallel backup.

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

On Fri, Nov 1, 2019 at 8:53 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Wed, Oct 30, 2019 at 10:16 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
> wrote:
> > 'startptr' is used by sendFile() during checksum verification. Since
> > SendBackupFiles() is using sendFIle we have to set a valid WAL location.
>
> Ugh, global variables.
>
> Why are START_BACKUP, SEND_BACKUP_FILELIST, SEND_BACKUP_FILES, and
> STOP_BACKUP all using the same base_backup_opt_list production as
> BASE_BACKUP? Presumably most of those options are not applicable to
> most of those commands, and the productions should therefore be
> separated.
>

Are you expecting something like the attached patch? Basically I have
reorganised the grammar
rules so each command can have the options required by it.

I was feeling a bit reluctant for this change because it may add some
unwanted grammar rules in
the replication grammar. Since these commands are using the same options as
base backup, may
be we could throw error inside the relevant functions on unwanted options?

> You should add docs, too. I wouldn't have to guess what some of this
> stuff was for if you wrote documentation explaining what this stuff
> was for. :-)
>

Yes I will add it in the next patch.

>
> >> The tablespace_path option appears entirely unused, and I don't know
> >> why that should be necessary here, either.
> >
> > This is to calculate the basepathlen. We need to exclude the tablespace
> location (or
> > base path) from the filename before it is sent to the client with
> sendFile call. I added
> > this option primarily to avoid performing string manipulation on
> filename to extract the
> > tablespace location and then calculate the basepathlen.
> >
> > Alternatively we can do it by extracting the base path from the received
> filename. What
> > do you suggest?
>
> I don't think the server needs any information from the client in
> order to be able to exclude the tablespace location from the pathname.
> Whatever it needs to know, it should be able to figure out, just as it
> would in a non-parallel backup.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

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

On Mon, Nov 4, 2019 at 6:08 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

>
>
> On Fri, Nov 1, 2019 at 8:53 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> On Wed, Oct 30, 2019 at 10:16 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
>> wrote:
>> > 'startptr' is used by sendFile() during checksum verification. Since
>> > SendBackupFiles() is using sendFIle we have to set a valid WAL location.
>>
>> Ugh, global variables.
>>
>> Why are START_BACKUP, SEND_BACKUP_FILELIST, SEND_BACKUP_FILES, and
>> STOP_BACKUP all using the same base_backup_opt_list production as
>> BASE_BACKUP? Presumably most of those options are not applicable to
>> most of those commands, and the productions should therefore be
>> separated.
>>
>
> Are you expecting something like the attached patch? Basically I have
> reorganised the grammar
> rules so each command can have the options required by it.
>
> I was feeling a bit reluctant for this change because it may add some
> unwanted grammar rules in
> the replication grammar. Since these commands are using the same options
> as base backup, may
> be we could throw error inside the relevant functions on unwanted options?
>
>
>
>> You should add docs, too. I wouldn't have to guess what some of this
>> stuff was for if you wrote documentation explaining what this stuff
>> was for. :-)
>>
>
> Yes I will add it in the next patch.
>
>
>>
>> >> The tablespace_path option appears entirely unused, and I don't know
>> >> why that should be necessary here, either.
>> >
>> > This is to calculate the basepathlen. We need to exclude the tablespace
>> location (or
>> > base path) from the filename before it is sent to the client with
>> sendFile call. I added
>> > this option primarily to avoid performing string manipulation on
>> filename to extract the
>> > tablespace location and then calculate the basepathlen.
>> >
>> > Alternatively we can do it by extracting the base path from the
>> received filename. What
>> > do you suggest?
>>
>> I don't think the server needs any information from the client in
>> order to be able to exclude the tablespace location from the pathname.
>> Whatever it needs to know, it should be able to figure out, just as it
>> would in a non-parallel backup.
>>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>
I have updated the replication grammar with some new rules to differentiate
the options production
for base backup and newly added commands.

I have also created a separate patch to include the documentation changes.
The current syntax is as below:

- START_BACKUP [ LABEL 'label' ] [ PROGRESS ] [ FAST ] [ TABLESPACE_MAP ]
- STOP_BACKUP [ LABEL 'label' ] [ WAL ] [ NOWAIT ]
- SEND_BACKUP_FILELIST
- SEND_BACKUP_FILES ( 'FILE' [, ...] ) [ MAX_RATE rate ] [
NOVERIFY_CHECKSUMS ] [ START_WAL_LOCATION ]

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

On Tue, Nov 12, 2019 at 5:07 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

>
>
> On Mon, Nov 4, 2019 at 6:08 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:
>
>>
>>
>> On Fri, Nov 1, 2019 at 8:53 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>
>>> On Wed, Oct 30, 2019 at 10:16 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
>>> wrote:
>>> > 'startptr' is used by sendFile() during checksum verification. Since
>>> > SendBackupFiles() is using sendFIle we have to set a valid WAL
>>> location.
>>>
>>> Ugh, global variables.
>>>
>>> Why are START_BACKUP, SEND_BACKUP_FILELIST, SEND_BACKUP_FILES, and
>>> STOP_BACKUP all using the same base_backup_opt_list production as
>>> BASE_BACKUP? Presumably most of those options are not applicable to
>>> most of those commands, and the productions should therefore be
>>> separated.
>>>
>>
>> Are you expecting something like the attached patch? Basically I have
>> reorganised the grammar
>> rules so each command can have the options required by it.
>>
>> I was feeling a bit reluctant for this change because it may add some
>> unwanted grammar rules in
>> the replication grammar. Since these commands are using the same options
>> as base backup, may
>> be we could throw error inside the relevant functions on unwanted options?
>>
>>
>>
>>> You should add docs, too. I wouldn't have to guess what some of this
>>> stuff was for if you wrote documentation explaining what this stuff
>>> was for. :-)
>>>
>>
>> Yes I will add it in the next patch.
>>
>>
>>>
>>> >> The tablespace_path option appears entirely unused, and I don't know
>>> >> why that should be necessary here, either.
>>> >
>>> > This is to calculate the basepathlen. We need to exclude the
>>> tablespace location (or
>>> > base path) from the filename before it is sent to the client with
>>> sendFile call. I added
>>> > this option primarily to avoid performing string manipulation on
>>> filename to extract the
>>> > tablespace location and then calculate the basepathlen.
>>> >
>>> > Alternatively we can do it by extracting the base path from the
>>> received filename. What
>>> > do you suggest?
>>>
>>> I don't think the server needs any information from the client in
>>> order to be able to exclude the tablespace location from the pathname.
>>> Whatever it needs to know, it should be able to figure out, just as it
>>> would in a non-parallel backup.
>>>
>>> --
>>> Robert Haas
>>> EnterpriseDB: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>
>>
> I have updated the replication grammar with some new rules to
> differentiate the options production
> for base backup and newly added commands.
>
> I have also created a separate patch to include the documentation changes.
> The current syntax is as below:
>
> - START_BACKUP [ LABEL 'label' ] [ PROGRESS ] [ FAST ] [ TABLESPACE_MAP ]
> - STOP_BACKUP [ LABEL 'label' ] [ WAL ] [ NOWAIT ]
> - SEND_BACKUP_FILELIST
> - SEND_BACKUP_FILES ( 'FILE' [, ...] ) [ MAX_RATE rate ] [
> NOVERIFY_CHECKSUMS ] [ START_WAL_LOCATION ]
>
>
Sorry, I sent the wrong patches. Please see the correct version of the
patches (_v6).

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

On Wed, Nov 13, 2019 at 7:04 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

>
> Sorry, I sent the wrong patches. Please see the correct version of the
> patches (_v6).
>

Review comments on these patches:

1.
+ XLogRecPtr wal_location;

Looking at the other field names in basebackup_options structure, let's use
wallocation instead. Or better startwallocation to be precise.

2.
+ int32 size;

Should we use size_t here?

3.
I am still not sure why we need SEND_BACKUP_FILELIST as a separate command.
Can't we return the file list with START_BACKUP itself?

4.
+ else if (
+#ifndef WIN32
+ S_ISLNK(statbuf.st_mode)
+#else
+ pgwin32_is_junction(pathbuf)
+#endif
+ )
+ {
+ /*
+ * If symlink, write it as a directory. file symlinks only
allowed
+ * in pg_tblspc
+ */
+ statbuf.st_mode = S_IFDIR | pg_dir_create_mode;
+ _tarWriteHeader(pathbuf + basepathlen + 1, NULL, &statbuf,
false);
+ }

In normal backup mode, we skip the special file which is not a regular file
or
a directory or a symlink inside pg_tblspc. But in your patch, above code,
treats it as a directory. Should parallel backup too skip such special
files?

5.
Please keep header file inclusions in alphabetical order in basebackup.c and
pg_basebackup.c

6.
+ /*
+ * build query in form of: SEND_BACKUP_FILES ('base/1/1245/32683',
+ * 'base/1/1245/32683', ...) [options]
+ */

Please update these comments as we fetch one file at a time.

7.
+backup_file:
+ SCONST { $$ = (Node *)
makeString($1); }
+ ;
+

Instead of having this rule with only one constant terminal, we can use
SCONST directly in backup_files_list. However, I don't see any issue with
this approach either, just trying to reduce the rules.

8.
Please indent code within 80 char limit at all applicable places.

9.
Please fix following typos:

identifing => identifying
optionaly => optionally
structre => structure
progrsss => progress
Retrive => Retrieve
direcotries => directories

=====

The other mail thread related to backup manifest [1], is creating a
backup_manifest file and sends that to the client which has optional
checksum and other details including filename, file size, mtime, etc.
There is a patch on the same thread which is then validating the backup too.

Since this patch too gets a file list from the server and has similar
details (except checksum), can somehow parallel backup use the
backup-manifest
infrastructure from that patch?

When the parallel backup is in use, will there be a backup_manifest file
created too? I am just visualizing what will be the scenario when both these
features are checked-in.

[1]
https://www.postgresql.org/message-id/CA+TgmoZV8dw1H2bzZ9xkKwdrk8+XYa+DC9H=F7heO2zna5T6qg@mail.gmail.com

> --
> Asif Rehman
> Highgo Software (Canada/China/Pakistan)
> URL : www.highgo.ca
>
>
Thanks
--
Jeevan Chalke
Associate Database Architect & Team Lead, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

On Wed, Nov 27, 2019 at 3:38 AM Jeevan Chalke
<jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
> I am still not sure why we need SEND_BACKUP_FILELIST as a separate command.
> Can't we return the file list with START_BACKUP itself?

I had the same thought, but I think it's better to keep them separate.
Somebody might want to use the SEND_BACKUP_FILELIST command for
something other than a backup (I actually think it should be called
just SEND_FILE_LIST). Somebody might want to start a backup without
getting a file list because they're going to copy the files at the FS
level. Somebody might want to get a list of files to process after
somebody else has started the backup on another connection. Or maybe
nobody wants to do any of those things, but it doesn't seem to cost us
much of anything to split the commands, so I think we should.

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

On Wed, Nov 27, 2019 at 1:38 PM Jeevan Chalke <
jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:

>
>
> On Wed, Nov 13, 2019 at 7:04 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
> wrote:
>
>>
>> Sorry, I sent the wrong patches. Please see the correct version of the
>> patches (_v6).
>>
>
> Review comments on these patches:
>
> 1.
> + XLogRecPtr wal_location;
>
> Looking at the other field names in basebackup_options structure, let's use
> wallocation instead. Or better startwallocation to be precise.
>
> 2.
> + int32 size;
>
> Should we use size_t here?
>
> 3.
> I am still not sure why we need SEND_BACKUP_FILELIST as a separate command.
> Can't we return the file list with START_BACKUP itself?
>
> 4.
> + else if (
> +#ifndef WIN32
> + S_ISLNK(statbuf.st_mode)
> +#else
> + pgwin32_is_junction(pathbuf)
> +#endif
> + )
> + {
> + /*
> + * If symlink, write it as a directory. file symlinks only
> allowed
> + * in pg_tblspc
> + */
> + statbuf.st_mode = S_IFDIR | pg_dir_create_mode;
> + _tarWriteHeader(pathbuf + basepathlen + 1, NULL, &statbuf,
> false);
> + }
>
> In normal backup mode, we skip the special file which is not a regular
> file or
> a directory or a symlink inside pg_tblspc. But in your patch, above code,
> treats it as a directory. Should parallel backup too skip such special
> files?
>

Yeah going through the code again, I found it a little bit inconsistent. In
fact
SendBackupFiles function is supposed to send the files that were requested
of
it. However, currently is performing these tasks:

1) If the requested file were to be a directory, it will return a TAR
directory entry.
2) If the requested files were to be symlink inside pg_tblspc, it will
return the link path.
3) and as you pointed out above, if the requested files were a symlink
outside pg_tblspc
and inside PGDATA then it will return TAR directory entry.

I think that this function should not take care of any of the above.
Instead, it should
be the client (i.e. pg_basebackup) managing it. The SendBackupFiles should
only send the
regular files and ignore the request of any other kind, be it a directory
or symlink.

Any thoughts?

> 5.
> Please keep header file inclusions in alphabetical order in basebackup.c
> and
> pg_basebackup.c
>
> 6.
> + /*
> + * build query in form of: SEND_BACKUP_FILES ('base/1/1245/32683',
> + * 'base/1/1245/32683', ...) [options]
> + */
>
> Please update these comments as we fetch one file at a time.
>
> 7.
> +backup_file:
> + SCONST { $$ = (Node *)
> makeString($1); }
> + ;
> +
>
> Instead of having this rule with only one constant terminal, we can use
> SCONST directly in backup_files_list. However, I don't see any issue with
> this approach either, just trying to reduce the rules.
>
> 8.
> Please indent code within 80 char limit at all applicable places.
>
> 9.
> Please fix following typos:
>
> identifing => identifying
> optionaly => optionally
> structre => structure
> progrsss => progress
> Retrive => Retrieve
> direcotries => directories
>
>
> =====
>
> The other mail thread related to backup manifest [1], is creating a
> backup_manifest file and sends that to the client which has optional
> checksum and other details including filename, file size, mtime, etc.
> There is a patch on the same thread which is then validating the backup
> too.
>
> Since this patch too gets a file list from the server and has similar
> details (except checksum), can somehow parallel backup use the
> backup-manifest
> infrastructure from that patch?
>

This was discussed earlier in the thread, and as Robert suggested, it would
complicate the
code to no real benefit.

> When the parallel backup is in use, will there be a backup_manifest file
> created too? I am just visualizing what will be the scenario when both
> these
> features are checked-in.
>

Yes, I think it should. Since the full backup will have a manifest file,
there is no
reason for parallel backup to not support it.

I'll share the updated patch in the next couple of days.

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

On Thu, Nov 28, 2019 at 12:57 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Wed, Nov 27, 2019 at 3:38 AM Jeevan Chalke
> <jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
> > I am still not sure why we need SEND_BACKUP_FILELIST as a separate
> command.
> > Can't we return the file list with START_BACKUP itself?
>
> I had the same thought, but I think it's better to keep them separate.
> Somebody might want to use the SEND_BACKUP_FILELIST command for
> something other than a backup (I actually think it should be called
> just SEND_FILE_LIST)

Sure. Thanks for the recommendation. To keep the function names in sync, I
intend to do following the
following renamings:
- SEND_BACKUP_FILES --> SEND_FILES
- SEND_BACKUP_FILELIST --> SEND_FILE_LIST

. Somebody might want to start a backup without
> getting a file list because they're going to copy the files at the FS
> level. Somebody might want to get a list of files to process after
> somebody else has started the backup on another connection. Or maybe
> nobody wants to do any of those things, but it doesn't seem to cost us
> much of anything to split the commands, so I think we should.
>

+1

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

On Tue, Dec 10, 2019 at 7:34 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

>
>
> On Thu, Nov 28, 2019 at 12:57 AM Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
>
>> On Wed, Nov 27, 2019 at 3:38 AM Jeevan Chalke
>> <jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>> > I am still not sure why we need SEND_BACKUP_FILELIST as a separate
>> command.
>> > Can't we return the file list with START_BACKUP itself?
>>
>> I had the same thought, but I think it's better to keep them separate.
>> Somebody might want to use the SEND_BACKUP_FILELIST command for
>> something other than a backup (I actually think it should be called
>> just SEND_FILE_LIST)
>
>
> Sure. Thanks for the recommendation. To keep the function names in sync, I
> intend to do following the
> following renamings:
> - SEND_BACKUP_FILES --> SEND_FILES
> - SEND_BACKUP_FILELIST --> SEND_FILE_LIST
>
> . Somebody might want to start a backup without
>> getting a file list because they're going to copy the files at the FS
>> level. Somebody might want to get a list of files to process after
>> somebody else has started the backup on another connection. Or maybe
>> nobody wants to do any of those things, but it doesn't seem to cost us
>> much of anything to split the commands, so I think we should.
>>
>
> +1
>
>
I have updated the patches (v7 attached) and have taken care of all issues
pointed by Jeevan, additionally
ran the pgindent on each patch. Furthermore, Command names have been
renamed as suggested and I
have simplified the SendFiles function. Client can only request the regular
files, any other kind such as
directories or symlinks will be skipped, the client will be responsible for
taking care of such.

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

On Thu, Dec 12, 2019 at 10:20 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:
> I have updated the patches (v7 attached) and have taken care of all issues pointed by Jeevan, additionally
> ran the pgindent on each patch. Furthermore, Command names have been renamed as suggested and I
> have simplified the SendFiles function. Client can only request the regular files, any other kind such as
> directories or symlinks will be skipped, the client will be responsible for taking care of such.

Hi,

Patch 0001 of this series conflicts with my recent commit
303640199d0436c5e7acdf50b837a027b5726594; that commit was actually
inspired by some previous study of 0001. That being said, I think 0001
has the wrong idea. There's no reason that I can see why it should be
correct to remove the PG_ENSURE_ERROR_CLEANUP calls from
perform_base_backup(). It's true that if we register a long-lived
before_shmem_exit hook, then the backup will get cleaned up even
without the PG_ENSURE_ERROR_CLEANUP block, but there's also the
question of the warning message. I think that our goal should be to
emit the warning message about a backup being stopped too early if the
user uses either pg_start_backup() or the new START_BACKUP command and
does not end the backup with either pg_stop_backup() or the new
STOP_BACKUP command -- but not if a single command that both starts
and ends a backup, like BASE_BACKUP, is interrupted. To accomplish
that goal in the wake of 303640199d0436c5e7acdf50b837a027b5726594, we
need to temporarily register do_pg_abort_backup() as a
before_shmem_exit() handler using PG_ENSURE_ERROR_CLEANUP() during
commands like BASE_BACKUP() -- and for things like pg_start_backup()
or the new START_BACKUP command, we just need to add a single call to
register_persistent_abort_backup_handler().

So I think you can drop 0001, and then in the patch that actually
introduces START_BACKUP, add the call to
register_persistent_abort_backup_handler() before calling
do_pg_start_backup(). Also in that patch, also adjust the warning text
that do_pg_abort_backup() emits to be more generic e.g. "aborting
backup due to backend exiting while a non-exclusive backup is in
progress".

0003 creates three new functions, moving code from
do_pg_start_backup() to a new function collectTablespaces() and from
perform_base_backup() to new functions setup_throttle() and
include_wal_files(). I'm skeptical about all of these changes. One
general nitpick is that the way these function names are capitalized
and punctuated does not seem to have been chosen very consistently;
how about name_like_this() throughout? A bit more substantively:

- collectTablespaces() is factored out of do_pg_start_backup() so that
it can also be used by SendFileList(), but that means that a client is
going to invoke START_BACKUP, indirectly calling collectTablespaces(),
and then immediately afterward the client is probably going to call
SEND_FILE_LIST, which will again call collectTablespaces(). That does
not appear to be super-great. For one thing, it's duplicate work,
although because SendFileList() is going to pass infotbssize as false,
it's not a lot of duplicated work. Also, what happens if the two calls
to collectTablespaces() return different answers due to concurrent
CREATE/DROP TABLESPACE commands? Maybe it would all work out fine, but
it seems like there is at least the possibility of bugs if different
parts of the backup have different notions of what tablespaces exist.

- setup_throttle() is factored out of perform_base_backup() so that it
can be called in StartBackup() and StopBackup() and SendFiles(). This
seems extremely odd. Why does it make any sense to give the user an
option to activate throttling when *ending* a backup? Why does it make
sense to give the user a chance to enable throttling *both* at the
startup of a backup *and also* for each individual file. If we're
going to support throttling here, it seems like it should be either a
backup-level property or a file-level property, not both.

- include_wal_files() is factored out of perform_base_backup() so that
it can be called by StopBackup(). This seems like a poor design
decision. The idea behind the BASE_BACKUP command is that you run that
one command, and the server sends you everything. The idea in this new
way of doing business is that the client requests the individual files
it wants -- except for the WAL files, which are for some reason not
requested individually but sent all together as part of the
STOP_BACKUP response. It seems like it would be more consistent if the
client were to decide which WAL files it needs and request them one by
one, just as we do with other files.

I think there's a common theme to all of these complaints, which is
that you haven't done enough to move things that are the
responsibility of the backend in the BASE_BACKUP model to the frontend
in this model. I started wondering, for example, whether it might not
be better to have the client rather than the server construct the
tablespace_map file. After all, the client needs to get the list of
files anyway (hence SEND_FILE_LIST) and if it's got that then it knows
almost enough to construct the tablespace map. The only additional
thing it needs is the full pathname to which the link points. But, it
seems that we could fairly easily extend SEND_FILE_LIST to send, for
files that are symbolic links, the target of the link, using a new
column. Or alternatively, using a separate command, so that instead of
just sending a single SEND_FILE_LIST command, the client might first
ask for a tablespace list and then might ask for a list of files
within each tablespace (e.g. LIST_TABLESPACES, then LIST_FILES <oid>
for each tablespace, with 0 for the main tablespace, perhaps). I'm not
sure which way is better.

Similarly, for throttling, I have a hard time understanding how what
you've got here is going to work reasonably. It looks like each client
is just going to request whatever MAX_RATE the user specifies, but the
result of that will be that the actual transfer rate is probably a
multiple of the specified rate, approximately equal to the specified
rate times the number of clients. That's probably not what the user
wants. You could take the specified rate and divide it by the number
of workers, but limiting each of 4 workers to a quarter of the rate
will probably lead to a combined rate of less than than the specified
rate, because if one worker doesn't use all of the bandwidth to which
it's entitled, or even exits earlier than the others, the other
workers don't get to go any faster as a result. Another problem is
that, in the current approach, throttling applies overall to the
entire backup, but in this approach, it is applied separately to each
SEND_FILE command. In the current approach, if one file finishes a
little faster or slower than anticipated, the next file in the tarball
will be sent a little slower or faster to compensate. But in this
approach, each SEND_FILES command is throttled separately, so this
property is lost. Furthermore, while BASEBACKUP sends data
continuously, this approach naturally involves pauses between
commands. If files are large, that won't matter much, but if they're
small and numerous, it will tend to cause the actual transfer rate to
be less than the throttling rate.

One potential way to solve this problem is... move it to the client
side. Instead of making it the server's job not to send data too fast,
make it the client's job not to receive data too fast. Let the server
backends write as fast as they want, and on the pg_basebackup side,
have the threads coordinate with each other so that they don't read
data faster than the configured rate. That's not quite the same thing,
though, because the server can get ahead by the size of the client's
receive buffers plus whatever data is on the wire. I don't know
whether that's a big enough problem to be worth caring about. If it
is, then I think we need some server infrastructure to "group
throttle" a group of cooperating backends.

A general comment about 0004 is that it seems like you've proceeded by
taking the code from perform_base_backup() and spreading it across
several different functions without, necessarily, as much thought as
is needed there. For instance, StartBackup() looks like just the
beginning of perform_base_backup(). But, why shouldn't it instead look
like pg_start_backup() -- in fact, a simplified version that only
handles the non-exclusive backup case? Is the extra stuff it's doing
really appropriate? I've already complained about the
tablespace-related stuff here and the throttling, but there's more.
Setting statrelpath here will probably break if somebody tries to use
SEND_FILES without first calling START_BACKUP. Sending the
backup_label file here is oddly asymmetric, because that's done by
pg_stop_backup(), not pg_start_backup(). And similarly, StopBackup()
looks like it's just the end of perform_base_backup(), but that's not
pretty strange-looking too. Again, I've already complained about
include_wal_files() being part of this, but there's also:

+ /* ... and pg_control after everything else. */

...which (1) is an odd thing to say when this is the first thing this
particular function is to send and (2) is another example of a sloppy
division of labor between client and server; apparently, the client is
supposed to know not to request pg_control, because the server is
going to send it unsolicited. There's no particular reason to have
this a special case. The client could just request it last. And then
the server code wouldn't need a special case, and you wouldn't have
this odd logic split between the client and the server.

Overall, I think this needs a lot more work. The overall idea's not
wrong, but there seem to be a very large number of details which, at
least to me, do not seem to be correct.

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

On Thu, Dec 19, 2019 at 10:47 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Thu, Dec 12, 2019 at 10:20 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
> wrote:
> > I have updated the patches (v7 attached) and have taken care of all
> issues pointed by Jeevan, additionally
> > ran the pgindent on each patch. Furthermore, Command names have been
> renamed as suggested and I
> > have simplified the SendFiles function. Client can only request the
> regular files, any other kind such as
> > directories or symlinks will be skipped, the client will be responsible
> for taking care of such.
>
> Hi,
>
> Patch 0001 of this series conflicts with my recent commit
> 303640199d0436c5e7acdf50b837a027b5726594; that commit was actually
> inspired by some previous study of 0001. That being said, I think 0001
> has the wrong idea. There's no reason that I can see why it should be
> correct to remove the PG_ENSURE_ERROR_CLEANUP calls from
> perform_base_backup(). It's true that if we register a long-lived
> before_shmem_exit hook, then the backup will get cleaned up even
> without the PG_ENSURE_ERROR_CLEANUP block, but there's also the
> question of the warning message. I think that our goal should be to
> emit the warning message about a backup being stopped too early if the
> user uses either pg_start_backup() or the new START_BACKUP command and
> does not end the backup with either pg_stop_backup() or the new
> STOP_BACKUP command -- but not if a single command that both starts
> and ends a backup, like BASE_BACKUP, is interrupted. To accomplish
> that goal in the wake of 303640199d0436c5e7acdf50b837a027b5726594, we
> need to temporarily register do_pg_abort_backup() as a
> before_shmem_exit() handler using PG_ENSURE_ERROR_CLEANUP() during
> commands like BASE_BACKUP() -- and for things like pg_start_backup()
> or the new START_BACKUP command, we just need to add a single call to
> register_persistent_abort_backup_handler().
>
> So I think you can drop 0001, and then in the patch that actually
> introduces START_BACKUP, add the call to
> register_persistent_abort_backup_handler() before calling
> do_pg_start_backup(). Also in that patch, also adjust the warning text
> that do_pg_abort_backup() emits to be more generic e.g. "aborting
> backup due to backend exiting while a non-exclusive backup is in
> progress".
>
> Sure. will do.

> 0003 creates three new functions, moving code from
> do_pg_start_backup() to a new function collectTablespaces() and from
> perform_base_backup() to new functions setup_throttle() and
> include_wal_files(). I'm skeptical about all of these changes. One
> general nitpick is that the way these function names are capitalized
> and punctuated does not seem to have been chosen very consistently;
> how about name_like_this() throughout? A bit more substantively:
>
> - collectTablespaces() is factored out of do_pg_start_backup() so that
> it can also be used by SendFileList(), but that means that a client is
> going to invoke START_BACKUP, indirectly calling collectTablespaces(),
> and then immediately afterward the client is probably going to call
> SEND_FILE_LIST, which will again call collectTablespaces(). That does
> not appear to be super-great. For one thing, it's duplicate work,
> although because SendFileList() is going to pass infotbssize as false,
> it's not a lot of duplicated work.

I'll remove this duplication by eliminating this call from START_BACKUP and
SEND_FILE_LIST functions. More about this is explained later in this email.

> Also, what happens if the two calls
> to collectTablespaces() return different answers due to concurrent
> CREATE/DROP TABLESPACE commands? Maybe it would all work out fine, but
> it seems like there is at least the possibility of bugs if different
> parts of the backup have different notions of what tablespaces exist.
>

The concurrent CREATE/DROP TABLESPACE commands, it can happen and will
be resolved by the WAL files collected for the backup. I don't think we
can do anything when objects are created or dropped in-between start and
stop backup. BASE_BACKUPalso relies on the WAL files to handle such a
scenario and does not error out when some relation files go away.

>
> - setup_throttle() is factored out of perform_base_backup() so that it
> can be called in StartBackup() and StopBackup() and SendFiles(). This
> seems extremely odd. Why does it make any sense to give the user an
> option to activate throttling when *ending* a backup? Why does it make
> sense to give the user a chance to enable throttling *both* at the
> startup of a backup *and also* for each individual file. If we're
> going to support throttling here, it seems like it should be either a
> backup-level property or a file-level property, not both.
>

It's a file-level property only. Throttle functionality relies on global
variables. StartBackup() and StopBackup() are calling setup_throttle
function to disable the throttling.

I should have been more explicit here by using -1 to setup_throttle,
Illustrating that throttling is disabled, instead of using 'opt->maxrate'.
(Although it defaults to -1 for these functions).

I'll remove the setup_throttle() call for both functions.

>
> - include_wal_files() is factored out of perform_base_backup() so that
> it can be called by StopBackup(). This seems like a poor design
> decision. The idea behind the BASE_BACKUP command is that you run that
> one command, and the server sends you everything. The idea in this new
> way of doing business is that the client requests the individual files
> it wants -- except for the WAL files, which are for some reason not
> requested individually but sent all together as part of the
> STOP_BACKUP response. It seems like it would be more consistent if the
> client were to decide which WAL files it needs and request them one by
> one, just as we do with other files.
>

As I understand you are suggesting to add another command to fetch the
list of WAL files which would be called by the client after executing stop
backup. Once the client gets that list, it starts requesting the WAL files
one
by one.

So I will add LIST_WAL_FILES command that will take start_lsn and end_lsn
as arguments and return the list of WAL files between these LSNs.

Something like this :
LIST_WAL_FILES 'start_lsn' 'end_lsn';

>
> I think there's a common theme to all of these complaints, which is
> that you haven't done enough to move things that are the
> responsibility of the backend in the BASE_BACKUP model to the frontend
> in this model. I started wondering, for example, whether it might not
> be better to have the client rather than the server construct the
> tablespace_map file. After all, the client needs to get the list of
> files anyway (hence SEND_FILE_LIST) and if it's got that then it knows
> almost enough to construct the tablespace map. The only additional
> thing it needs is the full pathname to which the link points. But, it
> seems that we could fairly easily extend SEND_FILE_LIST to send, for
> files that are symbolic links, the target of the link, using a new
> column. Or alternatively, using a separate command, so that instead of
> just sending a single SEND_FILE_LIST command, the client might first
> ask for a tablespace list and then might ask for a list of files
> within each tablespace (e.g. LIST_TABLESPACES, then LIST_FILES <oid>
> for each tablespace, with 0 for the main tablespace, perhaps). I'm not
> sure which way is better.
>

do_pg_start_backup is collecting the tablespace information anyway to
build the tablespace_map for BASE_BACKUP. So returning the same seemed
better than adding a new command for the same information. hence multiple
calls to the collectTablespaces() [to be renamed to collect_tablespaces].

tablespace_map can be constructed by the client, but then BASE_BACKUP
is returning it as part of the full backup. If clients in parallel mode
are to construct this themselves, these will seem like two different
approaches. Perhaps this should be done for BASE_BACKUP as
well?

I'll refactor the do_pg_start_backup function to remove the code related
to tablespace information collection (to collect_tablespaces) and
tablespace_map file creation, so that this function does not collect this
information unnecessarily. perform_base_backup function can collect and
send the tablespace information to the client and then the client can
construct the tablespace_map file.

I'll add a new command to fetch the list of tablespaces i.e.
LIST_TABLESPACES
which will return the tablespace information to the client for parallel
mode. And will refactor START_BACKUP and STOP_BACKUP commands,
so that they only do the specific job of putting the system in backup mode
or
out of it, nothing else.These commands should only return the start and end
LSN to the client.

>
> Similarly, for throttling, I have a hard time understanding how what
> you've got here is going to work reasonably. It looks like each client
> is just going to request whatever MAX_RATE the user specifies, but the
> result of that will be that the actual transfer rate is probably a
> multiple of the specified rate, approximately equal to the specified
> rate times the number of clients. That's probably not what the user
> wants. You could take the specified rate and divide it by the number
> of workers, but limiting each of 4 workers to a quarter of the rate
> will probably lead to a combined rate of less than than the specified
> rate, because if one worker doesn't use all of the bandwidth to which
> it's entitled, or even exits earlier than the others, the other
> workers don't get to go any faster as a result. Another problem is
> that, in the current approach, throttling applies overall to the
> entire backup, but in this approach, it is applied separately to each
> SEND_FILE command. In the current approach, if one file finishes a
> little faster or slower than anticipated, the next file in the tarball
> will be sent a little slower or faster to compensate. But in this
> approach, each SEND_FILES command is throttled separately, so this
> property is lost. Furthermore, while BASEBACKUP sends data
> continuously, this approach naturally involves pauses between
> commands. If files are large, that won't matter much, but if they're
> small and numerous, it will tend to cause the actual transfer rate to
> be less than the throttling rate.
>
> One potential way to solve this problem is... move it to the client
> side. Instead of making it the server's job not to send data too fast,
> make it the client's job not to receive data too fast. Let the server
> backends write as fast as they want, and on the pg_basebackup side,
> have the threads coordinate with each other so that they don't read
> data faster than the configured rate. That's not quite the same thing,
> though, because the server can get ahead by the size of the client's
> receive buffers plus whatever data is on the wire. I don't know
> whether that's a big enough problem to be worth caring about. If it
> is, then I think we need some server infrastructure to "group
> throttle" a group of cooperating backends.
>

That was a mistake in my code. maxrate should've been equally divided
amongst all threads. I agree that we should move this to the client-side.
When a thread exits, its share should also be equally divided amongst
the remaining threads (i.e. recalculate maxrate for each remaining thread).

Say we have 4 running threads with each allocation 25% of the bandwidth.
Thread 1 exits. We recalculate bandwidth and assign the remaining 3 threads
33.33% each. This solves one problem that you had identified. However,
it doesn't solve where one (or more) thread is not fully consuming their
allocated share. I'm not really sure how we can solve it properly.
Suggestions
are welcome.

>
> A general comment about 0004 is that it seems like you've proceeded by
> taking the code from perform_base_backup() and spreading it across
> several different functions without, necessarily, as much thought as
> is needed there. For instance, StartBackup() looks like just the
> beginning of perform_base_backup(). But, why shouldn't it instead look
> like pg_start_backup() -- in fact, a simplified version that only
> handles the non-exclusive backup case? Is the extra stuff it's doing
> really appropriate? I've already complained about the
> tablespace-related stuff here and the throttling, but there's more.
> Setting statrelpath here will probably break if somebody tries to use
> SEND_FILES without first calling START_BACKUP. Sending the
> backup_label file here is oddly asymmetric, because that's done by
> pg_stop_backup(), not pg_start_backup(). And similarly, StopBackup()
> looks like it's just the end of perform_base_backup(), but that's not
> pretty strange-looking too. Again, I've already complained about
> include_wal_files() being part of this, but there's also:
>
> + /* ... and pg_control after everything else. */
>
> ...which (1) is an odd thing to say when this is the first thing this
> particular function is to send and (2) is another example of a sloppy
> division of labor between client and server; apparently, the client is
> supposed to know not to request pg_control, because the server is
> going to send it unsolicited. There's no particular reason to have
> this a special case. The client could just request it last. And then
> the server code wouldn't need a special case, and you wouldn't have
> this odd logic split between the client and the server.
>
> Overall, I think this needs a lot more work. The overall idea's not
> wrong, but there seem to be a very large number of details which, at
> least to me, do not seem to be correct.
>
>

Thank you Robert for the detailed review. I really appreciate your insights
and very precise feedback.

After the changes suggested above, the design on a high level will look
something
like this:

=== SEQUENTIAL EXECUTION ===
START_BACKUP [LABEL | FAST]
- Starts backup on the server
- Returns the start LSN to client

LIST_TABLESPACES
- Sends a list of all tables spaces to client

Loops over LIST_TABLESPACES
- LIST_FILES [tablespace]
- Sends file list for the given tablespace
- Create a list of all files

=== PARALLEL EXECUTION ===
Thread loop until the list of files is exhausted
SEND_FILE <file(s)> [CHECKSUM | WAL_START_LOCATION]
- If the checksum is enabled then WAL_START_LOCATION is required.
- Can request server to send one or more files but we are requesting one at
a time
- Pick next file from list of files

- Threads sleep after the list is exhausted
- All threads are sleeping

=== SEQUENTIAL EXECUTION ===
STOP_BACKUP [NOWAIT]
- Stops backup mode
- Return end LSN

If --wal-method=fetch then
LIST_WAL_FILES 'start_lsn' 'end_lsn'
- Sends a list of WAL files between start LSN and end LSN

=== PARALLEL EXECUTION ===
Thread loop until the list of WAL files is exhausted
SEND_FILE <WAL file>
- Can request server to send one or more files but we are requesting one
WAL file at a time
- Pick next file from list of WAL files

- Threads terminate and set their status as completed/terminated

=== SEQUENTIAL EXECUTION ===
Cleanup

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

On Sat, Jan 4, 2020 at 11:53 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

>
>
> On Thu, Dec 19, 2019 at 10:47 PM Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
>
>> On Thu, Dec 12, 2019 at 10:20 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
>> wrote:
>> > I have updated the patches (v7 attached) and have taken care of all
>> issues pointed by Jeevan, additionally
>> > ran the pgindent on each patch. Furthermore, Command names have been
>> renamed as suggested and I
>> > have simplified the SendFiles function. Client can only request the
>> regular files, any other kind such as
>> > directories or symlinks will be skipped, the client will be responsible
>> for taking care of such.
>>
>> Hi,
>>
>> Patch 0001 of this series conflicts with my recent commit
>> 303640199d0436c5e7acdf50b837a027b5726594; that commit was actually
>> inspired by some previous study of 0001. That being said, I think 0001
>> has the wrong idea. There's no reason that I can see why it should be
>> correct to remove the PG_ENSURE_ERROR_CLEANUP calls from
>> perform_base_backup(). It's true that if we register a long-lived
>> before_shmem_exit hook, then the backup will get cleaned up even
>> without the PG_ENSURE_ERROR_CLEANUP block, but there's also the
>> question of the warning message. I think that our goal should be to
>> emit the warning message about a backup being stopped too early if the
>> user uses either pg_start_backup() or the new START_BACKUP command and
>> does not end the backup with either pg_stop_backup() or the new
>> STOP_BACKUP command -- but not if a single command that both starts
>> and ends a backup, like BASE_BACKUP, is interrupted. To accomplish
>> that goal in the wake of 303640199d0436c5e7acdf50b837a027b5726594, we
>> need to temporarily register do_pg_abort_backup() as a
>> before_shmem_exit() handler using PG_ENSURE_ERROR_CLEANUP() during
>> commands like BASE_BACKUP() -- and for things like pg_start_backup()
>> or the new START_BACKUP command, we just need to add a single call to
>> register_persistent_abort_backup_handler().
>>
>> So I think you can drop 0001, and then in the patch that actually
>> introduces START_BACKUP, add the call to
>> register_persistent_abort_backup_handler() before calling
>> do_pg_start_backup(). Also in that patch, also adjust the warning text
>> that do_pg_abort_backup() emits to be more generic e.g. "aborting
>> backup due to backend exiting while a non-exclusive backup is in
>> progress".
>>
>> Sure. will do.
>
>
>> 0003 creates three new functions, moving code from
>> do_pg_start_backup() to a new function collectTablespaces() and from
>> perform_base_backup() to new functions setup_throttle() and
>> include_wal_files(). I'm skeptical about all of these changes. One
>> general nitpick is that the way these function names are capitalized
>> and punctuated does not seem to have been chosen very consistently;
>> how about name_like_this() throughout? A bit more substantively:
>>
>> - collectTablespaces() is factored out of do_pg_start_backup() so that
>> it can also be used by SendFileList(), but that means that a client is
>> going to invoke START_BACKUP, indirectly calling collectTablespaces(),
>> and then immediately afterward the client is probably going to call
>> SEND_FILE_LIST, which will again call collectTablespaces(). That does
>> not appear to be super-great. For one thing, it's duplicate work,
>> although because SendFileList() is going to pass infotbssize as false,
>> it's not a lot of duplicated work.
>
>
> I'll remove this duplication by eliminating this call from START_BACKUP and
> SEND_FILE_LIST functions. More about this is explained later in this email.
>
>
>> Also, what happens if the two calls
>> to collectTablespaces() return different answers due to concurrent
>> CREATE/DROP TABLESPACE commands? Maybe it would all work out fine, but
>> it seems like there is at least the possibility of bugs if different
>> parts of the backup have different notions of what tablespaces exist.
>>
>
> The concurrent CREATE/DROP TABLESPACE commands, it can happen and will
> be resolved by the WAL files collected for the backup. I don't think we
> can do anything when objects are created or dropped in-between start and
> stop backup. BASE_BACKUPalso relies on the WAL files to handle such a
> scenario and does not error out when some relation files go away.
>
>
>>
>> - setup_throttle() is factored out of perform_base_backup() so that it
>> can be called in StartBackup() and StopBackup() and SendFiles(). This
>> seems extremely odd. Why does it make any sense to give the user an
>> option to activate throttling when *ending* a backup? Why does it make
>> sense to give the user a chance to enable throttling *both* at the
>> startup of a backup *and also* for each individual file. If we're
>> going to support throttling here, it seems like it should be either a
>> backup-level property or a file-level property, not both.
>>
>
> It's a file-level property only. Throttle functionality relies on global
> variables. StartBackup() and StopBackup() are calling setup_throttle
> function to disable the throttling.
>
> I should have been more explicit here by using -1 to setup_throttle,
> Illustrating that throttling is disabled, instead of using 'opt->maxrate'.
> (Although it defaults to -1 for these functions).
>
> I'll remove the setup_throttle() call for both functions.
>
>
>>
>> - include_wal_files() is factored out of perform_base_backup() so that
>> it can be called by StopBackup(). This seems like a poor design
>> decision. The idea behind the BASE_BACKUP command is that you run that
>> one command, and the server sends you everything. The idea in this new
>> way of doing business is that the client requests the individual files
>> it wants -- except for the WAL files, which are for some reason not
>> requested individually but sent all together as part of the
>> STOP_BACKUP response. It seems like it would be more consistent if the
>> client were to decide which WAL files it needs and request them one by
>> one, just as we do with other files.
>>
>
> As I understand you are suggesting to add another command to fetch the
> list of WAL files which would be called by the client after executing stop
> backup. Once the client gets that list, it starts requesting the WAL files
> one
> by one.
>
> So I will add LIST_WAL_FILES command that will take start_lsn and end_lsn
> as arguments and return the list of WAL files between these LSNs.
>
> Something like this :
> LIST_WAL_FILES 'start_lsn' 'end_lsn';
>
>
>>
>> I think there's a common theme to all of these complaints, which is
>> that you haven't done enough to move things that are the
>> responsibility of the backend in the BASE_BACKUP model to the frontend
>> in this model. I started wondering, for example, whether it might not
>> be better to have the client rather than the server construct the
>> tablespace_map file. After all, the client needs to get the list of
>> files anyway (hence SEND_FILE_LIST) and if it's got that then it knows
>> almost enough to construct the tablespace map. The only additional
>> thing it needs is the full pathname to which the link points. But, it
>> seems that we could fairly easily extend SEND_FILE_LIST to send, for
>> files that are symbolic links, the target of the link, using a new
>> column. Or alternatively, using a separate command, so that instead of
>> just sending a single SEND_FILE_LIST command, the client might first
>> ask for a tablespace list and then might ask for a list of files
>> within each tablespace (e.g. LIST_TABLESPACES, then LIST_FILES <oid>
>> for each tablespace, with 0 for the main tablespace, perhaps). I'm not
>> sure which way is better.
>>
>
> do_pg_start_backup is collecting the tablespace information anyway to
> build the tablespace_map for BASE_BACKUP. So returning the same seemed
> better than adding a new command for the same information. hence multiple
> calls to the collectTablespaces() [to be renamed to collect_tablespaces].
>
> tablespace_map can be constructed by the client, but then BASE_BACKUP
> is returning it as part of the full backup. If clients in parallel mode
> are to construct this themselves, these will seem like two different
> approaches. Perhaps this should be done for BASE_BACKUP as
> well?
>
> I'll refactor the do_pg_start_backup function to remove the code related
> to tablespace information collection (to collect_tablespaces) and
> tablespace_map file creation, so that this function does not collect this
> information unnecessarily. perform_base_backup function can collect and
> send the tablespace information to the client and then the client can
> construct the tablespace_map file.
>
> I'll add a new command to fetch the list of tablespaces i.e.
> LIST_TABLESPACES
> which will return the tablespace information to the client for parallel
> mode. And will refactor START_BACKUP and STOP_BACKUP commands,
> so that they only do the specific job of putting the system in backup mode
> or
> out of it, nothing else.These commands should only return the start and end
> LSN to the client.
>
>
>
>>
>> Similarly, for throttling, I have a hard time understanding how what
>> you've got here is going to work reasonably. It looks like each client
>> is just going to request whatever MAX_RATE the user specifies, but the
>> result of that will be that the actual transfer rate is probably a
>> multiple of the specified rate, approximately equal to the specified
>> rate times the number of clients. That's probably not what the user
>> wants. You could take the specified rate and divide it by the number
>> of workers, but limiting each of 4 workers to a quarter of the rate
>> will probably lead to a combined rate of less than than the specified
>> rate, because if one worker doesn't use all of the bandwidth to which
>> it's entitled, or even exits earlier than the others, the other
>> workers don't get to go any faster as a result. Another problem is
>> that, in the current approach, throttling applies overall to the
>> entire backup, but in this approach, it is applied separately to each
>> SEND_FILE command. In the current approach, if one file finishes a
>> little faster or slower than anticipated, the next file in the tarball
>> will be sent a little slower or faster to compensate. But in this
>> approach, each SEND_FILES command is throttled separately, so this
>> property is lost. Furthermore, while BASEBACKUP sends data
>> continuously, this approach naturally involves pauses between
>> commands. If files are large, that won't matter much, but if they're
>> small and numerous, it will tend to cause the actual transfer rate to
>> be less than the throttling rate.
>>
>> One potential way to solve this problem is... move it to the client
>> side. Instead of making it the server's job not to send data too fast,
>> make it the client's job not to receive data too fast. Let the server
>> backends write as fast as they want, and on the pg_basebackup side,
>> have the threads coordinate with each other so that they don't read
>> data faster than the configured rate. That's not quite the same thing,
>> though, because the server can get ahead by the size of the client's
>> receive buffers plus whatever data is on the wire. I don't know
>> whether that's a big enough problem to be worth caring about. If it
>> is, then I think we need some server infrastructure to "group
>> throttle" a group of cooperating backends.
>>
>
> That was a mistake in my code. maxrate should've been equally divided
> amongst all threads. I agree that we should move this to the client-side.
> When a thread exits, its share should also be equally divided amongst
> the remaining threads (i.e. recalculate maxrate for each remaining
> thread).
>
> Say we have 4 running threads with each allocation 25% of the bandwidth.
> Thread 1 exits. We recalculate bandwidth and assign the remaining 3 threads
> 33.33% each. This solves one problem that you had identified. However,
> it doesn't solve where one (or more) thread is not fully consuming their
> allocated share. I'm not really sure how we can solve it properly.
> Suggestions
> are welcome.
>
>
>>
>> A general comment about 0004 is that it seems like you've proceeded by
>> taking the code from perform_base_backup() and spreading it across
>> several different functions without, necessarily, as much thought as
>> is needed there. For instance, StartBackup() looks like just the
>> beginning of perform_base_backup(). But, why shouldn't it instead look
>> like pg_start_backup() -- in fact, a simplified version that only
>> handles the non-exclusive backup case? Is the extra stuff it's doing
>> really appropriate? I've already complained about the
>> tablespace-related stuff here and the throttling, but there's more.
>> Setting statrelpath here will probably break if somebody tries to use
>> SEND_FILES without first calling START_BACKUP. Sending the
>> backup_label file here is oddly asymmetric, because that's done by
>> pg_stop_backup(), not pg_start_backup(). And similarly, StopBackup()
>> looks like it's just the end of perform_base_backup(), but that's not
>> pretty strange-looking too. Again, I've already complained about
>> include_wal_files() being part of this, but there's also:
>>
>> + /* ... and pg_control after everything else. */
>>
>> ...which (1) is an odd thing to say when this is the first thing this
>> particular function is to send and (2) is another example of a sloppy
>> division of labor between client and server; apparently, the client is
>> supposed to know not to request pg_control, because the server is
>> going to send it unsolicited. There's no particular reason to have
>> this a special case. The client could just request it last. And then
>> the server code wouldn't need a special case, and you wouldn't have
>> this odd logic split between the client and the server.
>>
>> Overall, I think this needs a lot more work. The overall idea's not
>> wrong, but there seem to be a very large number of details which, at
>> least to me, do not seem to be correct.
>>
>>
>
> Thank you Robert for the detailed review. I really appreciate your insights
> and very precise feedback.
>
> After the changes suggested above, the design on a high level will look
> something
> like this:
>
> === SEQUENTIAL EXECUTION ===
> START_BACKUP [LABEL | FAST]
> - Starts backup on the server
> - Returns the start LSN to client
>
> LIST_TABLESPACES
> - Sends a list of all tables spaces to client
>
> Loops over LIST_TABLESPACES
> - LIST_FILES [tablespace]
> - Sends file list for the given tablespace
> - Create a list of all files
>
> === PARALLEL EXECUTION ===
> Thread loop until the list of files is exhausted
> SEND_FILE <file(s)> [CHECKSUM | WAL_START_LOCATION]
> - If the checksum is enabled then WAL_START_LOCATION is required.
> - Can request server to send one or more files but we are requesting one
> at a time
> - Pick next file from list of files
>
> - Threads sleep after the list is exhausted
> - All threads are sleeping
>
> === SEQUENTIAL EXECUTION ===
> STOP_BACKUP [NOWAIT]
> - Stops backup mode
> - Return end LSN
>
> If --wal-method=fetch then
> LIST_WAL_FILES 'start_lsn' 'end_lsn'
> - Sends a list of WAL files between start LSN and end LSN
>
> === PARALLEL EXECUTION ===
> Thread loop until the list of WAL files is exhausted
> SEND_FILE <WAL file>
> - Can request server to send one or more files but we are requesting one
> WAL file at a time
> - Pick next file from list of WAL files
>
> - Threads terminate and set their status as completed/terminated
>
> === SEQUENTIAL EXECUTION ===
> Cleanup
>
>
>
>
Here are the the updated patches, taking care of the issues pointed
earlier. This patch adds the following commands (with specified option):

START_BACKUP [LABEL '<label>'] [FAST]
STOP_BACKUP [NOWAIT]
LIST_TABLESPACES [PROGRESS]
LIST_FILES [TABLESPACE]
LIST_WAL_FILES [START_WAL_LOCATION 'X/X'] [END_WAL_LOCATION 'X/X']
SEND_FILES '(' FILE, FILE... ')' [START_WAL_LOCATION 'X/X']
[NOVERIFY_CHECKSUMS]

Parallel backup is not making any use of tablespace map, so I have
removed that option from the above commands. There is a patch pending
to remove the exclusive backup; we can further refactor the
do_pg_start_backup
function at that time, to remove the tablespace information and move the
creation of tablespace_map file to the client.

I have disabled the maxrate option for parallel backup. I intend to send
out a separate patch for it. Robert previously suggested to implement
throttling on the client-side. I found the original email thread [1]
where throttling was proposed and added to the server. In that thread,
it was originally implemented on the client-side, but per many suggestions,
it was moved to server-side.

So, I have a few suggestions on how we can implement this:

1- have another option for pg_basebackup (i.e. per-worker-maxrate) where
the user could choose the bandwidth allocation for each worker. This
approach
can be implemented on the client-side as well as on the server-side.

2- have the maxrate, be divided among workers equally at first. and the
let the main thread keep adjusting it whenever one of the workers finishes.
I believe this would only be possible if we handle throttling on the client.
Also, as I understand it, implementing this will introduce additional mutex
for handling of bandwidth consumption data so that rate may be adjusted
according to data received by threads.

[1]
https://www.postgresql.org/message-id/flat/521B4B29.20009%402ndquadrant.com#189bf840c87de5908c0b4467d31b50af

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

Hi Asif,

On Thu, Jan 30, 2020 at 7:10 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

>
> Here are the the updated patches, taking care of the issues pointed
> earlier. This patch adds the following commands (with specified option):
>
> START_BACKUP [LABEL '<label>'] [FAST]
> STOP_BACKUP [NOWAIT]
> LIST_TABLESPACES [PROGRESS]
> LIST_FILES [TABLESPACE]
> LIST_WAL_FILES [START_WAL_LOCATION 'X/X'] [END_WAL_LOCATION 'X/X']
> SEND_FILES '(' FILE, FILE... ')' [START_WAL_LOCATION 'X/X']
> [NOVERIFY_CHECKSUMS]
>
>
> Parallel backup is not making any use of tablespace map, so I have
> removed that option from the above commands. There is a patch pending
> to remove the exclusive backup; we can further refactor the
> do_pg_start_backup
> function at that time, to remove the tablespace information and move the
> creation of tablespace_map file to the client.
>
>
> I have disabled the maxrate option for parallel backup. I intend to send
> out a separate patch for it. Robert previously suggested to implement
> throttling on the client-side. I found the original email thread [1]
> where throttling was proposed and added to the server. In that thread,
> it was originally implemented on the client-side, but per many suggestions,
> it was moved to server-side.
>
> So, I have a few suggestions on how we can implement this:
>
> 1- have another option for pg_basebackup (i.e. per-worker-maxrate) where
> the user could choose the bandwidth allocation for each worker. This
> approach
> can be implemented on the client-side as well as on the server-side.
>
> 2- have the maxrate, be divided among workers equally at first. and the
> let the main thread keep adjusting it whenever one of the workers finishes.
> I believe this would only be possible if we handle throttling on the
> client.
> Also, as I understand it, implementing this will introduce additional mutex
> for handling of bandwidth consumption data so that rate may be adjusted
> according to data received by threads.
>
> [1]
> https://www.postgresql.org/message-id/flat/521B4B29.20009%402ndquadrant.com#189bf840c87de5908c0b4467d31b50af
>
> --
> Asif Rehman
> Highgo Software (Canada/China/Pakistan)
> URL : www.highgo.ca
>
>

The latest changes look good to me. However, the patch set is missing the
documentation.
Please add those.

Thanks

--
Jeevan Chalke
Associate Database Architect & Team Lead, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Thanks Jeevan. Here is the documentation patch.

On Mon, Feb 10, 2020 at 6:49 PM Jeevan Chalke <
jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:

> Hi Asif,
>
> On Thu, Jan 30, 2020 at 7:10 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
> wrote:
>
>>
>> Here are the the updated patches, taking care of the issues pointed
>> earlier. This patch adds the following commands (with specified option):
>>
>> START_BACKUP [LABEL '<label>'] [FAST]
>> STOP_BACKUP [NOWAIT]
>> LIST_TABLESPACES [PROGRESS]
>> LIST_FILES [TABLESPACE]
>> LIST_WAL_FILES [START_WAL_LOCATION 'X/X'] [END_WAL_LOCATION 'X/X']
>> SEND_FILES '(' FILE, FILE... ')' [START_WAL_LOCATION 'X/X']
>> [NOVERIFY_CHECKSUMS]
>>
>>
>> Parallel backup is not making any use of tablespace map, so I have
>> removed that option from the above commands. There is a patch pending
>> to remove the exclusive backup; we can further refactor the
>> do_pg_start_backup
>> function at that time, to remove the tablespace information and move the
>> creation of tablespace_map file to the client.
>>
>>
>> I have disabled the maxrate option for parallel backup. I intend to send
>> out a separate patch for it. Robert previously suggested to implement
>> throttling on the client-side. I found the original email thread [1]
>> where throttling was proposed and added to the server. In that thread,
>> it was originally implemented on the client-side, but per many
>> suggestions,
>> it was moved to server-side.
>>
>> So, I have a few suggestions on how we can implement this:
>>
>> 1- have another option for pg_basebackup (i.e. per-worker-maxrate) where
>> the user could choose the bandwidth allocation for each worker. This
>> approach
>> can be implemented on the client-side as well as on the server-side.
>>
>> 2- have the maxrate, be divided among workers equally at first. and the
>> let the main thread keep adjusting it whenever one of the workers
>> finishes.
>> I believe this would only be possible if we handle throttling on the
>> client.
>> Also, as I understand it, implementing this will introduce additional
>> mutex
>> for handling of bandwidth consumption data so that rate may be adjusted
>> according to data received by threads.
>>
>> [1]
>> https://www.postgresql.org/message-id/flat/521B4B29.20009%402ndquadrant.com#189bf840c87de5908c0b4467d31b50af
>>
>> --
>> Asif Rehman
>> Highgo Software (Canada/China/Pakistan)
>> URL : www.highgo.ca
>>
>>
>
> The latest changes look good to me. However, the patch set is missing the
> documentation.
> Please add those.
>
> Thanks
>
> --
> Jeevan Chalke
> Associate Database Architect & Team Lead, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
>

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

Hi,

I have created a commitfest entry.
https://commitfest.postgresql.org/27/2472/

On Mon, Feb 17, 2020 at 1:39 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

> Thanks Jeevan. Here is the documentation patch.
>
> On Mon, Feb 10, 2020 at 6:49 PM Jeevan Chalke <
> jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>
>> Hi Asif,
>>
>> On Thu, Jan 30, 2020 at 7:10 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
>> wrote:
>>
>>>
>>> Here are the the updated patches, taking care of the issues pointed
>>> earlier. This patch adds the following commands (with specified option):
>>>
>>> START_BACKUP [LABEL '<label>'] [FAST]
>>> STOP_BACKUP [NOWAIT]
>>> LIST_TABLESPACES [PROGRESS]
>>> LIST_FILES [TABLESPACE]
>>> LIST_WAL_FILES [START_WAL_LOCATION 'X/X'] [END_WAL_LOCATION 'X/X']
>>> SEND_FILES '(' FILE, FILE... ')' [START_WAL_LOCATION 'X/X']
>>> [NOVERIFY_CHECKSUMS]
>>>
>>>
>>> Parallel backup is not making any use of tablespace map, so I have
>>> removed that option from the above commands. There is a patch pending
>>> to remove the exclusive backup; we can further refactor the
>>> do_pg_start_backup
>>> function at that time, to remove the tablespace information and move the
>>> creation of tablespace_map file to the client.
>>>
>>>
>>> I have disabled the maxrate option for parallel backup. I intend to send
>>> out a separate patch for it. Robert previously suggested to implement
>>> throttling on the client-side. I found the original email thread [1]
>>> where throttling was proposed and added to the server. In that thread,
>>> it was originally implemented on the client-side, but per many
>>> suggestions,
>>> it was moved to server-side.
>>>
>>> So, I have a few suggestions on how we can implement this:
>>>
>>> 1- have another option for pg_basebackup (i.e. per-worker-maxrate) where
>>> the user could choose the bandwidth allocation for each worker. This
>>> approach
>>> can be implemented on the client-side as well as on the server-side.
>>>
>>> 2- have the maxrate, be divided among workers equally at first. and the
>>> let the main thread keep adjusting it whenever one of the workers
>>> finishes.
>>> I believe this would only be possible if we handle throttling on the
>>> client.
>>> Also, as I understand it, implementing this will introduce additional
>>> mutex
>>> for handling of bandwidth consumption data so that rate may be adjusted
>>> according to data received by threads.
>>>
>>> [1]
>>> https://www.postgresql.org/message-id/flat/521B4B29.20009%402ndquadrant.com#189bf840c87de5908c0b4467d31b50af
>>>
>>> --
>>> Asif Rehman
>>> Highgo Software (Canada/China/Pakistan)
>>> URL : www.highgo.ca
>>>
>>>
>>
>> The latest changes look good to me. However, the patch set is missing the
>> documentation.
>> Please add those.
>>
>> Thanks
>>
>> --
>> Jeevan Chalke
>> Associate Database Architect & Team Lead, Product Development
>> EnterpriseDB Corporation
>> The Enterprise PostgreSQL Company
>>
>>
>
> --
> --
> Asif Rehman
> Highgo Software (Canada/China/Pakistan)
> URL : www.highgo.ca
>
>

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

Hi Asif

I have started testing this feature. I have applied v6 patch on commit
a069218163704c44a8996e7e98e765c56e2b9c8e (30 Jan).
I got few observations, please take a look.

*--if backup failed, backup directory is not getting removed.*
[edb(at)localhost bin]$ ./pg_basebackup -p 5432 --jobs=9 -D /tmp/test_bkp/bkp6
pg_basebackup: error: could not connect to server: FATAL: number of
requested standby connections exceeds max_wal_senders (currently 10)
[edb(at)localhost bin]$ ./pg_basebackup -p 5432 --jobs=8 -D /tmp/test_bkp/bkp6
pg_basebackup: error: directory "/tmp/test_bkp/bkp6" exists but is not empty

*--giving large number of jobs leading segmentation fault.*
./pg_basebackup -p 5432 --jobs=1000 -D /tmp/t3
pg_basebackup: error: could not connect to server: FATAL: number of
requested standby connections exceeds max_wal_senders (currently 10)
pg_basebackup: error: could not connect to server: FATAL: number of
requested standby connections exceeds max_wal_senders (currently 10)
pg_basebackup: error: could not connect to server: FATAL: number of
requested standby connections exceeds max_wal_senders (currently 10)
.
.
.
pg_basebackup: error: could not connect to server: FATAL: number of
requested standby connections exceeds max_wal_senders (currently 10)
pg_basebackup: error: could not connect to server: FATAL: number of
requested standby connections exceeds max_wal_senders (currently 10)
pg_basebackup: error: could not connect to server: FATAL: number of
requested standby connections exceeds max_wal_senders (currently 10)
pg_basebackup: error: could not connect to server: FATAL: number of
requested standby connections exceeds max_wal_senders (currently 10)
pg_basebackup: error: could not connect to server: could not fork new
process for connection: Resource temporarily unavailable

could not fork new process for connection: Resource temporarily unavailable
pg_basebackup: error: failed to create thread: Resource temporarily
unavailable
Segmentation fault (core dumped)

--stack-trace
gdb -q -c core.11824 pg_basebackup
Loaded symbols for /lib64/libnss_files.so.2
Core was generated by `./pg_basebackup -p 5432 --jobs=1000 -D
/tmp/test_bkp/bkp10'.
Program terminated with signal 11, Segmentation fault.
#0 pthread_join (threadid=140503120623360, thread_return=0x0) at
pthread_join.c:46
46 if (INVALID_NOT_TERMINATED_TD_P (pd))
Missing separate debuginfos, use: debuginfo-install
keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
libcom_err-1.41.12-24.el6.x86_64 libselinux-2.0.94-7.el6.x86_64
openssl-1.0.1e-58.el6_10.x86_64 zlib-1.2.3-29.el6.x86_64
(gdb) bt
#0 pthread_join (threadid=140503120623360, thread_return=0x0) at
pthread_join.c:46
#1 0x0000000000408e21 in cleanup_workers () at pg_basebackup.c:2840
#2 0x0000000000403846 in disconnect_atexit () at pg_basebackup.c:316
#3 0x0000003921235a02 in __run_exit_handlers (status=1) at exit.c:78
#4 exit (status=1) at exit.c:100
#5 0x0000000000408aa6 in create_parallel_workers (backupinfo=0x1a4b8c0) at
pg_basebackup.c:2713
#6 0x0000000000407946 in BaseBackup () at pg_basebackup.c:2127
#7 0x000000000040895c in main (argc=6, argv=0x7ffd566f4718) at
pg_basebackup.c:2668

*--with tablespace is in the same directory as data, parallel_backup
crashed*
[edb(at)localhost bin]$ ./initdb -D /tmp/data
[edb(at)localhost bin]$ ./pg_ctl -D /tmp/data -l /tmp/logfile start
[edb(at)localhost bin]$ mkdir /tmp/ts
[edb(at)localhost bin]$ ./psql postgres
psql (13devel)
Type "help" for help.

postgres=# create tablespace ts location '/tmp/ts';
CREATE TABLESPACE
postgres=# create table tx (a int) tablespace ts;
CREATE TABLE
postgres=# \q
[edb(at)localhost bin]$ ./pg_basebackup -j 2 -D /tmp/tts -T /tmp/ts=/tmp/ts1
Segmentation fault (core dumped)

--stack-trace
[edb(at)localhost bin]$ gdb -q -c core.15778 pg_basebackup
Loaded symbols for /lib64/libnss_files.so.2
Core was generated by `./pg_basebackup -j 2 -D /tmp/tts -T
/tmp/ts=/tmp/ts1'.
Program terminated with signal 11, Segmentation fault.
#0 0x0000000000409442 in get_backup_filelist (conn=0x140cb20,
backupInfo=0x14210a0) at pg_basebackup.c:3000
3000 backupInfo->curr->next = file;
Missing separate debuginfos, use: debuginfo-install
keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
libcom_err-1.41.12-24.el6.x86_64 libselinux-2.0.94-7.el6.x86_64
openssl-1.0.1e-58.el6_10.x86_64 zlib-1.2.3-29.el6.x86_64
(gdb) bt
#0 0x0000000000409442 in get_backup_filelist (conn=0x140cb20,
backupInfo=0x14210a0) at pg_basebackup.c:3000
#1 0x0000000000408b56 in parallel_backup_run (backupinfo=0x14210a0) at
pg_basebackup.c:2739
#2 0x0000000000407955 in BaseBackup () at pg_basebackup.c:2128
#3 0x000000000040895c in main (argc=7, argv=0x7ffca2910c58) at
pg_basebackup.c:2668
(gdb)

Thanks & Regards,
Rajkumar Raghuwanshi

On Tue, Feb 25, 2020 at 7:49 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

> Hi,
>
> I have created a commitfest entry.
> https://commitfest.postgresql.org/27/2472/
>
>
> On Mon, Feb 17, 2020 at 1:39 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
> wrote:
>
>> Thanks Jeevan. Here is the documentation patch.
>>
>> On Mon, Feb 10, 2020 at 6:49 PM Jeevan Chalke <
>> jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Asif,
>>>
>>> On Thu, Jan 30, 2020 at 7:10 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com>
>>> wrote:
>>>
>>>>
>>>> Here are the the updated patches, taking care of the issues pointed
>>>> earlier. This patch adds the following commands (with specified option):
>>>>
>>>> START_BACKUP [LABEL '<label>'] [FAST]
>>>> STOP_BACKUP [NOWAIT]
>>>> LIST_TABLESPACES [PROGRESS]
>>>> LIST_FILES [TABLESPACE]
>>>> LIST_WAL_FILES [START_WAL_LOCATION 'X/X'] [END_WAL_LOCATION 'X/X']
>>>> SEND_FILES '(' FILE, FILE... ')' [START_WAL_LOCATION 'X/X']
>>>> [NOVERIFY_CHECKSUMS]
>>>>
>>>>
>>>> Parallel backup is not making any use of tablespace map, so I have
>>>> removed that option from the above commands. There is a patch pending
>>>> to remove the exclusive backup; we can further refactor the
>>>> do_pg_start_backup
>>>> function at that time, to remove the tablespace information and move the
>>>> creation of tablespace_map file to the client.
>>>>
>>>>
>>>> I have disabled the maxrate option for parallel backup. I intend to send
>>>> out a separate patch for it. Robert previously suggested to implement
>>>> throttling on the client-side. I found the original email thread [1]
>>>> where throttling was proposed and added to the server. In that thread,
>>>> it was originally implemented on the client-side, but per many
>>>> suggestions,
>>>> it was moved to server-side.
>>>>
>>>> So, I have a few suggestions on how we can implement this:
>>>>
>>>> 1- have another option for pg_basebackup (i.e. per-worker-maxrate) where
>>>> the user could choose the bandwidth allocation for each worker. This
>>>> approach
>>>> can be implemented on the client-side as well as on the server-side.
>>>>
>>>> 2- have the maxrate, be divided among workers equally at first. and the
>>>> let the main thread keep adjusting it whenever one of the workers
>>>> finishes.
>>>> I believe this would only be possible if we handle throttling on the
>>>> client.
>>>> Also, as I understand it, implementing this will introduce additional
>>>> mutex
>>>> for handling of bandwidth consumption data so that rate may be adjusted
>>>> according to data received by threads.
>>>>
>>>> [1]
>>>> https://www.postgresql.org/message-id/flat/521B4B29.20009%402ndquadrant.com#189bf840c87de5908c0b4467d31b50af
>>>>
>>>> --
>>>> Asif Rehman
>>>> Highgo Software (Canada/China/Pakistan)
>>>> URL : www.highgo.ca
>>>>
>>>>
>>>
>>> The latest changes look good to me. However, the patch set is missing
>>> the documentation.
>>> Please add those.
>>>
>>> Thanks
>>>
>>> --
>>> Jeevan Chalke
>>> Associate Database Architect & Team Lead, Product Development
>>> EnterpriseDB Corporation
>>> The Enterprise PostgreSQL Company
>>>
>>>
>>
>> --
>> --
>> Asif Rehman
>> Highgo Software (Canada/China/Pakistan)
>> URL : www.highgo.ca
>>
>>
>
> --
> --
> Asif Rehman
> Highgo Software (Canada/China/Pakistan)
> URL : www.highgo.ca
>
>

On Wed, Mar 11, 2020 at 2:38 PM Rajkumar Raghuwanshi <
rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com> wrote:

> Hi Asif
>
> I have started testing this feature. I have applied v6 patch on commit
> a069218163704c44a8996e7e98e765c56e2b9c8e (30 Jan).
> I got few observations, please take a look.
>
> *--if backup failed, backup directory is not getting removed.*
> [edb(at)localhost bin]$ ./pg_basebackup -p 5432 --jobs=9 -D
> /tmp/test_bkp/bkp6
> pg_basebackup: error: could not connect to server: FATAL: number of
> requested standby connections exceeds max_wal_senders (currently 10)
> [edb(at)localhost bin]$ ./pg_basebackup -p 5432 --jobs=8 -D
> /tmp/test_bkp/bkp6
> pg_basebackup: error: directory "/tmp/test_bkp/bkp6" exists but is not
> empty
>
>
> *--giving large number of jobs leading segmentation fault.*
> ./pg_basebackup -p 5432 --jobs=1000 -D /tmp/t3
> pg_basebackup: error: could not connect to server: FATAL: number of
> requested standby connections exceeds max_wal_senders (currently 10)
> pg_basebackup: error: could not connect to server: FATAL: number of
> requested standby connections exceeds max_wal_senders (currently 10)
> pg_basebackup: error: could not connect to server: FATAL: number of
> requested standby connections exceeds max_wal_senders (currently 10)
> .
> .
> .
> pg_basebackup: error: could not connect to server: FATAL: number of
> requested standby connections exceeds max_wal_senders (currently 10)
> pg_basebackup: error: could not connect to server: FATAL: number of
> requested standby connections exceeds max_wal_senders (currently 10)
> pg_basebackup: error: could not connect to server: FATAL: number of
> requested standby connections exceeds max_wal_senders (currently 10)
> pg_basebackup: error: could not connect to server: FATAL: number of
> requested standby connections exceeds max_wal_senders (currently 10)
> pg_basebackup: error: could not connect to server: could not fork new
> process for connection: Resource temporarily unavailable
>
> could not fork new process for connection: Resource temporarily unavailable
> pg_basebackup: error: failed to create thread: Resource temporarily
> unavailable
> Segmentation fault (core dumped)
>
> --stack-trace
> gdb -q -c core.11824 pg_basebackup
> Loaded symbols for /lib64/libnss_files.so.2
> Core was generated by `./pg_basebackup -p 5432 --jobs=1000 -D
> /tmp/test_bkp/bkp10'.
> Program terminated with signal 11, Segmentation fault.
> #0 pthread_join (threadid=140503120623360, thread_return=0x0) at
> pthread_join.c:46
> 46 if (INVALID_NOT_TERMINATED_TD_P (pd))
> Missing separate debuginfos, use: debuginfo-install
> keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
> libcom_err-1.41.12-24.el6.x86_64 libselinux-2.0.94-7.el6.x86_64
> openssl-1.0.1e-58.el6_10.x86_64 zlib-1.2.3-29.el6.x86_64
> (gdb) bt
> #0 pthread_join (threadid=140503120623360, thread_return=0x0) at
> pthread_join.c:46
> #1 0x0000000000408e21 in cleanup_workers () at pg_basebackup.c:2840
> #2 0x0000000000403846 in disconnect_atexit () at pg_basebackup.c:316
> #3 0x0000003921235a02 in __run_exit_handlers (status=1) at exit.c:78
> #4 exit (status=1) at exit.c:100
> #5 0x0000000000408aa6 in create_parallel_workers (backupinfo=0x1a4b8c0)
> at pg_basebackup.c:2713
> #6 0x0000000000407946 in BaseBackup () at pg_basebackup.c:2127
> #7 0x000000000040895c in main (argc=6, argv=0x7ffd566f4718) at
> pg_basebackup.c:2668
>
>
> *--with tablespace is in the same directory as data, parallel_backup
> crashed*
> [edb(at)localhost bin]$ ./initdb -D /tmp/data
> [edb(at)localhost bin]$ ./pg_ctl -D /tmp/data -l /tmp/logfile start
> [edb(at)localhost bin]$ mkdir /tmp/ts
> [edb(at)localhost bin]$ ./psql postgres
> psql (13devel)
> Type "help" for help.
>
> postgres=# create tablespace ts location '/tmp/ts';
> CREATE TABLESPACE
> postgres=# create table tx (a int) tablespace ts;
> CREATE TABLE
> postgres=# \q
> [edb(at)localhost bin]$ ./pg_basebackup -j 2 -D /tmp/tts -T /tmp/ts=/tmp/ts1
> Segmentation fault (core dumped)
>
> --stack-trace
> [edb(at)localhost bin]$ gdb -q -c core.15778 pg_basebackup
> Loaded symbols for /lib64/libnss_files.so.2
> Core was generated by `./pg_basebackup -j 2 -D /tmp/tts -T
> /tmp/ts=/tmp/ts1'.
> Program terminated with signal 11, Segmentation fault.
> #0 0x0000000000409442 in get_backup_filelist (conn=0x140cb20,
> backupInfo=0x14210a0) at pg_basebackup.c:3000
> 3000 backupInfo->curr->next = file;
> Missing separate debuginfos, use: debuginfo-install
> keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
> libcom_err-1.41.12-24.el6.x86_64 libselinux-2.0.94-7.el6.x86_64
> openssl-1.0.1e-58.el6_10.x86_64 zlib-1.2.3-29.el6.x86_64
> (gdb) bt
> #0 0x0000000000409442 in get_backup_filelist (conn=0x140cb20,
> backupInfo=0x14210a0) at pg_basebackup.c:3000
> #1 0x0000000000408b56 in parallel_backup_run (backupinfo=0x14210a0) at
> pg_basebackup.c:2739
> #2 0x0000000000407955 in BaseBackup () at pg_basebackup.c:2128
> #3 0x000000000040895c in main (argc=7, argv=0x7ffca2910c58) at
> pg_basebackup.c:2668
> (gdb)
>

Thanks Rajkumar. I have fixed the above issues and have rebased the patch
to the latest master (b7f64c64).
(V9 of the patches are attached).

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

Thanks for the patches.

I have verified reported issues with new patches, issues are fixed now.

I got another observation where If a new slot name given without -C option,
it leads to server crash error.

[edb(at)localhost bin]$ ./pg_basebackup -p 5432 -j 4 -D /tmp/bkp --slot
test_bkp_slot
pg_basebackup: error: could not send replication command
"START_REPLICATION": ERROR: replication slot "test_bkp_slot" does not exist
pg_basebackup: error: could not list backup files: server closed the
connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
pg_basebackup: removing data directory "/tmp/bkp"

Thanks & Regards,
Rajkumar Raghuwanshi

On Fri, Mar 13, 2020 at 9:51 PM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

>
> On Wed, Mar 11, 2020 at 2:38 PM Rajkumar Raghuwanshi <
> rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com> wrote:
>
>> Hi Asif
>>
>> I have started testing this feature. I have applied v6 patch on commit
>> a069218163704c44a8996e7e98e765c56e2b9c8e (30 Jan).
>> I got few observations, please take a look.
>>
>> *--if backup failed, backup directory is not getting removed.*
>> [edb(at)localhost bin]$ ./pg_basebackup -p 5432 --jobs=9 -D
>> /tmp/test_bkp/bkp6
>> pg_basebackup: error: could not connect to server: FATAL: number of
>> requested standby connections exceeds max_wal_senders (currently 10)
>> [edb(at)localhost bin]$ ./pg_basebackup -p 5432 --jobs=8 -D
>> /tmp/test_bkp/bkp6
>> pg_basebackup: error: directory "/tmp/test_bkp/bkp6" exists but is not
>> empty
>>
>>
>> *--giving large number of jobs leading segmentation fault.*
>> ./pg_basebackup -p 5432 --jobs=1000 -D /tmp/t3
>> pg_basebackup: error: could not connect to server: FATAL: number of
>> requested standby connections exceeds max_wal_senders (currently 10)
>> pg_basebackup: error: could not connect to server: FATAL: number of
>> requested standby connections exceeds max_wal_senders (currently 10)
>> pg_basebackup: error: could not connect to server: FATAL: number of
>> requested standby connections exceeds max_wal_senders (currently 10)
>> .
>> .
>> .
>> pg_basebackup: error: could not connect to server: FATAL: number of
>> requested standby connections exceeds max_wal_senders (currently 10)
>> pg_basebackup: error: could not connect to server: FATAL: number of
>> requested standby connections exceeds max_wal_senders (currently 10)
>> pg_basebackup: error: could not connect to server: FATAL: number of
>> requested standby connections exceeds max_wal_senders (currently 10)
>> pg_basebackup: error: could not connect to server: FATAL: number of
>> requested standby connections exceeds max_wal_senders (currently 10)
>> pg_basebackup: error: could not connect to server: could not fork new
>> process for connection: Resource temporarily unavailable
>>
>> could not fork new process for connection: Resource temporarily
>> unavailable
>> pg_basebackup: error: failed to create thread: Resource temporarily
>> unavailable
>> Segmentation fault (core dumped)
>>
>> --stack-trace
>> gdb -q -c core.11824 pg_basebackup
>> Loaded symbols for /lib64/libnss_files.so.2
>> Core was generated by `./pg_basebackup -p 5432 --jobs=1000 -D
>> /tmp/test_bkp/bkp10'.
>> Program terminated with signal 11, Segmentation fault.
>> #0 pthread_join (threadid=140503120623360, thread_return=0x0) at
>> pthread_join.c:46
>> 46 if (INVALID_NOT_TERMINATED_TD_P (pd))
>> Missing separate debuginfos, use: debuginfo-install
>> keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
>> libcom_err-1.41.12-24.el6.x86_64 libselinux-2.0.94-7.el6.x86_64
>> openssl-1.0.1e-58.el6_10.x86_64 zlib-1.2.3-29.el6.x86_64
>> (gdb) bt
>> #0 pthread_join (threadid=140503120623360, thread_return=0x0) at
>> pthread_join.c:46
>> #1 0x0000000000408e21 in cleanup_workers () at pg_basebackup.c:2840
>> #2 0x0000000000403846 in disconnect_atexit () at pg_basebackup.c:316
>> #3 0x0000003921235a02 in __run_exit_handlers (status=1) at exit.c:78
>> #4 exit (status=1) at exit.c:100
>> #5 0x0000000000408aa6 in create_parallel_workers (backupinfo=0x1a4b8c0)
>> at pg_basebackup.c:2713
>> #6 0x0000000000407946 in BaseBackup () at pg_basebackup.c:2127
>> #7 0x000000000040895c in main (argc=6, argv=0x7ffd566f4718) at
>> pg_basebackup.c:2668
>>
>>
>> *--with tablespace is in the same directory as data, parallel_backup
>> crashed*
>> [edb(at)localhost bin]$ ./initdb -D /tmp/data
>> [edb(at)localhost bin]$ ./pg_ctl -D /tmp/data -l /tmp/logfile start
>> [edb(at)localhost bin]$ mkdir /tmp/ts
>> [edb(at)localhost bin]$ ./psql postgres
>> psql (13devel)
>> Type "help" for help.
>>
>> postgres=# create tablespace ts location '/tmp/ts';
>> CREATE TABLESPACE
>> postgres=# create table tx (a int) tablespace ts;
>> CREATE TABLE
>> postgres=# \q
>> [edb(at)localhost bin]$ ./pg_basebackup -j 2 -D /tmp/tts -T /tmp/ts=/tmp/ts1
>> Segmentation fault (core dumped)
>>
>> --stack-trace
>> [edb(at)localhost bin]$ gdb -q -c core.15778 pg_basebackup
>> Loaded symbols for /lib64/libnss_files.so.2
>> Core was generated by `./pg_basebackup -j 2 -D /tmp/tts -T
>> /tmp/ts=/tmp/ts1'.
>> Program terminated with signal 11, Segmentation fault.
>> #0 0x0000000000409442 in get_backup_filelist (conn=0x140cb20,
>> backupInfo=0x14210a0) at pg_basebackup.c:3000
>> 3000 backupInfo->curr->next = file;
>> Missing separate debuginfos, use: debuginfo-install
>> keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
>> libcom_err-1.41.12-24.el6.x86_64 libselinux-2.0.94-7.el6.x86_64
>> openssl-1.0.1e-58.el6_10.x86_64 zlib-1.2.3-29.el6.x86_64
>> (gdb) bt
>> #0 0x0000000000409442 in get_backup_filelist (conn=0x140cb20,
>> backupInfo=0x14210a0) at pg_basebackup.c:3000
>> #1 0x0000000000408b56 in parallel_backup_run (backupinfo=0x14210a0) at
>> pg_basebackup.c:2739
>> #2 0x0000000000407955 in BaseBackup () at pg_basebackup.c:2128
>> #3 0x000000000040895c in main (argc=7, argv=0x7ffca2910c58) at
>> pg_basebackup.c:2668
>> (gdb)
>>
>
>
> Thanks Rajkumar. I have fixed the above issues and have rebased the patch
> to the latest master (b7f64c64).
> (V9 of the patches are attached).
>
>
> --
> Asif Rehman
> Highgo Software (Canada/China/Pakistan)
> URL : www.highgo.ca
>
>

On Mon, Mar 16, 2020 at 11:08 AM Rajkumar Raghuwanshi <
rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com> wrote:

> Thanks for the patches.
>
> I have verified reported issues with new patches, issues are fixed now.
>
> I got another observation where If a new slot name given without -C
> option, it leads to server crash error.
>
> [edb(at)localhost bin]$ ./pg_basebackup -p 5432 -j 4 -D /tmp/bkp --slot
> test_bkp_slot
> pg_basebackup: error: could not send replication command
> "START_REPLICATION": ERROR: replication slot "test_bkp_slot" does not exist
> pg_basebackup: error: could not list backup files: server closed the
> connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> pg_basebackup: removing data directory "/tmp/bkp"
>

It seems to be an expected behavior. The START_BACKUP command has been
executed, and
pg_basebackup tries to start a WAL streaming process with a non-existent
slot, which results in
an error. So the backup is aborted while terminating all other processes.

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

On Mon, Mar 16, 2020 at 11:52 AM Asif Rehman <asifr(dot)rehman(at)gmail(dot)com> wrote:

>
>
> On Mon, Mar 16, 2020 at 11:08 AM Rajkumar Raghuwanshi <
> rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com> wrote:
>
>> Thanks for the patches.
>>
>> I have verified reported issues with new patches, issues are fixed now.
>>
>> I got another observation where If a new slot name given without -C
>> option, it leads to server crash error.
>>
>> [edb(at)localhost bin]$ ./pg_basebackup -p 5432 -j 4 -D /tmp/bkp --slot
>> test_bkp_slot
>> pg_basebackup: error: could not send replication command
>> "START_REPLICATION": ERROR: replication slot "test_bkp_slot" does not exist
>> pg_basebackup: error: could not list backup files: server closed the
>> connection unexpectedly
>> This probably means the server terminated abnormally
>> before or while processing the request.
>> pg_basebackup: removing data directory "/tmp/bkp"
>>
>
> It seems to be an expected behavior. The START_BACKUP command has been
> executed, and
> pg_basebackup tries to start a WAL streaming process with a non-existent
> slot, which results in
> an error. So the backup is aborted while terminating all other processes.
>
I think error message can be improved. current error message looks like
database server is crashed.

on PG same is existing with exit 1.
[edb(at)localhost bin]$ ./pg_basebackup -p 5432 -D /tmp/bkp --slot
test_bkp_slot
pg_basebackup: error: could not send replication command
"START_REPLICATION": ERROR: replication slot "test_bkp_slot" does not exist
pg_basebackup: error: child process exited with exit code 1
pg_basebackup: removing data directory "/tmp/bkp"

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

Hi Asif,

> Thanks Rajkumar. I have fixed the above issues and have rebased the patch
> to the latest master (b7f64c64).
> (V9 of the patches are attached).
>

I had a further review of the patches and here are my few observations:

1.
+/*
+ * stop_backup() - ends an online backup
+ *
+ * The function is called at the end of an online backup. It sends out
pg_control
+ * file, optionally WAL segments and ending WAL location.
+ */

Comments seem out-dated.

2. With parallel jobs, maxrate is now not supported. Since we are now asking
data in multiple threads throttling seems important here. Can you please
explain why have you disabled that?

3. As we are always fetching a single file and as Robert suggested, let
rename
SEND_FILES to SEND_FILE instead.

4. Does this work on Windows? I mean does pthread_create() work on Windows?
I asked this as I see that pgbench has its own implementation for
pthread_create() for WIN32 but this patch doesn't.

5. Typos:
tablspace => tablespace
safly => safely

6. parallel_backup_run() needs some comments explaining the states it goes
through PB_* states.

7.
+ case PB_FETCH_REL_FILES: /* fetch files from server */
+ if (backupinfo->activeworkers == 0)
+ {
+ backupinfo->backupstate = PB_STOP_BACKUP;
+ free_filelist(backupinfo);
+ }
+ break;
+ case PB_FETCH_WAL_FILES: /* fetch WAL files from server */
+ if (backupinfo->activeworkers == 0)
+ {
+ backupinfo->backupstate = PB_BACKUP_COMPLETE;
+ }
+ break;

Why free_filelist() is not called in PB_FETCH_WAL_FILES case?

Thanks
--
Jeevan Chalke
Associate Database Architect & Team Lead, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company

Phone: +91 20 66449694

Website: www.enterprisedb.com
EnterpriseDB Blog: http://blogs.enterprisedb.com/
Follow us on Twitter: http://www.twitter.com/enterprisedb

This e-mail message (and any attachment) is intended for the use of the
individual or entity to whom it is addressed. This message contains
information from EnterpriseDB Corporation that may be privileged,
confidential, or exempt from disclosure under applicable law. If you are
not the intended recipient or authorized to receive this for the intended
recipient, any use, dissemination, distribution, retention, archiving, or
copying of this communication is strictly prohibited. If you have received
this e-mail in error, please notify the sender immediately by reply e-mail
and delete this message.

Hi Asif,

On testing further, I found when taking backup with -R, pg_basebackup
crashed
this crash is not consistently reproducible.

[edb(at)localhost bin]$ ./psql postgres -p 5432 -c "create table test (a
text);"
CREATE TABLE
[edb(at)localhost bin]$ ./psql postgres -p 5432 -c "insert into test values
('parallel_backup with -R recovery-conf');"
INSERT 0 1
[edb(at)localhost bin]$ ./pg_basebackup -p 5432 -j 2 -D /tmp/test_bkp/bkp -R
Segmentation fault (core dumped)

stack trace looks the same as it was on earlier reported crash with
tablespace.
--stack trace
[edb(at)localhost bin]$ gdb -q -c core.37915 pg_basebackup
Loaded symbols for /lib64/libnss_files.so.2
Core was generated by `./pg_basebackup -p 5432 -j 2 -D /tmp/test_bkp/bkp
-R'.
Program terminated with signal 11, Segmentation fault.
#0 0x00000000004099ee in worker_get_files (wstate=0xc1e458) at
pg_basebackup.c:3175
3175 backupinfo->curr = fetchfile->next;
Missing separate debuginfos, use: debuginfo-install
keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
libcom_err-1.41.12-24.el6.x86_64 libselinux-2.0.94-7.el6.x86_64
openssl-1.0.1e-58.el6_10.x86_64 zlib-1.2.3-29.el6.x86_64
(gdb) bt
#0 0x00000000004099ee in worker_get_files (wstate=0xc1e458) at
pg_basebackup.c:3175
#1 0x0000000000408a9e in worker_run (arg=0xc1e458) at pg_basebackup.c:2715
#2 0x0000003921a07aa1 in start_thread (arg=0x7f72207c0700) at
pthread_create.c:301
#3 0x00000039212e8c4d in clone () at
../sysdeps/unix/sysv/linux/x86_64/clone.S:115
(gdb)

Thanks & Regards,
Rajkumar Raghuwanshi

On Mon, Mar 16, 2020 at 2:14 PM Jeevan Chalke <
jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:

> Hi Asif,
>
>
>> Thanks Rajkumar. I have fixed the above issues and have rebased the patch
>> to the latest master (b7f64c64).
>> (V9 of the patches are attached).
>>
>
> I had a further review of the patches and here are my few observations:
>
> 1.
> +/*
> + * stop_backup() - ends an online backup
> + *
> + * The function is called at the end of an online backup. It sends out
> pg_control
> + * file, optionally WAL segments and ending WAL location.
> + */
>
> Comments seem out-dated.
>
> 2. With parallel jobs, maxrate is now not supported. Since we are now
> asking
> data in multiple threads throttling seems important here. Can you please
> explain why have you disabled that?
>
> 3. As we are always fetching a single file and as Robert suggested, let
> rename
> SEND_FILES to SEND_FILE instead.
>
> 4. Does this work on Windows? I mean does pthread_create() work on Windows?
> I asked this as I see that pgbench has its own implementation for
> pthread_create() for WIN32 but this patch doesn't.
>
> 5. Typos:
> tablspace => tablespace
> safly => safely
>
> 6. parallel_backup_run() needs some comments explaining the states it goes
> through PB_* states.
>
> 7.
> + case PB_FETCH_REL_FILES: /* fetch files from server */
> + if (backupinfo->activeworkers == 0)
> + {
> + backupinfo->backupstate = PB_STOP_BACKUP;
> + free_filelist(backupinfo);
> + }
> + break;
> + case PB_FETCH_WAL_FILES: /* fetch WAL files from server */
> + if (backupinfo->activeworkers == 0)
> + {
> + backupinfo->backupstate = PB_BACKUP_COMPLETE;
> + }
> + break;
>
> Why free_filelist() is not called in PB_FETCH_WAL_FILES case?
>
> Thanks
> --
> Jeevan Chalke
> Associate Database Architect & Team Lead, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
> Phone: +91 20 66449694
>
> Website: www.enterprisedb.com
> EnterpriseDB Blog: http://blogs.enterprisedb.com/
> Follow us on Twitter: http://www.twitter.com/enterprisedb
>
> This e-mail message (and any attachment) is intended for the use of the
> individual or entity to whom it is addressed. This message contains
> information from EnterpriseDB Corporation that may be privileged,
> confidential, or exempt from disclosure under applicable law. If you are
> not the intended recipient or authorized to receive this for the intended
> recipient, any use, dissemination, distribution, retention, archiving, or
> copying of this communication is strictly prohibited. If you have received
> this e-mail in error, please notify the sender immediately by reply e-mail
> and delete this message.
>

Hi Asif,

In another scenarios, bkp data is corrupted for tablespace. again this is
not reproducible everytime,
but If I am running the same set of commands I am getting the same error.

[edb(at)localhost bin]$ ./pg_ctl -D data -l logfile start
waiting for server to start.... done
server started
[edb(at)localhost bin]$
[edb(at)localhost bin]$ mkdir /tmp/tblsp
[edb(at)localhost bin]$ ./psql postgres -p 5432 -c "create tablespace tblsp
location '/tmp/tblsp';"
CREATE TABLESPACE
[edb(at)localhost bin]$ ./psql postgres -p 5432 -c "create database testdb
tablespace tblsp;"
CREATE DATABASE
[edb(at)localhost bin]$ ./psql testdb -p 5432 -c "create table testtbl (a
text);"
CREATE TABLE
[edb(at)localhost bin]$ ./psql testdb -p 5432 -c "insert into testtbl values
('parallel_backup with tablespace');"
INSERT 0 1
[edb(at)localhost bin]$ ./pg_basebackup -p 5432 -D /tmp/bkp -T
/tmp/tblsp=/tmp/tblsp_bkp --jobs 2
[edb(at)localhost bin]$ ./pg_ctl -D /tmp/bkp -l /tmp/bkp_logs -o "-p 5555"
start
waiting for server to start.... done
server started
[edb(at)localhost bin]$ ./psql postgres -p 5555 -c "select * from
pg_tablespace where spcname like 'tblsp%' or spcname = 'pg_default'";
oid | spcname | spcowner | spcacl | spcoptions
-------+------------+----------+--------+------------
1663 | pg_default | 10 | |
16384 | tblsp | 10 | |
(2 rows)

[edb(at)localhost bin]$ ./psql testdb -p 5555 -c "select * from testtbl";
psql: error: could not connect to server: FATAL:
"pg_tblspc/16384/PG_13_202003051/16385" is not a valid data directory
DETAIL: File "pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION" is missing.
[edb(at)localhost bin]$
[edb(at)localhost bin]$ ls
data/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION
data/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION
[edb(at)localhost bin]$ ls
/tmp/bkp/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION
ls: cannot access
/tmp/bkp/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION: No such file or
directory

Thanks & Regards,
Rajkumar Raghuwanshi

On Mon, Mar 16, 2020 at 6:19 PM Rajkumar Raghuwanshi <
rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com> wrote:

> Hi Asif,
>
> On testing further, I found when taking backup with -R, pg_basebackup
> crashed
> this crash is not consistently reproducible.
>
> [edb(at)localhost bin]$ ./psql postgres -p 5432 -c "create table test (a
> text);"
> CREATE TABLE
> [edb(at)localhost bin]$ ./psql postgres -p 5432 -c "insert into test values
> ('parallel_backup with -R recovery-conf');"
> INSERT 0 1
> [edb(at)localhost bin]$ ./pg_basebackup -p 5432 -j 2 -D /tmp/test_bkp/bkp -R
> Segmentation fault (core dumped)
>
> stack trace looks the same as it was on earlier reported crash with
> tablespace.
> --stack trace
> [edb(at)localhost bin]$ gdb -q -c core.37915 pg_basebackup
> Loaded symbols for /lib64/libnss_files.so.2
> Core was generated by `./pg_basebackup -p 5432 -j 2 -D /tmp/test_bkp/bkp
> -R'.
> Program terminated with signal 11, Segmentation fault.
> #0 0x00000000004099ee in worker_get_files (wstate=0xc1e458) at
> pg_basebackup.c:3175
> 3175 backupinfo->curr = fetchfile->next;
> Missing separate debuginfos, use: debuginfo-install
> keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
> libcom_err-1.41.12-24.el6.x86_64 libselinux-2.0.94-7.el6.x86_64
> openssl-1.0.1e-58.el6_10.x86_64 zlib-1.2.3-29.el6.x86_64
> (gdb) bt
> #0 0x00000000004099ee in worker_get_files (wstate=0xc1e458) at
> pg_basebackup.c:3175
> #1 0x0000000000408a9e in worker_run (arg=0xc1e458) at pg_basebackup.c:2715
> #2 0x0000003921a07aa1 in start_thread (arg=0x7f72207c0700) at
> pthread_create.c:301
> #3 0x00000039212e8c4d in clone () at
> ../sysdeps/unix/sysv/linux/x86_64/clone.S:115
> (gdb)
>
> Thanks & Regards,
> Rajkumar Raghuwanshi
>
>
> On Mon, Mar 16, 2020 at 2:14 PM Jeevan Chalke <
> jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>
>> Hi Asif,
>>
>>
>>> Thanks Rajkumar. I have fixed the above issues and have rebased the
>>> patch to the latest master (b7f64c64).
>>> (V9 of the patches are attached).
>>>
>>
>> I had a further review of the patches and here are my few observations:
>>
>> 1.
>> +/*
>> + * stop_backup() - ends an online backup
>> + *
>> + * The function is called at the end of an online backup. It sends out
>> pg_control
>> + * file, optionally WAL segments and ending WAL location.
>> + */
>>
>> Comments seem out-dated.
>>
>> 2. With parallel jobs, maxrate is now not supported. Since we are now
>> asking
>> data in multiple threads throttling seems important here. Can you please
>> explain why have you disabled that?
>>
>> 3. As we are always fetching a single file and as Robert suggested, let
>> rename
>> SEND_FILES to SEND_FILE instead.
>>
>> 4. Does this work on Windows? I mean does pthread_create() work on
>> Windows?
>> I asked this as I see that pgbench has its own implementation for
>> pthread_create() for WIN32 but this patch doesn't.
>>
>> 5. Typos:
>> tablspace => tablespace
>> safly => safely
>>
>> 6. parallel_backup_run() needs some comments explaining the states it goes
>> through PB_* states.
>>
>> 7.
>> + case PB_FETCH_REL_FILES: /* fetch files from server */
>> + if (backupinfo->activeworkers == 0)
>> + {
>> + backupinfo->backupstate = PB_STOP_BACKUP;
>> + free_filelist(backupinfo);
>> + }
>> + break;
>> + case PB_FETCH_WAL_FILES: /* fetch WAL files from server */
>> + if (backupinfo->activeworkers == 0)
>> + {
>> + backupinfo->backupstate = PB_BACKUP_COMPLETE;
>> + }
>> + break;
>>
>> Why free_filelist() is not called in PB_FETCH_WAL_FILES case?
>>
>> Thanks
>> --
>> Jeevan Chalke
>> Associate Database Architect & Team Lead, Product Development
>> EnterpriseDB Corporation
>> The Enterprise PostgreSQL Company
>>
>> Phone: +91 20 66449694
>>
>> Website: www.enterprisedb.com
>> EnterpriseDB Blog: http://blogs.enterprisedb.com/
>> Follow us on Twitter: http://www.twitter.com/enterprisedb
>>
>> This e-mail message (and any attachment) is intended for the use of the
>> individual or entity to whom it is addressed. This message contains
>> information from EnterpriseDB Corporation that may be privileged,
>> confidential, or exempt from disclosure under applicable law. If you are
>> not the intended recipient or authorized to receive this for the intended
>> recipient, any use, dissemination, distribution, retention, archiving, or
>> copying of this communication is strictly prohibited. If you have received
>> this e-mail in error, please notify the sender immediately by reply e-mail
>> and delete this message.
>>
>

Hi Asif,

While testing further I observed parallel backup is not able to take backup
of standby server.

mkdir /tmp/archive_dir
echo "archive_mode='on'">> data/postgresql.conf
echo "archive_command='cp %p /tmp/archive_dir/%f'">> data/postgresql.conf

./pg_ctl -D data -l logs start
./pg_basebackup -p 5432 -Fp -R -D /tmp/slave

echo "primary_conninfo='host=127.0.0.1 port=5432 user=edb'">>
/tmp/slave/postgresql.conf
echo "restore_command='cp /tmp/archive_dir/%f %p'">>
/tmp/slave/postgresql.conf
echo "promote_trigger_file='/tmp/failover.log'">> /tmp/slave/postgresql.conf

./pg_ctl -D /tmp/slave -l /tmp/slave_logs -o "-p 5433" start -c

[edb(at)localhost bin]$ ./psql postgres -p 5432 -c "select
pg_is_in_recovery();"
pg_is_in_recovery
-------------------
f
(1 row)

[edb(at)localhost bin]$ ./psql postgres -p 5433 -c "select
pg_is_in_recovery();"
pg_is_in_recovery
-------------------
t
(1 row)

*[edb(at)localhost bin]$ ./pg_basebackup -p 5433 -D /tmp/bkp_s --jobs
6pg_basebackup: error: could not list backup files: ERROR: the standby was
promoted during online backupHINT: This means that the backup being taken
is corrupt and should not be used. Try taking another online
backup.pg_basebackup: removing data directory "/tmp/bkp_s"*

#same is working fine without parallel backup
[edb(at)localhost bin]$ ./pg_basebackup -p 5433 -D /tmp/bkp_s --jobs 1
[edb(at)localhost bin]$ ls /tmp/bkp_s/PG_VERSION
/tmp/bkp_s/PG_VERSION

Thanks & Regards,
Rajkumar Raghuwanshi

On Thu, Mar 19, 2020 at 4:11 PM Rajkumar Raghuwanshi <
rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com> wrote:

> Hi Asif,
>
> In another scenarios, bkp data is corrupted for tablespace. again this is
> not reproducible everytime,
> but If I am running the same set of commands I am getting the same error.
>
> [edb(at)localhost bin]$ ./pg_ctl -D data -l logfile start
> waiting for server to start.... done
> server started
> [edb(at)localhost bin]$
> [edb(at)localhost bin]$ mkdir /tmp/tblsp
> [edb(at)localhost bin]$ ./psql postgres -p 5432 -c "create tablespace tblsp
> location '/tmp/tblsp';"
> CREATE TABLESPACE
> [edb(at)localhost bin]$ ./psql postgres -p 5432 -c "create database testdb
> tablespace tblsp;"
> CREATE DATABASE
> [edb(at)localhost bin]$ ./psql testdb -p 5432 -c "create table testtbl (a
> text);"
> CREATE TABLE
> [edb(at)localhost bin]$ ./psql testdb -p 5432 -c "insert into testtbl values
> ('parallel_backup with tablespace');"
> INSERT 0 1
> [edb(at)localhost bin]$ ./pg_basebackup -p 5432 -D /tmp/bkp -T
> /tmp/tblsp=/tmp/tblsp_bkp --jobs 2
> [edb(at)localhost bin]$ ./pg_ctl -D /tmp/bkp -l /tmp/bkp_logs -o "-p 5555"
> start
> waiting for server to start.... done
> server started
> [edb(at)localhost bin]$ ./psql postgres -p 5555 -c "select * from
> pg_tablespace where spcname like 'tblsp%' or spcname = 'pg_default'";
> oid | spcname | spcowner | spcacl | spcoptions
> -------+------------+----------+--------+------------
> 1663 | pg_default | 10 | |
> 16384 | tblsp | 10 | |
> (2 rows)
>
> [edb(at)localhost bin]$ ./psql testdb -p 5555 -c "select * from testtbl";
> psql: error: could not connect to server: FATAL:
> "pg_tblspc/16384/PG_13_202003051/16385" is not a valid data directory
> DETAIL: File "pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION" is
> missing.
> [edb(at)localhost bin]$
> [edb(at)localhost bin]$ ls
> data/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION
> data/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION
> [edb(at)localhost bin]$ ls
> /tmp/bkp/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION
> ls: cannot access
> /tmp/bkp/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION: No such file or
> directory
>
>
> Thanks & Regards,
> Rajkumar Raghuwanshi
>
>
> On Mon, Mar 16, 2020 at 6:19 PM Rajkumar Raghuwanshi <
> rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com> wrote:
>
>> Hi Asif,
>>
>> On testing further, I found when taking backup with -R, pg_basebackup
>> crashed
>> this crash is not consistently reproducible.
>>
>> [edb(at)localhost bin]$ ./psql postgres -p 5432 -c "create table test (a
>> text);"
>> CREATE TABLE
>> [edb(at)localhost bin]$ ./psql postgres -p 5432 -c "insert into test values
>> ('parallel_backup with -R recovery-conf');"
>> INSERT 0 1
>> [edb(at)localhost bin]$ ./pg_basebackup -p 5432 -j 2 -D /tmp/test_bkp/bkp -R
>> Segmentation fault (core dumped)
>>
>> stack trace looks the same as it was on earlier reported crash with
>> tablespace.
>> --stack trace
>> [edb(at)localhost bin]$ gdb -q -c core.37915 pg_basebackup
>> Loaded symbols for /lib64/libnss_files.so.2
>> Core was generated by `./pg_basebackup -p 5432 -j 2 -D /tmp/test_bkp/bkp
>> -R'.
>> Program terminated with signal 11, Segmentation fault.
>> #0 0x00000000004099ee in worker_get_files (wstate=0xc1e458) at
>> pg_basebackup.c:3175
>> 3175 backupinfo->curr = fetchfile->next;
>> Missing separate debuginfos, use: debuginfo-install
>> keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
>> libcom_err-1.41.12-24.el6.x86_64 libselinux-2.0.94-7.el6.x86_64
>> openssl-1.0.1e-58.el6_10.x86_64 zlib-1.2.3-29.el6.x86_64
>> (gdb) bt
>> #0 0x00000000004099ee in worker_get_files (wstate=0xc1e458) at
>> pg_basebackup.c:3175
>> #1 0x0000000000408a9e in worker_run (arg=0xc1e458) at
>> pg_basebackup.c:2715
>> #2 0x0000003921a07aa1 in start_thread (arg=0x7f72207c0700) at
>> pthread_create.c:301
>> #3 0x00000039212e8c4d in clone () at
>> ../sysdeps/unix/sysv/linux/x86_64/clone.S:115
>> (gdb)
>>
>> Thanks & Regards,
>> Rajkumar Raghuwanshi
>>
>>
>> On Mon, Mar 16, 2020 at 2:14 PM Jeevan Chalke <
>> jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Asif,
>>>
>>>
>>>> Thanks Rajkumar. I have fixed the above issues and have rebased the
>>>> patch to the latest master (b7f64c64).
>>>> (V9 of the patches are attached).
>>>>
>>>
>>> I had a further review of the patches and here are my few observations:
>>>
>>> 1.
>>> +/*
>>> + * stop_backup() - ends an online backup
>>> + *
>>> + * The function is called at the end of an online backup. It sends out
>>> pg_control
>>> + * file, optionally WAL segments and ending WAL location.
>>> + */
>>>
>>> Comments seem out-dated.
>>>
>>> 2. With parallel jobs, maxrate is now not supported. Since we are now
>>> asking
>>> data in multiple threads throttling seems important here. Can you please
>>> explain why have you disabled that?
>>>
>>> 3. As we are always fetching a single file and as Robert suggested, let
>>> rename
>>> SEND_FILES to SEND_FILE instead.
>>>
>>> 4. Does this work on Windows? I mean does pthread_create() work on
>>> Windows?
>>> I asked this as I see that pgbench has its own implementation for
>>> pthread_create() for WIN32 but this patch doesn't.
>>>
>>> 5. Typos:
>>> tablspace => tablespace
>>> safly => safely
>>>
>>> 6. parallel_backup_run() needs some comments explaining the states it
>>> goes
>>> through PB_* states.
>>>
>>> 7.
>>> + case PB_FETCH_REL_FILES: /* fetch files from server */
>>> + if (backupinfo->activeworkers == 0)
>>> + {
>>> + backupinfo->backupstate = PB_STOP_BACKUP;
>>> + free_filelist(backupinfo);
>>> + }
>>> + break;
>>> + case PB_FETCH_WAL_FILES: /* fetch WAL files from server
>>> */
>>> + if (backupinfo->activeworkers == 0)
>>> + {
>>> + backupinfo->backupstate = PB_BACKUP_COMPLETE;
>>> + }
>>> + break;
>>>
>>> Why free_filelist() is not called in PB_FETCH_WAL_FILES case?
>>>
>>> Thanks
>>> --
>>> Jeevan Chalke
>>> Associate Database Architect & Team Lead, Product Development
>>> EnterpriseDB Corporation
>>> The Enterprise PostgreSQL Company
>>>
>>> Phone: +91 20 66449694
>>>
>>> Website: www.enterprisedb.com
>>> EnterpriseDB Blog: http://blogs.enterprisedb.com/
>>> Follow us on Twitter: http://www.twitter.com/enterprisedb
>>>
>>> This e-mail message (and any attachment) is intended for the use of the
>>> individual or entity to whom it is addressed. This message contains
>>> information from EnterpriseDB Corporation that may be privileged,
>>> confidential, or exempt from disclosure under applicable law. If you are
>>> not the intended recipient or authorized to receive this for the intended
>>> recipient, any use, dissemination, distribution, retention, archiving, or
>>> copying of this communication is strictly prohibited. If you have received
>>> this e-mail in error, please notify the sender immediately by reply e-mail
>>> and delete this message.
>>>
>>

On Wed, Mar 25, 2020 at 12:22 PM Rajkumar Raghuwanshi <
rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com> wrote:

> Hi Asif,
>
> While testing further I observed parallel backup is not able to take
> backup of standby server.
>
> mkdir /tmp/archive_dir
> echo "archive_mode='on'">> data/postgresql.conf
> echo "archive_command='cp %p /tmp/archive_dir/%f'">> data/postgresql.conf
>
> ./pg_ctl -D data -l logs start
> ./pg_basebackup -p 5432 -Fp -R -D /tmp/slave
>
> echo "primary_conninfo='host=127.0.0.1 port=5432 user=edb'">>
> /tmp/slave/postgresql.conf
> echo "restore_command='cp /tmp/archive_dir/%f %p'">>
> /tmp/slave/postgresql.conf
> echo "promote_trigger_file='/tmp/failover.log'">>
> /tmp/slave/postgresql.conf
>
> ./pg_ctl -D /tmp/slave -l /tmp/slave_logs -o "-p 5433" start -c
>
> [edb(at)localhost bin]$ ./psql postgres -p 5432 -c "select
> pg_is_in_recovery();"
> pg_is_in_recovery
> -------------------
> f
> (1 row)
>
> [edb(at)localhost bin]$ ./psql postgres -p 5433 -c "select
> pg_is_in_recovery();"
> pg_is_in_recovery
> -------------------
> t
> (1 row)
>
>
>
>
> *[edb(at)localhost bin]$ ./pg_basebackup -p 5433 -D /tmp/bkp_s --jobs
> 6pg_basebackup: error: could not list backup files: ERROR: the standby was
> promoted during online backupHINT: This means that the backup being taken
> is corrupt and should not be used. Try taking another online
> backup.pg_basebackup: removing data directory "/tmp/bkp_s"*
>
> #same is working fine without parallel backup
> [edb(at)localhost bin]$ ./pg_basebackup -p 5433 -D /tmp/bkp_s --jobs 1
> [edb(at)localhost bin]$ ls /tmp/bkp_s/PG_VERSION
> /tmp/bkp_s/PG_VERSION
>
> Thanks & Regards,
> Rajkumar Raghuwanshi
>
>
> On Thu, Mar 19, 2020 at 4:11 PM Rajkumar Raghuwanshi <
> rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com> wrote:
>
>> Hi Asif,
>>
>> In another scenarios, bkp data is corrupted for tablespace. again this is
>> not reproducible everytime,
>> but If I am running the same set of commands I am getting the same error.
>>
>> [edb(at)localhost bin]$ ./pg_ctl -D data -l logfile start
>> waiting for server to start.... done
>> server started
>> [edb(at)localhost bin]$
>> [edb(at)localhost bin]$ mkdir /tmp/tblsp
>> [edb(at)localhost bin]$ ./psql postgres -p 5432 -c "create tablespace tblsp
>> location '/tmp/tblsp';"
>> CREATE TABLESPACE
>> [edb(at)localhost bin]$ ./psql postgres -p 5432 -c "create database testdb
>> tablespace tblsp;"
>> CREATE DATABASE
>> [edb(at)localhost bin]$ ./psql testdb -p 5432 -c "create table testtbl (a
>> text);"
>> CREATE TABLE
>> [edb(at)localhost bin]$ ./psql testdb -p 5432 -c "insert into testtbl
>> values ('parallel_backup with tablespace');"
>> INSERT 0 1
>> [edb(at)localhost bin]$ ./pg_basebackup -p 5432 -D /tmp/bkp -T
>> /tmp/tblsp=/tmp/tblsp_bkp --jobs 2
>> [edb(at)localhost bin]$ ./pg_ctl -D /tmp/bkp -l /tmp/bkp_logs -o "-p 5555"
>> start
>> waiting for server to start.... done
>> server started
>> [edb(at)localhost bin]$ ./psql postgres -p 5555 -c "select * from
>> pg_tablespace where spcname like 'tblsp%' or spcname = 'pg_default'";
>> oid | spcname | spcowner | spcacl | spcoptions
>> -------+------------+----------+--------+------------
>> 1663 | pg_default | 10 | |
>> 16384 | tblsp | 10 | |
>> (2 rows)
>>
>> [edb(at)localhost bin]$ ./psql testdb -p 5555 -c "select * from testtbl";
>> psql: error: could not connect to server: FATAL:
>> "pg_tblspc/16384/PG_13_202003051/16385" is not a valid data directory
>> DETAIL: File "pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION" is
>> missing.
>> [edb(at)localhost bin]$
>> [edb(at)localhost bin]$ ls
>> data/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION
>> data/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION
>> [edb(at)localhost bin]$ ls
>> /tmp/bkp/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION
>> ls: cannot access
>> /tmp/bkp/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION: No such file or
>> directory
>>
>>
>> Thanks & Regards,
>> Rajkumar Raghuwanshi
>>
>>
>> On Mon, Mar 16, 2020 at 6:19 PM Rajkumar Raghuwanshi <
>> rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi Asif,
>>>
>>> On testing further, I found when taking backup with -R, pg_basebackup
>>> crashed
>>> this crash is not consistently reproducible.
>>>
>>> [edb(at)localhost bin]$ ./psql postgres -p 5432 -c "create table test (a
>>> text);"
>>> CREATE TABLE
>>> [edb(at)localhost bin]$ ./psql postgres -p 5432 -c "insert into test
>>> values ('parallel_backup with -R recovery-conf');"
>>> INSERT 0 1
>>> [edb(at)localhost bin]$ ./pg_basebackup -p 5432 -j 2 -D /tmp/test_bkp/bkp
>>> -R
>>> Segmentation fault (core dumped)
>>>
>>> stack trace looks the same as it was on earlier reported crash with
>>> tablespace.
>>> --stack trace
>>> [edb(at)localhost bin]$ gdb -q -c core.37915 pg_basebackup
>>> Loaded symbols for /lib64/libnss_files.so.2
>>> Core was generated by `./pg_basebackup -p 5432 -j 2 -D /tmp/test_bkp/bkp
>>> -R'.
>>> Program terminated with signal 11, Segmentation fault.
>>> #0 0x00000000004099ee in worker_get_files (wstate=0xc1e458) at
>>> pg_basebackup.c:3175
>>> 3175 backupinfo->curr = fetchfile->next;
>>> Missing separate debuginfos, use: debuginfo-install
>>> keyutils-libs-1.4-5.el6.x86_64 krb5-libs-1.10.3-65.el6.x86_64
>>> libcom_err-1.41.12-24.el6.x86_64 libselinux-2.0.94-7.el6.x86_64
>>> openssl-1.0.1e-58.el6_10.x86_64 zlib-1.2.3-29.el6.x86_64
>>> (gdb) bt
>>> #0 0x00000000004099ee in worker_get_files (wstate=0xc1e458) at
>>> pg_basebackup.c:3175
>>> #1 0x0000000000408a9e in worker_run (arg=0xc1e458) at
>>> pg_basebackup.c:2715
>>> #2 0x0000003921a07aa1 in start_thread (arg=0x7f72207c0700) at
>>> pthread_create.c:301
>>> #3 0x00000039212e8c4d in clone () at
>>> ../sysdeps/unix/sysv/linux/x86_64/clone.S:115
>>> (gdb)
>>>
>>> Thanks & Regards,
>>> Rajkumar Raghuwanshi
>>>
>>>
>>> On Mon, Mar 16, 2020 at 2:14 PM Jeevan Chalke <
>>> jeevan(dot)chalke(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Asif,
>>>>
>>>>
>>>>> Thanks Rajkumar. I have fixed the above issues and have rebased the
>>>>> patch to the latest master (b7f64c64).
>>>>> (V9 of the patches are attached).
>>>>>
>>>>
>>>> I had a further review of the patches and here are my few observations:
>>>>
>>>> 1.
>>>> +/*
>>>> + * stop_backup() - ends an online backup
>>>> + *
>>>> + * The function is called at the end of an online backup. It sends out
>>>> pg_control
>>>> + * file, optionally WAL segments and ending WAL location.
>>>> + */
>>>>
>>>> Comments seem out-dated.
>>>>
>>>
Fixed.