Re: Conflict detection for update_deleted in logical replication

Please find some more comments:

1)
In CheckSubDeadTupleRetention(), shall we have below instead of
retain_dead_tuples check in all conditions?

if (retain_dead_tuples)
    guc checks (wal_level and tracl_commit)
else
   max retention check

2)
Currently stop and resume messages are:
~~
LOG:  logical replication worker for subscription "sub2" has stopped
retaining the information for detecting conflicts
DETAIL:  The retention duration for information used in conflict
detection has exceeded the maximum limit of 10000 ms.
HINT:  You might need to increase "max_conflict_retention_duration".
 --
LOG:  logical replication worker for subscription "sub2" will resume
retaining the information for detecting conflicts
DETAIL:  The retention duration for information used in conflict
detection is now within the acceptable limit of 10000 ms.
~~

Resume message does not mention GUC while stop does mention it in
HINT. Shall we have both stop and resume DETAIL msg mention GUC as:

Stop:
DETAIL: Retention of information used for conflict detection has
exceeded max_conflict_retention_duration of 10000 ms.

Resume:
DETAIL: Retention of information used for conflict detection is now
within the max_conflict_retention_duration of 1000 ms.

I think we should get rid of HINT in stop msg as that is not what we
actually should be suggesting without knowing the
system-workload/bloat condition. Hint seems oversimplified and thus
incomplete looking at the possibilities we may have here.


3)
CREATE SUBSCRIPTION sub CONNECTION '...' PUBLICATION pub WITH (connect
= false, retain_dead_tuples = true, max_conflict_retention_duration =
5000);
WARNING:  deleted rows to detect conflicts would not be removed until
the subscription is enabled
HINT:  Consider setting retain_dead_tuples to false.
WARNING:  subscription was created, but is not connected
HINT:  To initiate replication, you must manually create the
replication slot, enable the subscription, and refresh the
subscription.
CREATE SUBSCRIPTION

With connect=false, we get above messages. Reverse order of WARNINGs
will make more sense as 'not connected' WARNING and HINT clarifies a
few things including that the sub is disabled and needs to be enabled.
Can we attempt doing it provided it does not over-complicate code?

4)
postgres=# \dRs+
     List of subscriptions
.. | Retain dead tuples | Max conflict retention duration | Dead tuple
retention active |..

Here shall we have  'Max retention duration' and 'Retention Active'
instead of 'Max conflict retention duration' and 'Dead tuple retention
active'?

thanks
Shveta