Re: logical replication restrictions

Here are some review comments for your v4-0001 patch. I hope they are
useful for you.

======

1. General

This thread name "logical replication restrictions" seems quite
unrelated to the patch here. Maybe it's better to start a new thread
otherwise nobody is going to recognise what this thread is really
about.

======

2. Commit message

Similar to physical replication, a time-delayed copy of the data for
logical replication is useful for some scenarios (specially to fix
errors that might cause data loss).

"specially" -> "particularly" ?

~~~

3. Commit message

Maybe take some examples from the regression tests to show usage of
the new parameter

======

4. doc/src/sgml/catalogs.sgml

+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>subapplydelay</structfield> <type>int8</type>
+      </para>
+      <para>
+       Delay the application of changes by a specified amount of time.
+      </para></entry>
+     </row>

I think this should say that the units are ms.

======

5. doc/src/sgml/ref/create_subscription.sgml

+       <varlistentry>
+        <term><literal>min_apply_delay</literal> (<type>integer</type>)</term>
+        <listitem>

Is the "integer" type here correct? It might eventually be stored as
an integer, but IIUC (going by the tests) from the user point-of-view
this parameter is really "text" type for representing ms or interval,
right?

~~~

6. doc/src/sgml/ref/create_subscription.sgml

 Similar
+          to the physical replication feature
+          (<xref linkend="guc-recovery-min-apply-delay"/>), it may be useful to
+          have a time-delayed copy of data for logical replication.

SUGGESTION
As with the physical replication feature (recovery_min_apply_delay),
it can be useful for logical replication to delay the data
replication.

~~~

7. doc/src/sgml/ref/create_subscription.sgml

Delays in logical
+          decoding and in transfer the transaction may reduce the actual wait
+          time.

SUGGESTION
Time spent in logical decoding and in transferring the transaction may
reduce the actual wait time.

~~~

8. doc/src/sgml/ref/create_subscription.sgml

If the system clocks on publisher and subscriber are not
+          synchronized, this may lead to apply changes earlier than expected.

Why just say "earlier than expected"? If the publisher's time is ahead
of the subscriber then the changes might also be *later* than
expected, right? So, perhaps it is better to just say "other than
expected".

~~~

9. doc/src/sgml/ref/create_subscription.sgml

Should there also be a big warning box about the impact if using
synchronous_commit (like the other streaming replication page has this
warning)?

~~~

10. doc/src/sgml/ref/create_subscription.sgml

I think there should be some examples somewhere showing how to specify
this parameter. Maybe they are better added somewhere in "31.2
Subscription" and xrefed from here.

======

11. src/backend/commands/subscriptioncmds.c - parse_subscription_options

I think there should be a default assignment to 0 (done where all the
other supported option defaults are set)

~~~

12. src/backend/commands/subscriptioncmds.c - parse_subscription_options

+ if (opts->min_apply_delay < 0)
+ ereport(ERROR,
+ errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("option \"%s\" must not be negative", "min_apply_delay"));
+

I thought this check only needs to be do within scope of the preceding
if - (IsSet(supported_opts, SUBOPT_MIN_APPLY_DELAY) &&
strcmp(defel->defname, "min_apply_delay") == 0)

======

13. src/backend/commands/subscriptioncmds.c - AlterSubscription

