Subject: Force streaming every change in logical decoding

From: "[email-masked]" <[email-masked]>
Date: Tue, 6 Dec 2022 06:23:43 +0000

Hi hackers,

In logical decoding, when logical_decoding_work_mem is exceeded, the changes are
sent to output plugin in streaming mode. But there is a restriction that the
minimum value of logical_decoding_work_mem is 64kB. I tried to add a GUC to
allow sending every change to output plugin without waiting until
logical_decoding_work_mem is exceeded.

This helps to test streaming mode. For example, to test "Avoid streaming the
transaction which are skipped" [1], it needs many XLOG_XACT_INVALIDATIONS
messages. With the new option, it can be tested with fewer changes and in less
time. Also, this new option helps to test more scenarios for "Perform streaming
logical transactions by background workers" [2].

[1] https://www.postgresql.org/message-id/CAFiTN-tHK=[email-masked]
[2] https://www.postgresql.org/message-id/flat/CAA4eK1%2BwyN6zpaHUkCLorEWNx75MG0xhMwcFhvjqm2KURZEAGw%40mail.gmail.com

Regards,
Shi yu

---

From: Amit Kapila <[email-masked]>
Date: Tue, 6 Dec 2022 15:58:55 +0530

> logical transactions by background workers" [2].

Yeah, I think this can also help in reducing the time for various
tests in test_decoding/stream and
src/test/subscription/t/*_stream_*.pl file by reducing the number of
changes required to invoke streaming mode. Can we think of making this
GUC extendible to even test more options on server-side (publisher)
and client-side (subscriber) cases? For example, we can have something
like logical_replication_mode with the following valid values: (a)
server_serialize: this will serialize each change to file on
publishers and then on commit restore and send all changes; (b)
server_stream: this will stream each change as currently proposed in
your patch. Then if we want to extend it for subscriber-side testing
then we can introduce new options like client_serialize for the case
being discussed in the email [1].

Thoughts?

[1] - https://www.postgresql.org/message-id/CAD21AoAVUfDrm4-%3DykihNAmR7bTX-KpHXM9jc42RbHePJv5k1w%40mail.gmail.com

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 6 Dec 2022 22:48:18 +0900

On Tue, Dec 6, 2022 at 7:29 PM Amit Kapila <[email-masked]> wrote:
> changes required to invoke streaming mode.

+1

> Can we think of making this

Setting logical_replication_mode = 'client_serialize' implies that the
publisher behaves as server_stream? or do you mean we can set like
logical_replication_mode = 'server_stream, client_serialize'?

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

---

From: Peter Smith <[email-masked]>
Date: Wed, 7 Dec 2022 10:45:52 +1100

+1

> being discussed in the email [1].

There is potential for lots of developer GUCs for testing/debugging in
the area of logical replication but IMO it might be better to keep
them all separated. Putting everything into a single
'logical_replication_mode' might cause difficulties later when/if you
want combinations of the different modes.

For example, instead of

logical_replication_mode = XXX/YYY/ZZZ

maybe something like below will give more flexibility.

logical_replication_dev_XXX = true/false
logical_replication_dev_YYY = true/false
logical_replication_dev_ZZZ = true/false

------
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Peter Smith <[email-masked]>
Date: Wed, 7 Dec 2022 12:14:01 +1100

> logical_decoding_work_mem is exceeded.

Some review comments for patch v1-0001.

1. Typos

In several places "Wheather/wheather" -> "Whether/whether"

======

src/backend/replication/logical/reorderbuffer.c

2. ReorderBufferCheckMemoryLimit

 {
  ReorderBufferTXN *txn;

- /* bail out if we haven't exceeded the memory limit */
- if (rb->size < logical_decoding_work_mem * 1024L)
+ /*
+ * Stream the changes immediately if force_stream_mode is on and the output
+ * plugin supports streaming. Otherwise wait until size exceeds
+ * logical_decoding_work_mem.
+ */
+ bool force_stream = (force_stream_mode && ReorderBufferCanStream(rb));
+
+ /* bail out if force_stream is false and we haven't exceeded the
memory limit */
+ if (!force_stream && rb->size < logical_decoding_work_mem * 1024L)
  return;

  /*
- * Loop until we reach under the memory limit.  One might think that just
- * by evicting the largest (sub)transaction we will come under the memory
- * limit based on assumption that the selected transaction is at least as
- * large as the most recent change (which caused us to go over the memory
- * limit). However, that is not true because a user can reduce the
+ * If force_stream is true, loop until there's no change. Otherwise, loop
+ * until we reach under the memory limit. One might think that just by
+ * evicting the largest (sub)transaction we will come under the memory limit
+ * based on assumption that the selected transaction is at least as large as
+ * the most recent change (which caused us to go over the memory limit).
+ * However, that is not true because a user can reduce the
  * logical_decoding_work_mem to a smaller value before the most recent
  * change.
  */
