date:20170307

Re: [HACKERS] contrib modules and relkind check

2017-03-07 Thread Michael Paquier

On Tue, Mar 7, 2017 at 4:15 PM, Amit Langote
 wrote:
> Sorry about the absence on this thread.

No problems! Thanks for showing up with an updated patch.

> On 2017/02/14 15:30, Michael Paquier wrote:
>> On Tue, Feb 14, 2017 at 3:18 PM, Amit Langote wrote:
>>>
>>> Added more tests in pgstattuple and the new ones for pg_visibility,
>>> although I may have overdone the latter.
>>
>> A bonus idea is also to add tests for relkinds that work, with for
>> example the creation of a table, inserting some data in it, vacuum it,
>> and look at "SELECT count(*) > 0 FROM pg_visibility('foo'::regclass)".
>
> I assume you meant only for pg_visibility.  Done in the attached (a pretty
> basic test though).

Yep.

> If we decide to go with some different approach, we'd not be doing it
> here.  Maybe in the "partitioned tables and relfilenode" thread or a new one.

Okay.

+++ b/contrib/pg_visibility/expected/pg_visibility.out
@@ -0,0 +1,85 @@
+CREATE EXTENSION pg_visibility;
+--
+-- check that using the module's functions with unsupported relations will fail
+--
[...]
+select count(*) > 0 from pg_visibility('regular_table');
+ ?column?
+--
+ t
+(1 row)
Only regular tables are tested as valid objects. Testing toast tables
is not worth the complication. Could you add as well a matview?

Except for this small issue the patch looks good to me.
-- 
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] Enabling replication connections by default in pg_hba.conf

2017-03-07 Thread Michael Paquier

On Wed, Mar 8, 2017 at 11:29 AM, Peter Eisentraut
 wrote:
> On 3/6/17 21:11, Michael Paquier wrote:
>> I think that the documentation of initdb should mention that
>> pg_hba.conf entries are configured for replication connections as
>> well, something like a sentence in the Description paragraph:
>> initdb sets pg_hba.conf entries using the specified authentication
>> method (trust by default) for non-replication as well as replication
>> connections.
>
> OK, I was looking for a way to document this.  Your ideas seems the most
> sensible.
>
> See attached patch that puts it all together.

Thanks for the new version.

+This option specifies the default authentication method for local
+users used in pg_hba.conf (host
+and local lines).  initdb will
+prepopulate pg_hba.conf entries using the
+specified authentication method for non-replication as well as
+replication connections.
Fine for me with a paragraph here.

This patch looks good to me.
-- 
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] parallel index(-only) scan breaks when run without parallelism

2017-03-07 Thread Amit Kapila

On Wed, Mar 8, 2017 at 12:27 AM, Robert Haas  wrote:
> Amit, Rafia,
>
> nodeIndexscan.c, unlike nodeSeqscan.c, thinks that a parallel-aware
> scan will always be executed in parallel mode.  But that's not true:
> an Execute message with a non-zero row count could cause us to abandon
> planned parallelism and execute the plan serially.
>

Right, and the current code had assumed that if there is a parallel
plan then it will always enter the parallel mode.   I think the fix is
quite similar to what we do in nodeSeqscan.c i.e. initialize the scan
descriptor before starting the scan if it is not already initialized.
 There is an additional check required for ensuring if index runtime
keys are ready before calling index_rescan.  Attached patch fixes the
problem.

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

fix_scandesc_initialization_parallel_index_v1.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] Parallel Append implementation

2017-03-07 Thread Amit Khandekar

On 19 February 2017 at 14:59, Robert Haas  wrote:
> On Fri, Feb 17, 2017 at 2:56 PM, Amit Khandekar  
> wrote:
>> The log2(num_children)+1 formula which you proposed does not take into
>> account the number of workers for each of the subplans, that's why I
>> am a bit more inclined to look for some other logic. May be, treat the
>> children as if they belong to partitions, and accordingly calculate
>> the final number of workers. So for 2 children with 4 and 5 workers
>> respectively, Append parallel_workers would be : log3(3^4 + 3^5) .
>
> In general this will give an answer not different by more than 1 or 2
> from my answer, and often exactly the same.  In the case you mention,
> whether we get the same answer depends on which way you round:
> log3(3^4+3^5) is 5 if you round down, 6 if you round up.
>
> My formula is more aggressive when there are many subplans that are
> not parallel or take only 1 worker, because I'll always use at least 5
> workers for an append that has 9-16 children, whereas you might use
> only 2 if you do log3(3^0+3^0+3^0+3^0+3^0+3^0+3^0+3^0+3^0).  In that
> case I like my formula better. With lots of separate children, the
> chances of being able to use as many as 5 workers seem good.  (Note
> that using 9 workers as Ashutosh seems to be proposing would be a
> waste if the different children have very unequal execution times,
> because the workers that run children with short execution times can
> be reused to run additional subplans while the long ones are still
> running.  Running a separate worker for each child only works out if
> the shortest runtime is more than 50% of the longest runtime, which
> may sometimes be true but doesn't seem like a good bet in general.)
>
> Your formula is more aggressive when you have 3 children that all use
> the same number of workers; it'll always decide on  per child>+1, whereas mine won't add the extra worker in that case.
> Possibly your formula is better than mine in that case, but I'm not
> sure.  If you have as many as 9 children that all want N workers, your
> formula will decide on N+2 workers, but since my formula guarantees a
> minimum of 5 workers in such cases, I'll probably be within 1 of
> whatever answer you were getting.
>

Yeah, that seems to be right in most of the cases. The only cases
where your formula seems to give too few workers is for something like
: (2, 8, 8). For such subplans, we should at least allocate 8 workers.
It turns out that in most of the cases in my formula, the Append
workers allocated is just 1 worker more than the max per-subplan
worker count. So in (2, 1, 1, 8), it will be a fraction more than 8.
So in the patch, in addition to the log2() formula you proposed, I
have made sure that it allocates at least equal to max(per-subplan
parallel_workers values).

>
>> BTW, there is going to be some logic change in the choose-next-subplan
>> algorithm if we consider giving extra workers to subplans.
>
> I'm not sure that it's going to be useful to make this logic very
> complicated.  I think the most important thing is to give 1 worker to
> each plan before we give a second worker to any plan.  In general I
> think it's sufficient to assign a worker that becomes available to the
> subplan with the fewest number of workers (or one of them, if there's
> a tie) without worrying too much about the target number of workers
> for that subplan.

In the attached v5 patch, the logic of distributing the workers is now
kept simple : it just distributes the workers equally without
considering the per-sublan parallel_workers value. I have retained the
earlier logic of choosing the plan with minimum current workers. But
now that the pa_max_workers is not needed, I removed it, and instead a
partial_plans bitmapset is added in the Append node. Once a worker
picks up a non-partial subplan, it immediately changes its
pa_num_workers to -1. Whereas for partial subplans, the worker sets it
to -1 only after it finishes executing it.

Effectively, in parallel_append_next(), the check for whether subplan
is executing with max parallel_workers is now removed, and all code
that was using pa_max_workers is now removed.

Ashutosh Bapat  wrote:
> 10. We should probably move the parallel_safe calculation out of 
> cost_append().
> +path->parallel_safe = path->parallel_safe &&
> +  subpath->parallel_safe;
>
> 11. This check shouldn't be part of cost_append().
> +/* All child paths must have same parameterization */
> +Assert(bms_equal(PATH_REQ_OUTER(subpath), required_outer));
>

Moved out these two statements from cost_append(). Did it separately
in create_append_path().

Also, I have removed some elog() statements which were there while
inside Spinlock in parallel_append_next().

On 17 January 2017 at 11:10, Amit Langote  wrote:
> I was looking at the

[HACKERS] [bug fix] dblink leaks unnamed connections

2017-03-07 Thread Tsunakawa, Takayuki

Hello,

dblink fails to close the unnamed connection as follows when a new unnamed 
connection is requested.  The attached patch fixes this.



postgres=# select count(*) from pg_stat_activity;
 count
---
 1
(1 row)

postgres=# select dblink_connect('dbname=postgres');
 dblink_connect

 OK
(1 row)

postgres=# select count(*) from pg_stat_activity;
 count
---
 2
(1 row)

postgres=# select dblink_connect('dbname=postgres');
 dblink_connect

 OK
(1 row)

postgres=# select count(*) from pg_stat_activity;
 count
---
 3
(1 row)


Regards
Takayuki Tsunakawa



dblink_leak_unnamed_conn.patch
Description: dblink_leak_unnamed_conn.patch

-- 
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] CREATE/ALTER ROLE PASSWORD ('value' USING 'method')

2017-03-07 Thread Michael Paquier

On Wed, Mar 8, 2017 at 2:32 PM, Rushabh Lathia  wrote:
> One quick comments:
>
> +| PASSWORD '(' Sconst USING Sconst ')'
> +{
> +$$ = makeDefElem("methodPassword",
> + (Node *)list_make2(makeString($3),
> +makeString($5)),
> + @1);
> +}
>
> methodPassword looks bit weird, can we change it to passwordMethod
> or pwdEncryptMethod ?

Using "Password" in suffix looks still better to me though for
consistency with the other ones.
-- 
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] Parallel bitmap heap scan

2017-03-07 Thread Dilip Kumar

On Tue, Mar 7, 2017 at 10:10 PM, Dilip Kumar  wrote:
>> It's not about speed.  It's about not forgetting to prefetch.  Suppose
>> that worker 1 becomes the prefetch worker but then doesn't return to
>> the Bitmap Heap Scan node for a long time because it's busy in some
>> other part of the plan tree.  Now you just stop prefetching; that's
>> bad.  You want prefetching to continue regardless of which workers are
>> busy doing what; as long as SOME worker is executing the parallel
>> bitmap heap scan, prefetching should continue as needed.
>
> Right, I missed this part. I will fix this.
I have fixed this part, after doing that I realised if multiple
processes are prefetching then it may be possible that in boundary
cases (e.g. suppose prefetch_target is 3 and prefetch_pages is at 2)
there may be some extra prefetch but finally those prefetched blocks
will be used.  Another, solution to this problem is that we can
increase the prefetch_pages in advance then call tbm_shared_iterate,
this will avoid extra prefetch.  But I am not sure what will be best
here.


On Tue, Mar 7, 2017 at 9:41 PM, Robert Haas  wrote:
> +if (--ptbase->refcount == 0)
> +dsa_free(dsa, istate->pagetable);
> +
> +if (istate->spages)
> +{
> +ptpages = dsa_get_address(dsa, istate->spages);
> +if (--ptpages->refcount == 0)
> +dsa_free(dsa, istate->spages);
> +}
> +if (istate->schunks)
> +{
> +ptchunks = dsa_get_address(dsa, istate->schunks);
> +if (--ptchunks->refcount == 0)
> +dsa_free(dsa, istate->schunks);
> +}
>
> This doesn't involve any locking, which I think will happen to work
> with the current usage pattern but doesn't seem very robust in
> general.  I think you either need the refcounts to be protected by a
> spinlock, or maybe better, use pg_atomic_uint32 for them.  You want
> something like if (pg_atomic_sub_fetch_u32(, 1) == 0) {
> dsa_free(...) }
>
> Otherwise, there's no guarantee it will get freed exactly once.

