Re: PENDING_LIST_CLEANUP_SIZE - maximum size of GIN pending list Re: HEAD seems to generate larger WAL regarding GIN index

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