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

Hi Stephen,

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

OK, I wanted to word it like that, because I have been thinking that we
will never support that.

So right now, we do not support ONLY (or how I tend to read it: cause
error on specifying ONLY) if partitions exist at all.  In the future, we
will not output error until we have also seen that some partition does not
possess the constraint being added.  IOW, specifying ONLY won't cause an
error even with non-zero partitions if all of them have the constraint
already defined.  Maybe that's something we will support, but I am not
sure how many users will want to do things that way instead of the other
way around; I mean if the constraint is to be enforced on the whole
partitioned table anyway, why not just define it on the partitioned table
in the first place.  But then again, why restrict users in many number of
ways.

So alright, I'm fine with the wording.  If someone complains later that
they want more clarification on that point, it could be done later.

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

Yes, that's likely to be the impression.

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

I think why getPartitions() is separate from getInherits() and then
flagPartitions() separate from flagInhTables() is because I thought
originally that mixing the two would be undesirable.  In the partitioning
case, getPartitions() joins pg_inherits with pg_class so that it can also
get hold of relpartbound, which I then thought would be a bad idea to do
for all of the inheritance tables.  If we're OK with always doing the
join, I don't see why we couldn't get rid of the separation.

flagInhAttrs(), OTOH, seems unaffected by that concern and I think using
it for both inheritance and partitioning is fine.  It affects NOT NULL
constraints and defaults, which as far as it goes, applies to both
inheritance and partitioning the same.

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

So, the newly added tests seem enough for now?

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

Regards,
Amit