Re: [HACKERS] Logical Replication WIP

2016-12-09 Thread Petr Jelinek
Hi,

On 09/12/16 22:00, Erik Rijkers wrote:
> On 2016-12-09 17:08, Peter Eisentraut wrote:
> 
> Your earlier 0001-Add-support-for-temporary-replication-slots.patch
> could be applied instead of the similarly named, original patch by Petr.
> (I used 19fcc0058ecc8e5eb756547006bc1b24a93cbb80 to apply this patch-set
> to)
> 
> (And it was, by the way,  pretty stable and running well.)
> 

Great, thanks for testing.

> I'd like to get it running again but now I can't find a way to also
> include your newer 0001-fixup-Add-PUBLICATION-catalogs-and-DDL.patch of
> today.
> 
> How should these patches be applied (and at what level)?
> 
> 20161208: 0001-Add-support-for-temporary-replication-slots__petere.patch
>  # petere
> 20161202: 0002-Add-PUBLICATION-catalogs-and-DDL-v11.patch  # PJ
> 20161209: 0001-fixup-Add-PUBLICATION-catalogs-and-DDL.patch  # petere
> 20161202: 0003-Add-SUBSCRIPTION-catalog-and-DDL-v11.patch  # PJ
> 20161202:
> 0004-Define-logical-replication-protocol-and-output-plugi-v11.patch  # PJ
> 20161202: 0005-Add-logical-replication-workers-v11.patch  # PJ
> 20161202:
> 0006-Add-separate-synchronous-commit-control-for-logical--v11.patch  # PJ
> 
> Could (one of) you give me a hint?
> 

I just sent in a rebased patch that includes all of it.

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


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


Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

2016-12-09 Thread Keith Fiske
On Fri, Dec 9, 2016 at 10:01 PM, Robert Haas  wrote:

> On Fri, Dec 9, 2016 at 5:55 PM, Keith Fiske  wrote:
> > Another suggestion I had was for handling when data is inserted that
> doesn't
> > match any defined child tables. Right now it just errors out, but in
> > pg_partman I'd had it send the data to the parent instead to avoid data
> > loss. I know that's not possible here, but how about syntax to define a
> > child table as a "default" to take data that would normally be rejected?
> > Maybe something like
> >
> > CREATE TABLE measurement_default
> > PARTITION OF measurement (
> > unitsales DEFAULT 0
> > ) FOR VALUES DEFAULT;
>
> One thing that's tricky/annoying about this is that if you have a
> DEFAULT partition and then add a partition, you have to scan the
> DEFAULT partition for data that should be moved to the new partition.
> That makes what would otherwise be a quick operation slow.  Still, I'm
> sure there's a market for that feature.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

I would find that perfectly acceptable as long as a caveat about the
performance impact was included in the documentation. My intent with
putting the data in the parent in pg_partman was solely to avoid data loss
and I also included a function for monitoring if data went into the parent.
That sort of function may not have real utility in core, but I think the
intent of the DEFAULT location is a catchall "just in case" and not really
intended as a permanent data store. If people did use it that way, and a
warning was included about its cost when adding new partitions, then that's
on the user for doing that.

I recall reading in the other thread about this that you're looking to make
locking across the partition set less strict eventually. If you could make
the scan and data move not block on anything except the partitions
involved, I think the performance impact of scanning the default partition
and moving the data wouldn't even be that bad in the end.

Keith


Re: [HACKERS] proposal: psql statements \gstore \gstore_binary (instead COPY RAW)

2016-12-09 Thread Pavel Stehule
2016-12-10 2:27 GMT+01:00 Jim Nasby :

> On 12/9/16 9:39 AM, Pavel Stehule wrote:
>
>>
>> SELECT image FROM accounts WHERE id = xxx
>> \gstore_binary ~/image.png
>>
>> What do you think about this proposal?
>>
>
> Seems reasonable.
>
> I've lost track at this point... is there a way to go the other direction
> with that as well? Namely, stick the contents of a file into a field via an
> INSERT or UPDATE?
>

a target of this feature is storing only. For import there should be
another statements.

I am think so there is a consensus (with Tom) on binary passing mode. Some
like

\set USE_BINARY on

What is not clean (where is not a agreement is a way how to get a some
content) - if we use a variables with content (not references), then we can
or cannot to have special statement

so some ways how to push some binary content to server

A)
\set  `cat file`
\set USE_BINARY on
INSERT INTO tab(id, data) VALUES(1, :::bytea);

B)
\set  `cat file`
INSERT INTO tab(id, data) VALUES (1, :x''); -- use bytea escape

C)
\load_binary  file
INSERT INTO tab(id, data) VALUES(1, :'');

D)
\load  file
\set USE_BINARY on
INSERT INTO tab(id, data) VALUES(1, :::bytea);

E)
\set  ``cat file``
INSERT INTO tab(id, data) VALUES (1, :'');

any from mentioned variants has some advantages - and I don't see a clean
winner. I like a binary mode for passing - the patch is small and clean and
possible errors are well readable (not a MBytes of hexa numbers). Passing
in text mode is safe - although the some errors, logs can be crazy. I would
to use some form of "load" backslash command ("load", "load_binary"): a) we
can implement a file tab complete, b) we can hide some platform specific
("cat" linux, "type" windows).

Now, only text variables are supported - it is enough for passing XML, JSON
- but not for binary data (one important variant is passing XML binary for
automatic XML internal encoding transformation). So we should to encode
content before storing to variable, or we should to introduce binary
variables. It is not hard - introduce new functions, current API will
supports text variables.

The implementation of these variants is short, simple - we can implement
more than exactly one way - @E is general, but little bit magic, and
without a autocomplete possibility, @C is very clear

The discussion can be about importance following features:

1. binary passing  (important for XML, doesn't fill a logs, a speed is not
important in this context)
2. tab complete support
3. verbosity, readability

I would to know how these points are important, interesting for other
people? It can helps with choosing variant or variants that we can
implement. I don't expect some significant differences in implementation
complexity of mentioned variants - the code changes will be +/- same.

Regards

Pavel



>
> I've done that in the past via psql -v var=`cat file`, but there's
> obviously some significant drawbacks to that...
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
> 855-TREBLE2 (855-873-2532)
>


Re: [HACKERS] Patch to implement pg_current_logfile() function

2016-12-09 Thread Karl O. Pinc
Hi Gilles,

On Fri, 9 Dec 2016 23:41:25 +0100
Gilles Darold  wrote:

>   /* extract log format and log file path from the line */
>   log_filepath = strchr(lbuffer, ' ');
>   log_filepath++;
>   lbuffer[log_filepath-lbuffer-1] = '\0';
>   log_format = lbuffer;
>   *strchr(log_filepath, '\n') = '\0';

Instead I propose (code I have not actually executed):
...
charlbuffer[MAXPGPATH];
char*log_format = lbuffer;
...

/* extract log format and log file path from the line */
log_filepath = strchr(lbuffer, ' ');  /* lbuffer == log_format */
*log_filepath = '\0'; /* terminate log_format */
log_filepath++;   /* start of file path */
log_filepath[strcspn(log_filepath, "\n")] = '\0';


My first thought was to follow your example and begin with
log_format = lbuffer;
But upon reflection I think changing the declaration of log_format
to use an initializer better expresses how storage is always shared.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


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


Re: [HACKERS] Time to retire Windows XP buildfarm host?

2016-12-09 Thread Andrew Dunstan



On 12/05/2016 11:38 AM, Andrew Dunstan wrote:



On 12/05/2016 11:17 AM, Tom Lane wrote:

Andrew Dunstan  writes:
Windows XP has been past end of life for quite some time. 
Nevertheless I

have kept my instance running with three buildfarm members: frogmouth,
currawong and brolga. Howeever, a recent commit (apparently fa2fa99, 
but

I'm not 100% sure) started causing a compiler segmentation fault on
frogmouth, the mingw/gcc-4.5.0 animal running on this machine.
...
Does anyone have any strong opinion in favor of keeping any of these
animals running?

Hm, a look through our commit logs finds multiple mentions of each of
these animals as having been the only one showing a particular 
portability

issue.  So I'm worried about loss of portability coverage.  Are you sure
that the animals' build options and choices of toolchains are replicated
elsewhere?





Not exactly the same, no. e.g. jacana is similar but it doesn't build 
with python, tcl or openssl, and does build with nls, it's 64bit vs 
32bit, and it runs a more modern compiler on a more modern OS.


If you think it's worth it I will put some effort into fixing 
frogmouth. I can certainly keep the others running for a while with 
little difficulty. I will disable frogmouth until I get a chance to 
look for a fix - next week at the earliest.






... and miraculously it has fixed itself.

cheers

andrew



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


Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

2016-12-09 Thread Robert Haas
On Fri, Dec 9, 2016 at 5:55 PM, Keith Fiske  wrote:
> Another suggestion I had was for handling when data is inserted that doesn't
> match any defined child tables. Right now it just errors out, but in
> pg_partman I'd had it send the data to the parent instead to avoid data
> loss. I know that's not possible here, but how about syntax to define a
> child table as a "default" to take data that would normally be rejected?
> Maybe something like
>
> CREATE TABLE measurement_default
> PARTITION OF measurement (
> unitsales DEFAULT 0
> ) FOR VALUES DEFAULT;

One thing that's tricky/annoying about this is that if you have a
DEFAULT partition and then add a partition, you have to scan the
DEFAULT partition for data that should be moved to the new partition.
That makes what would otherwise be a quick operation slow.  Still, I'm
sure there's a market for that feature.

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


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


Re: [HACKERS] proposal: psql statements \gstore \gstore_binary (instead COPY RAW)

2016-12-09 Thread Andrew Dunstan



On 12/09/2016 08:27 PM, Jim Nasby wrote:

On 12/9/16 9:39 AM, Pavel Stehule wrote:


SELECT image FROM accounts WHERE id = xxx
\gstore_binary ~/image.png

What do you think about this proposal?


Seems reasonable.

I've lost track at this point... is there a way to go the other 
direction with that as well? Namely, stick the contents of a file into 
a field via an INSERT or UPDATE?


I've done that in the past via psql -v var=`cat file`, but there's 
obviously some significant drawbacks to that...



It all looks eerily familiar ...

cheers

andrew


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


Re: [HACKERS] proposal: psql statements \gstore \gstore_binary (instead COPY RAW)

2016-12-09 Thread Jim Nasby

On 12/9/16 9:39 AM, Pavel Stehule wrote:


SELECT image FROM accounts WHERE id = xxx
\gstore_binary ~/image.png

What do you think about this proposal?


Seems reasonable.

I've lost track at this point... is there a way to go the other 
direction with that as well? Namely, stick the contents of a file into a 
field via an INSERT or UPDATE?


I've done that in the past via psql -v var=`cat file`, but there's 
obviously some significant drawbacks to that...

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


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


Re: [HACKERS] Macro customizable hashtable / bitmapscan & aggregation perf

2016-12-09 Thread Andres Freund
Hi,

On 2016-12-09 15:21:36 -0800, Mark Dilger wrote:
> Andres,
> 
> Your patch, below, appears to have been applied to master in commit
> 5dfc198146b49ce7ecc8a1fc9d5e171fb75f6ba5.  It makes mention of a
> function, tuplehash_start_iterate, in a macro, but the function is not
> defined or declared, and its signature and intention is not clear.  Is there
> any chance you could add some documentation about how this function
> is intended to be used and defined?
> 
> See InitTupleHashIterator in src/include/nodes/execnodes.h

The patch generates functions based on the defined prefix. E.g.

/* define paramters necessary to generate the tuple hash table interface */
#define SH_PREFIX tuplehash
#define SH_ELEMENT_TYPE TupleHashEntryData
#define SH_KEY_TYPE MinimalTuple
#define SH_SCOPE extern
#define SH_DECLARE
#include "lib/simplehash.h"

makes tuplehash_iterate out of
#define SH_START_ITERATE SH_MAKE_NAME(start_iterate)
...
SH_SCOPE void SH_START_ITERATE(SH_TYPE *tb, SH_ITERATOR *iter);
SH_SCOPE void
SH_START_ITERATE(SH_TYPE *tb, SH_ITERATOR *iter)
{
...
}

See the simplehash.h's header for some explanation.


Hope that helps,

Andres


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


Re: [HACKERS] Macro customizable hashtable / bitmapscan & aggregation perf

2016-12-09 Thread Mark Dilger
Andres,

Your patch, below, appears to have been applied to master in commit
5dfc198146b49ce7ecc8a1fc9d5e171fb75f6ba5.  It makes mention of a
function, tuplehash_start_iterate, in a macro, but the function is not
defined or declared, and its signature and intention is not clear.  Is there
any chance you could add some documentation about how this function
is intended to be used and defined?

See InitTupleHashIterator in src/include/nodes/execnodes.h

Thanks,

Mark Dilger

> On Oct 9, 2016, at 4:38 PM, Andres Freund  wrote:
> 
> Hi,
> 
> Attached is an updated version of the patchset. The main changes are
> - address most of Tomas' feedback
> - address regression test output changes by adding explicit ORDER BYs,
>  in a separate commit.
> - fix issues around hash tables sized up to PG_UINT32_MAX
> - fix a bug in iteration with "concurrent" deletions
> 
>>> We didn't really for dynahash (as it basically assumed a fillfactor of 1
>>> all the time), not sure if changing it won't also cause issues.
>>> 
>> 
>> That's a case of glass half-full vs. half-empty, I guess. If we assumed load
>> factor 1.0, then I see it as accounting for load factor (while you see it as
>> not accounting of it).
> 
> Well, that load factor is almost never achieved, because we'd have grown
> since...  I added a comment about disregarding fill factor and growth
> policy to estimate_hashagg_tablesize, which is actually the place where
> it'd seemingly make sense to handle it.
> 
> Tomas, did you have time to run a benchmark?
> 
> I think this is starting to look good.
> 
> 
> Regards,
> 
> Andres
> <0001-Add-likely-unlikely-branch-hint-macros.patch><0002-Make-regression-tests-less-dependent-on-hash-table-o.patch><0003-Add-a-macro-customizable-hashtable.patch><0004-Use-more-efficient-hashtable-for-tidbitmap.c-to-spee.patch><0005-Use-more-efficient-hashtable-for-execGrouping.c-to-s.patch>
> -- 
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers



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


