Re: [HACKERS] Choosing parallel_degree

[Robert Haas]

On Wed, Mar 16, 2016 at 1:23 PM, Julien Rouhaud
 wrote:
> On 16/03/2016 17:55, Robert Haas wrote:
>> On Wed, Mar 16, 2016 at 12:47 PM, Julien Rouhaud
>>  wrote:
>>> Something like a "min_parallel_degree" then ?
>>
>> Why not just parallel_degree without any prefix?  As in, when scanning
>> this table in parallel, the reloption suggests using N workers.
>>
>
> Agreed.
>
> PFA v2 that implements that.

I think create_parallel_paths shouldn't actually run the loop if the
reloption is specified; it should just adopt the specified value (or
max_parallel_degree, whichever is less).  Right now, you have it doing
the work to compute the default value but then overriding it.

Also, I think parallel_degree should be down in the section that says
/* information about a base rel (not set for join rels!) */ and I
think it should be called something like rel_parallel_degree, to make
it more clear that it's a value set on the relation level.

Let's leave out the parallel_threshold stuff for now.

-- 
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: Pushdown target list below gather node (WAS Re: [HACKERS] WIP: Upper planner pathification)

[Robert Haas]

On Wed, Mar 16, 2016 at 2:25 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Mar 16, 2016 at 2:02 PM, Tom Lane  wrote:
>>> Looks pretty close.  One point is that if we do end up using a Result
>>> node, then the parent GatherPath does not get charged for the Result
>>> node's cpu_per_tuple overhead.  I'm not sure that that's worth changing
>>> though.  It's probably better to bet that the subpath is projectable and
>>> so no cost will ensue, than to bet the other way.
>
>> I'm almost sure this way is the better bet.
>
> Actually, we do know what will happen ... so maybe
>
> /*
>  * We always use create_projection_path here, even if the subpath is
>  * projection-capable, so as to avoid modifying the subpath in place.
>  * It seems unlikely at present that there could be any other
>  * references to the subpath anyway, but better safe than sorry.
>  */
> +   if (!is_projection_capable_path(gpath->subpath))
> +   gpath->path.total_cost += cpu_tuple_cost * gpath->subpath->rows;
> gpath->subpath = (Path *)
> create_projection_path(root,
>gpath->subpath->parent,
>gpath->subpath,
>target);
>
> The comment could use adjustment if you adopt that, to reference the fact
> that we know create_projection_plan will get rid of the Result if not
> needed.

OK, I've committed something along those lines.  Thanks for the
advice, and feel free to whack it around if you have an idea for
improving it further - though IMHO this is good enough for 9.6.

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


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

[HACKERS] Patch: typo, s/espaced/escaped/

[Aleksander Alekseev]

There is a typo in one word in this commit:

http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=9a206d063c410df7cd5da01b169b23bff413fef5

Patch attached.

-- 
Best regards,
Aleksander Alekseev
http://eax.me/
diff --git a/contrib/unaccent/generate_unaccent_rules.py b/contrib/unaccent/generate_unaccent_rules.py
index 2f5520c..a5eb42f 100644
--- a/contrib/unaccent/generate_unaccent_rules.py
+++ b/contrib/unaccent/generate_unaccent_rules.py
@@ -95,7 +95,7 @@ def parse_cldr_latin_ascii_transliterator(latinAsciiFilePath):
 # to the characters.
 #
 # Group 1: plain "src" char. Empty if group 2 is not.
-# Group 2: unicode-espaced "src" char (e.g. "\u0110"). Empty if group 1 is not.
+# Group 2: unicode-escaped "src" char (e.g. "\u0110"). Empty if group 1 is not.
 #
 # Group 3: plain "trg" char. Empty if group 4 is not.
 # Group 4: plain "trg" char between quotes. Empty if group 3 is not.

-- 
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] Choosing parallel_degree

[Tom Lane]

Julien Rouhaud  writes:
> Shouldn't we also check "parallel_degree < max_worker_process" ?

> There's no need to compute any further than that. I think the best fix
> would be to add a CheckHook or AssignHook on max_parallel_degree GUC to
> make sure it's not more than max_worker_process.

Please, let's not go there.  Interdependent checks on GUC values are far
harder to get right than you think.  It's far better to design the GUC
specifications so that it doesn't matter.

For an example whereof I speak, check the sordid history of commit
ee1e5662d8d83307 ("Auto-tune effective_cache size to be 4x shared
buffers"), which eventually got reverted after a huge amount of thrashing
trying to make it work consistently.  Admittedly, that was trying to make
the default value of GUC X depend on GUC Y, but I think checking whether
X <= Y would have many of the same problems.  The core issue is you don't
know which one's going to get set first.

In this particular case I think it'd be fine to document that the
effective amount of parallelism is Min(parallel_degree,max_worker_process).

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] Make primnodes.h gender neutral

[Kevin Grittner]

On Thu, Mar 17, 2016 at 4:34 PM, David G. Johnston
 wrote:

> If we do this how many new developers are we expecting to subscribe to the
> -hackers list and make serious contributions - say, by reviewing the large
> backlog of patches we presently have?

I would certainly welcome even one new contributor who would do
this without causing any new feature to slip from the next release.
 ;-)

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


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

Re: [HACKERS] fd.c doesn't remove files on a crash-restart

[Andres Freund]

On 2016-03-16 11:02:09 -0700, Joshua D. Drake wrote:
> >>3. The problem can get worse over time. If you have a very long running
> >>instance, any time the backend crash-restarts you have to potential to
> >>increase disk space used for no purpose.
> >
> >But I think these outweigh the debugging benefit.
> 
> Well as Andrew said, we could also create postmaster start option that
> defaults to don't save.

I think these days you'd simply use restart_after_crash = false. For
debugging I found that to be rather valuable.


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] Performance degradation in commit ac1d794

[Andres Freund]

On 2016-03-19 18:45:36 -0700, Andres Freund wrote:
> On 2016-03-19 16:44:49 +0530, Amit Kapila wrote:
> > On Fri, Mar 18, 2016 at 1:34 PM, Andres Freund  wrote:
> > >
> > >
> > > Attached is a significantly revised version of the earlier series. Most
> > > importantly I have:
> > > * Unified the window/unix latch implementation into one file (0004)
> > >
> > 
> > After applying patch 0004* on HEAD, using command patch -p1 <
> > , I am getting build failure:
> > 
> > c1 : fatal error C1083: Cannot open source file:
> > 'src/backend/storage/ipc/latch.c': No such file or directory
> > 
> > I think it could not rename port/unix_latch.c => storage/ipc/latch.c.  I
> > have tried with git apply, but no success.  Am I doing something wrong?
> 
> You skipped applying 0003.
> 
> I'll send an updated version - with all the docs and such - in the next
> hours.

Here we go. I think this is getting pretty clos eto being committable,
minus a bit of testing edge cases on unix (postmaster death,
disconnecting clients in various ways (especially with COPY)) and
windows (uh, does it even work at all?).

There's no large code changes in this revision, mainly some code
polishing and a good bit more comment improvements.

Regards,

Andres
>From 9195a0136c44eca4d798d1de478a477ab0ac4724 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Fri, 18 Mar 2016 00:52:07 -0700
Subject: [PATCH 1/2] Combine win32 and unix latch implementations.

Previously latches for windows and unix had been implemented in
different files. The next patch in this series will introduce an
expanded wait infrastructure, keeping the implementation separate would
introduce too much duplication.

This basically just moves the functions, without too much change. The
reason to keep this separate is that it allows blame to continue working
a little less badly; and to make review a tiny bit easier.
---
 configure  |  10 +-
 configure.in   |   8 -
 src/backend/Makefile   |   3 +-
 src/backend/port/.gitignore|   1 -
 src/backend/port/Makefile  |   2 +-
 src/backend/port/win32_latch.c | 349 -
 src/backend/storage/ipc/Makefile   |   5 +-
 .../{port/unix_latch.c => storage/ipc/latch.c} | 280 -
 src/include/storage/latch.h|   2 +-
 src/tools/msvc/Mkvcbuild.pm|   2 -
 10 files changed, 277 insertions(+), 385 deletions(-)
 delete mode 100644 src/backend/port/win32_latch.c
 rename src/backend/{port/unix_latch.c => storage/ipc/latch.c} (74%)

diff --git a/configure b/configure
index a45be67..c10d954 100755
--- a/configure
+++ b/configure
@@ -14786,13 +14786,6 @@ $as_echo "#define USE_WIN32_SHARED_MEMORY 1" >>confdefs.h
   SHMEM_IMPLEMENTATION="src/backend/port/win32_shmem.c"
 fi
 
-# Select latch implementation type.
-if test "$PORTNAME" != "win32"; then
-  LATCH_IMPLEMENTATION="src/backend/port/unix_latch.c"
-else
-  LATCH_IMPLEMENTATION="src/backend/port/win32_latch.c"
-fi
-
 # If not set in template file, set bytes to use libc memset()
 if test x"$MEMSET_LOOP_LIMIT" = x"" ; then
   MEMSET_LOOP_LIMIT=1024
@@ -15868,7 +15861,7 @@ fi
 ac_config_files="$ac_config_files GNUmakefile src/Makefile.global"
 
 
-ac_config_links="$ac_config_links src/backend/port/dynloader.c:src/backend/port/dynloader/${template}.c src/backend/port/pg_sema.c:${SEMA_IMPLEMENTATION} src/backend/port/pg_shmem.c:${SHMEM_IMPLEMENTATION} src/backend/port/pg_latch.c:${LATCH_IMPLEMENTATION} src/include/dynloader.h:src/backend/port/dynloader/${template}.h src/include/pg_config_os.h:src/include/port/${template}.h src/Makefile.port:src/makefiles/Makefile.${template}"
+ac_config_links="$ac_config_links src/backend/port/dynloader.c:src/backend/port/dynloader/${template}.c src/backend/port/pg_sema.c:${SEMA_IMPLEMENTATION} src/backend/port/pg_shmem.c:${SHMEM_IMPLEMENTATION} src/include/dynloader.h:src/backend/port/dynloader/${template}.h src/include/pg_config_os.h:src/include/port/${template}.h src/Makefile.port:src/makefiles/Makefile.${template}"
 
 
 if test "$PORTNAME" = "win32"; then
@@ -16592,7 +16585,6 @@ do
 "src/backend/port/dynloader.c") CONFIG_LINKS="$CONFIG_LINKS src/backend/port/dynloader.c:src/backend/port/dynloader/${template}.c" ;;
 "src/backend/port/pg_sema.c") CONFIG_LINKS="$CONFIG_LINKS src/backend/port/pg_sema.c:${SEMA_IMPLEMENTATION}" ;;
 "src/backend/port/pg_shmem.c") CONFIG_LINKS="$CONFIG_LINKS src/backend/port/pg_shmem.c:${SHMEM_IMPLEMENTATION}" ;;
-"src/backend/port/pg_latch.c") CONFIG_LINKS="$CONFIG_LINKS src/backend/port/pg_latch.c:${LATCH_IMPLEMENTATION}" ;;
 "src/include/dynloader.h") CONFIG_LINKS="$CONFIG_LINKS src/include/dynloader.h:src/backend/port/dynloader/${template}.h" ;;
 "src/include/pg_config_os.h") CONFIG_LINKS="$CONFIG_LINKS 

[HACKERS] [PATCH v8] GSSAPI encryption support

[Robbie Harwood]

Hello friends,

A new version of my GSSAPI encryption patchset is available, both in
this email and on my github:
https://github.com/frozencemetery/postgres/tree/feature/gssencrypt8

What changed:

- Fixed fallback in the new server/old client case.  The server flag
  checking has been reduced.  In the (distant) future when all old
  clients are gone, the checking should be increased in strength once
  again.

- Fixed fallback in the old server/new client case.  This consists of
  checking whether the first message we receive after auth is done is an
  error, and raising it without decrypting if it is so that reconnection
  can happen if desired.  The quality of the fallback path has also
  generally been improved.

- Removed overzealous whitespace cleanup.  I'm a packager and have been
  bitten by upstreams doing this; I should have known better.

- Made flushing the AUTH_REQ_OK message conditional again.

- Fixed typo in server error message for insufficient GSSAPI protection.

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

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

diff --git a/configure b/configure
index b3f3abe..a5bd629 100755
--- a/configure
+++ b/configure
@@ -713,6 +713,7 @@ with_systemd
 with_selinux
 with_openssl
 krb_srvtab
+with_gssapi
 with_python
 with_perl
 with_tcl
@@ -5491,6 +5492,7 @@ $as_echo "$with_gssapi" >&6; }
 
 
 
+
 #
 # Kerberos configuration parameters
 #
diff --git a/configure.in b/configure.in
index 0bd90d7..4fd8f05 100644
--- a/configure.in
+++ b/configure.in
@@ -636,6 +636,7 @@ PGAC_ARG_BOOL(with, gssapi, no, [build with GSSAPI support],
   krb_srvtab="FILE:\$(sysconfdir)/krb5.keytab"
 ])
 AC_MSG_RESULT([$with_gssapi])
+AC_SUBST(with_gssapi)
 
 
 AC_SUBST(krb_srvtab)
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index e94d6a5..3dbc5c2 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -183,6 +183,7 @@ with_perl	= @with_perl@
 with_python	= @with_python@
 with_tcl	= @with_tcl@
 with_openssl	= @with_openssl@
+with_gssapi	= @with_gssapi@
 with_selinux	= @with_selinux@
 with_systemd	= @with_systemd@
 with_libxml	= @with_libxml@
diff --git a/src/backend/libpq/Makefile b/src/backend/libpq/Makefile
index 09410c4..a8cc9a0 100644
--- a/src/backend/libpq/Makefile
+++ b/src/backend/libpq/Makefile
@@ -21,4 +21,8 @@ ifeq ($(with_openssl),yes)
 OBJS += be-secure-openssl.o
 endif
 
+ifeq ($(with_gssapi),yes)
+OBJS += be-gssapi-common.o
+endif
+
 include $(top_srcdir)/src/backend/common.mk
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 57c2f48..73d493e 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -136,11 +136,7 @@ bool		pg_krb_caseins_users;
  *
  */
 #ifdef ENABLE_GSS
-#if defined(HAVE_GSSAPI_H)
-#include 
-#else
-#include 
-#endif
+#include "libpq/be-gssapi-common.h"
 
 static int	pg_GSS_recvauth(Port *port);
 #endif   /* ENABLE_GSS */
@@ -715,63 +711,6 @@ recv_and_check_password_packet(Port *port, char **logdetail)
  */
 #ifdef ENABLE_GSS
 
-#if defined(WIN32) && !defined(WIN32_ONLY_COMPILER)
-/*
- * MIT Kerberos GSSAPI DLL doesn't properly export the symbols for MingW
- * that contain the OIDs required. Redefine here, values copied
- * from src/athena/auth/krb5/src/lib/gssapi/generic/gssapi_generic.c
- */
-static const gss_OID_desc GSS_C_NT_USER_NAME_desc =
-{10, (void *) "\x2a\x86\x48\x86\xf7\x12\x01\x02\x01\x02"};
-static GSS_DLLIMP gss_OID GSS_C_NT_USER_NAME = _C_NT_USER_NAME_desc;
-#endif
-
-
-static void
-pg_GSS_error(int severity, char *errmsg, OM_uint32 maj_stat, OM_uint32 min_stat)
-{
-	gss_buffer_desc gmsg;
-	OM_uint32	lmin_s,
-msg_ctx;
-	char		msg_major[128],
-

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

[Michael Paquier]

On Sun, Mar 20, 2016 at 8:06 AM, Andrew Dunstan  wrote:
>
>
> On 03/08/2016 07:40 PM, Michael Paquier wrote:
>>
>> On Wed, Mar 9, 2016 at 1:07 AM, Andrew Dunstan 
>> wrote:
>>>
>>> Michael's patch proposes to replace the use of sed to generate probes.h
>>> with
>>> the perl equivalent everywhere. That has the advantage that we keep to
>>> one
>>> script to generate probes.h, but it does impose a perl dependency.
>>
>> Good point. It did not occur to me that this would bring a hard
>> dependency for non-Windows builds. Let's keep both scripts then. The
>> attached is changed to do so.
>
>
>
> Actually, it wasn't, but I fixed it and committed it.

Ah, indeed, thanks. It looks like I have been bitten by an incorrect
rebase/cherry-pick.

> Still to do: the non-perl pieces.

The patch to address locales is the sensitive part. The patch from
Petr is taking the correct approach though I think that we had better
simplify it a bit and if possible we had better not rely on the else
block it introduces.
-- 
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] initdb: introduce PG_CMD_PRINTF intestead of PG_CMD_PRINTF{1,2,3}

[Tom Lane]

Alexander Kuleshov  writes:
> The src/bin/initdb/initdb.c provides three macros to write data to
> cmdfd. All of these macro do the same, but with different amount of
> arguments for fprintf().

> Attached patch introduces PG_CMD_PRINTF macro which will take set of
> variadic arguments via __VA_ARGS__ to replace the PG_CMD_PRINTF{1,2,3}
> macros.

> Any objections?

Yes.  We do not accept patches that don't work on compilers without
__VA_ARGS__ (cf 7b077af50 for a recent example).  At some point there
might be a reason that's compelling enough to make us move that
portability goalpost, but this hardly seems like such a reason.

regards, tom lane


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

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

[Michael Paquier]

On Tue, Mar 15, 2016 at 6:38 PM, David Steele  wrote:
> Hi Michael,
>
> On 3/14/16 7:07 PM, Michael Paquier wrote:
>
>> On Mon, Mar 14, 2016 at 5:06 PM, Michael Paquier  
>> wrote:
>>
>>> On Mon, Mar 14, 2016 at 4:32 PM, David Steele  wrote:
>>>
 Could you provide an updated set of patches for review?  Meanwhile I am
 marking this as "waiting for author".
>>>
>>> Sure. I'll provide them shortly with all the comments addressed. Up to
>>> now I just had a couple of comments about docs and whitespaces, so I
>>> didn't really bother sending a new set, but this meritates a rebase.
>>
>> And here they are. I have addressed the documentation and the
>> whitespaces reported up to now at the same time.
>
> For this first review I would like to focus on the user visible changes
> introduced in 0001-0002.

Thanks for the input!

> 1) I see that rolvaliduntil is still in pg_authid:
> I think that's OK if we now define it to be "role validity" (it's still
> password validity in the patched docs).  I would also like to see a
> validuntil column in pg_auth_verifiers so we can track password
> expiration for each verifier separately.  For now I think it's enough to
> copy the same validity both places since there can only be one verifier.

FWIW, this is an intentional change, and my goal is to focus on only
the protocol aging for now. We will need to move rolvaliduntil to
pg_auth_verifiers if we want to allow rolling updates of password
verifiers for a given role, but that's a different patch, and we need
to think about the SQL interface carefully. This infrastructure makes
the move easier by the way to do that, and honestly I don't really see
what we gain now by copying the same value to two different system
catalogs.

> 2) I don't think the column naming in pg_auth_verifiers is consistent
> with other catalogs:
> postgres=# select * from pg_auth_verifiers;
>  roleid | verimet |   verival
> +-+-
>   16387 | m   | md505a671c66aefea124cc08b76ea6d30bb
>   16388 | p   | testu
>
> System catalogs generally use a 3 character prefix so I would expect the
> columns to be (if we pick avr as a prefix):

OK, this makes sense.

> avrrole
> avrmethod
> avrverifier

Assuming "ver" is the prefix, we get: verroleid, vermethod, vervalue.
I kind of like those ones, more than with "avr" as prefix actually.
Other ideas are of course welcome.

> I'm not a big fan in abbreviating too much so you can see I've expanded
> the names a bit.

Sure.

> 3) rolpassword is still in pg_shadow even though it is not useful anymore:
> postgres=# select usename, passwd, valuntil from pg_shadow;
>
>  usename |  passwd  |valuntil
> -+--+
>  vagrant |  |
>  test|  |
>  testu   |  | 2017-01-01 00:00:00+00
>
> If anyone is actually using this column in a meaningful way they are in
> for a nasty surprise when trying use the value in passwd as a verifier.
>  I would prefer to drop the column entirely and produce a clear error.
>
> Perhaps a better option would be to drop pg_shadow entirely since it
> seems to have no further purpose in life.

We discussed that on the previous thread and the conclusion was to
keep pg_shadow, but to clobber the password value with "***",
explaining this choice:
http://www.postgresql.org/message-id/6174.1455501...@sss.pgh.pa.us
-- 
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] improving GROUP BY estimation

[Dean Rasheed]

On 18 March 2016 at 00:37, Tomas Vondra  wrote:
>> On Sun, 2016-03-13 at 15:24 +, Dean Rasheed wrote:
>>> I think that a better formula to use would be
>>>
>>> reldistinct *= (1 - powl(1 - rel-rows / rel->tuples, rel->tuples /
>>> reldistinct)
>
> Attached is a v3 of the patch using this formula instead of the original
> one. Interestingly, that apparently reduces the number of regression tests
> that get broken to a single one.
>

Cool.


> I'm not sure whether we need to provide a link to the PDF the formula comes
> from - perhaps we should?
>

Probably a better URL to give is
http://www.adellera.it/investigations/distinct_balls/ which has a link
to the PDF version of the paper and also some supporting material.

However, while that paper is in general very clear, I don't think it
gives a very clear explanation of that particular formula, and it
doesn't explain what it represents. It merely says that if "i" can be
ignored "for some reason (e.g. i << Nr)", then that formula is an
approximation to the exact "without replacement" formula, which is the
subject of that paper.

But actually, that formula is the exact formula for the expected
number of distinct values when selecting *with replacement* from a
collection where each of the distinct values occurs an equal number of
times. So I think we should say that.

Perhaps something along the lines of:

/*
 * Update the estimate based on the restriction selectivity.
 *
 * This uses the formula for the expected number of distinct values
 * when selecting with replacement from a collection where each of
 * the distinct values occurs an equal number of times (a uniform
 * distribution of values). This is a very close approximation to
 * the more correct method of selecting without replacement when
 * the number of distinct values is quite large --- for example,
 * see http://www.adellera.it/investigations/distinct_balls/.
 * Additionally, it also turns out to work very well even when the
 * number of distinct values is small.
 */

Regards,
Dean


-- 
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] [PATH] Jsonb, insert a new value into an array at arbitrary position

[Dmitry Dolgov]

> I still don't like that this works on path leading to an object given
that we can't fulfill the promise of inserting to an arbitrary position
there.

I'm thinking about this following way - `jsonb_insert` is just a function
to insert a new value, which has specific options to enforce arbitrary
position in case of jsonb array. I don't think this can confuse someone
since it's not something like `jsonb_insert_at_position` function. But of
course, if I'm wrong we can easy change the function name and make it
available only for arrays.

Re: [HACKERS] Combining Aggregates

[David Rowley]

On 18 March 2016 at 13:39, Haribabu Kommi  wrote:
> On Thu, Mar 17, 2016 at 10:59 PM, Tomas Vondra
>  wrote:
>> Hi,
>>
>> On 03/17/2016 12:53 PM, David Rowley wrote:
>>>
>> ...
>>>
>>>
>>> I just had a quick skim over the patch and noticed the naming
>>> convention you're using for the combine function is *_pl, and you have
>>> float8_pl. There's already a function named float8pl() which is quite
>>> close to what you have. I've been sticking to *_combine() for these,
>>> so maybe float8_combine() and float8_regr_combine() are better names.
>>
>>
>> +1 to the _combine naming convention.
>
> Thanks for the input. Makes sense, updated patch is attached with
> the changes.

I've had a look over this. I had to first base it on the 0005 patch,
as it seemed like the pg_aggregate.h changes didn't include the
serialfn and deserialfn changes, and an OID was being consumed by
another function I added in patch 0003.

On testing I also noticed some wrong results, which on investigation,
are due to the wrong array elements being added together.

For example:

postgres=# select stddev(num) from f;
  stddev
--
 28867.5149028984
(1 row)


postgres=# set max_parallel_degree=8;
SET
postgres=# select stddev(num) from f;
 stddev

  0
(1 row)

+ N += transvalues2[0];
+ sumX += transvalues2[1];
+ CHECKFLOATVAL(sumX, isinf(transvalues1[1]) || isinf(transvalues2[1]), true);
+ sumX2 += transvalues1[2];

The last line should use transvalues2[2], not transvalues1[2].

There's also quite a few mistakes in float8_regr_combine()

+ sumX2 += transvalues2[2];
+ CHECKFLOATVAL(sumX2, isinf(transvalues1[2]) || isinf(transvalues2[1]), true);

Wrong array element on isinf() check

+ sumY2 += transvalues2[4];
+ CHECKFLOATVAL(sumY2, isinf(transvalues1[4]) || isinf(transvalues2[3]), true);

Wrong array element on isinf() check

+ sumXY += transvalues2[5];
+ CHECKFLOATVAL(sumXY, isinf(transvalues1[5]) || isinf(transvalues2[1]) ||
+  isinf(transvalues2[3]), true);

Wrong array element on isinf() check, and the final
isinf(transvalues2[3]) check does not need to be there.

I've re-based the patch and fixed these already, so will send updated
patches shortly.

-- 
 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] Make primnodes.h gender neutral

[Kevin Grittner]

On Thu, Mar 17, 2016 at 4:11 PM, Tom Lane  wrote:

> I think it's important that we fix these issues in a way that doesn't
> degrade the readability of the prose, and that doesn't call attention
> to itself as "hey, we're being so politically correct!".  We're trying
> to convey technical information in a way that does not distract the
> reader from the technical content.  Sexist language is a distraction
> for some, in-your-face non-sexism (such as made-up pronouns) is a
> distraction for others, bad or awkward grammar is a distraction for yet
> others.  It's not that easy to write prose that manages not to call
> attention to itself in any of these ways.  But that's what we need to
> do, and s/xxx/yyy/g editing that's only thinking about one of these
> concerns is unlikely to get us there.

+1

> I also concur with Alvaro that fixing these issues one para at a time
> is pretty inefficient.

A grep with a quick skim of the results to exclude references to
particular people who are mentioned by name and then referred to
with a pronoun (which I assume we can leave alone), suggest there
are about 70 lines in the 1346667 line C code base that need work.

Any word-smiths out there who want to volunteer to sort this out?

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


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

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

[Amit Kapila]

On Wed, Mar 16, 2016 at 7:50 PM, Constantin S. Pan  wrote:
>
> On Wed, 16 Mar 2016 18:08:38 +0530
> Amit Kapila  wrote:
>
> >
> > Why backend just waits, why can't it does the same work as any worker
> > does?  In general, for other parallelism features the backend also
> > behaves the same way as worker in producing the results if the
> > results from workers is not available.
>
> We can make backend do the same work as any worker, but that
> will complicate the code for less than 2 % perfomance boost.

Why do you think it will be just 2%?  I think for single worker case, it
should be much more as the master backend will be less busy in consuming
tuples from tuple queue.  I can't say much about code-complexity, as I
haven't yet looked carefully at the logic of patch, but we didn't find much
difficulty while doing it for parallel scans.  One of the commit which
might help you in understanding how currently heap scans are parallelised
is ee7ca559fcf404f9a3bd99da85c8f4ea9fbc2e92, you can see if that can help
you in anyway for writing a generic API for Gin parallel builds.


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

Re: [HACKERS] Parallel Aggregate

[David Rowley]

On 17 March 2016 at 01:19, Amit Kapila  wrote:
> Few assorted comments:
>
> 1.
>   /*
> + * Determine if it's possible to perform aggregation in parallel using
> + * multiple worker processes. We can permit this when there's at least one
> + * partial_path in input_rel, but not if the query has grouping sets,
> + * (although this likely just requires a bit more thought). We must also
> + * ensure that any aggregate functions which are present in either the
> + * target list, or in the HAVING clause all support parallel mode.
> + */
> + can_parallel = false;
> +
> + if ((parse->hasAggs || parse->groupClause != NIL) &&
> + input_rel->partial_pathlist != NIL &&
> + parse->groupingSets == NIL &&
> + root->glob->parallelModeOK)
>
> I think here you need to use has_parallel_hazard() with second parameter as
> false to ensure expressions are parallel safe. glob->parallelModeOK flag
> indicates that there is no parallel unsafe expression, but it can still
> contain parallel restricted expression.

Yes, I'd not gotten to fixing that per Robert's original comment about
it, but I think I have now.

> 2.
>  AggPath *
>  create_agg_path(PlannerInfo *root,
> @@ -2397,9 +2399,11 @@ create_agg_path(PlannerInfo *root,
>
> List *groupClause,
>   List *qual,
>   const AggClauseCosts
> *aggcosts,
> - double numGroups)
> + double numGroups,
> +
> bool combineStates,
> + bool finalizeAggs)
>
> Don't you need to set parallel_aware flag in this function as we do for
> create_seqscan_path()?

I don't really know the answer to that... I mean there's nothing
special done in nodeAgg.c if the node is running in a worker or in the
main process. So I guess the only difference is that EXPLAIN will read
"Parallel Partial (Hash|Group)Aggregate" instead of "Partial
(Hash|Group)Aggregate", is that desired? What's the logic behind
having "Parallel" in EXPLAIN?

> 3.
> postgres=# explain select count(*) from t1;
>   QUERY PLAN
>
> 
> --
>  Finalize Aggregate  (cost=45420.57..45420.58 rows=1 width=8)
>->  Gather  (cost=45420.35..45420.56 rows=2 width=8)
>  Number of Workers: 2
>  ->  Partial Aggregate  (cost=44420.35..44420.36 rows=1 width=8)
>->  Parallel Seq Scan on t1  (cost=0.00..44107.88 rows=124988
> wid
> th=0)
> (5 rows)
>
> Isn't it better to call it as Parallel Aggregate instead of Partial
> Aggregate.  Initialy, we have kept Partial for seqscan, but later on we
> changed to Parallel Seq Scan, so I am not able to think why it is better to
> call Partial incase of Aggregates.

I already commented on this.

> 4.
>   /*
> + * Likewise for any partial paths, although this case is more simple as
> +
>  * we don't track the cheapest path.
> + */
> + foreach(lc, current_rel->partial_pathlist)
> +
> {
> + Path   *subpath = (Path *) lfirst(lc);
> +
> + Assert(subpath->param_info ==
> NULL);
> + lfirst(lc) = apply_projection_to_path(root, current_rel,
> +
> subpath, scanjoin_target);
> + }
> +
>
>
> Can't we do this by teaching apply_projection_to_path() as done in the
> latest patch posted by me to push down the target list beneath workers [1].

Probably, but I'm not sure I want to go changing that now. The patch
is useful without it, so perhaps it can be a follow-on fix.

> 5.
> + /*
> + * If we find any aggs with an internal transtype then we must ensure
> + * that
> pointers to aggregate states are not passed to other processes,
> + * therefore we set the maximum degree
> to PAT_INTERNAL_ONLY.
> + */
> + if (aggform->aggtranstype == INTERNALOID)
> +
> context->allowedtype = PAT_INTERNAL_ONLY;
>
> In the above comment, you have refered maximum degree which is not making
> much sense to me. If it is not a typo, then can you clarify the same.

hmm. I don't quite understand the confusion. Perhaps you think of
"degree" in the parallel sense? This is talking about the levels of
degree of partial aggregation, which is nothing to do with parallel
aggregation, parallel aggregation just requires that to work. The
different. "Degree" in this sense was just meaning that PAY_ANY is the
highest degree, PAT_INTERNAL_ONLY lesser so etc. I thought the
"degree" thing was explained ok in the comment for
aggregates_allow_partial(), but perhaps I should just remove it, if
it's confusing.

Changed to:
/*
* If we find any aggs with an internal transtype then we must ensure
* that pointers to aggregate states are not passed to other processes,
* therefore we set the maximum allowed type to PAT_INTERNAL_ONLY.
*/

> 6.
> + * fix_combine_agg_expr
> + *  Like fix_upper_expr() but additionally adjusts the Aggref->args of
> + *  Aggrefs so
> that they references the corresponding Aggref in the subplan.
> + */
> +static Node *
> +fix_combine_agg_expr(PlannerInfo
> *root,
> +   Node *node,
> +   indexed_tlist *subplan_itlist,
> +
> Index newvarno,
> +   int rtoffset)
> +{
> + 

Re: [HACKERS] fd.c doesn't remove files on a crash-restart

[Joshua D. Drake]

On 03/16/2016 11:04 AM, Andres Freund wrote:

On 2016-03-16 11:02:09 -0700, Joshua D. Drake wrote:

3. The problem can get worse over time. If you have a very long running
instance, any time the backend crash-restarts you have to potential to
increase disk space used for no purpose.


But I think these outweigh the debugging benefit.


Well as Andrew said, we could also create postmaster start option that
defaults to don't save.


I think these days you'd simply use restart_after_crash = false. For
debugging I found that to be rather valuable.


That would have created an extended outage for this installation. I am 
not sure we can force that for something that should not happen in the 
first place (filling up the hard drive with dead files).


JD




Andres





--
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] [PATCH] Integer overflow in timestamp[tz]_part() and date/time boundaries check

[Vitaly Burovoy]

On 2016-03-16, Tom Lane  wrote:
> My feeling is we ought to preserve the old behavior here, which would
> involve making JULIAN_MAXYEAR_FOR_TIMESTAMPS format-dependent and
> adjusting the float values for the two derived constants; not much of a
> problem code-wise.  I think though that it would break quite a number of
> the proposed new regression tests for the float case.

I think I can be solved by adding new tests for new upper bound for
float values and creating a new version of expected file like
date_1.out, horology_1.out, timestamp_1.out and timestamptz.out (the
original files are for integer values; with prefix "_1" are for float
ones).

> TBH, I thought
> the number of added test cases was rather excessive anyway, so I wouldn't
> have a problem with just leaving out whichever ones don't pass with both
> build options.

The tests checks edge cases. In case of saving bigger interval of
allowed values for float values you'll remove checks when they should
fail. What's the left cases for?

On 2016-03-16, Tom Lane  wrote:
> Actually, it seems a lot of the provided test cases fail for float
> timestamps anyway; there's an assumption that
>   294276-12-31 23:59:59.99
>   294277-01-01 00:00:00.00
> are distinct timestamps, which they are not in float mode.

I'm ready to change fractional part '.99' to '.5' (to avoid issues
of different implementation of floating point on different
architectures) to emphasize "almost reached the threshold".

On 2016-03-16, Tom Lane  wrote:
> AFAICS the only way that we can avoid a dump/reload hazard is to tighten
> up the allowed range of timestamps by at least one day, so that any
> timestamp that passes IS_VALID_TIMESTAMP() is guaranteed to print, in
> any timezone, with a contained date that the Julian-day routines can
> handle.  I'd be inclined to set the lower limit of timestamps as
> '4713-01-01 00:00 GMT BC' just to keep things simple.  (The upper limit
> can stay where it is.)
>
> While dates don't have this timezone rotation problem, the documentation
> says that they have the same lower-bound limit as timestamps, and there
> are places in the code that assume that too.  Is it okay to move their
> lower bound as well?

I think it is important (see initial letter) since it is out of
supported interval (according to the documentation) and don't work as
expected in some cases (see "to_char(date_trunc('week', '4714-12-28
BC'::date),'day')"). It seems it leads to change
IS_VALID_JULIAN[_FOR_STAMPS] as well to something like:

#define IS_VALID_JULIAN(y,m,d) \
((JULIAN_MINYEAR < (y) && (y) < JULIAN_MAXYEAR)
 ||(JULIAN_MINYEAR == (y) && (m) == 12 && (d) == 31)
 ||(JULIAN_MAXYEAR == (y) && (m) == 01 && (d) == 01))

It can be correct if checks for IS_VALID_DATE is added in date.c to
places marked as "IS_VALID_JULIAN is enough for checking..."
All other places are (I'm sure) covered by IS_VALID_DATE/IS_VALID_TIMESTAMP.

What about the question:
On 2016-02-24, Vitaly Burovoy  wrote:
> Also I'm asking for a help because the query (in default TZ='GMT+1'):
> postgres=# SELECT '4714-11-24 00:00:00.00+00 BC'::timestamptz;
>
> in psql gives a result "4714-11-23 23:00:00-01 BC",
> but in a testing system gives "Sun Nov 23 23:00:00 4714 GMT BC"
> without TZ offset.

Why there is "GMT" instead of "GMT+01"? Is it bug? If so should it be
fixed in this patch or can be fixed later?

-- 
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] btree_gin and btree_gist for enums

[Andrew Dunstan]

On 03/18/2016 09:54 AM, Robert Haas wrote:

On Thu, Mar 17, 2016 at 11:21 AM, Andrew Dunstan  wrote:

On 03/17/2016 06:44 AM, Andrew Dunstan wrote:

Here is a patch to add enum support to btree_gin and btree_gist. I didn't
include distance operations, as I didn't think it terribly important, and
there isn't a simple way to compute it sanely and efficiently, so no KNN
support.

This time including the data file for the gist regression tests.

Are you intending this as a 9.7 submission?  Because it's pretty late for 9.6.




I guess so. I would certainly find it hard to argue that it should be in 
9.6 unless there is a consensus on it.


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] Make primnodes.h gender neutral

[Joshua D. Drake]

On 03/17/2016 02:11 PM, Tom Lane wrote:

"Joshua D. Drake"  writes:

On 03/17/2016 01:36 PM, Alvaro Herrera wrote:

+1 what?  Are you saying this patch is good?  I don't think it is: the
previous sentence is written in third person, and the following ones are
currently in third person, but the patch changes the following sentences
to first person without changing the first one to match.  If he or she
(or they) want this updated, I think we should at least make an effort
of keeping it as consistent as it is today.



The wording was Meh to begin with. If you would like me to spend some
time cleaning up the paragraph as a whole, I will do that.


I think it's important that we fix these issues in a way that doesn't
degrade the readability of the prose,


I don't disagree.


and that doesn't call attention
to itself as "hey, we're being so politically correct!".  We're trying
to convey technical information in a way that does not distract the
reader from the technical content.  Sexist language is a distraction
for some, in-your-face non-sexism (such as made-up pronouns) is a
distraction for others, bad or awkward grammar is a distraction for yet
others.





I also concur with Alvaro that fixing these issues one para at a time
is pretty inefficient.


How else are we going to do it? If we use sed, we are basically going to 
end up with the "hey, we're being so politically correct!" issue. The 
only other way I can think to fix it is to fix it as we come across it. 
If you are in a file and see the gender issue, just fix it as part of 
the patch you are working on.


JD



regards, tom lane




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

[David Steele]

Hi Michael,

On 3/14/16 7:07 PM, Michael Paquier wrote:
> On Mon, Mar 14, 2016 at 5:06 PM, Michael Paquier
>  wrote:
>> On Mon, Mar 14, 2016 at 4:32 PM, David Steele  wrote:
>>> Could you provide an updated set of patches for review?  Meanwhile I am
>>> marking this as "waiting for author".
>>
>> Sure. I'll provide them shortly with all the comments addressed. Up to
>> now I just had a couple of comments about docs and whitespaces, so I
>> didn't really bother sending a new set, but this meritates a rebase.
> 
> And here they are. I have addressed the documentation and the
> whitespaces reported up to now at the same time.

Here's my full review of this patch set.

First let me thank you for submitting this patch for the current CF.  I
feel a bit guilty that I requested it and am only now posting a full
review.  In my defense I can only say that being CFM has been rather
more work than I was expecting, but I'm sure you know the feeling.

* [PATCH 1/9] Add facility to store multiple password verifiers

This is a pretty big patch but I went through it carefully and found
nothing to complain about.  Your attention to detail is impressive as
always.

Be sure to update the column names for pg_auth_verifiers as we discussed
in [1].

* [PATCH 2/9] Introduce password_protocols

diff --git a/src/test/regress/expected/password.out
b/src/test/regress/expected/password.out
+SET password_protocols = 'plain';
+ALTER ROLE role_passwd5 PASSWORD VERIFIERS (plain = 'foo'); -- ok
+ALTER ROLE role_passwd5 PASSWORD VERIFIERS (md5 = 'foo'); -- error
+ERROR:  specified password protocol not allowed
+DETAIL:  List of authorized protocols is specified by password_protocols.

So that makes sense but you get the same result if you do:

postgres=# alter user role_passwd5 password 'foo';
ERROR:  specified password protocol not allowed
DETAIL:  List of authorized protocols is specified by password_protocols.

I don't think this makes sense - if I have explicitly set
password_protocols to 'plain' and I don't specify a verifier for alter
user then it seems like it should work.  If nothing else the error
message lacks information needed to identify the problem.

* [PATCH 3/9] Add pg_auth_verifiers_sanitize

This function is just a little scary but since password_protocols
defaults to 'plain,md5' I can live with it.

* [PATCH 4/9] Remove password verifiers for unsupported protocols in
pg_upgrade

Same as above - it will always be important for password_protocols to
default to *all* protocols to avoid data being dropped during the
pg_upgrade by accident.  You've done that here (and later in the SCRAM
patch) so I'm satisfied but it bears watching.

What I would do is add some extra comments in the GUC code to make it
clear to always update the default when adding new verifiers.

* [PATCH 5/9] Move sha1.c to src/common

This looks fine to me and is a good reuse of code.

* [PATCH 6/9] Refactor sendAuthRequest

I tested this across different client versions and it seems to work fine.

* [PATCH 7/9] Refactor RandomSalt to handle salts of different lengths

A simple enough refactor.

* [PATCH 8/9] Move encoding routines to src/common/

A bit surprising that these functions were never used by any front end code.

* Subject: [PATCH 9/9] SCRAM authentication

diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
@@ -1616,18 +1619,34 @@ FlattenPasswordIdentifiers(List *verifiers, char
*rolname)
 * instances of Postgres, an md5 hash passed as a plain verifier
 * should still be treated as an MD5 entry.
 */
