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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
539 messages Options
12345 ... 27
Reply | Threaded
Open this post in threaded view
|

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

Tomas Vondra-4


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

Reply | Threaded
Open this post in threaded view
|

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

Tomas Vondra-4
In reply to this post by Peter Eisentraut-6


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

Reply | Threaded
Open this post in threaded view
|

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

Peter Eisentraut-6
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

Reply | Threaded
Open this post in threaded view
|

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

Tomas Vondra-4
In reply to this post by Tomas Vondra-4
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

0001-Introduce-logical_work_mem-to-limit-ReorderBuffer-v5.patch.gz (13K) Download Attachment
0002-Issue-XLOG_XACT_ASSIGNMENT-with-wal_level-logical-v5.patch.gz (3K) Download Attachment
0003-Issue-individual-invalidations-with-wal_level-log-v5.patch.gz (6K) Download Attachment
0004-Extend-the-output-plugin-API-with-stream-methods-v5.patch.gz (7K) Download Attachment
0005-Implement-streaming-mode-in-ReorderBuffer-v5.patch.gz (14K) Download Attachment
0006-Add-support-for-streaming-to-built-in-replication-v5.patch.gz (25K) Download Attachment
0007-Track-statistics-for-streaming-spilling-v5.patch.gz (5K) Download Attachment
0008-BUGFIX-make-sure-subxact-is-marked-as-is_known_as-v5.patch.gz (780 bytes) Download Attachment
0009-BUGFIX-set-final_lsn-for-subxacts-before-cleanup-v5.patch.gz (862 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Tomas Vondra-4
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

0001-Introduce-logical_work_mem-to-limit-ReorderBuffer-v6.patch.gz (13K) Download Attachment
0002-Issue-XLOG_XACT_ASSIGNMENT-with-wal_level-logical-v6.patch.gz (3K) Download Attachment
0003-Issue-individual-invalidations-with-wal_level-log-v6.patch.gz (6K) Download Attachment
0004-Extend-the-output-plugin-API-with-stream-methods-v6.patch.gz (7K) Download Attachment
0005-Implement-streaming-mode-in-ReorderBuffer-v6.patch.gz (14K) Download Attachment
0006-Add-support-for-streaming-to-built-in-replication-v6.patch.gz (25K) Download Attachment
0007-Track-statistics-for-streaming-spilling-v6.patch.gz (5K) Download Attachment
0008-BUGFIX-make-sure-subxact-is-marked-as-is_known_as-v6.patch.gz (780 bytes) Download Attachment
0009-BUGFIX-set-final_lsn-for-subxacts-before-cleanup-v6.patch.gz (866 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Masahiko Sawada
On Sat, Jan 20, 2018 at 7:08 AM, Tomas Vondra
<[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Peter Eisentraut-6
In reply to this post by Peter Eisentraut-6
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

Reply | Threaded
Open this post in threaded view
|

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

Tomas Vondra-4
In reply to this post by Masahiko Sawada
On 01/31/2018 07:53 AM, Masahiko Sawada wrote:

> On Sat, Jan 20, 2018 at 7:08 AM, Tomas Vondra
> <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Tomas Vondra-4
In reply to this post by Peter Eisentraut-6
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

Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
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

Reply | Threaded
Open this post in threaded view
|

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

Tomas Vondra-4
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

Reply | Threaded
Open this post in threaded view
|

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

David Steele
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
[hidden email]

Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
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

Reply | Threaded
Open this post in threaded view
|

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

Robert Haas
In reply to this post by Tomas Vondra-4
On Thu, Mar 1, 2018 at 9:33 PM, Tomas Vondra
<[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

David Steele
On 3/2/18 3:06 PM, Robert Haas wrote:

> On Thu, Mar 1, 2018 at 9:33 PM, Tomas Vondra
> <[hidden email]> 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
[hidden email]

Reply | Threaded
Open this post in threaded view
|

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

Tomas Vondra-4
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
>> <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

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

Tomas Vondra-4
In reply to this post by Tomas Vondra-4
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

0001-Introduce-logical_work_mem-to-limit-ReorderBuffer.patch.gz (13K) Download Attachment
0002-Immediatel-WAL-log-assignments.patch.gz (9K) Download Attachment
0003-Issue-individual-invalidations-with-wal_level-logica.patch.gz (6K) Download Attachment
0004-Extend-the-output-plugin-API-with-stream-methods.patch.gz (7K) Download Attachment
0005-Implement-streaming-mode-in-ReorderBuffer.patch.gz (14K) Download Attachment
0006-Add-support-for-streaming-to-built-in-replication.patch.gz (26K) Download Attachment
0007-Track-statistics-for-streaming-spilling.patch.gz (5K) Download Attachment
0008-BUGFIX-make-sure-subxact-is-marked-as-is_known_as_su.patch.gz (780 bytes) Download Attachment
0009-BUGFIX-set-final_lsn-for-subxacts-before-cleanup.patch.gz (858 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Tomas Vondra-4
In reply to this post by Andres Freund
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

Reply | Threaded
Open this post in threaded view
|

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

Andres Freund
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

Reply | Threaded
Open this post in threaded view
|

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

Tomas Vondra-4
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

12345 ... 27