Thread: Re: [COMMITTERS] pgsql: Add hash partitioning.
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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
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.
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
processingthe 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
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
Attachment
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. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
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
On Mon, Nov 20, 2017 at 7:46 AM, amul sul <sulamul@gmail.com> wrote:
> 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.
Oops. Looks like the indexing should be 1-based rather than 0-based.
Committed with that change.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company