From: | Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Stephen Frost <sfrost(at)snowman(dot)net> |
Cc: | Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Noah Misch <noah(at)leadboat(dot)com>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: pg_dump emits ALTER TABLE ONLY partitioned_table |
Date: | 2017-04-27 08:00:04 |
Message-ID: | 47e1d272-8ce0-dea8-2bb5-642efa95c337@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | 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
Attachment | Content-Type | Size |
---|---|---|
0001-Improve-test-coverage-of-partition-constraints.patch | text/x-diff | 6.9 KB |
0002-Allow-partition-columns-to-optionally-include-WITH-O.patch | text/x-diff | 2.8 KB |
0003-Change-the-way-pg_dump-retrieves-partitioning-info.patch | text/x-diff | 18.1 KB |
0004-Fix-a-bug-in-pg_dump-s-binary-upgrade-code.patch | text/x-diff | 2.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Petr Jelinek | 2017-04-27 08:08:03 | Re: Logical replication in the same cluster |
Previous Message | Michael Paquier | 2017-04-27 07:51:19 | Re: [PostgreSQL 10] default of hot_standby should be "on"? |