Re: CustomScan support on readfuncs.c

> -----Original Message-----
> From: Kaigai Kouhei(海外 浩平)
> Sent: Friday, November 06, 2015 11:23 AM
> To: 'Robert Haas'
> Cc: Amit Kapila; Andres Freund; pgsql-hackers
> Subject: RE: [HACKERS] 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.
>
I tried to split the previous version into two portions.

- custom-scan-on-readfuncs.v2.patch
It allows to serialize/deserialize CustomScan node as discussed upthread.
Regarding of get_current_library_filename(), I keep this feature as
the previous version right now, because I have no good alternatives.

In this patch, the role of TextReadCustomScan callback is to clean out
any tokens generated by TextOutCustomScan. The CustomScan node itself
can be reconstructed with common portion because we expect custom_exprs
and custom_private have objects which are safe to copyObject().

- custom-scan-on-copyfuncs.v2.patch
This is the portion that supports a larger structure with CustomScan
on its head.
The role of TextReadCustomScan is changed. It reconstruct the private
structure that contains CustomScan by palloc(), and fill-up its private
fields by tokens read. The common field of CustomScan shall be filled
up by the core.
One other callback I added is NodeCopyCustomScan; that enables extension
to allocate a structure larger than CustomScan and fill-up its private
fields.
Anyway, I'll have further discussion for 2nd patch in another thread.

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