Add backup_type to pg_stat_progress_basebackup

On Tue, 22 Jul 2025 17:48:35 +0900
Shinya Kato <shinya11.kato@gmail.com> wrote:

Hi hackers,

Starting with PostgreSQL 17, pg_basebackup supports incremental
backups. However, the pg_stat_progress_basebackup view doesn't
currently show the backup type (i.e., whether it's a full or
incremental backup).

Therefore, I propose adding a backup_type column to this view. While
this information is available in pg_stat_activity, the backup type is
important for monitoring the progress of pg_basebackup, and including
it directly in the progress view would be very useful.

Thoughts?

That seems reasonable to me.

Just one minor comment on the patch:

+
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>backup_type</structfield> <type>bigint</type>
+      </para>
+      <para>
+        Backup type. Either <literal>full</literal> or
+        <literal>incremental</literal>.
+      </para></entry>
+     </row>

The type should be text rather than bigint.

Regards,
Yugo Nagata

--
Yugo Nagata <nagata@sraoss.co.jp>

On Tue, Jul 22, 2025 at 6:06 PM Yugo Nagata <nagata@sraoss.co.jp> wrote:

On Tue, 22 Jul 2025 17:48:35 +0900
Shinya Kato <shinya11.kato@gmail.com> wrote:

Hi hackers,

Starting with PostgreSQL 17, pg_basebackup supports incremental
backups. However, the pg_stat_progress_basebackup view doesn't
currently show the backup type (i.e., whether it's a full or
incremental backup).

Therefore, I propose adding a backup_type column to this view. While
this information is available in pg_stat_activity, the backup type is
important for monitoring the progress of pg_basebackup, and including
it directly in the progress view would be very useful.

Thoughts?

That seems reasonable to me.

Just one minor comment on the patch:

+
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>backup_type</structfield> <type>bigint</type>
+      </para>
+      <para>
+        Backup type. Either <literal>full</literal> or
+        <literal>incremental</literal>.
+      </para></entry>
+     </row>

The type should be text rather than bigint.

Thank you for the review.
I made a careless mistake. Fixed.

--
Best regards,
Shinya Kato
NTT OSS Center

On Tue, Jul 22, 2025 at 2:42 AM Shinya Kato <shinya11.kato@gmail.com> wrote:

On Tue, Jul 22, 2025 at 6:06 PM Yugo Nagata <nagata@sraoss.co.jp> wrote:

On Tue, 22 Jul 2025 17:48:35 +0900
Shinya Kato <shinya11.kato@gmail.com> wrote:

Hi hackers,

Starting with PostgreSQL 17, pg_basebackup supports incremental
backups. However, the pg_stat_progress_basebackup view doesn't
currently show the backup type (i.e., whether it's a full or
incremental backup).

Therefore, I propose adding a backup_type column to this view. While
this information is available in pg_stat_activity, the backup type is
important for monitoring the progress of pg_basebackup, and including
it directly in the progress view would be very useful.

Thoughts?

That seems reasonable to me.

I like this idea.

Thank you for the review.
I made a careless mistake. Fixed.

The patch seems reasonably simple and looks good to me. I've updated
the comment in bbsink_progress_new() and attached the modified version
patch (with the commit message). Please review it.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Sat, Aug 2, 2025 at 8:12 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Jul 22, 2025 at 2:42 AM Shinya Kato <shinya11.kato@gmail.com> wrote:

On Tue, Jul 22, 2025 at 6:06 PM Yugo Nagata <nagata@sraoss.co.jp> wrote:

On Tue, 22 Jul 2025 17:48:35 +0900
Shinya Kato <shinya11.kato@gmail.com> wrote:

Hi hackers,

Starting with PostgreSQL 17, pg_basebackup supports incremental
backups. However, the pg_stat_progress_basebackup view doesn't
currently show the backup type (i.e., whether it's a full or
incremental backup).

Therefore, I propose adding a backup_type column to this view. While
this information is available in pg_stat_activity, the backup type is
important for monitoring the progress of pg_basebackup, and including
it directly in the progress view would be very useful.

Thoughts?

That seems reasonable to me.

I like this idea.

Thank you for reviewing my patch!

