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