Re: [HACKERS] plpgsql - DECLARE - cannot to use %TYPE or %ROWTYPE for composite types

[Tom Lane]

I wrote:
> However ... one thing I was intending to mention on this thread is that
> "get the array type over this type" isn't the only extension one might
> wish for.  Another likely desire is "get the type of field 'foo' of this
> composite type".  I don't suggest that this patch needs to implement
> that right now; but it would be a good thing if we could see how the
> chosen syntax could be extended in such a direction.  Otherwise we might
> be painting ourselves into a corner.

To enlarge a little bit: it seems to me that what we're really wishing for
here is a type name syntax that goes beyond simple names.  If we were
starting in a green field, we might choose a recursively-composable syntax
like the following.

type_name can be:

* A simple type name, such as int8 or varchar[42].

* TYPE_OF(expression), meaning that the SQL expression is parsed but never
executed, we just take this construct as naming its result type.

* ARRAY_OF(type_name), meaning the array type having type_name as its
element type.

* ELEMENT_OF(type_name), meaning the element type of the array type
named by type_name.

* ROW_OF(type_name [, type_name ...]), meaning the composite type with
those types as columns.

* FIELD_OF(type_name, foo), meaning the type of column "foo" of the
composite type named by type_name.  I'm not sure if there would be
use-cases for selecting a column other than by a simple literal name,
but there could be variants of this function if so.

It's possible to think of other cases, for example what about range
types?  You could allow ELEMENT_OF() to apply to range types, certainly.
I'm not sure about the other direction, because multiple range types
could have the same element type; but it's possible to hope that some
type-naming function along the lines of RANGE_OF(type_name, other args)
could disambiguate.  The main reason I'm thinking of a function-like
syntax here is that it can easily handle additional arguments when
needed.

Comparing this flight of fancy to where we are today, we have
%TYPE as a remarkably ugly and limited implementation of TYPE_OF(),
and we have the precedent that foo[] means ARRAY_OF(foo).  I'm not
sure how we get any extensibility out of either of those things.

Or in short: maybe it's time to blow up %TYPE and start fresh.

regards, tom lane


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

Re: [HACKERS] pg_dump dump catalog ACLs

[Stephen Frost]

Tom, all,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Joe Conway  writes:
> > Would it be a terrible idea to add some attribute to ACLs which can be
> > used to indicate they should not be dumped (and supporting syntax)?
> 
> Yes, we'd need some way to mark non-null ACLs as being "built-in
> defaults".  I do not see the need to have SQL syntax supporting that
> though.

The attached patch does this by adding a 'pg_init_privs' catalog and
populating that catalog at the end of initdb, after all of the initial
privileges have been set up.

> Actually, wouldn't you need to mark individual aclitems as built-in
> or not?  Consider a situation where we have some function foo() that
> by default has EXECUTE permission granted to some built-in "pg_admin"
> role.  If a given installation then also grants EXECUTE to "joe",
> what you really want to have happen is for pg_dump to dump only the
> grant to "joe".  Mentioning pg_admin's grant would tie the dump to
> a particular major PG version's idea of what the built-in roles are,
> which is what I'm arguing we need to avoid.

What I was thinking about for this was to have pg_dump simply not dump
out any GRANTs made to default roles (those starting with 'pg_'), as
part of the default roles patch.

Another option would be to have pg_dump figure out the delta between
what the initial privileges were, as recorded in pg_init_privs, as what
the current rights are.

I was thinking that the former was simpler, but I don't think it'd be
too difficult to do the latter if the consensus is that it's better to
do so.

> I guess this could also be addressed by having two separate aclitem[]
> columns, one that is expected to be frozen after initdb and one for
> user-added grants.

This is along the lines of what I've done, but I've used a new catalog
instead, which is quite small and doesn't complicate or change the
regular catalogs.

* Joe Conway (m...@joeconway.com) wrote:
> On 03/02/2016 12:54 PM, Stephen Frost wrote:
> > * Joe Conway (m...@joeconway.com) wrote:
> >> On 03/01/2016 08:00 AM, Tom Lane wrote:
> >>> Yes, we'd need some way to mark non-null ACLs as being "built-in
> >>> defaults".  I do not see the need to have SQL syntax supporting that
> >>> though.
> >>
> >> I was thinking the supporting syntax might be used by extensions, for
> >> example.
> > 
> > I tend to agree with Tom that we don't really need SQL syntax for this.
> 
> > I don't see any reason it couldn't be used by extensions also, though
> > we'd have to do a bit more work on pg_dump to make it actually dump
> > out any non-default ACLs for extension-owned objects.
> 
> Without any syntax, what does the extension do, directly insert into
> this special catalog table?

I've not included it in this patch, but my thinking here would be to add
a check to the GRANT functions which checks 'if (creating_extension)'
and records/updates the entries in pg_init_privs for those objects.
This is similar to how we handle dependencies for extensions currently.
That way, extensions don't have to do anything and if the user changes
the permissions on an extension's object, the permissions for that
object would then be dumped out.

This still requires more testing, documentation, and I'll see about
making it work completely for extensions also, but wanted to provide an
update and solicit for review/comments.

The patches have been broken up in what I hope is a logical way for
those who wish to review/comment:

#1 - Add the pg_init_privs catalog
#2 - Change pg_dump to use a bitmap instead of boolean for 'dump'
#3 - Split 'dump' into 'dump' and 'dump_contains'
#4 - Make pg_dump include changed ACLs in dump of 'pg_catalog'
#5 - Remove 'if (!superuser()) ereport()' calls and REVOKE EXECUTE

Thanks!

Stephen
From 31f4fafeaa06ad7c72ee7c6699253a65ea542d11 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 10 Mar 2016 20:56:09 -0500
Subject: [PATCH 1/5] Add new catalog called pg_init_privs

This new catalog will hold the privileges which the system is
initialized with at initdb time.  This will allow pg_dump
(and any other similar use-cases) to detect when the privileges
set on objects in pg_catalog have been changed and handle those
changes appropriately.

This could be extended to work for extensions also, but that
seems like an independent enough change that it should be in a
different commit.
---
 src/backend/catalog/Makefile   |   2 +-
 src/bin/initdb/initdb.c| 121 +
 src/include/catalog/indexing.h |   3 +
 src/include/catalog/pg_init_privs.h|  83 
 src/test/regress/expected/sanity_check.out |   1 +
 5 files changed, 209 insertions(+), 1 deletion(-)
 create mode 100644 src/include/catalog/pg_init_privs.h

diff --git a/src/backend/catalog/Makefile b/src/backend/catalog/Makefile
index 25130ec..1ce7610 100644
--- a/src/backend/catalog/Makefile
+++ b/src/backend/catalog/Makefile
@@ -40,7 

[HACKERS] Re: [PATCH] Integer overflow in timestamp[tz]_part() and date/time boundaries check

[Anastasia Lubennikova]

14.03.2016 16:23, David Steele:

On 2/25/16 4:44 PM, Vitaly Burovoy wrote:


Added to the commitfest 2016-03.

[CF] https://commitfest.postgresql.org/9/540/


This looks like a fairly straight-forward bug fix (the size of the 
patch is deceptive because there a lot of new tests included).  It 
applies cleanly.


Anastasia, I see you have signed up to review.  Do you have an idea 
when you will get the chance to do that?


Thanks,


I've read the patch thoroughly and haven't found any problems. I think 
that the patch is in a very good shape.

It fixes a bug and has an excellent set of tests.

There is an issue, mentioned in the thread above:


postgres=# select
postgres-#  to_char(date_trunc('week', '4713-01-01 BC'::date),'day')
postgres-# ,to_char(date_trunc('week', '4714-12-29 BC'::date),'day')
postgres-# ,to_char(date_trunc('week', '4714-12-28 BC'::date),'day');
 to_char  |  to_char  |  to_char
---+---+---
monday| monday| thursday
(1 row)



since 4714-12-28 BC and to the past detection when a week is starting
is broken (because it is boundary of isoyears -4713 and -4712).
Is it worth to break undocumented range or leave it as is?


But I suppose that behavior of undocumented dates is not essential.

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



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

Re: [HACKERS] unexpected result from to_tsvector

[Artur Zakirov]

On 14.03.2016 16:22, Shulgin, Oleksandr wrote:


Hm...  now that doesn't look all that consistent to me (after applying
the patch):

=# select ts_debug('simple', 'a...@123-yyy.zzz');
  ts_debug
---
  (email,"Email address",a...@123-yyy.zzz,{simple},simple,{a...@123-yyy.zzz})
(1 row)

But:

=# select ts_debug('simple', 'aaa@123_yyy.zzz');
 ts_debug
-
  (asciiword,"Word, all ASCII",aaa,{simple},simple,{aaa})
  (blank,"Space symbols",@,{},,)
  (uint,"Unsigned integer",123,{simple},simple,{123})
  (blank,"Space symbols",_,{},,)
  (host,Host,yyy.zzz,{simple},simple,{yyy.zzz})
(5 rows)

One can also see that if we only keep the domain name, the result is
similar:

=# select ts_debug('simple', '123-yyy.zzz');
ts_debug
---
  (host,Host,123-yyy.zzz,{simple},simple,{123-yyy.zzz})
(1 row)

=# select ts_debug('simple', '123_yyy.zzz');
   ts_debug
-
  (uint,"Unsigned integer",123,{simple},simple,{123})
  (blank,"Space symbols",_,{},,)
  (host,Host,yyy.zzz,{simple},simple,{yyy.zzz})
(3 rows)

But, this only has to do with 123 being recognized as a number, not with
the underscore:

=# select ts_debug('simple', 'abc_yyy.zzz');
ts_debug
---
  (host,Host,abc_yyy.zzz,{simple},simple,{abc_yyy.zzz})
(1 row)

=# select ts_debug('simple', '1abc_yyy.zzz');
ts_debug
---
  (host,Host,1abc_yyy.zzz,{simple},simple,{1abc_yyy.zzz})
(1 row)

In fact, the 123-yyy.zzz domain is not valid either according to the RFC
(subdomain can't start with a digit), but since we already allow it,
should we not allow 123_yyy.zzz to be recognized as a Host?  Then why
not recognize aaa@123_yyy.zzz as an email address?

Another option is to prohibit underscore in recognized host names, but
this has more breakage potential IMO.

--
Alex



It seems reasonable to me. I like more first option. But I am not 
confident that we should allow 123_yyy.zzz to be recognized as a Host.


By the way, in this question http://webmasters.stackexchange.com/a/775 
you can see examples of domain names with numbers (but not subdomains).


If there are not objections from others, I will send a new patch today 
later or tomorrow with 123_yyy.zzz recognizing.


--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


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

Re: [HACKERS] plpgsql - DECLARE - cannot to use %TYPE or %ROWTYPE for composite types

[Tom Lane]

Joe Conway  writes:
> This new version of the patch was posted after the commitfest item was
> marked ready for committer. Does anyone have further comments or
> objections to the concept or syntax before I try to take this forward?

The quoted excerpt fails to say what solution was adopted to the array
syntax issues, so it's impossible to have an opinion without actually
reading the patch.

However ... one thing I was intending to mention on this thread is that
"get the array type over this type" isn't the only extension one might
wish for.  Another likely desire is "get the type of field 'foo' of this
composite type".  I don't suggest that this patch needs to implement
that right now; but it would be a good thing if we could see how the
chosen syntax could be extended in such a direction.  Otherwise we might
be painting ourselves into a corner.

regards, tom lane


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

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[David Steele]

On 2/23/16 2:17 AM, Michael Paquier wrote:


As a continuation of the thread firstly dedicated to SCRAM:
http://www.postgresql.org/message-id/55192afe.6080...@iki.fi
Here is a new thread aimed at gathering all the ideas of this previous
thread and aimed at clarifying a bit what has been discussed until now
regarding password protocols, verifiers, and SCRAM itself.


It looks like this patch set is a bit out of date.

When applying 0004:

$ git apply 
../other/0004-Remove-password-verifiers-for-unsupported-protocols-.patch

error: patch failed: src/bin/pg_upgrade/pg_upgrade.c:262
error: src/bin/pg_upgrade/pg_upgrade.c: patch does not apply

Then I tried to build with just 0001-0003:

cd /postgres/src/include/catalog && '/usr/bin/perl' ./duplicate_oids
3318
3319
3320
3321
3322
make[3]: *** [postgres.bki] Error 1

Could you provide an updated set of patches for review?  Meanwhile I am 
marking this as "waiting for author".


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


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

Re: [HACKERS] [PATCH] Use correct types and limits for PL/Perl SPI query results

[Tom Lane]

ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> 1) Perl's integers are at least pointer-sized and either signed or
>unsigned, so can potentially hold up to 2⁶⁴-1. Floating point numbers
>can also be larger than double (up to 128bit), allowing for exact
>representation of integers beyond 2⁵³.  Hence, adjust the setting of
>the "processed" hash item to use UV_MAX for the limit and (NV) or
>(UV) for the casts.

I thought about using UV where feasible, but it was not clear to me
whether unsigned numbers behave semantically differently from signed ones
in Perl.  If they do, the change you suggest would create a small semantic
change in the behavior of code using spi_exec_query.  I'm not sure it's
worth taking any risk of that, considering we already discourage people
from using spi_exec_query for large results.

> 2) Perl 5.20 and later use SSize_t for array indices, so can cope with
>up to SSize_t_max items in an array (if you have the memory).

I don't much like having such hard-wired knowledge about Perl versions
in our code.  Is there a way to identify this change other than
#if PERL_BCDVERSION >= 0x5019004 ?

> 3) To be able to actually return such result sets from sp_exec_query(),
>I had to change the repalloc() in spi_printtup() to repalloc_huge().

Hmm, good point.  I wonder if there are any hazards to doing that.

regards, tom lane


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

Re: [HACKERS] Fuzzy substring searching with the pg_trgm extension

[David Steele]

Hi Jeff,

On 2/25/16 5:00 PM, Jeff Janes wrote:


But, It doesn't sound like I am going to win that debate.  Given that,
I don't think we need a different name for the function. I'm fine with
explaining the word-boundary subtlety in the documentation, and
keeping the function name itself simple.


It's not clear to me if you are requesting more documentation here or 
stating that you are happy with it as-is.  Care to elaborate?


Other than that I think this patch looks to be ready for committer. Any 
objections?


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


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

[HACKERS] [PATCH] Use correct types and limits for PL/Perl SPI query results

[Dagfinn Ilmari Mannsåker]

Hi hackers,

Commit 23a27b039d94ba359286694831eafe03cd970eef changed the type of
numbers-of-tuples-processed counters to uint64 and adjusted various PLs
to cope with this.  I noticed the PL/Perl changes did not take full
advantage of what Perl is capable of handling, so here's a patch that
improves that.

1) Perl's integers are at least pointer-sized and either signed or
   unsigned, so can potentially hold up to 2⁶⁴-1. Floating point numbers
   can also be larger than double (up to 128bit), allowing for exact
   representation of integers beyond 2⁵³.  Hence, adjust the setting of
   the "processed" hash item to use UV_MAX for the limit and (NV) or
   (UV) for the casts.

2) Perl 5.20 and later use SSize_t for array indices, so can cope with
   up to SSize_t_max items in an array (if you have the memory).

3) To be able to actually return such result sets from sp_exec_query(),
   I had to change the repalloc() in spi_printtup() to repalloc_huge().

-- 
"A disappointingly low fraction of the human race is,
 at any given time, on fire." - Stig Sandbeck Mathisen

>From f6d38f1a5c7d6135dd5f580aa8ed38bb74162a0f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= 
Date: Sun, 13 Mar 2016 01:21:43 +
Subject: [PATCH] Use correct types and limits for PL/Perl SPI query results

Perl's integers are pointer-sized, so can hold more than INT_MAX on LP64
platforms, and come in both signed (IV) and unsigned (UV).  Floating
point values (NV) may also be larger than double.

Since Perl 5.19.4 array indices are SSize_t instead of I32, so allow up
to SSize_t_max on those versions.  The limit is not imposed just by
av_extend's argument type, but all the array handling code, so remove
the speculative comment.

Finally, change spi_printtup() to use repalloc_huge() to be able to
return a tuple table of more than 1GiB.
---
 src/backend/executor/spi.c |  2 +-
 src/pl/plperl/plperl.c | 13 -
 src/pl/plperl/plperl.h |  7 +++
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index 3d04c23..fd94179 100644
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1800,7 +1800,7 @@ spi_printtup(TupleTableSlot *slot, DestReceiver *self)
 		/* Double the size of the pointer array */
 		tuptable->free = tuptable->alloced;
 		tuptable->alloced += tuptable->free;
-		tuptable->vals = (HeapTuple *) repalloc(tuptable->vals,
+		tuptable->vals = (HeapTuple *) repalloc_huge(tuptable->vals,
 	  tuptable->alloced * sizeof(HeapTuple));
 	}
 
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 269f7f3..d405179 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -3104,9 +3104,9 @@ plperl_spi_execute_fetch_result(SPITupleTable *tuptable, uint64 processed,
 	hv_store_string(result, "status",
 	cstr2sv(SPI_result_code_string(status)));
 	hv_store_string(result, "processed",
-	(processed > (uint64) INT_MAX) ?
-	newSVnv((double) processed) :
-	newSViv((int) processed));
+	(processed > (uint64) UV_MAX) ?
+	newSVnv((NV) processed) :
+	newSVuv((UV) processed));
 
 	if (status > 0 && tuptable)
 	{
@@ -3114,12 +3114,7 @@ plperl_spi_execute_fetch_result(SPITupleTable *tuptable, uint64 processed,
 		SV		   *row;
 		uint64		i;
 
-		/*
-		 * av_extend's 2nd argument is declared I32.  It's possible we could
-		 * nonetheless push more than INT_MAX elements into a Perl array, but
-		 * let's just fail instead of trying.
-		 */
-		if (processed > (uint64) INT_MAX)
+		if (processed > AV_SIZE_MAX)
 			ereport(ERROR,
 	(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
 			errmsg("query result has too many rows to fit in a Perl array")));
diff --git a/src/pl/plperl/plperl.h b/src/pl/plperl/plperl.h
index 9179bbd..2f376ee 100644
--- a/src/pl/plperl/plperl.h
+++ b/src/pl/plperl/plperl.h
@@ -100,4 +100,11 @@ void		plperl_spi_freeplan(char *);
 void		plperl_spi_cursor_close(char *);
 char	   *plperl_sv_to_literal(SV *, char *);
 
+/* Perl 5.19.4 changed array indices from I32 to SSize_t */
+#if PERL_BCDVERSION >= 0x5019004
+#define AV_SIZE_MAX ((uint64) SSize_t_MAX)
+#else
+#define AV_SIZE_MAX ((uint64) I32_MAX)
+#endif
+
 #endif   /* PL_PERL_H */
-- 
2.7.0


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

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[Michael Paquier]

On Mon, Mar 14, 2016 at 4:32 PM, David Steele  wrote:
> On 2/23/16 2:17 AM, Michael Paquier wrote:
>
>> As a continuation of the thread firstly dedicated to SCRAM:
>> http://www.postgresql.org/message-id/55192afe.6080...@iki.fi
>> Here is a new thread aimed at gathering all the ideas of this previous
>> thread and aimed at clarifying a bit what has been discussed until now
>> regarding password protocols, verifiers, and SCRAM itself.
>
>
> It looks like this patch set is a bit out of date.
>
> When applying 0004:
>
> $ git apply
> ../other/0004-Remove-password-verifiers-for-unsupported-protocols-.patch
> error: patch failed: src/bin/pg_upgrade/pg_upgrade.c:262
> error: src/bin/pg_upgrade/pg_upgrade.c: patch does not apply
>
> Then I tried to build with just 0001-0003:
>
> cd /postgres/src/include/catalog && '/usr/bin/perl' ./duplicate_oids
> 3318
> 3319
> 3320
> 3321
> 3322
> make[3]: *** [postgres.bki] Error 1
>
> Could you provide an updated set of patches for review?  Meanwhile I am
> marking this as "waiting for author".

Sure. I'll provide them shortly with all the comments addressed. Up to
now I just had a couple of comments about docs and whitespaces, so I
didn't really bother sending a new set, but this meritates a rebase.
-- 
Michael


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

Re: [HACKERS] proposal: function parse_ident

[Teodor Sigaev]

I afraid so I cannot to fix this inconsistency (if this is inconsistency - the
binary values are same) - the parameter of function is raw string with processed
escape codes, and I have not any information about original escape sequences.
When you enter octet value, and I show it as hex value, then there should be
difference. Buy I have not information about your input (octet or hex). I have
the original string of SQL identifier inside parser, executor, but I have not
original string of function parameter inside function (not without pretty
complex and long code).

Ok, agree



I am trying describe it in doc (I am sorry for my less level English) in new
patch. Fixed duplicated oid too.

Edited a bit + fix some typos and remove unneeded headers, patch attached

Sorry, I can't find all corner-cases at once, but:
SELECT parse_ident(E'"c".X XX');
ERROR:  identifier contains disallowed characters: "\"c"

Error message wrongly points to the reason of error.




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


parse_ident-13.patch
Description: binary/octet-stream

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

Re: [HACKERS] [PATCH] Integer overflow in timestamp[tz]_part() and date/time boundaries check

[Mark Dilger]

> On Mar 14, 2016, at 6:23 AM, David Steele  wrote:
> 
> On 2/25/16 4:44 PM, Vitaly Burovoy wrote:
> 
>> Added to the commitfest 2016-03.
>> 
>> [CF] https://commitfest.postgresql.org/9/540/
> 
> This looks like a fairly straight-forward bug fix (the size of the patch is 
> deceptive because there a lot of new tests included).  It applies cleanly.
> 
> Anastasia, I see you have signed up to review.  Do you have an idea when you 
> will get the chance to do that?

The first thing I notice about this patch is that 
src/include/datatype/timestamp.h
has some #defines that are brittle.  The #defines have comments explaining
their logic, but I'd rather embed that in the #define directly:

This:

+#ifdef HAVE_INT64_TIMESTAMP
+#define MIN_TIMESTAMP  INT64CONST(-2118134880)
+/* == (0 - POSTGRES_EPOCH_JDATE) * 86400 * USECS_PER_SEC */
+#define MAX_TIMESTAMP  INT64CONST(92233713312)
+/* == (JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * 86400 * USECS_PER_SEC */
+#else
+#define MIN_TIMESTAMP  -211813488000.0
+/* == (0 - POSTGRES_EPOCH_JDATE) * 86400 */
+#define MAX_TIMESTAMP  9223371331200.0
+/* == (JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * 86400 */
+#endif

Could be written as:

#ifdef HAVE_INT64_TIMESTAMP
#define MIN_TIMESTAMP   ((INT64CONST(0) - POSTGRES_EPOCH_JDATE) * USECS_PER_DAY)
#define MAX_TIMESTAMP   ((JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * 
USECS_PER_DAY)
#else
#define MIN_TIMESTAMP   ((INT64CONST(0) - POSTGRES_EPOCH_JDATE) * SECS_PER_DAY)
#define MAX_TIMESTAMP   ((JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * 
SECS_PER_DAY)
#endif

I assume modern compilers would convert these to the same constants at 
compile-time,
rather than impose a runtime penalty.  The #defines would be less brittle in 
the event, for
example, that the postgres epoch were ever changed.

Mark Dilger

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

Re: [HACKERS] Fuzzy substring searching with the pg_trgm extension

[Artur Zakirov]

On 14.03.2016 18:48, David Steele wrote:

Hi Jeff,

On 2/25/16 5:00 PM, Jeff Janes wrote:


But, It doesn't sound like I am going to win that debate.  Given that,
I don't think we need a different name for the function. I'm fine with
explaining the word-boundary subtlety in the documentation, and
keeping the function name itself simple.


It's not clear to me if you are requesting more documentation here or
stating that you are happy with it as-is.  Care to elaborate?

Other than that I think this patch looks to be ready for committer. Any
objections?



There was some comments about the word-boundary subtlety. But I think it 
was not enough.


I rephrased the explanation of word_similarity() and %>. It is better now.

But if it is not correct I can change the explanation.

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
*** a/contrib/pg_trgm/pg_trgm--1.2.sql
--- b/contrib/pg_trgm/pg_trgm--1.2.sql
***
*** 3,13 
--- 3,15 
  -- complain if script is sourced in psql, rather than via CREATE EXTENSION
  \echo Use "CREATE EXTENSION pg_trgm" to load this file. \quit
  
+ -- Deprecated function
  CREATE FUNCTION set_limit(float4)
  RETURNS float4
  AS 'MODULE_PATHNAME'
  LANGUAGE C STRICT VOLATILE;
  
+ -- Deprecated function
  CREATE FUNCTION show_limit()
  RETURNS float4
  AS 'MODULE_PATHNAME'
***
*** 26,32  LANGUAGE C STRICT IMMUTABLE;
  CREATE FUNCTION similarity_op(text,text)
  RETURNS bool
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT STABLE;  -- stable because depends on trgm_limit
  
  CREATE OPERATOR % (
  LEFTARG = text,
--- 28,34 
  CREATE FUNCTION similarity_op(text,text)
  RETURNS bool
  AS 'MODULE_PATHNAME'
! LANGUAGE C STRICT STABLE;  -- stable because depends on pg_trgm.similarity_threshold
  
  CREATE OPERATOR % (
  LEFTARG = text,
*** a/contrib/pg_trgm/trgm.h
--- b/contrib/pg_trgm/trgm.h
***
*** 105,111  typedef char *BITVECP;
  
  typedef struct TrgmPackedGraph TrgmPackedGraph;
  
! extern float4 trgm_limit;
  
  extern uint32 trgm2int(trgm *ptr);
  extern void compact_trigram(trgm *tptr, char *str, int bytelen);
--- 105,111 
  
  typedef struct TrgmPackedGraph TrgmPackedGraph;
  
! extern double similarity_threshold;
  
  extern uint32 trgm2int(trgm *ptr);
  extern void compact_trigram(trgm *tptr, char *str, int bytelen);
*** a/contrib/pg_trgm/trgm_gin.c
--- b/contrib/pg_trgm/trgm_gin.c
***
*** 206,212  gin_trgm_consistent(PG_FUNCTION_ARGS)
  			 * similarity is just c / len1.
  			 * So, independly on DIVUNION the upper bound formula is the same.
  			 */
! 			res = (nkeys == 0) ? false : ((float4) ntrue) / ((float4) nkeys))) >= trgm_limit) ? true : false);
  			break;
  		case ILikeStrategyNumber:
  #ifndef IGNORECASE
--- 206,214 
  			 * similarity is just c / len1.
  			 * So, independly on DIVUNION the upper bound formula is the same.
  			 */
! 			res = (nkeys == 0) ? false :
! ((float4) ntrue) / ((float4) nkeys))) >= similarity_threshold)
! 	? true : false);
  			break;
  		case ILikeStrategyNumber:
  #ifndef IGNORECASE
***
*** 283,289  gin_trgm_triconsistent(PG_FUNCTION_ARGS)
  			/*
  			 * See comment in gin_trgm_consistent() about * upper bound formula
  			 */
! 			res = (nkeys == 0) ? GIN_FALSE : (float4) ntrue) / ((float4) nkeys)) >= trgm_limit) ? GIN_MAYBE : GIN_FALSE);
  			break;
  		case ILikeStrategyNumber:
  #ifndef IGNORECASE
--- 285,293 
  			/*
  			 * See comment in gin_trgm_consistent() about * upper bound formula
  			 */
! 			res = (nkeys == 0) ? GIN_FALSE :
! (float4) ntrue) / ((float4) nkeys)) >= similarity_threshold)
! 	? GIN_MAYBE : GIN_FALSE);
  			break;
  		case ILikeStrategyNumber:
  #ifndef IGNORECASE
*** a/contrib/pg_trgm/trgm_gist.c
--- b/contrib/pg_trgm/trgm_gist.c
***
*** 294,300  gtrgm_consistent(PG_FUNCTION_ARGS)
  float4		tmpsml = cnt_sml(key, qtrg);
  
  /* strange bug at freebsd 5.2.1 and gcc 3.3.3 */
! res = (*(int *)  == *(int *) _limit || tmpsml > trgm_limit) ? true : false;
  			}
  			else if (ISALLTRUE(key))
  			{	/* non-leaf contains signature */
--- 294,301 
  float4		tmpsml = cnt_sml(key, qtrg);
  
  /* strange bug at freebsd 5.2.1 and gcc 3.3.3 */
! res = (*(int *)  == *(int *) _threshold
! 		|| tmpsml > similarity_threshold) ? true : false;
  			}
  			else if (ISALLTRUE(key))
  			{	/* non-leaf contains signature */
***
*** 308,314  gtrgm_consistent(PG_FUNCTION_ARGS)
  if (len == 0)
  	res = false;
  else
! 	res = (float8) count) / ((float8) len))) >= trgm_limit) ? true : false;
  			}
  			break;
  		case ILikeStrategyNumber:
--- 309,316 
  if (len == 0)
  	res = false;
  else
! 	res = (float8) count) / ((float8) len))) >= similarity_threshold)
! 			? true : false;
  			}
  			break;
  		case ILikeStrategyNumber:
*** 

Re: [HACKERS] [patch] Proposal for \crosstabview in psql

[Alvaro Herrera]

Jim Nasby wrote:
> On 3/13/16 12:48 AM, Pavel Stehule wrote:
> >crosstabview is really visualization tool. **But now, there are not any
> >other tool available from terminal.** So this can be significant help to
> >all people who would to use this functionality.
> 
> Not just the terminal either. Offhand I'm not aware of *any* fairly simple
> tool that provides crosstab. There's a bunch of complicated/expensive BI
> tools that do, but unless you've gone through the trouble of getting one of
> those setup you're currently pretty stuck.

I'm definitely +1 for this feature in psql also.

Some years ago we had a discussion about splitting psql in two parts, a
bare-bones one which would help script-writing and another one with
fancy features; we decided to keep one tool to rule them all and made
the implicit decision that we would grow exotic, sophisticated features
into psql.  ISTM that this patch is going in that direction.

> Ultimately I'd really like some way to remove/reduce the restriction of
> result set definitions needing to be determined at plan time. That would
> open the door for server-side crosstab/pivot as well a a host of other
> things (such as dynamically turning a hstore/json/xml field into a
> recordset).

That seems so far down the road that I don't think it should block the
psql feature being proposed in this thread, but yes I would like that
one too.

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


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

[HACKERS] Re: [COMMITTERS] pgsql: Refactor to create generic WAL page read callback

[Alvaro Herrera]

Simon Riggs wrote:
> Refactor to create generic WAL page read callback
> 
> Previously we didn’t have a generic WAL page read callback function,
> surprisingly. Logical decoding has logical_read_local_xlog_page(), which was
> actually generic, so move that to xlogfunc.c and rename to
> read_local_xlog_page().
> Maintain logical_read_local_xlog_page() so existing callers still work.

I wonder why we kept logical_read_local_xlog_page at all in this commit.
I vote for removing it completely and changing all existing code to use
the newer function.  If some external module is already calling it for
some reason, surely they can simply add
#define logical_read_local_xlog_page(a,b,c) read_local_xlog_page(a,b,c)
in a compatibility shim.

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


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

Re: [HACKERS] pg_stat_get_progress_info(NULL) blows up

[Tom Lane]

Thomas Munro  writes:
> I guess pg_stat_get_progress_info should either be strict (see
> attached) or check for NULL.

Pushed, thanks.

regards, tom lane


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

Re: [HACKERS] remove wal_level archive

[Michael Paquier]

On Mon, Mar 14, 2016 at 12:50 PM, David Steele  wrote:
> On 3/11/16 1:29 PM, David Steele wrote:
>
>> Unless anyone has objections I would like to mark this 'ready for
>> committer'.
>
>
> This patch is now ready for committer.

Yes, thanks, I am cool with this version as well. I guess I should
have done what you just did at my last review to be honest.
-- 
Michael


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

Re: [HACKERS] Background Processes and reporting

[Kevin Grittner]

On Sat, Mar 12, 2016 at 11:40 AM, Vladimir Borodin  wrote:
> 12 марта 2016 г., в 13:59, Amit Kapila  написал(а):

>> I think here another point which needs more thoughts is that many of the
>> pg_stat_activity fields are not relevant for background processes, ofcourse
>> one can say that we can keep those fields as NULL, but still I think that
>> indicates it is not the most suitable way to expose such information.
>>
>> Another way could be to have new view like pg_stat_background_activity with
>> only relevant fields or try expose via individual views like
>> pg_stat_bgwriter.
>
> From the DBA point of view it is much more convenient to see all wait events
> in one view. I don’t know if it is right to break compability even more, but
> IMHO exposing this data in different views is a bad plan.

+1

If they are split into separate views I think that there will be a
lot of effort put into views to present the UNION of them, probably
with weird corner cases and race conditions.  A single view can
probably better manage race conditions, and a WHERE clause is not
as tricky for the DBA and/or end user.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] plpgsql - DECLARE - cannot to use %TYPE or %ROWTYPE for composite types

[Artur Zakirov]

On 14.03.2016 17:54, Tom Lane wrote:

Joe Conway  writes:

This new version of the patch was posted after the commitfest item was
marked ready for committer. Does anyone have further comments or
objections to the concept or syntax before I try to take this forward?


The quoted excerpt fails to say what solution was adopted to the array
syntax issues, so it's impossible to have an opinion without actually
reading the patch.

However ... one thing I was intending to mention on this thread is that
"get the array type over this type" isn't the only extension one might
wish for.  Another likely desire is "get the type of field 'foo' of this
composite type".  I don't suggest that this patch needs to implement
that right now; but it would be a good thing if we could see how the
chosen syntax could be extended in such a direction.  Otherwise we might
be painting ourselves into a corner.

regards, tom lane



I looked this patch and the previous. The patch applies correctly to 
HEAD. Regression tests pass successfully, without errors.


In comparison with the previous patch it adds the following functionality:
- %TYPE - now can be used for composite types (same syntax).
- %TYPE[] - new functionality, provides the array type from a
variable or table column (syntax was changed).
- var ELEMENT OF othervar%TYPE - new funcitonality, provides the element 
type of a given array (syntax was changed).


Was changed plpgsql_derive_type(). Now it has the following definition:


PLpgSQL_type *
plpgsql_derive_type(PLpgSQL_type *base_type, bool to_element_type, bool 
to_array_type)


Previously it had the following definition:


static PLpgSQL_type *
derive_type(PLpgSQL_type *base_type, PLpgSQL_reftype reftype)


where PLpgSQL_reftype was the enum:


+ typedef enum
+ {
+   PLPGSQL_REFTYPE_TYPE,   /* use type of some variable */
+   PLPGSQL_REFTYPE_ELEMENT,/* use a element type of referenced 
variable */
+   PLPGSQL_REFTYPE_ARRAY   /* use a array type of referenced 
variable */
+ } PLpgSQL_reftype;


I think the previous version was better, because enum is better than 
additional function parameters. But it is only for me.


Also there is a little typo here:


+  * This routine is used for generating element or array type from base type.
+  * The options to_element_type and to_array_type can be used together, when
+  * we would to ensure valid result. The array array type is original type, so
+  * this direction is safe. The element of scalar type is not allowed, but if
+  * we do "to array" transformation first, then this direction should be safe
+  * too. This design is tolerant, because we should to support a design of
+  * polymorphic parameters, where a array value can be passed as anyelement
+  * or anyarray parameter.
+  */
+ PLpgSQL_type *
+ plpgsql_derive_type(PLpgSQL_type *base_type,


Here the word "array" occurs two times in the third line.

--
Artur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


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

Re: [HACKERS] Re: PATCH: Split stats file per database WAS: autovacuum stress-testing our system

[Noah Misch]

[Aside: your new mail editor is rewrapping lines in quoted material, and the
result is messy.  I have rerewrapped one paragraph below.]

On Mon, Mar 14, 2016 at 02:00:03AM +0100, Tomas Vondra wrote:
> On Sun, 2016-03-13 at 18:46 -0400, Noah Misch wrote:
> > I've not attempted to study the behavior on slow hardware.  Instead, my
> > report used stat-coalesce-v1.patch[1] to simulate slow writes.  (That
> > diagnostic patch no longer applies cleanly, so I'm attaching a rebased
> > version.  I've changed the patch name from "stat-coalesce" to
> > "slow-stat-simulate" to more-clearly distinguish it from the
> > "pgstat-coalesce" patch.)  Problems remain after applying your patch;
> > consider "VACUUM pg_am" behavior:
> > 
> > 9.2 w/ stat-coalesce-v1.patch:
> >   VACUUM returns in 3s, stats collector writes each file 1x over 3s
> > HEAD w/ slow-stat-simulate-v2.patch:
> >   VACUUM returns in 3s, stats collector writes each file 5x over 15s
> > HEAD w/ slow-stat-simulate-v2.patch and your patch:
> >   VACUUM returns in 10s, stats collector writes no files over 10s
> 
> Oh damn, the timestamp comparison in pgstat_recv_inquiry should be in
> the opposite direction. After fixing that "VACUUM pg_am" completes in 3
> seconds and writes each file just once.

That fixes things.  "make check" passes under an 8s stats write time.

> --- a/src/backend/postmaster/pgstat.c
> +++ b/src/backend/postmaster/pgstat.c
> @@ -4836,6 +4836,20 @@ pgstat_recv_inquiry(PgStat_MsgInquiry *msg, int len)
>   }
>  
>   /*
> +  * Ignore requests that are already resolved by the last write.
> +  *
> +  * We discard the queue of requests at the end of 
> pgstat_write_statsfiles(),
> +  * so the requests already waiting on the UDP socket at that moment 
> can't
> +  * be discarded in the previous loop.
> +  *
> +  * XXX Maybe this should also care about the clock skew, just like the
> +  * block a few lines down.

Yes, it should.  (The problem is large (>~100s), backward clock resets, not
skew.)  A clock reset causing "msg->clock_time < dbentry->stats_timestamp"
will usually also cause "msg->cutoff_time < dbentry->stats_timestamp".  Such
cases need the correction a few lines down.

The other thing needed here is to look through and update comments about
last_statrequests.  For example, this loop is dead code due to us writing
files as soon as we receive one inquiry:

/*
 * Find the last write request for this DB.  If it's older than the
 * request's cutoff time, update it; otherwise there's nothing to do.
 *
 * Note that if a request is found, we return early and skip the below
 * check for clock skew.  This is okay, since the only way for a DB
 * request to be present in the list is that we have been here since the
 * last write round.
 */
slist_foreach(iter, _statrequests) ...

I'm okay keeping the dead code for future flexibility, but the comment should
reflect that.

Thanks,
nm


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

Re: [HACKERS] Logical decoding slots can go backwards when used from SQL, docs are wrong

[Craig Ringer]

On 11 March 2016 at 20:15, Alvaro Herrera  wrote:

