Amit, * Amit Langote (langote_amit...@lab.ntt.co.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
signature.asc
Description: Digital signature
