Re: Changeset Extraction v7.6.1

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