On Wed, Feb 3, 2016 at 11:57 PM, Kouhei Kaigai <kai...@ak.jp.nec.com> wrote: > At this moment, I tried to write up description at nodes/nodes.h. > The amount of description is about 100lines. It is on a borderline > whether we split off this chunk into another header file, in my sense. > > > On the other hands, I noticed that we cannot omit checks for individual > callbacks on CustomXXXX node type, ExtensibleNodeMethods is embedded in > the CustomXXXXMethods structure, thus we may have CustomXXXX node with > no extensible feature. > This manner is beneficial because extension does not need to register > the library and symbol name for serialization. So, CustomScan related > code still checks existence of individual callbacks.
I was looking over this patch yesterday, and something was bugging me about it, but I couldn't quite put my finger on what it was. But now I think I know. I think of an extensible node type facility as something that ought to be defined to allow a user to create new types of nodes. But this is not that. What this does is allows you to have a CustomScan or ForeignScan node - that is, the existing node type - which is actually larger than a normal node of that type and has some extra data that is part of it. I'm having a really hard time being comfortable with that concept. Somehow, it seems like the node type should determine the size of the node. I can stretch my brain to the point of being able to say, well, maybe if the node tag is T_ExtensibleNode, then you can look at char *extnodename to figure out what type of node it is really, and then from there get the size. But what this does is: every time you see a T_CustomScan or T_ForeignScan node, it might not really be that kind of node but something else else, and tomorrow there might be another half-dozen node types with a similar property. And every one of those node types will have char *extnodename in a different place in the structure, so a hypothetical piece of code that wanted to find the extension methods for a node, or the size of a node, would need a switch that knows about all of those node types. It feels very awkward. So I have a slightly different proposal. Let's forget about letting T_CustomScan or T_ForeignScan or any other built-in node type vary in size. Instead, let's add T_ExtensibleNode which acts as a completely user-defined node. It has read/out/copy/equalfuncs support along the lines of what this patch implements, and that's it. It's not a scan or a path or anything like that: it's just an opaque data container that the system can input, output, copy, and test for equality, and nothing else. Isn't that useless? Not at all. If you're creating an FDW or custom scan provider and want to store some extra data, you can create a new type of extensible node and stick it in the fdw_private or custom_private field! The data won't be part of the CustomScan or ForeignScan structure itself, as in this patch, but who cares? The only objection I can see is that you still need several pointer deferences to get to the data since fdw_private is a List, but if that's actually a real performance problem we could probably fix it by just changing fdw_private to a Node instead. You'd still need one pointer dereference, but that doesn't seem too bad. Thoughts? -- 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
