Re: [HACKERS] Improvements in psql hooks for variables

[Ashutosh Bapat]

You may want to add this to the November commitfest
https://commitfest.postgresql.org/11/.

On Fri, Sep 16, 2016 at 6:05 PM, Daniel Verite  wrote:
>  Hi,
>
> Following the discussion on forbidding an AUTOCOMMIT off->on
> switch mid-transaction [1], attached is a patch that let the hooks
> return a boolean indicating whether a change is allowed.
>
> Using the hooks, bogus assignments to built-in variables can
> be dealt with more strictly.
>
> For example, pre-patch behavior:
>
>   =# \set ECHO errors
>   =# \set ECHO on
>   unrecognized value "on" for "ECHO"; assuming "none"
>   =# \echo :ECHO
>   on
>
> which has two problems:
> - we have to assume a value, even though we can't know what the user meant.
> - after assignment, the user-visible value of the variable diverges from its
> internal counterpart (pset.echo in this case).
>
>
> Post-patch:
>   =# \set ECHO errors
>   =# \set ECHO on
>   unrecognized value "on" for "ECHO"
>   \set: error while setting variable
>   =# \echo :ECHO
>   errors
>
> Both the internal pset.* state and the user-visible value are kept unchanged
> is the input value is incorrect.
>
> Concerning AUTOCOMMIT, autocommit_hook() could return false to forbid
> a switch when the conditions are not met.
>
>
> Another user-visible effect of the patch is that, using a bogus value
> for a built-in variable on the command-line becomes a fatal error
> that prevents psql to continue.
> This is not directly intended by the patch but is a consequence
> of SetVariable() failing.
>
> Example:
>   $ ./psql -vECHO=bogus
>   unrecognized value "bogus" for "ECHO"
>   psql: could not set variable "ECHO"
>   $ echo $?
>   1
>
> The built-in vars concerned by the change are:
>
> booleans: AUTOCOMMIT, ON_ERROR_STOP, QUIET, SINGLELINE, SINGLESTEP
>
> non-booleans: ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK, COMP_KEYWORD_CASE,
>  HISTCONTROL, VERBOSITY, SHOW_CONTEXT
>
> We could go further to close the gap between pset.* and the built-in
> variables,
> by changing how they're initialized and forbidding deletion as Tom
> suggests in [2], but if there's negative feedback on the above changes,
> I think we should hear it first.
>
> [1]
> https://www.postgresql.org/message-id/f2cb5838-0ee9-4fe3-acc0-df77aeb7d4c7%40mm
> [2]
> https://www.postgresql.org/message-id/4695.1473961140%40sss.pgh.pa.us
>
>
> Best regards,
> --
> Daniel Vérité
> PostgreSQL-powered mailer: http://www.manitou-mail.org
> Twitter: @DanielVerite
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>



-- 
Best Wishes,
Ashutosh Bapat
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] Hash Indexes

[Mark Kirkwood]

On 17/09/16 06:38, Andres Freund wrote:

On 2016-09-16 09:12:22 -0700, Jeff Janes wrote:

On Thu, Sep 15, 2016 at 7:23 AM, Andres Freund  wrote:

One earlier question about this is whether that is actually a worthwhile
goal.  Are the speed and space benefits big enough in the general case?
Could those benefits not be achieved in a more maintainable manner by
adding a layer that uses a btree over hash(columns), and adds
appropriate rechecks after heap scans?

Note that I'm not saying that hash indexes are not worthwhile, I'm just
doubtful that question has been explored sufficiently.

I think that exploring it well requires good code.  If the code is good,
why not commit it?

Because getting there requires a lot of effort, debugging it afterwards
would take effort, and maintaining it would also takes a fair amount?
Adding code isn't free.

I'm rather unenthused about having a hash index implementation that's
mildly better in some corner cases, but otherwise doesn't have much
benefit. That'll mean we'll have to step up our user education a lot,
and we'll have to maintain something for little benefit.



While I see the point of what you are saying here, I recall all previous 
discussions about has indexes tended to go a bit like this:


- until WAL logging of hash indexes is written it is not worthwhile 
trying to make improvements to them

- WAL logging will be a lot of work, patches 1st please

Now someone has done that work, and we seem to be objecting that because 
they are not improved then the patches are (maybe) not worthwhile. I 
think that is - essentially - somewhat unfair.


regards

Mark


--
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] IF (NOT) EXISTS in psql-completion

[Pavel Stehule]

2016-09-16 10:31 GMT+02:00 Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp>:

> Hello, this is the new version.
>
> At Tue, 13 Sep 2016 10:50:13 +0900 (Tokyo Standard Time), Kyotaro
> HORIGUCHI  wrote in <
> 20160913.105013.65452566.horiguchi.kyot...@lab.ntt.co.jp>
> > > > This patch consists of the following files. Since these files are
> > > > splitted in strange criteria and order for historical reasons,
> > > > I'll reorganize this and post them later.
>
> The focus of this patch has changed. The first motivation was
> completing IF-EXISTS but the underlying issue was flexibility of
> psql_completion. And as Pavel's suggestion, keywords suggested
> along with database objects should follow the character case of
> input.
>
> For the purpose of resolving the issues, I reorganized the
> confused patch set. The attached patches are organized as the
> following.
>
> 1. Refactoring tab-complete to make psql_completion code
>
>   Does two things. One is moving out the macros that has grown to
>   be too large to stay in tab_completion.c to new file
>   tab-complete-macros.h The other is separating out the else-if
>   sequence in psql_completion() as a new function
>   psql_completion_internal(). This allows us to the following
>   things.
>
>   - Exit from arbitrary place in the former-else-if sequence just
> by return.
>
>   - Do other than "if(matching) { completion }" in anywhere
> convenient in the midst of the former-els...
>
>   - Recursively matching for sub syntaxes. EXPLAIN, RULE and
> others are using this feature. (Needs the 4th patch to do
> this, though)
>
>
This first patch looks well - although it is big patch - it doesn't do any
not trivial work. No problems with a patching or compilation. I didn't find
any issue.

Regards

Pavel


> 2. Make keywords' case follow to input
>
>   Allow the keywords suggested along with databse objects to
>   follow the input letter case. The core part of this patch is a
>   new function additional_kw_query(), which dynamically generates
>   additional query string with specified keywords in the desired
>   letter case. COMPLETE_WITH_* macros are modified to accept the
>   function.
>
> 3. Fix suggested keywords to follow input in tab-completion session 2
>
>   The 2nd patch above leaves some query string containing static
>   keyword strings, which results in failure to follow input
>   letter cases. Most of them are naturally removed but role names
>   are a bother. This patch puts additional query strings for
>   several usage of roles but it might be overdone.
>
> 4. Introduce word shift and removal feature to psql-completion
>
>   This is the second core for the flexibility of completion code.
>   The word shift feature is the ability to omit first several
>   words in *MatchesN macros. For example this allows complete
>   create-schema's schema elements in a natural code. (Currently
>   those syntaxes that can be a schema elements are using
>   TailMatches instead of Matches, as the result HeadMatches are
>   not available there). The words removing feature is the ability
>   to (desructively) clip multiple suceessive words in the
>   previous_words list. This feature allows suceeding completion
>   code not to care about the removed words, such like UNIQUE,
>   CONCURRENTLY, VERBOSE and so on.
>
> 5. Add suggestion for IF (NOT) EXISTS for some syntaxes
>
>   This adds IF (NOT) EXISTS suggestion, as a PoC.  This patch no
>   loger covers all adoptable syntaces since the places where more
>   than boilerplating is required are omitted.
>
> regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
>
>
>
>

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

[Dilip Kumar]

On Mon, Sep 19, 2016 at 2:41 AM, Tomas Vondra
 wrote:
> But now that I look at the first post, I see it apparently used a plain
> tpc-b pgbench (with synchronous_commit=on) to show the benefits, which is
> the workload I'm running right now (results sometime tomorrow).

Good option, We can test plain TPC-B also..

I have some more results.. I have got the result for "Update with no
savepoint"

below is my script...

\set aid random (1,3000)
\set tid random (1,3000)
\set delta random(-5000, 5000)
BEGIN;
UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;
UPDATE pgbench_tellers SET tbalance = tbalance + :delta WHERE tid = :tid;
UPDATE pgbench_accounts SET abalance = abalance + :delta WHERE aid = :aid;


Results: (median of three, 10 minutes run).

Clients   Head GroupLock
16  2145221589
32  4242242688
64  4246052590   ~ 23%
1282268356825   ~150%
256   18748 54867

With this workload I observed that gain is bigger than my previous
workload (select for update with 2 SP)..

Just to confirm that the gain what we are seeing is because of Clog
Lock contention removal or it's
something else, I ran 128 client with perf for 5 minutes and below is my result.

I can see that after applying group lock patch, LWLockAcquire become
28% to just 4%, and all because
of Clog Lock.

On Head:

-   28.45% 0.24%  postgres  postgres   [.] LWLockAcquire
   - LWLockAcquire
  + 53.49% TransactionIdSetPageStatus
  + 40.83% SimpleLruReadPage_ReadOnly
  + 1.16% BufferAlloc
  + 0.92% GetSnapshotData
  + 0.89% GetNewTransactionId
  + 0.72% LockBuffer
  + 0.70% ProcArrayGroupClearXid


After Group Lock Patch:
---
-4.47% 0.26%  postgres  postgres   [.] LWLockAcquire
   - LWLockAcquire
  + 27.11% GetSnapshotData
  + 21.57% GetNewTransactionId
  + 11.44% SimpleLruReadPage_ReadOnly
  + 10.13% BufferAlloc
  + 7.24% ProcArrayGroupClearXid
  + 4.74% LockBuffer
  + 4.08% LockAcquireExtended
  + 2.91% TransactionGroupUpdateXidStatus
  + 2.71% LockReleaseAll
  + 1.90% WALInsertLockAcquire
  + 0.94% LockRelease
  + 0.91% VirtualXactLockTableInsert
  + 0.90% VirtualXactLockTableCleanup
  + 0.72% MultiXactIdSetOldestMember
  + 0.66% LockRefindAndRelease

Next I will test, "update with 2 savepoints", "select for update with
no savepoints"
I will also test the granular lock and atomic lock patch in next run..

-- 
Regards,
Dilip Kumar
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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[David Fetter]

On Fri, Sep 09, 2016 at 09:57:21AM -0400, Peter Eisentraut wrote:
> Review of the patch in the commit fest:
> 
> - Various naming/spelling inconsistencies: In the source, the module
>   is require_where, the documentation titles it require-where, the GUC
>   parameters are requires_where.*, but incorrectly documented.

Fixed.

> - Unusual indentation in the Makefile

Fixed.

> - Needs tests

Still needs some fixing.

> - Not sure about errcode(ERRCODE_CARDINALITY_VIOLATION), which is
>   documented in the code as "this means something returned the wrong
>   number of rows".  I think ERRCODE_SYNTAX_ERROR or something from
>   nearby there would be better.

Changed to ERRCODE_SYNTAX_ERROR.  CARDINALITY_VIOLATION was a bit too
cute.

> - errhint() string should end with a period.

Fixed.

> - The 7th argument of DefineCustomBoolVariable() is of type int, not
>   bool, so passing false is somewhat wrong, even if it works.

Fixed.

> - There ought to be a _PG_fini() function that undoes what _PG_init()
>   does.

Fixed.

> - The documentation should be expanded and clarified.  Given that this
>   is a "training wheels" module, we can be extra clear here.  I would
>   like to see some examples at least.

Working on this.

> - The documentation is a bit incorrect about the ways to load this
>   module.  shared_preload_libraries is not necessary.  session_ and
>   local_ (with prep) should also work.

I'm not 100% sure I understand what you want here.  I did manage to
get the thing loaded without a restart via LOAD, but that's it so far.
Will continue to poke at it.

> - The claim in the documentation that only superusers can do things
>   with this module is not generally correct.

I think that the claims are fixed.  This is SUSET, at least in this
patch, because anything short of that that changes query behavior
seems incautious.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
diff --git a/contrib/Makefile b/contrib/Makefile
index 25263c0..4bd456f 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -40,6 +40,7 @@ SUBDIRS = \
pgstattuple \
pg_visibility   \
postgres_fdw\
+   require_where   \
seg \
spi \
tablefunc   \
diff --git a/contrib/require_where/Makefile b/contrib/require_where/Makefile
new file mode 100644
index 000..0cf3663
--- /dev/null
+++ b/contrib/require_where/Makefile
@@ -0,0 +1,17 @@
+# contrib/require_where/Makefile
+
+MODULE_big = require_where
+OBJS = require_where.o
+
+PGFILEDESC = 'require_where - require DELETE and/or UPDATE to have a WHERE 
clause'
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS = $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/require_where
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_builddir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/require_where/require_where.c 
b/contrib/require_where/require_where.c
new file mode 100644
index 000..181b3bb
--- /dev/null
+++ b/contrib/require_where/require_where.c
@@ -0,0 +1,92 @@
+/*
+ * --
+ *
+ * require_where.c
+ *
+ * Copyright (C) 2016, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * contrib/require_where/require_where.c
+ *
+ * --
+ */
+#include "postgres.h"
+
+#include "fmgr.h"
+
+#include "parser/analyze.h"
+
+#include "utils/elog.h"
+#include "utils/guc.h"
+
+PG_MODULE_MAGIC;
+
+void   _PG_init(void);
+void   _PG_fini(void);
+
+static post_parse_analyze_hook_type original_post_parse_analyze_hook = 
NULL;
+static boolrequire_where_delete = false;
+static boolrequire_where_update = false;
+
+static void
+require_where_check(ParseState *pstate, Query *query)
+{
+
+   if (require_where_delete && query->commandType == CMD_DELETE)
+   {
+   Assert(query->jointree != NULL);
+   if (query->jointree->quals == NULL)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("DELETE requires a WHERE 
clause"),
+errhint("To delete all rows, use 
\"WHERE true\" or similar.")));
+   }
+
+   if (require_where_update && query->commandType == CMD_UPDATE)
+   {
+   Assert(query->jointree != NULL);
+   if (query->jointree->quals == NULL)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+   

[HACKERS] recovery_min_apply_delay vs. synchronous_commit

[Bernd Helmle]

Current PostgreSQL Documentation on recovery.conf has this about
recovery_min_apply_delay[1]:

---<---

This parameter is intended for use with streaming replication deployments;
however, if the parameter is specified it will be honored in all cases.
Synchronous replication is not affected by this setting because there is
not yet any setting to request synchronous apply of transaction commits.

--->---

If i understand correctly, this is not true anymore with 9.6, where
remote_apply will have exactly the behavior the paragraph above wants to
contradict: any transaction executed with synchronous_commit=remote_apply
will wait at least recovery_min_apply_delay to finish. Given that
synchronous_commit can be controlled by any user, this might be dangerous
if someone doesn't take care enough.

I think we need a doc patch for that at least, see attached patch against
master, but 9.6 should have a corrected one, too.

[1] 

-- 
Mit freundlichen Grüßen

Bernd Helmle


recovery-config-doc.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] Logical Replication WIP

[Steve Singer]

On 09/08/2016 06:59 PM, Petr Jelinek wrote:

Hi,

Updated version, this should address most of the things in Peter's 
reviews so far, not all though as some of it needs more discussion.




Another bug report.

I had subscribed a subscriber database to a publication with 1 table
create table a (a serial4 primary key, b text);

* I then dropped column b on the subscriber
* inserted some rows on the publisher
* Noticed the expected error about column b not existing in the 
subscriber log

* Added column c on the subscriber, then added column b after column C

I now get the following stack trace

#1  0x007dc8f9 in cstring_to_text (
s=0x16f238af0 )
at varlena.c:152
#2  0x008046a3 in InputFunctionCall (
flinfo=flinfo@entry=0x7fffa02d0250,
str=str@entry=0x16f238af0 0x16f238af0>, typioparam=typioparam@entry=25, typmod=typmod@entry=-1) at 
fmgr.c:1909

#3  0x00804971 in OidInputFunctionCall (functionId=,
str=0x16f238af0 ,
typioparam=25, typmod=-1) at fmgr.c:2040
#4  0x006aa485 in SlotStoreCStrings (slot=0x2748670,
values=0x7fffa02d0330) at apply.c:569
#5  0x006ab45c in handle_insert (s=0x274d088) at apply.c:756
#6  0x006abcea in handle_message (s=0x7fffa02d3e20) at apply.c:978
#7  LogicalRepApplyLoop (last_received=117457680) at apply.c:1146
#8  0x006ac37e in ApplyWorkerMain (main_arg=)
at apply.c:1530


In SlotStoreCStrings values only has 2 elements but natts is 4




Changes:
- I moved the publication.c to pg_publication.c, subscription.c to 
pg_subscription.c.

- changed \drp and \drs to \dRp and \dRs
- fixed definitions of the catalogs (BKI_ROWTYPE_OID)
- changed some GetPublication calls to get_publication_name
- fixed getObjectIdentityParts for OCLASS_PUBLICATION_REL
- fixed get_object_address_publication_rel
- fixed the dependencies between pkeys and publications, for this I 
actually had to add new interface to depenency.c that allows dropping 
single dependency

- fixed the 'for all tables' and 'for tables all in schema' publications
- changed the alter publication from FOR to SET
- added more test cases for the publication DDL
- fixed compilation of subscription patch alone and docs
- changed subpublications to name[]
- added check for publication list duplicates
- made the subscriptions behave more like they are inside the database 
instead of shared catalog (even though the catalog is still shared)
- added options for for CREATE SUBSCRIPTION to optionally not create 
slot and not do the initial data sync - that should solve the 
complaint about CREATE SUBSCRIPTION always connecting
- the CREATE SUBSCRIPTION also tries to check if the specified 
connection connects back to same db (although that check is somewhat 
imperfect) and if it gets stuck on create slot it should be normally 
cancelable (that should solve the issue Steve Singer had)

- fixed the tests to work in any timezone
- added DDL regress tests for subscription
- added proper detection of missing schemas and tables on subscriber
- rebased on top of 19acee8 as the DefElem changes broke the patch

The table sync is still far from ready.







--
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] Stopping logical replication protocol

[Vladimir Gordiychuk]

New patch in attach. 0001 and 0002 without changes.
0003 - patch contain improvements for pg_recvloginca, now pg_recvloginca
after receive SIGINT will send CopyDone package to postgresql and wait from
server CopyDone. For backward compatible after repeat send SIGINT
 pg_recvloginca will continue immediately without wait CopyDone from
server.
0004 patch contain regression tests on scenarios that fix 0001 and 0002
patches.

2016-09-16 10:11 GMT+03:00 Vladimir Gordiychuk :

> >Have you had a chance to look at adding the tests we discussed?
>
> Not yet. I plane do it on this weekends
>
> 2016-09-16 4:37 GMT+03:00 Craig Ringer :
>
>> On 9 September 2016 at 12:03, Craig Ringer  wrote:
>>
>> > Setting "waiting on author" in CF per discussion of the need for tests.
>>
>> Have you had a chance to look at adding the tests we discussed?
>>
>> --
>>  Craig Ringer   http://www.2ndQuadrant.com/
>>  PostgreSQL Development, 24x7 Support, Training & Services
>>
>
>
From 97da2c500f8f8edc69bcd520096c686e99e52612 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Tue, 10 May 2016 10:34:10 +0800
Subject: [PATCH 1/4] Respect client-initiated CopyDone in walsender

The walsender never looked for CopyDone sent by the client unless it
had already decided it was done sending data and dispatched its own
CopyDone message.

Check for client-initiated CopyDone when in COPY BOTH mode, returning to
command mode. In logical decoding this will allow the client to end a logical
decoding session between transactions without just unilaterally closing its
connection.

This change does not allow a client to end COPY BOTH session in the middle of
processing a logical decoding block.

TODO effect on physical walsender?
---
 src/backend/replication/walsender.c | 32 +++-
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 1ea2a5c..c43310c 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -759,6 +759,13 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req
 	/* make sure we have enough WAL available */
 	flushptr = WalSndWaitForWal(targetPagePtr + reqLen);
 
+	/*
+	 * If the client sent CopyDone while we were waiting,
+	 * bail out so we can wind up the decoding session.
+	 */
+	if (streamingDoneSending)
+		return -1;
+
 	/* more than one block available */
 	if (targetPagePtr + XLOG_BLCKSZ <= flushptr)
 		count = XLOG_BLCKSZ;
@@ -1220,8 +1227,11 @@ WalSndWaitForWal(XLogRecPtr loc)
 		 * It's important to do this check after the recomputation of
 		 * RecentFlushPtr, so we can send all remaining data before shutting
 		 * down.
+		 *
+		 * We'll also exit here if the client sent CopyDone because it wants
+		 * to return to command mode.
 		 */
-		if (walsender_ready_to_stop)
+		if (walsender_ready_to_stop || streamingDoneReceiving)
 			break;
 
 		/*
@@ -1787,7 +1797,14 @@ WalSndCheckTimeOut(TimestampTz now)
 	}
 }
 
-/* Main loop of walsender process that streams the WAL over Copy messages. */
+/*
+ * Main loop of walsender process that streams the WAL over Copy messages.
+ *
+ * The send_data callback must enqueue complete CopyData messages to libpq
+ * using pq_putmessage_noblock or similar, since the walsender loop may send
+ * CopyDone then exit and return to command mode in response to a client
+ * CopyDone between calls to send_data.
+ */
 static void
 WalSndLoop(WalSndSendDataCallback send_data)
 {
@@ -1850,10 +1867,15 @@ WalSndLoop(WalSndSendDataCallback send_data)
 		 * some more.  If there is some, we don't bother to call send_data
 		 * again until we've flushed it ... but we'd better assume we are not
 		 * caught up.
+		 *
+		 * If we're trying to finish sending and exit we shouldn't enqueue more
+		 * data to libpq. We need to finish writing out whatever we already
+		 * have in libpq's send buffer to maintain protocol sync so we still
+		 * need to loop until it's flushed.
 		 */
-		if (!pq_is_send_pending())
+		if (!pq_is_send_pending() && !streamingDoneSending)
 			send_data();
-		else
+		else if (!streamingDoneSending)
 			WalSndCaughtUp = false;
 
 		/* Try to flush pending output to the client */
@@ -2909,7 +2931,7 @@ WalSndKeepaliveIfNecessary(TimestampTz now)
 	if (wal_sender_timeout <= 0 || last_reply_timestamp <= 0)
 		return;
 
-	if (waiting_for_ping_response)
+	if (waiting_for_ping_response || streamingDoneSending)
 		return;
 
 	/*
-- 
1.9.1

From fcdcfe1aedfb3c7ef90c78c7d8acb4ca99a2ffdc Mon Sep 17 00:00:00 2001
From: Vladimir Gordiychuk 
Date: Wed, 7 Sep 2016 00:39:18 +0300
Subject: [PATCH 2/4] Client-initiated CopyDone during transaction decondig in
 walsender

The walsender never looked for client messages during decode transaction
by ReorderBufferCommit. It affect long transaction, even if client try

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

[Tomas Vondra]

On 09/18/2016 06:08 AM, Amit Kapila wrote:

On Sat, Sep 17, 2016 at 11:25 PM, Tomas Vondra
 wrote:

On 09/17/2016 07:05 AM, Amit Kapila wrote:


On Sat, Sep 17, 2016 at 9:17 AM, Tomas Vondra
 wrote:


On 09/14/2016 05:29 PM, Robert Haas wrote:


...


Sure, but you're testing at *really* high client counts here.
Almost nobody is going to benefit from a 5% improvement at 256
clients. You need to test 64 clients and 32 clients and 16
clients and 8 clients and see what happens there. Those cases are
a lot more likely than these stratospheric client counts.



Right. My impression from the discussion so far is that the patches
only improve performance with very many concurrent clients - but as
Robert points out, almost no one is running with 256 active
clients, unless they have 128 cores or so. At least not if they
value latency more than throughput.



See, I am also not in favor of going with any of these patches, if
they doesn't help in reduction of contention. However, I think it is
important to understand, under what kind of workload and which
environment it can show the benefit or regression whichever is
applicable.



Sure. Which is why I initially asked what type of workload should I be
testing, and then done the testing with multiple savepoints as that's what
you suggested. But apparently that's not a workload that could benefit from
this patch, so I'm a bit confused.


Just FYI, couple of days back one of EDB's partner who was doing the
performance tests by using HammerDB (which is again OLTP focussed
workload) on 9.5 based code has found that CLogControlLock has the
significantly high contention. They were using synchronous_commit=off
in their settings. Now, it is quite possible that with improvements
done in 9.6, the contention they are seeing will be eliminated, but
we have yet to figure that out. I just shared this information to you
with the intention that this seems to be a real problem and we should
try to work on it unless we are able to convince ourselves that this
is not a problem.



So, can we approach the problem from this direction instead? That is,
instead of looking for workloads that might benefit from the patches, look
at real-world examples of CLOG lock contention and then evaluate the impact
on those?



Sure, we can go that way as well, but I thought instead of testing
with a new benchmark kit (HammerDB), it is better to first get with
some simple statements.



IMHO in the ideal case the first message in this thread would provide a 
test case, demonstrating the effect of the patch. Then we wouldn't have 
the issue of looking for a good workload two years later.


But now that I look at the first post, I see it apparently used a plain 
tpc-b pgbench (with synchronous_commit=on) to show the benefits, which 
is the workload I'm running right now (results sometime tomorrow).


That workload clearly uses no savepoints at all, so I'm wondering why 
you suggested to use several of them - I know you said that it's to show 
differences between the approaches, but why should that matter to any of 
the patches (and if it matters, why I got almost no differences in the 
benchmarks)?


Pardon my ignorance, CLOG is not my area of expertise ...


Extracting the workload from benchmarks probably is not ideal, but
it's still better than constructing the workload on our own to fit
the patch.

FWIW I'll do a simple pgbench test - first with
synchronous_commit=on and then with synchronous_commit=off.
Probably the workloads we should have started with anyway, I
guess.



Here, synchronous_commit = off case could be interesting. Do you see
any problem with first trying a workload where Dilip is seeing
benefit? I am not suggesting we should not do any other testing, but
just first lets try to reproduce the performance gain which is seen
in Dilip's tests.



I plan to run Dilip's workload once the current benchmarks complete.


regard

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


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

Re: [HACKERS] Logical Replication WIP

[Steve Singer]

On 09/08/2016 06:59 PM, Petr Jelinek wrote:
- the CREATE SUBSCRIPTION also tries to check if the specified 
connection connects back to same db (although that check is somewhat 
imperfect) and if it gets stuck on create slot it should be normally 
cancelable (that should solve the issue Steve Singer had) 




When I create my subscriber database by doing a physical backup of the 
publisher cluster (with cp before I add any data) then I am unable to 
connect subscribe.

ie
initdb ../data
cp -r ../data ../data2
./postgres -D ../data
./postgres -D ../data2


This make sense when I look at your code, but it might not be what we want

I had the same issue when I created my subscriber cluster with 
pg_basebackup (The timeline on the destination cluster still shows as 1)






--
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] IF (NOT) EXISTS in psql-completion

[Pavel Stehule]

Hi

2016-09-16 10:31 GMT+02:00 Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp>:

> Hello, this is the new version.
>
> At Tue, 13 Sep 2016 10:50:13 +0900 (Tokyo Standard Time), Kyotaro
> HORIGUCHI  wrote in <
> 20160913.105013.65452566.horiguchi.kyot...@lab.ntt.co.jp>
> > > > This patch consists of the following files. Since these files are
> > > > splitted in strange criteria and order for historical reasons,
> > > > I'll reorganize this and post them later.
>
> The focus of this patch has changed. The first motivation was
> completing IF-EXISTS but the underlying issue was flexibility of
> psql_completion. And as Pavel's suggestion, keywords suggested
> along with database objects should follow the character case of
> input.
>
> For the purpose of resolving the issues, I reorganized the
> confused patch set. The attached patches are organized as the
> following.
>
> 1. Refactoring tab-complete to make psql_completion code
>
>   Does two things. One is moving out the macros that has grown to
>   be too large to stay in tab_completion.c to new file
>   tab-complete-macros.h The other is separating out the else-if
>   sequence in psql_completion() as a new function
>   psql_completion_internal(). This allows us to the following
>   things.
>
>   - Exit from arbitrary place in the former-else-if sequence just
> by return.
>
>   - Do other than "if(matching) { completion }" in anywhere
> convenient in the midst of the former-els...
>
>   - Recursively matching for sub syntaxes. EXPLAIN, RULE and
> others are using this feature. (Needs the 4th patch to do
> this, though)
>
> 2. Make keywords' case follow to input
>
>   Allow the keywords suggested along with databse objects to
>   follow the input letter case. The core part of this patch is a
>   new function additional_kw_query(), which dynamically generates
>   additional query string with specified keywords in the desired
>   letter case. COMPLETE_WITH_* macros are modified to accept the
>   function.
>
> 3. Fix suggested keywords to follow input in tab-completion session 2
>
>   The 2nd patch above leaves some query string containing static
>   keyword strings, which results in failure to follow input
>   letter cases. Most of them are naturally removed but role names
>   are a bother. This patch puts additional query strings for
>   several usage of roles but it might be overdone.
>
> 4. Introduce word shift and removal feature to psql-completion
>
>   This is the second core for the flexibility of completion code.
>   The word shift feature is the ability to omit first several
>   words in *MatchesN macros. For example this allows complete
>   create-schema's schema elements in a natural code. (Currently
>   those syntaxes that can be a schema elements are using
>   TailMatches instead of Matches, as the result HeadMatches are
>   not available there). The words removing feature is the ability
>   to (desructively) clip multiple suceessive words in the
>   previous_words list. This feature allows suceeding completion
>   code not to care about the removed words, such like UNIQUE,
>   CONCURRENTLY, VERBOSE and so on.
>
> 5. Add suggestion for IF (NOT) EXISTS for some syntaxes
>
>   This adds IF (NOT) EXISTS suggestion, as a PoC.  This patch no
>   loger covers all adoptable syntaces since the places where more
>   than boilerplating is required are omitted.
>

I am working on some initial tests - a compilation, a patching is ok.
Autocomplete for DROP TABLE IF EXISTS doesn't work. CREATE TABLE IF NOT
EXIST works well

Regards

Pavel



>
> regards,
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
>
>
>
>
>

Re: [HACKERS] PL/Python adding support for multi-dimensional arrays

[Dave Cramer]

On 10 August 2016 at 01:53, Pavel Stehule  wrote:

> Hi
>
> 2016-08-03 13:54 GMT+02:00 Alexey Grishchenko :
>
>> On Wed, Aug 3, 2016 at 12:49 PM, Alexey Grishchenko <
>> agrishche...@pivotal.io> wrote:
>>
>>> Hi
>>>
>>> Current implementation of PL/Python does not allow the use of
>>> multi-dimensional arrays, for both input and output parameters. This forces
>>> end users to introduce workarounds like casting arrays to text before
>>> passing them to the functions and parsing them after, which is an
>>> error-prone approach
>>>
>>> This patch adds support for multi-dimensional arrays as both input and
>>> output parameters for PL/Python functions. The number of dimensions
>>> supported is limited by Postgres MAXDIM macrovariable, by default equal to
>>> 6. Both input and output multi-dimensional arrays should have fixed
>>> dimension sizes, i.e. 2-d arrays should represent MxN matrix, 3-d arrays
>>> represent MxNxK cube, etc.
>>>
>>> This patch does not support multi-dimensional arrays of composite types,
>>> as composite types in Python might be represented as iterators and there is
>>> no obvious way to find out when the nested array stops and composite type
>>> structure starts. For example, if we have a composite type of (int, text),
>>> we can try to return "[ [ [1,'a'], [2,'b'] ], [ [3,'c'], [4,'d'] ] ]", and
>>> it is hard to find out that the first two lists are lists, and the third
>>> one represents structure. Things are getting even more complex when you
>>> have arrays as members of composite type. This is why I think this
>>> limitation is reasonable.
>>>
>>> Given the function:
>>>
>>> CREATE FUNCTION test_type_conversion_array_int4(x int4[]) RETURNS
>>> int4[] AS $$
>>> plpy.info(x, type(x))
>>> return x
>>> $$ LANGUAGE plpythonu;
>>>
>>> Before patch:
>>>
>>> # SELECT * FROM test_type_conversion_array_int4(ARRAY[[1,2,3],[4,5,6]]);
>>> ERROR:  cannot convert multidimensional array to Python list
>>> DETAIL:  PL/Python only supports one-dimensional arrays.
>>> CONTEXT:  PL/Python function "test_type_conversion_array_int4"
>>>
>>>
>>> After patch:
>>>
>>> # SELECT * FROM test_type_conversion_array_int4(ARRAY[[1,2,3],[4,5,6]]);
>>> INFO:  ([[1, 2, 3], [4, 5, 6]], )
>>>  test_type_conversion_array_int4
>>> -
>>>  {{1,2,3},{4,5,6}}
>>> (1 row)
>>>
>>>
>>> --
>>> Best regards,
>>> Alexey Grishchenko
>>>
>>
>> Also this patch incorporates the fix for https://www.postgresql.org
>> /message-id/CAH38_tkwA5qgLV8zPN1OpPzhtkNKQb30n3xq-
>> 2NR9jUfv3qwHA%40mail.gmail.com, as they touch the same piece of code -
>> array manipulation in PL/Python
>>
>>
> I am sending review of this patch:
>
> 1. The implemented functionality is clearly benefit - passing MD arrays,
> pretty faster passing bigger arrays
> 2. I was able to use this patch cleanly without any errors or warnings
> 3. There is no any error or warning
> 4. All tests passed - I tested Python 2.7 and Python 3.5
> 5. The code is well commented and clean
> 6. For this new functionality the documentation is not necessary
>
> 7. I invite more regress tests for both directions (Python <-> Postgres)
> for more than two dimensions
>
> My only one objection is not enough regress tests - after fixing this
> patch will be ready for commiters.
>
> Good work, Alexey
>
> Thank you
>
> Regards
>
> Pavel
>
>
>> --
>> Best regards,
>> Alexey Grishchenko
>>
>
>

Pavel,

I will pick this up.



Dave Cramer

da...@postgresintl.com
www.postgresintl.com

Re: [HACKERS] Fix for PL/Python slow input arrays traversal issue

[Dave Cramer]

Pavel,

I will pick these up.

-- 
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] Event trigger and CREATE/ALTER ROLE/USER

[Michael Paquier]

On Wed, Sep 14, 2016 at 11:09 AM, Tatsuo Ishii  wrote:
>> As I understand the issue, the main reason is that event triggers
>> execute procedures, and those exist in a single database only.  If you
>> were to create an event trigger in database A, then a user gets created
>> in database B, your function would not be invoked, which becomes a
>> problem.
>
> Can't we just create the event trigger in database B as well? We have
> been living with similar situation, for example, functions for years.
> (a work around would be creating functions in template1. This only
> works for freshly created database though).

Just a random thought: wouldn't what you are looking for be more
reliable if you use the utility hook and have it loaded via
preload_shared_libraries?
-- 
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] pgbench - allow to store select results into variables

[Fabien COELHO]

Hello Amit,


[...]
Then you would get the -r cut at the end of the compound command. Thus the
current version gives full control of what will appear in the summary. If
I change "\into xxx\n" to mean "also cut here", then there is less control
on when the cut occurs when into is used.


So it means that position of \into affects where a compound command gets
cut for -r display.  I was just wondering if that was unintentional.


Yes, but it happens to work reasonnably:-)


The other thing I have considered is whether to implemented a "\gset"
syntax, as suggested by Pavel and Tom. Bar the aesthetic, the main issue I
have with it is that it does not work with compound commands, and what I
want is to get the values out of compound commands... because of my focus
on latency... so basically "\gset" does not do the job I want... Now I
recognize that other people would like it, so probably I'll do it anyway
in another patch.


So, psql's \gset does not work as desired for compound commands (viz. it
is only able to save the result of the last sub-command).


Yes.

You need to use \gset with each sub-command separately if no result 
should be discarded. Because of undesirable latency characteristics of 
sending multiple commands, you want to be able to modify compound 
command handling such that every sub-command's result could be saved.


Exactly.

In that regard, it's good that pgbench does not use PQexec() which only 
returns the result of the last sub-command if a compound command was 
issued through it.


Indeed!


 pgbench's
doCustom() currently discards all results by issuing discard_response().
You propose to change things such that results are intercepted in a new
function read_response(), values assigned to intovars corresponding to
each sub-command, and then discarded. The intovars arrays are allocated
within each sub-command's Command struct when parsing the compound command
based on where \into is located wrt to the sub-command.


Yep.


So, most of the code in the patch is about handling compound statements to
be be able to save results of all sub-commands, not just the last.


Yep. Previously pgbench did not need to handle compounds commands which 
where just seen as one large string.


Note that the added machinery is also a first step to allow the handling 
of prepared statements on compounds command, which I think is a desirable 
feature for benchmarks.


Do you think it would be OK to suffer the bad latency of multiple round 
trips and implement a version of \into (or \gset or \ginto) for pgbench 
scripts that behaves exactly like psql's \gset as a first step?


I do not see gset as a reasonnable "first step" because: (0) "\into" 
already works while "\gset" in pgbench will need some time that I do not 
have at the moment (1) it is not what I want/need to do a clean bench (2) 
the feature is not orthogonal to compounds statements -- which is what I 
want -- (3) I do not like the "implicit" naming thing -- but this is 
really just a matter of taste.


I simply recognize that Peter & Tom have a point: whatever I think of gset 
it is there in "psql" so it makes some sense to have it as well in 
"pgbench", so I agree to put that on my pgbench todo list.



But you say you will do it as another patch.


Yep. I suggested another patch because it is a different feature and 
previous submissions where I mixed features, even closely related ones, 
all resulted in me having to separate the features in distinct patches.



By the way, I tend to agree with your point about \gset syntax being
suitable (only) in a interactive context such as psql; it's not as
readable as \into x y ... when used in a script.


Yep, but as people already know it, it makes sense to provide it as well 
at some point.


--
Fabien.


--
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] ecpg -? option doesn't work in windows

[Heikki Linnakangas]

On 08/29/2016 09:10 AM, Haribabu Kommi wrote:

ecpg option --help alternative -? doesn't work in windows.
In windows, the PG provided getopt_long function is used
for reading the provided options.

The getopt_long function returns '?' for invalid characters
also but it sets optopt option to 0 in case if the character
itself is a '?'. But this works for Linux and others, whereas
for windows, optopt is not 0. Because of this reason it is
failing.

I feel, from this commit 5b88b85c on wards, it is not working.
I feel instead of fixing the getopt_long function to reset optopt
parameter to zero whenever it is returning '?', I prefer fixing
the ecpg in handling the version and help options seperate.


Agreed. This does have one annoying consequence, though: --help and 
--version are now only accepted as the first argument. But that's 
consistent with most of our binaries. psql does this slightly 
differently, though, so e.g. "psql --t --help" works. It might be worth 
changing all our binaries to follow psql's example, but that's another 
story.



Patch is attached. Any one prefers the getopt_long function
fix, I can produce the patch for the same.


Committed, thanks!

- Heikki



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

Re: [HACKERS] patch: function xmltable

[Pavel Stehule]

Hi

new update:

* doc is moved to better place - xml processing functions
* few more regress tests
* call forgotten check_srf_call_placement

Regards

Pavel


xmltable-8.patch.gz
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] Declarative partitioning - another take

[Amit Langote]

On Thu, Sep 15, 2016 at 10:07 PM, Robert Haas  wrote:
> On Thu, Sep 15, 2016 at 4:53 AM, Amit Langote
>  wrote:
>> Wow, this is bad.  What is needed in this case is "canonicalization" of
>> the range partition bounds specified in the command.
>
> I think we shouldn't worry about this.  It seems like unnecessary
> scope creep.  All human beings capable of using PostgreSQL will
> understand that there are no integers between 4 and 5, so any
> practical impact on this would be on someone creating partitions
> automatically.  But if someone is creating partitions automatically
> they are highly likely to be using the same EXCLUSIVE/INCLUSIVE
> settings for all of the partitions, in which case this won't arise.
> And if they aren't, then I think we should just make them deal with
> this limitation in their code instead of dealing with it in our code.
> This patch is plenty complicated enough already; introducing a whole
> new canonicalization concept that will help practically nobody seems
> to me to be going in the wrong direction.  If somebody really cares
> enough to want to try to fix this, they can submit a followup patch
> someday.
>
>> To mitigate this, how about we restrict range partition key to contain
>> columns of only those types for which we know we can safely canonicalize a
>> range bound (ie, discrete range types)?  I don't think we can use, say,
>> existing int4range_canonical but will have to write a version of it for
>> partitioning usage (range bounds of partitions are different from what
>> int4range_canonical is ready to handle).  This approach will be very
>> limiting as then range partitions will be limited to columns of int,
>> bigint and date type only.
>
> -1.  That is letting the tail wag the dog.  Let's leave it the way you
> had it and be happy.

Alright, let's leave this as something to work out later.  We will
have to document the fact that such limitation exists though, I'd
think.

>>> -- Observation 2 : able to create sub-partition out of the range set for
>>> main table, causing not able to insert data satisfying any of the partition.
>>>
>>> create table test_subpart (c1 int) partition by range (c1);
>>> create table test_subpart_p1 partition of test_subpart for values start (1)
>>> end (100) inclusive partition by range (c1);
>>> create table test_subpart_p1_sub1 partition of test_subpart_p1 for values
>>> start (101) end (200);
>>
>> It seems that DDL should prevent the same column being used in partition
>> key of lower level partitions.  I don't know how much sense it would make,
>> but being able to use the same column as partition key of lower level
>> partitions may be a feature useful to some users if they know what they
>> are doing.  But this last part doesn't sound like a good thing.  I
>> modified the patch such that lower level partitions cannot use columns
>> used by ancestor tables.
>
> I again disagree.  If somebody defines partition bounds that make it
> impossible to insert the data that they care about, that's operator
> error.  The fact that, across multiple levels, you can manage to make
> it impossible to insert any data at all does not make it anything
> other than operator error.  If we take the contrary position that it's
> the system's job to prevent this sort of thing, we may disallow some
> useful cases, like partitioning by the year portion of a date and then
> subpartitioning by the month portion of that same date.
>
> I think we'll probably also find that we're making the code
> complicated to no purpose.  For example, now you have to check when
> attaching a partition that it doesn't violate the rule; otherwise you
> end up with a table that can't be created directly (and thus can't
> survive dump-and-restore) but can be created indirectly by exploiting
> loopholes in the checks.  It's tempting to think that we can check
> simple cases - e.g. if the parent and the child are partitioning on
> the same exact column, the child's range should be contained within
> the parent's range - but more complicated cases are tricky.  Suppose
> the table is range-partitioned on (a, b) and range-subpartitioned on
> b.  It's not trivial to figure out whether the set of values that the
> user can insert into that partition is non-empty.  If we allow
> partitioning on expressions, then it quickly becomes altogether
> impossible to deduce anything useful - unless you can solve the
> halting problem.
>
> And, really, why do we care?  If the user creates a partitioning
> scheme that permits no rows in some or all of the partitions, then
> they will have an empty table that can be correctly dumped and
> restored but which cannot be used for anything useful unless it is
> modified first.  They probably don't want that, but it's not any more
> broken than a table inheritance setup with mutually exclusive CHECK
> constraints, or for that matter a plain table with mutually exclusive
> CHECK constraints - and we don't