Re: [HACKERS] SCRAM authentication, take three

[Michael Paquier]

On Tue, Feb 7, 2017 at 11:28 AM, Michael Paquier
 wrote:
> Yes, I am actively working on this one now. I am trying to come up
> first with something in the shape of an extension to begin with, and
> get a patch out of it. That will be more simple for testing. For now
> the work that really remains in the patches attached on this thread is
> to get the internal work done, all the UTF8-related routines being
> already present in scram-common.c to work on the strings.

It took me a couple of days... And attached is the prototype
implementing SASLprep(), or NFKC if you want for UTF-8 strings. This
extension is pretty useful to check the validity of any normalization
forms defined in the Unicode specs, and is as well on my github:
https://github.com/michaelpq/pg_plugins/tree/master/pg_sasl_prepare

In short, at build what this extension does is fetching
UnicodeData.txt, and it builds a conversion table including the fields
necessary for NFKC:
- The utf code of a character.
- The combining class of the character.
- The decomposition sets of characters.
I am aware of the fact that the implemention of this extension is the
worst possible, there are many bytes wasted, and the resulting shared
library gets at 2.4MB. Now as an example of how normalization forms
work that's a great study, and with that I am now aware of what needs
to be done to reduce the size of the conversion tables.

This extension has two conversion functions for UTF-8 string <=>
integer array (as UTF-8 characters are on 4 bytes with pg_wchar):
=# SELECT array_to_utf8('{50064}');
 array_to_utf8
---
 Ð
(1 row)
=# SELECT utf8_to_array('ÐÐÐ');
utf8_to_array
-
 {50064,50064,50064}
(1 row)

Then using an integer array in input SASLprep() can be done using
pg_sasl_prepare(), which returns to caller a decomposed recursively
set, with reordering done using the combining class integer array from
the conversion table generated wiht UnicodeData.txt. Lookups at the
conversion tables are done using bsearch(), so that's, at least I
guess, fast.

I have implemented as well a function to query the whole conversion as
a SRF (character number, combining class and decomposition), which is
useful for analysis:
=# select count(*) from utf8_conv_table();
 count
---
 30590
(1 row)

Now using this module I have arrived to the following conclusions to
put to a minimum the size of the conversion tables, without much
impacting lookup performance:
- There are 24k characters with a combining class of 0 and no
decomposition for a total of 30k characters, those need to be dropped
from the conversion table.
- Most characters have a single, or double decomposition, one has a
decomposition of 18 characters. So we need to create two sets of
conversion tables:
-- A base table, with the character number (4 bytes), the combining
class (1 byte) and the size of the decomposition (1 byte).
-- A set of decomposition tables, classified by size.
So when decomposing a character, we check first the size of the
decomposition, then get the set from the correct table.

Now regarding the shape of the implementation for SCRAM, we need one
thing: a set of routines in src/common/ to build decompositions for a
given UTF-8 string with conversion UTF8 string <=> pg_wchar array, the
decomposition and the reordering. The extension attached roughly
implements that. What we can actually do as well is have in contrib/ a
module that does NFK[C|D] using the base APIs in src/common/. Using
arrays of pg_wchar (integers) to manipulate the characters, we can
validate and have a set of regression tests that do *not* have to
print non-ASCII characters. Let me know if this plan looks good, now I
think that I have enough material to get SASLprep done cleanly, with a
minimum memory footprint.

Heikki, others, how does that sound for you?
-- 
Michael


pg_sasl_prepare.tar.gz
Description: GNU Zip compressed data

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

Re: [HACKERS] Proposal : For Auto-Prewarm.

[Peter Geoghegan]

On Tue, Feb 7, 2017 at 1:31 AM, Mithun Cy  wrote:
> SEGFAULT was the coding mistake I have called the C-language function
> directly without initializing the functioncallinfo. Thanks for
> raising. Below patch fixes same.

It would be nice if this had an option to preload only internal B-tree
pages into shared_buffers. They're usually less than 1% of the total
pages in a B-Tree, and are by far the most frequently accessed. It's
reasonable to suppose that much of the random I/O incurred when
warming the cache occurs there. Now, prewarming those will incur
random I/O, often completely random I/O, but by and large it would be
a matter of swallowing that cost sooner, through using your tool,
rather than later, during the execution of queries.

-- 
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] Proposal : For Auto-Prewarm.

[Beena Emerson]

Hello,

On Wed, Feb 8, 2017 at 3:40 PM, Amit Kapila  wrote:

> On Tue, Feb 7, 2017 at 5:14 PM, Mithun Cy 
> wrote:
> > On Tue, Feb 7, 2017 at 12:24 PM, Amit Kapila 
> wrote:
> >> On Tue, Feb 7, 2017 at 11:53 AM, Beena Emerson 
> wrote:
> >>> Are 2 workers required?
> >>>
> >>
> >> I think in the new design there is a provision of launching the worker
> >> dynamically to dump the buffers, so there seems to be a need of
> >> separate workers for loading and dumping the buffers.  However, there
> >> is no explanation in the patch or otherwise when and why this needs a
> >> pair of workers.  Also, if the dump interval is greater than zero,
> >> then do we really need to separately register a dynamic worker?
> >
> > We have introduced a new value  -1 for pg_prewarm.dump_interval this
> > means we will not dump at all, At this state, I thought auto
> > pg_prewarm process need not run at all, so I coded to exit the auto
> > pg_prewarm immediately. But If the user decides to start the auto
> > pg_prewarm to dump only without restarting the server, I have
> > introduced a launcher function "launch_pg_prewarm_dump" to restart the
> > auto pg_prewarm only to dump. Since now we can launch worker only to
> > dump, I thought we can redistribute the code between two workers, one
> > which only does prewarm (load only) and another dumps periodically.
> > This helped me to modularize and reuse the code. So once load worker
> > has finished its job, it registers a dump worker and then exists.
> > But if max_worker_processes is not enough to launch the "auto
> > pg_prewarm dump" bgworker
> > We throw an error
> > 2017-02-07 14:51:59.789 IST [50481] ERROR:  registering dynamic
> > bgworker "auto pg_prewarm dump" failed c
> > 2017-02-07 14:51:59.789 IST [50481] HINT:  Consider increasing
> > configuration parameter "max_worker_processes".
> >
> > Now thinking again instead of such error and then correcting same by
> > explicitly launching the auto pg_prewarm dump bgwroker through
> > launch_pg_prewarm_dump(), I can go back to original design where there
> > will be one worker which loads and then dumps periodically. And
> > launch_pg_prewarm_dump will relaunch dump only activity of that
> > worker. Does this sound good?
> >
>
> Won't it be simple if you consider -1 as a value to just load library?
>  For *_interval = -1, it will neither load nor dump.
>
>
+1
That is what I thought was the behaviour we decided upon for -1.



-- 
Thank you,

Beena Emerson

Have a Great Day!

Re: [HACKERS] Parallel Index Scans

[Peter Geoghegan]

On Wed, Feb 8, 2017 at 10:33 PM, Amit Kapila  wrote:
> I had some offlist discussion with Robert about the above point and we
> feel that keeping only heap pages for parallel computation might not
> be future proof as for parallel index only scans there might not be
> any heap pages.  So, it is better to use separate GUC for parallel
> index (only) scans.  We can have two guc's
> min_parallel_table_scan_size (8MB) and min_parallel_index_scan_size
> (512kB) for computing parallel scans.  The parallel sequential scan
> and parallel bitmap heap scans can use min_parallel_table_scan_size as
> a threshold to compute parallel workers as we are doing now.  For
> parallel index scans, both min_parallel_table_scan_size and
> min_parallel_index_scan_size can be used for threshold;  We can
> compute parallel workers both based on heap_pages to be scanned and
> index_pages to be scanned and then keep the minimum of those.  This
> will help us to engage parallel index scans when the index pages are
> lower than threshold but there are many heap pages to be scanned and
> will also allow keeping a maximum cap on the number of workers based
> on index scan size.

What about parallel CREATE INDEX? The patch currently uses
min_parallel_relation_size as an input into the optimizer's custom
cost model. I had wondered if that made sense. Note that another such
input is the projected size of the final index. That's the thing that
increases at logarithmic intervals as there is a linear increase in
the number of workers assigned to the operation (so it's not the size
of the underlying table).

-- 
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] Parallel Index Scans

[Amit Kapila]

On Sat, Feb 4, 2017 at 7:14 AM, Amit Kapila  wrote:
> On Sat, Feb 4, 2017 at 5:54 AM, Robert Haas  wrote:
>> On Wed, Feb 1, 2017 at 12:58 AM, Amit Kapila  wrote:
>
>> On balance, I'm somewhat inclined to think that we ought to base
>> everything on heap pages, so that we're always measuring in the same
>> units.  That's what Dilip's patch for parallel bitmap heap scan does,
>> and I think it's a reasonable choice.  However, for parallel index
>> scan, we might want to also cap the number of workers to, say,
>> index_pages/10, just so we don't pick an index scan that's going to
>> result in a very lopsided work distribution.
>>
>
> I guess in the above context you mean heap_pages or index_pages that
> are expected to be *fetched* during index scan.
>
> Yet another thought is that for parallel index scan we use
> index_pages_fetched, but use either a different GUC
> (min_parallel_index_rel_size) with a relatively lower default value
> (say equal to min_parallel_relation_size/4 = 2MB) or directly use
> min_parallel_relation_size/4 for parallel index scans.
>

I had some offlist discussion with Robert about the above point and we
feel that keeping only heap pages for parallel computation might not
be future proof as for parallel index only scans there might not be
any heap pages.  So, it is better to use separate GUC for parallel
index (only) scans.  We can have two guc's
min_parallel_table_scan_size (8MB) and min_parallel_index_scan_size
(512kB) for computing parallel scans.  The parallel sequential scan
and parallel bitmap heap scans can use min_parallel_table_scan_size as
a threshold to compute parallel workers as we are doing now.  For
parallel index scans, both min_parallel_table_scan_size and
min_parallel_index_scan_size can be used for threshold;  We can
compute parallel workers both based on heap_pages to be scanned and
index_pages to be scanned and then keep the minimum of those.  This
will help us to engage parallel index scans when the index pages are
lower than threshold but there are many heap pages to be scanned and
will also allow keeping a maximum cap on the number of workers based
on index scan size.

guc_parallel_index_scan_v1.patch - Change name of existing
min_parallel_relation_size to min_parallel_table_scan_size and added a
new guc min_parallel_index_scan_size with default value of 512kB.
This patch also adjusted the computation in compute_parallel_worker
based on two guc's.

compute_index_pages_v2.patch - This function extracts the computation
of index pages to be scanned in a separate function and used it in
existing code.  You will notice that I have pulled up the logic of
conversion of clauses to indexquals from create_index_path to
build_index_paths as that is required to compute the number of index
and heap pages to be scanned by scan in patch
parallel_index_opt_exec_support_v8.patch.  This doesn't impact any
existing functionality.

parallel_index_scan_v7 - patch to parallelize btree scans, nothing is
changed from previous version (just rebased on latest head).

parallel_index_opt_exec_support_v8.patch - This contain changes to
compute parallel workers using both heap and index pages that need to
be scanned.

Patches guc_parallel_index_scan_v1.patch and
compute_index_pages_v2.patch are independent patches.  Both the
patches are required by parallel index scan patches.

The current set of patches handles all the reported comments.

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


guc_parallel_index_scan_v1.patch
Description: Binary data


compute_index_pages_v2.patch
Description: Binary data


parallel_index_scan_v7.patch
Description: Binary data


parallel_index_opt_exec_support_v8.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] Declarative partitioning - another take

[amul sul]

About 0001-Check-partition-strategy-in-ATExecDropNotNull.patch,
following test is already covered in alter_table.sql @ Line # 1918,
instead of this kindly add test for list_partition:

 77 +-- cannot drop NOT NULL constraint of a range partition key column
 78 +ALTER TABLE range_parted ALTER a DROP NOT NULL;
 79 +

Regards,
Amul


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

Re: [HACKERS] WAL consistency check facility

[Kuntal Ghosh]

On Thu, Feb 9, 2017 at 2:26 AM, Robert Haas  wrote:
> On Wed, Feb 8, 2017 at 1:25 AM, Kuntal Ghosh  
> wrote:
>> Thank you Robert for the review. Please find the updated patch in the
>> attachment.
>
> I have committed this patch after fairly extensive revisions:
>
Thank you, Robert, for the above corrections and commit. Thanks to
Michael Paquier, Peter Eisentraut, Amit Kapila, Álvaro Herrera, and
Simon Riggs for taking their time to complete the patch. It was a
great learning experience for me.

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: 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

[HACKERS] CREATE COLLATION IF NOT EXISTS

[Peter Eisentraut]

Here is a patch to complete the implementation of CREATE COLLATION IF
NOT EXISTS.  The meat of this was already implemented for
pg_import_system_collations; this just exposes it in the SQL command.

If we go ahead with ICU, then creating collations by hand will become
more common, so this could be useful in practice.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From aa6f7563b0893961bbc5d82c8496c2bbea990f3c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 8 Feb 2017 22:51:09 -0500
Subject: [PATCH] Add CREATE COLLATION IF NOT EXISTS clause

The core of the functionality was already implemented when
pg_import_system_collations was added.  This just exposes it as an
option in the SQL command.
---
 doc/src/sgml/ref/create_collation.sgml   | 15 +--
 src/backend/commands/collationcmds.c |  4 ++--
 src/backend/nodes/copyfuncs.c|  1 +
 src/backend/nodes/equalfuncs.c   |  1 +
 src/backend/parser/gram.y| 20 
 src/backend/tcop/utility.c   |  3 ++-
 src/include/commands/collationcmds.h |  2 +-
 src/include/nodes/parsenodes.h   |  1 +
 src/test/regress/expected/collate.linux.utf8.out |  4 
 src/test/regress/sql/collate.linux.utf8.sql  |  2 ++
 10 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/doc/src/sgml/ref/create_collation.sgml b/doc/src/sgml/ref/create_collation.sgml
index d757cdfb43..c09e5bd6d4 100644
--- a/doc/src/sgml/ref/create_collation.sgml
+++ b/doc/src/sgml/ref/create_collation.sgml
@@ -18,12 +18,12 @@
 
  
 
-CREATE COLLATION name (
+CREATE COLLATION [ IF NOT EXISTS ] name (
 [ LOCALE = locale, ]
 [ LC_COLLATE = lc_collate, ]
 [ LC_CTYPE = lc_ctype ]
 )
-CREATE COLLATION name FROM existing_collation
+CREATE COLLATION [ IF NOT EXISTS ] name FROM existing_collation
 
  
 
@@ -48,6 +48,17 @@ Parameters
 

 
+ IF NOT EXISTS
+ 
+  
+   Do not throw an error if a collation with the same name already exists.
+   A notice is issued in this case. Note that there is no guarantee that
+   the existing collation is anything like the one that would have been created.
+  
+ 
+
+
+
  name
 
  
diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c
index e165d4b2a6..919cfc6a06 100644
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -37,7 +37,7 @@
  * CREATE COLLATION
  */
 ObjectAddress
-DefineCollation(ParseState *pstate, List *names, List *parameters)
+DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_exists)
 {
 	char	   *collName;
 	Oid			collNamespace;
@@ -137,7 +137,7 @@ DefineCollation(ParseState *pstate, List *names, List *parameters)
 			 GetDatabaseEncoding(),
 			 collcollate,
 			 collctype,
-			 false);
+			 if_not_exists);
 
 	if (!OidIsValid(newoid))
 		return InvalidObjectAddress;
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 30d733e57a..ab96d71a8f 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3104,6 +3104,7 @@ _copyDefineStmt(const DefineStmt *from)
 	COPY_NODE_FIELD(defnames);
 	COPY_NODE_FIELD(args);
 	COPY_NODE_FIELD(definition);
+	COPY_SCALAR_FIELD(if_not_exists);
 
 	return newnode;
 }
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 55c73b7292..94789feb08 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1210,6 +1210,7 @@ _equalDefineStmt(const DefineStmt *a, const DefineStmt *b)
 	COMPARE_NODE_FIELD(defnames);
 	COMPARE_NODE_FIELD(args);
 	COMPARE_NODE_FIELD(definition);
+	COMPARE_SCALAR_FIELD(if_not_exists);
 
 	return true;
 }
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index cf97be512d..7c025f247d 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -5606,6 +5606,16 @@ DefineStmt:
 	n->definition = $4;
 	$$ = (Node *)n;
 }
+			| CREATE COLLATION IF_P NOT EXISTS any_name definition
+{
+	DefineStmt *n = makeNode(DefineStmt);
+	n->kind = OBJECT_COLLATION;
+	n->args = NIL;
+	n->defnames = $6;
+	n->definition = $7;
+	n->if_not_exists = true;
+	$$ = (Node *)n;
+}
 			| CREATE COLLATION any_name FROM any_name
 {
 	DefineStmt *n = makeNode(DefineStmt);
@@ -5615,6 +5625,16 @@ DefineStmt:
 	n->definition = list_make1(makeDefElem("from", (Node *) $5, @5));
 	$$ = (Node *)n;
 }
+			| CREATE COLLATION IF_P NOT EXISTS any_name FROM any_name
+{
+	DefineStmt *n = makeNode(DefineStmt);
+	n->kind = OBJECT_COLLATION;
+	n->args = NIL;
+	n->defnames = $6;
+	n->definition = list_make1(makeDefElem("from", (Node *) $8, @8));
+	n->if_not_exists = true;
+	$$ = 

Re: [HACKERS] Declarative partitioning - another take

[Amit Langote]

On 2017/02/08 21:20, amul sul wrote:
> Regarding following code in ATExecDropNotNull function, I don't see
> any special check for RANGE partitioned, is it intended to have same
> restriction for LIST partitioned too, I guess not?
> 
>   /*
>  * If the table is a range partitioned table, check that the column is not
>  * in the partition key.
>  */
> if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> {
> PartitionKey key = RelationGetPartitionKey(rel);
> int partnatts = get_partition_natts(key),
> i;
> 
> for (i = 0; i < partnatts; i++)
> {
> AttrNumber  partattnum = get_partition_col_attnum(key, i);
> 
> if (partattnum == attnum)
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
>  errmsg("column \"%s\" is in range partition key",
> colName)));
> }
> }

Good catch!  Seems like an oversight, attached fixes it.

Thanks,
Amit
>From b84ac63b6b4acd19a09e8507cf199db127c06d71 Mon Sep 17 00:00:00 2001
From: amit 
Date: Thu, 9 Feb 2017 10:31:58 +0900
Subject: [PATCH] Check partition strategy in ATExecDropNotNull

We should prevent dropping the NOT NULL constraint on a column only
if the column is in the *range* partition key (as the error message
also says).  Add a regression test while at it.

Reported by: Amul Sul
Patch by: Amit Langote
Report: https://postgr.es/m/CAAJ_b95g5AgkpJKbLajAt8ovKub874-B9X08PiOqHvVfMO2mLw%40mail.gmail.com
---
 src/backend/commands/tablecmds.c  | 22 +-
 src/test/regress/expected/alter_table.out |  3 +++
 src/test/regress/sql/alter_table.sql  |  3 +++
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 37a4c4a3d6..f33aa70da6 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5593,18 +5593,22 @@ ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode)
 	if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 	{
 		PartitionKey key = RelationGetPartitionKey(rel);
-		int			partnatts = get_partition_natts(key),
-	i;
 
-		for (i = 0; i < partnatts; i++)
+		if (get_partition_strategy(key) == PARTITION_STRATEGY_RANGE)
 		{
-			AttrNumber	partattnum = get_partition_col_attnum(key, i);
+			int			partnatts = get_partition_natts(key),
+		i;
 
-			if (partattnum == attnum)
-ereport(ERROR,
-		(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
-		 errmsg("column \"%s\" is in range partition key",
-colName)));
+			for (i = 0; i < partnatts; i++)
+			{
+AttrNumber	partattnum = get_partition_col_attnum(key, i);
+
+if (partattnum == attnum)
+	ereport(ERROR,
+			(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
+			 errmsg("column \"%s\" is in range partition key",
+	colName)));
+			}
 		}
 	}
 
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index d8e7b61294..e6d45fbf9c 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3330,6 +3330,9 @@ ALTER TABLE list_parted2 DROP COLUMN b;
 ERROR:  cannot drop column named in partition key
 ALTER TABLE list_parted2 ALTER COLUMN b TYPE text;
 ERROR:  cannot alter type of column named in partition key
+-- cannot drop NOT NULL constraint of a range partition key column
+ALTER TABLE range_parted ALTER a DROP NOT NULL;
+ERROR:  column "a" is in range partition key
 -- cleanup: avoid using CASCADE
 DROP TABLE list_parted, part_1;
 DROP TABLE list_parted2, part_2, part_5, part_5_a;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 1f551ec53c..12edb8b3ba 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2189,6 +2189,9 @@ ALTER TABLE part_2 INHERIT inh_test;
 ALTER TABLE list_parted2 DROP COLUMN b;
 ALTER TABLE list_parted2 ALTER COLUMN b TYPE text;
 
+-- cannot drop NOT NULL constraint of a range partition key column
+ALTER TABLE range_parted ALTER a DROP NOT NULL;
+
 -- cleanup: avoid using CASCADE
 DROP TABLE list_parted, part_1;
 DROP TABLE list_parted2, part_2, part_5, part_5_a;
-- 
2.11.0


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

Re: [HACKERS] Logical replication existing data copy

[Erik Rijkers]

On 2017-02-08 23:25, Petr Jelinek wrote:


0001-Use-asynchronous-connect-API-in-libpqwalreceiver-v2.patch
0002-Always-initialize-stringinfo-buffers-in-walsender-v2.patch
0003-Fix-after-trigger-execution-in-logical-replication-v2.patch
0004-Add-RENAME-support-for-PUBLICATIONs-and-SUBSCRIPTION-v2.patch
0001-Logical-replication-support-for-initial-data-copy-v4.patch


test 'object_address' fails, see atachment.

That's all I found in a quick first trial.

thanks,

Erik Rijkers




*** /home/aardvark/pg_stuff/pg_sandbox/pgsql.logical_replication/src/test/regress/expected/object_address.out	2017-02-09 00:51:30.345519608 +0100
--- /home/aardvark/pg_stuff/pg_sandbox/pgsql.logical_replication/src/test/regress/results/object_address.out	2017-02-09 00:54:11.884715532 +0100
***
*** 38,43 
--- 38,45 
  	TO SQL WITH FUNCTION int4recv(internal));
  CREATE PUBLICATION addr_pub FOR TABLE addr_nsp.gentable;
  CREATE SUBSCRIPTION addr_sub CONNECTION '' PUBLICATION bar WITH (DISABLED, NOCREATE SLOT);
+ ERROR:  could not connect to the publisher: FATAL:  no pg_hba.conf entry for replication connection from host "[local]", user "aardvark", SSL off
+ 
  -- test some error cases
  SELECT pg_get_object_address('stone', '{}', '{}');
  ERROR:  unrecognized object type "stone"
***
*** 409,463 
  			pg_identify_object_as_address(classid, objid, subobjid) ioa(typ,nms,args),
  			pg_get_object_address(typ, nms, ioa.args) as addr2
  	ORDER BY addr1.classid, addr1.objid, addr1.subobjid;
!type|   schema   |   name|   identity   | ?column? 
! ---++---+--+--
!  default acl   ||   | for role regress_addr_user in schema public on tables| t
!  default acl   ||   | for role regress_addr_user on tables | t
!  type  | pg_catalog | _int4 | integer[]| t
!  type  | addr_nsp   | gencomptype   | addr_nsp.gencomptype | t
!  type  | addr_nsp   | genenum   | addr_nsp.genenum | t
!  type  | addr_nsp   | gendomain | addr_nsp.gendomain   | t
!  function  | pg_catalog |   | pg_catalog.pg_identify_object(pg_catalog.oid,pg_catalog.oid,integer) | t
!  aggregate | addr_nsp   |   | addr_nsp.genaggr(integer)| t
!  sequence  | addr_nsp   | gentable_a_seq| addr_nsp.gentable_a_seq  | t
!  table | addr_nsp   | gentable  | addr_nsp.gentable| t
!  table column  | addr_nsp   | gentable  | addr_nsp.gentable.b  | t
!  index | addr_nsp   | gentable_pkey | addr_nsp.gentable_pkey   | t
!  view  | addr_nsp   | genview   | addr_nsp.genview | t
!  materialized view | addr_nsp   | genmatview| addr_nsp.genmatview  | t
!  foreign table | addr_nsp   | genftable | addr_nsp.genftable   | t
!  foreign table column  | addr_nsp   | genftable | addr_nsp.genftable.a | t
!  role  || regress_addr_user | regress_addr_user| t
!  server|| addr_fserv| addr_fserv   | t
!  user mapping  ||   | regress_addr_user on server integer  | t
!  foreign-data wrapper  || addr_fdw  | addr_fdw | t
!  access method || btree | btree| t
!  operator of access method ||   | operator 1 (integer, integer) of pg_catalog.integer_ops USING btree  | t
!  function of access method ||   | function 2 (integer, integer) of pg_catalog.integer_ops USING btree  | t
!  default value |   

Re: [HACKERS] Backport of pg_statistics typos fix

[Yugo Nagata]

On Wed, 8 Feb 2017 14:54:17 -0500
Robert Haas  wrote:

> On Tue, Feb 7, 2017 at 4:22 AM, Yugo Nagata  wrote:
> > I found typos "pg_statistics" in REL9_6_STABLE, but that has been
> > fixed in the master branch.
> >
> > Fix typo: pg_statistics -> pg_statistic
> > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=5a366b4ff4ceceb9793fcc13c3f097ee0d32c56d;hp=f7c62462402972b13d10e43f104ca0c0fecb6d08
> >
> > I think it would be better to backport this to other branches.
> 
> We usually leave such decisions to the discretion of the committer,
> because back-porting such changes takes time and sometimes it just
> isn't that important.  Nobody's likely to be confused by a few
> instances of writing pg_statistics rather than pg_statistic.
> Personally, I favor not back-porting such things in most cases,
> because I think patches that get back-ported should be strictly
> limited to bug fixes, and typos in code comments aren't bug fixes.
> But not everyone has the same opinion on this.  What's your reason for
> wanting it back-ported?

I agree typos in code comments aren't bug fixes and not need
to get back-ported. However, there are typos also in the document.

The scalarltsel function retrieves the histogram for
unique1 from
-   pg_statistics.  For manual queries it is more
+   pg_statistic.  For manual queries it is more
convenient to look in the simpler pg_stats
view:

I think this might be a document bug, but if nobody cares of it,
I also don't mind. 

Thanks,

> 
> BTW, looking at that commit, this change looks to have adjusted this
> from being wrong to still being wrong:
> 
> -Allow pg_statistics to be reset by calling
> pg_stat_reset() (Christopher)
> +Allow pg_statistic to be reset by calling
> pg_stat_reset() (Christopher)
> 
> It's true that pg_stat_reset() doesn't reset the nonexistent
> pg_statistics table.  But it doesn't reset pg_statistic either.  IIUC,
> it resets the data gathered by the statistics collector, which is
> something else altogether.
> 
> -- 
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company


-- 
Yugo Nagata 


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

Re: [HACKERS] Improve OR conditions on joined columns.

[Tom Lane]

Jim Nasby  writes:
> AFAICT this can be transformed into a UNION (not all) if dim.id is 
> unique. Does the upper planner pathification make this any easier? 

What I did in 9.6 is a first step.  The next step, I think, is to
replace prepunion.c with something that can consider more than one
implementation path for a union.

Although ... actually, that may not be the bottleneck for what you're
after.  The issue here is not so much discovering a clever plan for
a union as realizing that the query could be cast as a union in the
first place.

Maybe it'd be better to imagine this as something closer to planagg.c,
that is it knows how to apply a specific high-level optimization that
might or might not be a win, so it builds a path describing that and sees
if it looks cheaper than a path done the normal way.  The fact that
we could even build a path describing a union is something that wasn't
there before 9.6, but maybe there's enough infrastructure for that now.

> There's another transform using arrays that's possible as well (see 
> attached example); I believe that would work regardless of uniqueness.

That doesn't look terribly promising for automated application.
And I think it's really dependent on the exact shape of the OR
clause, which is an unpleasant limitation.  Considering the amount
of work this will take to do at all, you'd want it to be pretty
general I think.  I'm imagining something that would look for an
OR in which each clause referenced only one rel, then if it can
identify a way to re-unique-ify the result, split into a UNION
with an arm for each rel used in the OR.  The nature of the
conditions in each OR arm don't seem to matter ... though probably
you'd have to insist on there not being volatile conditions anywhere
in the query, because they'd get evaluated too many times.

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] Press Release Draft - 2016-02-09 Cumulative Update

[Jim Nasby]

On 2/7/17 9:37 AM, Jonathan S. Katz wrote:

Below is the draft of the press release for the update this Thursday:


Thanks for the work on this!

  11 There existed a race condition if CREATE INDEX CONCURRENTLY was 
called on a column that had not been indexed before, then rows that were 
updated by transactions running at the same time as the CREATE INDEX 
CONCURRENTLY command could have been indexed incorrectly.


I think that'd read better as

  11 There existed a race condition /where/ if CREATE INDEX 
CONCURRENTLY was called on a column that had not been indexed before, 
then rows that were updated by transactions running at the same time as 
the CREATE INDEX CONCURRENTLY command /may not/ have been indexed 
incorrectly.


Also, maybe we should mention that there's no way to test for this, and 
make a stronger suggestion to redo any affected indexes?


  20 These release contains several fixes to improve the stability of 
visible data and WAL logging that we wish to highlight here.


I think this sentence can just go. If we want to keep it, IMHO this is a 
better alternative: "This release contains several improvements to the 
stability of data visibility and WAL logging."


  22 Prior to this release, data could be prematurely pruned by a 
vacuum operation when a special snapshot used for catalog scans was 
presently available.


... vacuum operation even though a special catalog scan snapshot was in use.

BTW, I don't know what came out of the discussion of git references in 
release notes, but I'd find it useful to be able to at least get a 
complete list. Not hard for me to do that since I know git and our 
naming scheme, but maybe we should include directions for doing so?

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


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

Re: [HACKERS] Logical replication existing data copy

[Petr Jelinek]

Hi,

here is updated patch.

Note that it's rebased on top of logical replication improvements
patches [1] (which still apply fine to my surprise).

It will probably need another rebase once patches from Masahiko Sawada
and Fujii Masao get in.

[1]
https://www.postgresql.org/message-id/42655eb4-6b2e-b35b-c184-509efd6eba10%402ndquadrant.com

-- 
  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] contrib modules and relkind check

[Corey Huinker]

On Mon, Feb 6, 2017 at 4:01 AM, Amit Langote 
wrote:

> On 2017/01/24 15:35, Amit Langote wrote:
> > On 2017/01/24 15:11, Michael Paquier wrote:
> >> On Tue, Jan 24, 2017 at 2:14 PM, Amit Langote
> >>  wrote:
> >>> Some contrib functions fail to fail sooner when relations of
> unsupported
> >>> relkinds are passed, resulting in error message like one below:
> >>>
> >>> create table foo (a int);
> >>> create view foov as select * from foo;
> >>> select pg_visibility('foov', 0);
> >>> ERROR:  could not open file "base/13123/16488": No such file or
> directory
> >>>
> >>> Attached patch fixes that for all such functions I could find in
> contrib.
> >>>
> >>> It also installs RELKIND_PARTITIONED_TABLE as unsupported in a couple
> of
> >>> places (in pageinspect and pgstattuple).
> >>
> >> I have spent some time looking at your patch, and did not find any
> >> issues with it, nor did I notice code paths that were not treated or
> >> any other contrib modules sufferring from the same deficiencies that
> >> you may have missed. Nice work.
> >
> > Thanks for the review, Michael!
>
> Added to the next CF, just so someone can decide to pick it up later.
>
> https://commitfest.postgresql.org/13/988/
>
> Thanks,
> Amit
>

Is this still needing a reviewer? If so, here it goes:

Patch applies.

make check-pgstattuple-recurse, check-pg_visibility-recurse,
check-pageinspect-recurse
all pass.

Code is quite clear. It does raise two questions:

1. should have these tests named in core functions, like maybe:

relation_has_visibility(Relation)
relation_has_storage(Relation)
and/or corresponding void functions check_relation_has_visibility() and
check_relation_has_storage() which would raise the appropriate error
message when the boolean test fails.
Then again, this might be overkill since we don't add new relkinds very
often.

2. Is it better stylistically to have an AND-ed list of != tests, or a
negated list of OR-ed equality tests, and should the one style be changed
to conform to the other?

No new regression tests. I think we should create a dummy partitioned table
for each contrib and show that it fails.

Re: [HACKERS] Press Release Draft - 2016-02-09 Cumulative Update

[Jim Nasby]

On 2/8/17 2:51 PM, Alvaro Herrera wrote:

I always have a bit of mixed feelings with these kind of string
manipulations on dynamic SQL.

It may look a bit nasty, but locking tables for long periods (or being
without an important index for a period) is much worse in production
scenarios.


I think posting a recipe in the wiki is a great idea (especially if it 
handles corner cases like constraints). I'm not so keen on trying to 
code it entirely in psql though. I think it'd be a lot cleaner to have a 
plpgsql function that uses format() to construct the appropriate 
commands to run and then spit that out as text. Users can either cut and 
paste that, or they can \gset it to a variable that they then execute, 
or they can capture the output to a file which they execute.


The big advantage to this is by default you see what commands would be 
run, but you can still fully automate if you want to without much extra 
effort.

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


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

[HACKERS] Improve OR conditions on joined columns.

[Jim Nasby]

I've a client interested enough in $SUBJECT that they're willing to 
offer a bounty on it. An example of the pain is (working example attached):


create temp view denorm as
select f.*, d1.t t1, d2.t t2
from fact f
left join dim d1 on f1=d1.id
left join dim d2 on f2=d2.id
;

-- Fast
explain analyze select count(*) from denorm where 1 in (f1,f2);
explain analyze select count(*) from denorm where '1' in (t1);

-- Slow
explain analyze select count(*) from denorm where '1' in (t1,t2);

They currently work around this by doing a union:

select ... from denorm where t1 = '1'
union
select ... from denorm where t2 = '1'

... or depending on needs using IN instead of =.

AFAICT this can be transformed into a UNION (not all) if dim.id is 
unique. Does the upper planner pathification make this any easier? 
There's another transform using arrays that's possible as well (see 
attached example); I believe that would work regardless of uniqueness.


Just to be clear; the OR by itself is not a problem (as shown by the 
first fast query); it's the OR with the JOIN that's a problem.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)
create temp table fact as select generate_series(1,999) f1, 
generate_series(1,999) f2;
insert into fact select s,null from generate_series(1,999) s;
insert into fact select null,s from generate_series(1,999) s;
create index f_f1 on fact(f1);
create index f_f2 on fact(f2);
analyze fact;

create temp table dim as select s, s::text t from generate_series(1,999) s;
alter table dim add primary key(s), add unique (t);
create temp view denorm as
select f.*, d1.t t1, d2.t t2, array[f1,f2] ident_ids
from fact f
left join dim d1 on f1=d1.s
left join dim d2 on f2=d2.s
;

-- Fast
explain analyze select count(*) from denorm where 1 in (f1,f2);
explain analyze select count(*) from denorm where '1' in (t1);

-- Slow
explain analyze select count(*) from denorm where '1' in (t1,t2);

CREATE FUNCTION ident_ids(
idents text[]
) RETURNS int[] STABLE LANGUAGE sql AS $$
SELECT array( SELECT s FROM dim WHERE t = ANY(idents) )
$$;
CREATE FUNCTION ident_ids(
idents text
) RETURNS int[] STABLE LANGUAGE sql AS $$
SELECT ident_ids( ('{' || idents || '}')::text[] )
$$;

explain analyze select count(*) from denorm where ident_ids && 
ident_ids('42,84,128');

\echo ERROR OK here on version >= 10
create index fact__f_array on fact using gin ((array[f1,f2]) _int4_ops); -- pre 
10.0
\echo ERROR OK here on version < 10
create index fact__f_array on fact using gin ((array[f1,f2]) array_ops); -- 10.0

explain analyze select count(*) from denorm where ident_ids && 
ident_ids('42,84,128');

-- 
Sent 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: About CMake v2

[Andres Freund]

Hi,

On 2017-02-08 16:52:19 -0500, Tom Lane wrote:
> For my own purposes, the only thing that I find seriously annoying about
> the status quo is the amount of time required to run "configure".  For
> me, that step is usually comparable to or even more than the time to
> do the build proper, because (a) ccache and (b) multiple CPUs.
> configure isn't parallelizable, and there's probably nothing that
> can be done about that.

I use autoconf caching feature to make that a bit less painful (plus
some scripting about when to scrap the cache file...).  I find that
seriously annoying too.


> If CMake offers a substantial improvement
> in configuration time then that would be attractive.  Otherwise I'd
> just as soon see us put the effort into making the MSVC scripts more
> robust and able to pull more data from the makefiles.

Some of the build-tooling in cmake is quite nice, I have to admit. I've
e.g. grown to like using ninja instead of make to build the resulting
files. Primarily in projects that take longer than pg to compile - a
clean build in llvm with ninja takes like 0.1 seconds.

Being more able to rely on things working on windows when doing them on
linux does seem like an advantage to me - fiddlin with Mkvcbuild.pm is
quite annoying.

- 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] WIP: About CMake v2

[Jim Nasby]

On 2/8/17 3:52 PM, Tom Lane wrote:

For my own purposes, the only thing that I find seriously annoying about
the status quo is the amount of time required to run "configure".  For
me, that step is usually comparable to or even more than the time to
do the build proper, because (a) ccache and (b) multiple CPUs.
configure isn't parallelizable, and there's probably nothing that
can be done about that.  If CMake offers a substantial improvement
in configuration time then that would be attractive.  Otherwise I'd
just as soon see us put the effort into making the MSVC scripts more
robust and able to pull more data from the makefiles.


FWIW, I've had good luck adding -C to configure to cache the output. I'd 
guess it's at least 10x faster on my laptop.


Obviously doesn't help if you're doing where you're testing something 
that alters config output. In those cases I'll either edit config.cache 
and delete the relevant lines or just temporarily move it out of the way 
(or just nuke it...).

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


--
Sent 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: About CMake v2

[Tom Lane]

Peter Eisentraut  writes:
> On 2/8/17 6:21 AM, Yuriy Zhuravlev wrote:
>> Support two build systems it's not big deal really. I have been working
>> on this past year without any big troubles. 
>> Also we have second perl build system...

> The perl/msvc build system pulls in information from the makefiles.  So
> when you add a file or something basic like that, you don't have to
> update it.  So it's really more like 1.5 build systems.

Really it's more like 1.1 build systems, in that the MSVC scripts do that
just well enough that you *usually* don't have to think about them.  But
then when they fail, and you have to figure out why, it can be a pain.

For my own purposes, the only thing that I find seriously annoying about
the status quo is the amount of time required to run "configure".  For
me, that step is usually comparable to or even more than the time to
do the build proper, because (a) ccache and (b) multiple CPUs.
configure isn't parallelizable, and there's probably nothing that
can be done about that.  If CMake offers a substantial improvement
in configuration time then that would be attractive.  Otherwise I'd
just as soon see us put the effort into making the MSVC scripts more
robust and able to pull more data from the makefiles.

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] WIP: About CMake v2

[Andres Freund]

Hi,

On 2017-01-30 10:26:18 -0500, Peter Eisentraut wrote:
> On 1/30/17 1:28 AM, Andres Freund wrote:
> > Given that fact, I just don't buy why it's a good idea to not also
> > replace autoconf initially.
> 
> Well, I find it a bit scary.  If you do the big switch all at once, then
> you will have to dedicate the following 3 months to fixing complaints
> from developers and build farmers.

That'll be the case just as well if we spread it out over two cycles,
except that we'll have it in multiple phases, and we run into the danger
of a half-done conversion.

I'd rather not change systems at all than run into the danger of that.


> > Either we're able to properly test it - i.e. it runs all tests - on *nix or 
> > we're not.
> 
> That would work if there were a single entry point into the build
> system.  But in practice there are many, and every one of them is
> someone's favorite.  It's unlikely that we will be able to enumerate all
> of them during patch review.

Not sure what you mean with "entry point"?


- 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] WIP: About CMake v2

[Peter Eisentraut]

On 2/8/17 6:21 AM, Yuriy Zhuravlev wrote:
> Support two build systems it's not big deal really. I have been working
> on this past year without any big troubles. 
> Also we have second perl build system...

The perl/msvc build system pulls in information from the makefiles.  So
when you add a file or something basic like that, you don't have to
update it.  So it's really more like 1.5 build systems.

-- 
Peter Eisentraut  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] Documentation improvements for partitioning

[Corey Huinker]

On Fri, Feb 3, 2017 at 4:15 AM, Amit Langote 
wrote:

> Here are some patches to improve the documentation about partitioned
> tables:
>
> 0001: Adds some details about partition_bound_spec to the CREATE TABLE
> page, especially:
>
>  - a note about inclusivity of range partition bounds,
>  - a note about the UNBOUNDED literal in case of range partitioning,
>  - a note about the NULL literal in case of list partitioning,
>
> I wonder if the above "note" should be added under the Notes section or
> are they fine to be added as part of the description of PARTITION OF
> clause. Also:
>
>  - in syntax synopsis, it appears now that any expression is OK to be used
>for individual bound datum, but it's not true.  Only literals are
>allowed.  So fixed that.
>  - added an example showing how to create partitions of a range
>partitioned table with multiple columns in the partition key
>  - added PARTITION BY and PARTITION OF (FOR VALUES) as PostgreSQL
>extensions in the compatibility section
>
>
> 0002: Adds details about partitioned tables to the DDL chapter (ddl.sgml)
>
>  - a new section named "Partitioned Tables" right next to the
>Inheritance and Partitioning sections is created.
>  - examples are added to the existing Partitioning section using the new
>partitioned tables.  Old text about implementing table partitioning
>using inheritance is kept, sort of as a still supported older
>alternative.
>
> 0003: Add partitioning keywords to keywords.sgml
>
> This is all I have for now.  Any feedback is greatly appreciated.  Adding
> this to the next CF.
>
> Thanks,
> Amit
>

Patch applies.

Overall this looks really good. It goes a long way towards stating some of
the things I had to learn through experimentation.

I had to read a really long way into the patch before finding a blurb that
I felt wasn't completely clear:

+
+ 
+  INSERT statements with ON CONFLICT
+  clause are currently not allowed on partitioned tables, that is,
+  cause error when specified.
+ 


Here's some other tries at saying the same thing, none of which are
completely satisfying:

...ON CONFLICT clause are currently not allowed on partitioned tables and
will cause an error?
...ON CONFLICT clause are currently not allowed on partitioned tables and
will instead cause an error?
...ON CONFLICT clause will currently cause an error if used on a
partitioned table?

As far as additional issues to cover, this bit:

+ 
+  
+   One cannot drop a NOT NULL constraint on a
+   partition's column, if the constraint is present in the parent
table.
+  
+ 

Maybe we should add something about how one would go about dropping a NOT
NULL constraint (parent first then partitions?)

In reviewing this patch, do all our target formats make word spacing
irrelevant? i.e. is there any point in looking at the number of spaces
after a period, etc?

A final note, because I'm really familiar with partitioning on Postgres and
other databases, documentation which is clear to me might not be to someone
less familiar with partitioning. Maybe we want another reviewer for that?

Re: [HACKERS] Cannot shutdown subscriber after DROP SUBSCRIPTION

[Robert Haas]

On Sat, Feb 4, 2017 at 3:11 PM, Petr Jelinek
 wrote:
> That was the reason why DropSubscription didn't release the lock in the
> first place. It was supposed to be released at the end of the
> transaction though.

Holding an LWLock until end-of-transaction is a phenomenally bad idea,
both because you lose interruptibility and because of the deadlock
risk.

-- 
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] WAL consistency check facility

[Robert Haas]

On Wed, Feb 8, 2017 at 1:25 AM, Kuntal Ghosh  wrote:
> Thank you Robert for the review. Please find the updated patch in the
> attachment.

I have committed this patch after fairly extensive revisions:

* Rewrote the documentation to give some idea what the underlying
mechanism of operation of the feature is, so that users who choose to
enable this will hopefully have some understanding of what they've
turned on.
* Renamed "char *page" arguments to "char *pagedata" and "Page page",
because tempPage doesn't seem to be to be any better a name than
page_norm.
* Moved bufmask.c to src/backend/access/common, because there's no
code in src/backend/storage/buffer that knows anything about the
format of pages; that is the job of AMs, hence src/backend/access.
* Improved some comments in bufmask.c
* Removed consistencyCheck_is_enabled in favor of determining which
RMs support masking by the presence of absence of an rm_mask function.
* Removed assertion in checkXLogConsistency that consistency checking
is enabled for the RM; that's trivially false if
wal_consistency_checking is not the same on the master and the
standby.  (Note that quite apart from the issue of whether this
function should exist at all, adding it to a header file after the
closing #endif guard is certainly not right.)
* Changed checkXLogConsistency to use RBM_NORMAL_NO_LOG instead of
RBM_NORMAL.  I'm not sure if there are any cases where this makes a
difference, but it seems safer.
* Changed checkXLogConsistency to skip pages whose LSN is newer than
that of the record.  Without this, if you shut down recovery and
restart it, it complains of inconsistent pages and dies.  (I'm not
sure this is the only scenario that needs to be covered; it would be
smart to do more testing of restarting the standby.)
* Made wal_consistency_checking a developer option instead of a WAL
option.  Even though it CAN be used in production, we don't
particularly want to encourage that; enabling WAL consistency checking
has a big performance cost and makes your system more fragile not less
-- a WAL consistency failure causes your standby to die a hard death.
(Maybe there should be a way to suppress consistency checking on the
standby -- but I think not just by requiring wal_consistency_checking
on both ends.  Or maybe we should just downgrade the FATAL to WARNING;
blowing up the standby irrevocably seems like poor behavior.)
* Coding style improvement in check_wal_consistency_checking.
* Removed commas in messages added to pg_xlogdump; those didn't look
good to me, on further review.
* Comment improvements in xlog_internal.h and xlogreader.h

I also bumped XLOG_PAGE_MAGIC (which is normally done by the
committer, not the patch author, so I wasn't expecting that to be in
the patch as submitted).

-- 
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] Patch: Avoid precision error in to_timestamp().

[Tom Lane]

I wrote:
> I wonder if we could make things better just by using rint() rather than
> a naive cast-to-integer.  The cast will truncate not round, and I think
> that might be what's mostly biting you.  Does this help for you?

> #ifdef HAVE_INT64_TIMESTAMP
> - result = seconds * USECS_PER_SEC;
> + result = rint(seconds * USECS_PER_SEC);
> #else

I poked around looking for possible similar issues elsewhere, and found
that a substantial majority of the datetime-related code already uses
rint() when trying to go from float to int representations.  As far as
I can find, this function and make_interval() are the only ones that
fail to do so.  So I'm now thinking that this is a clear oversight,
and both those places need to be patched to use rint().

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] Press Release Draft - 2016-02-09 Cumulative Update

[Alvaro Herrera]

Tobias Bussmann wrote:

> But I could put this
> snippet as a "REINDEX CONCURRENTLY" workaround into the Administrative
> Snippets category of the wiki, if there are no further objections
> about the way it works.

Sounds like a good idea.  There are further complications:

* you can't DROP indexes belonging to constraints, so this recipe
doesn't work for them.  One useful trick is to create the index first,
then ADD CONSTRAINT USING INDEX.

* For unique constraints referenced by FKs, the above doesn't work
either.  One thing you can do is create a second index and swap the
relfilenode underneath.  This is a nasty, dirty, dangerous, unsupported
trick, but it can save people's neck at times.

> I always have a bit of mixed feelings with these kind of string
> manipulations on dynamic SQL.

It may look a bit nasty, but locking tables for long periods (or being
without an important index for a period) is much worse in production
scenarios.

-- 
Álvaro Herrerahttps://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] Press Release Draft - 2016-02-09 Cumulative Update

[Tobias Bussmann]

Am 08.02.2017 um 20:17 schrieb Alvaro Herrera :
> Note that this is likely to fail if the original index name is close to
> the 63 chars limit.  Perhaps it's enough to add substring() when
> computing index_name_tmp.  (You could just not use :'index_name' there
> and rely on the random md5 only, actually).  Watch out for UNIQUE too.

thank you for your valuable input! Here is a version that should take both into 
account - the query also could be simplified a bit:

\set index_name 'my_bad_index' 
\set table_schema 'public'
SELECT 'tmp_'||md5(random()::text) AS index_name_tmp \gset
SELECT 
replace(pg_get_indexdef((quote_ident(:'table_schema')||'.'||quote_ident(:'index_name'))::regclass),
 ' '||quote_ident(:'index_name')||' ON', ' CONCURRENTLY '||:'index_name_tmp'||' 
ON') \gexec
DROP INDEX CONCURRENTLY :"table_schema".:"index_name";
ALTER INDEX :"table_schema".:"index_name_tmp" RENAME TO :"index_name";


> FWIW for previous problems we've documented them in wiki pages along
> with suggested solutions, and added a link to that wiki page in the
> announce.  Perhaps one thing to do is create a wiki page for this one
> too (not volunteering myself). 

I'm not even remotely into the details of the CIC issue, so I'm not the right 
one to create a page on that topic. But I could put this snippet as a "REINDEX 
CONCURRENTLY" workaround into the Administrative Snippets category of the wiki, 
if there are no further objections about the way it works. I always have a bit 
of mixed feelings with these kind of string manipulations on dynamic SQL.

Best,
Tobias

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

Re: [HACKERS] possibility to specify template database for pg_regress

[Pavel Stehule]

Hi

2017-02-08 8:33 GMT+01:00 Pavel Stehule :

>
>
> 2017-02-08 8:30 GMT+01:00 Michael Paquier :
>
>> On Wed, Feb 8, 2017 at 4:24 PM, Pavel Stehule 
>> wrote:
>> > What is sense for list of databases?
>>
>> ECPG uses it for example, see 0992259.
>>
>> > Some option --template can be great - with backpatch if it is possible.
>>
>> That's not really complicated to patch... That could be a nice task
>> for a starter.
>>
>
> Today I am doing some training - I can look on it at evening
>

here is a patch

Regards

Pavel


>
> Regards
>
> Pavel
>
>
>> --
>> Michael
>>
>
>
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index d4d00d9..354b918 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -68,6 +68,7 @@ const char *pretty_diff_opts = "-w -C3";
 
 /* options settable from command line */
 _stringlist *dblist = NULL;
+_stringlist *templatelist = NULL;
 bool		debug = false;
 char	   *inputdir = ".";
 char	   *outputdir = ".";
@@ -1907,7 +1908,7 @@ drop_database_if_exists(const char *dbname)
 }
 
 static void
-create_database(const char *dbname)
+create_database(const char *dbname, const char *template)
 {
 	_stringlist *sl;
 
@@ -1917,10 +1918,12 @@ create_database(const char *dbname)
 	 */
 	header(_("creating database \"%s\""), dbname);
 	if (encoding)
-		psql_command("postgres", "CREATE DATABASE \"%s\" TEMPLATE=template0 ENCODING='%s'%s", dbname, encoding,
+		psql_command("postgres", "CREATE DATABASE \"%s\" TEMPLATE=\"%s\" ENCODING='%s'%s",
+	 dbname, template, encoding,
 	 (nolocale) ? " LC_COLLATE='C' LC_CTYPE='C'" : "");
 	else
-		psql_command("postgres", "CREATE DATABASE \"%s\" TEMPLATE=template0%s", dbname,
+		psql_command("postgres", "CREATE DATABASE \"%s\" TEMPLATE=\"%s\"%s",
+	 dbname, template,
 	 (nolocale) ? " LC_COLLATE='C' LC_CTYPE='C'" : "");
 	psql_command(dbname,
  "ALTER DATABASE \"%s\" SET lc_messages TO 'C';"
@@ -1995,6 +1998,7 @@ help(void)
 	printf(_("  --outputdir=DIR   place output files in DIR (default \".\")\n"));
 	printf(_("  --schedule=FILE   use test ordering schedule from FILE\n"));
 	printf(_("(can be used multiple times to concatenate)\n"));
+	printf(_("  --template=DB use template DB (default \"template0\")\n"));
 	printf(_("  --temp-instance=DIR   create a temporary instance in DIR\n"));
 	printf(_("  --use-existinguse an existing installation\n"));
 	printf(_("\n"));
@@ -2041,6 +2045,7 @@ regression_main(int argc, char *argv[], init_function ifunc, test_function tfunc
 		{"launcher", required_argument, NULL, 21},
 		{"load-extension", required_argument, NULL, 22},
 		{"config-auth", required_argument, NULL, 24},
+		{"template", required_argument, NULL, 25},
 		{NULL, 0, NULL, 0}
 	};
 
@@ -2154,6 +2159,16 @@ regression_main(int argc, char *argv[], init_function ifunc, test_function tfunc
 			case 24:
 config_auth_datadir = pg_strdup(optarg);
 break;
+			case 25:
+
+/*
+ * If a default template was specified, we need to remove it
+ * before we add the specified one.
+ */
+free_stringlist();
+split_to_stringlist(optarg, ",", );
+break;
+
 			default:
 /* getopt_long already emitted a complaint */
 fprintf(stderr, _("\nTry \"%s -h\" for more information.\n"),
@@ -2454,8 +2469,25 @@ regression_main(int argc, char *argv[], init_function ifunc, test_function tfunc
 	 */
 	if (!use_existing)
 	{
-		for (sl = dblist; sl; sl = sl->next)
-			create_database(sl->str);
+		if (templatelist != NULL)
+		{
+			_stringlist *tl;
+
+			for (sl = dblist, tl = templatelist; sl; sl = sl->next, tl = tl->next)
+			{
+if (tl != NULL)
+	create_database(sl->str, tl->str);
+else
+{
+	fprintf(stderr, _("%s: the template list is shorter than database list\n"),
+			progname);
+	exit(2);
+}
+			}
+		}
+		else
+			for (sl = dblist; sl; sl = sl->next)
+create_database(sl->str, "template0");
 		for (sl = extraroles; sl; sl = sl->next)
 			create_role(sl->str, dblist);
 	}

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

Re: [HACKERS] Backport of pg_statistics typos fix

[Robert Haas]

On Tue, Feb 7, 2017 at 4:22 AM, Yugo Nagata  wrote:
> I found typos "pg_statistics" in REL9_6_STABLE, but that has been
> fixed in the master branch.
>
> Fix typo: pg_statistics -> pg_statistic
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=5a366b4ff4ceceb9793fcc13c3f097ee0d32c56d;hp=f7c62462402972b13d10e43f104ca0c0fecb6d08
>
> I think it would be better to backport this to other branches.

We usually leave such decisions to the discretion of the committer,
because back-porting such changes takes time and sometimes it just
isn't that important.  Nobody's likely to be confused by a few
instances of writing pg_statistics rather than pg_statistic.
Personally, I favor not back-porting such things in most cases,
because I think patches that get back-ported should be strictly
limited to bug fixes, and typos in code comments aren't bug fixes.
But not everyone has the same opinion on this.  What's your reason for
wanting it back-ported?

BTW, looking at that commit, this change looks to have adjusted this
from being wrong to still being wrong:

-Allow pg_statistics to be reset by calling
pg_stat_reset() (Christopher)
+Allow pg_statistic to be reset by calling
pg_stat_reset() (Christopher)

It's true that pg_stat_reset() doesn't reset the nonexistent
pg_statistics table.  But it doesn't reset pg_statistic either.  IIUC,
it resets the data gathered by the statistics collector, which is
something else altogether.

-- 
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] Adding the optional clause 'AS' in CREATE TRIGGER

[Robert Haas]

On Tue, Feb 7, 2017 at 3:11 AM, Okano, Naoki  wrote:
> On Wednesday, November 16, 2016 4:31 PM Okano Naoki wrote:
>> > But in any case it would be a serious mistake to do this without first
>> > implementing CREATE OR REPLACE TRIGGER.  I think that's an entirely 
>> > separate
>> > proposal and you would be well advised to treat it as such.
>> I see. There are more problems than I expected...
>> Let me start with 'OR REPLACE' clause.
> I tried to cretae a patch for CREATE OR REPLACE TRIGGER.
> An example of execution is shown below.
>
> example)
> 1.define a new trigger
>   CREATE TRIGGER regular_trigger
>AFTER UPDATE ON table_name
>REFERENCING OLD TABLE AS oldtable_1 NEW TABLE AS newtable_1
>FOR EACH STATEMENT
>EXECUTE PROCEDURE func_1();
> 2.redinfe a trigger in single command
>  CREATE OR REPLACE TRIGGER regular_trigger
>AFTER UPDATE OR DELETE ON table_name
>REFERENCING OLD TABLE AS oldtable_2 NEW TABLE AS newtable_2
>FOR EACH ROW
>EXECUTE PROCEDURE func_2();
>
> If the named trigger does not exist.
> a new trigger is also created by using OR REPLACE clause.
> A regular trigger cannot be replaced by a constraint trigger and vice varsa.
> because a constraint trigger has a different role from regular triger.
>
> Please give me feedback.

You should add this to the next CommitFest so it doesn't get missed.

https://commitfest.postgresql.org/

-- 
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] Press Release Draft - 2016-02-09 Cumulative Update

[Alvaro Herrera]

Tobias Bussmann wrote:
> Am 07.02.2017 um 18:44 schrieb Alvaro Herrera :
> >  80 CREATE INDEX CONCURRENTLY bad_index_name ON table_name 
> > (column_name); /* replace names with your original index definition */
> 
> I was thinking if we could replace that "replace names with your original 
> index definition" with something more fancy using pg_get_indexdef in that 
> recipe. I ended up with quite a "REINDEX CONCURRENTLY" monster:
> 
> \set index_name 'my_bad_index' 
> \set table_schema 'public'
> SELECT :'index_name'||'_'||left(md5(random()::text), 5) AS index_name_tmp 
> \gset
> SELECT 
> replace(replace(pg_get_indexdef((quote_ident(:'table_schema')||'.'||quote_ident(:'index_name'))::regclass),
>  'INDEX '||quote_ident(:'index_name'), 'INDEX 
> '||quote_ident(:'index_name_tmp')), 'CREATE INDEX', 'CREATE INDEX 
> CONCURRENTLY') \gexec
> DROP INDEX CONCURRENTLY :"table_schema".:"index_name";
> ALTER INDEX :"table_schema".:"index_name_tmp" RENAME TO :"index_name";
> 
> Probably not useable as a recipe in such an announcement but it was fun to 
> build and to see what is actually possible with some psql magic :)

Note that this is likely to fail if the original index name is close to
the 63 chars limit.  Perhaps it's enough to add substring() when
computing index_name_tmp.  (You could just not use :'index_name' there
and rely on the random md5 only, actually).  Watch out for UNIQUE too.

FWIW for previous problems we've documented them in wiki pages along
with suggested solutions, and added a link to that wiki page in the
announce.  Perhaps one thing to do is create a wiki page for this one
too (not volunteering myself).  Probably too late to add the link to the
press release now, since it's already out as "final".

-- 
Álvaro Herrerahttps://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] Press Release Draft - 2016-02-09 Cumulative Update

[Tobias Bussmann]

Am 07.02.2017 um 18:44 schrieb Alvaro Herrera :
>  80 CREATE INDEX CONCURRENTLY bad_index_name ON table_name (column_name); 
> /* replace names with your original index definition */

I was thinking if we could replace that "replace names with your original index 
definition" with something more fancy using pg_get_indexdef in that recipe. I 
ended up with quite a "REINDEX CONCURRENTLY" monster:

\set index_name 'my_bad_index' 
\set table_schema 'public'
SELECT :'index_name'||'_'||left(md5(random()::text), 5) AS index_name_tmp \gset
SELECT 
replace(replace(pg_get_indexdef((quote_ident(:'table_schema')||'.'||quote_ident(:'index_name'))::regclass),
 'INDEX '||quote_ident(:'index_name'), 'INDEX 
'||quote_ident(:'index_name_tmp')), 'CREATE INDEX', 'CREATE INDEX 
CONCURRENTLY') \gexec
DROP INDEX CONCURRENTLY :"table_schema".:"index_name";
ALTER INDEX :"table_schema".:"index_name_tmp" RENAME TO :"index_name";

Probably not useable as a recipe in such an announcement but it was fun to 
build and to see what is actually possible with some psql magic :)

Tobias




-- 
Sent 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_bsd_indent: implement -lps ("leave preprocessor space")

[Andres Freund]

On 2017-02-07 23:30:44 -0500, Tom Lane wrote:
> Piotr Stefaniak  writes:
> > this is a patch that Andres asked me for. It makes pg_bsd_indent leave
> > preprocessor space alone, as in this example:
> 
> > #if 0
> > #  if 0
> > #   if 0
> > # error
> > #   endif
> > #  endif
> > #else
> > #  line 7
> > #endif

For context: I'd asked Piotr how dificult it'd be to add this.

> Um ... but the point of pgindent is to standardize spacing, not to let
> people invent their own style. If you wanted to have a discussion about
> whether pgindent should force preprocessor directives to look like the
> above, we could talk about that.  But I do not want to be reading code that
> looks like the above in one place and code that does not ten lines away.

I don't think that's something easily done in an automatic
manner. Because you'd e.g. obviously not want to indent everything
within include guards.   I don't really buy the danger of large
divergances in code nearby - this seems mostly useful when writing a bit
more complicated macros, and I don't think they'll be that frequently
added in existing files.

I do think this makes the nesting for #ifdefs a *lot* more readable, and
we have plenty of cases where it's currently really hard to discern what
"branch" one is currently reading.  Allowing to opt-in into the "newer"
formatting in places where it makes sense, seems reasonable to me.

Regards,

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] multivariate statistics (v19)

[Dean Rasheed]

On 8 February 2017 at 16:09, David Fetter  wrote:
> Combinations are n!/(k! * (n-k)!), so computing those is more
> along the lines of:
>
> unsigned long long
> choose(unsigned long long n, unsigned long long k) {
> if (k > n) {
> return 0;
> }
> unsigned long long r = 1;
> for (unsigned long long d = 1; d <= k; ++d) {
> r *= n--;
> r /= d;
> }
> return r;
> }
>
> which greatly reduces the chance of overflow.
>

Hmm, but that doesn't actually prevent overflows, since it can
overflow in the multiplication step, and there is no protection
against that.

In the algorithm I presented, the inputs and the intermediate result
are kept below INT_MAX, so the multiplication step cannot overflow the
64-bit integer, and it will only raise an overflow error if the actual
result won't fit in a 32-bit int. Actually a crucial part of that,
which I failed to mention previously, is the first step replacing k
with min(k, n-k). This is necessary for inputs like (100,99), which
should return 100, and which must be computed as 100 choose 1, not 100
choose 99, otherwise it will overflow internally before getting to the
final result.

Regards,
Dean


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

[HACKERS] pg_basebackup -R

[Robert Haas]

I just tried out pg_basebackup -R and got the following recovery.conf file:

standby_mode = 'on'
primary_conninfo = 'user=rhaas passfile=''/home/rhaas/.pgpass''
port=5432 sslmode=disable sslcompression=1 target_session_attrs=any'

This seems fairly random to me.  pg_basebackup explains:

 * Do not emit this setting if: - the setting is "replication",
 * "dbname" or "fallback_application_name", since these would be
 * overridden by the libpqwalreceiver module anyway. -
not set or
 * empty.

...which looks like it got clubbed by pgindent, but anyway I'm not
sure that's the best algorithm.  passfile and target_session_attrs are
both new in v10 and have non-empty default values, yet neither of them
seems like something that you necessarily want in your
automatically-created recovery.conf file.  It seems like it would be
more correct to leave out parameters that are set to the default
value, rather than those that are set to an empty value.

-- 
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] Patch: Avoid precision error in to_timestamp().

[Tom Lane]

=?UTF-8?Q?Erik_Nordstr=C3=B6m?=  writes:
> I stumbled upon a precision issue with the to_timestamp() function that
> causes it to return unexpected timestamp values. For instance, the query
> SELECT to_timestamp(1486480176.236538) returns the timestamp "2017-02-07
> 16:09:36.236537+01", which is off by one microsecond. Looking at the source
> code, the issue seems to be that the conversion is unnecessarily done using
> imprecise floating point calculations. Since the target timestamp has
> microsecond precision, and is internally represented by a 64-bit integer
> (on modern platforms), it is better to first convert the given floating
> point value to a microsecond integer and then doing the epoch conversion,
> rather than doing the conversion using floating point and finally casting
> to an integer/timestamp.

This change would introduce overflow failures near the end of the range of
valid inputs.  Maybe it's worth doing anyway and we should just tighten
the range bound tests right above what you patched, but I'm a bit
skeptical.  Float inputs are going to be inherently imprecise anyhow.

I wonder if we could make things better just by using rint() rather than
a naive cast-to-integer.  The cast will truncate not round, and I think
that might be what's mostly biting you.  Does this help for you?

#ifdef HAVE_INT64_TIMESTAMP
-   result = seconds * USECS_PER_SEC;
+   result = rint(seconds * USECS_PER_SEC);
#else

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] Idea on how to simplify comparing two sets

[David G. Johnston]

On Wed, Feb 8, 2017 at 10:30 AM, Tom Lane  wrote:

> David Fetter  writes:
> > On Wed, Feb 08, 2017 at 11:22:56AM -0500, Tom Lane wrote:
> >> Yes.  I think a new set-operation keyword would inevitably have to
> >> be fully reserved --- UNION, INTERSECT, and EXCEPT all are --- which
> >> means that you'd break every application that has used that word as
> >> a table, column, or function name.
>
> > I've long wanted a SYMMETRIC DIFFERENCE join type, that being the only
> > elementary set operation not included in join types, but nobody at the
> > SQL standards committee seems to have cared enough to help.
>
> I wonder whether you could shoehorn it in with no new reserved word
> by spelling it "EXCEPT SYMMETRIC", which could be justified by the
> precedent of BETWEEN SYMMETRIC.  But not sure what to do with
> duplicate rows (ie, if LHS has two occurrences of row X and RHS
> has one occurrence, do you output X?)
>

​Without SYMMETRIC its defined to return:

​max(m-n,0)

with SYMMETRIC I'd think that would just change to:

abs(m-n)

Then you still have to apply ALL or DISTINCT on the above result.

David J.

Re: [HACKERS] Idea on how to simplify comparing two sets

[Tom Lane]

David Fetter  writes:
> On Wed, Feb 08, 2017 at 11:22:56AM -0500, Tom Lane wrote:
>> Yes.  I think a new set-operation keyword would inevitably have to
>> be fully reserved --- UNION, INTERSECT, and EXCEPT all are --- which
>> means that you'd break every application that has used that word as
>> a table, column, or function name.

> I've long wanted a SYMMETRIC DIFFERENCE join type, that being the only
> elementary set operation not included in join types, but nobody at the
> SQL standards committee seems to have cared enough to help.

I wonder whether you could shoehorn it in with no new reserved word
by spelling it "EXCEPT SYMMETRIC", which could be justified by the
precedent of BETWEEN SYMMETRIC.  But not sure what to do with
duplicate rows (ie, if LHS has two occurrences of row X and RHS
has one occurrence, do you output X?)

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] Idea on how to simplify comparing two sets

[David Fetter]

On Wed, Feb 08, 2017 at 11:22:56AM -0500, Tom Lane wrote:
> Robert Haas  writes:
> > On Wed, Feb 8, 2017 at 4:24 AM, Pantelis Theodosiou  
> > wrote:
> >> I'm not advocating it but I don't see how introducing new SQL keywords
> >> breaks backwards compatibility.
> 
> > It does at least a little bit.
> 
> Yes.  I think a new set-operation keyword would inevitably have to
> be fully reserved --- UNION, INTERSECT, and EXCEPT all are --- which
> means that you'd break every application that has used that word as
> a table, column, or function name.

I've long wanted a SYMMETRIC DIFFERENCE join type, that being the only
elementary set operation not included in join types, but nobody at the
SQL standards committee seems to have cared enough to help.

> Generally speaking, we try very darn hard not to introduce new
> reserved words that are not called out as reserved in the SQL
> standard.  (And even for those, we've sometimes made the grammar
> jump through hoops so as not to reserve a word that we didn't
> reserve previously.)

We just never know what new keywords the standards committee will
dream up, or what silliness they'll introduce in the grammar :/

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

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


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

Re: [HACKERS] [PATCH] configure-time knob to set default ssl ciphers

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> If we did have code for multiple libraries, perhaps some people would
>> want to compile all the variants at once; in which case overloading a
>> single option to be used for all the libraries would be a problem.

> Hmm, I don't think our abstraction would allow for compiling more than
> one at a time.  ISTM that all that work has been considering that you'd
> choose at most one at compile time.

Very possibly it'll end up that way.  But I'm not eager to pre-judge that
decision, especially if we're doing it only to support a system-specific
hack that could be handled in other ways.

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] configure-time knob to set default ssl ciphers

[Alvaro Herrera]

Tom Lane wrote:
> Daniel Gustafsson  writes:
> > Since we hopefully will support more SSL libraries than OpenSSL at some 
> > point,
> > and we don’t want a torrent of configure options, wouldn’t this be better as
> > --with-server-ciphers=STRING or something similar?  
> 
> One of the reasons I'm not very excited about exposing this as a configure
> option is exactly that I'm not sure what happens when we get multiple TLS
> library support.  The cipher list we've got at the moment seems like it
> is probably OpenSSL-specific (but maybe not?).

Maybe the list of ciphers is not OpenSSL-specific, but the *syntax* most
likely is.  Particularly the abbreviations such as !eNULL and !MD5, etc.

> If we did have code for multiple libraries, perhaps some people would
> want to compile all the variants at once; in which case overloading a
> single option to be used for all the libraries would be a problem.

Hmm, I don't think our abstraction would allow for compiling more than
one at a time.  ISTM that all that work has been considering that you'd
choose at most one at compile time.  I'm not sure it's useful to have
more than one anyway.  If you choose one SSL implementation at configure
time, it's on your head to specify a ssl-ciphers that that
implementation accepts (of course, we would choose a working default if
you don't specify one.)

(I was going to suggest --with-ssl-ciphers but the protocol is called
TLS nowadays, so maybe not a great idea.)

-- 
Álvaro Herrerahttps://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] GSoC 2017

[Pavel Stehule]

2017-02-08 17:06 GMT+01:00 Robert Haas :

> On Mon, Feb 6, 2017 at 6:51 AM, Ruben Buchatskiy  wrote:
> > 2017-01-10 12:53 GMT+03:00 Alexander Korotkov  >:
> >> 1. What project ideas we have?
> >
> > We would like to propose a project on rewriting PostgreSQL executor from
> >
> > traditional Volcano-style [1] to so-called push-based architecture as
> > implemented in
> >
> > Hyper [2][3] and VitesseDB [4]. The idea is to reverse the direction of
> data
> > flow
> >
> > control: instead of pulling up tuples one-by-one with ExecProcNode(), we
> > suggest
> >
> > pushing them from below to top until blocking operator (e.g.
> Aggregation) is
> >
> > encountered. There’s a good example and more detailed explanation for
> this
> > approach in [2].
>
> I think this very possibly a good idea but extremely unlikely to be
> something that a college student or graduate student can complete in
> one summer.  More like an existing expert developer and a year of
> doing not much else.
>

+1

Pavel


>
> --
> 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] [PATCH] configure-time knob to set default ssl ciphers

[Tom Lane]

Daniel Gustafsson  writes:
> Since we hopefully will support more SSL libraries than OpenSSL at some point,
> and we don’t want a torrent of configure options, wouldn’t this be better as
> --with-server-ciphers=STRING or something similar?  

One of the reasons I'm not very excited about exposing this as a configure
option is exactly that I'm not sure what happens when we get multiple TLS
library support.  The cipher list we've got at the moment seems like it
is probably OpenSSL-specific (but maybe not?).  If we did have code for
multiple libraries, perhaps some people would want to compile all the
variants at once; in which case overloading a single option to be used for
all the libraries would be a problem.

This would all be a lot clearer if we already had that code, but since
we don't, I'm inclined to be conservative about exposing new features
that make assumptions about how it will be.

regards, tom lane


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

[HACKERS] Patch: Avoid precision error in to_timestamp().

[Erik Nordström]

Hello hackers,

I stumbled upon a precision issue with the to_timestamp() function that
causes it to return unexpected timestamp values. For instance, the query
SELECT to_timestamp(1486480176.236538) returns the timestamp "2017-02-07
16:09:36.236537+01", which is off by one microsecond. Looking at the source
code, the issue seems to be that the conversion is unnecessarily done using
imprecise floating point calculations. Since the target timestamp has
microsecond precision, and is internally represented by a 64-bit integer
(on modern platforms), it is better to first convert the given floating
point value to a microsecond integer and then doing the epoch conversion,
rather than doing the conversion using floating point and finally casting
to an integer/timestamp.

I am attaching a patch that fixes the above issue.

Regards,

Erik


to_timestamp_fix.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] Idea on how to simplify comparing two sets

[Tom Lane]

Robert Haas  writes:
> On Wed, Feb 8, 2017 at 4:24 AM, Pantelis Theodosiou  
> wrote:
>> I'm not advocating it but I don't see how introducing new SQL keywords
>> breaks backwards compatibility.

> It does at least a little bit.

Yes.  I think a new set-operation keyword would inevitably have to be
fully reserved --- UNION, INTERSECT, and EXCEPT all are --- which means
that you'd break every application that has used that word as a table,
column, or function name.

Generally speaking, we try very darn hard not to introduce new reserved
words that are not called out as reserved in the SQL standard.  (And even
for those, we've sometimes made the grammar jump through hoops so as
not to reserve a word that we didn't reserve previously.)

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] configure-time knob to set default ssl ciphers

[Daniel Gustafsson]

> On 08 Feb 2017, at 13:31, Pavel Raiskup  wrote:
> 
> On Wednesday, February 8, 2017 1:29:19 PM CET Pavel Raiskup wrote:
>> On Wednesday, February 8, 2017 1:05:08 AM CET Tom Lane wrote:
>>> Peter Eisentraut  writes:
 On 2/7/17 11:21 AM, Tom Lane wrote:
> A compromise that might be worth considering is to introduce
> #define PG_DEFAULT_SSL_CIPHERS "HIGH:MEDIUM:+3DES:!aNULL"
> into pg_config_manual.h, which would at least give you a reasonably
> stable target point for a long-lived patch.
>>> 
 You'd still need to patch postgresql.conf.sample somehow.
>>> 
>>> Right.  The compromise position that I had in mind was to add the
>>> #define in pg_config_manual.h and teach initdb to propagate it into
>>> the installed copy of postgresql.conf, as we've done with other GUCs
>>> with platform-dependent defaults, such as backend_flush_after.
>>> 
>>> That still leaves the question of what to do with the SGML docs.
>>> We could add some weasel wording to the effect that the default might
>>> be platform-specific, or we could leave the docs alone and expect the
>>> envisioned Red Hat patch to patch config.sgml along with
>>> pg_config_manual.h.
>> 
>> Thanks for quickt feedback.  Just to not give up too early, I'm attaching
>> 2nd iteration.  I'm fine to fallback to pg_config_manual.h solution though,
>> if this is considered too bad.
>> 
>> I tried to fix the docs now (crucial part indeed) so we are not that
>> "scrict" and there's some space for per-distributor change of ssl_ciphers
>> default.
>> 
>> From the previous mail:
>>> I'm not really sure that we want to carry around that much baggage for a
>>> single-system hack.
>> 
>> Accepted, but still I'm giving a chance.  OpenSSL maintainers predict this 
>> (or
>> something else in similar fashion) is going to be invented in OpenSSL 
>> upstream.
>> So there's still some potential in ./configure option.
> 
> Argh :( !  Attaching now and sorry.

Since we hopefully will support more SSL libraries than OpenSSL at some point,
and we don’t want a torrent of configure options, wouldn’t this be better as
--with-server-ciphers=STRING or something similar?  

+   --with-openssl-be-ciphers=STRING
+   Replace the default list of server-supported ciphers

Each SSL implementation would then be responsible for handling it appropriately.

cheers ./daniel



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

Re: [HACKERS] multivariate statistics (v19)

[David Fetter]

On Wed, Feb 08, 2017 at 03:23:25PM +, Dean Rasheed wrote:
> On 6 February 2017 at 21:26, Alvaro Herrera  wrote:
> > Tomas Vondra wrote:
> >> On 02/01/2017 11:52 PM, Alvaro Herrera wrote:
> >
> >> > Nearby, some auxiliary functions such as n_choose_k and
> >> > num_combinations are not documented. What it is that they do? I'd
> >> > move these at the end of the file, keeping the important entry points
> >> > at the top of the file.
> >>
> >> I'd say n-choose-k is pretty widely known term from combinatorics. The
> >> comment would essentially say just 'this is n-choose-k' which seems rather
> >> pointless. So as much as I dislike the self-documenting code, this actually
> >> seems like a good case of that.
> >
> > Actually, we do have such comments all over the place.  I knew this as
> > "n sobre k", so the english name doesn't immediately ring a bell with me
> > until I look it up; I think the function comment could just say
> > "n_choose_k -- this function returns the binomial coefficient".
> 
> One of the things you have to watch out for when writing code to
> compute binomial coefficients is integer overflow, since the numerator
> and denominator get large very quickly. For example, the current code
> will overflow for n=13, k=12, which really isn't that large.
> 
> This can be avoided by computing the product in reverse and using a
> larger datatype like a 64-bit integer to store a single intermediate
> result. The point about multiplying the terms in reverse is that it
> guarantees that each intermediate result is an exact integer (a
> smaller binomial coefficient), so there is no need to track separate
> numerators and denominators, and you avoid huge intermediate
> factorials. Here's what that looks like in psuedo-code:
> 
> binomial(int n, int k):
> # Save computational effort by using the symmetry of the binomial
> # coefficients
> k = min(k, n-k);
> 
> # Compute the result using binomial(n, k) = binomial(n-1, k-1) * n / k,
> # starting from binomial(n-k, 0) = 1, and computing the sequence
> # binomial(n-k+1, 1), binomial(n-k+2, 2), ...
> #
> # Note that each intermediate result is an exact integer.
> int64 result = 1;
> for (int i = 1; i <= k; i++)
> {
> result = (result * (n-k+i)) / i;
> if (result > INT_MAX) Raise overflow error
> }
> return (int) result;
> 
> 
> Note also that I think num_combinations(n) is just an expensive way of
> calculating 2^n - n - 1.

Combinations are n!/(k! * (n-k)!), so computing those is more
along the lines of:

unsigned long long
choose(unsigned long long n, unsigned long long k) {
if (k > n) {
return 0;
}
unsigned long long r = 1;
for (unsigned long long d = 1; d <= k; ++d) {
r *= n--;
r /= d;
}
return r;
}

which greatly reduces the chance of overflow.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

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


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

Re: [HACKERS] drop support for Python 2.3

[Andrew Dunstan]

On 02/07/2017 11:49 PM, Tom Lane wrote:
> Peter Eisentraut  writes:
>> I would like to propose that we drop support for Python 2.3.
>> ...
>> We do have buildfarm coverage on prairiedog.  However, that runs a >10
>> year old operating system, so I think it is not representing real usage.
> I have no particular objection to dropping 2.3 support, but should we
> make some effort to fail gracefully (ie, with a relevant error message)
> on older versions?  I would guess that the effect of your patch will be
> to produce quite opaque failures.  We seem to be computing python_version
> in configure, so it shouldn't be that hard to check.
>
>> - It's unlikely that Python 2.3 is still used in practice.  Python 2.4
>> is in RHEL 5, which is the typically the oldest mainstream OS we look at.
> Hm, is there anything running 2.4 in the buildfarm?  If we're going to
> claim support for 2.4, we'd be well advised to test it.



with top as (select distinct on (sysname) sysname, snapshot from
build_status_recent_500 where branch = 'HEAD' order by sysname,
snapshot desc ) select * from top where exists (select 1 from
build_status_log l where l.sysname = top.sysname and l.snapshot =
top.snapshot and l.branch = 'HEAD' and l.log_stage = 'config.log'
and l.log_text ~ 'python2\.4');


This returns no rows.

Maybe we need to set up a Centos5 or RHEL 5 animal.


cheers

andrew

-- 
Andrew Dunstanhttps://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] GSoC 2017

[Robert Haas]

On Mon, Feb 6, 2017 at 6:51 AM, Ruben Buchatskiy  wrote:
> 2017-01-10 12:53 GMT+03:00 Alexander Korotkov :
>> 1. What project ideas we have?
>
> We would like to propose a project on rewriting PostgreSQL executor from
>
> traditional Volcano-style [1] to so-called push-based architecture as
> implemented in
>
> Hyper [2][3] and VitesseDB [4]. The idea is to reverse the direction of data
> flow
>
> control: instead of pulling up tuples one-by-one with ExecProcNode(), we
> suggest
>
> pushing them from below to top until blocking operator (e.g. Aggregation) is
>
> encountered. There’s a good example and more detailed explanation for this
> approach in [2].

I think this very possibly a good idea but extremely unlikely to be
something that a college student or graduate student can complete in
one summer.  More like an existing expert developer and a year of
doing not much else.

-- 
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] chomp PQerrorMessage() in backend uses

[Tom Lane]

Peter Eisentraut  writes:
> Here is a patch to systematically trim the trailing newlines off
> PQerrorMessage() results in backend uses (dblink, postgres_fdw,
> libpqwalreceiver).

+1

> I noticed that there are some inconsistent assumptions about whether
> PQerrorMessage() can ever return NULL.  From the code, I think that
> should not be possible, but some code appears to be prepared for it.
> Other code is not.  What is correct?

libpq.sgml doesn't specify, so it's hard to argue that either position
is "correct".  I don't mind resolving the ambiguity via a documentation
change though.  I'd want to see it also cover other corner cases like
what if there hasn't been an error on the connection.

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] Parallel bitmap heap scan

[Robert Haas]

On Wed, Feb 8, 2017 at 8:58 AM, Dilip Kumar  wrote:
> On Wed, Feb 8, 2017 at 7:01 PM, Robert Haas  wrote:
>> You can store whatever you want in SH_TYPE's private_data member.
>> SH_ALLOCATE and SH_FREE both get a pointer to the SH_TYPE, so they
>> have access to that.  Hmm, but there's no way to get that set in
>> SH_CREATE before SH_ALLOCATE is called.  Maybe we need to add a
>> private_data argument to SH_CREATE.  execGrouping.c could use that
>> instead of frobbing private_data directly:
>>
>> -hashtable->hashtab = tuplehash_create(tablecxt, nbuckets);
>> -hashtable->hashtab->private_data = hashtable;
>> +hashtable->hashtab = tuplehash_create(tablecxt, nbuckets, hashtable);
>
> Okay, will go ahead as you suggested. Patch attached for the same.

Looks good to me.  If nobody has further ideas here, I'll push this
and your previous patch tomorrow.

-- 
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] pageinspect: Hash index support

[Robert Haas]

On Wed, Feb 8, 2017 at 9:25 AM, Ashutosh Sharma  wrote:
>>> 1) Check if an overflow page is a new page. If so, read a bitmap page
>>> to confirm if a bit corresponding to this overflow page is clear or
>>> not. For this, I would first add Assert statement to ensure that the
>>> bit is clear and if it is, then set the statusbit as false indicating
>>> that the page is free.
>>>
>>> 2) In case if an overflow page is not new, first verify if it is
>>> really an overflow page and if so, check if the bit corresponding to
>>> it in the bitmap page is SET. If so, set the statusbit as true; If
>>> not, we would see an assertion failure happening.
>>
>> I think this is complicated and not what anybody wants.  I think you
>> should do exactly what I said above: return true if the bit is set in
>> the bitmap, and false if it isn't.  Full stop.  Don't read or do
>> anything with the underlying page.  Only read the bitmap page.
>
> Okay, As per the inputs from you, I have modified hash_bitmap_info()
> and have tried to keep the things simple. Attached is the patch that
> has this changes. Please have a look and let me know if you feel it is
> not yet in the right shape. Thanks.

Well, this changes the function signature and I don't see any
advantage in doing that.  Also, it still doesn't read the code that
reads the overflow page.  Any correct patch for this problem needs to
include the following hunk:

-buf = ReadBufferExtended(indexRel, MAIN_FORKNUM, (BlockNumber) ovflblkno,
- RBM_NORMAL, NULL);
-LockBuffer(buf, BUFFER_LOCK_SHARE);
-_hash_checkpage(indexRel, buf, LH_PAGE_TYPE);
-page = BufferGetPage(buf);
-opaque = (HashPageOpaque) PageGetSpecialPointer(page);
-
-if (opaque->hasho_flag != LH_OVERFLOW_PAGE)
-ereport(ERROR,
-(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("page is not an overflow page"),
- errdetail("Expected %08x, got %08x.",
-LH_OVERFLOW_PAGE, opaque->hasho_flag)));
-
-if (BlockNumberIsValid(opaque->hasho_prevblkno))
-bit = true;
-
-UnlockReleaseBuffer(buf);

And then, instead, you need to add some code to set bit based on the
bitmap page, like what you have:

+mapbuf = _hash_getbuf(indexRel, bitmapblkno, HASH_READ, LH_BITMAP_PAGE);
+mappage = BufferGetPage(mapbuf);
+freep = HashPageGetBitmap(mappage);
+
+if (ISSET(freep, bitmapbit))
+bit = 1;

Except I would write that last line as...

bit = ISSET(freep, bitmapbit) != 0

...instead of using an if statement.

I'm sort of confused as to why the idea of not reading the underlying
page is so hard to understand.

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


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

Re: [HACKERS] DROP SUBSCRIPTION and ROLLBACK

[Petr Jelinek]

On 08/02/17 07:40, Masahiko Sawada wrote:
> On Wed, Feb 8, 2017 at 9:01 AM, Michael Paquier
>  wrote:
>> On Wed, Feb 8, 2017 at 1:30 AM, Fujii Masao  wrote:
>>> On Wed, Feb 8, 2017 at 12:26 AM, Petr Jelinek
>>>  wrote:
 For example what happens if apply crashes during the DROP
 SUBSCRIPTION/COMMIT and is not started because the delete from catalog
 is now visible so the subscription is no longer there?
>>>
>>> Another idea is to treat DROP SUBSCRIPTION in the same way as VACUUM, i.e.,
>>> make it emit an error if it's executed within user's transaction block.
>>
>> It seems to me that this is exactly Petr's point: using
>> PreventTransactionChain() to prevent things to happen.
> 
> Agreed. It's better to prevent to be executed inside user transaction
> block. And I understood there is too many failure scenarios we need to
> handle.
> 
>>> Also DROP SUBSCRIPTION should call CommitTransactionCommand() just
>>> after removing the entry from pg_subscription, then connect to the publisher
>>> and remove the replication slot.
>>
>> For consistency that may be important.
> 
> Agreed.
> 
> Attached patch, please give me feedback.
> 

This looks good (and similar to what initial patch had btw). Works fine
for me as well.

Remaining issue is, what to do about CREATE SUBSCRIPTION then, there are
similar failure scenarios there, should we prevent it from running
inside transaction as well?

-- 
  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] drop support for Python 2.3

[Devrim Gündüz]

Hi,

On Wed, 2017-02-08 at 09:16 -0500, Peter Eisentraut wrote:
> It appears that we don't have anything running 2.4.  A RHEL/CentOS 5
> system with standard components would be a good addition to the build farm.

I have CentOS 5 instances running on buildfarm. I'll register them via
buildfarm.pg.org soon.

Regards,
-- 
Devrim Gündüz
EnterpriseDB: http://www.enterprisedb.com
PostgreSQL Danışmanı/Consultant, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR


signature.asc
Description: This is a digitally signed message part

Re: [HACKERS] multivariate statistics (v19)

[Dean Rasheed]

On 6 February 2017 at 21:26, Alvaro Herrera  wrote:
> Tomas Vondra wrote:
>> On 02/01/2017 11:52 PM, Alvaro Herrera wrote:
>
>> > Nearby, some auxiliary functions such as n_choose_k and
>> > num_combinations are not documented. What it is that they do? I'd
>> > move these at the end of the file, keeping the important entry points
>> > at the top of the file.
>>
>> I'd say n-choose-k is pretty widely known term from combinatorics. The
>> comment would essentially say just 'this is n-choose-k' which seems rather
>> pointless. So as much as I dislike the self-documenting code, this actually
>> seems like a good case of that.
>
> Actually, we do have such comments all over the place.  I knew this as
> "n sobre k", so the english name doesn't immediately ring a bell with me
> until I look it up; I think the function comment could just say
> "n_choose_k -- this function returns the binomial coefficient".
>

One of the things you have to watch out for when writing code to
compute binomial coefficients is integer overflow, since the numerator
and denominator get large very quickly. For example, the current code
will overflow for n=13, k=12, which really isn't that large.

This can be avoided by computing the product in reverse and using a
larger datatype like a 64-bit integer to store a single intermediate
result. The point about multiplying the terms in reverse is that it
guarantees that each intermediate result is an exact integer (a
smaller binomial coefficient), so there is no need to track separate
numerators and denominators, and you avoid huge intermediate
factorials. Here's what that looks like in psuedo-code:

binomial(int n, int k):
# Save computational effort by using the symmetry of the binomial
# coefficients
k = min(k, n-k);

# Compute the result using binomial(n, k) = binomial(n-1, k-1) * n / k,
# starting from binomial(n-k, 0) = 1, and computing the sequence
# binomial(n-k+1, 1), binomial(n-k+2, 2), ...
#
# Note that each intermediate result is an exact integer.
int64 result = 1;
for (int i = 1; i <= k; i++)
{
result = (result * (n-k+i)) / i;
if (result > INT_MAX) Raise overflow error
}
return (int) result;


Note also that I think num_combinations(n) is just an expensive way of
calculating 2^n - n - 1.

Regards,
Dean


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

Re: [HACKERS] One-shot expanded output in psql using \gx

[David Fetter]

On Wed, Feb 08, 2017 at 03:52:40PM +0100, Christoph Berg wrote:
> Re: David Fetter 2017-02-07 <20170207051659.gc3...@fetter.org>
> > On Mon, Feb 06, 2017 at 08:54:13PM +0100, Christoph Berg wrote:
> > > The majority of voices here was in favor of using \gx, so here is
> > > another version of the same patch which implements that.
> > 
> > Patch is useful, and works as documented.
> > 
> > Maybe it could get a test or two in src/test/regress/*/psql.*
> 
> Good point. The new version tests \g and \gx with a new query, and
> re-running it on the last query buffer.
>   
> ! /* \g [filename] -- send query, optionally with output to file/pipe */
> ! else if (strcmp(cmd, "g") == 0)
>   {
>   char   *fname = psql_scan_slash_option(scan_state,
>   
>OT_FILEPIPE, NULL, false);
> --- 910,920 
>   free(fname);
>   }
>   
> ! /*
> !  * \g [filename] -- send query, optionally with output to file/pipe
> !  * \gx [filename] -- same as \g, with expanded mode forced
> !  */
> ! else if (strcmp(cmd, "g") == 0 || strcmp(cmd, "gx") == 0)
>   {
>   char   *fname = psql_scan_slash_option(scan_state,
>   
>OT_FILEPIPE, NULL, false);
> *** exec_command(const char *cmd,
> *** 924,929 
> --- 927,934 
>   pset.gfname = pg_strdup(fname);
>   }
>   free(fname);
> + if (strcmp(cmd, "gx") == 0)
> + pset.g_expanded = true;
>   status = PSQL_CMD_SEND;
>   }

Would you be open to saving the next person some work by doing
something similar to how \d is done, namely looking for an 'x'
modifier after g without regard to how far after?  As of this writing,
the \d version starts at line 398 in master.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

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


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

Re: [HACKERS] One-shot expanded output in psql using \gx

[Christoph Berg]

Re: David Fetter 2017-02-07 <20170207051659.gc3...@fetter.org>
> On Mon, Feb 06, 2017 at 08:54:13PM +0100, Christoph Berg wrote:
> > The majority of voices here was in favor of using \gx, so here is
> > another version of the same patch which implements that.
> 
> Patch is useful, and works as documented.
> 
> Maybe it could get a test or two in src/test/regress/*/psql.*

Good point. The new version tests \g and \gx with a new query, and
re-running it on the last query buffer.

Christoph
-- 
Senior Berater, Tel.: +49 2166 9901 187
credativ GmbH, HRB Mönchengladbach 12080, USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
pgp fingerprint: 5C48 FE61 57F4 9179 5970  87C6 4C5A 6BAB 12D2 A7AE
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
new file mode 100644
index ae58708..e0302ea
*** a/doc/src/sgml/ref/psql-ref.sgml
--- b/doc/src/sgml/ref/psql-ref.sgml
*** Tue Oct 26 21:40:57 CEST 1999
*** 1891,1896 
--- 1891,1908 
  
  

+ \gx [ filename ]
+ \gx [ |command ]
+ 
+ 
+ \gx is equivalent to \g, but
+ forces expanded output mode for this query.
+ 
+ 
+   
+ 
+ 
+   
  \gexec
  
  
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
new file mode 100644
index f17f610..6e140fe
*** a/src/bin/psql/command.c
--- b/src/bin/psql/command.c
*** exec_command(const char *cmd,
*** 910,917 
  		free(fname);
  	}
  
! 	/* \g [filename] -- send query, optionally with output to file/pipe */
! 	else if (strcmp(cmd, "g") == 0)
  	{
  		char	   *fname = psql_scan_slash_option(scan_state,
     OT_FILEPIPE, NULL, false);
--- 910,920 
  		free(fname);
  	}
  
! 	/*
! 	 * \g [filename] -- send query, optionally with output to file/pipe
! 	 * \gx [filename] -- same as \g, with expanded mode forced
! 	 */
! 	else if (strcmp(cmd, "g") == 0 || strcmp(cmd, "gx") == 0)
  	{
  		char	   *fname = psql_scan_slash_option(scan_state,
     OT_FILEPIPE, NULL, false);
*** exec_command(const char *cmd,
*** 924,929 
--- 927,934 
  			pset.gfname = pg_strdup(fname);
  		}
  		free(fname);
+ 		if (strcmp(cmd, "gx") == 0)
+ 			pset.g_expanded = true;
  		status = PSQL_CMD_SEND;
  	}
  
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
new file mode 100644
index 5349c39..4534bd9
*** a/src/bin/psql/common.c
--- b/src/bin/psql/common.c
*** PrintQueryTuples(const PGresult *results
*** 770,775 
--- 770,779 
  {
  	printQueryOpt my_popt = pset.popt;
  
+ 	/* one-shot expanded output requested via \gx */
+ 	if (pset.g_expanded)
+ 		my_popt.topt.expanded = true;
+ 
  	/* write output to \g argument, if any */
  	if (pset.gfname)
  	{
*** sendquery_cleanup:
*** 1410,1415 
--- 1414,1422 
  		pset.gfname = NULL;
  	}
  
+ 	/* reset \gx's expanded-mode flag */
+ 	pset.g_expanded = false;
+ 
  	/* reset \gset trigger */
  	if (pset.gset_prefix)
  	{
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
new file mode 100644
index 3e3cab4..2aece7e
*** a/src/bin/psql/help.c
--- b/src/bin/psql/help.c
*** slashUsage(unsigned short int pager)
*** 174,179 
--- 174,180 
  	fprintf(output, _("  \\copyright show PostgreSQL usage and distribution terms\n"));
  	fprintf(output, _("  \\errverboseshow most recent error message at maximum verbosity\n"));
  	fprintf(output, _("  \\g [FILE] or ; execute query (and send results to file or |pipe)\n"));
+ 	fprintf(output, _("  \\gx [FILE] as \\g, but force expanded output\n"));
  	fprintf(output, _("  \\gexec execute query, then execute each value in its result\n"));
  	fprintf(output, _("  \\gset [PREFIX] execute query and store results in psql variables\n"));
  	fprintf(output, _("  \\q quit psql\n"));
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
new file mode 100644
index 195f5a1..70ff181
*** a/src/bin/psql/settings.h
--- b/src/bin/psql/settings.h
*** typedef struct _psqlSettings
*** 91,96 
--- 91,97 
  	printQueryOpt popt;
  
  	char	   *gfname;			/* one-shot file output argument for \g */
+ 	bool		g_expanded;		/* one-shot expanded output requested via \gx */
  	char	   *gset_prefix;	/* one-shot prefix argument for \gset */
  	bool		gexec_flag;		/* one-shot flag to execute query's results */
  	bool		crosstab_flag;	/* one-shot request to crosstab results */
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
new file mode 100644
index 6e759d0..0bd2ae3
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*** psql_completion(const char *text, int st
*** 1338,1344 
  		"\\dm", "\\dn", "\\do", "\\dO", "\\dp", "\\drds", "\\ds", "\\dS",
  		"\\dt", "\\dT", "\\dv", "\\du", "\\dx", "\\dy",
  		"\\e", 

[HACKERS] chomp PQerrorMessage() in backend uses

[Peter Eisentraut]

Here is a patch to systematically trim the trailing newlines off
PQerrorMessage() results in backend uses (dblink, postgres_fdw,
libpqwalreceiver).

I noticed that there are some inconsistent assumptions about whether
PQerrorMessage() can ever return NULL.  From the code, I think that
should not be possible, but some code appears to be prepared for it.
Other code is not.  What is correct?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From d6835cff36bcc237462febf5ba7d3df7d560329f Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 8 Feb 2017 09:31:35 -0500
Subject: [PATCH] chomp PQerrorMessage() in backend uses

PQerrorMessage() returns an error message with a trailing newline, but
in backend use (dblink, postgres_fdw, libpqwalreceiver), we want to have
the error message without that for emitting via ereport().  To simplify
that, add a function pchomp() that returns a pstrdup'ed string with the
trailing newline characters removed.
---
 contrib/dblink/dblink.c| 16 ++--
 contrib/dblink/expected/dblink.out |  2 --
 contrib/postgres_fdw/connection.c  | 14 ++
 .../libpqwalreceiver/libpqwalreceiver.c| 30 +++---
 src/backend/utils/mmgr/mcxt.c  | 14 ++
 src/include/utils/palloc.h |  2 ++
 6 files changed, 41 insertions(+), 37 deletions(-)

diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index ac43c458cb..db0a8ba72d 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -166,7 +166,7 @@ typedef struct remoteConnHashEnt
 
 #define DBLINK_RES_INTERNALERROR(p2) \
 	do { \
-			msg = pstrdup(PQerrorMessage(conn)); \
+			msg = pchomp(PQerrorMessage(conn)); \
 			if (res) \
 PQclear(res); \
 			elog(ERROR, "%s: %s", p2, msg); \
@@ -204,7 +204,7 @@ typedef struct remoteConnHashEnt
 conn = PQconnectdb(connstr); \
 if (PQstatus(conn) == CONNECTION_BAD) \
 { \
-	msg = pstrdup(PQerrorMessage(conn)); \
+	msg = pchomp(PQerrorMessage(conn)); \
 	PQfinish(conn); \
 	ereport(ERROR, \
 			(errcode(ERRCODE_SQLCLIENT_UNABLE_TO_ESTABLISH_SQLCONNECTION), \
@@ -278,7 +278,7 @@ dblink_connect(PG_FUNCTION_ARGS)
 
 	if (PQstatus(conn) == CONNECTION_BAD)
 	{
-		msg = pstrdup(PQerrorMessage(conn));
+		msg = pchomp(PQerrorMessage(conn));
 		PQfinish(conn);
 		if (rconn)
 			pfree(rconn);
@@ -651,7 +651,7 @@ dblink_send_query(PG_FUNCTION_ARGS)
 	/* async query send */
 	retval = PQsendQuery(conn, sql);
 	if (retval != 1)
-		elog(NOTICE, "could not send query: %s", PQerrorMessage(conn));
+		elog(NOTICE, "could not send query: %s", pchomp(PQerrorMessage(conn)));
 
 	PG_RETURN_INT32(retval);
 }
@@ -1087,7 +1087,7 @@ storeQueryResult(volatile storeInfo *sinfo, PGconn *conn, const char *sql)
 	PGresult   *res;
 
 	if (!PQsendQuery(conn, sql))
-		elog(ERROR, "could not send query: %s", PQerrorMessage(conn));
+		elog(ERROR, "could not send query: %s", pchomp(PQerrorMessage(conn)));
 
 	if (!PQsetSingleRowMode(conn))		/* shouldn't fail */
 		elog(ERROR, "failed to set single-row mode for dblink query");
@@ -1366,8 +1366,8 @@ dblink_error_message(PG_FUNCTION_ARGS)
 	DBLINK_INIT;
 	DBLINK_GET_NAMED_CONN;
 
-	msg = PQerrorMessage(conn);
-	if (msg == NULL || msg[0] == '\0')
+	msg = pchomp(PQerrorMessage(conn));
+	if (msg[0] == '\0')
 		PG_RETURN_TEXT_P(cstring_to_text("OK"));
 	else
 		PG_RETURN_TEXT_P(cstring_to_text(msg));
@@ -2709,7 +2709,7 @@ dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
 	 * return NULL, not a PGresult at all.
 	 */
 	if (message_primary == NULL)
-		message_primary = PQerrorMessage(conn);
+		message_primary = pchomp(PQerrorMessage(conn));
 
 	if (res)
 		PQclear(res);
diff --git a/contrib/dblink/expected/dblink.out b/contrib/dblink/expected/dblink.out
index 5acaaf225a..4b6d26e574 100644
--- a/contrib/dblink/expected/dblink.out
+++ b/contrib/dblink/expected/dblink.out
@@ -377,7 +377,6 @@ FROM dblink('myconn','SELECT * FROM foo') AS t(a int, b text, c text[])
 WHERE t.a > 7;
 ERROR:  could not establish connection
 DETAIL:  missing "=" after "myconn" in connection info string
-
 -- create a named persistent connection
 SELECT dblink_connect('myconn',connection_parameters());
  dblink_connect 
@@ -604,7 +603,6 @@ FROM dblink('myconn','SELECT * FROM foo') AS t(a int, b text, c text[])
 WHERE t.a > 7;
 ERROR:  could not establish connection
 DETAIL:  missing "=" after "myconn" in connection info string
-
 -- create a named persistent connection
 SELECT dblink_connect('myconn',connection_parameters());
  dblink_connect 
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 7f7a744ac0..c6e3d44515 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -226,21 +226,11 @@ connect_pg_server(ForeignServer *server, UserMapping 

Re: [HACKERS] pageinspect: Hash index support

[Ashutosh Sharma]

>> 1) Check if an overflow page is a new page. If so, read a bitmap page
>> to confirm if a bit corresponding to this overflow page is clear or
>> not. For this, I would first add Assert statement to ensure that the
>> bit is clear and if it is, then set the statusbit as false indicating
>> that the page is free.
>>
>> 2) In case if an overflow page is not new, first verify if it is
>> really an overflow page and if so, check if the bit corresponding to
>> it in the bitmap page is SET. If so, set the statusbit as true; If
>> not, we would see an assertion failure happening.
>
> I think this is complicated and not what anybody wants.  I think you
> should do exactly what I said above: return true if the bit is set in
> the bitmap, and false if it isn't.  Full stop.  Don't read or do
> anything with the underlying page.  Only read the bitmap page.
>

Okay, As per the inputs from you, I have modified hash_bitmap_info()
and have tried to keep the things simple. Attached is the patch that
has this changes. Please have a look and let me know if you feel it is
not yet in the right shape. Thanks.


simplify_hash_bitmap_info_pageinspect.patch
Description: application/download

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

Re: [HACKERS] drop support for Python 2.3

[Peter Eisentraut]

On 2/7/17 11:49 PM, Tom Lane wrote:
> Hm, is there anything running 2.4 in the buildfarm?  If we're going to
> claim support for 2.4, we'd be well advised to test it.

It appears that we don't have anything running 2.4.  A RHEL/CentOS 5
system with standard components would be a good addition to the build farm.

-- 
Peter Eisentraut  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] LWLock optimization for multicore Power machines

[Bernd Helmle]

Am Dienstag, den 07.02.2017, 16:48 +0300 schrieb Alexander Korotkov:
> But win isn't
> as high as I observed earlier.  And I wonder why absolute numbers are
> lower
> than in our earlier experiments.  We used IBM E880 which is actually
> two

Did you run your tests on bare metal or were they also virtualized?

> nodes with interconnect.  However interconnect is not fast enough to
> make
> one PostgreSQL instance work on both nodes.  Thus, used half of IBM
> E880
> which has 4 sockets and 32 physical cores.  While you use IBM E850
> which is
> really single node with 4 sockets and 48 physical cores.  Thus, it
> seems
> that you have lower absolute numbers on more powerful hardware.  That
> makes
> me uneasy and I think we probably don't get the best from this
> hardware.
> Just in case, do you use SMT=8?

Yes, SMT=8 was used.

The machine has 4 sockets, 8 Core each, 3.7 GHz clock frequency. The
Ubuntu LPAR running on PowerVM isn't using all physical cores,
currently 28 cores are assigned (=224 SMT Threads). The other cores are
dedicated to the PowerVM hypervisor and a (very) small AIX LPAR.

I've done more pgbenches this morning with SMT-4, too, fastest result
with master was 

SMT-4

transaction type: 
scaling factor: 1000
query mode: prepared
number of clients: 100
number of threads: 100
duration: 300 s
number of transactions actually processed: 167306423
latency average = 0.179 ms
latency stddev = 0.072 ms
tps = 557685.144912 (including connections establishing)
tps = 557835.683204 (excluding connections establishing)

compared with SMT-8:

transaction type: 
scaling factor: 1000
query mode: prepared
number of clients: 100
number of threads: 100
duration: 300 s
number of transactions actually processed: 173476449
latency average = 0.173 ms
latency stddev = 0.059 ms
tps = 578250.676019 (including connections establishing)
tps = 578412.159601 (excluding connections establishing)

and retried with lwlocks-power-3, SMT-4:

transaction type: 
scaling factor: 1000
query mode: prepared
number of clients: 100
number of threads: 100
duration: 300 s
number of transactions actually processed: 185991995
latency average = 0.161 ms
latency stddev = 0.059 ms
tps = 619970.030069 (including connections establishing)
tps = 620112.263770 (excluding connections establishing)
credativ@iicl183:~/git/postgres$ 

...and SMT-8

transaction type: 
scaling factor: 1000
query mode: prepared
number of clients: 100
number of threads: 100
duration: 300 s
number of transactions actually processed: 185878717
latency average = 0.161 ms
latency stddev = 0.047 ms
tps = 619591.476154 (including connections establishing)
tps = 619655.867280 (excluding connections establishing)

Interestingly the lwlocks patch seems to decrease the SMT influence
factor.

Side note: the system makes around 2 Mio Context Switches during the
benchmarks, e.g.

awk '{print $12;}' /tmp/vmstat.out 

cs
10
2153533
2134864
2141623
2126845
2128330
2127454
2145325
2126769
2134492
2130246
2130071
2142660
2136077
2126783
2126107
2125823
2136511
2137752
2146307
2141127

I've also tried a more recent kernel this morning (4.4 vs. 4.8), but
this didn't change the picture. Is there anything more i can do?




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

Re: [HACKERS] Parallel bitmap heap scan

[Dilip Kumar]

On Wed, Feb 8, 2017 at 7:01 PM, Robert Haas  wrote:
> You can store whatever you want in SH_TYPE's private_data member.
> SH_ALLOCATE and SH_FREE both get a pointer to the SH_TYPE, so they
> have access to that.  Hmm, but there's no way to get that set in
> SH_CREATE before SH_ALLOCATE is called.  Maybe we need to add a
> private_data argument to SH_CREATE.  execGrouping.c could use that
> instead of frobbing private_data directly:
>
> -hashtable->hashtab = tuplehash_create(tablecxt, nbuckets);
> -hashtable->hashtab->private_data = hashtable;
> +hashtable->hashtab = tuplehash_create(tablecxt, nbuckets, hashtable);

Okay, will go ahead as you suggested. Patch attached for the same.


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


hash_create_fix.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 bitmap heap scan

[Robert Haas]

On Wed, Feb 8, 2017 at 1:59 AM, Dilip Kumar  wrote:
> IIUC, tbm_prepare_shared_iterate will be called only by the leader,
> for tbmiterator as well as for the prefetch_iterator. And,
> tbm_attach_shared_iterate will be called by each backend and for both
> the iterators.

That's what I had in mind.

> IMHO, tbm_attach_shared_iterate should return TBMIterator with
> reference to TBMSharedIterator inside it. The reason behind same is
> that we can not keep TBMIterateResult inside TBMSharedIterator
> otherwise, results will also go in shared memory but we want to have
> local result memory for each worker so that other worker doesn't
> disturb it.

No, I don't agree.  I think TBMSharedIterator should be an unshared
structure created by tbm_attach_shared_iterate, which can internally
contain backend-private state like a TBMIterateResult, and which can
also contain a pointer to the shared-memory structure previously
created by tbm_prepare_shared_iterate.  That thing needs to be called
something other than a TBMSharedIterator, like TBMSharedIterationState
or something.

-- 
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] Idea on how to simplify comparing two sets

[Robert Haas]

On Wed, Feb 8, 2017 at 4:24 AM, Pantelis Theodosiou  wrote:
> I'm not advocating it but I don't see how introducing new SQL keywords
> breaks backwards compatibility.

It does at least a little bit.  This starts failing:

select 1 new_keyword form blah;

(now you have to insert AS or quote the keyword)

If the new keyword is partially or fully reserved, then anybody who is
using that column in a now-impermissible way has to change names of
conflicting tables, columns, functions, etc.

But of course we do add keywords in every release, where it's
warranted by the value of the new feature.  I think the problem for
this proposed feature is not that adding new keywords is a completely
insane thing to do but that (1) there are lots of other good ways to
do more or less what is being requested with this new syntax, so it's
not clear that we need a new one and (2) there are cases where it
might be ambiguous which meaning of IS NOT DISTINCT FROM is met.
Joel's answer to #2 is to just prefer the existing meaning wherever
it's possible to assign that meaning and use the new meaning only in
cases where the existing meaning is impossible, but (2a) if you tried
to code it up, you'd probably find that it's quite difficult to make
bison generate a grammar that works that way, because bison isn't
designed around the idea of giving things a meaning only if they don't
already have one and (2b) apparently ambiguities can be confusing to
users even if they've been eliminated in the formal grammar.

-- 
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] Parallel bitmap heap scan

[Robert Haas]

On Wed, Feb 8, 2017 at 4:20 AM, Dilip Kumar  wrote:
> The new SH_CREATE(MemoryContext ctx, uint32 nelements) don't have any
> option to supply arguments to it. Our callback functions need access
> to TBM.
>
> Is it expected that if the user of SH_CREATE who doesn't want to pass
> a "MemoryContext" then we can pass arguments instead of ctx?

You can store whatever you want in SH_TYPE's private_data member.
SH_ALLOCATE and SH_FREE both get a pointer to the SH_TYPE, so they
have access to that.  Hmm, but there's no way to get that set in
SH_CREATE before SH_ALLOCATE is called.  Maybe we need to add a
private_data argument to SH_CREATE.  execGrouping.c could use that
instead of frobbing private_data directly:

-hashtable->hashtab = tuplehash_create(tablecxt, nbuckets);
-hashtable->hashtab->private_data = hashtable;
+hashtable->hashtab = tuplehash_create(tablecxt, nbuckets, hashtable);

-- 
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] Parallel bitmap heap scan

[Robert Haas]

On Wed, Feb 8, 2017 at 5:21 AM, Dilip Kumar  wrote:
> On Wed, Feb 8, 2017 at 3:44 AM, Robert Haas  wrote:
 +#ifndef SH_USE_NONDEFAULT_ALLOCATOR
 +
>>>
>>> That should probably be documented in the file header.
>>
>> Right.  OK, did that and a few other cleanups, and committed.
>
> I think we need to have prototype for the default allocator outside of
> #ifndef SH_USE_NONDEFAULT_ALLOCATOR. Because the file e.g. tidbitmap.c
> who wants to use SH_USE_NONDEFAULT_ALLOCATOR will provide the
> allocator function definition but it can not have the declaration of
> those function as that function take SH_TYPE as input and that will be
> only defined once we include the simplehash.h.
>
> So basically we can not declare prototype before including
> simplehash.h for allocator. And, if we don't declare we will get
> "implicit declaration warning" because simplehash itself is using
> those functions.
>
> The solution is simplehash.h, should always declare it, and provide
> the definitions only if SH_USE_NONDEFAULT_ALLOCATOR is not defined.
> Attached patch does that.

Makes sense, will commit.

-- 
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: [[Parallel] Shared] Hash

[Ashutosh Bapat]

>
> 0004-hj-refactor-batch-increases-v4.patch:
>
> Modify the existing hash join code to detect work_mem exhaustion at
> the point where chunks are allocated, instead of checking after every
> tuple insertion.  This matches the logic used for estimating, and more
> importantly allows for some parallelism in later patches.

The patch has three changes
1. change dense_alloc() to accept respect_workmem argument and use it
within the function.
2. Move call to ExecHashIncreaseNumBatches() into dense_alloc() from
ExecHashTableInsert() to account for memory before inserting new tuple
3. Check growEnabled before calling ExecHashIncreaseNumBatches().

I think checking growEnabled within ExecHashIncreaseNumBatches() is
more easy to maintain that checking at every caller. If someone is to
add a caller tomorrow, s/he has to remember to add the check.

It might be better to add some comments in
ExecHashRemoveNextSkewBucket() explaining why dense_alloc() should be
called with respect_work_mem = false? ExecHashSkewTableInsert() does
call ExecHashIncreaseNumBatches() after calling
ExecHashRemoveNextSkewBucket() multiple times, so it looks like we do
expect increase in space used and thus go beyond work_mem for a short
while. Is there a way we can handle this case in dense_alloc()?

Is it possible that increasing the number of batches changes the
bucket number of the tuple being inserted? If so, should we
recalculate the bucket and batch of the tuple being inserted?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] [PATCH] configure-time knob to set default ssl ciphers

[Pavel Raiskup]

On Wednesday, February 8, 2017 1:29:19 PM CET Pavel Raiskup wrote:
> On Wednesday, February 8, 2017 1:05:08 AM CET Tom Lane wrote:
> > Peter Eisentraut  writes:
> > > On 2/7/17 11:21 AM, Tom Lane wrote:
> > >> A compromise that might be worth considering is to introduce
> > >> #define PG_DEFAULT_SSL_CIPHERS "HIGH:MEDIUM:+3DES:!aNULL"
> > >> into pg_config_manual.h, which would at least give you a reasonably
> > >> stable target point for a long-lived patch.
> > 
> > > You'd still need to patch postgresql.conf.sample somehow.
> > 
> > Right.  The compromise position that I had in mind was to add the
> > #define in pg_config_manual.h and teach initdb to propagate it into
> > the installed copy of postgresql.conf, as we've done with other GUCs
> > with platform-dependent defaults, such as backend_flush_after.
> > 
> > That still leaves the question of what to do with the SGML docs.
> > We could add some weasel wording to the effect that the default might
> > be platform-specific, or we could leave the docs alone and expect the
> > envisioned Red Hat patch to patch config.sgml along with
> > pg_config_manual.h.
> 
> Thanks for quickt feedback.  Just to not give up too early, I'm attaching
> 2nd iteration.  I'm fine to fallback to pg_config_manual.h solution though,
> if this is considered too bad.
> 
> I tried to fix the docs now (crucial part indeed) so we are not that
> "scrict" and there's some space for per-distributor change of ssl_ciphers
> default.
> 
> From the previous mail:
> > I'm not really sure that we want to carry around that much baggage for a
> > single-system hack.
> 
> Accepted, but still I'm giving a chance.  OpenSSL maintainers predict this (or
> something else in similar fashion) is going to be invented in OpenSSL 
> upstream.
> So there's still some potential in ./configure option.

Argh :( !  Attaching now and sorry.

Pavel

> Thanks!
> Pavel
> 
> > It looks like the xxx_flush_after GUCs aren't exactly fully documented
> > as to this point, so we have some work to do there too :-(
> 
> 
> 
> > regards, tom lane
> > 
> 
> 

>From 41f73a910bb7afc2afa12be35a195df317f9447b Mon Sep 17 00:00:00 2001
From: Pavel Raiskup 
Date: Wed, 18 Jan 2017 13:34:55 +0100
Subject: [PATCH] Allow setting distribution-specific cipher set

Fedora OpenSSL maintainers invented a way to specify consolidated,
per-system cipher set [1] and it is our packaging policy to comply
(if this is a bit meaningful).

So for such situations ./configure options comes in handy instead
of downstream-patching, per Red Hat bug report [2].

[1] https://fedoraproject.org/wiki/Packaging:CryptoPolicies
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1348125
---
 configure | 34 +++
 configure.in  | 10 
 doc/src/sgml/config.sgml  |  3 ++-
 doc/src/sgml/installation.sgml| 15 
 src/backend/utils/misc/guc.c  |  2 +-
 src/backend/utils/misc/postgresql.conf.sample |  2 +-
 src/bin/initdb/initdb.c   |  4 
 src/include/pg_config.h.in|  3 +++
 8 files changed, 70 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
new file mode 100755
index bb285e4..15fad9e
*** a/configure
--- b/configure
*** with_bsd_auth
*** 831,836 
--- 831,837 
  with_ldap
  with_bonjour
  with_openssl
+ with_openssl_be_ciphers
  with_selinux
  with_systemd
  with_readline
*** Optional Packages:
*** 1521,1526 
--- 1522,1529 
--with-ldap build with LDAP support
--with-bonjour  build with Bonjour support
--with-openssl  build with OpenSSL support
+   --with-openssl-be-ciphers=STRING
+   Replace the default list of server-supported ciphers
--with-selinux  build with SELinux support
--with-systemd  build with systemd support
--without-readline  do not use GNU Readline nor BSD Libedit for editing
*** fi
*** 5712,5717 
--- 5715,5751 
  $as_echo "$with_openssl" >&6; }
  
  
+ pg_be_ciphers=HIGH:MEDIUM:+3DES:!aNULL
+ 
+ 
+ 
+ # Check whether --with-openssl-be-ciphers was given.
+ if test "${with_openssl_be_ciphers+set}" = set; then :
+   withval=$with_openssl_be_ciphers;
+   case $withval in
+ yes)
+   as_fn_error $? "argument required for --with-openssl-be-ciphers option" "$LINENO" 5
+   ;;
+ no)
+   as_fn_error $? "argument required for --with-openssl-be-ciphers option" "$LINENO" 5
+   ;;
+ *)
+   pg_be_ciphers=$withval
+   ;;
+   esac
+ 
+ fi
+ 
+ 
+ 
+ cat >>confdefs.h <<_ACEOF
+ #define PG_DEFAULT_SSL_CIPHERS 

Re: [HACKERS] [PATCH] configure-time knob to set default ssl ciphers

[Pavel Raiskup]

