Re: logical changeset generation v4 - Heikki's thoughts about the patch state

Hi Robert, Hi all,

On 2013-01-23 20:17:04 -0500, Robert Haas wrote:
> On Wed, Jan 23, 2013 at 5:30 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > The only reason the submitted version of logical decoding is
> > comparatively slow is that its xmin update policy is braindamaged,
> > working on that right now.
> 
> I agree.  The thing that scares me about the logical replication stuff
> is not that it might be slow (and if your numbers are to be believed,
> it isn't), but that I suspect it's riddled with bugs and possibly some
> questionable design decisions.  If we commit it and release it, then
> we're going to be stuck maintaining it for a very, very long time.  If
> it turns out to have serious bugs that can't be fixed without a new
> major release, it's going to be a serious black eye for the project.

Thats way much more along the lines of what I am afraid of than the
performance stuff - but Heikki cited those, so I replied to that.

Note that I didn't say this must, must go in - I just don't think
Heikki's reasoning about why not hit the nail on the head.

> Of course, I have no evidence that that will happen.  But it is a
> really big piece of code, and therefore unless you are superman, it's
> probably got a really large number of bugs.  The scary thing is that
> it is not as if we can say, well, this is a big hunk of code, but it
> doesn't really touch the core of the system, so if it's broken, it'll
> be broken itself, but it won't break anything else.  Rather, this code
> is deeply in bed with WAL, with MVCC, and with the on-disk format of
> tuples, and makes fundamental changes to the first two of those.  You
> agreed with Tom that 9.2 is the buggiest release in recent memory, but
> I think logical replication could easily be an order of magnitude
> worse.

I tried very, very hard to get the basics of the design & interface
solid. Which obviously doesn't man I am succeeding - luckily not being
superhuman after all ;). And I think thats very much where input is
desparetely needed and where I failed to raise enough attention. The
"output plugin" interface follewed by the walsender interface is what
needs to be most closely vetted.
Those are the permanent, user/developer exposed UI and the one we should
try to keep as stable as possible.

The output plugin callbacks are defined here:

http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=blob;f=src/include/replication/output_plugin.h;hb=xlog-decoding-rebasing-cf4
To make it more agnostic of the technology to implement changeset
extraction we possibly should replace the ReorderBuffer(TXN|Change)
structs being passed by something more implementation agnostic.

walsender interface:

http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=blob;f=src/backend/replication/repl_gram.y;hb=xlog-decoding-rebasing-cf4
The interesting new commands are:
1) K_INIT_LOGICAL_REPLICATION NAME NAME
2) K_START_LOGICAL_REPLICATION NAME RECPTR plugin_options
3) K_FREE_LOGICAL_REPLICATION NAME

1 & 3 allocate (respectively free) the permanent state associated with
one changeset consumer whereas START_LOGICAL_REPLICATION streams out
changes starting at RECPTR.

Btw, there are currently *no* changes to the wal format at all if
wal_format < logical except that xl_running_xacts are logged more
frequently which obviously could easily be made conditional. Baring bugs
of course.
The changes with wal_level>=logical aren't that big either imo:
* heap_insert, heap_update prevent full page writes from removing their normal record by using a separate XLogRecData
blockfor the buffer and the record
 
* heap_delete adds more data (the pkey of the tuple) after the unchanged xl_heap_delete struct
* On changes to catalog tables (relfilenode, tid, cmin, cmax) are logged.

No changes to mvcc for normal backends at all, unless you count the very
slightly changed *Satisfies interface (getting passed a HeapTuple
instead of HeapTupleHeader).

I am not sure what you're concerned about WRT the on-disk format of the
tuples? We are pretty much nailed down on that due to pg_upgrade'ability
anyway and it could be changed from this patches POV without a problem,
the output plugin just sees normal HeapTuples? Or are you concerned
about the code extracting them from the xlog records?

So I think the "won't break anything else" argument can be made rather
fairly if the heapam.c changes, which aren't that complex, are vetted
closely.

Now, the disucssion about all the code thats active *during* decoding is
something else entirely :/

> You
> agreed with Tom that 9.2 is the buggiest release in recent memory, but
> I think logical replication could easily be an order of magnitude
> worse.

I unfortunately think that not providing more builtin capabilities in
this area also has significant dangers. Imo this is one of the weakest,
or even the weakest, area of postgres.

I personally have the impression that just about nobody did actual beta
testing of the lastest releases, especially 9.2, and that is the reason
why its the buggiest recent release.

> I also have serious concerns about checksums and foreign key locks.
> Any single one of those three patches could really inflict
> unprecedented damage on our community's reputation for stability and
> reliability if they turn out to be seriously buggy, and unfortunately
> I don't consider that an unlikely outcome.  I don't know what to do
> about it, either.

I see your point although I would attest both having far more danger of
collateral damage than logical decoding itself. Especially fklocks
pretty much has to be active by default and there's not much that can
be reasonably done about that.
I tried to give fklocks a very thorough look but unfortunately I didn't
know anything beforehand about most areas of the code it touches which
obviously limits the amount of dangers one is seeing (FWIW I am still
mostly concerned with multixact logging and the locking changes in
heapam.c itself).

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services