Re: termination of backend waiting for sync rep generates a junk log message

On Tue, Oct 18, 2011 at 5:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Oct 17, 2011 at 6:53 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> The simple fix is to change InteractiveBackend() so that it calls
>>> CHECK_FOR_INTERRUPTS() before it outputs "backend> ". Thought?
>
>> I'm tempted to say we should do that in PostgresMain() instead, maybe
>> something like this:
>
>> diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
>> index 976a832..9e5557c 100644
>> --- a/src/backend/tcop/postgres.c
>> +++ b/src/backend/tcop/postgres.c
>> @@ -3769,6 +3769,8 @@ PostgresMain(int argc, char *argv[], const char *username)
>>                 MemoryContextSwitchTo(MessageContext);
>>                 MemoryContextResetAndDeleteChildren(MessageContext);
>
>> +               CHECK_FOR_INTERRUPTS();
>> +
>>                 initStringInfo(&input_message);
>
>>                 /*
>
> I don't like putting a CHECK_FOR_INTERRUPTS there, because it's way too
> late to throw an error for the previous query.
>
> The real problem here is probably that we're overloading the meaning of
> whereToSendOutput.  The reset of that variable during shutdown was only
> ever meant to prevent output from being sent to a no-longer-present
> client.  It should *not* result in trying to read a query from stdin.

I think you're right, but am not sure how to fix it.

> Another question worth asking is how is it that we're getting to
> ReadCommand at all, if we have already determined that the client is
> gone.  Fixing that with an additional CHECK_FOR_INTERRUPTS seems like
> a crock.

We haven't determined the client is gone; we're trying to close the
connection "unexpectedly".  As the comment in SyncRepWaitForLSN
explains:
               /*                * If a wait for synchronous replication is pending,
we can neither                * acknowledge the commit nor raise ERROR or FATAL.
The latter would                * lead the client to believe that that the
transaction aborted, which                * is not true: it's already committed locally. The
former is no good                * either: the client has requested synchronous
replication, and is                * entitled to assume that an acknowledged commit is
also replicated,                * which might not be true. So in this case we issue a
WARNING (which                * some clients may be able to interpret) and shut off
further output.                * We do NOT reset ProcDiePending, so that the process
will die after                * the commit is cleaned up.                */

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