Re: pgstat: Flush some statistics within running transactions, take 2

>
> > 1/ to also pgStatFlushInProgress when we're-queueing
> >
> > pgstat_prep_pending_from_entry_ref(PgStat_EntryRef *entry_ref)
> > {
> > ...
> > ....
> > else if (pgStatFlushInProgress && entry_ref->flushed_this_pass)
> > ...
> > ...
> > }
>
> This fixes the case where the stale flag moves an entry before the new
> flush pass
> starts.
>
> That said, IIUC, the flag remains true from the previous pass until the
> entry
> is visited in the new one. So, if an earlier callback updates an entry not
> yet
> visited, pgstat_prep_pending_from_entry_ref() treats it as already visited
> in
> the current pass.
>
> I wonder if the flags should be reset before starting the scan?

Done, added a scan to reset the flag at the start
of pgstat_flush_pending_entries().

> === 2
>
> +SELECT seq_scan - :seq_scan_before AS seq_scan_delta,
> + seq_tup_read - :seq_tup_read_before AS seq_tup_read_delta,
> + n_tup_ins - :n_tup_ins_before AS n_tup_ins_delta,
> + n_tup_upd - :n_tup_upd_before AS n_tup_upd_delta,
> + n_tup_hot_upd + n_tup_newpage_upd <= n_tup_upd AS upd_counts_ok
> + FROM pg_stat_user_tables WHERE relname = 'partial_flush';
>
> That would also succeed if both counters were zero. Since the test
> performs exactly
> one update on a table without indexes, their sum should increase by one.
> Could
> we record its value before the transaction and check that delta instead?

Done

I also did not think this in v9- was good enough.

+ /*
+ * Clear the per-pass flag before the callback so that a callback
+ * accumulating into its own entry does not re-queue it. Set again
+ * below once the entry has been visited.
+ */
+ entry_ref->flushed_this_pass = false;

Yes, it prevents a re-queue hazard,
as the same entry we are flushing should not be re-queued, but it also did
not
prevent a callback from preparing its own entry, which will never make any
sense
to do so. One can just update the entry without re-preparing it. I think
it's better
to harden this as not allowed. So I introduced to make it clear this is not
a supported operation.

+ /* The entry currently being flushed must not be made pending again. */
+ Assert(entry_ref != pgStatCurrentFlushEntry);

--
Sami Imseih
Amazon Web Services (AWS)