Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

Hi all,

Attached is a patch series that implements two features to the logical
replication - ability to define a memory limit for the reorderbuffer
(responsible for building the decoded transactions), and ability to
stream large in-progress transactions (exceeding the memory limit).

I'm submitting those two changes together, because one builds on the
other, and it's beneficial to discuss them together.

PART 1: adding logical_work_mem memory limit (0001)
---------------------------------------------------

Currently, limiting the amount of memory consumed by logical decoding is
tricky (or you might say impossible) for several reasons:

* The value is hard-coded, so it's not quite possible to customize it.

* The amount of decoded changes to keep in memory is restricted by
number of changes. It's not very unclear how this relates to memory
consumption, as the change size depends on table structure, etc.

* The number is "per (sub)transaction", so a transaction with many
subtransactions may easily consume significant amount of memory without
actually hitting the limit.

So the patch does two things. Firstly, it introduces logical_work_mem, a
GUC restricting memory consumed by all transactions currently kept in
the reorder buffer.

Secondly, it adds a simple memory accounting by tracking the amount of
memory used in total (for the whole reorder buffer, to compare against
logical_work_mem) and per transaction (so that we can quickly pick
transaction to spill to disk).

The one wrinkle on the patch is that the memory limit can't be enforced
when reading changes spilled to disk - with multiple subtransactions, we
can't easily predict how many changes to pre-read for each of them. At
that point we still use the existing max_changes_in_memory limit.

Luckily, changes introduced in the other parts of the patch should allow
addressing this deficiency.

PART 2: streaming of large in-progress transactions (0002-0006)
---------------------------------------------------------------

Note: This part is split into multiple smaller chunks, addressing
different parts of the logical decoding infrastructure. That's mostly to
allow easier reviews, though. Ultimately, it's just one patch.

Processing large transactions often results in significant apply lag,
for a couple of reasons. One reason is network bandwidth - while we do
decode the changes incrementally (as we read the WAL), we keep them
locally, either in memory, or spilled to files. Then at commit time, all
the changes get sent to the downstream (and applied) at the same time.
For large transactions the time to do the network transfer may be
significant, causing apply lag.

This patch extends the logical replication infrastructure (output plugin
API, reorder buffer, pgoutput, replication protocol etc.) so allow
streaming of in-progress transactions instead of spilling them to local
files.

The extensions to the API are pretty straightforward. Aside from adding
methods to stream changes/messages and commit a streamed transaction,
the API needs a function to abort a streamed (sub)transaction, and
functions to demarcate a block of streamed changes.

To decode a transaction, we need to know all it's subtransactions, and
invalidations. Currently, those are only known at commit time (although
some assignments may be known earlier), but invalidations are only ever
written in the commit record.

So far that was fine, because we only decode/replay transactions at
commit time, when all of this is known (because it's either in commit
record, or written before it).

But for in-progress transactions (i.e. the subject of interest here),
that is not the case. So the patch modifies WAL-logging to ensure those
two bits of information are written immediately (for wal_level=logical).

For assignments that was fairly simple, thanks to existing caching. For
invalidations, it requires a new WAL record type and a couple of changes
in inval.c.

On the apply side, we simply receive the streamed changes, write them
into a file (one file for toplevel transaction, which is possible thanks
to the assignments being known immediately). And then at commit time the
changes are replayed locally, without having to copy a large chunk of
data over network.

WAL overhead
------------

Of course, these changes to WAL logging are not for free - logging
assignments individually (instead of multiple subtransactions at once)
means higher xlog record overhead. Similarly, (sub)transactions doing a
lot of DDL may result in a lot of invalidations written to WAL (again,
with full xlog record overhead per invalidation).

I've done a number of tests to measure the impact, and for extreme
corner cases the additional amount of WAL is about 40% in both cases.

By an "extreme corner case" I mean a workloads intentionally triggering
many assignments/invalidations, without doing a lot of meaningful work.

For assignments, imagine a single-row table (no indexes), and a
transaction like this one:

BEGIN;
UPDATE t SET v = v + 1;
SAVEPOINT s1;
UPDATE t SET v = v + 1;
SAVEPOINT s2;
UPDATE t SET v = v + 1;
SAVEPOINT s3;
...
UPDATE t SET v = v + 1;
SAVEPOINT s10;
UPDATE t SET v = v + 1;
COMMIT;

For invalidations, add a CREATE TEMPORARY TABLE to each subtransaction.

For more realistic workloads (large table with indexes, runs long enough
to generate FPIs, etc.) the overhead drops below 5%. Which is much more
acceptable, of course, although not perfect.

In both cases, there was pretty much no measurable impact on performance
(as measured by tps).

I do not think there's a way around this requirement (having assignments
and invalidations), if we want to decode in-progress transactions. But
perhaps it would be possible to do some sort of caching (say, at command
level), to reduce the xlog record overhead? Not sure.

All ideas are welcome, of course. In the worst case, I think we can add
a GUC enabling this additional logging - when disabled, streaming of
in-progress transactions would not be possible.

Simplifying ReorderBuffer
-------------------------

One interesting consequence of having assignments is that we could get
rid of the ReorderBuffer iterator, used to merge changes from subxacts.
The assignments allow us to keep changes for each toplevel transaction
in a single list, in LSN order, and just walk it. Abort can be performed
by remembering position of the first change in each subxact, and just
discarding the tail.

This is what the apply worker does with the streamed changes and aborts.

It would also allow us to enforce the memory limit while restoring
transactions spilled to disk, because we would not have the problem with
restoring changes for many subtransactions.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2017-12-23 05:57, Tomas Vondra wrote:
> Hi all,
>
> Attached is a patch series that implements two features to the logical
> replication - ability to define a memory limit for the reorderbuffer
> (responsible for building the decoded transactions), and ability to
> stream large in-progress transactions (exceeding the memory limit).
>

logical replication of 2 instances is OK but 3 and up fail with:

TRAP: FailedAssertion("!(last_lsn < change->lsn)", File:
"reorderbuffer.c", Line: 1773)

I can cobble up a script but I hope you have enough from the assertion
to see what's going wrong...

On 12/23/2017 03:03 PM, Erikjan Rijkers wrote:
> On 2017-12-23 05:57, Tomas Vondra wrote:
>> Hi all,
>>
>> Attached is a patch series that implements two features to the logical
>> replication - ability to define a memory limit for the reorderbuffer
>> (responsible for building the decoded transactions), and ability to
>> stream large in-progress transactions (exceeding the memory limit).
>>
>
> logical replication of 2 instances is OK but 3 and up fail with:
>
> TRAP: FailedAssertion("!(last_lsn < change->lsn)", File:
> "reorderbuffer.c", Line: 1773)
>
> I can cobble up a script but I hope you have enough from the assertion
> to see what's going wrong...

The assertion says that the iterator produces changes in order that does
not correlate with LSN. But I have a hard time understanding how that
could happen, particularly because according to the line number this
happens in ReorderBufferCommit(), i.e. the current (non-streaming) case.

So instructions to reproduce the issue would be very helpful.

Attached is v2 of the patch series, fixing two bugs I discovered today.
I don't think any of these is related to your issue, though.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2017-12-23 21:06, Tomas Vondra wrote:
> On 12/23/2017 03:03 PM, Erikjan Rijkers wrote:
>> On 2017-12-23 05:57, Tomas Vondra wrote:
>>> Hi all,
>>>
>>> Attached is a patch series that implements two features to the
>>> logical
>>> replication - ability to define a memory limit for the reorderbuffer
>>> (responsible for building the decoded transactions), and ability to
>>> stream large in-progress transactions (exceeding the memory limit).
>>>
>>
>> logical replication of 2 instances is OK but 3 and up fail with:
>>
>> TRAP: FailedAssertion("!(last_lsn < change->lsn)", File:
>> "reorderbuffer.c", Line: 1773)
>>
>> I can cobble up a script but I hope you have enough from the assertion
>> to see what's going wrong...
>
> The assertion says that the iterator produces changes in order that
> does
> not correlate with LSN. But I have a hard time understanding how that
> could happen, particularly because according to the line number this
> happens in ReorderBufferCommit(), i.e. the current (non-streaming)
> case.
>
> So instructions to reproduce the issue would be very helpful.

Using:

0001-Introduce-logical_work_mem-to-limit-ReorderBuffer-v2.patch
0002-Issue-XLOG_XACT_ASSIGNMENT-with-wal_level-logical-v2.patch
0003-Issue-individual-invalidations-with-wal_level-log-v2.patch
0004-Extend-the-output-plugin-API-with-stream-methods-v2.patch
0005-Implement-streaming-mode-in-ReorderBuffer-v2.patch
0006-Add-support-for-streaming-to-built-in-replication-v2.patch

As you expected the problem is the same with these new patches.

I have now tested more, and seen that it not always fails. I guess that
it here fails 3 times out of 4. But the laptop I'm using at the moment
is old and slow -- it may well be a factor as we've seen before [1].

Attached is the bash that I put together. I tested with
NUM_INSTANCES=2, which yields success, and NUM_INSTANCES=3, which fails
often. This same program run with HEAD never seems to fail (I tried a
few dozen times).

thanks,

Erik Rijkers

[1]
https://www.postgresql.org/message-id/3897361c7010c4ac03f358173adbcd60%40xs4all.nl

On 12/23/2017 11:23 PM, Erik Rijkers wrote:
> On 2017-12-23 21:06, Tomas Vondra wrote:
>> On 12/23/2017 03:03 PM, Erikjan Rijkers wrote:
>>> On 2017-12-23 05:57, Tomas Vondra wrote:
>>>> Hi all,
>>>>
>>>> Attached is a patch series that implements two features to the logical
>>>> replication - ability to define a memory limit for the reorderbuffer
>>>> (responsible for building the decoded transactions), and ability to
>>>> stream large in-progress transactions (exceeding the memory limit).
>>>>
>>>
>>> logical replication of 2 instances is OK but 3 and up fail with:
>>>
>>> TRAP: FailedAssertion("!(last_lsn < change->lsn)", File:
>>> "reorderbuffer.c", Line: 1773)
>>>
>>> I can cobble up a script but I hope you have enough from the assertion
>>> to see what's going wrong...
>>
>> The assertion says that the iterator produces changes in order that does
>> not correlate with LSN. But I have a hard time understanding how that
>> could happen, particularly because according to the line number this
>> happens in ReorderBufferCommit(), i.e. the current (non-streaming) case.
>>
>> So instructions to reproduce the issue would be very helpful.
>
> Using:
>
> 0001-Introduce-logical_work_mem-to-limit-ReorderBuffer-v2.patch
> 0002-Issue-XLOG_XACT_ASSIGNMENT-with-wal_level-logical-v2.patch
> 0003-Issue-individual-invalidations-with-wal_level-log-v2.patch
> 0004-Extend-the-output-plugin-API-with-stream-methods-v2.patch
> 0005-Implement-streaming-mode-in-ReorderBuffer-v2.patch
> 0006-Add-support-for-streaming-to-built-in-replication-v2.patch
>
> As you expected the problem is the same with these new patches.
>
> I have now tested more, and seen that it not always fails.  I guess that
> it here fails 3 times out of 4.  But the laptop I'm using at the moment
> is old and slow -- it may well be a factor as we've seen before [1].
>
> Attached is the bash that I put together.  I tested with
> NUM_INSTANCES=2, which yields success, and NUM_INSTANCES=3, which fails
> often.  This same program run with HEAD never seems to fail (I tried a
> few dozen times).
>

Thanks. Unfortunately I still can't reproduce the issue. I even tried
running it in valgrind, to see if there are some memory access issues
(which should also slow it down significantly).

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 23 December 2017 at 12:57, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
wrote:

> Hi all,
>
> Attached is a patch series that implements two features to the logical
> replication - ability to define a memory limit for the reorderbuffer
> (responsible for building the decoded transactions), and ability to
> stream large in-progress transactions (exceeding the memory limit).
>
> I'm submitting those two changes together, because one builds on the
> other, and it's beneficial to discuss them together.
>
>
> PART 1: adding logical_work_mem memory limit (0001)
> ---------------------------------------------------
>
> Currently, limiting the amount of memory consumed by logical decoding is
> tricky (or you might say impossible) for several reasons:
>
> * The value is hard-coded, so it's not quite possible to customize it.
>
> * The amount of decoded changes to keep in memory is restricted by
> number of changes. It's not very unclear how this relates to memory
> consumption, as the change size depends on table structure, etc.
>
> * The number is "per (sub)transaction", so a transaction with many
> subtransactions may easily consume significant amount of memory without
> actually hitting the limit.
>

Also, even without subtransactions, we assemble a ReorderBufferTXN per
transaction. Since transactions usually occur concurrently, systems with
many concurrent txns can face lots of memory use.

We can't exclude tables that won't actually be replicated at the reorder
buffering phase either. So txns use memory whether or not they do anything
interesting as far as a given logical decoding session is concerned. Even
if we'll throw all the data away we must buffer and assemble it first so we
can make that decision.

Because logical decoding considers snapshots and cid increments even from
other DBs (at least when the txn makes catalog changes) the memory use can
get BIG too. I was recently working with a system that had accumulated 2GB
of snapshots ... on each slot. With 7 slots, one for each DB.

So there's lots of room for difficulty with unpredictable memory use.

So the patch does two things. Firstly, it introduces logical_work_mem, a
> GUC restricting memory consumed by all transactions currently kept in
> the reorder buffer
>

Does this consider the (currently high, IIRC) overhead of tracking
serialized changes?

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

>>>>
>>>> logical replication of 2 instances is OK but 3 and up fail with:
>>>>
>>>> TRAP: FailedAssertion("!(last_lsn < change->lsn)", File:
>>>> "reorderbuffer.c", Line: 1773)
>>>>
>>>> I can cobble up a script but I hope you have enough from the
>>>> assertion
>>>> to see what's going wrong...
>>>
>>> The assertion says that the iterator produces changes in order that
>>> does
>>> not correlate with LSN. But I have a hard time understanding how that
>>> could happen, particularly because according to the line number this
>>> happens in ReorderBufferCommit(), i.e. the current (non-streaming)
>>> case.
>>>
>>> So instructions to reproduce the issue would be very helpful.
>>
>> Using:
>>
>> 0001-Introduce-logical_work_mem-to-limit-ReorderBuffer-v2.patch
>> 0002-Issue-XLOG_XACT_ASSIGNMENT-with-wal_level-logical-v2.patch
>> 0003-Issue-individual-invalidations-with-wal_level-log-v2.patch
>> 0004-Extend-the-output-plugin-API-with-stream-methods-v2.patch
>> 0005-Implement-streaming-mode-in-ReorderBuffer-v2.patch
>> 0006-Add-support-for-streaming-to-built-in-replication-v2.patch
>>
>> As you expected the problem is the same with these new patches.
>>
>> I have now tested more, and seen that it not always fails.  I guess
>> that
>> it here fails 3 times out of 4.  But the laptop I'm using at the
>> moment
>> is old and slow -- it may well be a factor as we've seen before [1].
>>
>> Attached is the bash that I put together.  I tested with
>> NUM_INSTANCES=2, which yields success, and NUM_INSTANCES=3, which
>> fails
>> often.  This same program run with HEAD never seems to fail (I tried a
>> few dozen times).
>>
>
> Thanks. Unfortunately I still can't reproduce the issue. I even tried
> running it in valgrind, to see if there are some memory access issues
> (which should also slow it down significantly).

One wonders again if 2ndquadrant shouldn't invest in some old hardware
;)

Another Good Thing would be if there was a provision in the buildfarm to
test patches like these.

But I'm probably not to first one to suggest that; no doubt it'll be
possible someday. In the meantime I'll try to repeat this crash on
other machines (but that will be after the holidays).

Erik Rijkers

On 12/24/2017 05:51 AM, Craig Ringer wrote:
> On 23 December 2017 at 12:57, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com
> <mailto:tomas(dot)vondra(at)2ndquadrant(dot)com>> wrote:
>
> Hi all,
>
> Attached is a patch series that implements two features to the logical
> replication - ability to define a memory limit for the reorderbuffer
> (responsible for building the decoded transactions), and ability to
> stream large in-progress transactions (exceeding the memory limit).
>
> I'm submitting those two changes together, because one builds on the
> other, and it's beneficial to discuss them together.
>
>
> PART 1: adding logical_work_mem memory limit (0001)
> ---------------------------------------------------
>
> Currently, limiting the amount of memory consumed by logical decoding is
> tricky (or you might say impossible) for several reasons:
>
> * The value is hard-coded, so it's not quite possible to customize it.
>
> * The amount of decoded changes to keep in memory is restricted by
> number of changes. It's not very unclear how this relates to memory
> consumption, as the change size depends on table structure, etc.
>
> * The number is "per (sub)transaction", so a transaction with many
> subtransactions may easily consume significant amount of memory without
> actually hitting the limit.
>
>
> Also, even without subtransactions, we assemble a ReorderBufferTXN
> per transaction. Since transactions usually occur concurrently,
> systems with many concurrent txns can face lots of memory use.
>

I don't see how that could be a problem, considering the number of
toplevel transactions is rather limited (to max_connections or so).

> We can't exclude tables that won't actually be replicated at the reorder
> buffering phase either. So txns use memory whether or not they do
> anything interesting as far as a given logical decoding session is
> concerned. Even if we'll throw all the data away we must buffer and
> assemble it first so we can make that decision.

Yep.

> Because logical decoding considers snapshots and cid increments even
> from other DBs (at least when the txn makes catalog changes) the memory
> use can get BIG too. I was recently working with a system that had
> accumulated 2GB of snapshots ... on each slot. With 7 slots, one for
> each DB.
>
> So there's lots of room for difficulty with unpredictable memory use.
>

Yep.

> So the patch does two things. Firstly, it introduces logical_work_mem, a
> GUC restricting memory consumed by all transactions currently kept in
> the reorder buffer
>
>
> Does this consider the (currently high, IIRC) overhead of tracking
> serialized changes?
>  

Consider in what sense?

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 12/24/2017 10:00 AM, Erik Rijkers wrote:
>>>>>
>>>>> logical replication of 2 instances is OK but 3 and up fail with:
>>>>>
>>>>> TRAP: FailedAssertion("!(last_lsn < change->lsn)", File:
>>>>> "reorderbuffer.c", Line: 1773)
>>>>>
>>>>> I can cobble up a script but I hope you have enough from the assertion
>>>>> to see what's going wrong...
>>>>
>>>> The assertion says that the iterator produces changes in order that
>>>> does
>>>> not correlate with LSN. But I have a hard time understanding how that
>>>> could happen, particularly because according to the line number this
>>>> happens in ReorderBufferCommit(), i.e. the current (non-streaming)
>>>> case.
>>>>
>>>> So instructions to reproduce the issue would be very helpful.
>>>
>>> Using:
>>>
>>> 0001-Introduce-logical_work_mem-to-limit-ReorderBuffer-v2.patch
>>> 0002-Issue-XLOG_XACT_ASSIGNMENT-with-wal_level-logical-v2.patch
>>> 0003-Issue-individual-invalidations-with-wal_level-log-v2.patch
>>> 0004-Extend-the-output-plugin-API-with-stream-methods-v2.patch
>>> 0005-Implement-streaming-mode-in-ReorderBuffer-v2.patch
>>> 0006-Add-support-for-streaming-to-built-in-replication-v2.patch
>>>
>>> As you expected the problem is the same with these new patches.
>>>
>>> I have now tested more, and seen that it not always fails.  I guess that
>>> it here fails 3 times out of 4.  But the laptop I'm using at the moment
>>> is old and slow -- it may well be a factor as we've seen before [1].
>>>
>>> Attached is the bash that I put together.  I tested with
>>> NUM_INSTANCES=2, which yields success, and NUM_INSTANCES=3, which fails
>>> often.  This same program run with HEAD never seems to fail (I tried a
>>> few dozen times).
>>>
>>
>> Thanks. Unfortunately I still can't reproduce the issue. I even tried
>> running it in valgrind, to see if there are some memory access issues
>> (which should also slow it down significantly).
>
> One wonders again if 2ndquadrant shouldn't invest in some old hardware ;)
>

Well, I've done tests on various machines, including some really slow
ones, and I still haven't managed to reproduce the failures using your
script. So I don't think that would really help. But I have reproduced
it by using a custom stress test script.

Turns out the asserts are overly strict - instead of

Assert(prev_lsn < current_lsn);

it should have been

Assert(prev_lsn <= current_lsn);

because some XLOG records may contain multiple rows (e.g. MULTI_INSERT).

The attached v3 fixes this issue, and also a couple of other thinkos:

1) The AssertChangeLsnOrder assert check was somewhat broken.

2) We've been sending aborts for all subtransactions, even those not yet
streamed. So downstream got confused and fell over because of an assert.

3) The streamed transactions were written to /tmp, using filenames using
subscription OID and XID of the toplevel transaction. That's fine, as
long as there's just a single replica running - if there are more, the
filenames will clash, causing really strange failures. So move the files
to base/pgsql_tmp where regular temporary files are written. I'm not
claiming this is perfect, perhaps we need to invent another location.

FWIW I believe the relation sync cache is somewhat broken by the
streaming. I thought resetting it would be good enough, but it's more
complicated (and trickier) than that. I'm aware of it, and I'll look
into that next - but probably not before 2018.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

That indeed fixed the problem: running that same pgbench test, I see no
crashes anymore (on any of 3 different machines, and with several
pgbench parameters).

Thank you,

Erik Rijkers

> On 25 December 2017 at 18:40, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>
wrote:
> The attached v3 fixes this issue, and also a couple of other thinkos

Thank you for the patch, it looks quite interesting. After a quick look at
it
(mostly the first one so far, but I'm going to continue) I have a few
questions:

> + * XXX With many subtransactions this might be quite slow, because we'll
have
> + * to walk through all of them. There are some options how we could
improve
> + * that: (a) maintain some secondary structure with transactions sorted
by
> + * amount of changes, (b) not looking for the entirely largest
transaction,
> + * but e.g. for transaction using at least some fraction of the memory
limit,
> + * and (c) evicting multiple transactions at once, e.g. to free a given
portion
> + * of the memory limit (e.g. 50%).

Do you want to address these possible alternatives somehow in this patch or
you
want to left it outside? Maybe it makes sense to apply some combination of
them, e.g. maintain a secondary structure with relatively large
transactions,
and then start evicting them. If it's somehow not enough, then start to
evict
multiple transactions at once (option "c").

> + /*
> + * We clamp manually-set values to at least 64kB. The
maintenance_work_mem
> + * uses a higher minimum value (1MB), so this is OK.
> + */
> + if (*newval < 64)
> + *newval = 64;
> +

I'm not sure what's recommended practice here, but maybe it makes sense to
have a warning here about changing this value to 64kB? Otherwise it can be
unexpected.

On 12/22/17 23:57, Tomas Vondra wrote:
> PART 1: adding logical_work_mem memory limit (0001)
> ---------------------------------------------------

The documentation in this patch contains some references to later
features (streaming). Perhaps that could be separated so that the
patches can be applied independently.

I don't see the need to tie this setting to maintenance_work_mem.
maintenance_work_mem is often set to very large values, which could then
have undesirable side effects on this use.

Moreover, the name logical_work_mem makes it sound like it's a logical
version of work_mem. Maybe we could think of another name.

I think we need a way to report on how much memory is actually used, so
the setting can be tuned. Something analogous to log_temp_files perhaps.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 01/02/2018 04:07 PM, Peter Eisentraut wrote:
> On 12/22/17 23:57, Tomas Vondra wrote:
>> PART 1: adding logical_work_mem memory limit (0001)
>> ---------------------------------------------------
>
> The documentation in this patch contains some references to later
> features (streaming). Perhaps that could be separated so that the
> patches can be applied independently.
>

Yeah, that's probably a good idea. But now that you mention it, I wonder
if "streaming" is really a good term. We already use it for "streaming
replication" and it may be quite confusing to use it for another feature
(particularly when it's streaming within logical streaming replication).

But I can't really think of a better name ...

> I don't see the need to tie this setting to maintenance_work_mem.
> maintenance_work_mem is often set to very large values, which could
> then have undesirable side effects on this use.
>

Well, we need to pick some default value, and we can either use a fixed
value (not sure what would be a good default) or tie it to an existing
GUC. We only really have work_mem and maintenance_work_mem, and the
walsender process will never use more than one such buffer. Which seems
to be closer to maintenance_work_mem.

Pretty much any default value can have undesirable side effects.

> Moreover, the name logical_work_mem makes it sound like it's a logical
> version of work_mem. Maybe we could think of another name.
>

I won't object to a better name, of course. Any proposals?

> I think we need a way to report on how much memory is actually used,
> so the setting can be tuned. Something analogous to log_temp_files
> perhaps.
>

Yes, I agree. I'm just about to submit an updated version of the patch
series, that also introduces a few columns pg_stat_replication, tracking
this type of stats (amount of data spilled to disk or streamed, etc.).

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

attached is v4 of the patch series, with a couple of changes:

1) Fixes a bunch of bugs I discovered during stress testing.

I'm not going to go into details, but the main fixes are related to
properly updating progress from the worker, and not streaming when
creating the logical replication slot.

2) Introduces columns into pg_stat_replication.

The new columns track various kinds of statistics (number of xacts,
bytes, ...) about spill-to-disk/streaming. This will be useful when
tuning the GUC memory limit.

3) Two temporary bugfixes that make the patch series work.

The first one (0008) makes sure is_known_subxact is set properly for all
subtransactions, and there's a separate fix in the CF. So this will
eventually go away.

The second one (0009) fixes an issue that is specific to streaming. It
does fix the issue, but I need a bit more time to think about it before
merging it into 0005.

This does pass extensive stress testing with a workload mixing DML, DDL,
subtransactions, aborts, etc. under valgrind. I'm working on extending
the test coverage, and introducing various error conditions (e.g.
walsender/walreceiver timeouts, failures on both ends, etc.).

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 01/03/2018 09:06 PM, Tomas Vondra wrote:
> Hi,
>
> attached is v4 of the patch series, with a couple of changes:
>
> 1) Fixes a bunch of bugs I discovered during stress testing.
>
> I'm not going to go into details, but the main fixes are related to
> properly updating progress from the worker, and not streaming when
> creating the logical replication slot.
>
> 2) Introduces columns into pg_stat_replication.
>
> The new columns track various kinds of statistics (number of xacts,
> bytes, ...) about spill-to-disk/streaming. This will be useful when
> tuning the GUC memory limit.
>
> 3) Two temporary bugfixes that make the patch series work.
>

Forgot to mention that the v4 also extends the CREATE SUBSCRIPTION to
allow customizing the streaming and memory limit. So you can do

CREATE SUBSCRIPTION ... WITH (streaming=on, work_mem=1024)

and this subscription will allow streaming, and the logica_work_mem (on
provider) will be set to 1MB.

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 1/3/18 14:53, Tomas Vondra wrote:
>> I don't see the need to tie this setting to maintenance_work_mem.
>> maintenance_work_mem is often set to very large values, which could
>> then have undesirable side effects on this use.
>
> Well, we need to pick some default value, and we can either use a fixed
> value (not sure what would be a good default) or tie it to an existing
> GUC. We only really have work_mem and maintenance_work_mem, and the
> walsender process will never use more than one such buffer. Which seems
> to be closer to maintenance_work_mem.
>
> Pretty much any default value can have undesirable side effects.

Let's just make it an independent setting unless we know any better. We
don't have a lot of settings that depend on other settings, and the ones
we do have a very specific relationship.

>> Moreover, the name logical_work_mem makes it sound like it's a logical
>> version of work_mem. Maybe we could think of another name.
>
> I won't object to a better name, of course. Any proposals?

logical_decoding_[work_]mem?

>> I think we need a way to report on how much memory is actually used,
>> so the setting can be tuned. Something analogous to log_temp_files
>> perhaps.
>
> Yes, I agree. I'm just about to submit an updated version of the patch
> series, that also introduces a few columns pg_stat_replication, tracking
> this type of stats (amount of data spilled to disk or streamed, etc.).

That seems OK. Perhaps we could bring forward the part of that patch
that applies to this feature.

That would also help testing *this* feature and determine what
appropriate settings are.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 1/3/18 15:13, Tomas Vondra wrote:
> Forgot to mention that the v4 also extends the CREATE SUBSCRIPTION to
> allow customizing the streaming and memory limit. So you can do
>
> CREATE SUBSCRIPTION ... WITH (streaming=on, work_mem=1024)
>
> and this subscription will allow streaming, and the logica_work_mem (on
> provider) will be set to 1MB.

I was wondering already during PG10 development whether we should give
subscriptions a generic configuration array, like databases and roles
have, so we don't have to hardcode a bunch of similar stuff every time
we add an option like this. At the time we only had synchronous_commit,
but now we're adding more.

Also, instead of sticking this into the START_REPLICATION command, could
we just run a SET command? That should work over replication
connections as well.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 12/22/17 23:57, Tomas Vondra wrote:
> PART 1: adding logical_work_mem memory limit (0001)
> ---------------------------------------------------
>
> Currently, limiting the amount of memory consumed by logical decoding is
> tricky (or you might say impossible) for several reasons:

I would like to see some more discussion on this, but I think not a lot
of people understand the details, so I'll try to write up an explanation
here. This code is also somewhat new to me, so please correct me if
there are inaccuracies, while keeping in mind that I'm trying to simplify.

The data in the WAL is written as it happens, so the changes belonging
to different transactions are all mixed together. One of the jobs of
logical decoding is to reassemble the changes belonging to each
transaction. The top-level data structure for that is the infamous
ReorderBuffer. So as it reads the WAL and sees something about a
transaction, it keeps a copy of that change in memory, indexed by
transaction ID (ReorderBufferChange). When the transaction commits, the
accumulated changes are passed to the output plugin and then freed. If
the transaction aborts, then changes are just thrown away.

So when logical decoding is active, a copy of the changes for each
active transaction is kept in memory (once per walsender).

More precisely, the above happens for each subtransaction. When the
top-level transaction commits, it finds all its subtransactions in the
ReorderBuffer, reassembles everything in the right order, then invokes
the output plugin.

All this could end up using an unbounded amount of memory, so there is a
mechanism to spill changes to disk. The way this currently works is
hardcoded, and this patch proposes to change that.

Currently, when a transaction or subtransaction has accumulated 4096
changes, it is spilled to disk. When the top-level transaction commits,
things are read back from disk to do the final processing mentioned above.

This all works mostly fine, but you can construct some more extreme
cases where this can blow up.

Here is a mundane example. Let's say a change entry takes 100 bytes (it
might contain a new row, or an update key and some new column values,
for example). If you have 100 concurrent active sessions and no
subtransactions, then logical decoding memory is bounded by 4096 * 100 *
100 = 40 MB (per walsender) before things spill to disk.

Now let's say you are using a lot of subtransactions, because you are
using PL functions, exception handling, triggers, doing batch updates.
If you have 200 subtransactions on average per concurrent session, the
memory usage bound in that case would be 4096 * 100 * 100 * 200 = 8 GB
(per walsender). And so on. If you have more concurrent sessions or
larger changes or more subtransactions, you'll use much more than those
8 GB. And if you don't have those 8 GB, then you're stuck at this point.

That is the consideration when we record changes, but we also need
memory when we do the final processing at commit time. That is slightly
less problematic because we only process one top-level transaction at a
time, so the formula is only 4096 * avg_size_of_changes * nr_subxacts
(without the concurrent sessions factor).

So, this patch proposes to improve this as follows:

- We compute the actual size of each ReorderBufferChange and keep a
running tally for each transaction, instead of just counting the number
of changes.

- We have a configuration setting that allows us to change the limit
instead of the hardcoded 4096. The configuration setting is also in
terms of memory, not in number of changes.

- The configuration setting is for the total memory usage per decoding
session, not per subtransaction. (So we also keep a running tally for
the entire ReorderBuffer.)

There are two open issues with this patch:

One, this mechanism only applies when recording changes. The processing
at commit time still uses the previous hardcoded mechanism. The reason
for this is, AFAIU, that as things currently work, you have to have all
subtransactions in memory to do the final processing. There are some
proposals to change this as well, but they are more involved. Arguably,
per my explanation above, memory use at commit time is less likely to be
a problem.

Two, what to do when the memory limit is reached. With the old
accounting, this was easy, because we'd decide for each subtransaction
independently whether to spill it to disk, when it has reached its 4096
limit. Now, we are looking at a global limit, so we have to find a
transaction to spill in some other way. The proposed patch searches
through the entire list of transactions to find the largest one. But as
the patch says:

"XXX With many subtransactions this might be quite slow, because we'll
have to walk through all of them. There are some options how we could
improve that: (a) maintain some secondary structure with transactions
sorted by amount of changes, (b) not looking for the entirely largest
transaction, but e.g. for transaction using at least some fraction of
the memory limit, and (c) evicting multiple transactions at once, e.g.
to free a given portion of the memory limit (e.g. 50%)."

(a) would create more overhead for the case where everything fits into
memory, so it seems unattractive. Some combination of (b) and (c) seems
useful, but we'd have to come up with something concrete.

Thoughts?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 11 January 2018 at 19:41, Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:

> Two, what to do when the memory limit is reached. With the old
> accounting, this was easy, because we'd decide for each subtransaction
> independently whether to spill it to disk, when it has reached its 4096
> limit. Now, we are looking at a global limit, so we have to find a
> transaction to spill in some other way. The proposed patch searches
> through the entire list of transactions to find the largest one. But as
> the patch says:
>
> "XXX With many subtransactions this might be quite slow, because we'll
> have to walk through all of them. There are some options how we could
> improve that: (a) maintain some secondary structure with transactions
> sorted by amount of changes, (b) not looking for the entirely largest
> transaction, but e.g. for transaction using at least some fraction of
> the memory limit, and (c) evicting multiple transactions at once, e.g.
> to free a given portion of the memory limit (e.g. 50%)."

AIUI spilling to disk doesn't affect absorbing future updates, we
would just keep accumulating them in memory right? We won't need to
unspill until it comes time to commit.

Is there any actual advantage to picking the largest transaction? it
means fewer spills and fewer unspills at commit time but that just a
bigger spike of i/o and more of a chance of spilling more than
necessary to get by. In the end it'll be more or less the same amount
of data read back, just all in one big spike when spilling and one big
spike when committing. If you spilled smaller transactions you would
have a small amount of i/o more frequently and have to read back small
amounts for many commits. But it would add up to the same amount of
i/o (or less if you avoid spilling more than necessary).

The real aim should be to try to pick the transaction that will be
committed furthest in the future. That gives you the most memory to
use for live transactions for the longest time and could let you
process the maximum amount of transactions without spilling them. So
either the oldest transaction (in the expectation that it's been open
a while and appears to be a long-lived batch job that will stay open
for a long time) or the youngest transaction (in the expectation that
all transactions are more or less equally long-lived) might make
sense.

--
greg

On 1/11/18 18:23, Greg Stark wrote:
> AIUI spilling to disk doesn't affect absorbing future updates, we
> would just keep accumulating them in memory right? We won't need to
> unspill until it comes time to commit.

Once a transaction has been serialized, future updates keep accumulating
in memory, until perhaps it gets serialized again. But then at commit
time, if a transaction has been partially serialized at all, all the
remaining changes are also serialized before the whole thing is read
back in (see reorderbuffer.c line 855).

So one optimization would be to specially keep track of all transactions
that have been serialized already and pick those first for further
serialization, because it will be done eventually anyway.

But this is only a secondary optimization, because it doesn't help in
the extreme cases that either no (or few) transactions have been
serialized or all (or most) transactions have been serialized.

> The real aim should be to try to pick the transaction that will be
> committed furthest in the future. That gives you the most memory to
> use for live transactions for the longest time and could let you
> process the maximum amount of transactions without spilling them. So
> either the oldest transaction (in the expectation that it's been open
> a while and appears to be a long-lived batch job that will stay open
> for a long time) or the youngest transaction (in the expectation that
> all transactions are more or less equally long-lived) might make
> sense.

Yes, that makes sense. We'd still need to keep a separate ordered list
of transactions somewhere, but that might be easier if we just order
them in the order we see them.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 01/11/2018 08:41 PM, Peter Eisentraut wrote:
> On 12/22/17 23:57, Tomas Vondra wrote:
>> PART 1: adding logical_work_mem memory limit (0001)
>> ---------------------------------------------------
>>
>> Currently, limiting the amount of memory consumed by logical decoding is
>> tricky (or you might say impossible) for several reasons:
>
> I would like to see some more discussion on this, but I think not a lot
> of people understand the details, so I'll try to write up an explanation
> here. This code is also somewhat new to me, so please correct me if
> there are inaccuracies, while keeping in mind that I'm trying to simplify.
>
> ... snip ...

Thanks for a comprehensive summary of the patch!

>
> "XXX With many subtransactions this might be quite slow, because we'll
> have to walk through all of them. There are some options how we could
> improve that: (a) maintain some secondary structure with transactions
> sorted by amount of changes, (b) not looking for the entirely largest
> transaction, but e.g. for transaction using at least some fraction of
> the memory limit, and (c) evicting multiple transactions at once, e.g.
> to free a given portion of the memory limit (e.g. 50%)."
>
> (a) would create more overhead for the case where everything fits into
> memory, so it seems unattractive. Some combination of (b) and (c) seems
> useful, but we'd have to come up with something concrete.
>

Yeah, when writing that comment I was worried that (a) might get rather
expensive. I was thinking about maintaining a dlist of transactions
sorted by size (ReorderBuffer now only has a hash table), so that we
could evict transactions from the beginning of the list.

But while that speeds up the choice of transactions to evict, the added
cost is rather high, particularly when most transactions are roughly of
the same size. Because in that case we probably have to move the nodes
around in the list quite often. So it seems wiser to just walk the list
once when looking for a victim.

What I'm thinking about instead is tracking just some approximated
version of this - it does not really matter whether we evict the really
largest transaction or one that is a couple of kilobytes smaller. What
we care about is an answer to this question:

Is there some very large transaction that we could evict to free
a lot of memory, or are all transactions fairly small?

So perhaps we can define some "size classes" and track to which of them
each transaction belongs. For example, we could split the memory limit
into 100 buckets, each representing a 1% size increment.

A transaction would not switch the class very often, and it would be
trivial to pick the largest transaction. When all the transactions are
squashed in the smallest classes, we may switch to some alternative
strategy. Not sure.

In any case, I don't really know how expensive the selection actually
is, and if it's an issue. I'll do some measurements.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 01/12/2018 05:35 PM, Peter Eisentraut wrote:
> On 1/11/18 18:23, Greg Stark wrote:
>> AIUI spilling to disk doesn't affect absorbing future updates, we
>> would just keep accumulating them in memory right? We won't need to
>> unspill until it comes time to commit.
>
> Once a transaction has been serialized, future updates keep accumulating
> in memory, until perhaps it gets serialized again. But then at commit
> time, if a transaction has been partially serialized at all, all the
> remaining changes are also serialized before the whole thing is read
> back in (see reorderbuffer.c line 855).
>
> So one optimization would be to specially keep track of all transactions
> that have been serialized already and pick those first for further
> serialization, because it will be done eventually anyway.
>
> But this is only a secondary optimization, because it doesn't help in
> the extreme cases that either no (or few) transactions have been
> serialized or all (or most) transactions have been serialized.
>
>> The real aim should be to try to pick the transaction that will be
>> committed furthest in the future. That gives you the most memory to
>> use for live transactions for the longest time and could let you
>> process the maximum amount of transactions without spilling them. So
>> either the oldest transaction (in the expectation that it's been open
>> a while and appears to be a long-lived batch job that will stay open
>> for a long time) or the youngest transaction (in the expectation that
>> all transactions are more or less equally long-lived) might make
>> sense.
>
> Yes, that makes sense. We'd still need to keep a separate ordered list
> of transactions somewhere, but that might be easier if we just order
> them in the order we see them.
>

Wouldn't the 'toplevel_by_lsn' be suitable for this? Subtransactions
don't really commit independently, but as part of the toplevel xact. And
that list is ordered by LSN, which is pretty much exactly the order in
which we see the transactions.

I feel somewhat uncomfortable about evicting oldest (or youngest)
transactions for based on some assumed correlation with the commit
order. I'm pretty sure that will bite us badly for some workloads.

Another somewhat non-intuitive detail is that because ReorderBuffer
switched to Generation allocator for changes (which usually represent
99% of the memory used during decoding), it does not reuse memory the
way AllocSet does. Actually, it does not reuse memory at all, aiming to
eventually give the memory back to libc (which AllocSet can't do).

Because of this evicting the youngest transactions seems like a quite
bad idea, because those chunks will not be reused and there may be other
chunks on the blocks, preventing their release.

Yeah, complicated stuff.

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 1/12/18 23:19, Tomas Vondra wrote:
> Wouldn't the 'toplevel_by_lsn' be suitable for this? Subtransactions
> don't really commit independently, but as part of the toplevel xact. And
> that list is ordered by LSN, which is pretty much exactly the order in
> which we see the transactions.

Yes indeed. There is even ReorderBufferGetOldestTXN().

> Another somewhat non-intuitive detail is that because ReorderBuffer
> switched to Generation allocator for changes (which usually represent
> 99% of the memory used during decoding), it does not reuse memory the
> way AllocSet does. Actually, it does not reuse memory at all, aiming to
> eventually give the memory back to libc (which AllocSet can't do).
>
> Because of this evicting the youngest transactions seems like a quite
> bad idea, because those chunks will not be reused and there may be other
> chunks on the blocks, preventing their release.

Right. But this raises the question whether we are doing the memory
accounting on the right level. If we are doing all this tracking based
on ReorderBufferChanges, but then serializing changes possibly doesn't
actually free any memory in the operating system, that's no good. Can
we get some usage statistics out of the memory context? It seems like
we need to keep serializing transactions until we actually see the
memory context size drop.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attached is v5, fixing a silly bug in part 0006, causing segfault when
creating a subscription.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 01/19/2018 03:34 PM, Tomas Vondra wrote:
> Attached is v5, fixing a silly bug in part 0006, causing segfault when
> creating a subscription.
>

Meh, there was a bug in the sgml docs (<variable> vs. <varname>),
causing another failure. Hopefully v6 will pass the CI build, it does
pass a build with the same parameters on my system.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Jan 20, 2018 at 7:08 AM, Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
> On 01/19/2018 03:34 PM, Tomas Vondra wrote:
>> Attached is v5, fixing a silly bug in part 0006, causing segfault when
>> creating a subscription.
>>
>
> Meh, there was a bug in the sgml docs (<variable> vs. <varname>),
> causing another failure. Hopefully v6 will pass the CI build, it does
> pass a build with the same parameters on my system.

Thank you for working on this. This patch would be helpful for
synchronous replication.

I haven't looked at the code deeply yet, but I've reviewed the v6
patch set especially on subscriber side. All of the patches can be
applied to current HEAD cleanly. Here is review comment.

----
CREATE SUBSCRIPTION commands accept work_mem < 64 but it leads ERROR
on publisher side when starting replication. Probably we should check
the value on the subscriber side as well.

----
When streaming = on, if we drop subscription in the middle of
receiving stream changes, DROP SUBSCRIPTION could leak tmp files
(.chages file and .subxacts file). Also it also happens when a
transaction on upstream aborted without abort record.

----
Since we can change both streaming option and work_mem option by ALTER
SUBSCRIPTION, documentation of ALTER SUBSCRIPTION needs to be updated.

----
If we create a subscription without any options, both
pg_subscription.substream and pg_subscription.subworkmem are set to
null. However, since GetSubscription are't aware of NULL we start the
replication with invalid options like follows.
LOG: received replication command: START_REPLICATION SLOT "hoge_sub"
LOGICAL 0/0 (proto_version '2', work_mem '893219954', streaming 'on',
publication_names '"hoge_pub"')

I think we can set substream to false and subworkmem to -1 instead of
null, and then makes libpqrcv_startstreaming not set streaming option
if stream is -1.

----
Some WARNING messages appeared. Maybe these are for debug purpose?

WARNING: updating stream stats 0x1c12ef8 4 3 65604
WARNING: UpdateSpillStats: updating stats 0x1c12ef8 0 0 0 39 41 2632080

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

To close out this commit fest, I'm setting both of these patches as
returned with feedback, as there are apparently significant issues to be
addressed. Feel free to move them to the next commit fest when you
think they are ready to be continued.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 01/31/2018 07:53 AM, Masahiko Sawada wrote:
> On Sat, Jan 20, 2018 at 7:08 AM, Tomas Vondra
> <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>> On 01/19/2018 03:34 PM, Tomas Vondra wrote:
>>> Attached is v5, fixing a silly bug in part 0006, causing segfault when
>>> creating a subscription.
>>>
>>
>> Meh, there was a bug in the sgml docs (<variable> vs. <varname>),
>> causing another failure. Hopefully v6 will pass the CI build, it does
>> pass a build with the same parameters on my system.
>
> Thank you for working on this. This patch would be helpful for
> synchronous replication.
>
> I haven't looked at the code deeply yet, but I've reviewed the v6
> patch set especially on subscriber side. All of the patches can be
> applied to current HEAD cleanly. Here is review comment.
>
> ----
> CREATE SUBSCRIPTION commands accept work_mem < 64 but it leads ERROR
> on publisher side when starting replication. Probably we should check
> the value on the subscriber side as well.
>
> ----
> When streaming = on, if we drop subscription in the middle of
> receiving stream changes, DROP SUBSCRIPTION could leak tmp files
> (.chages file and .subxacts file). Also it also happens when a
> transaction on upstream aborted without abort record.
>
> ----
> Since we can change both streaming option and work_mem option by ALTER
> SUBSCRIPTION, documentation of ALTER SUBSCRIPTION needs to be updated.
>
> ----
> If we create a subscription without any options, both
> pg_subscription.substream and pg_subscription.subworkmem are set to
> null. However, since GetSubscription are't aware of NULL we start the
> replication with invalid options like follows.
> LOG: received replication command: START_REPLICATION SLOT "hoge_sub"
> LOGICAL 0/0 (proto_version '2', work_mem '893219954', streaming 'on',
> publication_names '"hoge_pub"')
>
> I think we can set substream to false and subworkmem to -1 instead of
> null, and then makes libpqrcv_startstreaming not set streaming option
> if stream is -1.
>
> ----
> Some WARNING messages appeared. Maybe these are for debug purpose?
>
> WARNING: updating stream stats 0x1c12ef8 4 3 65604
> WARNING: UpdateSpillStats: updating stats 0x1c12ef8 0 0 0 39 41 2632080
>
> Regards,
>

Thanks for the review! I'll address the issues in the next version of
the patch.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 02/01/2018 03:51 PM, Peter Eisentraut wrote:
> To close out this commit fest, I'm setting both of these patches as
> returned with feedback, as there are apparently significant issues to be
> addressed. Feel free to move them to the next commit fest when you
> think they are ready to be continued.
>

Will do. Thanks for the feedback.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-02-01 23:51:55 +0100, Tomas Vondra wrote:
> On 02/01/2018 03:51 PM, Peter Eisentraut wrote:
> > To close out this commit fest, I'm setting both of these patches as
> > returned with feedback, as there are apparently significant issues to be
> > addressed. Feel free to move them to the next commit fest when you
> > think they are ready to be continued.
> >
>
> Will do. Thanks for the feedback.

Hm, this CF entry is marked as needs review as of 2018-03-01 12:54:48,
but I don't see a newer version posted?

Greetings,

Andres Freund

On 03/02/2018 02:12 AM, Andres Freund wrote:
> On 2018-02-01 23:51:55 +0100, Tomas Vondra wrote:
>> On 02/01/2018 03:51 PM, Peter Eisentraut wrote:
>>> To close out this commit fest, I'm setting both of these patches as
>>> returned with feedback, as there are apparently significant issues to be
>>> addressed. Feel free to move them to the next commit fest when you
>>> think they are ready to be continued.
>>>
>>
>> Will do. Thanks for the feedback.
>
> Hm, this CF entry is marked as needs review as of 2018-03-01 12:54:48,
> but I don't see a newer version posted?
>

Ah, apologies - that's due to moving the patch from the last CF (it was
marked as RWF so I had to reopen it before moving it). I'll submit a new
version of the patch shortly, please mark it as WOA until then.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi Tomas.

On 3/1/18 9:33 PM, Tomas Vondra wrote:
> On 03/02/2018 02:12 AM, Andres Freund wrote:
>> On 2018-02-01 23:51:55 +0100, Tomas Vondra wrote:
>>> On 02/01/2018 03:51 PM, Peter Eisentraut wrote:
>>>> To close out this commit fest, I'm setting both of these patches as
>>>> returned with feedback, as there are apparently significant issues to be
>>>> addressed. Feel free to move them to the next commit fest when you
>>>> think they are ready to be continued.
>>>>
>>>
>>> Will do. Thanks for the feedback.
>>
>> Hm, this CF entry is marked as needs review as of 2018-03-01 12:54:48,
>> but I don't see a newer version posted?
>>
>
> Ah, apologies - that's due to moving the patch from the last CF (it was
> marked as RWF so I had to reopen it before moving it). I'll submit a new
> version of the patch shortly, please mark it as WOA until then.

Marked as Waiting on Author.

--
-David
david(at)pgmasters(dot)net

Hi,

On 2018-03-01 21:39:36 -0500, David Steele wrote:
> On 3/1/18 9:33 PM, Tomas Vondra wrote:
> > On 03/02/2018 02:12 AM, Andres Freund wrote:
> > > Hm, this CF entry is marked as needs review as of 2018-03-01 12:54:48,
> > > but I don't see a newer version posted?
> > >
> >
> > Ah, apologies - that's due to moving the patch from the last CF (it was
> > marked as RWF so I had to reopen it before moving it). I'll submit a new
> > version of the patch shortly, please mark it as WOA until then.
>
> Marked as Waiting on Author.

Sorry to be the hard-ass, but given this patch hasn't been moved forward
since 2018-01-19, I'm not sure why it's elegible to be in this CF in the
first place?

Greetings,

Andres Freund

On Thu, Mar 1, 2018 at 9:33 PM, Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
> Ah, apologies - that's due to moving the patch from the last CF (it was
> marked as RWF so I had to reopen it before moving it). I'll submit a new
> version of the patch shortly, please mark it as WOA until then.

So, the way it's supposed to work is you resubmit the patch first and
then re-activate the CF entry. If you get to re-activate the CF entry
without actually updating the patch, and then submit the patch
afterwards, then the CF deadline becomes largely meaningless. I think
a new patch should rejected as untimely.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 3/2/18 3:06 PM, Robert Haas wrote:
> On Thu, Mar 1, 2018 at 9:33 PM, Tomas Vondra
> <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>> Ah, apologies - that's due to moving the patch from the last CF (it was
>> marked as RWF so I had to reopen it before moving it). I'll submit a new
>> version of the patch shortly, please mark it as WOA until then.
>
> So, the way it's supposed to work is you resubmit the patch first and
> then re-activate the CF entry. If you get to re-activate the CF entry
> without actually updating the patch, and then submit the patch
> afterwards, then the CF deadline becomes largely meaningless. I think
> a new patch should rejected as untimely.

Hmmm, I missed that implication last night. I'll mark this Returned
with Feedback.

Tomas, please move to the next CF once you have an updated patch.

Thanks,
--
-David
david(at)pgmasters(dot)net

On 03/02/2018 09:21 PM, David Steele wrote:
> On 3/2/18 3:06 PM, Robert Haas wrote:
>> On Thu, Mar 1, 2018 at 9:33 PM, Tomas Vondra
>> <tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>>> Ah, apologies - that's due to moving the patch from the last CF (it was
>>> marked as RWF so I had to reopen it before moving it). I'll submit a new
>>> version of the patch shortly, please mark it as WOA until then.
>>
>> So, the way it's supposed to work is you resubmit the patch first and
>> then re-activate the CF entry. If you get to re-activate the CF entry
>> without actually updating the patch, and then submit the patch
>> afterwards, then the CF deadline becomes largely meaningless. I think
>> a new patch should rejected as untimely.
>
> Hmmm, I missed that implication last night. I'll mark this Returned
> with Feedback.
>
> Tomas, please move to the next CF once you have an updated patch.
>

Can you guys please point me to the CF rules that say this? Because my
understanding (and not just mine, AFAICS) was obviously different.
Clearly there's a disconnect somewhere.

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi there,

attached is an updated patch fixing all the reported issues (a bit more
about those below).

The main change in this patch version is reworked logging of subxact
assignments, which needs to be done immediately for incremental decoding
to work properly.

The previous patch versions did that by logging a separate xlog record,
which however had rather noticeable space overhead (~40% on a worst-case
test - tiny table, no FPWs, ...). While in practice the overhead would
be much closer to 0%, it still seemed unacceptable.

Andres proposed doing something like we do with replication origins in
XLogRecordAssemble, i.e. inventing a special block, and embedding the
assignment info into that (in the next xlog record). This turned out to
be working quite well, and the worst-case space overhead dropped to ~5%.

I have attempted to do something like that with the invalidations, which
is the other thing that needs to be logged immediately for incremental
decoding to work correctly. The plan was to use the same approach as for
assignments, i.e. embed the invalidations into the next xlog record and
stop sending them in the commit message. That however turned out to be
much more complicated - the embedding is fairly trivial, of course, but
unlike assignments the invalidations are needed for hot standbys. If we
only send them incrementally, I think the standby would have to collect
from the WAL records, and store them in a way that survives restarts.

So for invalidations the patch uses the original approach with a new
type xlog record type (ignored by standby), and still logging the
invalidations in commit record (which is that the standby relies on).

On 02/01/2018 11:50 PM, Tomas Vondra wrote:
> On 01/31/2018 07:53 AM, Masahiko Sawada wrote:
> ...
>> ----
>> CREATE SUBSCRIPTION commands accept work_mem < 64 but it leads ERROR
>> on publisher side when starting replication. Probably we should check
>> the value on the subscriber side as well.
>>

Added.

>> ----
>> When streaming = on, if we drop subscription in the middle of
>> receiving stream changes, DROP SUBSCRIPTION could leak tmp files
>> (.chages file and .subxacts file). Also it also happens when a
>> transaction on upstream aborted without abort record.
>>

Right. The files would get cleaned up eventually during restart (just
like other temporary files), but leaking them after DROP SUBSCRIPTION is
not cool. So I've added a simple tracking of files (or rather streamed
XIDs) in the worker, and clean them explicitly on exit.

>> ----
>> Since we can change both streaming option and work_mem option by ALTER
>> SUBSCRIPTION, documentation of ALTER SUBSCRIPTION needs to be updated.
>>

Yep, I've added note that work_mem and streaming can also be changed.
Those changes won't be applied to the already running worker, though.

>> ----
>> If we create a subscription without any options, both
>> pg_subscription.substream and pg_subscription.subworkmem are set to
>> null. However, since GetSubscription are't aware of NULL we start the
>> replication with invalid options like follows.
>> LOG: received replication command: START_REPLICATION SLOT "hoge_sub"
>> LOGICAL 0/0 (proto_version '2', work_mem '893219954', streaming 'on',
>> publication_names '"hoge_pub"')
>>
>> I think we can set substream to false and subworkmem to -1 instead of
>> null, and then makes libpqrcv_startstreaming not set streaming option
>> if stream is -1.
>>

Good catch! I've done pretty much what you suggested here, i.e. store
-1/false instead and then handle that in libpqrcv_startstreaming.

>> ----
>> Some WARNING messages appeared. Maybe these are for debug purpose?
>>
>> WARNING: updating stream stats 0x1c12ef8 4 3 65604
>> WARNING: UpdateSpillStats: updating stats 0x1c12ef8 0 0 0 39 41 2632080
>>

Yeah, those should be removed.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 03/02/2018 09:05 PM, Andres Freund wrote:
> Hi,
>
> On 2018-03-01 21:39:36 -0500, David Steele wrote:
>> On 3/1/18 9:33 PM, Tomas Vondra wrote:
>>> On 03/02/2018 02:12 AM, Andres Freund wrote:
>>>> Hm, this CF entry is marked as needs review as of 2018-03-01 12:54:48,
>>>> but I don't see a newer version posted?
>>>>
>>>
>>> Ah, apologies - that's due to moving the patch from the last CF (it was
>>> marked as RWF so I had to reopen it before moving it). I'll submit a new
>>> version of the patch shortly, please mark it as WOA until then.
>>
>> Marked as Waiting on Author.
>
> Sorry to be the hard-ass, but given this patch hasn't been moved forward
> since 2018-01-19, I'm not sure why it's elegible to be in this CF in the
> first place?
>

That is somewhat misleading, I think. You're right the last version was
submitted on 2018-01-19, but the next review arrived on 2018-01-31, i.e.
right at the end of the CF. So it's not like the patch was sitting there
with unresolved issues. Based on that review the patch was marked as RWF
and thus not moved to 2018-03 automatically.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-03-03 02:00:46 +0100, Tomas Vondra wrote:
> That is somewhat misleading, I think. You're right the last version was
> submitted on 2018-01-19, but the next review arrived on 2018-01-31, i.e.
> right at the end of the CF. So it's not like the patch was sitting there
> with unresolved issues. Based on that review the patch was marked as RWF
> and thus not moved to 2018-03 automatically.

I don't see how this changes anything.

- Andres

On 03/03/2018 02:01 AM, Andres Freund wrote:
> On 2018-03-03 02:00:46 +0100, Tomas Vondra wrote:
>> That is somewhat misleading, I think. You're right the last version
>> was submitted on 2018-01-19, but the next review arrived on
>> 2018-01-31, i.e. right at the end of the CF. So it's not like the
>> patch was sitting there with unresolved issues. Based on that
>> review the patch was marked as RWF and thus not moved to 2018-03
>> automatically.
>
> I don't see how this changes anything.
>

You've used "The patch hasn't moved forward since 2018-01-19," as an
argument why the patch is not eligible for 2018-03. I suggest that
argument is misleading, because patches generally do not move without
reviews, and it's difficult to respond to a review that arrives on the
last day of a commitfest.

Consider that without the review, the patch would end up with NR status,
and would be moved to the next CF automatically. Isn't that a bit weird?

kind regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-03-03 02:34:06 +0100, Tomas Vondra wrote:
> On 03/03/2018 02:01 AM, Andres Freund wrote:
> > On 2018-03-03 02:00:46 +0100, Tomas Vondra wrote:
> >> That is somewhat misleading, I think. You're right the last version
> >> was submitted on 2018-01-19, but the next review arrived on
> >> 2018-01-31, i.e. right at the end of the CF. So it's not like the
> >> patch was sitting there with unresolved issues. Based on that
> >> review the patch was marked as RWF and thus not moved to 2018-03
> >> automatically.
> >
> > I don't see how this changes anything.
> >
>
> You've used "The patch hasn't moved forward since 2018-01-19," as an
> argument why the patch is not eligible for 2018-03. I suggest that
> argument is misleading, because patches generally do not move without
> reviews, and it's difficult to respond to a review that arrives on the
> last day of a commitfest.
>
> Consider that without the review, the patch would end up with NR status,
> and would be moved to the next CF automatically. Isn't that a bit weird?

Not sure I follow. The point is that nobody would have complained if
you'd moved the patch into this fest if you'd updated it *before* it
started?

Greetings,

Andres Freund

On 3/2/18 8:01 PM, Andres Freund wrote:
> On 2018-03-03 02:00:46 +0100, Tomas Vondra wrote:
>> That is somewhat misleading, I think. You're right the last version was
>> submitted on 2018-01-19, but the next review arrived on 2018-01-31, i.e.
>> right at the end of the CF. So it's not like the patch was sitting there
>> with unresolved issues. Based on that review the patch was marked as RWF
>> and thus not moved to 2018-03 automatically.
>
> I don't see how this changes anything.

I agree that things could be clearer, and Andres has produced a great
document that we can build on. The old one had gotten a bit stale.

However, I think it's pretty obvious that a CF entry should be
accompanied with a patch. It sounds like the timing was awkward but you
still had 28 days to produce a new patch.

I also notice that you submitted 7 patches in this CF but are reviewing
zero.

--
-David
david(at)pgmasters(dot)net

On 03/03/2018 02:37 AM, David Steele wrote:
> On 3/2/18 8:01 PM, Andres Freund wrote:
>> On 2018-03-03 02:00:46 +0100, Tomas Vondra wrote:
>>> That is somewhat misleading, I think. You're right the last version was
>>> submitted on 2018-01-19, but the next review arrived on 2018-01-31, i.e.
>>> right at the end of the CF. So it's not like the patch was sitting there
>>> with unresolved issues. Based on that review the patch was marked as RWF
>>> and thus not moved to 2018-03 automatically.
>>
>> I don't see how this changes anything.
>
> I agree that things could be clearer, and Andres has produced a great
> document that we can build on. The old one had gotten a bit stale.
>
> However, I think it's pretty obvious that a CF entry should be
> accompanied with a patch. It sounds like the timing was awkward but
> you still had 28 days to produce a new patch.
>

Based on internal discussion I'm not so sure about the "pretty obvious"
part. It certainly wasn't that obvious to me, otherwise I'd submit the
revised patch earlier - hindsight is 20/20.

> I also notice that you submitted 7 patches in this CF but are
> reviewing zero.
>

