Re: Make relfile tombstone files conditional on WAL level - Mailing list pgsql-hackers

On Fri, Mar 4, 2022 at 12:37 AM Dilip Kumar <dilipbalaut@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