Re: [HACKERS] Try to fix endless loop in ecpg with informix mode
On Wed, Nov 1, 2017 at 12:22 PM, 高增琦 wrote: > Any comments? > Hi, You should register these patches for the next commitfest at https://commitfest.postgresql.org/15/. As Michael pointed out earlier, this commitfest will start soon so you should add your patches quickly. Regards. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] unsafe tuple releasing in get_default_partition_oid
On Sat, Oct 28, 2017 at 11:13 AM, Robert Haas wrote: > On Sat, Oct 28, 2017 at 10:03 AM, Julien Rouhaud wrote: >> I just noticed that get_default_partition_oid() tries to release the >> tuple even if it isn't valid. >> Trivial patch attached. > > Oops. I wonder how that managed to survive testing. > > Committed. Thanks! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] unsafe tuple releasing in get_default_partition_oid
Hi, I just noticed that get_default_partition_oid() tries to release the tuple even if it isn't valid. Trivial patch attached. diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index 07fdf66c38..66ec214e02 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@ -2831,9 +2831,9 @@ get_default_partition_oid(Oid parentId) part_table_form = (Form_pg_partitioned_table) GETSTRUCT(tuple); defaultPartId = part_table_form->partdefid; + ReleaseSysCache(tuple); } - ReleaseSysCache(tuple); return defaultPartId; } -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 64-bit queryId?
On Fri, Oct 20, 2017 at 3:44 PM, Robert Haas wrote: > On Thu, Oct 19, 2017 at 2:11 AM, Julien Rouhaud wrote: >> I agree, and I'm perfectly fine with adding a comment around pgssHashKey. >> >> PFA a patch to warn about the danger. > > Committed a somewhat different version of this - hope you are OK with > the result. That's much better than what I proposed. Thanks a lot! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 64-bit queryId?
On Thu, Oct 19, 2017 at 1:08 AM, Michael Paquier
wrote:
> On Thu, Oct 19, 2017 at 4:12 AM, Robert Haas wrote:
>> On Wed, Oct 18, 2017 at 9:20 AM, Julien Rouhaud wrote:
>>> WIth current pgssHashKey definition, there shouldn't be padding bits,
>>> so it should be safe. But I wonder if adding an explicit memset() of
>>> the key in pgss_store() could avoid extension authors to have
>>> duplicate entries if they rely on this code, or prevent future issue
>>> in the unlikely case of adding other fields to pgssHashKey.
>>
>> I guess we should probably add additional comment to the definition of
>> pgssHashKey warning of the danger. I'm OK with adding a memset if
>> somebody can promise me it will get optimized away by all reasonably
>> commonly-used compilers, but I'm not that keen on adding more cycles
>> to protect against a hypothetical danger.
>
> A comment is an adapted answer for me too.
I agree, and I'm perfectly fine with adding a comment around pgssHashKey.
PFA a patch to warn about the danger.
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c
b/contrib/pg_stat_statements/pg_stat_statements.c
index b04b4d6ce1..829ee69f51 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -124,7 +124,10 @@ typedef enum pgssVersion
/*
* Hashtable key that defines the identity of a hashtable entry. We separate
- * queries by user and by database even if they are otherwise identical.
+ * queries by user and by database even if they are otherwise identical. Be
+ * careful when adding new fields, tag_hash() is used to compute the hash key,
+ * so we rely on the fact that no padding bit is present in this structure.
+ * Otherwise, we'd have to zero the key variable in pgss_store.
*/
typedef struct pgssHashKey
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 64-bit queryId?
On Thu, Oct 12, 2017 at 2:46 AM, Michael Paquier wrote: > On Thu, Oct 12, 2017 at 9:05 AM, Robert Haas wrote: >> On Wed, Oct 4, 2017 at 9:00 PM, Michael Paquier >> wrote: >>> v4 looks correct to me. Testing it through (pgbench and some custom >>> queries) I have not spotted issues. If the final decision is to use >>> 64-bit query IDs, then this patch could be pushed. >> >> Cool. Committed, thanks for the review. > > The final result looks fine for me. Thanks. Sorry for replying so late, but I have a perhaps naive question about the hashtable handling with this new version. IIUC, the shared hash table is now created with HASH_BLOBS instead of HASH_FUNCTION, so since sizeof(pgssHashKey) != sizeof(uint32) the hash table will use tag_hash() to compute the hash key. tag_hash() uses all the bits present in the given struct, so this can be problematic if padding bits are not zeroed, which isn't garanted by C standard for local variable. WIth current pgssHashKey definition, there shouldn't be padding bits, so it should be safe. But I wonder if adding an explicit memset() of the key in pgss_store() could avoid extension authors to have duplicate entries if they rely on this code, or prevent future issue in the unlikely case of adding other fields to pgssHashKey. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] oversight in EphemeralNamedRelation support
On Fri, Oct 13, 2017 at 5:22 AM, Tom Lane wrote: > Thomas Munro writes: >> On Fri, Oct 13, 2017 at 12:46 PM, Tom Lane wrote: >>> Yeah, I agree --- personally I'd never write a query like that. But >>> the fact that somebody ran into it when v10 has been out for barely >>> a week suggests that people are doing it. > >> Not exactly -- Julien's bug report was about a *qualified* reference >> being incorrectly rejected. > > Nonetheless, he was using a CTE name equivalent to the name of the > query's target table. That's already confusing IMV ... and it does > not seem unreasonable to guess that he only qualified the target > because it stopped working unqualified. FWIW, the original (and much more complex) query Hugo sent me was inserting data in a qualified table name (the schema wasn't public, and I assume not in his search_path). -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] oversight in EphemeralNamedRelation support
On Mon, Oct 9, 2017 at 10:43 PM, Thomas Munro
wrote:
>
> I suppose we could consider moving the schemaname check into
> getRTEForSpecialRelationType(), since otherwise both callers need to
> do that (and as you discovered, one forgot).
Thanks for the feedback. That was my first idea, but I assumed there
could be future use for this function on qualified RangeVar if it
wasn't done this way.
I agree it'd be much safer, so v2 attached, check moved in
getRTEForSpecialRelationType().
diff --git a/src/backend/parser/parse_clause.c
b/src/backend/parser/parse_clause.c
index 9ff80b8b40..255f485494 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -184,8 +184,10 @@ setTargetTable(ParseState *pstate, RangeVar *relation,
RangeTblEntry *rte;
int rtindex;
- /* So far special relations are immutable; so they cannot be targets. */
+ /* Check if it's a CTE or tuplestore reference */
rte = getRTEForSpecialRelationTypes(pstate, relation);
+
+ /* So far special relations are immutable; so they cannot be targets. */
if (rte != NULL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -1072,6 +1074,12 @@ transformRangeTableSample(ParseState *pstate,
RangeTableSample *rts)
}
+/*
+ * getRTEForSpecialRelationTypes
+ *
+ * If given RangeVar if a CTE reference or an EphemeralNamedRelation, return
+ * the transformed RangeVar otherwise return NULL
+ */
static RangeTblEntry *
getRTEForSpecialRelationTypes(ParseState *pstate, RangeVar *rv)
{
@@ -1079,6 +1087,13 @@ getRTEForSpecialRelationTypes(ParseState *pstate,
RangeVar *rv)
Index levelsup;
RangeTblEntry *rte = NULL;
+ /*
+* if it is a qualified name, it can't be a CTE or tuplestore
+* reference
+*/
+ if (rv->schemaname)
+ return NULL;
+
cte = scanNameSpaceForCTE(pstate, rv->relname, &levelsup);
if (cte)
rte = transformCTEReference(pstate, rv, cte, levelsup);
@@ -1119,15 +1134,11 @@ transformFromClauseItem(ParseState *pstate, Node *n,
/* Plain relation reference, or perhaps a CTE reference */
RangeVar *rv = (RangeVar *) n;
RangeTblRef *rtr;
- RangeTblEntry *rte = NULL;
+ RangeTblEntry *rte;
int rtindex;
- /*
-* if it is an unqualified name, it might be a CTE or tuplestore
-* reference
-*/
- if (!rv->schemaname)
- rte = getRTEForSpecialRelationTypes(pstate, rv);
+ /* Check if it's a CTE or tuplestore reference */
+ rte = getRTEForSpecialRelationTypes(pstate, rv);
/* if not found above, must be a table reference */
if (!rte)
diff --git a/src/test/regress/expected/with.out
b/src/test/regress/expected/with.out
index c32a490580..53ea9991b2 100644
--- a/src/test/regress/expected/with.out
+++ b/src/test/regress/expected/with.out
@@ -2275,3 +2275,7 @@ with ordinality as (select 1 as x) select * from
ordinality;
-- check sane response to attempt to modify CTE relation
WITH d AS (SELECT 42) INSERT INTO d VALUES (1);
ERROR: relation "d" cannot be the target of a modifying statement
+-- check qualified relation name doesn't conflict with CTE name
+CREATE TABLE public.self (id integer);
+WITH self AS (SELECT 42) INSERT INTO public.self SELECT * from self;
+DROP TABLE public.self;
diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql
index 8ae5184d0f..17f32c3c87 100644
--- a/src/test/regress/sql/with.sql
+++ b/src/test/regress/sql/with.sql
@@ -1031,3 +1031,8 @@ with ordinality as (select 1 as x) select * from
ordinality;
-- check sane response to attempt to modify CTE relation
WITH d AS (SELECT 42) INSERT INTO d VALUES (1);
+
+-- check qualified relation name doesn't conflict with CTE name
+CREATE TABLE public.self (id integer);
+WITH self AS (SELECT 42) INSERT INTO public.self SELECT * from self;
+DROP TABLE public.self;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] oversight in EphemeralNamedRelation support
Hi,
Hugo Mercier (in Cc) reported me an error in a query, failing since pg10.
Simple test case to reproduce:
CREATE TABLE public.test (id integer);
WITH test AS (select 42) INSERT INTO public.test SELECT * FROM test;
which will fail with "relation "test" cannot be the target of a
modifying statement".
IIUC, that's an oversight in 18ce3a4ab22, where setTargetTable()
doesn't exclude qualified relation when searching for special
relation.
PFA a simple patch to fix this issue, with updated regression test.
Regards.
diff --git a/src/backend/parser/parse_clause.c
b/src/backend/parser/parse_clause.c
index 9ff80b8b40..48a065e41c 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -181,11 +181,17 @@ int
setTargetTable(ParseState *pstate, RangeVar *relation,
bool inh, bool alsoSource, AclMode requiredPerms)
{
- RangeTblEntry *rte;
+ RangeTblEntry *rte = NULL;
int rtindex;
+ /*
+* if it is an unqualified name, it might be a CTE or tuplestore
+* reference
+*/
+ if (!relation->schemaname)
+ rte = getRTEForSpecialRelationTypes(pstate, relation);
+
/* So far special relations are immutable; so they cannot be targets. */
- rte = getRTEForSpecialRelationTypes(pstate, relation);
if (rte != NULL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
diff --git a/src/test/regress/expected/with.out
b/src/test/regress/expected/with.out
index c32a490580..53ea9991b2 100644
--- a/src/test/regress/expected/with.out
+++ b/src/test/regress/expected/with.out
@@ -2275,3 +2275,7 @@ with ordinality as (select 1 as x) select * from
ordinality;
-- check sane response to attempt to modify CTE relation
WITH d AS (SELECT 42) INSERT INTO d VALUES (1);
ERROR: relation "d" cannot be the target of a modifying statement
+-- check qualified relation name doesn't conflict with CTE name
+CREATE TABLE public.self (id integer);
+WITH self AS (SELECT 42) INSERT INTO public.self SELECT * from self;
+DROP TABLE public.self;
diff --git a/src/test/regress/sql/with.sql b/src/test/regress/sql/with.sql
index 8ae5184d0f..17f32c3c87 100644
--- a/src/test/regress/sql/with.sql
+++ b/src/test/regress/sql/with.sql
@@ -1031,3 +1031,8 @@ with ordinality as (select 1 as x) select * from
ordinality;
-- check sane response to attempt to modify CTE relation
WITH d AS (SELECT 42) INSERT INTO d VALUES (1);
+
+-- check qualified relation name doesn't conflict with CTE name
+CREATE TABLE public.self (id integer);
+WITH self AS (SELECT 42) INSERT INTO public.self SELECT * from self;
+DROP TABLE public.self;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Proposal] Make the optimiser aware of partitions ordering
On Tue, Sep 26, 2017 at 2:56 PM, Robert Haas wrote: > On Sat, Sep 23, 2017 at 6:29 AM, Julien Rouhaud wrote: >> That's true, but numCols, sortColdIdx etc are also used to display the >> sort key in an explain. If an append can return sorted data, it >> should also display the sort information, so I think these fields are >> still required in an Append node. > > I don't think so. An index scan doesn't output that information, nor > does a nested loop which inherits a sort order from its outer path. I > think the rule is that a plan node which takes steps to get the data > into a certain order might want to output something about that, but a > plan node which somehow gets that ordering without taking any explicit > action doesn't print anything. Oh, ok that indeed makes sense. As I said I'll remove all the useless noise and send an updated patch. Thanks again. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Proposal] Make the optimiser aware of partitions ordering
On Fri, Sep 22, 2017 at 11:09 PM, Robert Haas wrote: > During planning, *every* node has a list of pathkeys associated with > it which represent the presumed output ordering. You can support > ordered append paths without changing any data structures at all; it's > just a matter of putting pathkeys into an AppendPath. > I totally agree. > The reason why MergeAppend has extra stuff in the node (numCols, > sortColIdx, etc.) is so that it can actually perform the sort at > runtime. But this feature requires no runtime support -- that's kinda > the point -- so there's no need for it to carry that information, or > to add any new nodes. > > At least, IIUC. That's true, but numCols, sortColdIdx etc are also used to display the sort key in an explain. If an append can return sorted data, it should also display the sort information, so I think these fields are still required in an Append node. If it's ok to only add these fields in an Append node for the explain part, I'll revert the Append/MergeAppend refactoring and do some other cleanup as you advised earlier. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Proposal] Make the optimiser aware of partitions ordering
On Fri, Sep 22, 2017 at 9:58 PM, Robert Haas wrote: > On Fri, Sep 22, 2017 at 3:34 PM, Julien Rouhaud wrote: >> PFA v3 of the patch, once again rebased and that now use all the newly >> available infrastructure. >> >> I also added a check that a sorted append can't be generated when a >> default partition has been declared. > > I just took a quick look at this and was kind of surprised to see that > it didn't look much like what I would expect. > > What I would expect is: >[...] Thanks a lot for the pointers, that's really helpful! > The extensive patch does a lot of other stuff, like > whacking around the structure of AppendPath vs. MergeAppendPath, and > I'm not sure why we need or want any of that. I might be missing > something, though. That was one of the first question we had with the initial POC. The reason is that this "sorted append" is not using a merge algorithm but just appending partitions in the right order, so it looked like we could either create a new SortedAppend node, or use current Append node and allow it to return sorted data. We chose the 2nd solution, and ended up with a lot of duplicated code (all the code for the ordering), so we tried to have Append and MergeAppend share this code. I'm not at all satisfied with current shape, but I'm still not sure on what node to use for this. Do you have any idea on this? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Proposal] Make the optimiser aware of partitions ordering
On Thu, Sep 21, 2017 at 11:13 AM, Julien Rouhaud wrote:
> On Thu, Sep 21, 2017 at 10:52 AM, Ashutosh Bapat
> wrote:
>> With 9140cf8269b0c4ae002b2748d93979d535891311, we store the
>> RelOptInfos of partitions in the RelOptInfo of partitioned table. For
>> range partitioned table without default partition, they are arranged
>> in the ascending order of partition bounds. This patch may avoid
>> MergeAppend if the sort keys match partition keys by creating an
>> Append path by picking sorted paths from partition RelOptInfos in
>> order. You may use slightly modified version of
>> add_paths_to_append_rel().
>
>
> Yes, I just saw this commit this morning, and this is exactly what I
> was missing, thanks for the pointer and the patch!
PFA v3 of the patch, once again rebased and that now use all the newly
available infrastructure.
I also added a check that a sorted append can't be generated when a
default partition has been declared.
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index c1602c59cc..20e63b3204 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -80,6 +80,8 @@ static void show_upper_qual(List *qual, const char *qlabel,
ExplainState *es);
static void show_sort_keys(SortState *sortstate, List *ancestors,
ExplainState *es);
+static void show_append_keys(AppendState *mstate, List *ancestors,
+ ExplainState *es);
static void show_merge_append_keys(MergeAppendState *mstate, List *ancestors,
ExplainState *es);
static void show_agg_keys(AggState *astate, List *ancestors,
@@ -1602,6 +1604,10 @@ ExplainNode(PlanState *planstate, List *ancestors,
show_sort_keys(castNode(SortState, planstate),
ancestors, es);
show_sort_info(castNode(SortState, planstate), es);
break;
+ case T_Append:
+ show_append_keys(castNode(AppendState, planstate),
+ ancestors,
es);
+ break;
case T_MergeAppend:
show_merge_append_keys(castNode(MergeAppendState,
planstate),
ancestors,
es);
@@ -1744,7 +1750,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
ancestors, es);
break;
case T_MergeAppend:
- ExplainMemberNodes(((MergeAppend *) plan)->mergeplans,
+ ExplainMemberNodes(((MergeAppend*)
plan)->plan.appendplans,
((MergeAppendState
*) planstate)->mergeplans,
ancestors, es);
break;
@@ -1935,6 +1941,22 @@ show_sort_keys(SortState *sortstate, List *ancestors,
ExplainState *es)
ancestors, es);
}
+/*
+ * Likewise, for an Append node.
+ */
+static void
+show_append_keys(AppendState *mstate, List *ancestors,
+ ExplainState *es)
+{
+ Append *plan = (Append *) mstate->ps.plan;
+
+ show_sort_group_keys((PlanState *) mstate, "Sort Key",
+plan->numCols,
plan->sortColIdx,
+plan->sortOperators,
plan->collations,
+plan->nullsFirst,
+ancestors, es);
+}
+
/*
* Likewise, for a MergeAppend node.
*/
@@ -1942,7 +1964,7 @@ static void
show_merge_append_keys(MergeAppendState *mstate, List *ancestors,
ExplainState *es)
{
- MergeAppend *plan = (MergeAppend *) mstate->ps.plan;
+ Append *plan = (Append *) mstate->ps.plan;
show_sort_group_keys((PlanState *) mstate, "Sort Key",
plan->numCols,
plan->sortColIdx,
diff --git a/src/backend/executor/nodeMergeAppend.c
b/src/backend/executor/nodeMergeAppend.c
index 6bf490bd70..601f2547d3 100644
--- a/src/backend/executor/nodeMergeAppend.c
+++ b/src/backend/executor/nodeMergeAppend.c
@@ -64,6 +64,7 @@ MergeAppendState *
ExecInitMergeAppend(MergeAppend *node, EState *estate, int eflags)
{
MergeAppendState *mergestate = makeNode(MergeAppendState);
+ Append *append = &node->plan;
PlanState **mergeplanstates;
int nplans;
int i;
@@ -76,12 +77,12 @@ ExecInitMergeAppend(MergeAppend *node, ESt
Re: [HACKERS] pg_stat_wal_write statistics view
Hello,
On Wed, Sep 13, 2017 at 3:01 AM, Haribabu Kommi
wrote:
> I ran the latest performance tests with and without IO times, there is an
> overhead involved with IO time calculation and didn't observe any
> performance
> overhead with normal stats. May be we can enable the IO stats only in the
> development environment to find out the IO stats?
>
-1 for me to have these columns depending on a GUC *and* wether it's a
debug or assert build (unless this behaviour already exists for other
functions, but AFAIK that's not the case).
> I added the other background stats to find out how much WAL write is
> carried out by the other background processes. Now I am able to collect
> the stats for the other background processes also after the pgbench test.
> So I feel now the separate background stats may be useful.
>
> Attached latest patch, performance test results and stats details with
> separate background stats and also combine them with backend including
> the IO stats also.
>
The results seem to vary too much to have a strong opinion, but it
looks like the timing instrumentation can be too expensive, so I'd be
-1 on adding this overhead to track_io_timing.
I have some minor comments on the last patch:
+
+ backend_writes
+ bigint
+ Number of WAL writes that are carried out by the backend
process
it should be backend processes
+#define NUM_PG_STAT_WALWRITE_ATTS 16
+
+Datum
+pg_stat_get_walwrites(PG_FUNCTION_ARGS)
+{
+ TupleDesc tupdesc;
+ Datum values[NUM_PG_STAT_WALWRITE_ATTS];
+ boolnulls[NUM_PG_STAT_WALWRITE_ATTS];
For consistency, the #define should be just before the tupdesc
declaration, and be named PG_STAT_GET_WALWRITE_COLS
+ PgStat_Counter backend_writes; /* No of writes by backend */
+ PgStat_Counter backend_dirty_writes;/* No of dirty writes by
+* backend when WAL buffers
+* full */
+ PgStat_Counter backend_write_blocks;/* Total no of pages
written by backend */
+ PgStat_Counter backend_total_write_time;/* Total write time in
+* milliseconds by backend */
+ PgStat_Counter backend_total_sync_time; /* Total write time in
+* milliseconds by backend */
these comments should all be say "backends" for consistency
+-- There will surely and maximum one record
+select count(*) > 0 as ok from pg_stat_walwrites;
The test should be count(*) = 1
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Proposal] Make the optimiser aware of partitions ordering
On Thu, Sep 21, 2017 at 10:52 AM, Ashutosh Bapat wrote: > With 9140cf8269b0c4ae002b2748d93979d535891311, we store the > RelOptInfos of partitions in the RelOptInfo of partitioned table. For > range partitioned table without default partition, they are arranged > in the ascending order of partition bounds. This patch may avoid > MergeAppend if the sort keys match partition keys by creating an > Append path by picking sorted paths from partition RelOptInfos in > order. You may use slightly modified version of > add_paths_to_append_rel(). Yes, I just saw this commit this morning, and this is exactly what I was missing, thanks for the pointer and the patch! -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Proposal] Make the optimiser aware of partitions ordering
On Thu, Aug 31, 2017 at 5:30 AM, Peter Eisentraut
wrote:
> On 3/20/17 11:03, Ronan Dunklau wrote:
>>> Great idea. This is too late for v10 at this point, but please add it
>>> to the next CommitFest so we don't forget about it.
>> I know it is too late, and thought that it was too early to add it to the
>> commitfest properly since so many design decisions should be discussed.
>> Thanks
>> for the feedback, I added it.
>
> This patch no longer applies. Please send an update.
Thanks for the reminder, and sorry for very very late answer. PFA
rebased patch on current head.
I unfortunately didn't have time yet to read carefully the newly added
infrastructure (30833ba154e0c1106d61e3270242dc5999a3e4f3 and
following), so this patch is still using its own copy of partitions
list. I hope I can work on it shortly, but I prefer to send the
rebased version now, since it's still a POC with knowns problems and
questions, and I'll more than welcome any guidance with it.
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index c1602c59cc..20e63b3204 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -80,6 +80,8 @@ static void show_upper_qual(List *qual, const char *qlabel,
ExplainState *es);
static void show_sort_keys(SortState *sortstate, List *ancestors,
ExplainState *es);
+static void show_append_keys(AppendState *mstate, List *ancestors,
+ ExplainState *es);
static void show_merge_append_keys(MergeAppendState *mstate, List *ancestors,
ExplainState *es);
static void show_agg_keys(AggState *astate, List *ancestors,
@@ -1602,6 +1604,10 @@ ExplainNode(PlanState *planstate, List *ancestors,
show_sort_keys(castNode(SortState, planstate),
ancestors, es);
show_sort_info(castNode(SortState, planstate), es);
break;
+ case T_Append:
+ show_append_keys(castNode(AppendState, planstate),
+ ancestors,
es);
+ break;
case T_MergeAppend:
show_merge_append_keys(castNode(MergeAppendState,
planstate),
ancestors,
es);
@@ -1744,7 +1750,7 @@ ExplainNode(PlanState *planstate, List *ancestors,
ancestors, es);
break;
case T_MergeAppend:
- ExplainMemberNodes(((MergeAppend *) plan)->mergeplans,
+ ExplainMemberNodes(((MergeAppend*)
plan)->plan.appendplans,
((MergeAppendState
*) planstate)->mergeplans,
ancestors, es);
break;
@@ -1935,6 +1941,22 @@ show_sort_keys(SortState *sortstate, List *ancestors,
ExplainState *es)
ancestors, es);
}
+/*
+ * Likewise, for an Append node.
+ */
+static void
+show_append_keys(AppendState *mstate, List *ancestors,
+ ExplainState *es)
+{
+ Append *plan = (Append *) mstate->ps.plan;
+
+ show_sort_group_keys((PlanState *) mstate, "Sort Key",
+plan->numCols,
plan->sortColIdx,
+plan->sortOperators,
plan->collations,
+plan->nullsFirst,
+ancestors, es);
+}
+
/*
* Likewise, for a MergeAppend node.
*/
@@ -1942,7 +1964,7 @@ static void
show_merge_append_keys(MergeAppendState *mstate, List *ancestors,
ExplainState *es)
{
- MergeAppend *plan = (MergeAppend *) mstate->ps.plan;
+ Append *plan = (Append *) mstate->ps.plan;
show_sort_group_keys((PlanState *) mstate, "Sort Key",
plan->numCols,
plan->sortColIdx,
diff --git a/src/backend/executor/nodeMergeAppend.c
b/src/backend/executor/nodeMergeAppend.c
index 6bf490bd70..601f2547d3 100644
--- a/src/backend/executor/nodeMergeAppend.c
+++ b/src/backend/executor/nodeMergeAppend.c
@@ -64,6 +64,7 @@ MergeAppendState *
ExecInitMergeAppend(MergeAppend *node, EState *estate, int eflags)
{
MergeAppendState *mergestate = makeNode(MergeAppendState);
+ Append *append = &node->plan;
PlanState **mergeplanstates;
int nplans;
int i;
@@ -76,12 +77,12 @@ ExecInitMergeAppend(MergeAppend *node, EState *estate, int
eflags)
* Lock the non-leaf tables in the partition tree controlled by this
no
Re: [HACKERS] segfault in HEAD when too many nested functions call
On 31/07/2017 01:47, Andres Freund wrote: > Julien, could you quickly verify that everything's good for you now too? > I just checked on current HEAD (cc9f08b6b813e30789100b6b34110d8be1090ba0) and everything's good for me. Thanks! -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] proposal: psql: check env variable PSQL_PAGER
On Wed, Jul 26, 2017 at 7:11 AM, Pavel Stehule wrote: > Hi > > I wrote a special pager for psql. Surely, this pager is not good for paging > of man pages. So is not good to set it as global pager. We can introduce new > env variable PSQL_PAGER for this purpose. It can work similar like > PSQL_EDITOR variable. > +1. I used to have a psql alias just for this, so it'd definitely be useful. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] segfault in HEAD when too many nested functions call
On 21/07/2017 13:40, Andres Freund wrote: > Attached is a > patch that converts just ExecProcNode. The big change in comparison to > the earlier patch is that the assignment of the callback is now done in > the respective ExecInit* routines. As a consequence the ExecProcNode > callbacks now are static. Thanks for working on it. Just in case, I reviewed the patch and didn't find any issue with it. -- Julien Rouhaud -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pageinspect function to decode infomasks
On Thu, Jul 20, 2017 at 5:44 AM, Peter Geoghegan wrote:
> On Wed, Jul 19, 2017 at 8:33 PM, Craig Ringer wrote:
>> That's silly, so here's a patch to teach pageinspect how to decode infomasks
>> to a human readable array of flag names.
>>
>> Example:
>>
>> SELECT t_infomask, t_infomask2, flags
>> FROM heap_page_items(get_raw_page('test1', 0)),
>> LATERAL heap_infomask_flags(t_infomask, t_infomask2, true) m(flags);
>> t_infomask | t_infomask2 | flags
>> +-+
>>2816 | 2 |
>> {HEAP_XMIN_COMMITTED,HEAP_XMIN_INVALID,HEAP_XMAX_INVALID,HEAP_XMIN_FROZEN}
>> (1 row)
>
> Seems like a good idea to me.
>
+1, it'll be really helpful.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql's \r broken since e984ef5861d
On 20/07/2017 04:24, Tom Lane wrote: > I wrote: >> Ah. I don't feel like trawling the archives for the discussion right now, >> but I believe this was an intentional change to make the behavior more >> consistent. > > Oh ... a quick look in the commit log finds the relevant discussion: > https://www.postgresql.org/message-id/flat/9b4ea968-753f-4b5f-b46c-d7d3bf7c8f90%40manitou-mail.org > Oh I see. Thanks a lot, sorry for the noise. -- Julien Rouhaud -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] psql's \r broken since e984ef5861d
On 20/07/2017 03:34, Tom Lane wrote: > Julien Rouhaud writes: >> Unless I miss something, \r isn't working anymore, > > Works for me. Please describe exactly what misbehavior you're seeing. > What libreadline or libedit version are you using? > I have libreadline 7.0_p3. Here's a simple test case, last \p still show the query buffer: psql -X postgres postgres=# select version(); version PostgreSQL 10beta2@decb08ebdf on x86_64-pc-linux-gnu, compiled by gcc (Gentoo 4.9.3 p1.5, pie-0.6.4) 4.9.3, 64-bit (1 row) postgres=# \p select version(); postgres=# \r Query buffer reset (cleared). postgres=# \p select version(); On a 9.6: postgres=# select version(); version -- PostgreSQL 9.6.3@3c017a545f on x86_64-pc-linux-gnu, compiled by gcc (Gentoo 4.9.3 p1.5, pie-0.6.4) 4.9.3, 64-bit (1 row) postgres=# \p select version(); postgres=# \r Query buffer reset (cleared). postgres=# \p Query buffer is empty. -- Julien Rouhaud -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] psql's \r broken since e984ef5861d
Hello,
Unless I miss something, \r isn't working anymore, since
exec_command_print() fallback to display previous_buf if query_buf has
been freed.
Trivial patch to fix issue (free both buffers in exec_command_reset())
attached.
Regards.
--
Julien Rouhaud
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 14c64208ca..4087532052 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -109,7 +109,7 @@ static backslashResult exec_command_prompt(PsqlScanState scan_state, bool active
static backslashResult exec_command_pset(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_quit(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_reset(PsqlScanState scan_state, bool active_branch,
- PQExpBuffer query_buf);
+ PQExpBuffer query_buf, PQExpBuffer previous_buf);
static backslashResult exec_command_s(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_set(PsqlScanState scan_state, bool active_branch);
static backslashResult exec_command_setenv(PsqlScanState scan_state, bool active_branch,
@@ -369,7 +369,7 @@ exec_command(const char *cmd,
else if (strcmp(cmd, "q") == 0 || strcmp(cmd, "quit") == 0)
status = exec_command_quit(scan_state, active_branch);
else if (strcmp(cmd, "r") == 0 || strcmp(cmd, "reset") == 0)
- status = exec_command_reset(scan_state, active_branch, query_buf);
+ status = exec_command_reset(scan_state, active_branch, query_buf, previous_buf);
else if (strcmp(cmd, "s") == 0)
status = exec_command_s(scan_state, active_branch);
else if (strcmp(cmd, "set") == 0)
@@ -2060,11 +2060,12 @@ exec_command_quit(PsqlScanState scan_state, bool active_branch)
*/
static backslashResult
exec_command_reset(PsqlScanState scan_state, bool active_branch,
- PQExpBuffer query_buf)
+ PQExpBuffer query_buf, PQExpBuffer previous_buf)
{
if (active_branch)
{
resetPQExpBuffer(query_buf);
+ resetPQExpBuffer(previous_buf);
psql_scan_reset(scan_state);
if (!pset.quiet)
puts(_("Query buffer reset (cleared)."));
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] segfault in HEAD when too many nested functions call
On 18/07/2017 14:04, Andres Freund wrote: > On 2017-07-17 23:04:43 +0200, Julien Rouhaud wrote: >> Is it v11 material or is there any chance to make it in v10? > > I think it'd make sense to apply the first to v10 (and earlier), and the > second to v11. I think this isn't a terribly risky patch, but it's > still a relatively large change for this point in the development > cycle. I'm willing to reconsider, but that's my default. Agreed. -- Julien Rouhaud -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] segfault in HEAD when too many nested functions call
On 17/07/2017 16:57, Andres Freund wrote: > The latter obviously isn't ready, but might make clearer what I'm > thinking about. It did for me, and FWIW I like this approach. > If we were to go this route we'd have to probably move > the callback assignment into the ExecInit* routines, and possibly > replace the switch in ExecInitNode() with another callback, assigned in > make_*, and implement callbacks for ExecShutdown, MultiExecProcNode etc. > > This results in a good speedup in tpc-h btw: > master total min: 46434.897 cb min: 44778.228 [diff -3.70] Is it v11 material or is there any chance to make it in v10? -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] segfault in HEAD when too many nested functions call
On 15/07/2017 17:22, Tom Lane wrote: > Julien Rouhaud writes: >> Since b8d7f053c5c2bf2a7e8734fe3327f6a8bc711755 (Andres in Cc), if you >> write queries which result in infinite recursion (or just too many >> nested function calls), execution ends with segfault instead of intended >> exhausted max_stack_depth: > > Yes. We discussed this before the patch went in [1]. Ah, I totally forgot about it, sorry. > I wanted to put > a stack depth check in ExecProcNode(), and still do. Andres demurred, > claiming that that was too much overhead, but didn't really provide a > credible alternative. The thread drifted off without any resolution, > but clearly we need to do something before 10.0 final. > I wanted to add an open item but I see you already did, thanks! >> Please find attached a trivial patch to fix this. I'm not sure >> ExecMakeTableFunctionResult() is the best or only place that needs to >> check the stack depth. > > I don't think that that's adequate at all. > > I experimented with a variant that doesn't go through > ExecMakeTableFunctionResult: > > [...] > This manages not to crash, but I think the reason it does not is purely > accidental: "SELECT so()" has a nontrivial targetlist so we end up running > ExecBuildProjectionInfo on that, meaning that a fresh expression > compilation happens at every nesting depth, and there are > check_stack_depth calls in expression compilation. Surely that's > something we'd try to improve someday. Or it could accidentally get > broken by unrelated changes in the way plpgsql sets up queries to be > executed. > > I still think that we really need to add a check in ExecProcNode(). > Even if there's an argument to be made that every recursion would > somewhere go through ExecMakeTableFunctionResult, very large/complex > queries could result in substantial stack getting chewed up before > we get to that --- and we don't have an infinite amount of stack slop. I was pretty sceptical about checking depth in ExecMakeTableFunctionResult() only vs ExecProcNode(), and this has finished convincing me so I definitely agree. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] segfault in HEAD when too many nested functions call
Hello, Since b8d7f053c5c2bf2a7e8734fe3327f6a8bc711755 (Andres in Cc), if you write queries which result in infinite recursion (or just too many nested function calls), execution ends with segfault instead of intended exhausted max_stack_depth: Program received signal SIGSEGV, Segmentation fault. 0x00df851e in DirectFunctionCall1Coll ( func=, collation=, arg1=) at fmgr.c:708 Please find attached a trivial patch to fix this. I'm not sure ExecMakeTableFunctionResult() is the best or only place that needs to check the stack depth. I also attached a simple sql file to reproduce the issue if needed. Should I add a regression test based on it? -- Julien Rouhaud http://dalibo.com - http://dalibo.org segfault.sql Description: application/sql diff --git a/src/backend/executor/execSRF.c b/src/backend/executor/execSRF.c index 138e86ac67..9d257d4d88 100644 --- a/src/backend/executor/execSRF.c +++ b/src/backend/executor/execSRF.c @@ -117,6 +117,9 @@ ExecMakeTableFunctionResult(SetExprState *setexpr, MemoryContext oldcontext; bool first_time = true; + /* Guard against stack overflow due to overly nested functions call */ + check_stack_depth(); + callerContext = CurrentMemoryContext; funcrettype = exprType((Node *) setexpr->expr); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Pluggable storage
On 23/06/2017 16:07, Tomas Vondra wrote: > > I think you're probably right - GIN does compress the posting lists by > exploiting the TID redundancy (that it's page/offset structure), and I > don't think there are other index AMs doing that. > > But I'm not sure we can simply rely on that - it's possible people will > try to improve other index types (e.g. by adding similar compression to > other index types). That reminds me https://www.postgresql.org/message-id/55e4051b.7020...@postgrespro.ru where Anastasia proposed something similar. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Typo in insert.sgml
On 20/06/2017 20:34, Peter Eisentraut wrote: > On 6/18/17 03:16, Julien Rouhaud wrote: >> Patch attached. > > This was not a typo, this was intentional. > Oh, sorry. I'm not a native english speaker, that sounded really weird. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Typo in drop_publication.sgml
On Sun, Jun 18, 2017 at 07:42:54PM +0200, Magnus Hagander wrote: > On Sun, Jun 18, 2017 at 8:46 AM, Julien Rouhaud > wrote: > > > Patch attached. > > > > Applied, thanks. > Thanks. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Typo in insert.sgml
Patch attached. -- Julien Rouhaud http://dalibo.com - http://dalibo.org diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml index 94dad00870..0e327e5b2e 100644 --- a/doc/src/sgml/ref/insert.sgml +++ b/doc/src/sgml/ref/insert.sgml @@ -228,7 +228,7 @@ INSERT INTO table_name [ AS INSERT INTO tbl2 OVERRIDING USER VALUE SELECT * FROM tbl1 will copy from tbl1 all columns that are not identity columns in tbl2 but will continue -the sequence counters for any identity columns. +to use the sequence counters for any identity columns. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Typo in drop_publication.sgml
Patch attached. -- Julien Rouhaud http://dalibo.com - http://dalibo.org diff --git a/doc/src/sgml/ref/drop_publication.sgml b/doc/src/sgml/ref/drop_publication.sgml index 517d142251..8e45a43982 100644 --- a/doc/src/sgml/ref/drop_publication.sgml +++ b/doc/src/sgml/ref/drop_publication.sgml @@ -46,8 +46,8 @@ DROP PUBLICATION [ IF EXISTS ] name IF EXISTS - Do not throw an error if the extension does not exist. A notice is issued - in this case. + Do not throw an error if the publication does not exist. A notice is + issued in this case. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Typo in planstats.sgml
On 18/06/2017 01:03, Peter Eisentraut wrote: > On 6/17/17 05:00, Julien Rouhaud wrote: >> A "condition" is missing, patch attached. > > fixed > Thanks. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Typo in CREATE SUBSCRIPTION documentation
On Sat, Jun 17, 2017 at 10:24:32AM -0400, Peter Eisentraut wrote: > On 6/15/17 15:19, Julien Rouhaud wrote: > > Hi, > > > > I just found $SUBJECT, patch attached. > > fixed Thanks! -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Typo in planstats.sgml
A "condition" is missing, patch attached. -- Julien Rouhaud http://dalibo.com - http://dalibo.org diff --git a/doc/src/sgml/planstats.sgml b/doc/src/sgml/planstats.sgml index 8caf297f85..838fcda6d2 100644 --- a/doc/src/sgml/planstats.sgml +++ b/doc/src/sgml/planstats.sgml @@ -501,8 +501,8 @@ EXPLAIN (ANALYZE, TIMING OFF) SELECT * FROM t WHERE a = 1; of this clause to be 1%. By comparing this estimate and the actual number of rows, we see that the estimate is very accurate (in fact exact, as the table is very small). Changing the -WHERE to use the b column, an identical -plan is generated. But observe what happens if we apply the same +WHERE condition to use the b column, an +identical plan is generated. But observe what happens if we apply the same condition on both columns, combining them with AND: -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Typo in CREATE SUBSCRIPTION documentation
Hi, I just found $SUBJECT, patch attached. -- Julien Rouhaud http://dalibo.com - http://dalibo.org diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml index 77bf87681b..3ca06fb3c3 100644 --- a/doc/src/sgml/ref/create_subscription.sgml +++ b/doc/src/sgml/ref/create_subscription.sgml @@ -32,7 +32,7 @@ CREATE SUBSCRIPTION subscription_nameDescription - CREATE SUBSCRIPTION adds a new subscription for a + CREATE SUBSCRIPTION adds a new subscription for the current database. The subscription name must be distinct from the name of any existing subscription in the database. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Typo in BRIN documentation
On Tue, Jun 13, 2017 at 11:29:30AM -0400, Peter Eisentraut wrote: > On 6/13/17 07:53, Julien Rouhaud wrote: > > I just found this typo while doing french translation, patch attached. > > fixed > Thanks ! -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Typo in BRIN documentation
Hi, I just found this typo while doing french translation, patch attached. Regards. -- Julien Rouhaud http://dalibo.com - http://dalibo.org diff --git a/doc/src/sgml/brin.sgml b/doc/src/sgml/brin.sgml index ad11109775..8dcc29925b 100644 --- a/doc/src/sgml/brin.sgml +++ b/doc/src/sgml/brin.sgml @@ -81,7 +81,7 @@ occur. (This last trigger is disabled by default and can be enabled with the autosummarize parameter.) Conversely, a range can be de-summarized using the - brin_desummarize_range(regclass, bigint) range, + brin_desummarize_range(regclass, bigint) function, which is useful when the index tuple is no longer a very good representation because the existing values have changed. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Do we need the gcc feature "__builtin_expect" to promote the branches prediction?
On 02/06/2017 12:50, Craig Ringer wrote: > > > On 2 Jun. 2017 16:42, "Hao Lee" <mailto:mixt...@gmail.com>> wrote: > > Hi all, >There is a lot of "if statement" in system, and GCC provides > a feature,"__builtin_expect", which let compilers know which branch > is mostly run. > > > Compilers and CPUs are really good at guessing this. > > Humans are wrong about it more than we'd like too. +1 > > It's surprisingly rarely s good idea to use branch prediction hints. > > See the vsrious Linux kernel discussions about this. > > If you find concrete sites of frequent or costly branch mis-prediction > please point them out, with benchmarks. And also see this thread: https://www.postgresql.org/message-id/CAKJS1f8yqRW3qx2CO9r4bqqvA2Vx68=3awbh8cjwtp9zxeo...@mail.gmail.com BTW Andres added support for likely/unlikely in aa3ca5e3dd6, but its usage is still really limited. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CTE inlining
On 04/05/2017 08:34, Petr Jelinek wrote: > On 03/05/17 23:24, Merlin Moncure wrote: >> On Wed, May 3, 2017 at 12:33 PM, Alvaro Herrera >> wrote: >>> David Fetter wrote: >>> >>>> When we add a "temporary" GUC, we're taking on a gigantic burden. >>>> Either we support it forever somehow, or we put it on a deprecation >>>> schedule immediately and expect to be answering questions about it for >>>> years after it's been removed. >>>> >>>> -1 for the GUC. >>> >>> Absolutely. >>> >>> So ISTM we have three choices: >>> >>> 1) we switch unmarked CTEs as inlineable by default in pg11. What seems >>> likely to happen for a user that upgrades to pg11 is that 5 out of 10 >>> CTE-using queries are going to become faster than with pg10, and they >>> are going to be happy; 4 out of five are going to see no difference, but >>> they didn't have to do anything about it; and the remaining query is >>> going to become slower, either indistinguishably so (in which case they >>> don't care and they remain happy because of the other improvements) or >>> notably so, in which case they can easily figure where to add the >>> MATERIALIZED option and regain the original performance. >> >> +1 for option 1. This change will be welcome for a large number of >> queries, but forced materialization is a real need and I use it often. >> This comes off as a very reasonable compromise in my opinion unless it >> requires major coding gymnastics to implement. >> > > +1 to this > +1 too -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [GENERAL] Column rename in an extension update script
moving this thread to -hackers, since this looks like a bug. On 01/05/2017 08:54, Philippe BEAUDOIN wrote: > Hi all, > > I am coding an update script for an extension. And I am in trouble when > trying to rename a column of an existing table. > > Just after the ALTER TABLE statement, I want to access this table. But > at this time, the altered column is not visible with its new name. > I can reproduce this issue. It looks like this is due to missing cache invalidation between the ALTER TABLE execution and next query planning (failure happens during pg_analyze_and_rewrite()). AFAICT, CommandCounterIncrement() is responsible for handling invalidation messages, but in execute_sql_string() this function is called before executing a Stmt instead of after. Moving the CommandCounterIncrement() just before the PopActiveSnapshot() call (and removing the final one) fixes this issue for me, but I have no idea if this is the right fix. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_stat_wal_write statistics view
On Wed, Feb 15, 2017 at 02:53:44PM +1100, Haribabu Kommi wrote:
> Here I attached patch that implements the view.
> I will add this patch to next commitfest.
Hello,
I just reviewed the patch.
First, there are some whitespace issues that make git-apply complaining (at
least lines 218 and 396).
Patch is rather straightforward and works as expected, doc compiles without
issue.
I only found some minor issues:
+ One row only, showing statistics about the
+ wal writing activity. See
+ Number of wal writes that are carried out by the backend
process
WAL should be uppercase (and for some more occurences).
+
+ Number of wal writes that are carried out by background workers such as
checkpointer,
+ writer and walwriter.
I guess you meant backgroung processes?
>+ This field data will be populated only when the track_io_timing GUC is
>enabled
(and similar occurences)
track_io_timing should link to instead of
mentionning GUC.
I think you also need to update the track_io_timing description in
sgml/config.sgml to mention this new view.
+ else
+ {
+ LocalWalWritesStats.m_wal_total_write_time = 0;
+ }
(and similar ones)
The brackets seem unnecessary.
I marked the commitfest entry as waiting on author.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Preserving param location
Hello, When a query contains parameters, the original param node contains the token location. However, this information is lost when the Const node is generated, this one will only contain position -1 (unknown). FWIW, we do have a use case for this (custom extension that tracks quals statistics, which among other thing is used to regenerate query string from a pgss normalized query, thus needing the original param location). Is this something we want to get fixed? If yes, attached is a simple patch for that. Regards. -- Julien Rouhaud http://dalibo.com - http://dalibo.org diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index b19380e1b1..beb0f99144 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -2444,6 +2444,7 @@ eval_const_expressions_mutator(Node *node, int16 typLen; bool typByVal; Datum pval; + Const *con; Assert(prm->ptype == param->paramtype); get_typlenbyval(param->paramtype, @@ -2452,13 +2453,17 @@ eval_const_expressions_mutator(Node *node, pval = prm->value; else pval = datumCopy(prm->value, typByVal, typLen); - return (Node *) makeConst(param->paramtype, + + con = makeConst(param->paramtype, param->paramtypmod, param->paramcollid, (int) typLen, pval, prm->isnull, typByVal); + con->location = param->location; + + return (Node *) con; } } } -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] SortSupport for macaddr type
On Tue, Feb 28, 2017 at 08:58:24AM -0800, Brandur Leach wrote: > Hi Julien, > Hello Brandur, > Thanks for the expedient reply, even after I'd dropped the > ball for so long :) :) > Cool! I just re-read my own comment a few days later and I > think that it still mostly makes sense, but definitely open > to other edits if anyone else has one. > Ok. > > One last thing, I noticed that you added: > > > > +static int macaddr_internal_cmp(macaddr *a1, macaddr *a2); > > > > but the existing function is declared as > > > > static int32 > > macaddr_internal_cmp(macaddr *a1, macaddr *a2) > > > > I'd be in favor to declare both as int. > > Great catch! I have no idea how I missed that. I've done as > you suggested and made them both "int", which seems > consistent with SortSupport implementations elsewhere. > Great. > > After this, I think this patch will be ready for committer. > > Excellent! I've attached a new (and hopefully final) > version of the patch. > > Two final questions about the process if you'd be so kind: > > * Should I change the status on the Commitfest link [1] or > do I leave that to you (or someone else like a committer)? > This is is done by the reviewer, after check of the last patch version. I just changed the status to ready for committer! > * I've been generating a new OID value with the > `unused_oids` script, but pretty much every time I rebase > I collide with someone else's addition and need to find a > new one. Is it better for me to pick an OID in an exotic > range for my final patch, or that a committer just finds > a new one (if necessary) as they're putting it into > master? I think picking the value with unused_oids as you dd is the right thing to do. As Robert said, if this oid is used in another patch in the meantime, updating it at commit time is not a big deal. Moreover, this patch will require a catversion bump, which is meant to be done by the committer. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] SortSupport for macaddr type
On Sun, Feb 05, 2017 at 01:56:41PM -0800, Brandur Leach wrote: Hello Brandur, thanks for the updated patch! > > > * you used macaddr_cmp_internal() function name, for uuid > > the same function is named uuid_internal_cmp(). Using > > the same naming pattern is probably better. > > I was a little split on this one! It's true that UUID uses > `_internal_cmp`, but `_cmp_internal` is also used in a > number of places like `enum`, `timetz`, and `network`. I > don't have a strong feeling about it either way, so I've > changed it to `_internal_cmp` to match UUID as you > suggested. > Indeed, I should have checked more examples :/ There isn't any clear pattern for this, so I guess any one would be ok. > > * the function comment on macaddr_abbrev_convert() > > doesn't mention specific little-endian handling > > I tried to bake this into the comment text. Here are the > relevant lines of the amended version: > > * Packs the bytes of a 6-byte MAC address into a Datum and treats it as > an > * unsigned integer for purposes of comparison. On a 64-bit machine, > there > * will be two zeroed bytes of padding. The integer is converted to > native > * endianness to facilitate easy comparison. > > > * "There will be two bytes of zero padding on the least > > significant end" > > > > "least significant bits" would be better > > Also done. Here is the amended version: > > * On a 64-bit machine, zero out the 8-byte datum and copy the 6 bytes of > * the MAC address in. There will be two bytes of zero padding on the end > * of the least significant bits. > Thanks. I'm ok with this, but maybe a native english speaker would have a better opinion on this. > > * This patch will trigger quite a lot modifications after > > a pgindent run. Could you try to run pgindent on mac.c > > before sending an updated patch? > > Good call! I've run the new version through pgindent. > Thanks also, no more issue here. > Let me know if you have any further feedback and/or > suggestions. Thanks! One last thing, I noticed that you added: +static int macaddr_internal_cmp(macaddr *a1, macaddr *a2); but the existing function is declared as static int32 macaddr_internal_cmp(macaddr *a1, macaddr *a2) I'd be in favor to declare both as int. After this, I think this patch will be ready for committer. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Rename max_parallel_degree?
On Fri, Dec 02, 2016 at 07:45:56AM -0500, Robert Haas wrote: > On Thu, Dec 1, 2016 at 10:07 PM, Haribabu Kommi > wrote: > > From the recent mails, it is not clear to me what is the status of this > > patch. > > moved to next CF with "needs review" state. Please feel free to update > > the status. > > I have committed this patch. And updated the status, too! Thanks! -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Overlook in 2bd9e412?
It looks like pq_putmessage_hook and pq_flush_hook were meant for development purpose and not supposed to be committed. Attached patch remove them. -- Julien Rouhaud http://dalibo.com - http://dalibo.org diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h index 18052cb..5fac817 100644 --- a/src/include/libpq/libpq.h +++ b/src/include/libpq/libpq.h @@ -81,9 +81,6 @@ extern char *ssl_key_file; extern char *ssl_ca_file; extern char *ssl_crl_file; -extern int (*pq_putmessage_hook) (char msgtype, const char *s, size_t len); -extern int (*pq_flush_hook) (void); - extern int secure_initialize(void); extern bool secure_loaded_verify_locations(void); extern void secure_destroy(void); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] issue with track_commit_timestamp and server restart
On 24/10/2016 06:58, Craig Ringer wrote: > On 22 October 2016 at 19:51, Julien Rouhaud wrote: >> I just noticed that if track_commit_timestamp is enabled, the >> oldestCommitTsXid and newestCommitTsXid don't persist after a server >> restart, so you can't ask for the commit ts of a transaction that >> committed before the last server start, although the information is >> still available (unless of course if a freeze occured). AFAICT it >> always behave this way. > > I initially could'n't see this when tested on 8f1fb7d with a > src/test/recovery/t test script. But it turns out it's OK on immediate > shutdown and crash recovery, but not on clean shutdown and restart. > > The attached patch adds a TAP test to src/test/recovery to show this. > If you run the TAP test before recompiling with the fix it'll fail. > "make" to apply the fix, then re-run and it'll succeed. Or just > temporarily roll back the fix with: > > git checkout HEAD^1 -- src/backend/access/transam/commit_ts.c > git reset src/backend/access/transam/commit_ts.c > > and rebuild to see it fail. > > To save time running the recovery suite, just > >rm src/test/recovery/00[0-8]*.pl > > (It'd be nice to have a prove_check target to run just one test file). > > This would explain a few issues I've seen reported with BDR from the > community which I've so far been unable to reproduce, so thanks very > much for the report. > > Álvaro, would you mind checking this and committing to HEAD and 9.6? > The commits.c change only should also be committed to 9.5, but not the > TAP test. > Thanks a lot for the review, and adding the tests! -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] issue with track_commit_timestamp and server restart
I just noticed that if track_commit_timestamp is enabled, the
oldestCommitTsXid and newestCommitTsXid don't persist after a server
restart, so you can't ask for the commit ts of a transaction that
committed before the last server start, although the information is
still available (unless of course if a freeze occured). AFAICT it
always behave this way.
I'm not familiar with that code, but it looks like these counters are
supposed to be restored in StartupXLOG(), with a call to
SetCommitTsLimit(). However, this function doesn't store the new value
if ShmemVariableCache->oldestCommitTsXid is InvalidTransactionId, which
is the initial value.
So the counters are initialized on a subsequent call of
ActivateCommitTs(), which does:
if (ShmemVariableCache->oldestCommitTsXid == InvalidTransactionId)
{
ShmemVariableCache->oldestCommitTsXid =
ShmemVariableCache->newestCommitTsXid =
ReadNewTransactionId();
}
(but doesn't try to cleanup the SLRU directory btw)
leaving any previous transaction unreachable. Simple attached patch
seems to fix the issue. I tried on a master and a replica, enabling and
disabling track_commit_timestamp, and everything seemed to work as intended.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
diff --git a/src/backend/access/transam/commit_ts.c b/src/backend/access/transam/commit_ts.c
index a8d275f..7746578 100644
--- a/src/backend/access/transam/commit_ts.c
+++ b/src/backend/access/transam/commit_ts.c
@@ -844,6 +844,8 @@ SetCommitTsLimit(TransactionId oldestXact, TransactionId newestXact)
else
{
Assert(ShmemVariableCache->newestCommitTsXid == InvalidTransactionId);
+ ShmemVariableCache->oldestCommitTsXid = oldestXact;
+ ShmemVariableCache->newestCommitTsXid = newestXact;
}
LWLockRelease(CommitTsLock);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Multiple psql history files
On 18/10/2016 18:26, Jonathan Jacobson wrote: > The .psql_history file is naturally used by different DB connections > (distinguished by a different combination of host + port + database + user). > At least in my multi-database working environment, this leads sometimes > to frustration because there are commands that cannot or should not be > used by different connections. > To solve this, psql could keep a separate command history file for each > connection. You can already do this, for instance in your .psqlrc: \set HISTFILE ~/.psql_history- :HOST - :PORT - :DBNAME - :USER -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_stat_statements and non default search_path
On 16/10/2016 11:21, Craig Ringer wrote: > On 16 Oct. 2016 14:31, Julien Rouhaud wrote: >> >> On 16/10/2016 02:38, Jim Nasby wrote: >> > On 10/10/16 12:58 AM, Julien Rouhaud wrote: >> >> Unless you mean deparsing the Query instead of using raw source > text? I >> >> think that would solve this issue (and also the other issue when >> >> multiple queries are submitted at once, you get the normalized version >> >> of all the queries multiple time), but AFAIK ruleutils.c doesn't expose >> >> enough to do it (like get_query_def()), and exposing it isn't an > option. >> > >> > Why couldn't we expose it? > > I'm interested in that too, for the purpose of passing the correct > substring of a multi-statement to ProcessUtility_hook. I have another use case for this: being able to easily print what is the query (or queries) that are actually executed (ie. what's get out of the rewriter). That's useful when trying to figure out / optimize nested views hell or abuse of rules. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_stat_statements and non default search_path
On 16/10/2016 10:47, Lukas Fittl wrote: > Can somebody chime in if it would be feasible to store this in the > out-of-band query text file, and whether a patch for this would be > considered acceptable? FWIW I already have a quick and dirty patch for this, I don't think there's any major difficulty here. I still hope we can find a way to store the fully qualified objects name in the normalized query instead. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_stat_statements and non default search_path
On 16/10/2016 02:38, Jim Nasby wrote: > On 10/10/16 12:58 AM, Julien Rouhaud wrote: >> Unless you mean deparsing the Query instead of using raw source text? I >> think that would solve this issue (and also the other issue when >> multiple queries are submitted at once, you get the normalized version >> of all the queries multiple time), but AFAIK ruleutils.c doesn't expose >> enough to do it (like get_query_def()), and exposing it isn't an option. > > Why couldn't we expose it? > I'm not really sure. The only thread I could find on this topic didn't get any answer: https://www.postgresql.org/message-id/flat/5e5bb0f8-b05e-47c2-8706-f85e70a6d...@citusdata.com > BTW, after thinking about it some more, I don't see how storing the > search_path would help at all... it's not like you can do anything with > it unless you have a huge chunk of the parser available. > My use case is not really to know the fully qualified name of each identifier, but being able to optimize a problematic query found with pg_stat_statements. I can already "unjumble" it automatically, but the search_path is needed to be able to get an explain or just execute the query. Of course, I'd prefer that pgss can generate a fully qualified normalized query instead of storing the search_path. > BTW, this issue isn't limited to just tables; it affects almost every > object identifier you can put in a query, like functions, operators, > types, etc. Yes, indeed. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] macaddr 64 bit (EUI-64) datatype support
On 12/10/2016 14:32, Alvaro Herrera wrote: > Julien Rouhaud wrote: > >> and you can instead make macaddr64 support both format, and provide a >> macaddr::macaddr64 cast > > Having macaddr64 support both formats sounds nice, but how does it work? > Will we have to reserve one additional bit to select the representation? > That would make the type be 65 bits which is a clear loser IMO. > > Is it allowed to just leave 16 bits as zeroes which would indicate that > the address is EUI48? I wouldn't think so ... > >From what I read, you can indicate it's an EUI-48 address by storing FF:FF (or FF:FE for MAC-48) in 4th and 5th bytes of the EUI-64 address. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] macaddr 64 bit (EUI-64) datatype support
On 12/10/2016 09:32, Craig Ringer wrote: > On 12 October 2016 at 14:30, Haribabu Kommi wrote: > >> As we are moving to PostgreSQL 10, so are there any plans of backward >> compatiblity >> breakage, so the existing macaddr datatype itself can be changed to support >> both >> 48 and 64 bit MAC addresses. If not, I will try update the POC patch with >> more details >> similar like macaddr datatype. > > There's been some minor BC breaking, but breaking on-disk format for > pg_upgrade is a much bigger deal. I'm really not a fan of that idea. > > Just use macaddr64 if you want wide MACs. > > +1 and you can instead make macaddr64 support both format, and provide a macaddr::macaddr64 cast -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_stat_statements and non default search_path
On 10/10/2016 05:00, Jim Nasby wrote: > On 10/7/16 4:39 AM, Julien Rouhaud wrote: >> I see two ways of fixing this. First one would be to store normalized >> queries while fully qualifiying the relation's names. After a quick look >> this can't be done without storing at least a token location in >> RangeTblEntry nodes, which sounds like a bad idea. >> >> The other way would be to store the value of search_path with each pgss >> entry (either in shared_memory or in the external file). >> >> Is it something that we should fix, and if yes is any of this >> acceptable, or does someone see another way to solve this problem? > > Would another option be to temporarily set search_path to '' when > getting the query text? ISTM that would be the best option. I don't think that possible. The query text used is raw query text provided by the post_parse_analyse hook (ParseState->p_sourcetext). Unless you mean deparsing the Query instead of using raw source text? I think that would solve this issue (and also the other issue when multiple queries are submitted at once, you get the normalized version of all the queries multiple time), but AFAIK ruleutils.c doesn't expose enough to do it (like get_query_def()), and exposing it isn't an option. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_stat_statements and non default search_path
Hello, I've faced multiple times a painful limitation with pg_stat_statements. Queries are normalized based on the textual SQL statements, and the queryid is computed using objects' oids. So for instance with different search_path settings, we can end up with the same normalized query but different queryids, and there's no way to know what are the actual relations each query is using. It also means that it can be almost impossible to use the normalized query to replay a query. I see two ways of fixing this. First one would be to store normalized queries while fully qualifiying the relation's names. After a quick look this can't be done without storing at least a token location in RangeTblEntry nodes, which sounds like a bad idea. The other way would be to store the value of search_path with each pgss entry (either in shared_memory or in the external file). Is it something that we should fix, and if yes is any of this acceptable, or does someone see another way to solve this problem? Regards. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Rename max_parallel_degree?
On 03/10/2016 21:27, Robert Haas wrote: > On Fri, Sep 30, 2016 at 12:23 PM, Julien Rouhaud > wrote: >> I've already fixed every other issues mentioned upthread, but I'm facing >> a problem for this one. Assuming that the bgworker classes are supposed >> to be mutually exclusive, I don't see a simple and clean way to add such >> a check in SanityCheckBackgroundWorker(). Am I missing something >> obvious, or can someone give me some advice for this? > > My advice is "don't worry about it". If somebody thinks of something > that can be usefully added there, it will take very little time to add > it and test that it works. Don't get hung up on that for now. > Ok, thanks! Please find attached v9 of the patch, adding the parallel worker class and changing max_worker_processes default to 16 and max_parallel_workers to 8. I also added Amit's explanation for the need of a write barrier in ForgetBackgroundWorker(). I'll add this patch to the next commitfest. -- Julien Rouhaud http://dalibo.com - http://dalibo.org diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index e826c19..61c5a7c 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1984,7 +1984,7 @@ include_dir 'conf.d' Sets the maximum number of background processes that the system can support. This parameter can only be set at server start. The - default is 8. + default is 16. @@ -2006,8 +2006,9 @@ include_dir 'conf.d' Sets the maximum number of workers that can be started by a single Gather node. Parallel workers are taken from the pool of processes established by - . Note that the requested - number of workers may not actually be available at run time. If this + , limited by + . Note that the requested + number of workers may not actually be available at runtime. If this occurs, the plan will run with fewer workers than expected, which may be inefficient. The default value is 2. Setting this value to 0 disables parallel query execution. @@ -2036,6 +2037,21 @@ include_dir 'conf.d' + + max_parallel_workers (integer) + +max_parallel_workers configuration parameter + + + + + Sets the maximum number of workers that the system can support for + parallel queries. The default value is 8. Setting this value to 0 + disables parallel query execution. + + + + backend_flush_after (integer) diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index cde0ed3..0177401 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -453,7 +453,8 @@ LaunchParallelWorkers(ParallelContext *pcxt) snprintf(worker.bgw_name, BGW_MAXLEN, "parallel worker for PID %d", MyProcPid); worker.bgw_flags = - BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION; + BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION + | BGWORKER_CLASS_PARALLEL; worker.bgw_start_time = BgWorkerStart_ConsistentState; worker.bgw_restart_time = BGW_NEVER_RESTART; worker.bgw_main = ParallelWorkerMain; diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index e42ef98..6ad8fd0 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -718,9 +718,11 @@ create_plain_partial_paths(PlannerInfo *root, RelOptInfo *rel) } /* - * In no case use more than max_parallel_workers_per_gather workers. + * In no case use more than max_parallel_workers or + * max_parallel_workers_per_gather workers. */ - parallel_workers = Min(parallel_workers, max_parallel_workers_per_gather); + parallel_workers = Min(max_parallel_workers, Min(parallel_workers, +max_parallel_workers_per_gather)); /* If any limit was set to zero, the user doesn't want a parallel scan. */ if (parallel_workers <= 0) diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c index 2a49639..09dc077 100644 --- a/src/backend/optimizer/path/costsize.c +++ b/src/backend/optimizer/path/costsize.c @@ -113,6 +113,7 @@ int effective_cache_size = DEFAULT_EFFECTIVE_CACHE_SIZE; Cost disable_cost = 1.0e10; +int max_parallel_workers = 8; int max_parallel_workers_per_gather = 2; bool enable_seqscan = true; diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index f657ffc..aa8670b 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -249,6 +249,7 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams) parse->
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE
On 30/09/2016 21:12, David Fetter wrote: > On Fri, Sep 30, 2016 at 06:37:17PM +0200, Julien Rouhaud wrote: >> On 30/09/2016 05:23, Thomas Munro wrote: >>> >>> It would be really nice to be able to set this to 'Ready for >>> Committer' in this CF. Do you want to post a v6 patch or are you >>> happy for me to ask a committer to look at v5 + these three >>> corrections? >> >> I just looked at the patch, and noticed that only plain DELETE and >> UPDATE commands are handled. Is it intended that writable CTE without >> WHERE clauses are not detected by this extension? I personally think >> that wCTE should be handled (everyone can forget a WHERE clause), but if >> not it should at least be documented. > > You are correct in that it should work for every unqualified UPDATE or > DELETE, not just some. Would you be so kind as to send along the > tests cases you used so I can add them to the patch? > Given your test case, these queries should fail if the related require_where GUCs are set (obviously last one should failed with either of the GUC set): WITH d AS (DELETE FROM test_require_where) SELECT 1; WITH u AS (UPDATE test_require_where SET t = t) SELECT 1; WITH d AS (DELETE FROM test_require_where), u AS (UPDATE test_require_where SET t = t) SELECT 1; -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE
On 30/09/2016 05:23, Thomas Munro wrote: > > It would be really nice to be able to set this to 'Ready for > Committer' in this CF. Do you want to post a v6 patch or are you > happy for me to ask a committer to look at v5 + these three > corrections? I just looked at the patch, and noticed that only plain DELETE and UPDATE commands are handled. Is it intended that writable CTE without WHERE clauses are not detected by this extension? I personally think that wCTE should be handled (everyone can forget a WHERE clause), but if not it should at least be documented. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Rename max_parallel_degree?
On 23/09/2016 21:10, Robert Haas wrote: > On Fri, Sep 23, 2016 at 8:54 AM, Peter Eisentraut > wrote: >> On 9/20/16 4:07 PM, Robert Haas wrote: >>> No, I'm assuming that the classes would be built-in. A string tag >>> seems like over-engineering to me, particularly because the postmaster >>> needs to switch on the tag, and we need to be very careful about the >>> degree to which the postmaster trusts the contents of shared memory. >> >> I'm hoping that we can come up with something that extensions can >> participate in, without the core having to know ahead of time what those >> extensions are or how they would be categorized. >> >> My vision is something like >> >> max_processes = 512 # requires restart >> >> process_allowances = 'connection:300 superuser:10 autovacuum:10 >> parallel:30 replication:10 someextension:20 someotherextension:20' >> # does not require restart > > I don't think it's going to be very practical to allow extensions to > participate in the mechanism because there have to be a finite number > of slots that is known at the time we create the main shared memory > segment. > > Also, it's really important that we don't add lots more surface area > for the postmaster to crash and burn. > It seems that there's no objection on Robert's initial proposal, so I'll try to implement it. I've already fixed every other issues mentioned upthread, but I'm facing a problem for this one. Assuming that the bgworker classes are supposed to be mutually exclusive, I don't see a simple and clean way to add such a check in SanityCheckBackgroundWorker(). Am I missing something obvious, or can someone give me some advice for this? -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issue with bgworker, SPI and pgstat_report_stat
On 28/09/2016 18:46, Robert Haas wrote: > > Everybody seems happy with this fix for a first step, so I've > committed it and back-patched it to 9.3. > Thanks! -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Rename max_parallel_degree?
On 16/09/2016 20:24, Robert Haas wrote:
> On Wed, Jun 29, 2016 at 10:46 PM, Amit Kapila wrote:
>> Your patch looks good to me and is ready for a committer's look.
>>
>> Notes for committer -
>> a. Verify if description of newly added Guc max_parallel_workers looks
>> okay to you, me and Julien are not in 100% agreement on that.
>> b. Comments might need some improvement.
>
> This patch needs to be rebased. I hope somebody can volunteer to do
> that, because I'd like to commit it once we've hashed out the details.
>
I just rebased the previous patch on current HEAD, with some other
modifications, see below (attached v8 if that helps).
> Would it bother anybody very much if we bumped up these values, say by
> increasing max_worker_processes from 8 to 16 and max_parallel_workers
> from 4 (as it is in the current patch version) to 8? I feel like 4 is
> a bit more conservative than I'd like to be by default, and I'd like
> to make sure that we leave room for other sorts of background workers
> between the two limits.
>
That's fine by me. Should this be done (if there's no objection) in the
same patch, or in another one?
> I'd suggest renaming the "parallel" flag to BackgroundWorkerSlot to
> "is_parallel_worker". Or, actually, what I think would be better is
> to give it a name like worker_class, and then we can have
> BGWORKER_CLASS_PARALLEL and perhaps eventually
> BGWORKER_CLASS_REPLICATION, etc.
>
For now I just renamed "parallel" to "is_parallel_worker", since this is
straightforward. For a new "worker_class", I guess we'd need a new enum
stored in BackgroundWorker struct instead of the
BGWORKER_IS_PARALLEL_WORKER flag, and store it in the
BackgroundWorkerSlot. Should I do that instead?
> + * terminated ones. These counters can of course overlaps, but it's not
> + * important here since the substraction will still give the right number.
>
> overlaps -> overflow. substraction -> subtraction.
>
oops sorry, fixed
> + /*
> +* We need a write barrier to make sure the update of
> +* parallel_terminate_count is done before the store to in_use
> +*/
>
> Does the order actually matter here?
>
After some more thinking, it looks like a reorder here won't have any
impact. I'll remove it, unless Amit has an objection about it.
> + {"max_parallel_workers", PGC_USERSET, RESOURCES_ASYNCHRONOUS,
> + gettext_noop("Sets the maximum number of
> parallel processes for the cluster."),
>
> I suggest: sets the maximum number of parallel workers that can be
> active at one time.
>
changed
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index cd66abc..3abd2e5 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2005,8 +2005,9 @@ include_dir 'conf.d'
Sets the maximum number of workers that can be started by a single
Gather node. Parallel workers are taken from the
pool of processes established by
- . Note that the requested
- number of workers may not actually be available at run time. If this
+ , limited by
+ . Note that the requested
+ number of workers may not actually be available at runtime. If this
occurs, the plan will run with fewer workers than expected, which may
be inefficient. The default value is 2. Setting this value to 0
disables parallel query execution.
@@ -2030,6 +2031,21 @@ include_dir 'conf.d'
+
+ max_parallel_workers (integer)
+
+max_parallel_workers configuration
parameter
+
+
+
+
+ Sets the maximum number of workers that the system can support for
+ parallel queries. The default value is 4. Setting this value to 0
+ disables parallel query execution.
+
+
+
+
backend_flush_after (integer)
diff --git a/src/backend/access/transam/parallel.c
b/src/backend/access/transam/parallel.c
index cde0ed3..5f6e429 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -453,7 +453,8 @@ LaunchParallelWorkers(ParallelContext *pcxt)
snprintf(worker.bgw_name, BGW_MAXLEN, "parallel worker for PID %d",
MyProcPid);
worker.bgw_flags =
- BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION;
+ BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION
+ | BGWORKER_IS_PARALLEL_WORKER;
worker.bgw_start_time = BgWorkerStart_ConsistentState;
Re: [HACKERS] [PATCH] SortSupport for macaddr type
On 26/08/2016 19:44, Brandur wrote: > Hello, > Hello, > I've attached a patch to add SortSupport for Postgres' macaddr which has the > effect of improving the performance of sorting operations for the type. The > strategy that I employ is very similar to that for UUID, which is to create > abbreviated keys by packing as many bytes from the MAC address as possible > into > Datums, and then performing fast unsigned integer comparisons while sorting. > > I ran some informal local benchmarks, and for cardinality greater than 100k > rows, I see a speed up on `CREATE INDEX` of roughly 25% to 65%. (For those > interested, I put a few more numbers into a small report here [2].) > That's a nice improvement! > Admittedly, this is not quite as useful as speeding up sorting on a more > common > data type like TEXT or UUID, but the change still seems like a useful > performance improvement. I largely wrote it as an exercise to familiarize > myself with the Postgres codebase. > > I'll add an entry into the current commitfest as suggested by the Postgres > Wiki > and follow up here with a link. > > Thanks, and if anyone has feedback or other thoughts, let me know! > I just reviewed your patch. It applies and compiles cleanly, and the abbrev feature works as intended. There's not much to say since this is heavily inspired on the uuid SortSupport. The only really specific part is in the abbrev_converter function, and I don't see any issue with it. I have a few trivial comments: * you used macaddr_cmp_internal() function name, for uuid the same function is named uuid_internal_cmp(). Using the same naming pattern is probably better. * the function comment on macaddr_abbrev_convert() doesn't mention specific little-endian handling * "There will be two bytes of zero padding on the least significant end" "least significant bits" would be better * This patch will trigger quite a lot modifications after a pgindent run. Could you try to run pgindent on mac.c before sending an updated patch? Best regards. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Assert(LWLockHeldByMeInMode(lock, LW_EXCLUSIVE))
On 05/09/2016 11:55, Julien Rouhaud wrote: > On 20/06/2016 06:28, Thomas Munro wrote: >> On Mon, Jun 20, 2016 at 3:43 PM, Craig Ringer wrote: >>> On 18 June 2016 at 11:28, Thomas Munro >>> wrote: >>>> Several times now when reading, debugging and writing code I've wished >>>> that LWLockHeldByMe assertions specified the expected mode, especially >>>> where exclusive locking is required. >>>> >>>> What do you think about something like the attached? See also an >>>> example of use. I will add this to the next commitfest. >>> >>> I've wanted this before too [...] >> > > same here. > >> Before ab5194e6f (25 December 2014) held_lwlocks didn't record the mode. >> > > I just reviewed both patches. They applies cleanly on current HEAD, > work as intended and make check run smoothly. Patches are pretty > straightforward, so I don't have much to say. > > My only remark is on following comment: > > + * LWLockHeldByMeInMode - test whether my process holds a lock in mode X > > Maybe something like "test whether my process holds a lock in given > mode" would be better? > > Otherwise, I think they're ready for committer. > Didn't saw that Simon just committed it, sorry about it. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Assert(LWLockHeldByMeInMode(lock, LW_EXCLUSIVE))
On 20/06/2016 06:28, Thomas Munro wrote: > On Mon, Jun 20, 2016 at 3:43 PM, Craig Ringer wrote: >> On 18 June 2016 at 11:28, Thomas Munro >> wrote: >>> Several times now when reading, debugging and writing code I've wished >>> that LWLockHeldByMe assertions specified the expected mode, especially >>> where exclusive locking is required. >>> >>> What do you think about something like the attached? See also an >>> example of use. I will add this to the next commitfest. >> >> I've wanted this before too [...] > same here. > Before ab5194e6f (25 December 2014) held_lwlocks didn't record the mode. > I just reviewed both patches. They applies cleanly on current HEAD, work as intended and make check run smoothly. Patches are pretty straightforward, so I don't have much to say. My only remark is on following comment: + * LWLockHeldByMeInMode - test whether my process holds a lock in mode X Maybe something like "test whether my process holds a lock in given mode" would be better? Otherwise, I think they're ready for committer. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On 20/08/2016 15:41, Michael Paquier wrote: > On Sat, Aug 20, 2016 at 10:44 AM, Petr Jelinek wrote: >> If we want to specifically name the recovery_target_lsn in the message, we >> could probably do it using context. > > So that would be basically assigning error_context_stack for each item > parsed for recovery.conf? That seems a bit narrow as usually > recovery.conf is not that long.. > That'd still be an improvement, since the error message doesn't even mention that the error comes from recovery.conf. I agree it can't come from many places, but it may be useful for some people. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Small issues in syncrep.c
On 10/08/2016 09:43, Michael Paquier wrote: > On Wed, Aug 10, 2016 at 4:29 PM, Simon Riggs wrote: >> >> I've updated Julien's patch to reflect Michael's suggestion. Thanks to you and Michael. >> 14e8803f1 was only a partial patch for the syncrep code, so I don't >> see any reason to keep the code as it currently is in 9.5/9.6. >> >> Any objections to backpatching this to 9.5 and 9.6? > > None from here. > same here. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Small issues in syncrep.c
Hello, Since 14e8803f1, it's not necessary to acquire the SyncRepLock to see up to date data. But it looks like this commit didn't update all the comment around MyProc->syncRepState, which still mention retrieving the value without and without lock. Also, there's I think a now unneeded test to try to retrieve again syncRepState. Patch attached to fix both small issues, present since 9.5. -- Julien Rouhaud http://dalibo.com - http://dalibo.org diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c index 67249d8..c3e11b8 100644 --- a/src/backend/replication/syncrep.c +++ b/src/backend/replication/syncrep.c @@ -195,17 +195,12 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit) ResetLatch(MyLatch); /* -* Try checking the state without the lock first. There's no -* guarantee that we'll read the most up-to-date value, so if it looks -* like we're still waiting, recheck while holding the lock. But if -* it looks like we're done, we must really be done, because once -* walsender changes the state to SYNC_REP_WAIT_COMPLETE, it will -* never update it again, so we can't be seeing a stale value in that -* case. +* Acquiring the lock is not needed, the latch ensure proper barriers. +* If it looks like we're done, we must really be done, because once +* walsender changes the state to SYNC_REP_WAIT_COMPLETE, it will never +* update it again, so we can't be seeing a stale value in that case. */ syncRepState = MyProc->syncRepState; - if (syncRepState == SYNC_REP_WAITING) - syncRepState = MyProc->syncRepState; if (syncRepState == SYNC_REP_WAIT_COMPLETE) break; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Notice lock waits
On 05/08/2016 19:00, Jeff Janes wrote: > One time too many, I ran some minor change using psql on a production > server and was wondering why it was taking so much longer than it did > on the test server. Only to discover, after messing around with > opening new windows and running queries against pg_stat_activity and > pg_locks and so on, that it was waiting for a lock. > > So I created a new guc, notice_lock_waits, which acts like > log_lock_waits but sends the message as NOTICE so it will show up on > interactive connections like psql. > > I turn it on in my .psqlrc, as it doesn't make much sense for me to > turn it on in non-interactive sessions. > > A general facility for promoting selected LOG messages to NOTICE would > be nice, but I don't know how to design or implement that. This is > much easier, and I find it quite useful. > > I have it PGC_SUSET because it does send some tiny amount of > information about the blocking process (the PID) to the blocked > process. That is probably too paranoid, because the PID can be seen > by anyone in the pg_locks table anyway. > > Do you think this is useful and generally implemented in the correct > way? If so, I'll try to write some sgml documentation for it. > I really like the idea. I'm not really sure on current implementation. Unless I'm wrong, disabling log_lock_waits would also disable notice_lock_waits, even if it's on. Maybe a new value for log_lock_waits, like "interactive". If switching this GUC from bool to enum is not acceptable or allowing to see blocking PID for anyone is an issue, then maybe adding a new GUC to say to also send a NOTICE instead? -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PROPOSAL] timestamp informations to pg_stat_statements
On 18/07/2016 01:06, Peter Geoghegan wrote: > On Sun, Jul 17, 2016 at 12:22 AM, Jun Cheol Gim wrote: >> If we have timestamp of first and last executed, we can easily gather thess >> informations and there are tons of more use cases. > > -1 from me. > > I think that this is the job of a tool that aggregates things from > pg_stat_statements. It's unfortunate that there isn't a good > third-party tool that does that, but there is nothing that prevents > it. > FWIW there's https://github.com/dalibo/powa-archivist which does that, assuming you're using a 9.4+ server. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Issue with bgworker, SPI and pgstat_report_stat
On 08/07/2016 01:53, Michael Paquier wrote: > On Fri, Jul 8, 2016 at 3:06 AM, Andres Freund wrote: >> On 2016-07-07 14:04:36 -0400, Robert Haas wrote: >>> On Thu, Jul 7, 2016 at 1:52 PM, Julien Rouhaud >>> wrote: >>>> Should a bgworker modifing data have to call pgstat_report_stat() to >>>> avoid this problem? I don't find any documentation suggesting it, and it >>>> seems that worker_spi (used as a template for this bgworker and I >>>> suppose a lot of other one) is also affected. >>> >>> That certainly seems like the simplest fix. Not sure if there's a better >>> one. >> >> I think a better fix would be to unify the startup & error handling >> code. We have way to many slightly diverging copies. But that's a bigger >> task, so I'm not protesting against making a more localized fix. > > It seems to me that the only fix is to have the bgworker call > pgstat_report_stat() and not mess up with the in-core backend code. > Personally, I always had the image of a bgworker to be an independent > process, so when it wants to do something, be it mimicking normal > backends, it should do it by itself. Take the example of SIGHUP > processing. If the bgworker does not ProcessConfigFile() no parameters > updates will happen in the context of the bgworker. > I'm not opposed, but in this case we should also provide a proper documentation of all the required actions to mimick normal backends. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PSA: Systemd will kill PostgreSQL
On 10/07/2016 19:56, Joshua D. Drake wrote: > Hackers, > > This just came across my twitter feed: > > https://lists.freedesktop.org/archives/systemd-devel/2014-April/018373.html > > tl;dr; Systemd 212 defaults to remove all IPC (including SYSV memory) > when a user "fully" logs out. > AFAIK it's only the case if the user is not a system user, and postgres user should be (at least with community packages). See https://github.com/systemd/systemd/issues/2039 -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Issue with bgworker, SPI and pgstat_report_stat
Hello, While investigating on a bloat issue with a colleague, we found that if a bgworker executes some queries with SPI, the statistic changes will never be reported, since pgstat_report_stat() is only called in regular backends. In our case, the bgworker is the only process inserting and deleting a large amount of data on some tables, so the autovacuum never tried to do any maintenance on these tables. Should a bgworker modifing data have to call pgstat_report_stat() to avoid this problem? I don't find any documentation suggesting it, and it seems that worker_spi (used as a template for this bgworker and I suppose a lot of other one) is also affected. If yes, I think at least worker_spi should be fixed (patched attached). Regards. -- Julien Rouhaud http://dalibo.com - http://dalibo.org diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c index 314e371..7c9a3eb 100644 --- a/src/test/modules/worker_spi/worker_spi.c +++ b/src/test/modules/worker_spi/worker_spi.c @@ -292,6 +292,7 @@ worker_spi_main(Datum main_arg) SPI_finish(); PopActiveSnapshot(); CommitTransactionCommand(); + pgstat_report_stat(false); pgstat_report_activity(STATE_IDLE, NULL); } -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Rename max_parallel_degree?
On 29/06/2016 08:51, Amit Kapila wrote: > On Wed, Jun 29, 2016 at 11:54 AM, Julien Rouhaud > wrote: >> Or should we allow setting it to -1 for instance to disable the limit? >> > > By disabling the limit, do you mean to say that only > max_parallel_workers_per_gather will determine the workers required or > something else? I meant what the current behavior (before this patch) is, which is probably what some user without custom dynamic bgworker may like to have. Otherwise, you'd have to change two parameters to effectively increase parallelism, and I think it can also cause some confusion. > If earlier, then I am not sure if it is good idea, > because it can cause some confusion to the user about usage of both > the parameters together. > I also agree. I don't see an ideal solution, so just keeping this patch behavior is fine for me. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Rename max_parallel_degree?
On 29/06/2016 06:29, Amit Kapila wrote:
> On Wed, Jun 29, 2016 at 2:57 AM, Julien Rouhaud
> wrote:
>>
>> Thanks a lot for the help!
>>
>> PFA v6 which should fix all the issues mentioned.
>
> Couple of minor suggestions.
>
> - . Note that the requested
> + , limited by
> + . Note that the requested
>
> Typo.
> /linked/linkend
>
Oops, fixed.
> You can always find such mistakes by doing make check in doc/src/sgml/
>
I wasn't aware of that, it's really a nice thing to know, thanks!
> + /*
> + * We need a memory barrier here to make sure the above test doesn't get
> + * reordered
> + */
> + pg_read_barrier();
>
> /memory barrier/read barrier
>
fixed
> + if (max_parallel_workers == 0)
> + {
> + ereport(elevel,
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("background worker \"%s\": cannot request parallel worker if
> no parallel worker allowed",
>
> " ..no parallel worker is allowed". 'is' seems to be missing.
>
fixed
>
>> Also, after second
>> thought I didn't add the extra hint about max_worker_processes in the
>> max_parallel_worker paragraph, since this line was a duplicate of the
>> precedent paragraph, it seemed better to leave the text as is.
>>
>
> not a big problem, we can leave it for committer to decide on same.
> However just by reading the description of max_parallel_worker, user
> can set its value more than max_wroker_processes which we don't want.
>
Right. On the other hand I'm not sure that's really an issue, because
such a case is handled in the code, and setting max_parallel_workers way
above max_worker_processes could be a way to configure it as unlimited.
Or should we allow setting it to -1 for instance to disable the limit?
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 061697b..3a47421 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2005,7 +2005,8 @@ include_dir 'conf.d'
Sets the maximum number of workers that can be started by a single
Gather node. Parallel workers are taken from the
pool of processes established by
- . Note that the requested
+ , limited by
+ . Note that the requested
number of workers may not actually be available at runtime. If this
occurs, the plan will run with fewer workers than expected, which may
be inefficient. The default value is 2. Setting this value to 0
@@ -2014,6 +2015,21 @@ include_dir 'conf.d'
+
+ max_parallel_workers (integer)
+
+max_parallel_workers configuration
parameter
+
+
+
+
+ Sets the maximum number of workers that the system can support for
+ parallel queries. The default value is 4. Setting this value to 0
+ disables parallel query execution.
+
+
+
+
backend_flush_after (integer)
diff --git a/src/backend/access/transam/parallel.c
b/src/backend/access/transam/parallel.c
index 088700e..ea7680b 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -452,7 +452,8 @@ LaunchParallelWorkers(ParallelContext *pcxt)
snprintf(worker.bgw_name, BGW_MAXLEN, "parallel worker for PID %d",
MyProcPid);
worker.bgw_flags =
- BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION;
+ BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION
+ | BGWORKER_IS_PARALLEL_WORKER;
worker.bgw_start_time = BgWorkerStart_ConsistentState;
worker.bgw_restart_time = BGW_NEVER_RESTART;
worker.bgw_main = ParallelWorkerMain;
diff --git a/src/backend/optimizer/path/allpaths.c
b/src/backend/optimizer/path/allpaths.c
index 2e4b670..e1da5f9 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -724,9 +724,11 @@ create_plain_partial_paths(PlannerInfo *root, RelOptInfo
*rel)
}
/*
-* In no case use more than max_parallel_workers_per_gather workers.
+* In no case use more than max_parallel_workers or
+* max_parallel_workers_per_gather workers.
*/
- parallel_workers = Min(parallel_workers,
max_parallel_workers_per_gather);
+ parallel_workers = Min(max_parallel_workers, Min(parallel_workers,
+ max_parallel_workers_per_gather));
/* If any limit was set to zero, the user doesn't want a parallel scan.
*/
if (parallel_workers <= 0)
diff --git a/src/backend/optimizer/path/costsize.c
b/src/backend/optimiz
Re: [HACKERS] Rename max_parallel_degree?
On 28/06/2016 04:44, Amit Kapila wrote:
> On Mon, Jun 27, 2016 at 10:35 PM, Julien Rouhaud
>>
>> There's already a pg_memory_barrier() call in
>> BackgroundWorkerStateChange(), to avoid reordering the notify_pid load.
>> Couldn't we use it to also make sure the parallel_terminate_count
>> increment happens before the slot->in_use store?
>>
>
> Yes, that is enough, as memory barrier ensures that both loads and
> stores are completed before any loads and stores that are after
> barrier.
>
>> I guess that a write
>> barrier will be needed in ForgetBacgroundWorker().
>>
>
> Yes.
>
>>>> 2.
>>>> + if (parallel && (BackgroundWorkerData->parallel_register_count -
>>>> +
>>>> BackgroundWorkerData->parallel_terminate_count) >=
>>>> +
>>>> max_parallel_workers)
>>>> + {
>>>> + LWLockRelease(BackgroundWorkerLock);
>>>> + return
>>>> false;
>>>> + }
>>>> +
>>>>
>>>> I think we need a read barrier here, so that this check doesn't get
>>>> reordered with the for loop below it.
>>
>> You mean between the end of this block and the for loop?
>>
>
> Yes.
>
>>>> Also, see if you find the code
>>>> more readable by moving the after && part of check to next line.
>>
>> I think I'll just pgindent the file.
>>
>
> make sense.
>
>
Thanks a lot for the help!
PFA v6 which should fix all the issues mentioned. Also, after second
thought I didn't add the extra hint about max_worker_processes in the
max_parallel_worker paragraph, since this line was a duplicate of the
precedent paragraph, it seemed better to leave the text as is.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index a82bf06..6812b0d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2009,7 +2009,8 @@ include_dir 'conf.d'
Sets the maximum number of workers that can be started by a single
Gather node. Parallel workers are taken from the
pool of processes established by
- . Note that the requested
+ , limited by
+ . Note that the requested
number of workers may not actually be available at runtime. If this
occurs, the plan will run with fewer workers than expected, which may
be inefficient. The default value is 2. Setting this value to 0
@@ -2018,6 +2019,21 @@ include_dir 'conf.d'
+
+ max_parallel_workers (integer)
+
+max_parallel_workers configuration
parameter
+
+
+
+
+ Sets the maximum number of workers that the system can support for
+ parallel queries. The default value is 4. Setting this value to 0
+ disables parallel query execution.
+
+
+
+
backend_flush_after (integer)
diff --git a/src/backend/access/transam/parallel.c
b/src/backend/access/transam/parallel.c
index 088700e..ea7680b 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -452,7 +452,8 @@ LaunchParallelWorkers(ParallelContext *pcxt)
snprintf(worker.bgw_name, BGW_MAXLEN, "parallel worker for PID %d",
MyProcPid);
worker.bgw_flags =
- BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION;
+ BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION
+ | BGWORKER_IS_PARALLEL_WORKER;
worker.bgw_start_time = BgWorkerStart_ConsistentState;
worker.bgw_restart_time = BGW_NEVER_RESTART;
worker.bgw_main = ParallelWorkerMain;
diff --git a/src/backend/optimizer/path/allpaths.c
b/src/backend/optimizer/path/allpaths.c
index 2e4b670..e1da5f9 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -724,9 +724,11 @@ create_plain_partial_paths(PlannerInfo *root, RelOptInfo
*rel)
}
/*
-* In no case use more than max_parallel_workers_per_gather workers.
+* In no case use more than max_parallel_workers or
+* max_parallel_workers_per_gather workers.
*/
- parallel_workers = Min(parallel_workers,
max_parallel_workers_per_gather);
+ parallel_workers = Min(max_parallel_workers, Min(parallel_workers,
+ max_parallel_workers_per_gather));
/* If any limit was set to zero, the user doesn't want a parallel scan.
*/
if (parallel_workers <= 0)
diff --git a/src/backend/optimizer/path/costsize.c
b/src/backend/optimizer/path/costsize.c
index 8c1dccc..6cb2f4e
Re: [HACKERS] Rename max_parallel_degree?
On 27/06/2016 07:18, Amit Kapila wrote:
> On Mon, Jun 27, 2016 at 10:21 AM, Amit Kapila wrote:
>>
>> I think as the parallel_terminate_count is only modified by postmaster
>> and read by other processes, such an operation will be considered
>> atomic. We don't need to specifically make it atomic unless multiple
>> processes are allowed to modify such a counter. Although, I think we
>> need to use some barriers in code to make it safe.
>>
>> 1.
>> @@ -272,6 +279,8 @@ BackgroundWorkerStateChange(void)
>> pg_memory_barrier();
>>
>> slot->pid = 0;
>> slot->in_use = false;
>> + if (slot->parallel)
>> +
>> BackgroundWorkerData->parallel_terminate_count++;
>>
>> I think the check of slot->parallel and increment to
>> parallel_terminate_count should be done before marking slot->in_use to
>> false. Consider the case if slot->in_use is marked as false and then
>> before next line execution, one of the backends registers dynamic
>> worker (non-parallel worker), so that
>> backend can see this slot as free and use it. It will also mark the
>> parallel flag of slot as false. Now when postmaster will check the
>> slot->parallel flag, it will find it false and then skip the increment
>> to parallel_terminate_count.
>>
>
> If you agree with above theory, then you need to use write barrier as
> well after incrementing the parallel_terminate_count to avoid
> re-ordering with slot->in_use flag.
>
I totally agree, I didn't thought about this corner case.
There's already a pg_memory_barrier() call in
BackgroundWorkerStateChange(), to avoid reordering the notify_pid load.
Couldn't we use it to also make sure the parallel_terminate_count
increment happens before the slot->in_use store? I guess that a write
barrier will be needed in ForgetBacgroundWorker().
>> 2.
>> + if (parallel && (BackgroundWorkerData->parallel_register_count -
>> +
>> BackgroundWorkerData->parallel_terminate_count) >=
>> +
>> max_parallel_workers)
>> + {
>> + LWLockRelease(BackgroundWorkerLock);
>> + return
>> false;
>> + }
>>+
>>
>> I think we need a read barrier here, so that this check doesn't get
>> reordered with the for loop below it.
You mean between the end of this block and the for loop? (Sorry, I'm not
at all familiar with the pg_barrier mechanism).
>> Also, see if you find the code
>> more readable by moving the after && part of check to next line.
I think I'll just pgindent the file.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Rename max_parallel_degree?
On 26/06/2016 08:37, Amit Kapila wrote: > On Sat, Jun 25, 2016 at 2:27 PM, Julien Rouhaud > wrote: >>> >> >> It's better thanks. Should we document somewhere the link between this >> parameter and custom dynamic background workers or is it pretty >> self-explanatory? >> > > How about if add an additiona line like: > Parallel workers are taken from the pool of processes established by > guc-max-worker-processes. > > I think one might feel some duplication of text between this and what > we have for max_parallel_workers_per_gather, but it seems genuine to > me. > Ok, I'll add it. > > @@ -370,6 +379,8 @@ ForgetBackgroundWorker(slist_mutable_iter *cur) > Assert(rw->rw_shmem_slot < > max_worker_processes); > slot = &BackgroundWorkerData->slot[rw->rw_shmem_slot]; > slot->in_use = > false; > + if (slot->parallel) > + BackgroundWorkerData->parallel_terminate_count++; > > I think operations on parallel_terminate_count are not safe. > ForgetBackgroundWorker() and RegisterDynamicBackgroundWorker() can try > to read write at same time. It seems you need to use atomic > operations to ensure safety. > > But operations on parallel_terminate_count are done by the postmaster, and per Robert's previous email postmaster can't use atomics: > We can't have the postmaster and the > backends share the same counter, because then it would need locking, > and the postmaster can't try to take spinlocks - can't even use > atomics, because those might be emulated using spinlocks. Do we support platforms on which 32bits operations are not atomic? -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Rename max_parallel_degree?
On 25/06/2016 09:33, Amit Kapila wrote:
> On Wed, Jun 15, 2016 at 11:43 PM, Julien Rouhaud
> wrote:
>>
>> Attached v4 implements the design you suggested, I hope everything's ok.
>>
>
> Few review comments:
>
Thanks for the review.
> 1.
> + if (parallel && (BackgroundWorkerData->parallel_register_count -
> +
> BackgroundWorkerData->parallel_terminate_count) >=
> +
> max_parallel_workers)
> + return false;
>
> I think this check should be done after acquiring
> BackgroundWorkerLock, otherwise some other backend can simultaneously
> increment parallel_register_count.
>
You're right, fixed.
> 2.
>
> +/*
> + * This flag is used internally for parallel queries, to keep track of the
> + * number of active
> parallel workers and make sure we never launch more than
> + * max_parallel_workers parallel workers at
> the same time. Third part
> + * background workers should not use this flag.
> + */
> +#define
> BGWORKER_IS_PARALLEL_WORKER 0x0004
> +
>
> "Third part", do yo want to say Third party?
>
Yes, sorry. Fixed
> 3.
> static bool
> SanityCheckBackgroundWorker(BackgroundWorker *worker, int elevel)
> {
> ..
> }
>
> Isn't it better to have a check in above function such that if
> bgw_flags is BGWORKER_IS_PARALLEL_WORKER and max_parallel_workers is
> zero, then ereport? Also, consider if it is better to have some other
> checks related to BGWORKER_IS_PARALLEL_WORKER, like if caller sets
> BGWORKER_IS_PARALLEL_WORKER, then it must have database connection and
> shared memory access.
>
I added these checks. I don't see another check to add right now.
> 4.
> + xreflabel="max_parallel_workers">
> + max_parallel_workers (integer)
> +
> +max_parallel_workers configuration
> parameter
> +
> +
> +
> +
> + Sets the maximum number of workers that can be launched at the same
> + time for the whole server. This parameter allows the administrator
> to
> + reserve background worker slots for for third part dynamic
> background
> + workers. The default value is 4. Setting this value to 0 disables
> + parallel query execution.
> +
> +
> +
>
> How about phrasing it as:
> Sets the maximum number of workers that the system can support for
> parallel queries. The default value is 4. Setting this value to 0
> disables parallel query execution.
>
It's better thanks. Should we document somewhere the link between this
parameter and custom dynamic background workers or is it pretty
self-explanatory?
> 5.
> max_parallel_workers_per_gather configuration
> parameter
>
>
>
>
> Sets the maximum number of workers that can be started by a single
> Gather node. Parallel workers are taken from the
> pool of processes established by
> .
>
> I think it is better to change above in documentation to indicate that
> "pool of processes established by guc-max-parallel-workers".
>
The real limit is the minimum of these two values, I think it's
important to be explicit here, since this pool is shared for parallelism
and custom bgworkers.
What about "pool of processes established by guc-max-worker-processes,
limited by guc-max-parallel-workers" (used in attached v5 patch)
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index a82bf06..6812b0d 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2009,7 +2009,8 @@ include_dir 'conf.d'
Sets the maximum number of workers that can be started by a single
Gather node. Parallel workers are taken from the
pool of processes established by
- . Note that the requested
+ , limited by
+ . Note that the requested
number of workers may not actually be available at runtime. If this
occurs, the plan will run with fewer workers than expected, which may
be inefficient. The default value is 2. Setting this value to 0
@@ -2018,6 +2019,21 @@ include_dir 'conf.d'
+
+ max_parallel_workers (integer)
+
+max_parallel_workers configuration
parameter
+
+
+
+
+ Sets the maximum number of workers that the system can support for
+ parallel queries. The default value is 4. Setting this value to 0
+ disables parallel query execution.
+
+
+
+
backend_flush_after (integer)
diff --git a/src/bac
Re: [HACKERS] Rename max_parallel_degree?
On 15/06/2016 18:14, Julien Rouhaud wrote: > On 15/06/2016 17:49, Robert Haas wrote: >> On Tue, Jun 14, 2016 at 7:10 AM, Julien Rouhaud >> wrote: >>>> I don't entirely like the new logic in >>>> RegisterDynamicBackgroundWorker. >>> >>>> I wonder if we can't drive this off >>>> of a couple of counters, instead of having the process registering the >>>> background worker iterate over every slot. [...] >>>> >> >> I think we should go that way. Some day we might try to make the >> process of finding a free slot more efficient than it is today; I'd >> rather not double down on linear search. >> > > Ok. > >> Are you going to update this patch? >> > > Sure, I'll post a new patch ASAP. > Attached v4 implements the design you suggested, I hope everything's ok. I didn't do anything for the statistics part, because I'm not sure having the counters separately is useful (instead of just having the current number of parallel workers), and if it's useful I'd find strange to have these counters get reset at restart instead of being stored and accumulated as other stats, and that's look too much for 9.6 material. I'd be happy to work on this though. -- Julien Rouhaud http://dalibo.com - http://dalibo.org diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index e0e5a1e..c661c7a 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -2018,6 +2018,23 @@ include_dir 'conf.d' + + max_parallel_workers (integer) + +max_parallel_workers configuration parameter + + + + + Sets the maximum number of workers that can be launched at the same + time for the whole server. This parameter allows the administrator to + reserve background worker slots for for third part dynamic background + workers. The default value is 4. Setting this value to 0 disables + parallel query execution. + + + + backend_flush_after (integer) diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index ab5ef25..b429474 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -453,7 +453,8 @@ LaunchParallelWorkers(ParallelContext *pcxt) snprintf(worker.bgw_name, BGW_MAXLEN, "parallel worker for PID %d", MyProcPid); worker.bgw_flags = - BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION; + BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION + | BGWORKER_IS_PARALLEL_WORKER; worker.bgw_start_time = BgWorkerStart_ConsistentState; worker.bgw_restart_time = BGW_NEVER_RESTART; worker.bgw_main = ParallelWorkerMain; diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index cc8ba61..2bcd86b 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -719,9 +719,11 @@ create_plain_partial_paths(PlannerInfo *root, RelOptInfo *rel) } /* -* In no case use more than max_parallel_workers_per_gather workers. +* In no case use more than max_parallel_workers or +* max_parallel_workers_per_gather workers. */ - parallel_workers = Min(parallel_workers, max_parallel_workers_per_gather); + parallel_workers = Min(max_parallel_workers, Min(parallel_workers, + max_parallel_workers_per_gather)); /* If any limit was set to zero, the user doesn't want a parallel scan. */ if (parallel_workers <= 0) diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c index e7f63f4..fd62126 100644 --- a/src/backend/optimizer/path/costsize.c +++ b/src/backend/optimizer/path/costsize.c @@ -113,6 +113,7 @@ int effective_cache_size = DEFAULT_EFFECTIVE_CACHE_SIZE; Cost disable_cost = 1.0e10; +intmax_parallel_workers = 4; intmax_parallel_workers_per_gather = 2; bool enable_seqscan = true; diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 07b925e..66e65c8 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -246,8 +246,9 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams) IsUnderPostmaster && dynamic_shared_memory_type != DSM_IMPL_NONE && parse->commandType == CMD_SELECT && !parse->hasModifyingCTE && parse->utilityStmt == NULL && max_parallel_workers_per_gathe
Re: [HACKERS] Rename max_parallel_degree?
On 15/06/2016 17:49, Robert Haas wrote: > On Tue, Jun 14, 2016 at 7:10 AM, Julien Rouhaud > wrote: >>> I don't entirely like the new logic in >>> RegisterDynamicBackgroundWorker. >> >>> I wonder if we can't drive this off >>> of a couple of counters, instead of having the process registering the >>> background worker iterate over every slot. [...] >>> > > I think we should go that way. Some day we might try to make the > process of finding a free slot more efficient than it is today; I'd > rather not double down on linear search. > Ok. > Are you going to update this patch? > Sure, I'll post a new patch ASAP. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Rename max_parallel_degree?
On 14/06/2016 04:09, Robert Haas wrote: > On Mon, Jun 13, 2016 at 5:42 AM, Julien Rouhaud > wrote: >> Agreed, and fixed in attached v3. > > I don't entirely like the new logic in > RegisterDynamicBackgroundWorker. I'm not that happy with it too. We can avoid iterating over every slots if the feature isn't activated though (max_parallel_workers >= max_worker_processes). > I wonder if we can't drive this off > of a couple of counters, instead of having the process registering the > background worker iterate over every slot. Suppose we add two > counters to BackgroundWorkerArray, parallel_register_count and > parallel_terminate_count. Whenever a backend successfully registers a > parallel worker, it increments parallel_register_count. Whenever the > postmaster marks a parallel wokrer slot as no longer in use, it > increments parallel_terminate_count. Then, the number of active > parallel workers is just parallel_register_count - > parallel_terminate_count. (We can't have the postmaster and the > backends share the same counter, because then it would need locking, > and the postmaster can't try to take spinlocks - can't even use > atomics, because those might be emulated using spinlocks.) > I wanted to maintain counters at first, but it seemed more invasive, and I thought that the max_parallel_worker would be ueful in environnements where there're lots of parallel workers and dynamic workers used, so finding a free slot would require iterating over most of the slots most of the time anyway. I'm of course also ok with maintaining counters. > If we want to allow the number of parallel workers started to be available > for statistical purposes, we can keep to uint32 values for that > (parallel_register_count_lo and parallel_register_count_hi, for > example), and increment the second one whenever the first one rolls > over to zero. > I didn't think about monitoring. I'm not sure if this counter would be really helpful without also having the number of time a parallel worker couldn't be launched (and I'd really like to have this one). -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Rename max_parallel_degree?
On 13/06/2016 03:16, Robert Haas wrote: > On Sat, Jun 11, 2016 at 6:24 PM, Julien Rouhaud > wrote: >> On 11/06/2016 23:37, Julien Rouhaud wrote: >>> On 09/06/2016 16:04, Robert Haas wrote: >>>> There remains only the task of adding max_parallel_degree >>>> as a system-wide limit >>>> >>> >>> PFA a patch to fix this open item. I used the GUC name provided in the >>> 9.6 open item page (max_parallel_workers), with a default value of 4. >>> Value 0 is another way to disable parallel query. >>> >> >> Sorry I just realized I made a stupid mistake, I didn't check in all >> slots to get the number of active parallel workers. Fixed in attached v2. > > I think instead of adding a "bool parallel" argument to > RegisterDynamicBackgroundWorker, we should just define a new constant > for bgw_flags, say BGW_IS_PARALLEL_WORKER. > Agreed, and fixed in attached v3. -- Julien Rouhaud http://dalibo.com - http://dalibo.org diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index e0e5a1e..c661c7a 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -2018,6 +2018,23 @@ include_dir 'conf.d' + + max_parallel_workers (integer) + +max_parallel_workers configuration parameter + + + + + Sets the maximum number of workers that can be launched at the same + time for the whole server. This parameter allows the administrator to + reserve background worker slots for for third part dynamic background + workers. The default value is 4. Setting this value to 0 disables + parallel query execution. + + + + backend_flush_after (integer) diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index ab5ef25..b429474 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -453,7 +453,8 @@ LaunchParallelWorkers(ParallelContext *pcxt) snprintf(worker.bgw_name, BGW_MAXLEN, "parallel worker for PID %d", MyProcPid); worker.bgw_flags = - BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION; + BGWORKER_SHMEM_ACCESS | BGWORKER_BACKEND_DATABASE_CONNECTION + | BGWORKER_IS_PARALLEL_WORKER; worker.bgw_start_time = BgWorkerStart_ConsistentState; worker.bgw_restart_time = BGW_NEVER_RESTART; worker.bgw_main = ParallelWorkerMain; diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index cc8ba61..2bcd86b 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -719,9 +719,11 @@ create_plain_partial_paths(PlannerInfo *root, RelOptInfo *rel) } /* -* In no case use more than max_parallel_workers_per_gather workers. +* In no case use more than max_parallel_workers or +* max_parallel_workers_per_gather workers. */ - parallel_workers = Min(parallel_workers, max_parallel_workers_per_gather); + parallel_workers = Min(max_parallel_workers, Min(parallel_workers, + max_parallel_workers_per_gather)); /* If any limit was set to zero, the user doesn't want a parallel scan. */ if (parallel_workers <= 0) diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c index e7f63f4..fd62126 100644 --- a/src/backend/optimizer/path/costsize.c +++ b/src/backend/optimizer/path/costsize.c @@ -113,6 +113,7 @@ int effective_cache_size = DEFAULT_EFFECTIVE_CACHE_SIZE; Cost disable_cost = 1.0e10; +intmax_parallel_workers = 4; intmax_parallel_workers_per_gather = 2; bool enable_seqscan = true; diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 54c0440..81d124c 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -246,8 +246,9 @@ standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams) IsUnderPostmaster && dynamic_shared_memory_type != DSM_IMPL_NONE && parse->commandType == CMD_SELECT && !parse->hasModifyingCTE && parse->utilityStmt == NULL && max_parallel_workers_per_gather > 0 && - !IsParallelWorker() && !IsolationIsSerializable() && - !has_parallel_hazard((Node *) parse, true); + max_parallel_workers > 0 && !IsParallelWorker() && + !IsolationIsSerializable() && !has_parallel_hazard((Node *) parse, +
Re: [HACKERS] Rename max_parallel_degree?
On 11/06/2016 23:37, Julien Rouhaud wrote:
> On 09/06/2016 16:04, Robert Haas wrote:
>>
>> OK, I pushed this after re-reviewing it and fixing a number of
>> oversights. There remains only the task of adding max_parallel_degree
>> as a system-wide limit (as opposed to max_parallel_degree now
>> max_parallel_workers_per_gather which is a per-Gather limit), which
>> I'm going to argue should be a new open item and not necessarily one
>> that I have to own myself. I would like to take care of it, but I
>> will not put it ahead of fixing actual defects and I will not promise
>> to have it done in time for 9.6.
>>
>
> PFA a patch to fix this open item. I used the GUC name provided in the
> 9.6 open item page (max_parallel_workers), with a default value of 4.
> Value 0 is another way to disable parallel query.
>
Sorry I just realized I made a stupid mistake, I didn't check in all
slots to get the number of active parallel workers. Fixed in attached v2.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e0e5a1e..c661c7a 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2018,6 +2018,23 @@ include_dir 'conf.d'
+
+ max_parallel_workers (integer)
+
+max_parallel_workers configuration
parameter
+
+
+
+
+ Sets the maximum number of workers that can be launched at the same
+ time for the whole server. This parameter allows the administrator to
+ reserve background worker slots for for third part dynamic background
+ workers. The default value is 4. Setting this value to 0 disables
+ parallel query execution.
+
+
+
+
backend_flush_after (integer)
diff --git a/src/backend/access/transam/parallel.c
b/src/backend/access/transam/parallel.c
index ab5ef25..7b53b53 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -473,8 +473,8 @@ LaunchParallelWorkers(ParallelContext *pcxt)
{
memcpy(worker.bgw_extra, &i, sizeof(int));
if (!any_registrations_failed &&
- RegisterDynamicBackgroundWorker(&worker,
-
&pcxt->worker[i].bgwhandle))
+ RegisterDynamicBackgroundWorkerInternal(&worker,
+
&pcxt->worker[i].bgwhandle, true))
{
shm_mq_set_handle(pcxt->worker[i].error_mqh,
pcxt->worker[i].bgwhandle);
diff --git a/src/backend/optimizer/path/allpaths.c
b/src/backend/optimizer/path/allpaths.c
index cc8ba61..2bcd86b 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -719,9 +719,11 @@ create_plain_partial_paths(PlannerInfo *root, RelOptInfo
*rel)
}
/*
-* In no case use more than max_parallel_workers_per_gather workers.
+* In no case use more than max_parallel_workers or
+* max_parallel_workers_per_gather workers.
*/
- parallel_workers = Min(parallel_workers,
max_parallel_workers_per_gather);
+ parallel_workers = Min(max_parallel_workers, Min(parallel_workers,
+ max_parallel_workers_per_gather));
/* If any limit was set to zero, the user doesn't want a parallel scan.
*/
if (parallel_workers <= 0)
diff --git a/src/backend/optimizer/path/costsize.c
b/src/backend/optimizer/path/costsize.c
index e7f63f4..fd62126 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -113,6 +113,7 @@ int effective_cache_size =
DEFAULT_EFFECTIVE_CACHE_SIZE;
Cost disable_cost = 1.0e10;
+intmax_parallel_workers = 4;
intmax_parallel_workers_per_gather = 2;
bool enable_seqscan = true;
diff --git a/src/backend/optimizer/plan/planner.c
b/src/backend/optimizer/plan/planner.c
index 54c0440..81d124c 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -246,8 +246,9 @@ standard_planner(Query *parse, int cursorOptions,
ParamListInfo boundParams)
IsUnderPostmaster && dynamic_shared_memory_type !=
DSM_IMPL_NONE &&
parse->commandType == CMD_SELECT && !parse->hasModifyingCTE &&
parse->utilityStmt == NULL && max_parallel_workers_per_gather >
0 &&
- !IsParallelWorker() && !IsolationIsSerializable(
Re: [HACKERS] Rename max_parallel_degree?
On 09/06/2016 16:04, Robert Haas wrote:
>
> OK, I pushed this after re-reviewing it and fixing a number of
> oversights. There remains only the task of adding max_parallel_degree
> as a system-wide limit (as opposed to max_parallel_degree now
> max_parallel_workers_per_gather which is a per-Gather limit), which
> I'm going to argue should be a new open item and not necessarily one
> that I have to own myself. I would like to take care of it, but I
> will not put it ahead of fixing actual defects and I will not promise
> to have it done in time for 9.6.
>
PFA a patch to fix this open item. I used the GUC name provided in the
9.6 open item page (max_parallel_workers), with a default value of 4.
Value 0 is another way to disable parallel query.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e0e5a1e..c661c7a 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -2018,6 +2018,23 @@ include_dir 'conf.d'
+
+ max_parallel_workers (integer)
+
+max_parallel_workers configuration
parameter
+
+
+
+
+ Sets the maximum number of workers that can be launched at the same
+ time for the whole server. This parameter allows the administrator to
+ reserve background worker slots for for third part dynamic background
+ workers. The default value is 4. Setting this value to 0 disables
+ parallel query execution.
+
+
+
+
backend_flush_after (integer)
diff --git a/src/backend/access/transam/parallel.c
b/src/backend/access/transam/parallel.c
index ab5ef25..7b53b53 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -473,8 +473,8 @@ LaunchParallelWorkers(ParallelContext *pcxt)
{
memcpy(worker.bgw_extra, &i, sizeof(int));
if (!any_registrations_failed &&
- RegisterDynamicBackgroundWorker(&worker,
-
&pcxt->worker[i].bgwhandle))
+ RegisterDynamicBackgroundWorkerInternal(&worker,
+
&pcxt->worker[i].bgwhandle, true))
{
shm_mq_set_handle(pcxt->worker[i].error_mqh,
pcxt->worker[i].bgwhandle);
diff --git a/src/backend/optimizer/path/allpaths.c
b/src/backend/optimizer/path/allpaths.c
index cc8ba61..2bcd86b 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -719,9 +719,11 @@ create_plain_partial_paths(PlannerInfo *root, RelOptInfo
*rel)
}
/*
-* In no case use more than max_parallel_workers_per_gather workers.
+* In no case use more than max_parallel_workers or
+* max_parallel_workers_per_gather workers.
*/
- parallel_workers = Min(parallel_workers,
max_parallel_workers_per_gather);
+ parallel_workers = Min(max_parallel_workers, Min(parallel_workers,
+ max_parallel_workers_per_gather));
/* If any limit was set to zero, the user doesn't want a parallel scan.
*/
if (parallel_workers <= 0)
diff --git a/src/backend/optimizer/path/costsize.c
b/src/backend/optimizer/path/costsize.c
index e7f63f4..fd62126 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -113,6 +113,7 @@ int effective_cache_size =
DEFAULT_EFFECTIVE_CACHE_SIZE;
Cost disable_cost = 1.0e10;
+intmax_parallel_workers = 4;
intmax_parallel_workers_per_gather = 2;
bool enable_seqscan = true;
diff --git a/src/backend/optimizer/plan/planner.c
b/src/backend/optimizer/plan/planner.c
index 54c0440..81d124c 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -246,8 +246,9 @@ standard_planner(Query *parse, int cursorOptions,
ParamListInfo boundParams)
IsUnderPostmaster && dynamic_shared_memory_type !=
DSM_IMPL_NONE &&
parse->commandType == CMD_SELECT && !parse->hasModifyingCTE &&
parse->utilityStmt == NULL && max_parallel_workers_per_gather >
0 &&
- !IsParallelWorker() && !IsolationIsSerializable() &&
- !has_parallel_hazard((Node *) parse, true);
+ max_parallel_workers > 0 && !IsParallelWorker() &&
+ !IsolationIsSerializable() && !has_parallel_hazard((Node *)
parse,
+
[HACKERS] max_worker_processes missing some documentation
I noticed that postgresql.conf.sample doesn't say that changing max_worker_processes requires a restart. Patch attached. -- Julien Rouhaud http://dalibo.com - http://dalibo.org diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index e6c6591..2d2db7e 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -166,7 +166,7 @@ # - Asynchronous Behavior - #effective_io_concurrency = 1 # 1-1000; 0 disables prefetching -#max_worker_processes = 8 +#max_worker_processes = 8 # (change requires restart) #max_parallel_degree = 2 # max number of worker processes per node #old_snapshot_threshold = -1 # 1min-60d; -1 disables; 0 is immediate # (change requires restart) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg9.6 segfault using simple query (related to use fk for join estimates)
On 29/04/2016 18:05, Tom Lane wrote:
> Julien Rouhaud writes:
>> The segfault is caused by quals_match_foreign_key() calling get_leftop()
>> and get_rightop() on a ScalarArrayOpExpr node.
>
>> Reordering the common fields of OpExpr and ScalarArrayOpExpr at the
>> beginning of the struct so the get_*op() work with either (as in
>> attached patch) fixes the issue.
>
>> I'm not sure that assuming this compatibility is the right way to fix
>> this though.
>
> It certainly isn't.
>
Agreed. New attached patch handles explicitly each node tag.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
diff --git a/src/backend/optimizer/util/clauses.c
b/src/backend/optimizer/util/clauses.c
index 759566a..3b9715b 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -100,6 +100,7 @@ typedef struct
boolallow_restricted;
} has_parallel_hazard_arg;
+static List *get_opargs(const Expr *clause);
static bool aggregates_allow_partial_walker(Node *node,
partial_agg_context *context);
static bool contain_agg_clause_walker(Node *node, void *context);
@@ -197,6 +198,19 @@ make_opclause(Oid opno, Oid opresulttype, bool opretset,
return (Expr *) expr;
}
+static List *
+get_opargs(const Expr *clause)
+{
+ if (IsA(clause, OpExpr))
+ return ((OpExpr *) clause)->args;
+
+ if (IsA(clause, ScalarArrayOpExpr))
+ return ((ScalarArrayOpExpr *) clause)->args;
+
+ elog(ERROR, "unrecognized node type: %d",
+ (int) nodeTag(clause));
+}
+
/*
* get_leftop
*
@@ -206,10 +220,10 @@ make_opclause(Oid opno, Oid opresulttype, bool opretset,
Node *
get_leftop(const Expr *clause)
{
- const OpExpr *expr = (const OpExpr *) clause;
+ const List *args = get_opargs(clause);
- if (expr->args != NIL)
- return linitial(expr->args);
+ if (args != NIL)
+ return linitial(args);
else
return NULL;
}
@@ -223,10 +237,10 @@ get_leftop(const Expr *clause)
Node *
get_rightop(const Expr *clause)
{
- const OpExpr *expr = (const OpExpr *) clause;
+ const List *args = get_opargs(clause);
- if (list_length(expr->args) >= 2)
- return lsecond(expr->args);
+ if (list_length(args) >= 2)
+ return lsecond(args);
else
return NULL;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg9.6 segfault using simple query (related to use fk for join estimates)
On 29/04/2016 13:20, Michael Paquier wrote: > On Fri, Apr 29, 2016 at 7:25 PM, Stefan Huehner wrote: >> If you need any more info or testing done just let me know. > > The information you are providing is sufficient to reproduce the > problem, thanks! I have added this bug to the list of open items for > 9.6. > The segfault is caused by quals_match_foreign_key() calling get_leftop() and get_rightop() on a ScalarArrayOpExpr node. Reordering the common fields of OpExpr and ScalarArrayOpExpr at the beginning of the struct so the get_*op() work with either (as in attached patch) fixes the issue. I'm not sure that assuming this compatibility is the right way to fix this though. -- Julien Rouhaud http://dalibo.com - http://dalibo.org diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h index 1ffc0a1..dffe129 100644 --- a/src/include/nodes/primnodes.h +++ b/src/include/nodes/primnodes.h @@ -468,12 +468,12 @@ typedef struct OpExpr Exprxpr; Oid opno; /* PG_OPERATOR OID of the operator */ Oid opfuncid; /* PG_PROC OID of underlying function */ - Oid opresulttype; /* PG_TYPE OID of result value */ - boolopretset; /* true if operator returns set */ - Oid opcollid; /* OID of collation of result */ Oid inputcollid;/* OID of collation that operator should use */ List *args; /* arguments to the operator (1 or 2) */ int location; /* token location, or -1 if unknown */ + Oid opresulttype; /* PG_TYPE OID of result value */ + boolopretset; /* true if operator returns set */ + Oid opcollid; /* OID of collation of result */ } OpExpr; /* @@ -511,10 +511,10 @@ typedef struct ScalarArrayOpExpr Exprxpr; Oid opno; /* PG_OPERATOR OID of the operator */ Oid opfuncid; /* PG_PROC OID of underlying function */ - booluseOr; /* true for ANY, false for ALL */ Oid inputcollid;/* OID of collation that operator should use */ List *args; /* the scalar and array operands */ int location; /* token location, or -1 if unknown */ + booluseOr; /* true for ANY, false for ALL */ } ScalarArrayOpExpr; /* -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Memory leak in GIN index build
On 18/04/2016 16:33, Tom Lane wrote: > > I poked at this over the weekend, and got more unhappy the more I poked. > Aside from the memory leakage issue, there are multiple coding-rule > violations besides the one you noted about scope of the critical sections. > One example is that in the page-split path for an inner page, we modify > the contents of childbuf long before getting into the critical section. > The number of out-of-date comments was depressingly large as well. > > I ended up deciding that the most reasonable fix was along the lines of > my first alternative above. In the attached draft patches, the > "placeToPage" method is split into two methods, "beginPlaceToPage" and > "execPlaceToPage", where the second method is what's supposed to happen > inside the critical section for a non-page-splitting update. Nothing > that's done inside beginPlaceToPage is critical. > > I've tested this to the extent of verifying that it passes make > check-world and stops the memory leak in your test case, but it could use > more eyeballs on it. > Thanks! I also started working on it but it was very far from being complete (and already much more ugly...). I couldn't find any problem in the patch. I wonder if asserting being in a critical section would be valuable in the *execPlaceToPage functions, or that (leaf->walinfolen > 0) in dataExecPlaceToPageLeaf(), since it's filled far from this function. > Attached are draft patches against HEAD and 9.5 (they're the same except > for BufferGetPage calls). I think we'd also want to back-patch to 9.4, > but that will take considerable additional work because of the XLOG API > rewrite that happened in 9.5. I also noted that some of the coding-rule > violations seem to be new in 9.5, so the problems may be less severe in > 9.4 --- the memory leak definitely exists there, though. > -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Memory leak in GIN index build
On 16/04/2016 20:45, Tom Lane wrote: > Julien Rouhaud writes: > >> Also, in dataPlaceToPageLeaf() and ginVacuumPostingTreeLeaf(), shouldn't >> the START_CRIT_SECTION() calls be placed before the xlog code? > > Yeah, they should. Evidently somebody kluged it to avoid doing a palloc > inside a critical section, while ignoring every other rule about where to > place critical sections. (Maybe we should put an assert about being > within a critical section into XLogBeginInsert.) > After a quick test, it appears that at least log_smgrcreate() calls XLogBeginInsert() without being in a critical section, from the various DDL commands. > This code is pretty fundamentally broken, isn't it. elog() calls > inside a critical section? Really? Even worse, a MemoryContextDelete, > which hardly seems like something that should be assumed error-proof. > > Not to mention the unbelievable ugliness of a function that sometimes > returns with an open critical section (and WAL insertion started) and > sometimes doesn't. > > It kinda looks like this was hacked up without bothering to keep the > comment block in ginPlaceToPage in sync with reality, either. > > I think this needs to be redesigned so that the critical section and WAL > insertion calls all happen within a single straight-line piece of code. > > We could try making that place be ginPlaceToPage(). So far as > registerLeafRecompressWALData is concerned, that does not look that hard; > it could palloc and fill the WAL-data buffer the same as it's doing now, > then pass it back up to be registered (and eventually pfreed) in > ginPlaceToPage. However, that would also mean postponing the call of > dataPlaceToPageLeafRecompress(), which seems much messier. I assume > the data it's working from is mostly in the tmpCtx that > dataPlaceToPageLeaf wants to free before returning. Maybe we could > move creation/destruction of the tmpCtx out to ginPlaceToPage, as well? > > The other line of thought is to move the WAL work that ginPlaceToPage > does down into the subroutines. That would probably result in some > duplication of code, but it might end up being a cleaner solution. > > Anyway, I think memory leakage is just the start of what's broken here, > and fixing it won't be a very small patch. > I'm not really confident, but I'll give a try. Probably with your second solution which looks easier. Any pointer is welcome, unless someone more familiar with this code wants to take it. -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Memory leak in GIN index build
Hello, Another colleague provided a report of memory leak, during a GIN index build. Test case to reproduce the attached (need to create a gin index on the val column after loading). Sorry, it generates a 24GB table, and memory start leaking with a 1GB maintenance_work_mem after reaching 8 or 9 times the m_w_m setting, leading to ~ 9GB used in Gin build temporary context. After some digging, the leak comes from walbufbegin palloc in registerLeafRecompressWALData(). IIUC, walbufbegin isn't pfree-d and can't be before XLogInsert() is called, which happens in ginPlaceToPage(). I don't see a simple way to fix that. My first idea would be to change GinBtreeData's placeToPage (and all other needed) functions signature to pass a pointer to pfree in ginPlaceToPage() if needed, but it doesn't really seem appealing. In any case, I'd be happy to work on a patch if needed. Also, in dataPlaceToPageLeaf() and ginVacuumPostingTreeLeaf(), shouldn't the START_CRIT_SECTION() calls be placed before the xlog code? Regards. -- Julien Rouhaud http://dalibo.com - http://dalibo.org generate_data.pl Description: Perl program -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Memory leak when querying GIN indexes
Hello, My colleague Adrien reported me a memory leak in GIN indexes while doing some benchmark on several am. Here is a test case to reproduce the issue: CREATE TABLE test AS ( SELECT t FROM generate_series(now(), now() + interval '10 day', '1 second') AS d(t) CROSS JOIN generate_series(1, 100) s ); CREATE EXTENSON btree_gin; CREATE INDEX ON test USING gin(t); EXPLAIN ANALYZE SELECT * FROM test WHERE t >= now() and t < now() + interval '10 day'; The last query will consume approximately 4GB of RAM (might need to force index scan) in ExecutorState memory context. I'm not at all familiar with GIN code, but naive attached patch seems to fix the issue and not break anything. I can reproduce this issue up to 9.4. Regards -- Julien Rouhaud http://dalibo.com - http://dalibo.org diff --git a/src/backend/access/gin/ginget.c b/src/backend/access/gin/ginget.c index b79ba1e..6108298 100644 --- a/src/backend/access/gin/ginget.c +++ b/src/backend/access/gin/ginget.c @@ -281,6 +281,7 @@ collectMatchBitmap(GinBtreeData *btree, GinBtreeStack *stack, ipd = ginReadTuple(btree->ginstate, scanEntry->attnum, itup, &nipd); tbm_add_tuples(scanEntry->matchBitmap, ipd, nipd, false); scanEntry->predictNumberResult += GinGetNPosting(itup); + pfree(ipd); } /* -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Choosing parallel_degree
On 13/04/2016 19:17, Robert Haas wrote:
> On Tue, Apr 12, 2016 at 6:31 PM, Julien Rouhaud
> wrote:
>> On 11/04/2016 22:53, Julien Rouhaud wrote:
>>> On 11/04/2016 17:44, Robert Haas wrote:
>>>> We should probably add the number of workers actually obtained to the
>>>> EXPLAIN ANALYZE output. That's been requested before.
>>>
>>> If it's not too late for 9.6, it would be very great.
>>
>> Just in case I attach a patch to implement it. I'll add it to the next
>> commitfest.
>
> I think we should go with "Workers Planned" and "Workers Launched",
> capitalized exactly that way, and lose "Number Of".
>
Fixed
> I would be inclined to view this as a reasonable 9.6 cleanup of
> parallel query, but other people may wish to construe things more
> strictly than I would.
>
FWIW, I also see it as a reasonable cleanup.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 713cd0e..379fc5c 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -1339,8 +1339,16 @@ ExplainNode(PlanState *planstate, List *ancestors,
if (plan->qual)
show_instrumentation_count("Rows
Removed by Filter", 1,
planstate, es);
- ExplainPropertyInteger("Number of Workers",
+ ExplainPropertyInteger("Workers Planned",
gather->num_workers, es);
+ if (es->analyze)
+ {
+ int nworkers;
+
+ nworkers = ((GatherState *)
planstate)->nworkers_launched;
+ ExplainPropertyInteger("Workers
Launched",
+
nworkers, es);
+ }
if (gather->single_copy)
ExplainPropertyText("Single Copy",
gather->single_copy ? "true" : "false",
diff --git a/src/backend/executor/nodeGather.c
b/src/backend/executor/nodeGather.c
index 3f0ed69..3834ed6 100644
--- a/src/backend/executor/nodeGather.c
+++ b/src/backend/executor/nodeGather.c
@@ -166,6 +166,7 @@ ExecGather(GatherState *node)
*/
pcxt = node->pei->pcxt;
LaunchParallelWorkers(pcxt);
+ node->nworkers_launched = pcxt->nworkers_launched;
/* Set up tuple queue readers to read the results. */
if (pcxt->nworkers_launched > 0)
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index dbec07e..ee4e189 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1956,6 +1956,7 @@ typedef struct GatherState
struct ParallelExecutorInfo *pei;
int nreaders;
int nextreader;
+ int nworkers_launched;
struct TupleQueueReader **reader;
TupleTableSlot *funnel_slot;
boolneed_to_scan_locally;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Choosing parallel_degree
On 11/04/2016 22:53, Julien Rouhaud wrote:
> On 11/04/2016 17:44, Robert Haas wrote:
>>
>> We should probably add the number of workers actually obtained to the
>> EXPLAIN ANALYZE output. That's been requested before.
>>
>
> If it's not too late for 9.6, it would be very great.
>
Just in case I attach a patch to implement it. I'll add it to the next
commitfest.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 713cd0e..8a1fbab 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -1339,8 +1339,16 @@ ExplainNode(PlanState *planstate, List *ancestors,
if (plan->qual)
show_instrumentation_count("Rows
Removed by Filter", 1,
planstate, es);
- ExplainPropertyInteger("Number of Workers",
+ ExplainPropertyInteger("Number of Workers
planned",
gather->num_workers, es);
+ if (es->analyze)
+ {
+ int nworkers;
+
+ nworkers = ((GatherState *)
planstate)->nworkers_launched;
+ ExplainPropertyInteger("Number of
Workers launched",
+
nworkers, es);
+ }
if (gather->single_copy)
ExplainPropertyText("Single Copy",
gather->single_copy ? "true" : "false",
diff --git a/src/backend/executor/nodeGather.c
b/src/backend/executor/nodeGather.c
index 3f0ed69..3834ed6 100644
--- a/src/backend/executor/nodeGather.c
+++ b/src/backend/executor/nodeGather.c
@@ -166,6 +166,7 @@ ExecGather(GatherState *node)
*/
pcxt = node->pei->pcxt;
LaunchParallelWorkers(pcxt);
+ node->nworkers_launched = pcxt->nworkers_launched;
/* Set up tuple queue readers to read the results. */
if (pcxt->nworkers_launched > 0)
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index dbec07e..ee4e189 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -1956,6 +1956,7 @@ typedef struct GatherState
struct ParallelExecutorInfo *pei;
int nreaders;
int nextreader;
+ int nworkers_launched;
struct TupleQueueReader **reader;
TupleTableSlot *funnel_slot;
boolneed_to_scan_locally;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Choosing parallel_degree
On 11/04/2016 17:44, Robert Haas wrote:
> On Mon, Apr 11, 2016 at 11:27 AM, Julien Rouhaud
> wrote:
>> On 11/04/2016 15:56, tushar wrote:
>>>
>>> I am assuming parallel_degree=0 is as same as not using it , i.e
>>> create table fok2(n int) with (parallel_degree=0); = create table
>>> fok2(n int);
>>>
>>> so in this case it should have accepted the parallel seq .scan.
>>>
>>
>> No, setting it to 0 means to force not using parallel workers (but
>> considering the parallel path IIRC).
>
> I'm not sure what the parenthesized bit means, because you can't use
> parallelism without workers.
Obvious mistake, sorry.
> But I think I should have made the docs
> more clear that 0 = don't parallelize scans of this table while
> committing this. Maybe we should go add a sentence about that.
>
What about
- the setting of .
+ the setting of . Setting
this
+ parameter to 0 will disable parallelism for that table.
>> Even if you set a per-table parallel_degree higher than
>> max_parallel_degree, it'll be maxed at max_parallel_degree.
>>
>> Then, the explain shows that the planner assumed it'll launch 9 workers,
>> but only 8 were available (or needed perhaps) at runtime.
>
> We should probably add the number of workers actually obtained to the
> EXPLAIN ANALYZE output. That's been requested before.
>
If it's not too late for 9.6, it would be very great.
>>> postgres=# set max_parallel_degree =262;
>>> ERROR: 262 is outside the valid range for parameter
>>> "max_parallel_degree" (0 .. 262143)
>>>
>>> postgres=# set max_parallel_degree =262143;
>>> SET
>>> postgres=#
>>>
>>> postgres=# explain analyze verbose select * from abd where n<=1;
>>> ERROR: requested shared memory size overflows size_t
>>>
>>> if we remove the analyze keyword then query running successfully.
>>>
>>> Expected = Is it not better to throw the error at the time of setting
>>> max_parallel_degree, if not supported ?
>>
>> +1
>
> It surprises me that that request overflowed size_t. I guess we
> should look into why that's happening. Did you test this on a 32-bit
> system?
>
I can reproduce the same issue on a 64 bits system. Setting
max_parallel_degree to 32768 or above raise this error:
ERROR: could not resize shared memory segment "/PostgreSQL.44279285" to
18446744072113360072 bytes: Invalid argument
On a 32 bits system, following assert fails:
TRAP: FailedAssertion("!(offset < total_bytes)", File: "shm_toc.c",
Line: 192)
After some gdb, it looks like the overflow comes from
/* Estimate space for tuple queues. */
shm_toc_estimate_chunk(&pcxt->estimator,
PARALLEL_TUPLE_QUEUE_SIZE * pcxt->nworkers);
372 shm_toc_estimate_chunk(&pcxt->estimator,
(gdb) p pcxt->estimator
$2 = {space_for_chunks = 3671712, number_of_keys = 3}
(gdb) n
374 shm_toc_estimate_keys(&pcxt->estimator, 1);
(gdb) p pcxt->estimator
$3 = {space_for_chunks = 18446744071565739680, number_of_keys = 3}
Following change fix the issue:
diff --git a/src/backend/executor/execParallel.c
b/src/backend/executor/execParallel.c
index 572a77b..0a5210e 100644
--- a/src/backend/executor/execParallel.c
+++ b/src/backend/executor/execParallel.c
@@ -370,7 +370,7 @@ ExecInitParallelPlan(PlanState *planstate, EState
*estate, int nworkers)
/* Estimate space for tuple queues. */
shm_toc_estimate_chunk(&pcxt->estimator,
- PARALLEL_TUPLE_QUEUE_SIZE * pcxt->nworkers);
+ (Size) PARALLEL_TUPLE_QUEUE_SIZE *
pcxt->nworkers);
shm_toc_estimate_keys(&pcxt->estimator, 1);
But the query still fails with "ERROR: out of shared memory".
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Choosing parallel_degree
On 11/04/2016 15:56, tushar wrote: > On 04/08/2016 08:53 PM, Robert Haas wrote: >> On Fri, Apr 8, 2016 at 1:22 AM, Amit Kapila wrote: >>> Other than that, patch looks good and I have marked it as Ready For >>> Committer. Hope, we get this for 9.6. >> Committed. I think this is likely to make parallel query >> significantly more usable in 9.6. >> > While testing ,I observed couple of things - > > Case 1 =Not accepting parallel seq scan when parallel_degree is set to 0 > > postgres=# create table fok2(n int) with (parallel_degree=0); > CREATE TABLE > postgres=# insert into fok2 values (generate_series(1,100)); analyze > fok2; vacuum fok2; > INSERT 0 100 > ANALYZE > VACUUM > postgres=# set max_parallel_degree =5; > SET > postgres=# explain analyze verbose select * from fok2 where n<=10; > QUERY > PLAN > -- > Seq Scan on public.fok2 (cost=0.00..16925.00 rows=100 width=4) (actual > time=0.027..217.882 rows=10 loops=1) >Output: n >Filter: (fok2.n <= 10) >Rows Removed by Filter: 90 > Planning time: 0.084 ms > Execution time: 217.935 ms > (6 rows) > > I am assuming parallel_degree=0 is as same as not using it , i.e > create table fok2(n int) with (parallel_degree=0); = create table > fok2(n int); > > so in this case it should have accepted the parallel seq .scan. > No, setting it to 0 means to force not using parallel workers (but considering the parallel path IIRC). > Case 2=Total no# of workers are NOT matching with the workers information - > > postgres=# alter table fok set (parallel_degree=10); > ALTER TABLE > postgres=# set max_parallel_degree =9; > SET > postgres=# explain analyze verbose select * from fok where n<=1; >QUERY > PLAN > - > Gather (cost=1000.00..6823.89 rows=100 width=4) (actual > time=0.621..107.755 rows=1 loops=1) >Output: n > * Number of Workers: 9* >-> Parallel Seq Scan on public.fok (cost=0.00..5814.00 rows=11 > width=4) (actual time=83.382..95.157 rows=0 loops=9) > Output: n > Filter: (fok.n <= 1) > Rows Removed by Filter: 11 > Worker 0: actual time=82.181..82.181 rows=0 loops=1 > Worker 1: actual time=97.236..97.236 rows=0 loops=1 > Worker 2: actual time=93.586..93.586 rows=0 loops=1 > Worker 3: actual time=94.159..94.159 rows=0 loops=1 > Worker 4: actual time=88.459..88.459 rows=0 loops=1 > Worker 5: actual time=90.245..90.245 rows=0 loops=1 > Worker 6: actual time=101.577..101.577 rows=0 loops=1 > Worker 7: actual time=102.955..102.955 rows=0 loops=1 > Planning time: 0.119 ms > Execution time: 108.585 ms > (17 rows) > > Expected = Expecting worker8 information , also loops=10 (including the > Master) > Even if you set a per-table parallel_degree higher than max_parallel_degree, it'll be maxed at max_parallel_degree. Then, the explain shows that the planner assumed it'll launch 9 workers, but only 8 were available (or needed perhaps) at runtime. > Case 3=Getting error if we set the max value in max_parallel_degree as > well in parallel_degree . > > postgres=# create table abd(n int) with (parallel_degree=262144); > ERROR: value 262144 out of bounds for option "parallel_degree" > DETAIL: Valid values are between "0" and "262143". > > postgres=# create table abd(n int) with (parallel_degree=262143); > CREATE TABLE > postgres=# insert into abd values (generate_series(1,100)); analyze > abd; vacuum abd; > INSERT 0 100 > ANALYZE > > postgres=# set max_parallel_degree =262; > ERROR: 262 is outside the valid range for parameter > "max_parallel_degree" (0 .. 262143) > > postgres=# set max_parallel_degree =262143; > SET > postgres=# > > postgres=# explain analyze verbose select * from abd where n<=1; > ERROR: requested shared memory size overflows size_t > > if we remove the analyze keyword then query running successfully. > > Expected = Is it not better to throw the error at the time of setting > max_parallel_degree, if not supported ? +1 -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Choosing parallel_degree
On 06/04/2016 07:38, Amit Kapila wrote:
> On Tue, Apr 5, 2016 at 11:55 PM, Julien Rouhaud
>>
>> In alter_table.sgml, I didn't comment the lock level needed to modify
>> parallel_degree since it requires an access exclusive lock for now.
>> While thinking about it, I think it's safe to use a share update
>> exclusive lock but I may be wrong. What do you think?
>>
>
> We require to keep AccessExclusiveLock for operations which can impact
> Select operation which I think this operation does, so lets
> retain AccessExclusiveLock for now. If somebody else thinks, we should
> not bother about Selects, then we can change it.
>
Ok. Isn't there also some considerations about forcing replanning of
prepared statements using the table for instance?
>>
>> I find your version better once again, but should we keep some
>> consistency between them or it's not important?
>>
>
> I think consistency is good, but this is different from
> max_parallel_degree, so I would prefer to use something on lines of what
> I have mentioned.
>
Agreed, changed in attached v8 (including fix for previous mail).
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
diff --git a/doc/src/sgml/ref/create_table.sgml
b/doc/src/sgml/ref/create_table.sgml
index cd234db..0eab2be 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -909,6 +909,17 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } |
UNLOGGED ] TABLE [ IF NOT EXI
+parallel_degree (integer)
+
+
+ Sets the degree of parallelism for an individual relation. The requested
+ number of workers will be limited by .
+
+
+
+
+
autovacuum_enabled,
toast.autovacuum_enabled (boolean)
diff --git a/src/backend/access/common/reloptions.c
b/src/backend/access/common/reloptions.c
index ea0755a..758457c 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -26,6 +26,7 @@
#include "commands/tablespace.h"
#include "commands/view.h"
#include "nodes/makefuncs.h"
+#include "postmaster/postmaster.h"
#include "utils/array.h"
#include "utils/attoptcache.h"
#include "utils/builtins.h"
@@ -267,6 +268,15 @@ static relopt_int intRelOpts[] =
0, 0, 0
#endif
},
+ {
+ {
+ "parallel_degree",
+ "Number of parallel processes that can be used per
executor node for this relation.",
+ RELOPT_KIND_HEAP,
+ AccessExclusiveLock
+ },
+ -1, 0, MAX_BACKENDS
+ },
/* list terminator */
{{NULL}}
@@ -1251,8 +1261,8 @@ fillRelOptions(void *rdopts, Size basesize,
/*
- * Option parser for anything that uses StdRdOptions (i.e. fillfactor and
- * autovacuum)
+ * Option parser for anything that uses StdRdOptions (i.e. fillfactor,
+ * autovacuum and parallel_degree)
*/
bytea *
default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
@@ -1291,7 +1301,9 @@ default_reloptions(Datum reloptions, bool validate,
relopt_kind kind)
{"autovacuum_analyze_scale_factor", RELOPT_TYPE_REAL,
offsetof(StdRdOptions, autovacuum) +offsetof(AutoVacOpts,
analyze_scale_factor)},
{"user_catalog_table", RELOPT_TYPE_BOOL,
- offsetof(StdRdOptions, user_catalog_table)}
+ offsetof(StdRdOptions, user_catalog_table)},
+ {"parallel_degree", RELOPT_TYPE_INT,
+ offsetof(StdRdOptions, parallel_degree)}
};
options = parseRelOptions(reloptions, validate, kind, &numoptions);
diff --git a/src/backend/optimizer/path/allpaths.c
b/src/backend/optimizer/path/allpaths.c
index cc77ff9..38233bc 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -659,31 +659,42 @@ set_plain_rel_pathlist(PlannerInfo *root, RelOptInfo
*rel, RangeTblEntry *rte)
static void
create_parallel_paths(PlannerInfo *root, RelOptInfo *rel)
{
- int parallel_threshold = 1000;
- int parallel_degree = 1;
+ int parallel_threshold = 1000;
+ int parallel_degree = 1;
/*
* If this relation is too small to be worth a parallel scan, just
return
-* without doing anything ... unless it's an inheritance child. In
that case,
-* we want to generate a parallel path here anyway. It might not be
worthwhile
-* just for this relation, but when combined with all of its
inheritance siblings
-* it may well pay off.
+* without doing anything ... unless parallel_degree has been set for
this
Re: [HACKERS] Choosing parallel_degree
On 05/04/2016 06:19, Amit Kapila wrote:
>
> Few more comments:
>
> 1.
> @@ -909,6 +909,17 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } |
> UNLOGGED ] TABLE [ IF NOT EXI
>
>
>
>
> +parallel_degree (integer)
> +
> +
>
> + Sets the degree of parallelism for an individual relation. The
> requested
> + number of workers will be
> limited by + linkend="guc-max-parallel-degree">.
> +
> +
> +
>
> All other parameters in this category are supportted by Alter table
> command as well, so I think this parameter should also be supported by
> Alter Table command (for both SET and RESET variants).
>
I don't quite understand. With the patch you can use parallel_degree in
either CREATE or ALTER table (SET and RESET) statements. Considering
documentation, the list of storage parameters only appears in
create_table.sgml, alter_table.sgml pointing to it.
In alter_table.sgml, I didn't comment the lock level needed to modify
parallel_degree since it requires an access exclusive lock for now.
While thinking about it, I think it's safe to use a share update
exclusive lock but I may be wrong. What do you think?
> 2.
> +"Number of parallel processes per executor node wanted for this relation.",
>
> How about
> Number of parallel processes that can be used for this relation per
> executor node.
>
I just rephrased what was used for the max_parallel_degree GUC, which is:
"Sets the maximum number of parallel processes per executor node."
I find your version better once again, but should we keep some
consistency between them or it's not important?
> 3.
> -if (rel->pages < parallel_threshold && rel->reloptkind == RELOPT_BASEREL)
> +if (rel->pages <
> parallel_threshold && rel->rel_parallel_degree == -1 &&
> +rel->reloptkind == RELOPT_BASEREL)
>
> A. Second line should be indented with the begin of first line after
> bracket '(' which means with rel->pages. Refer multiline condition in
> near by code. Or you can run pgindent.
I ran pgindent, fixed.
> B. The comment above this condition needs slight adjustment as per new
> condition.
>
Also fixed.
> 4.
> +intparallel_degree; /* max number of parallel worker */
> } StdRdOptions;
>
> Typo in comments
> /worker/workers
>
fixed.
I'll send an updated patch when I'll know what to do about the first two
points.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Choosing parallel_degree
On 04/04/2016 17:03, Julien Rouhaud wrote:
> On 04/04/2016 08:55, Amit Kapila wrote:
>
> Thanks for the review!
>
>> Few comments:
>> 1.
>> + limited according to the
>>
>> A. typo.
>>/gux-max-parallel-degree/guc-max-parallel-degree
>>/worker/workers
>
> Oops, fixed.
>
And I managed to no fix it, sorry :/ Thanks to Andreas who warned me.
--
Julien Rouhaud
http://dalibo.com - http://dalibo.org
diff --git a/doc/src/sgml/ref/create_table.sgml
b/doc/src/sgml/ref/create_table.sgml
index cd234db..0eab2be 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -909,6 +909,17 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } |
UNLOGGED ] TABLE [ IF NOT EXI
+parallel_degree (integer)
+
+
+ Sets the degree of parallelism for an individual relation. The requested
+ number of workers will be limited by .
+
+
+
+
+
autovacuum_enabled,
toast.autovacuum_enabled (boolean)
diff --git a/src/backend/access/common/reloptions.c
b/src/backend/access/common/reloptions.c
index ea0755a..8e4aa80 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -26,6 +26,7 @@
#include "commands/tablespace.h"
#include "commands/view.h"
#include "nodes/makefuncs.h"
+#include "postmaster/postmaster.h"
#include "utils/array.h"
#include "utils/attoptcache.h"
#include "utils/builtins.h"
@@ -267,6 +268,15 @@ static relopt_int intRelOpts[] =
0, 0, 0
#endif
},
+ {
+ {
+ "parallel_degree",
+ "Number of parallel processes per executor node wanted
for this relation.",
+ RELOPT_KIND_HEAP,
+ AccessExclusiveLock
+ },
+ -1, 0, MAX_BACKENDS
+ },
/* list terminator */
{{NULL}}
@@ -1251,8 +1261,8 @@ fillRelOptions(void *rdopts, Size basesize,
/*
- * Option parser for anything that uses StdRdOptions (i.e. fillfactor and
- * autovacuum)
+ * Option parser for anything that uses StdRdOptions (i.e. fillfactor,
+ * autovacuum and parallel_degree)
*/
bytea *
default_reloptions(Datum reloptions, bool validate, relopt_kind kind)
@@ -1291,7 +1301,9 @@ default_reloptions(Datum reloptions, bool validate,
relopt_kind kind)
{"autovacuum_analyze_scale_factor", RELOPT_TYPE_REAL,
offsetof(StdRdOptions, autovacuum) +offsetof(AutoVacOpts,
analyze_scale_factor)},
{"user_catalog_table", RELOPT_TYPE_BOOL,
- offsetof(StdRdOptions, user_catalog_table)}
+ offsetof(StdRdOptions, user_catalog_table)},
+ {"parallel_degree", RELOPT_TYPE_INT,
+ offsetof(StdRdOptions, parallel_degree)}
};
options = parseRelOptions(reloptions, validate, kind, &numoptions);
diff --git a/src/backend/optimizer/path/allpaths.c
b/src/backend/optimizer/path/allpaths.c
index cc77ff9..6032b95 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -669,21 +669,31 @@ create_parallel_paths(PlannerInfo *root, RelOptInfo *rel)
* just for this relation, but when combined with all of its
inheritance siblings
* it may well pay off.
*/
- if (rel->pages < parallel_threshold && rel->reloptkind ==
RELOPT_BASEREL)
+ if (rel->pages < parallel_threshold && rel->rel_parallel_degree == -1 &&
+ rel->reloptkind == RELOPT_BASEREL)
return;
/*
-* Limit the degree of parallelism logarithmically based on the size of
the
-* relation. This probably needs to be a good deal more sophisticated,
but we
-* need something here for now.
+* Use the table parallel_degree if specified, but don't go further than
+* max_parallel_degree
*/
- while (rel->pages > parallel_threshold * 3 &&
- parallel_degree < max_parallel_degree)
+ if (rel->rel_parallel_degree > 0)
+ parallel_degree = Min(rel->rel_parallel_degree,
max_parallel_degree);
+ else
{
- parallel_degree++;
- parallel_threshold *= 3;
- if (parallel_threshold >= PG_INT32_MAX / 3)
- break;
+ /*
+* Limit the degree of parallelism logarithmically based on the
size of the
+* relation. This probably needs to be a good deal more
sophisticated, but we
+* need something here for now.
+*/
+ while (rel->pages > parallel_threshold * 3 &&
+
