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

[pokurev]

Hi Amit,

> -Original Message-
> From: Amit Langote [mailto:langote_amit...@lab.ntt.co.jp]
> Sent: Wednesday, March 09, 2016 4:29 PM
> To: Kyotaro HORIGUCHI 
> Cc: robertmh...@gmail.com; amitlangot...@gmail.com; SPS ポクレ ヴィ
> ナヤック(三技術) ; pgsql-
> hack...@postgresql.org; SPS 坂野 昌平(三技術) 
> Subject: Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.
> 
> > On 2016/03/08 18:19, Kyotaro HORIGUCHI wrote:
> >> +WHEN 0 THEN 100::numeric(5, 2)
> >> +ELSE ((S.param3 + 1)::numeric / S.param2 *
> 100)::numeric(5, 2)
> >>
> >> This usage of numeric seems overkill to me.
> >
> > Hmm, how could this rather be written?
> 
> OK, agreed about the overkill. Following might be better:
> 
> + WHEN 0 THEN round(100.0, 2)
> + ELSE round((S.param3 + 1) * 100.0 / S.param2, 2)
+1

> Will update that patch.
> 
> 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] [PROPOSAL] VACUUM Progress Checker.

[Amit Langote]

On 2016/03/09 10:11, Amit Langote wrote:
> The attached revision addresses above and one of Horiguchi-san's comments
> in his email yesterday.

I fixed one more issue in 0002 per Horiguchi-san's comment.  Sorry about
so many versions.

Thanks,
Amit
>From 9473230af72e0a0e3b60a8ddf1922698f7f17145 Mon Sep 17 00:00:00 2001
From: Amit 
Date: Sun, 28 Feb 2016 01:50:07 +0900
Subject: [PATCH 1/3] Provide a way for utility commands to report progress

Commands can update a set of parameters in shared memory using:

  pgstat_progress_update_param(param_index, param_value)

Up to 10 independent 64-bit integer values can be published by commands.
In addition to those, a command should always report its BackendCommandType
and the OID of the relation it is about to begin processing at the beginning
of the processing using:

  pgstat_progress_start_command(cmdtype)
  pgstat_progress_set_command_target(relid)

A view can be defined in system_views.sql that outputs the values returned
by pg_stat_get_progress_info(cmdtype), where 'cmdtype' is numeric value as
mentioned above.  Each such view has columns corresponding to the counters
published by respective commands.
---
 src/backend/postmaster/pgstat.c |   60 +++
 src/backend/utils/adt/pgstatfuncs.c |   91 +++
 src/include/catalog/pg_proc.h   |2 +
 src/include/pgstat.h|   25 ++
 4 files changed, 178 insertions(+), 0 deletions(-)

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index da768c6..a7d0133 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -2731,6 +2731,7 @@ pgstat_bestart(void)
 	beentry->st_clienthostname[NAMEDATALEN - 1] = '\0';
 	beentry->st_appname[NAMEDATALEN - 1] = '\0';
 	beentry->st_activity[pgstat_track_activity_query_size - 1] = '\0';
+	beentry->st_command = COMMAND_INVALID;
 
 	pgstat_increment_changecount_after(beentry);
 
@@ -2851,6 +2852,65 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 	pgstat_increment_changecount_after(beentry);
 }
 
+/*---
+ * pgstat_progress_update_param() -
+ *
+ * Update index'th member in st_progress_param[] of own backend entry.
+ *---
+ */
+void
+pgstat_progress_update_param(int index, int64 val)
+{
+	volatile PgBackendStatus *beentry = MyBEEntry;
+
+	if(!beentry)
+		return;
+
+	if (!pgstat_track_activities)
+		return;
+
+	Assert(index >= 0 && index < PGSTAT_NUM_PROGRESS_PARAM);
+
+	pgstat_increment_changecount_before(beentry);
+	beentry->st_progress_param[index] = val;
+	pgstat_increment_changecount_after(beentry);
+}
+
+/*---
+ * pgstat_progress_start_command()-
+ *
+ * Set st_command in own backend entry.  Also, zero-initialize
+ * st_progress_param array.
+ *---
+ */
+void
+pgstat_progress_start_command(BackendCommandType cmdtype)
+{
+	volatile PgBackendStatus *beentry = MyBEEntry;
+
+	pgstat_increment_changecount_before(beentry);
+	beentry->st_command = cmdtype;
+	MemSet(>st_progress_param, 0,
+		   sizeof(beentry->st_progress_param));
+	pgstat_increment_changecount_after(beentry);
+}
+
+/*---
+ * pgstat_progress_set_command_target()-
+ *
+ * Set st_command_target in own backend entry.
+ *---
+ */
+void
+pgstat_progress_set_command_target(Oid relid)
+{
+	volatile PgBackendStatus *beentry = MyBEEntry;
+
+	pgstat_increment_changecount_before(beentry);
+	beentry->st_command_target = relid;
+	pgstat_increment_changecount_after(beentry);
+}
+
 /* --
  * pgstat_report_appname() -
  *
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 1b22fcc..a12310d 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -64,6 +64,7 @@ extern Datum pg_stat_get_backend_xact_start(PG_FUNCTION_ARGS);
 extern Datum pg_stat_get_backend_start(PG_FUNCTION_ARGS);
 extern Datum pg_stat_get_backend_client_addr(PG_FUNCTION_ARGS);
 extern Datum pg_stat_get_backend_client_port(PG_FUNCTION_ARGS);
+extern Datum pg_stat_get_progress_info(PG_FUNCTION_ARGS);
 
 extern Datum pg_stat_get_db_numbackends(PG_FUNCTION_ARGS);
 extern Datum pg_stat_get_db_xact_commit(PG_FUNCTION_ARGS);
@@ -525,6 +526,96 @@ pg_stat_get_backend_idset(PG_FUNCTION_ARGS)
 }
 
 /*
+ * Returns progress parameter values of backends running a given command
+ */
+Datum
+pg_stat_get_progress_info(PG_FUNCTION_ARGS)
+{
+#define PG_STAT_GET_PROGRESS_COLS	PGSTAT_NUM_PROGRESS_PARAM + 2
+	int			num_backends = pgstat_fetch_stat_numbackends();
+	int			curr_backend;
+	int32		cmdtype = PG_GETARG_INT32(0);
+	TupleDesc	tupdesc;
+	Tuplestorestate *tupstore;
+	ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+	MemoryContext per_query_ctx;
+	MemoryContext oldcontext;
+
+	/* check to see if caller supports us returning a tuplestore */
+	if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
+		ereport(ERROR,
+(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ 

Re: [HACKERS] pam auth - add rhost item

[Haribabu Kommi]

On Tue, Mar 8, 2016 at 10:43 PM, Grzegorz Sampolski 
wrote:
> Hi Hari.
> To use pam modules you can use whatever backend authentication method
> you want.
>
> This is example configuration:
>
> Install this library https://github.com/pam-pgsql/pam-pgsql
> Create some example database , schema access and two tables:
> pam_auth and pam_account with example defintion:
>
> pam_account:
> db_user character varying(16) NOT NULL,
> host character varying(255) NOT NULL
>
> pam_auth:
> db_user character varying(16) NOT NULL,
> password character varying(512) NOT NULL
>
> Sample /etc/pam_pgsql.conf:
> connect = dbname= user= password=
> auth_query = SELECT password FROM access.pam_auth WHERE db_user = %u
LIMIT 1
> acct_query = SELECT '0','0','' FROM access.pam_account WHERE db_user =
> %u AND (host = %h OR %h LIKE host) ORDER BY host DESC LIMIT 1;
> pw_type = crypt

Thanks for the details. I am able to test the host limitation based on
the host from where the connection request is given.This patch
provides the advantage of getting the connected host address
details for the PAM modules to provide/restrict the authentication.

A small change in the code, correct the following code from

+ if (retval) {

to

if (retval)
{

as per the code everywhere.


> I will try to update documentation in regard to this chagnes, but please
> take into account that my english isn't fluent so much. So if I'll do
> some mistakes please correct me.

I am also not a good English speaker :), but we can try to provide to
as good as possible, later community can help in correcting it if they find
any problem/improvement.

Regards,
Hari Babu
Fujitsu Australia

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

[Amit Langote]

> On 2016/03/08 18:19, Kyotaro HORIGUCHI wrote:
>> +  WHEN 0 THEN 100::numeric(5, 2)
>> +  ELSE ((S.param3 + 1)::numeric / S.param2 * 
>> 100)::numeric(5, 2)
>>
>> This usage of numeric seems overkill to me.
> 
> Hmm, how could this rather be written?

OK, agreed about the overkill. Following might be better:

+ WHEN 0 THEN round(100.0, 2)
+ ELSE round((S.param3 + 1) * 100.0 / S.param2, 2)

Will update that patch.

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] Pushing down sorted joins

[Ashutosh Bapat]

> This patch needs to be rebased.
>
>
Done.


> +   /*
> +* TODO: we should worry about EPQ path but should
> that path have
> +* pathkeys? I guess, that's not really important
> since it's just
> +* going to evaluate the join from whole-row
> references stuffed in the
> +* corresponding EPQ slots, for which the order doesn't
> matter.
> +*/
>
> The pathkeys for the EPQ path don't matter.  It'll only be called to
> recheck one single row, and there's only one order in which you can
> return one row.
>

Right. Removed the TODO


>
> -   if (bms_equal(em->em_relids, rel->relids))
> +   if (bms_is_subset(em->em_relids, rel->relids))
>
> Why do we need this?
>
>
The function find_em_expr_for_rel() find an equivalence member expression
that has all its Vars come from the given relation. It's not necessary that
it will have Vars from relids that are covered by the given relations. E.g.
in query SELECT A.c1, B.c2 FROM A join B ON ... ORDER BY A.c3, there will
be a single equivalence member A.c3 in the pathkeys and em_relids will
indicate only A. Hence instead of equal, (which used to be OK for single
relation join push-down) we have to use subset operation. We want an
equivalence members whose relids are subset of relids contained by given
relation.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 6479640..48bdbef 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -437,20 +437,54 @@ SELECT t1.c1, t2."C 1" FROM ft2 t1 LEFT JOIN "S 1"."T 1" t2 ON (t1.c1 = t2."C 1"
  103 | 103
  104 | 104
  105 | 105
  106 | 106
  107 | 107
  108 | 108
  109 | 109
  110 | 110
 (10 rows)
 
+-- A join between local table and foreign join. ORDER BY clause is added to the
+-- foreign join so that the local table can be joined using merge join strategy.
+EXPLAIN (COSTS false, VERBOSE)
+	SELECT t1."C 1" FROM "S 1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+ QUERY PLAN 
+
+ Limit
+   Output: t1."C 1"
+   ->  Merge Right Join
+ Output: t1."C 1"
+ Merge Cond: (t3.c1 = t1."C 1")
+ ->  Foreign Scan
+   Output: t3.c1
+   Relations: (public.ft1 t2) INNER JOIN (public.ft2 t3)
+   Remote SQL: SELECT r3."C 1" FROM ("S 1"."T 1" r2 INNER JOIN "S 1"."T 1" r3 ON (TRUE)) WHERE ((r2."C 1" = r3."C 1")) ORDER BY r2."C 1" ASC NULLS LAST
+ ->  Index Only Scan using t1_pkey on "S 1"."T 1" t1
+   Output: t1."C 1"
+(11 rows)
+
+SELECT t1."C 1" FROM "S 1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+ C 1 
+-
+ 101
+ 102
+ 103
+ 104
+ 105
+ 106
+ 107
+ 108
+ 109
+ 110
+(10 rows)
+
 RESET enable_hashjoin;
 RESET enable_nestloop;
 -- ===
 -- WHERE with remotely-executable conditions
 -- ===
 EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = 1; -- Var, OpExpr(b), Const
  QUERY PLAN  
 -
  Foreign Scan on public.ft1 t1
Output: c1, c2, c3, c4, c5, c6, c7, c8
@@ -862,32 +896,29 @@ SELECT count(c3) FROM ft1 t1 WHERE t1.c1 === t1.c2;
 -- ===
 -- JOIN queries
 -- ===
 -- Analyze ft4 and ft5 so that we have better statistics. These tables do not
 -- have use_remote_estimate set.
 ANALYZE ft4;
 ANALYZE ft5;
 -- join two tables
 EXPLAIN (COSTS false, VERBOSE)
 SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c3, t1.c1 OFFSET 100 LIMIT 10;
- QUERY PLAN 
-
+ QUERY PLAN  

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

[Haribabu Kommi]

On Sun, Oct 18, 2015 at 1:03 AM, Tom Lane  wrote:
> Amit Kapila  writes:
>> On Sat, Oct 17, 2015 at 12:07 AM, Robert Haas  wrote:
>>> Maybe we need to be using PostmasterRandom() rather than random() for
>>> the control segment name.
>
>> +1.  Though I think it is better to investigate the actual cause before
>> doing this.
>
> BackendRun() deliberately prevents that from working.  And it also sets
> srandom() to a new value for each subprocess, so that AFAICS this idea
> would be a net negative.  If you are seeing duplicate key values getting
> selected, the problem is elsewhere.

Coming back to an old thread, recently I got a problem in starting two
PostgreSQL services with a user that is not an administrator. The error
message is as follows.

FATAL:  could not create shared memory segment
"Global/PostgreSQL.851401618": Permission denied

The issue is happening only with the processes that are running as service.
I observed that the handle received in creating the dynamic shared memory
is same for two services, because of which the Access denied error is thrown
by the operating system and thus it leads to failure.

The PG shared memory name is always includes the data directory path as
below, because of which it doesn't match with two services.

"Global/PostgreSQL:C:/work/FEP/installation/bin/data"

But whereas the dynamic shared memory is formed with a random number
and this number getting generated same for two service thus it leads to
failure.

"Global/PostgreSQL.85140161"

I tried replacing the random() with PostmaterRandom() for a test and it worked.
This is generating different random values, so the issue is not occurring.

"Global/PostgreSQL.2115609797"

I feel, we should add the the data directory path + the random number to
generate the name for dynamic shared memory, this can fix problem.

comments?

Regards,
Hari Babu
Fujitsu Australia


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

Re: [HACKERS] Optimizer questions

[Tom Lane]

I wrote:
> BTW, there's some additional refactoring I had had in mind to do in
> grouping_planner to make its handling of the targetlist a bit more
> organized; in particular, I'd like to see it using PathTarget
> representation more consistently throughout the post-scan-join steps.

See 51c0f63e4d76a86b44e87876a6addcfffb01ec28 --- I think this gets
things to where we could plug in additional levels of targets without
too much complication.

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 Thu, Mar 3, 2016 at 7:34 PM, Michael Paquier
 wrote:
> WAL replay for 2PC should also
> call XLogRequestWalReceiverReply() when needed.

Ah yes, I missed this important sentence.  I will address that in the
next version after some testing.

-- 
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] Novice Presentation and Company Project

[Noah Misch]

On Thu, Mar 03, 2016 at 12:04:56PM +0100, Eduardo Morras wrote:
> My company is developing code for Postgresql for another company and want to 
> communicate, debate and share the results with the community.
> 
> The objetives are upgrade the network backend and frontend of Postgresql:
> 
> a) Add support for LibreSSL in it 2 flavors, OpenSSL compatibility mode and 
> own API,
> b) Add support for sctp protocol,
> c) Add support for dtls with LibreSSL, with udp/udplite and sctp datagrams,
> d) Add support to them in makefiles/configure and postgresql configuration.
> 
> I have read the FAQ entry [1] and company contributions document [2] and as 
> first step I had search Postgresql mailinglists for similar topics and found 
> nothing. I think this is the second step, share the plan. If you need more 
> information on any topic or has more ideas or howtos or.. reply mail.

Thanks for sharing these goals, which was an ideal first step.  I think your
next step is to pick a self-contained subset of those goals to implement as a
first project.  Then, start a thread describing a planned design for that
particular subset.  I recommend starting either with support for the OpenSSL
compatibility mode of LibreSSL or with SCTP.

When you send a design proposal for SCTP support, please explain why a person
should choose to reach PostgreSQL over SCTP in lieu of TCP.  The topic of SCTP
support in PostgreSQL has never come up before.

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] Crash with old Windows on new CPU

[Christian Ullrich]

* Peter Eisentraut wrote:


On 2/12/16 11:24 AM, Christian Ullrich wrote:



Otherwise, it may be time to update the manual (15.6 Supported
Platforms) where it says PostgreSQL "can be expected to work on these
operating systems: [...] Windows (Win2000 SP4 and later), [...]".
Perhaps we could add "except Windows before 7 SP1/2008R2 SP1 when
running in x64 mode on Intel CPUs introduced after May 2013 (Haswell and
later)"?



Wouldn't the fix be for users to upgrade their service packs?


Windows 2008 and 2008R2 are entirely different things: 2008 is the 
server sibling of Vista (internal version 6.0), 2008R2 is that of 
Windows 7 (6.1). There is no version of 2008 that supports AVX2.


--
Christian



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

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

[Pavan Deolasee]

On Fri, Mar 4, 2016 at 9:16 PM, Jesper Pedersen 
wrote:

>
>>
>>
> I can confirm the marginal speed up in tps due to the new WAL size.
>
> The TWOPHASE_MAGIC constant should be changed, as the file header has
> changed definition, right ?
>

Thanks for looking at it. I've revised the patch by incrementing the
TWOPHASE_MAGIC identifier.

Thanks,
Pavan

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


reduce_gid_wal_v2.patch
Description: Binary data

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

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Noah Misch]

On Tue, Mar 08, 2016 at 10:32:28PM +0900, Michael Paquier wrote:
> Subject: [PATCH 3/4] Fix use of locales for VS 2015
> 
> lc_codepage is a flag missing from locale.h, causing this code path
> introduced in VS 2012 to fail. Perhaps there is a reason for this field
> to have been clobbered, but let's fall back to the pre-VS-2012 code
> parsing directly LC_TYPE to get the codepage wanted.
> ---
>  src/port/chklocale.c | 11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/port/chklocale.c b/src/port/chklocale.c
> index a551fdc..a7d88fb 100644
> --- a/src/port/chklocale.c
> +++ b/src/port/chklocale.c
> @@ -203,7 +203,16 @@ win32_langinfo(const char *ctype)
>  {
>   char   *r = NULL;
>  
> -#if (_MSC_VER >= 1700)
> + /*
> +  * lc_codepage is correctly declared in Visual Studio 2012 and 2013.
> +  * However in VS 2015 this flag is missing from locale.h, visibly this
> +  * is an error of refactoring from Microsoft that is at the origin of
> +  * this missing field, causing a compilation failure in this code path.
> +  * Hence, it is more reliable to fall back to other code path grabbing

No, the other path can't handle a "locale name" like "initdb --locale=th-TH".
That makes the other code path inadequate for VS2012 and later.  See the
IsoLocaleName() header comment.

> +  * the codepage from the ctype name itself. If VS gets back this field
> +  * in the future, we may want to relax the use of _create_locale here.
> +  */
> +#if (_MSC_VER >= 1700) && (_MSC_VER <= 1800)
>   _locale_t   loct = NULL;
>  
>   loct = _create_locale(LC_CTYPE, ctype);


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

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

[Andreas Karlsson]

On 03/08/2016 01:24 AM, Regina Obe wrote:

On Fri, Mar 4, 2016 at 9:29 PM, Regina Obe > wrote:
I think the answer to this question is NO, but thought I'd ask.

A lot of folks in PostGIS land are suffering from restore issues,
materialized view issues etc. because we have functions such as

ST_Intersects

Which does _ST_Intersects  AND &&

Since _ST_Intersects is not schema qualified, during database restore
(which sets the schema to the table or view schema), materialized
views that depend on this do not come back.



Could you provide a self-contained, reproducible test case that illustrates 
this problem?  Ideally, one that doesn't involve installing PostGIS?


Here is a script just involving the built in geometric types that has the same 
issue:


Hi,

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

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


Andreas


--
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] Recovery test failure for recovery_min_apply_delay on hamster

[Alvaro Herrera]

Michael Paquier wrote:
> On Wed, Mar 9, 2016 at 12:14 AM, Alvaro Herrera
>  wrote:
> > Is there anything we can do to short-circuit the wait in the case that
> > replication happens promptly?  A one-minute wait would be acceptable we
> > terminate it early by checking every second.
> 
> After sleeping (best debugger ever) on that, actually a way popped up
> in my mind, and I propose the attached, which refactors a bit 005 and
> checks that the LSN position of master has been applied on standby
> after at least the delay wanted. A maximum delay of 90s is authorized,
> like poll_query_until.

Hmm, okay, that's great.  A question: what happens if the test itself is
slow and the servers are fast, and the test doesn't manage to run two
iterations before the two seconds have elapsed?  This may happen on
overloaded or slow servers, if you're unlucky.

I don't have any ideas on ensuring that we don't apply earlier than the
given period at the moment.

-- 
Á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] fun with "Ready for Committer" patches

[Andres Freund]

On 2016-03-09 08:18:09 +0900, Tatsuo Ishii wrote:
> > It's hard to miss the fact that there are an absolutely breathtaking
> > number of patches in this CommitFest - 80! - that are in the "needs
> > review" state.  We really, really, really need more review to happen -
> 
> Many of "needs review" state patches already have reviewer(s). Do you
> mean we want more reviewers in addition to them for such patches?

Any review performed is worthwhile. Somebody having signed up to review
a patch shouldn't stop you. Especially if that signup was more than a
couple hours ago.

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] fun with "Ready for Committer" patches

[Craig Ringer]

On 9 March 2016 at 07:18, Tatsuo Ishii  wrote:


>
> Many of "needs review" state patches already have reviewer(s). Do you
> mean we want more reviewers in addition to them for such patches?
>

Yeah. Personally I'm not too confident about what precisely is required to
move a patch from needs-review to ready-for-committer. I've done a chunk of
review for a number of patches, but I'm not always confident saying "all
clear, proceed".

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

Re: [HACKERS] syslog configurable line splitting behavior

[Andreas Karlsson]

On 03/09/2016 03:20 AM, Peter Eisentraut wrote:

On 3/8/16 9:12 PM, Andreas Karlsson wrote:

As someone who uses syslog for my servers I find both of these GUCs
useful, especially when used in combination, and I do not think a
compile time option like suggest by Alexander would be suitable
substitute because then I would need a custom build of PostgreSQL just
to change this which seems too much effort just for this.


I think he was suggesting to take the existing compile-time constant and
make a run-time setting out of it.


Ah, that makes more sense than my reading of his message. Still I do not 
think there would be much gain from doing so. I do not see much benefit 
from making it configurable. In the cases I envision you would either 
want to split with a log file in mind or not split at all.


Andreas



--
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] Crash with old Windows on new CPU

[Peter Eisentraut]

On 2/12/16 11:24 AM, Christian Ullrich wrote:
> Otherwise, it may be time to update the manual (15.6 Supported
> Platforms) where it says PostgreSQL "can be expected to work on these
> operating systems: [...] Windows (Win2000 SP4 and later), [...]".
> Perhaps we could add "except Windows before 7 SP1/2008R2 SP1 when
> running in x64 mode on Intel CPUs introduced after May 2013 (Haswell and
> later)"?

Wouldn't the fix be for users to upgrade their service packs?


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

Re: [HACKERS] multivariate statistics v11

[Jeff Janes]

On Tue, Mar 8, 2016 at 12:13 PM, Tomas Vondra
 wrote:
> Hi,
>
> attached is v11 of the patch - this is mostly a cleanup of v10, removing
> redundant code, adding missing comments, removing obsolete FIXME/TODOs
> and so on. Overall this shaves ~20kB from the patch (not a primary
> objective, though).

This has some some conflicts with the pathification commit, in the
regression tests.

To avoid that, I applied it to the commit before that, 3fc6e2d7f5b652b417fa6^

Having done that, In my hands, it fails its own regression tests.
Diff attached.

It breaks contrib postgres_fdw, I'll look into that when I get a
chance of no one beats me to it.

postgres_fdw.c: In function 'postgresGetForeignJoinPaths':
postgres_fdw.c:3623: error: too few arguments to function
'clauselist_selectivity'
postgres_fdw.c:3642: error: too few arguments to function
'clauselist_selectivity'

Cheers,

Jeff


regression.diffs
Description: Binary data

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

Re: [HACKERS] syslog configurable line splitting behavior

[Peter Eisentraut]

On 3/8/16 9:12 PM, Andreas Karlsson wrote:
> As someone who uses syslog for my servers I find both of these GUCs
> useful, especially when used in combination, and I do not think a
> compile time option like suggest by Alexander would be suitable
> substitute because then I would need a custom build of PostgreSQL just
> to change this which seems too much effort just for this.

I think he was suggesting to take the existing compile-time constant and
make a run-time setting out of it.


-- 
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] syslog configurable line splitting behavior

[Peter Eisentraut]

On 3/4/16 11:01 AM, Alexander Korotkov wrote:
> On Sat, Feb 27, 2016 at 6:49 AM, Peter Eisentraut  > wrote:
> 
> Writing log messages to syslog caters to ancient syslog implementations
> in two ways:
> 
> - sequence numbers
> - line splitting
> 
> While these are arguably reasonable defaults, I would like a way to turn
> them off, because they get in the way of doing more interesting things
> with syslog (e.g., logging somewhere that is not just a text file).
> 
> So I propose the two attached patches that introduce new configuration
> Boolean parameters syslog_sequence_numbers and syslog_split_lines that
> can toggle these behaviors.
> 
> 
> Would it have any usage if we make PG_SYSLOG_LIMIT configurable (-1 for
> disable) instead of introducing boolean?

That would work, too.  But then we'd need another setting to disable
splitting on newlines.  That way we'd have more settings, but they
actually mirror the corresponding settings on the rsyslogd side better.



-- 
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] syslog configurable line splitting behavior

[Andreas Karlsson]

On 02/27/2016 04:49 AM, Peter Eisentraut wrote:

So I propose the two attached patches that introduce new configuration
Boolean parameters syslog_sequence_numbers and syslog_split_lines that
can toggle these behaviors.


As someone who uses syslog for my servers I find both of these GUCs 
useful, especially when used in combination, and I do not think a 
compile time option like suggest by Alexander would be suitable 
substitute because then I would need a custom build of PostgreSQL just 
to change this which seems too much effort just for this.


The code itself is clean and I find the documentation to be easy to 
understand.


I have one nitpick: why is one of the variables "true" while the other 
is "on" in the example? I think both should be "on".


#syslog_sequence_numbers = true
#syslog_split_lines = on

Another possible improvement would be to change "Split messages sent to 
syslog." to something more verbose like "Split messages sent to syslog, 
by lines and to fit in 1024 bytes.".


I tested the code and it worked as advertised and also passed the test 
suite. My local rsyslogd cut the message at about 8 kB, but I do not 
think it is worth it to expose PG_SYSLOG_LIMIT as a GUC.


Andreas


--
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.

[Amit Langote]

Horiguchi-san,

Thanks for the review!

On 2016/03/08 18:19, Kyotaro HORIGUCHI wrote:
>> Updated versions attached.
>>
>> * changed st_progress_param to int64 and so did the argument of
>> pgstat_progress_update_param().  Likewise changed param1..param10 of
>> pg_stat_get_progress_info()'s output columns to bigint.
>>
>> * Added back the Oid field st_command_target and corresponding function
>> pgstat_progress_set_command_target(Oid).
> 
> + beentry->st_command = COMMAND_INVALID;
> + MemSet(>st_progress_param, 0, 
> sizeof(beentry->st_progress_param));
> 
> The MemSet seems useless since it gets the same initialization on
> setting st_command.

Right, every backend start should not have to pay that price.  Fixed in
the latest version.

> 
> + /*
> +  * Report values for only those backends which are running the 
> given
> +  * command.  XXX - privilege check is maybe dubious.
> +  */
> + if (!beentry ||
> + beentry->st_command != cmdtype ||
> + !has_privs_of_role(GetUserId(), beentry->st_userid))
> + continue;
> 
> We can simplly ignore unpriviledged backends, or all zeroz or
> nulls to signal that the caller has no priviledge.

As suggested by Robert, used pg_stat_get_activity() style.  In this case,
show 'pid' to all but the rest only to role members.

> 0002
> 
> +   FROM pg_stat_get_progress_info(1) AS S;
> 
> Ah... This magick number seems quite bad.. The function should
> take the command type in maybe string type.
> 
> +   FROM pg_stat_get_progress_info('lazy vacuum') AS S;
> 
> Using an array of the names would be acceptable, maybe.
> 
> | char *progress_command_names[] = {'lazy vacuum', NULL};

Hm, I think the way it is *may* be OK the way it is, but...

As done in the patch, the way we identify commands is with the enum
BackendCommandType:

+typedef enum BackendCommandType
+{
+COMMAND_INVALID = 0,
+COMMAND_LAZY_VACUUM
+} BackendCommandType;

Perhaps we could create a struct:

typedef struct PgStatProgressCommand
{
char *cmd_name;
BackendCommandTypecmd_type;
} PgStatProgressCommand;

static const struct PgStatProgressCommand commands[] = {
{"vacuum", COMMAND_LAZY_VACUUM},
{NULL, COMMAND_INVALID}
};

> However the numbers for the phases ('scanning heap' and so..) is
> acceptable for me for reasons uncertain to me, it also could be
> represented in names but is might be rahter bothersome..

In initial versions of the patch, it used to be char * that was passed for
identifying phases.  But, then we got rid of char * progress parameters
altogether. So, there are no longer any text columns in
pg_stat_get_progress_info()'s result.  It may not work out well in long
run to forever not have those (your recent example comes to mind).

> 
> +   WHEN 0 THEN 100::numeric(5, 2)
> +   ELSE ((S.param3 + 1)::numeric / S.param2 * 
> 100)::numeric(5, 2)
> 
> This usage of numeric seems overkill to me.

Hmm, how could this rather be written?

>> * I attempted to implement a method to report index blocks done from
>> lazy_tid_reaped() albeit with some limitations. Patch 0003 is that
>> attempt.  In summary, I modified the index bulk delete callback interface
>> to receive a BlockNumber argument index_blkno:

[ snip ]

>> way of traversing the index pages. I dared only touch the btree case to
>> make it pass current block number to the callback. It finishes with
>> index_blks_done << total_index_blks since I guess the callback is called
>> only on the leaf pages. Any ideas?
> 
> To the contrary, I suppose it counts one index page more than
> once for the cases of uncorrelated heaps. index_blks_vacuumd can
> exceed RelationGetNumberOfBlocks() in extreme cases. If I'm not
> missing something, it stands on a quite fragile graound.

Yeah, the method is not entirely foolproof yet.

>> * I am also tempted to add num_dead_tuples and dead_tuples_vacuumed to add
>> granularity to 'vacuuming heap' phase but didn't in this version. Should we?
> 
> How do you think they are to be used?

I just realized there are objections to some columns be counters for pages
and others counting tuples.  So, I guess I withdraw.  I am just worried
that 'vacuuming heap' phase may take arbitrarily long if dead tuples array
is big.  Since we were thinking of adding more granularity to 'vacuuming
indexes' phase, I thought we should do for the former too.

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] Managing a long-held TupleDesc reference

[Tom Lane]

Chapman Flack  writes:
> When PL/Java is told to map a PostgreSQL composite type to a certain
> Java class, on its first use of the type mapping it calls
> lookup_rowtype_tupdesc_noerror and then creates a PL/Java UDT structure
> that retains a reference to the TupleDesc. This seems to be what is leading
> to a TupleDesc reference leak warning at completion of the transaction.

> So I am wondering what a recommended way of managing these TupleDescs
> would be. I could use lookup_rowtype_tupdesc_copy before saving it in
> the UDT struct, and I assume that would silence the leak warning, but
> would that be asking for trouble if the composite type gets altered
> during the session and the saved TupleDesc is stale?

Probably, but holding a refcounted reference doesn't make that better.
If the typcache updates, it'll make a new TupleDesc and release its
own hold on the one you've got; it doesn't cause that one to change
in-place.

> If that's an issue, what would be better? Should UDT not retain a
> TupleDesc, but rather look it up with each use?

Probably the safest answer.  If you go through typcache then it will
avoid unnecessary recalculations of the type's tupdesc.  It means one
extra hashtable lookup per reference, but that's not that awful.

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: Access method extendability

[Petr Jelinek]

Hi,

I went over the latest version of this, here are my notes.

create-am.9:

+   case DO_ACCESS_METHOD:
+   snprintf(buf, bufsize,
+"");
+   return;

Missing the actual description.

+   appendPQExpBuffer(q, "CREATE ACCESS METHOD %s HANDLER %s;\n",
+ qamname, aminfo->amhandler);

Generates invalid syntax (missing TYPE).

I don't like that you hardcode 'i' everywhere in the code as am type 
index. It would be better to define it in pg_am.h and use that.


I was also debating in my head if amtype is correct name as it maps to 
relkind so amkind might be better name for the column, otoh then it 
won't map to the syntax we have agreed on for CREATE ACCESS METHOD so I 
guess there is no ideal name here.


object_type_map record is missing, as is getObjectTypeDescription and 
getObjectIdentityParts support


(you can check what I sent as part of sequence access methods where I 
fixed these things, otherwise it reuses most of your code)



generic-xlog.9:
Not much to say here, the api makes sense to me, it will have 
performance impact but don't see how we can do this generically without 
callbacks to extension in any better way.


One thing I don't understand is why there is support for unlogged 
relations. Shouldn't that be handled by whoever is using this to 
actually not call this at all? It's just call to RelationNeedsWAL() 
nothing to hard and hidden magic like not doing anything with WAL for 
the unlogged tables is seldomly good idea.


Another small thing is that we put the API explanation comments into .c 
file not .h file.


Didn't look at the bloom index too deeply yet.


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

[HACKERS] Managing a long-held TupleDesc reference

[Chapman Flack]

When PL/Java is told to map a PostgreSQL composite type to a certain
Java class, on its first use of the type mapping it calls
lookup_rowtype_tupdesc_noerror and then creates a PL/Java UDT structure
that retains a reference to the TupleDesc. This seems to be what is leading
to a TupleDesc reference leak warning at completion of the transaction.

So I am wondering what a recommended way of managing these TupleDescs
would be. I could use lookup_rowtype_tupdesc_copy before saving it in
the UDT struct, and I assume that would silence the leak warning, but
would that be asking for trouble if the composite type gets altered
during the session and the saved TupleDesc is stale?

If that's an issue, what would be better? Should UDT not retain a
TupleDesc, but rather look it up with each use? (That would happen
in preparation for calling a Java function with a parameter or return
of that type, or when a Java function makes JDBC-SPI calls touching
that type.) Reading typcache.c, I get the impression that it may be
intended to be fast enough for such usage, and that it manages the
complexity of invalidating entries when something gets altered.

Should there be some intermediate solution where UDT does retain a
TupleDesc reference within a transaction, but certain callbacks are
registered to know when to release or refresh it?

Is there a canonical, preferred approach?

Thanks,
-Chap


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

Re: [HACKERS] Declarative partitioning

[Corey Huinker]

>
> > So presently partitions that are unbounded on the lower end aren't
> > possible, but that's a creation syntax issue, not an infrastructure
> issue.
> > Correct?
>
> In case it wasn't apparent, you can create those:
>
> FOR VALUES END (upper-bound) [INCLUSIVE]
>
> which is equivalent to:
>
> FOR VALUES '(, upper-bound)' or FOR VALUES '(, upper-bound]'
>
>
Thanks for clarifying. My BNF-fu is weak.


>
>

Re: [HACKERS] Performance improvement for joins where outer side is unique

[Tom Lane]

David Rowley  writes:
> [1] http://www.postgresql.org/message-id/8907.1440383...@sss.pgh.pa.us

Oh, okay, I had looked at the many changes in the regression outputs and
jumped to the conclusion that you were printing the info all the time.
Looking closer I see it's only coming out in VERBOSE mode.  That's
probably fine.  Never mind

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] Optimizer questions

[Tom Lane]

Konstantin Knizhnik  writes:
> On 03/08/2016 07:01 AM, Tom Lane wrote:
>> I've not spent a lot of time on this, but I think maybe what would make
>> sense is to consider both the case where function calculations are
>> postponed to after ORDER BY and the case where they aren't, and generate
>> Paths for both.  Neither approach is a slam-dunk win.  For example,
>> suppose that one of the tlist columns is md5(wide_column) --- it will
>> likely not be preferable to pass the wide column data through the sort
>> step rather than reducing it to a hash first.  This would require some
>> work in grouping_planner to track two possible pathtargets, and work in
>> create_ordered_paths to generate paths for both possibilities.  A possible
>> objection is that this would add planning work even when no real benefit
>> is possible; so maybe we should only consider the new way if the tlist has
>> significant eval cost?  Not sure about that.  There is also something
>> to be said for the idea that we should try to guarantee consistent
>> semantics when the tlist contains volatile functions.

> Attached please find rebased patch.

This may be rebased, but it doesn't seem to respond to any of my concerns
above.  In particular, if we're going to change behavior in this area,
I think we need to try to ensure that volatile functions in the tlist will
see consistent behavior no matter which plan shape is chosen.  People may
well be depending on the existing behavior for particular queries.  If
we're going to break their queries, I'd at least like to be able to say
that there's now a predictable, well-defined semantics.  Something like
"volatile functions in the tlist that are not in sort/group columns are
certain to be evaluated after sorting occurs, if there is an ORDER BY".
This should not be dependent on whether there's a LIMIT, nor GROUP BY
nor windowing functions, nor on random other whims of the optimizer.
So I'm not at all happy with a patch that changes things only for the
LIMIT-but-no-grouping-or-windows case.

I'm not sure whether it's worth pursuing cost-based comparisons for
functions that aren't volatile.  In an ideal world we could deal with the
md5() example I gave, but as things stand I suspect we usually have too
little idea whether the result of f(x) is wider or narrower than x itself.
(One thing we do know is that f's output won't be toasted, whereas x might
be; so it might be a bad idea to bet on function results getting
narrower.)  So I'm afraid it's going to be really hard to tell whether
we're making the sort step itself more or less expensive if we postpone
function evals.  But we do know for certain that we're adding a projection
step that wasn't there before when we postpone functions to after the
Sort.  That kind of suggests that there should be some minimum estimated
cost of the functions before we add that extra step (unless they are
volatile of course).  Or only do it if there is a LIMIT or we have a
tuple_fraction suggesting that partial eval of the query is of interest.

BTW, there's some additional refactoring I had had in mind to do in
grouping_planner to make its handling of the targetlist a bit more
organized; in particular, I'd like to see it using PathTarget
representation more consistently throughout the post-scan-join steps.
I had figured that was just neatnik-ism and could be done later,
but we may need to do it now in order to be able to deal with these
considerations in a cleaner fashion.  I really don't like the way
that this patch hacks up the behavior of make_scanjoin_target() without
even a comment explaining its new API.

The rough sketch of what I'd had in mind is that we should convert the
processed, final tlist into PathTarget form immediately after
query_planner, since we know we're going to need that eventually anyway.
Then, if we need to do grouping/aggregation or windowing, derive modified
PathTargets that we want to use for the inputs of those steps.  (This'd
require some infrastructure that doesn't currently exist for manipulating
PathTargets, particularly the ability to copy a PathTarget and/or add a
column to an existing PathTarget.)

The way this optimization would fit into that structure is that the
DISTINCT/ORDER BY steps would now also have a desired input pathtarget
that might be different from the final target.

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] VACUUM Progress Checker.

[Amit Langote]

On 2016/03/09 9:22, Kyotaro HORIGUCHI wrote:
>> On Wed, Mar 9, 2016 at 12:24 AM, Robert Haas  wrote:
>>> This patch has been worked on by so many people and reviewed by so
>>> many people that I can't keep track of who should be credited when it
>>> gets committed.  Could someone provide a list of author(s) and
>>> reviewer(s)?
>>
>> Original authors are Rahila Syed and Vinayak Pokale.
>>
>> I have been reviewing this for last few CFs. I sent in last few
>> revisions as well.
> 
> The owner of this is Vinayak and, ah, I forgot to add myself as a
> reviewer. I have also reviewed this for last few CFs. 
> 
> So, as looking into CF app, it seems not so inconsistent with the
> persons who appears in this thread for thses three CFs.
> 
> Authors: Vinayak Pokale, Rahila Syed, Amit Langote
> Reviewers: Amit Langote, Kyotaro Horiguchi
> 
> Is there anyone who shold be added in this list?

Jim Nasby, Thom Brown, Masahiko Sawada, Fujii Masao, Masanori Oyama and of
course, Robert himself.

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] Parallel query fails on standby server

[Michael Paquier]

On Wed, Mar 9, 2016 at 12:34 AM, Robert Haas  wrote:
> On Tue, Mar 8, 2016 at 8:23 AM, Michael Paquier
>  wrote:
>> On Tue, Mar 8, 2016 at 9:51 PM, Craig Ringer  wrote:
>>> On 8 March 2016 at 20:30, Ashutosh Sharma  wrote:

 While testing a parallel scan feature on standby server, it is found that
 the parallel query fails with an error "ERROR:  failed to initialize
 transaction_read_only to 0".

>>>
>>> Looks like it might be a good idea to add some tests to src/test/recovery
>>> for parallel query on standby servers...
>>
>> An even better thing would be a set of read-only tests based on the
>> database "regression" generated by make check, itself run with
>> pg_regress.
>
> I'm not sure anything in the main regression suite actually goes
> parallel right now, which is probably the first thing to fix.
>
> Unless, of course, you use force_parallel_mode=regress, max_parallel_degree>0.

I was thinking about a test in src/test/recovery, that runs a standby
and a master. pg_regress with the main recovery test suite is run on
the master, then a second pg_regress run happens with a set of
read-only queries, set with sql/expected located in src/test/recovery
directly for example. Do we actually have a buildfarm animal using
those parameters in extra_config?
-- 
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] Recovery test failure for recovery_min_apply_delay on hamster

[Michael Paquier]

On Wed, Mar 9, 2016 at 12:14 AM, Alvaro Herrera
 wrote:
> Is there anything we can do to short-circuit the wait in the case that
> replication happens promptly?  A one-minute wait would be acceptable we
> terminate it early by checking every second.

After sleeping (best debugger ever) on that, actually a way popped up
in my mind, and I propose the attached, which refactors a bit 005 and
checks that the LSN position of master has been applied on standby
after at least the delay wanted. A maximum delay of 90s is authorized,
like poll_query_until.
-- 
Michael
diff --git a/src/test/recovery/t/005_replay_delay.pl b/src/test/recovery/t/005_replay_delay.pl
index 986851b..f95b5ab 100644
--- a/src/test/recovery/t/005_replay_delay.pl
+++ b/src/test/recovery/t/005_replay_delay.pl
@@ -3,7 +3,7 @@ use strict;
 use warnings;
 use PostgresNode;
 use TestLib;
-use Test::More tests => 2;
+use Test::More tests => 1;
 
 # Initialize master node
 my $node_master = get_new_node('master');
@@ -32,18 +32,30 @@ $node_standby->start;
 # depending on the delay of 2s applied above.
 $node_master->safe_psql('postgres',
 	"INSERT INTO tab_int VALUES (generate_series(11,20))");
-sleep 1;
 
-# Here we should have only 10 rows
-my $result = $node_standby->safe_psql('postgres', "SELECT count(*) FROM tab_int");
-is($result, qq(10), 'check content with delay of 1s');
-
-# Now wait for replay to complete on standby
+# Now wait for replay to complete on standby. We check if the current
+# LSN of master has been applied after at least 2s.
 my $until_lsn =
   $node_master->safe_psql('postgres', "SELECT pg_current_xlog_location();");
-my $caughtup_query =
-  "SELECT '$until_lsn'::pg_lsn <= pg_last_xlog_replay_location()";
-$node_standby->poll_query_until('postgres', $caughtup_query)
-  or die "Timed out while waiting for standby to catch up";
-$result = $node_standby->safe_psql('postgres', "SELECT count(*) FROM tab_int");
-is($result, qq(20), 'check content with delay of 2s');
+
+my $max_attempts = 90;
+my $attempts = 0;
+while ($attempts < $max_attempts)
+{
+	my $replay_status =
+	  $node_standby->safe_psql('postgres',
+		"SELECT (pg_last_xlog_replay_location() - '$until_lsn'::pg_lsn) >= 0");
+
+	# Leave now if standby has replayed at least up to the point of
+	# master.
+	last if $replay_status eq 't';
+
+	sleep 1;
+	$attempts++;
+}
+
+die "Maximum number of attempts reached" if $attempts >= $max_attempts;
+
+# This test is valid only if LSN has been applied with at least
+# the minimum apply delay expected.
+ok($attempts >= 2, 'Check if LSN is applied on standby after 2s');

-- 
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: RETURNING primary_key()

[Craig Ringer]

On 9 March 2016 at 04:12, Robert Haas  wrote:


> I think we have a general problem with the server lacking
> certain capabilities that make it easy to implement a high-quality
> JDBC driver.  And I think it would be good to work on figuring out how
> to fix that.


There are a few frustrations, to be sure, but I'm not sure there's actually
a ton server-side that drastically limits the driver.

One of the worst problems (IMO) is in the driver architecture its self. It
attempts to prevent blocking by guestimating the server's send buffer state
and its recv buffer state, trying to stop them filling and causing the
server to block on writes. It should just avoid blocking on its own send
buffer, which it can control with confidence. Or use some of Java's rather
good concurrency/threading features to simultaneously consume data from the
receive buffer and write to the send buffer when needed, like pgjdbc-ng
does. This makes making use of the pipelining features in Pg's protocol way
harder and less efficient than it should be - but then, PgJDBC still does
this better than libpq, which can't pipeline queries at all.

There certainly are server/protocol frustrations.

QUERY CANCEL RACES
---

Query cancellation sucks badly. Not because it requires a new connection,
though that's unfortunate, but because cancel is backend-level not
statement-level. A statement cancellation key returned as an immediate
response to the Execute message would be quite handy, so we could include
it in cancel requests and eliminate the race by having the cancel request
be a no-op if the statement cancel key doesn't match the currently running
statement.

EARLY CONNECTION CHARSETS
---

There's no way to know the charset of early connection error messages,
which is a flaw in the protocol that isn't specific to PgJDBC its self.
Similarly, you can't specify the text encoding of usernames, passwords, etc
sent to the server.

PER-QUERY GUCs
---

We also have no way to set GUCs per-query, and we need it for
statement_timeout. I really wish Parse and Execute messages allowed
statement-scoped GUCs to be passed at the protocol level. This would be
very, very helpful. The driver can probably work around it by fetching and
then SETing statement_timeout, running the query, then reSETing it
afterwards in a piplelined set of queries, but  yuck. Also, log spam
galore.

GENERATED KEYS AND RETURNING
---

To get generated keys we have to hack the statement text. There's no
protocol-level equivalent, like we have for row-count limits in the v3
protocol. The ability to specify the set of returned columns at the
protocol level would be very nice. That said, hacking the statement text
isn't *too* bad, mostly because few people are going to do their own
RETURNING statement *and* request generated keys from the driver, the only
time this becomes an issue.

STRING TYPE ISSUES
---

PgJDBC can work around Pg's IMO somewhat overzealous type checks for string
types by passing string parameters as being of unknown-type. The JDBC
interface offers us no easy way to differentiate between "this parameter is
a real textual value" and "this parameter is a string representation of
something that might be another type". We can do it with setObject and
extension class wrappers, but then the user has to import the JDBC driver's
classes directly, use PgJDBC-specific API, etc. The people who have the
most problem with our current behaviour are those least able to do that,
users who're behind a query generation layer or ORM. I'd like to just make
stringtype=unspecified the default in PgJDBC and be done with it; users can
still specify an explicit cast to 'text' in the SQL if they want

PROTOCOL-LEVEL SAVEPOINTS
---

psqlODBC would benefit from protocol-level SAVEPOINT and ROLLBACK TO
SAVEPOINT, mostly to reduce logspam and parser overhead. PgJDBC would be
able to use this to emulate other DBMSes error handling behaviour too, when
requested by a client. (Yes, I know about the correctness and performance
issues, but you tell that to someone who just wants to Port Their Stuff
>From Oracle But Can't Change The Code).

SERVER_VERSION_NUM
---

server_version_num should be GUC_REPORT and it's really annoying that it
isn't. I never agreed with the arguments about why that wasn't changed, and
I still want it changed.


LOST TYPMOD, NULLABILITY INFO
---

The server throws away typmod and nullability knowledge as soon as you do
anything with a column. This is frustrating for the driver's metadata API
support. Having result columns marked non-null in Describe would be handy.

LAZY BYTEA
---

The protocol offers no way to lazily fetch large values like BYTEA. Many
vendors can fetch small results and return a handle that gets larger
results from the server on-demand. This means that many clients expect that

SELECT * FROM my_table_with_100MB_bytea_column;

will not fetch all those bytea values to the client until/unless they're
actually accessed. They don't have 

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

[Amit Langote]

On 2016/03/09 0:24, Robert Haas wrote:
> On Tue, Mar 8, 2016 at 3:02 AM, Amit Langote
>  wrote:
>> Updated versions attached.
>>
>> * changed st_progress_param to int64 and so did the argument of
>> pgstat_progress_update_param().  Likewise changed param1..param10 of
>> pg_stat_get_progress_info()'s output columns to bigint.
>>
>> * Added back the Oid field st_command_target and corresponding function
>> pgstat_progress_set_command_target(Oid).
> 
> What the heck do we have an SQL-visible pg_stat_reset_local_progress()
> for?  Surely if we ever need that, it's a bug.

OK, now I am not sure what I was thinking adding that function. Removed.

> I think pgstat_progress_update_param() should Assert(index >= 0 &&
> index < N_PROGRESS_PARAM).  But I'd rename N_PROGRESS_PARAM to
> PGSTAT_NUM_PROGRESS_PARAM.

Agreed, done.

> Regarding "XXX - privilege check is maybe dubious" - I think the
> privilege check here should match pg_stat_activity.  If it does,
> there's nothing dubious about that IMHO.

OK, done.  So, it shows pid column to all, while rest of the values -
relid, param1..param10 are only shown to role members.  Unlike
pg_stat_activity, there is no text column to stash a "" message into, so all that's done is to output null values.

The attached revision addresses above and one of Horiguchi-san's comments
in his email yesterday.

Thanks a lot for the review.

Thanks,
Amit
>From 9473230af72e0a0e3b60a8ddf1922698f7f17145 Mon Sep 17 00:00:00 2001
From: Amit 
Date: Sun, 28 Feb 2016 01:50:07 +0900
Subject: [PATCH 1/3] Provide a way for utility commands to report progress

Commands can update a set of parameters in shared memory using:

  pgstat_progress_update_param(param_index, param_value)

Up to 10 independent 64-bit integer values can be published by commands.
In addition to those, a command should always report its BackendCommandType
and the OID of the relation it is about to begin processing at the beginning
of the processing using:

  pgstat_progress_start_command(cmdtype)
  pgstat_progress_set_command_target(relid)

A view can be defined in system_views.sql that outputs the values returned
by pg_stat_get_progress_info(cmdtype), where 'cmdtype' is numeric value as
mentioned above.  Each such view has columns corresponding to the counters
published by respective commands.
---
 src/backend/postmaster/pgstat.c |   60 +++
 src/backend/utils/adt/pgstatfuncs.c |   91 +++
 src/include/catalog/pg_proc.h   |2 +
 src/include/pgstat.h|   25 ++
 4 files changed, 178 insertions(+), 0 deletions(-)

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index da768c6..a7d0133 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -2731,6 +2731,7 @@ pgstat_bestart(void)
 	beentry->st_clienthostname[NAMEDATALEN - 1] = '\0';
 	beentry->st_appname[NAMEDATALEN - 1] = '\0';
 	beentry->st_activity[pgstat_track_activity_query_size - 1] = '\0';
+	beentry->st_command = COMMAND_INVALID;
 
 	pgstat_increment_changecount_after(beentry);
 
@@ -2851,6 +2852,65 @@ pgstat_report_activity(BackendState state, const char *cmd_str)
 	pgstat_increment_changecount_after(beentry);
 }
 
+/*---
+ * pgstat_progress_update_param() -
+ *
+ * Update index'th member in st_progress_param[] of own backend entry.
+ *---
+ */
+void
+pgstat_progress_update_param(int index, int64 val)
+{
+	volatile PgBackendStatus *beentry = MyBEEntry;
+
+	if(!beentry)
+		return;
+
+	if (!pgstat_track_activities)
+		return;
+
+	Assert(index >= 0 && index < PGSTAT_NUM_PROGRESS_PARAM);
+
+	pgstat_increment_changecount_before(beentry);
+	beentry->st_progress_param[index] = val;
+	pgstat_increment_changecount_after(beentry);
+}
+
+/*---
+ * pgstat_progress_start_command()-
+ *
+ * Set st_command in own backend entry.  Also, zero-initialize
+ * st_progress_param array.
+ *---
+ */
+void
+pgstat_progress_start_command(BackendCommandType cmdtype)
+{
+	volatile PgBackendStatus *beentry = MyBEEntry;
+
+	pgstat_increment_changecount_before(beentry);
+	beentry->st_command = cmdtype;
+	MemSet(>st_progress_param, 0,
+		   sizeof(beentry->st_progress_param));
+	pgstat_increment_changecount_after(beentry);
+}
+
+/*---
+ * pgstat_progress_set_command_target()-
+ *
+ * Set st_command_target in own backend entry.
+ *---
+ */
+void
+pgstat_progress_set_command_target(Oid relid)
+{
+	volatile PgBackendStatus *beentry = MyBEEntry;
+
+	pgstat_increment_changecount_before(beentry);
+	beentry->st_command_target = relid;
+	pgstat_increment_changecount_after(beentry);
+}
+
 /* --
  * pgstat_report_appname() -
  *
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 1b22fcc..a12310d 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -64,6 +64,7 @@ extern Datum 

Re: [HACKERS] Declarative partitioning

[Amit Langote]

Hi,

On 2016/03/09 9:17, Corey Huinker wrote:
>>
>> Sorry for replying so late.
> No worries! We have jobs to do aside from this.

Thanks!

>> Everything seemed to go dandy until I tried FOR VALUES (blah , blah],
>> where psql wouldn't send the command string without accepting the closing
>> parenthesis, :(.  So maybe I should try to put the whole thing in '', that
>> is, accept the full range_spec in a string, but then we are back to
>> requiring full-blown range parse function which I was trying to avoid by
>> using the aforementioned grammar.  So, I decided to move ahead with the
>> following grammar for time being:
>>
>> START (lower-bound) [ EXCLUSIVE ]
>> | END (upper-bound) [ INCLUSIVE ]
>> | START (lower-bound) [ EXCLUSIVE ] END (upper-bound) [ INCLUSIVE ]
>>
>> Where,
>>
>> *-bound: a_expr
>>  | *-bound ',' a_expr
>>
>> Note that in the absence of explicit specification, lower-bound is
>> inclusive and upper-bound is exclusive.
> 
> Thanks for trying. I agree that it would be a full blown range parser, and
> I'm not yet advanced enough to help you with that.
> 
> So presently partitions that are unbounded on the lower end aren't
> possible, but that's a creation syntax issue, not an infrastructure issue.
> Correct?

In case it wasn't apparent, you can create those:

FOR VALUES END (upper-bound) [INCLUSIVE]

which is equivalent to:

FOR VALUES '(, upper-bound)' or FOR VALUES '(, upper-bound]'

>>> Question: I haven't dove into the code, but I was curious about your
>> tuple
>>> routing algorithm. Is there any way for the algorithm to begin it's scan
>> of
>>> candidate partitions based on the destination of the last row inserted
>> this
>>> statement? I ask because most use cases (that I am aware of) have data
>> that
>>> would naturally cluster in the same partition.
>>
>> No.  Actually the tuple-routing function starts afresh for each row.  For
>> range partitions, it's binary search over an array of upper bounds.  There
>> is no row-to-row state caching in the partition module itself.
>>
>>
> bsearch should be fine, that's what I've used in my own custom partitioning
> schemes.
> 
> Was there a new patch, and if so, is it the one you want me to kick the
> tires on?

You didn't miss any.  I need a little more time to send the next revision
with significant design overhaul.

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] GCC 6 warning fixes

[Peter Eisentraut]

On 3/8/16 4:44 PM, Robert Haas wrote:
> On Mon, Feb 29, 2016 at 4:50 PM, Thomas Munro
>  wrote:
>> On Sat, Feb 20, 2016 at 5:14 PM, Peter Eisentraut  wrote:
>>> Here are three patches to fix new warnings in GCC 6.
>>>
>>> 0001 is apparently a typo.
>>
>> Right, looks like it.  Builds and tests OK with this change (though I
>> didn't get any warning from GCC6.0.0 -Wall for this one).
>>
>>> 0002 was just (my?) stupid code to begin with.
>>
>> Right, it makes sense to define QL_HELP in just one translation unit
>> with external linkage.  Builds and works fine.  I got the 'defined but
>> not used' warning from GCC6 and it went away with this patch.
>>
>>> 0003 is more of a workaround.  There could be other ways address this, too.
>>
>> This way seems fine to me (you probably want the function to continue
>> to exist rather than, say, becoming a macro evaluating to false on
>> non-WIN32, if this gets backpatched).  I got this warning from GCC6
>> and it went away with this patch.
> 
> Peter, are you going to commit this?

done



-- 
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: RETURNING primary_key()

[Craig Ringer]

On 9 March 2016 at 05:40, Igal @ Lucee.org  wrote:

> On 3/8/2016 12:12 PM, Robert Haas wrote:
>
>> I agree that some research should be done on how this works in other
>> systems, but I think we have a general problem with the server lacking
>> certain capabilities that make it easy to implement a high-quality
>> JDBC driver.  And I think it would be good to work on figuring out how
>> to fix that.
>>
> I will try to gather more information about the other DBMSs and drivers
> and will post my findings here when I have them.
>
>
Thanks. I know that's not the most fun thing to do in the world, but it's
often needed when implementing something where part of the goal is being
compatible with other vendors, etc.

Currently I suggest using Connection.prepareStatement(..., String[]
generatedKeyColumns) where possible. I realise that's not practical for all
apps, which is why supporting the int flag form better is desirable, and we
just have to figure out what exactly we should be returning...


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

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Wed, Mar 9, 2016 at 1:07 AM, Andrew Dunstan  wrote:
> Michael's patch proposes to replace the use of sed to generate probes.h with
> the perl equivalent everywhere. That has the advantage that we keep to one
> script to generate probes.h, but it does impose a perl dependency.

Good point. It did not occur to me that this would bring a hard
dependency for non-Windows builds. Let's keep both scripts then. The
attached is changed to do so.
-- 
Michael
From d7a100dae8816ff1287ae0eee2829d2b7ce6ef47 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Tue, 8 Mar 2016 22:18:16 +0900
Subject: [PATCH 1/4] Remove dependency to psed in MSVC scripts

psed has been removed from the core distribution of perl in 5.22, causing
the build of Postgres to fail in the case of MSVC on Windows should at
least this version of perl be used. This commit replaces the dependency
with psed by an equivalent perl script, the non-MSVC build use this script
as well instead of the former sed script when dtrace is not enabled in a
build.
---
 src/backend/utils/Gen_dummy_probes.pl  | 247 +
 src/backend/utils/Gen_dummy_probes.sed |  23 ---
 src/backend/utils/Makefile |   4 +-
 src/tools/msvc/Solution.pm |   2 +-
 4 files changed, 250 insertions(+), 26 deletions(-)
 create mode 100644 src/backend/utils/Gen_dummy_probes.pl
 delete mode 100644 src/backend/utils/Gen_dummy_probes.sed

diff --git a/src/backend/utils/Gen_dummy_probes.pl b/src/backend/utils/Gen_dummy_probes.pl
new file mode 100644
index 000..30c6d65
--- /dev/null
+++ b/src/backend/utils/Gen_dummy_probes.pl
@@ -0,0 +1,247 @@
+#! /usr/bin/perl -w
+#-
+#
+# Gen_dummy_probes.pl
+#Perl script that generates probes.h file when dtrace is not available
+#
+# Portions Copyright (c) 2008-2016, PostgreSQL Global Development Group
+#
+#
+# IDENTIFICATION
+#src/backend/utils/Gen_dummy_probes.pl
+#
+#-
+
+$0 =~ s/^.*?(\w+)[\.\w+]*$/$1/;
+
+use strict;
+use Symbol;
+use vars qw{ $isEOF $Hold %wFiles @Q $CondReg
+  $doAutoPrint $doOpenWrite $doPrint };
+$doAutoPrint = 1;
+$doOpenWrite = 1;
+
+# prototypes
+sub openARGV();
+sub getsARGV(;\$);
+sub eofARGV();
+sub printQ();
+
+# Run: the sed loop reading input and applying the script
+#
+sub Run()
+{
+	my ($h, $icnt, $s, $n);
+
+	# hack (not unbreakable :-/) to avoid // matching an empty string
+	my $z = "\000";
+	$z =~ /$z/;
+
+	# Initialize.
+	openARGV();
+	$Hold= '';
+	$CondReg = 0;
+	$doPrint = $doAutoPrint;
+  CYCLE:
+	while (getsARGV())
+	{
+		chomp();
+		$CondReg = 0;# cleared on t
+	  BOS:;
+
+		# /^[ 	]*probe /!d
+		unless (m /^[ \t]*probe /s)
+		{
+			$doPrint = 0;
+			goto EOS;
+		}
+
+		# s/^[ 	]*probe \([^(]*\)\(.*\);/\1\2/
+		{
+			$s = s /^[ \t]*probe ([^(]*)(.*);/${1}${2}/s;
+			$CondReg ||= $s;
+		}
+
+		# s/__/_/g
+		{
+			$s = s /__/_/sg;
+			$CondReg ||= $s;
+		}
+
+		# y/abcdefghijklmnopqrstuvwxyz/ABCDEFGHIJKLMNOPQRSTUVWXYZ/
+		{ y{abcdefghijklmnopqrstuvwxyz}{ABCDEFGHIJKLMNOPQRSTUVWXYZ}; }
+
+		# s/^/#define TRACE_POSTGRESQL_/
+		{
+			$s = s /^/#define TRACE_POSTGRESQL_/s;
+			$CondReg ||= $s;
+		}
+
+		# s/([^,)]\{1,\})/(INT1)/
+		{
+			$s = s /\([^,)]+\)/(INT1)/s;
+			$CondReg ||= $s;
+		}
+
+		# s/([^,)]\{1,\}, [^,)]\{1,\})/(INT1, INT2)/
+		{
+			$s = s /\([^,)]+, [^,)]+\)/(INT1, INT2)/s;
+			$CondReg ||= $s;
+		}
+
+		# s/([^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\})/(INT1, INT2, INT3)/
+		{
+			$s = s /\([^,)]+, [^,)]+, [^,)]+\)/(INT1, INT2, INT3)/s;
+			$CondReg ||= $s;
+		}
+
+# s/([^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\})/(INT1, INT2, INT3, INT4)/
+		{
+			$s =
+s /\([^,)]+, [^,)]+, [^,)]+, [^,)]+\)/(INT1, INT2, INT3, INT4)/s;
+			$CondReg ||= $s;
+		}
+
+# s/([^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\})/(INT1, INT2, INT3, INT4, INT5)/
+		{
+			$s =
+s /\([^,)]+, [^,)]+, [^,)]+, [^,)]+, [^,)]+\)/(INT1, INT2, INT3, INT4, INT5)/s;
+			$CondReg ||= $s;
+		}
+
+# s/([^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\})/(INT1, INT2, INT3, INT4, INT5, INT6)/
+		{
+			$s =
+s /\([^,)]+, [^,)]+, [^,)]+, [^,)]+, [^,)]+, [^,)]+\)/(INT1, INT2, INT3, INT4, INT5, INT6)/s;
+			$CondReg ||= $s;
+		}
+
+# s/([^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\})/(INT1, INT2, INT3, INT4, INT5, INT6, INT7)/
+		{
+			$s =
+s /\([^,)]+, [^,)]+, [^,)]+, [^,)]+, [^,)]+, [^,)]+, [^,)]+\)/(INT1, INT2, INT3, INT4, INT5, INT6, INT7)/s;
+			$CondReg ||= $s;
+		}
+
+# s/([^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\}, [^,)]\{1,\})/(INT1, INT2, INT3, INT4, INT5, INT6, INT7, INT8)/
+		{
+			$s =
+s /\([^,)]+, [^,)]+, [^,)]+, [^,)]+, [^,)]+, [^,)]+, [^,)]+, [^,)]+\)/(INT1, INT2, INT3, INT4, INT5, INT6, INT7, INT8)/s;
+			$CondReg ||= $s;
+		}
+
+		# P
+		{
+			if 

Re: [HACKERS] Performance improvement for joins where outer side is unique

[David Rowley]

On 9 March 2016 at 13:19, Tom Lane  wrote:
> I do think that the verbosity this adds to the EXPLAIN output is not
> desirable at all, at least not in text mode.  Another line for every
> darn join is a pretty high price.

For me it seems like a good idea to give some sort of indication in
EXPLAIN about this, although I'm not wedded to what I have currently
as being the best and neatest way to do this.
I was however under the impression that you thought this was
reasonable [1], so I didn't go seeking out any other way. Did you have
another idea, or are you just proposing to remove it for text EXPLAIN?
Perhaps, if you are then the tests can still exist with a non-text
output.

Also in [1], I disagree with your proposal for being inconsistent with
what's shown with VERBOSE depending on the EXPLAIN output format. If
we were going to do that then I'd rather just switch on verbose for
non-text, but that might make some people unhappy, so I'd rather we
didn't do that either. So how about I remove it from the TEXT output
and just include it in non-text formats when the VERBOSE flag is set?
then change the tests to use... something other than text... YAML
seems most compact.

[1] http://www.postgresql.org/message-id/8907.1440383...@sss.pgh.pa.us

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


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

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

[Kyotaro HORIGUCHI]

At Wed, 9 Mar 2016 01:16:26 +0900, Amit Langote  wrote 
in 
> On Wed, Mar 9, 2016 at 12:24 AM, Robert Haas  wrote:
> > This patch has been worked on by so many people and reviewed by so
> > many people that I can't keep track of who should be credited when it
> > gets committed.  Could someone provide a list of author(s) and
> > reviewer(s)?
> 
> Original authors are Rahila Syed and Vinayak Pokale.
> 
> I have been reviewing this for last few CFs. I sent in last few
> revisions as well.

The owner of this is Vinayak and, ah, I forgot to add myself as a
reviewer. I have also reviewed this for last few CFs. 

So, as looking into CF app, it seems not so inconsistent with the
persons who appears in this thread for thses three CFs.

Authors: Vinayak Pokale, Rahila Syed, Amit Langote
Reviewers: Amit Langote, Kyotaro Horiguchi

Is there anyone who shold be added in this list?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Performance improvement for joins where outer side is unique

[Tom Lane]

David Rowley  writes:
> I also notice that some regression tests, which I think some of which
> Tom updated in the upper planner changes have now changed back again
> due to the slightly reduced costs on hash and nested loop joins where
> the inner side is unique.

?? I don't see anything in this patch that touches the same queries
that changed plans in my previous patch.

I do think that the verbosity this adds to the EXPLAIN output is not
desirable at all, at least not in text mode.  Another line for every
darn join is a pretty high price.

regards, tom lane


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

Re: [HACKERS] Declarative partitioning

[Corey Huinker]

>
> Sorry for replying so late.
>

No worries! We have jobs to do aside from this.


>
> Everything seemed to go dandy until I tried FOR VALUES (blah , blah],
> where psql wouldn't send the command string without accepting the closing
> parenthesis, :(.  So maybe I should try to put the whole thing in '', that
> is, accept the full range_spec in a string, but then we are back to
> requiring full-blown range parse function which I was trying to avoid by
> using the aforementioned grammar.  So, I decided to move ahead with the
> following grammar for time being:
>
> START (lower-bound) [ EXCLUSIVE ]
> | END (upper-bound) [ INCLUSIVE ]
> | START (lower-bound) [ EXCLUSIVE ] END (upper-bound) [ INCLUSIVE ]
>
> Where,
>
> *-bound: a_expr
>  | *-bound ',' a_expr
>
> Note that in the absence of explicit specification, lower-bound is
> inclusive and upper-bound is exclusive.
>

Thanks for trying. I agree that it would be a full blown range parser, and
I'm not yet advanced enough to help you with that.

So presently partitions that are unbounded on the lower end aren't
possible, but that's a creation syntax issue, not an infrastructure issue.
Correct?


> Okay, perhaps I should not presume a certain usage.  However, as you know,
> the usage like yours requires some mechanism of data redistribution (also
> not without some syntax), which I am not targeting with the initial patch.
>

I'm quite fine with limitations in this initial patch, especially if they
don't limit what's possible in the future.


> > Question: I haven't dove into the code, but I was curious about your
> tuple
> > routing algorithm. Is there any way for the algorithm to begin it's scan
> of
> > candidate partitions based on the destination of the last row inserted
> this
> > statement? I ask because most use cases (that I am aware of) have data
> that
> > would naturally cluster in the same partition.
>
> No.  Actually the tuple-routing function starts afresh for each row.  For
> range partitions, it's binary search over an array of upper bounds.  There
> is no row-to-row state caching in the partition module itself.
>
>
bsearch should be fine, that's what I've used in my own custom partitioning
schemes.

Was there a new patch, and if so, is it the one you want me to kick the
tires on?

Re: [HACKERS] pgcrypto: add s2k-count

[Alvaro Herrera]

Jeff Janes wrote:
> pgcrypto supports s2k-mode for key-stretching during symmetric
> encryption, and even defaults to s2k-mode=3, which means configurable
> iterations.  But it doesn't support s2k-count to actually set those
> iterations to be anything other than the default.  If you are
> interested in key-stretching, the default is not going to cut it.

Talking about deep rabbit holes ...

I gave this a look.  I adjusted it here and there for project style and
general cleanliness (something that could be applied to pgcrypto much
more generally) and eventually arrived at trying to figure out how is
the s2k count actually used by the underlying algorithm.  I arrived at
the function calc_s2k_iter_salted which is where it is actually used to
encrypt things.  But that function is completely devoid of comments and
not completely trivial.  At this point I cannot for the life of me
determine whether that function should use the one-byte format specified
by the relevant RFC (4880) or the decoded, human-understandable number
of iterations.

I would love to be able to read gnupg's code to figure out what it is
that they do, but the structure of their code is even more impenetrable
than pgcrypto's.  Perhaps it would be easier to measure the time it
takes to run some s2k operations ...

I CCed Marko here.  Hopefully he can chime in on whether this patch is
correct.

Anyway, assuming that the iteration count was already being used
correctly, then as far as I'm concerned we're ready to go.  The attached
patch is what I would commit.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/contrib/pgcrypto/expected/pgp-encrypt.out b/contrib/pgcrypto/expected/pgp-encrypt.out
index b35de79..8fc558c 100644
--- a/contrib/pgcrypto/expected/pgp-encrypt.out
+++ b/contrib/pgcrypto/expected/pgp-encrypt.out
@@ -103,6 +103,25 @@ select pgp_sym_decrypt(
  Secret.
 (1 row)
 
+-- s2k count change
+select pgp_sym_decrypt(
+	pgp_sym_encrypt('Secret.', 'key', 's2k-count=1024'),
+	'key', 'expect-s2k-count=1024');
+ pgp_sym_decrypt 
+-
+ Secret.
+(1 row)
+
+-- s2k_count rounds up
+select pgp_sym_decrypt(
+	pgp_sym_encrypt('Secret.', 'key', 's2k-count=6500'),
+	'key', 'expect-s2k-count=6500');
+NOTICE:  pgp_decrypt: unexpected s2k_count: expected 6500 got 65011712
+ pgp_sym_decrypt 
+-
+ Secret.
+(1 row)
+
 -- s2k digest change
 select pgp_sym_decrypt(
 	pgp_sym_encrypt('Secret.', 'key', 's2k-digest-algo=md5'),
diff --git a/contrib/pgcrypto/pgp-decrypt.c b/contrib/pgcrypto/pgp-decrypt.c
index 5c69745..37f374a 100644
--- a/contrib/pgcrypto/pgp-decrypt.c
+++ b/contrib/pgcrypto/pgp-decrypt.c
@@ -643,6 +643,7 @@ parse_symenc_sesskey(PGP_Context *ctx, PullFilter *src)
 	if (res < 0)
 		return res;
 	ctx->s2k_mode = ctx->s2k.mode;
+	ctx->s2k_count = s2k_iter_to_count(ctx->s2k.iter);
 	ctx->s2k_digest_algo = ctx->s2k.digest_algo;
 
 	/*
diff --git a/contrib/pgcrypto/pgp-encrypt.c b/contrib/pgcrypto/pgp-encrypt.c
index 2320c75..c9148fd 100644
--- a/contrib/pgcrypto/pgp-encrypt.c
+++ b/contrib/pgcrypto/pgp-encrypt.c
@@ -567,7 +567,7 @@ init_s2k_key(PGP_Context *ctx)
 	if (ctx->s2k_cipher_algo < 0)
 		ctx->s2k_cipher_algo = ctx->cipher_algo;
 
-	res = pgp_s2k_fill(>s2k, ctx->s2k_mode, ctx->s2k_digest_algo);
+	res = pgp_s2k_fill(>s2k, ctx->s2k_mode, ctx->s2k_digest_algo, ctx->s2k_count);
 	if (res < 0)
 		return res;
 
diff --git a/contrib/pgcrypto/pgp-pgsql.c b/contrib/pgcrypto/pgp-pgsql.c
index 1842985..1f65b66 100644
--- a/contrib/pgcrypto/pgp-pgsql.c
+++ b/contrib/pgcrypto/pgp-pgsql.c
@@ -181,6 +181,7 @@ struct debug_expect
 	int			expect;
 	int			cipher_algo;
 	int			s2k_mode;
+	int			s2k_count;
 	int			s2k_cipher_algo;
 	int			s2k_digest_algo;
 	int			compress_algo;
@@ -196,6 +197,7 @@ fill_expect(struct debug_expect * ex, int text_mode)
 	ex->expect = 0;
 	ex->cipher_algo = -1;
 	ex->s2k_mode = -1;
+	ex->s2k_count = -1;
 	ex->s2k_cipher_algo = -1;
 	ex->s2k_digest_algo = -1;
 	ex->compress_algo = -1;
@@ -218,6 +220,7 @@ check_expect(PGP_Context *ctx, struct debug_expect * ex)
 {
 	EX_CHECK(cipher_algo);
 	EX_CHECK(s2k_mode);
+	EX_CHECK(s2k_count);
 	EX_CHECK(s2k_digest_algo);
 	EX_CHECK(use_sess_key);
 	if (ctx->use_sess_key)
@@ -247,6 +250,8 @@ set_arg(PGP_Context *ctx, char *key, char *val,
 		res = pgp_set_sess_key(ctx, atoi(val));
 	else if (strcmp(key, "s2k-mode") == 0)
 		res = pgp_set_s2k_mode(ctx, atoi(val));
+	else if (strcmp(key, "s2k-count") == 0)
+		res = pgp_set_s2k_count(ctx, atoi(val));
 	else if (strcmp(key, "s2k-digest-algo") == 0)
 		res = pgp_set_s2k_digest_algo(ctx, val);
 	else if (strcmp(key, "s2k-cipher-algo") == 0)
@@ -286,6 +291,11 @@ set_arg(PGP_Context *ctx, char *key, char *val,
 		ex->expect = 1;
 		ex->s2k_mode = atoi(val);
 	}
+	else if (ex != NULL && strcmp(key, "expect-s2k-count") == 0)
+	{
+		ex->expect = 1;
+		ex->s2k_count = atoi(val);
+	}
 	else if (ex != NULL && strcmp(key, 

Re: [HACKERS] Performance improvement for joins where outer side is unique

[David Rowley]

On 23 January 2016 at 05:36, Tomas Vondra  wrote:
> Hi,
>
> On 12/17/2015 02:17 PM, David Rowley wrote:
>>
>> On 17 December 2015 at 19:11, Simon Riggs > > wrote:
>>
>> On 17 December 2015 at 00:17, Tomas Vondra
>> >
>> wrote:
>>
>> I'd go with match_first_tuple_only.
>>
>>
>> +1
>>
>> unique_inner is a state that has been detected,
>> match_first_tuple_only is the action we take as a result.
>>
>>
>> Ok great. I've made it so in the attached. This means the comment in the
>> join code where we perform the skip can be a bit less verbose and all
>> the details can go in where we're actually setting the
>> match_first_tuple_only to true.
>
>
> OK. I've looked at the patch again today, and it seems broken bv 45be99f8 as
> the partial paths were not passing the unique_inner to the create_*_path()
> functions. The attached patch should fix that.
>
> Otherwise I think the patch is ready for committer - I think this is a
> valuable optimization and I see nothing wrong with the code.

I've attached an updated patch which updates it to fix the conflicts
with the recent upper planner changes.
I also notice that some regression tests, which I think some of which
Tom updated in the upper planner changes have now changed back again
due to the slightly reduced costs on hash and nested loop joins where
the inner side is unique. I checked the costs of one of these by
disabling hash join and noticed that the final totla cost is the same,
so it's not too surprising that they keep switching plans with these
planner changes going in. I verified that these remain as is when I
comment out the cost changing code in this patch.

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


unique_joins_dbcecda_2016-03-09.patch
Description: Binary data

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

Re: [HACKERS] silent data loss with ext4 / all current versions

[Joshua D. Drake]

On 03/08/2016 02:16 PM, Robert Haas wrote:

On Mon, Mar 7, 2016 at 10:18 PM, Andres Freund  wrote:

Instead of "durable" I think that "persistent" makes more sense.


I find durable a lot more descriptive. persistent could refer to
retrying the rename or something.


Yeah, I like durable, too.


There is also precedent, DURABLE as in aciD

JD



--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


--
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] fun with "Ready for Committer" patches

[Tatsuo Ishii]

> It's hard to miss the fact that there are an absolutely breathtaking
> number of patches in this CommitFest - 80! - that are in the "needs
> review" state.  We really, really, really need more review to happen -

Many of "needs review" state patches already have reviewer(s). Do you
mean we want more reviewers in addition to them for such patches?

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


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

Re: [HACKERS] fun with "Ready for Committer" patches

[Tom Lane]

Robert Haas  writes:
> Unique Joins - This patch has had a lot of review and discussion.  It
> would be best if Tom Lane looked at it.

Yeah, I'll pick it up soon.  I've basically been kicking as much as
I could down the road for the last couple of months, trying to get the
pathification changes done.  Now that that's in, I expect to be
substantially less AWOL from the commitfest.

> enhanced custom error in PLPythonu - No committer yet.  Tom Lane and
> Peter Eisentraut are the usual suspects for PL/python.  Again, I have
> neither the interest nor the knowledge.

I don't mind touching plpython at the C level, but this one requires
somebody who uses Python enough to have an informed opinion on the
tastefulness of the proposed language features.  That's not me.

regards, tom lane


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

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

[Alvaro Herrera]

Robert Haas wrote:

> Let's yank 'em.  This is a minor issue which is distracting us from
> the main point of this patch, and I don't think it's worth getting
> distracted.

+1

> I'm pretty meh about the whole idea of this function, though,
> actually, and I don't see a single clear +1 vote for this
> functionality upthread.  (Apologies if I've missed one.)  In the
> absence of a few of those, I recommend we reject this.

+1

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


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

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

[David G. Johnston]

On Tue, Mar 8, 2016 at 3:57 PM, Corey Huinker 
wrote:

>
>> I'm pretty meh about the whole idea of this function, though,
>> actually, and I don't see a single clear +1 vote for this
>> functionality upthread.  (Apologies if I've missed one.)  In the
>> absence of a few of those, I recommend we reject this.
>>
>
> Just David and Vik so far. The rest were either against(Simon),
> meh(Robert) or +1ed/-1ed the backpatch, leaving their thoughts on the
> function itself unspoken.
>
> Happy to make the changes above if we're moving forward with it.
>

​I'll toss a user-land +1 for this.

David J.

Re: [HACKERS] PATCH: index-only scans with partial indexes

[Tom Lane]

Kyotaro HORIGUCHI  writes:
> Hello, This is a (maybe) committer-ready patch of a Tomas
> Vondra's project.

I think this needs quite a bit of work yet.  A few comments:

* If we're going to pay the price of identifying implied restriction
conditions in check_partial_indexes(), we should at least recoup some
of that investment by not doing it again in create_indexscan_plan().

* create_indexscan_plan() has this comment about what it's doing:
 * We can also discard quals that are implied by a partial index's
 * predicate, but only in a plain SELECT; when scanning a target relation
 * of UPDATE/DELETE/SELECT FOR UPDATE, we must leave such quals in the
 * plan so that they'll be properly rechecked by EvalPlanQual testing.
I believe that that problem applies for this optimization as well,
and thus that you can only remove implied quals in plain SELECT.
At least, if there's some reason why that problem does *not* apply,
there had darn well better be a comment explaining why it's safe.

* Adding indexrinfos to IndexPath seems unnecessary, since that struct
already has the "index" pointer --- you can just get the field out of the
IndexOptInfo when you need it.  If you insist on having the extra field,
this patch is short of the threshold for correctness on adding fields to
paths.  It missed _outIndexPath for instance.

* The additional #include in costsize.c has no apparent reason.

* The changes in cost_index() really ought to come with some change
in the associated comments.

* Personally I'd not change the signature of
match_restriction_clauses_to_index; that's just code churn that
may have to get undone someday.

* The block comment in check_index_only needs more thought than this,
as the phrase "The same is true" is now invalid; the "same" it refers
to *isn't* the same anymore.

* I'm not too thrilled with injecting the initialization of
index->indrinfos into the initial loop in check_partial_indexes().
If it stays there, I'd certainly expect the comment ahead of the
loop to be changed to have something to do with reality.  But can't
we find some more-appropriate place to initialize it?  Like maybe
where the IndexOptInfo is first created?  I would not really expect
check_partial_indexes() to have side-effects on non-partial indexes.

* I think the double loop in check_partial_indexes() is too cute by half.
I'd be inclined to just build the replacement list unconditionally while
we do the predicate_implied_by() tests.  Those are expensive enough that
saving one lappend per implication-test is a useless optimization,
especially if it requires code as contorted and bug-prone as this.

* The comment added to IndexOptInfo is not very adequate, and not spelled
correctly either.  There's a block comment you should be adding a para to
(probably take the text you added for struct IndexPath).  And again,
there is more work to do to add a field to such a struct, eg outfuncs.c.
Usually a good way to find all the places to touch is to grep for some of
the existing field names in the struct.

* I don't much care for the field name "indrinfos"; it's neither very
readable nor descriptive.  Don't have a better suggestion right now
though.

* Not sure if new regression test cases would be appropriate.  The changes
in the existing cases seem a bit unfortunate actually; I'm afraid that
this may be defeating the original intent of those tests.

I'm setting this back to Waiting on Author.

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] Idle In Transaction Session Timeout, revived

[Andres Freund]

On 2016-03-08 16:42:37 -0500, Robert Haas wrote:
> - I really wonder if the decision to ignore sessions that are idle in
> transaction (aborted) should revisited.  Consider this:
> 
> rhaas=# begin;
> BEGIN
> rhaas=# lock table pg_class;
> LOCK TABLE
> rhaas=# savepoint a;
> SAVEPOINT
> rhaas=# select 1/0;
> ERROR:  division by zero

Probably only if the toplevel transaction is also aborted. Might not be
entirely trivial to determine.

> - What's the right order of events in PostgresMain?  Right now the
> patch disables the timeout after checking for interrupts and clearing
> DoingCommandRead, but maybe it would be better to disable the timeout
> first, so that we can't have it happen that start to execute the
> command and then, in medias res, bomb out because of the idle timeout.
> Then again, maybe you had some compelling reason for doing it this
> way, in which case we should document that in the comments.

Hm, we should never bomb out of the middle of anything with this, right?
I mean all the itmeout handler does is set a volatile var and set a
latch, the rest is done in the interrupt handler? Which is not called in
the signal handler.

I'm no targuing for the current order, just that one argument ;). FWIW,
I think Vik wrote this after discussing with me, and IIRC there was not
a lot of reasoning going into the specific location for doing this.

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

[Corey Huinker]

>
> > It would be simple enough to remove the infinity test on the "stop" and
> > leave it on the "start". Or yank both. Just waiting for others to agree
> > which checks should remain.
>
> Let's yank 'em.  This is a minor issue which is distracting us from
> the main point of this patch, and I don't think it's worth getting
> distracted.
>

+1. It leaves this function consistent with the others, and if we want to
add checks later we can do them all at the same time.


>
> + 
> +
> generate_series(start,
> stop, step
> integer)
> +  date
> +  setof date
> +  
> +   Generate a series of values, from start
> to stop
> +   with a step size of step
>
> I think this should be followed by the word "days" and a period.
>
>
No objections. I just followed the pattern of the other generate_series()
docs.


> +   else
> +   /* do when there is no more left */
> +   SRF_RETURN_DONE(funcctx);
>
> I think we should drop the "else" and unindent the next two lines.
> That's the style I have seen elsewhere.  Plus less indentation equals
> more happiness.
>

