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

Hi Nisha.

Here are some review comments for patch v57-0001.

======
src/backend/replication/slot.c

1.
+
+/*
+ * Raise an error based on the invalidation cause of the slot.
+ */
+static void
+RaiseSlotInvalidationError(ReplicationSlot *slot)
+{
+ StringInfoData err_detail;
+
+ initStringInfo(&err_detail);
+
+ switch (slot->data.invalidated)

1a.
/invalidation cause of the slot./slot's invalidation cause./

~

1b.
This function does not expect to be called with slot->data.invalidated
== RS_INVAL_NONE, so I think it will be better to assert that
up-front.

~

1c.
This code could be simplified if you declare/initialize the variable
together, like:

StringInfo err_detail = makeStringInfo();

~~~

2.
+ case RS_INVAL_WAL_REMOVED:
+ appendStringInfo(&err_detail, _("This slot has been invalidated
because the required WAL has been removed."));
+ break;
+
+ case RS_INVAL_HORIZON:
+ appendStringInfo(&err_detail, _("This slot has been invalidated
because the required rows have been removed."));
+ break;

Since there are no format strings here, appendStringInfoString can be
used directly in some places.

======

FYI. I've attached a diffs patch that implements some of the
above-suggested changes.

======
Kind Regards,
Peter Smith.
Fujitsu Australia