Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Hi Stephen,

On 2017/04/18 1:43, Stephen Frost wrote:
> Amit,
>
> * Amit Langote (Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp) wrote:
>> OK, I agree. I tweaked the existing bullet point about differences from
>> traditional inheritance when using ONLY with partitioned tables.
>
> Please take a look at the attached and let me know your thoughts on it.
> I changed the code to complain again regarding TRUNCATE ONLY, since that
> never actually makes sense on a partitioned table, unlike ALTER TABLE
> ONLY, and adjusted the error messages and added hints.

Thanks for polishing the patch. It looks mostly good to me.

+ Once
+ partitions exist, we do not support using <literal>ONLY</literal> to
+ add or drop constraints on only the partitioned table.

I wonder if the following sounds a bit more informative: Once partitions
exist, using <literal>ONLY</literal> will result in an error, because the
constraint being added or dropped must be added or dropped from the
partitions too.

>> Updated patches attached (0002 and 0003 unchanged).
>
> Regarding these, 0002 changes pg_dump to handle partitions much more
> like inheritance, which at least seems like a decent idea but makes me a
> bit concerned about making sure we're doing everything correctly.

Are you concerned about the correctness of the code in pg_dump or backend?

Rules of inheritance enforced by flagInhAttrs() are equally applicable to
the partitioning case, so actions performed by it for the partitioning
cases will not emit incorrect text.

The rule that backend follows is this: if a constraint is added to the
partitioned table (either a named check constraint or a NOT NULL
constraint), that constraint becomes an inherited *non-local* constraint
in all the partitions no matter if it was present in the partitions before
or not.

> In
> particular, we should really have regression tests for non-inherited
> CHECK (and NOT NULL) constraints on partitions. Perhaps there are tests
> covering those cases in the standard regression suite, but it's not
> obvious from the SQL.

There is one in create_table.sql that looks like this:

CREATE TABLE part_b PARTITION OF parted (
b NOT NULL DEFAULT 1 CHECK (b >= 0),
CONSTRAINT check_a CHECK (length(a) > 0)
) FOR VALUES IN ('b');
-- conislocal should be false for any merged constraints
SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid =
'part_b'::regclass AND conname = 'check_a';

But it doesn't really look like it's testing non-inherited constraints a
whole lot (CHECK (b >= 0) above is a local non-inherited constraint).

I added a few more tests right below the above test so that there is a bit
more coverage. Keep the rule I mentioned above when looking at these. I
also added a test in the pg_dump suite. That's in patch 0001 (since you
posted your version of what was 0001 before, I'm no longer including it here).

By the way, 0003 is a bug-fix. WITH OPTIONS that is emitted currently
like below is not the acceptable syntax:

CREATE TABLE a_partition OF a_partitioned_table (
a WITH OPTIONS NOT NULL DEFAULT 1
) FOR VALUES blah

But looking at the pg_dump code closely and also considering our
discussion upthread, I don't think this form is ever emitted. Because we
never emit a partition's column-level constraints and column defaults in
the CREATE TABLE, but rather separately using ALTER TABLE. In any case,
applying 0003 seems to be the right thing to do anyway, in case we might
rejigger things so that whatever can be emitted within the CREATE TABLE
text will be.

Thanks,
Amit