Re: Unhappy about API changes in the no-fsm-for-small-rels patch

On Fri, May 3, 2019 at 2:14 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, May 3, 2019 at 11:43 AM John Naylor <john.naylor@2ndquadrant.com> wrote:
>
> Fair enough.  I think we have tried to come up with a patch for an
> alternative approach, but it needs time.  I will revert this tomorrow.
>

Attached is a revert patch.  John, can you please once double-check to
ensure I have not missed anything?

To summarize for everyone:  This patch avoids the fsm creation for
small relations (which is a small but good improvement as it saves
space).   This patch was using a process local map to track the first
few blocks and was reset as soon as we get the block with enough free
space.   It was discussed in this thread that it would be better to
track the local map in relcache and then invalidate it whenever vacuum
frees up space in the page or when FSM is created.   There is a
prototype patch written for the same, but it is not 100% clear to me
that the new idea would be a win in all cases (like code complexity or
API design-wise) especially because resetting the map is not
straight-forward.  As time was not enough, we couldn't complete the
patch from all aspects to see if it is really better in all cases.

We have two options (a) revert this patch and try the new approach in
next release, (b) keep the current patch and replace with the new
approach if it turns out to be better in next release.

So, do we want to keep this feature for this release?

I am fine going with option (a), that's why I have prepared a revert
patch, but I have a slight fear that the other option might not turn
out to be better and even if it is then we can anyway replace it as
shown in the prototype, so going with option (b) doesn't sound to be
dumb.

Anybody else wants to weigh in?




--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com