Hi, On 2014-02-19 13:01:02 -0500, Robert Haas wrote: > > I think it should be fairly easy to relax the restriction to creating a > > slot, but not getting data from it. Do you think that would that be > > sufficient? > > That would be a big improvement, for sure, and might be entirely sufficient.
Turned out to be a 5 line change + tests or something... Pushed. > >> I don't think this is a very good idea. The problem with doing things > >> during error recovery that can themselves fail is that you'll lose the > >> original error, which is not cool, and maybe even blow out the error > >> stack. Many people have confuse PG_TRY()/PG_CATCH() with an > >> exception-handling system, but it's not. One way to fix this is to > >> put some of the initialization logic in ReplicationSlotCreate() just > >> prior to calling CreateSlotOnDisk(). If the work that needs to be > >> done is too complex or protracted to be done there, then I think that > >> it should be pulled out of the act of creating the replication slot > >> and made to happen as part of first use, or as a separate operation > >> like PrepareForLogicalDecoding. > > > > I think what should be done here is adding a drop_on_release flag. As > > soon as everything important is done, it gets unset. > > That might be more elegant, but I don't think it really fixes > anything, because backing stuff out from on disk can fail. If the slot is marked as "drop_on_release" during creation, and we fail during removal, it will just be dropped on the next startup. That seems ok to me? I still think it's not really important to put much effort in the "disk stuff fails" case, it's entirely hypothetical. If that fails you have *so* much huger problems, a leftover slot is the least of you problems. > AIUI, your > whole concern here is that you don't want the slot creation to fail > halfway through and leave behind the slot, but what you've got here > doesn't prevent that; it just makes it less likely. The more I think > about it, the more I think you're trying to pack stuff into slot > creation that really ought to be happening on first use. Well, having a leftover slot that never succeeded being created is going to be confusing lots of people, especially as it will not rollback or something. That's why I think it's important to make it unlikely. The typical reasons for failing are stuff like a output plugin that doesn't exist or being interrupted while initializing. I can sympathize with the "too much during init" argument, but I don't see how moving stuff to the first call would get rid of the problems. If we fail later it's going to be just as confusing. > >> ReorderBufferGetTXN() should get a comment about the performance > >> impact of this. There's a tiny bit there in ReorderBufferReturnTXN() > >> but it should be better called out. Should these call the valgrind > >> macros to make the memory inaccessible while it's being held in cache? > > > > Hm, I think it does call the valgrind stuff? > > VALGRIND_MAKE_MEM_UNDEFINED(txn, sizeof(ReorderBufferTXN)); > > VALGRIND_MAKE_MEM_DEFINED(&txn->node, sizeof(txn->node)); > > That's there in ReorderBufferReturnTXN, but don't you need something > in ReorderBufferGetTXN? Maybe not. Don't think so, it marks the memory as undefined, which allows writes, but will warn on reads. We could additionally mark the memory as inaccessible disallowing writes, but I don't really that catching much. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
