Thread: WIP/PoC for parallel backup

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?

Greetings,

* Asif Rehman (asifr.rehman@gmail.com) wrote:
> On Fri, Aug 23, 2019 at 3:18 PM Asim R P <apraveen@pivotal.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

Greetings,

* Asif Rehman (asifr.rehman@gmail.com) wrote:
> On Fri, Aug 23, 2019 at 3:18 PM Asim R P <apraveen@pivotal.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

Hi Robert,

Thanks for the feedback. Please see the comments below:

On Tue, Sep 24, 2019 at 10:53 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Aug 21, 2019 at 9:53 AM Asif Rehman <asifr.rehman@gmail.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 

On Mon, Oct 7, 2019 at 8:48 AM Asif Rehman <asifr.rehman@gmail.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.

On Mon, Oct 7, 2019 at 8:48 AM Asif Rehman <asifr.rehman@gmail.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

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)

On Thu, Oct 17, 2019 at 10:51 AM Asif Rehman <asifr.rehman@gmail.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.chalke@enterprisedb.com> wrote:

On Thu, Oct 17, 2019 at 10:51 AM Asif Rehman <asifr.rehman@gmail.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.ahmad@gmail.com> wrote:

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

On Thu, Oct 17, 2019 at 10:51 AM Asif Rehman <asifr.rehman@gmail.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 Mon, Oct 28, 2019 at 8:29 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Oct 28, 2019 at 10:03 AM Asif Rehman <asifr.rehman@gmail.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.

On Thu, Nov 28, 2019 at 12:57 AM Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Nov 27, 2019 at 3:38 AM Jeevan Chalke
<jeevan.chalke@enterprisedb.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 

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@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"

On Mon, Mar 16, 2020 at 11:08 AM Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.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@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

Thanks Asif,

I have re-verified reported issue. expect standby backup, others are fixed.

Thanks & Regards,

Rajkumar Raghuwanshi

On Fri, Mar 27, 2020 at 11:04 PM Asif Rehman <asifr.rehman@gmail.com> wrote:

On Wed, Mar 25, 2020 at 12:22 PM Rajkumar Raghuwanshi <rajkumar.raghuwanshi@enterprisedb.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@localhost bin]$ ./psql postgres -p 5432 -c "select pg_is_in_recovery();"
 pg_is_in_recovery
-------------------
 f
(1 row)

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

[edb@localhost bin]$ ./pg_basebackup -p 5433 -D /tmp/bkp_s --jobs 6
pg_basebackup: error: could not list backup files: ERROR:  the standby was promoted during online backup
HINT:  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@localhost bin]$ ./pg_basebackup -p 5433 -D /tmp/bkp_s --jobs 1
[edb@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.raghuwanshi@enterprisedb.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@localhost bin]$ ./pg_ctl -D data -l logfile start
waiting for server to start.... done
server started
[edb@localhost bin]$
[edb@localhost bin]$ mkdir /tmp/tblsp
[edb@localhost bin]$ ./psql postgres -p 5432 -c "create tablespace tblsp location '/tmp/tblsp';"
CREATE TABLESPACE
[edb@localhost bin]$ ./psql postgres -p 5432 -c "create database testdb tablespace tblsp;"
CREATE DATABASE
[edb@localhost bin]$ ./psql testdb -p 5432 -c "create table testtbl (a text);"
CREATE TABLE
[edb@localhost bin]$ ./psql testdb -p 5432 -c "insert into testtbl values ('parallel_backup with tablespace');"
INSERT 0 1
[edb@localhost bin]$ ./pg_basebackup -p 5432 -D /tmp/bkp -T /tmp/tblsp=/tmp/tblsp_bkp --jobs 2
[edb@localhost bin]$ ./pg_ctl -D /tmp/bkp -l /tmp/bkp_logs -o "-p 5555" start
waiting for server to start.... done
server started
[edb@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@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@localhost bin]$
[edb@localhost bin]$ ls data/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION
data/pg_tblspc/16384/PG_13_202003051/16385/PG_VERSION
[edb@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.raghuwanshi@enterprisedb.com> wrote:

Hi Asif,

On testing further, I found when taking backup with -R, pg_basebackup crashed

this crash is not consistently reproducible.

[edb@localhost bin]$ ./psql postgres -p 5432 -c "create table test (a text);"

CREATE TABLE
[edb@localhost bin]$ ./psql postgres -p 5432 -c "insert into test values ('parallel_backup with -R recovery-conf');"
INSERT 0 1
[edb@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@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.chalke@enterprisedb.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.

 

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.

Yes, we are fetching a single file. However, SEND_FILES is still capable of fetching multiple files in one

go, that's why the name.

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.

patch is updated to add support for the Windows platform.

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

Done.

 

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;

Done.

 

Why free_filelist() is not called in PB_FETCH_WAL_FILES case?

Done.

The corrupted tablespace and crash, reported by Rajkumar, have been fixed. A pointer

variable remained uninitialized which in turn caused the system to misbehave.

Attached is the updated set of patches. AFAIK, to complete parallel backup feature

set, there remain three sub-features:

1- parallel backup does not work with a standby server. In parallel backup, the server

spawns multiple processes and there is no shared state being maintained. So currently,

no way to tell multiple processes if the standby was promoted during the backup since

the START_BACKUP was called.

2- throttling. Robert previously suggested that we implement throttling on the client-side.

However, I found a previous discussion where it was advocated to be added to the

backend instead[1].

So, it was better to have a consensus before moving the throttle function to the client.

That’s why for the time being I have disabled it and have asked for suggestions on it

to move forward.

It seems to me that we have to maintain a shared state in order to support taking backup

from standby. Also, there is a new feature recently committed for backup progress

reporting in the backend (pg_stat_progress_basebackup). This functionality was recently

added via this commit ID: e65497df. For parallel backup to update these stats, a shared

state will be required.

Since multiple pg_basebackup can be running at the same time, maintaining a shared state

can become a little complex, unless we disallow taking multiple parallel backups.

So proceeding on with this patch, I will be working on:

- throttling to be implemented on the client-side.
- adding a shared state to handle backup from the standby.

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

--

Asif Rehman

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