Typo in ExecBuildSlotPartitionKeyDescription prologue

[Rushabh Lathia]

Hi

Here is a patch for fixing the function
ExecBuildSlotPartitionKeyDescription()
prologue.


Thanks,
Rushabh Lathia
www.EnterpriseDB.com
diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index d275cef..2fc411a 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -472,7 +472,7 @@ FormPartitionKeyDatum(PartitionDispatch pd,
 }
 
 /*
- * BuildSlotPartitionKeyDescription
+ * ExecBuildSlotPartitionKeyDescription
  *
  * This works very much like BuildIndexValueDescription() and is currently
  * used for building error messages when ExecFindPartition() fails to find

Re: [HACKERS] SQL procedures

[Craig Ringer]

On 23 November 2017 at 03:47, Andrew Dunstan  wrote:

>
>
> On 11/22/2017 02:39 PM, Corey Huinker wrote:
> >
> >
> > T-SQL procedures returns data or OUT variables.
> >
> > I remember, it was very frustrating
> >
> > Maybe first result can be reserved for OUT variables, others for
> > multi result set
> >
> >
> > It's been many years, but if I recall correctly, T-SQL returns a
> > series of result sets, with no description of the result sets to be
> > returned, each of which must be consumed fully before the client can
> > move onto the next result set. Then and only then can the output
> > parameters be read. Which was very frustrating because the OUT
> > parameters seemed like a good place to put values for things like
> > "result set 1 has 205 rows" and "X was false so we omitted one result
> > set entirely" so you couldn't, y'know easily omit entire result sets.
> >
>
>
>
> Exactly. If we want to handle OUT params this way they really need to be
> the first resultset for just this reason. That could possibly be done by
> the glue code reserving a spot in the resultset list and filling it in
> at the end of the procedure.
>

I fail to understand how that can work though. Wouldn't we have to buffer
all the resultset contents on the server in tuplestores or similar, so we
can send the parameters and then the result sets?

Isn't that going to cause a whole different set of painful difficulties
instead?

What it comes down to is that if we want to see output params before result
sets, but the output params are only emitted when the proc returns, then
someone has to buffer. We get to choose if it's the client or the server.

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

Re: [HACKERS] Parallel Append implementation

[amul sul]

On Wed, Nov 22, 2017 at 1:44 AM, Robert Haas  wrote:
> On Tue, Nov 21, 2017 at 6:57 AM, amul sul  wrote:
>> By doing following change on the v19 patch does the fix for me:
>>
>> --- a/src/backend/executor/nodeAppend.c
>> +++ b/src/backend/executor/nodeAppend.c
>> @@ -489,11 +489,9 @@ choose_next_subplan_for_worker(AppendState *node)
>> }
>>
>> /* Pick the plan we found, and advance pa_next_plan one more time. */
>> -   node->as_whichplan = pstate->pa_next_plan;
>> +   node->as_whichplan = pstate->pa_next_plan++;
>> if (pstate->pa_next_plan == node->as_nplans)
>> pstate->pa_next_plan = append->first_partial_plan;
>> -   else
>> -   pstate->pa_next_plan++;
>>
>> /* If non-partial, immediately mark as finished. */
>> if (node->as_whichplan < append->first_partial_plan)
>>
>> Attaching patch does same changes to Amit's ParallelAppend_v19_rebased.patch.
>
> Yes, that looks like a correct fix.  Thanks.
>

Attaching updated version of "ParallelAppend_v19_rebased" includes this fix.

Regards,
Amul


ParallelAppend_v20.patch
Description: Binary data

Re: has_sequence_privilege() never got the memo

[Tom Lane]

Joe Conway  writes:
> I just noticed that has_sequence_privilege() never got the memo about
> "WITH GRANT OPTION". Any objections to the attached going back to all
> supported versions?

That looks odd.  Patch certainly makes this case consistent with the
rest of acl.c, but maybe there's some deeper reason?  Peter?

regards, tom lane

Re: Allowing SSL connection of v11 client to v10 server with SCRAM channel binding

[Michael Paquier]

On Thu, Nov 23, 2017 at 4:08 AM, Peter Eisentraut
 wrote:
> On 11/19/17 23:08, Michael Paquier wrote:
>> When using "n" or "y", the data sent by the client to the server about
>> the use of channel binding is a base64-encoded string of respectively
>> "n,," (biws) and "y,," (eSws). However, as noticed by Peter E here, a
>> v10 server is able to allow connections with "n,,", but not with
>> "y,,":
>> https://www.postgresql.org/message-id/887b6fb7-15fe-239e-2aad-5911d2b09...@2ndquadrant.com
>>
>> When trying to connect to a v11 client based on current HEAD to a v10
>> server using SSL, then the connection would fail. The attached patch,
>> for REL_10_STABLE, allows a server to accept as well as input "eSws",
>> which is a combination that can now happen. This way, a v10 server
>> accepts connections from a v11 and newer client with SSL.
>
> I noticed what I think is an omission in the current v11 code.  We also
> need to check whether the channel binding flag (n/y/p) encoded in the
> client-final-message is the same one used in the client-first-message.
> See attached patch.  This would also affect what we might end up
> backpatching.

Yes, agreed. This patch looks good to me. In fe-auth-scram.c, it would
be also nice to add a comment to keep in sync the logics in
build_client_first_message() and build_client_final_message() which
assign the cbind flag value. I don't see much need for an assertion or
such.
-- 
Michael

has_sequence_privilege() never got the memo

[Joe Conway]

