Re: Partitioned tables and relfilenode

Hello,

At Wed, 29 Mar 2017 15:40:20 +0900, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote in <c4d71df2-9e0e-3912-dc81-9a72e080c238(at)lab(dot)ntt(dot)co(dot)jp>
> On 2017/03/27 23:27, Robert Haas wrote:
> > On Thu, Mar 23, 2017 at 8:54 PM, Amit Langote
> > <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> >> On 2017/03/23 23:47, Amit Langote wrote:
> >>> On Thu, Mar 23, 2017 at 11:27 PM, Maksim Milyutin
> >>> <m(dot)milyutin(at)postgrespro(dot)ru> wrote:
> >>>> Hi!
> >>>>
> >>>> I have noticed that there is scheduled unlinking of nonexistent physical
> >>>> storage under partitioned table when we execute DROP TABLE statement on this
> >>>> partitioned table. Though this action doesn't generate any error under
> >>>> typical behavior of postgres because the error of storage's lack is caught
> >>>> through if-statement [1] I think it is not safe.
> >>>>
> >>>> My patch fixes this issue.
> >>>>
> >>>> 1. src/backend/storage/smgr/md.c:1385
> >>>
> >>> Good catch, will incorporate that in the main patch.
> >>
> >> 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.

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

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center