Re: pg_background (and more parallelism infrastructure patches)

On Wed, Oct 29, 2014 at 3:28 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > Hm. So every user can do this once the extension is created as the
>> > functions are most likely to be PUBLIC. Is this a good idea?
>>
>> Why not?  If they can log in, they could start separate sessions with
>> similar effect.
>
> Connection limits and revoked connection permissions are possible
> reasons.

That's true.  I don't know what to do about it.  I'm somewhat inclined
to think that, if this remains in contrib, it's OK to ignore those
issues until such time as people complain about them, because anybody
who dislikes the things that can be done with this extension doesn't
have to install it.  Also, the people complaining might have useful
ideas about what a good fix would look like, which I currently don't.
There's some push to move this into core, which I think is overkill,
but if we do it then we'd better have a good solution to this problem.

We could try to make connection limits apply to pg_background, and we
could also check CONNECT permission when starting a background worker.
Both of those things feel slightly odd because there's no actual
server connection.  There *might* be a connection to the user backend
that started it, but it's sort of a "virtual" connection through
shared memory, and the background process continues running unimpeded
if it goes away, so there might be no actual connection at all.  At
the other extreme, we could invent a BACKGROUND permission and
background_limit as analogues of what we have now and enforce those
instead.  But I'm not keep to getting sucked into a long process of
perfecting pg_background.  It's here; I think some people will like
it, but it exists mostly to show that, dude, we can use dynamic shared
memory to talk to a background worker and do real stuff.

>> > I'm unsure right now about the rules surrounding this, but shouldn't we
>> > check that the user is allowed to execute these? And shouldn't we fall
>> > back to non binary functions if no binary ones are available?
>>
>> I can't see any reason to do either of those things.  I'm not aware
>> that returning data in binary format is in any way intended to be a
>> security-restricted operation, or that we have any data types that
>> actually matter without send and receive functions.  If we do, I think
>> the solution is to add them, not make this more complicated.
>
> We do have a couple of types that don't have binary send/recv
> functions. And there are many more out there. I don't think we can make
> that a hard prerequisite.

Well, again, I think this comes down partly to the question of whether
this is a toy, a first-class facility, or something in between.  But I
also think there's absolutely no rule that all types need to work with
every feature.  Some types, for example, don't provide a hash opclass.
You can't build hash indexes on those data types; more importantly,
you can't do hash joins.  Anyone who doesn't like that can write a
patch to provide a hash opclass.  Similarly, if you try to retrieve
results in binary from the server and there are no send/recv
functions, it will fail.  MUCH less significantly, the type also can't
be returned via pg_background.  I don't see why that should be viewed
as a problem pg_background has to fix rather than a problem with the
type.

>> Yeah, I was wondering if we needed some kind of annotation here.  What
>> I'm wondering about is appending something to the errcontext, perhaps
>> "background worker, PID %d".
>
> Can't we just add another error context? That seems cleaner to me. It
> should be sufficient to push something onto the relevant stack.

Right now, pq_parse_errornotice() just recontructs the exact ErrorData
that was present in the other backed, and ThrowErrorData() just
rethrows it as-is.  I think that's the right design.  It's for the
application code (patch 6) to figure out what to pass to
ThrowErrorData().  The error propagation machinery itself (patch 3)
should content itself with serializing and deserializing the data it's
given, and should not attempt to second-guess which parts of the error
data the user may want to change.

>> >> +                     case 'A':
>> >> +                             {
>> >> +                                     /* Propagate NotifyResponse. */
>> >> +                                     pq_putmessage(msg.data[0], &msg.data[1], nbytes - 1);
>> >> +                                     break;
>> >
>> > Hm. Are we sure to be in a situation where the client expects these? And
>> > are we sure their encoding is correct? The other data goe through
>> > input/output methods checking for that, but here we don't. And the other
>> > side AFAICS could have done a SET client_encoding.
>>
>> I think there's no problem at the protocol level; I think the server
>> can send NotifyResponse pretty much whenever.
>
> I think the server not having done so for pretty much forever will still
> confuse clients.
>
>> It could be argued that this is a POLA violation, but dropping the
>> notify messages on the floor (which seems to be the other option)
>> doesn't seem like superior.  So I think this is mostly a matter of
>> documentation.
>
> Maybe.

Yeah, I'm not totally sure about this either.  Perhaps some other
people will weigh in.

>> Do you think we need a restriction?  It's not obvious to me that there
>> are any security-relevant consequences to this, but it's an important
>> question, and I might be missing something.
>
> Well, a security definer function might have started the worker. And
> then the plain user kills it. Or the other way round.

Detaching the worker doesn't kill it; it just causes us not to wait
for the results.  It does mean that if a security definer function
starts a worker, and returns without detaching it or cleaning it up,
the unprivileged user could then read the data back from that worker.
That's more insidious than it may at first appear, because the
security definer function could have been intending to read back the
data before returning, and then a transaction abort happens.  We could
add a guard requiring that the data be read by the same effective user
ID that started the worker, although that might also foreclose useful
things people would otherwise be able to do with this.

>> Meh.  The launching process would have errored out if it hadn't been
>> able to set up the segment correctly.  We could add some Assert()
>> statements if you really feel strongly about it, but it seems fairly
>> pointless to me.  Any situation where those pointers come back NULL is
>> presumably going to be some sort of really stupid bug that will be
>> found even by trivial testing.
>
> If we write code like this we really should have made it an error to
> look something up that doesn't exist in the toc. But whatever.

You might be right.  I think there could be other cases where the data
could exist or be omitted and the worker would have to know to cope
with it, but having the dsm_toc stuff error out directly wouldn't have
been stupid either.

>> >> +     /* Restore GUC values from launching backend. */
>> >> +     StartTransactionCommand();
>> >> +     RestoreGUCState(gucstate);
>> >> +     CommitTransactionCommand();
>> >
>> > I haven't read the guc save patch, but is it a) required to this in a
>> > transaction? We normally reload the config even without. b) correct to
>> > do? What's with SET LOCAL variables?
>>
>> (a) Yeah, it doesn't work without that.  I forget what breaks, but if
>> you taking those out, it will blow up.
>
> That seems bad. Because we actually parse gucs without a surrounding
> transaction in some cases. But maybe it's just NOT_IN_FILE (or however
> they're called) variables that are problematic?

I just tried this and got:

ERROR:  invalid value for parameter "session_authorization": "rhaas"

> Btw, how are we dealing with connection gucs?

What do you mean by that?

>> (b) Do those need special handling for some reason?
>
> Well, the CommitTransactionCommand() will reset them (via AtEOXact_GUC),
> no? Or am I missing something?

We don't serialize and restore the GucStackState.

>> >> +     /* Post-execution cleanup. */
>> >> +     disable_timeout(STATEMENT_TIMEOUT, false);
>> >> +     CommitTransactionCommand();
>> >
>> > So, we're allowed to do nearly arbitrary nastyness here...
>>
>> Can you be more specific about the nature of your concern?  This is no
>> different than finish_xact_command().
>
> What I was thinking about was that the background process can do stuff
> that writes.

So what?

>> > Hm. This is a fair amount of code copied from postgres.c.
>>
>> Yes.  I'm open to suggestions, but I don't immediately see a better way.
>
> I don't immediately see something better either. But I definitely
> dislike the amount of code it duplicates.

I'm not crazy about it myself.  But I don't think polluting
exec_simple_query() with pg_background's concerns is a good plan
either.  That'd really be getting our priorities mixed up.

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