On Sat, Sep 17, 2016 at 2:45 AM, Stas Kelvich <s.kelv...@postgrespro.ru> wrote: > Looking through old version i’ve noted few things that were bothering me: > > * In case of crash replay PREPARE redo accesses SUBTRANS, but > StartupSUBTRANS() isn’t called yet > in StartupXLOG(). > * Several functions in twophase.c have to walk through both PGPROC and > pg_twophase directory. > > Now I slightly changed order of things in StartupXLOG() so twophase state > loaded from from file and > StartupSUBTRANS called before actual recovery starts. So in all other > functions we can be sure that > file were already loaded in memory. > > Also since 2pc transactions now are dumped to files only on checkpoint, we > can get rid of > PrescanPreparedTransactions() — all necessary info can reside in checkpoint > itself. I’ve changed > behaviour of oldestActiveXid write in checkpoint and that’s probably > discussable topic, but ISTM > that simplifies a lot recovery logic in both twophase.c and xlog.c. More > comments on that in patch itself.
Finally I am back on this one.. First be more careful about typos in the comments of your code. Just reading through the patch I found quite a couple of places that needed to be fixed. cosequent, unavalable, SUBTRANCE are some examples among many other things.. + StartupCLOG(); + StartupSUBTRANS(checkPoint.oldestActiveXid); + RecoverPreparedFromFiles(); [...] /* - * Startup commit log and subtrans only. MultiXact and commit - * timestamp have already been started up and other SLRUs are not - * maintained during recovery and need not be started yet. - */ - StartupCLOG(); - StartupSUBTRANS(oldestActiveXID); Something is definitely wrong in this patch if we begin to do that. There is no need to move those two calls normally, and I think that we had better continue to do that only for hot standbys just to improve 2PC handling... CleanupPreparedTransactionsOnPITR() cleans up pg_twophase, but isn't it necessary to do something as well for what is in memory? I have been thinking about this patch quite a bit, and the approach taken looks really over-complicated to me. Basically what we are trying to do here is to reuse as much as possible code paths that are being used by non-recovery code paths during recovery, and then try to adapt a bunch of things like SUBTRANS structures, CLOG, XID handling and PGXACT things in sync to handle the 2PC information in memory correctly. I am getting to think that this is *really* bug-prone in the future and really going to make maintenance hard. Taking one step back, and I know that I am a noisy one, but looking at this patch is making me aware of the fact that it is trying more and more to patch things around the more we dig into issues, and I'd suspect that trying to adapt for example sub-transaction and clog handling is just the tip of the iceberg and that there are more things that need to be taken care of if we continue to move on with this approach. Coming to the point: why don't we simplify things? In short: - Just use a static list and allocate a chunk of shared memory as needed. DSM could come into play here to adapt to the size of a 2PC status file, this way there is no need to allocate a chunk of memory with an upper bound. Still we could use an hardcoded upper-bound of say 2k with max_prepared_transactions, and just write the 2PC status file to disk if its size is higher than what can stick into memory. - save 2PC information in memory instead of writing it to a 2PC file when XLOG_XACT_PREPARE shows up. - flush information to disk when there is a valid restart point in RecoveryRestartPoint(). We would need as well to tweak PrescanPreparedTransactions accordingly than everything we are trying to do here. That would be way more simple than what's proposed here, and we'd still keep all the performance benefits. -- Michael -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers