Re: satisfies_hash_partition crash

Hi Tender,

On Thu, Jul 2, 2026 at 12:47 PM Tender Wang <tndrwang(at)gmail(dot)com> wrote:
>
> Ewan Young <kdbase(dot)hack(at)gmail(dot)com> 于2026年7月2日周四 10:08写道：
> > I tried the PG_ARGISNULL(3) approach and ran into one subtlety I
> > thought worth mentioning:
> > it seems like the check can't go into the top-level if()
> > unconditionally. In a non-variadic call, a NULL
> > fourth argument is a valid NULL value for the first partition key, and
> > it looks like satisfies_hash_partition()
> > needs to keep returning the normal result there — that's how the
> > hash-partition CHECK constraint routes
> > rows with NULL key columns. Making it an unconditional false seems to
> > make hash partitions reject NULL rows.
>
> Yes, you're right.
>
> > I've attached a patch with a small regression test in case any of it
> > is useful, but I'm happy to leave the actual fix
> > to you.
> How about this:
> diff --git a/src/backend/partitioning/partbounds.c
> b/src/backend/partitioning/partbounds.c
> index 6fb150a8763..53c409cf2b1 100644
> --- a/src/backend/partitioning/partbounds.c
> +++ b/src/backend/partitioning/partbounds.c
> @@ -4863,7 +4863,13 @@ satisfies_hash_partition(PG_FUNCTION_ARGS)
> }
> else
> {
> - ArrayType *variadic_array = PG_GETARG_ARRAYTYPE_P(3);
> + ArrayType *variadic_array;
> +
> + /* Return false if the array is NULL. */
> + if (PG_ARGISNULL(3))
> + PG_RETURN_BOOL(false);
> +
> + variadic_array = PG_GETARG_ARRAYTYPE_P(3);
>
> /* allocate space for our cache -- just one
> FmgrInfo in this case */
> fcinfo->flinfo->fn_extra =
>
> This way can avoid two get_fn_expr_variadic() calls if the last
> argument is not combined into an array.
> Thoughts?

Thanks for taking a look, and for confirming the non-variadic point.

I think that placement doesn't quite cover it, though. There are two
PG_GETARG_ARRAYTYPE_P(3) calls:
the one in the cache-setup branch you patched (reached only on the
first call, or when the parent OID changes),
and a second one in the deconstruct_array() path that runs on every
call. Because fn_extra is cached across calls,
a NULL array can skip the cache-setup block and reach that second
fetch unguarded — so it still crashes:

create table hp (a int) partition by hash (a);
create table hp0 partition of hp for values with (modulus 2, remainder 0);
create table hp1 partition of hp for values with (modulus 2, remainder 1);
create table t (arr int[]);
insert into t values (array[1]), (null), (array[2]);
select satisfies_hash_partition('hp'::regclass, 2, 0, variadic arr) from t;

The first (non-NULL) row builds the cache, and the NULL row then
crashes in the deconstruct path.
With the top-level check it returns t/f/t. (The cache-setup placement
also returns between relation_open()
and relation_close(), so it trips "resource was not closed" even for
the single-NULL case.)

You're right that the top-level version calls get_fn_expr_variadic()
once more in the non-variadic path.
It's just an O(1) read of the funcvariadic flag, so I left it as is —
but if you'd rather not repeat it, we could
compute it once into a local near the top and reuse it. Either way the
NULL-array guard needs to sit ahead
of both array fetches.

>
>
> --
> Thanks,
> Tender Wang

--
Regards,
Ewan Young