> Is the point 3 change related to pgindent? I think even if you want
> these, then don't prepare other patches on top of this, keep it
> entirely separate.
Both removed.
>> Also, we don't quite have a consensus on the threshold value, but I
>> have set it to 4 pages for v8. If this is still considered too
>> expensive (and basic tests show it shouldn't be), I suspect it'd be
>> better to interleave the available block numbers as described a couple
>> days ago than lower the threshold further.
>>
>
> Can you please repeat the copy test you have done above with
> fillfactor as 20 and 30?
I will send the results in a separate email soon.
> Few more comments:
> -------------------------------
> 1. I think we can add some test(s) to test the new functionality, may
> be something on the lines of what Robert has originally provided as an
> example of this behavior [1].
Done. I tried adding it to several schedules, but for some reason
vacuuming an empty table failed to truncate the heap to 0 blocks.
Putting the test in its own group fixed the problem, but that doesn't
seem ideal.
> 2.
> The similar call is required in AbortSubTransaction function as well.
> I suggest to add it after pgstat_progress_end_command in both
> functions.
Done.
> 3.
>> agree we shouldn't change that without a reason to. One simple idea is
>> add a 3rd boolean parameter to GetPageWithFreeSpace() to control
>> whether it gives up if the FSM fork doesn't indicate free space, like
> I have the exact fix in my mind, so let's do it that way.
Done. This also reverts comments and variable names that referred to
updating the local map after relation extension.
While at it, I changed a couple conditionals to check the locally
cached nblocks rather than the threshold. No functional change, but
looks more precise. Might save a few cycles as well.
>> > 5. Your logic to update FSM on standby seems okay, but can you show
>> > some tests which proves its sanity?
>>
>> I believe to convince myself it was working, I used the individual
>> commands in the sql file in [3], then used the size function on the
>> secondary. I'll redo that to verify.
I've verified the standby behaves precisely as the primary, as far as
the aforementioned script goes.
-John Naylor