Re: CustomScan in a larger structure (RE: CustomScan support on readfuncs.c) - Mailing list pgsql-hackers

From Robert Haas
Subject Re: CustomScan in a larger structure (RE: CustomScan support on readfuncs.c)
Date
Msg-id CA+TgmoYjVtcaXfXy5LypWGQUq2oRnyFVnP6eC7iL6uC78V6nvg@mail.gmail.com
Whole thread Raw
In response to Re: CustomScan in a larger structure (RE: CustomScan support on readfuncs.c)  (Robert Haas <robertmhaas@gmail.com>)
List pgsql-hackers
On Wed, Jan 27, 2016 at 9:36 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Jan 25, 2016 at 8:06 PM, Kouhei Kaigai <kaigai@ak.jp.nec.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



pgsql-hackers by date:

Previous
From: David Steele
Date:
Subject: Re: PostgreSQL Audit Extension
Next
From: Ashutosh Bapat
Date:
Subject: Re: [POC] FETCH limited by bytes.