On 15/04/17 06:01, Tom Lane wrote: > Petr Jelinek <petr.jeli...@2ndquadrant.com> writes: >> So this is what I came up with on plane. Generalized the loading >> functionality into load_library_function which that can load either >> known postgres functions or call load_external_function. > > I've had more than enough of seeing buildfarm failures from culicidae, > so I whacked this around until I was happy with it and pushed it. > Further adjustments welcome of course.
Thanks. Seems like culicidae is finally happy with master. > >> I am not quite sure if fmgr.c is best place to put it, but I didn't want >> to include stuff from executor in bgworker.c. > > I really did not care for putting those references into fmgr.c. Aside > from the layering violations involved, there was a type cheat, in that > you had functions with entirely different signatures in the same list. > I eventually decided that the least ugly solution was to create two > different pointer arrays and frontend functions, one for bgworkers and > one for parallel workers. That ends up requiring only one extra > export/import, to make ParallelQueryMain accessible in parallel.c. > Maybe we need to rethink the division of labor between parallel.c > and execParallel.c, but that would depend on somebody explaining > what the difference is in the first place. > Sure, I did not really like the placement in fmgr.c either, just could not find good place that would work well for both. I tend to err on the less code duplication side of "less ugly", but the different function signatures is reasonable argument for splitting it as we get additional compiler check. >> As with previous patch, 9.6 will need quite different patch as we need >> to keep compatibility there, > > I don't really understand why 9.6 needs a significantly different > patch. AFAICS, it simply does not work (with any reliability) > for a loadable module to call CreateParallelContext directly in 9.6. > The only thing that actually works is to call the undocumented > CreateParallelContextForExternalFunction. I suggest that we could > back-patch this as-is, if we add CreateParallelContextForExternalFunction > as a trivial wrapper around CreateParallelContext. > I think the problem is that if somebody was using CreateParallelContext it may have worked on unix when just normally forking if the extension was loaded via shared_preload_libraries. And lot of extensions are linux only. So we might break working setup for somebody if we change the function signature. We may need to keep CreateParallelContext as is in back branches and add some CreateParallelContextInternal which would do what CreateParallelContext does in master (and is used by postgres) and then make CreateParallelContextForExternalFunction simple wrapper around that. It's somewhat ugly though. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers