Introduce XID age and inactive timeout based replication slot invalidation

On Thu, Jan 11, 2024 at 10:48 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

Replication slots in postgres will prevent removal of required
resources when there is no connection using them (inactive). This
consumes storage because neither required WAL nor required rows from
the user tables/system catalogs can be removed by VACUUM as long as
they are required by a replication slot. In extreme cases this could
cause the transaction ID wraparound.

Currently postgres has the ability to invalidate inactive replication
slots based on the amount of WAL (set via max_slot_wal_keep_size GUC)
that will be needed for the slots in case they become active. However,
the wraparound issue isn't effectively covered by
max_slot_wal_keep_size - one can't tell postgres to invalidate a
replication slot if it is blocking VACUUM. Also, it is often tricky to
choose a default value for max_slot_wal_keep_size, because the amount
of WAL that gets generated and allocated storage for the database can
vary.

Therefore, it is often easy for developers to do the following:
a) set an XID age (age of slot's xmin or catalog_xmin) of say 1 or 1.5
billion, after which the slots get invalidated.
b) set a timeout of say 1 or 2 or 3 days, after which the inactive
slots get invalidated.

To implement (a), postgres needs a new GUC called max_slot_xid_age.
The checkpointer then invalidates all the slots whose xmin (the oldest
transaction that this slot needs the database to retain) or
catalog_xmin (the oldest transaction affecting the system catalogs
that this slot needs the database to retain) has reached the age
specified by this setting.

To implement (b), first postgres needs to track the replication slot
metrics like the time at which the slot became inactive (inactive_at
timestamptz) and the total number of times the slot became inactive in
its lifetime (inactive_count numeric) in ReplicationSlotPersistentData
structure. And, then it needs a new timeout GUC called
inactive_replication_slot_timeout. Whenever a slot becomes inactive,
the current timestamp and inactive count are stored in
ReplicationSlotPersistentData structure and persisted to disk. The
checkpointer then invalidates all the slots that are lying inactive
for about inactive_replication_slot_timeout duration starting from
inactive_at.

In addition to implementing (b), these two new metrics enable
developers to improve their monitoring tools as the metrics are
exposed via pg_replication_slots system view. For instance, one can
build a monitoring tool that signals when replication slots are lying
inactive for a day or so using inactive_at metric, and/or when a
replication slot is becoming inactive too frequently using inactive_at
metric.

I’m attaching the v1 patch set as described below:
0001 - Tracks invalidation_reason in pg_replication_slots. This is
needed because slots now have multiple reasons for slot invalidation.
0002 - Tracks inactive replication slot information inactive_at and
inactive_timeout.
0003 - Adds inactive_timeout based replication slot invalidation.
0004 - Adds XID based replication slot invalidation.

Thoughts?

Needed a rebase due to c393308b. Please find the attached v2 patch set.

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

On Sat, Jan 27, 2024 at 1:18 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

On Thu, Jan 11, 2024 at 10:48 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

Replication slots in postgres will prevent removal of required
resources when there is no connection using them (inactive). This
consumes storage because neither required WAL nor required rows from
the user tables/system catalogs can be removed by VACUUM as long as
they are required by a replication slot. In extreme cases this could
cause the transaction ID wraparound.

Currently postgres has the ability to invalidate inactive replication
slots based on the amount of WAL (set via max_slot_wal_keep_size GUC)
that will be needed for the slots in case they become active. However,
the wraparound issue isn't effectively covered by
max_slot_wal_keep_size - one can't tell postgres to invalidate a
replication slot if it is blocking VACUUM. Also, it is often tricky to
choose a default value for max_slot_wal_keep_size, because the amount
of WAL that gets generated and allocated storage for the database can
vary.

Therefore, it is often easy for developers to do the following:
a) set an XID age (age of slot's xmin or catalog_xmin) of say 1 or 1.5
billion, after which the slots get invalidated.
b) set a timeout of say 1 or 2 or 3 days, after which the inactive
slots get invalidated.

To implement (a), postgres needs a new GUC called max_slot_xid_age.
The checkpointer then invalidates all the slots whose xmin (the oldest
transaction that this slot needs the database to retain) or
catalog_xmin (the oldest transaction affecting the system catalogs
that this slot needs the database to retain) has reached the age
specified by this setting.

To implement (b), first postgres needs to track the replication slot
metrics like the time at which the slot became inactive (inactive_at
timestamptz) and the total number of times the slot became inactive in
its lifetime (inactive_count numeric) in ReplicationSlotPersistentData
structure. And, then it needs a new timeout GUC called
inactive_replication_slot_timeout. Whenever a slot becomes inactive,
the current timestamp and inactive count are stored in
ReplicationSlotPersistentData structure and persisted to disk. The
checkpointer then invalidates all the slots that are lying inactive
for about inactive_replication_slot_timeout duration starting from
inactive_at.

In addition to implementing (b), these two new metrics enable
developers to improve their monitoring tools as the metrics are
exposed via pg_replication_slots system view. For instance, one can
build a monitoring tool that signals when replication slots are lying
inactive for a day or so using inactive_at metric, and/or when a
replication slot is becoming inactive too frequently using inactive_at
metric.

I’m attaching the v1 patch set as described below:
0001 - Tracks invalidation_reason in pg_replication_slots. This is
needed because slots now have multiple reasons for slot invalidation.
0002 - Tracks inactive replication slot information inactive_at and
inactive_timeout.
0003 - Adds inactive_timeout based replication slot invalidation.
0004 - Adds XID based replication slot invalidation.

Thoughts?

Needed a rebase due to c393308b. Please find the attached v2 patch set.

Needed a rebase due to commit 776621a (conflict in
src/test/recovery/meson.build for new TAP test file added). Please
find the attached v3 patch set.

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

Hi,

On Thu, Jan 11, 2024 at 10:48:13AM +0530, Bharath Rupireddy wrote:

Hi,

Therefore, it is often easy for developers to do the following:
a) set an XID age (age of slot's xmin or catalog_xmin) of say 1 or 1.5
billion, after which the slots get invalidated.
b) set a timeout of say 1 or 2 or 3 days, after which the inactive
slots get invalidated.

To implement (a), postgres needs a new GUC called max_slot_xid_age.
The checkpointer then invalidates all the slots whose xmin (the oldest
transaction that this slot needs the database to retain) or
catalog_xmin (the oldest transaction affecting the system catalogs
that this slot needs the database to retain) has reached the age
specified by this setting.

To implement (b), first postgres needs to track the replication slot
metrics like the time at which the slot became inactive (inactive_at
timestamptz) and the total number of times the slot became inactive in
its lifetime (inactive_count numeric) in ReplicationSlotPersistentData
structure. And, then it needs a new timeout GUC called
inactive_replication_slot_timeout. Whenever a slot becomes inactive,
the current timestamp and inactive count are stored in
ReplicationSlotPersistentData structure and persisted to disk. The
checkpointer then invalidates all the slots that are lying inactive
for about inactive_replication_slot_timeout duration starting from
inactive_at.

In addition to implementing (b), these two new metrics enable
developers to improve their monitoring tools as the metrics are
exposed via pg_replication_slots system view. For instance, one can
build a monitoring tool that signals when replication slots are lying
inactive for a day or so using inactive_at metric, and/or when a
replication slot is becoming inactive too frequently using inactive_at
metric.

Thanks for the patch and +1 for the idea, I think adding those new
"invalidation reasons" make sense.

I’m attaching the v1 patch set as described below:
0001 - Tracks invalidation_reason in pg_replication_slots. This is
needed because slots now have multiple reasons for slot invalidation.
0002 - Tracks inactive replication slot information inactive_at and
inactive_timeout.
0003 - Adds inactive_timeout based replication slot invalidation.
0004 - Adds XID based replication slot invalidation.

I think it's better to have the XID one being discussed/implemented before the
inactive_timeout one: what about changing the 0002, 0003 and 0004 ordering?

0004 -> 0002
0002 -> 0003
0003 -> 0004

As far 0001:

"
This commit renames conflict_reason to
invalidation_reason, and adds the support to show invalidation
reasons for both physical and logical slots.
"

I'm not sure I like the fact that "invalidations" and "conflicts" are merged
into a single field. I'd vote to keep conflict_reason as it is and add a new
invalidation_reason (and put "conflict" as value when it is the case). The reason
is that I think they are 2 different concepts (could be linked though) and that
it would be easier to check for conflicts (means conflict_reason is not NULL).

Regards,

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

On Thu, Jan 11, 2024 at 10:48 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

Hi,

Replication slots in postgres will prevent removal of required
resources when there is no connection using them (inactive). This
consumes storage because neither required WAL nor required rows from
the user tables/system catalogs can be removed by VACUUM as long as
they are required by a replication slot. In extreme cases this could
cause the transaction ID wraparound.

Currently postgres has the ability to invalidate inactive replication
slots based on the amount of WAL (set via max_slot_wal_keep_size GUC)
that will be needed for the slots in case they become active. However,
the wraparound issue isn't effectively covered by
max_slot_wal_keep_size - one can't tell postgres to invalidate a
replication slot if it is blocking VACUUM. Also, it is often tricky to
choose a default value for max_slot_wal_keep_size, because the amount
of WAL that gets generated and allocated storage for the database can
vary.

Therefore, it is often easy for developers to do the following:
a) set an XID age (age of slot's xmin or catalog_xmin) of say 1 or 1.5
billion, after which the slots get invalidated.
b) set a timeout of say 1 or 2 or 3 days, after which the inactive
slots get invalidated.

To implement (a), postgres needs a new GUC called max_slot_xid_age.
The checkpointer then invalidates all the slots whose xmin (the oldest
transaction that this slot needs the database to retain) or
catalog_xmin (the oldest transaction affecting the system catalogs
that this slot needs the database to retain) has reached the age
specified by this setting.

To implement (b), first postgres needs to track the replication slot
metrics like the time at which the slot became inactive (inactive_at
timestamptz) and the total number of times the slot became inactive in
its lifetime (inactive_count numeric) in ReplicationSlotPersistentData
structure. And, then it needs a new timeout GUC called
inactive_replication_slot_timeout. Whenever a slot becomes inactive,
the current timestamp and inactive count are stored in
ReplicationSlotPersistentData structure and persisted to disk. The
checkpointer then invalidates all the slots that are lying inactive
for about inactive_replication_slot_timeout duration starting from
inactive_at.

In addition to implementing (b), these two new metrics enable
developers to improve their monitoring tools as the metrics are
exposed via pg_replication_slots system view. For instance, one can
build a monitoring tool that signals when replication slots are lying
inactive for a day or so using inactive_at metric, and/or when a
replication slot is becoming inactive too frequently using inactive_at
metric.

I’m attaching the v1 patch set as described below:
0001 - Tracks invalidation_reason in pg_replication_slots. This is
needed because slots now have multiple reasons for slot invalidation.
0002 - Tracks inactive replication slot information inactive_at and
inactive_timeout.
0003 - Adds inactive_timeout based replication slot invalidation.
0004 - Adds XID based replication slot invalidation.

Thoughts?

+1 for the idea, here are some comments on 0002, I will review other
patches soon and respond.

1.
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>inactive_at</structfield> <type>timestamptz</type>
+      </para>
+      <para>
+        The time at which the slot became inactive.
+        <literal>NULL</literal> if the slot is currently actively being
+        used.
+      </para></entry>
+     </row>

Maybe we can change the field name to 'last_inactive_at'? or maybe the
comment can explain timestampt at which slot was last inactivated.
I think since we are already maintaining the inactive_count so better
to explicitly say this is the last invaliding time.

2.
+ /*
+ * XXX: Can inactive_count of type uint64 ever overflow? It takes
+ * about a half-billion years for inactive_count to overflow even
+ * if slot becomes inactive for every 1 millisecond. So, using
+ * pg_add_u64_overflow might be an overkill.
+ */

Correct we don't need to use pg_add_u64_overflow for this counter.

3.

+
+ /* Convert to numeric. */
+ snprintf(buf, sizeof buf, UINT64_FORMAT, slot_contents.data.inactive_count);
+ values[i++] = DirectFunctionCall3(numeric_in,
+   CStringGetDatum(buf),
+   ObjectIdGetDatum(0),
+   Int32GetDatum(-1));

What is the purpose of doing this? I mean inactive_count is 8 byte
integer and you can define function outparameter as 'int8' which is 8
byte integer. Then you don't need to convert int to string and then
to numeric?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Tue, Feb 6, 2024 at 2:16 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:

Thoughts?

+1 for the idea, here are some comments on 0002, I will review other
patches soon and respond.

Thanks for looking at it.

+ <structfield>inactive_at</structfield> <type>timestamptz</type>

Maybe we can change the field name to 'last_inactive_at'? or maybe the
comment can explain timestampt at which slot was last inactivated.
I think since we are already maintaining the inactive_count so better
to explicitly say this is the last invaliding time.

last_inactive_at looks better, so will use that in the next version of
the patch.

2.
+ /*
+ * XXX: Can inactive_count of type uint64 ever overflow? It takes
+ * about a half-billion years for inactive_count to overflow even
+ * if slot becomes inactive for every 1 millisecond. So, using
+ * pg_add_u64_overflow might be an overkill.
+ */

Correct we don't need to use pg_add_u64_overflow for this counter.

Will remove this comment in the next version of the patch.

+ /* Convert to numeric. */
+ snprintf(buf, sizeof buf, UINT64_FORMAT, slot_contents.data.inactive_count);
+ values[i++] = DirectFunctionCall3(numeric_in,
+   CStringGetDatum(buf),
+   ObjectIdGetDatum(0),
+   Int32GetDatum(-1));

What is the purpose of doing this? I mean inactive_count is 8 byte
integer and you can define function outparameter as 'int8' which is 8
byte integer. Then you don't need to convert int to string and then
to numeric?

Nope, it's of type uint64, so reporting it as numeric is a way
typically used elsewhere - see code around /* Convert to numeric. */.

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

On Mon, Feb 5, 2024 at 3:15 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

Thanks for the patch and +1 for the idea, I think adding those new
"invalidation reasons" make sense.

Thanks for looking at it.

I think it's better to have the XID one being discussed/implemented before the
inactive_timeout one: what about changing the 0002, 0003 and 0004 ordering?

0004 -> 0002
0002 -> 0003
0003 -> 0004

Done that way.

As far 0001:

"
This commit renames conflict_reason to
invalidation_reason, and adds the support to show invalidation
reasons for both physical and logical slots.
"

I'm not sure I like the fact that "invalidations" and "conflicts" are merged
into a single field. I'd vote to keep conflict_reason as it is and add a new
invalidation_reason (and put "conflict" as value when it is the case). The reason
is that I think they are 2 different concepts (could be linked though) and that
it would be easier to check for conflicts (means conflict_reason is not NULL).

So, do you want conflict_reason for only logical slots, and a separate
column for invalidation_reason for both logical and physical slots? Is
there any strong reason to have two properties "conflict" and
"invalidated" for slots? They both are the same internally, so why
confuse the users?

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

Hi,

On Wed, Feb 07, 2024 at 12:22:07AM +0530, Bharath Rupireddy wrote:

On Mon, Feb 5, 2024 at 3:15 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

I'm not sure I like the fact that "invalidations" and "conflicts" are merged
into a single field. I'd vote to keep conflict_reason as it is and add a new
invalidation_reason (and put "conflict" as value when it is the case). The reason
is that I think they are 2 different concepts (could be linked though) and that
it would be easier to check for conflicts (means conflict_reason is not NULL).

So, do you want conflict_reason for only logical slots, and a separate
column for invalidation_reason for both logical and physical slots?

Yes, with "conflict" as value in case of conflicts (and one would need to refer
to the conflict_reason reason to see the reason).

Is there any strong reason to have two properties "conflict" and
"invalidated" for slots?

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 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"?

Regards,

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

On Fri, Feb 9, 2024 at 1:12 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> 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

On Tue, Feb 20, 2024 at 12:05 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

[...] 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"?

I'm able to reproduce [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 the state [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 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
./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
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

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

Hi,

On Wed, Feb 21, 2024 at 10:55:00AM +0530, Bharath Rupireddy wrote:

On Tue, Feb 20, 2024 at 12:05 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

[...] 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"?

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?

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
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

On Wed, Feb 21, 2024 at 5:55 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

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?

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?

I'll think about it.

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?

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.

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

Hi,

On Wed, Feb 21, 2024 at 08:10:00PM +0530, Bharath Rupireddy wrote:

On Wed, Feb 21, 2024 at 5:55 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

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?

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?

Yeah, I think that looks fine too. We would need more change (like take care of
ddd5f4f54a for example).

CC'ing Amit, Hou-San and Shveta to get their point of view (as the ones behind
007693f2a3 and ddd5f4f54a).

Regarding,

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

On Thu, Feb 22, 2024 at 1:44 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

Does that make sense to you to use "conflict" as value in "invalidation_reason"
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?

Yeah, I think that looks fine too. We would need more change (like take care of
ddd5f4f54a for example).

CC'ing Amit, Hou-San and Shveta to get their point of view (as the ones behind
007693f2a3 and ddd5f4f54a).

Yeah, let's wait for what others think about it.

FWIW, I've had to rebase the patches due to 943f7ae1c. Please see the
attached v6 patch set.

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

On Wed, Feb 21, 2024 at 08:10:00PM +0530, Bharath Rupireddy wrote:

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.

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

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Sat, Mar 2, 2024 at 3:41 AM Nathan Bossart <nathandbossart@gmail.com> wrote:

[....] how about we revert
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=007693f2a3ac2ac19affcb03ad43cdb36ccff5b5,

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

No. Because both conflict and invalidation_reason are decided based on
the invalidation reason i.e. value of slot_contents.data.invalidated.
IOW, a logical slot that reports conflict as true must have been
invalidated.

Do you have any thoughts on reverting 007693f and introducing
invalidation_reason?

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

On Sun, Mar 03, 2024 at 11:40:00PM +0530, Bharath Rupireddy wrote:

On Sat, Mar 2, 2024 at 3:41 AM Nathan Bossart <nathandbossart@gmail.com> wrote:

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

No. Because both conflict and invalidation_reason are decided based on
the invalidation reason i.e. value of slot_contents.data.invalidated.
IOW, a logical slot that reports conflict as true must have been
invalidated.

Do you have any thoughts on reverting 007693f and introducing
invalidation_reason?

Unless I am misinterpreting some details, ISTM we could rename this column
to invalidation_reason and use it for both logical and physical slots. I'm
not seeing a strong need for another column. Perhaps I am missing
something...

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Sun, Mar 03, 2024 at 03:44:34PM -0600, Nathan Bossart wrote:

On Sun, Mar 03, 2024 at 11:40:00PM +0530, Bharath Rupireddy wrote:

Do you have any thoughts on reverting 007693f and introducing
invalidation_reason?

Unless I am misinterpreting some details, ISTM we could rename this column
to invalidation_reason and use it for both logical and physical slots. I'm
not seeing a strong need for another column. Perhaps I am missing
something...

And also, please don't be hasty in taking a decision that would
involve a revert of 007693f without informing the committer of this
commit about that. I am adding Amit Kapila in CC of this thread for
awareness.
--
Michael

Hi,

On Sun, Mar 03, 2024 at 03:44:34PM -0600, Nathan Bossart wrote:

On Sun, Mar 03, 2024 at 11:40:00PM +0530, Bharath Rupireddy wrote:

On Sat, Mar 2, 2024 at 3:41 AM Nathan Bossart <nathandbossart@gmail.com> wrote:

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

No. Because both conflict and invalidation_reason are decided based on
the invalidation reason i.e. value of slot_contents.data.invalidated.
IOW, a logical slot that reports conflict as true must have been
invalidated.

Do you have any thoughts on reverting 007693f and introducing
invalidation_reason?

Unless I am misinterpreting some details, ISTM we could rename this column
to invalidation_reason and use it for both logical and physical slots. I'm
not seeing a strong need for another column.

Yeah having two columns was more for convenience purpose. Without the "conflict"
one, a slot conflicting with recovery would be "a logical slot having a non NULL
invalidation_reason".

I'm also fine with one column if most of you prefer that way.

Regards,

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

On Mon, Mar 4, 2024 at 2:11 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:

On Sun, Mar 03, 2024 at 03:44:34PM -0600, Nathan Bossart wrote:

On Sun, Mar 03, 2024 at 11:40:00PM +0530, Bharath Rupireddy wrote:

On Sat, Mar 2, 2024 at 3:41 AM Nathan Bossart <nathandbossart@gmail.com> wrote:

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

No. Because both conflict and invalidation_reason are decided based on
the invalidation reason i.e. value of slot_contents.data.invalidated.
IOW, a logical slot that reports conflict as true must have been
invalidated.

Do you have any thoughts on reverting 007693f and introducing
invalidation_reason?

Unless I am misinterpreting some details, ISTM we could rename this column
to invalidation_reason and use it for both logical and physical slots. I'm
not seeing a strong need for another column.

Yeah having two columns was more for convenience purpose. Without the "conflict"
one, a slot conflicting with recovery would be "a logical slot having a non NULL
invalidation_reason".

I'm also fine with one column if most of you prefer that way.

While we debate on the above, please find the attached v7 patch set
after rebasing.

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