Re: pgstat_report_activity() and parallel CREATE INDEX (was: Parallel index creation & pg_stat_activity)

2020-10-25 Thread Tom Lane 
Noah Misch  writes:
> Each elog(FATAL) check deserves a STATEMENT field if it fires, so I think that
> would be too early to clear.  Here's an version fixing the defects.  In
> worker_spi_main(), the timing now mirrors postgres.c.  initialize_worker_spi()
> is doing something not directly possible from SQL, so I improvised there.

I'm good with this version.  Thanks!

regards, tom lane

Re: pgstat_report_activity() and parallel CREATE INDEX (was: Parallel index creation & pg_stat_activity)

2020-10-25 Thread Noah Misch 
On Sun, Oct 25, 2020 at 10:52:50AM -0400, Tom Lane wrote:
> Noah Misch  writes:
> > On Sat, Oct 17, 2020 at 08:39:35AM -0700, Peter Geoghegan wrote:
> >> I also prefer 2.
> 
> > Thanks.  Here is the pair of patches I plan to use.  The second is 
> > equivalent
> > to v0; I just added a log message.
> 
> The change in worker_spi_main seems a little weird/lazy.  I'd
> be inclined to set and clear debug_query_string each time through
> the loop, so that those operations are adjacent to the other
> status-reporting operations, as they are in initialize_worker_spi.

True.  Emitting STATEMENT message fields during ProcessConfigFile(PGC_SIGHUP)
would be wrong.

> I don't really like modifying a StringInfo while debug_query_string
> is pointing at it, either.  Yeah, you'll *probably* get away with
> that because the buffer is unlikely to move, but since the whole
> exercise can only benefit crash-debugging scenarios, it'd be better
> to be more paranoid.

Good point.  This is supposed to be example code, so it shouldn't cut corners.

> One idea is to set debug_query_string immediately before each SPI_execute
> and clear it just afterwards, rather than trying to associate it with
> pgstat_report_activity calls.

Each elog(FATAL) check deserves a STATEMENT field if it fires, so I think that
would be too early to clear.  Here's an version fixing the defects.  In
worker_spi_main(), the timing now mirrors postgres.c.  initialize_worker_spi()
is doing something not directly possible from SQL, so I improvised there.
Author: Noah Misch 
Commit: Noah Misch 

Set debug_query_string in worker_spi.

This makes elog.c emit the string, which is good practice for a
background worker that executes SQL strings.

Reviewed by Tom Lane.

Discussion: https://postgr.es/m/20201014022636.ga1962...@rfd.leadboat.com

diff --git a/src/test/modules/worker_spi/worker_spi.c 
b/src/test/modules/worker_spi/worker_spi.c
index 1c7b17c..258237f 100644
--- a/src/test/modules/worker_spi/worker_spi.c
+++ b/src/test/modules/worker_spi/worker_spi.c
@@ -114,53 +114,59 @@ initialize_worker_spi(worktable *table)
PushActiveSnapshot(GetTransactionSnapshot());
pgstat_report_activity(STATE_RUNNING, "initializing worker_spi schema");
 
/* XXX could we use CREATE SCHEMA IF NOT EXISTS? */
initStringInfo();
appendStringInfo(, "select count(*) from pg_namespace where nspname 
= '%s'",
 table->schema);
 
+   debug_query_string = buf.data;
ret = SPI_execute(buf.data, true, 0);
if (ret != SPI_OK_SELECT)
elog(FATAL, "SPI_execute failed: error code %d", ret);
 
if (SPI_processed != 1)
elog(FATAL, "not a singleton result");
 
ntup = DatumGetInt64(SPI_getbinval(SPI_tuptable->vals[0],
   
SPI_tuptable->tupdesc,
   1, 
));
if (isnull)
elog(FATAL, "null result");
 
if (ntup == 0)
{
+   debug_query_string = NULL;
resetStringInfo();
appendStringInfo(,
 "CREATE SCHEMA \"%s\" "
 "CREATE TABLE \"%s\" ("
 "  type text CHECK 
(type IN ('total', 'delta')), "
 "  value   
integer)"
 "CREATE UNIQUE INDEX 
\"%s_unique_total\" ON \"%s\" (type) "
 "WHERE type = 'total'",
 table->schema, table->name, 
table->name, table->name);
 
/* set statement start time */
SetCurrentStatementStartTimestamp();
 
+   debug_query_string = buf.data;
ret = SPI_execute(buf.data, false, 0);
 
if (ret != SPI_OK_UTILITY)
elog(FATAL, "failed to create my schema");
+
+   debug_query_string = NULL;  /* rest is not 
statement-specific */
}
 
SPI_finish();
PopActiveSnapshot();
CommitTransactionCommand();
+   debug_query_string = NULL;
pgstat_report_activity(STATE_IDLE, NULL);
 }
 
 void
 worker_spi_main(Datum main_arg)
 {
int index = DatumGetInt32(main_arg);
worktable  *table;
@@ -257,16 +263,17 @@ worker_spi_main(Datum main_arg)
 *
 * The pgstat_report_activity() call makes our activity visible
 * through the pgstat views.
 */
SetCurrentStatementStartTimestamp();
StartTransactionCommand();
SPI_connect();

Re: pgstat_report_activity() and parallel CREATE INDEX (was: Parallel index creation & pg_stat_activity)

2020-10-25 Thread Tom Lane 
Noah Misch  writes:
> On Sat, Oct 17, 2020 at 08:39:35AM -0700, Peter Geoghegan wrote:
>> I also prefer 2.

> Thanks.  Here is the pair of patches I plan to use.  The second is equivalent
> to v0; I just added a log message.

The change in worker_spi_main seems a little weird/lazy.  I'd
be inclined to set and clear debug_query_string each time through
the loop, so that those operations are adjacent to the other
status-reporting operations, as they are in initialize_worker_spi.

I don't really like modifying a StringInfo while debug_query_string
is pointing at it, either.  Yeah, you'll *probably* get away with
that because the buffer is unlikely to move, but since the whole
exercise can only benefit crash-debugging scenarios, it'd be better
to be more paranoid.

One idea is to set debug_query_string immediately before each SPI_execute
and clear it just afterwards, rather than trying to associate it with
pgstat_report_activity calls.

regards, tom lane

Re: pgstat_report_activity() and parallel CREATE INDEX (was: Parallel index creation & pg_stat_activity)

2020-10-24 Thread Noah Misch 
On Sat, Oct 17, 2020 at 08:39:35AM -0700, Peter Geoghegan wrote:
> On Tue, Oct 13, 2020 at 7:26 PM Noah Misch  wrote:
> > 1. Disable parallelism for the index build under ExecuteTruncateGuts().
> >Nobody will mourn a performance loss from declining parallelism for an
> >empty index, but I feel like this is fixing in the wrong place.
> > 2. Make _bt_begin_parallel() and begin_parallel_vacuum() recognize the
> >debug_query_string==NULL case and reproduce it on the worker.
> > 3. Require bgworkers to set debug_query_string before entering code of 
> > vacuum,
> >truncate, etc.  Logical replication might synthesize a DDL statement, or 
> > it
> >might just use a constant string.
> >
> > I tend to prefer (2), but (3) isn't bad.  Opinions?
> 
> I also prefer 2.

Thanks.  Here is the pair of patches I plan to use.  The second is equivalent
to v0; I just added a log message.
Author: Noah Misch 
Commit: Noah Misch 

Reproduce debug_query_string==NULL on parallel workers.

Certain background workers initiate parallel queries while
debug_query_string==NULL, at which point they attempted strlen(NULL) and
died to SIGSEGV.  Older debug_query_string observers allow NULL, so do
likewise in these newer ones.  Back-patch to v11, where commit
7de4a1bcc56f494acbd0d6e70781df877dc8ecb5 introduced the first of these.

Reviewed by FIXME.

Discussion: https://postgr.es/m/20201014022636.ga1962...@rfd.leadboat.com

diff --git a/src/backend/access/heap/vacuumlazy.c 
b/src/backend/access/heap/vacuumlazy.c
index 4f2f381..34ae8aa 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -3238,7 +3238,6 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, 
LVRelStats *vacrelstats,
WalUsage   *wal_usage;
bool   *can_parallel_vacuum;
longmaxtuples;
-   char   *sharedquery;
Sizeest_shared;
Sizeest_deadtuples;
int nindexes_mwm = 0;
@@ -3335,9 +3334,14 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, 
LVRelStats *vacrelstats,
shm_toc_estimate_keys(>estimator, 1);
 
/* Finally, estimate PARALLEL_VACUUM_KEY_QUERY_TEXT space */
-   querylen = strlen(debug_query_string);
-   shm_toc_estimate_chunk(>estimator, querylen + 1);
-   shm_toc_estimate_keys(>estimator, 1);
+   if (debug_query_string)
+   {
+   querylen = strlen(debug_query_string);
+   shm_toc_estimate_chunk(>estimator, querylen + 1);
+   shm_toc_estimate_keys(>estimator, 1);
+   }
+   else
+   querylen = 0;   /* keep compiler quiet */
 
InitializeParallelDSM(pcxt);
 
@@ -3382,10 +3386,16 @@ begin_parallel_vacuum(Oid relid, Relation *Irel, 
LVRelStats *vacrelstats,
lps->wal_usage = wal_usage;
 
/* Store query string for workers */
-   sharedquery = (char *) shm_toc_allocate(pcxt->toc, querylen + 1);
-   memcpy(sharedquery, debug_query_string, querylen + 1);
-   sharedquery[querylen] = '\0';
-   shm_toc_insert(pcxt->toc, PARALLEL_VACUUM_KEY_QUERY_TEXT, sharedquery);
+   if (debug_query_string)
+   {
+   char   *sharedquery;
+
+   sharedquery = (char *) shm_toc_allocate(pcxt->toc, querylen + 
1);
+   memcpy(sharedquery, debug_query_string, querylen + 1);
+   sharedquery[querylen] = '\0';
+   shm_toc_insert(pcxt->toc,
+  PARALLEL_VACUUM_KEY_QUERY_TEXT, 
sharedquery);
+   }
 
pfree(can_parallel_vacuum);
return lps;
@@ -3528,7 +3538,7 @@ parallel_vacuum_main(dsm_segment *seg, shm_toc *toc)
elog(DEBUG1, "starting parallel vacuum worker for bulk delete");
 
/* Set debug_query_string for individual workers */
-   sharedquery = shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_QUERY_TEXT, 
false);
+   sharedquery = shm_toc_lookup(toc, PARALLEL_VACUUM_KEY_QUERY_TEXT, true);
debug_query_string = sharedquery;
pgstat_report_activity(STATE_RUNNING, debug_query_string);
 
diff --git a/src/backend/access/nbtree/nbtsort.c 
b/src/backend/access/nbtree/nbtsort.c
index efee867..8730de2 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -1466,7 +1466,6 @@ _bt_begin_parallel(BTBuildState *buildstate, bool 
isconcurrent, int request)
WalUsage   *walusage;
BufferUsage *bufferusage;
boolleaderparticipates = true;
-   char   *sharedquery;
int querylen;
 
 #ifdef DISABLE_LEADER_PARTICIPATION
@@ -1533,9 +1532,14 @@ _bt_begin_parallel(BTBuildState *buildstate, bool 
isconcurrent, int request)
shm_toc_estimate_keys(>estimator, 1);
 
/* Finally, estimate PARALLEL_KEY_QUERY_TEXT space */
-   querylen = strlen(debug_query_string);
-

Re: pgstat_report_activity() and parallel CREATE INDEX (was: Parallel index creation & pg_stat_activity)

2020-10-17 Thread Peter Geoghegan 
On Tue, Oct 13, 2020 at 7:26 PM Noah Misch  wrote:
> 1. Disable parallelism for the index build under ExecuteTruncateGuts().
>Nobody will mourn a performance loss from declining parallelism for an
>empty index, but I feel like this is fixing in the wrong place.
> 2. Make _bt_begin_parallel() and begin_parallel_vacuum() recognize the
>debug_query_string==NULL case and reproduce it on the worker.
> 3. Require bgworkers to set debug_query_string before entering code of vacuum,
>truncate, etc.  Logical replication might synthesize a DDL statement, or it
>might just use a constant string.
>
> I tend to prefer (2), but (3) isn't bad.  Opinions?

I also prefer 2.

Thanks
-- 
Peter Geoghegan

Re: pgstat_report_activity() and parallel CREATE INDEX (was: Parallel index creation & pg_stat_activity)

2020-10-13 Thread Noah Misch 
On Wed, Mar 07, 2018 at 05:53:18PM -0800, Peter Geoghegan wrote:
> On Thu, Mar 1, 2018 at 2:47 PM, Peter Geoghegan  wrote:
> > No. Just an oversight. Looks like _bt_parallel_build_main() should
> > call pgstat_report_activity(), just like ParallelQueryMain().
> >
> > I'll come up with a patch soon.
> 
> Attached patch has parallel CREATE INDEX propagate debug_query_string
> to workers.

Two test suites segfault under the following configuration:

cat >/tmp/max_parallel.conf <, buildstate=0x7ffe353692a0) at nbtsort.c:1536
#2  _bt_spools_heapscan (indexInfo=0x2446768, buildstate=0x7ffe353692a0, 
index=..., heap=0x7ff995b85948) at nbtsort.c:394
#3  btbuild (heap=0x7ff995b85948, index=0x7ff995b85d78, indexInfo=0x2446768) at 
nbtsort.c:326
#4  0x00568d0b in index_build 
(heapRelation=heapRelation@entry=0x7ff995b85948, 
indexRelation=indexRelation@entry=0x7ff995b85d78, 
indexInfo=indexInfo@entry=0x2446768, isreindex=isreindex@entry=false, 
parallel=parallel@entry=true) at index.c:2898
#5  0x0056a450 in index_create 
(heapRelation=heapRelation@entry=0x7ff995b85948, 
indexRelationName=indexRelationName@entry=0x7ffe35369800 
"pg_toast_16388_index", indexRelationId=16393, 
indexRelationId@entry=0, parentIndexRelid=parentIndexRelid@entry=0, 
parentConstraintId=parentConstraintId@entry=0, relFileNode=relFileNode@entry=0, 
indexInfo=indexInfo@entry=0x2446768, 
indexColNames=0x2445a28, 
accessMethodObjectId=accessMethodObjectId@entry=403, 
tableSpaceId=tableSpaceId@entry=0, 
collationObjectId=collationObjectId@entry=0x7ffe35369780, 
classObjectId=classObjectId@entry=0x7ffe35369790, 
coloptions=coloptions@entry=0x7ffe35369770, reloptions=reloptions@entry=0, 
flags=flags@entry=1, constr_flags=constr_flags@entry=0, 
allow_system_table_mods=allow_system_table_mods@entry=true, 
is_internal=is_internal@entry=true, constraintId=constraintId@entry=0x0) at 
index.c:1217
#6  0x0058ad36 in create_toast_table (rel=rel@entry=0x7ff995b7db68, 
toastOid=toastOid@entry=0, toastIndexOid=toastIndexOid@entry=0, 
reloptions=reloptions@entry=0, lockmode=lockmode@entry=8, 
check=check@entry=false) at toasting.c:310
#7  0x0058afe2 in CheckAndCreateToastTable (relOid=relOid@entry=16388, 
reloptions=reloptions@entry=0, lockmode=lockmode@entry=8, 
check=check@entry=false) at toasting.c:82
#8  0x0058b029 in NewRelationCreateToastTable 
(relOid=relOid@entry=16388, reloptions=reloptions@entry=0) at toasting.c:71
#9  0x007f3027 in ProcessUtilitySlow (pstate=pstate@entry=0x240dc40, 
pstmt=pstmt@entry=0x2409358, 
queryString=queryString@entry=0x24056a0 "CREATE SCHEMA \"schema4\" CREATE 
TABLE \"counted\" (\t\ttype text CHECK (type IN ('total', 'delta')), 
\t\tvalue\tinteger)CREATE UNIQUE INDEX \"counted_unique_total\" ON \"counted\" 
(type) WHERE type = 'total'", context=context@entry=PROCESS_UTILITY_SUBCOMMAND, 
params=params@entry=0x0, queryEnv=queryEnv@entry=0x0, qc=qc@entry=0x0, 
dest=0xa91ea0 )
at utility.c:1191
#10 0x007f14b4 in standard_ProcessUtility (pstmt=0x2409358, 
queryString=0x24056a0 "CREATE SCHEMA \"schema4\" CREATE TABLE \"counted\" 
(\t\ttype text CHECK (type IN ('total', 'delta')), \t\tvalue\tinteger)CREATE 
UNIQUE INDEX \"counted_unique_total\" ON \"counted\" (type) WHERE type = 
'total'", context=PROCESS_UTILITY_SUBCOMMAND, params=0x0, queryEnv=0x0, 
dest=0xa91ea0 , qc=0x0) at utility.c:1071
#11 0x00615ba6 in CreateSchemaCommand (stmt=stmt@entry=0x2408b40, 
queryString=queryString@entry=0x24056a0 "CREATE SCHEMA \"schema4\" CREATE 
TABLE \"counted\" (\t\ttype text CHECK (type IN ('total', 'delta')), 
\t\tvalue\tinteger)CREATE UNIQUE INDEX \"counted_unique_total\" ON \"counted\" 
(type) WHERE type = 'total'", stmt_location=0, stmt_len=0) at schemacmds.c:192
#12 0x007f2365 in ProcessUtilitySlow (pstate=pstate@entry=0x24090d0, 
pstmt=pstmt@entry=0x2408d98, 
queryString=queryString@entry=0x24056a0 "CREATE SCHEMA \"schema4\" CREATE 
TABLE \"counted\" (\t\ttype text CHECK (type IN ('total', 'delta')), 
\t\tvalue\tinteger)CREATE UNIQUE INDEX \"counted_unique_total\" ON \"counted\" 
(type) WHERE type = 'total'", context=context@entry=PROCESS_UTILITY_QUERY, 
params=params@entry=0x0, queryEnv=queryEnv@entry=0x0, 
qc=qc@entry=0x7ffe35369fd0, dest=0xa91de0 )
at utility.c:1125
#13 0x007f14b4 in standard_ProcessUtility (pstmt=0x2408d98, 
queryString=0x24056a0 "CREATE SCHEMA \"schema4\" CREATE TABLE \"counted\" 
(\t\ttype text CHECK (type IN ('total', 'delta')), \t\tvalue\tinteger)CREATE 
UNIQUE INDEX \"counted_unique_total\" ON \"counted\" (type) WHERE type = 
'total'", context=PROCESS_UTILITY_QUERY, params=0x0, queryEnv=0x0, 
dest=0xa91de0 , qc=0x7ffe35369fd0) at utility.c:1071
#14 0x006a1ace in _SPI_execute_plan (plan=plan@entry=0x7ffe3536a050, 
paramLI=paramLI@entry=0x0, snapshot=snapshot@entry=0x0, 
crosscheck_snapshot=crosscheck_snapshot@entry=0x0, 
read_only=read_only@entry=false,

RE: pgstat_report_activity() and parallel CREATE INDEX (was: Parallel index creation & pg_stat_activity)

2018-03-29 Thread Phil Florent 
Thanks, ran the original test and it works great after the patch.

Phil



De : Robert Haas <robertmh...@gmail.com>
Envoyé : jeudi 22 mars 2018 18:17
À : Peter Geoghegan
Cc : Andres Freund; Phil Florent; PostgreSQL Hackers
Objet : Re: pgstat_report_activity() and parallel CREATE INDEX (was: Parallel 
index creation & pg_stat_activity)

On Wed, Mar 7, 2018 at 8:53 PM, Peter Geoghegan <p...@bowt.ie> wrote:
> On Thu, Mar 1, 2018 at 2:47 PM, Peter Geoghegan <p...@bowt.ie> wrote:
>> No. Just an oversight. Looks like _bt_parallel_build_main() should
>> call pgstat_report_activity(), just like ParallelQueryMain().
>>
>> I'll come up with a patch soon.
>
> Attached patch has parallel CREATE INDEX propagate debug_query_string
> to workers. Workers go on to use this string as their own
> debug_query_string, as well as registering it using
> pgstat_report_activity(). Parallel CREATE INDEX pg_stat_activity
> entries will have a query text, too, which addresses Phil's complaint.

Committed.  Thanks for the patch.

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

Re: pgstat_report_activity() and parallel CREATE INDEX (was: Parallel index creation & pg_stat_activity)

2018-03-22 Thread Robert Haas 
On Wed, Mar 7, 2018 at 8:53 PM, Peter Geoghegan  wrote:
> On Thu, Mar 1, 2018 at 2:47 PM, Peter Geoghegan  wrote:
>> No. Just an oversight. Looks like _bt_parallel_build_main() should
>> call pgstat_report_activity(), just like ParallelQueryMain().
>>
>> I'll come up with a patch soon.
>
> Attached patch has parallel CREATE INDEX propagate debug_query_string
> to workers. Workers go on to use this string as their own
> debug_query_string, as well as registering it using
> pgstat_report_activity(). Parallel CREATE INDEX pg_stat_activity
> entries will have a query text, too, which addresses Phil's complaint.

Committed.  Thanks for the patch.

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

Re: pgstat_report_activity() and parallel CREATE INDEX (was: Parallel index creation & pg_stat_activity)

2018-03-07 Thread Peter Geoghegan 
On Thu, Mar 1, 2018 at 2:47 PM, Peter Geoghegan  wrote:
> No. Just an oversight. Looks like _bt_parallel_build_main() should
> call pgstat_report_activity(), just like ParallelQueryMain().
>
> I'll come up with a patch soon.

Attached patch has parallel CREATE INDEX propagate debug_query_string
to workers. Workers go on to use this string as their own
debug_query_string, as well as registering it using
pgstat_report_activity(). Parallel CREATE INDEX pg_stat_activity
entries will have a query text, too, which addresses Phil's complaint.

I wasn't 100% sure if I should actually show the leader's
debug_query_string within worker pg_stat_activity entries, since that
isn't what parallel query uses (the QueryDesc/Estate query string in
shared memory is *restored* as the debug_query_string for a parallel
query worker, though). I eventually decided that this
debug_query_string approach was okay. There is nothing remotely like a
QueryDesc or EState passed to btbuild(). I can imagine specific issues
with what I've done, such as a CREATE EXTENSION command that contains
a CREATE INDEX, and yet shows a CREATE EXTENSION in pg_stat_activity
for parallel workers. However, that's no different to what you'd see
with a serial index build. Existing tcop/postgres.c
pgstat_report_activity() calls are aligned with setting
debug_query_string.

-- 
Peter Geoghegan
From 1c380b337be86bb3c69cf70b39da1d3dd1fba18a Mon Sep 17 00:00:00 2001
From: Peter Geoghegan 
Date: Wed, 7 Mar 2018 14:22:56 -0800
Subject: [PATCH] Report query text in parallel CREATE INDEX workers.

Commit 4c728f38297 established that parallel query should propagate a
debug_query_string to worker processes, and display query text in worker
pg_stat_activity entries.  Parallel CREATE INDEX should follow this
precedent.

This fixes an oversight in commit 9da0cc35.

Author: Peter Geoghegan
Reported-By: Phil Florent
Discussion: https://postgr.es/m/cah2-wzkryapcqohbjkudkfni-hgfny31yjcbm-yp5ho-71i...@mail.gmail.com
---
 src/backend/access/nbtree/nbtsort.c | 30 +-
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index f0c276b52a..098e0ce1be 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -86,6 +86,7 @@
 #define PARALLEL_KEY_BTREE_SHARED		UINT64CONST(0xA001)
 #define PARALLEL_KEY_TUPLESORT			UINT64CONST(0xA002)
 #define PARALLEL_KEY_TUPLESORT_SPOOL2	UINT64CONST(0xA003)
+#define PARALLEL_KEY_QUERY_TEXT			UINT64CONST(0xA004)
 
 /*
  * DISABLE_LEADER_PARTICIPATION disables the leader's participation in
@@ -1195,6 +1196,8 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
 	BTSpool*btspool = buildstate->spool;
 	BTLeader   *btleader = (BTLeader *) palloc0(sizeof(BTLeader));
 	bool		leaderparticipates = true;
+	char	   *sharedquery;
+	int			querylen;
 
 #ifdef DISABLE_LEADER_PARTICIPATION
 	leaderparticipates = false;
@@ -1223,9 +1226,8 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
 		snapshot = RegisterSnapshot(GetTransactionSnapshot());
 
 	/*
-	 * Estimate size for at least two keys -- our own
-	 * PARALLEL_KEY_BTREE_SHARED workspace, and PARALLEL_KEY_TUPLESORT
-	 * tuplesort workspace
+	 * Estimate size for our own PARALLEL_KEY_BTREE_SHARED workspace, and
+	 * PARALLEL_KEY_TUPLESORT tuplesort workspace
 	 */
 	estbtshared = _bt_parallel_estimate_shared(snapshot);
 	shm_toc_estimate_chunk(>estimator, estbtshared);
@@ -1234,7 +1236,7 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
 
 	/*
 	 * Unique case requires a second spool, and so we may have to account for
-	 * a third shared workspace -- PARALLEL_KEY_TUPLESORT_SPOOL2
+	 * another shared workspace for that -- PARALLEL_KEY_TUPLESORT_SPOOL2
 	 */
 	if (!btspool->isunique)
 		shm_toc_estimate_keys(>estimator, 2);
@@ -1244,6 +1246,11 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
 		shm_toc_estimate_keys(>estimator, 3);
 	}
 
+	/* Finally, estimate PARALLEL_KEY_QUERY_TEXT space */
+	querylen = strlen(debug_query_string);
+	shm_toc_estimate_chunk(>estimator, querylen + 1);
+	shm_toc_estimate_keys(>estimator, 1);
+
 	/* Everyone's had a chance to ask for space, so now create the DSM */
 	InitializeParallelDSM(pcxt);
 
@@ -1293,6 +1300,11 @@ _bt_begin_parallel(BTBuildState *buildstate, bool isconcurrent, int request)
 		shm_toc_insert(pcxt->toc, PARALLEL_KEY_TUPLESORT_SPOOL2, sharedsort2);
 	}
 
+	/* Store query string for workers */
+	sharedquery = (char *) shm_toc_allocate(pcxt->toc, querylen + 1);
+	memcpy(sharedquery, debug_query_string, querylen + 1);
+	shm_toc_insert(pcxt->toc, PARALLEL_KEY_QUERY_TEXT, sharedquery);
+
 	/* Launch workers, saving status for leader/caller */
 	LaunchParallelWorkers(pcxt);
 	btleader->pcxt = pcxt;
@@ -1459,6 +1471,7 @@

pgstat_report_activity() and parallel CREATE INDEX (was: Parallel index creation & pg_stat_activity)

2018-03-01 Thread Peter Geoghegan 
On Wed, Feb 28, 2018 at 9:44 AM, Andres Freund  wrote:
> Looks like we're not doing a pgstat_report_activity() in the workers?
> Any argument for not doing so?

No. Just an oversight. Looks like _bt_parallel_build_main() should
call pgstat_report_activity(), just like ParallelQueryMain().

I'll come up with a patch soon.

-- 
Peter Geoghegan

Re: Parallel index creation & pg_stat_activity

2018-02-28 Thread Andres Freund 
Hi Peter,

On 2018-02-28 16:50:44 +, Phil Florent wrote:
> With an index creation (create index t1_i1 on t1(c1, c2);) I have this kind 
> of output :
> 
> ./t -d 20 -o "pid, backend_type, query, wait_event_type, wait_event"
> busy_pc | distinct_exe | pid  |  backend_type  |   query  
>  | wait_event_type |  wait_event
> -+--+--++---+-+--
>   68 | 1 / 136  | 8262 | client backend | create index t1_i1 on 
> t1(c1, c2); | IO  | DataFileRead
>   26 | 1 / 53   | 8262 | client backend | create index t1_i1 on 
> t1(c1, c2); | |
>6 | 1 / 11   | 8262 | client backend | create index t1_i1 on 
> t1(c1, c2); | IO  | BufFileWrite
> (3 rows)

> No parallel worker. At least one parallel worker was active though, I could 
> see its work with a direct query on pg_stat_activity or a ps -ef :
> 
> ...
> postgres  8262  8230  7 08:54 ?00:22:46 postgres: 11/main: postgres 
> postgres [local] CREATE INDEX
> ...
> postgres  9833  8230 23 14:17 ?00:00:33 postgres: 11/main: parallel 
> worker for PID 8262
> ...

Looks like we're not doing a pgstat_report_activity() in the workers?
Any argument for not doing so?

Greetings,

Andres Freund