Re: [HACKERS] [PATCH] Vacuum: Update FSM more frequently

On Wed, Feb 28, 2018 at 12:06 AM, Claudio Freire <klaussfreire(at)gmail(dot)com> wrote:
> On Mon, Feb 26, 2018 at 10:20 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> Thank you for updating patches!
>>
>> 0001 patch looks good to me except for the following unnecessary empty lines.
>>
>> + * If there are no indexes then we should periodically
>> vacuum the FSM
>> + * on huge relations to make free space visible early.
>> + */
>> + else if (nindexes == 0 && fsm_updated_pages >
>> vacuum_fsm_every_pages)
>> + {
>> + /* Vacuum the Free Space Map */
>> + FreeSpaceMapVacuum(onerel, max_freespace);
>> + fsm_updated_pages = 0;
>> + max_freespace = 0;
>> + }
>> +
>> +
>> + /*
>>
>> @@ -913,10 +967,14 @@ lazy_scan_heap(Relation onerel, int options,
>> LVRelStats *vacrelstats,
>>
>> vmbuffer, InvalidTransactionId,
>>
>> VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN);
>> END_CRIT_SECTION();
>> +
>> }
>>
>> 0002 patch looks good to me.
>>
>> For 0003 patch, I think fsm_set() function name could lead misreading
>> because it actually not only sets the value but also returns the
>> value. fsm_set_and_get() might be better?
>
> Attached is the patchset modified as requested

Thank you for updating the patches!

+/*
+ * When a table has no indexes, vacuum the FSM at most every this
+ * many dirty pages. With a default page size of 8kb, this value
+ * basically means 8GB of dirtied pages.
+ */
+#define VACUUM_FSM_EVERY_PAGES 1048576

Is it better if we vacuum fsm every 8GB regardless of the block size?
Because an installation that uses >8GB block size is likely to have
the pages less than what an 8GB block size installation has, the
current patch might lead to delay fsm vacuum. What do you think? If
it's better, we can define it as follows.

#define VACUUM_FSM_EVERY_PAGES ((8 * 1024 * 1024) / BLCKSZ)

--
@@ -470,7 +484,9 @@ lazy_scan_heap(Relation onerel, int options,
LVRelStats *vacrelstats,
TransactionId relfrozenxid = onerel->rd_rel->relfrozenxid;
TransactionId relminmxid = onerel->rd_rel->relminmxid;
BlockNumber empty_pages,
- vacuumed_pages;
+ vacuumed_pages,
+ fsm_updated_pages,
+ vacuum_fsm_every_pages;
double num_tuples,
tups_vacuumed,
nkeep,

Regarding fsm_updated_pages variable name, I think it's better to be
freespace_updated_pages because we actually counts the page updated
its freespace, not fsm.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center