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

On March 30, 2018 6:55:50 PM PDT, Peter Geoghegan <pg@bowt.ie> wrote:
>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.

I'm just saying that there should be two functions here, rather than dropping the old definition, and creating s new
onewith a default argument. 

(Phone, more another time)

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.