Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Amit,

* Amit Langote (Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp) wrote:
> On 2017/04/26 0:42, Stephen Frost wrote:
> > I'm not sure what you mean here. We're always going to call both
> > getInherits() and getPartitions() and run the queries in each, with the
> > way the code is written today. In my experience working with pg_dump
> > and trying to not slow it down, the last thing we want to do is run two
> > independent queries when one will do. Further, we aren't really
> > avoiding any work when we have to go look at pg_class to exclude the
> > partitioned tables in getInherits() anyway. I'm not seeing why we
> > really need the join at all though, see below.
>
> You're right and I realize that we don't need lots of new code for pg_dump
> to retrieve the partitioning information (including the parent-child
> relationships). I propose some patches below, one per each item you
> discovered could be improved.

Thanks!

> Attached patch 0004 gets rid of that separation. It removes the struct
> PartInfo, functions flagPartitions(), findPartitionParentByOid(),
> getPartitions(), and getTablePartitionKeyInfo(). All necessary
> partitioning-specific information is retrieved in getTables() itself.

That certainly looks better.

> Also, now that partitioning uses the old inheritance code, inherited NOT
> NULL constraints need not be emitted separately per child. The
> now-removed code that separately retrieved partitioning inheritance
> information was implemented such that partition attributes didn't receive
> the flagInhAttrs() treatment.

Right.

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

> I think I have found an oversight in the pg_dump's --binary-upgrade code
> that might have caused the error you saw (I proceeded to fix it after
> seeing the error that I saw). Fix is included as patch 0003.

Yeah, that was what I saw also.

> So to summarize what the patches do (some of these were posted earlier)
>
> 0001: Improve test coverage of partition constraints (non-inherited ones)

Looks fine.

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

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

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

> 0005: Teach pg_get_partkeydef_worker to deal with OIDs it can't handle

Looks fine, will commit this one later today.

Thanks!

Stephen