Hi,
On Fri, Aug 21, 2026 at 10:57:13AM -0500, Sami Imseih wrote:
> Hi,
>
> I posted v7
Thanks!
I focused on 0002 for now, as the second comment below could also impact 0001.
=== 1
> Done in v7. pgstat_report_stat() now uses PG_TRY/PG_FINALLY to clear
> pgStatFlushInProgress on all exit paths, and the reset at entry became
> an assert.
That makes sense but I think we still have a database/relation or index ordering
issue.
If pgstat_prep_database_pending() errors, the shared index (or relation) stats
have already been updated, while the database stats and flushed baseline have
not.
I wonder if pgstat_prep_database_pending() should be called before the first
shared stat update, or if the callback should ensure that an error cannot leave
the flush partially applied?
=== 2
> Done in v7. The relation flush path now compares only the
> non-transactional group before taking the lock for an in-transaction
> flush. If there are no changes in the non-transactional stats, it
> returns PGSTAT_FLUSH_PARTIAL immediately.
+ * counts and flushed are zeroed on allocation and no field write ever
+ * touches the padding, so these byte compares are safe
I'm not sure this is guaranteed by C11. Section 6.2.6.1 paragraph 6 (see [1])
says:
"
When a value is stored in an object of structure or union type, including in a
member object, the bytes of the object representation that correspond to any
padding bytes take unspecified values.
"
Moreover, the patch does a structure assignment:
+ if (!xact_boundary)
+ {
+ lstats->tab.flushed = lstats->tab.counts;
And the corresponding footnote explicitly says:
"
Thus, for example, structure assignment need not copy any padding bits
"
So, as PgStat_TableCountsTxn contains padding:
(gdb) ptype /o struct PgStat_TableCountsTxn
/* offset | size */ type = struct PgStat_TableCountsTxn {
/* 0 | 8 */ PgStat_Counter tuples_inserted;
/* 8 | 8 */ PgStat_Counter tuples_updated;
/* 16 | 8 */ PgStat_Counter tuples_deleted;
/* 24 | 8 */ PgStat_Counter tuples_hot_updated;
/* 32 | 8 */ PgStat_Counter tuples_newpage_updated;
/* 40 | 1 */ _Bool truncdropped;
/* XXX 7-byte hole */
/* 48 | 8 */ PgStat_Counter delta_live_tuples;
/* 56 | 8 */ PgStat_Counter delta_dead_tuples;
/* 64 | 8 */ PgStat_Counter changed_tuples;
/* total size (bytes): 72 */
}
then, I wonder if using memcmp() here is safe:
+ if (memcmp(&lstats->tab.counts.txn, &lstats->tab.flushed.txn,
+ sizeof(PgStat_TableCountsTxn)) == 0)
+ return PGSTAT_FLUSH_DONE;
as it could report a difference because of the padding even when all their
members are equal. That said that would cause extra work, not missed statistics.
[1]: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
