On Sat, Nov 18, 2017 at 1:19 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Nov 16, 2017 at 9:37 AM, amul sul <sulamul@gmail.com> wrote:
>> Fixed in the 001 patch.
>>
>> IMHO, this function is not meant for a user, so that instead of ereport() cant
>> we simply return false? TWIW, I have attached 003 patch which replaces all
>> erepots() by return false.
>
> I don't think just returning false is very helpful behavior, because
> the user may not realize that the problem is that the function is
> being called incorrectly rather than that the call is correct and the
> answer is false.
>
> I took your 0001 patch and made extensive modifications to it. I
> replaced your regression tests from 0002 with a new set that I wrote
> myself. The result is attached here. This version makes different
> decisions about how to handle the various problem cases than you did;
> it returns NULL for a NULL input or an OID for which no relation
> exists, and throws specific error messages for the other cases,
> matching the parser error messages that CREATE TABLE would issue where
> possible, but with a different error code. It also checks that the
> types match (which your patch did not, and which I'm fairly sure could
> crash the server), makes the function work when invoked using the
> explicit VARIADIC syntax (which seems fairly useless here but there's
> no in-core precedent for variadic function which doesn't support that
> case), and fixes the function header comment to describe the behavior
> we committed rather than the older behavior you had in earlier patch
> versions.
>
> As far as I can tell, this should nail things down pretty tight, but
> it would be great if someone can try to find a case where it still
> breaks.
>
Thanks for fixing this function. Patch looks good to me, except column number
in the following errors message should to be 2.
354 +SELECT satisfies_hash_partition('mchash'::regclass, 2, 1,
NULL::int, NULL::int);
355 +ERROR: column 1 of the partition key has type "text", but
supplied value is of type "integer"
Same at the line # 374 & 401 in the patch.
Regards,
Amul
