Thread: simplifying emode_for_corrupt_record

simplifying emode_for_corrupt_record

From
Robert Haas
Date:
On Mon, Jun 14, 2010 at 10:08 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Bruce Momjian <bruce@momjian.us> writes:
>> Magnus Hagander wrote:
>>> It means that we can't prevent people from configuring their tools to
>>> ignore important warning. We can't prevent them rom ignoring ERROR or
>>> FATAL either...
>
>> My point is that most tools are going to look at the tag first to
>> determine the severity of the message, and might even have
>> log_min_messages set to ignore warnings.
>
> Why is this discussion based on the idea that we have to cater to
> incorrectly written log-filtering apps?
>
> The correct log level for this message is LOG.  End of discussion.

I spend a little bit of time analyzing this today and it appears to me
that all of the calls to emode_for_corrupt_record() arrive via
ReadRecord(), which itself takes an emode argument that is always
passed by the caller as either LOG or PANIC.  Therefore, the effect of
the first "if" test in emode_for_corrupt_record() is to reduce the
logging level of messages coming from SR or the archive from LOG to
WARNING.  (WARNING would be higher in an interactive session, but not
here, per Tom's point.)  This seems clearly a bad idea, so I propose
to rip it out, which simplifies this function considerably.  Proposed
patch attached.  I wasn't totally sure what to do about the comments.

Assuming this change makes sense, there is still the question of
bounding the number of retries and eventually falling down hard, but
that's really a separate issue and I'll write a separate email about
it when I get my thoughts together.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Attachment

Re: simplifying emode_for_corrupt_record

From
Robert Haas
Date:
On Fri, Jun 25, 2010 at 5:07 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> I spend a little bit of time analyzing this today and it appears to me
> that all of the calls to emode_for_corrupt_record() arrive via
> ReadRecord(), which itself takes an emode argument that is always
> passed by the caller as either LOG or PANIC.  Therefore, the effect of
> the first "if" test in emode_for_corrupt_record() is to reduce the
> logging level of messages coming from SR or the archive from LOG to
> WARNING.  (WARNING would be higher in an interactive session, but not
> here, per Tom's point.)  This seems clearly a bad idea, so I propose
> to rip it out, which simplifies this function considerably.  Proposed
> patch attached.

Since this appears to be non-controversial, I'm going to go ahead and commit it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company