Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit

On 2020/12/11 19:16, Bharath Rupireddy wrote:
> On Thu, Dec 10, 2020 at 7:14 AM Bharath Rupireddy
> <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>>> + /* We only look for active and open remote connections. */
>>> + if (entry->invalidated || !entry->conn)
>>> + continue;
>>>
>>> We should return even invalidated entry because it has still cached connection?
>>>
>>
>> I checked this point earlier, for invalidated connections, the tuple
>> returned from the cache is also invalid and the following error will
>> be thrown. So, we can not get the server name for that user mapping.
>> Cache entries too would have been invalidated after the connection is
>> marked as invalid in pgfdw_inval_callback().
>>
>> umaptup = SearchSysCache1(USERMAPPINGOID, ObjectIdGetDatum(entry->key));
>> if (!HeapTupleIsValid(umaptup))
>> elog(ERROR, "cache lookup failed for user mapping with OID %u",
>> entry->key);
>>
>
> I further checked on returning invalidated connections in the output
> of the function. Actually, the reason I'm seeing a null tuple from sys
> cache (and hence the error "cache lookup failed for user mapping with
> OID xxxx") for an invalidated connection is that the user mapping
> (with OID entry->key that exists in the cache) is getting dropped, so
> the sys cache returns null tuple. The use case is as follows:
>
> 1) Create a server, role, and user mapping of the role with the server
> 2) Run a foreign table query, so that the connection related to the
> server gets cached
> 3) Issue DROP OWNED BY for the role created, since the user mapping is
> dependent on that role, it gets dropped from the catalogue table and
> an invalidation message will be pending to clear the sys cache
> associated with that user mapping.
> 4) Now, if I do select * from postgres_fdw_get_connections() or for
> that matter any query, at the beginning the txn
> AtStart_Cache()-->AcceptInvalidationMessages()-->pgfdw_inval_callback()
> gets called and marks the cached entry as invalidated. Remember the
> reason for this invalidation message is that the user mapping with the
> OID entry->key is dropped from 3). Later in
> postgres_fdw_get_connections(), when we search the sys cache with
> entry->key for that invalidated connection, since the user mapping is
> dropped from the system, null tuple is returned.

Thanks for the analysis! This means that the cached connection invalidated by drop of server or user mapping will not be closed even by the subsequent access to the foreign server and will remain until the backend exits. Right? If so, this seems like a connection-leak bug, at least for me.... Thought?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION