Re: Partitioned tables and relfilenode - Mailing list pgsql-hackers

Hello,

At Wed, 29 Mar 2017 15:40:20 +0900, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote in
<c4d71df2-9e0e-3912-dc81-9a72e080c238@lab.ntt.co.jp>
> On 2017/03/27 23:27, Robert Haas wrote:
> > On Thu, Mar 23, 2017 at 8:54 PM, Amit Langote
> > <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> >> On 2017/03/23 23:47, Amit Langote wrote:
> >>> On Thu, Mar 23, 2017 at 11:27 PM, Maksim Milyutin
> >>> <m.milyutin@postgrespro.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