Re: Perform streaming logical transactions by background workers and parallel apply

Below are some review comments for patch v14-0004:

========
v14-0004
========

4.0 General.

This comment is an after-thought but as I write this mail I am
wondering if most of this 0004 patch is even necessary at all? Instead
of introducing a new column and all the baggage that goes with it,
can't the same functionality be achieved just by toggling the
streaming mode 'substream' value from 'p' (parallel) to 't' (on)
whenever an error occurs causing a retry? Anyway, if you do change it
this way then most of the following comments can be disregarded.


======

4.1 Commit message

Patch needs an explanatory commit message. Currently, there is nothing.

======

4.2 doc/src/sgml/catalogs.sgml

+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>subretry</structfield> <type>bool</type>
+      </para>
+      <para>
+       If true, the subscription will not try to apply streaming transaction
+       in <literal>parallel</literal> mode. See
+       <xref linkend="sql-createsubscription"/> for more information.
+      </para></entry>
+     </row>

I think it is overkill to mention anything about the
streaming=parallel here because IIUC it is nothing to do with this
field at all. I thought you really only need to say something brief
like:

SUGGESTION:
True if the previous apply change failed and a retry was required.

======

4.3 doc/src/sgml/ref/create_subscription.sgml

@@ -244,6 +244,10 @@ CREATE SUBSCRIPTION <replaceable
class="parameter">subscription_name</replaceabl
           is that the unique column should be the same between publisher and
           subscriber; the second is that there should not be any non-immutable
           function in subscriber-side replicated table.
+          When applying a streaming transaction, if either requirement is not
+          met, the background worker will exit with an error. And when retrying
+          later, we will try to apply this transaction in <literal>on</literal>
+          mode.
          </para>

I did not think it is good to say "we" in the docs.

SUGGESTION
When applying a streaming transaction, if either requirement is not
met, the background worker will exit with an error. Parallel mode is
disregarded when retrying; instead the transaction will be applied
using <literal>streaming = on</literal>.

======

4.4 .../replication/logical/applybgworker.c

+ /*
+ * We don't start new background worker if retry was set as it's possible
+ * that the last time we tried to apply a transaction in background worker
+ * and the check failed (see function apply_bgworker_relation_check). So
+ * we will try to apply this transaction in apply worker.
+ */

SUGGESTION (simplified, and remove "we")
Don't use apply background workers for retries, because it is possible
that the last time we tried to apply a transaction using an apply
background worker the checks failed (see function
apply_bgworker_relation_check).

~~~

4.5

+ elog(DEBUG1, "retry to apply an streaming transaction in apply "
+ "background worker");

IMO the log message is too confusing

SUGGESTION
"apply background workers are not used for retries"

======

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

4.6.a - apply_handle_commit

+ /* Set the flag that we will not retry later. */
+ set_subscription_retry(false);

But the comment is wrong, isn't it? Shouldn’t it just say that we are
not *currently* retrying? And can’t this just anyway be redundant if
only the catalog column has a DEFAULT value of false?

4.6.b - apply_handle_prepare
Ditto

4.6.c - apply_handle_commit_prepared
Ditto

4.6.d - apply_handle_rollback_prepared
Ditto

4.6.e - apply_handle_stream_prepare
Ditto

4.6.f - apply_handle_stream_abort
Ditto

4.6.g - apply_handle_stream_commit
Ditto

~~~

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

4.7.a - start_table_sync

@@ -3894,6 +3917,9 @@ start_table_sync(XLogRecPtr *origin_startpos,
char **myslotname)
  }
  PG_CATCH();
  {
+ /* Set the flag that we will retry later. */
+ set_subscription_retry(true);

Maybe this should say more like "Flag that the next apply will be the
result of a retry"

4.7.b - start_apply
Ditto

~~~

4.8 src/backend/replication/logical/worker.c - set_subscription_retry

+
+/*
+ * Set subretry of pg_subscription catalog.
+ *
+ * If retry is true, subscriber is about to exit with an error. Otherwise, it
+ * means that the changes was applied successfully.
+ */
+static void
+set_subscription_retry(bool retry)

"changes" -> "change" ?

~~~

4.8 src/backend/replication/logical/worker.c - set_subscription_retry

Isn't this flag only every used when streaming=parallel? But it does
not seem ot be checking that anywhere before potentiall executing all
this code when maybe will never be used.

======

4.9 src/include/catalog/pg_subscription.h

@@ -76,6 +76,8 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId)
BKI_SHARED_RELATION BKI_ROW
  bool subdisableonerr; /* True if a worker error should cause the
  * subscription to be disabled */

+ bool subretry; /* True if the previous apply change failed. */

I was wondering if you can give this column a DEFAULT value of false,
because then perhaps most of the patch code from worker.c may be able
to be eliminated.

======

4.10 .../subscription/t/032_streaming_apply.pl

I felt that the test cases all seem to blend together. IMO it will be
more readable if the main text parts are  visually separated

e.g using a comment like:
# =================================================


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