On Fri, Mar 4, 2022 at 12:37 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > In this version I have fixed both of these issues.
Here's a bit of review for these patches: - The whole relnode vs. relfilenode thing is really confusing. I realize that there is some precedent for calling the number that pertains to the file on disk "relnode" and that value when combined with the database and tablespace OIDs "relfilenode," but it's definitely not the most obvious thing, especially since pg_class.relfilenode is a prominent case where we don't even adhere to that convention. I'm kind of tempted to think that we should go the other way and rename the RelFileNode struct to something like RelFileLocator, and then maybe call the new data type RelFileNumber. And then we could work toward removing references to "filenode" and "relfilenode" in favor of either (rel)filelocator or (rel)filenumber. Now the question (even assuming other people like this general direction) is how far do we go with it? Renaming pg_class.relfilenode itself wouldn't be the worst compatibility break we've ever had, but it would definitely cause some pain. I'd be inclined to leave the user-visible catalog column alone and just push in this direction for internal stuff. - What you're doing to pg_buffercache here is completely unacceptable. You can't change the definition of an already-released version of the extension. Please study how such issues have been handled in the past. - It looks to me like you need to give significantly more thought to the proper way of adjusting the relfilenode-related test cases in alter_table.out. - I think BufTagGetFileNode and BufTagGetSetFileNode should be introduced in 0001 and then just update the definition in 0002 as required. Note that as things stand you end up with both BufTagGetFileNode and BuffTagGetRelFileNode which is an artifact of the relnode/filenode/relfilenode confusion I mention above, and just to make matters worse, one returns a value while the other produces an out parameter. I think the renaming I'm talking about up above might help somewhat here, but it seems like it might also be good to change the one that uses an out parameter by doing Get -> Copy, just to help the reader get a clue a little more easily. - GetNewRelNode() needs to error out if we would wrap around, not wrap around. Probably similar to what happens if we exhaust 2^64 bytes of WAL. -- Robert Haas EDB: http://www.enterprisedb.com
