Skip site navigation (1) Skip section navigation (2)

Re: pgsql_fdw, FDW for PostgreSQL server

From: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
To: Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>
Cc: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>, Hitoshi Harada <umi(dot)tanuki(at)gmail(dot)com>, Martijn van Oosterhout <kleptog(at)svana(dot)org>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pgsql_fdw, FDW for PostgreSQL server
Date: 2012-02-15 15:09:57
Message-ID: CADyhKSUjv-+Rmx=E=A_xDC54_Z21p0z5Pwavd7Rs3u7U5XhpKQ@mail.gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
Harada-san,

I checked the v9 patch, however, it still has some uncertain implementation.

[memory context of tuple store]
It calls tuplestore_begin_heap() under the memory context of
festate->scan_cxt at pgsqlBeginForeignScan.
On the other hand, tuplestore_gettupleslot() is called under the
memory context of festate->tuples.
I could not find a callback functions being invoked on errors,
so I doubt the memory objects acquired within tuplestore_begin_heap()
shall be leaked, even though it is my suggestion to create a sub-context
under the existing one.

In my opinion, it is a good choice to use es_query_cxt of the supplied EState.
What does prevent to apply this per-query memory context?

You mention about PGresult being malloc()'ed. However, it seems to me
fetch_result() and store_result() once copy the contents on malloc()'ed
area to the palloc()'ed area, and PQresult is released on an error using
PG_TRY() ... PG_CATCH() block.

[Minor comments]
Please set NULL to "sql" variable at begin_remote_tx().
Compiler raises a warnning due to references of uninitialized variable,
even though the code path never run.

It potentially causes a problem in case when fetch_result() raises an
error because of unexpected status (!= PGRES_TUPLES_OK).
One code path is not protected with PG_TRY(), and other code path
will call PQclear towards already released PQresult.

Although it is just a preference of mine, is the exprFunction necessary?
It seems to me, the point of push-down check is whether the supplied
node is built-in object, or not. So, an sufficient check is is_builtin() onto
FuncExpr->funcid, OpExpr->opno, ScalarArrayOpExpr->opno and so on.
It does not depend on whether the function implementing these nodes
are built-in or not.

Thanks,

2012年2月14日9:09 Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com>:
> (2012/02/14 15:15), Shigeru Hanada wrote:
>> Good catch, thanks.  I'll revise pgsql_fdw tests little more.
>
> Here are the updated patches.  In addition to Fujita-san's comment, I
> moved DROP OPERATOR statements to clean up section of test script.
>
> Regards,
> --
> Shigeru Hanada



-- 
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

In response to

Responses

pgsql-hackers by date

Next:From: Bruce MomjianDate: 2012-02-15 15:14:28
Subject: Re: pg_test_fsync performance
Previous:From: Robert HaasDate: 2012-02-15 14:59:50
Subject: Re: [trivial patch] typo in doc/src/sgml/sepgsql.sgml

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group