I've volunteered to review a couple of patches at the FOSDEM Developer
Meeting - I thought Stephen was entering that into the CF app, not sure
where it got lost.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 3/2/18 8:54 PM, Tomas Vondra wrote:
> On 03/03/2018 02:37 AM, David Steele wrote:
>> On 3/2/18 8:01 PM, Andres Freund wrote:
>>> On 2018-03-03 02:00:46 +0100, Tomas Vondra wrote:
>>>> That is somewhat misleading, I think. You're right the last version was
>>>> submitted on 2018-01-19, but the next review arrived on 2018-01-31, i.e.
>>>> right at the end of the CF. So it's not like the patch was sitting there
>>>> with unresolved issues. Based on that review the patch was marked as RWF
>>>> and thus not moved to 2018-03 automatically.
>>>
>>> I don't see how this changes anything.
>>
>> I agree that things could be clearer, and Andres has produced a great
>> document that we can build on. The old one had gotten a bit stale.
>>
>> However, I think it's pretty obvious that a CF entry should be
>> accompanied with a patch. It sounds like the timing was awkward but
>> you still had 28 days to produce a new patch.
>
> Based on internal discussion I'm not so sure about the "pretty obvious"
> part. It certainly wasn't that obvious to me, otherwise I'd submit the
> revised patch earlier - hindsight is 20/20.

Indeed it is. Be assured that nobody takes pleasure in pushing patches,
but we have limited resources and must make some choices.

>> I also notice that you submitted 7 patches in this CF but are
>> reviewing zero.
>
> I've volunteered to review a couple of patches at the FOSDEM Developer
> Meeting - I thought Stephen was entering that into the CF app, not sure
> where it got lost.

There are plenty of patches that need review, so go for it.

Regards,
--
-David
david(at)pgmasters(dot)net

On 2018-03-03 01:55, Tomas Vondra wrote:
> Hi there,
>
> attached is an updated patch fixing all the reported issues (a bit more
> about those below).

Hi,

0007-Track-statistics-for-streaming-spilling.patch won't apply. All
the other patches apply ok.

patch complaints with:

patching file doc/src/sgml/monitoring.sgml
patching file src/backend/catalog/system_views.sql
Hunk #1 succeeded at 734 (offset 2 lines).
patching file src/backend/replication/logical/reorderbuffer.c
patching file src/backend/replication/walsender.c
patching file src/include/catalog/pg_proc.h
Hunk #1 FAILED at 2903.
1 out of 1 hunk FAILED -- saving rejects to file
src/include/catalog/pg_proc.h.rej
patching file src/include/replication/reorderbuffer.h
patching file src/include/replication/walsender_private.h
patching file src/test/regress/expected/rules.out
Hunk #1 succeeded at 1861 (offset 2 lines).

Attached the produced reject file.

thanks,

Erik Rijkers

On 03/03/2018 06:19 AM, Erik Rijkers wrote:
> On 2018-03-03 01:55, Tomas Vondra wrote:
>> Hi there,
>>
>> attached is an updated patch fixing all the reported issues (a bit more
>> about those below).
>
> Hi,
>
> 0007-Track-statistics-for-streaming-spilling.patch  won't apply.  All
> the other patches apply ok.
>
> patch complaints with:
>
> patching file doc/src/sgml/monitoring.sgml
> patching file src/backend/catalog/system_views.sql
> Hunk #1 succeeded at 734 (offset 2 lines).
> patching file src/backend/replication/logical/reorderbuffer.c
> patching file src/backend/replication/walsender.c
> patching file src/include/catalog/pg_proc.h
> Hunk #1 FAILED at 2903.
> 1 out of 1 hunk FAILED -- saving rejects to file
> src/include/catalog/pg_proc.h.rej
> patching file src/include/replication/reorderbuffer.h
> patching file src/include/replication/walsender_private.h
> patching file src/test/regress/expected/rules.out
> Hunk #1 succeeded at 1861 (offset 2 lines).
>
> Attached the produced reject file.
>

Yeah, that's due to fd1a421fe66 which changed columns in pg_proc.h.
Attached is a rebased patch, fixing this.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

I think this patch is not going to be ready for PG11.

- It depends on some work in the thread "logical decoding of two-phase
transactions", which is still in progress.

- Various details in the logical_work_mem patch (0001) are unresolved.

- This being partially a performance feature, we haven't seen any
performance tests (e.g., which settings result in which latencies under
which workloads).

That said, the feature seems useful and desirable, and the
implementation makes sense. There are documentation and tests. But
there is a significant amount of design and coding work still necessary.

Attached is a fixup patch that I needed to make it compile.

The last two patches in your series (0008, 0009) are labeled as bug
fixes. Would you like to argue that they should be applied
independently of the rest of the feature?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 11.01.2018 22:41, Peter Eisentraut wrote:
> On 12/22/17 23:57, Tomas Vondra wrote:
>> PART 1: adding logical_work_mem memory limit (0001)
>> ---------------------------------------------------
>>
>> Currently, limiting the amount of memory consumed by logical decoding is
>> tricky (or you might say impossible) for several reasons:
> I would like to see some more discussion on this, but I think not a lot
> of people understand the details, so I'll try to write up an explanation
> here. This code is also somewhat new to me, so please correct me if
> there are inaccuracies, while keeping in mind that I'm trying to simplify.
>
> The data in the WAL is written as it happens, so the changes belonging
> to different transactions are all mixed together. One of the jobs of
> logical decoding is to reassemble the changes belonging to each
> transaction. The top-level data structure for that is the infamous
> ReorderBuffer. So as it reads the WAL and sees something about a
> transaction, it keeps a copy of that change in memory, indexed by
> transaction ID (ReorderBufferChange). When the transaction commits, the
> accumulated changes are passed to the output plugin and then freed. If
> the transaction aborts, then changes are just thrown away.
>
> So when logical decoding is active, a copy of the changes for each
> active transaction is kept in memory (once per walsender).
>
> More precisely, the above happens for each subtransaction. When the
> top-level transaction commits, it finds all its subtransactions in the
> ReorderBuffer, reassembles everything in the right order, then invokes
> the output plugin.
>
> All this could end up using an unbounded amount of memory, so there is a
> mechanism to spill changes to disk. The way this currently works is
> hardcoded, and this patch proposes to change that.
>
> Currently, when a transaction or subtransaction has accumulated 4096
> changes, it is spilled to disk. When the top-level transaction commits,
> things are read back from disk to do the final processing mentioned above.
>
> This all works mostly fine, but you can construct some more extreme
> cases where this can blow up.
>
> Here is a mundane example. Let's say a change entry takes 100 bytes (it
> might contain a new row, or an update key and some new column values,
> for example). If you have 100 concurrent active sessions and no
> subtransactions, then logical decoding memory is bounded by 4096 * 100 *
> 100 = 40 MB (per walsender) before things spill to disk.
>
> Now let's say you are using a lot of subtransactions, because you are
> using PL functions, exception handling, triggers, doing batch updates.
> If you have 200 subtransactions on average per concurrent session, the
> memory usage bound in that case would be 4096 * 100 * 100 * 200 = 8 GB
> (per walsender). And so on. If you have more concurrent sessions or
> larger changes or more subtransactions, you'll use much more than those
> 8 GB. And if you don't have those 8 GB, then you're stuck at this point.
>
> That is the consideration when we record changes, but we also need
> memory when we do the final processing at commit time. That is slightly
> less problematic because we only process one top-level transaction at a
> time, so the formula is only 4096 * avg_size_of_changes * nr_subxacts
> (without the concurrent sessions factor).
>
> So, this patch proposes to improve this as follows:
>
> - We compute the actual size of each ReorderBufferChange and keep a
> running tally for each transaction, instead of just counting the number
> of changes.
>
> - We have a configuration setting that allows us to change the limit
> instead of the hardcoded 4096. The configuration setting is also in
> terms of memory, not in number of changes.
>
> - The configuration setting is for the total memory usage per decoding
> session, not per subtransaction. (So we also keep a running tally for
> the entire ReorderBuffer.)
>
> There are two open issues with this patch:
>
> One, this mechanism only applies when recording changes. The processing
> at commit time still uses the previous hardcoded mechanism. The reason
> for this is, AFAIU, that as things currently work, you have to have all
> subtransactions in memory to do the final processing. There are some
> proposals to change this as well, but they are more involved. Arguably,
> per my explanation above, memory use at commit time is less likely to be
> a problem.
>
> Two, what to do when the memory limit is reached. With the old
> accounting, this was easy, because we'd decide for each subtransaction
> independently whether to spill it to disk, when it has reached its 4096
> limit. Now, we are looking at a global limit, so we have to find a
> transaction to spill in some other way. The proposed patch searches
> through the entire list of transactions to find the largest one. But as
> the patch says:
>
> "XXX With many subtransactions this might be quite slow, because we'll
> have to walk through all of them. There are some options how we could
> improve that: (a) maintain some secondary structure with transactions
> sorted by amount of changes, (b) not looking for the entirely largest
> transaction, but e.g. for transaction using at least some fraction of
> the memory limit, and (c) evicting multiple transactions at once, e.g.
> to free a given portion of the memory limit (e.g. 50%)."
>
> (a) would create more overhead for the case where everything fits into
> memory, so it seems unattractive. Some combination of (b) and (c) seems
> useful, but we'd have to come up with something concrete.
>
> Thoughts?
>

I am very sorry that I have not noticed this thread before.
Spilling to the file in reorder buffer is the main factor limiting speed
of importing data in multimaster and shardman (sharding based on FDW
with redundancy provided by LR).
This is why we think a lot about possible ways of addressing this issue.
Right now data of huge transaction is written to the disk three times
before it is applied at replica. And obviously read also three times.
First it is saved in WAL, then spilled to the disk by reorder buffer and
once again spilled to the disk at replica before assignment to the
particular apply worker
(last one is specific of multimaster, which can apply received
transactions concurrently).

We considered three different approaches:
1. Streaming. It is similar with the proposed patch, the main difference
is that we do not want to spill transaction in temporary file at
replica, but apply it immediately in separate backend and abort
transaction if it is aborted at master. Certainly it will work only with
2PC.
2. Elimination of spilling by rescanning WAL.
3. Bypass WAL: add hooks to heapam to buffer and propagate changes
immediately to replica and apply them in dedicated backend.
I have implemented prototype of such replication. With one replica it
shows about 1.5x slowdown comparing with standalone/async LR and about
2-3 improvement comparing with sync LR. For two replicas result is 2x
slower than async LR and 2-8 times faster than sync LR (depending on
number of concurrent connections).

Approach 3) seems to be specific to multimaster/shardman, so most likely
it can not be considered for general LR.
So I want to compare 1 and 2. Did you ever though about something like 2?

Right now in the proposed patch you just move spilling to the file from
master to replica.
It still can make sense to avoid memory overflow and reduce disk IO at
master.
But if we have just one huge transaction (COPY) importing gigabytes of
data to the database,
then performance will be almost the same with your patch or without it.
The only difference is where we serialize transaction: at master or at
replica side.
In this sense this patch doesn't solve the problem with slow load of
large bulks of data though LR.

Alternatively (approach 2) we can have small in-memory buffer for
decoding transaction and remember LSN and snapshot of this transaction
start.
In case of buffer overflow we just continue WAL traversal until we reach
end of the transaction. After it we restart scanning WAL from the
beginning of this transaction at this second pass
send changes directly to the output plugin. So we have to scan WAL
several times but do not need to spill anything to the disk neither at
publisher, neither at subscriber side.
Certainly this approach will be inefficient if we have several long
interleaving transactions. But in most customer's use cases we have
observed until now there is just one huge transaction performing bulk load.
May be I missed something, but this approach seems to be easier for
implementation than transaction streaming. And it doesn't require any
changes in output plugin API.
I realize that it is a little bit late to ask this question once your
patch is almost ready, but what do you think about it? Are there some
pitfals with this approach?

There is one more aspect and performance problem with LR we have faced
with shardman: if there are several publications for different subsets
of table at one instance,
then WAL senders have to do a lot of useless work. Them are decoding
transactions which have no relation to this publication. But WAL sender
doesn't know it until it reaches the end of this transaction. What is
worser: if transaction is huge, then all WAL senders will spill it to
the disk even through only one of them actually needs it. So data of
huge transaction is written not three times, but N times, where N is
number of publications. The only solution of the problem we can imagine
is to let backend somehow inform WAL sender (through shared message queue?)
about LSN-s it should considered. In this case WAL sender can skip large
portions of WAL without decoding. We also want to know opinion of
2ndQuandarnt about this idea.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

This patch set was not updated for the 2018-07 commitfest, so moved to -09.

On 09.03.18 17:07, Peter Eisentraut wrote:
> I think this patch is not going to be ready for PG11.
>
> - It depends on some work in the thread "logical decoding of two-phase
> transactions", which is still in progress.
>
> - Various details in the logical_work_mem patch (0001) are unresolved.
>
> - This being partially a performance feature, we haven't seen any
> performance tests (e.g., which settings result in which latencies under
> which workloads).
>
> That said, the feature seems useful and desirable, and the
> implementation makes sense. There are documentation and tests. But
> there is a significant amount of design and coding work still necessary.
>
> Attached is a fixup patch that I needed to make it compile.
>
> The last two patches in your series (0008, 0009) are labeled as bug
> fixes. Would you like to argue that they should be applied
> independently of the rest of the feature?
>

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Mar 03, 2018 at 03:52:40PM +0100, Tomas Vondra wrote:
> Yeah, that's due to fd1a421fe66 which changed columns in pg_proc.h.
> Attached is a rebased patch, fixing this.

The latest patch set does not apply anymore, and had no activity for the
last two months, so I am marking it as returned with feedback.
--
Michael

Hi,

Attached is an updated version of this patch series. It's meant to be
applied on top of the 2pc decoding patch [1], because streaming of
in-progress transactions requires handling of concurrent aborts. So it
may or may not apply directly to master, I'm not sure - unfortunately
that's likely to confuse the cputube thing, but I don't want to include
the 2pc decoding bits here because that would be just confusing.

If needed, the part introducing logical_work_mem limit for ReorderBuffer
can be separated and committed independently, but I do expect this to be
committed after the 2pc decoding patch so I've left it like this.

This new version is mostly just a rebase to current master (or almost,
because 2pc decoding only applies to 29180e5d78 due to minor bitrot),
but it also addresses the new stuff committed since last version (most
importantly decoding of TRUNCATE). It also fixes a bug in WAL-logging of
subxact assignments, where the assignment was included in records with
XID=0, essentially failing to track the subxact properly.

For the logical_work_mem part, I think this is quite solid. The main
question is how to pick transactions for eviction. For now it uses the
same approach as master (i.e. picking the largest top-level transaction,
although measured by amount of memory and not just number of changes).

But I've realized that may not work with Generation context that great,
because unlike AllocSet it does not reuse the memory. That's nice as it
allows freeing old blocks (which AllocSet can't), but it means a small
transaction can have a change on old blocks preventing free(). That is
something we have in pg11 already, because that's where Generation
context got introduced - I haven't seen this issue in practice, but we
might need to do something about it.

In any case, I'm thinking we may need to pick a different eviction
algorithm - say using a transaction with the oldest change (and loop
until we release at least one block in the Generation context), or maybe
look for block mixing changes from the smallest number of transactions,
or something like that. Other ideas are welcome. I don't think the exact
algorithm is particularly critical, because it's meant to be triggered
only very rarely (i.e. pick logical_work_mem high enough).

The in-progress streaming is mostly mechanical extension of existing
functionality (new methods in various APIs, ...) and refactoring of
ReorderBuffer to handle incremental decoding. I'm sure it'd benefit from
reviews, of course.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

FWIW the original CF entry in 2018-07 [1] was marked as RWF. I'm not
sure what's the right way to resubmit such patches, so I've created a
new entry in 2019-01 [2] referencing the same hackers thread (and with
the same authors/reviewers metadata).

[1] https://commitfest.postgresql.org/19/1429/
[2] https://commitfest.postgresql.org/21/1927/

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi Tomas,

> This new version is mostly just a rebase to current master (or almost,
> because 2pc decoding only applies to 29180e5d78 due to minor bitrot),
> but it also addresses the new stuff committed since last version (most
> importantly decoding of TRUNCATE). It also fixes a bug in WAL-logging of
> subxact assignments, where the assignment was included in records with
> XID=0, essentially failing to track the subxact properly.

I started reviewing your patch about a month ago and tried to do an
in-depth review, since I am very interested in this patch too. The new
version is not applicable to master 29180e5d78, but everything is OK
after applying 2pc patch before. Anyway, I guess it may complicate
further testing and review, since any potential reviewer has to take
into account both patches at once. Previous version was applicable to
master and was working fine for me separately (excepting a few
patch-specific issues, which I try to explain below).

Patch review
========

First of all, I want to say thank you for such a huge work done. Here
are some problems, which I have found and hopefully fixed with my
additional patch (please, find attached, it should be applicable to the
last commit of your newest patch version):

1) The most important issue is that your tap tests were broken—there was
missing option "WITH (streaming=true)" in the subscription creating
statement. Therefore, spilling mechanism has been tested rather than
streaming.

2) After fixing tests the first one with simple streaming is immediately
failed, because of logical replication worker segmentation fault. It
happens, since worker tries to call stream_cleanup_files inside
stream_open_file at the stream start, while nxids is zero, then it goes
to the negative value and everything crashes. Something similar may
happen with xids array, so I added two checks there.

3) The next problem is much more critical and is dedicated to historic
MVCC visibility rules. Previously, walsender was starting to decode
transaction on commit and we were able to resolve all xmin, xmax,
combocids to cmin/cmax, build tuplecids hash and so on, but now we start
doing all these things on the fly.

Thus, rather difficult situation arises: HeapTupleSatisfiesHistoricMVCC
is trying to validate catalog tuples, which are currently in the future
relatively to the current decoder position inside transaction, e.g. we
may want to resolve cmin/cmax of a tuple, which was created with cid 3
and deleted with cid 5, while we are currently at cid 4, so our
tuplecids hash is not complete to handle such a case.

I have updated HeapTupleSatisfiesHistoricMVCC visibility rules with two
options:

/*
 * If we accidentally see a tuple from our transaction, but cannot
resolve its
 * cmin, so probably it is from the future, thus drop it.
 */
if (!resolved)
    return false;

and

/*
 * If we accidentally see a tuple from our transaction, but cannot
resolve its
 * cmax or cmax == InvalidCommandId, so probably it is still valid,
thus accept it.
 */
if (!resolved || cmax == InvalidCommandId)
    return true;

4) There was a problem with marking top-level transaction as having
catalog changes if one of its subtransactions has. It was causing a
problem with DDL statements just after subtransaction start (savepoint),
so data from new columns is not replicated.

5) Similar issue with schema send. You send schema only once per each
sub/transaction (IIRC), while we have to update schema on each catalog
change: invalidation execution, snapshot rebuild, adding new tuple cids.
So I ended up with adding is_schema_send flag to ReorderBufferTXN, since
it is easy to set it inside RB and read in the output plugin. Probably,
we have to choose a better place for this flag.

6) To better handle all these tricky cases I added new tap
test—014_stream_tough_ddl.pl—which consist of really tough combination
of DDL, DML, savepoints and ROLLBACK/RELEASE in a single transaction.

I marked all my fixes and every questionable place with comment and
"TOCHECK:" label for easy search. Removing of pretty any of these fixes
leads to the tests fail due to the segmentation fault or replication
mismatch. Though I mostly read and tested old version of patch, but
after a quick look it seems that all these fixes are applicable to the
new version of patch as well.

Performance
========

I have also performed a series of performance tests, and found that
patch adds a huge overhead in the case of a large transaction consisting
of many small rows, e.g.:

CREATE TABLE large_test (num1 bigint, num2 double precision, num3 double
precision);

EXPLAIN (ANALYZE, BUFFERS) INSERT INTO large_test (num1, num2, num3)
SELECT round(random()*10), random(), random()*142
FROM generate_series(1, 1000000) s(i);

Execution Time: 2407.709 ms
Total Time: 11494,238 ms (00:11,494)

With synchronous_standby_names and 64 MB logical_work_mem it takes up to
x5 longer, while without patch it is about x2. Thus, logical replication
streaming is approximately x4 as slower for similar transactions.

However, dealing with large transactions consisting of a small number of
large rows is much better:

CREATE TABLE large_text (t TEXT);

EXPLAIN (ANALYZE, BUFFERS) INSERT INTO large_text
SELECT (SELECT string_agg('x', ',')
FROM generate_series(1, 1000000)) FROM generate_series(1, 125);

Execution Time: 3545.642 ms
Total Time: 7678,617 ms (00:07,679)

It is around the same x2 as without patch. If someone is interested I
also added flamegraphs of walsender and logical replication worker
during first numerical transaction processing.

Regards

--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

Hi Alexey,

Thanks for the thorough and extremely valuable review!

On 12/17/18 5:23 PM, Alexey Kondratov wrote:
> Hi Tomas,
>
>> This new version is mostly just a rebase to current master (or almost,
>> because 2pc decoding only applies to 29180e5d78 due to minor bitrot),
>> but it also addresses the new stuff committed since last version (most
>> importantly decoding of TRUNCATE). It also fixes a bug in WAL-logging of
>> subxact assignments, where the assignment was included in records with
>> XID=0, essentially failing to track the subxact properly.
>
> I started reviewing your patch about a month ago and tried to do an
> in-depth review, since I am very interested in this patch too. The new
> version is not applicable to master 29180e5d78, but everything is OK
> after applying 2pc patch before. Anyway, I guess it may complicate
> further testing and review, since any potential reviewer has to take
> into account both patches at once. Previous version was applicable to
> master and was working fine for me separately (excepting a few
> patch-specific issues, which I try to explain below).
>

I agree it's somewhat annoying, but I don't think there's a better way,
unfortunately. Decoding in-progress transactions does require safe
handling of concurrent aborts, so it has to be committed after the 2pc
decoding patch (which makes that possible). But the 2pc patch also
touches the same places as this patch series (it reworks the reorder
buffer for example).

>
> Patch review
> ========
>
> First of all, I want to say thank you for such a huge work done. Here
> are some problems, which I have found and hopefully fixed with my
> additional patch (please, find attached, it should be applicable to the
> last commit of your newest patch version):
>
> 1) The most important issue is that your tap tests were broken—there was
> missing option "WITH (streaming=true)" in the subscription creating
> statement. Therefore, spilling mechanism has been tested rather than
> streaming.
>

D'oh!

> 2) After fixing tests the first one with simple streaming is immediately
> failed, because of logical replication worker segmentation fault. It
> happens, since worker tries to call stream_cleanup_files inside
> stream_open_file at the stream start, while nxids is zero, then it goes
> to the negative value and everything crashes. Something similar may
> happen with xids array, so I added two checks there.
>
> 3) The next problem is much more critical and is dedicated to historic
> MVCC visibility rules. Previously, walsender was starting to decode
> transaction on commit and we were able to resolve all xmin, xmax,
> combocids to cmin/cmax, build tuplecids hash and so on, but now we start
> doing all these things on the fly.
>
> Thus, rather difficult situation arises: HeapTupleSatisfiesHistoricMVCC
> is trying to validate catalog tuples, which are currently in the future
> relatively to the current decoder position inside transaction, e.g. we
> may want to resolve cmin/cmax of a tuple, which was created with cid 3
> and deleted with cid 5, while we are currently at cid 4, so our
> tuplecids hash is not complete to handle such a case.
>

Damn it! I ran into those two issues some time ago and I fixed it, but
I've forgotten to merge that fix into the patch. I'll merge those fixed
and compare them to your proposed fix, and send a new version tomorrow.

>
> 4) There was a problem with marking top-level transaction as having
> catalog changes if one of its subtransactions has. It was causing a
> problem with DDL statements just after subtransaction start (savepoint),
> so data from new columns is not replicated.
>
> 5) Similar issue with schema send. You send schema only once per each
> sub/transaction (IIRC), while we have to update schema on each catalog
> change: invalidation execution, snapshot rebuild, adding new tuple cids.
> So I ended up with adding is_schema_send flag to ReorderBufferTXN, since
> it is easy to set it inside RB and read in the output plugin. Probably,
> we have to choose a better place for this flag.
>

Hmm. Can you share an example how to trigger these issues?

> 6) To better handle all these tricky cases I added new tap
> test—014_stream_tough_ddl.pl—which consist of really tough combination
> of DDL, DML, savepoints and ROLLBACK/RELEASE in a single transaction.
>

Thanks!

> I marked all my fixes and every questionable place with comment and
> "TOCHECK:" label for easy search. Removing of pretty any of these fixes
> leads to the tests fail due to the segmentation fault or replication
> mismatch. Though I mostly read and tested old version of patch, but
> after a quick look it seems that all these fixes are applicable to the
> new version of patch as well.
>

Thanks. I'll go through your patch tomorrow.

>
> Performance
> ========
>
> I have also performed a series of performance tests, and found that
> patch adds a huge overhead in the case of a large transaction consisting
> of many small rows, e.g.:
>
> CREATE TABLE large_test (num1 bigint, num2 double precision, num3 double
> precision);
>
> EXPLAIN (ANALYZE, BUFFERS) INSERT INTO large_test (num1, num2, num3)
> SELECT round(random()*10), random(), random()*142
> FROM generate_series(1, 1000000) s(i);
>
> Execution Time: 2407.709 ms
> Total Time: 11494,238 ms (00:11,494)
>
> With synchronous_standby_names and 64 MB logical_work_mem it takes up to
> x5 longer, while without patch it is about x2. Thus, logical replication
> streaming is approximately x4 as slower for similar transactions.
>
> However, dealing with large transactions consisting of a small number of
> large rows is much better:
>
> CREATE TABLE large_text (t TEXT);
>
> EXPLAIN (ANALYZE, BUFFERS) INSERT INTO large_text
> SELECT (SELECT string_agg('x', ',')
> FROM generate_series(1, 1000000)) FROM generate_series(1, 125);
>
> Execution Time: 3545.642 ms
> Total Time: 7678,617 ms (00:07,679)
>
> It is around the same x2 as without patch. If someone is interested I
> also added flamegraphs of walsender and logical replication worker
> during first numerical transaction processing.
>

Interesting. Any idea where does the extra overhead in this particular
case come from? It's hard to deduce that from the single flame graph,
when I don't have anything to compare it with (i.e. the flame graph for
the "normal" case).

I'll investigate this (probably not this week), but in general it's good
to keep in mind a couple of things:

1) Some overhead is expected, due to doing things incrementally.

2) The memory limit should be set to sufficiently high value to be hit
only infrequently.

3) And when the limit is actually hit, it's an alternative to spilling
large amounts of data locally (to disk) or incurring significant
replication lag later.

So I'm not particularly worried, but I'll look into that. I'd be much
more worried if there was measurable overhead in cases when there's no
streaming happening (either because it's disabled or the memory limit
was not hit).

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 18.12.2018 1:28, Tomas Vondra wrote:
>> 4) There was a problem with marking top-level transaction as having
>> catalog changes if one of its subtransactions has. It was causing a
>> problem with DDL statements just after subtransaction start (savepoint),
>> so data from new columns is not replicated.
>>
>> 5) Similar issue with schema send. You send schema only once per each
>> sub/transaction (IIRC), while we have to update schema on each catalog
>> change: invalidation execution, snapshot rebuild, adding new tuple cids.
>> So I ended up with adding is_schema_send flag to ReorderBufferTXN, since
>> it is easy to set it inside RB and read in the output plugin. Probably,
>> we have to choose a better place for this flag.
>>
> Hmm. Can you share an example how to trigger these issues?

Test cases inside 014_stream_tough_ddl.pl and old ones (with
streaming=true option added) should reproduce all these issues. In
general, it happens in a txn like:

INSERT
SAVEPOINT
ALTER TABLE ... ADD COLUMN
INSERT

then the second insert may discover old version of catalog.

> Interesting. Any idea where does the extra overhead in this particular
> case come from? It's hard to deduce that from the single flame graph,
> when I don't have anything to compare it with (i.e. the flame graph for
> the "normal" case).

I guess that bottleneck is in disk operations. You can check
logical_repl_worker_new_perf.svg flame graph: disk reads (~9%) and
writes (~26%) take around 35% of CPU time in summary. To compare,
please, see attached flame graph for the following transaction:

INSERT INTO large_text
SELECT (SELECT string_agg('x', ',')
FROM generate_series(1, 2000)) FROM generate_series(1, 1000000);

Execution Time: 44519.816 ms
Time: 98333,642 ms (01:38,334)

where disk IO is only ~7-8% in total. So we get very roughly the same
~x4-5 performance drop here. JFYI, I am using a machine with SSD for tests.

Therefore, probably you may write changes on receiver in bigger chunks,
not each change separately.

> So I'm not particularly worried, but I'll look into that. I'd be much
> more worried if there was measurable overhead in cases when there's no
> streaming happening (either because it's disabled or the memory limit
> was not hit).

What I have also just found, is that if a table row is large enough to
be TOASTed, e.g.:

INSERT INTO large_text
SELECT (SELECT string_agg('x', ',')
FROM generate_series(1, 1000000)) FROM generate_series(1, 1000);

then logical_work_mem limit is not hit and we neither stream, nor spill
to disk this transaction, while it is still large. In contrast, the
transaction above (with 1000000 smaller rows) being comparable in size
is streamed. Not sure, that it is easy to add proper accounting of
TOAST-able columns, but it worth it.

--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

Hi Alexey,

Attached is an updated version of the patches, with all the fixes I've
done in the past. I believe it should fix at least some of the issues
you reported - certainly the problem with stream_cleanup_files, but
perhaps some of the other issues too.

I'm a bit confused by the changes to TAP tests. Per the patch summary,
some .pl files get renamed (nor sure why), a new one is added, etc. So
I've instead enabled streaming subscriptions in all tests, which with
this patch produces two failures:

Test Summary Report
-------------------
t/004_sync.pl (Wstat: 7424 Tests: 1 Failed: 0)
Non-zero exit status: 29
Parse errors: Bad plan. You planned 7 tests but ran 1.
t/011_stream_ddl.pl (Wstat: 256 Tests: 2 Failed: 1)
Failed test: 2
Non-zero exit status: 1

So yeah, there's more stuff to fix. But I can't directly apply your
fixes because the updated patches are somewhat different.

On 12/18/18 3:07 PM, Alexey Kondratov wrote:
> On 18.12.2018 1:28, Tomas Vondra wrote:
>>> 4) There was a problem with marking top-level transaction as having
>>> catalog changes if one of its subtransactions has. It was causing a
>>> problem with DDL statements just after subtransaction start (savepoint),
>>> so data from new columns is not replicated.
>>>
>>> 5) Similar issue with schema send. You send schema only once per each
>>> sub/transaction (IIRC), while we have to update schema on each catalog
>>> change: invalidation execution, snapshot rebuild, adding new tuple cids.
>>> So I ended up with adding is_schema_send flag to ReorderBufferTXN, since
>>> it is easy to set it inside RB and read in the output plugin. Probably,
>>> we have to choose a better place for this flag.
>>>
>> Hmm. Can you share an example how to trigger these issues?
>
> Test cases inside 014_stream_tough_ddl.pl and old ones (with
> streaming=true option added) should reproduce all these issues. In
> general, it happens in a txn like:
>
> INSERT
> SAVEPOINT
> ALTER TABLE ... ADD COLUMN
> INSERT
>
> then the second insert may discover old version of catalog.
>

Yeah, that's the issue I've discovered before and thought it got fixed.

>> Interesting. Any idea where does the extra overhead in this particular
>> case come from? It's hard to deduce that from the single flame graph,
>> when I don't have anything to compare it with (i.e. the flame graph for
>> the "normal" case).
>
> I guess that bottleneck is in disk operations. You can check
> logical_repl_worker_new_perf.svg flame graph: disk reads (~9%) and
> writes (~26%) take around 35% of CPU time in summary. To compare,
> please, see attached flame graph for the following transaction:
>
> INSERT INTO large_text
> SELECT (SELECT string_agg('x', ',')
> FROM generate_series(1, 2000)) FROM generate_series(1, 1000000);
>
> Execution Time: 44519.816 ms
> Time: 98333,642 ms (01:38,334)
>
> where disk IO is only ~7-8% in total. So we get very roughly the same
> ~x4-5 performance drop here. JFYI, I am using a machine with SSD for tests.
>
> Therefore, probably you may write changes on receiver in bigger chunks,
> not each change separately.
>

Possibly, I/O is certainly a possible culprit, although we should be
using buffered I/O and there certainly are not any fsyncs here. So I'm
not sure why would it be cheaper to do the writes in batches.

BTW does this mean you see the overhead on the apply side? Or are you
running this on a single machine, and it's difficult to decide?

>> So I'm not particularly worried, but I'll look into that. I'd be much
>> more worried if there was measurable overhead in cases when there's no
>> streaming happening (either because it's disabled or the memory limit
>> was not hit).
>
> What I have also just found, is that if a table row is large enough to
> be TOASTed, e.g.:
>
> INSERT INTO large_text
> SELECT (SELECT string_agg('x', ',')
> FROM generate_series(1, 1000000)) FROM generate_series(1, 1000);
>
> then logical_work_mem limit is not hit and we neither stream, nor spill
> to disk this transaction, while it is still large. In contrast, the
> transaction above (with 1000000 smaller rows) being comparable in size
> is streamed. Not sure, that it is easy to add proper accounting of
> TOAST-able columns, but it worth it.
>

That's certainly strange and possibly a bug in the memory accounting
code. I'm not sure why would that happen, though, because TOAST data
look just like regular INSERT changes. Interesting. I wonder if it's
already fixed in this updated version, but it's a bit too late to
investigate that today.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi Tomas,

> I'm a bit confused by the changes to TAP tests. Per the patch summary,
> some .pl files get renamed (nor sure why), a new one is added, etc.

I added new tap test case, streaming=true option inside old stream_*
ones and incremented streaming tests number (+2) because of the
collision between 009_matviews.pl / 009_stream_simple.pl and
010_truncate.pl / 010_stream_subxact.pl. At least in the previous
version of the patch they were under the same numbers. Nothing special,
but for simplicity, please, find attached my new tap test separately.

> So
> I've instead enabled streaming subscriptions in all tests, which with
> this patch produces two failures:
>
> Test Summary Report
> -------------------
> t/004_sync.pl (Wstat: 7424 Tests: 1 Failed: 0)
> Non-zero exit status: 29
> Parse errors: Bad plan. You planned 7 tests but ran 1.
> t/011_stream_ddl.pl (Wstat: 256 Tests: 2 Failed: 1)
> Failed test: 2
> Non-zero exit status: 1
>
> So yeah, there's more stuff to fix. But I can't directly apply your
> fixes because the updated patches are somewhat different.

Fixes should apply clearly to the previous version of your patch. Also,
I am not sure, that it is a good idea to simply enable streaming
subscriptions in all tests (e.g. pre streaming patch t/004_sync.pl),
since then they do not hit not streaming code.

>>> Interesting. Any idea where does the extra overhead in this particular
>>> case come from? It's hard to deduce that from the single flame graph,
>>> when I don't have anything to compare it with (i.e. the flame graph for
>>> the "normal" case).
>> I guess that bottleneck is in disk operations. You can check
>> logical_repl_worker_new_perf.svg flame graph: disk reads (~9%) and
>> writes (~26%) take around 35% of CPU time in summary. To compare,
>> please, see attached flame graph for the following transaction:
>>
>> INSERT INTO large_text
>> SELECT (SELECT string_agg('x', ',')
>> FROM generate_series(1, 2000)) FROM generate_series(1, 1000000);
>>
>> Execution Time: 44519.816 ms
>> Time: 98333,642 ms (01:38,334)
>>
>> where disk IO is only ~7-8% in total. So we get very roughly the same
>> ~x4-5 performance drop here. JFYI, I am using a machine with SSD for tests.
>>
>> Therefore, probably you may write changes on receiver in bigger chunks,
>> not each change separately.
>>
> Possibly, I/O is certainly a possible culprit, although we should be
> using buffered I/O and there certainly are not any fsyncs here. So I'm
> not sure why would it be cheaper to do the writes in batches.
>
> BTW does this mean you see the overhead on the apply side? Or are you
> running this on a single machine, and it's difficult to decide?

I run this on a single machine, but walsender and worker are utilizing
almost 100% of CPU per each process all the time, and at apply side I/O
syscalls take about 1/3 of CPU time. Though I am still not sure, but for
me this result somehow links performance drop with problems at receiver
side.

Writing in batches was just a hypothesis and to validate it I have
performed test with large txn, but consisting of a smaller number of
wide rows. This test does not exhibit any significant performance drop,
while it was streamed too. So it seems to be valid. Anyway, I do not
have other reasonable ideas beside that right now.

Regards

--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

Hi,

Attached is an updated patch series, merging fixes and changes to TAP
tests proposed by Alexey. I've merged the fixes into the appropriate
patches, and I've kept the TAP changes / new tests as separate patches
towards the end of the series.

I'm a bit unhappy with two aspects of the current patch series:

1) We now track schema changes in two ways - using the pre-existing
schema_sent flag in RelationSyncEntry, and the (newly added) flag in
ReorderBuffer. While those options are used for regular vs. streamed
transactions, fundamentally it's the same thing and so having two
competing ways seems like a bad idea. Not sure what's the best way to
resolve this, though.

2) We've removed quite a few asserts, particularly ensuring sanity of
cmin/cmax values. To some extent that's expected, because by allowing
decoding of in-progress transactions relaxes some of those rules. But
I'd be much happier if some of those asserts could be reinstated, even
if only in a weaker form.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Jan 14, 2019 at 07:23:31PM +0100, Tomas Vondra wrote:
> Attached is an updated patch series, merging fixes and changes to TAP
> tests proposed by Alexey. I've merged the fixes into the appropriate
> patches, and I've kept the TAP changes / new tests as separate patches
> towards the end of the series.

Patch 4 of the latest set fails to apply, so I have moved the patch to
next CF, waiting on author.
--
Michael

Hi Tomas,

On 14.01.2019 21:23, Tomas Vondra wrote:
> Attached is an updated patch series, merging fixes and changes to TAP
> tests proposed by Alexey. I've merged the fixes into the appropriate
> patches, and I've kept the TAP changes / new tests as separate patches
> towards the end of the series.

I had problems applying this patch along with 2pc streaming one to the
current master, but everything applied well on 97c39498e5. Regression
tests pass. What I personally do not like in the current TAP tests set
is that you have added "WITH (streaming=on)" to all tests including old
non-streaming ones. It seems unclear, which mechanism is tested there:
streaming, but those transactions probably do not hit memory limit, so
it depends on default server parameters; or non-streaming, but then what
is the need for (streaming=on)? I would prefer to add (streaming=on)
only to the new tests, where it is clearly necessary.

> I'm a bit unhappy with two aspects of the current patch series:
>
> 1) We now track schema changes in two ways - using the pre-existing
> schema_sent flag in RelationSyncEntry, and the (newly added) flag in
> ReorderBuffer. While those options are used for regular vs. streamed
> transactions, fundamentally it's the same thing and so having two
> competing ways seems like a bad idea. Not sure what's the best way to
> resolve this, though.

Yes, sure, when I have found problems with streaming of extensive DDL, I
added new flag in the simplest way, and it worked. Now, old schema_sent
flag is per relation based, while the new one - is_schema_sent - is per
top-level transaction based. If I get it correctly, the former seems to
be more thrifty, since new schema is sent only if we are streaming
change for relation, whose schema is outdated. In contrast, in the
latter case we will send new schema even if there will be no new changes
which belong to this relation.

I guess, it would be better to stick to the old behavior. I will try to
investigate how to better use it in the streaming mode as well.

> 2) We've removed quite a few asserts, particularly ensuring sanity of
> cmin/cmax values. To some extent that's expected, because by allowing
> decoding of in-progress transactions relaxes some of those rules. But
> I'd be much happier if some of those asserts could be reinstated, even
> if only in a weaker form.

Asserts have been removed from two places: (1)
HeapTupleSatisfiesHistoricMVCC, which seems inevitable, since we are
touching the essence of the MVCC visibility rules, when trying to decode
an in-progress transaction, and (2) ReorderBufferBuildTupleCidHash,
which is probably not related directly to the topic of the ongoing
patch, since Arseny Sher faced the same issue with simple repetitive DDL
decoding [1] recently.

Not many, but I agree, that replacing them with some softer asserts
would be better, than just removing, especially point 1).

[1] https://www.postgresql.org/message-id/flat/874l9p8hyw.fsf%40ars-thinkpad

Regards

--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

Hi Tomas,

>>>> Interesting. Any idea where does the extra overhead in this particular
>>>> case come from? It's hard to deduce that from the single flame graph,
>>>> when I don't have anything to compare it with (i.e. the flame graph
>>>> for
>>>> the "normal" case).
>>> I guess that bottleneck is in disk operations. You can check
>>> logical_repl_worker_new_perf.svg flame graph: disk reads (~9%) and
>>> writes (~26%) take around 35% of CPU time in summary. To compare,
>>> please, see attached flame graph for the following transaction:
>>>
>>> INSERT INTO large_text
>>> SELECT (SELECT string_agg('x', ',')
>>> FROM generate_series(1, 2000)) FROM generate_series(1, 1000000);
>>>
>>> Execution Time: 44519.816 ms
>>> Time: 98333,642 ms (01:38,334)
>>>
>>> where disk IO is only ~7-8% in total. So we get very roughly the same
>>> ~x4-5 performance drop here. JFYI, I am using a machine with SSD for
>>> tests.
>>>
>>> Therefore, probably you may write changes on receiver in bigger chunks,
>>> not each change separately.
>>>
>> Possibly, I/O is certainly a possible culprit, although we should be
>> using buffered I/O and there certainly are not any fsyncs here. So I'm
>> not sure why would it be cheaper to do the writes in batches.
>>
>> BTW does this mean you see the overhead on the apply side? Or are you
>> running this on a single machine, and it's difficult to decide?
>
> I run this on a single machine, but walsender and worker are utilizing
> almost 100% of CPU per each process all the time, and at apply side
> I/O syscalls take about 1/3 of CPU time. Though I am still not sure,
> but for me this result somehow links performance drop with problems at
> receiver side.
>
> Writing in batches was just a hypothesis and to validate it I have
> performed test with large txn, but consisting of a smaller number of
> wide rows. This test does not exhibit any significant performance
> drop, while it was streamed too. So it seems to be valid. Anyway, I do
> not have other reasonable ideas beside that right now.

I've checked recently this patch again and tried to elaborate it in
terms of performance. As a result I've implemented a new POC version of
the applier (attached). Almost everything in streaming logic stayed
intact, but apply worker is significantly different.

As I wrote earlier I still claim, that spilling changes on disk at the
applier side adds additional overhead, but it is possible to get rid of
it. In my additional patch I do the following:

1) Maintain a pool of additional background workers (bgworkers), that
are connected with main logical apply worker via shm_mq's. Each worker
is dedicated to the processing of specific streamed transaction.

2) When we receive a streamed change for some transaction, we check
whether there is an existing dedicated bgworker in HTAB (xid ->
bgworker), or there are some in the idle list, or spawn a new one.

3) We pass all changes (between STREAM START/STOP) to that bgworker via
shm_mq_send without intermediate waiting. However, we wait for bgworker
to apply the entire changes chunk at STREAM STOP, since we don't want
transactions reordering.

4) When transaction is commited/aborted worker is being added to the
idle list and is waiting for reassigning message.

5) I have used the same machinery with apply_dispatch in bgworkers,
since most of actions are practically very similar.

Thus, we do not spill anything at the applier side, so transaction
changes are processed by bgworkers as normal backends do. In the same
time, changes processing is strictly serial, which prevents transactions
reordering and possible conflicts/anomalies. Even though we trade off
performance in favor of stability the result is rather impressive. I
have used a similar query for testing as before:

EXPLAIN (ANALYZE, BUFFERS) INSERT INTO large_test (num1, num2, num3)
    SELECT round(random()*10), random(), random()*142
    FROM generate_series(1, 1000000) s(i);

with 1kk (1000000), 3kk and 5kk rows; logical_work_mem = 64MB and
synchronous_standby_names = 'FIRST 1 (large_sub)'. Table schema is
following:

CREATE TABLE large_test (
    id serial primary key,
    num1 bigint,
    num2 double precision,
    num3 double precision
);

Here are the results:

-------------------------------------------------------------------
| N | Time on master, sec | Total xact time, sec |     Ratio      |
-------------------------------------------------------------------
|                        On commit (master, v13)                  |
-------------------------------------------------------------------
| 1kk | 6.5               | 17.6                 | x2.74          |
-------------------------------------------------------------------
| 3kk | 21                | 55.4                 | x2.64          |
-------------------------------------------------------------------
| 5kk | 38.3              | 91.5                 | x2.39          |
-------------------------------------------------------------------
|                        Stream + spill                           |
-------------------------------------------------------------------
| 1kk | 5.9               | 18                   | x3             |
-------------------------------------------------------------------
| 3kk | 19.5              | 52.4                 | x2.7           |
-------------------------------------------------------------------
| 5kk | 33.3              | 86.7                 | x2.86          |
-------------------------------------------------------------------
|                        Stream + BGW pool                        |
-------------------------------------------------------------------
| 1kk | 6                 | 12                   | x2             |
-------------------------------------------------------------------
| 3kk | 18.5              | 30.5                 | x1.65          |
-------------------------------------------------------------------
| 5kk | 35.6              | 53.9                 | x1.51          |
-------------------------------------------------------------------

It seems that overhead added by synchronous replica is lower by 2-3
times compared with Postgres master and streaming with spilling.
Therefore, the original patch eliminated delay before large transaction
processing start by sender, while this additional patch speeds up the
applier side.

Although the overall speed up is surely measurable, there is a room for
improvements yet:

1) Currently bgworkers are only spawned on demand without some initial
pool and never stopped. Maybe we should create a small pool on
replication start and offload some of idle bgworkers if they exceed some
limit?

2) Probably we can track somehow that incoming change has conflicts with
some of being processed xacts, so we can wait for specific bgworkers
only in that case?

3) Since the communication between main logical apply worker and each
bgworker from the pool is a 'single producer --- single consumer'
problem, then probably it is possible to wait and set/check flags
without locks, but using just atomics.

What do you think about this concept in general? Any concerns and
criticism are welcome!

Regards

--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

P.S. This patch shloud be applicable to your last patch set. I would rebase it against master, but it depends on 2pc patch, that I don't know well enough.

On Wed, Aug 28, 2019 at 08:17:47PM +0300, Alexey Kondratov wrote:
>Hi Tomas,
>
>>>>>Interesting. Any idea where does the extra overhead in this particular
>>>>>case come from? It's hard to deduce that from the single flame graph,
>>>>>when I don't have anything to compare it with (i.e. the flame
>>>>>graph for
>>>>>the "normal" case).
>>>>I guess that bottleneck is in disk operations. You can check
>>>>logical_repl_worker_new_perf.svg flame graph: disk reads (~9%) and
>>>>writes (~26%) take around 35% of CPU time in summary. To compare,
>>>>please, see attached flame graph for the following transaction:
>>>>
>>>>INSERT INTO large_text
>>>>SELECT (SELECT string_agg('x', ',')
>>>>FROM generate_series(1, 2000)) FROM generate_series(1, 1000000);
>>>>
>>>>Execution Time: 44519.816 ms
>>>>Time: 98333,642 ms (01:38,334)
>>>>
>>>>where disk IO is only ~7-8% in total. So we get very roughly the same
>>>>~x4-5 performance drop here. JFYI, I am using a machine with SSD
>>>>for tests.
>>>>
>>>>Therefore, probably you may write changes on receiver in bigger chunks,
>>>>not each change separately.
>>>>
>>>Possibly, I/O is certainly a possible culprit, although we should be
>>>using buffered I/O and there certainly are not any fsyncs here. So I'm
>>>not sure why would it be cheaper to do the writes in batches.
>>>
>>>BTW does this mean you see the overhead on the apply side? Or are you
>>>running this on a single machine, and it's difficult to decide?
>>
>>I run this on a single machine, but walsender and worker are
>>utilizing almost 100% of CPU per each process all the time, and at
>>apply side I/O syscalls take about 1/3 of CPU time. Though I am
>>still not sure, but for me this result somehow links performance
>>drop with problems at receiver side.
>>
>>Writing in batches was just a hypothesis and to validate it I have
>>performed test with large txn, but consisting of a smaller number of
>>wide rows. This test does not exhibit any significant performance
>>drop, while it was streamed too. So it seems to be valid. Anyway, I
>>do not have other reasonable ideas beside that right now.
>
>I've checked recently this patch again and tried to elaborate it in
>terms of performance. As a result I've implemented a new POC version
>of the applier (attached). Almost everything in streaming logic stayed
>intact, but apply worker is significantly different.
>
>As I wrote earlier I still claim, that spilling changes on disk at the
>applier side adds additional overhead, but it is possible to get rid
>of it. In my additional patch I do the following:
>
>1) Maintain a pool of additional background workers (bgworkers), that
>are connected with main logical apply worker via shm_mq's. Each worker
>is dedicated to the processing of specific streamed transaction.
>
>2) When we receive a streamed change for some transaction, we check
>whether there is an existing dedicated bgworker in HTAB (xid ->
>bgworker), or there are some in the idle list, or spawn a new one.
>
>3) We pass all changes (between STREAM START/STOP) to that bgworker
>via shm_mq_send without intermediate waiting. However, we wait for
>bgworker to apply the entire changes chunk at STREAM STOP, since we
>don't want transactions reordering.
>
>4) When transaction is commited/aborted worker is being added to the
>idle list and is waiting for reassigning message.
>
>5) I have used the same machinery with apply_dispatch in bgworkers,
>since most of actions are practically very similar.
>
>Thus, we do not spill anything at the applier side, so transaction
>changes are processed by bgworkers as normal backends do. In the same
>time, changes processing is strictly serial, which prevents
>transactions reordering and possible conflicts/anomalies. Even though
>we trade off performance in favor of stability the result is rather
>impressive. I have used a similar query for testing as before:
>
>EXPLAIN (ANALYZE, BUFFERS) INSERT INTO large_test (num1, num2, num3)
>    SELECT round(random()*10), random(), random()*142
>    FROM generate_series(1, 1000000) s(i);
>
>with 1kk (1000000), 3kk and 5kk rows; logical_work_mem = 64MB and
>synchronous_standby_names = 'FIRST 1 (large_sub)'. Table schema is
>following:
>
>CREATE TABLE large_test (
>    id serial primary key,
>    num1 bigint,
>    num2 double precision,
>    num3 double precision
>);
>
>Here are the results:
>
>-------------------------------------------------------------------
>| N | Time on master, sec | Total xact time, sec |     Ratio      |
>-------------------------------------------------------------------
>|                        On commit (master, v13)                  |
>-------------------------------------------------------------------
>| 1kk | 6.5               | 17.6                 | x2.74          |
>-------------------------------------------------------------------
>| 3kk | 21                | 55.4                 | x2.64          |
>-------------------------------------------------------------------
>| 5kk | 38.3              | 91.5                 | x2.39          |
>-------------------------------------------------------------------
>|                        Stream + spill                           |
>-------------------------------------------------------------------
>| 1kk | 5.9               | 18                   | x3             |
>-------------------------------------------------------------------
>| 3kk | 19.5              | 52.4                 | x2.7           |
>-------------------------------------------------------------------
>| 5kk | 33.3              | 86.7                 | x2.86          |
>-------------------------------------------------------------------
>|                        Stream + BGW pool                        |
>-------------------------------------------------------------------
>| 1kk | 6                 | 12                   | x2             |
>-------------------------------------------------------------------
>| 3kk | 18.5              | 30.5                 | x1.65          |
>-------------------------------------------------------------------
>| 5kk | 35.6              | 53.9                 | x1.51          |
>-------------------------------------------------------------------
>
>It seems that overhead added by synchronous replica is lower by 2-3
>times compared with Postgres master and streaming with spilling.
>Therefore, the original patch eliminated delay before large
>transaction processing start by sender, while this additional patch
>speeds up the applier side.
>
>Although the overall speed up is surely measurable, there is a room
>for improvements yet:
>
>1) Currently bgworkers are only spawned on demand without some initial
>pool and never stopped. Maybe we should create a small pool on
>replication start and offload some of idle bgworkers if they exceed
>some limit?
>
>2) Probably we can track somehow that incoming change has conflicts
>with some of being processed xacts, so we can wait for specific
>bgworkers only in that case?
>
>3) Since the communication between main logical apply worker and each
>bgworker from the pool is a 'single producer --- single consumer'
>problem, then probably it is possible to wait and set/check flags
>without locks, but using just atomics.
>
>What do you think about this concept in general? Any concerns and
>criticism are welcome!
>

Hi Alexey,

I'm unable to do any in-depth review of the patch over the next two weeks
or so, but I think the idea of having a pool of apply workers is sound and
can be quite beneficial for some workloads.

I don't think it matters very much whether the workers are started at the
beginning or allocated ad hoc, that's IMO a minor implementation detail.

There's one huge challenge that I however don't see mentioned in your
message or in the patch (after cursory reading) - ensuring the same commit
order, and introducing deadlocks that would not exist in single-process
apply.

Surely, we want to end up with the same commit order as on the upstream,
otherwise we might easily get different data on the subscriber. So when we
pass the large transaction to a separate process, then this process has
to wait for the other processes processing transactions that committed
first. And similarly, other processes have to wait for this process.
Depending on the commit order. I might have missed something, but I don't
see anything like that in your patch.

Essentially, this means there needs to be some sort of wait between those
apply processes, enforcing the commit order.

That however means we can easily introduce deadlocks into workloads where
the serial-apply would not have that issue - imagine multiple large
transactions, touching the same set of rows. We may ship them to different
bgworkers, and those processes may deadlock.

Of course, the deadlock detector will come around (assuming the wait is
done in a way visible to the detector) and will abort one of the
processes. But we don't know it'll abort the right one - it may easily
abort the apply process that needs to comit first, and eveyone else is
waitiing for it. Which stalls the apply forever.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 28.08.2019 22:06, Tomas Vondra wrote:
>
>>
>>>>>> Interesting. Any idea where does the extra overhead in this
>>>>>> particular
>>>>>> case come from? It's hard to deduce that from the single flame
>>>>>> graph,
>>>>>> when I don't have anything to compare it with (i.e. the flame
>>>>>> graph for
>>>>>> the "normal" case).
>>>>> I guess that bottleneck is in disk operations. You can check
>>>>> logical_repl_worker_new_perf.svg flame graph: disk reads (~9%) and
>>>>> writes (~26%) take around 35% of CPU time in summary. To compare,
>>>>> please, see attached flame graph for the following transaction:
>>>>>
>>>>> INSERT INTO large_text
>>>>> SELECT (SELECT string_agg('x', ',')
>>>>> FROM generate_series(1, 2000)) FROM generate_series(1, 1000000);
>>>>>
>>>>> Execution Time: 44519.816 ms
>>>>> Time: 98333,642 ms (01:38,334)
>>>>>
>>>>> where disk IO is only ~7-8% in total. So we get very roughly the same
>>>>> ~x4-5 performance drop here. JFYI, I am using a machine with SSD
>>>>> for tests.
>>>>>
>>>>> Therefore, probably you may write changes on receiver in bigger
>>>>> chunks,
>>>>> not each change separately.
>>>>>
>>>> Possibly, I/O is certainly a possible culprit, although we should be
>>>> using buffered I/O and there certainly are not any fsyncs here. So I'm
>>>> not sure why would it be cheaper to do the writes in batches.
>>>>
>>>> BTW does this mean you see the overhead on the apply side? Or are you
>>>> running this on a single machine, and it's difficult to decide?
>>>
>>> I run this on a single machine, but walsender and worker are
>>> utilizing almost 100% of CPU per each process all the time, and at
>>> apply side I/O syscalls take about 1/3 of CPU time. Though I am
>>> still not sure, but for me this result somehow links performance
>>> drop with problems at receiver side.
>>>
>>> Writing in batches was just a hypothesis and to validate it I have
>>> performed test with large txn, but consisting of a smaller number of
>>> wide rows. This test does not exhibit any significant performance
>>> drop, while it was streamed too. So it seems to be valid. Anyway, I
>>> do not have other reasonable ideas beside that right now.
>>
>> It seems that overhead added by synchronous replica is lower by 2-3
>> times compared with Postgres master and streaming with spilling.
>> Therefore, the original patch eliminated delay before large
>> transaction processing start by sender, while this additional patch
>> speeds up the applier side.
>>
>> Although the overall speed up is surely measurable, there is a room
>> for improvements yet:
>>
>> 1) Currently bgworkers are only spawned on demand without some
>> initial pool and never stopped. Maybe we should create a small pool
>> on replication start and offload some of idle bgworkers if they
>> exceed some limit?
>>
>> 2) Probably we can track somehow that incoming change has conflicts
>> with some of being processed xacts, so we can wait for specific
>> bgworkers only in that case?
>>
>> 3) Since the communication between main logical apply worker and each
>> bgworker from the pool is a 'single producer --- single consumer'
>> problem, then probably it is possible to wait and set/check flags
>> without locks, but using just atomics.
>>
>> What do you think about this concept in general? Any concerns and
>> criticism are welcome!
>>
>

Hi Tomas,

Thank you for a quick response.

> I don't think it matters very much whether the workers are started at the
> beginning or allocated ad hoc, that's IMO a minor implementation detail.

OK, I had the same vision about this point. Any minor differences here
will be neglectable for a sufficiently large transaction.

>
> There's one huge challenge that I however don't see mentioned in your
> message or in the patch (after cursory reading) - ensuring the same
> commit
> order, and introducing deadlocks that would not exist in single-process
> apply.

Probably I haven't explained well this part, sorry for that. In my patch
I don't use workers pool for a concurrent transaction apply, but rather
for a fast context switch between long-lived streamed transactions. In
other words we apply all changes arrived from the sender in a completely
serial manner. Being written step-by-step it looks like:

1) Read STREAM START message and figure out the target worker by xid.

2) Put all changes, which belongs to this xact to the selected worker
one by one via shm_mq_send.

3) Read STREAM STOP message and wait until our worker will apply all
changes in the queue.

4) Process all other chunks of streamed xacts in the same manner.

5) Process all non-streamed xacts immediately in the main apply worker loop.

6) If we read STREAMED COMMIT/ABORT we again wait until selected worker
either commits or aborts.

Thus, it automatically guaranties the same commit order on replica as on
master. Yes, we loose some performance here, since we don't apply
transactions concurrently, but it would bring all those problems you
have described.

However, you helped me to figure out another point I have forgotten.
Although we ensure commit order automatically, the beginning of streamed
xacts may reorder. It happens if some small xacts have been commited on
master since the streamed one started, because we do not start streaming
immediately, but only after logical_work_mem hit. I have performed some
tests with conflicting xacts and it seems that it's not a problem, since
locking mechanism in Postgres guarantees that if there would some
deadlocks, they will happen earlier on master. So if some records hit
the WAL, it is safe to apply the sequentially. Am I wrong?

Anyway, I'm going to double check the safety of this part later.

Regards

--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

On Thu, Aug 29, 2019 at 05:37:45PM +0300, Alexey Kondratov wrote:
>On 28.08.2019 22:06, Tomas Vondra wrote:
>>
>>>
>>>>>>>Interesting. Any idea where does the extra overhead in
>>>>>>>this particular
>>>>>>>case come from? It's hard to deduce that from the single
>>>>>>>flame graph,
>>>>>>>when I don't have anything to compare it with (i.e. the
>>>>>>>flame graph for
>>>>>>>the "normal" case).
>>>>>>I guess that bottleneck is in disk operations. You can check
>>>>>>logical_repl_worker_new_perf.svg flame graph: disk reads (~9%) and
>>>>>>writes (~26%) take around 35% of CPU time in summary. To compare,
>>>>>>please, see attached flame graph for the following transaction:
>>>>>>
>>>>>>INSERT INTO large_text
>>>>>>SELECT (SELECT string_agg('x', ',')
>>>>>>FROM generate_series(1, 2000)) FROM generate_series(1, 1000000);
>>>>>>
>>>>>>Execution Time: 44519.816 ms
>>>>>>Time: 98333,642 ms (01:38,334)
>>>>>>
>>>>>>where disk IO is only ~7-8% in total. So we get very roughly the same
>>>>>>~x4-5 performance drop here. JFYI, I am using a machine with
>>>>>>SSD for tests.
>>>>>>
>>>>>>Therefore, probably you may write changes on receiver in
>>>>>>bigger chunks,
>>>>>>not each change separately.
>>>>>>
>>>>>Possibly, I/O is certainly a possible culprit, although we should be
>>>>>using buffered I/O and there certainly are not any fsyncs here. So I'm
>>>>>not sure why would it be cheaper to do the writes in batches.
>>>>>
>>>>>BTW does this mean you see the overhead on the apply side? Or are you
>>>>>running this on a single machine, and it's difficult to decide?
>>>>
>>>>I run this on a single machine, but walsender and worker are
>>>>utilizing almost 100% of CPU per each process all the time, and
>>>>at apply side I/O syscalls take about 1/3 of CPU time. Though I
>>>>am still not sure, but for me this result somehow links
>>>>performance drop with problems at receiver side.
>>>>
>>>>Writing in batches was just a hypothesis and to validate it I
>>>>have performed test with large txn, but consisting of a smaller
>>>>number of wide rows. This test does not exhibit any significant
>>>>performance drop, while it was streamed too. So it seems to be
>>>>valid. Anyway, I do not have other reasonable ideas beside that
>>>>right now.
>>>
>>>It seems that overhead added by synchronous replica is lower by
>>>2-3 times compared with Postgres master and streaming with
>>>spilling. Therefore, the original patch eliminated delay before
>>>large transaction processing start by sender, while this
>>>additional patch speeds up the applier side.
>>>
>>>Although the overall speed up is surely measurable, there is a
>>>room for improvements yet:
>>>
>>>1) Currently bgworkers are only spawned on demand without some
>>>initial pool and never stopped. Maybe we should create a small
>>>pool on replication start and offload some of idle bgworkers if
>>>they exceed some limit?
>>>
>>>2) Probably we can track somehow that incoming change has
>>>conflicts with some of being processed xacts, so we can wait for
>>>specific bgworkers only in that case?
>>>
>>>3) Since the communication between main logical apply worker and
>>>each bgworker from the pool is a 'single producer --- single
>>>consumer' problem, then probably it is possible to wait and
>>>set/check flags without locks, but using just atomics.
>>>
>>>What do you think about this concept in general? Any concerns and
>>>criticism are welcome!
>>>
>>
>
>Hi Tomas,
>
>Thank you for a quick response.
>
>>I don't think it matters very much whether the workers are started at the
>>beginning or allocated ad hoc, that's IMO a minor implementation detail.
>
>OK, I had the same vision about this point. Any minor differences here
>will be neglectable for a sufficiently large transaction.
>
>>
>>There's one huge challenge that I however don't see mentioned in your
>>message or in the patch (after cursory reading) - ensuring the same
>>commit
>>order, and introducing deadlocks that would not exist in single-process
>>apply.
>
>Probably I haven't explained well this part, sorry for that. In my
>patch I don't use workers pool for a concurrent transaction apply, but
>rather for a fast context switch between long-lived streamed
>transactions. In other words we apply all changes arrived from the
>sender in a completely serial manner. Being written step-by-step it
>looks like:
>
>1) Read STREAM START message and figure out the target worker by xid.
>
>2) Put all changes, which belongs to this xact to the selected worker
>one by one via shm_mq_send.
>
>3) Read STREAM STOP message and wait until our worker will apply all
>changes in the queue.
>
>4) Process all other chunks of streamed xacts in the same manner.
>
>5) Process all non-streamed xacts immediately in the main apply worker loop.
>
>6) If we read STREAMED COMMIT/ABORT we again wait until selected
>worker either commits or aborts.
>
>Thus, it automatically guaranties the same commit order on replica as
>on master. Yes, we loose some performance here, since we don't apply
>transactions concurrently, but it would bring all those problems you
>have described.
>

OK, so it's apply in multiple processes, but at any moment only a single
apply process is active.

>However, you helped me to figure out another point I have forgotten.
>Although we ensure commit order automatically, the beginning of
>streamed xacts may reorder. It happens if some small xacts have been
>commited on master since the streamed one started, because we do not
>start streaming immediately, but only after logical_work_mem hit. I
>have performed some tests with conflicting xacts and it seems that
>it's not a problem, since locking mechanism in Postgres guarantees
>that if there would some deadlocks, they will happen earlier on
>master. So if some records hit the WAL, it is safe to apply the
>sequentially. Am I wrong?
>

I think you're right the way you interleave the changes ensures you
can't introduce new deadlocks between transactions in this stream. I don't
think reordering the blocks of streamed trasactions does matter, as long
as the comit order is ensured in this case.

>Anyway, I'm going to double check the safety of this part later.
>

OK.

FWIW my understanding is that the speedup comes mostly from elimination of
the serialization to a file. That however requires savepoints to handle
aborts of subtransactions - I'm pretty sure I'd be trivial to create a
workload where this will be much slower (with many aborts of large
subtransactions).

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

>
> FWIW my understanding is that the speedup comes mostly from
> elimination of
> the serialization to a file. That however requires savepoints to handle
> aborts of subtransactions - I'm pretty sure I'd be trivial to create a
> workload where this will be much slower (with many aborts of large
> subtransactions).
>
>

I think that instead of defining savepoints it is simpler and more
efficient to use

BeginInternalSubTransaction +
ReleaseCurrentSubTransaction/RollbackAndReleaseCurrentSubTransaction

as it is done in PL/pgSQL (pl_exec.c).
Not sure if it can pr

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

In the interest of moving things forward, how far are we from making
0001 committable? If I understand correctly, the rest of this patchset
depends on https://commitfest.postgresql.org/24/944/ which seems to be
moving at a glacial pace (or, actually, slower, because glaciers do
move, which cannot be said of that other patch.)

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Sep 02, 2019 at 06:06:50PM -0400, Alvaro Herrera wrote:
>In the interest of moving things forward, how far are we from making
>0001 committable? If I understand correctly, the rest of this patchset
>depends on https://commitfest.postgresql.org/24/944/ which seems to be
>moving at a glacial pace (or, actually, slower, because glaciers do
>move, which cannot be said of that other patch.)
>

I think 0001 is mostly there. I think there's one bug in this patch
version, but I need to check and I'll post an updated version shortly if
needed.

FWIW maybe we should stop comparing things to glaciers. 50 years from not
people won't know what a glacier is, and it'll be just like the floppy
icon on the save button.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

>>
>> FWIW my understanding is that the speedup comes mostly from
>> elimination of
>> the serialization to a file. That however requires savepoints to handle
>> aborts of subtransactions - I'm pretty sure I'd be trivial to create a
>> workload where this will be much slower (with many aborts of large
>> subtransactions).
>>

Yes, and it was my main motivation to eliminate that extra serialization
to file. I've experimented a bit with large transactions + savepoints +
aborts and ended up with a following query (the same schema as before
with 600k rows):

BEGIN;
SAVEPOINT s1;
UPDATE large_test SET num1 = num1 + 1, num2 = num2 + 1, num3 = num3 + 1;
SAVEPOINT s2;
UPDATE large_test SET num1 = num1 + 1, num2 = num2 + 1, num3 = num3 + 1;
SAVEPOINT s3;
UPDATE large_test SET num1 = num1 + 1, num2 = num2 + 1, num3 = num3 + 1;
ROLLBACK TO SAVEPOINT s3;
ROLLBACK TO SAVEPOINT s2;
ROLLBACK TO SAVEPOINT s1;
END;

It looks like the worst case scenario, as we do a lot of work and then
abort all subxacts one by one. As expected,it takes much longer (up to
x30) to process using background worker instead of spilling to file.
Surely, it is much easier to truncate a file, than apply all changes +
abort. However, I guess that this kind of load pattern is not the most
typical for real-life applications.

Also this test helped me to find a bug in my current savepoints routine,
so new patch is attached.

On 30.08.2019 18:59, Konstantin Knizhnik wrote:
>
> I think that instead of defining savepoints it is simpler and more
> efficient to use
>
> BeginInternalSubTransaction +
> ReleaseCurrentSubTransaction/RollbackAndReleaseCurrentSubTransaction
>
> as it is done in PL/pgSQL (pl_exec.c).
> Not sure if it can pr
>

Both BeginInternalSubTransaction and DefineSavepoint use
PushTransaction() internally for a normal subtransaction start. So they
seems to be identical from the performance perspective, which is also
stated in the comment section:

/*
 * BeginInternalSubTransaction
 *        This is the same as DefineSavepoint except it allows
TBLOCK_STARTED,
 *        TBLOCK_IMPLICIT_INPROGRESS, TBLOCK_END, and TBLOCK_PREPARE
states,
 *        and therefore it can safely be used in functions that might
be called
 *        when not inside a BEGIN block or when running deferred
triggers at
 *        COMMIT/PREPARE time.  Also, it automatically does
 *        CommitTransactionCommand/StartTransactionCommand instead of
expecting
 *        the caller to do it.
 */

Please, correct me if I'm wrong.

Anyway, I've performed a profiling of my apply worker (flamegraph is
attached) and it spends the vast amount of time (>90%) applying changes.
So the problem is not in the savepoints their-self, but in the fact that
we first apply all changes and then abort all the work. Not sure, that
it is possible to do something in this case.

Regards

--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

On 16.09.2019 19:54, Alexey Kondratov wrote:
> On 30.08.2019 18:59, Konstantin Knizhnik wrote:
>>
>> I think that instead of defining savepoints it is simpler and more
>> efficient to use
>>
>> BeginInternalSubTransaction +
>> ReleaseCurrentSubTransaction/RollbackAndReleaseCurrentSubTransaction
>>
>> as it is done in PL/pgSQL (pl_exec.c).
>> Not sure if it can pr
>>
>
> Both BeginInternalSubTransaction and DefineSavepoint use
> PushTransaction() internally for a normal subtransaction start. So
> they seems to be identical from the performance perspective, which is
> also stated in the comment section:

Yes, definitely them are using the same mechanism and most likely
provides similar performance.
But BeginInternalSubTransaction does not require to generate some
savepoint name which seems to be redundant in this case.

>
> Anyway, I've performed a profiling of my apply worker (flamegraph is
> attached) and it spends the vast amount of time (>90%) applying
> changes. So the problem is not in the savepoints their-self, but in
> the fact that we first apply all changes and then abort all the work.
> Not sure, that it is possible to do something in this case.
>

Looks like the only way to increase apply speed is to do it in parallel:
make it possible to concurrently execute non-conflicting transactions.

On Mon, Sep 16, 2019 at 10:29:18PM +0300, Konstantin Knizhnik wrote:
>
>
>On 16.09.2019 19:54, Alexey Kondratov wrote:
>>On 30.08.2019 18:59, Konstantin Knizhnik wrote:
>>>
>>>I think that instead of defining savepoints it is simpler and more
>>>efficient to use
>>>
>>>BeginInternalSubTransaction +
>>>ReleaseCurrentSubTransaction/RollbackAndReleaseCurrentSubTransaction
>>>
>>>as it is done in PL/pgSQL (pl_exec.c).
>>>Not sure if it can pr
>>>
>>
>>Both BeginInternalSubTransaction and DefineSavepoint use
>>PushTransaction() internally for a normal subtransaction start. So
>>they seems to be identical from the performance perspective, which
>>is also stated in the comment section:
>
>Yes, definitely them are using the same mechanism and most likely
>provides similar performance.
>But BeginInternalSubTransaction does not require to generate some
>savepoint name which seems to be redundant in this case.
>
>
>>
>>Anyway, I've performed a profiling of my apply worker (flamegraph is
>>attached) and it spends the vast amount of time (>90%) applying
>>changes. So the problem is not in the savepoints their-self, but in
>>the fact that we first apply all changes and then abort all the
>>work. Not sure, that it is possible to do something in this case.
>>
>
>Looks like the only way to increase apply speed is to do it in
>parallel: make it possible to concurrently execute non-conflicting
>transactions.
>

True, although it seems like a massive can of worms to me. I'm not aware
a way to identify non-conflicting transactions in advance, so it would
have to be implemented as optimistic apply, with a detection and
recovery from conflicts.

I'm not against doing that, and I'm willing to spend some time on revies
etc. but it seems like a completely separate effort.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Sep 3, 2019 at 4:30 AM Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>
> In the interest of moving things forward, how far are we from making
> 0001 committable? If I understand correctly, the rest of this patchset
> depends on https://commitfest.postgresql.org/24/944/ which seems to be
> moving at a glacial pace (or, actually, slower, because glaciers do
> move, which cannot be said of that other patch.)
>

I am not sure if it is completely correct that the other part of the
patch is dependent on that CF entry. I have studied both the threads
(not every detail) and it seems to me it is dependent on one of the
patches from that series which handles concurrent aborts. It is patch
0003-Gracefully-handle-concurrent-aborts-of-uncommitted-t.Jan4.patch
from what the Nikhil has posted on that thread [1]. Am, I wrong?

So IIUC, the problem of concurrent aborts is that if we allow catalog
scans for in-progress transactions, then we might get wrong answers in
cases where somebody has performed Alter-Abort-Alter which is clearly
explained with an example in email [2]. To solve that problem Nikhil
seems to have written a patch [1] which detects these concurrent
aborts during a system table scan and then aborts the decoding of such
a transaction.

Now, the problem is that patch has written considering 2PC
transactions and might not deal with all cases for in-progress
transactions especially when sub-transactions are involved as alluded
by Arseny Sher [3]. So, the problem seems to be for cases when some
sub-transaction aborts, but the main transaction still continued and
we try to decode it. Nikhil's patch won't be able to deal with it
because I think it just checks top-level xid whereas for this we need
to check all-subxids which I think is possible now as Tomas seems to
have written WAL for each xid-assignment. It might or might not be
the best solution to check the status of all-subxids, but I think
first we need to agree that the problem is just for concurrent aborts
and that we can solve it by using some part of the technology being
developed as part of patch "Logical decoding of two-phase
transactions" (https://commitfest.postgresql.org/24/944/) rather than
the entire patchset.

I hope I am not saying something very obvious here and it helps in
moving this patch forward.

Thoughts?

[1] - https://www.postgresql.org/message-id/CAMGcDxcBmN6jNeQkgWddfhX8HbSjQpW%3DUo70iBY3P_EPdp%2BLTQ%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/EEBD82AA-61EE-46F4-845E-05B94168E8F2%40postgrespro.ru
[3] - https://www.postgresql.org/message-id/87a7py4iwl.fsf%40ars-thinkpad

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Sep 3, 2019 at 4:16 PM Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>
> On Mon, Sep 02, 2019 at 06:06:50PM -0400, Alvaro Herrera wrote:
> >In the interest of moving things forward, how far are we from making
> >0001 committable? If I understand correctly, the rest of this patchset
> >depends on https://commitfest.postgresql.org/24/944/ which seems to be
> >moving at a glacial pace (or, actually, slower, because glaciers do
> >move, which cannot be said of that other patch.)
> >
>
> I think 0001 is mostly there. I think there's one bug in this patch
> version, but I need to check and I'll post an updated version shortly if
> needed.
>

Did you get a chance to work on 0001? I have a few comments on that patch:
1.
+ * To limit the amount of memory used by decoded changes, we track memory
+ * used at the reorder buffer level (i.e. total amount of memory), and for
+ * each toplevel transaction. When the total amount of used memory exceeds
+ * the limit, the toplevel transaction consuming the most memory is either
+ * serialized or streamed.

Do we need to mention 'streamed' as part of this patch? It seems to
me that this is an independent patch which can be committed without
patches that stream the changes. So, we can remove it from here and
other places where it is used.

2.
+ * deserializing and applying very few changes). We probably to give more
+ * memory to the oldest subtransactions.

/We probably to/
It seems some word is missing after probably.

3.
+ * Find the largest transaction (toplevel or subxact) to evict (spill to disk).
+ *
+ * XXX With many subtransactions this might be quite slow, because we'll have
+ * to walk through all of them. There are some options how we could improve
+ * that: (a) maintain some secondary structure with transactions sorted by
+ * amount of changes, (b) not looking for the entirely largest transaction,
+ * but e.g. for transaction using at least some fraction of the memory limit,
+ * and (c) evicting multiple transactions at once, e.g. to free a given portion
+ * of the memory limit (e.g. 50%).
+ */
+static ReorderBufferTXN *
+ReorderBufferLargestTXN(ReorderBuffer *rb)

What is the guarantee that after evicting largest transaction, we
won't immediately hit the memory limit? Say, all of the transactions
are of almost similar size which I don't think is that uncommon a
case. Instead, the strategy mentioned in point (c) or something like
that seems more promising. In that strategy, there is some risk that
it might lead to many smaller disk writes which we might want to
control via some threshold (like we should not flush more than N
xacts). In this, we also need to ensure that the total memory freed
must be greater than the current change.

I think we have some discussion around this point but didn't reach any
conclusion which means some more brainstorming is required.

4.
+int logical_work_mem; /* 4MB */

What this 4MB in comments indicate?

5.
+/*
+ * Check whether the logical_work_mem limit was reached, and if yes pick
+ * the transaction tx should spill its data to disk.

The second part of the sentence "pick the transaction tx should spill"
seems to be incomplete.

Apart from this, I see that Peter E. has raised some other points on
this patch which are not yet addressed as those also need some
discussion, so I will respond to those separately with my opinion.

These comments are based on the last patch posted by you on this
thread [1]. You might have fixed some of these already, so ignore if
that is the case.

[1] - https://www.postgresql.org/message-id/76fc440e-91c3-afe2-b78a-987205b3c758%402ndquadrant.com

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hi,

Attached is an updated patch series, rebased on current master. It does
fix one memory accounting bug in ReorderBufferToastReplace (the code was
not properly updating the amount of memory).

I've also included the patch series with decoding of 2PC transactions,
which this depends on. This way we have a chance of making the cfbot
happy. So parts 0001-0004 and 0009-0014 are "this" patch series, while
0005-0008 are the extra pieces from the other patch.

I've done it like this because the initial parts are independent, and so
might be committed irrespectedly of the other patch series. In practice
that's only reasonable for 0001, which adds the memory limit - the rest
is infrastucture for the streaming of in-progress transactions.

On Wed, Sep 25, 2019 at 06:55:01PM +0530, Amit Kapila wrote:
>On Tue, Sep 3, 2019 at 4:30 AM Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>>
>> In the interest of moving things forward, how far are we from making
>> 0001 committable? If I understand correctly, the rest of this patchset
>> depends on https://commitfest.postgresql.org/24/944/ which seems to be
>> moving at a glacial pace (or, actually, slower, because glaciers do
>> move, which cannot be said of that other patch.)
>>
>
>I am not sure if it is completely correct that the other part of the
>patch is dependent on that CF entry. I have studied both the threads
>(not every detail) and it seems to me it is dependent on one of the
>patches from that series which handles concurrent aborts. It is patch
>0003-Gracefully-handle-concurrent-aborts-of-uncommitted-t.Jan4.patch
>from what the Nikhil has posted on that thread [1]. Am, I wrong?
>

You're right - the part handling aborts is the only part required. There
are dependencies on some other changes from the 2PC patch, but those are
mostly refactorings that can be undone (e.g. switch from independent
flags to a single bitmap in reorderbuffer).

>So IIUC, the problem of concurrent aborts is that if we allow catalog
>scans for in-progress transactions, then we might get wrong answers in
>cases where somebody has performed Alter-Abort-Alter which is clearly
>explained with an example in email [2]. To solve that problem Nikhil
>seems to have written a patch [1] which detects these concurrent
>aborts during a system table scan and then aborts the decoding of such
>a transaction.
>
>Now, the problem is that patch has written considering 2PC
>transactions and might not deal with all cases for in-progress
>transactions especially when sub-transactions are involved as alluded
>by Arseny Sher [3]. So, the problem seems to be for cases when some
>sub-transaction aborts, but the main transaction still continued and
>we try to decode it. Nikhil's patch won't be able to deal with it
>because I think it just checks top-level xid whereas for this we need
>to check all-subxids which I think is possible now as Tomas seems to
>have written WAL for each xid-assignment. It might or might not be
>the best solution to check the status of all-subxids, but I think
>first we need to agree that the problem is just for concurrent aborts
>and that we can solve it by using some part of the technology being
>developed as part of patch "Logical decoding of two-phase
>transactions" (https://commitfest.postgresql.org/24/944/) rather than
>the entire patchset.
>
>I hope I am not saying something very obvious here and it helps in
>moving this patch forward.
>

No, that's a good question, and I'm not sure what the answer is at the
moment. My understanding was that the infrastructure in the 2PC patch is
enough even for subtransactions, but I might be wrong. I need to think
about that for a while.

Maybe we should focus on the 0001 part for now - it can be committed
indepently and does provide useful feature.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Sep 26, 2019 at 06:58:17PM +0530, Amit Kapila wrote:
>On Tue, Sep 3, 2019 at 4:16 PM Tomas Vondra
><tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>>
>> On Mon, Sep 02, 2019 at 06:06:50PM -0400, Alvaro Herrera wrote:
>> >In the interest of moving things forward, how far are we from making
>> >0001 committable? If I understand correctly, the rest of this patchset
>> >depends on https://commitfest.postgresql.org/24/944/ which seems to be
>> >moving at a glacial pace (or, actually, slower, because glaciers do
>> >move, which cannot be said of that other patch.)
>> >
>>
>> I think 0001 is mostly there. I think there's one bug in this patch
>> version, but I need to check and I'll post an updated version shortly if
>> needed.
>>
>
>Did you get a chance to work on 0001? I have a few comments on that patch:
>1.
>+ * To limit the amount of memory used by decoded changes, we track memory
>+ * used at the reorder buffer level (i.e. total amount of memory), and for
>+ * each toplevel transaction. When the total amount of used memory exceeds
>+ * the limit, the toplevel transaction consuming the most memory is either
>+ * serialized or streamed.
>
>Do we need to mention 'streamed' as part of this patch? It seems to
>me that this is an independent patch which can be committed without
>patches that stream the changes. So, we can remove it from here and
>other places where it is used.
>

You're right - this patch should not mention streaming because the parts
adding that capability are later in the series. So it can trigger just
the serialization to disk.

>2.
>+ * deserializing and applying very few changes). We probably to give more
>+ * memory to the oldest subtransactions.
>
>/We probably to/
>It seems some word is missing after probably.
>

Yes.

>3.
>+ * Find the largest transaction (toplevel or subxact) to evict (spill to disk).
>+ *
>+ * XXX With many subtransactions this might be quite slow, because we'll have
>+ * to walk through all of them. There are some options how we could improve
>+ * that: (a) maintain some secondary structure with transactions sorted by
>+ * amount of changes, (b) not looking for the entirely largest transaction,
>+ * but e.g. for transaction using at least some fraction of the memory limit,
>+ * and (c) evicting multiple transactions at once, e.g. to free a given portion
>+ * of the memory limit (e.g. 50%).
>+ */
>+static ReorderBufferTXN *
>+ReorderBufferLargestTXN(ReorderBuffer *rb)
>
>What is the guarantee that after evicting largest transaction, we
>won't immediately hit the memory limit? Say, all of the transactions
>are of almost similar size which I don't think is that uncommon a
>case.

Not sure I understand - what do you mean 'immediately hit'?

We do check the limit after queueing a change, and we know that this
change is what got us over the limit. We pick the largest transaction
(which has to be larger than the change we just entered) and evict it,
getting below the memory limit again.

The next change can get us over the memory limit again, of course, but
there's not much we could do about that.

> Instead, the strategy mentioned in point (c) or something like
>that seems more promising. In that strategy, there is some risk that
>it might lead to many smaller disk writes which we might want to
>control via some threshold (like we should not flush more than N
>xacts). In this, we also need to ensure that the total memory freed
>must be greater than the current change.
>
>I think we have some discussion around this point but didn't reach any
>conclusion which means some more brainstorming is required.
>

I agree it's worth investigating, but I'm not sure it's necessary before
committing v1 of the feature. I don't think there's a clear winner
strategy, and the current approach works fairly well I think.

The comment is concerned with the cost of ReorderBufferLargestTXN with
many transactions, but we can only have certain number of top-level
transactions (max_connections + certain number of not-yet-assigned
subtransactions). And 0002 patch essentially gets rid of the subxacts
entirely, further reducing the maximum number of xacts to walk.

>4.
>+int logical_work_mem; /* 4MB */
>
>What this 4MB in comments indicate?
>

Sorry, that's a mistake.

>5.
>+/*
>+ * Check whether the logical_work_mem limit was reached, and if yes pick
>+ * the transaction tx should spill its data to disk.
>
>The second part of the sentence "pick the transaction tx should spill"
>seems to be incomplete.
>

Yeah, that's a poor wording. Will fix.

>Apart from this, I see that Peter E. has raised some other points on
>this patch which are not yet addressed as those also need some
>discussion, so I will respond to those separately with my opinion.
>

OK, thanks.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-Sep-26, Tomas Vondra wrote:

> Hi,
>
> Attached is an updated patch series, rebased on current master. It does
> fix one memory accounting bug in ReorderBufferToastReplace (the code was
> not properly updating the amount of memory).

Cool.

Can we aim to get 0001 pushed during this commitfest, or is that a lost
cause?

The large new comment in reorderbuffer.c says that a transaction might
get spilled *or streamed*, but surely that second thing is not correct,
since before the subsequent patches it's not possible to stream
transactions that have not yet finished?

How certain are you about the approach to measure memory used by a
reorderbuffer transaction ... does it not cause a measurable performance
drop? I wonder if it would make more sense to use a separate contexts
per transaction and use context-level accounting (per the patch Jeff
Davis posted elsewhere for hash joins ... though I see now that that
only works fot aset.c, not other memcxt implementations), or something
like that.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-Sep-26, Alvaro Herrera wrote:

> How certain are you about the approach to measure memory used by a
> reorderbuffer transaction ... does it not cause a measurable performance
> drop? I wonder if it would make more sense to use a separate contexts
> per transaction and use context-level accounting (per the patch Jeff
> Davis posted elsewhere for hash joins ... though I see now that that
> only works fot aset.c, not other memcxt implementations), or something
> like that.

Oh, I just noticed that that patch was posted separately in its own
thread, and that that improved version does include support for other
memory context implementations. Excellent.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Sep 26, 2019 at 04:36:20PM -0300, Alvaro Herrera wrote:
>On 2019-Sep-26, Alvaro Herrera wrote:
>
>> How certain are you about the approach to measure memory used by a
>> reorderbuffer transaction ... does it not cause a measurable performance
>> drop? I wonder if it would make more sense to use a separate contexts
>> per transaction and use context-level accounting (per the patch Jeff
>> Davis posted elsewhere for hash joins ... though I see now that that
>> only works fot aset.c, not other memcxt implementations), or something
>> like that.
>
>Oh, I just noticed that that patch was posted separately in its own
>thread, and that that improved version does include support for other
>memory context implementations. Excellent.
>

Unfortunately, that won't fly, for two simple reasons:

1) The memory accounting patch is known to perform poorly with many
child contexts - this was why array_agg/string_agg were problematic,
before we rewrote them not to create memory context for each group.

It could be done differently (eager accounting) but then the overhead
for regular/common cases (with just a couple of contexts) is higher. So
that seems like a much inferior option.

2) We can't actually have a single context per transaction. Some parts
(REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID) of a transaction are not
evicted, so we'd have to keep them in a separate context.

It'd also mean higher allocation overhead, because now we can reuse
chunks cross-transaction. So one transaction commits or gets serialized,
and we reuse the chunks for something else. With per-transaction
contexts we'd lose some of this benefit - we could only reuse chunks
within a transaction (i.e. large transactions that get spilled to disk)
but not across commits.

I don't have any numbers, of course, but I wouldn't be surprised if it
was significant e.g. for small transactions that don't get spilled. And
creating/destroying the contexts is not free either, I think.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Sep 26, 2019 at 04:33:59PM -0300, Alvaro Herrera wrote:
>On 2019-Sep-26, Tomas Vondra wrote:
>
>> Hi,
>>
>> Attached is an updated patch series, rebased on current master. It does
>> fix one memory accounting bug in ReorderBufferToastReplace (the code was
>> not properly updating the amount of memory).
>
>Cool.
>
>Can we aim to get 0001 pushed during this commitfest, or is that a lost
>cause?
>

It's tempting. The patch has been in the queue for quite a bit of time,
and I think it's solid (at least 0001). I'll address the comments from
Peter's review about separating the GUC etc. and polish it a bit more.
If I manage to do that by Monday, I'll consider pushing it.

If anyone feels I shouldn't do that, let me know.

The one open question pointed out by Amit is how the patch picks the
trasction for eviction. My feeling is that's fine and if needed can be
improved later if necessary, but I'll try to construct a worst case
(max_connections xacts, each with 64 subxact) to verify.

>The large new comment in reorderbuffer.c says that a transaction might
>get spilled *or streamed*, but surely that second thing is not correct,
>since before the subsequent patches it's not possible to stream
>transactions that have not yet finished?
>

True. That's a residue of reordering the patch series repeatedly, I
think. I'll fix that while polishing the patch.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Sep 27, 2019 at 12:06 AM Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>
> On Thu, Sep 26, 2019 at 06:58:17PM +0530, Amit Kapila wrote:
>
> >3.
> >+ * Find the largest transaction (toplevel or subxact) to evict (spill to disk).
> >+ *
> >+ * XXX With many subtransactions this might be quite slow, because we'll have
> >+ * to walk through all of them. There are some options how we could improve
> >+ * that: (a) maintain some secondary structure with transactions sorted by
> >+ * amount of changes, (b) not looking for the entirely largest transaction,
> >+ * but e.g. for transaction using at least some fraction of the memory limit,
> >+ * and (c) evicting multiple transactions at once, e.g. to free a given portion
> >+ * of the memory limit (e.g. 50%).
> >+ */
> >+static ReorderBufferTXN *
> >+ReorderBufferLargestTXN(ReorderBuffer *rb)
> >
> >What is the guarantee that after evicting largest transaction, we
> >won't immediately hit the memory limit? Say, all of the transactions
> >are of almost similar size which I don't think is that uncommon a
> >case.
>
> Not sure I understand - what do you mean 'immediately hit'?
>
> We do check the limit after queueing a change, and we know that this
> change is what got us over the limit. We pick the largest transaction
> (which has to be larger than the change we just entered) and evict it,
> getting below the memory limit again.
>
> The next change can get us over the memory limit again, of course,
>

Yeah, this is what I want to say when I wrote that it can immediately hit again.

> but
> there's not much we could do about that.
>
> > Instead, the strategy mentioned in point (c) or something like
> >that seems more promising. In that strategy, there is some risk that
> >it might lead to many smaller disk writes which we might want to
> >control via some threshold (like we should not flush more than N
> >xacts). In this, we also need to ensure that the total memory freed
> >must be greater than the current change.
> >
> >I think we have some discussion around this point but didn't reach any
> >conclusion which means some more brainstorming is required.
> >
>
> I agree it's worth investigating, but I'm not sure it's necessary before
> committing v1 of the feature. I don't think there's a clear winner
> strategy, and the current approach works fairly well I think.
>
> The comment is concerned with the cost of ReorderBufferLargestTXN with
> many transactions, but we can only have certain number of top-level
> transactions (max_connections + certain number of not-yet-assigned
> subtransactions). And 0002 patch essentially gets rid of the subxacts
> entirely, further reducing the maximum number of xacts to walk.
>

That would be good, but I don't understand how. The second patch will
update the subxacts in top-level ReorderBufferTxn, but it won't remove
it from hash table. It also doesn't seem to be caring for considering
the size of subxacts in top-level xact, so not sure how will it reduce
the number of xacts to walk. I might be missing something here. Can
you explain a bit how 0002 patch would help in reducing the maximum
number of xacts to walk?

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Jan 9, 2018 at 7:55 AM Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>
> On 1/3/18 14:53, Tomas Vondra wrote:
> >> I don't see the need to tie this setting to maintenance_work_mem.
> >> maintenance_work_mem is often set to very large values, which could
> >> then have undesirable side effects on this use.
> >
> > Well, we need to pick some default value, and we can either use a fixed
> > value (not sure what would be a good default) or tie it to an existing
> > GUC. We only really have work_mem and maintenance_work_mem, and the
> > walsender process will never use more than one such buffer. Which seems
> > to be closer to maintenance_work_mem.
> >
> > Pretty much any default value can have undesirable side effects.
>
> Let's just make it an independent setting unless we know any better. We
> don't have a lot of settings that depend on other settings, and the ones
> we do have a very specific relationship.
>
> >> Moreover, the name logical_work_mem makes it sound like it's a logical
> >> version of work_mem. Maybe we could think of another name.
> >
> > I won't object to a better name, of course. Any proposals?
>
> logical_decoding_[work_]mem?
>

Having a separate variable for this can give more flexibility, but
OTOH it will add one more knob which user might not have a good idea
to set. What are the problems we see if directly use work_mem for
this case?

If we can't use work_mem, then I think the name proposed by you
(logical_decoding_work_mem) sounds good to me.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Sep 26, 2019 at 11:37 PM Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>
> On Wed, Sep 25, 2019 at 06:55:01PM +0530, Amit Kapila wrote:
> >On Tue, Sep 3, 2019 at 4:30 AM Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> >>
> >> In the interest of moving things forward, how far are we from making
> >> 0001 committable? If I understand correctly, the rest of this patchset
> >> depends on https://commitfest.postgresql.org/24/944/ which seems to be
> >> moving at a glacial pace (or, actually, slower, because glaciers do
> >> move, which cannot be said of that other patch.)
> >>
> >
> >I am not sure if it is completely correct that the other part of the
> >patch is dependent on that CF entry. I have studied both the threads
> >(not every detail) and it seems to me it is dependent on one of the
> >patches from that series which handles concurrent aborts. It is patch
> >0003-Gracefully-handle-concurrent-aborts-of-uncommitted-t.Jan4.patch
> >from what the Nikhil has posted on that thread [1]. Am, I wrong?
> >
>
> You're right - the part handling aborts is the only part required. There
> are dependencies on some other changes from the 2PC patch, but those are
> mostly refactorings that can be undone (e.g. switch from independent
> flags to a single bitmap in reorderbuffer).
>
> >So IIUC, the problem of concurrent aborts is that if we allow catalog
> >scans for in-progress transactions, then we might get wrong answers in
> >cases where somebody has performed Alter-Abort-Alter which is clearly
> >explained with an example in email [2]. To solve that problem Nikhil
> >seems to have written a patch [1] which detects these concurrent
> >aborts during a system table scan and then aborts the decoding of such
> >a transaction.
> >
> >Now, the problem is that patch has written considering 2PC
> >transactions and might not deal with all cases for in-progress
> >transactions especially when sub-transactions are involved as alluded
> >by Arseny Sher [3]. So, the problem seems to be for cases when some
> >sub-transaction aborts, but the main transaction still continued and
> >we try to decode it. Nikhil's patch won't be able to deal with it
> >because I think it just checks top-level xid whereas for this we need
> >to check all-subxids which I think is possible now as Tomas seems to
> >have written WAL for each xid-assignment. It might or might not be
> >the best solution to check the status of all-subxids, but I think
> >first we need to agree that the problem is just for concurrent aborts
> >and that we can solve it by using some part of the technology being
> >developed as part of patch "Logical decoding of two-phase
> >transactions" (https://commitfest.postgresql.org/24/944/) rather than
> >the entire patchset.
> >
> >I hope I am not saying something very obvious here and it helps in
> >moving this patch forward.
> >
>
> No, that's a good question, and I'm not sure what the answer is at the
> moment. My understanding was that the infrastructure in the 2PC patch is
> enough even for subtransactions, but I might be wrong.
>

I also think the patch that handles concurrent aborts should be
sufficient, but that need to be integrated with your patch. Earlier,
I thought we need to check whether any of the subtransaction is
aborted as mentioned by Arseny Sher, but now after thinking again
about that problem, it seems that checking only the status current
subtransaction should be sufficient. Because, if the user does
Rollback to Savepoint concurrently which aborts multiple
subtransactions, the latest one must be aborted as well which is what
I think we want to detect. Once we detect that we have two options
(a) restart the decode of that transaction by removing changes of all
subxacts or (b) somehow mark the transaction such that it gets decoded
only at the commit time.

>
> Maybe we should focus on the 0001 part for now - it can be committed
> indepently and does provide useful feature.
>

If that can be done sooner, then it is fine, but otherwise, preparing
the patches on top of HEAD can facilitate the review of those.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, Sep 27, 2019 at 02:33:32PM +0530, Amit Kapila wrote:
>On Tue, Jan 9, 2018 at 7:55 AM Peter Eisentraut
><peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
>>
>> On 1/3/18 14:53, Tomas Vondra wrote:
>> >> I don't see the need to tie this setting to maintenance_work_mem.
>> >> maintenance_work_mem is often set to very large values, which could
>> >> then have undesirable side effects on this use.
>> >
>> > Well, we need to pick some default value, and we can either use a fixed
>> > value (not sure what would be a good default) or tie it to an existing
>> > GUC. We only really have work_mem and maintenance_work_mem, and the
>> > walsender process will never use more than one such buffer. Which seems
>> > to be closer to maintenance_work_mem.
>> >
>> > Pretty much any default value can have undesirable side effects.
>>
>> Let's just make it an independent setting unless we know any better. We
>> don't have a lot of settings that depend on other settings, and the ones
>> we do have a very specific relationship.
>>
>> >> Moreover, the name logical_work_mem makes it sound like it's a logical
>> >> version of work_mem. Maybe we could think of another name.
>> >
>> > I won't object to a better name, of course. Any proposals?
>>
>> logical_decoding_[work_]mem?
>>
>
>Having a separate variable for this can give more flexibility, but
>OTOH it will add one more knob which user might not have a good idea
>to set. What are the problems we see if directly use work_mem for
>this case?
>

IMHO it's similar to autovacuum_work_mem - we have an independent
setting, but most people use it as -1 so we use maintenance_work_mem as
a default value. I think it makes sense to do the same thing here.

It does ad an extra knob anyway (I don't think we should just use
maintenance_work_mem directly, the user should have an option to
override it when needed). But most users will not notice.

FWIW I don't think we should use work_mem, maintenace_work_mem seems
somewhat more appropriate here (not related to queries, etc.).

>If we can't use work_mem, then I think the name proposed by you
>(logical_decoding_work_mem) sounds good to me.
>

Yes, that name seems better.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Sep 27, 2019 at 4:55 PM Tomas Vondra
<tomas(dot)vondra(at)2ndquadrant(dot)com> wrote:
>
> On Fri, Sep 27, 2019 at 02:33:32PM +0530, Amit Kapila wrote:
> >On Tue, Jan 9, 2018 at 7:55 AM Peter Eisentraut
> ><peter(dot)eisentraut(at)2ndquadrant(dot)com> wrote:
> >>
> >> On 1/3/18 14:53, Tomas Vondra wrote:
> >> >> I don't see the need to tie this setting to maintenance_work_mem.
> >> >> maintenance_work_mem is often set to very large values, which could
> >> >> then have undesirable side effects on this use.
> >> >
> >> > Well, we need to pick some default value, and we can either use a fixed
> >> > value (not sure what would be a good default) or tie it to an existing
> >> > GUC. We only really have work_mem and maintenance_work_mem, and the
> >> > walsender process will never use more than one such buffer. Which seems
> >> > to be closer to maintenance_work_mem.
> >> >
> >> > Pretty much any default value can have undesirable side effects.
> >>
> >> Let's just make it an independent setting unless we know any better. We
> >> don't have a lot of settings that depend on other settings, and the ones
> >> we do have a very specific relationship.
> >>
> >> >> Moreover, the name logical_work_mem makes it sound like it's a logical
> >> >> version of work_mem. Maybe we could think of another name.
> >> >
> >> > I won't object to a better name, of course. Any proposals?
> >>
> >> logical_decoding_[work_]mem?
> >>
> >
> >Having a separate variable for this can give more flexibility, but
> >OTOH it will add one more knob which user might not have a good idea
> >to set. What are the problems we see if directly use work_mem for
> >this case?
> >
>
> IMHO it's similar to autovacuum_work_mem - we have an independent
> setting, but most people use it as -1 so we use maintenance_work_mem as
> a default value. I think it makes sense to do the same thing here.
>
> It does ad an extra knob anyway (I don't think we should just use
> maintenance_work_mem directly, the user should have an option to
> override it when needed). But most users will not notice.
>
> FWIW I don't think we should use work_mem, maintenace_work_mem seems
> somewhat more appropriate here (not related to queries, etc.).
>

I have the same concern for using maintenace_work_mem as Peter E.
which is that the value of maintenace_work_mem will generally be
higher which is suitable for its current purpose, but not for the
purpose this patch is using. AFAIU, at this stage we want a better
memory accounting system for logical decoding and we are not sure what
is a good value for this variable. So, I think using work_mem or
maintenace_work_mem should serve the purpose. Later, if we have
requirements from people to have better control over the memory
required for this purpose then we can introduce a new variable.

I understand that currently work_mem is primarily tied with memory
used for query workspaces, but it might be okay to extend it for this
purpose. Another point is that the default for that sound to be more
appealing for this case. I can see the argument against it which is
having a separate variable will make the things look clean and give
better control. So, if we can't convince ourselves for using
work_mem, we can introduce a new guc variable and keep the default as
4MB or work_mem.

I feel it is always tempting to introduce a new guc for the different
tasks unless there is an exact match, but OTOH, having lesser guc's
has its own advantage which is that people don't have to bother about
a new setting which they need to tune and especially for which they
can't decide with ease. I am not telling that we should not introduce
new guc when it is required, but just to give more thought before
doing so.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com