Re: [COMMITTERS] pgsql: Add hash partitioning.

From: Robert Haas <robertmhaas(at)gmail(dot)com>
To: amul sul <sulamul(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-17 19:49:23
Message-ID: CA+Tgmob4vykTJ7-hRJNTVFL1yFkDhwROmSegWURHh72nk4X2vw@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers pgsql-hackers

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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachment Content-Type Size
0001-Fix-multiple-problems-with-satisfies_hash_partition.patch application/octet-stream 18.5 KB

In response to

Responses

Browse pgsql-committers by date

  From Date Subject
Next Message Robert Haas 2017-11-17 19:55:19 pgsql: Update postgresql.conf.sample comment for bgwriter_lru_maxpages
Previous Message Tom Lane 2017-11-17 18:09:26 Re: pgsql: Remove BufFile's isTemp flag.

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2017-11-17 19:55:32 Re: bgwriter_lru_maxpages range in postgresql.conf
Previous Message Peter Geoghegan 2017-11-17 19:36:06 Re: Treating work_mem as a shared resource (Was: Parallel Hash take II)