> On 29/02/16 13:07, Kouhei Kaigai wrote: > > > > I'd like to adjust a few of custom-scan interface prior to v9.6 freeze. > > > > The major point is serialization/deserialization mechanism. > > Now, extension has to give LibraryName and SymbolName to reproduce > > same CustomScanMethods on the background worker process side. Indeed, > > it is sufficient information to pull the table of function pointers. > > > > On the other hands, we now have different mechanism to wrap private > > information - extensible node. It requires extensions to register its > > ExtensibleNodeMethods identified by name, usually, on _PG_init() time. > > It is also reasonable way to reproduce same objects on background > > worker side. > > > > However, mixture of two different ways is not good. My preference is > > what extensible-node is doing rather than what custom-scan is currently > > doing. > > The attached patch allows extension to register CustomScanMethods once, > > then readFunc.c can pull this table by CustomName in string form. > > > > Agreed, but this will break compatibility right? > The manner to pass a pair of library-name and symbol-name is a new feature in v9.6, not in v9.5, so it is now the last chance to fix up the interface requirement.
> > I'm not 100% certain whether "nodes/custom-apis.h" is the best location, > > but somewhere we can put these declarations rather than the primitive > > header files might be needed. > > custom-apis.c does not sound like right name to me, maybe it can be just > custom.c but custom.h might be bit too generic, maybe custom_node.h > OK, custom_node.h may be better. > I am not sure I like the fact that we have this EXTNODENAME_MAX_LEN and > now the CUSTOM_NAME_MAX_LEN with the same length and also they are both > same lenght as NAMEDATALEN I wonder if this shouldn't be somehow > squished to less defines. > Hmm. I just followed the manner in extensible.c, because this label was initially NAMEDATALEN, then Robert changed it with EXTNODENAME_MAX_LEN. I guess he avoid to apply same label on different entities - NAMEDATALEN is a limitation for NameData type, but identifier of extensible-node and custom-scan node are not restricted by. > Also in RegisterCustomScanMethods > + Assert(strlen(methods->CustomName) <= CUSTOM_NAME_MAX_LEN); > > Shouldn't this be actually "if" with ereport() considering this is > public API and extensions can pass anything there? (for that matter same > is true for RegisterExtensibleNodeMethods but that's already committed). > Hmm. I don't have clear answer which is better. The reason why I put Assert() here is that only c-binary extension uses this interface, thus, author will fix up the problem of too long name prior to its release. Of course, if-with-ereport() also informs extension author the name is too long. One downside of Assert() may be, it makes oversight if --enable-cassert was not specified. > Other than that this seems like straight conversion to same basic > template as extensible nodes so I think it's ok. > Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei <kai...@ak.jp.nec.com> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
