Hi
On Tuesday, March 16, 2021 1:35 AM Oh, Mike <min...@amazon.com> wrote:
> We noticed that the logical replication could fail when the
> Standby::RUNNING_XACT record is generated in the middle of a catalog
> modifying transaction and if the logical decoding has to restart from the
> RUNNING_XACT
> WAL entry.
...
> Proposed solution:
> If we’re decoding a catalog modifying commit record, then check whether
> it’s part of the RUNNING_XACT xid’s processed @ the restart_lsn. If so,
> then add its xid & subxacts in the committed txns list in the logical decoding
> snapshot.
>
> Please refer to the attachment for the proposed patch.
Let me share some review comments for the patch.
(1) last_running declaration
Isn't it better to add static for this variable,
because we don't use this in other places ?
@@ -85,6 +86,9 @@ static bool DecodeTXNNeedSkip(LogicalDecodingContext *ctx,
XLogRecordBuffer
*buf, Oid dbId,
RepOriginId
origin_id);
+/* record previous restart_lsn running xacts */
+xl_running_xacts *last_running = NULL;
(2) DecodeStandbyOp's memory free
I'm not sure when
we pass this condition with already allocated last_running,
but do you need to free it's xid array here as well,
if last_running isn't null ?
Otherwise, we'll miss the chance after this.
+ /* record restart_lsn running xacts */
+ if (MyReplicationSlot && (buf->origptr ==
MyReplicationSlot->data.restart_lsn))
+ {
+ if (last_running)
+ free(last_running);
+
+ last_running = NULL;
(3) suggestion of small readability improvement
We calculate the same size twice here and DecodeCommit.
I suggest you declare a variable that stores the computed result of size,
which might shorten those codes.
+ /*
+ * xl_running_xacts contains a xids
Flexible Array
+ * and its size is subxcnt + xcnt.
+ * Take that into account while
allocating
+ * the memory for last_running.
+ */
+ last_running = (xl_running_xacts *)
malloc(sizeof(xl_running_xacts)
+
+ sizeof(TransactionId )
+
* (running->subxcnt +
running->xcnt));
+ memcpy(last_running, running,
sizeof(xl_running_xacts)
+
+ (sizeof(TransactionId)
+
* (running->subxcnt + running->xcnt)));
Best Regards,
Takamichi Osumi
