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

[Peter Smith]

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

diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 2a99c1f..71c6ae2 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -2816,23 +2816,23 @@ WaitForStandbyConfirmation(XLogRecPtr wait_for_lsn)
 static void
 RaiseSlotInvalidationError(ReplicationSlot *slot)
 {
-       StringInfoData err_detail;
+       StringInfo err_detail = makeStringInfo();
 
-       initStringInfo(&err_detail);
+       Assert(slot->data.invalidated != RS_INVAL_NONE);
 
        switch (slot->data.invalidated)
        {
                case RS_INVAL_WAL_REMOVED:
-                       appendStringInfo(&err_detail, _("This slot has been 
invalidated because the required WAL has been removed."));
+                       appendStringInfoString(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."));
+                       appendStringInfoString(err_detail, _("This slot has 
been invalidated because the required rows have been removed."));
                        break;
 
                case RS_INVAL_WAL_LEVEL:
                        /* translator: %s is a GUC variable name */
-                       appendStringInfo(&err_detail, _("This slot has been 
invalidated because \"%s\" is insufficient for slot."),
+                       appendStringInfo(err_detail, _("This slot has been 
invalidated because \"%s\" is insufficient for slot."),
                                                         "wal_level");
                        break;
 
@@ -2844,5 +2844,5 @@ RaiseSlotInvalidationError(ReplicationSlot *slot)
                        errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                        errmsg("can no longer get changes from replication slot 
\"%s\"",
                                   NameStr(slot->data.name)),
-                       errdetail_internal("%s", err_detail.data));
+                       errdetail_internal("%s", err_detail->data));
 }