Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table - Mailing list pgsql-hackers

Amit,

* Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
> On 2017/04/18 1:43, Stephen Frost wrote:
> > * Amit Langote (Langote_Amit_f8@lab.ntt.co.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.

My thinking here is that we may add support later on down the line for
using the ONLY keyword, when the constraint has already been created on
the partitions themselves, so I would rather just clarify that we don't
(yet) support that.  Your wording, at least to me, seems to imply that
if the constraint was added to or dropped from the partitions then the
ONLY keyword could be used, which isn't accurate today.

> >> 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?

My concern is with regard to pg_dump in this case, primairly.

> 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.

I understand that this *should* be the case, but that's not how the code
in pg_dump was written originally when it came to native partitions,
having its own flagPartitions() function in fact, which makes me
hesitate to start assuming what we do for inheritance is going to work
in these cases the same.

> 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.

Right, I get that.

> > 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).

Right, I saw the additional tests in your original patch, though it
DROP'd at least parted_no_parts, meaning that it wasn't being tested as
part of pg_upgrade/pg_dump testing.

> 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.

Hm, ok.

I'm going over your latest set of patches.

Thanks!

Stephen