Re: Is RecoveryConflictInterrupt() entirely safe in a signal handler?

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