@@ -1093,6 +1126,17 @@ AlterSubscription(ParseState *pstate,
AlterSubscriptionStmt *stmt,
  if (opts.enabled)
  ApplyLauncherWakeupAtCommit();

+ /*
+ * If this subscription has been disabled and it has an apply
+ * delay set, wake up the logical replication worker to finish
+ * it as soon as possible.
+ */
+ if (!opts.enabled && sub->applydelay > 0)

I did not really understand the logic why should the min_apply_delay
override the enabled=false? It is a called *minimum* delay so if it
ends up being way over the parameter value (because the subscription
is disabled) then why does that matter?

======

14. src/backend/replication/logical/worker.c

@@ -252,6 +252,7 @@ WalReceiverConn *LogRepWorkerWalRcvConn = NULL;

 Subscription *MySubscription = NULL;
 static bool MySubscriptionValid = false;
+TimestampTz MySubscriptionMinApplyDelayUntil = 0;

Looking at the only usage of this variable (in apply_delay) and how it
is used I did see why this cannot just be a local member of the
apply_delay function?

~~~

15. src/backend/replication/logical/worker.c - apply_delay

+/*
+ * Apply the informed delay for the transaction.
+ *
+ * A regular transaction uses the commit time to calculate the delay. A
+ * prepared transaction uses the prepare time to calculate the delay.
+ */
+static void
+apply_delay(TimestampTz ts)

I didn't think it needs to mention here about the different kinds of
transactions because where it comes from has nothing really to do with
this function's logic.

~~~

16. src/backend/replication/logical/worker.c - apply_delay

Refer to comment #14 about MySubscriptionMinApplyDelayUntil.

~~~

17. src/backend/replication/logical/worker.c - apply_handle_stream_prepare

@@ -1090,6 +1146,19 @@ apply_handle_stream_prepare(StringInfo s)

  elog(DEBUG1, "received prepare for streamed transaction %u",
prepare_data.xid);

+ /*
+ * Should we delay the current prepared transaction?
+ *
+ * Although the delay is applied in BEGIN PREPARE messages, streamed
+ * prepared transactions apply the delay in a STREAM PREPARE message.
+ * That's ok because no changes have been applied yet
+ * (apply_spooled_messages() will do it).
+ * The STREAM START message does not contain a prepare time (it will be
+ * available when the in-progress prepared transaction finishes), hence, it
+ * was not possible to apply a delay at that time.
+ */
+ apply_delay(prepare_data.prepare_time);
+

It seems to rely on the spooling happening at the end. But won't this
cause a problem later when/if the "parallel apply" patch [1] is pushed
and the stream bgworkers are doing stuff on the fly instead of
spooling at the end?

Or are you expecting that the "parallel apply" feature should be
disabled if there is any min_apply_delay parameter specified?

~~~

18. src/backend/replication/logical/worker.c - apply_handle_stream_commit

Ditto comment #17.

======

19. src/bin/psql/tab-complete.c

Let's keep the alphabetical order of the parameters in COMPLETE_WITH, as per [2]

======

20. src/include/catalog/pg_subscription.h

@@ -58,6 +58,8 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId)
BKI_SHARED_RELATION BKI_ROW
  XLogRecPtr subskiplsn; /* All changes finished at this LSN are
  * skipped */

+ int64 subapplydelay; /* Replication apply delay */
+

IMO the comment should mention the units "(ms)"

======

21. src/test/regress/sql/subscription.sql

There are some test cases for CREATE SUBSCRIPTION but there are no
test cases for ALTER SUBSCRIPTION changing this new parameter.

====

22. src/test/subscription/t/032_apply_delay.pl

I received the following error when trying to run these 'subscription' tests:

t/032_apply_delay.pl ............... No such class log_location at
t/032_apply_delay.pl line 49, near "my log_location"
syntax error at t/032_apply_delay.pl line 49, near "my log_location ="
Global symbol "$log_location" requires explicit package name at
t/032_apply_delay.pl line 103.
Global symbol "$log_location" requires explicit package name at
t/032_apply_delay.pl line 105.
Global symbol "$log_location" requires explicit package name at
t/032_apply_delay.pl line 105.
Global symbol "$log_location" requires explicit package name at
t/032_apply_delay.pl line 107.
Global symbol "$sect" requires explicit package name at
t/032_apply_delay.pl line 108.
Execution of t/032_apply_delay.pl aborted due to compilation errors.
t/032_apply_delay.pl ............... Dubious, test returned 255 (wstat
65280, 0xff00)
No subtests run
t/100_bugs.pl ...................... ok

Test Summary Report
-------------------
t/032_apply_delay.pl             (Wstat: 65280 Tests: 0 Failed: 0)
  Non-zero exit status: 255
  Parse errors: No plan found in TAP output

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

Kind Regards,
Peter Smith.
Fujitsu Australia