Re: pg_background (and more parallelism infrastructure patches)

On Fri, Oct 24, 2014 at 4:46 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
> On 10/24/14, 12:21 PM, Robert Haas wrote:
>> - What should we call dsm_unkeep_mapping, if not that?
>
> Only option I can think of beyond unkeep would be
> dsm_(un)register_keep_mapping. Dunno that it's worth it.

Hmm, we could rename dsm_keep_mapping() to dsm_unregister_mapping(),
since it's arranging to keep it by unregistering it from the resource
owner. And then we could call the new function
dsm_register_mapping(). That has the appeal that "unregister" is a
word, whereas "unkeep" isn't, but it's a little confusing otherwise,
because the sense is reversed vs. the current naming. Or we could
just leave dsm_keep_mapping() alone and call the new function
dsm_register_mapping(). A little non-orthogonal, but I think it'd be
OK.

>> - Does anyone have a tangible suggestion for how to reduce the code
>> duplication in patch #6?
>
> Between execute_sql_string() and tcop/exec_simple_query()? Is there stuff in
> exec_simple that's not safe for bgwriter? I'm not seeing why we can't use
> exec_simple. :/

There are some differences if you compare them closely.

> BTW, it does occur to me that we could do something to combine
> AllocSetContextCreate() followed by oldcontext=MemoryContextSwitchTo().

Meh.

> pg_background_result()
> + dsm_unkeep_mapping(info->seg);
> +
> + /* Set up tuple-descriptor based on colum definition list.
> */
> + if (get_call_result_type(fcinfo, NULL, &tupdesc) !=
> TYPEFUNC_COMPOSITE)
> + ereport(ERROR,
> Is there a reason we can't check the result type before unkeeping? Seems
> silly to throw the results away just because someone flubbed a function
> call...

Hmm, yeah, I see no reason why we couldn't move that up higher in the
function. It's probably a pretty common failure mode, too, so I can
see the convenience factor there.

> + default:
> + elog(WARNING, "unknown message type: %c (%zu
> bytes)",
> + msg.data[0], nbytes);
> It'd be useful to also provide DEBUG output with the message itself...

I think that's more work than is justified. We'd have to hexdump it
or something because it might not be valid in the client encoding, and
it's a case that shouldn't happen anyway.

(Hmm, that reminds me that I haven't thought of a solution to the
problem that the background worker might try to SET client_encoding,
which would be bad. Not sure what the best fix for that is, off-hand.
I'd rather not prohibit ALL GUC changes in the background worker, as
there's no good reason for such a draconian restriction.)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company