In v21, all of your v20 improvements to the radix tree template and test have been squashed into 0003, with one exception: v20-0010 (recursive freeing of shared mem), which I've attached separately (for flexibility) as v21-0006. I believe one of your earlier patches had a new DSA function for freeing memory more quickly -- was there a problem with that approach? I don't recall where that discussion went.
> + * XXX: Most functions in this file have two variants for inner nodes and leaf
> + * nodes, therefore there are duplication codes. While this sometimes makes the
> + * code maintenance tricky, this reduces branch prediction misses when judging
> + * whether the node is a inner node of a leaf node.
> This comment seems to be out-of-date since we made it a template.
Done in 0020, along with a bunch of other comment editing.
Fixed in v21-0018.
Also:
0007 makes the value type configurable. Some debug functionality still assumes integer type, but I think the rest is agnostic.
0010 turns node4 into node3, as discussed, going from 48 bytes to 32.
0012 adopts the benchmark module to the template, and adds meson support (builds with warnings, but okay because not meant for commit).
The rest are cleanups, small refactorings, and more comment rewrites. I've kept them separate for visibility. Next patch can squash them unless there is any discussion.
> > uint32 is how we store the block number, so this too small and will wrap around on overflow. int64 seems better.
>
> Agreed, will fix.
Great, but it's now uint64, not int64. All the large counters in struct LVRelState, for example, are signed integers, as the usual practice. Unsigned ints are "usually" for things like bit patterns and where explicit wraparound is desired. There's probably more that can be done here to change to signed types, but I think it's still a bit early to get to that level of nitpicking. (Soon, I hope :-) )
> > + * We calculate the maximum bytes for the TidStore in different ways
> > + * for non-shared case and shared case. Please refer to the comment
> > + * TIDSTORE_MEMORY_DEDUCT for details.
> > + */
> >
> > Maybe the #define and comment should be close to here.
>
> Will fix.
For this, I intended that "here" meant "in or just above the function".
+#define TIDSTORE_LOCAL_MAX_MEMORY_DEDUCT (1024L * 70) /* 70kB */
+#define TIDSTORE_SHARED_MAX_MEMORY_RATIO_PO2 (float) 0.75
+#define TIDSTORE_SHARED_MAX_MEMORY_RATIO (float) 0.6
These symbols are used only once, in tidstore_create(), and are difficult to read. That function has few comments. The symbols have several paragraphs, but they are far away. It might be better for readability to just hard-code numbers in the function, with the explanation about the numbers near where they are used.
> > + * Destroy a TidStore, returning all memory. The caller must be certain that
> > + * no other backend will attempt to access the TidStore before calling this
> > + * function. Other backend must explicitly call tidstore_detach to free up
> > + * backend-local memory associated with the TidStore. The backend that calls
> > + * tidstore_destroy must not call tidstore_detach.
> > + */
> > +void
> > +tidstore_destroy(TidStore *ts)
> >
> > If not addressed by next patch, need to phrase comment with FIXME or TODO about making certain.
>
> Will fix.
Did anything change here? There is also this, in the template, which I'm not sure has been addressed:
* XXX: Currently we allow only one process to do iteration. Therefore, rt_node_iter
* has the local pointers to nodes, rather than RT_PTR_ALLOC.
* We need either a safeguard to disallow other processes to begin the iteration
* while one process is doing or to allow multiple processes to do the iteration.
> > This part only runs "if (vacrel->nindexes == 0)", so seems like unneeded complexity. It arises because lazy_scan_prune() populates the tid store even if no index vacuuming happens. Perhaps the caller of lazy_scan_prune() could pass the deadoffsets array, and upon returning, either populate the store or call lazy_vacuum_heap_page(), as needed. It's quite possible I'm missing some detail, so some description of the design choices made would be helpful.
>
> I agree that we don't need complexity here. I'll try this idea.
Keeping the offsets array in the prunestate seems to work out well.
Some other quick comments on tid store and vacuum, not comprehensive. Let me know if I've misunderstood something:
TID store:
+ * XXXXXXXX XXXYYYYY YYYYYYYY YYYYYYYY YYYYYYYY YYYuuuu
+ *
+ * X = bits used for offset number
+ * Y = bits used for block number
+ * u = unused bit
I was confused for a while, and I realized the bits are in reverse order from how they are usually pictured (high on left, low on the right).
+ * 11 bits enough for the offset number, because MaxHeapTuplesPerPage < 2^11
+ * on all supported block sizes (TIDSTORE_OFFSET_NBITS). We are frugal with
+ * XXX: if we want to support non-heap table AM that want to use the full
+ * range of possible offset numbers, we'll need to reconsider
+ * TIDSTORE_OFFSET_NBITS value.
Would it be worth it (or possible) to calculate constants based on compile-time block size? And/or have a fallback for other table AMs? Since this file is in access/common, the intention is to allow general-purpose, I imagine.
+typedef dsa_pointer tidstore_handle;
It's not clear why we need a typedef here, since here:
+tidstore_attach(dsa_area *area, tidstore_handle handle)
+{
+ TidStore *ts;
+ dsa_pointer control;
...
+ control = handle;
...there is a differently-named dsa_pointer variable that just gets the function parameter.
+/* Return the maximum memory TidStore can use */
+uint64
+tidstore_max_memory(TidStore *ts)
size_t is more suitable for memory.
+ /*
+ * Since the shared radix tree supports concurrent insert,
+ * we don't need to acquire the lock.
+ */
Hmm? IIUC, the caller only acquires the lock after returning from here, to update statistics. Why is it safe to insert with no lock? Am I missing something?
VACUUM integration:
-#define PARALLEL_VACUUM_KEY_DEAD_ITEMS 2
+#define PARALLEL_VACUUM_KEY_DSA 2
Seems like unnecessary churn? It is still all about dead items, after all. I understand using "DSA" for the LWLock, since that matches surrounding code.
+#define HAS_LPDEAD_ITEMS(state) (((state).lpdead_items) > 0)
This macro helps the patch readability in some places, but I'm not sure it helps readability of the file as a whole. The following is in the patch and seems perfectly clear without the macro:
- if (lpdead_items > 0)
+ if (prunestate->lpdead_items > 0)
About shared memory: I have some mild reservations about the naming of the "control object", which may be in shared memory. Is that an established term? (If so, disregard the rest): It seems backwards -- the thing in shared memory is the actual tree itself. The thing in backend-local memory has the "handle", and that's how we control the tree. I don't have a better naming scheme, though, and might not be that important. (Added a WIP comment)
Now might be a good time to look at earlier XXX comments and come up with a plan to address them.
That's all I have for now.
--
John Naylor
EDB:
http://www.enterprisedb.com
