Skipping logical replication transactions on subscriber side

On Mon, May 24, 2021 at 1:32 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

If a logical replication worker cannot apply the change on the
subscriber for some reason (e.g., missing table or violating a
constraint, etc.), logical replication stops until the problem is
resolved. Ideally, we resolve the problem on the subscriber (e.g., by
creating the missing table or removing the conflicting data, etc.) but
occasionally a problem cannot be fixed and it may be necessary to skip
the entire transaction in question. Currently, we have two ways to
skip transactions: advancing the LSN of the replication origin on the
subscriber and advancing the LSN of the replication slot on the
publisher. But both ways might not be able to skip exactly one
transaction in question and end up skipping other transactions too.

I’d like to propose a way to skip the particular transaction on the
subscriber side. As the first step, a transaction can be specified to
be skipped by specifying remote XID on the subscriber. This feature
would need two sub-features: (1) a sub-feature for users to identify
the problem subscription and the problem transaction’s XID, and (2) a
sub-feature to skip the particular transaction to apply.

For (1), I think the simplest way would be to put the details of the
change being applied in errcontext. For example, the following
errcontext shows the remote XID as well as the action name, the
relation name, and commit timestamp:

ERROR: duplicate key value violates unique constraint "test_pkey"
DETAIL: Key (c)=(1) already exists.
CONTEXT: during apply of "INSERT" for relation "public.test" in
transaction with xid 590 commit timestamp 2021-05-21
14:32:02.134273+09

In the above, the subscription name/id is not mentioned. I think you
need it for sub-feature-2.

The user can identify which remote XID has a problem during applying
the change (XID=590 in this case). As another idea, we can have a
statistics view for logical replication workers, showing information
of the last failure transaction.

It might be good to display at both places. Having subscriber-side
information in the view might be helpful in other ways as well like we
can use it to display the number of transactions processed by a
particular subscriber.

I think you need to consider few more things here:
(a) Say the error occurs after applying some part of changes, then
just skipping the remaining part won't be sufficient, we probably need
to someway rollback the applied changes (by rolling back the
transaction or in some other way).
(b) How do you handle streamed transactions? It is possible that some
of the streams are successful and the error occurs after that, say
when writing to the stream file. Now, would you skip writing to stream
file or will you write it, and then during apply, you will skip the
entire transaction and remove the corresponding stream file.
(c) There is also a possibility that the error occurs while applying
the changes of some subtransaction (this is only possible for
streaming xacts), so, in such cases, do we allow users to rollback the
subtransaction or user has to rollback the entire transaction. I am
not sure but maybe for very large transactions users might just want
to rollback the subtransaction.
(d) How about prepared transactions? Do we need to rollback the
prepared transaction if user decides to skip such a transaction? We
already allow prepared transactions to be streamed to plugins and the
work for subscriber-side apply is in progress [1]/messages/by-id/CAHut+PsDysQA=JWXb6oGFr1npvqi1e7RzzXV-juCCxnbiwHvfA@mail.gmail.com, so I think we need
to consider this case as well.
(e) Do we want to provide such a feature via output plugins as well,
if not, why?

For (2), what I'm thinking is to add a new action to ALTER
SUBSCRIPTION command like ALTER SUBSCRIPTION test_sub SET SKIP
TRANSACTION 590. Also, we can have actions to reset it; ALTER
SUBSCRIPTION test_sub RESET SKIP TRANSACTION. Those commands add the
XID to a new column of pg_subscription or a new catalog, having the
worker reread its subscription information. Once the worker skipped
the specified transaction, it resets the transaction to skip on the
catalog.

What if we fail while updating the reset information in the catalog?
Will it be the responsibility of the user to reset such a transaction
or we will retry it after restart of worker? Now, say, we give such a
responsibility to the user and the user forgets to reset it then there
is a possibility that after wraparound we will again skip the
transaction which is not intended. And, if we want to retry it after
restart of worker, how will the worker remember the previous failure?

I think this will be a useful feature but we need to consider few more things.

[1]: /messages/by-id/CAHut+PsDysQA=JWXb6oGFr1npvqi1e7RzzXV-juCCxnbiwHvfA@mail.gmail.com

--
With Regards,
Amit Kapila.

On Mon, May 24, 2021 at 1:32 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi all,

If a logical replication worker cannot apply the change on the
subscriber for some reason (e.g., missing table or violating a
constraint, etc.), logical replication stops until the problem is
resolved. Ideally, we resolve the problem on the subscriber (e.g., by
creating the missing table or removing the conflicting data, etc.) but
occasionally a problem cannot be fixed and it may be necessary to skip
the entire transaction in question. Currently, we have two ways to
skip transactions: advancing the LSN of the replication origin on the
subscriber and advancing the LSN of the replication slot on the
publisher. But both ways might not be able to skip exactly one
transaction in question and end up skipping other transactions too.

Does it mean pg_replication_origin_advance() can't skip exactly one
txn? I'm not familiar with the function or never used it though, I was
just searching for "how to skip a single txn in postgres" and ended up
in [1]https://www.postgresql.org/docs/devel/logical-replication-conflicts.html. Could you please give some more details on scenarios when we
can't skip exactly one txn? Is there any other way to advance the LSN,
something like directly updating the pg_replication_slots catalog?

[1]: https://www.postgresql.org/docs/devel/logical-replication-conflicts.html

I’d like to propose a way to skip the particular transaction on the
subscriber side. As the first step, a transaction can be specified to
be skipped by specifying remote XID on the subscriber. This feature
would need two sub-features: (1) a sub-feature for users to identify
the problem subscription and the problem transaction’s XID, and (2) a
sub-feature to skip the particular transaction to apply.

For (1), I think the simplest way would be to put the details of the
change being applied in errcontext. For example, the following
errcontext shows the remote XID as well as the action name, the
relation name, and commit timestamp:

ERROR: duplicate key value violates unique constraint "test_pkey"
DETAIL: Key (c)=(1) already exists.
CONTEXT: during apply of "INSERT" for relation "public.test" in
transaction with xid 590 commit timestamp 2021-05-21
14:32:02.134273+09

The user can identify which remote XID has a problem during applying
the change (XID=590 in this case). As another idea, we can have a
statistics view for logical replication workers, showing information
of the last failure transaction.

Agree with Amit on this. At times, it is difficult to look around in
the server logs, so it will be better to have it in both places.

For (2), what I'm thinking is to add a new action to ALTER
SUBSCRIPTION command like ALTER SUBSCRIPTION test_sub SET SKIP
TRANSACTION 590. Also, we can have actions to reset it; ALTER
SUBSCRIPTION test_sub RESET SKIP TRANSACTION. Those commands add the
XID to a new column of pg_subscription or a new catalog, having the
worker reread its subscription information. Once the worker skipped
the specified transaction, it resets the transaction to skip on the
catalog. The syntax allows users to specify one remote XID to skip. In
the future, it might be good if users can also specify multiple XIDs
(a range of XIDs or a list of XIDs, etc).

What's it like skipping a txn with txn id? Is it that the particular
txn is forced to commit or abort or just skipping some of the code in
the apply worker? IIUC, the behavior of RESET SKIP TRANSACTION is just
to forget the txn id specified in SET SKIP TRANSACTION right?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Mon, May 24, 2021 at 7:51 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, May 24, 2021 at 1:32 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

If a logical replication worker cannot apply the change on the
subscriber for some reason (e.g., missing table or violating a
constraint, etc.), logical replication stops until the problem is
resolved. Ideally, we resolve the problem on the subscriber (e.g., by
creating the missing table or removing the conflicting data, etc.) but
occasionally a problem cannot be fixed and it may be necessary to skip
the entire transaction in question. Currently, we have two ways to
skip transactions: advancing the LSN of the replication origin on the
subscriber and advancing the LSN of the replication slot on the
publisher. But both ways might not be able to skip exactly one
transaction in question and end up skipping other transactions too.

I’d like to propose a way to skip the particular transaction on the
subscriber side. As the first step, a transaction can be specified to
be skipped by specifying remote XID on the subscriber. This feature
would need two sub-features: (1) a sub-feature for users to identify
the problem subscription and the problem transaction’s XID, and (2) a
sub-feature to skip the particular transaction to apply.

For (1), I think the simplest way would be to put the details of the
change being applied in errcontext. For example, the following
errcontext shows the remote XID as well as the action name, the
relation name, and commit timestamp:

ERROR: duplicate key value violates unique constraint "test_pkey"
DETAIL: Key (c)=(1) already exists.
CONTEXT: during apply of "INSERT" for relation "public.test" in
transaction with xid 590 commit timestamp 2021-05-21
14:32:02.134273+09

In the above, the subscription name/id is not mentioned. I think you
need it for sub-feature-2.

Agreed.

The user can identify which remote XID has a problem during applying
the change (XID=590 in this case). As another idea, we can have a
statistics view for logical replication workers, showing information
of the last failure transaction.

It might be good to display at both places. Having subscriber-side
information in the view might be helpful in other ways as well like we
can use it to display the number of transactions processed by a
particular subscriber.

Yes. I think we can report that information to the stats collector. It
needs to live even after the worker exiting.

I think you need to consider few more things here:
(a) Say the error occurs after applying some part of changes, then
just skipping the remaining part won't be sufficient, we probably need
to someway rollback the applied changes (by rolling back the
transaction or in some other way).

After more thought, it might be better to that setting and resetting
the XID to skip requires disabling the subscription. This would not be
a restriction for users since logical replication is likely to already
stop (and possibly repeating restarting and stopping) due to an error.
Setting and resetting the XID modifies the system catalog so it's a
crash-safe change and survives beyond the server restarts. When a
logical replication worker starts, it checks the XID. If the worker
receives changes associated with the transaction with the specified
XID, it can ignore the entire transaction.

(b) How do you handle streamed transactions? It is possible that some
of the streams are successful and the error occurs after that, say
when writing to the stream file. Now, would you skip writing to stream
file or will you write it, and then during apply, you will skip the
entire transaction and remove the corresponding stream file.

I think streamed transactions can be handled in the same way described in (a).

(c) There is also a possibility that the error occurs while applying
the changes of some subtransaction (this is only possible for
streaming xacts), so, in such cases, do we allow users to rollback the
subtransaction or user has to rollback the entire transaction. I am
not sure but maybe for very large transactions users might just want
to rollback the subtransaction.

If the user specifies XID of a subtransaction, it would be better to
skip only the subtransaction. If specifies top transaction XID, it
would be better to skip the entire transaction. What do you think?

(d) How about prepared transactions? Do we need to rollback the
prepared transaction if user decides to skip such a transaction? We
already allow prepared transactions to be streamed to plugins and the
work for subscriber-side apply is in progress [1], so I think we need
to consider this case as well.

If a transaction replicated from the subscriber could be prepared on
the subscriber, it would be guaranteed to be able to be either
committed or rolled back. Given that this feature is to skip a problem
transaction, I think it should not do anything for transactions that
are already prepared on the subscriber.

(e) Do we want to provide such a feature via output plugins as well,
if not, why?

You mean to specify an XID to skip on the publisher side? Since I've
been considering this feature as a way to resume the logical
replication having a problem I've not thought of that idea but It
would be a good idea. Do you have any use cases? If we specified the
XID on the publisher, multiple subscribers would skip that
transaction.

For (2), what I'm thinking is to add a new action to ALTER
SUBSCRIPTION command like ALTER SUBSCRIPTION test_sub SET SKIP
TRANSACTION 590. Also, we can have actions to reset it; ALTER
SUBSCRIPTION test_sub RESET SKIP TRANSACTION. Those commands add the
XID to a new column of pg_subscription or a new catalog, having the
worker reread its subscription information. Once the worker skipped
the specified transaction, it resets the transaction to skip on the
catalog.

What if we fail while updating the reset information in the catalog?
Will it be the responsibility of the user to reset such a transaction
or we will retry it after restart of worker? Now, say, we give such a
responsibility to the user and the user forgets to reset it then there
is a possibility that after wraparound we will again skip the
transaction which is not intended. And, if we want to retry it after
restart of worker, how will the worker remember the previous failure?

As described above, setting and resetting XID to skip is implemented
as a normal system catalog change, so it's crash-safe and persisted. I
think that the worker can either removes the XID or mark it as done
once it skipped the specified transaction so that it won't skip the
same XID again after wraparound. Also, it might be better if we reset
the XID also when a subscription field such as subconninfo is changed
because it could imply the worker will connect to another publisher
having a different XID space.

We also need to handle the cases where the user specifies an old XID
or XID whose transaction is already prepared on the subscriber. I
think the worker can reset the XID with a warning when it finds out
that the XID seems no longer valid or it cannot skip the specified
XID. For example in the former case, it can do that when the first
received transaction’s XID is newer than the specified XID. In the
latter case, it can do that when it receives the commit/rollback
prepared message of the specified XID.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, May 25, 2021 at 2:49 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Mon, May 24, 2021 at 1:32 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi all,

If a logical replication worker cannot apply the change on the
subscriber for some reason (e.g., missing table or violating a
constraint, etc.), logical replication stops until the problem is
resolved. Ideally, we resolve the problem on the subscriber (e.g., by
creating the missing table or removing the conflicting data, etc.) but
occasionally a problem cannot be fixed and it may be necessary to skip
the entire transaction in question. Currently, we have two ways to
skip transactions: advancing the LSN of the replication origin on the
subscriber and advancing the LSN of the replication slot on the
publisher. But both ways might not be able to skip exactly one
transaction in question and end up skipping other transactions too.

Does it mean pg_replication_origin_advance() can't skip exactly one
txn? I'm not familiar with the function or never used it though, I was
just searching for "how to skip a single txn in postgres" and ended up
in [1]. Could you please give some more details on scenarios when we
can't skip exactly one txn? Is there any other way to advance the LSN,
something like directly updating the pg_replication_slots catalog?

Sorry, it's not impossible. Although the user mistakenly skips more
than one transaction by specifying a wrong LSN it's always possible to
skip an exact one transaction.

[1] - https://www.postgresql.org/docs/devel/logical-replication-conflicts.html

I’d like to propose a way to skip the particular transaction on the
subscriber side. As the first step, a transaction can be specified to
be skipped by specifying remote XID on the subscriber. This feature
would need two sub-features: (1) a sub-feature for users to identify
the problem subscription and the problem transaction’s XID, and (2) a
sub-feature to skip the particular transaction to apply.

For (1), I think the simplest way would be to put the details of the
change being applied in errcontext. For example, the following
errcontext shows the remote XID as well as the action name, the
relation name, and commit timestamp:

ERROR: duplicate key value violates unique constraint "test_pkey"
DETAIL: Key (c)=(1) already exists.
CONTEXT: during apply of "INSERT" for relation "public.test" in
transaction with xid 590 commit timestamp 2021-05-21
14:32:02.134273+09

The user can identify which remote XID has a problem during applying
the change (XID=590 in this case). As another idea, we can have a
statistics view for logical replication workers, showing information
of the last failure transaction.

Agree with Amit on this. At times, it is difficult to look around in
the server logs, so it will be better to have it in both places.

For (2), what I'm thinking is to add a new action to ALTER
SUBSCRIPTION command like ALTER SUBSCRIPTION test_sub SET SKIP
TRANSACTION 590. Also, we can have actions to reset it; ALTER
SUBSCRIPTION test_sub RESET SKIP TRANSACTION. Those commands add the
XID to a new column of pg_subscription or a new catalog, having the
worker reread its subscription information. Once the worker skipped
the specified transaction, it resets the transaction to skip on the
catalog. The syntax allows users to specify one remote XID to skip. In
the future, it might be good if users can also specify multiple XIDs
(a range of XIDs or a list of XIDs, etc).

What's it like skipping a txn with txn id? Is it that the particular
txn is forced to commit or abort or just skipping some of the code in
the apply worker?

What I'm thinking is to ignore the entire transaction with the
specified XID. IOW Logical replication workers don't even start the
transaction and ignore all changes associated with the XID.

IIUC, the behavior of RESET SKIP TRANSACTION is just
to forget the txn id specified in SET SKIP TRANSACTION right?

Right. I proposed this RESET command for users to cancel the skipping behavior.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, May 25, 2021 at 1:44 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, May 25, 2021 at 2:49 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Mon, May 24, 2021 at 1:32 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi all,

If a logical replication worker cannot apply the change on the
subscriber for some reason (e.g., missing table or violating a
constraint, etc.), logical replication stops until the problem is
resolved. Ideally, we resolve the problem on the subscriber (e.g., by
creating the missing table or removing the conflicting data, etc.) but
occasionally a problem cannot be fixed and it may be necessary to skip
the entire transaction in question. Currently, we have two ways to
skip transactions: advancing the LSN of the replication origin on the
subscriber and advancing the LSN of the replication slot on the
publisher. But both ways might not be able to skip exactly one
transaction in question and end up skipping other transactions too.

Does it mean pg_replication_origin_advance() can't skip exactly one
txn? I'm not familiar with the function or never used it though, I was
just searching for "how to skip a single txn in postgres" and ended up
in [1]. Could you please give some more details on scenarios when we
can't skip exactly one txn? Is there any other way to advance the LSN,
something like directly updating the pg_replication_slots catalog?

Sorry, it's not impossible. Although the user mistakenly skips more
than one transaction by specifying a wrong LSN it's always possible to
skip an exact one transaction.

IIUC, if the user specifies the "correct LSN", then it's possible to
skip exact txn for which the sync workers are unable to apply changes,
right?

How can the user get the LSN (which we call "correct LSN")? Is it from
pg_replication_slots? Or some other way?

If the user somehow can get the "correct LSN", can't the exact txn be
skipped using it with any of the existing ways, either using
pg_replication_origin_advance or any other ways?

If there's no way to get the "correct LSN", then why can't we just
print that LSN in the error context and/or in the new statistics view
for logical replication workers, so that any of the existing ways can
be used to skip exactly one txn?

IIUC, the feature proposed here guards against the users specifying
wrong LSN. If I'm right, what is the guarantee that users don't
specify the wrong txn id? Why can't we tell the users when a wrong LSN
is specified that "currently, an apply worker is failing to apply the
LSN XXXX, and you specified LSN YYYY, are you sure this is
intentional?"

Please correct me if I'm missing anything.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com

On Tue, May 25, 2021 at 7:21 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Tue, May 25, 2021 at 1:44 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, May 25, 2021 at 2:49 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Mon, May 24, 2021 at 1:32 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Hi all,

If a logical replication worker cannot apply the change on the
subscriber for some reason (e.g., missing table or violating a
constraint, etc.), logical replication stops until the problem is
resolved. Ideally, we resolve the problem on the subscriber (e.g., by
creating the missing table or removing the conflicting data, etc.) but
occasionally a problem cannot be fixed and it may be necessary to skip
the entire transaction in question. Currently, we have two ways to
skip transactions: advancing the LSN of the replication origin on the
subscriber and advancing the LSN of the replication slot on the
publisher. But both ways might not be able to skip exactly one
transaction in question and end up skipping other transactions too.

Does it mean pg_replication_origin_advance() can't skip exactly one
txn? I'm not familiar with the function or never used it though, I was
just searching for "how to skip a single txn in postgres" and ended up
in [1]. Could you please give some more details on scenarios when we
can't skip exactly one txn? Is there any other way to advance the LSN,
something like directly updating the pg_replication_slots catalog?

Sorry, it's not impossible. Although the user mistakenly skips more
than one transaction by specifying a wrong LSN it's always possible to
skip an exact one transaction.

IIUC, if the user specifies the "correct LSN", then it's possible to
skip exact txn for which the sync workers are unable to apply changes,
right?

How can the user get the LSN (which we call "correct LSN")? Is it from
pg_replication_slots? Or some other way?

If the user somehow can get the "correct LSN", can't the exact txn be
skipped using it with any of the existing ways, either using
pg_replication_origin_advance or any other ways?

One possible way I know is to copy the logical replication slot used
by the subscriber and peek at the changes to identify the correct LSN
(maybe there is another handy way though) . For example, suppose that
two transactions insert tuples as follows on the publisher:

TX-A: BEGIN;
TX-A: INSERT INTO test VALUES (1);
TX-B: BEGIN;
TX-B: INSERT INTO test VALUES (10);
TX-B: COMMIT;
TX-A: INSERT INTO test VALUES (2);
TX-A: COMMIT;

And suppose further that the insertion with value = 10 (by TX-A)
cannot be applied only on the subscriber due to unique constraint
violation. If we copy the slot by
pg_copy_logical_replication_slot('test_sub', 'copy_slot', true,
'test_decoding') , we can peek at those changes with LSN as follows:

=# select * from pg_logical_slot_peek_changes('copy', null, null) order by lsn;
lsn | xid | data
-----------+-----+------------------------------------------
0/1911548 | 736 | BEGIN 736
0/1911548 | 736 | table public.hoge: INSERT: c[integer]:1
0/1911588 | 737 | BEGIN 737
0/1911588 | 737 | table public.hoge: INSERT: c[integer]:10
0/19115F8 | 737 | COMMIT 737
0/1911630 | 736 | table public.hoge: INSERT: c[integer]:2
0/19116A0 | 736 | COMMIT 736
(7 rows)

In this case, '0/19115F8' is the correct LSN to specify. We can
advance the replication origin to ' 0/19115F8' by
pg_replication_origin_advance() so that logical replication streams
transactions committed after ' 0/19115F8'. After the logical
replication restarting, it skips the transaction with xid = 737 but
replicates the transaction with xid = 736.

If there's no way to get the "correct LSN", then why can't we just
print that LSN in the error context and/or in the new statistics view
for logical replication workers, so that any of the existing ways can
be used to skip exactly one txn?

I think specifying XID to the subscription is more understandable for users.

IIUC, the feature proposed here guards against the users specifying
wrong LSN. If I'm right, what is the guarantee that users don't
specify the wrong txn id? Why can't we tell the users when a wrong LSN
is specified that "currently, an apply worker is failing to apply the
LSN XXXX, and you specified LSN YYYY, are you sure this is
intentional?"

With the initial idea, specifying the correct XID is the user's
responsibility. If they specify an old XID, the worker invalids it and
raises a warning to tell "the worker invalidated the specified XID as
it's too old". As the second idea, if we store the last failed XID
somewhere (e.g., a system catalog), the user can just specify to skip
that transaction. That is, instead of specifying the XID they could do
something like "ALTER SUBSCRIPTION test_sub RESOLVE CONFLICT BY SKIP".

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, May 25, 2021 at 12:26 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, May 24, 2021 at 7:51 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, May 24, 2021 at 1:32 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I think you need to consider few more things here:
(a) Say the error occurs after applying some part of changes, then
just skipping the remaining part won't be sufficient, we probably need
to someway rollback the applied changes (by rolling back the
transaction or in some other way).

After more thought, it might be better to that setting and resetting
the XID to skip requires disabling the subscription.

It might be better if it doesn't require disabling the subscription
because it would be more steps for the user to disable/enable it. It
is not clear to me what exactly you want to gain by disabling the
subscription in this case.

This would not be
a restriction for users since logical replication is likely to already
stop (and possibly repeating restarting and stopping) due to an error.
Setting and resetting the XID modifies the system catalog so it's a
crash-safe change and survives beyond the server restarts. When a
logical replication worker starts, it checks the XID. If the worker
receives changes associated with the transaction with the specified
XID, it can ignore the entire transaction.

(b) How do you handle streamed transactions? It is possible that some
of the streams are successful and the error occurs after that, say
when writing to the stream file. Now, would you skip writing to stream
file or will you write it, and then during apply, you will skip the
entire transaction and remove the corresponding stream file.

I think streamed transactions can be handled in the same way described in (a).

(c) There is also a possibility that the error occurs while applying
the changes of some subtransaction (this is only possible for
streaming xacts), so, in such cases, do we allow users to rollback the
subtransaction or user has to rollback the entire transaction. I am
not sure but maybe for very large transactions users might just want
to rollback the subtransaction.

If the user specifies XID of a subtransaction, it would be better to
skip only the subtransaction. If specifies top transaction XID, it
would be better to skip the entire transaction. What do you think?

makes sense.

(d) How about prepared transactions? Do we need to rollback the
prepared transaction if user decides to skip such a transaction? We
already allow prepared transactions to be streamed to plugins and the
work for subscriber-side apply is in progress [1], so I think we need
to consider this case as well.

If a transaction replicated from the subscriber could be prepared on
the subscriber, it would be guaranteed to be able to be either
committed or rolled back. Given that this feature is to skip a problem
transaction, I think it should not do anything for transactions that
are already prepared on the subscriber.

makes sense, but I think we need to reset the XID in such a case.

(e) Do we want to provide such a feature via output plugins as well,
if not, why?

You mean to specify an XID to skip on the publisher side? Since I've
been considering this feature as a way to resume the logical
replication having a problem I've not thought of that idea but It
would be a good idea. Do you have any use cases?

No. On again thinking about this, I think we can leave this for now.

If we specified the
XID on the publisher, multiple subscribers would skip that
transaction.

For (2), what I'm thinking is to add a new action to ALTER
SUBSCRIPTION command like ALTER SUBSCRIPTION test_sub SET SKIP
TRANSACTION 590. Also, we can have actions to reset it; ALTER
SUBSCRIPTION test_sub RESET SKIP TRANSACTION. Those commands add the
XID to a new column of pg_subscription or a new catalog, having the
worker reread its subscription information. Once the worker skipped
the specified transaction, it resets the transaction to skip on the
catalog.

What if we fail while updating the reset information in the catalog?
Will it be the responsibility of the user to reset such a transaction
or we will retry it after restart of worker? Now, say, we give such a
responsibility to the user and the user forgets to reset it then there
is a possibility that after wraparound we will again skip the
transaction which is not intended. And, if we want to retry it after
restart of worker, how will the worker remember the previous failure?

As described above, setting and resetting XID to skip is implemented
as a normal system catalog change, so it's crash-safe and persisted. I
think that the worker can either removes the XID or mark it as done
once it skipped the specified transaction so that it won't skip the
same XID again after wraparound.

It all depends on when exactly you want to update the catalog
information. Say after skipping commit of the XID, we do update the
corresponding LSN to be communicated as already processed to the
subscriber and then get the error while updating the catalog
information then next time we might not know whether to update the
catalog for skipped XID.

Also, it might be better if we reset
the XID also when a subscription field such as subconninfo is changed
because it could imply the worker will connect to another publisher
having a different XID space.

We also need to handle the cases where the user specifies an old XID
or XID whose transaction is already prepared on the subscriber. I
think the worker can reset the XID with a warning when it finds out
that the XID seems no longer valid or it cannot skip the specified
XID. For example in the former case, it can do that when the first
received transaction’s XID is newer than the specified XID.

But how can we guarantee that older XID can't be received later? Is
there a guarantee that we receive the transactions on subscriber in
XID order.

--
With Regards,
Amit Kapila.

On Tue, May 25, 2021 at 6:12 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, May 25, 2021 at 7:21 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

If there's no way to get the "correct LSN", then why can't we just
print that LSN in the error context and/or in the new statistics view
for logical replication workers, so that any of the existing ways can
be used to skip exactly one txn?

I think specifying XID to the subscription is more understandable for users.

I agree with you that specifying XID could be easier and
understandable for users. I was thinking and studying a bit about what
other systems do in this regard. Why don't we try to provide conflict
resolution methods for users? The idea could be that either the
conflicts can be resolved automatically or manually. In the case of
manual resolution, users can use the existing methods or the XID stuff
you are proposing here and in case of automatic resolution, the
in-built or corresponding user-defined functions will be invoked for
conflict resolution. There are more details to figure out in the
automatic resolution scheme but I see a lot of value in doing the
same.

--
With Regards,
Amit Kapila.

On Wed, May 26, 2021 at 3:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, May 25, 2021 at 12:26 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, May 24, 2021 at 7:51 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, May 24, 2021 at 1:32 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I think you need to consider few more things here:
(a) Say the error occurs after applying some part of changes, then
just skipping the remaining part won't be sufficient, we probably need
to someway rollback the applied changes (by rolling back the
transaction or in some other way).

After more thought, it might be better to that setting and resetting
the XID to skip requires disabling the subscription.

It might be better if it doesn't require disabling the subscription
because it would be more steps for the user to disable/enable it. It
is not clear to me what exactly you want to gain by disabling the
subscription in this case.

The situation I’m considered is where the user specifies the XID while
the worker is applying the changes of the transaction with that XID.
In this case, I think we need to somehow rollback the changes applied
so far. Perhaps we can either rollback the transaction and ignore the
remaining changes or restart and ignore the entire transaction from
the beginning. Also, we need to handle the case where the user resets
the XID after the worker skips to write some stream files. I thought
those parts could be complicated but it might be not after more
thought.

This would not be
a restriction for users since logical replication is likely to already
stop (and possibly repeating restarting and stopping) due to an error.
Setting and resetting the XID modifies the system catalog so it's a
crash-safe change and survives beyond the server restarts. When a
logical replication worker starts, it checks the XID. If the worker
receives changes associated with the transaction with the specified
XID, it can ignore the entire transaction.

(b) How do you handle streamed transactions? It is possible that some
of the streams are successful and the error occurs after that, say
when writing to the stream file. Now, would you skip writing to stream
file or will you write it, and then during apply, you will skip the
entire transaction and remove the corresponding stream file.

I think streamed transactions can be handled in the same way described in (a).

If setting and resetting the XID can be performed during the worker
running, we would need to write stream files even if we’re receiving
changes that are associated with the specified XID. Since it could
happen that the user resets the XID after we processed some of the
streamed changes, we would need to decide whether or to skip the
transaction when starting to apply changes.

(c) There is also a possibility that the error occurs while applying
the changes of some subtransaction (this is only possible for
streaming xacts), so, in such cases, do we allow users to rollback the
subtransaction or user has to rollback the entire transaction. I am
not sure but maybe for very large transactions users might just want
to rollback the subtransaction.

If the user specifies XID of a subtransaction, it would be better to
skip only the subtransaction. If specifies top transaction XID, it
would be better to skip the entire transaction. What do you think?

makes sense.

(d) How about prepared transactions? Do we need to rollback the
prepared transaction if user decides to skip such a transaction? We
already allow prepared transactions to be streamed to plugins and the
work for subscriber-side apply is in progress [1], so I think we need
to consider this case as well.

If a transaction replicated from the subscriber could be prepared on
the subscriber, it would be guaranteed to be able to be either
committed or rolled back. Given that this feature is to skip a problem
transaction, I think it should not do anything for transactions that
are already prepared on the subscriber.

makes sense, but I think we need to reset the XID in such a case.

Agreed.

(e) Do we want to provide such a feature via output plugins as well,
if not, why?

You mean to specify an XID to skip on the publisher side? Since I've
been considering this feature as a way to resume the logical
replication having a problem I've not thought of that idea but It
would be a good idea. Do you have any use cases?

No. On again thinking about this, I think we can leave this for now.

If we specified the
XID on the publisher, multiple subscribers would skip that
transaction.

For (2), what I'm thinking is to add a new action to ALTER
SUBSCRIPTION command like ALTER SUBSCRIPTION test_sub SET SKIP
TRANSACTION 590. Also, we can have actions to reset it; ALTER
SUBSCRIPTION test_sub RESET SKIP TRANSACTION. Those commands add the
XID to a new column of pg_subscription or a new catalog, having the
worker reread its subscription information. Once the worker skipped
the specified transaction, it resets the transaction to skip on the
catalog.

What if we fail while updating the reset information in the catalog?
Will it be the responsibility of the user to reset such a transaction
or we will retry it after restart of worker? Now, say, we give such a
responsibility to the user and the user forgets to reset it then there
is a possibility that after wraparound we will again skip the
transaction which is not intended. And, if we want to retry it after
restart of worker, how will the worker remember the previous failure?

As described above, setting and resetting XID to skip is implemented
as a normal system catalog change, so it's crash-safe and persisted. I
think that the worker can either removes the XID or mark it as done
once it skipped the specified transaction so that it won't skip the
same XID again after wraparound.

It all depends on when exactly you want to update the catalog
information. Say after skipping commit of the XID, we do update the
corresponding LSN to be communicated as already processed to the
subscriber and then get the error while updating the catalog
information then next time we might not know whether to update the
catalog for skipped XID.

Also, it might be better if we reset
the XID also when a subscription field such as subconninfo is changed
because it could imply the worker will connect to another publisher
having a different XID space.

We also need to handle the cases where the user specifies an old XID
or XID whose transaction is already prepared on the subscriber. I
think the worker can reset the XID with a warning when it finds out
that the XID seems no longer valid or it cannot skip the specified
XID. For example in the former case, it can do that when the first
received transaction’s XID is newer than the specified XID.

But how can we guarantee that older XID can't be received later? Is
there a guarantee that we receive the transactions on subscriber in
XID order.

Considering the above two comments, it might be better to provide a
way to skip the transaction that is already known to be conflicted
rather than allowing users to specify the arbitrary XID.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Thu, May 27, 2021 at 9:56 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, May 26, 2021 at 3:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, May 25, 2021 at 12:26 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, May 24, 2021 at 7:51 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, May 24, 2021 at 1:32 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I think you need to consider few more things here:
(a) Say the error occurs after applying some part of changes, then
just skipping the remaining part won't be sufficient, we probably need
to someway rollback the applied changes (by rolling back the
transaction or in some other way).

After more thought, it might be better to that setting and resetting
the XID to skip requires disabling the subscription.

It might be better if it doesn't require disabling the subscription
because it would be more steps for the user to disable/enable it. It
is not clear to me what exactly you want to gain by disabling the
subscription in this case.

The situation I’m considered is where the user specifies the XID while
the worker is applying the changes of the transaction with that XID.
In this case, I think we need to somehow rollback the changes applied
so far. Perhaps we can either rollback the transaction and ignore the
remaining changes or restart and ignore the entire transaction from
the beginning.

If we follow your suggestion of only allowing XIDs that have been
known to have conflicts then probably we don't need to worry about
rollbacks.

For (2), what I'm thinking is to add a new action to ALTER
SUBSCRIPTION command like ALTER SUBSCRIPTION test_sub SET SKIP
TRANSACTION 590. Also, we can have actions to reset it; ALTER
SUBSCRIPTION test_sub RESET SKIP TRANSACTION. Those commands add the
XID to a new column of pg_subscription or a new catalog, having the
worker reread its subscription information. Once the worker skipped
the specified transaction, it resets the transaction to skip on the
catalog.

What if we fail while updating the reset information in the catalog?
Will it be the responsibility of the user to reset such a transaction
or we will retry it after restart of worker? Now, say, we give such a
responsibility to the user and the user forgets to reset it then there
is a possibility that after wraparound we will again skip the
transaction which is not intended. And, if we want to retry it after
restart of worker, how will the worker remember the previous failure?

As described above, setting and resetting XID to skip is implemented
as a normal system catalog change, so it's crash-safe and persisted. I
think that the worker can either removes the XID or mark it as done
once it skipped the specified transaction so that it won't skip the
same XID again after wraparound.

It all depends on when exactly you want to update the catalog
information. Say after skipping commit of the XID, we do update the
corresponding LSN to be communicated as already processed to the
subscriber and then get the error while updating the catalog
information then next time we might not know whether to update the
catalog for skipped XID.

Also, it might be better if we reset
the XID also when a subscription field such as subconninfo is changed
because it could imply the worker will connect to another publisher
having a different XID space.

We also need to handle the cases where the user specifies an old XID
or XID whose transaction is already prepared on the subscriber. I
think the worker can reset the XID with a warning when it finds out
that the XID seems no longer valid or it cannot skip the specified
XID. For example in the former case, it can do that when the first
received transaction’s XID is newer than the specified XID.

But how can we guarantee that older XID can't be received later? Is
there a guarantee that we receive the transactions on subscriber in
XID order.

Considering the above two comments, it might be better to provide a
way to skip the transaction that is already known to be conflicted
rather than allowing users to specify the arbitrary XID.

Okay, that makes sense but still not sure how will you identify if we
need to reset XID in case of failure doing that in the previous
attempt. Also, I am thinking that instead of a stat view, do we need
to consider having a system table (pg_replication_conflicts or
something like that) for this because what if stats information is
lost (say either due to crash or due to udp packet loss), can we rely
on stats view for this?

--
With Regards,
Amit Kapila.

On Wed, May 26, 2021 at 6:11 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, May 25, 2021 at 6:12 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, May 25, 2021 at 7:21 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

If there's no way to get the "correct LSN", then why can't we just
print that LSN in the error context and/or in the new statistics view
for logical replication workers, so that any of the existing ways can
be used to skip exactly one txn?

I think specifying XID to the subscription is more understandable for users.

I agree with you that specifying XID could be easier and
understandable for users. I was thinking and studying a bit about what
other systems do in this regard. Why don't we try to provide conflict
resolution methods for users? The idea could be that either the
conflicts can be resolved automatically or manually. In the case of
manual resolution, users can use the existing methods or the XID stuff
you are proposing here and in case of automatic resolution, the
in-built or corresponding user-defined functions will be invoked for
conflict resolution. There are more details to figure out in the
automatic resolution scheme but I see a lot of value in doing the
same.

Yeah, I also see a lot of value in automatic conflict resolution. But
maybe we can have both ways? For example, in case where the user wants
to resolve conflicts in different ways or a conflict that cannot be
resolved by automatic resolution (not sure there is in practice
though), the manual resolution would also have value.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Thu, May 27, 2021 at 2:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, May 27, 2021 at 9:56 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, May 26, 2021 at 3:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, May 25, 2021 at 12:26 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, May 24, 2021 at 7:51 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, May 24, 2021 at 1:32 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I think you need to consider few more things here:
(a) Say the error occurs after applying some part of changes, then
just skipping the remaining part won't be sufficient, we probably need
to someway rollback the applied changes (by rolling back the
transaction or in some other way).

After more thought, it might be better to that setting and resetting
the XID to skip requires disabling the subscription.

It might be better if it doesn't require disabling the subscription
because it would be more steps for the user to disable/enable it. It
is not clear to me what exactly you want to gain by disabling the
subscription in this case.

The situation I’m considered is where the user specifies the XID while
the worker is applying the changes of the transaction with that XID.
In this case, I think we need to somehow rollback the changes applied
so far. Perhaps we can either rollback the transaction and ignore the
remaining changes or restart and ignore the entire transaction from
the beginning.

If we follow your suggestion of only allowing XIDs that have been
known to have conflicts then probably we don't need to worry about
rollbacks.

For (2), what I'm thinking is to add a new action to ALTER
SUBSCRIPTION command like ALTER SUBSCRIPTION test_sub SET SKIP
TRANSACTION 590. Also, we can have actions to reset it; ALTER
SUBSCRIPTION test_sub RESET SKIP TRANSACTION. Those commands add the
XID to a new column of pg_subscription or a new catalog, having the
worker reread its subscription information. Once the worker skipped
the specified transaction, it resets the transaction to skip on the
catalog.

What if we fail while updating the reset information in the catalog?
Will it be the responsibility of the user to reset such a transaction
or we will retry it after restart of worker? Now, say, we give such a
responsibility to the user and the user forgets to reset it then there
is a possibility that after wraparound we will again skip the
transaction which is not intended. And, if we want to retry it after
restart of worker, how will the worker remember the previous failure?

As described above, setting and resetting XID to skip is implemented
as a normal system catalog change, so it's crash-safe and persisted. I
think that the worker can either removes the XID or mark it as done
once it skipped the specified transaction so that it won't skip the
same XID again after wraparound.

It all depends on when exactly you want to update the catalog
information. Say after skipping commit of the XID, we do update the
corresponding LSN to be communicated as already processed to the
subscriber and then get the error while updating the catalog
information then next time we might not know whether to update the
catalog for skipped XID.

Also, it might be better if we reset
the XID also when a subscription field such as subconninfo is changed
because it could imply the worker will connect to another publisher
having a different XID space.

We also need to handle the cases where the user specifies an old XID
or XID whose transaction is already prepared on the subscriber. I
think the worker can reset the XID with a warning when it finds out
that the XID seems no longer valid or it cannot skip the specified
XID. For example in the former case, it can do that when the first
received transaction’s XID is newer than the specified XID.

But how can we guarantee that older XID can't be received later? Is
there a guarantee that we receive the transactions on subscriber in
XID order.

Considering the above two comments, it might be better to provide a
way to skip the transaction that is already known to be conflicted
rather than allowing users to specify the arbitrary XID.

Okay, that makes sense but still not sure how will you identify if we
need to reset XID in case of failure doing that in the previous
attempt.

It's a just idea but we can record the failed transaction with XID as
well as its commit LSN passed? The sequence I'm thinking is,

1. the worker records the XID and commit LSN of the failed transaction
to a catalog.
2. the user specifies how to resolve that conflict transaction
(currently only 'skip' is supported) and writes to the catalog.
3. the worker does the resolution method according to the catalog. If
the worker didn't start to apply those changes, it can skip the entire
transaction. If did, it rollbacks the transaction and ignores the
remaining.

The worker needs neither to reset information of the last failed
transaction nor to mark the conflicted transaction as resolved. The
worker will ignore that information when checking the catalog if the
commit LSN is passed.

Also, I am thinking that instead of a stat view, do we need
to consider having a system table (pg_replication_conflicts or
something like that) for this because what if stats information is
lost (say either due to crash or due to udp packet loss), can we rely
on stats view for this?

Yeah, it seems better to use a catalog.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Thu, May 27, 2021 at 1:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, May 27, 2021 at 2:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

Okay, that makes sense but still not sure how will you identify if we
need to reset XID in case of failure doing that in the previous
attempt.

It's a just idea but we can record the failed transaction with XID as
well as its commit LSN passed? The sequence I'm thinking is,

1. the worker records the XID and commit LSN of the failed transaction
to a catalog.

When will you record this info? I am not sure if we can try to update
this when an error has occurred. We can think of using try..catch in
apply worker and then record it in catch on error but would that be
advisable? One random thought that occurred to me is to that apply
worker notifies such information to the launcher (or maybe another
process) which will log this information.

2. the user specifies how to resolve that conflict transaction
(currently only 'skip' is supported) and writes to the catalog.
3. the worker does the resolution method according to the catalog. If
the worker didn't start to apply those changes, it can skip the entire
transaction. If did, it rollbacks the transaction and ignores the
remaining.

The worker needs neither to reset information of the last failed
transaction nor to mark the conflicted transaction as resolved. The
worker will ignore that information when checking the catalog if the
commit LSN is passed.

So won't this require us to check the required info in the catalog
before applying each transaction? If so, that might be overhead, maybe
we can build some cache of the highest commitLSN that can be consulted
rather than the catalog table. I think we need to think about when to
remove rows for which conflict has been resolved as we can't let that
information grow infinitely.

Also, I am thinking that instead of a stat view, do we need
to consider having a system table (pg_replication_conflicts or
something like that) for this because what if stats information is
lost (say either due to crash or due to udp packet loss), can we rely
on stats view for this?

Yeah, it seems better to use a catalog.

Okay.

--
With Regards,
Amit Kapila.

On Thu, May 27, 2021 at 12:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, May 26, 2021 at 6:11 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

I agree with you that specifying XID could be easier and
understandable for users. I was thinking and studying a bit about what
other systems do in this regard. Why don't we try to provide conflict
resolution methods for users? The idea could be that either the
conflicts can be resolved automatically or manually. In the case of
manual resolution, users can use the existing methods or the XID stuff
you are proposing here and in case of automatic resolution, the
in-built or corresponding user-defined functions will be invoked for
conflict resolution. There are more details to figure out in the
automatic resolution scheme but I see a lot of value in doing the
same.

Yeah, I also see a lot of value in automatic conflict resolution. But
maybe we can have both ways? For example, in case where the user wants
to resolve conflicts in different ways or a conflict that cannot be
resolved by automatic resolution (not sure there is in practice
though), the manual resolution would also have value.

Right, that is exactly what I was saying. So, even if both can be done
as separate patches, we should try to design the manual resolution in
a way that can be extended for an automatic resolution system. I think
we can try to have some initial idea/design/POC for an automatic
resolution as well to ensure that the manual resolution scheme can be
further extended.

--
With Regards,
Amit Kapila.

On Thu, May 27, 2021 at 7:26 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, May 27, 2021 at 12:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, May 26, 2021 at 6:11 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

I agree with you that specifying XID could be easier and
understandable for users. I was thinking and studying a bit about what
other systems do in this regard. Why don't we try to provide conflict
resolution methods for users? The idea could be that either the
conflicts can be resolved automatically or manually. In the case of
manual resolution, users can use the existing methods or the XID stuff
you are proposing here and in case of automatic resolution, the
in-built or corresponding user-defined functions will be invoked for
conflict resolution. There are more details to figure out in the
automatic resolution scheme but I see a lot of value in doing the
same.

Yeah, I also see a lot of value in automatic conflict resolution. But
maybe we can have both ways? For example, in case where the user wants
to resolve conflicts in different ways or a conflict that cannot be
resolved by automatic resolution (not sure there is in practice
though), the manual resolution would also have value.

Right, that is exactly what I was saying. So, even if both can be done
as separate patches, we should try to design the manual resolution in
a way that can be extended for an automatic resolution system. I think
we can try to have some initial idea/design/POC for an automatic
resolution as well to ensure that the manual resolution scheme can be
further extended.

Totally agreed.

But perhaps we might want to note that the conflict resolution we're
talking about is to resolve conflicts at the row or column level. It
doesn't necessarily raise an ERROR and the granularity of resolution
is per record or column. For example, if a DELETE and an UPDATE
process the same tuple (searched by PK), the UPDATE may not find the
tuple and be ignored due to the tuple having been already deleted. In
this case, no ERROR will occur (i.g. UPDATE will be ignored), but the
user may want to do another conflict resolution. On the other hand,
the feature proposed here assumes that an error has already occurred
and logical replication has already been stopped. And resolves it by
skipping the entire transaction.

IIUC the conflict resolution can be thought of as a combination of
types of conflicts and the resolution that can be applied to them. For
example, if there is a conflict between INSERT and INSERT and the
latter INSERT violates the unique constraint, an ERROR is raised. So
DBA can resolve it manually. But there is another way to automatically
resolve it by selecting the tuple having a newer timestamp. On the
other hand, in the DELETE and UPDATE conflict described above, it's
possible to automatically ignore the fact that the UPDATE could update
the tuple. Or we can even generate an ERROR so that DBA can resolve it
manually. DBA can manually resolve the conflict in various ways:
skipping the entire transaction from the origin, choose the tuple
having a newer/older timestamp, etc.

In that sense, we can think of the feature proposed here as a feature
that provides a way to resolve the conflict that would originally
cause an ERROR by skipping the entire transaction. If we add a
solution that raises an ERROR for conflicts that don't originally
raise an ERROR (like DELETE and UPDATE conflict) in the future, we
will be able to manually skip each transaction for all types of
conflicts.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Thu, May 27, 2021 at 7:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, May 27, 2021 at 1:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, May 27, 2021 at 2:48 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

Okay, that makes sense but still not sure how will you identify if we
need to reset XID in case of failure doing that in the previous
attempt.

It's a just idea but we can record the failed transaction with XID as
well as its commit LSN passed? The sequence I'm thinking is,

1. the worker records the XID and commit LSN of the failed transaction
to a catalog.

When will you record this info? I am not sure if we can try to update
this when an error has occurred. We can think of using try..catch in
apply worker and then record it in catch on error but would that be
advisable? One random thought that occurred to me is to that apply
worker notifies such information to the launcher (or maybe another
process) which will log this information.

Yeah, I was concerned about that too and had the same idea. The
information still could not be written if the server crashes before
the launcher writes it. But I think it's an acceptable.

2. the user specifies how to resolve that conflict transaction
(currently only 'skip' is supported) and writes to the catalog.
3. the worker does the resolution method according to the catalog. If
the worker didn't start to apply those changes, it can skip the entire
transaction. If did, it rollbacks the transaction and ignores the
remaining.

The worker needs neither to reset information of the last failed
transaction nor to mark the conflicted transaction as resolved. The
worker will ignore that information when checking the catalog if the
commit LSN is passed.

So won't this require us to check the required info in the catalog
before applying each transaction? If so, that might be overhead, maybe
we can build some cache of the highest commitLSN that can be consulted
rather than the catalog table.

I think workers can cache that information when starts and invalidates
and reload the cache when the catalog gets updated. Specifying to
skip XID will update the catalog, invalidating the cache.

I think we need to think about when to
remove rows for which conflict has been resolved as we can't let that
information grow infinitely.

I guess we can update catalog tuples in place when another conflict
happens next time. The catalog tuple should be fixed size. The
already-resolved conflict will have the commit LSN older than its
replication origin's LSN.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Sat, May 29, 2021 at 8:27 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, May 27, 2021 at 7:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, May 27, 2021 at 1:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

1. the worker records the XID and commit LSN of the failed transaction
to a catalog.

When will you record this info? I am not sure if we can try to update
this when an error has occurred. We can think of using try..catch in
apply worker and then record it in catch on error but would that be
advisable? One random thought that occurred to me is to that apply
worker notifies such information to the launcher (or maybe another
process) which will log this information.

Yeah, I was concerned about that too and had the same idea. The
information still could not be written if the server crashes before
the launcher writes it. But I think it's an acceptable.

True, because even if the launcher restarts, the apply worker will
error out again and resend the information. I guess we can have an
error queue where apply workers can add their information and the
launcher will then process those. If we do that, then we need to
probably define what we want to do if the queue gets full, either
apply worker nudge launcher and wait or it can just throw an error and
continue. If you have any better ideas to share this information then
we can consider those as well.

2. the user specifies how to resolve that conflict transaction
(currently only 'skip' is supported) and writes to the catalog.
3. the worker does the resolution method according to the catalog. If
the worker didn't start to apply those changes, it can skip the entire
transaction. If did, it rollbacks the transaction and ignores the
remaining.

The worker needs neither to reset information of the last failed
transaction nor to mark the conflicted transaction as resolved. The
worker will ignore that information when checking the catalog if the
commit LSN is passed.

So won't this require us to check the required info in the catalog
before applying each transaction? If so, that might be overhead, maybe
we can build some cache of the highest commitLSN that can be consulted
rather than the catalog table.

I think workers can cache that information when starts and invalidates
and reload the cache when the catalog gets updated. Specifying to
skip XID will update the catalog, invalidating the cache.

I think we need to think about when to
remove rows for which conflict has been resolved as we can't let that
information grow infinitely.

I guess we can update catalog tuples in place when another conflict
happens next time. The catalog tuple should be fixed size. The
already-resolved conflict will have the commit LSN older than its
replication origin's LSN.

Okay, but I have a slight concern that we will keep xid in the system
which might have been no longer valid. So, we will keep this info
about subscribers around till one performs drop subscription,
hopefully, that doesn't lead to too many rows. This will be okay as
per the current design but say tomorrow we decide to parallelize the
apply for a subscription then there could be multiple errors
corresponding to a subscription and in that case, such a design might
appear quite limiting. One possibility could be that when the launcher
is periodically checking for new error messages, it can clean up the
conflicts catalog as well, or maybe autovacuum does this periodically
as it does for stats (via pgstat_vacuum_stat).

--
With Regards,
Amit Kapila.

On Sat, May 29, 2021 at 3:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Sat, May 29, 2021 at 8:27 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, May 27, 2021 at 7:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, May 27, 2021 at 1:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

1. the worker records the XID and commit LSN of the failed transaction
to a catalog.

When will you record this info? I am not sure if we can try to update
this when an error has occurred. We can think of using try..catch in
apply worker and then record it in catch on error but would that be
advisable? One random thought that occurred to me is to that apply
worker notifies such information to the launcher (or maybe another
process) which will log this information.

Yeah, I was concerned about that too and had the same idea. The
information still could not be written if the server crashes before
the launcher writes it. But I think it's an acceptable.

True, because even if the launcher restarts, the apply worker will
error out again and resend the information. I guess we can have an
error queue where apply workers can add their information and the
launcher will then process those. If we do that, then we need to
probably define what we want to do if the queue gets full, either
apply worker nudge launcher and wait or it can just throw an error and
continue. If you have any better ideas to share this information then
we can consider those as well.

+1 for using error queue. Maybe we need to avoid queuing the same
error more than once to avoid the catalog from being updated
frequently?

2. the user specifies how to resolve that conflict transaction
(currently only 'skip' is supported) and writes to the catalog.
3. the worker does the resolution method according to the catalog. If
the worker didn't start to apply those changes, it can skip the entire
transaction. If did, it rollbacks the transaction and ignores the
remaining.

The worker needs neither to reset information of the last failed
transaction nor to mark the conflicted transaction as resolved. The
worker will ignore that information when checking the catalog if the
commit LSN is passed.

So won't this require us to check the required info in the catalog
before applying each transaction? If so, that might be overhead, maybe
we can build some cache of the highest commitLSN that can be consulted
rather than the catalog table.

I think workers can cache that information when starts and invalidates
and reload the cache when the catalog gets updated. Specifying to
skip XID will update the catalog, invalidating the cache.

I think we need to think about when to
remove rows for which conflict has been resolved as we can't let that
information grow infinitely.

I guess we can update catalog tuples in place when another conflict
happens next time. The catalog tuple should be fixed size. The
already-resolved conflict will have the commit LSN older than its
replication origin's LSN.

Okay, but I have a slight concern that we will keep xid in the system
which might have been no longer valid. So, we will keep this info
about subscribers around till one performs drop subscription,
hopefully, that doesn't lead to too many rows. This will be okay as
per the current design but say tomorrow we decide to parallelize the
apply for a subscription then there could be multiple errors
corresponding to a subscription and in that case, such a design might
appear quite limiting. One possibility could be that when the launcher
is periodically checking for new error messages, it can clean up the
conflicts catalog as well, or maybe autovacuum does this periodically
as it does for stats (via pgstat_vacuum_stat).

Yeah, it's better to have a way to cleanup no longer valid entries in
the catalog in the case where the worker failed to remove it. I prefer
the former idea so far, so I'll implement it in a PoC patch.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Mon, May 31, 2021 at 12:39 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Sat, May 29, 2021 at 3:54 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

1. the worker records the XID and commit LSN of the failed transaction
to a catalog.

When will you record this info? I am not sure if we can try to update
this when an error has occurred. We can think of using try..catch in
apply worker and then record it in catch on error but would that be
advisable? One random thought that occurred to me is to that apply
worker notifies such information to the launcher (or maybe another
process) which will log this information.

Yeah, I was concerned about that too and had the same idea. The
information still could not be written if the server crashes before
the launcher writes it. But I think it's an acceptable.

True, because even if the launcher restarts, the apply worker will
error out again and resend the information. I guess we can have an
error queue where apply workers can add their information and the
launcher will then process those. If we do that, then we need to
probably define what we want to do if the queue gets full, either
apply worker nudge launcher and wait or it can just throw an error and
continue. If you have any better ideas to share this information then
we can consider those as well.

+1 for using error queue. Maybe we need to avoid queuing the same
error more than once to avoid the catalog from being updated
frequently?

Yes, I think it is important because after logging the subscription
may still error again unless the user does something to skip or
resolve the conflict. I guess you need to check for the existence of
error in systable and or in the queue.

I guess we can update catalog tuples in place when another conflict
happens next time. The catalog tuple should be fixed size. The
already-resolved conflict will have the commit LSN older than its
replication origin's LSN.

Okay, but I have a slight concern that we will keep xid in the system
which might have been no longer valid. So, we will keep this info
about subscribers around till one performs drop subscription,
hopefully, that doesn't lead to too many rows. This will be okay as
per the current design but say tomorrow we decide to parallelize the
apply for a subscription then there could be multiple errors
corresponding to a subscription and in that case, such a design might
appear quite limiting. One possibility could be that when the launcher
is periodically checking for new error messages, it can clean up the
conflicts catalog as well, or maybe autovacuum does this periodically
as it does for stats (via pgstat_vacuum_stat).

Yeah, it's better to have a way to cleanup no longer valid entries in
the catalog in the case where the worker failed to remove it. I prefer
the former idea so far,

Which idea do you refer to here as former (cleaning up by launcher)?

so I'll implement it in a PoC patch.

Okay.

--
With Regards,
Amit Kapila.

On 27.05.21 12:04, Amit Kapila wrote:

Also, I am thinking that instead of a stat view, do we need
to consider having a system table (pg_replication_conflicts or
something like that) for this because what if stats information is
lost (say either due to crash or due to udp packet loss), can we rely
on stats view for this?

Yeah, it seems better to use a catalog.

Okay.

Could you store it shared memory? You don't need it to be crash safe,
since the subscription will just run into the same error again after
restart. You just don't want it to be lost, like with the statistics
collector.

On Tue, Jun 1, 2021 at 12:55 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 27.05.21 12:04, Amit Kapila wrote:

Also, I am thinking that instead of a stat view, do we need
to consider having a system table (pg_replication_conflicts or
something like that) for this because what if stats information is
lost (say either due to crash or due to udp packet loss), can we rely
on stats view for this?

Yeah, it seems better to use a catalog.

Okay.

Could you store it shared memory? You don't need it to be crash safe,
since the subscription will just run into the same error again after
restart. You just don't want it to be lost, like with the statistics
collector.

But, won't that be costly in cases where we have errors in the
processing of very large transactions? Subscription has to process all
the data before it gets an error. I think we can even imagine this
feature to be extended to use commitLSN as a skip candidate in which
case we can even avoid getting the data of that transaction from the
publisher. So if this information is persistent, the user can even set
the skip identifier after the restart before the publisher can send
all the data.

Also, I think we can't assume after the restart we will get the same
error because the user can perform some operations after the restart
and before we try to apply the same transaction. It might be that the
user wanted to see all the errors before the user can set the skip
identifier (and or method).

I think the XID (or say another identifier like commitLSN) which we
want to use for skipping the transaction as specified by the user has
to be stored in the catalog because otherwise, after the restart we
won't remember it and the user won't know that he needs to set it
again. Now, say we have multiple skip identifiers (XIDs, commitLSN,
..), isn't it better to store all conflict-related information in a
separate catalog like pg_subscription_conflict or something like that.
I think it might be also better to later extend it for auto conflict
resolution where the user can specify auto conflict resolution info
for a subscription. Is it better to store all such information in
pg_subscription or have a separate catalog? It is possible that even
if we have a separate catalog for conflict info, we might not want to
store error info there.

--
With Regards,
Amit Kapila.

On Tue, Jun 1, 2021 at 1:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jun 1, 2021 at 12:55 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 27.05.21 12:04, Amit Kapila wrote:

