Re: WIP/PoC for parallel backup

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