No objections here either. I just followed the pattern of generate_series()
for int there.


>
> I'm pretty meh about the whole idea of this function, though,
> actually, and I don't see a single clear +1 vote for this
> functionality upthread.  (Apologies if I've missed one.)  In the
> absence of a few of those, I recommend we reject this.
>

Just David and Vik so far. The rest were either against(Simon), meh(Robert)
or +1ed/-1ed the backpatch, leaving their thoughts on the function itself
unspoken.

Happy to make the changes above if we're moving forward with it.

Re: [HACKERS] fun with "Ready for Committer" patches

[Joe Conway]

On 03/08/2016 02:45 PM, Robert Haas wrote:
> OK, so I made a pass through the "Ready for Committer" patches in the
> current CF.  One I committed, several I replied to the thread with
> review comments and set back to "Waiting on Author". Here's where we
> are with the rest:

> plpgsql - possibility to get element or array type for referenced
> variable type - No committer yet.  I don't have enough interest or
> knowledge to want to handle this.

I'll try to move this one forward, although later in the week as I am
traveling all day tomorrow

> pl/pgSQL, get diagnostics and big data - No committer yet.  Seems like
> a reasonable concept.  A borderline bug fix.

possibly this one too...

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] silent data loss with ext4 / all current versions

[Andres Freund]

On 2016-03-08 23:47:48 +0100, Tomas Vondra wrote:
> I've repeated the power-loss testing today. With the patches applied I'm
> not longer able to reproduce the issue (despite trying about 10x), while
> without them I've hit it on the first try. This is on kernel 4.4.2.

Yay, thanks for testing!

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] silent data loss with ext4 / all current versions

[Tomas Vondra]

Hi,

On Mon, 2016-03-07 at 21:55 -0800, Andres Freund wrote:
> On 2016-03-08 12:26:34 +0900, Michael Paquier wrote:
> > On Tue, Mar 8, 2016 at 12:18 PM, Andres Freund  wrote:
> > > On 2016-03-08 12:01:18 +0900, Michael Paquier wrote:
> > >> I have spent a couple of hours looking at that in details, and the
> > >> patch is neat.
> > >
> > > Cool. Doing some more polishing right now. Will be back with an updated
> > > version soonish.
> > >
> > > Did you do some testing?
> > 
> > Not much in details yet, I just ran a check-world with fsync enabled
> > for the recovery tests, plus quick manual tests with a cluster
> > manually set up. I'll do more with your new version now that I know
> > there will be one.
> 
> Here's my updated version.
> 
> Note that I've split the patch into two. One for the infrastructure, and
> one for the callsites.

I've repeated the power-loss testing today. With the patches applied I'm
not longer able to reproduce the issue (despite trying about 10x), while
without them I've hit it on the first try. This is on kernel 4.4.2.

regards

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



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

[HACKERS] fun with "Ready for Committer" patches

[Robert Haas]

OK, so I made a pass through the "Ready for Committer" patches in the
current CF.  One I committed, several I replied to the thread with
review comments and set back to "Waiting on Author". Here's where we
are with the rest:

Silent data loss with ext4 / all current versions - It looks to me
like Andres is handling this.  I set him as the committer.
pg_resetxlog reference page reorganization - Peter Eisentraut wrote
this patch.  I assume he will commit it.  If not, I'm not sure why
anyone else should.
GCC 6 warning fixes - Ditto.
TAP test enhancements - It looks to me like Alvaro is handling this.
I set him as the committer.
Unique Joins - This patch has had a lot of review and discussion.  It
would be best if Tom Lane looked at it.  If not, one of us lesser
mortals will have to have a go.
index-only scans with partial indexes - Kevin has claimed this as committer.
Fix lock contention for HASHHDR.mutex - I guess I need to go revisit
this one, unless somebody else is willing to jump in.  I wouldn't mind
a few more opinions on this patch.
plpgsql - possibility to get element or array type for referenced
variable type - No committer yet.  I don't have enough interest or
knowledge to want to handle this.
pl/pgSQL, get diagnostics and big data - No committer yet.  Seems like
a reasonable concept.  A borderline bug fix.
enhanced custom error in PLPythonu - No committer yet.  Tom Lane and
Peter Eisentraut are the usual suspects for PL/python.  Again, I have
neither the interest nor the knowledge.

It's hard to miss the fact that there are an absolutely breathtaking
number of patches in this CommitFest - 80! - that are in the "needs
review" state.  We really, really, really need more review to happen -
and there's no place for that to come from except other people who
would like their own patches reviewed in turn, other than the limited
bandwidth of committers and of course the absolutely stunning efforts
of Michael Paquier to review everything in sight.  But that's not
going to be enough: we really, really need other people to be
reviewing also.

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


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

[HACKERS] [PATCH v6] GSSAPI encryption support

[Robbie Harwood]

Hello friends,

Here's yet another version of GSSAPI encryption support.  It's also
available for viewing on my github:

https://github.com/frozencemetery/postgres/tree/feature/gssencrypt6

Let me hit the highlights of this time around:

- Fallback code is back!  It's almost unchanged from early versions of
  this patchset.  Corresponding doc changes for this and the next item
  are of course included.

- Minor protocol change.  I did not realize that connection parameters
  were not read until after auth was complete, which means that in this
  version I go back to sending the AUTH_REQ_OK in the clear.  Though I
  found this initially irritating since it required re-working the
  should_crypto conditions, it ends up being a net positive since I can
  trade a library call for a couple variables.

- Client buffer flush on completion of authentication.  This should
  prevent the issue with the client getting unexpected message type of
  NUL due to encrypted data not getting decrypted.  I continue to be
  unable to replicate this issue, but since the codepath triggers in the
  "no data buffered case" all the math is sound.  (Famous last words I'm
  sure.)

- Code motion is its own patch.  This was requested and hopefully
  clarifies what's going on.

- Some GSSAPI authentication fixes have been applied.  I've been staring
  at this code too long now and writing this made me feel better.  If it
  should be a separate change that's fine and easy to do.

Thanks!
From 5674aa74effab4931bac1044f32dee83d915aa90 Mon Sep 17 00:00:00 2001
From: Robbie Harwood 
Date: Fri, 26 Feb 2016 16:07:05 -0500
Subject: [PATCH 1/3] Move common GSSAPI code into its own files

On both the frontend and backend, prepare for GSSAPI encryption suport
by moving common code for error handling into a common file.  Other than
build-system changes, no code changes occur in this patch.
---
 configure   |  2 +
 configure.in|  1 +
 src/Makefile.global.in  |  1 +
 src/backend/libpq/Makefile  |  4 ++
 src/backend/libpq/auth.c| 63 +--
 src/backend/libpq/be-gssapi-common.c| 75 +
 src/include/libpq/be-gssapi-common.h| 26 
 src/interfaces/libpq/Makefile   |  4 ++
 src/interfaces/libpq/fe-auth.c  | 48 +
 src/interfaces/libpq/fe-gssapi-common.c | 63 +++
 src/interfaces/libpq/fe-gssapi-common.h | 21 +
 11 files changed, 199 insertions(+), 109 deletions(-)
 create mode 100644 src/backend/libpq/be-gssapi-common.c
 create mode 100644 src/include/libpq/be-gssapi-common.h
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.c
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.h

diff --git a/configure b/configure
index b3f3abe..a5bd629 100755
--- a/configure
+++ b/configure
@@ -713,6 +713,7 @@ with_systemd
 with_selinux
 with_openssl
 krb_srvtab
+with_gssapi
 with_python
 with_perl
 with_tcl
@@ -5491,6 +5492,7 @@ $as_echo "$with_gssapi" >&6; }
 
 
 
+
 #
 # Kerberos configuration parameters
 #
diff --git a/configure.in b/configure.in
index 0bd90d7..4fd8f05 100644
--- a/configure.in
+++ b/configure.in
@@ -636,6 +636,7 @@ PGAC_ARG_BOOL(with, gssapi, no, [build with GSSAPI support],
   krb_srvtab="FILE:\$(sysconfdir)/krb5.keytab"
 ])
 AC_MSG_RESULT([$with_gssapi])
+AC_SUBST(with_gssapi)
 
 
 AC_SUBST(krb_srvtab)
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index e94d6a5..3dbc5c2 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -183,6 +183,7 @@ with_perl	= @with_perl@
 with_python	= @with_python@
 with_tcl	= @with_tcl@
 with_openssl	= @with_openssl@
+with_gssapi	= @with_gssapi@
 with_selinux	= @with_selinux@
 with_systemd	= @with_systemd@
 with_libxml	= @with_libxml@
diff --git a/src/backend/libpq/Makefile b/src/backend/libpq/Makefile
index 09410c4..a8cc9a0 100644
--- a/src/backend/libpq/Makefile
+++ b/src/backend/libpq/Makefile
@@ -21,4 +21,8 @@ ifeq ($(with_openssl),yes)
 OBJS += be-secure-openssl.o
 endif
 
+ifeq ($(with_gssapi),yes)
+OBJS += be-gssapi-common.o
+endif
+
 include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 57c2f48..73d493e 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -136,11 +136,7 @@ bool		pg_krb_caseins_users;
  *
  */
 #ifdef ENABLE_GSS
-#if defined(HAVE_GSSAPI_H)
-#include 
-#else
-#include 
-#endif
+#include "libpq/be-gssapi-common.h"
 
 static int	pg_GSS_recvauth(Port *port);
 #endif   /* ENABLE_GSS */
@@ -715,63 +711,6 @@ recv_and_check_password_packet(Port *port, char **logdetail)
  */
 #ifdef ENABLE_GSS
 
-#if defined(WIN32) && !defined(WIN32_ONLY_COMPILER)
-/*
- * MIT Kerberos GSSAPI DLL doesn't properly export the symbols for MingW
- * that contain the OIDs required. Redefine 

Re: [HACKERS] SP-GiST support for inet datatypes

[Oleg Bartunov]

On Tue, Mar 8, 2016 at 11:17 PM, Oleg Bartunov  wrote:

>
>
> On Thu, Mar 3, 2016 at 11:45 AM, Emre Hasegeli  wrote:
>
>> > Emre, I checked original thread and didn't find sample data. Could you
>> provide them for testing ?
>>
>> I found it on the Git history:
>>
>>
>> https://github.com/job/irrexplorer/blob/9e8b5330d7ef0022abbe1af18291257e044eb24b/data/irrexplorer_dump.sql.gz?raw=true
>>
>
> Thanks !
>
> spgist index creates 2 times faster than gist, but index size is
> noticeably  bugger
>
> \di+ route_*
> List of relations
>  Schema | Name | Type  |  Owner   | Table  |  Size  | Description
> +--+---+--+++-
>  public | route_gist   | index | postgres | routes | 96 MB  |
>  public | route_spgist | index | postgres | routes | 132 MB |
> (2 rows)
>
> Spgist index tree is much better  than gist - 12149 pages vs 1334760 !
>

I also noticed, that spgist is much faster than gist for other inet
operators. I'd like to see in 9.6.



>
>
>
> EXPLAIN (ANALYZE, buffers) SELECT routes.route FROM routes JOIN hmm ON
> routes.route && hmm.route;
>QUERY PLAN
>
> 
>  Nested Loop  (cost=0.41..570430.27 rows=2338 width=7) (actual
> time=5.730..12085.747 rows=8127 loops=1)
>Buffers: shared hit=1334760
>->  Seq Scan on hmm  (cost=0.00..11.32 rows=732 width=7) (actual
> time=0.013..0.528 rows=732 loops=1)
>  Buffers: shared hit=4
>->  Index Only Scan using route_gist on routes  (cost=0.41..550.26
> rows=22900 width=7) (actual time=2.491..16.503 rows=11 loops=732)
>  Index Cond: (route && (hmm.route)::inet)
>  Heap Fetches: 8127
>  Buffers: shared hit=1334756
>  Planning time: 0.827 ms
>  Execution time: 12086.513 ms
> (10 rows)
>
> EXPLAIN (ANALYZE, buffers) SELECT routes.route FROM routes JOIN hmm ON
> routes.route && hmm.route;
>QUERY PLAN
>
> -
>  Nested Loop  (cost=0.41..588634.27 rows=2338 width=7) (actual
> time=0.043..12.150 rows=8127 loops=1)
>Buffers: shared hit=12149
>->  Seq Scan on hmm  (cost=0.00..11.32 rows=732 width=7) (actual
> time=0.013..0.075 rows=732 loops=1)
>  Buffers: shared hit=4
>->  Index Only Scan using route_spgist on routes  (cost=0.41..575.13
> rows=22900 width=7) (actual time=0.011..0.015 rows=11 loops=732)
>  Index Cond: (route && (hmm.route)::inet)
>  Heap Fetches: 8127
>  Buffers: shared hit=12145
>  Planning time: 0.779 ms
>  Execution time: 12.603 ms
> (10 rows)
>
>
>
>
>

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

[Robert Haas]

On Fri, Mar 4, 2016 at 3:17 PM, Corey Huinker  wrote:
>>
>> I feel rather uneasy about simply removing the 'infinity' checks. Is there
>> a way to differentiate those two cases, i.e. when the generate_series is
>> called in target list and in the FROM part? If yes, we could do the check
>> only in the FROM part, which is the case that does not work (and consumes
>> arbitrary amounts of memory).
>
> It would be simple enough to remove the infinity test on the "stop" and
> leave it on the "start". Or yank both. Just waiting for others to agree
> which checks should remain.

Let's yank 'em.  This is a minor issue which is distracting us from
the main point of this patch, and I don't think it's worth getting
distracted.

+ 
+  generate_series(start,
stop, step
integer)
+  date
+  setof date
+  
+   Generate a series of values, from start
to stop
+   with a step size of step

I think this should be followed by the word "days" and a period.

+   else
+   /* do when there is no more left */
+   SRF_RETURN_DONE(funcctx);

I think we should drop the "else" and unindent the next two lines.
That's the style I have seen elsewhere.  Plus less indentation equals
more happiness.

I'm pretty meh about the whole idea of this function, though,
actually, and I don't see a single clear +1 vote for this
functionality upthread.  (Apologies if I've missed one.)  In the
absence of a few of those, I recommend we reject this.

-- 
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] SP-GiST support for inet datatypes

[Oleg Bartunov]

On Thu, Mar 3, 2016 at 11:45 AM, Emre Hasegeli  wrote:

> > Emre, I checked original thread and didn't find sample data. Could you
> provide them for testing ?
>
> I found it on the Git history:
>
>
> https://github.com/job/irrexplorer/blob/9e8b5330d7ef0022abbe1af18291257e044eb24b/data/irrexplorer_dump.sql.gz?raw=true
>

Thanks !

spgist index creates 2 times faster than gist, but index size is
noticeably  bugger

\di+ route_*
List of relations
 Schema | Name | Type  |  Owner   | Table  |  Size  | Description
+--+---+--+++-
 public | route_gist   | index | postgres | routes | 96 MB  |
 public | route_spgist | index | postgres | routes | 132 MB |
(2 rows)

Spgist index tree is much better  than gist - 12149 pages vs 1334760 !



EXPLAIN (ANALYZE, buffers) SELECT routes.route FROM routes JOIN hmm ON
routes.route && hmm.route;
   QUERY PLAN

 Nested Loop  (cost=0.41..570430.27 rows=2338 width=7) (actual
time=5.730..12085.747 rows=8127 loops=1)
   Buffers: shared hit=1334760
   ->  Seq Scan on hmm  (cost=0.00..11.32 rows=732 width=7) (actual
time=0.013..0.528 rows=732 loops=1)
 Buffers: shared hit=4
   ->  Index Only Scan using route_gist on routes  (cost=0.41..550.26
rows=22900 width=7) (actual time=2.491..16.503 rows=11 loops=732)
 Index Cond: (route && (hmm.route)::inet)
 Heap Fetches: 8127
 Buffers: shared hit=1334756
 Planning time: 0.827 ms
 Execution time: 12086.513 ms
(10 rows)

EXPLAIN (ANALYZE, buffers) SELECT routes.route FROM routes JOIN hmm ON
routes.route && hmm.route;
   QUERY PLAN
-
 Nested Loop  (cost=0.41..588634.27 rows=2338 width=7) (actual
time=0.043..12.150 rows=8127 loops=1)
   Buffers: shared hit=12149
   ->  Seq Scan on hmm  (cost=0.00..11.32 rows=732 width=7) (actual
time=0.013..0.075 rows=732 loops=1)
 Buffers: shared hit=4
   ->  Index Only Scan using route_spgist on routes  (cost=0.41..575.13
rows=22900 width=7) (actual time=0.011..0.015 rows=11 loops=732)
 Index Cond: (route && (hmm.route)::inet)
 Heap Fetches: 8127
 Buffers: shared hit=12145
 Planning time: 0.779 ms
 Execution time: 12.603 ms
(10 rows)

Re: [HACKERS] silent data loss with ext4 / all current versions

[Robert Haas]

On Mon, Mar 7, 2016 at 10:18 PM, Andres Freund  wrote:
>> Instead of "durable" I think that "persistent" makes more sense.
>
> I find durable a lot more descriptive. persistent could refer to
> retrying the rename or something.

Yeah, I like durable, too.

-- 
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] extend pgbench expressions with functions

[Robert Haas]

On Tue, Mar 8, 2016 at 3:52 PM, Fabien COELHO  wrote:
> [ new patch and assorted color commentary ]

This is not acceptable:

+   /* guess double type (n for "inf",
"-inf" and "nan") */
+   if (strchr(var, '.') != NULL ||
strchr(var, 'n') != NULL)
+   {
+   double dv;
+   sscanf(var, "%lf", );
+   setDoubleValue(retval, dv);
+   }
+   else
+   setIntValue(retval, strtoint64(var));

That totally breaks the error handling which someone carefully established here.

+   PgBenchValue *varg = & vargs[0];

Extra space.

+   if (!coerceToDouble(st, & vargs[0], ))
+   return false;

Another extra space.

-   int nargs = 0;
-   int64   iargs[MAX_FARGS];
-   PgBenchExprLink *l = args;
+   int nargs = 0;
+   PgBenchValuevargs[MAX_FARGS];
+   PgBenchExprLink*l = args;

Completely unnecessary reindentation of the first and third lines.

+   setIntValue(retval, i < 0? -i: i);

Not project style.

Please fix the whitespace damage git diff --check complains about, and
check for other places where you haven't followed project style.

-- 
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 small bug fix

[Robert Haas]

On Wed, Jan 27, 2016 at 2:31 PM, Fabien COELHO  wrote:
>  - when a duration (-T) is specified, ensure that pgbench ends at that
>time (i.e. do not wait for a transaction beyond the end of the run).

Every other place where doCustom() returns false is implemented as
return clientDone(...).  I think this should probably do the same.

I also think that we should probably store the end time someplace
instead of recomputing it in multiple places (this patch adds two such
places).

-- 
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] empty array case in plperl_ref_from_pg_array not handled correctly

[Andres Freund]

On 2016-03-08 02:11:03 -0700, Alex Hunsaker wrote:
> On Mon, Mar 7, 2016 at 11:32 PM, Andres Freund  wrote:
> 
> > Hi,
> >
> > Per the new valgrind animal we get:
> >
> >
> > http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=skink=2016-03-08%2004%3A22%3A00
> > 2016-03-08 05:56:05.566 UTC [56de6971.723:5] LOG:  statement: select
> > plperl_sum_array('{}');
> > ==1827== Invalid write of size 4
> > ==1827==at 0x14E35DD1: plperl_ref_from_pg_array (plperl.c:1459)
> >
> >

> [ I think you may have meant to CC me not Alexey K. I'm probably the person
> responsible :D. ]

I just took the first person mentioned in the commit message
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=87bb2ade2ce646083f39d5ab3e3307490211ad04
sorry for leaving you out ;)

> Recursive calls to split_array() should be fine as nested 0 dim arrays get
> collapsed down to single 0 dim array (I hand tested it nonetheless):
>  # select ARRAY[ARRAY[ARRAY[]]]::int[];
>  array
> ───
>  {}
> (1 row)
> 
> Looking around, everything that makes an array seems to pass through
> plperl_ref_from_pg_array(), so I think once that is fixed all is good. I
> also looked for dimensions and ARR_NDIM() just to make sure (didn't find
> anything fishy).

Thanks for looking.

backpatched all the way.

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] GCC 6 warning fixes

[Robert Haas]

On Mon, Feb 29, 2016 at 4:50 PM, Thomas Munro
 wrote:
> On Sat, Feb 20, 2016 at 5:14 PM, Peter Eisentraut  wrote:
>> Here are three patches to fix new warnings in GCC 6.
>>
>> 0001 is apparently a typo.
>
> Right, looks like it.  Builds and tests OK with this change (though I
> didn't get any warning from GCC6.0.0 -Wall for this one).
>
>> 0002 was just (my?) stupid code to begin with.
>
> Right, it makes sense to define QL_HELP in just one translation unit
> with external linkage.  Builds and works fine.  I got the 'defined but
> not used' warning from GCC6 and it went away with this patch.
>
>> 0003 is more of a workaround.  There could be other ways address this, too.
>
> This way seems fine to me (you probably want the function to continue
> to exist rather than, say, becoming a macro evaluating to false on
> non-WIN32, if this gets backpatched).  I got this warning from GCC6
> and it went away with this patch.

Peter, are you going to commit this?

-- 
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] Idle In Transaction Session Timeout, revived

[Robert Haas]

On Sun, Jan 31, 2016 at 8:33 AM, Vik Fearing  wrote:
> Attached is a rebased and revised version of my
> idle_in_transaction_session_timeout patch from last year.
>
> This version does not suffer the problems the old one did where it would
> jump out of SSL code thanks to Andres' patch in commit
> 4f85fde8eb860f263384fffdca660e16e77c7f76.
>
> The basic idea is if a session remains idle in a transaction for longer
> than the configured time, that connection will be dropped thus releasing
> the connection slot and any locks that may have been held by the broken
> client.
>
> Added to the March commitfest.

I see this patch has been marked Ready for Committer despite the lack
of any really substantive review.  Generally, I think it looks good.
But I have a couple of questions/comments:

- I really wonder if the decision to ignore sessions that are idle in
transaction (aborted) should revisited.  Consider this:

rhaas=# begin;
BEGIN
rhaas=# lock table pg_class;
LOCK TABLE
rhaas=# savepoint a;
SAVEPOINT
rhaas=# select 1/0;
ERROR:  division by zero

- I wonder if the documentation should mention potential advantages
related to holding back xmin.

- What's the right order of events in PostgresMain?  Right now the
patch disables the timeout after checking for interrupts and clearing
DoingCommandRead, but maybe it would be better to disable the timeout
first, so that we can't have it happen that start to execute the
command and then, in medias res, bomb out because of the idle timeout.
Then again, maybe you had some compelling reason for doing it this
way, in which case we should document that in the comments.

- It would be nice if you reviewed someone else's patch in turn.

I'm attaching a lightly-edited version of your patch.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index a09ceb2..cdd5d77 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -5976,6 +5976,30 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
   
  
 
+ 
+  idle_in_transaction_session_timeout (integer)
+  
+   idle_in_transaction_session_timeout configuration parameter
+  
+  
+  
+   
+   Terminate any session with an open transaction that has been idle for
+   longer than the specified duration in milliseconds. This allows any
+   locks to be released and the connection slot to be reused.
+   
+   
+   Sessions in the state "idle in transaction (aborted)" occupy a
+   connection slot, but because they hold no locks, they are not considered
+   by this parameter.
+   
+   
+   The default value of 0 means that sessions which are idle in transaction
+   will will not be terminated.
+   
+  
+ 
+
  
   vacuum_freeze_table_age (integer)
   
diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index dd3c775..6352e12 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -725,7 +725,9 @@ ERROR:  could not serialize access due to read/write dependencies among transact
  
   
Don't leave connections dangling idle in transaction
-   longer than necessary.
+   longer than necessary.  The configuration parameter
+may be used to
+   automatically disconnect lingering sessions.
   
  
  
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 6453b88..109d090 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -57,6 +57,7 @@
 int			DeadlockTimeout = 1000;
 int			StatementTimeout = 0;
 int			LockTimeout = 0;
+int			IdleInTransactionSessionTimeout = 0;
 bool		log_lock_waits = false;
 
 /* Pointer to this process's PGPROC and PGXACT structs, if any */
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 115166b..8a75dd2 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2978,6 +2978,11 @@ ProcessInterrupts(void)
 		}
 	}
 
+	if (IdleInTransactionTimeoutSessionPending)
+		ereport(FATAL,
+(errcode(ERRCODE_IDLE_IN_TRANSACTION_SESSION_TIMEOUT),
+ errmsg("terminating connection due to idle-in-transaction timeout")));
+
 	if (ParallelMessagePending)
 		HandleParallelMessages();
 }
@@ -3947,6 +3952,11 @@ PostgresMain(int argc, char *argv[],
 			{
 set_ps_display("idle in transaction", false);
 pgstat_report_activity(STATE_IDLEINTRANSACTION, NULL);
+
+/* Start the idle-in-transaction timer */
+if (IdleInTransactionSessionTimeout > 0)
+	enable_timeout_after(IDLE_IN_TRANSACTION_SESSION_TIMEOUT,
+		 IdleInTransactionSessionTimeout);
 			}
 			else
 			{
@@ -3987,7 +3997,13 @@ PostgresMain(int argc, char *argv[],
 		DoingCommandRead = false;
 
 		/*
-		 * (5) check for any other interesting events that happened while we
+		 * (5) turn off the idle-in-transaction timeout
+		 */
+		

Re: [HACKERS] Proposal: RETURNING primary_key()

[Igal @ Lucee.org]

On 3/8/2016 12:12 PM, Robert Haas wrote:

I agree that some research should be done on how this works in other
systems, but I think we have a general problem with the server lacking
certain capabilities that make it easy to implement a high-quality
JDBC driver.  And I think it would be good to work on figuring out how
to fix that.
I will try to gather more information about the other DBMSs and drivers 
and will post my findings here when I have them.


Best,


Igal


--
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] More stable query plans via more predictable column statistics

[Tom Lane]

Robert Haas  writes:
> On Wed, Jan 20, 2016 at 5:09 PM, Tom Lane  wrote:
>> Um, I would like to review it, but I doubt I'll find time before the end
>> of the month.

> Tom, can you pick this up?

Yes, now that I've gotten out from under the pathification thing,
I have cycles for patch review.  I'll take this one.

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

[David Rowley]

On 9 March 2016 at 04:06, Robert Haas  wrote:
> On Mon, Mar 7, 2016 at 5:15 PM, David Rowley
>  wrote:
>> My concerns are:
>> 1. Since there's no cheapest_partial_path in RelOptInfo the code is
>> currently considering every partial_path for parallel hash aggregate.
>> With normal aggregation we only ever use the cheapest path, so this
>> may not be future proof. As of today we do only have at most one
>> partial path in the list, but there's no reason to code this with that
>> assumption. I didn't put in much effort to improve this as I see code
>> in generate_gather_paths() which also makes assumptions about there
>> just being 1 partial path. Perhaps we should expand RelOptInfo to
>> track the cheapest partial path? or maybe allpaths.c should have a
>> function to fetch the cheapest out of the list?
>
> The first one in the list will be the cheapest; why not just look at
> that?  Sorted partial paths are interesting if some subsequent path
> construction step can make use of that sort ordering, but they're
> never interesting from the point of view of matching the query's
> pathkeys because of the fact that Gather is order-destroying.

In this case a sorted partial path is useful as the partial agg node
sits below Gather. The sorted input is very interesting for the
partial agg node with a strategy of AGG_SORTED. In most cases with
parallel aggregate it's the partial stage that will take the most
time, so if we do get pre-sorted partial paths, this will be very good
indeed for parallel agg.

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


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

Re: [HACKERS] Allowing to run a buildfarm animal under valgrind

[Andres Freund]

On 2016-03-08 18:24:23 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> 
> > I do wonder if adding a PGCTLPOSTGRESWRAPPER or something to pg_ctl
> > would be ok. That'd just supplant calling the postgres binary, making
> > all this easier.
> 
> This seems a reasonably principled way to go about this.  Eventually we
> might plug other things in it ...

It's not that easy to write such a wrapper though - you *have* to exec
the final binary, because -w assumes that the child pid is going to
appear in postmaster.pid...

To be actually useful we kinda would have to backpatch this to 9.4
(where the valgrind hardening stuff started)...

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] Allowing to run a buildfarm animal under valgrind

[Alvaro Herrera]

Andres Freund wrote:

> I do wonder if adding a PGCTLPOSTGRESWRAPPER or something to pg_ctl
> would be ok. That'd just supplant calling the postgres binary, making
> all this easier.

This seems a reasonably principled way to go about this.  Eventually we
might plug other things in it ...

-- 
Á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] empty array case in plperl_ref_from_pg_array not handled correctly

[Oleksii Kliukin]

> On 08 Mar 2016, at 10:11, Alex Hunsaker  wrote:
> 
> 
> 
> On Mon, Mar 7, 2016 at 11:32 PM, Andres Freund  > wrote:
> Hi,
> 
> Per the new valgrind animal we get:
> 
> http://pgbuildfarm.org/cgi-bin/show_log.pl?nm=skink=2016-03-08%2004%3A22%3A00
> 2016-03-08 
> 
>  05:56:05.566 UTC [56de6971.723:5] LOG:  statement: select 
> plperl_sum_array('{}');
> ==1827== Invalid write of size 4
> ==1827==at 0x14E35DD1: plperl_ref_from_pg_array (plperl.c:1459)
> 
> 
> [ I think you may have meant to CC me not Alexey K. I'm probably the person 
> responsible :D. ]
> 
> Indeed, I think the simplest fix is to make plperl_ref_from_pg_array() return 
> an "empty" array in that case. The attached fixes the valgrind warning for 
> me. (cassert enabled build on master).

Looks good to me, thank you. Judging from the size in the error message it’s 
likely the

info->nelems[0] = items

line that caused this issue. The patch fixes it at first glance, although I 
have yet to make my valgrind setup on OS X working to check this for real :-)

Kind regards,
--
Oleksii

Re: [HACKERS] extend pgbench expressions with functions

[Fabien COELHO]

Hello Robert.

Here is a v34 b & c.


// comments are not allowed.  I'd just remove the two you have.


Back to the eighties!


It make no sense to exit(1) and then return 0, so don't do that.  I
might write this code as:
This would get rid of the internal-error case here altogether in favor
of testing it via an assertion.


I've put assertions instead of exit in some places.


I think that coerceToInt() should not exit(1) when an overflow occurs;


I think that it should, because the only sane option for the user is to 
fix the script and relaunch the bench: counting errors has no added value 
for the user.


The attached version does some error handling instead, too bad.


Now, if rval is out of range of an integer, that is going to overflow
while trying to see whether it should divide by zero.


I could not find a place where there where such potential issue. If the 
value is zero, it cannot overflow when cast to int. If it is not zero but 
it overflows, then it is an overflow, so it should overflow. Maybe I 
misunderstood your point.


--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index cc80b3f..2133bf7 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -794,9 +794,10 @@ pgbench  options  dbname
 
 
  
-  Sets variable varname to an integer value calculated
+  Sets variable varname to a value calculated
   from expression.
   The expression may contain integer constants such as 5432,
+  double constants such as 3.14159,
   references to variables :variablename,
   and expressions composed of unary (-) or binary operators
   (+, -, *, /,
@@ -809,7 +810,7 @@ pgbench  options  dbname
   Examples:
 
 \set ntellers 10 * :scale
-\set aid (1021 * :aid) % (10 * :scale) + 1
+\set aid (1021 * random(1, 10 * :scale)) % (10 * :scale) + 1
 
 

@@ -829,66 +830,35 @@ pgbench  options  dbname
  
 
  
-  By default, or when uniform is specified, all values in the
-  range are drawn with equal probability.  Specifying gaussian
-  or  exponential options modifies this behavior; each
-  requires a mandatory parameter which determines the precise shape of the
-  distribution.
- 
+  
+   
+
+ \setrandom n 1 10 or \setrandom n 1 10 uniform
+ is equivalent to \set n random(1, 10) and uses a uniform
+ distribution.
+
+   
 
- 
-  For a Gaussian distribution, the interval is mapped onto a standard
-  normal distribution (the classical bell-shaped Gaussian curve) truncated
-  at -parameter on the left and +parameter
-  on the right.
-  Values in the middle of the interval are more likely to be drawn.
-  To be precise, if PHI(x) is the cumulative distribution
-  function of the standard normal distribution, with mean mu
-  defined as (max + min) / 2.0, with
-
- f(x) = PHI(2.0 * parameter * (x - mu) / (max - min + 1)) /
-(2.0 * PHI(parameter) - 1.0)
-
-  then value i between min and
-  max inclusive is drawn with probability:
-  f(i + 0.5) - f(i - 0.5).
-  Intuitively, the larger parameter, the more
-  frequently values close to the middle of the interval are drawn, and the
-  less frequently values close to the min and
-  max bounds. About 67% of values are drawn from the
-  middle 1.0 / parameter, that is a relative
-  0.5 / parameter around the mean, and 95% in the middle
-  2.0 / parameter, that is a relative
-  1.0 / parameter around the mean; for instance, if
-  parameter is 4.0, 67% of values are drawn from the
-  middle quarter (1.0 / 4.0) of the interval (i.e. from
-  3.0 / 8.0 to 5.0 / 8.0) and 95% from
-  the middle half (2.0 / 4.0) of the interval (second and
-  third quartiles). The minimum parameter is 2.0 for
-  performance of the Box-Muller transform.
- 
+  
+   
+\setrandom n 1 10 exponential 3.0 is equivalent to
+\set n random_exponential(1, 10, 3.0) and uses an
+exponential distribution.
+   
+  
 
- 
-  For an exponential distribution, parameter
-  controls the distribution by truncating a quickly-decreasing
-  exponential distribution at parameter, and then
-  projecting onto integers between the bounds.
-  To be precise, with
-
-f(x) = exp(-parameter * (x - min) / (max - min + 1)) / (1.0 - exp(-parameter))
-
-  Then value i between min and
-  max inclusive is drawn with probability:
-  f(x) - f(x + 1).
-  Intuitively, the larger parameter, the more
-  frequently values close to min are accessed, and the
-  less frequently values close to max are accessed.
-  The closer to 0 parameter, the flatter (more uniform)
-  the access distribution.
-  A crude approximation of the distribution is that the most frequent 1%
-  values in the range, close to min, are drawn
-  parameter% 

Re: [HACKERS] Proposal: Generic WAL logical messages

[Artur Zakirov]

I think here


+const char *
+logicalmsg_identify(uint8 info)
+{
+   if (info & ~XLR_INFO_MASK == XLOG_LOGICAL_MESSAGE)
+   return "MESSAGE";
+
+   return NULL;
+}


we should use brackets

const char *
logicalmsg_identify(uint8 info)
{
if ((info & ~XLR_INFO_MASK) == XLOG_LOGICAL_MESSAGE)
return "MESSAGE";

return NULL;
}

Because of operator priorities 
http://en.cppreference.com/w/c/language/operator_precedence we may get 
errors.


On 01.03.2016 00:10, Petr Jelinek wrote:

Hi,

attached is the newest version of the patch.

I removed the registry, renamed the 'send' to 'emit', documented the
callback parameters properly. I also added the test to ddl.sql for the
serialization and deserialization (and of course found a bug there) and
in general fixed all the stuff Andres reported.

(see more inline)


--
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] Proposal: RETURNING primary_key()

[Robert Haas]

On Mon, Mar 7, 2016 at 11:18 PM, Tom Lane  wrote:
> Craig Ringer  writes:
>> On 8 March 2016 at 08:56, Igal @ Lucee.org  wrote:
>>> I'm not sure why it was not accepted at the end?
>
>> The biggest issue, though it might not be clear from that thread, is that
>> what exactly it means to "return generated keys" is poorly defined by JDBC,
>> and not necessarily the same thing as "return the PRIMARY KEY".
>>
>> Should we return the DEFAULT on a UNIQUE column, for example?
>>
>> IMO other vendors' drivers should be tested for behaviour in a variety of
>> cases.
>
> Yeah.  It was asserted in the earlier thread that other vendors implement
> this feature as "return the pkey", but that seems to conflict with the
> plain language of the JDBC spec: generated columns are an entirely
> different thing than primary key columns.  So really what I'd like to see
> is some work on surveying other implementations to confirm exactly what
> behavior they implement.  If we're to go against what the spec seems to
> say, I want to see a whole lot of evidence that other people do it
> consistently in a different way.

I agree that some research should be done on how this works in other
systems, but I think we have a general problem with the server lacking
certain capabilities that make it easy to implement a high-quality
JDBC driver.  And I think it would be good to work on figuring out how
to fix that.  I feel that some of the replies on this thread were
rather hostile considering that the goal -- good connectors for the
database server -- is extremely important.

-- 
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] More stable query plans via more predictable column statistics

[Alvaro Herrera]

Shulgin, Oleksandr wrote:

> Alright.  I'm attaching the latest version of this patch split in two
> parts: the first one is NULLs-related bugfix and the second is the
> "improvement" part, which applies on top of the first one.

I went over patch 0001 and it seems pretty reasonable.  It's missing
some comment updates -- at least the large comments that talk about Duj1
should be modified to indicate why the code is now subtracting the null
count.  Also, I can't quite figure out why the "else" now in line 2131
is now "else if track_cnt != 0".  What happens if track_cnt is zero?
The comment above the "if" block doesn't provide any guidance.

-- 
Á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] More stable query plans via more predictable column statistics

[Joel Jacobson]

On Wed, Mar 9, 2016 at 1:25 AM, Shulgin, Oleksandr
 wrote:
> Thank you for spending your time to run these :-)

n/p, it took like 30 seconds :-)

> I don't want to be asking for too much here, but is there a chance you could
> try the effects of the proposed patch on an offline copy of your database?

Yes, I think that should be possible.

> Do you envision or maybe have experienced problems with query plans
> referring to the columns that are near the top of the above hist_ratio
> report?  In other words: what are the practical implications for you with
> the values being duplicated rather badly throughout the histogram like in
> the example you shown?

I don't know much about the internals of query planner,
I just read the "57.1. Row Estimation Examples" to get a basic understanding.

If I understand it correctly, if the histogram_bounds contains a lot
of duplicated values,
then the row estimation will be inaccurate, which in turn will trick
the query planner
into a sub-optimal plan?

We've had some problems lately with the query planner, or actually we've always
had them but never noticed them nor cared about them, but now during peak times
we've had short periods where we haven't been able to fully cope up
with the traffic.

I tracked down the most self_time-consuming functions and quickly saw
how to optimize them.
Many of them where on the form:
SELECT .. FROM SomeBigTable WHERE Col1 = [some dynamic value] AND Col2
= [some constant value] AND Col3 = [some other constant value]
The number of rows matching the WHERE clause were very tiny, perfect
match for a partial index:
CREATE INDEX .. ON SomeBigTable USING btree (Col1) WHERE Col2 = [some
constant value] AND Col3 = [some other constant value];

Even though the new partial index matched the query perfectly, the
query planner didn't want to use it. Instead it continued to use some
other sub-optimal index.

The only way to force it to use the correct index was to use the
"+0"-trick which I recently learned from one of my colleagues:
SELECT .. FROM SomeBigTable WHERE Col1 = [some dynamic value] AND
Col2+0 = [some constant value] AND Col3+0 = [some other constant
value]
CREATE INDEX .. ON SomeBigTable USING btree (Col1) WHERE Col2+0 =
[some constant value] AND Col3+0 = [some other constant value];

By adding +0 to the columns, the query planner will as I understand it
be extremely motivated to use the correct index, as otherwise it would
have to do a seq scan on the entire big table, which would be very
costly.

I'm glad the trick worked, now the system is fast again.

We're still on 9.1, so maybe these problems will go away once we upgrade to 9.5.

I don't know if these problems I described can be fixed by your patch,
but I wanted to share this story since I know our systems (Trustly's
and Zalando's) are quite similar in design,
so maybe you have experienced something similar.

(Side note: My biggest wish would be some way to specify explicitly on
a per top-level function level a list of indexes the query planner is
allowed to consider or is NOT allowed to consider.)


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

Re: [HACKERS] Relation extension scalability

[Robert Haas]

On Tue, Mar 8, 2016 at 11:20 AM, Dilip Kumar  wrote:
> I have come up with this patch..
>
> If this approach looks fine then I will prepare final patch (more comments,
> indentation, and improve some code) and do some long run testing (current
> results are 5 mins run).
>
> Idea is same what Robert and Amit suggested up thread.

So this seems promising, but I think the code needs some work.

LockWaiterCount() bravely accesses a shared memory data structure that
is mutable with no locking at all.  That might actually be safe for
our purposes, but I think it would be better to take the partition
lock in shared mode if it doesn't cost too much performance.  If
that's too expensive, then it should at least have a comment
explaining (1) that it is doing this without the lock and (2) why
that's safe (sketch: the LOCK can't go away because we hold it, and
nRequested could change but we don't mind a stale value, and a 32-bit
read won't be torn).

A few of the other functions in here also lack comments, and perhaps
should have them.

The changes to RelationGetBufferForTuple need a visit from the
refactoring police.  Look at the way you are calling
RelationAddOneBlock.  The logic looks about like this:

if (needLock)
{
  if (trylock relation for extension)
RelationAddOneBlock();
  else
  {
lock relation for extension;
if (use fsm)
{
  complicated;
}
else
  RelationAddOneBlock();
}
else
  RelationAddOneBlock();

So basically you want to do the RelationAddOneBlock() thing if
!needLock || !use_fsm || can't trylock.  See if you can rearrange the
code so that there's only one fallthrough call to
RelationAddOneBlock() instead of three separate ones.

Also, consider moving the code that adds multiple blocks at a time to
its own function instead of including it all in line.

-- 
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] More stable query plans via more predictable column statistics

[Robert Haas]

On Wed, Jan 20, 2016 at 5:09 PM, Tom Lane  wrote:
> Alvaro Herrera  writes:
>> Tom Lane wrote:
>>> "Shulgin, Oleksandr"  writes:
 This post summarizes a few weeks of research of ANALYZE statistics
 distribution on one of our bigger production databases with some real-world
 data and proposes a patch to rectify some of the oddities observed.
>
>>> Please add this to the 2016-01 commitfest ...
>
>> Tom, are you reviewing this for the current commitfest?
>
> Um, I would like to review it, but I doubt I'll find time before the end
> of the month.

Tom, can you pick this up?

-- 
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] Minor comment update in setrefs.c

[Robert Haas]

On Fri, Jan 15, 2016 at 5:36 AM, Etsuro Fujita
 wrote:
> The point in the previous patch was to update the list of expressions to be
> adjusted for the case of scanrelid=0 like that for the case of scanrelid>0
> case in set_foreignscan_references.  So, I'd like to propose to add
> *fdw_recheck_quals* to both lists, then.  Updated patch attached.

OK, sure.  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] Allowing to run a buildfarm animal under valgrind

[Andres Freund]

On 2016-03-08 08:58:22 -0500, Andrew Dunstan wrote:
> On 03/07/2016 08:39 PM, Andres Freund wrote:
> >Does anybody have a better idea about how to do this?
> 
> Why not just create a make target which does this? It could be run after
> 'make' and before 'make check'. I would make it assume valgrind was
> installed and in the path rather than using vg-in-place.

I don't really see how that'd work. make check et al. start postgres via
pg_ctl, so we need to invoke valgrind from there. Additionally I don't
want to just be able to run make check via valgrind, but all contrib
modules et al too, including eventually the replication regression tests
and such.

> If that doesn't work and you want to do something with the buildfarm,
> probably a buildfarm module would be the way to go. We might need to add a
> new module hook to support it, to run right after make(), but that would be
> a one line addition to run_build.pl.

Yea, I think that's what it probably has to be. What I'm decidedly
unhappy with right now is that this seems to require moving make install
up, or manually add a new file for installation. The reason for that is
that if we replace the postgres binary with the wrapper, the original
file obviously doesn't get installed anymore. So it's invoked at it's
original location; which only works if share files are installed in the
correct location.


I do wonder if adding a PGCTLPOSTGRESWRAPPER or something to pg_ctl
would be ok. That'd just supplant calling the postgres binary, making
all this easier.

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] Minor documentation tweak to GetForeignPlan documentation

[Robert Haas]

On Fri, Jan 15, 2016 at 6:20 AM, Etsuro Fujita
 wrote:
> Attached patch makes minor modification to the GetForeignPlan
> documentation.  This adds the description about outer_plan, the new
> parameter added in 9.5.

Good catch.  Committed and back-patched to 9.5.

-- 
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] Pushing down sorted joins

[Robert Haas]

On Tue, Feb 23, 2016 at 7:48 AM, Ashutosh Bapat
 wrote:
> Rushabh pointed out that declarations of helper functions
> get_useful_ecs_for_relation and get_useful_pathkeys_for_relation() are part
> of FDW routines declarations rather than helper function declaration. Since
> those functions are related to this patch, the attached patch moves those
> declaration in their right place.

This patch needs to be rebased.

+   /*
+* TODO: we should worry about EPQ path but should
that path have
+* pathkeys? I guess, that's not really important
since it's just
+* going to evaluate the join from whole-row
references stuffed in the
+* corresponding EPQ slots, for which the order doesn't matter.
+*/

The pathkeys for the EPQ path don't matter.  It'll only be called to
recheck one single row, and there's only one order in which you can
return one row.

-   if (bms_equal(em->em_relids, rel->relids))
+   if (bms_is_subset(em->em_relids, rel->relids))

Why do we need this?

-- 
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] Getting sorted data from foreign server for merge join

[Robert Haas]

On Thu, Jan 7, 2016 at 4:05 AM, Ashutosh Bapat
 wrote:
> In get_useful_ecs_for_relation(), while checking whether to use left or
> right argument of a mergejoinable operator, the arguments to bms_is_subset()
> are passed in reverse order. bms_is_subset() checks whether the first
> argument in subset of the second, but in this function the subset to be
> checked is passed as the second argument. Because of this following query
> when run in contrib_regression database after "make installcheck" in
> contrib/postgres_fdw trips assertion Assert(bms_is_subset(relids,
> restrictinfo->left_ec->ec_relids));
>
> EXPLAIN (COSTS false, VERBOSE)
> SELECT t1."C 1" FROM "S 1"."T 1" t1 left join ft1 t2 join ft2 t3 on
> (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
>
> PFA patch to fix it.

The test case failed for me, possibly because of Tom's upper planner
pathification, but the substantive part of the fix looks right to me,
so 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] Optimizer questions

[Konstantin Knizhnik]

On 03/08/2016 07:01 AM, Tom Lane wrote:

Konstantin Knizhnik  writes:

Attached please find improved version of the optimizer patch for LIMIT clause.

This patch isn't anywhere close to working after 3fc6e2d7f5b652b4.
(TBH, the reason I was negative about this upthread is that I had that
one in the oven and knew it would conflict spectacularly.)  I encourage
you to think about how an optimization of this sort could be made to
work in a non-kluge fashion in the new code structure.

I've not spent a lot of time on this, but I think maybe what would make
sense is to consider both the case where function calculations are
postponed to after ORDER BY and the case where they aren't, and generate
Paths for both.  Neither approach is a slam-dunk win.  For example,
suppose that one of the tlist columns is md5(wide_column) --- it will
likely not be preferable to pass the wide column data through the sort
step rather than reducing it to a hash first.  This would require some
work in grouping_planner to track two possible pathtargets, and work in
create_ordered_paths to generate paths for both possibilities.  A possible
objection is that this would add planning work even when no real benefit
is possible; so maybe we should only consider the new way if the tlist has
significant eval cost?  Not sure about that.  There is also something
to be said for the idea that we should try to guarantee consistent
semantics when the tlist contains volatile functions.

For now, I've set this commitfest entry to Waiting on Author.  There's
still time to consider a rewrite in this 'fest, if you can get it done
in a week or two.

regards, tom lane


Attached please find rebased patch.
Unfortunately 3fc6e2d7f5b652b4 still doesn't fix the problem with "lazy" 
evaluation of target list.
This is why my patch is still useful. But frankly speaking I am not sure that 
it is best way of fixing this problem,
because it takes in account only one case: sort+limit. May be the same 
optimization can be useful for other queries.


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 5fc8e5b..709d1ad 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -126,8 +126,9 @@ static RelOptInfo *create_ordered_paths(PlannerInfo *root,
 	 RelOptInfo *input_rel,
 	 double limit_tuples);
 static PathTarget *make_scanjoin_target(PlannerInfo *root, List *tlist,
-	 AttrNumber **groupColIdx);
+		AttrNumber **groupColIdx, bool* splitted_projection);
 static int	get_grouping_column_index(Query *parse, TargetEntry *tle);
+static int	get_sort_column_index(Query *parse, TargetEntry *tle);
 static List *postprocess_setop_tlist(List *new_tlist, List *orig_tlist);
 static List *select_active_windows(PlannerInfo *root, WindowFuncLists *wflists);
 static List *make_windowInputTargetList(PlannerInfo *root,
@@ -1381,6 +1382,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 	RelOptInfo *current_rel;
 	RelOptInfo *final_rel;
 	ListCell   *lc;
+	bool splitted_projection = false;
 
 	/* Tweak caller-supplied tuple_fraction if have LIMIT/OFFSET */
 	if (parse->limitCount || parse->limitOffset)
@@ -1657,7 +1659,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 		 * that were obtained within query_planner().
 		 */
 		sub_target = make_scanjoin_target(root, tlist,
-		  );
+		  , _projection);
 
 		/*
 		 * Forcibly apply that tlist to all the Paths for the scan/join rel.
@@ -1801,6 +1803,13 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
 	{
 		Path	   *path = (Path *) lfirst(lc);
 
+		if (splitted_projection)
+		{			
+			path = apply_projection_to_path(root, current_rel,
+			path, create_pathtarget(root, tlist));
+		}
+
+
 		/*
 		 * If there is a FOR [KEY] UPDATE/SHARE clause, add the LockRows node.
 		 * (Note: we intentionally test parse->rowMarks not root->rowMarks
@@ -3775,15 +3784,17 @@ create_ordered_paths(PlannerInfo *root,
 static PathTarget *
 make_scanjoin_target(PlannerInfo *root,
 	 List *tlist,
-	 AttrNumber **groupColIdx)
+	 AttrNumber **groupColIdx,
+	 bool* splitted_projection)
 {
 	Query	   *parse = root->parse;
-	List	   *sub_tlist;
-	List	   *non_group_cols;
+	List	   *sub_tlist = NIL;
+	List	   *non_group_cols = NIL;
 	List	   *non_group_vars;
 	int			numCols;
 
 	*groupColIdx = NULL;
+	*splitted_projection = false;
 
 	/*
 	 * If we're not grouping or aggregating or windowing, there's nothing to
@@ -3791,14 +3802,66 @@ make_scanjoin_target(PlannerInfo *root,
 	 */
 	if (!parse->hasAggs && !parse->groupClause && !parse->groupingSets &&
 		!root->hasHavingQual && !parse->hasWindowFuncs)
+	{
+		if (parse->sortClause && limit_needed(parse)) {
+			ListCell   *tl;
+			bool contains_non_vars = false;
+			

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

[Robert Haas]

On Tue, Mar 8, 2016 at 12:59 PM, Masahiko Sawada  wrote:
>> How about instead changing things so that we specifically reject
>> indexes?  And maybe some kind of a check that will reject anything
>> that lacks a relfilnode?  That seems like it would be more on point.
>
> I agree, I don't have strong opinion about this.
> It would be good to add condition for rejecting only indexes.
> Attached patches are,
>  - Change heap2 rmgr description
>  - Add condition to pg_visibility
>  - Fix typo in pgvisibility.sgml
> (Sorry for the late notice..)

OK, committed the first and last of those.  I think the other one
needs some work yet; the error message doesn't seem like it is quite
our usual style, and if we're going to do something here we should
probably also insert a check to throw a better error when there is no
relfilenode.

-- 
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] More stable query plans via more predictable column statistics

[Shulgin, Oleksandr]

On Tue, Mar 8, 2016 at 3:36 PM, Joel Jacobson  wrote:

> Hi Alex,
>
> Thanks for excellent research.
>

Joel,

Thank you for spending your time to run these :-)

I've ran your queries against Trustly's production database and I can
> confirm your findings, the results are similar:
>
> WITH ...
> SELECT count(1),
>min(hist_ratio)::real,
>avg(hist_ratio)::real,
>max(hist_ratio)::real,
>stddev(hist_ratio)::real
>   FROM stats2
>  WHERE histogram_bounds IS NOT NULL;
>
> -[ RECORD 1 ]
> count  | 2814
> min| 0.193548
> avg| 0.927357
> max| 1
> stddev | 0.164134
>
>
> WHERE distinct_hist < num_hist
> -[ RECORD 1 ]
> count  | 624
> min| 0.193548
> avg| 0.672407
> max| 0.990099
> stddev | 0.194901
>
>
> WITH ..
> SELECT schemaname ||'.'|| tablename ||'.'|| attname || (CASE inherited
> WHEN TRUE THEN ' (inherited)' ELSE '' END) AS columnname,
>n_distinct, null_frac,
>num_mcv, most_common_vals, most_common_freqs,
>mcv_frac, (mcv_frac / (1 - null_frac))::real AS nonnull_mcv_frac,
>distinct_hist, num_hist, hist_ratio,
>histogram_bounds
>   FROM stats2
>  ORDER BY hist_ratio
>  LIMIT 1;
>
>  -[ RECORD 1
> ]-+-
> columnname| public.x.y
> n_distinct| 103
> null_frac | 0
> num_mcv   | 10
> most_common_vals  | {0,1,2,3,4,5,6,7,8,9}
> most_common_freqs |
>
> {0.4765,0.141733,0.1073,0.0830667,0.0559667,0.037,0.0251,0.0188,0.0141,0.0113667}
> mcv_frac  | 0.971267
> nonnull_mcv_frac  | 0.971267
> distinct_hist | 18
> num_hist  | 93
> hist_ratio| 0.193548387096774
> histogram_bounds  |
>
> {10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,10,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,11,12,12,12,12,12,12,12,12,12,12,12,12,13,13,13,13,13,13,13,13,13,13,14,14,14,14,14,15,15,15,15,16,16,16,16,21,23,5074,5437,5830,6049,6496,7046,7784,14629,21285}
>

I don't want to be asking for too much here, but is there a chance you
could try the effects of the proposed patch on an offline copy of your
database?

Do you envision or maybe have experienced problems with query plans
referring to the columns that are near the top of the above hist_ratio
report?  In other words: what are the practical implications for you with
the values being duplicated rather badly throughout the histogram like in
the example you shown?

Thank you!
--
Alex

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

[Robert Haas]

On Tue, Mar 8, 2016 at 12:49 PM, Tom Lane  wrote:
>> However, after some further thought, I think we might actually be OK.
>> If a page goes from all-frozen to not-all-frozen while VACUUM is
>> running, any new XID added to the page must be newer than the
>> oldestXmin value computed by vacuum_set_xid_limits(), so it won't
>> affect the value to which we can safely set relfrozenxid.  Similarly,
>> any MXID added to the page will be newer than GetOldestMultiXactId(),
>> so setting relminmxid is still safe for similar reasons.
>
> Yeah, I agree with this, as long as the issue is only that the visibility
> map result is slightly stale and not that it's, say, not crash-safe.

If the visibility map isn't crash safe, we've got big problems even
without this patch, but we dealt with that when index-only scans went
in.  Maybe this patch introduces more stringent requirements in this
area, but I can't think of any reason why that should be true.  If
anything occurs to you (or anyone else), it would be good to mention
that before I go further destroy the world.

> We can reasonably assume that any newly-added XID must be one that was
> in progress while VACUUM was running, and hence will be after the xmin
> horizon we computed earlier.  This requires the existence of a read
> barrier somewhere between computing xmin horizon and inspecting the
> visibility map, but I find it hard to believe there aren't plenty.

I'll check that, but I agree that it should be OK.

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


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

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

[Masahiko Sawada]

On Wed, Mar 9, 2016 at 1:23 AM, Jeff Janes  wrote:
> On Tue, Mar 8, 2016 at 5:30 AM, Robert Haas  wrote:

> I left out the relkind check from the final commit because, for one
> thing, the check you added isn't actually right: toast relations can
> also have a visibility map.  And also, I'm sort of wondering what the
> point of that check is.  What does it protect us from?  It doesn't
> seem very future-proof ... what if we add a new relkind in the future?
>  Do we really want to have to update this?
>
> How about instead changing things so that we specifically reject
> indexes?  And maybe some kind of a check that will reject anything
> that lacks a relfilnode?  That seems like it would be more on point.
>

I agree, I don't have strong opinion about this.
It would be good to add condition for rejecting only indexes.
Attached patches are,
 - Change heap2 rmgr description
 - Add condition to pg_visibility
 - Fix typo in pgvisibility.sgml
(Sorry for the late notice..)

Regards,

--
Masahiko Sawada
diff --git a/doc/src/sgml/pgvisibility.sgml b/doc/src/sgml/pgvisibility.sgml
index 6a98b55..cdd6a6f 100644
--- a/doc/src/sgml/pgvisibility.sgml
+++ b/doc/src/sgml/pgvisibility.sgml
@@ -21,7 +21,7 @@
   until such time as a tuple is inserted, updated, deleted, or locked on
   that page.  The page-level PD_ALL_VISIBLE bit has the
   same meaning as the all-visible bit in the visibility map, but is stored
-  within the data page itself rather than a separate data tructure.  These
+  within the data page itself rather than a separate data structure.  These
   will normally agree, but the page-level bit can sometimes be set while the
   visibility map bit is clear after a crash recovery; or they can disagree
   because of a change which occurs after pg_visibility examines
diff --git a/contrib/pg_visibility/pg_visibility.c b/contrib/pg_visibility/pg_visibility.c
index 5e5c7cc..c916d0d 100644
--- a/contrib/pg_visibility/pg_visibility.c
+++ b/contrib/pg_visibility/pg_visibility.c
@@ -56,6 +56,13 @@ pg_visibility_map(PG_FUNCTION_ARGS)
 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
  errmsg("invalid block number")));
 
+	/* Check for relation type */
+	if (rel->rd_rel->relkind == RELKIND_INDEX)
+		ereport(ERROR,
+(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot use for index \"%s\"",
+		RelationGetRelationName(rel;
+
 	tupdesc = pg_visibility_tupdesc(false, false);
 	MemSet(nulls, 0, sizeof(nulls));
 
@@ -95,6 +102,13 @@ pg_visibility(PG_FUNCTION_ARGS)
 (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
  errmsg("invalid block number")));
 
+	/* Check for relation type */
+	if (rel->rd_rel->relkind == RELKIND_INDEX)
+		ereport(ERROR,
+(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot use for index \"%s\"",
+		RelationGetRelationName(rel;
+
 	tupdesc = pg_visibility_tupdesc(false, true);
 	MemSet(nulls, 0, sizeof(nulls));
 
@@ -226,6 +240,13 @@ pg_visibility_map_summary(PG_FUNCTION_ARGS)
 	rel = relation_open(relid, AccessShareLock);
 	nblocks = RelationGetNumberOfBlocks(rel);
 
+	/* Check for relation type */
+	if (rel->rd_rel->relkind == RELKIND_INDEX)
+		ereport(ERROR,
+(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot use for index \"%s\"",
+		RelationGetRelationName(rel;
+
 	for (blkno = 0; blkno < nblocks; ++blkno)
 	{
 		int32		mapbits;
@@ -300,6 +321,13 @@ collect_visibility_data(Oid relid, bool include_pd)
 
 	rel = relation_open(relid, AccessShareLock);
 
+	/* Check for relation type */
+	if (rel->rd_rel->relkind == RELKIND_INDEX)
+		ereport(ERROR,
+(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot use for index \"%s\"",
+		RelationGetRelationName(rel;
+
 	nblocks = RelationGetNumberOfBlocks(rel);
 	info = palloc0(offsetof(vbits, bits) + nblocks);
 	info->next = 0;
diff --git a/src/backend/access/rmgrdesc/heapdesc.c b/src/backend/access/rmgrdesc/heapdesc.c
index a63162c..2b31ea4 100644
--- a/src/backend/access/rmgrdesc/heapdesc.c
+++ b/src/backend/access/rmgrdesc/heapdesc.c
@@ -125,7 +125,8 @@ heap2_desc(StringInfo buf, XLogReaderState *record)
 	{
 		xl_heap_visible *xlrec = (xl_heap_visible *) rec;
 
-		appendStringInfo(buf, "cutoff xid %u", xlrec->cutoff_xid);
+		appendStringInfo(buf, "cutoff xid %u flags %d",
+		 xlrec->cutoff_xid, xlrec->flags);
 	}
 	else if (info == XLOG_HEAP2_MULTI_INSERT)
 	{

-- 
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] JPUG wants to have a copyright notice on the translated doc

[Josh berkus]

On 03/04/2016 06:01 PM, Tatsuo Ishii wrote:

> I imagine kind of an extream case: a bad guy removes "Copyright
> 1996-2016 The PostgreSQL Global Development Group" and replaces it
> with his/her copyright.

The PostgreSQL license does not permit that; you have to retain the
original copyright notice.  You can *add* whatever you want.


-- 
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)


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

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

[Tom Lane]

Robert Haas  writes:
> The patch makes some attempt to update the comment mechanically, but
> that's not nearly enough.  That comment is explaining that you *can't*
> rely on the visibility map to tell you *for sure* that a page does not
> require vacuuming.  For current uses, that's OK, because if we miss a
> page we'll pick it up later.  But now we want to skip vacuuming pages
> for relfrozenxid/relminmxid advancement, that rationale doesn't apply.
> Missing pages that need to be frozen and advancing relfrozenxid anyway
> would be _bad_.

Check.

> However, after some further thought, I think we might actually be OK.
> If a page goes from all-frozen to not-all-frozen while VACUUM is
> running, any new XID added to the page must be newer than the
> oldestXmin value computed by vacuum_set_xid_limits(), so it won't
> affect the value to which we can safely set relfrozenxid.  Similarly,
> any MXID added to the page will be newer than GetOldestMultiXactId(),
> so setting relminmxid is still safe for similar reasons.

Yeah, I agree with this, as long as the issue is only that the visibility
map result is slightly stale and not that it's, say, not crash-safe.
We can reasonably assume that any newly-added XID must be one that was
in progress while VACUUM was running, and hence will be after the xmin
horizon we computed earlier.  This requires the existence of a read
barrier somewhere between computing xmin horizon and inspecting the
visibility map, but I find it hard to believe there aren't plenty.

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] Odd warning from pg_dump

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> So maybe we should drop the hunk you've got there (which frankly seems a
>> bit of a kluge) and instead hot-wire things so that stuff in pg_catalog
>> is excluded even if it would otherwise match the inclusion lists.

> Not sure that's reasonable.  We have at least one extension in contrib
> that creates objects in pg_catalog.  ISTM that's enough precedent that
> more could be created in the future.  (Now of course extensions get
> special treatment anyway, but my point is that there's no prohibition
> against creating objects in pg_catalog.)

True, and given the lack of prior complaints, it might be better to
leave well enough alone here.  What the -general thread was actually
suggesting is that pg_dump needs a way to forcibly omit blobs; the
question about behavior of the pattern-match switches was a sideshow.

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] extend pgbench expressions with functions

[Robert Haas]

On Tue, Mar 8, 2016 at 11:48 AM, Fabien COELHO  wrote:
> If this is a blocker I'll do them, but I'm convince it is not what should be
> done.

Well, I think it's a blocker.  Exiting within deeply-nested code
instead of propagating the error upward does not strike me as a good
practice.

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


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

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

[Robert Haas]

On Mon, Mar 7, 2016 at 12:41 PM, Masahiko Sawada  wrote:
> Attached latest version optimisation patch.
> I'm still consider regarding pg_upgrade regression test code, so I
> will submit that patch later.

I just spent some time looking at this and I'm a bit worried about the
following (existing) comment in vacuumlazy.c:

 * Note: The value returned by visibilitymap_get_status could be slightly
 * out-of-date, since we make this test before reading the corresponding
 * heap page or locking the buffer.  This is OK.  If we mistakenly think
 * that the page is all-visible when in fact the flag's just been cleared,
 * we might fail to vacuum the page.  But it's OK to skip pages when
 * scan_all is not set, so no great harm done; the next vacuum will find
 * them.  If we make the reverse mistake and vacuum a page unnecessarily,
 * it'll just be a no-op.

The patch makes some attempt to update the comment mechanically, but
that's not nearly enough.  That comment is explaining that you *can't*
rely on the visibility map to tell you *for sure* that a page does not
require vacuuming.  For current uses, that's OK, because if we miss a
page we'll pick it up later.  But now we want to skip vacuuming pages
for relfrozenxid/relminmxid advancement, that rationale doesn't apply.
Missing pages that need to be frozen and advancing relfrozenxid anyway
would be _bad_.

However, after some further thought, I think we might actually be OK.
If a page goes from all-frozen to not-all-frozen while VACUUM is
running, any new XID added to the page must be newer than the
oldestXmin value computed by vacuum_set_xid_limits(), so it won't
affect the value to which we can safely set relfrozenxid.  Similarly,
any MXID added to the page will be newer than GetOldestMultiXactId(),
so setting relminmxid is still safe for similar reasons.

I'd appreciate it if any other senior hackers could review that chain
of reasoning.  It would be really bad to get this wrong.

On another note, I didn't really like the way you updated the
documentation.  "eager freezing" doesn't seem like a great term to me,
and I think your changes were a little too localized.  Here's a draft
alternative where I used the term "aggressive vacuum" to describe
freezing all of the pages except for those already known to be
all-frozen.  Thoughts?

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

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

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

 

-VACUUM normally skips pages that don't have any dead row
-versions, but those pages might still have row versions with old XID
-values.  To ensure all old row versions have been frozen, a
-scan of the whole 

Re: [HACKERS] Odd warning from pg_dump

[Alvaro Herrera]

Tom Lane wrote:
> Stephen Frost  writes:
> > I think the real question is if "-n '*'" should still exclude
> > 'pg_catalog'.  Fixing the issue with defined pseudo types is wonderful,
> > but aren't you going to end up with a dump you can't restore,
> > regardless?
> 
> Yeah, perhaps so.  The thread on -general has also produced the
> information that pg_dump -t '*' tries to dump system catalogs as if
> they were user tables, which is another pretty useless bit of behavior.
> So maybe we should drop the hunk you've got there (which frankly seems a
> bit of a kluge) and instead hot-wire things so that stuff in pg_catalog
> is excluded even if it would otherwise match the inclusion lists.

Not sure that's reasonable.  We have at least one extension in contrib
that creates objects in pg_catalog.  ISTM that's enough precedent that
more could be created in the future.  (Now of course extensions get
special treatment anyway, but my point is that there's no prohibition
against creating objects in pg_catalog.)

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

[Tom Lane]

I wrote:
> There might be some other things we could do to provide a fast-path for
> particularly trivial cases.

I wanted to look into that before the code or tests had drifted far enough
to make comparisons dubious.  Attached is a simple patch that lets
grouping_planner fall out with a minimum amount of work if the query is
just "SELECT expression(s)", and a couple of scatter plots of regression
test query timing.  The first plot compares pre-pathification timing with
HEAD+this patch, and the second compares HEAD with HEAD+this patch.

I had hoped to see more of a benefit, actually, but it seems like this
might be enough of a win to be worth committing.  Probably the main
argument against it is that we'd have to remember to maintain the list
of things-to-check-before-using-the-fast-path.

Thoughts?

regards, tom lane

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 5fc8e5b..151f27f 100644
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
*** grouping_planner(PlannerInfo *root, bool
*** 1458,1463 
--- 1458,1504 
  			parse->sortClause,
  			tlist);
  	}
+ 	else if (parse->rtable == NIL &&
+ 			 parse->commandType == CMD_SELECT &&
+ 			 parse->jointree->quals == NULL &&
+ 			 !parse->hasAggs && !parse->hasWindowFuncs &&
+ 			 parse->groupClause == NIL && parse->groupingSets == NIL &&
+ 			 !root->hasHavingQual &&
+ 			 parse->distinctClause == NIL &&
+ 			 parse->sortClause == NIL &&
+ 			 parse->limitOffset == NULL && parse->limitCount == NULL)
+ 	{
+ 		/*
+ 		 * Trivial "SELECT expression(s)" query.  This case would be handled
+ 		 * correctly by the code below, but it comes up often enough to be
+ 		 * worth having a simplified fast-path for.  Need only create a Result
+ 		 * path with the desired targetlist and shove it into the final rel.
+ 		 */
+ 		Path	   *path;
+ 		double		tlist_rows;
+ 
+ 		/* Need not bother with preprocess_targetlist in a SELECT */
+ 		root->processed_tlist = tlist;
+ 
+ 		final_rel = fetch_upper_rel(root, UPPERREL_FINAL, NULL);
+ 
+ 		path = (Path *) create_result_path(final_rel,
+ 		   create_pathtarget(root, tlist),
+ 		   NIL);
+ 
+ 		/* We do take the trouble to fix the rows estimate for SRFs, though */
+ 		tlist_rows = tlist_returns_set_rows(tlist);
+ 		if (tlist_rows > 1)
+ 		{
+ 			path->total_cost += path->rows * (tlist_rows - 1) *
+ cpu_tuple_cost / 2;
+ 			path->rows *= tlist_rows;
+ 		}
+ 
+ 		add_path(final_rel, path);
+ 
+ 		return;
+ 	}
  	else
  	{
  		/* No set operations, do regular planning */

-- 
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] raw output from copy

[Pavel Stehule]

2016-03-04 18:06 GMT+01:00 Pavel Stehule :

>
>
> 2016-03-04 15:54 GMT+01:00 Daniel Verite :
>
>> Corey Huinker wrote:
>>
>> > So, for me, RAW is the right solution, or at least *a* right solution.
>>
>> Questions on how to extract from a bytea column come up on a regular
>> basis, as in [1] [2] [3], or [4] a few days ago, and so far the answers
>> are to encode the contents in text and decode them in an additional
>> step, or use COPY BINARY and filter out the headers.
>>
>> But none of this is as straightforward and efficient as the proposed
>> COPY RAW.
>> Also the conversion to text can't be used at all on very large
>> contents (>512MB), as mentioned in another recent thread [5]
>> (this is the same reason why pg_dump can't dump such rows),
>> but COPY RAW doesn't have this limitation.
>>
>> Technically COPY BINARY should be sufficient, but it seems that
>> people dislike having to deal with its headers.
>>
> Also it's not supported by any of the drivers of popular
>> script languages that otherwise provide COPY in text format
>> (DBD::Pg, php, psycopg2...)
>> Maybe the RAW format would have a better chance to get support
>> there, because of its simplicity.
>>
>
> exactly - I would to decrease dependency on PostgreSQL internals. Working
> with clean content is simple and possible with any environment without
> unclean operations.
>

COPY RAW can be used for import. I am not sure if this use case was tested.

cat image.jpg | psql -c "CREATE TEMP TABLE auxbuf(image bytea); COPY
auxbuf(image) FROM stdin RAW; ..." postgres

Regards

Pavel


> Regards
>
> Pavel
>
>
>>
>> [1]
>>
>> http://www.postgresql.org/message-id/038517CEB6DE43BD8422D7947B6BE8D8@fanliji
>> ng
>>
>> [2] http://www.postgresql.org/message-id/4c8272c4.1000...@arcor.de
>>
>> [3] http://stackoverflow.com/questions/6730729
>>
>> [4]
>> http://www.postgresql.org/message-id/56c66565.50...@consistentstate.com
>>
>> [5] http://www.postgresql.org/message-id/14620.1456851...@sss.pgh.pa.us
>>
>>
>> Best regards,
>> --
>> Daniel Vérité
>> PostgreSQL-powered mailer: http://www.manitou-mail.org
>> Twitter: @DanielVerite
>>
>
>

Re: [HACKERS] extend pgbench expressions with functions

[Fabien COELHO]

Hello Robert,


Having a look at 33-b, this looks pretty good now, but:

// comments are not allowed.  I'd just remove the two you have.


Oops, C89 did not make it everywhere yet!


It make no sense to exit(1) and then return 0, so don't do that. [...]
This would get rid of the internal-error case here altogether in favor
of testing it via an assertion.


Ok. Fine with me.


I think that coerceToInt() should not exit(1) when an overflow occurs;
instead, I think the function should be declared as bool
coerceToInt(PgBenchValue *pval, int64 *result), and the error return
should be propagated back to the caller, which can then return false
as we do for other error cases.


This is the pain (ugly repeated code) that I wish to avoid, because then 
we cannot write a simple addition for doing an addition, but have to do 
several ugly tests instead. Ok, elegance is probably not a sufficient 
argument against doing that.


Moreover, I do not see any condition in which doing what you suggest makes 
much sense from the user perspective: if there is an internal error in the 
bench code it seems much more logical to ask for the user for a sensible 
bench script, because I would not know how to interpret tps on a bench 
with internal failures in the client code anyway.


For me, exiting immediatly on internal script errors is the right action.

If this is a blocker I'll do them, but I'm convince it is not what should 
be done.



I think that some of the places you've used coerceToInt() are not
appropriate.  Like, for example:

+   if
(coerceToInt(rval) == 0)




+
fprintf(stderr, "division by zero\n");
+   return false;
+   }

Now, if rval is out of range of an integer, that is going to overflow
while trying to see whether it should divide by zero.  Please work a
little harder here and in similar cases.


Ok.


 Maybe add a helper function
checkIntegerEquality(PgBenchValue *, int64).


Maybe.

--
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] Odd warning from pg_dump

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > I think the real question is if "-n '*'" should still exclude
> > 'pg_catalog'.  Fixing the issue with defined pseudo types is wonderful,
> > but aren't you going to end up with a dump you can't restore,
> > regardless?
> 
> Yeah, perhaps so.  The thread on -general has also produced the
> information that pg_dump -t '*' tries to dump system catalogs as if
> they were user tables, which is another pretty useless bit of behavior.

Indeed.

> So maybe we should drop the hunk you've got there (which frankly seems a
> bit of a kluge) and instead hot-wire things so that stuff in pg_catalog
> is excluded even if it would otherwise match the inclusion lists.

An idealistic viewpoint might be that we should provide a way to
actually create defined pseudo-types and then make pg_dump work with
them, but I have a hard time seeing why that would be worth the effort.
One might also argue that we should have a way of dealing with every
type of object which exists and defined psuedo-types seem to be the only
thing left out.

I agree that it seems like the best idea is to just hot-wire pg_dump to
exclude pg_catalog, though I'm inclined to suggest that we just exclude
it from pattern matches.  We know that objects sometimes end up in
pg_catalog which aren't really supposed to be there and there might be
other reasons to want to dump it out, so having 'pg_dump -n pg_catalog'
still do its best to dump out what's been asked for seems reasonable.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 03/08/2016 11:17 AM, Tom Lane wrote:

On the whole, I'd rather that this patch left the non-Windows side alone.



OK, that's why I raised the issue. We'll do it that way.

As I noted upthread, the sed script has been very stable so the overhead 
of having to maintain two scripts is pretty minimal.


cheers

andrew



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