Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> Attached is a contrib module that lets you launch arbitrary command in
> a background worker, and supporting infrastructure patches for core.

Very cool!  Started looking into this while waiting on a few
CLOBBER_CACHE_ALWAYS runs to finish (ugh...).

Perhaps I'm just being a bit over the top, but all this per-character
work feels a bit ridiculous..  When we're using MAXIMUM_ALIGNOF, I
suppose it's not so bad, but is there no hope to increase that and make
this whole process more efficient?  Just a thought.

After reading through the code for 0001, I decided to actually take it
out for a spin- see attached.  I then passed a few megabytes of data
through it and it seemed to work just fine.

In general, I'm quite excited about this capability and will be looking
over the later patches also.  I also prefer the function-pointer based
approach which was taken up in later versions to the hook-based approach
in the initial patches, so glad to see things going in that direction.
Lastly, I will say that I feel it'd be good to support bi-directional
communication as I think it'll be needed eventually, but I'm not sure
that has to happen now.

	Thanks!

		Stephen

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

On 10/24/14, 6:17 PM, Jim Nasby wrote:
>>>> - 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...

I took a stab at this by refactoring the guts of exec_simple_query (patch attached) into a new function called exec_query_string (also attached in raw form). As indicated it needs a bit more work. In particular, how it's dealing with excluding transactional commands is rather ugly. Why do we need to do that in pg_background?

Andres was concerned about the performance impact of doing this. I tested this by doing

for i in {1..999999}; do echo 'SELECT 1;' >> test.sql; done

and then

time psql -f test.sql > /dev/nul

It appears there may be a ~1% performance hit, but my laptop isn't repeatable enough to be sure. I did try manually in-lining exec_query_string to see if it was the function call causing the issue; it didn't seem to make a difference.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

On Wed, Sep 10, 2014 at 4:01 PM, Robert Haas  wrote:
>> Yes, although my issue with the hooks was not that you only provided them
>> for 2 functions, but the fact that it had no structure and the
>> implementation was "if hook set do this, else do that" which I don't see
>> like a good way of doing it.
>
> We've done it that way in a bunch of other places, like ExecutorRun().
> An advantage of this approach (I think) is that jumping to a fixed
> address is faster than jumping through a function pointer - so with
> the approach I've taken here, the common case where we're talking to
> the client incurs only the overhead of a null-test, and the larger
> overhead of the function pointer jump is incurred only when the hook
> is in use.  Maybe that's not enough of a difference to matter to
> anything, but I think the contention that I've invented some novel
> kind of interface here doesn't hold up to scrutiny.  We have lots of
> hooks that work just like what I did here.

Here's what the other approach looks like.  I can't really see doing
this way and then only providing hooks for those two functions, so
this is with hooks for all the send-side stuff.

Original version: 9 files changed, 295 insertions(+), 3 deletions(-)
This version: 9 files changed, 428 insertions(+), 47 deletions(-)

There is admittedly a certain elegance to providing a complete set of
hooks, so maybe this is the way to go.  The remaining patches in the
patch series work with either the old version or this one; the changes
here don't affect anything else.

Anyone else have an opinion on which way is better here?

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

Re: pg_background (and more parallelism infrastructure patches)

pg_background (and more parallelism infrastructure patches)

Attached is a contrib module that lets you launch arbitrary command in
a background worker, and supporting infrastructure patches for core.
You can launch queries and fetch the results back, much as you could
do with a dblink connection back to the local database but without the
hassles of dealing with authentication; and you can also run utility
commands, like VACUUM.  For people who have always wanted to be able
to launch a vacuum (or an autonomous transaction, or a background
task) from a procedural language ... enjoy.

Here's an example of running vacuum and then fetching the results.
Notice that the notices from the original session are propagated to
our session; if an error had occurred, it would be re-thrown locally
when we try to read the results.

rhaas=# create table foo (a int);
CREATE TABLE
rhaas=# select pg_background_launch('vacuum verbose foo');
 pg_background_launch
----------------------
                51101
(1 row)

rhaas=# select * from pg_background_result(51101) as (x text);
INFO:  vacuuming "public.foo"
INFO:  "foo": found 0 removable, 0 nonremovable row versions in 0 out of 0 pages
DETAIL:  0 dead row versions cannot be removed yet.
There were 0 unused item pointers.
0 pages are entirely empty.
CPU 0.00s/0.00u sec elapsed 0.00 sec.
   x
--------
 VACUUM
(1 row)

Here's an overview of the attached patches:

Patches 1 and 2 add a few new interfaces to the shm_mq and dsm APIs
that happen to be convenient for later patches in the series.  I'm
pretty sure I could make all this work without these, but it would
take more code and be less efficient, so here they are.

Patch 3 adds the ability for a backend to request that the protocol
messages it would normally send to the frontend get redirected to a
shm_mq.  I did this by adding a couple of hook functions.  The best
design is definitely arguable here, so if you'd like to bikeshed, this
is probably the patch to look at.  This patch also adds a function to
help you parse an ErrorResponse or NoticeResponse and re-throw the
error or notice in the originating backend.  Obviously, parallelism is
going to need this kind of functionality, but I suspect a variety of
other applications people may develop using background workers may
want it too; and it's certainly important for pg_background itself.

Patch 4 adds infrastructure that allows one session to save all of its
non-default GUC values and another session to reload those values.
This was written by Amit Khandekar and Noah Misch.  It allows
pg_background to start up the background worker with the same GUC
settings that the launching process is using.  I intend this as a
demonstration of how to synchronize any given piece of state between
cooperating backends.  For real parallelism, we'll need to synchronize
snapshots, combo CIDs, transaction state, and so on, in addition to
GUCs.  But GUCs are ONE of the things that we'll need to synchronize
in that context, and this patch shows the kind of API we're thinking
about for these sorts of problems.

Patch 5 is a trivial patch to add a function to get the authenticated
user ID.  Noah pointed out to me that it's important for the
authenticated user ID, session user ID, and current user ID to all
match between the original session and the background worker.
Otherwise, pg_background could be used to circumvent restrictions that
we normally impose when those values differ from each other.  The
session and current user IDs are restored by the GUC save-and-restore
machinery ("session_authorization" and "role") but the authenticated
user ID requires special treatment.  To make that happen, it has to be
exposed somehow.

Patch 6 is pg_background itself.  I'm quite pleased with how easily
this came together.  The existing background worker, dsm, shm_toc, and
shm_mq infrastructure handles most of the heavily lifting here -
obviously with some exceptions addressed by the preceding patches.
Again, this is the kind of set-up that I'm expecting will happen in a
background worker used for actual parallelism - clearly, more state
will need to be restored there than here, but nonetheless the general
flow of the code here is about what I'm imagining, just with somewhat
more different kinds of state.  Most of the work of writing this patch
was actually figuring out how to execute the query itself; what I
ended up with is mostly copied form exec_simple_query, but with some
difference here and there.  I'm not sure if it would be
possible/advisable to try to refactor to reduce duplication.

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

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

On Wed, Oct 29, 2014 at 4:58 PM, Andres Freund  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

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

On Thu, Nov 20, 2014 at 7:30 AM, Amit Kapila  wrote:
> Few compilation errors in the patch:
> 1>contrib\postgres_fdw\postgres_fdw.c(2107): error C2198:
> 'set_config_option' : too few arguments for call
> 1>contrib\postgres_fdw\postgres_fdw.c(2111): error C2198:
> 'set_config_option' : too few arguments for call
> 1>contrib\postgres_fdw\postgres_fdw.c(2115): error C2198:
> 'set_config_option' : too few arguments for call
> 2>contrib\dblink\dblink.c(2983): error C2198: 'set_config_option' : too few
> arguments for call

Oops.  Good catch.  Fixed in the attached version.

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

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

On Wed, Oct 8, 2014 at 6:32 PM, Andres Freund  wrote:
> I got to ask: Why is it helpful that we have this in contrib? I have a
> good share of blame to bear for that, but I think we need to stop
> dilluting contrib evermore with test programs. These have a place, but I
> don't think it should be contrib.

I don't think pg_background is merely a test program: I think it's a
quite useful piece of functionality.  It can be used for running
VACUUM from a procedure, which is something people have asked for more
than once, or for simulating an autonomous transaction.  Granted,
it'll be a lot slower than a real autonomous transaction, but it's
still better than doing it via dblink, because you don't have to futz
with authentication.

I would be all in favor of moving things like test_decoding,
test_parser, and test_shm_mq to src/test or contrib/test or wherever
else we want to put them, but I think pg_background belongs in
contrib.

>> +/* Fixed-size data passed via our dynamic shared memory segment. */
>> +typedef struct pg_background_fixed_data
>> +{
>> +     Oid     database_id;
>> +     Oid     authenticated_user_id;
>> +     Oid     current_user_id;
>> +     int     sec_context;
>> +     char database[NAMEDATALEN];
>> +     char authenticated_user[NAMEDATALEN];
>> +} pg_background_fixed_data;
>
> Why not NameData?

No particular reason.  Changed.

> whitespace damage.

I went through and fixed everything that git diff --check complained
about.  Let me know if you see other problems yet.

>> +static HTAB *worker_hash;
>
> Hm. So the lifetime of this hash's contents is managed via
> on_dsm_detach(), do I understand that correctly?

More or less, yes.

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

>> +             /*
>> +              * Whether we succeed or fail, a future invocation of this function
>> +              * may not try to read from the DSM once we've begun to do so.
>> +              * Accordingly, make arrangements to clean things up at end of query.
>> +              */
>> +             dsm_unkeep_mapping(info->seg);
>> +
>> +             /* Set up tuple-descriptor based on colum definition list. */
>> +             if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
>> +                     ereport(ERROR,
>> +                                     (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>> +                                      errmsg("function returning record called in context "
>> +                                                     "that cannot accept type record"),
>> +                                      errhint("Try calling the function in the FROM clause "
>> +                                                      "using a column definition list.")));
>
> Hm, normally we don't add linebreaks inside error messages.

I copied it from dblink.

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

>> +                                     /*
>> +                                      * Limit the maximum error level to ERROR.  We don't want
>> +                                      * a FATAL inside the background worker to kill the user
>> +                                      * session.
>> +                                      */
>> +                                     if (edata.elevel > ERROR)
>> +                                             edata.elevel = ERROR;
>
> Hm. But we still should report that it FATALed? Maybe add error context
> notice about it? Not nice, but I don't have a immediately better idea. I
> think it generally would be rather helpful to add the information that
> this wasn't originally an error triggered by this process. The user
> might otherwise be confused when looking for the origin of the error in
> the log.

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

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

