pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Fix another instance of unsafe coding for shm_toc_lookup failure
5+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2018-02-05 05d0f13f07 Skip setting up shared instrumentation for Hash node if not needed.
* pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure
@ 2018-02-02 23:32 Tom Lane <tgl@sss.pgh.pa.us>
2018-02-03 01:59 ` Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure Thomas Munro <thomas.munro@enterprisedb.com>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2018-02-02 23:32 UTC (permalink / raw)
To: pgsql-committers
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.
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/d59ff4ab3111f20054425d82dab1393101dcfe8e
Modified Files
--------------
src/backend/executor/nodeHash.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure
2018-02-02 23:32 pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure Tom Lane <tgl@sss.pgh.pa.us>
@ 2018-02-03 01:59 ` Thomas Munro <thomas.munro@enterprisedb.com>
2018-02-03 02:08 ` Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Munro @ 2018-02-03 01:59 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers
On Sat, Feb 3, 2018 at 12:32 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure
2018-02-02 23:32 pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure Tom Lane <tgl@sss.pgh.pa.us>
2018-02-03 01:59 ` Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure Thomas Munro <thomas.munro@enterprisedb.com>
@ 2018-02-03 02:08 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-02-05 02:47 ` Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure Thomas Munro <thomas.munro@enterprisedb.com>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2018-02-03 02:08 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@enterprisedb.com>; +Cc: pgsql-committers
Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Sat, Feb 3, 2018 at 12:32 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure
2018-02-02 23:32 pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure Tom Lane <tgl@sss.pgh.pa.us>
2018-02-03 01:59 ` Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure Thomas Munro <thomas.munro@enterprisedb.com>
2018-02-03 02:08 ` Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure Tom Lane <tgl@sss.pgh.pa.us>
@ 2018-02-05 02:47 ` Thomas Munro <thomas.munro@enterprisedb.com>
2018-02-05 03:15 ` Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Munro @ 2018-02-05 02:47 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers
On Sat, Feb 3, 2018 at 3:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> 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
Attachments:
skip-hash-instrumentation-if-not-needed.patch (application/octet-stream, 2.7K) download
From 9374b365f10d3ecb1250b580c1700f2415f9c4df Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@enterprisedb.com>
Date: Mon, 5 Feb 2018 13:50:45 +1300
Subject: [PATCH] Skip setting up shared instrumentation for Hash node if not
needed.
We don't need to set up the shared space for hash join instrumentation data if
instrumentation hasn't been requested. Let's follow the example of the
similar Sort node code and save a few cycles by skipping that when we can.
Thomas Munro, following complaint from Tom Lane about a nearby bug
Discussion: https://postgr.es/m/E1ehkoZ-0005uW-43%40gemulon.postgresql.org
---
src/backend/executor/nodeHash.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index 70553b8fdf..b10f847452 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -2549,6 +2549,10 @@ ExecHashEstimate(HashState *node, ParallelContext *pcxt)
{
size_t size;
+ /* don't need this if not instrumenting or no workers */
+ if (!node->ps.instrument || pcxt->nworkers == 0)
+ return;
+
size = mul_size(pcxt->nworkers, sizeof(HashInstrumentation));
size = add_size(size, offsetof(SharedHashInfo, hinstrument));
shm_toc_estimate_chunk(&pcxt->estimator, size);
@@ -2564,6 +2568,10 @@ ExecHashInitializeDSM(HashState *node, ParallelContext *pcxt)
{
size_t size;
+ /* don't need this if not instrumenting or no workers */
+ if (!node->ps.instrument || pcxt->nworkers == 0)
+ return;
+
size = offsetof(SharedHashInfo, hinstrument) +
pcxt->nworkers * sizeof(HashInstrumentation);
node->shared_info = (SharedHashInfo *) shm_toc_allocate(pcxt->toc, size);
@@ -2582,13 +2590,13 @@ ExecHashInitializeWorker(HashState *node, ParallelWorkerContext *pwcxt)
{
SharedHashInfo *shared_info;
- /* might not be there ... */
+ /* don't need this if not instrumenting */
+ if (!node->ps.instrument)
+ return;
+
shared_info = (SharedHashInfo *)
- shm_toc_lookup(pwcxt->toc, node->ps.plan->plan_node_id, true);
- if (shared_info)
- node->hinstrument = &shared_info->hinstrument[ParallelWorkerNumber];
- else
- node->hinstrument = NULL;
+ shm_toc_lookup(pwcxt->toc, node->ps.plan->plan_node_id, false);
+ node->hinstrument = &shared_info->hinstrument[ParallelWorkerNumber];
}
/*
@@ -2614,6 +2622,9 @@ ExecHashRetrieveInstrumentation(HashState *node)
SharedHashInfo *shared_info = node->shared_info;
size_t size;
+ if (shared_info == NULL)
+ return;
+
/* Replace node->shared_info with a copy in backend-local memory. */
size = offsetof(SharedHashInfo, hinstrument) +
shared_info->num_workers * sizeof(HashInstrumentation);
--
2.15.1
* Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure
2018-02-02 23:32 pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure Tom Lane <tgl@sss.pgh.pa.us>
2018-02-03 01:59 ` Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure Thomas Munro <thomas.munro@enterprisedb.com>
2018-02-03 02:08 ` Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure Tom Lane <tgl@sss.pgh.pa.us>
2018-02-05 02:47 ` Re: pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure Thomas Munro <thomas.munro@enterprisedb.com>
@ 2018-02-05 03:15 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 5+ messages in thread
From: Tom Lane @ 2018-02-05 03:15 UTC (permalink / raw)
To: Thomas Munro <thomas.munro@enterprisedb.com>; +Cc: pgsql-committers
Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Sat, Feb 3, 2018 at 3:08 PM, Tom Lane <tgl@sss.pgh.pa.us> 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
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2018-02-05 03:15 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2018-02-02 23:32 pgsql: Fix another instance of unsafe coding for shm_toc_lookup failure Tom Lane <tgl@sss.pgh.pa.us>
2018-02-03 01:59 ` Thomas Munro <thomas.munro@enterprisedb.com>
2018-02-03 02:08 ` Tom Lane <tgl@sss.pgh.pa.us>
2018-02-05 02:47 ` Thomas Munro <thomas.munro@enterprisedb.com>
2018-02-05 03:15 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox