Re: POC: Cleaning up orphaned files using undo logs

On Wed, Aug 21, 2019 at 3:55 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> I have already attempted that part and I feel it is not making code
> any simpler than what we have today.  For packing, it's fine because I
> can process all the member once and directly pack it into one memory
> chunk and I can insert that to the buffer by one call of
> InsertUndoBytes and that will make the code simpler.

OK...

> But, while unpacking if I directly unpack to the UnpackUndoRecord then
> there are few complexities.  I am not saying those are difficult to
> implement but code may not look better.
>
> a) First, we need to add extra stages for unpacking as we need to do
> field by field.
>
> b) Some of the members like uur_payload and uur_tuple are not the same
> type in the UnpackUndoRecord compared to how it is stored in the page.
> In UnpackUndoRecord those are StringInfoData whereas on the page we
> store it as UndoRecordPayload header followed by the actual data.  I
> am not saying we can not unpack this directly we can do it like,
> first read the payload length from the page in uur_payload.len then
> read tuple length in uur_tuple.len then read both the data.  And, for
> that, we will have to add extra stages.

I don't think that this is true; or at least, I think it's avoidable.
The idea of an unpacking stage is that you refuse to advance to the
next stage until you've got a certain number of bytes of data; and
then you unpack everything that pertains to that stage.  If you have 2
4-byte fields that you want to unpack together, you can just wait
until you've 8 bytes of data and then unpack both.  You don't really
need 2 separate stages.  (Similarly, your concern about fields being
in a different order seems like it should be resolved by agreeing on
one ordering and having everything use it; I don't know why there
should be one order that is better in memory and another order that is
better on disk.)

The bigger issue here is that we don't seem to be making very much
progress toward improving the overall design.  Several significant
improvements have been suggested:

1. Thomas suggested quite some time ago that we should make sure that
the transaction header is the first optional header.  If we do that,
then I'm not clear on why we even need this incremental unpacking
stuff any more. The only reason for all of this machinery was so that
we could find the transaction header at an unknown offset inside a
complex record format; there is, if I understand correctly, no other
case in which we want to incrementally decode a record. But if the
transaction header is at a fixed offset, then there seems to be no
need to even have incremental decoding at all.  Or it can be very
simple, with three stages: (1) we don't yet have enough bytes to
figure out how big the record is; (2) we have enough bytes to figure
out how big the record is and we have figured that out but we don't
yet have all of those bytes; and (3) we have the whole record, we can
decode the whole thing and we're done.

2. Based on a discussion with Thomas, I suggested the GHOB stuff,
which gets rid of the idea of transaction headers inside individual
records altogether; instead, there is one undo blob per transaction
(or maybe several if we overflow to another undo log) which begins
with a sentinel byte that identifies it as owned by a transaction, and
then the transaction header immediately follows that without being
part of any record, and the records follow that data.  As with the
previous idea, this gets rid of the need for incremental decoding
because it gets rid of the need to find the transaction header inside
of a bigger record. As Thomas put it to me off-list, it puts the
records inside of a larger chunk of space owned by the transaction
instead of putting the transaction header inside of some particular
record; that seems more correct than the way we have it now.

3. Heikki suggested that the format should be much more generic and
that more should be left up to the AM.  While neither Andres nor I are
convinced that we should go as far in that direction as Heikki is
proposing, the idea clearly has some merit, and we should probably be
moving that way to some degree. For instance, the idea that we should
store a block number and TID is a bit sketchy when you consider that a
multi-insert operation really wants to store a TID list. The zheap
tree has a ridiculous kludge to work around that problem; clearly we
need something better.  We've also mentioned that, in the future, we
might want to support TIDs that are not 6 bytes, and that even just
looking at what's currently under development, zedstore wants to treat
TIDs as 48-bit opaque quantities, not a 4-byte block number and a
2-byte item pointer offset.  So, there is clearly a need to go through
the whole way we're doing this and rethink which parts are generic and
which parts are AM-specific.

4. A related problem, which has been mentioned or at least alluded to
by both Heikki and by me, is that we need a better way of handling the
AM-specific data. Right now, the zheap code packs fixed-size things
into the payload data and then finds them by knowing the offset where
any particular data is within that field, but that's an unmaintainable
mess.  The zheap code could be improved by at least defining those
offsets as constants someplace and adding some comments explaining the
payload formats of various undo records, but even if we do that, it's
not going to generalize very well to anything more complicated than a
few fixed-size bits of data.  I suggested using the pqformat stuff to
try to structure that -- a suggestion to which Heikki has
unfortunately not responded, because I'd really like to get his
thoughts on it -- but whether we do that particular thing or not, I
think we need to do something.  In addition to wanting a better way of
handling packing and unpacking for payload data, there's also a desire
to have it participate in record compression, for which we don't seem
to have any kind of plan.

5. Andres suggested multiple ideas for cleaning up and improving this
code in https://www.postgresql.org/message-id/20190814065745.2faw3hirvfhbrdwe%40alap3.anarazel.de
- which include the idea currently under discussion, several of the
same ideas that I mentioned above, and a number of other things, such
as making packing serialize to a char * rather than some ad-hoc
intermediate format and having a metadata array over which we can loop
rather than having multiple places where there's a separate bit of
code for every field type.  I don't think those suggestions are
entirely unproblematic; for instance, the metadata array would would
probably work a lot better if we moved the transaction and log-switch
headers outside of individual records as suggested in (2) above.
Otherwise, the metadata would have to include not only data-structure
offsets but some kind of a flag indicating which of several data
structures ought to contain the relevant information, which would make
the whole thing a lot messier. And depending on what we do about (4),
this might become moot or the details might change quite a bit,
because if we no longer have a long list of "generic" fields, then we
also won't have a bunch of places in the code that deal with that long
list of generic fields, which means the metadata array might not be
necessary, or might be simpler or smaller or designed differently.
All of which is to make the point that responding to Andres's feedback
will require a bunch of decisions about which parts of the feedback to
take (because some of them are mutually exclusive, as he acknowledges
himself) and what to do about them (because some of them are vague);
yet, taken together, they seem to amount to the need for significant
design changes, as do (1)-(4).

Now, just to be clear, the code we're talking about here is mostly
based on an original design by me, and whatever defects were present
in that original design are nobody's fault but mine. And that list of
defects includes pretty much everything in the above list. But, what
we need to figure out at this point is how we're going to get those
things fixed, and it seems to me that we're going to need a pretty
substantial redesign, but this discussion is kind of down in the
weeds.  I mean, what are we gaining by arguing about how many stages
we need for incremental unpacking if the real thing we need to is get
rid of that concept altogether?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company