Horton

Thread

Commits

  1. Add backup_type column to pg_stat_progress_basebackup.

  1. Add backup_type to pg_stat_progress_basebackup

    Shinya Kato <shinya11.kato@gmail.com> — 2025-07-22T08:48:35Z

    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?


-- 
Best regards,
Shinya Kato
NTT OSS Center

  2. Re: Add backup_type to pg_stat_progress_basebackup

    Yugo Nagata <nagata@sraoss.co.jp> — 2025-07-22T09:06:52Z

    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>

  3. Re: Add backup_type to pg_stat_progress_basebackup

    Shinya Kato <shinya11.kato@gmail.com> — 2025-07-22T09:41:35Z

    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

  4. Re: Add backup_type to pg_stat_progress_basebackup

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-08-01T23:12:15Z

    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

  5. Re: Add backup_type to pg_stat_progress_basebackup

    Shinya Kato <shinya11.kato@gmail.com> — 2025-08-04T08:09:50Z

    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

  6. Re: Add backup_type to pg_stat_progress_basebackup

    Yugo Nagata <nagata@sraoss.co.jp> — 2025-08-04T08:57:49Z

    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>

  7. Re: Add backup_type to pg_stat_progress_basebackup

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-08-05T18:05:29Z

    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

  8. Re: Add backup_type to pg_stat_progress_basebackup

    Shinya Kato <shinya11.kato@gmail.com> — 2025-08-06T00:15:21Z

    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