I just noticed that has_sequence_privilege() never got the memo about
"WITH GRANT OPTION". Any objections to the attached going back to all
supported versions?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index fa6b792..2f2758f 100644
*** a/src/backend/utils/adt/acl.c
--- b/src/backend/utils/adt/acl.c
*** convert_sequence_priv_string(text *priv_
*** 2242,2249 
--- 2242,2252 
  {
  	static const priv_map sequence_priv_map[] = {
  		{"USAGE", ACL_USAGE},
+ 		{"USAGE WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_USAGE)},
  		{"SELECT", ACL_SELECT},
+ 		{"SELECT WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_SELECT)},
  		{"UPDATE", ACL_UPDATE},
+ 		{"UPDATE WITH GRANT OPTION", ACL_GRANT_OPTION_FOR(ACL_UPDATE)},
  		{NULL, 0}
  	};
  


signature.asc
Description: OpenPGP digital signature

Re: Query became very slow after 9.6 -> 10 upgrade

[Tom Lane]

Dmitry Shalashov  writes:
> Turns out we had not 9.6 but 9.5.

I'd managed to reproduce the weird planner behavior locally in the
regression database:

regression=# create table foo (f1 int[], f2 int);
CREATE TABLE
regression=# explain select * from tenk1 where unique2 in (select distinct 
unnest(f1) from foo where f2=1);
QUERY PLAN  
   
---
 Nested Loop  (cost=30.85..80.50 rows=6 width=244)
   ->  HashAggregate  (cost=30.57..30.63 rows=6 width=4)
 Group Key: (unnest(foo.f1))
 ->  HashAggregate  (cost=30.42..30.49 rows=6 width=4)
   Group Key: unnest(foo.f1)
   ->  ProjectSet  (cost=0.00..28.92 rows=600 width=4)
 ->  Seq Scan on foo  (cost=0.00..25.88 rows=6 width=32)
   Filter: (f2 = 1)
   ->  Index Scan using tenk1_unique2 on tenk1  (cost=0.29..8.30 rows=1 
width=244)
 Index Cond: (unique2 = (unnest(foo.f1)))
(10 rows)

Digging into it, the reason for the duplicate HashAggregate step was that
query_supports_distinctness() punted on SRFs-in-the-targetlist, basically
on the argument that it wasn't worth extra work to handle that case.
Thinking a bit harder, it seems to me that the correct analysis is:
1. If we are proving distinctness on the grounds of a DISTINCT clause,
then it doesn't matter whether there are any SRFs, because DISTINCT
removes duplicates after tlist SRF expansion.
2. But tlist SRFs break the ability to prove distinctness on the grounds
of GROUP BY, unless all of them are within grouping columns.
It still seems like detecting the second case is harder than it's worth,
but we can trivially handle the first case, with little more than some
code rearrangement.

The other problem is that the output rowcount of the sub-select (ie, of
the HashAggregate) is being estimated as though the SRF weren't there.
This turns out to be because estimate_num_groups() doesn't consider the
possibility of SRFs in the grouping columns.  It never has, but in 9.6 and
before the problem was masked by the fact that grouping_planner scaled up
the result rowcount by tlist_returns_set_rows() *after* performing
grouping.  Now we're effectively doing that in the other order, which is
more correct, but that means estimate_num_groups() has to apply some sort
of adjustment.  I suggest that it just multiply its old estimate by the
maximum of the SRF expansion counts.  That's likely to be an overestimate,
but it's really hard to do better without specific knowledge of the
individual SRF's behavior.

In short, I propose the attached fixes.  I've checked this and it seems
to fix Dmitry's original problem according to the test case he sent
off-list.

regards, tom lane

diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c
index 5b0da14..5783f90 100644
*** a/src/backend/optimizer/plan/analyzejoins.c
--- b/src/backend/optimizer/plan/analyzejoins.c
*** rel_is_distinct_for(PlannerInfo *root, R
*** 744,751 
  bool
  query_supports_distinctness(Query *query)
  {
! 	/* we don't cope with SRFs, see comment below */
! 	if (query->hasTargetSRFs)
  		return false;
  
  	/* check for features we can prove distinctness with */
--- 744,751 
  bool
  query_supports_distinctness(Query *query)
  {
! 	/* SRFs break distinctness except with DISTINCT, see below */
! 	if (query->hasTargetSRFs && query->distinctClause == NIL)
  		return false;
  
  	/* check for features we can prove distinctness with */
*** query_is_distinct_for(Query *query, List
*** 787,806 
  	Assert(list_length(colnos) == list_length(opids));
  
  	/*
- 	 * A set-returning function in the query's targetlist can result in
- 	 * returning duplicate rows, if the SRF is evaluated after the
- 	 * de-duplication step; so we play it safe and say "no" if there are any
- 	 * SRFs.  (We could be certain that it's okay if SRFs appear only in the
- 	 * specified columns, since those must be evaluated before de-duplication;
- 	 * but it doesn't presently seem worth the complication to check that.)
- 	 */
- 	if (query->hasTargetSRFs)
- 		return false;
- 
- 	/*
  	 * DISTINCT (including DISTINCT ON) guarantees uniqueness if all the
  	 * columns in the DISTINCT clause appear in colnos and operator semantics
! 	 * match.
  	 */
  	if (query->distinctClause)
  	{
--- 787,796 
  	Assert(list_length(colnos) == list_length(opids));
  
  	/*
  	 * DISTINCT (including DISTINCT ON) guarantees uniqueness if all the
  	 * columns in the DISTINCT clause appear in colnos and operator semantics
! 	 * match.  This is true even if there are SRFs in the DISTINCT columns or
! 	 * elsewhere in the tlist.
  	 */
  	if (query->distinctClause)
  	{
*** query_is_distinct_for(Query *query, List
*** 820,825 
--- 810,825 
  	}
  
  	/*
+ 	 * Otherwise, a

Re: [HACKERS] Commits don't block for synchronous replication

[Michael Paquier]

On Thu, Nov 23, 2017 at 4:32 AM, Ashwin Agrawal  wrote:
>
> On Wed, Nov 22, 2017 at 9:57 AM, Simon Riggs  wrote:
>>
>> On 15 November 2017 at 10:07, Michael Paquier 
>> wrote:
>> > On Wed, Nov 15, 2017 at 7:28 AM, Ashwin Agrawal 
>> > wrote:
>> >>
>> >> https://commitfest.postgresql.org/15/1297/
>> >>
>> >> Am I missing something or not looking at right place, this is marked as
>> >> committed but don't see the change in latest master ?
>> >
>> > Good thing you double-checked. This has been marked as committed
>> > eleven day ago by Simon (added in CC), but no commit has happened. I
>> > am switching back the status as "ready for committer".
>>
>> The patch has been applied - look at the code. Marking back to committed.
>
> I have no idea which magical place this is being committed, atleast don't
> see on master unless checking something wrong, please can you post the
> commit here ?

I am afraid that I have to agree with Ashwin here, and would like to
know the commit number where you applied it.

The code on HEAD (and back-branches) in syncrep.c, does that, in
SyncRepWaitForLSN():
/*
 * Fast exit if user has not requested sync replication, or there are no
 * sync replication standby names defined. Note that those
standbys don't
 * need to be connected.
 */
if (!SyncRepRequested() || !SyncStandbysDefined())
return;

And the change proposed by Ashwin & co to address what is a bug is that:
/*
-* Fast exit if user has not requested sync replication, or there are no
-* sync replication standby names defined. Note that those
standbys don't
-* need to be connected.
+* Fast exit if user has not requested sync replication.
 */
-   if (!SyncRepRequested() || !SyncStandbysDefined())
+   if (!SyncRepRequested())
return;

On top of that the last commit from a certain Simon Riggs on syncrep.c
is this one:
commit: e05f6f75dbe00a7349dccf1116b5ed983b4728c0
author: Simon Riggs 
date: Fri, 12 Aug 2016 12:43:45 +0100
Code cleanup in SyncRepWaitForLSN()

This is older than the bug report of this thread. All those
indications point out that the patch has *not* been committed. So it
seems to me that you perhaps committed it to your local repository,
but forgot to push it to the remote. I am switching back the patch
status to what looks correct to me "Ready for committer". Thanks.
-- 
Michael

Re: [HACKERS] Transaction control in procedures

[Simon Riggs]

On 18 November 2017 at 02:16, Peter Eisentraut
 wrote:
> On 11/16/17 18:35, Simon Riggs wrote:
>> For the first two answers above the answer was "currently executing
>> statement", yet the third answer seems to be the procedure. So that is
>> a slight discrepancy.
>
> That's the way function execution, or really any nested execution,
> currently works.

I'm impressed that these features are so clean and simple. I wasn't
expecting that. I have very few review comments.

I vote in favour of applying these patches at the end of this CF, end of 11/17.
* Procedures
* Transaction control in PL/pgSQL (only)

That will give us 3 months to discuss problems and find solutions,
then later we can commit PL/Python, PL/perl and PL/tcl once we know
where the dragons are hiding.

If we delay, we will end up with some weird gotcha that needs changing
in the next release.

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

Re: [HACKERS] proposal - Default namespaces for XPath expressions (PostgreSQL 11)

[Thomas Munro]

On Thu, Nov 9, 2017 at 10:11 PM, Pavel Stehule  wrote:
> Attached new version.

Hi Pavel,

FYI my patch testing robot says[1]:

 xml  ... FAILED

regression.diffs says:

+ SELECT x.* FROM t1, xmltable(XMLNAMESPACES(DEFAULT 'http://x.y'),
'/rows/row' PASSING t1.doc COLUMNS data int PATH
'child::a[1][attribute::hoge="haha"]') as x;
+ data
+ --
+ (0 rows)
+

Maybe you forgot to git-add the expected file?

[1] https://travis-ci.org/postgresql-cfbot/postgresql/builds/305979133

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

Re: Fix comment in pg_upgrade

[Magnus Hagander]

On Wed, Nov 22, 2017 at 2:45 PM, Daniel Gustafsson  wrote:

> Attached patch removes a stray word in a comment in
> pg_upgrade/relfilenode.c
> which got left behind back in commit 29add0de4920e4f448a30bfc35798b
> 939c211d97.
>

Applied, thanks.

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

Re: [HACKERS] SQL procedures

[Andrew Dunstan]

On 11/22/2017 02:39 PM, Corey Huinker wrote:
>
>
> T-SQL procedures returns data or OUT variables.
>
> I remember, it was very frustrating
>
> Maybe first result can be reserved for OUT variables, others for
> multi result set
>
>
> It's been many years, but if I recall correctly, T-SQL returns a
> series of result sets, with no description of the result sets to be
> returned, each of which must be consumed fully before the client can
> move onto the next result set. Then and only then can the output
> parameters be read. Which was very frustrating because the OUT
> parameters seemed like a good place to put values for things like
> "result set 1 has 205 rows" and "X was false so we omitted one result
> set entirely" so you couldn't, y'know easily omit entire result sets. 
>



Exactly. If we want to handle OUT params this way they really need to be
the first resultset for just this reason. That could possibly be done by
the glue code reserving a spot in the resultset list and filling it in
at the end of the procedure.

cheers

andrew

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

Re: [HACKERS] SQL procedures

[Corey Huinker]

>
>
> T-SQL procedures returns data or OUT variables.
>
> I remember, it was very frustrating
>
> Maybe first result can be reserved for OUT variables, others for multi
> result set
>
>
It's been many years, but if I recall correctly, T-SQL returns a series of
result sets, with no description of the result sets to be returned, each of
which must be consumed fully before the client can move onto the next
result set. Then and only then can the output parameters be read. Which was
very frustrating because the OUT parameters seemed like a good place to put
values for things like "result set 1 has 205 rows" and "X was false so we
omitted one result set entirely" so you couldn't, y'know easily omit entire
result sets.

Re: [HACKERS] Commits don't block for synchronous replication

[Ashwin Agrawal]

On Wed, Nov 22, 2017 at 9:57 AM, Simon Riggs  wrote:

> On 15 November 2017 at 10:07, Michael Paquier 
> wrote:
> > On Wed, Nov 15, 2017 at 7:28 AM, Ashwin Agrawal 
> wrote:
> >>
> >> https://commitfest.postgresql.org/15/1297/
> >>
> >> Am I missing something or not looking at right place, this is marked as
> >> committed but don't see the change in latest master ?
> >
> > Good thing you double-checked. This has been marked as committed
> > eleven day ago by Simon (added in CC), but no commit has happened. I
> > am switching back the status as "ready for committer".
>
> The patch has been applied - look at the code. Marking back to committed.
>

I have no idea which magical place this is being committed, atleast don't
see on master unless checking something wrong, please can you post the
commit here ?

Re: [HACKERS] SQL procedures

[Andrew Dunstan]

On 11/22/2017 01:10 PM, Tom Lane wrote:
> Peter Eisentraut  writes:
>> On 11/20/17 16:25, Andrew Dunstan wrote:
>>> We should document where returned values in PL procedures are ignored
>>> (plperl, pltcl) and where they are not (plpython, plpgsql). Maybe we
>>> should think about possibly being more consistent here, too.
>> Yeah, suggestions?  I think it makes sense in PL/pgSQL and PL/Python to
>> disallow return values that would end up being ignored, because the only
>> way a return value could arise is if user code explicit calls
>> RETURN/return.  However, in Perl, the return value is the result of the
>> last statement, so prohibiting a return value would be very
>> inconvenient.  (Don't know about Tcl.)  So maybe the current behavior
>> makes sense.  Documentation is surely needed.
> Tcl has the same approach as Perl: the return value of a proc is the
> same as the value of its last command.  There's no such thing as a
> proc that doesn't return a value.
>
>   


Right. We probably just need to document the behaviour here. I'd be
satisfied with that.

cheers

andrew


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

Re: Allowing SSL connection of v11 client to v10 server with SCRAM channel binding

[Peter Eisentraut]

On 11/19/17 23:08, Michael Paquier wrote:
> When using "n" or "y", the data sent by the client to the server about
> the use of channel binding is a base64-encoded string of respectively
> "n,," (biws) and "y,," (eSws). However, as noticed by Peter E here, a
> v10 server is able to allow connections with "n,,", but not with
> "y,,":
> https://www.postgresql.org/message-id/887b6fb7-15fe-239e-2aad-5911d2b09...@2ndquadrant.com
> 
> When trying to connect to a v11 client based on current HEAD to a v10
> server using SSL, then the connection would fail. The attached patch,
> for REL_10_STABLE, allows a server to accept as well as input "eSws",
> which is a combination that can now happen. This way, a v10 server
> accepts connections from a v11 and newer client with SSL.

I noticed what I think is an omission in the current v11 code.  We also
need to check whether the channel binding flag (n/y/p) encoded in the
client-final-message is the same one used in the client-first-message.
See attached patch.  This would also affect what we might end up
backpatching.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 7db5626b0dd082ec782188759316fb345df37999 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Wed, 22 Nov 2017 14:02:57 -0500
Subject: [PATCH] Check channel binding flag at end of SCRAM exchange

We need to check whether the channel-binding flag encoded in the
client-final-message is the same one sent in the client-first-message.
---
 src/backend/libpq/auth-scram.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 22103ce479..bfde6b746d 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -110,6 +110,7 @@ typedef struct
 
const char *username;   /* username from startup packet */
 
+   charcbind_flag;
boolssl_in_use;
const char *tls_finished_message;
size_t  tls_finished_len;
@@ -788,6 +789,7 @@ read_client_first_message(scram_state *state, char *input)
 * Read gs2-cbind-flag.  (For details see also RFC 5802 Section 6 
"Channel
 * Binding".)
 */
+   state->cbind_flag = *input;
switch (*input)
{
case 'n':
@@ -1108,6 +1110,8 @@ read_client_final_message(scram_state *state, char *input)
char   *b64_message;
int b64_message_len;
 
+   Assert(state->cbind_flag == 'p');
+
/*
 * Fetch data appropriate for channel binding type
 */
@@ -1152,10 +1156,11 @@ read_client_final_message(scram_state *state, char 
*input)
/*
 * If we are not using channel binding, the binding data is 
expected
 * to always be "biws", which is "n,," base64-encoded, or 
"eSws",
-* which is "y,,".
+* which is "y,,".  We also have to check whether the flag is 
the same
+* one that the client originally sent.
 */
-   if (strcmp(channel_binding, "biws") != 0 &&
-   strcmp(channel_binding, "eSws") != 0)
+   if (!(strcmp(channel_binding, "biws") == 0 && state->cbind_flag 
== 'n') &&
+   !(strcmp(channel_binding, "eSws") == 0 && 
state->cbind_flag == 'y'))
ereport(ERROR,
(errcode(ERRCODE_PROTOCOL_VIOLATION),
 (errmsg("unexpected SCRAM 
channel-binding attribute in client-final-message";
-- 
2.15.0

Re: [HACKERS] path toward faster partition pruning

[Jesper Pedersen]

Hi Amit,

On 11/22/2017 03:59 AM, Amit Langote wrote:

Fixed in the attached.  No other changes beside that.



I have been using the following script to look at the patch

-- test.sql --

CREATE TABLE t1 (
a integer NOT NULL,
b integer NOT NULL
) PARTITION BY HASH (b);

CREATE TABLE t1_p00 PARTITION OF t1 FOR VALUES WITH (MODULUS 4, 
REMAINDER 0);
CREATE TABLE t1_p01 PARTITION OF t1 FOR VALUES WITH (MODULUS 4, 
REMAINDER 1);
CREATE TABLE t1_p02 PARTITION OF t1 FOR VALUES WITH (MODULUS 4, 
REMAINDER 2);
CREATE TABLE t1_p03 PARTITION OF t1 FOR VALUES WITH (MODULUS 4, 
REMAINDER 3);


CREATE INDEX idx_t1_b_a_p00 ON t1_p00 USING btree (b, a);
CREATE INDEX idx_t1_b_a_p01 ON t1_p01 USING btree (b, a);
CREATE INDEX idx_t1_b_a_p02 ON t1_p02 USING btree (b, a);
CREATE INDEX idx_t1_b_a_p03 ON t1_p03 USING btree (b, a);

CREATE TABLE t2 (
c integer NOT NULL,
d integer NOT NULL
) PARTITION BY HASH (d);

CREATE TABLE t2_p00 PARTITION OF t2 FOR VALUES WITH (MODULUS 4, 
REMAINDER 0);
CREATE TABLE t2_p01 PARTITION OF t2 FOR VALUES WITH (MODULUS 4, 
REMAINDER 1);
CREATE TABLE t2_p02 PARTITION OF t2 FOR VALUES WITH (MODULUS 4, 
REMAINDER 2);
CREATE TABLE t2_p03 PARTITION OF t2 FOR VALUES WITH (MODULUS 4, 
REMAINDER 3);


CREATE INDEX idx_t2_c_p00 ON t2_p00 USING btree (c);
CREATE INDEX idx_t2_c_p01 ON t2_p01 USING btree (c);
CREATE INDEX idx_t2_c_p02 ON t2_p02 USING btree (c);
CREATE INDEX idx_t2_c_p03 ON t2_p03 USING btree (c);

CREATE INDEX idx_t2_d_p00 ON t2_p00 USING btree (d);
CREATE INDEX idx_t2_d_p01 ON t2_p01 USING btree (d);
CREATE INDEX idx_t2_d_p02 ON t2_p02 USING btree (d);
CREATE INDEX idx_t2_d_p03 ON t2_p03 USING btree (d);

INSERT INTO t1 (SELECT i, i FROM generate_series(1, 1) AS i);
INSERT INTO t2 (SELECT i, i FROM generate_series(1, 1) AS i);

ANALYZE;

EXPLAIN (ANALYZE) SELECT t1.a, t1.b FROM t1 WHERE t1.b = 1;

EXPLAIN (ANALYZE) SELECT t1.a, t1.b, t2.c, t2.d FROM t1 INNER JOIN t2 ON 
t2.c = t1.b WHERE t2.d = 1;


BEGIN;
EXPLAIN (ANALYZE) UPDATE t1 SET a = 1 WHERE b = 1;
ROLLBACK;

BEGIN;
EXPLAIN (ANALYZE) DELETE FROM t1 WHERE b = 1;
ROLLBACK;

-- test.sql --

I just wanted to highlight that the "JOIN ON" partition isn't pruned - 
the "WHERE" one is.


Should pruning of partitions for UPDATEs (where the partition key isn't 
updated) and DELETEs be added to the TODO list ?


Thanks for working on this !

Best regards,
 Jesper

Re: [PATCH] using arc4random for strong randomness matters.

[David CARLIER]

> I dunno, it seems like this is opening us to a new set of portability
> hazards (ie, sub-par implementations of arc4random) with not much gain to
> show for it.
>

Hence I reduced to three platforms only.

>
> IIUC, what this code actually does is reseed itself from /dev/urandom
> every so often and work from a PRNG in between.  That's not a layer that
> we need, because the code on top is already designed to cope with the
> foibles of /dev/urandom --- or, to the extent it isn't, that's something
> we have to fix anyway.  So it seems like having this optionally in place
> just reduces what we can assume about the randomness properties of
> pg_strong_random output, which doesn't seem like a good idea.
>
> That I admit these are valid points.
Cheers.


> regards, tom lane
>

Re: [HACKERS] SQL procedures

[Pavel Stehule]

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

> On 11/20/17 16:25, Andrew Dunstan wrote:
> > I've been through this fairly closely, and I think it's pretty much
> > committable. The only big item that stands out for me is the issue of
> > OUT parameters.
>
> I figured that that's something that would come up.  I had intentionally
> prohibited OUT parameters for now so that we can come up with something
> for them without having to break any backward compatibility.
>
> From reading some of the references so far, I think it could be
> sufficient to return a one-row result set and have the drivers adapt the
> returned data into their interfaces.  The only thing that would be a bit
> weird about that is that a CALL command with OUT parameters would return
> a result set and a CALL command without any OUT parameters would return
> CommandComplete instead.  Maybe that's OK.
>

T-SQL procedures returns data or OUT variables.

I remember, it was very frustrating

Maybe first result can be reserved for OUT variables, others for multi
result set

Regards

Pavel



> > Default Privileges docs: although FUNCTIONS and ROUTINES are equivalent
> > here, we should probably advise using ROUTINES, as FUNCTIONS could
> > easily be take to mean "functions but not procedures".
>
> OK, I'll update the documentation a bit more.
>
> > CREATE/ALTER PROCEDURE: It seems more than a little odd to allow
> > attributes that are irrelevant to procedures in these statements. The
> > docco states "for compatibility with ALTER FUNCTION" but why do we want
> > such compatibility if it's meaningless? If we can manage it without too
> > much violence I'd prefer to see an exception raised if these are used.
>
> We can easily be more restrictive here.  I'm open to suggestions.  There
> is a difference between options that don't make sense for procedures
> (e.g., RETURNS NULL ON NULL INPUT), which are prohibited, and those that
> might make sense sometime, but are currently not used.  But maybe that's
> too confusing and we should just prohibit options that are not currently
> used.
>
> > In create_function.sgml, we have:
> >
> > If a schema name is included, then the function is created in the
> > specified schema.  Otherwise it is created in the current schema.
> > -   The name of the new function must not match any existing function
> > +   The name of the new function must not match any existing
> > function or procedure
> > with the same input argument types in the same schema.  However,
> > functions of different argument types can share a name (this is
> > called overloading).
> >
> > The last sentence should probably say "functions and procedures of
> > different argument types" There's a similar issue in
> create_procedure.sqml.
>
> fixing that
>
> > In grant.sgml, there is:
> >
> > +   The FUNCTION syntax also works for
> aggregate
> > +   functions.  Or use ROUTINE to refer to a
> > function,
> > +   aggregate function, or procedure regardless of what it is.
> >
> >
> > I would replace "Or" by "Alternatively,". I think it reads better that
> way.
>
> fixing that
>
> > In functions.c, there is:
> >
> > /* Should only get here for VOID functions */
> > -   Assert(fcache->rettype == VOIDOID);
> > +   Assert(fcache->rettype == InvalidOid || fcache->rettype
> > == VOIDOID);
> > fcinfo->isnull = true;
> > result = (Datum) 0;
> >
> > The comment should also refer to procedures.
>
> fixing that
>
> > It took me a minute to work out what is going on with the new code in
> > aclchk.c:objectsInSchemaToOids(). It probably merits a comment or two.
>
> improving that
>
> > We should document where returned values in PL procedures are ignored
> > (plperl, pltcl) and where they are not (plpython, plpgsql). Maybe we
> > should think about possibly being more consistent here, too.
>
> Yeah, suggestions?  I think it makes sense in PL/pgSQL and PL/Python to
> disallow return values that would end up being ignored, because the only
> way a return value could arise is if user code explicit calls
> RETURN/return.  However, in Perl, the return value is the result of the
> last statement, so prohibiting a return value would be very
> inconvenient.  (Don't know about Tcl.)  So maybe the current behavior
> makes sense.  Documentation is surely needed.
>
> > The context line here looks odd:
> >
> > CREATE PROCEDURE test_proc2()
> > LANGUAGE plpythonu
> > AS $$
> > return 5
> > $$;
> > CALL test_proc2();
> > ERROR:  PL/Python procedure did not return None
> > CONTEXT:  PL/Python function "test_proc2"
> >
> > Perhaps we need to change plpython_error_callback() so that "function"
> > isn't hardcoded.
>
> fixing that
>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & S

Re: [HACKERS] SQL procedures

[Tom Lane]

Peter Eisentraut  writes:
> On 11/20/17 16:25, Andrew Dunstan wrote:
>> We should document where returned values in PL procedures are ignored
>> (plperl, pltcl) and where they are not (plpython, plpgsql). Maybe we
>> should think about possibly being more consistent here, too.

> Yeah, suggestions?  I think it makes sense in PL/pgSQL and PL/Python to
> disallow return values that would end up being ignored, because the only
> way a return value could arise is if user code explicit calls
> RETURN/return.  However, in Perl, the return value is the result of the
> last statement, so prohibiting a return value would be very
> inconvenient.  (Don't know about Tcl.)  So maybe the current behavior
> makes sense.  Documentation is surely needed.

Tcl has the same approach as Perl: the return value of a proc is the
same as the value of its last command.  There's no such thing as a
proc that doesn't return a value.

regards, tom lane

Re: [HACKERS] SQL procedures

[Peter Eisentraut]

On 11/20/17 16:25, Andrew Dunstan wrote:
> I've been through this fairly closely, and I think it's pretty much
> committable. The only big item that stands out for me is the issue of
> OUT parameters.

I figured that that's something that would come up.  I had intentionally
prohibited OUT parameters for now so that we can come up with something
for them without having to break any backward compatibility.

>From reading some of the references so far, I think it could be
sufficient to return a one-row result set and have the drivers adapt the
returned data into their interfaces.  The only thing that would be a bit
weird about that is that a CALL command with OUT parameters would return
a result set and a CALL command without any OUT parameters would return
CommandComplete instead.  Maybe that's OK.

> Default Privileges docs: although FUNCTIONS and ROUTINES are equivalent
> here, we should probably advise using ROUTINES, as FUNCTIONS could
> easily be take to mean "functions but not procedures".

OK, I'll update the documentation a bit more.

> CREATE/ALTER PROCEDURE: It seems more than a little odd to allow
> attributes that are irrelevant to procedures in these statements. The
> docco states "for compatibility with ALTER FUNCTION" but why do we want
> such compatibility if it's meaningless? If we can manage it without too
> much violence I'd prefer to see an exception raised if these are used.

We can easily be more restrictive here.  I'm open to suggestions.  There
is a difference between options that don't make sense for procedures
(e.g., RETURNS NULL ON NULL INPUT), which are prohibited, and those that
might make sense sometime, but are currently not used.  But maybe that's
too confusing and we should just prohibit options that are not currently
used.

> In create_function.sgml, we have:
> 
>     If a schema name is included, then the function is created in the
>     specified schema.  Otherwise it is created in the current schema.
> -   The name of the new function must not match any existing function
> +   The name of the new function must not match any existing
> function or procedure
>     with the same input argument types in the same schema.  However,
>     functions of different argument types can share a name (this is
>     called overloading).
> 
> The last sentence should probably say "functions and procedures of
> different argument types" There's a similar issue in create_procedure.sqml.

fixing that

> In grant.sgml, there is:
> 
> +   The FUNCTION syntax also works for aggregate
> +   functions.  Or use ROUTINE to refer to a
> function,
> +   aggregate function, or procedure regardless of what it is.
> 
> 
> I would replace "Or" by "Alternatively,". I think it reads better that way.

fixing that

> In functions.c, there is:
> 
>     /* Should only get here for VOID functions */
> -   Assert(fcache->rettype == VOIDOID);
> +   Assert(fcache->rettype == InvalidOid || fcache->rettype
> == VOIDOID);
>     fcinfo->isnull = true;
>     result = (Datum) 0;
> 
> The comment should also refer to procedures.

fixing that

> It took me a minute to work out what is going on with the new code in
> aclchk.c:objectsInSchemaToOids(). It probably merits a comment or two.

improving that

> We should document where returned values in PL procedures are ignored
> (plperl, pltcl) and where they are not (plpython, plpgsql). Maybe we
> should think about possibly being more consistent here, too.

Yeah, suggestions?  I think it makes sense in PL/pgSQL and PL/Python to
disallow return values that would end up being ignored, because the only
way a return value could arise is if user code explicit calls
RETURN/return.  However, in Perl, the return value is the result of the
last statement, so prohibiting a return value would be very
inconvenient.  (Don't know about Tcl.)  So maybe the current behavior
makes sense.  Documentation is surely needed.

> The context line here looks odd:
> 
> CREATE PROCEDURE test_proc2()
> LANGUAGE plpythonu
> AS $$
> return 5
> $$;
> CALL test_proc2();
> ERROR:  PL/Python procedure did not return None
> CONTEXT:  PL/Python function "test_proc2"
> 
> Perhaps we need to change plpython_error_callback() so that "function"
> isn't hardcoded.

fixing that

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

Re: [HACKERS] Commits don't block for synchronous replication

[Simon Riggs]

On 15 November 2017 at 10:07, Michael Paquier  wrote:
> On Wed, Nov 15, 2017 at 7:28 AM, Ashwin Agrawal  wrote:
>>
>> https://commitfest.postgresql.org/15/1297/
>>
>> Am I missing something or not looking at right place, this is marked as
>> committed but don't see the change in latest master ?
>
> Good thing you double-checked. This has been marked as committed
> eleven day ago by Simon (added in CC), but no commit has happened. I
> am switching back the status as "ready for committer".

The patch has been applied - look at the code. Marking back to committed.

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

Re: [PATCH] using arc4random for strong randomness matters.

[Tom Lane]

ilm...@ilmari.org (Dagfinn Ilmari =?utf-8?Q?Manns=C3=A5ker?=) writes:
> Tom Lane  writes:
>> More generally, why should we bother with an additional implementation?
>> Is this better than /dev/urandom, and if so why?

> If what is wanted is something more like /dev/urandom, one can call
> getentropy(2) (or on Linux, getrandom(2)) directly, which avoids having
> to open the device file each time.

I dunno, it seems like this is opening us to a new set of portability
hazards (ie, sub-par implementations of arc4random) with not much gain to
show for it.

IIUC, what this code actually does is reseed itself from /dev/urandom
every so often and work from a PRNG in between.  That's not a layer that
we need, because the code on top is already designed to cope with the
foibles of /dev/urandom --- or, to the extent it isn't, that's something
we have to fix anyway.  So it seems like having this optionally in place
just reduces what we can assume about the randomness properties of
pg_strong_random output, which doesn't seem like a good idea.

regards, tom lane

Re: [PATCH] using arc4random for strong randomness matters.

[Andres Freund]

On November 22, 2017 8:51:07 AM PST, ilm...@ilmari.org wrote:
>If what is wanted is something more like /dev/urandom, one can call
>getentropy(2) (or on Linux, getrandom(2)) directly, which avoids having
>to open the device file each time.

What does that buy us for our usages?

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] using arc4random for strong randomness matters.

[Dagfinn Ilmari Mannsåker]

Tom Lane  writes:

> David CARLIER  writes:
>> I m not against as such that depends of the implementation but I ve seen in
>> quick glance it s RC4 ?

arc4random uses ChaCha20 since OpenBSD 5.5 (and libbsd 0.8.0 on Linux).
It uses getentropy(2) to seed itself at regular intervals and at fork().

http://man.openbsd.org/arc4random.3

> More generally, why should we bother with an additional implementation?
> Is this better than /dev/urandom, and if so why?

If what is wanted is something more like /dev/urandom, one can call
getentropy(2) (or on Linux, getrandom(2)) directly, which avoids having
to open the device file each time.

http://man.openbsd.org/getentropy.2
https://manpages.debian.org/stretch/manpages-dev/getrandom.2.en.html

- ilmari
-- 
"The surreality of the universe tends towards a maximum" -- Skud's Law
"Never formulate a law or axiom that you're not prepared to live with
 the consequences of."  -- Skud's Meta-Law

Re: [PATCH] using arc4random for strong randomness matters.

[Andres Freund]

Hi,

Please don't top-quote on postgres mailing lists.


On 2017-11-22 16:16:35 +, David CARLIER wrote:
> > David CARLIER  writes:
> > > I m not against as such that depends of the implementation but I ve seen
> > in
> > > quick glance it s RC4 ?
> >
> > More generally, why should we bother with an additional implementation?
> > Is this better than /dev/urandom, and if so why?

> Basically the call never fails, always generating high quality random data
> (especially the implementations based on Chacha* family, RC4 has
> predictability issues), there is no need of a file descriptor.

I don't really see much benefit in those properties for postgres
specifically. Not needing an fd is nice for cases where you're not
guaranteed to have access to a filesystem, but postgres isn't going to
work in those cases anyway.

Greetings,

Andres Freund

Re: [PATCH] using arc4random for strong randomness matters.

[David CARLIER]

Basically the call never fails, always generating high quality random data
(especially the implementations based on Chacha* family, RC4 has
predictability issues), there is no need of a file descriptor.

On 22 November 2017 at 16:06, Tom Lane  wrote:

> David CARLIER  writes:
> > I m not against as such that depends of the implementation but I ve seen
> in
> > quick glance it s RC4 ?
>
> More generally, why should we bother with an additional implementation?
> Is this better than /dev/urandom, and if so why?
>
> regards, tom lane
>

Re: [PATCH] using arc4random for strong randomness matters.

[Tom Lane]

David CARLIER  writes:
> I m not against as such that depends of the implementation but I ve seen in
> quick glance it s RC4 ?

More generally, why should we bother with an additional implementation?
Is this better than /dev/urandom, and if so why?

regards, tom lane

Re: [PATCH] using arc4random for strong randomness matters.

[David CARLIER]

I m not against as such that depends of the implementation but I ve seen in
quick glance it s RC4 ?

Regards.

On 22 November 2017 at 15:37, David Fetter  wrote:

> On Tue, Nov 21, 2017 at 12:08:46PM +, David CARLIER wrote:
> > Hello,
> >
> > This is my first small personal contribution.
> >
> > Motivation :
> > - Using fail-safe, file descriptor free solution on *BSD and Darwin
> system
> > - Somehow avoiding at the moment FreeBSD as it still uses RC4 (seemingly
> > updated to Chacha20 for FreeBSD 12.0 and eventually backported later on).
> > - For implementation based on Chacha* it is known to be enough fast for
> the
> > purpose.
> > - make check passes.
> >
> > Hope it is good.
> >
> > Thanks in advance.
>
> This is neat.  Apparently, it's useable on Linux with a gizmo called
> libbsd.  Would it be worth it to test for that library on that
> platform?
>
> Best,
> David.
> --
> David Fetter  http://fetter.org/
> Phone: +1 415 235 3778
>
> Remember to vote!
> Consider donating to Postgres: http://www.postgresql.org/about/donate
>

Re: generic-msvc.h(91): error C2664

[Vicky Vergara]

Hello all


FYI: I think I solved it by not including the files and only using


extern "C" {

extern
void* SPI_palloc(size_t size);

extern void *
SPI_repalloc(void *pointer, size_t size);

extern void
SPI_pfree(void *pointer);

}


Thanks

Vicky




De: Vicky Vergara 
Enviado: martes, 21 de noviembre de 2017 09:01 p. m.
Para: pgsql-hack...@postgresql.org
Asunto: generic-msvc.h(91): error C2664


Hello all


I am trying to compile pgRouting on appveyor CI when compiling with pg >= 9.5


I am getting the following error:


C:\Program Files\PostgreSQL\9.5\include\server\port/atomics/generic-msvc.h(91): 
error C2664: '__int64 _InterlockedCompareExchange64(volatile __int64 
*,__int64,__int64)' : cannot convert argument 1 from 'volatile uint64 *' to 
'volatile __int64 *' [C:\build\pgrouting\build\pg95\x64\src\tsp\tsp.vcxproj]


This error happens when C++ code that needs palloc is being compiled.

There is no problem when compiling C code.


On the documentation of the  _InterlockedExchangeAdd64 [1] there is no uint64 * 
type

This code [2] use [3] where uint64 is used


Maybe I am doing something wrong including some postgres files on these [4] & 
[5].


A sample compilation with this error is in [6] and there is no problem at all 
with pg 9.4 [7]


Any feedback would be greatly appreciated

Vicky


[1] 
https://docs.microsoft.com/en-us/cpp/intrinsics/interlockedexchangeadd-intrinsic-functions

[2] 
https://github.com/postgres/postgres/blob/REL9_5_STABLE/src/include/port/atomics/generic-msvc.h#L101

[3] 
https://github.com/postgres/postgres/blob/REL9_5_STABLE/src/include/port/atomics/generic-msvc.h#L43

[4] 
https://github.com/cvvergara/pgrouting/blob/appveyor-pg95/include/cpp_common/pgr_alloc.hpp

[5] 
https://github.com/cvvergara/pgrouting/blob/appveyor-pg95/include/c_common/postgres_connection.h

[6] https://ci.appveyor.com/project/cvvergara/pgrouting/build/2.6.1936#L415

[7] https://ci.appveyor.com/project/cvvergara/pgrouting/build/2.6.1934

Re: [PATCH] using arc4random for strong randomness matters.

[David Fetter]

On Tue, Nov 21, 2017 at 12:08:46PM +, David CARLIER wrote:
> Hello,
> 
> This is my first small personal contribution.
> 
> Motivation :
> - Using fail-safe, file descriptor free solution on *BSD and Darwin system
> - Somehow avoiding at the moment FreeBSD as it still uses RC4 (seemingly
> updated to Chacha20 for FreeBSD 12.0 and eventually backported later on).
> - For implementation based on Chacha* it is known to be enough fast for the
> purpose.
> - make check passes.
> 
> Hope it is good.
> 
> Thanks in advance.

This is neat.  Apparently, it's useable on Linux with a gizmo called
libbsd.  Would it be worth it to test for that library on that
platform?

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

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

Re: [HACKERS] Bug in to_timestamp().

[Arthur Zakirov]

I've attached new version of the patch. It is a little bit simpler now than the 
previous one.
The patch doesn't handle backslashes now, since there was a commit which fixes 
quoted-substring handling recently.
Anyway I'm not sure that this handling was necessary.

I've checked queries from this thread. It seems that they give right timestamps 
and work like in Oracle (except different messages).

The patch contains documentation and regression test fixes.

On Sun, Nov 19, 2017 at 12:26:39PM -0500, Tom Lane wrote:
> I don't much like the error message that you've got:
> 
> +SELECT to_timestamp('97/Feb/16', 'FXYY:Mon:DD');
> +ERROR:  unexpected character "/", expected ":"
> +DETAIL:  The given value did not match any of the allowed values for this 
> field.
> 
> The DETAIL message seems to have been copied-and-pasted into a context
> where it's not terribly accurate.  I'd consider replacing it with
> something along the lines of "HINT: In FX mode, punctuation in the input
> string must exactly match the format string."  This way the report will
> contain a pretty clear statement of the new rule that was broken.  (BTW,
> it's a hint not a detail because it might be guessing wrong as to what
> the real problem is.)
>
>   regards, tom lane

Messages have the following format now:

SELECT to_timestamp('97/Feb/16', 'FXYY:Mon:DD');
ERROR:  unexpected character "/", expected ":"
HINT:  In FX mode, punctuation in the input string must exactly match the 
format string.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 698daf69ea..1c8541d552 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -6166,7 +6166,8 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
to_timestamp and to_date
skip multiple blank spaces in the input string unless the
FX option is used. For example,
-   to_timestamp('2000JUN', ' MON') works, but
+   to_timestamp('2000JUN', ' MON') and
+   to_timestamp('2000 JUN', 'MON') work, but
to_timestamp('2000JUN', 'FX MON') returns an error
because to_timestamp expects one space only.
FX must be specified as the first item in
@@ -6174,6 +6175,19 @@ SELECT regexp_match('abc01234xyz', '(?:(.*?)(\d+)(.*)){1,1}');
   
  
 
+ 
+  
+   to_timestamp and to_date don't
+   skip multiple printable non letter and non digit characters in the input
+   string, but skip them in the formatting string. For example,
+   to_timestamp('2000-JUN', '/MON') and
+   to_timestamp('2000/JUN', '//MON') work, but
+   to_timestamp('2000//JUN', '/MON')
+   returns an error because count of the "/" character in the input string
+   doesn't match count of it in the formatting string.
+  
+ 
+
  
   
Ordinary text is allowed in to_char
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index ec97de0ad2..81565aab96 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -171,6 +171,8 @@ typedef struct
 #define NODE_TYPE_END		1
 #define NODE_TYPE_ACTION	2
 #define NODE_TYPE_CHAR		3
+#define NODE_TYPE_SEPARATOR	4
+#define NODE_TYPE_SPACE		5
 
 #define SUFFTYPE_PREFIX		1
 #define SUFFTYPE_POSTFIX	2
@@ -953,6 +955,7 @@ typedef struct NUMProc
 static const KeyWord *index_seq_search(const char *str, const KeyWord *kw,
  const int *index);
 static const KeySuffix *suff_search(const char *str, const KeySuffix *suf, int type);
+static bool is_separator_char(const char *str);
 static void NUMDesc_prepare(NUMDesc *num, FormatNode *n);
 static void parse_format(FormatNode *node, const char *str, const KeyWord *kw,
 			 const KeySuffix *suf, const int *index, int ver, NUMDesc *Num);
@@ -969,7 +972,6 @@ static void dump_node(FormatNode *node, int max);
 static const char *get_th(char *num, int type);
 static char *str_numth(char *dest, char *num, int type);
 static int	adjust_partial_year_to_2020(int year);
-static int	strspace_len(char *str);
 static void from_char_set_mode(TmFromChar *tmfc, const FromCharDateMode mode);
 static void from_char_set_int(int *dest, const int value, const FormatNode *node);
 static int	from_char_parse_int_len(int *dest, char **src, const int len, FormatNode *node);
@@ -1042,6 +1044,16 @@ suff_search(const char *str, const KeySuffix *suf, int type)
 	return NULL;
 }
 
+static bool
+is_separator_char(const char *str)
+{
+	/* ASCII printable character, but not letter or digit */
+	return (*str > 0x20 && *str < 0x7F &&
+			!(*str >= 'A' && *str <= 'Z') &&
+			!(*str >= 'a' && *str <= 'z') &&
+			!(*str >= '0' && *str <= '9'));
+}
+
 /* --
  * Prepare NUMDesc (number description struct) via FormatNode struct
  * --
@@ -1317,7 +1329,14 @@ parse_format(FormatNode *node, const char *str, const KeyWord *kw,
 	

Fix comment in pg_upgrade

[Daniel Gustafsson]

Attached patch removes a stray word in a comment in pg_upgrade/relfilenode.c
which got left behind back in commit 29add0de4920e4f448a30bfc35798b939c211d97.

cheers ./daniel



pg_upgrade_comment.patch
Description: Binary data

Re: [HACKERS] [POC] Faster processing at Gather node

[Amit Kapila]

On Sat, Nov 18, 2017 at 7:23 PM, Amit Kapila  wrote:
> On Fri, Nov 10, 2017 at 8:39 PM, Robert Haas  wrote:
>> On Fri, Nov 10, 2017 at 5:44 AM, Amit Kapila  wrote:
>>> I am seeing the assertion failure as below on executing the above
>>> mentioned Create statement:
>>
>
> - if (!ExecContextForcesOids(&gatherstate->ps, &hasoid))
> + if (!ExecContextForcesOids(outerPlanState(gatherstate), &hasoid))
>   hasoid = false;
>
> Don't we need a similar change in nodeGatherMerge.c (in function
> ExecInitGatherMerge)?
>
>> And here are the other patches again, too.
>>
>
> The 0001* patch doesn't apply, please find the attached rebased
> version which I have used to verify the patch.
>
> Now, along with 0001* and 0002*, 0003-skip-gather-project-v2 looks
> good to me.  I think we can proceed with the commit of 0001*~0003*
> patches unless somebody else has any comments.
>

I see that you have committed 0001* and 0002* patches, so continuing my review.

Review of 0006-remove-memory-leak-protection-v1

> remove-memory-leak-protection-v1.patch removes the memory leak
> protection that Tom installed upon discovering that the original
> version of tqueue.c leaked memory like crazy.  I think that it
> shouldn't do that any more, courtesy of
> 6b65a7fe62e129d5c2b85cd74d6a91d8f7564608.  Assuming that's correct, we
> can avoid a whole lot of tuple copying in Gather Merge and a much more
> modest amount of overhead in Gather.  Since my test case exercised
> Gather Merge, this bought ~400 ms or so.

I think Tom didn't installed memory protection in nodeGatherMerge.c
related to an additional copy of tuple.  I could see it is present in
the original commit of Gather Merge
(355d3993c53ed62c5b53d020648e4fbcfbf5f155).  Tom just avoided applying
heap_copytuple to a null tuple in his commit
04e9678614ec64ad9043174ac99a25b1dc45233a.  I am not sure whether you
are just referring to the protection Tom added in nodeGather.c,  If
so, it is not clear from your mail.

I think we don't need the additional copy of tuple in
nodeGatherMerge.c and your patch seem to be doing the right thing.
However, after your changes, it looks slightly odd that
gather_merge_clear_tuples() is explicitly calling heap_freetuple for
the tuples allocated by tqueue.c, maybe we can add some comment.  It
was much clear before this patch as nodeGatherMerge.c was explicitly
copying the tuples that it is freeing.

Isn't it better to explicitly call gather_merge_clear_tuples in
ExecEndGatherMerge so that we can free the memory for tuples allocated
in this node rather than relying on reset/free of MemoryContext in
which those tuples are allocated?

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

Re: [HACKERS] Removing LEFT JOINs in more cases

[Ashutosh Bapat]

On Wed, Nov 1, 2017 at 5:39 AM, David Rowley
 wrote:

> In this case, the join *can* cause row duplicates, but the distinct or
> group by would filter these out again anyway, so in these cases, we'd
> not only get the benefit of not joining but also not having to remove
> the duplicate rows caused by the join.

+1.

>
> Given how simple the code is to support this, it seems to me to be
> worth handling.
>

I find this patch very simple and still useful.

@@ -597,15 +615,25 @@ rel_supports_distinctness(PlannerInfo *root,
RelOptInfo *rel)
+if (root->parse->distinctClause != NIL)
+return true;
+
+if (root->parse->groupClause != NIL && !root->parse->hasAggs)
+return true;
+

The other callers of rel_supports_distinctness() are looking for distinctness
of the given relation, whereas the code change here are applicable to any
relation, not just the given relation. I find that confusing. Looking at the
way we are calling rel_supports_distinctness() in join_is_removable() this
change looks inevitable, but still if we could reduce the confusion, that will
be good. Also if we could avoid duplication of comment about unique index, that
will be good.

DISTINCT ON allows only a subset of columns being selected to be listed in that
clause. I initially thought that specifying only a subset would be a problem
and we should check whether the DISTINCT applies to all columns being selected.
But that's not true, since DISTINCT ON would eliminate any duplicates in the
columns listed in that clause, effectively deduplicating the row being
selected. So, we are good there. May be you want to add a testcase with
DISTINCT ON.

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

Logical Replication and PgPool

[Taylor, Nathaniel N.]

All,
I have high availability multi instance implementation of PostgreSQL using 
PgPooL v3.6. I am thinking of doing data separation and change data capture 
using logical replication . Any ideas?


Thank you
Dr. Nathaniel Taylor, Ph.D (Comp. Sci.),C|EH,CASP
LM, CPO, DCPS2- Enterprise  Systems  Database  Architect
Cell : (202) 415-1342

-Original Message-
From: Robert Haas [mailto:robertmh...@gmail.com] 
Sent: Tuesday, November 21, 2017 4:35 PM
To: Simon Riggs 
Cc: Thomas Rosenstein ; Craig Ringer 
; PostgreSQL Hackers 
Subject: Re: Logical Replication and triggers

On Tue, Nov 21, 2017 at 4:28 PM, Simon Riggs  wrote:
> I would have acted on this myself a few days back if I thought the 
> patch was correct, but I see multiple command counter increments 
> there, so suspect an alternate fix is correct.

Oh, well, I'm glad you said something.  I was actually thinking about 
committing the patch Peter posted in 
http://postgr.es/m/619c557d-93e6-1833-1692-b010b176f...@2ndquadrant.com
because it looks correct to me, but I'm certainly not an expert on that code so 
I'll wait for your analysis.

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

Re: [PATCH] using arc4random for strong randomness matters.

[Michael Paquier]

On Tue, Nov 21, 2017 at 9:08 PM, David CARLIER  wrote:
> Motivation :
> - Using fail-safe, file descriptor free solution on *BSD and Darwin system
> - Somehow avoiding at the moment FreeBSD as it still uses RC4 (seemingly
> updated to Chacha20 for FreeBSD 12.0 and eventually backported later on).
> - For implementation based on Chacha* it is known to be enough fast for the
> purpose.
> - make check passes.

This looks like a good idea to me at quick glance. Please make sure to
register it in the next commit fest if yo uwould like to get feedback
about this feature:
https://commitfest.postgresql.org/16/
-- 
Michael

Re: [HACKERS] Parallel Hash take II

[Thomas Munro]

Hi Andres and Peter,

Here's a new patch set with responses to the last batch of review comments.

On Wed, Nov 15, 2017 at 10:24 AM, Andres Freund  wrote:
> Hm. The way you access this doesn't quite seem right:
> ...
> +  matches := regexp_matches(line, '  Batches: ([0-9]+)');
> ...
>
> Why not use format json and access the output that way? Then you can be
> sure you access the right part of the tree and such?

Okay, done.

>> 1.  It determines the amount of unfairness when we run out of data:
>> it's the maximum amount of extra data that the unlucky last worker can
>> finish up with after all the others have finished.  I think this
>> effect is reduced by higher level factors: when a reader runs out of
>> data in one backend's file, it'll start reading another backend's
>> file.  If it's hit the end of all backends' files and this is an outer
>> batch, Parallel Hash will just go and work on another batch
>> immediately.
>
> Consider e.g. what happens if there's the occasional 500MB datum, and
> the rest's very small...
>
>> Better ideas?
>
> Not really. I'm more than a bit suspicous of this solution, but I don't
> really have a great suggestion otherwise.  One way to combat extreme
> size skew would be to put very large datums into different files.

Right.  I considered opening a separate file for each chunk size (4
page, 8 page, 16 page, ...).  Then each file has uniform chunk size,
so you're not stuck with a large chunk size after one monster tuple
comes down the pipe.  I didn't propose that because it leads to even
more file descriptors being used.  I'd like to move towards fewer file
descriptors, because it's a cap on the number of partitions you can
reasonably use.  Perhaps in future we could develop some kind of
general purpose file space manager that would let us allocate extents
within a file, and then SharedTuplestore could allocate extents for
each chunk size.  Not today though.

> But I think we probably can go with your approach for now, ignoring my
> failure prone spidey senses ;)

Cool.

>> > This looks more like the job of an lwlock rather than a spinlock.
>>
>> ... assembler ...
>>
>> That should be OK, right?
>
> It's not too bad. Personally I'm of the opinion though that pretty much
> no new spinlocks should be added - their worst case performance
> characteristics are bad enough for that to be only worth the
> experimentation in case swhere each cycle really matters and where
> contention is unlikely.

Changed to LWLock.

>> > One day we're going to need a better approach to this. I have no idea
>> > how, but this per-node, and now per_node * max_parallelism, approach has
>> > only implementation simplicity as its benefit.
>>
>> I agree, and I am interested in that subject.  In the meantime, I
>> think it'd be pretty unfair if parallel-oblivious hash join and
>> sort-merge join and every other parallel plan get to use work_mem * p
>> (and in some cases waste it with duplicate data), but Parallel Hash
>> isn't allowed to do the same (and put it to good use).
>
> I'm not sure I care about fairness between pieces of code ;)

I'll leave that discussion to run in a separate subthread...

>> BTW this is not per-tuple code -- it runs once at the end of hashing.
>> Not sure what you're looking for here.
>
> It was more a general statement about all the branches in nodeHashjoin,
> than about these specific branches. Should've made that clearer. There's
> definitely branches in very common parts:
>
> ...
>
> I don't think you should do so now, but I think a reasonable approach
> here would be to move the HJ_BUILD_HASHTABLE code into a separate
> function (it really can't be hot). Then have specialized ExecHashJoin()
> versions for parallel/non-parallel and potentially for outer/inner/anti.

Okay, here's my proposal for how to get new branches out of the
per-tuple path.  I have separated ExecHashJoin() and
ExecParallelHashJoin() functions.  They call an inline function
ExecHashJoinImpl() with a constant parameter, so that I effectively
instantiate two variants with the unwanted branches removed by
constant folding.  Then the appropriate variant is installed as the
ExecProcNode function pointer.

Just "inline" wasn't enough though.  I defined
pg_attribute_always_inline to force that on GCC/Clang et al and MSVC.

I also created a separate function ExecParallelScanHashBucket().  I
guess I could have extended the above trick into ExecScanHashBucket()
and further too, but it's called from a different translation unit,
and I also don't want to get too carried away with this technique.  I
chose to supply different functions.

So -- that's introducing a couple of new techniques into the tree.
Treating ExecProcNode as a configuration point for a single node type,
and the function instantiation trick.  Thoughts?

>> > If we don't split this into two versions, we at least should store
>> > hashNode->parallel_state in a local var, so the compiler doesn't have to
>> > pull that out of memory after every e

Re: [HACKERS] UPDATE of partition key

[Amit Khandekar]

On 21 November 2017 at 17:24, Amit Khandekar  wrote:
> On 13 November 2017 at 18:25, David Rowley  
> wrote:
>>
>> 30. The following chunk of code is giving me a headache trying to
>> verify which arrays are which size:
>>
>> ExecSetupPartitionTupleRouting(rel,
>>mtstate->resultRelInfo,
>>(operation == CMD_UPDATE ? nplans : 0),
>>node->nominalRelation,
>>estate,
>>&partition_dispatch_info,
>>&partitions,
>>&partition_tupconv_maps,
>>&subplan_leaf_map,
>>&partition_tuple_slot,
>>&num_parted, &num_partitions);
>> mtstate->mt_partition_dispatch_info = partition_dispatch_info;
>> mtstate->mt_num_dispatch = num_parted;
>> mtstate->mt_partitions = partitions;
>> mtstate->mt_num_partitions = num_partitions;
>> mtstate->mt_parentchild_tupconv_maps = partition_tupconv_maps;
>> mtstate->mt_subplan_partition_offsets = subplan_leaf_map;
>> mtstate->mt_partition_tuple_slot = partition_tuple_slot;
>> mtstate->mt_root_tuple_slot = MakeTupleTableSlot();
>>
>> I know this patch is not completely responsible for it, but you're not
>> making things any better.
>>
>> Would it not be better to invent some PartitionTupleRouting struct and
>> make that struct a member of ModifyTableState and CopyState, then just
>> pass the pointer to that struct to ExecSetupPartitionTupleRouting()
>> and have it fill in the required details? I think the complexity of
>> this is already on the high end, I think you really need to do the
>> refactor before this gets any worse.
>>
>
>Ok. I am currently working on doing this change. So not yet included in the 
>attached patch. Will send yet another revised patch for this change.

Attached incremental patch encapsulate_partinfo.patch (to be applied
over the latest v25 patch) contains changes to move all the
partition-related information into new structure
PartitionTupleRouting. Further to that, I also moved
PartitionDispatchInfo into this structure. So it looks like this :

typedef struct PartitionTupleRouting
{
PartitionDispatch *partition_dispatch_info;
int num_dispatch;
ResultRelInfo **partitions;
int num_partitions;
TupleConversionMap **parentchild_tupconv_maps;
int*subplan_partition_offsets;
TupleTableSlot *partition_tuple_slot;
TupleTableSlot *root_tuple_slot;
} PartitionTupleRouting;

So this structure now encapsulates *all* the
partition-tuple-routing-related information. So ModifyTableState now
has only one tuple-routing-related field 'mt_partition_tuple_routing'.
It is changed like this :

@@ -976,24 +976,14 @@ typedef struct ModifyTableState
TupleTableSlot *mt_existing;/* slot to store existing
target tuple in */
List   *mt_excludedtlist;   /* the excluded pseudo
relation's tlist  */
TupleTableSlot *mt_conflproj;   /* CONFLICT ... SET ...
projection target */
-   struct PartitionDispatchData **mt_partition_dispatch_info;
-   /* Tuple-routing support info */
-   int mt_num_dispatch;/* Number of
entries in the above array */
-   int mt_num_partitions;  /* Number of
members in the following
-
  * arrays */
-   ResultRelInfo **mt_partitions;  /* Per partition result
relation pointers */
-   TupleTableSlot *mt_partition_tuple_slot;
-   TupleTableSlot *mt_root_tuple_slot;
+   struct PartitionTupleRouting *mt_partition_tuple_routing; /*
Tuple-routing support info */
struct TransitionCaptureState *mt_transition_capture;
/* controls transition table population for specified operation */
struct TransitionCaptureState *mt_oc_transition_capture;
/* controls transition table population for INSERT...ON
CONFLICT UPDATE */
-   TupleConversionMap **mt_parentchild_tupconv_maps;
-   /* Per partition map for tuple conversion from root to leaf */
TupleConversionMap **mt_childparent_tupconv_maps;
/* Per plan/partition map for tuple conversion from child to root */
boolmt_is_tupconv_perpart;  /* Is the above map
per-partition ? */
-   int *mt_subplan_partition_offsets;
/* Stores position of update result rels in leaf partitions */
 } ModifyTableState;

So the code in nodeModifyTable.c (and similar code in copy.c) is
accordingly adjusted to use mtstate->mt_partition_tuple_routing.

The places where we use (mtstate->mt_partition_dispatch_info != NULL)
condition to run tuple-routing code, I have replaced it with
mtstate->mt_partition_tuple_routing != NULL.

If you are ok with the incremental patch, I can extract this change
into a separate preparatory patch to be applied on PG master.

Thanks
-Amit Khandekar


encapsulate_partinfo.patch
Description: Binary data

Re: [HACKERS] [PATCH] Incremental sort

[Antonin Houska]

Alexander Korotkov  wrote:

> Antonin Houska  wrote:

> >  * ExecIncrementalSort()
> >
> >  ** if (node->tuplesortstate == NULL)
> > 
> >  If both branches contain the expression
> > 
> >  node->groupsCount++;
> > 
> >  I suggest it to be moved outside the "if" construct.
> 
> Done.

One more comment on this: I wonder if the field isn't incremented too
early. It seems to me that the value can end up non-zero if the input set is
to be empty (not sure if it can happen in practice).

And finally one question about regression tests: what's the purpose of the
changes in contrib/postgres_fdw/sql/postgres_fdw.sql ? I see no
IncrementalSort node in the output.

-- 
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at

Re: [HACKERS] UPDATE of partition key

[Amit Langote]

Thanks Amit.

Looking at the latest v25 patch.

On 2017/11/16 23:50, Amit Khandekar wrote:
> Below has the responses for both Amit's and David's comments, starting
> with Amit's 
> On 2 November 2017 at 12:40, Amit Langote  
> wrote:
>> On 2017/10/24 0:15, Amit Khandekar wrote:
>>> On 16 October 2017 at 08:28, Amit Langote  
>>> wrote:

 +   (event == TRIGGER_EVENT_UPDATE && ((oldtup == NULL) ^ (newtup
 == NULL

 Is there some reason why a bitwise operator is used here?
>>>
>>> That exact condition means that the function is called for transition
>>> capture for updated rows being moved to another partition. For this
>>> scenario, either the oldtup or the newtup is NULL. I wanted to exactly
>>> capture that condition there. I think the bitwise operator is more
>>> user-friendly in emphasizing the point that it is indeed an "either a
>>> or b, not both" condition.
>>
>> I see.  In that case, since this patch adds the new condition, a note
>> about it in the comment just above would be good, because the situation
>> you describe here seems to arise only during update-tuple-routing, IIUC.
> 
> Done. Please check.

Looks fine.

>> + * 'update_rri' has the UPDATE per-subplan result rels. These are re-used
>> + *  instead of allocating new ones while generating the array of all 
>> leaf
>> + *  partition result rels.
>>
>> Instead of:
>>
>> "These are re-used instead of allocating new ones while generating the
>> array of all leaf partition result rels."
>>
>> how about:
>>
>> "There is no need to allocate a new ResultRellInfo entry for leaf
>> partitions for which one already exists in this array"
> 
> Ok. I have made it like this :
> 
> + * 'update_rri' contains the UPDATE per-subplan result rels. For the
> output param
> + * 'partitions', we don't allocate new ResultRelInfo objects for
> + * leaf partitions for which they are already available
> in 'update_rri'.

Sure.

>>> It looks like the interface does not much simplify, and above that, we
>>> have more number of lines in that function. Also, the caller anyway
>>> has to be aware whether the map_index is the index into the leaf
>>> partitions or the update subplans. So it is not like the caller does
>>> not have to be aware about whether the mapping should be
>>> mt_persubplan_childparent_maps or mt_perleaf_parentchild_maps.
>>
>> Hmm, I think we should try to make it so that the caller doesn't have to
>> be aware of that.  And by caller I guess you mean ExecInsert(), which
>> should not be a place, IMHO, where to try to introduce a lot of new logic
>> specific to update tuple routing.
> 
> I think, for ExecInsert() since we have already given the job of
> routing the tuple from root partitioned table to a partition, it makes
> sense to give the function the additional job of routing the tuple
> from any partition to any partition. ExecInsert() can be looked at as
> doing this job : "insert a tuple into the right partition; the
> original tuple can belong to any partition"

Yeah, that's one way of looking at that.  But I think ExecInsert() as it
is today thinks it's got a *new* tuple and that's it.  I think the newly
introduced code in it to find out that it is not so (that the tuple
actually comes from some other partition), that it's really the
update-turned-into-delete-plus-insert, and then switch to the root
partitioned table's ResultRelInfo, etc. really belongs outside of it.
Maybe in its caller, which is ExecUpdate().  I mean why not add this code
to the block in ExecUpdate() that handles update-row-movement.

Just before calling ExecInsert() to do the re-routing seems like a good
place to do all that.  For example, try the attached incremental patch
that applies on top of yours.  I can see after applying it that diffs to
ExecInsert() are now just some refactoring ones and there are no
significant additions making it look like supporting update-row-movement
required substantial changes to how ExecInsert() itself works.

> After doing the changes for the int[] array map in the previous patch
> version, I still feel that ConvertPartitionTupleSlot() should be
> retained. We save some repeated lines of code saved.

OK.

>> You may be right, but I see for WithCheckOptions initialization
>> specifically that the non-tuple-routing code passes the actual sub-plan
>> when initializing the WCO for a given result rel.
> 
> Yes that's true. The problem with WithCheckOptions for newly allocated
> partition result rels is : we can't use a subplan for the parent
> parameter because there is no subplan for it. But I will still think
> on it a bit more (TODO).

Alright.

>>> I think you are suggesting we do it like how it's done in
>>> is_partition_attr(). Can you please let me know other places we do
>>> this same way ? I couldn't find.
>>
>> OK, not as many as I thought there would be, but there are following
>> beside is_partition_attrs():
>>
>> partition.c: get_range_nulltest()
>> partition.c: g

Re: default range partition and constraint exclusion

[Amit Langote]

On 2017/11/22 6:31, Robert Haas wrote:
> On Tue, Nov 21, 2017 at 4:36 AM, Amit Langote
>  wrote:
 The attached will make the constraint to look like:
>>>
>>> Uh, if the constraint exclusion logic we're using is drawing false
>>> conclusions, we need to fix it so it doesn't, not change the
>>> constraint so that the wrong logic gives the right answer.
>>
>> I did actually consider it, but ended up concluding that constraint
>> exclusion is doing all it can using the provided list partition constraint
>> clauses.
>>
>> If all predicate_refuted_by() receives is the expression tree (AND/OR)
>> with individual nodes being strict clauses involving partition keys (and
>> nothing about the nullness of the keys), the downstream code is just
>> playing by the rules as explained in the header comment of
>> predicate_refuted_by_recurse() in concluding that query's restriction
>> clause a = 2 refutes it.
> 
> Oh, wait a minute.  Actually, I think predicate_refuted_by() is doing
> the right thing here.  Isn't the problem that mc2p2 shouldn't be
> accepting a (2, null) tuple at all?

It doesn't.  But, for a query, it does contain (2, ) tuples,
where  would always be non-null.  So, it should be scanned in the
plan for the query that has only a = 2 as restriction and no restriction
on b.  That seems to work.

\d+ mc2p2
...
Partition of: mc2p FOR VALUES FROM (1, 1) TO (MAXVALUE, MAXVALUE)
Partition constraint: ((a IS NOT NULL) AND (b IS NOT NULL) AND ((a > 1) OR
((a = 1) AND (b >= 1

insert into mc2p2 values (2, null);
ERROR:  new row for relation "mc2p2" violates partition constraint
DETAIL:  Failing row contains (2, null).

explain select * from mc2p where a = 2;
 QUERY PLAN
-
 Append  (cost=0.00..38.25 rows=11 width=8)
   ->  Seq Scan on mc2p2  (cost=0.00..38.25 rows=11 width=8)
 Filter: (a = 2)
(3 rows)


My complaint is about m2p_default not being included in the plan for a
query with only a = 2 restriction.  The aforementioned  includes
null and it's only m2p_default that has such tuples, so it should be in
the plan for queries that don't explicitly prevent null values for b by
including b is not null in the query.

With the patch:

explain (costs off) select * from mc2p where a = 2;
QUERY PLAN
---
 Append
   ->  Seq Scan on mc2p_default
 Filter: ((b IS NULL) AND (a = 2))
(3 rows)

explain (costs off) select * from mc2p where a = 2 and b is null;
QUERY PLAN
---
 Append
   ->  Seq Scan on mc2p_default
 Filter: ((b IS NULL) AND (a = 2))
(3 rows)

explain (costs off) select * from mc2p where a = 2 and b is not null;
 QUERY PLAN
-
 Append
   ->  Seq Scan on mc2p2
 Filter: ((b IS NOT NULL) AND (a = 2))
(3 rows)

Thanks,
Amit

Re: With commit 4e5fe9ad19, range partition missing handling for the NULL partition key

[Amit Langote]

On 2017/11/22 17:42, amul sul wrote:
> On Wed, Nov 22, 2017 at 11:38 AM, Amit Langote wrote:
>> On 2017/11/22 13:45, Rushabh Lathia wrote:
>>> Attaching patch to fix as well as regression test.
>>
>> Thanks for the patch.  About the code, how about do it like the attached
>> instead?
>>
> 
> Looks good to me, even we can skip the following change in v2 patch:
> 
> 19 @@ -2560,6 +2559,8 @@ get_partition_for_tuple(Relation relation,
> Datum *values, bool *isnull)
>  20  */
>  21 part_index =
> partdesc->boundinfo->indexes[bound_offset + 1];
>  22 }
>  23 +   else
>  24 +   part_index = partdesc->boundinfo->default_index;
>  25 }
>  26 break;
>  27
> 
> default_index will get assign by following code in get_partition_for_tuple() :
> 
>/*
>  * part_index < 0 means we failed to find a partition of this parent.
>  * Use the default partition, if there is one.
>  */
> if (part_index < 0)
> part_index = partdesc->boundinfo->default_index;

Good point.  Updated patch attached.

Thanks,
Amit
diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index 9a44cceb22..3b5df5164a 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -2531,16 +2531,14 @@ get_partition_for_tuple(Relation relation, Datum 
*values, bool *isnull)
 
/*
 * No range includes NULL, so this will be 
accepted by the
-* default partition if there is one, and 
otherwise
-* rejected.
+* default partition if there is one, otherwise 
rejected.
 */
for (i = 0; i < key->partnatts; i++)
{
-   if (isnull[i] &&
-   
partition_bound_has_default(partdesc->boundinfo))
+   if (isnull[i])
{
range_partkey_has_null = true;
-   part_index = 
partdesc->boundinfo->default_index;
+   break;
}
}
 
diff --git a/src/test/regress/expected/insert.out 
b/src/test/regress/expected/insert.out
index 9d84ba4658..a0e3746e70 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -642,6 +642,10 @@ create table mcrparted2 partition of mcrparted for values 
from (10, 6, minvalue)
 create table mcrparted3 partition of mcrparted for values from (11, 1, 1) to 
(20, 10, 10);
 create table mcrparted4 partition of mcrparted for values from (21, minvalue, 
minvalue) to (30, 20, maxvalue);
 create table mcrparted5 partition of mcrparted for values from (30, 21, 20) to 
(maxvalue, maxvalue, maxvalue);
+-- null not allowed in range partition
+insert into mcrparted values (null, null, null);
+ERROR:  no partition of relation "mcrparted" found for row
+DETAIL:  Partition key of the failing row contains (a, abs(b), c) = (null, 
null, null).
 -- routed to mcrparted0
 insert into mcrparted values (0, 1, 1);
 insert into mcrparted0 values (0, 1, 1);
diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql
index 791817ba50..1c4491a6a2 100644
--- a/src/test/regress/sql/insert.sql
+++ b/src/test/regress/sql/insert.sql
@@ -417,6 +417,9 @@ create table mcrparted3 partition of mcrparted for values 
from (11, 1, 1) to (20
 create table mcrparted4 partition of mcrparted for values from (21, minvalue, 
minvalue) to (30, 20, maxvalue);
 create table mcrparted5 partition of mcrparted for values from (30, 21, 20) to 
(maxvalue, maxvalue, maxvalue);
 
+-- null not allowed in range partition
+insert into mcrparted values (null, null, null);
+
 -- routed to mcrparted0
 insert into mcrparted values (0, 1, 1);
 insert into mcrparted0 values (0, 1, 1);

Re: With commit 4e5fe9ad19, range partition missing handling for the NULL partition key

[amul sul]

On Wed, Nov 22, 2017 at 11:38 AM, Amit Langote
 wrote:
> Hi Rushabh,
>
> On 2017/11/22 13:45, Rushabh Lathia wrote:
>> Consider the below test:
>>
>> CREATE TABLE range_tab(a int, b int) PARTITION BY RANGE(a);
>> CREATE TABLE range_tab_p1 PARTITION OF range_tab FOR VALUES FROM (minvalue)
>> TO (10);
>> CREATE TABLE range_tab_p2 PARTITION OF range_tab FOR VALUES FROM (10) TO
>> (20);
>> CREATE TABLE range_tab_p3 PARTITION OF range_tab FOR VALUES FROM (20) TO
>> (maxvalue);
>>
>> INSERT INTO range_tab VALUES(NULL, 10);
>>
>> Above insert should fail with an error "no partition of relation found for
>> row".
>>
>> Looking further I found that, this behaviour is changed after below commit:
>>
>> commit 4e5fe9ad19e14af360de7970caa8b150436c9dec
>> Author: Robert Haas 
>> Date:   Wed Nov 15 10:23:28 2017 -0500
>>
>> Centralize executor-related partitioning code.
>>
>> Some code is moved from partition.c, which has grown very quickly
>> lately;
>> splitting the executor parts out might help to keep it from getting
>> totally out of control.  Other code is moved from execMain.c.  All is
>> moved to a new file execPartition.c.  get_partition_for_tuple now has
>> a new interface that more clearly separates executor concerns from
>> generic concerns.
>>
>> Amit Langote.  A slight comment tweak by me.
>>
>> Before above commit insert with NULL partition key in the range partition
>> was throwing a proper error.
>
> Oops, good catch.
>
>> Attaching patch to fix as well as regression test.
>
> Thanks for the patch.  About the code, how about do it like the attached
> instead?
>

Looks good to me, even we can skip the following change in v2 patch:

19 @@ -2560,6 +2559,8 @@ get_partition_for_tuple(Relation relation,
Datum *values, bool *isnull)
 20  */
 21 part_index =
partdesc->boundinfo->indexes[bound_offset + 1];
 22 }
 23 +   else
 24 +   part_index = partdesc->boundinfo->default_index;
 25 }
 26 break;
 27

default_index will get assign by following code in get_partition_for_tuple() :

   /*
 * part_index < 0 means we failed to find a partition of this parent.
 * Use the default partition, if there is one.
 */
if (part_index < 0)
part_index = partdesc->boundinfo->default_index;


Regards,
Amul

Re: Failed to delete old ReorderBuffer spilled files

[Thomas Munro]

On Wed, Nov 22, 2017 at 12:27 AM, atorikoshi
 wrote:
> [set_final_lsn_2.patch]

Hi Torikoshi-san,

FYI "make check" in contrib/test_decoding fails a couple of isolation
tests, one with an assertion failure for my automatic patch tester[1].
Same result on my laptop:

test ondisk_startup   ... FAILED (test process exited with exit code 1)
test concurrent_ddl_dml   ... FAILED (test process exited with exit code 1)

TRAP: FailedAssertion("!(!dlist_is_empty(head))", File:
"../../../../src/include/lib/ilist.h", Line: 458)

[1] https://travis-ci.org/postgresql-cfbot/postgresql/builds/305636335

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