- while (rb->size >= logical_decoding_work_mem * 1024L)
+ while ((!force_stream && rb->size >= logical_decoding_work_mem * 1024L) ||
+    (force_stream && rb->size > 0))
  {
  /*
  * Pick the largest transaction (or subtransaction) and evict it from

IIUC this logic can be simplified quite a lot just by shifting that
"bail out" condition into the loop.

Something like:

while (true)
{
if (!(force_stream && rb->size > 0 || rb->size <
logical_decoding_work_mem * 1024L))
break;
...
}

------

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 7 Dec 2022 11:00:58 +0900

> want combinations of the different modes.

I think we want the developer option that forces streaming changes
during logical decoding to be PGC_USERSET but probably the developer
option for testing the parallel apply feature would be PGC_SIGHUP.
Also, since streaming changes is not specific to logical replication
but to logical decoding, I'm not sure logical_replication_XXX is a
good name. IMO having force_stream_mode and a different GUC for
testing the parallel apply feature makes sense to me.

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Wed, 7 Dec 2022 08:14:32 +0530

> logical_replication_mode = 'server_stream, client_serialize'?

The latter one (logical_replication_mode = 'server_stream,
client_serialize'). The idea is to cover more options with one GUC and
each option can be used individually as well as in combination with
others.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Wed, 7 Dec 2022 09:25:06 +0530

> option for testing the parallel apply feature would be PGC_SIGHUP.

Ideally, that is true but if we want to combine the multiple modes in
one parameter, is there a harm in keeping it as PGC_SIGHUP?

> testing the parallel apply feature makes sense to me.

But if we want to have a separate variable for testing/debugging
streaming like force_stream_mode, why not for serializing as well? And
if we want for both then we can even think of combining them in one
variable as logical_decoding_mode with values as 'stream' and
'serialize'. The first one specified would be given preference. Also,
the name force_stream_mode doesn't seem to convey that it is for
logical decoding. We can probably have a separate variable for the
subscriber side.

On one side having separate GUCs for publisher and subscriber seems to
give better flexibility but having more GUCs also sometimes makes them
less usable. Here, my thought was to have a single or as few GUCs as
possible which can be extendible by providing multiple values instead
of having different GUCs. I was trying to map this with the existing
string parameters in developer options.

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 7 Dec 2022 14:24:37 +0900

> one parameter, is there a harm in keeping it as PGC_SIGHUP?

It's not a big harm but we will end up doing ALTER SYSTEM and
pg_reload_conf() even in regression tests (e.g. in
test_decoding/stream.sql).

> 'serialize'.

Making it enum makes sense to me.

> The first one specified would be given preference. Also,
> logical decoding.

Agreed.

> string parameters in developer options.

I see your point. On the other hand, I'm not sure it's a good idea to
control different features by one GUC in general. The developer option
could be an exception?

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Wed, 7 Dec 2022 13:21:04 +0530

> control different features by one GUC in general. The developer option

I am not sure what is the best thing if this was proposed as a
non-developer option but it seems to me that having a single parameter
for publisher/subscriber, in this case, can serve our need for
testing/debugging. BTW, even though it is not a very good example but
we use max_replication_slots for different purposes on the publisher
(the limit for slots) and subscriber (the limit for origins).

-- 
With Regards,
Amit Kapila.

---

From: Dilip Kumar <[email-masked]>
Date: Sat, 10 Dec 2022 11:18:14 +0530

On Wed, Dec 7, 2022 at 5:16 AM Peter Smith <[email-masked]> wrote:

+1 for the idea

> logical_replication_dev_ZZZ = true/false

Even I agree that usability wise keeping them independent is better.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

---

From: Dilip Kumar <[email-masked]>
Date: Sat, 10 Dec 2022 11:33:06 +0530

> logical transactions by background workers" [2].

Some comments on the patch

1. Can you add one test case using this option

2. +     <varlistentry id="guc-force-stream-mode" xreflabel="force_stream_mode">
+      <term><varname>force_stream_mode</varname> (<type>boolean</type>)
+      <indexterm>
+       <primary><varname>force_stream_mode</varname> configuration
parameter</primary>
+      </indexterm>
+      </term>

This GUC name "force_stream_mode" somehow appears like we are forcing
this streaming mode irrespective of whether the
subscriber has requested for this mode or not.  But actually it is not
that, it is just streaming each change if
it is enabled.  So we might need to think on the name (at least we
should avoid using *mode* in the name IMHO).

3.

It seems like if force_stream is on then indirectly it is enabling
force serialization as well.  Because once we enter into the loop
based on "force_stream" then it will either stream or serialize but I
guess we do not want to force serialize based on this parameter.

---

From: Peter Smith <[email-masked]>
Date: Mon, 12 Dec 2022 11:03:08 +1100

...
> should avoid using *mode* in the name IMHO).

I thought the same. Names like those shown below might be more appropriate:
stream_checks_work_mem = true/false
stream_mode_checks_size = true/false
stream_for_large_tx_only = true/false
... etc.

The GUC name length could get a bit unwieldy but isn't it better for
it to have the correct meaning than to have a shorter but slightly
ambiguous name? Anyway, it is a developer option so I guess longer
names are less of a problem.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Mon, 12 Dec 2022 17:44:17 +0530

> Even I agree that usability wise keeping them independent is better.

But OTOH, doesn't introducing multiple GUCs (one to allow streaming
each change, another to allow serialization, and a third one to
probably test subscriber-side work) for the purpose of testing, and
debugging logical replication code sound a bit more?

-- 
With Regards,
Amit Kapila.

---

From: Peter Smith <[email-masked]>
Date: Tue, 13 Dec 2022 14:33:51 +1100

> [2] https://www.postgresql.org/message-id/flat/CAA4eK1%2BwyN6zpaHUkCLorEWNx75MG0xhMwcFhvjqm2KURZEAGw%40mail.gmail.com

Hi, I've been doing some testing that makes use of this new developer
GUC `force_stream_mode`.

IIUC this GUC is used by the walsender during the logic of the
ReorderBufferCheckMemoryLimit(). Also, AFAIK the only way that the
walsender is going to know this GUC value is by inheritance from the
parent publisher at the time the walsender process gets launched.

I may be overthinking this. Isn't there potential for this to become
quite confusing depending on the timing of when this GUC is modified?

E.g.1 When the walsender is launched, it will use whatever is the
current value of this GUC.
E.g.2 But if the GUC is changed after the walsender is already
launched, then that will have no effect on the already running
walsender.

Is that understanding correct?

------
Kind Regards,
Peter Smith.
Fujitsu Australia.

---

From: Peter Smith <[email-masked]>
Date: Tue, 13 Dec 2022 16:45:00 +1100

> Is that understanding correct?

I think I was mistaken above. It looks like even the already-launched
walsender gets the updated GUC value via a SIGHUP on the parent
publisher.

2022-12-13 16:31:33.453 AEDT [1902] LOG:  received SIGHUP, reloading
configuration files
2022-12-13 16:31:33.455 AEDT [1902] LOG:  parameter
"force_stream_mode" changed to "true"

Sorry for the noise.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 14 Dec 2022 08:44:56 +0000

> Some comments on the patch

Thanks for your comments.

> 1. Can you add one test case using this option

I added a simple test to confirm the new option works.

> should avoid using *mode* in the name IMHO).

I think a similar GUC is force_parallel_mode, and if the query is parallel
unsafe or max_worker_processes is exceeded, force_parallel_mode will not work.
This is similar to what we do in this patch. So, maybe it's ok to use "mode". I
didn't change it in the new version of patch. What do you think?

> guess we do not want to force serialize based on this parameter.

Agreed, I refactored the code and modified this point.

Please see the attached patch. I also fix Peter's comments[1]. The GUC name and
design are still under discussion, so I didn't modify them.

By the way, I noticed that the comment for ReorderBufferCheckMemoryLimit() on
HEAD missed something. I fix it in this patch, too.

[1] https://www.postgresql.org/message-id/CAHut%2BPtOjZ_e-KLf26i1XLH2ffPEZGOmGSKy0wDjwyB_uvzxBQ%40mail.gmail.com

Regards,
Shi yu

---

From: Amit Kapila <[email-masked]>
Date: Wed, 14 Dec 2022 17:29:34 +0530

> design are still under discussion, so I didn't modify them.

Let me summarize the discussion on name and design till now. As per my
understanding, we have three requirements: (a) In publisher, stream
each change of transaction instead of waiting till
logical_decoding_work_mem or commit; this will help us to reduce the
test timings of current and future tests for replication of
in-progress transactions; (b) In publisher, serialize each change
instead of waiting till logical_decoding_work_mem; this can help
reduce the test time of tests related to serialization of changes in
logical decoding; (c) In subscriber, during parallel apply for
in-progress transactions (a new feature being discussed at [1]) allow
the system to switch to serialize mode (no more space in shared memory
queue between leader and parallel apply worker either due to a
parallel worker being busy or waiting on some lock) while sending
changes.

Having a GUC that controls these actions/features will allow us to
write tests with shorter duration and better predictability as
otherwise, they require a lot of changes. Apart from tests, these also
help to easily debug the required code. So they fit the Developer
Options category of GUC [2].

We have discussed three different ways to provide GUC for these
features. (1) Have separate GUCs like force_server_stream_mode,
force_server_serialize_mode, force_client_serialize_mode (we can use
different names for these) for each of these; (2) Have two sets of
GUCs for server and client. We can have logical_decoding_mode with
values as 'stream' and 'serialize' for the server and then
logical_apply_serialize = true/false for the client. (3) Have one GUC
like logical_replication_mode with values as 'server_stream',
'server_serialize', 'client_serialize'.

The names used here are tentative mainly to explain each of the
options, we can use different names once we decide among the above.

Thoughts?

[1] - https://www.postgresql.org/message-id/flat/CAA4eK1%2BwyN6zpaHUkCLorEWNx75MG0xhMwcFhvjqm2KURZEAGw%40mail.gmail.com
[2] - https://www.postgresql.org/docs/devel/runtime-config-developer.html

-- 
With Regards,
Amit Kapila.

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 20 Dec 2022 10:48:39 +0530

> Agreed, I refactored the code and modified this point.

After thinking more on this I feel the previous behavior made more
sense.  Because without this patch if we cross the work_mem we try to
stream and if we can not stream for some reason e.g. partial change
then we serialize.  And I feel your previous patch was mimicking the
same behavior for each change.  Now in the new patch, we will try to
stream and if we can not we will queue the change so I feel we are
creating a new patch that actually doesn't exist without the force
mode.

---

From: Dilip Kumar <[email-masked]>
Date: Tue, 20 Dec 2022 10:52:23 +0530

> options, we can use different names once we decide among the above.

I think option 2 makes sense because stream/serialize are two related
options and both are dependent on the same parameter
(logical_decoding_work_mem) so having a single know is logical.  And
another GUC for serializing logical apply.

---

From: Amit Kapila <[email-masked]>
Date: Tue, 20 Dec 2022 14:44:21 +0530

> another GUC for serializing logical apply.

Thanks for your input. Sawada-San, others, any preferences/suggestions?

-- 
With Regards,
Amit Kapila.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 20 Dec 2022 09:16:29 +0000

Dear hackers,

> 'server_serialize', 'client_serialize'.

I also agreed for adding new GUC parameters (and I have already done partially
in parallel apply[1]), and basically options 2 made sense for me. But is it OK
that we can choose "serialize" mode even if subscribers require streaming?

Currently the reorder buffer transactions are serialized on publisher only when
the there are no streamable transaction. So what happen if the
logical_decoding_mode = "serialize" but streaming option streaming is on? If we
break the first one and serialize changes on publisher anyway, it may be not
suitable for testing the normal operation.

Therefore, I came up with the variant of (2): logical_decoding_mode can be
"normal" or "immediate".

"normal" is a default value, which is same as current HEAD. Changes are streamed
or serialized when the buffered size exceeds logical_decoding_work_mem.

When users set to "immediate", the walsenders starts to stream or serialize all
changes. The choice is depends on the subscription option.

In short: +1 for (2) family.

[1]: https://www.postgresql.org/message-id/TYAPR01MB5866160DE81FA2D88B8F22DEF5159%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Amit Kapila <[email-masked]>
Date: Tue, 20 Dec 2022 16:19:42 +0530

> suitable for testing the normal operation.

I think the change will be streamed as soon as the next change is
processed even if we serialize based on this option. See
ReorderBufferProcessPartialChange. However, I see your point that when
the streaming option is given, the value 'serialize' for this GUC may
not make much sense.

> changes. The choice is depends on the subscription option.

The other possibility to achieve what you are saying is that we allow
a minimum value of logical_decoding_work_mem as 0 which would mean
stream or serialize each change depending on whether the streaming
option is enabled. I think we normally don't allow a minimum value
below a certain threshold for other *_work_mem parameters (like
maintenance_work_mem, work_mem), so we have followed the same here.
And, I think it makes sense from the user's perspective because below
a certain threshold it will just add overhead by either writing small
changes to the disk or by sending those over the network. However, it
can be quite useful for testing/debugging. So, not sure, if we should
restrict setting logical_decoding_work_mem below a certain threshold.
What do you think?

-- 
With Regards,
Amit Kapila.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 21 Dec 2022 05:55:41 +0000

Dear Amit,

> option is enabled.

I understood that logical_decoding_work_mem may double as normal option as
developer option. I think yours is smarter because we can reduce # of GUCs.

> I think we normally don't allow a minimum value

You mean to say that there is a possibility that users may set a small value without deep
considerations, right? If so, how about using the approach like autovacuum_work_mem?

autovacuum_work_mem has a range [-1, MAX_KIROBYTES], and -1 mean that it follows
maintenance_work_mem. If it is set small value like 5KB, its working memory is rounded
up to 1024KB. See check_autovacuum_work_mem().

Based on that, I suggest followings. Can they solve the problem what you said?

* If logical_decoding_work_mem is set to 0, all transactions are streamed or serialized
  on publisher.
* If logical_decoding_work_mem is set within [1, 63KB], the value is rounded up or ERROR
  is raised.
* If logical_decoding_work_mem  is set greater than or equal to 64KB, the set value
  is used.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: shveta malik <[email-masked]>
Date: Wed, 21 Dec 2022 12:49:08 +0530

Going with ' logical_decoding_work_mem' seems a reasonable solution, but
since we are mixing
the functionality of developer and production GUC, there is a slight risk
that customer/DBAs may end
up setting it to 0 and forget about it and thus hampering system's
performance.
Have seen many such cases in previous org.

Adding a new developer parameter seems slightly safe, considering we
already have one
such category supported in postgres. It can be on the same line as that of
'force_parallel_mode'.
It will be purely developer GUC, plus if we want to extend something in
future to add/automate
heavier test-cases or any other streaming related dev option, we can extend
the same parameter w/o
disturbing production's one. (see force_parallel_mode=regress for ref).

thanks
Shveta

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 21 Dec 2022 16:21:55 +0900

> restrict setting logical_decoding_work_mem below a certain threshold.

I agree with (2), having separate GUCs for publisher side and
subscriber side. Also, on the publisher side, Amit's idea, controlling
the logical decoding behavior by changing logical_decoding_work_mem,
seems like a good idea.

But I'm not sure it's a good idea if we lower the minimum value of
logical_decoding_work_mem to 0. I agree it's helpful for testing and
debugging but setting logical_decoding_work_mem = 0 doesn't benefit
users at all, rather brings risks.

I prefer the idea Kuroda-san previously proposed; setting
logical_decoding_mode = 'immediate' means setting
logical_decoding_work_mem = 0. We might not need to have it as an enum
parameter since it has only two values, though.

Regards,

---

From: Peter Smith <[email-masked]>
Date: Wed, 21 Dec 2022 19:05:22 +1100

Here are some review comments for patch v2.

Since the GUC is still under design maybe these comments can be
ignored for now, but I guess similar comments will apply in future
anyhow (just with some name changes).

======

1. Commit message

Add a new GUC force_stream_mode, when it is set on, send the change to
output plugin immediately in streaming mode. Otherwise, send until
logical_decoding_work_mem is exceeded.

~

Is that quite right? I thought it was more like shown below:

SUGGESTION
Add a new GUC 'force_stream_mode' which modifies behavior when
streaming mode is enabled. If force_stream_mode is on the changes are
sent to the output plugin immediately. Otherwise,(when
force_stream_mode is off) changes are written to memory until
logical_decoding_work_mem is exceeded.

======

2. doc/src/sgml/config.sgml

+       <para>
+        Specifies whether to force sending the changes to output plugin
+        immediately in streaming mode. If set to <literal>off</literal> (the
+        default), send until <varname>logical_decoding_work_mem</varname> is
+        exceeded.
+       </para>

Suggest slight rewording like #1.

SUGGESTION
This modifies the behavior when streaming mode is enabled. If set to
<literal>on</literal> the changes are sent to the output plugin
immediately. If set <literal>off</literal> (the default), changes are
written to memory until <varname>logical_decoding_work_mem</varname>
is exceeded.

======

3. More docs.

It might be helpful if this developer option is referenced also on the
page "31.10.1 Logical Replication > Configuration Settings >
Publishers" [1]

======

4. GUCs

+/*
+ * Whether to send the change to output plugin immediately in streaming mode.
+ * When it is off, wait until logical_decoding_work_mem is exceeded.
+ */
+bool force_stream_mode;

4a.
"to output plugin" -> "to the output plugin"

~

4b.
By convention (see. [2]) IIUC there should be some indication that
these (this and 'logical_decoding_work_mem') are GUC variables. e.g.
these should be refactored to be grouped togther without the other
static var in between. And add a comment for them both like:

/* GUC variable. */

~

4c.
Also, (see [2]) it makes the code more readable to give the GUC an
explicit initial value.

SUGGESTION
bool force_stream_mode = false;

~~~

5. ReorderBufferCheckMemoryLimit

+ /* we know there has to be one, because the size is not zero */

Uppercase comment. Although not strictly added by this patch you might
as well make this change while adjusting the indentation.

======

src/backend/utils/misc/guc_tables.c

6.

+ {
+ {"force_stream_mode", PGC_USERSET, DEVELOPER_OPTIONS,
+ gettext_noop("Force sending the changes to output plugin immediately
if streaming is supported, without waiting till
logical_decoding_work_mem."),
+ NULL,
+ GUC_NOT_IN_SAMPLE
+ },
+ &force_stream_mode,
+ false,
+ NULL, NULL, NULL
+ },

"without waiting till logical_decoding_work_mem.".... seem like an
incomplete sentence

SUGGESTION
Force sending any streaming changes to the output plugin immediately
without waiting until logical_decoding_work_mem is exceeded."),

------
[1] https://www.postgresql.org/docs/devel/logical-replication-config.html
[2] GUC declarations -
https://github.com/postgres/postgres/commit/d9d873bac67047cfacc9f5ef96ee488f2cb0f1c3

Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Peter Smith <[email-masked]>
Date: Wed, 21 Dec 2022 19:25:31 +1100

> parameter since it has only two values, though.

Did you mean one GUC (logical_decoding_mode) will cause a side-effect
implicit value change on another GUC value
(logical_decoding_work_mem)?

If so, then that seems a like potential source of confusion IMO.
- e.g. actual value is invisibly set differently from what the user
sees in the conf file
- e.g. will it depend on the order they get assigned

Are there any GUC precedents for something like that?

------
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Amit Kapila <[email-masked]>
Date: Wed, 21 Dec 2022 14:24:07 +0530

> (logical_decoding_work_mem)?

I don't think that is required. The only value that can be allowed for
logical_decoding_mode will be "immediate", something like we do for
recovery_target. The default will be "". The "immediate" value will
mean that stream each change if the "streaming" option is enabled
("on" of "parallel") or if "streaming" is not enabled then that would
mean serializing each change.

-- 
With Regards,
Amit Kapila.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 21 Dec 2022 09:50:12 +0000

Dear Shveta, other hackers

> Going with ' logical_decoding_work_mem' seems a reasonable solution, but since we are mixing
> the functionality of developer and production GUC, there is a slight risk that customer/DBAs may end
> up setting it to 0 and forget about it and thus hampering system's performance.

That never crossed my mind at all. Indeed, DBA may be confused, this proposal seems to be too optimized.
I can withdraw this and we can go another way, maybe my previous approach.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 21 Dec 2022 13:14:09 +0000

> mean serializing each change.

I agreed and updated the patch as Amit suggested.
Please see the attached patch.

Regards,
Shi yu

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 21 Dec 2022 13:17:18 +0000

> page "31.10.1 Logical Replication > Configuration Settings >

The new added GUC is developer option, and it seems that most developer options
are not referenced on other pages. So, I am not sure if we need to add this to
other docs.

Other comments are fixed [1]. (Some of them are ignored because of the new
design.)

[1] https://www.postgresql.org/message-id/OSZPR01MB6310AAE12BC281158880380DFDEB9%40OSZPR01MB6310.jpnprd01.prod.outlook.com

Regards,
Shi yu

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 21 Dec 2022 22:54:44 +0900

> Please see the attached patch.

The patch looks good to me. Some minor comments are:

- * limit, but we might also adapt a more elaborate eviction strategy
- for example
- * evicting enough transactions to free certain fraction (e.g. 50%)
of the memory
- * limit.
+ * limit, but we might also adapt a more elaborate eviction strategy - for
+ * example evicting enough transactions to free certain fraction (e.g. 50%) of
+ * the memory limit.

This change is not relevant with this feature.

---
+        if (logical_decoding_mode == LOGICAL_DECODING_MODE_DEFAULT
+                && rb->size < logical_decoding_work_mem * 1024L)

Since we put '&&' before the new line in all other places in
reorderbuffer.c, I think it's better to make it consistent. The same
is true for the change for while loop in the patch.

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Thu, 22 Dec 2022 12:35:46 +0530

> is true for the change for while loop in the patch.

I have addressed these comments in the attached. Additionally, I have
modified the docs and commit messages to make those clear. I think
instead of adding new tests with this patch, it may be better to
change some of the existing tests related to streaming to use this
parameter as that will clearly show one of the purposes of this patch.

-- 
With Regards,
Amit Kapila.

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 22 Dec 2022 16:16:32 +0900

On Thu, Dec 22, 2022 at 4:05 PM Amit Kapila <[email-masked]> wrote:
> modified the docs and commit messages to make those clear.

Thanks!

>  I think

+1. I think test_decoding/sql/stream.sql and spill.sql are good
candidates and we change logical replication TAP tests in a separate
patch.

Regards,

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Thu, 22 Dec 2022 07:18:45 +0000

Dear Amit,

Thank you for updating the patch. I have also checked the patch
and basically it has worked well. Almost all things I found were modified
by v4.

One comment: while setting logical_decoding_mode to wrong value,
I got unfriendly ERROR message.

```
postgres=# SET logical_decoding_mode = 1;
ERROR:  invalid value for parameter "logical_decoding_mode": "1"
HINT:  Available values: , immediate
```

Here all acceptable enum should be output as HINT, but we could not see the empty string.
Should we modify config_enum_get_options() for treating empty string, maybe
like (empty)? Or we can ignore now.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 22 Dec 2022 16:45:07 +0900 (JST)

At Thu, 22 Dec 2022 12:35:46 +0530, Amit Kapila <[email-masked]> wrote in 
> parameter as that will clearly show one of the purposes of this patch.

Being late but I'm warried that we might sometime regret that the lack
of the explicit default. Don't we really need it?

+        Allows streaming or serializing changes immediately in logical decoding.
+        The allowed values of <varname>logical_decoding_mode</varname> are the
+        empty string and <literal>immediate</literal>. When set to
+        <literal>immediate</literal>, stream each change if
+        <literal>streaming</literal> option is enabled, otherwise, serialize
+        each change.  When set to an empty string, which is the default,
+        decoding will stream or serialize changes when
+        <varname>logical_decoding_work_mem</varname> is reached.

With (really) fresh eyes, I took a bit long time to understand what
the "streaming" option is. Couldn't we augment the description by a
bit?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: Masahiko Sawada <[email-masked]>
Date: Thu, 22 Dec 2022 16:59:30 +0900

On Thu, Dec 22, 2022 at 4:18 PM Hayato Kuroda (Fujitsu)
<[email-masked]> wrote:
> like (empty)?

Good point. I think the hint message can say "The only allowed value
is \"immediate\" as recovery_target does. Or considering the name of
logical_decoding_mode, we might want to have a non-empty string, say
'normal' as Kuroda-san proposed, as the default value.

Regards,

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 22 Dec 2022 17:25:40 +0900 (JST)

At Thu, 22 Dec 2022 16:59:30 +0900, Masahiko Sawada <[email-masked]> wrote in 
> 'normal' as Kuroda-san proposed, as the default value.

Oh. I missed this, and agree to have the explicit default option.
I slightly prefer "buffered" but "normal" also works fine for me.

regards.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 22 Dec 2022 14:46:04 +0530

> I slightly prefer "buffered" but "normal" also works fine for me.

+1 for "buffered" as that seems to convey the meaning better.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 22 Dec 2022 14:49:40 +0530

> candidates and we change logical replication TAP tests in a separate

I prefer the other way, let's first do TAP tests because that will
also help immediately with the parallel apply feature. We need to
execute most of those tests in parallel mode.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 22 Dec 2022 14:53:34 +0530

> of the explicit default. Don't we really need it?

For this, I like your proposal for "buffered" as an explicit default value.

> the "streaming" option is. Couldn't we augment the description by a

Okay, how about modifying it to: "When set to
<literal>immediate</literal>, stream each change if
<literal>streaming</literal> option (see optional parameters set by
CREATE SUBSCRIPTION) is enabled, otherwise, serialize each change.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 22 Dec 2022 12:48:37 +0000

> CREATE SUBSCRIPTION) is enabled, otherwise, serialize each change.

I updated the patch to use "buffered" as the explicit default value, and include
Amit's changes about document.

Besides, I tried to reduce data size in streaming subscription tap tests by this
new GUC (see 0002 patch). But I didn't covert all streaming tap tests because I
think we also need to cover the case that there are lots of changes. So, 015* is
not modified. And 017* is not modified because streaming transactions and
non-streaming transactions are tested alternately in this test.

I collected the time to run these tests before and after applying the patch set
on my machine. In debug version, it saves about 5.3 s; and in release version,
it saves about 1.8 s. The time of each test is attached.

Regards,
Shi yu

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Fri, 23 Dec 2022 11:25:24 +0900 (JST)

At Thu, 22 Dec 2022 14:53:34 +0530, Amit Kapila <[email-masked]> wrote in 
> For this, I like your proposal for "buffered" as an explicit default value.

Good to hear.

> CREATE SUBSCRIPTION) is enabled, otherwise, serialize each change.

Looks fine. Thanks!

regards.

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 23 Dec 2022 12:56:28 +0900

> execute most of those tests in parallel mode.

Good point. Or we can do both if changes for test_decoding tests are not huge?

Regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Fri, 23 Dec 2022 14:02:01 +0900

On Thu, Dec 22, 2022 at 9:48 PM [email-masked]
<[email-masked]> wrote:
> not modified.

If we want to eventually convert 015 some time, isn't it better to
include it even if it requires many changes? Is there any reason we
want to change 017 in a separate patch?

> And 017* is not modified because streaming transactions and

How about 029_on_error.pl? It also sets logical_decoding_work_mem to
64kb to test the STREAM COMMIT case.

> it saves about 1.8 s. The time of each test is attached.

Nice improvements.

Regards,

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 23 Dec 2022 05:18:45 +0000

Dear Shi,

Thanks for updating the patch! Followings are comments.

ReorderBufferCheckMemoryLimit()

```
+	/*
+	 * Bail out if logical_decoding_mode is disabled and we haven't exceeded
+	 * the memory limit.
+	 */
```

I think 'disabled' should be '"buffered"'.

```
+	 * If logical_decoding_mode is immediate, loop until there's no change.
+	 * Otherwise, loop until we reach under the memory limit. One might think
+	 * that just by evicting the largest (sub)transaction we will come under
+	 * the memory limit based on assumption that the selected transaction is
+	 * at least as large as the most recent change (which caused us to go over
+	 * the memory limit). However, that is not true because a user can reduce
+	 * the logical_decoding_work_mem to a smaller value before the most recent
 	 * change.
 	 */
```

