Re: pg_dump emits ALTER TABLE ONLY partitioned_table

Hi Stephen,

On 2017/04/26 0:42, Stephen Frost wrote:
> Amit,
>
> * Amit Langote (Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp) wrote:
>> I think why getPartitions() is separate from getInherits() and then
>> flagPartitions() separate from flagInhTables() is because I thought
>> originally that mixing the two would be undesirable. In the partitioning
>> case, getPartitions() joins pg_inherits with pg_class so that it can also
>> get hold of relpartbound, which I then thought would be a bad idea to do
>> for all of the inheritance tables. If we're OK with always doing the
>> join, I don't see why we couldn't get rid of the separation.
>
> 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.

>> flagInhAttrs(), OTOH, seems unaffected by that concern and I think using
>> it for both inheritance and partitioning is fine. It affects NOT NULL
>> constraints and defaults, which as far as it goes, applies to both
>> inheritance and partitioning the same.
>
> I don't see why we need to have getPartitions(), flagPartitions(), or
> findPartitonParentByOid(). They all seem to be largely copies of the
> inheritance functions, but it doesn't seem like that's really necessary
> or sensible.
>
> I also don't follow why we're pulling the partbound definition in
> getPartitions() instead of in getTables(), where we're already pulling
> the rest of the data from pg_class? Or why getTablePartitionKeyInfo()
> exists instead of also doing that during getTables()?

I think it had to do with wanting to avoid creating yet another copy of
the big getTables() query for >= v10, back when the original patch was
written, but doing that is not really needed.

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.

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.

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

>> So, the newly added tests seem enough for now?
>
> Considering the pg_upgrade regression test didn't pass with the patch
> as sent, I'd say that more tests are needed. I will be working to add
> those also.

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.

So to summarize what the patches do (some of these were posted earlier)

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

0002: pg_dump: Do not emit WITH OPTIONS keywords with partition's columns

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

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

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

Thanks,
Amit