Hi Haiyang,
1 ====
@@ -1301,6 +1338,7 @@ DecodeTXNNeedSkip(LogicalDecodingContext *ctx, XLogRecordBuffer *buf,
Oid txn_dbid, RepOriginId origin_id)
{
if (SnapBuildXactNeedsSkip(ctx->snapshot_builder, buf->origptr) ||
+ SnapBuildCurrentState(ctx->snapshot_builder) < SNAPBUILD_CONSISTENT ||
(txn_dbid != InvalidOid && txn_dbid != ctx->slot->data.database) ||
FilterByOrigin(ctx, origin_id))
return true;
It seems that this change will cause unexpected bahavior. For expample, it will make the xact commited in SNAPBUILD_FULL_SNAPSHOT
to be skipped in DecodeCommit(). Do you mean "SnapBuildCurrentState(ctx->snapshot_builder) < SNAPBUILD_FULL_SNAPSHOT"?
I think it's correct. Note that we only decode txns commited after SNAPBUILD_CONSISTENT, txns commited before SNAPBUILD_CONSISTENT will not be decoded even if they start after SNAPBUILD_FULL_SNAPSHOT. I add this for safe because we now need handle xlog records in xact_decode() when building snapshot, but we didn't handle them before. We should make sure that processing those xlog records has no side effects.
3===
@@ -416,7 +423,22 @@ heap2_decode(LogicalDecodingContext *ctx, XLogRecordBuffer *buf)
*/
if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT ||
ctx->fast_forward)
+ {
+ /*
+ * Note that during or after BUILDING_SNAPSHOT, we need handle the xlog
+ * that might mark a transaction as catalog modifying because the snapshot
+ * only tracks catalog modifying transactions. The transaction before
+ * BUILDING_SNAPSHOT will not be tracked anyway(see SnapBuildCommitTxn()
+ * for details), so just return.
+ */
+ if (SnapBuildCurrentState(builder) >= SNAPBUILD_BUILDING_SNAPSHOT)
+ {
+ /* Currently only XLOG_HEAP2_NEW_CID means a catalog modifying */
+ if (info == XLOG_HEAP2_NEW_CID && TransactionIdIsValid(xid))
+ ReorderBufferXidSetCatalogChanges(ctx->reorder, xid, buf->origptr);
+ }
return;
+ }
It should be removed from v18 and master because commit 243e9b4 intrduced.
Agreed. (But it seems to come from an outdated patch (before v4-0001))
4===
IMUC, v4-0001 patch is to only mark transactions with catalog changes during the buildsnapshot phase,
without performing any other actions. I’m wondering if we should make the behavior of snapshot building
the same in both SNAPBUILD_BUILDING_SNAPSHOT and SNAPBUILD_FULL_SNAPSHOT states,
except in cases where a base snapshot is needed. If so, the attached patch is for the master version.
My thought is marking transactions with catalog changes and call DecodeCommit() to build the snapshot correctly when we are in SNAPBUILD_BUILDING_SNAPSHOT.
The v5-0001 seems to based on an outdated master.
--
Regards,
ChangAo Chen
