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

On Wed, Jan 27, 2016 at 9:36 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Jan 25, 2016 at 8:06 PM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
>> 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.
>
> On a first read-through, I see nothing in this patch to which I would
> want to object except for the fact that the comments and documentation
> need some work from a native speaker of English. It looks like what
> we discussed, and I think it's an improvement over what we have now.

Well, looking at this a bit more, it seems like the documentation
you've written here is really misplaced. The patch is introducing a
new facility that applies to both CustomScan and ForeignScan, but the
documentation is only to do with CustomScan. I think we need a whole
new chapter on extensible nodes, or something. I'm actually not
really keen on the fact that we keep adding SGML documentation for
this stuff; it seems like it belongs in a README in the source tree.
We don't explain nodes in general, but now we're going to have to try
to explain extensible nodes. How's that going to work?

I think you should avoid the call to GetExtensibleNodeMethods() in the
case where extnodename is NULL. On the other hand, I think that if
extnodename is non-NULL, all four methods should be required, so that
you don't have to check if (methods && methods->nodeRead) but just if
(extnodename) { methods = GetExtensibleNodeMethods(extnodename);
methods->nodeRead( ... ); }. That seems like it would be a bit
tidier.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company