Re: postgres_fdw : altering foreign table not invalidating prepare statement execution plan. - Mailing list pgsql-hackers

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 <amitlangote09@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
alreadyestablished connections.* We could manage that by watching for invalidation events on the relevant* syscaches.
Forthe moment, though, it's not clear that this would really* be useful and not mere pedantry.  We could not flush any
activeconnections* 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