Re: [HACKERS] POC: Cache data in GetSnapshotData()

[Amit Kapila]

On Thu, Mar 10, 2016 at 1:04 PM, Mithun Cy 
wrote:

>
>
> On Thu, Mar 3, 2016 at 11:50 PM, Robert Haas 
> wrote:
> >What if you apply both this and Amit's clog control log patch(es)?  Maybe
> the combination of the two helps substantially more than either >one alone.
>
>
> I did the above tests along with Amit's clog patch. Machine :8 socket, 64
> core. 128 hyperthread.
>
> clients BASE ONLY CLOG CHANGES % Increase ONLY SAVE SNAPSHOT % Increase CLOG
> CHANGES + SAVE SNAPSHOT % Increase
> 64 29247.658034 30855.728835 5.4981181711 29254.532186 0.0235032562
> 32691.832776 11.7758992463
> 88 31214.305391 33313.393095 6.7247618606 32109.248609 2.8670931702
> 35433.655203 13.5173592978
> 128 30896.673949 34015.362152 10.0939285832 *** *** 34947.296355
> 13.110221549
> 256 27183.780921 31192.895437 14.7481857938 *** *** 32873.782735
> 20.9316056164
> With clog changes, perf of caching the snapshot patch improves.
>
>
This data looks promising to me and indicates that saving the snapshot has
benefits and we can see noticeable performance improvement especially once
the CLog contention gets reduced.  I wonder if we should try these tests
with unlogged tables, because I suspect most of the contention after
CLogControlLock and ProcArrayLock is for WAL related locks, so you might be
able to see better gain with these patches.  Do you think it makes sense to
check the performance by increasing CLOG buffers (patch for same is posted
in Speed up Clog thread [1]) as that also relieves contention on CLOG as
per the tests I have done?


[1] -
http://www.postgresql.org/message-id/caa4ek1lmmgnq439bum0lcs3p0sb8s9kc-cugu_thnqmwa8_...@mail.gmail.com

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

Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.

[Amit Langote]

Hi Vinayak,

Thanks for the quick review!

On 2016/03/10 16:22, poku...@pm.nttdata.co.jp wrote:
>> On 2016/03/10 14:29, Amit Langote wrote:
>> Updated patches attached.
> In 0002-

[ snip ]

> I think we need to use datid instead of datname.
> Robert added datid in pg_stat_get_progress_info() and we are using that 
> function here.
> +values[1] = ObjectIdGetDatum(beentry->st_databaseid);

[ snip ]

> So I think it's better to report datid not datname.
> The definition of view is simply like:
> +CREATE VIEW pg_stat_progress_vacuum AS
> +SELECT
> +S.pid AS pid,
> +S.datid AS datid,
> +S.relid AS relid,
> +CASE S.param1
> +WHEN 1 THEN 'scanning heap'
> +WHEN 2 THEN 'vacuuming indexes'
> +WHEN 3 THEN 'vacuuming heap'
> +WHEN 4 THEN 'cleanup'
> +ELSE 'unknown phase'
> +END AS processing_phase,
> +S.param2 AS total_heap_blocks,
> +S.param3 AS current_heap_block,
> +S.param4 AS total_index_blocks,
> +S.param5 AS index_blocks_done,
> +S.param6 AS index_vacuum_count,
> +CASE S.param2
> +WHEN 0 THEN round(100.0, 2)
> + ELSE round((S.param3 + 1) * 100.0 / S.param2, 2)
> +END AS percent_done
> +FROM pg_stat_get_progress_info('VACUUM') AS S;
> 
> So maybe we can add datname as separate column in pg_stat_progress_vacuum, I 
> think it's not required only datid is sufficient.
> Any comment?

Why do you think showing the name may be unacceptable?  Wouldn't that be a
little more user-friendly?  Though maybe, we can follow the
pg_stat_activity style and have both instead, as you suggest.  Attached
updated version does that.

Thanks,
Amit
>From 7cb702c7fae9fceef3048a82522390844c5a67cc Mon Sep 17 00:00:00 2001
From: amit 
Date: Thu, 10 Mar 2016 11:25:20 +0900
Subject: [PATCH 1/3] Some minor fixes in commit b6fb6471.

---
 src/backend/postmaster/pgstat.c |7 ---
 src/backend/utils/adt/pgstatfuncs.c |2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index ce5da3e..4424cb8 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -2861,8 +2861,8 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 /*---
  * pgstat_progress_start_command() -
  *
- * Set st_command in own backend entry.  Also, zero-initialize
- * st_progress_param array.
+ * Set st_progress_command (and st_progress_command_target) in own backend
+ * entry.  Also, zero-initialize st_progress_param array.
  *---
  */
 void
@@ -2904,7 +2904,8 @@ pgstat_progress_update_param(int index, int64 val)
 /*---
  * pgstat_progress_end_command() -
  *
- * Update index'th member in st_progress_param[] of own backend entry.
+ * Reset st_progress_command (and st_progress_command_target) in own backend
+ * entry.  This signals the end of the command.
  *---
  */
 void
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 0c790ff..2fb51fa 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -614,7 +614,7 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS)
 		else
 		{
 			nulls[2] = true;
-			for (i = 1; i < PGSTAT_NUM_PROGRESS_PARAM + 1; i++)
+			for (i = 0; i < PGSTAT_NUM_PROGRESS_PARAM; i++)
 nulls[i+3] = true;
 		}
 
-- 
1.7.1

>From 9e3ac989d8583c6ba5e74945602aeaacfaee127f Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 7 Mar 2016 14:38:34 +0900
Subject: [PATCH 2/3] WIP: Implement progress reporting for VACUUM command.

This basically utilizes the pgstat_progress* API to report a handful of
paramters to indicate its progress.  lazy_vacuum_rel() and lazy_scan_heap()
have been altered to report command start, command target table, and the
following parameters: processing phase, number of heap blocks, number of
index blocks (all indexes), current heap block number in the main scan loop
(whenever changes), index blocks vacuumed (once per finished index vacuum),
and number of index vacuum passes (every time when all indexes are vacuumed).
Following processing phases are identified and reported whenever one changes
to another: 'scanning heap', 'vacuuming indexes', 'vacuuming heap', and
'cleanup'.

TODO: find a way to report index pages vacuumed in a more granular manner than
the current report per index vacuumed.

A view named pg_stat_progress_vacuum has been added that shows these values.
---
 doc/src/sgml/monitoring.sgml |  111 ++
 src/backend/catalog/system_views.sql |   25 
 src/backend/commands/vacuumlazy.c|   73 ++-
 src/test/regress/expected/rules.out  |   23 +++
 4 files changed, 231 insertions(+), 1 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sg

Re: [HACKERS] Is there a way around function search_path killing SQL function inlining?

[Regina Obe]

-Original Message-
> From: Andreas Karlsson [mailto:andr...@proxel.se] 
> Sent: Tuesday, March 08, 2016 10:43 PM
> To: Regina Obe ; 'Robert Haas' 
> Cc: pgsql-hackers@postgresql.org
> Subject: Re: [HACKERS] Is there a way around function search_path killing SQL 
> function inlining?


> Hi,

> I think Robert was asking for a test case for the database restore problems.

> The reason your no_inline() function cannot be inlined is due to lack of 
> support of inlining of any functions which have any config variable set, not 
> matter which. The search_path does not get any special treatment, and I am 
> not sure if it
> could in the general case since the new search path will apply too to 
> functions called by the function which changed the search path.

> Andreas

Restore has been an issue since as far back as I can remember.  It's more of an 
issue now now that people are using materialized views and raster constraints.
Anytime you have a materialized view or check constraint on a table that  uses 
a function that calls a non-schema qualified function you have a problem.

For a simple example lets say you created a database like this:
-- code start here --
CREATE DATABASE test;
ALTER DATABASE test
  SET search_path = public,funcs;

\connect test;
CREATE SCHEMA funcs;
CREATE OR REPLACE FUNCTION funcs._helper(box, box) RETURNS float8 AS
$$
  SELECT box_distance($1,$2);
$$
language 'sql' IMMUTABLE STRICT;
CREATE OR REPLACE FUNCTION funcs.inline(box,box) RETURNS boolean AS
$$
 SELECT $1 && $2 AND _helper($1,$2) = 0;
$$
language 'sql' IMMUTABLE;


CREATE TABLE bag_boxes(id serial primary key, geom box);
CREATE INDEX idx_bag_boxes_geom ON bag_boxes USING gist(geom);

INSERT INTO bag_boxes(geom)
SELECT ('((' || i::text || ',' || j::text || '), (' || k::text || ', ' || 
l::text || '))')::box
FROM generate_series(1,10) i , generate_series(11,20) j, generate_series(5,10) 
k, generate_series(10, 15) l ;


CREATE MATERIALIZED VIEW vw_bag_boxes AS
SELECT *
FROM bag_boxes 
WHERE funcs.inline('((1,2),(3,4))'::box, geom);

-- code end here --


When you back up the database, it would create a backup with this line:

SET search_path = public, pg_catalog;
--your create materialized view here

When you restore even if your database has search_paths set, your materialized 
view will not come back and will error out with:

ERROR:  function _helper(box, box) does not exist
LINE 2:  SELECT $1 && $2 AND _helper($1,$2) = 0;
 ^
HINT:  No function matches the given name and argument types. You might need to 
add explicit type casts.
QUERY:
 SELECT $1 && $2 AND _helper($1,$2) = 0;

In the case of table constraints, if you have any that rely on functions like 
this, your data fails validation so will not come back.

Ideally it would be nice if pg_dump allowed specifying additional schemas to 
add to the search_path.

We have a similar issue with Foreign tables, but that's probably a harder one 
to fix.

Anyway it seems I underestimated the many ways setting search path on functions 
(even ones that don't rely on anything else as far as I can tell) screws up 
performance
Even when it doesn't affect index usage so that has to be done with caution I 
guess.

Thanks,
Regina











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

Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.

[pokurev]

Hi Amit,

Thank you for updating the patch.
> -Original Message-
> From: Amit Langote [mailto:langote_amit...@lab.ntt.co.jp]
> Sent: Thursday, March 10, 2016 5:09 PM
> To: SPS ポクレ ヴィナヤック(三技術) ;
> robertmh...@gmail.com
> Cc: horiguchi.kyot...@lab.ntt.co.jp; amitlangot...@gmail.com; pgsql-
> hack...@postgresql.org; SPS 坂野 昌平(三技術) 
> Subject: Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.
> 
> 
> Hi Vinayak,
> 
> Thanks for the quick review!
> 
> On 2016/03/10 16:22, poku...@pm.nttdata.co.jp wrote:
> >> On 2016/03/10 14:29, Amit Langote wrote:
> >> Updated patches attached.
> > In 0002-
> 
> [ snip ]
> 
> > I think we need to use datid instead of datname.
> > Robert added datid in pg_stat_get_progress_info() and we are using that
> function here.
> > +values[1] = ObjectIdGetDatum(beentry->st_databaseid);
> 
> [ snip ]
> 
> > So I think it's better to report datid not datname.
> > The definition of view is simply like:
> > +CREATE VIEW pg_stat_progress_vacuum AS
> > +SELECT
> > +S.pid AS pid,
> > +S.datid AS datid,
> > +S.relid AS relid,
> > +CASE S.param1
> > +WHEN 1 THEN 'scanning heap'
> > +WHEN 2 THEN 'vacuuming indexes'
> > +WHEN 3 THEN 'vacuuming heap'
> > +WHEN 4 THEN 'cleanup'
> > +ELSE 'unknown phase'
> > +END AS processing_phase,
> > +S.param2 AS total_heap_blocks,
> > +S.param3 AS current_heap_block,
> > +S.param4 AS total_index_blocks,
> > +S.param5 AS index_blocks_done,
> > +S.param6 AS index_vacuum_count,
> > +CASE S.param2
> > +WHEN 0 THEN round(100.0, 2)
> > +   ELSE round((S.param3 + 1) * 100.0 / S.param2, 2)
> > +END AS percent_done
> > +FROM pg_stat_get_progress_info('VACUUM') AS S;
> >
> > So maybe we can add datname as separate column in
> pg_stat_progress_vacuum, I think it's not required only datid is sufficient.
> > Any comment?
> 
> Why do you think showing the name may be unacceptable?  Wouldn't that
> be a little more user-friendly?  Though maybe, we can follow the
> pg_stat_activity style and have both instead, as you suggest.  Attached
> updated version does that.
+1
I think reporting both (datid and datname) is more user-friendly.
Thank you.

Regards,
Vinayak Pokale

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

[HACKERS] utils/misc: Simplify search of end of argv in save_ps_display_args()

[Alexander Kuleshov]

Hello,

Attached patch provides trivial simplification of the search of end of
the argv[] area by replacing for() loop with calculation based on
argc.
diff --git a/src/backend/utils/misc/ps_status.c b/src/backend/utils/misc/ps_status.c
index 892a810..d8f2105 100644
--- a/src/backend/utils/misc/ps_status.c
+++ b/src/backend/utils/misc/ps_status.c
@@ -134,11 +134,8 @@ save_ps_display_args(int argc, char **argv)
 		/*
 		 * check for contiguous argv strings
 		 */
-		for (i = 0; i < argc; i++)
-		{
-			if (i == 0 || end_of_area + 1 == argv[i])
-end_of_area = argv[i] + strlen(argv[i]);
-		}
+		if (argc)
+			end_of_area = argv[argc - 1] + strlen(argv[argc - 1]);
 
 		if (end_of_area == NULL)	/* probably can't happen? */
 		{

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

[HACKERS] Floating point timestamps

[Thomas Munro]

Hi,

Is the plan to remove support for floating point timestamps at some
stage?  If so, what is that waiting on, and would it provide
sufficient warning if (say) 9.6 were documented as the last major
release to support that build option?

Thanks!

-- 
Thomas Munro
http://www.enterprisedb.com


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

Re: [HACKERS] Improving replay of XLOG_BTREE_VACUUM records

[Simon Riggs]

On 10 March 2016 at 06:27, Michael Paquier 
wrote:

> On Thu, Mar 10, 2016 at 1:29 AM, David Steele  wrote:
> > On 1/8/16 9:34 AM, Alvaro Herrera wrote:
> >> Simon Riggs wrote:
> >>>
> >>> On 8 January 2016 at 13:36, Alvaro Herrera 
> >>> wrote:
> 
>  I would agree except for the observation on toast indexes.  I think
>  that's an important enough use case that perhaps we should have both.
> >>>
> >>> The exclusion of toast indexes is something we can remove also, I have
> >>> recently discovered. When we access toast data we ignore MVCC, but we
> >>> still
> >>> have the toast pointer and chunkid to use for rechecking our scan
> >>> results.
> >>> So a later patch will add some rechecks.
> >>
> >> Ah, interesting, glad to hear.  I take it you're pushing your patch
> >> soon, then?
> >
> > ISTM that this patch should be "returned with feedback" or "rejected"
> based
> > on the thread.  I'm marking it "waiting for author" for the time being.
>
> I think that we are still waiting for some input from Simon here...
> Simon, are you going to finish wrapping up your other patch?
>

Yes, I have done the research, so think patch should be rejected now.

Thanks to everyone for their input. It's good to have alternate approaches.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Relation extension scalability

[Dilip Kumar]

On Thu, Mar 10, 2016 at 7:55 AM, Petr Jelinek  wrote:

Thanks for the comments..


> Hmm, why did you remove the comment above the call to
> UnlockRelationForExtension?

While re factoring I lose this comment.. Fixed it


> It still seems relevant, maybe with some minor modification?
>
> Also there is a bit of whitespace mess inside the conditional lock block.


Fixed

I got the result of 10 mins run so posting it..
Note: Base code results are copied from up thread...

Results For 10 Mins run of COPY 1 records of size 4 bytes script and
configuration are same as used in up thread


ClientBasePatch
1105111
2217246
4210428
8166653
16  145808
32  124988
64---974


Results For 10 Mins run of INSERT 1000 records of size 1024 bytes(data
don't fits in shared buffer)
--

ClientBasePatch
1117120
2111126
4 51 130
8 43 147
16   40 209
32   ---  254
64   ---  205

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 8140418..b73535c 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -169,6 +169,87 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
 }
 
 /*
+ * RelationAddOneBlock
+ *
+ * Extend relation by one block and lock the buffer
+ */
+static Buffer
+RelationAddOneBlock(Relation relation, Buffer otherBuffer, BulkInsertState bistate)
+{
+	Buffer buffer;
+	/*
+	 * XXX This does an lseek - rather expensive - but at the moment it is the
+	 * only way to accurately determine how many blocks are in a relation.  Is
+	 * it worth keeping an accurate file length in shared memory someplace,
+	 * rather than relying on the kernel to do it for us?
+	 */
+	buffer = ReadBufferBI(relation, P_NEW, bistate);
+
+	/*
+	 * We can be certain that locking the otherBuffer first is OK, since
+	 * it must have a lower page number.
+	 */
+	if (otherBuffer != InvalidBuffer)
+		LockBuffer(otherBuffer, BUFFER_LOCK_EXCLUSIVE);
+
+	/*
+	 * Now acquire lock on the new page.
+	 */
+	LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
+
+	return buffer;
+}
+
+/*
+ * RelationAddExtraBlocks
+ *
+ * Extend extra blocks for the relations to avoid the future contention
+ * on the relation extension lock.
+ */
+
+static void
+RelationAddExtraBlocks(Relation relation, BulkInsertState bistate)
+{
+	Page		page;
+	Size		freespace;
+	BlockNumber blockNum;
+	int			extraBlocks = 0;
+	int 		lockWaiters = 0;
+	Buffer		buffer;
+
+	/*
+	 * For calculating number of extra blocks to extend, find the level
+	 * of contention on this lock, by getting the requester of this lock
+	 * and add extra blocks in multiple of waiters.
+	 */
+	lockWaiters = RelationExtensionLockWaiter(relation);
+
+	extraBlocks = lockWaiters * 20;
+
+	while (extraBlocks--)
+	{
+		/*
+		 * Here we are adding extra blocks to the relation after
+		 * adding each block update the information in FSM so that
+		 * other backend running parallel can find the block.
+		 */
+		buffer = ReadBufferBI(relation, P_NEW, bistate);
+
+		LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE);
+
+		page = BufferGetPage(buffer);
+		PageInit(page, BufferGetPageSize(buffer), 0);
+		freespace = PageGetHeapFreeSpace(page);
+		MarkBufferDirty(buffer);
+		blockNum = BufferGetBlockNumber(buffer);
+
+		UnlockReleaseBuffer(buffer);
+
+		RecordPageWithFreeSpace(relation, blockNum, freespace);
+	}
+}
+
+/*
  * RelationGetBufferForTuple
  *
  *	Returns pinned and exclusive-locked buffer of a page in given relation
@@ -233,10 +314,11 @@ RelationGetBufferForTuple(Relation relation, Size len,
 	bool		use_fsm = !(options & HEAP_INSERT_SKIP_FSM);
 	Buffer		buffer = InvalidBuffer;
 	Page		page;
-	Size		pageFreeSpace,
-saveFreeSpace;
+	Size		pageFreeSpace = 0,
+saveFreeSpace = 0;
 	BlockNumber targetBlock,
-otherBlock;
+otherBlock,
+lastValidBlock = InvalidBlockNumber;
 	bool		needLock;
 
 	len = MAXALIGN(len);		/* be conservative */
@@ -308,6 +390,7 @@ RelationGetBufferForTuple(Relation relation, Size len,
 		}
 	}
 
+loop:
 	while (targetBlock != InvalidBlockNumber)
 	{
 		/*
@@ -388,6 +471,8 @@ RelationGetBufferForTuple(Relation relation, Size len,
  otherBlock, targetBlock, vmbuffer_other,
  vmbuffer);
 
+		lastValidBlock = targetBlock;
+
 		/*
 		 * Now we can check to see if there's enough free space here. If so,
 		 * we're done.
@@ -441,27 +526,47 @@ RelationGetBufferForTuple(Relation relation, Size len,
 	needLock = !RELATION_IS_LOCAL(relation);
 
 	if (needLock)
-		LockRelationForExtension(relation, ExclusiveLock);
-
-	/*
-	 * XXX This does an lseek - rat

Re: [HACKERS] Improving replay of XLOG_BTREE_VACUUM records

[Vladimir Borodin]

> 10 марта 2016 г., в 11:50, Simon Riggs  написал(а):
> 
> On 10 March 2016 at 06:27, Michael Paquier  > wrote:
> On Thu, Mar 10, 2016 at 1:29 AM, David Steele  > wrote:
> > On 1/8/16 9:34 AM, Alvaro Herrera wrote:
> >> Simon Riggs wrote:
> >>>
> >>> On 8 January 2016 at 13:36, Alvaro Herrera  >>> >
> >>> wrote:
> 
>  I would agree except for the observation on toast indexes.  I think
>  that's an important enough use case that perhaps we should have both.
> >>>
> >>> The exclusion of toast indexes is something we can remove also, I have
> >>> recently discovered. When we access toast data we ignore MVCC, but we
> >>> still
> >>> have the toast pointer and chunkid to use for rechecking our scan
> >>> results.
> >>> So a later patch will add some rechecks.
> >>
> >> Ah, interesting, glad to hear.  I take it you're pushing your patch
> >> soon, then?
> >
> > ISTM that this patch should be "returned with feedback" or "rejected" based
> > on the thread.  I'm marking it "waiting for author" for the time being.
> 
> I think that we are still waiting for some input from Simon here...
> Simon, are you going to finish wrapping up your other patch?
> 
> Yes, I have done the research, so think patch should be rejected now.

Let’s do immediately after you will send a new version of your patch? Or even 
better after testing your patch? Don’t get me wrong, but rejecting my patch 
without tangible work on your patch may lead to forgiving about the problem 
before 9.6 freeze.

> 
> Thanks to everyone for their input. It's good to have alternate approaches.
> 
> -- 
> Simon Riggshttp://www.2ndQuadrant.com/ 
> 
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


--
May the force be with you…
https://simply.name

[HACKERS] Small patch for pgstat.c: fix comment + pgindent

[Aleksander Alekseev]

Hello

I noticed:

http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b6fb6471f6afaf649e52f38269fd8c5c60647669

... that comments for procedures pgstat_progress_update_param and
pgstat_progress_end_command are identical. Here is a patch that fixes
this. Also one empty line added after running pgindent on this file.

Best regards,
Aleksanderdiff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index ce5da3e..5005fe5 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -2733,6 +2733,7 @@ pgstat_bestart(void)
 	beentry->st_activity[pgstat_track_activity_query_size - 1] = '\0';
 	beentry->st_progress_command = PROGRESS_COMMAND_INVALID;
 	beentry->st_progress_command_target = InvalidOid;
+
 	/*
 	 * we don't zero st_progress_param here to save cycles; nobody should
 	 * examine it until st_progress_command has been set to something other
@@ -2904,7 +2905,7 @@ pgstat_progress_update_param(int index, int64 val)
 /*---
  * pgstat_progress_end_command() -
  *
- * Update index'th member in st_progress_param[] of own backend entry.
+ * Clear command progress indicator of own backend entry.
  *---
  */
 void

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

Re: [HACKERS] Small patch for pgstat.c: fix comment + pgindent

[Amit Langote]

Hi,

Thanks for the report.

On 2016/03/10 18:05, Aleksander Alekseev wrote:
> Hello
> 
> I noticed:
> 
> http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b6fb6471f6afaf649e52f38269fd8c5c60647669
> 
> ... that comments for procedures pgstat_progress_update_param and
> pgstat_progress_end_command are identical. Here is a patch that fixes
> this. Also one empty line added after running pgindent on this file.

This was reported on an in-progress thread[1] and I sent a patch in reply:

Thanks,
Amit

[1]
http://www.postgresql.org/message-id/8f95982489b849c7ae662f20e908a...@mp-msgss-mbx007.msg.nttdata.co.jp




-- 
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] Improving replay of XLOG_BTREE_VACUUM records

[Michael Paquier]

On Thu, Mar 10, 2016 at 10:00 AM, Vladimir Borodin  wrote:
> Let’s do immediately after you will send a new version of your patch? Or
> even better after testing your patch? Don’t get me wrong, but rejecting my
> patch without tangible work on your patch may lead to forgiving about the
> problem before 9.6 freeze.

This makes sense. Let's not reject this patch yet if the alternative
approach is not committed.
-- 
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] Explain [Analyze] produces parallel scan for select Into table statements.

[Amit Kapila]

On Wed, Mar 9, 2016 at 8:18 PM, Mithun Cy 
wrote:
>
> Hi All,
>
> Explain [Analyze] Select Into table. produces the plan which uses
parallel scans.
>
> Possible Fix:
>
> I tried to make a patch to fix this. Now in ExplainOneQuery if into
clause is
>
> defined then parallel plans are disabled as similar to their execution.
>


- /* plan the query */

- plan = pg_plan_query(query, CURSOR_OPT_PARALLEL_OK, params);

+ /*

+ * plan the query.

+ * Note: If Explain is for CreateTableAs / SelectInto Avoid parallel

+ *   plans.

+ */

+ plan = pg_plan_query(query, into ? 0:CURSOR_OPT_PARALLEL_OK, params);


There should be a white space between 0:CURSOR_OPT_PARALLEL_OK.  Also I
don't see this comment is required as similar other usage doesn't have any
such comment.  Fixed these two points in the attached patch.

In general, the patch looks good to me and solves the problem mentioned.  I
have ran the regression tests with force_parallel_mode and doesn't see any
problem.


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


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

[Fujii Masao]

On Fri, Mar 4, 2016 at 3:40 AM, Masahiko Sawada  wrote:
> On Thu, Mar 3, 2016 at 11:30 PM, Masahiko Sawada  
> wrote:
>> Hi,
>>
>> Thank you so much for reviewing this patch!
>>
>> All review comments regarding document and comment are fixed.
>> Attached latest v14 patch.
>>
>>> This accepts 'abc^Id' as a name, which is wrong behavior (but
>>> such appliction names are not allowed anyway. If you assume so,
>>> I'd like to see a comment for that.).
>>
>> 'abc^Id' is accepted as application_name, no?
>> postgres(1)=# set application_name to 'abc^Id';
>> SET
>> postgres(1)=# show application_name ;
>>  application_name
>> --
>>  abc^Id
>> (1 row)
>>
>>> addlit_xd_string(char *ytext) and addlitchar_xd_string(unsigned
>>> char ychar) requires differnt character types. Is there any reason
>>> for that?
>>
>> Because addlit_xd_string() is for adding string(char *) to xd_string,
>> OTOH addlit_xd_char() is for adding just one character to xd_string.
>>
>>> I personally don't like addlit*string() things for such simple
>>> syntax but itself is acceptble enough for me. However it uses
>>> StringInfo to hold double-quoted names, which pallocs 1024 bytes
>>> of memory chunk for every double-quoted name. The chunks are
>>> finally stacked up left uncollected until the current
>>> memorycontext is deleted or reset (It is deleted just after
>>> finishing config file processing). Addition to that, setting
>>> s_s_names runs the parser twice. It seems to me too greedy and
>>> seems that static char [NAMEDATALEN] is enough using the v12 way
>>> without palloc/repalloc.
>>
>> I though that length of group name could be more than NAMEDATALEN, so
>> I use StringInfo.
>> Is it not necessary?
>>
>>> I found that the name SyncGroupName.wait_num is not
>>> instinctive. How about sync_num, sync_member_num or
>>> sync_standby_num? If the last is preferable, .members also should
>>> be .standbys .
>>
>> Thanks, sync_num is preferable to me.
>>
>> ===
>>> I am quite uncomfortable with the existence of
>>> WanSnd.sync_standby_priority. It represented the pirority in the
>>> old linear s_s_names format but nested groups or even
>>> single-level quarum list obviously doesn't fit it. Can we get rid
>>> of sync_standby_priority, even though we realize atmost
>>> n-priority for now?
>>
>> We could get rid of sync_standby_priority.
>> But if so, we will not be able to see the next sync standby in
>> pg_stat_replication system view.
>> Regarding each node priority, I was thinking that standbys in quorum
>> list have same priority, and in nested group each standbys are given
>> the priority starting from 1.
>>
>> ===
>>> The function SyncRepGetSyncedLsnsUsingPriority doesn't seem to
>>> have specific code for every prioritizing method (which are
>>> priority, quorum, nested and so). Is there any reson to use it as
>>> a callback of SyncGroupNode?
>>
>> The reason why the current code is so is that current code is for only
>> priority method supporting.
>> At first version of this feature, I'd like to implement it more simple.
>>
>> Aside from this, of course I'm planning to have specific code for nested 
>> design.
>> - The group can have some name nodes or group nodes.
>> - The group can use either 2 types of method: priority or quorum.
>> - The group has SyncRepGetSyncedLsnFn() and SyncRepGetStandbysFn()
>>   - SyncRepGetSyncedLsnsFn() function recursively determine synced LSN
>> at that moment using group's method.
>>   - SyncRepGetStandbysFn() function returns standbys of its group,
>> which are considered as sync using group's method.
>>
>> For example, s_s_name  = '3(a, b, 2[c,d]::group1)', SyncRepStandbys
>> memory structure will be,
>>
>> "main(quorum)" --- "a"
>> |
>> -- "b"
>> |
>> -- "group1(priority)" --- "c"
>>  |
>>  -- "d"
>>
>> When determine synced LSNs, we need to consider group1's LSN using by
>> priority method at first, and then we can determine main's LSN using
>> by quorum method with "a" LSNs, "b" LSNs and "group1" LSNs.
>> So SyncRepGetSyncedLsnsUsingPriority() function would be,
>>
>> bool
>> SyncRepGetSyncedLsnsUsingPriority(*group, *write_lsn, *flush_lsn)
>> {
>> sync_num = group->SynRepGetSyncstandbysFn(group, sync_list);
>>
>> if (sync_num < group->sync_num)
>> return false;
>>
>> for (each member of sync_list)
>> {
>> if (member->type == group node)
>> call SyncRepGetSyncedLsnsFn(member, w, f) and store w and
>> f into lsn_list.
>> else
>> Store name node LSNs into lsn_list.
>> }
>>
>> Determine synced LSNs of this group using lsn_list and priority method.
>> Store synced LSNs into write_lsn and flush_lsn.
>> return true;
>> }
>>
>>> SyncRepClearStandbyGroupList is defined in syncrep.c but the
>>> other related functions a

Re: [HACKERS] Optimizer questions

[konstantin knizhnik]

On Mar 10, 2016, at 1:56 AM, Tom Lane wrote:

> Konstantin Knizhnik  writes:
>> I think that the best approach is to generate two different paths: 
>> original one, when projection is always done before sort and another one 
>> with postponed projection of non-trivial columns. Then we compare costs 
>> of two paths and choose the best one.
>> Unfortunately, I do not understand now how to implement it with existed 
>> grouping_planner.
>> Do you think that it is possible?
> 
> After fooling with this awhile, I don't think it's actually necessary
> to do that.  See attached proof-of-concept patch.

O, you did all my work:)

But right now the rule for cost estimation makes it not possible to apply this 
optimization for simple expressions like this:

postgres=# create table a (b integer);
CREATE TABLE
postgres=# insert into a values (generate_series(1, 10));
INSERT 0 10
postgres=# select b+b from a order by b limit 1;
NOTICE:  int4pl
NOTICE:  int4pl
NOTICE:  int4pl
NOTICE:  int4pl
NOTICE:  int4pl
NOTICE:  int4pl
NOTICE:  int4pl
NOTICE:  int4pl
NOTICE:  int4pl
NOTICE:  int4pl
 ?column? 
--
2
(1 row)
postgres=# create or replace function twice(x integer) returns integer as $$ 
begin raise notice 'exec function' ; return x + x ; end $$ language plpgsql;
CREATE FUNCTION
postgres=# select twice(b) from a order by b limit 1;   
NOTICE:  exec function
NOTICE:  int4pl
 twice 
---
 2
(1 row)


I wonder is there any advantages of earlier evaluation of such simple 
expressions if them are not needed for sort?
It seems to be only true for narrowing functions, like md5... But I think that 
it is quite exotic case, isn't it?
May be it is reasonable to be more optimistic in estimation cost of postponed 
expression evaluation?


Also I do not completely understand your concern about windows functions.
Is there any example of query with windows or aggregate functions when this 
optimization (postponing expression evaluation) can be applied?
It will be also interesting to me to know if there are some other cases (except 
SORT+LIMIT) when delaying projection leeds to more efficient plan.



> 
> Although this patch gets through our regression tests, that's only because
> it's conservative about deciding to postpone evaluation; if it tried to
> postpone evaluation in a query with window functions, it would fail
> miserably, because pull_var_clause doesn't know about window functions.
> I think that that's a clear oversight and we should extend it to offer
> the same sorts of behaviors as it does for Aggrefs.  But that would be
> slightly invasive, there being a dozen or so callers; so I didn't bother
> to do it yet pending comments on this patch.
> 
> I think it's probably also broken for SRFs in the tlist; we need to work
> out what semantics we want for those.  If we postpone any SRF to after
> the Sort, we can no longer assume that a query LIMIT enables use of
> bounded sort (because the SRF might repeatedly return zero rows).
> I don't have a huge problem with that, but I think now would be a good
> time to nail down some semantics.
> 
> Some other work that would be useful would be to refactor so that the
> cost_qual_eval operations aren't so redundant.  But that's just a small
> time savings not a question of functionality.
> 
> And we'd have to document that this changes the behavior for volatile
> functions.  For the better, I think: this will mean that you get
> consistent results no matter whether the query is implemented by
> indexscan or seqscan-and-sort, which has never been true before.
> But it is a change.
> 
> Do people approve of this sort of change in general, or this patch
> approach in particular?  Want to bikeshed the specific
> when-to-postpone-eval policies implemented here?  Other comments?
> 
>   regards, tom lane
> 
> diff --git a/src/backend/optimizer/plan/planner.c 
> b/src/backend/optimizer/plan/planner.c
> index 8937e71..b15fca1 100644
> *** a/src/backend/optimizer/plan/planner.c
> --- b/src/backend/optimizer/plan/planner.c
> *** static RelOptInfo *create_distinct_paths
> *** 126,131 
> --- 126,132 
> RelOptInfo *input_rel);
>  static RelOptInfo *create_ordered_paths(PlannerInfo *root,
>RelOptInfo *input_rel,
> +  PathTarget *target,
>double limit_tuples);
>  static PathTarget *make_group_input_target(PlannerInfo *root, List *tlist);
>  static List *postprocess_setop_tlist(List *new_tlist, List *orig_tlist);
> *** static PathTarget *make_window_input_tar
> *** 134,139 
> --- 135,142 
>List *tlist, List 
> *activeWindows);
>  static List *make_pathkeys_for_window(PlannerInfo *root, WindowClause *wc,
>List *tlist);
> + static PathTarget *make_sor

Re: [HACKERS] create opclass documentation outdated

[Emre Hasegeli]

>> In create_opclass.sgml, not only do we have the list of AMs supporting
>> STORAGE, but there is also a paragraph describing which AMs do what
>> for input datatypes of FUNCTION members (around line 175).
>
> I placed BRIN together with gist/gin/spgist here, namely that the optype
> defaults to the opclass' datatype.

Requiring STORAGE type for BRIN opclasses is more of a bug than a
feature.  None of the operator classes actually support storing values
with different type.  We had intended to support it for inclusion
opclass to index points by casting them to box, but then ripped it out
because of inconsistencies on the operators caused by floating point
arithmetics.

The problem is that the operator classes try to query the strategies
using the datatype they got from TupleDesc structure.  It fails at
least for cidr, because it is casted implicitly and indexed as inet.
All of the BRIN operator classes can fail the same way for user
defined types.  Adding STORAGE to all operator classes have seemed to
me like an easy fix at the time I noticed this problem, but what we
really need to do is to fix this than document.  We should to make
BRIN use the datatype of the operator class as btree does.

> +   of each operator class interpret the strategy nnumbers according to the

Typo: nnumbers.

> +   Operator classes based on the Inclusion framework can
> +   theoretically support cross-data-type operations, but there's no
> +   demonstrating implementation yet.

This part of the inclusion class is not committed, so this paragraph
shouldn't be there.

Other than those, the changes look good to me.


-- 
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] Freeze avoidance of very large table.

[Robert Haas]

On Wed, Mar 9, 2016 at 9:09 AM, Masahiko Sawada  wrote:
> * 001 patch : Incorporated the documentation suggestions and updated
> logic a little.

This 001 patch looks so little like what I was expecting that I
decided to start over from scratch.  The new version I wrote is
attached here.  I don't understand why your version tinkers with the
logic for setting the all-frozen bit; I thought that what I already
committed dealt with that already, and in any case, your version
doesn't even compile against latest sources.  Your version also leaves
the scan_all terminology intact even though it's not accurate any
more, and I am not very convinced that the updates to the
page-skipping logic are actually correct.  Please have a look over
this version and see what you think.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index a09ceb2..2f72633 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5984,12 +5984,15 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
   
   

-VACUUM performs a whole-table scan if the table's
+VACUUM performs an aggressive scan if the table's
 pg_class.relfrozenxid field has reached
-the age specified by this setting.  The default is 150 million
-transactions.  Although users can set this value anywhere from zero to
-two billions, VACUUM will silently limit the effective value
-to 95% of , so that a
+the age specified by this setting.  An aggressive scan differs from
+a regular VACUUM in that it visits every page that might
+contain unfrozen XIDs or MXIDs, not just those that might contain dead
+tuples.  The default is 150 million transactions.  Although users can
+set this value anywhere from zero to two billions, VACUUM
+will silently limit the effective value to 95% of
+, so that a
 periodical manual VACUUM has a chance to run before an
 anti-wraparound autovacuum is launched for the table. For more
 information see
@@ -6028,9 +6031,12 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
   
   

-VACUUM performs a whole-table scan if the table's
+VACUUM performs an aggressive scan if the table's
 pg_class.relminmxid field has reached
-the age specified by this setting.  The default is 150 million multixacts.
+the age specified by this setting.  An aggressive scan differs from
+a regular VACUUM in that it visits every page that might
+contain unfrozen XIDs or MXIDs, not just those that might contain dead
+tuples.  The default is 150 million multixacts.
 Although users can set this value anywhere from zero to two billions,
 VACUUM will silently limit the effective value to 95% of
 , so that a
diff --git a/doc/src/sgml/maintenance.sgml b/doc/src/sgml/maintenance.sgml
index 5204b34..d742ec9 100644
--- a/doc/src/sgml/maintenance.sgml
+++ b/doc/src/sgml/maintenance.sgml
@@ -438,22 +438,27 @@

 

-VACUUM normally skips pages that don't have any dead row
-versions, but those pages might still have row versions with old XID
-values.  To ensure all old row versions have been frozen, a
-scan of the whole table is needed.
- controls when
-VACUUM does that: a whole table sweep is forced if
-the table hasn't been fully scanned for vacuum_freeze_table_age
-minus vacuum_freeze_min_age transactions. Setting it to 0
-forces VACUUM to always scan all pages, effectively ignoring
-the visibility map.
+VACUUM uses the visibility map
+to determine which pages of a relation must be scanned.  Normally, it
+will skips pages that don't have any dead row versions even if those pages
+might still have row versions with old XID values.  Therefore, normal
+scans won't succeed in freezing every row version in the table.
+Periodically, VACUUM will perform an aggressive
+vacuum, skipping only those pages which contain neither dead rows nor
+any unfrozen XID or MXID values.
+
+controls when VACUUM does that: all-visible but not all-frozen
+pages are scanned if the number of transactions that have passed since the
+last such scan is greater than vacuum_freeze_table_age minus
+vacuum_freeze_min_age. Setting
+vacuum_freeze_table_age to 0 forces VACUUM to
+use this more aggressive strategy for all scans.

 

 The maximum time that a table can go unvacuumed is two billion
 transactions minus the vacuum_freeze_min_age value at
-the time VACUUM last scanned the whole table.  If it were to go
+the time of the last aggressive vacuum. If it were to go
 unvacuumed for longer than
 that, data loss could result.  To ensure that this does not happen,
 autovacuum is invoked on any table th

Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW

[Robert Haas]

On Wed, Mar 9, 2016 at 3:30 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Mar 9, 2016 at 1:12 PM, Tom Lane  wrote:
>>> I hadn't been paying any attention to this thread, but I wonder whether
>>> this entire approach isn't considerably inferior to what we can do now
>>> with the planner pathification patch.  To quote from the new docs:
>
>> Well, I guess I'm not quite seeing it.  What do you have in mind?
>> Just taking a guess here, you might be thinking that instead of
>> something like this...
>
>>   Update on public.ft2
>> ->  Foreign Update on public.ft2
>
>> We could boil it all the way down to this:
>
>> Foreign Update on public.ft2
>
> Exactly.  I'm not claiming that that would be particularly faster, but
> it would eliminate a whole bunch of seriously ugly stuff that's in
> this patch.

Like what?

>> But can you, really?  What if the UPDATE is targeting an inheritance
>> hierarchy containing some local tables and some remote tables?
>
> I don't really see why that couldn't be made to work.  You'd end up
> with ForeignUpdates on the remote tables and a ModifyTable handling
> the rest.

I don't get it.  I mean, what's the parent node going to be?  If it's
the ModifyTable, then the plan tree looks the same as what this
already does.  If not, then what?

Just to recap the history, this patch was rewritten, criticized by
Stephen and you and rewritten to match your feedback.  Then, both of
you ignored it for a long time while I and others but a lot of work
into it.  Now, we're up against the deadline for this release and you
don't like it again.  Well, OK.  If you want to rewrite it into some
form you think is better, I'm cool with that.  But it would be really
unfair if this slipped out of this release after so much work has been
put into making it match the design ideas that *you* put forward just
because, at the eleventh hour, you now have new ones.  Personally, I
think we should just commit the darned thing and you can rewrite it
later if you want.  If you want to rewrite it now instead, I can live
with that, too.  But let's figure out how we're going to move this
forward.

