Re: Proposal: Conflict log history table for Logical Replication

A few tirival comments for v81:

v81-001:
1)
insert_conflict_log_tuple:

+ if (TransactionIdIsValid(remote_xid))
+ values[attno++] = TransactionIdGetDatum(remote_xid);
+ else
+ nulls[attno++] = true;

Would it better to replace it with:

/*
* The remote xid is always valid here: a conflict is only detected while
* applying a row change, which is preceded by a BEGIN, BEGIN PREPARE or
* STREAM START that recorded a valid top-level xid.
*/
Assert(TransactionIdIsValid(remote_xid));
values[attno++] = TransactionIdGetDatum(remote_xid);

v81-002:
2)
drop_subscription.sgml:

"If a conflict log table exists for the subscription (that is, when
conflict_log_destination is set to table or all), DROP SUBSCRIPTION
automatically drops the associated conflict log table. In that case,
the subscription cannot be dropped while it has prepared transactions
(see two_phase), because they may hold a lock on the conflict log
table that is released only when the apply worker applies the
corresponding COMMIT PREPARED or ROLLBACK PREPARED. Resolve these
transactions first (see pg_prepared_xacts)."

Above doc seems misleading. It gives an impression that CLT is dropped
while sub can not be dropped in presence of prepared txns.

Should we change it to:
If conflict_log_destination is set to table or all, the subscription
has an associated conflict log table, which DROP SUBSCRIPTION drops
along with the subscription. However, DROP SUBSCRIPTION cannot be
executed while the subscription has prepared transactions (see
two_phase), because such a transaction may hold a lock on the conflict
log table that is released only when the apply worker processes the
corresponding COMMIT PREPARED or ROLLBACK PREPARED. Resolve these
transactions first (see pg_prepared_xacts) before dropping the
subscription.

3)
- "SELECT replica_identity::text FROM $clt_BA WHERE relname = 'tab_temporal';");
-is($clt_row_temporal,
- '{"id":"[1,2)","valid_at":"{[2020-01-01,2021-01-01)}"}',
- 'replica identity uses table column type rather than index opckeytype'
+ "SELECT replica_identity::text FROM $clt_BA WHERE relname = 'tab_temporal';"
);
+is( $clt_row_temporal,
+ '{"id":"[1,2)","valid_at":"{[2020-01-01,2021-01-01)}"}',
+ 'replica identity uses table column type rather than index opckeytype');

Why this change in 002, do we need it?

4)
The test added in 0002 does not cover apply-worker's error in
lock_conflictlog_relation(). Is that intentionally left?

thanks
Shveta