Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING
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
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
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
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
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
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
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, &key, HASH_ENTER, &found); 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
Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING
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
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, &key, HASH_ENTER, &found); 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, &key, HASH_ENTER, &found); 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 connection %p for opt
Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING
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 +{ + my ($self) = @_; + + # Create anonymous scalar references to be p
Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING
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, &key, HASH_ENTER, &found); if (!found) { ! /* initi
Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING
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
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 > >> 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> >> > 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 > > Right, it is too bad typo, but fixed it as "unnecessary", which > would more appropriate here. > >> that comment should be part of the function which marks the connection >> as invalid e.g. InvalidateConnectionForMapping(). > > Agreed. It'd been there but somehow I moved it to there. I have > moved it back to the place it used to be. > >> 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 > > Agreed. Also, I had placed LOG message there but removedxs. Now it > emits a DEBUG3 message as shown below. > > | DEBUG: closing connection 0 for option changes to take effect > | DEBUG: new postgres_fdw connection 0 for server ".." (user mapping oid > >> to discard connection from an entry so that we consistently do the >> same things while discarding a connection. > > Sure. Now there's two places a connection is closed intentionally. Thanks. Can you please add this to the next CF. I don't think we will be able to accept this change in v10. -- 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
Thank you for the comments. At Thu, 13 Jul 2017 16:54:42 +0530, Ashutosh Bapat wrote in > 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> > > 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 Right, it is too bad typo, but fixed it as "unnecessary", which would more appropriate here. > that comment should be part of the function which marks the connection > as invalid e.g. InvalidateConnectionForMapping(). Agreed. It'd been there but somehow I moved it to there. I have moved it back to the place it used to be. > 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 Agreed. Also, I had placed LOG message there but removedxs. Now it emits a DEBUG3 message as shown below. | DEBUG: closing connection 0 for option changes to take effect | DEBUG: new postgres_fdw connection 0 for server ".." (user mapping oid > to discard connection from an entry so that we consistently do the > same things while discarding a connection. Sure. Now there's two places a connection is closed intentionally. I'm a bit uneasy that many menbers of entry is getting reset in so many places. Since the validity of an entry is checked only by conn so it is enough to clear the flags of ConnCacheEntry only at the time of connection creation. Instead, pgfdw_reject_incomplete_xact_state_chanbe is no longer complains on an inactive (conn == NULL) entry. I think this is safe but a bit inconfident.. 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; /* *** *** 69,74 static bool xact_got_connection = false; --- 72,78 /* 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); *** *** 144,160 GetConnection(UserMapping *user, bool will_prep_stmt) entry = hash_search(ConnectionHash, &key, HASH_ENTER, &found); 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 --- 148,176 entry = hash_search(ConnectionHash, &key, HASH_ENTER, &found); if (!found) { ! /* ! * key is already filled in, flags well be initialized at the time of ! * making a new connection, so just clear conn here. ! */ entry->conn = NULL; } /* 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. + */ + if (entry->conn != NULL && entry->invalidated && entry->xact_depth == 0) + { + /* reconneced immediately, so the messages is "reconnecting" */ + elog(DEBUG3, "closing connection %p for option changes to take effect", + entry->conn); + disconnect_pg_server(entry); + } + /* * We don't check the health of cached connection here
Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING
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
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 = + GetSysCacheHashValue1(USERMAPPINGOID, user->umid); entry->conn = connect_p