>> +/*
>> + * Parse a DataRow message and form a result tuple.
>> + */
>> +static HeapTuple
>> +form_result_tuple(pg_background_result_state *state, TupleDesc tupdesc,
>> +                               StringInfo msg)
>> +{
>> +     /* Handle DataRow message. */
>> +     int16   natts = pq_getmsgint(msg, 2);
>> +     int16   i;
>> +     Datum  *values = NULL;
>> +     bool   *isnull = NULL;
>> +     StringInfoData  buf;
>> +
>> +     if (!state->has_row_description)
>> +             elog(ERROR, "DataRow not preceded by RowDescription");
>> +     if (natts != tupdesc->natts)
>> +             elog(ERROR, "malformed DataRow");
>> +     if (natts > 0)
>> +     {
>> +             values = palloc(natts * sizeof(Datum));
>> +             isnull = palloc(natts * sizeof(bool));
>> +     }
>> +     initStringInfo(&buf);
>> +
>> +     for (i = 0; i < natts; ++i)
>> +     {
>> +             int32   bytes = pq_getmsgint(msg, 4);
>> +
>> +             if (bytes < 0)
>> +             {
>> +                     values[i] = ReceiveFunctionCall(&state->receive_functions[i],
>> +                                                                                     NULL,
>> +                                                                                     state->typioparams[i],
>> +                                                                                     tupdesc->attrs[i]->atttypmod);
>> +                     isnull[i] = true;
>
>> +             }
>> +             else
>> +             {
>> +                     resetStringInfo(&buf);
>> +                     appendBinaryStringInfo(&buf, pq_getmsgbytes(msg, bytes), bytes);
>> +                     values[i] = ReceiveFunctionCall(&state->receive_functions[i],
>> +                                                                                     &buf,
>> +                                                                                     state->typioparams[i],
>> +                                                                                     tupdesc->attrs[i]->atttypmod);
>> +                     isnull[i] = false;
>> +             }
>> +     }
>
> Hm. I think you didn't check that the typemods are the same above.

The same as what?

>> +Datum
>> +pg_background_detach(PG_FUNCTION_ARGS)
>> +{
>> +     int32           pid = PG_GETARG_INT32(0);
>> +     pg_background_worker_info *info;
>> +
>> +     info = find_worker_info(pid);
>> +     if (info == NULL)
>> +             ereport(ERROR,
>> +                             (errcode(ERRCODE_UNDEFINED_OBJECT),
>> +                              errmsg("PID %d is not attached to this session", pid)));
>> +     dsm_detach(info->seg);
>> +
>> +     PG_RETURN_VOID();
>> +}
>
> So there 's really no limit of who is allowed to do stuff like
> this. I.e. any SECURITY DEFINER and such may do the same.

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.

>> +     /* Establish signal handlers. */
>> +     pqsignal(SIGTERM, handle_sigterm);
>
> Hm. No SIGINT?

Nope; bgworker.c sets it to StatementCancelHandler, which should be
fine.  Ideally I wouldn't have to do anything with SIGTERM either, but
bgworker.c sets it to bgworker_die(), which is pretty much complete
junk.  It's not safe to just ereport() from within whatever the heck
the caller is doing.  We should probably drop a small thermonuclear
weapon on bgworker_die(), but that's a problem for another patch.

>> +     /* Find data structures in dynamic shared memory. */
>> +     fdata = shm_toc_lookup(toc, PG_BACKGROUND_KEY_FIXED_DATA);
>> +     sql = shm_toc_lookup(toc, PG_BACKGROUND_KEY_SQL);
>> +     gucstate = shm_toc_lookup(toc, PG_BACKGROUND_KEY_GUC);
>> +     mq = shm_toc_lookup(toc, PG_BACKGROUND_KEY_QUEUE);
>> +     shm_mq_set_sender(mq, MyProc);
>> +     responseq = shm_mq_attach(mq, seg, NULL);
>
> Don't these need to ensure that values have been found? shm_toc_lookup
> returns NULL for unknown itmes and such and such?

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.

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

(b) Do those need special handling for some reason?

> I doubt that actually works correctly without a SIGINT handler as
> statement timeout just falls back to kill(SIGINT)? Or does it, because
> it falls back to just doing a proc_exit()? If so, is that actually safe?

See above kvetching.

>> +     /* 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().

>> +     /*
>> +      * Parse the SQL string into a list of raw parse trees.
>> +      *
>> +      * Because we allow statements that perform internal transaction control,
>> +      * we can't do this in TopTransactionContext; the parse trees might get
>> +      * blown away before we're done executing them.
>> +      */
>> +     parsecontext = AllocSetContextCreate(TopMemoryContext,
>> +                                                                              "pg_background parse/plan",
>> +                                                                              ALLOCSET_DEFAULT_MINSIZE,
>> +                                                                              ALLOCSET_DEFAULT_INITSIZE,
>> +                                                                              ALLOCSET_DEFAULT_MAXSIZE);
>
> Not that it hugely matters, but shouldn't this rather be
> TopTransactionContext?

No, for the reasons explained in the command that you quoted right, uh, there.

>> +             bool            snapshot_set = false;
>> +             Portal          portal;
>> +             DestReceiver *receiver;
>> +             int16           format = 1;
>> +
>> +             /*
>> +              * We don't allow transaction-control commands like COMMIT and ABORT
>> +              * here.  The entire SQL statement is executed as a single transaction
>> +              * which commits if no errors are encountered.
>> +              */
>> +             if (IsA(parsetree, TransactionStmt))
>> +                     ereport(ERROR,
>> +                                     (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>> +                                      errmsg("transaction control statements are not allowed in pg_background")));
>
> Hm. I don't think that goes far enough. This allows commands that
> internally stop/start transactions like CREATE INDEX CONCURRETNLY. Are
> you sure that's working right now?

I tested VACUUM not of a specific table and that seems to work
correctly.  I could try CIC, but I think the issues are the same.  If
we only get one parse tree, then isTopLevel will be true and it's safe
for commands to do their own transaction control.  If we get multiple
parse trees, then PreventTransactionChain will do its thing.  This is
not novel territory.

> 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 think this is interesting work, but I doubt it's ready yet. I need to
> read the preceding patches, to really understand where breakage lies
> hidden.

Breakage??? In my code??? Surely not.  :-)