Also, I am thinking that instead of a stat view, do we need
to consider having a system table (pg_replication_conflicts or
something like that) for this because what if stats information is
lost (say either due to crash or due to udp packet loss), can we rely
on stats view for this?

Yeah, it seems better to use a catalog.

Okay.

Could you store it shared memory? You don't need it to be crash safe,
since the subscription will just run into the same error again after
restart. You just don't want it to be lost, like with the statistics
collector.

But, won't that be costly in cases where we have errors in the
processing of very large transactions? Subscription has to process all
the data before it gets an error.

I had the same concern. Particularly, the approach we currently
discussed is to skip the transaction based on the information written
by the worker rather than require the user to specify the XID.
Therefore, we will always require the worker to process the same large
transaction after the restart in order to skip the transaction.

I think we can even imagine this
feature to be extended to use commitLSN as a skip candidate in which
case we can even avoid getting the data of that transaction from the
publisher. So if this information is persistent, the user can even set
the skip identifier after the restart before the publisher can send
all the data.

Another possible benefit of writing it to a catalog is that we can
replicate it to the physical standbys. If we have failover slots in
the future, the physical standby server also can resolve the conflict
without processing a possibly large transaction.

I think the XID (or say another identifier like commitLSN) which we
want to use for skipping the transaction as specified by the user has
to be stored in the catalog because otherwise, after the restart we
won't remember it and the user won't know that he needs to set it
again. Now, say we have multiple skip identifiers (XIDs, commitLSN,
..), isn't it better to store all conflict-related information in a
separate catalog like pg_subscription_conflict or something like that.
I think it might be also better to later extend it for auto conflict
resolution where the user can specify auto conflict resolution info
for a subscription. Is it better to store all such information in
pg_subscription or have a separate catalog? It is possible that even
if we have a separate catalog for conflict info, we might not want to
store error info there.

Just to be clear, we need to store only the conflict-related
information that cannot be resolved without manual intervention,
right? That is, conflicts cause an error, exiting the workers. In
general, replication conflicts include also conflicts that don’t cause
an error. I think that those conflicts don’t necessarily need to be
stored in the catalog and don’t require manual intervention.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, Jun 1, 2021 at 10:07 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Jun 1, 2021 at 1:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jun 1, 2021 at 12:55 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 27.05.21 12:04, Amit Kapila wrote:

Also, I am thinking that instead of a stat view, do we need
to consider having a system table (pg_replication_conflicts or
something like that) for this because what if stats information is
lost (say either due to crash or due to udp packet loss), can we rely
on stats view for this?

Yeah, it seems better to use a catalog.

Okay.

Could you store it shared memory? You don't need it to be crash safe,
since the subscription will just run into the same error again after
restart. You just don't want it to be lost, like with the statistics
collector.

But, won't that be costly in cases where we have errors in the
processing of very large transactions? Subscription has to process all
the data before it gets an error.

I had the same concern. Particularly, the approach we currently
discussed is to skip the transaction based on the information written
by the worker rather than require the user to specify the XID.

Yeah, but I was imagining that the user still needs to specify
something to indicate that we need to skip it, otherwise, we might try
to skip a transaction that the user wants to resolve by itself rather
than expecting us to skip it. Another point is if we don't store this
information in a persistent way then how will we restrict a user to
specify some random XID which is not even errored after restart.

Therefore, we will always require the worker to process the same large
transaction after the restart in order to skip the transaction.

I think we can even imagine this
feature to be extended to use commitLSN as a skip candidate in which
case we can even avoid getting the data of that transaction from the
publisher. So if this information is persistent, the user can even set
the skip identifier after the restart before the publisher can send
all the data.

Another possible benefit of writing it to a catalog is that we can
replicate it to the physical standbys. If we have failover slots in
the future, the physical standby server also can resolve the conflict
without processing a possibly large transaction.

makes sense.

I think the XID (or say another identifier like commitLSN) which we
want to use for skipping the transaction as specified by the user has
to be stored in the catalog because otherwise, after the restart we
won't remember it and the user won't know that he needs to set it
again. Now, say we have multiple skip identifiers (XIDs, commitLSN,
..), isn't it better to store all conflict-related information in a
separate catalog like pg_subscription_conflict or something like that.
I think it might be also better to later extend it for auto conflict
resolution where the user can specify auto conflict resolution info
for a subscription. Is it better to store all such information in
pg_subscription or have a separate catalog? It is possible that even
if we have a separate catalog for conflict info, we might not want to
store error info there.

Just to be clear, we need to store only the conflict-related
information that cannot be resolved without manual intervention,
right? That is, conflicts cause an error, exiting the workers. In
general, replication conflicts include also conflicts that don’t cause
an error. I think that those conflicts don’t necessarily need to be
stored in the catalog and don’t require manual intervention.

Yeah, I think we want to record the error cases but which other
conflicts you are talking about here which doesn't lead to any sort of
error?

--
With Regards,
Amit Kapila.

On Tue, Jun 1, 2021 at 2:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jun 1, 2021 at 10:07 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Jun 1, 2021 at 1:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jun 1, 2021 at 12:55 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 27.05.21 12:04, Amit Kapila wrote:

Also, I am thinking that instead of a stat view, do we need
to consider having a system table (pg_replication_conflicts or
something like that) for this because what if stats information is
lost (say either due to crash or due to udp packet loss), can we rely
on stats view for this?

Yeah, it seems better to use a catalog.

Okay.

Could you store it shared memory? You don't need it to be crash safe,
since the subscription will just run into the same error again after
restart. You just don't want it to be lost, like with the statistics
collector.

But, won't that be costly in cases where we have errors in the
processing of very large transactions? Subscription has to process all
the data before it gets an error.

I had the same concern. Particularly, the approach we currently
discussed is to skip the transaction based on the information written
by the worker rather than require the user to specify the XID.

Yeah, but I was imagining that the user still needs to specify
something to indicate that we need to skip it, otherwise, we might try
to skip a transaction that the user wants to resolve by itself rather
than expecting us to skip it.

Yeah, currently what I'm thinking is that the worker writes the
conflict that caused an error somewhere. If the user wants to resolve
it manually they can specify the resolution method to the stopped
subscription. Until the user specifies the method and the worker
resolves it or some fields of the subscription such as subconninfo are
updated, the conflict is not resolved and the information lasts.

I think the XID (or say another identifier like commitLSN) which we
want to use for skipping the transaction as specified by the user has
to be stored in the catalog because otherwise, after the restart we
won't remember it and the user won't know that he needs to set it
again. Now, say we have multiple skip identifiers (XIDs, commitLSN,
..), isn't it better to store all conflict-related information in a
separate catalog like pg_subscription_conflict or something like that.
I think it might be also better to later extend it for auto conflict
resolution where the user can specify auto conflict resolution info
for a subscription. Is it better to store all such information in
pg_subscription or have a separate catalog? It is possible that even
if we have a separate catalog for conflict info, we might not want to
store error info there.

Just to be clear, we need to store only the conflict-related
information that cannot be resolved without manual intervention,
right? That is, conflicts cause an error, exiting the workers. In
general, replication conflicts include also conflicts that don’t cause
an error. I think that those conflicts don’t necessarily need to be
stored in the catalog and don’t require manual intervention.

Yeah, I think we want to record the error cases but which other
conflicts you are talking about here which doesn't lead to any sort of
error?

For example, I think it's one type of replication conflict that two
updates that arrived via logical replication or from the client update
the same record (e.g., having the same primary key) at the same time.
In that case an error doesn't happen and we always choose the update
that arrived later. But there are other possible resolution methods
such as choosing the one that arrived former, using the one having a
newer commit timestamp, using something like priority of the node, and
even raising an error so that the user manually resolves it.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, Jun 1, 2021 at 1:24 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Jun 1, 2021 at 2:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jun 1, 2021 at 10:07 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Jun 1, 2021 at 1:01 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jun 1, 2021 at 12:55 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 27.05.21 12:04, Amit Kapila wrote:

Also, I am thinking that instead of a stat view, do we need
to consider having a system table (pg_replication_conflicts or
something like that) for this because what if stats information is
lost (say either due to crash or due to udp packet loss), can we rely
on stats view for this?

Yeah, it seems better to use a catalog.

Okay.

Could you store it shared memory? You don't need it to be crash safe,
since the subscription will just run into the same error again after
restart. You just don't want it to be lost, like with the statistics
collector.

But, won't that be costly in cases where we have errors in the
processing of very large transactions? Subscription has to process all
the data before it gets an error.

I had the same concern. Particularly, the approach we currently
discussed is to skip the transaction based on the information written
by the worker rather than require the user to specify the XID.

Yeah, but I was imagining that the user still needs to specify
something to indicate that we need to skip it, otherwise, we might try
to skip a transaction that the user wants to resolve by itself rather
than expecting us to skip it.

Yeah, currently what I'm thinking is that the worker writes the
conflict that caused an error somewhere. If the user wants to resolve
it manually they can specify the resolution method to the stopped
subscription. Until the user specifies the method and the worker
resolves it or some fields of the subscription such as subconninfo are
updated, the conflict is not resolved and the information lasts.

I think we can work out such details but not sure tinkering anything
with subconninfo was not in my mind.

I think the XID (or say another identifier like commitLSN) which we
want to use for skipping the transaction as specified by the user has
to be stored in the catalog because otherwise, after the restart we
won't remember it and the user won't know that he needs to set it
again. Now, say we have multiple skip identifiers (XIDs, commitLSN,
..), isn't it better to store all conflict-related information in a
separate catalog like pg_subscription_conflict or something like that.
I think it might be also better to later extend it for auto conflict
resolution where the user can specify auto conflict resolution info
for a subscription. Is it better to store all such information in
pg_subscription or have a separate catalog? It is possible that even
if we have a separate catalog for conflict info, we might not want to
store error info there.

Just to be clear, we need to store only the conflict-related
information that cannot be resolved without manual intervention,
right? That is, conflicts cause an error, exiting the workers. In
general, replication conflicts include also conflicts that don’t cause
an error. I think that those conflicts don’t necessarily need to be
stored in the catalog and don’t require manual intervention.

Yeah, I think we want to record the error cases but which other
conflicts you are talking about here which doesn't lead to any sort of
error?

For example, I think it's one type of replication conflict that two
updates that arrived via logical replication or from the client update
the same record (e.g., having the same primary key) at the same time.
In that case an error doesn't happen and we always choose the update
that arrived later.

I think we choose whichever is earlier as we first try to find the
tuple in local rel and if not found then we silently ignore the
update/delete operation.

But there are other possible resolution methods
such as choosing the one that arrived former, using the one having a
newer commit timestamp, using something like priority of the node, and
even raising an error so that the user manually resolves it.

Agreed. I think we need to log only the ones which lead to error.

--
With Regards,
Amit Kapila.

On 01.06.21 06:01, Amit Kapila wrote:

But, won't that be costly in cases where we have errors in the
processing of very large transactions? Subscription has to process all
the data before it gets an error. I think we can even imagine this
feature to be extended to use commitLSN as a skip candidate in which
case we can even avoid getting the data of that transaction from the
publisher. So if this information is persistent, the user can even set
the skip identifier after the restart before the publisher can send
all the data.

At least in current practice, skipping parts of the logical replication
stream on the subscriber is a rare, emergency-level operation when
something that shouldn't have happened happened. So it doesn't really
matter how costly it is. It's not going to be more costly than the
error happening in the first place. All you'd need is one shared memory
slot per subscription to store a xid to skip.

We will also want some proper conflict handling at some point. But I
think what is being discussed here is meant to be a repair tool, not a
policy tool, and I'm afraid it might get over-engineered.

On Tue, Jun 1, 2021 at 9:05 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 01.06.21 06:01, Amit Kapila wrote:

But, won't that be costly in cases where we have errors in the
processing of very large transactions? Subscription has to process all
the data before it gets an error. I think we can even imagine this
feature to be extended to use commitLSN as a skip candidate in which
case we can even avoid getting the data of that transaction from the
publisher. So if this information is persistent, the user can even set
the skip identifier after the restart before the publisher can send
all the data.

At least in current practice, skipping parts of the logical replication
stream on the subscriber is a rare, emergency-level operation when
something that shouldn't have happened happened. So it doesn't really
matter how costly it is. It's not going to be more costly than the
error happening in the first place. All you'd need is one shared memory
slot per subscription to store a xid to skip.

Leaving aside the performance point, how can we do by just storing
skip identifier (XID/commitLSN) in shared_memory? How will the apply
worker know about that information after restart? Do you expect the
user to set it again, if so, I think users might not like that? Also,
how will we prohibit users to give some identifier other than for
failed transactions, and if users provide that what should be our
action? Without that, if users provide XID of some in-progress
transaction, we might need to do more work (rollback) than just
skipping it.

We will also want some proper conflict handling at some point. But I
think what is being discussed here is meant to be a repair tool, not a
policy tool, and I'm afraid it might get over-engineered.

I got your point but I am also a bit skeptical that handling all
boundary cases might become tricky if we go with a simple shared
memory technique but OTOH if we can handle all such cases then it is
fine.

--
With Regards,
Amit Kapila.

On Wed, Jun 2, 2021 at 3:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jun 1, 2021 at 9:05 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 01.06.21 06:01, Amit Kapila wrote:

But, won't that be costly in cases where we have errors in the
processing of very large transactions? Subscription has to process all
the data before it gets an error. I think we can even imagine this
feature to be extended to use commitLSN as a skip candidate in which
case we can even avoid getting the data of that transaction from the
publisher. So if this information is persistent, the user can even set
the skip identifier after the restart before the publisher can send
all the data.

At least in current practice, skipping parts of the logical replication
stream on the subscriber is a rare, emergency-level operation when
something that shouldn't have happened happened. So it doesn't really
matter how costly it is. It's not going to be more costly than the
error happening in the first place. All you'd need is one shared memory
slot per subscription to store a xid to skip.

Leaving aside the performance point, how can we do by just storing
skip identifier (XID/commitLSN) in shared_memory? How will the apply
worker know about that information after restart? Do you expect the
user to set it again, if so, I think users might not like that? Also,
how will we prohibit users to give some identifier other than for
failed transactions, and if users provide that what should be our
action? Without that, if users provide XID of some in-progress
transaction, we might need to do more work (rollback) than just
skipping it.

I think the simplest solution would be to have a fixed-size array on
the shared memory to store information of skipping transactions on the
particular subscription. Given that this feature is meant to be a
repair tool in emergency cases, 32 or 64 entries seem enough. That
information should be visible to users via a system view and each
entry is cleared once the worker has skipped the transaction. Also, we
also would need to clear the entry if the meta information of the
subscription such as conninfo and slot name has been changed. The
worker reads that information at least when starting logical
replication. The worker receives changes from the publication and
checks if the transaction should be skipped when start to apply those
changes. If so the worker skips applying all changes of the
transaction and removes stream files if exist.

Regarding the point of how to check if the specified XID by the user
is valid, I guess it’s not easy to do that since XIDs sent from the
publisher are in random order. Considering the use case of this tool,
the situation seems like the logical replication gets stuck due to a
problem transaction and the worker repeatedly restarts and raises an
error. So I guess it also would be a good idea that the user can
specify to skip the first transaction (or first N transactions) since
the subscription starts logical replication. It’s less flexible but
seems enough to solve such a situation and doesn’t have such a problem
of validating the XID. If the functionality like letting the
subscriber know the oldest XID that is possibly sent is useful also
for other purposes it would also be a good idea to implement it but
not sure about other use cases.

Anyway, it seems to me that we need to consider the user interface
first, especially how and what the user specifies the transaction to
skip. My current feeling is that specifying XID is intuitive and
flexible but the user needs to have 2 steps: checks XID and then
specifies it, and there is a risk that the user mistakenly specifies a
wrong XID. On the other hand, the idea of specifying to skip the first
transaction doesn’t require the user to check and specify XID but is
less flexible, and “the first” transaction might be ambiguous for the
user.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, Jun 15, 2021 at 6:13 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Jun 2, 2021 at 3:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jun 1, 2021 at 9:05 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 01.06.21 06:01, Amit Kapila wrote:

But, won't that be costly in cases where we have errors in the
processing of very large transactions? Subscription has to process all
the data before it gets an error. I think we can even imagine this
feature to be extended to use commitLSN as a skip candidate in which
case we can even avoid getting the data of that transaction from the
publisher. So if this information is persistent, the user can even set
the skip identifier after the restart before the publisher can send
all the data.

At least in current practice, skipping parts of the logical replication
stream on the subscriber is a rare, emergency-level operation when
something that shouldn't have happened happened. So it doesn't really
matter how costly it is. It's not going to be more costly than the
error happening in the first place. All you'd need is one shared memory
slot per subscription to store a xid to skip.

Leaving aside the performance point, how can we do by just storing
skip identifier (XID/commitLSN) in shared_memory? How will the apply
worker know about that information after restart? Do you expect the
user to set it again, if so, I think users might not like that? Also,
how will we prohibit users to give some identifier other than for
failed transactions, and if users provide that what should be our
action? Without that, if users provide XID of some in-progress
transaction, we might need to do more work (rollback) than just
skipping it.

I think the simplest solution would be to have a fixed-size array on
the shared memory to store information of skipping transactions on the
particular subscription. Given that this feature is meant to be a
repair tool in emergency cases, 32 or 64 entries seem enough.

IIUC, here you are talking about xids specified by the user to skip?
If so, then how will you get that information after the restart, and
why you need 32 or 64 entries for it?

Anyway, it seems to me that we need to consider the user interface
first, especially how and what the user specifies the transaction to
skip. My current feeling is that specifying XID is intuitive and
flexible but the user needs to have 2 steps: checks XID and then
specifies it, and there is a risk that the user mistakenly specifies a
wrong XID. On the other hand, the idea of specifying to skip the first
transaction doesn’t require the user to check and specify XID but is
less flexible, and “the first” transaction might be ambiguous for the
user.

I see your point in allowing to specify First N transactions but OTOH,
I am slightly afraid that it might lead to skipping some useful
transactions which will make replica out-of-sync. BTW, is there any
data point for the user to check how many transactions it can skip?
Normally, we won't be able to proceed till we resolve/skip the
transaction that is generating an error. One possibility could be that
we provide some *superuser* functions like
pg_logical_replication_skip_xact()/pg_logical_replication_reset_skip_xact()
which takes subscription name/id and xid as input parameters. Then, I
think we can store this information in ReplicationState and probably
try to map to originid from subscription name/id to retrieve that
info. We can probably document that the effects of these functions
won't last after the restart. Now, if this function is used by super
users then we can probably trust that they provide the XIDs that we
can trust to be skipped but OTOH making a restriction to allow these
functions to be used by superusers might restrict the usage of this
repair tool.

--
With Regards,
Amit Kapila.

On Wed, Jun 16, 2021 at 6:05 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jun 15, 2021 at 6:13 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Jun 2, 2021 at 3:07 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jun 1, 2021 at 9:05 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:

On 01.06.21 06:01, Amit Kapila wrote:

But, won't that be costly in cases where we have errors in the
processing of very large transactions? Subscription has to process all
the data before it gets an error. I think we can even imagine this
feature to be extended to use commitLSN as a skip candidate in which
case we can even avoid getting the data of that transaction from the
publisher. So if this information is persistent, the user can even set
the skip identifier after the restart before the publisher can send
all the data.

At least in current practice, skipping parts of the logical replication
stream on the subscriber is a rare, emergency-level operation when
something that shouldn't have happened happened. So it doesn't really
matter how costly it is. It's not going to be more costly than the
error happening in the first place. All you'd need is one shared memory
slot per subscription to store a xid to skip.

Leaving aside the performance point, how can we do by just storing
skip identifier (XID/commitLSN) in shared_memory? How will the apply
worker know about that information after restart? Do you expect the
user to set it again, if so, I think users might not like that? Also,
how will we prohibit users to give some identifier other than for
failed transactions, and if users provide that what should be our
action? Without that, if users provide XID of some in-progress
transaction, we might need to do more work (rollback) than just
skipping it.

I think the simplest solution would be to have a fixed-size array on
the shared memory to store information of skipping transactions on the
particular subscription. Given that this feature is meant to be a
repair tool in emergency cases, 32 or 64 entries seem enough.

IIUC, here you are talking about xids specified by the user to skip?

Yes. I think we need to store pairs of subid and xid.

If so, then how will you get that information after the restart, and
why you need 32 or 64 entries for it?

That information doesn't last after the restart. I think that the
situation that DBA uses this tool would be that they fix the
subscription on the spot. Once the subscription skipped the
transaction, the entry of that information is cleared. So I’m thinking
that we don’t need to hold many entries and it does not necessarily to
be durable. I think your below idea of storing that information in
ReplicationState seems better to me.

Anyway, it seems to me that we need to consider the user interface
first, especially how and what the user specifies the transaction to
skip. My current feeling is that specifying XID is intuitive and
flexible but the user needs to have 2 steps: checks XID and then
specifies it, and there is a risk that the user mistakenly specifies a
wrong XID. On the other hand, the idea of specifying to skip the first
transaction doesn’t require the user to check and specify XID but is
less flexible, and “the first” transaction might be ambiguous for the
user.

I see your point in allowing to specify First N transactions but OTOH,
I am slightly afraid that it might lead to skipping some useful
transactions which will make replica out-of-sync.

Agreed.

It might be better to skip only the first transaction.

BTW, is there any
data point for the user to check how many transactions it can skip?
Normally, we won't be able to proceed till we resolve/skip the
transaction that is generating an error. One possibility could be that
we provide some *superuser* functions like
pg_logical_replication_skip_xact()/pg_logical_replication_reset_skip_xact()
which takes subscription name/id and xid as input parameters. Then, I
think we can store this information in ReplicationState and probably
try to map to originid from subscription name/id to retrieve that
info. We can probably document that the effects of these functions
won't last after the restart.

ReplicationState seems a reasonable place to store that information.

Now, if this function is used by super
users then we can probably trust that they provide the XIDs that we
can trust to be skipped but OTOH making a restriction to allow these
functions to be used by superusers might restrict the usage of this
repair tool.

If we specify the subscription id or name, maybe we can allow also the
owner of subscription to do that operation?

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Thu, Jun 17, 2021 at 3:24 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Now, if this function is used by super
users then we can probably trust that they provide the XIDs that we
can trust to be skipped but OTOH making a restriction to allow these
functions to be used by superusers might restrict the usage of this
repair tool.

If we specify the subscription id or name, maybe we can allow also the
owner of subscription to do that operation?

Ah, the owner of the subscription must be superuser.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Thu, Jun 17, 2021 at 6:20 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jun 17, 2021 at 3:24 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Now, if this function is used by super
users then we can probably trust that they provide the XIDs that we
can trust to be skipped but OTOH making a restriction to allow these
functions to be used by superusers might restrict the usage of this
repair tool.

If we specify the subscription id or name, maybe we can allow also the
owner of subscription to do that operation?

Ah, the owner of the subscription must be superuser.

I've attached PoC patches.

0001 patch introduces the ability to skip transactions on the
subscriber side. We can specify XID to the subscription by like ALTER
SUBSCRIPTION test_sub SET SKIP TRANSACTION 100. The implementation
seems straightforward except for setting origin state. After skipping
the transaction we have to update the session origin state so that we
can start streaming the transaction next to the one that we just
skipped in case of the server crash or restarting the apply worker. We
set origin state to the commit WAL record. However, since we skip all
changes we don’t write any WAL even if we call CommitTransaction() at
the end of the skipped transaction. So the patch sets the origin state
to the transaction that updates the pg_subscription system catalog to
reset the skip XID. I think we need a discussion of this part.

With 0002 and 0003 patches, we report the error information in server
logs and the stats view, respectively. 0002 patch adds errcontext for
messages that happened during applying the changes:

ERROR: duplicate key value violates unique constraint "hoge_pkey"
DETAIL: Key (c)=(1) already exists.
CONTEXT: during apply of "INSERT" for relation "public.hoge" in
transaction with xid 736 committs 2021-06-27 12:12:30.053887+09

0003 patch adds pg_stat_logical_replication_error statistics view
discussed on another thread[1]/messages/by-id/DB35438F-9356-4841-89A0-412709EBD3AB@enterprisedb.com. The apply worker sends the error
information to the stats collector if an error happens during applying
changes. We can check those errors as follow:

postgres(1:25250)=# select * from pg_stat_logical_replication_error;
subname | relid | action | xid | last_failure
----------+-------+--------+-----+-------------------------------
test_sub | 16384 | INSERT | 736 | 2021-06-27 12:12:45.142675+09
(1 row)

I added only columns required for the skipping transaction feature to
the view for now.

Please note that those patches are meant to evaluate the concept we've
discussed so far. Those don't have the doc update yet.

Regards,

[1]: /messages/by-id/DB35438F-9356-4841-89A0-412709EBD3AB@enterprisedb.com

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Mon, Jun 28, 2021 at 10:12 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jun 17, 2021 at 6:20 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jun 17, 2021 at 3:24 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Now, if this function is used by super
users then we can probably trust that they provide the XIDs that we
can trust to be skipped but OTOH making a restriction to allow these
functions to be used by superusers might restrict the usage of this
repair tool.

If we specify the subscription id or name, maybe we can allow also the
owner of subscription to do that operation?

Ah, the owner of the subscription must be superuser.

I've attached PoC patches.

0001 patch introduces the ability to skip transactions on the
subscriber side. We can specify XID to the subscription by like ALTER
SUBSCRIPTION test_sub SET SKIP TRANSACTION 100. The implementation
seems straightforward except for setting origin state. After skipping
the transaction we have to update the session origin state so that we
can start streaming the transaction next to the one that we just
skipped in case of the server crash or restarting the apply worker. We
set origin state to the commit WAL record. However, since we skip all
changes we don’t write any WAL even if we call CommitTransaction() at
the end of the skipped transaction. So the patch sets the origin state
to the transaction that updates the pg_subscription system catalog to
reset the skip XID. I think we need a discussion of this part.

IIUC, for streaming transactions you are allowing stream file to be
created and then remove it at stream_commit/stream_abort time, is that
right? If so, in which cases are you imagining the files to be
created, is it in the case of relation message
(LOGICAL_REP_MSG_RELATION)? Assuming the previous two statements are
correct, this will skip the relation message as well as part of the
removal of stream files which might lead to a problem because the
publisher won't know that we have skipped the relation message and it
won't send it again. This can cause problems while processing the next
messages.

With 0002 and 0003 patches, we report the error information in server
logs and the stats view, respectively. 0002 patch adds errcontext for
messages that happened during applying the changes:

ERROR: duplicate key value violates unique constraint "hoge_pkey"
DETAIL: Key (c)=(1) already exists.
CONTEXT: during apply of "INSERT" for relation "public.hoge" in
transaction with xid 736 committs 2021-06-27 12:12:30.053887+09

0003 patch adds pg_stat_logical_replication_error statistics view
discussed on another thread[1]. The apply worker sends the error
information to the stats collector if an error happens during applying
changes. We can check those errors as follow:

postgres(1:25250)=# select * from pg_stat_logical_replication_error;
subname | relid | action | xid | last_failure
----------+-------+--------+-----+-------------------------------
test_sub | 16384 | INSERT | 736 | 2021-06-27 12:12:45.142675+09
(1 row)

I added only columns required for the skipping transaction feature to
the view for now.

Isn't it better to add an error message if possible?

Please note that those patches are meant to evaluate the concept we've
discussed so far. Those don't have the doc update yet.

I think your patch is on the lines of what we have discussed. It would
be good if you can update docs and add few tests.

--
With Regards,
Amit Kapila.

On Wed, Jun 30, 2021 at 4:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Jun 28, 2021 at 10:12 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

0003 patch adds pg_stat_logical_replication_error statistics view
discussed on another thread[1]. The apply worker sends the error
information to the stats collector if an error happens during applying
changes. We can check those errors as follow:

postgres(1:25250)=# select * from pg_stat_logical_replication_error;
subname | relid | action | xid | last_failure
----------+-------+--------+-----+-------------------------------
test_sub | 16384 | INSERT | 736 | 2021-06-27 12:12:45.142675+09
(1 row)

I added only columns required for the skipping transaction feature to
the view for now.

Isn't it better to add an error message if possible?

Don't we want to clear stats at drop subscription as well? We do drop
database stats in dropdb via pgstat_drop_database, so I think we need
to clear subscription stats at the time of drop subscription.

In the 0003 patch, if I am reading it correctly then the patch is not
doing anything for tablesync worker. It is not clear to me at this
stage what exactly we want to do about it? Do we want to just ignore
errors from tablesync worker and let the system behave as it is
without this feature? If we want to do anything then I think the way
to skip the initial table sync would be to behave like the user has
given 'copy_data' option as false.

--
With Regards,
Amit Kapila.

On Wed, Jun 30, 2021 at 8:05 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Jun 28, 2021 at 10:12 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jun 17, 2021 at 6:20 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jun 17, 2021 at 3:24 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Now, if this function is used by super
users then we can probably trust that they provide the XIDs that we
can trust to be skipped but OTOH making a restriction to allow these
functions to be used by superusers might restrict the usage of this
repair tool.

If we specify the subscription id or name, maybe we can allow also the
owner of subscription to do that operation?

Ah, the owner of the subscription must be superuser.

I've attached PoC patches.

0001 patch introduces the ability to skip transactions on the
subscriber side. We can specify XID to the subscription by like ALTER
SUBSCRIPTION test_sub SET SKIP TRANSACTION 100. The implementation
seems straightforward except for setting origin state. After skipping
the transaction we have to update the session origin state so that we
can start streaming the transaction next to the one that we just
skipped in case of the server crash or restarting the apply worker. We
set origin state to the commit WAL record. However, since we skip all
changes we don’t write any WAL even if we call CommitTransaction() at
the end of the skipped transaction. So the patch sets the origin state
to the transaction that updates the pg_subscription system catalog to
reset the skip XID. I think we need a discussion of this part.

IIUC, for streaming transactions you are allowing stream file to be
created and then remove it at stream_commit/stream_abort time, is that
right?

Right.

If so, in which cases are you imagining the files to be
created, is it in the case of relation message
(LOGICAL_REP_MSG_RELATION)? Assuming the previous two statements are
correct, this will skip the relation message as well as part of the
removal of stream files which might lead to a problem because the
publisher won't know that we have skipped the relation message and it
won't send it again. This can cause problems while processing the next
messages.

Good point. In the current patch, we skip all streamed changes at
stream_commit/abort but it should apply changes while skipping only
data-modification changes as we do for non-stream changes.

With 0002 and 0003 patches, we report the error information in server
logs and the stats view, respectively. 0002 patch adds errcontext for
messages that happened during applying the changes:

ERROR: duplicate key value violates unique constraint "hoge_pkey"
DETAIL: Key (c)=(1) already exists.
CONTEXT: during apply of "INSERT" for relation "public.hoge" in
transaction with xid 736 committs 2021-06-27 12:12:30.053887+09

0003 patch adds pg_stat_logical_replication_error statistics view
discussed on another thread[1]. The apply worker sends the error
information to the stats collector if an error happens during applying
changes. We can check those errors as follow:

postgres(1:25250)=# select * from pg_stat_logical_replication_error;
subname | relid | action | xid | last_failure
----------+-------+--------+-----+-------------------------------
test_sub | 16384 | INSERT | 736 | 2021-06-27 12:12:45.142675+09
(1 row)

I added only columns required for the skipping transaction feature to
the view for now.

Isn't it better to add an error message if possible?

Please note that those patches are meant to evaluate the concept we've
discussed so far. Those don't have the doc update yet.

I think your patch is on the lines of what we have discussed. It would
be good if you can update docs and add few tests.

Okay. I'll incorporate the above suggestions in the next version patch.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Thu, Jul 1, 2021 at 1:24 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Jun 30, 2021 at 8:05 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

If so, in which cases are you imagining the files to be
created, is it in the case of relation message
(LOGICAL_REP_MSG_RELATION)? Assuming the previous two statements are
correct, this will skip the relation message as well as part of the
removal of stream files which might lead to a problem because the
publisher won't know that we have skipped the relation message and it
won't send it again. This can cause problems while processing the next
messages.

Good point. In the current patch, we skip all streamed changes at
stream_commit/abort but it should apply changes while skipping only
data-modification changes as we do for non-stream changes.

Right.

--
With Regards,
Amit Kapila.

On Thu, Jul 1, 2021 at 12:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Jun 30, 2021 at 4:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Jun 28, 2021 at 10:12 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

0003 patch adds pg_stat_logical_replication_error statistics view
discussed on another thread[1]. The apply worker sends the error
information to the stats collector if an error happens during applying
changes. We can check those errors as follow:

postgres(1:25250)=# select * from pg_stat_logical_replication_error;
subname | relid | action | xid | last_failure
----------+-------+--------+-----+-------------------------------
test_sub | 16384 | INSERT | 736 | 2021-06-27 12:12:45.142675+09
(1 row)

I added only columns required for the skipping transaction feature to
the view for now.

Isn't it better to add an error message if possible?

Don't we want to clear stats at drop subscription as well? We do drop
database stats in dropdb via pgstat_drop_database, so I think we need
to clear subscription stats at the time of drop subscription.

Yes, it needs to be cleared. In the 0003 patch, pgstat_vacuum_stat()
sends the message to clear the stats. I think it's better to have
pgstat_vacuum_stat() do that job similar to dropping replication slot
statistics rather than relying on the single message send at DROP
SUBSCRIPTION. I've considered doing both: sending the message at DROP
SUBSCRIPTION and periodical checking by pgstat_vacuum_stat(), but
dropping subscription not setting a replication slot is able to
rollback. So we need to send it only at commit time. Given that we
don’t necessarily need the stats to be updated immediately, I think
it’s reasonable to go with only a way of pgstat_vacuum_stat().

In the 0003 patch, if I am reading it correctly then the patch is not
doing anything for tablesync worker. It is not clear to me at this
stage what exactly we want to do about it? Do we want to just ignore
errors from tablesync worker and let the system behave as it is
without this feature? If we want to do anything then I think the way
to skip the initial table sync would be to behave like the user has
given 'copy_data' option as false.

It might be better to have also sync workers report errors, even if
SKIP TRANSACTION feature doesn’t support anything for initial table
synchronization. From the user perspective, The initial table
synchronization is also the part of logical replication operations. If
we report only error information of applying logical changes, it could
confuse users.

But I’m not sure about the way to skip the initial table
synchronization. Once we set `copy_data` to false, all table
synchronizations are disabled. Some of them might have been able to
synchronize successfully. It might be useful if the user can disable
the table initialization for the particular tables.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Thu, Jul 1, 2021 at 6:31 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jul 1, 2021 at 12:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

Don't we want to clear stats at drop subscription as well? We do drop
database stats in dropdb via pgstat_drop_database, so I think we need
to clear subscription stats at the time of drop subscription.

Yes, it needs to be cleared. In the 0003 patch, pgstat_vacuum_stat()
sends the message to clear the stats. I think it's better to have
pgstat_vacuum_stat() do that job similar to dropping replication slot
statistics rather than relying on the single message send at DROP
SUBSCRIPTION. I've considered doing both: sending the message at DROP
SUBSCRIPTION and periodical checking by pgstat_vacuum_stat(), but
dropping subscription not setting a replication slot is able to
rollback. So we need to send it only at commit time. Given that we
don’t necessarily need the stats to be updated immediately, I think
it’s reasonable to go with only a way of pgstat_vacuum_stat().

Okay, that makes sense. Can we consider sending the multiple ids in
one message as we do for relations or functions in
pgstat_vacuum_stat()? That will reduce some message traffic. BTW, do
we have some way to avoid wrapping around the OID before we clean up
via pgstat_vacuum_stat()?

In the 0003 patch, if I am reading it correctly then the patch is not
doing anything for tablesync worker. It is not clear to me at this
stage what exactly we want to do about it? Do we want to just ignore
errors from tablesync worker and let the system behave as it is
without this feature? If we want to do anything then I think the way
to skip the initial table sync would be to behave like the user has
given 'copy_data' option as false.

It might be better to have also sync workers report errors, even if
SKIP TRANSACTION feature doesn’t support anything for initial table
synchronization. From the user perspective, The initial table
synchronization is also the part of logical replication operations. If
we report only error information of applying logical changes, it could
confuse users.

But I’m not sure about the way to skip the initial table
synchronization. Once we set `copy_data` to false, all table
synchronizations are disabled. Some of them might have been able to
synchronize successfully. It might be useful if the user can disable
the table initialization for the particular tables.

True but I guess the user can wait for all the tablesyncs to either
finish or get an error corresponding to the table sync. After that, it
can use 'copy_data' as false. This is not a very good method but I
don't see any other option. I guess whatever is the case logging
errors from tablesyncs is anyway not a bad idea.

Instead of using the syntax "ALTER SUBSCRIPTION name SET SKIP
TRANSACTION Iconst", isn't it better to use it as a subscription
option like Mark has done for his patch (disable_on_error)?

I am slightly nervous about this way of allowing the user to skip the
errors because if it is not used carefully then it can easily lead to
inconsistent data on the subscriber. I agree that as only superusers
will be allowed to use this option and we can document clearly the
side-effects, the risk could be reduced but is that sufficient? It is
not that we don't have any other tool which allows users to make their
data inconsistent (one recent example is functions
(heap_force_kill/heap_force_freeze) in pg_surgery module) if not used
carefully but it might be better to not expose such tools.

OTOH, if we use the error infrastructure of this patch and allow users
to just disable the subscription on error as was proposed by Mark then
that can't lead to any inconsistency.

What do you think?

--
With Regards,
Amit Kapila.

On Mon, Jul 5, 2021 at 3:16 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Jul 1, 2021 at 6:31 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jul 1, 2021 at 12:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

Instead of using the syntax "ALTER SUBSCRIPTION name SET SKIP
TRANSACTION Iconst", isn't it better to use it as a subscription
option like Mark has done for his patch (disable_on_error)?

I am slightly nervous about this way of allowing the user to skip the
errors because if it is not used carefully then it can easily lead to
inconsistent data on the subscriber. I agree that as only superusers
will be allowed to use this option and we can document clearly the
side-effects, the risk could be reduced but is that sufficient?

I see that users can create a similar effect by using
pg_replication_origin_advance() and it is mentioned in the docs that
careless use of this function can lead to inconsistently replicated
data. So, this new way doesn't seem to be any more dangerous than what
we already have.

--
With Regards,
Amit Kapila.

Hi,
Have a few notes about pg_stat_logical_replication_error from the DBA point
of view (which will use this view in the future).
1. As I understand it, this view might contain many errors related to
different subscriptions. It is better to name
"pg_stat_logical_replication_errors" using the plural form (like this done
for stat views for tables, indexes, functions). Also, I'd like to suggest
thinking twice about the view name (and function used in view DDL) -
"pg_stat_logical_replication_error" contains very common "logical
replication" words, but the view contains errors related to subscriptions
only. In the future there could be other kinds of errors related to logical
replication, but not related to subscriptions - what will you do?
2. Add a field with database name or id - it helps to quickly understand to
which database the subscription belongs.
3. Add a counter field with total number of errors - it helps to calculate
errors rates and aggregations (sum), and don't lose information about
errors between view checks.
4. Add text of last error (if it will not be too expensive).
5. Rename the "action" field to "command", as I know this is right from
terminology point of view.

Finally, the view might seems like this:

postgres(1:25250)=# select * from pg_stat_logical_replication_errors;
subname | datid | relid | command | xid | total | last_failure |
last_failure_text
----------+--------+-------+---------+-----+-------+-------------------------------+---------------------------
sub_1 | 12345 | 16384 | INSERT | 736 | 145 | 2021-06-27 12:12:45.142675+09
| something goes wrong...
sub_2 | 12346 | 16458 | UPDATE | 845 | 12 | 2021-06-27 12:16:01.458752+09 |
hmm, something goes wrong

Regards, Alexey

On Mon, Jul 5, 2021 at 2:59 PM Masahiko Sawada <sawada.mshk@gmail.com>
wrote:

On Thu, Jun 17, 2021 at 6:20 PM Masahiko Sawada <sawada.mshk@gmail.com>
wrote:

On Thu, Jun 17, 2021 at 3:24 PM Masahiko Sawada <sawada.mshk@gmail.com>

wrote:

Now, if this function is used by super
users then we can probably trust that they provide the XIDs that we
can trust to be skipped but OTOH making a restriction to allow these
functions to be used by superusers might restrict the usage of this
repair tool.

If we specify the subscription id or name, maybe we can allow also the
owner of subscription to do that operation?

Ah, the owner of the subscription must be superuser.

I've attached PoC patches.

0001 patch introduces the ability to skip transactions on the
subscriber side. We can specify XID to the subscription by like ALTER
SUBSCRIPTION test_sub SET SKIP TRANSACTION 100. The implementation
seems straightforward except for setting origin state. After skipping
the transaction we have to update the session origin state so that we
can start streaming the transaction next to the one that we just
skipped in case of the server crash or restarting the apply worker. We
set origin state to the commit WAL record. However, since we skip all
changes we don’t write any WAL even if we call CommitTransaction() at
the end of the skipped transaction. So the patch sets the origin state
to the transaction that updates the pg_subscription system catalog to
reset the skip XID. I think we need a discussion of this part.

With 0002 and 0003 patches, we report the error information in server
logs and the stats view, respectively. 0002 patch adds errcontext for
messages that happened during applying the changes:

ERROR: duplicate key value violates unique constraint "hoge_pkey"
DETAIL: Key (c)=(1) already exists.
CONTEXT: during apply of "INSERT" for relation "public.hoge" in
transaction with xid 736 committs 2021-06-27 12:12:30.053887+09

0003 patch adds pg_stat_logical_replication_error statistics view
discussed on another thread[1]. The apply worker sends the error
information to the stats collector if an error happens during applying
changes. We can check those errors as follow:

postgres(1:25250)=# select * from pg_stat_logical_replication_error;
subname | relid | action | xid | last_failure
----------+-------+--------+-----+-------------------------------
test_sub | 16384 | INSERT | 736 | 2021-06-27 12:12:45.142675+09
(1 row)

I added only columns required for the skipping transaction feature to
the view for now.

Please note that those patches are meant to evaluate the concept we've
discussed so far. Those don't have the doc update yet.

Regards,

[1]
/messages/by-id/DB35438F-9356-4841-89A0-412709EBD3AB@enterprisedb.com

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

--
С уважением Алексей В. Лесовский

On Mon, Jul 5, 2021 at 7:33 PM Alexey Lesovsky <lesovsky@gmail.com> wrote:

Hi,
Have a few notes about pg_stat_logical_replication_error from the DBA point of view (which will use this view in the future).

Thank you for the comments!

1. As I understand it, this view might contain many errors related to different subscriptions. It is better to name "pg_stat_logical_replication_errors" using the plural form (like this done for stat views for tables, indexes, functions).

Agreed.

Also, I'd like to suggest thinking twice about the view name (and function used in view DDL) - "pg_stat_logical_replication_error" contains very common "logical replication" words, but the view contains errors related to subscriptions only. In the future there could be other kinds of errors related to logical replication, but not related to subscriptions - what will you do?

Is pg_stat_subscription_errors or
pg_stat_logical_replication_apply_errors better?

2. Add a field with database name or id - it helps to quickly understand to which database the subscription belongs.

Agreed.

3. Add a counter field with total number of errors - it helps to calculate errors rates and aggregations (sum), and don't lose information about errors between view checks.

Do you mean to increment the error count if the error (command, xid,
and relid) is the same as the previous one? or to have the total
number of errors per subscription? And what can we infer from the
error rates and aggregations?

4. Add text of last error (if it will not be too expensive).

Agreed.

5. Rename the "action" field to "command", as I know this is right from terminology point of view.

Okay.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Mon, Jul 5, 2021 at 6:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Jul 1, 2021 at 6:31 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jul 1, 2021 at 12:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

Don't we want to clear stats at drop subscription as well? We do drop
database stats in dropdb via pgstat_drop_database, so I think we need
to clear subscription stats at the time of drop subscription.

Yes, it needs to be cleared. In the 0003 patch, pgstat_vacuum_stat()
sends the message to clear the stats. I think it's better to have
pgstat_vacuum_stat() do that job similar to dropping replication slot
statistics rather than relying on the single message send at DROP
SUBSCRIPTION. I've considered doing both: sending the message at DROP
SUBSCRIPTION and periodical checking by pgstat_vacuum_stat(), but
dropping subscription not setting a replication slot is able to
rollback. So we need to send it only at commit time. Given that we
don’t necessarily need the stats to be updated immediately, I think
it’s reasonable to go with only a way of pgstat_vacuum_stat().

Okay, that makes sense. Can we consider sending the multiple ids in
one message as we do for relations or functions in
pgstat_vacuum_stat()? That will reduce some message traffic.

Yes. Since subscriptions are objects that are not frequently created
and dropped I prioritized not to increase the message type. But if we
do that for subscriptions, is it better to do that for replication
slots as well? It seems to me that the lifetime of subscriptions and
replication slots are similar.

BTW, do
we have some way to avoid wrapping around the OID before we clean up
via pgstat_vacuum_stat()?

As far as I know there is not.

In the 0003 patch, if I am reading it correctly then the patch is not
doing anything for tablesync worker. It is not clear to me at this
stage what exactly we want to do about it? Do we want to just ignore
errors from tablesync worker and let the system behave as it is
without this feature? If we want to do anything then I think the way
to skip the initial table sync would be to behave like the user has
given 'copy_data' option as false.

It might be better to have also sync workers report errors, even if
SKIP TRANSACTION feature doesn’t support anything for initial table
synchronization. From the user perspective, The initial table
synchronization is also the part of logical replication operations. If
we report only error information of applying logical changes, it could
confuse users.

But I’m not sure about the way to skip the initial table
synchronization. Once we set `copy_data` to false, all table
synchronizations are disabled. Some of them might have been able to
synchronize successfully. It might be useful if the user can disable
the table initialization for the particular tables.

True but I guess the user can wait for all the tablesyncs to either
finish or get an error corresponding to the table sync. After that, it
can use 'copy_data' as false. This is not a very good method but I
don't see any other option. I guess whatever is the case logging
errors from tablesyncs is anyway not a bad idea.

Instead of using the syntax "ALTER SUBSCRIPTION name SET SKIP
TRANSACTION Iconst", isn't it better to use it as a subscription
option like Mark has done for his patch (disable_on_error)?

According to the doc, ALTER SUBSCRIPTION ... SET is used to alter
parameters originally set by CREATE SUBSCRIPTION. Therefore, we can
specify a subset of parameters that can be specified by CREATE
SUBSCRIPTION. It makes sense to me for 'disable_on_error' since it can
be specified by CREATE SUBSCRIPTION. Whereas SKIP TRANSACTION stuff
cannot be done. Are you concerned about adding a syntax to ALTER
SUBSCRIPTION?

I am slightly nervous about this way of allowing the user to skip the
errors because if it is not used carefully then it can easily lead to
inconsistent data on the subscriber. I agree that as only superusers
will be allowed to use this option and we can document clearly the
side-effects, the risk could be reduced but is that sufficient? It is
not that we don't have any other tool which allows users to make their
data inconsistent (one recent example is functions
(heap_force_kill/heap_force_freeze) in pg_surgery module) if not used
carefully but it might be better to not expose such tools.

OTOH, if we use the error infrastructure of this patch and allow users
to just disable the subscription on error as was proposed by Mark then
that can't lead to any inconsistency.

What do you think?

As you mentioned in another mail, what we can do with this feature is
the same as pg_replication_origin_advance(). Like there is a risk that
the user specifies a wrong LSN to pg_replication_origin_advance(),
there is a similar risk at this feature.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, Jul 6, 2021 at 11:28 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 5, 2021 at 7:33 PM Alexey Lesovsky <lesovsky@gmail.com> wrote:

Hi,
Have a few notes about pg_stat_logical_replication_error from the DBA point of view (which will use this view in the future).

Thank you for the comments!

1. As I understand it, this view might contain many errors related to different subscriptions. It is better to name "pg_stat_logical_replication_errors" using the plural form (like this done for stat views for tables, indexes, functions).

Agreed.

Also, I'd like to suggest thinking twice about the view name (and function used in view DDL) - "pg_stat_logical_replication_error" contains very common "logical replication" words, but the view contains errors related to subscriptions only. In the future there could be other kinds of errors related to logical replication, but not related to subscriptions - what will you do?

Is pg_stat_subscription_errors or
pg_stat_logical_replication_apply_errors better?

Few more to consider: pg_stat_apply_failures,
pg_stat_subscription_failures, pg_stat_apply_conflicts,
pg_stat_subscription_conflicts.

2. Add a field with database name or id - it helps to quickly understand to which database the subscription belongs.

Agreed.

3. Add a counter field with total number of errors - it helps to calculate errors rates and aggregations (sum), and don't lose information about errors between view checks.

Do you mean to increment the error count if the error (command, xid,
and relid) is the same as the previous one? or to have the total
number of errors per subscription?

I would prefer the total number of errors per subscription.

And what can we infer from the
error rates and aggregations?

Say, if we add a column like failure_type/conflict_type as well and
one would be interested in knowing how many conflicts are due to
primary key conflicts vs. update/delete conflicts.

You might want to consider keeping this view patch before the skip_xid
patch in your patch series as this will be base for the skip_xid
patch.

--
With Regards,
Amit Kapila.

On Tue, Jul 6, 2021 at 12:30 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 5, 2021 at 6:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Jul 1, 2021 at 6:31 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jul 1, 2021 at 12:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

Don't we want to clear stats at drop subscription as well? We do drop
database stats in dropdb via pgstat_drop_database, so I think we need
to clear subscription stats at the time of drop subscription.

Yes, it needs to be cleared. In the 0003 patch, pgstat_vacuum_stat()
sends the message to clear the stats. I think it's better to have
pgstat_vacuum_stat() do that job similar to dropping replication slot
statistics rather than relying on the single message send at DROP
SUBSCRIPTION. I've considered doing both: sending the message at DROP
SUBSCRIPTION and periodical checking by pgstat_vacuum_stat(), but
dropping subscription not setting a replication slot is able to
rollback. So we need to send it only at commit time. Given that we
don’t necessarily need the stats to be updated immediately, I think
it’s reasonable to go with only a way of pgstat_vacuum_stat().

Okay, that makes sense. Can we consider sending the multiple ids in
one message as we do for relations or functions in
pgstat_vacuum_stat()? That will reduce some message traffic.

Yes. Since subscriptions are objects that are not frequently created
and dropped I prioritized not to increase the message type. But if we
do that for subscriptions, is it better to do that for replication
slots as well? It seems to me that the lifetime of subscriptions and
replication slots are similar.

Yeah, I think it makes sense to do for both, we can work on slots
patch separately. I don't see a reason why we shouldn't send a single
message for multiple clear/drop entries.

True but I guess the user can wait for all the tablesyncs to either
finish or get an error corresponding to the table sync. After that, it
can use 'copy_data' as false. This is not a very good method but I
don't see any other option. I guess whatever is the case logging
errors from tablesyncs is anyway not a bad idea.

Instead of using the syntax "ALTER SUBSCRIPTION name SET SKIP
TRANSACTION Iconst", isn't it better to use it as a subscription
option like Mark has done for his patch (disable_on_error)?

According to the doc, ALTER SUBSCRIPTION ... SET is used to alter
parameters originally set by CREATE SUBSCRIPTION. Therefore, we can
specify a subset of parameters that can be specified by CREATE
SUBSCRIPTION. It makes sense to me for 'disable_on_error' since it can
be specified by CREATE SUBSCRIPTION. Whereas SKIP TRANSACTION stuff
cannot be done. Are you concerned about adding a syntax to ALTER
SUBSCRIPTION?

Both for additional syntax and consistency with disable_on_error.
Isn't it just a current implementation that Alter only allows to
change parameters supported by Create? Is there a reason why we can't
allow Alter to set/change some parameters not supported by Create?

--
With Regards,
Amit Kapila.

On Tue, Jul 6, 2021 at 10:58 AM Masahiko Sawada <sawada.mshk@gmail.com>
wrote:

Also, I'd like to suggest thinking twice about the view name (and

function used in view DDL) - "pg_stat_logical_replication_error" contains
very common "logical replication" words, but the view contains errors
related to subscriptions only. In the future there could be other kinds of
errors related to logical replication, but not related to subscriptions -
what will you do?

Is pg_stat_subscription_errors or
pg_stat_logical_replication_apply_errors better?

It seems to me 'pg_stat_subscription_conflicts' proposed by Amit Kapila is
the most suitable, because it directly says about conflicts occurring on
the subscription side. The name 'pg_stat_subscription_errors' is also good,
especially in case of further extension if some kind of similar errors will
be tracked.

3. Add a counter field with total number of errors - it helps to

calculate errors rates and aggregations (sum), and don't lose information
about errors between view checks.

Do you mean to increment the error count if the error (command, xid,
and relid) is the same as the previous one? or to have the total
number of errors per subscription? And what can we infer from the
error rates and aggregations?

To be honest, I hurried up when I wrote the first email, and read only
about stats view. Later, I read the starting email about the patch and
rethought this note.

As I understand, when the conflict occurs, replication stops (until
conflict is resolved), an error appears in the stats view. Now, no new
errors can occur in the blocked subscription. Hence, there are impossible
situations when many errors (like spikes) have occurred and a user didn't
see that. If I am correct in my assumption, there is no need for counters.
They are necessary only when errors might occur too frequently (like
pg_stat_database.deadlocks). But if this is possible, I would prefer the
total number of errors per subscription, as also proposed by Amit.

Under "error rates and aggregations" I also mean in the context of when a
high number of errors occured in a short period of time. If a user can
read the "total errors" counter and keep this metric in his monitoring
system, he will be able to calculate rates over time using functions in the
monitoring system. This is extremely useful.

I also would like to clarify, when conflict is resolved - the error record
is cleared or kept in the view? If it is cleared, the error counter is
required (because we don't want to lose all history of errors). If it is
kept - the flag telling about the error is resolved is needed (or set xid
to NULL). I mean when the user is watching the view, he should be able to
identify if the error has already been resolved or not.

--
Regards, Alexey

On Tue, Jul 6, 2021 at 10:58 AM Masahiko Sawada <sawada.mshk@gmail.com>
wrote:

On Mon, Jul 5, 2021 at 7:33 PM Alexey Lesovsky <lesovsky@gmail.com> wrote:

Hi,
Have a few notes about pg_stat_logical_replication_error from the DBA

point of view (which will use this view in the future).

Thank you for the comments!

1. As I understand it, this view might contain many errors related to

different subscriptions. It is better to name
"pg_stat_logical_replication_errors" using the plural form (like this done
for stat views for tables, indexes, functions).

Agreed.

Also, I'd like to suggest thinking twice about the view name (and

function used in view DDL) - "pg_stat_logical_replication_error" contains
very common "logical replication" words, but the view contains errors
related to subscriptions only. In the future there could be other kinds of
errors related to logical replication, but not related to subscriptions -
what will you do?

Is pg_stat_subscription_errors or
pg_stat_logical_replication_apply_errors better?

2. Add a field with database name or id - it helps to quickly understand

to which database the subscription belongs.

Agreed.

3. Add a counter field with total number of errors - it helps to

calculate errors rates and aggregations (sum), and don't lose information
about errors between view checks.

Do you mean to increment the error count if the error (command, xid,
and relid) is the same as the previous one? or to have the total
number of errors per subscription? And what can we infer from the
error rates and aggregations?

4. Add text of last error (if it will not be too expensive).

Agreed.

5. Rename the "action" field to "command", as I know this is right from

terminology point of view.

Okay.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

--
С уважением Алексей В. Лесовский

On Tue, Jul 6, 2021 at 6:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Jul 6, 2021 at 12:30 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 5, 2021 at 6:46 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Jul 1, 2021 at 6:31 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jul 1, 2021 at 12:56 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

Don't we want to clear stats at drop subscription as well? We do drop
database stats in dropdb via pgstat_drop_database, so I think we need
to clear subscription stats at the time of drop subscription.

Yes, it needs to be cleared. In the 0003 patch, pgstat_vacuum_stat()
sends the message to clear the stats. I think it's better to have
pgstat_vacuum_stat() do that job similar to dropping replication slot
statistics rather than relying on the single message send at DROP
SUBSCRIPTION. I've considered doing both: sending the message at DROP
SUBSCRIPTION and periodical checking by pgstat_vacuum_stat(), but
dropping subscription not setting a replication slot is able to
rollback. So we need to send it only at commit time. Given that we
don’t necessarily need the stats to be updated immediately, I think
it’s reasonable to go with only a way of pgstat_vacuum_stat().

Okay, that makes sense. Can we consider sending the multiple ids in
one message as we do for relations or functions in
pgstat_vacuum_stat()? That will reduce some message traffic.

Yes. Since subscriptions are objects that are not frequently created
and dropped I prioritized not to increase the message type. But if we
do that for subscriptions, is it better to do that for replication
slots as well? It seems to me that the lifetime of subscriptions and
replication slots are similar.

Yeah, I think it makes sense to do for both, we can work on slots
patch separately. I don't see a reason why we shouldn't send a single
message for multiple clear/drop entries.

+1

True but I guess the user can wait for all the tablesyncs to either
finish or get an error corresponding to the table sync. After that, it
can use 'copy_data' as false. This is not a very good method but I
don't see any other option. I guess whatever is the case logging
errors from tablesyncs is anyway not a bad idea.

Instead of using the syntax "ALTER SUBSCRIPTION name SET SKIP
TRANSACTION Iconst", isn't it better to use it as a subscription
option like Mark has done for his patch (disable_on_error)?

According to the doc, ALTER SUBSCRIPTION ... SET is used to alter
parameters originally set by CREATE SUBSCRIPTION. Therefore, we can
specify a subset of parameters that can be specified by CREATE
SUBSCRIPTION. It makes sense to me for 'disable_on_error' since it can
be specified by CREATE SUBSCRIPTION. Whereas SKIP TRANSACTION stuff
cannot be done. Are you concerned about adding a syntax to ALTER
SUBSCRIPTION?

Both for additional syntax and consistency with disable_on_error.
Isn't it just a current implementation that Alter only allows to
change parameters supported by Create? Is there a reason why we can't
allow Alter to set/change some parameters not supported by Create?

I think there is not reason for that but looking at ALTER TABLE I
thought there is such a policy. I thought the skipping transaction
feature is somewhat different from disable_on_error feature. The
former seems a feature to deal with a problem on the spot whereas the
latter seems a setting of a subscription. Anyway, if we use the
subscription option, we can reset the XID by setting 0? Or do we need
ALTER SUBSCRIPTION RESET?

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Wed, Jul 7, 2021 at 11:47 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Jul 6, 2021 at 6:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

According to the doc, ALTER SUBSCRIPTION ... SET is used to alter
parameters originally set by CREATE SUBSCRIPTION. Therefore, we can
specify a subset of parameters that can be specified by CREATE
SUBSCRIPTION. It makes sense to me for 'disable_on_error' since it can
be specified by CREATE SUBSCRIPTION. Whereas SKIP TRANSACTION stuff
cannot be done. Are you concerned about adding a syntax to ALTER
SUBSCRIPTION?

Both for additional syntax and consistency with disable_on_error.
Isn't it just a current implementation that Alter only allows to
change parameters supported by Create? Is there a reason why we can't
allow Alter to set/change some parameters not supported by Create?

I think there is not reason for that but looking at ALTER TABLE I
thought there is such a policy.

If we are looking for precedent then I think we allow to set
configuration parameters via Alter Database but not via Create
Database. Does that address your concern?

I thought the skipping transaction
feature is somewhat different from disable_on_error feature. The
former seems a feature to deal with a problem on the spot whereas the
latter seems a setting of a subscription. Anyway, if we use the
subscription option, we can reset the XID by setting 0? Or do we need
ALTER SUBSCRIPTION RESET?

The other commands like Alter Table, Alter Database, etc, which
provides a way to Set some parameter/option, have a Reset variant. I
think it would be good to have it for Alter Subscription as well but
we might want to allow other parameters to be reset by that as well.

--
With Regards,
Amit Kapila.

On Thu, Jul 8, 2021 at 6:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Jul 7, 2021 at 11:47 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Jul 6, 2021 at 6:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

According to the doc, ALTER SUBSCRIPTION ... SET is used to alter
parameters originally set by CREATE SUBSCRIPTION. Therefore, we can
specify a subset of parameters that can be specified by CREATE
SUBSCRIPTION. It makes sense to me for 'disable_on_error' since it can
be specified by CREATE SUBSCRIPTION. Whereas SKIP TRANSACTION stuff
cannot be done. Are you concerned about adding a syntax to ALTER
SUBSCRIPTION?

Both for additional syntax and consistency with disable_on_error.
Isn't it just a current implementation that Alter only allows to
change parameters supported by Create? Is there a reason why we can't
allow Alter to set/change some parameters not supported by Create?

I think there is not reason for that but looking at ALTER TABLE I
thought there is such a policy.

If we are looking for precedent then I think we allow to set
configuration parameters via Alter Database but not via Create
Database. Does that address your concern?

Thank you for the info! But it seems like CREATE DATABASE doesn't
support SET in the first place. Also interestingly, ALTER SUBSCRIPTION
support both ENABLE/DISABLE and SET (enabled = on/off). I’m not sure
from the point of view of consistency with other CREATE, ALTER
commands, and disable_on_error but it might be better to avoid adding
additional syntax.

I thought the skipping transaction
feature is somewhat different from disable_on_error feature. The
former seems a feature to deal with a problem on the spot whereas the
latter seems a setting of a subscription. Anyway, if we use the
subscription option, we can reset the XID by setting 0? Or do we need
ALTER SUBSCRIPTION RESET?

The other commands like Alter Table, Alter Database, etc, which
provides a way to Set some parameter/option, have a Reset variant. I
think it would be good to have it for Alter Subscription as well but
we might want to allow other parameters to be reset by that as well.

Agreed.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, Jul 6, 2021 at 7:13 PM Alexey Lesovsky <lesovsky@gmail.com> wrote:

On Tue, Jul 6, 2021 at 10:58 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Also, I'd like to suggest thinking twice about the view name (and function used in view DDL) - "pg_stat_logical_replication_error" contains very common "logical replication" words, but the view contains errors related to subscriptions only. In the future there could be other kinds of errors related to logical replication, but not related to subscriptions - what will you do?

Is pg_stat_subscription_errors or
pg_stat_logical_replication_apply_errors better?

It seems to me 'pg_stat_subscription_conflicts' proposed by Amit Kapila is the most suitable, because it directly says about conflicts occurring on the subscription side. The name 'pg_stat_subscription_errors' is also good, especially in case of further extension if some kind of similar errors will be tracked.

I personally prefer pg_stat_subscription_errors since
pg_stat_subscription_conflicts could be used for conflict resolution
features in the future. This stats view I'm proposing is meant to
focus on errors that happened during applying logical changes. So
using the term 'errors' seems to make sense to me.

3. Add a counter field with total number of errors - it helps to calculate errors rates and aggregations (sum), and don't lose information about errors between view checks.

Do you mean to increment the error count if the error (command, xid,
and relid) is the same as the previous one? or to have the total
number of errors per subscription? And what can we infer from the
error rates and aggregations?

To be honest, I hurried up when I wrote the first email, and read only about stats view. Later, I read the starting email about the patch and rethought this note.

As I understand, when the conflict occurs, replication stops (until conflict is resolved), an error appears in the stats view. Now, no new errors can occur in the blocked subscription. Hence, there are impossible situations when many errors (like spikes) have occurred and a user didn't see that. If I am correct in my assumption, there is no need for counters. They are necessary only when errors might occur too frequently (like pg_stat_database.deadlocks). But if this is possible, I would prefer the total number of errors per subscription, as also proposed by Amit.

Yeah, the total number of errors seems better.

Under "error rates and aggregations" I also mean in the context of when a high number of errors occured in a short period of time. If a user can read the "total errors" counter and keep this metric in his monitoring system, he will be able to calculate rates over time using functions in the monitoring system. This is extremely useful.

Thanks for your explanation. Agreed. But the rate depends on
wal_retrieve_retry_interval so is not likely to be high in practice.

I also would like to clarify, when conflict is resolved - the error record is cleared or kept in the view? If it is cleared, the error counter is required (because we don't want to lose all history of errors). If it is kept - the flag telling about the error is resolved is needed (or set xid to NULL). I mean when the user is watching the view, he should be able to identify if the error has already been resolved or not.

With the current patch, once the conflict is resolved by skipping the
transaction in question, its entry on the stats view is cleared. As
you suggested, if we have the total error counts in that view, it
would be good to keep the count and clear other fields such as xid,
last_failure, and command etc.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Fri, Jul 9, 2021 at 5:43 AM Masahiko Sawada <sawada.mshk@gmail.com>
wrote:

On Tue, Jul 6, 2021 at 7:13 PM Alexey Lesovsky <lesovsky@gmail.com> wrote:

On Tue, Jul 6, 2021 at 10:58 AM Masahiko Sawada <sawada.mshk@gmail.com>

wrote:

Also, I'd like to suggest thinking twice about the view name (and

function used in view DDL) - "pg_stat_logical_replication_error" contains
very common "logical replication" words, but the view contains errors
related to subscriptions only. In the future there could be other kinds of
errors related to logical replication, but not related to subscriptions -
what will you do?

Is pg_stat_subscription_errors or
pg_stat_logical_replication_apply_errors better?

It seems to me 'pg_stat_subscription_conflicts' proposed by Amit Kapila

is the most suitable, because it directly says about conflicts occurring on
the subscription side. The name 'pg_stat_subscription_errors' is also good,
especially in case of further extension if some kind of similar errors will
be tracked.

I personally prefer pg_stat_subscription_errors since
pg_stat_subscription_conflicts could be used for conflict resolution
features in the future. This stats view I'm proposing is meant to
focus on errors that happened during applying logical changes. So
using the term 'errors' seems to make sense to me.

Agreed

3. Add a counter field with total number of errors - it helps to

calculate errors rates and aggregations (sum), and don't lose information
about errors between view checks.

Do you mean to increment the error count if the error (command, xid,
and relid) is the same as the previous one? or to have the total
number of errors per subscription? And what can we infer from the
error rates and aggregations?

To be honest, I hurried up when I wrote the first email, and read only

about stats view. Later, I read the starting email about the patch and
rethought this note.

As I understand, when the conflict occurs, replication stops (until

conflict is resolved), an error appears in the stats view. Now, no new
errors can occur in the blocked subscription. Hence, there are impossible
situations when many errors (like spikes) have occurred and a user didn't
see that. If I am correct in my assumption, there is no need for counters.
They are necessary only when errors might occur too frequently (like
pg_stat_database.deadlocks). But if this is possible, I would prefer the
total number of errors per subscription, as also proposed by Amit.

Yeah, the total number of errors seems better.

Agreed

Under "error rates and aggregations" I also mean in the context of when

a high number of errors occured in a short period of time. If a user can
read the "total errors" counter and keep this metric in his monitoring
system, he will be able to calculate rates over time using functions in the
monitoring system. This is extremely useful.

Thanks for your explanation. Agreed. But the rate depends on
wal_retrieve_retry_interval so is not likely to be high in practice.

Agreed

I also would like to clarify, when conflict is resolved - the error

record is cleared or kept in the view? If it is cleared, the error counter
is required (because we don't want to lose all history of errors). If it is
kept - the flag telling about the error is resolved is needed (or set xid
to NULL). I mean when the user is watching the view, he should be able to
identify if the error has already been resolved or not.

With the current patch, once the conflict is resolved by skipping the
transaction in question, its entry on the stats view is cleared. As
you suggested, if we have the total error counts in that view, it
would be good to keep the count and clear other fields such as xid,
last_failure, and command etc.

Ok, looks nice. But I am curious how this will work in the case when there
are two (or more) errors in the same subscription, but different relations?
After resolution all these records are kept or they will be merged into a
single record (because subscription was the same for all errors)?

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

--
Regards, Alexey Lesovsky

On Fri, Jul 9, 2021 at 5:58 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jul 8, 2021 at 6:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Jul 7, 2021 at 11:47 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Jul 6, 2021 at 6:33 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

According to the doc, ALTER SUBSCRIPTION ... SET is used to alter
parameters originally set by CREATE SUBSCRIPTION. Therefore, we can
specify a subset of parameters that can be specified by CREATE
SUBSCRIPTION. It makes sense to me for 'disable_on_error' since it can
be specified by CREATE SUBSCRIPTION. Whereas SKIP TRANSACTION stuff
cannot be done. Are you concerned about adding a syntax to ALTER
SUBSCRIPTION?

Both for additional syntax and consistency with disable_on_error.
Isn't it just a current implementation that Alter only allows to
change parameters supported by Create? Is there a reason why we can't
allow Alter to set/change some parameters not supported by Create?

I think there is not reason for that but looking at ALTER TABLE I
thought there is such a policy.

If we are looking for precedent then I think we allow to set
configuration parameters via Alter Database but not via Create
Database. Does that address your concern?

Thank you for the info! But it seems like CREATE DATABASE doesn't
support SET in the first place. Also interestingly, ALTER SUBSCRIPTION
support both ENABLE/DISABLE and SET (enabled = on/off).

I think that is redundant but not sure if there is any reason behind doing so.

I’m not sure
from the point of view of consistency with other CREATE, ALTER
commands, and disable_on_error but it might be better to avoid adding
additional syntax.

If we can avoid introducing new syntax that in itself is a good reason
to introduce it as an option.

--
With Regards,
Amit Kapila.

On Fri, Jul 9, 2021 at 9:02 AM Alexey Lesovsky <lesovsky@gmail.com> wrote:

On Fri, Jul 9, 2021 at 5:43 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I also would like to clarify, when conflict is resolved - the error record is cleared or kept in the view? If it is cleared, the error counter is required (because we don't want to lose all history of errors). If it is kept - the flag telling about the error is resolved is needed (or set xid to NULL). I mean when the user is watching the view, he should be able to identify if the error has already been resolved or not.

With the current patch, once the conflict is resolved by skipping the
transaction in question, its entry on the stats view is cleared. As
you suggested, if we have the total error counts in that view, it
would be good to keep the count and clear other fields such as xid,
last_failure, and command etc.

Ok, looks nice. But I am curious how this will work in the case when there are two (or more) errors in the same subscription, but different relations?

We can't proceed unless the first error is resolved, so there
shouldn't be multiple unresolved errors. However, there is an
exception to it which is during initial table sync and I think the
view should have separate rows for each table sync.

--
With Regards,
Amit Kapila.

On Mon, Jul 12, 2021 at 8:36 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

Ok, looks nice. But I am curious how this will work in the case when

there are two (or more) errors in the same subscription, but different
relations?

We can't proceed unless the first error is resolved, so there
shouldn't be multiple unresolved errors.

Ok. I thought multiple errors are possible when many tables are initialized
using parallel workers (with max_sync_workers_per_subscription > 1).

--
Regards, Alexey

On Mon, Jul 12, 2021 at 9:37 AM Alexey Lesovsky <lesovsky@gmail.com> wrote:

On Mon, Jul 12, 2021 at 8:36 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

Ok, looks nice. But I am curious how this will work in the case when there are two (or more) errors in the same subscription, but different relations?

We can't proceed unless the first error is resolved, so there
shouldn't be multiple unresolved errors.

Ok. I thought multiple errors are possible when many tables are initialized using parallel workers (with max_sync_workers_per_subscription > 1).

Yeah, that is possible but that covers under the second condition
mentioned by me and in such cases I think we should have separate rows
for each tablesync. Is that right, Sawada-san or do you have something
else in mind?

--
With Regards,
Amit Kapila.

On Mon, Jul 12, 2021 at 1:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Jul 12, 2021 at 9:37 AM Alexey Lesovsky <lesovsky@gmail.com> wrote:

On Mon, Jul 12, 2021 at 8:36 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

Ok, looks nice. But I am curious how this will work in the case when there are two (or more) errors in the same subscription, but different relations?

We can't proceed unless the first error is resolved, so there
shouldn't be multiple unresolved errors.

Ok. I thought multiple errors are possible when many tables are initialized using parallel workers (with max_sync_workers_per_subscription > 1).

Yeah, that is possible but that covers under the second condition
mentioned by me and in such cases I think we should have separate rows
for each tablesync. Is that right, Sawada-san or do you have something
else in mind?

Yeah, I agree to have separate rows for each table sync. The table
should not be processed by both the table sync worker and the apply
worker at a time so the pair of subscription OID and relation OID will
be unique. I think that we have a boolean column in the view,
indicating whether the error entry is reported by the table sync
worker or the apply worker, or maybe we also can have the action
column show "TABLE SYNC" if the error is reported by the table sync
worker.

When it comes to removing the subscription errors in
pgstat_vacuum_stat(), I think we need to seq scan on the hash table
and send the messages to purge the subscription error entries.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Mon, Jul 12, 2021 at 11:13 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 12, 2021 at 1:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Jul 12, 2021 at 9:37 AM Alexey Lesovsky <lesovsky@gmail.com> wrote:

On Mon, Jul 12, 2021 at 8:36 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

Ok, looks nice. But I am curious how this will work in the case when there are two (or more) errors in the same subscription, but different relations?

We can't proceed unless the first error is resolved, so there
shouldn't be multiple unresolved errors.

Ok. I thought multiple errors are possible when many tables are initialized using parallel workers (with max_sync_workers_per_subscription > 1).

Yeah, that is possible but that covers under the second condition
mentioned by me and in such cases I think we should have separate rows
for each tablesync. Is that right, Sawada-san or do you have something
else in mind?

Yeah, I agree to have separate rows for each table sync. The table
should not be processed by both the table sync worker and the apply
worker at a time so the pair of subscription OID and relation OID will
be unique. I think that we have a boolean column in the view,
indicating whether the error entry is reported by the table sync
worker or the apply worker, or maybe we also can have the action
column show "TABLE SYNC" if the error is reported by the table sync
worker.

Or similar to backend_type (text) in pg_stat_activity, we can have
something like error_source (text) which will display apply worker or
tablesync worker? I think if we have this column then even if there is
a chance that both apply and sync worker operates on the same
relation, we can identify it via this column.

--
With Regards,
Amit Kapila.

On Mon, Jul 12, 2021 at 8:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Jul 12, 2021 at 11:13 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 12, 2021 at 1:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Jul 12, 2021 at 9:37 AM Alexey Lesovsky <lesovsky@gmail.com> wrote:

On Mon, Jul 12, 2021 at 8:36 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

Ok, looks nice. But I am curious how this will work in the case when there are two (or more) errors in the same subscription, but different relations?

We can't proceed unless the first error is resolved, so there
shouldn't be multiple unresolved errors.

Ok. I thought multiple errors are possible when many tables are initialized using parallel workers (with max_sync_workers_per_subscription > 1).

Yeah, that is possible but that covers under the second condition
mentioned by me and in such cases I think we should have separate rows
for each tablesync. Is that right, Sawada-san or do you have something
else in mind?

Yeah, I agree to have separate rows for each table sync. The table
should not be processed by both the table sync worker and the apply
worker at a time so the pair of subscription OID and relation OID will
be unique. I think that we have a boolean column in the view,
indicating whether the error entry is reported by the table sync
worker or the apply worker, or maybe we also can have the action
column show "TABLE SYNC" if the error is reported by the table sync
worker.

Or similar to backend_type (text) in pg_stat_activity, we can have
something like error_source (text) which will display apply worker or
tablesync worker? I think if we have this column then even if there is
a chance that both apply and sync worker operates on the same
relation, we can identify it via this column.

Sounds good. I'll incorporate this in the next version patch that I'm
planning to submit this week.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Wed, Jul 14, 2021 at 5:14 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 12, 2021 at 8:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Jul 12, 2021 at 11:13 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 12, 2021 at 1:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Jul 12, 2021 at 9:37 AM Alexey Lesovsky <lesovsky@gmail.com> wrote:

On Mon, Jul 12, 2021 at 8:36 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

Ok, looks nice. But I am curious how this will work in the case when there are two (or more) errors in the same subscription, but different relations?

We can't proceed unless the first error is resolved, so there
shouldn't be multiple unresolved errors.

Ok. I thought multiple errors are possible when many tables are initialized using parallel workers (with max_sync_workers_per_subscription > 1).

Yeah, that is possible but that covers under the second condition
mentioned by me and in such cases I think we should have separate rows
for each tablesync. Is that right, Sawada-san or do you have something
else in mind?

Yeah, I agree to have separate rows for each table sync. The table
should not be processed by both the table sync worker and the apply
worker at a time so the pair of subscription OID and relation OID will
be unique. I think that we have a boolean column in the view,
indicating whether the error entry is reported by the table sync
worker or the apply worker, or maybe we also can have the action
column show "TABLE SYNC" if the error is reported by the table sync
worker.

Or similar to backend_type (text) in pg_stat_activity, we can have
something like error_source (text) which will display apply worker or
tablesync worker? I think if we have this column then even if there is
a chance that both apply and sync worker operates on the same
relation, we can identify it via this column.

Sounds good. I'll incorporate this in the next version patch that I'm
planning to submit this week.

Sorry, I could not make it this week. I'll submit them early next week.
While updating the patch I thought we need to have more design
discussion on two points of clearing error details after the error is
resolved:

1. How to clear apply worker errors. IIUC we've discussed that once
the apply worker skipped the transaction we leave the error entry
itself but clear its fields except for some fields such as failure
counts. But given that the stats messages could be lost, how can we
ensure to clear those error details? For table sync workers’ error, we
can have autovacuum workers periodically check entires of
pg_subscription_rel and clear the error entry if the table sync worker
completes table sync (i.g., checking if srsubstate = ‘r’). But there
is no such information for the apply workers and subscriptions. In
addition to sending the message clearing the error details just after
skipping the transaction, I thought that we can have apply workers
periodically send the message clearing the error details but it seems
not good.

2. Do we really want to leave the table sync worker even after the
error is resolved and the table sync completes? Unlike the apply
worker error, the number of table sync worker errors could be very
large, for example, if a subscriber subscribes to many tables. If we
leave those errors in the stats view, it uses more memory space and
could affect writing and reading stats file performance. If such left
table sync error entries are not helpful in practice I think we can
remove them rather than clear some fields. What do you think?

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Fri, Jul 16, 2021 at 8:33 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Jul 14, 2021 at 5:14 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Sounds good. I'll incorporate this in the next version patch that I'm
planning to submit this week.

Sorry, I could not make it this week. I'll submit them early next week.

No problem.

While updating the patch I thought we need to have more design
discussion on two points of clearing error details after the error is
resolved:

1. How to clear apply worker errors. IIUC we've discussed that once
the apply worker skipped the transaction we leave the error entry
itself but clear its fields except for some fields such as failure
counts. But given that the stats messages could be lost, how can we
ensure to clear those error details? For table sync workers’ error, we
can have autovacuum workers periodically check entires of
pg_subscription_rel and clear the error entry if the table sync worker
completes table sync (i.g., checking if srsubstate = ‘r’). But there
is no such information for the apply workers and subscriptions.

But won't the corresponding subscription (pg_subscription) have the
XID as InvalidTransactionid once the xid is skipped or at least a
different XID then we would have in pg_stat view? Can we use that to
reset entry via vacuum?

In
addition to sending the message clearing the error details just after
skipping the transaction, I thought that we can have apply workers
periodically send the message clearing the error details but it seems
not good.

Yeah, such things should be a last resort.

2. Do we really want to leave the table sync worker even after the
error is resolved and the table sync completes? Unlike the apply
worker error, the number of table sync worker errors could be very
large, for example, if a subscriber subscribes to many tables. If we
leave those errors in the stats view, it uses more memory space and
could affect writing and reading stats file performance. If such left
table sync error entries are not helpful in practice I think we can
remove them rather than clear some fields. What do you think?

Sounds reasonable to me. One might think to update the subscription
error count by including table_sync errors but not sure if that is
helpful and even if that is helpful, we can extend it later.

--
With Regards,
Amit Kapila.

On Mon, Jul 19, 2021 at 2:22 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, Jul 16, 2021 at 8:33 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Jul 14, 2021 at 5:14 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Sounds good. I'll incorporate this in the next version patch that I'm
planning to submit this week.

Sorry, I could not make it this week. I'll submit them early next week.

No problem.

While updating the patch I thought we need to have more design
discussion on two points of clearing error details after the error is
resolved:

1. How to clear apply worker errors. IIUC we've discussed that once
the apply worker skipped the transaction we leave the error entry
itself but clear its fields except for some fields such as failure
counts. But given that the stats messages could be lost, how can we
ensure to clear those error details? For table sync workers’ error, we
can have autovacuum workers periodically check entires of
pg_subscription_rel and clear the error entry if the table sync worker
completes table sync (i.g., checking if srsubstate = ‘r’). But there
is no such information for the apply workers and subscriptions.

But won't the corresponding subscription (pg_subscription) have the
XID as InvalidTransactionid once the xid is skipped or at least a
different XID then we would have in pg_stat view? Can we use that to
reset entry via vacuum?

I think the XID is InvalidTransaction until the user specifies it. So
I think we cannot know whether we're before skipping or after skipping
only by the transaction ID. No?

In
addition to sending the message clearing the error details just after
skipping the transaction, I thought that we can have apply workers
periodically send the message clearing the error details but it seems
not good.

Yeah, such things should be a last resort.

2. Do we really want to leave the table sync worker even after the
error is resolved and the table sync completes? Unlike the apply
worker error, the number of table sync worker errors could be very
large, for example, if a subscriber subscribes to many tables. If we
leave those errors in the stats view, it uses more memory space and
could affect writing and reading stats file performance. If such left
table sync error entries are not helpful in practice I think we can
remove them rather than clear some fields. What do you think?

Sounds reasonable to me. One might think to update the subscription
error count by including table_sync errors but not sure if that is
helpful and even if that is helpful, we can extend it later.

Agreed.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Sat, Jul 17, 2021 at 12:02 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Jul 14, 2021 at 5:14 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 12, 2021 at 8:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Jul 12, 2021 at 11:13 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 12, 2021 at 1:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Jul 12, 2021 at 9:37 AM Alexey Lesovsky <lesovsky@gmail.com> wrote:

On Mon, Jul 12, 2021 at 8:36 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

Ok, looks nice. But I am curious how this will work in the case when there are two (or more) errors in the same subscription, but different relations?

We can't proceed unless the first error is resolved, so there
shouldn't be multiple unresolved errors.

Ok. I thought multiple errors are possible when many tables are initialized using parallel workers (with max_sync_workers_per_subscription > 1).

Yeah, that is possible but that covers under the second condition
mentioned by me and in such cases I think we should have separate rows
for each tablesync. Is that right, Sawada-san or do you have something
else in mind?

Yeah, I agree to have separate rows for each table sync. The table
should not be processed by both the table sync worker and the apply
worker at a time so the pair of subscription OID and relation OID will
be unique. I think that we have a boolean column in the view,
indicating whether the error entry is reported by the table sync
worker or the apply worker, or maybe we also can have the action
column show "TABLE SYNC" if the error is reported by the table sync
worker.

Or similar to backend_type (text) in pg_stat_activity, we can have
something like error_source (text) which will display apply worker or
tablesync worker? I think if we have this column then even if there is
a chance that both apply and sync worker operates on the same
relation, we can identify it via this column.

Sounds good. I'll incorporate this in the next version patch that I'm
planning to submit this week.

Sorry, I could not make it this week. I'll submit them early next week.
While updating the patch I thought we need to have more design
discussion on two points of clearing error details after the error is
resolved:

1. How to clear apply worker errors. IIUC we've discussed that once
the apply worker skipped the transaction we leave the error entry
itself but clear its fields except for some fields such as failure
counts. But given that the stats messages could be lost, how can we
ensure to clear those error details? For table sync workers’ error, we
can have autovacuum workers periodically check entires of
pg_subscription_rel and clear the error entry if the table sync worker
completes table sync (i.g., checking if srsubstate = ‘r’). But there
is no such information for the apply workers and subscriptions. In
addition to sending the message clearing the error details just after
skipping the transaction, I thought that we can have apply workers
periodically send the message clearing the error details but it seems
not good.

I think that the motivation behind the idea of leaving error entries
and clearing theirs some fields is that users can check if the error
is successfully resolved and the worker is working find. But we can
check it also in another way, for example, checking
pg_stat_subscription view. So is it worth considering leaving the
apply worker errors as they are?

2. Do we really want to leave the table sync worker even after the
error is resolved and the table sync completes? Unlike the apply
worker error, the number of table sync worker errors could be very
large, for example, if a subscriber subscribes to many tables. If we
leave those errors in the stats view, it uses more memory space and
could affect writing and reading stats file performance. If such left
table sync error entries are not helpful in practice I think we can
remove them rather than clear some fields. What do you think?

I've attached the updated version patch that incorporated all comments
I got so far except for the clearing error details part I mentioned
above. After getting a consensus on those parts, I'll incorporate the
idea into the patches.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Mon, Jul 19, 2021 at 12:10 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Sat, Jul 17, 2021 at 12:02 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

1. How to clear apply worker errors. IIUC we've discussed that once
the apply worker skipped the transaction we leave the error entry
itself but clear its fields except for some fields such as failure
counts. But given that the stats messages could be lost, how can we
ensure to clear those error details? For table sync workers’ error, we
can have autovacuum workers periodically check entires of
pg_subscription_rel and clear the error entry if the table sync worker
completes table sync (i.g., checking if srsubstate = ‘r’). But there
is no such information for the apply workers and subscriptions. In
addition to sending the message clearing the error details just after
skipping the transaction, I thought that we can have apply workers
periodically send the message clearing the error details but it seems
not good.

I think that the motivation behind the idea of leaving error entries
and clearing theirs some fields is that users can check if the error
is successfully resolved and the worker is working find. But we can
check it also in another way, for example, checking
pg_stat_subscription view. So is it worth considering leaving the
apply worker errors as they are?

I think so. Basically, we will send the clear message after skipping
the exact but I think it is fine if that message is lost. At worst, it
will be displayed as the last error details. If there is another error
it will be overwritten or probably we should have a function *_reset()
which allows the user to reset a particular subscription's error info.

--
With Regards,
Amit Kapila.

On July 19, 2021 2:40 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the updated version patch that incorporated all comments
I got so far except for the clearing error details part I mentioned
above. After getting a consensus on those parts, I'll incorporate the
idea into the patches.

Hi Sawada-san,

I am interested in this feature.
After having a look at the patch, I have a few questions about it.
(Sorry in advance if I missed something)

1) In 0002 patch, it introduces a new view called pg_stat_subscription_errors.
Since it won't be cleaned automatically after we resolve the conflict, do we
need a reset function to clean the statistics in it ? Maybe something
similar to pg_stat_reset_replication_slot which clean the
pg_stat_replication_slots.

2) For 0003 patch, When I am faced with a conflict, I set skip_xid = xxx, and
then I resolve the conflict. If I reset skip_xid after resolving the
conflict, will the change(which cause the conflict before) be applied again ?

3) For 0003 patch, if user set skip_xid to a wrong xid which have not been
assigned, and then will the change be skipped when the xid is assigned in
the future even if it doesn't cause any conflicts ?

Besides, It might be better to add some description of patch in each patch's
commit message which will make it easier for new reviewers to follow.

Best regards,
Houzj

On Mon, Jul 19, 2021 at 5:47 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Jul 19, 2021 at 12:10 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Sat, Jul 17, 2021 at 12:02 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

