On Sun, May 5, 2019 at 9:25 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > I understand that we have to take a call here shortly, but as there is > a weekend so I would like to wait for another day to see if anyone > else wants to share his opinion.
I haven't looked deeply into the issues with this patch, but it seems to me that if two other committers is saying that this should be reverted, and the original author of the patch is agreeing, and your patch to try to fix it still has demonstrable bugs ... it's time to give up. We're well past feature freeze at this point. Some other random comments: I'm really surprised that the original design of this patch involved storing state in global variables. That seems like a pretty poor decision. This is properly per-relation information, and any approach like that isn't going to work well when there are multiple relations involved, unless the information is only being used for a single attempt to find a free page, in which case it should use parameters and function-local variables, not globals. I think it's legitimate to question whether sending additional invalidation messages as part of the design of this feature is a good idea. If it happens frequently, it could trigger expensive sinval resets more often. I don't understand the various proposals well enough to know whether that's really a problem, but if you've got a lot of relations for which this optimization is in use, I'm not sure I see why it couldn't be. I think at some point it was proposed that, since an FSM access involves touching 3 blocks, it ought to be fine for any relation of 4 or fewer blocks to just check all the others. I don't really understand why we drifted off that design principle, because it seems like a reasonable theory. Such an approach doesn't require anything in the relcache, any global variables, or an every-other-page algorithm. If we wanted to avoid creating the FSM for relation with >4 blocks, we might want to take a step back and think about a whole different approach. For instance, we could have a cache in shared memory that can store N entries, and not bother creating FSM forks until things no longer fit in that cache. Or, what might be better, we could put FSM data for many relations into a single FSM file, instead of having a separate fork for each relation. I think that would get at what's really driving this work: having a zillion tiny little FSM files sucks. Of course, those kinds of changes are far too big to contemplate for v12, but they might be worth some thought in the future. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
