Re: [HACKERS] Parallel build with MSVC

[Michael Paquier]

On Tue, Apr 26, 2016 at 10:09 PM, Christian Ullrich
 wrote:
> The first patch passes the value of the MSBFLAGS environment variable to
> msbuild's command line.
>
> The output of parallel and sequential builds has identical file count and
> size after installing; all tests pass.

If a committer is willing to pick up that, I'd say go for it.
Personally, I have modified some of my windows build scripts in this
area to pass additional flags. Not sure that there are many people in
a similar case than me around though :)

> Even without /m, MSBuild will already run enough compilers to keep all CPUs
> busy whenever it can do that with only a single project's files. With /m, it
> will also process _projects_ in parallel.
>
> One additional fix required is to put gendef.pl's "symbols.out" file into
> the directory it is working on, rather than the source tree root, to avoid
> collisions that cause the parallel build to fail otherwise.

 
-build DEBUG
+build -c DEBUG
 
To build just a single project, for example psql, run the commands:
 
 build psql
-build DEBUG psql
+build -c DEBUG psql
 
Why is that? Your patch just has a look at argv[0] to see if that's a
debug or release build.

+use File::Spec::Functions qw(splitpath catpath);
This is present since at least perl 5.8, so that's not a blocker.

vcbuild also supports /m. Wouldn't it make sense to have a environment
variable flag for it as well? vcbuild has been replaced by msbuild in
VS2010 but I would think that in back-branches this would have value
for users still compiling with VS2008 or older, and those are still
supported things.
-- 
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] Bogus cleanup code in PostgresNode.pm

[Michael Paquier]

On Tue, Apr 26, 2016 at 2:24 PM, Tom Lane  wrote:
> Now, whether using END is really an improvement is a separate question.
> I have the impression that END calls happen in a better-defined order,
> but I'm not a perl monk.

For the archive's sake, 08af9219 is the result commit.
-- 
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] Removing faulty hyperLogLog merge function

[Michael Paquier]

On Wed, Apr 27, 2016 at 12:08 PM, Joe Conway  wrote:
> On 04/26/2016 07:23 PM, Robert Haas wrote:
>> On Tue, Apr 26, 2016 at 9:35 PM, Tom Lane  wrote:
>>> Robert Haas  writes:
 I'm not prepared to commit this over the objection offered by Tomas
 Vondra on that thread.
>>>
>>> FWIW, I agree with Peter that we should remove this code.  We know that it
>>> is buggy.  Leaving it there constitutes an "attractive nuisance" --- that
>>> is, I'm afraid that someone will submit a patch that depends on that
>>> function, and that we might forget that the function is broken and commit
>>> said patch.
>>>
>>> Tomas' objection would be reasonable if a fix was simple, but so far as
>>> I can tell from the thread, it's not.  In particular, Peter doesn't trust
>>> the upstream patch in question.  But whether or not you trust it, doing
>>> nothing is not a sane choice.  The reasonable alternatives are to remove
>>> the merge function or sync the upstream patch.
>>
>> Now I agree with that.  And now we do not have a 1-1 tie on which
>> alternative to prefer, which is a good start towards a consensus.  Any
>> other views?
>
> I haven't followed this issue all that closely, but to me it seems
> pretty clear. If the function is brand new to 9.6, buggy, and not even
> used anywhere, I cannot imagine why we would leave it in the tree.

+1. We should definitely not encourage its use for 3rd-part plugins.
-- 
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] Suspicious behaviour on applying XLOG_HEAP2_VISIBLE.

[Michael Paquier]

On Wed, Apr 27, 2016 at 12:25 AM, Robert Haas  wrote:
> In other words, I think Masahiko Sawada's patch in the original post
> is pretty much right on target, except that we don't need to do that
> always, but rather only in the FPI case when the call to
> visibilitymap_pin() is being optimized away.  If we solve the problem
> that way, I don't think we even need a new WAL record for this case,
> which is a non-trivial fringe benefit.

The visibility map is not the only thing that need to be addressed,
no? For example take this report from Dmitry Vasilyev of a couple of
months back where index relations are not visible on a standby:
http://www.postgresql.org/message-id/CAB-SwXY6oH=9twbkxjtgr4uc1nqt-vpyatxcseme62adwyk...@mail.gmail.com
This is really leading to a solution where we need to take a more
general approach to this problem instead of trying to patch multiple
WAL replay code paths. And Andres' stuff does so.
-- 
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] pg_dump dump catalog ACLs

[Noah Misch]

A later thought:

On Mon, Apr 25, 2016 at 12:39:09AM -0400, Stephen Frost wrote:
> src/bin/pg_dump: make check
> 
> implemented using the TAP testing system.  There are a total of 360
> tests, generally covering:
> 
> Various invalid sets of command-line options.
> 
> Valid command-line options (generally independently used):
> 
>   (no options- defaults)
>   --clean
>   --clean --if-exists
>   --data-only
>   --format=c (tested with pg_restore)
>   --format=d (tested with pg_restore)
>   --format=t (tested with pg_restore)
>   --format=d --jobs=2 (tested with pg_restore)
>   --exclude-schema
>   --exclude-table
>   --no-privileges
>   --no-owner
>   --schema
>   --schema-only
> 
> Note that this is only including tests for basic schemas, tables, data,
> and privileges, so far.  I believe it's pretty obvious that we want to
> include all object types and include testing of extensions, I just
> haven't gotten there yet and figured now would be a good time to solicit
> feedback on the approach I've used.
> 
> I'm not sure how valuable it is to test all the different combinations
> of command-line options, though clearly some should be tested (eg:
> include-schema, exclude table in that schema, and similar cases).

You mention that you test valid options independently.  Keep an eye out for
good opportunities to save runtime by testing multiple options per invocation.
To give one example, --exclude-table seems fairly independent of --format;
maybe those could test as a group.  That complicates the suite, but saving ten
or more seconds might justify the complexity.


-- 
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] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

On Wed, Apr 27, 2016 at 10:14 AM, Kyotaro HORIGUCHI
 wrote:
> I just added a comment in the v9.

Sorry, I have attached an empty patch. This is another one that should
be with content.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center


fix_sync_rep_update_conf_v9.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] EXPLAIN VERBOSE with parallel Aggregate

[David Rowley]

On 27 April 2016 at 15:12, Robert Haas  wrote:
> On Tue, Apr 26, 2016 at 10:57 PM, David Rowley
>  wrote:
>> On 27 April 2016 at 14:30, Robert Haas  wrote:
>>> On Tue, Apr 26, 2016 at 9:56 PM, Robert Haas  wrote:
 On Tue, Apr 26, 2016 at 9:14 PM, David Rowley
  wrote:
> I'd also have expected the output of both partial nodes to be the
> same, i.e. both prefixed with PARTIAL. Is it intended that they don't?
> or have I made some other mistake?

 No, that's a defect in the patch.  I didn't consider that we need to
 support nodes with finalizeAggs = false and combineStates = true,
 which is why that ERROR was there.  Working on a fix now.
>>>
>>> I think this version should work, provided you use
>>> partial_grouping_target where needed.
>>
>> +static void get_special_variable(Node *node, deparse_context *context,
>> + void *private);
>>
>> "private" is reserved in C++? I understood we want our C code to
>> compile as C++ too, right? or did I get my wires crossed somewhere?
>
> I can call it something other than "private", if you have a
> suggestion; normally I would have used "context", but that's already
> taken in this case.  private_context would work, I guess.

It's fine. After Andres' email I looked and saw many other usages of
C++ keywords in our C code. Perhaps it would be a good idea to name it
something else we wanted to work towards it, but it sounds like it's
not, so probably keep what you've got.

The patch looks good. The only other thing I thought about was perhaps
it would be a good idea to re-add the sanity checks in execQual.c.
Patch for that is attached.

I removed the aggoutputtype check, as I only bothered adding that in
the first place because I lost the aggpartial field in some previous
revision of the parallel aggregate developments. I'd say the
aggpartial check makes it surplus to requirements.

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


execQual_sanity_checks.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] [BUGS] Breakage with VACUUM ANALYSE + partitions

[Andres Freund]

Hi,

On 2016-04-25 20:53:26 +0200, Fabien COELHO wrote:
> I have just a small naming point:
> 
>   /* ereport if segment not present, create in recovery */
>   EXTENSION_FAIL,
>   /* return NULL if not present, create in recovery */
>   EXTENSION_RETURN_NULL,
>   /* return NULL if not present */
>   EXTENSION_REALLY_RETURN_NULL,
>   /* create new segments as needed */
>   EXTENSION_CREATE
> 
> The comments seem pretty clear, but the naming of these options are more
> behavioral than functional somehow (or the reverse?), especially the
> RETURN_NULL and REALLY_RETURN_NULL names seemed pretty contrived to me.

I tend to agree. But "fixing" that would mean changing quite some
additional pieces of code, more than I want to do in a bugfix. I also
think it's not *that* confusing...

Thanks for looking.

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] Optimization for updating foreign tables in Postgres FDW

[Etsuro Fujita]

On 2016/04/26 21:45, Etsuro Fujita wrote:

While re-reviewing the fix, I noticed that since PQcancel we added to
pgfdw_xact_callback to cancel a DML pushdown query isn't followed by a
ROLLBACK, the connection to the remote server will be discarded at the
end of the while loop in that function, which will cause a FATAL error
of "connection to client lost".  Probably, that was proposed by me in
the first version of the patch, but I don't think that's a good idea.
Shouldn't we execute ROLLBACK after that PQcancel?

Another thing I noticed is, ISTM that we miss the case where DML
pushdown queries are performed in subtransactions.  I think cancellation
logic would also need to be added to pgfdw_subxact_callback.


Attached is a patch for that.

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/connection.c
--- b/contrib/postgres_fdw/connection.c
***
*** 677,684  pgfdw_xact_callback(XactEvent event, void *arg)
  	 * using an asynchronous execution function, the command
  	 * might not have yet completed.  Check to see if a command
  	 * is still being processed by the remote server, and if so,
! 	 * request cancellation of the command; if not, abort
! 	 * gracefully.
  	 */
  	if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE)
  	{
--- 677,683 
  	 * using an asynchronous execution function, the command
  	 * might not have yet completed.  Check to see if a command
  	 * is still being processed by the remote server, and if so,
! 	 * request cancellation of the command.
  	 */
  	if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE)
  	{
***
*** 694,700  pgfdw_xact_callback(XactEvent event, void *arg)
  errbuf)));
  			PQfreeCancel(cancel);
  		}
- 		break;
  	}
  
  	/* If we're aborting, abort all remote transactions too */
--- 693,698 
***
*** 798,803  pgfdw_subxact_callback(SubXactEvent event, SubTransactionId mySubid,
--- 796,825 
  		{
  			/* Assume we might have lost track of prepared statements */
  			entry->have_error = true;
+ 
+ 			/*
+ 			 * If a command has been submitted to the remote server by using an
+ 			 * asynchronous execution function, the command might not have yet
+ 			 * completed.  Check to see if a command is still being processed by
+ 			 * the remote server, and if so, request cancellation of the
+ 			 * command.
+ 			 */
+ 			if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE)
+ 			{
+ PGcancel   *cancel;
+ char		errbuf[256];
+ 
+ if ((cancel = PQgetCancel(entry->conn)))
+ {
+ 	if (!PQcancel(cancel, errbuf, sizeof(errbuf)))
+ 		ereport(WARNING,
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+  errmsg("could not send cancel request: %s",
+ 		errbuf)));
+ 	PQfreeCancel(cancel);
+ }
+ 			}
+ 
  			/* Rollback all remote subtransactions during abort */
  			snprintf(sql, sizeof(sql),
  	 "ROLLBACK TO SAVEPOINT s%d; RELEASE SAVEPOINT s%d",

-- 
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] EXPLAIN VERBOSE with parallel Aggregate

[Robert Haas]

On Tue, Apr 26, 2016 at 10:57 PM, David Rowley
 wrote:
> On 27 April 2016 at 14:30, Robert Haas  wrote:
>> On Tue, Apr 26, 2016 at 9:56 PM, Robert Haas  wrote:
>>> On Tue, Apr 26, 2016 at 9:14 PM, David Rowley
>>>  wrote:
 I'd also have expected the output of both partial nodes to be the
 same, i.e. both prefixed with PARTIAL. Is it intended that they don't?
 or have I made some other mistake?
>>>
>>> No, that's a defect in the patch.  I didn't consider that we need to
>>> support nodes with finalizeAggs = false and combineStates = true,
>>> which is why that ERROR was there.  Working on a fix now.
>>
>> I think this version should work, provided you use
>> partial_grouping_target where needed.
>
> +static void get_special_variable(Node *node, deparse_context *context,
> + void *private);
>
> "private" is reserved in C++? I understood we want our C code to
> compile as C++ too, right? or did I get my wires crossed somewhere?

I can call it something other than "private", if you have a
suggestion; normally I would have used "context", but that's already
taken in this case.  private_context would work, I guess.

-- 
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] Removing faulty hyperLogLog merge function

[Joe Conway]

On 04/26/2016 07:23 PM, Robert Haas wrote:
> On Tue, Apr 26, 2016 at 9:35 PM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> I'm not prepared to commit this over the objection offered by Tomas
>>> Vondra on that thread.
>>
>> FWIW, I agree with Peter that we should remove this code.  We know that it
>> is buggy.  Leaving it there constitutes an "attractive nuisance" --- that
>> is, I'm afraid that someone will submit a patch that depends on that
>> function, and that we might forget that the function is broken and commit
>> said patch.
>>
>> Tomas' objection would be reasonable if a fix was simple, but so far as
>> I can tell from the thread, it's not.  In particular, Peter doesn't trust
>> the upstream patch in question.  But whether or not you trust it, doing
>> nothing is not a sane choice.  The reasonable alternatives are to remove
>> the merge function or sync the upstream patch.
> 
> Now I agree with that.  And now we do not have a 1-1 tie on which
> alternative to prefer, which is a good start towards a consensus.  Any
> other views?

I haven't followed this issue all that closely, but to me it seems
pretty clear. If the function is brand new to 9.6, buggy, and not even
used anywhere, I cannot imagine why we would leave it in the tree.

Joe

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] EXPLAIN VERBOSE with parallel Aggregate

[Andres Freund]

On 2016-04-27 14:57:24 +1200, David Rowley wrote:
> "private" is reserved in C++? I understood we want our C code to
> compile as C++ too, right? or did I get my wires crossed somewhere?

Just headers. Our C code unfortunately is very far away from being C++
compliant.


-- 
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] EXPLAIN VERBOSE with parallel Aggregate

[David Rowley]

On 27 April 2016 at 14:30, Robert Haas  wrote:
> On Tue, Apr 26, 2016 at 9:56 PM, Robert Haas  wrote:
>> On Tue, Apr 26, 2016 at 9:14 PM, David Rowley
>>  wrote:
>>> I'd also have expected the output of both partial nodes to be the
>>> same, i.e. both prefixed with PARTIAL. Is it intended that they don't?
>>> or have I made some other mistake?
>>
>> No, that's a defect in the patch.  I didn't consider that we need to
>> support nodes with finalizeAggs = false and combineStates = true,
>> which is why that ERROR was there.  Working on a fix now.
>
> I think this version should work, provided you use
> partial_grouping_target where needed.

+static void get_special_variable(Node *node, deparse_context *context,
+ void *private);

"private" is reserved in C++? I understood we want our C code to
compile as C++ too, right? or did I get my wires crossed somewhere?


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

Re: [HACKERS] Parallel SAFE information missing in CREATE OR REPLACE FUNCTION definition

[Robert Haas]

On Tue, Apr 26, 2016 at 1:24 AM, Ashutosh Sharma  wrote:
> In PGSQL-9.6, if we create a function with PARALLEL clause and try
> displaying it's definition using "pg_get_functiondef" we see that the
> PARALLEL keyword used during function creation is missing.
>
> Below are the steps to reproduce:
>
> postgres=# CREATE FUNCTION add(integer, integer) RETURNS integer
> postgres-# AS 'select $1 + $2;'
> postgres-# LANGUAGE SQL
> postgres-# PARALLEL SAFE
> postgres-# RETURNS NULL ON NULL INPUT;
> CREATE FUNCTION
>
> postgres=# CREATE OR REPLACE FUNCTION increment(i integer) RETURNS integer
> AS $$
> postgres$# BEGIN
> postgres$# RETURN i + 1;
> postgres$# END;
> postgres$# $$ LANGUAGE plpgsql PARALLEL SAFE;
> CREATE FUNCTION
>
> postgres=# CREATE FUNCTION square_root(double precision) RETURNS double
> precision
> AS 'dsqrt'
> LANGUAGE internal
> PARALLEL SAFE;
> CREATE FUNCTION
>
> postgres=# \df
>List of functions
>  Schema |Name | Result data type | Argument data types |  Type
> +-+--+-+
>  public | add | integer  | integer, integer| normal
>  public | increment   | integer  | i integer   | normal
>  public | square_root | double precision | double precision| normal
> (3 rows)
>
> postgres=# SELECT  pg_get_functiondef('add'::regproc);
>pg_get_functiondef
> -
>  CREATE OR REPLACE FUNCTION public.add(integer, integer)+
>   RETURNS integer   +
>   LANGUAGE sql  +
>   STRICT+
>  AS $function$select $1 + $2;$function$ +
>
> (1 row)
>
> postgres=# SELECT  pg_get_functiondef('increment'::regproc);
>pg_get_functiondef
> 
>  CREATE OR REPLACE FUNCTION public.increment(i integer)+
>   RETURNS integer  +
>   LANGUAGE plpgsql +
>  AS $function$ +
>  BEGIN +
>  RETURN i + 1; +
>  END;  +
>  $function$+
>
> (1 row)
>
> postgres=# SELECT  pg_get_functiondef('square_root'::regproc);
>pg_get_functiondef
> -
>  CREATE OR REPLACE FUNCTION public.square_root(double precision)+
>   RETURNS double precision  +
>   LANGUAGE internal +
>  AS $function$dsqrt$function$   +
>
> (1 row)
>
>
> RCA: The proparallel information for a function is not considered while
> preparing its definition inside pg_get_functiondef().
>
> Solution: Add a  check for the proparallel flag inside pg_get_functiondef()
> and based on the value in proparallel flag, store the parallel {safe |
> unsafe | restricted} info in the buffer  that holds the function definition.
> PFA patch to fix the issue.

Thanks, committed.  Boy, I feel stupid.

-- 
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] plan for beta1 & open issues

[Robert Haas]

On Mon, Apr 11, 2016 at 12:16 PM, Robert Haas  wrote:
> The PostgreSQL 9.6 release management team has determined that
> PostgreSQL 9.6beta1 should wrap on May 9, 2016 for release on May
> 12,2016, ...

Update: It looks like this plan is going to stick.  So, the plan of
record is still to wrap 9.6beta1 and back-branches on May 9th.

-- 
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] EXPLAIN VERBOSE with parallel Aggregate

[Robert Haas]

On Tue, Apr 26, 2016 at 9:56 PM, Robert Haas  wrote:
> On Tue, Apr 26, 2016 at 9:14 PM, David Rowley
>  wrote:
>> I'd also have expected the output of both partial nodes to be the
>> same, i.e. both prefixed with PARTIAL. Is it intended that they don't?
>> or have I made some other mistake?
>
> No, that's a defect in the patch.  I didn't consider that we need to
> support nodes with finalizeAggs = false and combineStates = true,
> which is why that ERROR was there.  Working on a fix now.

I think this version should work, provided you use
partial_grouping_target where needed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index a21928b..20e38f0 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -1244,6 +1244,8 @@ _copyAggref(const Aggref *from)
 	COPY_NODE_FIELD(aggfilter);
 	COPY_SCALAR_FIELD(aggstar);
 	COPY_SCALAR_FIELD(aggvariadic);
+	COPY_SCALAR_FIELD(aggpartial);
+	COPY_SCALAR_FIELD(aggcombine);
 	COPY_SCALAR_FIELD(aggkind);
 	COPY_SCALAR_FIELD(agglevelsup);
 	COPY_LOCATION_FIELD(location);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 3c6c567..c5ccc42 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -202,6 +202,8 @@ _equalAggref(const Aggref *a, const Aggref *b)
 	COMPARE_NODE_FIELD(aggfilter);
 	COMPARE_SCALAR_FIELD(aggstar);
 	COMPARE_SCALAR_FIELD(aggvariadic);
+	COMPARE_SCALAR_FIELD(aggcombine);
+	COMPARE_SCALAR_FIELD(aggpartial);
 	COMPARE_SCALAR_FIELD(aggkind);
 	COMPARE_SCALAR_FIELD(agglevelsup);
 	COMPARE_LOCATION_FIELD(location);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 5ac7446..c2f0e0f 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1040,6 +1040,8 @@ _outAggref(StringInfo str, const Aggref *node)
 	WRITE_NODE_FIELD(aggfilter);
 	WRITE_BOOL_FIELD(aggstar);
 	WRITE_BOOL_FIELD(aggvariadic);
+	WRITE_BOOL_FIELD(aggcombine);
+	WRITE_BOOL_FIELD(aggpartial);
 	WRITE_CHAR_FIELD(aggkind);
 	WRITE_UINT_FIELD(agglevelsup);
 	WRITE_LOCATION_FIELD(location);
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 8059594..6f28047 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -556,6 +556,8 @@ _readAggref(void)
 	READ_NODE_FIELD(aggfilter);
 	READ_BOOL_FIELD(aggstar);
 	READ_BOOL_FIELD(aggvariadic);
+	READ_BOOL_FIELD(aggcombine);
+	READ_BOOL_FIELD(aggpartial);
 	READ_CHAR_FIELD(aggkind);
 	READ_UINT_FIELD(agglevelsup);
 	READ_LOCATION_FIELD(location);
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 5537c14..266e830 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -2100,6 +2100,10 @@ search_indexed_tlist_for_partial_aggref(Aggref *aggref, indexed_tlist *itlist,
 continue;
 			if (aggref->aggvariadic != tlistaggref->aggvariadic)
 continue;
+			/*
+			 * it would be harmless to compare aggcombine and aggpartial, but
+			 * it's also unnecessary
+			 */
 			if (aggref->aggkind != tlistaggref->aggkind)
 continue;
 			if (aggref->agglevelsup != tlistaggref->agglevelsup)
@@ -2463,6 +2467,7 @@ fix_combine_agg_expr_mutator(Node *node, fix_upper_expr_context *context)
 			newtle = makeTargetEntry((Expr *) newvar, 1, NULL, false);
 			newaggref = (Aggref *) copyObject(aggref);
 			newaggref->args = list_make1(newtle);
+			newaggref->aggcombine = true;
 
 			return (Node *) newaggref;
 		}
diff --git a/src/backend/optimizer/util/tlist.c b/src/backend/optimizer/util/tlist.c
index 4c8c83d..aa2c2f8 100644
--- a/src/backend/optimizer/util/tlist.c
+++ b/src/backend/optimizer/util/tlist.c
@@ -792,6 +792,9 @@ apply_partialaggref_adjustment(PathTarget *target)
 			else
 newaggref->aggoutputtype = aggform->aggtranstype;
 
+			/* flag it as partial */
+			newaggref->aggpartial = true;
+
 			lfirst(lc) = newaggref;
 
 			ReleaseSysCache(aggTuple);
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 1b8f0ae..75b26dd 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -392,6 +392,11 @@ static void get_rule_windowspec(WindowClause *wc, List *targetList,
 	deparse_context *context);
 static char *get_variable(Var *var, int levelsup, bool istoplevel,
 			 deparse_context *context);
+static void get_special_variable(Node *node, deparse_context *context,
+	 void *private);
+static void resolve_special_varno(Node *node, deparse_context *context,
+	  void *private,
+	  void (*callback) (Node *, deparse_context *, void *));
 static Node *find_param_referent(Param *param, deparse_context *context,
 	deparse_namespace **dpns_p, ListCell **ancestor_cell_p);
 static void get_parameter(Param *param, deparse_context *context);
@@ -407,7 +412,10 @@ static void get_rule_expr_toplevel(Node *node, 

Re: [HACKERS] Removing faulty hyperLogLog merge function

[Robert Haas]

On Tue, Apr 26, 2016 at 9:35 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> I'm not prepared to commit this over the objection offered by Tomas
>> Vondra on that thread.
>
> FWIW, I agree with Peter that we should remove this code.  We know that it
> is buggy.  Leaving it there constitutes an "attractive nuisance" --- that
> is, I'm afraid that someone will submit a patch that depends on that
> function, and that we might forget that the function is broken and commit
> said patch.
>
> Tomas' objection would be reasonable if a fix was simple, but so far as
> I can tell from the thread, it's not.  In particular, Peter doesn't trust
> the upstream patch in question.  But whether or not you trust it, doing
> nothing is not a sane choice.  The reasonable alternatives are to remove
> the merge function or sync the upstream patch.

Now I agree with that.  And now we do not have a 1-1 tie on which
alternative to prefer, which is a good start towards a consensus.  Any
other views?

-- 
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] SET ROLE and reserved roles

[Alvaro Herrera]

Robert Haas wrote:
> On Mon, Apr 25, 2016 at 6:55 PM, Stephen Frost  wrote:

> > Working up a patch to remove these checks should be pretty quickly done
> > (iirc, I've actually got an independent patch around from when I added
> > them, just need to find it and then go through the committed patches to
> > make sure I take care of everything), but would like to make sure that
> > we're now all on the same page and that *all* of these checks should be
> > removed, making default roles just exactly like "regular" roles, except
> > that they're created at initdb time and have "special" rights provided
> > by C-level code checks.
> 
> That's what I'm thinking.  I would welcome other views.

I agree with that view, assuming pg_dump correctly produces GRANTs to
replicate what was done in a database regarding those roles (and same
with pg_dumpall -g).  I think it already works that way, but I may be
mistaken.

-- 
Á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] xlc atomics

[Noah Misch]

On Mon, Apr 25, 2016 at 11:52:04AM -0700, Andres Freund wrote:
> On 2016-04-23 21:54:07 -0400, Noah Misch wrote:
> > The bug is that the pg_atomic_compare_exchange_*() specifications
> > grant "full barrier semantics", but generic-xlc.h provided only the
> > semantics of an acquire barrier.
> 
> I find the docs at
> http://www-01.ibm.com/support/knowledgecenter/SSGH3R_13.1.2/com.ibm.xlcpp131.aix.doc/compiler_ref/bifs_sync_atomic.html
> to be be awfully silent about that matter. I guess you just looked at
> the assembler code?  It's nice that one can figure out stuff like that
> from an architecture manual, but it's sad that the docs for the
> intrinsics is silent about that matter.

Right.  The intrinsics provide little value as abstractions if one checks the
generated code to deduce how to use them.  I was tempted to replace the
intrinsics calls with inline asm.  At least these functions are unlikely to
change over time.

> Except that I didn't verify the rs6000_pre_atomic_barrier() and
> __fetch_and_add() internals about emitted sync/isync, the patch looks
> good.   We've so far not referred to "sequential consistency", but given
> it's rise in popularity, I don't think it hurts.

Thanks; committed.


-- 
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] EXPLAIN VERBOSE with parallel Aggregate

[Robert Haas]

On Tue, Apr 26, 2016 at 9:14 PM, David Rowley
 wrote:
> I'd also have expected the output of both partial nodes to be the
> same, i.e. both prefixed with PARTIAL. Is it intended that they don't?
> or have I made some other mistake?

No, that's a defect in the patch.  I didn't consider that we need to
support nodes with finalizeAggs = false and combineStates = true,
which is why that ERROR was there.  Working on a fix now.

-- 
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] Removing faulty hyperLogLog merge function

[Tom Lane]

Robert Haas  writes:
> I'm not prepared to commit this over the objection offered by Tomas
> Vondra on that thread.

FWIW, I agree with Peter that we should remove this code.  We know that it
is buggy.  Leaving it there constitutes an "attractive nuisance" --- that
is, I'm afraid that someone will submit a patch that depends on that
function, and that we might forget that the function is broken and commit
said patch.

Tomas' objection would be reasonable if a fix was simple, but so far as
I can tell from the thread, it's not.  In particular, Peter doesn't trust
the upstream patch in question.  But whether or not you trust it, doing
nothing is not a sane choice.  The reasonable alternatives are to remove
the merge function or sync the upstream patch.

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] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

Hello,

At Tue, 26 Apr 2016 09:57:50 +0530, Amit Kapila  wrote 
in 
> > > > Amit Kapila  writes:
> > > > > The main point for this improvement is that the handling for guc
> > s_s_names
> > > > > is not similar to what we do for other somewhat similar guc's and
> > which
> > > > > causes in-efficiency in non-hot code path (less used code).
> > > >
> > > > This is not about efficiency, this is about correctness.  The proposed
> > > > v7 patch is flat out not acceptable, not now and not for 9.7 either,
> > > > because it introduces a GUC assign hook that can easily fail (eg,
> > through
> > > > out-of-memory for the copy step).  Assign hook functions need to be
> > > > incapable of failure.
> 
> 
> It seems to me that similar problem can be there
> for assign_pgstat_temp_directory() as it can also lead to "out of memory"
> error.  However, in general I understand your concern and I think we should
> avoid any such failure in assign functions.

I noticed that forgetting error handling of malloc then searched
for the callers of guc_malloc just now and found the same
thing. This should be addressed as another issue.

> > > > You are going to
> > > > need to find a way to package the parse result into a single malloc'd
> > > > blob, though, because that's as much as guc.c can keep track of for an
> > > > "extra" value.
> > >
> > > Ok, I'll post the v8 with the blob solution sooner.
> >
> > Hmm. It was way easier than I thought. The attached v8 patch does,
...
> + /* Convert SyncRepConfig into the packed struct fit to guc extra */
> 
> + pconf = (SyncRepConfigData *)
> 
> + malloc(SizeOfSyncRepConfig(
> 
> +   list_length(syncrep_parse_result->members)));
> 
> I think there should be a check for malloc failure in above code.

Yes, I'm ashamed to have forgotten what I mentioned just
before. Added the same thing with guc_malloc. The error is at
ERROR since parsing GUC files should continue on parse errors
(and seeing check_log_destination).


> + /* No further need for syncrep_parse_result */
> 
> + syncrep_parse_result = NULL;
> 
> Isn't this a memory leak?  Shouldn't we need to free the corresponding
> memory as well.

It is palloc'ed on the current context, which AFAICS would be
'config file processing' or 'PortalHeapMemory'for the ALTER
SYSTEM case. Both of them are rather short-living. I don't think
that leaving them is a problem on both of the cases and there's
no point freeing only it among those (if any) allocated in the
generated code by bison and flex... I suppose.

I just added a comment in the v9.

|   * No further need for syncrep_parse_result. The memory blocks are
|   * released along with the deletion of the current context.

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] EXPLAIN VERBOSE with parallel Aggregate

[David Rowley]

On 27 April 2016 at 12:37, Robert Haas  wrote:
> On Tue, Apr 26, 2016 at 6:44 PM, David Rowley
>  wrote:
>> On 27 April 2016 at 08:46, Robert Haas  wrote:
>>> My proposed fix for this issue is attached.  Review is welcome;
>>> otherwise, I'll just commit this.  The output looks like what I
>>> suggested upthread:
>>
>> + if (!aggref->aggpartial)
>> + elog(ERROR, "referenced Aggref is not partial");
>>
>> I think this is overly restrictive; ruleutils seems a bit out of line
>> here to say that plans can't have > 1 combine aggregate node.
>>
>> When coding the combine aggs stuff I had test code to inject
>> additional combine paths to make sure everything worked as expected.
>> It did, but it won't if you add these two lines. I'd say just remove
>> them.
>>
>> If you apply the attached and execute;
>>
>> create table t1 (num int not null);
>> insert into t1 select generate_series(1,200);
>>
>> explain verbose select avg(num) from t1;
>>
>> You'll get;
>>
>> ERROR: referenced Aggref is not partial
>
> In this test patch, should aggpath be using partial_grouping_target
> rather than target?  I feel like we need to use
> partial_grouping_target unless finalizeAggs is true.

Yes. I should also have removed the HAVING clause from the path.

After having changed that, I do still get the error.

I changed it to a NOTICE for now;

# explain verbose select avg(num) from t1 having sum(num) > 0;
NOTICE:  referenced Aggref is not partial
NOTICE:  referenced Aggref is not partial
QUERY PLAN
---
 Finalize Aggregate  (cost=22350.24..22350.25 rows=1 width=32)
   Output: avg(num)
   Filter: (sum(t1.num) > 0)
   ->  Partial Aggregate  (cost=22350.22..22350.23 rows=1 width=40)
 Output: avg(num), sum(num)
 ->  Gather  (cost=22350.00..22350.21 rows=2 width=40)
   Output: (PARTIAL avg(num)), (PARTIAL sum(num))
   Workers Planned: 2
   ->  Partial Aggregate  (cost=21350.00..21350.01 rows=1 width=40)
 Output: PARTIAL avg(num), PARTIAL sum(num)
 ->  Parallel Seq Scan on public.t1
(cost=0.00..17183.33 rows=83 width=4)
   Output: num
(12 rows)

You can see that the middle aggregate node properly includes the
sum(num) from the having clause, so is using the partial target list.

I'd also have expected the output of both partial nodes to be the
same, i.e. both prefixed with PARTIAL. Is it intended that they don't?
or have I made some other mistake?

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

Re: [HACKERS] Re: [COMMITTERS] pgsql: pg_upgrade: Convert old visibility map format to new format.

[Masahiko Sawada]

On Wed, Apr 27, 2016 at 10:00 AM, Robert Haas  wrote:
> On Tue, Apr 26, 2016 at 8:45 PM, Bruce Momjian  wrote:
>> On Fri, Mar 11, 2016 at 05:36:34PM +, Robert Haas wrote:
>>> pg_upgrade: Convert old visibility map format to new format.
>>>
>>> Commit a892234f830e832110f63fc0a2afce2fb21d1584 added a second bit per
>>> page to the visibility map, but pg_upgrade has been unaware of it up
>>> until now.  Therefore, a pg_upgrade from an earlier major release of
>>> PostgreSQL to any commit preceding this one and following the one
>>> mentioned above would result in invalid visibility map contents on the
>>> new cluster, very possibly leading to data corruption.  This plugs
>>> that hole.
>>>
>>> Masahiko Sawada, reviewed by Jeff Janes, Bruce Momjian, Simon Riggs,
>>> Michael Paquier, Andres Freund, me, and others.
>>
>> I have tested the current git trees of all supported versions of
>> Postgres in all possible pg_upgrade combinations and they all worked
>> perfectly.

Thank you for testing!
Good news for me.

>
> That's good!
>
> All the commits related to this topic could use extra-careful review
> and testing.  If Masahiko-san got anything wrong, or I did, this could
> eat people's data in ways that are very hard to track down.
>

Yeah, we should test this feature in various ways, and I'm going to do so.

Regards,

--
Masahiko Sawada


-- 
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] Background Processes and reporting

[Bruce Momjian]

On Sat, Mar 12, 2016 at 08:33:55PM +0300, Vladimir Borodin wrote:
> That’s why proposal included GUC for that with a default to turn timings
> measuring off. I don’t remember any objections against that.
> 
> And I’m absolutely sure that a real highload production (which of course
> doesn’t use virtualization and windows) can’t exist without measuring timings.
> Oracle guys have written several chapters (!) about that [0]. Long story 
> short,
> sampling doesn’t give enough precision. I have shown overhead [1] on bare 
> metal
> linux with high stressed lwlocks worload. BTW Oracle doesn’t give you any ways
> to turn timings measurement off, even with hidden parameters. All other
> commercial databases have waits monitoring with timings measurement. Let’s do
> it and turn it off by default so that all other platforms don’t suffer from 
> it.

I realize that users of other databases have found sampling to be
insufficient, but I think we need to use the Postgres sampling method in
production to see if it is insufficient for Postgres as well.  We can't
design based on the limitations of other databases.

Also, we have enabled the sampling method in 9.6 that we know can be
enabled on every platform with limited overhead.  We can add additional
potential-overhead methods in later releases.

Frankly, it would be odd to add these features in the opposite order,
and the stubbornness of some in this thread to understand that is
concerning.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +


-- 
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: [COMMITTERS] pgsql: pg_upgrade: Convert old visibility map format to new format.

[Robert Haas]

On Tue, Apr 26, 2016 at 8:45 PM, Bruce Momjian  wrote:
> On Fri, Mar 11, 2016 at 05:36:34PM +, Robert Haas wrote:
>> pg_upgrade: Convert old visibility map format to new format.
>>
>> Commit a892234f830e832110f63fc0a2afce2fb21d1584 added a second bit per
>> page to the visibility map, but pg_upgrade has been unaware of it up
>> until now.  Therefore, a pg_upgrade from an earlier major release of
>> PostgreSQL to any commit preceding this one and following the one
>> mentioned above would result in invalid visibility map contents on the
>> new cluster, very possibly leading to data corruption.  This plugs
>> that hole.
>>
>> Masahiko Sawada, reviewed by Jeff Janes, Bruce Momjian, Simon Riggs,
>> Michael Paquier, Andres Freund, me, and others.
>
> I have tested the current git trees of all supported versions of
> Postgres in all possible pg_upgrade combinations and they all worked
> perfectly.

That's good!

All the commits related to this topic could use extra-careful review
and testing.  If Masahiko-san got anything wrong, or I did, this could
eat people's data in ways that are very hard to track down.

So I hope we were both extra-smart while working on this patch.

-- 
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] Timeline following for logical slots

[Noah Misch]

On Tue, Apr 26, 2016 at 05:37:40PM -0300, Alvaro Herrera wrote:
> Alvaro Herrera wrote:
> > Noah Misch wrote:
> > > The above-described topic is currently a PostgreSQL 9.6 open item.  
> > > Alvaro,
> > > since you committed the patch believed to have created it, you own this 
> > > open
> > > item.  If that responsibility lies elsewhere, please let us know whose
> > > responsibility it is to fix this.  Since new open items may be discovered 
> > > at
> > > any time and I want to plan to have them all fixed well in advance of the 
> > > ship
> > > date, I will appreciate your efforts toward speedy resolution.  Please
> > > present, within 72 hours, a plan to fix the defect within seven days of 
> > > this
> > > message.  Thanks.
> > 
> > I plan to get Craig's patch applied on Monday 25th, giving me some more
> > time for study.
> 
> I failed to meet this deadline, for which I apologize.  This week is
> going to be hectic around here, so my new deadline is to get these two
> patches applied on Friday 29th.  Ok?

Okay; thanks for this notification.


-- 
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] EXPLAIN VERBOSE with parallel Aggregate

[Robert Haas]

On Tue, Apr 26, 2016 at 6:44 PM, David Rowley
 wrote:
> On 27 April 2016 at 08:46, Robert Haas  wrote:
>> My proposed fix for this issue is attached.  Review is welcome;
>> otherwise, I'll just commit this.  The output looks like what I
>> suggested upthread:
>
> + if (!aggref->aggpartial)
> + elog(ERROR, "referenced Aggref is not partial");
>
> I think this is overly restrictive; ruleutils seems a bit out of line
> here to say that plans can't have > 1 combine aggregate node.
>
> When coding the combine aggs stuff I had test code to inject
> additional combine paths to make sure everything worked as expected.
> It did, but it won't if you add these two lines. I'd say just remove
> them.
>
> If you apply the attached and execute;
>
> create table t1 (num int not null);
> insert into t1 select generate_series(1,200);
>
> explain verbose select avg(num) from t1;
>
> You'll get;
>
> ERROR: referenced Aggref is not partial

In this test patch, should aggpath be using partial_grouping_target
rather than target?  I feel like we need to use
partial_grouping_target unless finalizeAggs is true.

-- 
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] Removing faulty hyperLogLog merge function

[Robert Haas]

On Tue, Apr 26, 2016 at 7:54 PM, Peter Geoghegan  wrote:
> On Tue, Apr 26, 2016 at 4:47 PM, Robert Haas  wrote:
>>> You don't want to remove buggy code that is currently unused simply
>>> because it might be useful to have that functionality in the future?
>>
>> No, I don't want to remove code that somebody thinks we should fix
>> instead of removing on your say-so.
>
> I don't think that that's an efficient use of anyone's time. If
> somebody proposes a patch with functionality that needs to merge HLL
> states, then they can add an implementation at that time.

Peter, if there is consensus on this patch, I will commit it.  If
there isn't, I won't.  If you don't like that, sorry.

-- 
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] Removing faulty hyperLogLog merge function

[Peter Geoghegan]

On Tue, Apr 26, 2016 at 4:47 PM, Robert Haas  wrote:
>> You don't want to remove buggy code that is currently unused simply
>> because it might be useful to have that functionality in the future?
>
> No, I don't want to remove code that somebody thinks we should fix
> instead of removing on your say-so.

I don't think that that's an efficient use of anyone's time. If
somebody proposes a patch with functionality that needs to merge HLL
states, then they can add an implementation at that time.

-- 
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] Removing faulty hyperLogLog merge function

[Robert Haas]

On Tue, Apr 26, 2016 at 7:46 PM, Peter Geoghegan  wrote:
> On Tue, Apr 26, 2016 at 4:41 PM, Robert Haas  wrote:
>> I'm not prepared to commit this over the objection offered by Tomas
>> Vondra on that thread.
>
> You don't want to remove buggy code that is currently unused simply
> because it might be useful to have that functionality in the future?

No, I don't want to remove code that somebody thinks we should fix
instead of removing on your say-so.

-- 
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] Removing faulty hyperLogLog merge function

[Peter Geoghegan]

On Tue, Apr 26, 2016 at 4:41 PM, Robert Haas  wrote:
> I'm not prepared to commit this over the objection offered by Tomas
> Vondra on that thread.

You don't want to remove buggy code that is currently unused simply
because it might be useful to have that functionality in the future?

-- 
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] Removing faulty hyperLogLog merge function

[Robert Haas]

On Mon, Apr 25, 2016 at 10:39 PM, Peter Geoghegan  wrote:
> The function hyperLogLogMerge() is faulty [1]. It has no current
> callers, though. I propose that we rip it out, as in the attached
> patch.
>
> [1] 
> http://www.postgresql.org/message-id/CAM3SWZT-i6R9JU5YXa8MJUou2_r3LfGJZpQ9tYa1BYxfkj0=c...@mail.gmail.com

I'm not prepared to commit this over the objection offered by Tomas
Vondra on that thread.

