Re: logical replication launcher crash on buildfarm

On 15/04/17 06:01, Tom Lane wrote:
> Petr Jelinek <petr(dot)jelinek(at)2ndquadrant(dot)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