Re: Questions/Observations related to Gist vacuum

On Fri, Oct 18, 2019 at 4:51 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> I have prepared a first version of the patch. Currently, I am
> performing an empty page deletion for all the cases.
>

Few comments:
----------------------
1.
-/*
- * State kept across vacuum stages.
- */
typedef struct
{
- IndexBulkDeleteResult stats; /* must be first */
+ IndexBulkDeleteResult *stats; /* kept across vacuum stages. */

/*
- * These are used to memorize all internal and empty leaf pages in the 1st
- * vacuum stage. They are used in the 2nd stage, to delete all the empty
- * pages.
+ * These are used to memorize all internal and empty leaf pages. They are
+ * used for deleting all the empty pages.
*/
IntegerSet *internal_page_set;
IntegerSet *empty_leaf_set;

Now, if we don't want to share the remaining stats across
gistbulkdelete and gistvacuumcleanup, isn't it better to keep the
information of internal and empty leaf pages as part of GistVacState?
Also, I think it is better to call gistvacuum_delete_empty_pages from
function gistvacuumscan as that will avoid it calling from multiple
places.

2.
- gist_stats->page_set_context = NULL;
- gist_stats->internal_page_set = NULL;
- gist_stats->empty_leaf_set = NULL;

Why have you removed this initialization?

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