simplifying emode_for_corrupt_record

On Mon, Jun 14, 2010 at 10:08 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Bruce Momjian <bruce(at)momjian(dot)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