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

Hi Stephen,

On 2017/04/14 0:05, Stephen Frost wrote:
> Robert,
> 
> * Robert Haas (robertmhaas@gmail.com) wrote:
>> So I think I was indeed confused before, and I think you're basically
>> right here, but on one point I think you are not right -- ALTER TABLE
>> ONLY .. CHECK () doesn't work on a table with inheritance children
>> regardless of whether the children already have the matching
>> constraint:
> 
> To be clear- I wasn't thinking about what's possible today with
> inheiritance or partitioning or in PG at all, but rather focusing on
> what a user is likely to expect and understand from the messaging.
> 
>> rhaas=# create table foo (a int, b text);
>> CREATE TABLE
>> rhaas=# create table bar () inherits (foo);
>> CREATE TABLE
>> rhaas=# alter table only foo add check (a = 1);
>> ERROR:  constraint must be added to child tables too
>> rhaas=# alter table only bar add check (a = 1);
>> ALTER TABLE
>> rhaas=# alter table only foo add check (a = 1);
>> ERROR:  constraint must be added to child tables too
> 
> If that's the case then we should really change that error message, in
> my view.  I'd be happier if we did support adding the constraint after
> child tables exist,

Do you mean to use ONLY even if the child tables exist and still succeed?
You can succeed in that case only by specifying NO INHERIT against the
constraint with traditional inheritance.  It does not work however with
partitioned tables, since we do not allow NO INHERIT constraints to be
defined on the partitioned tables; such a constraint would never be
enforced if we had allowed that and just sit there.  In the traditional
inheritance case, it is applied to the parent's own rows.

> but if we don't, we shouldn't imply to the user that
> they can add it after adding it to the children.

Hmm, the error message as it is *might* give the impression that we are
asking a user to go add the constraint to the child tables first.  Another
way the user might interpret the message is that it is asking to drop the
ONLY from the command (at least if they have read the documentation of
ONLY on the ALTER TABLE reference page).  But it perhaps isn't readily
apparent from the error message itself, so maybe a HINT message along
those lines might work.

>> So, regarding Amit's 0001:
>>
>> - I think we should update the relevant hunk of the documentation
>> rather than just removing it.
> 
> Alright.

You may have seen that the latest patch has taken care of this.  But maybe
you'll want to tweak my rewording further as you see fit.

>> - Should we similarly allow TRUNCATE ONLY foo and ALTER TABLE ONLY foo
>> .. to work on a partitioned table without partitions, or is that just
>> pointless tinkering?  That seems to be the only case where, after this
>> patch, an ONLY operation will fail on a childless partitioned table.
> 
> I'm less sure about TRUNCATE ONLY because that really isn't meaningful
> at all, is it?  A partitioned table doesn't have any data to truncate
> and truncating it doesn't have any impact on what happens later, does
> it?

That's right.  If you perform truncate (or truncate only) on an empty
partitioned table (i.e. with no partitions yet), it's essentially a no-op.Nor does it affect anything in the future.

> Having a sensible error be reported if someone tries to do that
> would be good though.

The latest patch implements the following:

-- create an empty partitioned table, aka without partitions
create table p (a int) partition by list (a);

-- no error, though nothing really happens
truncate only p;
TRUNCATE TABLE

-- add a partition
create table p1 partition of p for values in (1);

-- error, because a partition exists
truncate only p;
ERROR:  must truncate partitions too

-- ok, partition p1 will be truncated
truncate p;
TRUNCATE TABLE

I do now wonder if the error message "must truncate partitions *too*" is
somewhat inappropriate here.  The "too" seems to imply that table
specified in the command (which is partitioned) is somehow truncate-able,
which it is not.  Hmm.

>> Do you want to be responsible for committing these or should I do it?
> 
> Sure, though I won't be able to today and I've got some doubts about the
> other patches.  I'll have more time tomorrow though.

Thanks, I'll revise the patches per any review comments you might have.

Regards,
Amit