Horton

Thread

Commits

  1. Doc: document cases where queryid is stable

  1. Stability of queryid in minor versions

    David Rowley <dgrowleyml@gmail.com> — 2024-04-14T23:20:16Z

    I was recently asked internally about the stability guarantees we
offer for queryid.  My answer consisted of:

1. We cannot change Node enums in minor versions
2. We're *unlikely* to add fields to Node types in minor versions, and
if we did we'd likely be leaving them out of the jumble calc, plus it
seems highly unlikely any new field we wedged into the padding would
relate at all to the parsed query.

While answering, I checked what our documentation says. It does not
seem to offer much in the way of what is guaranteed between minor
versions.

In [1] I see:

"As a rule of thumb, queryid values can be assumed to be stable and
comparable only so long as the underlying server version"

It's the "underlying server version" that I think needs some
clarification. It's unclear if the minor version must match or just
the major version number. The preceding paragraph does mention:

"Furthermore, it is not safe to assume that queryid will be stable
across major versions of PostgreSQL."

but not stable across *major* versions does *not* mean stable across
*minor* versions. The reader is just left guessing if that's true.

Maybe the paragraph starting with "Consumers of" can detail the
reasons queryid might be unstable and the following paragraph can
describe the scenario for when the queryid can generally assumed to be
stable.

I've drafted a patch which I think improves things, but it probably
needs more work and opinions.

David

[1] https://www.postgresql.org/docs/current/pgstatstatements.html

  2. Re: Stability of queryid in minor versions

    Peter Geoghegan <pg@bowt.ie> — 2024-04-14T23:47:11Z

    On Sun, Apr 14, 2024 at 7:20 PM David Rowley <dgrowleyml@gmail.com> wrote:
> It's the "underlying server version" that I think needs some
> clarification. It's unclear if the minor version must match or just
> the major version number. The preceding paragraph does mention:
>
> "Furthermore, it is not safe to assume that queryid will be stable
> across major versions of PostgreSQL."
>
> but not stable across *major* versions does *not* mean stable across
> *minor* versions. The reader is just left guessing if that's true.

Technically we don't promise that WAL records won't change in minor
versions. In fact, the docs specifically state that the format of any
WAL record might change, and that users should upgrade standbys first
on general principle (though I imagine few do). We try hard to avoid
changing the format of WAL records in point releases, of course, but
strictly speaking there is no guarantee. This situation seems similar
(though much lower stakes) to me. Query normalization isn't perfect --
there's a trade-off.

> Maybe the paragraph starting with "Consumers of" can detail the
> reasons queryid might be unstable and the following paragraph can
> describe the scenario for when the queryid can generally assumed to be
> stable.

I think that it would be reasonable to say that we strive to not break
the format in point releases. Fundamentally, if pg_stat_statements
sees a hard queryid format change (e.g. due to a major version
upgrade), then pg_stat_statements throws away the accumulated query
stats without being asked to.

-- 
Peter Geoghegan

  3. Re: Stability of queryid in minor versions

    Michael Paquier <michael@paquier.xyz> — 2024-04-15T00:04:07Z

    On Mon, Apr 15, 2024 at 11:20:16AM +1200, David Rowley wrote:
> I was recently asked internally about the stability guarantees we
> offer for queryid.  My answer consisted of:
> 
> 1. We cannot change Node enums in minor versions
> 2. We're *unlikely* to add fields to Node types in minor versions, and
> if we did we'd likely be leaving them out of the jumble calc, plus it
> seems highly unlikely any new field we wedged into the padding would
> relate at all to the parsed query.

Since 16 these new fields would be added by default unless the node
attribute query_jumble_ignore is appended to it.  I agree that this
may not be entirely intuitive when it comes to force compatibility
across the same major version.  Could there be cases where it is worth 
breaking compatibility and include something more in the jumbling,
though?  I've not seen the case in recent years even in stable
branches.

> Maybe the paragraph starting with "Consumers of" can detail the
> reasons queryid might be unstable and the following paragraph can
> describe the scenario for when the queryid can generally assumed to be
> stable.
>
>    <para>
>     As a rule of thumb, <structfield>queryid</structfield> values can be assumed to be
> -   stable and comparable only so long as the underlying server version and
> -   catalog metadata details stay exactly the same.  Two servers
> +   stable and comparable only between <productname>PostgreSQL</productname> instances
> +   which are running the same major version of <productname>PostgreSQL</productname>
> +   and are running on the same machine architecture and catalog metadata details match.  Two servers
>     participating in replication based on physical WAL replay can be expected
>     to have identical <structfield>queryid</structfield> values for the same query.
>     However, logical replication schemes do not promise to keep replicas

Assuming that a query ID will be always stable across major versions
is overconfident, I think.  As Peter said, like for WAL, we may face
cases where a slight breakage for a subset of queries could be
justified, and pg_stat_statement would be able to cope with that by
discarding the oldest entries in its hash tables.
--
Michael

  4. Re: Stability of queryid in minor versions

    Peter Geoghegan <pg@bowt.ie> — 2024-04-15T00:22:03Z

    On Sun, Apr 14, 2024 at 8:04 PM Michael Paquier <michael@paquier.xyz> wrote:
> Assuming that a query ID will be always stable across major versions
> is overconfident, I think.  As Peter said, like for WAL, we may face
> cases where a slight breakage for a subset of queries could be
> justified, and pg_stat_statement would be able to cope with that by
> discarding the oldest entries in its hash tables.

If there was a minor break in compatibility, that either went
unnoticed, or was considered too minor to matter, then
pg_stat_statements would be in exactly the same position as any
external tool that uses its queryid values to accumulate query costs.
While external tools can't understand the provenance of old queryid
values, pg_stat_statements can't either.


-- 
Peter Geoghegan

  5. Re: Stability of queryid in minor versions

    David Rowley <dgrowleyml@gmail.com> — 2024-04-15T01:00:56Z

    On Mon, 15 Apr 2024 at 11:47, Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Sun, Apr 14, 2024 at 7:20 PM David Rowley <dgrowleyml@gmail.com> wrote:
> > It's the "underlying server version" that I think needs some
> > clarification. It's unclear if the minor version must match or just
> > the major version number. The preceding paragraph does mention:
> >
> > "Furthermore, it is not safe to assume that queryid will be stable
> > across major versions of PostgreSQL."
> >
> > but not stable across *major* versions does *not* mean stable across
> > *minor* versions. The reader is just left guessing if that's true.
>
> Technically we don't promise that WAL records won't change in minor
> versions. In fact, the docs specifically state that the format of any
> WAL record might change, and that users should upgrade standbys first
> on general principle (though I imagine few do). We try hard to avoid
> changing the format of WAL records in point releases, of course, but
> strictly speaking there is no guarantee. This situation seems similar
> (though much lower stakes) to me. Query normalization isn't perfect --
> there's a trade-off.

set compute_query_id = 'on';
explain (costs off, verbose) select oid from pg_class;
                           QUERY PLAN
-----------------------------------------------------------------
 Index Only Scan using pg_class_oid_index on pg_catalog.pg_class
   Output: oid
 Query Identifier: -8748805461085747951
(3 rows)

As far as I understand query ID; it's based on the parse nodes and
values in the system catalogue tables and is calculated on the local
server. Computed values are susceptible to variations in hash values
calculated by different CPU architectures.

Where does WAL fit into this? And why would a WAL format change the
computed value?

David

  6. Re: Stability of queryid in minor versions

    Peter Geoghegan <pg@bowt.ie> — 2024-04-15T01:11:44Z

    On Sun, Apr 14, 2024 at 9:01 PM David Rowley <dgrowleyml@gmail.com> wrote:
> On Mon, 15 Apr 2024 at 11:47, Peter Geoghegan <pg@bowt.ie> wrote:
> > Technically we don't promise that WAL records won't change in minor
> > versions. In fact, the docs specifically state that the format of any
> > WAL record might change, and that users should upgrade standbys first
> > on general principle (though I imagine few do). We try hard to avoid
> > changing the format of WAL records in point releases, of course, but
> > strictly speaking there is no guarantee. This situation seems similar
> > (though much lower stakes) to me. Query normalization isn't perfect --
> > there's a trade-off.

> Where does WAL fit into this? And why would a WAL format change the
> computed value?

It doesn't. I just compared the two situations, which seem analogous.

-- 
Peter Geoghegan

  7. Re: Stability of queryid in minor versions

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-15T01:19:34Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Apr 15, 2024 at 11:20:16AM +1200, David Rowley wrote:
>> 1. We cannot change Node enums in minor versions
>> 2. We're *unlikely* to add fields to Node types in minor versions, and
>> if we did we'd likely be leaving them out of the jumble calc, plus it
>> seems highly unlikely any new field we wedged into the padding would
>> relate at all to the parsed query.

> Since 16 these new fields would be added by default unless the node
> attribute query_jumble_ignore is appended to it.

They'd also be written/read by outfuncs/readfuncs, thereby breaking
stored views/rules if the Node is one that can appear in a parsetree.
So the bar to making such a change in a stable branch would be very
high.

			regards, tom lane

  8. Re: Stability of queryid in minor versions

    David Rowley <dgrowleyml@gmail.com> — 2024-04-15T01:27:09Z

    On Mon, 15 Apr 2024 at 12:04, Michael Paquier <michael@paquier.xyz> wrote:
> Since 16 these new fields would be added by default unless the node
> attribute query_jumble_ignore is appended to it.  I agree that this
> may not be entirely intuitive when it comes to force compatibility
> across the same major version.  Could there be cases where it is worth
> breaking compatibility and include something more in the jumbling,
> though?  I've not seen the case in recent years even in stable
> branches.

I think that's a valid possible situation which could result in a
change. however, I think it's much less likely than it used to be
because we'd have to accidentally have used query_jumble_ignore,
whereas before all the struct parsing stuff went in, we could have
just forgotten to jumble the field when adding a new field to a parse
struct.

David

  9. Re: Stability of queryid in minor versions

    David Rowley <dgrowleyml@gmail.com> — 2024-04-15T01:31:47Z

    On Mon, 15 Apr 2024 at 13:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Michael Paquier <michael@paquier.xyz> writes:
> > On Mon, Apr 15, 2024 at 11:20:16AM +1200, David Rowley wrote:
> >> 1. We cannot change Node enums in minor versions
> >> 2. We're *unlikely* to add fields to Node types in minor versions, and
> >> if we did we'd likely be leaving them out of the jumble calc, plus it
> >> seems highly unlikely any new field we wedged into the padding would
> >> relate at all to the parsed query.
>
> > Since 16 these new fields would be added by default unless the node
> > attribute query_jumble_ignore is appended to it.
>
> They'd also be written/read by outfuncs/readfuncs, thereby breaking
> stored views/rules if the Node is one that can appear in a parsetree.
> So the bar to making such a change in a stable branch would be very
> high.

I think a soft guarantee in the docs for it being stable in minor
versions would be ok then.

I'm unsure if "Rule of thumb" is the correct way to convey that. We
can't really write "We endeavour to", as who is "We".  Maybe something
like "Generally, it can be assumed that queryid is stable between all
minor versions of a major version of ..., providing that <other
reasons>".

David

  10. Re: Stability of queryid in minor versions

    David G. Johnston <david.g.johnston@gmail.com> — 2024-04-15T01:36:19Z

    On Sun, Apr 14, 2024 at 4:20 PM David Rowley <dgrowleyml@gmail.com> wrote:

>
> I've drafted a patch which I think improves things, but it probably
> needs more work and opinions.
>
>
Seems we can improve things by simply removing the "rule of thumb" sentence
altogether.  The prior paragraph states the things the queryid depends upon
at the level of detail the reader needs.

The sentence "Two servers participating in replication based on physical
WAL replay can be expected to have identical queryid values for the same
query." apparently assumes that to participate both servers must share the
same machine architecture.  I am under the impression that this is only an
advisory, not a requirement.  Rather, two servers participating in physical
replication will be ensured that the catalog metadata and major versions
are identical.  This is not the case for servers related via logical
replication.

David J.

  11. Re: Stability of queryid in minor versions

    David G. Johnston <david.g.johnston@gmail.com> — 2024-04-15T01:58:35Z

    On Sun, Apr 14, 2024 at 6:32 PM David Rowley <dgrowleyml@gmail.com> wrote:

> On Mon, 15 Apr 2024 at 13:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Michael Paquier <michael@paquier.xyz> writes:
> > > On Mon, Apr 15, 2024 at 11:20:16AM +1200, David Rowley wrote:
> > >> 1. We cannot change Node enums in minor versions
> > >> 2. We're *unlikely* to add fields to Node types in minor versions, and
> > >> if we did we'd likely be leaving them out of the jumble calc, plus it
> > >> seems highly unlikely any new field we wedged into the padding would
> > >> relate at all to the parsed query.
> >
> > > Since 16 these new fields would be added by default unless the node
> > > attribute query_jumble_ignore is appended to it.
> >
> > They'd also be written/read by outfuncs/readfuncs, thereby breaking
> > stored views/rules if the Node is one that can appear in a parsetree.
> > So the bar to making such a change in a stable branch would be very
> > high.
>
> I think a soft guarantee in the docs for it being stable in minor
> versions would be ok then.
>
> I'm unsure if "Rule of thumb" is the correct way to convey that. We
> can't really write "We endeavour to", as who is "We".  Maybe something
> like "Generally, it can be assumed that queryid is stable between all
> minor versions of a major version of ..., providing that <other
> reasons>".
>
>
So, there are three kinds of dependencies:

Product
Machine
User Data

The user data dependencies are noted as being OID among other things
The machine dependencies are the architecture and other facets
The product dependencies are not enumerated but can be simply stated to be
internals stable throughout a major version.

A minimal rewording of the last sentence in the prior paragraph could be:

Lastly, the queryid depends upon aspects of PostgreSQL internals that can
only change with each major version release.

I'm disinclined to note minor releases here given the above wording.  Sure,
like with lots of things, circumstances may require us to break a policy,
but we don't seem to make that point everywhere we conceive it could happen.

David J.

  12. Re: Stability of queryid in minor versions

    David Rowley <dgrowleyml@gmail.com> — 2024-04-15T02:03:02Z

    On Mon, 15 Apr 2024 at 13:37, David G. Johnston
<david.g.johnston@gmail.com> wrote:
> Seems we can improve things by simply removing the "rule of thumb" sentence altogether.  The prior paragraph states the things the queryid depends upon at the level of detail the reader needs.

I don't think that addresses the following, which I mentioned earlier:

> but not stable across *major* versions does *not* mean stable across
> *minor* versions. The reader is just left guessing if that's true.

David

  13. Re: Stability of queryid in minor versions

    Michael Paquier <michael@paquier.xyz> — 2024-04-15T02:09:13Z

    On Mon, Apr 15, 2024 at 01:31:47PM +1200, David Rowley wrote:
> I think a soft guarantee in the docs for it being stable in minor
> versions would be ok then.
> 
> I'm unsure if "Rule of thumb" is the correct way to convey that. We
> can't really write "We endeavour to", as who is "We".  Maybe something
> like "Generally, it can be assumed that queryid is stable between all
> minor versions of a major version of ..., providing that <other
> reasons>".

It sounds to me that the term "best-effort" is adapted here?  Like in
"The compatibility of query IDs is preserved across minor versions on
a best-effort basis.  It is possible that the post-parse-analysis tree
changes across minor releases, impacting the value of queryid for the
same query run across two different minor versions.".
--
Michael

  14. Re: Stability of queryid in minor versions

    David G. Johnston <david.g.johnston@gmail.com> — 2024-04-15T02:23:33Z

    On Sun, Apr 14, 2024 at 7:03 PM David Rowley <dgrowleyml@gmail.com> wrote:

> On Mon, 15 Apr 2024 at 13:37, David G. Johnston
> <david.g.johnston@gmail.com> wrote:
> > Seems we can improve things by simply removing the "rule of thumb"
> sentence altogether.  The prior paragraph states the things the queryid
> depends upon at the level of detail the reader needs.
>
> I don't think that addresses the following, which I mentioned earlier:
>
> > but not stable across *major* versions does *not* mean stable across
> > *minor* versions. The reader is just left guessing if that's true.
>
>
The base assumption here is that changes in the things we don't mention do
not influence the queryid.  We didn't mention minor versions, changing them
doesn't influence the queryid.

Now, reading that entire paragraph is a bit of a challenge IMO, and agree,
as I subsequently noted, that the sentence you pointed out could be
reworked.  I stand by my statement that removing the sentence about "rule
of thumb" altogether is a win.  The prior paragraph should be sufficient -
it is technically at the moment but am not opposed to rewording.

David J.

  15. Re: Stability of queryid in minor versions

    David Rowley <dgrowleyml@gmail.com> — 2024-04-15T02:54:52Z

    On Mon, 15 Apr 2024 at 14:09, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Apr 15, 2024 at 01:31:47PM +1200, David Rowley wrote:
> > I'm unsure if "Rule of thumb" is the correct way to convey that. We
> > can't really write "We endeavour to", as who is "We".  Maybe something
> > like "Generally, it can be assumed that queryid is stable between all
> > minor versions of a major version of ..., providing that <other
> > reasons>".
>
> It sounds to me that the term "best-effort" is adapted here?  Like in
> "The compatibility of query IDs is preserved across minor versions on
> a best-effort basis.  It is possible that the post-parse-analysis tree
> changes across minor releases, impacting the value of queryid for the
> same query run across two different minor versions.".

I had another try and ended up pushing the logical / physical replica
details up to the paragraph above. It seems more relevant to mention
this in the section which details reasons why the queryid can be
unstable due to metadata variations.  I think keeping the 2nd
paragraph for reasons it's stable is a good separation of
responsibility.  I didn't include the "best-effort" word, but here's
what I did come up with.

