Re: Move postgresql_fdw_validator into dblink

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>