> Craig Ringer wrote:
> > Hi all
> >
> > I think I found a couple of logical decoding issues while writing tests
> for
> > failover slots.
> >
> > Despite the docs' claim that a logical slot will replay data "exactly
> > once", a slot's confirmed_lsn can go backwards and the SQL functions can
> > replay the same data more than once.We don't mark a slot as dirty if only
> > its confirmed_lsn is advanced, so it isn't flushed to disk. For failover
> > slots this means it also doesn't get replicated via WAL. After a master
> > crash, or for failover slots after a promote event, the confirmed_lsn
> will
> > go backwards.  Users of the SQL interface must keep track of the safely
> > locally flushed slot position themselves and throw the repeated data
> away.
> > Unlike with the walsender protocol it has no way to ask the server to
> skip
> > that data.
> >
> > Worse, because we don't dirty the slot even a *clean shutdown* causes
> slot
> > confirmed_lsn to go backwards. That's a bug IMO. We should force a flush
> of
> > all slots at the shutdown checkpoint, whether dirty or not, to address
> it.
>
> Why don't we mark the slot dirty when confirmed_lsn advances?  If we fix
> that, doesn't it fix the other problems too?
>

Yes, it does.

That'll cause slots to be written out at checkpoints when they otherwise
wouldn't have to be, but I'd rather be doing a little more work in this
case. Compared to the disk activity from WAL decoding etc the effect should
be undetectable anyway.

Andres? Any objection to dirtying a slot when the confirmed lsn advances,
so we write it out at the next checkpoint?

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

Re: [HACKERS] Obsolete comment in postgres_fdw.c

[Etsuro Fujita]

On 2016/03/14 16:42, Ashutosh Bapat wrote:

On Mon, Mar 14, 2016 at 9:05 AM, Etsuro Fujita
> wrote:



Here is the comments for foreign_join_ok in postgres_fdw.c:

/*
  * Assess whether the join between inner and outer relations can be
pushed down
  * to the foreign server. As a side effect, save information we obtain
in this
  * function to PgFdwRelationInfo passed in.
  *
  * Joins that satisfy conditions below are safe to push down.
  *
  * 1) Join type is INNER or OUTER (one of LEFT/RIGHT/FULL)
  * 2) Both outer and inner portions are safe to push-down
  * 3) All foreign tables in the join belong to the same foreign server
and use
  *the same user mapping.
  * 4) All join conditions are safe to push down
  * 5) No relation has local filter (this can be relaxed for INNER JOIN,
if we
  *can move unpushable clauses upwards in the join tree).
  */

The condition 3 is now checked by the core, so I'd like to remove that
condition from the above comments.



It was left there intentionally to document all the conditions in one
place (some from the core and some from the FDW itself), for a ready
reference. In case tomorrow core thinks that matching user mapping is
not required, postgres_fdw would still require it to be incorporated.


Thank you for the explanation!  I understand the reason, but that seems 
confusing to me.



In addition, I'd like to update some related comments in
src/include/nodes/relation.h and src/backend/optimizer/path/joinpath.c.


Those look fine. Sorry for missing those in the commit and thanks for
providing a patch for the same.


No problem.

Best regards,
Etsuro Fujita




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

Re: [HACKERS] Use %u to print user mapping's umid and userid

[Etsuro Fujita]

Hi,

On 2016/02/09 14:09, Ashutosh Bapat wrote:

Sorry, I was wrong. For public user mapping userid is 0 (InvalidOid),
which is returned as is in UserMapping object. I confused InvalidOid
with -1.


I think the following umid handling in postgresGetForeignPlan has the 
same issue:


/*
 * Build the fdw_private list that will be available to the executor.
 * Items in the list must match order in enum FdwScanPrivateIndex.
 */
fdw_private = list_make4(makeString(sql.data),
 retrieved_attrs,
 makeInteger(fpinfo->fetch_size),
 makeInteger(foreignrel->umid));

I don't think it's correct to use makeInteger for the foreignrel's umid.

You store the umid in the fdw_private list here and extract it from the 
list in postgresBeginForeignScan, to get the user mapping.  But we 
really need that?  We have a validated plan when getting called from 
postgresBeginForeignScan, so if foreign join, we can simply pick any of 
the plan's fs_relids and use it to identify which user to do the remote 
access as, in the same way as for foreign tables.


Attached is a patch for that.

Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 65,72  enum FdwScanPrivateIndex
  	FdwScanPrivateRetrievedAttrs,
  	/* Integer representing the desired fetch_size */
  	FdwScanPrivateFetchSize,
- 	/* Oid of user mapping to be used while connecting to the foreign server */
- 	FdwScanPrivateUserMappingOid,
  
  	/*
  	 * String describing join i.e. names of relations being joined and types
--- 65,70 
***
*** 1133,1142  postgresGetForeignPlan(PlannerInfo *root,
  	 * Build the fdw_private list that will be available to the executor.
  	 * Items in the list must match order in enum FdwScanPrivateIndex.
  	 */
! 	fdw_private = list_make4(makeString(sql.data),
  			 retrieved_attrs,
! 			 makeInteger(fpinfo->fetch_size),
! 			 makeInteger(foreignrel->umid));
  	if (foreignrel->reloptkind == RELOPT_JOINREL)
  		fdw_private = lappend(fdw_private,
  			  makeString(fpinfo->relation_name->data));
--- 1131,1139 
  	 * Build the fdw_private list that will be available to the executor.
  	 * Items in the list must match order in enum FdwScanPrivateIndex.
  	 */
! 	fdw_private = list_make3(makeString(sql.data),
  			 retrieved_attrs,
! 			 makeInteger(fpinfo->fetch_size));
  	if (foreignrel->reloptkind == RELOPT_JOINREL)
  		fdw_private = lappend(fdw_private,
  			  makeString(fpinfo->relation_name->data));
***
*** 1168,1174  postgresBeginForeignScan(ForeignScanState *node, int eflags)
--- 1165,1175 
  	ForeignScan *fsplan = (ForeignScan *) node->ss.ps.plan;
  	EState	   *estate = node->ss.ps.state;
  	PgFdwScanState *fsstate;
+ 	RangeTblEntry *rte;
+ 	Oid			userid;
+ 	ForeignTable *table;
  	UserMapping *user;
+ 	int			rtindex;
  	int			numParams;
  	int			i;
  	ListCell   *lc;
***
*** 1193,1221  postgresBeginForeignScan(ForeignScanState *node, int eflags)
  	 * information goes stale between planning and execution, plan will be
  	 * invalidated and replanned.
  	 */
- 	if (fsplan->scan.scanrelid > 0)
- 	{
- 		ForeignTable *table;
  
! 		/*
! 		 * Identify which user to do the remote access as.  This should match
! 		 * what ExecCheckRTEPerms() does.
! 		 */
! 		RangeTblEntry *rte = rt_fetch(fsplan->scan.scanrelid, estate->es_range_table);
! 		Oid			userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
! 
! 		fsstate->rel = node->ss.ss_currentRelation;
! 		table = GetForeignTable(RelationGetRelid(fsstate->rel));
! 
! 		user = GetUserMapping(userid, table->serverid);
! 	}
  	else
  	{
! 		Oid			umid = intVal(list_nth(fsplan->fdw_private, FdwScanPrivateUserMappingOid));
! 
! 		user = GetUserMappingById(umid);
! 		Assert(fsplan->fs_server == user->serverid);
  	}
  
  	/*
  	 * Get connection to the foreign server.  Connection manager will
--- 1194,1217 
  	 * information goes stale between planning and execution, plan will be
  	 * invalidated and replanned.
  	 */
  
! 	/*
! 	 * Identify which user to do the remote access as.  This should match what
! 	 * ExecCheckRTEPerms() does.
! 	 */
! 	if (fsplan->scan.scanrelid > 0)
! 		rtindex = fsplan->scan.scanrelid;
  	else
  	{
! 		/* Pick the lowest-numbered one as a representative */
! 		rtindex = bms_first_member(fsplan->fs_relids);
  	}
+ 	rte = rt_fetch(rtindex, estate->es_range_table);
+ 	userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
+ 
+ 	/* Get info about foreign table */
+ 	table = GetForeignTable(rte->relid);
+ 	user = GetUserMapping(userid, table->serverid);
  
  	/*
  	 * Get connection to the foreign server.  Connection manager will
***
*** 1252,1260  postgresBeginForeignScan(ForeignScanState *node, int eflags)
--- 1248,1262 
  	 * into local representation and error reporting during that 

Re: [HACKERS] Use %u to print user mapping's umid and userid

[Kouhei Kaigai]

> -Original Message-
> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Etsuro Fujita
> Sent: Monday, March 14, 2016 4:59 PM
> To: Ashutosh Bapat; Tom Lane
> Cc: pgsql-hackers
> Subject: Re: [HACKERS] Use %u to print user mapping's umid and userid
> 
> Hi,
> 
> On 2016/02/09 14:09, Ashutosh Bapat wrote:
> > Sorry, I was wrong. For public user mapping userid is 0 (InvalidOid),
> > which is returned as is in UserMapping object. I confused InvalidOid
> > with -1.
> 
> I think the following umid handling in postgresGetForeignPlan has the
> same issue:
> 
>  /*
>   * Build the fdw_private list that will be available to the executor.
>   * Items in the list must match order in enum FdwScanPrivateIndex.
>   */
>  fdw_private = list_make4(makeString(sql.data),
>   retrieved_attrs,
>   makeInteger(fpinfo->fetch_size),
>   makeInteger(foreignrel->umid));
> 
> I don't think it's correct to use makeInteger for the foreignrel's umid.
>
BTW, use of ExtensibleNode allows to forget problems come from data format
translation.

--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei 

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

Re: [HACKERS] Use %u to print user mapping's umid and userid

[Ashutosh Bapat]

On Mon, Mar 14, 2016 at 1:29 PM, Etsuro Fujita 
wrote:

> Hi,
>
> On 2016/02/09 14:09, Ashutosh Bapat wrote:
>
>> Sorry, I was wrong. For public user mapping userid is 0 (InvalidOid),
>> which is returned as is in UserMapping object. I confused InvalidOid
>> with -1.
>>
>
> I think the following umid handling in postgresGetForeignPlan has the same
> issue:
>
> /*
>  * Build the fdw_private list that will be available to the executor.
>  * Items in the list must match order in enum FdwScanPrivateIndex.
>  */
> fdw_private = list_make4(makeString(sql.data),
>  retrieved_attrs,
>  makeInteger(fpinfo->fetch_size),
>  makeInteger(foreignrel->umid));
>
> I don't think it's correct to use makeInteger for the foreignrel's umid.
>

As long as we are using makeInteger() and inVal() pair to set and extract
the values, it should be fine.

>
> You store the umid in the fdw_private list here and extract it from the
> list in postgresBeginForeignScan, to get the user mapping.  But we really
> need that?  We have a validated plan when getting called from
> postgresBeginForeignScan, so if foreign join, we can simply pick any of the
> plan's fs_relids and use it to identify which user to do the remote access
> as, in the same way as for foreign tables.
>

We have done that calculation ones while creating the plan, why do we want
to do that again? For a base relation, the user mapping needs to be found
out at the time of execution, since it could change between plan creation
and execution. But for a join plan invalidation takes care of this change.


> Attached is a patch for that.
>
> Best regards,
> Etsuro Fujita
>



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

Re: [HACKERS] Logical decoding slots can go backwards when used from SQL, docs are wrong

[Craig Ringer]

On 14 March 2016 at 17:16, Petr Jelinek  wrote:


> It will not change the fact that slot can go backwards


Sure. I don't consider that a problem in general though. It's similar to
the way we lose cached sequence chunks on crash - a performance
optimisation with a user visible effect. It needs to be documented
correctly, but isn't IMO a problem.

Failing to save the full slot state on clean shutdown is though, IMO. It
wouldn't matter if it was only used for sync rep, but since it's also used
to manage the resume point for SQL-level logical decoding calls we should
make a reasonable effort to preserve it ... and allow the user to handle
the situation correctly when we fail to preserve it.


> You btw can emulate asking for the specific LSN in SQL interface by first
> calling the pg_logical_slot_get_changes function with upto_lsn set to
> whatever lsn you expect to start at, but it's ugly.


Ugh.

I didn't realise pg_logical_slot_get_changes could backtrack by setting an
upto_lsn in the past. That doesn't seem like something we should really be
doing - if it's a limit, and we're already past that limit, we should just
be returning the empty set.

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

Re: [HACKERS] Obsolete comment in postgres_fdw.c

[Ashutosh Bapat]

On Mon, Mar 14, 2016 at 9:05 AM, Etsuro Fujita 
wrote:

> Hi,
>
> Here is the comments for foreign_join_ok in postgres_fdw.c:
>
> /*
>  * Assess whether the join between inner and outer relations can be
> pushed down
>  * to the foreign server. As a side effect, save information we obtain
> in this
>  * function to PgFdwRelationInfo passed in.
>  *
>  * Joins that satisfy conditions below are safe to push down.
>  *
>  * 1) Join type is INNER or OUTER (one of LEFT/RIGHT/FULL)
>  * 2) Both outer and inner portions are safe to push-down
>  * 3) All foreign tables in the join belong to the same foreign server
> and use
>  *the same user mapping.
>  * 4) All join conditions are safe to push down
>  * 5) No relation has local filter (this can be relaxed for INNER JOIN,
> if we
>  *can move unpushable clauses upwards in the join tree).
>  */
>
>
The condition 3 is now checked by the core, so I'd like to remove that
> condition from the above comments.
>

It was left there intentionally to document all the conditions in one place
(some from the core and some from the FDW itself), for a ready reference.
In case tomorrow core thinks that matching user mapping is not required,
postgres_fdw would still require it to be incorporated.


>
> In addition, I'd like to update some related comments in
> src/include/nodes/relation.h and src/backend/optimizer/path/joinpath.c.
>

Those look fine. Sorry for missing those in the commit and thanks for
providing a patch for the same.

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

Re: [HACKERS] Logical decoding slots can go backwards when used from SQL, docs are wrong

[Petr Jelinek]

On 14/03/16 08:08, Craig Ringer wrote:

On 11 March 2016 at 20:15, Alvaro Herrera > wrote:

Craig Ringer wrote:
> Hi all
>
> I think I found a couple of logical decoding issues while writing tests 
for
> failover slots.
>
> Despite the docs' claim that a logical slot will replay data "exactly
> once", a slot's confirmed_lsn can go backwards and the SQL functions can
> replay the same data more than once.We don't mark a slot as dirty if only
> its confirmed_lsn is advanced, so it isn't flushed to disk. For failover
> slots this means it also doesn't get replicated via WAL. After a master
> crash, or for failover slots after a promote event, the confirmed_lsn will
> go backwards.  Users of the SQL interface must keep track of the safely
> locally flushed slot position themselves and throw the repeated data away.
> Unlike with the walsender protocol it has no way to ask the server to skip
> that data.
>
> Worse, because we don't dirty the slot even a *clean shutdown* causes slot
> confirmed_lsn to go backwards. That's a bug IMO. We should force a flush 
of
> all slots at the shutdown checkpoint, whether dirty or not, to address it.

Why don't we mark the slot dirty when confirmed_lsn advances?  If we fix
that, doesn't it fix the other problems too?


Yes, it does.



It will not change the fact that slot can go backwards however even in 
clean shutdown of the server as in walsender the confirmed_lsn only 
changes after feedback message so if client crashes it won't get updated 
(for obvious reasons).


You btw can emulate asking for the specific LSN in SQL interface by 
first calling the pg_logical_slot_get_changes function with upto_lsn set 
to whatever lsn you expect to start at, but it's ugly.


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


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

Re: [HACKERS] Logical decoding slots can go backwards when used from SQL, docs are wrong

[Petr Jelinek]

On 14/03/16 10:48, Craig Ringer wrote:


You btw can emulate asking for the specific LSN in SQL interface by
first calling the pg_logical_slot_get_changes function with upto_lsn
set to whatever lsn you expect to start at, but it's ugly.


Ugh.

I didn't realise pg_logical_slot_get_changes could backtrack by setting
an upto_lsn in the past. That doesn't seem like something we should
really be doing - if it's a limit, and we're already past that limit, we
should just be returning the empty set.



Not past, future from the point of old confirmed_lsn at least. The point 
was that the next call will start from whatever lsn you specified as 
upto_lsn.


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


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

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

[Rahila Syed]

Hello,

While I am still looking at this WIP patch, I had one suggestion.

Instead of making changes in the index AM API can we have a call to update
the shared state using pgstat_progress* API

directly from specific index level code?

Like  pgstat_count_index_scan(rel) call from _bt_first does. Though this
function basically updates local structures and sends the count to stat
collector via messages we can have a function which will instead modify the
shared state using the progress API committed recently.

Thank you,

Rahila Syed

Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

[Ashutosh Bapat]

On Mon, Mar 14, 2016 at 8:21 AM, Tom Lane  wrote:

> Etsuro Fujita  writes:
> > On 2016/03/13 4:46, Andres Freund wrote:
> >> ... The difference apears to be the
> >> check that's now in build_simple_rel() - there was nothing hitting the
> >> user mapping code before for file_fdw.
>
> > Exactly.
>
> > I'm not sure it's worth complicating the code to keep that behavior, so
> > I'd vote for adding the change notice to 9.6 release notes or something
> > like that in addition to updating file-fdw.sgml.
>
> Perhaps it would be useful for an FDW to be able to specify that user
> mappings are meaningless to it?  And then bypass this check for such FDWs?
>

In such a case, whether FDWs be given chance to push down joins? I guess
the answer is yes, but wanted to confirm.


>
> I'm not really sold on enforcing that people create meaningless user
> mappings.  For one thing, they're likely to be sloppy about it, which
> could lead to latent security problems if the FDW later acquires a
> concept that user mappings mean something.
>
>
Hmm. Should we let FDW handler set a boolean in fdwroutine specifying
whether the core code should bother about user mapping or not. This way the
author of FDW decides whether s/he is going to write code that uses user
mappings or not. A small problem with that is PG documentation describes
fdwroutine as a structure holding function pointers and now it will store a
boolean variable. But I think that can be managed either by having this
option as a function pointer returning boolean or changing the
documentation.

Other problem is what should we do when a user creates or has an existing
user mapping for an FDW which specifies that user mapping is meaningless to
it. Should we allow the user mapping to be created but ignore it or do not
allow it to be created? In the later case, what should happen to the
existing user mappings?

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

Re: [HACKERS] Add schema-qualified relnames in constraint error messages.

[Shulgin, Oleksandr]

On Wed, Feb 10, 2016 at 12:33 AM, Daniel Verite 
wrote:

> Shulgin, Oleksandr wrote:
>
> > Most importantly, I'd like to learn of better options than storing the
> > whole last_result in psql's pset structure.
>
> I guess that you could, each time a query fails, gather silently the
> result of \errverbose, store it in a buffer, discard the PGresult,
> and in case the user does \errverbose before running another query,
> output what was in that buffer.
>

That's a neat idea.  I also think that we could only store last PGresult
when the query fails actually and discard it otherwise: the PGresult
holding only the error doesn't occupy too much space.

What I dislike about this POC is all the disruption in libpq, to be
honest.  It would be much neater if we could form the verbose message every
time and let the client decide where to cut it.  Maybe a bit "too clever"
would be to put a \0 char between short message and it's verbose
continuation.  The client could then reach the verbose part like this
(assuming that libpq did put a verbose part there): msg + strlen(msg) + 1.

--
Alex

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

[Amit Langote]

Hi,

Thanks for taking a look at the patch.

On Mon, Mar 14, 2016 at 6:55 PM, Rahila Syed  wrote:
> Hello,
>
> While I am still looking at this WIP patch, I had one suggestion.
>
> Instead of making changes in the index AM API can we have a call to update
> the shared state using pgstat_progress* API
>
> directly from specific index level code?
>
> Like  pgstat_count_index_scan(rel) call from _bt_first does. Though this
> function basically updates local structures and sends the count to stat
> collector via messages we can have a function which will instead modify the
> shared state using the progress API committed recently.

I chose the callback approach because we need to count the index
blocks within the context of a given vacuum run.  For example, as
proposed, progress_callback_state (in this case, a pointer to the
LVRelStats struct for a given vacuum run) keeps the block count for a
given index vacuum run.  It is reset when next index vacuuming round
starts.  Also, remember that the count is across all indexes.

If we call pgstat_progress API directly from within AM, what I just
described above seems difficult to achieve modularly. But maybe, I'm
missing something.

Aside from whether we should use one of the above two methods, I think
we also have to figure out, for each AM, how to count correctly
considering non-linearity (tree traversal, recursion and such) of most
AMs' vacuum scans.

Thanks,
Amit


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

Re: [HACKERS] Sanity checking for ./configure options?

[Alex Shulgin]

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:not tested

Looks good to me.  It only allows valid number between 1 and 65535, disallows 
leading zero, empty string, or non-digit chars.  Error messages looks good.

Marking this Ready for Committer.

--
Alex


The new status of this patch is: Ready for Committer

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

Re: [HACKERS] psql metaqueries with \gexec

[Corey Huinker]

>
>
> I'm getting a warning from this patch:
>
> 1 warning generated.
>

Fixed that one.


(note that I'm using CC='ccache clang -Qunused-arguments
> -fcolor-diagnostics')
>
> for (r = 0; r < nrows; r++)
>> {
>> for (c = 0; c < ncolumns; c++)
>> {
>>
> etc...
>
> Normally we don't use gratuitous {'s, and I don't think it's helping
> anything in this case. But I'll let whoever commits this decide.
>


Good to know in the future. I can remove or leave to the committer.


> diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
>> index 5f27120..0f87f29 100644
>> --- a/src/bin/psql/tab-complete.c
>> +++ b/src/bin/psql/tab-complete.c
>> @@ -1280,8 +1280,8 @@ psql_completion(const char *text, int start, int
>> end)
>> "\\dm", "\\dn", "\\do", "\\dO", "\\dp", "\\drds", "\\ds",
>> "\\dS",
>> "\\dt", "\\dT", "\\dv", "\\du", "\\dx", "\\dy",
>> "\\e", "\\echo", "\\ef", "\\encoding", "\\ev",
>> -   "\\f", "\\g", "\\gset", "\\h", "\\help", "\\H", "\\i",
>> "\\ir", "\\l",
>> -   "\\lo_import", "\\lo_export", "\\lo_list", "\\lo_unlink",
>> +   "\\f", "\\g", "\\gexec", "\\gset", "\\h", "\\help",
>> "\\H", "\\i", "\\ir",
>> +   "\\l", "\\lo_import", "\\lo_export", "\\lo_list",
>> "\\lo_unlink",
>>
>
> FWIW, it's generally better to leave that kind of re-wrapping to the next
> pg_indent run.
>

Good to know in the future. Not much point in undoing it now, I suppose.


>
> I added tests for ON_ERROR_STOP. New patch attached.
>

I was wondering if ON_ERROR_STOP tests were verbotten because you only get
to kick the tires on one feature...


>
> The patch still needs to document this feature in the psql docs (and maybe
> the manpage? not sure how that's generated...)


doc/src/sgml/ref/psql-ref.sgml is the source for both html and man pagers.

I'm on it. I didn't expect the name "gexec" to survive first contact with
the community.

Patch attached. Changes are thus:
- proper assignment of success var
- added documentation to psql manpage/html with examples pulled from
regression tests.

Not changed are:
- exuberant braces, can remove if someone wants me to
- attempt at line-wrappng the enumerated slash commands, leave that to
pg_indent
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 8a85804..acb0eb7 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1753,6 +1753,91 @@ Tue Oct 26 21:40:57 CEST 1999
   
 
   
+\gexec
+
+
+
+ Sends the current query input buffer to the server and treats
+ every column of every row of query output (if any) as a separate
+ SQL statement to be immediately executed. For example:
+
+= SELECT 'select 1 as ones', 'select x.y, x.y*2 as double from 
generate_series(1,4) as x(y)'
+- UNION ALL
+- SELECT 'select true as is_true', 'select ''2000-01-01''::date 
as party_over'
+- \gexec
+ones
+
+   1
+(1 row)
+
+y double
+- --
+1  2
+2  4
+3  6
+4  8
+(4 rows)
+
+is_true
+---
+t
+(1 row)
+
+party_over
+--
+01-01-2000
+(1 row)
+
+
+
+The secondary queries are executed in top-to-bottom, left-to-right 
order, so the command
+above is the equivalent of:
+
+= select 1 as ones;
+= select x.y, x.y*2 as double from generate_series(1,4) as 
x(y);
+= select true as is_true;
+= select '2000-01-01'::date as party_over;
+
+
+
+If the query returns no rows, no error is raised, but no secondary 
query 
+is executed, either.
+
+=%gt; SELECT 'select 1 as expect_zero_rows ' where false
+- \gexec
+
+
+
+
+Results that are not valid SQL will of course fail, and the execution 
of further
+secondary statements is subject to the current \ON_ERROR_STOP setting.
+
+= SELECT 'a', 'select 1', 'b'
+- \gexec
+ERROR:  syntax error at or near "a"
+LINE 1: a
+^
+?column?
+
+   1
+(1 row)
+ERROR:  syntax error at or near "b"
+LINE 1: b
+^
+= \set ON_ERROR_STOP 1
+= SELECT 'a', 'select 1', 'b'
+- \gexec
+ERROR:  syntax error at or near "a"
+LINE 1: a
+^
+
+
+The results of the main query are sent directly to the server, without
+evaluation by psql. Therefore, they cannot contain psql vars or \ 
commands.
+
+
+  
+  
 \gset [ prefix ]
 
 
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 9750a5b..5ca769f 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -849,6 +849,13 @@ exec_command(const char *cmd,
status = PSQL_CMD_SEND;
}
 
+   /* \gexec -- send query and treat every result cell as a query to be 
executed */
+   else if (strcmp(cmd, "gexec") == 0)
+   {
+   pset.gexec_flag = true;
+   status = PSQL_CMD_SEND;
+   }
+
/* \gset 

Re: [HACKERS] remove wal_level archive

[David Steele]

On 3/11/16 1:29 PM, David Steele wrote:


Unless anyone has objections I would like to mark this 'ready for
committer'.


This patch is now ready for committer.

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


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

Re: [HACKERS] Batch update of indexes

[David Steele]

Hi Konstantin,

On 2/3/16 11:47 AM, Konstantin Knizhnik wrote:

Attached please find patch for "ALTER INDEX ... WHERE ..." clause.
It is now able to handle all three possible situations:
1. Making index partial (add WHERE condition to the ordinary index)
2. Extend partial index range (less restricted index predicate)
3. Arbitrary change of partial index predicate

In case 2) new records are added to the index.
In other two cases index is completely reconstructed.

This patch includes src/bin/insbench utility for testing insert
performance. It can be easily excluded from the patch to reduce it size.
Also it is better to apply this patch together with "index-only scans
with partial indexes" patch:


This patch no longer applies on master:

$ git apply ../other/alter-index.patch
../other/alter-index.patch:27: trailing whitespace.
static void
../other/alter-index.patch:62: indent with spaces.
SPIPlanPtr plan;
../other/alter-index.patch:63: indent with spaces.
Portal portal;
../other/alter-index.patch:90: trailing whitespace.

../other/alter-index.patch:99: trailing whitespace.

error: patch failed: src/test/regress/expected/aggregates.out:831
error: src/test/regress/expected/aggregates.out: patch does not apply

Please provide a new patch for review.  Meanwhile I am marking this 
"waiting on author".


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


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

Re: [HACKERS] pgbench - allow backslash-continuations in custom scripts

[David Steele]

On 2/18/16 6:54 AM, Kyotaro HORIGUCHI wrote:


First, I rebased the previous patch set and merged three of
them. Now they are of three patches.


1. Making SQL parser part of psqlscan independent from psql.

Moved psql's baskslsh command stuff out of original psqlscan.l
and some psql stuff the parser directly looked are used via a
set of callback functions, which can be all NULL for usages
from other than psql.

2. Making pgbench to use the new psqlscan parser.

3. Changing the way to hold SQL/META commands from array to
 linked list.

 The #2 introduced linked list to store SQL multistatement but
 immediately the caller moves the elements into an array. This
 patch totally changes the way to linked list.


Any takers to review this updated patch?

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


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

Re: [HACKERS] Re: PATCH: Split stats file per database WAS: autovacuum stress-testing our system

[Tomas Vondra]

Hi,

On 03/14/2016 07:14 AM, Noah Misch wrote:

[Aside: your new mail editor is rewrapping lines in quoted material, and the
result is messy.  I have rerewrapped one paragraph below.]


Thanks, I've noticed that too. I've been testing Evolution in the past 
few days, and apparently the line wrapping algorithm is broken. I've 
switched back to Thunderbird, so hopefully that'll fix it.




On Mon, Mar 14, 2016 at 02:00:03AM +0100, Tomas Vondra wrote:

On Sun, 2016-03-13 at 18:46 -0400, Noah Misch wrote:

I've not attempted to study the behavior on slow hardware.  Instead, my
report used stat-coalesce-v1.patch[1] to simulate slow writes.  (That
diagnostic patch no longer applies cleanly, so I'm attaching a rebased
version.  I've changed the patch name from "stat-coalesce" to
"slow-stat-simulate" to more-clearly distinguish it from the
"pgstat-coalesce" patch.)  Problems remain after applying your patch;
consider "VACUUM pg_am" behavior:

9.2 w/ stat-coalesce-v1.patch:
  VACUUM returns in 3s, stats collector writes each file 1x over 3s
HEAD w/ slow-stat-simulate-v2.patch:
  VACUUM returns in 3s, stats collector writes each file 5x over 15s
HEAD w/ slow-stat-simulate-v2.patch and your patch:
  VACUUM returns in 10s, stats collector writes no files over 10s


Oh damn, the timestamp comparison in pgstat_recv_inquiry should be in
the opposite direction. After fixing that "VACUUM pg_am" completes in 3
seconds and writes each file just once.


That fixes things.  "make check" passes under an 8s stats write time.


OK, good.




--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -4836,6 +4836,20 @@ pgstat_recv_inquiry(PgStat_MsgInquiry *msg, int len)
}

/*
+* Ignore requests that are already resolved by the last write.
+*
+* We discard the queue of requests at the end of 
pgstat_write_statsfiles(),
+* so the requests already waiting on the UDP socket at that moment 
can't
+* be discarded in the previous loop.
+*
+* XXX Maybe this should also care about the clock skew, just like the
+* block a few lines down.


Yes, it should.  (The problem is large (>~100s), backward clock resets, not
skew.)  A clock reset causing "msg->clock_time < dbentry->stats_timestamp"
will usually also cause "msg->cutoff_time < dbentry->stats_timestamp".  Such
cases need the correction a few lines down.


I'll look into that. I have to admit I have a hard time reasoning about 
the code handling clock skew, so it might take some time, though.




The other thing needed here is to look through and update comments about
last_statrequests.  For example, this loop is dead code due to us writing
files as soon as we receive one inquiry:

/*
 * Find the last write request for this DB.  If it's older than the
 * request's cutoff time, update it; otherwise there's nothing to do.
 *
 * Note that if a request is found, we return early and skip the below
 * check for clock skew.  This is okay, since the only way for a DB
 * request to be present in the list is that we have been here since the
 * last write round.
 */
slist_foreach(iter, _statrequests) ...

I'm okay keeping the dead code for future flexibility, but the comment should
reflect that.


Yes, that's another thing that I'd like to look into. Essentially the 
problem is that we always process the inquiries one by one, so we never 
actually see a list with more than a single element. Correct?


I think the best way to address that is to peek is to first check how 
much data is in the UDP queue, and then fetching all of that before 
actually doing the writes. Peeking at the number of requests first (or 
even some reasonable hard-coded limit) should should prevent starving 
the inquirers in case of a steady stream or inquiries.


regards
Tomas

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


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

Re: [HACKERS] [WIP] speeding up GIN build with parallel workers

[David Steele]

On 2/18/16 10:10 AM, Constantin S. Pan wrote:

On Wed, 17 Feb 2016 23:01:47 +0300
Oleg Bartunov  wrote:


My feedback is (Mac OS X 10.11.3)

set gin_parallel_workers=2;
create index message_body_idx on messages using gin(body_tsvector);
LOG:  worker process: parallel worker for PID 5689 (PID 6906) was
terminated by signal 11: Segmentation fault


Fixed this, try the new patch. The bug was in incorrect handling
of some GIN categories.


Oleg, it looks like Constantin has updated to patch to address the issue 
you were seeing.  Do you have time to retest and review?


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


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

Re: [HACKERS] PATCH: use foreign keys to improve join estimates v1

[David Steele]

Hi Thomas,

On 2/24/16 11:21 AM, Tomas Vondra wrote:


Overall, I still believe the FK patch is a clear improvement of the
current status - while it's true it does not improve all possible cases
and there's a room for additional improvements (like handling multiple
candidate FK constraints), it should not make existing estimates worse.


The latest version of this patch does not apply:

$ git apply ../other/0001-estimation-with-fkeys-v2.patch
../other/0001-estimation-with-fkeys-v2.patch:748: trailing whitespace.

error: patch failed: src/backend/optimizer/util/plancat.c:27
error: src/backend/optimizer/util/plancat.c: patch does not apply
error: patch failed: src/include/nodes/relation.h:468
error: src/include/nodes/relation.h: patch does not apply

David's most recent version also does not apply:

$ git apply ../other/estimation-with-fkeys-v2_davidv4.patch
../other/estimation-with-fkeys-v2_davidv4.patch:517: trailing whitespace.

error: patch failed: src/backend/optimizer/util/plancat.c:27
error: src/backend/optimizer/util/plancat.c: patch does not apply
error: patch failed: src/include/nodes/relation.h:472
error: src/include/nodes/relation.h: patch does not apply

I don't think it would be clear to any reviewer which patch to apply 
even if they were working.  I'm marking this "waiting for author".


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


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

Re: [HACKERS] [WIP] Effective storage of duplicates in B-tree index.

[David Steele]

Hi Anastasia,

On 2/18/16 12:29 PM, Anastasia Lubennikova wrote:

18.02.2016 20:18, Anastasia Lubennikova:

04.02.2016 20:16, Peter Geoghegan:

On Fri, Jan 29, 2016 at 8:50 AM, Anastasia Lubennikova
  wrote:

I fixed it in the new version (attached).


Thank you for the review.
At last, there is a new patch version 3.0. After some refactoring it
looks much better.
I described all details of the compression in this document
https://goo.gl/50O8Q0 (the same text without pictures is attached in
btc_readme_1.0.txt).
Consider it as a rough copy of readme. It contains some notes about
tricky moments of implementation and questions about future work.
Please don't hesitate to comment it.


Sorry, previous patch was dirty. Hotfix is attached.


This looks like an extremely valuable optimization for btree indexes but 
unfortunately it is not getting a lot of attention.  It still applies 
cleanly for anyone interested in reviewing.


It's not clear to me that you answered all of Peter's questions in [1]. 
 I understand that you've provided a README but it may not be clear if 
the answers are in there (and where).


Also, at the end of the README it says:

13. Xlog. TODO.

Does that mean the patch is not yet complete?

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

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



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

Re: [HACKERS] Prepared Statement support for Parallel query

[Amit Kapila]

On Fri, Feb 26, 2016 at 4:37 PM, Robert Haas  wrote:
>
>
> And, I'm going to revert this part.  If you'd run the regression tests
> under force_parallel_mode=regress, max_parallel_degree>0, you would
> have noticed that this part breaks it, because of CREATE TABLE ... AS
> EXECUTE.
>

I have looked into this issue and found that the reason for the failure is
that in force_parallel_mode=regress, we enable parallel mode restrictions
even if the parallel plan is not choosen as part of below code in
standard_planner()

if (force_parallel_mode == FORCE_PARALLEL_OFF || !glob->parallelModeOK)

{

glob->parallelModeNeeded = false;

glob->wholePlanParallelSafe = false; /* either false or don't care */

}

else

{

glob->parallelModeNeeded = true;

glob->wholePlanParallelSafe =

!has_parallel_hazard((Node *) parse, false);

}


The failure cases fall into that category, basically wholePlanParallelSafe
will be false, but parallelModeNeeded will be true which will enable
parallel mode restrictions even though the plan won't contain Gather node.
I think if we want to operate in above way for testing purpose, then we
need to force during execution that statements for non read-only operations
should not enter into parallel mode similar to what we are doing for
non-zero tuple count case.  Attached patch fixes the problem.


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


prepared_stmt_parallel_query_v2.patch
Description: Binary data

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

[HACKERS] Re: [PATCH] Integer overflow in timestamp[tz]_part() and date/time boundaries check

[David Steele]

On 2/25/16 4:44 PM, Vitaly Burovoy wrote:


Added to the commitfest 2016-03.

[CF] https://commitfest.postgresql.org/9/540/


This looks like a fairly straight-forward bug fix (the size of the patch 
is deceptive because there a lot of new tests included).  It applies 
cleanly.


Anastasia, I see you have signed up to review.  Do you have an idea when 
you will get the chance to do that?


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


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

Re: [HACKERS] unexpected result from to_tsvector

[Shulgin, Oleksandr]

On Mon, Mar 7, 2016 at 10:46 PM, Artur Zakirov 
wrote:

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


Hm...  now that doesn't look all that consistent to me (after applying the
patch):

=# select ts_debug('simple', 'a...@123-yyy.zzz');
 ts_debug
---
 (email,"Email address",a...@123-yyy.zzz,{simple},simple,{a...@123-yyy.zzz})
(1 row)

But:

=# select ts_debug('simple', 'aaa@123_yyy.zzz');
ts_debug
-
 (asciiword,"Word, all ASCII",aaa,{simple},simple,{aaa})
 (blank,"Space symbols",@,{},,)
 (uint,"Unsigned integer",123,{simple},simple,{123})
 (blank,"Space symbols",_,{},,)
 (host,Host,yyy.zzz,{simple},simple,{yyy.zzz})
(5 rows)

One can also see that if we only keep the domain name, the result is
similar:

=# select ts_debug('simple', '123-yyy.zzz');
   ts_debug
---
 (host,Host,123-yyy.zzz,{simple},simple,{123-yyy.zzz})
(1 row)

=# select ts_debug('simple', '123_yyy.zzz');
  ts_debug
-
 (uint,"Unsigned integer",123,{simple},simple,{123})
 (blank,"Space symbols",_,{},,)
 (host,Host,yyy.zzz,{simple},simple,{yyy.zzz})
(3 rows)

But, this only has to do with 123 being recognized as a number, not with
the underscore:

=# select ts_debug('simple', 'abc_yyy.zzz');
   ts_debug
---
 (host,Host,abc_yyy.zzz,{simple},simple,{abc_yyy.zzz})
(1 row)

=# select ts_debug('simple', '1abc_yyy.zzz');
   ts_debug
---
 (host,Host,1abc_yyy.zzz,{simple},simple,{1abc_yyy.zzz})
(1 row)

In fact, the 123-yyy.zzz domain is not valid either according to the RFC
(subdomain can't start with a digit), but since we already allow it, should
we not allow 123_yyy.zzz to be recognized as a Host?  Then why not
recognize aaa@123_yyy.zzz as an email address?

Another option is to prohibit underscore in recognized host names, but this
has more breakage potential IMO.

--
Alex

Re: [HACKERS] amcheck (B-Tree integrity checking tool)

[Robert Haas]

On Fri, Mar 11, 2016 at 7:45 PM, Peter Geoghegan  wrote:
> On Fri, Mar 11, 2016 at 4:30 PM, Jim Nasby  wrote:
>> Right, but you still have the option to enable them if you don't want to
>> swamp your IO system. That's why CIC obeys it too. If I was running a
>> consistency check on a production system I'd certainly want the option to
>> throttle it. Without that option, I don't see running this on production
>> systems as being an option. If that's not a goal then fine, but if it is a
>> goal I think it needs to be there.
>>
>> Isn't it just a few extra lines of code to support it?
>
> I see your point.
>
> I'll add that if people like the interface you propose. (Overloading
> the VACUUM cost delay functions to cause a delay for amcheck
> functions, too). Note that the functions already use an appropriate
> buffer access strategy (it avoids blowing shared_buffers, much like
> VACUUM itself).

I don't particularly like that interface.  I also suggest that it
would be better to leave throttling to a future commit, and focus on
getting the basic feature in first.

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


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

Re: [HACKERS] WIP: Upper planner pathification

[Robert Haas]

On Mon, Mar 14, 2016 at 1:37 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Mon, Mar 14, 2016 at 1:04 PM, Tom Lane  wrote:
>>> It would be better if we invent an FDW callback that's meant to be
>>> invoked at this stage, but only call it for FDW(s) actively involved
>>> in the query.  I'm not sure exactly what that ought to look like though.
>>> Maybe only call the FDW identified as possible owner of the topmost
>>> scan/join relation?
>
>> I think in the short term that's as well as we're going to do, so +1.
>> In the long run, I'm interested in making FDWs be able to optimize
>> queries like foreigntab JOIN localtab ON foreigntab.x = localtab.x
>> (e.g. by copying localtab to the remote side when it's small); but
>> that will require revisiting some old decisions, too.
>
> Yeah.  An alternative definition that would support that would be to
> call the upper-path-providing callback for each FDW that's responsible
> for any base relation of the query.  But I think that that would often
> lead to a lot of redundant/wasted computation, and it's not clear to
> me that we can support such cases without other changes as well.

Sure, that's fine with me.  Are you going to go make these changes now?

Eventually, we might just support a configurable flag on FDWs where
FDWs that want to do this sort of thing can request callbacks on every
join and every upper rel in the query.  But that can wait.

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


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

Re: [HACKERS] Reworks of CustomScan serialization/deserialization

[Robert Haas]

On Sun, Mar 13, 2016 at 9:53 PM, Kouhei Kaigai  wrote:
> OK, I split the previous small patch into two tiny patches.
> The one is bugfix around max length of the extnodename.
> The other replaces Assert() by ereport() according to the upthread discussion.

Committed, except that (1) I replaced ereport() with elog(), because I
can't see making translators care about this message; and (2) I
reworded the error message a bit.

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


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

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

[David Steele]

On 2/26/16 11:37 PM, Amit Kapila wrote:


On Sat, Feb 27, 2016 at 10:03 AM, Amit Kapila 

Re: [HACKERS] Obsolete comment in postgres_fdw.c

[Robert Haas]

On Mon, Mar 14, 2016 at 4:31 AM, Etsuro Fujita
 wrote:
>> It was left there intentionally to document all the conditions in one
>> place (some from the core and some from the FDW itself), for a ready
>> reference. In case tomorrow core thinks that matching user mapping is
>> not required, postgres_fdw would still require it to be incorporated.
>
> Thank you for the explanation!  I understand the reason, but that seems
> confusing to me.

Agreed.

>> In addition, I'd like to update some related comments in
>> src/include/nodes/relation.h and
>> src/backend/optimizer/path/joinpath.c.
>>
>>
>> Those look fine. Sorry for missing those in the commit and thanks for
>> providing a patch for the same.
>
> No problem.

Committed.

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


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

Re: [HACKERS] [PATCH] Use correct types and limits for PL/Perl SPI query results

[Tom Lane]

ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Tom Lane  writes:
>> I thought about using UV where feasible, but it was not clear to me
>> whether unsigned numbers behave semantically differently from signed ones
>> in Perl.  If they do, the change you suggest would create a small semantic
>> change in the behavior of code using spi_exec_query.  I'm not sure it's
>> worth taking any risk of that, considering we already discourage people
>> from using spi_exec_query for large results.

> IVs and UVs are semantically equivalent, and Perl will automatically
> convert between them (and NVs) as necessary, i.e. when crossing
> IV_MAX/UV_MAX/IV_MIN.

OK, thanks for the authoritative statement.

>> I don't much like having such hard-wired knowledge about Perl versions
>> in our code.  Is there a way to identify this change other than
>> #if PERL_BCDVERSION >= 0x5019004 ?

> There isn't, unfortunately.

Sigh ... it was worth asking anyway.

>>> 3) To be able to actually return such result sets from sp_exec_query(),
>>> I had to change the repalloc() in spi_printtup() to repalloc_huge().

>> Hmm, good point.  I wonder if there are any hazards to doing that.

> One hazard would be that people not heeding the warning in
> spi_exec_query will get a visit from the OOM killer (or death by swap)
> instead of a friendly "invalid memory alloc request size".

Yeah.  But there are plenty of other ways to drive a backend into swap
hell, and the whole point of this change is to eliminate arbitrary
boundaries on query result size.  So let's do it.

Thanks for the patch and discussion!

regards, tom lane


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

Re: [HACKERS] Proposal: "Causal reads" mode for load balancing reads without stale data

[Thomas Munro]

On Tue, Mar 15, 2016 at 6:58 AM, Robert Haas  wrote:
> On Sun, Mar 13, 2016 at 11:50 PM, Thomas Munro
>  wrote:
>> The last patches I posted don't apply today due to changes in master,
>> so here's a freshly merged patch series.
>
> +from the current synchronous stanbyindicates it has received the
>
> Uh, no.

Oops, thanks, fixed.  I'll wait for some more feedback or a conflict
with master before sending a new version.

> -SyncRepWaitForLSN(gxact->prepare_end_lsn);
> +{
> +/*
> + * Don't wait for the prepare to be applied remotely in remote_apply
> + * mode, just wait for it to be flushed to the WAL.  We will wait for
> + * apply when the transaction eventuallly commits or aborts.
> + */
> +if (synchronous_commit == SYNCHRONOUS_COMMIT_REMOTE_APPLY)
> +assign_synchronous_commit(SYNCHRONOUS_COMMIT_REMOTE_FLUSH, NULL);
> +
> +SyncRepWaitForLSN(gxact->prepare_end_lsn);
> +
> +if (synchronous_commit == SYNCHRONOUS_COMMIT_REMOTE_APPLY)
> +assign_synchronous_commit(SYNCHRONOUS_COMMIT_REMOTE_APPLY, NULL);
> +}
>
> What's with the extra block?

Yeah, that's silly, thanks.  Tidied up for the next version.

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


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

Re: [HACKERS] [PATCH] Obsolete wording in PL/Perl comment

[Tom Lane]

ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> The comment in hv_store_string() says that negative key length to
> hv_store() for UTF-8 is not documented, and mentions that 5.6 doesn't
> track UTF-8-ness of keys.  However, the negative length convention has
> been documented since 5.16, and 5.6 is no longer supported.  The
> attached patch updates the comment to reflect this.

Pushed, thanks.

regards, tom lane


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

Re: [HACKERS] [PATCH] we have added support for box type in SP-GiST index

[David Steele]

On 2/15/16 10:29 AM, Teodor Sigaev wrote:


It's very pity but author is not able to continue work on this patch,
and I would like to raise this flag.

I'd like to add some comments about patches:

traversalValue patch adds arbitrary value assoсiated with branch in
SP-GiST tree walk. Unlike to recostructedValue it could be just pointer,
it havn't to be a regular pgsql type. Also, it could be used independly
from reconstructedValue. This patch is used both following attached
patches.

range patch just switchs using reconstructedValue to traversalValue in
range opclasses. reconstructedValue was used just because it was an
acceptable workaround in case of range type. Range opclase stores a full
value in leafs and doesn't need to use reconstructedValue to return
tuple in index only scan.
See http://www.postgresql.org/message-id/5399.1343512...@sss.pgh.pa.us

q4d patch implements index over boxes using SP-GiST. Basic idea was an
observation, range opclass thinks about one-dimensional ranges as 2D
points.
Following this idea, we can think that 2D box (what is 2 points or 4
numbers) could represent a 4D point. We hoped that this approach will be
much more effective than traditional R-Tree in case of many overlaps in
box's collection.
Performance results are coming shortly.


It appears that the issues raised in this thread have been addressed but 
the patch still has not gone though a real review.


Anybody out there willing to take a crack at a review?  All three 
patches apply (with whitespace issues).


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


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

Re: [HACKERS] dealing with extension dependencies that aren't quite 'e'

[David Steele]

Hi Abhijit,

On 3/1/16 8:36 AM, Jim Nasby wrote:

On 2/29/16 10:33 PM, Abhijit Menon-Sen wrote:

>Given the audience for this, I think it'd probably be OK to just
>provide a function that does this, instead of DDL.

That seems like a promising idea. Can you suggest some possible usage?


pg_extension_dependency( regextension, any )

where "any" would be all the other reg* types. That should be a lot less
work to code up than messing with the grammar.


So where are we on this now?  Were you going to implement this as a 
function the way Jim suggested?


Alexander, you are signed up to review.  Any opinion on which course is 
best?


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


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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

[Tom Lane]

Robert Haas  writes:
> On Sun, Mar 13, 2016 at 10:51 PM, Tom Lane  wrote:
>> I'm not really sold on enforcing that people create meaningless user
>> mappings.  For one thing, they're likely to be sloppy about it, which
>> could lead to latent security problems if the FDW later acquires a
>> concept that user mappings mean something.

> I think we should just fix build_simple_rel() so that it doesn't fail
> if there is no user mapping.  It can just set rel->umid to InvalidOid
> in that case, and if necessary we can adjust the code elsewhere to
> tolerate that.  This wasn't an intentional behavior change, and I
> think we should just put things back to the way they were.

So, allow rel->umid to be InvalidOid if there's no user mapping, and
when dealing with a join, allow combining when both sides have InvalidOid?

regards, tom lane


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

Re: [HACKERS] [PATCH] Use correct types and limits for PL/Perl SPI query results

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
>> 1) Perl's integers are at least pointer-sized and either signed or
>>unsigned, so can potentially hold up to 2⁶⁴-1. Floating point numbers
>>can also be larger than double (up to 128bit), allowing for exact
>>representation of integers beyond 2⁵³.  Hence, adjust the setting of
>>the "processed" hash item to use UV_MAX for the limit and (NV) or
>>(UV) for the casts.
>
> I thought about using UV where feasible, but it was not clear to me
> whether unsigned numbers behave semantically differently from signed ones
> in Perl.  If they do, the change you suggest would create a small semantic
> change in the behavior of code using spi_exec_query.  I'm not sure it's
> worth taking any risk of that, considering we already discourage people
> from using spi_exec_query for large results.

