Thanks, Michael & Robert for your suggestions, and apologize for
delayed response
On Tue, Nov 14, 2017 at 6:02 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Nov 14, 2017 at 3:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Mon, Nov 13, 2017 at 3:24 AM, amul sul <sulamul@gmail.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.
>
Understood, fixed in the 001 patch.
> 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.
>
Fixed in the 001 patch.
> Could you add regression tests calling directly this function?
>
Yes sure, but I am not sure that we really need this and also not sure about
which regression file is well suitable for these tests (to be honest, I haven't
browsed regression directory in detail due to lack of time today), so created
new file in 002 patch which is WIP.
Do let me know your thoughts will try to improve 0002 patch, tomorrow.
> 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
>
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.
Regards,
Amul Sul