Re: PENDING_LIST_CLEANUP_SIZE - maximum size of GIN pending list Re: HEAD seems to generate larger WAL regarding GIN index - Mailing list pgsql-hackers
From | Fujii Masao |
---|---|
Subject | Re: PENDING_LIST_CLEANUP_SIZE - maximum size of GIN pending list Re: HEAD seems to generate larger WAL regarding GIN index |
Date | |
Msg-id | CAHGQGwFGooZ7X_WMTAjNWaaLUSYop0xNn8=6xnN56QnqXWCwjg@mail.gmail.com Whole thread Raw |
In response to | Re: PENDING_LIST_CLEANUP_SIZE - maximum size of GIN pending list Re: HEAD seems to generate larger WAL regarding GIN index (Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>) |
Responses |
Re: PENDING_LIST_CLEANUP_SIZE - maximum size of GIN pending list
Re: HEAD seems to generate larger WAL regarding GIN index
Re: PENDING_LIST_CLEANUP_SIZE - maximum size of GIN pending list Re: HEAD seems to generate larger WAL regarding GIN index |
List | pgsql-hackers |
On Thu, Oct 30, 2014 at 7:30 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp> wrote: > (2014/10/09 11:49), Etsuro Fujita wrote: >> >> (2014/10/08 22:51), Fujii Masao wrote: >>> >>> On Wed, Sep 24, 2014 at 11:10 AM, Etsuro Fujita >>> <fujita.etsuro@lab.ntt.co.jp> wrote: >>>>> >>>>> On Wed, Sep 10, 2014 at 10:37 PM, Alvaro Herrera >>>>> <alvherre@2ndquadrant.com> wrote: >>>>>> >>>>>> Fujii Masao wrote: >>>>>>> >>>>>>> On Wed, Sep 10, 2014 at 12:15 PM, Etsuro Fujita >>>>>>> <fujita.etsuro@lab.ntt.co.jp> wrote: > > >>>>>>>> PENDING_LIST_CLEANUP_SIZE and work_mem, for this setting. >>>>>>>> Wouldn't it be easy-to-use to have only one parameter, >>>>>>>> PENDING_LIST_CLEANUP_SIZE? How about setting >>>>>>>> PENDING_LIST_CLEANUP_SIZE >>>>>>>> to >>>>>>>> work_mem as the default value when running the CREATE INDEX command? > > >>>>>>> So what about introduing pending_list_cleanup_size also as GUC? >>>>>>> That is, users can set the threshold by using either the reloption or >>>>>>> GUC. > > >>>>>> Yes, I think having both a GUC and a reloption makes sense -- the GUC >>>>>> applies to all indexes, and can be tweaked for individual indexes >>>>>> using >>>>>> the reloption. > > >>>> OK, I'd vote for your idea of having both the GUC and the reloption. >>>> So, I >>>> think the patch needs to be updated. Fujii-san, what plan do you >>>> have about >>>> the patch? > > >>> Please see the attached patch. In this patch, I introduced the GUC >>> parameter, >>> pending_list_cleanup_size. I chose 4MB as the default value of the >>> parameter. >>> But do you have any better idea about that default value? > > >> It seems reasonable to me that the GUC has the same default value as >> work_mem. So, +1 for the default value of 4MB. > > >>> BTW, I moved the CommitFest entry of this patch to next CF 2014-10. > > >> OK, I'll review the patch in the CF. > > > Thank you for updating the patch! Here are my review comments. > > * The patch applies cleanly and make and make check run successfully. I > think that the patch is mostly good. Thanks! Attached is the updated version of the patch. > * In src/backend/utils/misc/guc.c: > + { > + {"pending_list_cleanup_size", PGC_USERSET, > CLIENT_CONN_STATEMENT, > + gettext_noop("Sets the maximum size of the pending > list for GIN index."), > + NULL, > + GUC_UNIT_KB > + }, > + &pending_list_cleanup_size, > + 4096, 0, MAX_KILOBYTES, > + NULL, NULL, NULL > + }, > > ISTM it'd be better to use RESOURCES_MEM, not CLIENT_CONN_STATEMENT. No? Yes if the pending list always exists in the memory. But not, IIUC. Thought? > Also why not set min to 64, not to 0, in accoradance with that of work_mem? I'm OK to use 64. But I just chose 0 because I could not think of any reasonable reason why 64k is suitable as the minimum size of the pending list. IOW, I have no idea about whether it's reasonable to use the min value of work_mem as the min size of the pending list. > * In src/backend/utils/misc/postgresql.conf.sample: > Likewise, why not put this variable in the section of RESOURCE USAGE, not in > that of CLIENT CONNECTION DEFAULTS. Same as above. > > * In src/backend/access/common/reloptions.c: > + { > + { > + "pending_list_cleanup_size", > + "Maximum size of the pending list for this GIN > index, in kilobytes.", > + RELOPT_KIND_GIN > + }, > + -1, 0, MAX_KILOBYTES > + }, > > As in guc.c, why not set min to 64, not to 0? Same as above. > * In src/include/access/gin.h: > /* GUC parameter */ > extern PGDLLIMPORT int GinFuzzySearchLimit; > + extern int pending_list_cleanup_size; > > The comment should be "GUC parameters". Yes, fixed. > * In src/backend/access/gin/ginfast.c: > + /* GUC parameter */ > + int pending_list_cleanup_size = 0; > > Do we need to substitute 0 for pending_list_cleanup_size? Same as above. > > * In doc/src/sgml/config.sgml: > + <varlistentry id="guc-pending-list-cleanup-size" > xreflabel="pending_list_cleanup_size"> > + <term><varname>pending_list_cleanup_size</varname> > (<type>integer</type>) > > As in postgresql.conf.sample, ISTM it'd be better to explain this variable > in the section of Resource Consumption (maybe in "Memory"), not in that of > Client Connection Defaults. Same as above. > * In doc/src/sgml/gin.sgml: > ! <literal>pending_list_cleanup_size</>. To avoid fluctuations in > observed > > ISTM it'd be better to use <varname> for pending_list_cleanup_size, not > <literal>, here. Yes, fixed. > * In doc/src/sgml/ref/create_index.sgml: > + <term><literal>PENDING_LIST_CLEANUP_SIZE</></term> > > IMHO, it seems to me better for this variable to be in lowercase in > accordance with the GUC version. Using lowercase only for pending_list_cleanup_size and uppercase for other options looks strange to me. What about using lowercase for all the storage options? I changed the document in that way. > Also, I think that the words "GIN indexes > accept a different parameter:" in the section of "Index Storage Parameters" > in this reference page would be "GIN indexes accept different parameters:". Yes, fixed. > Sorry for the delay in reviewing the patch. No problem. Thanks! Regards, -- Fujii Masao
Attachment
pgsql-hackers by date: