Re: CustomScan in a larger structure (RE: CustomScan support on readfuncs.c)
От | Kouhei Kaigai |
---|---|
Тема | Re: CustomScan in a larger structure (RE: CustomScan support on readfuncs.c) |
Дата | |
Msg-id | 9A28C8860F777E439AA12E8AEA7694F8011A689B@BPXM15GP.gisp.nec.co.jp обсуждение исходный текст |
Ответы |
Re: CustomScan in a larger structure (RE: CustomScan
support on readfuncs.c)
(Robert Haas <robertmhaas@gmail.com>)
|
Список | 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? > The detail of these callbacks are not for end-users, administrators and so on except for core/extension developers. So, I loves idea not to have such a detailed description in SGML. How about an idea to have more detailed source code comments close to the definition of ExtensibleNodeMethods? I haven't seen the src/backend/nodes/README yet, and it has only 6 updates history from Postgres95 era. I guess people may forget to update README file if description is separately located from the implementation. > 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. > OK, I'll fix up. No need to have 'missing_ok' argument here. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei <kaigai@ak.jp.nec.com>
В списке pgsql-hackers по дате отправления: