Re: [PATCH 08/16] Introduce the ApplyCache module which can reassemble transactions from a stream of interspersed changes

Hi Steve,

On Tuesday, June 26, 2012 02:14:22 AM Steve Singer wrote:
> I planned to have some cutoff 'max_changes_in_memory_per_txn' value.
> > If it has
> > been reached for one transaction all existing changes are spilled to
> > disk. New changes again can be kept in memory till its reached again.
> Do you want max_changes_per_in_memory_txn or do you want to put a limit
> on the total amount of memory that the cache is able to use? How are you
> going to tell a DBA to tune max_changes_in_memory_per_txn? They know how
> much memory their system has and that they can devote to the apply cache
> versus other things, giving them guidance on how estimating how much
> open transactions they might have at a point in time and how many
> WAL change records each transaction generates seems like a step
> backwards from the progress we've been making in getting Postgresql to
> be easier to tune. The maximum number of transactions that could be
> opened at a time is governed by max_connections on the master at the
> time the WAL was generated , so I don't even see how the machine
> processing the WAL records could autotune/guess that.
It even can be significantly higher than max_connections because
subtransactions are only recognizable as part of their parent transaction
uppon commit.

I think max_changes_in_memory_per_txn will be the number of changes for now.
Making memory based accounting across multiple concurrent transactions work
efficiently and correctly isn't easy.

> > We need to support serializing the cache for crash recovery + shutdown of
> > the receiving side as well. Depending on how we do the wal decoding we
> > will need it more frequently...
> Have you described your thoughts on crash recovery on another thread?
I think I have somewhere, but given how much in flux our thoughts on decoding
are I think its not that important yet.

> I am thinking that this module would have to serialize some state
> everytime it calls cache->commit() to ensure that consumers don't get
> invoked twice on the same transaction.
In one of the other patches I implemented it by adding the (origin_id,
origin_lsn) pair to replicated commits. During recovery the startup process
sets up the shared memory status up to which point we applied.
If you then every now and then perform a 'logical checkpoint' writing down
whats the beginning lsn of the longest in-progress transaction is you can
fully recover from that point on.

> If the apply module is making changes to the same backend that the apply
> cache serializes to then both the state for the apply cache and the
> changes that committed changes/transactions make will be persisted (or
> not persisted) together. What if I am replicating from x86 to x86_64
> via a apply module that does textout conversions?
>
> x86 Proxy x86_64
> ----WAL------> apply
> cache
>
> | (proxy catalog)
>
> apply module
> textout --------------------->
> SQL statements
>
>
> How do we ensure that the commits are all visible(or not visible) on
> the catalog on the proxy instance used for decoding WAL, the destination
> database, and the state + spill files of the apply cache stay consistent
> in the event of a crash of either the proxy or the target?
> I don't think you can (unless we consider two-phase commit, and I'd
> rather we didn't). Can we come up with a way of avoiding the need for
> them to be consistent with each other?
Thats discussed in the "Catalog/Metadata consistency during changeset
extraction from wal" thread and we haven't yet determined which solution is
the best ;)

> Code Review
> =========
>
> applycache.h
> -----------------------
> +typedef struct ApplyCacheTupleBuf
> +{
> + /* position in preallocated list */
> + ilist_s_node node;
> +
> + HeapTupleData tuple;
> + HeapTupleHeaderData header;
> + char data[MaxHeapTupleSize];
> +} ApplyCacheTupleBuf;
>
> Each ApplyCacheTupleBuf will be about 8k (BLKSZ) big no matter how big
> the data in the transaction is? Wouldn't workloads with inserts of lots
> of small rows in a transaction eat up lots of memory that is allocated
> but storing nothing? The only alternative I can think of is dynamically
> allocating these and I don't know what the cost/benefit of that overhead
> will be versus spilling to disk sooner.
Dynamically allocating them totally destroys performance, I tried that. I
think at some point we should have 4 or so list of preallocated tuple bufs of
different sizes and then use the smallest possible one. But I think this
solution is ok in the very first version.

If you allocate dynamically you also get a noticeable performance drop when
you let the decoding run for a while because of fragmentation inside the
memory allocator.

> +* FIXME: better name
> + */
> +ApplyCacheChange*
> +ApplyCacheGetChange(ApplyCache*);
>
> How about:
>
> ApplyCacheReserveChangeStruct(..)
> ApplyCacheReserveChange(...)
> ApplyCacheAllocateChange(...)
>
> as ideas?
> +/*
> + * Return an unused ApplyCacheChange struct
> +*/
> +void
> +ApplyCacheReturnChange(ApplyCache*, ApplyCacheChange*);
>
> ApplyCacheReleaseChange(...) ? I keep thinking of 'Return' as us
> returning the data somewhere not the memory.
Hm. Reserve/Release doesn't sound bad. Acquire/Release is possibly even better
because reserve could be understood as a preparatory step?

> applycache.c:
> -------------------
>
> I've taken a quick look through this file and I don't see any issues
> other than the many FIXME's and other issues you've identified already,
> which I don't expect you to address in this CF.
Thanks for the review so far!

Greetings,

Andres

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