Re: [HACKERS] A design for amcheck heapam verification - Mailing list pgsql-hackers

On Fri, Mar 30, 2018 at 6:20 PM, Andres Freund <andres@anarazel.de> wrote:
>> What would the upcasting you have in mind look like?
>
> Just use UINT64CONST()?  Let's try not to introduce further code that'll
> need to get painfully fixed.

What I have right now is based on imitating the style that Tom uses.
I'm pretty sure that I did something like that in the patch I posted
that became 9563d5b5, which Tom editorialized to be in
"maintenance_work_mem * 1024L" style. That was only about 2 years ago.

I'll go ahead and use UINT64CONST(), as requested, but I wish that the
messaging on the right approach to such a standard question of style
was not contradictory.

> Well, they're not supposed to be, but they are. Practical realities
> matter...  But I think it's moot because we don't use any of the bad
> ones, I only got to that later...

Okay. My point was that that's not really the right level to solve the
problem (if that was a problem we had, which it turns out it isn't).
Not that practical realities don't matter.

>> This sounds like a general argument against ever changing a function's
>> signature. It's not like we haven't done that a number of times in
>> extensions like pageinspect. Does it really matter?
>
> Yes, it does. And imo we shouldn't.

My recollection is that you didn't like the original bt_index_check()
function signature in the final v10 CF because it didn't allow you to
add arbitrary new arguments in the future; the name was too
prescriptive to support that. You wanted to only have one function
signature, envisioning a time when there'd be many arguments, that
you'd typically invoke using the named function argument (=>) syntax,
since many arguments may not actually be interesting, depending on the
exact details. I pushed back specifically because I thought there
should be simple rules for the heavyweight lock strength --
bt_index_check() should always acquire an ASL and only an ASL, and so
on. I also thought that we were unlikely to need many more options
that specifically deal with B-Tree indexes.

You brought this up again recently, recalling that my original
preferred signature style (the one that we went with) was bad because
it now necessitates altering a function signature to add a new
argument. I must admit that I am rather confused. Weren't *you* the
one that wanted to add lots of new arguments in the future? As I said,
I'm sure that I'm done adding new arguments to bt_index_check() +
bt_index_parent_check(). It's possible that there'll be another way to
get essentially the same functionality at a coarser granularity (e.g.
database, table), certainly, but I don't see that there is much more
that we can do while starting with a B-Tree index as our target.

Please propose an alternative user interface for the new check. If you
prefer, I can invent new bt_index_check_heap() +
bt_index_parent_check_heap() variants. That would be okay with me.

-- 
Peter Geoghegan