Subject: Re: Introduce XID age and inactive timeout based replication slot invalidation

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 8 Mar 2024 20:08:19 +0530

On Wed, Mar 6, 2024 at 4:28 PM Amit Kapila <[email-masked]> wrote:
>
> IIUC, the current conflict_reason is primarily used to determine
> logical slots on standby that got invalidated due to recovery time
> conflict. On the primary, it will also show logical slots that got
> invalidated due to the corresponding WAL got removed. Is that
> understanding correct?

That's right.

> If so, we are already sort of overloading this
> column. However, now adding more invalidation reasons that won't
> happen during recovery conflict handling will change entirely the
> purpose (as per the name we use) of this variable. I think
> invalidation_reason could depict this column correctly but OTOH I
> guess it would lose its original meaning/purpose.

Hm. I get the concern. Are you okay with having inavlidation_reason
separately for both logical and physical slots? In such a case,
logical slots that got invalidated on the standby will have duplicate
info in conflict_reason and invalidation_reason, is this fine?

Another idea is to make 'conflict_reason text' as a 'conflicting
boolean' again (revert 007693f2a3), and have 'invalidation_reason
text' for both logical and physical slots. So, whenever 'conflicting'
is true, one can look at invalidation_reason for the reason for
conflict. How does this sound?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Amit Kapila <[email-masked]>
Date: Mon, 11 Mar 2024 11:25:57 +0530

> info in conflict_reason and invalidation_reason, is this fine?

If we have duplicate information in two columns that could be
confusing for users. BTW, isn't the recovery conflict occur only
because of rows_removed and wal_level_insufficient reasons? The
wal_removed or the new reasons you are proposing can't happen because
of recovery conflict. Am, I missing something here?

> conflict. How does this sound?

So, does this mean that conflicting will only be true for some of the
reasons (say wal_level_insufficient, rows_removed, wal_removed) and
logical slots but not for others? I think that will also not eliminate
the duplicate information as user could have deduced that from single
column

-- 
With Regards,
Amit Kapila.

---

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 12 Mar 2024 20:55:37 +0530

> of recovery conflict. Am, I missing something here?

My understanding aligns with yours that the rows_removed and
wal_level_insufficient invalidations can occur only upon recovery
conflict.

FWIW, a test named 'synchronized slot has been invalidated' in
040_standby_failover_slots_sync.pl inappropriately uses
conflict_reason = 'wal_removed' logical slot on standby. As per the
above understanding, it's inappropriate to use conflict_reason here
because wal_removed invalidation doesn't conflict with recovery.

> column.

So, how about we turn conflict_reason to only report the reasons that
actually cause conflict with recovery for logical slots, something
like below, and then have invalidation_cause as a generic column for
all sorts of invalidation reasons for both logical and physical slots?

ReplicationSlotInvalidationCause cause = slot_contents.data.invalidated;

if (slot_contents.data.database == InvalidOid ||
    cause == RS_INVAL_NONE ||
    cause != RS_INVAL_HORIZON ||
    cause != RS_INVAL_WAL_LEVEL)
{
    nulls[i++] = true;
}
else
{
    Assert(cause == RS_INVAL_HORIZON || cause == RS_INVAL_WAL_LEVEL);

    values[i++] = CStringGetTextDatum(SlotInvalidationCauses[cause]);
}

---

From: Amit Kapila <[email-masked]>
Date: Wed, 13 Mar 2024 09:21:32 +0530

> all sorts of invalidation reasons for both logical and physical slots?

If our above understanding is correct then coflict_reason will be a
subset of invalidation_reason. If so, whatever way we arrange this
information, there will be some sort of duplicity unless we just have
one column 'invalidation_reason' and update the docs to interpret it
correctly for conflicts.

-- 
With Regards,
Amit Kapila.

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 13 Mar 2024 21:24:17 +0530

> one column 'invalidation_reason' and update the docs to interpret it

Yes, there will be some sort of duplicity if we emit conflict_reason
as a text field. However, I still think the better way is to turn
conflict_reason text to conflict boolean and set it to true only on
rows_removed and wal_level_insufficient invalidations. When conflict
boolean is true, one (including all the tests that we've added
recently) can look for invalidation_reason text field for the reason.
This sounds reasonable to me as opposed to we just mentioning in the
docs that "if invalidation_reason is rows_removed or
wal_level_insufficient it's the reason for conflict with recovery".

Thoughts?

---

From: Amit Kapila <[email-masked]>
Date: Thu, 14 Mar 2024 12:24:00 +0530

> wal_level_insufficient it's the reason for conflict with recovery".

Fair point. I think we can go either way. Bertrand, Nathan, and
others, do you have an opinion on this matter?

-- 
With Regards,
Amit Kapila.

---

From: Bharath Rupireddy <[email-masked]>
Date: Thu, 14 Mar 2024 19:57:46 +0530

> others, do you have an opinion on this matter?

While we wait to hear from others on this, I'm attaching the v9 patch
set implementing the above idea (check 0001 patch). Please have a
look. I'll come back to the other review comments soon.

---

From: shveta malik <[email-masked]>
Date: Fri, 15 Mar 2024 10:14:49 +0530

> > > wal_level_insufficient it's the reason for conflict with recovery".

+1 on maintaining both conflicting and invalidation_reason

> look. I'll come back to the other review comments soon.

Thanks for the patch. JFYI, patch09 does not apply to HEAD, some
recent commit caused the conflict.

Some trivial comments on patch001 (yet to review other patches)

1)
info.c:

- "%s as caught_up, conflict_reason IS NOT NULL as invalid "
+ "%s as caught_up, invalidation_reason IS NOT NULL as invalid "

Can we revert back to 'conflicting as invalid' since it is a query for
logical slots only.

2)
040_standby_failover_slots_sync.pl:

- q{SELECT conflict_reason IS NULL AND synced AND NOT temporary FROM
pg_replication_slots WHERE slot_name = 'lsub1_slot';}
+ q{SELECT invalidation_reason IS NULL AND synced AND NOT temporary
FROM pg_replication_slots WHERE slot_name = 'lsub1_slot';}

Here too, can we have 'NOT conflicting' instead of '
invalidation_reason IS NULL' as it is a logical slot test.

thanks
Shveta

---

From: shveta malik <[email-masked]>
Date: Fri, 15 Mar 2024 12:49:07 +0530

> look. I'll come back to the other review comments soon.

patch002:

1)
I would like to understand the purpose of 'inactive_count'? Is it only
for users for monitoring purposes? We are not using it anywhere
internally.

I shutdown the instance 5 times and found that 'inactive_count' became
5 for all the slots created on that instance. Is this intentional? I
mean we can not really use them if the instance is down.  I felt it
should increment the inactive_count only if during the span of
instance, they were actually inactive i.e. no streaming or replication
happening through them.

2)
slot.c:
+ case RS_INVAL_XID_AGE:
+ {
+ if (TransactionIdIsNormal(s->data.xmin))
+ {
+                          ..........
+ }
+ if (TransactionIdIsNormal(s->data.catalog_xmin))
+ {
+                          ..........
+ }
+ }

Can we optimize this code? It has duplicate code for processing
s->data.catalog_xmin and s->data.xmin. Can we create a sub-function
for this purpose and call it twice here?

thanks
Shveta

---

From: Bharath Rupireddy <[email-masked]>
Date: Fri, 15 Mar 2024 17:35:27 +0530

> +1 on maintaining both conflicting and invalidation_reason

Thanks.

> recent commit caused the conflict.

Yep, the conflict is in src/test/recovery/meson.build and is because
of e6927270cd18d535b77cbe79c55c6584351524be.

> Some trivial comments on patch001 (yet to review other patches)

Thanks for looking into this.

> Can we revert back to 'conflicting as invalid' since it is a query for

I guess, no. There the intention is to check for invalid logical slots
not just for the conflicting ones. The logical slots can get
invalidated due to other reasons as well.

> invalidation_reason IS NULL' as it is a logical slot test.

I guess no. The tests are ensuring the slot on the standby isn't invalidated.

In general, one needs to use the 'conflicting' column from
pg_replication_slots when the intention is to look for reasons for
conflicts, otherwise use the 'invalidation_reason' column for
invalidations.

Please see the attached v10 patch set after resolving the merge
conflict and fixing an indentation warning in the TAP test file.

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 15 Mar 2024 09:28:31 -0500

> others, do you have an opinion on this matter?

WFM

-- 
Nathan Bossart

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 15 Mar 2024 16:45:18 +0000

Hi,

> others, do you have an opinion on this matter?

Sounds like a good approach to me and one will be able to quickly identify
if a conflict occured.

Regards,

-- 
Bertrand Drouvot

---

From: Bharath Rupireddy <[email-masked]>
Date: Sat, 16 Mar 2024 09:29:01 +0530

> for users for monitoring purposes? We are not using it anywhere

inactive_count metric helps detect unstable replication slots
connections that have a lot of disconnections. It's not used for the
inactive_timeout based slot invalidation mechanism.

> 5 for all the slots created on that instance. Is this intentional?

Yes, it's incremented on shutdown (and for that matter upon every slot
release) for all the slots that are tied to walsenders.

> I mean we can not really use them if the instance is down.  I felt it

inactive_count is persisted to disk- upon clean shutdown, so, once the
slots become active again, one gets to see the metric and deduce some
info on disconnections.

Having said that, I'm okay to hear from others on the inactive_count
metric being added.

> for this purpose and call it twice here?

Good idea. Done that way.

> 2)
> The msg for patch 3 says:
> --------------
> a) when replication slots is lying inactive for a day or so using
> last_inactive_at metric,
> b) when a replication slot is becoming inactive too frequently using
> last_inactive_at metric.
> --------------
>  I think in b, you want to refer to inactive_count instead of last_inactive_at?

Right. Changed.

> 3)
> I do not see invalidation_reason updated for 2 new reasons in system-views.sgml

Nice catch. Added them now.

I've also responded to Bertrand's comments here.

On Wed, Mar 6, 2024 at 3:56 PM Bertrand Drouvot
<[email-masked]> wrote:
>
> A few comments:
>
> 1 ===
>
> +       The reason for the slot's invalidation. <literal>NULL</literal> if the
> +       slot is currently actively being used.
>
> s/currently actively being used/not invalidated/ ? (I mean it could be valid
> and not being used).

Changed.

> 3 ===
>
>         res = executeQueryOrDie(conn, "SELECT slot_name, plugin, two_phase, failover, "
>                                                         "FROM pg_catalog.pg_replication_slots "
> -                                                       "(CASE WHEN conflict_reason IS NOT NULL THEN FALSE "
> +                                                       "(CASE WHEN invalidation_reason IS NOT NULL THEN FALSE "
>
> Yeah that's fine because there is logical slot filtering here.

Right. And, we really are looking for invalid slots there, so use of
invalidation_reason is much more correct than conflicting.

> 4 ===
>
> -GetSlotInvalidationCause(const char *conflict_reason)
> +GetSlotInvalidationCause(const char *invalidation_reason)
>
> Should we change the comment "Maps a conflict reason" above this function?

Changed.

> 5 ===
>
> -# Check conflict_reason is NULL for physical slot
> +# Check invalidation_reason is NULL for physical slot
>  $res = $node_primary->safe_psql(
>         'postgres', qq[
> -                SELECT conflict_reason is null FROM pg_replication_slots where slot_name = '$primary_slotname';]
> +                SELECT invalidation_reason is null FROM pg_replication_slots where slot_name = '$primary_slotname';]
>  );
>
>
> I don't think this test is needed anymore: it does not make that much sense since
> it's done after the primary database initialization and startup.

It is now turned into a test verifying 'conflicting boolean' is null
for the physical slot. Isn't that okay?

> 6 ===
>
>         'Logical slots are reported as non conflicting');
>
> What about?
>
> "
> # Verify slots are reported as valid in pg_replication_slots
>     'Logical slots are reported as valid');
> "

Changed.

Please see the attached v11 patch set with all the above review
comments addressed.

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 18 Mar 2024 09:18:37 +0000

Hi,

> I've also responded to Bertrand's comments here.

Thanks!

> for the physical slot. Isn't that okay?

Yeah makes more sense now, thanks!

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 18 Mar 2024 10:12:15 +0000

Hi,

Thanks!

Looking at 0001:

1 ===

+       True if this logical slot conflicted with recovery (and so is now
+       invalidated). When this column is true, check

Worth to add back the physical slot mention "Always NULL for physical slots."?

2 ===

@@ -1023,9 +1023,10 @@ CREATE VIEW pg_replication_slots AS
             L.wal_status,
             L.safe_wal_size,
             L.two_phase,
-            L.conflict_reason,
+            L.conflicting,
             L.failover,
-            L.synced
+            L.synced,
+            L.invalidation_reason

What about making invalidation_reason close to conflict_reason?

3 ===

- * Maps a conflict reason for a replication slot to
+ * Maps a invalidation reason for a replication slot to

s/a invalidation/an invalidation/?

4 ===

While at it, shouldn't we also rename "conflict" to say "invalidation_cause" in
InvalidatePossiblyObsoleteSlot()?

5 ===

+ * rows_removed and wal_level_insufficient are only two reasons

s/are only two/are the only two/?

Regards,

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 20 Mar 2024 11:17:47 +0530

> Looking at 0001:

Thanks for reviewing.

> Worth to add back the physical slot mention "Always NULL for physical slots."?

Will change.

> What about making invalidation_reason close to conflict_reason?

Not required I think. One can pick the required columns in the SELECT
clause anyways.

> s/a invalidation/an invalidation/?

Will change.

> InvalidatePossiblyObsoleteSlot()?

That's inline with our understanding about conflict vs invalidation,
and keeps the function generic. Will change.

> s/are only two/are the only two/?

Will change..