Horton

Thread

Commits

  1. Fix multiple problems with satisfies_hash_partition.

  1. satisfies_hash_partition crash

    Robert Haas <robertmhaas@gmail.com> — 2026-07-01T19:11:17Z

    Hi,

This test case crashes the server for me:

CREATE TABLE hp (a int) PARTITION BY HASH (a);
SELECT satisfies_hash_partition('hp'::regclass, 1, 0, VARIADIC NULL::int[]);

I'm inclined to mostly blame my commit
f3b0897a1213f46b4d3a99a7f8ef3a4b32e03572, which fixed related problems
but overlooked this one. I think the fix should be simple, but since
that commit is from 2017, it will need to be back-patched all the way.

--
Robert Haas
EDB: http://www.enterprisedb.com

  2. Re: satisfies_hash_partition crash

    Tender Wang <tndrwang@gmail.com> — 2026-07-02T01:22:30Z

    Robert Haas <robertmhaas@gmail.com> 于2026年7月2日周四 03:11写道：
>
> Hi,
>
> This test case crashes the server for me:
>
> CREATE TABLE hp (a int) PARTITION BY HASH (a);
> SELECT satisfies_hash_partition('hp'::regclass, 1, 0, VARIADIC NULL::int[]);
>
> I'm inclined to mostly blame my commit
> f3b0897a1213f46b4d3a99a7f8ef3a4b32e03572, which fixed related problems
> but overlooked this one. I think the fix should be simple, but since
> that commit is from 2017, it will need to be back-patched all the way.

Adding PG_ARGISNULL(3) to the beginning of the if() seems workable.
And the comments need to be adjusted.


-- 
Thanks,
Tender Wang

  3. Re: satisfies_hash_partition crash

    Ewan Young <kdbase.hack@gmail.com> — 2026-07-02T02:08:33Z

    On Thu, Jul 2, 2026 at 9:22 AM Tender Wang <tndrwang@gmail.com> wrote:
>
> Robert Haas <robertmhaas@gmail.com> 于2026年7月2日周四 03:11写道：
> >
> > Hi,
> >
> > This test case crashes the server for me:
> >
> > CREATE TABLE hp (a int) PARTITION BY HASH (a);
> > SELECT satisfies_hash_partition('hp'::regclass, 1, 0, VARIADIC NULL::int[]);
> >
> > I'm inclined to mostly blame my commit
> > f3b0897a1213f46b4d3a99a7f8ef3a4b32e03572, which fixed related problems
> > but overlooked this one. I think the fix should be simple, but since
> > that commit is from 2017, it will need to be back-patched all the way.
>
> Adding PG_ARGISNULL(3) to the beginning of the if() seems workable.

Thanks for looking at this!

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.

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.

Thanks again!

> And the comments need to be adjusted.
>
>
> --
> Thanks,
> Tender Wang
>
>


-- 
Regards,
Ewan Young

  4. Re: satisfies_hash_partition crash

    Tender Wang <tndrwang@gmail.com> — 2026-07-02T04:47:30Z

    Ewan Young <kdbase.hack@gmail.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,
Tender Wang

  5. Re: satisfies_hash_partition crash

    Ewan Young <kdbase.hack@gmail.com> — 2026-07-02T05:43:17Z

    Hi Tender,

On Thu, Jul 2, 2026 at 12:47 PM Tender Wang <tndrwang@gmail.com> wrote:
>
> Ewan Young <kdbase.hack@gmail.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

  6. Re: satisfies_hash_partition crash

    Tender Wang <tndrwang@gmail.com> — 2026-07-02T06:17:01Z

    Ewan Young <kdbase.hack@gmail.com> 于2026年7月2日周四 13:43写道：
>
> 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.)

Yes.  The previous fix cannot prevent a crash if my_extra is not NULL,
as your case shows
And I forgot to call relation_close(), so "resource was not closed" would occur.
Anyway, the NULL-array guard must sit ahead.

>
> 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.

Personally,  I prefer a local bool variable near the top and reuse it.


-- 
Thanks,
Tender Wang

  7. Re: satisfies_hash_partition crash

    Robert Haas <robertmhaas@gmail.com> — 2026-07-02T17:54:00Z

    On Thu, Jul 2, 2026 at 2:17 AM Tender Wang <tndrwang@gmail.com> wrote:
> Personally,  I prefer a local bool variable near the top and reuse it.

I prefer to more thoroughly confine this fix to the variadic code
path, since I don't think we really expect that to be exercised in
practice. So here's my proposal.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  8. Re: satisfies_hash_partition crash

    Tender Wang <tndrwang@gmail.com> — 2026-07-03T00:26:51Z

    Robert Haas <robertmhaas@gmail.com> 于2026年7月3日周五 01:54写道：
>
> On Thu, Jul 2, 2026 at 2:17 AM Tender Wang <tndrwang@gmail.com> wrote:
> > Personally,  I prefer a local bool variable near the top and reuse it.
>
> I prefer to more thoroughly confine this fix to the variadic code
> path, since I don't think we really expect that to be exercised in
> practice. So here's my proposal.
WFM
+1



-- 
Thanks,
Tender Wang

  9. Re: satisfies_hash_partition crash

    Ewan Young <kdbase.hack@gmail.com> — 2026-07-03T02:07:46Z

    On Fri, Jul 3, 2026 at 1:54 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Jul 2, 2026 at 2:17 AM Tender Wang <tndrwang@gmail.com> wrote:
> > Personally,  I prefer a local bool variable near the top and reuse it.
>
> I prefer to more thoroughly confine this fix to the variadic code
> path, since I don't think we really expect that to be exercised in
> practice. So here's my proposal.

v2 looks good to me — I tested it and confirmed that it works. Thanks
for picking this up.

>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com

-- 
Regards,
Ewan Young

  10. Re: satisfies_hash_partition crash

    Robert Haas <robertmhaas@gmail.com> — 2026-07-06T17:04:53Z

    On Thu, Jul 2, 2026 at 10:07 PM Ewan Young <kdbase.hack@gmail.com> wrote:
> v2 looks good to me — I tested it and confirmed that it works. Thanks
> for picking this up.

OK, thanks for the reviews, committed and back-patched.

-- 
Robert Haas
EDB: http://www.enterprisedb.com