On Tue, Jan 4, 2022 at 10:21 PM Shinya Kato <shinya11.k...@oss.nttdata.com> wrote:
> Thank you for the new patch! > > On 2021-12-15 15:40, kuroda.hay...@fujitsu.com wrote: > > Dear Kato-san, > > > > Thank you for giving comments! And sorry for late reply. > > I rebased my patches. > > > >> Even for local-only transaction, I thought it useless to execute > >> CallCheckingRemoteServersCallbacks() and enable_timeout_after(). Can I > >> make it so that it determines outside whether it contains SQL to the > >> remote or not? > > > > Yeah, remote-checking timeout will be enable even ifa local > > transaction is opened. > > In my understanding, postgres cannot distinguish whether opening > > transactions > > are using only local object or not. > > My first idea was that turning on the timeout when GetFdwRoutineXXX > > functions > > were called, but in some cases FDWs may not use remote connection even > > if > > they call such a handler function. e.g. postgresExplainForeignScan(). > > Hence I tried another approach that unregister all checking callback > > the end of each transactions. Only FDWs can notice that transactions > > are remote or local, > > so they must register callback when they really connect to other > > database. > > This implementation will avoid calling remote checking in the case of > > local transaction, > > but multiple registering and unregistering may lead another overhead. > > I attached which implements that. > > > It certainly incurs another overhead, but I think it's better than the > previous one. > So far, I haven't encountered any problems, but I'd like other people's > opinions. > > >> The following points are minor. > >> 1. In foreign.c, some of the lines are too long and should be broken. > >> 2. In pqcomm.c, the newline have been removed, what is the intention > >> of > >> this? > >> 3. In postgres.c, > >> 3-1. how about inserting a comment between lines 2713 and 2714, > >> similar > >> to line 2707? > >> 3-2. the line breaks in line 3242 and line 3243 should be aligned. > >> 3-3. you should change > >> /* > >> * Skip checking foreign servers while reading > >> messages. > >> */ > >> to > >> /* > >> * Skip checking foreign servers while reading > >> messages. > >> */ > >> 4. In connection.c, There is a typo in line 1684, so "fucntion" should > >> be changed to "function". > > > > Maybe all of them were fixed. Thanks! > Thank you, and it looks good to me. > > > Hi, + UnregisterAllCheckingRemoteServersCallback(); UnregisterAllCheckingRemoteServersCallback -> UnregisterAllCheckingRemoteServersCallbacks + CheckingRemoteServersCallbackItem *item; + item = fdw_callbacks; The above two lines can be combined. +UnregisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback, + void *arg) Is the above func called anywhere ? + if (item->callback == callback && item->arg == arg) Is comparing argument pointers stable ? What if the same argument is cloned ? + CallCheckingRemoteServersCallbacks(); + + if (remote_servers_connection_check_interval > 0) + enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT, + remote_servers_connection_check_interval); Should the call to CallCheckingRemoteServersCallbacks() be placed under the if block checking remote_servers_connection_check_interval ? If remote_servers_connection_check_interval is 0, it seems the callbacks shouldn't be called. Cheers
