Re: preserving db/ts/relfilenode OIDs across pg_upgrade (was Re: storing an explicit nonce) - Mailing list pgsql-hackers

> The rest of this email will be detailed review comments on the patch
> as presented, and thus probably only interesting to someone actually
> working on the patch. Feel free to skip if that's not you.
>
> - I suggest splitting the patch into one portion that deals with
> database OID and another portion that deals with tablespace OID and
> relfilenode OID, or maybe splitting it all the way into three separate
> patches, one for each. This could allow the uncontroversial parts to
> get committed first while we're wondering what to do about the problem
> described above.

Thanks Robert for your comments.
I have split the patch into two portions. One that handles DB OID and
the other that
handles tablespace OID and relfilenode OID.

> - There are two places in the patch, one in dumpDatabase() and one in
> generate_old_dump() where blank lines are removed with no other
> changes. Please avoid whitespace-only hunks.

These changes are avoided.

> - If possible, please try to pgindent the new code. It's pretty good
> what you did, but e.g. the declaration of
> binary_upgrade_next_pg_tablespace_oid probably has less whitespace
> than pgindent is going to want.

Taken care in latest patches

> - The comments in dumpDatabase() claim that "postgres" and "template1"
> are handled specially in some way, but there seems to be no code that
> matches those comments.

The comment is removed.

> - heap_create()'s logic around setting create_storage looks slightly
> redundant. I'm not positive what would be better, but ... suppose you
> just took the part that's currently gated by if (!IsBinaryUpgrade) and
> did it unconditionally. Then put if (IsBinaryUpgrade) around the else
> clause, but delete the last bit from there that sets create_storage.
> Maybe we'd still want a comment saying that it's intentional that
> create_storage = true even though it will be overwritten later, but
> then, I think, we wouldn't need to set create_storage in two different
> places. Maybe I'm wrong.
>
> - If we're not going to do that, then I think you should swap the if
> and else clauses and reverse the sense of the test. In createdb(),
> CreateTableSpace(), and a bunch of existing places, we do if
> (IsBinaryUpgrade) { ... } else { ... } so I don't think it makes sense
> for this one to instead do if (!IsBinaryUpgrade) { ... } else { ... }.

I have avoided the redundant code and removed the comment as it does
not make sense now that we are setting the create_storage conditionally.
(In the original patch, create_storage was set to TRUE by default for binary
upgrade case which was wrong and was hitting assert in the following flow).

> - I'm not sure that I'd bother renaming
> binary_upgrade_set_pg_class_oids_and_relfilenodes(). It's such a long
> name, and a relfilenode is kind of an OID, so the current name isn't
> even really wrong. I'd probably drop the header comment too, since it
> seems rather obvious. But both of these things are judgement calls.

I agree. I have retained the old function name.

> - Inside that function, there is a comment that says "Indexes cannot
> have toast tables, so we need not make this probe in the index code
> path." However, you have moved the code from someplace where it didn't
> happen for indexes to someplace where it happens for both tables and
> indexes. Therefore the comment, which was true when the code was where
> it was before, is now false. So you need to update it.

The comment is updated.

> - It is not clear to me why pg_upgrade's Makefile needs to be changed
> to include -DFRONTEND in CPPFLAGS. All of the .c files in this
> directory include postgres_fe.h rather than postgres.h, and that file
> has #define FRONTEND 1. Moreover, there are no actual code changes in
> this directory, so why should the Makefile need any change?

Makefile change is removed.

> - A couple of comment changes - and the commit message - mention data
> encryption, but that's not a feature that this patch implements, nor
> are we committed to adding it in the immediate future (or ever,
> really). So I think those places should be revised to say that we do
> this because we want the filenames to match between the old and new
> clusters, and leave the reasons why that might be a good thing up to
> the reader's imagination.

Taken care.

Regards,
Shruthi KC
EnterpriseDB: http://www.enterprisedb.com