Re: pg_background (and more parallelism infrastructure patches)

On 10/24/14, 4:03 PM, Robert Haas wrote:
> On Fri, Oct 24, 2014 at 4:46 PM, Jim Nasby <Jim.Nasby@bluetreble.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.

I think it's actually important to keep the names matching, even if it means "unkeep".

dsm_unregister_mapping seems like the opposite of what you'd want to do to have the mapping be remembered. I get that
itworks that way because of how it's implemented, but that seems like a lot of leakage of implementation into API...
 

FWIW, I don't see "unkeep" as being that horrible.

>>> - 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.

Without doing a deep dive, I'm guessing that most of the extra stuff would be safe to re-use; it just wouldn't affect
execute_sql_string.Obviously we could add a boolean to exec_simple_query for the case when it's being used by a
bgwriter.Though, it seems like the biggest differences have to do with logging
 

Here's the differences I see:

- Disallowing transaction commands
- Logging
- What memory context is used (could we just use that differently in a pg_backend backend?)
- Portal output format
- What to do with the output of intermediate commands (surely there's other places we need to handle that? plpgsql
maybe?)

I think all of those except logging could be handled by a function serving both exec_simple_query and
execute_sql_commandthat accepts a few booleans (or maybe just one to indicate the caller) and some if's. At least I
don'tthink it'd be too bad, without actually writing it.
 

I'm not sure what to do about logging. If the original backend has logging turned on, is it that horrible to do the
samelogging as exec_simple_query would do?
 

Another option would be factoring out parts of exec_simple_query; the for loop over the parse tree might be a good
candidate.But I suspect that would be uglier than a separate support function.
 

I do feel uncomfortable with the amount of duplication there is right now though...

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

Well, it does seem to be a fairly common pattern throughout the codebase. And you were pretty vague when you asked for
ideasto reduce duplication. ;P
 

>> +                       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.

I'm worried that a user wouldn't have any good way to see what the unexpected data is... but I guess if a user ever saw
thiswe've got bigger problems 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.)

Perhaps a new GucContext for background workers? Or maybe a new GucSource, though I'm not sure if that'd work and it
seemspretty wrong anyway.
 

BTW, perhaps the amount of discussion on internal parts is an indication this shouldn't be in contrib. I think it's a
damnstrong indication it shouldn't be in PGXN. :)
 
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com