pgsql: Allow extensions to add new backup targets.

Hi Robert,

On Tue, Mar 15, 2022 at 05:33:12PM +0000, Robert Haas wrote:

Allow extensions to add new backup targets.

Commit 3500ccc39b0dadd1068a03938e4b8ff562587ccc allowed for base backup
targets, meaning that we could do something with the backup other than
send it to the client, but all of those targets had to be baked in to
the core code. This commit makes it possible for extensions to define
additional backup targets.

I have noticed that this commit produces a warning when building with
MSVC, as of the end of BaseBackupGetTargetHandle() when the target
cannot be found.  I guess that you'd better add a fake return NULL to
keep such compilers quiet about that, like that:
+++ b/src/backend/replication/basebackup_target.c
@@ -144,6 +144,9 @@ BaseBackupGetTargetHandle(char *target, char *target_detail)
    ereport(ERROR,
            (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
             errmsg("unrecognized target: \"%s\"", target)));
+
+   /* keep compiler quiet */
+   return NULL;
 }

Thanks,
--
Michael

On Wed, Mar 16, 2022 at 5:36 AM Michael Paquier <michael@paquier.xyz> wrote:

I have noticed that this commit produces a warning when building with
MSVC, as of the end of BaseBackupGetTargetHandle() when the target
cannot be found.  I guess that you'd better add a fake return NULL to
keep such compilers quiet about that, like that:
+++ b/src/backend/replication/basebackup_target.c
@@ -144,6 +144,9 @@ BaseBackupGetTargetHandle(char *target, char *target_detail)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("unrecognized target: \"%s\"", target)));
+
+   /* keep compiler quiet */
+   return NULL;
}

Done.

It sucks that I keep missing this point. And it also sucks that we
have to have this kind of stuff. :-(

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Mar 16, 2022 at 09:52:41AM -0400, Robert Haas wrote:

On Wed, Mar 16, 2022 at 5:36 AM Michael Paquier <michael@paquier.xyz> wrote:

I have noticed that this commit produces a warning when building with
MSVC, as of the end of BaseBackupGetTargetHandle() when the target
cannot be found.  I guess that you'd better add a fake return NULL to
keep such compilers quiet about that, like that:
+++ b/src/backend/replication/basebackup_target.c
@@ -144,6 +144,9 @@ BaseBackupGetTargetHandle(char *target, char *target_detail)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("unrecognized target: \"%s\"", target)));
+
+   /* keep compiler quiet */
+   return NULL;
}

Done.

It sucks that I keep missing this point. And it also sucks that we
have to have this kind of stuff. :-(

FYI - at some point, CI will catch these in advance, per Andres' suggestion on
the big "basebackup" thread.

https://github.com/justinpryzby/postgres/commit/451e522a26f2b9a62976834ef7848279514ca700

--
Justin

On Wed, Mar 16, 2022 at 09:52:41AM -0400, Robert Haas wrote:

It sucks that I keep missing this point.

So do I. No worries. Thanks for fixing it!
--
Michael

On Tue, Mar 15, 2022 at 7:33 PM Robert Haas <rhaas@postgresql.org> wrote:

Allow extensions to add new backup targets.

Commit 3500ccc39b0dadd1068a03938e4b8ff562587ccc allowed for base backup
targets, meaning that we could do something with the backup other than
send it to the client, but all of those targets had to be baked in to
the core code. This commit makes it possible for extensions to define
additional backup targets.

Patch by me, reviewed by Abhijit Menon-Sen.

Discussion: /messages/by-id/CA+TgmoaqvdT-u3nt+_kkZ7bgDAyqDB0i-+XOMmr5JN2Rd37hxw@mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/e4ba69f3f4a1b997aa493cc02e563a91c0f35b87

Modified Files
--------------
src/backend/replication/Makefile | 1 +
src/backend/replication/Makefile.orig | 49 ++++++
src/backend/replication/basebackup.c | 82 ++++------
src/backend/replication/basebackup_target.c | 238 ++++++++++++++++++++++++++++
src/include/replication/basebackup_target.h | 66 ++++++++
5 files changed, 381 insertions(+), 55 deletions(-)

This one has a tiny copy/paste error where the idenfiication comment
for basebackup_target.c claims it's basebackup_gzip.c. I've pushed a
fix.

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/

On Mon, Mar 21, 2022 at 6:38 AM Magnus Hagander <magnus@hagander.net> wrote:

This one has a tiny copy/paste error where the idenfiication comment
for basebackup_target.c claims it's basebackup_gzip.c. I've pushed a
fix.

Thanks. If that's the extent of the problems we're doing well!

--
Robert Haas
EDB: http://www.enterprisedb.com