Upgrade to Postgres 16

[msepga]

TODO:

• Fix remaining failing deparse tests
• Fix unoptimized debug builds
• Fix leaks detected by valgrind

[lfittl]

@msepga FWIW, I think the following log output when building may be worth investigating:

(1)

 In file included from src/pg_query_outfuncs_json.c:272:
src/pg_query_outfuncs_defs.c:1243:3: warning: format specifies type 'unsigned int' but the argument has type 'AclMode' (aka 'unsigned long') [-Wformat]
  WRITE_UINT_FIELD(required_perms, requiredPerms, requiredPerms);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/pg_query_outfuncs_json.c:36:66: note: expanded from macro 'WRITE_UINT_FIELD'
                appendStringInfo(out, "\"" CppAsString(outname_json) "\":%u,", node->fldname); \
                                                                         ~~    ^~~~~~~~~~~~~
1 warning generated.
compiling src/pg_query_fingerprint.c
In file included from src/pg_query_fingerprint.c:263:
src/pg_query_fingerprint_defs.c:5897:27: warning: format specifies type 'int' but the argument has type 'AclMode' (aka 'unsigned long') [-Wformat]
    sprintf(buffer, "%d", node->requiredPerms);
                     ~~   ^~~~~~~~~~~~~~~~~~~
                     %lu
1 warning generated.

That looks like we maybe got the wrong type somewhere for AclMode - possibly that needs to be added somewhere in the extraction logic that is used as a source for generating those files.

(2)

compiling src/postgres_deparse.c
src/postgres_deparse.c:3286:10: warning: enumeration value 'JOIN_RIGHT_ANTI' not handled in switch [-Wswitch]
        switch (join_expr->jointype)
                ^
1 warning generated.

It appears JOIN_RIGHT_ANTI is only used by the planner, not during raw parsing, at least if I read https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=16dc2703c5413534d4989e08253e8f4fcb0e2aab correctly. So I think that just needs to be added next to JOIN_ANTI in that case statement, to avoid the warning:

libpg_query/src/postgres_deparse.c

Line 3302 in bd15fd5

case JOIN_ANTI:

[msepga]

The AclMode issue should be fixed in d2f9781, and we now add a case for JOIN_RIGHT_ANTI in 6a47fa4.

I've updated the changes here to match Postgres 16.1 and resolved remaining test failures. It looks like there are still some valgrind issues to work out here before this is ready to merge.

[msepga]

Looks like the valgrind issue was actually triggered by chance here; the new incremental_sort.sql test from upstream is exactly 12288 bytes, which is exactly 3 times the typical 4096 page size used with mmap. This means loading the test wasn't providing us with free null terminator bytes (typical when mmap zeros the rest of the page).

Patched in 397ccc9.

[lelit]

Hi @msepga, I'm starting to upgrade my pglast to this...

I see that the extracted list of node tags are missing the usual T_ prefix: is that expected/on purpose?

[msepga]

I see that the extracted list of node tags are missing the usual T_ prefix: is that expected/on purpose?

Thanks for spotting that @lelit 🙂 Fixed in 3a64b99 & regenerated in f716046.

[seanlinsley]

I'm not sure if we have a process to keep the different branches in sync, but it looks like 16-latest is 3 commits behind 15-latest. This PR includes one of those commits 16-latest...15-latest

[lelit]

I almost completed the upgrade of pglast (see its v6 branch) and all tests are green 🥳

On the GH Actions, there's a failure on the job that builds a 32bit wheel: it's related to the fingerprinting functionality, does that ring any bell to you?

[lfittl]

I almost completed the upgrade of pglast (see its v6 branch) and all tests are green 🥳

Excellent, thanks for the testing!

FYI, I discussed the plan for releasing this PR with @msepga earlier today - the plan is to merge most of the open PRs into 15-latest first, then rebase this, and then make the actual 16-latest release at the end of the week.

Thus, I'd recommend not making an official release of the pglast v6 branch just yet, until we tagged a release here, just to avoid any unexpected changes when we do that rebase.

On the GH Actions, there's a failure on the job that builds a 32bit wheel: it's related to the fingerprinting functionality, does that ring any bell to you?

Copying the error for easier context:

         In file included from ./src/postgres/include/access/htup_details.h:20,
                         from ./src/postgres/include/executor/tuptable.h:18,
                         from ./src/postgres/include/access/tupconvert.h:20,
                         from ./src/postgres/include/nodes/execnodes.h:32,
                         from ./src/postgres/include/catalog/index.h:18,
                         from src/postgres_deparse.c:2:
        ./src/postgres/include/access/tupmacs.h: In function ‘fetch_att’:
        ./src/postgres/include/access/tupmacs.h:65:4: error: duplicate case value
           65 |    case sizeof(Datum):
              |    ^~~~
        ./src/postgres/include/access/tupmacs.h:62:4: note: previously used here
           62 |    case sizeof(int32):
              |    ^~~~

This looks to me like that 32-bit build is using the wrong setting for SIZEOF_DATUM - we don't really support 32-bit builds, but if you wanted to make them work, you'd have to override the SIZEOF_VOID_P and other size-related flags defined in pg_config.h (these are usually set by autoconf / meson in regular Postgres, but in case of libpg_query we hardcode them).

I don't think that's a new problem on the 16 branch, though it could be that for some reason this worked by coincidence in earlier versions. For now I'd suggest we discuss 32-bit issues separately from this PR.

[lelit]

FYI, I discussed the plan for releasing this PR with @msepga earlier today - the plan is to merge most of the open PRs into 15-latest first, then rebase this, and then make the actual 16-latest release at the end of the week.

Thus, I'd recommend not making an official release of the pglast v6 branch just yet, until we tagged a release here, just to avoid any unexpected changes when we do that rebase.

Sure, there's no hurry.

On the GH Actions, there's a failure...

This looks to me like that 32-bit build is using the wrong setting for SIZEOF_DATUM - we don't really support 32-bit builds, but if you wanted to make them work, you'd have to override the SIZEOF_VOID_P and other size-related flags defined in pg_config.h (these are usually set by autoconf / meson in regular Postgres, but in case of libpg_query we hardcode them).

Thank you, I will try to understand what's going on.

I don't think that's a new problem on the 16 branch, though it could be that for some reason this worked by coincidence in earlier versions. For now I'd suggest we discuss 32-bit issues separately from this PR.

Ok, will report back somewhere else if I find something interesting.

[lfittl]

✅ I've tested this, reviewed the files to make sure we don't pull in unnecessary code, and confirmed that the protobuf compiler now (after the update to 25.1) produces the exact protobuf/ folder contents for me locally.

Nicely done 🥳

Also, FYI, I pushed up some more deparser fixes in #229 -- we could merge those in separately after this is merged, or you can pull this into this PR, as you prefer.

[lfittl]

This looks to me like that 32-bit build is using the wrong setting for SIZEOF_DATUM - we don't really support 32-bit builds, but if you wanted to make them work, you'd have to override the SIZEOF_VOID_P and other size-related flags defined in pg_config.h (these are usually set by autoconf / meson in regular Postgres, but in case of libpg_query we hardcode them).

Thank you, I will try to understand what's going on.

I don't think that's a new problem on the 16 branch, though it could be that for some reason this worked by coincidence in earlier versions. For now I'd suggest we discuss 32-bit issues separately from this PR.

Ok, will report back somewhere else if I find something interesting.

@lelit FWIW, it appears there was actually a new issue on 16, which is that we were missing some of the functions required when building with 32-bit.

I ended up putting a bit of effort into this, so that we can actually support 32-bit out of the box with some extra define/undefs + extracting a bit more code. See #232