Re: WIP/PoC for parallel backup

From: Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com>
To: asifr(dot)rehman(at)gmail(dot)com
Cc: Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: WIP/PoC for parallel backup
Date: 2020-03-16 08:43:48
Message-ID: CAM2+6=W+hBp2+dNB=8BZqLOHgps6dk73DJvocw1QZiKHs=C9Xg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message tushar 2020-03-16 08:58:17 Re: [Proposal] Global temporary tables
Previous Message Pavel Stehule 2020-03-16 08:43:18 Re: [HACKERS] [PATCH] Generic type subscripting