Re: pg_dump emits ALTER TABLE ONLY partitioned_table

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.

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

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

Thanks!

Stephen