Re: Partitioned tables and relfilenode

Horiguchi-san,

Thanks for taking a look.

On 2017/03/29 16:49, Kyotaro HORIGUCHI wrote:
> At Wed, 29 Mar 2017 15:40:20 +0900, Amit Langote wrote:
>> On 2017/03/27 23:27, Robert Haas wrote:
>>>> And here is the updated patch.
>>>
>>> I think you should go back to the earlier strategy of disallowing heap
>>> reloptions to be set on the partitioned table.  The thing is, we don't
>>> really know which way we're going to want to go in the future.  Maybe
>>> we'll come up with a system where you can set options on the
>>> partitioned table, and those options will cascade to the children.  Or
>>> maybe we'll come up with a system where partitioned tables have a
>>> completely different set of options to control behaviors specific to
>>> partitioned tables.  If we do the latter, then we don't want to also
>>> have to support useless heap reloptions for forward compatibility, nor
>>> do we want to break backward-compatibility to remove support.  If we
>>> do the former, then it's better if we allow it in the same release
>>> where it starts working.
>>
>> You're right, modified the patch accordingly.
>>
>> By the way, the previous version of the patch didn't really "disallow"
>> specifying heap reloptions though.  What I'd imagine that should entail is
>> CREATE TABLE or ALTER TABLE raising error if one of those reloptions is
>> specified, which didn't really occur with the patch.  The options were
>> silently accepted and stored into pg_class, but their values were never
>> used.  I modified the patch so that an error occurs instead of silently
>> accepting the user input.
>>
>> create table p (a int) partition by list (a) with (fillfactor = 10);
>> ERROR:  unrecognized parameter "fillfactor" for a partitioned table
> 
> The following attracted my eyes.
> 
> +      if (def->defnamespace == NULL &&
> +          pg_strcasecmp(def->defname, "oids") != 0)
> +        ereport(ERROR,
> +          (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> +           errmsg("unrecognized parameter \"%s\" for a partitioned table",
> 
> This works since defnamespace is always NULL here, but if I
> understand correctly what we should do here is "reject any option
> other than "(default).OID"". So I think that the condition should
> be like the following.

You're right.  The following *wrongly* succeeds:

create table p (a int) partition by list (a) with
(toast.autovacuum_enabled = true);
CREATE TABLE

> +      if (def->defnamespace != NULL ||
> +          pg_strcasecmp(def->defname, "oids") != 0)

Looks correct, so incorporated in the attached updated patch.  Thanks.

Regards,
Amit