Re: Resetting spilled txn statistics in pg_stat_replication

On Tue, Sep 8, 2020 at 7:02 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Sep 8, 2020 at 7:53 AM Masahiko Sawada
> <masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:

I have fixed these review comments in the attached patch.

>
> Comments on the latest patch:
> =============================
> 1.
> +CREATE VIEW pg_stat_replication_slots AS
> + SELECT
> + s.name,
> + s.spill_txns,
> + s.spill_count,
> + s.spill_bytes,
> + s.stats_reset
> + FROM pg_stat_get_replication_slots() AS s;
>
> You forgot to update the docs for the new parameter.
>

Updated the docs for this.

> 2.
> @@ -5187,6 +5305,12 @@ pgstat_read_statsfiles(Oid onlydb, bool
> permanent, bool deep)
> for (i = 0; i < SLRU_NUM_ELEMENTS; i++)
> slruStats[i].stat_reset_timestamp = globalStats.stat_reset_timestamp;
>
> + /*
> + * Set the same reset timestamp for all replication slots too.
> + */
> + for (i = 0; i < max_replication_slots; i++)
> + replSlotStats[i].stat_reset_timestamp = globalStats.stat_reset_timestamp;
> +
>
> I don't understand why you have removed the above code from the new
> version of the patch?
>

Added back.

> 3.
> pgstat_recv_resetreplslotcounter()
> {
> ..
> + ts = GetCurrentTimestamp();
> + for (i = 0; i < nReplSlotStats; i++)
> + {
> + /* reset entry with the given index, or all entries */
> + if (msg->clearall || idx == i)
> + {
> + /* reset only counters. Don't clear slot name */
> + replSlotStats[i].spill_txns = 0;
> + replSlotStats[i].spill_count = 0;
> + replSlotStats[i].spill_bytes = 0;
> + replSlotStats[i].stat_reset_timestamp = ts;
> + }
> + }
> ..
>
> I don't like this coding pattern as in the worst case we need to
> traverse all the slots to reset a particular slot. This could be okay
> for a fixed number of elements as we have in SLRU but here it appears
> quite inefficient. We can move the reset of stats part to a separate
> function and then invoke it from the place where we need to reset a
> particular slot and the above place.
>

Changed the code as per the above idea.

> 4.
> +pgstat_replslot_index(const char *name, bool create_it)
> {
> ..
> + replSlotStats[nReplSlotStats].stat_reset_timestamp = GetCurrentTimestamp();
> ..
> }
>
> Why do we need to set the reset timestamp on the creation of slot entry?
>

I don't think we need to show any time if the slot is never reset. Let
me know if there is any reason to show it.

> 5.
> @@ -3170,6 +3175,13 @@ ReorderBufferSerializeTXN(ReorderBuffer *rb,
> ReorderBufferTXN *txn)
> spilled++;
> }
>
> + /* update the statistics */
> + rb->spillCount += 1;
> + rb->spillBytes += size;
> +
> + /* Don't consider already serialized transactions. */
> + rb->spillTxns += rbtxn_is_serialized(txn) ? 0 : 1;
>
> We can't increment the spillTxns in the above way because now
> sometimes we do serialize before streaming and in that case we clear
> the serialized flag after streaming, see ReorderBufferTruncateTXN. So,
> the count can go wrong.
>

To fix, this I have added another flag which indicates if we have ever
serialized the txn. I couldn't find a better way, do let me know if
you can think of a better way to address this comment.

> Another problem is currently the patch call
> UpdateSpillStats only from begin_cb_wrapper which means it won't
> consider streaming transactions (streaming transactions that might
> have spilled).
>

The other problem I see with updating in begin_cb_wrapper is that it
will ignore the spilling done for transactions that get aborted. To
fix both the issues, I have updated the stats in DecodeCommit and
DecodeAbort.

> 6.
> @@ -322,6 +321,9 @@ ReplicationSlotCreate(const char *name, bool db_specific,
>
> /* Let everybody know we've modified this slot */
> ConditionVariableBroadcast(&slot->active_cv);
> +
> + /* Create statistics entry for the new slot */
> + pgstat_report_replslot(NameStr(slot->data.name), 0, 0, 0);
> }
> ..
> ..
> @@ -683,6 +685,18 @@ ReplicationSlotDropPtr(ReplicationSlot *slot)
> ereport(WARNING,
> (errmsg("could not remove directory \"%s\"", tmppath)));
>
> + /*
> + * Report to drop the replication slot to stats collector. Since there
> + * is no guarantee the order of message arrival on an UDP connection,
> + * it's possible that a message for creating a new slot arrives before a
> + * message for removing the old slot. We send the drop message while
> + * holding ReplicationSlotAllocationLock to reduce that possibility.
> + * If the messages arrived in reverse, we would lose one statistics update
> + * message. But the next update message will create the statistics for
> + * the replication slot.
> + */
> + pgstat_report_replslot_drop(NameStr(slot->data.name));
> +
>
> Similar to drop message, why don't we send the create message while
> holding the ReplicationSlotAllocationLock?
>

Updated code to address this comment, basically moved the create
message under lock.

Apart from the above,
(a) fixed one bug in ReorderBufferSerializeTXN() where we were
updating the stats even when we have not spilled anything.
(b) made changes in pgstat_read_db_statsfile_timestamp to return false
when the replication slot entry is corrupt.
(c) move the declaration and definitions in pgstat.c to make them
consistent with existing code
(d) made another couple of cosmetic fixes and changed a few comments
(e) Tested the patch by using a guc which allows spilling all the
changes. See v4-0001-guc-always-spill

Let me know what do you think about the changes?

--
With Regards,
Amit Kapila.