On Wednesday, February 8, 2017 1:05:08 AM CET Tom Lane wrote:
> Peter Eisentraut  writes:
> > On 2/7/17 11:21 AM, Tom Lane wrote:
> >> A compromise that might be worth considering is to introduce
> >> #define PG_DEFAULT_SSL_CIPHERS "HIGH:MEDIUM:+3DES:!aNULL"
> >> into pg_config_manual.h, which would at least give you a reasonably
> >> stable target point for a long-lived patch.
> 
> > You'd still need to patch postgresql.conf.sample somehow.
> 
> Right.  The compromise position that I had in mind was to add the
> #define in pg_config_manual.h and teach initdb to propagate it into
> the installed copy of postgresql.conf, as we've done with other GUCs
> with platform-dependent defaults, such as backend_flush_after.
> 
> That still leaves the question of what to do with the SGML docs.
> We could add some weasel wording to the effect that the default might
> be platform-specific, or we could leave the docs alone and expect the
> envisioned Red Hat patch to patch config.sgml along with
> pg_config_manual.h.

Thanks for quickt feedback.  Just to not give up too early, I'm attaching
2nd iteration.  I'm fine to fallback to pg_config_manual.h solution though,
if this is considered too bad.

I tried to fix the docs now (crucial part indeed) so we are not that
"scrict" and there's some space for per-distributor change of ssl_ciphers
default.

>From the previous mail:
> I'm not really sure that we want to carry around that much baggage for a
> single-system hack.

Accepted, but still I'm giving a chance.  OpenSSL maintainers predict this (or
something else in similar fashion) is going to be invented in OpenSSL upstream.
So there's still some potential in ./configure option.

Thanks!
Pavel

> It looks like the xxx_flush_after GUCs aren't exactly fully documented
> as to this point, so we have some work to do there too :-(



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

[amul sul]

Hi Amit,

Regarding following code in ATExecDropNotNull function, I don't see
any special check for RANGE partitioned, is it intended to have same
restriction for LIST partitioned too, I guess not?

  /*
 * If the table is a range partitioned table, check that the column is not
 * in the partition key.
 */
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
{
PartitionKey key = RelationGetPartitionKey(rel);
int partnatts = get_partition_natts(key),
i;

for (i = 0; i < partnatts; i++)
{
AttrNumber  partattnum = get_partition_col_attnum(key, i);

if (partattnum == attnum)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
 errmsg("column \"%s\" is in range partition key",
colName)));
}
}

Regards,
Amul


-- 
Sent 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: About CMake v2

[Yuriy Zhuravlev]

2017-01-28 1:50 GMT+03:00 Michael Paquier :

> On Fri, Jan 27, 2017 at 11:09 PM, Peter Eisentraut
>  wrote:
> > On 1/24/17 8:37 AM, Tom Lane wrote:
> >> Craig Ringer  writes:
> >>> Personally I think we should aim to have this in as a non default build
> >>> mode in pg10 if it can be made ready, and aim to make it default in
> pg11 at
> >>> least for Windows.
> >>
> >> AFAIK we haven't committed to accepting this at all, let alone trying
> >> to do so on a tight schedule.  And I believe there was general agreement
> >> that we would not accept it as something to maintain in parallel with
> >> the existing makefiles.  If we have to maintain two build systems, we
> >> have that already.
> >
> > My preferred scenario would be to replace the Windows build system by
> > this first, then refine it, then get rid of Autoconf.
> >
> > The ideal timeline would be to have a ready patch to commit early in a
> > development cycle, then get rid of the Windows build system by the end
> > of it.  Naturally, this would need buy-in from Windows developers.
>
> This looks like a really good plan to me.


I think it's best plan because when this patch will be in Postgres guys
from community can test it for Unix systems too.
Support two build systems it's not big deal really. I have been working on
this past year without any big troubles.
Also we have second perl build system...

Re: [HACKERS] WIP: About CMake v2

[Yuriy Zhuravlev]

>
> I don't understand what this has to do with cmake.  If this is a
> worthwhile improvement for the Windows build, then please explain why,
> with a "before" and "after" output and a patch for the existing build
> system as well.

During the porting process, I meet such situations when I should fix
something. It's happening because I build with different way also current
build system is trying to avoid many sharp corners.
If talk about this situation - without strict mode many "floats" checks
don't work correctly. You can read the link above. Besides this option puts
by build system. I think we can make a new thread for this approach. (with
patch for current perl system)

It might also be worth refactoring the existing Autoconf code here to
> make this consistent.


I do it because it's convenient in CMake. I can change this it's not big
deal.

Please explain what the circular dependency is.  If there is one, we
> should also side-port this change.


It's an important part.  I have a rule for generate rijndael.tbl by
gen-rtab who make from rijndael.c (with special define) who include
rijndael.tbl .
If I generate rijndael.tbl it's to force build gen-rtab and generate
rijndael.tbl again.
CMake knows about "includes" in files but we can make the wraparound macro
to hide include.

This patch removes the uuid.h include but doesn't add it anywhere else.

How does it work?


CMake sends to compiler right place for uuid.h (I mean -I/usr/include and
etc for gcc).


> Yeah, I think this is how the MSVC stuff effectively works right now as
> well.


I glad to hear it.

2017-01-03 17:11 GMT+03:00 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com>:

> On 12/30/16 9:10 AM, Yuriy Zhuravlev wrote:
> > cmake_v2_2_c_define.patch
> >
> > Small chages in c.h . At first it is “#pragma fenv_access (off)” it is
> > necessary if we use /fp:strict for MSVC compiler. Without this pragma we
> > can’t calc floats for const variables in compiller time (2 * M_PI for
> > example). Strict mode important if we want to be close with ieee754
> > float format on MSVC (1.0 / 0.0 = inf for example).  Detail info here:
> > https://msdn.microsoft.com/en-us/library/e7s85ffb.aspx
>
> I don't understand what this has to do with cmake.  If this is a
> worthwhile improvement for the Windows build, then please explain why,
> with a "before" and "after" output and a patch for the existing build
> system as well.
>
> > Second change is because I find and set HAVE_INT128 directly from CMake.
> > PG_INT128_TYPE used only for autoconfig scripts.
>
> It might also be worth refactoring the existing Autoconf code here to
> make this consistent.
>
> (My assumption is that if we were to move forward with cmake or any
> other build system change, we would have to keep the old one alongside
> at least for a little while.  So any changes to the C code would need to
> be side-ported.)
>
> > cmake_v2_3_rijndael.patch
> >
> > First I added special wraparound because here CMake have circular
> > dependency (cmake very smart here). Second I removed rijndael.tbl
> > because it generated during build process every time.
>
> Please explain what the circular dependency is.  If there is one, we
> should also side-port this change.
>
> > cmake_v2_4_uuid.patch
> >
> > Another small patch. Right place for uuid.h I find by CMake and not
> > necessary this ifdef hell.
>
> This patch removes the uuid.h include but doesn't add it anywhere else.
> How does it work?
>
> > Questions for discussion:
> >
> > In generated project by CMake we always have only one enter point. Also
> > INSTALL macross support only including to “all” targets. It follows that
> > it is impossible build contrib modules separately only with “all”
> > target. Here write about this behavior:
> > https://cmake.org/cmake/help/v3.7/prop_tgt/EXCLUDE_FROM_ALL.html
>
> Yeah, I think this is how the MSVC stuff effectively works right now as
> well.
>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] pg_stat_wal_write statistics view

[Amit Kapila]

On Tue, Feb 7, 2017 at 11:47 AM, Haribabu Kommi
 wrote:
> Hi Hackers,
>
> I just want to discuss adding of a new statistics view that provides
> the information of wal writing details as follows
>

+1.  I think it will be useful to observe WAL activity.

> postgres=# \d pg_stat_wal_writer
> View "pg_catalog.pg_stat_wal_writer"
> Column |   Type   | Collation | Nullable |
> Default
> ---+--+---+--+-
>  num_backend_writes   | bigint   |   |
> |
>  num_total_writes  | bigint   |   |  |
>  num_blocks  | bigint   |   |  |
>  total_write_time   | bigint|   |  |
>  stats_reset   | timestamp with time zone |   |  |
>
> The columns of the view are
> 1. Total number of xlog writes that are called from the backend.
> 2. Total number of xlog writes that are called from both backend
>  and background workers. (This column can be changed to just
>  display on the background writes).
> 3. The number of the blocks that are written.
> 4. Total write_time of the IO operation it took, this variable data is
> filled only when the track_io_timing GUC is enabled.

So, here is *write_time* the total time system has spent in WAL
writing before the last reset?

I think there should be a separate column for write and sync time.


> Or it is possible to integrate the new columns into the existing
> pg_stat_bgwriter view also.
>

I feel separate view is better.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: 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] Parallel tuplesort (for parallel B-Tree index creation)

[Thomas Munro]

On Wed, Feb 8, 2017 at 8:40 PM, Thomas Munro
 wrote:
> On Tue, Feb 7, 2017 at 5:43 PM, Peter Geoghegan  wrote:
>> Does anyone have any suggestions on how to tackle this?
>
> Hmm.  One approach might be like this:
>
> [hand-wavy stuff]

Thinking a bit harder about this, I suppose there could be a kind of
object called a SharedBufFileManager (insert better name) which you
can store in a DSM segment.  The leader backend that initialises a DSM
segment containing one of these would then call a constructor function
that sets an internal refcount to 1 and registers an on_dsm_detach
callback for its on-detach function.  All worker backends that attach
to the DSM segment would need to call an attach function for the
SharedBufFileManager to increment a refcount and also register the
on_dsm_detach callback, before any chance that an error might be
thrown (is that difficult?); failure to do so could result in file
leaks.  Then, when a BufFile is to be shared (AKA exported, made
unifiable), a SharedBufFile object can be initialised somewhere in the
same DSM segment and registered with the SharedBufFileManager.
Internally all registered SharedBufFile objects would be linked
together using offsets from the start of the DSM segment for link
pointers.  Now when SharedBufFileManager's on-detach function runs, it
decrements the refcount in the SharedBufFileManager, and if that
reaches zero then it runs a destructor that spins through the list of
SharedBufFile objects deleting files that haven't already been deleted
explicitly.

I retract the pin/unpin and per-file refcounting stuff I mentioned
earlier.  You could make the default that all files registered with a
SharedBufFileManager survive until the containing DSM segment is
detached everywhere using that single refcount in the
SharedBufFileManager object, but also provide a 'no really delete this
particular shared file now' operation for client code that knows it's
safe to do that sooner (which would be the case for me, I think).  I
don't think per-file refcounts are needed.

There are a couple of problems with the above though.  Firstly, doing
reference counting in DSM segment on-detach hooks is really a way to
figure out when the DSM segment is about to be destroyed by keeping a
separate refcount in sync with the DSM segment's refcount, but it
doesn't account for pinned DSM segments.  It's not your use-case or
mine currently, but someone might want a DSM segment to live even when
it's not attached anywhere, to be reattached later.  If we're trying
to use DSM segment lifetime as a scope, we'd be ignoring this detail.
Perhaps instead of adding our own refcount we need a new kind of hook
on_dsm_destroy.  Secondly, I might not want to be constrained by a
fixed-sized DSM segment to hold my SharedBufFile objects... there are
cases where I need to shared a number of batch files that is unknown
at the start of execution time when the DSM segment is sized (I'll
write about that shortly on the Parallel Shared Hash thread).  Maybe I
can find a way to get rid of that requirement.  Or maybe it could
support DSA memory too, but I don't think it's possible to use
on_dsm_detach-based cleanup routines that refer to DSA memory because
by the time any given DSM segment's detach hook runs, there's no
telling which other DSM segments have been detached already, so the
DSA area may already have partially vanished; some other kind of hook
that runs earlier would be needed...

Hmm.

-- 
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] Parallel bitmap heap scan

[Dilip Kumar]

On Wed, Feb 8, 2017 at 3:44 AM, Robert Haas  wrote:
>>> +#ifndef SH_USE_NONDEFAULT_ALLOCATOR
>>> +
>>
>> That should probably be documented in the file header.
>
> Right.  OK, did that and a few other cleanups, and committed.

I think we need to have prototype for the default allocator outside of
#ifndef SH_USE_NONDEFAULT_ALLOCATOR. Because the file e.g. tidbitmap.c
who wants to use SH_USE_NONDEFAULT_ALLOCATOR will provide the
allocator function definition but it can not have the declaration of
those function as that function take SH_TYPE as input and that will be
only defined once we include the simplehash.h.

So basically we can not declare prototype before including
simplehash.h for allocator. And, if we don't declare we will get
"implicit declaration warning" because simplehash itself is using
those functions.

The solution is simplehash.h, should always declare it, and provide
the definitions only if SH_USE_NONDEFAULT_ALLOCATOR is not defined.
Attached patch does that.


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


hash_allocate_fix.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 : For Auto-Prewarm.

[Amit Kapila]

On Tue, Feb 7, 2017 at 5:14 PM, Mithun Cy  wrote:
> On Tue, Feb 7, 2017 at 12:24 PM, Amit Kapila  wrote:
>> On Tue, Feb 7, 2017 at 11:53 AM, Beena Emerson  
>> wrote:
>>> Are 2 workers required?
>>>
>>
>> I think in the new design there is a provision of launching the worker
>> dynamically to dump the buffers, so there seems to be a need of
>> separate workers for loading and dumping the buffers.  However, there
>> is no explanation in the patch or otherwise when and why this needs a
>> pair of workers.  Also, if the dump interval is greater than zero,
>> then do we really need to separately register a dynamic worker?
>
> We have introduced a new value  -1 for pg_prewarm.dump_interval this
> means we will not dump at all, At this state, I thought auto
> pg_prewarm process need not run at all, so I coded to exit the auto
> pg_prewarm immediately. But If the user decides to start the auto
> pg_prewarm to dump only without restarting the server, I have
> introduced a launcher function "launch_pg_prewarm_dump" to restart the
> auto pg_prewarm only to dump. Since now we can launch worker only to
> dump, I thought we can redistribute the code between two workers, one
> which only does prewarm (load only) and another dumps periodically.
> This helped me to modularize and reuse the code. So once load worker
> has finished its job, it registers a dump worker and then exists.
> But if max_worker_processes is not enough to launch the "auto
> pg_prewarm dump" bgworker
> We throw an error
> 2017-02-07 14:51:59.789 IST [50481] ERROR:  registering dynamic
> bgworker "auto pg_prewarm dump" failed c
> 2017-02-07 14:51:59.789 IST [50481] HINT:  Consider increasing
> configuration parameter "max_worker_processes".
>
> Now thinking again instead of such error and then correcting same by
> explicitly launching the auto pg_prewarm dump bgwroker through
> launch_pg_prewarm_dump(), I can go back to original design where there
> will be one worker which loads and then dumps periodically. And
> launch_pg_prewarm_dump will relaunch dump only activity of that
> worker. Does this sound good?
>

Won't it be simple if you consider -1 as a value to just load library?
 For *_interval = -1, it will neither load nor dump.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: 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] Idea on how to simplify comparing two sets

[Pantelis Theodosiou]

On Tue, Feb 7, 2017 at 3:13 PM, Joel Jacobson  wrote:

>  Hi hackers,
>
> Currently there is no simple way to check if two sets are equal.
>
> Looks like no RDBMS in the world has a simple command for it.
>
> You have to do something like:
>
> ...
>
> Introducing new SQL keywords is of course not an option,
> since it would possibly break backwards compatibility.
>
>
I'm not advocating it but I don't see how introducing new SQL keywords
breaks backwards compatibility.

Pantelis Theodosiou

Re: [HACKERS] Parallel bitmap heap scan

[Dilip Kumar]

On Wed, Feb 8, 2017 at 3:44 AM, Robert Haas  wrote:
>>> +#ifndef SH_USE_NONDEFAULT_ALLOCATOR
>>> +
>>
>> That should probably be documented in the file header.
>
> Right.  OK, did that and a few other cleanups, and committed.

The new SH_CREATE(MemoryContext ctx, uint32 nelements) don't have any
option to supply arguments to it. Our callback functions need access
to TBM.

Is it expected that if the user of SH_CREATE who doesn't want to pass
a "MemoryContext" then we can pass arguments instead of ctx?

something like this ?
if (!tbm->dsa)
   tbm->pagetable = pagetable_create(tbm->mcxt, 128);
else
   tbm->pagetable = pagetable_create((MemoryContext)tbm, 128);

And, In allocation function, we can access this context and typecast to tbm?

As shown below.
static void *
pagetable_allocate(pagetable_hash *pagetable, Size size)
{
TIDBitmap  *tbm = pagetable->ctx;


So Is it expected to do like I explained above, or we missed to have
an arg parameter to SH_CREATE as well as in SH_TYPE structure or there
is some other way you have in mind?

-- 
Regards,
Dilip Kumar
EnterpriseDB: 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] Idea on how to simplify comparing two sets

[Pantelis Theodosiou]

On Tue, Feb 7, 2017 at 3:58 PM, Tom Lane  wrote:

> Joel Jacobson  writes:
> > Currently there is no simple way to check if two sets are equal.
>
> Uh ... maybe check whether SELECT set1 EXCEPT SELECT set2
> and SELECT set2 EXCEPT SELECT set1 are both empty?
>
> regards, tom lane
>
>
> Yes, if the wanted result is true or false, something like this:

SELECT EXISTS (TABLE a EXCEPT TABLE b)
OR EXISTS (TABLE b EXCEPT TABLE a) ;


And if a new operator was added (in the same category as UNION and
EXCEPT), it could be:


SELECT EXISTS (TABLE a XORSET TABLE b) ;

What about using the = and <> operators in sets? Is the following
allowed in the standard?


SELECT (TABLE a) <> (TABLE b) ;