On Mon, Dec 13, 2021 at 9:40 AM Shruthi Gowda <gowdas...@gmail.com> wrote: > > I am reviewing another patch > > "v5-0001-Preserve-relfilenode-and-tablespace-OID-in-pg_upg" as well > > and will provide the comments soon if any...
I spent much of today reviewing 0001. Here's an updated version, so far only lightly tested. Please check whether I've broken anything. Here are the changes: - I adjusted the function header comment for heap_create. Your proposed comment seemed like it was pretty detailed but not 100% correct. It also made one of the lines kind of long because you didn't wrap the text in the surrounding style. I decided to make it simpler and shorter instead of longer still and 100% correct. - I removed a one-line comment that said /* Override the toast relfilenode */ because it preceded an error check, not a line of code that would have done what the comment claimed. - I removed a one-line comment that said /* Override the relfilenode */ because the following code would only sometimes override the relfilenode. The code didn't seem complex enough to justify a a longer and more accurate comment, so I just took it out. - I changed a test for (relkind == RELKIND_RELATION || relkind == RELKIND_SEQUENCE || relkind == RELKIND_MATVIEW) to use RELKIND_HAS_STORAGE(). It's true that not all of the storage types that RELKIND_HAS_STORAGE() tests are possible here, but that's not a reason to avoiding using the macro. If somebody adds a new relkind with storage in the future, they might miss the need to manually update this place, but they will not likely miss the need to update RELKIND_HAS_STORAGE() since, if they did, their code probably wouldn't work at all. - I changed the way that you were passing create_storage down to heap_create. I think I said before that you should EITHER fix it so that we set create_storage = true only when the relation actually has storage OR ELSE have heap_create() itself override the value to false when there is no storage. You did both. There are times when it's reasonable to ensure the same thing in multiple places, but this doesn't seem to be one of them. So I took that out. I chose to retain the code in heap_create() that overrides the value to false, added a comment explaining that it does that, and then adjusted the callers to ignore the storage type. I then added comments, and in one place an assertion, to make it clearer what is happening. - In pg_dump.c, I adjusted the comment that says "Not every relation has storage." and the test that immediately follows, to ignore the relfilenode when relkind says it's a partitioned table. Really, partitioned tables should never have had relfilenodes, but as it turns out, they used to have them. Let me know your thoughts. -- Robert Haas EDB: http://www.enterprisedb.com
v7-0001-pg_upgrade-Preserve-relfilenodes-and-tablespace-O.patch
Description: Binary data
