On Wed, Jan 27, 2016 at 9:36 PM, Robert Haas <robertmh...@gmail.com> wrote: > On Mon, Jan 25, 2016 at 8:06 PM, Kouhei Kaigai <kai...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers