Typos from Covering Index patch

[Michael Paquier]

Hi all,

I am catching up with new features so I have begun going through
Covering indexes.  While reading the code, I have noticed a couple of
things:
1) Some typos.
2) An inconsistent variable name in pg_dump.

The thing and structure of the code is pretty interesting by the way.
That's nice work.  I am still trying to break the DDL data like
NULL-ness of columns for some weird combinations of columns without
being able to.

Thanks,
--
Michael
diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c
index 3ef861494b..10509cfe8a 100644
--- a/src/backend/access/nbtree/nbtinsert.c
+++ b/src/backend/access/nbtree/nbtinsert.c
@@ -1733,8 +1733,8 @@ _bt_checksplitloc(FindSplitData *state,
 	/*
 	 * The first item on the right page becomes the high key of the left page;
 	 * therefore it counts against left space as well as right space. When
-	 * index has included attribues, then those attributes of left page high
-	 * key will be truncate leaving that page with slightly more free space.
+	 * index has included attributes, then those attributes of left page high
+	 * key will be truncated leaving that page with slightly more free space.
 	 * However, that shouldn't affect our ability to find valid split
 	 * location, because anyway split location should exists even without high
 	 * key truncation.
diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c
index 12b636253e..eedb7a91e9 100644
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -2087,7 +2087,8 @@ btproperty(Oid index_oid, int attno,
  *
  *	Transforms an ordinal B-tree leaf index tuple into pivot tuple to be used
  *	as hikey or non-leaf page tuple with downlink.  Note that t_tid offset
- *	will be overritten in order to represent number of present tuple attributes.
+ *	will be overwritten in order to represent number of present tuple
+ * attributes.
  */
 IndexTuple
 _bt_truncate_tuple(Relation idxrel, IndexTuple olditup)
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 93c869fd68..b11fe94212 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -6726,7 +6726,7 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
 i_indexname,
 i_parentidx,
 i_indexdef,
-i_indnnkeyatts,
+i_indnkeyatts,
 i_indnatts,
 i_indkey,
 i_indisclustered,
@@ -6952,7 +6952,7 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
 		i_indexname = PQfnumber(res, "indexname");
 		i_parentidx = PQfnumber(res, "parentidx");
 		i_indexdef = PQfnumber(res, "indexdef");
-		i_indnnkeyatts = PQfnumber(res, "indnkeyatts");
+		i_indnkeyatts = PQfnumber(res, "indnkeyatts");
 		i_indnatts = PQfnumber(res, "indnatts");
 		i_indkey = PQfnumber(res, "indkey");
 		i_indisclustered = PQfnumber(res, "indisclustered");
@@ -6986,7 +6986,7 @@ getIndexes(Archive *fout, TableInfo tblinfo[], int numTables)
 			indxinfo[j].dobj.namespace = tbinfo->dobj.namespace;
 			indxinfo[j].indextable = tbinfo;
 			indxinfo[j].indexdef = pg_strdup(PQgetvalue(res, j, i_indexdef));
-			indxinfo[j].indnkeyattrs = atoi(PQgetvalue(res, j, i_indnnkeyatts));
+			indxinfo[j].indnkeyattrs = atoi(PQgetvalue(res, j, i_indnkeyatts));
 			indxinfo[j].indnattrs = atoi(PQgetvalue(res, j, i_indnatts));
 			indxinfo[j].tablespace = pg_strdup(PQgetvalue(res, j, i_tablespace));
 			indxinfo[j].indreloptions = pg_strdup(PQgetvalue(res, j, i_indreloptions));


signature.asc
Description: PGP signature

Fix for documentation of Covering Indexes

[Michael Paquier]

Hi all,

The documentation of covering indexes is incorrect for CREATE and ALTER
TABLE:
- ALTER TABLE's page is missing the call.
- Exclusion constraints can use INCLUDE clauses.

In order to simplify the documentation, please let me suggest the
attached which moves the definition of the INCLUDE clause into the
section index_parameters, which is compatible with what I read from the
parser.

Thanks,
--
Michael
diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index bd2262761e..8c7b9619b8 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -125,6 +125,7 @@ WITH ( MODULUS numeric_literal, REM
 
 index_parameters in UNIQUE, PRIMARY KEY, and EXCLUDE constraints are:
 
+[ INCLUDE ( column_name [, ... ] ) ]
 [ WITH ( storage_parameter [= value] [, ... ] ) ]
 [ USING INDEX TABLESPACE tablespace_name ]
 
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index cb3867dbd5..f2bd562d55 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -73,8 +73,8 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
 
 [ CONSTRAINT constraint_name ]
 { CHECK ( expression ) [ NO INHERIT ] |
-  UNIQUE ( column_name [, ... ] ) index_parameters  INCLUDE (column_name [, ...])  |
-  PRIMARY KEY ( column_name [, ... ] ) index_parameters  INCLUDE (column_name [, ...])  |
+  UNIQUE ( column_name [, ... ] ) index_parameters |
+  PRIMARY KEY ( column_name [, ... ] ) index_parameters |
   EXCLUDE [ USING index_method ] ( exclude_element WITH operator [, ... ] ) index_parameters [ WHERE ( predicate ) ] |
   FOREIGN KEY ( column_name [, ... ] ) REFERENCES reftable [ ( refcolumn [, ... ] ) ]
 [ MATCH FULL | MATCH PARTIAL | MATCH SIMPLE ] [ ON DELETE action ] [ ON UPDATE action ] }
@@ -93,6 +93,7 @@ WITH ( MODULUS numeric_literal, REM
 
 index_parameters in UNIQUE, PRIMARY KEY, and EXCLUDE constraints are:
 
+[ INCLUDE ( column_name [, ... ] ) ]
 [ WITH ( storage_parameter [= value] [, ... ] ) ]
 [ USING INDEX TABLESPACE tablespace_name ]
 


signature.asc
Description: PGP signature

Re: [HACKERS] PATCH: Keep one postmaster monitoring pipe per process

[Thomas Munro]

On Wed, Apr 11, 2018 at 12:47 PM, Thomas Munro
 wrote:
> On Wed, Apr 11, 2018 at 12:26 PM, Andres Freund  wrote:
>> On 2018-04-11 12:17:14 +1200, Thomas Munro wrote:
>>> I arrived at this idea via the realisation that the closest thing to
>>> prctl(PR_SET_PDEATHSIG) on BSD-family systems today is
>>> please-tell-my-kqueue-if-this-process-dies.  It so happens that my
>>> kqueue patch already uses that instead of the pipe to detect
>>> postmaster death.  The only problem is that you have to ask it, by
>>> calling it kevent().  In a busy loop like those two, where there is no
>>> other kind of waiting going on, you could do that by calling kevent()
>>> with timeout = 0 to check the queue.
>>
>> Which is not cheap.

After bouncing that idea around with a fellow pgsql-hacker off-list, I
take that idea back.  It's a lot simpler and as cheap if not cheaper
to check getppid() == 1 or similar every Nth time through the busy
loop.

I heard another interesting idea -- you can use F_SETOWN + O_ASYNC to
ask for SIGIO to be delivered to you when the pipe is closed.
Unfortunately that'd require a different pipe for each backend so
wouldn't work for us.

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

Re: Boolean partitions syntax

[Kyotaro HORIGUCHI]

Thank you for the comments.

At Wed, 11 Apr 2018 13:51:55 +0900, Amit Langote 
 wrote in 
<3d0fda29-986c-d970-a22c-b4bd44f56...@lab.ntt.co.jp>
> Horiguchi-san,
> 
> Thanks for working on this.
> 
> On 2018/04/11 13:20, Kyotaro HORIGUCHI wrote:
> > At Wed, 11 Apr 2018 11:27:17 +0900, Amit Langote wrote:
> >> On 2018/04/11 10:44, Tom Lane wrote:
> >>> Kyotaro HORIGUCHI  writes:
>  At least partition bound *must* be a constant. Any expression
>  that can be reduced to a constant at parse time ought to be
>  accepted but must not be accepted if not.
> >>>
> >>> My point is that *any* expression can be reduced to a constant,
> >>> we just have to do so.
> > 
> > Agreed in that sense. What was in my mind was something like
> > column reference, random() falls into reducible category of
> > course.
> > 
> > # I regard the change in gram.y is regarded as acceptable as a
> > # direction, so I'll continue to working on this.
> 
> I haven't yet reviewed the grammar changes in detail yet...

> >> I think what Tom is proposing here, instead of bailing out upon
> >> eval_const_expressions() failing to simplify the input expression to a
> >> Const, is to *invoke the executor* to evaluate the expression, like the
> >> optimizer does in evaluate_expr, and cook up a Const with whatever comes
> >> out to store it into the catalog (that is, in relpartbound).
> > 
> > Yes. In the attached I used evaluate_expr by making it non-static
> > function. a_expr used instead of partbound_datum is changed to
> > u_expr, which is the same with range bounds.
> > 
> >> =# create table c1 partition of p for values in (random() * 100);
> >> CREATE TABLE
> >> =# \d c1
> > ...
> >> Partition of: p FOR VALUES IN (97)
> 
> I looked at the non-gram.y portions of the patch for now as I was also
> playing with this.  Some comments on your patch:
> 
> * You missed adding a break here for the EXPR_KIND_PARTITION_EXPRESSION case
> 
>  case EXPR_KIND_PARTITION_EXPRESSION:
>  err = _("window functions are not allowed in partition key
> expressions");
> +case EXPR_KIND_PARTITION_BOUNDS:
> +err = _("window functions are not allowed in partition bounds");
>  break;
> 
> So, the following is the wrong error message that you probably failed to
> notice:

Oops! Fixed along with another one. I haven't looked the
difference so seriously.

> --- a/src/test/regress/expected/create_table.out
> +++ b/src/test/regress/expected/create_table.out
> @@ -308,7 +308,7 @@ CREATE TABLE partitioned (
>  a int,
>  b int
>  ) PARTITION BY RANGE ((avg(a) OVER (PARTITION BY b)));
> -ERROR:  window functions are not allowed in partition key expressions
> +ERROR:  window functions are not allowed in partition bounds

I felt a bit uneasy to saw that in the very corner of my mind..

> * I think the new ParseExprKind you added should omit the last "S", that
> is, name it EXPR_KIND_PARTITION_BOUND, because these are expressions to
> represent individual bound values.  And so adjust the comment to say "bound".
> 
>  +EXPR_KIND_PARTITION_BOUNDS, /* partition bounds value */

Agreed.

> * When looking at the changes to transformPartitionBoundValue, I noticed
> that there is no new argument Oid colTypColl
> 
>  static Const *
> -transformPartitionBoundValue(ParseState *pstate, A_Const *con,
> +transformPartitionBoundValue(ParseState *pstate, Node *val,
>   const char *colName, Oid colType, int32
> colTypmod)
> 
> that's because you seem to pass the expression's type, typmod, and typcoll
> to the newly added call to evaluate_expr.  I wonder if we should really
> pass the partition key specified values here.  We already receive first
> two from the caller.

I overlooked that the value (Expr) is already coerced at the
time. Added collation handling and this would be back-patchable.

I'm not sure how we should handle collate in this case but the
patch rejects bound values with non-default collation if it is
different from partition key.

Collation check works

=# create table p (a text collate "de_DE") partition by (a)
=# create table c1 partition of p for values in (('a' collate "ja_JP"));
ERROR:  collation mismatch between partition key expression (12622) and 
partition bound value (12684)
LINE 1: create table c1 partition of p for values in (('a' collate "...

> * In the changes to create_table.out
> 
> @@ -450,13 +450,9 @@ CREATE TABLE part_1 PARTITION OF list_parted FOR
> VALUES IN ('1');
>  CREATE TABLE part_2 PARTITION OF list_parted FOR VALUES IN (2);
>  CREATE TABLE part_null PARTITION OF list_parted FOR VALUES IN (null);
>  CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN (int '1');
> -ERROR:  syntax error at or near "int"
> -LINE 1: ... fail_part PARTITION OF list_parted FOR VALUES IN (int '1');
> -  ^
> +ERROR:  partition "fail_part" would overlap partition "part_1"
>  CREATE TABLE fail_part PARTITION O

Re: submake-errcodes

[Christoph Berg]

Re: Tom Lane 2018-04-10 <24426.1523387...@sss.pgh.pa.us>
> The short-term solution seems to be to put that back, but that's sort
> of annoying because it means this isn't a bulletproof solution.  It
> will only work for builds started in one of the directories that we
> take the trouble to put this defense into, and I can't see doing that
> everywhere.  Still, such things didn't work reliably before either
> except in these few directories, so maybe it won't matter.

Is that supposed to work now, without explicitly invoking sub-generated-headers?
It still doesn't work on current HEAD:

/home/cbe/projects/postgresql/pg/master/build-py3/../src/include/utils/elog.h:71:10:
 fatal error: utils/errcodes.h: Datei oder Verzeichnis nicht gefunden
 #include "utils/errcodes.h"
  ^~
compilation terminated.
make[2]: *** [: plpy_cursorobject.o] Fehler 1
make[2]: Verzeichnis 
„/home/cbe/projects/postgresql/pg/master/build-py3/src/pl/plpython“ wird 
verlassen

The other directories we'd need it are contrib/{hstore,jsonb,ltree}_plpython.

Christoph

Re: [HACKERS] path toward faster partition pruning

[David Rowley]

On 11 April 2018 at 18:04, Amit Langote  wrote:
> Updated patch attached.

Thanks for the updated patch.

The only thing I'm not sure about is the chances you've made to the
COALESCE function.

+CREATE OR REPLACE FUNCTION pp_hashint4_noop(int4, int8) RETURNS int8 AS
+$$SELECT coalesce($1, $2)::int8$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR CLASS pp_test_int4_ops FOR TYPE int4 USING HASH AS
+OPERATOR 1 = , FUNCTION 2 pp_hashint4_noop(int4, int8);
+CREATE OR REPLACE FUNCTION pp_hashtext_length(text, int8) RETURNS int8 AS
+$$SELECT length(coalesce($1, ''))::int8$$ LANGUAGE sql IMMUTABLE;

Why does one default to the seed and the other to an empty string?
Shouldn't they both do the same thing? If you were to copy the
hash_part.sql you'd just coalesce($1, 0) and coalesce($1, ''), any
special reason not to do that?

Also just wondering if it's worth adding some verification that we've
actually eliminated the correct partitions by backing the tests up
with a call to satisfies_hash_partition.

I've attached a delta patch that applies to your v2 which does this.
Do you think it's worth doing?

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


add_satisfies_hash_partition_verification.patch
Description: Binary data

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Mark Rofail]

On Tue, 10 Apr 2018 at 4:17 pm, Alvaro Herrera 
wrote:

> Mark Rofail wrote:
> I meant for the GIN operator. (Remember, these are two patches, and each
> of them needs its own tests.)

Yes, you are right. I have been dealing with the code as a single patch
that I almost forgot.

True.  So my impression from the numbers you posted last time is that
> you need to run each measurement case several times, and provide
> averages/ stddevs/etc for the resulting numbers, and see about outliers
> (maybe throw them away, or maybe they indicate some problem in the test
> or in the code); then we can make an informed decision about whether the
> variations between the several different scenarios are real improvements
> (or pessimizations) or just measurement noise.

I'd rather just throw away the previous results and start over with new
performance tests. However, like I showed you, it was my first time to
write performance tests. If there's something I could use as a reference
that would help me so much.

>
> In particular: it seemed to me that you decided to throw away the idea
> of the new GIN operator without sufficient evidence that it was
> unnecessary.

I have to admit to that. But in my defence @> is also GIN indexable so the
only difference in performance between 'anyarray @>> anyelement' and
'anyarray @> ARRAY [anyelement]' is the delay caused by the ARRAY[]
operation theoretically.

I apologise, however, I needed more evidence to support my claims.

Regards

On Tue, Apr 10, 2018 at 4:17 PM, Alvaro Herrera 
wrote:

> Mark Rofail wrote:
> > On Tue, Apr 10, 2018 at 3:59 PM, Alvaro Herrera  >
> > wrote:
> > >
> > > documentation to it and a few extensive tests to ensure it works well);
> >
> > I think the existing regression tests verify that the patch works as
> > expectations, correct?
>
> I meant for the GIN operator. (Remember, these are two patches, and each
> of them needs its own tests.)
>
> > We need more *exhaustive* tests to test performance, not functionality.
>
> True.  So my impression from the numbers you posted last time is that
> you need to run each measurement case several times, and provide
> averages/ stddevs/etc for the resulting numbers, and see about outliers
> (maybe throw them away, or maybe they indicate some problem in the test
> or in the code); then we can make an informed decision about whether the
> variations between the several different scenarios are real improvements
> (or pessimizations) or just measurement noise.
>
> In particular: it seemed to me that you decided to throw away the idea
> of the new GIN operator without sufficient evidence that it was
> unnecessary.
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] path toward faster partition pruning

[Amit Langote]

Hi David.

Thanks for the review.

On 2018/04/11 17:59, David Rowley wrote:
> On 11 April 2018 at 18:04, Amit Langote  wrote:
>> Updated patch attached.
> 
> Thanks for the updated patch.
> 
> The only thing I'm not sure about is the chances you've made to the
> COALESCE function.
> 
> +CREATE OR REPLACE FUNCTION pp_hashint4_noop(int4, int8) RETURNS int8 AS
> +$$SELECT coalesce($1, $2)::int8$$ LANGUAGE sql IMMUTABLE;
> +CREATE OPERATOR CLASS pp_test_int4_ops FOR TYPE int4 USING HASH AS
> +OPERATOR 1 = , FUNCTION 2 pp_hashint4_noop(int4, int8);
> +CREATE OR REPLACE FUNCTION pp_hashtext_length(text, int8) RETURNS int8 AS
> +$$SELECT length(coalesce($1, ''))::int8$$ LANGUAGE sql IMMUTABLE;
> 
> Why does one default to the seed and the other to an empty string?
> Shouldn't they both do the same thing? If you were to copy the
> hash_part.sql you'd just coalesce($1, 0) and coalesce($1, ''), any
> special reason not to do that?

Oops, so I hadn't actually restored it to the way it is in hash_part.sql.

Also, Ashutosh was talking about the custom hashing function used in
insert.sql, not hash_part.sql, which I based my revision upon.

Fixed it now.

> Also just wondering if it's worth adding some verification that we've
> actually eliminated the correct partitions by backing the tests up
> with a call to satisfies_hash_partition.
> 
> I've attached a delta patch that applies to your v2 which does this.
> Do you think it's worth doing?

We can see check by inspection that individual values are in appropriate
partitions, which is the point of having the inserts and the select just
above the actual pruning related tests.  So, I'm not sure if adding the
satisfies_hash_partition against each pruning tests adds much.

Attached revised patch.

Thanks,
Amit
From 4685448a7eb2eaf5feceea2206d136c135b2dea7 Mon Sep 17 00:00:00 2001
From: amit 
Date: Tue, 10 Apr 2018 16:06:33 +0900
Subject: [PATCH v3] Rewrite hash partition pruning tests to use custom opclass

Relying on platform-provided hashing functions makes tests unreliable
as shown by buildfarm recently.

This adds adjusted tests to partition_prune.sql itself and hence
partition_prune_hash.sql is deleted along with two expected output
files.

Discussion: 
https://postgr.es/m/CA%2BTgmoZ0D5kJbt8eKXtvVdvTcGGWn6ehWCRSZbWytD-uzH92mQ%40mail.gmail.com
---
 src/test/regress/expected/partition_prune.out  | 237 +
 src/test/regress/expected/partition_prune_hash.out | 189 
 .../regress/expected/partition_prune_hash_1.out| 187 
 src/test/regress/parallel_schedule |   2 +-
 src/test/regress/serial_schedule   |   1 -
 src/test/regress/sql/partition_prune.sql   |  70 +-
 src/test/regress/sql/partition_prune_hash.sql  |  41 
 7 files changed, 307 insertions(+), 420 deletions(-)
 delete mode 100644 src/test/regress/expected/partition_prune_hash.out
 delete mode 100644 src/test/regress/expected/partition_prune_hash_1.out
 delete mode 100644 src/test/regress/sql/partition_prune_hash.sql

diff --git a/src/test/regress/expected/partition_prune.out 
b/src/test/regress/expected/partition_prune.out
index df3fca025e..d13389b9c2 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -1332,6 +1332,243 @@ explain (costs off) select * from rparted_by_int2 where 
a > 100;
 
 drop table lp, coll_pruning, rlp, mc3p, mc2p, boolpart, rp, 
coll_pruning_multi, like_op_noprune, lparted_by_int2, rparted_by_int2;
 --
+-- Test Partition pruning for HASH partitioning
+-- We roll our own operator classes to use for tests, because depending on the
+-- platform-provided hashing functions makes tests unreliable
+--
+CREATE OR REPLACE FUNCTION pp_hashint4_noop(int4, int8) RETURNS int8 AS
+$$SELECT coalesce($1, 0)::int8$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR CLASS pp_test_int4_ops FOR TYPE int4 USING HASH AS
+OPERATOR 1 = , FUNCTION 2 pp_hashint4_noop(int4, int8);
+CREATE OR REPLACE FUNCTION pp_hashtext_length(text, int8) RETURNS int8 AS
+$$SELECT length(coalesce($1, ''))::int8$$ LANGUAGE sql IMMUTABLE;
+CREATE OPERATOR CLASS pp_test_text_ops FOR TYPE text USING HASH AS
+OPERATOR 1 = , FUNCTION 2 pp_hashtext_length(text, int8);
+create table hp (a int, b text) partition by hash (a pp_test_int4_ops, b 
pp_test_text_ops);
+create table hp0 partition of hp for values with (modulus 4, remainder 0);
+create table hp3 partition of hp for values with (modulus 4, remainder 3);
+create table hp1 partition of hp for values with (modulus 4, remainder 1);
+create table hp2 partition of hp for values with (modulus 4, remainder 2);
+insert into hp values (null, null);
+insert into hp values (1, null);
+insert into hp values (1, 'xxx');
+insert into hp values (null, 'xxx');
+insert into hp values (2, 'xxx');
+insert into hp values (1, 'abcde');
+select tableoid::regclass, * from hp order by 1;
+ tableoid | a |   b   
+--+---+---
+ hp0 

Re: [HACKERS] path toward faster partition pruning

[Ashutosh Bapat]

On Wed, Apr 11, 2018 at 2:52 PM, Amit Langote
 wrote:

>>
>> I've attached a delta patch that applies to your v2 which does this.
>> Do you think it's worth doing?
>
> We can see check by inspection that individual values are in appropriate
> partitions, which is the point of having the inserts and the select just
> above the actual pruning related tests.  So, I'm not sure if adding the
> satisfies_hash_partition against each pruning tests adds much.

+1.

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

Re: [sqlsmith] Failed assertion in create_gather_path

[Jeevan Chalke]

On Tue, Apr 10, 2018 at 11:14 PM, Robert Haas  wrote:

> On Tue, Apr 10, 2018 at 2:59 AM, Jeevan Chalke
>  wrote:
> > I actually wanted to have rel->consider_parallel in the condition (yes,
> for
> > additional safety) as we are adding a partial path into rel. But then
> > observed that it is same as that of final_rel->consider_parallel and thus
> > used it along with other condition.
> >
> > I have observed at many places that we do check consider_parallel flag
> > before adding a partial path to it. Thus for consistency added here too,
> but
> > yes, it just adds an additional safety here.
>
> Thanks to Andreas for reporting this issue and to Jeevan for fixing
> it.  My commit 0927d2f46ddd4cf7d6bf2cc84b3be923e0aedc52 seems clearly
> to blame.
>
> The change to set_subquery_pathlist() looks correct, but can we add a
> simple test case?


I have tried adding simple testcase in this version of the patch. This test
hits the Assertion added in add_partial_path() like you have tried.


>   Right now if I keep the new Assert() in
> add_partial_path() and leave out the rest of the changes, the
> regression tests still pass.  That's not so good.  Also, I think I
> would be inclined to wrap the if-statement around the rest of the
> function instead of returning early.
>

OK.
Wrapped if block instead of returning mid-way.


>
> The new Assert() in add_partial_path() is an excellent idea.  I had
> the same thought before, but I didn't do anything about it.  That was
> a bad idea; your plan is better. In fact, I suggest an additional
> Assert() that any relation to which we're adding a partial path is
> marked consider_parallel, like this:
>
> +/* Path to be added must be parallel safe. */
> +Assert(new_path->parallel_safe);
> +
> +/* Relation should be OK for parallelism, too. */
> +Assert(parent_rel->consider_parallel);
>

Yep.
Added this new one too.


>
> Regarding recurse_set_operations, since the rel->consider_parallel is
> always the same as final_rel->consider_parallel there's really no
> value in testing it.  If it were possible for rel->consider_parallel
> to be false even when final_rel->consider_parallel were true then the
> test would be necessary for correctness.  That might or might not
> happen in the future, so I guess it wouldn't hurt to include this for
> future-proofing,


In that case, we should have rel in a condition rather than final_rel as we
are adding a path into rel. For future-proofing added check on
lateral_relids too.


> but it's not actually a bug fix, so it also wouldn't
> hurt to leave it out.  I could go either way, I guess.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>



-- 
Jeevan Chalke
Technical Architect, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
From 708026f34f6a5d087660930f9cb05aa3e08c130b Mon Sep 17 00:00:00 2001
From: Jeevan Chalke 
Date: Wed, 11 Apr 2018 14:08:13 +0530
Subject: [PATCH] Add partial path only when rel's consider_parallel is true.

Otherwise, it will result in an assertion failure later in the
create_gather_path().
---
 src/backend/optimizer/path/allpaths.c | 41 +++
 src/backend/optimizer/prep/prepunion.c|  3 +-
 src/backend/optimizer/util/pathnode.c |  6 
 src/test/regress/expected/select_parallel.out | 19 +
 src/test/regress/sql/select_parallel.sql  |  6 
 5 files changed, 56 insertions(+), 19 deletions(-)

diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 3ba3f87..c3d0634 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -2242,26 +2242,31 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
 		  pathkeys, required_outer));
 	}
 
-	/* If consider_parallel is false, there should be no partial paths. */
-	Assert(sub_final_rel->consider_parallel ||
-		   sub_final_rel->partial_pathlist == NIL);
-
-	/* Same for partial paths. */
-	foreach(lc, sub_final_rel->partial_pathlist)
+	/* If outer rel allows parallelism, do same for partial paths. */
+	if (rel->consider_parallel && bms_is_empty(required_outer))
 	{
-		Path	   *subpath = (Path *) lfirst(lc);
-		List	   *pathkeys;
-
-		/* Convert subpath's pathkeys to outer representation */
-		pathkeys = convert_subquery_pathkeys(root,
-			 rel,
-			 subpath->pathkeys,
-			 make_tlist_from_pathtarget(subpath->pathtarget));
+		/* If consider_parallel is false, there should be no partial paths. */
+		Assert(sub_final_rel->consider_parallel ||
+			   sub_final_rel->partial_pathlist == NIL);
 
-		/* Generate outer path using this subpath */
-		add_partial_path(rel, (Path *)
-		 create_subqueryscan_path(root, rel, subpath,
-  pathkeys, required_outer));
+		/* Same for partial paths. */
+		foreach(lc, sub_final_rel->partial_pathlist)
+		{
+			Path	   *subpath = (Path *) lfirst(lc);
+			List	   *pat

Re: Excessive PostmasterIsAlive calls slow down WAL redo

[Heikki Linnakangas]

On 10/04/18 04:36, Thomas Munro wrote:

On Tue, Apr 10, 2018 at 12:53 PM, Andres Freund  wrote:

I coincidentally got pinged about our current approach causing
performance problems on FreeBSD and started writing a patch.  The
problem there appears to be that constantly attaching events to the read
pipe end, from multiple processes, causes significant contention inside
the kernel. Which isn't that surprising.   That's distinct from the
problem netbsd/openbsd reported a while back (superflous wakeups).

That person said he'd work on adding an equivalent of linux'
prctl(PR_SET_PDEATHSIG) to FreeBSD.


Just an idea, not tested: what about a reusable WaitEventSet with zero
timeout?  Using the kqueue patch, that'd call kevent() which'd return
immediately and tell you if any postmaster death notifications had
arrive on your queue since last time you asked.  It doesn't even touch
the pipe, or any other kernel objects apart from your own queue IIUC.


Hmm. In PostmasterIsAlive(), you'd still need to call kevent() to check 
if postmaster has died. It would just replace the current read() syscall 
on the pipe with the kevent() syscall. Is it faster?


- Heikki

'make check' fails

[Bruce Momjian]

I have discovered that:

make clean; make check

fails with:

/bin/mkdir -p '/pgtop'/tmp_install/log
make -C '.' DESTDIR='/pgtop'/tmp_install install 
>'/pgtop'/tmp_install/log/install.log 2>&1
src/Makefile.global:388: recipe for target 'temp-install' failed
make: *** [temp-install] Error 2

and '/pgtop'/tmp_install/log/install.log says:

relpath.c:21:37: fatal error: catalog/pg_tablespace_d.h: No such file 
or directory
 #include "catalog/pg_tablespace_d.h"
 ^
compilation terminated.
: recipe for target 'relpath.o' failed
make[3]: *** [relpath.o] Error 1

Oddly, this works:

make clean; make; make check

I found this because I have some scripts that do the former.  The
problem has existed for several days now and I only now dug into it.

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

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

Re: ERROR: invalid memory alloc request size 1073741824

[Suhal Vemu]

Hi all,


This is gisdb and my POSTGIS VERSION

-
*gisdb=# SELECT PostGIS_version();*
*postgis_version*
*---*
* 2.4 USE_GEOS=1 USE_PROJ=1 USE_STATS=1*
*(1 row)*
*-*
*It didn't create the table it rolled back .*

*Actually the table creating command is: *

*raster2pgsql -I -C
 
/home/X/Downloads/20161207T055222_20161207T055323_T42QYL/20161207T055222_20161207T055323_T42QYL.ndvi.tif
postgis.gistesttable6 |psql -h localhost -U  -d gisdb -p 5432*


*output :*
*__*
*:~$ raster2pgsql -I -C
 
/home/X/Downloads/20161207T055222_20161207T055323_T42QYL/20161207T055222_20161207T055323_T42QYL.ndvi.tif
postgis.gistesttable6 |psql -h localhost -U XXX -d gisdb -p 5432*
*Processing 1/1:
/home/suhalvemu/Downloads/20161207T055222_20161207T055323_T42QYL/20161207T055222_20161207T055323_T42QYL.ndvi.tif*
*Password for user cropin_suhal: *
*BEGIN*
*CREATE TABLE*
*ERROR:  invalid memory alloc request size 1073741824*
*ERROR:  current transaction is aborted, commands ignored until end of
transaction block*
*ERROR:  current transaction is aborted, commands ignored until end of
transaction block*
*ERROR:  current transaction is aborted, commands ignored until end of
transaction block*
*ROLLBACK*




On Wed, Apr 11, 2018 at 3:16 PM, Suhal Vemu 
wrote:

> Hi David,
>
> This is gisdb and my POSTGIS VERSION
> 
> -
> *gisdb=# SELECT PostGIS_version();*
> *postgis_version*
> *---*
> * 2.4 USE_GEOS=1 USE_PROJ=1 USE_STATS=1*
> *(1 row)*
>
> *-*
> *It didn't create the table it rolled back .*
>
> *Actually the table creating command is: *
>
> *raster2pgsql -I -C
>  
> /home/X/Downloads/20161207T055222_20161207T055323_T42QYL/20161207T055222_20161207T055323_T42QYL.ndvi.tif
> postgis.gistesttable6 |psql -h localhost -U  -d gisdb -p 5432*
>
>
> *output :*
>
> *__*
> *:~$ raster2pgsql -I -C
>  
> /home/X/Downloads/20161207T055222_20161207T055323_T42QYL/20161207T055222_20161207T055323_T42QYL.ndvi.tif
> postgis.gistesttable6 |psql -h localhost -U XXX -d gisdb -p 5432*
> *Processing 1/1:
> /home/suhalvemu/Downloads/20161207T055222_20161207T055323_T42QYL/20161207T055222_20161207T055323_T42QYL.ndvi.tif*
> *Password for user cropin_suhal: *
> *BEGIN*
> *CREATE TABLE*
> *ERROR:  invalid memory alloc request size 1073741824*
> *ERROR:  current transaction is aborted, commands ignored until end of
> transaction block*
> *ERROR:  current transaction is aborted, commands ignored until end of
> transaction block*
> *ERROR:  current transaction is aborted, commands ignored until end of
> transaction block*
> *ROLLBACK*
>
>
>
> On Wed, Apr 11, 2018 at 2:34 PM, David Rowley <
> david.row...@2ndquadrant.com> wrote:
>
>> On 7 April 2018 at 02:14, Suhal Vemu  wrote:
>> > Hi David,
>> >
>> > This is gisdb and my POSTGIS VERSION
>> > 
>> -
>> > gisdb=# SELECT PostGIS_version();
>> > postgis_version
>>
>> Hi Suhal,
>>
>> Please reply including the list. You've not really shown the
>> information I asked for. Best to reply to the list with that
>> information.
>>
>> --
>>  David Rowley   http://www.2ndQuadrant.com/
>>  PostgreSQL Development, 24x7 Support, Training & Services
>>
>
>
>
> --
> Thanks,
> Suhal Vemu
>



-- 
Thanks,
Suhal Vemu

-- 
Please note that this email, including any attachments, is intended solely 
for the individual (s) or entity (ies) to whom they are addressed and may 
contain information that is private, confidential and privileged. In case 
you are not the intended recipient, request you to notify the sender by 
reply mail and delete this email, including any copies or attachments from 
your system. Any unauthorized dissemination, disclosure and/or use of the 
contents of this communication to anyone is strictly prohibited and 
punishable by law.

Re: Excessive PostmasterIsAlive calls slow down WAL redo

[Thomas Munro]

On Wed, Apr 11, 2018 at 10:22 PM, Heikki Linnakangas  wrote:
> On 10/04/18 04:36, Thomas Munro wrote:
>> Just an idea, not tested: what about a reusable WaitEventSet with zero
>> timeout?  Using the kqueue patch, that'd call kevent() which'd return
>> immediately and tell you if any postmaster death notifications had
>> arrive on your queue since last time you asked.  It doesn't even touch
>> the pipe, or any other kernel objects apart from your own queue IIUC.
>
> Hmm. In PostmasterIsAlive(), you'd still need to call kevent() to check if
> postmaster has died. It would just replace the current read() syscall on the
> pipe with the kevent() syscall. Is it faster?

It should be (based on the report of read() being slow here because of
contention on the pipe itself, I guess because of frequent poll() in
WaitLatch() elsewhere?).

But as I said over on another thread[1] (sorry, it got tangled up with
that other conversation about a related topic), maybe testing
getppid() would be simpler and about as fast as possible given you
have to make a syscall (all processes should normally be children of
postmaster, right?).  And only check every nth time through the loop,
as you said, to avoid high frequency syscalls.  I think I might have
been guilty of having a solution looking for a problem, there ;-)

[1] 
https://www.postgresql.org/message-id/CAEepm%3D298omvRS2C8WO%3DCxp%2BWcM-Vn8V3x4_QhxURhKTRCSfYg%40mail.gmail.com

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

Re: Problem while setting the fpw with SIGHUP

[Heikki Linnakangas]

On 09/04/18 11:13, Kyotaro HORIGUCHI wrote:


At Fri, 6 Apr 2018 17:59:58 +0530, Amit Kapila  wrote in 


On Fri, Apr 6, 2018 at 1:50 PM, Kyotaro HORIGUCHI
 wrote:

Hello.

At Wed, 04 Apr 2018 17:26:46 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20180404.172646.238325981.horiguchi.kyot...@lab.ntt.co.jp>

In general, I was wondering why in the first place this variable
(full_page_writes) is a SIGHUP variable?  I think if the user tries to
switch it to 'on' from 'off', it won't guarantee the recovery from
torn pages.  Yeah, one can turn it to 'off' from 'on' without any
problem, but as the reverse doesn't guarantee anything, it can confuse
users. What do you think?


I tend to agree with you. It works as expected after the next
checkpoint. So define the variable as "it can be changed any time
but has an effect at the next checkpoint time", then remove
XLOG_FPW_CHANGE record. And that eliminates the problem of
concurrent calls since the checkpointer becomes the only modifier
of the variable. And the problematic function
UpdateFullPageWrites also no longer needs to write a WAL
record. The information is conveyed only by checkpoint records.


I noticed that XLOG_FPW_CHANGE(fpw=false) is still required by
pg_start/stop_backup to know FPW's turning-off without waiting
for the next checkpoint record. But XLOG_FPW_CHANGE(true) is not
required since no one uses the information. It seems even harmful
when it is written at the incorrect place.

In the attached patch, shared fullPageWrites is updated only at
REDO point


I am not completely sure if that is the right option because this
would mean that we are defining the new scope for a GUC variable.  I
guess we should take the input of others as well.  I am not sure what
is the right way to do that, but maybe we can start a new thread with
a proper subject and description rather than discussing this under
some related bug fix patch discussion.  I guess we can try that after
CF unless some other people pitch in and share their feedback.


I think the new behavior where the GUC only takes effect at next 
checkpoint is OK. It seems quite intuitive.



[rebased patch version]


Looks good at a quick glance. Assuming no objections from others, I'll 
take a closer look and commit tomorrow. Thanks!


- Heikki

Re: Bugs in TOAST handling, OID assignment and redo recovery

[Pavan Deolasee]

On Tue, Apr 10, 2018 at 7:24 PM, Pavan Deolasee 
wrote:

> Hi Heikki,
>
> On Tue, Apr 10, 2018 at 7:07 PM, Heikki Linnakangas 
> wrote:
>
>>
>>>
>> It would seem more straightforward to add a snapshot parameter to
>> GetNewOidWithIndex(), so that the this one caller could pass SnapshotToast,
>> while others pass SnapshotDirty. With your patch, you check the index
>> twice: first with SnapshotDirty, in GetNewOidWithIndex(), and then with
>> SnapshotToast, in the caller.
>>
>
> Hmm. I actually wrote my first patch exactly like that. I am trying to
> remember why I discarded that approach. Was that to do with the fact that  
> SnapshotToast
> can't see all toast tuples either? Yeah, I think so. For example, it won't
> see tuples with uncommitted-xmin, leading to different issues. Now it's
> unlikely that we will have a OID conflict where the old tuple has
> uncommitted-xmin, but not sure if we can completely rule that out.
>

Or may be we simply err on the side of caution and scan the toast table
with SnapshotAny while looking for a duplicate? That might prevent us from
reusing an OID for a known-dead tuple, but should save us a second index
scan and still work.

Thanks,
Pavan

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

Re: [PATCH] Add missing type conversion functions for PL/Python

[David Steele]

On 4/11/18 2:36 AM, Haozhou Wang wrote:
> 
> Thanks for your email.
> Just wondering will I need to re-submit this patch?

No need to resubmit, the CF entry has been moved here:

https://commitfest.postgresql.org/18/1499/

You should have a look at Nikita's patch, though.

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

Re: [HACKERS] path toward faster partition pruning

[David Rowley]

On 11 April 2018 at 21:22, Amit Langote  wrote:
>> Also just wondering if it's worth adding some verification that we've
>> actually eliminated the correct partitions by backing the tests up
>> with a call to satisfies_hash_partition.
>>
>> I've attached a delta patch that applies to your v2 which does this.
>> Do you think it's worth doing?
>
> We can see check by inspection that individual values are in appropriate
> partitions, which is the point of having the inserts and the select just
> above the actual pruning related tests.  So, I'm not sure if adding the
> satisfies_hash_partition against each pruning tests adds much.

Right, that's true.

> Attached revised patch.

Thanks. It looks fine to me, with or without the
satisfies_hash_partition tests. I agree that they're probably
overkill, but I see you've added them now.

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

Re: PostgreSQL's handling of fsync() errors is unsafe and risks data loss at least on XFS

[Jonathan Corbet]

On Tue, 10 Apr 2018 17:40:05 +0200
Anthony Iliopoulos  wrote:

> LSF/MM'18 is upcoming and it would
> have been the perfect opportunity but it's past the CFP deadline.
> It may still worth contacting the organizers to bring forward
> the issue, and see if there is a chance to have someone from
> Pg invited for further discussions.

FWIW, it is my current intention to be sure that the development
community is at least aware of the issue by the time LSFMM starts.

The event is April 23-25 in Park City, Utah.  I bet that room could be
found for somebody from the postgresql community, should there be
somebody who would like to represent the group on this issue.  Let me
know if an introduction or advocacy from my direction would be helpful.

jon

Re: PostgreSQL's handling of fsync() errors is unsafe and risks data loss at least on XFS

[Greg Stark]

On 10 April 2018 at 19:58, Joshua D. Drake  wrote:
> You can't unmount the file system --- that requires writing out all of the 
> pages
> such that the dirty bit is turned off.

I always wondered why Linux didn't implement umount -f. It's been in
BSD since forever and it's a major annoyance that it's missing in
Linux. Even without leaking memory it still leaks other resources,
causes confusion and awkward workarounds in UI and automation
software.

-- 
greg

Re: User defined data types in Logical Replication

[Đặng Minh Hướng]

2018-04-11 10:16 GMT+09:00 Masahiko Sawada :

> On Mon, Mar 19, 2018 at 7:57 PM, Masahiko Sawada 
> wrote:
> >
> > Attached an updated test patch added the above test(0002 patch). Since
> > for this test case it's enough to use existing test functions I didn't
> > create new test functions. Also I found that the local data type name
> > in log for data type conversion isn't qualified whereas the remote
> > data type is always qualified. Attached 0001 patch fixes that.
> >
>
> The original issue has been fixed and this entry on CommitFest has
> been marked as "Committed" but there are still works for improving
> testing. Perhaps I should register a new entry of remaining patches to
> next CommitFest.


Thanks. I appreciate it.

---
Dang Minh Huong

Re: [HACKERS] path toward faster partition pruning

[Alvaro Herrera]

Here's an idea.  Why don't we move the function/opclass creation lines
to insert.sql, without the DROPs, and use the same functions/opclasses
in the three tests insert.sql, alter_table.sql, hash_part.sql and
partition_prune.sql, i.e. not recreate what are essentially the same
objects three times?  This also leaves them around for the pg_upgrade
test, which is not a bad thing.

(This would require a few updates to insert.sql because the definitions
there are different, but it shouldn't be a problem coverage-wise.)

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

Re: pgbench doc typos

[Peter Eisentraut]

On 3/30/18 03:30, Edmund Horner wrote:
> On 30 March 2018 at 19:26, Fabien COELHO  wrote:
>> Thanks for the check. You might consider turning the patch as ready in the
>> cf app.
> 
> Ok, I have done so, since the patch is small and simple.
> 
>>> Fixing the abs/hash bracketing seems clear.  The wasn't sure about
>>> rewriting "mod(i, bj)" as "mod(i, j)", because there could be some
>>> convention about parameter names, but I can't think of anything the "b"
>>> could be in the case of mod.
>>
>>
>> Might be a left-over a previous version which might have used MOD(a, b), but
>> a/b are rather used for fp numbers and i/j for integers.
> 
> Ah that seems possible.

committed

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

Re: [WIP] Document update for Logical Replication security

[Peter Eisentraut]

On 3/3/18 07:35, Shinoda, Noriyoshi wrote:
> Hi, Hackers, 
> 
> The attached patch adds the following information to the document on Logical 
> Replication.
> About the requirement of connection role of Logical Replication, written in 
> 31.7 of the manual is as follows.
> --
> The role used for the replication connection must have the REPLICATION 
> attribute.
> --
> However, the Logical Replication connection role also requires the LOGIN 
> attribute.
> And, for initial snapshots of Logical Replication, the connection role 
> requires SELECT privilege on the replication target table, but it is not 
> described in the manual.

Committed, thanks.


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

Re: segmentation fault in pg head with SQL function.

[Peter Eisentraut]

On 3/16/18 11:40, Tom Lane wrote:
> Michael Paquier  writes:
>> On Fri, Mar 16, 2018 at 11:35:13AM +0530, Prabhat Sahu wrote:
>>> postgres=# CREATE OR REPLACE FUNCTION func1() RETURNS VOID
>>> LANGUAGE SQL
>>> AS $$
>>> select 10;
>>> $$;
> 
>> Problem reproducible here, and the bug has been introduced by fd1a421f.
>> It seems to me that the function should not be authorized to be created
>> to begin with, as it returns an integer in its last query, where I think
>> that check_sql_fn_retval is doing it wrong when called in
>> inline_function() as we know that it handles a function, and not a
>> procedure thanks to the first sanity checks at the top of the function.
> 
> Hm.  Actually, I think this is my fault.  It is true that previous PG
> versions would have rejected this function definition, but my intention
> while revising Peter's prokind patch was that we'd start allowing a
> VOID-returning SQL function to contain anything, and just ignore whatever
> the last statement within it might be.  The documentation doesn't say
> much about VOID-returning SQL functions, but I certainly don't see
> anything saying that they can't end with a SELECT, so arguably the old
> behavior is a POLA violation.  In any case, this is the behavior we
> need for VOID-returning procedures, and there seems little reason not
> to make functions act similarly.
> 
> So apparently I missed something with that.  Will look more closely.

This was listed as an open item, but it was already fixed by
877cdf11eaa9cabcb9b1e3c1bef0760fe08efdc3, so I'll remove it.

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

Re: Creation of wiki page for open items of v11

[Peter Eisentraut]

On 2/18/18 22:42, Michael Paquier wrote:
> Okay, I have created a skeleton of page here:
> https://wiki.postgresql.org/wiki/PostgreSQL_11_Open_Items
> Feel free to add any bugs or issues related to v11 that need to be
> tracked before the release.

Do people find it useful to move the resolved items to a separate
section on the page, instead of just removing them?  I'm not sure that
the resolved sections are useful, compared to just using the git log.

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

Re: lazy detoasting

[Amit Kapila]

On Wed, Apr 11, 2018 at 2:34 AM, Chapman Flack  wrote:
> On 04/10/2018 04:03 PM, Robert Haas wrote:
>
>> I suspect you want, or maybe need, to use the same snapshot as the
>> scan that retrieved the tuple containing the toasted datum.
>
> I'm sure it's worth more than that, but I don't know if it's
> implementable.
>
> If I'm a function, and the datum came to me as a parameter, I may
> have no way to determine what snapshot the enclosing query used to
> obtain the thing passed to me.
>

Before query execution, we push the active snapshot, so any time
during execution, if you get the active snapshot via
GetActiveSnapshot(), you can access it.  So, I think during a function
execution, if you use GetActiveSnapshot, you should get the snapshot
used by enclosing query.

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

Re: [HACKERS] GSoC 2017: Foreign Key Arrays

[Alvaro Herrera]

Mark Rofail wrote:

> > In particular: it seemed to me that you decided to throw away the idea
> > of the new GIN operator without sufficient evidence that it was
> > unnecessary.
> 
> I have to admit to that. But in my defence @> is also GIN indexable so the
> only difference in performance between 'anyarray @>> anyelement' and
> 'anyarray @> ARRAY [anyelement]' is the delay caused by the ARRAY[]
> operation theoretically.

I think I need to review Tom's bounce-for-rework email
https://postgr.es/m/28389.1351094...@sss.pgh.pa.us
to respond to this intelligently.  Tom mentioned @> there but there was
a comment about the comparison semantics used by that operator, so I'm
unclear on whether or not that issue has been fixed.

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

Re: submake-errcodes

[Tom Lane]

Christoph Berg  writes:
> It still doesn't work on current HEAD:

*What* still doesn't work on current HEAD?  I don't know what commands
you are running to get this.

regards, tom lane

Re: Boolean partitions syntax

[Jonathan S. Katz]

> On Apr 11, 2018, at 4:33 AM, Kyotaro HORIGUCHI 
>  wrote:
> 
> Thank you for the comments.
> 
> At Wed, 11 Apr 2018 13:51:55 +0900, Amit Langote 
>  wrote in 
> <3d0fda29-986c-d970-a22c-b4bd44f56...@lab.ntt.co.jp>
>> Horiguchi-san,
>> 
>> Thanks for working on this.
>> 
>> On 2018/04/11 13:20, Kyotaro HORIGUCHI wrote:
>>> At Wed, 11 Apr 2018 11:27:17 +0900, Amit Langote wrote:
 On 2018/04/11 10:44, Tom Lane wrote:
> Kyotaro HORIGUCHI  writes:
>> At least partition bound *must* be a constant. Any expression
>> that can be reduced to a constant at parse time ought to be
>> accepted but must not be accepted if not.
> 
> My point is that *any* expression can be reduced to a constant,
> we just have to do so.
>>> 
>>> Agreed in that sense. What was in my mind was something like
>>> column reference, random() falls into reducible category of
>>> course.
>>> 
>>> # I regard the change in gram.y is regarded as acceptable as a
>>> # direction, so I'll continue to working on this.
>> 
>> I haven't yet reviewed the grammar changes in detail yet...
> 
 I think what Tom is proposing here, instead of bailing out upon
 eval_const_expressions() failing to simplify the input expression to a
 Const, is to *invoke the executor* to evaluate the expression, like the
 optimizer does in evaluate_expr, and cook up a Const with whatever comes
 out to store it into the catalog (that is, in relpartbound).
>>> 
>>> Yes. In the attached I used evaluate_expr by making it non-static
>>> function. a_expr used instead of partbound_datum is changed to
>>> u_expr, which is the same with range bounds.
>>> 
 =# create table c1 partition of p for values in (random() * 100);
 CREATE TABLE
 =# \d c1
>>> ...
 Partition of: p FOR VALUES IN (97)
>> 
>> I looked at the non-gram.y portions of the patch for now as I was also
>> playing with this.  Some comments on your patch:
>> 
>> * You missed adding a break here for the EXPR_KIND_PARTITION_EXPRESSION case
>> 
>> case EXPR_KIND_PARTITION_EXPRESSION:
>> err = _("window functions are not allowed in partition key
>> expressions");
>> +case EXPR_KIND_PARTITION_BOUNDS:
>> +err = _("window functions are not allowed in partition bounds");
>> break;
>> 
>> So, the following is the wrong error message that you probably failed to
>> notice:
> 
> Oops! Fixed along with another one. I haven't looked the
> difference so seriously.
> 
>> --- a/src/test/regress/expected/create_table.out
>> +++ b/src/test/regress/expected/create_table.out
>> @@ -308,7 +308,7 @@ CREATE TABLE partitioned (
>> a int,
>> b int
>> ) PARTITION BY RANGE ((avg(a) OVER (PARTITION BY b)));
>> -ERROR:  window functions are not allowed in partition key expressions
>> +ERROR:  window functions are not allowed in partition bounds
> 
> I felt a bit uneasy to saw that in the very corner of my mind..
> 
>> * I think the new ParseExprKind you added should omit the last "S", that
>> is, name it EXPR_KIND_PARTITION_BOUND, because these are expressions to
>> represent individual bound values.  And so adjust the comment to say "bound".
>> 
>> +EXPR_KIND_PARTITION_BOUNDS, /* partition bounds value */
> 
> Agreed.
> 
>> * When looking at the changes to transformPartitionBoundValue, I noticed
>> that there is no new argument Oid colTypColl
>> 
>> static Const *
>> -transformPartitionBoundValue(ParseState *pstate, A_Const *con,
>> +transformPartitionBoundValue(ParseState *pstate, Node *val,
>>  const char *colName, Oid colType, int32
>> colTypmod)
>> 
>> that's because you seem to pass the expression's type, typmod, and typcoll
>> to the newly added call to evaluate_expr.  I wonder if we should really
>> pass the partition key specified values here.  We already receive first
>> two from the caller.
> 
> I overlooked that the value (Expr) is already coerced at the
> time. Added collation handling and this would be back-patchable.
> 
> I'm not sure how we should handle collate in this case but the
> patch rejects bound values with non-default collation if it is
> different from partition key.
> 
> Collation check works
> 
> =# create table p (a text collate "de_DE") partition by (a)
> =# create table c1 partition of p for values in (('a' collate "ja_JP"));
> ERROR:  collation mismatch between partition key expression (12622) and 
> partition bound value (12684)
> LINE 1: create table c1 partition of p for values in (('a' collate "...
> 
>> * In the changes to create_table.out
>> 
>> @@ -450,13 +450,9 @@ CREATE TABLE part_1 PARTITION OF list_parted FOR
>> VALUES IN ('1');
>> CREATE TABLE part_2 PARTITION OF list_parted FOR VALUES IN (2);
>> CREATE TABLE part_null PARTITION OF list_parted FOR VALUES IN (null);
>> CREATE TABLE fail_part PARTITION OF list_parted FOR VALUES IN (int '1');
>> -ERROR:  syntax error at or near "int"
>> -LINE 1: ... fail_part PARTITION OF list_parted FOR VALUES IN (int '1

Re: Partitioned tables and covering indexes

[Teodor Sigaev]

Thank you, pushed

Amit Langote wrote:

Hi.

On 2018/04/11 0:36, Teodor Sigaev wrote:

     Does the attached fix look correct?  Haven't checked the fix with
ATTACH
     PARTITION though.


Attached patch seems to fix the problem.  However, I would rather get
rid of modifying stmt->indexParams.  That seems to be more logical
for me.  Also, it would be good to check some covering indexes on
partitioned tables.  See the attached patch.


Seems right way, do not modify incoming object and do not copy rather
large and deep nested structure as suggested by Amit.


Yeah, Alexander's suggested way of using a separate variable for
indexParams is better.


But it will  be better to have a ATTACH PARTITION test too.


I have added tests.  Actually, instead of modifying existing tests, I
think it might be better to have a separate section at the end of
indexing.sql to test covering indexes feature for partitioned tables.

Attached find updated patch.

Thanks,
Amit



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

Re: Creation of wiki page for open items of v11

[Ashutosh Bapat]

On Wed, Apr 11, 2018 at 6:53 PM, Peter Eisentraut
 wrote:
> On 2/18/18 22:42, Michael Paquier wrote:
>> Okay, I have created a skeleton of page here:
>> https://wiki.postgresql.org/wiki/PostgreSQL_11_Open_Items
>> Feel free to add any bugs or issues related to v11 that need to be
>> tracked before the release.
>
> Do people find it useful to move the resolved items to a separate
> section on the page, instead of just removing them?  I'm not sure that
> the resolved sections are useful, compared to just using the git log.

If we have a section for resolved items, we can keep track of all
items. If we just delete the resolved items, we wouldn't know if it
was a mistake or it was intentional removal.



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

Re: WIP: Covering + unique indexes.

[Teodor Sigaev]

_bt_mark_page_halfdead() looked like it had a problem, but it now
looks like I was wrong. I also verified every other
BTreeInnerTupleGetDownLink() caller. It now looks like everything is
good here.


Right - it tries to find right page by conlsulting in parent page, by taking of 
next key.




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

Re: submake-errcodes

[Devrim Gündüz]

Hi,

On Wed, 2018-04-11 at 09:38 -0400, Tom Lane wrote:
> *What* still doesn't work on current HEAD?  I don't know what commands
> you are running to get this.

I think my build and Christoph's builds fail because of the same reason again
(the same as yesterday):

==
+ cd src/pl/plpython
+ make all


gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement 
-Wendif-labels -Wmissing-format-attribute -Wformat-security 
-fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O2 -g -pipe -Wall 
-Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions 
-fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches 
-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic 
-fasynchronous-unwind-tables -fPIC -I. -I. -I/usr/include/python3.6m 
-I../../../src/include  -D_GNU_SOURCE -I/usr/include/libxml2  -I/usr/include  
-c -o plpy_cursorobject.o plpy_cursorobject.c
In file included from ../../../src/include/postgres.h:47:0,
 from plpy_cursorobject.c:7:
../../../src/include/utils/elog.h:71:10: fatal error: utils/errcodes.h: No such 
file or directory
 #include "utils/errcodes.h"
  ^~
compilation terminated.
make[1]: *** [: plpy_cursorobject.o] Error 1
===

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

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

Re: submake-errcodes

[Tom Lane]

Devrim =?ISO-8859-1?Q?G=FCnd=FCz?=  writes:
> On Wed, 2018-04-11 at 09:38 -0400, Tom Lane wrote:
>> *What* still doesn't work on current HEAD?  I don't know what commands
>> you are running to get this.

> I think my build and Christoph's builds fail because of the same reason again
> (the same as yesterday):

> + cd src/pl/plpython
> + make all

That works for me:

$ git clean -dfx
$ ./configure ... --with-python and other stuff ...
$ cd src/pl/plpython
$ make all
make -C ../../../src/backend generated-headers
make[1]: Entering directory `/home/postgres/pgsql/src/backend'
make -C catalog distprep generated-header-symlinks
make[2]: Entering directory `/home/postgres/pgsql/src/backend/catalog'
...
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement 
-Wendif-labels -Wmissing-format-attribute -Wformat-security 
-fno-strict-aliasing -fwrapv -g -O2 -fPIC -I. -I. -I/usr/include/python2.6 
-I../../../src/include  -D_GNU_SOURCE   -c -o plpy_util.o plpy_util.c
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement 
-Wendif-labels -Wmissing-format-attribute -Wformat-security 
-fno-strict-aliasing -fwrapv -g -O2 -fPIC -shared -o plpython2.so 
plpy_cursorobject.o plpy_elog.o plpy_exec.o plpy_main.o plpy_planobject.o 
plpy_plpymodule.o plpy_procedure.o plpy_resultobject.o plpy_spi.o 
plpy_subxactobject.o plpy_typeio.o plpy_util.o  -L../../../src/port 
-L../../../src/common-Wl,--as-needed 
-Wl,-rpath,'/usr/lib64',--enable-new-dtags  -L/usr/lib64 -lpython2.6 -lpthread 
-ldl  -lutil -lm  

You sure you're on 31f1f0bb4fd642643994d35c35ecb5b929711a99 or later?
Which gmake version is this?

regards, tom lane

Re: lazy detoasting

[Chapman Flack]

On 04/10/2018 10:17 PM, Jan Wieck wrote:
> If your session has a transaction snapshot that protects the old toast
> slices from being vacuumed away, you are fine.

That harks back to my earlier (naïve?) thought that, as long as
my lazy datum doesn't have to outlive the transaction, lazy
detoasting might Just Work.

Tom seems to say otherwise, in
https://www.postgresql.org/message-id/23711.1522559987%40sss.pgh.pa.us

The message of the commit he mentions there includes:

  I believe this code was all right when written, because our
  management of a session's exposed xmin was such that the TOAST
  references were safe until end of transaction.  But that's
  no longer true now that we can advance or clear our PGXACT.xmin
  intra-transaction.

So perhaps my original thought really would have Just Worked,
in PG versions before that changed? When did that change, btw?

On 04/11/2018 09:28 AM, Amit Kapila wrote:
> Before query execution, we push the active snapshot, so any time
> during execution, if you get the active snapshot via
> GetActiveSnapshot(), you can access it.  So, I think during a function
> execution, if you use GetActiveSnapshot, you should get the snapshot
> used by enclosing query.

So perhaps GetActiveSnapshot is the right choice to accompany a datum
that came as a function parameter.

For something retrieved from an SPI query within the function, it
looks like I will have a portal->holdSnapshot in certain cases
(PORTAL_ONE_RETURNING, PORTAL_ONE_MOD_WITH, PORTAL_UTIL_SELECT).

The comments added to portal.h in that commit say:

* Snapshot under which tuples in the holdStore were read.  We must keep
* a reference to this snapshot if there is any possibility that the
* tuples contain TOAST references, because releasing the snapshot ...

Soo, is that telling me that the three cases named above, where
holdSnapshot gets set, are in fact the only cases where "there is any
possibility that the tuples contain TOAST references", and therefore
I could count on holdSnapshot to be nonnull whenever it matters?

-Chap

Re: PostgreSQL's handling of fsync() errors is unsafe and risks data loss at least on XFS

[Andres Freund]

Hi,

On 2018-04-11 06:05:27 -0600, Jonathan Corbet wrote:
> The event is April 23-25 in Park City, Utah.  I bet that room could be
> found for somebody from the postgresql community, should there be
> somebody who would like to represent the group on this issue.  Let me
> know if an introduction or advocacy from my direction would be helpful.

If that room can be found, I might be able to make it. Being in SF, I'm
probably the physically closest PG dev involved in the discussion.

Thanks for chiming in,

Andres

Re: Transform for pl/perl

[Peter Eisentraut]

On 4/10/18 07:33, Dagfinn Ilmari Mannsåker wrote:
> 1) both the jsonb_plperl and jsonb_plperlu extensions contain the SQL
>functions jsonb_to_plperl and plperl_to_jsonb, so can't be installed
>simultaneously
> 
> 2) jsonb scalar values are passed to the plperl function wrapped in not
>one, but _two_ layers of references
> 
> 3) jsonb numeric values are passed as perl's NV (floating point) type,
>losing precision if they're integers that would fit in an IV or UV.
> 
> 4) SV_to_JsonbValue() throws an error for infinite NVs, but not NaNs
> 
> Attached is a patch for the first issue.  I'll look at fixing the rest
> later this week.

Committed #1.  Please keep more patches coming. :)

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

Re: PostgreSQL's handling of fsync() errors is unsafe and risks data loss at least on XFS

[Jonathan Corbet]

On Wed, 11 Apr 2018 07:29:09 -0700
Andres Freund  wrote:

> If that room can be found, I might be able to make it. Being in SF, I'm
> probably the physically closest PG dev involved in the discussion.

OK, I've dropped the PC a note; hopefully you'll be hearing from them.

jon

Re: lazy detoasting

[Tom Lane]

Chapman Flack  writes:
> On 04/10/2018 10:17 PM, Jan Wieck wrote:
>> If your session has a transaction snapshot that protects the old toast
>> slices from being vacuumed away, you are fine.

> That harks back to my earlier (naïve?) thought that, as long as
> my lazy datum doesn't have to outlive the transaction, lazy
> detoasting might Just Work.

> Tom seems to say otherwise, in
> https://www.postgresql.org/message-id/23711.1522559987%40sss.pgh.pa.us

> The message of the commit he mentions there includes:

>   I believe this code was all right when written, because our
>   management of a session's exposed xmin was such that the TOAST
>   references were safe until end of transaction.  But that's
>   no longer true now that we can advance or clear our PGXACT.xmin
>   intra-transaction.

> So perhaps my original thought really would have Just Worked,
> in PG versions before that changed? When did that change, btw?

When snapmgr.c came in, which seems to have been 8.4.

The core of the problem now is that in a READ COMMITTED transaction, we
may not be holding any snapshot at all between statements.  So if you're
hanging onto a toast pointer across statements you're at risk.

On the other hand, it's also arguable that you shouldn't be interested
in dereferencing such a pointer later than the statement in which it
was read, precisely because it no longer represents accessible data.
So maybe we need to take two steps back and talk about the semantics
of what you're doing.

regards, tom lane

Re: Bugs in TOAST handling, OID assignment and redo recovery

[Tom Lane]

Pavan Deolasee  writes:
> Or may be we simply err on the side of caution and scan the toast table
> with SnapshotAny while looking for a duplicate? That might prevent us from
> reusing an OID for a known-dead tuple, but should save us a second index
> scan and still work.

+1.  We really don't want to expend two indexscans on this.

I was worried about changing the signature of GetNewOidWithIndex in
a back-patched fix, but after looking around I think that's probably
safe.  External callers really shouldn't be using anything lower-level
than GetNewOid.

regards, tom lane

Re: Creation of wiki page for open items of v11

[Tom Lane]

Ashutosh Bapat  writes:
> On Wed, Apr 11, 2018 at 6:53 PM, Peter Eisentraut
>  wrote:
>> Do people find it useful to move the resolved items to a separate
>> section on the page, instead of just removing them?  I'm not sure that
>> the resolved sections are useful, compared to just using the git log.

> If we have a section for resolved items, we can keep track of all
> items. If we just delete the resolved items, we wouldn't know if it
> was a mistake or it was intentional removal.

It's not that much work to move the items rather than remove them,
so I'd vote for keeping up the practice.  It has some value in terms
of tracking activity.

What *does* take time is adding a link to the commit, so I'd happily
drop that step.  As Peter says, you can usually look in the commit
log if you care.

regards, tom lane

Re: 'make check' fails

[Tom Lane]

Bruce Momjian  writes:
> I have discovered that:
>   make clean; make check
> fails with:

No doubt this is related to the generated-headers changes I've been
making, but I find your recipe confusing.  "make clean" should not
have removed the generated headers from the previous build.  I can
believe that if you started from a bare git checkout, did configure
and then immediately "make check", that would fail ... but I don't
think that worked before either.  If it did it was certainly subject
to parallel-make race conditions.

Please be more explicit about what state you're starting from.

regards, tom lane

Re: Creation of wiki page for open items of v11

[Robert Haas]

On Wed, Apr 11, 2018 at 10:53 AM, Tom Lane  wrote:
> Ashutosh Bapat  writes:
>> On Wed, Apr 11, 2018 at 6:53 PM, Peter Eisentraut
>>  wrote:
>>> Do people find it useful to move the resolved items to a separate
>>> section on the page, instead of just removing them?  I'm not sure that
>>> the resolved sections are useful, compared to just using the git log.
>
>> If we have a section for resolved items, we can keep track of all
>> items. If we just delete the resolved items, we wouldn't know if it
>> was a mistake or it was intentional removal.
>
> It's not that much work to move the items rather than remove them,
> so I'd vote for keeping up the practice.  It has some value in terms
> of tracking activity.
>
> What *does* take time is adding a link to the commit, so I'd happily
> drop that step.  As Peter says, you can usually look in the commit
> log if you care.

The trouble is that sometimes it's not very obvious which commit log
entry relates to which open item.

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

Re: lazy detoasting

[Chapman Flack]

On 04/11/2018 10:41 AM, Tom Lane wrote:
> The core of the problem now is that in a READ COMMITTED transaction, we
> may not be holding any snapshot at all between statements.  So if you're
> hanging onto a toast pointer across statements you're at risk.
> 
> On the other hand, it's also arguable that you shouldn't be interested
> in dereferencing such a pointer later than the statement in which it
> was read, precisely because it no longer represents accessible data.
> So maybe we need to take two steps back and talk about the semantics
> of what you're doing.

The mission is to implement java.sql.SQLXML, which has long been
missing from PL/Java.

https://docs.oracle.com/javase/6/docs/api/index.html?java/sql/SQLXML.html

The API spec includes this: "An SQLXML object is valid for
the duration of the transaction in which it was created."

This is the class of object your Java code can retrieve from a
ResultSet row from a query with an XML column type. (It's also
what can be passed to you as a function parameter, if your
function's SQL declaration has a parameter type XML.)

An SQLXML object represents the XML value. You get to read the
object (exactly once, or not at all) for the content it represents,
in your choice of a half-dozen different ways corresponding to
available Java XML-processing APIs. (The SQLXML implementation knows
what the underlying stored form is, so it encapsulates the knowledge
of how most efficiently to get it from there to the form the program
wants to work with.)

In most cases I can easily imagine, a function that gets an SQLXML
object is going to read it "pretty soon" ... surely some time during
the function call, if it was passed as a parameter, and probably
within the same row loop iteration, for one retrieved from a query.
However, the spec does explicitly provide that you could, for whatever
reason, sit on the thing for a while, then read it later in the same
transaction. You should get the same stuff you would have if you had
read it right away, whether or not stuff has changed in the database
since. Eager detoasting at the time of creating the object, into a
transaction-scoped memory context, would trivially have that behavior,
but on the chance that XML values might be large, and a function might
conceivably never read the thing at all on certain code paths, I'd
rather defer detoasting until the code holding the SQLXML object
actually wants to read it.

-Chap

Re: lazy detoasting

[Tom Lane]

Chapman Flack  writes:
> On 04/11/2018 10:41 AM, Tom Lane wrote:
>> So maybe we need to take two steps back and talk about the semantics
>> of what you're doing.

> The mission is to implement java.sql.SQLXML, which has long been
> missing from PL/Java.
> This is the class of object your Java code can retrieve from a
> ResultSet row from a query with an XML column type. (It's also
> what can be passed to you as a function parameter, if your
> function's SQL declaration has a parameter type XML.)

OK, but if this object only lives within a single function call,
what's the problem?  The underlying row must be visible to the
calling query, and that condition won't change for the duration
of the call.

Things could get interesting if you consider a Java *procedure*,
but I think you could afford to detoast-on-entry for that case.

(Wanders away wondering what Peter has done about toasted parameter
values for procedures in general ...)

regards, tom lane

Re: Creation of wiki page for open items of v11

[Tom Lane]

Robert Haas  writes:
> On Wed, Apr 11, 2018 at 10:53 AM, Tom Lane  wrote:
>> What *does* take time is adding a link to the commit, so I'd happily
>> drop that step.  As Peter says, you can usually look in the commit
>> log if you care.

> The trouble is that sometimes it's not very obvious which commit log
> entry relates to which open item.

Sure, but is annotating the wiki page that way worth the trouble?
If the alternative is that committers refuse to update the wiki page
at all, or decide to remove entries rather than move-and-add-a-link,
we're not coming out ahead.

I'm not particularly wedded to the above idea; I'm just proposing it
as a compromise solution.

regards, tom lane

Re: WARNING in parallel index creation.

[Andres Freund]

On 2018-03-12 20:44:01 -0700, Peter Geoghegan wrote:
> On Sun, Mar 11, 2018 at 10:15 PM, Jeff Janes  wrote:
> > Then when I create in index, I get a warning:
> >
> > jjanes=# create index on pgbench_accounts (foobar(filler));
> > WARNING:  cannot set parameters during a parallel operation
> > WARNING:  cannot set parameters during a parallel operation
> >
> > If I create the index again within the same session, there is no WARNING.
> >
> > This only occurs if plperl.on_init is set in the postgresql.conf file.  It
> > doesn't seem to matter what it is set to,
> > even the empty string triggers the warning.
> 
> I wonder why DefineCustomStringVariable() does not set var->reset_val.
> We see that within DefineCustomEnumVariable(),
> DefineCustomRealVariable(), DefineCustomIntVariable(), etc.

Peter, have you investigated this any further? This has been an open
item for a while.

Greetings,

Andres Freund

Re: es_query_dsa is broken

[Andres Freund]

Hi,

On 2017-12-07 12:51:56 +1300, Thomas Munro wrote:
> 1.  Removing es_query_dsa and injecting the right context into the
> executor tree as discussed.
> 
> 2.  Another idea mentioned by Robert in an off-list chat:  We could
> consolidate all DSM segments in a multi-gather plan into one.  See the
> nearby thread where someone had over a hundred Gather nodes and had to
> crank up max_connections to reserve enough DSM slots.  Of course,
> optimising for that case doesn't make too much sense (I suspect
> multi-gather plans will become less likely with Parallel Append and
> Parallel Hash in the picture anyway), but it would reduce a bunch of
> duplicated work we do when it happens as well as scarce slot
> consumption.  If we did that, then all of a sudden es_query_dsa would
> make sense again (ie it'd be whole-query scoped), and we could revert
> that hacky change.
> 
> Or we could do both things anyway.

This is an open item for v11:

 Tidy up es_query_dsa and possibly ParallelWorkerContext?
Original commit: e13029a5ce353574516c64fd1ec9c50201e705fd (principal 
author: Thomas Munro; owner: Robert Haas)
Bug fix: fd7c0fa732d97a4b4ebb58730e6244ea30d0a618
While the bug was fixed with something back-patchable, we should 
considering improving this situation. As discussed in the above-linked thread, 
options might include (1) getting rid of es_query_dsa entirely and injecting 
dependencies into nodes, (2) making all Gather nodes share the same DSM segment 
so there truly could be per-query DSA segment. 

Do we want to make any changes here for v11? If not, are we ok with just
closing the entry and waiting till it bugs anybody for some reason?

Greetings,

Andres Freund

Re: missing support of named convention for procedures

[Andres Freund]

Hi Peter, Pavel,

On 2018-03-22 15:19:12 +0100, Pavel Stehule wrote:
> attached patch should to fix it

This is still broken, and has been an open item for a bit. Peter, Could
you check whether Pavel's fix resolves the issue for you?

Regards,

Andres

Re: 'make check' fails

[Bruce Momjian]

On Wed, Apr 11, 2018 at 10:59:45AM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > I have discovered that:
> > make clean; make check
> > fails with:
> 
> No doubt this is related to the generated-headers changes I've been
> making, but I find your recipe confusing.  "make clean" should not
> have removed the generated headers from the previous build.  I can
> believe that if you started from a bare git checkout, did configure
> and then immediately "make check", that would fail ... but I don't
> think that worked before either.  If it did it was certainly subject
> to parallel-make race conditions.
> 
> Please be more explicit about what state you're starting from.

OK, I can reproduce it with this:

make distclean; configure; make clean check

This is not supposed to work?  It should be this?

make distclean; configure; make clean; make; make check

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

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

Re: relispartition for index partitions

[Andres Freund]

Hi,

On 2018-01-26 18:57:03 +0900, Amit Langote wrote:
> I noticed that relispartition isn't set for index's partitions.
> 
> create table p (a int) partition by list (a);
> create table p12 partition of p for values in (1, 2);
> create index on p (a);
> select relname, relkind from pg_class where relnamespace =
> 'public'::regnamespace and relispartition is true;
>  relname | relkind
> -+-
>  p12 | r
> (1 row)
> 
> Is that intentional?

This appears to be a question about
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=8b08f7d4820fd7a8ef6152a9dd8c6e3cb01e5f99
et al.  Could you look into it?  It's been an open item for quite a
while.

Greetings,

Andres Freund

Re: relispartition for index partitions

[Alvaro Herrera]

Hello

Andres Freund wrote:

> On 2018-01-26 18:57:03 +0900, Amit Langote wrote:
> > I noticed that relispartition isn't set for index's partitions.

> > Is that intentional?
> 
> This appears to be a question about
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=8b08f7d4820fd7a8ef6152a9dd8c6e3cb01e5f99
> et al.  Could you look into it?  It's been an open item for quite a
> while.

Sure.  I'll get this done this week.

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

Re: 'make check' fails

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 Tom> No doubt this is related to the generated-headers changes I've
 Tom> been making, but I find your recipe confusing. "make clean" should
 Tom> not have removed the generated headers from the previous build. I
 Tom> can believe that if you started from a bare git checkout, did
 Tom> configure and then immediately "make check", that would fail ...
 Tom> but I don't think that worked before either. If it did it was
 Tom> certainly subject to parallel-make race conditions.

 Tom> Please be more explicit about what state you're starting from.

This is consistently failing for me, on FreeBSD with GNU Make 4.2.1,
clang 3.9.1, at commit 651cb90941:

git clean -dfx
./configure '--prefix=/home/andrew/work/pgsql/head' \
 '--with-includes=/usr/local/include' '--with-libs=/usr/local/lib' \
 '--enable-debug' '--enable-depend' 'CFLAGS=-O0' '--enable-cassert' \
gmake -j4
gmake clean
gmake -j4 check

install.log shows:

relpath.c:21:10: fatal error: 'catalog/pg_tablespace_d.h' file not found
#include "catalog/pg_tablespace_d.h"
 ^
1 error generated.

-- 
Andrew (irc:RhodiumToad)

Re: WARNING in parallel index creation.

[Tom Lane]

Andres Freund  writes:
> On 2018-03-12 20:44:01 -0700, Peter Geoghegan wrote:
>> I wonder why DefineCustomStringVariable() does not set var->reset_val.
>> We see that within DefineCustomEnumVariable(),
>> DefineCustomRealVariable(), DefineCustomIntVariable(), etc.

> Peter, have you investigated this any further? This has been an open
> item for a while.

It looks to me like the "oversight" in DefineCustomStringVariable is
not really one; rather, the reset_val assignments in the sibling
routines are dead code.  define_custom_variable will call
InitializeOneGUCOption which overwrites reset_val from the boot_val
in all cases.  We should probably clean this up by removing the dead
assignments.

The WARNING seems to indicate that the error check in set_config_option
is too aggressive.  I kind of wonder why it was placed there at all,
rather than in SQL-level operations like ExecSetVariableStmt.

regards, tom lane

Re: 'make check' fails

[Tom Lane]

Andrew Gierth  writes:
>  Tom> Please be more explicit about what state you're starting from.

> This is consistently failing for me, on FreeBSD with GNU Make 4.2.1,
> clang 3.9.1, at commit 651cb90941:

> git clean -dfx
> ./configure '--prefix=/home/andrew/work/pgsql/head' \
>  '--with-includes=/usr/local/include' '--with-libs=/usr/local/lib' \
>  '--enable-debug' '--enable-depend' 'CFLAGS=-O0' '--enable-cassert' \
> gmake -j4
> gmake clean
> gmake -j4 check

Oh ... I forgot that "make clean" removes the symlinks in
src/include/catalog.  Still, that also removes
src/include/utils/errcodes.h, so there was a parallel-make hazard
here before too.

We can fix this by making submake-generated-headers be a recursive
prerequisite for "check" as well as "all" and "install".  I wonder
whether anybody is expecting any other shortcuts to work.

regards, tom lane

Re: 'make check' fails

[Bruce Momjian]

On Wed, Apr 11, 2018 at 12:35:41PM -0400, Tom Lane wrote:
> Andrew Gierth  writes:
> >  Tom> Please be more explicit about what state you're starting from.
> 
> > This is consistently failing for me, on FreeBSD with GNU Make 4.2.1,
> > clang 3.9.1, at commit 651cb90941:
> 
> > git clean -dfx
> > ./configure '--prefix=/home/andrew/work/pgsql/head' \
> >  '--with-includes=/usr/local/include' '--with-libs=/usr/local/lib' \
> >  '--enable-debug' '--enable-depend' 'CFLAGS=-O0' '--enable-cassert' \
> > gmake -j4
> > gmake clean
> > gmake -j4 check
> 
> Oh ... I forgot that "make clean" removes the symlinks in
> src/include/catalog.  Still, that also removes
> src/include/utils/errcodes.h, so there was a parallel-make hazard
> here before too.
> 
> We can fix this by making submake-generated-headers be a recursive
> prerequisite for "check" as well as "all" and "install".  I wonder
> whether anybody is expecting any other shortcuts to work.

In case it wasn't clear, I can reproduce this failure without parallel
mode.  FYI, the reason I used this shortcut is so I can just run one
command and check one error code to know if they all worked;  see
src/tools/pgtest for an example.

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

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

Re: 'make check' fails

[Alvaro Herrera]

Tom Lane wrote:

> We can fix this by making submake-generated-headers be a recursive
> prerequisite for "check" as well as "all" and "install".  I wonder
> whether anybody is expecting any other shortcuts to work.

check-world certainly, but presumably that depends on check?

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

Re: Partitioned tables and covering indexes

[Teodor Sigaev]

Patch makes buildfarm almost red, patch is temporary reverted.

Actually, discovered bug is not related to patch except new test faces with it,
problem is: CompareIndexInfo() checks rd_opfamily for equality for all 
attributes, not only for key attribute.


Obviously, CompareIndexInfo() needs more work:
for (i = 0; i < info1->ii_NumIndexAttrs; i++)
{
if (maplen < info2->ii_KeyAttrNumbers[i])

Seems, we can go out from ii_KeyAttrNumbers array.




Amit Langote wrote:

Hi.

On 2018/04/11 0:36, Teodor Sigaev wrote:

     Does the attached fix look correct?  Haven't checked the fix with
ATTACH
     PARTITION though.


Attached patch seems to fix the problem.  However, I would rather get
rid of modifying stmt->indexParams.  That seems to be more logical
for me.  Also, it would be good to check some covering indexes on
partitioned tables.  See the attached patch.


Seems right way, do not modify incoming object and do not copy rather
large and deep nested structure as suggested by Amit.


Yeah, Alexander's suggested way of using a separate variable for
indexParams is better.


But it will  be better to have a ATTACH PARTITION test too.


I have added tests.  Actually, instead of modifying existing tests, I
think it might be better to have a separate section at the end of
indexing.sql to test covering indexes feature for partitioned tables.

Attached find updated patch.

Thanks,
Amit



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

Re: Creation of wiki page for open items of v11

[Jonathan S. Katz]

> On Apr 11, 2018, at 11:54 AM, Tom Lane  wrote:
> 
> Robert Haas  writes:
>> On Wed, Apr 11, 2018 at 10:53 AM, Tom Lane  wrote:
>>> What *does* take time is adding a link to the commit, so I'd happily
>>> drop that step.  As Peter says, you can usually look in the commit
>>> log if you care.
> 
>> The trouble is that sometimes it's not very obvious which commit log
>> entry relates to which open item.
> 
> Sure, but is annotating the wiki page that way worth the trouble?
> If the alternative is that committers refuse to update the wiki page
> at all, or decide to remove entries rather than move-and-add-a-link,
> we're not coming out ahead.
> 
> I'm not particularly wedded to the above idea; I'm just proposing it
> as a compromise solution.

During some RMT discussions I had proposed formatting the open items
into a table on the Wiki page with some useful info to help track the status
and surface the necessary info to track down the open item.

It has been on my TODO to have a draft of that.  Perhaps it will help solve
this problem.

Jonathan

Re: 'make check' fails

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> We can fix this by making submake-generated-headers be a recursive
>> prerequisite for "check" as well as "all" and "install".  I wonder
>> whether anybody is expecting any other shortcuts to work.

> check-world certainly, but presumably that depends on check?

world is OK because that already depends on "all" at the top level.
(The reason "check" is failing is basically that the "check: all"
dependencies only appear below the top level, and the MAKELEVEL
hack prevents submake-generated-headers from doing anything in
recursive child makes.)

regards, tom lane

Native partitioning tablespace inheritance

[Keith Fiske]

Just had someone report that pg_partman wasn't handling tablespaces for
native partitioning.

https://github.com/keithf4/pg_partman/issues/212

I'd assumed that that was a property that was being inherited from the
parent table, but apparently the TABLESPACE flag to CREATE TABLE is
completely ignored for the parent table. I know the documentation states
that you can set the tablespace per child table, but accepting the flag on
the parent and completely ignoring it seems rather misleading to me.

keith@keith=# CREATE TABLE (YY TIMESTAMP NOT NULL) PARTITION BY
RANGE (YY ) TABLESPACE mytablespace;
CREATE TABLE
Time: 11.569 ms
keith@keith=# \d+ ;
 Table "public."
 Column |Type | Collation | Nullable | Default |
Storage | Stats target | Description
+-+---+--+-+-+--+-
 yy | timestamp without time zone |   | not null | |
plain   |  |
Partition key: RANGE (yy)

keith@keith=# select relname, reltablespace from pg_class where relname =
'';
 relname | reltablespace
-+---
 | 0
(1 row)


Any chance of this being an inheritable property that can simply be
overridden if the TABLESPACE flag is set when creating a child table? If
it's not set, just set the tablespace to whatever was set for the parent.

For now, partman is going to have to rely on the template table option to
handle this the same way it does for indexes right now.

-- 
Keith Fiske
Senior Database Engineer
Crunchy Data - http://crunchydata.com

Re: Creation of wiki page for open items of v11

[Alvaro Herrera]

Jonathan S. Katz wrote:

> During some RMT discussions I had proposed formatting the open items
> into a table on the Wiki page with some useful info to help track the status
> and surface the necessary info to track down the open item.

The other proposal was that we could have a simple web app to track open
items.  After all, we now know what we need from it.  A wiki page seems
more laborious.  (The commitfest app also sprung from a wiki page.)

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

Re: Creation of wiki page for open items of v11

[Andres Freund]

On 2018-04-11 13:54:34 -0300, Alvaro Herrera wrote:
> The other proposal was that we could have a simple web app to track open
> items.  After all, we now know what we need from it.  A wiki page seems
> more laborious.  (The commitfest app also sprung from a wiki page.)

Growing a number of non-issue issue trackers...

Re: Creation of wiki page for open items of v11

[Magnus Hagander]

On Wed, Apr 11, 2018 at 6:57 PM, Andres Freund  wrote:

> On 2018-04-11 13:54:34 -0300, Alvaro Herrera wrote:
> > The other proposal was that we could have a simple web app to track open
> > items.  After all, we now know what we need from it.  A wiki page seems
> > more laborious.  (The commitfest app also sprung from a wiki page.)
>
> Growing a number of non-issue issue trackers...
>
>
Indeed.

(And of course, if we want to go in *any* direction away from the wiki,
it's not going to happen in time for *this* release..)


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

Re: 'make check' fails

[Tom Lane]

Bruce Momjian  writes:
> On Wed, Apr 11, 2018 at 12:35:41PM -0400, Tom Lane wrote:
>> We can fix this by making submake-generated-headers be a recursive
>> prerequisite for "check" as well as "all" and "install".  I wonder
>> whether anybody is expecting any other shortcuts to work.

> In case it wasn't clear, I can reproduce this failure without parallel
> mode.  FYI, the reason I used this shortcut is so I can just run one
> command and check one error code to know if they all worked;  see
> src/tools/pgtest for an example.

Probably, you got out of the habit of using parallel mode because it
fell over from time to time.  The way things were done before, there
were race conditions in this usage, due to different subdirectories
independently trying to update the same generated headers.

Will commit a fix in a bit.

regards, tom lane

Re: Creation of wiki page for open items of v11

[Alvaro Herrera]

Andres Freund wrote:
> On 2018-04-11 13:54:34 -0300, Alvaro Herrera wrote:
> > The other proposal was that we could have a simple web app to track open
> > items.  After all, we now know what we need from it.  A wiki page seems
> > more laborious.  (The commitfest app also sprung from a wiki page.)
> 
> Growing a number of non-issue issue trackers...

Yes, because the generic ones are confusing, hard to search, easy to
misuse, easy for things to get lost, easy for dupes to crop up --- easy
for things go wrong all over the place.

The dedicated apps have a very limited purpose and work the way we want,
avoiding all these problems: in essence, they are but a glorified
repository of categorized links to our mailing list archives.

We've had wiki pages for open items for 10 years now.  It's gotten
boring and tiresome.
https://wiki.postgresql.org/wiki/Category:Open_Items

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

Re: Creation of wiki page for open items of v11

[Alvaro Herrera]

Magnus Hagander wrote:

> (And of course, if we want to go in *any* direction away from the wiki,
> it's not going to happen in time for *this* release..)

Absolutely.  But if we never start, it'll never get done.

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

Re: Creation of wiki page for open items of v11

[Joshua D. Drake]

On 04/11/2018 10:06 AM, Alvaro Herrera wrote:

Andres Freund wrote:

On 2018-04-11 13:54:34 -0300, Alvaro Herrera wrote:

The other proposal was that we could have a simple web app to track open
items.  After all, we now know what we need from it.  A wiki page seems
more laborious.  (The commitfest app also sprung from a wiki page.)


Growing a number of non-issue issue trackers...


Yes, because the generic ones are confusing, hard to search, easy to
misuse, easy for things to get lost, easy for dupes to crop up --- easy
for things go wrong all over the place.


Just like now except at least with an issue tracker it is easy to move 
items from one place to another, assign ownership and track what is 
actually going on.




The dedicated apps have a very limited purpose and work the way we want,
avoiding all these problems: in essence, they are but a glorified


No, they work the way you want which in turn creates an ever higher 
barrier of entry for new community members.



We've had wiki pages for open items for 10 years now.  It's gotten
boring and tiresome.
https://wiki.postgresql.org/wiki/Category:Open_Items


At least with an issue tracker it would be easy to see and know what is 
going on there.


jD



--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc
***  A fault and talent of mine is to tell it exactly how it is.  ***
PostgreSQL centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://postgresconf.org
* Unless otherwise stated, opinions are my own.   *

Re: lazy detoasting

[Chapman Flack]

On 04/11/2018 11:33 AM, Tom Lane wrote:
> Chapman Flack  writes:
>> The mission is to implement java.sql.SQLXML, which has long been
>> missing from PL/Java.
>> This is the class of object your Java code can retrieve from a
>> ResultSet row from a query with an XML column type. (It's also
>> what can be passed to you as a function parameter, if your
>> function's SQL declaration has a parameter type XML.)
> 
> OK, but if this object only lives within a single function call,
> what's the problem?  The underlying row must be visible to the
> calling query, and that condition won't change for the duration
> of the call.

Well, the devilsAdvocate() function would stash the object
in a static, then try to look at it some time in a later call
in the same transaction.

Mind you, I have no use case in mind for a function wanting to do that
(other than as a test case for spec compliance). But the API spec for
the SQLXML class expressly says "object is valid for the duration
of the transaction", and I try not to renege on spec guarantees
if I can find a practical way not to.

> Things could get interesting if you consider a Java *procedure*,

and, yes, that. Though so far, there still are no PL/Java *procedures*.
I haven't even been following that development closely enough yet to
think about what a plan for supporting those would require.

-Chap

Re: 'make check' fails

[Bruce Momjian]

On Wed, Apr 11, 2018 at 01:04:56PM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > On Wed, Apr 11, 2018 at 12:35:41PM -0400, Tom Lane wrote:
> >> We can fix this by making submake-generated-headers be a recursive
> >> prerequisite for "check" as well as "all" and "install".  I wonder
> >> whether anybody is expecting any other shortcuts to work.
> 
> > In case it wasn't clear, I can reproduce this failure without parallel
> > mode.  FYI, the reason I used this shortcut is so I can just run one
> > command and check one error code to know if they all worked;  see
> > src/tools/pgtest for an example.
> 
> Probably, you got out of the habit of using parallel mode because it
> fell over from time to time.  The way things were done before, there
> independently trying to update the same generated headers.

I do use parallel make but was testing without it just in case.

> Will commit a fix in a bit.

I can confirm the fix.  Thanks.

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

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

Re: lazy detoasting

[Tom Lane]

Chapman Flack  writes:
> On 04/11/2018 11:33 AM, Tom Lane wrote:
>> OK, but if this object only lives within a single function call,
>> what's the problem?  The underlying row must be visible to the
>> calling query, and that condition won't change for the duration
>> of the call.

> Well, the devilsAdvocate() function would stash the object
> in a static, then try to look at it some time in a later call
> in the same transaction.

If you're worried about that, you should also worry about what happens
if the function uses the static variable in some later transaction.
The spec grants you license to throw an error, but it still needs to
be a clean error (not something about "can't find toast value", IMO).

Can you detect that the value is being stored in a long-lived variable
and detoast at that point?

regards, tom lane

Re: WARNING in parallel index creation.

[Tom Lane]

I wrote:
> The WARNING seems to indicate that the error check in set_config_option
> is too aggressive.  I kind of wonder why it was placed there at all,
> rather than in SQL-level operations like ExecSetVariableStmt.

BTW, looking back at the thread, nobody seems to have posted an analysis
of why this happens, but it's pretty simple.  During initial library load
of plperl.so, that module does DefineCustomStringVariable.  If there is
a pre-existing reset_val of the variable (taken from the postgresql.conf
file, in this example), then define_custom_variable uses set_config_option
to apply that value to the newly created variable struct.  So if plperl
library load happens during a parallel operation, the IsInParallelMode
check in set_config_option will bleat about it.

That test is, therefore, wrong.  Otherwise, no non-builtin function
could ever be marked parallel safe, for fear that the shlib it lives
in might try to set up a custom variable at load time.

I'm of the opinion that having such a test here at all is crummy design.
It implies that set_config_option is in charge of knowing about every
one of its callers and passing judgment on whether they represent
parallel-safe usages, which is the exact opposite of modularity,
even if set_config_option had enough information to make that judgment
which it does not.

In any case, the parallel index build patch is not at fault, it just
happens to be involved in this particular example.  I'd put the blame
on commit 924bcf4f1 which installed this test in the first place.

regards, tom lane

Re: lazy detoasting

[Chapman Flack]

On 04/11/2018 01:55 PM, Tom Lane wrote:
> Chapman Flack  writes:
>> Well, the devilsAdvocate() function would stash the object
>> in a static, then try to look at it some time in a later call
>> in the same transaction.
> 
> If you're worried about that, you should also worry about what happens
> if the function uses the static variable in some later transaction.
> The spec grants you license to throw an error, but it still needs to
> be a clean error (not something about "can't find toast value", IMO).

There's precedent for that kind of thing in PL/Java already ... objects
that Java considers alive as long as some code holds a reference
to them, but proxy for things in PG that may only have function-call
lifetime or cursor-row lifetime, etc. If they are closed by Java code
(or the Java GC finds them unreachable) first, they have to remember
to release their PG stuff; if the PG stuff goes first, they have to
update themselves to throw a suitable "you've kept me past my sell-by
date" exception if the Java code tries to use them again.

Thomas implemented most of those things ages ago; this is the first
I've added myself, with a little adjustment of technique because his
were for lifetimes shorter than transaction. I'm using the
TopTransactionResourceOwner to learn when the transaction is finished.
Resource owners have been around as long as any PG version PL/Java
supports, so that seems ok.

> Can you detect that the value is being stored in a long-lived variable
> and detoast at that point?

Not easily, I don't think. The question resembles "is this object
still reachable, or unreachable, now as I exit this function call?"
or at some other specific time. The Java garbage collector eventually
learns what's become unreachable, but it doesn't promise *when*.

But let me return to the earlier idea for a moment: are you saying
that it might *not* be sufficient to find an applicable snapshot at
the time of constructing the object, and register that snapshot
on TopTransactionResourceOwner?

It would obviously not be kept around any longer than the transaction,
and would be released earlier whenever the Java code reads/closes it,
or lets go of the last reference and GC finds it. In all the typical
cases I can imagine, it would be registered very briefly, with the
object being constructed/read/freed in quick succession.

-Chap

Re: Bugs in TOAST handling, OID assignment and redo recovery

[Pavan Deolasee]

On Wed, Apr 11, 2018 at 8:20 PM, Tom Lane  wrote:

> Pavan Deolasee  writes:
> > Or may be we simply err on the side of caution and scan the toast table
> > with SnapshotAny while looking for a duplicate? That might prevent us
> from
> > reusing an OID for a known-dead tuple, but should save us a second index
> > scan and still work.
>
> +1.  We really don't want to expend two indexscans on this.
>
>
Ok. I propose attached patches, more polished this time. I also
changed toastrel_valueid_exists() to use SnapshotAny, but I don't have any
proofs to claim that's a needed change. But it seems harmless and given the
number of toast related, hard to chase bugs we have seen over the years,
may it's worth making it full proof too.

Thanks,
Pavan

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


0002-Do-more-extensive-search-while-looking-for-duplicate.patch
Description: Binary data


0001-Do-not-overwrite-the-nextOid-counter-while-replaying.patch
Description: Binary data

Re: Commit 4dba331cb3 broke ATTACH PARTITION behaviour.

[Alvaro Herrera]

Thanks for the discussion.  Per your suggestions, I changed the check
for default partition OID to an assert instead of the 'if' condition,
and removed the code that attempted vainly to verify the constraint when
attaching a foreign table as a partition.  And pushed.

I think we're done here, so marked the Open Item as fixed.

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

Re: using index or check in ALTER TABLE SET NOT NULL

[Sergei Kornilov]

Hi
I noticed new merge conflict, updated version attached.

regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index bd22627..db98a98 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -215,8 +215,15 @@ WITH ( MODULUS numeric_literal, REM
 
  
   These forms change whether a column is marked to allow null
-  values or to reject null values.  You can only use SET
-  NOT NULL when the column contains no null values.
+  values or to reject null values.
+ 
+
+ 
+  You can only use SET NOT NULL when the column 
+  contains no null values. Full table scan is performed to check 
+  this condition unless there are valid CHECK 
+  constraints that prohibit NULL values for specified column.
+  In the latter case table scan is skipped.
  
 
  
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 0f5932f..6fd3663 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1153,7 +1153,7 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 	 * not contain any row that would belong to the new partition, we can
 	 * avoid scanning the default partition.
 	 */
-	if (PartConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
+	if (ConstraintImpliedByRelConstraint(default_rel, def_part_constraints))
 	{
 		ereport(INFO,
 (errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
@@ -1197,8 +1197,8 @@ check_default_allows_bound(Relation parent, Relation default_rel,
 			 * that it will not contain any row that would belong to the new
 			 * partition, we can avoid scanning the child table.
 			 */
-			if (PartConstraintImpliedByRelConstraint(part_rel,
-	 def_part_constraints))
+			if (ConstraintImpliedByRelConstraint(part_rel,
+ def_part_constraints))
 			{
 ereport(INFO,
 		(errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints",
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f810885..e10edb1 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -160,7 +160,7 @@ typedef struct AlteredTableInfo
 	/* Information saved by Phases 1/2 for Phase 3: */
 	List	   *constraints;	/* List of NewConstraint */
 	List	   *newvals;		/* List of NewColumnValue */
-	bool		new_notnull;	/* T if we added new NOT NULL constraints */
+	bool		verify_new_notnull; /* T if we should recheck NOT NULL */
 	int			rewrite;		/* Reason for forced rewrite, if any */
 	Oid			newTableSpace;	/* new tablespace; 0 means no change */
 	bool		chgPersistence; /* T if SET LOGGED/UNLOGGED is used */
@@ -372,6 +372,7 @@ static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMO
 static void ATPrepSetNotNull(Relation rel, bool recurse, bool recursing);
 static ObjectAddress ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
  const char *colName, LOCKMODE lockmode);
+static bool NotNullImpliedByRelConstraints(Relation rel, Form_pg_attribute attr);
 static ObjectAddress ATExecColumnDefault(Relation rel, const char *colName,
 	Node *newDefault, LOCKMODE lockmode);
 static ObjectAddress ATExecAddIdentity(Relation rel, const char *colName,
@@ -4461,7 +4462,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode)
 			 * Test the current data within the table against new constraints
 			 * generated by ALTER TABLE commands, but don't rebuild data.
 			 */
-			if (tab->constraints != NIL || tab->new_notnull ||
+			if (tab->constraints != NIL || tab->verify_new_notnull ||
 tab->partition_constraint != NULL)
 ATRewriteTable(tab, InvalidOid, lockmode);
 
@@ -4622,7 +4623,7 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap, LOCKMODE lockmode)
 	}
 
 	notnull_attrs = NIL;
-	if (newrel || tab->new_notnull)
+	if (newrel || tab->verify_new_notnull)
 	{
 		/*
 		 * If we are rebuilding the tuples OR if we added any new NOT NULL
@@ -5657,7 +5658,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
 			 * an OID column.  OID will be marked not null, but since it's
 			 * filled specially, there's no need to test anything.)
 			 */
-			tab->new_notnull |= colDef->is_not_null;
+			tab->verify_new_notnull |= colDef->is_not_null;
 		}
 	}
 
@@ -6061,8 +6062,17 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
 
 		CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple);
 
-		/* Tell Phase 3 it needs to test the constraint */
-		tab->new_notnull = true;
+		/*
+		 * Verifying table data can be done in Phase 3 with single table scan
+		 * for all constraint (both existing and new)
+		 * We can skip this check only if every new NOT NULL contraint
+		 * implied by existed table constraints
+		 */
+		if (!NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) GETSTRUCT(tupl

Re: Bugs in TOAST handling, OID assignment and redo recovery

[Tom Lane]

Pavan Deolasee  writes:
> Ok. I propose attached patches, more polished this time.

I'll take these, unless some other committer is hot to do so?

regards, tom lane

Re: lazy detoasting

[Tom Lane]

Chapman Flack  writes:
> But let me return to the earlier idea for a moment: are you saying
> that it might *not* be sufficient to find an applicable snapshot at
> the time of constructing the object, and register that snapshot
> on TopTransactionResourceOwner?

The problem is to know which snapshot is applicable; if the transaction
has more than one, you don't know which was used to read the row of
interest.  I suppose you could be conservative and use the oldest one,
if snapmgr lets you find that.

regards, tom lane

Re: lazy detoasting

[Chapman Flack]

On 04/11/2018 03:04 PM, Tom Lane wrote:
> Chapman Flack  writes:
>> that it might *not* be sufficient to find an applicable snapshot at
>> the time of constructing the object, and register that snapshot
>> on TopTransactionResourceOwner?
> 
> The problem is to know which snapshot is applicable; if the transaction
> has more than one, you don't know which was used to read the row of
> interest.  I suppose you could be conservative and use the oldest one,
> if snapmgr lets you find that.

There does seem to be GetOldestSnapshot(), returning
older( oldest on active stack, oldest on registered heap ).

And it seems to be the very thing called by tuptoaster itself,
right after the comment "since we don't know which one to use,
just use the oldest one".

-Chap

Re: Partitioned tables and covering indexes

[Teodor Sigaev]

Actually, discovered bug is not related to patch except new test faces 
with it,
problem is: CompareIndexInfo() checks rd_opfamily for equality for all 
attributes, not only for key attribute.


Patch attached. But it seems to me, field's names of
IndexInfo structure are a bit confused now:
int ii_NumIndexAttrs;   /* total number of columns in index */
int ii_NumIndexKeyAttrs;/* number of key columns in 
index */

AttrNumber  ii_KeyAttrNumbers[INDEX_MAX_KEYS];


ii_KeyAttrNumbers contains all columns, i.e. it contains 
ii_NumIndexAttrs number of columns, not a ii_NumIndexKeyAttrs number as 
easy to think.


I suggest rename ii_KeyAttrNumbers to ii_AttrNumbers or 
ii_IndexAttrNumbers. Opinions?




     for (i = 0; i < info1->ii_NumIndexAttrs; i++)
     {
     if (maplen < info2->ii_KeyAttrNumbers[i])




--
Teodor Sigaev  E-mail: teo...@sigaev.ru
  WWW: http://www.sigaev.ru/
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 5d73e92901..0002816fcc 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1831,6 +1831,10 @@ CompareIndexInfo(IndexInfo *info1, IndexInfo *info2,
 	if (info1->ii_NumIndexAttrs != info2->ii_NumIndexAttrs)
 		return false;
 
+	/* and same number of key attributes */
+	if (info1->ii_NumIndexKeyAttrs != info2->ii_NumIndexKeyAttrs)
+		return false;
+
 	/*
 	 * and columns match through the attribute map (actual attribute numbers
 	 * might differ!)  Note that this implies that index columns that are
@@ -1850,7 +1854,9 @@ CompareIndexInfo(IndexInfo *info1, IndexInfo *info2,
 
 		if (collations1[i] != collations2[i])
 			return false;
-		if (opfamilies1[i] != opfamilies2[i])
+
+		/* opfamily is valid on for key attributes */
+		if (i < info2->ii_NumIndexKeyAttrs && opfamilies1[i] != opfamilies2[i])
 			return false;
 	}
 

Re: Partitioned tables and covering indexes

[Alvaro Herrera]

Teodor Sigaev wrote:

> Patch attached.

I wonder why this is a problem in opfamilies but not collations.
If we don't compare collations, wouldn't it make more sense to break out
of the loop once the number of keys is reached?

When this code was written, there was no question as to what length the
opfamilies/collations the arrays were; it was obvious that they must be
of the length of the index attributes.  It's not obvious now.  Maybe add
a comment about that?

> But it seems to me, field's names of
> IndexInfo structure are a bit confused now:
> int ii_NumIndexAttrs;   /* total number of columns in index */
> int ii_NumIndexKeyAttrs;/* number of key columns in index */
> AttrNumber  ii_KeyAttrNumbers[INDEX_MAX_KEYS];
> 
> ii_KeyAttrNumbers contains all columns, i.e. it contains ii_NumIndexAttrs
> number of columns, not a ii_NumIndexKeyAttrs number as easy to think.
> 
> I suggest rename ii_KeyAttrNumbers to ii_AttrNumbers or ii_IndexAttrNumbers.
> Opinions?

Yeah, the current situation seems very odd.

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

Re: lazy detoasting

[Andrew Gierth]

> "Chapman" == Chapman Flack  writes:

 Chapman> There's precedent for that kind of thing in PL/Java already
 Chapman> ... objects that Java considers alive as long as some code
 Chapman> holds a reference to them, but proxy for things in PG that may
 Chapman> only have function-call lifetime or cursor-row lifetime, etc.
 Chapman> If they are closed by Java code (or the Java GC finds them
 Chapman> unreachable) first, they have to remember to release their PG
 Chapman> stuff; if the PG stuff goes first, they have to update
 Chapman> themselves to throw a suitable "you've kept me past my sell-by
 Chapman> date" exception if the Java code tries to use them again.

How's the code doing this? I couldn't find it in a cursory scan.

-- 
Andrew (irc:RhodiumToad)

Re: pgsql: New files for MERGE

[Simon Riggs]

On 7 April 2018 at 18:45, Tom Lane  wrote:
> Simon Riggs  writes:
>> On 6 April 2018 at 17:22, Bruce Momjian  wrote:
>>> My point was that people didn't ask you to work harder on fixing the
>>> patch, but in reverting it.  You can work harder on fixing things in the
>>> hope they change their minds, but again, that isn't addressing their
>>> request.
>
>> If Tom or Andres still feel that their concerns have not been
>> addressed over the last few days, I am happy to revert the patch with
>> no further discussion from me in this cycle.
>
> FWIW, I still vote to revert.  Even if the patch were now perfect,
> there is not time for people to satisfy themselves of that, and
> we've got lots of other things on our plates.
>
> I'd be glad to participate in a proper review of this when v12
> opens.  But right now it just seems too rushed, and I have little
> confidence in it being right.
>
> regards, tom lane
>
> PS: If you do revert, please wrap it up as a single revert commit,
> not a series of half a dozen.  You've already put several
> non-buildable states into the commit history as a result of this
> patch, each one of which is a land mine for git bisect testing.
> We don't need more of those.  Also, looking at the reverse of the
> reversion commit will provide a handy way of seeing the starting
> point for future discussion of this patch.

Will do.



"Commence primary ignition."

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Bugs in TOAST handling, OID assignment and redo recovery

[Simon Riggs]

On 11 April 2018 at 19:57, Tom Lane  wrote:
> Pavan Deolasee  writes:
>> Ok. I propose attached patches, more polished this time.
>
> I'll take these, unless some other committer is hot to do so?

Please go ahead.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Gotchas about pg_verify_checksums

[Daniel Gustafsson]

> On 11 Apr 2018, at 01:53, Michael Paquier  wrote:
> 
> On Tue, Apr 10, 2018 at 10:27:19PM +0200, Daniel Gustafsson wrote:
>>> On 10 Apr 2018, at 06:21, Michael Paquier  wrote:
>> Does it really imply that?  Either way, the tool could potentially be useful
>> for debugging a broken cluster so I’m not sure that stating it requires a
>> cleanly shut down server is useful.
> 
> Torn pages could lead to false positives.  So I think that the tool's
> assumptions are right.

Right, I misunderstood your initial email but I see what you mean.  Yes, you
are right and with that +1 on your patch.

> I am wondering as well if we should not actually rename the tool?  Why
> not naming it pg_checksums instead of pg_verify_checksums, and add an
> --action switch to it which can be used to work on checksums.  The
> obvious option to support in v11 is a "verify" mode, but I would imagine
> that a "disable" and "enable" modes would be useful as well, and all the
> APIs are here already to be able to do an in-place update of the
> checksums, and then switch the control file properly.  We have no idea
> at this stage if a patch to enable checksums while the cluster is online
> will be able to make it, still a way to switch checksums while the
> cluster is offline is both reliable and easy to implement.  Not saying
> do to that for v11 of course, I would like to keep the door open for
> v12.

Naming it pg_checksums, with only verification as an option, seems to me to
imply future direction for 12 more than what pg_verify_checksums does.  I would
leave it the way it is, but I don’t have very strong opinions (or any plans on
hacking on offline checksum enabling for that matter).

cheers ./daniel

Re: Problem while setting the fpw with SIGHUP

[Michael Paquier]

On Wed, Apr 11, 2018 at 02:09:48PM +0300, Heikki Linnakangas wrote:
> I think the new behavior where the GUC only takes effect at next checkpoint
> is OK. It seems quite intuitive.
> 
> > [rebased patch version]
> 
> Looks good at a quick glance. Assuming no objections from others, I'll take
> a closer look and commit tomorrow. Thanks!

Sorry for not following up closely this thread lately.

+   /*
+* If full_page_writes has been turned off, issue XLOG_FPW_CHANGE before
+* the flag actually takes effect. No lock is required since checkpointer
+* is the only updator of shared fullPageWrites after recovery is
+* finished. Both shared and local fullPageWrites do not change before the
+* next reading below.
+*/
+   if (Insert->fullPageWrites && !fullPageWrites)
+   {
+   XLogBeginInsert();
+   XLogRegisterData((char *) (&fullPageWrites), sizeof(bool));
+   XLogInsert(RM_XLOG_ID, XLOG_FPW_CHANGE);
+   }

This is not actually true.  If a fallback_promote is used, then
CreateCheckPoint() is called by the startup process which is in charge
of issuing the end-of-recovery checkpoint, and not the checkpointer.  So
I still fail to see how a no-lock approach is fine except if we remove
fallback_promote?
--
Michael


signature.asc
Description: PGP signature

Re: Bugs in TOAST handling, OID assignment and redo recovery

[Tom Lane]

So while looking at this, it suddenly occurred to me that probing with
SnapshotDirty isn't that safe for regular (non-TOAST) Oid assignment
either.  SnapshotDirty will consider a row dead the instant the
deleting transaction has committed, but it may remain visible to other
transactions for awhile after that --- and now that we use MVCC for
catalog scans, that applies to them too.  Hence, the existing logic
is capable of creating objects with transiently-conflicting OIDs.
I don't think this could create a conflict that's visible outside
our own transaction, since anyone who can see our commit would also
see the commit of the deleting transaction.  But there's definitely
a hazard inside the transaction that creates a new object.

I propose therefore that the right fix does not require an API change
for GetNewOidWithIndex: we can just make it use SnapshotAny all the
time.

regards, tom lane

Re: relispartition for index partitions

[Alvaro Herrera]

Amit Langote wrote:
> Hi.
> 
> I noticed that relispartition isn't set for index's partitions.

This patch should fix it.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 5d73e92901..218c457fa4 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -933,6 +933,7 @@ index_create(Relation heapRelation,
indexRelation->rd_rel->relowner = heapRelation->rd_rel->relowner;
indexRelation->rd_rel->relam = accessMethodObjectId;
indexRelation->rd_rel->relhasoids = false;
+   indexRelation->rd_rel->relispartition = OidIsValid(parentIndexRelid);
 
/*
 * store index's pg_class entry
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f8108858ae..56e87d6251 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -490,6 +490,8 @@ static ObjectAddress ATExecAttachPartitionIdx(List 
**wqueue, Relation rel,
 static void validatePartitionedIndex(Relation partedIdx, Relation partedTbl);
 static void refuseDupeIndexAttach(Relation parentIdx, Relation partIdx,
  Relation partitionTbl);
+static void update_relispartition(Relation classRel, Oid partIdxId,
+ bool newval);
 
 
 /* 
@@ -14405,10 +14407,11 @@ AttachPartitionEnsureIndexes(Relation rel, Relation 
attachrel)
 */
for (i = 0; i < list_length(attachRelIdxs); i++)
{
+   Oid cldIdxId = 
RelationGetRelid(attachrelIdxRels[i]);
Oid cldConstrOid = InvalidOid;
 
/* does this index have a parent?  if so, can't use it 
*/
-   if 
(has_superclass(RelationGetRelid(attachrelIdxRels[i])))
+   if (attachrelIdxRels[i]->rd_rel->relispartition)
continue;
 
if (CompareIndexInfo(attachInfos[i], info,
@@ -14429,7 +14432,7 @@ AttachPartitionEnsureIndexes(Relation rel, Relation 
attachrel)
{
cldConstrOid =

get_relation_idx_constraint_oid(RelationGetRelid(attachrel),
-   
RelationGetRelid(attachrelIdxRels[i]));
+   
cldIdxId);
/* no dice */
if (!OidIsValid(cldConstrOid))
continue;
@@ -14439,6 +14442,7 @@ AttachPartitionEnsureIndexes(Relation rel, Relation 
attachrel)
IndexSetParentIndex(attachrelIdxRels[i], idx);
if (OidIsValid(constraintOid))

ConstraintSetParentConstraint(cldConstrOid, constraintOid);
+   update_relispartition(NULL, cldIdxId, true);
found = true;
break;
}
@@ -14659,7 +14663,6 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
((Form_pg_class) GETSTRUCT(newtuple))->relispartition = false;
CatalogTupleUpdate(classRel, &newtuple->t_self, newtuple);
heap_freetuple(newtuple);
-   heap_close(classRel, RowExclusiveLock);
 
if (OidIsValid(defaultPartOid))
{
@@ -14692,8 +14695,10 @@ ATExecDetachPartition(Relation rel, RangeVar *name)
 
idx = index_open(idxid, AccessExclusiveLock);
IndexSetParentIndex(idx, InvalidOid);
+   update_relispartition(classRel, idxid, false);
relation_close(idx, AccessExclusiveLock);
}
+   heap_close(classRel, RowExclusiveLock);
 
/*
 * Invalidate the parent's relcache so that the partition is no longer
@@ -14772,6 +14777,39 @@ RangeVarCallbackForAttachIndex(const RangeVar *rv, Oid 
relOid, Oid oldRelOid,
 }
 
 /*
+ * Update the relispartition flag of the relation with the given OID, to the
+ * given value.
+ *
+ * classRel is the pg_class relation, already open and suitably locked; if
+ * passed as NULL, we open it internally and close before returning.
+ */
+static void
+update_relispartition(Relation classRel, Oid partIdxId, bool newval)
+{
+   HeapTuple   tup;
+   HeapTuple   newtup;
+   Form_pg_class classForm;
+   boolopened = false;
+
+   if (classRel == NULL)
+   {
+   classRel = heap_open(RelationRelationId, RowExclusiveLock);
+  

Re: Gotchas about pg_verify_checksums

[Michael Paquier]

On Wed, Apr 11, 2018 at 10:21:29PM +0200, Daniel Gustafsson wrote:
> Right, I misunderstood your initial email but I see what you mean.  Yes, you
> are right and with that +1 on your patch.

OK, no problem.

> Naming it pg_checksums, with only verification as an option, seems to me to
> imply future direction for 12 more than what pg_verify_checksums does.  I 
> would
> leave it the way it is, but I don’t have very strong opinions (or any plans on
> hacking on offline checksum enabling for that matter).

Okay, I am fine to let such decision to you and Magnus at the end as the
authors and committers of the thing.  I think that I will just hack out
this tool myself after reusing this code if you don't mind of course..
--
Michael


signature.asc
Description: PGP signature

Re: Native partitioning tablespace inheritance

[Michael Paquier]

On Wed, Apr 11, 2018 at 12:52:06PM -0400, Keith Fiske wrote:
> Any chance of this being an inheritable property that can simply be
> overridden if the TABLESPACE flag is set when creating a child table? If
> it's not set, just set the tablespace to whatever was set for the parent.

I am wondering how you would actually design that without some kind of
unintuitive behavior for the end user as for some applications a set of
child partitions sometimes take advantage of the fact that they are on
separate tablespaces.  Hence why not relying on default_tablespace
instead when creating the partition set, or use a function wrapper which
enforces the tablespace when the partition is created?
--
Michael


signature.asc
Description: PGP signature

Re: es_query_dsa is broken

[Thomas Munro]

On Thu, Apr 12, 2018 at 4:04 AM, Andres Freund  wrote:
> This is an open item for v11:
>
>  Tidy up es_query_dsa and possibly ParallelWorkerContext?
> Original commit: e13029a5ce353574516c64fd1ec9c50201e705fd (principal 
> author: Thomas Munro; owner: Robert Haas)
> Bug fix: fd7c0fa732d97a4b4ebb58730e6244ea30d0a618
> While the bug was fixed with something back-patchable, we should 
> considering improving this situation. As discussed in the above-linked 
> thread, options might include (1) getting rid of es_query_dsa entirely and 
> injecting dependencies into nodes, (2) making all Gather nodes share the same 
> DSM segment so there truly could be per-query DSA segment.
>
> Do we want to make any changes here for v11? If not, are we ok with just
> closing the entry and waiting till it bugs anybody for some reason?

I think we should probably do both of the things listed above, but
given where we are and given that it's refactoring work and not a
stop-ship issue, I propose to do that in the v12 cycle.  I'll go
remove that from the open items if no one objects soon.

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

Re: Partitioned tables and covering indexes

[Alexander Korotkov]

On Wed, Apr 11, 2018 at 10:47 PM, Alvaro Herrera 
wrote:

> Teodor Sigaev wrote:
>
> > Patch attached.
>
> I wonder why this is a problem in opfamilies but not collations.
> If we don't compare collations, wouldn't it make more sense to break out
> of the loop once the number of keys is reached?
>

It appears that INCLUDE columns might have collation defined.
For instance, following query is working:

create index t_s1_s2_idx on t (s1) include (s2 collate "en_US.UTF-8");

However, I don't see any point in defining collations here, because
INCLUDE attributes exist solely for index-only scans.  So, index just
can return value of INCLUDE attribute "as is", no point to do something
with collation.

So, I propose to disable collations for INCLUDE attributes.

When this code was written, there was no question as to what length the
> opfamilies/collations the arrays were; it was obvious that they must be
> of the length of the index attributes.  It's not obvious now.  Maybe add
> a comment about that?
>

In b3b7f789 Tom have resized one array size from total number of
attributes to number of key attributes.  And I like idea of applying the
same to all other array: make them sized to total number of attributes
with filling of absent attributes with 0.


> > But it seems to me, field's names of
> > IndexInfo structure are a bit confused now:
> > int ii_NumIndexAttrs;   /* total number of columns in index
> */
> > int ii_NumIndexKeyAttrs;/* number of key columns in
> index */
> > AttrNumber  ii_KeyAttrNumbers[INDEX_MAX_KEYS];
> >
> > ii_KeyAttrNumbers contains all columns, i.e. it contains ii_NumIndexAttrs
> > number of columns, not a ii_NumIndexKeyAttrs number as easy to think.
> >
> > I suggest rename ii_KeyAttrNumbers to ii_AttrNumbers or
> ii_IndexAttrNumbers.
> > Opinions?
>
> Yeah, the current situation seems very odd.
>

+1 for ii_IndexAttrNumbers.

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

Re: Creation of wiki page for open items of v11

[Peter Eisentraut]

On 4/11/18 10:53, Tom Lane wrote:
> It's not that much work to move the items rather than remove them,

Well, toward the end of the cycle, when the list of closed items is
quite long, then it does become a bit of a burden to carefully cut and
paste the item in the little browser window without making hash of the
wiki page.

It's not a very large deal, but I doubt the result is very useful.  The
current "resolved before 11beta1" section is pretty much useless.

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

Re: Fix for pg_stat_activity putting client hostaddr into appname field

[Heikki Linnakangas]

On 29/03/18 10:46, Michael Paquier wrote:

On Tue, Mar 27, 2018 at 03:47:07PM +1300, Edmund Horner wrote:

I considered whether aux processes really need those strings
(especially st_clienthostname), but decided it was more consistent
just to assume that they might.  (It's an extra 3 kB... if we want to
save that we can put a bunch of if statements in pgstat_bestart and
other places.)


BackendStatusShmemSize has been originally changed to use
NumBackendStatSlots, so the intention is visibly to be consistent in the
number of backends used, even if this means consuming a bit more memory,
so the idea was to focus on consistency and simplicity.


Yep, agreed.

I've committed this, and backpatched to v10.


The patch also allocates local memory for st_clienthostname in
pgstat_read_current_status.  These strings shouldn't change very
often, but it seems safer and more consistent to treat them as we
treat the other two string fields.  Without the string copy, I think a
client disconnect and be replaced after the stats have been copied,
resulting in the new hostname appearing alongside the copied stats
fields from the old connection.


Agreed on that as well.


Yeah, that's also a bug. I pushed your fix for that as a separate 
commit, and backpatched to all supported versions.


Thanks for the debugging and the patch, Edmund!

- Heikki

Re: Creation of wiki page for open items of v11

[Tom Lane]

Peter Eisentraut  writes:
> On 4/11/18 10:53, Tom Lane wrote:
>> It's not that much work to move the items rather than remove them,

> Well, toward the end of the cycle, when the list of closed items is
> quite long, then it does become a bit of a burden to carefully cut and
> paste the item in the little browser window without making hash of the
> wiki page.

> It's not a very large deal, but I doubt the result is very useful.  The
> current "resolved before 11beta1" section is pretty much useless.

Hm.  There's definitely an argument to be made that it's not worth
tracking resolved items till after beta1.  Once betas exist, the list
becomes useful to beta testers who may not be tracking events as closely
as hackers do.

regards, tom lane

Re: Partitioned tables and covering indexes

[Peter Geoghegan]

On Wed, Apr 11, 2018 at 1:58 PM, Alexander Korotkov
 wrote:
> It appears that INCLUDE columns might have collation defined.
> For instance, following query is working:
>
> create index t_s1_s2_idx on t (s1) include (s2 collate "en_US.UTF-8");
>
> However, I don't see any point in defining collations here, because
> INCLUDE attributes exist solely for index-only scans.  So, index just
> can return value of INCLUDE attribute "as is", no point to do something
> with collation.
>
> So, I propose to disable collations for INCLUDE attributes.

Hmm. I'm not sure that that's exactly the right thing to do. We seem
to want to have case-insensitive collations in the future. The fact
that you can spell out collation name in ON CONFLICT's unique index
inference specification suggests this, for example. I think that a
collation is theoretically allowed to affect the behavior of equality,
even though so far we've never tried to make that work for any
collatable datatype.

Maybe the right thing to do is to say that any collation will work
equally well (as will any opclass). Maybe that's the same thing as
what you said, though.

> +1 for ii_IndexAttrNumbers.

+1

-- 
Peter Geoghegan

Re: Native partitioning tablespace inheritance

[Keith Fiske]

On Wed, Apr 11, 2018 at 4:54 PM, Michael Paquier 
wrote:

> On Wed, Apr 11, 2018 at 12:52:06PM -0400, Keith Fiske wrote:
> > Any chance of this being an inheritable property that can simply be
> > overridden if the TABLESPACE flag is set when creating a child table? If
> > it's not set, just set the tablespace to whatever was set for the parent.
>
> I am wondering how you would actually design that without some kind of
> unintuitive behavior for the end user as for some applications a set of
> child partitions sometimes take advantage of the fact that they are on
> separate tablespaces.  Hence why not relying on default_tablespace
> instead when creating the partition set, or use a function wrapper which
> enforces the tablespace when the partition is created?
> --
> Michael
>


To me the current behavior is even more unintuitive. You tell it to put the
parent table in a specific tablespace and it completely ignores the option
and puts it in the default. Then when you go create children without
specifying a tablespace, you don't see it going where you thought it would
based on the parent's creation statement. Yes, you can tell each child
where to go, but why not have at least a basic mechanism for setting a
single tablespace value for a partition set into the parent itself the same
way we're doing with indexes?

If you set the tablespace you want a partition set to be in by setting that
on that parent, I think that's pretty intuitive. If you want children to be
in a different tablespace than the partition's default, then you can tell
it that at child creation.

Having to rely on custom written function to enforce just basic tablespace
rules seems to be overcomplicating it to me.

-- 
Keith Fiske
Senior Database Engineer
Crunchy Data - http://crunchydata.com

Re: missing support of named convention for procedures

[Peter Eisentraut]

On 4/11/18 12:06, Andres Freund wrote:
> On 2018-03-22 15:19:12 +0100, Pavel Stehule wrote:
>> attached patch should to fix it
> 
> This is still broken, and has been an open item for a bit. Peter, Could
> you check whether Pavel's fix resolves the issue for you?

Yes, I will work on this.

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

Re: Partitioned tables and covering indexes

[Peter Eisentraut]

On 4/11/18 17:08, Peter Geoghegan wrote:
>> However, I don't see any point in defining collations here, because
>> INCLUDE attributes exist solely for index-only scans.  So, index just
>> can return value of INCLUDE attribute "as is", no point to do something
>> with collation.
>>
>> So, I propose to disable collations for INCLUDE attributes.
> Hmm. I'm not sure that that's exactly the right thing to do. We seem
> to want to have case-insensitive collations in the future. The fact
> that you can spell out collation name in ON CONFLICT's unique index
> inference specification suggests this, for example. I think that a
> collation is theoretically allowed to affect the behavior of equality,
> even though so far we've never tried to make that work for any
> collatable datatype.

But in this case it doesn't even do equality comparison, it just returns
the value.

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