On Thu, Jan 21, 2021 at 12:17 PM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
> On 2021/01/21 14:46, Bharath Rupireddy wrote:
> > On Thu, Jan 21, 2021 at 10:06 AM Fujii Masao
> > <masao.fujii@oss.nttdata.com> wrote:
> > > >> + if (entry->server_hashvalue == hashvalue &&
> >>>> + (entry->xact_depth > 0 || result))
> >>>> + {
> >>>> + hash_seq_term(&scan);
> >>>> + break;
> >>>>
> >>>> entry->server_hashvalue can be 0? If yes, since postgres_fdw_disconnect_all()
> >>>> specifies 0 as hashvalue, ISTM that the above condition can be true
> >>>> unexpectedly. Can we replace this condition with just "if (!all)"?
> >>>
> >>> I don't think so entry->server_hashvalue can be zero, because
> >>> GetSysCacheHashValue1/CatalogCacheComputeHashValue will not return 0
> >>> as hash value. I have not seen someone comparing hashvalue with an
> >>> expectation that it has 0 value, for instance see if (hashvalue == 0
> >>> || riinfo->oidHashValue == hashvalue).
> >>>
> >>> Having if(!all) something like below there doesn't suffice because we
> >>> might call hash_seq_term, when some connection other than the given
> >>> foreign server connection is in use.
> >>
> >> No because we check the following condition before reaching that code. No?
> >>
> >> + if ((all || entry->server_hashvalue == hashvalue) &&
> >>
> >>
> >> I was thinking that "(entry->xact_depth > 0 || result))" condition is not
> >> necessary because "result" is set to true when xact_depth <= 0 and that
> >> condition always indicates true.
> >
> > I think that condition is too confusing. How about having a boolean
> > can_terminate_scan like below?
>
> Thanks for thinking this. But at least for me, "if (!all)" looks not so confusing.
> And the comment seems to explain why we can end the scan.
May I know if it's okay to have the boolean can_terminate_scan as shown in [1]?
[1] -
https://www.postgresql.org/message-id/flat/CALj2ACVx0%2BiOsrAA-wXbo3RLAKqUoNvvEd7foJ0vLwOdu8XjXw%40mail.gmail.com
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
