Re: POC: Cleaning up orphaned files using undo logs

Hi,

On 2019-08-06 14:18:42 -0700, Andres Freund wrote:
> Here's the last section of my low-leve review. Plan to write a higher
> level summary afterwards, now that I have a better picture of the code.

General comments:

- For a feature of this complexity, there's very little architectural
  documentation. Some individual parts have a bit, but there's basically
  nothing over-arching. That makes it extremely hard for anybody that is
  not already involved to understand the design constraints, and even
  for people involved it's hard to understand.

  I think it's very important for this to have a document that first
  explains what the goals, and non-goals, of this feature are. And then
  secondly explains the chosen architecture referencing those
  constraints.  Once that's there it's a lot easier to review this
  patchset, to discuss the overall architecture, etc.


- There are too many different email threads and git branches. The same
  patches are discussed in different threads, layers exist in slightly
  diverging versions in different git trees. Again making it very hard
  for anybody not primarily focussing on undo to join the discussion.

  I think most of the older git branches should be renamed into
  something indicating their historic status. The remaining branches
  should be referenced from a wiki page (linked to in each submission of
  a new patch version), explaining what they're about.  I don't think
  it's realistic to have people infer meaning from the current branch
  names (undo, proposal-undo-log, undo-log-storage, undo-log-storage-v2,
  undo_interface_v1, undoprocessing).

  Given the size of the the overall project it's quite possibly not
  realistic to manage the whole work in a single git branch. With
  separate git branches, as currently done, it's however hard to
  understand which version of a what layer is used.  I think at the very
  least higher layers need to indicate the version of the underlying
  layers is being used.  I suggest just adding a "v01: " style prefix to
  all commit subjects a branch is rebased onto.

  It's also currently hard to understand what version of a layer is
  being discussed. I think submissions all need to include a version
  number (c.f. git format-patch's -v option), and that version ought to
  be included in the subject line. Each new major version of a patch
  should be started as a reply to the first message of a thread, to
  keep the structure of a discussion in a managable shape. New versions
  should include explicit explanations about the major changes compared
  to the last version.


- The code quality of pretty significant parts of the patchset is not
  even close to being good enough. There are areas with a lot of code
  duplication. There are very few higher level explanations for
  interfaces. There's a lot of "i++; /* increment i to increment it */"
  style comments, but not enough higher level comments. There are
  significant issues with parts of the code that aren't noted anywhere
  in comments, leading to reviewers having to repeatedly re-discover
  them (and wasting time on that).

  There's different naming styles in related code without a discernible
  pattern (e.g. UndoRecordSetInfo being followed by
  get_undo_rec_cid_offset). The word-ordering of different layers is
  confusing (e.g. BeginUndoRecordInsert vs UndoLogBeginInsert vs
  PrepareUndoInsert). Different important externally visible functions
  have names that don't allow to determine which is supposed to do what
  (PrepareUndoInsert vs BeginUndoRecordInsert).


More specific comments:

- The whole sequencing of undo record insertion in combination with WAL
  logging does not appear to be right. It's a bit hard to say, because
  there's very little documentation on what the intended default
  sequence of operations is.

  My understanding is that the currently expected pattern is to:

  1) Collect information / perform work needed to perform the action
     that needs to be UNDO logged. E.g. perform visibility
     determinations, wait for lockers, compute new infomask, etc.

     This will likely end with the "content" page(s) (e.g. a table's
     page) being exclusively locked.

  2) Estimate space needed for all UNDO logging (BeginUndoRecordInsert)

  3) Prepare for each undo record, this includes building the
     content for each undo record. PrepareUndoInsert(). This acquires,
     pins and locks buffers for undo.

  4) begin a critical section

  5) modify content page, mark buffer dirty

  6) write UNDO, using InsertPreparedUndo()

  7) associate undo with WAL record (RegisterUndoLogBuffers)

  8) write WAL

  9) End critical section

  But despite reading through the code, including READMEs, I'm doubtful
  that's quite the intended pattern. It REALLY can't be right that one
  needs to parse many function headers to figure out how the most basic
  use of undo could possibly work.  There needs to be very clear
  documentation about how to write undo records. Functions that sound
  like they're involved, need actually useful comments, rather than just
  restatements of their function names (cf RegisterUndoLogBuffers,
  UndoLogBuffersSetLSN, UndoLogRegister).


- I think there's two fairly fundamental, and related, problems with
  the sequence outlined above:

  - We can't search for target buffers to store undo data, while holding
    the "table" page content locked. That can involve writing out
    multiple pages till we find a usable victim buffer. That can take a
    pretty long time. While that's happening the content page would
    currently be locked. Note how e.g. heapam.c is careful to not hold
    *any* content locks while potentially performing IO.  I think the
    current interface makes that hard.

    The easy way to solve would be to require sites performing UNDO
    logging to acquire victim pages before even acquiring other content
    locks. Perhaps the better approach could be for the undo layer to
    hold onto a number of clean buffers, and to keep the last page in an
    already written to undo log pinned.

  - We can't search for victim buffers for further undo data while
    already holding other undo pages content locked. Doing so means that
    while we're e.g. doing IO to clean out the new page, old undo data
    on the previous page can't be read.

    This seems easier to fix. Instead of PrepareUndoInsert() acquiring,
    pinning and locking buffers, it'd need to be split into two
    operations. One that acquires buffers and pins them, and one that
    locks them.  I think it's quite possible that the locking operation
    could just be delayed until InsertPreparedUndo().  But if we solve
    the above problem, most of this might already be solved.


- To me the current split between the packed and unpacked UNDO record
  formats makes very little sense, the reasoning behind having them is
  poorly if at all documented, results in extremely verbose code, and
  isn't extensible.

  When preparing to insert an undo record the in-buffer size is computed
  with UndoRecordHeaderSize() (needs to know about all optional data)
  from within PrepareUndoInsert() (which has a bunch a bunch of
  additional knowledge about the record format). Then during insertion
  InsertPreparedUndo(), first copies the UnpackedUndoRecord into an
  UndoPackContext (again needing ...), and then, via InsertUndoData(),
  copies that in small increments into the respective buffers (again
  needing knowledge about the complete record format, two copies
  even). Beside the code duplication, that also means the memory copies
  are very inefficient, because they're all done in tiny increments,
  multiple times.

  When reading undo it's smilar: UnpackUndoData(), again in small
  chunks, reads the buffer data into an UndoPackContext (another full
  copy of the unpacked record format). But then FinishUnpackUndo()
  *again* copies all that data, into an actual UnpackedUndoRecord
  (again, with a copy of the record format, albeit slightly different
  looking).

  I'm not convinced by Heikki's argument that we shouldn't have
  structure within undo records. In my opinion that is a significant
  weakness of how WAL was initially designed, and even after Heikki's
  work, still is a problem.  But this isn't the right design either.

  Even if were to stay with the current fixed record format, I think
  the current code needs a substantial redesign:

  - I think 'packing' during insertion needs to serialize into a char*
    allocation during PrepareUndoInsert computing the size in parallel
    (or perhaps in InsertPreparedUndo, but probably not).  The size of
    the record should never be split across record boundaries
    (i.e. we'll leave that space unused if we otherwise would need to
    split the size).  The actual insertion would be a maximally sized
    memcpy() (so we'd as many memcpys as the buffer fits in, rather than
    one for each sub-type of a record).

    That allows to remove most of the duplicated knowledge of the record
    format, and makes insertions faster (by doing only large memcpys
    while holding exclusive content locks).

  - When reading an undo record, the whole stage of UnpackUndoData()
    reading data into a the UndoPackContext is omitted, reading directly
    into the UnpackedUndoRecord. That removes one further copy of the
    record format.

  - To avoid having separate copies of the record format logic, I'd
    probably encode it into *one* array of metadata. If we had
    {offsetoff(UnpackedUndoRecord, member),
     membersize(UnpackedUndoRecord, member),
     flag}
    we could fairly trivially remove most knowledge from the places
    currently knowing about the record format.


  I have some vague ideas for how to specify the format in a way that is
  more extensible, but with more structure than just a blob of data. But
  I don't think they're there yet.


- The interface to read undo also doesn't seem right to me. For one
  there's various different ways to read records, with associated code
  duplication (batch, batch in "one page" mode - but that's being
  removed now I think, single record mode).

  I think the batch mode is too restrictive. We might not need this
  during the first merged version, but I think before long we're going
  to want to be able to efficiently traverse all the undo chains we need
  to determine the visibility of all tuples on a page. Otherwise we'll
  cause a lot of additional synchronous read IO, and will repeatedly
  re-fetch information, especially during sequential scans for an older
  snapshot.  I think I briefly outlined this in an earlier email - my
  current though is that the batch interface (which the non-batch
  interface should just be a tiny helper around), should basically be a
  queue of "to-be-fetched" undo records. When batching reading an entire
  transaction, all blocks get put onto that queue. When traversing
  multiple chains, the chains are processed in a breadth-first fashion
  (by just looking at the queue, and pushing additional work to the
  end). That allows to efficiently issue prefetch requests for blocks to
  be read in the near future.

  I think that batch reading should just copy the underlying data into a
  char* buffer. Only the records that currently are being used by
  higher layers should get exploded into an unpacked record. That will
  reduce memory usage quite noticably (and I suspect it also drastically
  reduce the overhead due to a large context with a lot of small
  allocations that then get individually freed).  That will make the
  sorting of undo a bit more CPU inefficient, because individual records
  will need to be partially unpacked for comparison, but I think that's
  going to be a far smaller loss than the win.


- My reading of the current xact.c integration is that it's not workable
  as is. Undo is executed outside of a valid transaction state,
  exceptions aren't properly undone, logic would need to be duplicated
  to a significant degree, new kind of critical section.


Greetings,

Andres Freund