IVs and UVs are semantically equivalent, and Perl will automatically
convert between them (and NVs) as necessary, i.e. when crossing
IV_MAX/UV_MAX/IV_MIN.

>> 2) Perl 5.20 and later use SSize_t for array indices, so can cope with
>>up to SSize_t_max items in an array (if you have the memory).
>
> I don't much like having such hard-wired knowledge about Perl versions
> in our code.  Is there a way to identify this change other than
> #if PERL_BCDVERSION >= 0x5019004 ?

There isn't, unfortunately.  I could add e.g. AVidx_t and _MAX defines,
but that wouldn't be available until 5.26 (May 2017) at the earliest,
since full code freeze for May's 5.24 is next week.

>> 3) To be able to actually return such result sets from sp_exec_query(),
>>I had to change the repalloc() in spi_printtup() to repalloc_huge().
>
> Hmm, good point.  I wonder if there are any hazards to doing that.

One hazard would be that people not heeding the warning in
spi_exec_query will get a visit from the OOM killer (or death by swap)
instead of a friendly "invalid memory alloc request size".

>   regards, tom lane

ilmari

-- 
- Twitter seems more influential [than blogs] in the 'gets reported in
  the mainstream press' sense at least.   - Matt McLeod
- That'd be because the content of a tweet is easier to condense down
  to a mainstream media article.  - Calle Dybedahl


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

Re: [HACKERS] Prepared Statement support for Parallel query

[Robert Haas]

On Mon, Mar 14, 2016 at 9:18 AM, Amit Kapila  wrote:
> On Fri, Feb 26, 2016 at 4:37 PM, Robert Haas  wrote:
>> And, I'm going to revert this part.  If you'd run the regression tests
>> under force_parallel_mode=regress, max_parallel_degree>0, you would
>> have noticed that this part breaks it, because of CREATE TABLE ... AS
>> EXECUTE.
>>
>
> I have looked into this issue and found that the reason for the failure is
> that in force_parallel_mode=regress, we enable parallel mode restrictions
> even if the parallel plan is not choosen as part of below code in
> standard_planner()
>
> if (force_parallel_mode == FORCE_PARALLEL_OFF || !glob->parallelModeOK)
>
> {
>
> glob->parallelModeNeeded = false;
>
> glob->wholePlanParallelSafe = false; /* either false or don't care */
>
> }
>
> else
>
> {
>
> glob->parallelModeNeeded = true;
>
> glob->wholePlanParallelSafe =
>
> !has_parallel_hazard((Node *) parse, false);
>
> }
>
>
> The failure cases fall into that category, basically wholePlanParallelSafe
> will be false, but parallelModeNeeded will be true which will enable
> parallel mode restrictions even though the plan won't contain Gather node.
> I think if we want to operate in above way for testing purpose, then we need
> to force during execution that statements for non read-only operations
> should not enter into parallel mode similar to what we are doing for
> non-zero tuple count case.  Attached patch fixes the problem.

This seems like a really ugly fix.  It might be possible to come up
with a fix along these lines, but I don't have much confidence in the
specific new test you've injected into executePlan().  Broadly, any
change of this type implicitly changes the contract between
executePlan() and the planner infrastructure - the planner can now
legally generate parallel plans in some cases where that would
previously have been unacceptable.  But I'm not in a hurry to rethink
where we've drawn the line there for 9.6.  Let's punt this issue for
now and come back to it in a future release.

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


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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

[Robert Haas]

On Mon, Mar 14, 2016 at 1:05 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Sun, Mar 13, 2016 at 10:51 PM, Tom Lane  wrote:
>>> I'm not really sold on enforcing that people create meaningless user
>>> mappings.  For one thing, they're likely to be sloppy about it, which
>>> could lead to latent security problems if the FDW later acquires a
>>> concept that user mappings mean something.
>
>> I think we should just fix build_simple_rel() so that it doesn't fail
>> if there is no user mapping.  It can just set rel->umid to InvalidOid
>> in that case, and if necessary we can adjust the code elsewhere to
>> tolerate that.  This wasn't an intentional behavior change, and I
>> think we should just put things back to the way they were.
>
> So, allow rel->umid to be InvalidOid if there's no user mapping, and
> when dealing with a join, allow combining when both sides have InvalidOid?

Exactly.  And we should make sure (possibly with a regression test)
that postgres_fdw handles that case correctly - i.e. with the right
error.

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


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

Re: [HACKERS] WIP: Upper planner pathification

[Robert Haas]

On Mon, Mar 14, 2016 at 1:04 PM, Tom Lane  wrote:
> Petr Jelinek  writes:
>> On 14/03/16 02:43, Kouhei Kaigai wrote:
>>> Even though I couldn't check the new planner implementation entirely,
>>> it seems to be the points below are good candidate to inject CustomPath
>>> (and potentially ForeignScan).
>>>
>>> - create_grouping_paths
>>> - create_window_paths
>>> - create_distinct_paths
>>> - create_ordered_paths
>>> - just below of the create_modifytable_path
>>> (may be valuable for foreign-update pushdown)
>
>> To me that seems too low inside the planning tree, perhaps adding it
>> just to the subquery_planner before SS_identify_outer_params would be
>> better, that's the place where you see the path for the whole (sub)query
>> so you can search and modify what you need from there.
>
> I don't like either of those too much.  The main thing I've noticed over
> the past few days is that you can't readily generate custom upper-level
> Paths unless you know what PathTarget grouping_planner is expecting each
> level to produce.  So what I was toying with doing is (1) having
> grouping_planner put all those targets into the PlannerInfo, perhaps
> in an array indexed by UpperRelationKind; and (2) adding a hook call
> immediately after those targets are computed, say right before
> the create_grouping_paths() call (approximately planner.c:1738
> in HEAD).  It should be sufficient to have one hook there since
> you can inject Paths into any of the upper relations at that point;
> moreover, that's late enough that you shouldn't have to recompute
> anything you figured out during scan/join planning.

That is a nice set of characteristics for a hook.

> Now, a simple hook function is probably about the best we can offer
> CustomScan providers, since we don't really know what they're going
> to do.  But I'm pretty unenthused about telling FDW authors to use
> such a hook, because generally speaking they're simply going to waste
> cycles finding out that they aren't involved in a given query.
> It would be better if we invent an FDW callback that's meant to be
> invoked at this stage, but only call it for FDW(s) actively involved
> in the query.  I'm not sure exactly what that ought to look like though.
> Maybe only call the FDW identified as possible owner of the topmost
> scan/join relation?

I think in the short term that's as well as we're going to do, so +1.
In the long run, I'm interested in making FDWs be able to optimize
queries like foreigntab JOIN localtab ON foreigntab.x = localtab.x
(e.g. by copying localtab to the remote side when it's small); but
that will require revisiting some old decisions, too.  What you're
proposing here sounds consistent with what we've done so far.

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


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

Re: [HACKERS] Proposal: "Causal reads" mode for load balancing reads without stale data

[Robert Haas]

On Sun, Mar 13, 2016 at 11:50 PM, Thomas Munro
 wrote:
> The last patches I posted don't apply today due to changes in master,
> so here's a freshly merged patch series.

+from the current synchronous stanbyindicates it has received the

Uh, no.

-SyncRepWaitForLSN(gxact->prepare_end_lsn);
+{
+/*
+ * Don't wait for the prepare to be applied remotely in remote_apply
+ * mode, just wait for it to be flushed to the WAL.  We will wait for
+ * apply when the transaction eventuallly commits or aborts.
+ */
+if (synchronous_commit == SYNCHRONOUS_COMMIT_REMOTE_APPLY)
+assign_synchronous_commit(SYNCHRONOUS_COMMIT_REMOTE_FLUSH, NULL);
+
+SyncRepWaitForLSN(gxact->prepare_end_lsn);
+
+if (synchronous_commit == SYNCHRONOUS_COMMIT_REMOTE_APPLY)
+assign_synchronous_commit(SYNCHRONOUS_COMMIT_REMOTE_APPLY, NULL);
+}

What's with the extra block?

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


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

Re: [HACKERS] Improving replay of XLOG_BTREE_VACUUM records

[Vladimir Borodin]

> 10 марта 2016 г., в 14:38, Simon Riggs  написал(а):
> 
> On 10 March 2016 at 09:22, Michael Paquier  > wrote:
> On Thu, Mar 10, 2016 at 10:00 AM, Vladimir Borodin  > wrote:
> > Let’s do immediately after you will send a new version of your patch? Or
> > even better after testing your patch? Don’t get me wrong, but rejecting my
> > patch without tangible work on your patch may lead to forgiving about the
> > problem before 9.6 freeze.
> 
> This makes sense. Let's not reject this patch yet if the alternative
> approach is not committed.
> 
> I attach 2 patches.
> 
> avoid_pin_scan_always.v1.patch 
> Takes the approach that we generate the same WAL records as in 9.5, we just 
> choose not to do anything with that information. This is possible because we 
> don't care anymore whether it is toast or other relations. So it effectively 
> reverts parts of the earlier patch.
> This could be easily back-patched more easily.
> 
> toast_recheck.v1.patch
> Adds recheck code for toast access. I'm not certain this is necessary, but 
> here it is. No problems found with it.

JFYI, I’m preparing the stand to reproduce the initial problem and I hope to 
finish testing this week.

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


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

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

[Robert Haas]

On Sat, Mar 12, 2016 at 7:49 AM, Amit Langote  wrote:
> On Fri, Mar 11, 2016 at 2:31 PM, Amit Langote
>  wrote:
>> On 2016/03/11 13:16, Robert Haas wrote:
>>> On Thu, Mar 10, 2016 at 9:04 PM, Amit Langote
>>>  wrote:
 So, from what I understand here, we should not put total count of index
 pages into st_progress_param; rather, have the client (reading
 pg_stat_progress_vacuum) derive it using pg_indexes_size() (?), as and
 when necessary.  However, only server is able to tell the current position
 within an index vacuuming round (or how many pages into a given index
 vacuuming round), so report that using some not-yet-existent mechanism.
>>>
>>> Isn't that mechanism what you are trying to create in 0003?
>>
>> Right, 0003 should hopefully become that mechanism.
>
> About 0003:
>
> Earlier, it was trying to report vacuumed index block count using
> lazy_tid_reaped() callback for which I had added a index_blkno
> argument to IndexBulkDeleteCallback. Turns out it's not such a good
> place to do what we are trying to do.  This callback is called for
> every heap pointer in an index. Not all index pages contain heap
> pointers, which means the existing callback does not allow to count
> all the index blocks that AM would read to finish a given index vacuum
> run.
>
> Instead, the attached patch adds a IndexBulkDeleteProgressCallback
> which AMs should call for every block that's read (say, right before a
> call to ReadBufferExtended) as part of a given vacuum run. The
> callback with help of some bookkeeping state can count each block and
> report to pgstat_progress API. Now, I am not sure if all AMs read 1..N
> blocks for every vacuum or if it's possible that some blocks are read
> more than once in single vacuum, etc.  IOW, some AM's processing may
> be non-linear and counting blocks 1..N (where N is reported total
> index blocks) may not be possible.  However, this is the best I could
> think of as doing what we are trying to do here. Maybe index AM
> experts can chime in on that.
>
> Thoughts?

Well, I think you need to study the index AMs and figure this out.

But I think for starters you should write a patch that reports the following:

1. phase
2. number of heap blocks scanned
3. number of heap blocks vacuumed
4. number of completed index vac cycles
5. number of dead tuples collected since the last index vac cycle
6. number of dead tuples that we can store before needing to perform
an index vac cycle

All of that should be pretty straightforward, and then we'd have
something we can ship.  We can add the detailed index reporting later,
when we get to it, perhaps for 9.7.

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


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

Re: [HACKERS] eXtensible Transaction Manager API (v2)

[Kevin Grittner]

On Sat, Mar 12, 2016 at 11:21 AM, Robert Haas  wrote:

> I'd also be interested in hearing Kevin Grittner's thoughts about
> serializability in a distributed environment, since he's obviously
> thought about the topic of serializability quite a bit.

I haven't done a thorough search of the academic literature on
this, and I wouldn't be comfortable taking a really solid position
without that; but in thinking about it it seems like there are at
least three distinct problems which may have distinct solutions.

*Physical replication* may be best handled by leveraging the "safe
snapshot" idea already implemented in READ ONLY DEFERRABLE
transactions, and passing through information in the WAL stream to
allow the receiver to identify points where a snapshot can be taken
which cannot see an anomaly.  There should probably be an option to
use the last known safe snapshot or wait for a point in the stream
where one next appears.  This might take as little as a bit or two
per WAL commit record.  It's not clear what the processing overhead
would be -- it wouldn't surprise me if it was "in the noise", nor
would it surprise me if it wasn't.  We would need some careful
benchmarking, and, if performance was an issue, A GUC to control
whether the information was passed along (and, thus, whether
SERIALIZABLE transactions were allowed on the replica).

*Logical replication* (considered for the moment in a
unidirectional context) might best be handled by some reordering of
application of the commits on the replica into "apparent order of
execution" -- which is pretty well defined on the primary based on
commit order adjusted by read-write dependencies.  Basically, the
"simple" implementation would be that WAL is applied normally
unless you receive a commit record which is flagged in some way to
indicate that it is for a serializable transaction which wrote data
and at the time of commit was concurrent with at least one other
serializable transaction which had not completed and was not READ
ONLY.  Such a commit would await information in the WAL stream to
tell it when all such concurrent transactions completed, and would
indicate when such a transaction had a read-write dependency *in*
to the transaction with the suspended commit; commits for any such
transactions must be moved ahead of the suspended commit.  This
would allow logical replication, with all the filtering and such,
to avoid ever showing a state on the replica which contained
serialization anomalies.

*Logical replication with cycles* (where there is some path for
cluster A to replicate to cluster B, and some other path for
cluster B to replicate the same or related data to cluster A) has a
few options.  You could opt for "eventual consistency" --
essentially giving up on the I in ACID and managing the anomalies.
In practice this seems to lead to some form of S2PL at the
application coding level, which is very bad for performance and
concurrency, so I tend to think it should not be the only option.
Built-in S2PL would probably perform better than having it pasted
on at the application layer through some locking API, but for most
workloads is still inferior to SSI in both concurrency and
performance.  Unless a search of the literature turns up some new
alternative, I'm inclined to think that if you want to distribute a
"logical" database over multiple clusters and still manage race
conditions through use of SERIALIZABLE transactions, a distributed
SSI implementation may be the best bet.  That requires the
transaction manager (or something like it) to track non-blocking
predicate "locks" (what the implementation calls a SIReadLock)
across the whole environment, as well as tracking rw-conflicts (our
short name for read-write dependencies) across the whole
environment.  Since SSI also looks at the MVCC state, handling
checks of that without falling victim to race conditions would also
need to be handled somehow.

If I remember correctly, the patches to add the SSI implementation
of SERIALIZABLE transactions were about ten times the size of the
patches to remove S2PL and initially replace it with MVCC.  I don't
have even a gut feel as to how much bigger the distributed form is
likely to be.  On the one hand the *fundamental logic* is all there
and should not need to change; on the other hand the *mechanism*
for acquiring the data to be *used* in that logic would be
different and potentially complex.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] [patch] Proposal for \crosstabview in psql

[Daniel Verite]

Jim Nasby wrote:

> Ultimately I'd really like some way to remove/reduce the restriction of 
> result set definitions needing to be determined at plan time. That would 
> open the door for server-side crosstab/pivot as well a a host of other 
> things (such as dynamically turning a hstore/json/xml field into a 
> recordset).

> Ultimately I'd really like some way to remove/reduce the restriction of 
> result set definitions needing to be determined at plan time. That would 
> open the door for server-side crosstab/pivot as well a a host of other 
> things (such as dynamically turning a hstore/json/xml field into a 
> recordset).

That would go against a basic expectation of prepared statements, the
fact that queries can be parsed/prepared without any part of them
being executed.

For a dynamic pivot, but probably also for the other examples you
have in mind, the SQL engine wouldn't be able to determine the output
columns without executing a least a subselect to look inside some
table(s).

I suspect that the implications of this would be so far reaching and
problematic that it will just not happen.

It seems to me that a dynamic pivot will always consist of
two SQL queries that can never be combined into one,
unless using a workaround à la Oracle, which encapsulates the
entire dynamic resultset into an XML blob as output.
The problem here being that the client-side tools
that people routinely use are not equipped to process it anyway;
at least that's what  I find by anecdotal evidence for instance in:
https://community.oracle.com/thread/2133154?tstart=0
 or
http://stackoverflow.com/questions/19298424
 or
https://community.oracle.com/thread/2388982?tstart=0


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


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

Re: [HACKERS] WIP: Upper planner pathification

[Tom Lane]

Petr Jelinek  writes:
> On 14/03/16 02:43, Kouhei Kaigai wrote:
>> Even though I couldn't check the new planner implementation entirely,
>> it seems to be the points below are good candidate to inject CustomPath
>> (and potentially ForeignScan).
>> 
>> - create_grouping_paths
>> - create_window_paths
>> - create_distinct_paths
>> - create_ordered_paths
>> - just below of the create_modifytable_path
>> (may be valuable for foreign-update pushdown)

> To me that seems too low inside the planning tree, perhaps adding it
> just to the subquery_planner before SS_identify_outer_params would be
> better, that's the place where you see the path for the whole (sub)query
> so you can search and modify what you need from there.

I don't like either of those too much.  The main thing I've noticed over
the past few days is that you can't readily generate custom upper-level
Paths unless you know what PathTarget grouping_planner is expecting each
level to produce.  So what I was toying with doing is (1) having
grouping_planner put all those targets into the PlannerInfo, perhaps
in an array indexed by UpperRelationKind; and (2) adding a hook call
immediately after those targets are computed, say right before
the create_grouping_paths() call (approximately planner.c:1738
in HEAD).  It should be sufficient to have one hook there since
you can inject Paths into any of the upper relations at that point;
moreover, that's late enough that you shouldn't have to recompute
anything you figured out during scan/join planning.

Now, a simple hook function is probably about the best we can offer
CustomScan providers, since we don't really know what they're going
to do.  But I'm pretty unenthused about telling FDW authors to use
such a hook, because generally speaking they're simply going to waste
cycles finding out that they aren't involved in a given query.
It would be better if we invent an FDW callback that's meant to be
invoked at this stage, but only call it for FDW(s) actively involved
in the query.  I'm not sure exactly what that ought to look like though.
Maybe only call the FDW identified as possible owner of the topmost
scan/join relation?

regards, tom lane


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

Re: [HACKERS] WIP: Upper planner pathification

[Tom Lane]

Robert Haas  writes:
> On Mon, Mar 14, 2016 at 1:04 PM, Tom Lane  wrote:
>> It would be better if we invent an FDW callback that's meant to be
>> invoked at this stage, but only call it for FDW(s) actively involved
>> in the query.  I'm not sure exactly what that ought to look like though.
>> Maybe only call the FDW identified as possible owner of the topmost
>> scan/join relation?

> I think in the short term that's as well as we're going to do, so +1.
> In the long run, I'm interested in making FDWs be able to optimize
> queries like foreigntab JOIN localtab ON foreigntab.x = localtab.x
> (e.g. by copying localtab to the remote side when it's small); but
> that will require revisiting some old decisions, too.

Yeah.  An alternative definition that would support that would be to
call the upper-path-providing callback for each FDW that's responsible
for any base relation of the query.  But I think that that would often
lead to a lot of redundant/wasted computation, and it's not clear to
me that we can support such cases without other changes as well.

regards, tom lane


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

[HACKERS] Re: BUG #13685: Archiving while idle every archive_timeout with wal_level hot_standby

[David Steele]

On 2/24/16 12:40 AM, Michael Paquier wrote:


This has the merit to be clear, thanks for the input. Whatever the
approach taken at the end we have two candidates:
- Extend XLogInsert() with an extra argument for flags (Andres)
- Introduce XLogInsertExtended with this extra argument and let
XLogInsert() in peace (Robert and I).
Actually, I lied, there was still something I could do for this
thread: attached are two patches implementing both approaches as
respectively a-1 and a-2. Patch b is the set of logs I used for the
tests to show with a low checkpoint_timeout that checkpoints are
getting correctly skipped on an idle system.


Unfortunately neither A nor B apply anymore.

However, since the patches can still be read through I wonder if Robert 
or Andres would care to opine on whether A or B is better now that they 
can see the full implementation?


For my 2c I'm happy with XLogInsertExtended() since it seems to be a 
rare use case where flags are required.  This can always be refactored 
in the future if/when the use of flags spreads.


I think it would be good to make a decision on this before asking 
Michael to rebase.


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


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

Re: [HACKERS] WIP: Upper planner pathification

[Tom Lane]

Robert Haas  writes:
> On Mon, Mar 14, 2016 at 1:37 PM, Tom Lane  wrote:
>> Yeah.  An alternative definition that would support that would be to
>> call the upper-path-providing callback for each FDW that's responsible
>> for any base relation of the query.  But I think that that would often
>> lead to a lot of redundant/wasted computation, and it's not clear to
>> me that we can support such cases without other changes as well.

> Sure, that's fine with me.  Are you going to go make these changes now?

Yeah, in a bit.

> Eventually, we might just support a configurable flag on FDWs where
> FDWs that want to do this sort of thing can request callbacks on every
> join and every upper rel in the query.  But that can wait.

That'd be a possibility, too.

regards, tom lane


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

Re: [HACKERS] [patch] Proposal for \crosstabview in psql

[Robert Haas]

On Sat, Mar 12, 2016 at 10:34 AM, Daniel Verite  wrote:
>> But worse than either of  those things, there is no real
>> agreement on what the overall design of this feature
>> should be.
>
> The part in the design that raised concerns upthread is
> essentially how headers sorting is exposed to the user and
> implemented.
>
> As suggested in [1], I've made some drastic changes in the
> attached patch to take the comments (from Dean R., Tom L.)
> into account.
> [ ... lengthy explanation ... ]
> - also NULLs are no longer excluded from headers, per Peter E.
>   comment in [2].

Dean, Tom, Peter, what do you think of the new version?

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


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

Re: [HACKERS] plpgsql - DECLARE - cannot to use %TYPE or %ROWTYPE for composite types

[Robert Haas]

On Mon, Mar 14, 2016 at 12:04 PM, Tom Lane  wrote:
> I wrote:
>> However ... one thing I was intending to mention on this thread is that
>> "get the array type over this type" isn't the only extension one might
>> wish for.  Another likely desire is "get the type of field 'foo' of this
>> composite type".  I don't suggest that this patch needs to implement
>> that right now; but it would be a good thing if we could see how the
>> chosen syntax could be extended in such a direction.  Otherwise we might
>> be painting ourselves into a corner.
>
> To enlarge a little bit: it seems to me that what we're really wishing for
> here is a type name syntax that goes beyond simple names.  If we were
> starting in a green field, we might choose a recursively-composable syntax
> like the following.
>
> type_name can be:
>
> * A simple type name, such as int8 or varchar[42].
>
> * TYPE_OF(expression), meaning that the SQL expression is parsed but never
> executed, we just take this construct as naming its result type.
>
> * ARRAY_OF(type_name), meaning the array type having type_name as its
> element type.
>
> * ELEMENT_OF(type_name), meaning the element type of the array type
> named by type_name.
>
> * ROW_OF(type_name [, type_name ...]), meaning the composite type with
> those types as columns.
>
> * FIELD_OF(type_name, foo), meaning the type of column "foo" of the
> composite type named by type_name.  I'm not sure if there would be
> use-cases for selecting a column other than by a simple literal name,
> but there could be variants of this function if so.
>
> It's possible to think of other cases, for example what about range
> types?  You could allow ELEMENT_OF() to apply to range types, certainly.
> I'm not sure about the other direction, because multiple range types
> could have the same element type; but it's possible to hope that some
> type-naming function along the lines of RANGE_OF(type_name, other args)
> could disambiguate.  The main reason I'm thinking of a function-like
> syntax here is that it can easily handle additional arguments when
> needed.
>
> Comparing this flight of fancy to where we are today, we have
> %TYPE as a remarkably ugly and limited implementation of TYPE_OF(),
> and we have the precedent that foo[] means ARRAY_OF(foo).  I'm not
> sure how we get any extensibility out of either of those things.
>
> Or in short: maybe it's time to blow up %TYPE and start fresh.

That's not a dumb idea.  I think %TYPE is an Oracle-ism, and it
doesn't seem to have been their best-ever design decision.

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


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

Re: [HACKERS] pgbench - allow backslash-continuations in custom scripts

[David Steele]

Hi Fabien,

On 3/14/16 3:27 PM, Fabien COELHO wrote:

>> Any takers to review this updated patch?
> 
> I intend to have a look at it, I had a look at a previous instance, but
> I'm ok if someone wants to proceed.

There's not exactly a long line of reviewers at the moment so if you
could do a followup review that would be great.

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] WIP: Upper planner pathification

[Tom Lane]

Robert Haas  writes:
> On Mon, Mar 14, 2016 at 1:37 PM, Tom Lane  wrote:
>> Yeah.  An alternative definition that would support that would be to
>> call the upper-path-providing callback for each FDW that's responsible
>> for any base relation of the query.  But I think that that would often
>> lead to a lot of redundant/wasted computation, and it's not clear to
>> me that we can support such cases without other changes as well.

> Sure, that's fine with me.  Are you going to go make these changes now?

So I started looking at this, and almost immediately came to two
conclusions:

1. We need to add a "PathTarget *" parameter to create_foreignscan_path.
The hard-wired assignment that's in there now,

pathnode->path.pathtarget = &(rel->reltarget);

is sufficient for scan and join paths, but it doesn't work at all for
upper relations because we don't put anything in their reltarget fields.
We could do so, but it's still problematic because there will be
situations where non-topmost Paths associated with an upperrel have other
targets than what topmost Paths do.  This is true today for set-operation
Paths and WindowAgg Paths, and I think possibly other cases as well (such
as inserted projection nodes).  We could possibly insist on creating a
separate upperrel for every distinct target that's used by intermediate
levels of Path in these trees, but that seems kinda pointless to me, at
least for now.  Really the best answer is to let FDWs have control of it.
And it's not like we've never whacked this function's API around before.

2. I was being way too cycle-miserly when I decided to make
RelOptInfo.reltarget be an embedded struct rather than defining PathTarget
as a regular, separate node type.  I'm gonna change that before it's too
late.  One extra palloc per RelOptInfo is not a noticeable price, and
there are too many places where this choice is resulting in notational
weirdness.

regards, tom lane


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

Re: [HACKERS] Background Processes and reporting

[Robert Haas]

On Mon, Mar 14, 2016 at 3:54 PM, Vladimir Borodin  wrote:
> 5. Show extra information about wait event (i.e. exclusive of shared mode
> for LWLocks, relation/forknum/blknum for I/O operations, etc.).

I doubt that this is a good idea.  Everybody will pay the cost of it,
and who will get a benefit?  We haven't had any wait monitoring at all
in PostgreSQL for years and years and years and it's only just now
getting to the top of our list of things to fix.  So I have a hard
time believing that now we suddenly need this level of detail.  The
very good thing about the committed implementation is that it requires
*no* synchronization, and anything more than a 4-byte integer will
(probably an st_changecount type protocol).  I continue to believe
that a feature that is on for everyone and dirt cheap is going to be
more valuable than anything that is expensive enough to require an
"off" switch.

> I have already shown [0, 1] the overhead of measuring timings in linux on
> representative workload. AFAIK, these tests were the only one that showed
> any numbers. All other statements about terrible performance have been and
> remain unconfirmed.

