Re: base backup client as auxiliary backend process

On Fri, Aug 30, 2019 at 09:10:10PM +0200, Peter Eisentraut wrote:
> > Attached is a very hackish patch to implement this.  It works like this:
> >
> >     # (assuming you have a primary already running somewhere)
> >     initdb -D data2 --replica
> >     $EDITOR data2/postgresql.conf  # set primary_conninfo
> >     pg_ctl -D data2 start
>
> Attached is an updated patch for this.  I have changed the initdb option
> name per suggestion.  The WAL receiver is now started concurrently with
> the base backup.  There is progress reporting (ps display), fsyncing.
> Configuration files are not copied anymore.  There is a simple test
> suite.  Tablespace support is still missing, but it would be
> straightforward.

I find this idea and this spec neat.

-    * Verify XLOG status looks valid.
+    * Check that contents look valid.
     */
-   if (ControlFile->state < DB_SHUTDOWNED ||
-       ControlFile->state > DB_IN_PRODUCTION ||
-       !XRecOffIsValid(ControlFile->checkPoint))
+   if (!XRecOffIsValid(ControlFile->checkPoint))
             ereport(FATAL,
Doesn't seem like a good idea to me to remove this sanity check for
normal deployments, but actually you moved that down in StartupXLOG().
It seems to me tha this is unrelated and could be a separate patch so
as the errors produced are more verbose.  I think that we should also
change that code to use a switch/case on ControlFile->state.

The current defaults of pg_basebackup have been thought so as the
backups taken have a good stability and so as monitoring is eased
thanks to --wal-method=stream and the use of replication slots.
Shouldn't the use of a least a temporary replication slot be mandatory
for the stability of the copy?  It seems to me that there is a good
argument for having a second process which streams WAL on top of the
main backup process, and just use a WAL receiver for that.

One problem which is not tackled here is what to do for the tablespace
map.  pg_basebackup has its own specific trick for that, and with that
new feature we may want something equivalent?  Not something to
consider as a first stage of course.

  */
-static void
+void
 WriteControlFile(void)
[...]
-static void
+void
 ReadControlFile(void)
[...]
If you begin to publish those routines, it seems to me that there
could be more consolidation with controldata_utils.c which includes
now a routine to update a control file.

+#ifndef FRONTEND
+extern void InitControlFile(uint64 sysidentifier);
+extern void WriteControlFile(void);
+extern void ReadControlFile(void);
+#endif
It would be nice to avoid that.

-extern char *cluster_name;
+extern PGDLLIMPORT char *cluster_name;
Separate patch here?

+   if (stat(BASEBACKUP_SIGNAL_FILE, &stat_buf) == 0)
+   {
+       int         fd;
+
+       fd = BasicOpenFilePerm(STANDBY_SIGNAL_FILE, O_RDWR |
PG_BINARY,
+                              S_IRUSR | S_IWUSR);
+       if (fd >= 0)
+       {
+           (void) pg_fsync(fd);
+           close(fd);
+       }
+       basebackup_signal_file_found = true;
+   }
I would put that in a different routine.

+       /*
+        * Wait until done.  Start WAL receiver in the meantime, once
base
+        * backup has received the starting position.
+        */
+       while (BaseBackupPID != 0)
+       {
+           PG_SETMASK(&UnBlockSig);
+           pg_usleep(1000000L);
+           PG_SETMASK(&BlockSig);

+   primary_sysid = strtoull(walrcv_identify_system(wrconn,
&primaryTLI), NULL, 10);
No more strtol with base 10 stuff please :)
--
Michael