(2014/10/30 21:30), Fujii Masao wrote:
On Thu, Oct 30, 2014 at 7:30 PM, Etsuro Fujita
<fujita.ets...@lab.ntt.co.jp> wrote:
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.
Thank you for updating 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?
Exactly. But I think we can expect that in many cases, since I think
that the users would often set the GUC to a small value to the extent
that most of the pending list pages would be cached by shared buffer, to
maintain *search* performance.
I'd like to hear the opinions of others about the category for the GUC.
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.
IIUC, I think that min = 0 disables fast update, so ISTM that it'd be
appropriate to set min to some positive value. And ISTM that the idea
of using the min value of work_mem is not so bad.
* 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?
+1
I changed the document in that way.
*** 356,361 **** CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ <replaceable
class="parameter">name</
--- 356,372 ----
</listitem>
</varlistentry>
</variablelist>
+ <variablelist>
+ <varlistentry>
+ <term><literal>PENDING_LIST_CLEANUP_SIZE</></term>
The above is still in uppercse.
Thanks,
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
