Re: pg_background (and more parallelism infrastructure patches)

On Wed, Oct 29, 2014 at 4:58 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> 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.

I don't think it's appropriate to put superuser() checks in the code,
if that's what you are proposing. Forcing this to be super-user only
is hitting a mouse with a battleship. If you're saying we should put
REVOKE commands into the install script as we do for some other
modules, like dblink, that makes sense to me.

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

Looks like those checks happen in InitializeSessionUserId(), which is
called from InitPostgres(), which is called from
BackgroundWorkerInitializeConnection(). That makes me think we're
already applying these checks.

rhaas=# select * from
pg_background_result(pg_background_launch('vacuum pg_enum')) as (x
text);
x
--------
VACUUM
(1 row)

rhaas=# alter role rhaas nologin;
ALTER ROLE
rhaas=# select * from
pg_background_result(pg_background_launch('vacuum pg_enum')) as (x
text);
ERROR: role "rhaas" is not permitted to log in
CONTEXT: background worker, pid 64311
rhaas=# alter role rhaas login;
ALTER ROLE
rhaas=# select * from
pg_background_result(pg_background_launch('vacuum pg_enum')) as (x
text);
x
--------
VACUUM
(1 row)

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

It's hard to argue with "I'm unconvinced". What specifically about
that argument do you think isn't valid?

While I am sure the problem can be worked around, it isn't trivial.
Right now, execute_sql_string() just requests binary format
unconditionally. To do what you're talking about, we'd need to
iterate through all of the types and figure out which ones have
typsend/typreceive functions. If there's a convenience function that
will do that for us, I don't see it, probably because I believe there
are exact zero situations where we do that kind of inference today.
Then, the user backend has to save the format codes from the
RowDescription message and decide whether to use text or binary. That
just seems like a silly waste of code and cycles.

I think this actually matters, too, because the question is what we're
going to do with full-blown parallelism. Best would be to actually
shuttle entire raw tuples between backends; second best, binary
format; third best, text format or mixture of text and binary. I'm
not sure what it's reasonable to try to get away with there, but I
certainly think minimizing IPC costs is going to be an important goal.
I didn't try to get around with shipping raw tuples here because we
don't lock types while they're in use, and I'm worried that Bad Things
Could Happen. But I'm sure somebody's going to care about the
overhead of converting back and forth at some point.

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

I don't think I was proposing that, but I think I may have gotten a
little off-track here. See what you think of the attached, which
seems to work.

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

Done in the attached version.

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

Eh, I'm embarrassed to admit I don't know exactly how to do this. My
install directory doesn't seem to have a subdirectory called "plugin".

BTW, are we in agreement, more or less, on patch #3 now, the support
for error propagation? I'd like to go ahead and commit that if there
are no further review comments there.

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