date:20160303

2016-03-04 3:13 GMT+01:00 Corey Huinker :

> On Sat, Feb 27, 2016 at 2:26 AM, Pavel Stehule 
> wrote:
>
>> Hi
>>
>> 2015-08-06 10:37 GMT+02:00 Pavel Stehule :
>>
>>> Hi,
>>>
>>> Psql based implementation needs new infrastructure (more than few lines)
>>>
>>> Missing:
>>>
>>> * binary mode support
>>> * parametrized query support,
>>>
>>> I am not against, but both points I proposed, and both was rejected.
>>>
>>> So why dont use current infrastructure? Raw copy is trivial patch.
>>>
>>
>> I was asked by Daniel Verite about reopening this patch in opened
>> commitfest.
>>
>> I am sending rebased patch
>>
>> Regards
>>
>> Pavel
>>
>>
>>
> Since this patch does something I need for my own work, I've signed up as
> a reviewer.
>
> From a design standpoint, I feel that COPY is the preferred means of
> dealing with data from sources too transient to justify setting up a
> foreign data wrapper, and too simple to justify writing application code.
> So, for me, RAW is the right solution, or at least *a* right solution.
>

my opinion is same - there all necessary infrastructure is ready and when
we work with IO, then we use COPY natively. I hope so main use case (export
bytea) is solved, but there are a possibility to enhance this command by
COPY options - what is, I am thinking, a advantage of this way.


>
> My first pass of reading the code changes and the regression tests is
> complete, and I found the changes to be clear and fairly straightforward.
> This shouldn't surprise anyone, as the previous reviewers had only minor
> quibbles with the code. So far, so good.
>
> The regression tests seem to adequately cover all new functionality,
> though I wonder if we should add some cases that highlight situations where
> BINARY mode is insufficient.
>
> Before I give my approval, I want to read it again more closely to make
> sure that no cases were skipped with regard to the  (binary || raw) and
> (binary || !raw) tests. Also, I want to use it on some of my problematic
> files. Maybe I'll find a good edge case. Probably not.
>
> I hope to find time for those things in the next few days.
>

Thank you very much

Regards

Pavel

Re: [HACKERS] proposal: psql autocomplete for casting

2016-03-04 5:29 GMT+01:00 Kyotaro HORIGUCHI :

> At Thu, 3 Mar 2016 12:15:13 +0100, Pavel Stehule 
> wrote in <
> cafj8prdb2ppeslxnwndxmhvts9vl0nmeanudv_hps9wzywx...@mail.gmail.com>
> pavel.stehule> 2016-03-03 12:06 GMT+01:00 Kyotaro HORIGUCHI <
> > the requirement of space before is not good :( - It should be any
> different
> > than operator chars. Not only space.
> >
> > all other is perfect :)
>
> Yeah, I fortunately agree with you:p
>
> But the things are not so simple. readline can handle single
> prefix characters but cannot not handle prefix strings.
>
> The new diff adds ':' to WORD_BREAKS and adjusts related codes.
> As far as I can see, colons are used only for variable prefix,
> type casts and named parameter assignments in function
> calls. This covers the first two and the last wouldn't be a
> matter of tab-completion. This works as the following.
>
> > =# select now()::t
> > text trigger
> > tid  tsm_handler
> > ...
> > tintervaltxid_snapshot
> > =# select now()::te
> > =# select now()::text
>
> As an inevitable side effect, this makes completion for ": :"
> with types (which results in an syntax error) but I believe it
> won't be a matter.
>
> I'm quite unpleasant that the additional conditional expressions
> use bare previous_words but found no good solution.
>
> >   else if (previous_words_count >= 2 &&
> >previous_words[1][strlen(previous_words[1])-1] == ':' &&
> >TailMatches1(":"))
>
> It is good if we could limit the candidate types by knowing the
> operand type but it seems a bit complicated.
>
> Some familiar type names won't come as candidates. They are
> "int", "float", "decimal", "dec", which are known only to gram.y
> but it does't seem to matter, too.
>
> Thoughts? Opinions?
>

It works. I found only one issue with multi word named types

I have not any complete option for  select 1::timestamp with

maybe it is another limit of readline

Regards

Pavel


> regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>

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

On Fri, Mar 4, 2016 at 9:36 AM, Michael Paquier
 wrote:
> Yeah, that's my first impression as well. We should not need any APIs
> changes and the changes would be limited to if extra blocks with
> _MSC_VER, if that would occur then I definitely agree that patching
> only HEAD is the way to go. I'll look at that today and the next
> couple of days, let's see what I can get out of it...

So, I have finally been able to set up my environment correctly, and I
am at a stage where I can compile Postgres with VS 2015 thanks to the
patch attached that extends src/tools/msvc to do so. Unsurprisingly,
the first failures detected are related to locales :)

I still need to dig into that in more details. For the time being the
patch attached is useful IMO to plug in VS 2015 with the existing
infrastructure. So if anybody has a Windows environment, feel free to
play with it and dig into those problems. I'll update this thread once
I have a more advanced status.
Regards,
-- 
Michael
From 2e0a84eab407baacbc7372a08c6b533b97e2736b Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Fri, 4 Mar 2016 15:25:55 +0900
Subject: [PATCH] Add support for VS 2015

---
 doc/src/sgml/install-windows.sgml |  8 
 src/tools/msvc/MSBuildProject.pm  | 23 +++
 src/tools/msvc/Solution.pm| 26 ++
 src/tools/msvc/VSObjectFactory.pm | 12 ++--
 4 files changed, 63 insertions(+), 6 deletions(-)

diff --git a/doc/src/sgml/install-windows.sgml b/doc/src/sgml/install-windows.sgml
index f08cca7..148180b 100644
--- a/doc/src/sgml/install-windows.sgml
+++ b/doc/src/sgml/install-windows.sgml
@@ -19,10 +19,10 @@
  
   There are several different ways of building PostgreSQL on
   Windows. The simplest way to build with
-  Microsoft tools is to install Visual Studio Express 2013
+  Microsoft tools is to install Visual Studio Express 2015
   for Windows Desktop and use the included
   compiler. It is also possible to build with the full
-  Microsoft Visual C++ 2005 to 2013.
+  Microsoft Visual C++ 2005 to 2015.
   In some cases that requires the installation of the
   Windows SDK in addition to the compiler.
  
@@ -77,7 +77,7 @@
   Visual Studio Express or some versions of the
   Microsoft Windows SDK. If you do not already have a
   Visual Studio environment set up, the easiest
-  ways are to use the compilers from Visual Studio Express 2013
+  ways are to use the compilers from Visual Studio Express 2015
   for Windows Desktop or those in the Windows SDK
   7.1, which are both free downloads from Microsoft.
  
@@ -85,7 +85,7 @@
  
   PostgreSQL is known to support compilation using the compilers shipped with
   Visual Studio 2005 to
-  Visual Studio 2013 (including Express editions),
+  Visual Studio 2015 (including Express editions),
   as well as standalone Windows SDK releases 6.0 to 7.1.
   64-bit PostgreSQL builds are only supported with
   Microsoft Windows SDK version 6.0a to 7.1 or
diff --git a/src/tools/msvc/MSBuildProject.pm b/src/tools/msvc/MSBuildProject.pm
index 3d60b64..d7638b4 100644
--- a/src/tools/msvc/MSBuildProject.pm
+++ b/src/tools/msvc/MSBuildProject.pm
@@ -465,4 +465,27 @@ sub new
 	return $self;
 }
 
+package VC2015Project;
+
+#
+# Package that encapsulates a Visual C++ 2015 project file
+#
+
+use strict;
+use warnings;
+use base qw(VC2012Project);
+
+sub new
+{
+	my $classname = shift;
+	my $self  = $classname->SUPER::_new(@_);
+	bless($self, $classname);
+
+	$self->{vcver}   = '14.00';
+	$self->{PlatformToolset} = 'v140';
+	$self->{ToolsVersion}= '14.0';
+
+	return $self;
+}
+
 1;
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index ac116b7..e627a7b 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -778,6 +778,32 @@ sub new
 	return $self;
 }
 
+package VS2015Solution;
+
+#
+# Package that encapsulates a Visual Studio 2015 solution file
+#
+
+use Carp;
+use strict;
+use warnings;
+use base qw(Solution);
+
+sub new
+{
+	my $classname = shift;
+	my $self  = $classname->SUPER::_new(@_);
+	bless($self, $classname);
+
+	$self->{solutionFileVersion}= '14.00';
+	$self->{vcver}  = '14.00';
+	$self->{visualStudioName}   = 'Visual Studio 2015';
+	$self->{VisualStudioVersion}= '14.0.24730.2';
+	$self->{MinimumVisualStudioVersion} = '10.0.40219.1';
+
+	return $self;
+}
+
 sub GetAdditionalHeaders
 {
 	my ($self, $f) = @_;
diff --git a/src/tools/msvc/VSObjectFactory.pm b/src/tools/msvc/VSObjectFactory.pm
index fee4684..4190ada 100644
--- a/src/tools/msvc/VSObjectFactory.pm
+++ b/src/tools/msvc/VSObjectFactory.pm
@@ -49,6 +49,10 @@ sub CreateSolution
 	{
 		return new VS2013Solution(@_);
 	}
+	elsif ($visualStudioVersion eq '14.00')
+	{
+		return new VS2015Solution(@_);
+	}
 	else
 	{
 		croak "The requested Visual Studio version is not supported.";
@@ -84,6 +88,10 @@ sub CreateProject
 	{
 		return new

Re: [HACKERS] ExecGather() + nworkers

2016-03-03 Thread Haribabu Kommi

On Wed, Jan 13, 2016 at 7:19 PM, Amit Kapila  wrote:
> On Mon, Jan 11, 2016 at 9:16 AM, Amit Kapila 
> wrote:
>> On Mon, Jan 11, 2016 at 3:14 AM, Peter Geoghegan  wrote:
>> >
>> > On Sun, Jan 10, 2016 at 9:13 AM, Robert Haas 
>> > wrote:
>> > >> I'm not sure why the test for nworkers following the
>> > >> LaunchParallelWorkers() call doesn't look like this, though:
>> > >>
>> > >> /* Set up tuple queue readers to read the results. */
>> > >> if (pcxt->nworkers_launched > 0)
>> > >> {
>> > >> ...
>> > >> }
>> > >
>> > > Hmm, yeah, I guess it could do that.
>> >
>> > That would make it clearer as an example.
>> >
>> > >> But going to this additional trouble (detecting no workers launched
>> > >> on
>> > >> the basis of !nworkers_launched) suggests that simply testing
>> > >> nworkers_launched would be wrong, which AFAICT it isn't. Can't we
>> > >> just
>> > >> do that, and in so doing also totally remove the "for" loop shown
>> > >> here?
>> > >
>> > > I don't see how the for loop goes away.
>> >
>> > I meant that some code in the "for" loop goes away. Not all of it.
>> > Just the more obscure code. As I said, I'm mostly pointing this out
>> > out of concern for making it clearer as example code.
>> >
>>
>> Right, I can write a patch to do it in a way you are suggesting if you
>> are not planning to do it.
>>
>
> Changed the code such that nworkers_launched gets used wherever
> appropriate instead of nworkers.  This includes places other than
> pointed out above.

The changes of the patch are simple optimizations that are trivial.
I didn't find any problem regarding the changes. I think the same
optimization is required in "ExecParallelFinish" function also.

Regards,
Hari Babu
Fujitsu Australia


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

Re: [HACKERS] TAP / recovery-test fs-level backups, psql enhancements etc

On 4 March 2016 at 12:54, Michael Paquier  wrote:

> No objections from here as well for the feature itself. I am glad to
> see interest in extending the current infrastructure, and just
> wondering what kind of tests are going to show up.

The tests themselves really aren't that exciting. Create a slot, online
base-backup the node, start it in archive recovery, immediately fail over
to it or do some amount of writes first, do some more writes on it, then
read from the logical slot on the replica and prove that we can
successfully read the logical change change stream across the timeline
switch.

I'm now adding a module that goes a step further by exposing some functions
to SQL (test only, if you use it in production you're insane) to permit
creation of a logical slot on a replica and to advance that slot's
persistent data. It basically lets you advance the slot state on the
replica while it's in recovery so you can retain slot states copied with a
filesystem-level base backup and have the slots ready to use when you
promote the server.

I'm doing it to prove the concept of doing logical failover with an
extension and without full WAL-based failover slots in order to get the
timeline following for logical decoding patch committed separately to, and
prior to, failover slots. It's complicated enough that it needs a variety
of tests, but self-contained and non-intrusive enough to be a fairly easy
commit if it's shown to work well.

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

Re: [HACKERS] Patch: fix lock contention for HASHHDR.mutex

2016-03-03 Thread Amit Kapila

On Thu, Mar 3, 2016 at 1:50 PM, Aleksander Alekseev <
a.aleks...@postgrespro.ru> wrote:

> > Won't it always use the same freelist to remove and add the entry from
> > freelist as for both cases it will calculate the freelist_idx in same
> > way?
>
> No. If "our" freelist is empty when we try to remove an item from it we
> borrow item from another freelist.


I think the way patch works is if indicated freelist is empty, then it
tries to allocate new elements in that list and if it can't allocate, then
it tries to borrow from other freelist and in both cases the element to be
removed from freelist is considered to be the element of indicated freelist
(basically it always increment nentries[freelist_idx]).


> Then this borrowed item will be
> returned to "our" freelist instead of original. Without some sort of
> additional logic there is no way to figure out what freelist was
> original.
>
>
As the patch always operates on nentries[freelist_idx], so it seems to me
that in both cases (remove, add), the element is considered to be belonging
to same freelist.  Have you faced this problem of negative entries in any
of your test, if so then share the test, so that I can also understand the
scenario?



> Generally speaking negative nentries value is not something that
> couldn't be solved. But I would like to remind that in this context we
> are discussing a quick and dirty solution created for benchmark
> purposes in a first place.
>
>
I thought negative entries could be a problem for the patch as indicated by
Robert[1], but may be I am missing something.



> > You are not convinced, then lets leave it to committer unless
> > somebody else wants to try that suggestion.
>
> Agree. Frankly I'm tired of rewriting this patch over and over and over
> again. So I would like to avoid rewriting it once again unless there is
> a clear indication that this way we would gain something. Benchmarks
> shows that this is not a case thus it's only a matter of taste and
> intuition.


I can understand your feeling here and agree with you that it is a matter
of taste.  However, many a times if we change the patch according to
committer's taste, the chances of patch getting accepted early is better,
but I am not sure if that applies here, so feel free to go in the way you
think is better.

[1] -
http://www.postgresql.org/message-id/CA+TgmoacVsdcY=qn0do7nok7w2-ssqz3kr2y84bavifckqd...@mail.gmail.com


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

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

On Fri, Mar 4, 2016 at 4:06 AM, Andres Freund  wrote:
> Hi,

Thanks for the review.

>> +/*
>> + * rename_safe -- rename of a file, making it on-disk persistent
>> + *
>> + * This routine ensures that a rename file persists in case of a crash by 
>> using
>> + * fsync on the old and new files before and after performing the rename so 
>> as
>> + * this categorizes as an all-or-nothing operation.
>> + */
>> +int
>> +rename_safe(const char *oldfile, const char *newfile)
>> +{
>> + struct stat filestats;
>> +
>> + /*
>> +  * First fsync the old entry and new entry, it this one exists, to 
>> ensure
>> +  * that they are properly persistent on disk. Calling this routine with
>> +  * an existing new target file is fine, rename() will first remove it
>> +  * before performing its operation.
>> +  */
>> + fsync_fname(oldfile, false);
>> + if (stat(newfile, ) == 0)
>> + fsync_fname(newfile, false);
>
> I don't think we want any stat()s here. I'd much, much rather check open
> for ENOENT.

OK. So you mean more or less that, right?
int fd;
fd = OpenTransientFile(newfile, PG_BINARY | O_RDONLY, 0);
if (fd < 0)
{
if (errno != ENOENT)
return -1;
}
else
{
pg_fsync(fd);
CloseTransientFile(fd);
}

>> +/*
>> + * link_safe -- make a file hard link, making it on-disk persistent
>> + *
>> + * This routine ensures that a hard link created on a file persists on 
>> system
>> + * in case of a crash by using fsync where on the link generated as well as 
>> on
>> + * its parent directory.
>> + */
>> +int
>> +link_safe(const char *oldfile, const char *newfile)
>> +{
>
> If we go for a new abstraction here, I'd rather make it
> 'replace_file_safe' or something, and move the link/rename code #ifdef
> into it.

Hm. OK. I don't see any reason why switching to link() even in code
paths like KeepFileRestoredFromArchive() or pgarch_archiveDone() would
be a problem thinking about it. Should HAVE_WORKING_LINK be available
on a platform we can combine it with unlink. Is that in line with what
you think?

>> + if (link(oldfile, newfile) < 0)
>> + return -1;
>> +
>> + /*
>> +  * Make the link persistent in case of an OS crash, the new entry
>> +  * generated as well as its parent directory need to be flushed.
>> +  */
>> + fsync_fname(newfile, false);
>> +
>> + /*
>> +  * Same for parent directory. This routine is never called to rename
>> +  * files across directories, but if this proves to become the case,
>> +  * flushing the parent directory if the old file would be necessary.
>> +  */
>> + fsync_parent_path(newfile);
>> + return 0;
>
> I think it's a seriously bad idea to encode that knowledge in such a
> general sounding routine.  We could however argue that this is about
> safely replacing the *target* file; not about safely removing the old
> file.

Not sure I am following here. Are you referring to the fact that if
the new file and old file are on different directories would make this
routine unreliable? Because yes that's the case if we want to make
both of them persistent, and I think we want to do so. Do you suggest
to correct this comment to remove the mention to the old file's parent
directory because we just care about having the new file as being
persistent? Or do you suggest that we should actually extend this
routine so as we fsync both the new and old file's parent directory if
they differ?

> Currently I'm inclined to apply this to master soon. But I think we
> might want to wait a while with backpatching.  The recent fsync upgrade
> disaster kinda makes me a bit careful...

There have not been actual field complaints about that yet. That's
fine for me to wait a bit.
-- 
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] postgres_fdw vs. force_parallel_mode on ppc

Robert Haas  writes:
> A couple of my colleagues have been looking into this.  It's not
> entirely clear to me what's going on here yet, but it looks like the
> stats get there if you wait long enough.  Rahila Syed was able to
> reproduce the problem and says that the attached patch fixes it.  But
> I don't quite understand why this should fix it.

I don't like this patch much.  While the new function is not bad in
itself, it looks really weird to call it immediately after the other
wait function.  And the reason for that, AFAICT, is that somebody dropped
the entire "truncation stats" test sequence into the middle of unrelated
tests, evidently in the vain hope that that way they could piggyback
on the existing wait.  Which these failures are showing us is wrong.

I think we should move all the inserted logic down so that it's not in the
middle of unrelated testing.

> BTW, this comment is obsolete:

> -- force the rate-limiting logic in pgstat_report_tabstat() to time out
> -- and send a message
> SELECT pg_sleep(1.0);
>  pg_sleep
> --

> (1 row)

> That function was renamed in commit
> 93c701edc6c6f065cd25f77f63ab31aff085d6ac, but apparently Tom forgot to
> grep for other uses of that identifier name.

Duh :-(.  Actually, do we need that sleep at all anymore?  Seems like
wait_for_stats ought to cover it.

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] OOM in libpq and infinite loop with getCopyStart()

On Fri, Mar 4, 2016 at 12:59 AM, Aleksander Alekseev
 wrote:
>> The easiest way to perform tests with this patch is to take a debugger
>> and enforce the malloc'd pointers to NULL in the code paths.
>
> I see. Still I don't think it's an excuse to not provide clear steps to
> reproduce an issue. As I see it anyone should be able to easily check
> your patch locally without having deep understanding of how libpq is
> implemented or reading thread which contains 48 e-mails.

OK, here they are if that helps. Following those steps and having some
knowledge of lldb or gdb is enough. The key point is that getCopyStart
is the routine to breakpoint and make fail.
A) psql and COPY.
1) gdb --args psql -c 'COPY (SELECT 1) TO stdout'
2) Then take a breakpoint at getCopyStart()
3) run
4) Enforce the return result of PQmakeEmptyPGresult to NULL:
p result = 0
5) Enjoy the infinite loop with HEAD, and the error with the patch

B) pg_receivexlog:
1) mkdir data
gdb --args pg_receivexlog --verbose -D data/
2) Take a breakpoint at getCopyStart
3) run
4) enforce result = 0 after the call to PQmakeEmptyPGresult
5) And enjoy what has been reported

C) pg_recvlogical, similar to B.
1) Create a replication slot on a server that accepts them
select pg_create_logical_replication_slot('foo', 'test_decoding');
2) Same breakpoint as B) with this start command or similar (depends
on where the logical slot has been created):
pg_recvlogical --start --slot foo -f - -d postgres

D) triggering standby problem:
1) Patch upstream, as follows by adding a sleep in
libpqrcv_startstreaming of libpqwalreceiver.c. This is a simple method
to have the time to take a breakpoint on the WAL receiver process that
has been started, with streaming that has not begun yet.
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index f670957..c7fccf1 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -188,6 +188,9 @@ libpqrcv_startstreaming(TimeLineID tli, XLogRecPtr
startpoint, char *slotname)
charcmd[256];
PGresult   *res;

+   /* ZZZzzz... */
+   pg_usleep(1L);
+
/* Start streaming from the point requested by startup process */
if (slotname != NULL)
snprintf(cmd, sizeof(cmd),
Trying to design a test taking into account the context of a WAL
receiver does not seem worth it to me with a simple method like this
one...
2) Start the standby and attach debugger on the WAL receiver process,
send SIGSTOP to it, whatever...
3) breakpoint at getCopyStart
4) Then move on with the same process as previously, and enjoy the errors.
-- 
Michael
diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index f670957..c7fccf1 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -188,6 +188,9 @@ libpqrcv_startstreaming(TimeLineID tli, XLogRecPtr startpoint, char *slotname)
 	char		cmd[256];
 	PGresult   *res;
 
+	/* ZZZzzz... */
+	pg_usleep(1L);
+
 	/* Start streaming from the point requested by startup process */
 	if (slotname != NULL)
 		snprintf(cmd, sizeof(cmd),

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

Re: [HACKERS] Relation extension scalability

2016-03-03 Thread Dilip Kumar

On Wed, Mar 2, 2016 at 10:31 AM, Dilip Kumar  wrote:

> 1. One option can be as you suggested like ProcArrayGroupClearXid, With
> some modification, because when we wait for the request and extend w.r.t
> that, may be again we face the Context Switch problem, So may be we can
> extend in some multiple of the Request.
> (But we need to take care whether to give block directly to requester or
> add it into FSM or do both i.e. give at-least one block to requester and
> put some multiple in FSM)
>
>
> 2. Other option can be that we analyse the current Load on the extend and
> then speed up or slow down the extending speed.
> Simple algorithm can look like this
>

I have tried the approach of group extend,

1. We convert the extension lock to TryLock and if we get the lock then
extend by one block.2.
2. If don't get the Lock then use the Group leader concep where only one
process will extend for all, Slight change from ProcArrayGroupClear is that
here other than satisfying the requested backend we Add some extra blocks
in FSM, say GroupSize*10.
3. So Actually we can not get exact load but still we have some factor like
group size tell us exactly the contention size and we extend in multiple of
that.

Performance Analysis:
-
Performance is scaling with this approach, its slightly less compared to
previous patch where we directly give extend_by_block parameter and extend
in multiple blocks, and I think that's obvious because in group extend case
only after contention happen on lock we add extra blocks, but in former
case it was extending extra blocks optimistically.

Test1(COPY)
-
./psql -d postgres -c "COPY (select g.i::text FROM generate_series(1,
1) g(i)) TO '/tmp/copybinary' WITH BINARY";
echo COPY data from '/tmp/copybinary' WITH BINARY; > copy_script

./pgbench -f copy_script -T 300 -c$ -j$ postgres

Shared Buffer:8GBmax_wal_size:10GB  Storage:Magnetic DiskWAL:SSD
---
ClientBase  multi_extend by 20 page group_extend_patch(groupsize*10)
1  105157   149
2  217255   282
4  210 494  452
8  166702   629
16145 563  561
32124 480  566

Test2(INSERT)

./psql -d postgres -c "create table test_data(a int, b text)"
./psql -d postgres -c "insert into test_data
values(generate_series(1,1000),repeat('x', 1024))"
./psql -d postgres -c "create table data (a int, b text)
echo "insert into data select * from test_data;" >> insert_script

 shared_buffers=512GB  max_wal_size=20GB  checkpoint_timeout=10min
./pgbench -c $ -j $ -f insert_script -T 300 postgres

ClientBaseMulti-extend by 1000*group extend (group*10)
*group extend (group*100)
1117118
125 122
2111140
   161 159
4 51 190
141 187
843  254
148 172
16   40 243
150 173

* (group*10)-> means inside the code, Group leader will see how many
members are in the group who want blocks, so we will satisfy request of all
member + will put extra blocks in FSM extra block to extend are =
(group*10) --> 10 is just some constant.

Summary:

1. Here with group extend patch, there is no configuration to tell that how
many block to extend, so that should be decided by current load in the
system (contention on the extension lock).
2. With small multiplier i.e. 10 we can get fairly good improvement compare
to base code, but when load is high like record size is 1K, improving the
multiplier size giving better results.

* Note: Currently this is POC patch, It has only one group Extend List, so
currently can handle only one relation Group extend.

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

diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c
index 8140418..adb82ba 100644
--- a/src/backend/access/heap/hio.c
+++ b/src/backend/access/heap/hio.c
@@ -23,6 +23,7 @@
 #include "storage/freespace.h"
 #include "storage/lmgr.h"
 #include "storage/smgr.h"
+#include "storage/proc.h"

 /*
@@ -168,6 +169,160 @@ GetVisibilityMapPins(Relation relation, Buffer buffer1, Buffer buffer2,
 	}
 }

+
+static BlockNumber
+GroupExtendRelation(PGPROC *proc, Relation relation, BulkInsertState bistate)
+{
+	volatile PROC_HDR *procglobal = ProcGlobal;
+	uint32		nextidx;
+	uint32

Re: [HACKERS] TAP / recovery-test fs-level backups, psql enhancements etc

On Fri, Mar 4, 2016 at 8:22 AM, Craig Ringer  wrote:
> On 4 March 2016 at 05:08, Alvaro Herrera  wrote:
>> Patches 0004 and 0007 remain.
>
> For readers who're not following closely that's the filtering support for
> RecursiveCopy and the support for taking filesystem-level backups in
> PostgresNode that uses it.

I am still not sure that the filter for pg_log by default is that useful but...

>> I think 0007 is uncontroversial; I
>> reworked 0004 a bit and gave it to someone else to finish a couple of
>> didn't I didn't quite like -- hopefully she'll be submitting a new
>> version soonish.  Once we have that I'm happy to push them too.
>
> Great, thanks.

No objections from here as well for the feature itself. I am glad to
see interest in extending the current infrastructure, and just
wondering what kind of tests are going to show up.
-- 
Michael


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

Re: [HACKERS] proposal: psql autocomplete for casting

2016-03-03 Thread Kyotaro HORIGUCHI

At Thu, 3 Mar 2016 12:15:13 +0100, Pavel Stehule  
wrote in 
pavel.stehule> 2016-03-03 12:06 GMT+01:00 Kyotaro HORIGUCHI <
> the requirement of space before is not good :( - It should be any different
> than operator chars. Not only space.
> 
> all other is perfect :)

Yeah, I fortunately agree with you:p

But the things are not so simple. readline can handle single
prefix characters but cannot not handle prefix strings.

The new diff adds ':' to WORD_BREAKS and adjusts related codes.
As far as I can see, colons are used only for variable prefix,
type casts and named parameter assignments in function
calls. This covers the first two and the last wouldn't be a
matter of tab-completion. This works as the following.

> =# select now()::t
> text trigger
> tid  tsm_handler
> ...
> tintervaltxid_snapshot
> =# select now()::te
> =# select now()::text 

As an inevitable side effect, this makes completion for ": :"
with types (which results in an syntax error) but I believe it
won't be a matter.

I'm quite unpleasant that the additional conditional expressions
use bare previous_words but found no good solution.

>   else if (previous_words_count >= 2 &&
>previous_words[1][strlen(previous_words[1])-1] == ':' &&
>TailMatches1(":"))

It is good if we could limit the candidate types by knowing the
operand type but it seems a bit complicated.

Some familiar type names won't come as candidates. They are
"int", "float", "decimal", "dec", which are known only to gram.y
but it does't seem to matter, too.

Thoughts? Opinions?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 5f27120..ab0e858 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -58,7 +58,7 @@ extern char *filename_completion_function();
 #endif
 
 /* word break characters */
-#define WORD_BREAKS		"\t\n@$><=;|&{() "
+#define WORD_BREAKS		"\t\n@$><=;:|&{() "
 
 /*
  * Since readline doesn't let us pass any state through to the tab completion
@@ -1312,15 +1312,23 @@ psql_completion(const char *text, int start, int end)
 	if (text[0] == '\\')
 		COMPLETE_WITH_LIST_CS(backslash_commands);
 
+	/* If current word is a typecast, handle that case */
+	else if (previous_words_count >= 2 &&
+			 previous_words[1][strlen(previous_words[1])-1] == ':' &&
+			 TailMatches1(":"))
+		COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_datatypes, NULL);
+
 	/* If current word is a variable interpolation, handle that case */
-	else if (text[0] == ':' && text[1] != ':')
-	{
-		if (text[1] == '\'')
-			matches = complete_from_variables(text, ":'", "'", true);
-		else if (text[1] == '"')
-			matches = complete_from_variables(text, ":\"", "\"", true);
+	else if ((previous_words_count < 2 ||
+			  previous_words[1][strlen(previous_words[1])-1] != ':') &&
+			 TailMatches1(":"))
+	{
+		if (text[0] == '\'')
+			matches = complete_from_variables(text, "'", "'", true);
+		else if (text[0] == '"')
+			matches = complete_from_variables(text, "\"", "\"", true);
 		else
-			matches = complete_from_variables(text, ":", "", true);
+			matches = complete_from_variables(text, "", "", true);
 	}
 
 	/* If no previous word, suggest one of the basic sql commands */

-- 
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] RFC: replace pg_stat_activity.waiting with something more descriptive

2016-03-03 Thread Amit Kapila

On Wed, Feb 24, 2016 at 7:14 PM, Robert Haas  wrote:
>
> On Mon, Feb 22, 2016 at 10:05 AM, Amit Kapila 
wrote:
> >> I wouldn't bother tinkering with it at this point.  The value isn't
> >> going to be recorded on disk anywhere, so it will be easy to change
> >> the way it's computed in the future if we ever need to do that.
> >>
> >
> > Okay. Find the rebased patch attached with this mail.  I have moved
> > this patch to upcoming CF.
>
> I would call the functions pgstat_report_wait_start() and
> pgstat_report_wait_end() instead of pgstat_report_start_waiting() and
> pgstat_report_end_waiting().
>

Changed as per suggestion and made these functions inline.

> I think pgstat_get_wait_event_type should not return HWLock, a term
> that appears nowhere in our source tree at present.  How about just
> "Lock"?
>

Changed as per suggestion.

> I think the wait event types should be documented - and the wait
> events too, perhaps.
>

As discussed upthread, I have added documentation for all the possible wait
events and an example.  Some of the LWLocks like AsyncQueueLock and
AsyncCtlLock are used for quite similar purpose, so I have kept there
explanation as same.

> Maybe it's worth having separate wait event type names for lwlocks and
> lwlock tranches.  We could report LWLockNamed and LWLockTranche and
> document the difference: "LWLockNamed indicates that the backend is
> waiting for a specific, named LWLock.  The event name is the name of
> that lock.  LWLockTranche indicates that the backend is waiting for
> any one of a group of locks with similar function.  The event name
> identifies the general purpose of locks in that group."
>

Changed as per suggestion.

> There's no requirement that every session have every tranche
> registered.  I think we should consider displaying "extension" for any
> tranche that's not built-in, or at least for tranches that are not
> registered (rather than "unknown wait event").
>
> +   if (lock->tranche == 0 && lockId < NUM_INDIVIDUAL_LWLOCKS)
>
> Isn't the second part automatically true at this point?
>

Fixed.

> The changes to LockBufferForCleanup() don't look right to me.  Waiting
> for a buffer pin might be a reasonable thing to define as a wait
> event, but it shouldn't reported as if we were waiting on the LWLock
> itself.
>

As discussed upthread, added a new wait event BufferPin for this case.

> What happens if an error is thrown while we're in a wait?
>

As discussed upthread, added in AbortTransaction and from where
ever LWLockReleaseAll() gets called, point to note is that we can call this
function only when we are sure there is no further possibility of wait on
LWLock.

> Does this patch hurt performance?
>

Performance tests are underway.

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


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

On 17 February 2016 at 17:50, Haribabu Kommi  wrote:
> Here I attached a draft patch based on previous discussions. It still needs
> better comments and optimization.

Over in [1] Tom posted a large change to the grouping planner which
causes large conflict with the parallel aggregation patch. I've been
looking over Tom's patch and reading the related thread and I've
observed 3 things:

1. Parallel Aggregate will be much easier to write and less code to
base it up top of Tom's upper planner changes. The latest patch does
add a bit of cruft (e.g create_gather_plan_from_subplan()) which won't
be required after Tom pushes the changes to the upper planner.
2. If we apply parallel aggregate before Tom's upper planner changes
go in, then Tom needs to reinvent it again when rebasing his patch.
This seems senseless, so this is why I did this work.
3. Based on the thread, most people are leaning towards getting Tom's
changes in early to allow a bit more settle time before beta, and
perhaps also to allow other patches to go in after (e.g this)

So, I've done a bit of work and I've rewritten the parallel aggregate
code to base it on top of Tom's patch posted in [1]. There's a few
things that are left unsolved at this stage.

1. exprType() for Aggref still returns the aggtype, where it needs to
return the trans type for partial agg nodes, this need to return the
trans type rather than the aggtype. I had thought I might fix this by
adding a proxy node type that sits in the targetlist until setrefs.c
where it can be plucked out and replaced by the Aggref. I need to
investigate this further.
2. There's an outstanding bug relating to HAVING clause not seeing the
right state of aggregation and returning wrong results. I've not had
much time to look into this yet, but I suspect its an existing bug
that's already in master from my combine aggregate patch. I will
investigate this on Sunday.

In regards to the patch, there's a few things worth mentioning here:

1. I've had to add a parallel_degree parameter to create_group_path()
and create_agg_path(). I think Tom is going to make changes to his
patch so that the Path's parallel_degree is propagated to subnodes,
this should allow me to remove this parameter and just use
parallel_degree the one from the subpath.
2. I had to add a new parameter to pass an optional row estimate to
cost_gather() as I don't have a RelOptInfo available to get a row
estimate from which represents the state after partial aggregation. I
thought this change was ok, but I'll listen to anyone who thinks of a
better way to do it.
3. The code never attempts to mix and match Grouping Agg and Hash Agg
plans. e.g it could be an idea to perform Partial Hash Aggregate ->
Gather -> Sort -> Finalize Group Aggregate, or hash as in the Finalize
stage. I just thought doing this is more complex than what's really
needed, but if someone can think of a case where this would be a great
win then I'll listen, but you have to remember we don't have any
pre-sorted partial paths at this stage, so an explicit sort is
required *always*. This might change if someone invented partial btree
index scans... but until then...

Due to the existence of the outstanding issues above, I feel like I
might be posting the patch a little earlier, but wanted to do so since
this is quite a hot area in the code at the moment and I wanted to
post for transparency.

To apply the patch please apply [1] first.

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

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

parallel_aggregation_d6850a9f_2016-03-04.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] pgbench stats per script & other stuff

Michael Paquier wrote:
> On Tue, Feb 9, 2016 at 4:22 AM, Fabien COELHO  wrote:

> > Hmmm, I type them and I'm not so good with a keyboard, so "se" is better
> > than:
> >
> > "selct-onlyect-only".
> 
> I can understand that feeling.

Pushed 19-e, thanks.

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


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

Re: [HACKERS] Incorrect error message in InitializeSessionUserId

On Fri, Mar 4, 2016 at 10:45 AM, Haribabu Kommi
 wrote:
> On Wed, Mar 2, 2016 at 12:21 AM, Dmitriy Sarafannikov
>  wrote:
>> Hi all,
>>
>> I have found incorrect error message in InitializeSessionUserId function
>> if you try to connect to database by role Oid (for example
>> BackgroundWorkerInitializeConnectionByOid).
>> If role have no permissions to login, you will see error message like this:
>> FATAL:  role "(null)" is not permitted to log in
>>
>> I changed few lines of code and fixed this.
>> Patch is attached.
>> I want to add this patch to commitfest.
>> Any objections?
>>
>
> The patch adds the support of taking the role name from the role tuple
> instead of using the provided rolename variable, because it is possible
> that rolename variable is NULL if the connection is from a background
> worker.
>
> The patch is fine, I didn't find any problems, I marked it as ready for
> committer.
>
> IMO this patch may need to backpatch supported branches as it is
> a bug fix. Committer can decide.

+1 for the backpatch. The current error message should the rolename be
undefined in this context is misleading for users.
-- 
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] WIP: Upper planner pathification

On 4 March 2016 at 09:29, Robert Haas  wrote:
> On Thu, Mar 3, 2016 at 2:19 PM, Tom Lane  wrote:
>> This leads me to the conclusion that all these new node types should
>> set parallel_aware to false and copy up the other two fields from the
>> child, except for LockRows and ModifyTable which should set them all
>> to false/0.  Correct?  If so, I'll go fix.
>
> Well, I'd probably bubble it up regardless.  The fact that the overall
> plan writes data will cause everything in the plan to have
> parallel_safe = false and parallel_degree = 0, so you'll get the same
> outcome either way.  However, that way, if writes eventually become
> safe, then this won't need adjusting.  But it doesn't really matter
> much; feel free to do it as you say if you prefer.

This would help me too. I hit a problem with this when adding Group
Parallel Aggregate, as the sort path is conditionally added in
create_grouping_paths() which causes the parallel_degree for the
subpath which is later passed into create_agg_path() to become 0. in
create_agg_path() I was using the parallel_degree variable to
determine if I should construct a normal serial agg path, or a chain
of partial agg -> gather -> final agg paths. To get around the problem
I added a parallel_degree parameter to create_agg_path() of which that
parameter so far seems to only exist in create_seqscan_path(), which
naturally is a bit special as it's a "leaf node" in the path tree.
Propagating these would mean I could remove that parameter again.

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

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

Re: [HACKERS] raw output from copy

2016-03-03 Thread Corey Huinker

On Sat, Feb 27, 2016 at 2:26 AM, Pavel Stehule 
wrote:

> Hi
>
> 2015-08-06 10:37 GMT+02:00 Pavel Stehule :
>
>> Hi,
>>
>> Psql based implementation needs new infrastructure (more than few lines)
>>
>> Missing:
>>
>> * binary mode support
>> * parametrized query support,
>>
>> I am not against, but both points I proposed, and both was rejected.
>>
>> So why dont use current infrastructure? Raw copy is trivial patch.
>>
>
> I was asked by Daniel Verite about reopening this patch in opened
> commitfest.
>
> I am sending rebased patch
>
> Regards
>
> Pavel
>
>
>
Since this patch does something I need for my own work, I've signed up as a
reviewer.

>From a design standpoint, I feel that COPY is the preferred means of
dealing with data from sources too transient to justify setting up a
foreign data wrapper, and too simple to justify writing application code.
So, for me, RAW is the right solution, or at least *a* right solution.

My first pass of reading the code changes and the regression tests is
complete, and I found the changes to be clear and fairly straightforward.
This shouldn't surprise anyone, as the previous reviewers had only minor
quibbles with the code. So far, so good.

The regression tests seem to adequately cover all new functionality, though
I wonder if we should add some cases that highlight situations where BINARY
mode is insufficient.

Before I give my approval, I want to read it again more closely to make
sure that no cases were skipped with regard to the  (binary || raw) and
(binary || !raw) tests. Also, I want to use it on some of my problematic
files. Maybe I'll find a good edge case. Probably not.

I hope to find time for those things in the next few days.

Re: [HACKERS] Way to check whether a particular block is on the shared_buffer?

2016-03-03 Thread Kouhei Kaigai

I found one other, but tiny, problem to implement SSD-to-GPU direct
data transfer feature under the PostgreSQL storage.

Extension cannot know the raw file descriptor opened by smgr.

I expect an extension issues an ioctl(2) on the special device file
on behalf of the special kernel driver, to control the P2P DMA.
This ioctl(2) will pack file descriptor of the DMA source and some
various information (like base position, range, destination device
pointer, ...).

However, the raw file descriptor is wrapped in the fd.c, instead of
the File handler, thus, not visible to extension. oops...

The attached patch provides a way to obtain raw file descriptor (and
relevant flags) of a particular File virtual file descriptor on
PostgreSQL. (No need to say, extension has to treat the raw descriptor
carefully not to give an adverse effect to the storage manager.)

How about this tiny enhancement?

> > -Original Message-
> > From: pgsql-hackers-ow...@postgresql.org
> > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Robert Haas
> > Sent: Saturday, February 13, 2016 1:46 PM
> > To: Kaigai Kouhei(海外 浩平)
> > Cc: Jim Nasby; pgsql-hackers@postgresql.org; Amit Langote
> > Subject: Re: [HACKERS] Way to check whether a particular block is on the
> > shared_buffer?
> >
> > On Thu, Feb 11, 2016 at 9:05 PM, Kouhei Kaigai  wrote:
> > > Hmm. In my experience, it is often not a productive discussion whether
> > > a feature is niche or commodity. So, let me change the viewpoint.
> > >
> > > We may utilize OS-level locking mechanism here.
> > >
> > > Even though it depends on filesystem implementation under the VFS,
> > > we may use inode->i_mutex lock that shall be acquired during the buffer
> > > copy from user to kernel, at least, on a few major filesystems; ext4,
> > > xfs and btrfs in my research. As well, the modified NVMe SSD driver can
> > > acquire the inode->i_mutex lock during P2P DMA transfer.
> > >
> > > Once we can consider the OS buffer is updated atomically by the lock,
> > > we don't need to worry about corrupted pages, but still needs to pay
> > > attention to the scenario when updated buffer page is moved to GPU.
> > >
> > > In this case, PD_ALL_VISIBLE may give us a hint. GPU side has no MVCC
> > > infrastructure, so I intend to move all-visible pages only.
> > > If someone updates the buffer concurrently, then write out the page
> > > including invisible tuples, PD_ALL_VISIBLE flag shall be cleared because
> > > updated tuples should not be visible to the transaction which issued
> > > P2P DMA.
> > >
> > > Once GPU met a page with !PD_ALL_VISIBLE, it can return an error status
> > > that indicates CPU to retry this page again. In this case, this page is
> > > likely loaded to the shared buffer already, so retry penalty is not so
> > > much.
> > >
> > > I'll try to investigate the implementation in this way.
> > > Please correct me, if I misunderstand something (especially, treatment
> > > of PD_ALL_VISIBLE).
> >
> > I suppose there's no theoretical reason why the buffer couldn't go
> > from all-visible to not-all-visible and back to all-visible again all
> > during the time you are copying it.
> >
> The backend process that is copying the data to GPU has a transaction
> in-progress (= not committed). Is it possible to get the updated buffer
> page back to the all-visible state again?
> I expect that in-progress transactions works as a blocker for backing
> to all-visible. Right?
> 
> > Honestly, I think trying to access buffers without going through
> > shared_buffers is likely to be very hard to make correct and probably
> > a loser.
> >
> No challenge, no outcome. ;-)
> 
> > Copying the data into shared_buffers and then to the GPU is,
> > doubtless, at least somewhat slower.  But I kind of doubt that it's
> > enough slower to make up for all of the problems you're going to have
> > with the approach you've chosen.
> >
> Honestly, I'm still uncertain whether it works well as I expects.
> However, scan workload on the table larger than main memory is
> headache for PG-Strom, so I'd like to try ideas we can implement.
> 
> Thanks,
> --
> NEC Business Creation Division / PG-Strom Project
> KaiGai Kohei 
>



pgsql-v9.6-filegetrawdesc.1.patch
Description: pgsql-v9.6-filegetrawdesc.1.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] Incorrect error message in InitializeSessionUserId

2016-03-03 Thread Haribabu Kommi

On Wed, Mar 2, 2016 at 12:21 AM, Dmitriy Sarafannikov
 wrote:
> Hi all,
>
> I have found incorrect error message in InitializeSessionUserId function
> if you try to connect to database by role Oid (for example
> BackgroundWorkerInitializeConnectionByOid).
> If role have no permissions to login, you will see error message like this:
> FATAL:  role "(null)" is not permitted to log in
>
> I changed few lines of code and fixed this.
> Patch is attached.
> I want to add this patch to commitfest.
> Any objections?
>

The patch adds the support of taking the role name from the role tuple
instead of using the provided rolename variable, because it is possible
that rolename variable is NULL if the connection is from a background
worker.

The patch is fine, I didn't find any problems, I marked it as ready for
committer.

IMO this patch may need to backpatch supported branches as it is
a bug fix. Committer can decide.

Regards,
Hari Babu
Fujitsu Australia

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

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

On Fri, Mar 4, 2016 at 7:21 AM, Petr Jelinek  wrote:
> Well the source code does not compile on MSVC2015, the perl changes needed
> are really tiny, there is some code that needs changes to work with 2015,
> particularly in the locale code-page detection area so it's definitely not
> just build system. But I think it should be fairly localized and fenced by
> ifdef anyway.

Yeah, that's my first impression as well. We should not need any APIs
changes and the changes would be limited to if extra blocks with
_MSC_VER, if that would occur then I definitely agree that patching
only HEAD is the way to go. I'll look at that today and the next
couple of days, let's see what I can get out of it...
-- 
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] Fix for OpenSSL error queue bug

2016-03-03 Thread Peter Eisentraut

On 2/5/16 5:04 AM, Peter Geoghegan wrote:
> As Heikki goes into on that thread, the appropriate action seems to be
> to constantly reset the error queue, and to make sure that we
> ourselves clear the queue consistently. (Note that we might not have
> consistently called ERR_get_error() in the event of an OOM within
> SSLerrmessage(), for example). I have not changed backend code in the
> patch, though; I felt that we had enough control of the general
> situation there for it to be unnecessary to lock everything down.

I think clearing the error after a call is not necessary.  The API
clearly requires that you should clear the error queue before a call, so
clearing it afterwards does not accomplish anything, except maybe make
broken code work sometimes, for a while.  Also, there is nothing that
says that an error produces exactly one entry in the error queue; it
could be multiple.  Or that errors couldn't arise at random times
between the reset and whatever happens next.

I think this is analogous to clearing errno before a C library call.
You could clear it afterwards as well, to be nice to the next guy, but
the next guy should really take care of that themselves, and we can't
rely on what happens in between anyway.

The places that you identified for change look correct as far as libpq
goes.  I do think that the backend should be updated in the same way,
because it's a) correct, b) easy enough, and c) there could well be
interactions with postgres_fdw, plproxy, plperl, or who knows what.

-- 
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] TAP / recovery-test fs-level backups, psql enhancements etc

On 4 March 2016 at 05:08, Alvaro Herrera  wrote:


>
> Okay, so far I have pushed 0001 and 0002 squashed (commit 5bec1ad4648),
> 0003 (commit 7d9a4301c08), 0005 and 0006 squashed (commit 2c83f435a3de).
> In the last one I chose to rename your psql_check to safe_psql and
> tweaked a few other things, not worthy of individual mention.  I think
> the result should still work on Perl 5.8 though I didn't actually verify
> that -- I don't think I made any changes that would affect portability.
> I will be downloading your Dockerized stuff shortly while I still have a
> convenient network connection, just in case.
>
>
Thanks very much. Your commit messages are much better too.

Patches 0004 and 0007 remain.


For readers who're not following closely that's the filtering support for
RecursiveCopy and the support for taking filesystem-level backups in
PostgresNode that uses it.


> I think 0007 is uncontroversial; I
> reworked 0004 a bit and gave it to someone else to finish a couple of
> didn't I didn't quite like -- hopefully she'll be submitting a new
> version soonish.  Once we have that I'm happy to push them too.


Great, thanks.


>
> > I don't expect to be doing much more on the framework at this point as I
> > want to be able to get back to the code I had to enhance the framework in
> > order to test
>
> How come!?!?


This yak grows hair faster than I can shave it ;)

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

Re: [HACKERS] pgbench stats per script & other stuff

I looked at 19.d and I think the design has gotten pretty convoluted.  I
think we could simplify with the following changes:

struct script_t gets a new member, of type Command **, which is
initially null.

function process_builtin receives the complete script_t (not individual
memebers of it) constructs the Command ** array and puts it in
script_t's new member; return value is the same script_t struct it got
(except it's now augmented with the Command **array).

function process_file constructs a new script_t from the string list,
creates its Command **array just like process_builtin and returns the
constructed struct.

function addScript receives script_t instead of individual members of
it, and does the appropriate thing.


Alternatively, we could have a different struct that's defined to carry
only the Command ** array (not the command string array) and is returned 
by both process_builtin and process_file.  Perhaps we could also put the
script weight in there.  With this arrangement we don't need to typedef
script_t at all and we can just keep it as an anonymous struct as today.

This is what I tried to describe earlier, but obviously I wasn't clear
enough.

Thanks,

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


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

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

2016-03-03 Thread Petr Jelinek


On 03/03/16 18:41, Magnus Hagander wrote:

On Thu, Mar 3, 2016 at 6:18 PM, Andrew Dunstan > wrote:

On 03/03/2016 09:02 AM, Michael Paquier wrote:

Microsoft provides a set of VMs that one can use for testing and
Windows 10 is in the set:
https://dev.windows.com/en-us/microsoft-edge/tools/vms/windows/
I have grabbed one and installed the community version of Visual
Studio 2015 so I think that I am going to be able to compile
Postgres
with VS2015 with a bit of black magic.

So, would there be interest for a patch on the perl scripts in
src/tools/msvc or are they considered a lost cause? Having a look at
the failures could be done with the cmake work, but it seems a bit
premature to me to look at that for the moment, and having
support for
VS2015 with 9.5 (support for new versions of VS won a backpatch the
last couple of years) would be a good thing I think.




I am not holding my breath on cmake. Until we have something pretty
solid on that front I'm going to assume it's not happening. If we're
going to support VS2015 (and I think we should) then it should be
supported for all live branches if possible. I'm assuming the
changes would be pretty localized, at least in src/tools/msvc, and
adding a new compile shouldn't break anything with existing compilers.


+1.

Definitely do it for HEAD.

Then if it gets backpatched is going to depend on the locality I think.
If it's just the build system then it should be no problem, but I
thought Michael also suggested some API changes. If that's so, then it
is going to depend on how invasive those are. But that part should be
done for HEAD regardless, so it's definitely worth the effort to figure
out exactly what it involves.




Well the source code does not compile on MSVC2015, the perl changes 
needed are really tiny, there is some code that needs changes to work 
with 2015, particularly in the locale code-page detection area so it's 
definitely not just build system. But I think it should be fairly 
localized and fenced by ifdef anyway.


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


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

Re: [HACKERS] TAP / recovery-test fs-level backups, psql enhancements etc

Craig Ringer wrote:

> The first three are simple fixes that should go in without fuss:
> 
> 001 fixes the above 5.8.8 compat issue.
> 
> 002  fixes another minor whoopsie, a syntax error in  src/test/recovery/t/
> 003_recovery_targets.pl that never got noticed because exit codes are
> ignored.
> 
> 003 runs perltidy on PostgresNode.pm to bring it back into conformance
> after the recovery tests commit.
> 
> The rest are feature patches:
> 
> 004 allows filtering on RecursiveCopy by a predicate function. Needed for
> filesystem level backups (007). It could probably be squashed with 007 if
> desired.
> 
> 005 adds the new psql functions psql_expert and psql_check. Then 006
> renames psql_expert to psql and fixes up all call sites across the tree to
> use the new interface. I found the bug in 002 as part of that process. I
> anticipate that 005 and 006 would be squashed into one commit to master,
> but I've kept them separate in my tree for easier review.
> 
> 007 adds PostgresNode support for hot and cold filesystem-level backups
> using pg_start_backup and pg_stop_backup, which will be required for some
> coming tests and are useful by themselves.

Okay, so far I have pushed 0001 and 0002 squashed (commit 5bec1ad4648),
0003 (commit 7d9a4301c08), 0005 and 0006 squashed (commit 2c83f435a3de).
In the last one I chose to rename your psql_check to safe_psql and
tweaked a few other things, not worthy of individual mention.  I think
the result should still work on Perl 5.8 though I didn't actually verify
that -- I don't think I made any changes that would affect portability.
I will be downloading your Dockerized stuff shortly while I still have a
convenient network connection, just in case.

Patches 0004 and 0007 remain.  I think 0007 is uncontroversial; I
reworked 0004 a bit and gave it to someone else to finish a couple of
didn't I didn't quite like -- hopefully she'll be submitting a new
version soonish.  Once we have that I'm happy to push them too.

> I don't expect to be doing much more on the framework at this point as I
> want to be able to get back to the code I had to enhance the framework in
> order to test

How come!?!?

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

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

Re: [HACKERS] WIP: Upper planner pathification

Robert Haas  writes:
> On Thu, Mar 3, 2016 at 2:19 PM, Tom Lane  wrote:
>> Thanks.  As I told Teodor last night, I can't reproduce a performance
>> issue here with pgbench-style queries.  Do you have any thoughts about
>> how we might satisfy ourselves whether there is or isn't a performance
>> problem?

> One idea might be to run a whole bunch of queries and record all of
> the planning times, and then run them all again and compare somehow.
> Maybe the regression tests, for example.

That sounds like something we could do pretty easily, though interpreting
the results might be nontrivial.  There will, I expect, be a mix of
queries that do get slower as well as those that don't.  As long as the
former are just queries that we hope to get some plan-quality win on,
I think that's an acceptable result ... but we'll need to record enough
data to tell what we're looking at.

For starters, I'll try logging whether the query has setops, grouping,
aggregation, window functions, and try to measure planning time change
in each of those categories.

regards, tom lane

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

Re: [HACKERS] WIP: Upper planner pathification

On Thu, Mar 3, 2016 at 2:19 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> Thanks for working on this.  Some review comments:
>
>> - I think all of the new path creation functions should bubble up
>> parallel_degree from their subpath.
>
> Ah, thanks, I didn't have any clue how to set that (though in my defense,
> the documentation about it is next to nonexistent).  Just to confirm,
> I assume the rules are:
>
> * parallel_aware: indicates whether the plan node itself has any
> parallelism behavior
>
> * parallel_safe: indicates that the entire plan tree rooted at this
> node is safe to execute in a parallel worker
>
> * parallel_degree: indicates number of parallel threads potentially
> useful for this plan tree (0 if not parallel-safe)

Right.

> This leads me to the conclusion that all these new node types should
> set parallel_aware to false and copy up the other two fields from the
> child, except for LockRows and ModifyTable which should set them all
> to false/0.  Correct?  If so, I'll go fix.

Well, I'd probably bubble it up regardless.  The fact that the overall
plan writes data will cause everything in the plan to have
parallel_safe = false and parallel_degree = 0, so you'll get the same
outcome either way.  However, that way, if writes eventually become
safe, then this won't need adjusting.  But it doesn't really matter
much; feel free to do it as you say if you prefer.

>> - RollupPath seems like a poor choice of name, if nothing else.  You
>> would expect that it would be related to GROUP BY ROLLUP() but of
>> course that's really the same thing as GROUP BY GROUPING SETS () or
>> GROUP BY CUBE (), and the fundamental operation is actually GROUPING
>> SETS, not ROLLUP.
>
> As I noted to David, that thing seems to me to be in need of refactoring,
> but I'd just as soon leave untangling the grouping-sets mess for later.
> I don't mind substituting a different name if you have a better idea,
> but don't really want to do more work than that right now.

Seems reasonable.  GroupingSetsPath?  MultipleGroupingPath?
RepeatedGroupingPath?

>> - A related point that is more connected to this patch is that you've
>> added 13 (!) new calls to disuse_physical_tlist, and 8 of those are
>> marked with /* XXX hack: need original tlist with sortgroupref marking
>> */.  I don't quite understand what's going on there.  I think if we're
>> going to be stuck with that hack we at least need some comments
>> explaining what is going on there.  What has caused us to suddenly
>> need these calls when we didn't before, and why these places and not
>> some others?
>
> Yeah, that's a hack to get things working.  The problem is that these node
> types need to be able to identify sort/group columns in their inputs, but
> if the child has switched to a "physical tlist" then the ressortgroupref
> marking isn't there, and indeed the needed column might not be there at
> all if it's a computed expression not a Var.  So what I did for the moment
> was to force the inputs back to their nominal tlists.  In the old code we
> didn't have this problem because none of the upper-level plan node types
> could see a physical tlist unless make_subplanTargetList had allowed it,
> and then we applied locate_grouping_columns() to re-identify the grouping
> columns.  That logic probably needs to be transposed into createplan.c,
> but I've not taken the time yet to figure out exactly how.  I don't know
> if it's reasonable to do that separately from rethinking how the whole
> disuse_physical_tlist thing works.

I don't know either, but it doesn't seem good to let this linger too long.

>> - For SortPath, you mention that the SortGroupClauses representation
>> isn't currently used.  It's not clear to me that it ever would be;
>> what would be the case where that might be useful?  At any rate, I'd
>> be inclined to rip it out for now; you can always put it back later.
>
> Yeah, I was dithering about that.  It seems like createplan.c now has
> a few too many ways to identify sort/group columns, and I was hoping
> to consolidate them somehow.  That might lead to wanting to use
> SortGroupClauses not PathKeys in some cases.  But until that's worked
> out, I agree the extra field is useless and we can just drop it.

OK.

>> - create_distinct_paths() disables the hash path if it seems like it
>> would exceed work_mem, unless the sort path isn't viable.  But there's
>> something that feels a bit uncomfortable about this.  Suppose the sort
>> path isn't viable but some other kind of future path is viable.  It
>> seems like it would be better to restructure this a bit so that the
>> decision about whether to add the hash path is based on whether there
>> are any other paths in the rel when we reach the bottom of the
>> function.  create_grouping_paths() has  a similar issue.
>
> OK, I'll take a look.  Quite a lot of these functions can probably stand
> more local rearrangements; I've been mainly concerned

Re: [HACKERS] postgres_fdw vs. force_parallel_mode on ppc

On Thu, Mar 3, 2016 at 1:10 AM, Tom Lane  wrote:
> Noah Misch  writes:
>> I've modified buildfarm member mandrill to use force_parallel_mode=regress 
>> and
>> max_parallel_degree=5; a full run passes.  We'll now see if it intermittently
>> fails the stats test, like Tom witnessed:
>> http://www.postgresql.org/message-id/30385.1456077...@sss.pgh.pa.us
>
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mandrill=2016-03-02%2023%3A34%3A10

A couple of my colleagues have been looking into this.  It's not
entirely clear to me what's going on here yet, but it looks like the
stats get there if you wait long enough.  Rahila Syed was able to
reproduce the problem and says that the attached patch fixes it.  But
I don't quite understand why this should fix it.

BTW, this comment is obsolete:

-- force the rate-limiting logic in pgstat_report_tabstat() to time out
-- and send a message
SELECT pg_sleep(1.0);
 pg_sleep
--

(1 row)

That function was renamed in commit
93c701edc6c6f065cd25f77f63ab31aff085d6ac, but apparently Tom forgot to
grep for other uses of that identifier name.

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

wait-for-trunc-stats.patch
Description: application/download

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

Re: [HACKERS] improving GROUP BY estimation

2016-03-03 Thread Mark Dilger

> On Mar 3, 2016, at 11:27 AM, Alexander Korotkov  
> wrote:
> 
> On Thu, Mar 3, 2016 at 10:16 PM, Tomas Vondra  
> wrote:
> So yes, each estimator works great for exactly the opposite cases. But notice 
> that typically, the results of the new formula is much higher than the old 
> one, sometimes by two orders of magnitude (and it shouldn't be difficult to 
> construct examples of much higher differences).
> 
> The table also includes the 'average' estimator you propose, but it's rather 
> obvious that the result is always much closer to the new value, simply because
> 
>(small number) + (huge number)
>--
>   2
> 
> is always much closer to the huge number. We're usually quite happy when the 
> estimates are within the same order of magnitude, so whether it's K or K/2 
> makes pretty much no difference.
> 
> I believe that Mark means geometrical average, i.e. sqrt((small number) * 
> (huge number)).

Yes, that is what I proposed upthread.  I'm not wedded to that, though.
In general, I am with Tomas on this one, believing that his estimate
will be much better than the current estimate.  But I believe the *best*
estimate will be somewhere between his and the current one, and I'm
fishing for any decent way of coming up with a weighted average that
is closer to his than to the current, but not simply equal to his proposal.

The reason I want the formula to be closer to Tomas's than to the
current is that I think that on average, across all tables, across all
databases, in practice it will be closer to the right estimate than the
current formula.  That's just my intuition, and so I can't defend it.
But if my intuition is right, the best formula we can adopt would be one
that is moderated from his by a little bit, and in the direction of the
estimate that the current code generates.

I could easily lose this debate merely for lack of a principled basis
for saying how far toward the current estimate the new estimate should
be adjusted.  The geometric average is one suggestion, but I don't have
a principled argument for it.

Like I said above, I'm fishing for a decent formula here.

Mark Dilger

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

Re: [HACKERS] snapshot too old, configured by time

2016-03-03 Thread Kevin Grittner

On Tue, Mar 1, 2016 at 12:58 AM, Michael Paquier
 wrote:
> On Tue, Mar 1, 2016 at 9:35 AM, Andres Freund  wrote:
>> On 2016-02-29 18:30:27 -0600, Kevin Grittner wrote:
>>> Basically, a connection needs to remain open and interleave
>>> commands with other connections, which the isolation tester does
>>> just fine; but it needs to do that using a custom postgresql.conf
>>> file, which TAP does just fine.  I haven't been able to see the
>>> right way to get a TAP test to set up a customized installation to
>>> run isolation tests against.  If I can get that working, I have
>>> additional tests I can drop into that.

>> Check contrib/test_decoding's makefile. It does just that with
>> isolationtester.
>
> pg_isolation_regress --temp-config is the key item here, you can
> enforce a test to run on a server with a wanted configuration set.

Thanks for the tips.  Attached is a minimal set of isolation tests.
I can expand on it if needed, but wanted:

(1) to confirm that this is the right way to do this, and

(2) how long people were willing to tolerate these tests running.

Since we're making this time-based (by popular demand), there must
be delays to see the new behavior.  This very minimal pair of tests
runs in just under one minute on my i7.  Decent coverage of all the
index AMs would probably require tests which run for at least 10
minutes, and probably double that.  I don't recall any satisfactory
resolution to prior discussions about long-running tests.

This is a follow-on patch, just to add isolation testing; the prior
patch must be applied, too.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 6167ec1..9b93552 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -8,6 +8,7 @@ SUBDIRS = \
 		  brin \
 		  commit_ts \
 		  dummy_seclabel \
+		  snapshot_too_old \
 		  test_ddl_deparse \
 		  test_extensions \
 		  test_parser \
diff --git a/src/test/modules/snapshot_too_old/Makefile b/src/test/modules/snapshot_too_old/Makefile
new file mode 100644
index 000..7b9feca
--- /dev/null
+++ b/src/test/modules/snapshot_too_old/Makefile
@@ -0,0 +1,47 @@
+# src/test/modules/snapshot_too_old/Makefile
+
+EXTRA_CLEAN = ./isolation_output
+
+ISOLATIONCHECKS=sto_using_cursor sto_using_select
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/snapshot_too_old
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+# Disabled because these tests require "old_snapshot_threshold" >= 0, which
+# typical installcheck users do not have (e.g. buildfarm clients).
+installcheck:;
+
+# But it can nonetheless be very helpful to run tests on preexisting
+# installation, allow to do so, but only if requested explicitly.
+installcheck-force: isolationcheck-install-force
+
+check: isolationcheck
+
+submake-isolation:
+	$(MAKE) -C $(top_builddir)/src/test/isolation all
+
+submake-test_decoding:
+	$(MAKE) -C $(top_builddir)/src/test/modules/snapshot_too_old
+
+isolationcheck: | submake-isolation temp-install
+	$(MKDIR_P) isolation_output
+	$(pg_isolation_regress_check) \
+	--temp-config $(top_srcdir)/src/test/modules/snapshot_too_old/sto.conf \
+	--outputdir=./isolation_output \
+	$(ISOLATIONCHECKS)
+
+isolationcheck-install-force: all | submake-isolation temp-install
+	$(pg_isolation_regress_installcheck) \
+	$(ISOLATIONCHECKS)
+
+PHONY: check isolationcheck isolationcheck-install-force
+
+temp-install: EXTRA_INSTALL=src/test/modules/snapshot_too_old
diff --git a/src/test/modules/snapshot_too_old/expected/sto_using_cursor.out b/src/test/modules/snapshot_too_old/expected/sto_using_cursor.out
new file mode 100644
index 000..8cc29ec
--- /dev/null
+++ b/src/test/modules/snapshot_too_old/expected/sto_using_cursor.out
@@ -0,0 +1,73 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1decl s1f1 s1sleep s1f2 s2u
+step s1decl: DECLARE cursor1 CURSOR FOR SELECT c FROM sto1;
+step s1f1: FETCH FIRST FROM cursor1;
+c  
+
+1  
+step s1sleep: SELECT setting, pg_sleep(6) FROM pg_settings WHERE name = 'old_snapshot_threshold';
+settingpg_sleep   
+
+0 
+step s1f2: FETCH FIRST FROM cursor1;
+c  
+
+1  
+step s2u: UPDATE sto1 SET c = 1001 WHERE c = 1;
+
+starting permutation: s1decl s1f1 s1sleep s2u s1f2
+step s1decl: DECLARE cursor1 CURSOR FOR SELECT c FROM sto1;
+step s1f1: FETCH FIRST FROM cursor1;
+c  
+
+1  
+step s1sleep: SELECT setting, pg_sleep(6) FROM pg_settings WHERE name = 'old_snapshot_threshold';
+settingpg_sleep   
+
+0 
+step s2u: UPDATE sto1 SET c = 1001 WHERE c = 1;
+step s1f2: FETCH FIRST FROM cursor1;
+ERROR:  snapshot too old

Re: [HACKERS] Support for N synchronous standby servers - take 2

2016-03-03 Thread Thomas Munro

On Fri, Mar 4, 2016 at 7:40 AM, Masahiko Sawada  wrote:
> Previous patch has bug around GUC parameter handling.
> Attached updated version.

I spotted a couple of typos:

+used.  Priority is given to servers in the order that the appear
in the list.

s/the appear/they appear/

-The minimum wait time is the roundtrip time between primary to standby.
+The minimum wait time is the roundtrip time between the primary and the
+almost synchronous standby.

s/almost/slowest/

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


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

Re: [HACKERS] pgbench small bug fix

2016-03-03 Thread Fabien COELHO



Hello Aleksander,

Thanks for the look at the patch.


time pgbench -T 5 -R 0.1 -P 1 -c 2 -j 2


On my laptop this command executes 25 seconds instead of 5.
I'm pretty sure it IS a bug. Probably a minor one though.


Sure.


[...] you should probably write:

if(someint > 0)


Ok.


if(somebool == TRUE)


I like "if (somebool)", the "== TRUE" looks like a tautology, and the 
short version is also the current practice in the project.



Also I suggest to introduce a few new boolean variables with meaningful
names instead of writing all these long expressions right inside of
if( ... ).


I agree about the lisibility, but there are semantics issues to consider:

if (short-A && pretty-long-B)

If short-A is false then pretty-long-B is not evaluated, which is a win 
because it also costs, I try to order conditions... If I move 
pretty-long-B before then the cost is always paid. Now I could write:


if (short-A) {
  bool b = pretty-long-B;
  if (b) {
...

But this looks contrived and people would raise other questions about such
a strange construct for implementing && in 3 lines, 2 if and 1 variable...


As a side note I noticed that pgbench.c is not pgindent'ed. Since you
are modifying this file anyway probably you cold solve this issue too?
As a separate patch perhaps.


As Robert said, not the purpose of this patch.

Attached is a v3 which test integers more logically. I'm a lazy programmer 
who tends to minimize the number of key strokes.


--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 66cfdc9..82707ed 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -1362,6 +1362,12 @@ top:
 		thread->throttle_trigger += wait;
 		st->txn_scheduled = thread->throttle_trigger;
 
+		/* stop client if next transaction is beyond pgbench end of execution */
+		if (duration > 0 &&
+			st->txn_scheduled >
+			INSTR_TIME_GET_MICROSEC(thread->start_time) + (int64) 100 * duration)
+			return false;
+
 		/*
 		 * If this --latency-limit is used, and this slot is already late so
 		 * that the transaction will miss the latency limit even if it
@@ -3724,7 +3730,10 @@ threadRun(void *arg)
 		}
 	}
 
-	while (remains > 0)
+	while (remains > 0 ||
+		   /* thread zero keeps on printing progress report if any */
+		   (progress > 0 && thread->tid == 0 && duration > 0 &&
+			next_report <= thread_start + (int64) duration * 100))
 	{
 		fd_set		input_mask;
 		int			maxsock;	/* max socket number to be waited */

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

Re: [HACKERS] improving GROUP BY estimation

2016-03-03 Thread Alexander Korotkov

On Thu, Mar 3, 2016 at 10:16 PM, Tomas Vondra 
wrote:

> So yes, each estimator works great for exactly the opposite cases. But
> notice that typically, the results of the new formula is much higher than
> the old one, sometimes by two orders of magnitude (and it shouldn't be
> difficult to construct examples of much higher differences).
>
> The table also includes the 'average' estimator you propose, but it's
> rather obvious that the result is always much closer to the new value,
> simply because
>
>(small number) + (huge number)
>--
>   2
>
> is always much closer to the huge number. We're usually quite happy when
> the estimates are within the same order of magnitude, so whether it's K or
> K/2 makes pretty much no difference.


I believe that Mark means geometrical average, i.e. sqrt((small number) *
(huge number)).

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

Re: [HACKERS][PATCH] Supporting +-Infinity values by to_timestamp(float8)

2016-03-03 Thread Vitaly Burovoy

On 2/26/16, Vitaly Burovoy  wrote:
> Proposed patch implements it.

I'm sorry, I forgot to leave a note for reviewers and committers:

This patch requires CATALOG_VERSION_NO be bumped.

Since pg_proc.h entry has changed, it is important to check and run
regress tests on a new cluster (as if CATALOG_VERSION_NO was bumped).

-- 
Best regards,
Vitaly Burovoy


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

Re: [HACKERS] WIP: Upper planner pathification

Robert Haas  writes:
> Thanks for working on this.  Some review comments:

> - I think all of the new path creation functions should bubble up
> parallel_degree from their subpath.

Ah, thanks, I didn't have any clue how to set that (though in my defense,
the documentation about it is next to nonexistent).  Just to confirm,
I assume the rules are:

* parallel_aware: indicates whether the plan node itself has any
parallelism behavior

* parallel_safe: indicates that the entire plan tree rooted at this
node is safe to execute in a parallel worker

* parallel_degree: indicates number of parallel threads potentially
useful for this plan tree (0 if not parallel-safe)

This leads me to the conclusion that all these new node types should
set parallel_aware to false and copy up the other two fields from the
child, except for LockRows and ModifyTable which should set them all
to false/0.  Correct?  If so, I'll go fix.

> - RollupPath seems like a poor choice of name, if nothing else.  You
> would expect that it would be related to GROUP BY ROLLUP() but of
> course that's really the same thing as GROUP BY GROUPING SETS () or
> GROUP BY CUBE (), and the fundamental operation is actually GROUPING
> SETS, not ROLLUP.

As I noted to David, that thing seems to me to be in need of refactoring,
but I'd just as soon leave untangling the grouping-sets mess for later.
I don't mind substituting a different name if you have a better idea,
but don't really want to do more work than that right now.

> - It's not entirely related to this patch, but I'm starting to wonder
> if we've made the wrong bet about target lists.  It seems to me that
> there's a huge difference between a projection which simply throws
> away columns we don't need and one which actually computes something,
> and maybe those cases ought to be treated differently instead of
> saying "well, it's a target list".  It strikes me that there are
> probably execution-time optimizations that are possible in the former
> case, and maybe a more compact representation of the projection
> operation as well.  I can't shake the feeling that our extensive use
> of lists can't be the best thing ever for performance.

We do already have the "physical tlist" optimization.  I agree that
there's more work to be done here, but again would rather leave that
to a later patch.

> - A related point that is more connected to this patch is that you've
> added 13 (!) new calls to disuse_physical_tlist, and 8 of those are
> marked with /* XXX hack: need original tlist with sortgroupref marking
> */.  I don't quite understand what's going on there.  I think if we're
> going to be stuck with that hack we at least need some comments
> explaining what is going on there.  What has caused us to suddenly
> need these calls when we didn't before, and why these places and not
> some others?

Yeah, that's a hack to get things working.  The problem is that these node
types need to be able to identify sort/group columns in their inputs, but
if the child has switched to a "physical tlist" then the ressortgroupref
marking isn't there, and indeed the needed column might not be there at
all if it's a computed expression not a Var.  So what I did for the moment
was to force the inputs back to their nominal tlists.  In the old code we
didn't have this problem because none of the upper-level plan node types
could see a physical tlist unless make_subplanTargetList had allowed it,
and then we applied locate_grouping_columns() to re-identify the grouping
columns.  That logic probably needs to be transposed into createplan.c,
but I've not taken the time yet to figure out exactly how.  I don't know
if it's reasonable to do that separately from rethinking how the whole
disuse_physical_tlist thing works.

> - For SortPath, you mention that the SortGroupClauses representation
> isn't currently used.  It's not clear to me that it ever would be;
> what would be the case where that might be useful?  At any rate, I'd
> be inclined to rip it out for now; you can always put it back later.

Yeah, I was dithering about that.  It seems like createplan.c now has
a few too many ways to identify sort/group columns, and I was hoping
to consolidate them somehow.  That might lead to wanting to use
SortGroupClauses not PathKeys in some cases.  But until that's worked
out, I agree the extra field is useless and we can just drop it.

> - create_distinct_paths() disables the hash path if it seems like it
> would exceed work_mem, unless the sort path isn't viable.  But there's
> something that feels a bit uncomfortable about this.  Suppose the sort
> path isn't viable but some other kind of future path is viable.  It
> seems like it would be better to restructure this a bit so that the
> decision about whether to add the hash path is based on whether there
> are any other paths in the rel when we reach the bottom of the
> function.  create_grouping_paths() has  a similar issue.

OK, I'll take a look.  Quite a

Re: [HACKERS] improving GROUP BY estimation

2016-03-03 Thread Tomas Vondra


Hi,

On 03/03/2016 06:40 PM, Mark Dilger wrote:



On Mar 3, 2016, at 8:35 AM, Tomas Vondra  wrote:

Hi,

On 03/03/2016 12:53 PM, Alexander Korotkov wrote:

Hi, Tomas!

I've assigned to review this patch.

I've checked version estimate-num-groups-v2.txt by Mark Dilger.
It applies to head cleanly, passes corrected regression tests.

About correlated/uncorrelated cases. I think now optimizer mostly assumes
all distributions to be independent. I think we should follow this
assumption in this case also until we have fundamentally better option (like
your multivariate statistics).

@@ -3438,9 +3438,9 @@ estimate_num_groups(PlannerInfo *root, List *groupExprs,
double input_rows,
reldistinct = clamp;

/*
-* Multiply by restriction selectivity.
+* Estimate number of distinct values expected in given number of rows.
*/
-reldistinct *= rel->rows / rel->tuples;
+reldistinct *= (1 - powl((reldistinct - 1) / reldistinct, rel->rows));

/*
* Update estimate of total distinct groups.

I think we need way more explanation in comments here (possibly with
external links). For now, it looks like formula which appears from nowhere.


I thought the details (particularly the link to stackexchange, discussing
the formula) would be enough, but let me elaborate.

The current formula

reldistinct *= rel->rows / rel->tuples;

simply multiplies the ndistinct estimate with selectivity. So when you
select 1% of the table, the estimate says we'll see 1% of ndistinct
values. But clearly that's naive, because for example when you have 10k
distinct values and you select 10M rows, you should expect nearly all of
them in the sample.


The current formula assumes total dependence between the columns.  You can
create tables where this is true, and the formula gives precisely the right
estimate.  I'm not claiming this is common, or that it should be the default
assumption, but it is one end of the spectrum of possible correct
estimates.


I'm not really sure what you mean by dependence between columns, as both the 
old and new formula only works with total cardinality and selectivity, and has 
absolutely no idea about multiple columns.


In case you mean dependence between columns in the GROUP BY and columns used to 
compute the selectivity (i.e. referenced in WHERE), then perhaps you could say 
it like that, although I think there's no such explicit assumption.





And that's what the formula does - it gives you the expected number of
distinct values, when selecting 'k' values from 'd' distinct values with
replacements:

count(k, d) = d * (1 - ((d-1)/d) ^ k)

It's assuming the distinct values are equally probable (uniform
distribution) and that the probabilities do not change (that's the "with
replacement").


The new formula assumes total independence between the columns. You can
likewise create tables where this is true, and you did so upthread, and for
those tables it gives precisely the right estimate. This is the other end of
the spectrum of possible correct estimates.

In the absence of multivariate statistics, either because you haven't
committed that work yet, or because the multivariate data the planner needs
hasn't been collected, choosing an estimate exactly in the middle of the
spectrum (whatever that would mean mathematically) would minimize the
maximum possible error in the estimate.


FWIW the current version of multivariate statistics can't really help in this 
case. Perhaps it will help in the future, but it's way more complicated that it 
might seem as it requires transferring information from the WHERE clause to the 
GROUP BY clause.




I have the sense that my point of view is in the minority, because the
expectation is the problems caused by independence assumptions will only be
fixed when multivariate statistics are implemented and available, and so we
should just keep the independence assumptions everywhere until that time. I
 tend to disagree with that, on the grounds that even when that work is
finished, we'll never have complete coverage of every possible set of
columns and what their degree of dependence is.


Well, in a sense you're right that those estimates work best in different 
situations. The problem with constructing an estimator the way you propose 
(simply returning an average) is that in both the extreme cases (perfect 
dependence or independence) one of those estimates is really bad. Moreover the 
new formula typically produces higher values than the old one,


Consider for example this:

CREATE TABLE t AS SELECT (1 * random())::int a,
 (1 * random())::int b
FROM generate_series(1,100) s(i);

and let's see estimates for queries like this:

SELECT a FROM t WHERE b < $1 GROUP BY a;

Then for different values of $1 we get this:

   |  10 |   50 |  100 |  500 |  1000 |  5000
   
actual | 919 | 3829 | 6244 | 9944 | 10001 |

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

2016-03-03 Thread Andres Freund

Hi,

> +/*
> + * rename_safe -- rename of a file, making it on-disk persistent
> + *
> + * This routine ensures that a rename file persists in case of a crash by 
> using
> + * fsync on the old and new files before and after performing the rename so 
> as
> + * this categorizes as an all-or-nothing operation.
> + */
> +int
> +rename_safe(const char *oldfile, const char *newfile)
> +{
> + struct stat filestats;
> +
> + /*
> +  * First fsync the old entry and new entry, it this one exists, to 
> ensure
> +  * that they are properly persistent on disk. Calling this routine with
> +  * an existing new target file is fine, rename() will first remove it
> +  * before performing its operation.
> +  */
> + fsync_fname(oldfile, false);
> + if (stat(newfile, ) == 0)
> + fsync_fname(newfile, false);

I don't think we want any stat()s here. I'd much, much rather check open
for ENOENT.


> +/*
> + * link_safe -- make a file hard link, making it on-disk persistent
> + *
> + * This routine ensures that a hard link created on a file persists on system
> + * in case of a crash by using fsync where on the link generated as well as 
> on
> + * its parent directory.
> + */
> +int
> +link_safe(const char *oldfile, const char *newfile)
> +{

If we go for a new abstraction here, I'd rather make it
'replace_file_safe' or something, and move the link/rename code #ifdef
into it.


> + if (link(oldfile, newfile) < 0)
> + return -1;
> +
> + /*
> +  * Make the link persistent in case of an OS crash, the new entry
> +  * generated as well as its parent directory need to be flushed.
> +  */
> + fsync_fname(newfile, false);
> +
> + /*
> +  * Same for parent directory. This routine is never called to rename
> +  * files across directories, but if this proves to become the case,
> +  * flushing the parent directory if the old file would be necessary.
> +  */
> + fsync_parent_path(newfile);
> + return 0;

I think it's a seriously bad idea to encode that knowledge in such a
general sounding routine.  We could however argue that this is about
safely replacing the *target* file; not about safely removing the old
file.


Currently I'm inclined to apply this to master soon. But I think we
might want to wait a while with backpatching.  The recent fsync upgrade
disaster kinda makes me a bit careful...


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] improving GROUP BY estimation

2016-03-03 Thread Alexander Korotkov

On Thu, Mar 3, 2016 at 7:35 PM, Tomas Vondra 
wrote:

> On 03/03/2016 12:53 PM, Alexander Korotkov wrote:
>
>> I've assigned to review this patch.
>>
>> I've checked version estimate-num-groups-v2.txt by Mark Dilger.
>> It applies to head cleanly, passes corrected regression tests.
>>
>> About correlated/uncorrelated cases. I think now optimizer mostly assumes
>> all distributions to be independent. I think we should follow this
>> assumption in this case also until we have fundamentally better option
>> (like
>> your multivariate statistics).
>>
>> @@ -3438,9 +3438,9 @@ estimate_num_groups(PlannerInfo *root, List
>> *groupExprs,
>> double input_rows,
>> reldistinct = clamp;
>>
>> /*
>> -* Multiply by restriction selectivity.
>> +* Estimate number of distinct values expected in given number of rows.
>> */
>> -reldistinct *= rel->rows / rel->tuples;
>> +reldistinct *= (1 - powl((reldistinct - 1) / reldistinct, rel->rows));
>>
>> /*
>> * Update estimate of total distinct groups.
>>
>> I think we need way more explanation in comments here (possibly with
>> external links). For now, it looks like formula which appears from
>> nowhere.
>>
>
> I thought the details (particularly the link to stackexchange, discussing
> the formula) would be enough, but let me elaborate.
>
> The current formula
>
> reldistinct *= rel->rows / rel->tuples;
>
> simply multiplies the ndistinct estimate with selectivity. So when you
> select 1% of the table, the estimate says we'll see 1% of ndistinct values.
> But clearly that's naive, because for example when you have 10k distinct
> values and you select 10M rows, you should expect nearly all of them in the
> sample.
>
> And that's what the formula does - it gives you the expected number of
> distinct values, when selecting 'k' values from 'd' distinct values with
> replacements:
>
> count(k, d) = d * (1 - ((d-1)/d) ^ k)
>
> It's assuming the distinct values are equally probable (uniform
> distribution) and that the probabilities do not change (that's the "with
> replacement").
>
> I guess we could relax those assumptions and for example use the MCV
> statistics to further improve the estimate, and also relax the 'with
> replacement' but that'd make the formula way more complicated.
>
> [1]
> http://math.stackexchange.com/questions/72223/finding-expected-number-of-distinct-values-selected-from-a-set-of-integers


Your explanation in the first mail was good enough. Not it's even better.
But actually I mean that we need to include some brief explanation into
source code (either comments or readme). It would be good if one who want
to understand this could do without searching mailing list archives or git
history.


>> Also, note that rel->tuples gone away from formula parameters. So, we
>> actually don't care about how may tuples are in the table. This is because
>> this formula assumes that same tuple could be selected multiple times. For
>> low numbers this can lead to some errors. Consider selecting 2 from 3
>> distinct tuples. If you can't select same tuple twice then you always
>> selecting 2 distinct tuples. But according to formula you will select 5/3
>> in
>> average. I think this error in small numbers is negligible, especially
>> because getting rid of this error would require way more computations. But
>> it worth mentioning in comments though.
>>
>
> Well, yeah. That's the consequence of 'with replacement' assumption.
>
> I guess we could handle this somehow, though. For example we can compute
> the minimum possible number of distinct values like this:
>
> average_rows_per_value = (tuples / ndistinct);
> min_estimate = ceil(rows / average_rows_per_value);
>
> and use that as a minimum for the estimate. In the example you've given
> this would say
>
> average_rows_per_value = (3 / 3) = 1;
> min_estimate = ceil(2 / 1) = 2;
>
> So it serves as a correction for the 'with replacement' assumption. With
> only 2 distinct values we'd get this:
>
> average_rows_per_value = (3 / 2) = 1.5;
> min_estimate = ceil(2 / 1.5) = 2;
>
> Of course, it's just a heuristics, so this may fail in some other cases.


I'm not sure we actually need it. Even in worst case error doesn't seems to
be very big. But feel free to add this heuristics, it looks OK for me.

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

Re: [HACKERS] Support for N synchronous standby servers - take 2

2016-03-03 Thread Masahiko Sawada

On Thu, Mar 3, 2016 at 11:30 PM, Masahiko Sawada  wrote:
> Hi,
>
> Thank you so much for reviewing this patch!
>
> All review comments regarding document and comment are fixed.
> Attached latest v14 patch.
>
>> This accepts 'abc^Id' as a name, which is wrong behavior (but
>> such appliction names are not allowed anyway. If you assume so,
>> I'd like to see a comment for that.).
>
> 'abc^Id' is accepted as application_name, no?
> postgres(1)=# set application_name to 'abc^Id';
> SET
> postgres(1)=# show application_name ;
>  application_name
> --
>  abc^Id
> (1 row)
>
>> addlit_xd_string(char *ytext) and addlitchar_xd_string(unsigned
>> char ychar) requires differnt character types. Is there any reason
>> for that?
>
> Because addlit_xd_string() is for adding string(char *) to xd_string,
> OTOH addlit_xd_char() is for adding just one character to xd_string.
>
>> I personally don't like addlit*string() things for such simple
>> syntax but itself is acceptble enough for me. However it uses
>> StringInfo to hold double-quoted names, which pallocs 1024 bytes
>> of memory chunk for every double-quoted name. The chunks are
>> finally stacked up left uncollected until the current
>> memorycontext is deleted or reset (It is deleted just after
>> finishing config file processing). Addition to that, setting
>> s_s_names runs the parser twice. It seems to me too greedy and
>> seems that static char [NAMEDATALEN] is enough using the v12 way
>> without palloc/repalloc.
>
> I though that length of group name could be more than NAMEDATALEN, so
> I use StringInfo.
> Is it not necessary?
>
>> I found that the name SyncGroupName.wait_num is not
>> instinctive. How about sync_num, sync_member_num or
>> sync_standby_num? If the last is preferable, .members also should
>> be .standbys .
>
> Thanks, sync_num is preferable to me.
>
> ===
>> I am quite uncomfortable with the existence of
>> WanSnd.sync_standby_priority. It represented the pirority in the
>> old linear s_s_names format but nested groups or even
>> single-level quarum list obviously doesn't fit it. Can we get rid
>> of sync_standby_priority, even though we realize atmost
>> n-priority for now?
>
> We could get rid of sync_standby_priority.
> But if so, we will not be able to see the next sync standby in
> pg_stat_replication system view.
> Regarding each node priority, I was thinking that standbys in quorum
> list have same priority, and in nested group each standbys are given
> the priority starting from 1.
>
> ===
>> The function SyncRepGetSyncedLsnsUsingPriority doesn't seem to
>> have specific code for every prioritizing method (which are
>> priority, quorum, nested and so). Is there any reson to use it as
>> a callback of SyncGroupNode?
>
> The reason why the current code is so is that current code is for only
> priority method supporting.
> At first version of this feature, I'd like to implement it more simple.
>
> Aside from this, of course I'm planning to have specific code for nested 
> design.
> - The group can have some name nodes or group nodes.
> - The group can use either 2 types of method: priority or quorum.
> - The group has SyncRepGetSyncedLsnFn() and SyncRepGetStandbysFn()
>   - SyncRepGetSyncedLsnsFn() function recursively determine synced LSN
> at that moment using group's method.
>   - SyncRepGetStandbysFn() function returns standbys of its group,
> which are considered as sync using group's method.
>
> For example, s_s_name  = '3(a, b, 2[c,d]::group1)', SyncRepStandbys
> memory structure will be,
>
> "main(quorum)" --- "a"
> |
> -- "b"
> |
> -- "group1(priority)" --- "c"
>  |
>  -- "d"
>
> When determine synced LSNs, we need to consider group1's LSN using by
> priority method at first, and then we can determine main's LSN using
> by quorum method with "a" LSNs, "b" LSNs and "group1" LSNs.
> So SyncRepGetSyncedLsnsUsingPriority() function would be,
>
> bool
> SyncRepGetSyncedLsnsUsingPriority(*group, *write_lsn, *flush_lsn)
> {
> sync_num = group->SynRepGetSyncstandbysFn(group, sync_list);
>
> if (sync_num < group->sync_num)
> return false;
>
> for (each member of sync_list)
> {
> if (member->type == group node)
> call SyncRepGetSyncedLsnsFn(member, w, f) and store w and
> f into lsn_list.
> else
> Store name node LSNs into lsn_list.
> }
>
> Determine synced LSNs of this group using lsn_list and priority method.
> Store synced LSNs into write_lsn and flush_lsn.
> return true;
> }
>
>> SyncRepClearStandbyGroupList is defined in syncrep.c but the
>> other related functions are defined in syncgroup_gram.y. It would
>> be better to place them together.
>
> SyncRepClearStandbyGroupList() is used by
> check_synchronous_standby_names(),

Re: [HACKERS] pgbench small bug fix

On Thu, Mar 3, 2016 at 7:23 AM, Aleksander Alekseev
 wrote:
>> time pgbench -T 5 -R 0.1 -P 1 -c 2 -j 2
>
> On my laptop this command executes 25 seconds instead of 5. I'm pretty
> sure it IS a bug. Probably a minor one though.
>
> I tested this patch on Ubuntu 14.04 LTS with GCC 4.8. It applies
> cleanly on master branch (c7111d11) and solves a described problem.
> No compilation warnings. Everything else works as before.
>
> Still I wonder if code could be patched more cleanly. Instead of:
>
> if(someint)
> if(somebool)
>
> ... you should probably write:
>
> if(someint > 0)
> if(somebool == TRUE)

I think our usual style is to test Booleans directly; that is if
(somebool).  But for other types, we typically include an explicit
comparison, like if (someint != 0) or if (someint > 0).

> As a side note I noticed that pgbench.c is not pgindent'ed. Since you
> are modifying this file anyway probably you cold solve this issue too?
> As a separate patch perhaps.

That's not really this patch's job.

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


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

Re: [HACKERS] POC: Cache data in GetSnapshotData()

On Thu, Mar 3, 2016 at 6:20 AM, Mithun Cy 
wrote:

>
> On Tue, Mar 1, 2016 at 12:59 PM, Amit Kapila 
> wrote:
> >Don't we need to add this only when the xid of current transaction is
> valid?  Also, I think it will be better if we can explain why we need to
> add the our >own transaction id while caching the snapshot.
> I have fixed the same thing and patch is attached.
>
> Some more tests done after that
>
> *pgbench write tests: on 8 socket, 64 core machine.*
>
> /postgres -c shared_buffers=16GB -N 200 -c min_wal_size=15GB -c
> max_wal_size=20GB -c checkpoint_timeout=900 -c maintenance_work_mem=1GB -c
> checkpoint_completion_target=0.9
>
> ./pgbench -c $clients -j $clients -T 1800 -M prepared postgres
>
> [image: Inline image 3]
>
> A small improvement in performance at 64 thread.
>

What if you apply both this and Amit's clog control log patch(es)?  Maybe
the combination of the two helps substantially more than either one alone.

Or maybe not, but seems worth checking.

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

Re: [HACKERS] WIP: Upper planner pathification

On Sun, Feb 28, 2016 at 3:03 PM, Tom Lane  wrote:
> I'm pretty pleased with the way this turned out.  grouping_planner() is
> about half the length it was before, and much more straightforward IMO.
> planagg.c no longer seems like a complete hack; it's a reasonable
> prototype for injecting nontraditional implementation paths into
> aggregation or other late planner stages, and grouping_planner() doesn't
> need to know about it.

Thanks for working on this.  Some review comments:

- I think all of the new path creation functions should bubble up
parallel_degree from their subpath.  It's fine for LockRows to use 0
for now, because we have no hope of supporting write operations in
parallel mode any time soon, but it's one less thing to change later.
The others should really all use the subpath's parallel degree.
Actually, they should use the subpath's parallel degree or maybe a
larger number if they add a lot of CPU work to the query, but don't
have any principled way to model that right now, so just copying the
value is probably as good as we're going to do for the moment.

- RollupPath seems like a poor choice of name, if nothing else.  You
would expect that it would be related to GROUP BY ROLLUP() but of
course that's really the same thing as GROUP BY GROUPING SETS () or
GROUP BY CUBE (), and the fundamental operation is actually GROUPING
SETS, not ROLLUP.

- It's not entirely related to this patch, but I'm starting to wonder
if we've made the wrong bet about target lists.  It seems to me that
there's a huge difference between a projection which simply throws
away columns we don't need and one which actually computes something,
and maybe those cases ought to be treated differently instead of
saying "well, it's a target list".  It strikes me that there are
probably execution-time optimizations that are possible in the former
case, and maybe a more compact representation of the projection
operation as well.  I can't shake the feeling that our extensive use
of lists can't be the best thing ever for performance.

- A related point that is more connected to this patch is that you've
added 13 (!) new calls to disuse_physical_tlist, and 8 of those are
marked with /* XXX hack: need original tlist with sortgroupref marking
*/.  I don't quite understand what's going on there.  I think if we're
going to be stuck with that hack we at least need some comments
explaining what is going on there.  What has caused us to suddenly
need these calls when we didn't before, and why these places and not
some others?

- For SortPath, you mention that the SortGroupClauses representation
isn't currently used.  It's not clear to me that it ever would be;
what would be the case where that might be useful?  At any rate, I'd
be inclined to rip it out for now; you can always put it back later.

- create_distinct_paths() disables the hash path if it seems like it
would exceed work_mem, unless the sort path isn't viable.  But there's
something that feels a bit uncomfortable about this.  Suppose the sort
path isn't viable but some other kind of future path is viable.  It
seems like it would be better to restructure this a bit so that the
decision about whether to add the hash path is based on whether there
are any other paths in the rel when we reach the bottom of the
function.  create_grouping_paths() has  a similar issue.

In general, and I'm sure this is not a huge surprise, most of this
looks very good to me.  I think the design is sound and that, if the
performance is OK, we ought to move forward with it.

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


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

Re: [HACKERS] pg_basebackup compression TODO item

2016-03-03 Thread Andres Freund

On 2016-03-03 18:44:24 +0100, Magnus Hagander wrote:
> On Thu, Mar 3, 2016 at 6:34 PM, Andres Freund  wrote:
> 
> > On 2016-03-03 18:31:03 +0100, Magnus Hagander wrote:
> > > I think we want it at protocol level rather than pg_basebackup level.
> >
> > I think we may want both eventually, but I do agree that protocol level
> > has a lot higher "priority" than that. Something like protocol level
> > compression has a bit of different tradeofs than compressing base
> > backups, and it's nice not to compress, uncompress, compress again.

> Yeah, good point, we definitely want both. Based on the field experience
> I've had (which might differ from others), having it protocol level would
> help more people tough, so should be higher prio.

Agreed. But then our priorities are not necessary the implementers, and
I don't think there's strong enough architectural reasons to only accept
protocol level for now...

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] pg_basebackup compression TODO item

2016-03-03 Thread Joshua D. Drake


On 03/03/2016 09:34 AM, Andres Freund wrote:

On 2016-03-03 18:31:03 +0100, Magnus Hagander wrote:

I think we want it at protocol level rather than pg_basebackup level.


I think we may want both eventually, but I do agree that protocol level
has a lot higher "priority" than that. Something like protocol level
compression has a bit of different tradeofs than compressing base
backups, and it's nice not to compress, uncompress, compress again.


Agreed. This is something that we have neglected and argued against for 
no good reason for over a decade. It is time to get it done.


Sincerely,

JD


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


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

Re: [HACKERS] pg_basebackup compression TODO item

On Thu, Mar 3, 2016 at 6:34 PM, Andres Freund  wrote:

> On 2016-03-03 18:31:03 +0100, Magnus Hagander wrote:
> > I think we want it at protocol level rather than pg_basebackup level.
>
> I think we may want both eventually, but I do agree that protocol level
> has a lot higher "priority" than that. Something like protocol level
> compression has a bit of different tradeofs than compressing base
> backups, and it's nice not to compress, uncompress, compress again.
>

Yeah, good point, we definitely want both. Based on the field experience
I've had (which might differ from others), having it protocol level would
help more people tough, so should be higher prio.

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

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

On Thu, Mar 3, 2016 at 6:18 PM, Andrew Dunstan  wrote:

>
>
> On 03/03/2016 09:02 AM, Michael Paquier wrote:
>
>> Hi all,
>>
>> Microsoft provides a set of VMs that one can use for testing and
>> Windows 10 is in the set:
>> https://dev.windows.com/en-us/microsoft-edge/tools/vms/windows/
>> I have grabbed one and installed the community version of Visual
>> Studio 2015 so I think that I am going to be able to compile Postgres
>> with VS2015 with a bit of black magic.
>>
>> My goal is double:
>> 1) As far as things have been discussed, VS2015 is making difficult
>> the compilation of Postgres, particularly for locales. So I'd like to
>> see what are the problems behind that and see if we can patch it
>> properly. This would facilitate the integration of cmake as well for
>> Windows.
>> 2) src/tools/msvc stuff has support only up to VS2013. I think that it
>> would be nice to bump that a bit and get something for 9.5~.
>>
>> So, would there be interest for a patch on the perl scripts in
>> src/tools/msvc or are they considered a lost cause? Having a look at
>> the failures could be done with the cmake work, but it seems a bit
>> premature to me to look at that for the moment, and having support for
>> VS2015 with 9.5 (support for new versions of VS won a backpatch the
>> last couple of years) would be a good thing I think.
>>
>
>
>
> I am not holding my breath on cmake. Until we have something pretty solid
> on that front I'm going to assume it's not happening. If we're going to
> support VS2015 (and I think we should) then it should be supported for all
> live branches if possible. I'm assuming the changes would be pretty
> localized, at least in src/tools/msvc, and adding a new compile shouldn't
> break anything with existing compilers.
>
>
+1.

Definitely do it for HEAD.

Then if it gets backpatched is going to depend on the locality I think. If
it's just the build system then it should be no problem, but I thought
Michael also suggested some API changes. If that's so, then it is going to
depend on how invasive those are. But that part should be done for HEAD
regardless, so it's definitely worth the effort to figure out exactly what
it involves.


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

Re: [HACKERS] pg_basebackup compression TODO item

2016-03-03 Thread Stephen Frost

* Andres Freund (and...@anarazel.de) wrote:
> On 2016-03-03 18:31:03 +0100, Magnus Hagander wrote:
> > I think we want it at protocol level rather than pg_basebackup level.
> 
> I think we may want both eventually, but I do agree that protocol level
> has a lot higher "priority" than that. Something like protocol level
> compression has a bit of different tradeofs than compressing base
> backups, and it's nice not to compress, uncompress, compress again.

+1, the whole compress-uncompress-compress thing was why I was trying to
add support to COPY to do zlib compression, which could have then been
used to compress server-side and then just write the results out to a
file for -Fc/-Fd style dumps.  We ended up implementing the 'PROGRAM'
thing for COPY, which is nice, but isn't the same.

> > If SSL compression is busted on base backups, it's equally busted on
> > regular connection and replication streams. People do ask for
> > compression on that (in particular I've had a lot of requests when it
> > comes to replication), and our response there has traditionally been
> > "ssl compression"...
> 
> Agreed. I think our answer there was always a bit of a cop out...

Agreed on this also.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] improving GROUP BY estimation

