Support PG 14

[jmo-qap]

As of 2021-09-30 postgres 14 was released: https://www.postgresql.org/about/news/postgresql-14-released-2318/

There's (even more) new JSON syntax along with several other FUNCTION/PROCEDURE syntax additions.

[lfittl]

@jmo-qap Yup, thanks for opening an issue to track this - I will see if I can prioritize this in the near future.

[sgreene570]

Hey @lfittl !

We (@jmo-qap and myself) are very interested in seeing libgpg_query support postgres 14 (since that would enable postgres 14 support for pglast).

Do you have a rough idea of what changes need to happen to this repo to enable support for postgres 14?
We would like to contribute to this effort if possible.

Looking at previous postgres "bump" PR's looks like the upgrade process at a super high level is as follows:

• Checkout desired postgres version locally
• Run some set of the scripts in scripts/ to copy upstream source over (looks like theres a make directive for this).
• Author and commit any necessary patch files in patches/
• Other code changes to make things work, expose new features, unit tests, examples, etc.

If you could provide any input, or share any documentation around this process that I may missed, that would be great!

BTW, is there any community slack channels, mailing lists, forum boards, etc for libpq_query and other upstream offerings from pganalyze? Something on the pg community slack?

[jmo-qap]

Thanks for tackling this @sgreene570

[lfittl]

@sgreene570 Generally speaking, the complex part is determining whether the fingerprinting and normalization logic need any changes.

The core parser itself is usually just a quick run of the make extract_source script (that downloads a Postgres tarball and applies patches by itself) - but sometimes there are more complexities if any of the small patches applied on top no longer apply.

This week is particularly busy here, but I'll see if I can set aside some time soon to get up a first version of this that you can help test. Whilst you are welcome to run through the logic yourself (I'd start with that extract_source task), the documentation is rather basic at this point :)

Re: Community Slack / etc - good question! We don't have a dedicated Slack for pganalyze, or associated projects, but I do hang out in the Postgres Slack myself (same name as on here), so feel free to ping me there directly with questions.

[sgreene570]

Thanks for the info @lfittl !

I took a stab at this and was able to get the patches applied to upstream 14.2. However, I was unable to fix patches/02_parser_support_question_mark_as_param_ref.patch, which may be incompatible with the postgres 14 grammar (or at least not trivially fixed). But sounds like that patch should ultimately be dropped at some point anyways.

After running make extract_source, I am unable to build libpg_query. Running make outputs a bunch of missing variable declaration errors for src/postgres/src_backend_utils_error_elog.c. Looking at the diff for that file generated by make extract_source, I can see almost all symbols have been removed from the "Symbols referenced in this file" section, which seems like a bug in the extraction script.

I then checked out the latest commit on the 13-latest branch and tried to see if I could reproduce this issue elsewhere. Running make extract_source on that branch generated a large diff, which seems unexpected, and also lead to the same build errors with elog.c. Have you run into issues with how the extraction script handles this particular file before?

If you'd like, I could open a separate issue on the elog.c topic (and maybe even another issue on why make extract_source is not idempotent on the current main branch) but figured i'd ask here first in case there's any known files that make extract_source modifies that need special attention during a PG "bump". Thanks in advance!

[sgreene570]

Ah, looks like my problem was trying to use the make extract_source command on an M1 macbook pro. Running the script on an intel mac, things work out better and I dont get the elog.c errors mentioned in my prior comment. Not sure why, but I suspect there's some subtle differences between clang/other tools on M1 vs intel.

[sgreene570]

I've made some progress on this, but not enough to open a preliminary pull request.
I'll continue taking a stab at this, but for anyone else following along at home, I have identified these three commits as "breaking" changes in PG 14.2 w.r.t libpg_query:

postgres/postgres@926fa80
postgres/postgres@a676386
postgres/postgres@a3dc926

[sgreene570]

Chipping away at the compilation errors that arise after running all of the extract/generation scripts, i've found 2 more commits that could be considered "breaking" changes when bumping to PG 14.2:

postgres/postgres@1788828
postgres/postgres@844fe9f

[lelit]

As one of my own usages of pglast approaches PG14, I gently wonder if there is any ETA on this.

[sgreene570]

As one of my own usages of pglast approaches PG14, I gently wonder if there is any ETA on this.

@lfittl have you had the chance to look into this issue at all?

I have identified a few upstream breaking commits, but its unclear to me how many more non-trivial upstream commits there are, especially now that PG 14.3 is out the door.

[lfittl]

@lfittl have you had the chance to look into this issue at all?

I have identified a few upstream breaking commits, but its unclear to me how many more non-trivial upstream commits there are, especially now that PG 14.3 is out the door.

Pretty swamped currently with getting a release out of the door, but its on my list to look into this as part of our next product sprint here (~ late June is probably a realistic estimate for me to get to this)

