Re: [HACKERS] Logical Replication WIP
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.
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-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
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?
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.
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)
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)
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
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
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
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 +581,8 @@ fetch_input_tuple(AggState *aggstat
Re: [HACKERS] [COMMITTERS] pgsql: Implement table partitioning.
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
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, &stat_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 ' ' specifically.
> Overwrite
Re: [HACKERS] [OSSTEST PATCH 0/1] PostgreSQL db: Retry on constraint violation
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
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
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
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 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)
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
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
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
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.
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
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.
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.
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
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
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
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)
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
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
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" ?
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
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. - The other vari
Re: [HACKERS] building HEAD on macos fails with #error no source of random numbers configured
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
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
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
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
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
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
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
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" ?
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
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, &exception_map[i].sqlstate,
HASH_ENTER, &found);
--
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
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?
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
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 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?
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
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
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
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
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
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
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, &isnull);
> @@ -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
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
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, &shadow_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, &shadow_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 = 0;
- char
Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol
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
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
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
