Hi Kirk,

On Tue, Nov 05, 2019 at 09:58:22AM +0000, k.jami...@fujitsu.com wrote:
Hi,


Another one that I'd need feedback of is the use of new dlist operations

for this cached buffer list. I did not use in this patch the existing

Postgres dlist architecture (ilist.h) because I want to save memory space

as much as possible especially when NBuffers become large. Both dlist_node

& dlist_head are 16 bytes. OTOH, two int pointers for this patch is 8 bytes.

In cb_dlist_combine(), the code block below can impact performance
especially for cases when the doubly linked list is long (IOW, many cached 
buffers).
             /* Point to the tail of main dlist */
             while (curr_main->next != CACHEDBLOCK_END_OF_LIST)
                           curr_main = cb_dlist_next(curr_main);

Attached is an improved version of the previous patch, which adds a pointer
information of the TAIL field in order to speed up the abovementioned operation.
I stored the tail field in the prev pointer of the head entry (maybe not a 
typical
approach). A more typical one is by adding a tail field (int tail) to 
CachedBufferEnt,
but I didn’t do that because as I mentioned in previous email I want to avoid
using more memory as much as possible.
The patch worked as intended and passed the tests.

Any thoughts?


A couple of comments based on briefly looking at the patch.

1) I don't think you should / need to expose most of the ne stuff in
   buf_internals.h. It's only used from buf_internals.c and having all
   the various cb_dlist_* function in .h seems strange.

2) This adds another hashtable maintenance to BufferAlloc etc. but
   you've only done tests / benchmark for the case this optimizes. I
   think we need to see a benchmark for workload that allocates and
   invalidates lot of buffers. A pgbench with a workload that fits into
   RAM but not into shared buffers would be interesting.

3) I see this triggered a failure on cputube, in the commit_ts TAP test.
   That's a bit strange, someone should investigate I guess.
https://travis-ci.org/postgresql-cfbot/postgresql/builds/607563900

regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply via email to