Re: CustomScan in a larger structure (RE: CustomScan support on readfuncs.c)

Sorry for my late response. I've been unavailable to have enough
time to touch code for the last 1.5 month.

The attached patch is a revised one to handle private data of
foregn/custom scan node more gracefully.

The overall consensus upthread were:
- A new ExtensibleNodeMethods structure defines a unique name
and a set of callbacks to handle node copy, serialization,
deserialization and equality checks.
- (Foreign|Custom)(Path|Scan|ScanState) are first host of the
ExtensibleNodeMethods, to allow extension to define larger
structure to store its private fields.
- ExtensibleNodeMethods does not support variable length
structure (like a structure with an array on the tail, use
separately allocated array).
- ExtensibleNodeMethods shall be registered on _PG_init() of
extensions.

The 'pgsql-v9.6-custom-private.v3.patch' is the main part of
this feature. As I pointed out before, it uses dynhash instead
of the self invented hash table.

Interfaces are defined as follows (not changed from v2):

typedef struct ExtensibleNodeMethods
{
const char *extnodename;
Size node_size;
void (*nodeCopy)(Node *newnode, const Node *oldnode);
bool (*nodeEqual)(const Node *a, const Node *b);
void (*nodeOut)(struct StringInfoData *str, const Node *node);
void (*nodeRead)(Node *node);
} ExtensibleNodeMethods;

extern void
RegisterExtensibleNodeMethods(const ExtensibleNodeMethods *methods);

extern const ExtensibleNodeMethods *
GetExtensibleNodeMethods(const char *extnodename, bool missing_ok);

Also, 'extensible-node-example-on-pgstrom.patch' is a working
example on its "GpuScan" node.
The code below uses all of copy, serialization and deserialization.

gscan = (GpuScan *)stringToNode(nodeToString(copyObject(cscan)));
elog(INFO, "GpuScan: %s", nodeToString(gscan));

Then, I could confirm private fields are reproduced correctly.

In addition to this, I'd like to suggest two small improvement.

On nodeOut callback, extension will need _outToken() and _outBitmap(),
however, these two functions are static. Entrypoint for extensions
are needed. (Of course, extension can copy & paste these small functions...)

ExtensibleNodeMethods may be registered with a unique pair of its
name and node-tag which is associated with. The current code requires
the name is unique to others, however, it may make a bit inconvenience.
In case of CustomScan, extension need to define three nodes: CustomPath,
CustomScan and CustomScanState, thus, ExtensibleNodeMethods which is
associated with these node must have individually unique name, like
"GpuScanPath", "GpuScan" and "GpuScanState".
If extnodename would be unique within a particular node type, we can
apply same name for all of the three above.

How about your thought?

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

> -----Original Message-----
> From: Kaigai Kouhei(海外 浩平)
> Sent: Wednesday, December 02, 2015 5:52 PM
> To: 'Robert Haas'
> Cc: Andres Freund; Amit Kapila; pgsql-hackers
> Subject: Re: CustomScan in a larger structure (RE: [HACKERS] CustomScan support
> on readfuncs.c)
>
> > On Thu, Nov 26, 2015 at 5:27 AM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> > > I'm now implementing. The above design perfectly works on ForeignScan.
> > > On the other hands, I'd like to have deeper consideration for CustomScan.
> > >
> > > My recent patch adds LibraryName and SymbolName on CustomScanMethods
> > > to lookup the method table even if library is not loaded yet.
> > > However, this ExtensibleNodeMethods relies custom scan provider shall
> > > be loaded, by parallel infrastructure, prior to the deserialization.
> > > It means extension has a chance to register itself as well.
> > >
> > > My idea is, redefine CustomScanMethod as follows:
> > >
> > > typedef struct ExtensibleNodeMethods
> > > {
> > > const char *extnodename;
> > > Size node_size;
> > > Node *(*nodeCopy)(const Node *from);
> > > bool (*nodeEqual)(const Node *a, const Node *b);
> > > void (*nodeOut)(struct StringInfoData *str, const Node *node);
> > > void (*nodeRead)(Node *node);
> > > } ExtensibleNodeMethods;
> > >
> > > typedef struct CustomScanMethods
> > > {
> > > union {
> > > const char *CustomName;
> > > ExtensibleNodeMethods xnode;
> > > };
> > > /* Create execution state (CustomScanState) from a CustomScan plan node
> > */
> > > Node *(*CreateCustomScanState) (struct CustomScan *cscan);
> > > } CustomScanMethods;
> > >
> > > It allows to pull CustomScanMethods using GetExtensibleNodeMethods
> > > by CustomName; that is alias of extnodename in ExtensibleNodeMethods.
> > > Thus, we don't need to care about LibraryName and SymbolName.
> > >
> > > This kind of trick is not needed for ForeignScan because all the pointer
> > > stuff are reproducible by catalog, however, method table of CustomScan
> > > is not.
> > >
> > > How about your opinion?
> >
> > Anonymous unions are not C89, so this is a no-go.
> >
> > I think we should just drop CustomName and replace it with
> > ExtensibleNodeMethods. That will break things for third-party code
> > that is using the existing CustomScan stuff, but there's a good chance
> > that nobody other than you has written any such code. And even if
> > someone has, updating it for this change will not be very difficult.
> >
> Thanks, I have same opinion.
> At this moment, CustomName is not utilized so much except for EXPLAIN
> and debug output, so it is not a hard stuff to replace this field by
> extnodename, even if custom scan provider does not have own structure
> thus no callbacks of node operations are defined.
>
> The attached patch adds 'extnodename' fields on ForeignPath and
> ForeignScan, also embeds ExtensibleNodeMethods in CustomPathMethods,
> CustomScanMethods and CustomExecMethods.
>
> Its handlers in copyfuncs.c were enhanced to utilize the callback
> to allocate a larger structure and copy private fields.
> Handlers in outfuncs.c and readfuncs.c have to be symmetric. The
> core backend writes out 'extnodename' prior to any private fields,
> then we can identify the callback to process rest of tokens for
> private fields.
>
> RegisterExtensibleNodeMethods() tracks a pair of 'extnodename'
> and ExtensibleNodeMethods itself. It saves the pointer of the
> method table, but not duplicate, because _readCustomScan()
> cast the method pulled by 'extnodename' thus registered table
> has to be a static variable on extension; that means extension
> never update ExtensibleNodeMethods once registered.
>
> The one other patch is my test using PG-Strom, however, I didn't
> have a test on FDW extensions yet.
>
> Thanks,
> --
> NEC Business Creation Division / PG-Strom Project
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>