-- 
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] Is there a way around function search_path killing SQL function inlining?

[Robert Haas]

On Thu, Mar 10, 2016 at 3:21 AM, Regina Obe  wrote:
> When you back up the database, it would create a backup with this line:
>
> SET search_path = public, pg_catalog;
> --your create materialized view here
>
> When you restore even if your database has search_paths set, your 
> materialized view will not come back and will error out with:
>
> ERROR:  function _helper(box, box) does not exist
> LINE 2:  SELECT $1 && $2 AND _helper($1,$2) = 0;
>  ^
> HINT:  No function matches the given name and argument types. You might need 
> to add explicit type casts.
> QUERY:
>  SELECT $1 && $2 AND _helper($1,$2) = 0;

Hmm.  The meaning of funcs.inline depends on the search_path, not just
during dump restoration but all the time.  So anything uses it under a
different search_path setting than the normal one will have this kind
of problem; not just dump/restore.

I don't have a very good idea what to do about that.

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


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

Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.

[Kyotaro HORIGUCHI]

Hi,

At Thu, 10 Mar 2016 08:21:36 +,  wrote in 
<8e09c2fe530d4008aa0019e38c1d5...@mp-msgss-mbx007.msg.nttdata.co.jp>
> > > So maybe we can add datname as separate column in
> > pg_stat_progress_vacuum, I think it's not required only datid is sufficient.
> > > Any comment?
> > 
> > Why do you think showing the name may be unacceptable?  Wouldn't that
> > be a little more user-friendly?  Though maybe, we can follow the
> > pg_stat_activity style and have both instead, as you suggest.  Attached
> > updated version does that.
> +1
> I think reporting both (datid and datname) is more user-friendly.
> Thank you.

I don't like showing both oid and name and only "user friendry"
doesn't seem to justify adding redundant columns in-a-sense.

So, I have looked into system_views.sql and picked up what
catalogs/views shows objects in such way, that is, showing both
object id and its name.

Show by name: pg_policies, pg_rules, pg_tablespg_matviews,
  pg_indexes, pg_stats, pg_prepared_xacts, pg_seclabels,
  pg_stat(io)_*_tables/indexes.schemaname
  pg_stat_*_functions.schemaname

Show by oid : pg_locks, pg_user_mappings.umid

Both: pg_stat(io)_*_tables/indexes.relid/relname, indexrelid/indexname;
  pg_stat_activity.datid/datname, usesysid/usename
  pg_stat_activity.datid/datname, usesysid/usename
  pg_replication_slots.datoid/database
  pg_stat_database(_conflicts).datid/datname
  pg_stat_*_functions.funcid/funcname
  pg_user_mappings.srvid/srvname,umuser/usename

It's surprising to see this result for me. The nature of this
view is near to pg_stat* views so it is proper to show *both of
database and relation* in both of oid and name.

Thoughts?

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] psql completion for ids in multibyte string

[Kyotaro HORIGUCHI]

Hello,

At Mon, 7 Mar 2016 12:34:15 -0500, Robert Haas  wrote in 

> >> Fix pushed.
> >
> > Thank you for committing this. I can see only one commit for this
> > in the repository but I believe it is the fixed one.
> 
> It was OK in master, but the back-branches had problems.  See
> 369c0b09080812943a2efcebe91cf4b271bc4f86.

I understand that. Thank you for replying.

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] Improving replay of XLOG_BTREE_VACUUM records

[Simon Riggs]

On 10 March 2016 at 09:22, Michael Paquier 
wrote:

> On Thu, Mar 10, 2016 at 10:00 AM, Vladimir Borodin 
> wrote:
> > Let’s do immediately after you will send a new version of your patch? Or
> > even better after testing your patch? Don’t get me wrong, but rejecting
> my
> > patch without tangible work on your patch may lead to forgiving about the
> > problem before 9.6 freeze.
>
> This makes sense. Let's not reject this patch yet if the alternative
> approach is not committed.
>

I attach 2 patches.

avoid_pin_scan_always.v1.patch
Takes the approach that we generate the same WAL records as in 9.5, we just
choose not to do anything with that information. This is possible because
we don't care anymore whether it is toast or other relations. So it
effectively reverts parts of the earlier patch.
This could be easily back-patched more easily.

toast_recheck.v1.patch
Adds recheck code for toast access. I'm not certain this is necessary, but
here it is. No problems found with it.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


avoid_pin_scan_always.v1.patch
Description: Binary data


toast_recheck.v1.patch
Description: Binary data

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

Re: [HACKERS] Parallel Aggregate

[David Rowley]

On 9 March 2016 at 04:06, Robert Haas  wrote:
> On Mon, Mar 7, 2016 at 5:15 PM, David Rowley
>  wrote:
>> 3. Nothing in create_grouping_paths() looks at the force_parallel_mode
>> GUC. I had a quick look at this GUC and was a bit surprised to see 3
>> possible states, but no explanation of what they do, so I've not added
>> code which pays attention to this setting yet. I'd imagine this is
>> just a matter of skipping serial path generation when parallel is
>> possible when force_parallel_mode is FORCE_PARALLEL_ON. I've no idea
>> what FORCE_PARALLEL_REGRESS is for yet.
>
> The GUC is documented just like all the other GUCs are documented.
> Maybe that's not enough, but I don't think "no explanation of what
> they do" is accurate.  But I don't see why this patch should need to
> care about force_parallel_mode at all.  force_parallel_mode is about
> making queries that wouldn't choose to run in parallel do on their own
> do so anyway, whereas this patch is about making more queries able to
> do more work in parallel.

Hmm, it appears I only looked as far as the enum declaration, which I
expected to have something. Perhaps I'm just not used to looking up
the manual for things relating to code.

The one reason that I asked about force_parallel_mode was that I
assumed there was some buildfarm member running somewhere that
switches this on and runs the regression tests. I figured that if it
exists for other parallel features, then it probably should for this
too. Can you explain why you think this should be handled differently?

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

[Etsuro Fujita]

On 2016/03/10 19:51, Robert Haas wrote:

On Wed, Mar 9, 2016 at 3:30 PM, Tom Lane  wrote:

Robert Haas  writes:

Just taking a guess here, you might be thinking that instead of
something like this...



   Update on public.ft2
 ->  Foreign Update on public.ft2



We could boil it all the way down to this:



 Foreign Update on public.ft2


Exactly.  I'm not claiming that that would be particularly faster, but
it would eliminate a whole bunch of seriously ugly stuff that's in
this patch.



But can you, really?  What if the UPDATE is targeting an inheritance
hierarchy containing some local tables and some remote tables?


I don't really see why that couldn't be made to work.  You'd end up
with ForeignUpdates on the remote tables and a ModifyTable handling
the rest.


I don't get it.  I mean, what's the parent node going to be?  If it's
the ModifyTable, then the plan tree looks the same as what this
already does.  If not, then what?


I don't get it, either.  If the ForeignUpdates would be executed 
separately from the ModifyTable, how would the query's reported row 
count (ie, estate->es_processed) be handled?



Just to recap the history, this patch was rewritten, criticized by
Stephen and you and rewritten to match your feedback.  Then, both of
you ignored it for a long time while I and others but a lot of work
into it.  Now, we're up against the deadline for this release and you
don't like it again.  Well, OK.  If you want to rewrite it into some
form you think is better, I'm cool with that.  But it would be really
unfair if this slipped out of this release after so much work has been
put into making it match the design ideas that *you* put forward just
because, at the eleventh hour, you now have new ones.  Personally, I
think we should just commit the darned thing and you can rewrite it
later if you want.  If you want to rewrite it now instead, I can live
with that, too.  But let's figure out how we're going to move this
forward.


I agree with Robert.

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] Reduce lock levels others reloptions in ALTER TABLE

[Simon Riggs]

On 1 March 2016 at 19:00, Robert Haas  wrote:

> On Mon, Feb 29, 2016 at 12:58 PM, Fabrízio de Royes Mello
>  wrote:
> > Some time ago we added [1] the infrastructure to allow different lock
> levels
> > for relation options.
> >
> > So per discussion [2] the attached patch reduce lock levels down to
> > ShareUpdateExclusiveLock for:
> > - fastupdate
> > - fillfactor
> > - gin_pending_list_limit
> > - seq_page_cost
> > - random_page_cost
> > - n_distinct
> > - n_distinct_inherited
> > - buffering
>
> I am of the opinion that there needs to be comments or a README or
> some kind of documentation somewhere indicating the rationale for the
> lock levels we choose here.  Just changing it without analyzing why a
> certain level is sufficient for safety and no higher than necessary
> seems poor.  And the reasoning should be documented for future
> readers.
>

+1

The patch altered tests for fillfactor, so I extracted just that parameter
from the patch for commit, after adding comments and docs.

Fabrízio, you've been around long enough to know that patches need docs and
tests. And as Robert says, comments that show you've done the analysis to
show you know the patch is safe.

Some parts of this patch could be resubmitted in a later CF with some time
and attention spent on it, but it isn't in a good enough state for last CF.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Identifying a message in emit_log_hook.

[Simon Riggs]

On 25 February 2016 at 07:42, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:

> Hello,
>
> At Wed, 17 Feb 2016 09:13:01 +, Simon Riggs 
> wrote in <
> canp8+jlbge_ybxulgzxvce44oob8v0t93e5_inhvbde2pxk...@mail.gmail.com>
> > On 17 February 2016 at 08:34, Kyotaro HORIGUCHI <
> > horiguchi.kyot...@lab.ntt.co.jp> wrote:
> >
> > >
> > > > I'm guessing this would require making the pre-translated error text
> > > > available to plugins as well as translated form.
> > >
> > > If I understand you correctly, edata->messgage_id in my patch is
> > > just what offers the pre-translated error text to plugins.
> >
> >
> > OK, now I understand the patch, I am happy to apply it.
>
> Thank you very much. I have one concern about this patch.
>
> I have added an id only for .message in the patch but it
> theoretically can apply to all other message typs eventually
> given to gettext() in two macros EVALUATE_MESSAGE(_PLURAL). They
> are detail, detail_log, hint and context and the modification
> could be limited within the two macros by doing so but extra four
> *_id members are to be added to ErrorData. I doubt it is useful
> for the extra members.
>
> If you agree with this, it doesn't seem to me to need more
> modification. Is there anything else to do?


David,

Can you add this to the CF? It was submitted before deadline.
I presume you have access to do that?

Other people may want to see this before commit.

If you can't, I'll commit anyway, but if we have a system we should follow
it.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

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

[Simon Riggs]

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


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

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

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

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] WAL log only necessary part of 2PC GID

[Pavan Deolasee]

On Wed, Mar 9, 2016 at 7:56 PM, Petr Jelinek  wrote:

> Hi,
>
> I wonder why you define the gidlen as uint32 when it would fit into uint8
> which in the current TwoPhaseFileHeader struct should be win of  8 bytes on
> padding (on 64bit). I think that's something worth considering given that
> this patch aims to lower the size of the data.
>
>
Hi Petr,

That sounds like a good idea; I didn't think about that. I would like to
make it uint16 though just in case if we decide to increase GIDSIZE from
200 to something more than 256 (Postgres-XL does that already). That still
fits in the same aligned width, on both 32 as well as 64-bit machines. New
version attached.

Thanks,
Pavan

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


reduce_gid_wal_v3.patch
Description: Binary data

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

[HACKERS] Re: [HACKERS] Re: [HACKERS] Re: [HACKERS] Windows service is not starting so there’s message in log: FATAL: "could not create shared memory segment “Global/PostgreSQL.851401618”: Permission

[Amit Kapila]

On Wed, Mar 9, 2016 at 5:46 PM, Haribabu Kommi 
wrote:
> On Wed, Mar 9, 2016 at 10:06 PM, Amit Kapila 
wrote:
> > On Wed, Mar 9, 2016 at 11:46 AM, Haribabu Kommi <
kommi.harib...@gmail.com>
> > wrote:
> >>
> >>
> >> I tried replacing the random() with PostmaterRandom() for a test and it
> >> worked.
> >> This is generating different random values, so the issue is not
occurring.
> >>
> >> "Global/PostgreSQL.2115609797"
> >>
> >> I feel, we should add the the data directory path + the random number
to
> >> generate the name for dynamic shared memory, this can fix problem.
> >>
> >
> > As mentioned above, I think if we can investigate why this error is
> > generated, that will be helpful.  Currently the code ensures that if the
> > segment already exists, it should retry to create a segment with other
name
> > (refer dsm_impl_windows()), so the point of investigation is, why it is
not
> > going via that path?  I am guessing due to some reason
CreateFileMapping()
> > is returning NULL in this case whereas ideally it should return the
existing
> > handle with an error ERROR_ALREADY_EXISTS.
>
> DEBUG:  mapped win32 error code 5 to 13
>
> Yes, the CreateFileMapping() is returning NULL with an error of
> ERROR_ACCESS_DENIED.
>

Okay, so one probable theory for such an error could be that when there is
already an object with same name exists, this API requests access to the
that existing object and found that it can't access it due to some reason.
On googling, I found some people suggesting to try by disabling UAC [1] on
your m/c, can you once try that to see what is the result (this experiment
is just to find out the actual reason of failure, rather than a permanent
change suggestion).


>
> I am not able to find the reason for this error. This error is occurring
only
> when the PostgreSQL is started as a service only.
>

Did you use pg_ctl register/unregister to register different services.  Can
you share the detail steps and OS version on which you saw this behaviour?

[1] -
http://windows.microsoft.com/en-in/windows/turn-user-account-control-on-off#1TC=windows-7

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

Re: [HACKERS] POC, WIP: OR-clause support for indexes

[Tomas Vondra]

Hi Teodor,


I've looked into v2 of the patch you sent a few days ago. Firstly, I
definitely agree that being able to use OR conditions with an index is
definitely a cool idea.

I do however agree with David that the patch would definitely benefit
from comments documenting various bits that are less obvious to mere
mortals like me, with limited knowledge of the index internals.

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

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

Now, some review comments from eyeballing the patch. Some of those are
nitpicking, but well ...

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

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

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

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

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

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

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

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

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

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

SET enable_indexscan = off;
EXPLAIN SELECT * FROM t WHERE (c && ARRAY[1] OR c && ARRAY[2]);

 QUERY PLAN
---
 Index Scan using t_c_idx on t  (cost=0.00..4.29 rows=0 width=33)
   Index Cond: ((c && '{1}'::integer[]) OR (c && '{2}'::integer[]))
(2 rows)

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

9) This already breaks estimation for some reason. Consider this
example, using a table with int[] column, with gist index built using
intarray:

EXPLAIN SELECT * FROM t WHERE (c && ARRAY[1,2,3,4,5,6,7]);

   QUERY PLAN

 Index Scan using t_c_idx on t  (cost=0.28..52.48 rows=12 width=33)
   Index Cond: (c && '{1,2,3,4,5,6,7}'::integer[])
(2 rows)

EXPLAIN SELECT * FROM t WHERE (c && ARRAY[8,9,10,11,12,13,14]);

   QUERY PLAN

 Index Scan using t_c_idx on t  (cost=0.28..44.45 rows=10 width=33)
   Index Cond: (c && '{8,9,10,11,12,13,14}'::integer[])
(2 rows)

EXPLAIN SELECT * FROM t WHERE (c && ARRAY[1,2,3,4,5,6,7])
   OR (c && ARRAY[8,9,10,11,12,13,14]);

   QUERY PLAN

 Index Scan using t_c_idx on t  (cost=0.00..4.37 rows=0 width=33)
   Index Cond: ((c && '{1,2,3,4,5,6,7}'::integer[])
 OR (c && '{8,9,10,11,12,13,14}'::integer[]))
(2 rows)

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

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

So I think this way of building the index path from a BitmapOr path is
pretty much a dead-end.


regards

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



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

Re: [HACKERS] pam auth - add rhost item

[Grzegorz Sampolski]

Hi.
In attchment new patch with updated documentation and with small change
to coding style as you suggested.

Regards.
Grzegorz.

On 03/09/2016 08:30 AM, Haribabu Kommi wrote:
> On Tue, Mar 8, 2016 at 10:43 PM, Grzegorz Sampolski  > wrote:
>> Hi Hari.
>> To use pam modules you can use whatever backend authentication method
>> you want.
>>
>> This is example configuration:
>>
>> Install this library https://github.com/pam-pgsql/pam-pgsql
>> Create some example database , schema access and two tables:
>> pam_auth and pam_account with example defintion:
>>
>> pam_account:
>> db_user character varying(16) NOT NULL,
>> host character varying(255) NOT NULL
>>
>> pam_auth:
>> db_user character varying(16) NOT NULL,
>> password character varying(512) NOT NULL
>>
>> Sample /etc/pam_pgsql.conf:
>> connect = dbname= user= password=
>> auth_query = SELECT password FROM access.pam_auth WHERE db_user = %u
> LIMIT 1
>> acct_query = SELECT '0','0','' FROM access.pam_account WHERE db_user =
>> %u AND (host = %h OR %h LIKE host) ORDER BY host DESC LIMIT 1;
>> pw_type = crypt
> 
> Thanks for the details. I am able to test the host limitation based on
> the host from where the connection request is given.This patch
> provides the advantage of getting the connected host address 
> details for the PAM modules to provide/restrict the authentication.
> 
> A small change in the code, correct the following code from
> 
> +if (retval) {
> 
> to
> 
> if (retval)
> {
> 
> as per the code everywhere.
> 
> 
>> I will try to update documentation in regard to this chagnes, but please
>> take into account that my english isn't fluent so much. So if I'll do
>> some mistakes please correct me.
> 
> I am also not a good English speaker :), but we can try to provide to
> as good as possible, later community can help in correcting it if they find
> any problem/improvement.
> 
> Regards,
> Hari Babu
> Fujitsu Australia
diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 3b2935c..c43322d 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -1627,6 +1627,7 @@ host ... ldap ldapurl="ldap://ldap.example.net/dc=example,dc=net?uid?sub";

 The following configuration options are supported for PAM:
 
+ 
  
   pamservice
   
@@ -1635,6 +1636,19 @@ host ... ldap ldapurl="ldap://ldap.example.net/dc=example,dc=net?uid?sub";

   
  
+ 
+ 
+  pamusedns
+  
+   
+   When not set (which is default), then ip address of connected host
+   will be passed to pam modules through PAM_RHOST item.
+   Otherwise it will be an attempt to determine host's name which can lead
+   to login delays.
+   
+  
+ 
+
 

 
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index cdc5bf1..af0d641 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -1735,6 +1735,21 @@ CheckPAMAuth(Port *port, char *user, char *password)
 {
 	int			retval;
 	pam_handle_t *pamh = NULL;
+	char hostinfo[NI_MAXHOST];
+
+	if (port->hba->pamusedns == true)
+		retval = pg_getnameinfo_all(&port->raddr.addr, port->raddr.salen,
+hostinfo, sizeof(hostinfo), NULL, 0, 0);
+	else
+		retval = pg_getnameinfo_all(&port->raddr.addr, port->raddr.salen,
+hostinfo, sizeof(hostinfo), NULL, 0, NI_NUMERICHOST);
+	if (retval)
+	{
+		ereport(LOG,
+(errmsg("(pam) couldn not determine the remote host information (%s)",
+	gai_strerror(retval;
+		return STATUS_ERROR;
+	}
 
 	/*
 	 * We can't entirely rely on PAM to pass through appdata --- it appears
@@ -1780,6 +1795,17 @@ CheckPAMAuth(Port *port, char *user, char *password)
 		return STATUS_ERROR;
 	}
 
+	retval = pam_set_item(pamh, PAM_RHOST, hostinfo);
+
+	if (retval != PAM_SUCCESS)
+	{
+		ereport(LOG,
+(errmsg("pam_set_item(PAM_RHOST) failed: %s",
+	pam_strerror(pamh, retval;
+		pam_passwd = NULL;
+		return STATUS_ERROR;
+	}
+
 	retval = pam_set_item(pamh, PAM_CONV, &pam_passw_conv);
 
 	if (retval != PAM_SUCCESS)
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 94f7cfa..db3fe3c 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -1447,6 +1447,15 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline, int line_num)
 		REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
 		hbaline->pamservice = pstrdup(val);
 	}
+	else if (strcmp(name, "pamusedns") == 0)
+	{
+		REQUIRE_AUTH_OPTION(uaPAM, "pamusedns", "pam");
+		if (strcmp(val, "1") == 0)
+			hbaline->pamusedns = true;
+		else
+			hbaline->pamusedns = false;
+
+	}
 	else if (strcmp(name, "ldapurl") == 0)
 	{
 #ifdef LDAP_API_FEATURE_X_OPENLDAP
diff --git a/src/include/libpq/hba.h b/src/include/libpq/hba.h
index 68a953a..f39240d 100644
--- a/src/include/libpq/hba.h
+++ b/src/include/libpq/hba.h
@@ -64,6 +64,7 @@ typedef struct HbaLine
 
 	char	   *usermap;
 	char	   *pamservice;
+	bool		pamusedns;
 	bool		ldaptls;
 	char	   *ldapserver;
 

Re: [HACKERS] Crash with old Windows on new CPU

[Magnus Hagander]

On Wed, Mar 9, 2016 at 5:48 AM, Christian Ullrich 
wrote:

> * Peter Eisentraut wrote:
>
> On 2/12/16 11:24 AM, Christian Ullrich wrote:
>>
>
> Otherwise, it may be time to update the manual (15.6 Supported
>>> Platforms) where it says PostgreSQL "can be expected to work on these
>>> operating systems: [...] Windows (Win2000 SP4 and later), [...]".
>>> Perhaps we could add "except Windows before 7 SP1/2008R2 SP1 when
>>> running in x64 mode on Intel CPUs introduced after May 2013 (Haswell and
>>> later)"?
>>>
>>
> Wouldn't the fix be for users to upgrade their service packs?
>>
>
> Windows 2008 and 2008R2 are entirely different things: 2008 is the server
> sibling of Vista (internal version 6.0), 2008R2 is that of Windows 7 (6.1).
> There is no version of 2008 that supports AVX2.
>
>
Windows 2008 went out of mainstream support in January last year, but is on
extended support until 2020. Extended support means both paid support and
security support is still there (what you don't get is new hardware support
and generic non-security related updates). That means we're going to see
these versions around for a long time.  (And Vista is in extended support
until 2017).

And exactly the type of upgrade scenario outlined in the OP is only going
to be come more common as old hardware gets replaced. If we didn't patch
this, the reasonable thing would be to say we don't support Visual Studio
2013, rather than a specific version of Windows, I think.

Given the small and localized change of this (and hey it even goes inside a
function labeled hacks), I definitely think it's worth doing.

I've commited this one, including a reference to the MS bug report where
they say they're not going to fix it in existing versions. I didn't
actually test it on mingw, but as it doesn't define MSC_VER it shouldn't be
affected (by the bug or by the patch).

I did notice the #ifdef's are actually different in the header and body
section of the patch, which seems wrong. I used the one from the actual
implementation (_M_AMD64) for the header includes as, and also merged the
#ifdef's together to a single #if in each section. Please verify!

Thanks for a very good analysis and patch, and for good explanations of the
details! :)

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

Re: [HACKERS] Identifying a message in emit_log_hook.

[David Steele]

Hi Simon,

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


Can you add this to the CF? It was submitted before deadline.
I presume you have access to do that?


No problem - here it is:

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

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


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

[HACKERS] Small patch: fix warnings during compilation on FreeBSD

[Aleksander Alekseev]

Hello

I noticed that master branch of PostgreSQL currently compiles with
warnings on FreeBSD 10.2 RELEASE:

```
pg_locale.c:1290:12: warning: implicit declaration of function
'wcstombs_l' is invalid in C99 [-Wimplicit-function-declaration]

result = wcstombs_l(to, from, tolen, locale);

pg_locale.c:1365:13: warning: implicit declaration of function
'mbstowcs_l' is invalid in C99 [-Wimplicit-function-declaration]

result = mbstowcs_l(to, str, tolen, locale);

2 warnings generated.
```

The problem is that both procedures are declared in xlocale.h file
which is not included in given context. If I remember correctly in
this case compiler assumes that procedures return `int` (instead of
size_t) so I doubt we can ignore this issue.

Frankly I'm not sure what is a right way of fixing this. My best guess
is attached to this message. I tested this patch on Ubuntu Linux 14.04
and FreeBSD 10.2. It solves a described problem and apparently doesn't
break anything (code compiles, regression tests pass, etc).

Best regards,
Aleksanderdiff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h
index 2e6dba1..2d2146c 100644
--- a/src/include/utils/pg_locale.h
+++ b/src/include/utils/pg_locale.h
@@ -15,7 +15,15 @@
 #include 
 #ifdef LOCALE_T_IN_XLOCALE
 #include 
-#endif
+#else			/* LOCALE_T_IN_XLOCALE */
+#ifdef HAVE_MBSTOWCS_L
+#include 
+#else
+#ifdef HAVE_WCSTOMBS_L
+#include 
+#endif   /* HAVE_WCSTOMBS_L */
+#endif   /* HAVE_MBSTOWCS_L */
+#endif   /* LOCALE_T_IN_XLOCALE */
 
 #include "utils/guc.h"
 

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

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

[Tomas Vondra]

Hi,

On Mon, 2015-12-28 at 15:03 -0800, Peter Geoghegan wrote:
> On Fri, Dec 18, 2015 at 11:57 AM, Peter Geoghegan  wrote:
> > BTW, I'm not necessarily determined to make the new special-purpose
> > allocator work exactly as proposed. It seemed useful to prioritize
> > simplicity, and currently so there is one big "huge palloc()" with
> > which we blow our memory budget, and that's it. However, I could
> > probably be more clever about "freeing ranges" initially preserved for
> > a now-exhausted tape. That kind of thing.
> 
> Attached is a revision that significantly overhauls the memory patch,
> with several smaller changes.

I was thinking about running some benchmarks on this patch, but the
thread is pretty huge so I want to make sure I'm not missing something
and this is indeed the most recent version.

Is that the case?

regards

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



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

Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.

[Robert Haas]

On Thu, Mar 10, 2016 at 6:10 AM, Kyotaro HORIGUCHI
 wrote:
> So, I have looked into system_views.sql and picked up what
> catalogs/views shows objects in such way, that is, showing both
> object id and its name.
>
> Show by name: pg_policies, pg_rules, pg_tablespg_matviews,
>   pg_indexes, pg_stats, pg_prepared_xacts, pg_seclabels,
>   pg_stat(io)_*_tables/indexes.schemaname
>   pg_stat_*_functions.schemaname
>
> Show by oid : pg_locks, pg_user_mappings.umid
>
> Both: pg_stat(io)_*_tables/indexes.relid/relname, 
> indexrelid/indexname;
>   pg_stat_activity.datid/datname, usesysid/usename
>   pg_stat_activity.datid/datname, usesysid/usename
>   pg_replication_slots.datoid/database
>   pg_stat_database(_conflicts).datid/datname
>   pg_stat_*_functions.funcid/funcname
>   pg_user_mappings.srvid/srvname,umuser/usename
>
> It's surprising to see this result for me. The nature of this
> view is near to pg_stat* views so it is proper to show *both of
> database and relation* in both of oid and name.
>
> Thoughts?

I think the problem is that you can't show the name of a non-global
SQL object (such as a relation) unless the object is in the current
database.  Many of the views in the first group are database-local
views, while things like pg_locks span all databases.  We can show the
datid/relid always, but if we have a relname column it will have to be
NULL unless the datid is our database.

-- 
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] Freeze avoidance of very large table.

[Masahiko Sawada]

On Thu, Mar 10, 2016 at 3:27 PM, Masahiko Sawada  wrote:
> Thank you for reviewing!
> Attached updated patch.
>
>
> On Thu, Mar 10, 2016 at 3:37 AM, Robert Haas  wrote:
>> On Wed, Mar 9, 2016 at 9:09 AM, Masahiko Sawada
>>  wrote: Attached latest 2 patches.
>>> * 000 patch : Incorporated the review comments and made rewriting
>>> logic more clearly.
>>
>> That's better, thanks.  But your comments don't survive pgindent.
>> After running pgindent, I get this:
>>
>> +   /*
>> +* These old_* variables point to old visibility map page.
>> +*
>> +* cur_old: Points to current position on old
>> page. blkend_old :
>> +* Points to end of old block. break_old  : Points to
>> old page break
>> +* position for rewriting a new page. After wrote a
>> new page, old_end
>> +* proceeds rewriteVmBytesPerPgae bytes.
>> +*/
>>
>> You need to either surround this sort of thing with dashes to make
>> pgindent ignore it, or, probably better, rewrite it using complete
>> sentences that together form a paragraph.
>
> Fixed.
>
>>
>> +   Oid pg_database_oid;/* OID of
>> pg_database relation */
>>
>> Not used anywhere?
>
> Fixed.
>
>> Instead of vm_need_rewrite, how about vm_must_add_frozenbit?
>
> Fixed.
>
>> Can you explain the changes to test.sh?
>
> Current regression test scenario is,
> 1. Do 'make check' on pre-upgrade cluster
> 2. Dump relallvisible values of all relation in pre-upgrade cluster to
> vm_test1.txt
> 3. Do pg_upgrade
> 4. Do analyze (not vacuum), dump relallvisibile values of all relation
> in post-upgrade cluster to vm_test2.txt
> 5. Compare between vm_test1.txt and vm_test2.txt
>
> That is, regression test compares between relallvisible values in
> pre-upgrade cluster and post-upgrade cluster.
> But because test.sh always uses pre/post clusters with same catalog
> version, I realized that we cannot ensure that visibility map
> rewriting is processed successfully on test.sh framework.
> Rewriting visibility map never be executed.
> We might need to have another framework for rewriting visibility map page..
>

After some further thought, I thought that it's better to add check
logic for result of rewriting visibility map to upgrading logic rather
than regression test in order to ensure that rewriting visibility map
has been successfully done.
As a draft, attached patch checks the result of rewriting visibility
map after rewrote for each relation as a routine of pg_upgrade.
The disadvantage point of this is that we need to scan each visibility
map page for 2 times.
But since visibility map size would not be so large, it would not bad.
Thoughts?

Regards,


-- 
Regards,

--
Masahiko Sawada
diff --git a/src/bin/pg_upgrade/file.c b/src/bin/pg_upgrade/file.c
index 2a99a28..6fd1460 100644
--- a/src/bin/pg_upgrade/file.c
+++ b/src/bin/pg_upgrade/file.c
@@ -9,10 +9,15 @@
 
 #include "postgres_fe.h"
 
+#include "access/visibilitymap.h"
 #include "pg_upgrade.h"
+#include "storage/bufpage.h"
+#include "storage/checksum.h"
+#include "storage/checksum_impl.h"
 
 #include 
 
+#define BITS_PER_HEAPBLOCK_OLD 1
 
 
 #ifndef WIN32
@@ -21,6 +26,7 @@ static int	copy_file(const char *fromfile, const char *tofile, bool force);
 static int	win32_pghardlink(const char *src, const char *dst);
 #endif
 
+static bool checkRewriteVisibilityMap(const char *oldfile, const char *newfile);
 
 /*
  * copyFile()
@@ -138,6 +144,235 @@ copy_file(const char *srcfile, const char *dstfile, bool force)
 #endif
 
 
+/*
+ * rewriteVisibilityMap()
+ *
+ * In versions of PostgreSQL prior to catversion 201603011, PostgreSQL's
+ * visibility map included one bit per heap page; it now includes two.
+ * When upgrading a cluster from before that time to a current PostgreSQL
+ * version, we could refuse to copy visibility maps from the old cluster
+ * to the new cluster; the next VACUUM would recreate them, but at the
+ * price of scanning the entire table.  So, instead, we rewrite the old
+ * visibility maps in the new format.  That way, the all-visible bit
+ * remains set for the pages for which it was set previously.  The
+ * all-frozen bit is never set by this conversion; we leave that to
+ * VACUUM.
+ */
+const char *
+rewriteVisibilityMap(const char *fromfile, const char *tofile, bool force)
+{
+	int			src_fd = 0;
+	int			dst_fd = 0;
+	char		buffer[BLCKSZ];
+	ssize_t 	bytesRead;
+	int			rewriteVmBytesPerPage;
+	BlockNumber	blkno = 0;
+
+	/* Compute we need how many old page bytes to rewrite a new page */
+	rewriteVmBytesPerPage = (BLCKSZ - SizeOfPageHeaderData) / 2;
+
+	if ((fromfile == NULL) || (tofile == NULL))
+		return "Invalid old file or new file";
+
+	if ((src_fd = open(fromfile, O_RDONLY, 0)) < 0)
+		return getErrorText();
+
+	if ((dst_fd = open(tofile, O_RDWR | O_CREAT | (force ? 0 : O_EXCL), S_IRUSR | S_IWUSR)) < 0)
+	{
+		close(src_fd);
+		return getErrorText();
+	}
+
+	/*
+	 * Turn 

Re: [HACKERS] Crash with old Windows on new CPU

[Christian Ullrich]

* Magnus Hagander wrote:


I did notice the #ifdef's are actually different in the header and body
section of the patch, which seems wrong. I used the one from the actual
implementation (_M_AMD64) for the header includes as, and also merged the
#ifdef's together to a single #if in each section. Please verify!


Should be fine. I remember I had a reason for doing it this way, but it 
can't have been a very good one. Thanks for cleaning it up.


--
Christian



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

Re: [HACKERS] Parallel Aggregate

[Robert Haas]

On Thu, Mar 10, 2016 at 6:42 AM, David Rowley
 wrote:
> Hmm, it appears I only looked as far as the enum declaration, which I
> expected to have something. Perhaps I'm just not used to looking up
> the manual for things relating to code.

I don't mind adding some comments there, it just didn't seem all that
important to me.  Feel free to propose something.

> The one reason that I asked about force_parallel_mode was that I
> assumed there was some buildfarm member running somewhere that
> switches this on and runs the regression tests. I figured that if it
> exists for other parallel features, then it probably should for this
> too. Can you explain why you think this should be handled differently?

Yeah, I think Noah set up such a buildfarm member, but I can't
remember the name of it off-hand.

I think running the tests with this patch and
force_parallel_mode=regress, max_parallel_degree>0 is a good idea, but
I don't expect it to turn up too much.  That configuration is mostly
designed to test whether the basic parallelism infrastructure works or
breaks things.  It's not intended to test whether your parallel query
plans are any good - you have to write your own tests for that.

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


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

[HACKERS] Endless loop calling PL/Python set returning functions

[Alexey Grishchenko]

Hello

There is a bug in implementation of set-returning functions in PL/Python.
When you call the same set-returning function twice in a single query, the
executor falls to infinite loop which causes OOM. Here is a simple
reproduction for this issue:

CREATE OR REPLACE FUNCTION func(iter int) RETURNS SETOF int AS $$
return xrange(iter)
$$ LANGUAGE plpythonu;

select func(3), func(4);


The endless loop is caused by the fact that PL/Python uses PLyProcedure
structure for each of the functions, containing information specific for
the function. This structure is used to store the result set iterator
returned by the Python function call. But in fact, when we call the same
function twice, PL/Python uses the same structure for both calls, and the
same result set iterator (PLyProcedure.setof), which is being constantly
updated by one function after another. When the iterator reaches the end,
the first function sets it to null. Then Postgres calls the second
function, it receives NULL iterator and calls Python function once again,
receiving another iterator. This is an endless loop

In fact, for set-returning functions in Postgres we should use a set
of SRF_* functions, which gives us an access to function call context
(FuncCallContext). In my implementation this context is used to store the
iterator for function result set, so these two calls would have separate
iterators and the query would succeed.

Another issue with calling the same set-returning function twice in the
same query, is that it would delete the input parameter of the function
from the global variables dictionary at the end of execution. With calling
the function twice, this code attempts to delete the same entry from global
variables dict twice, thus causing KeyError. This is why the
function PLy_function_delete_args is modified as well to check whether the
key we intend to delete is in the globals dictionary.

New regression test is included in the patch.

-- 
Best regards,
Alexey Grishchenko


0001-Fix-endless-loop-in-plpython-set-returning-function.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] [PROPOSAL] Client Log Output Filtering

[David Steele]

On 3/9/16 7:37 PM, Petr Jelinek wrote:

On 03/02/16 05:02, Robert Haas wrote:

On Mon, Feb 1, 2016 at 7:24 PM, David Steele  wrote:


I have attached a patch that adds an ereport() macro to suppress client
output for a single report call (applies cleanly on 1d0c3b3).  I'll also
move it to the next CF.


I don't see any reason not to accept this.



Yes, the idea seems sane.

Looking at the code, this adds bool hide_from_client to edata which is
not initialized in errstart so that needs to be fixed.


I figured this would take care of it:

MemSet(edata, 0, sizeof(ErrorData));

Since I want hide_from_client to default to false.  Am I missing something?

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


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

Re: [HACKERS] Parallel Aggregate

[Amit Langote]

On Thu, Mar 10, 2016 at 10:55 PM, Robert Haas  wrote:
> On Thu, Mar 10, 2016 at 6:42 AM, David Rowley
>  wrote:
>> The one reason that I asked about force_parallel_mode was that I
>> assumed there was some buildfarm member running somewhere that
>> switches this on and runs the regression tests. I figured that if it
>> exists for other parallel features, then it probably should for this
>> too. Can you explain why you think this should be handled differently?
>
> Yeah, I think Noah set up such a buildfarm member, but I can't
> remember the name of it off-hand.

mandrill [1]?

Thanks,
Amit

[1] http://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=mandrill&br=HEAD


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

Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.

[Robert Haas]

On Thu, Mar 10, 2016 at 3:08 AM, Amit Langote
 wrote:
> Hi Vinayak,
>
> Thanks for the quick review!

Committed 0001 earlier this morning.

On 0002:

+   /* total_index_blks */
+   current_index_blks = (BlockNumber *) palloc(nindexes *
sizeof(BlockNumber));
+   total_index_blks = 0;
+   for (i = 0; i < nindexes; i++)
+   {
+   BlockNumber nblocks =
RelationGetNumberOfBlocks(Irel[i]);
+
+   current_index_blks[i] = nblocks;
+   total_index_blks += nblocks;
+   }
+   pgstat_progress_update_param(PROG_PARAM_VAC_IDX_BLKS, total_index_blks);

I think this is a bad idea.  The value calculated here isn't
necessarily accurate, because the number of index blocks can change
between the time this is calculated and the time the indexes are
actually vacuumed.  If a client just wants the length of the indexes
in round figures, that's already SQL-visible, and there's little
reason to make VACUUM do it all the time whether anyone is looking at
the progress information or not.  Note that I'm not complaining about
the fact that you exposed the heap block count, because in that case
you are exposing the actual value that VACUUM is using to guide its
work.  The client can get the *current* length of the relation, but
the value you are exposing gives you the number of blocks *this
particular VACUUM intends to scan*.  That has some incremental value -
but the index information doesn't have the same thing going for it.

On 0003:

I think you should make this work for all AMs, not just btree, and
then consolidate it with 0002.

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


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

Re: [HACKERS] proposal: function parse_ident

[Teodor Sigaev]

select
parse_ident(E'X\rXX');


I am sending updated patch - I used json function for correct escaping - the
escaping behave is same.


Hmm, it doesn't look so:
% select parse_ident(E'_\005');
ERROR:  identifier contains disallowed characters: "_\u0005"
% select parse_ident(E'\005');
ERROR:  missing identifier: "\u0005"

but

# select parse_ident(E'"\005"');
 parse_ident
-
 {\x05}

Error messages above point wrong character wrongly.

One more inconsistence:
# select parse_ident(E'"\005"') as "\005";
  \005

 {\x05}

Display outputs of actual identifier and parse_indent are differ.

Actually, I can live with both but are any other opinions? Seems, at least 
difference of actual identifier and output of parse_indent should be pointed in 
docs.


--
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] utils/misc: Simplify search of end of argv in save_ps_display_args()

[Alvaro Herrera]

Alexander Kuleshov wrote:
> Hello,
> 
> Attached patch provides trivial simplification of the search of end of
> the argv[] area by replacing for() loop with calculation based on
> argc.

Uhm.  Doesn't this break the very same thing that the comment explains
it's doing?

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

[David Rowley]

On 8 March 2016 at 11:15, David Rowley  wrote:
> The setrefs.c parts of the patch remain completely broken. I've not
> had time to look at this again yet, sorry.

Ok, so again, apologies for previously sending such a broken patch.
I've since managed to free up a bit of time to work on this, which now
consists of a good bit more than the sum total of my weekly lunch
hours.

The attached patch is a much more complete patch, and quite possible
now review worthy.

I set about solving the setrefs.c problem by inventing a PartialAggref
node type which wraps up Aggrefs when they're in a agg node which has
finalizeAggs = false. This node type exists all the way until executor
init time, where it's then removed and replaced with the underlying
Aggref. This seems to solve the targetlist return type issue.
I'd really like some feedback on this method of solving that problem.

I've also fixed numerous other bugs, including the HAVING clause problem.

Things left to do:
1. Make a final pass of the patch not at 3am.
2. Write some tests.
3. I've probably missed a few places that should handle T_PartialAggref.

A couple of things which I'm not 100% happy with.

1. make_partialgroup_input_target() doing lookups to the syscache.
Perhaps this job can be offloaded to a new function in a more suitable
location. Ideally the Aggref would already store the required
information, but I don't see a great place to be looking that up.
2. I don't really like how I had to add tlist to
create_grouping_paths(), but I didn't want to go to the trouble of
calculating the partial agg PathTarget if Parallel Aggregation is not
possible, as this does do syscache lookups, so it's not free, so I'd
rather only do it if we're actually going to add some parallel paths.
3. Something about the force_parallel_mode GUC. I'll think about this
when I start to think about how to test this, as likely I'll need
that, else I'd have to create tables bigger than we'd want to in the
regression tests.

I've also attached an .sql file with an aggregate function aimed to
test the new PartialAggref stuff works properly, as previously it
seemed to work by accident with sum(int).

Just some numbers to maybe make this more interesting:

create table t (num int not null, one int not null, ten int not null,
hundred int not null, thousand int not null, tenk int not null,
hundredk int not null, million int not null);
insert into t select
x,1,x%10+1,x%100+1,x%1000+1,x%1+1,x%10+1,x%100 from
generate_series(1,1000)x(x);

-- Serial Plan
# explain select sum(num) from t;
QUERY PLAN
---
 Aggregate  (cost=198530.00..198530.01 rows=1 width=8)
   ->  Seq Scan on t  (cost=0.00..173530.00 rows=1000 width=4)

# select sum(num) from t;
  sum

 500500
(1 row)
Time: 1036.119 ms

# set max_parallel_degree=4;

-- Parallel Plan
# explain select sum(num) from t;
  QUERY PLAN
--
 Finalize Aggregate  (cost=105780.52..105780.53 rows=1 width=8)
   ->  Gather  (cost=105780.00..105780.51 rows=5 width=8)
 Number of Workers: 4
 ->  Partial Aggregate  (cost=104780.00..104780.01 rows=1 width=8)
   ->  Parallel Seq Scan on t  (cost=0.00..98530.00
rows=250 width=4)
(5 rows)

# select sum(num) from t;
  sum

 500500
(1 row)

Time: 379.117 ms

I'll try and throw a bit parallel aggregate work to a 4 socket / 64
core server which I have access to... just for fun.

Reviews are now welcome.

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


parallel_aggregation_cc3763e_2016-03-11.patch
Description: Binary data


parallel_aggregation_test.sql
Description: Binary data

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

Re: [HACKERS] Floating point timestamps

[Tom Lane]

Thomas Munro  writes:
> Is the plan to remove support for floating point timestamps at some
> stage?  If so, what is that waiting on, and would it provide
> sufficient warning if (say) 9.6 were documented as the last major
> release to support that build option?

AFAIK there is no particular plan to do that.  It's not like leaving
that code in place is costing us huge amounts of maintenance effort.

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

[Robert Haas]

On Thu, Mar 10, 2016 at 1:53 AM, Michael Paquier
 wrote:
> On Wed, Mar 9, 2016 at 12:13 AM, Alvaro Herrera
>  wrote:
>> Robert Haas wrote:
>>> I'm pretty meh about the whole idea of this function, though,
>>> actually, and I don't see a single clear +1 vote for this
>>> functionality upthread.  (Apologies if I've missed one.)  In the
>>> absence of a few of those, I recommend we reject this.
>>
>> +1
>
> I'm meh for this patch.

+1: David Steele, Vik Fearing, David Johnson, Alvaro Herrera
meh: Robert Haas, Michael Paquier
-1: Simon Riggs

That's probably marginally enough support to proceed with this, but
I'm somewhat unenthused about putting time into it myself.  Alvaro,
any chance you want to handle this one?

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


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

[HACKERS] 2PC support for pglogical

[Stas Kelvich]

Hi.

Here is proof-of-concept version of two phase commit support for logical 
replication. There are some changes in core postgres, pglogical_output and 
pglogical extensions. I’ve used version from 2nd Quadrant repo, pglogical 
branch and rebased it to current head. Some notes about this patch:

* LWLockAssign() was deleted, so I changed that to new locks tranche api.

* Seems that only reliable way to get GID during replay of commit/rollback 
prepared is to force postgres to write GID in corresponding records, otherwise 
we can lose correspondence between xid and gid  if we are replaying data from 
wal sender while transaction was commited some time ago. So i’ve changed 
postgres to write gid’s not only on prepare, but also on commit/rollback 
prepared. That should be done only in logical level, but now I just want to 
here some other opinions on that.

* Abort prepared xlog record also lack database information. Normally logical 
decoding just cleans reorder buffer when facing abort, but in case of 2PC we 
should send it to callbacks anyway. So I’ve added that info to abort records.

* Prepare emits xlog record with TwoPhaseFileHeader in it and that structure is 
the same as xl_xact_parsed_commit, but with some fields renamed. Probably that 
is just due to historical reasons. It is possible to change PREPARE to write 
ordinary commit records with some flag and then use the same infrastructure to 
parse it. So DecodePrepare/DecodeCommit can be done with the same function. 





pglogical_twophase.diff
Description: Binary data


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


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

Re: [HACKERS] Floating point timestamps

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Thomas Munro  writes:
> > Is the plan to remove support for floating point timestamps at some
> > stage?  If so, what is that waiting on, and would it provide
> > sufficient warning if (say) 9.6 were documented as the last major
> > release to support that build option?
> 
> AFAIK there is no particular plan to do that.  It's not like leaving
> that code in place is costing us huge amounts of maintenance effort.

Agreed, and I have little doubt that it's still used in the field given
how long it was the default for some distributions.

Thanks!

Stephen


signature.asc
Description: Digital signature

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

[Alvaro Herrera]

Robert Haas wrote:

> That's probably marginally enough support to proceed with this, but
> I'm somewhat unenthused about putting time into it myself.  Alvaro,
> any chance you want to handle this one?

OK, I can take it, but I have a few other things earlier in my queue so
it will be a few days.

-- 
Á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] [PROPOSAL] Client Log Output Filtering

[Tom Lane]

David Steele  writes:
> On 3/9/16 7:37 PM, Petr Jelinek wrote:
>> Looking at the code, this adds bool hide_from_client to edata which is
>> not initialized in errstart so that needs to be fixed.

> I figured this would take care of it:
> MemSet(edata, 0, sizeof(ErrorData));
> Since I want hide_from_client to default to false.  Am I missing something?

The patch is evidently modeled on errhidestmt and errhidectx, which are
making the same assumption for their fields.

I wonder whether, instead of continuing to proliferate random bool fields
in struct ErrorData, we oughta replace them all with an "int flags" field.
That's probably material for a separate patch though.

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] Move PinBuffer and UnpinBuffer to atomics

[Alexander Korotkov]

On Mon, Mar 7, 2016 at 6:19 PM, Robert Haas  wrote:

> On Sat, Mar 5, 2016 at 7:22 AM, Dilip Kumar  wrote:
> > On Wed, Mar 2, 2016 at 11:05 AM, Dilip Kumar 
> wrote:
> >> And this latest result (no regression) is on X86 but on my local
> machine.
> >>
> >> I did not exactly saw what this new version of patch is doing different,
> >> so I will test this version in other machines also and see the results.
> >
> >
> > I tested this on PPC again, This time in various order (sometime patch
> first
> > and then base first).
> >  I tested with latest patch pinunpin-cas-2.patch on Power8.
> >
> > Shared Buffer = 8GB
> > ./pgbench  -j$ -c$ -T300 -M prepared -S postgres
> >
> > BASE
> > -
> > Clientsrun1run2run3
> > 1   212001875420537
> > 2   403313952038746
> >
> >
> > Patch
> > -
> > Clientsrun1run2run3
> > 1   202251980619778
> > 2   398304189836620
> >
> > I think, here we can not see any regression, (If I take median then it
> may
> > looks low with patch so posting all 3 reading).
>
> If the median looks low, how is that not a regression?
>

I don't think we can rely on median that much if we have only 3 runs.
For 3 runs we can only apply Kornfeld method which claims that confidence
interval should be between lower and upper values.
Since confidence intervals for master and patched versions are overlapping
we can't conclude that expected TPS numbers are different.
Dilip, could you do more runs? 10, for example. Using such statistics we
would be able to conclude something.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] [PROPOSAL] Client Log Output Filtering

[Tom Lane]

David Steele  writes:
> I have attached a patch that adds an ereport() macro to suppress client
> output for a single report call (applies cleanly on 1d0c3b3).  I'll also
> move it to the next CF.

This patch fails to add the necessary documentation (see sources.sgml)

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] Floating point timestamps

[Alvaro Herrera]

Stephen Frost wrote:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
> > Thomas Munro  writes:
> > > Is the plan to remove support for floating point timestamps at some
> > > stage?  If so, what is that waiting on, and would it provide
> > > sufficient warning if (say) 9.6 were documented as the last major
> > > release to support that build option?
> > 
> > AFAIK there is no particular plan to do that.  It's not like leaving
> > that code in place is costing us huge amounts of maintenance effort.
> 
> Agreed, and I have little doubt that it's still used in the field given
> how long it was the default for some distributions.

... and the fact that you can pg_upgrade from old versions that had such
defaults, but only if the new install uses the same datetime
representation.  IOW it's quite likely that there are versions in the
field still working with FP datetimes.

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


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

Re: [HACKERS] Patch to implement pg_current_logfile() function

[Robert Haas]

On Wed, Mar 9, 2016 at 12:32 PM, Gilles Darold  wrote:
> I choose to allow the log collector to write his current log file name
> into the lock file 'postmaster.pid'. This allow simple access to this
> information through system commands, for example:
>
> postgres@W230ST:~$ tail -n1 /usr/local/pgql-devel/data/postmaster.pid
> pg_log/postgresql-2016-03-09_152908.log
>
> Log filename is written at the 8th line position when log collection
> is active and all other information have been written to lock file.
>
> The function pg_current_logfile() use in SQL mode read the lock file
> to report the information.
>
> I don't know if there's any limitation on using postmaster.pid file to
> do that but it seems to me a bit weird to log this information to an
> other file. My first attempt was to use a dedicated file and save it
> to global/pg_current_logfile or pg_stat_tmp/pg_current_logfile but I
> think it is better to use the postmaster.pid file for that.

Gosh, why?  Piggybacking this on a file written for a specific purpose
by a different process seems like making life very hard for yourself,
and almost certainly a recipe for bugs.

-- 
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] Small patch: fix warnings during compilation on FreeBSD

[Tom Lane]

Aleksander Alekseev  writes:
> I noticed that master branch of PostgreSQL currently compiles with
> warnings on FreeBSD 10.2 RELEASE:
> pg_locale.c:1290:12: warning: implicit declaration of function
> 'wcstombs_l' is invalid in C99 [-Wimplicit-function-declaration]

> The problem is that both procedures are declared in xlocale.h file
> which is not included in given context.

OK.

> Frankly I'm not sure what is a right way of fixing this.

Not like that, as it will break entirely on machines without xlocale.h
(which I presume is pretty nonstandard; it's certainly not mentioned
in the POSIX spec).  It will also break things on glibc, according to
this comment in our c-library.m4:

# Check for the locale_t type and find the right header file.  Mac OS
# X needs xlocale.h; standard is locale.h, but glibc also has an
# xlocale.h file that we should not use.

I think what we need is configure logic to find out where wcstombs_l()
is declared, and then #include  only if it's necessary to get
that definition.  I haven't experimented but probably you could make such
a check with nested uses of AC_CHECK_DECL.

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] Patch to implement pg_current_logfile() function

[Tom Lane]

Robert Haas  writes:
> On Wed, Mar 9, 2016 at 12:32 PM, Gilles Darold  
> wrote:
>> I choose to allow the log collector to write his current log file name
>> into the lock file 'postmaster.pid'.

> Gosh, why?  Piggybacking this on a file written for a specific purpose
> by a different process seems like making life very hard for yourself,
> and almost certainly a recipe for bugs.

That's a *complete* nonstarter.  postmaster.pid has to be written by the
postmaster process and nobody else.

It's a particularly bad choice for the syslogger, which will exist
fractionally longer than the postmaster, and thus might be trying to write
into the file after the postmaster has removed it.

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

[Simon Riggs]

On 10 March 2016 at 06:53, Michael Paquier 
wrote:

> On Wed, Mar 9, 2016 at 12:13 AM, Alvaro Herrera
>  wrote:
> > Robert Haas wrote:
> >> I'm pretty meh about the whole idea of this function, though,
> >> actually, and I don't see a single clear +1 vote for this
> >> functionality upthread.  (Apologies if I've missed one.)  In the
> >> absence of a few of those, I recommend we reject this.
> >
> > +1
>
> I'm meh for this patch.
>

"meh" == +1

I thought it meant -1

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

[Petr Jelinek]

On 10/03/16 08:08, Kouhei Kaigai wrote:
>>
 Also in RegisterCustomScanMethods
 +  Assert(strlen(methods->CustomName) <= CUSTOM_NAME_MAX_LEN);

 Shouldn't this be actually "if" with ereport() considering this is
 public API and extensions can pass anything there? (for that matter same
 is true for RegisterExtensibleNodeMethods but that's already committed).

>>> Hmm. I don't have clear answer which is better. The reason why I put
>>> Assert() here is that only c-binary extension uses this interface, thus,
>>> author will fix up the problem of too long name prior to its release.
>>> Of course, if-with-ereport() also informs extension author the name is
>>> too long.
>>> One downside of Assert() may be, it makes oversight if --enable-cassert
>>> was not specified.
>>>
>>
>> Well that's exactly my problem, this should IMHO throw error even
>> without --enable-cassert. It's not like it's some performance sensitive
>> API where if would be big problem, ensuring correctness of the input is
>> more imporant here IMHO.
>>
> We may need to fix up RegisterExtensibleNodeMethods() first.
> 
> Also, length limitation is (EXTNODENAME_MAX_LEN-1) because the last byte
> is consumed by '\0' character. In fact, hash, match and keycopy function
> of HTAB for string keys deal with the first (keysize - 1) bytes.
> So, strkey(extnodename) == EXTNODENAME_MAX_LEN is not legal.
> 

Yes, my thoughts as well but that can be separate tiny patch that does
not have to affect this one. In my opinion if we fixed this one it would
be otherwise ready to go in, and I definitely prefer this approach to
the previous one.

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


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

Re: [HACKERS] Endless loop calling PL/Python set returning functions

[Tom Lane]

Alexey Grishchenko  writes:
> There is a bug in implementation of set-returning functions in PL/Python.
> When you call the same set-returning function twice in a single query, the
> executor falls to infinite loop which causes OOM.

Ugh.

> Another issue with calling the same set-returning function twice in the
> same query, is that it would delete the input parameter of the function
> from the global variables dictionary at the end of execution. With calling
> the function twice, this code attempts to delete the same entry from global
> variables dict twice, thus causing KeyError. This is why the
> function PLy_function_delete_args is modified as well to check whether the
> key we intend to delete is in the globals dictionary.

That whole business with putting a function's parameters into a global
dictionary makes me itch.  Doesn't it mean problems if one plpython
function calls another (presumably via SPI)?

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

[Alvaro Herrera]

Simon Riggs wrote:
> On 10 March 2016 at 06:53, Michael Paquier 
> wrote:
> 
> > On Wed, Mar 9, 2016 at 12:13 AM, Alvaro Herrera
> >  wrote:
> > > Robert Haas wrote:
> > >> I'm pretty meh about the whole idea of this function, though,
> > >> actually, and I don't see a single clear +1 vote for this
> > >> functionality upthread.  (Apologies if I've missed one.)  In the
> > >> absence of a few of those, I recommend we reject this.
> > >
> > > +1
> >
> > I'm meh for this patch.
> 
> "meh" == +1
> 
> I thought it meant -1

I would say that "meh" is +0, actually.  So the the tally is a small
positive number -- not great, but seems enough to press forward unless
we get more -1 votes.

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

[Stephen Frost]

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Simon Riggs wrote:
> > On 10 March 2016 at 06:53, Michael Paquier 
> > wrote:
> > 
> > > On Wed, Mar 9, 2016 at 12:13 AM, Alvaro Herrera
> > >  wrote:
> > > > Robert Haas wrote:
> > > >> I'm pretty meh about the whole idea of this function, though,
> > > >> actually, and I don't see a single clear +1 vote for this
> > > >> functionality upthread.  (Apologies if I've missed one.)  In the
> > > >> absence of a few of those, I recommend we reject this.
> > > >
> > > > +1
> > >
> > > I'm meh for this patch.
> > 
> > "meh" == +1
> > 
> > I thought it meant -1
> 
> I would say that "meh" is +0, actually.  So the the tally is a small
> positive number -- not great, but seems enough to press forward unless
> we get more -1 votes.

I'm +1 on this also, for my part.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] New Windows animal, mostly ecpg trouble (was: Crash with old Windows on new CPU)

[Christian Ullrich]

* Alvaro Herrera wrote:


Magnus Hagander wrote:



On Wed, Mar 9, 2016 at 4:36 PM, Christian Ullrich 
wrote:



And apparently not a single one with VS 2013. OK, I'll see what I can do
about setting some up soonish, at least with (server) 2008 and (client) 7.
FWIW, I have a local build of 9.5.1 with this patch in daily use on 2008
now, with no complaints.


Having that added to the buildfarm would definitely be very useful!


Yeah, ideally we would have one buildfarm member for each MSVC compiler
supported by each Windows version.  AFAICS we only have

Windows 8 8.1 Pro Visual Studio 2012 x86_64
Windows Server 2008 R2 (64bit) Visual C++ 2005 AMD64
Windows Server 2003 R2 5.2.3790 MSVC 2005 Express 8.0.50727.42 x86
Vista Ultimate 6.0.6000 MSVC 2005 Pro 8.0.50727.867 x86
Windows XP-PRO SP3 MSVC++ 2008 Express i686


Add to that Windows 7 with Visual Studio 2013 (x64), named woodlouse, 
and there will be another one sometime over the weekend. Setting up 
Windows animals is a major pain; no wonder so few people run any.


I spent some hours getting rid of random linker errors in ecpg 
(kernel32.lib not found, msvcrt.lib not found, or unresolved symbols 
that should be in one of the two); they finally went away when I stopped 
defining the build environment in build-farm.conf and ran vcvarsall.bat 
instead (this was probably pilot error; I think I had a mix of x86 and 
x64 paths there). At that point, I managed one successful run on HEAD. 
Things started failing in ecpg again after that, with crashes of the 
test programs for the thread-thread and thread-prep test cases, 
apparently somewhere in setlocale(). I had to --skip-steps=ecpg-check 
because the crashes cause blocking UI.


The other troublemaker is plperl; it produces a wall of compiler errors 
which I cleverly did not copy, most are about something not being in a 
formal parameter list. Some preprocessor trouble, I suspect. This is 
with both ActiveState and Strawberry Perl.


I did not try Tcl, and plpython3 apparently builds, but I cannot see any 
tests being run.


--
Christian



--
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] create opclass documentation outdated

[Alvaro Herrera]

Emre Hasegeli wrote:
> >> In create_opclass.sgml, not only do we have the list of AMs supporting
> >> STORAGE, but there is also a paragraph describing which AMs do what
> >> for input datatypes of FUNCTION members (around line 175).
> >
> > I placed BRIN together with gist/gin/spgist here, namely that the optype
> > defaults to the opclass' datatype.
> 
> Requiring STORAGE type for BRIN opclasses is more of a bug than a
> feature.  None of the operator classes actually support storing values
> with different type.  We had intended to support it for inclusion
> opclass to index points by casting them to box, but then ripped it out
> because of inconsistencies on the operators caused by floating point
> arithmetics.
> 
> The problem is that the operator classes try to query the strategies
> using the datatype they got from TupleDesc structure.  It fails at
> least for cidr, because it is casted implicitly and indexed as inet.
> All of the BRIN operator classes can fail the same way for user
> defined types.  Adding STORAGE to all operator classes have seemed to
> me like an easy fix at the time I noticed this problem, but what we
> really need to do is to fix this than document.  We should to make
> BRIN use the datatype of the operator class as btree does.

Hmm, but if we ever add support for other types in inclusion as you
describe, we will need STORAGE, no?  I think it's unfortunate that the
code currently uses the field where it's not really necessary, but
harmless; if people break their systems by introducing bogus optypes,
it's their fault.  We can discuss improving this in the future, but in
the back branches it seems fine to leave it as is.

One possible idea to improve this (not for 9.6!) is to use the new
opclass-checker mechanism recently introduced in 65c5fcd353a859da9e6;
for minmax we could check that the opclass is defined with optype set to
0 or a type to which there's an implicit cast -- or some similar rule.
Not sure what we need for inclusion, and I'm fairly certain that we
could need completely different rules for other types of opclasses;
consider the idea of a bloom filter which was floated early during BRIN
development, for example, where the stored type is completely different
from the indexed type.

Anyway I'm incluined to push the patch with that paragraph as originally
posted.

> Other than those, the changes look good to me.

Thanks for the review.  I will push with those fixes.

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

[Robert Haas]

On Thu, Mar 10, 2016 at 10:30 AM, Simon Riggs  wrote:
> On 10 March 2016 at 06:53, Michael Paquier 
> wrote:
>>
>> On Wed, Mar 9, 2016 at 12:13 AM, Alvaro Herrera
>>  wrote:
>> > Robert Haas wrote:
>> >> I'm pretty meh about the whole idea of this function, though,
>> >> actually, and I don't see a single clear +1 vote for this
>> >> functionality upthread.  (Apologies if I've missed one.)  In the
>> >> absence of a few of those, I recommend we reject this.
>> >
>> > +1
>>
>> I'm meh for this patch.
>
>
> "meh" == +1
>
> I thought it meant -1

In my case it meant, like, -0.5.  I don't really like adding lots of
utility functions like this to the default install, because I'm not
sure how widely they get used and it gradually increases the size of
the code, system catalogs, etc.  But I also don't want to block
genuinely useful things.  So basically, I'm not excited about this
patch, but I don't want to fight about it either.

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


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

Re: [HACKERS] [PROPOSAL] Client Log Output Filtering

[Petr Jelinek]

On 10/03/16 15:08, David Steele wrote:

Looking at the code, this adds bool hide_from_client to edata which is
not initialized in errstart so that needs to be fixed.


I figured this would take care of it:

MemSet(edata, 0, sizeof(ErrorData));

Since I want hide_from_client to default to false.  Am I missing something?



Right, missed that, sorry for the noise.

I have another issue though.

The comment above errhidefromclient says "Only log levels below ERROR 
can be hidden from the client." but use of the errhidefromclient(true) 
actually does hide the error message from client, client just gets 
failed query without any message when used with ERROR level.


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


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

Re: [HACKERS] create opclass documentation outdated

[Alvaro Herrera]

Alvaro Herrera wrote:

> Hmm, but if we ever add support for other types in inclusion as you
> describe, we will need STORAGE, no?  I think it's unfortunate that the
> code currently uses the field where it's not really necessary, but
> harmless; if people break their systems by introducing bogus optypes,
> it's their fault.  We can discuss improving this in the future, but in
> the back branches it seems fine to leave it as is.

Hypothetical situation where a different storage type might be useful
with minmax: you store int2 values containing ln() of a numeric column.
(Not sure that there's any actual situation for people to store data
where this is valuable; consider astronomical distances, for example.)
You'd need to have support for a "cast" method that takes the values
from the ScanKey and applies ln() to them before doing the comparisons,
but it seems a reassonable setup.

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

[Corey Huinker]

On Thu, Mar 10, 2016 at 10:58 AM, Robert Haas  wrote:

> On Thu, Mar 10, 2016 at 10:30 AM, Simon Riggs 
> wrote:
> > On 10 March 2016 at 06:53, Michael Paquier 
> > wrote:
> >>
> >> On Wed, Mar 9, 2016 at 12:13 AM, Alvaro Herrera
> >>  wrote:
> >> > Robert Haas wrote:
> >> >> I'm pretty meh about the whole idea of this function, though,
> >> >> actually, and I don't see a single clear +1 vote for this
> >> >> functionality upthread.  (Apologies if I've missed one.)  In the
> >> >> absence of a few of those, I recommend we reject this.
> >> >
> >> > +1
> >>
> >> I'm meh for this patch.
> >
> >
> > "meh" == +1
> >
> > I thought it meant -1
>
> In my case it meant, like, -0.5.  I don't really like adding lots of
> utility functions like this to the default install, because I'm not
> sure how widely they get used and it gradually increases the size of
> the code, system catalogs, etc.  But I also don't want to block
> genuinely useful things.  So basically, I'm not excited about this
> patch, but I don't want to fight about it either.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

New patch for Alvaro's consideration.

Very minor changes since the last time, the explanations below are
literally longer than the changes:
- Rebased, though I don't think any of the files had changed in the mean
time
- Removed infinity checks/errors and the test cases to match
- Amended documentation to add 'days' after 'step' as suggested
- Did not add a period as suggested, to remain consistent with other
descriptions in the same sgml table
- Altered test case and documentation of 7 day step example so that the
generated dates do not land evenly on the end date. A reader might
incorrectly infer that the end date must be in the result set, when it
doesn't have to be.
- Removed unnecessary indentation that existed purely due to following of
other generate_series implementations
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4b5ee81..0a8c280 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -14700,6 +14700,26 @@ AND
   
  
 
+ 
+  generate_series(start, 
stop)
+  date
+  setof date
+  
+   Generate a series of values, from start to 
stop
+   with a step size of one day
+  
+ 
+
+ 
+  generate_series(start, 
stop, step 
integer)
+  date
+  setof date
+  
+   Generate a series of values, from start to 
stop
+   with a step size of step days
+  
+ 
+
 

   
@@ -14764,6 +14784,26 @@ SELECT * FROM generate_series('2008-03-01 
00:00'::timestamp,
  2008-03-03 22:00:00
  2008-03-04 08:00:00
 (9 rows)
+
+SELECT d.date_val FROM generate_series('1991-09-24'::date,'1991-10-01'::date) 
as d(date_val);
+  date_val  
+
+ 1991-09-24
+ 1991-09-25
+ 1991-09-26
+ 1991-09-27
+ 1991-09-28
+ 1991-09-29
+ 1991-09-30
+ 1991-10-01
+(8 rows)
+
+SELECT d.date_val FROM 
generate_series('1991-09-24'::date,'1991-10-02'::date,7) as d(date_val);
+  date_val  
+
+ 1991-09-24
+ 1991-10-01
+(2 rows)
 
   
 
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 332db7e..af4000d 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -30,6 +30,7 @@
 #include "utils/datetime.h"
 #include "utils/nabstime.h"
 #include "utils/sortsupport.h"
+#include "funcapi.h"
 
 /*
  * gcc's -ffast-math switch breaks routines that expect exact results from
@@ -2811,3 +2812,92 @@ timetz_izone(PG_FUNCTION_ARGS)
 
PG_RETURN_TIMETZADT_P(result);
 }
+
+/* Corey BEGIN */
+typedef struct
+{
+   DateADT current;
+   DateADT stop;
+   int32   step;
+} generate_series_date_fctx;
+
+
+/* generate_series_date()
+ * Generate the set of dates from start to stop by step
+ */
+Datum
+generate_series_date(PG_FUNCTION_ARGS)
+{
+   return generate_series_step_date(fcinfo);
+}
+
+Datum
+generate_series_step_date(PG_FUNCTION_ARGS)
+{
+   FuncCallContext *funcctx;
+   generate_series_date_fctx *fctx;
+   DateADT result;
+
+   /* stuff done only on the first call of the function */
+   if (SRF_IS_FIRSTCALL())
+   {
+   DateADT start = PG_GETARG_DATEADT(0);
+   DateADT stop = PG_GETARG_DATEADT(1);
+   int32   step = 1;
+   MemoryContext oldcontext;
+
+   /* see if we were given an explicit step size */
+   if (PG_NARGS() == 3)
+   {
+   step = PG_GETARG_INT32(2);
+   if (step == 0)
+   ereport(ERROR,
+   
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("step size cannot equal 
zero")));
+   }
+
+   /* create a function context for cross-call persistence */
+   funcctx = SRF_FIRSTCALL_INI

Re: [HACKERS] Freeze avoidance of very large table.

[Robert Haas]

On Thu, Mar 10, 2016 at 8:51 AM, Masahiko Sawada  wrote:
> After some further thought, I thought that it's better to add check
> logic for result of rewriting visibility map to upgrading logic rather
> than regression test in order to ensure that rewriting visibility map
> has been successfully done.
> As a draft, attached patch checks the result of rewriting visibility
> map after rewrote for each relation as a routine of pg_upgrade.
> The disadvantage point of this is that we need to scan each visibility
> map page for 2 times.
> But since visibility map size would not be so large, it would not bad.
> Thoughts?

I think that's kind of pointless.  We need to test that this
conversion code works, but once it does, I don't think we should make
everybody pay the overhead of retesting that.  Anyway, the test code
could have bugs, too.

Here's an updated version of your patch with that code removed and
some cosmetic cleanups like fixing typos and stuff like that.  I think
this is mostly ready to commit, but I noticed one problem: your
conversion code always produces two output pages for each input page
even if one of them would be empty.  In particular, if you have a
large number of small relations and run pg_upgrade, all of their
visibility maps will go from 8kB to 16kB.  That isn't the end of the
world, maybe, but I think you should see if you can't fix it
somehow

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/bin/pg_upgrade/file.c b/src/bin/pg_upgrade/file.c
index 2a99a28..34e1451 100644
--- a/src/bin/pg_upgrade/file.c
+++ b/src/bin/pg_upgrade/file.c
@@ -9,10 +9,15 @@
 
 #include "postgres_fe.h"
 
+#include "access/visibilitymap.h"
 #include "pg_upgrade.h"
+#include "storage/bufpage.h"
+#include "storage/checksum.h"
+#include "storage/checksum_impl.h"
 
 #include 
 
+#define BITS_PER_HEAPBLOCK_OLD 1
 
 
 #ifndef WIN32
@@ -138,6 +143,130 @@ copy_file(const char *srcfile, const char *dstfile, bool force)
 #endif
 
 
+/*
+ * rewriteVisibilityMap()
+ *
+ * In versions of PostgreSQL prior to catversion 201603011, PostgreSQL's
+ * visibility map included one bit per heap page; it now includes two.
+ * When upgrading a cluster from before that time to a current PostgreSQL
+ * version, we could refuse to copy visibility maps from the old cluster
+ * to the new cluster; the next VACUUM would recreate them, but at the
+ * price of scanning the entire table.  So, instead, we rewrite the old
+ * visibility maps in the new format.  That way, the all-visible bit
+ * remains set for the pages for which it was set previously.  The
+ * all-frozen bit is never set by this conversion; we leave that to
+ * VACUUM.
+ */
+const char *
+rewriteVisibilityMap(const char *fromfile, const char *tofile, bool force)
+{
+	int			src_fd = 0;
+	int			dst_fd = 0;
+	char		buffer[BLCKSZ];
+	ssize_t		bytesRead;
+	int			rewriteVmBytesPerPage;
+	BlockNumber blkno = 0;
+
+	/* Compute we need how many old page bytes to rewrite a new page */
+	rewriteVmBytesPerPage = (BLCKSZ - SizeOfPageHeaderData) / 2;
+
+	if ((fromfile == NULL) || (tofile == NULL))
+		return "Invalid old file or new file";
+
+	if ((src_fd = open(fromfile, O_RDONLY, 0)) < 0)
+		return getErrorText();
+
+	if ((dst_fd = open(tofile, O_RDWR | O_CREAT | (force ? 0 : O_EXCL), S_IRUSR | S_IWUSR)) < 0)
+	{
+		close(src_fd);
+		return getErrorText();
+	}
+
+	/*
+	 * Turn each visibility map page into 2 pages one by one. Each new page
+	 * has the same page header as the old one.
+	 */
+	while ((bytesRead = read(src_fd, buffer, BLCKSZ)) == BLCKSZ)
+	{
+		char	   *old_cur,
+   *old_break,
+   *old_blkend;
+		PageHeaderData pageheader;
+
+		/* Save the page header data */
+		memcpy(&pageheader, buffer, SizeOfPageHeaderData);
+
+		/*
+		 * These old_* variables point to old visibility map page. old_cur
+		 * points to current position on old page. old_blkend points to end of
+		 * old block. old_break points to old page break position for rewriting
+		 * a new page. After wrote a new page, old_break proceeds
+		 * rewriteVmBytesPerPage bytes.
+		 */
+		old_cur = buffer + SizeOfPageHeaderData;
+		old_blkend = buffer + bytesRead;
+		old_break = old_cur + rewriteVmBytesPerPage;
+
+		while (old_blkend >= old_break)
+		{
+			char		vmbuf[BLCKSZ];
+			char	   *new_cur = vmbuf;
+
+			/* Copy page header in advance */
+			memcpy(vmbuf, &pageheader, SizeOfPageHeaderData);
+
+			new_cur += SizeOfPageHeaderData;
+
+			/*
+			 * Process old page bytes one by one, and turn it into new page.
+			 */
+			while (old_break > old_cur)
+			{
+uint16		new_vmbits = 0;
+int			i;
+
+/* Generate new format bits while keeping old information */
+for (i = 0; i < BITS_PER_BYTE; i++)
+{
+	uint8	byte = * (uint8 *) old_cur;
+
+	if (((byte & (1 << (BITS_PER_HEAPBLOCK_OLD * i != 0)
+		new_vmbits |= 1 << (BITS_PER_HEAPBLOCK * i);
+}
+
+/* Copy new visibility map bit to new format page */
+memcpy(new_cur, 

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

[Alvaro Herrera]

Corey Huinker wrote:

> New patch for Alvaro's consideration.

Thanks.  As I said, it will be a few days before I get to this; any
further reviews in the meantime are welcome.

-- 
Á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] unexpected result from to_tsvector

[Dmitrii Golub]

2016-03-08 0:46 GMT+03:00 Artur Zakirov :

> Hello,
>
> On 07.03.2016 23:55, Dmitrii Golub wrote:
>
>>
>>
>> Hello,
>>
>> Should we added tests for this case?
>>
>
> I think we should. I have added tests for teo...@123-stack.net and
> 1...@stack.net emails.
>
>
>> 123_reg.ro  is not valid domain name, bacause of
>> symbol "_"
>>
>> https://tools.ietf.org/html/rfc1035 page 8.
>>
>> Dmitrii Golub
>>
>
> Thank you for the information. Fixed.
>
>
> --
> Artur Zakirov
> Postgres Professional: http://www.postgrespro.com
> Russian Postgres Company
>

Looks good to me

Dmitrii Golub

Re: [HACKERS] [PROPOSAL] Client Log Output Filtering

[Tom Lane]

Petr Jelinek  writes:
> The comment above errhidefromclient says "Only log levels below ERROR 
> can be hidden from the client." but use of the errhidefromclient(true) 
> actually does hide the error message from client, client just gets 
> failed query without any message when used with ERROR level.

Um.  That seems pretty broken --- I think it's a violation of the wire
protocol spec.

I notice though that we allow client_min_messages to be set to FATAL,
which would be a different way of violating the protocol.  Maybe we
should reduce the max setting of that to ERROR?

libpq/psql seem to more or less survive this situation:

regression=# set client_min_messages to fatal;
SET
regression=# select 2/0;
regression=# select 1;
 ?column? 
--
1
(1 row)

but it doesn't really seem like a great idea.

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 [Analyze] produces parallel scan for select Into table statements.

[Robert Haas]

On Thu, Mar 10, 2016 at 4:43 AM, Amit Kapila  wrote:
> There should be a white space between 0:CURSOR_OPT_PARALLEL_OK.  Also I
> don't see this comment is required as similar other usage doesn't have any
> such comment.  Fixed these two points in the attached patch.
>
> In general, the patch looks good to me and solves the problem mentioned.  I
> have ran the regression tests with force_parallel_mode and doesn't see any
> problem.

I guess there must not be an occurrence of this pattern in the
regression tests, or previous force_parallel_mode testing would have
found this problem.  Perhaps this patch should add one?

-- 
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] Endless loop calling PL/Python set returning functions

[Alexey Grishchenko]

I agree that passing function parameters through globals is not the best
solution

It works in a following way - executing custom code (in our case Python
function invocation) in Python is made with PyEval_EvalCode
. As an input to this C
function you specify dictionary of globals that would be available to this
code. The structure PLyProcedure stores "PyObject *globals;", which is the
dictionary of globals for specific function. So SPI works pretty fine, as
each function has a separate dictionary of globals and they don't conflict
with each other

One scenario when the problem occurs, is when you are calling the same
set-returning function in a single query twice. This way they share the
same "globals" which is not a bad thing, but when one function finishes
execution and deallocates input parameter's global, the second will fail
trying to do the same. I included the fix for this problem in my patch

The second scenario when the problem occurs is when you want to call the
same PL/Python function in recursion. For example, this code will not work:


create or replace function test(a int) returns int as $BODY$
r = 0
if a > 1:
r = plpy.execute("SELECT test(%d) as a" % (a-1))[0]['a']
return a + r
$BODY$ language plpythonu;

select test(10);


The function "test" has a single PLyProcedure object allocated to handle
it, thus it has a single "globals" dictionary. When internal function call
finishes, it removes the key "a" from the dictionary, and the outer
function fails with "NameError: global name 'a' is not defined" when it
tries to execute "return a + r"

But the second issue is a separate story and I think it is worth a separate
patch


On Thu, Mar 10, 2016 at 3:35 PM, Tom Lane  wrote:

> Alexey Grishchenko  writes:
> > There is a bug in implementation of set-returning functions in PL/Python.
> > When you call the same set-returning function twice in a single query,
> the
> > executor falls to infinite loop which causes OOM.
>
> Ugh.
>
> > Another issue with calling the same set-returning function twice in the
> > same query, is that it would delete the input parameter of the function
> > from the global variables dictionary at the end of execution. With
> calling
> > the function twice, this code attempts to delete the same entry from
> global
> > variables dict twice, thus causing KeyError. This is why the
> > function PLy_function_delete_args is modified as well to check whether
> the
> > key we intend to delete is in the globals dictionary.
>
> That whole business with putting a function's parameters into a global
> dictionary makes me itch.  Doesn't it mean problems if one plpython
> function calls another (presumably via SPI)?
>
> regards, tom lane
>



-- 
Best regards,
Alexey Grishchenko

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

[Michael Paquier]

On Thu, Mar 10, 2016 at 4:58 PM, Robert Haas  wrote:
> On Thu, Mar 10, 2016 at 10:30 AM, Simon Riggs  wrote:
>> On 10 March 2016 at 06:53, Michael Paquier 
>> wrote:
>>>
>>> On Wed, Mar 9, 2016 at 12:13 AM, Alvaro Herrera
>>>  wrote:
>>> > Robert Haas wrote:
>>> >> I'm pretty meh about the whole idea of this function, though,
>>> >> actually, and I don't see a single clear +1 vote for this
>>> >> functionality upthread.  (Apologies if I've missed one.)  In the
>>> >> absence of a few of those, I recommend we reject this.
>>> >
>>> > +1
>>>
>>> I'm meh for this patch.
>>
>>
>> "meh" == +1
>>
>> I thought it meant -1
>
> In my case it meant, like, -0.5.  I don't really like adding lots of
> utility functions like this to the default install, because I'm not
> sure how widely they get used and it gradually increases the size of
> the code, system catalogs, etc.  But I also don't want to block
> genuinely useful things.  So basically, I'm not excited about this
> patch, but I don't want to fight about it either.

I am of the same feeling, at -0.5. I don't feel like putting -1 for
this patch, as I don't really understand why this is worth adding more
complexity in the code for something that can be done with
generate_series using timestamps. Also, why only dates? And why not
other units like hours or seconds?
-- 
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] Add generate_series(date,date) and generate_series(date,date,integer)

[David G. Johnston]

On Thu, Mar 10, 2016 at 8:58 AM, Robert Haas  wrote:

> On Thu, Mar 10, 2016 at 10:30 AM, Simon Riggs 
> wrote:
> > On 10 March 2016 at 06:53, Michael Paquier 
> > wrote:
> >>
> >> On Wed, Mar 9, 2016 at 12:13 AM, Alvaro Herrera
> >>  wrote:
> >> > Robert Haas wrote:
> >> >> I'm pretty meh about the whole idea of this function, though,
> >> >> actually, and I don't see a single clear +1 vote for this
> >> >> functionality upthread.  (Apologies if I've missed one.)  In the
> >> >> absence of a few of those, I recommend we reject this.
> >> >
> >> > +1
> >>
> >> I'm meh for this patch.
> >
> >
> > "meh" == +1
> >
> > I thought it meant -1
>
> In my case it meant, like, -0.5.  I don't really like adding lots of
> utility functions like this to the default install, because I'm not
> sure how widely they get used and it gradually increases the size of
> the code, system catalogs, etc.  But I also don't want to block
> genuinely useful things.  So basically, I'm not excited about this
> patch, but I don't want to fight about it either.
>

​I tend to think we err toward this too much.  This seems like development
concerns trumping usability.  Consider that anything someone took the time
to write and polish to make committable to core was obviously genuinely
useful to them - and for every person capable of actually taking things
that far there are likely many more like myself who cannot but have
encountered the, albeit minor, usability annoyance that this kind of
function seeks to remove.

I really don't care that the codebase is marginally larger or that there is
another few records in the catalog - I hope that our code organization and
performance is capable of handling it (and many more).

The overhead of adding an entirely new concept to core would give me more
pause in that situation but this function in particular simply fleshes out
what the user community sees to be a minor yet notable lack in our existing
offering of the generate_series() feature.  Its threshold for meeting
"genuinely" should be considerably lower than for more invasive or complex
features such as those that require entirely new syntax (e.g., the recently
rejected ALTER ROLE patch) to implement.

If something can be done with a user-defined function on stock PostgreSQL,
especially for concepts or features we already have, the decision to commit
a c-language function that someone else has written and others have
reviewed should, IMO, be given the benefit of assumed usefulness.

My $0.02

David J.

Re: [HACKERS] Endless loop calling PL/Python set returning functions

[Tom Lane]

Alexey Grishchenko  writes:
> One scenario when the problem occurs, is when you are calling the same
> set-returning function in a single query twice. This way they share the
> same "globals" which is not a bad thing, but when one function finishes
> execution and deallocates input parameter's global, the second will fail
> trying to do the same. I included the fix for this problem in my patch

> The second scenario when the problem occurs is when you want to call the
> same PL/Python function in recursion. For example, this code will not work:

Right, the recursion case is what's not being covered by this patch.
I would rather have a single patch that deals with both of those cases,
perhaps by *not* sharing the same dictionary across calls.  I think
what you've done here is not so much a fix as a band-aid.  In fact,
it doesn't even really fix the problem for the two-calls-per-query
case does it?  It'll work if the first execution of the SRF is run to
completion before starting the second one, but not if the two executions
are interleaved.  I believe you can test that type of scenario with
something like

  select set_returning_function_1(...), set_returning_function_2(...);

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] Is there a way around function search_path killing SQL function inlining?

[Tom Lane]

Robert Haas  writes:
> Hmm.  The meaning of funcs.inline depends on the search_path, not just
> during dump restoration but all the time.  So anything uses it under a
> different search_path setting than the normal one will have this kind
> of problem; not just dump/restore.

Yeah, I see no reason to claim that this is a dump/restore-specific
problem.

> I don't have a very good idea what to do about that.

The safe way to write SQL-language functions to be search-path-proof
is to schema-qualify the names in them, or to add a "SET search_path"
clause to the function definition.

The problem with the latter approach is that it defeats inlining.
I thought for a minute that maybe we could teach the planner to do
inlining anyway by parsing the function body with the adjusted
search_path, but that doesn't really preserve the same semantics
(a SET would change the environment for called functions too).

So for now, the recommendation has to be "write functions you want
to inline with schema qualifications".  If you're worried about
preserving relocatability of an extension containing such functions,
the @extschema@ feature might help.

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] Endless loop calling PL/Python set returning functions

[Alexey Grishchenko]

No, my fix handles this well.

In fact, with the first function call you allocate global variables
representing Python function input parameters, call the function and
receive iterator over the function results. Then in a series of Postgres
calls to PL/Python handler you just fetch next value from the iterator, you
are not calling the Python function anymore. When the iterator reaches the
end, PL/Python call handler deallocates the global variable representing
function input parameter.

Regardless of the number of parallel invocations of the same function, each
of them in my patch would set its own input parameters to the Python
function, call the function and receive separate iterators. When the first
function's result iterator would reach its end, it would deallocate the
input global variable. But it won't affect other functions as they no
longer need to invoke any Python code. Even if they need - they would
reallocate global variable (it would be set before the Python function
invocation). The issue here was in the fact that they tried to deallocate
the global input variable multiple times independently, which caused error
that I fixed.

Regarding the patch for the second case with recursion - not caching the
"globals" between function calls would have a performance impact, as you
would have to construct "globals" object before each function call. And you
need globals as it contains references to "plpy" module and global
variables and global dictionary ("GD"). I will think on this, maybe there
might be a better design for this scenario. But I still think the second
scenario requires a separate patch

On Thu, Mar 10, 2016 at 4:33 PM, Tom Lane  wrote:

> Alexey Grishchenko  writes:
> > One scenario when the problem occurs, is when you are calling the same
> > set-returning function in a single query twice. This way they share the
> > same "globals" which is not a bad thing, but when one function finishes
> > execution and deallocates input parameter's global, the second will fail
> > trying to do the same. I included the fix for this problem in my patch
>
> > The second scenario when the problem occurs is when you want to call the
> > same PL/Python function in recursion. For example, this code will not
> work:
>
> Right, the recursion case is what's not being covered by this patch.
> I would rather have a single patch that deals with both of those cases,
> perhaps by *not* sharing the same dictionary across calls.  I think
> what you've done here is not so much a fix as a band-aid.  In fact,
> it doesn't even really fix the problem for the two-calls-per-query
> case does it?  It'll work if the first execution of the SRF is run to
> completion before starting the second one, but not if the two executions
> are interleaved.  I believe you can test that type of scenario with
> something like
>
>   select set_returning_function_1(...), set_returning_function_2(...);
>
> regards, tom lane
>



-- 
Best regards,
Alexey Grishchenko

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

[David G. Johnston]

On Thu, Mar 10, 2016 at 9:30 AM, Michael Paquier 
wrote:

> On Thu, Mar 10, 2016 at 4:58 PM, Robert Haas 
> wrote:
> > On Thu, Mar 10, 2016 at 10:30 AM, Simon Riggs 
> wrote:
> >> On 10 March 2016 at 06:53, Michael Paquier 
> >> wrote:
> >>>
> >>> On Wed, Mar 9, 2016 at 12:13 AM, Alvaro Herrera
> >>>  wrote:
> >>> > Robert Haas wrote:
> >>> >> I'm pretty meh about the whole idea of this function, though,
> >>> >> actually, and I don't see a single clear +1 vote for this
> >>> >> functionality upthread.  (Apologies if I've missed one.)  In the
> >>> >> absence of a few of those, I recommend we reject this.
> >>> >
> >>> > +1
> >>>
> >>> I'm meh for this patch.
> >>
> >>
> >> "meh" == +1
> >>
> >> I thought it meant -1
> >
> > In my case it meant, like, -0.5.  I don't really like adding lots of
> > utility functions like this to the default install, because I'm not
> > sure how widely they get used and it gradually increases the size of
> > the code, system catalogs, etc.  But I also don't want to block
> > genuinely useful things.  So basically, I'm not excited about this
> > patch, but I don't want to fight about it either.
>
> I am of the same feeling, at -0.5. I don't feel like putting -1 for
> this patch, as I don't really understand why this is worth adding more
> complexity in the code for something that can be done with
> generate_series using timestamps. Also, why only dates? And why not
> other units like hours or seconds?
>

​A date is a type, hours and seconds are not types.  To use hours and
seconds you need timestamp (I guess we could offer a "time" version of this
function too) which we already have.  Also, not choosing to implement
something else generally shouldn't preclude something that exists and have
genuine value from being committed.

Obviously there is some user-land annoyance at having to play with
timestamp when all one really cares about is date.  Given the prevalence of
date usage in user-land this is not surprising.

We're not forcing anyone to review this that doesn't see that it is worth
their time.  We are asking th
​at​
the people that the community has placed in a position of authority spend
some a limited amount of effort reviewing a minor addition that has been
deemed desirable and that has already been reviewed and deemed something
that meets the project's technical requirements.
​  The expectation is that the amount of ongoing support this function
would require is similar to that of the existing generate_series functions.​


​This is something that can be easily added by the user as a SQL function -
its complexity cannot be so great as to be deemed a risk to the system but
including its c-language variant.  As you said, we already do something
very similar for timestamps so the marginal complexity being added
shouldn't be significant.

If you are going to -1 or -0.5 for "adds too much complexity" it would be
helpful to know specifics.  Scanning the thread the only real concern was
dealing with infinity - which is already a complexity the current functions
have so there is no "additional" there - but maybe I've missed something.

I understand Robert's position and while I find it to be community-hostile
this is an open source project and so I accept that this is a possible
outcome.  But as soon as he asked for some +1s he got them (mostly without
reasons but the reality is that the request was for desire) and a few "I
vote -0.5 since my dislike is only half-baked".  And the fact is that a
single +1 for the idea likely represents many people at large who would use
the function if present while I suspect most of those who could offer an
informed -1 are already on this list.  The vast majority probably don't
care either way as long as we don't introduce bugs.

David J.

Re: [HACKERS] [PROPOSAL] Client Log Output Filtering

[Petr Jelinek]

On 10/03/16 17:07, Tom Lane wrote:

Petr Jelinek  writes:

The comment above errhidefromclient says "Only log levels below ERROR
can be hidden from the client." but use of the errhidefromclient(true)
actually does hide the error message from client, client just gets
failed query without any message when used with ERROR level.


Um.  That seems pretty broken --- I think it's a violation of the wire
protocol spec.



I was thinking that as well. The doc says that on error the 
ErrorResponse is sent and connection is closed and we clearly fail to 
send the ErrorResponse in this case.



I notice though that we allow client_min_messages to be set to FATAL,
which would be a different way of violating the protocol.  Maybe we
should reduce the max setting of that to ERROR?



Hmm yeah that seems to be that way for very long time though so I wonder 
if that's intentional although it's also against protocol spec then. In 
any case I would be in favor of lowering the max setting to ERROR.


For the patch at hand, it should be sufficient for errhidefromclient() 
to check that the edata->elevel is lower than ERROR.


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


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

Re: [HACKERS] Optimizer questions

[Tom Lane]

konstantin knizhnik  writes:
> But right now the rule for cost estimation makes it not possible to apply 
> this optimization for simple expressions like this: ...
> I wonder is there any advantages of earlier evaluation of such simple 
> expressions if them are not needed for sort?

Well, as I said, my patch was intentionally written to be conservative
about when to change the semantics from what it's been for the last
twenty years.  I think there's a good argument for changing it for
volatile functions, but I'm less convinced that we should whack around
what happens in cases where there is not a clear benefit.  It's fairly
likely that there are users out there who have (perhaps without even
knowing it) optimized their queries to work well with eval-before-sort
behavior, perhaps by applying data-narrowing functions.  They won't
thank us for changing the behavior "just because".

If we had a more reliable idea of whether we are widening or narrowing
the sort data by postponing eval, I'd be willing to be more aggressive
about doing it.  But without that, I think it's best to be conservative
and only change when there's a pretty clear potential benefit.

> Also I do not completely understand your concern about windows functions.

The point is just that we have to not disassemble WindowFunc nodes when
building the sort-input tlist, in the same way that we don't disassemble
Aggref nodes.  If we are sorting the output of a WindowAgg, the WindowFunc
expressions have to appear as expressions in the WindowAgg's output tlist.

>> I think it's probably also broken for SRFs in the tlist; we need to work
>> out what semantics we want for those.  If we postpone any SRF to after
>> the Sort, we can no longer assume that a query LIMIT enables use of
>> bounded sort (because the SRF might repeatedly return zero rows).
>> I don't have a huge problem with that, but I think now would be a good
>> time to nail down some semantics.

As far as that goes, it seems to me after thinking about it that
non-sort-column tlist items containing SRFs should always be postponed,
too.  Performing a SRF before sorting bloats the sort data vertically,
rather than horizontally, but it's still bloat.  (Although as against
that, when you have ORDER BY + LIMIT, postponing SRFs loses the ability
to use a bounded sort.)  The killer point though is that unless the sort
is stable, it might cause surprising changes in the order of the SRF
output values.  Our sorts aren't stable; here's an example in HEAD:

# select q1, generate_series(1,9) from int8_tbl order by q1 limit 7;
 q1  | generate_series 
-+-
 123 |   2
 123 |   3
 123 |   4
 123 |   5
 123 |   6
 123 |   7
 123 |   1
(7 rows)

I think that's pretty surprising, and if we have an opportunity to
provide more intuitive semantics here, we should do it.

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] amcheck (B-Tree integrity checking tool)

[Tomas Vondra]

Hi,

I've looked at this patch today, mostly to educate myself, so this
probably should not count as a full review. Anyway, the patch seems in
excellent shape - it'd be great if all patches (including those written
by me) had this level of comments/docs.

On Mon, 2016-02-29 at 16:09 -0800, Peter Geoghegan wrote:
> I was assigned an "action point" during the FOSDEM developer meeting:
> "Post new version of btree consistency checker patch". I attach a new
> WIP version of my consistency checker tool, amcheck.

...

> To recap, the extension adds some SQL-callable functions that verify
> certain invariant conditions hold within some particular B-Tree index.
> These are the conditions that index scans rely on always being true.
> The tool's scope may eventually cover other AMs, including heapam, but
> nbtree seems like the best place to start.
> 
> Note that no function currently checks that the index is consistent
> with the heap, which would be very useful (that's probably how I'd
> eventually target the heapam, actually).

I'm afraid I don't understand what "target the heapam" means. Could you
elaborate?


> Invariants
> 
...
> Obviously, this tool is all about distrusting the structure of a
> B-Tree index. That being the case, it's not always totally clear where
> to draw the line. I think I have the balance about right, though.

I agree. It'd be nice to have a tool that also checks for data
corruption the a lower level (e.g. that varlena headers are not
corrupted etc.) but that seems like a task for some other tool.


> Interface
> ===
> 
> There are only 2 SQL callable functions in the extension, which are
> very similar:
> 
> bt_index_check(index regclass)
> 
> bt_index_parent_check(index regclass)
> 

Can we come up with names that more clearly identify the difference
between those two functions? I mean, _parent_ does not make it
particularly obvious that the second function acquires exclusive lock
and performs more thorough checks.

This also applies to the name of the contrib module - it's not
particularly obvious what "amcheck" unless you're familiar with the
concept of access methods. Which is quite unlikely for regular users.
Maybe something like "check_index" would be more illustrative?

> Locking
> ==
...
> 
> We do, on the other hand, have a detailed rationale for why it's okay
> that we don't have an ExclusiveLock on the index relation for checks
> that span the key space of more than one page by following right links
> to compare items across sibling pages. This isn't the same thing as
> having an explicit interlock like a pin -- our interlock is one
> against *recycling* by vacuum, which is based on recentGlobalXmin.
> This rationale requires expert review.

Well, I wouldn't count myself as an expert here, but do we actually need
such protection? I mean, we only do such checks when holding an
exclusive lock on the parent relation, no? And even if we don't vacuum
can only remove entries from the pages - why should that cause
violations of any invariants?

A few minor comments:

1) This should probably say "one block per level":

/*
 * A B-Tree cannot possibly have this many levels, since there must be 
 * one block per page, and that is bound by the range of BlockNumber:
 */

2) comment before bt_check_every_level() says:

   ... assumed to prevent any kind of physically modification ...

   probably should be "physical modification" instead.

3) s/targecontext/targetcontext/ in BtreeCheckState comment

regards

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



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

Re: [HACKERS] Endless loop calling PL/Python set returning functions

[Tom Lane]

Alexey Grishchenko  writes:
> No, my fix handles this well.
> In fact, with the first function call you allocate global variables
> representing Python function input parameters, call the function and
> receive iterator over the function results. Then in a series of Postgres
> calls to PL/Python handler you just fetch next value from the iterator, you
> are not calling the Python function anymore. When the iterator reaches the
> end, PL/Python call handler deallocates the global variable representing
> function input parameter.

> Regardless of the number of parallel invocations of the same function, each
> of them in my patch would set its own input parameters to the Python
> function, call the function and receive separate iterators. When the first
> function's result iterator would reach its end, it would deallocate the
> input global variable. But it won't affect other functions as they no
> longer need to invoke any Python code.

Well, if you think that works, why not undo the global-dictionary changes
at the end of the first call, rather than later?  Then there's certainly
no overlap in their lifespan.

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] Tsvector editing functions

[Teodor Sigaev]

Thanks! Fixed and added tests.

Thank you!

I did some patch cleanup/fix, but I have some doubt with function's names:

1 to_tsvector:
# \df to_tsvector
 List of functions
   Schema   |Name | Result data type | Argument data types |  Type
+-+--+-+
 pg_catalog | to_tsvector | tsvector | regconfig, text | normal
 pg_catalog | to_tsvector | tsvector | text| normal
 pg_catalog | to_tsvector | tsvector | text[]  | normal

First two variants of to_tsvector make a morphological processing, last one 
doesn't.

2 to_array
# \df *to_array
  List of functions
   Schema   | Name  | Result data type | Argument data types | 
 Type

+---+--+-+
 pg_catalog | regexp_split_to_array | text[]   | text, text  | 
normal
 pg_catalog | regexp_split_to_array | text[]   | text, text, text| 
normal
 pg_catalog | string_to_array   | text[]   | text, text  | 
normal
 pg_catalog | string_to_array   | text[]   | text, text, text| 
normal
 pg_catalog | to_array  | text[]   | tsvector| 
normal


Seems, to_array is not a right name compared to other *to_array.

I would like to suggest rename both functions to array_to_tsvector and 
tsvector_to_array to have consistent name. Later we could add 
to_tsvector([regconfig, ], text[]) with morphological processing.


Thoughts?



--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4b5ee81..ed0b6be 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -9211,16 +9211,29 @@ CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green', 'blue', 'purple
  
   setweight
  
- setweight(tsvector, "char")
+ setweight(vector tsvector, weight "char")
 
 tsvector
-assign weight to each element of tsvector
+assign weight to each element of vector
 setweight('fat:2,4 cat:3 rat:5B'::tsvector, 'A')
 'cat':3A 'fat':2A,4A 'rat':5A


 
  
+  setweight
+  setweight by filter
+ 
+ setweight(vector tsvector, weight "char", lexemes "text"[])
+
+tsvector
+assign weight to elements of vector that are listed in lexemes array
+setweight('fat:2,4 cat:3 rat:5B'::tsvector, 'A', '{cat,rat}')
+'cat':3A 'fat':2,4 'rat':5A
+   
+   
+
+ 
   strip
  
  strip(tsvector)
@@ -9233,6 +9246,81 @@ CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green', 'blue', 'purple

 
  
+  delete
+  delete lemexeme
+ 
+ delete(vector tsvector, lexeme text)
+
+tsvector
+remove given lexeme from vector
+delete('fat:2,4 cat:3 rat:5A'::tsvector, 'fat')
+'cat':3 'rat':5A
+   
+   
+
+ 
+  delete
+  delete lemexemes array
+ 
+ delete(vector tsvector, lexemes text[])
+
+tsvector
+remove any occurrence of lexemes in lexemes array from vector
+delete('fat:2,4 cat:3 rat:5A'::tsvector, ARRAY['fat','rat'])
+'cat':3
+   
+   
+
+ 
+  unnest
+ 
+ unnest(tsvector, OUT lexeme text, OUT positions smallint[], OUT weights text)
+
+setof record
+expand a tsvector to a set of rows
+unnest('fat:2,4 cat:3 rat:5A'::tsvector)
+(cat,{3},{D}) ...
+   
+   
+
+ 
+  to_array
+ 
+ to_array(tsvector)
+
+text[]
+convert tsvector to array of lexemes
+to_array('fat:2,4 cat:3 rat:5A'::tsvector)
+{cat,fat,rat}
+   
+   
+
+ 
+  to_tsvector
+  array to tsvector
+ 
+ to_tsvector(text[])
+
+tsvector
+convert array of lexemes to tsvector
+to_tsvector('{fat,cat,rat}'::text[])
+'fat' 'cat' 'rat'
+   
+   
+
+ 
+  filter
+ 
+ filter(vector tsvector, weights "char"[])
+
+tsvector
+Select only elements with given weights from vector
+filter('fat:2,4 cat:3b rat:5A'::tsvector, '{a,b}')
+'cat':3B 'rat':5A
+   
+   
+
+ 
   to_tsquery
  
  to_tsquery( config regconfig ,  query text)
diff --git a/doc/src/sgml/textsearch.sgml b/doc/src/sgml/textsearch.sgml
index ff99976..ea3abc9 100644
--- a/doc/src/sgml/textsear

Re: [HACKERS] WAL log only necessary part of 2PC GID

[Petr Jelinek]

On 10/03/16 13:43, Pavan Deolasee wrote:

On Wed, Mar 9, 2016 at 7:56 PM, Petr Jelinek mailto:p...@2ndquadrant.com>> wrote:

Hi,

I wonder why you define the gidlen as uint32 when it would fit into
uint8 which in the current TwoPhaseFileHeader struct should be win
of  8 bytes on padding (on 64bit). I think that's something worth
considering given that this patch aims to lower the size of the data.


Hi Petr,

That sounds like a good idea; I didn't think about that. I would like to
make it uint16 though just in case if we decide to increase GIDSIZE from
200 to something more than 256 (Postgres-XL does that already). That
still fits in the same aligned width, on both 32 as well as 64-bit
machines. New version attached.


Correct, and I see Simon committed it like this in meantime, thanks.

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


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

Re: [HACKERS] Endless loop calling PL/Python set returning functions

[Alexey Grishchenko]

Tom Lane  wrote:

> Alexey Grishchenko > writes:
> > No, my fix handles this well.
> > In fact, with the first function call you allocate global variables
> > representing Python function input parameters, call the function and
> > receive iterator over the function results. Then in a series of Postgres
> > calls to PL/Python handler you just fetch next value from the iterator,
> you
> > are not calling the Python function anymore. When the iterator reaches
> the
> > end, PL/Python call handler deallocates the global variable representing
> > function input parameter.
>
> > Regardless of the number of parallel invocations of the same function,
> each
> > of them in my patch would set its own input parameters to the Python
> > function, call the function and receive separate iterators. When the
> first
> > function's result iterator would reach its end, it would deallocate the
> > input global variable. But it won't affect other functions as they no
> > longer need to invoke any Python code.
>
> Well, if you think that works, why not undo the global-dictionary changes
> at the end of the first call, rather than later?  Then there's certainly
> no overlap in their lifespan.
>
> regards, tom lane
>

Could you elaborate more on this? In general, stack-like solution would
work - if before the function call there is a global variable with the name
matching input variable name, push its value to the stack, and pop it after
the function execution. Would implement it tomorrow and see how it works


-- 

Sent from handheld device

[HACKERS] Adjusting the API of pull_var_clause()

[Tom Lane]

Over in the "Optimizer questions" thread, it's become apparent that
we need to fix pull_var_clause() to offer multiple behaviors for
WindowFunc nodes that are parallel to the ones it has for Aggrefs
(viz, reject, recurse, or include in result).  This should've been
done when window functions were introduced, likely; but we've escaped
the need for it so far because the planner hasn't done any real
analysis of post-WindowAgg targetlists.

The straightforward way to do this would be to add another enum type
similar to PVCAggregateBehavior and a fourth argument to pull_var_clause,
plus tedious updates of all twenty-or-so existing call sites, almost all
of which should choose PVC_REJECT_WINDOWFUNCS because they'd not expect
to get invoked on expressions that could contain WindowFuncs.

Now, I'm pretty sure that the last time we touched pull_var_clause's
API, we intentionally set it up to force every call site to be visited
when new behaviors were added.  But right at the moment that's looking
like it was a bad call.

An alternative API design could look like

#define PVC_INCLUDE_AGGREGATES   0x0001 /* include Aggrefs in output list */
#define PVC_RECURSE_AGGREGATES   0x0002 /* recurse into Aggref arguments */
#define PVC_INCLUDE_PLACEHOLDERS 0x0004 /* include PlaceHolderVars in output 
list */
#define PVC_RECURSE_PLACEHOLDERS 0x0008 /* recurse into PlaceHolderVar 
arguments */

extern List *pull_var_clause(Node *node, int flags);

with calls along the line of

pull_var_clause(node, PVC_INCLUDE_AGGREGATES | PVC_RECURSE_PLACEHOLDERS);

the default behavior if you specify no flag being "error if node type
is seen".

The attraction of this approach is that if we add another behavior
to pull_var_clause, while we'd still likely need to run around and
check every call site, it wouldn't be positively guaranteed that
we'd need to edit every darn one of them.

This might all be moot of course.  Either way, we'll have to touch every
call site today; and there is nothing on the horizon suggesting that we'll
need to make another change in pull_var_clause in the foreseeable future.

I'm undecided which way to fix it.  Anybody else have an opinion?

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

[Corey Huinker]

removed leftover development comment

On Thu, Mar 10, 2016 at 11:02 AM, Corey Huinker 
wrote:

> On Thu, Mar 10, 2016 at 10:58 AM, Robert Haas 
> wrote:
>
>> On Thu, Mar 10, 2016 at 10:30 AM, Simon Riggs 
>> wrote:
>> > On 10 March 2016 at 06:53, Michael Paquier 
>> > wrote:
>> >>
>> >> On Wed, Mar 9, 2016 at 12:13 AM, Alvaro Herrera
>> >>  wrote:
>> >> > Robert Haas wrote:
>> >> >> I'm pretty meh about the whole idea of this function, though,
>> >> >> actually, and I don't see a single clear +1 vote for this
>> >> >> functionality upthread.  (Apologies if I've missed one.)  In the
>> >> >> absence of a few of those, I recommend we reject this.
>> >> >
>> >> > +1
>> >>
>> >> I'm meh for this patch.
>> >
>> >
>> > "meh" == +1
>> >
>> > I thought it meant -1
>>
>> In my case it meant, like, -0.5.  I don't really like adding lots of
>> utility functions like this to the default install, because I'm not
>> sure how widely they get used and it gradually increases the size of
>> the code, system catalogs, etc.  But I also don't want to block
>> genuinely useful things.  So basically, I'm not excited about this
>> patch, but I don't want to fight about it either.
>>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
> New patch for Alvaro's consideration.
>
> Very minor changes since the last time, the explanations below are
> literally longer than the changes:
> - Rebased, though I don't think any of the files had changed in the mean
> time
> - Removed infinity checks/errors and the test cases to match
> - Amended documentation to add 'days' after 'step' as suggested
> - Did not add a period as suggested, to remain consistent with other
> descriptions in the same sgml table
> - Altered test case and documentation of 7 day step example so that the
> generated dates do not land evenly on the end date. A reader might
> incorrectly infer that the end date must be in the result set, when it
> doesn't have to be.
> - Removed unnecessary indentation that existed purely due to following of
> other generate_series implementations
>
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 4b5ee81..0a8c280 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -14700,6 +14700,26 @@ AND
   
  
 
+ 
+  generate_series(start, 
stop)
+  date
+  setof date
+  
+   Generate a series of values, from start to 
stop
+   with a step size of one day
+  
+ 
+
+ 
+  generate_series(start, 
stop, step 
integer)
+  date
+  setof date
+  
+   Generate a series of values, from start to 
stop
+   with a step size of step days
+  
+ 
+
 

   
@@ -14764,6 +14784,26 @@ SELECT * FROM generate_series('2008-03-01 
00:00'::timestamp,
  2008-03-03 22:00:00
  2008-03-04 08:00:00
 (9 rows)
+
+SELECT d.date_val FROM generate_series('1991-09-24'::date,'1991-10-01'::date) 
as d(date_val);
+  date_val  
+
+ 1991-09-24
+ 1991-09-25
+ 1991-09-26
+ 1991-09-27
+ 1991-09-28
+ 1991-09-29
+ 1991-09-30
+ 1991-10-01
+(8 rows)
+
+SELECT d.date_val FROM 
generate_series('1991-09-24'::date,'1991-10-02'::date,7) as d(date_val);
+  date_val  
+
+ 1991-09-24
+ 1991-10-01
+(2 rows)
 
   
 
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 332db7e..af4000d 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -30,6 +30,7 @@
 #include "utils/datetime.h"
 #include "utils/nabstime.h"
 #include "utils/sortsupport.h"
+#include "funcapi.h"
 
 /*
  * gcc's -ffast-math switch breaks routines that expect exact results from
@@ -2811,3 +2812,92 @@ timetz_izone(PG_FUNCTION_ARGS)
 
PG_RETURN_TIMETZADT_P(result);
 }
+
+typedef struct
+{
+   DateADT current;
+   DateADT stop;
+   int32   step;
+} generate_series_date_fctx;
+
+
+/* generate_series_date()
+ * Generate the set of dates from start to stop by step
+ */
+Datum
+generate_series_date(PG_FUNCTION_ARGS)
+{
+   return generate_series_step_date(fcinfo);
+}
+
+Datum
+generate_series_step_date(PG_FUNCTION_ARGS)
+{
+   FuncCallContext *funcctx;
+   generate_series_date_fctx *fctx;
+   DateADT result;
+
+   /* stuff done only on the first call of the function */
+   if (SRF_IS_FIRSTCALL())
+   {
+   DateADT start = PG_GETARG_DATEADT(0);
+   DateADT stop = PG_GETARG_DATEADT(1);
+   int32   step = 1;
+   MemoryContext oldcontext;
+
+   /* see if we were given an explicit step size */
+   if (PG_NARGS() == 3)
+   {
+   step = PG_GETARG_INT32(2);
+   if (step == 0)
+   ereport(ERROR,
+   
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+errmsg("step size cannot equal 

Re: [HACKERS] WIP: Upper planner pathification

[Andres Freund]

Hi Tom,

On 2016-02-28 15:03:28 -0500, Tom Lane wrote:
> diff --git a/src/include/optimizer/planmain.h 
> b/src/include/optimizer/planmain.h
> index eaa642b..cd7338a 100644
> *** a/src/include/optimizer/planmain.h
> --- b/src/include/optimizer/planmain.h
> *** extern RelOptInfo *query_planner(Planner
> *** 43,102 
>* prototypes for plan/planagg.c
>*/
>   extern void preprocess_minmax_aggregates(PlannerInfo *root, List *tlist);
> - extern Plan *optimize_minmax_aggregates(PlannerInfo *root, List *tlist,
> -const AggClauseCosts 
> *aggcosts, Path *best_path);
>   
>   /*
>* prototypes for plan/createplan.c
>*/
>   extern Plan *create_plan(PlannerInfo *root, Path *best_path);
> - extern SubqueryScan *make_subqueryscan(List *qptlist, List *qpqual,
> -   Index scanrelid, Plan *subplan);
>   extern ForeignScan *make_foreignscan(List *qptlist, List *qpqual,
>Index scanrelid, List *fdw_exprs, List 
> *fdw_private,
>List *fdw_scan_tlist, List *fdw_recheck_quals,
>Plan *outer_plan);
> - extern Append *make_append(List *appendplans, List *tlist);
> - extern RecursiveUnion *make_recursive_union(List *tlist,
> -  Plan *lefttree, Plan *righttree, int 
> wtParam,
> -  List *distinctList, long numGroups);
> - extern Sort *make_sort_from_pathkeys(PlannerInfo *root, Plan *lefttree,
> - List *pathkeys, double 
> limit_tuples);
> - extern Sort *make_sort_from_sortclauses(PlannerInfo *root, List *sortcls,
> -Plan *lefttree);
> - extern Sort *make_sort_from_groupcols(PlannerInfo *root, List *groupcls,
> -  AttrNumber *grpColIdx, Plan 
> *lefttree);
> - extern Agg *make_agg(PlannerInfo *root, List *tlist, List *qual,
> -  AggStrategy aggstrategy, const AggClauseCosts *aggcosts,
> -  int numGroupCols, AttrNumber *grpColIdx, Oid *grpOperators,
> -  List *groupingSets, long numGroups, bool combineStates,
> -  bool finalizeAggs, Plan *lefttree);
> - extern WindowAgg *make_windowagg(PlannerInfo *root, List *tlist,
> -List *windowFuncs, Index winref,
> -int partNumCols, AttrNumber *partColIdx, Oid 
> *partOperators,
> -int ordNumCols, AttrNumber *ordColIdx, Oid 
> *ordOperators,
> -int frameOptions, Node *startOffset, Node *endOffset,
> -Plan *lefttree);
> - extern Group *make_group(PlannerInfo *root, List *tlist, List *qual,
> -int numGroupCols, AttrNumber *grpColIdx, Oid *grpOperators,
> -double numGroups,
> -Plan *lefttree);
>   extern Plan *materialize_finished_plan(Plan *subplan);
> ! extern Unique *make_unique(Plan *lefttree, List *distinctList);
> ! extern LockRows *make_lockrows(Plan *lefttree, List *rowMarks, int 
> epqParam);
> ! extern Limit *make_limit(Plan *lefttree, Node *limitOffset, Node 
> *limitCount,
> !int64 offset_est, int64 count_est);
> ! extern SetOp *make_setop(SetOpCmd cmd, SetOpStrategy strategy, Plan 
> *lefttree,
> !List *distinctList, AttrNumber flagColIdx, int firstFlag,
> !long numGroups, double outputRows);
> ! extern Result *make_result(PlannerInfo *root, List *tlist,
> ! Node *resconstantqual, Plan *subplan);
> ! extern ModifyTable *make_modifytable(PlannerInfo *root,
> !  CmdType operation, bool canSetTag,
> !  Index nominalRelation,
> !  List *resultRelations, List *subplans,
> !  List *withCheckOptionLists, List 
> *returningLists,
> !  List *rowMarks, OnConflictExpr *onconflict, 
> int epqParam);
>   extern bool is_projection_capable_plan(Plan *plan);


I see that you made a lot of formerly externally visible make_* routines
static. The Citus extension (which will be open source in a few days)
uses several of these (make_agg, make_sort_from_sortclauses, make_limit
at least).

Can we please re-consider making these static?

It's fine if their parameter list changes from release to release,
that's easy enough to adjust to, and it's easily visible. But having to
copy all of these into extension code is more than a bit painful;
especially make_sort_from_sortclauses.

Greetings,

Andres Freund


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

Re: [HACKERS] Relation extension scalability

[Petr Jelinek]

On 10/03/16 09:57, Dilip Kumar wrote:


On Thu, Mar 10, 2016 at 7:55 AM, Petr Jelinek mailto:p...@2ndquadrant.com>> wrote:

Thanks for the comments..

Hmm, why did you remove the comment above the call to
UnlockRelationForExtension?

While re factoring I lose this comment.. Fixed it

It still seems relevant, maybe with some minor modification?

Also there is a bit of whitespace mess inside the conditional lock
block.

Fixed

I got the result of 10 mins run so posting it..
Note: Base code results are copied from up thread...

Results For 10 Mins run of COPY 1 records of size 4 bytes script and
configuration are same as used in up thread


ClientBasePatch
1105111
2217246
4210428
8166653
16  145808
32  124988
64---974


Results For 10 Mins run of INSERT 1000 records of size 1024 bytes(data
don't fits in shared buffer)
--

ClientBasePatch
1117120
2111126
4 51 130
8 43 147
16   40 209
32   ---  254
64   ---  205



Those look good. The patch looks good in general now. I am bit scared by 
the lockWaiters * 20 as it can result in relatively big changes in rare 
corner cases when for example a lot of backends were waiting for lock on 
relation and suddenly all try to extend it. I wonder if we should clamp 
it to something sane (although what's sane today might be small in 
couple of years).


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


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

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

[Peter Geoghegan]

On Thu, Mar 10, 2016 at 5:40 AM, Tomas Vondra
 wrote:
> I was thinking about running some benchmarks on this patch, but the
> thread is pretty huge so I want to make sure I'm not missing something
> and this is indeed the most recent version.

Wait 24 - 48 hours, please. Big update coming.


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

[Greg Stark]

On Thu, Mar 10, 2016 at 1:40 PM, Tomas Vondra 
wrote:
> I was thinking about running some benchmarks on this patch, but the
> thread is pretty huge so I want to make sure I'm not missing something
> and this is indeed the most recent version.

I also ran some preliminary benchmarks just before FOSDEM and intend to get
back to in after running different benchmarks. These are preliminary
because it was only a single run and on a machine that wasn't dedicated for
benchmarks. These were comparing the quicksort-all-runs patch against HEAD
at the time without the memory management optimizations which I think are
independent of the sort algorithm.

It looks to me like the interesting space to test is on fairly small
work_mem compared to the data size. There's a general slowdown on 4MB-8MB
work_mem when the data set is more than a gigabyte but but even in the
worst case it's only a 30% slowdown and the speedup in the more realistic
scenarios looks at least as big.




I want to rerun these on a dedicated machine and with trace_sort enabled so
that we can see how many merge passes were actually happening and how much
I/O was actually happening.

-- 
greg

Re: [HACKERS] Freeze avoidance of very large table.

[Masahiko Sawada]

On Fri, Mar 11, 2016 at 1:03 AM, Robert Haas  wrote:
> This 001 patch looks so little like what I was expecting that I
> decided to start over from scratch.  The new version I wrote is
> attached here.  I don't understand why your version tinkers with the
> logic for setting the all-frozen bit; I thought that what I already
> committed dealt with that already, and in any case, your version
> doesn't even compile against latest sources.  Your version also leaves
> the scan_all terminology intact even though it's not accurate any
> more, and I am not very convinced that the updates to the
> page-skipping logic are actually correct.  Please have a look over
> this version and see what you think.

Thank you for your advise.
Sorry, optimising logic of previous patch was old by mistake.
Attached latest patch incorporated your suggestions with a little revising.

>
> I think that's kind of pointless.  We need to test that this
> conversion code works, but once it does, I don't think we should make
> everybody pay the overhead of retesting that.  Anyway, the test code
> could have bugs, too.
>
> Here's an updated version of your patch with that code removed and
> some cosmetic cleanups like fixing typos and stuff like that.  I think
> this is mostly ready to commit, but I noticed one problem: your
> conversion code always produces two output pages for each input page
> even if one of them would be empty.  In particular, if you have a
> large number of small relations and run pg_upgrade, all of their
> visibility maps will go from 8kB to 16kB.  That isn't the end of the
> world, maybe, but I think you should see if you can't fix it
> somehow

Thank you for updating patch.
To deal with this problem, I've changed it so that pg_upgrade checks
file size before conversion.
And if fork file does not exist or size is 0 (empty), ignore.
Attached latest patch.

Regards,

--
Masahiko Sawada


001_optimize_vacuum_by_frozen_bit_v40.patch
Description: Binary data


000_pgupgrade_rewrite_vm_v42.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] Add generate_series(date,date) and generate_series(date,date,integer)

[Robert Haas]

On Thu, Mar 10, 2016 at 11:33 AM, David G. Johnston
 wrote:
> I tend to think we err toward this too much.  This seems like development
> concerns trumping usability.  Consider that anything someone took the time
> to write and polish to make committable to core was obviously genuinely
> useful to them - and for every person capable of actually taking things that
> far there are likely many more like myself who cannot but have encountered
> the, albeit minor, usability annoyance that this kind of function seeks to
> remove.

Sure, an individual function like this has almost no negative impact.
On the other hand, working around its absence is also trivial.  You
can create a wrapper function that does exactly this in a couple of
lines of SQL.  In my opinion, saying that people should do that in
they need it has some advantages over shipping it to everyone.  If you
don't have it and you want it, you can easily get it.  But what if you
have it and you don't want it, for example because what you really
want is a minimal postgres installation?  You can't take anything in
core back out again, or at least not easily.  Everything about core is
expanding very randomly - code size, shared memory footprint, all of
it.  If you think that has no downside for users, I respectfully
disagree.

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

[Tom Lane]

Andres Freund  writes:
> I see that you made a lot of formerly externally visible make_* routines
> static. The Citus extension (which will be open source in a few days)
> uses several of these (make_agg, make_sort_from_sortclauses, make_limit
> at least).

> Can we please re-consider making these static?

That was intentional: in my opinion, nothing outside createplan.c ought
to be making Plan nodes anymore.  The expectation is that you make a
Path describing what you want.  Can you explain why, in the new planner
structure, it would be sane to have external callers of these functions?

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] pg_rewind just doesn't fsync *anything*?

[Andres Freund]

Hi,

On 2016-03-10 08:47:16 +0100, Michael Paquier wrote:
> Still, I think that we had better fsync only entries that are modified
> by pg_rewind, and files that got updated, and not the whole directory

Why? If any files in there are dirty, they need to be fsynced. If
they're not dirty, fsync's free.


> a target data folder should be stopped properly to be able to rewind,
> and it is better to avoid dependencies between utilities if that's not
> strictly necessary.  initdb is likely to be installed side-by-side
> with pg_rewind in any distribution though.

It's not like we don't have any other such dependencies, in other
binaries. I'm not concerned.

Having to backpatch a single system() invocation + find_other_exec()
call, and backporting a more general FRONTEND version of initdb's
fsync_pgdata() are wildly differing in complexity.


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

[Robert Haas]

On Thu, Mar 10, 2016 at 12:18 AM, Amit Kapila  wrote:
> On Wed, Mar 9, 2016 at 7:17 PM, Robert Haas  wrote:
>>
>> On Wed, Mar 9, 2016 at 8:31 AM, Amit Kapila 
>> wrote:
>> > Thanks for the suggestion.  I have updated the patch to include
>> > wait_event_type information in the wait_event table.
>>
>> I think we should remove "a server process is" from all of these entries.
>>
>> Also, I think this kind of thing should be tightened up:
>>
>> + A server process is waiting on any one of the
>> commit_timestamp
>> + buffer locks to read or write the commit_timestamp page in the
>> + pg_commit_ts subdirectory.
>>
>> I'd just write: Waiting to read or write a commit timestamp buffer.
>>
>
> Okay, changed as per suggestion and fixed the morerows issue pointed by
> Thom.

Committed with some further editing.  In particular, the way you
determined whether we could safely access the tranche information for
any given ID was wrong; please check over what I did and make sure
that isn't also wrong.

Whew, this was a long process, but we got there.  Some initial pgbench
testing shows that by far the most common wait event observed on that
workload is WALWriteLock, which is pretty interesting: perf -e cs and
LWLOCK_STATS let you measure the most *frequent* wait events, but that
ignores duration.  Sampling pg_stat_activity tells you which things
you're spending the most *time* waiting for, which is awfully neat.

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

[Andres Freund]

On 2016-03-10 13:48:31 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > I see that you made a lot of formerly externally visible make_* routines
> > static. The Citus extension (which will be open source in a few days)
> > uses several of these (make_agg, make_sort_from_sortclauses, make_limit
> > at least).
> 
> > Can we please re-consider making these static?
> 
> That was intentional: in my opinion, nothing outside createplan.c ought
> to be making Plan nodes anymore.  The expectation is that you make a
> Path describing what you want.  Can you explain why, in the new planner
> structure, it would be sane to have external callers of these functions?

In Citus' case a full PlannedStmt is generated on the master node, to
combine the data generated on worker nodes (where the bog standard
postgres planner is used).  It's not the only way to do things, but I
don't see why the approach would be entirely invalidated by the
pathification work.

We do provide the planner_hook, so restricting the toolbox for that to
do something useful, doesn't seem like a necessarily good idea.

- Andres


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