Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure
Thomas Munro writes: > On Sat, Feb 3, 2018 at 3:08 PM, Tom Lane wrote: >> OK. Please send in a patch to either do that or switch this call to use >> noError = false. Or possibly both: shouldn't there be some other signal >> path that tells the worker whether instrumentation is needed? I'll >> leave it alone pending your investigation. > Here's a patch do to both. LGTM, pushed. regards, tom lane
Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure
On Sat, Feb 3, 2018 at 3:08 PM, Tom Lane wrote: > Thomas Munro writes: >> However, it's not surprising that you drew the >> opposite conclusion (ie that it might in fact not be in the TOC), >> since the shm space is really only necessary for EXPLAIN ANALYZE. >> Perhaps I should make it skip setting up this shm stuff if >> !node->ss.ps.instrument, just like the equivalent Sort node code. I >> will look into that on Monday. > > OK. Please send in a patch to either do that or switch this call to use > noError = false. Or possibly both: shouldn't there be some other signal > path that tells the worker whether instrumentation is needed? I'll > leave it alone pending your investigation. Here's a patch do to both. -- Thomas Munro http://www.enterprisedb.com skip-hash-instrumentation-if-not-needed.patch Description: Binary data
Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure
Thomas Munro writes: > On Sat, Feb 3, 2018 at 12:32 PM, Tom Lane wrote: >> Fix another instance of unsafe coding for shm_toc_lookup failure. > my mistake was actually to put noError = true there when noError = > false was called for. Ah. > However, it's not surprising that you drew the > opposite conclusion (ie that it might in fact not be in the TOC), > since the shm space is really only necessary for EXPLAIN ANALYZE. > Perhaps I should make it skip setting up this shm stuff if > !node->ss.ps.instrument, just like the equivalent Sort node code. I > will look into that on Monday. OK. Please send in a patch to either do that or switch this call to use noError = false. Or possibly both: shouldn't there be some other signal path that tells the worker whether instrumentation is needed? I'll leave it alone pending your investigation. regards, tom lane
Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure
On Sat, Feb 3, 2018 at 12:32 PM, Tom Lane wrote: > Fix another instance of unsafe coding for shm_toc_lookup failure. > > One or another author of commit 5bcf389ec seems to have thought that > computing an offset from a NULL pointer would yield another NULL pointer. > There may possibly be architectures where that works, but common machines > don't work like that. Per a quick code review of places calling > shm_toc_lookup and not using noError = false. Hmm... That was me. FWIW I certainly didn't think that about pointer arithmetic... I think I must have got confused about the sense of that flag. ExecHashInitializeWorker() should always find a TOC entry using plan_node_id, because ExecHashInitializeDSM() always inserts one, so my mistake was actually to put noError = true there when noError = false was called for. However, it's not surprising that you drew the opposite conclusion (ie that it might in fact not be in the TOC), since the shm space is really only necessary for EXPLAIN ANALYZE. Perhaps I should make it skip setting up this shm stuff if !node->ss.ps.instrument, just like the equivalent Sort node code. I will look into that on Monday. -- Thomas Munro http://www.enterprisedb.com
