Re: CustomScan support on readfuncs.c

> On Wed, Nov 4, 2015 at 10:13 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> > Sorry for my late.
> >
> > I confirmed that CustomScan support of readfuncs.c works fine using the
> > attached patch for the PG-Strom tree. It worked as expected - duplication,
> > serialization and deserialization by:
> > plan_dup = stringToNode(nodeToString(copyObject(plan_orig)))
> >
> > So, the custom-scan-on-readfuncs.v1.path itself is good for me.
>
> I don't think this is is what we want. In particular, I like this:
>
> Plan trees must be able to be duplicated using <function>copyObject</>,
> - so all the data stored within the <quote>custom</> fields must consist of
> - nodes that that function can handle. Furthermore, custom scan providers
> - cannot substitute a larger structure that embeds
> - a <structname>CustomScan</> for the structure itself, as would be possible
> - for a <structname>CustomPath</> or <structname>CustomScanState</>.
> + serialized using <function>nodeToString</> and deserialized using
> + <function>stringToNode</>, so so all the data stored within
> + the <quote>custom</> fields must consist of nodes that that function
> + can handle.
> + Furthermore, custom scan providers have to implement <quote>optional</>
> + callbacks if it defines substitute a larger structure that embeds
> + a <structname>CustomScan</> for the structure itself.
> + </para>
>
> The purposes of this patch is supposedly to add readfuncs.c support to
> custom scans - I agree with that goal. This documentation change is
> talking about something else, namely using a CustomScan as the fist
> field in a larger structure. I'm not sure I agree with that goal, and
> in any case it seems like it ought to be a separate patch. This patch
> would be a lot smaller if it weren't trying to make that change too.
>
Hmm. I might mix up two individual discussions here.
OK, I'll cut off the minimum portion for readfuncs.c support.
The other portion - to use a CustomScan as the first field in a larger
structure - shall be submitted as a separate one.

> I also don't think that this patch ought to introduce
> get_current_library_filename(). There are other cases - e.g. for
> launching background workers - where such a thing could be used, but
> we haven't introduced it up until now. It isn't a requirement to get
> readfuncs support working.
>
Even though it isn't a minimum requirement (because extension can put
arbitrary cstring here), it can tell reasonably reliable pathname for
extensions if backend tracks filename of the library to be loaded.

Extension knows its name; its Makefile can embed module name somewhere,
for example. However, we cannot ensure use always install the modules
under the $libdir. Even if "$libdir/pg_strom" is expected, we have no
mechanism to prohibit users to install them under the "$libdir/buggy"
directory. In this case, "pg_strom" is not a right name to solve the
library path.

Even though glibc has dladdr(3) to know the filename that contains the
target symbol, it is not a portable way. So, I thought it is the best
way to inform extension by the core, on _PG_init() time where all the
extension get control once on loading.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>