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

From: Bharath Rupireddy <[email-masked]>
Date: Tue, 20 Feb 2024 12:05:00 +0530

On Fri, Feb 9, 2024 at 1:12 PM Bertrand Drouvot
<[email-masked]> wrote:
>
> I think "conflict" is an important topic and does contain several reasons. The
> slot "first" conflict and then leads to slot "invalidation".
>
> > They both are the same internally, so why
> > confuse the users?
>
> I don't think that would confuse the users, I do think that would be easier to
> check for conflicting slots.

I've added a separate column for invalidation reasons for now. I'll
see how others think on this as the time goes by.

> I did not look closely at the code, just played a bit with the patch and was able
> to produce something like:
>
> postgres=# select slot_name,slot_type,active,active_pid,wal_status,invalidation_reason from pg_replication_slots;
>   slot_name  | slot_type | active | active_pid | wal_status | invalidation_reason
> -------------+-----------+--------+------------+------------+---------------------
>  rep1        | physical  | f      |            | reserved   |
>  master_slot | physical  | t      |    1482441 | unreserved | wal_removed
> (2 rows)
>
> does that make sense to have an "active/working" slot "ivalidated"?

Thanks. Can you please provide the steps to generate this error? Are
you setting max_slot_wal_keep_size on primary to generate
"wal_removed"?

Attached v5 patch set after rebasing and addressing review comments.
Please review it further.

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

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 21 Feb 2024 10:55:00 +0530

On Tue, Feb 20, 2024 at 12:05 PM Bharath Rupireddy
<[email-masked]> wrote:
>
>> [...] and was able to produce something like:
> >

I'm able to reproduce [1] the state [2] where the slot got invalidated
first, then its wal_status became unreserved, but still the slot is
serving after the standby comes up online after it catches up with the
primary getting the WAL files from the archive. There's a good reason
for this state -
https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/replication/slotfuncs.c;h=d2fa5e669a32f19989b0d987d3c7329851a1272e;hb=ff9e1e764fcce9a34467d614611a34d4d2a91b50#l351.
This intermittent state can only happen for physical slots, not for
logical slots because logical subscribers can't get the missing
changes from the WAL stored in the archive.

And, the fact looks to be that an invalidated slot can never become
normal but still can serve a standby if the standby is able to catch
up by fetching required WAL (this is the WAL the slot couldn't keep
for the standby) from elsewhere (archive via restore_command).

As far as the 0001 patch is concerned, it reports the
invalidation_reason as long as slot_contents.data.invalidated !=
RS_INVAL_NONE. I think this is okay.

Thoughts?

[1]
./initdb -D db17
echo "max_wal_size = 128MB
max_slot_wal_keep_size = 64MB
archive_mode = on
archive_command='cp %p
/home/ubuntu/postgres/pg17/bin/archived_wal/%f'" | tee -a
db17/postgresql.conf

./pg_ctl -D db17 -l logfile17 start

./psql -d postgres -p 5432 -c "SELECT
pg_create_physical_replication_slot('sb_repl_slot', true, false);"

rm -rf sbdata logfilesbdata
./pg_basebackup -D sbdata

echo "port=5433
primary_conninfo='host=localhost port=5432 dbname=postgres user=ubuntu'
primary_slot_name='sb_repl_slot'
restore_command='cp /home/ubuntu/postgres/pg17/bin/archived_wal/%f
%p'" | tee -a sbdata/postgresql.conf

touch sbdata/standby.signal

./pg_ctl -D sbdata -l logfilesbdata start
./psql -d postgres -p 5433 -c "SELECT pg_is_in_recovery();"

./pg_ctl -D sbdata -l logfilesbdata stop

./psql -d postgres -p 5432 -c "SELECT pg_logical_emit_message(true,
'mymessage', repeat('aaaa', 10000000));"
./psql -d postgres -p 5432 -c "CHECKPOINT;"
./pg_ctl -D sbdata -l logfilesbdata start
./psql -d postgres -p 5432 -xc "SELECT * FROM pg_replication_slots;"

[2]
postgres=# SELECT * FROM pg_replication_slots;
-[ RECORD 1 ]-------+-------------
slot_name           | sb_repl_slot
plugin              |
slot_type           | physical
datoid              |
database            |
temporary           | f
active              | t
active_pid          | 710667
xmin                |
catalog_xmin        |
restart_lsn         | 0/115D21A0
confirmed_flush_lsn |
wal_status          | unreserved
safe_wal_size       | 77782624
two_phase           | f
conflict_reason     |
failover            | f
synced              | f
invalidation_reason | wal_removed
last_inactive_at    |
inactive_count      | 1

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 21 Feb 2024 12:25:25 +0000

Hi,

> RS_INVAL_NONE. I think this is okay.

Yeah, looking at the code I agree that looks ok. OTOH, that looks confusing,
maybe we should add a few words about it in the doc?

Looking at v5-0001:

+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>invalidation_reason</structfield> <type>text</type>
+      </para>
+      <para>

My initial thought was to put "conflict" value in this new field in case of
conflict (not to mention the conflict reason in it). With the current proposal
invalidation_reason could report the same as conflict_reason, which sounds weird
to me.

Does that make sense to you to use "conflict" as value in "invalidation_reason"
when the slot has "conflict_reason" not NULL?

Regards,

-- 
Bertrand Drouvot

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 21 Feb 2024 20:10:00 +0530

> maybe we should add a few words about it in the doc?

I'll think about it.

> when the slot has "conflict_reason" not NULL?

I'm thinking the other way around - how about we revert
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=007693f2a3ac2ac19affcb03ad43cdb36ccff5b5,
that is, put in place "conflict" as a boolean and introduce
invalidation_reason the text form. So, for logical slots, whenever the
"conflict" column is true, the reason is found in invaldiation_reason
column? How does it sound? Again the debate might be "conflict" vs
"invalidation", but that looks clean IMHO.

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 1 Mar 2024 16:11:08 -0600

> "invalidation", but that looks clean IMHO.

Would you ever see "conflict" as false and "invalidation_reason" as
non-null for a logical slot?

-- 
Nathan Bossart