-- 
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] SET ROLE and reserved roles

[Robert Haas]

On Mon, Apr 25, 2016 at 6:55 PM, Stephen Frost  wrote:
> Based on our discussion at PGConf.US and the comments up-thread from
> Tom, I'll work up a patch to remove those checks around SET ROLE and
> friends which were trying to prevent default roles from possibly being
> made to own objects.
>
> Should the checks, which have been included since nearly the start of
> this version of the patch, to prevent users from GRANT'ing other rights
> to the default roles remain?  Or should those also be removed?  I
> *think* pg_dump/pg_upgrade would be fine with rights being added, and if
> we aren't preventing ownership of objects then we aren't going to be
> able to remove such roles in any case.

It'd be good to test that that works.  If it does, I think we may as
well allow it.

> Of course, with these default roles, users can't REVOKE the rights which
> are granted to them as that happens in C code, outside of the GRANT
> system.

I think you mean that they can't revoke the special magic rights, but
they could revoke any additional privileges which were granted.

> Working up a patch to remove these checks should be pretty quickly done
> (iirc, I've actually got an independent patch around from when I added
> them, just need to find it and then go through the committed patches to
> make sure I take care of everything), but would like to make sure that
> we're now all on the same page and that *all* of these checks should be
> removed, making default roles just exactly like "regular" roles, except
> that they're created at initdb time and have "special" rights provided
> by C-level code checks.

That's what I'm thinking.  I would welcome other views.

-- 
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] EXPLAIN VERBOSE with parallel Aggregate

[David Rowley]

On 27 April 2016 at 08:46, Robert Haas  wrote:
> My proposed fix for this issue is attached.  Review is welcome;
> otherwise, I'll just commit this.  The output looks like what I
> suggested upthread:

+ if (!aggref->aggpartial)
+ elog(ERROR, "referenced Aggref is not partial");

I think this is overly restrictive; ruleutils seems a bit out of line
here to say that plans can't have > 1 combine aggregate node.

When coding the combine aggs stuff I had test code to inject
additional combine paths to make sure everything worked as expected.
It did, but it won't if you add these two lines. I'd say just remove
them.

If you apply the attached and execute;

create table t1 (num int not null);
insert into t1 select generate_series(1,200);

explain verbose select avg(num) from t1;

You'll get;

ERROR: referenced Aggref is not partial

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


extra_combine_node.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] Proposed change to make cancellations safe

[Tom Lane]

Robert Haas  writes:
> On Mon, Apr 25, 2016 at 12:16 PM, Shay Rojansky  wrote:
>> Sure. I'd consider sending in a patch, but as this is a protocol-changing
>> feature it seems like working on this before the team "officially" starts
>> working on a new protocol might be a waste of time. Once there's critical
>> mass for a new protocol and agreement that PostgreSQL is going for it I'd be
>> happy to work on it.

> I don't immediately see a reason why this couldn't be done as an
> isolated change.  Suppose that we change the server to allow a cancel
> request to be either 16 bytes or 20 bytes, rather than always 16 bytes
> as they are currently.  Clients will need to be careful not to send
> the new type of cancel request to a server that is too old to
> understand it, but since they've got an active connection,
> server_version will have been previously reported.

> More generally, as long as new protocol bits are client-initiated, I
> don't think we really need to bump the protocol version.  If we want
> to change the kinds of responses the server sends or structurally
> change the format of protocol messages or deprecate messages that
> exist now, then we do.

Meh --- I'm fairly suspicious of shoehorning things in and pretending
it's not a protocol change.  That usually leads to crufty and ultimately
unmaintainable designs, because you're forced to do strange things when
you do it that way.  (Cf the COPY RAW thread for a recent example.)

Having said that, CANCEL is sufficiently outside the normal protocol
that maybe you are right: we could invent what amounts to a new cancel
protocol and trust clients to look at server_version to know what to send.

One problem is that we really ought to widen the random cancel key while
we're at it; 32 bits doesn't seem like enough to prevent brute-force
searches anymore.  However, since the cancel key is transmitted from the
server within the normal protocol, I don't see any way to do that without
a compatibility break.

Is there anything else people have complained about w.r.t. CANCEL?

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] [BUGS] Breakage with VACUUM ANALYSE + partitions

[Andres Freund]

On 2016-04-26 17:25:18 -0400, Robert Haas wrote:
> On Mon, Apr 25, 2016 at 4:57 PM, Andres Freund  wrote:
> > On 2016-04-25 16:29:36 -0400, Robert Haas wrote:
> >> On Mon, Apr 25, 2016 at 2:05 PM, Andres Freund  wrote:
> >> > Well, I posted a patch. I'd have applied it too (after addressing your
> >> > comments obviously), except that there's some interdependencies with the
> >> > nsmg > 0 thread (some of my tests fail spuriously without that
> >> > fixed). Early last week I waited for a patch on that thread, but when
> >> > that didn't materialize by Friday I switched to work on that [1].  With
> >> > both fixes applied I can't reproduce any problems anymore.
> >>
> >> OK.  What are the interdependencies?  You've said that a few times but
> >> I am not clear on what the nature of those interdependencies is.
> >
> > I added checks to smgr/md.c that verify that the smgr state is
> > consistent with the on-file state. Without the two issues in [1] fixed,
> > these tests fail in a standby, while running regression tests.  Adding
> > those tests made me notice a bug in an unreleased version of the patch,
> > so it seems they're worthwhile to run.
>
> Footnote [1] is used, but not defined.

Oops, it's the thread you replied too..

> I think it makes sense to go ahead and push this fix rather soon.

Will do.


-- 
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] [BUGS] Breakage with VACUUM ANALYSE + partitions

[Robert Haas]

On Mon, Apr 25, 2016 at 4:57 PM, Andres Freund  wrote:
> On 2016-04-25 16:29:36 -0400, Robert Haas wrote:
>> On Mon, Apr 25, 2016 at 2:05 PM, Andres Freund  wrote:
>> > Well, I posted a patch. I'd have applied it too (after addressing your
>> > comments obviously), except that there's some interdependencies with the
>> > nsmg > 0 thread (some of my tests fail spuriously without that
>> > fixed). Early last week I waited for a patch on that thread, but when
>> > that didn't materialize by Friday I switched to work on that [1].  With
>> > both fixes applied I can't reproduce any problems anymore.
>>
>> OK.  What are the interdependencies?  You've said that a few times but
>> I am not clear on what the nature of those interdependencies is.
>
> I added checks to smgr/md.c that verify that the smgr state is
> consistent with the on-file state. Without the two issues in [1] fixed,
> these tests fail in a standby, while running regression tests.  Adding
> those tests made me notice a bug in an unreleased version of the patch,
> so it seems they're worthwhile to run.

Footnote [1] is used, but not defined.

>> >> I assume you will be pretty darned unhappy if we end up at #2, so I am
>> >> trying to figure out if we can achieve #1.  OK?
>> >
>> > Ok.
>>
>> I'm still not clear on the answer to this.  I think the answer is that
>> you think that this patch is OK to commit with some editing, but the
>> situation with the nmsgs > 0 patch is not so clear yet because it
>> hasn't had any review yet.  And they depend on each other somehow.  Am
>> I close?
>
> You are. I'd prefer pushing this after fixes for the two invalidation
> issues, because it makes testing easier. But I guess the timeframe there
> is unclear enough, that it makes sense to test this patch on top of the
> the preliminary fixes, and then push without those.

I think it makes sense to go ahead and push this fix rather soon.  I'd
much rather not have this bug, even if verifying that I don't have it
is a little harder than it might be under other circumstances.  The
fix could be buggy too, and if that fix doesn't go in until right
before we ship beta1, we're less likely to be able to find and correct
any deficiencies before that goes out the door.

-- 
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] Proposed change to make cancellations safe

[Robert Haas]

On Mon, Apr 25, 2016 at 12:16 PM, Shay Rojansky  wrote:
> Sure. I'd consider sending in a patch, but as this is a protocol-changing
> feature it seems like working on this before the team "officially" starts
> working on a new protocol might be a waste of time. Once there's critical
> mass for a new protocol and agreement that PostgreSQL is going for it I'd be
> happy to work on it.

I don't immediately see a reason why this couldn't be done as an
isolated change.  Suppose that we change the server to allow a cancel
request to be either 16 bytes or 20 bytes, rather than always 16 bytes
as they are currently.  Clients will need to be careful not to send
the new type of cancel request to a server that is too old to
understand it, but since they've got an active connection,
server_version will have been previously reported.

More generally, as long as new protocol bits are client-initiated, I
don't think we really need to bump the protocol version.  If we want
to change the kinds of responses the server sends or structurally
change the format of protocol messages or deprecate messages that
exist now, then we do.

-- 
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] EXPLAIN VERBOSE with parallel Aggregate

[Tom Lane]

Robert Haas  writes:
> My proposed fix for this issue is attached.  Review is welcome;
> otherwise, I'll just commit this.  The output looks like what I
> suggested upthread:

I haven't read the patch, but the sample output looks sane.

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] EXPLAIN VERBOSE with parallel Aggregate

[Robert Haas]

On Mon, Apr 25, 2016 at 1:03 PM, Robert Haas  wrote:
> Yeah, I'd be happy to have more people chime in.  I think your example
> is interesting, but it doesn't persuade me, because the rule has
> always been that EXPLAIN shows the output *columns*, not the output
> *rows*.  The disappearance of some rows doesn't change the list of
> output columns.  For scans and joins this rule is easy to apply; for
> aggregates, where many rows become one, less so.  Some of the existing
> choices there are certainly arguable, like the fact that FILTER is
> shown anywhere at all, which seems like an artifact to me.  But I
> think that now is not the time to rethink those decisions.

My proposed fix for this issue is attached.  Review is welcome;
otherwise, I'll just commit this.  The output looks like what I
suggested upthread:

rhaas=# explain verbose select count(*), corr(aid, bid) from pgbench_accounts ;
  QUERY PLAN
--
 Finalize Aggregate  (cost=742804.22..742804.23 rows=1 width=16)
   Output: count(*), corr((aid)::double precision, (bid)::double precision)
   ->  Gather  (cost=742804.00..742804.21 rows=2 width=40)
 Output: (PARTIAL count(*)), (PARTIAL corr((aid)::double
precision, (bid)::double precision))
 Workers Planned: 2
 ->  Partial Aggregate  (cost=741804.00..741804.01 rows=1 width=40)
   Output: PARTIAL count(*), PARTIAL corr((aid)::double
precision, (bid)::double precision)
   ->  Parallel Seq Scan on public.pgbench_accounts
(cost=0.00..616804.00 rows=1250 width=8)
 Output: aid, bid, abalance, filler
(9 rows)

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index a21928b..20e38f0 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -1244,6 +1244,8 @@ _copyAggref(const Aggref *from)
 	COPY_NODE_FIELD(aggfilter);
 	COPY_SCALAR_FIELD(aggstar);
 	COPY_SCALAR_FIELD(aggvariadic);
+	COPY_SCALAR_FIELD(aggpartial);
+	COPY_SCALAR_FIELD(aggcombine);
 	COPY_SCALAR_FIELD(aggkind);
 	COPY_SCALAR_FIELD(agglevelsup);
 	COPY_LOCATION_FIELD(location);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 3c6c567..c5ccc42 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -202,6 +202,8 @@ _equalAggref(const Aggref *a, const Aggref *b)
 	COMPARE_NODE_FIELD(aggfilter);
 	COMPARE_SCALAR_FIELD(aggstar);
 	COMPARE_SCALAR_FIELD(aggvariadic);
+	COMPARE_SCALAR_FIELD(aggcombine);
+	COMPARE_SCALAR_FIELD(aggpartial);
 	COMPARE_SCALAR_FIELD(aggkind);
 	COMPARE_SCALAR_FIELD(agglevelsup);
 	COMPARE_LOCATION_FIELD(location);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 5ac7446..c2f0e0f 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1040,6 +1040,8 @@ _outAggref(StringInfo str, const Aggref *node)
 	WRITE_NODE_FIELD(aggfilter);
 	WRITE_BOOL_FIELD(aggstar);
 	WRITE_BOOL_FIELD(aggvariadic);
+	WRITE_BOOL_FIELD(aggcombine);
+	WRITE_BOOL_FIELD(aggpartial);
 	WRITE_CHAR_FIELD(aggkind);
 	WRITE_UINT_FIELD(agglevelsup);
 	WRITE_LOCATION_FIELD(location);
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 8059594..6f28047 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -556,6 +556,8 @@ _readAggref(void)
 	READ_NODE_FIELD(aggfilter);
 	READ_BOOL_FIELD(aggstar);
 	READ_BOOL_FIELD(aggvariadic);
+	READ_BOOL_FIELD(aggcombine);
+	READ_BOOL_FIELD(aggpartial);
 	READ_CHAR_FIELD(aggkind);
 	READ_UINT_FIELD(agglevelsup);
 	READ_LOCATION_FIELD(location);
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 5537c14..93fe3a3 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -2100,6 +2100,10 @@ search_indexed_tlist_for_partial_aggref(Aggref *aggref, indexed_tlist *itlist,
 continue;
 			if (aggref->aggvariadic != tlistaggref->aggvariadic)
 continue;
+			/*
+			 * it would be harmless to compare aggcombine and aggpartial, but
+			 * it's also unnecessary
+			 */
 			if (aggref->aggkind != tlistaggref->aggkind)
 continue;
 			if (aggref->agglevelsup != tlistaggref->agglevelsup)
@@ -2463,6 +2467,8 @@ fix_combine_agg_expr_mutator(Node *node, fix_upper_expr_context *context)
 			newtle = makeTargetEntry((Expr *) newvar, 1, NULL, false);
 			newaggref = (Aggref *) copyObject(aggref);
 			newaggref->args = list_make1(newtle);
+			newaggref->aggcombine = true;
+			newaggref->aggpartial = false;
 
 			return (Node *) newaggref;
 		}
diff --git a/src/backend/optimizer/util/tlist.c b/src/backend/optimizer/util/tlist.c
index 4c8c83d..aa2c2f8 100644
--- a/src/backend/optimizer/util/tlist.c
+++ b/src/backend/optimizer/util/t

Re: [HACKERS] Timeline following for logical slots

[Alvaro Herrera]

Alvaro Herrera wrote:
> Noah Misch wrote:
> 
> > The above-described topic is currently a PostgreSQL 9.6 open item.  Alvaro,
> > since you committed the patch believed to have created it, you own this open
> > item.  If that responsibility lies elsewhere, please let us know whose
> > responsibility it is to fix this.  Since new open items may be discovered at
> > any time and I want to plan to have them all fixed well in advance of the 
> > ship
> > date, I will appreciate your efforts toward speedy resolution.  Please
> > present, within 72 hours, a plan to fix the defect within seven days of this
> > message.  Thanks.
> 
> I plan to get Craig's patch applied on Monday 25th, giving me some more
> time for study.

I failed to meet this deadline, for which I apologize.  This week is
going to be hectic around here, so my new deadline is to get these two
patches applied on Friday 29th.  Ok?

-- 
Á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] Timeline following for logical slots

[Alvaro Herrera]

Craig Ringer wrote:

> The reason the new src/test/recovery/ tests don't notice this is that using
> pg_recvlogical from the TAP tests is currently pretty awkward.
> pg_recvlogical has no way to specify a stop-point or return when there's no
> immediately pending data like the SQL interface does. So you have to read
> from it until you figure it's not going to return anything more then kill
> it and look at what it did return and hope you don't lose anything in
> buffering.I don't much like relying on timeouts as part of normal
> successful results since they can upset some of the older and slower
> buildfarm members. I'd rather be able to pass a --stoppos= or a --n-xacts=
> option, but it was a bit too late to add those.

Considering that pg_recvlogical was introduced mostly as a way to test
logical decoding features, I think this is a serious oversight and we
should patch it.  I suppose we could leave it for 9.7, thought I admit I
would prefer it to introduce it in 9.6.  Now everyone throwing stones at
me in 3 ... 2 ...

> Per the separate mail I sent to hackers, xlogreader is currently pretty
> much timeline-agnostic. The timeline tracking code needs to know when the
> xlogreader does a random read and do a fresh lookup so its state is part of
> the XLogReaderState struct. But the walsender's xlogreader page read
> callback doesn't use the xlogreader's state, and it can't because it's also
> used for physical replication, which communicates the timeline to read from
> with the page read function via globals.

The globals there are a disaster and I welcome efforts to get rid of
them.

> So for now, I just set the globals before calling the page read
> function:
> 
> +   XLogReadDetermineTimeline(state);
> +   sendTimeLineIsHistoric = state->currTLI == ThisTimeLineID;
> +   sendTimeLine = state->currTLI;
> +   sendTimeLineValidUpto = state->currTLIValidUntil;
> +   sendTimeLineNextTLI = state->nextTLI;

Ok, this is pretty ugly but seems acceptable.

> Per separate mail, I'd quite like to tackle the level of duplication in
> timeline following logic in 9.7 and maybe see if the _three_ separate read
> xlog page functions can be unified at the same time. But that sure isn't
> 9.6 material.

Agreed.

-- 
Á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] Timeline following for logical slots

[Andres Freund]

On 2016-04-26 17:22:49 -0300, Alvaro Herrera wrote:
> > -   /* oldest LSN that might be required by this replication slot */
> > +   /*
> > +* oldest LSN that might be required by this replication slot.
> > +*
> > +* Points to the LSN of the most recent xl_running_xacts record where
> > +* no transaction that commits after confirmed_flush is already in
> > +* progress. At this point all xacts committing after confirmed_flush
> > +* can be read entirely into reorder buffers and all visibility
> > +* information can be reconstructed.
> > +*/
> > XLogRecPtr  restart_lsn;
> 
> I'm unsure about this one.  Why are we speaking of reorder buffers here,
> if this is generically about replication slots?  Is it that slots used
> by physical replication do not *need* a restart_lsn?

It's used in physical slots as well, so I agree.  Also I think the
xl_running_xacts bit is going into way to much detail; it could very
well just be shutdown checkpoint or other similar locations.

> > diff --git a/src/backend/replication/logical/logicalfuncs.c 
> > b/src/backend/replication/logical/logicalfuncs.c
> > index 5af6751..5f74941 100644
> > --- a/src/backend/replication/logical/logicalfuncs.c
> > +++ b/src/backend/replication/logical/logicalfuncs.c
> > @@ -236,8 +236,10 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo 
> > fcinfo, bool confirm, bool bin
> > PG_TRY();
> > {
> > /*
> > -* Passing InvalidXLogRecPtr here causes decoding to start 
> > returning
> > -* commited xacts to the client at the slot's confirmed_flush.
> > +* Passing InvalidXLogRecPtr here causes logical decoding to
> > +* start calling the output plugin for transactions that commit
> > +* at or after the slot's confirmed_flush. This filters commits
> > +* out from the client but they're still decoded.
> >  */
> > ctx = CreateDecodingContext(InvalidXLogRecPtr,
> > options,
> 
> I this it's weird to have the parameter explained in the callsite rather
> than in the comment about CreateDecodingContext.  I think this patch
> needs to apply to logical.c, not logicalfuncs.

I still think the entire comment ought to be removed.


- 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] Timeline following for logical slots

[Alvaro Herrera]

Craig Ringer wrote:

> - /* oldest LSN that the client has acked receipt for */
> + /*
> +  * oldest LSN that the client has acked receipt for
> +  *
> +  * Decoding will start calling output plugin callbacks for commits
> +  * after this LSN unless a different start point is specified by
> +  * the client.
> +  */
>   XLogRecPtr  confirmed_flush;

How about this wording?

/*
 * Oldest LSN that the client has acked receipt for.  This is used as
 * the start_lsn point in case the client doesn't specify one, and also
 * as a safety measure to back off in case the client specifies a
 * start_lsn that's further in the future than this value.
 */
XLogRecPtr  confirmed_flush;

> - /* oldest LSN that might be required by this replication slot */
> + /*
> +  * oldest LSN that might be required by this replication slot.
> +  *
> +  * Points to the LSN of the most recent xl_running_xacts record where
> +  * no transaction that commits after confirmed_flush is already in
> +  * progress. At this point all xacts committing after confirmed_flush
> +  * can be read entirely into reorder buffers and all visibility
> +  * information can be reconstructed.
> +  */
>   XLogRecPtr  restart_lsn;

I'm unsure about this one.  Why are we speaking of reorder buffers here,
if this is generically about replication slots?  Is it that slots used
by physical replication do not *need* a restart_lsn?  I think the
comment should be worded in a way that's not specific to logical
decoding; or, if the restart_lsn field *is* specific to logical
decoding, then the comment should state as much.


> diff --git a/src/backend/replication/logical/logicalfuncs.c 
> b/src/backend/replication/logical/logicalfuncs.c
> index 5af6751..5f74941 100644
> --- a/src/backend/replication/logical/logicalfuncs.c
> +++ b/src/backend/replication/logical/logicalfuncs.c
> @@ -236,8 +236,10 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo 
> fcinfo, bool confirm, bool bin
>   PG_TRY();
>   {
>   /*
> -  * Passing InvalidXLogRecPtr here causes decoding to start 
> returning
> -  * commited xacts to the client at the slot's confirmed_flush.
> +  * Passing InvalidXLogRecPtr here causes logical decoding to
> +  * start calling the output plugin for transactions that commit
> +  * at or after the slot's confirmed_flush. This filters commits
> +  * out from the client but they're still decoded.
>*/
>   ctx = CreateDecodingContext(InvalidXLogRecPtr,
>   options,

I this it's weird to have the parameter explained in the callsite rather
than in the comment about CreateDecodingContext.  I think this patch
needs to apply to logical.c, not logicalfuncs.

> @@ -262,11 +264,9 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo 
> fcinfo, bool confirm, bool bin
>   ctx->output_writer_private = p;
>  
>   /*
> -  * We start reading xlog from the restart lsn, even though we 
> won't
> -  * start returning decoded data to the user until the point set 
> up in
> -  * CreateDecodingContext. The restart_lsn is far enough back 
> that we'll
> -  * see the beginning of any xact we're expected to return to 
> the client
> -  * so we can assemble a complete reorder buffer for it.
> +  * Reading and decoding of WAL must start at restart_lsn so 
> that the
> +  * entirety of each of the xacts that commit after confimed_lsn 
> can be
> +  * accumulated into reorder buffers.
>*/
>   startptr = MyReplicationSlot->data.restart_lsn;

This one looks fine to me, except typo s/confimed/confirmed/

-- 
Á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] Parallel Queries and PostGIS

[Paul Ramsey]

On Fri, Apr 22, 2016 at 11:44 AM, Stephen Frost  wrote:
> Paul,
>
> * Paul Ramsey (pram...@cleverelephant.ca) wrote:
>> On Mon, Mar 28, 2016 at 9:45 AM, Stephen Frost  wrote:
>> > Would you agree that it'd be helpful to have for making the st_union()
>> > work better in parallel?
>>
>> For our particular situation w/ ST_Union, yes, it would be ideal to be
>> able to run a worker-side combine function as well as the master-side
>> one. Although the cascaded union would be less effective spread out
>> over N nodes, doing it only once per worker, rather than every N
>> records would minimize the loss of effectiveness.
>
> I chatted with Robert a bit about this and he had an interesting
> suggestion.  I'm not sure that it would work for you, but the
> serialize/deserialize functions are used to transfer the results from
> the worker process to the main process.  You could possibly do the
> per-worker finalize work in the serialize function to get the benefit of
> running that in parallel.
>
> You'll need to mark the aggtranstype as 'internal' to have the
> serialize/deserialize code called.  Hopefully that's not too much of an
> issue.

Thanks Stephen. We were actually thinking that it might make more
sense to just do the parallel processing in our own threads in the
finalfunc. Not as elegant and magical as bolting into the PgSQL infra,
but if we're doing something hacky anyways, might as well be our own
hacky.

ATB,
P

>
> Thanks!
>
> Stephen


-- 
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] Add jsonb_compact(...) for whitespace-free jsonb to text

[Merlin Moncure]

On Sun, Apr 24, 2016 at 8:16 PM, Andrew Dunstan  wrote:
> Note that the json type, unlike jsonb, preserves exactly the white space and
> key order of its input. In fact, the input is exactly what it stores.

That is true, but the json serialization functions (to_json etc)
really out to have the same whitespace strategy is jsonb text out.  Of
the two ways it's currently done, the json serialization variant seems
better but a completely compact variant seems like a good idea basis
of efficiency.

merlin


-- 
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] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Christian Ullrich wrote:


wrong even without considering the debug/release split. If we load a
compiled extension built with a CRT we have not seen yet, _after_ the
first call to pgwin32_putenv(), that module's CRT's view of its
environment will be frozen because we will never attempt to update it.


Four patches attached:

master --- 0001 --- 0002 --- 0003
 \
  `- 0004

0001 is just some various fixes to set the stage.

0002 fixes this "load race" by not remembering a negative result 
anymore. However, ...



If it can happen that a CRT DLL is unloaded before the process exits,
and we cached the module handle while it was loaded, and later
pgwin32_putenv() is called, that won't end well for the process. This
might be a bit far-fetched; I have to see if I can actually make it happen.


... this *can* and does happen, so fixing the load race alone is not 
enough. 0004 fixes the unload race as well, by dropping the entire DLL 
handle/_putenv pointer cache from the function and going through the 
list of DLLs each time.


I tested this with a project 
() that contains two DLLs:


- The first one is built with VS 2013 (debug), as is the server. It
  does not matter what it is built with, except it must not be the same
  as the second DLL. It exports a single function callable from SQL.

- The second one is built with VS 2015 (debug), and again, the exact
  CRT is not important as long as it is not the same as the server
  or the first DLL.

The function does this:

1. It loads the second DLL. This brings in ucrtbased.dll as well.
2. It calls putenv().
3. It unloads the second DLL. This also causes ucrtbased.dll to be
   unloaded because it is not needed anymore.
4. It calls putenv() again.

   - With current master, this works, because pgwin32_putenv(),
 after the first call somewhere early during backend startup,
 never looks for ucrtbased again (including in step 2).

   - With patch 0002 applied, it crashes because pgwin32_putenv(),
 having detected ucrtbased.dll and cached its HMODULE during
 the call in step 2 above, reuses these values after the DLL
 is long gone.

   - With patch 0004 applied as well, it works again because no
 caching is done anymore.

Even with patch 0004, there is still a race condition between the main 
thread and a theoretical additional thread created by some other 
component that unloads some CRT between GetProcAddress() and the 
_putenv() call, but that is hardly fixable.


The fact that master looks like it does means that there have not been 
many (or any) complaints about missing cross-module environment 
variables. If nobody ever needs to see a variable set elsewhere, we have 
a very simple solution: Why don't we simply throw out the whole #ifdef 
_MSC_VER block?


There is another possible fix, ugly as sin, if we really need to keep 
the whole environment update machinery *and* cannot do the full loop 
each time. Patch 0003 pins each CRT when we see it for the first time. 
GET_MODULE_HANDLE_EX_FLAG_PIN is documented as "The module stays loaded 
until the process is terminated, no matter how many times FreeLibrary is 
called", so the unload race cannot occur anymore.


--
Christian

From d74e778d8abbfea244bacbeb352cadc737032e85 Mon Sep 17 00:00:00 2001
From: Christian Ullrich 
Date: Tue, 26 Apr 2016 15:26:14 +0200
Subject: [PATCH 1/3] Cleanups in pgwin32_putenv().

- Added UCRT and all debug CRTs
- Condensed the array to one line per item (it was getting long)
- Test HMODULEs for NULL instead of zero
- Removed unnecessary CloseHandle() call
---
 src/port/win32env.c | 49 -
 1 file changed, 20 insertions(+), 29 deletions(-)

diff --git a/src/port/win32env.c b/src/port/win32env.c
index 7e4ff62..fd6762e 100644
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -45,33 +45,25 @@ pgwin32_putenv(const char *envval)
PUTENVPROC  putenvFunc;
}   rtmodules[] =
{
-   {
-   "msvcrt", 0, NULL
-   },  /* Visual 
Studio 6.0 / mingw */
-   {
-   "msvcr70", 0, NULL
-   },  /* Visual 
Studio 2002 */
-   {
-   "msvcr71", 0, NULL
-   },  /* Visual 
Studio 2003 */
-   {
-   "msvcr80", 0, NULL
-   },  /* Visual 
Studio 2005 */
-   {
-   "msvcr90", 0, NULL
-   },  /* Visual 
Studio 2008 */
-   {
-   "msvcr100", 0, NULL
-   },  /* Visual 
Studio 2010 */
-

Re: [HACKERS] [PATCH] Phrase search ported to 9.6

[Teodor Sigaev]

There are some previously unnoticed errors in docs (master branch), this patch
fixes them.

Thank you, pushed

--
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] GIN data corruption bug(s) in 9.6devel

[Teodor Sigaev]

Check my reasoning: In version 4 I added a remebering of tail of pending
list into blknoFinish variable. And when we read page which was a tail on
cleanup start then we sets cleanupFinish variable and after cleaning that
page we will stop further cleanup. Any insert caused during cleanup will be
placed after blknoFinish (corner case: in that page), so, vacuum should not
miss tuples marked as deleted.


Yes, I agree with the correctness of v4.  But I do wonder if we should
use that early stopping for vacuum and gin_clean_pending_list, rather

Interesting, I've missed this possible option


than just using it for user backends.  While I think correctness
allows it to stop early, since these routines are explicitly about
cleaning things up it seems like they should volunteer to clean the
whole thing.


I believe that autovacuum should not require guaranteed full clean up, only 
vacuum and gin_clean_pending_list() should do that. In all other cases it should 
stop early to prevent possible infinite cleanup. Patch attached.



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


gin_alone_cleanup-5.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] Add jsonb_compact(...) for whitespace-free jsonb to text

[Stephen Frost]

* Ryan Pedela (rped...@datalanche.com) wrote:
> On Sun, Apr 24, 2016 at 4:02 PM, Sehrope Sarkuni  wrote:
> > The default text representation of jsonb adds whitespace in between
> > key/value pairs (after the colon ":") and after successive properties
> > (after the comma ","):

[...]

> > It'd be nice to have a stable text representation of a jsonb value with
> > minimal whitespace. The latter would also save a few bytes per record in
> > text output formats, on the wire, and in backups (ex: COPY ... TO STDOUT).
> 
> +1
> 
> I cannot comment on the patch itself, but I welcome jsonb_compact() or some
> way to get JSON with no inserted whitespace.

As I mentioned to Sehrope on IRC, at least for my 2c, if you want a
compact JSON format to reduce the amount of traffic over the wire or to
do things with on the client side, we should probably come up with a
binary format, rather than just hack out the whitespace.  It's not like
representing numbers using ASCII characters is terribly efficient
either.

Compression might be another option, though that's certainly less
flexible and only (easily) used in combination with SSL, today.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Add jsonb_compact(...) for whitespace-free jsonb to text

[Ryan Pedela]

On Sun, Apr 24, 2016 at 4:02 PM, Sehrope Sarkuni  wrote:

> Hi,
>
> The default text representation of jsonb adds whitespace in between
> key/value pairs (after the colon ":") and after successive properties
> (after the comma ","):
>
> postgres=# SELECT '{"b":2,"a":1}'::jsonb::text;
>text
> --
>  {"a": 1, "b": 2}
> (1 row)
>
> AFAIK, there's also no guarantee on the specific order of the resulting
> properties in the text representation either. I would suppose it's fixed
> for a given jsonb value within a database major version but across major
> versions it could change (if the underlying representation changes).
>
> I originally ran into this when comparing the hashes of the text
> representation of jsonb columns. The results didn't match up because the
> javascript function JSON.stringify(...) does not add any extra whitespace.
>
> It'd be nice to have a stable text representation of a jsonb value with
> minimal whitespace. The latter would also save a few bytes per record in
> text output formats, on the wire, and in backups (ex: COPY ... TO STDOUT).
>

+1

I cannot comment on the patch itself, but I welcome jsonb_compact() or some
way to get JSON with no inserted whitespace.

Re: [HACKERS] [COMMITTERS] pgsql: Add trigonometric functions that work in degrees.

[Peter Eisentraut]

On 04/26/2016 11:26 AM, Tom Lane wrote:
> OK, I've pushed a change along these lines.  Peter, would you see whether
> HEAD fixes it for you?

Yeah, it passes now.  Nobody touch anything! ;-)


-- 
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] [COMMITTERS] pgsql: Add trigonometric functions that work in degrees.

[Dean Rasheed]

On 26 April 2016 at 16:26, Tom Lane  wrote:
> The next time somebody proposes that we can get exact results out of
> floating-point arithmetic, I'm going to run away screaming.
>

Yeah, I think I will have the same reaction.

Thanks for all your hard work getting this to work.

Regards,
Dean


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

Re: [HACKERS] Rename max_parallel_degree?

[Robert Haas]

On Tue, Apr 26, 2016 at 11:44 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> I still think
>> max_parallel_workers is confusingly similar to max_worker_processes,
>> but nothing's going to make everyone completely happy here.
>
> Well, what was suggested upthread was to change all of these to follow
> the pattern max_foo_workers or max_foo_worker_processes, where foo would
> (hopefully) clarify the scope in which the limitation applies.

Well, I don't like max_node_parallel_degree much.  We don't call it
max_node_work_mem.  And node is not exactly a term that's going to be
more familiar to the average PostgreSQL user than parallel degree is
to (apparently) the average PostgreSQL developer.  I think at some
point adding noise words hurts more than it helps, and you've just got
to ask people to RTFM if they really want to understand.

-- 
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] [COMMITTERS] pgsql: Add trigonometric functions that work in degrees.

[Robert Haas]

On Tue, Apr 26, 2016 at 11:36 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Apr 26, 2016 at 11:26 AM, Tom Lane  wrote:
>>> The next time somebody proposes that we can get exact results out of
>>> floating-point arithmetic, I'm going to run away screaming.
>
>> And you wonder why I avoid floating point variables like the plague...!
>
> Float variables are fine for their intended use-case.  The problem is
> with trying to guarantee identical results across all platforms.

Yeah, yeah... :-)

-- 
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] Rename max_parallel_degree?

[Tom Lane]

Robert Haas  writes:
> I still think
> max_parallel_workers is confusingly similar to max_worker_processes,
> but nothing's going to make everyone completely happy here.

Well, what was suggested upthread was to change all of these to follow
the pattern max_foo_workers or max_foo_worker_processes, where foo would
(hopefully) clarify the scope in which the limitation applies.

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] Rename max_parallel_degree?

[Robert Haas]

On Tue, Apr 26, 2016 at 11:34 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Apr 26, 2016 at 11:22 AM, Alvaro Herrera
>>  wrote:
>>> I think the word "degree" is largely seen as a bad idea: it would become
>>> a somewhat better idea only if we change how it works so that it matches
>>> what other DBMSs do, but you oppose that.  Hence my proposal to get rid
>>> of that word in the UI.
>
>> Well I agree with that up to a point, but I think ALTER TABLE foo SET
>> (parallelism = 4) is not a model of clarity.  "parallelism" or
>> "parallel" is not obviously an integer quality.  I guess we could
>> s/parallel_degree/parallel_workers/g.  I find that terminology less
>> elegant than "parallel degree", but I can live with it.
>
> Shouldn't it be "max_parallel_workers", at least in some contexts?
> Otherwise, I'd read it as a promise that exactly that many workers
> will be used.

Yeah, I guess it would be parallel_degree -> parallel_workers and
max_parallel_degree -> max_parallel_workers.  I still think
max_parallel_workers is confusingly similar to max_worker_processes,
but nothing's going to make everyone completely happy here.

-- 
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] [COMMITTERS] pgsql: Add trigonometric functions that work in degrees.

[Tom Lane]

Robert Haas  writes:
> On Tue, Apr 26, 2016 at 11:26 AM, Tom Lane  wrote:
>> The next time somebody proposes that we can get exact results out of
>> floating-point arithmetic, I'm going to run away screaming.

> And you wonder why I avoid floating point variables like the plague...!

Float variables are fine for their intended use-case.  The problem is
with trying to guarantee identical results across all platforms.

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] Rename max_parallel_degree?

[Tom Lane]

Robert Haas  writes:
> On Tue, Apr 26, 2016 at 11:22 AM, Alvaro Herrera
>  wrote:
>> I think the word "degree" is largely seen as a bad idea: it would become
>> a somewhat better idea only if we change how it works so that it matches
>> what other DBMSs do, but you oppose that.  Hence my proposal to get rid
>> of that word in the UI.

> Well I agree with that up to a point, but I think ALTER TABLE foo SET
> (parallelism = 4) is not a model of clarity.  "parallelism" or
> "parallel" is not obviously an integer quality.  I guess we could
> s/parallel_degree/parallel_workers/g.  I find that terminology less
> elegant than "parallel degree", but I can live with it.

Shouldn't it be "max_parallel_workers", at least in some contexts?
Otherwise, I'd read it as a promise that exactly that many workers
will be used.

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] [COMMITTERS] pgsql: Add trigonometric functions that work in degrees.

[Robert Haas]

On Tue, Apr 26, 2016 at 11:26 AM, Tom Lane  wrote:
> The next time somebody proposes that we can get exact results out of
> floating-point arithmetic, I'm going to run away screaming.

And you wonder why I avoid floating point variables like the plague...!

-- 
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] Rename max_parallel_degree?

[Robert Haas]

On Tue, Apr 26, 2016 at 11:22 AM, Alvaro Herrera
 wrote:
> Robert Haas wrote:
>> On Mon, Apr 25, 2016 at 2:58 PM, Alvaro Herrera
>>  wrote:
>
>> > What about calling it something even simpler, such as "max_parallelism"?
>> > This avoids such cargo cult, and there's no implication that it's
>> > per-query.
>>
>> So what would we call the "parallel_degree" member of the Path data
>> structure, and the "parallel_degree" reloption?  I don't think
>> renaming either of those to "parallelism" is going to be an
>> improvement.
>
> I think we should define the UI first, *then* decide what to call the
> internal variable names.  In most cases we're able to call the variables
> the same as the user-visible names, but not always and there's no rule
> that it must be so.  Having source code variable names determine what
> the user visible name is seems to me like putting the cart before the
> horse.
>
> I think the word "degree" is largely seen as a bad idea: it would become
> a somewhat better idea only if we change how it works so that it matches
> what other DBMSs do, but you oppose that.  Hence my proposal to get rid
> of that word in the UI.  (My first thought yesterday was to look for
> synonyms for the "degree" word, so I got as far as "amount of
> parallelism" when I realized that such accompanying words add no value
> and so we might as well not have any word there.)

Well I agree with that up to a point, but I think ALTER TABLE foo SET
(parallelism = 4) is not a model of clarity.  "parallelism" or
"parallel" is not obviously an integer quality.  I guess we could
s/parallel_degree/parallel_workers/g.  I find that terminology less
elegant than "parallel degree", but I can live with it.

-- 
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] [COMMITTERS] pgsql: Add trigonometric functions that work in degrees.

[Tom Lane]

Dean Rasheed  writes:
> On 26 April 2016 at 04:25, Tom Lane  wrote:
>> In short, these tests suggest that we need a coding pattern like
>> this:
>> volatile float8 asin_x = asin(x);
>> return (asin_x / asin_0_5) * 30.0;

> Agreed. That looks like the least hacky way of solving the problem. I
> think it's more readable when the logic is kept local, and it's
> preferable to avoid any compiler-specific options or global flags that
> would affect other code.

OK, I've pushed a change along these lines.  Peter, would you see whether
HEAD fixes it for you?

The next time somebody proposes that we can get exact results out of
floating-point arithmetic, I'm going to run away screaming.

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] Suspicious behaviour on applying XLOG_HEAP2_VISIBLE.

[Robert Haas]

On Mon, Apr 25, 2016 at 2:10 PM, Andres Freund  wrote:
> Afaics the problem described below was introduced in b4e07417, do you
> have a different/better proposal than
> s/CacheInvalidateSmgr/CacheInvalidateRelcache/? Doing that doesn't feel
> quite right either, because it only makes the file extension visible at
> end-of-xact - which is mostly harmless, but still.

Maybe I'm all wet here, but it seems to me like this is a bug in
heap_xlog_visible().  If the relation has been extended on the master
but no bits are set, then it doesn't really matter whether any
invalidation happens on the standby.  On the other hand, if the
relation has been extended on the master and a bit has actually been
set on the new page, then there should be an XLOG_HEAP2_VISIBLE record
that gets replayed on the standby.  And replay of that record should
call visibilitymap_pin(), which should call vm_readbuf(), which should
call vm_extend(), which should issue the same smgr invalidation on the
standby that got issued on the master.  So no problem!  However, in
the FPI case, heap_xlog_visible() optimizes all of that away, so the
invalidation doesn't get issued.  Oops.  But we could cure that just
by having heap_xlog_visible() still call CacheInvalidateSmgr() even in
that case, and then we'd be actually be following the rule that
non-transactional invalidations should be issued on the standby even
while in replay.  That seems cleaner to me than switching to a
transactional invalidation, which isn't really the right thing anyway.

In other words, I think Masahiko Sawada's patch in the original post
is pretty much right on target, except that we don't need to do that
always, but rather only in the FPI case when the call to
visibilitymap_pin() is being optimized away.  If we solve the problem
that way, I don't think we even need a new WAL record for this case,
which is a non-trivial fringe benefit.

-- 
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] Rename max_parallel_degree?

[Alvaro Herrera]

Robert Haas wrote:
> On Mon, Apr 25, 2016 at 2:58 PM, Alvaro Herrera
>  wrote:

> > What about calling it something even simpler, such as "max_parallelism"?
> > This avoids such cargo cult, and there's no implication that it's
> > per-query.
> 
> So what would we call the "parallel_degree" member of the Path data
> structure, and the "parallel_degree" reloption?  I don't think
> renaming either of those to "parallelism" is going to be an
> improvement.

I think we should define the UI first, *then* decide what to call the
internal variable names.  In most cases we're able to call the variables
the same as the user-visible names, but not always and there's no rule
that it must be so.  Having source code variable names determine what
the user visible name is seems to me like putting the cart before the
horse.

I think the word "degree" is largely seen as a bad idea: it would become
a somewhat better idea only if we change how it works so that it matches
what other DBMSs do, but you oppose that.  Hence my proposal to get rid
of that word in the UI.  (My first thought yesterday was to look for
synonyms for the "degree" word, so I got as far as "amount of
parallelism" when I realized that such accompanying words add no value
and so we might as well not have any word there.)

-- 
Á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] Protocol buffer support for Postgres

[David Fetter]

On Mon, Apr 25, 2016 at 11:06:11PM -0700, 陈天舟 wrote:
> I am interested in adding Protocol Buffer support for Postgres. Protocol
> Buffer occupies less space than JSON. More importantly, it has schema and
> is forward/backward compatible. All these make it a very good format for
> persistency.

Thanks for the idea.  There are many different serializations, some of
which are listed below, each with their own qualities, which can be
good or bad with a very strong dependency on context:

https://en.wikipedia.org/wiki/Comparison_of_data_serialization_formats

Should we see about making a more flexible serialization
infrastructure?  What we have is mostly /ad hoc/, and has already
caused real pain to the PostGIS folks, this even after some pretty
significant and successful efforts were made in this direction.

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


-- 
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-04-25 19:40 GMT+02:00 Bruce Momjian :

>
> Good summary.  Is there a TODO item here?
>

no, it is not

Regars

Pavel


>
> ---
>
> On Tue, Mar 15, 2016 at 08:17:07PM -0400, Tom Lane wrote:
> > Pavel Stehule  writes:
> > >> Robert Haas  writes:
> > >>> That's not a dumb idea.  I think %TYPE is an Oracle-ism, and it
> > >>> doesn't seem to have been their best-ever design decision.
> >
> > > Using %TYPE has sense in PostgreSQL too.
> >
> > It's certainly useful functionality; the question is whether this
> > particular syntax is an appropriate base for extended features.
> >
> > As I see it, what we're talking about here could be called type
> operators:
> > given a type name or some other kind of SQL expression, produce the name
> > of a related type.  The existing things of that sort are %TYPE and []
> > (we don't really implement [] as a type operator, but a user could
> > reasonably think of it as one).  This patch proposes to make %TYPE and []
> > composable into a single operator, and then it proposes to add ELEMENT OF
> > as a different operator; and these things are only implemented in
> plpgsql.
> >
> > My concern is basically that I don't want to stop there.  I think we want
> > more type operators in future, such as the rowtype-related operators
> > I sketched upthread; and I think we will want these operators anywhere
> > that you can write a type name.
> >
> > Now, in the core grammar we have [] which can be attached to any type
> > name, and we have %TYPE but it only works in very limited contexts.
> > There's a fundamental problem with extending %TYPE to be used anywhere
> > a type name can: consider
> >
> >   select 'foo'::x%type from t;
> >
> > It's ambiguous whether this is an invocation of %TYPE syntax or whether %
> > is meant to be a regular operator and TYPE the name of a variable.  Now,
> > we could remove that ambiguity by promoting TYPE to be a fully reserved
> > word (it is unreserved today).  But that's not very palatable, and even
> > if we did reserve TYPE, I think we'd still need a lexer kluge to convert
> > %TYPE into a single token, else bison will have lookahead problems.
> > That sort of kluge is ugly, costs performance, and tends to have
> > unforeseen side-effects.
> >
> > So my opinion is that rather than extending %TYPE, we need a new syntax
> > that is capable of being used in more general contexts.
> >
> > There's another problem with the proposal as given: it adds a prefix
> > type operator (ELEMENT OF) where before we only had postfix ones.
> > That means there's an ambiguity about which one binds tighter.  This is
> > not a big deal right now, since there'd be little point in combining
> > ELEMENT OF and [] in the same operation, but it's going to create a mess
> > when we try to add additional type operators.  You're going to need to
> > allow parentheses to control binding order.  I also find it unsightly
> > that the prefix operator looks so little like the postfix operators
> > syntactically, even though they do very similar sorts of things.
> >
> > In short there basically isn't much to like about these syntax details.
> >
> > I also do not like adding the feature to plpgsql first.  At best, that's
> > going to be code we throw away when we implement the same functionality
> > in the core's typename parser.  At worst, we'll have a permanent
> > incompatibility because we find we can't make the core parser use exactly
> > the same syntax.  (For example, it's possible we'd find out we have to
> > make ELEMENT a fully-reserved word in order to use this ELEMENT OF
> syntax.
> > Or maybe it's fine; but until we've tried to cram it into the Typename
> > production, we won't know.  I'm a bit suspicious of expecting it to be
> > fine, though, since AFAICS this patch breaks the ability to use "element"
> > as a plain type name in a plpgsql variable declaration.  Handwritten
> > parsing code like this tends to be full of such gotchas.)
> >
> > 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.
> >
> >   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
>
> --
>   Bruce Momjian  http://momjian.us
>   EnterpriseDB http://enterprisedb.com
>
> + As you are, so once was I. As I am, so you will be. +
> + Ancient Roman gra

Re: [HACKERS] Suspicious behaviour on applying XLOG_HEAP2_VISIBLE.

[Dean Rasheed]

On 26 April 2016 at 04:48, Andres Freund  wrote:
> No, I think we got to do this in all branches. I was just wondering
> about how to fix vm_extend(). Which I do think we got to fix, even in
> the back-branches.
>

I think replacing CacheInvalidateSmgr() with CacheInvalidateRelcache()
in vm_extend() is probably the safer thing to do, and ought to be
relatively harmless.

It means that an index-only scan won't be notified of VM extension
until the end of the other transaction, which might lead to extra heap
fetches, but I think that's unlikely to have any performance impact
because it ought to be a fairly rare event, and if it was another
transaction adding tuples, they wouldn't be all visible before it was
committed anyway, so the extra heap fetches would be required in any
case.

Regards,
Dean


-- 
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] Protocol buffer support for Postgres

[Paul Ramsey]

On Tue, Apr 26, 2016 at 6:40 AM, Tom Lane  wrote:
> Craig Ringer  writes:
>> On 26 April 2016 at 14:06, 陈天舟  wrote:
>>> (1) Since each protocol buffer column requires a schema. I am not sure
>>> where is the best place to store that schema info. Should it be in a
>>> CONSTRAINT (but I am not able to find the doc referring any custom
>>> constraint), or should it be in the COMMENT or somewhere else?
>
>> I can't really imagine how you'd do that without adding a new catalog like
>> we have for enum members. A typmod isn't sufficient since you need a whole
>> lot more than an integer, and typmods aren't tracked throughout the server
>> that well.
>
>> That'll make it hard to do it with an extension.
>
> PostGIS manages to reference quite a lot of schema-like information via
> a geometry column's typmod.  Maybe there's a reason why their approach
> wouldn't be a good fit for this, but it'd be worth investigating.

We pack a short type number, two flags and 24 bits of SRID number into
an integer. The SRID number is in turn a foreign key into the
spatial_ref_sys table where the fully spelled out spatial reference
definition lives.  It's not very nice, and it's quite breakable since
there's no foreign key integrity between the typmod and the
spatial_ref_sys pk.

P.


-- 
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] Protocol buffer support for Postgres

[Tom Lane]

Craig Ringer  writes:
> On 26 April 2016 at 14:06, 陈天舟  wrote:
>> (1) Since each protocol buffer column requires a schema. I am not sure
>> where is the best place to store that schema info. Should it be in a
>> CONSTRAINT (but I am not able to find the doc referring any custom
>> constraint), or should it be in the COMMENT or somewhere else?

> I can't really imagine how you'd do that without adding a new catalog like
> we have for enum members. A typmod isn't sufficient since you need a whole
> lot more than an integer, and typmods aren't tracked throughout the server
> that well.

> That'll make it hard to do it with an extension.

PostGIS manages to reference quite a lot of schema-like information via
a geometry column's typmod.  Maybe there's a reason why their approach
wouldn't be a good fit for this, but it'd be worth investigating.

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

[HACKERS] [PATCH] amroutine->amsupport from numeric to defined constants

[Nikolay Shaplov]

While working with postgres code, I found that for gist index number of 
amsupport procs are defined two times. First in src/include/access/gist.h

#define GISTNProcs  9

and second in src/backend/access/gist/gist.c

amroutine->amsupport = 9;

I think this number should be specified only once, in .h file.

So I would offer a patch for all access methods. That changes amsupport and 
amstrategies from numbers to defined constants. (I've added two of them where 
they were missing)

See attachment.

If it is good, I will add it to the commitfest.

-- 
Nikolay Shaplov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Companydiff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index 9450267..a2450f4 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -35,7 +35,7 @@ ginhandler(PG_FUNCTION_ARGS)
 	IndexAmRoutine *amroutine = makeNode(IndexAmRoutine);
 
 	amroutine->amstrategies = 0;
-	amroutine->amsupport = 6;
+	amroutine->amsupport = GINNProcs;
 	amroutine->amcanorder = false;
 	amroutine->amcanorderbyop = false;
 	amroutine->amcanbackward = false;
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 996363c..a290887 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -57,7 +57,7 @@ gisthandler(PG_FUNCTION_ARGS)
 	IndexAmRoutine *amroutine = makeNode(IndexAmRoutine);
 
 	amroutine->amstrategies = 0;
-	amroutine->amsupport = 9;
+	amroutine->amsupport = GISTNProcs;
 	amroutine->amcanorder = false;
 	amroutine->amcanorderbyop = true;
 	amroutine->amcanbackward = false;
diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c
index 8c89ee7..4fecece 100644
--- a/src/backend/access/hash/hash.c
+++ b/src/backend/access/hash/hash.c
@@ -51,8 +51,8 @@ hashhandler(PG_FUNCTION_ARGS)
 {
 	IndexAmRoutine *amroutine = makeNode(IndexAmRoutine);
 
-	amroutine->amstrategies = 1;
-	amroutine->amsupport = 1;
+	amroutine->amstrategies = HTMaxStrategyNumber;
+	amroutine->amsupport = HASHNProcs;
 	amroutine->amcanorder = false;
 	amroutine->amcanorderbyop = false;
 	amroutine->amcanbackward = true;
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index bf8ade3..013394c 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -84,8 +84,8 @@ bthandler(PG_FUNCTION_ARGS)
 {
 	IndexAmRoutine *amroutine = makeNode(IndexAmRoutine);
 
-	amroutine->amstrategies = 5;
-	amroutine->amsupport = 2;
+	amroutine->amstrategies = BTMaxStrategyNumber;
+	amroutine->amsupport = BTNProcs;
 	amroutine->amcanorder = true;
 	amroutine->amcanorderbyop = false;
 	amroutine->amcanbackward = true;
diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 201203f..bc679bf 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -36,7 +36,7 @@ spghandler(PG_FUNCTION_ARGS)
 	IndexAmRoutine *amroutine = makeNode(IndexAmRoutine);
 
 	amroutine->amstrategies = 0;
-	amroutine->amsupport = 5;
+	amroutine->amsupport = SPGISTNProc;
 	amroutine->amcanorder = false;
 	amroutine->amcanorderbyop = false;
 	amroutine->amcanbackward = false;
diff --git a/src/include/access/hash.h b/src/include/access/hash.h
index 3a68390..fa3f9b6 100644
--- a/src/include/access/hash.h
+++ b/src/include/access/hash.h
@@ -239,6 +239,7 @@ typedef HashMetaPageData *HashMetaPage;
  *	Since we only have one such proc in amproc, it's number 1.
  */
 #define HASHPROC		1
+#define HASHNProcs		1
 
 
 /* public routines */
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index ca50349..d956900 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -454,6 +454,7 @@ typedef struct xl_btree_newroot
 
 #define BTORDER_PROC		1
 #define BTSORTSUPPORT_PROC	2
+#define BTNProcs			2
 
 /*
  *	We need to be able to tell the difference between read and write

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

[HACKERS] Parallel build with MSVC

[Christian Ullrich]

On my system, "build DEBUG" takes ~2.75 minutes. When I tell MSBuild to 
build in parallel by passing it the /m flag, that goes down to 1.5 minutes.


The first patch passes the value of the MSBFLAGS environment variable to 
msbuild's command line.


The output of parallel and sequential builds has identical file count 
and size after installing; all tests pass.


Even without /m, MSBuild will already run enough compilers to keep all 
CPUs busy whenever it can do that with only a single project's files. 
With /m, it will also process _projects_ in parallel.


One additional fix required is to put gendef.pl's "symbols.out" file 
into the directory it is working on, rather than the source tree root, 
to avoid collisions that cause the parallel build to fail otherwise.


--
Christian
From 4b455e1ac38f1441f1faf695da4be8c5eb478970 Mon Sep 17 00:00:00 2001
From: Christian Ullrich 
Date: Tue, 26 Apr 2016 14:52:27 +0200
Subject: [PATCH 1/2] Support passing arbitrary arguments to MSBuild.

This is particularly useful to pass /m, to perform a parallel build.
---
 doc/src/sgml/install-windows.sgml | 11 +--
 src/tools/msvc/build.pl   |  5 +++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/install-windows.sgml 
b/doc/src/sgml/install-windows.sgml
index 74265fc..2b9cf1a 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -137,6 +137,13 @@ $config->{python} = 'c:\python26';
 $ENV{PATH}=$ENV{PATH} . ';c:\some\where\bison\bin';
 
  
+ 
+  To pass additional command line arguments to msbuild (Visual Studio 2010
+  and later):
+
+$ENV{MSBFLAGS}="/m";
+
+ 
 
  
   Requirements
@@ -361,12 +368,12 @@ $ENV{PATH}=$ENV{PATH} . ';c:\some\where\bison\bin';
 
To build all of PostgreSQL in debug configuration, run the command:
 
-build DEBUG
+build -c DEBUG
 
To build just a single project, for example psql, run the commands:
 
 build psql
-build DEBUG psql
+build -c DEBUG psql
 
To change the default build configuration to debug, put the following
in the buildenv.pl file:
diff --git a/src/tools/msvc/build.pl b/src/tools/msvc/build.pl
index c4e4dc7..85109a2 100644
--- a/src/tools/msvc/build.pl
+++ b/src/tools/msvc/build.pl
@@ -38,6 +38,7 @@ my $vcver = Mkvcbuild::mkvcbuild($config);
 # check what sort of build we are doing
 
 my $bconf = $ENV{CONFIG} || "Release";
+my $msbflags  = $ENV{MSBFLAGS} || "";
 my $buildwhat = $ARGV[1] || "";
 if (uc($ARGV[0]) eq 'DEBUG')
 {
@@ -53,7 +54,7 @@ elsif (uc($ARGV[0]) ne "RELEASE")
 if ($buildwhat and $vcver >= 10.00)
 {
system(
-"msbuild $buildwhat.vcxproj /verbosity:normal /p:Configuration=$bconf");
+"msbuild $buildwhat.vcxproj $msbflags /verbosity:normal 
/p:Configuration=$bconf");
 }
 elsif ($buildwhat)
 {
@@ -61,7 +62,7 @@ elsif ($buildwhat)
 }
 else
 {
-   system("msbuild pgsql.sln /verbosity:normal /p:Configuration=$bconf");
+   system("msbuild pgsql.sln $msbflags /verbosity:normal 
/p:Configuration=$bconf");
 }
 
 # report status
-- 
2.8.1.windows.1

From 51cccd968b5ba98da4d971d9c69b59ec85cfa513 Mon Sep 17 00:00:00 2001
From: Christian Ullrich 
Date: Tue, 26 Apr 2016 14:52:53 +0200
Subject: [PATCH 2/2] Support parallel build with MSBuild.

gendef.pl now puts the temporary output file into the target directory instead 
of the source tree root, to avoid collisions.
---
 src/tools/msvc/gendef.pl | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/tools/msvc/gendef.pl b/src/tools/msvc/gendef.pl
index 8ccaab3..4bd05fa 100644
--- a/src/tools/msvc/gendef.pl
+++ b/src/tools/msvc/gendef.pl
@@ -4,6 +4,7 @@ use warnings;
 use strict;
 use 5.8.0;
 use List::Util qw(max);
+use File::Spec::Functions qw(splitpath catpath);
 
 #
 # Script that generates a .DEF file for all objects in a directory
@@ -14,9 +15,11 @@ use List::Util qw(max);
 sub dumpsyms
 {
my ($objfile, $symfile) = @_;
-   system("dumpbin /symbols /out:symbols.out $_ >NUL")
+   my ($symvol, $symdirs, $symbase) = splitpath($symfile);
+   my $tmpfile = catpath($symvol, $symdirs, "symbols.out");
+   system("dumpbin /symbols /out:$tmpfile $_ >NUL")
  && die "Could not call dumpbin";
-   rename("symbols.out", $symfile);
+   rename($tmpfile, $symfile);
 }
 
 # Given a symbol file path, loops over its contents
-- 
2.8.1.windows.1


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

Re: [HACKERS] postgres_fdw : Not able to update foreign table referring to a local table's view when use_remote_estimate = true

[Etsuro Fujita]

On 2016/04/26 12:52, Robert Haas wrote:

On Mon, Apr 25, 2016 at 2:54 AM, Ashutosh Bapat
 wrote:

Thinking loudly:

This error is hard to interpret for a user who doesn't know about ctid. Till
we find a solution, we can at least fail gracefully with an error something
like "DMLs are not supported on foreign tables referring to views/non-tables
on foreign server" is not supported. While creating the foreign table a user
can specify whether the object being referred is updatable (writable?) or
not, Import foreign schema can set the status by looking at pg_class type
entry. The efforts required may not be worth the usage given that this case
is highly unlikely. May be we should just update the documents saying that a
user may encounter such an error if s/he attempts to update/delete such a
foreign table.



I would be disinclined to add code specifically to catch this, since
the only report we have so far is from someone whose job is to find
corner cases that break.  Which he did, and good for him, but like you
say, that doesn't mean it's a big real-world problem.  Adding a
sentence to the documentation stating that pointing a foreign table at
a view is fine for SELECT, but that UPDATE and DELETE may not work,
seems like enough.


+1 for just updating the documentation.

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] Optimization for updating foreign tables in Postgres FDW

[Etsuro Fujita]

Hi,

While re-reviewing the fix, I noticed that since PQcancel we added to 
pgfdw_xact_callback to cancel a DML pushdown query isn't followed by a 
ROLLBACK, the connection to the remote server will be discarded at the 
end of the while loop in that function, which will cause a FATAL error 
of "connection to client lost".  Probably, that was proposed by me in 
the first version of the patch, but I don't think that's a good idea. 
Shouldn't we execute ROLLBACK after that PQcancel?


Another thing I noticed is, ISTM that we miss the case where DML 
pushdown queries are performed in subtransactions.  I think cancellation 
logic would also need to be added to pgfdw_subxact_callback.


Comments are welcome!

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] Protocol buffer support for Postgres

[José Luis Tallón]

On 04/26/2016 08:06 AM, 陈天舟 wrote:
I am interested in adding Protocol Buffer support for Postgres. 
Protocol Buffer occupies less space than JSON. More importantly, it 
has schema and is forward/backward compatible. All these make it a 
very good format for persistency.


Have you investigated JSONB vs ProtoBuf space usage ?
(the key being  the "B" -- Postgres' own binary JSON implementation)


The "per-column schema" thing sounds difficult to do without major 
changes to the core unless/until we have generalized user-defined 
metadata for objects 



Just my .02€

/ J.L.



--
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] [COMMITTERS] pgsql: Add trigonometric functions that work in degrees.

[Dean Rasheed]

On 26 April 2016 at 04:25, Tom Lane  wrote:
> In short, these tests suggest that we need a coding pattern like
> this:
>
> volatile float8 asin_x = asin(x);
> return (asin_x / asin_0_5) * 30.0;
>
> We could drop the "volatile" by adopting -ffloat-store, but that
> doesn't seem like a better answer, because -ffloat-store would
> pessimize a lot of code elsewhere.  (It causes a whole lot of
> changes in float.c, for sure.)

Agreed. That looks like the least hacky way of solving the problem. I
think it's more readable when the logic is kept local, and it's
preferable to avoid any compiler-specific options or global flags that
would affect other code.

6b1a213bbd6599228b2b67f7552ff7cc378797bf by itself looks like a
worthwhile improvement that ought to be more robust, even though it
didn't fix this problem.

Regards,
Dean


-- 
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_basebackup creates a corrupt file for pg_stat_tmp and pg_replslot on a backup location

[Ashutosh Sharma]

Hi,

Knowing that pg_basebackup always creates an empty directory for
pg_stat_tmp and pg_replslot in backup location, even i think it would be
better to handle these directories in such a way that pg_basebackup
generates an empty directory for pg_replslot and pg_stat_tmp if they are
symbolic link.

PFA patch for the same.

With Regards,
Ashutosh Sharma
EnterpriseDB: *http://www.enterprisedb.com *

On Thu, Apr 14, 2016 at 11:57 PM, Magnus Hagander 
wrote:

> On Thu, Apr 14, 2016 at 8:20 PM, Ashutosh Sharma 
> wrote:
>
>> Hi,
>>
>> I was just curious to know how would "*pg_basebackup*" behave if we do
>> create a symbolic link for directories other than pg_xlog/pg_tblspc.
>> However it is clearly mentioned in the documentation of pg_basebackup that
>> if a  Symbolic link for the directories other than pg_tblspc and pg_xlog is
>> created then it would be skipped. But, that is not the case for pg_replslot
>> and pg_stat_tmp. Is this not an issue. Should these directories not be
>> skipped. Please let me know your thoughts on this. Thanks.
>>
>
> I agree that actually generating a corrupt tarfile is not good. But I
> think the correct fix is to actually generate an empty placeholder
> directory rather than skipping it - thereby making the backup look the same
> as it would if it was a correct directory where we just skipped the
> contents.
>
> --
>  Magnus Hagander
>  Me: http://www.hagander.net/
>  Work: http://www.redpill-linpro.com/
>
diff --git a/src/backend/replication/basebackup.c 
b/src/backend/replication/basebackup.c
index 1008873..5e3932b 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -970,7 +970,16 @@ sendDir(char *path, int basepathlen, bool sizeonly, List 
*tablespaces,
  strncmp(de->d_name, PG_STAT_TMP_DIR, strlen(PG_STAT_TMP_DIR)) 
== 0)
{
if (!sizeonly)
+   {
+   /* If pg_stat_tmp is a symlink, write it as a 
directory anyway */
+#ifndef WIN32
+   if (S_ISLNK(statbuf.st_mode))
+#else
+   if (pgwin32_is_junction(pathbuf))
+#endif
+   statbuf.st_mode = S_IFDIR | S_IRWXU;
_tarWriteHeader(pathbuf + basepathlen + 1, 
NULL, &statbuf);
+   }
size += 512;
continue;
}
@@ -982,7 +991,16 @@ sendDir(char *path, int basepathlen, bool sizeonly, List 
*tablespaces,
if (strcmp(de->d_name, "pg_replslot") == 0)
{
if (!sizeonly)
+   {
+   /* If pg_replslot is a symlink, write it as a 
directory anyway */
+#ifndef WIN32
+   if (S_ISLNK(statbuf.st_mode))
+#else
+   if (pgwin32_is_junction(pathbuf))
+#endif
+   statbuf.st_mode = S_IFDIR | S_IRWXU;
_tarWriteHeader(pathbuf + basepathlen + 1, 
NULL, &statbuf);
+   }
size += 512;/* Size of the header just 
added */
continue;
}

-- 
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] Declarative partitioning

[Erik Rijkers]

On 2016-04-15 04:35, Amit Langote wrote:

A quick test with:


0001-Add-syntax-to-specify-partition-key-v3.patch
0002-Infrastructure-for-creation-of-partitioned-tables-v3.patch
0003-Add-syntax-to-create-partitions-v3.patch
0004-Infrastructure-for-partition-metadata-storage-and-ma-v3.patch
0005-Introduce-tuple-routing-for-partitioned-tables-v3.patch


patches apply, build and make check ok.

There is somwthing wrong with indexes on child tables (and only with 
higher rowcounts).


Surely the below code should give 6 rows; it actually does return 6 rows 
without the indexes.

With indexes it returns 0 rows.

(but when doing the same test with low rowcounts, things are OK.)


thanks,

Erik Rijkers


(Linux Centos 6.6)


---
create table inh(a int, b int) partition by range ((a+b));
create table inh_1 partition of inh for values start ( 0) end (  
1);
create table inh_2 partition of inh for values start ( 1) end (  
2);
create table inh_3 partition of inh for values start ( 2) end ( 
10);


create index inh_1_a_idx on inh_1 (a);
create index inh_2_a_idx on inh_2 (a);
create index inh_3_a_idx on inh_3 (a);

insert into inh select i, i as j from generate_series(1, 1) as f(i);

analyze inh_1;
analyze inh_2;
analyze inh_3;

  select 'inh'  , count(*)  from inh
union all select 'inh_1', count(*)  from inh_1
union all select 'inh_2', count(*)  from inh_2
union all select 'inh_3', count(*)  from inh_3
;

explain analyze  select * from inh where a between 10110 and 10115;
---



# output :

create table inh(a int, b int) partition by range ((a+b));
create table inh_1 partition of inh for values start ( 0) end (  
1);  create index inh_1_a_idx on inh_1 (a);
create table inh_2 partition of inh for values start ( 1) end (  
2);  create index inh_2_a_idx on inh_2 (a);
create table inh_3 partition of inh for values start ( 2) end ( 
10);  create index inh_3_a_idx on inh_3 (a);

insert into inh select i, i as j from generate_series(1, 1) as f(i);
analyze inh_1;
analyze inh_2;
analyze inh_3;
  select 'inh'  , count(*)  from inh
union all select 'inh_1', count(*)  from inh_1
union all select 'inh_2', count(*)  from inh_2
union all select 'inh_3', count(*)  from inh_3
;
 ?column? | count
--+---
 inh  | 1
 inh_1|  4999
 inh_2|  5000
 inh_3| 1
(4 rows)

explain analyze  select * from inh where a between 10110 and 10115;
   QUERY PLAN
-
 Append  (cost=0.00..17.37 rows=4 width=8) (actual time=0.023..0.023 
rows=0 loops=1)
   ->  Seq Scan on inh  (cost=0.00..0.00 rows=1 width=8) (actual 
time=0.004..0.004 rows=0 loops=1)

 Filter: ((a >= 10110) AND (a <= 10115))
   ->  Index Scan using inh_1_a_idx on inh_1  (cost=0.16..8.18 rows=1 
width=8) (actual time=0.007..0.007 rows=0 loops=1)

 Index Cond: ((a >= 10110) AND (a <= 10115))
   ->  Index Scan using inh_2_a_idx on inh_2  (cost=0.16..8.18 rows=1 
width=8) (actual time=0.002..0.002 rows=0 loops=1)

 Index Cond: ((a >= 10110) AND (a <= 10115))
   ->  Seq Scan on inh_3  (cost=0.00..1.01 rows=1 width=8) (actual 
time=0.008..0.008 rows=0 loops=1)

 Filter: ((a >= 10110) AND (a <= 10115))
 Rows Removed by Filter: 1
 Planning time: 0.858 ms
 Execution time: 0.093 ms
(12 rows)





--
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 execProcnode.c documentation

[Magnus Hagander]

On Tue, Apr 26, 2016 at 10:34 AM, Daniel Gustafsson  wrote:

> Theres a tiny typo in the EXAMPLE section of the documentation in
> execProcnode.c:
>
> - *  their work to the appopriate node support routines which may
> + *  their work to the appropriate node support routines which may
>


Applied, thanks.

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

[HACKERS] Typo in execProcnode.c documentation

[Daniel Gustafsson]

Theres a tiny typo in the EXAMPLE section of the documentation in
execProcnode.c:

- *  their work to the appopriate node support routines which may
+ *  their work to the appropriate node support routines which may

cheers ./daniel



execProcnode-type.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] Can we improve this error message?

[Magnus Hagander]

On Apr 26, 2016 4:41 AM, "Tom Lane"  wrote:
>
> Andreas Karlsson  writes:
> > On 04/17/2016 09:28 PM, Bill Moran wrote:
> >> What I believe is happening, is that the pg connection libs
> >> first try to connect via ssl and get a password failed error,
> >> then fallback to trying to connect without ssl, and get a "no
> >> pg_hba.conf entry" error. The problem is that the second error
> >> masks the first one, hiding the real cause of the connection
> >> failure, and causing a lot of confusion.
>
> > I got both the messages when I tried this with psql. What did you do
> > when you only got the second message?
>
> Maybe Bill tried it with a rather old libpq?  This rings a bell
> as being something we fixed awhile back.
>

Yeah, libpq used to keep just one error message. Iirc, this was changed
quite long ago though, but I guess if it's a really old libpq..

/Magnus