Hi,
> === 1
>
> Thanks! Looking closer, I think there is still another window though:
>
> pgstat_unlock_entry() calls LWLockRelease(), which can process a pending
> interrupt. At that point, the shared relation stats have been updated,
> while the
> database pending stats and flushed baseline have not. A later retry could
> then
> apply the same delta again.
>
> That is also already possible in HEAD and should be very rare in practice.
> That
> said, I think it is worth mentioning and deciding whether we want to
> handle this
> case too.
>
I looked at this a bit more. We could close the window you mention
either by moving pgstat_unlock_entry() until after the pending
database stats are updated, or by holding interrupts across both the
shared relation/index update and the pending database update.
But there are still other windows where the relation/index stats and
database stats can diverge, including a possible double count in the
database entry, in particular between finishing the relation/index
flush callbacks and later flushing the database stats.
So I would rather not do anything here for now, including v8-0001,
and instead take it up in a separate thread. WDYT?
=== 2
>
> + else if (entry_ref->flushed_this_pass)
> + {
> + /*
> + * The entry is already pending and was already visited in the
> current
> + * flush pass. Move it to the tail so the data just accumulated
> into
> + * it is flushed again before the pass ends.
> + */
> + dlist_move_tail(&pgStatPending, &entry_ref->pending_node);
>
> IIUC, flushed_this_pass remains set after the flush pass ends. So, the next
> pgstat_update_dbstats() moves the database entry to the tail before the
> new pass
> starts.
>
> Should we clear this flag between passes, or at least check
> pgStatFlushInProgress
> here?
Yeah right. I will fix this and send out a new rev.
--
Sami
