BUG #14738: ALTER SERVER for foregin servers not working

fcs1@poczta.onet.pl writes:

PostgreSQL version: 9.4.1

Command like this doesn't take effect:
ALTER SERVER srw_egib_1
OPTIONS (
SET host 'localhost',
SET port '5432',
SET dbname 'gml2m1');
It changes definition of this server but tables connected to it are still
connected to previous definition of this server, for example:

It would help if you provided a concrete example of misbehavior rather
than abstract claims. However, I *think* this is something we fixed in
9.4.11. Please update and try again.

regards, tom lane

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

On 2017/07/11 1:18, Tom Lane wrote:

fcs1@poczta.onet.pl writes:

PostgreSQL version: 9.4.1

Command like this doesn't take effect:
ALTER SERVER srw_egib_1
OPTIONS (
SET host 'localhost',
SET port '5432',
SET dbname 'gml2m1');
It changes definition of this server but tables connected to it are still
connected to previous definition of this server, for example:

It would help if you provided a concrete example of misbehavior rather
than abstract claims. However, I *think* this is something we fixed in
9.4.11.

Perhaps you are referring to the following item fixed in 9.4.11 [1]https://www.postgresql.org/docs/devel/static/release-9-4-11.html:

"Ensure that cached plans are invalidated by changes in foreign-table options"

ISTM, OP's problem is unrelated. Steps to reproduce:

create extension postgres_fdw ;
create server s1 foreign data wrapper postgres_fdw options (dbname 'db1');
create server s2 foreign data wrapper postgres_fdw options (dbname 'db2');
create user mapping for current_user server s1;
create user mapping for current_user server s2;
create foreign table t1 (a int) server s1 options (table_name 't1');

-- in db1
create table t1 (a) as select 1;

-- in db2
create table t1 (a) as select 2;

-- back in the original database; t1's server s1 connected to db1
select * from t1;
a
---
1
(1 row)

-- make s1 point to db2
alter server s1 options (set dbname 'db2');

-- postgres_fdw will still connect to db1
select * from t1;
a
---
1
(1 row)

I think that's because postgres_fdw/connection.c keeps a cache of
connections and does not invalidate it upon pg_foreign_server and/or
pg_user_mapping changes. I think we discussed the possibility of fixing
this back when the above-mentioned fix was being worked on [2]/messages/by-id/20160405.184408.166437663.horiguchi.kyotaro@lab.ntt.co.jp, but it
went nowhere.

Thanks,
Amit

[1]: https://www.postgresql.org/docs/devel/static/release-9-4-11.html
[2]: /messages/by-id/20160405.184408.166437663.horiguchi.kyotaro@lab.ntt.co.jp
/messages/by-id/20160405.184408.166437663.horiguchi.kyotaro@lab.ntt.co.jp

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

At Mon, 10 Jul 2017 12:18:53 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <1838.1499703533@sss.pgh.pa.us>

fcs1@poczta.onet.pl writes:

PostgreSQL version: 9.4.1

Command like this doesn't take effect:
ALTER SERVER srw_egib_1
OPTIONS (
SET host 'localhost',
SET port '5432',
SET dbname 'gml2m1');
It changes definition of this server but tables connected to it are still
connected to previous definition of this server, for example:

It would help if you provided a concrete example of misbehavior rather
than abstract claims. However, I *think* this is something we fixed in
9.4.11. Please update and try again.

Since 9.4.11 plancache is invalidated by ALTER SERVER but the
master still doesn't disconnect existing connection by ALTER
SERVER like that.

create server sv1 foreign data wrapper postgres_fdw options (host '/tmp', port '5432', dbname 'postgres');
create user mapping for public server sv1;
create table t1 (a int);
create foreign table ft1 (a int) server sv1 options (table_name 't1');
insert into t1 values (1), (2), (3);
select * from ft1;
<returns 1, 2, 3>
alter server sv1 options (set host 'hoge');
insert into t1 values (4);
select * from ft1;
<returns 1, 2, 3, 4, still on the old connection>

\c -- reconnect backend
select * from ft1;
ERROR: could not connect to server "sv1"
DETAIL: could not translate host name "hoge" to address: Name or service not known

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

If we are to "fix" this, GetConnection() of postgres_fdw needs to
recheck foreign server options, or add an FDW interface to notify
such option changes.

regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center

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

Oops! I lost a race.

At Tue, 11 Jul 2017 10:23:05 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in <76f1487a-6b8a-61a9-ebd9-8ff047d0ba94@lab.ntt.co.jp>

On 2017/07/11 1:18, Tom Lane wrote:

fcs1@poczta.onet.pl writes:

PostgreSQL version: 9.4.1

Command like this doesn't take effect:
ALTER SERVER srw_egib_1
OPTIONS (
SET host 'localhost',
SET port '5432',
SET dbname 'gml2m1');
It changes definition of this server but tables connected to it are still
connected to previous definition of this server, for example:

It would help if you provided a concrete example of misbehavior rather
than abstract claims. However, I *think* this is something we fixed in
9.4.11.

Perhaps you are referring to the following item fixed in 9.4.11 [1]:

"Ensure that cached plans are invalidated by changes in foreign-table options"

ISTM, OP's problem is unrelated. Steps to reproduce:

Agreed.

create extension postgres_fdw ;
create server s1 foreign data wrapper postgres_fdw options (dbname 'db1');
create server s2 foreign data wrapper postgres_fdw options (dbname 'db2');
create user mapping for current_user server s1;
create user mapping for current_user server s2;
create foreign table t1 (a int) server s1 options (table_name 't1');

-- in db1
create table t1 (a) as select 1;

-- in db2
create table t1 (a) as select 2;

-- back in the original database; t1's server s1 connected to db1
select * from t1;
a
---
1
(1 row)

-- make s1 point to db2
alter server s1 options (set dbname 'db2');

-- postgres_fdw will still connect to db1
select * from t1;
a
---
1
(1 row)

I think that's because postgres_fdw/connection.c keeps a cache of
connections and does not invalidate it upon pg_foreign_server and/or
pg_user_mapping changes. I think we discussed the possibility of fixing
this back when the above-mentioned fix was being worked on [2], but it
went nowhere.

Thanks,
Amit

[1] https://www.postgresql.org/docs/devel/static/release-9-4-11.html
[2]
/messages/by-id/20160405.184408.166437663.horiguchi.kyotaro@lab.ntt.co.jp

Many thanks for digging out it, that have almost faded out of my
memory..

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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

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:

/messages/by-id/20160405.184408.166437663.horiguchi.kyotaro@lab.ntt.co.jp

Thanks,
Amit

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

Hello,

Thank you for confirmation this problem.

I'm waiting for a fix.

Best regards
Michal

W dniu 2017-07-11 o 03:38, Amit Langote pisze:

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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:
/messages/by-id/20160405.184408.166437663.horiguchi.kyotaro@lab.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?

regards, tom lane

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

At Tue, 11 Jul 2017 15:39:14 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <6234.1499801954@sss.pgh.pa.us>

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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:
/messages/by-id/20160405.184408.166437663.horiguchi.kyotaro@lab.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.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

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

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 <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20170713.134249.97825982.horiguchi.kyotaro@lab.ntt.co.jp>

At Tue, 11 Jul 2017 15:39:14 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <6234.1499801954@sss.pgh.pa.us>

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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:
/messages/by-id/20160405.184408.166437663.horiguchi.kyotaro@lab.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

On Thu, Jul 13, 2017 at 2:53 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> 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 <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20170713.134249.97825982.horiguchi.kyotaro@lab.ntt.co.jp>

At Tue, 11 Jul 2017 15:39:14 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <6234.1499801954@sss.pgh.pa.us>

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> 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:
/messages/by-id/20160405.184408.166437663.horiguchi.kyotaro@lab.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

Thank you for the comments.

At Thu, 13 Jul 2017 16:54:42 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in <CAFjFpRd0yz3v0rL2yxmr95e_iDntkeQia9709KXaHLyVcZ=_mQ@mail.gmail.com>

On Thu, Jul 13, 2017 at 2:53 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> 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 <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20170713.134249.97825982.horiguchi.kyotaro@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 0xxxx for option changes to take effect
| DEBUG: new postgres_fdw connection 0xxxx 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

On Fri, Jul 14, 2017 at 2:04 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

Thank you for the comments.

At Thu, 13 Jul 2017 16:54:42 +0530, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote in <CAFjFpRd0yz3v0rL2yxmr95e_iDntkeQia9709KXaHLyVcZ=_mQ@mail.gmail.com>

On Thu, Jul 13, 2017 at 2:53 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> 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 <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20170713.134249.97825982.horiguchi.kyotaro@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 0xxxx for option changes to take effect
| DEBUG: new postgres_fdw connection 0xxxx 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

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> 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

Thank you for the comments.

At Mon, 17 Jul 2017 16:09:04 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <9897.1500322144@sss.pgh.pa.us>

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> 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

Finally, I added new TAP test library PsqlSession.

At Tue, 18 Jul 2017 18:12:13 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20170718.181213.206979369.horiguchi.kyotaro@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

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> 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

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 Herrera https://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

At Thu, 20 Jul 2017 18:15:42 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <18927.1500588942@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

At Thu, 20 Jul 2017 18:23:05 -0400, Alvaro Herrera <alvherre@2ndquadrant.com> 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