On 2016/04/05 18:44, Kyotaro HORIGUCHI wrote: > At Tue, 5 Apr 2016 14:24:52 +0900, Amit Langote wrote: >> On 2016/04/05 0:23, Tom Lane wrote: >>> Amit Langote <amitlangot...@gmail.com> writes: >>>> Hm, some kind of PlanInvalItem-based solution could work maybe? >>> >>> Hm, so we'd expect that whenever an FDW consulted the options while >>> making a plan, it'd have to record a plan dependency on them? That >>> would be a clean fix maybe, but I'm worried that third-party FDWs >>> would fail to get the word about needing to do this. >> >> I would imagine that that level of granularity may be a little too much; I >> mean tracking dependencies at the level of individual FDW/foreign >> table/foreign server options. I think it should really be possible to do >> the entire thing in core instead of requiring this to be made a concern of >> FDW authors. How about the attached that teaches >> extract_query_dependencies() to add a foreign table and associated foreign >> data wrapper and foreign server to invalItems. Also, it adds plan cache >> callbacks for respective caches. > > It seems to work but some renaming of functions would needed.
Yeah, I felt the names were getting quite long, too :) >> One thing that I observed that may not be all that surprising is that we >> may need a similar mechanism for postgres_fdw's connection cache, which >> doesn't drop connections using older server connection info after I alter >> them. I was trying to test my patch by altering dbaname option of a >> foreign server but that was silly, ;). Although, I did confirm that the >> patch works by altering use_remote_estimates server option. I could not >> really test for FDW options though. >> >> Thoughts? > > It seems a issue of FDW drivers but notification can be issued by > the core. The attached ultra-POC patch does it. > > > Disconnecting of a fdw connection with active transaction causes > an unexpected rollback on the remote side. So disconnection > should be allowed only when xact_depth == 0 in > pgfdw_subxact_callback(). > > There are so many parameters that affect connections, which are > listed as PQconninfoOptions. It is a bit too complex to detect > changes of one or some of them. So, I try to use object access > hook even though using hook is not proper as fdw interface. An > additional interface would be needed to do this anyway. > > With this patch, making any change on foreign servers or user > mappings corresponding to exiting connection causes > disconnection. This could be moved in to core, with the following > API like this. > > reoutine->NotifyObjectModification(ObjectAccessType access, > Oid classId, Oid objId); > > - using object access hook (which is put by the core itself) is not bad? > > - If ok, the API above looks bad. Any better API? Although helps achieve our goal here, object access hook stuff seems to be targeted at different users: /* * Hook on object accesses. This is intended as infrastructure for security * and logging plugins. */ object_access_hook_type object_access_hook = NULL; I just noticed the following comment above GetConnection(): * * XXX Note that caching connections theoretically requires a mechanism to * detect change of FDW objects to invalidate already established connections. * We could manage that by watching for invalidation events on the relevant * syscaches. For the moment, though, it's not clear that this would really * be useful and not mere pedantry. We could not flush any active connections * mid-transaction anyway. So, the hazard of flushing the connection mid-transaction sounds like it may be a show-stopper here, even if we research some approach based on syscache invalidation. Although I see that your patch takes care of it. > By the way, AlterUserMapping seems missing calling > InvokeObjectPostAlterHook. Is this a bug? Probably, yes. Thanks, Amit -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers