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

Hi Stephen,

On 2017/04/26 23:31, Stephen Frost wrote:
>>> I looked through
>>> pg_get_partkeydef() and it didn't seem to be particularly expensive to
>>> run, though evidently it doesn't handle being passed an OID that it
>>> doesn't expect very cleanly:
>>>
>>> =# select pg_get_partkeydef(oid) from pg_class;
>>> ERROR:  cache lookup failed for partition key of 52333
>>>
>>> I don't believe that's really very appropriate of it to do though and
>>> instead it should work the same way pg_get_indexdef() and others do and
>>> return NULL in such cases, so people can use it sanely.
>>>
>>> I'm working through this but it's going to take a bit of time to clean
>>> it up and it's not going to get finished today, but I do think it needs
>>> to get done and so I'll work to make it happen this week.  I didn't
>>> anticipate finding this, obviously and am a bit disappointed by it.
>>
>> Yeah, that was sloppy. :-(
>>
>> Attached patch 0005 fixes that and adds a test to rules.sql.
> 
> Good, I'll commit that first, since it will make things simpler for
> pg_dump.

Thanks for committing this one.

>> So to summarize what the patches do (some of these were posted earlier)
>>
>> 0002: pg_dump: Do not emit WITH OPTIONS keywords with partition's columns
> 
> I'm trying to understand why this is also different.  At least on an
> initial look, there seems to be a bunch more copy-and-paste from the
> existing TypedTable to Partition in gram.y, which seems to all boil down
> to removing the need for WITH OPTIONS to be specified.  If WITH OPTIONS
> would be too much to have included for partitioning, and it isn't
> actually necessary, why not just make it optional, and use the same
> construct for both, removing all the duplicaiton and the need for
> pg_dump to output it?

OK, I think optionally allowing WITH OPTIONS keywords would be nice.

So in lieu of this patch, I propose a patch that modifies gram.y to allow
WITH OPTIONS to specified.

>> 0003: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE
>>       INHERIT to be emitted to attach a partition instead of ALTER TABLE
>>       ATTACH PARTITION
> 
> Well, worse, it was dumping out both, the INHERITs and the ATTACH
> PARTITION.

Oops, you're right.  Actually meant to say that.

> Given that we're now setting numParents for partitions, I
> would think we'd just move the if (tbinfo->partitionOf) branches up
> under the if (numParents > 0) branches, which I think would also help us
> catch additional issues, like the fact that a binary-upgrade with
> partitions in a different namespace from the partitioned table would
> fail because the ATTACH PARTITION code doesn't account for it:
> 
>                 appendPQExpBuffer(q, "ALTER TABLE ONLY %s ",
>                                   fmtId(tbinfo->partitionOf->dobj.name));
>                 appendPQExpBuffer(q, "ATTACH PARTITION %s %s;\n",
>                                   fmtId(tbinfo->dobj.name),
>                                   tbinfo->partitiondef);
> 
> unlike the existing inheritance code a few lines above, which does:
> 
>                     appendPQExpBuffer(q, "ALTER TABLE ONLY %s INHERIT ",
>                                       fmtId(tbinfo->dobj.name));
>                     if (parentRel->dobj.namespace != tbinfo->dobj.namespace)
>                         appendPQExpBuffer(q, "%s.",
>                                 fmtId(parentRel->dobj.namespace->dobj.name));
>                     appendPQExpBuffer(q, "%s;\n",
>                                       fmtId(parentRel->dobj.name));

That's a good point.  I put both cases under the if (numParents > 0) block
and appropriate text is emitted depending on whether the table is a
partition or plain inheritance child.

>> 0004: Change the way pg_dump retrieves partitioning info (removes a lot
>>       of unnecessary code and instead makes partitioning case use the old
>>       inheritance code, etc.)
> 
> This looks pretty good, at least on first blush, probably in part
> because it's mostly removing code.  The CASE statement in the
> getTables() query isn't needed, once pg_get_partkeydef() has been
> changed to not throw an ERROR when passed a non-partitioned table OID.

Oh yes, fixed.

I put the patches in different order this time so that the refactoring
patch appears before the --binary-upgrade bug-fix patch (0003 and 0004
have reversed their roles.)  Also, 0002 is no longer a pg_dump fix.

0001: Improve test coverage of partition constraints (non-inherited ones)

0002: Allow partition columns to optionally include WITH OPTIONS keywords

0003: Change the way pg_dump retrieves partitioning info (removes a lot
      of unnecessary code and instead makes partitioning case use the old
      inheritance code, etc.)

0004: Fix a bug in pg_dump's --binary-upgrade code that caused ALTER TABLE
      INHERIT to be emitted to attach a partition in addition to of ALTER
      TABLE ATTACH PARTITION and that no schema-name was emitted where it
      should have

Thanks,
Amit

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers