date:20180731

> "Andres" == Andres Freund  writes:

 >> Sure, it works for the simple cases like hstore, but how does it
 >> handle the case of a pgxs extension that installs more than one
 >> include file, one of which includes another?

 Andres> I might be momentarily daft (just was on a conference call for
 Andres> a while ;)), but why'd that be problematic? The header files
 Andres> can just specify the full path, and they'll find each other
 Andres> because of the aforementioned -I?

The #includes in the header files need to work both for the module's own
build, and also for building modules depending on it.

So existing practice would be that the module's own source dir (for
module "foo") would look something like:

./Makefile
./foo.h(contains #include "foo_2.h")
./foo_2.h
./foo.c(contains #include "foo.h")

and eventually foo.h and foo_int.h get installed as
$(includedir_server)/extension/foo/foo.h and /foo_2.h

To make this work with Tom's scheme means changing the directory layout
of the original module. For contrib modules it's easy because the "." in
the above paths is already "contrib/foo", but for PGXS the module should
not be making assumptions about the name of its build dir, so you end up
having to rejig the layout to something like

./Makefile
./foo
./foo/foo.h (contains #include "foo/foo_2.h")
./foo/foo_2.h
./foo.c  (contains #include "foo/foo.h", compiled with -I$(srcdir))

or

./Makefile
./include/foo
./include/foo/foo.h
./include/foo/foo_2.h
./foo.c  (compiled with -I$(srcdir)/include)

Either way, it's a forced change to the PGXS module's file layout if it
wants to install headers that work for other people using Tom's
suggested approach. I'm not on board with this unless there's a better
solution than I've seen so far.

-- 
Andrew (irc:RhodiumToad)

Re: Documentaion fix.

2018-07-31 Thread Kyotaro HORIGUCHI

At Wed, 1 Aug 2018 02:47:32 +0900, Michael Paquier  wrote 
in <20180731174732.gd2...@paquier.xyz>
> On Tue, Jul 31, 2018 at 10:59:14AM -0400, Alvaro Herrera wrote:
> > How about pasting it like this?
> > 
> > alvherre=# select * from pg_replication_slots \gx
> > ─[ RECORD 1 ]───┬
> > slot_name   │ node_a_slot
> > plugin  │ 
> > slot_type   │ physical
> > datoid  │ 
> > database│ 
> > temporary   │ f
> > active  │ f
> > active_pid  │ 
> > xmin│ 
> > catalog_xmin│ 
> > restart_lsn │ 
> > confirmed_flush_lsn │ 
> 
> Each time this catalog is changed, this would become incorrect.  My
> suggestion would be to change the query to that and call it a day:
> SELECT slot_name, slot_type, active FROM pg_replication_slots;

Yeah, I'm also concerned about the unstability. I didn't came up
with a list of columns that makes sense at that time but I
rethought on that.

> slot_name   │ required
> plugin  │ 
> slot_type   │ required
> datoid  │ 
> database│ 
> temporary   │ 
> active  │ required?
> active_pid  │ 
> xmin│ 
> catalog_xmin│ 
> restart_lsn │ better to be shown?
> confirmed_flush_lsn │ 

The query and the result with four columns fit the current width.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 2b5788b3f1ecf4ebd31cf36eecdd4619cf3f394e Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Wed, 1 Aug 2018 12:41:03 +0900
Subject: [PATCH] Documentation fix of "Log-Shipping Standy Servers" for PG95.

The example output of pg_replication_slot is wrong. Correct and make
the output stable by explicitly specifying a part of the all columns
in pg_replication_slots.
---
 doc/src/sgml/high-availability.sgml | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index 35daf0103a..ae831192d0 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -931,10 +931,10 @@ postgres=# SELECT * FROM pg_create_physical_replication_slot('node_a_slot');
 -+---
  node_a_slot |
 
-postgres=# SELECT * FROM pg_replication_slots;
-  slot_name  | slot_type | datoid | database | active | xmin | restart_lsn
--+---++--++--+-
- node_a_slot | physical  ||  | f  |  |
+postgres=# SELECT slot_name, slot_type, active, restart_lsn FROM pg_replication_slots;
+  slot_name  | slot_type | active | restart_lsn 
+-+---++-
+ node_a_slot | physical  | f  | 
 (1 row)
 
  To configure the standby to use this slot, primary_slot_name
-- 
2.16.3

>From 4367cdea3d6ac19ef9c6ec1545fbc2329dda47bb Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Wed, 1 Aug 2018 11:50:03 +0900
Subject: [PATCH] Documentation fix of "Log-Shipping Standy Servers" for
 master.

The example output of pg_replication_slot is wrong. Correct and make
the output stable by explicitly specifying a part of the all columns
in pg_replication_slots.
---
 doc/src/sgml/high-availability.sgml | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/doc/src/sgml/high-availability.sgml b/doc/src/sgml/high-availability.sgml
index 934eb9052d..39cada1278 100644
--- a/doc/src/sgml/high-availability.sgml
+++ b/doc/src/sgml/high-availability.sgml
@@ -966,10 +966,10 @@ postgres=# SELECT * FROM pg_create_physical_replication_slot('node_a_slot');
 -+-
  node_a_slot |
 
-postgres=# SELECT * FROM pg_replication_slots;
-  slot_name  | slot_type | datoid | database | active | xmin | restart_lsn | confirmed_flush_lsn
--+---++--++--+-+-
- node_a_slot | physical  ||  | f  |  | |
+postgres=# SELECT slot_name, slot_type, active, restart_lsn FROM pg_replication_slots;
+  slot_name  | slot_type | active | restart_lsn 
+-+---++-
+ node_a_slot | physical  | f  | 
 (1 row)
 
  To configure the standby to use this slot, primary_slot_name
-- 
2.16.3

Re: pg_ugprade test failure on data set with column with default value with type bit/varbit

2018-07-31 Thread Paul Guo

Thanks. I updated the patch as attached.

Double-checked those tests passed.

2018-07-30 9:38 GMT+08:00 Thomas Munro :

> On Thu, May 17, 2018 at 8:20 PM, Paul Guo  wrote:
> > Thanks. I tentatively submitted a patch (See the attachment).
>
> Hi Paul,
>
> It looks like you missed a couple of changes in the contrib/btree_gist
> bit and varbit tests, so make check-world fails:
>
> - Index Cond: ((a >= B'100'::"bit") AND (a <= B'101'::"bit"))
> + Index Cond: ((a >= '100'::"bit") AND (a <= '101'::"bit"))
>
> --
> Thomas Munro
> http://www.enterprisedb.com
>


0001-Fix-pg_upgrade-test-failure-caused-by-the-DDL-below.v2.patch
Description: Binary data

Re: Should contrib modules install .h files?

On 2018-08-01 04:52:28 +0100, Andrew Gierth wrote:
> > "Tom" == Tom Lane  writes:
> 
>  >> If your extension is relying on pg11+, or you have checked the pg
>  >> version when constructing the makefile, you can just do:
>  >> PG_CPPFLAGS += -I$(includedir_server)/extension/hstore
>  >> and #include "hstore.h" will work.
> 
>  Tom> I remain of the opinion that it'd be smarter to do
> 
>  Tom> PG_CPPFLAGS += -I$(includedir_server)/extension
> 
>  Tom> then
> 
>  Tom> #include "hstore/hstore.h"
> 
>  Tom> This way requires fewer -I options and is far more robust against
>  Tom> header name conflicts.
> 
> Sure, it works for the simple cases like hstore, but how does it handle
> the case of a pgxs extension that installs more than one include file,
> one of which includes another?

I might be momentarily daft (just was on a conference call for a while
;)), but why'd that be problematic? The header files can just specify
the full path, and they'll find each other because of the aforementioned
-I?

Greetings,

Andres Freund

Re: Should contrib modules install .h files?

> "Tom" == Tom Lane  writes:

 >> If your extension is relying on pg11+, or you have checked the pg
 >> version when constructing the makefile, you can just do:
 >> PG_CPPFLAGS += -I$(includedir_server)/extension/hstore
 >> and #include "hstore.h" will work.

 Tom> I remain of the opinion that it'd be smarter to do

 Tom> PG_CPPFLAGS += -I$(includedir_server)/extension

 Tom> then

 Tom> #include "hstore/hstore.h"

 Tom> This way requires fewer -I options and is far more robust against
 Tom> header name conflicts.

Sure, it works for the simple cases like hstore, but how does it handle
the case of a pgxs extension that installs more than one include file,
one of which includes another?

-- 
Andrew (irc:RhodiumToad)

Re: pg_upgrade from 9.4 to 10.4

On Tue, Jul 31, 2018 at 08:05:06PM -0400, Andrew Dunstan wrote:
> On 07/31/2018 07:08 PM, Bruce Momjian wrote:
> >Oh, jacana must be a Windows server with spaces in the file name paths.
> >Fixed.  Thanks again.  I really didn't want to backpatch the original
> >fix but had to since it could produce corrupt upgrades.
> >
> 
> 
> It is a Windows machine, but there should be any spaces in the paths.

The comment at the top of src/port/system.c explains why we need those
quotes.  Spaces was not the issue.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Should contrib modules install .h files?

Andrew Gierth  writes:
> "Peter" == Peter Eisentraut  writes:
>  Peter> I'm missing some guidance what an extension using those headers
>  Peter> is supposed to do. How does it get the right -I options?

> If your extension is relying on pg11+, or you have checked the pg
> version when constructing the makefile, you can just do:
> PG_CPPFLAGS += -I$(includedir_server)/extension/hstore
> and #include "hstore.h" will work.

I remain of the opinion that it'd be smarter to do

PG_CPPFLAGS += -I$(includedir_server)/extension

then

#include "hstore/hstore.h"

This way requires fewer -I options and is far more robust against header
name conflicts.

regards, tom lane

Re: New Defects reported by Coverity Scan for PostgreSQL

Ning Yu  writes:
> From my point of view it's better to also put some comments for humans to
> understand why we are passing l1 and l2 in reverse order.

The header comment for lseg_closept_lseg() is pretty far from adequate
as well.  After studying it for awhile I think I've puzzled out the
indeterminacies, but I shouldn't have had to.  I think it probably
should have read

 * Closest point on line segment l2 to line segment l1
 *
 * This returns the minimum distance from l1 to the closest point on l2.
 * If result is not NULL, the closest point on l2 is stored at *result.

Or perhaps I have it backwards and "l1" and "l2" need to be swapped in
that description.  But the mere fact that there is any question about
that means that the function is poorly documented and perhaps poorly
named as well.  For that matter, is there a good reason why l1/l2
have those roles and not the reverse?

While Coverity is surely operating from a shaky heuristic here, it's
got a point.  The fact that it makes a difference which argument is
given first means that you've got to be really careful about which
is which, and this API spec is giving no help in that regard at all.

regards, tom lane

Re: [HACKERS] Restricting maximum keep segments by repslots

2018-07-31 Thread Kyotaro HORIGUCHI

At Tue, 31 Jul 2018 12:24:13 -0700, Andres Freund  wrote in 
<20180731192413.7lr4qbc4qbyoi...@alap3.anarazel.de>
> On 2018-07-31 15:21:27 -0400, Stephen Frost wrote:
> > Greetings,
> > 
> > * Andres Freund (and...@anarazel.de) wrote:
> > > On 2018-07-31 15:11:52 -0400, Bruce Momjian wrote:
> > > > On Tue, Jun 26, 2018 at 04:26:59PM +0900, Kyotaro HORIGUCHI wrote:
> > > > > Hello. This is the reabased version of slot-limit feature.
> > > > > 
> > > > > This patch limits maximum WAL segments to be kept by replication
> > > > > slots. Replication slot is useful to avoid desync with replicas
> > > > > after temporary disconnection but it is dangerous when some of
> > > > > replicas are lost. The WAL space can be exhausted and server can
> > > > > PANIC in the worst case. This can prevent the worst case having a
> > > > > benefit from replication slots using a new GUC variable
> > > > > max_slot_wal_keep_size.
> > > > 
> > > > Have you considered just using a boolean to control if max_wal_size
> > > > honors WAL preserved by replication slots, rather than creating the new
> > > > GUC max_slot_wal_keep_size?
> > > 
> > > That seems like a bad idea. max_wal_size influences checkpoint
> > > scheduling - there's no good reason to conflate that with retention?
> > 
> > I agree that we shouldn't conflate checkpointing and retention.  What I
> > wonder about though is what value will wal_keep_segments have once this
> > new GUC exists..?  I wonder if we could deprecate it...
> 
> Don't think that's a good idea. It's entirely conceivable to have a
> wal_keep_segments much lower than max_slot_wal_keep_size.  For some
> throwaway things it can be annoying to have to slots, and if you remove
> wal_keep_segments there's no alternative.

I thought it's to be deprecated for some reason so I'm leaving
wal_keep_segments in '# of segments' even though the new GUC is
in MB. I'm a bit uneasy that the two similar settings are in
different units. Couldn't we turn it into MB taking this
opportunity if we will keep wal_keep_segments, changing its name
to min_wal_keep_size?  max_slot_wal_keep_size could be changed to
just max_wal_keep_size along with it.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: New Defects reported by Coverity Scan for PostgreSQL

2018-07-31 Thread Ning Yu

To make coverity happy we might be able to suppress this false alarm by
adding a line like below:

```
/* coverity[SWAPPED_ARGUMENTS] */
lseg_closept_lseg(result, l2, l1);
```

>From my point of view it's better to also put some comments for humans to
understand why we are passing l1 and l2 in reverse order.

On Wed, Aug 1, 2018 at 1:21 AM, Alvaro Herrera 
wrote:

> Hello guys.  Coverity complained about this patch as below.  What, if
> anything, should be done about it?  One solution is to mark it as a
> false-positive in Coverity, of course.
>
> On 2018-Jul-29, scan-ad...@coverity.com wrote:
>
> > ** CID 1438146:  API usage errors  (SWAPPED_ARGUMENTS)
> >
> >
> > 
> 
> > *** CID 1438146:  API usage errors  (SWAPPED_ARGUMENTS)
> > /srv/coverity/git/pgsql-git/postgresql/src/backend/utils/adt/geo_ops.c:
> 2647 in close_lseg()
> > 2641  LSEG   *l1 = PG_GETARG_LSEG_P(0);
> > 2642  LSEG   *l2 = PG_GETARG_LSEG_P(1);
> > 2643  Point  *result;
> > 2644
> > 2645  result = (Point *) palloc(sizeof(Point));
> > 2646
> > >>> CID 1438146:  API usage errors  (SWAPPED_ARGUMENTS)
> > >>> The positions of arguments in the call to "lseg_closept_lseg" do
> not match the ordering of the parameters:
> > * "l2" is passed to "l1"
> > * "l1" is passed to "l2"
> > 2647  lseg_closept_lseg(result, l2, l1);
> > 2648
> > 2649  PG_RETURN_POINT_P(result);
> > 2650 }
> > 2651
> > 2652
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>

Re: pg_upgrade from 9.4 to 10.4

2018-07-31 Thread Andrew Dunstan





On 07/31/2018 07:08 PM, Bruce Momjian wrote:

On Tue, Jul 31, 2018 at 06:29:34PM -0400, Tom Lane wrote:

Bruce Momjian  writes:

On Tue, Jul 31, 2018 at 03:26:47PM -0400, Tom Lane wrote:

This patch evidently broke build farm member jacana, although only
in the 9.3 branch.  It's been failing with
Jul 28 23:22:30 The system cannot find the path specified.

Well, that's interesting.  I have a test script that upgrades all
version combinations of Postgres from 9.3 to head and it worked for me.
However, I now see that the tests for checking a live server were wrong,
and 9.3 must have a slightly different test than the others.  Fixes
applied.  Thanks for finding this so quickly.

After poking around a bit more, I think the more likely explanation is
that 9.3 was still using the SYSTEMQUOTE macro in commands to be popen'd,
and you did not make that adjustment when back-patching into that branch.
Note the adjacent pre-existing invocation of pg_controldata:

 snprintf(cmd, sizeof(cmd), SYSTEMQUOTE "\"%s/%s \"%s\"" SYSTEMQUOTE,
  cluster->bindir,
  live_check ? "pg_controldata\"" : "pg_resetxlog\" -n",

Oh, jacana must be a Windows server with spaces in the file name paths.
Fixed.  Thanks again.  I really didn't want to backpatch the original
fix but had to since it could produce corrupt upgrades.




It is a Windows machine, but there should be any spaces in the paths.

cheers

andrew

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

Re: Should contrib modules install .h files?

> "Peter" == Peter Eisentraut  writes:

 >> A review of contrib/ suggested that cube, hstore, isn, ltree and seg
 >> were the only modules that had useful headers to install, so do
 >> those.

 Peter> I'm missing some guidance what an extension using those headers
 Peter> is supposed to do. How does it get the right -I options?

All of the below assumes PGXS.

If your extension is relying on pg11+, or you have checked the pg
version when constructing the makefile, you can just do:

PG_CPPFLAGS += -I$(includedir_server)/extension/hstore

and #include "hstore.h" will work.

If you need to workaround for old versions in one makefile, as I do,
then you can do something along these lines (this goes before the
include $(PGXS) line):

# MAJORVERSION and includedir_server are not defined yet, but will be
# defined before PG_CPPFLAGS is expanded. So we use conditional
# expansions rather than 'ifeq' syntax.

# for pg11+, hstore.h will be installed here
HSTORE_INCDIR = $(includedir_server)/extension/hstore

# for pg 9.5/9.6/10, we have a local copy of hstore.h since it happens
# to be the same, barring non-semantic whitespace, between the three
# versions
HSTORE_INCDIR_OLD = old_inc

PG_CPPFLAGS += -I$(HSTORE_INCDIR$(if $(filter 9.% 10,$(MAJORVERSION)),_OLD))

If you need to distinguish more versions for whatever reason you can do
this:

HSTORE_INCDIR = $(includedir_server)/extension/hstore
HSTORE_INCDIR_10 =  whatever
HSTORE_INCDIR_9.6 = whatever
HSTORE_INCDIR_9.5 = whatever
HSTORE_INCDIR_9.4 = whatever
HSTORE_INCDIR_9.3 = whatever
PG_CPPFLAGS += -I$(HSTORE_INCDIR$(if $(filter 9.% 
10,$(MAJORVERSION)),_$(MAJORVERSION)))

-- 
Andrew (irc:RhodiumToad)

Re: pg_upgrade from 9.4 to 10.4

On Tue, Jul 31, 2018 at 06:29:34PM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > On Tue, Jul 31, 2018 at 03:26:47PM -0400, Tom Lane wrote:
> >> This patch evidently broke build farm member jacana, although only
> >> in the 9.3 branch.  It's been failing with
> >> Jul 28 23:22:30 The system cannot find the path specified.
> 
> > Well, that's interesting.  I have a test script that upgrades all
> > version combinations of Postgres from 9.3 to head and it worked for me. 
> > However, I now see that the tests for checking a live server were wrong,
> > and 9.3 must have a slightly different test than the others.  Fixes
> > applied.  Thanks for finding this so quickly.
> 
> After poking around a bit more, I think the more likely explanation is
> that 9.3 was still using the SYSTEMQUOTE macro in commands to be popen'd,
> and you did not make that adjustment when back-patching into that branch.
> Note the adjacent pre-existing invocation of pg_controldata:
> 
> snprintf(cmd, sizeof(cmd), SYSTEMQUOTE "\"%s/%s \"%s\"" SYSTEMQUOTE,
>  cluster->bindir,
>  live_check ? "pg_controldata\"" : "pg_resetxlog\" -n",

Oh, jacana must be a Windows server with spaces in the file name paths. 
Fixed.  Thanks again.  I really didn't want to backpatch the original
fix but had to since it could produce corrupt upgrades.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Online enabling of checksums

Hi,

On 2018-07-31 18:56:29 -0400, Alvaro Herrera wrote:
> In the spirit of supporting incremental development, I think it's quite
> sensible to get the current thing done, then see what it takes to get
> the next thing done.  Each is an improvement on its own merits.  And it
> doesn't have to be made by the same people.

I just don't buy this. An earlier version of this feature was committed
to v11 without the restart, over objections. There's now extra state in
the control file to support the restart based system, there's extra
tests, extra docs. And it'd not be much code to just make it work
without the restart. The process around this patchset is just plain
weird.

Greetings,

Andres Freund

Re: Online enabling of checksums

On 2018-Jul-31, Tom Lane wrote:

> Andres Freund  writes:
> > On 2018-07-31 23:20:27 +0200, Daniel Gustafsson wrote:
> >> Not really arguing for or against, but just to understand the reasoning 
> >> before
> >> starting hacking.  Why do we feel that a restart (intended for safety 
> >> here) in
> >> this case is a burden on a use-once process?  Is it from a usability or
> >> technical point of view?  Just want to make sure we are on the same page 
> >> before
> >> digging in to not hack on this patch in a direction which isn’t what is
> >> requested.
> 
> > Having, at some arbitrary seeming point in the middle of enabling
> > checksums to restart the server makes it harder to use and to schedule.
> > The restart is only needed to fix a relatively small issue, and doesn't
> > save that much code.
> 
> Without taking a position on the merits ... I don't see how you can
> claim "it doesn't save that much code" when we don't have a patch to
> compare to that doesn't require the restart.  Maybe it will turn out
> not to be much code, but we don't know that now.

The ability to get checksums enabled is a killer feature; the ability to
do it with no restart ... okay, it's better than requiring a restart,
but it's not *that* big a deal.

In the spirit of supporting incremental development, I think it's quite
sensible to get the current thing done, then see what it takes to get
the next thing done.  Each is an improvement on its own merits.  And it
doesn't have to be made by the same people.

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

Re: Should contrib modules install .h files?

On 31/07/2018 21:19, Andrew Gierth wrote:
>> "Andrew" == Andrew Gierth  writes:
> 
> Final patch just to let cfbot test the windows code for me.
> 
> A review of contrib/ suggested that cube, hstore, isn, ltree and seg
> were the only modules that had useful headers to install, so do those.

I'm missing some guidance what an extension using those headers is
supposed to do.  How does it get the right -I options?

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

Re: Should contrib modules install .h files?

On 31/07/2018 23:46, Andrew Gierth wrote:
> not pushing
> to 11-beta would mean delaying the issue for another year and forcing
> yet another cycle of workarounds.

But that applies to every single patch.

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

Re: Should contrib modules install .h files?

On 23/07/2018 18:32, Andrew Gierth wrote:
>> "Tom" == Tom Lane  writes:
> 
>  Tom> As I said before, I think that we should change the existing
>  Tom> contrib modules to be coded likewise, all using a single -I switch
>  Tom> that points at SRCDIR/contrib. That'd help give people the right
>  Tom> coding model to follow.
> 
> I don't see that playing nicely with PGXS?

I'm also not on board that my random third-party extension now has to
refer to its own header files as "subdirectory/headerfile.h".  Which
will mess up existing extensions that have header files in their tree.

Or at least I'm not totally sure what the exact proposal and real-world
implications are, with regard to existing extensions with one or more
header files.

By all means, let's make it easier for large or small extensions to
manage their header files with PGXS.  But let's separate what PGXS can
and should do from what the extension's own file layout is.

But I think there are some fundamentally incompatible goals here with
regard to how the final -I options are supposed to look.

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

Re: pg_upgrade from 9.4 to 10.4

Bruce Momjian  writes:
> On Tue, Jul 31, 2018 at 03:26:47PM -0400, Tom Lane wrote:
>> This patch evidently broke buildfarm member jacana, although only
>> in the 9.3 branch.  It's been failing with
>> Jul 28 23:22:30 The system cannot find the path specified.

> Well, that's interesting.  I have a test script that upgrades all
> version combinations of Postgres from 9.3 to head and it worked for me. 
> However, I now see that the tests for checking a live server were wrong,
> and 9.3 must have a slightly different test than the others.  Fixes
> applied.  Thanks for finding this so quickly.

After poking around a bit more, I think the more likely explanation is
that 9.3 was still using the SYSTEMQUOTE macro in commands to be popen'd,
and you did not make that adjustment when back-patching into that branch.
Note the adjacent pre-existing invocation of pg_controldata:

snprintf(cmd, sizeof(cmd), SYSTEMQUOTE "\"%s/%s \"%s\"" SYSTEMQUOTE,
 cluster->bindir,
 live_check ? "pg_controldata\"" : "pg_resetxlog\" -n",


regards, tom lane

Re: pg_upgrade from 9.4 to 10.4

On Tue, Jul 31, 2018 at 03:26:47PM -0400, Tom Lane wrote:
> Bruce Momjian  writes:
> > Updated patch applied through 9.3:
> > 
> > https://git.postgresql.org/pg/commitdiff/260fe9f2b02b67de1e5ff29faf123e4220586c43
> 
> This patch evidently broke buildfarm member jacana, although only
> in the 9.3 branch.  It's been failing with
> 
> Jul 28 23:22:29 Performing Consistency Checks
> Jul 28 23:22:29 -
> Jul 28 23:22:29 Checking cluster versions   ok
> Jul 28 23:22:30 The system cannot find the path specified.
> Jul 28 23:22:30 
> Jul 28 23:22:30 The source cluster lacks cluster state information:
> Jul 28 23:22:30 Failure, exiting
> Jul 28 23:22:30 make: *** [check] Error 1

Well, that's interesting.  I have a test script that upgrades all
version combinations of Postgres from 9.3 to head and it worked for me. 
However, I now see that the tests for checking a live server were wrong,
and 9.3 must have a slightly different test than the others.  Fixes
applied.  Thanks for finding this so quickly.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Should contrib modules install .h files?

On 2018-07-31 17:53:19 -0400, Tom Lane wrote:
> Peter Geoghegan  writes:
> > On Tue, Jul 31, 2018 at 1:17 PM, Andres Freund  wrote:
> >> I'm a bit surprised that you decided to push to the 11 branch - the
> >> consensus in this thread seem to have gone the other way by my read?
> >> Given that that's the rule, and pushing non-fixes is the exception, I'm
> >> not particularly ok with just ignoring that?
> 
> > +1
> 
> By my count of people expressing opinions, we had Michael -1, Stephen -1,
> me -0.1 or so, Alvaro +1, Peter -1, presumably +1 from Andrew; and Andres
> made a comment about not waiting, which perhaps Andrew read as a +1 for
> backpatching.  So it's not unreasonable for Andrew to have decided that
> it was about tied.  Nonetheless, it does seem like a feature and we're
> past feature freeze, so the default assumption ought to be "no backpatch"
> IMO.

Yea, I don't think it's an entirely unreasonable to decide to backpatch
based on these votes, but I think if the stated opinions are like you
count, it's pretty reasonable to at least announce that the more
controversial choice is the plan and give a chance to more vigorously
object.

Greetings,

Andres Freund

Re: Should contrib modules install .h files?

Peter Geoghegan  writes:
> On Tue, Jul 31, 2018 at 1:17 PM, Andres Freund  wrote:
>> I'm a bit surprised that you decided to push to the 11 branch - the
>> consensus in this thread seem to have gone the other way by my read?
>> Given that that's the rule, and pushing non-fixes is the exception, I'm
>> not particularly ok with just ignoring that?

> +1

By my count of people expressing opinions, we had Michael -1, Stephen -1,
me -0.1 or so, Alvaro +1, Peter -1, presumably +1 from Andrew; and Andres
made a comment about not waiting, which perhaps Andrew read as a +1 for
backpatching.  So it's not unreasonable for Andrew to have decided that
it was about tied.  Nonetheless, it does seem like a feature and we're
past feature freeze, so the default assumption ought to be "no backpatch"
IMO.

regards, tom lane

Re: Should contrib modules install .h files?

> "Andres" == Andres Freund  writes:

 Andres> I'm a bit surprised that you decided to push to the 11 branch -
 Andres> the consensus in this thread seem to have gone the other way by
 Andres> my read?

There were also quite a few non-objections or "don't care"s to the idea
of pushing it to 11beta, and no real justification for not doing so was
ever given other than "it's a new feature", which I don't really agree
with (it's certainly fixing a defect, even if you don't want to call it
a bug). I actually think an argument could be made for backpatching it
even further (though I don't intend to pursue that).

The reason for making the change in the first place is to support
out-of-tree development (particularly of PLs and their transforms) which
isn't strongly tied to pg's development and release cycle; not pushing
to 11-beta would mean delaying the issue for another year and forcing
yet another cycle of workarounds. (Right now for example I can take
advantage of the fact that hstore.h didn't change between 9.5, 9.6 and
10 except in non-significant whitespace; if I can rely on hstore.h being
actually installed in pg11 onwards, then I can draw a line under the
issue and move on. Otherwise, I have to deal with the fact that hstore.h
_does_ change between 10 and 11 and then wait for 12 to see what that
ends up doing.)

(Obviously I'm biased by the fact that this is an issue that impacts me
personally.)

-- 
Andrew (irc:RhodiumToad)

doc - improve description of default privileges



I have not found a convenient presentation of the default privileges for 
different objects, and how to display them (if possible, not always).


The information is partly provided within the GRANT description, and not 
very explicit: eg it is said that owners have all possible perms, but 
which they are is not said explicitely, although they are implied by the 
different GRANT sysnopsys. Then some objects are given perms for the 
PUBLIC.


The attached patch tries to improve the documentation, in particular with 
an added table to summarizes my findings, so that they are recorded 
somewhere.


--
Fabien.diff --git a/doc/src/sgml/ref/grant.sgml b/doc/src/sgml/ref/grant.sgml
index ff64c7a3ba..5a10a50c60 100644
--- a/doc/src/sgml/ref/grant.sgml
+++ b/doc/src/sgml/ref/grant.sgml
@@ -175,6 +175,8 @@ GRANT role_name [, ...] TO EXECUTE privilege for functions and procedures; and
USAGE privilege for languages and data types
(including domains).
+summarizes the hardcoded
+   default privileges granted to all object's types.
The object owner can, of course, REVOKE
both default and  expressly granted privileges. (For maximum
security, issue the REVOKE in the same transaction that
@@ -205,6 +207,9 @@ GRANT role_name [, ...] TO currval function.
For large objects, this privilege allows the object to be read.
   
+  
+   This privilege is abbreviated r when displayed.
+  
  
 
 
@@ -218,6 +223,9 @@ GRANT role_name [, ...] TO  FROM.
   
+  
+   This privilege is abbreviated a when displayed.
+  
  
 
 
@@ -240,6 +248,9 @@ GRANT role_name [, ...] TO 
+  
+   This privilege is abbreviated w when displayed.
+  
  
 
 
@@ -253,6 +264,9 @@ GRANT role_name [, ...] TO SELECT privilege as well, since it must reference 
table
columns to determine which rows to delete.)
   
+  
+   This privilege is abbreviated d when displayed.
+  
  
 
 
@@ -263,6 +277,9 @@ GRANT role_name [, ...] TO  on
the specified table.
   
+  
+   This privilege is abbreviated D when displayed.
+  
  
 
 
@@ -274,6 +291,9 @@ GRANT role_name [, ...] TO  statement.)
   
+  
+   This privilege is abbreviated x when displayed.
+  
  
 
 
@@ -284,6 +304,9 @@ GRANT role_name [, ...] TO  statement.)
   
+  
+   This privilege is abbreviated t when displayed.
+  
  
 
 
@@ -304,6 +327,9 @@ GRANT role_name [, ...] TO 
+  
+   This privilege is abbreviated C when displayed.
+  
  
 
 
@@ -315,6 +341,9 @@ GRANT role_name [, ...] TO pg_hba.conf).
   
+  
+   This privilege is abbreviated c when displayed.
+  
  
 
 
@@ -325,6 +354,9 @@ GRANT role_name [, ...] TO 
Allows temporary tables to be created while using the specified 
database.
   
+  
+   This privilege is abbreviated T when displayed.
+  
  
 
 
@@ -339,6 +371,9 @@ GRANT role_name [, ...] TO ROUTINE to refer to a 
function,
aggregate function, or procedure regardless of what it is.
   
+  
+   This privilege is abbreviated X when displayed.
+  
  
 
 
@@ -382,6 +417,9 @@ GRANT role_name [, ...] TO 
+  
+   This privilege is abbreviated U when displayed.
+  
  
 
 
@@ -627,6 +665,95 @@ GRANT ALL PRIVILEGES ON kinds TO manuel;
 
 GRANT admins TO joe;
 
+
+  
+   Default hardcoded access privileges per object's type
+   
+
+ 
+  Object's type
+  psql \-command
+  Owner
+  PUBLIC
+ 
+
+
+ 
+  DATABASE
+  \l
+  CTc
+  Tc
+ 
+ 
+  DOMAIN
+  \dD+
+  U
+  U
+ 
+ 
+  FUNCTION or 
PROCEDURE
+  \df+
+  X
+  X
+ 
+ 
+  FOREIGN DATA WRAPPER
+  \dew+
+  U
+  
+ 
+ 
+  FOREIGN SERVER
+  \des+
+  U
+  
+ 
+ 
+  LANGUAGE
+  \dL+
+  U
+  U
+ 
+ 
+  LARGE OBJECT
+  
+  rw
+  
+ 
+ 
+  SCHEMA
+  \dn+
+  UC
+  
+ 
+ 
+  SEQUENCE
+  \dp
+  rwU
+  
+ 
+ 
+  TABLE and relation-like objects
+  \dp
+  arwdDxt
+  
+ 
+ 
+  TABLESPACE
+  \db+
+  
+  
+ 
+ 
+  TYPE
+  \dT+
+  U
+  U
+ 
+
+   
+  
+

commitfest 2018-07

2018-07-31 Thread Andrew Dunstan




Well, here we are at the end of July.


here's the current state of Commitfest 2018-07:


*Status summary: *Needs review 
: 83. Waiting on Author 
: 31. Ready for 
Committer : 17. 
Committed : 54. Moved to 
next CF : 8. Rejected 
: 5. Returned with 
Feedback : 2. Total 
: 200.



At the start of the commitfest I endeavoured privately to encourage some 
movement particularly on the oldest items, which a number of people 
suggested we should try to focus on. Four of those items have been 
successfully disposed of.


There has also been some movement on these old items (arbitrarily 
defined as those that are 5 or more CFs old):



https://commitfest.postgresql.org/18/528/ "Fix the optimization to skip 
WAL-logging on table created in same transaction"


we have a proposed fix from Heikki / Kyatoro. That seems to be the 
consensus solution, although it's somewhat invasive. I hope we keep 
moving on this.



https://commitfest.postgresql.org/18/713/ "Correct space parsing in 
to_timestamp()"


There seems to be close to a consensus on the solution, Alexander has 
asked for some more documentation.



https://commitfest.postgresql.org/18/944/ "Logical decoding of two-phase 
transactions"


There are significant ongoing discussions on a way forward


https://commitfest.postgresql.org/18/1004/ "SERIALIZABLE with parallel 
query"


Masahiko Sawada has posted a review, and we're waiting on the author.


https://commitfest.postgresql.org/18/1141/ "Full merge join on 
comparison clause"


Ashutosh posted a review, the author has responded with updated patches, 
further review is needed



https://commitfest.postgresql.org/18/1160/ "Improve geometric types"

Ongoing discussion, possibly close to a commit.


https://commitfest.postgresql.org/18/1264/ "Exclude partition constaint 
checking in query execution plan for partitioned table queries"


a.k.a. "Secondary index access optimizations "

David Rowley posted a very substantial review, including a patch with an 
alternative approach. The author has not responded



No real progress has been made on the following old items:


https://commitfest.postgresql.org/18/669/ "pgbench - allow to store 
query results into variables"


This items is marked RFC and Stephen has claimed it as committer. It's 
been around for 9 CFs now.



https://commitfest.postgresql.org/18/931/ "Protect syscache from 
bloating with negative cache entries"


I suggested at the start of the CF that this item should possibly be 
marked RWF, but that wasn't universally agreed to. However, nothing 
further has happened.



https://commitfest.postgresql.org/18/1001/ "Convert join OR clauses into 
UNION queries"


Silence since April.


https://commitfest.postgresql.org/18/1085/ "XML XPath default namespace 
support"


Has been RFC since January but nobody has claimed it.


https://commitfest.postgresql.org/18/1124/ "Incremental sort"

Nothing since the patch was rebased at the end of May


https://commitfest.postgresql.org/18/1138/ "Improve compactify_tuples 
and PageRepairFragmentation"


Not clear that it will make any significant improvement. Nothing 
substantial since April.



https://commitfest.postgresql.org/18/1166/ "Fix LWLock degradation on NUMA"

this item has been marked RFC since September, but nobody has claimed it.


https://commitfest.postgresql.org/18/1252/ "Foreign Key Arrays"

In May author stated he was having trouble rebasing the patch and 
devising tests, and asked for help, but there has been no response




It appears that a combination of concentrating on Release 11 issues and 
northern summer vacation time has meant we havem't accomplished very 
much with this CF, at least in terms of getting actual commits and 
clearing up[ the backlog. I'm not sure that I would recommend repeating 
the experiment.


Early on I published some stats of patch sizes, which I obtained by 
doing some pretty ugly screen scraping. All the data underlying the CF 
should be public, and it would possibly make sense to provide some API 
for querying it directly.


Sometimes thde app sees an attachment in the email thread and marks it 
as the latest patch, but in a number of cases it isn't - it's either not 
a patch at all or occasionally something to be applied after the main 
patch is applied. I realize it has to use heuristics but it might be 
well to allow some sort of manual override of the "latest patch".


cheers

andrew

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

Re: [HACKERS] Cached plans and statement generalization

2018-07-31 Thread Konstantin Knizhnik

Hi Yamaji,

On 31.07.2018 12:12, Yamaji, Ryo wrote:

-Original Message-
From: Konstantin Knizhnik [mailto:k.knizh...@postgrespro.ru]
Sent: Friday, January 12, 2018 9:53 PM
To: Thomas Munro ; Stephen Frost

Cc: Michael Paquier ; PostgreSQL mailing
lists ; Tsunakawa, Takayuki/綱川 貴之

Subject: Re: [HACKERS] Cached plans and statement generalization

Thank you very much for reporting the problem.
Rebased version of the patch is attached.

Hi Konstantin.

I think that this patch excel very much. Because the customer of our
company has the demand that migrates from other DB to PostgreSQL, and
the problem to have to modify the application program to do prepare in
that case occurs. It is possible to solve it by the problem's using this
patch. I want to be helping this patch to be committed. Will you participate
in the following CF?

This patch will be included in next release of PgPro EE.
Concerning next commit fest - I am not sure.
At previous commitfest it was returned with feedback that it "has 
received no review or comments since last May".

May be your review will help to change this situation.

To review this patch, I verified it. The verified environment is
PostgreSQL 11beta2. It is necessary to add "executor/spi.h" and "jit/jit.h"
to postgres.c of the patch by the updating of PostgreSQL. Please rebase.

1. I confirmed the influence on the performance by having applied this patch.
The result showed the tendency similar to Konstantin.
-s:100  -c:8-t: 1   read-only
simple: 20251 TPS
prepare:29502 TPS
simple(autoprepare):28001 TPS

2. I confirmed the influence on the memory utilization by the length of query 
that did
autoprepare. Short queries have 1 constant. Long queries have 100 constants.
This result was shown that preparing long query used the memory more.
before prepare：plan cache context: 1032 used
prepare 10 short query statement：plan cache context: 15664 used
prepare 10 long query statement：plan cache context: 558032 used

In this patch, the maximum number of query that can do prepare can be set to 
autoprepare_limit.
However, is it good in this? I think that I can assume the scene in the 
following.
- Application side user: To elicit the performance, they want to specify the 
number of prepared
query.
- Operation side user: To prevent the memory from overflowing, they want to set 
the maximum value
of the memory utilization.
Therefore, I propose to add the parameter to specify the maximum memory 
utilization.

I agree it may be more useful to limit amount of memory used by prepare 
queries, rather than number of prepared statements.
But it is just more difficult to calculate and maintain (I am not sure 
that just looking at CacheMemoryContext is enough for it).
Also, if working set of queries (frequently repeated queries) doesn't 
fir in memory, then autoprepare will be almost useless (because with 
high probability
prepared query will be thrown away from the cache before it can be 
reused). So limiting queries from "application side" seems to be more 
practical.

3. I confirmed the transition of the amount of the memory when it tried to 
prepare query
of the number that exceeded the value specified for autoprepare_limit.
[autoprepare_limit=1 and execute 10 different queries]
plan cache context: 1032 used
plan cache context: 39832 used
plan cache context: 78552 used
plan cache context: 117272 used
plan cache context: 155952 used
plan cache context: 194632 used
plan cache context: 233312 used
plan cache context: 272032 used
plan cache context: 310712 used
plan cache context: 349392 used
plan cache context: 388072 used

I feel the doubt in an increase of the memory utilization when I execute a lot 
of
query though cached query is one (autoprepare_limit=1).
This behavior is correct?

I will check it.

Re: Online enabling of checksums

Andres Freund  writes:
> On 2018-07-31 23:20:27 +0200, Daniel Gustafsson wrote:
>> Not really arguing for or against, but just to understand the reasoning 
>> before
>> starting hacking.  Why do we feel that a restart (intended for safety here) 
>> in
>> this case is a burden on a use-once process?  Is it from a usability or
>> technical point of view?  Just want to make sure we are on the same page 
>> before
>> digging in to not hack on this patch in a direction which isn’t what is
>> requested.

> Having, at some arbitrary seeming point in the middle of enabling
> checksums to restart the server makes it harder to use and to schedule.
> The restart is only needed to fix a relatively small issue, and doesn't
> save that much code.

Without taking a position on the merits ... I don't see how you can
claim "it doesn't save that much code" when we don't have a patch to
compare to that doesn't require the restart.  Maybe it will turn out
not to be much code, but we don't know that now.

regards, tom lane

Re: Alter index rename concurrently to

Peter Eisentraut  writes:
> On 27/07/2018 16:16, Robert Haas wrote:
>> I also suspect that an appropriate fix might be to ensure that
>> AcceptInvalidationMessages() is run at least once at the beginning of
>> parse analysis.

> Why don't we just do that?

Don't we do that already?  Certainly it should get run in advance of
any relation name lookup.  There is one at transaction start also,
if memory serves.

regards, tom lane

Re: Online enabling of checksums

Hi,

On 2018-07-31 23:20:27 +0200, Daniel Gustafsson wrote:
> > On 26 Jul 2018, at 19:35, Andres Freund  wrote:
> > On July 26, 2018 10:03:39 AM PDT, Robert Haas  > > wrote:
> 
> >> Why can't we do better?
> > 
> > I don't think it's that hard to do better. IIRC I even outlined something 
> > before the freeze. If not, o certainly can (sketch: use procsignal based 
> > acknowledgment protocol, using a 64 bit integer. Useful for plenty other 
> > things).
> 
> Not really arguing for or against, but just to understand the reasoning before
> starting hacking.  Why do we feel that a restart (intended for safety here) in
> this case is a burden on a use-once process?  Is it from a usability or
> technical point of view?  Just want to make sure we are on the same page 
> before
> digging in to not hack on this patch in a direction which isn’t what is
> requested.

Having, at some arbitrary seeming point in the middle of enabling
checksums to restart the server makes it harder to use and to schedule.
The restart is only needed to fix a relatively small issue, and doesn't
save that much code.

Greetings,

Andres Freund

Re: Bizarre behavior in libpq's searching of ~/.pgpass

I wrote:
> But now that I look at it, it seems like the code in connectOptions2
> has also Gotten It Wrong.  Shouldn't the replacement of "unspecified"
> cases by DEFAULT_PGSOCKET_DIR/DefaultHost also happen on an entry-by-
> entry basis, so that "host=foo," would behave as though the empty
> entry were "localhost"?

Here's an updated patch that fixes that aspect too.  Although this
might seem independent, it's not really: this version of the patch
eliminates the corner case where we have neither a host or hostaddr
to search for.  The check for empty hostname in passwordFromFile is
thereby dead code, though it seemed better style to leave it in.

(Basically, the point here is to guarantee that passwordFromFile has
the same idea of what host/port we are going to connect to as the
actual connection code does.  That was not true before, either for
the host or the port :-()

regards, tom lane

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index d67212b..0b1ffb7 100644
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
*** postgresql://%2Fvar%2Flib%2Fpostgresql/d
*** 938,945 

 If a password file is used, you can have different passwords for
 different hosts. All the other connection options are the same for every
!host, it is not possible to e.g. specify a different username for
!different hosts.

--- 938,945 

 If a password file is used, you can have different passwords for
 different hosts. All the other connection options are the same for every
!host in the list; it is not possible to e.g. specify different
!usernames for different hosts.

*** postgresql://%2Fvar%2Flib%2Fpostgresql/d
*** 961,967 
  name of the directory in which the socket file is stored.  If
  multiple host names are specified, each will be tried in turn in
  the order given.  The default behavior when host is
! not specified is to connect to a Unix-domain
  socketUnix domain socket in
  /tmp (or whatever socket directory was specified
  when PostgreSQL was built). On machines without
--- 961,967 
  name of the directory in which the socket file is stored.  If
  multiple host names are specified, each will be tried in turn in
  the order given.  The default behavior when host is
! not specified, or is empty, is to connect to a Unix-domain
  socketUnix domain socket in
  /tmp (or whatever socket directory was specified
  when PostgreSQL was built). On machines without
*** postgresql://%2Fvar%2Flib%2Fpostgresql/d
*** 969,975 

  A comma-separated list of host names is also accepted, in which case
! each host name in the list is tried in order. See
   for details.

--- 969,976 

  A comma-separated list of host names is also accepted, in which case
! each host name in the list is tried in order; an empty item in the
! list selects the default behavior as explained above. See
   for details.

*** postgresql://%2Fvar%2Flib%2Fpostgresql/d
*** 1020,1033 

  Note that authentication is likely to fail if host
  is not the name of the server at network address hostaddr.
! Also, note that host rather than hostaddr
  is used to identify the connection in a password file (see
  ).

  A comma-separated list of hostaddr values is also
! accepted, in which case each host in the list is tried in order. See
   for details.

--- 1021,1037 

  Note that authentication is likely to fail if host
  is not the name of the server at network address hostaddr.
! Also, when both host and hostaddr
! are specified, host
  is used to identify the connection in a password file (see
  ).

  A comma-separated list of hostaddr values is also
! accepted, in which case each host in the list is tried in order.
! An empty item in the list causes the corresponding host name to be
! used, or the default host name if that is empty as well. See
   for details.

*** postgresql://%2Fvar%2Flib%2Fpostgresql/d
*** 1047,1055 
  name extension for Unix-domain
  connections.port
  If multiple hosts were given in the host or
! hostaddr parameters, this parameter may specify a list
! of ports of equal length, or it may specify a single port number to
! be used for all hosts.

--- 1051,1062 
  name extension for Unix-domain
  connections.port
  If

Re: Online enabling of checksums

2018-07-31 Thread Daniel Gustafsson

> On 31 Jul 2018, at 21:52, Joshua D. Drake  wrote:
> 
> On 07/31/2018 12:45 PM, Bruce Momjian wrote:

>>> Thanks for reviewing, I’ve updated the patch with the above mentioned 
>>> incorrect
>>> linkends as well as fixed the comments you made in a previous review.
>>> 
>>> The CF-builder-bot is red, but it’s because it’s trying to apply the already
>>> committed patch which is in the attached datallowconn thread.
>> I think checksumhelper_cost_delay should be checksum_helper_cost_delay.
>> ^
>> Is "helper" the right word?

IIRC, “helper” was chosen to signal that it’s a single process where “worker”
may be thought of as a process of which there can be many.

> Based on other terminology within the postgresql.conf should it be 
> "checksum_worker_cost_delay”?

Yes, I think it makes sense to rename it “worker” to align better with the
postgres nomenclature. Will fix.

cheers ./daniel

Re: Alter index rename concurrently to

On 27/07/2018 16:16, Robert Haas wrote:
> With respect to this particular patch, I don't know whether there are
> any hazards of the second type.  What I'd check, if it were me, is
> what structures in the index's relcache entry are going to get rebuilt
> when the index name changes.  If any of those look like things that
> something that somebody could hold a pointer to during the course of
> query execution, or more generally be relying on not to change during
> the course of query execution, then you've got a problem.

I have investigated this, and I think it's safe.  Relcache reloads for
open indexes are already handled specially in RelationReloadIndexInfo().
 The only pointers that get replaced there are rd_amcache and
rd_options.  I have checked where those are used, and it looks like they
are not used across possible relcache reloads.  The code structure in
those two cases make this pretty unlikely even in the future.  Also,
violations would probably be caught by CLOBBER_CACHE_ALWAYS.

> As to the
> first category, I suspect it's possible to construct cases where the
> fact that the index's name hasn't change fails to get noticed for an
> alarmingly long period of time after the change has happened.  I also
> suspect that an appropriate fix might be to ensure that
> AcceptInvalidationMessages() is run at least once at the beginning of
> parse analysis.

Why don't we just do that?  I have run some performance tests and there
was no impact (when no invalidation events are generated).  The code
path in the case where there are no events queued up looks pretty
harmless, certainly compared to all the other stuff that goes on per
command.

Then again, I don't know why you consider this such a big problem in the
first place.  If a concurrent transaction doesn't get the new index name
until the next transaction, what's the problem?

Then again, I don't know why we don't just call
AcceptInvalidationMessages() before each command or at least before each
top-level command?  That would make way too much sense.

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

Re: Online enabling of checksums

On Tue, Jul 31, 2018 at 12:52:40PM -0700, Joshua Drake wrote:
> On 07/31/2018 12:45 PM, Bruce Momjian wrote:
> >
> >>Hi!,
> >>
> >>Thanks for reviewing, I’ve updated the patch with the above mentioned 
> >>incorrect
> >>linkends as well as fixed the comments you made in a previous review.
> >>
> >>The CF-builder-bot is red, but it’s because it’s trying to apply the already
> >>committed patch which is in the attached datallowconn thread.
> >I think checksumhelper_cost_delay should be checksum_helper_cost_delay.
> > ^
> >
> >Is "helper" the right word?
> 
> Based on other terminology within the postgresql.conf should it be
> "checksum_worker_cost_delay"?

+1> 

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Expression errors with "FOR UPDATE" and postgres_fdw with partition wise join enabled.

On 2018-Jul-31, Etsuro Fujita wrote:

> (2018/07/31 4:06), Andres Freund wrote:
> > On 2018-07-20 08:38:09 -0400, Robert Haas wrote:
> > > I'm going to study this some more now, but I really think this is
> > > going in the wrong direction.
> > 
> > We're going to have to get somewhere on this topic soon. This thread has
> > been open for nearly half a year, and we're late in the beta phase now.
> > What can we do to get to an agreement soon?
> 
> I'd like to propose an updated patch as proposed in [1].

But there is no patch there, and neither there is agreement from the
other party that the approach described there is sound.

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

Re: Should contrib modules install .h files?

2018-07-31 Thread Peter Geoghegan

On Tue, Jul 31, 2018 at 1:17 PM, Andres Freund  wrote:
> I'm a bit surprised that you decided to push to the 11 branch - the
> consensus in this thread seem to have gone the other way by my read?
> Given that that's the rule, and pushing non-fixes is the exception, I'm
> not particularly ok with just ignoring that?

+1

-- 
Peter Geoghegan

Re: GiST VACUUM

2018-07-31 Thread Andrey Borodin

Hi! Thanks for looking into the patch!

> 30 июля 2018 г., в 18:39, Heikki Linnakangas  написал(а):
> 
> On 29/07/18 14:47, Andrey Borodin wrote:
>> Fixed both problems. PFA v14.
> 
> Thanks, took a really quick look at this.
> 
> The text being added to README is outdated for these latest changes.
Fixed.
> 
>> In second step I still use paloc's memory, but only to store two
>> bitmaps: bitmap of internal pages and bitmap of empty leafs. Second
>> physical scan only reads internal pages. I can omit that bitmap, if
>> I'll scan everything. Also, I can replace emptyLeafs bitmap with
>> array\list, but I do not really think it will be big.
> 
> On a typical GiST index, what's the ratio of leaf vs. internal pages? Perhaps 
> an array would indeed be better.
Typical GiST has around 200 tuples per internal page. I've switched to List 
since it's more efficient than bitmap. Is 
> If you have a really large index, the bitmaps can take a fair amount of 
> memory, on top of the memory used for tracking the dead TIDs. I.e. that 
> memory will be in addition to maintenance_work_mem. That's not nice, but I 
> think it's OK in practice, and not worth spending too much effort to 
> eliminate. For a 1 TB index with default 8k block size, the two bitmaps will 
> take 32 MB of memory in total. If you're dealing with a database of that 
> size, you ought to have some memory to spare. But if an array would use less 
> memory, that'd be better.

> 
> If you go with bitmaps, please use the existing Bitmapset instead of rolling 
> your own. Saves some code, and it provides more optimized routines for 
> iterating through all the set bits, too (bms_next_member()). Another 
> possibility would be to use Tidbitmap, in the "lossy" mode, i.e. add the 
> pages with tbm_add_page(). That might save some memory, compared to 
> Bitmapset, if the bitmap is very sparse. Not sure how it compares with a 
> plain array.
Yeah, I've stopped reinventing that bicycle. But I have to note that default 
growth strategy of Bitmap is not good: we will be repallocing byte by byte.

> 
> A straightforward little optimization would be to skip scanning the internal 
> pages, when the first scan didn't find any empty pages. And stop the scan of 
> the internal pages as soon as all the empty pages have been recycled.
Done.

PFA v15.

Best regards, Andrey Borodin.


0002-Delete-pages-during-GiST-VACUUM-v15.patch
Description: Binary data


0001-Physical-GiST-scan-in-VACUUM-v15.patch
Description: Binary data

Re: Online enabling of checksums

2018-07-31 Thread Joshua D. Drake


On 07/31/2018 12:45 PM, Bruce Momjian wrote:



Hi!,

Thanks for reviewing, I’ve updated the patch with the above mentioned incorrect
linkends as well as fixed the comments you made in a previous review.

The CF-builder-bot is red, but it’s because it’s trying to apply the already
committed patch which is in the attached datallowconn thread.

I think checksumhelper_cost_delay should be checksum_helper_cost_delay.
 ^

Is "helper" the right word?


Based on other terminology within the postgresql.conf should it be 
"checksum_worker_cost_delay"?


JD





--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc
***  A fault and talent of mine is to tell it exactly how it is.  ***
PostgreSQL centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://postgresconf.org
* Unless otherwise stated, opinions are my own.   *

Re: Online enabling of checksums

On Wed, Jul 25, 2018 at 11:35:31AM +0200, Daniel Gustafsson wrote:
> > On 24 Jul 2018, at 11:05, Sergei Kornilov  wrote:
> > 
> > The following review has been posted through the commitfest application:
> > make installcheck-world:  tested, failed
> > Implements feature:   not tested
> > Spec compliant:   not tested
> > Documentation:tested, failed
> > 
> > Hello
> > As i wrote few weeks ago i can not build documentation due errors:
> >> postgres.sgml:19625: element xref: validity error : IDREF attribute 
> >> linkend references an unknown ID "runtime-checksumhelper-cost-delay"
> >> postgres.sgml:19626: element xref: validity error : IDREF attribute 
> >> linkend references an unknown ID "runtime-checksumhelper-cost-limit"
> > 
> > After remove such xref for test purposes patch pass check-world.
> 
> Hi!,
> 
> Thanks for reviewing, I’ve updated the patch with the above mentioned 
> incorrect
> linkends as well as fixed the comments you made in a previous review.
> 
> The CF-builder-bot is red, but it’s because it’s trying to apply the already
> committed patch which is in the attached datallowconn thread.

I think checksumhelper_cost_delay should be checksum_helper_cost_delay.
^

Is "helper" the right word?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Usability fail with psql's \dp command

2018-07-31 Thread Isaac Morland

On 31 July 2018 at 15:02, Fabien COELHO  wrote:
[]

> Indeed, all \d* which display perms have the empty/default confusion:
>
>   \dp \ddp \des \dew \l \dn \db \df \dT \dD \dL
>
> I fixed them all to display the default acl in the patch I just sent.
>
> I also noticed that although large objects have permissions, they are not
> printed by any backslash commands.
>
> Also using \df to display permissions is inconvenient because \df+ is
required, which also shows function source code which usually overwhelms
the rest of the display. Any chance we can remove the source code column
from \df now that we have \sf? I usually avoid looking at function
permissions and select directly from pg_proc if I absolutely must know.

Re: pg_upgrade from 9.4 to 10.4

Bruce Momjian  writes:
> Updated patch applied through 9.3:
>   
> https://git.postgresql.org/pg/commitdiff/260fe9f2b02b67de1e5ff29faf123e4220586c43

This patch evidently broke buildfarm member jacana, although only
in the 9.3 branch.  It's been failing with

Jul 28 23:22:29 Performing Consistency Checks
Jul 28 23:22:29 -
Jul 28 23:22:29 Checking cluster versions   ok
Jul 28 23:22:30 The system cannot find the path specified.
Jul 28 23:22:30 
Jul 28 23:22:30 The source cluster lacks cluster state information:
Jul 28 23:22:30 Failure, exiting
Jul 28 23:22:30 make: *** [check] Error 1

regards, tom lane

Re: [HACKERS] Restricting maximum keep segments by repslots

On 2018-07-31 15:21:27 -0400, Stephen Frost wrote:
> Greetings,
> 
> * Andres Freund (and...@anarazel.de) wrote:
> > On 2018-07-31 15:11:52 -0400, Bruce Momjian wrote:
> > > On Tue, Jun 26, 2018 at 04:26:59PM +0900, Kyotaro HORIGUCHI wrote:
> > > > Hello. This is the reabased version of slot-limit feature.
> > > > 
> > > > This patch limits maximum WAL segments to be kept by replication
> > > > slots. Replication slot is useful to avoid desync with replicas
> > > > after temporary disconnection but it is dangerous when some of
> > > > replicas are lost. The WAL space can be exhausted and server can
> > > > PANIC in the worst case. This can prevent the worst case having a
> > > > benefit from replication slots using a new GUC variable
> > > > max_slot_wal_keep_size.
> > > 
> > > Have you considered just using a boolean to control if max_wal_size
> > > honors WAL preserved by replication slots, rather than creating the new
> > > GUC max_slot_wal_keep_size?
> > 
> > That seems like a bad idea. max_wal_size influences checkpoint
> > scheduling - there's no good reason to conflate that with retention?
> 
> I agree that we shouldn't conflate checkpointing and retention.  What I
> wonder about though is what value will wal_keep_segments have once this
> new GUC exists..?  I wonder if we could deprecate it...

Don't think that's a good idea. It's entirely conceivable to have a
wal_keep_segments much lower than max_slot_wal_keep_size.  For some
throwaway things it can be annoying to have to slots, and if you remove
wal_keep_segments there's no alternative.

Greetings,

Andres Freund

Re: [HACKERS] Restricting maximum keep segments by repslots

2018-07-31 Thread Stephen Frost

Greetings,

* Andres Freund (and...@anarazel.de) wrote:
> On 2018-07-31 15:11:52 -0400, Bruce Momjian wrote:
> > On Tue, Jun 26, 2018 at 04:26:59PM +0900, Kyotaro HORIGUCHI wrote:
> > > Hello. This is the reabased version of slot-limit feature.
> > > 
> > > This patch limits maximum WAL segments to be kept by replication
> > > slots. Replication slot is useful to avoid desync with replicas
> > > after temporary disconnection but it is dangerous when some of
> > > replicas are lost. The WAL space can be exhausted and server can
> > > PANIC in the worst case. This can prevent the worst case having a
> > > benefit from replication slots using a new GUC variable
> > > max_slot_wal_keep_size.
> > 
> > Have you considered just using a boolean to control if max_wal_size
> > honors WAL preserved by replication slots, rather than creating the new
> > GUC max_slot_wal_keep_size?
> 
> That seems like a bad idea. max_wal_size influences checkpoint
> scheduling - there's no good reason to conflate that with retention?

I agree that we shouldn't conflate checkpointing and retention.  What I
wonder about though is what value will wal_keep_segments have once this
new GUC exists..?  I wonder if we could deprecate it...  I wish we had
implemented repliation slots from the start with wal_keep_segments
capping the max WAL retained but that ship has sailed and changing it
now would break existing configurations.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Should contrib modules install .h files?

> "Andrew" == Andrew Gierth  writes:

Final patch just to let cfbot test the windows code for me.

A review of contrib/ suggested that cube, hstore, isn, ltree and seg
were the only modules that had useful headers to install, so do those.

-- 
Andrew (irc:RhodiumToad)

>From df163230b92b79468c241e9480e7fb85b7614440 Mon Sep 17 00:00:00 2001
From: Andrew Gierth 
Date: Tue, 31 Jul 2018 19:58:39 +0100
Subject: [PATCH] Provide for contrib and pgxs modules to install include
 files.

This allows out-of-tree PLs and similar code to get access to
definitions needed to work with extension data types.

The following existing modules now install headers: contrib/cube,
contrib/hstore, contrib/isn, contrib/ltree, contrib/seg.

Discussion: https://postgr.es/m/87y3euomjh.fsf%40news-spur.riddles.org.uk
---
 contrib/cube/Makefile |  2 ++
 contrib/hstore/Makefile   |  2 ++
 contrib/isn/Makefile  |  3 +++
 contrib/ltree/Makefile|  2 ++
 contrib/seg/Makefile  |  2 ++
 doc/src/sgml/extend.sgml  | 28 --
 src/makefiles/pgxs.mk | 50 +++
 src/tools/msvc/Install.pm | 43 
 8 files changed, 130 insertions(+), 2 deletions(-)

diff --git a/contrib/cube/Makefile b/contrib/cube/Makefile
index accb7d28a3..a679ac626e 100644
--- a/contrib/cube/Makefile
+++ b/contrib/cube/Makefile
@@ -9,6 +9,8 @@ DATA = cube--1.2.sql cube--1.2--1.3.sql cube--1.3--1.4.sql \
 	cube--unpackaged--1.0.sql
 PGFILEDESC = "cube - multidimensional cube data type"
 
+HEADERS = cubedata.h
+
 REGRESS = cube
 
 EXTRA_CLEAN = y.tab.c y.tab.h
diff --git a/contrib/hstore/Makefile b/contrib/hstore/Makefile
index ab7fef3979..46d26f8052 100644
--- a/contrib/hstore/Makefile
+++ b/contrib/hstore/Makefile
@@ -11,6 +11,8 @@ DATA = hstore--1.4.sql hstore--1.4--1.5.sql \
 	hstore--unpackaged--1.0.sql
 PGFILEDESC = "hstore - key/value pair data type"
 
+HEADERS = hstore.h
+
 REGRESS = hstore
 
 ifdef USE_PGXS
diff --git a/contrib/isn/Makefile b/contrib/isn/Makefile
index ab6b175f9a..c3600dac30 100644
--- a/contrib/isn/Makefile
+++ b/contrib/isn/Makefile
@@ -7,6 +7,9 @@ DATA = isn--1.1.sql isn--1.1--1.2.sql \
 	isn--1.0--1.1.sql isn--unpackaged--1.0.sql
 PGFILEDESC = "isn - data types for international product numbering standards"
 
+# the other .h files are data tables, we don't install those
+HEADERS_isn = isn.h
+
 REGRESS = isn
 
 ifdef USE_PGXS
diff --git a/contrib/ltree/Makefile b/contrib/ltree/Makefile
index c101603e6c..416c8da312 100644
--- a/contrib/ltree/Makefile
+++ b/contrib/ltree/Makefile
@@ -9,6 +9,8 @@ EXTENSION = ltree
 DATA = ltree--1.1.sql ltree--1.0--1.1.sql ltree--unpackaged--1.0.sql
 PGFILEDESC = "ltree - hierarchical label data type"
 
+HEADERS = ltree.h
+
 REGRESS = ltree
 
 ifdef USE_PGXS
diff --git a/contrib/seg/Makefile b/contrib/seg/Makefile
index 41270f84f6..62b658e724 100644
--- a/contrib/seg/Makefile
+++ b/contrib/seg/Makefile
@@ -8,6 +8,8 @@ DATA = seg--1.1.sql seg--1.1--1.2.sql seg--1.2--1.3.sql \
 	seg--1.0--1.1.sql seg--unpackaged--1.0.sql
 PGFILEDESC = "seg - line segment data type"
 
+HEADERS = segdata.h
+
 REGRESS = seg
 
 EXTRA_CLEAN = y.tab.c y.tab.h
diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml
index 348ae71423..a3cb064131 100644
--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -1100,13 +1100,15 @@ include $(PGXS)
 and include the global PGXS makefile.
 Here is an example that builds an extension module named
 isbn_issn, consisting of a shared library containing
-some C code, an extension control file, a SQL script, and a documentation
-text file:
+some C code, an extension control file, a SQL script, an include file
+(only needed if other modules might need to access the extension functions
+without going via SQL), and a documentation text file:
 
 MODULES = isbn_issn
 EXTENSION = isbn_issn
 DATA = isbn_issn--1.0.sql
 DOCS = README.isbn_issn
+HEADERS_isbn_issn = isbn_issn.h
 
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
@@ -1221,6 +1223,28 @@ include $(PGXS)
  
 
  
+  HEADERS
+  
+   
+files to install under
+prefix/include/server/$MODULEDIR/$MODULE_big
+   
+  
+ 
+
+ 
+  HEADERS_$MODULE
+  
+   
+files to install under
+prefix/include/server/$MODULEDIR/$MODULE,
+where $MODULE must be a module name used
+in MODULES or MODULE_big
+   
+  
+ 
+
+ 
   SCRIPTS
   

diff --git a/src/makefiles/pgxs.mk b/src/makefiles/pgxs.mk
index bfae02f90c..158581b3f5 100644
--- a/src/makefiles/pgxs.mk
+++ b/src/makefiles/pgxs.mk
@@ -38,6 +38,10 @@
 #   SCRIPTS -- script files (not binaries) to install into $PREFIX/bin
 #   SCRIPTS_built -- script files (not binaries) to install into $PREFIX/bin,
 # which need to be built first
+#   HEADERS -- files to install into $(includedir_server)/$MODULEDIR/$MODULE_big
+#

Re: [HACKERS] Restricting maximum keep segments by repslots

On 2018-07-31 15:11:52 -0400, Bruce Momjian wrote:
> On Tue, Jun 26, 2018 at 04:26:59PM +0900, Kyotaro HORIGUCHI wrote:
> > Hello. This is the reabased version of slot-limit feature.
> > 
> > This patch limits maximum WAL segments to be kept by replication
> > slots. Replication slot is useful to avoid desync with replicas
> > after temporary disconnection but it is dangerous when some of
> > replicas are lost. The WAL space can be exhausted and server can
> > PANIC in the worst case. This can prevent the worst case having a
> > benefit from replication slots using a new GUC variable
> > max_slot_wal_keep_size.
> 
> Have you considered just using a boolean to control if max_wal_size
> honors WAL preserved by replication slots, rather than creating the new
> GUC max_slot_wal_keep_size?

That seems like a bad idea. max_wal_size influences checkpoint
scheduling - there's no good reason to conflate that with retention?

Greetings,

Andres Freund

Re: Doc patch: add RECURSIVE to bookindex





Why referencing only create_view, but not delete, insert, update, select
or select_into where RECURSIVE is also used?

ISTM that at least the select page should be referenced, I'm less sure of
the others because there it appears only in the synopsys.


Looking at other occurrences of , it seems
they're used quite sparingly, limiting the references to only
the pages that have the most relevant explanation for the term,
as opposed to trying to be exhaustive.

For instance, select.sgml has only three , and
the entries for WHERE or JOIN don't even refer to it.
ISTM that it's on purpose, to keep the index lean, and it works.


Ok.

Patch applies cleanly, doc build ok, works for me.

--
Fabien.

Re: [HACKERS] Restricting maximum keep segments by repslots

On Tue, Jun 26, 2018 at 04:26:59PM +0900, Kyotaro HORIGUCHI wrote:
> Hello. This is the reabased version of slot-limit feature.
> 
> This patch limits maximum WAL segments to be kept by replication
> slots. Replication slot is useful to avoid desync with replicas
> after temporary disconnection but it is dangerous when some of
> replicas are lost. The WAL space can be exhausted and server can
> PANIC in the worst case. This can prevent the worst case having a
> benefit from replication slots using a new GUC variable
> max_slot_wal_keep_size.

Have you considered just using a boolean to control if max_wal_size
honors WAL preserved by replication slots, rather than creating the new
GUC max_slot_wal_keep_size?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Usability fail with psql's \dp command




Hello Pavel,


I noticed today that \dp does not distinguish empty acl fields
(meaning nobody has any privileges) from null acl fields
(which mean default privileges, typically not empty).


This confusing behavior exists not only for \dp command.
Consider schemas and \dn+ command:


Indeed, all \d* which display perms have the empty/default confusion:

  \dp \ddp \des \dew \l \dn \db \df \dT \dD \dL

I fixed them all to display the default acl in the patch I just sent.

I also noticed that although large objects have permissions, they are not 
printed by any backslash commands.


--
Fabien.

Re: make installcheck-world in a clean environment

Alexander Lakhin  writes:
> 31.07.2018 02:42, Tom Lane wrote:
>> Alexander's USE_INSTALLED_ASSETS patch attempted to fix that, and I now
>> see the point of it, but it still seems pretty hacky and invasive (why
>> should an ecpg-only problem be touching stuff outside ecpg)?  Also,
>> unless I'm still missing something, it doesn't fix the problem with
>> "make clean check": ecpg won't have been built before we try to build the
>> test programs.

> In fact, after fixing ECPG usage with installcheck, I found that "make
> installcheck" then rebuilds libpostgres, libpgport and libpq (for
> installcheck-world). (I mentioned it in this thread before.) Later I
> proposed a more comprehensive patch that allows us to make installcheck
> cleanly (without building something).
> So the problem is not ecpg-only, the ecpg's failure to build is
> prominent part of it, but there are another assets getting built under
> the hood.

Well, there's a lot of moving parts here, and it's not clear to me what
makes the most sense.  We potentially have the following things that
ECPG "make check" generates or references in the build tree, but we might
wish for "make installcheck" to use preinstalled versions of instead:

1. Server executables and other "installed" server support files.
2. PG cluster (datadir and running server).
3. pg_regress and libraries it depends on, such as libpq.
4. ecpg preprocessor.
5. ecpg's exported C header files, needed to compile test programs.
6. ecpg's libraries, used by test programs; also libpq.

Currently, installcheck references an existing running cluster (#2)
and therefore a fortiori #1 as well.  That's fine.  I believe we
reference a local pg_regress and libraries (#3) in both cases,
and that's also fine, because we don't install pg_regress at all.
(Well, PGXS does, but ecpg tests shouldn't rely on that.)
So it boils down to what to do about #4/#5/#6.

I suppose we could define "make installcheck" as referring only
to using an installed server, not any ecpg support, in which
case we don't need any big surgery to the ecpg makefiles.
I'm not sure how consistent a definition that is, but maybe it
makes sense by analogy to pg_regress.

Another point here is that if you did "make check" and then
"make installcheck", it presumably would not rebuild the test
programs, meaning they'd still have been built using the local
resources not the installed ones (or vice versa for the other
order).  So there's also a difference between "build" and
"test execution", which seems relevant, and it's one we don't
really have any make-target nomenclature to distinguish.

Given that you'd expect "make all" to use local copies of the
build resources, perhaps there should be a separate target
named along the lines of "make tests-from-install" that uses
installed ecpg+other resources to compile the test programs.

Anyway, as long as the installcheck target is dependent on "all" not
something else, maybe it's fine as is.  I'm definitely not sure what
would be a consistent design for doing it differently.

regards, tom lane

Re: patch to ensure logical decoding errors early

Hi,

On 2018-07-31 14:51:12 -0400, Dave Cramer wrote:
> This patch does 2 things
> 
> 1) Ensure that when the slot is created
> with pg_create_physical_replication_slot if the output plugin does not
> exist it will error.

*logical, I assume?


> diff --git a/src/backend/replication/logical/logical.c 
> b/src/backend/replication/logical/logical.c
> index 3cd4eef..9f883b9 100644
> --- a/src/backend/replication/logical/logical.c
> +++ b/src/backend/replication/logical/logical.c
> @@ -143,8 +143,7 @@ StartupDecodingContext(List *output_plugin_options,
>* (re-)load output plugins, so we detect a bad (removed) output plugin
>* now.
>*/
> - if (!fast_forward)
> - LoadOutputPlugin(>callbacks, NameStr(slot->data.plugin));
> + LoadOutputPlugin(>callbacks, NameStr(slot->data.plugin));

So this actually was broken by 9c7d06d60680c7f00d931233873dee81fdb311c6
and worked before? Petr, Simon?  Isn't the actual bug here that
CreateInitDecodingContext() passes true for fast_forward?  Dave, could
you confirm this is the case?  If so, this'll end up actually being an
open items entry...


>   /*
>* Now that the slot's xmin has been set, we can announce ourselves as a
> @@ -312,7 +311,7 @@ CreateInitDecodingContext(char *plugin,
>   ReplicationSlotSave();
>  
>   ctx = StartupDecodingContext(NIL, InvalidXLogRecPtr, xmin_horizon,
> -  
> need_full_snapshot, true,
> +  
> need_full_snapshot, true, 
>read_page, 
> prepare_write, do_write,
>
> update_progress);

Huh?


Greetings,

Andres Freund

patch to ensure logical decoding errors early

2018-07-31 Thread Dave Cramer

This patch does 2 things

1) Ensure that when the slot is created
with pg_create_physical_replication_slot if the output plugin does not
exist it will error.

2) Currently when the decoding context is created and the output plugin
does not exist the protocol will respond with CopyDone. This patch will
return an error instead and abort the copy connection.



Dave Cramer


0002-remove-space.patch
Description: Binary data


0001-Ensure-that-pg_create_physical_replication_slot-erro.patch
Description: Binary data

Re: Standby trying "restore_command" before local WAL

2018-07-31 Thread Stephen Frost

Greetings,

* Sergei Kornilov (s...@zsrv.org) wrote:
> > As mentioned by others, it sounds like we could have an option to try
> > contacting the primary before running restore_commnad
> Why about primary?
> If we have restore_command on slave (or during point in time recovery) - we 
> force using XLOG_FROM_ARCHIVE, even if XLOG_FROM_PG_WAL source can provide 
> next WAL. As say xlog.c comment [1]:

Right..

> > * We just successfully read a file in pg_wal. We prefer files in
> > * the archive over ones in pg_wal, so try the next file again
> > * from the archive first.
> 
> We have some actual reason why we prefer restore_command instead of using 
> local wal files first?

Yes, as discussed in the comments mentioned up-thread.

> Partially written WAL? Streaming replication can leave partially written WAL 
> and we can handle this correctly.

Sure, though even in that case there seems to be a reasonable use-case
here for an option to control if restore_command is used to get the next
needed WAL or if the primary should be asked for the WAL first.

There's still a question here, at least from my perspective, as to which
is actually going to be faster to perform recovery based off of.  A good
restore command, which pre-fetches the WAL in parallel and gets it local
and on the same filesystem, meaning that the restore_command only has to
execute essentially a 'mv' and return back to PG for the next WAL file,
is really rather fast, compared to streaming that same data over the
network with a single TCP connection to the primary.  Of course, there's
a lot of variables there and it depends on the network speed between the
various pieces, but I've certainly had cases where a replica catches up
much faster using restore command than streaming from the primary.

Thanks!

Stephen

signature.asc
Description: PGP signature

Re: Documentaion fix.

2018-07-31 Thread Michael Paquier

On Tue, Jul 31, 2018 at 10:59:14AM -0400, Alvaro Herrera wrote:
> How about pasting it like this?
> 
> alvherre=# select * from pg_replication_slots \gx
> ─[ RECORD 1 ]───┬
> slot_name   │ node_a_slot
> plugin  │ 
> slot_type   │ physical
> datoid  │ 
> database│ 
> temporary   │ f
> active  │ f
> active_pid  │ 
> xmin│ 
> catalog_xmin│ 
> restart_lsn │ 
> confirmed_flush_lsn │ 

Each time this catalog is changed, this would become incorrect.  My
suggestion would be to change the query to that and call it a day:
SELECT slot_name, slot_type, active FROM pg_replication_slots;
--
Michael


signature.asc
Description: PGP signature

Re: BUG #15182: Canceling authentication due to timeout aka Denial of Service Attack

2018-07-31 Thread Michael Paquier

On Tue, Jul 31, 2018 at 10:34:57AM -0400, Robert Haas wrote:
> Just as a general statement, I don't think we should, as part of a
> patch for the issue discussed on this thread, make any changes AT ALL
> to who has permission to perform which operations.  We *certainly*
> should not back-patch such changes, but we really also should not make
> them on master unless they are discussed on a separate thread with a
> clear subject line and agreed by a clear consensus.

Well, perhaps spawning one thread for each command would make the most
sense then?  I am feeling a bit of confusion here.  There have been
three cases discussed up to now:
1) TRUNCATE, which results in a patch that has no behavioral changes.
2) VACUUM [FULL], where we are also heading toward a patch that has no
behavioral change per the last arguments exchanged, where we make sure
that permission checks are done before acquiring any locks.
3) REINDEX, where a database or a schema owner is able to take an
exclusive lock on a shared catalog with limited permissions.  That
sucks as this block calls to load_critical_index, but I would be ready
to buy to not back-patch such a change if the consensus reached is to
not skip shared catalogs if a database/schema owner has no ownership on
the shared catalog directly.

> This patch should only be about detecting cases where we lack
> permission earlier, before we try to acquire a lock.

Yeah, the REINDEX case is the only one among the three where the set of
relations worked on changes.  Please note that ownership checks happen
in this case for indexes, tables, database and schemas before taking a
lock on them.  Databases/systems and schemas just check for ownership of
respectively the database and the schema.

I'll be happy to create one thread per patch if that helps.  Thinking
about it this would attract more attention to each individual problem
reported.
--
Michael

signature.asc
Description: PGP signature

Re: New Defects reported by Coverity Scan for PostgreSQL

Hello guys.  Coverity complained about this patch as below.  What, if
anything, should be done about it?  One solution is to mark it as a
false-positive in Coverity, of course.

On 2018-Jul-29, scan-ad...@coverity.com wrote:

> ** CID 1438146:  API usage errors  (SWAPPED_ARGUMENTS)
> 
> 
> 
> *** CID 1438146:  API usage errors  (SWAPPED_ARGUMENTS)
> /srv/coverity/git/pgsql-git/postgresql/src/backend/utils/adt/geo_ops.c: 2647 
> in close_lseg()
> 2641  LSEG   *l1 = PG_GETARG_LSEG_P(0);
> 2642  LSEG   *l2 = PG_GETARG_LSEG_P(1);
> 2643  Point  *result;
> 2644 
> 2645  result = (Point *) palloc(sizeof(Point));
> 2646 
> >>> CID 1438146:  API usage errors  (SWAPPED_ARGUMENTS)
> >>> The positions of arguments in the call to "lseg_closept_lseg" do not 
> >>> match the ordering of the parameters:
> * "l2" is passed to "l1"
> * "l1" is passed to "l2"
> 2647  lseg_closept_lseg(result, l2, l1);
> 2648 
> 2649  PG_RETURN_POINT_P(result);
> 2650 }
> 2651 
> 2652 

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

Re: patch to allow disable of WAL recycling

2018-07-31 Thread Robert Haas

On Mon, Jul 30, 2018 at 4:43 AM, Peter Eisentraut
 wrote:
> On 19/07/2018 05:59, Kyotaro HORIGUCHI wrote:
>> My result is that we cannot disable recycling perfectly just by
>> setting min/max_wal_size.
>
> Maybe the behavior of min_wal_size should be rethought?  Elsewhere in
> this thread, there was also a complaint that max_wal_size isn't actually
> a max.  It seems like there might be some interest in making these
> settings more accurate.
>
> I mean, what is the point of the min_wal_size setting if not controlling
> this very thing?

See the logic in XLOGfileslop().  The number of segments that the
server recycles (by renaming) after a checkpoint is bounded to not
less than min_wal_size and not more than max_wal_size, but the actual
value fluctuates between those two extremes based on the number of
segments the server believes will be required before the next
checkpoint completes.  Logically, min_wal_size = 0 would mean that the
number of recycled segments could be as small as zero.  However, what
is being requested here is to force the number of recycled segments to
never be larger than zero, which is different.

As far as the log in XLOGfileslop() is concerned, that would
correspond to max_wal_size = 0, not min_wal_size = 0.  However, that's
an impractical setting because max_wal_size is also used in other
places, like CalculateCheckpointSegments().

In other words, min_wal_size = 0 logically means that we MIGHT NOT
recycle any WAL segments, but the desired behavior here is that we DO
NOT recycle any WAL segments.

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

PostgreSQL 11 Beta 3 Release: 2018-08-09

2018-07-31 Thread Jonathan S. Katz

Hi,

The Release Management Team is pleased to announce that
the release date for PostgreSQL 11 Beta 3 is set to be 2018-08-09,
which coincides with the scheduled cumulative release.

As always, we appreciate everyone’s diligent effort fixing open items[1] and we
hope to get even more closed before the Beta 3 release. We do ask everyone to
pay extra attention to some of the items that have been open for awhile so we
can work on bringing them to a close.

Thank you again for your hard work and please let us know if you
have any questions.

Thanks,

Jonathan

[1] https://wiki.postgresql.org/wiki/PostgreSQL_11_Open_Items 




signature.asc
Description: Message signed with OpenPGP

Re: Standby trying "restore_command" before local WAL

2018-07-31 Thread Sergei Kornilov

Hello

> As mentioned by others, it sounds like we could have an option to try
> contacting the primary before running restore_commnad
Why about primary?
If we have restore_command on slave (or during point in time recovery) - we 
force using XLOG_FROM_ARCHIVE, even if XLOG_FROM_PG_WAL source can provide next 
WAL. As say xlog.c comment [1]:

> * We just successfully read a file in pg_wal. We prefer files in
> * the archive over ones in pg_wal, so try the next file again
> * from the archive first.

We have some actual reason why we prefer restore_command instead of using local 
wal files first?
Partially written WAL? Streaming replication can leave partially written WAL 
and we can handle this correctly.

Later (in XLogFileReadAnyTLI caller) we change XLOG_FROM_ARCHIVE to 
XLOG_FROM_ANY, but we force call XLOG_FROM_ARCHIVE first and then look in 
XLOG_FROM_PG_WAL. Or i am wrong?

regards, Sergei

[1] 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/transam/xlog.c;h=493f1db7b97a94b882382fc3d2112634f56c86a3;hb=refs/heads/master#l12113

Re: Standby trying "restore_command" before local WAL

2018-07-31 Thread Stephen Frost

Greetings,

* Emre Hasegeli (e...@hasegeli.com) wrote:
> This issue came to our attention after we migrated an application from
> an object storage backend, and noticed that restarting a standby node
> takes hours or sometimes days.
> 
> We are using shared WAL archive and find it practical to have
> "restore_command" configured in case we would left a standby offline
> for a long time.  However, during the short window the standby is
> restarted, the master manages to archive a segment.  Then,
> the standby executes "restore_command"  successfully, and continues
> downloading WAL from the archive causing the recovery to take orders
> of magnitude longer.

Why is it taking so long for the restore command to return the WAL
file..?  Is the issue that the primary is generating enough WAL that
whatever you're using for the restore command isn't able to feed PG the
WAL fast enough?  We ran into exactly that problem with users of
pgbackrest and took a different approach- we parallelized pulling down
the WAL files into a local queue to be able to feed PG's replay as fast
as possible.  We've also moved to C for the parts of the archive-get
and archive-push commands that are run out of archive_command and
restore_command to make them as fast as possible (and are working to
finish rewriting the rest into C as well).

> == The Workarounds ==
> 
> We can possibly work around this inside the "restore_command" or
> by delaying the archiving.  Working around inside the "restore_command"
> would involve checking whether the file exists under pg_wal/.  This
> should not be easy because the WAL file may be written partially.  It
> should be easier for Postgres to do this as it knows where to stop
> processing the local WAL.

Yeah, handing PG a partially written WAL from the existing pg_wal
directory sounds like a rather bad idea..

> It should also be possible to work around this problem by delaying
> archiving using "wal_keep_segments", or replication slots, or simply
> with sleep().  Though, none of those is the correct solution to
> the problem.  We don't need the master to keep more segments for
> the standbys.  We already have more than enough.

Right, you certainly don't want to hope that wal_keep_segments is
enough or to keep extra WAL on the primary.  You could use a replication
slot to avoid the wal_keep_segments risk, but then you're still going to
have potentially a lot of unnecessary WAL on the primary if the replica
has been offline.

> == The Change ==
> 
> This "restore_command" behavior is coming from the initial archiving
> and point-in-time-recovery implementation [2].   The code says
> "the reason is that the file in XLOGDIR could be an old, un-filled or
> partly-filled version that was copied and restored as part of
> backing up $PGDATA."  This was probably a good reason in 2004, but
> I don't think it still is.  AFAIK "pg_basebackup" eliminates this
> problem.  Also, with this reasoning, we should also try streaming from
> the master before trying the local WAL, but AFAIU we don't.

Just because pg_basebackup wouldn't necessairly create that risk doesn't
mean that the risk has gone away; pg_basebackup isn't the only way for a
replica to be brought up.

> If there will be a consensus on fixing this, I can try to prepare
> a patch.  The company, I am currently working for, is also interested
> in sponsoring a support company to fix this problem.

As mentioned by others, it sounds like we could have an option to try
contacting the primary before running restore_commnad (or, more
generally, an option which allows a user to specify their preference
regarding what PG should try to do first), but I'm trying to figure out
how going to the primary is going to solve your problem- if the replica
is offline for long enough, then the WAL isn't going to be available on
the primary as you outlined above and the WAL won't be local either
(since the replica wasn't online while the primary was creating WAL..),
so the only option would be to go to the restore_command.

As such, it seems like a faster restore_command would end up being a
better and more general solution.

That said, I don't have any particular issue with a patch to allow the
user to tell PG to try asking the primary first.  Of course, that
wouldn't be available until v12.

Thanks!

Stephen

signature.asc
Description: PGP signature

Re: Usability fail with psql's \dp command

2018-07-31 Thread Pavel Luzanov


On 28.07.2018 21:41, Tom Lane wrote:

I noticed today that \dp does not distinguish empty acl fields
(meaning nobody has any privileges) from null acl fields
(which mean default privileges, typically not empty).

This confusing behavior exists not only for \dp command.
Consider schemas and \dn+ command:

postgres=# create schema s authorization u;
CREATE SCHEMA
postgres=# \dn+ s
    List of schemas
 Name | Owner | Access privileges | Description
--+---+---+-
 s    | u |   |
(1 row)

postgres=# \c - u
You are now connected to database "postgres" as user "u".
postgres=> create table s.t(id int);
CREATE TABLE
postgres=> revoke all on schema s from u;
REVOKE
postgres=> \dn+ s
    List of schemas
 Name | Owner | Access privileges | Description
--+---+---+-
 s    | u |   |
(1 row)

postgres=> create table s.t2(id int);
ERROR:  permission denied for schema s
LINE 1: create table s.t2(id int);



One idea is to replace a null ACL value with the actual effective
permissions, which we could get from the acldefault() function.

As for me, this is a right option.
Very hard to describe (I am engaged in the development of training 
courses) why after GRANT command

we see two records in acl column, but after CREATE TABLE - no records.
Phrases like "for historical reasons" are not very convincing:

postgres=# create table t (id int);
CREATE TABLE
postgres=# \dp t
    Access privileges
 Schema | Name | Type  | Access privileges | Column privileges | Policies
+--+---+---+---+--
 public | t    | table | |   |
(1 row)

postgres=# grant select on t to u;
GRANT
postgres=# \dp t
    Access privileges
 Schema | Name | Type  | Access privileges | Column privileges 
| Policies

+--+---+---+---+--
 public | t    | table | postgres=arwdDxt/postgres+|   |
    |  |   | u=r/postgres |   |


-
Pavel Luzanov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [PATCH] Improve geometric types

2018-07-31 Thread Tomas Vondra





On 07/31/2018 05:14 PM, Emre Hasegeli wrote:

1) common_entry_cmp is still handling 'delta' as double, although the
CommonEntry was modified to use float8. IMHO it should also simply call
float8_cmp_internal instead of doing comparisons.


I am changing it to define delta as "float8" and to use float8_cmp_internal().


2) gist_box_picksplit does this

int m = ceil(LIMIT_RATIO * (float8) nentries);

instead of

int m = ceil(LIMIT_RATIO * (double) nentries);

which seems rather unnecessary, considering the only point of the cast was
probably to do more accurate multiplication. And it seems pointless to cast
it to float8 but then not use float8_mul().


I am removing the cast.


3) computeDistance does this:

 if (point->y > box->high.y)
 result = float8_mi(point->y, box->high.y);
 else if (point->y < box->low.y)
 result = float8_mi(box->low.y, point->y);

which seems suspicious. Shouldn't the comparisons be done by float8_lt and
float8_gt too? That's what we do elsewhere.


I assumed the GiST code already handles NaNs correctly and tried not
to change its behavior.  It may be a good idea to revert existing NaN
handling in favour of using the inline functions every time.  Should I
do that?


Ah, so there's an assumption that NaNs are handled earlier and never 
reach this place? That's probably a safe assumption. I haven't thought 
about that, it simply seemed suspicious that the code mixes direct 
comparisons and float8_mi() calls.





4) I think we should just get rid of GEODEBUG entirely. The preceding
patches removes about 20 out of 27 occurrences anyway, so let's ditch the
remaining few.


I agree.  Shall I append it to this patch?



Not sure, I'll leave that up to you. I don't mind doing it in a separate 
patch (I'd probably prefer that over mixing it into unrelated patch).


regards

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

Re: [PATCH] Improve geometric types

2018-07-31 Thread Emre Hasegeli

> 1) common_entry_cmp is still handling 'delta' as double, although the
> CommonEntry was modified to use float8. IMHO it should also simply call
> float8_cmp_internal instead of doing comparisons.

I am changing it to define delta as "float8" and to use float8_cmp_internal().

> 2) gist_box_picksplit does this
>
>int m = ceil(LIMIT_RATIO * (float8) nentries);
>
> instead of
>
>int m = ceil(LIMIT_RATIO * (double) nentries);
>
> which seems rather unnecessary, considering the only point of the cast was
> probably to do more accurate multiplication. And it seems pointless to cast
> it to float8 but then not use float8_mul().

I am removing the cast.

> 3) computeDistance does this:
>
> if (point->y > box->high.y)
> result = float8_mi(point->y, box->high.y);
> else if (point->y < box->low.y)
> result = float8_mi(box->low.y, point->y);
>
> which seems suspicious. Shouldn't the comparisons be done by float8_lt and
> float8_gt too? That's what we do elsewhere.

I assumed the GiST code already handles NaNs correctly and tried not
to change its behavior.  It may be a good idea to revert existing NaN
handling in favour of using the inline functions every time.  Should I
do that?

> 4) I think we should just get rid of GEODEBUG entirely. The preceding
> patches removes about 20 out of 27 occurrences anyway, so let's ditch the
> remaining few.

I agree.  Shall I append it to this patch?

Re: Documentaion fix.

On 2018-Jul-31, Kyotaro HORIGUCHI wrote:

> But, just fixing it makes the line seemingly a bit too long..

How about pasting it like this?

alvherre=# select * from pg_replication_slots \gx
─[ RECORD 1 ]───┬
slot_name   │ node_a_slot
plugin  │ 
slot_type   │ physical
datoid  │ 
database│ 
temporary   │ f
active  │ f
active_pid  │ 
xmin│ 
catalog_xmin│ 
restart_lsn │ 
confirmed_flush_lsn │ 

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

Re: Usability fail with psql's \dp command

2018-07-31 Thread David G. Johnston

On Tue, Jul 31, 2018 at 7:24 AM, Robert Haas  wrote:

> On Sat, Jul 28, 2018 at 4:36 PM, David Fetter  wrote:
> > Please find attached a patch to fix this.  Would this be a
> > back-patchable bug?
>
> In my view, this is not a bug fix, but an improvement, and therefore
> should not be back-patched.

I was leaning toward "bug fix" but what we are actually doing here is
compensating for our default presentation of null being the empty string;
so I'm inclined to side with "usability" and not back-patching.

David J.

Re: [PATCH] Improve geometric types

2018-07-31 Thread Tomas Vondra


Hi Emre,

Now that the buildfarm is no longer complaining about 0001 and 0002, I'm 
working on reviewing and committing 0003. It seems quite straightforward 
but I do have couple of comment/questions:


1) common_entry_cmp is still handling 'delta' as double, although the 
CommonEntry was modified to use float8. IMHO it should also simply call 
float8_cmp_internal instead of doing comparisons.


2) gist_box_picksplit does this

   int m = ceil(LIMIT_RATIO * (float8) nentries);

instead of

   int m = ceil(LIMIT_RATIO * (double) nentries);

which seems rather unnecessary, considering the only point of the cast 
was probably to do more accurate multiplication. And it seems pointless 
to cast it to float8 but then not use float8_mul().


3) computeDistance does this:

if (point->y > box->high.y)
result = float8_mi(point->y, box->high.y);
else if (point->y < box->low.y)
result = float8_mi(box->low.y, point->y);

which seems suspicious. Shouldn't the comparisons be done by float8_lt 
and float8_gt too? That's what we do elsewhere.


4) I think we should just get rid of GEODEBUG entirely. The preceding 
patches removes about 20 out of 27 occurrences anyway, so let's ditch 
the remaining few.



regards

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

Re: Bizarre behavior in libpq's searching of ~/.pgpass

Robert Haas  writes:
> On Fri, Jul 27, 2018 at 11:38 PM, Tom Lane  wrote:
>> I noticed that there's some strange coding in libpq's choice of
>> what hostname to use for searching ~/.pgpass for a password.

> Yeah, that's bad code.  The intent was that if you set host=a,b you
> probably want to use either 'a' or 'b' as the thing to look up in
> .pgpass, not 'a,b', but the implementation leaves something to be
> desired.

Right.  Closely related to that is that the existing code in
passwordFromFile behaves differently for null vs. empty hostname.
This is bad on its face, because nowhere else in libpq do we treat
empty-string parameters differently from unset ones, but it's particularly
a mess for the comma-separated-list case because then it's impossible
for one of the alternatives to be NULL.  In the already-posted patch
I fixed that so that an empty alternative is replaced by DefaultHost
in passwordFromFile, and likewise for port (though I think the latter
case may be unreachable).

But now that I look at it, it seems like the code in connectOptions2
has also Gotten It Wrong.  Shouldn't the replacement of "unspecified"
cases by DEFAULT_PGSOCKET_DIR/DefaultHost also happen on an entry-by-
entry basis, so that "host=foo," would behave as though the empty
entry were "localhost"?

regards, tom lane

Re: Usability fail with psql's \dp command

2018-07-31 Thread Robert Haas

On Sat, Jul 28, 2018 at 4:36 PM, David Fetter  wrote:
> Please find attached a patch to fix this.  Would this be a
> back-patchable bug?

In my view, this is not a bug fix, but an improvement, and therefore
should not be back-patched.

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

Re: Doc patch: add RECURSIVE to bookindex

2018-07-31 Thread Daniel Verite

Fabien COELHO wrote:

> Why referencing only create_view, but not delete, insert, update, select 
> or select_into where RECURSIVE is also used?
> 
> ISTM that at least the select page should be referenced, I'm less sure of 
> the others because there it appears only in the synopsys.

Looking at other occurrences of , it seems
they're used quite sparingly, limiting the references to only
the pages that have the most relevant explanation for the term,
as opposed to trying to be exhaustive.

For instance, select.sgml has only three , and
the entries for WHERE or JOIN don't even refer to it.
ISTM that it's on purpose, to keep the index lean, and it works.

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

Re: Adding a note to protocol.sgml regarding CopyData




Hello Tatsuo-san,


Minor suggestions, although I'm not a native English speaker.



Well, I did not intend to enhance libpq.sgml but maybe your points is
valid (I cannot judge because I am not an native English speaker).


Argh, sorry, I did not read the right part:-(

The note looks good to me. Doc build is ok.


So I am include your point to the patch and wait for feed back from
(possibly) native English speakers.


Indeed. As Tom said in another thread, a good part of the documentation 
has been written by non native English speaker, and it shows.



I also added to September commit fest, including you as a reviewer.


Hmmm... Already having a committer may deter other people to review it, 
while it is exactly what is desired.


--
Fabien.

Re: why doesn't pg_create_logical_replication_slot throw an error if the encoder doesn't exist

2018-07-31 Thread Dave Cramer

On 26 July 2018 at 16:49, Andres Freund  wrote:

> Hi,
>
> On 2018-07-26 16:40:18 -0400, Dave Cramer wrote:
> > Since nothing else can be done we should throw an error early.
>
> I can't immediately think of a reason not to do that, but I personally
> don't care enough to write a patch...
>

Fair enough. I will.

>
> Greetings,
>
> Andres Freund
>


Dave Cramer

Standby trying "restore_command" before local WAL

2018-07-31 Thread Emre Hasegeli

Currently the startup process tries the "restore_command" before
the WAL files locally available under pg_wal/ [1]. I believe we should
change this behavior.

== The Problem ==

This issue came to our attention after we migrated an application from
an object storage backend, and noticed that restarting a standby node
takes hours or sometimes days.

We are using shared WAL archive and find it practical to have
"restore_command" configured in case we would left a standby offline
for a long time. However, during the short window the standby is
restarted, the master manages to archive a segment. Then,
the standby executes "restore_command" successfully, and continues
downloading WAL from the archive causing the recovery to take orders
of magnitude longer.

== The Workarounds ==

We can possibly work around this inside the "restore_command" or
by delaying the archiving. Working around inside the "restore_command"
would involve checking whether the file exists under pg_wal/. This
should not be easy because the WAL file may be written partially. It
should be easier for Postgres to do this as it knows where to stop
processing the local WAL.

It should also be possible to work around this problem by delaying
archiving using "wal_keep_segments", or replication slots, or simply
with sleep(). Though, none of those is the correct solution to
the problem. We don't need the master to keep more segments for
the standbys. We already have more than enough.

== The Change ==

This "restore_command" behavior is coming from the initial archiving
and point-in-time-recovery implementation [2]. The code says
"the reason is that the file in XLOGDIR could be an old, un-filled or
partly-filled version that was copied and restored as part of
backing up $PGDATA." This was probably a good reason in 2004, but
I don't think it still is. AFAIK "pg_basebackup" eliminates this
problem. Also, with this reasoning, we should also try streaming from
the master before trying the local WAL, but AFAIU we don't.

If there will be a consensus on fixing this, I can try to prepare
a patch. The company, I am currently working for, is also interested
in sponsoring a support company to fix this problem.

[1]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/access/transam/xlogarchive.c;h=5c6de4989c9a#l72

[2]
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=66ec2db7284

Re: Doc patch: add RECURSIVE to bookindex