Re: [HACKERS] jsonb_set array append hack?

[Thom Brown]

On 20 September 2015 at 16:17, Dmitry Dolgov <9erthali...@gmail.com> wrote:

> I'm sorry, but I'm not sure, what behavior is expected in this case?
> Right now the following logic was implemented:
> "we trying to set an element inside an array, but we've got a
> non-integer path item
> ("nonsense" in this particular case), so we're going to add a new
> element at the end of array by default"
>
> If it's wrong, should we refuse to perform such kind of operations, or
> should we replace
> "vehicle_type": ["car", "van"]
> to
> "vehicle_type: {"nonsense": "motorcycle"}
> ?
>

(please bottom-post)

I would expect some kind of error.  We're trying to address a position in
an array, and we're instead passing a key.  If it completes successfully,
the chances are it isn't what the user intended.

Thom

Re: [HACKERS] Speed up Clog Access by increasing CLOG buffers

[Peter Geoghegan]

On Mon, Aug 31, 2015 at 9:49 PM, Amit Kapila  wrote:
> Increasing CLOG buffers to 64 helps in reducing the contention due to second
> reason.  Experiments revealed that increasing CLOG buffers only helps
> once the contention around ProcArrayLock is reduced.

There has been a lot of research on bitmap compression, more or less
for the benefit of bitmap index access methods.

Simple techniques like run length encoding are effective for some
things. If the need to map the bitmap into memory to access the status
of transactions is a concern, there has been work done on that, too.
Byte-aligned bitmap compression is a technique that might offer a good
trade-off between compression clog, and decompression overhead -- I
think that there basically is no decompression overhead, because set
operations can be performed on the "compressed" representation
directly. There are other techniques, too.

Something to consider. There could be multiple benefits to compressing
clog, even beyond simply avoiding managing clog buffers.

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

[Robert Haas]

On Fri, Sep 18, 2015 at 4:43 PM, Vladimir Borodin  wrote:
> No, probably you misunderstood the results, let me explain one more time.

Yeah, I did, sorry.

> Unpatched PostgreSQL from REL9_4_STABLE gave 15500 tps. Version with timings
> - 14500 tps which is 6,5% worse. Version with sampling wait events every 10
> ms gave 13800 tps (11% worse than unpatched and 5% worse than with timings).

OK.

So, I don't really care about the timing stuff right now.  I want to
get the stuff without timings done first.  To do that, we need to come
to an agreement on how much information we're going to try to expose
and from where, and we need to make sure that doing that doesn't cause
a performance hit.  Then, if we want to add timing as an optional
feature for people who can tolerate the overhead, that's fine.

-- 
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] [BUGS] BUG #12989: pg_size_pretty with negative values

[Adrian.Vondendriesch]

Hi all,

Am 06.04.2015 um 20:52 schrieb Tom Lane:
> "David G. Johnston"  writes:
>> I'll let a hacker determine whether this is a bug or a feature request
>> though it is a POLA violation in either case.
> 
> I'd say it's a feature request --- a perfectly reasonable one, but I doubt
> we'd alter the behavior of the function in the back branches.

I was also wondering about the described behaviour. IMO pg_size_pretty
should handle negative values the same way as positive values are handled.

I've attached a patch which implements the requested behaviour. The
patch applies clean to HEAD (currently 85eda7e92).

AFAICS the documentation doesn't say anything about pg_size_pretty and
negative values. So, I didn't touch the documentation. If this is a
oversight by me or should be documented after all, I will provide a
additional documentation patch.

Before the patch:

> SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 
> 100::bigint) foo(size);
>  pg_size_pretty | pg_size_pretty 
> +
>  91 TB  | -100 bytes
> (1 row)

> SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 
> 100::numeric) foo(size);
>  pg_size_pretty | pg_size_pretty 
> +
>  91 TB  | -100 bytes
> (1 row)

After the patch:

> SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 
> 100::bigint) foo(size);
>  pg_size_pretty | pg_size_pretty 
> +
>  91 TB  | -91 TB
> (1 row)

> SELECT pg_size_pretty(size), pg_size_pretty(-1 * size) FROM (SELECT 
> 100::numeric) foo(size);
>  pg_size_pretty | pg_size_pretty 
> +
>  91 TB  | -91 TB
> (1 row)


The patch contains two tests (pg_size_pretty_bigint and
pg_size_pretty_numeric), to verify that positive and negative values
return the same result (except sign).

Greetings,

 - Adrian
From 08ae6cdeaf261e156ce5f7622f0d7426c1249485 Mon Sep 17 00:00:00 2001
From: Adrian Vondendriesch 
Date: Sat, 19 Sep 2015 15:54:13 +0200
Subject: [PATCH] Make pg_size_pretty handle negative values.

Make pg_size_pretty(bigint) and pg_size_pretty(numeric) handle
negative values the same way as positive values are.

This commit introduces a new macro "Sign", which is used within
pg_size_pretty(bigint).  Also numeric_plus_one_over_two is renamed to
numeric_divide_by_two.  numeric_divide_by_two now handles negative
values the same way as positive values are handled.  To get the
absolute value of a Numeric variable, a new static function called
numeric_absolute is introduced.
---
 src/backend/utils/adt/dbsize.c | 61 +++---
 src/include/c.h|  6 ++
 .../regress/expected/pg_size_pretty_bigint.out | 39 +++
 .../regress/expected/pg_size_pretty_numeric.out| 75 ++
 src/test/regress/sql/pg_size_pretty_bigint.sql | 15 +
 src/test/regress/sql/pg_size_pretty_numeric.sql| 29 +
 6 files changed, 203 insertions(+), 22 deletions(-)
 create mode 100644 src/test/regress/expected/pg_size_pretty_bigint.out
 create mode 100644 src/test/regress/expected/pg_size_pretty_numeric.out
 create mode 100644 src/test/regress/sql/pg_size_pretty_bigint.sql
 create mode 100644 src/test/regress/sql/pg_size_pretty_numeric.sql

diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index 82311b4..97095bb 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -534,31 +534,31 @@ pg_size_pretty(PG_FUNCTION_ARGS)
 	int64		limit = 10 * 1024;
 	int64		limit2 = limit * 2 - 1;
 
-	if (size < limit)
+	if (Abs(size) < limit)
 		snprintf(buf, sizeof(buf), INT64_FORMAT " bytes", size);
 	else
 	{
 		size >>= 9;/* keep one extra bit for rounding */
-		if (size < limit2)
+		if (Abs(size) < limit2)
 			snprintf(buf, sizeof(buf), INT64_FORMAT " kB",
-	 (size + 1) / 2);
+	 (size + Sign(size)) / 2);
 		else
 		{
 			size >>= 10;
-			if (size < limit2)
+			if (Abs(size) < limit2)
 snprintf(buf, sizeof(buf), INT64_FORMAT " MB",
-		 (size + 1) / 2);
+		 (size + Sign(size)) / 2);
 			else
 			{
 size >>= 10;
-if (size < limit2)
+if (Abs(size) < limit2)
 	snprintf(buf, sizeof(buf), INT64_FORMAT " GB",
-			 (size + 1) / 2);
+			 (size + Sign(size)) / 2);
 else
 {
 	size >>= 10;
 	snprintf(buf, sizeof(buf), INT64_FORMAT " TB",
-			 (size + 1) / 2);
+			 (size + Sign(size)) / 2);
 }
 			}
 		}
@@ -593,16 +593,37 @@ numeric_is_less(Numeric a, Numeric b)
 }
 
 static Numeric
-numeric_plus_one_over_two(Numeric n)
+numeric_absolute(Numeric n)
 {
 	Datum		d = NumericGetDatum(n);
+	Datum		result;
+
+	result = DirectFunctionCall1(numeric_abs, d);
+	return DatumGetNumeric(result);
+}
+

Re: [HACKERS] WIP: Rework access method interface

[Petr Jelinek]

On 2015-09-18 14:58, Alexander Korotkov wrote:

On Wed, Sep 16, 2015 at 8:44 PM, Teodor Sigaev > wrote:

validate_opclass was renamed to amvalidate.


It seems to me, that amvalidate method of AM should get as argument
only Oid of operator family. Layout and meaning of amproc/amop
fields are differ for different AM and there isn't an AM which
implements all possible features.

After, further personal discussion with Teodor, we decided that
amvalidate is out of scope for this patch.
It's not evident what should we validate in amvalidate and which way. I
think if we need amvalidate it should be subject of separate patch.
The attached patch exposes index access method parameters to SQL using
following fucntions:
  * get_am_param_oid(oid, text)
  * get_am_param_int(oid, text)
  * get_am_param_bool(oid, text)



Hmm, we might want these functons in any case (although I think just one 
function which would return all am params would be better).


But why is it not evident? We do the validations in regression tests, 
even if we just copy those then it's enough for a start.


If you mean that it's not obvious what are all the possible things that 
amvalidate should validate in the future, then yes, that will always be 
the case though. I think better solution would be to provide more 
granular validation interface in the C api (i.e. have amvalidateopclass 
etc). We can still have just one SQL exposed function called amvalidate 
which will call all those C interfaces that are supported by current 
version (I agree that those interfaces like amvalidateopclass should 
accept just Oid).


--
 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] WIP: Rework access method interface

[Alexander Korotkov]

On Sun, Sep 20, 2015 at 5:02 PM, Petr Jelinek  wrote:

> On 2015-09-18 14:58, Alexander Korotkov wrote:
>
>> On Wed, Sep 16, 2015 at 8:44 PM, Teodor Sigaev > > wrote:
>>
>> validate_opclass was renamed to amvalidate.
>>
>>
>> It seems to me, that amvalidate method of AM should get as argument
>> only Oid of operator family. Layout and meaning of amproc/amop
>> fields are differ for different AM and there isn't an AM which
>> implements all possible features.
>>
>> After, further personal discussion with Teodor, we decided that
>> amvalidate is out of scope for this patch.
>> It's not evident what should we validate in amvalidate and which way. I
>> think if we need amvalidate it should be subject of separate patch.
>> The attached patch exposes index access method parameters to SQL using
>> following fucntions:
>>   * get_am_param_oid(oid, text)
>>   * get_am_param_int(oid, text)
>>   * get_am_param_bool(oid, text)
>>
>>
> Hmm, we might want these functons in any case (although I think just one
> function which would return all am params would be better).
>
> But why is it not evident? We do the validations in regression tests, even
> if we just copy those then it's enough for a start
>

The reason is that those validations were used only in regression tests
yet. It wasn't used for user-defined operator classes. User might define
invalid opclass and then alter it to valid. Or user might upgrade opclass
in two steps where intermediate step is invalid. This is why I think
validating opclasses in CREATE/ALTER command is not evident since it
changes user visible behavior and compatibility.
Simultaneously, implementing new API function just for regression tests
doesn't seem to worth it for me.

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

Re: [HACKERS] jsonb_set array append hack?

[Dmitry Dolgov]

I'm sorry, but I'm not sure, what behavior is expected in this case?
Right now the following logic was implemented:
"we trying to set an element inside an array, but we've got a
non-integer path item
("nonsense" in this particular case), so we're going to add a new
element at the end of array by default"

If it's wrong, should we refuse to perform such kind of operations, or
should we replace
"vehicle_type": ["car", "van"]
to
"vehicle_type: {"nonsense": "motorcycle"}
?

On 15 September 2015 at 01:59, Andrew Dunstan  wrote:

>
>
> On 09/14/2015 01:29 PM, Thom Brown wrote:
>
>> Hi,
>>
>> I've noticed that if you use a string for an element key in jsonb_set
>> with create_missing set to true, you can use it to append to an array:
>>
>> postgres=# SELECT jsonb_set(
>> '{"name": "Joe", "vehicle_types": ["car","van"]}'::jsonb,
>>'{vehicle_types,nonsense}',
>>'"motorcycle"', true);
>> jsonb_set
>> 
>>  {"name": "Joe", "vehicle_types": ["car", "van", "motorcycle"]}
>> (1 row)
>>
>> What this really should match is a nested element inside "vehicle_types"
>> called "nonsense".  But this seems to be a hack to get an element added to
>> an array.  To do it properly currently requires specifying an arbitrary
>> number in the hope that it will exceed the number of elements you have in
>> the array.
>>
>
>
> That's a bug and we should fix it.
>
>
>
>> e.g.
>>
>> postgres=# SELECT jsonb_set(
>>'{"name": "Joe", "vehicle_types": ["car","van"]}'::jsonb,
>>'{vehicle_types,10}',
>>'"motorcycle"', true);
>> jsonb_set
>> 
>>  {"name": "Joe", "vehicle_types": ["car", "van", "motorcycle"]}
>> (1 row)
>>
>> But I'm guessing people shouldn't be relying on the hack in the first
>> example.  Isn't this a bug?  If so, wouldn't this also be a bug?:
>>
>> postgres=# SELECT jsonb_set(
>>'{"name": "Joe", "vehicle_types": ["car","van"]}'::jsonb,
>>array['vehicle_types',NULL],
>>'"motorcycle"', true);
>>
>>
>>
> I think that's a bug too.
>
> cheers
>
> andrew
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Re: [HACKERS] Improving test coverage of extensions with pg_dump

[Euler Taveira]

On 17-09-2015 14:21, Michael Paquier wrote:

pg_dump relies on attnum to define the column ordering, so one
possibility would be to do things more consistently at backend level.
Thoughts?

According to your example, problem is the columns from the parent table 
"aa" are added _before_ declaring the inherited table "bb". Then, an 
attnum from column "d" (part of parent table "aa") is assigned to a 
lower number than in the original table "bb".


Someone can say that we could assign an attnum for column "d" 
considering all of the inheritance tree. However, attnum is used as an 
index to arrays (we could bloat some of those) and some logic rely on it 
to count the number of columns. It would become tablecmds.c into an 
spaghetti.


IMHO a possible way to solve it is adding support for logical column 
ordering. An ALTER TABLE command (emitted if a parameter was informed) 
during dump could handle it. BTW, last thread [1] about logical column 
ordering seems to have died a few months ago. Alvaro?



[1] 
http://www.postgresql.org/message-id/20141209174146.gp1...@alvh.no-ip.org



--
   Euler Taveira   Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento


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

Re: [HACKERS] 9.3.9 and pg_multixact corruption

[Christoph Berg]

Re: Andreas Seltenreich 2015-09-13 <87si6i1875@credativ.de>
> I managed disassemble RecordNewMultiXact from the core dump using a
> cross-binutils, and it reveals that the compiler[1] appears to have
> indeed generated a signed division here.  I'm attaching a piece of C
> code that does the same computation as the assembly (I think), as well
> as the disassembly itself.
> 
> Footnotes: 
> [1]  Sun C 5.12 SunOS_sparc Patch 148917-07 2013/10/18, 64-bit

Hi,

a short update here: the customer updated the compiler to a newer
version, is now compiling using -O2 instead of -O3, and the code
generated now looks sane, so this turned out to be a compiler issue.
(Though it's unclear if the upgrade fixed it, or the different -O
level.)

Thanks to all who provided feedback, it was very valuable in actually
tracking down the root of the issue.

Christoph


-- 
Sent 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: Rework access method interface

[Tom Lane]

Petr Jelinek  writes:
> On 2015-09-18 14:58, Alexander Korotkov wrote:
>> After, further personal discussion with Teodor, we decided that
>> amvalidate is out of scope for this patch.
>> It's not evident what should we validate in amvalidate and which way. I
>> think if we need amvalidate it should be subject of separate patch.

> But why is it not evident? We do the validations in regression tests, 
> even if we just copy those then it's enough for a start.

I think the main reason this question is in-scope for this patch is
precisely the problem of what do we do about the regression tests.

I'm not in favor of exposing some SQL-level functions whose sole purpose
is to support those regression test queries, because while those queries
are very useful for detecting errors in handmade opclasses, they're hacks,
and always have been.  They don't work well (or at all, really) for
anything more than btree/hash cases.  It'd be better to expose amvalidate
functions, even if we don't yet have full infrastructure for them.

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: Rework access method interface

[Petr Jelinek]

On 2015-09-20 16:17, Alexander Korotkov wrote:

On Sun, Sep 20, 2015 at 5:02 PM, Petr Jelinek 

Re: [HACKERS] WIP: Rework access method interface

[Alexander Korotkov]

On Sun, Sep 20, 2015 at 5:18 PM, Tom Lane  wrote:

> Petr Jelinek  writes:
> > On 2015-09-18 14:58, Alexander Korotkov wrote:
> >> After, further personal discussion with Teodor, we decided that
> >> amvalidate is out of scope for this patch.
> >> It's not evident what should we validate in amvalidate and which way. I
> >> think if we need amvalidate it should be subject of separate patch.
>
> > But why is it not evident? We do the validations in regression tests,
> > even if we just copy those then it's enough for a start.
>
> I think the main reason this question is in-scope for this patch is
> precisely the problem of what do we do about the regression tests.
>
> I'm not in favor of exposing some SQL-level functions whose sole purpose
> is to support those regression test queries, because while those queries
> are very useful for detecting errors in handmade opclasses, they're hacks,
> and always have been.  They don't work well (or at all, really) for
> anything more than btree/hash cases.  It'd be better to expose amvalidate
> functions, even if we don't yet have full infrastructure for them.
>

I'm OK about continuing work on amvalidate if we can build consuensus on
its design.
Could you give some feedback on amvalidate version of patch please?
http://www.postgresql.org/message-id/capphfds8zywenz9vw6te5rzxbol1vu_wsw181veq+mu+v1d...@mail.gmail.com

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

Re: [HACKERS] creating extension including dependencies

[Petr Jelinek]

On 2015-09-18 04:52, Petr Jelinek wrote:

On 2015-09-17 17:31, Jeff Janes wrote:


If I fail to specify CASCADE and get an ERROR, I think there should be a
HINT which suggests the use of CASCADE.


create extension earthdistance ;
ERROR:  required extension "cube" is not installed

(no hint)

There is a HINT on the reverse operation:
drop extension cube;
ERROR:  cannot drop extension cube because other objects depend on it
DETAIL:  extension earthdistance depends on extension cube
HINT:  Use DROP ... CASCADE to drop the dependent objects too.


Makes sense.



Here it is.


--
 Petr Jelinek  http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
>From a6cc566f4d8600398d6dde1ab478b1565fe9871b Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Sat, 13 Jun 2015 19:49:10 +0200
Subject: [PATCH] CREATE EXTENSION CASCADE

---
 contrib/hstore_plperl/expected/hstore_plperl.out   |   6 +-
 contrib/hstore_plperl/expected/hstore_plperlu.out  |   6 +-
 contrib/hstore_plperl/sql/hstore_plperl.sql|   4 +-
 contrib/hstore_plperl/sql/hstore_plperlu.sql   |   4 +-
 .../hstore_plpython/expected/hstore_plpython.out   |   4 +-
 contrib/hstore_plpython/sql/hstore_plpython.sql|   3 +-
 contrib/ltree_plpython/expected/ltree_plpython.out |   4 +-
 contrib/ltree_plpython/sql/ltree_plpython.sql  |   3 +-
 doc/src/sgml/ref/create_extension.sgml |  47 -
 src/backend/commands/extension.c   | 210 ++---
 src/backend/parser/gram.y  |   4 +
 src/bin/psql/tab-complete.c|   7 +-
 src/test/modules/Makefile  |   1 +
 src/test/modules/test_extensions/.gitignore|   4 +
 src/test/modules/test_extensions/Makefile  |  21 +++
 .../test_extensions/expected/test_extensions.out   |  31 +++
 .../test_extensions/sql/test_extensions.sql|  18 ++
 .../modules/test_extensions/test_ext1--1.0.sql |   3 +
 src/test/modules/test_extensions/test_ext1.control |   5 +
 .../modules/test_extensions/test_ext2--1.0.sql |   3 +
 src/test/modules/test_extensions/test_ext2.control |   4 +
 .../modules/test_extensions/test_ext3--1.0.sql |   3 +
 src/test/modules/test_extensions/test_ext3.control |   3 +
 .../test_extensions/test_ext_cyclic1--1.0.sql  |   3 +
 .../test_extensions/test_ext_cyclic1.control   |   4 +
 .../test_extensions/test_ext_cyclic2--1.0.sql  |   3 +
 .../test_extensions/test_ext_cyclic2.control   |   4 +
 src/tools/msvc/Mkvcbuild.pm|   3 +-
 28 files changed, 328 insertions(+), 87 deletions(-)
 create mode 100644 src/test/modules/test_extensions/.gitignore
 create mode 100644 src/test/modules/test_extensions/Makefile
 create mode 100644 src/test/modules/test_extensions/expected/test_extensions.out
 create mode 100644 src/test/modules/test_extensions/sql/test_extensions.sql
 create mode 100644 src/test/modules/test_extensions/test_ext1--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext1.control
 create mode 100644 src/test/modules/test_extensions/test_ext2--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext2.control
 create mode 100644 src/test/modules/test_extensions/test_ext3--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext3.control
 create mode 100644 src/test/modules/test_extensions/test_ext_cyclic1--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_cyclic1.control
 create mode 100644 src/test/modules/test_extensions/test_ext_cyclic2--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_cyclic2.control

diff --git a/contrib/hstore_plperl/expected/hstore_plperl.out b/contrib/hstore_plperl/expected/hstore_plperl.out
index cf384eb..25fc506 100644
--- a/contrib/hstore_plperl/expected/hstore_plperl.out
+++ b/contrib/hstore_plperl/expected/hstore_plperl.out
@@ -1,6 +1,6 @@
-CREATE EXTENSION hstore;
-CREATE EXTENSION plperl;
-CREATE EXTENSION hstore_plperl;
+CREATE EXTENSION hstore_plperl CASCADE;
+NOTICE:  installing required extension "hstore"
+NOTICE:  installing required extension "plperl"
 SELECT transforms.udt_schema, transforms.udt_name,
routine_schema, routine_name,
group_name, transform_type
diff --git a/contrib/hstore_plperl/expected/hstore_plperlu.out b/contrib/hstore_plperl/expected/hstore_plperlu.out
index c97fd3f..b09fb78 100644
--- a/contrib/hstore_plperl/expected/hstore_plperlu.out
+++ b/contrib/hstore_plperl/expected/hstore_plperlu.out
@@ -1,6 +1,6 @@
-CREATE EXTENSION hstore;
-CREATE EXTENSION plperlu;
-CREATE EXTENSION hstore_plperlu;
+CREATE EXTENSION hstore_plperlu CASCADE;
+NOTICE:  installing required extension "hstore"
+NOTICE:  installing required extension "plperlu"
 SELECT transforms.udt_schema, transforms.udt_name,
routine_schema, routine_name,
group_name, transform_type
diff --git 

[HACKERS] Limit GIST_MAX_SPLIT_PAGES to XLR_MAX_BLOCK_ID

[Gurjeet Singh]

Gin code respects the XLR_MAX_BLOCK_ID when
calling XLogEnsureRecordSpace(). But it appears that the Gist code does not
try to limit its block-id consumption to XLR_MAX_BLOCK_ID.

The GIST_MAX_SPLIT_PAGES is hard-coded at 75, but XLogEnsureRecordSpace()
would reject a request of more than 32 (XLR_MAX_BLOCK_ID).

The attached patch redefines GIST_MAX_SPLIT_PAGES so that in case of a
split, gistplacetopage() now throws an error when the block-ids needed
exceed 32.

I have used Min(75, XLR_MAX_BLOCK_ID) as the macro expansion, but I believe
it can be set to plain XLR_MAX_BLOCK_ID.


-- 
Gurjeet Singh http://gurjeet.singh.im/


gist_limit_blockid_to_XLR_MAX_BLOCK_ID.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] row_security GUC, BYPASSRLS

[Tom Lane]

Robert Haas  writes:
> On Sun, Sep 20, 2015 at 5:35 PM, Stephen Frost  wrote:
>> * Joe Conway (m...@joeconway.com) wrote:
>>> That would be awesome, but I would say that if we do #1 & 2 for 9.5, we
>>> also need #3.

>> Agreed on all of the above.

> Well, then, we should get cracking.  beta1 is coming soon, and it
> would be best if this were done before then.

I'd say it's *necessary*.  We're not adding new features after beta1.

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] row_security GUC, BYPASSRLS

[Robert Haas]

On Sun, Sep 20, 2015 at 5:35 PM, Stephen Frost  wrote:
> * Joe Conway (m...@joeconway.com) wrote:
>> On 09/18/2015 01:07 AM, Noah Misch wrote:
>> > Great.  Robert, does that work for you, too?  If so, this sub-thread is
>> > looking at three patches:
>> >
>> > 1. remove row_security=force
>> > 2. remove SECURITY_ROW_LEVEL_DISABLED; make ri_triggers.c subject to 
>> > policies
>> > 3. add DDL-controlled, per-table policy forcing
>> >
>> > They ought to land in that order.  PostgreSQL 9.5 would need at least (1) 
>> > and
>> > (2); would RLS experts find it beneficial for me to take care of those?
>>
>> That would be awesome, but I would say that if we do #1 & 2 for 9.5, we
>> also need #3.
>
> Agreed on all of the above.

Well, then, we should get cracking.  beta1 is coming soon, and it
would be best if this were done before then.

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


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

Re: [HACKERS] [POC] FETCH limited by bytes.

[Corey Huinker]

On Fri, Sep 4, 2015 at 2:45 AM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:

> Hello,
>
> > > > @@ -915,6 +933,23 @@ postgresBeginForeignScan(ForeignScanState *node,
> > > int eflags)
> ...
> > > > + def = get_option(table->options, "fetch_size");
> >
> > > I don't think it's a good idea to make such checks at runtime - and
> > > either way it's somethign that should be reported back using an
> > > ereport(), not an elog.
> >
> > > Also, it seems somewhat wrong to determine this at execution
> > > time. Shouldn't this rather be done when creating the foreign scan
> node?
> > > And be a part of the scan state?
> >
> > I agree, that was my original plan, but I wasn't familiar enough with the
> > FDW architecture to know where the table struct and the scan struct were
> > both exposed in the same function.
> >
> > What I submitted incorporated some of Kyotaro's feedback (and working
> > patch) to my original incomplete patch.
>
> Sorry, it certainly shouldn't be a good place to do such thing. I
> easily selected the place in order to avoid adding new similar
> member in multiple existing structs (PgFdwRelationInfo and
> PgFdwScanState).
>
> Having a new member fetch_size is added in PgFdwRelationInfo and
> PgFdwScanState, I think postgresGetForeignRelSize is the best
> place to do that, from the point that it collects basic
> information needed to calculate scan costs.
>
> # Fetch sizes of foreign join would be the future issue..
>
> > typedef struct PgFdwRelationInfo
> > {
>   ...
> + intfetch_size;   /* fetch size for this remote table */
>
> 
> > postgreGetForeignRelSize()
> > {
>   ...
> > fpinfo->table = GetForeignTable(foreigntableid);
> > fpinfo->server = GetForeignServer(fpinfo->table->serverid);
> >
> + def = get_option(table->options, "fetch_size");
> + ..
> + fpinfo->fetch_size = strtod(defGetString...
>
> Also it is doable in postgresGetForeignPlan and doing there
> removes redundant copy of fetch_size from PgFdwRelation to
> PgFdwScanState but theoretical basis would be weak.
>
> regards,
>
> > > On 2015-02-27 13:50:22 -0500, Corey Huinker wrote:
> > > > +static DefElem*
> > > > +get_option(List *options, char *optname)
> > > > +{
> > > > + ListCell *lc;
> > > > +
> > > > + foreach(lc, options)
> > > > + {
> > > > + DefElem *def = (DefElem *) lfirst(lc);
> > > > +
> > > > + if (strcmp(def->defname, optname) == 0)
> > > > + return def;
> > > > + }
> > > > + return NULL;
> > > > +}
> > >
> > >
> > > >   /*
> > > >* Do nothing in EXPLAIN (no ANALYZE) case.  node->fdw_state
> stays
> > > NULL.
> > > > @@ -915,6 +933,23 @@ postgresBeginForeignScan(ForeignScanState *node,
> > > int eflags)
> > > >   server = GetForeignServer(table->serverid);
> > > >   user = GetUserMapping(userid, server->serverid);
> > > >
> > > > + /* Reading table options */
> > > > + fsstate->fetch_size = -1;
> > > > +
> > > > + def = get_option(table->options, "fetch_size");
> > > > + if (!def)
> > > > + def = get_option(server->options, "fetch_size");
> > > > +
> > > > + if (def)
> > > > + {
> > > > + fsstate->fetch_size = strtod(defGetString(def), NULL);
> > > > + if (fsstate->fetch_size < 0)
> > > > + elog(ERROR, "invalid fetch size for foreign
> table
> > > \"%s\"",
> > > > +  get_rel_name(table->relid));
> > > > + }
> > > > + else
> > > > + fsstate->fetch_size = 100;
> > >
> > > I don't think it's a good idea to make such checks at runtime - and
> > > either way it's somethign that should be reported back using an
> > > ereport(), not an elog.
> >
> > > Also, it seems somewhat wrong to determine this at execution
> > > time. Shouldn't this rather be done when creating the foreign scan
> node?
> > > And be a part of the scan state?
> >
> > I agree, that was my original plan, but I wasn't familiar enough with the
> > FDW architecture to know where the table struct and the scan struct were
> > both exposed in the same function.
> >
> > What I submitted incorporated some of Kyotaro's feedback (and working
> > patch) to my original incomplete patch.
> >
> > > Have you thought about how this option should cooperate with join
> > > pushdown once implemented?
> > >
> >
> > I hadn't until now, but I think the only sensible thing would be to
> > disregard table-specific settings once a second foreign table is
> detected,
> > and instead consider only the server-level setting.
> >
> > I suppose one could argue that if ALL the tables in the join had the same
> > table-level setting, we should go with that, but I think that would be
> > complicated, expensive, and generally a good argument for changing the
> > server setting instead.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
>
Ok, with some guidance from RhodiumToad (thanks!) I was able to get 

Re: [HACKERS] row_security GUC, BYPASSRLS

[Noah Misch]

On Mon, Sep 14, 2015 at 03:29:16AM -0400, Noah Misch wrote:
> On a related topic, check_enable_rls() has two kinds of RLS bypass semantics.
> Under row_security=off|on, superusers bypass every policy, and table owners
> bypass policies of their own tables.  Under row_security=off only, roles with
> BYPASSRLS privilege additionally bypass every policy; BYPASSRLS doesn't affect
> semantics under row_security=on.  Is that distinction a good thing?  Making
> BYPASSRLS GUC-independent, like superuser bypass, would simplify things for
> the user and would make row_security no longer a behavior-changing GUC.
> row_security would come to mean simply "if a policy would otherwise attach to
> one of my queries, raise an error instead."  Again, if you have cause to
> toggle BYPASSRLS, use multiple roles.  Implementing that with GUCs creates
> harder problems.

Having pondered this further in the course of finalizing commit 537bd17,
arming BYPASSRLS independent of the row_security GUC is indeed a good thing.
Right now, if a BYPASSRLS user creates a SECURITY DEFINER function, any caller
can change that function's behavior by toggling the GUC.  Users won't test
accordingly; better to have just one success-case behavior.

On Wed, Jul 29, 2015 at 09:09:27AM -0400, Stephen Frost wrote:
> For superuser (the only similar precedent that we have, I believe), we
> go based on the view owner, but that isn't quite the same as BYPASSRLS.
> 
> The reason this doesn't hold is that you have to use a combination of
> BYPASSRLS and row_security=off to actually bypass RLS, unlike the
> superuser role attribute which is just always "on" if you've got it.  If
> having BYPASSRLS simply always meant "don't do any RLS" then we could
> use the superuser precedent to use what the view owner has, but at least
> for my part, I'm a lot happier with BYPASSRLS and row_security than with
> superuser and would rather we continue in that direction, where the user
> has the choice of if they want their role attribute to be in effect or
> not.

If I make BYPASSRLS GUC-independent, I should then also make it take effect
when the BYPASSRLS role owns a view.  Barring objections, I will change both.

I do share your wish for an ability to suppress privileges temporarily.  I
have no specific design in mind, but privilege activation and suppression
should be subject to the approval of roles affected.  GUCs probably can't
serve here; apart from the grandfathered search_path, functions can ignore
them.  GUCs are mostly a property of the whole session.


By the way, is there a reason for RI_Initial_Check() to hard-code the rules
for RLS enablement instead of calling check_enable_rls(..., InvalidOid, true)
twice?  I refer to this code:

>   /*
>* Also punt if RLS is enabled on either table unless this role has the
>* bypassrls right or is the table owner of the table(s) involved which
>* have RLS enabled.
>*/
>   if (!has_bypassrls_privilege(GetUserId()) &&
>   ((pk_rel->rd_rel->relrowsecurity &&
> !pg_class_ownercheck(pkrte->relid, GetUserId())) ||
>(fk_rel->rd_rel->relrowsecurity &&
> !pg_class_ownercheck(fkrte->relid, GetUserId()
>   return false;


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

Re: [HACKERS] row_security GUC, BYPASSRLS

[Noah Misch]

On Fri, Sep 18, 2015 at 09:27:13AM -0500, Joe Conway wrote:
> On 09/18/2015 09:25 AM, Adam Brightwell wrote:
> >>> 1. remove row_security=force
> >>> 2. remove SECURITY_ROW_LEVEL_DISABLED; make ri_triggers.c subject to 
> >>> policies
> >>> 3. add DDL-controlled, per-table policy forcing
> >>>
> >>> They ought to land in that order.  PostgreSQL 9.5 would need at least (1) 
> >>> and
> >>> (2); would RLS experts find it beneficial for me to take care of those?

Done.

> >> That would be awesome, but I would say that if we do #1 & 2 for 9.5, we
> >> also need #3.

Understood.

> > Agreed.  Please let me know if there is anything I can do to help.
> 
> Yes, same here.

Thanks.  https://wiki.postgresql.org/wiki/PostgreSQL_9.5_Open_Items is the big
board of things needing work.  These items are long-idle:

- 84 days: UPSERT on partition
- 81 days: Refactoring speculative insertion with unique indexes a little
- 49 days: Arguable RLS security bug, EvalPlanQual() paranoia

If you grab one or more of those and figure out what it/they need to get
moving again, that would help move us toward 9.5 final.  (Don't feel the need
to limit yourself to those three, but they are the low-hanging fruit.)


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

Re: [HACKERS] row_security GUC, BYPASSRLS

[Stephen Frost]

* Joe Conway (m...@joeconway.com) wrote:
> On 09/18/2015 01:07 AM, Noah Misch wrote:
> > Great.  Robert, does that work for you, too?  If so, this sub-thread is
> > looking at three patches:
> > 
> > 1. remove row_security=force
> > 2. remove SECURITY_ROW_LEVEL_DISABLED; make ri_triggers.c subject to 
> > policies
> > 3. add DDL-controlled, per-table policy forcing
> > 
> > They ought to land in that order.  PostgreSQL 9.5 would need at least (1) 
> > and
> > (2); would RLS experts find it beneficial for me to take care of those?
> 
> That would be awesome, but I would say that if we do #1 & 2 for 9.5, we
> also need #3.

Agreed on all of the above.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] CREATE POLICY and RETURNING

[Stephen Frost]

Zhaomo,

* Zhaomo Yang (zmp...@gmail.com) wrote:
> > I agree that if we force a single visibility policy for all commands
> > then we wouldn't need the USING clauses for UPDATE and DELETE, but we
> > would certainly need *some* policy for DELETE to prevent users from
> > being able to delete records that they aren't supposed to be allowed to.
> > Therefore, we'd just be replacing the USING policy with a 'WITH CHECK'
> > policy, no?
> 
> If we force a single visibility policy (SELECT policy), then  we will
> need a command-specific policy for each of UPDATE/DELETE/INSERT. A
> command-specific policy may be  a writing policy (as for INSERT), a
> reading policy (as for DELETE), or a hybrid policy (as for UPDATE).
> 
> For DELETE we can either combine the visibility policy (SELECT policy)
> with the DELETE policy using AND and then scan the table, or just
> attach the DELETE policy to the WHERE clause after the visibility
> policy has been enforced. I don't see why we need to replace USING
> policy with a "WITH CHECK".

We are certainly interested in supporting restrictive policies, which is
what you're asking for here.  I don't have any problem with that and
have written a fair bit of code to help it happen.  It'd be great if
others who are interested can help define the grammar changes necessary
and perhaps even help with the code aspect of it.

> BTW, what is the fundamental difference between a USING predicate and
> a WITH CHECK predicate? Is it that which phase they are applied (read
> or write)? Or is it that how they handle violations (nothing-happens
> or error-out)?

Yes, USING is a filter, WITH CHECK throws an error.

> > Removing the existing ability to control the visibility on a
> > per-command basis is pretty clearly a reduction in the overall
> > flexibility of the system without a clear gain to me.
> 
> I think there is a clear gain: security.

I don't buy that argument.

> One interesting issue related to this discussion is that how
> violations are handled. Now reading violations fail silently
> (nothing-happens result) while writing violations cause errors
> (throw-error result).

The proposal which I made, to which you are responding, was specifically
to provide users with the ability to specify which kind of handling they
want.  I'm firmly of the opinion that there are perfectly valid and
secure use-cases of both permissive and restrictive policies, for all
commands.

> In the paper named "Extending Query Rewriting Techniques for
> Fine-Grained Access Control" [1], Rizvi et al. added row level access
> control to DBMSes using an interesting syntax: GRANT-WHERE. They added
> a WHERE predicate to the SQL GRANT statement
> to achieve row-level access control. Besides the interesting syntax,
> they brought up the two possible models of handling violations in the
> paper. One model is "nothing-happens" model (they call it Truman's
> world model) and another is "error out" model (they call it Non-Truman
> model). The authors discussed the pros and cons of both models: the
> "nothing-happens" model is more secure since it leaks less information
> but a user may get surprised  by the results; the "error-out" model
> leaks information but may be more convenient when a user is debugging
> his queries. I curious about our community's  take on this issue.

That grammar was considered and I recall that paper being specifically
discussed, but it wasn't what we decided to go with and I don't see that
changing at this point.  In the end, I believe we will provide support
for both models (ideally as early as 9.6, if we can begin making
progress towards that goal now).

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] RemoveLocalLock pfree'ing NULL when out-of-memory

[Tom Lane]

Andreas Seltenreich  writes:
> a memory-starved instance of sqlsmith just caught RemoveLocalLock
> pfree'ing a NULL in locallock->lockOwners.  I think what happened is
> that it was called to clean up after LockAcquireExtended's
> MemoryContextAlloc failed.  The content of errordata seems consistent
> with this.

Ooops.  Looks to have been my bug originally.  Will fix, thanks!

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] RemoveLocalLock pfree'ing NULL when out-of-memory

[Andreas Seltenreich]

Hi,

a memory-starved instance of sqlsmith just caught RemoveLocalLock
pfree'ing a NULL in locallock->lockOwners.  I think what happened is
that it was called to clean up after LockAcquireExtended's
MemoryContextAlloc failed.  The content of errordata seems consistent
with this.

Caught in master as of 85eda7e (sorry about the bogus hashes in earlier
reports, I had some cruft on my local branch).

regards,
Andreas

FailedAssertion("!(pointer != ((void *)0))", File: "mcxt.c", Line: 1002)

#3  0x007e1c80 in pfree (pointer=) at mcxt.c:1002
#4  0x006bdd24 in RemoveLocalLock (locallock=locallock@entry=0x3a90d68) 
at lock.c:1225
#5  0x006c1ceb in LockReleaseAll (lockmethodid=lockmethodid@entry=1, 
allLocks=1 '\001') at lock.c:2083
#6  0x006c3274 in ProcReleaseLocks (isCommit=isCommit@entry=0 '\000') 
at proc.c:752
#7  0x007e3700 in ResourceOwnerReleaseInternal 
(owner=owner@entry=0x208b488, phase=phase@entry=RESOURCE_RELEASE_LOCKS, 
isCommit=isCommit@entry=0 '\000', isTopLevel=isTopLevel@entry=1 '\001') at 
resowner.c:307
#8  0x007e381f in ResourceOwnerRelease (owner=0x208b488, 
phase=phase@entry=RESOURCE_RELEASE_LOCKS, 
isCommit=isCommit@entry=0 '\000', isTopLevel=isTopLevel@entry=1 '\001') at 
resowner.c:212
#9  0x004e903b in AbortTransaction () at xact.c:2557
#10 0x004e98ad in AbortCurrentTransaction () at xact.c:3003
#11 0x006d45a3 in PostgresMain (argc=1, argv=0x202e638, 
dbname=0x202e610 "regression", username=0x202e5f0 "smith")
at postgres.c:3856
#12 0x00466964 in BackendRun (port=0x204e080) at postmaster.c:4204
#13 BackendStartup (port=0x204e080) at postmaster.c:3880
#14 ServerLoop () at postmaster.c:1683
#15 0x0067867e in PostmasterMain (argc=argc@entry=3, 
argv=argv@entry=0x202d600) at postmaster.c:1292
#16 0x0046774d in main (argc=3, argv=0x202d600) at main.c:223

(gdb) p errordata[0]
$4 = {elevel = 20, output_to_server = 0 '\000', output_to_client = 1 '\001', 
show_funcname = 0 '\000', hide_stmt = 0 '\000', 
  hide_ctx = 0 '\000', filename = 0x9a1f80 "mcxt.c", lineno = 769, funcname = 
0x9a24b0 <__func__.5880> "MemoryContextAlloc", 
  domain = 0x9350f6 "postgres-9.6", context_domain = 0x9350f6 "postgres-9.6", 
sqlerrcode = 8389, 
  message = 0x296d020 "out of memory", detail = 0x296cfe8 "Failed on request of 
size 128.", detail_log = 0x0, hint = 0x0, 
  context = 0x0, schema_name = 0x0, table_name = 0x0, column_name = 0x0, 
datatype_name = 0x0, constraint_name = 0x0, cursorpos = 0, 
  internalpos = 0, internalquery = 0x0, saved_errno = 12, assoc_context = 
0x296a7a8}


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