David

  16. Re: Stability of queryid in minor versions

    Michael Paquier <michael@paquier.xyz> — 2024-04-16T00:10:19Z

    On Mon, Apr 15, 2024 at 02:54:52PM +1200, David Rowley wrote:
>     <filename>pg_stat_statements</filename> will consider two apparently-identical
>     queries to be distinct, if they reference a table that was dropped
>     and recreated between the executions of the two queries.
> +   Two servers participating in replication based on physical WAL replay can
> +   be expected to have identical <structfield>queryid</structfield> values for
> +   the same query.  However, logical replication schemes do not promise to
> +   keep replicas identical in all relevant details, so
> +   <structfield>queryid</structfield> will not be a useful identifier for
> +   accumulating costs across a set of logical replicas.
>     The hashing process is also sensitive to differences in
>     machine architecture and other facets of the platform.
>     Furthermore, it is not safe to assume that <structfield>queryid</structfield>
>     will be stable across major versions of <productname>PostgreSQL</productname>.
> +   If in doubt, direct testing is recommended.
>    </para>

Not sure that this is an improvement in clarity.  There are a few
bullet points that treat about the instability of the query ID, and
your patch is now mixing the query ID being different for two
mostly-identical queries on the same host with larger conditions like
the environment involved.  Perhaps it would be better to move the last
sentence of the first <para> ("Furthermore, it is not safe..") with
the part you are adding about replication in this paragraph.

>    <para>
> -   As a rule of thumb, <structfield>queryid</structfield> values can be assumed to be
> -   stable and comparable only so long as the underlying server version and
> -   catalog metadata details stay exactly the same.  Two servers
> -   participating in replication based on physical WAL replay can be expected
> -   to have identical <structfield>queryid</structfield> values for the same query.
> -   However, logical replication schemes do not promise to keep replicas
> -   identical in all relevant details, so <structfield>queryid</structfield> will
> -   not be a useful identifier for accumulating costs across a set of logical
> -   replicas.  If in doubt, direct testing is recommended.
> +   Generally, it can be assumed that <structfield>queryid</structfield> values
> +   are stable between minor version releases of <productname>PostgreSQL</productname>,
> +   providing that instances are running on the same machine architecture and
> +   the catalog metadata details match.  Compatibility will only be broken
> +   between minor versions as a last resort.

This split is cleaner.
--
Michael

  17. Re: Stability of queryid in minor versions

    David Rowley <dgrowleyml@gmail.com> — 2024-04-16T02:04:22Z

    On Tue, 16 Apr 2024 at 12:10, Michael Paquier <michael@paquier.xyz> wrote:
> Not sure that this is an improvement in clarity.  There are a few
> bullet points that treat about the instability of the query ID, and
> your patch is now mixing the query ID being different for two
> mostly-identical queries on the same host with larger conditions like
> the environment involved.  Perhaps it would be better to move the last
> sentence of the first <para> ("Furthermore, it is not safe..") with
> the part you are adding about replication in this paragraph.

Yeah, I think this is better.  I think the attached is what you mean.

It makes sense to talk about the hashing variations closer to the
object identifier part.

David

  18. Re: Stability of queryid in minor versions

    Michael Paquier <michael@paquier.xyz> — 2024-04-16T03:16:42Z

    On Tue, Apr 16, 2024 at 02:04:22PM +1200, David Rowley wrote:
> It makes sense to talk about the hashing variations closer to the
> object identifier part.

Mostly what I had in mind.  A separate <para> for the new part you are
adding at the end of the first part feels a bit more natural split
here.  Feel free to discard my comment if you think that's not worth
it.
--
Michael

  19. Re: Stability of queryid in minor versions

    David Rowley <dgrowleyml@gmail.com> — 2024-04-20T01:56:48Z

    On Tue, 16 Apr 2024 at 15:16, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Apr 16, 2024 at 02:04:22PM +1200, David Rowley wrote:
> > It makes sense to talk about the hashing variations closer to the
> > object identifier part.
>
> Mostly what I had in mind.  A separate <para> for the new part you are
> adding at the end of the first part feels a bit more natural split
> here.  Feel free to discard my comment if you think that's not worth
> it.

Thanks for the review. I've now pushed this, backpatching to 12.

David

  20. Re: Stability of queryid in minor versions

    Michael Paquier <michael@paquier.xyz> — 2024-04-20T02:59:45Z

    On Sat, Apr 20, 2024 at 01:56:48PM +1200, David Rowley wrote:
> Thanks for the review. I've now pushed this, backpatching to 12.

You've split that into two separate paragraphs with 2d3389c28c5c.
Thanks for the commit.
--
Michael