On 10/24/14, 4:03 PM, Robert Haas wrote:
On Fri, Oct 24, 2014 at 4:46 PM, Jim Nasby <jim.na...@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 it works 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_command that accepts a few booleans (or maybe
just one to indicate the caller) and some if's. At least I don't think 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 same logging 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 ideas to 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 this we'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 seems pretty 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 damn strong indication it shouldn't be
in PGXN. :)
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
