On 2020/03/19 0:37, Magnus Hagander wrote:
On Wed, Mar 11, 2020 at 5:53 AM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
On 2020/03/11 3:39, Magnus Hagander wrote:
On Tue, Mar 10, 2020 at 6:19 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
On 2020/03/10 22:43, Amit Langote wrote:
On Tue, Mar 10, 2020 at 6:09 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
So, I will make the patch adding support for --no-estimate-size option
in pg_basebackup.
Patch attached.
Like the idea and the patch looks mostly good.
Thanks for reviewing the patch!
+ total size. If the estimation is disabled in
+ <application>pg_basebackup</application>
+ (i.e., <literal>--no-estimate-size</literal> option is specified),
+ this is always <literal>0</literal>.
"always" seems unnecessary.
Fixed.
+ This option prevents the server from estimating the total
+ amount of backup data that will be streamed. In other words,
+ <literal>backup_total</literal> column in the
+ <structname>pg_stat_progress_basebackup</structname>
+ view always indicates <literal>0</literal> if this option is enabled.
Here too.
Fixed.
Attached is the updated version of the patch.
Would it perhaps be better to return NULL instead of 0 in the
statistics view if there is no data?
Did you miss this comment, or not agree? :)
Oh, I forgot to attached the patch... Patch attached.
This patch needs to be applied after applying
add_no_estimate_size_v3.patch.
Also, should it really be the server version that decides how this
feature behaves, and not the pg_basebackup version? Given that the
implementation is entirely in the client, it seems that's more
logical?
Yeah, you're right. I changed the patch that way.
Attached is the updated version of the patch.
The other changes in it look good!
Thanks for the review!
Regards,
--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 221acbe3ec..cf74e3da45 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -4403,7 +4403,7 @@ SELECT pg_stat_get_backend_pid(s.backendid) AS pid,
total size. If the estimation is disabled in
<application>pg_basebackup</application>
(i.e., <literal>--no-estimate-size</literal> option is specified),
- this is <literal>0</literal>.
+ this is <literal>NULL</literal>.
</entry>
</row>
<row>
diff --git a/doc/src/sgml/ref/pg_basebackup.sgml
b/doc/src/sgml/ref/pg_basebackup.sgml
index 90638aad0e..c8e040bacf 100644
--- a/doc/src/sgml/ref/pg_basebackup.sgml
+++ b/doc/src/sgml/ref/pg_basebackup.sgml
@@ -546,7 +546,7 @@ PostgreSQL documentation
amount of backup data that will be streamed, resulting in the
<literal>backup_total</literal> column in the
<structname>pg_stat_progress_basebackup</structname>
- to be <literal>0</literal>.
+ to be <literal>NULL</literal>.
</para>
<para>
Without this option, the backup will start by enumerating
diff --git a/src/backend/catalog/system_views.sql
b/src/backend/catalog/system_views.sql
index b8a3f46912..5a6dc61630 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -1070,7 +1070,7 @@ CREATE VIEW pg_stat_progress_basebackup AS
WHEN 4 THEN 'waiting for wal archiving to finish'
WHEN 5 THEN 'transferring wal files'
END AS phase,
- S.param2 AS backup_total,
+ CASE S.param2 WHEN -1 THEN NULL ELSE S.param2 END AS backup_total,
S.param3 AS backup_streamed,
S.param4 AS tablespaces_total,
S.param5 AS tablespaces_streamed
diff --git a/src/backend/replication/basebackup.c
b/src/backend/replication/basebackup.c
index 806d013108..a2e28b064c 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -123,7 +123,10 @@ static long long int total_checksum_failures;
/* Do not verify checksums. */
static bool noverify_checksums = false;
-/* Total amount of backup data that will be streamed */
+/*
+ * Total amount of backup data that will be streamed.
+ * -1 means that the size is not estimated.
+ */
static int64 backup_total = 0;
/* Amount of backup data already streamed */
@@ -258,6 +261,18 @@ perform_base_backup(basebackup_options *opt)
backup_streamed = 0;
pgstat_progress_start_command(PROGRESS_COMMAND_BASEBACKUP, InvalidOid);
+ /*
+ * If the estimation of the total backup size is disabled, make the
+ * backup_total column in the view return NULL by setting the parameter
to
+ * -1.
+ */
+ if (!opt->progress)
+ {
+ backup_total = -1;
+ pgstat_progress_update_param(PROGRESS_BASEBACKUP_BACKUP_TOTAL,
+
backup_total);
+ }
+
datadirpathlen = strlen(DataDir);
backup_started_in_recovery = RecoveryInProgress();
@@ -1842,7 +1857,7 @@ update_basebackup_progress(int64 delta)
* will always be wrong if WAL is included), but that's better than
having
* the done column be bigger than the total.
*/
- if (backup_total > 0 && backup_streamed > backup_total)
+ if (backup_total > -1 && backup_streamed > backup_total)
{
backup_total = backup_streamed;
val[nparam++] = backup_total;
diff --git a/src/test/regress/expected/rules.out
b/src/test/regress/expected/rules.out
index c7304611c3..a2077bbad4 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1886,7 +1886,10 @@ pg_stat_progress_basebackup| SELECT s.pid,
WHEN 5 THEN 'transferring wal files'::text
ELSE NULL::text
END AS phase,
- s.param2 AS backup_total,
+ CASE s.param2
+ WHEN '-1'::integer THEN NULL::bigint
+ ELSE s.param2
+ END AS backup_total,
s.param3 AS backup_streamed,
s.param4 AS tablespaces_total,
s.param5 AS tablespaces_streamed