Re: pg_background (and more parallelism infrastructure patches)
От | Robert Haas |
---|---|
Тема | Re: pg_background (and more parallelism infrastructure patches) |
Дата | |
Msg-id | CA+Tgmob+vvn++HS=8m_Tn114g=rH+WqDMt6=ZSOf6GNQ10FXoA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: pg_background (and more parallelism infrastructure patches) (Andres Freund <andres@2ndquadrant.com>) |
Ответы |
Re: pg_background (and more parallelism infrastructure
patches)
(Andres Freund <andres@2ndquadrant.com>)
|
Список | pgsql-hackers |
On Wed, Oct 29, 2014 at 4:58 PM, Andres Freund <andres@2ndquadrant.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
Вложения
В списке pgsql-hackers по дате отправления: