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

Hi Stephen,

Sorry about the delay.

On 2017/04/27 23:17, Stephen Frost wrote:
> Amit,
> 
> * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
>>>> 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.
> 
> The point I was trying to get at was that if you make WITH OPTIONS
> optional for the TypedTableElement case, you can remove all of the
> PartitionElement-related productions and then both the OF type_name and
> the PARTITION OF case will accept WITH OPTIONS as noise words and also
> work without WITH OPTIONS being specified.
> 
> This also makes the code actually match the documentation since, at
> least in the CREATE FOREIGN TABLE documentation, we claim that WITH
> OPTIONS is required.
> 
> Please see a proposed patch attached to accomplish this.

The committed patch looks good to me.  Thanks.

Now that WITH OPTIONS is optional even for CREATE TABLE OF, perhaps it
needs to be mentioned in the release notes?

>>> 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.
> 
> Right, ok.
> 
>>>> 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.
> 
> Good.
> 
>> 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.)
> 
> This has conflicts due to my proposed patch as my patch changes pg_dump
> to not output the now-noise-words WITH OPTIONS at all.

Rebased.

>> 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
> 
> Given that it's touching the same places, this would really make sense
> to merge into 0003 now.

OK, done.

> I'm going to continue to mull over the attached for a bit and add some
> tests to it, but if it looks good then I'll push it this afternoon,
> after which you'll hopefully have time to rebase 0003 and merge in 0004
> to that, which I can review and probably push tomorrow.

Attached updated patches.

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