From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com> |
Cc: | Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-committers(at)postgresql(dot)org |
Subject: | Re: pgsql: pg_ctl: Detect current standby state from pg_control |
Date: | 2016-09-26 12:53:51 |
Message-ID: | 27012.1474894431@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-committers pgsql-hackers |
Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:
> On Mon, Sep 26, 2016 at 12:54 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Coverity thinks that this patch introduced a bunch of
>> null-pointer-dereference hazards, and AFAICS it is right.
>> The change in get_controlfile()'s API is completely broken
>> and needs to be undone.
> So you'd rather have a sleep(1) and complicate this code to replace it
> with a WaitLatch() when the code is used by the backend? I don't agree
> with that as the new API is cleaner as presented, though I agree that
> it is a change that caller does not get any result in case of a CRC
> mismatch, but who's going to use results that cannot be trusted
> anyway?
Well, they certainly won't be using any untrustworthy reports if
pg_controldata dumps core instead of printing the data, which is what
will happen in HEAD. Although I don't understand your reference to
needing a sleep. 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.
> It seems to me that the correct fix here is that
> pg_controldata.c should just exit like in the attached patch.
No. Removing pg_controldata's designed-in ability to print information
from corrupted pg_control files was not part of the advertised impact of
this patch, and I will absolutely not hold still for it happening as an
accidental consequence of ill-advised refactoring.
It may well be that you need two different APIs for get_controlfile(),
maybe with a flag, or two different wrappers around some common code.
Or just go back to using #ifdef FRONTEND, and forget the silliness of
expecting pg_ctl not to throw an error for bad pg_control content.
BTW, now that get_controlfile has become a global symbol rather than
something private to pg_controldata.c, I suggest that it needs a less
generic name. "control file" could apply to a lot of things.
Possibly "get_pg_control_contents" would be better.
regards, tom lane
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2016-09-26 15:50:51 | pgsql: Document has_type_privilege(). |
Previous Message | Heikki Linnakangas | 2016-09-26 07:56:48 | pgsql: Refactor script execution state machine in pgbench. |
From | Date | Subject | |
---|---|---|---|
Next Message | Ryan Murphy | 2016-09-26 12:57:42 | Re: proposal: psql \setfileref |
Previous Message | Ashutosh Bapat | 2016-09-26 12:45:46 | Re: Aggregate Push Down - Performing aggregation on foreign server |