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

On Wednesday, May 25, 2022 11:25 AM wangw.fnst@fujitsu.com <wangw.fnst@fujitsu.com> wrote:
> Attach the patches. (Did not change v6-0001 and v6-0002.)
Hi,


Some review comments on the new patches from v6-0001 to v6-0004.

<v6-0001>

(1) create_subscription.sgml

+          the transaction is committed. Note that if an error happens when
+          applying changes in a background worker, it might not report the
+          finish LSN of the remote transaction in the server log.

I suggest to add a couple of sentences like below
to the section of logical-replication-conflicts in logical-replication.sgml.

"
Setting streaming mode to 'apply' can export invalid LSN as
finish LSN of failed transaction. Changing the streaming mode and
making the same conflict writes the finish LSN of the
failed transaction in the server log if required.
"

(2) ApplyBgworkerMain


+       PG_TRY();
+       {
+               LogicalApplyBgwLoop(mqh, pst);
+       }
+       PG_CATCH();
+       {

...

+               pgstat_report_subscription_error(MySubscription->oid, false);
+
+               PG_RE_THROW();
+       }
+       PG_END_TRY();


When I stream a transaction in-progress and it causes an error(duplication error),
seemingly the subscription stats (values in pg_stat_subscription_stats) don't
get updated properly. The 2nd argument should be true for apply error.

Also, I observe that both apply_error_count and sync_error_count
get updated together by error. I think we need to check this point as well.


<v6-0003>


(3) logicalrep_write_attrs

+       if (rel->rd_rel->relhasindex)
+       {
+               List       *indexoidlist = RelationGetIndexList(rel);
+               ListCell   *indexoidscan;
+               foreach(indexoidscan, indexoidlist)

and

+                       if (indexRel->rd_index->indisunique)
+                       {
+                               int             i;
+                               /* Add referenced attributes to idindexattrs */
+                               for (i = 0; i < indexRel->rd_index->indnatts; i++)

We don't have each blank line after variable declarations.
There might be some other codes where this point can be applied.
Please check.


(4)

+       /*
+        * If any unique index exist, check that they are same as remoterel.
+        */
+       if (!rel->sameunique)
+               ereport(ERROR,
+                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                errmsg("cannot replicate relation with different unique index"),
+                                errhint("Please change the streaming option to 'on' instead of 'apply'.")));


When I create a logical replication setup with different constraints
and let streaming of in-progress transaction run,
I keep getting this error.

This should be documented as a restriction or something,
to let users know the replication progress can't go forward by
any differences written like in the commit-message in v6-0003.

Also, it would be preferable to test this as well, if we
don't dislike having TAP tests for this.


Best Regards,
    Takamichi Osumi