Thank you for the review.
I made a careless mistake. Fixed.

The patch seems reasonably simple and looks good to me. I've updated
the comment in bbsink_progress_new() and attached the modified version
patch (with the commit message). Please review it.

Thanks for the patch. I reviewed it, and LGTM.

--
Best regards,
Shinya Kato
NTT OSS Center

On Fri, 1 Aug 2025 16:12:15 -0700
Masahiko Sawada <sawada.mshk@gmail.com> wrote:

The patch seems reasonably simple and looks good to me. I've updated
the comment in bbsink_progress_new() and attached the modified version
patch (with the commit message). Please review it.

 	/*
-	 * Report that a base backup is in progress, and set the total size of the
-	 * backup to -1, which will get translated to NULL. If we're estimating
-	 * the backup size, we'll insert the real estimate when we have it.
+	 * Report that a base backup is in progress, and set the backup type and
+	 * the total size of the backup to -1, which will get translated to NULL,
+	 * and backup. If we're estimating the backup size, we'll insert the real
+	 * estimate when we have it.
 	 */

It seems to me that "set the backup type and the total size of the backup to -1"
is a bit confusing because it could be read that the backup type would be also set
to -1, and the subsequent sentence describes just the total size.

Therefore, I think it is better to just add "Also, the backup type is set."
(or similar) to the end of the comment block.

That said, I'm not a native English speaker, so if no one else sees a problem,
I'm fine with it.

Regards,
Yugo Nagata

--
Yugo Nagata <nagata@sraoss.co.jp>

On Mon, Aug 4, 2025 at 1:57 AM Yugo Nagata <nagata@sraoss.co.jp> wrote:

On Fri, 1 Aug 2025 16:12:15 -0700
Masahiko Sawada <sawada.mshk@gmail.com> wrote:

The patch seems reasonably simple and looks good to me. I've updated
the comment in bbsink_progress_new() and attached the modified version
patch (with the commit message). Please review it.

/*
-        * Report that a base backup is in progress, and set the total size of the
-        * backup to -1, which will get translated to NULL. If we're estimating
-        * the backup size, we'll insert the real estimate when we have it.
+        * Report that a base backup is in progress, and set the backup type and
+        * the total size of the backup to -1, which will get translated to NULL,
+        * and backup. If we're estimating the backup size, we'll insert the real
+        * estimate when we have it.
*/

It seems to me that "set the backup type and the total size of the backup to -1"
is a bit confusing because it could be read that the backup type would be also set
to -1, and the subsequent sentence describes just the total size.

Therefore, I think it is better to just add "Also, the backup type is set."
(or similar) to the end of the comment block.

That said, I'm not a native English speaker, so if no one else sees a problem,
I'm fine with it.

Agreed. I've changed the comment and pushed the patch.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Wed, Aug 6, 2025 at 3:06 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Aug 4, 2025 at 1:57 AM Yugo Nagata <nagata@sraoss.co.jp> wrote:

On Fri, 1 Aug 2025 16:12:15 -0700
Masahiko Sawada <sawada.mshk@gmail.com> wrote:

The patch seems reasonably simple and looks good to me. I've updated
the comment in bbsink_progress_new() and attached the modified version
patch (with the commit message). Please review it.

/*
-        * Report that a base backup is in progress, and set the total size of the
-        * backup to -1, which will get translated to NULL. If we're estimating
-        * the backup size, we'll insert the real estimate when we have it.
+        * Report that a base backup is in progress, and set the backup type and
+        * the total size of the backup to -1, which will get translated to NULL,
+        * and backup. If we're estimating the backup size, we'll insert the real
+        * estimate when we have it.
*/

It seems to me that "set the backup type and the total size of the backup to -1"
is a bit confusing because it could be read that the backup type would be also set
to -1, and the subsequent sentence describes just the total size.

Therefore, I think it is better to just add "Also, the backup type is set."
(or similar) to the end of the comment block.

That said, I'm not a native English speaker, so if no one else sees a problem,
I'm fine with it.

Agreed. I've changed the comment and pushed the patch.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Thanks for pushing it!

--
Best regards,
Shinya Kato
NTT OSS Center