-   if (spec->veriftype == AUTH_VERIFIER_MD5 &&
-   !isMD5(spec->value))
+   switch (spec->veriftype)
{
-   char encrypted_passwd[MD5_PASSWD_LEN + 1];
-   if (!pg_md5_encrypt(spec->value, rolname, 
strlen(rolname),
-   
encrypted_passwd))
-   elog(ERROR, "password encryption failed");
-   spec->value = pstrdup(encrypted_passwd);
+   case AUTH_VERIFIER_MD5:

It seems like this case statement should have been introduced in patch
0001.  Were you just trying to avoid churn in the code unless SCRAM is
committed?

diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
+
+static char *
+read_attr_value(char **input, char attr)
+{

Numerous functions like the above in auth-scram.c do not have comments.

diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
+   else if (strcmp(token->string, "scram") == 0)
+   {
+   if (Db_user_namespace)
+   {
+   ereport(LOG,
+   (errcode(ERRCODE_CONFIG_FILE_ERROR),
+errmsg("SCRAM authentication is not 
supported when

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

[David Steele]

On 3/9/16 3:29 AM, Kyotaro HORIGUCHI wrote:

> Hello, thank you for the comments. The new v8 patch is attched.

As far as I can see this patch should be marked "ready for review" so
now it is.

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


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

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

[Jesper Pedersen]

On 03/15/2016 01:17 AM, Amit Kapila wrote:

I have updated the comments and changed the name of one of a variable from
"all_trans_same_page" to "all_xact_same_page" as pointed out offlist by
Alvaro.




I have done a run, and don't see any regressions.

Intel Xeon 28C/56T @ 2GHz w/ 256GB + 2 x RAID10 (data + xlog) SSD.

I can provide perf / flamegraph profiles if needed.

Thanks for working on this !

Best regards,
 Jesper


-- 
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] Choosing parallel_degree

[Robert Haas]

On Wed, Mar 16, 2016 at 12:47 PM, Julien Rouhaud
 wrote:
> Something like a "min_parallel_degree" then ?

Why not just parallel_degree without any prefix?  As in, when scanning
this table in parallel, the reloption suggests using N workers.

-- 
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] Make primnodes.h gender neutral

[Joshua D. Drake]

On 03/17/2016 01:36 PM, Alvaro Herrera wrote:

Robert Haas wrote:

On Thu, Mar 17, 2016 at 3:31 PM, Joshua D. Drake  wrote:

Per the twitter verse, here is an updated version of primnodes.h


+1.


+1 what?  Are you saying this patch is good?  I don't think it is: the
previous sentence is written in third person, and the following ones are
currently in third person, but the patch changes the following sentences
to first person without changing the first one to match.  If he or she
(or they) want this updated, I think we should at least make an effort
of keeping it as consistent as it is today.


The wording was Meh to begin with. If you would like me to spend some 
time cleaning up the paragraph as a whole, I will do that.




I *hope* this isn't the start of a trend to patch 1500 files one by one,
each on its own thread.  That's going to be annoying and noisy, achieve
nothing useful(*), and cause backpatching pain that the "twitter
verse"(**) is not going to help with.


So we have two choices I see:

1. As we come across the gender issue, we fix it as well as incorporate 
a gender neutral requirement for all documentation unless the gender is 
relevant to the context.


2. We do "one big patch".

#2 seems to be a really bad idea.



(*) I'm probably going to be expelled from the project for saying this,
but I very much doubt that female coders stay away from PostgreSQL just
because some files say "he" in comments rather than "she" or "he or she"
or "one" or "they".  They probably have different reasons for staying
away from the project.


Wanna bet? There is a very loud movement about this. We can either:

A. Start fighting about it

B. Just fix it, it doesn't matter anyway and it doesn't hurt the quality 
of the code or the documentation


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] pgbench - allow backslash-continuations in custom scripts

[Robert Haas]

On Thu, Mar 17, 2016 at 4:12 AM, Kyotaro HORIGUCHI
 wrote:
> Thank you for the comment, but could you please tell me what kind
> of criteria should I take to split this patch? The discussion
> about splitting criteria is in the following reply (in the
> sentence begins with "By the way").

Well, I'm trying to find a piece of this patch that is small enough
that I can understand it and in good enough shape that I can commit it
independently, but I am having some difficulty with that.  I keep
hoping some other committer is going to come along and be able to grok
this well enough to apply it based on what you've already done, but so
far it seems to be the all-me show.

> These patches are made so as to keep the compilable and workable
> state of the source files. It might be a bit more readable if
> unshackled from such restriction.

Keeping it compilable and workable after each patch is essential, but
the first patch is still big and doing a lot of stuff.  I'm wondering
if it can be further decomposed.

>>  I see this went to psqlscan_int.h, but there's no
>> obvious reason for that particular name, and the comments don't explain it;
>
> I assumed that is a convention of naming by looking libpq-int.h
> (though it doesn't use underscore, but hyphen). But the file
> needs a comment like libpq-int.h. I'll rename it and add such
> comment to it.

OK.

>> -   yyless(0);
>> +   my_yyless(0);
>>
>> Why do we need to do this?  Is "my_" really the best prefix?  Is this
>> another change that could be its own patch?
>
> Oops! Sorry for the silly name. I was not able to think up a
> proper name for it. Does psqlscan_yyless seems good?

That does sound better.

-- 
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] Performance degradation in commit ac1d794

[Andres Freund]

On 2016-03-16 15:08:07 -0400, Robert Haas wrote:
> On Wed, Mar 16, 2016 at 2:52 PM, Andres Freund  wrote:
> > How about the following sketch of an API
> >
> > typedef struct LatchEvent
> > {
> > uint32 events; /* interesting events */
> > uint32 revents;  /* returned events */
> > int fd; /* fd associated with event */
> > } LatchEvent;
> >
> > typedef struct LatchEventSet
> > {
> > int nevents;
> > LatchEvent events[FLEXIBLE_ARRAY_MEMBER];
> > } LatchEventSet;
> >
> > /*
> >  * Create a LatchEventSet with space for nevents different events to wait 
> > for.
> >  *
> >  * latch may be NULL.
> >  */
> > extern LatchEventSet *CreateLatchEventSet(int nevents, Latch *latch);
> 
> We might be able to rejigger this so that it didn't require palloc, if
> we got rid of FLEXIBLE_ARRAY_MEMBER and passed int nevents and
> LatchEvent * separately to WaitLatchThingy().  But I guess maybe this
> will be infrequent enough not to matter.

I think we'll basically end up allocating them once for the frequent
callsites.


> > - Given current users we don't need a large amount of events, so having
> >   to iterate through the registered events doesn't seem bothersome. We
> >   could however change the api to be something like
> >
> >   int WaitLatchEventSet(LatchEventSet *set, OccurredEvents *, int nevents, 
> > long timeout);
> >
> >   which would return the number of events that happened, and would
> >   basically "fill" one of the (usually stack allocated) OccurredEvent
> >   structures with what happened.
> 
> I definitely think something along these lines is useful.  I want to
> be able to have an Append node with 100 ForeignScans under it and kick
> off all the scans asynchronously and wait for all of the FDs at once.

So you'd like to get only an event for the FD with data back? Or are you
ok with iterating through hundred elements in an array, to see which are
ready?

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] Make primnodes.h gender neutral

[Tom Lane]

Chapman Flack  writes:
> On 03/17/16 17:29, Kevin Grittner wrote:
>> A grep with a quick skim of the results to exclude references to
>> particular people who are mentioned by name and then referred to
>> with a pronoun (which I assume we can leave alone), suggest there
>> are about 70 lines in the 1346667 line C code base that need work.
>> 
>> Any word-smiths out there who want to volunteer to sort this out?

> So that must be N affected files for some N <= 70 ...

> what would you think of starting a wiki page with those N filenames
> (so nobody has to repeat your grepping/skimming effort), and volunteers
> can claim a file or five, marking them taken on that page, and wordsmith
> away?

Yeah, Kevin, could you post your results?  I'd have guessed there were
more trouble spots than that.  If that really is the size of the problem,
seems like we could fix all those instances in one patch and be done
with it.  (At least till new ones sneak in :-()

regards, tom lane


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

Re: [HACKERS] Idle In Transaction Session Timeout, revived

[Pavel Stehule]

2016-03-16 17:54 GMT+01:00 Vik Fearing :

> On 03/16/2016 05:32 PM, Robert Haas wrote:
>
> > Committed with slight changes to the docs, and I added a flag variable
> > instead of relying on IdleInTransactionSessionTimeout not changing at
> > an inopportune time.
>
> Thank you!
>

great, important feature

Thank you

Pavel


> --
> Vik Fearing  +33 6 46 75 15 36
> http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Re: [HACKERS] Performance degradation in commit ac1d794

[Andres Freund]

On 2016-03-19 16:44:49 +0530, Amit Kapila wrote:
> On Fri, Mar 18, 2016 at 1:34 PM, Andres Freund  wrote:
> >
> >
> > Attached is a significantly revised version of the earlier series. Most
> > importantly I have:
> > * Unified the window/unix latch implementation into one file (0004)
> >
> 
> After applying patch 0004* on HEAD, using command patch -p1 <
> , I am getting build failure:
> 
> c1 : fatal error C1083: Cannot open source file:
> 'src/backend/storage/ipc/latch.c': No such file or directory
> 
> I think it could not rename port/unix_latch.c => storage/ipc/latch.c.  I
> have tried with git apply, but no success.  Am I doing something wrong?

You skipped applying 0003.

I'll send an updated version - with all the docs and such - in the next
hours.

> One minor suggestion about patch:
> 
> +#ifndef WIN32
>  void
>  latch_sigusr1_handler(void)
>  {
>   if (waiting)
>   sendSelfPipeByte();
>  }
> +#endif   /* !WIN32 */
> 
>  /* Send one byte to the self-pipe, to wake up WaitLatch */
> +#ifndef WIN32
>  static void
>  sendSelfPipeByte(void)
> 
> 
> Instead of individually defining these functions under #ifndef WIN32, isn't
> it better to combine them all as they are at end of file.

They're all at the end of the file. I just found it easier to reason
about if both #if and #endif are visible in one screen full of
code. Don't feel super strong about it tho.

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] Performance degradation in commit ac1d794

[Andres Freund]

On 2016-03-19 15:43:27 +0530, Amit Kapila wrote:
> On Sat, Mar 19, 2016 at 12:40 PM, Andres Freund  wrote:
> >
> > On March 18, 2016 11:52:08 PM PDT, Amit Kapila 
> wrote:
> > >> >Won't the new code needs to ensure that ResetEvent(latchevent)
> > >should
> > >> >get
> > >> >called in case WaitForMultipleObjects() comes out when both
> > >> >pgwin32_signal_event and latchevent are signalled at the same time?

> > >> WaitForMultiple only reports the readiness of on event at a time, no?
> > >>
> > >
> > >I don't think so, please read link [1] with a focus on below paragraph
> > >which states how it reports the readiness or signaled state when
> > >multiple
> > >objects become signaled.
> > >
> > >"When *bWaitAll* is *FALSE*, this function checks the handles in the
> > >array
> > >in order starting with index 0, until one of the objects is signaled.
> > >If
> > >multiple objects become signaled, the function returns the index of the
> > >first handle in the array whose object was signaled."

I think this is just incredibly bad documentation. See
https://blogs.msdn.microsoft.com/oldnewthing/20150409-00/?p=44273
(Raymond Chen can be considered an authority here imo).

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] Performance degradation in commit ac1d794

[Andres Freund]

On 2016-02-08 17:49:18 +0900, Kyotaro HORIGUCHI wrote:
> How about allowing registration of a callback for every waiting
> socket. The signature of the callback function would be like

I don't think a a callback based API is going to serve us well. Most of
the current latch callers would get noticeably more complex that
way. And a number of them will benefit from latches using epoll
internally.

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] Reworks of CustomScan serialization/deserialization

[Petr Jelinek]

On 15/03/16 05:03, Kouhei Kaigai wrote:

Petr,

The attached patch is the revised one that follows the new extensible-
node routine.

It is almost same the previous version except for:
- custom-apis.[ch] was renamed to custom-node.[ch]
- check for the length of custom-scan-method name followed
   the manner of RegisterExtensibleNodeMethods()



Hi,

looks good, only nitpick I have is that it probably should be 
custom_node.h with underscore given that we use underscore everywhere 
(except for libpq and for some reason atomic ops).


--
  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] Using quicksort for every external sort run

[Robert Haas]

On Thu, Mar 10, 2016 at 9:54 PM, Peter Geoghegan  wrote:
> 1. Creates a separate memory context for tuplesort's copies of
> caller's tuples, which can be reset at key points, avoiding
> fragmentation. Every SortTuple.tuple is allocated there (with trivial
> exception); *everything else*, including the memtuples array, is
> allocated in the existing tuplesort context, which becomes the parent
> of this new "caller's tuples" context. Roughly speaking, that means
> that about half of total memory for the sort is managed by each
> context in common cases. Even with a high work_mem memory budget,
> memory fragmentation could previously get so bad that tuplesort would
> in effect claim a share of memory from the OS that is *significantly*
> higher than the work_mem budget allotted to its sort. And with low
> work_mem settings, fragmentation previously made palloc() thrash the
> sort, especially during non-final merging. In this latest revision,
> tuplesort now almost gets to use 100% of the memory that was requested
> from the OS by palloc() is cases tested.

I spent some time looking at this part of the patch yesterday and
today.  This is not a full review yet, but here are some things I
noticed:

- I think that batchmemtuples() is somewhat weird.  Normally,
grow_memtuples() doubles the size of the array each time it's called.
So if you somehow called this function when you still had lots of
memory available, it would just double the size of the array.
However, I think the expectation is that it's only going to be called
when availMem is less than half of allowedMem, in which case we're
going to get the special "last increment of memtupsize" behavior,
where we expand the memtuples array by some multiple between 1.0 and
2.0 based on allowedMem/memNowUsed.  And after staring at this for a
long time ... yeah, I think this does the right thing.  But it
certainly is hairy.

- It's not exactly clear what you mean when you say that the tuplesort
context contains "everything else".  I don't understand why that only
ends up containing half the memory ... what, other than the memtuples
array, ends up there?

- If I understand correctly, the point of the MemoryContextReset call
is: there wouldn't be any tuples in memory at that point anyway.  But
the OS-allocated chunks might be divided up into a bunch of small
chunks that then got stuck on freelists, and those chunks might not be
the right size for the next merge pass.  Resetting the context avoids
that problem by blowing up the freslists.  Right?  Clever.

- I haven't yet figured out why we use batching only for the final
on-the-fly merge pass, instead of doing it for all merges.  I expect
you have a reason.  I just don't know what it is.

- I have also not yet figured out why you chose to replace
state->datumTypByVal with state->tuples and reverse the sense.  I bet
there's a reason for this, too.  I don't know what it is, either.

That's as far as I've gotten thus far.

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


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

Re: [HACKERS] 2016-03 Commitfest

[David Steele]

On 3/18/16 11:25 AM, Andreas Karlsson wrote:


The COPY RAW patch seems to have two entries in the commitfest.

https://commitfest.postgresql.org/9/223/ and
https://commitfest.postgresql.org/9/547/

Are those about the same patch?


Indeed they are - good catch!  I've responded on the thread and closed #547.

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


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

Re: [HACKERS] Performance degradation in commit ac1d794

[Robert Haas]

On Thu, Mar 17, 2016 at 9:01 AM, Robert Haas  wrote:
> I'll look at 0005 next, but thought I would send these comments along first.

0005: This is obviously very much WIP, but I think the overall
direction of it is good.
0006: Same.

I think you should use PGINVALID_SOCKET rather than -1 in various
places in various patches in this series, especially if you are going
to try to merge the Windows code path.

I wonder if CreateEventSet should accept a MemoryContext argument.  It
seems like callers will usually want TopMemoryContext, and just being
able to pass that might be more convenient than having to switch back
and forth in the calling code.

I wonder if there's a way to refactor this code to avoid having so
much cut-and-paste duplication.

When iterating over the returned events, maybe check whether events is
0 at the top of the loop and skip it forthwith if so.

That's all I've got for now.

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

[Valery Popov]

Hi!

On 03/15/2016 06:59 PM, Michael Paquier wrote:
The set of patches I am proposing here does not go through those code 
paths, and this is likely an aggregate failure. 

Michael, you were right. It was incorrect installation of contrib binaries.
Now all tests pass OK, both check-world and installcheck-world,
Thanks.

--
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] [PATCH] Phrase search ported to 9.6

[Dmitry Ivanov]

Hi, Artur

I've made an attempt to fix some of the issues you've listed, although there's 
still much work to be done. I'll add some comments later.

> This function has the duplicated piece from the tsvector_setweight()
> from tsvector_op.c. You can define new function for it.

I'm not sure it's worth the trouble. IMO these functions are relatively small 
and we won't benefit from extracting the duplicate code.

> These functions was removed in 9acb9007de30b3daaa9efc16763c3bc6e3e0a92d.
> It seems that tsvector_op.c was not synchronized with the master.

Got it, thanks!

-- 
Dmitry Ivanov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
diff --git a/contrib/tsearch2/expected/tsearch2.out b/contrib/tsearch2/expected/tsearch2.out
index 972f764..97379e7 100644
--- a/contrib/tsearch2/expected/tsearch2.out
+++ b/contrib/tsearch2/expected/tsearch2.out
@@ -278,15 +278,15 @@ SELECT '(!1|2)&3'::tsquery;
 (1 row)
 
 SELECT '1|(2|(4|(5|6)))'::tsquery;
- tsquery 
--
- '1' | ( '2' | ( '4' | ( '5' | '6' ) ) )
+   tsquery   
+-
+ '1' | '2' | '4' | '5' | '6'
 (1 row)
 
 SELECT '1|2|4|5|6'::tsquery;
- tsquery 
--
- ( ( ( '1' | '2' ) | '4' ) | '5' ) | '6'
+   tsquery   
+-
+ '1' | '2' | '4' | '5' | '6'
 (1 row)
 
 SELECT '1&(2&(4&(5&6)))'::tsquery;
@@ -340,7 +340,7 @@ select 'a' > 'b & c'::tsquery;
 select 'a | f' < 'b & c'::tsquery;
  ?column? 
 --
- t
+ f
 (1 row)
 
 select 'a | ff' < 'b & c'::tsquery;
@@ -443,9 +443,9 @@ select count(*) from test_tsquery where keyword >  'new & york';
 
 set enable_seqscan=on;
 select rewrite('foo & bar & qq & new & york',  'new & york'::tsquery, 'big & apple | nyc | new & york & city');
- rewrite  
---
- 'foo' & 'bar' & 'qq' & ( 'city' & 'new' & 'york' | ( 'nyc' | 'big' & 'apple' ) )
+   rewrite
+--
+ 'foo' & 'bar' & 'qq' & ( 'nyc' | 'big' & 'apple' | 'city' & 'new' & 'york' )
 (1 row)
 
 select rewrite('moscow', 'select keyword, sample from test_tsquery'::text );
@@ -461,9 +461,9 @@ select rewrite('moscow & hotel', 'select keyword, sample from test_tsquery'::tex
 (1 row)
 
 select rewrite('bar &  new & qq & foo & york', 'select keyword, sample from test_tsquery'::text );
-   rewrite   
--
- 'citi' & 'foo' & ( 'bar' | 'qq' ) & ( 'nyc' | ( 'big' & 'appl' | 'new' & 'york' ) )
+ rewrite 
+-
+ ( 'nyc' | 'big' & 'appl' | 'new' & 'york' ) & 'citi' & 'foo' & ( 'bar' | 'qq' )
 (1 row)
 
 select rewrite( ARRAY['moscow', keyword, sample] ) from test_tsquery;
@@ -479,9 +479,9 @@ select rewrite( ARRAY['moscow & hotel', keyword, sample] ) from test_tsquery;
 (1 row)
 
 select rewrite( ARRAY['bar &  new & qq & foo & york', keyword, sample] ) from test_tsquery;
-   rewrite   
--
- 'citi' & 'foo' & ( 'bar' | 'qq' ) & ( 'nyc' | ( 'big' & 'appl' | 'new' & 'york' ) )
+ rewrite 
+-
+ ( 'nyc' | 'big' & 'appl' | 'new' & 'york' ) & 'citi' & 'foo' & ( 'bar' | 'qq' )
 (1 row)
 
 select keyword from test_tsquery where keyword @> 'new';
@@ -520,9 +520,9 @@ select rewrite( ARRAY[query, keyword, sample] ) from test_tsquery, to_tsquery('e
 (1 row)
 
 select rewrite( ARRAY[query, keyword, sample] ) from test_tsquery, to_tsquery('english', 'bar &  new & qq & foo & york') as query where keyword <@ query;
-   rewrite   
--
- 'citi' & 'foo' & ( 'bar' | 'qq' ) & ( 'nyc' | ( 'big' & 'appl' | 'new' & 'york' ) )
+ rewrite 
+-
+ ( 'nyc' | 'big' & 'appl' | 'new' & 'york' ) & 'citi' & 'foo' & ( 'bar' | 'qq' )
 (1 row)
 
 select rewrite( ARRAY[query, keyword, sample] ) from test_tsquery, to_tsquery('english', 'moscow') as query where query @> keyword;
@@ -538,9 +538,9 @@ 

Re: [HACKERS] fd.c doesn't remove files on a crash-restart

[Joshua D. Drake]

On 03/16/2016 10:56 AM, Andres Freund wrote:


I understand that this is designed this way. I think it is a bad idea
because:

1. The majority crash-restarts in the wild are going to be diagnosed rather
easily within the OS itself. They fall into things like OOM killer and out
of disk space.


I don't buy 1), like at all. I've seen numerous production instances
with crashes outside of os triggered things.


I don't argue that. I argue that 9 times out of 10, it will not be one 
of those things but 




3. The problem can get worse over time. If you have a very long running
instance, any time the backend crash-restarts you have to potential to
increase disk space used for no purpose.


But I think these outweigh the debugging benefit.


Well as Andrew said, we could also create postmaster start option that 
defaults to don't save.


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] fd.c: flush data problems on osx

[Andres Freund]

Hi,

On 2016-03-17 20:09:53 +0300, Stas Kelvich wrote:
> Current implementation of pg_flush_data when called with zero offset and zero 
> nbytes is assumed to flush all file. In osx it uses mmap with these 
> arguments, but according to man: 
> 
> "[EINVAL]   The len argument was negative or zero. Historically, the 
> system call would not return an
> error if the argument was zero.  See other potential 
> additional restrictions in the COMPAT-
> IBILITY section below."
> 
> so it is generate a lot of warnings:
> 
> "WARNING:  could not mmap while flushing dirty data: Invalid argument"

Hm, yea, that's buggy.


> One possible solution for that is just fallback to pg_fdatasync in case when 
> offset = nbytes = 0.

Hm, that's a bit heavyweight. I'd rather do an lseek(SEEK_END) to get
the file size. Could you test that?


> Also there are no default ifdef inside this function, is there any
> check that will guarantee that pg_flush_data will not end up with
> empty body on some platform?

There doesn't need to be - it's purely "advisory", i.e. just an
optimization.

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] GinPageIs* don't actually return a boolean

[Michael Paquier]

On Fri, Mar 18, 2016 at 8:36 PM, Yury Zhuravlev
 wrote:
> Robert Haas wrote:
>>
>> On Wed, Mar 2, 2016 at 9:48 PM, Peter Eisentraut  wrote:
>>>
>>> On 2/11/16 9:30 PM, Michael Paquier wrote: ...
>>
>>
>> We need to decide what to do about this.  I disagree with Peter: I
>> think that regardless of stdbool, what we've got right now is sloppy
>> coding - bad style if nothing else.  Furthermore, I think that while C
>> lets you use any non-zero value to represent true, our bool type is
>> supposed to contain only one of those two values.  Therefore, I think
>> we should commit the full patch, back-patch it as far as somebody has
>> the energy for, and move on.  But regardless, this patch can't keep
>> sitting in the CommitFest - we either have to take it or reject it,
>> and soon.
>>
>
> I know that we are trying to do the right thing. But right now there is an
> error only in ginStepRight. Maybe now the fix this place, and we will think
> about "bool" then? The patch is attached (small and simple).

FWIW, when compiling with MS 2015 using the set of perl scripts I am
not seeing this compilation error... We may want to understand first
what kind of dependency is involved when doing the cmake build
compared to what is done with src/tools/msvc.
-- 
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] Performance degradation in commit ac1d794

[Robert Haas]

On Wed, Mar 16, 2016 at 3:25 PM, Andres Freund  wrote:
>> > - Given current users we don't need a large amount of events, so having
>> >   to iterate through the registered events doesn't seem bothersome. We
>> >   could however change the api to be something like
>> >
>> >   int WaitLatchEventSet(LatchEventSet *set, OccurredEvents *, int nevents, 
>> > long timeout);
>> >
>> >   which would return the number of events that happened, and would
>> >   basically "fill" one of the (usually stack allocated) OccurredEvent
>> >   structures with what happened.
>>
>> I definitely think something along these lines is useful.  I want to
>> be able to have an Append node with 100 ForeignScans under it and kick
>> off all the scans asynchronously and wait for all of the FDs at once.
>
> So you'd like to get only an event for the FD with data back? Or are you
> ok with iterating through hundred elements in an array, to see which are
> ready?

I'd like to get an event back for the FD with data.  Iterating sounds
like it could be really slow.  Say you get lots of little packets back
from the same connection, while the others are idle.  Now you've got
to keep iterating through them all over and over again.  Blech.

-- 
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] async replication code

[otheus uibk]

On Wed, Mar 16, 2016 at 8:30 PM, otheus uibk  wrote:

> I will ask again: where in the code is the asynchronous replication code?
> The docs are not detailed/exact enough.
>
>
Nevermind. Thomas Munro answered my question in the General list. Quoting:

> Look for WalSndWakeupRequest() in xlog.c, which expands to a call to
> WalSndWakeup in walsender.c which sets latches (= a mechanism for
> waking processes) on all walsenders, and see the WaitLatchOrSocket
> calls in walsender.c which wait for that to happen.


-- 
Otheus
otheus.u...@gmail.com
otheus.shell...@uibk.ac.at

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

[Ashutosh Bapat]

On Wed, Mar 16, 2016 at 10:22 PM, Tom Lane  wrote:

> Robert Haas  writes:
> > On Wed, Mar 16, 2016 at 4:10 AM, Ashutosh Bapat <
> > ashutosh.ba...@enterprisedb.com> wrote:
> >> In 9.5, postgres_fdw allowed to prepare statements involving foreign
> >> tables without an associated user mapping as long as planning did not
> >> require the user mapping. Remember, planner would require user mapping
> in
> >> case use_remote_estimate is ON for that foreign table. The user mapping
> >> would be certainly required at the time of execution. So executing such
> a
> >> prepared statement would throw error. If somebody created a user mapping
> >> between prepare and execute, execute would not throw an error. A join
> can
> >> be pushed down only when user mappings associated with the joining
> >> relations are known and they match. But given the behavior of 9.5 we
> should
> >> let the prepare succeed, even if the join can not be pushed down
> because of
> >> unknown user mapping. Before this fix, the test was not letting the
> prepare
> >> succeed, which is not compliant with 9.5.
>
> > If a query against a single table with no user mapping is legal, I don't
> > see why pushing down a join between two tables neither of which has a
> user
> > mapping shouldn't also be legal.
>
> The key point here is that Ashutosh is arguing on the basis of the
> behavior of postgres_fdw, which is not representative of all FDWs.
> The core code has no business assuming that all FDWs require user
> mappings; file_fdw is a counterexample.
>
>
Here's what the patch is doing.

The "core" code FDW is given chance to add paths for a join between foreign
relations if they are from the same (valid) server and have same user
mappings, even if the user mapping is invalid. This is code in
build_join_rel(). So, from core code's perspective it's perfectly fine to
push a join down when both sides do not have valid user mapping associated
with them. So, file_fdw is capable of implementing join pushdown even
without having user mapping.

postgres_fdw code however is different. Rest of the discussion only
pertains to postgres_fdw. The comment in postgres_fdw.sql testcase, to
which Robert objected, is relevant only for postgres_fdw.

postgres_fdw requires user mapping to execute the query. For base foreign
tables, it's fine not to have a user mapping at the time of planning as
long as planner doesn't need to query the foreign server. But it certainly
requires a user mapping at the time of execution, or else it throws error
at the time of execution. So, Robert's statement "If a query against a
single table with no user mapping is legal" is not entirely correct in the
context of postgres_fdw; postgresGetForeignRelSize(), which is called
during planning, can throw error if it doesn't find a valid user mapping.
If a join between two postgres_fdw foreign relations without valid user
mappings is decided to be pushed down at the time of planning, it will
require a user mapping at the time of execution. This user mapping is
derived from the joining relations recursively. If all of them have same
user mapping (even if invalid) it's fine. If it's invalid, we will throw
error at the time of execution. But if they acquire different user
mappings, postgres_fdw can not fire the join query. It can not use any of
the user mappings since that will compromise security. So, we have landed
with a plan which can not be executed. To be on the safer side,
postgres_fdw does not push a join down if joining sides do not have a valid
user mapping. A base foreign relation with postgres_fdw will require a user
mapping, for all serious uses. So, it's not likely that we will end up with
joins between postgres_fdw foreign relations with invalid user mapping.

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

Re: [HACKERS] [GENERAL] Request - repeat value of \pset title during \watch interations

[Tom Lane]

David Steele  writes:
> On 3/17/16 5:07 PM, David G. Johnston wrote:
>> Figured out it had to be added to 2016-09...done

> Hmm ... this patch is currently marked "needs review" in CF 2016-03.  Am
> I missing something, should this have been closed?

The message I saw was post-1-March.  If it was in fact submitted in
time for 2016-03, then we owe it a review.

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] POC, WIP: OR-clause support for indexes

[Tomas Vondra]

Hi Teodor,

Sadly the v4 does not work for me - I do get assertion failures. For 
example with the example Andreas Karlsson posted in this thread:


CREATE EXTENSION btree_gin;
CREATE TABLE test (a int, b int, c int);
CREATE INDEX ON test USING gin (a, b, c);
INSERT INTO test SELECT i % 7, i % 9, i % 11 FROM generate_series(1, 
100) i;

EXPLAIN ANALYZE SELECT * FROM test WHERE (a = 3 OR b = 5) AND c = 2;

It seems working, but only until I run ANALYZE on the table. Once I do 
that, I start getting crashes at this line


*qualcols = list_concat(*qualcols,
list_copy(idx_path->indexqualcols));

in convert_bitmap_path_to_index_clause. Apparently one of the lists is 
T_List while the other one is T_IntList, so list_concat() errors out.


My guess is that the T_BitmapOrPath branch should do

oredqualcols = list_concat(oredqualcols, li_qualcols);
...
*qualcols = list_concat(qualcols, oredqualcols);

instead of

oredqualcols = lappend(oredqualcols, li_qualcols);
...
*qualcols = lappend(*qualcols, oredqualcols);

but once I fixed that I got some other assert failures further down, 
that I haven't tried to fix.


So the patch seems to be broken, and I suspect this might be related to 
the broken index condition reported by Andreas (although I don't see 
that - I either see correct condition or assertion failures).



On 03/17/2016 06:19 PM, Teodor Sigaev wrote:
...


7) I find it rather ugly that the paths are built by converting BitmapOr
paths. Firstly, it means indexes without amgetbitmap can't benefit from
this change. Maybe that's reasonable limitation, though?

I based on following thoughts:
1 code which tries to find OR-index path will be very similar to existing
  generate_or_bitmap code. Obviously, it should not be duplicated.
2 all existsing indexes have amgetbitmap method, only a few don't.
amgetbitmap
  interface is simpler. Anyway, I can add an option for generate_or_bitmap
  to use any index, but, in current state it will just repeat all work.


I agree that the code should not be duplicated, but is this really a 
good solution. Perhaps a refactoring that'd allow sharing most of the 
code would be more appropriate.




But more importantly, this design already has a bunch of unintended
consequences. For example, the current code completely ignores
enable_indexscan setting, because it merely copies the costs from the
bitmap path.

>

I'd like to add separate enable_indexorscan


That may be useful, but why shouldn't enable_indexscan=off also disable 
indexorscan? I would find it rather surprising if after setting 
enable_indexscan=off I'd still get index scans for OR-clauses.





That's pretty dubious, I guess. So this code probably needs to be made
aware of enable_indexscan - right now it entirely ignores startup_cost
in convert_bitmap_path_to_index_clause(). But of course if there are
multiple IndexPaths, the  enable_indexscan=off will be included multiple
times.


... and it does not address this at all.

I really doubt a costing derived from the bitmap index scan nodes will 
make much sense - you essentially need to revert unknown parts of the 
costing to only include building the bitmap once, etc.




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] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

[Robert Haas]

On Wed, Mar 16, 2016 at 4:10 AM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

>
> Now what's going on here?  It seems to me that either postgres_fdw
>> requires a user mapping (in which case this ought to fail) or it
>> doesn't (in which case this ought to push the join down).  I don't
>> understand how working but not pushing the join down can ever be the
>> right behavior.
>>
> In 9.5, postgres_fdw allowed to prepare statements involving foreign
> tables without an associated user mapping as long as planning did not
> require the user mapping. Remember, planner would require user mapping in
> case use_remote_estimate is ON for that foreign table. The user mapping
> would be certainly required at the time of execution. So executing such a
> prepared statement would throw error. If somebody created a user mapping
> between prepare and execute, execute would not throw an error. A join can
> be pushed down only when user mappings associated with the joining
> relations are known and they match. But given the behavior of 9.5 we should
> let the prepare succeed, even if the join can not be pushed down because of
> unknown user mapping. Before this fix, the test was not letting the prepare
> succeed, which is not compliant with 9.5.
>

If a query against a single table with no user mapping is legal, I don't
see why pushing down a join between two tables neither of which has a user
mapping shouldn't also be legal.

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

Re: [HACKERS] unexpected result from to_tsvector

[Artur Zakirov]

I found the discussion about allowing an underscore in emails 
http://www.postgresql.org/message-id/200908281359.n7sdxfaf044...@wwwmaster.postgresql.org


That bug report is about recognizing an underscore in the local part of 
an email. And is not about recognizing an underscore in a domain name. 
But that patch allows an underscore in recognized host names also.


I am not good in RFC, so I put excerpt from Wikipedia 
https://en.wikipedia.org/wiki/Email_address:



The local-part of the email address may use any of these ASCII characters:

Uppercase and lowercase Latin letters (A–Z, a–z) (ASCII: 65–90, 97–122)
Digits 0 to 9 (ASCII: 48–57)
These special characters: !#$%&'*+-/=?^_`{|}~ (ASCII: 33, 35–39, 42, 43, 45, 
47, 61, 63, 94–96, 123–126)
Character . (dot, period, full stop), ASCII 46, provided that it is not the 
first or last character, and provided also that it does not appear 
consecutively (e.g. john.@example.com is not allowed).
Other special characters are allowed with restrictions (they are only allowed 
inside a quoted string, as described in the paragraph below, and in addition, a 
backslash or double-quote must be preceded by a backslash). These characters 
are:
Space and "(),:;<>@[\] (ASCII: 32, 34, 40, 41, 44, 58, 59, 60, 62, 64, 91–93)
Comments are allowed with parentheses at either end of the local part; e.g. 
john.smith(comment)@example.com and (comment)john.sm...@example.com are both 
equivalent to john.sm...@example.com.


and https://en.wikipedia.org/wiki/Hostname#Restrictions_on_valid_host_names


The Internet standards (Requests for Comments) for protocols mandate that 
component hostname labels may contain only the ASCII letters 'a' through 'z' 
(in a case-insensitive manner),the digits '0' through '9', and the hyphen 
('-'). The original specification of hostnames in RFC 952, mandated that labels 
could not start with a digit or with a hyphen, and must not end with a hyphen. 
However, a subsequent specification (RFC 1123) permitted hostname labels to 
start with digits. No other symbols, punctuation characters, or white space are 
permitted.


Hence the valid emails is (I might be wrong):

12...@sample.com
12...@sample.com
1...@123-sample.com
1...@123sample.com

The attached patch allow them to be recognized as a email. But this 
patch does not prohibit underscore in recognized host names.


As a result this patch gives the following results with underscores:

=# select * from ts_debug('simple', 'aaa@123_yyy.zzz');
 alias |  description  |  token  | dictionaries | dictionary | 
 lexemes

---+---+-+--++---
 email | Email address | aaa@123_yyy.zzz | {simple} | simple | 
{aaa@123_yyy.zzz}

(1 row)

=# select * from ts_debug('simple', '123_yyy.zzz');
 alias | description |token| dictionaries | dictionary | 
lexemes

---+-+-+--++---
 host  | Host| 123_yyy.zzz | {simple} | simple | 
{123_yyy.zzz}

(1 row)

On 14.03.2016 17:45, Artur Zakirov wrote:

On 14.03.2016 16:22, Shulgin, Oleksandr wrote:


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

=# select ts_debug('simple', 'a...@123-yyy.zzz');
  ts_debug
---

  (email,"Email
address",a...@123-yyy.zzz,{simple},simple,{a...@123-yyy.zzz})
(1 row)

But:

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

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

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

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

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

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

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

In fact, the 123-yyy.zzz domain is not valid either according to the RFC

Re: [HACKERS] IF (NOT) EXISTS in psql-completion

[Michael Paquier]

On Wed, Mar 16, 2016 at 2:42 AM, Robert Haas  wrote:
> On Fri, Feb 26, 2016 at 2:37 AM, Kyotaro HORIGUCHI
>  wrote:
>> Hello, this is the new version of this patch.
>
> The CommitFest entry for this patch is messed up.  It shows no author,
> even though I'm pretty sure that a patch has to have one by
> definition.

The CF app does not require an author name when the entry is first
created. The author needs to be added afterwards. A message-id, a
description and a category are the mandatory things.
-- 
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] Make primnodes.h gender neutral

[Tom Lane]

"Joshua D. Drake"  writes:
> On 03/17/2016 01:36 PM, Alvaro Herrera wrote:
>> +1 what?  Are you saying this patch is good?  I don't think it is: the
>> previous sentence is written in third person, and the following ones are
>> currently in third person, but the patch changes the following sentences
>> to first person without changing the first one to match.  If he or she
>> (or they) want this updated, I think we should at least make an effort
>> of keeping it as consistent as it is today.

> The wording was Meh to begin with. If you would like me to spend some 
> time cleaning up the paragraph as a whole, I will do that.

I think it's important that we fix these issues in a way that doesn't
degrade the readability of the prose, and that doesn't call attention
to itself as "hey, we're being so politically correct!".  We're trying
to convey technical information in a way that does not distract the
reader from the technical content.  Sexist language is a distraction
for some, in-your-face non-sexism (such as made-up pronouns) is a
distraction for others, bad or awkward grammar is a distraction for yet
others.  It's not that easy to write prose that manages not to call
attention to itself in any of these ways.  But that's what we need to
do, and s/xxx/yyy/g editing that's only thinking about one of these
concerns is unlikely to get us there.

I also concur with Alvaro that fixing these issues one para at a time
is pretty inefficient.

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: Pushdown target list below gather node (WAS Re: [HACKERS] WIP: Upper planner pathification)

[Tom Lane]

Robert Haas  writes:
> On Wed, Mar 16, 2016 at 12:36 PM, Tom Lane  wrote:
>> And there is a larger problem with this: I'm not sure that it's
>> appropriate for apply_projection_to_path to assume that the subpath is not
>> shared with any other purposes.  If it is shared, and we update the
>> subpath's target in-place, we just broke the other path chains.

> That's true.  I don't see an obvious hazard here, because the Gather's
> child came from the rel's partial_pathlist, and the only way it gets
> used from there is to stick the Gather on top of it.  So it really
> can't show up anywhere else.  I think.

The key question I think is could there ever be more than one Gather
sharing the same subpath?

> (To some lesser extent, apply_projection_to_path is always
> scary like that.)

Right, that's why there's also create_projection_path for when you
aren't sure.

> Mmmph.  That seems like a 2-bit solution, but I guess it would work.
> What if we taught create_projection_plan() to elide the Result node in
> that case?

Yeah, I was thinking about the same thing.  The comment block above
where you're looking would need some adjustment.

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] fd.c doesn't remove files on a crash-restart

[Robert Haas]

On Wed, Mar 16, 2016 at 2:05 PM, Tom Lane  wrote:
> "Joshua D. Drake"  writes:
>> Hello,
>> fd.c[1] will remove files from pgsql_tmp on a restart but not a
>> crash-restart per this comment:
>
>> /*
>> * NOTE: we could, but don't, call this during a post-backend-crash restart
>> * cycle.  The argument for not doing it is that someone might want to
>> examine
>> * the temp files for debugging purposes.  This does however mean that
>> * OpenTemporaryFile had better allow for collision with an existing temp
>> * file name.
>> */
>
>> I understand that this is designed this way. I think it is a bad idea
>> because:
>
> Possible compromise: remove files only in non-Assert builds?

That sorta seems like tying two things together that aren't obviously
related.  I think building with --enable-cassert is support to enable
debugging cross-checks, not change behavior.

I would vote for removing them always, and if somebody doesn't want to
remove them, well then they can comment the code out.

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


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

Re: [HACKERS] unexpected result from to_tsvector

[Dmitrii Golub]

2016-03-14 16:22 GMT+03:00 Shulgin, Oleksandr 
:

