Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Hi Stephen,

On 2017/04/11 22:12, Stephen Frost wrote:
> Amit,
>
> * Amit Langote (Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp) wrote:
>> On 2017/04/11 0:26, Robert Haas wrote:
>>> Children can have constraints (including NOT NULL constraints) which
>>> parents lack, and can have a different column order, but must have
>>> exactly the same column names and types.
>>
>> Also, what is different in the partitioned parent case is that NOT NULL
>> constraints must be inherited. That is, one cannot add it only to the parent.
>
> If I'm following, children can have additional constraints but any
> constraints on the parent must also exist on all the children. Is that
> correct?

Yes. As shown in the examples I posted, in traditional inheritance, only
CHECK constraints are treated that way, whereas NOT NULL constraints are
not. With partitioning, even the NOT NULL constraints are inherited. So
if a particular column is set NOT NULL in the parent, all partitions in
the tree must have that constraint and it cannot be dropped from a
partition without dropping it from the parent as well. Traditional
inheritance applies that rule only to attributes and CHECK constraints.

>> --
>> -- partitioning inheritance
>> --
>> create table parted_parent (a int) partition by list (a);
>> create table part partition of parted_parent for values in (1);
>>
>> -- this is same as traditional inheritance
>> alter table only parted_parent add constraint chka check (a > 0);
>> -- ERROR: constraint must be added to child tables too
>
> Ok, this makes sense, but surely that constraint does, in fact, exist on
> the child already or we wouldn't be trying to dump out this constraint
> that exists on the parent?

Yes, that's right.

Now that I have thought about this a bit more, I think I can articulate
the problem statement more clearly. The problem we have here is about
non-inherited constraints on partitions, specifically, the NOT NULL
constraints which need to be emitted per attribute. We have two options of
doing it: emit it inline with CREATE TABLE or separately with ALTER TABLE
ONLY.

I have been saying that it is preferable to use CREATE TABLE, because
ALTER TABLE ONLY will fail if the partition is itself partitioned. Why
does it cause an error? Because of the ONLY part. It is a user error to
specify ONLY when adding a constraint to a partitioned tables, but...

>>> In Amit's example from the original post, the child has an implicit
>>> NOT NULL constraint that does not exist in the parent. p1.b isn't
>>> declared NOT NULL, but the fact that it is range-partitioned on b
>>> requires it to be so, just as we would do if b were declared as the
>>> PRIMARY KEY. Somehow that's not playing nice with pg_dump, but I'm
>>> still fuzzy on the details.
>>
>> Actually, I would like to change the problem definition from "ALTER TABLE
>> ONLY partitioned_table should be avoided" to "Emitting partition's
>> attributes separately should be avoided".
>
> I don't follow why we think doing:
>
> CREATE TABLE t1 (c1 int);
> ALTER TABLE ONLY t1 SET c1 NOT NULL;
>
> is really different from:
>
> CREATE TABLE t1 (c1 int NOT NULL);
>
> or why we should teach pg_dump that it's "correct" to consider those two
> to be different. There are specific cases where they have to be done
> independently, but that's for views because we don't have a way to set a
> default on a view column during CREATE VIEW, or to deal with dropped
> columns or traditionally inheirited columns.
>
> What isn't clear to me is why the CREATE TABLE + ALTER TABLE isn't
> working, when apparently a CREATE TABLE with the NOT NULL included would
> work. The issue here seems like it's the order in which the operations
> are happening in, and not that CREATE TABLE + ALTER TABLE is somehow
> different than just the CREATE TABLE.

I think you are right. CREATE TABLE + ALTER TABLE ONLY should work just
fine in this particular case (i.e. the way pg_dump outputs it).

>> create table p (a int, b int) partition by list (a);
>> create table p1 partition of p for values in (1) partition by range (b);
>> create table p11 partition of p1 (
>> a not null default '1'
>> ) for values from (1) to (10);
>
> Using the above example, doing a pg_dump and then a restore (into a
> clean initdb'd cluster), I get the following:
>
> =# CREATE TABLE p (
> -# a integer,
> -# b integer
> -# )
> -# PARTITION BY LIST (a);
> CREATE TABLE
>
> =*# CREATE TABLE p1 PARTITION OF p
> -*# FOR VALUES IN (1)
> -*# PARTITION BY RANGE (b);
> CREATE TABLE
>
> =*# ALTER TABLE ONLY p1 ALTER COLUMN b SET NOT NULL;
> ERROR: constraint must be added to child tables too
>
> Now, perhaps I'm confused, but isn't p1 the child here? Which is
> supposed to be able to have constraints that the parent doesn't?

Actually, p1 is a partitioned table, so the error. And I realize that
that's a wrong behavior. Currently the check is performed using only the
relkind, which is bogus. Specifying ONLY should cause an error only when
the table has partitions.

> We haven't even gotten to the point where p1 is a parent yet because p11
> hasn't been created yet. Further, according to psql's \d, 'p1.b'
> already has a NOT NULL constraint on it, so the above really should just
> be a no-op.

Right. The already existing NOT NULL constraint results from p1.b being
the range partition key column.

> I get the feeling that we're looking in the wrong place for the issue
> here.

Yes, I think we should consider fixing the backend behavior here as Tom
also suspected. Throwing an error when no partitions yet exist might be a
bad idea after all.

Also, I mentioned above that the problem is only with non-inherited
constraints of partitions, but I've just discovered an issue with
inherited constraints as well. They need not be emitted for partitions
*again*, but they are.

So, here are the patches:

0001: Fix ALTER TABLE .. SET/DROP NOT NULL and DROP CONSTRAINT so that
they don't cause an error when ONLY is specified and no partitions exist.

0002: Fix pg_dump so that inherited constraints are not printed separately
for partitions. (pg_dump TAP test output is updated)

0003: Fix pg_dump to not emit WITH OPTIONS when printing constraints of
a partition's columns

It would be great if you could take a look.

Thanks,
Amit