Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. simplehash: preserve consistency in case of OOM.

  1. simplehash: preserve consistency in case of OOM

    Jeff Davis <pgsql@j-davis.com> — 2023-11-17T18:42:54Z

    Right now, if allocation fails while growing a hashtable, it's left in
an inconsistent state and can't be used again.

Patch attached.


-- 
Jeff Davis
PostgreSQL Contributor Team - AWS

  2. Re: simplehash: preserve consistency in case of OOM

    Andres Freund <andres@anarazel.de> — 2023-11-17T20:13:34Z

    Hi,

On 2023-11-17 10:42:54 -0800, Jeff Davis wrote:
> Right now, if allocation fails while growing a hashtable, it's left in
> an inconsistent state and can't be used again.

I'm not against allowing this - but I am curious, in which use cases is this
useful?


> @@ -446,10 +459,11 @@ SH_CREATE(MemoryContext ctx, uint32 nelements, void *private_data)
>  	/* increase nelements by fillfactor, want to store nelements elements */
>  	size = Min((double) SH_MAX_SIZE, ((double) nelements) / SH_FILLFACTOR);
>  
> -	SH_COMPUTE_PARAMETERS(tb, size);
> +	size = SH_COMPUTE_SIZE(size);
>  
> -	tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * tb->size);
> +	tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * size);
>  
> +	SH_UPDATE_PARAMETERS(tb, size);
>  	return tb;
>  }

Maybe add a comment explaining why it's important to update parameters after
allocating?

Greetings,

Andres Freund

  3. Re: simplehash: preserve consistency in case of OOM

    Jeff Davis <pgsql@j-davis.com> — 2023-11-17T21:00:19Z

    On Fri, 2023-11-17 at 12:13 -0800, Andres Freund wrote:
> On 2023-11-17 10:42:54 -0800, Jeff Davis wrote:
> > Right now, if allocation fails while growing a hashtable, it's left
> > in
> > an inconsistent state and can't be used again.
> 
> I'm not against allowing this - but I am curious, in which use cases
> is this
> useful?

I committed a cache for search_path (f26c2368dc), and afterwards got
concerned that I missed some potential OOM hazards. I separately posted
a patch to fix those (mostly by simplifying things, which in hindsight
was how it should have been done to begin with). Along the way, I also
noticed that simplehash itself is not safe in that case.

I don't think there are other bugs in the system due to simplehash and
OOM, because it's mainly used in the executor.

Please tell me if you think the use of simplehash for a search_path
cache is the wrong tool for the job.

> Maybe add a comment explaining why it's important to update
> parameters after
> allocating?

Will do.

Regards,
	Jeff Davis

  4. Re: simplehash: preserve consistency in case of OOM

    Gurjeet Singh <gurjeet@singh.im> — 2023-11-17T21:00:59Z

    On Fri, Nov 17, 2023 at 12:13 PM Andres Freund <andres@anarazel.de> wrote:
>
> On 2023-11-17 10:42:54 -0800, Jeff Davis wrote:
> > Right now, if allocation fails while growing a hashtable, it's left in
> > an inconsistent state and can't be used again.

+1 to the patch.

> I'm not against allowing this - but I am curious, in which use cases is this
> useful?

I don't have an answer to that, but a guess would be when the server
is dealing with memory pressure. In my view the patch has merit purely
on the grounds of increasing robustness.

> > @@ -446,10 +459,11 @@ SH_CREATE(MemoryContext ctx, uint32 nelements, void *private_data)
> >       /* increase nelements by fillfactor, want to store nelements elements */
> >       size = Min((double) SH_MAX_SIZE, ((double) nelements) / SH_FILLFACTOR);
> >
> > -     SH_COMPUTE_PARAMETERS(tb, size);
> > +     size = SH_COMPUTE_SIZE(size);
> >
> > -     tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * tb->size);
> > +     tb->data = (SH_ELEMENT_TYPE *) SH_ALLOCATE(tb, sizeof(SH_ELEMENT_TYPE) * size);
> >
> > +     SH_UPDATE_PARAMETERS(tb, size);
> >       return tb;
> >  }
>
> Maybe add a comment explaining why it's important to update parameters after
> allocating?

Perhaps something like this:

+     /*
+     * Update parameters _after_ allocation succeeds; prevent
+     * bogus/corrupted state.
+     */
+     SH_UPDATE_PARAMETERS(tb, size);

Best regards,
Gurjeet
http://Gurje.et

  5. Re: simplehash: preserve consistency in case of OOM

    Andres Freund <andres@anarazel.de> — 2023-11-17T21:22:31Z

    On 2023-11-17 13:00:19 -0800, Jeff Davis wrote:
> Please tell me if you think the use of simplehash for a search_path
> cache is the wrong tool for the job.

No, seems fine. I just was curious - as you said, the older existing users
won't ever care about this case.