Re: [HACKERS] multivariate statistics v14

[Tomas Vondra]

Hi,

On 03/16/2016 03:58 AM, Tatsuo Ishii wrote:

I apology if it's already discussed. I am new to this patch.


Attached is v15 of the patch series, fixing this and also doing quite a
few additional improvements:

* added some basic examples into the SGML documentation

* addressing the objectaddress omissions, as pointed out by Alvaro

* support for ALTER STATISTICS ... OWNER TO / RENAME / SET SCHEMA

* significant refactoring of MCV and histogram code, particularly
  serialization, deserialization and building

* reworking the functional dependencies to support more complex
  dependencies, with multiple columns as 'conditions'

* the reduction using functional dependencies is also significantly
  simplified (I decided to get rid of computing the transitive closure
  for now - it got too complex after the multi-condition dependencies,
  so I'll leave that for the future


Do you have any other missing parts in this work? I am asking
because I wonder if you want to push this into 9.6 or rather 9.7.


I think the first few parts of the patch series, namely:

  * shared infrastructure (0002)
  * functional dependencies (0003)
  * MCV lists (0004)
  * histograms (0005)

might make it into 9.6. I believe the code for building and storing the 
different kinds of stats is reasonably solid. What probably needs more 
thorough review are the changes in clauselist_selectivity(), but the 
code in these parts is reasonably simple as it only supports using a 
single multi-variate statistics per relation.


The part (0006) that allows using multiple statistics (i.e. selects 
which of the available stats to use and in what order) is probably the 
most complex part of the whole patch, and I myself do have some 
questions about some aspects of it. I don't think this part might get 
into 9.6 at this point (although it'd be nice if we managed to do that).


I can also imagine moving the ndistinct pieces forward, in front of 0006 
if that helps getting it into 9.6. There's a bit more work on making it 
more flexible, though, to allow handling subsets columns (currently we 
need a perfect match).



regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


--
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: BSD Authentication support

[Marisa Emerson]

>Our usual wording is "the PostgreSQL user account". Perhaps we should 
>be more explicit about the fact that membership of this Unix group is 
>needed on *OpenBSD*, since other current or future BSD forks could 
>vary. I see that the specific reason this is needed on this OpenBSD 
>5.8 box is so that it can fork/exec the setuid login_XXX binaries that 
>live under /usr/libexec/auth. 

The BSD Authentication framework currently only exists on OpenBSD. I've added 
some explicit documentation that this mechanism is currently only supported on 
OpenBSD and I've tried to be a bit more explicit about the auth group as 
suggested by Peter.

>auth_userokay is called with a type of "pg-auth". I noticed from 
>looking at man page and source of some other applications that the 
>convention is usually a hardcoded string like "auth-myserver", 
>"auth-sockd", "auth-ssh", "auth-doas", "auth-popa3d" etc. So perhaps 
>we should have "auth-postgresql" (or "auth-postgres" or "auth-pgsql") 
>here? And as Peter E already said, that string should probably be 
>documented: it looks a bit like it is useful for allowing the 
>available authentication styles to be restricted or defaulted 
>specifically for PostgreSQL in login.conf based on that string. 
>(Though when I tried to set that up, it seemed to ignore my 
>possibly-incorrectly-specified rule asking it to use "reject" so I may 
>have misunderstood.) 

This is correct, although so far I've only tested using the default login 
class. The attached patch includes some more explicit documentation about this 
string. 

>The style argument is hard coded as NULL, as I see is the case in some 
>other applications. From the man page: "If style is not NULL, it 
>specifies the desired style of authentication to be used. If it is 
>NULL then the default style for the user is used. In this case, name 
>may include the desired style by appending it to the user's name with 
>a single colon (‘:’) as a separator." I wonder if such 
>user-controllable styles are OK (though I guess would require username 
>mapping to strip them off if we do want that as a feature). I wonder 
>if it should be possible to provide the style argument that we pass to 
>auth_userokay explicitly in pg_hba.conf, so that the DBA could 
>explicitly say BSD auth with style=radius. 

I've so far only tested passwd authentication. I'd be interested to test some 
of the other authentication styles, I think this would be a useful feature.


bsd_auth.patch
Description: Binary data

-- 
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] Parallel Aggregate

[David Rowley]

On 18 March 2016 at 01:22, Amit Kapila  wrote:
> On Thu, Mar 17, 2016 at 10:35 AM, David Rowley
>  wrote:
>>
>> On 17 March 2016 at 01:19, Amit Kapila  wrote:
>> > Few assorted comments:
>> >
>> > 2.
>> >  AggPath *
>> >  create_agg_path(PlannerInfo *root,
>> > @@ -2397,9 +2399,11 @@ create_agg_path(PlannerInfo *root,
>> >
>> > List *groupClause,
>> >   List *qual,
>> >   const AggClauseCosts
>> > *aggcosts,
>> > - double numGroups)
>> > + double numGroups,
>> > +
>> > bool combineStates,
>> > + bool finalizeAggs)
>> >
>> > Don't you need to set parallel_aware flag in this function as we do for
>> > create_seqscan_path()?
>>
>> I don't really know the answer to that... I mean there's nothing
>> special done in nodeAgg.c if the node is running in a worker or in the
>> main process.
>>
>
> On again thinking about it, I think it is okay to set parallel_aware flag as
> false.  This flag means whether that particular node has any parallelism
> behaviour which is true for seqscan, but I think not for partial aggregate
> node.
>
> Few other comments on latest patch:
>
> 1.
>
> + /*
> + * XXX does this need estimated for each partial path, or are they
> + * all
> going to be the same anyway?
> + */
> + dNumPartialGroups = get_number_of_groups(root,
> +
> clamp_row_est(partial_aggregate_path->rows),
> +
> rollup_lists,
> +
> rollup_groupclauses);
>
> For considering partial groups, do we need to rollup related lists?

No it doesn't you're right. I did mean to remove these, but they're
NIL anyway. Seems better to remove them to prevent confusion.

> 2.
> + hashaggtablesize = estimate_hashagg_tablesize(partial_aggregate_path,
> +
>  _costs,
> +
>  dNumPartialGroups);
> +
> + /*
> + * Generate a
> hashagg Path, if we can, but we'll skip this if the hash
> + * table looks like it'll exceed work_mem.
> +
> */
> + if (can_hash && hashaggtablesize < work_mem * 1024L)
>
>
> hash table size should be estimated only if can_hash is true.

Good point. Changed.

>
> 3.
> + foreach(lc, grouped_rel->partial_pathlist)
> + {
> + Path   *path =
> (Path *) lfirst(lc);
> + double total_groups;
> +
> + total_groups = path-
>>parallel_degree * path->rows;
> +
> + path = (Path *) create_gather_path(root, grouped_rel, path,
> NULL,
> +   _groups);
>
> Do you need to perform it foreach partial path or just do it for
> firstpartial path?

That's true. The order here does not matter since we're passing
directly into a Gather node, so it's wasteful to consider anything
apart from the cheapest path. -- Fixed.

There was also a missing hash table size check on the Finalize
HashAggregate Path consideration. I've added that now.

Updated patch is attached. Thanks for the re-review.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


0001-Allow-aggregation-to-happen-in-parallel_2016-03-18a.patch
Description: Binary data

-- 
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] Odd system-column handling in postgres_fdw join pushdown patch

[Ashutosh Bapat]

(2) when any of
>> xmins, xmaxs, cmins, and cmaxs are requested, postgres_fdw gives up
>> pushing down foreign joins.  (We might be able to set appropriate
>> values
>> for them locally the same way as for tableoids, but I'm not sure it's
>> worth complicating the code.)  I think that would be probably OK,
>> because users wouldn't retrieve any such columns in practice.
>>
>
> In that patch you have set pushdown_safe to false for the base relation
>> fetching system columns. But pushdown_safe = false means that that
>> relation is not safe to push down. A base foreign relation is always
>> safe to push down, so should have pushdown_safe = true always. Instead,
>> I suggest having a separate boolean has_unshippable_syscols (or
>> something with similar name) in PgFdwRelationInfo, which is set to true
>> in such case. In foreign_join_ok, we return false (thus not pushing down
>> the join), if any of the joining relation has that attribute set. By
>> default this member is false.
>>
>
> Maybe I'm missing something, but why do you consider that base foreign
> tables need always be safe to push down?  IIUC, the pushdown_safe flag is
> used only for foreign_join_ok, so I think that a base foreign table needs
> not necessarily be safe to push down.
>
>
A base foreign table "always" fetches data from the foreign server, so it
"has to be" always safe to push down. pushdown_safe flag is designated to
tell whether the relation corresponding to PgFdwRelationInfo where this
flag is set is safe to push down.Right now it's only used for joins but in
future it would be used for any push down of higher operations. It seems
very much possible after the upper pathification changes. We can not have a
query sent to the foreign server for a relation, when pushdown_safe is
false for that. Your patch does that for foreign base relation which try to
fetch system columns.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: [HACKERS] Small patch: fix comments in contrib/pg_trgm/

[Aleksander Alekseev]

Here are a few more patches.

On Wed, 16 Mar 2016 18:11:04 +0300
Aleksander Alekseev  wrote:

> Typos for the most part.
> 



-- 
Best regards,
Aleksander Alekseev
http://eax.me/
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 3f013e3..c9e5270 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -258,7 +258,7 @@ timestamp_recv(PG_FUNCTION_ARGS)
  errmsg("timestamp cannot be NaN")));
 #endif
 
-	/* rangecheck: see if timestamp_out would like it */
+	/* range check: see if timestamp_out would like it */
 	if (TIMESTAMP_NOT_FINITE(timestamp))
 		 /* ok */ ;
 	else if (timestamp2tm(timestamp, NULL, tm, , NULL, NULL) != 0 ||
@@ -792,7 +792,7 @@ timestamptz_recv(PG_FUNCTION_ARGS)
 	timestamp = (TimestampTz) pq_getmsgfloat8(buf);
 #endif
 
-	/* rangecheck: see if timestamptz_out would like it */
+	/* range check: see if timestamptz_out would like it */
 	if (TIMESTAMP_NOT_FINITE(timestamp))
 		 /* ok */ ;
 	else if (timestamp2tm(timestamp, , tm, , NULL, NULL) != 0 ||
@@ -1435,7 +1435,7 @@ AdjustIntervalForTypmod(Interval *interval, int32 typmod)
 		else
 			elog(ERROR, "unrecognized interval typmod: %d", typmod);
 
-		/* Need to adjust subsecond precision? */
+		/* Need to adjust sub-second precision? */
 		if (precision != INTERVAL_FULL_PRECISION)
 		{
 			if (precision < 0 || precision > MAX_INTERVAL_PRECISION)
@@ -1635,7 +1635,7 @@ IntegerTimestampToTimestampTz(int64 timestamp)
  * Both inputs must be ordinary finite timestamps (in current usage,
  * they'll be results from GetCurrentTimestamp()).
  *
- * We expect start_time <= stop_time.  If not, we return zeroes; for current
+ * We expect start_time <= stop_time.  If not, we return zeros; for current
  * callers there is no need to be tense about which way division rounds on
  * negative inputs.
  */
@@ -2276,7 +2276,7 @@ timestamp_hash(PG_FUNCTION_ARGS)
 
 
 /*
- * Crosstype comparison functions for timestamp vs timestamptz
+ * Cross-type comparison functions for timestamp vs timestamptz
  */
 
 Datum
@@ -2678,7 +2678,7 @@ overlaps_timestamp(PG_FUNCTION_ARGS)
 	{
 		/*
 		 * For ts1 = ts2 the spec says te1 <> te2 OR te1 = te2, which is a
-		 * rather silly way of saying "true if both are nonnull, else null".
+		 * rather silly way of saying "true if both are non-null, else null".
 		 */
 		if (te1IsNull || te2IsNull)
 			PG_RETURN_NULL();
@@ -2996,7 +2996,7 @@ timestamp_pl_interval(PG_FUNCTION_ARGS)
 		(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
 		 errmsg("timestamp out of range")));
 
-			/* Add days by converting to and from julian */
+			/* Add days by converting to and from Julian */
 			julian = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday) + span->day;
 			j2date(julian, >tm_year, >tm_mon, >tm_mday);
 
@@ -3104,7 +3104,7 @@ timestamptz_pl_interval(PG_FUNCTION_ARGS)
 		(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
 		 errmsg("timestamp out of range")));
 
-			/* Add days by converting to and from julian */
+			/* Add days by converting to and from Julian */
 			julian = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday) + span->day;
 			j2date(julian, >tm_year, >tm_mon, >tm_mday);
 
@@ -3309,7 +3309,7 @@ interval_mul(PG_FUNCTION_ARGS)
 	/*
 	 * The above correctly handles the whole-number part of the month and day
 	 * products, but we have to do something with any fractional part
-	 * resulting when the factor is nonintegral.  We cascade the fractions
+	 * resulting when the factor is non-integral.  We cascade the fractions
 	 * down to lower units using the conversion factors DAYS_PER_MONTH and
 	 * SECS_PER_DAY.  Note we do NOT cascade up, since we are not forced to do
 	 * so by the representation.  The user can choose to cascade up later,
@@ -3319,7 +3319,7 @@ interval_mul(PG_FUNCTION_ARGS)
 	/*
 	 * Fractional months full days into days.
 	 *
-	 * Floating point calculation are inherently inprecise, so these
+	 * Floating point calculation are inherently imprecise, so these
 	 * calculations are crafted to produce the most reliable result possible.
 	 * TSROUND() is needed to more accurately produce whole numbers where
 	 * appropriate.
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a325943..9d60a45 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1634,7 +1634,7 @@ static struct config_bool ConfigureNamesBool[] =
 
 	{
 		{"syslog_sequence_numbers", PGC_SIGHUP, LOGGING_WHERE,
-			gettext_noop("Add sequence number to syslog messags to avoid duplicate suppression."),
+			gettext_noop("Add sequence number to syslog messages to avoid duplicate suppression."),
 			NULL
 		},
 		_sequence_numbers,
@@ -2681,7 +2681,7 @@ static struct config_int ConfigureNamesInt[] =
 
 	{
 		{"ssl_renegotiation_limit", PGC_USERSET, CONN_AUTH_SECURITY,
-			gettext_noop("SSL regenotiation is no longer supported; this can only be 0."),
+			gettext_noop("SSL renegotiation is no longer 

Re: [HACKERS] Make primnodes.h gender neutral

[Peter Geoghegan]

On Thu, Mar 17, 2016 at 4:46 PM, Tom Lane  wrote:
> Alvaro's original complaint that the sentences no longer agree as to
> person is on-point.

That's reasonable. Still, there are only a few existing instances of
gendered pronouns in the code, so fixing them carefully, without
losing anything important seems like a relatively straightforward
task.

-- 
Peter Geoghegan


-- 
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] Logical decoding slots can go backwards when used from SQL, docs are wrong

[Craig Ringer]

The first patch was incorrectly created on top of failover slots not HEAD.
Attached patch applies on HEAD.
From 87d839f8a2e78abb17fa985502fd5b66f0872b57 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Wed, 16 Mar 2016 15:45:16 +0800
Subject: [PATCH 1/2] Correct incorrect claim that slots output a change
 "exactly once"

Replication slots may actually emit a change more than once
if the master crashes before flushing the slot.

See
http://www.postgresql.org/message-id/camsr+ygsatrgqpcx9qx4eocizwsa27xjkeipsotaje8ofix...@mail.gmail.com
for details.
---
 doc/src/sgml/logicaldecoding.sgml | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/logicaldecoding.sgml b/doc/src/sgml/logicaldecoding.sgml
index e841348..78e3dba 100644
--- a/doc/src/sgml/logicaldecoding.sgml
+++ b/doc/src/sgml/logicaldecoding.sgml
@@ -12,7 +12,6 @@
 
   
Changes are sent out in streams identified by logical replication slots.
-   Each stream outputs each change exactly once.
   
 
   
@@ -204,8 +203,7 @@ $ pg_recvlogical -d postgres --slot test --drop-slot
  In the context of logical replication, a slot represents a stream of
  changes that can be replayed to a client in the order they were made on
  the origin server. Each slot streams a sequence of changes from a single
- database, sending each change exactly once (except when peeking forward
- in the stream).
+ database.
 
 
 
@@ -218,7 +216,17 @@ $ pg_recvlogical -d postgres --slot test --drop-slot
 
  A replication slot has an identifier that is unique across all databases
  in a PostgreSQL cluster. Slots persist
- independently of the connection using them and are crash-safe.
+ independently of the connection using them. Slot creation and drop is
+ crash-safe, and slots will never be corrupted by a crash.
+
+
+
+ A logical slot outputs each database change at least once. A slot will
+ usually only emit a change once, but recently-sent changes may be sent
+ again if the server server crashes and restarts. Clients should remember
+ the last LSN they saw when decoding and skip over any repeated data or
+ (when using the replication protocol) request that decoding start from
+ that LSN rather than letting the server determine the start point.
 
 
 
-- 
2.1.0

From fdfe91482d7dd28920db67067d77388ef3871165 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Wed, 16 Mar 2016 15:12:34 +0800
Subject: [PATCH 2/2] Dirty replication slots when confirm_lsn is changed

---
 src/backend/replication/logical/logical.c | 62 +--
 1 file changed, 42 insertions(+), 20 deletions(-)

diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 2e6d3f9..40db6ff 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -437,6 +437,7 @@ DecodingContextFindStartpoint(LogicalDecodingContext *ctx)
 	}
 
 	ctx->slot->data.confirmed_flush = ctx->reader->EndRecPtr;
+	ReplicationSlotMarkDirty();
 }
 
 /*
@@ -847,10 +848,15 @@ LogicalConfirmReceivedLocation(XLogRecPtr lsn)
 	{
 		bool		updated_xmin = false;
 		bool		updated_restart = false;
+		bool		updated_confirm = false;
 
 		SpinLockAcquire(>mutex);
 
-		MyReplicationSlot->data.confirmed_flush = lsn;
+		if (MyReplicationSlot->data.confirmed_flush != lsn)
+		{
+			MyReplicationSlot->data.confirmed_flush = lsn;
+			updated_confirm = true;
+		}
 
 		/* if were past the location required for bumping xmin, do so */
 		if (MyReplicationSlot->candidate_xmin_lsn != InvalidXLogRecPtr &&
@@ -888,34 +894,50 @@ LogicalConfirmReceivedLocation(XLogRecPtr lsn)
 
 		SpinLockRelease(>mutex);
 
-		/* first write new xmin to disk, so we know whats up after a crash */
-		if (updated_xmin || updated_restart)
+		if (updated_xmin || updated_restart || updated_confirm)
 		{
 			ReplicationSlotMarkDirty();
-			ReplicationSlotSave();
-			elog(DEBUG1, "updated xmin: %u restart: %u", updated_xmin, updated_restart);
-		}
 
-		/*
-		 * Now the new xmin is safely on disk, we can let the global value
-		 * advance. We do not take ProcArrayLock or similar since we only
-		 * advance xmin here and there's not much harm done by a concurrent
-		 * computation missing that.
-		 */
-		if (updated_xmin)
-		{
-			SpinLockAcquire(>mutex);
-			MyReplicationSlot->effective_catalog_xmin = MyReplicationSlot->data.catalog_xmin;
-			SpinLockRelease(>mutex);
+			/*
+			 * first write new xmin to disk, so we know whats up
+			 * after a crash.
+			 */
+			if (updated_xmin || updated_restart)
+			{
+ReplicationSlotSave();
+elog(DEBUG1, "updated xmin: %u restart: %u", updated_xmin, updated_restart);
+			}
 
-			ReplicationSlotsComputeRequiredXmin(false);
-			ReplicationSlotsComputeRequiredLSN();
+			/*
+			 * Now the new xmin is safely on disk, we can let the global value
+			 * advance. We do 

Re: [HACKERS] 2016-03 Commitfest

[Andreas Karlsson]

Hi,

The COPY RAW patch seems to have two entries in the commitfest.

https://commitfest.postgresql.org/9/223/ and 
https://commitfest.postgresql.org/9/547/


Are those about the same patch?

Andreas


--
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] logger process infinite loop

[Andres Freund]

Hi,

On 2016-03-18 21:59:01 -0700, Jeff Janes wrote:
> While testing some patches on my laptop, I noticed my knee getting
> uncomfortably warm.  It turns out I has accumulating deranged logging
> processes, needing kill -9 to get rid of them.
> 
> The culprit is:
> 
> commit c4901a1e03a7730e4471fd1143f1caf79695493d
> Author: Andres Freund 
> Date:   Fri Mar 18 11:43:59 2016 -0700
> 
> Only clear latch self-pipe/event if there is a pending notification.

God, whan an awfully stupid mistake/typo. Thanks for the report!

- Andres


-- 
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] fd.c doesn't remove files on a crash-restart

[Joshua D. Drake]

On 03/16/2016 11:05 AM, Tom Lane wrote:

"Joshua D. Drake"  writes:

Hello,
fd.c[1] will remove files from pgsql_tmp on a restart but not a
crash-restart per this comment:



/*
* NOTE: we could, but don't, call this during a post-backend-crash restart
* cycle.  The argument for not doing it is that someone might want to
examine
* the temp files for debugging purposes.  This does however mean that
* OpenTemporaryFile had better allow for collision with an existing temp
* file name.
*/



I understand that this is designed this way. I think it is a bad idea
because:


Possible compromise: remove files only in non-Assert builds?


Oh, that seems absolutely reasonable. If we are using assert builds it 
is because we are debugging something (or should be) anyway.


Sincerely,

JD



regards, tom lane




--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


--
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] fd.c doesn't remove files on a crash-restart

[Tom Lane]

Robert Haas  writes:
> On Wed, Mar 16, 2016 at 2:05 PM, Tom Lane  wrote:
>> Possible compromise: remove files only in non-Assert builds?

> That sorta seems like tying two things together that aren't obviously
> related.  I think building with --enable-cassert is support to enable
> debugging cross-checks, not change behavior.

Well, it's support to enable debugging, and I would classify not
destroying evidence as being debugging support.

regards, tom lane


-- 
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] GinPageIs* don't actually return a boolean

[Yury Zhuravlev]

Robert Haas wrote:

On Wed, Mar 2, 2016 at 9:48 PM, Peter Eisentraut  wrote:

On 2/11/16 9:30 PM, Michael Paquier wrote: ...


We need to decide what to do about this.  I disagree with Peter: I
think that regardless of stdbool, what we've got right now is sloppy
coding - bad style if nothing else.  Furthermore, I think that while C
lets you use any non-zero value to represent true, our bool type is
supposed to contain only one of those two values.  Therefore, I think
we should commit the full patch, back-patch it as far as somebody has
the energy for, and move on.  But regardless, this patch can't keep
sitting in the CommitFest - we either have to take it or reject it,
and soon.



I know that we are trying to do the right thing. But right now there is an 
error only in ginStepRight. Maybe now the fix this place, and we will think 
about "bool" then? The patch is attached (small and simple).


Thanks.


--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c
index 06ba9cb..30113d0 100644
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -162,8 +162,8 @@ ginStepRight(Buffer buffer, Relation index, int lockmode)
 {
Buffer  nextbuffer;
Pagepage = BufferGetPage(buffer);
-   boolisLeaf = GinPageIsLeaf(page);
-   boolisData = GinPageIsData(page);
+   uint8   isLeaf = GinPageIsLeaf(page);
+   uint8   isData = GinPageIsData(page);
BlockNumber blkno = GinPageGetOpaque(page)->rightlink;
 
nextbuffer = ReadBuffer(index, blkno);

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Re: Add generate_series(date,date) and generate_series(date,date,integer)

[David Steele]

On 3/10/16 1:24 PM, Corey Huinker wrote:


New patch for Alvaro's consideration.

Very minor changes since the last time, the explanations below are
literally longer than the changes:
- Rebased, though I don't think any of the files had changed in the
mean time
- Removed infinity checks/errors and the test cases to match
- Amended documentation to add 'days' after 'step' as suggested
- Did not add a period as suggested, to remain consistent with other
descriptions in the same sgml table
- Altered test case and documentation of 7 day step example so that
the generated dates do not land evenly on the end date. A reader
might incorrectly infer that the end date must be in the result set,
when it doesn't have to be.
- Removed unnecessary indentation that existed purely due to
following of other generate_series implementations


As far as I can see overall support is in favor of this patch although 
it is not overwhelming by any means.


I think in this case it comes down to a committer's judgement so I have 
marked this "ready for committer" and passed the buck on to Álvaro.


--
-David
da...@pgmasters.net


--
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] silent data loss with ext4 / all current versions

[Robert Haas]

On Thu, Mar 17, 2016 at 11:03 AM, Andres Freund  wrote:
> On 2016-03-17 23:05:42 +0900, Michael Paquier wrote:
>> > Are you working on a fix for pg_rewind? Let's go with initdb -S in a
>> > first iteration, then we can, if somebody is interest enough, work on
>> > making this nicer in master.
>>
>> I am really -1 for this approach. Wrapping initdb -S with
>> find_other_exec is intrusive in back-branches knowing that all the I/O
>> write operations manipulating file descriptors go through file_ops.c,
>> and we actually just need to fsync the target file in
>> close_target_file(), making the fix being a 7-line patch, and there is
>> no need to depend on another binary at all. The backup label file, as
>> well as the control file are using the same code path in file_ops.c...
>> And I like simple things :)
>
> This is a *much* more expensive approach though. Doing the fsync
> directly after modifying the file. One file by one file. Will usually
> result in each fsync blocking for a while.
>
> In comparison of doing a flush and then an fsync pass over the whole
> directory will usually only block seldomly. The flushes for all files
> can be combined into very few barrier operations.

Yeah, I'm pretty sure this was discussed when initdb -S went in.  I
think reusing that is a good idea.

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


-- 
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, WIP: OR-clause support for indexes

[Teodor Sigaev]

I also wonder whether the patch should add explanation of OR-clauses
handling into the READMEs in src/backend/access/*


Oops, will add shortly.


The patch would probably benefit from transforming it into a patch
series - one patch for the infrastructure shared by all the indexes,
then one patch per index type. That should make it easier to review, and
I seriously doubt we'd want to commit this in one huge chunk anyway.

Ok, will do it.


1) fields in BrinOpaque are not following the naming convention (all the
existing fields start with bo_)

fixed



2) there's plenty of places violating the usual code style (e.g. for
single-command if branches) - not a big deal for WIP patch, but needs to
get fixed eventually

hope, fixed



3) I wonder whether we really need both SK_OR and SK_AND, considering
they are mutually exclusive. Why not to assume SK_AND by default, and
only use SK_OR? If we really need them, perhaps an assert making sure
they are not set at the same time would be appropriate.

In short: possible ambiguity and increasing stack machine complexity.
Let we have follow expression in reversed polish notation (letters represent a 
condtion, | - OR, & - AND logical operation, ANDs are omitted):

a b c |

Is it ((a & b)| c) or (a & (b | c)) ?

Also, using both SK_ makes code more readable.



4) scanGetItem is a prime example of the "badly needs comments" issue,
particularly because the previous version of the function actually had
quite a lot of them while the new function has none.

Will add soon



5) scanGetItem() may end up using uninitialized 'cmp' - it only gets
initialized when (!leftFinished && !rightFinished), but then gets used
when either part of the condition evaluates to true. Probably should be

 if (!leftFinished || !rightFinished)
 cmp = ...

fixed



6) the code in nodeIndexscan.c should not include call to abort()

 {
 abort();
 elog(ERROR, "unsupported indexqual type: %d",
 (int) nodeTag(clause));
 }

fixed, just forgot to remove



7) I find it rather ugly that the paths are built by converting BitmapOr
paths. Firstly, it means indexes without amgetbitmap can't benefit from
this change. Maybe that's reasonable limitation, though?

I based on following thoughts:
1 code which tries to find OR-index path will be very similar to existing
  generate_or_bitmap code. Obviously, it should not be duplicated.
2 all existsing indexes have amgetbitmap method, only a few don't. amgetbitmap
  interface is simpler. Anyway, I can add an option for generate_or_bitmap
  to use any index, but, in current state it will just repeat all work.



But more importantly, this design already has a bunch of unintended
consequences. For example, the current code completely ignores
enable_indexscan setting, because it merely copies the costs from the
bitmap path.

I'd like to add separate enable_indexorscan


That's pretty dubious, I guess. So this code probably needs to be made
aware of enable_indexscan - right now it entirely ignores startup_cost
in convert_bitmap_path_to_index_clause(). But of course if there are
multiple IndexPaths, the  enable_indexscan=off will be included multiple
times.

9) This already breaks estimation for some reason. Consider this

...

So the OR-clause is estimated to match 0 rows, less than each of the
clauses independently. Needless to say that without the patch this works
just fine.

fixed



10) Also, this already breaks some regression tests, apparently because
it changes how 'width' is computed.

fixed too



So I think this way of building the index path from a BitmapOr path is
pretty much a dead-end.
I don't think so because separate code path to support OR-clause in index will 
significanlty duplicate BitmapOr generator.


Will send next version as soon as possible. Thank you for your attention!

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


index_or-3.patch.gz
Description: GNU Zip compressed data

-- 
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] GinPageIs* don't actually return a boolean

[Tom Lane]

Michael Paquier  writes:
> FWIW, when compiling with MS 2015 using the set of perl scripts I am
> not seeing this compilation error... We may want to understand first
> what kind of dependency is involved when doing the cmake build
> compared to what is done with src/tools/msvc.

That is strange ... unless maybe cmake is supplying a different set
of warning-enabling switches to the compiler?

regards, tom lane


-- 
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] insufficient qualification of some objects in dump files

[Michael Paquier]

On Fri, Mar 18, 2016 at 5:38 AM, Tom Lane  wrote:
> Given the lack of any other complaints about this, I'm okay with the
> approach as presented.  (I haven't read the patch in detail, though.)

FWIW, I am still of the opinion that the last patch sent by Peter is
in a pretty good shape.
-- 
Michael


-- 
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: function parse_ident

[Pavel Stehule]

Hi

2016-03-14 17:39 GMT+01:00 Teodor Sigaev :

> I afraid so I cannot to fix this inconsistency (if this is inconsistency -
>> the
>> binary values are same) - the parameter of function is raw string with
>> processed
>> escape codes, and I have not any information about original escape
>> sequences.
>> When you enter octet value, and I show it as hex value, then there should
>> be
>> difference. Buy I have not information about your input (octet or hex). I
>> have
>> the original string of SQL identifier inside parser, executor, but I have
>> not
>> original string of function parameter inside function (not without pretty
>> complex and long code).
>>
> Ok, agree
>
>
>> I am trying describe it in doc (I am sorry for my less level English) in
>> new
>> patch. Fixed duplicated oid too.
>>
> Edited a bit + fix some typos and remove unneeded headers, patch attached
>
> Sorry, I can't find all corner-cases at once, but:
> SELECT parse_ident(E'"c".X XX');
> ERROR:  identifier contains disallowed characters: "\"c"
>
> Error message wrongly points to the reason of error.
>
>
I forgot my original plan - show full original string. Now, complete
original parameter is used as part of message everywhere. It is more
consistent.

I cannot to reuse escape_json - it escape double quotes, and then the
paremeter in message looks strange.

I hope so the messages are ok now. Few more regress tests added.

Thank you for your patience.

Regards

Pavel



>
>
>
>
> --
> Teodor Sigaev   E-mail: teo...@sigaev.ru
>WWW:
> http://www.sigaev.ru/
>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
new file mode 100644
index 000489d..918356c
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
***
*** 1821,1826 
--- 1821,1852 

 
  
+  parse_ident
+ 
+ parse_ident(str text,
+[ strictmode boolean DEFAULT true ] )
+
+text[]
+Split qualified identifier into array
+parts. When strictmode is
+false, extra characters after the identifier are ignored. This is useful
+for parsing identifiers for objects like functions and arrays that may
+have trailing characters. By default, extra characters after the last
+identifier are considered an error, but if second parameter is false,
+then chararacters after last identifier are ignored. Note that this
+function does not truncate quoted identifiers. If you care about that
+you should cast the result of this function to name[]. A non-printable
+chararacters (like 0 to 31) are displayed as hexadecimal codes always,
+what can be different from PostgreSQL internal SQL identifiers
+processing, when the original escaped value is displayed.
+
+parse_ident('"SomeSchema".someTable')
+"SomeSchema,sometable"
+   
+ 
+   
+
+ 
   pg_client_encoding
  
  pg_client_encoding()
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
new file mode 100644
index fef67bd..9ae1ef4
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*** RETURNS jsonb
*** 990,992 
--- 990,999 
  LANGUAGE INTERNAL
  STRICT IMMUTABLE
  AS 'jsonb_set';
+ 
+ CREATE OR REPLACE FUNCTION
+   parse_ident(str text, strict boolean DEFAULT true)
+ RETURNS text[]
+ LANGUAGE INTERNAL
+ STRICT IMMUTABLE
+ AS 'parse_ident';
diff --git a/src/backend/parser/scansup.c b/src/backend/parser/scansup.c
new file mode 100644
index 2b4ab20..7aa5b76
*** a/src/backend/parser/scansup.c
--- b/src/backend/parser/scansup.c
*** scanstr(const char *s)
*** 130,135 
--- 130,144 
  char *
  downcase_truncate_identifier(const char *ident, int len, bool warn)
  {
+ 	return downcase_identifier(ident, len, warn, true);
+ }
+ 
+ /*
+  * a workhorse for downcase_truncate_identifier
+  */
+ char *
+ downcase_identifier(const char *ident, int len, bool warn, bool truncate)
+ {
  	char	   *result;
  	int			i;
  	bool		enc_is_single_byte;
*** downcase_truncate_identifier(const char
*** 158,169 
  	}
  	result[i] = '\0';
  
! 	if (i >= NAMEDATALEN)
  		truncate_identifier(result, i, warn);
  
  	return result;
  }
  
  /*
   * truncate_identifier() --- truncate an identifier to NAMEDATALEN-1 bytes.
   *
--- 167,179 
  	}
  	result[i] = '\0';
  
! 	if (i >= NAMEDATALEN && truncate)
  		truncate_identifier(result, i, warn);
  
  	return result;
  }
  
+ 
  /*
   * truncate_identifier() --- truncate an identifier to NAMEDATALEN-1 bytes.
   *
diff --git a/src/backend/utils/adt/misc.c b/src/backend/utils/adt/misc.c
new file mode 100644
index 43f36db..8917b1e
*** a/src/backend/utils/adt/misc.c
--- b/src/backend/utils/adt/misc.c
***
*** 27,32 
--- 27,33 
  #include 

Re: [HACKERS] Performance degradation in commit ac1d794

[Robert Haas]

On Thu, Mar 17, 2016 at 11:17 AM, Andres Freund  wrote:
> Right now, do use a WaitEventSet you'd do something like
> WaitEvent   event;
>
> ModifyWaitEvent(FeBeWaitSet, 0, waitfor, NULL);
>
> WaitEventSetWait(FeBeWaitSet, 0 /* no timeout */, , 1);
>
> i.e. use a WaitEvent on the stack to receive the changes. If you wanted
> to get more changes than just one, you could end up allocating a fair
> bit of stack space.
>
> We could instead allocate the returned events as part of the event set,
> and return them. Either by returning a NULL terminated array, or by
> continuing to return the number of events as now, and additionally
> return the event data structure via a pointer.
>
> So the above would be
>
> WaitEvent   *events;
> int nevents;
>
> ModifyWaitEvent(FeBeWaitSet, 0, waitfor, NULL);
>
> nevents = WaitEventSetWait(FeBeWaitSet, 0 /* no timeout */, 
> events, 10);
>
> for (int off = 0; off <= nevents; nevents++)
> ; // stuff

I don't see this as being particularly better.

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


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

[Julien Rouhaud]

On 16/03/2016 18:42, Robert Haas wrote:
> On Wed, Mar 16, 2016 at 1:23 PM, Julien Rouhaud
>  wrote:
>> On 16/03/2016 17:55, Robert Haas wrote:
>>> On Wed, Mar 16, 2016 at 12:47 PM, Julien Rouhaud
>>>  wrote:
 Something like a "min_parallel_degree" then ?
>>>
>>> Why not just parallel_degree without any prefix?  As in, when scanning
>>> this table in parallel, the reloption suggests using N workers.
>>>
>>
>> Agreed.
>>
>> PFA v2 that implements that.
> 
> I think create_parallel_paths shouldn't actually run the loop if the
> reloption is specified; it should just adopt the specified value (or
> max_parallel_degree, whichever is less).  Right now, you have it doing
> the work to compute the default value but then overriding it.
> 

Oh ugly mistake. Fixed.

> Also, I think parallel_degree should be down in the section that says
> /* information about a base rel (not set for join rels!) */ and I
> think it should be called something like rel_parallel_degree, to make
> it more clear that it's a value set on the relation level.
> 

You're right, fixed.

> Let's leave out the parallel_threshold stuff for now.
> 

attached v3 drops the GUC part.

-- 
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..80e1f09 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)
+
+ 
+  Number of workers wanted for this table. The number of worker will be
+  limited according to the 
+  parameter.
+ 
+
+   
+
+   
 autovacuum_enabled, toast.autovacuum_enabled (boolean)
 
  
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index ea0755a..68bb133 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -267,6 +267,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, 1, INT_MAX
+	},
 
 	/* list terminator */
 	{{NULL}}
@@ -1291,7 +1300,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, );
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 4f60b85..617872d 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -673,17 +673,26 @@ create_parallel_paths(PlannerInfo *root, RelOptInfo *rel)
 		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 &&
+			   parallel_degree < max_parallel_degree)
+		{
+			parallel_degree++;
+			parallel_threshold *= 3;
+			if (parallel_threshold >= PG_INT32_MAX / 3)
+break;
+		}
 	}
 
 	/* Add an unordered partial path based on a parallel sequential scan. */
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index ad715bb..709d192 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -128,6 +128,8 @@ get_relation_info(PlannerInfo *root, Oid relationObjectId, bool inhparent,
 		estimate_rel_size(relation, rel->attr_widths - rel->min_attr,
 		  >pages, >tuples, >allvisfrac);
 
+	/* Setup the per-relation parallel_degree */
+	 rel->rel_parallel_degree = RelationGetParallelDegree(relation, -1);
 	/*
 	 * Make list of indexes.  Ignore indexes on system catalogs if told to.
 	 * Don't bother with indexes for an inheritance parent, either.

Re: [HACKERS] Patch: Implement failover on libpq connect level.

[David Steele]

On 3/10/16 7:34 AM, Simon Riggs wrote:

On 9 March 2016 at 23:11, David Steele > wrote:

There hasn't been any movement on this patch in a while.  Will you
have a new tests ready for review soon?


I see the value in this feature, but the patch itself needs work and
probably some slimming down/reality and a good spellcheck.

If someone takes this on soon it can go into 9.6, otherwise I vote to
reject this early to avoid wasting review time.


Since there has been no response from the author I have marked this 
patch "returned with feedback".  Please feel free to resubmit for 9.7!


--
-David
da...@pgmasters.net


--
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] [PATH] Jsonb, insert a new value into an array at arbitrary position

[Dmitry Dolgov]

Hi Vitaly, thanks for the review. I've attached a new version of path with
improvements. Few notes:

> 7. Why did you remove "skip"? It is a comment what "true" means...

Actually, I thought that this comment was about skipping an element from
jsonb in order to change/delete it,
not about the last argument.  E.g. you can find several occurrences of
`JsonbIteratorNext` with `true` as the last
argument but without a "last argument is about skip" comment.
And there is a piece of code in the function `jsonb_delete` with a "skip
element" commentary:

```
/* skip corresponding value as well */
if (r == WJB_KEY)
JsonbIteratorNext(, , true);
```

So since in this patch it's not a simple skipping for setPathArray, I
removed that commentary. Am I wrong?

> 9. And finally... it does not work as expected in case of:

Yes, good catch, thanks.
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index c0b94bc..158e7fb 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -10791,6 +10791,9 @@ table2-mapping
jsonb_set
   
   
+   jsonb_insert
+  
+  
jsonb_pretty
   
 
@@ -11072,6 +11075,39 @@ table2-mapping
 

   
+   
+   
+   jsonb_insert(target jsonb, path text[], new_value jsonb, after boolean)
+   
+   
+   jsonb
+   
+ Returns target with
+ new_value inserted.
+ Iftarget section designated by
+ path is a JSONB array,
+ new_value will be inserted before it, or
+ after if after is true (defailt is
+ false).  If target section
+ designated by path is a JSONB object,
+ new_value will be added just like a regular
+ key.  As with the path orientated operators, negative integers that
+ appear in path count from the end of JSON
+ arrays.
+   
+   
+   
+   jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"')
+   
+   
+   jsonb_insert('{"a": [0,1,2]}', '{a, 1}', '"new_value"', true)
+   
+   
+   {"a": [0, "new_value", 1, 2]}
+ {"a": [0, 1, "new_value", 2]}
+
+   
+  
jsonb_pretty(from_json jsonb)
  
text
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index abf9a70..b1281e7 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -971,3 +971,11 @@ RETURNS jsonb
 LANGUAGE INTERNAL
 STRICT IMMUTABLE
 AS 'jsonb_set';
+
+CREATE OR REPLACE FUNCTION
+  jsonb_insert(jsonb_in jsonb, path text[] , replacement jsonb,
+insert_before_after boolean DEFAULT false)
+RETURNS jsonb
+LANGUAGE INTERNAL
+STRICT IMMUTABLE
+AS 'jsonb_insert';
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 88225aa..1c1da7c 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -33,6 +33,13 @@
 #include "utils/memutils.h"
 #include "utils/typcache.h"
 
+/* Operations available for setPath */
+#define JB_PATH_NOOP	0x0
+#define JB_PATH_CREATE	0x0001
+#define JB_PATH_DELETE	0x0002
+#define JB_PATH_INSERT_BEFORE			0x0004
+#define JB_PATH_INSERT_AFTER			0x0008
+
 /* semantic action functions for json_object_keys */
 static void okeys_object_field_start(void *state, char *fname, bool isnull);
 static void okeys_array_start(void *state);
@@ -130,14 +137,14 @@ static JsonbValue *IteratorConcat(JsonbIterator **it1, JsonbIterator **it2,
 static JsonbValue *setPath(JsonbIterator **it, Datum *path_elems,
 		bool *path_nulls, int path_len,
 		JsonbParseState **st, int level, Jsonb *newval,
-		bool create);
+		int op_type);
 static void setPathObject(JsonbIterator **it, Datum *path_elems,
 			  bool *path_nulls, int path_len, JsonbParseState **st,
 			  int level,
-			  Jsonb *newval, uint32 npairs, bool create);
+			  Jsonb *newval, uint32 npairs, int op_type);
 static void setPathArray(JsonbIterator **it, Datum *path_elems,
 			 bool *path_nulls, int path_len, JsonbParseState **st,
-			 int level, Jsonb *newval, uint32 nelems, bool create);
+			 int level, Jsonb *newval, uint32 nelems, int op_type);
 static void addJsonbToParseState(JsonbParseState **jbps, Jsonb *jb);
 
 /* state for json_object_keys */
@@ -3544,7 +3551,7 @@ jsonb_set(PG_FUNCTION_ARGS)
 	it = JsonbIteratorInit(>root);
 
 	res = setPath(, path_elems, path_nulls, path_len, ,
-  0, newval, create);
+  0, newval, create ? JB_PATH_CREATE : JB_PATH_NOOP);
 
 	Assert(res != NULL);
 
@@ -3588,7 +3595,52 @@ jsonb_delete_path(PG_FUNCTION_ARGS)
 
 	it = JsonbIteratorInit(>root);
 
-	res = setPath(, path_elems, path_nulls, path_len, , 0, NULL, false);
+	res = setPath(, path_elems, path_nulls, path_len, ,
+  0, NULL, JB_PATH_DELETE);
+
+	Assert(res != NULL);
+
+	PG_RETURN_JSONB(JsonbValueToJsonb(res));
+}
+
+/*
+ * SQL function jsonb_insert(jsonb, text[], jsonb, boolean)
+ *
+ */
+Datum
+jsonb_insert(PG_FUNCTION_ARGS)
+{
+	

Re: [HACKERS] Make primnodes.h gender neutral

[Kevin Grittner]

On Thu, Mar 17, 2016 at 6:07 PM, Tom Lane  wrote:
> Chapman Flack  writes:
>> On 03/17/16 17:29, Kevin Grittner wrote:
>>> A grep with a quick skim of the results to exclude references to
>>> particular people who are mentioned by name and then referred to
>>> with a pronoun (which I assume we can leave alone), suggest there
>>> are about 70 lines in the 1346667 line C code base that need work.
>>>
>>> Any word-smiths out there who want to volunteer to sort this out?
>
>> So that must be N affected files for some N <= 70 ...
>
>> what would you think of starting a wiki page with those N filenames
>> (so nobody has to repeat your grepping/skimming effort), and volunteers
>> can claim a file or five, marking them taken on that page, and wordsmith
>> away?
>
> Yeah, Kevin, could you post your results?  I'd have guessed there were
> more trouble spots than that.  If that really is the size of the problem,
> seems like we could fix all those instances in one patch and be done
> with it.  (At least till new ones sneak in :-()

I see others have also been scanning the sgml sources, while I was
just looking at .c and .h files.  FWIW, I've attached grep results
based on what I used for my initial count, which was just scanning
for the six gender-specific pronouns that came to mind at the time
-- I probably missed some, but this should be the bulk of it I
think.  This time I included a few lines around each pronoun for
context; hopefully that makes it easier for the word-smiths.  I
removed results where "he" was used as a local variable name for a
HeapEntry and those places where the pronoun referred back to a
person (e.g., Knuth) who was mentioned just above.

Note that since multiple lines with gender-specific pronouns
sometimes are near each other and thus show up in the same block,
there are 59 blocks in 42 files.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
kgrittn@Kevin-Desktop:~/pg/master$ find -type f -name '*.[hc]' | grep -v 
'/tmp_check/' | grep -v '/Debug/' | xargs egrep -i -B3 -A3 
'\b(him|her|his|hers|he|she)\b'
./src/include/nodes/primnodes.h- *
./src/include/nodes/primnodes.h- * isNatural, usingClause, and quals are 
interdependent.  The user can write
./src/include/nodes/primnodes.h- * only one of NATURAL, USING(), or ON() (this 
is enforced by the grammar).
./src/include/nodes/primnodes.h: * If he writes NATURAL then parse analysis 
generates the equivalent USING()
./src/include/nodes/primnodes.h- * list, and from that fills in "quals" with 
the right equality comparisons.
./src/include/nodes/primnodes.h: * If he writes USING() then "quals" is filled 
with equality comparisons.
./src/include/nodes/primnodes.h: * If he writes ON() then only "quals" is set.  
Note that NATURAL/USING
./src/include/nodes/primnodes.h- * are not equivalent to ON() since they also 
affect the output column list.
./src/include/nodes/primnodes.h- *
./src/include/nodes/primnodes.h- * alias is an Alias node representing the AS 
alias-clause attached to the
--
./src/include/c.h-/*
./src/include/c.h- * MemSetAligned is the same as MemSet except it omits the 
test to see if
./src/include/c.h- * "start" is word-aligned.  This is okay to use if the 
caller knows a-priori
./src/include/c.h: * that the pointer is suitably aligned (typically, because 
he just got it
./src/include/c.h- * from palloc(), which always delivers a max-aligned 
pointer).
./src/include/c.h- */
./src/include/c.h-#define MemSetAligned(start, val, len) \
--
./src/bin/pg_dump/pg_backup_directory.c-typedef struct
./src/bin/pg_dump/pg_backup_directory.c-{
./src/bin/pg_dump/pg_backup_directory.c-/*
./src/bin/pg_dump/pg_backup_directory.c: * Our archive location. This 
is basically what the user specified as his
./src/bin/pg_dump/pg_backup_directory.c- * backup file but of course 
here it is a directory.
./src/bin/pg_dump/pg_backup_directory.c- */
./src/bin/pg_dump/pg_backup_directory.c-char   *directory;
--
./src/backend/nodes/makefuncs.c-tle->resname = resname;
./src/backend/nodes/makefuncs.c-
./src/backend/nodes/makefuncs.c-/*
./src/backend/nodes/makefuncs.c: * We always set these fields to 0. If 
the caller wants to change them he
./src/backend/nodes/makefuncs.c- * must do so explicitly.  Few callers 
do that, so omitting these
./src/backend/nodes/makefuncs.c- * arguments reduces the chance of 
error.
./src/backend/nodes/makefuncs.c- */
--
./src/backend/parser/parse_clause.c- *we may as well accept this 
common extension.
./src/backend/parser/parse_clause.c- *
./src/backend/parser/parse_clause.c- * Note that pre-existing resjunk 
targets must not be used in either case,
./src/backend/parser/parse_clause.c: * since the user didn't write them in 
his SELECT list.
./src/backend/parser/parse_clause.c- *
./src/backend/parser/parse_clause.c- * If neither special 

Re: [HACKERS] [PATCH] Integer overflow in timestamp[tz]_part() and date/time boundaries check

[Tom Lane]

Vitaly Burovoy  writes:
> Is there any reason to leave JULIAN_MINDAY and JULIAN_MAXDAY which are
> not used now?

Those are just there to document what the limits really are.  Possibly
some code would need them in future.

> Also why JULIAN_MAXMONTH is set to "6" whereas
> {DATE|TIMESTAMP}_END_JULIAN use "1" as month?

Because we're intentionally allowing a wider range for IS_VALID_JULIAN
than for IS_VALID_DATE/TIMESTAMP.

regards, tom lane


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

[Robert Haas]

On Wed, Mar 16, 2016 at 1:38 AM, Amit Langote  wrote:

> Attached fixes a minor typo as follows:
>
> s/index vacuums cycles/index vacuum cycles/g
>

Committed, thanks.

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

Re: [HACKERS] pg_hba_lookup function to get all matching pg_hba.conf entries

[Haribabu Kommi]

On Thu, Mar 17, 2016 at 6:56 PM, Shulgin, Oleksandr
 wrote:
> On Thu, Mar 17, 2016 at 2:12 AM, Haribabu Kommi 
> wrote:
>>
>> On Wed, Mar 16, 2016 at 9:49 PM, Shulgin, Oleksandr
>>  wrote:
>> >
>> > Some comments:
>> >
>> > +/* Context to use with hba_line_callback function. */
>> > +typedef struct
>> > +{
>> > +   MemoryContext memcxt;
>> > +   TupleDesc   tupdesc;
>> > +   Tuplestorestate *tuple_store;
>> > +}  HbalineContext;
>> >
>> > Rather "with *lookup_hba_line_callback*", as hba_line_callback() is a
>> > generic one.
>>
>> Fine. I will change the function and context names.
>
>
> You mean change context name and correct the comment?  I didn't suggest to
> change the function name.

Changed the context name and the comment only.

>> > Still remains an issue of representing special keywords in database and
>> > user_name fields, but there was no consensus about that though.
>>
>> How about adding keyword_database and keyword_user columns to listing
>> out the keywords.  These columns will be populated only when the hba line
>> contains any keywords.
>
>
> Hm... that could work too.

Here I attached patch with the added two keyword columns.
During the testing with different IP comparison methods like 'samehost' or
'samenet', the address details are not displayed. Is there any need of
showing the IP compare method also?

Regards,
Hari Babu
Fujitsu Australia


pg-hba-lookup-18-03-2016.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] logger process infinite loop

[Jeff Janes]

While testing some patches on my laptop, I noticed my knee getting
uncomfortably warm.  It turns out I has accumulating deranged logging
processes, needing kill -9 to get rid of them.

The culprit is:

commit c4901a1e03a7730e4471fd1143f1caf79695493d
Author: Andres Freund 
Date:   Fri Mar 18 11:43:59 2016 -0700

Only clear latch self-pipe/event if there is a pending notification.


All I need to do to reproduce it is turn logging_collector = on,
then:
pg_ctl start
pg_ctl stop


A strace of a logger process shows:

poll([{fd=10, events=POLLIN}, {fd=7, events=POLLIN}], 2, 9771863) = 1
([{fd=7, revents=POLLHUP}])
poll([{fd=10, events=POLLIN}, {fd=7, events=POLLIN}], 2, 9771863) = 1
([{fd=7, revents=POLLHUP}])
poll([{fd=10, events=POLLIN}, {fd=7, events=POLLIN}], 2, 9771863) = 1
([{fd=7, revents=POLLHUP}])
poll([{fd=10, events=POLLIN}, {fd=7, events=POLLIN}], 2, 9771863) = 1
([{fd=7, revents=POLLHUP}])
poll([{fd=10, events=POLLIN}, {fd=7, events=POLLIN}], 2, 9771863) = 1
([{fd=7, revents=POLLHUP}])
poll([{fd=10, events=POLLIN}, {fd=7, events=POLLIN}], 2, 9771863) = 1
([{fd=7, revents=POLLHUP}])
poll([{fd=10, events=POLLIN}, {fd=7, events=POLLIN}], 2, 9771862) = 1
([{fd=7, revents=POLLHUP}])
poll([{fd=10, events=POLLIN}, {fd=7, events=POLLIN}], 2, 9771862) = 1
([{fd=7, revents=POLLHUP}])
poll([{fd=10, events=POLLIN}, {fd=7, events=POLLIN}], 2, 9771862) = 1
([{fd=7, revents=POLLHUP}])
poll([{fd=10, events=POLLIN}, {fd=7, events=POLLIN}], 2, 9771862) = 1
([{fd=7, revents=POLLHUP}])

And a backtrace shows:

(gdb) bt full
#0  0x7fce93f90050 in __poll_nocancel () from /lib64/libc.so.6
No symbol table info available.
#1  0x0065a52a in WaitLatchOrSocket (latch=0xc179c4
, wakeEvents=11, sock=7, timeout=7259000) at
pg_latch.c:350
result = 0
rc = 
start_time = {tv_sec = 1458363541, tv_usec = 583339}
cur_time = {tv_sec = 19, tv_usec = 795497}
cur_timeout = 7239205
pfds = {{fd = 10, events = 1, revents = 0}, {fd = 7, events =
1, revents = 16}, {fd = 222842297, events = 32766, revents = 0}}
nfds = 2
__func__ = "WaitLatchOrSocket"
#2  0x0066b64f in SysLoggerMain (argv=,
argc=) at syslogger.c:424
time_based_rotation = 
size_rotation_for = 
cur_timeout = 
rc = 
logbuffer = "\000\000K\000\362\021\000\000t4594  0
2016-03-18 21:59:01.583 PDT LOG:  database system is shut
down\nbuffers (0.0%); 0 transaction log file(s) added, 0 removed, 0
recycled; write=0.000 s, sync=0.000 s, total=0.001 s; "...
bytes_in_logbuffer = 0
currentLogDir = 0x26ef130 "pg_log"
currentLogFilename = 0x26ef200 "postgresql-%Y-%m-%d_%H%M%S.log"
currentLogRotationAge = 1440
now = 1458363541
#3  0x0066bdfb in SysLogger_Start () at syslogger.c:600
sysloggerPid = 0
filename = 0x0
__func__ = "SysLogger_Start"
#4  0x0066a878 in PostmasterMain (argc=argc@entry=1,
argv=argv@entry=0x26d5a10) at postmaster.c:1208
opt = 
status = 
userDoption = 
listen_addr_saved = 
i = 
output_config_variable = 
__func__ = "PostmasterMain"
#5  0x0047cd1e in main (argc=1, argv=0x26d5a10) at main.c:228

Cheers,

Jeff


-- 
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] WIP: Covering + unique indexes.

[Peter Geoghegan]

On Fri, Mar 18, 2016 at 5:15 AM, David Steele  wrote:
> It looks like this patch should be marked "needs review" and I have done so.

Uh, no it shouldn't. I've posted an extensive review on the original
design thread. See CF entry:

https://commitfest.postgresql.org/9/433/

Marked "Waiting on Author".

-- 
Peter Geoghegan


-- 
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] Speedup twophase transactions

[Stas Kelvich]

> On 11 Mar 2016, at 19:41, Jesper Pedersen  wrote:
> 

Thanks for review, Jesper.

> Some comments:
> 
> * The patch needs a rebase against the latest TwoPhaseFileHeader change

Done.

> * Rework the check.sh script into a TAP test case (src/test/recovery), as 
> suggested by Alvaro and Michael down thread

Done. Originally I thought about reducing number of tests (11 right now), but 
now, after some debugging, I’m more convinced that it is better to include them 
all, as they are really testing different code paths.

> * Add documentation for RecoverPreparedFromXLOG

Done.

---
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company



twophase_replay.v2.diff
Description: Binary data

-- 
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, WIP: OR-clause support for indexes

[Andreas Karlsson]

I gave this patch a quick spin and noticed a strange query plan.

CREATE TABLE test (a int, b int, c int);
CREATE INDEX ON test USING gin (a, b, c);
INSERT INTO test SELECT i % 7, i % 9, i % 11 FROM generate_series(1, 
100) i;

EXPLAIN ANALYZE SELECT * FROM test WHERE (a = 3 OR b = 5) AND c = 2;

QUERY PLAN 


--
 Bitmap Heap Scan on test  (cost=829.45..4892.10 rows=21819 width=12) 
(actual time=66.494..76.234 rows=21645 loops=1)
   Recheck Cond: a = 3) AND (c = 2)) OR ((b = 5) AND (c = 2))) AND 
(c = 2))

   Heap Blocks: exact=5406
   ->  Bitmap Index Scan on test_a_b_c_idx  (cost=0.00..824.00 
rows=2100 width=0) (actual time=65.272..65.272 rows=21645 loops=1)
 Index Cond: a = 3) AND (c = 2)) OR ((b = 5) AND (c = 2))) 
AND (c = 2))

 Planning time: 0.200 ms
 Execution time: 77.206 ms
(7 rows)

Shouldn't the index condition just be "((a = 3) AND (c = 2)) OR ((b = 5) 
AND (c = 2))"?


Also when applying and reading the patch I noticed some minor 
issues/nitpick.


- I get whitespace warnings from git apply when I apply the patches.
- You have any insconstent style for casts: I think "(Node*)clause" 
should be "(Node *) clause".

- Same with pointers. "List* quals" should be "List *quals"
- I am personally not a fan of seeing the "isorderby == false && 
index->rd_amroutine->amcanorclause" clause twice. Feels like a risk for 
diverging code paths. But it could be that there is no clean alternative.


Andreas


--
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] [WIP] speeding up GIN build with parallel workers

[Dmitry Ivanov]

Hi Constantin,

I did a quick review of your patch, and here are my comments:

- This patch applies cleanly to the current HEAD (61d2ebdbf91).

- Code compiles without warnings.

- Currently there's no documentation regarding parallel gin build feature and 
provided GUC variables.

- Built indexes work seem to work normally.


Performance
---

I've made a few runs on my laptop (i7-4710HQ, default postgresql.conf), here 
are the results:

workers avg. time (s)
0   412
4   133
8   81

Looks like 8 workers & a backend do the job 5x times faster than a sole 
backend, which is good!


Code style
--

There are some things that you've probably overlooked, such as:

> task->heap_oid = heap->rd_id;
> task->index_oid = index->rd_id;

You could replace direct access to 'rd_id' field with the RelationGetRelid 
macro.

> static void ginDumpEntry(GinState *ginstate,
>volatile WorkerResult *r

Parameter 'r' is unused, you could remove it.

Some of the functions and pieces of code that you've added do not comply to 
the formatting conventions, e. g.

> static int claimSomeBlocks(...
> static GinEntryStack *pushEntry(

>> // The message consists of 2 or 3 parts. iovec allows us to send them as

etc.

Keep up the good work!


-- 
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] Idle In Transaction Session Timeout, revived

[Jeff Janes]

On Wed, Mar 16, 2016 at 8:32 AM, Robert Haas  wrote:
>
> Committed with slight changes to the docs, and I added a flag variable
> instead of relying on IdleInTransactionSessionTimeout not changing at
> an inopportune time.

Thanks for committing, this should be a useful feature.

I get a pretty strange error message:


jjanes=# set idle_in_transaction_session_timeout = "1s";
jjanes=# begin;
-- wait for more than 1 second.
jjanes=# select count(*) from pgbench_accounts;
FATAL:  terminating connection due to idle-in-transaction timeout
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.


First it tells me exactly why the connection was killed, then it tells
me it doesn't know why the connection was killed and probably the
server has crashed.

I can't find the spot in the code where the FATAL message is getting
printed.  I suppose it will not be easy to pass a
"dont_plead_ignorance" flag over to the part that prints the follow-up
message?



>
> Thanks.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


-- 
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] Make primnodes.h gender neutral

[Tom Lane]

Peter Geoghegan  writes:
> (In case it matters, I'm in favor of this proposal on its merits).

For the record, I'm also in favor of fixing that para, but I'd like
to see attention paid to grammatical correctness as well as political.
Alvaro's original complaint that the sentences no longer agree as to
person is on-point.

regards, tom lane


-- 
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] Using quicksort for every external sort run

[Robert Haas]

On Wed, Mar 16, 2016 at 9:42 PM, Peter Geoghegan  wrote:
>> - I haven't yet figured out why we use batching only for the final
>> on-the-fly merge pass, instead of doing it for all merges.  I expect
>> you have a reason.  I just don't know what it is.
>
> The most obvious reason, and possibly the only reason, is that I have
> license to lock down memory accounting in the final on-the-fly merge
> phase. Almost equi-sized runs are the norm, and code like this is no
> longer obligated to work:
>
> FREEMEM(state, GetMemoryChunkSpace(stup->tuple));
>
> That's why I explicitly give up on "conventional accounting". USEMEM()
> and FREEMEM() calls become unnecessary for this case that is well
> locked down. Oh, and I know that I won't use most tapes, so I can give
> myself a FREEMEM() refund before doing the new grow_memtuples() thing.
>
> I want to make batch memory usable for runs, too. I haven't done that
> either for similar reasons. FWIW, I see no great reason to worry about
> non-final merges.

Fair enough.  My concern was mostly whether the code would become
simpler if we always did this when merging, instead of only on the
final merge.  But the final merge seems to be special in quite a few
respects, so maybe not.

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


-- 
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] plpgsql - DECLARE - cannot to use %TYPE or %ROWTYPE for composite types

[Pavel Stehule]

2016-03-16 16:50 GMT+01:00 Pavel Stehule :

>
>
> 2016-03-16 16:46 GMT+01:00 Joe Conway :
>
>> On 03/15/2016 05:17 PM, Tom Lane wrote:
>> > In short, I think we should reject this implementation and instead try
>> > to implement the type operators we want in the core grammar's Typename
>> > production, from which plpgsql will pick it up automatically.  That is
>> > going to require some other syntax than this.  As I said, I'm not
>> > particularly pushing the function-like syntax I wrote upthread; but
>> > I want to see something that is capable of supporting all those features
>> > and can be extended later if we think of other type operators we want.
>>
>> +1
>>
>> Anyone want to argue against changing the status of this to Rejected or
>> at least Returned with feedback?
>>
>
> I would to reduce this patch to fix row type issue. There is not any
> disagreement. I'll send reduced patch today.
>
> Any other functionality is not 9.6 topic.
>

I played with the reduced patch, and the benefit without all other things
is negligible. It should be rejected.

Regards

Pavel


>
> Regards
>
> Pavel
>
>
>> Joe
>>
>> --
>> Crunchy Data - http://crunchydata.com
>> PostgreSQL Support for Secure Enterprises
>> Consulting, Training, & Open Source Development
>>
>>
>

Re: [HACKERS] Proposal: Generic WAL logical messages

[Craig Ringer]

On 17 March 2016 at 19:08, Artur Zakirov  wrote:

> On 16.03.2016 18:56, David Steele wrote:
>
>>
>> This patch applies cleanly and is ready for review with no outstanding
>> issues that I can see.  Simon and Artur, you are both signed up as
>> reviewers.  Care to take a crack at it?
>>
>> Thanks,
>>
>>
> I have tested the patch once again and have looked the code. It looks good
> for me. I haven't any observation.
>
> The patch does its function correctly. I have tested it with a plugin,
> which writes DDL queries to the WAL using a hook and replicates this
> queries at subscribers.
>

Would you mind sharing the plugin here? I could add it to src/test/modules
and add some t/ tests so it runs under the TAP test framework.


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

[HACKERS] typmod is always -1

[Chapman Flack]

nothing like resurrecting a really old thread ...

> Pavel Stehule  writes:
>> I have a problem - every call of mvarcharin is with typmod = -1.

2009/3/17 Tom Lane :
> Also, there are a bunch of scenarios where we rely on a cast function to
> apply the typmod rather than passing it to the input function initially.
> I'm not sure if the particular case you're checking here falls into that
> category,

Is it possible to name any case that *does not* fall into that category?

I'm in the same boat ... I have an input function I want to test, and so
far I have failed to think of *any* sql construct that causes it to be
invoked with other than -1 for the typmod.

> but you definitely should have a "length conversion cast"
> function in pg_cast if you expect to do anything useful with typmod.

Ok, that's good to know (and I didn't until now). But back to the
input and recv functions, which are both documented to have 3-arg
forms that get typmods ... how would one test them?  Is there any
sql syntax that can be written to make them get passed a typmod?

If I just write them with assert(typmod == -1), will anyone ever
see a failure?

-Chap


-- 
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] WIP: Access method extendability

[Teodor Sigaev]

Good catch, thanks! Tests were added.


I don't see any objection, is consensus reached? I'm close to comiit that...

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
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] plpgsql - DECLARE - cannot to use %TYPE or %ROWTYPE for composite types

[Joe Conway]

On 03/15/2016 05:17 PM, Tom Lane wrote:
> In short, I think we should reject this implementation and instead try
> to implement the type operators we want in the core grammar's Typename
> production, from which plpgsql will pick it up automatically.  That is
> going to require some other syntax than this.  As I said, I'm not
> particularly pushing the function-like syntax I wrote upthread; but
> I want to see something that is capable of supporting all those features
> and can be extended later if we think of other type operators we want.

+1

Anyone want to argue against changing the status of this to Rejected or
at least Returned with feedback?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Proposal: Generic WAL logical messages

[Artur Zakirov]

On 16.03.2016 18:56, David Steele wrote:


This patch applies cleanly and is ready for review with no outstanding
issues that I can see.  Simon and Artur, you are both signed up as
reviewers.  Care to take a crack at it?

Thanks,



I have tested the patch once again and have looked the code. It looks 
good for me. I haven't any observation.


The patch does its function correctly. I have tested it with a plugin, 
which writes DDL queries to the WAL using a hook and replicates this 
queries at subscribers.


If Simon is not against, the patch can be marked as "Ready for Commiter".

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


--
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] oldest xmin is far in the past

[Tomas Vondra]

Hi,

On 03/18/2016 09:42 AM, John Snow wrote:

Hi everyone!

Trying to make VACUUM FREEZE on PG instance and keep getting this error:

2016-03-18 05:56:51 UTC   46750 WARNING:  oldest xmin is far in the past
2016-03-18 05:56:51 UTC   46750 HINT:  Close open transactions soon to
avoid wraparound problems.
2016-03-18 05:56:51 UTC   46750 DEBUG:  transaction ID wrap limit is
2654342112, limited by database with OID 1
2016-03-18 05:56:51 UTC   46750 DEBUG:  MultiXactId wrap limit is
2147483648, limited by database with OID 12451

Also "age" and "relfrozenxid" doesnt't change.


That probably means there's an old transaction somewhere - either a 
regular one (check pg_stat_activity) or a prepared one (pg_prepared_xacts).


The meaning of "old" depends on autovacuum_freeze_max_age - what value 
is set in the session running the VACUUM FREEZE?


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


--
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_dump / copy bugs with "big lines" ?

[Alvaro Herrera]

Daniel Verite wrote:

> To go past that problem, I've tried tweaking the StringInfoData
> used for COPY FROM, like the original patch does in CopyOneRowTo. 
> 
> It turns out that it fails a bit later when trying to make a tuple
> from the big line, in heap_form_tuple():
> 
>   tuple = (HeapTuple) palloc0(HEAPTUPLESIZE + len);
> 
> which fails because (HEAPTUPLESIZE + len) is again considered
> an invalid size, the  size being 1468006476 in my test.

Um, it seems reasonable to make this one be a huge-zero-alloc:

MemoryContextAllocExtended(CurrentMemoryContext, HEAPTUPLESIZE + len,
   MCXT_ALLOC_HUGE | MCXT_ALLOC_ZERO)

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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: fix lock contention for HASHHDR.mutex

[Amit Kapila]

On Sat, Mar 19, 2016 at 1:41 AM, Robert Haas  wrote:
>
> On Tue, Mar 1, 2016 at 9:43 AM, Aleksander Alekseev
>  wrote:
> >
> > So answering your question - it turned out that we _can't_ reduce
> > NUM_FREELISTS this way.
>
> That's perplexing.  I would have expected that with all of the mutexes
> packed in back-to-back like this, we would end up with a considerable
> amount of false sharing.  I don't know why it ever helps to have an
> array of bytes all in the same cache line of which each one is a
> heavily-trafficked mutex.   Anybody else have a theory?
>
> One other thing that concerns me mildly is the way we're handling
> nentries.  It should be true, with this patch, that the individual
> nentries sum to the right value modulo 2^32.  But I don't think
> there's any guarantee that the values are positive any more, and in
> theory after running long enough one of them could manage to overflow
> or underflow.
>

Won't in theory, without patch as well nentries can overflow after running
for very long time?  I think with patch it is more prone to overflow
because we start borrowing from other free lists as well.

  So at a very minimum I think we need to remove the
> Assert() the value is not negative.  But really, I wonder if we
> shouldn't rework things a little more than that.
>
> One idea is to jigger things so that we maintain a count of the total
> number of entries that doesn't change except when we allocate, and
> then for each freelist partition we maintain the number of entries in
> that freelist partition.  So then the size of the hash table, instead
> of being sum(nentries) is totalsize - sum(nfree).
>

To me, your idea sounds much better than current code in terms of
understanding the free list concept as well.  So, +1 for changing the code
in this way.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Fuzzy substring searching with the pg_trgm extension

[Artur Zakirov]

2016-03-18 23:46 GMT+03:00 Jeff Janes :
>
>
> <% and <<-> are not documented at all.  Is that a deliberate choice?
> Since they were added as convenience functions for the user, I think
> they really need to be in the user documentation.
>

I can send a patch a little bit later. I documented  %>
and <->> because examples of other operators have the following order:

SELECT t, t <-> 'word' AS dist
  FROM test_trgm
  ORDER BY dist LIMIT 10;

and

SELECT * FROM test_trgm WHERE t LIKE '%foo%bar';

I did not include <% and <<-> because I did not know how to document
commutators. But I can fix it.

And honestly the following order:

SELECT 'word' <% t FROM test_trgm;

is more convenient to me too.

Do you know how do not break the line in the operators table in the first
column? Now I see:

Operator | Returns
|--
text %   |boolean
text  |

But the following is better:

Operator | Returns
|--
text % text |boolean


>
> Also, the documentation should probably include <% and <<-> as the
> "parent" operators and use them in the examples, and only mention %>
> and <->> in passing, as the commutators.  That is because <% and <<->
> take their arguments in the same order as word_similarity does.  It
> would be less confusing if the documentation and the examples didn't
> need to keep changing the argument orders.
>
> Cheers,
>
> Jeff
>



-- 
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Re: [HACKERS] Performance degradation in commit ac1d794

[Andres Freund]

On 2016-03-18 15:49:51 -0300, Alvaro Herrera wrote:
> This seems a reasonable change, but I think that the use of WIN32 vs.
> LATCH_USE_WIN32 is pretty confusing.  In particular, LATCH_USE_WIN32
> isn't actually used for anything ... I suppose we don't care since this
> is a temporary state of affairs only?

Hm, I guess we could make some more of those use LATCH_USE_WIN32.
There's essentially two axes here a) latch notification method
(self-pipe vs windows events) b) readiness notification (epoll vs poll
vs select vs WaitForMultipleObjects).


> In 0005: In latch.c you typedef WaitEventSet, but the typedef already
> appears in latch.h.  You need only declare the struct in latch.c,
> without typedef'ing.

Good catch. It's even important, some compilers choke on that.


> Haven't really reviewed anything here yet, just skimming ATM.  Having so
> many #ifdefs all over the place in this file looks really bad, but I
> guess there's no way around that because this is very platform-specific.

I think it's hard to further reduce the number of ifdefs, but if you
have ideas...


> I hope pgindent doesn't choke on it.

The patch is pgindented (I'd personally never decrease indentation just
to fit a line into 79 chars, as pgindent does...).

Thanks for looking!


Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] flex: where's THIS been all this time?

[Tom Lane]

For years upon years, we have endured the ugly hack of compiling
flex-generated lexers as part of some other .c file, because of
the problem explained thus in, eg, psql/mainloop.c:

/*
 * psqlscan.c is #include'd here instead of being compiled on its own.
 * This is because we need postgres_fe.h to be read before any system
 * include files, else things tend to break on platforms that have
 * multiple infrastructures for stdio.h and so on.  flex is absolutely
 * uncooperative about that, so we can't compile psqlscan.c on its own.
 */
#include "psqlscan.c"

Perhaps that was true when written, but I just happened across this
bit in the flex manual:

   A `%top' block is similar to a `%{' ... `%}' block, except that the
   code in a `%top' block is relocated to the _top_ of the generated file,
   before any flex definitions.

I've confirmed that this works as stated back to flex 2.5.33, which
is the oldest version we support.  So we could compile lexers on their
own with a simple %top inclusion of postgres.h or postgres-fe.h,
as appropriate.

While I'm not quite sufficiently excited to run around and fix all our .l
files like this today, I'm definitely planning to do it for psql's lexer,
since I'm messing with that right now, and I don't much like
Horiguchi-san's solution to the problem.

regards, tom lane


-- 
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] Re: PATCH: Split stats file per database WAS: autovacuum stress-testing our system

[Tomas Vondra]

Hi,

On 03/15/2016 03:04 AM, Noah Misch wrote:

On Mon, Mar 14, 2016 at 01:33:08PM +0100, Tomas Vondra wrote:

On 03/14/2016 07:14 AM, Noah Misch wrote:

On Mon, Mar 14, 2016 at 02:00:03AM +0100, Tomas Vondra wrote:

+* XXX Maybe this should also care about the clock skew, just like the
+* block a few lines down.


Yes, it should.  (The problem is large (>~100s), backward clock resets, not
skew.)  A clock reset causing "msg->clock_time < dbentry->stats_timestamp"
will usually also cause "msg->cutoff_time < dbentry->stats_timestamp".  Such
cases need the correction a few lines down.


I'll look into that. I have to admit I have a hard time reasoning about the
code handling clock skew, so it might take some time, though.


No hurry; it would be no problem to delay this several months.


Attached is a patch that should fix the coalescing, including the clock 
skew detection. In the end I reorganized the code a bit, moving the 
check at the end, after the clock skew detection. Otherwise I'd have to 
do the clock skew detection on multiple places, and that seemed ugly.


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


pgstat-coalesce-v3.patch
Description: binary/octet-stream

-- 
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] Fuzzy substring searching with the pg_trgm extension

[Jeff Janes]

On Mon, Mar 14, 2016 at 9:27 AM, Artur Zakirov  wrote:
> On 14.03.2016 18:48, David Steele wrote:
>>
>> Hi Jeff,
>>
>> On 2/25/16 5:00 PM, Jeff Janes wrote:
>>
>>> But, It doesn't sound like I am going to win that debate.  Given that,
>>> I don't think we need a different name for the function. I'm fine with
>>> explaining the word-boundary subtlety in the documentation, and
>>> keeping the function name itself simple.
>>
>>
>> It's not clear to me if you are requesting more documentation here or
>> stating that you are happy with it as-is.  Care to elaborate?
>>
>> Other than that I think this patch looks to be ready for committer. Any
>> objections?
>>
>
> There was some comments about the word-boundary subtlety. But I think it was
> not enough.
>
> I rephrased the explanation of word_similarity() and %>. It is better now.
>
> But if it is not correct I can change the explanation.

<% and <<-> are not documented at all.  Is that a deliberate choice?
Since they were added as convenience functions for the user, I think
they really need to be in the user documentation.

Also, the documentation should probably include <% and <<-> as the
"parent" operators and use them in the examples, and only mention %>
and <->> in passing, as the commutators.  That is because <% and <<->
take their arguments in the same order as word_similarity does.  It
would be less confusing if the documentation and the examples didn't
need to keep changing the argument orders.

Cheers,

Jeff


-- 
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] Publish autovacuum informations

[Jim Nasby]

On 3/3/16 3:54 AM, Kyotaro HORIGUCHI wrote:

I wonder why there haven't been discussions so far on what kind
of information we want by this feature. For example I'd be happy
to see the time of last autovacuum trial and the cause if it has
been skipped for every table. Such information would (maybe)
naturally be shown in pg_stat_*_tables.

=
=# select relid, last_completed_autovacuum, last_completed_autovacv_status, 
last_autovacuum_trial, last_autovacuum_result from pg_stat_user_tables;
-[ RECORD 1 ]-+--
relid | 16390
last_completed_autovacuum | 2016-03-01 01:25:00.349074+09
last_completed_autovac_status | Completed in 4 seconds. Scanned 434 pages, 
skipped 23 pages
last_autovacuum_trial | 2016-03-03 17:33:04.004322+09
last_autovac_traial_status| Canceled by PID 2355. Processed 144/553 pages.
-[ RECORD 2 ]--+--
...
last_autovacuum_trial | 2016-03-03 07:25:00.349074+09
last_autovac_traial_status| Completed in 4 seconds. Scanned 434 pages, 
skipped 23 pages
-[ RECORD 3 ]--+--
...
last_autovacuum_trial | 2016-03-03 17:59:12.324454+09
last_autovac_trial_status | Processing by PID 42334, 564 / 32526 pages done.
-[ RECORD 4 ]--+--
...
last_autovacuum_trial | 2016-03-03 17:59:12.324454+09
last_autovac_trial_status | Skipped by dead-tuple threashold.
=


I kinda like where you're going here, but I certainly don't think the 
stats system is the way to do it. Stats bloat is already a problem on 
bigger systems. More important, I don't think having just the last 
result is very useful. If you've got a vacuum problem, you want to see 
history, especially history of the vacuum runs themselves.


The good news is that vacuum is a very low-frequency operation, so it 
has none of the concerns that the generic stats system does. I think it 
would be reasonable to provide event triggers that fire on every 
launcher loop, after a worker has built it's "TODO list", and after 
every (auto)vacuum.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
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] Weighted Stats

[Jeff Janes]

On Tue, Mar 15, 2016 at 8:36 AM, David Fetter  wrote:
>
> Please find attached a patch that uses the float8 version to cover the
> numeric types.

Is there a well-defined meaning for having a negative weight?  If no,
should it be disallowed?

I don't know what I was expecting,  but not this:

select weighted_avg(x,1000-2*x) from generate_series(1,1000) f(x);
   weighted_avg
--
 1671666717.1


Also, I think it might not give the correct answer even without
negative weights:

create table foo as select floor(random()*1)::int val from
generate_series(1,1000);

create table foo2 as select val, count(*) from foo group by val;

Shouldn't these then give the same result:

select stddev_samp(val) from foo;
stddev_samp
---
 2887.054977297105

select weighted_stddev_samp(val,count) from foo2;
 weighted_stddev_samp
--
 2887.19919651336

The 5th digit seems too early to be seeing round-off error.

Cheers,

Jeff


-- 
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] RFC: replace pg_stat_activity.waiting with something more descriptive

[Robert Haas]

On Thu, Mar 17, 2016 at 9:22 PM, Michael Paquier
 wrote:
> FWIW, my instinctive thought on the matter is to report the event
> directly in WaitLatch() via a name of the event caller provided
> directly in it. The category of the event is then defined
> automatically as we would know its origin. The code path defining the
> origin point from where a event type comes from is the critical thing
> I think to define an event category. The LWLock events are doing that
> in lwlock.c.

I'm very skeptical of grouping everything that waits using latches as
a latch wait, but maybe it's OK to do it that way.  I was thinking
more of adding categories like "client wait" with events like "client
read" and "client write".

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


-- 
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] Odd system-column handling in postgres_fdw join pushdown patch

[Etsuro Fujita]

On 2016/03/17 22:15, Ashutosh Bapat wrote:

On Thu, Mar 17, 2016 at 4:30 PM, Etsuro Fujita
> wrote:



BUT: we don't make any effort to ensure that local and remote values
match, so system columns other than ctid and oid should not be retrieved
from the remote server.  So, I'd like to propose: (1) when tableoids are
requested from the remote server, postgres_fdw sets valid values for
them locally, instead (core should support that?) and



If we are disabling join pushdown when the targetlist has other system
columns, shouldn't we treat tableoid in the same fashion. We should
disable join pushdown when tableoid is requested?


That seems a bit too restrictive to me.


I agree that we might want to do this in core instead of FDW specific
core. That way we avoid each FDW implementing its own solution.
Ultimately, all that needs to be done to push OID of the foreign table
in place of tableoid column. The core code can do that. It already does
that for the base tables.


OK, I'll try to modify the patch so that the core does that work.


(2) when any of
xmins, xmaxs, cmins, and cmaxs are requested, postgres_fdw gives up
pushing down foreign joins.  (We might be able to set appropriate values
for them locally the same way as for tableoids, but I'm not sure it's
worth complicating the code.)  I think that would be probably OK,
because users wouldn't retrieve any such columns in practice.



In that patch you have set pushdown_safe to false for the base relation
fetching system columns. But pushdown_safe = false means that that
relation is not safe to push down. A base foreign relation is always
safe to push down, so should have pushdown_safe = true always. Instead,
I suggest having a separate boolean has_unshippable_syscols (or
something with similar name) in PgFdwRelationInfo, which is set to true
in such case. In foreign_join_ok, we return false (thus not pushing down
the join), if any of the joining relation has that attribute set. By
default this member is false.


Maybe I'm missing something, but why do you consider that base foreign 
tables need always be safe to push down?  IIUC, the pushdown_safe flag 
is used only for foreign_join_ok, so I think that a base foreign table 
needs not necessarily be safe to push down.



Even for a base table those values are rather random, although they are
not fetched from the foreign server. Instead of not pushing the join
down, we should push the join down without fetching those attributes.
While constructing the query, don't include these system attributes in
SELECT clause and don't include corresponding positions in
retrieved_attributes list. That means those attributes won't be set
while fetching the row from the foreign server and will have garbage
values in corresponding places. I guess that would work.


That might be an idea, but as I said above, users wouldn't specify any 
system columns other than tableoid and ctid (and oid) in their queries, 
in practice, so I'm not sure it's worth complicating the code.


Best regards,
Etsuro Fujita




--
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] Parallel Aggregate

[David Rowley]

On 19 March 2016 at 05:46, Robert Haas  wrote:
> On Wed, Mar 16, 2016 at 5:05 PM, David Rowley
>  wrote:
>>> Cool!  Why not initialize aggpartialtype always?
>>
>> Because the follow-on patch sets that to either the serialtype or the
>> aggtranstype, depending on if serialisation is required. Serialisation
>> is required for parallel aggregate, but if we're performing the
>> partial agg in the main process, then we'd not need to do that. This
>> could be solved by adding more fields to AggRef to cover the
>> aggserialtype and perhaps expanding aggpartial into an enum mode which
>> allows NORMAL, PARTIAL, PARTIAL_SERIALIZE, and have exprType() pay
>> attention to the mode and return 1 of the 3 possible types.
>
> Urk.  That might still be better than what you have right now, but
> it's obviously not great.  How about ditching aggpartialtype and
> adding aggoutputtype instead?  Then you can always initialize that to
> whatever it's supposed to be based on the type of aggregation you are
> doing, and exprType() can simply return that field.

hmm, that might be better, but it kinda leaves aggpartial without much
of a job to do. The only code which depends on that is the sanity
checks that I added in execQual.c, and it does not really seem worth
keeping it for that. The only sanity check that I can think to do here
is if (aggstate->finalizeAggs && aggref->aggoutputtype !=
aggref->aggtype) -- we have a problem. Obviously we can't check that
for non-finalize nodes since the aggtype can match the aggoutputtype
for legitimate reasons.



-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] postgresql 9.4 on AIX 7.1

[Lizeth Solis Aramayo]

Hello,

Please I want to install the software mentioned Up (postgresql 9.4 on AIX 7.1 - 
8.0 ).
Do you think that I could do this?

Thanks a lot, I am from Bolivia,  south America.

[dos]

La informaci?n contenida en este mensaje esta dirigida en forma exclusiva 
para el uso personal y confidencial del o los destinatarios arriba nombrados. 
Si el lector de este mensaje no es el destinatario previsto o una persona 
responsable para su distribuci?n al destinatario, se le notifica que ha 
recibido este correo por error y que la revisi?n, distribuci?n, difusi?n o 
copia de este mensaje esta estrictamente prohibida. Si por error recibi? esta 
comunicaci?n, por favor notifiquenos inmediatamente y borre el mensaje 
original. The information contained in this message is intended only for 
the personal and confidential use of the recipient(s) named above. If the 
reader of this message is not the intended recipient or an agent responsible 
for delivering it to the intended recipient, you are hereby notified that you 
have received this document in error and that any review, dissemination, 
distribution, or copying of this message is strictly prohibited. If you have 
received this communication in error, please notify us immediately, and delete 
the original message.

Re: [HACKERS] Using quicksort for every external sort run

[Peter Geoghegan]

On Thu, Mar 17, 2016 at 1:13 PM, Robert Haas  wrote:
> OK, I have now committed 0001, and separately, some comment
> improvements - or at least, I think they are improvements - based on
> this discussion.

Thanks!

Your changes look good to me. It's always interesting to learn what
wasn't so obvious to you when you review my patches. It's probably
impossible to stare at something like tuplesort.c for as long as I
have and get that balance just right.

-- 
Peter Geoghegan


-- 
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] WIP: Upper planner pathification

[Tom Lane]

Robert Haas  writes:
> On Wed, Mar 16, 2016 at 2:47 PM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> On Mon, Mar 14, 2016 at 9:21 PM, Kouhei Kaigai  wrote:
 So, even though we don't need to define multiple hook declarations,
 I think the hook invocation is needed just after create__paths()
 for each. It will need to inform extension the context of hook
 invocation, the argument list will take UpperRelationKind.

>>> That actually seems like a pretty good point.  Otherwise you can't
>>> push anything from the upper rels down unless you are prepared to
>>> handle all of it.

>> I'm not exactly convinced of the use-case for that.  What external
>> thing is likely to handle window functions but not aggregation,
>> for example?

> I'm not going to say that you're entirely wrong, but I think that
> attitude is a bit short-sighted.

Well, I'm prepared to yield to the extent of repeating the hook call
before each phase with an UpperRelationKind parameter to tell which phase
we're about to do.  The main concern here is to avoid redundant
computation, but the hook can check the "kind" parameter and fall out
quickly if it has nothing useful to do at the current phase.

I do not, however, like the proposal to expose wflists and so forth.
Those are internal data structures in grouping_planner and I absolutely
refuse to promise that they're going to stay stable.  (I had already
been thinking a couple of weeks ago about revising the activeWindows
data structure, now that it would be reasonably practical to cost out
different orders for doing the window functions in.)  I think a hook
that has its own ideas about window function implementation methods
can gather its own information about the WFs without that much extra
cost, and it very probably wouldn't want exactly the same data that
create_window_paths uses today anyway.

So what I would now propose is

typedef void (*create_upper_paths_hook_type) (PlannerInfo *root,
  UpperRelationKind stage,
  RelOptInfo *input_rel);

and have this invoked at each stage right before we call
create_grouping_paths, create_window_paths, etc.

Also, I don't particularly see a need for a corresponding API for FDWs.
If an FDW is going to do anything in this space, it presumably has to
build up ForeignPaths for all the steps anyway.  So I'd be inclined
to leave GetForeignUpperPaths as-is.

Comments?

regards, tom lane


-- 
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: BSD Authentication support

[Marisa Emerson]

On 18/03/16 03:57, Thomas Munro wrote:
You used one name in the docs and another in the code:

+BSD Authentication on PostgreSQL uses the auth-postgres
+login type and authenticates with the postgres login

+ retval = auth_userokay(user, NULL, "auth-postgresql", passwd);

Woops, fix attached.
diff --git a/configure b/configure
index a45be67..8f305eb 100755
--- a/configure
+++ b/configure
@@ -827,6 +827,7 @@ with_python
 with_gssapi
 with_krb_srvnam
 with_pam
+with_bsd_auth
 with_ldap
 with_bonjour
 with_openssl
@@ -1516,6 +1517,7 @@ Optional Packages:
   --with-krb-srvnam=NAME  default service principal name in Kerberos (GSSAPI)
   [postgres]
   --with-pam  build with PAM support
+  --with-bsd-auth build with BSD Authentication support
   --with-ldap build with LDAP support
   --with-bonjour  build with Bonjour support
   --with-openssl  build with OpenSSL support
@@ -5571,6 +5573,41 @@ $as_echo "$with_pam" >&6; }
 
 
 #
+# BSD AUTH
+#
+{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether to build with BSD Authentication support" >&5
+$as_echo_n "checking whether to build with BSD Authentication support... " >&6; }
+
+
+
+# Check whether --with-bsd-auth was given.
+if test "${with_bsd_auth+set}" = set; then :
+  withval=$with_bsd_auth;
+  case $withval in
+yes)
+
+$as_echo "#define USE_BSD_AUTH 1" >>confdefs.h
+
+  ;;
+no)
+  :
+  ;;
+*)
+  as_fn_error $? "no argument expected for --with-bsd-auth option" "$LINENO" 5
+  ;;
+  esac
+
+else
+  with_bsd_auth=no
+
+fi
+
+
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $with_bsd_auth" >&5
+$as_echo "$with_bsd_auth" >&6; }
+
+
+#
 # LDAP
 #
 { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether to build with LDAP support" >&5
@@ -10524,6 +10561,23 @@ done
 
 fi
 
+if test "$with_bsd_auth" = yes ; then
+  for ac_header in bsd_auth.h
+do :
+  ac_fn_c_check_header_mongrel "$LINENO" "bsd_auth.h" "ac_cv_header_bsd_auth_h" "$ac_includes_default"
+if test "x$ac_cv_header_bsd_auth_h" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_BSD_AUTH_H 1
+_ACEOF
+
+else
+  as_fn_error $? "header file  is required for BSD Authentication support" "$LINENO" 5
+fi
+
+done
+
+fi
+
 if test "$with_systemd" = yes ; then
   ac_fn_c_check_header_mongrel "$LINENO" "systemd/sd-daemon.h" "ac_cv_header_systemd_sd_daemon_h" "$ac_includes_default"
 if test "x$ac_cv_header_systemd_sd_daemon_h" = xyes; then :
diff --git a/configure.in b/configure.in
index c298926..f17bfcc 100644
--- a/configure.in
+++ b/configure.in
@@ -674,6 +674,16 @@ AC_MSG_RESULT([$with_pam])
 
 
 #
+# BSD AUTH
+#
+AC_MSG_CHECKING([whether to build with BSD Authentication support])
+PGAC_ARG_BOOL(with, bsd-auth, no,
+  [build with BSD Authentication support],
+  [AC_DEFINE([USE_BSD_AUTH], 1, [Define to 1 to build with BSD support. (--with-bsd-auth)])])
+AC_MSG_RESULT([$with_bsd_auth])
+
+
+#
 # LDAP
 #
 AC_MSG_CHECKING([whether to build with LDAP support])
@@ -1269,6 +1279,10 @@ if test "$with_pam" = yes ; then
  [AC_MSG_ERROR([header file  or  is required for PAM.])])])
 fi
 
+if test "$with_bsd_auth" = yes ; then
+  AC_CHECK_HEADERS(bsd_auth.h, [], [AC_MSG_ERROR([header file  is required for BSD Authentication support])])
+fi
+
 if test "$with_systemd" = yes ; then
   AC_CHECK_HEADER(systemd/sd-daemon.h, [], [AC_MSG_ERROR([header file  is required for systemd support])])
 fi
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 3b2935c..0b63e42 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -522,6 +522,17 @@ hostnossl  database  user
  
 

+
+   
+bsd
+
+ 
+  Authenticate using BSD Authentication provided by the
+  operating system. See  for
+  details.
+ 
+
+   
   
 
   
@@ -1647,6 +1658,40 @@ host ... ldap ldapurl="ldap://ldap.example.net/dc=example,dc=net?uid?sub";
 

   
+
+  
+   
+
+   
+BSD
+   
+
+   
+This authentication method operates similarly to
+password 

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[David Steele]

On 3/16/16 9:00 AM, Michael Paquier wrote:

> On Tue, Mar 15, 2016 at 6:38 PM, David Steele  wrote:
>
>> 1) I see that rolvaliduntil is still in pg_authid:
>> I think that's OK if we now define it to be "role validity" (it's still
>> password validity in the patched docs).  I would also like to see a
>> validuntil column in pg_auth_verifiers so we can track password
>> expiration for each verifier separately.  For now I think it's enough to
>> copy the same validity both places since there can only be one verifier.
> 
> FWIW, this is an intentional change, and my goal is to focus on only
> the protocol aging for now. We will need to move rolvaliduntil to
> pg_auth_verifiers if we want to allow rolling updates of password
> verifiers for a given role, but that's a different patch, and we need
> to think about the SQL interface carefully. This infrastructure makes
> the move easier by the way to do that, and honestly I don't really see
> what we gain now by copying the same value to two different system
> catalogs.

Here's my thinking.  If validuntil is moved to pg_auth_verifiers now
then people can start using it there.  That will make it less traumatic
when/if validuntil in pg_authid is removed later.  The field in
pg_authid could be deprecated in this release to let people know not to
use it.

Or, as I suggested it could be recast as role validity, which right now
happens to be the same as password validity.

>> 2) I don't think the column naming in pg_auth_verifiers is consistent
>> with other catalogs:
>> postgres=# select * from pg_auth_verifiers;
>>  roleid | verimet |   verival
>> +-+-
>>   16387 | m   | md505a671c66aefea124cc08b76ea6d30bb
>>   16388 | p   | testu
>>
>> System catalogs generally use a 3 character prefix so I would expect the
>> columns to be (if we pick avr as a prefix):
> 
> OK, this makes sense.
> 
>> avrrole
>> avrmethod
>> avrverifier
> 
> Assuming "ver" is the prefix, we get: verroleid, vermethod, vervalue.
> I kind of like those ones, more than with "avr" as prefix actually.
> Other ideas are of course welcome.

ver is fine as a prefix.

>> 3) rolpassword is still in pg_shadow even though it is not useful anymore:
>> postgres=# select usename, passwd, valuntil from pg_shadow;
>>
>>  usename |  passwd  |valuntil
>> -+--+
>>  vagrant |  |
>>  test|  |
>>  testu   |  | 2017-01-01 00:00:00+00
>>
>> If anyone is actually using this column in a meaningful way they are in
>> for a nasty surprise when trying use the value in passwd as a verifier.
>>  I would prefer to drop the column entirely and produce a clear error.
>>
>> Perhaps a better option would be to drop pg_shadow entirely since it
>> seems to have no further purpose in life.
> 
> We discussed that on the previous thread and the conclusion was to
> keep pg_shadow, but to clobber the password value with "***",
> explaining this choice:
> http://www.postgresql.org/message-id/6174.1455501...@sss.pgh.pa.us

Ah, I missed that one.

-- 
-David
da...@pgmasters.net


-- 
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] IF (NOT) EXISTS in psql-completion

[Pavel Stehule]

Hi

2016-03-16 5:01 GMT+01:00 Kyotaro HORIGUCHI :

> Hello,
>
> # It seems that I have been forgotten in the recepient list..
>
> At Tue, 15 Mar 2016 22:09:59 -0400, Peter Eisentraut 
> wrote in <56e8c077.2000...@gmx.net>
> > On 2/5/16 3:09 AM, Kyotaro HORIGUCHI wrote:
> > > I considered how to make tab-completion robust for syntactical
> > > noises, in other words, optional words in syntax. Typically "IF
> > > (NOT) EXISTS", UNIQUE and TEMPORARY are words that don't affect
> > > further completion.
> >
> > To repeat the question I raised in the previous commit fest about tab
> > completion: Why do you want tab completion for IF NOT EXISTS?  When you
> > tab complete, the completion mechanism will show you whether the item in
> > question exists.  What is the use case?
>
> Ah, I think I understand you question. It's not about IF EXISTS,
> but only IF NOT EXSTS. It is needed when repeated execution of
> the same SQL statement will be done using command line
> history. Such stocks of commands in history is often
> convenient. And sometimes I rely on psql-completion to write a
> SQL script. The completions for such words seemingly useless on
> instant-execution will be handy to do that.
>
> Another thing I want to do by this patch is that we can get
> completion even after such optional words. I have been annoyed
> many times by this. Some of them, such as UNIQUE, TEMPORARY and
> CONCURRENTLY are treated but they just doubles the matching
> condition expressions.
>

I am looking this patch. It looks well, but this feature doesn't respect
upper or lower chars. It enforce upper chars. This is not consistent with
any other autocomplete.

Regards

Pavel



>
> regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
>
>
>
> --
> 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] Relation extension scalability

[Petr Jelinek]

On 17/03/16 04:42, Dilip Kumar wrote:


On Mon, Mar 14, 2016 at 8:26 AM, Petr Jelinek > wrote:

Well any value we choose will be very arbitrary. If we look at it
from the point of maximum absolute disk space we allocate for
relation at once, the 4MB limit would represent 2.5 orders of
magnitude change. That sounds like enough for one release cycle, I
think we can further tune it if the need arises in next one. (with
my love for round numbers I would have suggested 8MB as that's 3
orders of magnitude, but I am fine with 4MB as well)


I have modified the patch, this contains the max limit on extra pages,
512(4MB) pages is the max limit.

I have measured the performance also and that looks equally good.



Great.

Just small notational thing, maybe this would be simpler?:
extraBlocks = Min(512, lockWaiters * 20);

--
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services


--
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] Re: Add generate_series(date,date) and generate_series(date,date,integer)

[Tom Lane]

David Steele  writes:
> On 3/17/16 11:30 AM, David G. Johnston wrote:
>> ​I'd call it "generate_dates(...)" and be done with it.
>> We would then have:
>> generate_series()
>> generate_subscripts()
>> generate_dates()

> To me this completely negates the idea of this "just working" which is
> why it got a +1 from me in the first place.  If I have to remember to
> use a different function name then I'd prefer to just cast on the
> timestamp version of generate_series().

Yeah, this point greatly weakens the desirability of this function IMO.
I've also gone from "don't care" to "-1".

regards, tom lane


-- 
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] Re: Add generate_series(date,date) and generate_series(date,date,integer)

[David G. Johnston]

On Thu, Mar 17, 2016 at 8:41 AM, David Steele  wrote:

> On 3/17/16 11:30 AM, David G. Johnston wrote:
> > On Thu, Mar 17, 2016 at 7:57 AM, Corey Huinker  > >wrote:
> >
> > On Thu, Mar 17, 2016 at 10:00 AM, David Steele  > > wrote:
> >
> > On 3/17/16 4:49 AM, Dean Rasheed wrote:
> >
> > > On 16 March 2016 at 23:32, David Steele  > wrote:
> > >
> > >>
> > >> I think in this case it comes down to a committer's judgement
> so I have
> > >> marked this "ready for committer" and passed the buck on to
> Álvaro.
> > >
> > > So I was pretty much "meh" on this patch too, because I'm not
> > > convinced it actually saves much typing, if any.
> > >
> > > However, I now realise that it introduces a
> backwards-compatibility
> > > breakage. Today it is possible to type
> > >
> > > SELECT * FROM generate_series('01-01-2000'::date,
> '01-04-2000', '7 days');
> >
> > It can also be broken as below and this is even scarier to me:
> >
> >
> > Above and below are the same query​...
>
> Not sure I agree.  My point was that even if developers were pretty
> careful with their casting (or are using two actual dates) then there's
> still possibility for breakage.
>

With the first argument casted to date it doesn't matter whether you cast
the second argument as the pseudo-type anyelement will take its value from
the first argument and force the second to date.

The main problem is that the system tries to cast unknown to integer based
upon finding gs(date, date, integer) and fails without ever considering
that gs(ts, ts, interval) would succeed.


> > postgres=# SELECT * FROM generate_series('01-01-2000'::date,
> > '01-04-2000'::date, '7 days');
> > ERROR:  invalid input syntax for integer: "7 days"
> > LINE 1: ...te_series('01-01-2000'::date, '01-04-2000'::date, '7
> > days');
> > <...>
> >
> > I see two ways around this:
> >
> > 1. Drop the step parameter entirely. My own use cases only ever
> > require the step values 1 and -1, and which one the user wants can
> > be inferred from (start < end). This would still change the output
> > type where a person wanted timestamps, but instead input two dates.
> >
> > ​Undesirable.​
>
> Very undesirable.  Week intervals are a very valid use case and I don't
> like the automagic interval idea.
>
> >
> > 2. Rename the function date_series() or generate_series_date()
> >
> > I still think this is an important function because at the last
> > several places I've worked, I've found myself manufacturing a query
> > where some event data is likely to have date gaps, but we want to
> > see the date gaps or at least have the 0 values contribute to a
> > later average aggregate.
> >
> >
> > ​I'd call it "generate_dates(...)" and be done with it.
> >
> > We would then have:
> >
> > generate_series()
> > generate_subscripts()
> > generate_dates()
>
> To me this completely negates the idea of this "just working" which is
> why it got a +1 from me in the first place.  If I have to remember to
> use a different function name then I'd prefer to just cast on the
> timestamp version of generate_series().
>
>
​So let the user decide whether to trade-off learning a new function name
but getting dates instead of timestamps or going through the hassle of
casting.​

For me, it would have been a nice bonus if the generate_series() could be
used directly but I feel that the desired functionality is desirable and
the name itself is of only minor consideration.

I can see that newbies might ponder why two functions exist but they should
understand "backward compatibility".

I suspect that most people will simply think: "use generate_series for
numbers and use generate_dates for, well, dates".  The ones left out in the
cold are those wondering why they use "generate_series" for timestamp
series with a finer precision than date.  I'd be willing to offer a
"generate_timestamps" to placate them.  And might as well toss in
"generate_numbers" for completeness - though now I likely doom the
proposal...so I'll stick with just add generate_dates so the behavior is
possible without superfluous casting or the need to write a custom
function.  Dates are too common a unit of measure to leave working with
them this cumbersome.

David J.

Re: [HACKERS] Proposal: Generic WAL logical messages

[David Steele]

On 3/9/16 6:58 PM, Petr Jelinek wrote:
> On 08/03/16 21:21, Artur Zakirov wrote:
>> I think here
>>
>>> +const char *
>>> +logicalmsg_identify(uint8 info)
>>> +{
>>> +if (info & ~XLR_INFO_MASK == XLOG_LOGICAL_MESSAGE)
>>> +return "MESSAGE";
>>> +
>>> +return NULL;
>>> +}
>>
>> we should use brackets
>>
>> const char *
>> logicalmsg_identify(uint8 info)
>> {
>>  if ((info & ~XLR_INFO_MASK) == XLOG_LOGICAL_MESSAGE)
>>  return "MESSAGE";
>>
>>  return NULL;
>> }
>>
> 
> Correct, fixed, thanks.
> 
> I also rebased this as there was conflict after the fixes to logical
> decoding by Andres.

This patch applies cleanly and is ready for review with no outstanding
issues that I can see.  Simon and Artur, you are both signed up as
reviewers.  Care to take a crack at it?

Thanks,
-- 
-David
da...@pgmasters.net


-- 
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] Password identifiers, protocol aging and SCRAM protocol

[Robert Haas]

On Fri, Mar 18, 2016 at 9:31 AM, Michael Paquier
 wrote:
> That's not an issue for me to rebase this set of patches. The only
> conflicts that I anticipate are on 0009, but I don't have high hopes
> to get this portion integrating into core for 9.6, the rest of the
> patches is complicated enough, and everyone bandwidth is limited.

I really think we ought to consider pushing this whole thing out to
9.7.  I don't see how we're going to get all of this into 9.6, and
these are big, user-facing changes that I don't think we should rush
into under time pressure.  I think it'd be better to do this early in
the 9.7 cycle so that it has time to settle before the time crunch at
the end.  I predict this is going to have a lot of loose ends that are
going to take months to settle, and we don't have that time right now.
And I'd rather see all of the changes in one release than split them
across two releases.

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


-- 
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: Cache data in GetSnapshotData()

[Mithun Cy]

On Thu, Mar 17, 2016 at 9:00 AM, Amit Kapila 
wrote:
>If you see, for the Base readings, there is a performance increase up till
64 clients and then there is a fall at 88 clients, which to me >indicates
that it hits very high-contention around CLogControlLock at 88 clients
which CLog patch is able to control to a great degree (if >required, I
think the same can be verified by LWLock stats data).  One reason for
hitting contention at 88 clients is that this machine >seems to have
64-cores (it has 8 sockets and 8 Core(s) per socket) as per below
information of lscpu command.

I am attaching LWLock stats data for above test setups(unlogged tables).

*BASE* At 64 clients Block-time unit At 88 clients Block-time unit
ProcArrayLock 182946 117827
ClogControlLock 107420 120266
*clog patch*

ProcArrayLock 183663 121215
ClogControlLock 72806 65220
*clog patch + save snapshot*

ProcArrayLock 128260 83356
ClogControlLock 78921 74011
This is for unlogged lables, I mainly see ProcArrayLock have higher
contention at 64 clients and at 88 contention is slightly moved to other
entities.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


lwlock details.odt
Description: application/vnd.oasis.opendocument.text

-- 
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] Relaxing SSL key permission checks

[Christoph Berg]

Re: Peter Eisentraut 2016-03-16 <56e8c221.1050...@gmx.net>
> >> * it failed to check for S_IXUSR, so permissions 0700 were okay, in
> >> contradiction with what the error message indicates.  This is a
> >> preexisting bug actually.  Do we want to fix it by preventing a
> >> user-executable file (possibly breaking compability with existing
> >> executable key files), or do we want to document what the restriction
> >> really is?
> > 
> > I think we should not check for S_IXUSR.  There is no reason for doing that.
> > 
> > I can imagine that key files are sometimes copied around using USB
> > drives with FAT file systems or other means of that sort where
> > permissions can scrambled.  While I hate gratuitous executable bits as
> > much as the next person, insisting here would just create annoyances in
> > practice.
> 
> I'm happy with this patch except this minor point.  Any final comments?

I'm fine with that change.

Do you want me to update the patch or do you already have a new
version, given it's marked as Ready for Committer?

Christoph
-- 
c...@df7cb.de | http://www.df7cb.de/


-- 
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] we have added support for box type in SP-GiST index

[Teodor Sigaev]

Do reconstructedValues is required now?  Wouldn't it be cleaner to use
the new field on the prefix tree implementation, too?
reconstructedValues is needed to reconctruct full indexed value and should match 
to type info indexed column




We haven't had specific memory context for reconstructedValues.  Why
is it required for the new field?
because pg knows type of reconstructedValues (see above why) but traversalValue 
just a void*, SP-GiST core doesn't knnow how to free memory of allocated for it.






+   Memory for traversalValues should be allocated in
+   the default context, but make sure to switch to
+   traversalMemoryContext before allocating memory
+   for pointers themselves.


This sentence is not understandable.  Shouldn't it say "the memory
should *not* be allocated in the default context"?  What are the
"pointers themselves"?

Clarified, I hope




The box opclass is broken because of the floating point arithmetics of
the box type.  You can reproduce it with the attached script.  We

hope, fixed. At least, seems, syncronized with seqscan


really need to fix the geometric types, before building more on them.
They fail to serve the only purpose they are useful for, demonstrating
features.

agree, but it's not a deal for this patch



It think the opclass should support the cross data type operators like
box @> point.  Ideally we should do it by using multiple opclasses in
an opfamily.  The floating problem will bite us again in here, because
some of the operators are not using FP macros.

Again, agree. But I suggest to do it by separate patch.



The tests check not supported operator "@".  It should be "<@".  It is
nice that the opclass doesn't support long deprecated operators.

fixed



+ #define LT  -1
+ #define GT   1
+ #define EQ   0


Using these numbers is a very well established pattern.  I don't think
macros make the code any more readable.

fixed




+ /* SP-GiST API functions */
+ Datum   spg_box_quad_config(PG_FUNCTION_ARGS);
+ Datum   spg_box_quad_choose(PG_FUNCTION_ARGS);
+ Datum   spg_box_quad_picksplit(PG_FUNCTION_ARGS);
+ Datum   spg_box_quad_inner_consistent(PG_FUNCTION_ARGS);
+ Datum   spg_box_quad_leaf_consistent(PG_FUNCTION_ARGS);


I guess they should go to the header file.
Remove them because they are presented in auto-generated file 
./src/include/utils/builtins.h


range patch is unchanged, but still attached to keep them altogether.

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


q4d-2.patch.gz
Description: GNU Zip compressed data


traversalValue-2.patch.gz
Description: GNU Zip compressed data


range-1.patch.gz
Description: GNU Zip compressed data

-- 
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] WIP: Upper planner pathification

[Kouhei Kaigai]

> -Original Message-
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tom Lane
> Sent: Friday, March 18, 2016 11:44 PM
> To: Kaigai Kouhei(海外 浩平)
> Cc: Robert Haas; Petr Jelinek; David Rowley; pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] WIP: Upper planner pathification
> 
> Kouhei Kaigai  writes:
> > On Wed, Mar 16, 2016 at 2:47 PM, Tom Lane  wrote:
> >> I do not, however, like the proposal to expose wflists and so forth.
> >> Those are internal data structures in grouping_planner and I absolutely
> >> refuse to promise that they're going to stay stable.
> 
> > Hmm... It's not easy to imagine a case that extension wants own idea
> > to extract window functions from the target list and select active
> > windows, even if extension wants to have own executor and own cost
> > estimation logic.
> > In case when extension tries to add WindowPath + CustomPath(Sort),
> > extension is interested in alternative sort task, but not window
> > function itself. It is natural to follow the built-in implementation,
> > thus, it motivates extension author to take copy & paste the code.
> > select_active_windows() is static, so extension needs to have same
> > routine on their side.
> 
> Well, to be perfectly blunt about it, I have said from day one that this
> notion that a CustomScan extension will be able to cause arbitrary planner
> behavior changes is loony.  We are simply not going to drop a hook into
> every tenth line of the planner for you, nor de-static-ify every internal
> function, nor (almost equivalently) expose the data structures those
> functions produce, because it would cripple core planner development to
> try to keep the implied APIs stable.  And I continue to maintain that any
> actually-generally-useful ideas would be better handled by submitting them
> as patches to the core planner, rather than trying to implement them in an
> arms-length extension.
> 
> In the case at hand, I notice that the WindowFuncLists struct is
> actually from find_window_functions in clauses.c, so an extension
> that needed to get hold of that would be unlikely to do any copying
> and pasting anyhow -- it'd just call find_window_functions again.
> (That only needs to search the targetlist, so it's not that expensive.)
> The other lists you mention are all tightly tied to specific, and not
> terribly well-designed, implementation strategies for grouping sets and
> window functions.  Those are *very* likely to change in the near future;
> and even if they don't, I'm skeptical that an external implementor of
> grouping sets or window functions would want to use exactly those same
> implementation strategies.  Maybe the only reason you're there at all
> is you want to be smarter about the order of doing window functions,
> for example.
> 
> So I really don't want to export any of that stuff.
>
Hmm. I could understand we have active development around this area
thus miscellaneous internal data structure may not be enough stable
to expose the extension.
Don't you deny recall the discussion once implementation gets calmed
down on the future development cycle?

> As for other details of the proposed patch, I was intending to put
> all the hook calls into grouping_planner for consistency, rather than
> scattering them between grouping_planner and its subroutines.  So that
> would probably mean calling the hook for a given step *before* we
> generate the core paths for that step, not after.  Did you have a
> reason to want the other order?  (If you say "so the hook can look
> at the core-made paths", I'm going to say "that's a bad idea".  It'd
> further increase the coupling between a CustomScan extension and core.)
>
No deep reason. I just followed the manner in scan/join path hook; that
calls extension once the core feature adds built-in path nodes.

Thanks,
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 


-- 
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] [WIP] speeding up GIN build with parallel workers

[Robert Haas]

On Thu, Mar 17, 2016 at 11:42 PM, Amit Kapila  wrote:
> I think here the comparison should be between the case of (active backend +
> 1 worker) with (passive backend + 1 worker) or  (active backend + 2 worker)
> with (passive backend + 2 workers).  I don't think it is good assumption
> that workers are always freely available and you can use them as and when
> required for any operation.

Strong +1.  The pool of background workers is necessarily quite
limited and you can't just gobble them up.  I'm not saying that it's
absolutely essential that the leader can also participate, but saying
that 1 active leader + 1 worker is only 2% faster than 1 passive
leader + 2 workers is not comparing apples to apples.

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] incorrect docs for pgbench / skipped transactions

[Tomas Vondra]

Hi,

while learning about format of the transaction log produced by pgbench, 
I've noticed this sentence in the section describing format of the 
per-transaction log:


   The last field skipped_transactions reports the number of
   transactions skipped because they were too far behind schedule.
   It is only present when both options --rate and --latency-limit
   are used.

Which is wrong, because this field is only added in the aggregated log, 
not in the per-transaction one. So we should delete this.


It also does not explicitly explain that with --latency-limit the 
latency column will contain "skipped" for transactions exceeding the 
limit latency (it's only mentioned in the example output).


So I think we should apply the attached patch (and also backpatch it to 
9.5, where the --latency-limit got introduced).


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


pgbench-skipped-doc-fix.patch
Description: binary/octet-stream

-- 
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] GinPageIs* don't actually return a boolean

[Yury Zhuravlev]

Michael Paquier wrote:

FWIW, when compiling with MS 2015 using the set of perl scripts I am
not seeing this compilation error...


This error not in build stage but in GIN regresion tests. CMake nothing to 
do with.


--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


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

[Julien Rouhaud]

On 18/03/2016 00:56, Tom Lane wrote:
> Julien Rouhaud  writes:
>> Shouldn't we also check "parallel_degree < max_worker_process" ?
> 
>> There's no need to compute any further than that. I think the best fix
>> would be to add a CheckHook or AssignHook on max_parallel_degree GUC to
>> make sure it's not more than max_worker_process.
> 
> Please, let's not go there.  Interdependent checks on GUC values are far
> harder to get right than you think.  It's far better to design the GUC
> specifications so that it doesn't matter.
> 
> For an example whereof I speak, check the sordid history of commit
> ee1e5662d8d83307 ("Auto-tune effective_cache size to be 4x shared
> buffers"), which eventually got reverted after a huge amount of thrashing
> trying to make it work consistently.  Admittedly, that was trying to make
> the default value of GUC X depend on GUC Y, but I think checking whether
> X <= Y would have many of the same problems.  The core issue is you don't
> know which one's going to get set first.
> 

Oh, I wasn't aware of that, thanks for the pointer.

> In this particular case I think it'd be fine to document that the
> effective amount of parallelism is Min(parallel_degree,max_worker_process).
> 
>   regards, tom lane
> 

I just saw that it's already documented that way. I attach a patch that
makes sure we don't try to compute a parallel_degree beyond this limit
(if you think it's worth it), and a missing description and "change
requires restart" for the max_worker_processes parameter in
postgresql.conf.sample.

-- 
Julien Rouhaud
http://dalibo.com - http://dalibo.org
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 4f60b85..2886219 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -23,6 +23,7 @@
 #include "catalog/pg_operator.h"
 #include "catalog/pg_proc.h"
 #include "foreign/fdwapi.h"
+#include "miscadmin.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
 #ifdef OPTIMIZER_DEBUG
@@ -678,7 +679,7 @@ create_parallel_paths(PlannerInfo *root, RelOptInfo *rel)
 	 * need something here for now.
 	 */
 	while (rel->pages > parallel_threshold * 3 &&
-		   parallel_degree < max_parallel_degree)
+		   parallel_degree < Min(max_parallel_degree, max_worker_processes))
 	{
 		parallel_degree++;
 		parallel_threshold *= 3;
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 773b4e8..00368bb 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -163,7 +163,8 @@
 # - Asynchronous Behavior -
 
 #effective_io_concurrency = 1		# 1-1000; 0 disables prefetching
-#max_worker_processes = 8
+#max_worker_processes = 8   # max number of background workers
+#(change requires restart)
 #max_parallel_degree = 0		# max number of worker processes per node
 
 

-- 
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] we have added support for box type in SP-GiST index

[Emre Hasegeli]

Here are my first comments.  I haven't read the actual index
implementation, yet.

I think traversal value is a useful addition.  It is nice that the
implementation for the range types is also changed.  My questions
about them are:

Do reconstructedValues is required now?  Wouldn't it be cleaner to use
the new field on the prefix tree implementation, too?

We haven't had specific memory context for reconstructedValues.  Why
is it required for the new field?

> +   Memory for traversalValues should be allocated in
> +   the default context, but make sure to switch to
> +   traversalMemoryContext before allocating memory
> +   for pointers themselves.

This sentence is not understandable.  Shouldn't it say "the memory
should *not* be allocated in the default context"?  What are the
"pointers themselves"?

The box opclass is broken because of the floating point arithmetics of
the box type.  You can reproduce it with the attached script.  We
really need to fix the geometric types, before building more on them.
They fail to serve the only purpose they are useful for, demonstrating
features.

It think the opclass should support the cross data type operators like
box @> point.  Ideally we should do it by using multiple opclasses in
an opfamily.  The floating problem will bite us again in here, because
some of the operators are not using FP macros.

The tests check not supported operator "@".  It should be "<@".  It is
nice that the opclass doesn't support long deprecated operators.

We needs tests for the remaining operators and for adding new values
to the index.  I am not sure create_index.sql is the best place for
them.  Maybe we should use the test scripts of "box".

> + #define LT  -1
> + #define GT   1
> + #define EQ   0

Using these numbers is a very well established pattern.  I don't think
macros make the code any more readable.

> + /* SP-GiST API functions */
> + Datum   spg_box_quad_config(PG_FUNCTION_ARGS);
> + Datum   spg_box_quad_choose(PG_FUNCTION_ARGS);
> + Datum   spg_box_quad_picksplit(PG_FUNCTION_ARGS);
> + Datum   spg_box_quad_inner_consistent(PG_FUNCTION_ARGS);
> + Datum   spg_box_quad_leaf_consistent(PG_FUNCTION_ARGS);

I guess they should go to the header file.


box-index-test.sql
Description: Binary data

-- 
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 patch: fix comments in contrib/pg_trgm/

[Robert Haas]

On Thu, Mar 17, 2016 at 5:38 AM, Aleksander Alekseev
 wrote:
> Here are a few more patches.

OK, I committed all three of these, minus some parts of
02-guc-c-typos.diff, plus the other patch you posted on
pgsql-committers.

The parts I left out of 02-guc-c-typos.diff included downcasing ->
downcasting where downcasing was in fact what it was intended to say;
plus some apparent whitespace fiddling that seems to me to be without
merit.

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


-- 
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: function parse_ident

[Teodor Sigaev]

I hope so the messages are ok now. Few more regress tests added.


Thank you, committed.

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


--
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] statistics for array types

[David Steele]

On 3/9/16 7:42 PM, David Steele wrote:
> On 1/18/16 11:24 AM, Alvaro Herrera wrote:
>> Alexander Korotkov wrote:
>>
>>> The patch implementing my idea above is attached.
>> What's the status here?  Jeff, did you have a look at Alexander's
>> version of your patch?  Tomas, does this patch satisfy your concerns?
> 
> This was marked as "needs review" but I have changed it to "waiting for
> author".

There has been no activity on this thread in quite a while and no
updated patch during this CF.  It has been marked "returned with
feedback". Please feel free to resubmit for 9.7!

-- 
-David
da...@pgmasters.net


-- 
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: fix lock contention for HASHHDR.mutex

[Robert Haas]

On Tue, Mar 1, 2016 at 9:43 AM, Aleksander Alekseev
 wrote:
>> I am not sure, if this is exactly what has been suggested by Robert,
>> so it is not straightforward to see if his suggestion can allow us to
>> use NUM_FREELISTS as 8 rather than 32.  I think instead of trying to
>> use FREELISTBUFF, why not do it as Robert has suggested and try with
>> different values of NUM_FREELISTS?
>
> Well, what I did is in fact a bit more general solution then Robert
> suggested. At first I just joined freeList and mutex arrays into one
> array and tried different NUM_FREELISTS values (see FREELISTS column).
> That was Robert's idea. Unfortunately it didn't give any considerable
> speedup comparing with previous approach.
>
> Then I tried to manually control sizeof every array item (see
> different SIZE values). By making it larger we can put every array item
> into a separate cache line. This approach helped a bit in terms of TPS
> (before: +33.9%, after: +34.2%) but only if NUM_FREELISTS remains the
> same (32).
>
> So answering your question - it turned out that we _can't_ reduce
> NUM_FREELISTS this way.

That's perplexing.  I would have expected that with all of the mutexes
packed in back-to-back like this, we would end up with a considerable
amount of false sharing.  I don't know why it ever helps to have an
array of bytes all in the same cache line of which each one is a
heavily-trafficked mutex.   Anybody else have a theory?

One other thing that concerns me mildly is the way we're handling
nentries.  It should be true, with this patch, that the individual
nentries sum to the right value modulo 2^32.  But I don't think
there's any guarantee that the values are positive any more, and in
theory after running long enough one of them could manage to overflow
or underflow.  So at a very minimum I think we need to remove the
Assert() the value is not negative.  But really, I wonder if we
shouldn't rework things a little more than that.

One idea is to jigger things so that we maintain a count of the total
number of entries that doesn't change except when we allocate, and
then for each freelist partition we maintain the number of entries in
that freelist partition.  So then the size of the hash table, instead
of being sum(nentries) is totalsize - sum(nfree).

Thoughts?

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

[ P.S. Sorry it's taken me a while to circle back around to 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] Parallel Aggregate

[David Rowley]

On 19 March 2016 at 09:53, Alvaro Herrera  wrote:
> I read this a bit, as an exercise to try to follow parallel query a bit.

Thanks for taking a look at this.

> I think the most interesting thing I have to say is that the new error
> messages in ExecInitExpr do not conform to our style.  Probably just
> downcase everything except Aggref and you're done, since they're
> can't-happen conditions anyway.  The comments below are mostly as I
> learn how the whole thing works so if I'm talking nonsense, I'm happy to
> be ignored or, better yet, educated.

Per a comment above from Robert I ended up just removing most of that
code due to the removal of Aggref->aggpartial. It did not seem worth
keeping aggpartial around just for these errors either, but let me
know if you think otherwise.


> I think the way we set the "consider_parallel" flag is a bit odd (we
> just "return" out of the function in the cases were it mustn't be set);
> but that mechanism is already part of set_rel_consider_parallel and
> similar to (but not quite like) longstanding routines such as
> set_rel_width, so nothing new in this patch.  I find this a bit funny
> coding, but then this is the planner so maybe it's okay.

hmm, I think its very similar to set_rel_consider_parallel(), as that
just does return for non-supported cases, and if it makes it until the
end of the function consider_parallel is set to true.

Would you rather see;

/* we can do nothing in parallel if there's no aggregates or group by */
if (!parse->hasAggs && parse->groupClause == NIL)
grouped_rel->consider_parallel = false;

/* grouping sets are currently not supported by parallel aggregate */
else if (parse->groupingSets)
grouped_rel->consider_parallel = false;

...?

> I think the comment on search_indexed_tlist_for_partial_aggref is a bit
> bogus; it says it returns an existing Var, but what it does is
> manufacture one itself.  I *think* the code is all right, but the
> comment seems misleading.

Ok, I've changed it to:

search_indexed_tlist_for_partial_aggref - find an Aggref in an indexed tlist

which seems to be equivalent to search_indexed_tlist_for_non_var()'s comment.

> In set_combineagg_references(), there are two calls to
> fix_combine_agg_expr(); I think the one hanging on the
> search_indexed_tlist_for_sortgroupref call is useless; you could just
> have the "if newexpr != NULL" in the outer block (after initializing to
> NULL before the ressortgroupref check).

Yes, but see set_upper_references(), it just follows the pattern there
but calls fix_combine_agg_expr() instead of fix_upper_expr(). For
simplicity of review it seems to be nice to keep it following the same
pattern.

> set_combineagg_references's comment says it does something "similar to
> set_upper_references, and additionally" some more stuff, but reading the
> code for both functions I think that's not quite true.  I think the
> comment should say that both functions are parallel, but one works for
> partial aggs and the other doesn't.

Ok, I've changed the comment to:
/*
 * set_combineagg_references
 *  This does a similar job as set_upper_references(), but treats Aggrefs
 *  in a different way. Here we transforms Aggref nodes args to suit the
 *  combine aggregate phase. This means that the Aggref->args are converted
 *  to reference the corresponding aggregate function in the subplan rather
 *  than simple Var(s), as would be the case for a non-combine aggregate
 *  node.
 */

> Actually, what happens if you feed
> an agg plan with combineStates to set_upper_references?  If it still
> works but the result is not optimal, maybe we should check against that
> case, so as to avoid the case where somebody hacks this further and the
> plans are suddenly not agg-combined anymore.  What I actually expect to
> happen is that something would explode during execution; in that case
> perhaps it's better to add a comment?  (In further looking at other
> setrefs.c similar functions, maybe it's fine the way you have it.)

This simply won't work as this is the code which causes the
sum((sum(num))) in the combine aggregate's target list.
The normal code is trying to look for Vars, but this code is trying to
find that sum(num) inside the subplan target list.

Example EXPLAIN VERBOSE output:
Finalize Aggregate  (cost=105780.67..105780.68 rows=1 width=8)
  Output: pg_catalog.sum((sum(num)))

Try commenting out;

if (aggplan->combineStates)
set_combineagg_references(root, plan, rtoffset);
else

to see the horrors than ensue. It'll basically turn aggregate
functions in to a rather inefficient random number generator. This is
because the combine Aggref->args still point to "num", instead of
sum(num).

> Back at make_partialgroup_input_target, the comment says "so that they
> can be found later in Combine Aggregate nodes during setrefs".  I think
> it's better to be explicit and say ".. can be found later during
> set_combineagg_references" or something.

Changed. Thanks for the review.