Re: Move postgresql_fdw_validator into dblink

Hanada-san,

The attached patch is a revised one according to my previous
suggestion. It re-defines "PQconninfoOption *options" as static
variable with NULL initial value, then, PQconndefaults() shall
be invoked at once. The default options never changed during
duration of the backend process, so here is no reason why we
allocate and free this object for each validator invocation.

If it is also OK for you, I'd like to take over this patch to comitter.
This patch is prerequisite of postgresql_fdw, so I hope this patch
getting merged soon.

Thanks,

2012/9/20 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> Hanada-san,
>
> I checked your patch. It can be applied to the latest master branch
> without any conflicts, and regression tests were fine.
>
> Unlike the original postgresql_fdw_validator(), the new
> dblink_fdw_validator() has wise idea; that pulls list of connection
> options from libpq, instead of self-defined static table.
> I basically agree not to have multiple source of option list.
>
> + /*
> + * Get list of valid libpq options. It contains default values too, but we
> + * don't care the values. Obtained list is allocated with malloc, so we
> + * must free it before leaving this function.
> + */
> + options = PQconndefaults();
> + if (options == NULL)
> + ereport(ERROR,
> + (errcode(ERRCODE_FDW_OUT_OF_MEMORY),
> + errmsg("out of memory"),
> + errdetail("could not get libpq default connection options")));
>
> I doubt whether PQconndefaults needs to be invoked for each
> validator calls. At least, supported option list of libpq should be
> never changed during run-time. So, I think PQconndefaults()
> should be called only once at first invocation, then option list
> can be stored in static variables or somewhere.
> As source code comments says, it is allocated with malloc, thus,
> we don't worry about unintentional release. :-)
>
> I don't think other part of this patch is arguable.
>
> Thanks,
>
> 2012/9/13 Shigeru HANADA <shigeru(dot)hanada(at)gmail(dot)com>:
>> Kaigai-san,
>>
>> (2012/09/13 16:56), Kohei KaiGai wrote:
>>> What about your plan to upstream contrib/pgsql_fdw module on the upcoming
>>> commit-fest?
>>
>> I will post pgsql_fdw patch (though it will be renamed to
>> postgresql_fdw) in opening CF (2012 Sep), as soon as I resolve a bug in
>> ANALYZE support, maybe on tomorrow. :-)
>>
>>> Even though I understand the point I noticed (miss-synchronization of sub-
>>> transaction block between local and remote side) is never easy problem to
>>> solve, it is worth to get the patch on the table of discussion.
>>> In my opinion, it is an idea to split-off the transaction control portion as
>>> a limitation of current version. For example, just raise an error when
>>> the foreign-table being referenced in sub-transaction block; with explicit
>>> description in the document.
>>
>> I agree not to support synchronize TX block between remote and local, at
>> least in next CF (I mean keeping remote TX open until local COMMIT or
>> ABORT). It would require 2PC and many issues to be solved, so I'd like
>> to focus fundamental part first. OTOH, using foreign tables in
>> sub-transaction seems essential to me.
>>
>>> Anyway, let me pick up your patch for reviewing. And, I hope you to prepare
>>> contrib/pgsql_fdw patch based on this patch.
>>
>> Thanks for your volunteer :-)
>>
>> Regards,
>> --
>> Shigeru HANADA
>
>
>
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

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