I'm reattaching all the uncommitted patches here.  #3 and #6 have been
updated; #2 and #4 are unchanged.

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

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

On Sat, Sep 20, 2014 at 3:03 AM, Amit Kapila  wrote:
> Okay, but as there is no predictability (it can be either same as what
> launching process has at the when it has launched background worker
> or it could be some different value if got changed later due to sighup)
> which GUC value will be used by background worker, it might be good
> to clarify the same in pg_bacground docs (which are not there currently,
> but I assume it will eventually be part of this patch).

OK, I will mention that in the documentation when I write it.  I
didn't sweat that too much originally because I wasn't sure how much
churn there was going to be in the user-visible API, but so far
everybody seems happy with that, so maybe it's time to go document it.
It's a pretty simple API but, as you say, there are a few details
worth mentioning.  I still need some review of the earlier patches in
the series before this really gets urgent, though: so far no one has
commented on #1, #2, #4, or #5, and I'm not entirely whether my
revised version of #3 passed muster.

> Keeping transaction control (Start/commit) outside the function
> execute_sql_string() could lead to EndCommand() message being
> sent before transaction end which could be a problem in case
> transaction commit fails due to any reason. exec_simple_query() takes
> care of the same by calling finish_xact_command() before reporting
> command-complete for last parse tree.  It even has comment indicating
> that we should follow this protocol.

Fixed in the attached version.

> Won't CommandCounterIncrement() required after every command like
> we do in exec_simple_query()?

Fixed in the attached version.

> Whats the need to reverse the order of execution for EndCommand()
> and PortalDrop()?  Any error in PortalDrop() will lead to wrong
> message being sent to client.

Fixed in the attached version.

> What is the reason for not logging statements executed via
> pg_background, even though it allows to report the sql statement
> to various monitoring tools by setting debug_query_string?

I wasn't really sure whether core GUCs should affect the behavior of a
contrib module, and I wasn't excited about duplicating the code.

> Isn't it better to add a comment why  execute_sql_string() uses
> binary format for result?

Done in the attached version.

> Sure, please take a call based on what you feel is right here, I
> mentioned it because I felt it might be little bit easier for other people
> to understand that code.

I played around with this a bit but it didn't seem like it worked out
to a win.  There were a bunch of things that had to be passed down
into that function and it didn't seem like it was really reducing
complexity.  What I think makes sense is to keep an eye on the
complexity of the handling for each individual message type and move
any handlers that get complex to their own functions.

> There can be certain scenarios where user might like to invoke this
> again.  Assume, user calls function
> pg_background_launch('select count(*) from t1') and this statement
> execution via background worker is going to take very long time before
> it could return anything. After sometime user tries to retrieve data via
> pg_background_result(), but the call couldn't came out as it is waiting
> for results, so user presses cancel and on again trying after sometime,
> he won't get any data.  I think this behaviour is bit annoying.

Yep. I don't have a better solution at the moment, but there may be one.

> To avoid user to wait for longer, function pg_background_result()
> can take an additional parameter where user can specify whether
> to WAIT incase results are not available.

That gets complicated.  Until any results are available?  Until all
results are available?  What if we try to read from the queue to find
out if results are available, and the first message in the queue is
long enough that it wraps the queue, so that we have to block and wait
for the background worker to send more data before we can continue?

> Why FATAL inside background worker is not propagated at same level by
> launcher process?
> If PANIC in background worker can kill other backends and restart server
> then ideally FATAL in background worker should also be treated at same
> level by client backend.

It was initially like that, but then I realized it was silly.  If the
background worker hits some error that forces its session to
terminate, there is no need to terminate the user's session too - and
in fact doing so is really annoying, as I rapidly found out while
experimenting with this.  Generally a FATAL is something we do because
backend-local state is corrupted to a degree that makes it impractical
to continue, but the fact that that other backend is messed up does
not mean our backend is messed up too.

> Any error ("unable to map dynamic shared memory segment") before
> pq_redirect_to_shm_mq() will not reach launcher.  Launcher client will
> get "ERROR:  lost connection to worker process with PID 4020".
>
> I think it is very difficult for user to know why such an error has
> occurred and what he can do to proceed.  I am not sure if there is any
> sensible way to report such an error, but OTOH it seems we should
> provide some information regarding what has happened to client.

I don't think this is really a fixable problem.  There's no way to
communicate an error that happens before you establish communications.
The same problem exists for user connections, but it's not serious in
practice because it's rare.  I expect the same to be true here.

> postgres=# select * from pg_background_result(4672) as (result TEXT);
> WARNING:  unknown message type: G (6 bytes)
> ERROR:  there is no client connection
> CONTEXT:  COPY t1, line 1: ""
>
> Something similar to what is returned for transaction statements
> ("transaction control statements are not allowed in pg_background")
> would be better.

Fixed in the attached version.

> If you have to discard results of statements other than last,
> then why at first place you want to allow multiple statements?

You can run statements with side effects, or you can run multiply
utility commands.

> Like in below case, how will user identify whether the result is
> for first statement or second statement?

By reading the documentation that I will write.

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

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

On Wed, Nov 12, 2014 at 11:36 AM, Robert Haas  wrote:
> On Wed, Nov 12, 2014 at 11:19 AM, Andres Freund  wrote:
>> The question is whether the library is actually loaded in that case?
>> Because that normally only happens early during startup - which is why
>> it's a PGC_BACKEND guc.
>
> It looks like that does not work.
>
> [rhaas pgsql]$ PGOPTIONS='-c local_preload_libraries=auto_explain' psql
> psql (9.5devel)
> Type "help" for help.
>
> rhaas=# select * from pg_background_result(pg_background_launch('show
> auto_explain.log_min_duration')) as (x text);
> ERROR:  unrecognized configuration parameter "auto_explain.log_min_duration"
> CONTEXT:  background worker, pid 31316
>
> So, there's more to be done here.  Rats.

It turned out to be quite simple to fix both problems.  This
particular case fails because the call that loads the libraries
specified by session_preload_libraries and local_preload_libraries is
in PostgresMain() and thus never gets called by pg_background.  I
fixed that by adding that call to pg_background in the appropriate
place.  While I was at it, I added the REVOKE statements we discussed
earlier to pg_background's .sql file.

The other problem was due to this code in set_config_option:

                                /*
                                 * If a PGC_BACKEND or PGC_SU_BACKEND
parameter is changed in
                                 * the config file, we want to accept
the new value in the
                                 * postmaster (whence it will propagate to
                                 * subsequently-started backends), but
ignore it in existing
                                 * backends.  This is a tad klugy, but
necessary because we
                                 * don't re-read the config file
during backend start.
                                 *
                                 * In EXEC_BACKEND builds, this works
differently: we load all
                                 * nondefault settings from the
CONFIG_EXEC_PARAMS file during
                                 * backend start.  In that case we
must accept PGC_SIGHUP
                                 * settings, so as to have the same
value as if we'd forked
                                 * from the postmaster.  We detect
this situation by checking
                                 * IsInitProcessingMode, which is a
bit ugly, but it doesn't
                                 * seem worth passing down an explicit
flag saying we're doing
                                 * read_nondefault_variables().
                                 */
#ifdef EXEC_BACKEND
                                if (IsUnderPostmaster &&
!IsInitProcessingMode())
                                        return -1;
#else
                                if (IsUnderPostmaster)
                                        return -1;
#endif

When restoring variables via RestoreGUCState(), we need the same kind
of special-handling that we do when running in EXEC_BACKEND mode and
restoring variables via read_nondefault_variables().  Extending the
IsInitProcessingMode kludge() doesn't look appealing, so I instead
added the flag contemplated by the comment.

Updated patches attached.

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

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)

Re: pg_background (and more parallelism infrastructure patches)