Re: pg_background (and more parallelism infrastructure patches) - Mailing list pgsql-hackers

On Wed, Oct 8, 2014 at 6:32 PM, Andres Freund <andres@2ndquadrant.com> 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