Re: PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING

On Thu, Jul 13, 2017 at 2:53 PM, Kyotaro HORIGUCHI
<horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)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(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <20170713(dot)134249(dot)97825982(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
>> At Tue, 11 Jul 2017 15:39:14 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote in <6234(dot)1499801954(at)sss(dot)pgh(dot)pa(dot)us>
>> > Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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:
>> > > 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