public inbox for [email protected]  

help / color / mirror / Atom feed

From: Tom Lane <[email protected]>
To: [email protected]
Cc: Ivan Kush <[email protected]>
Cc: [email protected]
Cc: [email protected]
Subject: Re: pg_stat_statements: improve loading and saving routines for the dump file
Date: Thu, 03 Apr 2025 18:32:12 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

[email protected] writes:
>> What does this patch give on aglobal scale? Does it save much memory or 
>> increase performance? How many times?

> This patch makes the code semantically more correct and we don't lose 
> anything. It is obviously not about performance or memory optimisation.

Indeed not: on my machine I see sizeof(pgssEntry) = 432.  It's full of
int64 fields, so the alignment requirement is 8 bytes, meaning that
the mutex field accounts for 8 bytes even though it's likely just 1
or 4 bytes wide.  Still, that means what you suggest is only going
to save 8/432 = 1.8% of the on-disk size of the struct.  Given that
we also store the SQL query text for each entry, the net savings
fraction is even smaller.

I don't really agree that this is adding any semantic correctness
either.  If we were reading directly into a live hashtable entry,
overwriting the mutex field could indeed be bad.  But the fread()
call is reading into a local variable "temp" and we only copy
selected fields out of that.  So it's just some bytes in a
transient stack entry.

On the whole therefore, I'm inclined to reject this on several
grounds:

* The savings is not worth the costs of bumping the stats file format
magic number (and thereby invalidating everyone's stored statistics).

* I'd rather keep the flexibility to put the mutex wherever we want in
the struct.  Right now that isn't worth much either, but depending on
what fields get added in future, we might have the opportunity to move
the mutex to someplace where it fits into padding space and hence adds
nothing to sizeof(pgssEntry).

* Adding the requirement on where the mutex is makes the code more
fragile, since somebody might ignore the comment and place things
incorrectly.  I'd like to think that such an error would be spotted
immediately, but we've committed sillier mistakes.  The consequences
would likely be that some fields don't get stored/reloaded correctly,
which might escape notice for awhile.

			regards, tom lane

view thread (7+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: pg_stat_statements: improve loading and saving routines for the dump file
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox