Re: Partitioned tables and relfilenode

From: Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp>
To: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: robertmhaas(at)gmail(dot)com, amitlangote09(at)gmail(dot)com, m(dot)milyutin(at)postgrespro(dot)ru, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Partitioned tables and relfilenode
Date: 2017-03-29 08:21:26
Message-ID: b1234f04-53e0-011d-9f02-2437b909cce4@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
0001-Do-not-allocate-storage-for-partitioned-tables.patch text/x-diff 9.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Kang Yuzhe 2017-03-29 08:29:17 Re: On How To Shorten the Steep Learning Curve Towards PG Hacking...
Previous Message Erik Rijkers 2017-03-29 08:14:28 Re: Logical replication existing data copy