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

On 12-06-21 04:37 AM, Andres Freund wrote:
> Hi Steve,
> Thanks!
>

Attached is a detailed review of the patch.

Very good analysis, thanks!
> Another reasons why we cannot easily do 1) is that subtransactions aren't
> discernible from top-level transactions before the top-level commit happens,
> we can only properly merge in the right order (by "sorting" via lsn) once we
> have seen the commit record which includes a list of all committed
> subtransactions.
>

Based on that and your comments further down in your reply (and that no
one spoke up and disagreed with you) It sounds like that doing (1) isn't
going to be practical.

> I also don't think 1) would be particularly welcome by people trying to
> replicate into foreign systems.
>

They could still sort the changes into transaction groups before
applying to the foreign system.

> 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.

> 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 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.

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?

I think apply modules will need to be able to be passed the same
transaction twice (once before the crash and again after) and come up
with a way of deciding if that transaction has a) Been applied to the
translation/proxy catalog and b) been applied on the replica instance.
How is the walreceiver going to decide which WAL sgements it needs to
re-process after a crash? I would want to see more of these details
worked out before we finalize the interface between the apply cache and
the apply modules and how the serialization works.

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.

+* 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.

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.

> Andres
>