Re: pgsql_fdw, FDW for PostgreSQL server

2012年2月17日6:08 Shigeru Hanada <shigeru.hanada@gmail.com>:
> (2012/02/17 2:02), Kohei KaiGai wrote:
>> I found a strange behavior with v10. Is it available to reproduce?
> <snip>
>> I tried to raise an error on remote side.
>>
>>    postgres=# select * FROM ftbl WHERE 100 / (a - 3)>  0;
>>    The connection to the server was lost. Attempting reset: Failed.
>>    The connection to the server was lost. Attempting reset: Failed.
>>    !>  \q
>
> I could reproduce the error by omitting CFLAGS=-O0 from configure
> option.  I usually this for coding environment so that gdb debugging
> works correctly, so I haven't noticed this issue.  I should test
> optimized environment too...
>
> Expected result in that case is:
>
> postgres=# select * from pgbench_accounts where 100 / (aid - 3) > 0;
> ERROR:  could not fetch rows from foreign server
> DETAIL:  ERROR:  division by zero
>
> HINT:  FETCH 10000 FROM pgsql_fdw_cursor_0
> postgres=#
>
>> This is the PG_CATCH block at execute_query(). fetch_result() raises
>> an error, then it shall be catched to release PGresult.
>> Although "res" should be NULL at this point, PQclear was called with
>> a non-zero value according to the call trace.
>>
>> More strangely, I tried to inject elog(INFO, ...) to show the value of "res"
>> at this point. Then, it become unavailable to reproduce when I tried to
>> show the pointer of "res" with elog(INFO, "&res = %p",&res);
>>
>> Why the "res" has a non-zero value, even though it was cleared prior
>> to fetch_result() and an error was raised within this function?
>
> I've found the the problem is uninitialized PGresult variables.
> Uninitialized PGresult pointer is used in some places, so its value is
> garbage in PG_CATCH block when assignment code has been interrupted by
> longjmp.
>
> Probably recommended style would be like this:
>
> <pseudo_code>
>    PGresult *res = NULL;    /* must be NULL in PG_CATCH */
>
>    PG_TRY();
>    {
>        res = func_might_throw_exception();
>        if (PQstatus(res) != PGRES_xxx_OK)
>        {
>            /* error handling, pass message to caller */
>            ereport(ERROR, ...);
>        }
>
>        /* success case, use result of query and release it */
>        ...
>        PQclear(res);
>    }
>    PG_CATCH();
>    {
>        PQclear(res);
>        PG_RE_THROW();
>        /* caller should catch this exception. */
>    }
> </pseudo_code>
>
> I misunderstood that PGresult pointer always has valid value after that
> line, because I had wrote assignment of PGresult pointer before PG_TRY
> block.  Fixes for this issue are:
>
> (1) Initialize PGresult pointer with NULL, if it is used in PG_CATCH.
> (2) Move PGresult assignment into PG_TRY block so that we can get
> compiler warning of uninitialized  variable, just in case.
>
> Please find attached a patch including fixes for this issue.
>
I marked this patch as "Ready for Committer", since I have nothing to
comment any more.

I'd like committer help to review this patch and it get merged.

Thanks,
-- 
KaiGai Kohei <kaigai@kaigai.gr.jp>