Re: [HACKERS] tuplesort_gettuple_common() and *should_free argument

2016-12-09 Thread Peter Geoghegan
On Fri, Oct 21, 2016 at 4:45 PM, Peter Geoghegan  wrote:
> More importantly, there are no remaining cases where
> tuplesort_gettuple_common() sets "*should_free = true", because there
> is no remaining need for caller to *ever* pfree() tuple. Moreover, I
> don't anticipate any future need for this -- the scheme recently
> established around per-tape "slab slots" makes it seem unlikely to me
> that the need to "*should_free = true" will ever arise again.

Attached patch 0001-* removes all should_free arguments. To reiterate,
this is purely a refactoring patch.

> Now, it is still true that at least some callers to
> tuplesort_gettupleslot() (but not any other "get tuple" interface
> routine) are going to *require* ownership of memory for returned
> tuples, which means a call to heap_copy_minimal_tuple() remains
> necessary there (see recent commit
> d8589946ddd5c43e1ebd01c5e92d0e177cbfc198 for details). But, that's
> beside the point. In the master branch only, the
> tuplesort_gettupleslot() test "if (!should_free)" seems tautological,
> just as similar tests are for every other tuplesort_gettuple_common()
> caller. So, tuplesort_gettupleslot() can safely assume it should
> *always* heap_copy_minimal_tuple() in the master branch. (BTW, I'm
> going to teach tuplesort_gettuple_common() to avoid this
> heap_copy_minimal_tuple() call for callers that don't actually need
> it, but that's a discussion for another thread).

I decided that it made sense to address this at the same time after
all. So, the second patch attached here, 0002-*, goes on to do so.
This is a performance optimization that we already agreed was a good
idea for Postgres 10 when d8589946ddd5c43e1ebd01c5e92d0e177cbfc198
went in.

Note that the call sites that now opt out of receiving their own
personal copy are essentially returning to their behavior for the
entire beta phase of PostgreSQL 9.6. The problem with that accidental
state of affairs was that it wasn't always safe. But, it was still
generally safe for the majority of callers, I believe, not least
because it took months to hear any complaints at all. That majority of
callers now opt out.

The main question for reviewers of 0002-*, then, is whether or not I
have correctly determined which particular callers can safely opt out.

Finally, 0003-* is a Valgrind suppression borrowed from my parallel
CREATE INDEX patch. It's self-explanatory.

-- 
Peter Geoghegan
From 440f9724362c376807c36d814046015285e5c237 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan 
Date: Fri, 9 Dec 2016 13:49:18 -0800
Subject: [PATCH 3/3] Add valgrind suppression for logtape_write

This avoids having Valgrind report that logtape.c writes out
uninitialized memory.
---
 src/tools/valgrind.supp | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/tools/valgrind.supp b/src/tools/valgrind.supp
index af03051..775db0d 100644
--- a/src/tools/valgrind.supp
+++ b/src/tools/valgrind.supp
@@ -120,6 +120,15 @@
 	fun:RelationMapFinishBootstrap
 }
 
+{
+	logtape_write
+	Memcheck:Param
+	write(buf)
+
+	...
+	fun:LogicalTape*
+}
+
 
 # gcc on ppc64 can generate a four-byte read to fetch the final "char" fields
 # of a FormData_pg_cast.  This is valid compiler behavior, because a proper
-- 
2.7.4

From 3e3ef067713527e210ce76511955a5220fbbf15d Mon Sep 17 00:00:00 2001
From: Peter Geoghegan 
Date: Thu, 13 Oct 2016 10:54:31 -0700
Subject: [PATCH 2/3] Avoid copying within tuplesort_gettupleslot()

Add a "copy" argument to make it optional to receive a copy of caller
tuple that is safe to use across tuplesort_gettupleslot() calls, as a
performance optimization.  Existing tuplesort_gettupleslot() callers are
made to opt out of copying where that has been determined to be safe.
This brings tuplesort_gettupleslot() in line with
tuplestore_gettupleslot().

In the future, a "copy" tuplesort_getdatum() argument may be added, that
similarly allows callers to opt out of receiving a new copy of tuple
under their direct ownership.
---
 src/backend/executor/nodeAgg.c |  9 ++---
 src/backend/executor/nodeSort.c|  5 +++--
 src/backend/utils/adt/orderedsetaggs.c |  5 +++--
 src/backend/utils/sort/tuplesort.c | 17 +++--
 src/include/utils/tuplesort.h  |  2 +-
 5 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index eefb3d6..16a1263 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -570,6 +570,9 @@ initialize_phase(AggState *aggstate, int newphase)
  * Fetch a tuple from either the outer plan (for phase 0) or from the sorter
  * populated by the previous phase.  Copy it to the sorter for the next phase
  * if any.
+ *
+ * Callers cannot rely on memory for tuple in returned slot remaining allocated
+ * past any subsequent call here.  (The sorter may recycle the memory.)
  */
 static TupleTableSlot *
 fetch_input_tuple(AggState *aggstate)
@@ -578,8 

Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

2016-12-09 Thread Keith Fiske
On Fri, Dec 9, 2016 at 1:23 PM, Keith Fiske  wrote:

>
>
> On Fri, Dec 9, 2016 at 1:13 PM, Amit Langote 
> wrote:
>
>> Hi Keith,
>>
>> On Sat, Dec 10, 2016 at 3:00 AM, Keith Fiske  wrote:
>> > Being that table partitioning is something I'm slightly interested in,
>> > figured I'd give it a whirl.
>> >
>> > This example in the docs has an extraneous comma after the second column
>> >
>> > CREATE TABLE cities (
>> > name text not null,
>> > population   int,
>> > ) PARTITION BY LIST (initcap(name));
>> >
>> > And the WITH OPTIONS clause does not appear to be working using another
>> > example from the docs. Not seeing any obvious typos.
>> >
>> > keith@keith=# CREATE TABLE measurement_y2016m07
>> > keith-# PARTITION OF measurement (
>> > keith(# unitsales WITH OPTIONS DEFAULT 0
>> > keith(# ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
>> > 2016-12-09 12:51:48.728 EST [11711] ERROR:  syntax error at or near
>> "WITH"
>> > at character 80
>> > 2016-12-09 12:51:48.728 EST [11711] STATEMENT:  CREATE TABLE
>> > measurement_y2016m07
>> > PARTITION OF measurement (
>> > unitsales WITH OPTIONS DEFAULT 0
>> > ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
>> > ERROR:  syntax error at or near "WITH"
>> > LINE 3: unitsales WITH OPTIONS DEFAULT 0
>> >   ^
>> > Time: 0.184 ms
>> >
>> > Removing the unit_sales default allows it to work fine
>>
>> WITH OPTIONS keyword phrase is something that was made redundant in
>> the last version of the patch, but I forgot to remove the same in the
>> example.  I've sent a doc patch to fix that.
>>
>> If you try - unitsales DEFAULT 0, it will work.  Note that I did not
>> specify WITH OPTIONS.
>>
>> Thanks,
>> Amit
>>
>
> That works. Thanks!
>
> keith@keith=# CREATE TABLE measurement_y2016m07
> PARTITION OF measurement (
> unitsales DEFAULT 0
> ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
> CREATE TABLE
> Time: 4.091 ms
>
>
Working on a blog post for this feature and just found some more
inconsistencies with the doc examples. Looks like the city_id column was
defined in the measurements table when it should be in the cities table.
The addition of the partition to the cities table fails since it's missing.

Examples should look like this:

CREATE TABLE measurement (
logdate date not null,
peaktempint,
unitsales   int
) PARTITION BY RANGE (logdate);

CREATE TABLE cities (
city_id bigserial not null,
name text not null,
population   int
) PARTITION BY LIST (initcap(name));

I actually changed my example to have city_id use bigserial to show that
sequences are inherited automatically. May be good to show that in the docs.

Another suggestion I had was for handling when data is inserted that
doesn't match any defined child tables. Right now it just errors out, but
in pg_partman I'd had it send the data to the parent instead to avoid data
loss. I know that's not possible here, but how about syntax to define a
child table as a "default" to take data that would normally be rejected?
Maybe something like

CREATE TABLE measurement_default
PARTITION OF measurement (
unitsales DEFAULT 0
) FOR VALUES DEFAULT;

Keith


Re: [HACKERS] Patch to implement pg_current_logfile() function

2016-12-09 Thread Gilles Darold
Le 08/12/2016 à 00:52, Robert Haas a écrit :
> On Tue, Dec 6, 2016 at 6:11 PM, Gilles Darold  
> wrote:
>> It seems that all fixes have been included in this patch.
> Yikes.  This patch has certainly had a lot of review, but it has lots
> of basic inconsistencies with PostgreSQL coding style, which one would
> think somebody would have noticed by now, and there are multiple
> places where the logic seems to do in a complicated way something that
> could be done significantly more simply.  I don't have any objection
> to the basic idea of this patch, but it's got to look and feel like
> the surrounding PostgreSQL code.  And not be unnecessarily
> complicated.
>
> Detailed comments below:
>
> The SGML doesn't match the surrounding style - for example, 
> typically appears on a line by itself.
Fixed.

> +if (!Logging_collector) {
>
> Formatting.
Fixed.

> rm_log_metainfo() could be merged into logfile_writename(), since
> they're always called in conjunction and in the same pattern.  The
> function is poorly named; it should be something like
> update_metainfo_datafile().
Merge and logfile_writename() function renamed as suggested.

> +if (errno == ENFILE || errno == EMFILE)
> +ereport(LOG,
> +(errmsg("system is too busy to write logfile meta
> info, %s will be updated on next rotation (or use SIGHUP to retry)",
> LOG_METAINFO_DATAFILE)));
>
> This seems like a totally unprincipled reaction to a purely arbitrary
> subset of errors.  EMFILE or ENFILE doesn't represent a general "too
> busy" condition, and logfile_open() has already logged the real error.
Removed.

> +snprintf(tempfn, sizeof(tempfn), "%s.tmp", LOG_METAINFO_DATAFILE);
>
> You don't really need to use snprintf() here.  You could #define
> LOG_METAINFO_DATAFILE_TMP LOG_METAINFO_DATAFILE ".tmp" and compute
> this at compile time instead of runtime.
Done and added to syslogger.h

> +if (PG_NARGS() == 1) {
> +fmt = PG_ARGISNULL(0) ? NULL : PG_GETARG_TEXT_PP(0);
> +if (fmt != NULL) {
> +logfmt = text_to_cstring(fmt);
> +if ( (strcmp(logfmt, "stderr") != 0) &&
> +(strcmp(logfmt, "csvlog") != 0) ) {
> +ereport(ERROR,
> +(errmsg("log format %s not supported, possible values are
> stderr or csvlog", logfmt)));
> +PG_RETURN_NULL();
> +}
> +}
> +} else {
> +logfmt = NULL;
> +}
>
> Formatting.  This is unlike PostgreSQL style in multiple ways,
> including cuddled braces, extra parentheses and spaces, and use of
> braces around a single-line block.  Also, this could be written in a
> much less contorted way, like:
>
> if (PG_NARGS() == 0 || PG_ARGISNULL(0))
> logfmt = NULL;
> else
> {
> logfmt = text_to_cstring(PG_GETARG_TEXT_PP(0));
> if (strcmp(logfmt, "stderr") != 0 && strcmp(logfmt, "csvlog") != 0)
>  ereport(...);
> }
Applied.

> Also, the ereport() needs an errcode(), not just an errmsg().
> Otherwise it defaults to ERRCODE_INTERNAL_ERROR, which is not correct.
Add errcode(ERRCODE_INVALID_PARAMETER_VALUE) to the ereport call.

> +/* Check if current log file is present */
> +if (stat(LOG_METAINFO_DATAFILE, _buf) != 0)
> +PG_RETURN_NULL();
>
> Useless test.  The code that follows catches this case anyway and
> handles it the same way.  Which is good, because this has an inherent
> race condition. The previous if (!Logging_collector) test sems fairly
> useless too; unless there's a bug in your syslogger.c code, the file
> won't exist anyway, so we'll return NULL for that reason with no extra
> code needed here.
That's right, both test have been removed.

> +/*
> +* Read the file to gather current log filename(s) registered
> +* by the syslogger.
> +*/
>
> Whitespace isn't right.
>
> +while (fgets(lbuffer, sizeof(lbuffer), fd) != NULL) {
> +charlog_format[10];
> +int i = 0, space_pos = 0;
> +
> +/* Check for a read error. */
> +if (ferror(fd)) {
>
> More coding style issues.
>
> +ereport(ERROR,
> +(errcode_for_file_access(),
> +errmsg("could not read file \"%s\": %m",
> LOG_METAINFO_DATAFILE)));
> +FreeFile(fd);
> +break;
>
> ereport(ERROR) doesn't return, so the code that follows is pointless.
All issues above are fixed.

> +if (strchr(lbuffer, '\n') != NULL)
> +*strchr(lbuffer, '\n') = '\0';
>
> Probably worth adding a local variable instead of doing this twice.
> Local variables are cheap, and the code would be more readable.
Removed

> +if ((space_pos == 0) && (isspace(lbuffer[i]) != 0))
>
> Too many parentheses.  Also, I think the whole loop in which this is
> contained could be eliminated entirely.  Just search for the first ' '
> character with strchr(); you don't need to are about isspace() because
> you know the code that writes this file uses ' ' 

Re: [HACKERS] [OSSTEST PATCH 0/1] PostgreSQL db: Retry on constraint violation

2016-12-09 Thread Kevin Grittner
On Fri, Dec 9, 2016 at 12:26 PM, Ian Jackson  wrote:

> In summary: PostgreSQL only provides transaction serialisability for
> successful transactions.  Even with SERIALIZABLE, transactions may
> fail due to spurious and "impossible" constraint violations.
>
> As a result, I need to make osstest retry transactions not only on
> explicitly reported serialisation failures and deadlocks, but also on
> integrity violations.
>
> It is not clear to me from the thread
>   WIP: Detecting SSI conflicts before reporting constraint violations
> which was on pgsql-hackers earlier this year, whether it is only
> unique constraint violations which may spuriously occur.
>
> Can anyone from the PostgreSQL hacker community advise ?  The existing
> documentation patch just talks about "successful" transactions, and
> uses unique constraints as an example.  In principle this leaves open
> the possibility that the transaction might fail with bizarre and
> unpredictable error codes, although I hope this isn't possible.

It is, especially prior to 9.6.

> It would be good to narrow the scope of the retries in my system as
> much as possible.

If I recall correctly, the constraints for which there can be
errors appearing due to concurrent transactions are primary key,
unique, and foreign key constraints.  I don't remember seeing it
happen, but it would not surprise me if an exclusion constraint can
also cause an error due to a concurrent transaction's interaction
with the transaction receiving the error.

The good news is that in 9.6 we were able to change many of these
to serialization failures if there was an explicit check for a
violation before creating the conflict.  For example, if there is a
primary key (or unique constraint or unique index) and within a
transaction you SELECT to test for the presence of a particular
value (or set of values, in the case of a multi-column constraint
or index) and it is not found, an attempt to insert that runs into
an insert from a concurrent transaction will now generate a
serialization failure.

> I'm hoping to get an authoritative answer here but it seems that this
> is a common problem to which there is still not yet a definitive
> solution.  I would like there to be a definitive solution.

I'm afraid that even though the frequency of getting some error
other than a serialization failure due to the actions of a
concurrent transaction, even if the failing transaction could not
have failed in the absence of the concurrent transaction and is
likely to succeed on retry, has gone down in 9.6, it has not been
eliminated.  You are forced to choose between performing some
limited number of retries for constraint violations or presenting
users with error messages which "should not be possible".
Obviously, the former means you will sometimes retry when there is
no hope of the retry succeeding and give up on retries when a retry
is likely to work, and the latter can confuse users and waste their
time.  :-(

> If I get a clear answer I'll submit a further docs patch to pgsql :-).

Of course, code patches to improve the situation are also welcome!  :-)

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


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


Re: [HACKERS] Logical Replication WIP

2016-12-09 Thread Erik Rijkers

On 2016-12-09 17:08, Peter Eisentraut wrote:

Your earlier 0001-Add-support-for-temporary-replication-slots.patch 
could be applied instead of the similarly named, original patch by Petr.
(I used 19fcc0058ecc8e5eb756547006bc1b24a93cbb80 to apply this patch-set 
to)


(And it was, by the way,  pretty stable and running well.)

I'd like to get it running again but now I can't find a way to also 
include your newer 0001-fixup-Add-PUBLICATION-catalogs-and-DDL.patch of 
today.


How should these patches be applied (and at what level)?

20161208: 0001-Add-support-for-temporary-replication-slots__petere.patch 
 # petere

20161202: 0002-Add-PUBLICATION-catalogs-and-DDL-v11.patch  # PJ
20161209: 0001-fixup-Add-PUBLICATION-catalogs-and-DDL.patch  # petere
20161202: 0003-Add-SUBSCRIPTION-catalog-and-DDL-v11.patch  # PJ
20161202: 
0004-Define-logical-replication-protocol-and-output-plugi-v11.patch  # 
PJ

20161202: 0005-Add-logical-replication-workers-v11.patch  # PJ
20161202: 
0006-Add-separate-synchronous-commit-control-for-logical--v11.patch  # 
PJ


Could (one of) you give me a hint?

Thanks,

Erik Rijkers


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


Re: [HACKERS] Fix for segfault in plpython's exception handling

2016-12-09 Thread Tom Lane
I wrote:
> Rafa de la Torre  writes:
>>  exc = PyErr_NewException(exception_map[i].name, base, dict);
>> +Py_INCREF(exc);
>>  PyModule_AddObject(mod, exception_map[i].classname, exc);

> Hm.  Seems like if this is a problem, the code for the other three
> exceptions is being a bit careless: it does do Py_INCREFs on them,
> but not soon enough to ensure no problems.  Also, I wonder why that
> code checks for a null result from PyErr_NewException but this doesn't.

> Good catch though.  A naive person would have assumed that
> PyModule_AddObject would increment the object's refcount, but
> the Python docs say "This steals a reference to value", which
> I guess must mean that the caller is required to do it.

For me (testing with Python 2.6.6 on RHEL6), this test case didn't result
in a server crash, but in the wrong exception object name being reported.
Tracing through it showed that a python GC was happening during the loop
adding all the exceptions to the spiexceptions module, so that some of the
exception objects went away and then were immediately reallocated as other
exception objects.  The explicit gc call in the test case wasn't
necessary, because the problem happened before that.  Fun fun.

I've pushed a patch that deals with all these problems.  Thanks for
the report!

regards, tom lane


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


Re: [HACKERS] new table partitioning breaks \d table to older versions

2016-12-09 Thread Jeff Janes
On Fri, Dec 9, 2016 at 10:18 AM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:

>
>
> On Fri, Dec 9, 2016 at 3:59 PM, Jeff Janes  wrote:
> >
> > Since:
> >
> > commit f0e44751d7175fa3394da2c8f85e3ceb3cdbfe63
> > Author: Robert Haas 
> > Date:   Wed Dec 7 13:17:43 2016 -0500
> >
> > Implement table partitioning.
> >
> > If I use psql compiled from 10devel to connect to a 9.6.1 server, then
> \d fails:
> >
> > psql (10devel-f0e4475, server 9.6.1-16e7c02)
> > Type "help" for help.
> >
> >
> > # \d pgbench_accounts
> > ERROR:  column c.relpartbound does not exist
> > LINE 1: ...ELECT inhparent::pg_catalog.regclass,
> pg_get_expr(c.relpartb...
> >
>
> Looks like they forgot to adjust version check number in describe.c code.
>
> Attched patch fix it.
>

Tested (but not read) and it fixes it for me.  thanks.

Jeff


Re: [HACKERS] proposal: psql statements \gstore \gstore_binary (instead COPY RAW)

2016-12-09 Thread Pavel Stehule
2016-12-09 19:48 GMT+01:00 Oleksandr Shulgin :

> On Dec 9, 2016 18:40, "Pavel Stehule"  wrote:
>
> Hi
>
> Long time I am pushing a COPY RAW - without success.
>
> Now I propose functionally similar solution - reduced to only to psql
> console
>
> Now we have a statement \g for execution query, \gset for exec and store
> result in memory and I propose \gstore for storing result in file and
> \gstore_binary for storing result in file with binary passing. The query
> result should be one row, one column.
>
> Usage:
>
> SELECT image FROM accounts WHERE id = xxx
> \gstore_binary ~/image.png
>
> What do you think about this proposal?
>
>
> I might be missing something, but is it different from:
>
> \t
> \a
> \o output_filename
> SELECT ...
> \o
>
> ?
>
>
The \gstore is same like these commands - but it is user friendly - one
liner statement.

For \gstore_binary there is not any workaround

Pavel


> --
> Alex
>
>
>


Re: [HACKERS] proposal: psql statements \gstore \gstore_binary (instead COPY RAW)

2016-12-09 Thread Oleksandr Shulgin
On Dec 9, 2016 18:40, "Pavel Stehule"  wrote:

Hi

Long time I am pushing a COPY RAW - without success.

Now I propose functionally similar solution - reduced to only to psql
console

Now we have a statement \g for execution query, \gset for exec and store
result in memory and I propose \gstore for storing result in file and
\gstore_binary for storing result in file with binary passing. The query
result should be one row, one column.

Usage:

SELECT image FROM accounts WHERE id = xxx
\gstore_binary ~/image.png

What do you think about this proposal?


I might be missing something, but is it different from:

\t
\a
\o output_filename
SELECT ...
\o

?

--
Alex


[HACKERS] [OSSTEST PATCH 1/1] PostgreSQL db: Retry transactions on constraint failures

2016-12-09 Thread Ian Jackson
This is unfortunate but appears to be necessary.

Signed-off-by: Ian Jackson 
CC: pgsql-hackers@postgresql.org
---
 Osstest/JobDB/Executive.pm | 45 -
 tcl/JobDB-Executive.tcl|  6 --
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/Osstest/JobDB/Executive.pm b/Osstest/JobDB/Executive.pm
index 610549a..dc6d3c2 100644
--- a/Osstest/JobDB/Executive.pm
+++ b/Osstest/JobDB/Executive.pm
@@ -62,8 +62,51 @@ sub need_retry ($$$) {
 my ($jd, $dbh,$committing) = @_;
 return
($dbh_tests->err() // 0)==7 &&
-   ($dbh_tests->state =~ m/^(?:40P01|40001)/);
+   ($dbh_tests->state =~ m/^(?:40P01|40001|23|40002)/);
 # DEADLOCK DETECTED or SERIALIZATION FAILURE
+# or any Integrity Constraint Violation including
+# TRANSACTION_INTEGRITY_CONSTRAINT_VIOLATION.
+#
+# An Integrity Constraint Violation ought not to occur with
+# serialisable transactions, so it is aways a bug.  These bugs
+# should not be retried.  However, there is a longstanding bug in
+# PostgreSQL: SERIALIZABLE's guarantee of transaction
+# serialisability only applies to successful transactions.
+# Concurrent SERIALIZABLE transactions may generate "impossible"
+# errors.  For example, doing a SELECT to ensure that a row does
+# not exist, and then inserting it, may produce a unique
+# constraint violation.
+#
+# I have not been able to find out clearly which error codes may
+# be spuriously generated.  At the very least "23505
+# UNIQUE_VIOLATION" is, but I'm not sure about others.  I am
+# making the (hopefully not unwarranted) assumption that this is
+# the only class of spurious errors.  (We don't have triggers.)
+#
+# The undesirable side effect is that a buggy transaction would be
+# retried at intervals until the retry count is reached.  But
+# there seems no way to avoid this.
+#
+# This bug may have been fixed in very recent PostgreSQL (although
+# a better promise still seems absent from the documentation, at
+# the time of writing in December 2016).  But we need to work with
+# PostgreSQL back to at least 9.1.  Perhaps in the future we can
+# make this behaviour conditional on the pgsql bug being fixed.
+#
+# References:
+#
+# "WIP: Detecting SSI conflicts before reporting constraint violations"
+# January 2016 - April 2016 on pgsql-hackers
+# 
https://www.postgresql.org/message-id/flat/CAEepm%3D2_9PxSqnjp%3D8uo1XthkDVyOU9SO3%2BOLAgo6LASpAd5Bw%40mail.gmail.com
+# (includes patch for PostgreSQL and its documentation)
+#
+# BUG #9301: INSERT WHERE NOT EXISTS on table with UNIQUE constraint in 
concurrent SERIALIZABLE transactions
+# 2014, pgsql-bugs
+# 
https://www.postgresql.org/message-id/flat/3F697CF1-2BB7-40D4-9D20-919D1A5D6D93%40apple.com
+#
+# "Working around spurious unique constraint errors due to SERIALIZABLE 
bug"
+# 2009, pgsql-general
+# 
https://www.postgresql.org/message-id/flat/D960CB61B694CF459DCFB4B0128514C203937E44%40exadv11.host.magwien.gv.at
 }
 
 sub current_flight ($) { #method
diff --git a/tcl/JobDB-Executive.tcl b/tcl/JobDB-Executive.tcl
index 62c63af..6b9bcb0 100644
--- a/tcl/JobDB-Executive.tcl
+++ b/tcl/JobDB-Executive.tcl
@@ -365,8 +365,10 @@ proc transaction {tables script {autoreconnect 0}} {
if {$rc} {
switch -glob $errorCode {
{OSSTEST-PSQL * 40P01} -
-   {OSSTEST-PSQL * 40001} {
-   # DEADLOCK DETECTED or SERIALIZATION FAILURE
+   {OSSTEST-PSQL * 40001} -
+   {OSSTEST-PSQL * 23*}   -
+   {OSSTEST-PSQL * 40002} {
+   # See Osstest/JobDB/Executive.pm:need_retry
logputs stdout \
  "transaction serialisation failure ($errorCode) ($result) retrying ..."
if {$dbopen} { db-execute ROLLBACK }
-- 
2.1.4



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


[HACKERS] [OSSTEST PATCH 0/1] PostgreSQL db: Retry on constraint violation

2016-12-09 Thread Ian Jackson
Hi.  This message is going to xen-devel (because that's where the
osstest project is) and to pgsql-hackers (because I hope they may be
able to advise about the scope of the PostgreSQL SERIALIZABLE
constraint problem).

In summary: PostgreSQL only provides transaction serialisability for
successful transactions.  Even with SERIALIZABLE, transactions may
fail due to spurious and "impossible" constraint violations.

As a result, I need to make osstest retry transactions not only on
explicitly reported serialisation failures and deadlocks, but also on
integrity violations.

It is not clear to me from the thread
  WIP: Detecting SSI conflicts before reporting constraint violations
which was on pgsql-hackers earlier this year, whether it is only
unique constraint violations which may spuriously occur.

Can anyone from the PostgreSQL hacker community advise ?  The existing
documentation patch just talks about "successful" transactions, and
uses unique constraints as an example.  In principle this leaves open
the possibility that the transaction might fail with bizarre and
unpredictable error codes, although I hope this isn't possible.

It would be good to narrow the scope of the retries in my system as
much as possible.

I'm hoping to get an authoritative answer here but it seems that this
is a common problem to which there is still not yet a definitive
solution.  I would like there to be a definitive solution.

If I get a clear answer I'll submit a further docs patch to pgsql :-).

Thanks,
Ian.


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


Re: [HACKERS] new table partitioning breaks \d table to older versions

2016-12-09 Thread Amit Langote
On Sat, Dec 10, 2016 at 2:59 AM, Jeff Janes  wrote:
> Since:
>
> commit f0e44751d7175fa3394da2c8f85e3ceb3cdbfe63
> Author: Robert Haas 
> Date:   Wed Dec 7 13:17:43 2016 -0500
>
> Implement table partitioning.
>
> If I use psql compiled from 10devel to connect to a 9.6.1 server, then \d
> fails:
>
> psql (10devel-f0e4475, server 9.6.1-16e7c02)
> Type "help" for help.
>
>
> # \d pgbench_accounts
> ERROR:  column c.relpartbound does not exist
> LINE 1: ...ELECT inhparent::pg_catalog.regclass, pg_get_expr(c.relpartb...

Oops, server version check was added to the relevant code for pg_dump,
but not psql...  Will send a patch soon.  Thanks for catching it.

Thanks,
Amit


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


Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

2016-12-09 Thread Keith Fiske
On Fri, Dec 9, 2016 at 1:13 PM, Amit Langote 
wrote:

> Hi Keith,
>
> On Sat, Dec 10, 2016 at 3:00 AM, Keith Fiske  wrote:
> > Being that table partitioning is something I'm slightly interested in,
> > figured I'd give it a whirl.
> >
> > This example in the docs has an extraneous comma after the second column
> >
> > CREATE TABLE cities (
> > name text not null,
> > population   int,
> > ) PARTITION BY LIST (initcap(name));
> >
> > And the WITH OPTIONS clause does not appear to be working using another
> > example from the docs. Not seeing any obvious typos.
> >
> > keith@keith=# CREATE TABLE measurement_y2016m07
> > keith-# PARTITION OF measurement (
> > keith(# unitsales WITH OPTIONS DEFAULT 0
> > keith(# ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
> > 2016-12-09 12:51:48.728 EST [11711] ERROR:  syntax error at or near
> "WITH"
> > at character 80
> > 2016-12-09 12:51:48.728 EST [11711] STATEMENT:  CREATE TABLE
> > measurement_y2016m07
> > PARTITION OF measurement (
> > unitsales WITH OPTIONS DEFAULT 0
> > ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
> > ERROR:  syntax error at or near "WITH"
> > LINE 3: unitsales WITH OPTIONS DEFAULT 0
> >   ^
> > Time: 0.184 ms
> >
> > Removing the unit_sales default allows it to work fine
>
> WITH OPTIONS keyword phrase is something that was made redundant in
> the last version of the patch, but I forgot to remove the same in the
> example.  I've sent a doc patch to fix that.
>
> If you try - unitsales DEFAULT 0, it will work.  Note that I did not
> specify WITH OPTIONS.
>
> Thanks,
> Amit
>

That works. Thanks!

keith@keith=# CREATE TABLE measurement_y2016m07
PARTITION OF measurement (
unitsales DEFAULT 0
) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
CREATE TABLE
Time: 4.091 ms


Re: [HACKERS] new table partitioning breaks \d table to older versions

2016-12-09 Thread Fabrízio de Royes Mello
On Fri, Dec 9, 2016 at 3:59 PM, Jeff Janes  wrote:
>
> Since:
>
> commit f0e44751d7175fa3394da2c8f85e3ceb3cdbfe63
> Author: Robert Haas 
> Date:   Wed Dec 7 13:17:43 2016 -0500
>
> Implement table partitioning.
>
> If I use psql compiled from 10devel to connect to a 9.6.1 server, then \d
fails:
>
> psql (10devel-f0e4475, server 9.6.1-16e7c02)
> Type "help" for help.
>
>
> # \d pgbench_accounts
> ERROR:  column c.relpartbound does not exist
> LINE 1: ...ELECT inhparent::pg_catalog.regclass, pg_get_expr(c.relpartb...
>

Looks like they forgot to adjust version check number in describe.c code.

Attched patch fix it.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index f0d955b..a582a37 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1808,7 +1808,7 @@ describeOneTableDetails(const char *schemaname,
 	}
 
 	/* Make footers */
-	if (pset.sversion >= 90600)
+	if (pset.sversion >= 10)
 	{
 		/* Get the partition information  */
 		PGresult   *result;

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


Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

2016-12-09 Thread Amit Langote
Hi Keith,

On Sat, Dec 10, 2016 at 3:00 AM, Keith Fiske  wrote:
> Being that table partitioning is something I'm slightly interested in,
> figured I'd give it a whirl.
>
> This example in the docs has an extraneous comma after the second column
>
> CREATE TABLE cities (
> name text not null,
> population   int,
> ) PARTITION BY LIST (initcap(name));
>
> And the WITH OPTIONS clause does not appear to be working using another
> example from the docs. Not seeing any obvious typos.
>
> keith@keith=# CREATE TABLE measurement_y2016m07
> keith-# PARTITION OF measurement (
> keith(# unitsales WITH OPTIONS DEFAULT 0
> keith(# ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
> 2016-12-09 12:51:48.728 EST [11711] ERROR:  syntax error at or near "WITH"
> at character 80
> 2016-12-09 12:51:48.728 EST [11711] STATEMENT:  CREATE TABLE
> measurement_y2016m07
> PARTITION OF measurement (
> unitsales WITH OPTIONS DEFAULT 0
> ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
> ERROR:  syntax error at or near "WITH"
> LINE 3: unitsales WITH OPTIONS DEFAULT 0
>   ^
> Time: 0.184 ms
>
> Removing the unit_sales default allows it to work fine

WITH OPTIONS keyword phrase is something that was made redundant in
the last version of the patch, but I forgot to remove the same in the
example.  I've sent a doc patch to fix that.

If you try - unitsales DEFAULT 0, it will work.  Note that I did not
specify WITH OPTIONS.

Thanks,
Amit


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


Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.

2016-12-09 Thread Keith Fiske
On Wed, Dec 7, 2016 at 1:30 PM, Robert Haas  wrote:

> On Wed, Dec 7, 2016 at 1:20 PM, Robert Haas  wrote:
> > Implement table partitioning.
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Being that table partitioning is something I'm slightly interested in,
figured I'd give it a whirl.

This example in the docs has an extraneous comma after the second column

CREATE TABLE cities (
name text not null,
population   int,
) PARTITION BY LIST (initcap(name));

And the WITH OPTIONS clause does not appear to be working using another
example from the docs. Not seeing any obvious typos.

keith@keith=# CREATE TABLE measurement_y2016m07
keith-# PARTITION OF measurement (
keith(# unitsales WITH OPTIONS DEFAULT 0
keith(# ) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
2016-12-09 12:51:48.728 EST [11711] ERROR:  syntax error at or near "WITH"
at character 80
2016-12-09 12:51:48.728 EST [11711] STATEMENT:  CREATE TABLE
measurement_y2016m07
PARTITION OF measurement (
unitsales WITH OPTIONS DEFAULT 0
) FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
ERROR:  syntax error at or near "WITH"
LINE 3: unitsales WITH OPTIONS DEFAULT 0
  ^
Time: 0.184 ms

Removing the unit_sales default allows it to work fine

keith@keith=# CREATE TABLE measurement_y2016m07
PARTITION OF measurement
 FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');
CREATE TABLE
Time: 5.001 ms


[HACKERS] new table partitioning breaks \d table to older versions

2016-12-09 Thread Jeff Janes
Since:

commit f0e44751d7175fa3394da2c8f85e3ceb3cdbfe63
Author: Robert Haas 
Date:   Wed Dec 7 13:17:43 2016 -0500

Implement table partitioning.

If I use psql compiled from 10devel to connect to a 9.6.1 server, then \d
fails:

psql (10devel-f0e4475, server 9.6.1-16e7c02)
Type "help" for help.


# \d pgbench_accounts
ERROR:  column c.relpartbound does not exist
LINE 1: ...ELECT inhparent::pg_catalog.regclass, pg_get_expr(c.relpartb...


Cheers,

Jeff


Re: [HACKERS] jsonb problematic operators

2016-12-09 Thread Andres Freund
On 2016-12-09 12:17:32 -0500, Robert Haas wrote:
> As Geoff says, you don't have to use the operators; you could use the
> equivalent functions instead.  Every operator just gets turned into a
> function call internally, so this is always possible.

Well, except that only operators support indexing :(

- Andres


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


Re: [HACKERS] jsonb problematic operators

2016-12-09 Thread David G. Johnston
On Fri, Dec 9, 2016 at 10:17 AM, Robert Haas  wrote:

>
> As Geoff says, you don't have to use the operators; you could use the
> equivalent functions instead.  Every operator just gets turned into a
> function call internally, so this is always possible.
>

In most cases - the decision to tie indexing to operators makes this
imperfect.  Whether there is an actual problem with these operators I am
unsure.

David J.


[HACKERS] proposal: psql statements \gstore \gstore_binary (instead COPY RAW)

2016-12-09 Thread Pavel Stehule
Hi

Long time I am pushing a COPY RAW - without success.

Now I propose functionally similar solution - reduced to only to psql
console

Now we have a statement \g for execution query, \gset for exec and store
result in memory and I propose \gstore for storing result in file and
\gstore_binary for storing result in file with binary passing. The query
result should be one row, one column.

Usage:

SELECT image FROM accounts WHERE id = xxx
\gstore_binary ~/image.png

What do you think about this proposal?

Regards

Pavel


Re: [HACKERS] jsonb problematic operators

2016-12-09 Thread Merlin Moncure
On Fri, Dec 9, 2016 at 5:50 AM, Jordan Gigov  wrote:
> There is this problem with the jsonb operators "? text" "?| text[]"
> and "?& text[]" that the question mark is typically used for prepared
> statement parameters in the most used abstraction APIs in Java and
> PHP.
>
> This really needs an alternative. Something like "HAS text", "HAS
> ANY(text[])" and "HAS ALL(text[])" same as regular array usage. It
> probably should be another word that has less chance of becoming a
> conflict with another operator in future SQL specifications, but
> that's for you to decide.
>
> It's not a good idea to expect everyone else to make for workarounds
> for problems you choose to create.

You are griping in the wrong place.  "everyone else" has reserved
characters for its own use that were not allowed to be reserved
without a clean escaping mechanism -- hibernate does this, for example
reserving ':'  which is used in many places within SQL.

Typically when you embed special characters in strings designed to be
processed by something else you allow for that character to be
directly.  In the computer science world we generally call this
escaping strings and it a very common and well understood practice.
For some odd reason however the authors of java various frameworks
seem to be impervious to the utility of the concept.

merlin


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


Re: [HACKERS] jsonb problematic operators

2016-12-09 Thread Robert Haas
On Fri, Dec 9, 2016 at 6:50 AM, Jordan Gigov  wrote:
> It's not a good idea to expect everyone else to make for workarounds
> for problems you choose to create.

True.  I actually kinda agree that the use of ? wasn't a great choice
here, precisely because a number of drivers do use it to indicate a
placeholder.  However, I also think that it was done without realizing
that it was going to create problems.  Your phrasing implies that we
did that on purpose just to mess with users, which isn't true.

As Geoff says, you don't have to use the operators; you could use the
equivalent functions instead.  Every operator just gets turned into a
function call internally, so this is always possible.

It would also be smart for driver authors who use ? to indicate a
placeholder to also provide some way of escaping it.  There are plenty
of perfectly valid PostgreSQL queries that include a ? as something
other than a driver-interpreted placeholder, and if driver authors
have failed to foresee that, it's not entirely our fault.

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


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


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

2016-12-09 Thread Alvaro Herrera
Daniel Verite wrote:

> My tests are OK too but I see an issue with the code in
> enlargeStringInfo(), regarding integer overflow.
> The bit of comment that says:
> 
>   Note we are assuming here that limit <= INT_MAX/2, else the above
>   loop could overflow.
> 
> is obsolete, it's now INT_MAX instead of INT_MAX/2.

Hmm, I think what it really needs to say there is UINT_MAX/2, which is
what we really care about.  I may be all wet, but what I see is that the
expression
(((Size) 1) << (sizeof(int32) * 8 - 1)) - 1
which is what we use as limit is exactly half the unsigned 32-bit
integer range.  So I would just update the constant in that comment
instead of removing it completely.  (We're still relying on the loop not
to overflow in 32-bit machines, surely).

> There's a related problem here:
>   newlen = 2 * str->maxlen;
>   while (needed > newlen)
>   newlen = 2 * newlen;
> str->maxlen is an int going up to INT_MAX so [2 * str->maxlen] now
> *will* overflow when [str->maxlen > INT_MAX/2].
> Eventually it somehow works because of this:
>   if (newlen > limit)
>   newlen = limit;
> but newlen is wonky (when resulting from int overflow)
> before being brought back to limit.

Yeah, this is bogus and your patch looks correct to me.

I propose this:

diff --git a/src/backend/lib/stringinfo.c b/src/backend/lib/stringinfo.c
index b618b37..a1d786d 100644
--- a/src/backend/lib/stringinfo.c
+++ b/src/backend/lib/stringinfo.c
@@ -313,13 +313,13 @@ enlargeStringInfo(StringInfo str, int needed)
 * for efficiency, double the buffer size each time it overflows.
 * Actually, we might need to more than double it if 'needed' is big...
 */
-   newlen = 2 * str->maxlen;
+   newlen = 2 * (Size) str->maxlen;
while (needed > newlen)
newlen = 2 * newlen;
 
/*
 * Clamp to the limit in case we went past it.  Note we are assuming 
here
-* that limit <= INT_MAX/2, else the above loop could overflow.  We will
+* that limit <= UINT_MAX/2, else the above loop could overflow.  We 
will
 * still have newlen >= needed.
 */
if (newlen > limit)

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


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


Re: [HACKERS] Logical Replication WIP

2016-12-09 Thread Peter Eisentraut
Here is a "fixup" patch for
0002-Add-PUBLICATION-catalogs-and-DDL-v11.patch.gz with some minor fixes.

Two issues that should be addressed:

1. I think ALTER PUBLICATION does not need to require CREATE privilege
on the database.  That should be easy to change.

2. By requiring only SELECT privilege to include a table in a
publication, someone could include a table without replica identity into
a publication and thus prevent updates to the table.

A while ago I had been working on a patch to create a new PUBLICATION
privilege for this purpose.  I have attached the in-progress patch here.
 We could either finish that up and include it, or commit your patch
initially with requiring superuser and then refine the permissions later.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 38aba08eb00ffc0b1f0d52a38864f825435a1694 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 8 Dec 2016 12:00:00 -0500
Subject: [PATCH] fixup! Add PUBLICATION catalogs and DDL

---
 doc/src/sgml/catalogs.sgml| 35 ++-
 doc/src/sgml/ref/alter_publication.sgml   | 41 +--
 doc/src/sgml/ref/create_publication.sgml  | 46 +--
 doc/src/sgml/ref/drop_publication.sgml|  6 ++--
 doc/src/sgml/ref/psql-ref.sgml|  6 ++--
 src/backend/catalog/Makefile  |  2 +-
 src/backend/catalog/dependency.c  |  4 +--
 src/backend/catalog/objectaddress.c   | 25 +++--
 src/backend/catalog/pg_publication.c  | 27 ++
 src/backend/commands/publicationcmds.c| 12 
 src/backend/commands/tablecmds.c  |  9 +++---
 src/backend/parser/gram.y |  2 +-
 src/backend/utils/cache/relcache.c|  2 +-
 src/backend/utils/cache/syscache.c|  2 +-
 src/bin/pg_dump/pg_dump.c |  3 --
 src/bin/psql/tab-complete.c   |  8 +++---
 src/test/regress/expected/publication.out | 22 +++
 17 files changed, 115 insertions(+), 137 deletions(-)

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index aacd4bcb23..5213aa4f5e 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -5149,12 +5149,11 @@ pg_publication
   
 
   
-   The pg_publication catalog contains
-   all publications created in the database.
+   The catalog pg_publication contains all
+   publications created in the database.
   
 
   
-
pg_publication Columns
 

@@ -5179,7 +5178,7 @@ pg_publication Columns
   pubname
   Name
   
-  A unique, database-wide identifier for the publication.
+  Name of the publication
  
 
  
@@ -5202,26 +5201,25 @@ pg_publication Columns
   pubinsert
   bool
   
-  If true, INSERT operations are replicated for tables in the
-   publication.
+  If true, INSERT operations are replicated for
+   tables in the publication.
  
 
  
   pubupdate
   bool
   
-  If true, UPDATE operations are replicated for tables in the
-   publication.
+  If true, UPDATE operations are replicated for
+   tables in the publication.
  
 
  
   pubdelete
   bool
   
-  If true, DELETE operations are replicated for tables in the
-   publication.
+  If true, DELETE operations are replicated for
+   tables in the publication.
  
-
 

   
@@ -5235,13 +5233,12 @@ pg_publication_rel
   
 
   
-   The pg_publication_rel catalog contains
-   mapping between tables and publications in the database. This is many to
-   many mapping.
+   The catalog pg_publication_rel contains the
+   mapping between relations and publications in the database.  This is a
+   many-to-many mapping.
   
 
   
-
pg_publication_rel Columns
 

@@ -5255,21 +5252,19 @@ pg_publication_rel Columns
 
 
 
-
  
   prpubid
   oid
   pg_publication.oid
-  Publication reference.
+  Reference to publication
  
 
  
   prrelid
-  bool
+  oid
   pg_class.oid
-  Relation reference.
+  Reference to relation
  
-
 

   
diff --git a/doc/src/sgml/ref/alter_publication.sgml b/doc/src/sgml/ref/alter_publication.sgml
index c17666c97f..eb8cb07126 100644
--- a/doc/src/sgml/ref/alter_publication.sgml
+++ b/doc/src/sgml/ref/alter_publication.sgml
@@ -40,21 +40,20 @@ Description
 
   
The first variant of this command listed in the synopsis can change
-   all of the publication attributes specified in
-   .
-   Attributes not mentioned in the command retain their previous settings.
-   Database superusers can change any of these settings for any role.
+   all of the publication properties specified in
+   .  Properties not mentioned in the
+   command retain their previous settings.  Database superusers can change any
+   of these settings for any role.
   
 
   

Re: [HACKERS] building HEAD on macos fails with #error no source of random numbers configured

2016-12-09 Thread Heikki Linnakangas

On 12/09/2016 05:43 PM, Tom Lane wrote:

Dave Cramer  writes:

Looking at src/port/pg_strong_random.c  this would be a bug in autoconf


It looks more like self-inflicted damage from here:


./configure --prefix=/usr/local/pgsql/10 --enable-debug --with-python
--with-openssl --with-libraries=/usr/local/opt/openssl/lib
--with-includes=/usr/local/opt/openssl/include/ --no-create --no-recursion


Why are you using either --no-create or --no-recursion?  The former
*definitely* breaks things:

$ ./configure --help | grep create
  -n, --no-create do not create output files

Presumably the proximate cause of that error message is that configure
hasn't updated pg_config.h from some ancient version thereof, as a
consequence of this switch.

I'm not sure what --no-recursion does, but I would say that we'd
consider that unsupported as well.


Interesting. Running config.status adds those --no-create --no-recursion 
flags automatically. You can see them in the command-line at the top of 
config.log, too. I never bothered to check what they do...


- Heikki



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


Re: [HACKERS] Fix for segfault in plpython's exception handling

2016-12-09 Thread Tom Lane
Rafa de la Torre  writes:
>   exc = PyErr_NewException(exception_map[i].name, base, dict);
> + Py_INCREF(exc);
>   PyModule_AddObject(mod, exception_map[i].classname, exc);

Hm.  Seems like if this is a problem, the code for the other three
exceptions is being a bit careless: it does do Py_INCREFs on them,
but not soon enough to ensure no problems.  Also, I wonder why that
code checks for a null result from PyErr_NewException but this doesn't.

Good catch though.  A naive person would have assumed that
PyModule_AddObject would increment the object's refcount, but
the Python docs say "This steals a reference to value", which
I guess must mean that the caller is required to do it.

regards, tom lane


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


Re: [HACKERS] jsonb problematic operators

2016-12-09 Thread David G. Johnston
On Fri, Dec 9, 2016 at 4:50 AM, Jordan Gigov  wrote:

> There is this problem with the jsonb operators "? text" "?| text[]"
> and "?& text[]" that the question mark is typically used for prepared
> statement parameters in the most used abstraction APIs in Java and
> PHP.
>

​Unfortunately true.  These APIs made a poor decision in taking a very
useful query operator character, the question mark, and turning it into a
position-important placeholder for query parameters.  Using "$1, $2, $3,
etc..." is a much better design since you have fewer things to worry about
if you modify the query and add (or want to reuse) a parameter.

Given that PostgreSQL already choose to go with the better designed API
here the fact that the relatively new JSON feature allows "?" to be used as
an operator character makes perfect sense.


>
> This really needs an alternative. Something like "HAS text", "HAS
> ANY(text[])" and "HAS ALL(text[])" same as regular array usage. It
> probably should be another word that has less chance of becoming a
> conflict with another operator in future SQL specifications, but
> that's for you to decide.
>

​I don't think making it a keyword is a good idea, or possible via
extension, but otherwise you are free to create custom operators (and
related infrastructure) if this bothers you enough.  Such a "JDBC
compatability extension" would probably be welcomed by the community.​


> It's not a good idea to expect everyone else to make for workarounds
> for problems you choose to create.
>

The choosing was for a superior, and internally consistent, design.  While
I see the problem you bring up I don't see introducing yet another set of
alternative operators to core.  But as mentioned one of the advantages of
PostgreSQL is its extensability and hopefully someone will enjoy working
with Open Source PostgreSQL in their affected language enough to consider
doing something about it.

David J.


Re: [HACKERS] building HEAD on macos fails with #error no source of random numbers configured

2016-12-09 Thread Dave Cramer
That will teach me to copy and paste a config from somewhere ...

Thanks

Dave Cramer

On 9 December 2016 at 10:43, Tom Lane  wrote:

> Dave Cramer  writes:
> > Looking at src/port/pg_strong_random.c  this would be a bug in autoconf
>
> It looks more like self-inflicted damage from here:
>
> > ./configure --prefix=/usr/local/pgsql/10 --enable-debug --with-python
> > --with-openssl --with-libraries=/usr/local/opt/openssl/lib
> > --with-includes=/usr/local/opt/openssl/include/ --no-create
> --no-recursion
>
> Why are you using either --no-create or --no-recursion?  The former
> *definitely* breaks things:
>
> $ ./configure --help | grep create
>   -n, --no-create do not create output files
>
> Presumably the proximate cause of that error message is that configure
> hasn't updated pg_config.h from some ancient version thereof, as a
> consequence of this switch.
>
> I'm not sure what --no-recursion does, but I would say that we'd
> consider that unsupported as well.
>
> regards, tom lane
>


Re: [HACKERS] `array_position...()` causes SIGSEGV

2016-12-09 Thread Alvaro Herrera
Junseok Yang wrote:
> Hello hackers,
> 
> I met SIGSEGV when using `array_position()` with record type
> arguments, so I've written a patch which corrects this problem. It
> seems that `array_position...()` sets wrong memory context for the
> cached function (in this case `record_eq()`) which is used to find a
> matching element.

Looks correct to me, so pushed to all affected branches.

> The problem is reproducable with the following query.
> 
> SELECT array_position(ids, (1, 1))
> FROM (VALUES (ARRAY[(0, 0)]), (ARRAY[(1, 1)])) AS _(ids);

I used this as a new regression test.

Thanks for the report and patch!

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


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


Re: [HACKERS] building HEAD on macos fails with #error no source of random numbers configured

2016-12-09 Thread Tom Lane
Dave Cramer  writes:
> Looking at src/port/pg_strong_random.c  this would be a bug in autoconf

It looks more like self-inflicted damage from here:

> ./configure --prefix=/usr/local/pgsql/10 --enable-debug --with-python
> --with-openssl --with-libraries=/usr/local/opt/openssl/lib
> --with-includes=/usr/local/opt/openssl/include/ --no-create --no-recursion

Why are you using either --no-create or --no-recursion?  The former
*definitely* breaks things:

$ ./configure --help | grep create
  -n, --no-create do not create output files

Presumably the proximate cause of that error message is that configure
hasn't updated pg_config.h from some ancient version thereof, as a
consequence of this switch.

I'm not sure what --no-recursion does, but I would say that we'd
consider that unsupported as well.

regards, tom lane


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


Re: [HACKERS] Re: [COMMITTERS] pgsql: Permit dump/reload of not-too-large >1GB tuples

2016-12-09 Thread Alvaro Herrera
Alvaro Herrera wrote:
> Tom Lane wrote:
> > Alvaro Herrera  writes:
> > > Permit dump/reload of not-too-large >1GB tuples
> > 
> > I apologize for not having paid close enough attention earlier, but:
> > this patch is absolutely unacceptable for the back branches and MUST
> > be reverted there.  Adding another field to StringInfoData is an ABI
> > change that will certainly break large numbers of extensions.  We
> > can't expect those to get recompiled for minor releases.
> 
> Oh, I see the problem now -- it can (and frequently is) stack allocated,
> not just palloc'ed using the StringInfo api, and changing the size
> breaks that.  Rats.  I'll revert tomorrow.

Done.

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


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


Re: [HACKERS] tzdata 2016j

2016-12-09 Thread Alvaro Herrera
David Fetter wrote:
> On Tue, Dec 06, 2016 at 10:52:47AM -0500, Tom Lane wrote:
> > Vladimir Borodin  writes:
> > > Any chance to get tzdata 2016j in supported branches?
> > 
> > When the next scheduled releases come around (February), we'll update
> > to whatever tzdata is current at that time.
> 
> I'm guessing that request came through because Vladimir is actually
> affected by the change.
> 
> Apparently, you can replace the tzdata file and restart the server per
> https://wiki.postgresql.org/wiki/FAQ#Will_PostgreSQL_handle_recent_daylight_saving_time_changes_in_various_countries.3F

I think a better way to handle this is to use configure's
--with-system-tzdata and make sure the underlying system's tzdata
packages are up to date.  Then you don't need to worry about Postgres'
tzdata files at all.

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


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


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

2016-12-09 Thread Alvaro Herrera
Alvaro Herrera wrote:
> 
> I have now pushed this to 9.5, 9.6 and master.  It could be backpatched
> to 9.4 with ease (just a small change in heap_form_tuple); anything
> further back would require much more effort.

I had to revert this on 9.5 and 9.6 -- it is obvious (in hindsight) that
changing StringInfoData is an ABI break, so we can't do it in back
branches; see
https://www.postgresql.org/message-id/27737.1480993...@sss.pgh.pa.us
The patch still remains in master, with the bugs you pointed out.  I
suppose if somebody is desperate about getting data out from a table
with large tuples, they'd need to use pg10's pg_dump for it.

We could use the highest-order bit in StringInfoData->maxlen to
represent the boolean flag instead, if we really cared.  But I'm not
going to sweat over it ...

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


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


[HACKERS] Fix for segfault in plpython's exception handling

2016-12-09 Thread Rafa de la Torre
The patch attached (a one-liner) includes information about how to
reproduce the issue and why it happens.

We applied it to our production servers (9.5) and has been working as
expected for a while. I've also checked that it can be applied cleanly to
current master.

Could it make its way into master? maybe 9.5 and 9.6 as well?

Thanks!

-- 
Rafa de la Torre
rto...@carto.com
From e95649d8d173f483d67d9338a9089947a7667e5f Mon Sep 17 00:00:00 2001
From: Rafa de la Torre 
Date: Wed, 23 Nov 2016 10:50:40 +0100
Subject: [PATCH] Fix segfault in plpython's exception handling

When a subtransaction is aborted in plpython because of an SPI
exception, it tries to find a matching python exception in a hash
`PLy_spi_exceptions` and to make python vm raise it.

That hash is generated during module initialization, but the exception
objects are not marked to prevent the garbage collector from collecting
them, which can lead to a segmentation fault when processing any SPI
exception.

PoC to reproduce the issue:

```sql
CREATE OR REPLACE FUNCTION server_crashes()
RETURNS VOID
AS $$
import gc
gc.collect()
plan = plpy.prepare('SELECT raises_an_spi_exception();', [])
plpy.execute(plan)
$$ LANGUAGE plpythonu;

CREATE OR REPLACE FUNCTION raises_an_spi_exception()
RETURNS VOID
AS $$
DECLARE
  sql TEXT;
BEGIN
  sql = format('%I', NULL); -- NullValueNotAllowed
END
$$ LANGUAGE plpgsql;

SELECT server_crashes(); -- segfault here
```

Stacktrace of the problem (using PostgreSQL `REL9_5_STABLE` and python
`2.7.3-0ubuntu3.8` on a Ubuntu 12.04):

```
 Program received signal SIGSEGV, Segmentation fault.
 0x7f3155c7670b in PyObject_Call (func=0x7f31b7db2a30, arg=0x7f31b87d17d0, kw=0x0) at ../Objects/abstract.c:2525
 2525../Objects/abstract.c: No such file or directory.
 (gdb) bt
 #0  0x7f3155c7670b in PyObject_Call (func=0x7f31b7db2a30, arg=0x7f31b87d17d0, kw=0x0) at ../Objects/abstract.c:2525
 #1  0x7f3155d81ab1 in PyEval_CallObjectWithKeywords (func=0x7f31b7db2a30, arg=0x7f31b87d17d0, kw=0x0) at ../Python/ceval.c:3890
 #2  0x7f3155c766ed in PyObject_CallObject (o=0x7f31b7db2a30, a=0x7f31b87d17d0) at ../Objects/abstract.c:2517
 #3  0x7f31561e112b in PLy_spi_exception_set (edata=0x7f31b8743d78, excclass=0x7f31b7db2a30) at plpy_spi.c:547
 #4  PLy_spi_subtransaction_abort (oldcontext=, oldowner=) at plpy_spi.c:527
 #5  0x7f31561e2185 in PLy_spi_execute_plan (ob=0x7f31b87d0cd8, list=0x7f31b7c530d8, limit=0) at plpy_spi.c:307
 #6  0x7f31561e22d4 in PLy_spi_execute (self=, args=0x7f31b87a6d08) at plpy_spi.c:180
 #7  0x7f3155cda4d6 in PyCFunction_Call (func=0x7f31b7d29600, arg=0x7f31b87a6d08, kw=0x0) at ../Objects/methodobject.c:81
 #8  0x7f3155d82383 in call_function (pp_stack=0x7fff9207e710, oparg=2) at ../Python/ceval.c:4021
 #9  0x7f3155d7cda4 in PyEval_EvalFrameEx (f=0x7f31b8805be0, throwflag=0) at ../Python/ceval.c:2666
 #10 0x7f3155d82898 in fast_function (func=0x7f31b88b5ed0, pp_stack=0x7fff9207ea70, n=0, na=0, nk=0) at ../Python/ceval.c:4107
 #11 0x7f3155d82584 in call_function (pp_stack=0x7fff9207ea70, oparg=0) at ../Python/ceval.c:4042
 #12 0x7f3155d7cda4 in PyEval_EvalFrameEx (f=0x7f31b8805a00, throwflag=0) at ../Python/ceval.c:2666
 #13 0x7f3155d7f8a9 in PyEval_EvalCodeEx (co=0x7f31b88aa460, globals=0x7f31b8727ea0, locals=0x7f31b8727ea0, args=0x0, argcount=0, kws=0x0, kwcount=0, defs=0x0, defcount=0, closure=0x0) at ../Python/ceval.c:3253
 #14 0x7f3155d74ff4 in PyEval_EvalCode (co=0x7f31b88aa460, globals=0x7f31b8727ea0, locals=0x7f31b8727ea0) at ../Python/ceval.c:667
 #15 0x7f31561dc476 in PLy_procedure_call (kargs=kargs@entry=0x7f31561e5690 "args", vargs=, proc=0x7f31b873b2d0, proc=0x7f31b873b2d0) at plpy_exec.c:801
 #16 0x7f31561dd9c6 in PLy_exec_function (fcinfo=fcinfo@entry=0x7f31b7c1f870, proc=0x7f31b873b2d0) at plpy_exec.c:61#17 0x7f31561de9f9 in plpython_call_handler (fcinfo=0x7f31b7c1f870) at plpy_main.c:291
```
---
 src/pl/plpython/plpy_plpymodule.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/pl/plpython/plpy_plpymodule.c b/src/pl/plpython/plpy_plpymodule.c
index a44b7fb..3334739 100644
--- a/src/pl/plpython/plpy_plpymodule.c
+++ b/src/pl/plpython/plpy_plpymodule.c
@@ -258,6 +258,7 @@ PLy_generate_spi_exceptions(PyObject *mod, PyObject *base)
 		PyDict_SetItemString(dict, "sqlstate", sqlstate);
 		Py_DECREF(sqlstate);
 		exc = PyErr_NewException(exception_map[i].name, base, dict);
+		Py_INCREF(exc);
 		PyModule_AddObject(mod, exception_map[i].classname, exc);
 		entry = hash_search(PLy_spi_exceptions, _map[i].sqlstate,
 			HASH_ENTER, );

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


[HACKERS] building HEAD on macos fails with #error no source of random numbers configured

2016-12-09 Thread Dave Cramer
Looking at src/port/pg_strong_random.c  this would be a bug in autoconf

#else
/* The autoconf script should not have allowed this */
#error no source of random numbers configured

My configure line is:

./configure --prefix=/usr/local/pgsql/10 --enable-debug --with-python
--with-openssl --with-libraries=/usr/local/opt/openssl/lib
--with-includes=/usr/local/opt/openssl/include/ --no-create --no-recursion

I am using openssl-1.0.2j

Regards,

Dave Cramer


Re: [HACKERS] Do we need use more meaningful variables to replace 0 in catalog head files?

2016-12-09 Thread Peter Eisentraut
On 11/13/16 12:19 PM, Tom Lane wrote:
>> It'd also be very pg_proc specific, which isn't where I think this
>> should go..
> 
> The presumption is that we have a CREATE command for every type of
> object that we need to put into the system catalogs.  But yes, the
> other problem with this approach is that you need to do a lot more
> work per-catalog to build the converter script.  I'm not sure how
> much of that could be imported from gram.y, but I'm afraid the
> answer would be "not enough".

I'd think about converting about 75% of what is currently in the catalog
headers into some sort of built-in extension that is loaded via an SQL
script.  There are surely some details about that that would need to be
worked out, but I think that's a more sensible direction than inventing
another custom format.

I wonder how big the essential bootstrap set of pg_proc.h would be and
how manageable the file would be if it were to be reduced like that.

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


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


Re: [HACKERS] pg_background contrib module proposal

2016-12-09 Thread Robert Haas
On Wed, Nov 23, 2016 at 10:46 PM, amul sul  wrote:
> I would like to take over pg_background patch and repost for
> discussion and review.
>
> Initially Robert Haas has share this for parallelism demonstration[1]
> and abandoned later with
> summary of open issue[2] with this pg_background patch need to be
> fixed, most of them seems to be
> addressed in core except handling of type exists without binary
> send/recv functions and documentation.
> I have added handling for types that don't have binary send/recv
> functions in the attach patch and will
> work on documentation at the end.
>
> One concern with this patch is code duplication with
> exec_simple_query(), we could
> consider Jim Nasby’s patch[3] to overcome this,  but  certainly we
> will end up by complicating
> exec_simple_query() to make pg_background happy.
>
> As discussed previously[1] pg_background is a contrib module that lets
> you launch
> arbitrary command in a background worker.

It looks like this could be reworked as a client of Peter Eisentraut's
background sessions code, which I think is also derived from
pg_background:

http://archives.postgresql.org/message-id/e1c2d331-ee6a-432d-e9f5-dcf85cffa...@2ndquadrant.com

That might be good, because then we wouldn't have to maintain two
copies of the code.

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


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


Re: [HACKERS] pg_background contrib module proposal

2016-12-09 Thread Pavel Stehule
2016-12-09 13:48 GMT+01:00 Andrew Borodin :

> I've read the code and now I have more suggestions.
>
> 1. Easy one. The case of different natts of expected and acutal result
> throws same errmsg as the case of wrong types.
> I think we should assist the user more here
>
> if (natts != tupdesc->natts)
>ereport(ERROR,
>  (errcode(ERRCODE_DATATYPE_MISMATCH),
>   errmsg("remote query result rowtype does not match the
> specified FROM clause rowtype")));
>
> and here
>
>if (type_id != tupdesc->attrs[i]->atttypid)
>   ereport(ERROR,
> (errcode(ERRCODE_DATATYPE_MISMATCH),
>  errmsg("remote query result rowtype does not match the
> specified FROM clause rowtype")));
>
> 2. This code
> errhint("You may need to increase max_worker_processes.")
> Is technically hinting absolutley right.
> But this extension requires something like pg_bouncer or what is
> called "thread pool".
> You just cannot safely fire a background task because you do not know
> how many workes can be spawned. Maybe it'd be better to create 'pool'
> of workers and form a queue of queries for them?
> This will make possible start a millions of subtasks.
>

This is not easy, because pgworker is related to database, not to server.
There are servers where you can have thousands databases.

Regards

Pavel


>
> 3. About getting it to the core, not as an extension. IMO this is too
> sharp thing to place it into core, I think that it's best place is in
> contribs.
>
> Thanks for the work on such a cool and fun extension.
>
> Best regards, Andrey Borodin.
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>


Re: [HACKERS] Time to drop old-style (V0) functions?

2016-12-09 Thread Robert Haas
On Thu, Dec 8, 2016 at 5:53 PM, Andres Freund  wrote:
> I mean throwing an error.  Silently assuming V1 seems like a horrible
> idea to me.  It doesn't seem unlikely that we want to introduce a new
> call interface at some point given the runtime cost of the current one,
> and that'd just bring back the current problem.

I don't have a problem with getting rid of V0; I think it's an
annoyance.  But I think a much more interesting question is how to
redesign this interface.  I think V0 actually had the right idea in
passing arguments as C arguments rather than marshaling them in some
other data structure.  If the function doesn't need flinfo or context
or collation and takes a fixed number of argument each of which is
either strict or a pass-by-reference data type (where you can use
DatumGetPointer(NULL) to represent a NULL!) and either never returns
NULL or returns a pass-by-reference data type, then you don't need any
of this complicated fcinfo stuff.  You can just pass the arguments and
get back the result.  That's less work for both the caller (who
doesn't have to stick the arguments into an fcinfo) and the callee
(who doesn't have to fish them back out).  In the sortsupport
machinery, we've gone to some lengths to make it possible - at least
in the context of sorts - to get back to something closer to that kind
of interface.  But that interface goes to considerable trouble to
achieve that result, making it practical only for bulk operations.
It's kind of ironic, at least IMHO, that the DirectionFunctionCall
macros are anything but direct.  Each one is a non-inlined function
call that does a minimum of 8 variable assignments before actually
calling the function.

There obviously has to be some provision for functions that actually
need all the stuff we pass in the V1 calling convention, but there are
a huge number of functions that don't need any of it.

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


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


Re: [HACKERS] pg_background contrib module proposal

2016-12-09 Thread Andrew Borodin
I've read the code and now I have more suggestions.

1. Easy one. The case of different natts of expected and acutal result
throws same errmsg as the case of wrong types.
I think we should assist the user more here

if (natts != tupdesc->natts)
   ereport(ERROR,
 (errcode(ERRCODE_DATATYPE_MISMATCH),
  errmsg("remote query result rowtype does not match the
specified FROM clause rowtype")));

and here

   if (type_id != tupdesc->attrs[i]->atttypid)
  ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
 errmsg("remote query result rowtype does not match the
specified FROM clause rowtype")));

2. This code
errhint("You may need to increase max_worker_processes.")
Is technically hinting absolutley right.
But this extension requires something like pg_bouncer or what is
called "thread pool".
You just cannot safely fire a background task because you do not know
how many workes can be spawned. Maybe it'd be better to create 'pool'
of workers and form a queue of queries for them?
This will make possible start a millions of subtasks.

3. About getting it to the core, not as an extension. IMO this is too
sharp thing to place it into core, I think that it's best place is in
contribs.

Thanks for the work on such a cool and fun extension.

Best regards, Andrey Borodin.


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


Re: [HACKERS] Declarative partitioning - another take

2016-12-09 Thread Amit Langote
On Fri, Dec 9, 2016 at 3:16 PM, Venkata B Nagothi  wrote:
> Hi,
>
> I am testing the partitioning feature from the latest master and got the
> following error while loading the data -
>
> db01=# create table orders_y1993 PARTITION OF orders FOR VALUES FROM
> ('1993-01-01') TO ('1993-12-31');
> CREATE TABLE
>
> db01=# copy orders from '/data/orders-1993.csv' delimiter '|';
> ERROR:  could not read block 6060 in file "base/16384/16412": read only 0 of
> 8192 bytes
> CONTEXT:  COPY orders, line 376589:
> "9876391|374509|O|54847|1997-07-16|3-MEDIUM   |Clerk#01993|0|ithely
> regular pack"

Hmm.   Could you tell what relation the file/relfilenode 16412 belongs to?

Also, is orders_y1993 the only partition of orders?  How about \d+ orders?

Thanks,
Amit


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


Re: [HACKERS] jsonb problematic operators

2016-12-09 Thread Geoff Winkless
On 9 December 2016 at 11:50, Jordan Gigov  wrote:
> There is this problem with the jsonb operators "? text" "?| text[]"
> and "?& text[]" that the question mark is typically used for prepared
> statement parameters in the most used abstraction APIs in Java and
> PHP.
>
> This really needs an alternative. Something like "HAS text", "HAS
> ANY(text[])" and "HAS ALL(text[])" same as regular array usage. It
> probably should be another word that has less chance of becoming a
> conflict with another operator in future SQL specifications, but
> that's for you to decide.

You mean something like the jsonb_ functions ?

\df jsonb*

> It's not a good idea to expect everyone else to make for workarounds
> for problems you choose to create.

I'd say it's not a good idea to come asking questions of a mailing
list with an attitude like that, but hey, it's nearly Holidays.

Geoff


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


Re: [HACKERS] `array_position...()` causes SIGSEGV

2016-12-09 Thread Alvaro Herrera
Michael Paquier wrote:
> On Fri, Dec 9, 2016 at 3:14 PM, Junseok Yang  wrote:
> > I met SIGSEGV when using `array_position()` with record type
> > arguments, so I've written a patch which corrects this problem. It
> > seems that `array_position...()` sets wrong memory context for the
> > cached function (in this case `record_eq()`) which is used to find a
> > matching element.
> >
> > The problem is reproducable with the following query.
> >
> > SELECT array_position(ids, (1, 1))
> > FROM (VALUES (ARRAY[(0, 0)]), (ARRAY[(1, 1)])) AS _(ids);
> 
> Good catch. That's present since 13dbc7a8 and the introduction of
> array_offset(), or array_position() on HEAD, so the patch should be
> applied down to 9.5.

Thanks for CC'ing me.  Looking now.

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


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


[HACKERS] jsonb problematic operators

2016-12-09 Thread Jordan Gigov
There is this problem with the jsonb operators "? text" "?| text[]"
and "?& text[]" that the question mark is typically used for prepared
statement parameters in the most used abstraction APIs in Java and
PHP.

This really needs an alternative. Something like "HAS text", "HAS
ANY(text[])" and "HAS ALL(text[])" same as regular array usage. It
probably should be another word that has less chance of becoming a
conflict with another operator in future SQL specifications, but
that's for you to decide.

It's not a good idea to expect everyone else to make for workarounds
for problems you choose to create.


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


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

2016-12-09 Thread Michael Paquier
On Fri, Dec 09, 2016 at 11:51:45AM +0200, Heikki Linnakangas wrote:
> On 12/09/2016 05:58 AM, Michael Paquier wrote:
> > 
> > One thing is: when do we look up at pg_authid? After receiving the
> > first message from client or before beginning the exchange? As the
> > first message from client has the user name, it would make sense to do
> > the lookup after receiving it, but from PG prospective it would just
> > make sense to use the data already present in the startup packet. The
> > current patch does the latter. What do you think?
> 
> While hacking on this, I came up with the attached refactoring, against
> current master. I think it makes the current code more readable, anyway, and
> it provides a get_role_password() function that SCRAM can use, to look up
> the stored password. (This is essentially the same refactoring that was
> included in the SCRAM patch set, that introduced the get_role_details()
> function.)
> 
> Barring objections, I'll go ahead and commit this first.

Here are some comments.

> @@ -720,12 +721,16 @@ CheckMD5Auth(Port *port, char **logdetail)
>   sendAuthRequest(port, AUTH_REQ_MD5, md5Salt, 4);
>  
>   passwd = recv_password_packet(port);
> -
>   if (passwd == NULL)
>   return STATUS_EOF;  /* client wouldn't send 
> password */

This looks like useless noise.

> - shadow_pass = TextDatumGetCString(datum);
> + *shadow_pass = TextDatumGetCString(datum);
>  
>   datum = SysCacheGetAttr(AUTHNAME, roleTup,
>   
> Anum_pg_authid_rolvaliduntil, );
> @@ -83,100 +83,146 @@ md5_crypt_verify(const char *role, char *client_pass,
>   {
>   *logdetail = psprintf(_("User \"%s\" has an empty password."),
> role);
> + *shadow_pass = NULL;
>   return STATUS_ERROR;/* empty password */
>   }

Here the password is allocated by text_to_cstring(), that's only 1 byte
but it should be free()'d.
-- 
Michael


signature.asc
Description: PGP signature


Re: [HACKERS] Declarative partitioning - another take

2016-12-09 Thread Maksim Milyutin

Hi, everyone!


08.12.16 18:25, Robert Haas wrote:

Of course, this is the beginning, not the end.  I've been thinking
about next steps -- here's an expanded list:

- more efficient plan-time partition pruning (constraint exclusion is too slow)
- run-time partition pruning
- partition-wise join (Ashutosh Bapat is already working on this)
- try to reduce lock levels
- hash partitioning
- the ability to create an index on the parent and have all of the
children inherit it; this should work something like constraint
inheritance.  you could argue that this doesn't add any real new
capability but it's a huge usability feature.
- teaching autovacuum enough about inheritance hierarchies for it to
update the parent statistics when they get stale despite the lack of
any actual inserts/updates/deletes to the parent.  this has been
pending for a long time, but it's only going to get more important
- row movement (aka avoiding the need for an ON UPDATE trigger on each
partition)
- insert (and eventually update) tuple routing for foreign partitions
- not scanning the parent
- fixing the insert routing so that we can skip tuple conversion where possible
- fleshing out the documentation


I would like to work on two tasks:
 - insert (and eventually update) tuple routing for foreign partition.
 - the ability to create an index on the parent and have all of the 
children inherit it;


The first one has been implemented in pg_pathman somehow, but the code 
relies on dirty hacks, so the FDW API has to be improved. As for the 
extended index support, it doesn't look like a super-hard task.


--
Maksim Milyutin
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company



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


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

2016-12-09 Thread Heikki Linnakangas

On 12/09/2016 05:58 AM, Michael Paquier wrote:


One thing is: when do we look up at pg_authid? After receiving the
first message from client or before beginning the exchange? As the
first message from client has the user name, it would make sense to do
the lookup after receiving it, but from PG prospective it would just
make sense to use the data already present in the startup packet. The
current patch does the latter. What do you think?


While hacking on this, I came up with the attached refactoring, against 
current master. I think it makes the current code more readable, anyway, 
and it provides a get_role_password() function that SCRAM can use, to 
look up the stored password. (This is essentially the same refactoring 
that was included in the SCRAM patch set, that introduced the 
get_role_details() function.)


Barring objections, I'll go ahead and commit this first.

- Heikki

>From 30be98cf09e8807d477827257a1e55c979dbe877 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas 
Date: Fri, 9 Dec 2016 11:49:36 +0200
Subject: [PATCH 1/1] Refactor the code for verifying user's password.

Split md5_crypt_verify() into three functions:
* get_role_password() to fetch user's password from pg_authid, and check
  its expiration.
* md5_crypt_verify() to check an MD5 authentication challenge
* plain_crypt_verify() to check a plaintext password.

get_role_password() will be needed as a separate function by the upcoming
SCRAM authentication patch set. Most of the remaining functionality in
md5_crypt_verify() was different for MD5 and plaintext authentication, so
split that for readability.

While we're at it, simplify the *_crypt_verify functions by using
stack-allocated buffers to hold the temporary MD5 hashes, instead of
pallocing.
---
 src/backend/libpq/auth.c  |  18 +++-
 src/backend/libpq/crypt.c | 214 --
 src/include/libpq/crypt.h |   9 +-
 3 files changed, 151 insertions(+), 90 deletions(-)

diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index f8bffe3..5c9ee06 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -707,6 +707,7 @@ CheckMD5Auth(Port *port, char **logdetail)
 {
 	char		md5Salt[4];		/* Password salt */
 	char	   *passwd;
+	char	   *shadow_pass;
 	int			result;
 
 	if (Db_user_namespace)
@@ -720,12 +721,16 @@ CheckMD5Auth(Port *port, char **logdetail)
 	sendAuthRequest(port, AUTH_REQ_MD5, md5Salt, 4);
 
 	passwd = recv_password_packet(port);
-
 	if (passwd == NULL)
 		return STATUS_EOF;		/* client wouldn't send password */
 
-	result = md5_crypt_verify(port->user_name, passwd, md5Salt, 4, logdetail);
+	result = get_role_password(port->user_name, _pass, logdetail);
+	if (result == STATUS_OK)
+		result = md5_crypt_verify(port->user_name, shadow_pass, passwd,
+  md5Salt, 4, logdetail);
 
+	if (shadow_pass)
+		pfree(shadow_pass);
 	pfree(passwd);
 
 	return result;
@@ -741,16 +746,21 @@ CheckPasswordAuth(Port *port, char **logdetail)
 {
 	char	   *passwd;
 	int			result;
+	char	   *shadow_pass;
 
 	sendAuthRequest(port, AUTH_REQ_PASSWORD, NULL, 0);
 
 	passwd = recv_password_packet(port);
-
 	if (passwd == NULL)
 		return STATUS_EOF;		/* client wouldn't send password */
 
-	result = md5_crypt_verify(port->user_name, passwd, NULL, 0, logdetail);
+	result = get_role_password(port->user_name, _pass, logdetail);
+	if (result == STATUS_OK)
+		result = plain_crypt_verify(port->user_name, shadow_pass, passwd,
+	logdetail);
 
+	if (shadow_pass)
+		pfree(shadow_pass);
 	pfree(passwd);
 
 	return result;
diff --git a/src/backend/libpq/crypt.c b/src/backend/libpq/crypt.c
index b4ca174..fb6d1af 100644
--- a/src/backend/libpq/crypt.c
+++ b/src/backend/libpq/crypt.c
@@ -30,28 +30,28 @@
 
 
 /*
- * Check given password for given user, and return STATUS_OK or STATUS_ERROR.
+ * Fetch stored password for a user, for authentication.
  *
- * 'client_pass' is the password response given by the remote user.  If
- * 'md5_salt' is not NULL, it is a response to an MD5 authentication
- * challenge, with the given salt.  Otherwise, it is a plaintext password.
+ * Returns STATUS_OK on success.  On error, returns STATUS_ERROR, and stores
+ * a palloc'd string describing the reason, for the postmaster log, in
+ * *logdetail.  The error reason should *not* be sent to the client, to avoid
+ * giving away user information!
  *
- * In the error case, optionally store a palloc'd string at *logdetail
- * that will be sent to the postmaster log (but not the client).
+ * If the password is expired, it is still returned in *shadow_pass, but the
+ * return code is STATUS_ERROR.  On other errors, *shadow_pass is set to
+ * NULL.
  */
 int
-md5_crypt_verify(const char *role, char *client_pass,
- char *md5_salt, int md5_salt_len, char **logdetail)
+get_role_password(const char *role, char **shadow_pass, char **logdetail)
 {
 	int			retval = STATUS_ERROR;
-	char	   *shadow_pass,
-			   *crypt_pwd;
 	TimestampTz vuntil 

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

2016-12-09 Thread Michael Paquier
On Fri, Dec 9, 2016 at 5:11 PM, Heikki Linnakangas  wrote:
> Couple of things I should write down before I forget:
>
> 1. It's a bit cumbersome that the scram verifiers stored in
> pg_authid.rolpassword don't have any clear indication that they're scram
> verifiers. MD5 hashes are readily identifiable by the "md5" prefix. I think
> we should use a "scram-sha-256:" for scram verifiers.

scram-sha-256 would make the most sense to me.

> Actually, I think it'd be awfully nice to also prefix plaintext passwords
> with "plain:", but I'm not sure it's worth breaking the compatibility, if
> there are tools out there that peek into rolpassword. Thoughts?

pgbouncer is the only thing coming up in mind. It looks at pg_shadow
for password values. pg_dump'ing data from pre-10 instances will also
need to adapt. I see tricky the compatibility with the exiting CREATE
USER PASSWORD command though, so I am wondering if that's worth the
complication.

> 2. It's currently not possible to use the plaintext "password"
> authentication method, for a user that has a SCRAM verifier in rolpassword.
> That seems like an oversight. We can't do MD5 authentication with a SCRAM
> verifier, but "password" we could.

Yeah, that should be possible...
-- 
Michael


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


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

2016-12-09 Thread Heikki Linnakangas

Couple of things I should write down before I forget:

1. It's a bit cumbersome that the scram verifiers stored in 
pg_authid.rolpassword don't have any clear indication that they're scram 
verifiers. MD5 hashes are readily identifiable by the "md5" prefix. I 
think we should use a "scram-sha-256:" for scram verifiers.


Actually, I think it'd be awfully nice to also prefix plaintext 
passwords with "plain:", but I'm not sure it's worth breaking the 
compatibility, if there are tools out there that peek into rolpassword. 
Thoughts?


2. It's currently not possible to use the plaintext "password" 
authentication method, for a user that has a SCRAM verifier in 
rolpassword. That seems like an oversight. We can't do MD5 
authentication with a SCRAM verifier, but "password" we could.


- Heikki



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


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

2016-12-09 Thread Heikki Linnakangas

On 12/09/2016 05:58 AM, Michael Paquier wrote:

On Thu, Dec 8, 2016 at 10:05 PM, Michael Paquier
 wrote:

On Thu, Dec 8, 2016 at 5:55 PM, Heikki Linnakangas  wrote:

Actually, we don't give away that information currently. If you try to log
in with password or MD5 authentication, and the user doesn't exist, you get
the same error as with an incorrect password. So, I think we do need to give
the client a made-up salt and iteration count in that case, to hide the fact
that the user doesn't exist. Furthermore, you can't just generate random
salt and iteration count, because then you could simply try connecting
twice, and see if you get the same salt and iteration count. We need to
deterministically derive the salt from the username, so that you get the
same salt/iteration count every time you try connecting with that username.
But it needs indistinguishable from a random salt, to the client. Perhaps a
SHA hash of the username and some per-cluster secret value, created by
initdb. There must be research papers out there on how to do this..


A simple idea would be to use the system ID when generating this fake
salt? That's generated by initdb, once per cluster. I am wondering if
it would be risky to use it for the salt. For the number of iterations
the default number could be used.


I think I'd feel better with a completely separate randomly-generated 
value for this. System ID is not too difficult to guess, and there's no 
need to skimp on this. Yes, default number of iterations makes sense.


We cannot completely avoid leaking information through this, 
unfortunately. For example, if you have a user with a non-default number 
of iterations, and an attacker probes that, he'll know that the username 
was valid, because he got back a non-default number of iterations. But 
let's do our best.



I have been thinking more about this part quite a bit, and here is the
most simple thing that we could do while respecting the protocol.
That's more or less what I think you have in mind by re-reading
upthread, but it does not hurt to rewrite the whole flow to be clear:
1) Server gets the startup packet, maps pg_hba.conf and moves on to
the scram authentication code path.
2) Server sends back sendAuthRequest() to request user to provide a
password. This maps to the plain/md5 behavior as no errors would be
issued to user until he has provided a password.
3) Client sends back the password, and the first message with the user name.
4) Server receives it, and checks the data. If a failure happens at
this stage, just ERROR on PG-side without sending back a e= message.
This includes the username-mismatch, empty password and end of
password validity. So we would never use e=unknown-user. This sticks
with what you quoted upthread that the server may end the exchange
before sending the final message.


If we want to mimic the current behavior with MD5 authentication, I 
think we need to follow through with the challenge, and only fail in the 
last step, even if we know the password was empty or expired. MD5 
authentication doesn't currently give away that information to the user.


But it's OK to bail out early on OOM, or if the client sends an outright 
broken message. Those don't give away any information on the user account.



5) Server sends back the challenge, and client answers back with its
reply to it.



Then enters the final stage of the exchange, at which point the server
would issue its final message that would be e= in case of errors. If
something like an OOM happens, no message would be sent so failing on
an OOM ERROR on PG side would be fine as well.



6) Read final message from client and validate.
7) issue final message of server.

On failure at steps 6) or 7), an e= message is returned instead of the
final message. Does that look right?


Yep.


One thing is: when do we look up at pg_authid? After receiving the
first message from client or before beginning the exchange? As the
first message from client has the user name, it would make sense to do
the lookup after receiving it, but from PG prospective it would just
make sense to use the data already present in the startup packet. The
current patch does the latter. What do you think?


Let's see what fits the program flow best. Probably best to do it before 
beginning the exchange. I'm hacking on this right now...



By the way, I have pushed the extra patches you sent into this branch:
https://github.com/michaelpq/postgres/tree/scram


Thanks! We had a quick chat with Michael, and agreed that we'd hack 
together on that github repository, to avoid stepping on each other's 
toes, and cut rebased patch sets from there to pgsql-hackers every now 
and then.


- Heikki



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