Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-25 Thread Kyotaro HORIGUCHI 
At Mon, 24 Jul 2017 10:23:07 +0530, Ashutosh Bapat 
 wrote in 

> On Fri, Jul 21, 2017 at 10:39 PM, Tom Lane  wrote:
> > Ashutosh Bapat  writes:
> >> On Fri, Jul 21, 2017 at 10:55 AM, Kyotaro HORIGUCHI
> >>  wrote:
> >>> The attached patch differs only in this point.
> >
> >> +1. The patch looks good to me.
> >
> > Pushed with a couple additional changes: we'd all missed that the header
> > comment for GetConnection was obsoleted by this change, and the arguments
> > for GetSysCacheHashValue really need to be coerced to Datum.  (I think
> > OID to Datum is the same as what the compiler would do anyway, but best
> > not to assume that.)
> 
> Thanks and sorry for not noticing the prologue.

Ditto.

> >
> > Back-patching was more exciting than I could wish.  It seems that
> > before 9.6, we did not have struct UserMapping storing the OID of the
> > pg_user_mapping row it had been made from.  I changed GetConnection to
> > re-look-up that row and get the OID.  But that's ugly, and there's a
> > race condition: if user mappings are being added or deleted meanwhile,
> > we might locate a per-user mapping when we're really using a PUBLIC
> > mapping or vice versa, causing the ConnCacheEntry to be labeled with
> > the wrong hash value so that it might not get invalidated properly later.
> > Still, it's significantly better than it was, and that corner case seems
> > unlikely to get hit in practice --- for one thing, you'd have to then
> > revert the mapping addition/deletion before the ConnCacheEntry would be
> > found and used again.  I don't want to take the risk of modifying struct
> > UserMapping in stable branches, so it's hard to see a way to make that
> > completely bulletproof before 9.6.
> 
> +1.

Agreed.

> -- 
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company

-- 
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

Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-23 Thread Ashutosh Bapat 
On Fri, Jul 21, 2017 at 10:39 PM, Tom Lane  wrote:
> Ashutosh Bapat  writes:
>> On Fri, Jul 21, 2017 at 10:55 AM, Kyotaro HORIGUCHI
>>  wrote:
>>> The attached patch differs only in this point.
>
>> +1. The patch looks good to me.
>
> Pushed with a couple additional changes: we'd all missed that the header
> comment for GetConnection was obsoleted by this change, and the arguments
> for GetSysCacheHashValue really need to be coerced to Datum.  (I think
> OID to Datum is the same as what the compiler would do anyway, but best
> not to assume that.)

Thanks and sorry for not noticing the prologue.

>
> Back-patching was more exciting than I could wish.  It seems that
> before 9.6, we did not have struct UserMapping storing the OID of the
> pg_user_mapping row it had been made from.  I changed GetConnection to
> re-look-up that row and get the OID.  But that's ugly, and there's a
> race condition: if user mappings are being added or deleted meanwhile,
> we might locate a per-user mapping when we're really using a PUBLIC
> mapping or vice versa, causing the ConnCacheEntry to be labeled with
> the wrong hash value so that it might not get invalidated properly later.
> Still, it's significantly better than it was, and that corner case seems
> unlikely to get hit in practice --- for one thing, you'd have to then
> revert the mapping addition/deletion before the ConnCacheEntry would be
> found and used again.  I don't want to take the risk of modifying struct
> UserMapping in stable branches, so it's hard to see a way to make that
> completely bulletproof before 9.6.

+1.

-- 
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] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-21 Thread Tom Lane 
Ashutosh Bapat  writes:
> On Fri, Jul 21, 2017 at 10:55 AM, Kyotaro HORIGUCHI
>  wrote:
>> The attached patch differs only in this point.

> +1. The patch looks good to me.

Pushed with a couple additional changes: we'd all missed that the header
comment for GetConnection was obsoleted by this change, and the arguments
for GetSysCacheHashValue really need to be coerced to Datum.  (I think
OID to Datum is the same as what the compiler would do anyway, but best
not to assume that.)

Back-patching was more exciting than I could wish.  It seems that
before 9.6, we did not have struct UserMapping storing the OID of the
pg_user_mapping row it had been made from.  I changed GetConnection to
re-look-up that row and get the OID.  But that's ugly, and there's a
race condition: if user mappings are being added or deleted meanwhile,
we might locate a per-user mapping when we're really using a PUBLIC
mapping or vice versa, causing the ConnCacheEntry to be labeled with
the wrong hash value so that it might not get invalidated properly later.
Still, it's significantly better than it was, and that corner case seems
unlikely to get hit in practice --- for one thing, you'd have to then
revert the mapping addition/deletion before the ConnCacheEntry would be
found and used again.  I don't want to take the risk of modifying struct
UserMapping in stable branches, so it's hard to see a way to make that
completely bulletproof before 9.6.

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] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-21 Thread Ashutosh Bapat 
On Fri, Jul 21, 2017 at 10:55 AM, Kyotaro HORIGUCHI
 wrote:
> At Thu, 20 Jul 2017 18:15:42 -0400, Tom Lane  wrote in 
> <18927.1500588...@sss.pgh.pa.us>
>> This seems like overkill.  We can test it reasonably easily within the
>> existing framework, as shown in the attached patch.  I'm also fairly
>
> It checks for a disconnection caused in a single session. I
> thought that its inter-process characteristics is important
> (since I had forgot that in the previous version), but it is
> reasonable enough if we can rely on the fact that it surely works
> through invalidation mechanism.
>
> In shoft, I agree to the test in your patch.
>
>> concerned that what you're showing here would be unstable in the buildfarm
>> as a result of race conditions between the multiple sessions.
>
> Sure. It is what I meant by 'fragile'.
>
>> I made some cosmetic updates to the code patch, as well.
>
> Thank you for leaving the hashvalue staff and revising the comment.
>
> By the way I mistakenly had left the following code in the
> previous patch.
>
> + /* hashvalue == 0 means a cache reset, must clear all state */
> + if (hashvalue == 0)
> +   entry->invalidated = true;
> + else if ((cacheid == FOREIGNSERVEROID &&
> +   entry->server_hashvalue == hashvalue) ||
> +  (cacheid == USERMAPPINGOID &&
> +   entry->mapping_hashvalue == hashvalue))
> +   entry->invalidated = true;
>
> The reason for the redundancy was that it had used switch-case in
> the else block just before. However, it is no longer
> reasonable. I'd like to change here as the follows.
>
> + /* hashvalue == 0 means a cache reset, must clear all state */
> + if ((hashvalue == 0) ||
> + ((cacheid == FOREIGNSERVEROID &&
> +   entry->server_hashvalue == hashvalue) ||
> +  (cacheid == USERMAPPINGOID &&
> +   entry->mapping_hashvalue == hashvalue)))
> +   entry->invalidated = true;
>
> The attached patch differs only in this point.
>

+1. The patch looks good to me.


-- 
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] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-20 Thread Michael Paquier 
On Fri, Jul 21, 2017 at 12:23 AM, Alvaro Herrera
 wrote:
> Kyotaro HORIGUCHI wrote:
>> Finally, I added a new TAP test library PsqlSession. It offers
>> interactive psql sessions. Then added a simple test to
>> postgres_fdw using it.
>
> Hmm, I think this can be very useful for other things.  Let's keep this
> in mind to use in the future, even if we find another way to fix the
> issue at hand.  In fact, I had a problem a couple of weeks ago in which
> I needed two concurrent sessions and one of them disconnected in the
> middle of the test.

Agreed, I wanted the ability to hold a session at hand a couple of
times already for tests. And I agree with the point of having a
separate discussion for such things out of the scope of a bug fix.
Thinking larger, I think that it would be more helpful to hold
processes and run commands in parallel, say for pg_receivewal.

> Can't do that with isolationtester ...

In the pglogical fork of Postgres, you guys improved isolationtester
to handle multiple hosts, right? That sounds harder to integrate than
a perl module though, as isolation tester starts only one server.
-- 
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] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-20 Thread Kyotaro HORIGUCHI 
At Thu, 20 Jul 2017 18:23:05 -0400, Alvaro Herrera  
wrote in <20170720222305.ij3pk7qw5im3wozr@alvherre.pgsql>
> Kyotaro HORIGUCHI wrote:
> 
> > Finally, I added a new TAP test library PsqlSession. It offers
> > interactive psql sessions. Then added a simple test to
> > postgres_fdw using it.
> 
> Hmm, I think this can be very useful for other things.  Let's keep this
> in mind to use in the future, even if we find another way to fix the
> issue at hand.  In fact, I had a problem a couple of weeks ago in which
> I needed two concurrent sessions and one of them disconnected in the
> middle of the test.  Can't do that with isolationtester ...

Thanks. I agree that it still useful to write more complex
tests. The most significant issue on this (PsqlSession.pm) comes
from the fact that I didn't find the way to detect the end of an
query execution without attaching a bogus query.. And this kind
of things tend to be unstable on an high-load environment.

regards,

-- 
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

Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-20 Thread Kyotaro HORIGUCHI 
At Thu, 20 Jul 2017 18:15:42 -0400, Tom Lane  wrote in 
<18927.1500588...@sss.pgh.pa.us>
> This seems like overkill.  We can test it reasonably easily within the
> existing framework, as shown in the attached patch.  I'm also fairly

It checks for a disconnection caused in a single session. I
thought that its inter-process characteristics is important
(since I had forgot that in the previous version), but it is
reasonable enough if we can rely on the fact that it surely works
through invalidation mechanism.

In shoft, I agree to the test in your patch.

> concerned that what you're showing here would be unstable in the buildfarm
> as a result of race conditions between the multiple sessions.

Sure. It is what I meant by 'fragile'.

> I made some cosmetic updates to the code patch, as well.

Thank you for leaving the hashvalue staff and revising the comment.

By the way I mistakenly had left the following code in the
previous patch.

+ /* hashvalue == 0 means a cache reset, must clear all state */
+ if (hashvalue == 0)
+   entry->invalidated = true;
+ else if ((cacheid == FOREIGNSERVEROID &&
+   entry->server_hashvalue == hashvalue) ||
+  (cacheid == USERMAPPINGOID &&
+   entry->mapping_hashvalue == hashvalue))
+   entry->invalidated = true;

The reason for the redundancy was that it had used switch-case in
the else block just before. However, it is no longer
reasonable. I'd like to change here as the follows.

+ /* hashvalue == 0 means a cache reset, must clear all state */
+ if ((hashvalue == 0) ||
+ ((cacheid == FOREIGNSERVEROID &&
+   entry->server_hashvalue == hashvalue) ||
+  (cacheid == USERMAPPINGOID &&
+   entry->mapping_hashvalue == hashvalue)))
+   entry->invalidated = true;

The attached patch differs only in this point.

> I think this is actually a bug fix, and should not wait for the next
> commitfest.

Agreed.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
*** a/contrib/postgres_fdw/connection.c
--- b/contrib/postgres_fdw/connection.c
***
*** 22,27 
--- 22,28 
  #include "pgstat.h"
  #include "storage/latch.h"
  #include "utils/hsearch.h"
+ #include "utils/inval.h"
  #include "utils/memutils.h"
  #include "utils/syscache.h"
  
***
*** 48,58  typedef struct ConnCacheEntry
--- 49,63 
  {
  	ConnCacheKey key;			/* hash key (must be first) */
  	PGconn	   *conn;			/* connection to foreign server, or NULL */
+ 	/* Remaining fields are invalid when conn is NULL: */
  	int			xact_depth;		/* 0 = no xact open, 1 = main xact open, 2 =
   * one level of subxact open, etc */
  	bool		have_prep_stmt; /* have we prepared any stmts in this xact? */
  	bool		have_error;		/* have any subxacts aborted in this xact? */
  	bool		changing_xact_state;	/* xact state change in process */
+ 	bool		invalidated;	/* true if reconnect is pending */
+ 	uint32		server_hashvalue;	/* hash value of foreign server OID */
+ 	uint32		mapping_hashvalue;	/* hash value of user mapping OID */
  } ConnCacheEntry;
  
  /*
***
*** 69,74  static bool xact_got_connection = false;
--- 74,80 
  
  /* prototypes of private functions */
  static PGconn *connect_pg_server(ForeignServer *server, UserMapping *user);
+ static void disconnect_pg_server(ConnCacheEntry *entry);
  static void check_conn_params(const char **keywords, const char **values);
  static void configure_remote_session(PGconn *conn);
  static void do_sql_command(PGconn *conn, const char *sql);
***
*** 78,83  static void pgfdw_subxact_callback(SubXactEvent event,
--- 84,90 
  	   SubTransactionId mySubid,
  	   SubTransactionId parentSubid,
  	   void *arg);
+ static void pgfdw_inval_callback(Datum arg, int cacheid, uint32 hashvalue);
  static void pgfdw_reject_incomplete_xact_state_change(ConnCacheEntry *entry);
  static bool pgfdw_cancel_query(PGconn *conn);
  static bool pgfdw_exec_cleanup_query(PGconn *conn, const char *query,
***
*** 130,135  GetConnection(UserMapping *user, bool will_prep_stmt)
--- 137,146 
  		 */
  		RegisterXactCallback(pgfdw_xact_callback, NULL);
  		RegisterSubXactCallback(pgfdw_subxact_callback, NULL);
+ 		CacheRegisterSyscacheCallback(FOREIGNSERVEROID,
+ 	  pgfdw_inval_callback, (Datum) 0);
+ 		CacheRegisterSyscacheCallback(USERMAPPINGOID,
+ 	  pgfdw_inval_callback, (Datum) 0);
  	}
  
  	/* Set flag that we did GetConnection during the current transaction */
***
*** 144,161  GetConnection(UserMapping *user, bool will_prep_stmt)
  	entry = hash_search(ConnectionHash, , HASH_ENTER, );
  	if (!found)
  	{
! 		/* initialize new hashtable entry (key is already filled in) */
  		entry->conn = NULL;
- 		entry->xact_depth = 0;
- 		entry->have_prep_stmt = false;
- 		entry->have_error = false;
- 		entry->changing_xact_state = false;
  	}
  
  	/* Reject further use of connections

Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-20 Thread Alvaro Herrera 
Kyotaro HORIGUCHI wrote:

> Finally, I added a new TAP test library PsqlSession. It offers
> interactive psql sessions. Then added a simple test to
> postgres_fdw using it.

Hmm, I think this can be very useful for other things.  Let's keep this
in mind to use in the future, even if we find another way to fix the
issue at hand.  In fact, I had a problem a couple of weeks ago in which
I needed two concurrent sessions and one of them disconnected in the
middle of the test.  Can't do that with isolationtester ...

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


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

Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-20 Thread Tom Lane 
Kyotaro HORIGUCHI  writes:
> Here it is. First I tried this using ordinary regression
> framework but the effect of this patch is shown only in log and
> it contains variable parts so I gave up it before trying more
> complex way.

> Next I tried existing TAP test but this test needs continuous
> session to achieve alternating operation on two sessions but
> PostgresNode::psql doesn't offer such a functionality.

> Finally, I added a new TAP test library PsqlSession. It offers
> interactive psql sessions. Then added a simple test to
> postgres_fdw using it.

This seems like overkill.  We can test it reasonably easily within the
existing framework, as shown in the attached patch.  I'm also fairly
concerned that what you're showing here would be unstable in the buildfarm
as a result of race conditions between the multiple sessions.

I made some cosmetic updates to the code patch, as well.

I think this is actually a bug fix, and should not wait for the next
commitfest.

regards, tom lane

diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 8c33dea..8eb477b 100644
*** a/contrib/postgres_fdw/connection.c
--- b/contrib/postgres_fdw/connection.c
***
*** 22,27 
--- 22,28 
  #include "pgstat.h"
  #include "storage/latch.h"
  #include "utils/hsearch.h"
+ #include "utils/inval.h"
  #include "utils/memutils.h"
  #include "utils/syscache.h"
  
*** typedef struct ConnCacheEntry
*** 48,58 
--- 49,63 
  {
  	ConnCacheKey key;			/* hash key (must be first) */
  	PGconn	   *conn;			/* connection to foreign server, or NULL */
+ 	/* Remaining fields are invalid when conn is NULL: */
  	int			xact_depth;		/* 0 = no xact open, 1 = main xact open, 2 =
   * one level of subxact open, etc */
  	bool		have_prep_stmt; /* have we prepared any stmts in this xact? */
  	bool		have_error;		/* have any subxacts aborted in this xact? */
  	bool		changing_xact_state;	/* xact state change in process */
+ 	bool		invalidated;	/* true if reconnect is pending */
+ 	uint32		server_hashvalue;	/* hash value of foreign server OID */
+ 	uint32		mapping_hashvalue;	/* hash value of user mapping OID */
  } ConnCacheEntry;
  
  /*
*** static bool xact_got_connection = false;
*** 69,74 
--- 74,80 
  
  /* prototypes of private functions */
  static PGconn *connect_pg_server(ForeignServer *server, UserMapping *user);
+ static void disconnect_pg_server(ConnCacheEntry *entry);
  static void check_conn_params(const char **keywords, const char **values);
  static void configure_remote_session(PGconn *conn);
  static void do_sql_command(PGconn *conn, const char *sql);
*** static void pgfdw_subxact_callback(SubXa
*** 78,83 
--- 84,90 
  	   SubTransactionId mySubid,
  	   SubTransactionId parentSubid,
  	   void *arg);
+ static void pgfdw_inval_callback(Datum arg, int cacheid, uint32 hashvalue);
  static void pgfdw_reject_incomplete_xact_state_change(ConnCacheEntry *entry);
  static bool pgfdw_cancel_query(PGconn *conn);
  static bool pgfdw_exec_cleanup_query(PGconn *conn, const char *query,
*** GetConnection(UserMapping *user, bool wi
*** 130,135 
--- 137,146 
  		 */
  		RegisterXactCallback(pgfdw_xact_callback, NULL);
  		RegisterSubXactCallback(pgfdw_subxact_callback, NULL);
+ 		CacheRegisterSyscacheCallback(FOREIGNSERVEROID,
+ 	  pgfdw_inval_callback, (Datum) 0);
+ 		CacheRegisterSyscacheCallback(USERMAPPINGOID,
+ 	  pgfdw_inval_callback, (Datum) 0);
  	}
  
  	/* Set flag that we did GetConnection during the current transaction */
*** GetConnection(UserMapping *user, bool wi
*** 144,161 
  	entry = hash_search(ConnectionHash, , HASH_ENTER, );
  	if (!found)
  	{
! 		/* initialize new hashtable entry (key is already filled in) */
  		entry->conn = NULL;
- 		entry->xact_depth = 0;
- 		entry->have_prep_stmt = false;
- 		entry->have_error = false;
- 		entry->changing_xact_state = false;
  	}
  
  	/* Reject further use of connections which failed abort cleanup. */
  	pgfdw_reject_incomplete_xact_state_change(entry);
  
  	/*
  	 * We don't check the health of cached connection here, because it would
  	 * require some overhead.  Broken connection will be detected when the
  	 * connection is actually used.
--- 155,182 
  	entry = hash_search(ConnectionHash, , HASH_ENTER, );
  	if (!found)
  	{
! 		/*
! 		 * We need only clear "conn" here; remaining fields will be filled
! 		 * later when "conn" is set.
! 		 */
  		entry->conn = NULL;
  	}
  
  	/* Reject further use of connections which failed abort cleanup. */
  	pgfdw_reject_incomplete_xact_state_change(entry);
  
  	/*
+ 	 * If the connection needs to be remade due to invalidation, disconnect as
+ 	 * soon as we're out of all transactions.
+ 	 */
+ 	if (entry->conn != NULL && entry->invalidated && entry->xact_depth == 0)
+ 	{
+ 		elog(DEBUG3, "closing

Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-20 Thread Kyotaro HORIGUCHI 
Finally, I added new TAP test library PsqlSession.

At Tue, 18 Jul 2017 18:12:13 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20170718.181213.206979369.horiguchi.kyot...@lab.ntt.co.jp>
> > * How about some regression test cases?  You couldn't really exercise
> > cross-session invalidation easily, but I don't think you need to.
> 
> Ha Ha. You got me. I will add some test cases for this in the
> next version. Thanks.

Here it is. First I tried this using ordinary regression
framework but the effect of this patch is shown only in log and
it contains variable parts so I gave up it before trying more
complex way.

Next I tried existing TAP test but this test needs continuous
session to achieve alternating operation on two sessions but
PostgresNode::psql doesn't offer such a functionality.

Finally, I added a new TAP test library PsqlSession. It offers
interactive psql sessions. Then added a simple test to
postgres_fdw using it.

The first patch is the PsqlSession.pm and the second is the new
test for postgres_fdw.

- The current PsqlSession is quite fragile but seems working
  enough for this usage at the present.

- I'm afraid this might not work on Windows according to the
  manpage of IPC::Run, but I haven't confirmed yet.

  http://search.cpan.org/~toddr/IPC-Run-0.96/lib/IPC/Run.pm#Win32_LIMITATIONS


Any comment or suggestions are welcome.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From fdb5cbab3375d9d2e4da078cf6ee7eaf7de5c8fd Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 20 Jul 2017 14:56:51 +0900
Subject: [PATCH 1/2] Add new TAP test library PsqlSession.pm

PostgreNode::psql makes temporary session to run commands so it is not
usable when more interactive operation on a continued session. This
library offers continuous sessions feature that can execute multiple
sql commands separately.
---
 src/test/perl/PsqlSession.pm | 341 +++
 1 file changed, 341 insertions(+)
 create mode 100644 src/test/perl/PsqlSession.pm

diff --git a/src/test/perl/PsqlSession.pm b/src/test/perl/PsqlSession.pm
new file mode 100644
index 000..d69fd14
--- /dev/null
+++ b/src/test/perl/PsqlSession.pm
@@ -0,0 +1,341 @@
+
+=pod
+
+=head1 NAME
+
+PsqlSession - class representing PostgreSQL psql instance
+
+=head1 SYNOPSIS
+
+  use PsqlSession;
+
+  my $session = get_new_session('session1', $server);
+
+  to connect to a PostgreNode as $server, or
+
+  my $session = get_new_session('session1', 'localhost', '5432', 'postgres');
+
+  to specify each options explicitly.
+
+  # Connect to the server
+  $session->open();
+
+  # Execute an SQL query
+  $ret = $session->execsql('SELECT now();');
+
+  Returns a pair of output of stdout, and stderr in array context.
+
+  ($out, $err) = $session->execsql('SELECT now();');
+
+  $session->execsql_multi('SELECT 1;', 'SELECT now();');
+
+  is just a shortcut of writing many execsqls.
+
+  # End the session
+  $session->close();
+
+=head1 DESCRIPTION
+
+PsqlSession contains a set of routines able to work on a psql session,
+allowing to connect, send a command and receive the result and close.
+
+The IPC::Run module is required.
+
+=cut
+
+package PsqlSession;
+
+use strict;
+use warnings;
+
+use Exporter 'import';
+use Test::More;
+use TestLib ();
+use Scalar::Util qw(blessed);
+
+our @EXPORT = qw(
+  get_new_session
+);
+
+
+=pod
+
+=head1 METHODS
+
+=over
+
+=item PsqlSession::new($class, $name, $pghost, $pgport, $dbname)
+
+Create a new PsqlSession instance. Does not connect.
+
+You should generally prefer to use get_new_session() instead since it
+takes care of finding host name, port number or database name.
+
+=cut
+
+sub new
+{
+	my ($class, $name, $pghost, $pgport, $dbname) = @_;
+
+	my $self = {
+		_name => $name,
+		_host => $pghost,
+		_port => $pgport,
+		_dbname => $dbname };
+
+	bless $self, $class;
+
+#	$self->dump_info;
+
+	return $self;
+}
+
+=pod
+
+=item $session->name()
+
+The name assigned to the session at creation time.
+
+=cut
+
+sub name
+{
+	return $_[0]->{_name};
+}
+
+=pod
+
+=item $session->host()
+
+Return the host (like PGHOST) for this instance. May be a UNIX socket path.
+
+=cut
+
+sub host
+{
+	return $_[0]->{_host};
+}
+
+=pod
+
+=item $session->port()
+
+Get the port number connects to. This won't necessarily be a TCP port
+open on the local host since we prefer to use unix sockets if
+possible.
+
+=cut
+
+sub port
+{
+	return $_[0]->{_port};
+}
+
+=pod
+
+=item $session->dbname()
+
+Get the database name this session connects to.
+
+=cut
+
+sub dbname
+{
+	return $_[0]->{_dbname};
+}
+
+=pod
+
+=item $session->errstate()
+
+Get the error state of this session. 0 means no error and 1 means
+error. This value is reset at the starting of every execution of an
+SQL query.
+
+=cut
+
+sub errstate
+{
+	return $_[0]->{_errstate};
+}
+
+=pod
+
+=item $session->open()
+
+Open this session.
+
+=cut
+
+sub open
+{

Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-18 Thread Kyotaro HORIGUCHI 
Thank you for the comments.

At Mon, 17 Jul 2017 16:09:04 -0400, Tom Lane  wrote in 
<9897.1500322...@sss.pgh.pa.us>
> Kyotaro HORIGUCHI  writes:
> > This is the revased and revised version of the previous patch.
> 
> A few more comments:
> 
> * Per the spec for CacheRegisterSyscacheCallback, a zero hash value means
> to flush all associated state.  This isn't handling that case correctly.

Right, fixed.

> Even when you are given a specific hash value, I think exiting after
> finding one match is incorrect: there could be multiple connections
> to the same server with different user mappings, or vice versa.

Sure. I'm confused that key hash value nails an entry in "the
connection cache". Thank you for pointing out that.

> * I'm not really sure that it's worth the trouble to pay attention
> to the hash value at all.  Very few other syscache callbacks do,
> and the pg_server/pg_user_mapping catalogs don't seem likely to
> have higher than average traffic.

Agreed to the points. But there is another point that reconection
is far intensive than re-looking up of a system catalog or maybe
even than replanning. For now I choosed to avoid a possibility of
causing massive number of simultaneous reconnection.

> * Personally I'd be inclined to register the callbacks at the same
> time the hashtables are created, which is a pattern we use elsewhere
> ... in fact, postgres_fdw itself does it that way for the transaction
> related callbacks, so it seems a bit weird that you are going in a
> different direction for these callbacks.  That way avoids the need to
> depend on a _PG_init function and means that the callbacks don't have to
> worry about the hashtables not being valid.

Sure, seems more reasonable than it is now. Changed the way of
registring a callback in the attached.

>  Also, it seems a bit
> pointless to have separate layers of postgresMappingSysCallback and
> InvalidateConnectionForMapping functions.

It used to be one function but it seemed a bit wierd that the
function is called from two places (two catalogs) then branchs
according to the caller. I don't have a firm opinion on this so
changed.

As the result the changes in postgres_fdw.c has been disappeard.

> * How about some regression test cases?  You couldn't really exercise
> cross-session invalidation easily, but I don't think you need to.

Ha Ha. You got me. I will add some test cases for this in the
next version. Thanks.


Ashutosh, I'll register this to the next CF after providing a
regression, thanks.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
*** a/contrib/postgres_fdw/connection.c
--- b/contrib/postgres_fdw/connection.c
***
*** 22,27 
--- 22,28 
  #include "pgstat.h"
  #include "storage/latch.h"
  #include "utils/hsearch.h"
+ #include "utils/inval.h"
  #include "utils/memutils.h"
  #include "utils/syscache.h"
  
***
*** 53,58  typedef struct ConnCacheEntry
--- 54,62 
  	bool		have_prep_stmt; /* have we prepared any stmts in this xact? */
  	bool		have_error;		/* have any subxacts aborted in this xact? */
  	bool		changing_xact_state;	/* xact state change in process */
+ 	bool		invalidated;	/* true if reconnect is requried */
+ 	uint32		server_hashvalue;	/* hash value of foreign server oid */
+ 	uint32		mapping_hashvalue;  /* hash value of user mapping oid */
  } ConnCacheEntry;
  
  /*
***
*** 69,74  static bool xact_got_connection = false;
--- 73,79 
  
  /* prototypes of private functions */
  static PGconn *connect_pg_server(ForeignServer *server, UserMapping *user);
+ static void disconnect_pg_server(ConnCacheEntry *entry);
  static void check_conn_params(const char **keywords, const char **values);
  static void configure_remote_session(PGconn *conn);
  static void do_sql_command(PGconn *conn, const char *sql);
***
*** 78,83  static void pgfdw_subxact_callback(SubXactEvent event,
--- 83,89 
  	   SubTransactionId mySubid,
  	   SubTransactionId parentSubid,
  	   void *arg);
+ static void pgfdw_inval_callback(Datum arg, int cacheid, uint32 hashvalue);
  static void pgfdw_reject_incomplete_xact_state_change(ConnCacheEntry *entry);
  static bool pgfdw_cancel_query(PGconn *conn);
  static bool pgfdw_exec_cleanup_query(PGconn *conn, const char *query,
***
*** 130,135  GetConnection(UserMapping *user, bool will_prep_stmt)
--- 136,145 
  		 */
  		RegisterXactCallback(pgfdw_xact_callback, NULL);
  		RegisterSubXactCallback(pgfdw_subxact_callback, NULL);
+ 		CacheRegisterSyscacheCallback(FOREIGNSERVEROID,
+ 	  pgfdw_inval_callback, (Datum)0);
+ 		CacheRegisterSyscacheCallback(USERMAPPINGOID,
+ 	  pgfdw_inval_callback, (Datum)0);
  	}
  
  	/* Set flag that we did GetConnection during the current transaction */
***
*** 144,160  GetConnection(UserMapping *user, bool will_prep_stmt)
  	entry = hash_search(ConnectionHash, ,

Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-17 Thread Tom Lane 
Kyotaro HORIGUCHI  writes:
> This is the revased and revised version of the previous patch.

A few more comments:

* Per the spec for CacheRegisterSyscacheCallback, a zero hash value means
to flush all associated state.  This isn't handling that case correctly.
Even when you are given a specific hash value, I think exiting after
finding one match is incorrect: there could be multiple connections
to the same server with different user mappings, or vice versa.

* I'm not really sure that it's worth the trouble to pay attention
to the hash value at all.  Very few other syscache callbacks do,
and the pg_server/pg_user_mapping catalogs don't seem likely to
have higher than average traffic.

* Personally I'd be inclined to register the callbacks at the same
time the hashtables are created, which is a pattern we use elsewhere
... in fact, postgres_fdw itself does it that way for the transaction
related callbacks, so it seems a bit weird that you are going in a
different direction for these callbacks.  That way avoids the need to
depend on a _PG_init function and means that the callbacks don't have to
worry about the hashtables not being valid.  Also, it seems a bit
pointless to have separate layers of postgresMappingSysCallback and
InvalidateConnectionForMapping functions.

* How about some regression test cases?  You couldn't really exercise
cross-session invalidation easily, but I don't think you need to.

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] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-14 Thread Ashutosh Bapat 
On Fri, Jul 14, 2017 at 2:04 PM, Kyotaro HORIGUCHI
 wrote:
> Thank you for the comments.
>
> At Thu, 13 Jul 2017 16:54:42 +0530, Ashutosh Bapat 
>  wrote in 
>

Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-14 Thread Kyotaro HORIGUCHI 
Thank you for the comments.

At Thu, 13 Jul 2017 16:54:42 +0530, Ashutosh Bapat 
 wrote in

Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-13 Thread Ashutosh Bapat 
On Thu, Jul 13, 2017 at 2:53 PM, Kyotaro HORIGUCHI
 wrote:
> Hello, moved to pgsql-hackers.
>
> This is the revased and revised version of the previous patch.
>
> At Thu, 13 Jul 2017 13:42:49 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
>  wrote in 
> <20170713.134249.97825982.horiguchi.kyot...@lab.ntt.co.jp>
>> At Tue, 11 Jul 2017 15:39:14 -0400, Tom Lane  wrote in 
>> <6234.1499801...@sss.pgh.pa.us>
>> > Amit Langote  writes:
>> > > Horiguchi-san,
>> > > On 2017/07/11 10:28, Kyotaro HORIGUCHI wrote:
>> > >> I faintly recall such discussion was held aroud that time and
>> > >> maybe we concluded that we don't do that but I haven't find such
>> > >> a thread in pgsql-hackers..
>> >
>> > > I mentioned it in my reply.  Here again:
>> > > https://www.postgresql.org/message-id/20160405.184408.166437663.horiguchi.kyotaro%40lab.ntt.co.jp
>> >
>> > The followup discussion noted that that approach was no good because it
>> > would only close connections in the same session that had done the ALTER
>> > SERVER.  I think the basic idea of marking postgres_fdw connections as
>> > needing to be remade when next possible is OK, but we have to drive it
>> > off catcache invalidation events, the same as we did in c52d37c8b.  An
>> > advantage of that way is we don't need any new hooks in the core code.
>> >
>> > Kyotaro-san, are you planning to update your old patch?
>>
>> I'm pleased to do that. I will reconsider the way shown in a mail
>> in the thread soon.
>
> done.
>
> (As a recap) Changing of some options such as host of a foreign
> server or password of a user mapping make the existing
> connections stale, but postgres_fdw continue using them.
>
> The attached patch does the following things.
>
> - postgres_fdw registers two invalidation callbacks on loading.
>
> - On any change on a foreign server or a user mapping, the
>   callbacks mark the affected connection as 'invalid'
>
> - The invalidated connections will be once disconnected before
>   the next execution if no transaction exists.
>
> As the consequence, changes of options properly affects
> subsequent queries in the next transaction on any session. It
> occurs after whatever option has been modifed.
>
> ==
> create server sv1 foreign data wrapper postgres_fdw options (host '/tmp', 
> port '5432', dbname 'postgres');
> create user mapping for public server sv1;
> create table t (a int);
> create foreign table ft1 (a int) server sv1 options (table_name 't1');
> begin;
> select * from ft1; -- no error
> alter server sv1 options (set host '/tmpe');
> select * from ft1; -- no error because transaction is living.
> commit;
> select * from ft1;
> ERROR:  could not connect to server "sv1"  - NEW
> ==
>
> This patch is postgres_fdw-private but it's annoying that hash
> value of syscache is handled in connection.c. If we allow to add
> something to the core for this feature, I could add a new member
> in FdwRoutine to notify invalidation of mapping and server by
> oid. (Of course it is not back-patcheable, though)
>
> Does anyone have opinitons or suggestions?
>

The patch and the idea looks good to me. I haven't reviewed it
thoroughly though.

I noticed a type "suporious", I think you meant "spurious"? Probably
that comment should be part of the function which marks the connection
as invalid e.g. InvalidateConnectionForMapping().

pgfdw_xact_callback() reports the reason for disconnection while
closing a connection. May be we want to report the reason for
disconnection here as well. Also, may be we want to create a function
to discard connection from an entry so that we consistently do the
same things while discarding a connection.

-- 
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

[HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

2017-07-13 Thread Kyotaro HORIGUCHI 
Hello, moved to pgsql-hackers.

This is the revased and revised version of the previous patch.

At Thu, 13 Jul 2017 13:42:49 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20170713.134249.97825982.horiguchi.kyot...@lab.ntt.co.jp>
> At Tue, 11 Jul 2017 15:39:14 -0400, Tom Lane  wrote in 
> <6234.1499801...@sss.pgh.pa.us>
> > Amit Langote  writes:
> > > Horiguchi-san,
> > > On 2017/07/11 10:28, Kyotaro HORIGUCHI wrote:
> > >> I faintly recall such discussion was held aroud that time and
> > >> maybe we concluded that we don't do that but I haven't find such
> > >> a thread in pgsql-hackers..
> > 
> > > I mentioned it in my reply.  Here again:
> > > https://www.postgresql.org/message-id/20160405.184408.166437663.horiguchi.kyotaro%40lab.ntt.co.jp
> > 
> > The followup discussion noted that that approach was no good because it
> > would only close connections in the same session that had done the ALTER
> > SERVER.  I think the basic idea of marking postgres_fdw connections as
> > needing to be remade when next possible is OK, but we have to drive it
> > off catcache invalidation events, the same as we did in c52d37c8b.  An
> > advantage of that way is we don't need any new hooks in the core code.
> > 
> > Kyotaro-san, are you planning to update your old patch?
> 
> I'm pleased to do that. I will reconsider the way shown in a mail
> in the thread soon.

done.

(As a recap) Changing of some options such as host of a foreign
server or password of a user mapping make the existing
connections stale, but postgres_fdw continue using them.

The attached patch does the following things.

- postgres_fdw registers two invalidation callbacks on loading.

- On any change on a foreign server or a user mapping, the
  callbacks mark the affected connection as 'invalid'

- The invalidated connections will be once disconnected before
  the next execution if no transaction exists.

As the consequence, changes of options properly affects
subsequent queries in the next transaction on any session. It
occurs after whatever option has been modifed.

==
create server sv1 foreign data wrapper postgres_fdw options (host '/tmp', port 
'5432', dbname 'postgres');
create user mapping for public server sv1;
create table t (a int);
create foreign table ft1 (a int) server sv1 options (table_name 't1');
begin;
select * from ft1; -- no error
alter server sv1 options (set host '/tmpe');
select * from ft1; -- no error because transaction is living.
commit;
select * from ft1;
ERROR:  could not connect to server "sv1"  - NEW
==

This patch is postgres_fdw-private but it's annoying that hash
value of syscache is handled in connection.c. If we allow to add
something to the core for this feature, I could add a new member
in FdwRoutine to notify invalidation of mapping and server by
oid. (Of course it is not back-patcheable, though)

Does anyone have opinitons or suggestions?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
*** a/contrib/postgres_fdw/connection.c
--- b/contrib/postgres_fdw/connection.c
***
*** 53,58  typedef struct ConnCacheEntry
--- 53,61 
  	bool		have_prep_stmt; /* have we prepared any stmts in this xact? */
  	bool		have_error;		/* have any subxacts aborted in this xact? */
  	bool		changing_xact_state;	/* xact state change in process */
+ 	bool		invalidated;	/* true if reconnect is requried */
+ 	uint32		server_hashvalue;	/* hash value of foreign server oid */
+ 	uint32		mapping_hashvalue;  /* hash value of user mapping oid */
  } ConnCacheEntry;
  
  /*
***
*** 150,160  GetConnection(UserMapping *user, bool will_prep_stmt)
--- 153,180 
  		entry->have_prep_stmt = false;
  		entry->have_error = false;
  		entry->changing_xact_state = false;
+ 		entry->invalidated = false;
+ 		entry->server_hashvalue = 0;
+ 		entry->mapping_hashvalue = 0;
  	}
  
  	/* Reject further use of connections which failed abort cleanup. */
  	pgfdw_reject_incomplete_xact_state_change(entry);
  
+ 
+ 	/*
+ 	 * This connection is no longer valid. Disconnect such connections if no
+ 	 * transaction is running. We could avoid suporious disconnection by
+ 	 * examining individual option values but it would be too-much for the
+ 	 * gain.
+ 	 */
+ 	if (entry->conn != NULL && entry->invalidated && entry->xact_depth == 0)
+ 	{
+ 		PQfinish(entry->conn);
+ 		entry->conn = NULL;
+ 		entry->invalidated = false;
+ 	}
+ 
  	/*
  	 * We don't check the health of cached connection here, because it would
  	 * require some overhead.  Broken connection will be detected when the
***
*** 173,178  GetConnection(UserMapping *user, bool will_prep_stmt)
--- 193,202 
  		entry->xact_depth = 0;	/* just to be sure */
  		entry->have_prep_stmt = false;
  		entry->have_error = false;
+ 		entry->server_hashvalue =
+ 			GetSysCacheHashValue1(FOREIGNSERVEROID, server->serverid);
+ 		entry->mapping_hashvalue