Thread: Some oversights in query_id calculation

Hi,

While doing some sanity checks on the regression tests, I found some queries
that are semantically different but end up with identical query_id.

Two are an old issues:

- the "ONLY" in FROM [ONLY] isn't hashed
- the agglevelsup field in GROUPING isn't hashed

Another one was introduced in pg13 with the WITH TIES not being hashed.

The last one new in pg14: the "DISTINCT" in "GROUP BY [DISTINCT]" isn't hash.

I'm attaching a patch that fixes those, with regression tests to reproduce each
problem.

There are also 2 additional debatable cases on whether this is a semantic
difference or not:

- aliases aren't hashed.  That's usually not a problem, except when you use
  row_to_json(), since you'll get different keys

- the NAME in XmlExpr (eg: xmlpi(NAME foo,...)) isn't hashed, so you generate
  different elements

Hi Julien,

> I'm attaching a patch that fixes those, with regression tests to reproduce each
> problem.

I believe something could be not quite right with the patch. Here is what I did:

$ git apply ...
# revert the changes in the code but keep the new tests
$ git checkout src/backend/utils/misc/queryjumble.c
$ ./full-build.sh && single-install.sh && make installcheck-world

... where named .sh scripts are something I use to quickly check a patch [1].

I was expecting that several tests will fail but they didn't. Maybe I
missed something?

[1]: https://github.com/afiskon/pgscripts

-- 
Best regards,
Aleksander Alekseev

Hi Aleksander,

On Wed, Apr 28, 2021 at 01:19:36PM +0300, Aleksander Alekseev wrote:
> Hi Julien,
> 
> > I'm attaching a patch that fixes those, with regression tests to reproduce each
> > problem.
> 
> I believe something could be not quite right with the patch. Here is what I did:
> 
> $ git apply ...
> # revert the changes in the code but keep the new tests
> $ git checkout src/backend/utils/misc/queryjumble.c
> $ ./full-build.sh && single-install.sh && make installcheck-world
> 
> ... where named .sh scripts are something I use to quickly check a patch [1].
> 
> I was expecting that several tests will fail but they didn't. Maybe I
> missed something?

I think it's because installcheck-* don't run pg_stat_statements' tests, see
its Makefile:

> # Disabled because these tests require "shared_preload_libraries=pg_stat_statements",
> # which typical installcheck users do not have (e.g. buildfarm clients).
> NO_INSTALLCHECK = 1

You should see failures doing a check-world or simply a make -C
contrib/pg_stat_statements check

Hi Aleksander,

On Wed, Apr 28, 2021 at 03:22:39PM +0300, Aleksander Alekseev wrote:
> Hi Julien,
> 
> > You should see failures doing a check-world or simply a make -C
> > contrib/pg_stat_statements check
> 
> Sorry, my bad. I was running make check-world, but did it with -j4 flag
> which was a mistake.
> 
> The patch is OK.

Thanks for reviewing!

On Sun, May  2, 2021 at 12:27:37PM +0800, Julien Rouhaud wrote:
> Hi Aleksander,
> 
> On Wed, Apr 28, 2021 at 03:22:39PM +0300, Aleksander Alekseev wrote:
> > Hi Julien,
> > 
> > > You should see failures doing a check-world or simply a make -C
> > > contrib/pg_stat_statements check
> > 
> > Sorry, my bad. I was running make check-world, but did it with -j4 flag
> > which was a mistake.
> > 
> > The patch is OK.
> 
> Thanks for reviewing!

Patch applied, thanks.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  If only the physical world exists, free will is an illusion.

On Mon, May 03, 2021 at 02:59:42PM -0400, Bruce Momjian wrote:
> On Sun, May  2, 2021 at 12:27:37PM +0800, Julien Rouhaud wrote:
> > Hi Aleksander,
> > 
> > On Wed, Apr 28, 2021 at 03:22:39PM +0300, Aleksander Alekseev wrote:
> > > Hi Julien,
> > > 
> > > > You should see failures doing a check-world or simply a make -C
> > > > contrib/pg_stat_statements check
> > > 
> > > Sorry, my bad. I was running make check-world, but did it with -j4 flag
> > > which was a mistake.
> > > 
> > > The patch is OK.
> > 
> > Thanks for reviewing!
> 
> Patch applied, thanks.

Thanks a lot Bruce!