Do we need to pick the largest (sub)transaciton even if we are in the immediate mode?
It seems that the liner search is done in ReorderBufferLargestStreamableTopTXN()
to find the largest transaction, but in this case we can choose the arbitrary one.

reorderbuffer.h

+/* possible values for logical_decoding_mode */
+typedef enum
+{
+	LOGICAL_DECODING_MODE_BUFFERED,
+	LOGICAL_DECODING_MODE_IMMEDIATE
+}			LogicalDecodingMode;

I'm not sure, but do we have to modify typedefs.list?

Moreover, I have also checked the improvements of elapsed time.
All builds were made by meson system, and the unit of each cells is second.
It seemed that results have same trends with Shi.

Debug build:

	HEAD	PATCH	Delta
16	6.44	5.96	0.48
18	6.92	6.47	0.45
19	5.93	5.91	0.02
22	14.98	13.7	1.28
23	12.01	8.22	3.79

SUM of delta: 6.02s

Release build:

	HEAD	PATCH	Delta
16	3.51	3.22	0.29
17	4.04	3.48	0.56
19	3.43	3.3	0.13
22	10.06	8.46	1.6
23	6.74	5.39	1.35

SUM of delta: 3.93s

I will check and report the test coverage if I can.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Amit Kapila <[email-masked]>
Date: Fri, 23 Dec 2022 10:49:08 +0530

