Re: standalone backend PANICs during recovery

On Tue, Aug 30, 2016 at 11:00 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
>> Does the attached suit better then?
>
> Thinking about it some more ... what we actually need to prevent, AFAICS,
> is standby_mode becoming true in a standalone backend. If you don't set
> that, then the process will do PITR recovery, but I'm not aware that there
> is any problem with running that standalone, and maybe it's useful to
> allow it for debugging purposes. So I'm thinking more like
>
> /*
> * Check for recovery control file, and if so set up state for offline
> * recovery
> */
> readRecoveryCommandFile();
>
> + /*
> + * We don't support standby_mode in standalone backends; that
> + * requires other processes such as WAL receivers to be alive.
> + */
> + if (StandbyModeRequested && !IsUnderPostmaster)
> + ereport(FATAL, ...);
> +
> /*
> * Save archive_cleanup_command in shared memory so that other processes
> * can see it.
> */
>
> or we could put the check near the bottom of readRecoveryCommandFile.
> Not sure which placement is clearer.

I have spent some time playing with that and you are right. Only
standby_mode = on is able trigger a failure here, and the first one is
in WaitForWALToBecomeAvailable(). I'd just put the check at the end of
readRecoveryCommandFile(), this will avoid extra thinking should
readRecoveryCommandFile() be moved around. That's unlikely to happen
but it is a cheap insurance.

I have added a test on that using 001_stream_rep.pl, now that's not
actually a good idea to push this test as if it passes the execution
of postgres would just hang and prevent the test to continue to run
and move on. But it makes it easier for anybody looking at this patch
to test the pattern prevented here.
--
Michael