Re: pg_background (and more parallelism infrastructure patches)

On 2014-10-29 16:38:59 -0400, Robert Haas wrote:
> On Wed, Oct 29, 2014 at 3:28 PM, Andres Freund <andres(at)2ndquadrant(dot)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.

At the very least it need to be clearly documented. Another solution
would be to simply not give out PUBLIC rights, and restrict it to the
owner/superuesers lest somebody makes explicit grants. I favor
combining those two.

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

I think that'd not be bad.

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

Hm. I'm unconvinced. It looks almost trivial to fail back to the text
based protocol.

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

I don't see how that follows. The error context logic is there to make
it clearer where an error originated from. It'll be really confusing if
there's ERRORs jumping out of a block of code without emitting context
that has set a error context set.

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

I think in many cases that's a academical distinction.

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

I think such a restriction would be a good idea for now.

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

There could be an explicit boolean function to check for the presence...

> >> >> + /* 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"

Hm. Ok.

> > Btw, how are we dealing with connection gucs?
>
> What do you mean by that?

There's some PGC_BACKEND guc's that normally are only allowed to be set
at connection start. I'm not sure whether you're just circumventing that
check or whether you didn't hit a problematic case.

What does e.g. happen if you set PGOPTIONS='-c
local_preload_libraries=auto_explain' after linking auto_explain into
the plugin directory?

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

Ah, ok.

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

Agreed that that'd be the wrong way round.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services