From: | amul sul <sulamul(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Andreas Seltenreich <seltenreich(at)gmx(dot)de>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [COMMITTERS] pgsql: Add hash partitioning. |
Date: | 2017-11-20 12:46:51 |
Message-ID: | CAAJ_b94uofRz9FYwKsSFwdesr7oApy0Wye9hWRDFtOmZ2LHCrQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-committers pgsql-hackers |
On Sat, Nov 18, 2017 at 1:19 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Nov 16, 2017 at 9:37 AM, amul sul <sulamul(at)gmail(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Amit Kapila | 2017-11-20 13:38:28 | Re: [COMMITTERS] pgsql: Remove secondary checkpoint |
Previous Message | Simon Riggs | 2017-11-20 07:09:56 | Re: pgsql: Parameter toast_tuple_target controls TOAST for new rows |
From | Date | Subject | |
---|---|---|---|
Next Message | Laurenz Albe | 2017-11-20 12:51:19 | Re: Unicode or local language |
Previous Message | Lelisa Diriba | 2017-11-20 12:15:32 | Unicode or local language |