Re: Is RecoveryConflictInterrupt() entirely safe in a signal handler? - Mailing list pgsql-hackers

Hi,

On 2023-01-04 16:46:05 +1300, Thomas Munro wrote:
> postgres=# select 'x' ~ 'hello world .*';
> -[ RECORD 1 ]
> ?column? | f
> 
> postgres=# select * from pg_backend_memory_contexts where name =
> 'RegexpMemoryContext';
> -[ RECORD 1 ]-+-------------------------
> name          | RegexpMemoryContext
> ident         | hello world .*
> parent        | RegexpCacheMemoryContext
> level         | 2
> total_bytes   | 13376
> total_nblocks | 5

Hm, if a trivial re uses 13kB, using ALLOCSET_SMALL_SIZES might actually
increase memory usage by increasing the number of blocks.


> free_bytes    | 5144
> free_chunks   | 8
> used_bytes    | 8232

Hm. So we actually have a bunch of temporary allocations in here. I assume
that's all the stuff from the "non-compact" representation that
src/backend/regex/README talks about?

I doesn't immedialy look trivial to use a separate memory context for the
"final" representation and scratch memory though.


> There's some more memory allocated in regc_pg_locale.c with raw
> malloc() that could probably benefit from a pallocisation just to be
> able to measure it, but I didn't touch that here.

It might also effectively reduce the overhead of using palloc, by filling the
context up further.



> diff --git a/src/backend/regex/regcomp.c b/src/backend/regex/regcomp.c
> index bb8c240598..c0f8e77b49 100644
> --- a/src/backend/regex/regcomp.c
> +++ b/src/backend/regex/regcomp.c
> @@ -2471,17 +2471,17 @@ rfree(regex_t *re)
>  /*
>   * rcancelrequested - check for external request to cancel regex operation
>   *
> - * Return nonzero to fail the operation with error code REG_CANCEL,
> - * zero to keep going
> - *
> - * The current implementation is Postgres-specific.  If we ever get around
> - * to splitting the regex code out as a standalone library, there will need
> - * to be some API to let applications define a callback function for this.
> + * The current implementation always returns 0, if CHECK_FOR_INTERRUPTS()
> + * doesn't exit non-locally via ereport().  Memory allocated while compiling is
> + * expected to be cleaned up by virtue of being allocated using palloc in a
> + * suitable memory context.
>   */
>  static int
>  rcancelrequested(void)
>  {
> -    return InterruptPending && (QueryCancelPending || ProcDiePending);
> +    CHECK_FOR_INTERRUPTS();
> +
> +    return 0;
>  }

Hm. Seems confusing for this to continue being called rcancelrequested() and
to be called via if(CANCEL_REQUESTED()), if we're not even documenting that
it's intended to be usable that way?

Seems at the minimum we ought to keep more of the old comment, to explain the
somewhat odd API?


> +    /* Set up the cache memory on first go through. */
> +    if (unlikely(RegexpCacheMemoryContext == NULL))
> +        RegexpCacheMemoryContext =
> +            AllocSetContextCreate(TopMemoryContext,
> +                                  "RegexpCacheMemoryContext",
> +                                  ALLOCSET_SMALL_SIZES);

I think it might be nicer to create this below CacheMemoryContext? Just so the
"memory context tree" stays nicely ordered.

Greetings,

Andres Freund