From: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
---|---|
To: | Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> |
Cc: | Zhihong Yu <zyu(at)yugabyte(dot)com>, Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>, "Hou, Zhijie" <houzj(dot)fnst(at)cn(dot)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit |
Date: | 2021-01-21 07:16:44 |
Message-ID: | CALj2ACUVGO=KL2meDins3Mhseus4Hpke-TiiaZvVDJkFmbo6hg@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, Jan 21, 2021 at 12:17 PM Fujii Masao
<masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> On 2021/01/21 14:46, Bharath Rupireddy wrote:
> > On Thu, Jan 21, 2021 at 10:06 AM Fujii Masao
> > <masao(dot)fujii(at)oss(dot)nttdata(dot)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]?
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From | Date | Subject | |
---|---|---|---|
Next Message | Greg Nancarrow | 2021-01-21 07:34:20 | Re: Parallel INSERT (INTO ... SELECT ...) |
Previous Message | Greg Nancarrow | 2021-01-21 07:14:14 | Re: Parallel INSERT (INTO ... SELECT ...) |