1. How to clear apply worker errors. IIUC we've discussed that once
the apply worker skipped the transaction we leave the error entry
itself but clear its fields except for some fields such as failure
counts. But given that the stats messages could be lost, how can we
ensure to clear those error details? For table sync workers’ error, we
can have autovacuum workers periodically check entires of
pg_subscription_rel and clear the error entry if the table sync worker
completes table sync (i.g., checking if srsubstate = ‘r’). But there
is no such information for the apply workers and subscriptions. In
addition to sending the message clearing the error details just after
skipping the transaction, I thought that we can have apply workers
periodically send the message clearing the error details but it seems
not good.

I think that the motivation behind the idea of leaving error entries
and clearing theirs some fields is that users can check if the error
is successfully resolved and the worker is working find. But we can
check it also in another way, for example, checking
pg_stat_subscription view. So is it worth considering leaving the
apply worker errors as they are?

I think so. Basically, we will send the clear message after skipping
the exact but I think it is fine if that message is lost. At worst, it
will be displayed as the last error details. If there is another error
it will be overwritten or probably we should have a function *_reset()
which allows the user to reset a particular subscription's error info.

That makes sense. I'll incorporate this idea in the next version patch.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Mon, Jul 19, 2021 at 8:38 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

On July 19, 2021 2:40 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the updated version patch that incorporated all comments
I got so far except for the clearing error details part I mentioned
above. After getting a consensus on those parts, I'll incorporate the
idea into the patches.

Hi Sawada-san,

I am interested in this feature.
After having a look at the patch, I have a few questions about it.

Thank you for having a look at the patches!

1) In 0002 patch, it introduces a new view called pg_stat_subscription_errors.
Since it won't be cleaned automatically after we resolve the conflict, do we
need a reset function to clean the statistics in it ? Maybe something
similar to pg_stat_reset_replication_slot which clean the
pg_stat_replication_slots.

Agreed. As Amit also mentioned, providing a reset function to clean
the statistics seems a good idea. If the message clearing the stats
that is sent after skipping the transaction gets lost, the user is
able to reset those stats manually.

2) For 0003 patch, When I am faced with a conflict, I set skip_xid = xxx, and
then I resolve the conflict. If I reset skip_xid after resolving the
conflict, will the change(which cause the conflict before) be applied again ?

The apply worker checks skip_xid when it reads the subscription.
Therefore, if you reset skip_xid before the apply worker restarts and
skips the transaction, the change is applied. But if you reset
skip_xid after the apply worker skips transaction, the change is
already skipped and your resetting skip_xid has no effect.

3) For 0003 patch, if user set skip_xid to a wrong xid which have not been
assigned, and then will the change be skipped when the xid is assigned in
the future even if it doesn't cause any conflicts ?

Yes. Currently, setting a correct xid is the user's responsibility. I
think it would be better to disable it or emit WARNING/ERROR when the
user mistakenly set the wrong xid if we find out a convenient way to
detect that.

Besides, It might be better to add some description of patch in each patch's
commit message which will make it easier for new reviewers to follow.

I'll add commit messages in the next version patch.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, Jul 20, 2021 at 6:56 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 19, 2021 at 8:38 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

3) For 0003 patch, if user set skip_xid to a wrong xid which have not been
assigned, and then will the change be skipped when the xid is assigned in
the future even if it doesn't cause any conflicts ?

Yes. Currently, setting a correct xid is the user's responsibility. I
think it would be better to disable it or emit WARNING/ERROR when the
user mistakenly set the wrong xid if we find out a convenient way to
detect that.

I think in this regard we should clearly document how this can be
misused by users. I see that you have mentioned about skip_xid but
maybe we can add more on how it could lead to skipping a
non-conflicting XID and can lead to an inconsistent replica. As
discussed earlier as well, users can anyway do similar harm by using
pg_replication_slot_advance(). I think if possible we might want to
give some examples as well where it would be helpful for users to use
this functionality.

--
With Regards,
Amit Kapila.

On July 20, 2021 9:26 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 19, 2021 at 8:38 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

On July 19, 2021 2:40 PM Masahiko Sawada <sawada.mshk@gmail.com>

wrote:

I've attached the updated version patch that incorporated all
comments I got so far except for the clearing error details part I
mentioned above. After getting a consensus on those parts, I'll
incorporate the idea into the patches.

3) For 0003 patch, if user set skip_xid to a wrong xid which have not been
assigned, and then will the change be skipped when the xid is assigned in
the future even if it doesn't cause any conflicts ?

Yes. Currently, setting a correct xid is the user's responsibility. I think it would
be better to disable it or emit WARNING/ERROR when the user mistakenly set
the wrong xid if we find out a convenient way to detect that.

Thanks for the explanation. As Amit suggested, it seems we can document the
risk of misusing skip_xid. Besides, I found some minor things in the patch.

1) In 0002 patch

+ */
+static void
+pgstat_recv_subscription_purge(PgStat_MsgSubscriptionPurge *msg, int len)
+{
+	if (subscriptionErrHash != NULL)
+		return;
+

+static void
+pgstat_recv_subscription_error(PgStat_MsgSubscriptionErr *msg, int len)
+{

the second paramater "len" seems not used in the function
pgstat_recv_subscription_purge() and pgstat_recv_subscription_error().

2) in 0003 patch

  * Helper function for apply_handle_commit and apply_handle_stream_commit.
  */
 static void
-apply_handle_commit_internal(StringInfo s, LogicalRepCommitData *commit_data)
+apply_handle_commit_internal(LogicalRepCommitData *commit_data)
 {

This looks like a separate change which remove unused paramater in existing
code, maybe we can get this committed first ?

Best regards,
Houzj

On Thu, Jul 22, 2021 at 8:53 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

On July 20, 2021 9:26 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 19, 2021 at 8:38 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

On July 19, 2021 2:40 PM Masahiko Sawada <sawada.mshk@gmail.com>

wrote:

I've attached the updated version patch that incorporated all
comments I got so far except for the clearing error details part I
mentioned above. After getting a consensus on those parts, I'll
incorporate the idea into the patches.

3) For 0003 patch, if user set skip_xid to a wrong xid which have not been
assigned, and then will the change be skipped when the xid is assigned in
the future even if it doesn't cause any conflicts ?

Yes. Currently, setting a correct xid is the user's responsibility. I think it would
be better to disable it or emit WARNING/ERROR when the user mistakenly set
the wrong xid if we find out a convenient way to detect that.

Thanks for the explanation. As Amit suggested, it seems we can document the
risk of misusing skip_xid. Besides, I found some minor things in the patch.

1) In 0002 patch

+ */
+static void
+pgstat_recv_subscription_purge(PgStat_MsgSubscriptionPurge *msg, int len)
+{
+       if (subscriptionErrHash != NULL)
+               return;
+

+static void
+pgstat_recv_subscription_error(PgStat_MsgSubscriptionErr *msg, int len)
+{

the second paramater "len" seems not used in the function
pgstat_recv_subscription_purge() and pgstat_recv_subscription_error().

'len' is not used at all in not only functions the patch added but
also other pgstat_recv_* functions. Can we remove all of them in a
separate patch? 'len' in pgstat_recv_* functions has never been used
since the stats collector code is introduced. It seems like that it
was mistakenly introduced in the first commit and other pgstat_recv_*
functions were added that followed it to define ‘len’ but didn’t also
use it at all.

2) in 0003 patch

* Helper function for apply_handle_commit and apply_handle_stream_commit.
*/
static void
-apply_handle_commit_internal(StringInfo s, LogicalRepCommitData *commit_data)
+apply_handle_commit_internal(LogicalRepCommitData *commit_data)
{

This looks like a separate change which remove unused paramater in existing
code, maybe we can get this committed first ?

Yeah, it seems to be introduced by commit 0926e96c493. I've attached
the patch for that.

Also, I've attached the updated version patches. This version patch
has pg_stat_reset_subscription_error() SQL function and sends a clear
message after skipping the transaction. 0004 patch includes the
skipping transaction feature and introducing RESET to ALTER
SUBSCRIPTION. It would be better to separate them.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Mon, Jul 26, 2021 at 11:58 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jul 22, 2021 at 8:53 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

On July 20, 2021 9:26 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 19, 2021 at 8:38 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

On July 19, 2021 2:40 PM Masahiko Sawada <sawada.mshk@gmail.com>

wrote:

I've attached the updated version patch that incorporated all
comments I got so far except for the clearing error details part I
mentioned above. After getting a consensus on those parts, I'll
incorporate the idea into the patches.

3) For 0003 patch, if user set skip_xid to a wrong xid which have not been
assigned, and then will the change be skipped when the xid is assigned in
the future even if it doesn't cause any conflicts ?

Yes. Currently, setting a correct xid is the user's responsibility. I think it would
be better to disable it or emit WARNING/ERROR when the user mistakenly set
the wrong xid if we find out a convenient way to detect that.

Thanks for the explanation. As Amit suggested, it seems we can document the
risk of misusing skip_xid. Besides, I found some minor things in the patch.

1) In 0002 patch

+ */
+static void
+pgstat_recv_subscription_purge(PgStat_MsgSubscriptionPurge *msg, int len)
+{
+       if (subscriptionErrHash != NULL)
+               return;
+

+static void
+pgstat_recv_subscription_error(PgStat_MsgSubscriptionErr *msg, int len)
+{

the second paramater "len" seems not used in the function
pgstat_recv_subscription_purge() and pgstat_recv_subscription_error().

'len' is not used at all in not only functions the patch added but
also other pgstat_recv_* functions. Can we remove all of them in a
separate patch? 'len' in pgstat_recv_* functions has never been used
since the stats collector code is introduced. It seems like that it
was mistakenly introduced in the first commit and other pgstat_recv_*
functions were added that followed it to define ‘len’ but didn’t also
use it at all.

2) in 0003 patch

* Helper function for apply_handle_commit and apply_handle_stream_commit.
*/
static void
-apply_handle_commit_internal(StringInfo s, LogicalRepCommitData *commit_data)
+apply_handle_commit_internal(LogicalRepCommitData *commit_data)
{

This looks like a separate change which remove unused paramater in existing
code, maybe we can get this committed first ?

Yeah, it seems to be introduced by commit 0926e96c493. I've attached
the patch for that.

Also, I've attached the updated version patches. This version patch
has pg_stat_reset_subscription_error() SQL function and sends a clear
message after skipping the transaction. 0004 patch includes the
skipping transaction feature and introducing RESET to ALTER
SUBSCRIPTION. It would be better to separate them.

I've attached the new version patches that fix cfbot failure.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Thu, Jul 29, 2021 at 2:04 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 26, 2021 at 11:58 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jul 22, 2021 at 8:53 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

On July 20, 2021 9:26 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Jul 19, 2021 at 8:38 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

On July 19, 2021 2:40 PM Masahiko Sawada <sawada.mshk@gmail.com>

wrote:

I've attached the updated version patch that incorporated all
comments I got so far except for the clearing error details part I
mentioned above. After getting a consensus on those parts, I'll
incorporate the idea into the patches.

3) For 0003 patch, if user set skip_xid to a wrong xid which have not been
assigned, and then will the change be skipped when the xid is assigned in
the future even if it doesn't cause any conflicts ?

Yes. Currently, setting a correct xid is the user's responsibility. I think it would
be better to disable it or emit WARNING/ERROR when the user mistakenly set
the wrong xid if we find out a convenient way to detect that.

Thanks for the explanation. As Amit suggested, it seems we can document the
risk of misusing skip_xid. Besides, I found some minor things in the patch.

1) In 0002 patch

+ */
+static void
+pgstat_recv_subscription_purge(PgStat_MsgSubscriptionPurge *msg, int len)
+{
+       if (subscriptionErrHash != NULL)
+               return;
+

+static void
+pgstat_recv_subscription_error(PgStat_MsgSubscriptionErr *msg, int len)
+{

the second paramater "len" seems not used in the function
pgstat_recv_subscription_purge() and pgstat_recv_subscription_error().

'len' is not used at all in not only functions the patch added but
also other pgstat_recv_* functions. Can we remove all of them in a
separate patch? 'len' in pgstat_recv_* functions has never been used
since the stats collector code is introduced. It seems like that it
was mistakenly introduced in the first commit and other pgstat_recv_*
functions were added that followed it to define ‘len’ but didn’t also
use it at all.

2) in 0003 patch

* Helper function for apply_handle_commit and apply_handle_stream_commit.
*/
static void
-apply_handle_commit_internal(StringInfo s, LogicalRepCommitData *commit_data)
+apply_handle_commit_internal(LogicalRepCommitData *commit_data)
{

This looks like a separate change which remove unused paramater in existing
code, maybe we can get this committed first ?

Yeah, it seems to be introduced by commit 0926e96c493. I've attached
the patch for that.

Also, I've attached the updated version patches. This version patch
has pg_stat_reset_subscription_error() SQL function and sends a clear
message after skipping the transaction. 0004 patch includes the
skipping transaction feature and introducing RESET to ALTER
SUBSCRIPTION. It would be better to separate them.

I've attached the new version patches that fix cfbot failure.

Sorry I've attached wrong ones. Reattached the correct version patches.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Thu, Jul 29, 2021 at 11:18 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jul 29, 2021 at 2:04 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Yeah, it seems to be introduced by commit 0926e96c493. I've attached
the patch for that.

Also, I've attached the updated version patches. This version patch
has pg_stat_reset_subscription_error() SQL function and sends a clear
message after skipping the transaction. 0004 patch includes the
skipping transaction feature and introducing RESET to ALTER
SUBSCRIPTION. It would be better to separate them.

+1, to separate out the reset part.

I've attached the new version patches that fix cfbot failure.

Sorry I've attached wrong ones. Reattached the correct version patches.

Pushed the 0001* patch that removes the unused parameter.

Few comments on v4-0001-Add-errcontext-to-errors-of-the-applying-logical-
===========================================================
1.
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -78,6 +78,7 @@
 #include "partitioning/partbounds.h"
 #include "partitioning/partdesc.h"
 #include "pgstat.h"
+#include "replication/logicalworker.h"
 #include "rewrite/rewriteDefine.h"
 #include "rewrite/rewriteHandler.h"
 #include "rewrite/rewriteManip.h"
@@ -1899,6 +1900,9 @@ ExecuteTruncateGuts(List *explicit_rels,
  continue;
  }

+ /* Set logical replication error callback info if necessary */
+ set_logicalrep_error_context_rel(rel);
+
  /*
  * Build the lists of foreign tables belonging to each foreign server
  * and pass each list to the foreign data wrapper's callback function,
@@ -2006,6 +2010,9 @@ ExecuteTruncateGuts(List *explicit_rels,
  pgstat_count_truncate(rel);
  }

+ /* Reset logical replication error callback info */
+ reset_logicalrep_error_context_rel();
+

Setting up logical rep error context in a generic function looks a bit
odd to me. Do we really need to set up error context here? I
understand we can't do this in caller but anyway I think we are not
sending this to logical replication view as well, so not sure we need
to do it here.

2.
+/* Struct for saving and restoring apply information */
+typedef struct ApplyErrCallbackArg
+{
+ LogicalRepMsgType command; /* 0 if invalid */
+
+ /* Local relation information */
+ char    *nspname; /* used for error context */
+ char    *relname; /* used for error context */
+
+ TransactionId remote_xid;
+ TimestampTz committs;
+} ApplyErrCallbackArg;
+static ApplyErrCallbackArg apply_error_callback_arg =
+{
+ .command = 0,
+ .relname = NULL,
+ .nspname = NULL,
+ .remote_xid = InvalidTransactionId,
+ .committs = 0,
+};
+

Better to have a space between the above two declarations.

3. commit message:
This commit adds the error context to errors happening during applying
logical replication changes, showing the command, the relation
relation, transaction ID, and commit timestamp in the server log.

'relation' is mentioned twice.

The patch is not getting applied probably due to yesterday's commit in
this area.

--
With Regards,
Amit Kapila.

On July 29, 2021 1:48 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Sorry I've attached wrong ones. Reattached the correct version patches.

Hi,

I had some comments on the new version patches.

1)

-       relstate = (SubscriptionRelState *) palloc(sizeof(SubscriptionRelState));
-       relstate->relid = subrel->srrelid;
+       relstate = (SubscriptionRelState *) hash_search(htab, (void *) &subrel->srrelid,
+                                                       HASH_ENTER, NULL);

I found the new version patch changes the List type 'relstate' to hash table type
'relstate'. Will this bring significant performance improvements ?

2)
+ * PgStat_StatSubRelErrEntry represents a error happened during logical

a error => an error

3)
+CREATE VIEW pg_stat_subscription_errors AS
+    SELECT
+   d.datname,
+   sr.subid,
+   s.subname,

It seems the 'subid' column is not mentioned in the document of the
pg_stat_subscription_errors view.

4)
+
+                   if (fread(&nrels, 1, sizeof(long), fpin) != sizeof(long))
+                   {
    ...
+                   for (int i = 0; i < nrels; i++)

the type of i(int) seems different of the type or 'nrels'(long), it might be
better to use the same type.

Best regards,
houzj

On Fri, Jul 30, 2021 at 12:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Jul 29, 2021 at 11:18 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jul 29, 2021 at 2:04 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Yeah, it seems to be introduced by commit 0926e96c493. I've attached
the patch for that.

Also, I've attached the updated version patches. This version patch
has pg_stat_reset_subscription_error() SQL function and sends a clear
message after skipping the transaction. 0004 patch includes the
skipping transaction feature and introducing RESET to ALTER
SUBSCRIPTION. It would be better to separate them.

+1, to separate out the reset part.

Okay, I'll do that.

I've attached the new version patches that fix cfbot failure.

Sorry I've attached wrong ones. Reattached the correct version patches.

Pushed the 0001* patch that removes the unused parameter.

Thanks!

Few comments on v4-0001-Add-errcontext-to-errors-of-the-applying-logical-
===========================================================

Thank you for the comments!

1.
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -78,6 +78,7 @@
#include "partitioning/partbounds.h"
#include "partitioning/partdesc.h"
#include "pgstat.h"
+#include "replication/logicalworker.h"
#include "rewrite/rewriteDefine.h"
#include "rewrite/rewriteHandler.h"
#include "rewrite/rewriteManip.h"
@@ -1899,6 +1900,9 @@ ExecuteTruncateGuts(List *explicit_rels,
continue;
}

+ /* Set logical replication error callback info if necessary */
+ set_logicalrep_error_context_rel(rel);
+
/*
* Build the lists of foreign tables belonging to each foreign server
* and pass each list to the foreign data wrapper's callback function,
@@ -2006,6 +2010,9 @@ ExecuteTruncateGuts(List *explicit_rels,
pgstat_count_truncate(rel);
}

+ /* Reset logical replication error callback info */
+ reset_logicalrep_error_context_rel();
+

Setting up logical rep error context in a generic function looks a bit
odd to me. Do we really need to set up error context here? I
understand we can't do this in caller but anyway I think we are not
sending this to logical replication view as well, so not sure we need
to do it here.

Yeah, I'm not convinced of this part yet. I wanted to show relid also
in truncate cases but I came up with only this idea.

If an error happens during truncating the table (in
ExecuteTruncateGuts()), relid set by
set_logicalrep_error_context_rel() is actually sent to the view. If we
don’t have it, the view always shows relid as NULL in truncate cases.
On the other hand, it doesn’t cover all cases. For example, it doesn’t
cover an error that the target table doesn’t exist on the subscriber,
which happens when opening the target table. Anyway, in most cases,
even if relid is NULL, the error message in the view helps users to
know which relation the error happened on. What do you think?

2.
+/* Struct for saving and restoring apply information */
+typedef struct ApplyErrCallbackArg
+{
+ LogicalRepMsgType command; /* 0 if invalid */
+
+ /* Local relation information */
+ char    *nspname; /* used for error context */
+ char    *relname; /* used for error context */
+
+ TransactionId remote_xid;
+ TimestampTz committs;
+} ApplyErrCallbackArg;
+static ApplyErrCallbackArg apply_error_callback_arg =
+{
+ .command = 0,
+ .relname = NULL,
+ .nspname = NULL,
+ .remote_xid = InvalidTransactionId,
+ .committs = 0,
+};
+

Better to have a space between the above two declarations.

Will fix.

3. commit message:
This commit adds the error context to errors happening during applying
logical replication changes, showing the command, the relation
relation, transaction ID, and commit timestamp in the server log.

'relation' is mentioned twice.

Will fix.

The patch is not getting applied probably due to yesterday's commit in
this area.

Okay. I'll rebase the patches to the current HEAD.

I'm incorporating all comments from you and Houzj, and will submit the
new patch soon.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Fri, Jul 30, 2021 at 3:47 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

On July 29, 2021 1:48 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Sorry I've attached wrong ones. Reattached the correct version patches.

Hi,

I had some comments on the new version patches.

Thank you for the comments!

1)

-       relstate = (SubscriptionRelState *) palloc(sizeof(SubscriptionRelState));
-       relstate->relid = subrel->srrelid;
+       relstate = (SubscriptionRelState *) hash_search(htab, (void *) &subrel->srrelid,
+                                                       HASH_ENTER, NULL);

I found the new version patch changes the List type 'relstate' to hash table type
'relstate'. Will this bring significant performance improvements ?

For pgstat_vacuum_stat() purposes, I think it's better to use a hash
table to avoid O(N) lookup. But it might not be good to change the
type of the return value of GetSubscriptionNotReadyRelations() since
this returned value is used by other functions to iterate over
elements. The list iteration is faster than the hash table’s one. It
would be better to change it so that pgstat_vacuum_stat() constructs a
hash table for its own purpose.

2)
+ * PgStat_StatSubRelErrEntry represents a error happened during logical

a error => an error

Will fix.

3)
+CREATE VIEW pg_stat_subscription_errors AS
+    SELECT
+   d.datname,
+   sr.subid,
+   s.subname,

It seems the 'subid' column is not mentioned in the document of the
pg_stat_subscription_errors view.

Will fix.

4)
+
+                   if (fread(&nrels, 1, sizeof(long), fpin) != sizeof(long))
+                   {
...
+                   for (int i = 0; i < nrels; i++)

the type of i(int) seems different of the type or 'nrels'(long), it might be
better to use the same type.

Will fix.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Mon, Aug 2, 2021 at 7:45 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Fri, Jul 30, 2021 at 12:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Jul 29, 2021 at 11:18 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Setting up logical rep error context in a generic function looks a bit
odd to me. Do we really need to set up error context here? I
understand we can't do this in caller but anyway I think we are not
sending this to logical replication view as well, so not sure we need
to do it here.

Yeah, I'm not convinced of this part yet. I wanted to show relid also
in truncate cases but I came up with only this idea.

If an error happens during truncating the table (in
ExecuteTruncateGuts()), relid set by
set_logicalrep_error_context_rel() is actually sent to the view. If we
don’t have it, the view always shows relid as NULL in truncate cases.
On the other hand, it doesn’t cover all cases. For example, it doesn’t
cover an error that the target table doesn’t exist on the subscriber,
which happens when opening the target table. Anyway, in most cases,
even if relid is NULL, the error message in the view helps users to
know which relation the error happened on. What do you think?

Yeah, I also think at this stage error message is sufficient in such cases.

--
With Regards,
Amit Kapila.

On Mon, Aug 2, 2021 at 12:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Aug 2, 2021 at 7:45 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Fri, Jul 30, 2021 at 12:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Jul 29, 2021 at 11:18 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Setting up logical rep error context in a generic function looks a bit
odd to me. Do we really need to set up error context here? I
understand we can't do this in caller but anyway I think we are not
sending this to logical replication view as well, so not sure we need
to do it here.

Yeah, I'm not convinced of this part yet. I wanted to show relid also
in truncate cases but I came up with only this idea.

If an error happens during truncating the table (in
ExecuteTruncateGuts()), relid set by
set_logicalrep_error_context_rel() is actually sent to the view. If we
don’t have it, the view always shows relid as NULL in truncate cases.
On the other hand, it doesn’t cover all cases. For example, it doesn’t
cover an error that the target table doesn’t exist on the subscriber,
which happens when opening the target table. Anyway, in most cases,
even if relid is NULL, the error message in the view helps users to
know which relation the error happened on. What do you think?

Yeah, I also think at this stage error message is sufficient in such cases.

I've attached new patches that incorporate all comments I got so far.
Please review them.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, Aug 3, 2021 at 12:20 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Aug 2, 2021 at 12:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Aug 2, 2021 at 7:45 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Fri, Jul 30, 2021 at 12:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Jul 29, 2021 at 11:18 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Setting up logical rep error context in a generic function looks a bit
odd to me. Do we really need to set up error context here? I
understand we can't do this in caller but anyway I think we are not
sending this to logical replication view as well, so not sure we need
to do it here.

Yeah, I'm not convinced of this part yet. I wanted to show relid also
in truncate cases but I came up with only this idea.

If an error happens during truncating the table (in
ExecuteTruncateGuts()), relid set by
set_logicalrep_error_context_rel() is actually sent to the view. If we
don’t have it, the view always shows relid as NULL in truncate cases.
On the other hand, it doesn’t cover all cases. For example, it doesn’t
cover an error that the target table doesn’t exist on the subscriber,
which happens when opening the target table. Anyway, in most cases,
even if relid is NULL, the error message in the view helps users to
know which relation the error happened on. What do you think?

Yeah, I also think at this stage error message is sufficient in such cases.

I've attached new patches that incorporate all comments I got so far.
Please review them.

I had a look at the first patch, couple of minor comments:
1) Should we include this in typedefs.lst
+/* Struct for saving and restoring apply information */
+typedef struct ApplyErrCallbackArg
+{
+       LogicalRepMsgType command;      /* 0 if invalid */
+
+       /* Local relation information */
+       char       *nspname;

2)  We can keep the case statement in the same order as in the
LogicalRepMsgType enum, this will help in easily identifying if any
enum gets missed.
+               case LOGICAL_REP_MSG_RELATION:
+                       return "RELATION";
+               case LOGICAL_REP_MSG_TYPE:
+                       return "TYPE";
+               case LOGICAL_REP_MSG_ORIGIN:
+                       return "ORIGIN";
+               case LOGICAL_REP_MSG_MESSAGE:
+                       return "MESSAGE";
+               case LOGICAL_REP_MSG_STREAM_START:
+                       return "STREAM START";

Regards,
Vignesh

On Tuesday, August 3, 2021 2:49 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached new patches that incorporate all comments I got so far.
Please review them.

Hi,

I had a few comments for the 0003 patch.

1).
-      This clause alters parameters originally set by
-      <xref linkend="sql-createsubscription"/>.  See there for more
-      information.  The parameters that can be altered
-      are <literal>slot_name</literal>,
-      <literal>synchronous_commit</literal>,
-      <literal>binary</literal>, and
-      <literal>streaming</literal>.
+      This clause sets or resets a subscription option. The parameters that can be
+      set are the parameters originally set by <xref linkend="sql-createsubscription"/>:
+      <literal>slot_name</literal>, <literal>synchronous_commit</literal>,
+      <literal>binary</literal>, <literal>streaming</literal>.
+     </para>
+     <para>
+       The parameters that can be reset are: <literal>streaming</literal>,
+       <literal>binary</literal>, <literal>synchronous_commit</literal>.

Maybe the doc looks better like the following ?

+      This clause alters parameters originally set by
+      <xref linkend="sql-createsubscription"/>.  See there for more
+      information.  The parameters that can be set
+      are <literal>slot_name</literal>,
+      <literal>synchronous_commit</literal>,
+      <literal>binary</literal>, and
+      <literal>streaming</literal>.
+     </para>
+     <para>
+       The parameters that can be reset are: <literal>streaming</literal>,
+       <literal>binary</literal>, <literal>synchronous_commit</literal>.

2).
-           opts->create_slot = defGetBoolean(defel);
+           if (!is_reset)
+               opts->create_slot = defGetBoolean(defel);
        }

Since we only support RESET streaming/binary/synchronous_commit, it
might be unnecessary to add the check 'if (!is_reset)' for other
option.

3).
typedef struct AlterSubscriptionStmt
{
NodeTag type;
AlterSubscriptionType kind; /* ALTER_SUBSCRIPTION_OPTIONS, etc */

Since the patch change the remove the enum value
'ALTER_SUBSCRIPTION_OPTIONS', it'd better to change the comment here
as well.

Best regards,
houzj

On Tue, Aug 3, 2021 at 7:54 PM vignesh C <vignesh21@gmail.com> wrote:

On Tue, Aug 3, 2021 at 12:20 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Aug 2, 2021 at 12:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Aug 2, 2021 at 7:45 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Fri, Jul 30, 2021 at 12:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Jul 29, 2021 at 11:18 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Setting up logical rep error context in a generic function looks a bit
odd to me. Do we really need to set up error context here? I
understand we can't do this in caller but anyway I think we are not
sending this to logical replication view as well, so not sure we need
to do it here.

Yeah, I'm not convinced of this part yet. I wanted to show relid also
in truncate cases but I came up with only this idea.

If an error happens during truncating the table (in
ExecuteTruncateGuts()), relid set by
set_logicalrep_error_context_rel() is actually sent to the view. If we
don’t have it, the view always shows relid as NULL in truncate cases.
On the other hand, it doesn’t cover all cases. For example, it doesn’t
cover an error that the target table doesn’t exist on the subscriber,
which happens when opening the target table. Anyway, in most cases,
even if relid is NULL, the error message in the view helps users to
know which relation the error happened on. What do you think?

Yeah, I also think at this stage error message is sufficient in such cases.

I've attached new patches that incorporate all comments I got so far.
Please review them.

I had a look at the first patch, couple of minor comments:
1) Should we include this in typedefs.lst
+/* Struct for saving and restoring apply information */
+typedef struct ApplyErrCallbackArg
+{
+       LogicalRepMsgType command;      /* 0 if invalid */
+
+       /* Local relation information */
+       char       *nspname;

2)  We can keep the case statement in the same order as in the
LogicalRepMsgType enum, this will help in easily identifying if any
enum gets missed.
+               case LOGICAL_REP_MSG_RELATION:
+                       return "RELATION";
+               case LOGICAL_REP_MSG_TYPE:
+                       return "TYPE";
+               case LOGICAL_REP_MSG_ORIGIN:
+                       return "ORIGIN";
+               case LOGICAL_REP_MSG_MESSAGE:
+                       return "MESSAGE";
+               case LOGICAL_REP_MSG_STREAM_START:
+                       return "STREAM START";

