On Sun, May 29, 2022 8:25 PM osumi.takami...@fujitsu.com
<osumi.takami...@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
