Re: base backup client as auxiliary backend process

Updated patch attached.

On 2019-09-18 10:31, Michael Paquier wrote:
> -    * 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.

Done.  Yes, this was really a change made to get more precise error 
messaged during debugging.  It could be committed separately.

> 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.

Is this something that the walreceiver should be doing independent of 
this patch?

> 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.

The updated has support for tablespaces without mapping.  I'm thinking 
about putting the mapping specification into a GUC list somehow. 
Shouldn't be too hard.

>    */
> -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.

Hmm, maybe long-term, but it seems too much dangerous surgery for this 
patch.

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

Fixed by renaming a function in pg_resetwal.c.

> +       /*
> +        * 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 :)

Hmm, why not?  What's the replacement?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services