On Tue, Sep 27, 2016 at 9:45 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 9/26/16 7:56 PM, Peter Eisentraut wrote:
>> On 9/26/16 8:53 AM, Tom Lane wrote:
>>> I think that it's 100% pointless for get_control_dbstate
>>> to be worried about transient CRC failures. If writes to pg_control
>>> aren't atomic then we have problems enormously larger than whether
>>> "pg_ctl promote" throws an error or not.
>>
>> The new code was designed to handle that, but if we think it's not an
>> issue, then we can simplify it. I'm on it.
>
> How about this?
+ if (crc_ok_p)
+ *crc_ok_p = false;
+ else
+ {
+ pfree(ControlFile);
+ return NULL;
+ }
Seems overcomplicated to me. How about returning the control file all
the time and let the caller pfree the result? You could then use
crc_ok in pg_ctl.c's get_control_dbstate() to do the decision-making.
Also, if the sleep() loop is removed for promote -w, we assume that
we'd fail immediately in case of a corrupted control file. Could it be
possible that after a couple of seconds the backend gets it right and
overwrites a correct control file on top of a corrupted one? I am just
curious about such possibilities, still I am +1 for removing the
pg_usleep loop and failing right away as you propose.
If we do the renaming of get_controlfile(), it may be also a good idea
to do the same for get_configdata() in config_info.c for consistency.
That's not really critical IMHO though.
--
Michael