[df7cb]

Fwiw, I'd also appreciate a new release with PG14 support since that would allow me to upload pglast and sqlreduce to Debian which has PG14 only, and postgresql-server-dev-13 has already been removed.

[lelit]

that would allow me to upload pglast and sqlreduce to Debian which has PG14 only

OOC, can you explain why the availability of the PG version affects the possibility to upload them? Isn't pglast basically a statically linked extension, that thus is indipendent from the actual version of PG you use?

[df7cb]

@lelit: Even when pglast is a static lib, we need to build it, and Debian requires that to be done from things that are present within Debian. postgresql-server-dev-14 isn't enough:

creating build/temp.linux-x86_64-3.9/pglast
x86_64-linux-gnu-gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O2 -Wall -g -fstack-protector-strong -Wformat -Werror=format-security -g -fwrapv -O2 -g -O2 -ffile-prefix-map=/home/cbe/projects/postgresql/pglast/pglast=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -I/home/cbe/projects/postgresql/pglast/pglast/libpg_query -I/usr/include/postgresql/13/server -I/home/cbe/projects/postgresql/pglast/pglast/libpg_query/vendor -I/usr/include/python3.9 -c pglast/parser.c -o build/temp.linux-x86_64-3.9/pglast/parser.o
pglast/parser.c:713:10: fatal error: postgres.h: Datei oder Verzeichnis nicht gefunden
  713 | #include "postgres.h"

Forcing that to use -I/usr/include/postgresql/14/server doesn't work:

x86_64-linux-gnu-gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O2 -Wall -g -fstack-protector-strong -Wformat -Werror=format-security -g -fwrapv -O2 -g -O2 -ffile-prefix-map=/home/cbe/projects/postgresql/pglast/pglast=. -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -I/home/cbe/projects/postgresql/pglast/pglast/libpg_query -I/usr/include/postgresql/14/server -I/home/cbe/projects/postgresql/pglast/pglast/libpg_query/vendor -I/usr/include/python3.9 -c pglast/parser.c -o build/temp.linux-x86_64-3.9/pglast/parser.o
... some warnings ...
pglast/parser.c: In function ‘__pyx_f_6pglast_6parser_create_AlterTableStmt’:
pglast/parser.c:26631:55: error: ‘AlterTableStmt’ has no member named ‘relkind’
26631 |   __pyx_t_6 = __Pyx_PyInt_From_ObjectType(__pyx_v_data->relkind); if (unlikely(!__pyx_t_6)) __PYX_ERR(0, 1876, __pyx_L1_error)
      |                                                       ^~
...

[lelit]

@df7cb: thank you, but I'm still confused: did you have to patch in some way pglast sources? I cannot check immediately, but I'm confused by that reference to the system PG headers, that should not happen... in other words, pglast should be compiled and linked against the libpg_query headers/library, that in turn should be isolated/independent from the availability of system PG.

[df7cb]

@lelit you are right, and I'm sorry for not having forwarded this in the beginning (and then forgetting that it exists). There is indeed a patch in pglast.deb that makes it use the system PG headers instead of the files in the libpg_query source:
https://salsa.debian.org/postgresql/pglast/-/blob/master/debian/patches/libpq-query

The problem there is that we can only build-depend on things that are installed as part of some package, not on the source code on other packages (as BuildLibPgQueryFirst in pglast's setup.py wants to). I had to pick between shoveling a large set of postgresql server headers into libpg-query or pglast to make it build, or have pglast build-depend on postgresql-server-dev-13 where the files are located. The second option is much simpler.

[lelit]

Ah, ok then.

Maybe @lfittl may chime in to confirm that libpg_query itself does not alter the PG include files it vendors?

Because that would be another problem for you...

[lelit]

@df7cb: at least one patch seems problematic, as it changes the order of the items of an enum, and that would mean that you should update the related definition in pglast.
I'm not able to determine the impact of other patches that libpg_query applies to pristine PG sources.

[lelit]

Hi @lfittl, any news on this?

[lelit]

FYI, I have found https://github.com/tlisanti/libpg_query/tree/14-latest in a good shape: while it has some glitches (one cured here, the other related to statements signature), I was able to use that branch to support PG 14 in pglast v4.
Many thanks to @tlisanti and to @wolfgangwalther!

[lfittl]

Thanks @lelit for the update (and everyone else for their patience) - and thanks @tlisanti and @wolfgangwalther for their efforts!

We've prioritized our effort to get this updated here in the upstream repo as well, @msepga on the pganalyze team is starting to review the open PR (the changes look good from a first review) and also review additional changes by @tlisanti and we should have the official branch updated here soon.

[lfittl]

Closing, as this has been completed in #165 and the 14-3.0.0 release has been tagged.

Thanks @msepga for driving this release, and again thanks @tlisanti and @wolfgangwalther for their contributions!