Thank you for reviewing the patch!

I agreed with all comments and will fix them in the next version patch.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Wed, Aug 4, 2021 at 1:02 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

On Tuesday, August 3, 2021 2:49 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached new patches that incorporate all comments I got so far.
Please review them.

Hi,

I had a few comments for the 0003 patch.

Thanks for reviewing the patch!

1).
-      This clause alters parameters originally set by
-      <xref linkend="sql-createsubscription"/>.  See there for more
-      information.  The parameters that can be altered
-      are <literal>slot_name</literal>,
-      <literal>synchronous_commit</literal>,
-      <literal>binary</literal>, and
-      <literal>streaming</literal>.
+      This clause sets or resets a subscription option. The parameters that can be
+      set are the parameters originally set by <xref linkend="sql-createsubscription"/>:
+      <literal>slot_name</literal>, <literal>synchronous_commit</literal>,
+      <literal>binary</literal>, <literal>streaming</literal>.
+     </para>
+     <para>
+       The parameters that can be reset are: <literal>streaming</literal>,
+       <literal>binary</literal>, <literal>synchronous_commit</literal>.

Maybe the doc looks better like the following ?

+      This clause alters parameters originally set by
+      <xref linkend="sql-createsubscription"/>.  See there for more
+      information.  The parameters that can be set
+      are <literal>slot_name</literal>,
+      <literal>synchronous_commit</literal>,
+      <literal>binary</literal>, and
+      <literal>streaming</literal>.
+     </para>
+     <para>
+       The parameters that can be reset are: <literal>streaming</literal>,
+       <literal>binary</literal>, <literal>synchronous_commit</literal>.

Agreed.

2).
-           opts->create_slot = defGetBoolean(defel);
+           if (!is_reset)
+               opts->create_slot = defGetBoolean(defel);
}

Since we only support RESET streaming/binary/synchronous_commit, it
might be unnecessary to add the check 'if (!is_reset)' for other
option.

Good point.

3).
typedef struct AlterSubscriptionStmt
{
NodeTag type;
AlterSubscriptionType kind; /* ALTER_SUBSCRIPTION_OPTIONS, etc */

Since the patch change the remove the enum value
'ALTER_SUBSCRIPTION_OPTIONS', it'd better to change the comment here
as well.

Agreed.

I'll incorporate those comments in the next version patch.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tuesday, August 3, 2021 3:49 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached new patches that incorporate all comments I got so far.
Please review them.

Hi, I had a chance to look at the patch-set during my other development.
Just let me share some minor cosmetic things.

[1] unnatural wording ? in v5-0002.
+ * create tells whether to create the new subscription entry if it is not
+ * create tells whether to create the new subscription relation entry if it is

I'm not sure if this wording is correct or not.
You meant just "tells whether to create ...." ?,
although we already have 1 other "create tells" in HEAD.

[2]: typo "kep" in v05-0002.

I think you meant "kept" in below sentence.

+/*
+ * Subscription error statistics kep in the stats collector.  One entry represents
+ * an error that happened during logical replication, reported by the apply worker
+ * (subrelid is InvalidOid) or by the table sync worker (subrelid is a valid OID).

[3]: typo "lotigcal" in the v05-0004 commit message.

If incoming change violates any constraint, lotigcal replication stops
until it's resolved. This commit introduces another way to skip the
transaction in question.

It should be "logical".

[4]: warning of doc build

I've gotten an output like below during my process of make html.
Could you please check this ?

Link element has no content and no Endterm. Nothing to show in the link to monitoring-pg-stat-subscription-errors

Best Regards,
Takamichi Osumi

On Wednesday, August 4, 2021 8:46 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I'll incorporate those comments in the next version patch.

Hi, when are you going to make and share the updated v6 ?

Best Regards,
Takamichi Osumi

On Thu, Aug 5, 2021 at 5:58 PM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:

On Tuesday, August 3, 2021 3:49 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached new patches that incorporate all comments I got so far.
Please review them.

Hi, I had a chance to look at the patch-set during my other development.
Just let me share some minor cosmetic things.

Thank you for reviewing the patches!

[1] unnatural wording ? in v5-0002.
+ * create tells whether to create the new subscription entry if it is not
+ * create tells whether to create the new subscription relation entry if it is

I'm not sure if this wording is correct or not.
You meant just "tells whether to create ...." ?,
although we already have 1 other "create tells" in HEAD.

create here means the function argument of
pgstat_get_subscription_entry() and
pgstat_get_subscription_error_entry(). That is, the function argument
'create' tells whether to create the new entry if not found. I
single-quoted the 'create' to avoid confusion.g

[2] typo "kep" in v05-0002.

I think you meant "kept" in below sentence.

+/*
+ * Subscription error statistics kep in the stats collector.  One entry represents
+ * an error that happened during logical replication, reported by the apply worker
+ * (subrelid is InvalidOid) or by the table sync worker (subrelid is a valid OID).

Fixed.

[3] typo "lotigcal" in the v05-0004 commit message.

If incoming change violates any constraint, lotigcal replication stops
until it's resolved. This commit introduces another way to skip the
transaction in question.

It should be "logical".

Fixed.

[4] warning of doc build

I've gotten an output like below during my process of make html.
Could you please check this ?

Link element has no content and no Endterm. Nothing to show in the link to monitoring-pg-stat-subscription-errors

Fixed.

I've attached the latest patches that incorporated all comments I got
so far. Please review them.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, Aug 10, 2021 at 10:37 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the latest patches that incorporated all comments I got
so far. Please review them.

I am not able to apply the latest patch
(v6-0001-Add-errcontext-to-errors-happening-during-applyin) on HEAD,
getting the below error:
patching file src/backend/replication/logical/worker.c
Hunk #11 succeeded at 1195 (offset 50 lines).
Hunk #12 succeeded at 1253 (offset 50 lines).
Hunk #13 succeeded at 1277 (offset 50 lines).
Hunk #14 succeeded at 1305 (offset 50 lines).
Hunk #15 succeeded at 1330 (offset 50 lines).
Hunk #16 succeeded at 1362 (offset 50 lines).
Hunk #17 succeeded at 1508 (offset 50 lines).
Hunk #18 succeeded at 1524 (offset 50 lines).
Hunk #19 succeeded at 1645 (offset 50 lines).
Hunk #20 succeeded at 1671 (offset 50 lines).
Hunk #21 succeeded at 1772 (offset 50 lines).
Hunk #22 succeeded at 1828 (offset 50 lines).
Hunk #23 succeeded at 1934 (offset 50 lines).
Hunk #24 succeeded at 1962 (offset 50 lines).
Hunk #25 succeeded at 2399 (offset 50 lines).
Hunk #26 FAILED at 2405.
Hunk #27 succeeded at 3730 (offset 54 lines).
1 out of 27 hunks FAILED -- saving rejects to file
src/backend/replication/logical/worker.c.rej

--
With Regards,
Amit Kapila.

On Tue, Aug 10, 2021 at 11:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Aug 10, 2021 at 10:37 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the latest patches that incorporated all comments I got
so far. Please review them.

I am not able to apply the latest patch
(v6-0001-Add-errcontext-to-errors-happening-during-applyin) on HEAD,
getting the below error:

Few comments on v6-0001-Add-errcontext-to-errors-happening-during-applyin
==============================================================

1. While applying DML operations, we are setting up the error context
multiple times due to which the context information is not
appropriate. The first is set in apply_dispatch and then during
processing, we set another error callback slot_store_error_callback in
slot_store_data and slot_modify_data. When I forced one of the errors
in slot_store_data(), it displays the below information in CONTEXT
which doesn't make much sense.

2021-08-10 15:16:39.887 IST [6784] ERROR: incorrect binary data
format in logical replication column 1
2021-08-10 15:16:39.887 IST [6784] CONTEXT: processing remote data
for replication target relation "public.test1" column "id"
during apply of "INSERT" for relation "public.test1" in
transaction with xid 740 committs 2021-08-10 14:44:38.058174+05:30

2.
I think we can slightly change the new context information as below:
Before
during apply of "INSERT" for relation "public.test1" in transaction
with xid 740 committs 2021-08-10 14:44:38.058174+05:30
After
during apply of "INSERT" for relation "public.test1" in transaction id
740 with commit timestamp 2021-08-10 14:44:38.058174+05:30

3.
+/* Struct for saving and restoring apply information */
+typedef struct ApplyErrCallbackArg
+{
+ LogicalRepMsgType command; /* 0 if invalid */
+
+ /* Local relation information */
+ char    *nspname;
+ char    *relname;

...
...

+
+static ApplyErrCallbackArg apply_error_callback_arg =
+{
+ .command = 0,
+ .relname = NULL,
+ .nspname = NULL,

Let's initialize the struct members in the order they are declared.
The order of relname and nspname should be another way.

4.
+
+ TransactionId remote_xid;
+ TimestampTz committs;
+} ApplyErrCallbackArg;

It might be better to add a comment like "remote xact information"
above these structure members.

5.
+static void
+apply_error_callback(void *arg)
+{
+ StringInfoData buf;
+
+ if (apply_error_callback_arg.command == 0)
+ return;
+
+ initStringInfo(&buf);

At the end of this call, it is better to free this (pfree(buf.data))

6. In the commit message, you might want to indicate that this
additional information can be used by the future patch to skip the
conflicting transaction.

--
With Regards,
Amit Kapila.

On Tue, Aug 10, 2021 at 3:29 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Aug 10, 2021 at 10:37 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the latest patches that incorporated all comments I got
so far. Please review them.

I am not able to apply the latest patch
(v6-0001-Add-errcontext-to-errors-happening-during-applyin) on HEAD,
getting the below error:
patching file src/backend/replication/logical/worker.c
Hunk #11 succeeded at 1195 (offset 50 lines).
Hunk #12 succeeded at 1253 (offset 50 lines).
Hunk #13 succeeded at 1277 (offset 50 lines).
Hunk #14 succeeded at 1305 (offset 50 lines).
Hunk #15 succeeded at 1330 (offset 50 lines).
Hunk #16 succeeded at 1362 (offset 50 lines).
Hunk #17 succeeded at 1508 (offset 50 lines).
Hunk #18 succeeded at 1524 (offset 50 lines).
Hunk #19 succeeded at 1645 (offset 50 lines).
Hunk #20 succeeded at 1671 (offset 50 lines).
Hunk #21 succeeded at 1772 (offset 50 lines).
Hunk #22 succeeded at 1828 (offset 50 lines).
Hunk #23 succeeded at 1934 (offset 50 lines).
Hunk #24 succeeded at 1962 (offset 50 lines).
Hunk #25 succeeded at 2399 (offset 50 lines).
Hunk #26 FAILED at 2405.
Hunk #27 succeeded at 3730 (offset 54 lines).
1 out of 27 hunks FAILED -- saving rejects to file
src/backend/replication/logical/worker.c.rej

Sorry, I forgot to rebase the patches to the current HEAD. Since
stream_prepare is introduced, I'll add some tests to the patches. I’ll
submit the new patches tomorrow that also incorporates your comments
on v6-0001 patch.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, Aug 10, 2021 at 3:07 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the latest patches that incorporated all comments I got
so far. Please review them.

Some initial review comments on the v6-0001 patch:

src/backend/replication/logical/proto.c:
(1)

+ TimestampTz committs;

I think it looks better to name "committs" as "commit_ts", and also is
more consistent with naming for other member "remote_xid".

src/backend/replication/logical/worker.c:
(2)
To be consistent with all other function headers, should start
sentence with capital: "get" -> "Get"

+ * get string representing LogicalRepMsgType.

(3) It looks a bit cumbersome and repetitive to set/update the members
of apply_error_callback_arg in numerous places.

I suggest making the "set_apply_error_context..." and
"reset_apply_error_context..." functions as "static inline void"
functions (moving them to the top part of the source file, and
removing the existing function declarations for these).

Also, can add something similar to below:

static inline void
set_apply_error_callback_xid(TransactionId xid)
{
apply_error_callback_arg.remote_xid = xid;
}

static inline void
set_apply_error_callback_xid_info(TransactionId xid, TimestampTz commit_ts)
{
apply_error_callback_arg.remote_xid = xid;
apply_error_callback_arg.commit_ts = commit_ts;
}

so that instances of, for example:

apply_error_callback_arg.remote_xid = prepare_data.xid;
apply_error_callback_arg.committs = prepare_data.commit_time;

can be:

set_apply_error_callback_tx_info(prepare_data.xid, prepare_data.commit_time);

(4) The apply_error_callback() function is missing a function header/comment.

Regards,
Greg Nancarrow
Fujitsu Australia

On Tue, Aug 10, 2021 at 7:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Aug 10, 2021 at 11:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Aug 10, 2021 at 10:37 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the latest patches that incorporated all comments I got
so far. Please review them.

I am not able to apply the latest patch
(v6-0001-Add-errcontext-to-errors-happening-during-applyin) on HEAD,
getting the below error:

Few comments on v6-0001-Add-errcontext-to-errors-happening-during-applyin

Thank you for the comments!

==============================================================

1. While applying DML operations, we are setting up the error context
multiple times due to which the context information is not
appropriate. The first is set in apply_dispatch and then during
processing, we set another error callback slot_store_error_callback in
slot_store_data and slot_modify_data. When I forced one of the errors
in slot_store_data(), it displays the below information in CONTEXT
which doesn't make much sense.

2021-08-10 15:16:39.887 IST [6784] ERROR: incorrect binary data
format in logical replication column 1
2021-08-10 15:16:39.887 IST [6784] CONTEXT: processing remote data
for replication target relation "public.test1" column "id"
during apply of "INSERT" for relation "public.test1" in
transaction with xid 740 committs 2021-08-10 14:44:38.058174+05:30

Yes, but we cannot change the error context message depending on other
error context messages. So it seems hard to construct a complete
sentence in the context message that is okay in terms of English
grammar. Is the following message better?

CONTEXT: processing remote data for replication target relation
"public.test1" column “id"
applying "INSERT" for relation "public.test1” in transaction
with xid 740 committs 2021-08-10 14:44:38.058174+05:30

2.
I think we can slightly change the new context information as below:
Before
during apply of "INSERT" for relation "public.test1" in transaction
with xid 740 committs 2021-08-10 14:44:38.058174+05:30
After
during apply of "INSERT" for relation "public.test1" in transaction id
740 with commit timestamp 2021-08-10 14:44:38.058174+05:30

Fixed.

3.
+/* Struct for saving and restoring apply information */
+typedef struct ApplyErrCallbackArg
+{
+ LogicalRepMsgType command; /* 0 if invalid */
+
+ /* Local relation information */
+ char    *nspname;
+ char    *relname;

...
...

+
+static ApplyErrCallbackArg apply_error_callback_arg =
+{
+ .command = 0,
+ .relname = NULL,
+ .nspname = NULL,

Let's initialize the struct members in the order they are declared.
The order of relname and nspname should be another way.

Fixed.

4.
+
+ TransactionId remote_xid;
+ TimestampTz committs;
+} ApplyErrCallbackArg;

It might be better to add a comment like "remote xact information"
above these structure members.

Fixed.

5.
+static void
+apply_error_callback(void *arg)
+{
+ StringInfoData buf;
+
+ if (apply_error_callback_arg.command == 0)
+ return;
+
+ initStringInfo(&buf);

At the end of this call, it is better to free this (pfree(buf.data))

Fixed.

6. In the commit message, you might want to indicate that this
additional information can be used by the future patch to skip the
conflicting transaction.

Fixed.

I've attached the new patches. Please review them.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, Aug 10, 2021 at 10:27 PM Greg Nancarrow <gregn4422@gmail.com> wrote:

On Tue, Aug 10, 2021 at 3:07 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the latest patches that incorporated all comments I got
so far. Please review them.

Some initial review comments on the v6-0001 patch:

Thanks for reviewing the patch!

src/backend/replication/logical/proto.c:
(1)

+ TimestampTz committs;

I think it looks better to name "committs" as "commit_ts", and also is
more consistent with naming for other member "remote_xid".

Fixed.

src/backend/replication/logical/worker.c:
(2)
To be consistent with all other function headers, should start
sentence with capital: "get" -> "Get"

+ * get string representing LogicalRepMsgType.

Fixed

(3) It looks a bit cumbersome and repetitive to set/update the members
of apply_error_callback_arg in numerous places.

I suggest making the "set_apply_error_context..." and
"reset_apply_error_context..." functions as "static inline void"
functions (moving them to the top part of the source file, and
removing the existing function declarations for these).

Also, can add something similar to below:

static inline void
set_apply_error_callback_xid(TransactionId xid)
{
apply_error_callback_arg.remote_xid = xid;
}

static inline void
set_apply_error_callback_xid_info(TransactionId xid, TimestampTz commit_ts)
{
apply_error_callback_arg.remote_xid = xid;
apply_error_callback_arg.commit_ts = commit_ts;
}

so that instances of, for example:

apply_error_callback_arg.remote_xid = prepare_data.xid;
apply_error_callback_arg.committs = prepare_data.commit_time;

can be:

set_apply_error_callback_tx_info(prepare_data.xid, prepare_data.commit_time);

Okay. I've added set_apply_error_callback_xact() function to set
transaction information to apply error callback. Also, I inlined those
helper functions since we call them every change.

(4) The apply_error_callback() function is missing a function header/comment.

Added.

The fixes for the above comments are incorporated in the v7 patch I
just submitted[1]/messages/by-id/CAD21AoALAq_0q_Zz2K0tO=kuUj8aBrDdMJXbey1P6t4w8snpQQ@mail.gmail.com.

Regards,

[1]: /messages/by-id/CAD21AoALAq_0q_Zz2K0tO=kuUj8aBrDdMJXbey1P6t4w8snpQQ@mail.gmail.com

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Wed, Aug 11, 2021 at 11:19 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Aug 10, 2021 at 7:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

==============================================================

1. While applying DML operations, we are setting up the error context
multiple times due to which the context information is not
appropriate. The first is set in apply_dispatch and then during
processing, we set another error callback slot_store_error_callback in
slot_store_data and slot_modify_data. When I forced one of the errors
in slot_store_data(), it displays the below information in CONTEXT
which doesn't make much sense.

2021-08-10 15:16:39.887 IST [6784] ERROR: incorrect binary data
format in logical replication column 1
2021-08-10 15:16:39.887 IST [6784] CONTEXT: processing remote data
for replication target relation "public.test1" column "id"
during apply of "INSERT" for relation "public.test1" in
transaction with xid 740 committs 2021-08-10 14:44:38.058174+05:30

Yes, but we cannot change the error context message depending on other
error context messages. So it seems hard to construct a complete
sentence in the context message that is okay in terms of English
grammar. Is the following message better?

CONTEXT: processing remote data for replication target relation
"public.test1" column “id"
applying "INSERT" for relation "public.test1” in transaction
with xid 740 committs 2021-08-10 14:44:38.058174+05:30

I don't like the proposed text. How about if we combine both and have
something like: "processing remote data during "UPDATE" for
replication target relation "public.test1" column "id" in transaction
id 740 with commit timestamp 2021-08-10 14:44:38.058174+05:30"? For
this, I think we need to remove slot_store_error_callback and
add/change the ApplyErrCallbackArg to include the additional required
information in that callback.

--
With Regards,
Amit Kapila.

On Wed, Aug 11, 2021 at 2:48 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the new patches. Please review them.

Please note that newly added tap tests fail due to known assertion
failure in pgstats that I reported here[1]/messages/by-id/CAD21AoCCAa+J1-udHRo5-Hbtv=D38WdZDAaXZGDbQQ_Vg_d3bQ@mail.gmail.com.

Regards,

[1]: /messages/by-id/CAD21AoCCAa+J1-udHRo5-Hbtv=D38WdZDAaXZGDbQQ_Vg_d3bQ@mail.gmail.com

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Wed, Aug 11, 2021 at 5:19 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Aug 11, 2021 at 11:19 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Aug 10, 2021 at 7:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

==============================================================

1. While applying DML operations, we are setting up the error context
multiple times due to which the context information is not
appropriate. The first is set in apply_dispatch and then during
processing, we set another error callback slot_store_error_callback in
slot_store_data and slot_modify_data. When I forced one of the errors
in slot_store_data(), it displays the below information in CONTEXT
which doesn't make much sense.

2021-08-10 15:16:39.887 IST [6784] ERROR: incorrect binary data
format in logical replication column 1
2021-08-10 15:16:39.887 IST [6784] CONTEXT: processing remote data
for replication target relation "public.test1" column "id"
during apply of "INSERT" for relation "public.test1" in
transaction with xid 740 committs 2021-08-10 14:44:38.058174+05:30

Yes, but we cannot change the error context message depending on other
error context messages. So it seems hard to construct a complete
sentence in the context message that is okay in terms of English
grammar. Is the following message better?

CONTEXT: processing remote data for replication target relation
"public.test1" column “id"
applying "INSERT" for relation "public.test1” in transaction
with xid 740 committs 2021-08-10 14:44:38.058174+05:30

I don't like the proposed text. How about if we combine both and have
something like: "processing remote data during "UPDATE" for
replication target relation "public.test1" column "id" in transaction
id 740 with commit timestamp 2021-08-10 14:44:38.058174+05:30"? For
this, I think we need to remove slot_store_error_callback and
add/change the ApplyErrCallbackArg to include the additional required
information in that callback.

Oh, I've never thought about that. That's a good idea.

I've attached the updated patches. FYI I've included the patch
(v8-0005) that fixes the assertion failure during shared fileset
cleanup to make cfbot tests happy.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Thu, Aug 12, 2021 at 3:54 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the updated patches. FYI I've included the patch
(v8-0005) that fixes the assertion failure during shared fileset
cleanup to make cfbot tests happy.

A minor comment on the 0001 patch: In the message I think that using
"ID" would look better than lowercase "id" and AFAICS it's more
consistent with existing messages.

+ appendStringInfo(&buf, _(" in transaction id %u with commit timestamp %s"),

Regards,
Greg Nancarrow
Fujitsu Australia

On Thu, Aug 12, 2021 at 1:21 PM Greg Nancarrow <gregn4422@gmail.com> wrote:

On Thu, Aug 12, 2021 at 3:54 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the updated patches. FYI I've included the patch
(v8-0005) that fixes the assertion failure during shared fileset
cleanup to make cfbot tests happy.

A minor comment on the 0001 patch: In the message I think that using
"ID" would look better than lowercase "id" and AFAICS it's more
consistent with existing messages.

+ appendStringInfo(&buf, _(" in transaction id %u with commit timestamp %s"),

You have a point but I think in this case it might look a bit odd as
we have another field 'commit timestamp' after that which is
lowercase.

--
With Regards,
Amit Kapila.

On Thu, Aug 12, 2021 at 9:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

A minor comment on the 0001 patch: In the message I think that using
"ID" would look better than lowercase "id" and AFAICS it's more
consistent with existing messages.

+ appendStringInfo(&buf, _(" in transaction id %u with commit timestamp %s"),

You have a point but I think in this case it might look a bit odd as
we have another field 'commit timestamp' after that which is
lowercase.

I did a quick search and I couldn't find any other messages in the
Postgres code that use "transaction id", but I could find some that
use "transaction ID" and "transaction identifier".

Regards,
Greg Nancarrow
Fujitsu Australia

On Thu, Aug 12, 2021 at 5:41 PM Greg Nancarrow <gregn4422@gmail.com> wrote:

On Thu, Aug 12, 2021 at 9:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

A minor comment on the 0001 patch: In the message I think that using
"ID" would look better than lowercase "id" and AFAICS it's more
consistent with existing messages.

+ appendStringInfo(&buf, _(" in transaction id %u with commit timestamp %s"),

You have a point but I think in this case it might look a bit odd as
we have another field 'commit timestamp' after that which is
lowercase.

I did a quick search and I couldn't find any other messages in the
Postgres code that use "transaction id", but I could find some that
use "transaction ID" and "transaction identifier".

Okay, but that doesn't mean using it here is bad. I am personally fine
with a message containing something like "... in transaction
id 740 with commit timestamp 2021-08-10 14:44:38.058174+05:30" but I
won't mind if you and or others find some other way convenient. Any
opinion from others?

--
With Regards,
Amit Kapila.

On Fri, Aug 13, 2021 at 2:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Aug 12, 2021 at 5:41 PM Greg Nancarrow <gregn4422@gmail.com> wrote:

On Thu, Aug 12, 2021 at 9:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

A minor comment on the 0001 patch: In the message I think that using
"ID" would look better than lowercase "id" and AFAICS it's more
consistent with existing messages.

+ appendStringInfo(&buf, _(" in transaction id %u with commit timestamp %s"),

You have a point but I think in this case it might look a bit odd as
we have another field 'commit timestamp' after that which is
lowercase.

I did a quick search and I couldn't find any other messages in the
Postgres code that use "transaction id", but I could find some that
use "transaction ID" and "transaction identifier".

Okay, but that doesn't mean using it here is bad. I am personally fine
with a message containing something like "... in transaction
id 740 with commit timestamp 2021-08-10 14:44:38.058174+05:30" but I
won't mind if you and or others find some other way convenient. Any
opinion from others?

Just to be clear, all I was saying is that I thought using uppercase
"ID" looked better in the message, and was more consistent with
existing logged messages, than using lowercase "id".
i.e. my suggestion was a trivial change:

BEFORE:
+ appendStringInfo(&buf, _(" in transaction id %u with commit timestamp %s"),
AFTER:
+ appendStringInfo(&buf, _(" in transaction ID %u with commit timestamp %s"),

But it was just a suggestion. Maybe others feel differently.

Regards,
Greg Nancarrow
Fujitsu Australia

On Fri, Aug 13, 2021 at 1:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Aug 12, 2021 at 5:41 PM Greg Nancarrow <gregn4422@gmail.com> wrote:

On Thu, Aug 12, 2021 at 9:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

A minor comment on the 0001 patch: In the message I think that using
"ID" would look better than lowercase "id" and AFAICS it's more
consistent with existing messages.

+ appendStringInfo(&buf, _(" in transaction id %u with commit timestamp %s"),

You have a point but I think in this case it might look a bit odd as
we have another field 'commit timestamp' after that which is
lowercase.

I did a quick search and I couldn't find any other messages in the
Postgres code that use "transaction id", but I could find some that
use "transaction ID" and "transaction identifier".

Okay, but that doesn't mean using it here is bad. I am personally fine
with a message containing something like "... in transaction
id 740 with commit timestamp 2021-08-10 14:44:38.058174+05:30" but I
won't mind if you and or others find some other way convenient. Any
opinion from others?

I don't have a strong opinion on this but in terms of consistency we
often use like "transaction %u" in messages when showing XID value,
rather than "transaction [id|ID|identifier]":

$ git grep -i "errmsg.*transaction %u" src/backend/
src/backend/access/transam/commit_ts.c: errmsg("cannot
retrieve commit timestamp for transaction %u", xid)));
src/backend/access/transam/slru.c: errmsg("could not
access status of transaction %u", xid),
src/backend/access/transam/slru.c: errmsg("could not
access status of transaction %u", xid),
src/backend/access/transam/slru.c: errmsg("could
not access status of transaction %u", xid),
src/backend/access/transam/slru.c: (errmsg("could
not access status of transaction %u", xid),
src/backend/access/transam/slru.c: errmsg("could
not access status of transaction %u", xid),
src/backend/access/transam/slru.c: (errmsg("could
not access status of transaction %u", xid),
src/backend/access/transam/slru.c: errmsg("could not
access status of transaction %u", xid),
src/backend/access/transam/slru.c: errmsg("could not
access status of transaction %u", xid),
src/backend/access/transam/twophase.c:
(errmsg("recovering prepared transaction %u from shared memory",
xid)));
src/backend/access/transam/twophase.c:
(errmsg("removing stale two-phase state file for transaction %u",
src/backend/access/transam/twophase.c:
(errmsg("removing stale two-phase state from memory for transaction
%u",
src/backend/access/transam/twophase.c:
(errmsg("removing future two-phase state file for transaction %u",
src/backend/access/transam/twophase.c:
(errmsg("removing future two-phase state from memory for transaction
%u",
src/backend/access/transam/twophase.c:
errmsg("corrupted two-phase state file for transaction %u",
src/backend/access/transam/twophase.c:
errmsg("corrupted two-phase state in memory for transaction %u",
src/backend/access/transam/xlog.c: (errmsg("recovery
stopping before commit of transaction %u, time %s",
src/backend/access/transam/xlog.c: (errmsg("recovery
stopping before abort of transaction %u, time %s",
src/backend/access/transam/xlog.c:
(errmsg("recovery stopping after commit of transaction %u, time %s",
src/backend/access/transam/xlog.c:
(errmsg("recovery stopping after abort of transaction %u, time %s",
src/backend/replication/logical/worker.c:
errmsg_internal("transaction %u not found in stream XID hash table",
src/backend/replication/logical/worker.c:
errmsg_internal("transaction %u not found in stream XID hash table",
src/backend/replication/logical/worker.c:
errmsg_internal("transaction %u not found in stream XID hash table",
src/backend/replication/logical/worker.c:
errmsg_internal("transaction %u not found in stream XID hash table",

$ git grep -i "errmsg.*transaction identifier" src/backend/
src/backend/access/transam/twophase.c:
errmsg("transaction identifier \"%s\" is too long",
src/backend/access/transam/twophase.c:
errmsg("transaction identifier \"%s\" is already in use",

$ git grep -i "errmsg.*transaction id" src/backend/
src/backend/access/transam/twophase.c:
errmsg("transaction identifier \"%s\" is too long",
src/backend/access/transam/twophase.c:
errmsg("transaction identifier \"%s\" is already in use",
src/backend/access/transam/varsup.c:
(errmsg_internal("transaction ID wrap limit is %u, limited by database
with OID %u",
src/backend/access/transam/xlog.c: (errmsg_internal("next
transaction ID: " UINT64_FORMAT "; next OID: %u",
src/backend/access/transam/xlog.c: (errmsg_internal("oldest
unfrozen transaction ID: %u, in database %u",
src/backend/access/transam/xlog.c: (errmsg("invalid next
transaction ID")));
src/backend/replication/logical/snapbuild.c:
(errmsg_plural("exported logical decoding snapshot: \"%s\" with %u
transaction ID",
src/backend/replication/logical/worker.c:
errmsg_internal("invalid transaction ID in streamed replication
transaction")));
src/backend/replication/logical/worker.c:
errmsg_internal("invalid transaction ID in streamed replication
transaction")));
src/backend/replication/logical/worker.c:
errmsg_internal("invalid two-phase transaction ID")));
src/backend/utils/adt/xid8funcs.c: errmsg("transaction
ID %s is in the future",

Therefore, perhaps a message like "... in transaction 740 with commit
timestamp 2021-08-10 14:44:38.058174+05:30" is better in terms of
consistency with other messages?

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Mon, Aug 16, 2021 at 6:24 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

Therefore, perhaps a message like "... in transaction 740 with commit
timestamp 2021-08-10 14:44:38.058174+05:30" is better in terms of
consistency with other messages?

Yes, I think that would be more consistent.

On another note, for the 0001 patch, the elog ERROR at the bottom of
the logicalrep_message_type() function seems to assume that the
unrecognized "action" is a printable character (with its use of %c)
and also that the character is meaningful to the user in some way.
But given that the compiler normally warns of an unhandled enum value
when switching on an enum, such an error would most likely be when
action is some int value that wouldn't be meaningful to the user (as
it wouldn't be one of the LogicalRepMsgType enum values).
I therefore think it would be better to use %d in that ERROR:

i.e.

+ elog(ERROR, "invalid logical replication message type %d", action);

Similar comments apply to the apply_dispatch() function (and I realise
it used %c before your patch).

Regards,
Greg Nancarrow
Fujitsu Australia

On Mon, Aug 16, 2021 at 1:54 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Fri, Aug 13, 2021 at 1:06 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

Okay, but that doesn't mean using it here is bad. I am personally fine
with a message containing something like "... in transaction
id 740 with commit timestamp 2021-08-10 14:44:38.058174+05:30" but I
won't mind if you and or others find some other way convenient. Any
opinion from others?

I don't have a strong opinion on this but in terms of consistency we
often use like "transaction %u" in messages when showing XID value,
rather than "transaction [id|ID|identifier]":

..

Therefore, perhaps a message like "... in transaction 740 with commit
timestamp 2021-08-10 14:44:38.058174+05:30" is better in terms of
consistency with other messages?

+1.

--
With Regards,
Amit Kapila.

On Thu, Aug 12, 2021 1:53 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the updated patches. FYI I've included the patch
(v8-0005) that fixes the assertion failure during shared fileset cleanup to make
cfbot tests happy.

Hi,

Thanks for the new patches.
I have a few comments on the v8-0001 patch.

1)
+
+	if (TransactionIdIsNormal(errarg->remote_xid))
+		appendStringInfo(&buf, _(" in transaction id %u with commit timestamp %s"),
+						 errarg->remote_xid,
+						 errarg->commit_ts == 0
+						 ? "(unset)"
+						 : timestamptz_to_str(errarg->commit_ts));
+
+	errcontext("%s", buf.data);

I think we can output the timestamp in a separete check which can be more
consistent with the other code style in apply_error_callback()
(ie)
+	if (errarg->commit_ts != 0)
+		appendStringInfo(&buf, _(" with commit timestamp %s"),
+						timestamptz_to_str(errarg->commit_ts));

2)
+/*
+ * Get string representing LogicalRepMsgType.
+ */
+char *
+logicalrep_message_type(LogicalRepMsgType action)
+{
...
+
+	elog(ERROR, "invalid logical replication message type \"%c\"", action);
+}

Some old compilers might complain that the function doesn't have a return value
at the end of the function, maybe we can code like the following:

+char *
+logicalrep_message_type(LogicalRepMsgType action)
+{
+	switch (action)
+	{
+		case LOGICAL_REP_MSG_BEGIN:
+			return "BEGIN";
...
+		default:
+			elog(ERROR, "invalid logical replication message type \"%c\"", action);
+	}
+	return NULL;				/* keep compiler quiet */
+}

3)
Do we need to invoke set_apply_error_context_xact() in the function
apply_handle_stream_prepare() to save the xid and timestamp ?

Best regards,
Hou zj

Monday, August 16, 2021 3:00 PM Hou, Zhijie wrote:

On Thu, Aug 12, 2021 1:53 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the updated patches. FYI I've included the patch
(v8-0005) that fixes the assertion failure during shared fileset
cleanup to make cfbot tests happy.

Hi,

Thanks for the new patches.
I have a few comments on the v8-0001 patch.
3)
Do we need to invoke set_apply_error_context_xact() in the function
apply_handle_stream_prepare() to save the xid and timestamp ?

Sorry, this comment wasn't correct, please ignore it.
Here is another comment:

+char *
+logicalrep_message_type(LogicalRepMsgType action)
+{
...
+		case LOGICAL_REP_MSG_STREAM_END:
+			return "STREAM END";
...

I think most the existing code use "STREAM STOP" to describe the
LOGICAL_REP_MSG_STREAM_END message, is it better to return "STREAM STOP" in
function logicalrep_message_type() too ?

Best regards,
Hou zj

On Mon, Aug 16, 2021 at 5:54 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

Here is another comment:

+char *
+logicalrep_message_type(LogicalRepMsgType action)
+{
...
+               case LOGICAL_REP_MSG_STREAM_END:
+                       return "STREAM END";
...

I think most the existing code use "STREAM STOP" to describe the
LOGICAL_REP_MSG_STREAM_END message, is it better to return "STREAM STOP" in
function logicalrep_message_type() too ?

+1
I think you're right, it should be "STREAM STOP" in that case.

Regards,
Greg Nancarrow
Fujitsu Australia

On Thu, Aug 12, 2021 at 3:54 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the updated patches. FYI I've included the patch
(v8-0005) that fixes the assertion failure during shared fileset
cleanup to make cfbot tests happy.

Another comment on the 0001 patch: as there is now a mix of setting
"apply_error_callback_arg" members directly and also through inline
functions, it might look better to have it done consistently with
functions having prototypes something like the following:

static inline void set_apply_error_context_rel(LogicalRepRelMapEntry *rel);
static inline void reset_apply_error_context_rel(void);
static inline void set_apply_error_context_attnum(int remote_attnum);

Regards,
Greg Nancarrow
Fujitsu Australia

On Mon, Aug 16, 2021 at 3:59 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

On Thu, Aug 12, 2021 1:53 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the updated patches. FYI I've included the patch
(v8-0005) that fixes the assertion failure during shared fileset cleanup to make
cfbot tests happy.

Hi,

Thanks for the new patches.
I have a few comments on the v8-0001 patch.

Thank you for the comments!

2)
+/*
+ * Get string representing LogicalRepMsgType.
+ */
+char *
+logicalrep_message_type(LogicalRepMsgType action)
+{
...
+
+       elog(ERROR, "invalid logical replication message type \"%c\"", action);
+}

Some old compilers might complain that the function doesn't have a return value
at the end of the function, maybe we can code like the following:

+char *
+logicalrep_message_type(LogicalRepMsgType action)
+{
+       switch (action)
+       {
+               case LOGICAL_REP_MSG_BEGIN:
+                       return "BEGIN";
...
+               default:
+                       elog(ERROR, "invalid logical replication message type \"%c\"", action);
+       }
+       return NULL;                            /* keep compiler quiet */
+}

Fixed.

3)
Do we need to invoke set_apply_error_context_xact() in the function
apply_handle_stream_prepare() to save the xid and timestamp ?

Yes. I think that v8-0001 patch already set xid and timestamp just
after parsing stream_prepare message. You meant it's not necessary?

I'll submit the updated patches soon.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Mon, Aug 16, 2021 at 5:30 PM Greg Nancarrow <gregn4422@gmail.com> wrote:

On Mon, Aug 16, 2021 at 5:54 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

Here is another comment:

+char *
+logicalrep_message_type(LogicalRepMsgType action)
+{
...
+               case LOGICAL_REP_MSG_STREAM_END:
+                       return "STREAM END";
...

I think most the existing code use "STREAM STOP" to describe the
LOGICAL_REP_MSG_STREAM_END message, is it better to return "STREAM STOP" in
function logicalrep_message_type() too ?

+1
I think you're right, it should be "STREAM STOP" in that case.

It's right that we use "STREAM STOP" rather than "STREAM END" in many
places such as elog messages, a callback name, and source code
comments. As far as I have found there are two places where we’re
using "STREAM STOP": LOGICAL_REP_MSG_STREAM_END and a description in
doc/src/sgml/protocol.sgml. Isn't it better to fix these
inconsistencies in the first place? I think “STREAM STOP” would be
more appropriate.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Tue, Aug 17, 2021 at 10:46 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Aug 16, 2021 at 5:30 PM Greg Nancarrow <gregn4422@gmail.com> wrote:

On Mon, Aug 16, 2021 at 5:54 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

Here is another comment:

+char *
+logicalrep_message_type(LogicalRepMsgType action)
+{
...
+               case LOGICAL_REP_MSG_STREAM_END:
+                       return "STREAM END";
...

I think most the existing code use "STREAM STOP" to describe the
LOGICAL_REP_MSG_STREAM_END message, is it better to return "STREAM STOP" in
function logicalrep_message_type() too ?

+1
I think you're right, it should be "STREAM STOP" in that case.

It's right that we use "STREAM STOP" rather than "STREAM END" in many
places such as elog messages, a callback name, and source code
comments. As far as I have found there are two places where we’re
using "STREAM STOP": LOGICAL_REP_MSG_STREAM_END and a description in
doc/src/sgml/protocol.sgml. Isn't it better to fix these
inconsistencies in the first place? I think “STREAM STOP” would be
more appropriate.

I think keeping STREAM_END in the enum 'LOGICAL_REP_MSG_STREAM_END'
seems to be a bit better because of the value 'E' we use for it.

--
With Regards,
Amit Kapila.

On Thursday, August 12, 2021 1:53 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I've attached the updated patches. FYI I've included the patch
(v8-0005) that fixes the assertion failure during shared fileset
cleanup to make cfbot tests happy.

Hi

Thanks for your patch. I met a problem when using it. The log is not what I expected in some cases, but in streaming mode, they work well.

For example:
------publisher------
create table test (a int primary key, b varchar);
create publication pub for table test;

------subscriber------
create table test (a int primary key, b varchar);
insert into test values (10000);
create subscription sub connection 'dbname=postgres port=5432' publication pub with(streaming=on);

------publisher------
insert into test values (10000);

Subscriber log:
2021-08-17 14:24:43.415 CST [3630341] ERROR: duplicate key value violates unique constraint "test_pkey"
2021-08-17 14:24:43.415 CST [3630341] DETAIL: Key (a)=(10000) already exists.

It didn't give more context info generated by apply_error_callback function.

In streaming mode(which worked as I expected):
------publisher------
INSERT INTO test SELECT i, md5(i::text) FROM generate_series(1, 10000) s(i);

Subscriber log:
2021-08-17 14:26:26.521 CST [3630510] ERROR: duplicate key value violates unique constraint "test_pkey"
2021-08-17 14:26:26.521 CST [3630510] DETAIL: Key (a)=(10000) already exists.
2021-08-17 14:26:26.521 CST [3630510] CONTEXT: processing remote data during "INSERT" for replication target relation "public.test" in transaction id 710 with commit timestamp 2021-08-17 14:26:26.403214+08

I looked into it briefly and thought it was related to some code in
apply_dispatch function. It set callback when apply_error_callback_arg.command
is 0, and reset the callback back at the end of the function. But
apply_error_callback_arg.command was not reset to 0, so it won't set callback
when calling apply_dispatch function next time.

I tried to fix it with the following change, thoughts?

@@ -2455,7 +2455,10 @@ apply_dispatch(StringInfo s)

        /* Pop the error context stack */
        if (set_callback)
+       {
                error_context_stack = errcallback.previous;
+               apply_error_callback_arg.command = 0;
+       }
 }

Besides, if we make the changes like this, do we still need to reset
apply_error_callback_arg.command in reset_apply_error_context_info function?

Regards
Tang

On Tue, Aug 17, 2021 at 2:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, Aug 17, 2021 at 10:46 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Aug 16, 2021 at 5:30 PM Greg Nancarrow <gregn4422@gmail.com> wrote:

On Mon, Aug 16, 2021 at 5:54 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

Here is another comment:

+char *
+logicalrep_message_type(LogicalRepMsgType action)
+{
...
+               case LOGICAL_REP_MSG_STREAM_END:
+                       return "STREAM END";
...

I think most the existing code use "STREAM STOP" to describe the
LOGICAL_REP_MSG_STREAM_END message, is it better to return "STREAM STOP" in
function logicalrep_message_type() too ?

+1
I think you're right, it should be "STREAM STOP" in that case.

It's right that we use "STREAM STOP" rather than "STREAM END" in many
places such as elog messages, a callback name, and source code
comments. As far as I have found there are two places where we’re
using "STREAM STOP": LOGICAL_REP_MSG_STREAM_END and a description in
doc/src/sgml/protocol.sgml. Isn't it better to fix these
inconsistencies in the first place? I think “STREAM STOP” would be
more appropriate.

I think keeping STREAM_END in the enum 'LOGICAL_REP_MSG_STREAM_END'
seems to be a bit better because of the value 'E' we use for it.

But I think we don't care about the actual value of
LOGICAL_REP_MSG_STREAM_END since we use the enum value rather than
'E'?

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Wed, Aug 18, 2021 at 6:53 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Aug 17, 2021 at 2:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

It's right that we use "STREAM STOP" rather than "STREAM END" in many
places such as elog messages, a callback name, and source code
comments. As far as I have found there are two places where we’re
using "STREAM STOP": LOGICAL_REP_MSG_STREAM_END and a description in
doc/src/sgml/protocol.sgml. Isn't it better to fix these
inconsistencies in the first place? I think “STREAM STOP” would be
more appropriate.

I think keeping STREAM_END in the enum 'LOGICAL_REP_MSG_STREAM_END'
seems to be a bit better because of the value 'E' we use for it.

But I think we don't care about the actual value of
LOGICAL_REP_MSG_STREAM_END since we use the enum value rather than
'E'?

True, but here we are trying to be consistent with other enum values
where we try to use the first letter of the last word (which is E in
this case). I can see there are other cases where we are not
consistent so it won't be a big deal if we won't be consistent here. I
am neutral on this one, so, if you feel using STREAM_STOP would be
better from a code readability perspective then that is fine.

--
With Regards,
Amit Kapila.

On Wed, Aug 18, 2021 at 12:02 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Aug 18, 2021 at 6:53 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Aug 17, 2021 at 2:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

It's right that we use "STREAM STOP" rather than "STREAM END" in many
places such as elog messages, a callback name, and source code
comments. As far as I have found there are two places where we’re
using "STREAM STOP": LOGICAL_REP_MSG_STREAM_END and a description in
doc/src/sgml/protocol.sgml. Isn't it better to fix these
inconsistencies in the first place? I think “STREAM STOP” would be
more appropriate.

I think keeping STREAM_END in the enum 'LOGICAL_REP_MSG_STREAM_END'
seems to be a bit better because of the value 'E' we use for it.

But I think we don't care about the actual value of
LOGICAL_REP_MSG_STREAM_END since we use the enum value rather than
'E'?

True, but here we are trying to be consistent with other enum values
where we try to use the first letter of the last word (which is E in
this case). I can see there are other cases where we are not
consistent so it won't be a big deal if we won't be consistent here. I
am neutral on this one, so, if you feel using STREAM_STOP would be
better from a code readability perspective then that is fine.

In addition of a code readability, there is a description in the doc
that mentions "Stream End" but we describe "Stream Stop" in the later
description, which seems a bug in the doc to me:

The following messages (Stream Start, Stream End, Stream Commit, and
Stream Abort) are available since protocol version 2.

</para>

(snip)

<varlistentry>
<term>
Stream Stop
</term>
<listitem>

Perhaps it's better to hear other opinions too, but I've attached the
patch. Please review it.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Wed, Aug 18, 2021 at 10:00 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Aug 18, 2021 at 12:02 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Aug 18, 2021 at 6:53 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Aug 17, 2021 at 2:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

It's right that we use "STREAM STOP" rather than "STREAM END" in many
places such as elog messages, a callback name, and source code
comments. As far as I have found there are two places where we’re
using "STREAM STOP": LOGICAL_REP_MSG_STREAM_END and a description in
doc/src/sgml/protocol.sgml. Isn't it better to fix these
inconsistencies in the first place? I think “STREAM STOP” would be
more appropriate.

I think keeping STREAM_END in the enum 'LOGICAL_REP_MSG_STREAM_END'
seems to be a bit better because of the value 'E' we use for it.

But I think we don't care about the actual value of
LOGICAL_REP_MSG_STREAM_END since we use the enum value rather than
'E'?

True, but here we are trying to be consistent with other enum values
where we try to use the first letter of the last word (which is E in
this case). I can see there are other cases where we are not
consistent so it won't be a big deal if we won't be consistent here. I
am neutral on this one, so, if you feel using STREAM_STOP would be
better from a code readability perspective then that is fine.

In addition of a code readability, there is a description in the doc
that mentions "Stream End" but we describe "Stream Stop" in the later
description, which seems a bug in the doc to me:

Doc changes looks good to me. But, I have question for code change:

--- a/src/include/replication/logicalproto.h
+++ b/src/include/replication/logicalproto.h
@@ -65,7 +65,7 @@ typedef enum LogicalRepMsgType
  LOGICAL_REP_MSG_COMMIT_PREPARED = 'K',
  LOGICAL_REP_MSG_ROLLBACK_PREPARED = 'r',
  LOGICAL_REP_MSG_STREAM_START = 'S',
- LOGICAL_REP_MSG_STREAM_END = 'E',
+ LOGICAL_REP_MSG_STREAM_STOP = 'E',
  LOGICAL_REP_MSG_STREAM_COMMIT = 'c',

As this is changing the enum name and if any extension (logical
replication extension) has started using it then they would require a
change. As this is the latest change in PG-14, so it might be okay but
OTOH, as this is just a code readability change, shall we do it only
for PG-15?

--
With Regards,
Amit Kapila.

On Tues, Aug 17, 2021 1:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Aug 16, 2021 at 3:59 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:

3)
Do we need to invoke set_apply_error_context_xact() in the function
apply_handle_stream_prepare() to save the xid and timestamp ?

Yes. I think that v8-0001 patch already set xid and timestamp just after parsing
stream_prepare message. You meant it's not necessary?

Sorry, I thought of something wrong, please ignore the above comment.

I'll submit the updated patches soon.

I was thinking about the place to set the errcallback.callback.

apply_dispatch(StringInfo s)
 {
 	LogicalRepMsgType action = pq_getmsgbyte(s);
+	ErrorContextCallback errcallback;
+	bool		set_callback = false;
+
+	/*
+	 * Push apply error context callback if not yet. Other fields will be
+	 * filled during applying the change.  Since this function can be called
+	 * recursively when applying spooled changes, we set the callback only
+	 * once.
+	 */
+	if (apply_error_callback_arg.command == 0)
+	{
+		errcallback.callback = apply_error_callback;
+		errcallback.previous = error_context_stack;
+		error_context_stack = &errcallback;
+		set_callback = true;
+	}
...
+	/* Pop the error context stack */
+	if (set_callback)
+		error_context_stack = errcallback.previous;

It seems we can put the above code in the function LogicalRepApplyLoop()
around invoking apply_dispatch(), and in that approach we don't need to worry
about the recursively case. What do you think ?

Best regards,
Hou zj

On Wed, Aug 18, 2021 at 3:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Aug 18, 2021 at 10:00 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Aug 18, 2021 at 12:02 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Aug 18, 2021 at 6:53 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Aug 17, 2021 at 2:35 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

It's right that we use "STREAM STOP" rather than "STREAM END" in many
places such as elog messages, a callback name, and source code
comments. As far as I have found there are two places where we’re
using "STREAM STOP": LOGICAL_REP_MSG_STREAM_END and a description in
doc/src/sgml/protocol.sgml. Isn't it better to fix these
inconsistencies in the first place? I think “STREAM STOP” would be
more appropriate.

I think keeping STREAM_END in the enum 'LOGICAL_REP_MSG_STREAM_END'
seems to be a bit better because of the value 'E' we use for it.

But I think we don't care about the actual value of
LOGICAL_REP_MSG_STREAM_END since we use the enum value rather than
'E'?

True, but here we are trying to be consistent with other enum values
where we try to use the first letter of the last word (which is E in
this case). I can see there are other cases where we are not
consistent so it won't be a big deal if we won't be consistent here. I
am neutral on this one, so, if you feel using STREAM_STOP would be
better from a code readability perspective then that is fine.

In addition of a code readability, there is a description in the doc
that mentions "Stream End" but we describe "Stream Stop" in the later
description, which seems a bug in the doc to me:

Doc changes looks good to me. But, I have question for code change:

--- a/src/include/replication/logicalproto.h
+++ b/src/include/replication/logicalproto.h
@@ -65,7 +65,7 @@ typedef enum LogicalRepMsgType
LOGICAL_REP_MSG_COMMIT_PREPARED = 'K',
LOGICAL_REP_MSG_ROLLBACK_PREPARED = 'r',
LOGICAL_REP_MSG_STREAM_START = 'S',
- LOGICAL_REP_MSG_STREAM_END = 'E',
+ LOGICAL_REP_MSG_STREAM_STOP = 'E',
LOGICAL_REP_MSG_STREAM_COMMIT = 'c',

As this is changing the enum name and if any extension (logical
replication extension) has started using it then they would require a
change. As this is the latest change in PG-14, so it might be okay but
OTOH, as this is just a code readability change, shall we do it only
for PG-15?

I think that the doc changes could be backpatched to PG14 but I think
we should do the code change only for PG15.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Wed, Aug 18, 2021 2:41 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Aug 18, 2021 at 3:15 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Wed, Aug 18, 2021 at 10:00 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

In addition of a code readability, there is a description in the doc
that mentions "Stream End" but we describe "Stream Stop" in the
later description, which seems a bug in the doc to me:

Doc changes looks good to me. But, I have question for code change:

--- a/src/include/replication/logicalproto.h
+++ b/src/include/replication/logicalproto.h
@@ -65,7 +65,7 @@ typedef enum LogicalRepMsgType
LOGICAL_REP_MSG_COMMIT_PREPARED = 'K',
LOGICAL_REP_MSG_ROLLBACK_PREPARED = 'r',
LOGICAL_REP_MSG_STREAM_START = 'S',
- LOGICAL_REP_MSG_STREAM_END = 'E',
+ LOGICAL_REP_MSG_STREAM_STOP = 'E',
LOGICAL_REP_MSG_STREAM_COMMIT = 'c',

As this is changing the enum name and if any extension (logical
replication extension) has started using it then they would require a
change. As this is the latest change in PG-14, so it might be okay but
OTOH, as this is just a code readability change, shall we do it only
for PG-15?

I think that the doc changes could be backpatched to PG14 but I think we
should do the code change only for PG15.

+1, and the patch looks good to me.

Best regards,
Hou zj

On Wed, Aug 18, 2021 at 3:33 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

On Tues, Aug 17, 2021 1:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Aug 16, 2021 at 3:59 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:

3)
Do we need to invoke set_apply_error_context_xact() in the function
apply_handle_stream_prepare() to save the xid and timestamp ?

Yes. I think that v8-0001 patch already set xid and timestamp just after parsing
stream_prepare message. You meant it's not necessary?

Sorry, I thought of something wrong, please ignore the above comment.

I'll submit the updated patches soon.

I was thinking about the place to set the errcallback.callback.

apply_dispatch(StringInfo s)
{
LogicalRepMsgType action = pq_getmsgbyte(s);
+       ErrorContextCallback errcallback;
+       bool            set_callback = false;
+
+       /*
+        * Push apply error context callback if not yet. Other fields will be
+        * filled during applying the change.  Since this function can be called
+        * recursively when applying spooled changes, we set the callback only
+        * once.
+        */
+       if (apply_error_callback_arg.command == 0)
+       {
+               errcallback.callback = apply_error_callback;
+               errcallback.previous = error_context_stack;
+               error_context_stack = &errcallback;
+               set_callback = true;
+       }
...
+       /* Pop the error context stack */
+       if (set_callback)
+               error_context_stack = errcallback.previous;

It seems we can put the above code in the function LogicalRepApplyLoop()
around invoking apply_dispatch(), and in that approach we don't need to worry
about the recursively case. What do you think ?

Thank you for the comment!

I think you're right. Maybe we can set the callback before entering to
the main loop and pop it after breaking from it. It would also fix the
problem reported by Tang[1]/messages/by-id/OS0PR01MB6113E5BC24922A2D05D16051FBFE9@OS0PR01MB6113.jpnprd01.prod.outlook.com. But one thing we need to note that since
we want to reset apply_error_callback_arg.command at the end of
apply_dispatch() (otherwise we could end up setting the apply error
context to an irrelevant error such as network error), when
apply_dispatch() is called recursively probably we need to save the
apply_error_callback_arg.command before setting the new command and
then revert back to the saved command. Is that right?

Regards,

[1]: /messages/by-id/OS0PR01MB6113E5BC24922A2D05D16051FBFE9@OS0PR01MB6113.jpnprd01.prod.outlook.com

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

On Wed, Aug 18, 2021 at 5:39 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Aug 18, 2021 at 3:33 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:

On Tues, Aug 17, 2021 1:01 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Mon, Aug 16, 2021 at 3:59 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com> wrote:

3)
Do we need to invoke set_apply_error_context_xact() in the function
apply_handle_stream_prepare() to save the xid and timestamp ?

Yes. I think that v8-0001 patch already set xid and timestamp just after parsing
stream_prepare message. You meant it's not necessary?

Sorry, I thought of something wrong, please ignore the above comment.

I'll submit the updated patches soon.

I was thinking about the place to set the errcallback.callback.

apply_dispatch(StringInfo s)
{
LogicalRepMsgType action = pq_getmsgbyte(s);
+       ErrorContextCallback errcallback;
+       bool            set_callback = false;
+
+       /*
+        * Push apply error context callback if not yet. Other fields will be
+        * filled during applying the change.  Since this function can be called
+        * recursively when applying spooled changes, we set the callback only
+        * once.
+        */
+       if (apply_error_callback_arg.command == 0)
+       {
+               errcallback.callback = apply_error_callback;
+               errcallback.previous = error_context_stack;
+               error_context_stack = &errcallback;
+               set_callback = true;
+       }
...
+       /* Pop the error context stack */
+       if (set_callback)
+               error_context_stack = errcallback.previous;

It seems we can put the above code in the function LogicalRepApplyLoop()
around invoking apply_dispatch(), and in that approach we don't need to worry
about the recursively case. What do you think ?

Thank you for the comment!

I think you're right. Maybe we can set the callback before entering to
the main loop and pop it after breaking from it. It would also fix the
problem reported by Tang[1]. But one thing we need to note that since
we want to reset apply_error_callback_arg.command at the end of
apply_dispatch() (otherwise we could end up setting the apply error
context to an irrelevant error such as network error), when
apply_dispatch() is called recursively probably we need to save the
apply_error_callback_arg.command before setting the new command and
then revert back to the saved command. Is that right?

I've attached the updated version patches that incorporated all
comments I got so far unless I'm missing something. Please review
them.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/