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

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?

> /*
> * 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.
> +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?

> 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.

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.

Greetings,

Andres Freund