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

On Sun, May 29, 2022 8:25 PM osumi.takamichi@fujitsu.com <osumi.takamichi@fujitsu.com> wrote:
> Hi,
> 
> 
> Some review comments on the new patches from v6-0001 to v6-0004.
Thanks for your comments.

> <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.
> "
Add the sentences as suggested.

> (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.
Yes, we should input "true" to 2nd argument here to log "apply error".
And after checking the second point you mentioned, I think it is caused by the
first point you mentioned and another reason:
With the patch v6 (or v7) and we specify option "apply", when a streamed
transaction causes an error (duplication error), the function
pgstat_report_subscription_error is invoke twice (in main apply worker and
apply background worker, see function ApplyWorkerMain()->start_apply() and
ApplyBgworkerMain). This means for one same error, we will send twice stats
message.
So to fix this, I removed the code that you mentioned and then, just invoke
function LogicalApplyBgwLoop here.

> <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.
Improve the formatting as you suggested. And I run pgindent for new patches.

> (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.
Yes, you are right. Thank for your reminder.
I added this in the paragraph introducing value "apply" in
create_subscription.sgml:
```
To run in this mode, there are following two requirements. The first
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.
```
Also added the related tests. (refer to 032_streaming_apply.pl in v8-0003)

I also made some other changes. The new patches and the modification details
were attached in [1].

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

Regards,
Wang wei