Re: Questions/Observations related to Gist vacuum

On Tue, Oct 22, 2019 at 9:10 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 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?

Basically, only IndexBulkDeleteResult is now shared across the stage
so we can move all members to GistVacState and completely get rid of
GistBulkDeleteResult?

IndexBulkDeleteResult *stats
IntegerSet *internal_page_set;
IntegerSet *empty_leaf_set;
MemoryContext page_set_context;

> Also, I think it is better to call gistvacuum_delete_empty_pages from
> function gistvacuumscan as that will avoid it calling from multiple
> places.
Yeah we can do that.
>
> 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?
This was post-cleanup reset since we were returning the gist_stats so
it was better to clean up but now we are not returning it out so I
though we don't need to clean this. But, I think now we can free the
memory gist_stats itself.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com