Re: [COMMITTERS] pgsql: Add hash partitioning.

On Tue, Nov 14, 2017 at 3:59 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Mon, Nov 13, 2017 at 3:24 AM, amul sul <sulamul(at)gmail(dot)com> wrote:
>> Updated patch attached -- Adjusted code comment to survive against pgindent.
>
> That's not the right fix, or at least it's not complete. You
> shouldn't call PG_GETARG_...(n) until you've verified that
> PG_ARGISNULL(n) returns false.
>
> Also, I don't think moving the heap_open() earlier helps anything, but
> you do need to replace Assert(key->partnatts == nkeys) with an
> ereport() -- or just return false, but I think ereport() is probably
> better. Otherwise someone calling satisfies_hash_function() with a
> wrong number of arguments for the partitioned table can cause an
> assertion failure, which is bad.

Yeah, this patch needs more work. There is no need to do much efforts
on HEAD to crash it:
=# create table aa (a int);
CREATE TABLE
=# select satisfies_hash_partition('aa'::regclass, 0, NULL, 'po');
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.

Could you add regression tests calling directly this function?

elog() can also be triggered easily, which should not happen with
user-callable functions:
=# select satisfies_hash_partition(0, 0, NULL, 'po');
ERROR: XX000: could not open relation with OID 0

Thanks,
--
Michael