Re: generalized conveyor belt storage - Mailing list pgsql-hackers

On Wed, Dec 15, 2021 at 9:04 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Dec 15, 2021 at 10:03 AM Matthias van de Meent
> <boekewurm+postgres@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?
--

+   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 ?
--

+   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()
--

+/*
+ * 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.
--

+++ b/src/backend/access/conveyor/cbxlog.c
@@ -0,0 +1,442 @@
+/*-------------------------------------------------------------------------
+ *
+ * cbxlog.c
+ *   XLOG support for conveyor belts.
+ *
+ * For each REDO function in this file, see cbmodify.c for the
+ * corresponding function that performs the modification during normal
+ * running and logs the record that we REDO here.
+ *
+ * Copyright (c) 2016-2021, PostgreSQL Global Development Group
+ *
+ * src/backend/access/conveyor/cbmodify.c

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

+           can_allocate_segment =
+               (free_segno_first_blkno < possibly_not_on_disk_blkno)

The condition should be '<=' ?
--

+ * ConveyorBeltPhysicalTruncate. For more aggressive cleanup options, see
+ * ConveyorBeltCompact or ConveyorBeltRewrite.

Didn't find ConveyorBeltCompact or ConveyorBeltRewrite code, might yet
to be implemented?
--

+ * the value computed here here if the entry at that offset is already

"here" twice.
--

And few typos:

othr
semgents
fucntion
refrenced
initialied
remve
extrordinarily
implemenation
--

Regards,
Amul