On Fri, Mar 30, 2018 at 6:20 PM, Andres Freund <and...@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