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

> On Wed, Dec 2, 2015 at 4:06 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2015-12-02 08:52:20 +0000, Kouhei Kaigai wrote:
> >> diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
> >> index 26264cb..c4bb76e 100644
> >> --- a/src/backend/nodes/copyfuncs.c
> >> +++ b/src/backend/nodes/copyfuncs.c
> >> @@ -635,8 +635,12 @@ _copyWorkTableScan(const WorkTableScan *from)
> >> static ForeignScan *
> >> _copyForeignScan(const ForeignScan *from)
> >> {
> >> - ForeignScan *newnode = makeNode(ForeignScan);
> >> -
> >> + const ExtensibleNodeMethods *methods
> >> + = GetExtensibleNodeMethods(from->extnodename, true);
> >> + ForeignScan *newnode = (ForeignScan *) newNode(!methods
> >>
> +
> ? sizeof(ForeignScan)
> >>
> +
> : methods->node_size,
> >> +
> T_ForeignScan);
> >
> > Changing the FDW API seems to put this squarely into 9.6
> > territory. Agreed?
>
> I don't think anybody thought this patch was 9.5 material. I didn't,
> anyway. The FDW changes for the join-pushdown-EPQ problem are another
> issue, but that can be discussed on the other thread.
>
I don't expect it is 9.5 feature.
The name of attached patch was "pgsql-v9.6-custom-private.v2.patch".

> >> /*
> >> * copy node superclass fields
> >> */
> >> @@ -645,6 +649,7 @@ _copyForeignScan(const ForeignScan *from)
> >> /*
> >> * copy remainder of node
> >> */
> >> + COPY_STRING_FIELD(extnodename);
> >> COPY_SCALAR_FIELD(fs_server);
> >> COPY_NODE_FIELD(fdw_exprs);
> >> COPY_NODE_FIELD(fdw_private);
> >> @@ -652,6 +657,8 @@ _copyForeignScan(const ForeignScan *from)
> >> COPY_NODE_FIELD(fdw_recheck_quals);
> >> COPY_BITMAPSET_FIELD(fs_relids);
> >> COPY_SCALAR_FIELD(fsSystemCol);
> >> + if (methods && methods->nodeCopy)
> >> + methods->nodeCopy((Node *) newnode, (const Node *) from);
> >
> > I wonder if we shouldn't instead "recurse" into a generic routine for
> > extensible nodes here.
>
> I don't know what that means.
>
> >> +void
> >> +RegisterExtensibleNodeMethods(const ExtensibleNodeMethods *methods)
> >> +{
> >> + uint32 hash;
> >> + int index;
> >> + ListCell *lc;
> >> + MemoryContext oldcxt;
> >> +
> >> + if (!xnodes_methods_slots)
> >> + xnodes_methods_slots =
> MemoryContextAllocZero(TopMemoryContext,
> >> +
> sizeof(List *) * XNODES_NUM_SLOTS);
> >> +
> >> + hash = hash_any(methods->extnodename, strlen(methods->extnodename));
> >> + index = hash % XNODES_NUM_SLOTS;
> >> +
> >> + /* duplication check */
> >> + foreach (lc, xnodes_methods_slots[index])
> >> + {
> >> + const ExtensibleNodeMethods *curr = lfirst(lc);
> >> +
> >> + if (strcmp(methods->extnodename, curr->extnodename) == 0)
> >> + ereport(ERROR,
> >> + (errcode(ERRCODE_DUPLICATE_OBJECT),
> >> + errmsg("ExtensibleNodeMethods
> \"%s\" already exists",
> >> +
> methods->extnodename)));
> >> + }
> >> + /* ok, register this entry */
> >> + oldcxt = MemoryContextSwitchTo(TopMemoryContext);
> >> + xnodes_methods_slots[index] = lappend(xnodes_methods_slots[index],
> >> +
> (void *)methods);
> >> + MemoryContextSwitchTo(oldcxt);
> >> +}
> >
> > Why aren't you using dynahash here, and instead reimplement a hashtable?
>
> I had thought we might assume that the number of extensible nodes was
> small enough that we could use an array for this and just use linear
> search. But if we want to keep the door open to having enough
> extensible nodes that this would be inefficient, then I agree that
> dynahash is better than a hand-rolled hash table.
>
Hmm. I also expected the number of extensible nodes are not so much,
so self-defined hash table is sufficient. However, it is not strong
reason. Let me revise the hash implementation.

> >> static ForeignScan *
> >> _readForeignScan(void)
> >> {
> >> + const ExtensibleNodeMethods *methods;
> >> READ_LOCALS(ForeignScan);
> >>
> >> ReadCommonScan(&local_node->scan);
> >>
> >> + READ_STRING_FIELD(extnodename);
> >> READ_OID_FIELD(fs_server);
> >> READ_NODE_FIELD(fdw_exprs);
> >> READ_NODE_FIELD(fdw_private);
> >> @@ -1804,6 +1806,19 @@ _readForeignScan(void)
> >> READ_BITMAPSET_FIELD(fs_relids);
> >> READ_BOOL_FIELD(fsSystemCol);
> >>
> >> + methods = GetExtensibleNodeMethods(local_node->extnodename, true);
> >> + if (methods && methods->nodeRead)
> >> + {
> >> + ForeignScan *local_temp =
> palloc0(methods->node_size);
> >> +
> >> + Assert(methods->node_size >= sizeof(ForeignScan));
> >> +
> >> + memcpy(local_temp, local_node, sizeof(ForeignScan));
> >> + methods->nodeRead((Node *) local_temp);
> >> +
> >> + pfree(local_node);
> >> + local_node = local_temp;
> >> + }
> >> READ_DONE();
> >> }
> >
> > This imo pretty clearly shows why the 'extensible node' handling should
> > be delegated to separate routines.
>
> This does look ugly. I'm not sure off-hand how to fix it.
>
The problem is that we cannot know what extensible node will fit on the
node currently we read unless we don't read extnodename. It also means we
cannot know exact size of the structure prior to the read of extnodename.

I can agree that the above code is never graceful, however, it is not an
avoidable restriction.

> > I wonder if we shouldn't, before committing this, at least write a PoC
> > implementation of actual extensible nodes. I.e. new node types that are
> > registered at runtime by extensions. ISTM those are relatively closely
> > linked, and the above doesn't yet support them nicely afaics.
>
> How is what you have in mind different from the pgstrom patch KaiGai attached?
>
This example make node copy by copyObject, then serialize and deserialize
the CustomScan node including extra private fields. I attached it for the
purpose of PoC.

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