Of course, those numbers are substantial regressions which would
likely make it impractical to turn this on on a heavily-loaded
production system.  On the other hand, the patch actually committed is
turned on by default and Amit posted numbers showing no performance
change at all.

> As for the size of such information it of course should be configurable.
> I.e. in Oracle there is a GUC for the size of ring buffer to store history
> of sampling with extra information about each wait event.

That's a reasonable idea, although not one I'm very excited about.

> Ok, doing it in short steps seems to be a good plan. Any objections against
> giving people an ability to turn some feature (i.e. notorious measuring
> timings) even if it makes some performance degradation? Of course, it should
> be turned off by default.

I am not totally opposed to that, but I think a feature that causes a
10% performance hit when you turn it on will be mostly useless.  The
people who need it won't be able to risk turning it on.

> If anything, I’m not from PostgresPro and I’m not «accusing you». But to be
> honest current committed implementation has been tested exactly on one
> machine with two workloads. And I think, it is somehow unfair to demand more
> from others. Although it doesn’t mean that testing on exactly one machine
> with only one OS is enough, of course. I suppose, you should ask the authors
> to test it on some representative hardware and workload but if authors don’t
> have them, it would be nice to help them with that.

I'm not necessarily opposed to that, but this thread has a lot more
heat than light, and some of the other threads on this topic have had
the same problem. There seems to be tremendous resistance to the idea
that recording timestamps is going to be extensive even though there
are many previous threads on pgsql-hackers about many different
features showing that this is true.  Somehow, I've got to justify a
position which has been taken by many people many times before on this
very same mailing list.  That strikes me as 100% backwards.

Similarly, the position that a wait-reporting interface that does not
require synchronization will be a lot cheaper than one that does
require synchronization has been questioned repeatedly.  I'm not very
interested in spending a lot of time defending that proposition or
producing benchmarking results to support it, and I don't think I
should have to.  We wouldn't have so many patches floating around that
aimed to reduce locking if synchronization overhead didn't cost, and
what is being proposed is to stick those into low-level code paths
that are sometimes highly trafficked.

> Also it would be really interesting to hear your opinion about the initial
> Andres’s question. Any thoughts about changing current committed
> implementation?

I'm a little vague on specifically what Andres has in mind.  I tend to
think that there's not much point in allowing
pg_stat_get_progress_info('checkpointer') because we can just have a
dedicated view for that sort of thing, cf. pg_stat_bgwriter, which
seems better.  Exposing the wait events from background processes
might be worth doing, but I don't think we want to add a bunch of
dummy lines to pg_stat_activity.

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


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

Re: [HACKERS] Fix for OpenSSL error queue bug

[Peter Geoghegan]

On Thu, Mar 10, 2016 at 8:16 PM, Peter Geoghegan  wrote:
> On Thu, Mar 10, 2016 at 7:22 PM, Peter Eisentraut  wrote:
>> Arguably, if everyone followed "my" approach, this should be very easy
>> to fix everywhere.
>
> I don't think that there is any clear indication that the OpenSSL
> people would share that view. Or my view. Or anything that's sensible
> or practical or actionable.

Ideally, we'd be able to get this into the upcoming minor release.
This bug has caused Heroku some serious problems.

-- 
Peter Geoghegan


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

Re: [HACKERS] Background Processes and reporting

[Vladimir Borodin]

> 14 марта 2016 г., в 22:21, Robert Haas  написал(а):
> 
> On Sat, Mar 12, 2016 at 6:05 AM, Oleg Bartunov  wrote:
>>> So?
>> 
>> So, Robert already has experience with the subject, probably,  he has bad
>> experience with edb implementation and he'd like to see something better in
>> community version. That's fair and I accept his position.
> 
> Bingo - though maybe "bad" experience is not quite as accurate as
> "could be better".
> 
>> Wait monitoring is one of the popular requirement of russian companies, who
>> migrated from Oracle. Overwhelming majority of them use Linux, so I suggest
>> to have configure flag for including wait monitoring at compile time
>> (default is no wait monitoring), or have GUC variable, which is also off by
>> default, so we have zero to minimal overhead of monitoring. That way we'll
>> satisfy many enterprises and help them to choose postgres, will get feedback
>> from production use and have time for feature improving.
> 
> So, right now we can only display the wait information in
> pg_stat_activity.  There are a couple of other things that somebody
> might want to do:
> 
> 1. Sample the wait state information across all backends in the
> system.  On a large, busy system, this figures to be quite cheap, and
> the sampling interval could be configurable.
> 
> 2. Count every instance of every wait event in every backend, and roll
> that up either via shared memory or additional stats messges.
> 
> 3. Like #2, but with timing information.
> 
> 4. Like #2, but on a per-query basis, somehow integrated with
> pg_stat_statements.

5. Show extra information about wait event (i.e. exclusive of shared mode for 
LWLocks, relation/forknum/blknum for I/O operations, etc.).

> 
> The challenge with any of these except #1 is that they are going to
> produce a huge volume of data, and, whether you believe it or not, #3
> is going to sometimes be crushingly slow.  Really.  I tend to think
> that #1 might be better than #2 or #3, but I'm not unwilling to listen
> to contrary arguments, especially if backed up by careful benchmarking
> showing that the performance hit is negligible.

I have already shown [0, 1] the overhead of measuring timings in linux on 
representative workload. AFAIK, these tests were the only one that showed any 
numbers. All other statements about terrible performance have been and remain 
unconfirmed.

As for the size of such information it of course should be configurable. I.e. 
in Oracle there is a GUC for the size of ring buffer to store history of 
sampling with extra information about each wait event.

[0] 
http://www.postgresql.org/message-id/eee78e40-0e48-411a-9f90-cf9339da9...@simply.name
[1] 
http://www.postgresql.org/message-id/5f3dd73a-2a85-44bf-9f47-54049a81c...@simply.name

>  My reason for wanting
> to get the stuff we already had committed first is because I have
> found that it is best to proceed with these kinds of problems
> incrementally, not trying to solve too much in a single commit.  Now
> that we have the basics, we can build on it, adding more wait events
> and possibly more recordkeeping for the ones we have already - but
> anything that regresses performance for people not using the feature
> is a dead end in my book, as is anything that introduces overall
> stability risks.

Ok, doing it in short steps seems to be a good plan. Any objections against 
giving people an ability to turn some feature (i.e. notorious measuring 
timings) even if it makes some performance degradation? Of course, it should be 
turned off by default.

> 
> I think the way forward from here is that Postgres Pro should (a)
> rework their implementation to work with what has already been
> committed, (b) consider carefully whether they've done everything
> possible to contain the performance loss, (c) benchmark it on several
> different machines and workloads to see how much performance loss
> there is, and (d) stop accusing me of acting in bad faith.

If anything, I’m not from PostgresPro and I’m not «accusing you». But to be 
honest current committed implementation has been tested exactly on one machine 
with two workloads. And I think, it is somehow unfair to demand more from 
others. Although it doesn’t mean that testing on exactly one machine with only 
one OS is enough, of course. I suppose, you should ask the authors to test it 
on some representative hardware and workload but if authors don’t have them, it 
would be nice to help them with that.

Also it would be really interesting to hear your opinion about the initial 
Andres’s question. Any thoughts about changing current committed implementation?

> 
> -- 
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
> 
> 
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


--
May the force be with you…

Re: [HACKERS] [PATCH v6] GSSAPI encryption support

[David Steele]

On 3/14/16 4:10 PM, Robbie Harwood wrote:
> David Steele  writes:
> 
>> Hi Robbie,
>>
>> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>>> Hello friends,
>>>
>>> Here's yet another version of GSSAPI encryption support.  It's also
>>> available for viewing on my github:
>>
>> The build went fine but when testing I was unable to logon at all.  I'm
>> using the same methodology as in
>> http://www.postgresql.org/message-id/56be0ff9.70...@pgmasters.net except
>> that I'm running against 51c0f63 and using the v6 patch set.
>>
>> psql simply hangs and never returns.  I have attached a pcap of the
>> psql/postgres session generated with:
>>
>> tcpdump -i lo -nnvvXSs 1514 port 5432 -w gssapi.pcap
>>
>> If you would like me to capture more information please let me know
>> specifically how you would like me to capture it.
> 
> Please disregard my other email.  I think I've found the issue; will
> post a new version in a moment.

Strange timing since I was just testing this.  Here's what I got:

$ pg/bin/psql -h localhost -U vagr...@pgmasters.net postgres
conn->inStart = 179, conn->inEnd = 179, conn->inCursor = 179
psql (9.6devel)
Type "help" for help.

postgres=>

This was after commenting out:

// appendBinaryPQExpBuffer(>gwritebuf,
//  conn->inBuffer + conn->inStart,
//  conn->inEnd - conn->inStart);
// conn->inEnd = conn->inStart;

The good news I can log on every time now!

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Add schema-qualified relnames in constraint error messages.

[Tom Lane]

"Shulgin, Oleksandr"  writes:
> What I dislike about this POC is all the disruption in libpq, to be
> honest.

Yeah, I don't much like that either.  But I don't think we can avoid
some refactoring there; as designed, conversion of an error message into
user-visible form is too tightly tied to receipt of the message.

> It would be much neater if we could form the verbose message every
> time and let the client decide where to cut it.  Maybe a bit "too clever"
> would be to put a \0 char between short message and it's verbose
> continuation.  The client could then reach the verbose part like this
> (assuming that libpq did put a verbose part there): msg + strlen(msg) + 1.

Blech :-(

Thinking about it, though, it seems to me that we could get away with
always performing both styles of conversion and sticking both strings
into the PGresult.  That would eat a little more space but not much.
Then we just need to add API to let the application get at both forms.

regards, tom lane


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

[HACKERS] Upcoming back-branch releases

[Tom Lane]

In view of some rather important recent fixes such as commits
bf7ced5e2dc8622f and 301cc3549c29aaa5, the release team has decided
that it'd be a good thing to schedule a set of minor releases in
the near future.  The nearest convenient time seems to be week after
next, that is, wrap Monday Mar 28 for public announcement on Thursday
Mar 31st.

regards, tom lane


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

Re: [HACKERS] Background Processes and reporting

[Robert Haas]

On Sat, Mar 12, 2016 at 6:05 AM, Oleg Bartunov  wrote:
>> So?
>
> So, Robert already has experience with the subject, probably,  he has bad
> experience with edb implementation and he'd like to see something better in
> community version. That's fair and I accept his position.

Bingo - though maybe "bad" experience is not quite as accurate as
"could be better".

> Wait monitoring is one of the popular requirement of russian companies, who
> migrated from Oracle. Overwhelming majority of them use Linux, so I suggest
> to have configure flag for including wait monitoring at compile time
> (default is no wait monitoring), or have GUC variable, which is also off by
> default, so we have zero to minimal overhead of monitoring. That way we'll
> satisfy many enterprises and help them to choose postgres, will get feedback
> from production use and have time for feature improving.

So, right now we can only display the wait information in
pg_stat_activity.  There are a couple of other things that somebody
might want to do:

1. Sample the wait state information across all backends in the
system.  On a large, busy system, this figures to be quite cheap, and
the sampling interval could be configurable.

2. Count every instance of every wait event in every backend, and roll
that up either via shared memory or additional stats messges.

3. Like #2, but with timing information.

4. Like #2, but on a per-query basis, somehow integrated with
pg_stat_statements.

The challenge with any of these except #1 is that they are going to
produce a huge volume of data, and, whether you believe it or not, #3
is going to sometimes be crushingly slow.  Really.  I tend to think
that #1 might be better than #2 or #3, but I'm not unwilling to listen
to contrary arguments, especially if backed up by careful benchmarking
showing that the performance hit is negligible.  My reason for wanting
to get the stuff we already had committed first is because I have
found that it is best to proceed with these kinds of problems
incrementally, not trying to solve too much in a single commit.  Now
that we have the basics, we can build on it, adding more wait events
and possibly more recordkeeping for the ones we have already - but
anything that regresses performance for people not using the feature
is a dead end in my book, as is anything that introduces overall
stability risks.

I think the way forward from here is that Postgres Pro should (a)
rework their implementation to work with what has already been
committed, (b) consider carefully whether they've done everything
possible to contain the performance loss, (c) benchmark it on several
different machines and workloads to see how much performance loss
there is, and (d) stop accusing me of acting in bad faith.

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


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

Re: [HACKERS] Background Processes and reporting

[Andres Freund]

Hi,

On 2016-03-14 16:16:43 -0400, Robert Haas wrote:
> > I have already shown [0, 1] the overhead of measuring timings in linux on
> > representative workload. AFAIK, these tests were the only one that showed
> > any numbers. All other statements about terrible performance have been and
> > remain unconfirmed.
>
> Of course, those numbers are substantial regressions which would
> likely make it impractical to turn this on on a heavily-loaded
> production system.

A lot of people operating production systems are fine with trading a <=
10% impact for more insight into the system; especially if that
configuration can be changed without a restart.  I know a lot of systems
that use pg_stat_statements, track_io_timing = on, etc; just to get
that. In fact there's people running perf more or less continuously in
production environments; just to get more insight.

I think it's important to get as much information out there without
performance overhead, so it can be enabled by default. But I don't think
it makes sense to not allow features in that cannot be enabled by
default, *if* we tried to make them cheap enough beforehand.


> > Ok, doing it in short steps seems to be a good plan. Any objections against
> > giving people an ability to turn some feature (i.e. notorious measuring
> > timings) even if it makes some performance degradation? Of course, it should
> > be turned off by default.
>
> I am not totally opposed to that, but I think a feature that causes a
> 10% performance hit when you turn it on will be mostly useless.  The
> people who need it won't be able to risk turning it on.

That's not my experience.


> > If anything, I’m not from PostgresPro and I’m not «accusing you». But to be
> > honest current committed implementation has been tested exactly on one
> > machine with two workloads. And I think, it is somehow unfair to demand more
> > from others. Although it doesn’t mean that testing on exactly one machine
> > with only one OS is enough, of course. I suppose, you should ask the authors
> > to test it on some representative hardware and workload but if authors don’t
> > have them, it would be nice to help them with that.
>
> I'm not necessarily opposed to that, but this thread has a lot more
> heat than light

Indeed.


>, and some of the other threads on this topic have had
> the same problem. There seems to be tremendous resistance to the idea
> that recording timestamps is going to be extensive even though there
> are many previous threads on pgsql-hackers about many different
> features showing that this is true.  Somehow, I've got to justify a
> position which has been taken by many people many times before on this
> very same mailing list.  That strikes me as 100% backwards.

Agreed; I find that pretty baffling. Especially that pointing out
problems like timestamp overhead generates a remarkable amount of
hostility is weird.


> > Also it would be really interesting to hear your opinion about the initial
> > Andres’s question. Any thoughts about changing current committed
> > implementation?
>
> I'm a little vague on specifically what Andres has in mind.

That makes two of us.


> I tend to think that there's not much point in allowing
> pg_stat_get_progress_info('checkpointer') because we can just have a
> dedicated view for that sort of thing, cf. pg_stat_bgwriter, which
> seems better.

But that infrastructure isn't really suitable for exposing quickly
changing counters imo. And given that we now have a relatively generic
framework, it seems like a pain to add a custom implementation just for
the checkpointer. Also, using custom infrastructure means it's not
extensible to custom bgworker, which doesn't seem like a good
idea. E.g. it'd be very neat to show the progress of a logical
replication catchup process that way, no?


> Exposing the wait events from background processes
> might be worth doing, but I don't think we want to add a bunch of
> dummy lines to pg_stat_activity.

Why are those dummy lines? It's activity in the cluster? We already show
autovacuum workers in there. And walsenders, if you query the underlying
function, instead of pg_stat_activity (due to a join to pg_database).

Andres


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

Re: [HACKERS] pgbench - allow backslash-continuations in custom scripts

[Fabien COELHO]

Hello David,


Any takers to review this updated patch?


I intend to have a look at it, I had a look at a previous instance, but 
I'm ok if someone wants to proceed.


--
Fabien.


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

Re: [HACKERS] amcheck (B-Tree integrity checking tool)

[Peter Geoghegan]

On Mon, Mar 14, 2016 at 10:39 AM, Robert Haas  wrote:
> I don't particularly like that interface.  I also suggest that it
> would be better to leave throttling to a future commit, and focus on
> getting the basic feature in first.

Works for me. I don't think throttling is an especially compelling feature.

-- 
Peter Geoghegan


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

Re: [HACKERS] PATCH: use foreign keys to improve join estimates v1

[Tomas Vondra]

Hi,

On 03/14/2016 02:12 PM, David Steele wrote:

Hi Thomas,

...

I don't think it would be clear to any reviewer which patch to apply
even if they were working.  I'm marking this "waiting for author".


Yeah. Rebasing the patches to current master was simple enough (there 
was just a simple #include conflict), but figuring out which of the 
patches is review-worthy was definitely difficult.


I do believe David's last patch is the best step forward, so I've 
rebased it, and made some basic aesthetic fixes (adding or rewording 
comments on a few places, etc.)


The one important code change is that I've removed the piece of code 
from find_best_foreign_key_quals that tried to be a bit too smart about 
equivalence classes.


My understanding is that it tried to handle cases like this example:

CREATE TABLE f (id1 INT, id2 INT, PRIMARY KEY (id1, id2));

CREATE TABLE d1 (id1 INT, id2 INT, FOREIGN KEY (id1, id2)
   REFERENCES f(id1, id2));

CREATE TABLE d2 (id1 INT, id2 INT, FOREIGN KEY (id1, id2)
   REFERENCES f(id1, id2));

SELECT * FROM f JOIN d1 ON (f.id1 = d1.id1 AND f.id2 = d1.id2)
JOIN d2 ON (f.id1 = d2.id1 AND f.id2 = d2.id2);

But it did so by also deriving foreign keys between d1 and d2, which I 
believe is wrong as there really is no foreign key, and thus no 
guarantee of existence of a matching row.


FWIW as I explained in a message from 24/2/2015, while this is 
definitely an issue worth fixing, I believe it needs to be done in some 
other way, not by foreign keys.


Attached is v3 of the patch, and also three SQL scripts demonstrating 
the impact of the patch on simple examples.



regards

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


diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index eb0fc1e..3d38384 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -2153,6 +2153,16 @@ _outIndexOptInfo(StringInfo str, const IndexOptInfo *node)
 }
 
 static void
+_outForeignKeyOptInfo(StringInfo str, const ForeignKeyOptInfo *node)
+{
+	WRITE_NODE_TYPE("FOREIGNKEYOPTINFO");
+
+	WRITE_OID_FIELD(conrelid);
+	WRITE_OID_FIELD(confrelid);
+	WRITE_INT_FIELD(nkeys);
+}
+
+static void
 _outEquivalenceClass(StringInfo str, const EquivalenceClass *node)
 {
 	/*
@@ -3603,6 +3613,9 @@ _outNode(StringInfo str, const void *obj)
 			case T_IndexOptInfo:
 _outIndexOptInfo(str, obj);
 break;
+			case T_ForeignKeyOptInfo:
+_outForeignKeyOptInfo(str, obj);
+break;
 			case T_EquivalenceClass:
 _outEquivalenceClass(str, obj);
 break;
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 5350329..4399a9f 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -3870,6 +3870,347 @@ get_parameterized_joinrel_size(PlannerInfo *root, RelOptInfo *rel,
 }
 
 /*
+ * quals_match_foreign_key
+ *		Determines if the foreign key is matched by joinquals.
+ *
+ * Checks that there are conditions on all columns of the foreign key, matching
+ * the operator used by the foreign key etc. If such complete match is found,
+ * the function returns bitmap identifying the matching quals (0-based).
+ *
+ * Otherwise (no match at all or incomplete match), NULL is returned.
+ */
+static Bitmapset *
+quals_match_foreign_key(PlannerInfo *root, ForeignKeyOptInfo *fkinfo,
+		RelOptInfo *fkrel, RelOptInfo *foreignrel,
+		List *joinquals)
+{
+	int i;
+	int nkeys = fkinfo->nkeys;
+	Bitmapset *qualmatches = NULL;
+	Bitmapset *fkmatches = NULL;
+
+	/*
+	 * Loop over each column of the foreign key and build a bitmap index
+	 * of each joinqual which matches. Note that we don't stop when we find
+	 * the first match, as the expression could be duplicated in the
+	 * joinquals, and we want to generate a bitmap index which has bits set for
+	 * every matching join qual.
+	 */
+	for (i = 0; i < nkeys; i++)
+	{
+		ListCell *lc;
+		int quallstidx = -1;
+
+		foreach(lc, joinquals)
+		{
+			RestrictInfo   *rinfo;
+			OpExpr		   *clause;
+			Var			   *leftvar;
+			Var			   *rightvar;
+
+			quallstidx++;
+
+			/*
+			 * Technically we don't need to, but here we skip this qual if
+			 * we've matched it to part of the foreign key already. This
+			 * should prove to be a useful optimization when the quals appear
+			 * in the same order as the foreign key's keys. We need only bother
+			 * doing this when the foreign key is made up of more than 1 set
+			 * of columns, and we're not testing the first column.
+			 */
+			if (i > 0 && bms_is_member(quallstidx, qualmatches))
+continue;
+
+			/*
+			 * Here since 'usefulquals' only contains bitmap indexes for quals
+			 * of type "var op var" we can safely skip checking this.
+			 */
+			rinfo = (RestrictInfo *) lfirst(lc);
+			clause = (OpExpr *) rinfo->clause;
+

Re: [HACKERS] Parallel Aggregate

[Paul Ramsey]

On Sun, Mar 13, 2016 at 7:31 PM, David Rowley
 wrote:
> On 14 March 2016 at 14:52, James Sewell  wrote:
>> One question - how is the upper limit of workers chosen?
>
> See create_parallel_paths() in allpaths.c. Basically the bigger the
> relation (in pages) the more workers will be allocated, up until
> max_parallel_degree.

Does the cost of the aggregate function come into this calculation at
all? In PostGIS land, much smaller numbers of rows can generate loads
that would be effective to parallelize (worker time much >> than
startup cost).

P


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

Re: [HACKERS] pglogical_output - a general purpose logical decoding output plugin

[Andres Freund]

On 2016-01-31 05:09:33 +0800, Craig Ringer wrote:
> On 29 January 2016 at 18:16, Andres Freund  wrote:
> 
> > Hi,
> >
> > so, I'm reviewing the output of:
> >
> 
> Thankyou very much for the review.

Afaics you've not posted an updated version of this? Any chance you
could?

Greetings,

Andres Freund


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

Re: [HACKERS] plpgsql - DECLARE - cannot to use %TYPE or %ROWTYPE for composite types

[Tom Lane]

Robert Haas  writes:
> On Mon, Mar 14, 2016 at 12:04 PM, Tom Lane  wrote:
>> Or in short: maybe it's time to blow up %TYPE and start fresh.

> That's not a dumb idea.  I think %TYPE is an Oracle-ism, and it
> doesn't seem to have been their best-ever design decision.

It is, and it wasn't.  What concerns me about the present patch is
that it's trying to shoehorn more functionality into something that
was badly designed to start with.  I think we'd be better off leaving
%TYPE as a deprecated backwards-compatibility feature and inventing
something new and more extensible.

I'm not wedded to any part of the syntax I just wrote, but I do say
that we need something that allows composability of type selectors.

regards, tom lane


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

Re: [HACKERS] Parallel Aggregate

[Robert Haas]

On Sun, Mar 13, 2016 at 5:44 PM, David Rowley
 wrote:
> On 12 March 2016 at 16:31, David Rowley  wrote:
>> I've attached an updated patch which is based on commit 7087166,
>> things are really changing fast in the grouping path area at the
>> moment, but hopefully the dust is starting to settle now.
>
> The attached patch fixes a harmless compiler warning about a possible
> uninitialised variable.

I haven't fully studied every line of this yet, but here are a few comments:

+   case T_PartialAggref:
+   coll = InvalidOid; /* XXX is this correct? */
+   break;

I doubt it.  More generally, why are we inventing PartialAggref
instead of reusing Aggref?  The code comments seem to contain no
indication as to why we shouldn't need all the same details for
PartialAggref that we do for Aggref, instead of only a small subset of
them.  Hmm... actually, it looks like PartialAggref is intended to
wrap Aggref, but that seems like a weird design.  Why not make Aggref
itself DTRT?   There's not enough explanation in the patch of what is
going on here and why.

}
+   if (can_parallel)
+   {

Seems like a blank line would be in order.

I don't see where this applies has_parallel_hazard or anything
comparable to the aggregate functions.  I think it needs to do that.

+   /* XXX +1 ? do we expect the main process to actually do real work? */
+   numPartialGroups = Min(numGroups, subpath->rows) *
+   (subpath->parallel_degree + 1);

I'd leave out the + 1, but I don't think it matters much.

+   aggstate->finalizeAggs == true)

We usually just say if (a) not if (a == true) when it's a boolean.
Similarly !a rather than a == false.

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


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

Re: [HACKERS] Background Processes and reporting

[Andres Freund]

On 2016-03-12 16:29:11 +0530, Amit Kapila wrote:
> On Sat, Mar 12, 2016 at 3:10 AM, Andres Freund  wrote:
> >
> >
> > > Similarly for the wait event stuff - checkpointer, wal writer,
> > > background writer are in many cases processes that very often are
> > > blocked on locks, IO and such.  Thus restricting the facility to
> > > database connected processes seems like a loss.
> >
> > I think one way to address this would be to not only report
> > PgBackendStatus type processes in pg_stat_activity. While that'd
> > obviously be a compatibility break, I think it'd be an improvement.
> >
> 
> I think here another point which needs more thoughts is that many of the
> pg_stat_activity fields are not relevant for background processes, ofcourse
> one can say that we can keep those fields as NULL, but still I think that
> indicates it is not the most suitable way to expose such information.

But neither are all of them relevant for autovacuum workers, and we
already show them.  pg_stat_activity as a name imo doesn't really imply
that it's about plain queries.  ISTM we should add a 'backend_type'
column that is one of backend|checkpointer|autovacuum|autovacuum-worker|wal 
writer| bgwriter| bgworker
(or similar). That makes querying easier.  And then display all shmem
connected processes.

> Another way could be to have new view like pg_stat_background_activity with
> only relevant fields or try expose via individual views like
> pg_stat_bgwriter.

I think the second is a pretty bad alternative; it'll force us to add
new views with very similar information; and it'll be hard to get
information about the whole system.   I mean if you want to know which
locks are causing problems, you don't primarily care whether it's a
background process or a backend that has contention issues.


> Do you intend to get this done for 9.6 considering an add-on patch for wait
> event information displayed in pg_stat_activity?

I think we should fix this for 9.6; it's a weakness in a new
interface. Let's not yank people around more than we need to.

I'm willing to do some work on that, if we can agree upon a course.

Andres


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

Re: [HACKERS] [PATCH v6] GSSAPI encryption support

[Robbie Harwood]

David Steele  writes:

> Hi Robbie,
>
> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>> Hello friends,
>> 
>> Here's yet another version of GSSAPI encryption support.  It's also
>> available for viewing on my github:
>
> The build went fine but when testing I was unable to logon at all.  I'm
> using the same methodology as in
> http://www.postgresql.org/message-id/56be0ff9.70...@pgmasters.net except
> that I'm running against 51c0f63 and using the v6 patch set.
>
> psql simply hangs and never returns.  I have attached a pcap of the
> psql/postgres session generated with:
>
> tcpdump -i lo -nnvvXSs 1514 port 5432 -w gssapi.pcap
>
> If you would like me to capture more information please let me know
> specifically how you would like me to capture it.

Please disregard my other email.  I think I've found the issue; will
post a new version in a moment.


signature.asc
Description: PGP signature

Re: [HACKERS] Parallel Aggregate

[Robert Haas]

On Mon, Mar 14, 2016 at 3:56 PM, Paul Ramsey  wrote:
> On Sun, Mar 13, 2016 at 7:31 PM, David Rowley
>  wrote:
>> On 14 March 2016 at 14:52, James Sewell  wrote:
>>> One question - how is the upper limit of workers chosen?
>>
>> See create_parallel_paths() in allpaths.c. Basically the bigger the
>> relation (in pages) the more workers will be allocated, up until
>> max_parallel_degree.
>
> Does the cost of the aggregate function come into this calculation at
> all? In PostGIS land, much smaller numbers of rows can generate loads
> that would be effective to parallelize (worker time much >> than
> startup cost).

Unfortunately, no - only the table size.  This is a problem, and needs
to be fixed.  However, it's probably not going to get fixed for 9.6.
:-(

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


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

Re: [HACKERS] Timeline following for logical slots

[Andres Freund]

On 2016-03-14 20:10:58 -0300, Alvaro Herrera wrote:
> Great, thanks.  I've studied this to the point where I'm confident that
> it makes sense, so I'm about to push it.  I didn't change any logic,
> only updated comments here and there, both in the patch and in some
> preexisting code.  I also changed the "List *timelineHistory" to be
> #ifndef FRONTEND, which seems cleaner than having it and insist that it
> couldn't be used.

Could you perhaps wait till tomorrow? I'd like to do a pass over it.

Thanks

Andres


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

Re: [HACKERS] Parallel Aggregate

[Robert Haas]

On Mon, Mar 14, 2016 at 6:24 PM, James Sewell  wrote:
> Any chance of getting a GUC (say min_parallel_degree) added to allow setting
> the initial value of parallel_degree, then changing the small relation check
> to also pass if parallel_degree > 1?
>
> That way you could set min_parallel_degree on a query by query basis if you
> are running aggregates which you know will take a lot of CPU.
>
> I suppose it wouldn't make much sense at all to set globally though, so it
> could just confuse matters.

I kind of doubt this would work well, but somebody could write a patch
for it and try it out.

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


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

Re: [HACKERS] [PATCH v6] GSSAPI encryption support

[David Steele]

On 3/14/16 7:20 PM, Robbie Harwood wrote:

> David Steele  writes:
>
>>
>> Strange timing since I was just testing this.  Here's what I got:
>>
>> $ pg/bin/psql -h localhost -U vagr...@pgmasters.net postgres
>> conn->inStart = 179, conn->inEnd = 179, conn->inCursor = 179
>> psql (9.6devel)
>> Type "help" for help.
>>
>> postgres=>
> 
> Thanks, that certainly is interesting!  I did finally manage to
> reproduce the issue on my end, but the rate of incidence is much lower
> than what you and Michael were seeing: I have to run connections in a
> loop for about 10-20 minutes before it makes itself apparent (and no,
> it's not due to entropy).  Apparently I just wasn't patient enough.

I'm running client and server on the same VM - perhaps that led to less
latency than your setup?

> All that is to say: thank you very much for investigating that!

My pleasure!

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] plpgsql - DECLARE - cannot to use %TYPE or %ROWTYPE for composite types

[Pavel Stehule]

2016-03-14 20:38 GMT+01:00 Tom Lane :

> Robert Haas  writes:
> > On Mon, Mar 14, 2016 at 12:04 PM, Tom Lane  wrote:
> >> Or in short: maybe it's time to blow up %TYPE and start fresh.
>
> > That's not a dumb idea.  I think %TYPE is an Oracle-ism, and it
> > doesn't seem to have been their best-ever design decision.
>
>
Using %TYPE has sense in PostgreSQL too. It is protection against a
domain's explosion - I don't need to declare the domains like varchar_10,
varchar_100, etc. It does more work in Oracle due living relation between
table columns and PL/SQL variables - but this differences are same for
domain types in PL/pgSQL. What is redundant in Postgres, is using %TYPE and
%ROWTYPE. Because native PL languages has strong relation to system catalog
I see this feature like well designed and helpful. Some different can be
our implementation.


>
> It is, and it wasn't.  What concerns me about the present patch is
> that it's trying to shoehorn more functionality into something that
> was badly designed to start with.  I think we'd be better off leaving
> %TYPE as a deprecated backwards-compatibility feature and inventing
> something new and more extensible.
>
> I'm not wedded to any part of the syntax I just wrote, but I do say
> that we need something that allows composability of type selectors.
>

Last version of this patch doesn't modify %TYPE part - and the implemented
syntax is near to your proposed (not all cases), and near to ADA syntax.
But, probably, you are unhappy with it.

Can you describe your expectations from this feature little bit more,
please?

We can leave the original discussion about %TYPE. It was my mistake. I push
two different ingredients to one soup.

There are two independent features - referenced types - the original %TYPE
and %ROWTYPE features, the possibility to take type from system catalog
information.

Last patch implements linear ELEMENT OF ... , ARRAY OF ... . Can be
solution recursive enhancing of last patch? We can implement other type
modificator like RANGE OF (any other?)

Then we can write some like

ARRAY OF RANGE OF int;  or ELEMENT OF ELEMENT OF array_of_ranges

or if we use functional syntax

ARRAY_OF(RANGE_OF(int))

I prefer non functional syntax - it looks more natural in DECLARE part, but
it is detail in this moment. I can live with functional syntax too. The
functional syntax is easy parserable, but the SQL is not functional - so I
see it foreign there.

Where you are expecting the implementation? In PLpgSQL only, or generally
in DDL, or in both levels?

Regards

Pavel



>
> regards, tom lane
>

Re: [HACKERS] Background Processes and reporting

[Robert Haas]

On Mon, Mar 14, 2016 at 4:42 PM, Andres Freund  wrote:
> On 2016-03-14 16:16:43 -0400, Robert Haas wrote:
>> > I have already shown [0, 1] the overhead of measuring timings in linux on
>> > representative workload. AFAIK, these tests were the only one that showed
>> > any numbers. All other statements about terrible performance have been and
>> > remain unconfirmed.
>>
>> Of course, those numbers are substantial regressions which would
>> likely make it impractical to turn this on on a heavily-loaded
>> production system.
>
> A lot of people operating production systems are fine with trading a <=
> 10% impact for more insight into the system; especially if that
> configuration can be changed without a restart.  I know a lot of systems
> that use pg_stat_statements, track_io_timing = on, etc; just to get
> that. In fact there's people running perf more or less continuously in
> production environments; just to get more insight.
>
> I think it's important to get as much information out there without
> performance overhead, so it can be enabled by default. But I don't think
> it makes sense to not allow features in that cannot be enabled by
> default, *if* we tried to make them cheap enough beforehand.

Hmm, OK.  I would have expected you to be on the other side of this
question, so maybe I'm all wet.  One point I am concerned about is
that, right now, we have only a handful of types of wait events.  I'm
very interested in seeing us add more, like I/O and client wait.  So
any overhead we pay here is likely to eventually be paid in a lot of
places - thus it had better be extremely small.

>> I tend to think that there's not much point in allowing
>> pg_stat_get_progress_info('checkpointer') because we can just have a
>> dedicated view for that sort of thing, cf. pg_stat_bgwriter, which
>> seems better.
>
> But that infrastructure isn't really suitable for exposing quickly
> changing counters imo. And given that we now have a relatively generic
> framework, it seems like a pain to add a custom implementation just for
> the checkpointer. Also, using custom infrastructure means it's not
> extensible to custom bgworker, which doesn't seem like a good
> idea. E.g. it'd be very neat to show the progress of a logical
> replication catchup process that way, no?

It isn't really that hard to make a purpose-built shared memory area
for each permanent background process, and I think that would be a
better design.  Then you can have whatever types you need, whatever
column labels make sense, etc.  You can't really do that for command
progress reporting because there are so many commands, but a
single-purpose backend doesn't have that issue.  I do agree that
having background workers report into this facility could make sense.

>> Exposing the wait events from background processes
>> might be worth doing, but I don't think we want to add a bunch of
>> dummy lines to pg_stat_activity.
>
> Why are those dummy lines? It's activity in the cluster? We already show
> autovacuum workers in there. And walsenders, if you query the underlying
> function, instead of pg_stat_activity (due to a join to pg_database).

Hmm.  Well, OK, maybe.  I didn't realize walsenders were showing up
there ... sorta.  I guess my concern was that people would complain
about breaking compatibility, but since we're doing that already maybe
we ought to double down on it.

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


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

Re: [HACKERS] plpgsql - DECLARE - cannot to use %TYPE or %ROWTYPE for composite types

[Tom Lane]

Pavel Stehule  writes:
> Where you are expecting the implementation? In PLpgSQL only, or generally
> in DDL, or in both levels?

I'd envision this as something the main parser does and plpgsql piggybacks
on.  One of the many half-baked things about %TYPE is that the main parser
has an implementation, and plpgsql has its own not-entirely-compatible
one.  (And one thing I especially don't like about the submitted patch is
that it makes those two diverge even further.)

regards, tom lane


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

Re: [HACKERS] Fix for OpenSSL error queue bug

[Tom Lane]

Peter Geoghegan  writes:
> On Mon, Mar 14, 2016 at 3:06 PM, Tom Lane  wrote:
>> Agreed, we need to deal with this one way or the other.  My proposal
>> is:
>> 
>> 1. In HEAD, do it as Peter E. suggests, ie clear error queue before calls.
>> 
>> 2. In back branches, clear error queue before *and* after calls.  This
>> will waste a few nanoseconds but will avoid any risk of breaking
>> existing third-party code.

> I am concerned that users will never be able to get this right, since
> I think it requires every Ruby or PHP app using some thin OpenSSL
> wrapper to clear the per-queue thread. It's a big mess, but it's our
> mess to some degree.

So your proposal is basically to do #2 in all branches?  I won't fight it,
if it doesn't bloat the code much.  The overhead should surely be trivial
compared to network communication costs, and I'm afraid you might be right
about the risk of latent bugs.

regards, tom lane


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

Re: [HACKERS] Explain [Analyze] produces parallel scan for select Into table statements.

[Robert Haas]

On Sat, Mar 12, 2016 at 1:58 AM, Amit Kapila  wrote:
> Yeah, that makes the addition of test for this functionality difficult.
> Robert, do you have any idea what kind of test would have caught this issue?

Yep.  Committed with that test:

DO $$
BEGIN
   EXECUTE 'EXPLAIN ANALYZE SELECT * INTO TABLE easi FROM int8_tbl';
END$$;

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


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

Re: [HACKERS] Parallel Aggregate

[James Sewell]

On Tue, Mar 15, 2016 at 9:32 AM, Robert Haas  wrote:

>
> I kind of doubt this would work well, but somebody could write a patch
> for it and try it out.


OK I'll give this a go today and report back.

Would the eventual plan be to use pg_proc.procost for the functions from
each aggregate concerned? If so I might have a peek at that too, although I
imagine I won't get far.

Cheers,

-- 


--
The contents of this email are confidential and may be subject to legal or 
professional privilege and copyright. No representation is made that this 
email is free of viruses or other defects. If you have received this 
communication in error, you may not copy or distribute any part of it or 
otherwise disclose its contents to anyone. Please advise the sender of your 
incorrect receipt of this correspondence.