Re: POC: Cleaning up orphaned files using undo logs

I had a look at the UNDO patches at 
https://www.postgresql.org/message-id/CAA4eK1KKAFBCJuPnFtgdc89djv4xO%3DZkAdXvKQinqN4hWiRbvA%40mail.gmail.com, 
and at the patch to use the UNDO logs to clean up orphaned files, from 
undo-2019-05-10.tgz earlier in this thread. Are these the latest ones to 
review?

Thanks Thomas and Amit and others for working on this! Orphaned relfiles 
has been an ugly wart forever. It's a small thing, but really nice to 
fix that finally. This has been a long thread, and I haven't read it 
all, so please forgive me if I repeat stuff that's already been discussed.

There are similar issues in CREATE/DROP DATABASE code. If you crash in 
the middle of CREATE DATABASE, you can be left with orphaned files in 
the data directory, or if you crash in the middle of DROP DATABASE, the 
data might be gone already but the pg_database entry is still there. We 
should plug those holes too.

There's a lot of stuff in the patches that are not relevant for cleaning 
up orphaned files. I know this cleaning up orphaned files work is mainly 
a vehicle to get the UNDO log committed, so that's expected. If we only 
cared about orphaned files, I'm sure the patches wouldn't spend so much 
effort on concurrency, for example. Nevertheless, I think we should 
leave out some stuff that's clearly unused, for now. For example, a 
bunch of fields in the record format: uur_block, uur_offset, uur_tuple. 
You can add them later, as part of the patches that actually need them, 
but for now they just make the patch larger to review.

Some more thoughts on the record format:

I feel that the level of abstraction is not quite right. There are a 
bunch of fields, like uur_block, uur_offset, uur_tuple, that are 
probably useful for some UNDO resource managers (zheap I presume), but 
seem kind of arbitrary. How is uur_tuple different from uur_payload? 
Should they be named more generically as uur_payload1 and uur_payload2? 
And why two, why not three or four different payloads? In the WAL record 
format, there's a concept of "block id", which allows you to store N 
number of different payloads in the record, I think that would be a 
better approach. Or only have one payload, and let the resource manager 
code divide it as it sees fit.

Many of the fields support a primitive type of compression, where a 
field can be omitted if it has the same value as on the first record on 
an UNDO page. That's handy. But again I don't like the fact that the 
fields have been hard-coded into the UNDO record format. I can see e.g. 
the relation oid to be useful for many AMs. But not all. And other AMs 
might well want to store and deduplicate other things, aside from the 
fields that are in the patch now. I'd like to move most of the fields to 
AM specific code, and somehow generalize the compression. One approach 
would be to let the AM store an arbitrary struct, and run it through a 
general-purpose compression algorithm, using the UNDO page's first 
record as the "dictionary". Or make the UNDO page's first record 
available in whole to the AM specific code, and let the AM do the 
deduplication. For cleaning up orphaned files, though, we don't really 
care about any of that, so I'd recommend just ripping it out for now. 
Compression/deduplication can be added later as a separate patch.

The orphaned-file cleanup patch doesn't actually use the uur_reloid 
field. It stores the RelFileNode instead, in the paylod. I think that's 
further evidence that the hard-coded fields in the record format are not 
quite right.


I don't like the way UndoFetchRecord returns a palloc'd 
UnpackedUndoRecord. I would prefer something similar to the xlogreader 
API, where a new call to UndoFetchRecord invalidates the previous 
result. On efficiency grounds, to avoid the palloc, but also to be 
consistent with xlogreader.

In the UNDO page header, there are a bunch of fields like 
pd_lower/pd_upper/pd_special that are copied from the "standard" page 
header, that are unused. There's a FIXME comment about that too. Let's 
remove them, there's no need for UNDO pages to look like standard 
relation pages. The LSN needs to be at the beginning, to work with the 
buffer manager, but that's the only requirement.

Could we leave out the UNDO and discard worker processes for now? 
Execute all UNDO actions immediately at rollback, and after crash 
recovery. That would be fine for cleaning up orphaned files, and it 
would cut down the size of the patch to review.

Can this race condition happen: Transaction A creates a table and an 
UNDO record to remember it. The transaction is rolled back, and the file 
is removed. Another transaction, B, creates a different table, and 
chooses the same relfilenode. It loads the table with data, and commits. 
Then the system crashes. After crash recovery, the UNDO record for the 
first transaction is applied, and it removes the file that belongs to 
the second table, created by transaction B.

- Heikki