Re: pgsql_fdw, FDW for PostgreSQL server

2012年2月17日6:08 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)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(at)kaigai(dot)gr(dot)jp>