On Fri, Mar 11, 2022 4:20 PM Masahiko Sawada <[email protected]> wrote:
>
> I've attached an updated version patch. This patch can be applied on
> top of the latest disable_on_error patch[1].
>
Thanks for your patch. Here are some comments for the v13 patch.
1. doc/src/sgml/ref/alter_subscription.sgml
+ Specifies the transaction's finish LSN of the remote transaction
whose changes
Could it be simplified to "Specifies the finish LSN of the remote transaction
whose ...".
2.
I met a failed assertion, the backtrace is attached. This is caused by the
following code in maybe_start_skipping_changes().
+ /*
+ * It's a rare case; a past subskiplsn was left because the
server
+ * crashed after preparing the transaction and before clearing
the
+ * subskiplsn. We clear it without a warning message so as not
confuse
+ * the user.
+ */
+ if (unlikely(MySubscription->skiplsn < lsn))
+ {
+ clear_subscription_skip_lsn(MySubscription->skiplsn,
InvalidXLogRecPtr, 0,
+
false);
+ Assert(!IsTransactionState());
+ }
We want to clear subskiplsn in the case mentioned in comment. But if the next
transaction is a steaming transaction and this function is called by
apply_spooled_messages(), we are inside a transaction here. So, I think this
assertion is not suitable for streaming transaction. Thoughts?
3.
+ XLogRecPtr subskiplsn; /* All changes which committed
at this LSN are
+ * skipped */
To be consistent, should the comment be changed to "All changes which finished
at this LSN are skipped"?
4.
+ After logical replication worker successfully skips the transaction or
commits
+ non-empty transaction, the LSN (stored in
+
<structname>pg_subscription</structname>.<structfield>subskiplsn</structfield>)
+ is cleared.
Besides "commits non-empty transaction", subskiplsn would also be cleared in
some two-phase commit cases I think. Like prepare/commit/rollback a transaction,
even if it is an empty transaction. So, should we change it for these cases?
5.
+ * Clear subskiplsn of pg_subscription catalog with origin state update.
Should "with origin state update" modified to "with origin state updated"?
Regards,
Shi yu
#0 0x00007fe3d1db170f in raise () from /lib64/libc.so.6
#1 0x00007fe3d1d9bb25 in abort () from /lib64/libc.so.6
#2 0x0000000000e0a657 in ExceptionalCondition
(conditionName=conditionName@entry=0xfe9265 "!IsTransactionState()",
errorType=errorType@entry=0xeb6917 "FailedAssertion",
fileName=fileName@entry=0xfdc44c "worker.c",
lineNumber=lineNumber@entry=3846) at assert.c:69
#3 0x0000000000ae5a52 in maybe_start_skipping_changes (lsn=lsn@entry=22983032)
at worker.c:3846
#4 0x0000000000aedd35 in apply_spooled_messages (xid=xid@entry=722,
lsn=22983032) at worker.c:1385
#5 0x0000000000aee3b0 in apply_handle_stream_commit (s=s@entry=0x7ffd1354ee00)
at worker.c:1486
#6 0x0000000000aecf83 in apply_dispatch (s=s@entry=0x7ffd1354ee00) at
worker.c:2520
#7 0x0000000000aed41c in LogicalRepApplyLoop (last_received=22983080,
last_received@entry=0) at worker.c:2751
#8 0x0000000000aedb25 in start_apply (origin_startpos=0) at worker.c:3514
#9 0x0000000000aef656 in ApplyWorkerMain (main_arg=<optimized out>) at
worker.c:3770
#10 0x0000000000a72881 in StartBackgroundWorker () at bgworker.c:858
#11 0x0000000000a8e159 in do_start_bgworker (rw=rw@entry=0x27e99f0) at
postmaster.c:5916
#12 0x0000000000a8e30a in maybe_start_bgworkers () at postmaster.c:6141
#13 0x0000000000a8f833 in sigusr1_handler (postgres_signal_arg=<optimized out>)
at postmaster.c:5305
#14 <signal handler called>
#15 0x00007fe3d1e6d4ab in select () from /lib64/libc.so.6
#16 0x0000000000a90d68 in ServerLoop () at postmaster.c:1765
#17 0x0000000000a933c7 in PostmasterMain (argc=argc@entry=3,
argv=argv@entry=0x27bdaf0) at postmaster.c:1473
#18 0x00000000009203c4 in main (argc=3, argv=0x27bdaf0) at main.c:202
