Re: generalized conveyor belt storage

On Wed, Dec 29, 2021 at 7:08 AM Amul Sul <sulamul@gmail.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.

> +   if (meta->cbm_index[offset] != offset)
> +       elog(ERROR,
> +            "index entry at offset %u was expected to be %u but found %u",
> +            offset, segno, meta->cbm_index[offset]);
>
> IF condition should be : meta->cbm_index[offset] != segno ?

Oops, you're right.

> +   if (segno >= CB_METAPAGE_FREESPACE_BYTES * BITS_PER_BYTE)
> +       elog(ERROR, "segment %u out of range for metapage fsm", segno);
>
> I think CB_FSM_SEGMENTS_FOR_METAPAGE should be used like
> cb_metapage_set_fsm_bit()

Good call.

> +/*
> + * Increment the count of segments allocated.
> + */
> +void
> +cb_metapage_increment_next_segment(CBMetapageData *meta, CBSegNo segno)
> +{
> +   if (segno != meta->cbm_next_segment)
> +       elog(ERROR, "extending to create segment %u but next segment is %u",
> +            segno, meta->cbm_next_segment);
>
> I didn't understand this error, what does it mean?  It would be
> helpful to add a brief about what it means and why we are throwing it
> and/or rephrasing the error bit.

cbm_next_segment is supposed to be the lowest-numbered segment that
doesn't yet exist. Imagine that it's 4. But, suppose that the free
space map shows segment 4 as in use, even though the metapage's
cbm_next_segment value claims it's not allocated yet. Then maybe we
decide that the lowest-numbered free segment according to the
freespace map is 5, while meanwhile the metapage is pretty sure we've
never created 4. Then I think we'll end up here and trip over this
error check. To get more understanding of this, look at how
ConveyorBeltGetNewPage selects free_segno.

> Incorrect file name: s/cbmodify.c/cbxlog.c/

Right, thanks.

> +           can_allocate_segment =
> +               (free_segno_first_blkno < possibly_not_on_disk_blkno)
>
> The condition should be '<=' ?

It doesn't look that way to me. If they were equal, then that block
doesn't necessarily exist on disk ... in which case we should not
allocate. Am I missing something?

> + * ConveyorBeltPhysicalTruncate. For more aggressive cleanup options, see
> + * ConveyorBeltCompact or ConveyorBeltRewrite.
>
> Didn't find ConveyorBeltCompact or ConveyorBeltRewrite code, might yet
> to be implemented?

Right.

> + * the value computed here here if the entry at that offset is already
>
> "here" twice.

Oops.

> And few typos:
>
> othr
> semgents
> fucntion
> refrenced
> initialied
> remve
> extrordinarily
> implemenation

Wow, OK, that's a lot of typos.

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

-- 
Robert Haas
EDB: http://www.enterprisedb.com