2016-03-03 Thread Mark Dilger

> On Mar 3, 2016, at 8:35 AM, Tomas Vondra  wrote:
> 
> Hi,
> 
> On 03/03/2016 12:53 PM, Alexander Korotkov wrote:
>> Hi, Tomas!
>> 
>> I've assigned to review this patch.
>> 
>> I've checked version estimate-num-groups-v2.txt by Mark Dilger.
>> It applies to head cleanly, passes corrected regression tests.
>> 
>> About correlated/uncorrelated cases. I think now optimizer mostly assumes
>> all distributions to be independent. I think we should follow this
>> assumption in this case also until we have fundamentally better option (like
>> your multivariate statistics).
>> 
>> @@ -3438,9 +3438,9 @@ estimate_num_groups(PlannerInfo *root, List 
>> *groupExprs,
>> double input_rows,
>> reldistinct = clamp;
>> 
>> /*
>> -* Multiply by restriction selectivity.
>> +* Estimate number of distinct values expected in given number of rows.
>> */
>> -reldistinct *= rel->rows / rel->tuples;
>> +reldistinct *= (1 - powl((reldistinct - 1) / reldistinct, rel->rows));
>> 
>> /*
>> * Update estimate of total distinct groups.
>> 
>> I think we need way more explanation in comments here (possibly with
>> external links). For now, it looks like formula which appears from nowhere.
> 
> I thought the details (particularly the link to stackexchange, discussing the 
> formula) would be enough, but let me elaborate.
> 
> The current formula
> 
>reldistinct *= rel->rows / rel->tuples;
> 
> simply multiplies the ndistinct estimate with selectivity. So when you select 
> 1% of the table, the estimate says we'll see 1% of ndistinct values. But 
> clearly that's naive, because for example when you have 10k distinct values 
> and you select 10M rows, you should expect nearly all of them in the sample.

The current formula assumes total dependence between the
columns.  You can create tables where this is true, and the
formula gives precisely the right estimate.  I'm not claiming this
is common, or that it should be the default assumption, but it
is one end of the spectrum of possible correct estimates.

> And that's what the formula does - it gives you the expected number of 
> distinct values, when selecting 'k' values from 'd' distinct values with 
> replacements:
> 
>count(k, d) = d * (1 - ((d-1)/d) ^ k)
> 
> It's assuming the distinct values are equally probable (uniform distribution) 
> and that the probabilities do not change (that's the "with replacement").

The new formula assumes total independence between the
columns.  You can likewise create tables where this is true,
and you did so upthread, and for those tables it gives precisely
the right estimate.  This is the other end of the spectrum of
possible correct estimates.

In the absence of multivariate statistics, either because you
haven't committed that work yet, or because the multivariate
data the planner needs hasn't been collected, choosing an
estimate exactly in the middle of the spectrum (whatever
that would mean mathematically) would minimize the
maximum possible error in the estimate.

I have the sense that my point of view is in the minority, because
the expectation is the problems caused by independence
assumptions will only be fixed when multivariate statistics are
implemented and available, and so we should just keep the
independence assumptions everywhere until that time.  I
tend to disagree with that, on the grounds that even when that
work is finished, we'll never have complete coverage of every
possible set of columns and what their degree of dependence
is.

Perhaps I am badly mistaken.

Can somebody more familiar with the issues than I am please
disabuse me of my wrongheadedness?

Mark Dilger

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

Re: [HACKERS] pg_basebackup compression TODO item

2016-03-03 Thread Joshua D. Drake


On 03/03/2016 09:23 AM, Jeff Janes wrote:

Since SSL compression seems to be a busted flush, I would like to see
pg_basebackup be able to do compression on the server end, not just
the client end, in order to spare network bandwidth.

Any comments on how hard this would be, or why we don't want it?


We want it and we want it over multiple workers.

JD



Cheers,

Jeff





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


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

Re: [HACKERS] pg_basebackup compression TODO item

2016-03-03 Thread Andres Freund

On 2016-03-03 18:31:03 +0100, Magnus Hagander wrote:
> I think we want it at protocol level rather than pg_basebackup level.

I think we may want both eventually, but I do agree that protocol level
has a lot higher "priority" than that. Something like protocol level
compression has a bit of different tradeofs than compressing base
backups, and it's nice not to compress, uncompress, compress again.

> If SSL compression is busted on base backups, it's equally busted on
> regular connection and replication streams. People do ask for
> compression on that (in particular I've had a lot of requests when it
> comes to replication), and our response there has traditionally been
> "ssl compression"...

Agreed. I think our answer there was always a bit of a cop out...

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] pg_basebackup compression TODO item

On Thu, Mar 3, 2016 at 6:23 PM, Jeff Janes  wrote:

> Since SSL compression seems to be a busted flush, I would like to see
> pg_basebackup be able to do compression on the server end, not just
> the client end, in order to spare network bandwidth.
>
> Any comments on how hard this would be, or why we don't want it?
>

I think we want it at protocol level rather than pg_basebackup level. If
SSL compression is busted on base backups, it's equally busted on regular
connection and replication streams. People do ask for compression on that
(in particular I've had a lot of requests when it comes to replication),
and our response there has traditionally been "ssl compression"...

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

[HACKERS] pg_basebackup compression TODO item

2016-03-03 Thread Jeff Janes

Since SSL compression seems to be a busted flush, I would like to see
pg_basebackup be able to do compression on the server end, not just
the client end, in order to spare network bandwidth.

Any comments on how hard this would be, or why we don't want it?

Cheers,

Jeff


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

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

2016-03-03 Thread Andrew Dunstan




On 03/03/2016 09:02 AM, Michael Paquier wrote:

Hi all,

Microsoft provides a set of VMs that one can use for testing and
Windows 10 is in the set:
https://dev.windows.com/en-us/microsoft-edge/tools/vms/windows/
I have grabbed one and installed the community version of Visual
Studio 2015 so I think that I am going to be able to compile Postgres
with VS2015 with a bit of black magic.

My goal is double:
1) As far as things have been discussed, VS2015 is making difficult
the compilation of Postgres, particularly for locales. So I'd like to
see what are the problems behind that and see if we can patch it
properly. This would facilitate the integration of cmake as well for
Windows.
2) src/tools/msvc stuff has support only up to VS2013. I think that it
would be nice to bump that a bit and get something for 9.5~.

So, would there be interest for a patch on the perl scripts in
src/tools/msvc or are they considered a lost cause? Having a look at
the failures could be done with the cmake work, but it seems a bit
premature to me to look at that for the moment, and having support for
VS2015 with 9.5 (support for new versions of VS won a backpatch the
last couple of years) would be a good thing I think.




I am not holding my breath on cmake. Until we have something pretty 
solid on that front I'm going to assume it's not happening. If we're 
going to support VS2015 (and I think we should) then it should be 
supported for all live branches if possible. I'm assuming the changes 
would be pretty localized, at least in src/tools/msvc, and adding a new 
compile shouldn't break anything with existing compilers.


cheers

andrew


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

Re: [HACKERS] pg_dump / copy bugs with "big lines" ?

2016-03-03 Thread Daniel Verite

Tom Lane wrote:

> BTW, is anyone checking the other side of this, ie "COPY IN" with equally
> wide rows?  There doesn't seem to be a lot of value in supporting dump
> if you can't reload ...

Indeed, the lines bigger than 1GB can't be reloaded :(

My test: with a stock 9.5.1 plus Alvaro's patch with my additional
int64 fix mentioned upthread, creating this table and filling
all columns with 200MB of text each:

create table bigtext(t1 text ,t2 text, t3 text, t4 text,
 t5 text, t6 text, t7 text);

# \copy bigtext to '/var/tmp/bigtext.sql'

That part does work as expected, producing this huge single line:
$ wc /var/tmp/bigtext.sql
 1  7 1468006407 /var/tmp/bigtext.sql

But reloading it fails:

# create table bigtext2 (like bigtext);
CREATE TABLE

# copy bigtext2 from '/var/tmp/bigtext.sql';
ERROR:  54000: out of memory
DETAIL:  Cannot enlarge string buffer containing 1073676288 bytes by 65536
more bytes.
CONTEXT:  COPY bigtext2, line 1
LOCATION:  enlargeStringInfo, stringinfo.c:278

# \copy bigtext2 from '/var/tmp/bigtext.sql'
ERROR:  54000: out of memory
DETAIL:  Cannot enlarge string buffer containing 1073741808 bytes by 8191
more bytes.
CONTEXT:  COPY bigtext2, line 1
LOCATION:  enlargeStringInfo, stringinfo.c:278

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

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

Re: [HACKERS] improving GROUP BY estimation

2016-03-03 Thread Tomas Vondra


Hi,

On 03/03/2016 12:53 PM, Alexander Korotkov wrote:

Hi, Tomas!

I've assigned to review this patch.

I've checked version estimate-num-groups-v2.txt by Mark Dilger.
It applies to head cleanly, passes corrected regression tests.

About correlated/uncorrelated cases. I think now optimizer mostly assumes
all distributions to be independent. I think we should follow this
assumption in this case also until we have fundamentally better option (like
your multivariate statistics).

@@ -3438,9 +3438,9 @@ estimate_num_groups(PlannerInfo *root, List *groupExprs,
double input_rows,
reldistinct = clamp;

/*
-* Multiply by restriction selectivity.
+* Estimate number of distinct values expected in given number of rows.
*/
-reldistinct *= rel->rows / rel->tuples;
+reldistinct *= (1 - powl((reldistinct - 1) / reldistinct, rel->rows));

/*
* Update estimate of total distinct groups.

I think we need way more explanation in comments here (possibly with
external links). For now, it looks like formula which appears from nowhere.


I thought the details (particularly the link to stackexchange, discussing the 
formula) would be enough, but let me elaborate.


The current formula

reldistinct *= rel->rows / rel->tuples;

simply multiplies the ndistinct estimate with selectivity. So when you select 
1% of the table, the estimate says we'll see 1% of ndistinct values. But 
clearly that's naive, because for example when you have 10k distinct values and 
you select 10M rows, you should expect nearly all of them in the sample.


And that's what the formula does - it gives you the expected number of distinct 
values, when selecting 'k' values from 'd' distinct values with replacements:


count(k, d) = d * (1 - ((d-1)/d) ^ k)

It's assuming the distinct values are equally probable (uniform distribution) 
and that the probabilities do not change (that's the "with replacement").


I guess we could relax those assumptions and for example use the MCV statistics 
to further improve the estimate, and also relax the 'with replacement' but 
that'd make the formula way more complicated.


[1] 
http://math.stackexchange.com/questions/72223/finding-expected-number-of-distinct-values-selected-from-a-set-of-integers




Also, note that rel->tuples gone away from formula parameters. So, we
actually don't care about how may tuples are in the table. This is because
this formula assumes that same tuple could be selected multiple times. For
low numbers this can lead to some errors. Consider selecting 2 from 3
distinct tuples. If you can't select same tuple twice then you always
selecting 2 distinct tuples. But according to formula you will select 5/3 in
average. I think this error in small numbers is negligible, especially
because getting rid of this error would require way more computations. But
it worth mentioning in comments though.


Well, yeah. That's the consequence of 'with replacement' assumption.

I guess we could handle this somehow, though. For example we can compute the 
minimum possible number of distinct values like this:


average_rows_per_value = (tuples / ndistinct);
min_estimate = ceil(rows / average_rows_per_value);

and use that as a minimum for the estimate. In the example you've given this 
would say


average_rows_per_value = (3 / 3) = 1;
min_estimate = ceil(2 / 1) = 2;

So it serves as a correction for the 'with replacement' assumption. With only 2 
distinct values we'd get this:


average_rows_per_value = (3 / 2) = 1.5;
min_estimate = ceil(2 / 1.5) = 2;

Of course, it's just a heuristics, so this may fail in some other cases.

regards

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


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

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

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Yeah.  At this point, the earliest the cmake work could land is 9.7,
>> and TBH I do not think we are committed to landing it at all.  So it'd
>> behoove us to fix at least HEAD, and probably older branches too,
>> to work with up-to-date MSVC.

> In that case we should probably add the configure check for IPC::Run
> that was proposed in another thread.

Yeah, agreed.  I opposed it at the time because I thought we might be
considering cmake conversion for 9.6.  But now that that's missed the
final 9.6 commitfest, we'd better operate on the assumption that it's
not landing anytime soon.

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] OOM in libpq and infinite loop with getCopyStart()

2016-03-03 Thread Aleksander Alekseev

Hello, Michael

> The easiest way to perform tests with this patch is to take a debugger
> and enforce the malloc'd pointers to NULL in the code paths.

I see. Still I don't think it's an excuse to not provide clear steps to
reproduce an issue. As I see it anyone should be able to easily check
your patch locally without having deep understanding of how libpq is
implemented or reading thread which contains 48 e-mails.

For instance you cold provide a step-by-step instruction like:

1. run gdb --args some-prog arg1 arg2 ...
2. b some_file.c:123
3. c
4. ...
5. we expect ... but actually see ...

Or you cold automate these steps using gdb batch file:

gdb --batch --command=gdb.script --args some-prog arg1 arg2

... where gdb.script is:

b some_file.c:123
c
... etc ...

Naturally all of this is optional. But it will simplify both
understanding and reviewing of this path. Thus chances that this patch
will be accepted will be increased and time required for this will be
reduced significantly.

Best regards,
Aleksander


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

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

Tom Lane wrote:

> Yeah.  At this point, the earliest the cmake work could land is 9.7,
> and TBH I do not think we are committed to landing it at all.  So it'd
> behoove us to fix at least HEAD, and probably older branches too,
> to work with up-to-date MSVC.

In that case we should probably add the configure check for IPC::Run
that was proposed in another thread.

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


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

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

Petr Jelinek  writes:
> On 03/03/16 15:02, Michael Paquier wrote:
>> So, would there be interest for a patch on the perl scripts in
>> src/tools/msvc or are they considered a lost cause? Having a look at
>> the failures could be done with the cmake work, but it seems a bit
>> premature to me to look at that for the moment, and having support for
>> VS2015 with 9.5 (support for new versions of VS won a backpatch the
>> last couple of years) would be a good thing I think.

> +1, and I can help (at least review and testing if nothing else).

Yeah.  At this point, the earliest the cmake work could land is 9.7,
and TBH I do not think we are committed to landing it at all.  So it'd
behoove us to fix at least HEAD, and probably older branches too,
to work with up-to-date MSVC.

regards, tom lane


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

Re: [HACKERS] WIP: Upper planner pathification

David Rowley  writes:
> My gripe is that I've added the required code to build the parallel
> group aggregate to create_agg_path() already, but since Group
> Aggregate uses the RollupPath I'm forced to add code in
> create_rollup_plan() which manually stacks up Plan nodes rather than
> just dealing with Paths and create_plan() and its recursive call
> magic.

Yeah, RollupPath was something I did to be expeditious rather than
something I'm particularly in love with.  It's OK for a first version,
I think, but we'd need to refactor it if we were to consider more than
one implementation strategy for a rollup.  Also it's pretty ugly that
the code makes a RollupPath even when a basic AggPath is what is meant;
that's a leftover from the fact that current HEAD goes through
build_grouping_chain() even for simple aggregation without grouping sets.

One point to consider is that we don't want the create_foo_path stage
to do much more than what's necessary to get a cost/rows estimate.
So in general, postponing as much work as possible to createplan.c
is a good thing.  But we don't want the Path representation to leave
any interesting planning choices implicit.

My general feeling about this is that I don't want it to be a blocker
for getting the basic patch in, but I'll happily consider further
refactoring of individual path types once we're over that hump.
If you wanted to start on a follow-on patch to deal with this particular
issue, be my guest.

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] Improve error handling in pltcl

Hi

I am testing behave, and some results looks strange

postgres=# \sf foo
CREATE OR REPLACE FUNCTION public.foo()
 RETURNS void
 LANGUAGE plpgsql
AS $function$
begin
  raise exception sqlstate 'ZZ666' using message='hello, world',
detail='hello, my world', hint = 'dont afraid';
end
$function$

postgres=# select tcl_eval('spi_exec "select foo();"');
ERROR:  38000: hello, world
CONTEXT:  hello, world   ==???

the message was in context. Probably it is out of scope of this patch, but
it isn't consistent with other PL


while executing
"spi_exec "select foo();""
("eval" body line 1)
invoked from within
"eval $1"
(procedure "__PLTcl_proc_16864" line 3)
invoked from within
"__PLTcl_proc_16864 {spi_exec "select foo();"}"
in PL/Tcl function "tcl_eval"
LOCATION:  throw_tcl_error, pltcl.c:1217
Time: 1.178 ms

postgres=# select tcl_eval('join $::errorCode "\n"');
tcl_eval
═
 POSTGRES   ↵
 message↵
 hello, world   ↵
 detail ↵
 hello, my world↵
 hint   ↵
 dont afraid↵
 domain ↵
 plpgsql-9.6↵
 context_domain ↵
 postgres-9.6   ↵
 context↵
 PL/pgSQL function foo() line 3 at RAISE↵
 SQL statement "select foo();"  ↵
 cursor_position↵
 0  ↵
 filename   ↵
 pl_exec.c  ↵
 lineno ↵
 3165   ↵
 funcname   ↵
 exec_stmt_raise
(1 row)

I miss a SQLSTATE.

Why is used List object instead dictionary? TCL supports it
https://www.tcl.tk/man/tcl8.5/tutorial/Tcl23a.html

Regards

Pavel

Re: [HACKERS] TAP / recovery-test fs-level backups, psql enhancements etc

Craig Ringer wrote:
> On 3 March 2016 at 21:16, Michael Paquier  wrote:

> > > The rest are feature patches:
> > > 004 allows filtering on RecursiveCopy by a predicate function. Needed for
> > > filesystem level backups (007). It could probably be squashed with 007 if
> > > desired.
> >
> > Adding perldoc to this module should be done separately, let's not mix
> > things. Applying a filter is useful as well to remove for example the
> > contents of pg_xlog, so no objections to it.
> 
> Eh, ok. I figured it was so trivial it didn't matter, but will split.

Please don't mess with this one.

> > psql_check sounds wrong to me. I thought first that this triggers a
> > test. Why not psql_simple or psql_basic, or just keep psql.
> 
> I guess I'm used to Python's subprocess.check_call so to me it's natural.
> 
> I want something that makes it clear that failure is a fatal error
> condition, i.e. "do this in psql and if it produces an error, treat it like
> you would any other error in Perl and die appropriately".

Shrug.  psql_check seemed reasonable to me for that.

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


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

Re: [HACKERS] On columnar storage (2)

2016-03-03 Thread Bert

Hey Alvaro,

I was referring to https://wiki.postgresql.org/wiki/ColumnOrientedSTorage .
and yes, I'll be at the next fosdem / pgconf.eu for sure. :-)

Bert

On Thu, Mar 3, 2016 at 3:40 PM, Alvaro Herrera 
wrote:

> Bert wrote:
>
> > Alvaro,
> > You wrote that a wiki page would be opened regarding this. But I still
> > cannot find such a page (expect for an old page which hasn't changed in
> the
> > last year). Is there already something we can look at?
>
> Yeah, I haven't done that yet.  I will post here as soon as I get that
> done.  Happy to share another beer to discuss, next time I'm over there.
> I'm also going to have code to share for you to test by then!
>
> What's the other page you mention?
>
> --
> Álvaro Herrerahttp://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>



-- 
Bert Desmet
0477/305361

Re: [HACKERS] On columnar storage (2)

Haribabu Kommi wrote:

> The performance report is taken on the patch that is WIP columnar storage
> on PostgreSQL database. Only the storage part of the code is finished.
> To test the performance, we used custom plan to generate the plans
> where it can use the columnar storage. This way we ran the performance
> test.

Quickly eyeballing your results I think they are similar to ours: there
are some performance gains but nothing spectacular.  That's why I want
to take another, more invasive approach that buys us more.  The wiki
page I'm to write will describe our rough plan for that.  Your input on
that will be appreciated.

> I want to integrate this patch with syntax proposed by Alvaro for columnar
> storage and share it with community, before that i want to share the current
> storage design with the community for review by preparing some readme
> file. I will try to send this soon.

Please do, thanks.

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


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

Re: [HACKERS] On columnar storage (2)

Bert wrote:

> Alvaro,
> You wrote that a wiki page would be opened regarding this. But I still
> cannot find such a page (expect for an old page which hasn't changed in the
> last year). Is there already something we can look at?

Yeah, I haven't done that yet.  I will post here as soon as I get that
done.  Happy to share another beer to discuss, next time I'm over there.
I'm also going to have code to share for you to test by then!

What's the other page you mention?

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

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

Re: [HACKERS] Support for N synchronous standby servers - take 2

2016-03-03 Thread Masahiko Sawada

Hi,

Thank you so much for reviewing this patch!

All review comments regarding document and comment are fixed.
Attached latest v14 patch.

> This accepts 'abc^Id' as a name, which is wrong behavior (but
> such appliction names are not allowed anyway. If you assume so,
> I'd like to see a comment for that.).

'abc^Id' is accepted as application_name, no?
postgres(1)=# set application_name to 'abc^Id';
SET
postgres(1)=# show application_name ;
 application_name
--
 abc^Id
(1 row)

> addlit_xd_string(char *ytext) and addlitchar_xd_string(unsigned
> char ychar) requires differnt character types. Is there any reason
> for that?

Because addlit_xd_string() is for adding string(char *) to xd_string,
OTOH addlit_xd_char() is for adding just one character to xd_string.

> I personally don't like addlit*string() things for such simple
> syntax but itself is acceptble enough for me. However it uses
> StringInfo to hold double-quoted names, which pallocs 1024 bytes
> of memory chunk for every double-quoted name. The chunks are
> finally stacked up left uncollected until the current
> memorycontext is deleted or reset (It is deleted just after
> finishing config file processing). Addition to that, setting
> s_s_names runs the parser twice. It seems to me too greedy and
> seems that static char [NAMEDATALEN] is enough using the v12 way
> without palloc/repalloc.

I though that length of group name could be more than NAMEDATALEN, so
I use StringInfo.
Is it not necessary?

> I found that the name SyncGroupName.wait_num is not
> instinctive. How about sync_num, sync_member_num or
> sync_standby_num? If the last is preferable, .members also should
> be .standbys .

Thanks, sync_num is preferable to me.

===
> I am quite uncomfortable with the existence of
> WanSnd.sync_standby_priority. It represented the pirority in the
> old linear s_s_names format but nested groups or even
> single-level quarum list obviously doesn't fit it. Can we get rid
> of sync_standby_priority, even though we realize atmost
> n-priority for now?

We could get rid of sync_standby_priority.
But if so, we will not be able to see the next sync standby in
pg_stat_replication system view.
Regarding each node priority, I was thinking that standbys in quorum
list have same priority, and in nested group each standbys are given
the priority starting from 1.

===
> The function SyncRepGetSyncedLsnsUsingPriority doesn't seem to
> have specific code for every prioritizing method (which are
> priority, quorum, nested and so). Is there any reson to use it as
> a callback of SyncGroupNode?

The reason why the current code is so is that current code is for only
priority method supporting.
At first version of this feature, I'd like to implement it more simple.

Aside from this, of course I'm planning to have specific code for nested design.
- The group can have some name nodes or group nodes.
- The group can use either 2 types of method: priority or quorum.
- The group has SyncRepGetSyncedLsnFn() and SyncRepGetStandbysFn()
  - SyncRepGetSyncedLsnsFn() function recursively determine synced LSN
at that moment using group's method.
  - SyncRepGetStandbysFn() function returns standbys of its group,
which are considered as sync using group's method.

For example, s_s_name  = '3(a, b, 2[c,d]::group1)', SyncRepStandbys
memory structure will be,

"main(quorum)" --- "a"
|
-- "b"
|
-- "group1(priority)" --- "c"
 |
 -- "d"

When determine synced LSNs, we need to consider group1's LSN using by
priority method at first, and then we can determine main's LSN using
by quorum method with "a" LSNs, "b" LSNs and "group1" LSNs.
So SyncRepGetSyncedLsnsUsingPriority() function would be,

bool
SyncRepGetSyncedLsnsUsingPriority(*group, *write_lsn, *flush_lsn)
{
sync_num = group->SynRepGetSyncstandbysFn(group, sync_list);

if (sync_num < group->sync_num)
return false;

for (each member of sync_list)
{
if (member->type == group node)
call SyncRepGetSyncedLsnsFn(member, w, f) and store w and
f into lsn_list.
else
Store name node LSNs into lsn_list.
}

Determine synced LSNs of this group using lsn_list and priority method.
Store synced LSNs into write_lsn and flush_lsn.
return true;
}

> SyncRepClearStandbyGroupList is defined in syncrep.c but the
> other related functions are defined in syncgroup_gram.y. It would
> be better to place them together.

SyncRepClearStandbyGroupList() is used by
check_synchronous_standby_names(), so I put this function syncrep.c.

> SyncRepStandbys are to be in multilevel and the struct is
> naturally allowed to be so but SyncRepClearStandbyGroupList
> assumes it in single level.

Because I think that we don't need to implement to fully support
nested style at first

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

2016-03-03 Thread Petr Jelinek


On 03/03/16 15:02, Michael Paquier wrote:

Hi all,

Microsoft provides a set of VMs that one can use for testing and
Windows 10 is in the set:
https://dev.windows.com/en-us/microsoft-edge/tools/vms/windows/
I have grabbed one and installed the community version of Visual
Studio 2015 so I think that I am going to be able to compile Postgres
with VS2015 with a bit of black magic.

My goal is double:
1) As far as things have been discussed, VS2015 is making difficult
the compilation of Postgres, particularly for locales. So I'd like to
see what are the problems behind that and see if we can patch it
properly. This would facilitate the integration of cmake as well for
Windows.


The locale problem is that:
a) the MS crt headers are broken for this particular part due to 
unfinished refactoring, even their msvcrt sources don't compile with 
them, if you read them it's obvious they forgot to put one variable in 
the struct
b) we are using somewhat legacy API there that's internally implemented 
as hacks over the new API (string parsing and generation and stuff is 
happening there)
c) the non-legacy API works only on Vista+ and does not support the old 
locale names (which is why the legacy api that is written on top of this 
has to do the string parsing and generation)


To me the least bad solution for MSVC 2015 and codepage detection seemed 
to use the new API when possible (GetLocaleInfoEx and friends) and fall 
back to our old string parsing that we did pre-VC2013 when it's not, 
since afaics it works correctly on all the locale names that are not 
supported by the new API. It means that MSVC 2015 builds would be Vista+ 
but I honestly don't see that as big issue given Microsoft's own policy 
about old Windows versions.



2) src/tools/msvc stuff has support only up to VS2013. I think that it
would be nice to bump that a bit and get something for 9.5~.



Yeah, we'll also have to do something about perl 5.22 compatibility 
there as psed is not included in the core distribution anymore from that 
version and we use it to generate one header file IIRC.



So, would there be interest for a patch on the perl scripts in
src/tools/msvc or are they considered a lost cause? Having a look at
the failures could be done with the cmake work, but it seems a bit
premature to me to look at that for the moment, and having support for
VS2015 with 9.5 (support for new versions of VS won a backpatch the
last couple of years) would be a good thing I think.



+1, and I can help (at least review and testing if nothing else).

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


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

Re: [HACKERS] WIP: Upper planner pathification

2016-03-03 Thread Teodor Sigaev


So I see no evidence for a slowdown on pgbench's SELECT queries.
Anybody else want to check performance on simple scan/join queries?


I did your tests with configure --enable-depend and pgbench -i -s 100 and 
slightly tweaked postgresql.conf, on notebook with CPU i7-3520M (2 cores + 2 
HT), FreeBSD 10.2.


pgbench -c 4 -j 4 -P 1 -T 60 -S
HEAD 35834 tps avg (35825/35828/35808/35844/35865)
Patched HEAD 35529 tps avg (35516/35561/35527/35534/35510)

~1% slowdown. I can live with that.

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


--
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] Improve error handling in pltcl

Hi

2016-03-01 22:23 GMT+01:00 Jim Nasby :

> On 2/29/16 10:01 PM, Tom Lane wrote:
>
>> Jim Nasby  writes:
>>
>>> On 2/28/16 5:50 PM, Jim Nasby wrote:
>>>
 Per discussion in [1], this patch improves error reporting in pltcl.

>>>
>> I forgot to mention that this work is sponsored by Flight Aware
>>> (http://flightaware.com).
>>>
>>
>> Huh ... I use that site.  There's PG and pltcl code behind it?
>> Cool!
>>
>
>
I didn't study this patch deeper yet. Just I am sending rebased code

I found one issue. The context

+ ERROR:  relation "foo" does not exist
+ CONTEXT:  relation "foo" does not exist
+ while executing
+ "spi_exec "select * from foo;""
+ ("eval" body line 1)
+ invoked from within
+ "eval $1"
+ (procedure "__PLTcl_proc_16461" line 3)
+ invoked from within
+ "__PLTcl_proc_16461 {spi_exec "select * from foo;"}"
+ in PL/Tcl function "tcl_eval"

is changed in any call - when I did "make installcheck"


*** 567,575 
  ("eval" body line 1)
  invoked from within
  "eval $1"
! (procedure "__PLTcl_proc_16461" line 3)
  invoked from within
! "__PLTcl_proc_16461 {spi_exec "select * from foo;"}"
  in PL/Tcl function "tcl_eval"
  select pg_temp.tcl_eval($$
  set list [lindex $::errorCode 0];
--- 567,575 
  ("eval" body line 1)
  invoked from within
  "eval $1"
! (procedure "__PLTcl_proc_16841" line 3) <
_PLTcl_proc_
  invoked from within
! "__PLTcl_proc_16841 {spi_exec "select * from foo;"}"
  in PL/Tcl function "tcl_eval"
  select pg_temp.tcl_eval($$
  set list [lindex $::errorCode 0];

I am not able to see, if this information is interesting or not. We can
hide context, but I have not a idea, if it is ok or not.

Regards

Pavel
diff --git a/doc/src/sgml/pltcl.sgml b/doc/src/sgml/pltcl.sgml
new file mode 100644
index d2175d5..d5c576d
*** a/doc/src/sgml/pltcl.sgml
--- b/doc/src/sgml/pltcl.sgml
*** CREATE EVENT TRIGGER tcl_a_snitch ON ddl
*** 775,780 
--- 775,901 
  
 
  
+
+ Error Handling in PL/Tcl
+ 
+ 
+  error handling
+  in PL/Tcl
+ 
+ 
+ 
+  All Tcl errors that are allowed to propagate back to the top level of the
+  interpreter, that is, errors not caught within the stored procedure
+  using the Tcl catch command will raise a database
+  error.
+ 
+ 
+  Tcl code within or called from the stored procedure can choose to
+  raise a database error by invoking the elog
+  command provided by PL/Tcl or by generating an error using the Tcl
+  error command and not catching it with Tcl's
+  catch command.
+ 
+ 
+  Database errors that occur from the PL/Tcl stored procedure's
+  use of spi_exec, spi_prepare,
+  and spi_execp are also catchable by Tcl's
+  catch command.
+ 
+ 
+  Tcl provides an errorCode variable that can
+  represent additional information about the error in a form that
+  is easy for programs to interpret.  The contents are in Tcl list
+  format and the first word identifies the subsystem or
+  library responsible for the error and beyond that the contents are left
+  to the individual code or library.  For example if Tcl's
+  open command is asked to open a file that doesn't
+  exist, errorCode
+  might contain POSIX ENOENT {no such file or directory}
+  where the third element may vary by locale but the first and second
+  will not.
+ 
+ 
+  When spi_exec, spi_prepare
+  or spi_execp cause a database error to be raised,
+  that database eror propagates back to Tcl as a Tcl error.
+  In this case errorCode is set to a list
+  where the first element is POSTGRES followed by a
+  copious decoding of the Postgres error structure.  Since fields in the
+  structure may or may not be present depending on the nature of the
+  error, how the function was invoked, etc, PL/Tcl has adopted the 
+  convention that subsequent elements of the errorCode
+  list are key-value pairs where the first value is the name of the
+  field and the second is its value.
+ 
+ 
+  Fields that may be present include message,
+  detail, detail_log,
+  hint, domain,
+  context_domain, context,
+  schema, table,
+  column, datatype,
+  constraint, cursor_position,
+  internalquery, internal_position,
+  filename, lineno and
+  funcname.
+ 
+ 
+  You might find it useful to load the results into an array. Code
+  for doing that might look like
+ 
+ if {[lindex $errorCode 0] == "POSTGRES"} {
+ array set errorRow [lrange $errorCode 1 end]
+ }
+ 
+ 
+ 
+  In the example below we cause an error by attempting to
+  SELECT from a table that doesn't exist.
+ 
+ select tcl_eval('spi_exec "select * from foo;"');
+ 
+ 
+ 
+ ERROR:  relation "foo" does not exist
+ 
+ 
+

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

Hi all,

Microsoft provides a set of VMs that one can use for testing and
Windows 10 is in the set:
https://dev.windows.com/en-us/microsoft-edge/tools/vms/windows/
I have grabbed one and installed the community version of Visual
Studio 2015 so I think that I am going to be able to compile Postgres
with VS2015 with a bit of black magic.

My goal is double:
1) As far as things have been discussed, VS2015 is making difficult
the compilation of Postgres, particularly for locales. So I'd like to
see what are the problems behind that and see if we can patch it
properly. This would facilitate the integration of cmake as well for
Windows.
2) src/tools/msvc stuff has support only up to VS2013. I think that it
would be nice to bump that a bit and get something for 9.5~.

So, would there be interest for a patch on the perl scripts in
src/tools/msvc or are they considered a lost cause? Having a look at
the failures could be done with the cmake work, but it seems a bit
premature to me to look at that for the moment, and having support for
VS2015 with 9.5 (support for new versions of VS won a backpatch the
last couple of years) would be a good thing I think.

Thoughts?
-- 
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] OOM in libpq and infinite loop with getCopyStart()

On Thu, Mar 3, 2016 at 10:18 PM, Aleksander Alekseev
 wrote:
> I didn't checked your patch in detail yet but here is a thought I would
> like to share.
>
> In my experience usually it takes number of rewrites before patch will
> be accepted. To make sure that after every rewrite your patch still
> solves an issue you described you should probably provide a few test
> programs too. If it's possible to make these programs part of regression
> tests suite it would be just great.

Yes that's usually the case, this patch is not at its first version.

> Without such test programs I personally find it difficult to verify
> that your patch fixes something. If such examples are provided your
> patch would be much more likely to accepted. "See, this program crashes
> everything. Now we apply a patch and everything works! Cool, heh?"

The easiest way to perform tests with this patch is to take a debugger
and enforce the malloc'd pointers to NULL in the code paths. That's
wild but efficient, and that's actually for we did for the other two
libpq patches treating those OOM failures. In the case of the standby
code path what I did was to put a sleep in the WAL receiver code and
then trigger the OOM manually, leading to the errors listed upthread.
Regarding automated tests, it is largely feasible to have tests on
Linux at least by using for example LD_PRELOAD or glibc-specific
malloc hooks, though those would be platform-restricted. Here is for
example one for COPY IN/OUT, that this patch also passed (there has
been already some discussion for this patch).
http://www.postgresql.org/message-id/55fc501a.5000...@iki.fi
Applying it for recovery is doable as well by manipulating the
recovery protocol with libpq though it is aimed really at covering a
narrow case.
-- 
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] WIP: Upper planner pathification

On 3 March 2016 at 22:57, David Rowley  wrote:
> On 3 March 2016 at 18:04, Tom Lane  wrote:
>> If you come across any points where it seems like it could be
>> done better or more easily, that would be tremendously valuable feedback
>> at this stage.
>
> Well since you mention it, I started on hash grouping and it was
> rather simple and clean as in create_agg_path() I just created a chain
> of the required paths and let create_plan() recursively build the
> Finalize Aggregate -> Gather -> Partial Aggregate -> Seq Scan plan.
> That was rather simple, and actually very nice when compared to how
> things are handled in today's grouping planner. When it comes to Group
> Aggregate I notice that you're using a RollupPath rather than an
> AggPath even when there's no grouping sets. This also means that
> create_agg_path() is only ever used for the AGG_HASHED strategy, even
> though the 'strategy' is passed as a parameter to that function, so it
> seemed prudent to me, to make sure all strategies are handled properly
> there.
>
> My gripe is that I've added the required code to build the parallel
> group aggregate to create_agg_path() already, but since Group
> Aggregate uses the RollupPath I'm forced to add code in
> create_rollup_plan() which manually stacks up Plan nodes rather than
> just dealing with Paths and create_plan() and its recursive call
> magic.
>
> I can't quite see any blocker for not doing this, so would you object
> to separating out the treatment of Group Aggregate and Grouping Sets
> in create_grouping_paths() ? I think it would require less code
> overall.

Actually I might have jumped the gun a little here with my complaint.
I think it does not matter about this as I've just coded Parallel
Group Aggregate part to reuse create_agg_path(), but left the
non-parallel version to make use of create_rollup_path(). I don't
think I want to go to the trouble of parallel grouping sets at this
stage anyway.


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


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

Re: [HACKERS] Submit Pull Request

On 3 March 2016 at 19:11, Maton, Brett  wrote:

> Hi,
>
>   How do I push a branch to git.postgresql.org/git/pgrpms.git ?
>

You can't push, but you can open a ticket at
https://redmine.postgresql.org/projects/pgrpms or write to the
pgsql-pkg-...@postgresql.org mailing list to request merging of a branch
you've published somewhere accessible.

Please explain what the change is and why it's needed in your ticket/email,
rather than just "please merge" etc.

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

Re: [HACKERS] TAP / recovery-test fs-level backups, psql enhancements etc

On 3 March 2016 at 21:16, Michael Paquier  wrote:

> On Thu, Mar 3, 2016 at 4:11 PM, Craig Ringer 
> wrote:
> > The first three are simple fixes that should go in without fuss:
> >
> > 001 fixes the above 5.8.8 compat issue.
> > 002  fixes another minor whoopsie, a syntax error in
> > src/test/recovery/t/003_recovery_targets.pl that never got noticed
> because
> > exit codes are ignored.
>
> Those two are embarrassing things... However:
> -$node_master->psql('postgres',
> -   "SELECT pg_create_restore_point('$recovery_name'");
> +$node_master->psql_check('postgres',
> +   "SELECT pg_create_restore_point('$recovery_name');");
> In 0002 this is not correct, psql_check is part of a routine you
> introduce later on.
>

Damn, I meant to fix that when I rebased it back in the history but forgot.
Thankyou.


>
> > The rest are feature patches:
> > 004 allows filtering on RecursiveCopy by a predicate function. Needed for
> > filesystem level backups (007). It could probably be squashed with 007 if
> > desired.
>
> Adding perldoc to this module should be done separately, let's not mix
> things. Applying a filter is useful as well to remove for example the
> contents of pg_xlog, so no objections to it.
>

Eh, ok. I figured it was so trivial it didn't matter, but will split.


> > 005 adds the new psql functions psql_expert and psql_check. Then 006
> renames
> > psql_expert to psql and fixes up all call sites across the tree to use
> the
> > new interface. I found the bug in 002 as part of that process. I
> anticipate
> > that 005 and 006 would be squashed into one commit to master, but I've
> kept
> > them separate in my tree for easier review.
>
> psql_check sounds wrong to me. I thought first that this triggers a
> test. Why not psql_simple or psql_basic, or just keep psql.
>

I guess I'm used to Python's subprocess.check_call so to me it's natural.

I want something that makes it clear that failure is a fatal error
condition, i.e. "do this in psql and if it produces an error, treat it like
you would any other error in Perl and die appropriately".


> > 007 adds PostgresNode support for hot and cold filesystem-level backups
> > using pg_start_backup and pg_stop_backup, which will be required for some
> > coming tests and are useful by themselves.
>
> It would be nice as well to have a flag in _backup_fs to filter the
> contents of pg_xlog at will.


i.e. copy without any pg_xlog at all? without_xlog => 1 ?


> log_collector is not enabled in the nodes
> deployed by PostgresNode, so why filtering it?
>

Because at the time I wrote that the test dirs were deleted unconditionally
and I didn't bother checking if we actually wrote anything to pg_log. Also,
because if it _does_ get enabled by someone I can't imagine why we'd want
to copy the logs.


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

Re: [HACKERS] OOM in libpq and infinite loop with getCopyStart()

2016-03-03 Thread Aleksander Alekseev

Hello, Michael

I didn't checked your patch in detail yet but here is a thought I would
like to share.

In my experience usually it takes number of rewrites before patch will
be accepted. To make sure that after every rewrite your patch still
solves an issue you described you should probably provide a few test
programs too. If it's possible to make these programs part of regression
tests suite it would be just great.

Without such test programs I personally find it difficult to verify
that your patch fixes something. If such examples are provided your
patch would be much more likely to accepted. "See, this program crashes
everything. Now we apply a patch and everything works! Cool, heh?"

Best regards,
Aleksander


-- 
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] TAP / recovery-test fs-level backups, psql enhancements etc

On Thu, Mar 3, 2016 at 4:11 PM, Craig Ringer  wrote:
> The first three are simple fixes that should go in without fuss:
>
> 001 fixes the above 5.8.8 compat issue.
> 002  fixes another minor whoopsie, a syntax error in
> src/test/recovery/t/003_recovery_targets.pl that never got noticed because
> exit codes are ignored.

Those two are embarrassing things... However:
-$node_master->psql('postgres',
-   "SELECT pg_create_restore_point('$recovery_name'");
+$node_master->psql_check('postgres',
+   "SELECT pg_create_restore_point('$recovery_name');");
In 0002 this is not correct, psql_check is part of a routine you
introduce later on.

> 003 runs perltidy on PostgresNode.pm to bring it back into conformance after
> the recovery tests commit.

No objections to that.

> The rest are feature patches:
> 004 allows filtering on RecursiveCopy by a predicate function. Needed for
> filesystem level backups (007). It could probably be squashed with 007 if
> desired.

Adding perldoc to this module should be done separately, let's not mix
things. Applying a filter is useful as well to remove for example the
contents of pg_xlog, so no objections to it.

> 005 adds the new psql functions psql_expert and psql_check. Then 006 renames
> psql_expert to psql and fixes up all call sites across the tree to use the
> new interface. I found the bug in 002 as part of that process. I anticipate
> that 005 and 006 would be squashed into one commit to master, but I've kept
> them separate in my tree for easier review.

psql_check sounds wrong to me. I thought first that this triggers a
test. Why not psql_simple or psql_basic, or just keep psql.

> 007 adds PostgresNode support for hot and cold filesystem-level backups
> using pg_start_backup and pg_stop_backup, which will be required for some
> coming tests and are useful by themselves.

It would be nice as well to have a flag in _backup_fs to filter the
contents of pg_xlog at will. log_collector is not enabled in the nodes
deployed by PostgresNode, so why filtering it?
-- 
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] Submit Pull Request

2016-03-03 Thread Andreas Karlsson


On 03/03/2016 12:11 PM, Maton, Brett wrote:

   How do I push a branch to git.postgresql.org/git/pgrpms.git
 ?


I would email pgsql-pkg-...@postgresql.org, as mentioned at 
http://yum.postgresql.org/contact.php, with questions about how to 
contribute to the pgrpms.


Andreas



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

Re: [HACKERS] pgbench small bug fix

2016-03-03 Thread Aleksander Alekseev

> time pgbench -T 5 -R 0.1 -P 1 -c 2 -j 2

On my laptop this command executes 25 seconds instead of 5. I'm pretty
sure it IS a bug. Probably a minor one though.

I tested this patch on Ubuntu 14.04 LTS with GCC 4.8. It applies
cleanly on master branch (c7111d11) and solves a described problem.
No compilation warnings. Everything else works as before.

Still I wonder if code could be patched more cleanly. Instead of:

if(someint)
if(somebool)

... you should probably write:

if(someint > 0)
if(somebool == TRUE)

Also I suggest to introduce a few new boolean variables with meaningful
names instead of writing all these long expressions right inside of
if( ... ). 

As a side note I noticed that pgbench.c is not pgindent'ed. Since you
are modifying this file anyway probably you cold solve this issue too?
As a separate patch perhaps.


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

Re: [HACKERS] improving GROUP BY estimation

2016-03-03 Thread Alexander Korotkov

Hi, Tomas!

I've assigned to review this patch.

I've checked version estimate-num-groups-v2.txt by Mark Dilger.
It applies to head cleanly, passes corrected regression tests.

About correlated/uncorrelated cases. I think now optimizer mostly assumes
all distributions to be independent.
I think we should follow this assumption in this case also until we have
fundamentally better option (like your multivariate statistics).

@@ -3438,9 +3438,9 @@ estimate_num_groups(PlannerInfo *root, List
*groupExprs, double input_rows,
  reldistinct = clamp;

  /*
- * Multiply by restriction selectivity.
+ * Estimate number of distinct values expected in given number of rows.
  */
- reldistinct *= rel->rows / rel->tuples;
+ reldistinct *= (1 - powl((reldistinct - 1) / reldistinct, rel->rows));

  /*
  * Update estimate of total distinct groups.

I think we need way more explanation in comments here (possibly with
external links). For now, it looks like formula which appears from nowhere.

Also, note that rel->tuples gone away from formula parameters. So, we
actually don't care about how may tuples are in the table. This is because
this formula assumes that same tuple could be selected multiple times. For
low numbers this can lead to some errors. Consider selecting 2 from 3
distinct tuples. If you can't select same tuple twice then you always
selecting 2 distinct tuples. But according to formula you will select 5/3
in average. I think this error in small numbers is negligible, especially
because getting rid of this error would require way more computations. But
it worth mentioning in comments though.

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

Re: [HACKERS] [NOVICE] WHERE clause not used when index is used

On Thu, Mar 3, 2016 at 7:49 PM, Tobias Florek  wrote:
>> > > Reverted patch in HEAD and 9.5
>> >
>> > Is there an ETA?
>> >
>>
>> I just committed the fix to the repo.
>
> Sorry for being unclear, is there an ETA for a new point-release?

Nothing concrete yet.
-- 
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] POC: Cache data in GetSnapshotData()

2016-03-03 Thread Mithun Cy

On Tue, Mar 1, 2016 at 12:59 PM, Amit Kapila 
wrote:
>Don't we need to add this only when the xid of current transaction is
valid?  Also, I think it will be better if we can explain why we need to
add the our >own transaction id while caching the snapshot.
I have fixed the same thing and patch is attached.

Some more tests done after that

*pgbench write tests: on 8 socket, 64 core machine.*

/postgres -c shared_buffers=16GB -N 200 -c min_wal_size=15GB -c
max_wal_size=20GB -c checkpoint_timeout=900 -c maintenance_work_mem=1GB -c
checkpoint_completion_target=0.9

./pgbench -c $clients -j $clients -T 1800 -M prepared postgres

[image: Inline image 3]

A small improvement in performance at 64 thread.

*LWLock_Stats data:*

ProcArrayLock: Base.

=

postgresql-2016-03-01_115252.log:PID 110019 lwlock main 4: shacq 1867601
exacq 35625 blk 134682 spindelay 128 dequeue self 28871

postgresql-2016-03-01_115253.log:PID 110115 lwlock main 4: shacq 2201613
exacq 43489 blk 155499 spindelay 127 dequeue self 32751

postgresql-2016-03-01_115253.log:PID 110122 lwlock main 4: shacq 2231327
exacq 44824 blk 159440 spindelay 128 dequeue self 6

postgresql-2016-03-01_115254.log:PID 110126 lwlock main 4: shacq 2247983
exacq 44632 blk 158669 spindelay 131 dequeue self 33365

postgresql-2016-03-01_115254.log:PID 110059 lwlock main 4: shacq 2036809
exacq 38607 blk 143538 spindelay 117 dequeue self 31008


ProcArrayLock: With Patch.

=

postgresql-2016-03-01_124747.log:PID 1789 lwlock main 4: shacq 2273958
exacq 61605 blk 79581 spindelay 307 dequeue self 66088

postgresql-2016-03-01_124748.log:PID 1880 lwlock main 4: shacq 2456388
exacq 65996 blk 82300 spindelay 470 dequeue self 68770

postgresql-2016-03-01_124748.log:PID 1765 lwlock main 4: shacq 2244083
exacq 60835 blk 79042 spindelay 336 dequeue self 65212

postgresql-2016-03-01_124749.log:PID 1882 lwlock main 4: shacq 2489271
exacq 67043 blk 85650 spindelay 463 dequeue self 68401

postgresql-2016-03-01_124749.log:PID 1753 lwlock main 4: shacq 2232791
exacq 60647 blk 78659 spindelay 364 dequeue self 65180

postgresql-2016-03-01_124750.log:PID 1849 lwlock main 4: shacq 2374922
exacq 64075 blk 81860 spindelay 339 dequeue self 67584

*-Block time of ProcArrayLock has reduced significantly.*


ClogControlLock : Base.

===

postgresql-2016-03-01_115302.log:PID 110040 lwlock main 11: shacq 586129
exacq 268808 blk 90570 spindelay 261 dequeue self 59619

postgresql-2016-03-01_115303.log:PID 110047 lwlock main 11: shacq 593492
exacq 271019 blk 89547 spindelay 268 dequeue self 5

postgresql-2016-03-01_115303.log:PID 110078 lwlock main 11: shacq 620830
exacq 285244 blk 92939 spindelay 262 dequeue self 61912

postgresql-2016-03-01_115304.log:PID 110083 lwlock main 11: shacq 633101
exacq 289983 blk 93485 spindelay 262 dequeue self 62394

postgresql-2016-03-01_115305.log:PID 110105 lwlock main 11: shacq 646584
exacq 297598 blk 93331 spindelay 312 dequeue self 63279


ClogControlLock : With Patch.

===

postgresql-2016-03-01_124730.log:PID 1865 lwlock main 11: shacq 722881
exacq 330273 blk 106163 spindelay 468 dequeue self 80316

postgresql-2016-03-01_124731.log:PID 1857 lwlock main 11: shacq 713720
exacq 327158 blk 106719 spindelay 439 dequeue self 79996

postgresql-2016-03-01_124732.log:PID 1826 lwlock main 11: shacq 696762
exacq 317239 blk 104523 spindelay 448 dequeue self 79374

postgresql-2016-03-01_124732.log:PID 1862 lwlock main 11: shacq 721272
exacq 330350 blk 105965 spindelay 492 dequeue self 81036

postgresql-2016-03-01_124733.log:PID 1879 lwlock main 11: shacq 737398
exacq 335357 blk 105424 spindelay 520 dequeue self 80977

*-Block time of ClogControlLock has increased slightly.*


Will continue with further tests on lower clients.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


Cache_data_in_GetSnapshotData_POC_01.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] Re: redo failed in physical streaming replication while stopping the master server

On Thu, Mar 3, 2016 at 6:58 PM, lannis  wrote:
> So in the replay scenario, before we read the page from wal segment file,
> using the specical RecPtr which point to the next page header address, can
> we predicat the page header is a long or short?

I am not sure I am getting what you are looking for, but if you mean
if we can predict it or not, the answer is yes. A long header is used
at the beginning of a WAL segment, by default 16MB, and the short
header at the beginning of a WAL page, or XLOG_BLCKSZ, 8kB by default.
-- 
Michael


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

Re: [HACKERS] proposal: psql autocomplete for casting

2016-03-03 12:06 GMT+01:00 Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp>:

> Hello, I considered on this,
>
> At Thu, 3 Mar 2016 10:05:06 +0100, Pavel Stehule 
> wrote in <
> cafj8prdz456okbpv9jdj_vcgtwprqxyu1kqp6z_eu_wgnvs...@mail.gmail.com>
> > We have  not autocomplete for casting introduced by symbol "::". A
> > implementation should not be hard.
>
> Completion needs a sequence of break characters between two
> succeeding words and the break characters cannot be a part of a
> word. If cast operators are allowed to be preceded by white
> space, the following diff gives maybe-desired behavior.
>
> ===
> --- a/src/bin/psql/tab-complete.c
> +++ b/src/bin/psql/tab-complete.c
> @@ -1322,6 +1322,8 @@ psql_completion(const char *text, int start, int end)
> else
> matches = complete_from_variables(text, ":", "",
> true);
> }
> +   else if (text[0] == ':' && text[1] == ':')
> +   COMPLETE_WITH_LIST3("::integer", "::text", "::date");
>
> /* If no previous word, suggest one of the basic sql commands */
> else if (previous_words_count == 0)
> ===
>
> > =# select 123 ::
> > ::DATE ::INTEGER  ::TEXT
> > =# select 123 ::T
> > =# select 123 ::TEXT
>
> It might not be instinctive but would be doable by making such a
> candidate list.
>
> 
> diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
> index 5f27120..179c9f0 100644
> --- a/src/bin/psql/tab-complete.c
> +++ b/src/bin/psql/tab-complete.c
> @@ -1322,6 +1322,8 @@ psql_completion(const char *text, int start, int end)
> else
> matches = complete_from_variables(text, ":", "",
> true);
> }
> +   else if (text[0] == ':' && text[1] == ':')
> +   COMPLETE_WITH_QUERY("SELECT distinct('::' || typname) FROM
> pg_type where typname !~'^(pg)?_.*'");
>
> /* If no previous word, suggest one of the basic sql commands */
> else if (previous_words_count == 0)
> 
>
> > =# select 2314 ::t
> > ::t ::tinterval
> > ::table_constraints ::transforms
> > ::table_privileges  ::trigger
> > ::tables::triggered_update_columns
> > ::text  ::triggers
> > ::tid   ::tsm_handler
> > ::time  ::tsquery
> > ::timestamp ::tsrange
> > ::time_stamp::tstzrange
> > ::timestamptz   ::tsvector
> > ::timetz::txid_snapshot
>
> Does this make sense?
>

the requirement of space before is not good :( - It should be any different
than operator chars. Not only space.

all other is perfect :)

Regards

Pavel


>
> regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
>
>

[HACKERS] Submit Pull Request

2016-03-03 Thread Maton, Brett

Hi,

  How do I push a branch to git.postgresql.org/git/pgrpms.git ?

Re: [HACKERS] proposal: psql autocomplete for casting

2016-03-03 Thread Kyotaro HORIGUCHI

Hello, I considered on this,

At Thu, 3 Mar 2016 10:05:06 +0100, Pavel Stehule  
wrote in 
> We have  not autocomplete for casting introduced by symbol "::". A
> implementation should not be hard.

Completion needs a sequence of break characters between two
succeeding words and the break characters cannot be a part of a
word. If cast operators are allowed to be preceded by white
space, the following diff gives maybe-desired behavior.

===
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1322,6 +1322,8 @@ psql_completion(const char *text, int start, int end)
else
matches = complete_from_variables(text, ":", "", true);
}
+   else if (text[0] == ':' && text[1] == ':')
+   COMPLETE_WITH_LIST3("::integer", "::text", "::date");
 
/* If no previous word, suggest one of the basic sql commands */
else if (previous_words_count == 0)
===

> =# select 123 ::
> ::DATE ::INTEGER  ::TEXT
> =# select 123 ::T
> =# select 123 ::TEXT 

It might not be instinctive but would be doable by making such a
candidate list.


diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 5f27120..179c9f0 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -1322,6 +1322,8 @@ psql_completion(const char *text, int start, int end)
else
matches = complete_from_variables(text, ":", "", true);
}
+   else if (text[0] == ':' && text[1] == ':')
+   COMPLETE_WITH_QUERY("SELECT distinct('::' || typname) FROM 
pg_type where typname !~'^(pg)?_.*'");
 
/* If no previous word, suggest one of the basic sql commands */
else if (previous_words_count == 0)


> =# select 2314 ::t
> ::t ::tinterval
> ::table_constraints ::transforms
> ::table_privileges  ::trigger
> ::tables::triggered_update_columns
> ::text  ::triggers
> ::tid   ::tsm_handler
> ::time  ::tsquery
> ::timestamp ::tsrange
> ::time_stamp::tstzrange
> ::timestamptz   ::tsvector
> ::timetz::txid_snapshot

Does this make sense?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

[HACKERS] Novice Presentation and Company Project

2016-03-03 Thread Eduardo Morras


Hello everyone, I'm Eduardo Morras from Spain.

My company is developing code for Postgresql for another company and want to 
communicate, debate and share the results with the community.

The objetives are upgrade the network backend and frontend of Postgresql:

a) Add support for LibreSSL in it 2 flavors, OpenSSL compatibility mode and own 
API,
b) Add support for sctp protocol,
c) Add support for dtls with LibreSSL, with udp/udplite and sctp datagrams,
d) Add support to them in makefiles/configure and postgresql configuration.

I have read the FAQ entry [1] and company contributions document [2] and as 
first step I had search Postgresql mailinglists for similar topics and found 
nothing. I think this is the second step, share the plan. If you need more 
information on any topic or has more ideas or howtos or.. reply mail.


[1] https://wiki.postgresql.org/wiki/So,_you_want_to_be_a_developer%3F
[2] http://momjian.us/main/writings/pgsql/company_contributions/

---   ---
Eduardo Morras 


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

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

Hi

2016-03-03 0:27 GMT+01:00 Jim Nasby :

> On 3/2/16 3:52 PM, Pavel Stehule wrote:
>
>> Right, and it's arguably dubious that that doesn't already work.
>> Unfortunately, these % things are just random plpgsql parser hacks,
>> not
>> real types.  Maybe this should be done in the main PostgreSQL parser
>> with parameter hooks, if we wanted this feature to be available
>> outside
>> plpgsql as well.
>>
>>
>> I am not fan to propagate this feature outside PLpgSQL - it is possible
>> new dependency between database object, and the cost is higher than
>> benefits.
>>
>
> I fail to see how it'd be a dependency. I'd expect it to look up the type
> when you run the command, just like plpgsql does. I think it'd be useful to
> have.
>

if we publish this feature to SQL, then somebody can use it in table
definition

CREATE TABLE a(a int);
CREATE TABLE b(a a.a%TYPE)

And the people expecting the living relation between table a and table b.
So when I do ALTER a.a, then b.a should be changed. What if I drop a.a or
drop a?

So this is reason, why I don't would this feature in SQL side.

Regards

Pavel


>
> That said, I think that should be a completely separate patch and
> discussion. Lets at least get it into plpgsql first.
>
> As for the array of element/element of array feature; I agree it would be
> nice, but we're pretty late in the game for that, and I don't see why that
> couldn't be added later.
>
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
>

Re: [HACKERS] [NOVICE] WHERE clause not used when index is used

2016-03-03 Thread Tobias Florek

> > > Reverted patch in HEAD and 9.5
> >
> > Is there an ETA?
> >
> 
> I just committed the fix to the repo.

Sorry for being unclear, is there an ETA for a new point-release?

Thank you,
 Tobias Florek


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

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

Hi

2016-02-24 22:18 GMT+01:00 Peter Eisentraut :

> On 1/18/16 4:21 PM, Robert Haas wrote:
> > One idea that occurs to me is: If you can DECLARE BAR FOO%TYPE, but
> > then you want to make BAR an array of that type rather than a scalar,
> > why not write that as DECLARE BAR FOO%TYPE[]?  That seems quite
> > natural to me.
>
> Right, and it's arguably dubious that that doesn't already work.
> Unfortunately, these % things are just random plpgsql parser hacks, not
> real types.  Maybe this should be done in the main PostgreSQL parser
> with parameter hooks, if we wanted this feature to be available outside
> plpgsql as well.
>
> > I think the part of this patch that makes %TYPE work for more kinds of
> > types is probably a good idea, although I haven't carefully studied
> > exactly what it does.
>
> I agree that this should be more general.  For instance, this patch
> would allow you to get the element type of an array-typed variable, but
> there is no way to get the element type of just another type.  If we
> could do something like
>
> DECLARE
>   var ELEMENT OF point;
>
> (not necessary that syntax)
>
> then
>
> DECLARE
>   var ELEMENT OF othervar%TYPE;
>
> should just fall into place.
>
>
I am sending update of this patch. The basic concept is same, syntax was
changed per your and Robert requirement.

Regards

Pavel
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
new file mode 100644
index 9786242..5587839
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
*** url varchar;
*** 322,334 
  myrow tablename%ROWTYPE;
  myfield tablename.columnname%TYPE;
  arow RECORD;
  
  
  
  
   The general syntax of a variable declaration is:
  
! name  CONSTANT  type  COLLATE collation_name   NOT NULL   { DEFAULT | := | = } expression ;
  
The DEFAULT clause, if given, specifies the initial value assigned
to the variable when the block is entered.  If the DEFAULT clause
--- 322,336 
  myrow tablename%ROWTYPE;
  myfield tablename.columnname%TYPE;
  arow RECORD;
+ myarray tablename.columnname%TYPE[];
+ myelement ELEMENT OF arrayparam%TYPE;
  
  
  
  
   The general syntax of a variable declaration is:
  
! name  CONSTANT   ELEMENT OF  type  COLLATE collation_name   NOT NULL   { DEFAULT | := | = } expression ;
  
The DEFAULT clause, if given, specifies the initial value assigned
to the variable when the block is entered.  If the DEFAULT clause
*** arow RECORD;
*** 337,342 
--- 339,347 
The CONSTANT option prevents the variable from being
assigned to after initialization, so that its value will remain constant
for the duration of the block.
+   The ELEMENT OF ensure using the element type of a given array type.
+   This construct is valuable in polymorphic functions, since the data types needed
+   for internal variables can change from one call to the next call.
The COLLATE option specifies a collation to use for the
variable (see ).
If NOT NULL
*** user_id users.user_id%TYPE;
*** 611,616 
--- 616,666 
  change from one call to the next.  Appropriate variables can be
  created by applying %TYPE to the function's
  arguments or result placeholders.
+ 
+ CREATE OR REPLACE FUNCTION array_init(v anyelement, size integer)
+ RETURNS anyarray AS $$
+ DECLARE
+ result v%TYPE[] DEFAULT '{}';
+ BEGIN
+ -- prefer builtin function array_fill
+ FOR i IN 1 .. size
+ LOOP
+ result := result || v;
+ END LOOP;
+ RETURN result;
+ END;
+ $$ LANGUAGE plpgsql;
+ 
+ SELECT array_init(0::numeric, 10);
+ SELECT array_init(''::varchar, 10);
+ 
+ 
+ CREATE OR REPLACE FUNCTION bubble_sort(a anyarray)
+ RETURNS anyarray AS $$
+ DECLARE
+ aux ELEMENT OF a%TYPE;
+ repeat_again boolean DEFAULT true;
+ BEGIN
+ -- Don't use this code for large arrays!
+ -- use builtin sort
+ WHILE repeat_again
+ LOOP
+ repeat_again := false;
+ FOR i IN array_lower(a, 1) .. array_upper(a, 1)
+ LOOP
+ IF a[i] > a[i+1] THEN
+ aux := a[i+1];
+ a[i+1] := a[i]; a[i] := aux;
+ repeat_again := true;
+ END IF;
+ END LOOP;
+ END LOOP;
+ RETURN a;
+ END;
+ $$ LANGUAGE plpgsql;
+ 
+ SELECT bubble_sort(ARRAY[3,2,4,6,1]);
+ 
 
  

diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
new file mode 100644
index 2aeab96..b77117e
*** a/src/pl/plpgsql/src/pl_comp.c
--- b/src/pl/plpgsql/src/pl_comp.c
*** plpgsql_parse_wordtype(char *ident)
*** 1646,1653 
  			case PLPGSQL_NSTYPE_VAR:
  return ((PLpgSQL_var *) (plpgsql_Datums[nse->itemno]))->datatype;
  
! /* XXX perhaps allow REC/ROW here? */
  
  			default:
  return NULL;
  		}
--- 1646,1660 
  			case PLPGSQL_NSTYPE_VAR:
  return ((PLpgSQL_var *)

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

2016-03-03 Thread Emre Hasegeli

> Emre, I checked original thread and didn't find sample data. Could you 
> provide them for testing ?

I found it on the Git history:

https://github.com/job/irrexplorer/blob/9e8b5330d7ef0022abbe1af18291257e044eb24b/data/irrexplorer_dump.sql.gz?raw=true


-- 
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][REVIEW]: Password identifiers, protocol aging and SCRAM protocol

2016-03-03 Thread Valery Popov

This is a review of "Password identifiers, protocol aging and SCRAM
protocol" patches

http://www.postgresql.org/message-id/CAB7nPqSMXU35g=w9x74hveqp0uvgjxvyoua4a-a3m+0wfeb...@mail.gmail.com

Contents & Purpose
--
There was a discussion dedicated to SCRAM:
http://www.postgresql.org/message-id/55192afe.6080...@iki.fi

This set of patches implements the following:
- Introduce in Postgres an extensible password aging facility, by having
a new concept of 1 user/multiple password verifier, one password
verifier per protocol.
- Give to system administrators tools to decide unsupported protocols,
and have pg_upgrade use that
- Introduce new password protocols for Postgres, aimed at replacing
existing, say limited ones.

This set of patches consists of 9 separate patches.
Description of each patch is well described in initial thread email and
in comments.
The first set of patches 0001-0008 adds facility to store multiple
password verifiers,
CREATE ROLE and ALTER ROLE are extended with PASSWORD VERIFIERS, new
superuser GUC parameters which specifies a list of supported password
protocols in Postgres backend, added pg_auth_verifiers_sanitize
function, removed password verifiers for unsupported protocols in
pg_upgrade, and more features.
The second set of patch 0005~0008 introduces a new protocol, SCRAM, and
0009 is SCRAM itself.

Initial Run
-
Included in the patches are:
- source code
- regression tests
- documentation
The source code is well commented.
The patches are in context diff format and were applied correctly to
HEAD (there were 2 warnings, and it was fixed by author).

There were several markup warnings, should be fixed by author.
Regression tests pass successfully, without errors. It seems that the
patches work as expected.
The patch 0009 depends on all previous patches 0001-0008: first we need
to apply patches 0001-0008, then 0009.

Performance
---
I have not tested possible performance issues yet.

Conclusion
--
I think introduced features are useful and I vote for commit +1.

On 03/02/2016 02:55 PM, Michael Paquier wrote:

On Wed, Mar 2, 2016 at 5:43 PM, Valery Popov wrote:
So the markup is missing. Thanks. I am taking note of it.

--
Regards,
Valery Popov
Postgres Professional http://www.postgrespro.com
The Russian Postgres Company

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

Re: [HACKERS] jsonb array-style subscription

2016-03-03 Thread Dmitry Dolgov

> If the patch were proposing a similar amount of new infrastructure to
> support some datatype-extensible concept of subscripting, I'd be much
> happier about it.

Well, actually, I agree with that. I can try to rework the patch to achieve
this goal.

Re: [HACKERS] [NOVICE] WHERE clause not used when index is used

2016-03-03 Thread Simon Riggs

On 3 March 2016 at 10:11, Tobias Florek  wrote:

> Hi,
>
> > Reverted patch in HEAD and 9.5
>
> Is there an ETA?
>

I just committed the fix to the repo.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] [NOVICE] WHERE clause not used when index is used

2016-03-03 Thread Tobias Florek

Hi,

> Reverted patch in HEAD and 9.5

Is there an ETA? We can't easily go back to 9.4 and while adding
additional conditions not to trigger this index only scan is possible,
but a little fragile.

Thank you for your work, btw. I was very surprised to find a bug in
PostgreSQL! I honestly still am.

Cheers,
 Tobias Florek


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

Re: [HACKERS] [NOVICE] WHERE clause not used when index is used

2016-03-03 Thread Simon Riggs

On 2 March 2016 at 10:57, Simon Riggs  wrote:

> On 1 March 2016 at 20:03, Tom Lane  wrote:
>
>
>> In any event, I am now of the opinion that this patch needs to be reverted
>> outright and returned to the authors for redesign.  There are too many
>> things wrong with it and too little reason to think that we have to have
>> it in 9.5.
>>
>
> Agreed; I'd already got to this thought while reading the thread.
>
> I'll get on with that.
>
> The patch is not of great importance to us? Since we are past 9.6 deadline
> it seems just worth reverting and leaving for next release to come back
> with a more isolated optimization. I don't want to add the last CF workload
> with this.
>

Reverted patch in HEAD and 9.5

Later, I will add the tests we discovered here to index scans, so that
further optimization work is more easily possible.

Thanks Tom, Petr for analysis; thanks Jeff for bisecting.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

[HACKERS] Re: redo failed in physical streaming replication while stopping the master server

2016-03-03 Thread lannis

Thanks for your reply.

If we only take replay for consideration, yeah, we do this header check
until we've read the page first.

But thanks to the master xlog generator, we know that:
when we try advance XLOG insert buffer (page), we treate the new page header
as short header at first.
then we use this condition to make it a long header.

if ((NewPage->xlp_pageaddr.xrecoff % XLogSegSize) == 0)
{
XLogLongPageHeader NewLongPage = (XLogLongPageHeader) NewPage;

NewLongPage->xlp_sysid = ControlFile->system_identifier;
NewLongPage->xlp_seg_size = XLogSegSize;
NewLongPage->xlp_xlog_blcksz = XLOG_BLCKSZ;
NewPage   ->xlp_info |= XLP_LONG_HEADER;

Insert->currpos = ((char *) NewPage) +SizeOfXLogLongPHD;
}

So in the replay scenario, before we read the page from wal segment file,
using the specical RecPtr which point to the next page header address, can
we predicat the page header is a long or short?

regards,

fanbin





--
View this message in context: 
http://postgresql.nabble.com/redo-failed-in-physical-streaming-replication-while-stopping-the-master-server-tp5889961p5890391.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.


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

Re: [HACKERS] WIP: Upper planner pathification

On 3 March 2016 at 18:04, Tom Lane  wrote:
> David Rowley  writes:
>> I agree that it would be good to get this in as soon as possible. I'm
>> currently very close to being done with writing Parallel Aggregate on
>> top of the upper planner changes. So far this version is much cleaner
>> as there's less cruft added compared with the other version,
>
> Cool!  If you come across any points where it seems like it could be
> done better or more easily, that would be tremendously valuable feedback
> at this stage.

Well since you mention it, I started on hash grouping and it was
rather simple and clean as in create_agg_path() I just created a chain
of the required paths and let create_plan() recursively build the
Finalize Aggregate -> Gather -> Partial Aggregate -> Seq Scan plan.
That was rather simple, and actually very nice when compared to how
things are handled in today's grouping planner. When it comes to Group
Aggregate I notice that you're using a RollupPath rather than an
AggPath even when there's no grouping sets. This also means that
create_agg_path() is only ever used for the AGG_HASHED strategy, even
though the 'strategy' is passed as a parameter to that function, so it
seemed prudent to me, to make sure all strategies are handled properly
there.

My gripe is that I've added the required code to build the parallel
group aggregate to create_agg_path() already, but since Group
Aggregate uses the RollupPath I'm forced to add code in
create_rollup_plan() which manually stacks up Plan nodes rather than
just dealing with Paths and create_plan() and its recursive call
magic.

I can't quite see any blocker for not doing this, so would you object
to separating out the treatment of Group Aggregate and Grouping Sets
in create_grouping_paths() ? I think it would require less code
overall.

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

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

Re: [HACKERS] Publish autovacuum informations

2016-03-03 Thread Kyotaro HORIGUCHI

Hello,

At Wed, 2 Mar 2016 17:48:06 -0600, Jim Nasby  wrote 
in <56d77bb6.6080...@bluetreble.com>
> On 3/2/16 10:48 AM, Julien Rouhaud wrote:
> > Good point, I don't see a lot of information available with this hooks
> > that a native system statistics couldn't offer. To have the same
> > amount
> > of information, I think we'd need a pg_stat_autovacuum view that shows
> > a
> > realtime insight of the workers, and also add some aggregated counters
> > to PgStat_StatTabEntry. I wonder if adding counters to
> > PgStat_StatTabEntry would be accepted though.
> 
> I would also really like to see a means of logging (auto)vacuum
> activity in the database itself. We figured out how to do that with
> pg_stat_statements, which was a lot harder... it seems kinda silly not
> to offer that for vacuum. Hooks plus shared memory data should allow
> for that (the only tricky bit is the hook would need to start and then
> commit a transaction, but that doesn't seem onerous).
> 
> I think the shared memory structures should be done as well. Having
> that real-time info is also valuable.
> 
> I don't see too much point in adding stuff to the stats system for
> this.

I wonder why there haven't been discussions so far on what kind
of information we want by this feature. For example I'd be happy
to see the time of last autovacuum trial and the cause if it has
been skipped for every table. Such information would (maybe)
naturally be shown in pg_stat_*_tables.

=
=# select relid, last_completed_autovacuum, last_completed_autovacv_status, 
last_autovacuum_trial, last_autovacuum_result from pg_stat_user_tables;
-[ RECORD 1 ]-+--
relid | 16390
last_completed_autovacuum | 2016-03-01 01:25:00.349074+09
last_completed_autovac_status | Completed in 4 seconds. Scanned 434 pages, 
skipped 23 pages
last_autovacuum_trial | 2016-03-03 17:33:04.004322+09
last_autovac_traial_status| Canceled by PID 2355. Processed 144/553 pages.
-[ RECORD 2 ]--+--
...
last_autovacuum_trial | 2016-03-03 07:25:00.349074+09
last_autovac_traial_status| Completed in 4 seconds. Scanned 434 pages, 
skipped 23 pages
-[ RECORD 3 ]--+--
...
last_autovacuum_trial | 2016-03-03 17:59:12.324454+09
last_autovac_trial_status | Processing by PID 42334, 564 / 32526 pages done.
-[ RECORD 4 ]--+--
...
last_autovacuum_trial | 2016-03-03 17:59:12.324454+09
last_autovac_trial_status | Skipped by dead-tuple threashold.
=

Apart from the appropriateness of the concrete shape, it would be
done by extending the current stats system and needs modification
of some other parts but the hooks and WorkerInfoData is not
enough. This might be a business of Rahila's "VACUUM Progress
Checker" and it convers some real-time info.

https://commitfest.postgresql.org/9/545/

On the other hand, it would be in another place and needs another
method if we want a history like the current autovacuum
completion logs (at debug3..) of 100 latest invocation of
autovacuum worker. Anyway the WorkerInfoData is not enough.


What kind of information we (will) want to have?


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] pl/pgsql exported functions