> On Mon, Mar 7, 2016 at 10:46 PM, Artur Zakirov 
> wrote:
>
>> Hello,
>>
>> On 07.03.2016 23:55, Dmitrii Golub wrote:
>>
>>>
>>>
>>> Hello,
>>>
>>> Should we added tests for this case?
>>>
>>
>> I think we should. I have added tests for teo...@123-stack.net and
>> 1...@stack.net emails.
>>
>>
>>> 123_reg.ro  is not valid domain name, bacause of
>>> symbol "_"
>>>
>>> https://tools.ietf.org/html/rfc1035 page 8.
>>>
>>> Dmitrii Golub
>>>
>>
>> Thank you for the information. Fixed.
>
>
> Hm...  now that doesn't look all that consistent to me (after applying the
> patch):
>
> =# select ts_debug('simple', 'a...@123-yyy.zzz');
>  ts_debug
> ---
>  (email,"Email address",a...@123-yyy.zzz,{simple},simple,{a...@123-yyy.zzz})
> (1 row)
>
> But:
>
> =# select ts_debug('simple', 'aaa@123_yyy.zzz');
> ts_debug
> -
>  (asciiword,"Word, all ASCII",aaa,{simple},simple,{aaa})
>  (blank,"Space symbols",@,{},,)
>  (uint,"Unsigned integer",123,{simple},simple,{123})
>  (blank,"Space symbols",_,{},,)
>  (host,Host,yyy.zzz,{simple},simple,{yyy.zzz})
> (5 rows)
>
> One can also see that if we only keep the domain name, the result is
> similar:
>
> =# select ts_debug('simple', '123-yyy.zzz');
>ts_debug
> ---
>  (host,Host,123-yyy.zzz,{simple},simple,{123-yyy.zzz})
> (1 row)
>
> =# select ts_debug('simple', '123_yyy.zzz');
>   ts_debug
> -
>  (uint,"Unsigned integer",123,{simple},simple,{123})
>  (blank,"Space symbols",_,{},,)
>  (host,Host,yyy.zzz,{simple},simple,{yyy.zzz})
> (3 rows)
>
> But, this only has to do with 123 being recognized as a number, not with
> the underscore:
>
> =# select ts_debug('simple', 'abc_yyy.zzz');
>ts_debug
> ---
>  (host,Host,abc_yyy.zzz,{simple},simple,{abc_yyy.zzz})
> (1 row)
>
> =# select ts_debug('simple', '1abc_yyy.zzz');
>ts_debug
> ---
>  (host,Host,1abc_yyy.zzz,{simple},simple,{1abc_yyy.zzz})
> (1 row)
>
> In fact, the 123-yyy.zzz domain is not valid either according to the RFC
> (subdomain can't start with a digit), but since we already allow it, should
> we not allow 123_yyy.zzz to be recognized as a Host?  Then why not
> recognize aaa@123_yyy.zzz as an email address?
>
> Another option is to prohibit underscore in recognized host names, but
> this has more breakage potential IMO.
>
> --
> Alex
>
>
Alex, actually subdomain can start with digit, try it.

Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW

[Etsuro Fujita]

On 2016/03/10 2:56, Robert Haas wrote:

I see that you went and changed all of the places that tested for !=
CMD_SELECT and made them test for == CMD_INSERT || == CMD_UPDATE || ==
CMD_DELETE instead.  I think that's the wrong direction.  I think that
we should use the != CMD_SELECT version of the test everywhere.
That's a single test instead of three, so marginally faster, and maybe
marginally more future-proof.

I think deparsePushedDownUpdateSql should be renamed to use the new
"direct modify" naming, like deparseDirectUpdateSql, maybe.

I would suggest not numbering the tests in postgresPlanDirectModify.
That just becomes a nuisance to keep up to date as things change.


Agreed.  I updated the patch to address these comments.  Attached is the 
updated version of the patch.


Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 1315,1320  deparseUpdateSql(StringInfo buf, PlannerInfo *root,
--- 1315,1383 
  }
  
  /*
+  * deparse remote UPDATE statement
+  *
+  * The statement text is appended to buf, and we also create an integer List
+  * of the columns being retrieved by RETURNING (if any), which is returned
+  * to *retrieved_attrs.
+  */
+ void
+ deparseDirectUpdateSql(StringInfo buf, PlannerInfo *root,
+ 	   Index rtindex, Relation rel,
+ 	   List *targetlist,
+ 	   List *targetAttrs,
+ 	   List *remote_conds,
+ 	   List **params_list,
+ 	   List *returningList,
+ 	   List **retrieved_attrs)
+ {
+ 	RelOptInfo *baserel = root->simple_rel_array[rtindex];
+ 	deparse_expr_cxt context;
+ 	int			nestlevel;
+ 	bool		first;
+ 	ListCell   *lc;
+ 
+ 	/* Set up context struct for recursion */
+ 	context.root = root;
+ 	context.foreignrel = baserel;
+ 	context.buf = buf;
+ 	context.params_list = params_list;
+ 
+ 	appendStringInfoString(buf, "UPDATE ");
+ 	deparseRelation(buf, rel);
+ 	appendStringInfoString(buf, " SET ");
+ 
+ 	/* Make sure any constants in the exprs are printed portably */
+ 	nestlevel = set_transmission_modes();
+ 
+ 	first = true;
+ 	foreach(lc, targetAttrs)
+ 	{
+ 		int			attnum = lfirst_int(lc);
+ 		TargetEntry *tle = get_tle_by_resno(targetlist, attnum);
+ 
+ 		if (!first)
+ 			appendStringInfoString(buf, ", ");
+ 		first = false;
+ 
+ 		deparseColumnRef(buf, rtindex, attnum, root, false);
+ 		appendStringInfoString(buf, " = ");
+ 		deparseExpr((Expr *) tle->expr, );
+ 	}
+ 
+ 	reset_transmission_modes(nestlevel);
+ 
+ 	if (remote_conds)
+ 	{
+ 		appendStringInfo(buf, " WHERE ");
+ 		appendConditions(remote_conds, );
+ 	}
+ 
+ 	deparseReturningList(buf, root, rtindex, rel, false,
+ 		 returningList, retrieved_attrs);
+ }
+ 
+ /*
   * deparse remote DELETE statement
   *
   * The statement text is appended to buf, and we also create an integer List
***
*** 1337,1342  deparseDeleteSql(StringInfo buf, PlannerInfo *root,
--- 1400,1442 
  }
  
  /*
+  * deparse remote DELETE statement
+  *
+  * The statement text is appended to buf, and we also create an integer List
+  * of the columns being retrieved by RETURNING (if any), which is returned
+  * to *retrieved_attrs.
+  */
+ void
+ deparseDirectDeleteSql(StringInfo buf, PlannerInfo *root,
+ 	   Index rtindex, Relation rel,
+ 	   List *remote_conds,
+ 	   List **params_list,
+ 	   List *returningList,
+ 	   List **retrieved_attrs)
+ {
+ 	RelOptInfo *baserel = root->simple_rel_array[rtindex];
+ 	deparse_expr_cxt context;
+ 
+ 	/* Set up context struct for recursion */
+ 	context.root = root;
+ 	context.foreignrel = baserel;
+ 	context.buf = buf;
+ 	context.params_list = params_list;
+ 
+ 	appendStringInfoString(buf, "DELETE FROM ");
+ 	deparseRelation(buf, rel);
+ 
+ 	if (remote_conds)
+ 	{
+ 		appendStringInfo(buf, " WHERE ");
+ 		appendConditions(remote_conds, );
+ 	}
+ 
+ 	deparseReturningList(buf, root, rtindex, rel, false,
+ 		 returningList, retrieved_attrs);
+ }
+ 
+ /*
   * Add a RETURNING clause, if needed, to an INSERT/UPDATE/DELETE.
   */
  static void
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 2259,2265  INSERT INTO ft2 (c1,c2,c3)
--- 2259,2284 
  (3 rows)
  
  INSERT INTO ft2 (c1,c2,c3) VALUES (1104,204,'ddd'), (1105,205,'eee');
+ EXPLAIN (verbose, costs off)
+ UPDATE ft2 SET c2 = c2 + 300, c3 = c3 || '_update3' WHERE c1 % 10 = 3;  -- can be pushed down
+   QUERY PLAN  
+ --
+  Update on public.ft2
+->  Foreign Update on public.ft2
+  Remote SQL: UPDATE "S 1"."T 1" SET c2 = (c2 + 300), c3 = (c3 || '_update3'::text) WHERE ((("C 1" % 10) = 3))
+ (3 rows)
+ 
  UPDATE ft2 SET c2 = c2 + 300, c3 = c3 || '_update3' WHERE c1 % 10 = 3;
+ EXPLAIN (verbose, 

Re: [HACKERS] Make primnodes.h gender neutral

[Mark Dilger]

WIP patch to comments and documentation.  Note that I ignored the release notes,
as I don't know if it is appropriate to change those retrospectively.  A few of 
the
changes make the prose worse and will likely be rejected, but I included the 
best
change that came to mind as a starting point for conversation; perhaps somebody
will reply with improvements for v2.




v1.patch
Description: Binary data

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

Re: [HACKERS] Combining Aggregates

[David Rowley]

On 17 March 2016 at 16:30, Haribabu Kommi  wrote:
> On Wed, Mar 16, 2016 at 10:08 PM, David Rowley
>  wrote:
>> On 16 March 2016 at 23:54, Haribabu Kommi  wrote:
>>> On Wed, Mar 16, 2016 at 8:34 AM, David Rowley
>>>  wrote:
 Yes me too, so I spent several hours yesterday writing all of the
 combine functions and serialisation/deserialisation that are required
 for all of SUM(), AVG() STDDEV*(). I also noticed that I had missed
 using some existing functions for bool_and() and bool_or() so I added
 those to pg_aggregate.h. I'm just chasing down a crash bug on
 HAVE_INT128 enabled builds, so should be posting a patch quite soon. I
 didn't touch the FLOAT4 and FLOAT8 aggregates as I believe Haribabu
 has a patch for that over on the parallel aggregate thread. I've not
 looked at it in detail yet.
>>>
>>> The additional combine function patch that I posted handles all float4 and
>>> float8 aggregates. There is an OID conflict with the latest source code,
>>> I will update the patch and post it in that thread.
>>
>> Thanks! I just send a series of patches which add a whole host of
>> serial/deserial functions, and a patch which adds some documentation.
>> Maybe you could base your patch on the 0005 patch, and update the
>> documents to remove the "All types apart from floating-point types"
>> text and replace that with "Yes".
>
> Here I attached updated float aggregates patch based on 0005 patch.

Great! Thanks for sending that.

I just had a quick skim over the patch and noticed the naming
convention you're using for the combine function is *_pl, and you have
float8_pl. There's already a function named float8pl() which is quite
close to what you have. I've been sticking to *_combine() for these,
so maybe float8_combine() and float8_regr_combine() are better names.

-- 
 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] Refactoring speculative insertion with unique indexes a little

[Peter Geoghegan]

On Wed, Mar 16, 2016 at 11:25 AM, Robert Haas  wrote:
> Sure, and if everybody does that, then there will be 40 patches that
> get updated in the last 2 days if the CommitFest, and that will be
> impossible.  Come on.  You're demanding a degree of preferential
> treatment which is unsupportable.

It's unexpected that an entirely maintenance-orientated patch like
this would be received this way. I'm not demanding anything, or
applying any real pressure. Let's just get on with it.

I attach a revision, that makes all the changes that Heikki suggested,
except one. As already noted several times, following this suggestion
would have added a bug. Alvaro preferred my original approach here in
any case. I refer to my original approach of making the new
UNIQUE_CHECK_SPECULATIVE case minimally different from the existing
UNIQUE_CHECK_PARTIAL case currently used for deferred unique
constraints and speculative insertion, as opposed to making the new
UNIQUE_CHECK_SPECULATIVE "like CHECK_UNIQUE_YES, but return FALSE
instead of throwing an error on conflict". That was broken because
CHECK_UNIQUE_YES waits for the outcome of an xact, which
UNIQUE_CHECK_PARTIAL never does, and so UNIQUE_CHECK_SPECULATIVE must
never do.

Any and all waits happen in the first phase of speculative insertion,
and never the seconds. I could give a complicated explanation for why,
involving a deadlock scenario, but a simple explanation will do: it
has always worked that way, and was tested to work that way.

Feedback from Heikki led to these changes for this revision:

* The use of arguments within ExecInsert() was simplified.

* More concise AM documentation.

The docs essentially describe two new concepts:

- What unique index insertion needs to know about speculative
insertion in general. This doesn't just apply to speculative inserters
themselves, of course.

- What speculative insertion is. Why it exists (why we don't just wait
on xact). In other words, what "unprincipled deadlocks" are, and how
they are avoided (from a relatively high level).

I feel like I have a responsibility to make sure that this mechanism
is well documented, especially given that not that many people were
involved in its design. It's possible that no more than the 3 original
authors of UPSERT fully understand speculative insertion -- it's easy
to miss some of the subtleties.

I do not pursue something like this without good reason. I'm
optimistic that the patch will be accepted if it is carefully
considered.

-- 
Peter Geoghegan
From 2b2a4c40a5e60ac1f28a75f11204ce88eb48cc73 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan 
Date: Tue, 2 Jun 2015 17:34:16 -0700
Subject: [PATCH] Refactor speculative insertion into unique indexes

Add a dedicated IndexUniqueCheck constant for the speculative insertion
case, UNIQUE_CHECK_SPECULATIVE, rather than reusing
UNIQUE_CHECK_PARTIAL, which should now only be used for deferrable
unique constraints.

This change allows btinsert() (and, in principle, any amcanunique
aminsert function) to avoid physically inserting an IndexTuple in the
event of detecting a conflict during speculative insertion's second
phase.  With nbtree, this avoidance now occurs at the critical point in
_bt_doinsert() immediately after establishing that there is a conflict,
but immediately before actually calling _bt_insertonpg() to proceed with
physical IndexTuple insertion.

At that point during UNIQUE_CHECK_PARTIAL insertion it makes sense to
soldier on, because the possibility remains that the conflict will later
go away and everything will happen to work out because the conflicting
insertion's transaction aborted.  Speculative inserters, in contrast,
have no chance of working out a way to proceed without first deleting
the would-be-pointed-to heap tuple already physically inserted.  For the
current row proposed for insertion, no useful progress will have been
made at this point.

This patch has nothing to do with performance; it is intended to clarify
how amcanunique AMs perform speculative insertion, and the general
theory of operation.  It is natural to avoid an unnecessary index tuple
insertion.  That that could happen before was quite misleading, because
it implied that it was necessary, and didn't acknowledge the differing
requirements in each case.
---
 doc/src/sgml/indexam.sgml  | 101 ++---
 src/backend/access/nbtree/nbtinsert.c  |  49 +---
 src/backend/executor/execIndexing.c|  34 +--
 src/backend/executor/nodeModifyTable.c |   2 +-
 src/include/access/genam.h |   8 +++
 5 files changed, 148 insertions(+), 46 deletions(-)

diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index 5f7befb..1b26dd0 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -271,10 +271,13 @@ aminsert (Relation indexRelation,
 
   
The function's Boolean result value is significant only when
-   checkUnique is 

Re: [HACKERS] WIP: Upper planner pathification

[Kouhei Kaigai]

> Robert Haas  writes:
> > On Wed, Mar 16, 2016 at 2:47 PM, Tom Lane  wrote:
> >> Robert Haas  writes:
> >>> On Mon, Mar 14, 2016 at 9:21 PM, Kouhei Kaigai  
> >>> wrote:
>  So, even though we don't need to define multiple hook declarations,
>  I think the hook invocation is needed just after create__paths()
>  for each. It will need to inform extension the context of hook
>  invocation, the argument list will take UpperRelationKind.
> 
> >>> That actually seems like a pretty good point.  Otherwise you can't
> >>> push anything from the upper rels down unless you are prepared to
> >>> handle all of it.
> 
> >> I'm not exactly convinced of the use-case for that.  What external
> >> thing is likely to handle window functions but not aggregation,
> >> for example?
> 
> > I'm not going to say that you're entirely wrong, but I think that
> > attitude is a bit short-sighted.
> 
> Well, I'm prepared to yield to the extent of repeating the hook call
> before each phase with an UpperRelationKind parameter to tell which phase
> we're about to do.  The main concern here is to avoid redundant
> computation, but the hook can check the "kind" parameter and fall out
> quickly if it has nothing useful to do at the current phase.
> 
> I do not, however, like the proposal to expose wflists and so forth.
> Those are internal data structures in grouping_planner and I absolutely
> refuse to promise that they're going to stay stable.  (I had already
> been thinking a couple of weeks ago about revising the activeWindows
> data structure, now that it would be reasonably practical to cost out
> different orders for doing the window functions in.)  I think a hook
> that has its own ideas about window function implementation methods
> can gather its own information about the WFs without that much extra
> cost, and it very probably wouldn't want exactly the same data that
> create_window_paths uses today anyway.
> 
> So what I would now propose is
> 
> typedef void (*create_upper_paths_hook_type) (PlannerInfo *root,
>   UpperRelationKind stage,
>   RelOptInfo *input_rel);
>
Hmm... It's not easy to imagine a case that extension wants own idea
to extract window functions from the target list and select active
windows, even if extension wants to have own executor and own cost
estimation logic.
In case when extension tries to add WindowPath + CustomPath(Sort),
extension is interested in alternative sort task, but not window
function itself. It is natural to follow the built-in implementation,
thus, it motivates extension author to take copy & paste the code.
select_active_windows() is static, so extension needs to have same
routine on their side.

On the other hands, 'rollup_lists' and 'rollup_groupclauses' need
three static functions (extract_rollup_sets(), reorder_grouping_sets()
and preprocess_groupclause() to reproduce the equivalent data structure.
It is larger copy & paste burden, if extension is not interested in
extracting the information related to grouping set.


I understand it is not "best", but better to provide extra information
needed for extension to reproduce equivalent pathnode, even if fields
of UpperPathExtraData structure is not stable right now.

> and have this invoked at each stage right before we call
> create_grouping_paths, create_window_paths, etc.
>
It seems to me reasonable.

> Also, I don't particularly see a need for a corresponding API for FDWs.
> If an FDW is going to do anything in this space, it presumably has to
> build up ForeignPaths for all the steps anyway.  So I'd be inclined
> to leave GetForeignUpperPaths as-is.
>
It seems to me reasonable. FDW driver which is interested in remote
execution of upper path can use the hook arbitrary.

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



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

Re: [HACKERS] [COMMITTERS] pgsql: Improve memory management for external sorts.

[Robert Haas]

On Fri, Mar 18, 2016 at 2:25 PM, Andres Freund  wrote:
> On 2016-03-17 20:11:00 +, Robert Haas wrote:
>> Improve memory management for external sorts.
>>
>> Introduce a new memory context which stores tuple data, and reset it
>> at the end of each merge pass; this helps avoid memory fragmentation
>> and, consequently, overallocation.  Also, for the final merge patch,
>> eliminate memory context chunk header overhead entirely by allocating
>> all of the memory used for buffering tuples during the merge in a
>> single chunk.  Since this modestly increases the number of tuples we
>> can store, grow the memtuples array a bit so that we're less likely to
>> run short of slots there.
>>
>> Peter Geoghegan.  Review and testing of patches in this series by
>> Jeff Janes, Greg Stark, Mithun Cy, and me.
>
> Cross compiling for windows results in:
> /home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c: In function 
> ‘beginmerge’:
> /home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c:2695:34: 
> warning: format ‘%ld’ expects argument of type ‘long int’, but argument 4 has 
> type ‘int64 {aka long long int}’ [-Wformat=]
>  elog(LOG, "tape %d initially used %ld KB of %ld KB batch "
>   ^
> /home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c:2695:34: 
> warning: format ‘%ld’ expects argument of type ‘long int’, but argument 5 has 
> type ‘int64 {aka long long int}’ [-Wformat=]
> config.status: creating src/interfaces/ecpg/include/ecpg_config.h
>
> /home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c: In
> function ‘beginmerge’:
> /home/andres/src/postgresql/src/backend/utils/sort/tuplesort.c:2695:34:
> warning: format ‘%ld’ expects argument of type ‘long int’, but argument
> 4 has type ‘int64 {aka long long int}’ [-Wformat=]
>  elog(LOG, "tape %d initially used %ld KB of %ld KB batch "
>
> Which seems like a valid complain on a LLP64 platform (i.e. where long
> is 32bit) like windows.

Oops.  Thanks for the report.  Does this fix 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] Performance degradation in commit ac1d794

[Andres Freund]

Hi,

On 2016-03-17 09:01:36 -0400, Robert Haas wrote:
> 0001: Looking at this again, I'm no longer sure this is a bug.
> Doesn't your patch just check the same conditions in the opposite
> order?

Yes, that's what's required

> 0004:
> 
> + * drain it everytime WaitLatchOrSocket() is used. Should the
> + * pipe-buffer fill up in some scenarios - widly unlikely - we're
> 
> every time
> wildly
> 
> Why is it wildly (or widly) unlikely?
> 
> The rejiggering this does between what is on which element of pfds[]
> appears to be unrelated to the ostensible purpose of the patch.

Well, not really. We need to know when to do drainSelfPipe(); Which gets
more complicated if pfds[0] is registered optionally.

I'm actually considering to drop this entirely, given the much heavier
rework in the WaitEvent set patch; making these details a bit obsolete.


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] Performance degradation in commit ac1d794

[Alvaro Herrera]

Andres Freund wrote:

> From 1d444b0855dbf65d66d73beb647b772fff3404c8 Mon Sep 17 00:00:00 2001
> From: Andres Freund 
> Date: Fri, 18 Mar 2016 00:52:07 -0700
> Subject: [PATCH 4/5] Combine win32 and unix latch implementations.
> 
> Previously latches for windows and unix had been implemented in
> different files. The next patch in this series will introduce an
> expanded wait infrastructure, keeping the implementation separate would
> introduce too much duplication.
> 
> This basically just moves the functions, without too much change. The
> reason to keep this separate is that it allows blame to continue working
> a little less badly; and to make review a tiny bit easier.

This seems a reasonable change, but I think that the use of WIN32 vs.
LATCH_USE_WIN32 is pretty confusing.  In particular, LATCH_USE_WIN32
isn't actually used for anything ... I suppose we don't care since this
is a temporary state of affairs only?

In 0005: In latch.c you typedef WaitEventSet, but the typedef already
appears in latch.h.  You need only declare the struct in latch.c,
without typedef'ing.


Haven't really reviewed anything here yet, just skimming ATM.  Having so
many #ifdefs all over the place in this file looks really bad, but I
guess there's no way around that because this is very platform-specific.
I hope pgindent doesn't choke on it.

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


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

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

[Andres Freund]

On 2016-03-16 13:29:22 -0400, Robert Haas wrote:
> Whoa.  At 64 clients, we're hardly getting any benefit, but then by 88
> clients, we're getting a huge benefit.  I wonder why there's that sharp
> change there.

What's the specifics of the machine tested? I wonder if it either
correlates with the number of hardware threads, real cores, or cache
sizes.

- 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] Sequence Access Method WIP

[David Steele]

On 3/4/16 11:09 PM, Petr Jelinek wrote:

> But first here is updated patch for sequence access methods. I went with
> the previously discussed custom type as this gives us proper control
> over the width of the state and making sure that it's not gonna be
> toastable, etc and we need this as sequence is limited to single page.
> 
> Attached are 3 separate files. The first one (0001-create-am) is mainly
> separate for the reason that there is some overlap with Alexander's
> index access methods patch, so I extracted common code into separate
> patch as it will make it easier to merge in case we are lucky enough to
> get these patches in (but it's still based on Alexander's code). It
> provides infra for defining new access methods from SQL, although
> without the parser and without any actual access methods.
> 
> The 0002-seqam provides the SQL interface and support for sequence
> access methods on top of the infra patch, and also provides all the
> sequence access methods infrastructure and abstraction and documentation.
> 
> And finally the 0003-gapless-seq is example contrib module that
> implements dependably and transitionally safe gapless sequence access
> method. It's obviously slow as it has to do locking and basically
> serialize all the changes to sequence so only one transaction may use it
> at a time but it's truly gapless. It also serves as an example of use of
> the api and as a test.

As far as I can see Petr has addressed all the outstanding issues in
this patch and it's ready for a review.  The patch applies with some
easily-fixed conflicts:

$ git apply -3 ../other/0002-seqam-2015-03-05.patch
error: patch failed: src/include/catalog/pg_proc.h:5225
Falling back to three-way merge...
Applied patch to 'src/include/catalog/pg_proc.h' with conflicts.

Simon, you are signed up to review.  Do you have an idea of when you'll
be doing that?

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


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

Re: [HACKERS] 2016-03 Commitfest

[David Steele]

We are now nearly two thirds of the way through the 2016-03 Commitfest.

There are still some patches left that need review but have no reviewer
(https://commitfest.postgresql.org/9/?status=1=-2) and this 
hasn't changed much since last week.


Needs review: 55 (was 56 last week)
Needs *reviewer*: 12 (was 15 last week)

There are still a lot of patches that need review but the good news is 
that nearly all of them have seen some level of review.


Waiting on Author: 11 (was 29 last week)

The number of patches waiting on author has gone down quite a bit but 
that was somewhat on account of idle patches being closed and a few that 
needed to be marked for review.


49% of the patches are now closed and I suspect that the low hanging 
fruit has been cleared away and we are left with more complicated patches.


Committed: 56 (was 46 last week)
Rejected: 5 (was 4 last week)
Returned with Feedback: 11 (was 4 last week)

** PATCH AUTHORS PLEASE READ **

If you have been pinged on a thread that is "waiting for author" and do 
not respond by next Tuesday your patch will likely be closed.  We are 
now in crunch time with two weeks until the end of the CF and three 
weeks until feature freeze.  If you have extenuating circumstances 
please make them clear on the thread so everyone knows the status.


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


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

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

[Andrew Dunstan]

On 03/08/2016 07:40 PM, Michael Paquier wrote:

On Wed, Mar 9, 2016 at 1:07 AM, Andrew Dunstan  wrote:

Michael's patch proposes to replace the use of sed to generate probes.h with
the perl equivalent everywhere. That has the advantage that we keep to one
script to generate probes.h, but it does impose a perl dependency.

Good point. It did not occur to me that this would bring a hard
dependency for non-Windows builds. Let's keep both scripts then. The
attached is changed to do so.



Actually, it wasn't, but I fixed it and committed it.

Still to do: the non-perl pieces.

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

[David Steele]

On 3/17/16 11:55 AM, David G. Johnston wrote:

> On Thu, Mar 17, 2016 at 8:41 AM, David Steele  >wrote:
> 
>> Not sure I agree.  My point was that even if developers were pretty
>> careful with their casting (or are using two actual dates) then there's
>> still possibility for breakage.
> 
> With the first argument casted to date it doesn't matter whether you
> cast the second argument as the pseudo-type anyelement will take its
> value from the first argument and force the second to date.

Ah, I see.

> The main problem is that the system tries to cast unknown to integer
> based upon finding gs(date, date, integer) and fails without ever
> considering that gs(ts, ts, interval) would succeed.

I'm tempted to say, "why don't we just fix that?" but I'm sure any
changes in that area would introduce a variety of new and interesting
behaviors.

> ​So let the user decide whether to trade-off learning a new function
> name but getting dates instead of timestamps or going through the hassle
> of casting.​
> 
> For me, it would have been a nice bonus if the generate_series() could
> be used directly but I feel that the desired functionality is desirable
> and the name itself is of only minor consideration.
> 
> I can see that newbies might ponder why two functions exist but they
> should understand "backward compatibility".
> 
> I suspect that most people will simply think: "use generate_series for
> numbers and use generate_dates for, well, dates".  The ones left out in
> the cold are those wondering why they use "generate_series" for
> timestamp series with a finer precision than date.  I'd be willing to
> offer a "generate_timestamps" to placate them.  And might as well toss
> in "generate_numbers" for completeness - though now I likely doom the
> proposal...so I'll stick with just add generate_dates so the behavior is
> possible without superfluous casting or the need to write a custom
> function.  Dates are too common a unit of measure to leave working with
> them this cumbersome.

I'm not finding this argument persuasive.

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


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

Re: [HACKERS] Make primnodes.h gender neutral

[Mark Dilger]

> On Mar 17, 2016, at 5:40 PM, Mark Dilger  wrote:
> 
> 
>> On Mar 17, 2016, at 5:05 PM, Peter Geoghegan  wrote:
>> 
>> On Thu, Mar 17, 2016 at 4:46 PM, Tom Lane  wrote:
>>> Alvaro's original complaint that the sentences no longer agree as to
>>> person is on-point.
>> 
>> That's reasonable. Still, there are only a few existing instances of
>> gendered pronouns in the code, so fixing them carefully, without
>> losing anything important seems like a relatively straightforward
>> task.
> 
> I have compiled a list of all the ones I found, manually excluding instances
> where the pronoun is appropriate:
> 
> ./configure.in
> --
>  line 751: # so we make the user say which one she wants.
> 
> ./doc/src/sgml/release-7.4.sgml
> ---
>  line 223:   a database he owns, this would remove all special parameter 
> settings
> 
> ./doc/src/sgml/release-8.0.sgml
> ---
>  line 293:   a database he owns, this would remove all special parameter 
> settings
> 
> ./doc/src/sgml/release-8.1.sgml
> ---
>  line 520:   a database he owns, this would remove all special parameter 
> settings
> line 3446:Once a user logs into a role, she obtains capabilities of
> line 3448:SET ROLE to switch to other roles she is a 
> member of.
> 
> ./doc/src/sgml/release-8.2.sgml
> ---
> line 1423:   a database he owns, this would remove all special parameter 
> settings
> 
> ./doc/src/sgml/release-8.3.sgml
> ---
> line 1072:   function with forged input data, by installing it on a table 
> he owns.
> line 2996:   a database he owns, this would remove all special parameter 
> settings
> 
> ./doc/src/sgml/release-8.4.sgml
> ---
>  line 492:   revoke the access of others, contrary to the wishes of his 
> grantor.
>  line 494:   uncooperative role member could provide most of his rights 
> to others
> line 2719:   function with forged input data, by installing it on a table 
> he owns.
> line 5223:   a database he owns, this would remove all special parameter 
> settings
> 
> ./doc/src/sgml/release-9.0.sgml
> ---
> line 1220:   within a command parameter might succeed in injecting his 
> own SQL
> line 2382:   revoke the access of others, contrary to the wishes of his 
> grantor.
> line 2384:   uncooperative role member could provide most of his rights 
> to others
> line 5089:   function with forged input data, by installing it on a table 
> he owns.
> 
> ./doc/src/sgml/release-9.1.sgml
> ---
> line 1867:   within a command parameter might succeed in injecting his 
> own SQL
> line 3164:   revoke the access of others, contrary to the wishes of his 
> grantor.
> line 3166:   uncooperative role member could provide most of his rights 
> to others
> line 6551:   function with forged input data, by installing it on a table 
> he owns.
> 
> ./doc/src/sgml/release-9.2.sgml
> ---
> line 2006:   within a command parameter might succeed in injecting his 
> own SQL
> line 3521:   revoke the access of others, contrary to the wishes of his 
> grantor.
> line 3523:   uncooperative role member could provide most of his rights 
> to others
> 
> ./doc/src/sgml/release-9.3.sgml
> ---
> line 2273:   within a command parameter might succeed in injecting his 
> own SQL
> line 5641:   revoke the access of others, contrary to the wishes of his 
> grantor.
> line 5643:   uncooperative role member could provide most of his rights 
> to others
> 
> ./doc/src/sgml/release-9.4.sgml
> ---
> line 4394:   within a command parameter might succeed in injecting his 
> own SQL
> 
> ./doc/src/sgml/user-manag.sgml
> --
>  line 132:need not match his or her real name.)  Since the role
> 
> ./src/backend/access/hash/README
> 
>  line 446: page acquirer will scan more bitmap bits than he needs to.  What 
> must be
> 
> ./src/backend/access/heap/heapam.c
> --
> line 5274:  * It's a committed update, so we need to preserve him 
> as updater of
> line 5381:  * It's a committed update, so we gotta preserve him 
> as updater of the
> line 6557:  * the VACUUM and perhaps truncate off the part of pg_clog he 
> needs.  Getting
> 
> ./src/backend/access/index/genam.c
> --
>   line 48:  *  whatever kind of locking he wants.
> 
> ./src/backend/access/transam/README
> ---
>  line 108:she sees and types ABORT(syntax error, etc)
> 
> ./src/backend/access/transam/twophase.c
> 

[HACKERS] async replication code

[otheus uibk]

Greetings,

I am new here.  Apologetic question: how can i search the archives of this
mailing list? Is there some set of magic words to put in Google? Must I
wade through 20 pages of hits? Should I download all the archives and grep?
:)

Main question:  I want to understand very precisely the exact algirithm
used in PG to do asynchronous streaming replication. Eg, I want to know how
the record is written and flushed to the socket and how that happens in
time w.r.t WAL-to-disk and response to client. Will someone please give me
a head start as to where to begin my code-spelunking?


-- 
Otheus
otheus.u...@gmail.com
otheus.shell...@uibk.ac.at

Re: [HACKERS] raw output from copy

[David Steele]

On 3/12/16 1:24 AM, Pavel Stehule wrote:


Great, thank you very much. I hope so this feature really useful. It
allow to simple export/import XML doc in different encodings, JSONs and
can be enhanced future via options. The nice feature  (but not for this
release) can be additional cast info for import -- like "COPY
table(jsonb_column) FROM stdin (FORMAT RAW, CAST json_2_jsonb). Because
there are the options, there are big space for other enhancing.


Andres Karlsson pointed out that this patch has two CF entries:

https://commitfest.postgresql.org/9/223/
https://commitfest.postgresql.org/9/547/

I closed the one that was in the "needs review" (547) state and kept the 
one that is "ready for committer" (223).


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


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

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

[Jeff Janes]

On Fri, Jul 17, 2015 at 6:50 AM, Fabien  wrote:
>
> This patch adds per-script statistics & other improvements to pgbench
>
> Rationale: Josh asked for the per-script stats:-)
>
> Some restructuring is done so that all stats (-l --aggregate-interval
> --progress --per-script-stats, latency & lag...) share the same structures
> and functions to accumulate data. This limits a lot the growth of pgbench
> from this patch (+17 lines).
>
> In passing, remove the distinction between internal and external scripts.
> Pgbench just execute scripts, some of them may be internal...
>
> As a side effect, all scripts can be accumulated "pgbench -B -N -S -f ..."
> would execute 4 scripts, 3 of which internal (tpc-b, simple-update,
> select-only and another externally supplied one).
>
> Also add a weight option to change the probability of choosing some scripts
> when several are available.

I was eager to use this to do some performance testing on a series of
workloads gradually transitioning from write-heavy to read-only.

So I wanted to do something like:

for f in `seq 0 5 100`; do
  pgbench -T 180 -c8 -j8 -b tpcb-like@$f -b select-only@100
done;

But, I'm not allowed to specify a weight of zero.  That means I have
to special-case the first iteration of the "for" loop where $f is
zero.  I think it would be more convenient if I was allowed to specify
a zero weight, and the script would just ignore that script.  All I
had to do to make this work is remove the check that prevents from
setting the weight to zero.  But then I would need to add in a check
that the sum of all weights is not zero, which I have done here.

We could get more complex by not adding a zero-weight script into the
array of scripts at all, rather than adding it in a way where it can
never be selected.  But then that would complicate the parsing of the
per-script stats report, when one of the scripts was no longer
reported.  I like this way better.

Would this be a welcome change?

Cheers,

Jeff


pgbench_zero_weight.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] FIX : teach expression walker about RestrictInfo

[Robert Haas]

On Fri, Mar 18, 2016 at 3:29 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Apr 29, 2015 at 12:33 PM, Tomas Vondra
>>  wrote:
>>> On 04/29/15 18:26, Tom Lane wrote:
 But there are basic reasons why expression_tree_walker should not try
 to deal with RestrictInfos; the most obvious one being that it's not
 clear whether it should descend into both the basic and OR-clause
 subtrees. Also, if a node has expression_tree_walker support then it
 should logically have expression_tree_mutator support as well, but
 that's got multiple issues. RestrictInfos are not supposed to be
 copied once created; and the mutator couldn't detect whether their
 derived fields are still valid.
>
>>> OK, I do understand that. So what about pull_varnos_walker and
>>> pull_varattnos_walker - what about teaching them about RestrictInfos?
>
>> This patch has become part 1 of many under the "multivariate
>> statistics vNNN" family of threads, but I guess it would be helpful if
>> you could opine on the reasonable-ness of this approach.  I don't want
>> to commit anything over your objections, but this kind of tailed off
>> without a conclusion.
>
> I'm up to my ears in psql at the moment, but hope to get to the
> multivariate stats patch later, maybe next week.

Oh, cool.

> In the meantime,
> I remain of the opinion that RestrictInfo is not an expression node
> and wanting expression_tree_walker to handle it is wrong.  I'm
> suspicious about pull_varnos; it might be all right given that we
> can assume the same Vars are in both subtrees, but I still don't
> really see why that one function has suddenly grown this need when
> others have not.

I haven't studied the patch series in enough detail to have an
educated opinion on that.

-- 
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] Using quicksort for every external sort run

[Robert Haas]

On Wed, Mar 16, 2016 at 9:42 PM, Peter Geoghegan  wrote:
> On Wed, Mar 16, 2016 at 3:31 PM, Robert Haas  wrote:
>> I spent some time looking at this part of the patch yesterday and
>> today.
>
> Thanks for getting back to it.

OK, I have now committed 0001, and separately, some comment
improvements - or at least, I think they are improvements - based on
this discussion.

Thanks.

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


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

Re: [HACKERS] extend pgbench expressions with functions

[Fabien COELHO]

v38 is a simple rebase, trying to keep up-to-date with Tom's work.

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

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

   
-
-  
-   As an example, the full definition of the built-in TPC-B-like
-   transaction is:
-
-
-\set nbranches :scale
-\set ntellers 10 * :scale
-\set naccounts 10 * :scale
-\setrandom aid 1 :naccounts
-\setrandom bid 1 :nbranches
-\setrandom tid 1 :ntellers
-\setrandom delta -5000 5000
-BEGIN;
-UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
-SELECT abalance FROM pgbench_accounts WHERE 

Re: [HACKERS] Sequence Access Method WIP

[Alvaro Herrera]

Petr Jelinek wrote:

> And finally the 0003-gapless-seq is example contrib module that implements
> dependably and transitionally safe gapless sequence access method. It's
> obviously slow as it has to do locking and basically serialize all the
> changes to sequence so only one transaction may use it at a time but it's
> truly gapless. It also serves as an example of use of the api and as a test.

I'm trying to figure out this one, and I think I found something very
surprising.  This code contains this

+#define GAPLESS_SEQ_NAMESPACE "gapless_seq"
+#define VALUES_TABLE_NAME "seqam_gapless_values"

which as far as I understand is something like a side table where values
for all the sequences are stored.  Is that right?  If it is, I admit I
didn't realize that these sequences worked in this way.  This seems
broken to me.  I had been under the impression that the values were
always stored in the sequence's relation.  Maybe I'm misunderstanding;
please explain how this works.


FWIW I find it annoying that this submission's 0001 patch and
Alexander's 0001 have so much in common, yet they aren't similar enough
to consider that either is the definite form.  Also, you have the SGML
docs for CREATE ACCESS METHOD in 0002 instead of 0001, so comparing both
versions isn't trivial.

Anyway I'm back at reviewing Alexander's 0001, which should be okay as a
basis for this series regardless of any edits I suggest there.

-- 
Á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] Parallel Aggregate

[David Rowley]

On 19 March 2016 at 06:15, Robert Haas  wrote:
> On Fri, Mar 18, 2016 at 9:16 AM, David Rowley
>  wrote:
>> I've attached an updated patch.
>
> This looks substantially better than earlier versions, although I
> haven't studied every detail of it yet.
>
> + * Partial aggregation requires that each aggregate does not have a DISTINCT 
> or
> + * ORDER BY clause, and that it also has a combine function set. Since 
> partial
>
> I understand why partial aggregation doesn't work if you have an ORDER
> BY clause attached to the aggregate itself, but it's not so obvious to
> me that using DISTINCT should rule it out.  I guess we can do it that
> way for now, but it seems aggregate-specific - e.g. AVG() can't cope
> with DISTINCT, but MIN() or MAX() wouldn't care.  Maybe MIN() and
> MAX() are the outliers in this regard, but they are a pretty common
> case.

hmm? We'd have no way to ensure that two worker processes aggregated
only values which other workers didn't.
Of course this just happens to be equivalent for MIN() and MAX(), but
today we don't attempt to transform MIN(DISTINCT col) to MIN(col), so
I see no reason at all why this patch should go and add something
along those lines. Perhaps it's something for the future though,
although it's certainly not anything specific to parallel aggregate.

> + * An Aggref can operate in one of two modes. Normally an aggregate 
> function's
> + * value is calculated with a single executor Agg node, however there are
> + * times, such as parallel aggregation when we want to calculate the 
> aggregate
>
> I think you should adjust the punctuation to "with a single executor
> Agg node; however, there are".  And maybe drop the word "executor".
>
> And on the next line, I'd add a comma: "such as parallel aggregation,
> when we want".

Fixed. Although I've revised that block a bit after getting rid of aggpartial.

>
> astate->xprstate.evalfunc =
> (ExprStateEvalFunc) ExecEvalAggref;
> -   if (parent && IsA(parent, AggState))
> +   if (!aggstate || !IsA(aggstate, AggState))
> {
> -   AggState   *aggstate =
> (AggState *) parent;
> -
> -   aggstate->aggs = lcons(astate,
> aggstate->aggs);
> -   aggstate->numaggs++;
> +   /* planner messed up */
> +   elog(ERROR, "Aggref found in
> non-Agg plan node");
> }
> -   else
> +   if (aggref->aggpartial ==
> aggstate->finalizeAggs)
> {
> /* planner messed up */
> -   elog(ERROR, "Aggref found in
> non-Agg plan node");
> +   if (aggref->aggpartial)
> +   elog(ERROR, "Partial
> type Aggref found in FinalizeAgg plan node");
> +   else
> +   elog(ERROR,
> "Non-Partial type Aggref found in Non-FinalizeAgg plan node");
> }
> +   aggstate->aggs = lcons(astate, 
> aggstate->aggs);
> +   aggstate->numaggs++;
>
> This seems like it involves more code rearrangement than is really
> necessary here.

This is mostly gone, as after removing aggpartial some of these checks
are not possible. I just have some additional code:

Aggref   *aggref = (Aggref *) node;

if (aggstate->finalizeAggs &&
aggref->aggoutputtype != aggref->aggtype)
{
/* planner messed up */
elog(ERROR, "Aggref aggoutputtype must match aggtype");
}

But nothing to sanity check non-finalize nodes.

>
> +* Partial paths in the input rel could allow us to perform
> aggregation in
> +* parallel, set_grouped_rel_consider_parallel() will determine if 
> it's
> +* going to be safe to do so.
>
> Change comma to semicolon or period.

Changed.

> /*
>  * Generate a HashAgg Path atop of the cheapest partial path, once
>  * again, we'll only do this if it looks as though the hash table 
> won't
>  * exceed work_mem.
>  */
>
> Same here.  Commas are not the way to connect two independent sentences.

Changed.

-- 
 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] Relaxing SSL key permission checks

[Peter Eisentraut]

Committed with the discussed adjustment and documentation update.

On 3/18/16 2:26 PM, Christoph Berg wrote:
> Re: Peter Eisentraut 2016-03-16 <56e8c221.1050...@gmx.net>
 * it failed to check for S_IXUSR, so permissions 0700 were okay, in
 contradiction with what the error message indicates.  This is a
 preexisting bug actually.  Do we want to fix it by preventing a
 user-executable file (possibly breaking compability with existing
 executable key files), or do we want to document what the restriction
 really is?
>>>
>>> I think we should not check for S_IXUSR.  There is no reason for doing that.
>>>
>>> I can imagine that key files are sometimes copied around using USB
>>> drives with FAT file systems or other means of that sort where
>>> permissions can scrambled.  While I hate gratuitous executable bits as
>>> much as the next person, insisting here would just create annoyances in
>>> practice.
>>
>> I'm happy with this patch except this minor point.  Any final comments?
> 
> I'm fine with that change.
> 
> Do you want me to update the patch or do you already have a new
> version, given it's marked as Ready for Committer?
> 
> Christoph
> 



-- 
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] Make primnodes.h gender neutral

[Peter Geoghegan]

On Thu, Mar 17, 2016 at 4:09 PM, Robert Haas  wrote:
> Debating whether or not somebody is currently upset about this, and
> how upset the are, and what the value is of fixing it is missing the
> point.  When somebody sends a patch for a typographical error, we
> don't say: well, we could fix that typographical error, but let's wait
> until the next time we have cause to reword the paragraph.  We just
> commit the patch

Right. We could spend significant time debating how much this matters.
I expect that few if any contributors would consider that a policy on
gendered pronouns has negative value, though, and it really isn't that
hard to fix. So we should just fix it.

(In case it matters, I'm in favor of this proposal on its merits).

-- 
Peter Geoghegan


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

Re: [HACKERS] Performance degradation in commit ac1d794

[Andres Freund]

On 2016-01-14 12:07:23 -0500, Robert Haas wrote:
> On Thu, Jan 14, 2016 at 12:06 PM, Andres Freund  wrote:
> > On 2016-01-14 11:31:03 -0500, Robert Haas wrote:
> >> On Thu, Jan 14, 2016 at 10:56 AM, Andres Freund  wrote:
> >> I think your idea of a data structure the encapsulates a set of events
> >> for which to wait is probably a good one.  WaitLatch doesn't seem like
> >> a great name.  Maybe WaitEventSet, and then we can have
> >> WaitLatch() and WaitEvents().
> >
> > Hm, I'd like to have latch in the name. It seems far from improbably to
> > have another wait data structure. LatchEventSet maybe? The wait would be
> > implied by WaitLatch.
> 
> I can live with that.


How about the following sketch of an API

typedef struct LatchEvent
{
uint32 events; /* interesting events */
uint32 revents;  /* returned events */
int fd; /* fd associated with event */
} LatchEvent;

typedef struct LatchEventSet
{
int nevents;
LatchEvent events[FLEXIBLE_ARRAY_MEMBER];
} LatchEventSet;

/*
 * Create a LatchEventSet with space for nevents different events to wait for.
 *
 * latch may be NULL.
 */
extern LatchEventSet *CreateLatchEventSet(int nevents, Latch *latch);

/* ---
 * Add an event to the set. Possible events are:
 * - WL_LATCH_SET: Wait for the latch to be set
 * - WL_SOCKET_READABLE: Wait for socket to become readable
 *   can be combined in one event with WL_SOCKET_WRITEABLE
 * - WL_SOCKET_WRITABLE: Wait for socket to become readable
 *   can be combined with WL_SOCKET_READABLE
 * - WL_POSTMASTER_DEATH: Wait for postmaster to die
 */
extern void AddLatchEventToSet(LatchEventSet *set, uint32 events, int fd);

/*
 * Wait for any events added to the set to happen, or until the timeout is
 * reached.
 *
 * The return value is the union of all the events that were detected. This
 * allows to avoid having to look into the associated events[i].revents
 * fields.
 */
extern uint32 WaitLatchEventSet(LatchEventSet *set, long timeout);



I've two questions:
- Is there any benefit of being able to wait for more than one latch?
  I'm inclined to not allow that for now, that'd make the patch bigger,
  and I don't see a use-case right now.
- Given current users we don't need a large amount of events, so having
  to iterate through the registered events doesn't seem bothersome. We
  could however change the api to be something like
  
  int WaitLatchEventSet(LatchEventSet *set, OccurredEvents *, int nevents, long 
timeout);

  which would return the number of events that happened, and would
  basically "fill" one of the (usually stack allocated) OccurredEvent
  structures with what happened.


Comments?


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

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

[David Steele]

On 3/16/16 7:59 AM, Stas Kelvich wrote:
> On 12 Mar 2016, at 13:19, Michael Paquier  wrote:
>>
>> On Fri, Mar 11, 2016 at 9:35 PM, Tom Lane  wrote:
>>> IMO this is not committable as-is, and I don't think that it's something
>>> that will become committable during this 'fest.  I think we'd be well
>>> advised to boot it to the 2016-09 CF and focus our efforts on other stuff
>>> that has a better chance of getting finished this month.
>>
>> Yeah, I would believe that a good first step would be to discuss
>> deeply about that directly at PGCon for folks that will be there and
>> interested in the subject. It seems like a good timing to brainstorm
>> things F2F at the developer unconference for example, a couple of
>> months before the 1st CF of 9.7. We may perhaps (or not) get to
>> cleaner picture of what kind of things are wanted in this area.
> 
> To give overview of xtm coupled with postgres_fdw from users perspective i’ve 
> packed patched postgres with docker
> and provided test case when it is easy to spot violation of READ COMMITTED 
> isolation level without XTM.
> 
> This test fills database with users across two shards connected by 
> postgres_fdw and inherits the same table. Then 
> starts to concurrently transfers money between users in different shards:
> 
> begin;
> update t set v = v - 1 where u=%d; -- this is user from t_fdw1, first shard
> update t set v = v + 1 where u=%d; -- this is user from t_fdw2, second shard
> commit;
> 
> Also test simultaneously runs reader thread that counts all money in system:
> 
> select sum(v) from t;
> 
> So in transactional system we expect that sum should be always constant (zero 
> in our case, as we initialize user with zero balance).
> But we can see that without tsdtm total amount of money fluctuates around 
> zero.
> 
> https://github.com/kelvich/postgres_xtm_docker

This is an interesting example but I don't believe it does much to
address the concerns that were raised in this thread.

As far as I can see the consensus is that this patch should not be
considered for the current CF so I have marked it "returned with feedback".

If possible please follow Michael's advice and create a session at the
PGCon unconference in May.  I'm certain there will be a lot of interest.

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


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

Re: [HACKERS] Declarative partitioning

[Amit Langote]

Hi Alexander,

Thanks a lot for taking a look!

On Wed, Mar 16, 2016 at 10:56 PM, Alexander Korotkov
 wrote:
> I tried to apply your patch.  It still applies, but has some duplicate oids.

Actually, a reworked version I will post hopefully early next week
will have fixed this.

> After fixing duplicate oids, I've noticed following warning during
> compilation by clang-700.1.81.
>
> scan.c:10308:23: warning: unused variable 'yyg' [-Wunused-variable]
> struct yyguts_t * yyg = (struct yyguts_t*)yyscanner; /* This var may be
> unused depending upon options. */
>   ^
> tablecmds.c:12922:6: warning: variable 'is_subpart' is used uninitialized
> whenever 'if' condition is false [-Wsometimes-uninitialized]
> if (parent != NULL)
> ^~
> tablecmds.c:12931:12: note: uninitialized use occurs here
> partKey = is_subpart ? list_nth(rootParent->rd_partkeys,
> parent_level) :
>   ^~
> tablecmds.c:12922:2: note: remove the 'if' if its condition is always true
> if (parent != NULL)
> ^~~
> tablecmds.c:12912:19: note: initialize the variable 'is_subpart' to silence
> this warning
> boolis_subpart;
>   ^
>= '\0'
> tablecmds.c:13375:37: warning: variable 'operoid' is uninitialized when used
> here [-Wuninitialized]
> comp_left_expr =
> make_opclause(operoid, BOOLOID, false,
>
> ^~~
> tablecmds.c:13326:17: note: initialize the variable 'operoid' to silence
> this warning
> Oid operoid;
>^
> = 0

Oh, I will find and fix these issues if they are still left in the
newer version.  Thanks for reporting.

> Regression tests passed cleanly for me.  I also examined code a bit.  As I
> get, for DML queries, declarative partitioning works like inheritance.  It
> just provides alternative way for collecting append_rel_list.

Right, my latest patch doesn't touch this part at all.

> We're working on the other side of partitioning problem.  Without solving
> syntax problem, we're solving performance problems in pg_pathman extension:
> https://github.com/postgrespro/pg_pathman.  We already have interesting
> results which you can see in blog posts [1], [2], [3].

I have been following the blog posts.  Results look promising, :)

> We already have fast algorithm for partition selection in optimizer [1] and
> effective optimization of filter conditions [3]. And we're planning to
> implement following features:
>
> Execute time partitions selection (useful for nested loops and prepared
> statements);
> Optimization of ordered output from patitioned tables (use Append instead of
> MergeAppend when possible);
> Optimization of hash join when both tables are patitioned by join key.
>
> 9.6 feature freeze in coming, and we're planning our development resources
> for 9.7. Besides providing an extension, we would like these features to
> eventually arrive to core.  In order to achieve this we need to port out
> functionality over your declarative syntax.  At first, we would need some
> way for caching partition metadata suitable for fast partition selection.
> For range partition it could be sorted array or RB-tree of partition bounds.
> When we have this infrastructure, we can start porting pieces of pg_pathman
> functionality to declarative partitiong.

I had to think about the internal metadata representation (and its
caching) when developing the tuple routing solution.  I am hopeful
that it is suitable for other executor mechanisms we will build for
partitioned tables.

> So, our draft plan of patches would be following:
>
> Implement partition metadata cache suitable for fast partition selection.
> Fast partition selection using metadata cache.
> Optimization of filter conditions passed to partitions.
> Execute time partitions selection (useful for nested loops and prepared
> statements);
> Optimization of ordered output from patitioned tables (use Append instead of
> MergeAppend when possible);
> Optimization of hash join when both tables are patitioned by join key.
>
> I'd like to validate that this development plan doesn't overlaps with your
> plans.  If out plans are not overlapping then let's accept this plan of work
> for 9.7.

It looks OK to me.  Thanks for sharing it.

Thanks,
Amit


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

Re: [HACKERS] Make primnodes.h gender neutral

[Robert Haas]

On Thu, Mar 17, 2016 at 3:31 PM, Joshua D. Drake  wrote:
> Per the twitter verse, here is an updated version of primnodes.h

+1.

-- 
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] Relaxing SSL key permission checks

[Peter Eisentraut]

Committed with the discussed adjustment and documentation update.

On 3/18/16 2:26 PM, Christoph Berg wrote:
> Re: Peter Eisentraut 2016-03-16 <56e8c221.1050...@gmx.net>
 * it failed to check for S_IXUSR, so permissions 0700 were okay, in
 contradiction with what the error message indicates.  This is a
 preexisting bug actually.  Do we want to fix it by preventing a
 user-executable file (possibly breaking compability with existing
 executable key files), or do we want to document what the restriction
 really is?
>>>
>>> I think we should not check for S_IXUSR.  There is no reason for doing that.
>>>
>>> I can imagine that key files are sometimes copied around using USB
>>> drives with FAT file systems or other means of that sort where
>>> permissions can scrambled.  While I hate gratuitous executable bits as
>>> much as the next person, insisting here would just create annoyances in
>>> practice.
>>
>> I'm happy with this patch except this minor point.  Any final comments?
> 
> I'm fine with that change.
> 
> Do you want me to update the patch or do you already have a new
> version, given it's marked as Ready for Committer?
> 
> Christoph
> 



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

[Corey Huinker]

On Thu, Mar 17, 2016 at 12:04 PM, Tom Lane  wrote:

> David Steele  writes:
> > On 3/17/16 11:30 AM, David G. Johnston wrote:
> >> ​I'd call it "generate_dates(...)" and be done with it.
> >> We would then have:
> >> generate_series()
> >> generate_subscripts()
> >> generate_dates()
>
> > To me this completely negates the idea of this "just working" which is
> > why it got a +1 from me in the first place.  If I have to remember to
> > use a different function name then I'd prefer to just cast on the
> > timestamp version of generate_series().
>
> Yeah, this point greatly weakens the desirability of this function IMO.
> I've also gone from "don't care" to "-1".
>
> regards, tom lane
>

Since that diminishes the already moderate support for this patch, I'll
withdraw it.

Re: [HACKERS] Make primnodes.h gender neutral

[Robert Haas]

On Thu, Mar 17, 2016 at 6:34 PM, Chapman Flack  wrote:
> For those of us who are outside of the twitterverse sort of on purpose,
> are there a few representative links you could post? Maybe this is such
> fresh breaking news Google hasn't spidered it yet, but I didn't find
> any reference to the primnodes language when I looked, and I really am
> curious to see just exactly what kind of issue is being made around it

Not to pick on you in particular, but rather in general response to
everyone who has expressed doubt about this idea:

Debating whether or not somebody is currently upset about this, and
how upset the are, and what the value is of fixing it is missing the
point.  When somebody sends a patch for a typographical error, we
don't say: well, we could fix that typographical error, but let's wait
until the next time we have cause to reword the paragraph.  We just
commit the patch.  Now, I realize this is not quite the same thing,
because, as Tom rightly points out, it is possible to degrade the
readability of comments or documentation in the pursuit of political
correctness, and we should not do that.  On the other hand, if we
found a comment somewhere in our source code that implied that all of
our users were white, or that they were all English-speaking, or that
one American political party was more worthy than the other, we
wouldn't sit around debating whether it was worth the effort to fix
it.  We would just fix it, even if it meant changing a few surrounding
words rather than just one or two.  And it wouldn't be that much work,
and it wouldn't cause major features to slip out of the release, and
it wouldn't be a waste of time.

Similarly here.  The comment implies that the user is male.  It
shouldn't.  Let's fix it in whatever way is most expedient and move
on.  If at some point we are overwhelmed with a slough of patches
making similar changes, we can at that time ask for them to be
consolidated, just as we would do for typo fixes, grammar fixes, or
warning fixes.  It is not necessary to insist on that that now because
we are not faced with any such issue at this time.  If we gain a
reputation as a community that is not willing to make reasonable
efforts to use gender-neutral language, it will hurt us far more than
the comment itself.  Let's not go there.

-- 
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] [BUGS] pgbench -C -M prepared gives an error

[Tatsuo Ishii]

> It's certainly a bug that the combination of the switches doesn't work,
> and I already fixed it (47211af17a).  My question was more towards
> whether -C is a useful benchmarking option at all.  I cannot imagine
> a situation in which, if someone said "I'm doing only one transaction per
> session, and I have a performance problem", I would not answer "yes,
> and you just explained why".

You could use -f option to execute multiple transactions in a session
using a custom script file.

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


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

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

[Alvaro Herrera]

Abhijit Menon-Sen wrote:
> At 2016-01-18 11:08:19 +0530, a...@2ndquadrant.com wrote:
> >
> > I'm proposing to address a part of that problem by allowing extension
> > dependencies to be explicitly declared for functions and objects
> > created either by a user or dynamically by the extension itself—things
> > that need the extension to function, but aren't a part of it.
> 
> I didn't hear any further suggestions, so here's a patch for discussion.
> 
> 1. This adds the 'x'/DEPENDENCY_AUTO_EXTENSION type.
> 2. This adds an 'ALTER FUNCTION … ADD DEPENDENT FUNCTION …' command.
> 
> I split up the two because we may want the new dependency type without
> going to the trouble of adding a new command. Maybe extension authors
> should just insert an 'x' row into pg_depend directly?

Surely not.  I don't think the first patch is acceptable standalone --
we need both things together.

> I was inclined to implement it using ALTER FUNCTION, but AlterFunction()
> is focused on altering the pg_proc entry for a function, so the new code
> didn't fit. Ultimately, ExecAlterExtensionContentsStmt() was the closest
> match, so that's where I did it.

Right, but see AlterObjectOwner() or ExecAlterObjectSchemaStmt() whereby
an arbitrary object has some property altered.  I think that's a closer
model for this.  It's still not quite the same, because those two
functions are still about modifying an object's catalog row rather than
messing with things outside of its own catalog.  But in reality,
pg_depend handling is mixed up with other changes all over the place.

Anyway I think this should be something along the lines of
ALTER FUNCTION foo() DEPENDS ON EXTENSION bar;
because it's really that object's behavior that you're modifying, not
the extension's.  Perhaps we could use the precedent that columns "own"
sequences when they use them in their default value, which would lead to
ALTER FUNCTION foo() OWNED BY EXTENSION bar;
(which might cause a problem when you want to mark sequences as
dependant on extensions, because we already have OWNED BY for them.  But
since EXTENSION is already a reserved word, maybe it's fine.)


I wondered whether it's right to be focusing solely on extensions as
being possible targets of such dependencies.  It's true that extensions
are the only "object containers" we have, but perhaps you want to mark a
function as dependant on some view, type, or another function, for
instance.  Another argument to focus only on extensions is that pg_dump
knows specifically about extensions for supressing objects to dump, and
we don't have any other object type doing the same kind of thing; so
perhaps extensions-only is fine.  I'm undecided on this.

-- 
Á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] Speedup twophase transactions

[David Steele]

On 1/26/16 3:39 PM, Stas Kelvich wrote:

Agree, I had the same idea in my mind when was writing that script.

I will migrate it to TAP suite and write a review for Michael Paquier's patch.

Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company



On 26 Jan 2016, at 20:20, Alvaro Herrera  wrote:

Stas Kelvich wrote:


While this patch touches quite sensible part of postgres replay and there is 
some rarely used code paths, I wrote shell script to setup master/slave 
replication and test different failure scenarios that can happened with 
instances. Attaching this file to show test scenarios that I have tested and 
more importantly to show what I didn’t tested. Particularly I failed to 
reproduce situation where StandbyTransactionIdIsPrepared() is called, may be 
somebody can suggest way how to force it’s usage. Also I’m not too sure about 
necessity of calling cache invalidation callbacks during 
XlogRedoFinishPrepared(), I’ve marked this place in patch with 2REVIEWER 
comment.


I think this is the third thread in which I say this: We need to push
Michael Paquier's recovery test framework, then convert your test script
to that.  That way we can put your tests in core.


It seems this thread has been waiting quite a while on a new patch.  If 
one doesn't appear by Monday I will mark this "returned with feedback".


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


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

Re: [HACKERS] Parallel Aggregate

[David Rowley]

On 18 March 2016 at 20:25, Amit Kapila  wrote:
> Few more comments:
>
> 1.
>
> + if (parse->groupClause)
> + path = (Path *) create_sort_path(root,
> + grouped_rel,
> + path,
> + root->group_pathkeys,
> + -1.0);
>
> For final path, why do you want to sort just for group by case?

If there's no GROUP BY then there will only be a single group, this
does not require sorting, e.g SELECT SUM(col) from sometable;

I added the comment:

/*
* Gather is always unsorted, so we'll need to sort, unless there's
* no GROUP BY clause, in which case there will only be a single
* group.
*/


> 2.
> + path = (Path *) create_gather_path(root, partial_grouped_rel, path,
> +   NULL, _groups);
> +
> + if (parse->groupClause)
> + path = (Path *) create_sort_path(root,
> + grouped_rel,
> + path,
> + root->group_pathkeys,
> + -1.0);
> +
> + if (parse->hasAggs)
> + add_path(grouped_rel, (Path *)
> + create_agg_path(root,
> + grouped_rel,
> + path,
> + target,
> + parse->groupClause ? AGG_SORTED : AGG_PLAIN,
> + parse->groupClause,
> + (List *) parse->havingQual,
> + _costs,
> + partial_grouped_rel->rows,
> + true,
> + true));
> + else
> + add_path(grouped_rel, (Path *)
> + create_group_path(root,
> +  grouped_rel,
> +  path,
> +  target,
> +  parse->groupClause,
> +  (List *) parse->havingQual,
> +  total_groups));
>
> In above part of patch, it seems you are using number of groups
> differenetly; for create_group_path() and create_gather_path(), you have
> used total_groups whereas for create_agg_path() partial_grouped_rel->rows is
> used, is there a reason for the same?

That's a mistake... too much code shuffling yesterday it seems.

> 3.
> + if (grouped_rel->partial_pathlist)
> + {
> + Path   *path = (Path *)
> linitial(grouped_rel->partial_pathlist);
> + double total_groups;
> +
> + total_groups =
> path->rows * path->parallel_degree;
> + path = (Path *) create_gather_path(root, partial_grouped_rel,
> path,
> +   NULL, _groups);
>
> A. Won't passing partial_grouped_rel lead to incomplete information required
> by create_gather_path() w.r.t the case of parameterized path info?

There should be no parameterized path info after joins are over, but
never-the-less I took your advice about passing PathTarget to
create_gather_path(), so this partial_grouped_rel no longer exists.

> B. You have mentioned that passing grouped_rel will make gather path contain
> the information of final path target, but what is the problem with that? I
> mean to ask why Gather node is required to contain partial path target
> information instead of final path target.

Imagine a query such as: SELECT col,SUM(this) FROM sometable GROUP BY
col HAVING SUM(somecolumn) > 0;

In this case SUM(somecolumn) won't be in the final PathTarget. The
partial grouping target will contain the Aggref from the HAVING
clause. The other difference with te partial aggregate PathTarget is
that the Aggrefs return the partial state in exprType() rather than
the final value's type, which is required so the executor knows how to
form and deform tuples, plus many other things.

> C. Can we consider passing pathtarget to create_gather_path() as that seems
> to save us from inventing new UpperRelationKind? If you are worried about
> adding the new parameter (pathtarget) to create_gather_path(), then I think
> we are already passing it in many other path generation functions, so why
> not for gather path generation as well?

That's a better idea... Changed to that...

> 4A.
> Overall function create_grouping_paths() looks better than previous, but I
> think still it is difficult to read.  I think it can be improved by
> generating partial aggregate paths separately as we do for nestloop join
> refer function consider_parallel_nestloop

hmm, perhaps the partial path generation could be moved off to another
static function, although we'd need to pass quite a few parameters to
it, like can_sort, can_hash, partial_grouping_target, grouped_rel,
root. Perhaps it's worth doing, but we still need the
partial_grouping_target for the Gather node, so it's not like that
other function can do all of the parallel stuff... We'd still need
some knowledge of that in create_grouping_paths()

> 4B.
> Rather than directly using create_gather_path(), can't we use
> generate_gather_paths as for all places where we generate gather node,
> generate_gather_paths() is used.

I don't think this is a good fit here, although it would be nice as it
would save having to special case generating the final aggregate paths
on the top of the partial paths. It does not seem that nice as it's
not really that clear if we need to make a combine aggregate node, or
a normal aggregate node on the path. The only way to determine that
would by by checking if it was a GatherPath or not, and that does not
seem like a nice way to go about doing that. Someone might go and
invent something new like MergeGather one day.


> 5.
> +make_partialgroup_input_target(PlannerInfo *root, PathTarget *final_target)
> {

Re: [HACKERS] [GENERAL] Request - repeat value of \pset title during \watch interations

[Tom Lane]

David Steele  writes:
> On 3/17/16 7:00 PM, Tom Lane wrote:
>> The message I saw was post-1-March.  If it was in fact submitted in
>> time for 2016-03, then we owe it a review.

> I meant to add the CF record and forgot:
> https://commitfest.postgresql.org/9/480
> It was added 2016-01-13 by Michael Paquier.

OK, so it looks like David's 10-Mar patch was actually just a repost of
Michael's 28-Jan patch, which was already in the queue to be reviewed in
2016-03 (and hasn't yet been).  So the addition to 2016-09 was simply
erroneous and should be deleted.

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] Performance degradation in commit ac1d794

[Andres Freund]

Hi,

On 2016-03-16 15:41:28 -0400, Robert Haas wrote:
> On Wed, Mar 16, 2016 at 3:25 PM, Andres Freund  wrote:
> >> > - Given current users we don't need a large amount of events, so having
> >> >   to iterate through the registered events doesn't seem bothersome. We
> >> >   could however change the api to be something like
> >> >
> >> >   int WaitLatchEventSet(LatchEventSet *set, OccurredEvents *, int 
> >> > nevents, long timeout);
> >> >
> >> >   which would return the number of events that happened, and would
> >> >   basically "fill" one of the (usually stack allocated) OccurredEvent
> >> >   structures with what happened.
> >>
> >> I definitely think something along these lines is useful.  I want to
> >> be able to have an Append node with 100 ForeignScans under it and kick
> >> off all the scans asynchronously and wait for all of the FDs at once.
> >
> > So you'd like to get only an event for the FD with data back? Or are you
> > ok with iterating through hundred elements in an array, to see which are
> > ready?
> 
> I'd like to get an event back for the FD with data.  Iterating sounds
> like it could be really slow.  Say you get lots of little packets back
> from the same connection, while the others are idle.  Now you've got
> to keep iterating through them all over and over again.  Blech.

I've a (working) WIP version that works like I think you want.  It's
implemented in patch 05 (rework with just poll() support) and (add epoll
suppport). It's based on patches posted here earlier, but these aren't
interesting for the discussion.

The API is now:

typedef struct WaitEventSet WaitEventSet;

typedef struct WaitEvent
{
int pos;/* position in the event data structure 
*/
uint32  events; /* tripped events */
int fd; /* fd associated with event */
} WaitEvent;

/*
 * Create a WaitEventSet with space for nevents different events to wait for.
 *
 * latch may be NULL.
 */
extern WaitEventSet *CreateWaitEventSet(int nevents);

/* ---
 * Add an event to the set. Possible events are:
 * - WL_LATCH_SET: Wait for the latch to be set
 * - WL_POSTMASTER_DEATH: Wait for postmaster to die
 * - WL_SOCKET_READABLE: Wait for socket to become readable
 *   can be combined in one event with WL_SOCKET_WRITEABLE
 * - WL_SOCKET_WRITABLE: Wait for socket to become readable
 *   can be combined with WL_SOCKET_READABLE
 *
 * Returns the offset in WaitEventSet->events (starting from 0), which can be
 * used to modify previously added wait events.
 */
extern int AddWaitEventToSet(WaitEventSet *set, uint32 events, int fd, Latch 
*latch);

/*
 * Change the event mask and, if applicable, the associated latch of of a
 * WaitEvent.
 */
extern void ModifyWaitEvent(WaitEventSet *set, int pos, uint32 events, Latch 
*latch);

/*
 * Wait for events added to the set to happen, or until the timeout is
 * reached.  At most nevents occurrent events are returned.
 *
 * Returns the number of events occurred, or 0 if the timeout was reached.
 */
extern int WaitEventSetWait(WaitEventSet *set, long timeout, WaitEvent* 
occurred_events, int nevents);


I've for now left the old latch API in place, and only converted
be-secure.c to the new style.  I'd appreciate some feedback before I go
around and convert and polish everything.

Questions:
* I'm kinda inclined to merge the win32 and unix latch
  implementations. There's already a fair bit in common, and this is
  just going to increase that amount.

* Right now the caller has to allocate the WaitEvents he's waiting for
  locally (likely on the stack), but we also could allocate them as part
  of the WaitEventSet. Not sure if that'd be a benefit.

* I can do a blind rewrite of the windows implementation, but I'm
  obviously not going to get that entirely right. So I need some help
  from a windows person to test this.

* This approach, with a 'stateful' wait event data structure, will
  actually allow to fix a couple linering bugs we have on the windows
  port. C.f. http://www.postgresql.org/message-id/4351.1336927...@sss.pgh.pa.us

- Andres
>From a692a0bd6a8af7427d491adfecff10df3953a8ae Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Wed, 16 Mar 2016 10:44:04 -0700
Subject: [PATCH 1/6] Access the correct pollfd when checking for socket errors
 in the latch code.

Previously, if waiting for a latch, but not a socket, we checked
pfds[0].revents for socket errors. Even though pfds[0] wasn't actually
associated with the socket in that case.

This is currently harmless, because we check wakeEvents after the the
aforementioned check. But it's a bug waiting to be happening.
---
 src/backend/port/unix_latch.c | 16 ++--
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/src/backend/port/unix_latch.c b/src/backend/port/unix_latch.c
index 2ad609c..2ffce60 100644
--- a/src/backend/port/unix_latch.c
+++ b/src/backend/port/unix_latch.c
@@ -365,13 +365,17 

Re: [HACKERS] Make primnodes.h gender neutral

[Mark Dilger]

> On Mar 17, 2016, at 5:05 PM, Peter Geoghegan  wrote:
> 
> On Thu, Mar 17, 2016 at 4:46 PM, Tom Lane  wrote:
>> Alvaro's original complaint that the sentences no longer agree as to
>> person is on-point.
> 
> That's reasonable. Still, there are only a few existing instances of
> gendered pronouns in the code, so fixing them carefully, without
> losing anything important seems like a relatively straightforward
> task.

I have compiled a list of all the ones I found, manually excluding instances
where the pronoun is appropriate:

./configure.in
--
  line 751: # so we make the user say which one she wants.

./doc/src/sgml/release-7.4.sgml
---
  line 223:   a database he owns, this would remove all special parameter 
settings

./doc/src/sgml/release-8.0.sgml
---
  line 293:   a database he owns, this would remove all special parameter 
settings

./doc/src/sgml/release-8.1.sgml
---
  line 520:   a database he owns, this would remove all special parameter 
settings
 line 3446:Once a user logs into a role, she obtains capabilities of
 line 3448:SET ROLE to switch to other roles she is a 
member of.

./doc/src/sgml/release-8.2.sgml
---
 line 1423:   a database he owns, this would remove all special parameter 
settings

./doc/src/sgml/release-8.3.sgml
---
 line 1072:   function with forged input data, by installing it on a table 
he owns.
 line 2996:   a database he owns, this would remove all special parameter 
settings

./doc/src/sgml/release-8.4.sgml
---
  line 492:   revoke the access of others, contrary to the wishes of his 
grantor.
  line 494:   uncooperative role member could provide most of his rights to 
others
 line 2719:   function with forged input data, by installing it on a table 
he owns.
 line 5223:   a database he owns, this would remove all special parameter 
settings

./doc/src/sgml/release-9.0.sgml
---
 line 1220:   within a command parameter might succeed in injecting his own 
SQL
 line 2382:   revoke the access of others, contrary to the wishes of his 
grantor.
 line 2384:   uncooperative role member could provide most of his rights to 
others
 line 5089:   function with forged input data, by installing it on a table 
he owns.

./doc/src/sgml/release-9.1.sgml
---
 line 1867:   within a command parameter might succeed in injecting his own 
SQL
 line 3164:   revoke the access of others, contrary to the wishes of his 
grantor.
 line 3166:   uncooperative role member could provide most of his rights to 
others
 line 6551:   function with forged input data, by installing it on a table 
he owns.

./doc/src/sgml/release-9.2.sgml
---
 line 2006:   within a command parameter might succeed in injecting his own 
SQL
 line 3521:   revoke the access of others, contrary to the wishes of his 
grantor.
 line 3523:   uncooperative role member could provide most of his rights to 
others

./doc/src/sgml/release-9.3.sgml
---
 line 2273:   within a command parameter might succeed in injecting his own 
SQL
 line 5641:   revoke the access of others, contrary to the wishes of his 
grantor.
 line 5643:   uncooperative role member could provide most of his rights to 
others

./doc/src/sgml/release-9.4.sgml
---
 line 4394:   within a command parameter might succeed in injecting his own 
SQL

./doc/src/sgml/user-manag.sgml
--
  line 132:need not match his or her real name.)  Since the role

./src/backend/access/hash/README

  line 446: page acquirer will scan more bitmap bits than he needs to.  What 
must be

./src/backend/access/heap/heapam.c
--
 line 5274:  * It's a committed update, so we need to preserve him 
as updater of
 line 5381:  * It's a committed update, so we gotta preserve him as 
updater of the
 line 6557:  * the VACUUM and perhaps truncate off the part of pg_clog he 
needs.  Getting

./src/backend/access/index/genam.c
--
   line 48:  *  whatever kind of locking he wants.

./src/backend/access/transam/README
---
  line 108:she sees and types ABORT(syntax error, etc)

./src/backend/access/transam/twophase.c
---
  line 624:  * yet.  The caller should filter them out if he doesn't want them.

./src/backend/access/transam/xact.c
---
 line 3004:  * will interpret the error as meaning the 
BEGIN failed to get him

./src/backend/access/transam/xlog.c

Re: [HACKERS] Parallel Aggregate

[David Rowley]

On 17 March 2016 at 18:05, David Rowley  wrote:
> Updated patch attached.

Please disregard
0001-Allow-aggregation-to-happen-in-parallel_2016-03-17.patch. This
contained a badly thought through last minute change to how the Gather
path is generated and is broken.

I played around with ways of generating the Gather node as
create_gather_path() is not really geared up for what's needed here,
since grouped_rel cannot be passed into create_gather_path() since it
contains the final aggregate PathTarget rather than the partial
PathTarget, and also incorrect row estimates. I've ended up with an
extra double *rows argument in this function to make it possible to
override the rows from rel. I'm not sure how this'll go down... It
does not seem perfect.

In the end I've now added a new upper planner type to allow me to
create a RelOptInfo for the partial aggregate relation, so that I can
pass create_gather_path() a relation with the correct PathTarget. This
seemed better than borrowing grouped_rel, then overriding the
reltarget after create_gather_path() returned. Although I'm willing to
consider the option that someone will disagree with how I've done
things here.

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


0001-Allow-aggregation-to-happen-in-parallel_2016-03-18.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] Choosing parallel_degree

[Julien Rouhaud]

On 17/03/2016 12:21, David Rowley wrote:
> On 18 March 2016 at 00:13, Julien Rouhaud  wrote:
>> With the current threshold, you need a table bigger than 8 MB to be able
>> to force parallel workers. I'm not sure there'll be benefits for
>> multiple workers on a table smaller than 8 MB, since setting up all the
>> parallel stuff takes time.
> 
> It would be really nice if it were possible to drop the setting really
> low, so that combined with a low parallel_setup_cost we could enable
> parallel query on small tables in the regression test suite.
> 
> 

Indeed. That could also be a use case for moving parallel_threshold to a
GUC, but not sure what'd be best.

-- 
Julien Rouhaud
http://dalibo.com - http://dalibo.org


-- 
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] Move PinBuffer and UnpinBuffer to atomics

[Alexander Korotkov]

On Sat, Mar 19, 2016 at 3:22 PM, Dilip Kumar  wrote:

>
> On Mon, Mar 14, 2016 at 3:09 AM, Alexander Korotkov <
> a.korot...@postgrespro.ru> wrote:
>
>> I've drawn graphs for these measurements. The variation doesn't look
>> random here.  TPS is going higher from measurement to measurement.  I bet
>> you did measurements sequentially.
>> I think we should do more measurements until TPS stop growing and beyond
>> to see accumulate average statistics.  Could you please do the same tests
>> but 50 runs.
>>
>
> I have taken reading at different client count (1,2 and 4)
>
> 1. Avg: Is taken average after discarding lowest 5 and highest 5 readings.
> 2. With 4 client I have only 30 reading.
>
> Summary:
>
> I think if we check avg or median atleast at 1 or 2 client head always
> looks winner, but same is not the case with 4 clients. And with 4 clients I
> can see much more fluctuation in the reading.
>
>
> Head(1 Client) Patch (1 Client)
> Head (2 Client) Patch (2 Client)
> Head (4 Client) Patch (4 Client)
> Avg: 19628 19578
> 37180 36536
> 70044 70731
> Median: 19663 19581
> 37967 37484
> 73003 75376
> Below is all the reading. (Arranged in sorted order)
>
> *Runs* *Head (1 Client)* *Patch (1 Client)*
> *Head (2 Client)* *Patch (2 Client)*
> *Head (4 Client)* *Patch (4 Client)*
> 1 18191 18153
> 29454 26128
> 49931 47210
> 2 18365 18768
> 31218 26956
> 53358 47287
> 3 19053 18769
> 31808 29286
> 53575 55458
> 4 19128 18915
> 31860 30558
> 54282 55794
> 5 19160 18948
> 32411 30945
> 56629 56253
> 6 19177 19055
> 32453 31316
> 57595 58147
> 7 19351 19232
> 32571 31703
> 59490 58543
> 8 19353 19239
> 33294 32029
> 63887 58990
> 9 19361 19255
> 33936 32217
> 64718 60233
> 10 19390 19297
> 34361 32767
> 65737 68210
> 11 19452 19368
> 34563 34487
> 65881 71409
> 12 19478 19387
> 36110 34907
> 67151 72108
> 13 19488 19388
> 36221 34936
> 70974 73977
> 14 19490 19395
> 36461 35068
> 72212 74891
> 15 19512 19406
> 36712 35298
> 73003 75376
> 16 19538 19450
> 37104 35492
> 74842 75468
> 17 19547 19487
> 37246 36648
> 75400 75515
> 18 19592 19521
> 37567 37263
> 75573 75626
> 19 19627 19536
> 37707 37430
> 75798 75745
> 20 19661 19556
> 37958 37461
> 75834 75868
> 21 19666 19607
> 37976 37507
> 76240 76161
> 22 19701 19624
> 38060 37557
> 76426 76162
> 23 19708 19643
> 38244 37717
> 76770 76333
> 24 19748 19684
> 38272 38285
> 77011 77009
> 25 19751 19694
> 38467 38294
> 77114 77168
> 26 19776 19695
> 38524 38469
> 77630 77318
> 27 19781 19709
> 38625 38642
> 77865 77550
> 28 19786 19765
> 38756 38643
> 77912 77904
> 29 19796 19823
> 38939 38649
> 78242 78736
> 30 19826 19847
> 39139 38659
>
>
> 31 19837 19899
> 39208 38713
>
>
> 32 19849 19909
> 39211 38837
>
>
> 33 19854 19932
> 39230 38876
>
>
> 34 19867 19949
> 39249 39088
>
>
> 35 19891 19990
> 39259 39148
>
>
> 36 20038 20085
> 39286 39453
>
>
> 37 20083 20128
> 39435 39563
>
>
> 38 20143 20166
> 39448 39959
>
>
> 39 20191 20198
> 39475 40495
>
>
> 40 20437 20455
> 40375 40664
>
>
>
So, I think it's really some regression here on small number clients.  I
have following hypothesis about that.  In some cases we use more atomic
operations than before. For instace, in BufferAlloc we have following block
of code.  Previous code deals with that without atomic operations relying
on spinlock.  So, we have 2 extra atomic operations here, and similar
situation in other places.

pg_atomic_fetch_and_u32(>state, ~(BM_VALID | BM_DIRTY |
BM_JUST_DIRTIED | BM_CHECKPOINT_NEEDED | BM_IO_ERROR | BM_PERMANENT |
BUF_USAGECOUNT_MASK)); if (relpersistence == RELPERSISTENCE_PERMANENT)
pg_atomic_fetch_or_u32(>state, BM_TAG_VALID | BM_PERMANENT |
BUF_USAGECOUNT_ONE); else pg_atomic_fetch_or_u32(>state, BM_TAG_VALID
| BUF_USAGECOUNT_ONE);
Actually, we behave like old code and do such modifications without
increasing number of atomic operations.  We can just calculate new value of
state (including unset of BM_LOCKED flag) and write it to the buffer.  In
this case we don't require more atomic operations.  However, it becomes not
safe to use atomic decrement in UnpinBuffer().  We can use there loop of
CAS which wait buffer header to be unlocked like PinBuffer() does.  I'm not
sure if this affects multicore scalability, but I think it worth trying.

So, this idea is implemented in attached patch.  Please, try it for both
regression on lower number of clients and scalability on large number of
clients.

Other changes in this patch:
1) PinBuffer() and UnpinBuffer() use exponential backoff in spindealy
like LockBufHdr() does.
2) Previous patch contained revert
of ac1d7945f866b1928c2554c0f80fd52d7f92.  This was not intentional.
No, it doesn't contains this revert.

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


pinunpin-cas-4.patch
Description: Binary data

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

Re: [HACKERS] pg_ctl promote wait

[David Steele]

Hi Peter,

On 3/9/16 3:08 PM, Michael Paquier wrote:

> Here are some comments about 0002
<...>
> I think that we had better do something like the attached first.
> Thoughts?

It's been a week since Michael reviewed this patch.  Could you please
comment on his proposed changes as soon as possible?

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


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

Re: [HACKERS] Parallel Aggregate

[Robert Haas]

On Wed, Mar 16, 2016 at 6:49 AM, David Rowley
 wrote:
> On 16 March 2016 at 15:04, Robert Haas  wrote:
>> I don't think I'd be objecting if you made PartialAggref a real
>> alternative to Aggref.  But that's not what you've got here.  A
>> PartialAggref is just a wrapper around an underlying Aggref that
>> changes the interpretation of it - and I think that's not a good idea.
>> If you want to have Aggref and PartialAggref as truly parallel node
>> types, that seems cool, and possibly better than what you've got here
>> now.  Alternatively, Aggref can do everything.  But I don't think we
>> should go with this wrapper concept.
>
> Ok, I've now gotten rid of the PartialAggref node, and I'm actually
> quite happy with how it turned out. I made
> search_indexed_tlist_for_partial_aggref() to follow-on the series of
> other search_indexed_tlist_for_* functions and have made it behave the
> same way, by returning the newly created Var instead of doing that in
> fix_combine_agg_expr_mutator(), as the last version did.
>
> Thanks for the suggestion.
>
> New patch attached.

Cool!  Why not initialize aggpartialtype always?

-- 
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] Make primnodes.h gender neutral

[David G. Johnston]

On Thursday, March 17, 2016, Robert Haas  wrote:

> On Thu, Mar 17, 2016 at 6:34 PM, Chapman Flack  > wrote:
> > For those of us who are outside of the twitterverse sort of on purpose,
> > are there a few representative links you could post? Maybe this is such
> > fresh breaking news Google hasn't spidered it yet, but I didn't find
> > any reference to the primnodes language when I looked, and I really am
> > curious to see just exactly what kind of issue is being made around
> it
>
> Similarly here.  The comment implies that the user is male.  It
> shouldn't.  Let's fix it in whatever way is most expedient and move
> on.  If at some point we are overwhelmed with a slough of patches
> making similar changes, we can at that time ask for them to be
> consolidated, just as we would do for typo fixes, grammar fixes, or
> warning fixes.  It is not necessary to insist on that that now because
> we are not faced with any such issue at this time.  If we gain a
> reputation as a community that is not willing to make reasonable
> efforts to use gender-neutral language, it will hurt us far more than
> the comment itself.  Let's not go there.


>
And if someone had just posted the patch and not prefaced it "we need to
check all of our comments for gender usage" it probably would have slipped
thorough without comment.

But that isn't what happened and since, based upon previous comments, we
expected many other commits of this sort we rightly discussed how to do
it.  The original author indeed figured one patch per file was probably a
good way to do things but I can others would seem to prefer one targeted
patch.

Beyond that it's the usual bike shedding when it comes to writing...which
was the original contra-vote.

David J.

Re: [HACKERS] Parallel Aggregate

[Haribabu Kommi]

On Thu, Mar 17, 2016 at 2:13 PM, James Sewell  wrote:
>
> Hi again,
>
> This is probably me missing something, but is there a reason parallel 
> aggregate doesn't seem to ever create append nodes containing Index scans?
>
> SET random_page_cost TO 0.2;
> SET max_parallel_degree TO 8;
>
> postgres=# explain SELECT sum(count_i) FROM base GROUP BY view_time_day;
>QUERY PLAN
> -
>  Finalize GroupAggregate  (cost=310596.32..310598.03 rows=31 width=16)
>Group Key: view_time_day
>->  Sort  (cost=310596.32..310596.79 rows=186 width=16)
>  Sort Key: view_time_day
>  ->  Gather  (cost=310589.00..310589.31 rows=186 width=16)
>Number of Workers: 5
>->  Partial HashAggregate  (cost=310589.00..310589.31 rows=31 
> width=16)
>  Group Key: view_time_day
>  ->  Parallel Seq Scan on base  (cost=0.00..280589.00 
> rows=600 width=12)
>
>
> SET max_parallel_degree TO 0;
>
> postgres=# explain SELECT sum(count_i) FROM base GROUP BY view_time_day;
> QUERY PLAN
> ---
>  GroupAggregate  (cost=0.56..600085.92 rows=31 width=16)
>Group Key: view_time_day
>->  Index Only Scan using base_view_time_day_count_i_idx on base  
> (cost=0.56..450085.61 rows=3000 width=12)
> (3 rows)


To get good parallelism benefit, the workers has to execute most of
the plan in parallel.
If we run only some part of the upper plan in parallel, we may not get
better parallelism
benefit. At present only seq scan node possible for parallelism at
scan node level.
Index scan is not possible as of now. So because of this reason based
on the overall
cost of the parallel aggregate + parallel seq scan, the plan is chosen.

If index scan is changed to make it parallel in future, it is possible
that parallel aggregate +
parallel index scan plan may chosen.

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] fd.c: flush data problems on osx

[Stas Kelvich]

> On 17 Mar 2016, at 20:23, Andres Freund  wrote:
> 
>> 
>> Also there are no default ifdef inside this function, is there any
>> check that will guarantee that pg_flush_data will not end up with
>> empty body on some platform?
> 
> There doesn't need to be - it's purely "advisory", i.e. just an
> optimization.


Ah, okay, then I misinterpret purpose of that function and it shouldn’t be 
forced to sync.

> 
>> One possible solution for that is just fallback to pg_fdatasync in case when 
>> offset = nbytes = 0.
> 
> Hm, that's a bit heavyweight. I'd rather do an lseek(SEEK_END) to get
> the file size. Could you test that?
> 

It looks like OSX mmap raises EINVAL when length isn’t aligned to pagesize 
while manual says it can be of arbitrary length, so i aligned it.
Also there were call to mmap with PROT_READ | PROT_WRITE, but when called from 
pre_sync_fname file descriptor is just O_RDONLY, so i changed mmap mode to 
PROT_READ — seems that PROT_WRITE wasn’t needed anyway.

And all of that reduces number of warnings in order of magnitude but there are 
still some and I don’t yet understand why are they happening.

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

---
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company



flushdata.v2.patch
Description: Binary data




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