On Fri, Dec 23, 2022 at 10:32 AM Masahiko Sawada <[email-masked]> wrote:
> include it even if it requires many changes?
>

I think there is some confusion here because the point is that we
don't want to convert all the tests. It would be good to have some
tests which follow the regular path of logical_decoding_work_mem.

> Is there any reason we

Here also, the idea is to leave it as it is. It has a mix of stream
and non-stream cases and it would be tricky to convert it because we
need to change GUC before streamed one and reload the config and
ensure reloaded config is in effect.

> 64kb to test the STREAM COMMIT case.

How would you like to change this? Do we want to enable the GUC or
streaming option just before that case and wait for it? If so, that
might take more time than we save.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Fri, 23 Dec 2022 11:02:27 +0530

> to find the largest transaction, but in this case we can choose the arbitrary one.

In immediate mode, we will stream/spill each change, so ideally, we
don't need to perform any search. Otherwise, also, I think changing
those functions will complicate the code without serving any purpose.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Fri, 23 Dec 2022 11:20:22 +0530

> non-streaming transactions are tested alternately in this test.

I think we can remove the newly added test from the patch and instead
combine the 0001 and 0002 patches. I think we should leave the
022_twophase_cascade as it is because it can impact code coverage,
especially the below part of the test:
# 2PC PREPARE with a nested ROLLBACK TO SAVEPOINT
$node_A->safe_psql(
    'postgres', "
    BEGIN;
    INSERT INTO test_tab VALUES (9999, 'foobar');
    SAVEPOINT sp_inner;
    INSERT INTO test_tab SELECT i, md5(i::text) FROM
generate_series(3, 5000) s(i);

Here, we will stream first time after the subtransaction, so can
impact the below part of the code in ReorderBufferStreamTXN:
if (txn->snapshot_now == NULL)
{
...
dlist_foreach(subxact_i, &txn->subtxns)
{
ReorderBufferTXN *subtxn;

subtxn = dlist_container(ReorderBufferTXN, node, subxact_i.cur);
ReorderBufferTransferSnapToParent(txn, subtxn);
}
...

-- 
With Regards,
Amit Kapila.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 23 Dec 2022 07:42:09 +0000

Dear hackers,

> I will check and report the test coverage if I can.

I ran make coverage. PSA the screen shot that shows results.
According to the result the coverage seemed to be not changed
even if the elapsed time was reduced.

Only following lines at process_syncing_tables_for_apply() seemed to be not hit after patching,
but I thought it was the timing issue because we do not modify around there.

```
					/*
					 * Enter busy loop and wait for synchronization worker to
					 * reach expected state (or die trying).
					 */
					if (!started_tx)
					{
						StartTransactionCommand();
						started_tx = true;
					}
```

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Amit Kapila <[email-masked]>
Date: Fri, 23 Dec 2022 13:59:37 +0530

>                                                 StartTransactionCommand();

This part of the code is related to synchronization between apply and
sync workers which depends upon timing. So, we can ignore this
difference.

-- 
With Regards,
Amit Kapila.

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 23 Dec 2022 08:32:45 +0000

On Fri, Dec 23, 2022 1:50 PM Amit Kapila <[email-masked]>
> ReorderBufferTransferSnapToParent(txn, subtxn);

OK, I removed the modification in 022_twophase_cascade.pl and combine the two patches.

Please see the attached patch.
I also fixed Kuroda-san's comments[1].

[1] https://www.postgresql.org/message-id/TYAPR01MB5866CD99CF86EAC84119BC91F5E99%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Regards,
Shi yu

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 23 Dec 2022 09:12:18 +0000

Dear Shi,

> I also fixed Kuroda-san's comments[1].

Thanks for updating the patch! I confirmed that my comments were fixed and
your patch could pass subscription and test_decoding tests. I think we can
change the status to "Ready for Committer".

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: shveta malik <[email-masked]>
Date: Fri, 23 Dec 2022 15:15:54 +0530

On Fri, Dec 23, 2022 at 2:03 PM [email-masked] <[email-masked]>
wrote:
> > > think we also need to cover the case that there are lots of changes.
So, 015*
> > is
> [1]
https://www.postgresql.org/message-id/TYAPR01MB5866CD99CF86EAC84119BC91F5E99%40TYAPR01MB5866.jpnprd01.prod.outlook.com
> Shi yu

Hello,
I ran tests (4 runs) on both versions (v5 and v6) in release mode. And the
data looks promising, time is reduced now:

  HEAD                                 V5
    Delta (sec)
2:20.535307                      2:15.865241
4.670066
2:19.220917                      2:14.445312
4.775605
2:22.492128                      2:17.35755
 5.134578
2:20.737309                      2:15.564306
5.173003

HEAD                                V6
   Delta (sec)
2:20.535307                   2:15.363567
5.17174
2:19.220917                   2:15.079082
4.14.1835
2:22.492128                   2:16.244139
6.247989
2:20.737309                   2:16.108033
4.629276

thanks
Shveta

---

From: Masahiko Sawada <[email-masked]>
Date: Sat, 24 Dec 2022 02:01:14 +0900

> OK, I removed the modification in 022_twophase_cascade.pl and combine the two patches.

Thank you for updating the patch. The v6 patch looks good to me.

Regards,

---

From: Amit Kapila <[email-masked]>
Date: Sat, 24 Dec 2022 08:56:14 +0530

> Thank you for updating the patch. The v6 patch looks good to me.

LGTM as well. I'll push this on Monday.

-- 
With Regards,
Amit Kapila.

---

From: Dilip Kumar <[email-masked]>
Date: Sat, 24 Dec 2022 15:28:02 +0530

> LGTM as well. I'll push this on Monday.

The patch looks good to me.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 26 Dec 2022 14:04:28 +0530

> The patch looks good to me.

Pushed.

-- 
With Regards,
Amit Kapila.

---

From: Andres Freund <[email-masked]>
Date: Tue, 27 Dec 2022 12:12:32 -0800

Hi,

> Pushed.

I did not follow this thread but saw the commit. Could you explain why a GUC
is the right mechanism here? The commit message didn't explain why a GUC was
chosen.

To me an option like this should be passed in when decoding rather than a GUC.

Greetings,

Andres Freund

---

From: Amit Kapila <[email-masked]>
Date: Wed, 28 Dec 2022 07:49:56 +0530

> To me an option like this should be passed in when decoding rather than a GUC.

For that, we need to have a subscription option for this as we need to
reduce test timing for streaming TAP tests (by streaming, I mean
replication of large in-progress transactions) as well. Currently,
there is no subscription option that is merely used for
testing/debugging purpose and there doesn't seem to be a use for this
for users. So, we didn't want to expose it as a user option. There is
also a risk that if users use it they will face a slowdown.

-- 
With Regards,
Amit Kapila.