Re: generalized conveyor belt storage

On Wed, Jan 5, 2022 at 1:12 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Wed, Dec 29, 2021 at 7:08 AM Amul Sul <sulamul(at)gmail(dot)com> wrote:
> > Thought patch is WIP, here are a few comments that I found while
> > reading the patch and thought might help:
> >
> > + {
> > + if (meta->cbm_oldest_index_segment ==
> > + meta->cbm_newest_index_segment)
> > + elog(ERROR, "must remove last index segment when only one remains");
> > + meta->cbm_oldest_index_segment = segno;
> > + }
> >
> > How about having to assert or elog to ensure that 'segno' is indeed
> > the successor?
>
> This code doesn't have any inexpensive way to know whether segno is
> the successor. To figure that out you'd need to look at the latest
> index page that does exist, but this function's job is just to look at
> the metapage. Besides, the eventual caller will have just looked up
> that value in order to pass it to this function, so double-checking
> what we just computed right before doesn't really make sense.
>

Ok.

Assert(meta->cbm_oldest_index_segment < segno) was in my mind since
the segment to be removed is between meta->cbm_oldest_index_segment
and segno, IIUC.

> [...]
> Updated patch attached, also with a fix for the mistake in the readme
> that Matthias found, and a few other bug fixes.
>

Few more comments for this version:

+void
+cb_cache_invalidate(CBCache *cache, CBPageNo index_start,
+ uint64 index_segments_moved)
+{
+ if (index_segments_moved != cache->index_segments_moved)
+ {
+ cb_iseg_reset(cache->iseg);
+ cache->index_segments_moved = index_segments_moved;
+ }
+ else if (index_start > cache->oldest_possible_start)
+ {
+ cb_iseg_iterator it;
+ cb_iseg_entry *entry;
+
+ cb_iseg_start_iterate(cache->iseg, &it);
+ while ((entry = cb_iseg_iterate(cache->iseg, &it)) != NULL)
+ if (entry->index_segment_start < index_start)
+ cb_iseg_delete_item(cache->iseg, entry);
+ }
+}

Shouldn't update oldest_possible_start, once all the entries preceding
the index_start are deleted?
--

+CBSegNo
+cbfsmpage_find_free_segment(Page page)
+{
+ CBFSMPageData *fsmp = cb_fsmpage_get_special(page);
+ unsigned i;
+ unsigned j;
+
+ StaticAssertStmt(CB_FSMPAGE_FREESPACE_BYTES % sizeof(uint64) == 0,
+ "CB_FSMPAGE_FREESPACE_BYTES should be a multiple of 8");
+

I am a bit confused about this assertion, why is that so?
Why should CB_FSMPAGE_FREESPACE_BYTES be multiple of 8?
Do you mean CB_FSM_SEGMENTS_PER_FSMPAGE instead of CB_FSMPAGE_FREESPACE_BYTES?
--

+/*
+ * Add index entries for logical pages beginning at 'pageno'.
+ *
+ * It is the caller's responsibility to supply the correct index
page, and
+ * to make sure that there is enough room for the entries to be added.
+ */
+void
+cb_indexpage_add_index_entries(Page page,
+ unsigned pageoffset,
+ unsigned num_index_entries,
+ CBSegNo *index_entries)

The first comment line says "...logical pages beginning at 'pageno'",
there is nothing 'pageno' in that function, does it mean pageoffset?
--

+ * Sets *pageno to the first logical page covered by this index page.
+ *
+ * Returns the segment number to which the obsolete index entry points.
+ */
+CBSegNo
+cb_indexpage_get_obsolete_entry(Page page, unsigned *pageoffset,
+ CBPageNo *first_pageno)
+{
+ CBIndexPageData *ipd = cb_indexpage_get_special(page);
+
+ *first_pageno = ipd->cbidx_first_page;
+
+ while (*pageoffset < CB_INDEXPAGE_INDEX_ENTRIES &&
+ ipd->cbidx_entry[*pageoffset] != CB_INVALID_SEGMENT)
+ ++*pageoffset;
+
+ return ipd->cbidx_entry[*pageoffset];
+}

Here I think *first_pageno should be instead of *pageno in the comment
line. The second line says "Returns the segment number to which the
obsolete index entry points." I am not sure if that is correct or I
might have misunderstood this? Look like function returns
CB_INVALID_SEGMENT or ipd->cbidx_entry[CB_INDEXPAGE_INDEX_ENTRIES]
value which could be a garbage value due to out-of-bound memory
access.
--

+/*
+ * Copy the indicated number of index entries out of the metapage.
+ */
+void
+cb_metapage_get_index_entries(CBMetapageData *meta, unsigned num_index_entries,
+ CBSegNo *index_entries)
+{
+ Assert(num_index_entries <= cb_metapage_get_index_entries_used(meta));

IMO, having elog instead of assert would be good, like
cb_metapage_remove_index_entries() has an elog for (used < count).
--

+ * Copyright (c) 2016-2021, PostgreSQL Global Development Group

This rather is a question for my knowledge, why this copyright year
starting from 2016, I thought we would add the current year only for the
new file to be added.

Regards,
Amul