Re: pg_background (and more parallelism infrastructure patches)

On Thu, Sep 11, 2014 at 7:34 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Don't we need some way to prohibit changing GUC by launching process,
> once it has shared the existing GUC?

Nope.  I mean, eventually, for true parallelism ... we absolutely will
need that.  But pg_background itself doesn't need that; it's perfectly
fine for configuration settings to get changed in the background
worker.  So it's a different piece of infrastructure from this patch
set.

> 1. This patch generates warning on windows
> 1>pg_background.obj : error LNK2001: unresolved external symbol
> StatementTimeout
>
> You need to add PGDLLIMPORT for StatementTimeout

OK.  I still think we should go back and PGDLLIMPORT-ize all the GUC variables.

> 2.
> CREATE FUNCTION pg_background_launch(sql pg_catalog.text,
>   queue_size pg_catalog.int4 DEFAULT 65536)
>
> Here shouldn't queue_size be pg_catalog.int8 as I could see
> some related functions in test_shm_mq uses int8?

I think if you think you want a queue that's more than 2GB in size,
you should should re-think.

> pg_background_launch(PG_FUNCTION_ARGS)
> {
> text   *sql = PG_GETARG_TEXT_PP(0);
> int32 queue_size = PG_GETARG_INT64(1);
>
> Here it should _INT32 variant to match with current sql definition,
> otherwise it leads to below error.
>
> postgres=# select pg_background_launch('vacuum verbose foo');
> ERROR:  queue size must be at least 64 bytes

Oops.

> 3.
> Comparing execute_sql_string() and exec_simple_query(), I could see below
> main differences:
> a. Allocate a new memory context different from message context
> b. Start/commit control of transaction is outside execute_sql_string
> c. enable/disable statement timeout is done from outside incase of
> execute_sql_string()
> d. execute_sql_string() prohibits Start/Commit/Abort transaction statements.
> e. execute_sql_string() doesn't log statements
> f. execute_sql_string() uses binary format for result whereas
> exec_simple_query()
>    uses TEXT as defult format
> g. processed stat related info from caller incase of execute_sql_string().
>
> Can't we handle all these or other changes inside exec_simple_query()
> based on some parameter or may be a use a hook similar to what we
> do in ProcessUtility?
>
> Basically it looks bit odd to have a duplicate (mostly) copy of
> exec_simple_query().

It is.  But I didn't think hacking up exec_simple_query() was a better
option.  We could do that if most people like that approach, but to me
it seemed there were enough differences to make it unappealing.

> 4.
> Won't it be better if pg_background_worker_main() can look more
> like PostgresMain() (in terms of handling different kind of messages),
> so that it can be extended in future to handle parallel worker.

I don't think that a parallel worker will look like pg_background in
much more than broad outline.  Some of the same constructs will get
reused, but overall I think it's a different problem that I'd rather
not conflate with this patch.

> 5.
> pg_background_result()
> {
> ..
> /* Read and processes messages from the shared memory queue. */
> }
>
> Shouldn't the processing of messages be a separate function as
> we do for pqParseInput3().

I guess we could.  It's not all that much code, though.

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