Fixed


On Tue, Mar 7, 2017 at 9:34 PM, Robert Haas  wrote:
> You've still got this:
>
> +   if (DsaPointerIsValid(node->pstate->tbmiterator))
> +   tbm_free_shared_area(dsa, node->pstate->tbmiterator);
> +
> +   if (DsaPointerIsValid(node->pstate->prefetch_iterator))
> +   dsa_free(dsa, node->pstate->prefetch_iterator);
>
> I'm trying to get to a point where both calls use
> tbm_free_shared_area() - i.e. no peeking behind the abstraction layer.

Fixed

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


0001-tidbitmap-support-shared-v8.patch
Description: Binary data


0003-parallel-bitmap-heapscan-v8.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] Reporting planning time with EXPLAIN

2017-03-07 Thread Ashutosh Bapat

On Tue, Mar 7, 2017 at 9:23 PM, Stephen Frost  wrote:
> Ashutosh,
>
> * Ashutosh Bapat (ashutosh.ba...@enterprisedb.com) wrote:
>> Here are patches for follwing
>> 1. pg_explain_plan_time_v3 adds SUMMARY option which behaves as:
>> SUMMARY when ON prints planning time. With ANALYZE ON, it also prints
>> execution time. When user explicitly uses SUMMARY OFF, it does not
>> print planning and execution time (even when ANALYZE is ON). By
>> default SUMMARY is ON when ANALYZE is ON, otherwise SUMMARY defaults
>> to OFF. Attached explain_summary_examples.out.txt shows examples.
>>
>> 2. explain_exec_timing adds support to print planning time in EXPLAIN
>> EXECUTE output with SUMMARY option. In this case, planning time
>> includes time required to fetch the plan from cache and plan the query
>> if necessary (i.e. after invalidation or the first time it's
>> executed.) E.g.
>
> I'm going through these with an eye towards committing them soon.  I've
> already adjusted some of the documentation and comments per our earlier
> discussion

Thanks a lot.

> but I'm now reviewing the changes to ExplainExecuteQuery()
> and trying to understand the reasoning for not including the
> EvaluateParams() call in the planning time.  Not including that feels to
> me like we're ending up leaving something out of the overall timing
> picture, which doesn't seem right.
>
> If we do include that, then planning time+execution time will equal the
> overall query time and that feels like the right approach to use here.
> Otherwise the overall query time is "planning time+execution
> time+something else that we don't tell you about" which doesn't seem
> good to me.

Thanks for pointing that out. I didn't include parameter evaluation
time earlier, since it's not strictly planning time. But I think it's
important to include the parameter evaluation since different set of
parameters may cause planner to create a customized plan. So it looks
like something we should include in the planning time. I have updated
the patch to do so. I have also rebased the patches on top of current
head, resolving a conflict. The new patches have slightly different
names than previous ones, since I am now using git format-patch to
create those.

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


0001-Support-SUMMARY-option-in-EXPLAIN-output.patch
Description: Binary data


0002-Print-planning-time-in-EXPLAIN-EXECUTE.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] dropping partitioned tables without CASCADE

2017-03-07 Thread Amit Langote

Hi Ashutosh,

On 2017/03/06 18:19, Ashutosh Bapat wrote:
> On Mon, Mar 6, 2017 at 11:12 AM, Ashutosh Bapat wrote:
>> On Mon, Mar 6, 2017 at 11:05 AM, Simon Riggs  wrote:
>>> "has partitions" is not part of the DDL, whereas "FOR VALUES FROM (0)
>>> TO (100)" is. So ISTM sensible to differentiate between DDL and
>>> non-ddl using upper and lower case.
>>>
>>
>> Make sense. Will try to cook up a patch soon.
> 
> here's the patch. I have added a testcase in insert.sql to test \d+
> output for a partitioned table which has partitions which are further
> partitioned and also some partitions which are not partitioned
> themselves. I have also refactored a statement few lines above,
> replacing an if condition with ? : operator similar to code few lines
> below.

Thanks for cooking up the patch.  Looks good and works as expected.
Regression tests pass.

About the other statement you changed, I just realized that we should
perhaps do one more thing.  Show the Number of partitions, even if it's 0.
 In case of inheritance, the parent table stands on its own when there are
no child tables, but a partitioned table doesn't in the same sense.  I
tried to implement that in attached patch 0002.  Example below:

create table p (a int) partition by list (a);
\d p

Partition key: LIST (a)
Number of partitions: 0

\d+ p

Partition key: LIST (a)
Number of partitions: 0

create table p1 partition of p for values in (1);
\d p

Partition key: LIST (a)
Number of partitions: 1 (Use \d+ to list them.)

\d+ p

Partition key: LIST (a)
Partitions: p1 FOR VALUES IN (1)

0001 is your original patch.

Thanks,
Amit
>From f972a1231e66b84de0b1922e6a6fd96ea661ae20 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 8 Mar 2017 11:21:55 +0900
Subject: [PATCH 1/2] Ashutosh's patch to improve \dt+ of partitioned tables

---
 src/bin/psql/describe.c  | 26 +-
 src/test/regress/expected/insert.out | 15 +++
 src/test/regress/sql/insert.sql  |  4 
 3 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index e2e4cbcc08..f8617cf328 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2684,7 +2684,7 @@ describeOneTableDetails(const char *schemaname,
 		/* print child tables (with additional info if partitions) */
 		if (pset.sversion >= 10)
 			printfPQExpBuffer(,
-	"SELECT c.oid::pg_catalog.regclass, pg_get_expr(c.relpartbound, c.oid)"
+	"SELECT c.oid::pg_catalog.regclass, pg_get_expr(c.relpartbound, c.oid), c.relkind"
 	" FROM pg_catalog.pg_class c, pg_catalog.pg_inherits i"
 	" WHERE c.oid=i.inhrelid AND"
 	" i.inhparent = '%s' AND"
@@ -2709,13 +2709,12 @@ describeOneTableDetails(const char *schemaname,
 
 		if (!verbose)
 		{
+			const char *ct = tableinfo.relkind != 'P' ? _("child tables") : _("partitions");
+
 			/* print the number of child tables, if any */
 			if (tuples > 0)
 			{
-if (tableinfo.relkind != 'P')
-	printfPQExpBuffer(, _("Number of child tables: %d (Use \\d+ to list them.)"), tuples);
-else
-	printfPQExpBuffer(, _("Number of partitions: %d (Use \\d+ to list them.)"), tuples);
+printfPQExpBuffer(, _("Number of %s: %d (Use \\d+ to list them.)"), ct, tuples);
 printTableAddFooter(, buf.data);
 			}
 		}
@@ -2738,12 +2737,21 @@ describeOneTableDetails(const char *schemaname,
 }
 else
 {
+	char *partitioned_note;
+
+	if (*PQgetvalue(result, i, 2) == 'P')
+		partitioned_note = " has partitions";
+	else
+		partitioned_note = "";
+
 	if (i == 0)
-		printfPQExpBuffer(, "%s: %s %s",
-		  ct, PQgetvalue(result, i, 0), PQgetvalue(result, i, 1));
+		printfPQExpBuffer(, "%s: %s %s%s",
+		  ct, PQgetvalue(result, i, 0), PQgetvalue(result, i, 1),
+		  partitioned_note);
 	else
-		printfPQExpBuffer(, "%*s  %s %s",
-		  ctw, "", PQgetvalue(result, i, 0), PQgetvalue(result, i, 1));
+		printfPQExpBuffer(, "%*s  %s %s%s",
+		  ctw, "", PQgetvalue(result, i, 0), PQgetvalue(result, i, 1),
+		  partitioned_note);
 }
 if (i < tuples - 1)
 	appendPQExpBufferChar(, ',');
diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out
index 116854e142..c8ff863882 100644
--- a/src/test/regress/expected/insert.out
+++ b/src/test/regress/expected/insert.out
@@ -313,6 +313,21 @@ select tableoid::regclass::text, a, min(b) as min_b, max(b) as max_b from list_p
  part_null || 1 | 1
 (9 rows)
 
+-- test \d+ output on a table which has both partitioned and unpartitioned
+-- partitions
+\d+ list_parted
+Table "public.list_parted"
+ Column |  Type   | Collation | Nullable | Default | Storage  | Stats target | Description 
++-+---+--+-+--+--+-
+ a  | text|   |  |

Re: [HACKERS] CREATE/ALTER ROLE PASSWORD ('value' USING 'method')

2017-03-07 Thread Rushabh Lathia

On Wed, Mar 8, 2017 at 9:59 AM, Tom Lane  wrote:

> Michael Paquier  writes:
> > here is a separate thread dedicated to the following extension for
> > CREATE/ALTER ROLE: PASSWORD ('value' USING 'method').
>
> The parentheses seem weird ... do we really need those?
>

+1

I had quick glance to patch and it looks great.

One quick comments:

+| PASSWORD '(' Sconst USING Sconst ')'
+{
+$$ = makeDefElem("methodPassword",
+ (Node *)list_make2(makeString($3),
+makeString($5)),
+ @1);
+}

methodPassword looks bit weird, can we change it to passwordMethod
or pwdEncryptMethod ?


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



-- 
Rushabh Lathia

Re: [HACKERS] some dblink refactoring

2017-03-07 Thread Tsunakawa, Takayuki

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tsunakawa,
> Takayuki
> How about applying the attached small patch for another refactoring?  This
> merely changes makeStringInfo() to initStringInfo() at two sites just other
> places in the same file.  makeStringInfo() on the function local variables
> leaves memory for StringInfoData allocated unnecessarily (which may be
> automatically reclaimed some time after.)

Sorry, I forgot to attach the file.

Regards
Takayuki Tsunakawa




dblink_strinfo_refactor.patch
Description: dblink_strinfo_refactor.patch

-- 
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] some dblink refactoring

2017-03-07 Thread Tsunakawa, Takayuki

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Peter Eisentraut
> Here is a patch to refactor some macro hell in dblink.
> 
> This patch was discussed in the background sessions thread as a prerequisite
> for some work there, but I figure I'll make a separate thread for it to
> give everyone interested in dblink a chance to respond separate from the
> other thread.

I changed the status to ready for committer.  The patch applied cleanly, passed 
the regression test (make installcheck in contrib/dblink/), and the code looks 
perfect.

How about applying the attached small patch for another refactoring?  This 
merely changes makeStringInfo() to initStringInfo() at two sites just other 
places in the same file.  makeStringInfo() on the function local variables 
leaves memory for StringInfoData allocated unnecessarily (which may be 
automatically reclaimed some time after.)

Regards
Takayuki Tsunakawa




-- 
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] CREATE/ALTER ROLE PASSWORD ('value' USING 'method')

2017-03-07 Thread Michael Paquier

On Wed, Mar 8, 2017 at 1:29 PM, Tom Lane  wrote:
> Michael Paquier  writes:
>> here is a separate thread dedicated to the following extension for
>> CREATE/ALTER ROLE: PASSWORD ('value' USING 'method').
>
> The parentheses seem weird ... do we really need those?

A grammar without parenthesis works as well, and I am open to suggestions.
-- 
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] CREATE/ALTER ROLE PASSWORD ('value' USING 'method')

2017-03-07 Thread Tom Lane

Michael Paquier  writes:
> here is a separate thread dedicated to the following extension for
> CREATE/ALTER ROLE: PASSWORD ('value' USING 'method').

The parentheses seem weird ... do we really need those?

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] Need a builtin way to run all tests faster manner

2017-03-07 Thread Andres Freund

On 2017-03-07 20:58:35 +0800, Simon Riggs wrote:
> On 7 March 2017 at 20:36, Alvaro Herrera  wrote:
> 
> > FWIW, +1 on improving matters here.
> 
> +1 also.
> 
> I don't see what's wrong with relying on buildfarm though; testing is
> exactly what its there for.
> 
> If we had a two-stage process, where committers can issue "trial
> commits" as a way of seeing if the build farm likes things. If they
> do, we can push to the main repo.

Personally that's not addressing my main concern, which is that the
latency of getting done with some patch/topic takes a long while. If I
have to wait for the buildfarm to check some preliminary patch, I still
have to afterwards work on pushing it to master.  And very likely my
local check would finish a lot faster than a bunch of buildfarm animals
- I have after all a plenty powerfull machine, lots of cores, fast ssd,
lots of memory, ...

So I really want faster end-to-end test, not less cpu time spent on my
own machine.

- 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] foreign partition DDL regression tests

2017-03-07 Thread Ashutosh Bapat

On Wed, Mar 8, 2017 at 7:36 AM, Robert Haas  wrote:
> On Tue, Mar 7, 2017 at 7:14 AM, Ashutosh Bapat
>  wrote:
>> Hi Amit,
>> Thanks for adding testcases. Overall the testcases look good.
>>
>> The testcase is using ALTER TABLE to modify foreign table schema.
>> Though this works, I think a better option is to use ALTER FOREIGN
>> TABLE.
>>
>> Something not related to this patch but
>> -- no attach partition validation occurs for foreign tables
>> ALTER TABLE pt2 ATTACH PARTITION pt2_1 FOR VALUES IN (1);
>>
>> I am wondering whether we can improve this. For local tables, if a
>> valid constraint equivalent to the partitioning condition is not
>> present on the table being attached, it scans the data to verify
>> partition conditions. But for a foreign table, we don't want to do
>> that since the constraint is not guaranteed to be valid after the
>> initial check. For a normal foreign table a user can set a constraint
>> if s/he knows that that constraint will be honoured on the foreign
>> server. Thus instead of saying that we do not validate data, we can
>> refuse to attach a partition if corresponding check constraint is
>> absent on the foreign table being attached. A user will then be forced
>> to add that constraint if s/he is sure that the constraint will be
>> obeyed on the foreign server.
>
> I agree that we could do that, but what value would it have?  It just
> forces the user to spend two SQL commands doing what could otherwise
> be done in one.

I don't think it's going to be two commands always. A user who wants
to attach a foreign table as a partition, "knows" that the data on the
foreign server honours the partitioning bounds. If s/he knows that
probably he added the constraint on the foreign table, so that planner
could make use of it. Remember this is an existing foreign table. If
s/he is not aware that the data on the foreign server doesn't honour
partition bounds, adding that as a partition would be a problem. I
think, this step gives the user a chance to make a conscious decision.

> And the first command might not be that obvious.

A hint with error reported would help. In fact, the hint might as well
say "add constraint if you are sure that the foreign data honours
partition bound specification" or something like that. I noticed that
the documentation at
https://www.postgresql.org/docs/devel/static/sql-altertable.html for
ATTACH PARTITION does not have anything about foreign tables. May be
we should add whatever is the current status.

>  I
> think we should leave well enough alone.

At least we need to update the documentation.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] SQL/JSON in PostgreSQL

2017-03-07 Thread David Steele


On 3/7/17 11:38 AM, Andres Freund wrote:

<...>


We have a plenty of time and we dedicate one full-time developer for
this project.


How about having that, and perhaps others, developer participate in
reviewing patches and getting to the bottom of the commitfest?  Should
we end up being done early, we can look at this patch...  There's not
been review activity corresponding to the amount of submissions from
pgpro...


This patch has been moved to CF 2017-07.

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


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

Re: [HACKERS] Bizarre choice of case for RELKIND_PARTITIONED_TABLE

2017-03-07 Thread Amit Langote

On 2017/03/08 2:55, Tom Lane wrote:
> Is there a good reason why RELKIND_PARTITIONED_TABLE is 'P' not 'p'?
> It looks rather out of place considering that seven of the eight
> pre-existing relkind codes are lower case.  (And no, I don't especially
> approve of RELKIND_SEQUENCE being 'S' either, but it's far too late to
> change that.)  Also, in typical low-res monospaced fonts, there's nearly
> no difference except vertical alignment between P and p, meaning that in
> something like
> 
> regression=# select distinct relkind from pg_class;
>  relkind 
> -
>  r
>  t
>  P
>  v
>  m
>  i
>  S
>  c
> (8 rows)
> 
> you have to look rather closely even to notice that what you're seeing
> isn't in the case you might expect.
> 
> I think we should change this while we still can.

I remember that one of the earliest versions of the patch I submitted had
two new relkinds: 'P' for partitioned tables and 'p' for leaf partitions.
The latter was dropped subsequently and I never thought of using 'p'
instead of 'P' for partitioned tables.

Attached patch that implements this change.

Thanks,
Amit
>From b356a5e61aedc9fcf4edd99a095b06c5c8e57a86 Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 8 Mar 2017 12:07:56 +0900
Subject: [PATCH] Change RELKIND_PARTITIONED_TABLE to 'p'

---
 src/backend/catalog/information_schema.sql | 40 +++---
 src/backend/catalog/system_views.sql   |  4 +--
 src/bin/pg_dump/pg_dump.c  |  2 +-
 src/bin/psql/describe.c| 38 ++--
 src/bin/psql/tab-complete.c|  8 +++---
 src/include/catalog/pg_class.h |  2 +-
 src/test/regress/expected/create_table.out |  2 +-
 src/test/regress/expected/rules.out|  4 +--
 src/test/regress/expected/sanity_check.out |  2 +-
 src/test/regress/sql/sanity_check.sql  |  2 +-
 10 files changed, 52 insertions(+), 52 deletions(-)

diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
index 51795cd6de..0011e06494 100644
--- a/src/backend/catalog/information_schema.sql
+++ b/src/backend/catalog/information_schema.sql
@@ -453,7 +453,7 @@ CREATE VIEW check_constraints AS
   AND a.attnum > 0
   AND NOT a.attisdropped
   AND a.attnotnull
-  AND r.relkind IN ('r', 'P')
+  AND r.relkind IN ('r', 'p')
   AND pg_has_role(r.relowner, 'USAGE');
 
 GRANT SELECT ON check_constraints TO PUBLIC;
@@ -525,7 +525,7 @@ CREATE VIEW column_domain_usage AS
   AND a.attrelid = c.oid
   AND a.atttypid = t.oid
   AND t.typtype = 'd'
-  AND c.relkind IN ('r', 'v', 'f', 'P')
+  AND c.relkind IN ('r', 'v', 'f', 'p')
   AND a.attnum > 0
   AND NOT a.attisdropped
   AND pg_has_role(t.typowner, 'USAGE');
@@ -564,7 +564,7 @@ CREATE VIEW column_privileges AS
   pr_c.relowner
FROM (SELECT oid, relname, relnamespace, relowner, (aclexplode(coalesce(relacl, acldefault('r', relowner.*
  FROM pg_class
- WHERE relkind IN ('r', 'v', 'f', 'P')
+ WHERE relkind IN ('r', 'v', 'f', 'p')
 ) pr_c (oid, relname, relnamespace, relowner, grantor, grantee, prtype, grantable),
 pg_attribute a
WHERE a.attrelid = pr_c.oid
@@ -586,7 +586,7 @@ CREATE VIEW column_privileges AS
 ) pr_a (attrelid, attname, grantor, grantee, prtype, grantable),
 pg_class c
WHERE pr_a.attrelid = c.oid
- AND relkind IN ('r', 'v', 'f', 'P')
+ AND relkind IN ('r', 'v', 'f', 'p')
  ) x,
  pg_namespace nc,
  pg_authid u_grantor,
@@ -629,7 +629,7 @@ CREATE VIEW column_udt_usage AS
 WHERE a.attrelid = c.oid
   AND a.atttypid = t.oid
   AND nc.oid = c.relnamespace
-  AND a.attnum > 0 AND NOT a.attisdropped AND c.relkind in ('r', 'v', 'f', 'P')
+  AND a.attnum > 0 AND NOT a.attisdropped AND c.relkind in ('r', 'v', 'f', 'p')
   AND pg_has_role(coalesce(bt.typowner, t.typowner), 'USAGE');
 
 GRANT SELECT ON column_udt_usage TO PUBLIC;
@@ -738,7 +738,7 @@ CREATE VIEW columns AS
CAST('NEVER' AS character_data) AS is_generated,
CAST(null AS character_data) AS generation_expression,
 
-   CAST(CASE WHEN c.relkind IN ('r', 'P') OR
+   CAST(CASE WHEN c.relkind IN ('r', 'p') OR
   (c.relkind IN ('v', 'f') AND
pg_column_is_updatable(c.oid, a.attnum, false))
 THEN 'YES' ELSE 'NO' END AS yes_or_no) AS is_updatable
@@ -753,7 +753,7 @@ CREATE VIEW columns AS
 
 WHERE (NOT pg_is_other_temp_schema(nc.oid))
 
-  AND a.attnum > 0 AND NOT a.attisdropped AND c.relkind in ('r', 'v', 'f', 'P')
+  AND a.attnum > 0 AND NOT a.attisdropped AND c.relkind in ('r', 'v', 'f', 'p')
 
   AND

Re: [HACKERS] Need a builtin way to run all tests faster manner

2017-03-07 Thread Magnus Hagander

On Tue, Mar 7, 2017 at 7:24 AM, Andrew Dunstan <
andrew.duns...@2ndquadrant.com> wrote:

>
>
> On 03/07/2017 07:58 AM, Simon Riggs wrote:
> > On 7 March 2017 at 20:36, Alvaro Herrera 
> wrote:
> >
> >> FWIW, +1 on improving matters here.
> > +1 also.
> >
> > I don't see what's wrong with relying on buildfarm though; testing is
> > exactly what its there for.
> >
> > If we had a two-stage process, where committers can issue "trial
> > commits" as a way of seeing if the build farm likes things. If they
> > do, we can push to the main repo.
> >
>
>
> I'm happy to work on this.  Not quite sure how it would work, but I'm
> open to any suggestions.
>

Assuming the intention is a service for *committers only*, I suggest
setting up a separate (closed) git repository that committers can push to
and a separate set of BF animals could work from. We could just have it do
all branches in it, no need to filter that way as long as we keep it a
separate repo.

There have also on and off been discussions about building arbitrary
patches as they are sent to the mailinglists. Doing that without any
committer (or other trusted party) as a filter is a completely different
challenge of course, given that it basically amounts to downloading and
running random code off the internet.

But doing just the first would make it a lot easier, and probably still be
of good value.

An in-between could be to hook something off the CF app, but one important
question is how important covering many platforms is. Since we already have
good functionality for doing that in the buildfarm, it makes sense to
utilize that if we can.

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

Re: [HACKERS] ALTER PUBLICATION and segmentation fault

2017-03-07 Thread Peter Eisentraut

On 3/7/17 11:56, Masahiko Sawada wrote:
> This issue happen even ALTER SUBSCRIPTION. I guess the main cause is
> that acl_kind of pg_publication and pg_subscription of ObjectProperty
> array are not correct. These values should be set ACL_KIND_PUBLICATION
> and ACL_KIND_SUBSCRIPTION instead of -1. Attached small patch fixes
> this issue and adds regression test.

committed, thanks

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


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

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

2017-03-07 Thread Haribabu Kommi

On Wed, Feb 1, 2017 at 3:13 PM, Michael Paquier 
wrote:

> On Fri, Jan 27, 2017 at 10:26 AM, Haribabu Kommi
>  wrote:
> > Thanks for the review.
> > Let's wait for the committer's opinion.
>
> I have moved this patch to CF 2017-03 to wait for this to happen.
>

Attached a rebased patch to resolve the OID conflict.

Regards,
Hari Babu
Fujitsu Australia


pg_stat_sql_row_mode_5.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] SCRAM authentication, take three

2017-03-07 Thread Magnus Hagander

On Tue, Mar 7, 2017 at 4:36 AM, Heikki Linnakangas  wrote:

> On 03/02/2017 08:50 AM, Michael Paquier wrote:
>
>> Attached is a new patch set. I have combined SASLprep with the rest
>> and fixed some conflicts. At the same time when going through NFKC
>> this morning I have noticed that the implementation was doing the
>> canonical decomposition and reordered the characters using the
>> combining classes, but the string recomposition was still missing.
>> This is addressed in this patch set, and well as on my dev tree:
>> https://github.com/michaelpq/postgres/tree/scram
>>
>
> I've now committed the bulk of these patches. Many thanks to everyone
> involved, and especially you, Michael, for your persistence!
>

+1!


Currently, the AuthenticationSASL protocol message specifies the mechanism
> that the client must use, but as future-proofing, it'd probably be best to
> redefine that to be a list of mechanisms, and let the client choose among
> those. That's not a show-stopper, but would be good to get that right in
> version 10, so that clients can prepare for that, even if we only support
> one mechanism ATM.
>

+1, and let's make sure we get it into 10. We don't want to be stuck with
something without flexibility -- then we're going to have to do the whole
"is it time yet" dance again. It would be especially bad since the
underlying protocol is pluggable.

This seems like an obvious place, but are there any other places where we
should also consider something like that for compatibility?

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

Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall

2017-03-07 Thread Haribabu Kommi

On Wed, Mar 1, 2017 at 12:59 PM, Haribabu Kommi 
wrote:

>
> Patch attached. Still some more docs needs to be added.
>

Updated patch attached to resolve the conflicts with following commit.

commit 9a83d56b38c870ce47b7651385ff2add583bf136
Author: Simon Riggs 
Date:   Tue Mar 7 22:00:54 2017 +0800

Allow pg_dumpall to dump roles w/o user passwords

Add new option --no-role-passwords which dumps roles without passwords.
Since we don’t need passwords, we choose to use pg_roles in preference
to pg_authid since access may be restricted for security reasons in
some configrations.

Robins Tharakan and Simon Riggs

Regards,
Hari Babu
Fujitsu Australia

pg_dump_changes_2.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] Protect syscache from bloating with negative cache entries

2017-03-07 Thread David Steele


On 3/3/17 4:54 PM, David Steele wrote:


On 2/1/17 1:25 AM, Kyotaro HORIGUCHI wrote:

Hello, thank you for moving this to the next CF.

At Wed, 1 Feb 2017 13:09:51 +0900, Michael Paquier  wrote 
in 

On Tue, Jan 24, 2017 at 4:58 PM, Kyotaro HORIGUCHI
 wrote:

Six new syscaches in 665d1fa was conflicted and 3-way merge
worked correctly. The new syscaches don't seem to be targets of
this patch.

To be honest, I am not completely sure what to think about this patch.
Moved to next CF as there is a new version, and no new reviews to make
the discussion perhaps move on.

I'm thinking the following is the status of this topic.

- The patch stll is not getting conflicted.

- This is not a hollistic measure for memory leak but surely
   saves some existing cases.

- Shared catcache is another discussion (and won't really
   proposed in a short time due to the issue on locking.)

- As I mentioned, a patch that caps the number of negative
   entries is avaiable (in first-created - first-delete manner)
   but it is having a loose end of how to determine the
   limitation.

While preventing bloat in the syscache is a worthwhile goal, it appears
there are a number of loose ends here and a new patch has not been provided.

It's a pretty major change so I recommend moving this patch to the
2017-07 CF.


Not hearing any opinions pro or con, I'm moving this patch to the 
2017-07 CF.


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



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

Re: [HACKERS] REINDEX CONCURRENTLY 2.0

2017-03-07 Thread Andres Freund

On 2017-03-07 21:48:23 -0500, Robert Haas wrote:
> On Sun, Mar 5, 2017 at 7:13 PM, Andreas Karlsson  wrote:
> > And I would argue that his feature is useful for quite many, based on my
> > experience running a semi-large database. Index bloat happens and without
> > REINDEX CONCURRENTLY it can be really annoying to solve, especially for
> > primary keys. Certainly more people have problems with index bloat than the
> > number of people who store index oids in their database.
> 
> Yeah, but that's not the only wart, I think.

I don't really see any other warts that don't correspond to CREATE/DROP
INDEX CONCURRENTLY.

> For example, I believe (haven't looked at this patch series in a
> while) that the patch takes a lock and later escalates the lock level.

It shouldn't* - that was required precisely because we had to switch the
relfilenodes when the oid stayed the same.  Otherwise in-progress index
lookups could end up using the wrong relfilenodes and/or switch in the
middle of a lookup.

* excepting the exclusive lock DROP INDEX CONCURRENTLY style dropping
  uses after marking the index as dead - but that shouldn't be much of a
  concern?

> Also, if by any chance you think (or use any software that thinks)
> that OIDs for system objects are a stable identifier, this will be the
> first case where that ceases to be true.

Can you come up with an halfway realistic scenario why an index oid, not
a table, constraint, sequence oid, would be relied upon?

> If the system is shut down or crashes or the session is killed, you'll
> be left with stray objects with names that you've never typed into the
> system.

Given how relatively few complaints we have about CIC's possibility of
ending up with invalid indexes - not that there are none - and it's
widespread usage, I'm not too concerned about 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] Skip all-visible pages during second HeapScan of CIC

2017-03-07 Thread Andres Freund

On 2017-03-07 21:38:40 -0500, Robert Haas wrote:
> > I wonder however, if careful snapshot managment couldn't solve this as
> > well.  I have *not* thought a lot about this, but afaics we can easily
> > prevent all-visible from being set in cases it'd bother us by having an
> > "appropriate" xmin / registered snapshot.
> 
> Yeah, but that's a tax on the whole system.

I'm not sure I can buy that argument. CIC *already* holds a snapshot
during each of the two scans. By reducing the amount of time that's held
in the second scan, the systemwide impact is reduced, because it's held
for a shorter amount of time.  We need to hold a slightly "more
aggressive" snapshot, that's true, but if you have high xid throughput
those effects should roughly balance each other.

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

[HACKERS] Implementation of SASLprep for SCRAM-SHA-256

2017-03-07 Thread Michael Paquier

Hi all,

Here is another thread for an issue related to SCRAM
(https://www.postgresql.org/message-id/243d8c11-6149-a4bb-0909-136992f74...@iki.fi),
that can be discussed independently: SASLprep.

RFC5802 (https://tools.ietf.org/html/rfc5802) regarding the
implementation of SCRAM, needs to have passwords normalized as per
RFC4013 (https://tools.ietf.org/html/rfc4013) using SASLprep, which is
actually NFKC. I have previously described what happens in this case
here:
https://www.postgresql.org/message-id/CAB7nPqScgwh6Eu4=c-0l7-cufzru5rultsdpmoowiz1yfvg...@mail.gmail.com
This way, we can be sure that two UTf-8 strings are considered as
equivalent in a SASL exchange, in our case we care about the password
string (we should care about the username as well). Without SASLprep,
our implementation of SCRAM may fail with other third-part tools if
passwords are not made only of ASCII characters. And that sucks.

To put in short words, NFKC is made of two steps: a canonical
decomposition of the UTF-8 string (decomposition of the string
following by a reordering using the class of each character), followed
by its recomposition.

Postgres does not have any logic on the frontend-side related to
encoding, still it is possible with some of the APIs of wchar.c to
check if a string provided is valid UTF-8 or not. If the string is not
valid UTF-8, we should just use the string as-is in the exchange,
which leads to undefined behaviors anyway as SASL needs to do things
with UTF-8.

I have also implementation a Postgres extension able to do this
operation, which has been useful for testing.
https://github.com/michaelpq/pg_plugins/tree/master/pg_sasl_prepare

Attached is a patch implementing that, the conversion tables are built
from UnicodeData.txt to be minimal in size. I have added an Open Item
on the wiki for this problem as well.

Thanks,
--
Michael

0001-Implement-SASLprep-aka-NFKC-for-SCRAM-authentication.patch.gz
Description: GNU Zip compressed data

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

1 2 >

1 - 100 of 155 matches

Mail list logo