On Tue, Jun 18, 2019 at 2:07 PM Robert Haas <robertmh...@gmail.com> wrote: > On Tue, Jun 18, 2019 at 7:31 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > [ new patches ] > > I tried writing some code that throws an error from an undo log > handler and the results were not good.
I discovered another bothersome thing here: if you have a single transaction that generates a bunch of undo records, the first one has uur_dbid set correctly and the remaining records have uur_dbid set to 0. That means if you try to write a sanity check like if (record->uur_dbid != MyDatabaseId) elog(ERROR, "the undo system messed up") it fails. The original idea of UnpackedUndoRecord was this: you would put a bunch of data into an UnpackedUndoRecord that you wanted written to undo, and the undo system would find ways to compress stuff out of the on-disk representation by e.g. omitting the fork number if it's MAIN_FORKNUM. Then, when you read an undo record, it would decompress so that you ended up with the same UnpackedUndoRecord that you had at the beginning. However, the inclusion of transaction headers has made this a bit confusing: that stuff isn't being added by the user but by the undo system itself. It's not very clear from the comments what the contract is around these things: do you need to set uur_dbid to MyDatabaseId when preparing to insert an undo record? Or can you just leave it unset and then it'll possibly be set at decoding time? The comments for the UnpackedUndoRecord structure don't explain this. I'd really like to see this draw a cleaner distinction between the stuff that the user is expected to set and the other stuff we deal with internally to the undo subsystem. For example, suppose that UnpackedUndoRecord didn't include any of the fields that are only present in the transaction header. Maybe there's another structure, like UndoTransactionHeader, that includes those fields. The client of the undo subsystem creates a bunch of UnpackedUndoRecords and inserts them. At undo time, the callback gets back an identical set of UnpackedUndoRecords. And maybe it also gets a pointer to the UndoTransactionHeader which contains all of the system-generated stuff. Under this scheme, uur_xid, uur_xidepoch (which still need to be combined into uur_fxid), uur_progress, uur_dbid, uur_next, uur_prevlogstart, and uur_prevurp would all move out of the UnpackedUndoRecord and into the UndoTransactionHeader. The user would supply none of those things when inserting undo records, but the rm_undo callback could examine those values if it wished. A weaker approach would be to at least clean up the structure definition so that the transaction-header fields set by the system are clearly segregated from the per-record fields set by the undo-inserter, with comments explaining that those fields don't need to be set but will (or may?) be set at undo time. That would be better than what we have right now - because it would hopefully make it much more clear which fields need to be set on insert and which fields can be expected to be set when decoding - but I think it's probably not going far enough. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
