Re: [HACKERS] PG 10 release notes

[Andrew Borodin]

Hi, Bruce!

2017-04-25 6:31 GMT+05:00 Bruce Momjian :
> The only unusual thing is that this release has ~180 items while most
> recent release have had ~220.  The pattern I see that there are more
> large features in this release than previous ones.

I'm not sure, but, probably, it worth mentioning this [1,2] in the
E.1.3.1.2. Indexes section.
Better tuple management on GiST page allows faster inserts and
updates. (In numbers: 15% for randomized data,3x for ordered data in
specific cases)

[1] https://commitfest.postgresql.org/10/661/
[2] 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b1328d78f88cdf4f7504004159e530b776f0de16

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] [PostgreSQL 10] default of hot_standby should be "on"?

[Huong Dangminh]

> On Thu, Apr 27, 2017 at 11:48 AM, Masahiko Sawada 
> wrote:
> > Thank you for updating the patch. Also maybe we can update line in
> > PostgresNode.pm where hot_standby is set to on explicitly.
> 
> I would refrain from doing that, having some parameters listed in the
> tests makes the intention behind those perl routines clear.
> --
> Michael

Thanks, attached patch update PostgresNode.pm file.
I also did the regression test and found no problem.

# sorry Sawada-san, Michael-san, because of security restriction
# we could not mail to gmail address from our environment.

--- 
Thanks and best regards, 
Dang Minh Huong 
NEC Solution Innovators, Ltd. 
http://www.nec-solutioninnovators.co.jp/en/



hot_standby.patch
Description: hot_standby.patch

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

Re: [HACKERS] scram and \password

[Ashesh Vashi]

On Thu, Apr 27, 2017 at 9:57 AM, Michael Paquier 
wrote:

> On Thu, Apr 27, 2017 at 1:25 PM, Ashesh Vashi
>  wrote:
> > - Do we need to provide the method here?
> > We have connection object itself, it can decide from the type of
> connection,
> > which method to be used.
>
> Providing the method is not mandatory. If you look upthread... If the
> caller of this routine specified method = NULL, then the value of
> password_encryption on the server is queried automatically and that
> will be the method used to hash the password.
>
I missed that. Sorry.

Thanks.

--Thanks, Ashesh

> --
> Michael
>

Re: [HACKERS] scram and \password

[Michael Paquier]

On Thu, Apr 27, 2017 at 1:25 PM, Ashesh Vashi
 wrote:
> - Do we need to provide the method here?
> We have connection object itself, it can decide from the type of connection,
> which method to be used.

Providing the method is not mandatory. If you look upthread... If the
caller of this routine specified method = NULL, then the value of
password_encryption on the server is queried automatically and that
will be the method used to hash the password.
-- 
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] scram and \password

[Ashesh Vashi]

On Tue, Apr 25, 2017 at 8:56 PM, Heikki Linnakangas  wrote:

> On 04/22/2017 01:20 AM, Michael Paquier wrote:
>
>> On Sat, Apr 22, 2017 at 5:04 AM, Heikki Linnakangas 
>> wrote:
>>
>>> I'll continue reviewing the rest of the patch on Monday, but one glaring
>>> issue is that we cannot add an argument to the existing libpq
>>> PQencryptPassword() function, because that's part of the public API. It
>>> would break all applications that use PQencryptPassword().
>>>
>>> What we need to do is to add a new function. What should we call that? We
>>> don't have a tradition of using "Ex" or such suffix to mark extended
>>> versions of existing functions. PQencryptPasswordWithMethod(user, pass,
>>> method) ?
>>>
>>
>> Do we really want to add a new function or have a hard failure? Any
>> application calling PQencryptPassword may trap itself silently if the
>> server uses scram as hba key or if the default is switched to that,
>> from this point of view extending the function makes sense as well.
>>
>
> Yeah, there is that. But we simply cannot change the signature of an
> existing function. It would not only produce compile-time errors when
> building old applications, which would arguably be a good thing, but it
> would also cause old binaries to segfault when dynamically linked with the
> new libpq.
>
> I think it's clear that we should have a new function that takes the
> algorithm as argument. But there are open decisions on what the old
> PQencryptPassword() function should do, and also what the new function
> should do by default, if you don't specify an algorithm:
>
> A) Have PQencryptPassword() return an md5 hash.
>
> B) Have PQencryptPassword() return a SCRAM verifier
>
> C) Have PQencryptPassword() return a SCRAM verifier if connected to a v10
> server, and an md5 hash otherwise. This is tricky, because
> PQencryptPassword doesn't take a PGconn argument. It could behave like
> PQescapeString() does, and choose md5/scram depending on the server version
> of the last connection that was established.
>
> For the new function, it's probably best to pass a PGconn argument. That
> way we can use the connection to determine the default, and it seems to be
> a good idea for future-proofing too. And an extra "options" argument might
> be good, while we're at it, to e.g. specify the number of iterations for
> SCRAM. So all in all, I propose the documentation for these functions to be
> (I chose option C from above for this):
>
> 
> char *
> PQencryptPasswordConn(PGconn *conn,
>   const char *passwd,
>   const char *user,
>   const char *method,
>   const char *options)
>
I was just thinking:
- Do we need to provide the method here?
We have connection object itself, it can decide from the type of
connection, which method to be used.

-- Thanks, Ashesh


>
> [this paragraph is the same as current PQencryptPassword()]
> This function is intended to be used by client applications that wish to
> send commands like ALTER ROLE joe PASSWORD 'pwd'. It is good practice to
> not send the original cleartext password in such a command, because it
> might be exposed in command logs, activity displays and so on. Instead, use
> this function to convert the password to encrypted form before it is sent.
> [end of unchanged part]
>
> This function may execute internal queries to the server to determine
> appropriate defaults, using the given connection object. The call can
> therefore fail if the connection is busy executing another query, or the
> current transaction is aborted.
>
> The return value is a string allocated by malloc, or NULL if out of memory
> or other error. On error, a suitable message is stored in the 'conn'
> object. The caller can assume the string doesn't contain any special
> characters that would require escaping. Use PQfreemem to free the result
> when done with it.
>
> The function arguments are:
>
> conn
>   Connection object for the database where the password is to be changed.
>
> passwd
>   The new password
>
> user
>   Name of the role whose password is to be changed
>
> method
>   Name of the password encryption method to use. Currently supported
> methods are "md5" or "scram-sha-256". If method is NULL, the default for
> the current database is used. [i.e. this looks at password_encryption]
>
> options
>   Options specific to the encryption method, or NULL to use the defaults.
> (This argument is for future expansion, there are currently no options, and
> you should always pass NULL.)
>
>
> char *
> PQencryptPassword(const char *passwd, const char *user)
>
> PQencryptPassword is an older, deprecated version of PQencryptPasswodConn.
> The difference is that PQencryptPassword does not require a connection
> object. The encryption method will be chosen depending on the server
> version of the last established connection, and built-in default options.
>
> 
>
> Thoughts? Unless someone 

Re: [HACKERS] [PostgreSQL 10] default of hot_standby should be "on"?

[Michael Paquier]

On Thu, Apr 27, 2017 at 11:48 AM, Masahiko Sawada  wrote:
> Thank you for updating the patch. Also maybe we can update line in
> PostgresNode.pm where hot_standby is set to on explicitly.

I would refrain from doing that, having some parameters listed in the
tests makes the intention behind those perl routines clear.
-- 
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] scram and \password

[Michael Paquier]

On Wed, Apr 26, 2017 at 7:22 PM, Heikki Linnakangas  wrote:
> We talked about the alternative where PQencryptPasswordConn() would not look
> at password_encryption, but would always use the strongest possible
> algorithm supported by the server. That would avoid querying the server. But
> then I started thinking how this would work, if we make the number of
> iterations in the SCRAM verifier configurable in the future. The client
> would not know the desired number of iterations based only on the server
> version, it would need to query the server, and we would be back to square
> one. We could add an "options" argument to PQencryptPasswordConn() that the
> application could use to pass that information, but documenting how to fetch
> that information, if you don't want PQencryptPasswordConn() to block, gets
> tedious, and puts a lot of burden to applications. That is why I left out
> the "options" argument, after all.

Fine for me.

> I'm now thinking that if we add password hashing options like the iteration
> count in the future, they will be tacked on to password_encryption. For
> example, "password_encryption='scram-sha-256, iterations=1". That way,
> "password_encryption" will always contain enough information to construct
> password verifiers.

That's possible as well, adding more GUCs for sub-options of a hashing
algorithm is wrong.

> As an alternative, I considered making password_encryption GUC_REPORT, so
> that libpq would always know it without having to issue a query. But it
> feels wrong to send that option to the client on every connection, when it's
> only needed in the rare case that you use PQencryptPasswordConn(). And if we
> added new settings like the iteration count in the future, those would need
> to be GUC_REPORT too.

Agreed, PQencryptPassword is not that widely used..

Here are some comments.

+   /*
+* Normalize the password with SASLprep.  If that doesn't work, because
+* the password isn't valid UTF-8 or contains prohibited
characters, just
+* proceed with the original password.  (See comments at top of file.)
+*/
+   rc = pg_saslprep(password, _password);
This comment is not true, comments are at the top of auth-scram.c.

+ * The password should already have been processed with SASLprep, if necessary!
+ *
+ * If iterations is 0, default number of iterations is used.  The result is
+ * palloc'd or malloc'd, so caller is responsible for freeing it.
+ */
+char *
+scram_build_verifier(const char *salt, int saltlen, int iterations,
+const char *password)
+{
+   uint8   storedkeybuf[SCRAM_KEY_LEN];
+   uint8   serverkeybuf[SCRAM_KEY_LEN];
+   char   *result;
+   char   *p;
+   int maxlen;
I think that it is a mistake to move SASLprep out of
scram_build_verifier, because pre-processing the password is not
necessary, it is normally mandatory. The BE/FE versions that you are
adding also duplicate the calls to pg_saslprep().

Using "encrypt" instead of "hash" in the function name :(

+   else if (strcmp(algorithm, "plain") == 0)
+   {
+   crypt_pwd = strdup(passwd);
+   }
This is not documented, and users should be warned about using it as well.
-- 
Michael


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

[HACKERS] Crash when partition column specified twice

[Amit Langote]

Noticed that a crash occurs if a column is specified twice when creating a
partition:

create table p (a int) partition by list (a);

-- crashes
create table p1 partition of parent (
  a not null,
  a default 1
) for values in (1);

The logic in MergeAttributes() that merged partition column options with
those of the parent didn't properly check for column being specified twice
and instead tried to delete the same ColumnDef from a list twice, causing
the crash.

Attached fixes that.

Added to the open items list.

Thanks,
Amit
>From 8b1c2ea63c22f4b5180244a41350c2391caffda3 Mon Sep 17 00:00:00 2001
From: amit 
Date: Thu, 27 Apr 2017 11:22:55 +0900
Subject: [PATCH] Fix crash when partition column specified twice

---
 src/backend/commands/sequence.c|  1 +
 src/backend/commands/tablecmds.c   | 20 +++-
 src/backend/nodes/copyfuncs.c  |  1 +
 src/backend/nodes/equalfuncs.c |  1 +
 src/backend/nodes/makefuncs.c  |  1 +
 src/backend/nodes/outfuncs.c   |  1 +
 src/backend/parser/gram.y  |  5 +
 src/backend/parser/parse_utilcmd.c |  2 ++
 src/include/nodes/parsenodes.h |  1 +
 src/test/regress/expected/create_table.out |  8 
 src/test/regress/sql/create_table.sql  |  9 +
 11 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index ad28225b36..acd4e359bb 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -167,6 +167,7 @@ DefineSequence(ParseState *pstate, CreateSeqStmt *seq)
 		coldef->is_local = true;
 		coldef->is_not_null = true;
 		coldef->is_from_type = false;
+		coldef->is_from_parent = false;
 		coldef->storage = 0;
 		coldef->raw_default = NULL;
 		coldef->cooked_default = NULL;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a35713096d..4df17c0efc 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -1919,6 +1919,7 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
 def->is_local = false;
 def->is_not_null = attribute->attnotnull;
 def->is_from_type = false;
+def->is_from_parent = true;
 def->storage = attribute->attstorage;
 def->raw_default = NULL;
 def->cooked_default = NULL;
@@ -2206,11 +2207,20 @@ MergeAttributes(List *schema, List *supers, char relpersistence,
 	 * merge the column options into the column from the
 	 * parent
 	 */
-	coldef->is_not_null = restdef->is_not_null;
-	coldef->raw_default = restdef->raw_default;
-	coldef->cooked_default = restdef->cooked_default;
-	coldef->constraints = restdef->constraints;
-	list_delete_cell(schema, rest, prev);
+	if (coldef->is_from_parent)
+	{
+		coldef->is_not_null = restdef->is_not_null;
+		coldef->raw_default = restdef->raw_default;
+		coldef->cooked_default = restdef->cooked_default;
+		coldef->constraints = restdef->constraints;
+		coldef->is_from_parent = false;
+		list_delete_cell(schema, rest, prev);
+	}
+	else
+		ereport(ERROR,
+(errcode(ERRCODE_DUPLICATE_COLUMN),
+ errmsg("column \"%s\" specified more than once",
+		coldef->colname)));
 }
 prev = rest;
 rest = next;
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 00a0fed23d..8fb872d288 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2804,6 +2804,7 @@ _copyColumnDef(const ColumnDef *from)
 	COPY_SCALAR_FIELD(is_local);
 	COPY_SCALAR_FIELD(is_not_null);
 	COPY_SCALAR_FIELD(is_from_type);
+	COPY_SCALAR_FIELD(is_from_parent);
 	COPY_SCALAR_FIELD(storage);
 	COPY_NODE_FIELD(raw_default);
 	COPY_NODE_FIELD(cooked_default);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 46573ae767..21dfbb0d75 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -2540,6 +2540,7 @@ _equalColumnDef(const ColumnDef *a, const ColumnDef *b)
 	COMPARE_SCALAR_FIELD(is_local);
 	COMPARE_SCALAR_FIELD(is_not_null);
 	COMPARE_SCALAR_FIELD(is_from_type);
+	COMPARE_SCALAR_FIELD(is_from_parent);
 	COMPARE_SCALAR_FIELD(storage);
 	COMPARE_NODE_FIELD(raw_default);
 	COMPARE_NODE_FIELD(cooked_default);
diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index e88d82f3b0..f5fde1533f 100644
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -494,6 +494,7 @@ makeColumnDef(const char *colname, Oid typeOid, int32 typmod, Oid collOid)
 	n->is_local = true;
 	n->is_not_null = false;
 	n->is_from_type = false;
+	n->is_from_parent = false;
 	n->storage = 0;
 	n->raw_default = NULL;
 	n->cooked_default = NULL;
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 28cef85579..05a78b32b7 100644
--- a/src/backend/nodes/outfuncs.c
+++ 

[HACKERS] Inefficient shutdown of pg_basebackup

[Tom Lane]

I griped before that the src/test/recovery/ tests take an unreasonably
long time.  My interest in that was piqued further when I noticed that
the tests consume not very much CPU time, and aren't exactly saturating
my disks either.  That suggests that the problem isn't so much that the
tests do too much work, as that we've got dire performance problems in
either the test harness or the code under test.

While I'm continuing to poke at it, I've identified one such problem:
the system basically stops dead for about ten seconds at the end of
the pg_basebackup run invoked by t/001_stream_rep.pl.  The length of
the idle time corresponds to pg_basebackup's -s (standby_message_timeout)
parameter; you can make it even worse by increasing that parameter or
setting it to zero.  (In principle, setting it to zero ought to cause
pg_basebackup to never terminate at all :-( ... but apparently there is
some other effect that will wake it up after 30 seconds or so.  I've not
found out what yet.)

The reason for this appears to be that by the time the pg_basebackup
parent process has determined the xlogend position and sent it down
the bgpipe to the child process, the child process has already read
all the WAL that the source server is going to send, and is waiting
for more such input with a timeout corresponding to
standby_message_timeout.  Only after that timeout elapses does it
get around to noticing that some input is available from the bgpipe
and then realizing that it's time to stop streaming.

The attached draft patch fixes this by expanding the StreamCtl API
with a socket that the low-level wait routine should check for input.
For me, this consistently knocks about 10 seconds off the runtime of
001_stream_rep.pl.

It could be argued that this isn't too significant in the real world
because pg_basebackup would always run far longer than 10 seconds
anyway for non-toy data.  But it still looks like a bug to me.

regards, tom lane

diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 40ec0e1..e2a2ebb 100644
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
*** LogStreamerMain(logstreamer_param *param
*** 480,485 
--- 480,490 
  	stream.timeline = param->timeline;
  	stream.sysidentifier = param->sysidentifier;
  	stream.stream_stop = reached_end_position;
+ #ifndef WIN32
+ 	stream.stop_socket = bgpipe[0];
+ #else
+ 	stream.stop_socket = PGINVALID_SOCKET;
+ #endif
  	stream.standby_message_timeout = standby_message_timeout;
  	stream.synchronous = false;
  	stream.do_sync = do_sync;
diff --git a/src/bin/pg_basebackup/pg_receivewal.c b/src/bin/pg_basebackup/pg_receivewal.c
index 1a9fe81..09385c5 100644
*** a/src/bin/pg_basebackup/pg_receivewal.c
--- b/src/bin/pg_basebackup/pg_receivewal.c
*** StreamLog(void)
*** 409,414 
--- 409,415 
  stream.timeline);
  
  	stream.stream_stop = stop_streaming;
+ 	stream.stop_socket = PGINVALID_SOCKET;
  	stream.standby_message_timeout = standby_message_timeout;
  	stream.synchronous = synchronous;
  	stream.do_sync = true;
diff --git a/src/bin/pg_basebackup/receivelog.c b/src/bin/pg_basebackup/receivelog.c
index 8511e57..c41bba2 100644
*** a/src/bin/pg_basebackup/receivelog.c
--- b/src/bin/pg_basebackup/receivelog.c
*** static bool still_sending = true;		/* fe
*** 39,46 
  
  static PGresult *HandleCopyStream(PGconn *conn, StreamCtl *stream,
   XLogRecPtr *stoppos);
! static int	CopyStreamPoll(PGconn *conn, long timeout_ms);
! static int	CopyStreamReceive(PGconn *conn, long timeout, char **buffer);
  static bool ProcessKeepaliveMsg(PGconn *conn, StreamCtl *stream, char *copybuf,
  	int len, XLogRecPtr blockpos, TimestampTz *last_status);
  static bool ProcessXLogDataMsg(PGconn *conn, StreamCtl *stream, char *copybuf, int len,
--- 39,47 
  
  static PGresult *HandleCopyStream(PGconn *conn, StreamCtl *stream,
   XLogRecPtr *stoppos);
! static int	CopyStreamPoll(PGconn *conn, long timeout_ms, pgsocket stop_socket);
! static int CopyStreamReceive(PGconn *conn, long timeout, pgsocket stop_socket,
!   char **buffer);
  static bool ProcessKeepaliveMsg(PGconn *conn, StreamCtl *stream, char *copybuf,
  	int len, XLogRecPtr blockpos, TimestampTz *last_status);
  static bool ProcessXLogDataMsg(PGconn *conn, StreamCtl *stream, char *copybuf, int len,
*** CheckServerVersionForStreaming(PGconn *c
*** 417,424 
--- 418,432 
   * return. As long as it returns false, streaming will continue
   * indefinitely.
   *
+  * If stream_stop() checks for external input, stop_socket should be set to
+  * the FD it checks.  This will allow such input to be detected promptly
+  * rather than after standby_message_timeout (which might be indefinite).
+  * Note that signals will interrupt waits for input as well, but that is
+  * race-y since a signal received while busy won't interrupt the wait.
+  *
   * 

Re: [HACKERS] Logical replication in the same cluster

[Tom Lane]

Peter Eisentraut  writes:
>>> If that's a predictable deadlock, I think a minimum expectation is that
>>> the system should notice it and throw an error, not just hang.

> We had some discussions early on about detecting connections to the same
> server, but it's not entirely clear how to do that and it didn't seem
> worth it at the time.

I wonder whether we actually need to detect connections to the same
server per se.  I'm thinking about the one end taking some special
heavyweight lock, and the other end taking the same lock, which would
generally be free as long as the two ends aren't on the same server.
Cascading replication might be a problem though ...

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] [PostgreSQL 10] default of hot_standby should be "on"?

[Masahiko Sawada]

On Thu, Apr 27, 2017 at 9:54 AM, Huong Dangminh
 wrote:
> Thanks all for your comments.
>
>> Magnus Hagander  writes:
>> > +1. I definitely think we should do it, and 10 would be the time to do
>> it.
>>
>> Agreed.  It's mainly a historical accident that the default is what it
>> is,
>> I think.
>>
>> > I wonder if we should also consider changing the standby error message
>> to
>> > be a WARNING instead of an ERROR. So that if you try to start up a standby
>> > with hot_standby=on but master with wal_level=replica it would turn into
>> a
>> > cold standby.
>>
>> I'm -1 for that: if you fat-finger the configuration, you should be told
>> about it, not have the system start up in an unintended mode that lacks
>> critical functionality.
>>
>>   regards, tom lane
>
> I attached the patch which also update manual as the mention of sawada-san.
>

Thank you for updating the patch. Also maybe we can update line in
PostgresNode.pm where hot_standby is set to on explicitly.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


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

Re: [HACKERS] PG 10 release notes

[Amit Kapila]

On Tue, Apr 25, 2017 at 8:59 AM, Bruce Momjian  wrote:
> On Mon, Apr 24, 2017 at 11:05:41PM -0400, Bruce Momjian wrote:
>> > I think the above commit needs a separate mention, as this is a really
>> > huge step forward to control the size of hash indexes.
>>
>> Yes, it is unfotunate that the item is in the incompatibility item.  I
>> wonder if I should split out the need to rebuild the hash indexes and
>> keep it there and move this item into the "Index" section.
>
> Done, items split.
>




 Improve efficiency of hash index growth (Amit Kapila, Mithun Cy)

 

The first two commits b0f18cb77, 30df93f69 are done as preliminary
work to "Add write-ahead logging support to hash indexes", so it seems
inappropriate to add them here.  We can add it along with below item:

 

 Add write-ahead logging support to hash indexes (Amit Kapila)




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


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

Re: [HACKERS] vcregress support for single TAP tests

[Peter Eisentraut]

On 4/23/17 17:09, Andrew Dunstan wrote:
> 
> Here's a patch that will allow calling vcregress.pl to run a single TAP
> test set. It would work like this:
> 
> 
> vcregress.pl src/test/recover true
> 
> 
> The second argument if true (in the perl sense, of course) would trigger
> a temp install before running the tests. It defaults to off, in an
> attempt to minimize the unnecessary running of installs.

Seems kind of weird to have that functionality only for the tap tests,
though.

-- 
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 10 release notes

[Amit Kapila]

On Wed, Apr 26, 2017 at 8:46 PM, Bruce Momjian  wrote:
> On Wed, Apr 26, 2017 at 07:38:05PM +0530, Amit Kapila wrote:
>> >> I have already mentioned the commit id (5e6d8d2b).  Text can be "Allow
>> >> queries containing subplans to execute in parallel".  We should also
>> >> mention in some way that this applies only when the query contains
>> >> uncorrelated subplan.
>> >
>> > Sorry but I don't know what that means, and if I don't know, others
>> > might not either.
>> >
>>
>> regression=# explain (costs off) select count(*) from tenk1 where
>> (two, four) not in (select hundred, thousand from tenk2 where
>> thousand > 100);
>>   QUERY PLAN
>> --
>>  Finalize Aggregate
>>->  Gather
>>  Workers Planned: 2
>>  ->  Partial Aggregate
>>->  Parallel Seq Scan on tenk1
>>  Filter: (NOT (hashed SubPlan 1))
>>  SubPlan 1
>>->  Seq Scan on tenk2
>>  Filter: (thousand > 100)
>> (9 rows)
>>
>
> Oh, so non-correlated subqueries can be run in parallel.  Yes, that is
> something we should have in the release notes.  How is this?
>
> Author: Robert Haas 
> 2017-02-14 [5e6d8d2bb] Allow parallel workers to execute subplans.
>
> Allow non-correlated subqueries to be run in parallel (Amit Kapila)
>

Looks good to me.


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


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

Re: [HACKERS] StandbyRecoverPreparedTransactions recovers subtrans links incorrectly

[Michael Paquier]

On Thu, Apr 27, 2017 at 2:25 AM, Tom Lane  wrote:
> Simon Riggs  writes:
>> I've added code following Michael and Tom's comments to the previous
>> patch. New patch attached.
>
> Couple of minor suggestions:
>
> * Rather than deleting the comment for SubTransSetParent entirely,
> maybe make it say "It's possible that the parent was already recorded.
> However, we should never be asked to change an already-set entry to
> something else."
>
> * In SubTransGetTopmostTransaction, maybe it'd be better to spell
> "TransactionIdFollowsOrEquals" as "!TransactionIdPrecedes" to make
> it look more like the test just above.  Matter of taste though.
>
> * Less a matter of taste is that I think that should be just elog(ERROR);
> there's no good reason to make it FATAL.
>
> * Also, I think there should be a comment there, along the lines of
> "check for reversed linkage to ensure this isn't an infinite loop".

No more comments from here, thanks for working on the patch.
-- 
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] identity columns

[Peter Eisentraut]

On 4/23/17 16:58, Robert Haas wrote:
> I agree that ADD is a little odd here, but it doesn't seem terrible.
> But why do we need it?  Instead of:
> 
> ADD GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
> SET GENERATED { ALWAYS | BY DEFAULT }
> DROP IDENTITY [ IF EXISTS ]
> 
> Why not just:
> 
> SET GENERATED { ALWAYS | BY DEFAULT } AS IDENTITY
> DROP IDENTITY [ IF EXISTS ]
> 
> Surely the ALTER TABLE command can tell whether the column is already
> GENERATED, so the first form could make it generated if it's not and
> adjust the ALWAYS/BY DEFAULT property if it is.

Note that DROP IDENTITY is a non-idempotent command (i.e., it errors if
the thing has already been dropped), per SQL standard, which is why we
have IF EXISTS there.  So it would be weird if the corresponding
creation command would be idempotent (i.e., it did not care whether the
thing is already there).

Also, if we tried to merge the ADD and SET cases, the syntax would come
out weird.  The creation syntax is

CREATE TABLE t1 (c1 int GENERATED ALWAYS AS IDENTITY);

The syntax to change an existing column is

ALTER TABLE t1 ALTER COLUMN c1 SET GENERATED ALWAYS;

But we can't just make the "AS IDENTITY" optional, because that same
syntax is also used by the "generated columns" feature.

So we could make up new syntax

ALTER TABLE t1 ALTER COLUMN c1 SET GENERATED ALWAYS AS IDENTITY;

and let that be set-or-add, but then the argument for staying within the
SQL standard goes out the window.

Finally, I had mentioned that earlier in this thread, the behavior of
the sequence options differs depending on whether the sequence already
exists.  So if you wrote

ALTER TABLE t1 ALTER COLUMN c1 SET GENERATED ALWAYS AS IDENTITY (START 2);

and the sequence does not exist, you get a new sequence with START 2 and
all default values otherwise.  If the sequence already exists, you keep
the sequence and just change the start value.  So that's not truly
idempotent either.

So I think altogether it is much clearer and more consistent to have
separate verbs for create/change/remove.

-- 
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] Unportable implementation of background worker start

[Tom Lane]

Robert Haas  writes:
> On Wed, Apr 26, 2017 at 8:37 PM, Andres Freund  wrote:
>>> 3. Go ahead with converting the postmaster to use WaitEventSet, a la
>>> the draft patch I posted earlier.  I'd be happy to do this if we were
>>> at the start of a devel cycle, but right now seems a bit late --- not
>>> to mention that we really need to fix 9.6 as well.

>> Yea, backpatching this to 9.6 seems like a bigger hammer than
>> appropriate.  I'm on the fence WRT master, I think there's an argument
>> to be made that this is going to become a bigger and bigger problem, and
>> that we'll wish in a year or two that we had fewer releases with
>> parallelism etc that don't use WaitEventSets.

> I think changing this might be wise.  This problem isn't going away
> for real until we do this, right?

Sure, but we have a lot of other problems that aren't going away until
we fix them, either.  This patch smells like new development to me ---
and it's not even very complete, because really what Andres wants to do
(and I concur) is to get rid of the postmaster's habit of doing
interesting things in signal handlers.  I'm definitely not on board with
doing that for v10 at this point.  But if we apply this patch, and then
do that in v11, then v10 will look like neither earlier nor later branches
with respect to the postmaster's event wait mechanisms.  I think that's
a recipe for undue maintenance pain.

I believe that the already-committed patches represent a sufficient-for-now
response to the known performance problems here, and so I'm thinking we
should stop here for v10.  I'm okay with pushing the latch.c changes
I just proposed, because those provide a useful safeguard against
extension modules doing something exciting in the postmaster process.
But I don't think we should go much further than that for v10.

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] Declarative partitioning - another take

[Amit Langote]

On 2017/04/27 1:52, Robert Haas wrote:
> On Tue, Apr 25, 2017 at 10:34 PM, Amit Langote
>  wrote:
>> FWIW, I too prefer the latter, that is, fire only the parent's triggers.
>> In that case, applying only the patch 0001 will do.
> 
> Do we need to update the documentation?

Yes, I think we should.  How about as in the attached?

By the way, code changes I made in the attached are such that a subsequent
patch could implement firing statement-level triggers of all the tables in
a partition hierarchy, which it seems we don't want to do.  Should then
the code be changed to not create ResultRelInfos of all the tables but
only the root table (the one mentioned in the command)?  You will see that
the patch adds fields named es_nonleaf_result_relations and
es_num_nonleaf_result_relations, whereas just es_root_result_relation
would perhaps do, for example.

Thanks,
Amit
>From c8b785a77a2a193906967762066e03e09de80442 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 24 Apr 2017 14:55:08 +0900
Subject: [PATCH 1/2] Fire per-statement triggers of partitioned tables

Current implementation completely misses them, because it assumes
that no ResultRelInfos need to be created for partitioned tables,
whereas they *are* needed to fire the per-statment triggers, which
we *do* allow to be defined on the partitioned tables.

Reported By: Rajkumar Raghuwanshi
Patch by: Amit Langote
Report: https://www.postgresql.org/message-id/CAKcux6%3DwYospCRY2J4XEFuVy0L41S%3Dfic7rmkbsU-GXhhSbmBg%40mail.gmail.com
---
 doc/src/sgml/trigger.sgml   | 26 -
 src/backend/executor/execMain.c | 33 -
 src/backend/executor/nodeModifyTable.c  | 66 -
 src/backend/nodes/copyfuncs.c   |  1 +
 src/backend/nodes/outfuncs.c|  1 +
 src/backend/nodes/readfuncs.c   |  1 +
 src/backend/optimizer/plan/createplan.c |  1 +
 src/backend/optimizer/plan/setrefs.c|  4 ++
 src/include/nodes/execnodes.h   | 11 ++
 src/include/nodes/plannodes.h   |  2 +
 src/test/regress/expected/triggers.out  | 54 +++
 src/test/regress/sql/triggers.sql   | 48 
 12 files changed, 227 insertions(+), 21 deletions(-)

diff --git a/doc/src/sgml/trigger.sgml b/doc/src/sgml/trigger.sgml
index 2a718d7f47..5771bd5fdc 100644
--- a/doc/src/sgml/trigger.sgml
+++ b/doc/src/sgml/trigger.sgml
@@ -33,7 +33,8 @@

 A trigger is a specification that the database should automatically
 execute a particular function whenever a certain type of operation is
-performed.  Triggers can be attached to tables, views, and foreign tables.
+performed.  Triggers can be attached to tables (partitioned or not),
+views, and foreign tables.
   
 
   
@@ -111,14 +112,21 @@
 Statement-level BEFORE triggers naturally fire before the
 statement starts to do anything, while statement-level AFTER
 triggers fire at the very end of the statement.  These types of
-triggers may be defined on tables or views.  Row-level BEFORE
-triggers fire immediately before a particular row is operated on,
-while row-level AFTER triggers fire at the end of the
-statement (but before any statement-level AFTER triggers).
-These types of triggers may only be defined on tables and foreign tables.
-Row-level INSTEAD OF triggers may only be defined on views,
-and fire immediately as each row in the view is identified as needing to
-be operated on.
+triggers may be defined on tables, views, or foreign tables.  Row-level
+BEFORE triggers fire immediately before a particular row is
+operated on, while row-level AFTER triggers fire at the end of
+the statement (but before any statement-level AFTER triggers).
+These types of triggers may only be defined on non-partitioned tables and
+foreign tables.  Row-level INSTEAD OF triggers may only be
+defined on views, and fire immediately as each row in the view is
+identified as needing to be operated on.
+   
+
+   
+A statement-level trigger defined on partitioned tables is fired only
+once for the table itself, not once for every table in the partitioning
+hierarchy.  However, row-level triggers of any affected leaf partitions
+will be fired.

 

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 5c12fb457d..586b396b3e 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -861,18 +861,35 @@ InitPlan(QueryDesc *queryDesc, int eflags)
 
 		/*
 		 * In the partitioned result relation case, lock the non-leaf result
-		 * relations too.  We don't however need ResultRelInfos for them.
+		 * relations too.  We also need ResultRelInfos so that per-statement
+		 * triggers defined on these relations can be fired.
 		 */
 		if (plannedstmt->nonleafResultRelations)
 		{
+			numResultRelations =
+			

Re: [HACKERS] Logical replication in the same cluster

[Peter Eisentraut]

On 4/26/17 19:18, Michael Paquier wrote:
>> If that's a predictable deadlock, I think a minimum expectation is that
>> the system should notice it and throw an error, not just hang.  (Then
>> the error could give a hint about how to work around it.)  But the case
>> Bruce has in mind doesn't seem like a crazy use-case to me.  Can't we
>> make it "just work"?
> 
> Perhaps using some detection with the replication origins? Just an
> instinctive idea.. The current behavior is confusing for users, I have
> fallen into this trap a couple of times already.

We had some discussions early on about detecting connections to the same
server, but it's not entirely clear how to do that and it didn't seem
worth it at the time.

-- 
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] Logical replication in the same cluster

[Robert Haas]

On Wed, Apr 26, 2017 at 6:02 PM, Tom Lane  wrote:
> Petr Jelinek  writes:
>> On 26/04/17 18:59, Bruce Momjian wrote:
>>> ... it just hangs.  My server logs say:
>
>> Yes that's result of how logical replication slots work, the transaction
>> that needs to finish is your transaction. It can be worked around by
>> creating the slot manually via the SQL interface for example and create
>> the subscription using WITH (NOCREATE SLOT, SLOT NAME = 'your slot') .
>
> If that's a predictable deadlock, I think a minimum expectation is that
> the system should notice it and throw an error, not just hang.  (Then
> the error could give a hint about how to work around it.)  But the case
> Bruce has in mind doesn't seem like a crazy use-case to me.  Can't we
> make it "just work"?

+1.

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


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

Re: [HACKERS] Fix a typo in worker.c

[Peter Eisentraut]

On 4/26/17 12:43, Masahiko Sawada wrote:
> Attached patch for $subject.
> 
> s/strigs/strings/

done, thanks

-- 
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] Unportable implementation of background worker start

[Andres Freund]

On 2017-04-26 17:05:39 -0400, Tom Lane wrote:
> Here's an updated version of that, which makes use of our previous
> conclusion that F_SETFD/FD_CLOEXEC are available everywhere except
> Windows, and fixes some sloppy thinking about the EXEC_BACKEND case.
> 
> I went ahead and changed the call to epoll_create into epoll_create1.
> I'm not too concerned about loss of portability there --- it seems
> unlikely that many people are still using ten-year-old glibc, and
> even less likely that any of them would be interested in running
> current Postgres on their stable-unto-death platform.  We could add
> a configure test for epoll_create1 if you feel one's needed, but
> I think it'd just be a waste of cycles.

Yea, I think we can live with that.  If we find it's a problem, we can
add a configure test later.


> I propose to push this into HEAD and 9.6 too.

Cool.


Change looks good to me.


Greetings,

Andres Freund


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

Re: [HACKERS] Unportable implementation of background worker start

[Robert Haas]

On Wed, Apr 26, 2017 at 8:37 PM, Andres Freund  wrote:
>> 3. Go ahead with converting the postmaster to use WaitEventSet, a la
>> the draft patch I posted earlier.  I'd be happy to do this if we were
>> at the start of a devel cycle, but right now seems a bit late --- not
>> to mention that we really need to fix 9.6 as well.
>
> Yea, backpatching this to 9.6 seems like a bigger hammer than
> appropriate.  I'm on the fence WRT master, I think there's an argument
> to be made that this is going to become a bigger and bigger problem, and
> that we'll wish in a year or two that we had fewer releases with
> parallelism etc that don't use WaitEventSets.

I think changing this might be wise.  This problem isn't going away
for real until we do this, right?

-- 
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] [PostgreSQL 10] default of hot_standby should be "on"?

[Huong Dangminh]

Thanks all for your comments.

> Magnus Hagander  writes:
> > +1. I definitely think we should do it, and 10 would be the time to do
> it.
> 
> Agreed.  It's mainly a historical accident that the default is what it
> is,
> I think.
> 
> > I wonder if we should also consider changing the standby error message
> to
> > be a WARNING instead of an ERROR. So that if you try to start up a standby
> > with hot_standby=on but master with wal_level=replica it would turn into
> a
> > cold standby.
> 
> I'm -1 for that: if you fat-finger the configuration, you should be told
> about it, not have the system start up in an unintended mode that lacks
> critical functionality.
> 
>   regards, tom lane

I attached the patch which also update manual as the mention of sawada-san.

--- 
Thanks and best regards, 
Dang Minh Huong 
NEC Solution Innovators, Ltd. 
http://www.nec-solutioninnovators.co.jp/en/


hot_standby.patch
Description: hot_standby.patch

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

Re: [HACKERS] [PostgreSQL 10] default of hot_standby should be "on"?

[Vaishnavi Prabakaran]

On Wed, Apr 26, 2017 at 9:52 PM, Magnus Hagander 
wrote:

>
> I wonder if we should also consider changing the standby error message to
> be a WARNING instead of an ERROR. So that if you try to start up a standby
> with hot_standby=on but master with wal_level=replica it would turn into a
> cold standby.
>
> Perhaps, you mean hot_standby=off here?

Regards,
Vaishnavi
Fujitsu Australia.

Re: [HACKERS] Unportable implementation of background worker start

[Andres Freund]

On 2017-04-26 11:42:38 -0400, Tom Lane wrote:
> 1. Let HEAD stand as it is.  We have a problem with slow response to
> bgworker start requests that arrive while ServerLoop is active, but that's
> a pretty tight window usually (although I believe I've seen it hit at
> least once in testing).
> 
> 2. Reinstall the pselect patch, blacklisting NetBSD and HPUX and whatever
> else we find to be flaky.  Then only the blacklisted platforms have the
> problem.

That seems unattractive at this point.  I'm not looking forward to
having to debug more random platforms that implement this badly in a
yet another weird way.


> 3. Go ahead with converting the postmaster to use WaitEventSet, a la
> the draft patch I posted earlier.  I'd be happy to do this if we were
> at the start of a devel cycle, but right now seems a bit late --- not
> to mention that we really need to fix 9.6 as well.

Yea, backpatching this to 9.6 seems like a bigger hammer than
appropriate.  I'm on the fence WRT master, I think there's an argument
to be made that this is going to become a bigger and bigger problem, and
that we'll wish in a year or two that we had fewer releases with
parallelism etc that don't use WaitEventSets.


> I'm leaning to doing #1 plus the maybe_start_bgworker change.  There's
> certainly room for difference of opinion here, though.  Thoughts?

I see you did the bgworker thing - that seems good to me.

Thanks,

Andres


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

[HACKERS] Re: [BUGS] BUG #14629: ALTER TABLE VALIDATE CONSTRAINTS does not obey NO INHERIT clause

[Amit Langote]

On 2017/04/24 13:16, Amit Langote wrote:
> On 2017/04/22 3:40, buschm...@nidsa.net wrote:
>> The following bug has been logged on the website:
>>
>> Bug reference:  14629
>> Logged by:  Hans Buschmann
>> Email address:  buschm...@nidsa.net
>> PostgreSQL version: 9.6.2
>> Operating system:   Windows x64
>> Description:
>>
>>
>> Given these both tables (orders_archiv inherits orders) on 9.6.2 Windows
>> x64
>> (cut/paste from psql)
>>
>> xxxdb=# \d orders_archiv;
>> ...
>> Check constraints:
>> "ck_or_old" CHECK (or_season < 24) NO INHERIT
>> Inherits: orders
>>
>>
>> xxxdb=# \d orders
>> ...
>> Check constraints:
>> "ck_or_new" CHECK (or_season >= 24) NO INHERIT NOT VALID
>> Triggers:
>> tr_orders_insert BEFORE INSERT ON orders FOR EACH ROW WHEN
>> (new.or_season < 24) EXECUTE PROCEDURE fn_orders_insert()
>> Number of child tables: 1 (Use \d+ to list them.)
>>
>>
>> When applying these commands to the parent table, the following errors are
>> returned:
>>
>> xxxdb=# alter table orders validate constraint ck_or_new;
>> ERROR:  constraint "ck_or_new" of relation "orders_archiv" does not exist
>>
>>
>> xxxdb=# alter table only orders validate constraint ck_or_new;
>> ERROR:  constraint must be validated on child tables too
>>
>> Background:
>> From our original partitioning of quite a lot of tables according to
>> xx_season columns (a season is a half year period) I dropped and recreated
>> the check constraintsin a not valid state.
>>
>> At the end of the script to move the data from 2 seasons into the archiv
>> tables I tried to reenable the check constraints and encountered those two
>> errors.
>>
>> It seems that I can circumvent these errors by recreating the constraints
>> without the not valid clause.
>>
>> Do I miss something here ?
> 
> Looks indeed like a bug to me.  Performing VALIDATE CONSTRAINT on what is
> a non-inheritable constraint shouldn't look for that constraint in the
> child tables.  Attached patch fixes that.  Should be applied in all of the
> supported branches.

Should have included -hackers when posting the patch.  Here it is again
for -hackers' perusal.

Thanks,
Amit
>From 3687032a63906a7f71d97459fd362c2f9138b5b2 Mon Sep 17 00:00:00 2001
From: amit 
Date: Mon, 24 Apr 2017 11:33:05 +0900
Subject: [PATCH] Fix VALIDATE CONSTRAINT to consider NO INHERIT attribute

Currently, trying to validate a NO INHERIT constraint on the parent will
search for the constraint in child tables (where it is not supposed to
exist), wrongly causing a "constraint does not exist" error.
---
 src/backend/commands/tablecmds.c  |  5 +++--
 src/test/regress/expected/alter_table.out | 20 
 src/test/regress/sql/alter_table.sql  | 15 +++
 3 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index a02904c85c..626928658b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7680,9 +7680,10 @@ ATExecValidateConstraint(Relation rel, char *constrName, bool recurse,
 
 			/*
 			 * If we're recursing, the parent has already done this, so skip
-			 * it.
+			 * it.  Also, if the constraint is a NO INHERIT constraint, we
+			 * shouldn't try to look for it in the children.
 			 */
-			if (!recursing)
+			if (!recursing && !con->connoinherit)
 children = find_all_inheritors(RelationGetRelid(rel),
 			   lockmode, NULL);
 
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 883a5c9864..aed6964724 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -367,6 +367,26 @@ NOTICE:  merging constraint "identity" with inherited definition
 ALTER TABLE tmp3 VALIDATE CONSTRAINT identity;
 NOTICE:  boo: 16
 NOTICE:  boo: 20
+-- A NO INHERIT constraint should not be looked for in children during VALIDATE CONSTRAINT
+create table parent_noinh_convalid (a int);
+create table child_noinh_convalid () inherits (parent_noinh_convalid);
+insert into parent_noinh_convalid values (1);
+insert into child_noinh_convalid values (1);
+alter table parent_noinh_convalid add constraint check_a_is_2 check (a = 2) no inherit not valid;
+-- fail, because of the row in parent
+alter table parent_noinh_convalid validate constraint check_a_is_2;
+ERROR:  check constraint "check_a_is_2" is violated by some row
+delete from only parent_noinh_convalid;
+-- ok (parent itself contains no violating rows)
+alter table parent_noinh_convalid validate constraint check_a_is_2;
+select convalidated from pg_constraint where conrelid = 'parent_noinh_convalid'::regclass and conname = 'check_a_is_2';
+ convalidated 
+--
+ t
+(1 row)
+
+-- cleanup
+drop table parent_noinh_convalid, child_noinh_convalid;
 -- Try (and fail) to create constraint from tmp5(a) to tmp4(a) - unique constraint on
 -- tmp4 is a,b
 ALTER TABLE tmp5 add 

Re: [HACKERS] Fixup some misusage of appendStringInfo and friends

[David Rowley]

On 27 April 2017 at 06:41, Peter Eisentraut
 wrote:
> On 4/19/17 08:42, Ashutosh Bapat wrote:
>> I reviewed the patch. It compiles clean, make check-world passes. I do
>> not see any issue with it.
>
> Looks reasonable.  Let's keep it for the next commit fest.

Thank you to both of you for looking. I'd thought that maybe the new
stuff in PG10 should be fixed before the release. If we waited, and
fix in PG11 then backpatching is more of a pain.

However, I wasn't careful in the patch to touch only new to PG10 code.

I'll defer to your better judgment and add to the next 'fest.

Thanks

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


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

Re: [HACKERS] Logical replication in the same cluster

[Michael Paquier]

On Thu, Apr 27, 2017 at 7:02 AM, Tom Lane  wrote:
> Petr Jelinek  writes:
>> On 26/04/17 18:59, Bruce Momjian wrote:
>>> ... it just hangs.  My server logs say:
>
>> Yes that's result of how logical replication slots work, the transaction
>> that needs to finish is your transaction. It can be worked around by
>> creating the slot manually via the SQL interface for example and create
>> the subscription using WITH (NOCREATE SLOT, SLOT NAME = 'your slot') .
>
> If that's a predictable deadlock, I think a minimum expectation is that
> the system should notice it and throw an error, not just hang.  (Then
> the error could give a hint about how to work around it.)  But the case
> Bruce has in mind doesn't seem like a crazy use-case to me.  Can't we
> make it "just work"?

Perhaps using some detection with the replication origins? Just an
instinctive idea.. The current behavior is confusing for users, I have
fallen into this trap a couple of times already.
-- 
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] Foreign Join pushdowns not working properly for outer joins

[David Rowley]

On 27 April 2017 at 01:31, Peter Eisentraut
 wrote:
> committed

Great. Thanks!

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


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

Re: [HACKERS] WIP: [[Parallel] Shared] Hash

[Thomas Munro]

On Thu, Apr 27, 2017 at 5:13 AM, Oleg Golovanov  wrote:
> Can you actualize your patch set? The error got from
> 0010-hj-parallel-v12.patch.

I really should get around to setting up a cron job to tell me about
that.  Here's a rebased version.

The things currently on my list for this patch are:

1.  Implement the skew optimisation.
2.  Consider Andres's suggestion of splitting MultiExecHash into two
functions, serial and parallel version, rather than having all those
conditional blocks in there.
3.  Figure out whether the shared BufFile stuff I propose would work
well for Peter Geoghegan's parallel tuple sort patch, by trying it
(I've made a start, more soon).
4.  Figure out how the costing model needs to be tweaked, probably
based on experimentation.

I'm taking a short break to work on other things right now but will
post a version with those changes soon.

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


parallel-shared-hash-v13.tgz
Description: GNU Zip compressed data

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

Re: [HACKERS] Logical replication in the same cluster

[Tom Lane]

Petr Jelinek  writes:
> On 26/04/17 18:59, Bruce Momjian wrote:
>> ... it just hangs.  My server logs say:

> Yes that's result of how logical replication slots work, the transaction
> that needs to finish is your transaction. It can be worked around by
> creating the slot manually via the SQL interface for example and create
> the subscription using WITH (NOCREATE SLOT, SLOT NAME = 'your slot') .

If that's a predictable deadlock, I think a minimum expectation is that
the system should notice it and throw an error, not just hang.  (Then
the error could give a hint about how to work around it.)  But the case
Bruce has in mind doesn't seem like a crazy use-case to me.  Can't we
make it "just work"?

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] Logical replication in the same cluster

[Bruce Momjian]

On Wed, Apr 26, 2017 at 11:41:51PM +0200, Petr Jelinek wrote:
> On 26/04/17 18:59, Bruce Momjian wrote:
> > I tried setting up logical replication on the same server between two
> > different databases, and got, from database test:
> > 
> > test=> CREATE TABLE test (x INT PRIMARY KEY);
> > CREATE TABLE
> > test=>
> > test=> INSERT INTO test VALUES (1);
> > INSERT 0 1
> > test=> CREATE PUBLICATION mypub FOR TABLE test;
> > CREATE PUBLICATION
> > 
> > then from database test2:
> > 
> > test2=> CREATE TABLE test (x INT PRIMARY KEY);
> > CREATE TABLE
> > test2=> CREATE SUBSCRIPTION mysub CONNECTION 'dbname=test port=5432'
> > PUBLICATION mypub;
> > NOTICE:  synchronized table states
> > 
> > and it just hangs.  My server logs say:
> > 
> > 2017-04-26 12:50:53.694 EDT [29363] LOG:  logical decoding found initial
> > starting point at 0/15FF3E0
> > 2017-04-26 12:50:53.694 EDT [29363] DETAIL:  1 transaction needs to
> > finish.
> > 
> > Is this expected?  I can get it working from two different clusters.
> > 
> 
> Yes that's result of how logical replication slots work, the transaction
> that needs to finish is your transaction. It can be worked around by
> creating the slot manually via the SQL interface for example and create
> the subscription using WITH (NOCREATE SLOT, SLOT NAME = 'your slot') .

Oh, OK.  Is there a way we can give users a hint do that if they try
what I did?

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

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


-- 
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 in the same cluster

[Petr Jelinek]

On 26/04/17 18:59, Bruce Momjian wrote:
> I tried setting up logical replication on the same server between two
> different databases, and got, from database test:
> 
>   test=> CREATE TABLE test (x INT PRIMARY KEY);
>   CREATE TABLE
>   test=>
>   test=> INSERT INTO test VALUES (1);
>   INSERT 0 1
>   test=> CREATE PUBLICATION mypub FOR TABLE test;
>   CREATE PUBLICATION
> 
> then from database test2:
> 
>   test2=> CREATE TABLE test (x INT PRIMARY KEY);
>   CREATE TABLE
>   test2=> CREATE SUBSCRIPTION mysub CONNECTION 'dbname=test port=5432'
>   PUBLICATION mypub;
>   NOTICE:  synchronized table states
> 
> and it just hangs.  My server logs say:
> 
>   2017-04-26 12:50:53.694 EDT [29363] LOG:  logical decoding found initial
>   starting point at 0/15FF3E0
>   2017-04-26 12:50:53.694 EDT [29363] DETAIL:  1 transaction needs to
>   finish.
> 
> Is this expected?  I can get it working from two different clusters.
> 

Yes that's result of how logical replication slots work, the transaction
that needs to finish is your transaction. It can be worked around by
creating the slot manually via the SQL interface for example and create
the subscription using WITH (NOCREATE SLOT, SLOT NAME = 'your slot') .

-- 
  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] Unportable implementation of background worker start

[Tom Lane]

Andres Freund  writes:
> I'd still like to get something like your CLOEXEC patch applied
> independently however.

Here's an updated version of that, which makes use of our previous
conclusion that F_SETFD/FD_CLOEXEC are available everywhere except
Windows, and fixes some sloppy thinking about the EXEC_BACKEND case.

I went ahead and changed the call to epoll_create into epoll_create1.
I'm not too concerned about loss of portability there --- it seems
unlikely that many people are still using ten-year-old glibc, and
even less likely that any of them would be interested in running
current Postgres on their stable-unto-death platform.  We could add
a configure test for epoll_create1 if you feel one's needed, but
I think it'd just be a waste of cycles.

I propose to push this into HEAD and 9.6 too.

regards, tom lane

diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 0d0701a..946fbff 100644
*** a/src/backend/storage/ipc/latch.c
--- b/src/backend/storage/ipc/latch.c
*** static volatile sig_atomic_t waiting = f
*** 118,123 
--- 118,126 
  static int	selfpipe_readfd = -1;
  static int	selfpipe_writefd = -1;
  
+ /* Process owning the self-pipe --- needed for checking purposes */
+ static int	selfpipe_owner_pid = 0;
+ 
  /* Private function prototypes */
  static void sendSelfPipeByte(void);
  static void drainSelfPipe(void);
*** InitializeLatchSupport(void)
*** 146,152 
  #ifndef WIN32
  	int			pipefd[2];
  
! 	Assert(selfpipe_readfd == -1);
  
  	/*
  	 * Set up the self-pipe that allows a signal handler to wake up the
--- 149,189 
  #ifndef WIN32
  	int			pipefd[2];
  
! 	if (IsUnderPostmaster)
! 	{
! 		/*
! 		 * We might have inherited connections to a self-pipe created by the
! 		 * postmaster.  It's critical that child processes create their own
! 		 * self-pipes, of course, and we really want them to close the
! 		 * inherited FDs for safety's sake.
! 		 */
! 		if (selfpipe_owner_pid != 0)
! 		{
! 			/* Assert we go through here but once in a child process */
! 			Assert(selfpipe_owner_pid != MyProcPid);
! 			/* Release postmaster's pipe FDs; ignore any error */
! 			(void) close(selfpipe_readfd);
! 			(void) close(selfpipe_writefd);
! 			/* Clean up, just for safety's sake; we'll set these below */
! 			selfpipe_readfd = selfpipe_writefd = -1;
! 			selfpipe_owner_pid = 0;
! 		}
! 		else
! 		{
! 			/*
! 			 * Postmaster didn't create a self-pipe ... or else we're in an
! 			 * EXEC_BACKEND build, in which case it doesn't matter since the
! 			 * postmaster's pipe FDs were closed by the action of FD_CLOEXEC.
! 			 */
! 			Assert(selfpipe_readfd == -1);
! 		}
! 	}
! 	else
! 	{
! 		/* In postmaster or standalone backend, assert we do this but once */
! 		Assert(selfpipe_readfd == -1);
! 		Assert(selfpipe_owner_pid == 0);
! 	}
  
  	/*
  	 * Set up the self-pipe that allows a signal handler to wake up the
*** InitializeLatchSupport(void)
*** 154,176 
  	 * that SetLatch won't block if the event has already been set many times
  	 * filling the kernel buffer. Make the read-end non-blocking too, so that
  	 * we can easily clear the pipe by reading until EAGAIN or EWOULDBLOCK.
  	 */
  	if (pipe(pipefd) < 0)
  		elog(FATAL, "pipe() failed: %m");
  	if (fcntl(pipefd[0], F_SETFL, O_NONBLOCK) == -1)
! 		elog(FATAL, "fcntl() failed on read-end of self-pipe: %m");
  	if (fcntl(pipefd[1], F_SETFL, O_NONBLOCK) == -1)
! 		elog(FATAL, "fcntl() failed on write-end of self-pipe: %m");
  
  	selfpipe_readfd = pipefd[0];
  	selfpipe_writefd = pipefd[1];
  #else
  	/* currently, nothing to do here for Windows */
  #endif
  }
  
  /*
!  * Initialize a backend-local latch.
   */
  void
  InitLatch(volatile Latch *latch)
--- 191,220 
  	 * that SetLatch won't block if the event has already been set many times
  	 * filling the kernel buffer. Make the read-end non-blocking too, so that
  	 * we can easily clear the pipe by reading until EAGAIN or EWOULDBLOCK.
+ 	 * Also, make both FDs close-on-exec, since we surely do not want any
+ 	 * child processes messing with them.
  	 */
  	if (pipe(pipefd) < 0)
  		elog(FATAL, "pipe() failed: %m");
  	if (fcntl(pipefd[0], F_SETFL, O_NONBLOCK) == -1)
! 		elog(FATAL, "fcntl(F_SETFL) failed on read-end of self-pipe: %m");
  	if (fcntl(pipefd[1], F_SETFL, O_NONBLOCK) == -1)
! 		elog(FATAL, "fcntl(F_SETFL) failed on write-end of self-pipe: %m");
! 	if (fcntl(pipefd[0], F_SETFD, FD_CLOEXEC) == -1)
! 		elog(FATAL, "fcntl(F_SETFD) failed on read-end of self-pipe: %m");
! 	if (fcntl(pipefd[1], F_SETFD, FD_CLOEXEC) == -1)
! 		elog(FATAL, "fcntl(F_SETFD) failed on write-end of self-pipe: %m");
  
  	selfpipe_readfd = pipefd[0];
  	selfpipe_writefd = pipefd[1];
+ 	selfpipe_owner_pid = MyProcPid;
  #else
  	/* currently, nothing to do here for Windows */
  #endif
  }
  
  /*
!  * Initialize a process-local latch.
   */
  void
  InitLatch(volatile Latch *latch)

Re: [HACKERS] [POC] hash partitioning

[Robert Haas]

On Thu, Apr 20, 2017 at 4:27 PM, Robert Haas  wrote:
> So, I think we really need something like the syntax in Amul's patch
> in order for this to work at all.  Of course, the details can be
> changed according to what seems best but I think the overall picture
> is about right.

I spent some time today looking at these patches.  It seems like there
is some more work still needed here to produce something committable
regardless of which way we go, but I am inclined to think that Amul's
patch is a better basis for work going forward than Nagata-san's
patch. Here are some general comments on the two patches:

- As noted above, the syntax implemented by Amul's patch allows us to
know the final partition constraint right away.  Nagata-san's proposed
syntax does not do that.  Also, Amul's syntax allows for a way to
split partitions (awkwardly, but we can improve it later);
Nagata-san's doesn't provide any method at all.

- Amul's patch derives the hash function to be used from the relevant
hash opclass, whereas Nagata-san's patch requires the user to specify
it explicitly.  I think that there is no real use case for a user
providing a custom hash function, and that using the opclass machinery
to derive the function to be used is better.  If a user DOES want to
provide their own, they can always create a custom opclass with the
appropriate support function and specify that it should be used when
creating a hash-partitioned table, but most users will be happy for
the system to supply the appropriate function automatically.

- In Nagata-san's patch, convert_expr_for_hash() looks up a function
called "abs" and an operator called "%" by name, which is not a good
idea.  We don't want to just find whatever is in the current search
path; we want to make sure we're using the system-defined operators
that we intend to be using.  Amul's patch builds the constraint using
a hard-coded internal function OID, F_SATISFIES_HASH_PARTITION.
That's a lot more robust, and it's also likely to be faster because,
in Amul's patch, we only call one function at the SQL level
(satisfies_hash_partition), whereas in Nagata-san's patch, we'll end
up calling three (abs, %, =).  Nagata-san's version of
get_qual_for_hash is implicated in this problem, too: it's looking up
the operator to use based on the operator name (=) rather than the
opclass properties.  Note that the existing get_qual_for_list() and
get_qual_for_range() use opclass properties, as does Amul's patch.

- Nagata-san's patch only supports hash partitioning based on a single
column, and that column must be NOT NULL.  Amul's patch does not have
these restrictions.

- Neither patch contains any documentation updates, which is bad.
Nagata-san's patch also contains no regression tests.  Amul's patch
does, but they need to be rebased, since they no longer apply, and I
think some other improvements are possible as well.  It's probably not
necessary to re-test things like whether temp and non-temp tables can
be mixed within a partitioning hierarchy, but there should be tests
that tuple routing actually works.  The case where it fails because no
matching partition exists should be tested as well.  Also, the tests
should validate not only that FOR VALUES isn't accept when creating a
hash partition (which they do) but also that WITH (...) isn't accepted
for a range or list partition (which they do not).

- When I try to do even something pretty trivial with Nagata-san's
patches, it crashes:

rhaas=# create table foo (a int, b text) partition by hash (a)
partitions 7 using hashint4;
CREATE TABLE
rhaas=# create table foo1 partition of foo;


The ruleutils.c support in Nagata-san's patch is broken.  If you
execute the non-crashing statement from the above example and then run
pg_dump, it doesn't dump "partitions 7 using hashint4", which means
that the syntax in the dump is invalid.

- Neither patch does anything about the fact that constraint exclusion
won't work for hash partitioning.  I mentioned this issue upthread in
the last paragraph of
http://postgr.es/m/CA+Tgmob7RsN5A=ehgYbLPx--c5CmptrK-dB=y-v--o+tkyf...@mail.gmail.com
and I think it's imperative that we fix it in some way before we think
about committing any of this.  I think that needs to be done by
extending relation_excluded_by_constraints() to have some specific
smarts about hash partitioning, and maybe other kinds of partitioning
as well (because it could probably be made much faster for list and
range partitioning, too).

- Amul's patch should perhaps update tab completion support:  create
table foo1 partition of foo  completes with "for values", but now
"with" will be another option.

- Amul's patch probably needs to validate the WITH () clause more
thoroughly.  I bet you get a not-very-great error message if you leave
out "modulus" and no error at all if you leave out "remainder".

This is not yet a detailed review - I may be missing things, and
review and commentary from others is welcome.  If there 

Re: [HACKERS] Cached plans and statement generalization

[Konstantin Knizhnik]

On 04/26/2017 08:08 PM, Doug Doole wrote:


A naive option would be to invalidate anything that depends on table or 
view *.FOOBAR. You could probably make it a bit smarter by also requiring that 
schema A appear in the path.


This has been rumbling around in my head. I wonder if you could solve this 
problem by registering dependencies on objects which don't yet exist. Consider:

CREATE TABLE C.T1(...);
CREATE TABLE C.T2(...);
SET search_path='A,B,C,D';
SELECT * FROM C.T1, T2;

For T1, you'd register a hard dependency on C.T1 and no virtual dependencies 
since the table is explicitly qualified.

For T2, you'd register a hard dependency on C.T2 since that is the table that was selected for the query. You'd also register virtual dependencies on A.T2 and B.T2 since if either of those tables (or views) are created you need to recompile the 
statement. (Note that no virtual dependency is created on D.T2() since that table would never be selected by the compiler.)


The catch is that virtual dependencies would have to be recorded and searched 
as strings, not OIDs since the objects don't exist. Virtual dependencies only 
have to be checked during CREATE processing though, so that might not be too 
bad.

But this is getting off topic - I just wanted to capture the idea while it was 
rumbling around.


I think that it will be enough to handle modification of search path and 
invalidate prepared statements cache in this case.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] RFC: ALTER SYSTEM [...] COMMENT

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> It'd be better to invent inverse pg_get_comment and pg_set_comment
>> functions, then you could do bulk-update things like
>> select pg_set_comment('table', pg_get_comment('table') || ' more')
>> from pg_class where ...

> Of course, once we start thinking about what kind of comments people
> might be interested in, as alluded to elsewhere, it's entirely likely
> they'll want to get things like timestamps included and other
> information that, ultimately, would be better if it was structured and
> not just thrown together into a free-form text field.

It's not hard to imagine people deciding that all their comments will
be (the text form of) JSON objects containing certain fields.  But
I don't think it's appropriate for us to mandate that sort of thing.
Given get/set comment functions as above, users could write wrapper
functions implementing their local conventions.

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] Fixup some misusage of appendStringInfo and friends

[Peter Eisentraut]

On 4/19/17 08:42, Ashutosh Bapat wrote:
> I reviewed the patch. It compiles clean, make check-world passes. I do
> not see any issue with it.

Looks reasonable.  Let's keep it for the next commit fest.

> 
> On Wed, Apr 19, 2017 at 9:13 AM, David Rowley
>  wrote:
>> The attached cleans up a few small misusages of appendStringInfo and
>> related functions.
>>
>> Some similar work was done in,
>>
>> f92d6a540ac443f85f0929b284edff67da14687a
>> d02f16470f117db3038dbfd87662d5f0eb5a2a9b
>> cacbdd78106526d7c4f11f90b538f96ba8696fb0
>>
>> so the majority of these should all be new misuseages.
>>
>> --
>>  David Rowley   http://www.2ndQuadrant.com/
>>  PostgreSQL Development, 24x7 Support, Training & Services
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>>
> 
> 
> 


-- 
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] RFC: ALTER SYSTEM [...] COMMENT

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > Having COMMENT ON accept a general query whose result is then cast to
> > text and stored as the comment would allow this to be done, eg:
> 
> > COMMENT ON table IS (pg_get_comment('table') || ' new text');
> 
> Putting general subexpressions into utility statements has some
> implementation issues.  Plus, it's not really all that powerful.
> It'd be better to invent inverse pg_get_comment and pg_set_comment
> functions, then you could do bulk-update things like
> 
>   select pg_set_comment('table', pg_get_comment('table') || ' more')
>   from pg_class where ...
> 
> The main thing lacking to make that into a real proposal would be
> a way of naming the object the comment is for; but I think Alvaro's
> already exposed something corresponding to ObjectAddress to SQL, no?

Yes and yes.  I like the 'pg_set_comment' idea, and I think we do have
something or other now through ObjectAddress and friends.

> > We could also have new syntax along these lines, for this specific case:
> > COMMENT ON table ADD ' new text';
> 
> Wouldn't have a big problem with that, as it'd address a common case
> for not much work.

We could have this also, I suppose.

Of course, once we start thinking about what kind of comments people
might be interested in, as alluded to elsewhere, it's entirely likely
they'll want to get things like timestamps included and other
information that, ultimately, would be better if it was structured and
not just thrown together into a free-form text field.  Not sure how much
we want to try and go down that road, or if we are happy to tell people
to just use:

select pg_set_comment('table','q',pg_get_comment('table','q') || 'E\n' || now() 
|| ': new comment'));

Not sure why anyone would grouse about that though ...

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] tablesync patch broke the assumption that logical rep depends on?

[Tom Lane]

Jeff Janes  writes:
> This gives me compiler warning:
> launcher.c: In function 'logicalrep_worker_launch':
> launcher.c:257: warning: 'slot' may be used uninitialized in this function

Yeah, me too.  Fix pushed.

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] RFC: ALTER SYSTEM [...] COMMENT

[Joshua D. Drake]

On 04/26/2017 10:31 AM, Tom Lane wrote:

"Joshua D. Drake"  writes:

I wouldn't fight hard to change it but really if we think about it, what
makes more sense from usability perspective?



CREATE TABLE foo() COMMENT IS


I think it's likely to be impossible to shoehorn such a thing into every
type of CREATE command without making COMMENT a fully reserved word,
which is going to be a very hard sell.


Well if it is a complete uphill battle, this is certainly not the 
feature that I am going to dig my heels in about.





2. Make it so comments are appended not replaced.


Backwards compatibility fail ... not to mention that you haven't offered
an argument as to why everyone would think this is an improvement.


"Everyone" is a bit of a stretch for every single feature we have.

I would think that most people that work with production systems would 
like to know the history of any object modification.


Thanks,

jD





regards, tom lane




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


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

Re: [HACKERS] RFC: ALTER SYSTEM [...] COMMENT

[Tom Lane]

Stephen Frost  writes:
> Having COMMENT ON accept a general query whose result is then cast to
> text and stored as the comment would allow this to be done, eg:

> COMMENT ON table IS (pg_get_comment('table') || ' new text');

Putting general subexpressions into utility statements has some
implementation issues.  Plus, it's not really all that powerful.
It'd be better to invent inverse pg_get_comment and pg_set_comment
functions, then you could do bulk-update things like

select pg_set_comment('table', pg_get_comment('table') || ' more')
from pg_class where ...

The main thing lacking to make that into a real proposal would be
a way of naming the object the comment is for; but I think Alvaro's
already exposed something corresponding to ObjectAddress to SQL, no?

> We could also have new syntax along these lines, for this specific case:
> COMMENT ON table ADD ' new text';

Wouldn't have a big problem with that, as it'd address a common case
for not much work.

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] [PATCH] Incremental sort

[Alexander Korotkov]

On Wed, Apr 26, 2017 at 8:20 PM, Peter Geoghegan  wrote:

> On Wed, Apr 26, 2017 at 10:10 AM, Alexander Korotkov
>  wrote:
> > OK, I get it.  Our qsort is so fast not only on 100% presorted case.
> > However, that doesn't change many things in context of incremental sort.
>
> The important point is to make any presorted test case only ~99%
> presorted, so as to not give too much credit to the "high risk"
> presort check optimization.
>
> The switch to insertion sort that we left in (not the bad one removed
> by a3f0b3d -- the insertion sort that actually comes from the B
> paper) does "legitimately" make sorting faster with presorted cases.


I'm still focusing on making incremental sort not slower than qsort with
presorted optimization.  Independently on whether this is "high risk"
optimization or not...
However, adding more test cases is always good.

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

Re: [HACKERS] RFC: ALTER SYSTEM [...] COMMENT

[Hunley, Douglas]

On Wed, Apr 26, 2017 at 1:03 PM, Joshua D. Drake 
wrote:

> Problem we are trying to solve:
>
> Having documentation for changes to GUC parameters that are modified via
> ALTER SYSTEM.
>
> Why?
>
> Because documentation is good and required for a proper production system.
>
> How?
>

+1 for commenting on ALTER SYSTEM (regardless of the syntax)

-- 
{
  "name" : "douglas j hunley",
  "title" : "database engineer",
  "email" : "douglas.hun...@openscg.com ",
  "mobile" : "+1 614 316 5079"
}

Re: [HACKERS] RFC: ALTER SYSTEM [...] COMMENT

[Tom Lane]

"Joshua D. Drake"  writes:
> I wouldn't fight hard to change it but really if we think about it, what 
> makes more sense from usability perspective?

> CREATE TABLE foo() COMMENT IS

I think it's likely to be impossible to shoehorn such a thing into every
type of CREATE command without making COMMENT a fully reserved word,
which is going to be a very hard sell.

> 2. Make it so comments are appended not replaced.

Backwards compatibility fail ... not to mention that you haven't offered
an argument as to why everyone would think this is an improvement.

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] StandbyRecoverPreparedTransactions recovers subtrans links incorrectly

[Tom Lane]

Simon Riggs  writes:
> I've added code following Michael and Tom's comments to the previous
> patch. New patch attached.

Couple of minor suggestions:

* Rather than deleting the comment for SubTransSetParent entirely,
maybe make it say "It's possible that the parent was already recorded.
However, we should never be asked to change an already-set entry to
something else."

* In SubTransGetTopmostTransaction, maybe it'd be better to spell
"TransactionIdFollowsOrEquals" as "!TransactionIdPrecedes" to make
it look more like the test just above.  Matter of taste though.

* Less a matter of taste is that I think that should be just elog(ERROR);
there's no good reason to make it FATAL.

* Also, I think there should be a comment there, along the lines of
"check for reversed linkage to ensure this isn't an infinite loop".

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] RFC: ALTER SYSTEM [...] COMMENT

[Joshua D. Drake]

On 04/26/2017 10:14 AM, Stephen Frost wrote:

JD,



Having COMMENT ON accept a general query whose result is then cast to
text and stored as the comment would allow this to be done, eg:

COMMENT ON table IS (pg_get_comment('table') || ' new text');


Dig it, although we probably want the equivalent of:

COMMENT ON table IS (pg_get_comment('table') || '\n\n' || ' new text');

Or something like that.



We could also have new syntax along these lines, for this specific case:

COMMENT ON table ADD ' new text';

Though we have this pretty powerful language, seems a bit of a shame to
invent something new for working with comments.


Agreed and I think that using existing COMMENT ON capability is likely 
to get this pushed farther down the road.


I wouldn't fight hard to change it but really if we think about it, what 
makes more sense from usability perspective?


CREATE TABLE foo() COMMENT IS

or

CREATE TABLE foo;
COMMENT ON TABLE foo IS

Thanks,

JD




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


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

Re: [HACKERS] RFC: ALTER SYSTEM [...] COMMENT

[Thom Brown]

On 26 April 2017 at 18:03, Joshua D. Drake  wrote:
> -hackers,
>
> We have had ALTER SYSTEM for some time now. It is awesome to be able to make
> changes that can be system wide without ever having to hit a shell but it
> does lack a feature that seems like an oversight, the ability to comment.
>
> Problem we are trying to solve:
>
> Having documentation for changes to GUC parameters that are modified via
> ALTER SYSTEM.
>
> Why?
>
> Because documentation is good and required for a proper production system.
>
> How?
>
> I propose:
>
> Add a column to pg_settings comment(text)
> Change the grammar to allow:
>
> ALTER SYSTEM SET configuration_parameter { TO | = } { value | 'value' |
> DEFAULT } COMMENT 'comment'
>
> Example:
>
> ALTER SYSTEM SET maintenance_work_mem TO '1GB' COMMENT IS 'Increased to
> allow autovacuum to be more efficient';
>
> Potential issues:
>
> Does not use existing comment functionality. Alternate solution which would
> decrease functionality is:
>
> COMMENT ON SETTING setting IS 'comment';
>
> Looking forward, we may want to do the following:
>
> 1. Make it so any object can have a comment with creation, e.g;
>
> CREATE TABLE table () COMMENT IS '';
>
> 2. Make it so comments are appended not replaced.

Yeah, this is something I've also suggested previously:

https://www.postgresql.org/message-id/CAA-aLv50MZdjdVk_=Tep6na94dNmi1Y9XkCp3er7FQqvX=d...@mail.gmail.com

Thom


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

Re: [HACKERS] [PATCH] Incremental sort

[Peter Geoghegan]

On Wed, Apr 26, 2017 at 10:10 AM, Alexander Korotkov
 wrote:
> OK, I get it.  Our qsort is so fast not only on 100% presorted case.
> However, that doesn't change many things in context of incremental sort.

The important point is to make any presorted test case only ~99%
presorted, so as to not give too much credit to the "high risk"
presort check optimization.

The switch to insertion sort that we left in (not the bad one removed
by a3f0b3d -- the insertion sort that actually comes from the B
paper) does "legitimately" make sorting faster with presorted cases.

-- 
Peter Geoghegan

VMware vCenter Server
https://www.vmware.com/


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

Re: [HACKERS] RFC: ALTER SYSTEM [...] COMMENT

[Stephen Frost]

JD,

* Joshua D. Drake (j...@commandprompt.com) wrote:
> Does not use existing comment functionality. Alternate solution
> which would decrease functionality is:
> 
> COMMENT ON SETTING setting IS 'comment';

That seems like a pretty reasonable idea, at least from where I sit.

> Looking forward, we may want to do the following:
> 
> 1. Make it so any object can have a comment with creation, e.g;
> 
> CREATE TABLE table () COMMENT IS '';

I'm pretty sure this has been discussed previously.

> 2. Make it so comments are appended not replaced.

Having COMMENT ON accept a general query whose result is then cast to
text and stored as the comment would allow this to be done, eg:

COMMENT ON table IS (pg_get_comment('table') || ' new text');

We could also have new syntax along these lines, for this specific case:

COMMENT ON table ADD ' new text';

Though we have this pretty powerful language, seems a bit of a shame to
invent something new for working with comments.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] Re: [HACKERS] WIP: [[Parallel] Shared] Hash

[Oleg Golovanov]

Hi.

Thanks for rebased patch set v12. Currently I try to use this patch on my new 
test site and get following:

Hmm...  The next patch looks like a unified diff to me...
The text leading up to this was:
--
|diff --git a/src/include/access/parallel.h b/src/include/access/parallel.h
|index bdf15621c83..e9db8880161 100644
|--- a/src/include/access/parallel.h
|+++ b/src/include/access/parallel.h
--
patching file src/include/access/parallel.h
Using Plan A...
Hunk #1 FAILED at 58.
1 out of 1 hunk FAILED -- saving rejects to file 
src/include/access/parallel.h.rej

Can you actualize your patch set? The error got from 0010-hj-parallel-v12.patch.

Best Regards,

Oleg Golovanov
Moscow, Russia

>Четверг, 13 апреля 2017, 13:49 +03:00 от Thomas Munro 
>:
>
>On Thu, Apr 13, 2017 at 10:04 PM, Oleg Golovanov < rent...@mail.ru > wrote:
>> bash-4.2$ grep Hunk *.log | grep FAILED
>> 0005-hj-leader-gate-v11.patch.log:Hunk #1 FAILED at 14.
>> 0010-hj-parallel-v11.patch.log:Hunk #2 FAILED at 2850.
>> 0010-hj-parallel-v11.patch.log:Hunk #1 FAILED at 21.
>> 0010-hj-parallel-v11.patch.log:Hunk #3 FAILED at 622.
>> 0010-hj-parallel-v11.patch.log:Hunk #6 FAILED at 687.
>> 0010-hj-parallel-v11.patch.log:Hunk #1 FAILED at 21.
>> 0010-hj-parallel-v11.patch.log:Hunk #3 FAILED at 153.
>
>Hi Oleg
>
>Thanks for looking at this.  It conflicted with commit 9c7f5229.  Here
>is a rebased patch set.
>
>This version also removes some code for dealing with transient record
>types which didn't work out.  I'm trying to deal with that problem
>separately[1] and in a general way so that the parallel hash join
>patch doesn't have to deal with it at all.
>
>[1]  
>https://www.postgresql.org/message-id/CAEepm=0ZtQ-SpsgCyzzYpsXS6e=kzwqk3g5ygn3mdv7a8da...@mail.gmail.com
>
>-- 
>Thomas Munro
>http://www.enterprisedb.com
>
>-- 
>Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
>To make changes to your subscription:
>http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Incremental sort

[Alexander Korotkov]

On Wed, Apr 26, 2017 at 7:56 PM, Peter Geoghegan  wrote:

> On Wed, Apr 26, 2017 at 8:39 AM, Alexander Korotkov
>  wrote:
> > That appears to be wrong.  I intended to make cost_sort prefer plain sort
> > over incremental sort for this dataset size.  But, that appears to be not
> > always right solution.  Quick sort is so fast only on presorted data.
>
> As you may know, I've often said that the precheck for sorted input
> added to our quicksort implementation by a3f0b3d is misguided. It
> sometimes throws away a ton of work if the presorted input isn't
> *perfectly* presorted. This happens when the first out of order tuple
> is towards the end of the presorted input.
>
> I think that it isn't fair to credit our qsort with doing so well on a
> 100% presorted case, because it doesn't do the necessary bookkeeping
> to not throw that work away completely in certain important cases.
>

OK, I get it.  Our qsort is so fast not only on 100% presorted case.
However, that doesn't change many things in context of incremental sort.

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

Re: [HACKERS] Cached plans and statement generalization

[Doug Doole]

>
> A naive option would be to invalidate anything that depends on table or
> view *.FOOBAR. You could probably make it a bit smarter by also requiring
> that schema A appear in the path.
>

This has been rumbling around in my head. I wonder if you could solve this
problem by registering dependencies on objects which don't yet exist.
Consider:

CREATE TABLE C.T1(...);
CREATE TABLE C.T2(...);
SET search_path='A,B,C,D';
SELECT * FROM C.T1, T2;

For T1, you'd register a hard dependency on C.T1 and no virtual
dependencies since the table is explicitly qualified.

For T2, you'd register a hard dependency on C.T2 since that is the table
that was selected for the query. You'd also register virtual dependencies
on A.T2 and B.T2 since if either of those tables (or views) are created you
need to recompile the statement. (Note that no virtual dependency is
created on D.T2() since that table would never be selected by the compiler.)

The catch is that virtual dependencies would have to be recorded and
searched as strings, not OIDs since the objects don't exist. Virtual
dependencies only have to be checked during CREATE processing though, so
that might not be too bad.

But this is getting off topic - I just wanted to capture the idea while it
was rumbling around.

[HACKERS] RFC: ALTER SYSTEM [...] COMMENT

[Joshua D. Drake]

-hackers,

We have had ALTER SYSTEM for some time now. It is awesome to be able to 
make changes that can be system wide without ever having to hit a shell 
but it does lack a feature that seems like an oversight, the ability to 
comment.


Problem we are trying to solve:

Having documentation for changes to GUC parameters that are modified via 
ALTER SYSTEM.


Why?

Because documentation is good and required for a proper production system.

How?

I propose:

Add a column to pg_settings comment(text)
Change the grammar to allow:

ALTER SYSTEM SET configuration_parameter { TO | = } { value | 'value' | 
DEFAULT } COMMENT 'comment'


Example:

ALTER SYSTEM SET maintenance_work_mem TO '1GB' COMMENT IS 'Increased to 
allow autovacuum to be more efficient';


Potential issues:

Does not use existing comment functionality. Alternate solution which 
would decrease functionality is:


COMMENT ON SETTING setting IS 'comment';

Looking forward, we may want to do the following:

1. Make it so any object can have a comment with creation, e.g;

CREATE TABLE table () COMMENT IS '';

2. Make it so comments are appended not replaced.

Thanks in advance,

JD



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


--
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] scram and \password

[Robert Haas]

On Wed, Apr 26, 2017 at 12:57 PM, Tom Lane  wrote:
> Would it be worth making password_encryption be GUC_REPORT so that
> it could be guaranteed available, without a server transaction,
> from any valid connection?  I'm generally resistant to adding
> GUC_REPORT flags, but maybe this is a time for an exception.

Well, as Heikki just wrote a few messages upthread:

---
As an alternative, I considered making password_encryption GUC_REPORT,
so that libpq would always know it without having to issue a query.
But it feels wrong to send that option to the client on every
connection, when it's only needed in the rare case that you use
PQencryptPasswordConn(). And if we added new settings like the
iteration count in the future, those would need to be GUC_REPORT too.
---

I think those are both good points.

-- 
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] StandbyRecoverPreparedTransactions recovers subtrans links incorrectly

[Simon Riggs]

On 26 April 2017 at 15:28, Tom Lane  wrote:
> Nikhil Sontakke  writes:
>> A SELECT query on the newly promoted master on any of the tables involved
>> in the 2PC hangs. The hang is due to a loop in
>> SubTransGetTopmostTransaction(). Due to incorrect linkages, we get a
>> circular reference in parentxid <-> subxid inducing the infinite loop.
>
> I'm inclined to propose that
>
> (1) SubTransSetParent should contain an assert that
> Assert(TransactionIdFollows(xid, parent));
> There is a similar assertion near one of the call sites, but that's
> obviously too far away from the action.
>
> (2) SubTransGetTopmostTransaction ought to contain an actual runtime
> test and ereport (not just Assert) that the parent XID it got from
> pg_subtrans precedes the child XID that was looked up.  This would
> protect us against similar infinite loops in the future.  Even without
> bugs, such a loop could arise due to corrupted data.

Pretty much what I was thinking.


I've added code following Michael and Tom's comments to the previous
patch. New patch attached.

Passes with and without Nikhil's new test.

I plan to apply both patches tomorrow morning my time to give time for
further comments.

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


2PC_rework.v2.patch
Description: Binary data

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

[HACKERS] Logical replication in the same cluster

[Bruce Momjian]

I tried setting up logical replication on the same server between two
different databases, and got, from database test:

test=> CREATE TABLE test (x INT PRIMARY KEY);
CREATE TABLE
test=>
test=> INSERT INTO test VALUES (1);
INSERT 0 1
test=> CREATE PUBLICATION mypub FOR TABLE test;
CREATE PUBLICATION

then from database test2:

test2=> CREATE TABLE test (x INT PRIMARY KEY);
CREATE TABLE
test2=> CREATE SUBSCRIPTION mysub CONNECTION 'dbname=test port=5432'
PUBLICATION mypub;
NOTICE:  synchronized table states

and it just hangs.  My server logs say:

2017-04-26 12:50:53.694 EDT [29363] LOG:  logical decoding found initial
starting point at 0/15FF3E0
2017-04-26 12:50:53.694 EDT [29363] DETAIL:  1 transaction needs to
finish.

Is this expected?  I can get it working from two different clusters.

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

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


-- 
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] scram and \password

[Tom Lane]

Robert Haas  writes:
> On Wed, Apr 26, 2017 at 6:22 AM, Heikki Linnakangas  wrote:
>> * If algorithm is not given explicitly, PQencryptPasswordConn() queries
>> "SHOW password_encryption", and uses that. That is documented, and it is
>> also documented that it will *not* issue queries, and hence will not block,
>> if the algorithm is given explicitly. That's an important property for some
>> applications. If you want the default behavior without blocking, query "SHOW
>> password_encryption" yourself, and pass the result as the 'algorithm'.

> TBH, I'd just require the user to specify the algorithm explicitly.
> Having it run SHOW on the server seems wonky.  It introduces a bunch
> of failure modes for ... no real benefit, I think.

Yeah.  Blocking is the least of your worries --- what about being in
a failed transaction, for instance?

However, it's not entirely true that there's no real benefit.  If the
client app has to specify the algorithm then client code will need
extension every time we add another algorithm.  Maybe that's going to
happen so seldom that it's not a big deal, but it would be nice to
avoid that.

Would it be worth making password_encryption be GUC_REPORT so that
it could be guaranteed available, without a server transaction,
from any valid connection?  I'm generally resistant to adding
GUC_REPORT flags, but maybe this is a time for an exception.

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] Partition-wise aggregation/grouping

[Robert Haas]

On Wed, Apr 26, 2017 at 6:28 AM, Antonin Houska  wrote:
> Attached is a diff that contains both patches merged. This is just to prove my
> assumption, details to be elaborated later. The scripts attached produce the
> following plan in my environment:
>
>QUERY PLAN
> 
>  Parallel Finalize HashAggregate
>Group Key: b_1.j
>->  Append
>  ->  Parallel Partial HashAggregate
>Group Key: b_1.j
>->  Hash Join
>  Hash Cond: (b_1.j = c_1.k)
>  ->  Seq Scan on b_1
>  ->  Hash
>->  Seq Scan on c_1
>  ->  Parallel Partial HashAggregate
>Group Key: b_2.j
>->  Hash Join
>  Hash Cond: (b_2.j = c_2.k)
>  ->  Seq Scan on b_2
>  ->  Hash
>->  Seq Scan on c_2

Well, I'm confused.  I see that there's a relationship between what
Antonin is trying to do and what Jeevan is trying to do, but I can't
figure out whether one is a subset of the other, whether they're both
orthogonal, or something else.  This plan looks similar to what I
would expect Jeevan's patch to produce, except i have no idea what
"Parallel" would mean in a plan that contains no Gather node.

-- 
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] [PATCH] Incremental sort

[Peter Geoghegan]

On Wed, Apr 26, 2017 at 8:39 AM, Alexander Korotkov
 wrote:
> That appears to be wrong.  I intended to make cost_sort prefer plain sort
> over incremental sort for this dataset size.  But, that appears to be not
> always right solution.  Quick sort is so fast only on presorted data.

As you may know, I've often said that the precheck for sorted input
added to our quicksort implementation by a3f0b3d is misguided. It
sometimes throws away a ton of work if the presorted input isn't
*perfectly* presorted. This happens when the first out of order tuple
is towards the end of the presorted input.

I think that it isn't fair to credit our qsort with doing so well on a
100% presorted case, because it doesn't do the necessary bookkeeping
to not throw that work away completely in certain important cases.

-- 
Peter Geoghegan

VMware vCenter Server
https://www.vmware.com/


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

Re: [HACKERS] Declarative partitioning - another take

[Robert Haas]

On Tue, Apr 25, 2017 at 10:34 PM, Amit Langote
 wrote:
> FWIW, I too prefer the latter, that is, fire only the parent's triggers.
> In that case, applying only the patch 0001 will do.

Do we need to update the documentation?

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


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

[HACKERS] Fix a typo in worker.c

[Masahiko Sawada]

HI,

Attached patch for $subject.

s/strigs/strings/

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


fix_typo_in_worker_c.patch
Description: Binary data

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

Re: [HACKERS] Partition-wise join for join between (declaratively) partitioned tables

[Robert Haas]

On Wed, Apr 26, 2017 at 12:41 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> So this is about a cross-type join,
>> not multiple types within a single partitioning hierarchy, as you
>> might also gather from the subject line of this thread.
>
> OK, but I still don't understand why any type conversion is needed
> in such a case.  The existing join estimators don't try to do that,
> for the good and sufficient reasons you and I have already mentioned.
> They just apply the given cross-type join operator, and whatever
> cross-type selectivity estimator might be associated with it, and
> possibly other cross-type operators obtained from the same btree
> opfamily.
>
> The minute you get into trying to do any type conversion that is not
> mandated by the semantics of the query as written, you're going to
> have problems.

There is no daylight whatsoever between us on this issue.

-- 
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] Partition-wise join for join between (declaratively) partitioned tables

[Tom Lane]

Robert Haas  writes:
> So this is about a cross-type join,
> not multiple types within a single partitioning hierarchy, as you
> might also gather from the subject line of this thread.

OK, but I still don't understand why any type conversion is needed
in such a case.  The existing join estimators don't try to do that,
for the good and sufficient reasons you and I have already mentioned.
They just apply the given cross-type join operator, and whatever
cross-type selectivity estimator might be associated with it, and
possibly other cross-type operators obtained from the same btree
opfamily.

The minute you get into trying to do any type conversion that is not
mandated by the semantics of the query as written, you're going to
have problems.

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] scram and \password

[Robert Haas]

On Wed, Apr 26, 2017 at 6:22 AM, Heikki Linnakangas  wrote:
> On 04/25/2017 06:26 PM, Heikki Linnakangas wrote:
>> Thoughts? Unless someone has better ideas or objections, I'll go
>> implement that.
> This is what I came up with in the end. Some highlights and differences vs
> the plan I posted earlier:
>
> * If algorithm is not given explicitly, PQencryptPasswordConn() queries
> "SHOW password_encryption", and uses that. That is documented, and it is
> also documented that it will *not* issue queries, and hence will not block,
> if the algorithm is given explicitly. That's an important property for some
> applications. If you want the default behavior without blocking, query "SHOW
> password_encryption" yourself, and pass the result as the 'algorithm'.

TBH, I'd just require the user to specify the algorithm explicitly.
Having it run SHOW on the server seems wonky.  It introduces a bunch
of failure modes for ... no real benefit, I think.

-- 
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] some review comments on logical rep code

[Fujii Masao]

On Thu, Apr 27, 2017 at 1:28 AM, Fujii Masao  wrote:
> On Wed, Apr 26, 2017 at 3:47 PM, Kyotaro HORIGUCHI
>  wrote:
>> At Wed, 26 Apr 2017 14:31:12 +0900, Masahiko Sawada  
>> wrote in 
>> 
>>> On Wed, Apr 26, 2017 at 12:35 PM, Petr Jelinek
>>>  wrote:
>>> > On 26/04/17 01:01, Fujii Masao wrote:
>>>  However this is overkill for small gain and false wakeup of the
>>>  launcher is not so harmful (probably we can live with that), so
>>>  we do nothing here for this issue.
>>> >>>
>>> >>> I agree this as a whole. But I think that the main issue here is
>>> >>> not false wakeups, but 'possible delay of launching new workers
>>> >>> by 3 minutes at most' (this is centainly a kind of false wakeups,
>>> >>> though). We can live with this failure when using two-paase
>>> >>> commmit, but I think it shouldn't happen silently.
>>> >>>
>>> >>>
>>> >>> How about providing AtPrepare_ApplyLauncher(void) like the
>>> >>> follows and calling it in PrepareTransaction?
>>> >>
>>> >> Or we should apply the attached patch and handle the 2PC case properly?
>>> >> I was thinking that it's overkill more than necessary, but that seems 
>>> >> not true
>>> >> as far as I implement that.
>>> >>
>>> > Looks like it does not even increase size of the 2pc file, +1 for this.
>>>
>>> In my honest opinion, I didn't have a big will that we should handle
>>> even two-phase commit case, because this case is very rare (I could
>>> not image such case) and doesn't mean to lead a harmful result such as
>>> crash of server and returning inconsistent result. it just delays the
>>> launching worker for at most 3 minutes. We also can deal with this for
>>> example by making maximum nap time of apply launcher user-configurable
>>> and document it.
>>> But if we can deal with it by minimum changes like attached your patch I 
>>> agree.
>>
>> This change looks reasonable to me, +1 from me to this.
>>
>> The patch reads on_commit_launcher_wakeup directly then updates
>> it via ApplyALuncherWakeupAtCommit() but it's too much to add a
>> function for the sake of this.
>
> OK, so what about the attached patch? I replaced all the calls to
> ApplyLauncherWakeupAtCommit() with the code "on_commit_launcher_wakeup = 
> true".

BTW, while I was reading the code to implement the patch that
I posted upthread, I found that the following condition doesn't
work as expected. This is because "last_start_time" is always 0.

/* Limit the start retry to once a wal_retrieve_retry_interval */
if (TimestampDifferenceExceeds(last_start_time, now,
  wal_retrieve_retry_interval))

"last_start_time" is set to "now" when the launcher starts up new
worker. But "last_start_time" is defined and always initialized with 0
at the beginning of the main loop in ApplyLauncherMain(), so
the above condition becomes always true. This is obviously a bug.
Attached patch fixes this issue.

Regards,

-- 
Fujii Masao


bugfix.patch
Description: Binary data

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

Re: [HACKERS] Dropping a partitioned table takes too long

[Robert Haas]

On Wed, Apr 26, 2017 at 12:22 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Tue, Apr 25, 2017 at 10:05 PM, Amit Langote
>>  wrote:
>>> Your patch seems to be a much better solution to the problem, thanks.
>
>> Does anyone wish to object to this patch as untimely?
>
>> If not, I'll commit it.
>
> It's certainly not untimely to address such problems.  What I'm wondering
> is if we should commit both patches.  Avoiding an unnecessary heap_open
> is certainly a good thing, but it seems like the memory leak addressed
> by the first patch might still be of concern in other scenarios.

I will defer to you on that.  If you think that patch is a good idea,
please have at 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] Partition-wise join for join between (declaratively) partitioned tables

[Robert Haas]

On Wed, Apr 26, 2017 at 12:19 PM, Tom Lane  wrote:
> What I'm going to ask one more time, though, is why we are even discussing
> this.  Surely the partition bounds of a partitioned table must all be of
> the same type already.  If there is a case where they are not, that is
> a bug we had better close off before v10 ships, not a feature that we
> need to write a lot of code to accommodate.

This question was answered before, by Ashutosh.

http://postgr.es/m/cafjfprfakso4yzjvv7jkcmemvgdcnqc4yhqvwho5gczb5mw...@mail.gmail.com

Since you either didn't read his answer, or else didn't understand it
and didn't bother asking for clarification, I'll try to be more blunt:
of course all of the partition bounds of a single partitioned table
have to be of the same type.  We're not talking about that, because no
kidding.  This thread is about the possibility -- in a future release
-- of implementing a join between two different partitioned tables by
joining each pair of matching partitions.  To do that, you need the
tables to be compatibly partitioned, which requires that the
partitioning columns use the same opfamily for each partitioning
column but not necessarily that the types be the same.  Making
partition-wise join work in the case where the partitioning columns
are of different types within an opfamily (like int4 vs. int8) is
giving Ashutosh a bit of trouble.  So this is about a cross-type join,
not multiple types within a single partitioning hierarchy, as you
might also gather from the subject line of this thread.

-- 
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] some review comments on logical rep code

[Fujii Masao]

On Wed, Apr 26, 2017 at 3:47 PM, Kyotaro HORIGUCHI
 wrote:
> At Wed, 26 Apr 2017 14:31:12 +0900, Masahiko Sawada  
> wrote in 
> 
>> On Wed, Apr 26, 2017 at 12:35 PM, Petr Jelinek
>>  wrote:
>> > On 26/04/17 01:01, Fujii Masao wrote:
>>  However this is overkill for small gain and false wakeup of the
>>  launcher is not so harmful (probably we can live with that), so
>>  we do nothing here for this issue.
>> >>>
>> >>> I agree this as a whole. But I think that the main issue here is
>> >>> not false wakeups, but 'possible delay of launching new workers
>> >>> by 3 minutes at most' (this is centainly a kind of false wakeups,
>> >>> though). We can live with this failure when using two-paase
>> >>> commmit, but I think it shouldn't happen silently.
>> >>>
>> >>>
>> >>> How about providing AtPrepare_ApplyLauncher(void) like the
>> >>> follows and calling it in PrepareTransaction?
>> >>
>> >> Or we should apply the attached patch and handle the 2PC case properly?
>> >> I was thinking that it's overkill more than necessary, but that seems not 
>> >> true
>> >> as far as I implement that.
>> >>
>> > Looks like it does not even increase size of the 2pc file, +1 for this.
>>
>> In my honest opinion, I didn't have a big will that we should handle
>> even two-phase commit case, because this case is very rare (I could
>> not image such case) and doesn't mean to lead a harmful result such as
>> crash of server and returning inconsistent result. it just delays the
>> launching worker for at most 3 minutes. We also can deal with this for
>> example by making maximum nap time of apply launcher user-configurable
>> and document it.
>> But if we can deal with it by minimum changes like attached your patch I 
>> agree.
>
> This change looks reasonable to me, +1 from me to this.
>
> The patch reads on_commit_launcher_wakeup directly then updates
> it via ApplyALuncherWakeupAtCommit() but it's too much to add a
> function for the sake of this.

OK, so what about the attached patch? I replaced all the calls to
ApplyLauncherWakeupAtCommit() with the code "on_commit_launcher_wakeup = true".

Regards,

-- 
Fujii Masao


002_Reset_on_commit_launcher_wakeup_v6.patch
Description: Binary data

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

Re: [HACKERS] Dropping a partitioned table takes too long

[Tom Lane]

Robert Haas  writes:
> On Tue, Apr 25, 2017 at 10:05 PM, Amit Langote
>  wrote:
>> Your patch seems to be a much better solution to the problem, thanks.

> Does anyone wish to object to this patch as untimely?

> If not, I'll commit it.

It's certainly not untimely to address such problems.  What I'm wondering
is if we should commit both patches.  Avoiding an unnecessary heap_open
is certainly a good thing, but it seems like the memory leak addressed
by the first patch might still be of concern in other scenarios.

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] Dropping a partitioned table takes too long

[Peter Eisentraut]

On 4/26/17 12:15, Robert Haas wrote:
> On Tue, Apr 25, 2017 at 10:05 PM, Amit Langote
>  wrote:
>>> The attached patch try to replace 'heap_open' with 'LockRelationOid' when
>>> locking parent table.
>>> It improved dropping a table with 7000 partitions.
>>
>> Your patch seems to be a much better solution to the problem, thanks.
> 
> Does anyone wish to object to this patch as untimely?
> 
> If not, I'll commit it.

Seems quite reasonable.

-- 
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] Partition-wise join for join between (declaratively) partitioned tables

[Tom Lane]

Robert Haas  writes:
> I'm going to say this one more time: I really, really, really think
> you need to avoid trying to convert the partition bounds to a common
> type.  I said before that the infrastructure to do that is not present
> in our type system, and I'm pretty sure that statement is 100%
> correct.  The fact that you can find other cases where we do something
> sorta like that but in a different case with different requirements
> doesn't make that false.

It's not just a matter of lack of infrastructure: the very attempt is
flawed, because in some cases there simply isn't a supertype that can
hold all values of both types.  An easy counterexample is float8 vs
numeric: you can't convert float8 'Infinity' to numeric, but also there
are values of numeric that can't be converted to float8 without overflow
and/or loss of precision.

The whole business of precision loss makes things very touchy for almost
anything involving float and a non-float type, actually.

What I'm going to ask one more time, though, is why we are even discussing
this.  Surely the partition bounds of a partitioned table must all be of
the same type already.  If there is a case where they are not, that is
a bug we had better close off before v10 ships, not a feature that we
need to write a lot of code to accommodate.

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] Dropping a partitioned table takes too long

[Robert Haas]

On Tue, Apr 25, 2017 at 10:05 PM, Amit Langote
 wrote:
>> The attached patch try to replace 'heap_open' with 'LockRelationOid' when
>> locking parent table.
>> It improved dropping a table with 7000 partitions.
>
> Your patch seems to be a much better solution to the problem, thanks.

Does anyone wish to object to this patch as untimely?

If not, I'll commit 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] [BUGS] ON CONFLICT with constraint name doesn't work

[Nikolay Samokhvalov]

This is a kindly reminder, that this problem (message about "constraint"
violation, while there is no such a constraint defined, just an index) is
still unresolved.

Let's fix that naming?

Patch is attached in the previous message (posted to -bugs list)

On Thu, Mar 16, 2017 at 9:15 PM, Nikolay Samokhvalov 
wrote:
>
> Anyway, attached are 2 separate patches:
>  1) version 2 of patch fixing the message, including regression tests;
>  2) proposed change to the documentation https://www.postgresql.org/
> docs/current/static/sql-insert.html
>
>

Re: [HACKERS] logical replication fixes

[Peter Eisentraut]

On 4/18/17 12:17, Euler Taveira wrote:
> While inspecting the logical replication code, I found a bug that could
> pick the wrong remote relation if they have the same name but different
> schemas. Also, I did some spelling/cosmetic changes and fixed an
> oversight in the ALTER SUBSCRIPTION documentation. Patches attached.

committed

-- 
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] Partition-wise join for join between (declaratively) partitioned tables

[Robert Haas]

On Mon, Apr 24, 2017 at 7:06 AM, Ashutosh Bapat
 wrote:
> This assumes that datums in partition bounds have one to one mapping
> with the partitions, which isn't true for list partitions. For list
> partitions we have multiple datums corresponding to the items listed
> associated with a given partition. So, simply looping over the
> partitions of outer relations doesn't work; in fact there are two
> outer relations for a full outer join, so we have to loop over both of
> them together in a merge join fashion.

Maybe so, but my point is that it can be done with the original types,
without converting anything to a different type.

> When using clause is used the columns specified by using clause from
> the joining relations are merged into a single column. Here it has
> used a "wider" type column t2.a as the merged column for t1.a and
> t2.a. The logic is in buildMergedJoinVar().

That relies on select_common_type(), which can error out if it can't
find a common type.  That's OK for the current uses of that function,
because if it fails it means that the query is invalid.  But it's not
OK for what you want here, because it's not OK to error out due to
inability to do a partition-wise join when a non-partition-wise join
would have worked.  Also, note that all select_common_type() is really
doing is looking for the type within the type category that is marked
typispreferred, or else checking which direction has an implicit cast.
Neither of those things guarantee the property you want here, namely
that the "common" type is in the same opfamily and can store every
value of any of the input types without loss of precision.  So I don't
think you can rely on that.

I'm going to say this one more time: I really, really, really think
you need to avoid trying to convert the partition bounds to a common
type.  I said before that the infrastructure to do that is not present
in our type system, and I'm pretty sure that statement is 100%
correct.  The fact that you can find other cases where we do something
sorta like that but in a different case with different requirements
doesn't make that false.

-- 
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] subscription worker doesn't start immediately on eabled

[Fujii Masao]

On Wed, Apr 26, 2017 at 4:03 PM, Kyotaro HORIGUCHI
 wrote:
> At Tue, 25 Apr 2017 14:45:03 -0400, Peter Eisentraut 
>  wrote in 
> <3d6a1bd0-08ce-301d-3336-ec9f623a3...@2ndquadrant.com>
>> On 4/6/17 08:24, Kyotaro HORIGUCHI wrote:
>> > The attached patch wakes up launcher when a subscription is
>> > enabled. This fails when a subscription is enabled immedaitely
>> > after disabling but it won't be a matter.
>>
>> committed, thanks
>
> Thanks!

This patch makes me think that CREATE SUBSCRIPTION should also wake up
the launcher only when ENABLE is specified. Patch attached. Thought?

Regards,

-- 
Fujii Masao


wakeup_launcher_when_enabled.patch
Description: Binary data

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

Re: [HACKERS] Unportable implementation of background worker start

[Tom Lane]

I wrote:
> =?utf-8?Q?R=C3=A9mi_Zara?=  writes:
>> coypu was not stuck (no buildfarm related process running), but failed to 
>> clean-up shared memory and semaphores.
>> I’ve done the clean-up.

> Huh, that's even more interesting.

I installed NetBSD 5.1.5 on an old Mac G4; I believe this is a reasonable
approximation to coypu's environment.  With the pselect patch installed,
I can replicate the behavior we saw in the buildfarm of connections
immediately failing with "the database system is starting up".
Investigation shows that pselect reports ready sockets correctly (which is
what allows connections to get in at all), and it does stop waiting either
for a signal or for a timeout.  What it forgets to do is to actually
service the signal.  The observed behavior is caused by the fact that
reaper() is never called so the postmaster never realizes that the startup
process has finished.

I experimented with putting

PG_SETMASK();
PG_SETMASK();

immediately after the pselect() call, and found that indeed that lets
signals get serviced, and things work pretty much normally.

However, closer inspection finds that pselect only stops waiting when
a signal arrives *while it's waiting*, not if there was a signal already
pending.  So this is actually even more broken than the so called "non
atomic" behavior we had expected to see --- at least with that, the
pending signal would have gotten serviced promptly, even if ServerLoop
itself didn't iterate.

This is all giving me less than warm fuzzy feelings about the state of
pselect support out in the real world.

So at this point we seem to have three plausible alternatives:

1. Let HEAD stand as it is.  We have a problem with slow response to
bgworker start requests that arrive while ServerLoop is active, but that's
a pretty tight window usually (although I believe I've seen it hit at
least once in testing).

2. Reinstall the pselect patch, blacklisting NetBSD and HPUX and whatever
else we find to be flaky.  Then only the blacklisted platforms have the
problem.

3. Go ahead with converting the postmaster to use WaitEventSet, a la
the draft patch I posted earlier.  I'd be happy to do this if we were
at the start of a devel cycle, but right now seems a bit late --- not
to mention that we really need to fix 9.6 as well.

We could substantially ameliorate the slow-response problem by allowing
maybe_start_bgworker to launch multiple workers per call, which is
something I think we should do regardless.  (I have a patch written to
allow it to launch up to N workers per call, but have held off committing
that till after the dust settles in ServerLoop.)

I'm leaning to doing #1 plus the maybe_start_bgworker change.  There's
certainly room for difference of opinion here, though.  Thoughts?

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] tablesync patch broke the assumption that logical rep depends on?

[Jeff Janes]

On Wed, Apr 26, 2017 at 8:00 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 4/25/17 15:42, Petr Jelinek wrote:
> >> Here is the patch doing just that.
> >
> > And one more revision which also checks in_use when attaching shared
> > memory. This is mainly to improve the user visible behavior in
> > theoretical corner case when the worker is supposed to be cleaned up but
> > actually still manages to start (it would still fail even without this
> > change but the error message would be more obscure).
>
> Committed that, with some editorializing.
>

This gives me compiler warning:

launcher.c: In function 'logicalrep_worker_launch':
launcher.c:257: warning: 'slot' may be used uninitialized in this function

gcc version 4.4.7 20120313 (Red Hat 4.4.7-18) (GCC)


Cheers,

Jeff

Re: [HACKERS] PG 10 release notes

[Bruce Momjian]

On Wed, Apr 26, 2017 at 07:38:05PM +0530, Amit Kapila wrote:
> >> I have already mentioned the commit id (5e6d8d2b).  Text can be "Allow
> >> queries containing subplans to execute in parallel".  We should also
> >> mention in some way that this applies only when the query contains
> >> uncorrelated subplan.
> >
> > Sorry but I don't know what that means, and if I don't know, others
> > might not either.
> >
> 
> regression=# explain (costs off) select count(*) from tenk1 where
> (two, four) not in (select hundred, thousand from tenk2 where
> thousand > 100);
>   QUERY PLAN
> --
>  Finalize Aggregate
>->  Gather
>  Workers Planned: 2
>  ->  Partial Aggregate
>->  Parallel Seq Scan on tenk1
>  Filter: (NOT (hashed SubPlan 1))
>  SubPlan 1
>->  Seq Scan on tenk2
>  Filter: (thousand > 100)
> (9 rows)
> 

Oh, so non-correlated subqueries can be run in parallel.  Yes, that is
something we should have in the release notes.  How is this?

Author: Robert Haas 
2017-02-14 [5e6d8d2bb] Allow parallel workers to execute subplans.

Allow non-correlated subqueries to be run in parallel (Amit Kapila)

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

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


-- 
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 a typo in subscriptioncmd.c

[Fujii Masao]

On Wed, Apr 26, 2017 at 3:07 PM, Masahiko Sawada  wrote:
> Hi,
>
> Attached patch for $subject.
>
> s/accomodate/accommodate/

Pushed. Thanks!

Regards,

-- 
Fujii Masao


-- 
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] tablesync patch broke the assumption that logical rep depends on?

[Peter Eisentraut]

On 4/25/17 15:42, Petr Jelinek wrote:
>> Here is the patch doing just that.
> 
> And one more revision which also checks in_use when attaching shared
> memory. This is mainly to improve the user visible behavior in
> theoretical corner case when the worker is supposed to be cleaned up but
> actually still manages to start (it would still fail even without this
> change but the error message would be more obscure).

Committed that, with some editorializing.

-- 
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_dump emits ALTER TABLE ONLY partitioned_table

[Stephen Frost]

Amit,

* Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
> On 2017/04/26 0:42, Stephen Frost wrote:
> > I'm not sure what you mean here.  We're always going to call both
> > getInherits() and getPartitions() and run the queries in each, with the
> > way the code is written today.  In my experience working with pg_dump
> > and trying to not slow it down, the last thing we want to do is run two
> > independent queries when one will do.  Further, we aren't really
> > avoiding any work when we have to go look at pg_class to exclude the
> > partitioned tables in getInherits() anyway.  I'm not seeing why we
> > really need the join at all though, see below.
> 
> You're right and I realize that we don't need lots of new code for pg_dump
> to retrieve the partitioning information (including the parent-child
> relationships).  I propose some patches below, one per each item you
> discovered could be improved.

Thanks!

> Attached patch 0004 gets rid of that separation.  It removes the struct
> PartInfo, functions flagPartitions(), findPartitionParentByOid(),
> getPartitions(), and getTablePartitionKeyInfo().  All necessary
> partitioning-specific information is retrieved in getTables() itself.

That certainly looks better.

> Also, now that partitioning uses the old inheritance code, inherited NOT
> NULL constraints need not be emitted separately per child.  The
> now-removed code that separately retrieved partitioning inheritance
> information was implemented such that partition attributes didn't receive
> the flagInhAttrs() treatment.

Right.

> > I looked through
> > pg_get_partkeydef() and it didn't seem to be particularly expensive to
> > run, though evidently it doesn't handle being passed an OID that it
> > doesn't expect very cleanly:
> > 
> > =# select pg_get_partkeydef(oid) from pg_class;
> > ERROR:  cache lookup failed for partition key of 52333
> > 
> > I don't believe that's really very appropriate of it to do though and
> > instead it should work the same way pg_get_indexdef() and others do and
> > return NULL in such cases, so people can use it sanely.
> > 
> > I'm working through this but it's going to take a bit of time to clean
> > it up and it's not going to get finished today, but I do think it needs
> > to get done and so I'll work to make it happen this week.  I didn't
> > anticipate finding this, obviously and am a bit disappointed by it.
> 
> Yeah, that was sloppy. :-(
> 
> Attached patch 0005 fixes that and adds a test to rules.sql.

Good, I'll commit that first, since it will make things simpler for
pg_dump.

> I think I have found an oversight in the pg_dump's --binary-upgrade code
> that might have caused the error you saw (I proceeded to fix it after
> seeing the error that I saw).  Fix is included as patch 0003.

Yeah, that was what I saw also.

> So to summarize what the patches do (some of these were posted earlier)
> 
> 0001: Improve test coverage of partition constraints (non-inherited ones)

Looks fine.

> 0002: pg_dump: Do not emit WITH OPTIONS keywords with partition's columns

I'm trying to understand why this is also different.  At least on an
initial look, there seems to be a bunch more copy-and-paste from the
existing TypedTable to Partition in gram.y, which seems to all boil down
to removing the need for WITH OPTIONS to be specified.  If WITH OPTIONS
would be too much to have included for partitioning, and it isn't
actually necessary, why not just make it optional, and use the same
construct for both, removing all the duplicaiton and the need for
pg_dump to output it?

> 0003: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE
>   INHERIT to be emitted to attach a partition instead of ALTER TABLE
>   ATTACH PARTITION

Well, worse, it was dumping out both, the INHERITs and the ATTACH
PARTITION.  Given that we're now setting numParents for partitions, I
would think we'd just move the if (tbinfo->partitionOf) branches up
under the if (numParents > 0) branches, which I think would also help us
catch additional issues, like the fact that a binary-upgrade with
partitions in a different namespace from the partitioned table would
fail because the ATTACH PARTITION code doesn't account for it:

appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
  fmtId(tbinfo->partitionOf->dobj.name));
appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n",
  fmtId(tbinfo->dobj.name),
  tbinfo->partitiondef);

unlike the existing inheritance code a few lines above, which does:

appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT ",
  fmtId(tbinfo->dobj.name));
if (parentRel->dobj.namespace != tbinfo->dobj.namespace)
appendPQExpBuffer(q, "%s.",
fmtId(parentRel->dobj.namespace->dobj.name));
appendPQExpBuffer(q, 

Re: [HACKERS] StandbyRecoverPreparedTransactions recovers subtrans links incorrectly

[Tom Lane]

Nikhil Sontakke  writes:
> A SELECT query on the newly promoted master on any of the tables involved
> in the 2PC hangs. The hang is due to a loop in
> SubTransGetTopmostTransaction(). Due to incorrect linkages, we get a
> circular reference in parentxid <-> subxid inducing the infinite loop.

I'm inclined to propose that

(1) SubTransSetParent should contain an assert that 
Assert(TransactionIdFollows(xid, parent));
There is a similar assertion near one of the call sites, but that's
obviously too far away from the action.

(2) SubTransGetTopmostTransaction ought to contain an actual runtime
test and ereport (not just Assert) that the parent XID it got from
pg_subtrans precedes the child XID that was looked up.  This would
protect us against similar infinite loops in the future.  Even without
bugs, such a loop could arise due to corrupted data.

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] [PostgreSQL 10] default of hot_standby should be "on"?

[Tom Lane]

Magnus Hagander  writes:
> +1. I definitely think we should do it, and 10 would be the time to do it.

Agreed.  It's mainly a historical accident that the default is what it is,
I think.

> I wonder if we should also consider changing the standby error message to
> be a WARNING instead of an ERROR. So that if you try to start up a standby
> with hot_standby=on but master with wal_level=replica it would turn into a
> cold standby.

I'm -1 for that: if you fat-finger the configuration, you should be told
about it, not have the system start up in an unintended mode that lacks
critical functionality.

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] PG 10 release notes

[Amit Kapila]

On Tue, Apr 25, 2017 at 7:19 PM, Bruce Momjian  wrote:
> On Tue, Apr 25, 2017 at 09:00:45AM +0530, Amit Kapila wrote:
>> On Tue, Apr 25, 2017 at 8:35 AM, Bruce Momjian  wrote:
>> > On Tue, Apr 25, 2017 at 08:30:50AM +0530, Amit Kapila wrote:
>> >> On Tue, Apr 25, 2017 at 7:01 AM, Bruce Momjian  wrote:
>> >> > I have committed the first draft of the Postgres 10 release notes.  They
>> >> > are current as of two days ago, and I will keep them current.  Please
>> >> > give me any feedback you have.
>> >> >
>> >>
>> >> Some of the items which I feel could be added:
>> >>
>> >> 5e6d8d2bbbcace304450b309e79366c0da4063e4
>> >> Allow parallel workers to execute subplans.
>> >
>> > Uh, can you show me the commit on that and give some text ideas?
>> >
>>
>> I have already mentioned the commit id (5e6d8d2b).  Text can be "Allow
>> queries containing subplans to execute in parallel".  We should also
>> mention in some way that this applies only when the query contains
>> uncorrelated subplan.
>
> Sorry but I don't know what that means, and if I don't know, others
> might not either.
>

Let me try to explain by example:

Without this feature, the queries that refer subplans will be executed
serially like below:

regression=# explain (costs off) select count(*) from tenk1 where
(two, four) not in (select hundred, thousand from tenk2 where thousand
> 100);
QUERY PLAN
--
 Aggregate
   ->  Seq Scan on tenk1
 Filter: (NOT (hashed SubPlan 1))
 SubPlan 1
   ->  Seq Scan on tenk2
 Filter: (thousand > 100)
(6 rows)

After this feature, the queries that refer subplans can use parallel
plans like below:

regression=# explain (costs off) select count(*) from tenk1 where
(two, four) not in (select hundred, thousand from tenk2 where
thousand > 100);
  QUERY PLAN
--
 Finalize Aggregate
   ->  Gather
 Workers Planned: 2
 ->  Partial Aggregate
   ->  Parallel Seq Scan on tenk1
 Filter: (NOT (hashed SubPlan 1))
 SubPlan 1
   ->  Seq Scan on tenk2
 Filter: (thousand > 100)
(9 rows)


Now, it won't use parallelism if there is correlated subplan like below:

Seq Scan on t1
   Filter: (SubPlan 1)
   SubPlan 1
   ->  Result
 One-Time Filter: (t1.k = 0)
 ->  Parallel Seq Scan on t2

In this plan difference is that SubPlan refers to outer relation t1.

Do the above examples helps in understanding the feature?

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


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

Re: [HACKERS] StandbyRecoverPreparedTransactions recovers subtrans links incorrectly

[Nikhil Sontakke]

> I'm suggesting we take the approach that if there is a problem we can
> recreate it as a way of exploring what conditions are required and
> therefore work out the impact. Nikhil Sontakke appears to have
> re-created something, but not quite what I had expected. I think he
> will post here tomorrow with an update for us to discuss.
>
>

So, I reverted commit 0874d4f3e183757ba15a4b3f3bf563e0393dd9c2 to go back
to the earlier bad swapped arguments to SubTransSetParent resulting in
incorrect parent linkages and used the attached TAP test patch.

The test prepares a 2PC with more than 64 subtransactions. It then stops
the master and promotes the standby.

A SELECT query on the newly promoted master on any of the tables involved
in the 2PC hangs. The hang is due to a loop in
SubTransGetTopmostTransaction(). Due to incorrect linkages, we get a
circular reference in parentxid <-> subxid inducing the infinite loop.

Any further DML on these objects which will need to check visibility of
these tuples hangs as well. All unrelated objects and new transactions are
ok AFAICS.

I do not see any data loss, which is good. However tables involved in the
2PC are inaccessible till after a hard restart.

The attached TAP test patch can be considered for commit to test handling
2PC with large subtransactions on promoted standby instances.

Regards,
Nikhils
-- 
 Nikhil Sontakke   http://www.2ndQuadrant.com/
 PostgreSQL/Postgres-XL Development, 24x7 Support, Training & Services


subxid_bug_with_test_case_v1.0.patch
Description: Binary data

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

Re: [HACKERS] Adding support for Default partition in partitioning

[Rahila Syed]

Hello Jeevan,

Thank you for comments.

I will include your comments in the updated patch.

>7.The output of describe needs to be improved.

The syntax for DEFAULT partitioning is still under discussion. This comment
wont be
applicable if the syntax is changed.

>6.
>I am wondering, isn't it possible to retrieve the has_default and
default_index
>here to find out if default partition exists and if exist then find it's
oid
>using rd_partdesc, that would avoid above(7) loop to check if partition
bound is
>default
The checks are used to find the default partition bound and
exclude it from the list of partition bounds to form the partition
constraint.
This cant be accomplished by using has_default flag.
isDefaultPartitionBound() is written to accomplish that.


>8.
>Following call to find_inheritance_children() in
generate_qual_for_defaultpart()
>is an overhead, instead we can simply use an array of oids in rd_partdesc.
I think using find_inheritance_children() will take into consideration
concurrent
drop of a partition which the value in rd_partdesc will not.

Thank you,
Rahila Syed

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Peter Eisentraut]

On 4/26/17 04:32, David Rowley wrote:
>> For backpatching to 9.6, I came up with the attached reduced version.
>> Since we don't have add_foreign_grouping_paths() in 9.6, we can omit the
>> refactoring and keep the changes much simpler.  Does that make sense?
> 
> That makes sense to me. It fixes the reported issue and is less
> invasive than the pg10 patch.

committed

-- 
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] [PostgreSQL 10] default of hot_standby should be "on"?

[Magnus Hagander]

On Wed, Apr 26, 2017 at 1:25 PM, Bruce Momjian  wrote:

> On Wed, Apr 26, 2017 at 07:33:27AM +, Tsunakawa, Takayuki wrote:
> > From: pgsql-hackers-ow...@postgresql.org
> > > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Masahiko
> Sawada
> > > The idea of changing the default value seems good to me but I'm not
> sure
> > > it's good idea to change the default value now under the circumstances
> where
> > > we're focus on stabilization.
> > > Also we should update the document as well.
> > >
> >
> > We can consider like this: the OP found a usability problem as a result
> of PG 10 development, so we will fix it as a stabilization work.
>
> We did work in Postgres 10 to make replication simpler with better
> defaults.  This would be part of that improvement.
>


+1. I definitely think we should do it, and 10 would be the time to do it.

The failure scenario is that a standby node will no longer work by default
*if* you have changed the master to minimal. But unless you have explicitly
dropped that one, it would work.

So I definitely think we should change that.

I wonder if we should also consider changing the standby error message to
be a WARNING instead of an ERROR. So that if you try to start up a standby
with hot_standby=on but master with wal_level=replica it would turn into a
cold standby.

We should change the default independently of that, I think, but it might
make sense to do both.

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

Re: [HACKERS] [PostgreSQL 10] default of hot_standby should be "on"?

[Bruce Momjian]

On Wed, Apr 26, 2017 at 07:33:27AM +, Tsunakawa, Takayuki wrote:
> From: pgsql-hackers-ow...@postgresql.org
> > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Masahiko Sawada
> > The idea of changing the default value seems good to me but I'm not sure
> > it's good idea to change the default value now under the circumstances where
> > we're focus on stabilization.
> > Also we should update the document as well.
> > 
> 
> We can consider like this: the OP found a usability problem as a result of PG 
> 10 development, so we will fix it as a stabilization work.

We did work in Postgres 10 to make replication simpler with better
defaults.  This would be part of that improvement.

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

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


-- 
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] Cached plans and statement generalization

[Pavel Stehule]

2017-04-26 12:30 GMT+02:00 Konstantin Knizhnik :

>
>
> On 26.04.2017 10:49, Konstantin Knizhnik wrote:
>
>
>
> On 26.04.2017 04:00, Tsunakawa, Takayuki wrote:   Are you considering some
> upper limit on the number of prepared statements?
> In this case we need some kind of LRU for maintaining cache of
> autoprepared statements.
> I think that it is good idea to have such limited cached - it can avoid
> memory overflow problem.
> I will try to implement it.
>
>
> I attach new patch which allows to limit the number of autoprepared
> statements (autoprepare_limit GUC variable).
> Also I did more measurements, now with several concurrent connections and
> read-only statements.
> Results of pgbench with 10 connections, scale 10 and read-only statements
> are below:
>
> Protocol
> TPS
> extended
> 87k
> prepared
> 209k
> simple+autoprepare
> 206k
>
> As you can see, autoprepare provides more than 2 times speed improvement.
>
> Also I tried to measure overhead of parsing (to be able to substitute all
> literals, not only string literals).
> I just added extra call of pg_parse_query. Speed is reduced to 181k.
> So overhead is noticeable, but still making such optimization useful.
> This is why I want to ask question:  is it better to implement slower but
> safer and more universal solution?
>

Unsafe solution has not any sense, and it is dangerous (80% of database
users has not necessary knowledge). If somebody needs the max possible
performance, then he use explicit prepared statements.

Regards

Pavel


>
>
> --
> Konstantin Knizhnik
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

Re: [HACKERS] PG 10 release notes

[Bruce Momjian]

On Wed, Apr 26, 2017 at 09:34:02AM +0530, Amit Kapila wrote:
> >> Just wondering if the mention of commit
> >> 0414b26bac09379a4cbf1fbd847d1cee2293c5e4 is missed? Not sure if this
> >> requires a separate entry or could be merged with -- Support parallel
> >> btree index scans.
> >
> > This item was merged into the general parallel index scan item:
> >
> 
> Okay, but then shouldn't we add Rafia's name as well to that release
> notes entry as she is the author of one of the merged item (commit
> 0414b26bac09379a4cbf1fbd847d1cee2293c5e4) which has an independent
> value.

Oh, yes, added.  I think in a quick look I saw Rafia Sabih as the same
as Rahila Syed and thought the name was already listed. Sorry to both of
them.

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

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


-- 
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 10 release notes

[Bruce Momjian]

On Tue, Apr 25, 2017 at 09:56:58PM -0400, Bruce Momjian wrote:
> > SCRAM-SHA-256 improves deficiencies of MD5 password hashing by
> > preventing any kind of pass-the-hash vulnerabilities, where a user
> > would be able to connect to a PostgreSQL instance by just knowing the
> > hash of a password and not the password itself.
> 
> First, I don't think RFC references belong in the release notes, let
> alone RFC links.
> 
> Second, there seems to be some confusion over what SCRAM-SHA-256 gives
> us over MD5.  I think there are a few benefits:
> 
> o  packets cannot be replayed as easily, i.e. md5 replayed random salt
> packets with a 50% probability after 16k sessions
   ^^^

Sorry, it is after 64k sessions.  The rule of thumb is that if you
choose from 2^x random values, you have a 50% chance of repeating one
after 2^(x/2) numbers.

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

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


-- 
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] Cached plans and statement generalization

[Konstantin Knizhnik]

On 26.04.2017 10:49, Konstantin Knizhnik wrote:



On 26.04.2017 04:00, Tsunakawa, Takayuki wrote:   Are you considering 
some upper limit on the number of prepared statements?
In this case we need some kind of LRU for maintaining cache of 
autoprepared statements.
I think that it is good idea to have such limited cached - it can 
avoid memory overflow problem.

I will try to implement it.


I attach new patch which allows to limit the number of autoprepared 
statements (autoprepare_limit GUC variable).
Also I did more measurements, now with several concurrent connections 
and read-only statements.
Results of pgbench with 10 connections, scale 10 and read-only 
statements are below:


Protocol
TPS
extended
87k
prepared
209k
simple+autoprepare
206k


As you can see, autoprepare provides more than 2 times speed improvement.

Also I tried to measure overhead of parsing (to be able to substitute 
all literals, not only string literals).

I just added extra call of pg_parse_query. Speed is reduced to 181k.
So overhead is noticeable, but still making such optimization useful.
This is why I want to ask question:  is it better to implement slower 
but safer and more universal solution?


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index f6be98b..0c9abfc 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -188,6 +188,7 @@ static bool IsTransactionStmtList(List *parseTrees);
 static void drop_unnamed_stmt(void);
 static void SigHupHandler(SIGNAL_ARGS);
 static void log_disconnections(int code, Datum arg);
+static bool exec_cached_query(const char *query_string);
 
 
 /* 
@@ -916,6 +917,14 @@ exec_simple_query(const char *query_string)
 	drop_unnamed_stmt();
 
 	/*
+	 * Try to find cached plan
+	 */
+	if (autoprepare_threshold != 0 && exec_cached_query(query_string))
+	{
+		return;
+	}
+
+	/*
 	 * Switch to appropriate context for constructing parsetrees.
 	 */
 	oldcontext = MemoryContextSwitchTo(MessageContext);
@@ -4500,3 +4509,606 @@ log_disconnections(int code, Datum arg)
 	port->user_name, port->database_name, port->remote_host,
   port->remote_port[0] ? " port=" : "", port->remote_port)));
 }
+
+typedef struct { 
+	char const*   query;
+	dlist_nodelru;
+	int64 exec_count;
+	CachedPlanSource* plan;	
+	int   n_params;
+	int16 format;
+	bool  disable_autoprepare;
+} plan_cache_entry;
+
+/*
+ * Replace string literals with parameters. We do not consider integer or real literals to avoid problems with 
+ * negative number, user defined operators, ... For example it is not easy to distinguish cases (-1), (1-1), (1-1)-1
+ */
+static void generalize_statement(const char *query_string, char** gen_query, char** query_params, int* n_params)
+{
+	size_t query_len = strlen(query_string);
+	char const* src = query_string;
+	char* dst;
+	char* params;
+	unsigned char ch;
+
+	*n_params = 0;
+
+	*gen_query = (char*)palloc(query_len*2); /* assume that we have less than 1000 parameters, the worst case is replacing '' with $999 */
+	*query_params = (char*)palloc(query_len + 1);
+	dst = *gen_query;
+	params = *query_params;
+
+	while ((ch = *src++) != '\0') { 
+		if (isspace(ch)) { 
+			/* Replace sequence of whitespaces with just one space */
+			while (*src && isspace(*(unsigned char*)src)) { 
+src += 1;
+			}
+			*dst++ = ' ';
+		} else if (ch == '\'') { 
+			while (true) { 
+ch = *src++;
+if (ch == '\'') { 
+	if (*src != '\'') { 
+		break;
+	} else {
+		/* escaped quote */
+		*params++ = '\'';
+		src += 1;
+	}
+} else { 
+	*params++ = ch;
+}
+			}
+			*params++ = '\0';
+			dst += sprintf(dst, "$%d", ++*n_params);
+		} else { 
+			*dst++ = ch;
+		}
+	}			
+	Assert(dst <= *gen_query + query_len);
+	Assert(params <= *query_params + query_len*2);
+	*dst = '\0';
+}
+
+static uint32 plan_cache_hash_fn(const void *key, Size keysize)
+{
+	return string_hash(((plan_cache_entry*)key)->query, 0);
+}
+
+static int plan_cache_match_fn(const void *key1, const void *key2, Size keysize)
+{
+	return strcmp(((plan_cache_entry*)key1)->query, ((plan_cache_entry*)key2)->query);
+}
+
+static void* plan_cache_keycopy_fn(void *dest, const void *src, Size keysize)
+{ 
+	((plan_cache_entry*)dest)->query = pstrdup(((plan_cache_entry*)src)->query);
+return dest;
+}
+
+#define PLAN_CACHE_SIZE 113
+
+size_t nPlanCacheHits;
+size_t nPlanCacheMisses;
+
+/*
+ * Try to generalize query, find cached plan for it and execute
+ */
+static bool exec_cached_query(const char *query_string)
+{
+	CommandDest   dest = whereToSendOutput;
+	DestReceiver *receiver;
+	char *gen_query;
+	char *query_params;
+	int   n_params;
+	plan_cache_entry *entry;
+	

Re: [HACKERS] scram and \password

[Heikki Linnakangas]

On 04/25/2017 06:26 PM, Heikki Linnakangas wrote:

Thoughts? Unless someone has better ideas or objections, I'll go
implement that.


This is what I came up with in the end. Some highlights and differences 
vs the plan I posted earlier:


* If algorithm is not given explicitly, PQencryptPasswordConn() queries 
"SHOW password_encryption", and uses that. That is documented, and it is 
also documented that it will *not* issue queries, and hence will not 
block, if the algorithm is given explicitly. That's an important 
property for some applications. If you want the default behavior without 
blocking, query "SHOW password_encryption" yourself, and pass the result 
as the 'algorithm'.


* In the previous draft, I envisioned an algorithm-specific 'options' 
argument. I left it out, on second thoughts (more on that below).


* The old PQencryptPassword() function is unchanged, and always uses 
md5. Per public opinion.



We talked about the alternative where PQencryptPasswordConn() would not 
look at password_encryption, but would always use the strongest possible 
algorithm supported by the server. That would avoid querying the server. 
But then I started thinking how this would work, if we make the number 
of iterations in the SCRAM verifier configurable in the future. The 
client would not know the desired number of iterations based only on the 
server version, it would need to query the server, and we would be back 
to square one. We could add an "options" argument to 
PQencryptPasswordConn() that the application could use to pass that 
information, but documenting how to fetch that information, if you don't 
want PQencryptPasswordConn() to block, gets tedious, and puts a lot of 
burden to applications. That is why I left out the "options" argument, 
after all.


I'm now thinking that if we add password hashing options like the 
iteration count in the future, they will be tacked on to 
password_encryption. For example, "password_encryption='scram-sha-256, 
iterations=1". That way, "password_encryption" will always contain 
enough information to construct password verifiers.


What will happen to existing applications using PQencryptPasswordConn() 
if a new password_encryption algorithm (or options) is added in the 
future? With this scheme, the application doesn't need to know what 
algorithms exist. An application can pass algorithm=NULL, to use the 
server default, or do "show password_encryption" and pass that, for the 
non-blocking behavior. If you use an older libpq against a new server, 
so that libpq doesn't know about the algorithm used by the server, you 
get an error.


For reviewer convenience, I put up the patched docs at 
http://hlinnaka.iki.fi/temp/scram-wip-docs/libpq-misc.html#libpq-pqencryptpasswordconn.


Thoughts? Am I missing anything?

As an alternative, I considered making password_encryption GUC_REPORT, 
so that libpq would always know it without having to issue a query. But 
it feels wrong to send that option to the client on every connection, 
when it's only needed in the rare case that you use 
PQencryptPasswordConn(). And if we added new settings like the iteration 
count in the future, those would need to be GUC_REPORT too.


- Heikki



0001-Move-scram_build_verifier-to-src-common.patch
Description: application/download


0002-Add-PQencryptPasswordConn-function-to-libpq.patch
Description: application/download


0003-Use-new-PQencryptPasswordConn-function-in-psql-and-c.patch
Description: application/download

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

Re: [HACKERS] Declarative partitioning - another take

[Amit Khandekar]

On 26 April 2017 at 00:28, Robert Haas  wrote:
> So what I'd prefer -- on
> the totally unprincipled basis that it would let us improve
> performance in the future -- if you operate on a partition directly,
> you fire the partition's triggers, but if you operate on the parent,
> only the parent's triggers fire.

I would also opt for this behaviour.

Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database 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 emits ALTER TABLE ONLY partitioned_table

[Amit Langote]

Hi Stephen,

On 2017/04/26 0:42, Stephen Frost wrote:
> Amit,
> 
> * Amit Langote (langote_amit...@lab.ntt.co.jp) wrote:
>> I think why getPartitions() is separate from getInherits() and then
>> flagPartitions() separate from flagInhTables() is because I thought
>> originally that mixing the two would be undesirable.  In the partitioning
>> case, getPartitions() joins pg_inherits with pg_class so that it can also
>> get hold of relpartbound, which I then thought would be a bad idea to do
>> for all of the inheritance tables.  If we're OK with always doing the
>> join, I don't see why we couldn't get rid of the separation.
> 
> I'm not sure what you mean here.  We're always going to call both
> getInherits() and getPartitions() and run the queries in each, with the
> way the code is written today.  In my experience working with pg_dump
> and trying to not slow it down, the last thing we want to do is run two
> independent queries when one will do.  Further, we aren't really
> avoiding any work when we have to go look at pg_class to exclude the
> partitioned tables in getInherits() anyway.  I'm not seeing why we
> really need the join at all though, see below.

You're right and I realize that we don't need lots of new code for pg_dump
to retrieve the partitioning information (including the parent-child
relationships).  I propose some patches below, one per each item you
discovered could be improved.

>> flagInhAttrs(), OTOH, seems unaffected by that concern and I think using
>> it for both inheritance and partitioning is fine.  It affects NOT NULL
>> constraints and defaults, which as far as it goes, applies to both
>> inheritance and partitioning the same.
> 
> I don't see why we need to have getPartitions(), flagPartitions(), or
> findPartitonParentByOid().  They all seem to be largely copies of the
> inheritance functions, but it doesn't seem like that's really necessary
> or sensible.
> 
> I also don't follow why we're pulling the partbound definition in
> getPartitions() instead of in getTables(), where we're already pulling
> the rest of the data from pg_class?  Or why getTablePartitionKeyInfo()
> exists instead of also doing that during getTables()?

I think it had to do with wanting to avoid creating yet another copy of
the big getTables() query for >= v10, back when the original patch was
written, but doing that is not really needed.

Attached patch 0004 gets rid of that separation.  It removes the struct
PartInfo, functions flagPartitions(), findPartitionParentByOid(),
getPartitions(), and getTablePartitionKeyInfo().  All necessary
partitioning-specific information is retrieved in getTables() itself.

Also, now that partitioning uses the old inheritance code, inherited NOT
NULL constraints need not be emitted separately per child.  The
now-removed code that separately retrieved partitioning inheritance
information was implemented such that partition attributes didn't receive
the flagInhAttrs() treatment.

> I looked through
> pg_get_partkeydef() and it didn't seem to be particularly expensive to
> run, though evidently it doesn't handle being passed an OID that it
> doesn't expect very cleanly:
> 
> =# select pg_get_partkeydef(oid) from pg_class;
> ERROR:  cache lookup failed for partition key of 52333
> 
> I don't believe that's really very appropriate of it to do though and
> instead it should work the same way pg_get_indexdef() and others do and
> return NULL in such cases, so people can use it sanely.
> 
> I'm working through this but it's going to take a bit of time to clean
> it up and it's not going to get finished today, but I do think it needs
> to get done and so I'll work to make it happen this week.  I didn't
> anticipate finding this, obviously and am a bit disappointed by it.

Yeah, that was sloppy. :-(

Attached patch 0005 fixes that and adds a test to rules.sql.

>> So, the newly added tests seem enough for now?
> 
> Considering the pg_upgrade regression test didn't pass with the patch
> as sent, I'd say that more tests are needed.  I will be working to add
> those also.

I think I have found an oversight in the pg_dump's --binary-upgrade code
that might have caused the error you saw (I proceeded to fix it after
seeing the error that I saw).  Fix is included as patch 0003.

So to summarize what the patches do (some of these were posted earlier)

0001: Improve test coverage of partition constraints (non-inherited ones)

0002: pg_dump: Do not emit WITH OPTIONS keywords with partition's columns

0003: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE
  INHERIT to be emitted to attach a partition instead of ALTER TABLE
  ATTACH PARTITION

0004: Change the way pg_dump retrieves partitioning info (removes a lot
  of unnecessary code and instead makes partitioning case use the old
  inheritance code, etc.)

0005: Teach pg_get_partkeydef_worker to deal with OIDs it can't handle

Thanks,
Amit
>From 5ba1b532b020648457e92d34d5f2712f36efd9ff Mon Sep 17 

Re: [HACKERS] Quorum commit for multiple synchronous replication.

[Kyotaro HORIGUCHI]

At Tue, 25 Apr 2017 21:21:29 +0900, Masahiko Sawada  
wrote in 
> On Tue, Apr 25, 2017 at 8:07 PM, Amit Kapila  wrote:
> > On Tue, Apr 25, 2017 at 2:09 PM, Kyotaro HORIGUCHI
> >  wrote:
> >>
> >> I'm not good at composition, so I cannot insist on my
> >> proposal. For the convenience of others, here is the proposal
> >> from Fujii-san.
> >>
> >
> > Do you see any problem with the below proposal?
> > To me, this sounds reasonable.
> 
> I agree.

Ok, I give up:p Thanks for shoving me.

> >> + A quorum-based synchronous replication is basically more efficient 
> >> than
> >> + a priority-based one when you specify multiple standbys in
> >> + synchronous_standby_names and want to replicate
> >> + the transactions to some of them synchronously. In this case,
> >> + the transactions in a priority-based synchronous replication must 
> >> wait for
> >> + reply from the slowest standby in synchronous standbys chosen based 
> >> on
> >> + their priorities, and which may increase the transaction latencies.
> >> + On the other hand, using a quorum-based synchronous replication may
> >> + improve those latencies because it makes the transactions wait only 
> >> for
> >> + replies from the requested number of faster standbys in all the 
> >> listed
> >> + standbys, i.e., such slow standby doesn't block the transactions.
> >>
> >
> > Can we do few modifications like:
> > improve those latencies --> reduce those latencies
> > such slow standby --> a slow standby
> >
> > --
> > With Regards,
> > Amit Kapila.
> > EnterpriseDB: http://www.enterprisedb.com
> 
> 
> Regards,
> 
> --
> Masahiko Sawada
> NIPPON TELEGRAPH AND TELEPHONE CORPORATION
> NTT Open Source Software Center

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



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