Re: pgsql: psql: add an optional execution-count limit to \watch.
On 2023-04-07 Fr 10:00, Tom Lane wrote: Alexander Korotkov writes: On Thu, Apr 6, 2023 at 8:18 PM Tom Lane wrote: psql: add an optional execution-count limit to \watch. This commit makes tests fail for me. psql parses 'i' option of '\watch' using locale-aware strtod(), but 001_basic.pl uses hard-coded decimal separator. Huh, yeah, I see it too if I set LANG=ru_RU.utf8 before running psql's TAP tests. It seems unfortunate that none of the buildfarm has noticed this. I guess all the TAP tests are run under C locale? [just noticed this, redirecting to -hackers] When run under meson, yes unless the LANG/LC_* settings are explicitly in the build_env. I'm fixing that so we will allow them to pass through. When run with configure/make they run with whatever is in the calling environment unless overridden in the build_env. We do have support for running installchecks with multiple locales.This is done by passing --locale=foo to initdb. We could locale-enable the non-install checks (for meson builds, that's the 'misc-check' step, for configure/make builds it's more or less everything between the install stages and the (first) initdb step. We'd have to do that via appropriate environment settings, I guess. Would it be enough to set LANG, or do we need to set the LC_foo settings individually? Not sure how we manage it on Windows. Maybe just not enable it for the first go-round. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: New committers: Melanie Plageman, Richard Guo
On 2024-04-26 Fr 07:54, Jonathan S. Katz wrote: The Core Team would like to extend our congratulations to Melanie Plageman and Richard Guo, who have accepted invitations to become our newest PostgreSQL committers. Please join us in wishing them much success and few reverts! Very happy about both of these. Many congratulations to Melanie and Richard. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-04-24 We 04:56, Michael Paquier wrote: On Fri, Apr 19, 2024 at 02:04:40PM -0400, Robert Haas wrote: Yeah, I think this patch invented a new solution to a problem that we've solved in a different way everywhere else. I think we should change it to match what we do in general. As of ba3e6e2bca97, did you notice that test_json_parser_perf generates two core files because progname is not set, failing an assertion when using the frontend logging? No, it didn't for me. Thanks for noticing, I've pushed a fix. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Why does pgindent's README say to download typedefs.list from the buildfarm?
On 2024-04-24 We 06:12, Peter Eisentraut wrote: On 22.04.24 22:28, Tom Lane wrote: Nathan Bossart writes: On Mon, Apr 22, 2024 at 04:08:08PM -0400, Tom Lane wrote: I think the actual plan now is that we'll sync the in-tree copy with the buildfarm's results (and then do a tree-wide pgindent) every so often, probably shortly before beta every year. Okay. Is this just to resolve the delta between the manual updates and a clean autogenerated copy every once in a while? The main reason there's a delta is that people don't manage to maintain the in-tree copy perfectly (at least, they certainly haven't done so for this past year). So we need to do that to clean up every now and then. A secondary reason is that the set of typedefs we absorb from system include files changes over time. Is the code to extract typedefs available somewhere independent of the buildfarm? It would be useful sometimes to be able to run this locally, like before and after some patch, to keep the in-tree typedefs list updated. There's been talk about it but I don't think anyone's done it. I'd be more than happy if the buildfarm client could just call something in the core repo (c.f. src/test/perl/Postgres/Test/AdjustUpgrade.pm). Regarding testing with your patch, some years ago I wrote this blog post: <http://adpgtech.blogspot.com/2015/05/running-pgindent-on-non-core-code-or.html> cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Why does pgindent's README say to download typedefs.list from the buildfarm?
On 2024-04-23 Tu 06:23, Alvaro Herrera wrote:
But there are others: InjectionPointEntry, ResourceOwnerData,
JsonNonTerminal, JsonParserSem, ...
The last two are down to me. Let's just get rid of them like the
attached (no need for a typedef at all)
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index 12fabcaccf..fc0cb36974 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -50,16 +50,16 @@ typedef enum /* contexts of JSON parser */
* tokens, non-terminals, and semantic action markers.
*/
-typedef enum
+enum JsonNonTerminal
{
JSON_NT_JSON = 32,
JSON_NT_ARRAY_ELEMENTS,
JSON_NT_MORE_ARRAY_ELEMENTS,
JSON_NT_KEY_PAIRS,
JSON_NT_MORE_KEY_PAIRS,
-} JsonNonTerminal;
+};
-typedef enum
+enum JsonParserSem
{
JSON_SEM_OSTART = 64,
JSON_SEM_OEND,
@@ -72,7 +72,7 @@ typedef enum
JSON_SEM_AELEM_END,
JSON_SEM_SCALAR_INIT,
JSON_SEM_SCALAR_CALL,
-} JsonParserSem;
+};
/*
* struct containing the 3 stacks used in non-recursive parsing,
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index d551ada325..ba48a3371d 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1312,14 +1312,12 @@ JsonManifestParseIncrementalState
JsonManifestParseState
JsonManifestSemanticState
JsonManifestWALRangeField
-JsonNonTerminal
JsonObjectAgg
JsonObjectConstructor
JsonOutput
JsonParseExpr
JsonParseContext
JsonParseErrorType
-JsonParserSem
JsonParserStack
JsonPath
JsonPathBool
Re: pgsql: Introduce "builtin" collation provider.
On 2024-03-14 Th 02:39, Jeff Davis wrote: Introduce "builtin" collation provider. The new value "b" for pg_collation.collprovider doesn't seem to be documented. Is that deliberate? cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: pgsql: Fix restore of not-null constraints with inheritance
On 2024-04-18 Th 11:39, Alvaro Herrera wrote: On 2024-Apr-18, Alvaro Herrera wrote: On 2024-Apr-18, Alvaro Herrera wrote: Lastly, make two changes to pg_dump: 1) do not try to drop a not-null constraint that's marked as inherited; this allows a dump to restore with no errors if a table with a PK inherits from another which also has a PK; 2) avoid giving inherited constraints throwaway names, for the rare cases where such a constraint survives after the restore. Hmm, this last bit broke pg_upgrade on crake. I'll revert this part, meanwhile I'll be installing 9.2 to see if it can be fixed in a better way. Eh, so: 1. running "make check" on pg_upgrade using an oldinstall pointing to 9.2 fails, because PostgreSQL::Test::Cluster doesn't support that version -- it only goes back to 9.2. How difficult was it to port it back to all these old versions? It's not that hard to make it go back to 9.2. Here's a version that's a couple of years old, but it supports versions all the way back to 7.2 :-) If there's interest I'll work on supporting our official "old" versions (i.e. 9.2 and up) 2. running "make check" with an oldinstall pointing to 10 fails, because the "invalid database" checks fail: not ok 7 - invalid database causes failure status (got 0 vs expected 1) # Failed test 'invalid database causes failure status (got 0 vs expected 1)' # at t/002_pg_upgrade.pl line 407. not ok 8 - invalid database causes failure stdout /(?^:invalid)/ 3. Lastly, even if I put back the code that causes the failures on crake and restore from 10 (and ignore those two problems), I cannot reproduce the issues it reported. Is crake running some funky code that's not what "make check" on pg_upgrade does, perchance? It's running the buildfarm cross version upgrade module. See <https://github.com/PGBuildFarm/client-code/blob/main/PGBuild/Modules/TestUpgradeXversion.pm> It's choking on the change in constraint names between the dump of the pre-upgrade database and the dump of the post-upgrade database, e.g. CREATE TABLE public.rule_and_refint_t2 ( -id2a integer CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL NO INHERIT, -id2c integer CONSTRAINT pgdump_throwaway_notnull_1 NOT NULL NO INHERIT +id2a integer CONSTRAINT rule_and_refint_t2_id2a_not_null NOT NULL NO INHERIT, +id2c integer CONSTRAINT rule_and_refint_t2_id2c_not_null NOT NULL NO INHERIT ); look at the dumpdiff-REL9_2_STABLE file for the full list. I assume that means pg_dump is generating names that pg_upgrade throws away? That seems ... unfortunate. There is a perl module at src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm. This is used to adjust the dump files before we diff them. Maybe you can remedy the problem by adding some code in there. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com Cluster.pm Description: Perl program
Re: WIP Incremental JSON Parser
On 2024-04-18 Th 02:04, Michael Paquier wrote: On Wed, Apr 10, 2024 at 07:47:38AM -0400, Andrew Dunstan wrote: Here's v2 of the cleanup patch 4, that fixes some more typos kindly pointed out to me by Alexander Lakhin. I can see that this has been applied as of 42fa4b660143 with some extra commits. Anyway, I have noticed another thing in the surroundings that's annoying. 003 has this logic: useFile::Temp qw(tempfile); [...] my ($fh, $fname) = tempfile(); print $fh $stdout,"\n"; close($fh); This creates a temporary file in /tmp/ that remains around, slowing bloating the temporary file space on a node while leaving around some data. My bad, I should have used the UNLINK option like in the other tests. Why usingFile::Temp::tempfile here? Couldn't you just use a file in a PostgreSQL::Test::Utils::tempdir() that would be cleaned up once the test finishes? That's another possibility, but I think the above is the simplest. Per [1], escape_json() has no coverage outside its default path. Is that intended? Not particularly. I'll add some stuff to get complete coverage. Documenting all these test files with a few comments would be welcome, as well, with some copyright notices... ok json_file = fopen(testfile, "r"); fstat(fileno(json_file), ); bytes_left = statbuf.st_size; No checks on failure of fstat() here? ok will fix json_file = fopen(argv[2], "r"); Second one in test_json_parser_perf.c, with more stuff for fread(). ok will fix cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: HEAD build error on Fedora 39
On 2024-04-15 Mo 05:59, Devrim Gündüz wrote: Hi, I'm unable to build the latest snapshot on my Fedora 39 box. I think this problem appeared before the weekend (not sure, though). This is libxml 2.10.4: === '/usr/bin/perl' ../../../src/backend/utils/activity/generate-wait_event_types.pl --docs ../../../src/backend/utils/activity/wait_event_names.txt /usr/bin/xmllint --nonet --path . --path . --output postgres-full.xml --noent --valid postgres.sgml I/O error : Attempt to load network entityhttp://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd postgres.sgml:21: warning: failed to load external entity"http://www.oasis-open.org/docbook/xml/4.5/docbookx.dtd; ]> It's working on my Fedora 39. This error suggests to me that you don't have docbook-dtds installed. If you do, then I don't know :-) cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: Should we add a compiler warning for large stack frames?
On 2024-04-11 Th 16:17, Andres Freund wrote: 128k is probably not going to be an issue in practice. However, it also seems not great from a performance POV to use this much stack in a function that's called fairly often. I'd allocate the buffer in verify_backup_checksums() and reuse it across all to-be-checked files. Yes, I agree. I'll make that happen in the next day or two. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: Should we add a compiler warning for large stack frames?
On 2024-04-11 Th 15:01, Andres Freund wrote: Hi, d8f5acbdb9b made me wonder if we should add a compiler option to warn when stack frames are large. gcc compatible compilers have -Wstack-usage=limit, so that's not hard. Huge stack frames are somewhat dangerous, as they can defeat our stack-depth checking logic. There are also some cases where large stack frames defeat stack-protector logic by compilers/libc/os. It's not always obvious how large the stack will be. Even if you look at all the sizes of the variables defined in a function, inlining can increase that substantially. Here are all the cases a limit of 64k finds. [1345/1940 42 69%] Compiling C object src/bin/pg_verifybackup/pg_verifybackup.p/pg_verifybackup.c.o ../../../../../home/andres/src/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c: In function 'verify_file_checksum': ../../../../../home/andres/src/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c:842:1: warning: stack usage is 131232 bytes [-Wstack-usage=] 842 | verify_file_checksum(verifier_context *context, manifest_file *m, | ^~~~ This one's down to me. I asked Robert some time back why we were using a very conservative buffer size, and he agreed we could probably make it larger, but the number chosen is mine, not his. It was a completely arbitrary choice. I'm happy to reduce it, but it's not clear to me why we care that much for a client binary. There's no stack depth checking going on here. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: Typos in the code and README
On 2024-04-11 Th 09:05, Daniel Gustafsson wrote: Now that the tree has settled down a bit post-freeze I ran some tooling to check spelling. I was primarily interested in docs and README* which were mostly free from simply typos, while the code had some in various comments and one in code. The attached fixes all that I came across (not cross-referenced against ongoing reverts or any other fixup threads but will be before pushing of course). I have these covered: src/test/modules/test_json_parser/README | 2 +- .../test_json_parser/test_json_parser_incremental.c | 4 ++-- src/test/modules/test_json_parser/test_json_parser_perf.c | 2 +- cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: type in basebackup_incremental.c ?
On 2024-04-11 Th 06:15, Daniel Gustafsson wrote: On 11 Apr 2024, at 11:49, Daniel Westermann (DWE) wrote: Hi, /* * we expect the find the last lines of the manifest, including the checksum, * in the last MIN_CHUNK bytes of the manifest. We trigger an incremental * parse step if we are about to overflow MAX_CHUNK bytes. */ Shouldn't this be: /* * we expect to find the last lines of the manifest,... */ That sounds about right, and since it's a full sentence it should also start with a capital 'W': "We expect to find the..". Thanks, I will include that in the cleanups I'm intending to push today. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-04-09 Tu 15:42, Andrew Dunstan wrote: On 2024-04-09 Tu 07:54, Andrew Dunstan wrote: On 2024-04-09 Tu 01:23, Michael Paquier wrote: On Tue, Apr 09, 2024 at 09:48:18AM +0900, Michael Paquier wrote: There is no direct check on test_json_parser_perf.c, either, only a custom rule in the Makefile without specifying something for meson. So it looks like you could do short execution check in a TAP test, at least. Not adding a test for that was deliberate - any sane test takes a while, and I didn't want to spend that much time on it every time someone runs "make check-world" or equivalent. However, adding a test to run it with a trivial number of iterations seems reasonable, so I'll add that. I'll also add a meson target for the binary. While reading the code, I've noticed a few things, as well: + /* max delicious line length is less than this */ + char buff[6001]; Delicious applies to the contents, nothing to do with this code even if I'd like to think that these lines of code are edible and good. Using a hardcoded limit for some JSON input sounds like a bad idea to me even if this is a test module. Comment that applies to both the perf and the incremental tools. You could use a #define'd buffer size for readability rather than assuming this value in many places. The comment is a remnant of when I hadn't yet added support for incomplete tokens, and so I had to parse the input line by line. I agree it can go, and we can use a manifest constant for the buffer size. +++ b/src/test/modules/test_json_parser/test_json_parser_incremental.c + * This progam tests incremental parsing of json. The input is fed into + * full range of incement handling, especially in the lexer, is exercised. +++ b/src/test/modules/test_json_parser/test_json_parser_perf.c + * Performancet est program for both flavors of the JSON parser + * This progam tests either the standard (recursive descent) JSON parser +++ b/src/test/modules/test_json_parser/README + reads in a file and pases it in very small chunks (60 bytes at a time) to Collection of typos across various files. Will fix. (The older I get the more typos I seem to make and the harder it is to notice them. It's very annoying.) + appendStringInfoString(, "1+23 trailing junk"); What's the purpose here? Perhaps the intention should be documented in a comment? The purpose is to ensure that if there is not a trailing '\0' on the json chunk the parser will still do the right thing. I'll add a comment to that effect. At the end, having a way to generate JSON blobs randomly to test this stuff would be more appealing than what you have currently, with perhaps a few factors like: - Array and simple object density. - Max Level of nesting. - Limitation to ASCII characters that can be escaped. - Perhaps more things I cannot think about? No, I disagree. Maybe we need those things as well, but we do need a static test where we can test the output against known results. I have no objection to changing the input and output files. It's worth noting that t/002_inline.pl does generate some input and test e.g., the maximum nesting levels among other errors. Perhaps you missed that. If you think we need more tests there adding them would be extremely simple. So the current state of things is kind of disappointing, and the size of the data set added to the tree is not that portable either if you want to test various scenarios depending on the data set. It seems to me that this has been committed too hastily and that this is not ready for integration, even if that's just a test module. Tom also has shared some concerns upthread, as far as I can see. I have posted a patch already that addresses the issue Tom raised. Here's a consolidated set of cleanup patches, including the memory leak patch and Jacob's shrink-tiny patch. Here's v2 of the cleanup patch 4, that fixes some more typos kindly pointed out to me by Alexander Lakhin. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com From 6b12a71b6b43354c0f897ac5feb1e53419a2c15f Mon Sep 17 00:00:00 2001 From: Andrew Dunstan Date: Tue, 9 Apr 2024 15:30:48 -0400 Subject: [PATCH v2] Assorted minor cleanups in the test_json_parser module Per gripes from Michael Paquier Discussion: https://postgr.es/m/zhtq6_w1vwohq...@paquier.xyz Along the way, also clean up a handful of typos in 3311ea86ed and ea7b4e9a2a, found by Alexander Lakhin. --- src/common/jsonapi.c | 6 ++--- src/common/parse_manifest.c | 2 +- src/test/modules/test_json_parser/README | 19 +++-- .../test_json_parser_incremental.c| 27 ++- .../test_json_parser/test_json_parser_perf.c | 11 5 files changed, 39 insertions(+), 26 deletions(-) diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c index 9dfbc397c0..12fabcaccf 100644 --- a/src/common/jsonap
Re: WIP Incremental JSON Parser
On 2024-04-09 Tu 09:45, Jacob Champion wrote: On Tue, Apr 9, 2024 at 4:54 AM Andrew Dunstan wrote: On 2024-04-09 Tu 01:23, Michael Paquier wrote: There is no direct check on test_json_parser_perf.c, either, only a custom rule in the Makefile without specifying something for meson. So it looks like you could do short execution check in a TAP test, at least. Not adding a test for that was deliberate - any sane test takes a while, and I didn't want to spend that much time on it every time someone runs "make check-world" or equivalent. However, adding a test to run it with a trivial number of iterations seems reasonable, so I'll add that. I'll also add a meson target for the binary. Okay, but for what purpose? My understanding during review was that this was a convenience utility for people who were actively hacking on the code (and I used it for exactly that purpose a few months back, so I didn't question that any further). Why does the farm need to spend any time running it at all? I think Michael's point was that if we carry the code we should test we can run it. The other possibility would be just to remove it. I can see arguments for both. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: IPC::Run::time[r|out] vs our TAP tests
On 2024-04-09 Tu 09:46, Tom Lane wrote: Michael Paquier writes: By the way, are you planning to do something like [1]? I've not looked in details at the callers of IPC::Run::timeout, still the extra debug output would be nice. It needs more review I think - I didn't check every call site to see if anything would be broken. I believe Andrew has undertaken a survey of all the timeout/timer calls, but if he doesn't produce anything I might have a go at it after awhile. What I looked at so far was the use of is_expired, but when you look into that you see that you need to delve further, to where timeout/timer objects are created and passed around. I'll take a closer look when I have done some incremental json housekeeping. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On 2024-04-08 Mo 19:26, Tom Lane wrote: Andrew Dunstan writes: I quite like the triage idea. But I think there's also a case for being more a bit more flexible with those patches we don't throw out. A case close to my heart: I'd have been very sad if the NESTED piece of JSON_TABLE hadn't made the cut, which it did with a few hours to spare, and I would not have been alone, far from it. I'd have been happy to give Amit a few more days or a week if he needed it, for a significant headline feature. I know there will be those who say it's the thin end of the wedge and rulez is rulez, but this is my view. You've certainly been around the project long enough to remember the times in the past when we let the schedule slip for "one more big patch". It just about never worked out well, so I'm definitely in favor of a hard deadline. The trick is to control the tendency to push in patches that are only almost-ready in order to nominally meet the deadline. (I don't pretend to be immune from that temptation myself, but I think I resisted it better than some this year.) If we want to change how things are working I suspect we probably need something a lot more radical than any of the suggestions I've seen floating around. I don't know what that might be, but ISTM we're not thinking boldly enough. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-04-09 Tu 01:23, Michael Paquier wrote: On Tue, Apr 09, 2024 at 09:48:18AM +0900, Michael Paquier wrote: There is no direct check on test_json_parser_perf.c, either, only a custom rule in the Makefile without specifying something for meson. So it looks like you could do short execution check in a TAP test, at least. Not adding a test for that was deliberate - any sane test takes a while, and I didn't want to spend that much time on it every time someone runs "make check-world" or equivalent. However, adding a test to run it with a trivial number of iterations seems reasonable, so I'll add that. I'll also add a meson target for the binary. While reading the code, I've noticed a few things, as well: + /* max delicious line length is less than this */ + charbuff[6001]; Delicious applies to the contents, nothing to do with this code even if I'd like to think that these lines of code are edible and good. Using a hardcoded limit for some JSON input sounds like a bad idea to me even if this is a test module. Comment that applies to both the perf and the incremental tools. You could use a #define'd buffer size for readability rather than assuming this value in many places. The comment is a remnant of when I hadn't yet added support for incomplete tokens, and so I had to parse the input line by line. I agree it can go, and we can use a manifest constant for the buffer size. +++ b/src/test/modules/test_json_parser/test_json_parser_incremental.c + * This progam tests incremental parsing of json. The input is fed into + * full range of incement handling, especially in the lexer, is exercised. +++ b/src/test/modules/test_json_parser/test_json_parser_perf.c + *Performancet est program for both flavors of the JSON parser + * This progam tests either the standard (recursive descent) JSON parser +++ b/src/test/modules/test_json_parser/README + reads in a file and pases it in very small chunks (60 bytes at a time) to Collection of typos across various files. Will fix. (The older I get the more typos I seem to make and the harder it is to notice them. It's very annoying.) + appendStringInfoString(, "1+23 trailing junk"); What's the purpose here? Perhaps the intention should be documented in a comment? The purpose is to ensure that if there is not a trailing '\0' on the json chunk the parser will still do the right thing. I'll add a comment to that effect. At the end, having a way to generate JSON blobs randomly to test this stuff would be more appealing than what you have currently, with perhaps a few factors like: - Array and simple object density. - Max Level of nesting. - Limitation to ASCII characters that can be escaped. - Perhaps more things I cannot think about? No, I disagree. Maybe we need those things as well, but we do need a static test where we can test the output against known results. I have no objection to changing the input and output files. It's worth noting that t/002_inline.pl does generate some input and test e.g., the maximum nesting levels among other errors. Perhaps you missed that. If you think we need more tests there adding them would be extremely simple. So the current state of things is kind of disappointing, and the size of the data set added to the tree is not that portable either if you want to test various scenarios depending on the data set. It seems to me that this has been committed too hastily and that this is not ready for integration, even if that's just a test module. Tom also has shared some concerns upthread, as far as I can see. I have posted a patch already that addresses the issue Tom raised. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: PostgreSQL 17 Release Management Team & Feature Freeze
On 2024-04-08 Mo 12:07, Alvaro Herrera wrote: On 2024-Apr-08, Robert Haas wrote: And maybe we need to think of a way to further mitigate this crush of last minute commits. e.g. In the last week, you can't have more feature commits, or more lines of insertions in your commits, than you did in the prior 3 weeks combined. I don't know. I think this mad rush of last-minute commits is bad for the project. Another idea is to run a patch triage around mid March 15th, with the intention of punting patches to the next cycle early enough. But rather than considering each patch in its own merits, consider the responsible _committers_ and the load that they are reasonably expected to handle: determine which patches each committer deems his or her responsibility for the rest of that March commitfest, and punt all the rest. That way we have a reasonably vetted amount of effort that each committer is allowed to spend for the remainder of that commitfest. Excesses should be obvious enough and discouraged. I quite like the triage idea. But I think there's also a case for being more a bit more flexible with those patches we don't throw out. A case close to my heart: I'd have been very sad if the NESTED piece of JSON_TABLE hadn't made the cut, which it did with a few hours to spare, and I would not have been alone, far from it. I'd have been happy to give Amit a few more days or a week if he needed it, for a significant headline feature. I know there will be those who say it's the thin end of the wedge and rulez is rulez, but this is my view. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-04-08 Mo 09:29, Andrew Dunstan wrote:
On 2024-04-07 Su 20:58, Tom Lane wrote:
Coverity complained that this patch leaks memory:
/srv/coverity/git/pgsql-git/postgresql/src/bin/pg_combinebackup/load_manifest.c:
212 in load_backup_manifest()
206 bytes_left -= rc;
207 json_parse_manifest_incremental_chunk(
208 inc_state, buffer, rc, bytes_left == 0);
209 }
210
211 close(fd);
CID 1596259: (RESOURCE_LEAK)
Variable "inc_state" going out of scope leaks the storage it
points to.
212 }
213
214 /* All done. */
215 pfree(buffer);
216 return result;
217 }
/srv/coverity/git/pgsql-git/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c:
488 in parse_manifest_file()
482 bytes_left -= rc;
483 json_parse_manifest_incremental_chunk(
484 inc_state, buffer, rc, bytes_left == 0);
485 }
486
487 close(fd);
CID 1596257: (RESOURCE_LEAK)
Variable "inc_state" going out of scope leaks the storage it
points to.
488 }
489
490 /* Done with the buffer. */
491 pfree(buffer);
492
493 return result;
It's right about that AFAICS, and not only is the "inc_state" itself
leaked but so is its assorted infrastructure. Perhaps we don't care
too much about that in the existing applications, but ISTM that
isn't going to be a tenable assumption across the board. Shouldn't
there be a "json_parse_manifest_incremental_shutdown()" or the like
to deallocate all the storage allocated by the parser?
yeah, probably. Will work on it.
Here's a patch. In addition to the leaks Coverity found, there was
another site in the backend code that should call the shutdown function,
and a probably memory leak from a logic bug in the incremental json
parser code. All these are fixed.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/src/backend/backup/basebackup_incremental.c b/src/backend/backup/basebackup_incremental.c
index 131598bade..8ae4a62ccc 100644
--- a/src/backend/backup/basebackup_incremental.c
+++ b/src/backend/backup/basebackup_incremental.c
@@ -241,6 +241,9 @@ FinalizeIncrementalManifest(IncrementalBackupInfo *ib)
pfree(ib->buf.data);
ib->buf.data = NULL;
+ /* Done with inc_state, so release that memory too */
+ json_parse_manifest_incremental_shutdown(ib->inc_state);
+
/* Switch back to previous memory context. */
MemoryContextSwitchTo(oldcontext);
}
diff --git a/src/bin/pg_combinebackup/load_manifest.c b/src/bin/pg_combinebackup/load_manifest.c
index 9c9332cdd5..d857ea0006 100644
--- a/src/bin/pg_combinebackup/load_manifest.c
+++ b/src/bin/pg_combinebackup/load_manifest.c
@@ -208,6 +208,9 @@ load_backup_manifest(char *backup_directory)
inc_state, buffer, rc, bytes_left == 0);
}
+ /* Release the incremental state memory */
+ json_parse_manifest_incremental_shutdown(inc_state);
+
close(fd);
}
diff --git a/src/bin/pg_verifybackup/pg_verifybackup.c b/src/bin/pg_verifybackup/pg_verifybackup.c
index 90ef4b2037..9594c615c7 100644
--- a/src/bin/pg_verifybackup/pg_verifybackup.c
+++ b/src/bin/pg_verifybackup/pg_verifybackup.c
@@ -484,6 +484,9 @@ parse_manifest_file(char *manifest_path)
inc_state, buffer, rc, bytes_left == 0);
}
+ /* Release the incremental state memory */
+ json_parse_manifest_incremental_shutdown(inc_state);
+
close(fd);
}
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index 44dbb7f7f9..9dfbc397c0 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -488,19 +488,18 @@ freeJsonLexContext(JsonLexContext *lex)
if (lex->errormsg)
destroyStringInfo(lex->errormsg);
+ if (lex->incremental)
+ {
+ pfree(lex->inc_state->partial_token.data);
+ pfree(lex->inc_state);
+ pfree(lex->pstack->prediction);
+ pfree(lex->pstack->fnames);
+ pfree(lex->pstack->fnull);
+ pfree(lex->pstack);
+ }
+
if (lex->flags & JSONLEX_FREE_STRUCT)
- {
- if (lex->incremental)
- {
- pfree(lex->inc_state->partial_token.data);
- pfree(lex->inc_state);
- pfree(lex->pstack->prediction);
- pfree(lex->pstack->fnames);
- pfree(lex->pstack->fnull);
- pfree(lex->pstack);
- }
pfree(lex);
- }
}
/*
diff --git a/src/common/parse_manifest.c b/src/common/parse_manifest.c
index 970a756ce8..a94e3d6b15 100644
--- a/src/common/parse_manifest.c
+++ b/src/common/parse_manifest.c
@@ -123,7 +123,6 @@ static bool parse_xlogrecptr(XLogRecPtr *result, char *input);
/*
* Set up for incremental parsing of the manifest.
- *
*/
JsonManifestParseIncrementalState *
@@ -163,6 +162,18 @@ json_parse_manifest_incremental_init(JsonManifestParseContext *context)
return incstate;
}
+/*
+ * Free an incremental state object and its contents.
+ */
+void
+json_parse_manifest_incrementa
Re: WIP Incremental JSON Parser
On 2024-04-08 Mo 14:24, Jacob Champion wrote: Michael pointed out over at [1] that the new tiny.json is pretty inscrutable given its size, and I have to agree. Attached is a patch to pare it down 98% or so. I think people wanting to run the performance comparisons will need to come up with their own gigantic files. Let's see if we can do a bit better than that. Maybe a script to construct a larger input for the speed test from the smaller file. Should be pretty simple. Michael, with your "Jacob might be a nefarious cabal of state-sponsored hackers" hat on, is this observable enough, or do we need to get it smaller? I was thinking we may want to replace the URLs with stuff that doesn't link randomly around the Internet. Delicious in its original form is long gone. Arguably the fact that it points nowhere is a good thing. But feel free to replace it with something else. It doesn't have to be URLs at all. That happened simply because it was easy to extract from a very large piece of JSON I had lying around, probably from the last time I wrote a JSON parser :-) cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-04-07 Su 20:58, Tom Lane wrote: Coverity complained that this patch leaks memory: /srv/coverity/git/pgsql-git/postgresql/src/bin/pg_combinebackup/load_manifest.c: 212 in load_backup_manifest() 206 bytes_left -= rc; 207 json_parse_manifest_incremental_chunk( 208 inc_state, buffer, rc, bytes_left == 0); 209 } 210 211 close(fd); CID 1596259:(RESOURCE_LEAK) Variable "inc_state" going out of scope leaks the storage it points to. 212 } 213 214 /* All done. */ 215 pfree(buffer); 216 return result; 217 } /srv/coverity/git/pgsql-git/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c: 488 in parse_manifest_file() 482 bytes_left -= rc; 483 json_parse_manifest_incremental_chunk( 484 inc_state, buffer, rc, bytes_left == 0); 485 } 486 487 close(fd); CID 1596257:(RESOURCE_LEAK) Variable "inc_state" going out of scope leaks the storage it points to. 488 } 489 490 /* Done with the buffer. */ 491 pfree(buffer); 492 493 return result; It's right about that AFAICS, and not only is the "inc_state" itself leaked but so is its assorted infrastructure. Perhaps we don't care too much about that in the existing applications, but ISTM that isn't going to be a tenable assumption across the board. Shouldn't there be a "json_parse_manifest_incremental_shutdown()" or the like to deallocate all the storage allocated by the parser? yeah, probably. Will work on it. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Cluster::restart dumping logs when stop fails
On 2024-04-06 Sa 20:49, Andres Freund wrote:
That's probably unnecessary optimization, but it seems a tad silly to read an
entire, potentially sizable, file to just use the last 1k. Not sure if the way
slurp_file() uses seek supports negative ofsets, the docs read to me like that
may only be supported with SEEK_END.
We should enhance slurp_file() so it uses SEEK_END if the offset is
negative. It would be a trivial patch:
diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm
b/src/test/perl/PostgreSQL/Test/Utils.pm
index 42d5a50dc8..8256573957 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -524,7 +524,7 @@ sub slurp_file
if (defined($offset))
{
- seek($fh, $offset, SEEK_SET)
+ seek($fh, $offset, ($offset < 0 ? SEEK_END : SEEK_SET))
or croak "could not seek \"$filename\": $!";
}
cheers
andrew
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
Re: meson vs windows perl
On 2024-04-05 Fr 10:12, Andrew Dunstan wrote:
On 2024-04-05 Fr 08:25, Andrew Dunstan wrote:
Here is an attempt to fix all that. It's ugly, but I think it's more
principled.
First, instead of getting the ldopts and then trying to filter out
the ldflags and ccdlflags, it tells perl not to include those in the
first place, by overriding a couple of routines in ExtUtils::Embed.
And second, it's smarter about splitting what's left, so that it
doesn't split on a space that's in a quoted item. The perl that's
used to do that second bit is not pretty, but it has been tested on
the system where the problem arose and apparently cures the problem.
(No doubt some perl guru could improve it.) It also works on my
Ubuntu system, so I don't think we'll be breaking anything (famous
last words).
Apparently I spoke too soon. Please ignore the above for now.
OK, this has been fixed and checked. The attached is what I propose.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/meson.build b/meson.build
index 87437960bc..be87ef6950 100644
--- a/meson.build
+++ b/meson.build
@@ -993,21 +993,15 @@ if not perlopt.disabled()
# Config's ccdlflags and ldflags. (Those are the choices of those who
# built the Perl installation, which are not necessarily appropriate
# for building PostgreSQL.)
-ldopts = run_command(perl, '-MExtUtils::Embed', '-e', 'ldopts', check: true).stdout().strip()
-undesired = run_command(perl_conf_cmd, 'ccdlflags', check: true).stdout().split()
-undesired += run_command(perl_conf_cmd, 'ldflags', check: true).stdout().split()
-
-perl_ldopts = []
-foreach ldopt : ldopts.split(' ')
- if ldopt == '' or ldopt in undesired
-continue
- endif
-
- perl_ldopts += ldopt.strip('"')
-endforeach
-
+ldopts = run_command(perl, '-MExtUtils::Embed', '-e', '*ExtUtils::Embed::_ldflags = *Extutils::Embed::_ccdlflags = sub { return q[]; }; ldopts',
+ check: true).stdout().strip()
+# avoid use of " char in this perl mini-program to avoid possibly
+# confusing the Windows command processor
+perl_ldopts = run_command(perl, '-MEnglish', '-e',
+ 'my $arg = shift; while ($arg =~ /\S/) { if ($arg =~ /^\s*([^\042 ]+)(?![\042])/) { print qq[$1\n]; $arg = $POSTMATCH;} elsif ($arg =~ /^\s*\042([^\042]+)\042/) { print qq[$1\n]; $arg = $POSTMATCH;} }',
+ '--', ldopts, check: true).stdout().strip().split('\n')
message('LDFLAGS recommended by perl: "@0@"'.format(ldopts))
-message('LDFLAGS for embedding perl: "@0@"'.format(' '.join(perl_ldopts)))
+message('LDFLAGS for embedding perl:\n"@0@"'.format('\n'.join(perl_ldopts)))
perl_dep_int = declare_dependency(
compile_args: perl_ccflags,
Re: WIP Incremental JSON Parser
On 2024-04-05 Fr 11:43, Nathan Bossart wrote: On Fri, Apr 05, 2024 at 10:15:45AM -0400, Andrew Dunstan wrote: On 2024-04-04 Th 15:54, Nathan Bossart wrote: On Thu, Apr 04, 2024 at 03:31:12PM -0400, Andrew Dunstan wrote: Does the attached patch fix it for you? It clears up 2 of the 3 warnings for me: ../postgresql/src/common/jsonapi.c: In function ‘IsValidJsonNumber’: ../postgresql/src/common/jsonapi.c:2018:30: error: ‘dummy_lex.inc_state’ may be used uninitialized in this function [-Werror=maybe-uninitialized] 2018 | if (lex->incremental && !lex->inc_state->is_last_chunk && | Thanks, please try this instead. LGTM, thanks! Thanks for checking. Pushed. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-04-04 Th 15:54, Nathan Bossart wrote:
On Thu, Apr 04, 2024 at 03:31:12PM -0400, Andrew Dunstan wrote:
Does the attached patch fix it for you?
It clears up 2 of the 3 warnings for me:
../postgresql/src/common/jsonapi.c: In function ‘IsValidJsonNumber’:
../postgresql/src/common/jsonapi.c:2018:30: error: ‘dummy_lex.inc_state’ may be
used uninitialized in this function [-Werror=maybe-uninitialized]
2018 | if (lex->incremental && !lex->inc_state->is_last_chunk &&
|
Thanks, please try this instead.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index 5f947dd618..44dbb7f7f9 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -279,6 +279,8 @@ IsValidJsonNumber(const char *str, int len)
return false;
dummy_lex.incremental = false;
+ dummy_lex.inc_state = NULL;
+ dummy_lex.pstack = NULL;
/*
* json_lex_number expects a leading '-' to have been eaten already.
@@ -297,6 +299,8 @@ IsValidJsonNumber(const char *str, int len)
dummy_lex.input_length = len;
}
+ dummy_lex.token_start = dummy_lex.input;
+
json_lex_number(_lex, dummy_lex.input, _error, _len);
return (!numeric_error) && (total_len == dummy_lex.input_length);
@@ -2018,6 +2022,9 @@ json_lex_number(JsonLexContext *lex, char *s,
{
appendBinaryStringInfo(>inc_state->partial_token,
lex->token_start, s - lex->token_start);
+ if (num_err != NULL)
+ *num_err = error;
+
return JSON_INCOMPLETE;
}
else if (num_err != NULL)
Re: IPC::Run::time[r|out] vs our TAP tests
On 2024-04-04 Th 17:24, Tom Lane wrote:
TIL that IPC::Run::timer is not the same as IPC::Run::timeout.
With a timer object you have to check $timer->is_expired to see
if the timeout has elapsed, but with a timeout object you don't
because it will throw a Perl exception upon timing out, probably
killing your test program.
It appears that a good chunk of our TAP codebase has not read this
memo, because I see plenty of places that are checking is_expired
in the naive belief that they'll still have control after a timeout
has fired.
I started having a look at these.
Here are the cases I found:
./src/bin/psql/t/010_tab_completion.pl: my $okay = ($out =~ $pattern
&& !$h->{timeout}->is_expired);
./src/test/perl/PostgreSQL/Test/BackgroundPsql.pm: until
$self->{stdout} =~ /$banner/ || $self->{timeout}->is_expired;
./src/test/perl/PostgreSQL/Test/BackgroundPsql.pm: die "psql startup
timed out" if $self->{timeout}->is_expired;
./src/test/perl/PostgreSQL/Test/BackgroundPsql.pm: die "psql query
timed out" if $self->{timeout}->is_expired;
./src/test/perl/PostgreSQL/Test/BackgroundPsql.pm: die "psql query
timed out" if $self->{timeout}->is_expired;
./src/test/perl/PostgreSQL/Test/Cluster.pm: # timeout, which
we'll handle by testing is_expired
./src/test/perl/PostgreSQL/Test/Cluster.pm: unless
$timeout->is_expired;
./src/test/perl/PostgreSQL/Test/Cluster.pm:timeout is the
IPC::Run::Timeout object whose is_expired method can be tested
./src/test/perl/PostgreSQL/Test/Cluster.pm: # timeout, which
we'll handle by testing is_expired
./src/test/perl/PostgreSQL/Test/Cluster.pm: unless
$timeout->is_expired;
./src/test/perl/PostgreSQL/Test/Utils.pm: if ($timeout->is_expired)
./src/test/recovery/t/021_row_visibility.pl: if
($psql_timeout->is_expired)
./src/test/recovery/t/032_relfilenode_reuse.pl: if
($psql_timeout->is_expired)
Those in Cluster.pm look correct - they are doing the run() in an eval
block and testing for the is_expired setting in an exception block. The
other cases look more suspect. I'll take a closer look.
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
Re: meson vs windows perl
On 2024-04-05 Fr 08:25, Andrew Dunstan wrote: Here is an attempt to fix all that. It's ugly, but I think it's more principled. First, instead of getting the ldopts and then trying to filter out the ldflags and ccdlflags, it tells perl not to include those in the first place, by overriding a couple of routines in ExtUtils::Embed. And second, it's smarter about splitting what's left, so that it doesn't split on a space that's in a quoted item. The perl that's used to do that second bit is not pretty, but it has been tested on the system where the problem arose and apparently cures the problem. (No doubt some perl guru could improve it.) It also works on my Ubuntu system, so I don't think we'll be breaking anything (famous last words). Apparently I spoke too soon. Please ignore the above for now. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: meson vs windows perl
On 2024-04-02 Tu 09:34, Andrew Dunstan wrote:
meson.build has this code
ldopts = run_command(perl, '-MExtUtils::Embed', '-e',
'ldopts', check: true).stdout().strip() undesired =
run_command(perl_conf_cmd, 'ccdlflags', check:
true).stdout().split() undesired += run_command(perl_conf_cmd,
'ldflags', check: true).stdout().split() perl_ldopts = []
foreach ldopt : ldopts.split(' ') if ldopt == '' or ldopt in
undesired continue endif perl_ldopts +=
ldopt.strip('"') endforeach message('LDFLAGS recommended
by perl: "@0@"'.format(ldopts)) message('LDFLAGS for embedding
perl: "@0@"'.format(' '.join(perl_ldopts)))
This code is seriously broken if perl reports items including spaces,
when a) removing the quotes is quite wrong, and b) splitting on spaces
is also wrong.
Here's an example from one of my colleagues:
C:\Program Files\Microsoft Visual Studio\2022\Professional>perl.EXE
-MExtUtils::Embed -e ldopts
-nologo -nodefaultlib -debug -opt:ref,icf -ltcg
-libpath:"C:\edb\languagepack\v4\Perl-5.38\lib\CORE"
-machine:AMD64 -subsystem:console,"5.02"
"C:\edb\languagepack\v4\Perl-5.38\lib\CORE\perl538.lib"
"C:\Program Files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.38.33130\lib\x64\oldnames.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\kernel32.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\user32.lib"
"C:\Program Files (x86)\Windows Kits\10\\lib\10.0.22621.0\\um\x64\gdi32.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\winspool.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\comdlg32.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\advapi32.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\shell32.lib"
"C:\Program Files (x86)\Windows Kits\10\\lib\10.0.22621.0\\um\x64\ole32.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\oleaut32.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\netapi32.lib"
"C:\Program Files (x86)\Windows Kits\10\\lib\10.0.22621.0\\um\x64\uuid.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\ws2_32.lib"
"C:\Program Files (x86)\Windows Kits\10\\lib\10.0.22621.0\\um\x64\mpr.lib"
"C:\Program Files (x86)\Windows Kits\10\\lib\10.0.22621.0\\um\x64\winmm.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\version.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\odbc32.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\odbccp32.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\comctl32.lib"
"C:\Program Files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.38.33130\lib\x64\msvcrt.lib"
"C:\Program Files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.38.33130\lib\x64\vcruntime.lib"
"C:\Program Files (x86)\Windows Kits\10\lib\10.0.22621.0\ucrt\x64\ucrt.lib"
And with that we get errors like
cl : Command line warning D9024 : unrecognized source file type
'C:\Program', object file assumed
cl : Command line warning D9024 : unrecognized source file type
'Files\Microsoft', object file assumed
cl : Command line warning D9024 : unrecognized source file type 'Visual',
object file assumed
cl : Command line warning D9024 : unrecognized source file type
'C:\Program', object file assumed
cl : Command line warning D9024 : unrecognized source file type 'Files',
object file assumed
cl : Command line warning D9024 : unrecognized source file type
'(x86)\Windows', object file assumed
It looks like we need to get smarter about how we process the ldopts and strip
out the ccdlflags and ldflags
Here is an attempt to fix all that. It's ugly, but I think it's more
principled.
First, instead of getting the ldopts and then trying to filter out the
ldflags and ccdlflags, it tells perl not to include those in the first
place, by overriding a couple of routines in ExtUtils::Embed. And
second, it's smarter about splitting what's left, so that it doesn't
split on a space that's in a quoted item. The perl that's used to do
that second bit is not pretty, but it has been tested on the system
where the problem arose and apparently cures the problem. (No doubt some
perl guru could improve it.) It also works on my Ubuntu system, so I
don't think we'll be breaking anything (famous last words).
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
diff --git a/meson
Re: WIP Incremental JSON Parser
On 2024-04-04 Th 14:06, Nathan Bossart wrote:
Apologies for piling on, but my compiler (gcc 9.4.0) is unhappy:
../postgresql/src/common/jsonapi.c: In function ‘IsValidJsonNumber’:
../postgresql/src/common/jsonapi.c:2016:30: error: ‘dummy_lex.inc_state’ may be
used uninitialized in this function [-Werror=maybe-uninitialized]
2016 | if (lex->incremental && !lex->inc_state->is_last_chunk &&
| ~~~^~~
../postgresql/src/common/jsonapi.c:2020:36: error: ‘dummy_lex.token_start’ may
be used uninitialized in this function [-Werror=maybe-uninitialized]
2020 | lex->token_start, s - lex->token_start);
| ~~~^
../postgresql/src/common/jsonapi.c:302:26: error: ‘numeric_error’ may be used
uninitialized in this function [-Werror=maybe-uninitialized]
302 | return (!numeric_error) && (total_len == dummy_lex.input_length);
| ~^~~~
Please pile on. I want to get this fixed.
It seems odd that my much later gcc didn't complain.
Does the attached patch fix it for you?
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index 5f947dd618..b003656ff8 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -297,6 +297,8 @@ IsValidJsonNumber(const char *str, int len)
dummy_lex.input_length = len;
}
+ dummy_lex.token_start = dummy_lex.input;
+
json_lex_number(_lex, dummy_lex.input, _error, _len);
return (!numeric_error) && (total_len == dummy_lex.input_length);
@@ -2018,6 +2020,9 @@ json_lex_number(JsonLexContext *lex, char *s,
{
appendBinaryStringInfo(>inc_state->partial_token,
lex->token_start, s - lex->token_start);
+ if (num_err != NULL)
+ *num_err = false;
+
return JSON_INCOMPLETE;
}
else if (num_err != NULL)
Re: WIP Incremental JSON Parser
On 2024-04-04 Th 12:04, Tom Lane wrote: Oh, more problems: after running check-world in a non-VPATH build, there are droppings all over my tree: $ git status On branch master Your branch is up to date with 'origin/master'. Untracked files: (use "git add ..." to include in what will be committed) src/interfaces/ecpg/test/sql/sqljson_jsontable src/interfaces/ecpg/test/sql/sqljson_jsontable.c src/test/modules/test_json_parser/test_json_parser_incremental src/test/modules/test_json_parser/test_json_parser_perf src/test/modules/test_json_parser/tmp_check/ nothing added to commit but untracked files present (use "git add" to track) "make clean" or "make distclean" removes some of that but not all: $ make -s distclean $ git status On branch master Your branch is up to date with 'origin/master'. Untracked files: (use "git add ..." to include in what will be committed) src/test/modules/test_json_parser/test_json_parser_incremental src/test/modules/test_json_parser/test_json_parser_perf nothing added to commit but untracked files present (use "git add" to track) So we're short several .gitignore entries, and apparently also shy a couple of make-clean rules. Argh. You get out of the habit when you're running with meson :-( There's another issue I'm running down too. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-04-04 Th 11:16, Tom Lane wrote: I wrote: I think you just need to follow the standard pattern: Yeah, the attached is enough to silence it for me. Thanks, that's what I came up with too (after converting my setup to use clang) (But personally I'd add comments saying that the typedef appears in thus-and-such header file; see examples in our tree.) Done cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-04-04 Th 10:26, Tom Lane wrote: Andrew Dunstan writes: pushed. My animals don't like this [1][2]: parse_manifest.c:99:3: error: redefinition of typedef 'JsonManifestParseIncrementalState' is a C11 feature [-Werror,-Wtypedef-redefinition] } JsonManifestParseIncrementalState; ^ ../../src/include/common/parse_manifest.h:23:50: note: previous definition is here typedef struct JsonManifestParseIncrementalState JsonManifestParseIncrementalState; ^ 1 error generated. (and similarly in other places) regards, tom lane [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=longfin=2024-04-04%2013%3A08%3A02 [2] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sifaka=2024-04-04%2013%3A07%3A01 Darn it. Ok, will try to fix. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-04-02 Tu 17:12, Andrew Dunstan wrote: On 2024-04-02 Tu 15:38, Jacob Champion wrote: On Mon, Apr 1, 2024 at 4:53 PM Andrew Dunstan wrote: Anyway, here are new patches. I've rolled the new semantic test into the first patch. Looks good! I've marked RfC. Thanks! I appreciate all the work you've done on this. I will give it one more pass and commit RSN. pushed. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Cutting support for OpenSSL 1.0.1 and 1.0.2 in 17~?
On 2024-04-03 We 15:12, Daniel Gustafsson wrote: The fact that very few animals run the ssl tests is a pet peeve of mine, it would be nice if we could get broader coverage there. Well, the only reason for that is that the SSL tests need to be listed in PG_TEST_EXTRA, and the only reason for that is that there's a possible hazard on multi-user servers. But I bet precious few buildfarm animals run in such an environment. Mine don't - I'm the only user. Maybe we could send out an email to the buildfarm-owners list asking people to consider allowing the ssl tests. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: Extension for PostgreSQL cast jsonb to hstore WIP
On 2024-04-02 Tu 11:43, ShadowGhost wrote:
At the moment, this cast supports only these structures, as it was
enough for my tasks:
{str:numeric}
{str:str}
{str:bool}
{str:null}
But it's a great idea and I'll think about implementing it.
Please don't top-post on the PostgreSQL lists. See
<https://wiki.postgresql.org/wiki/Mailing_Lists#Email_etiquette_mechanics>
I don't think a cast that doesn't cater for all the forms json can take
is going to work very well. At the very least you would need to error
out in cases you didn't want to cover, and have tests for all of those
errors. But the above is only a tiny fraction of those. If the error
cases are going to be so much more than the cases that work it seems a
bit pointless.
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-04-02 Tu 15:38, Jacob Champion wrote: On Mon, Apr 1, 2024 at 4:53 PM Andrew Dunstan wrote: Anyway, here are new patches. I've rolled the new semantic test into the first patch. Looks good! I've marked RfC. Thanks! I appreciate all the work you've done on this. I will give it one more pass and commit RSN. json_lex() is not really a very hot piece of code. Sure, but I figure if someone is trying to get the performance of the incremental parser to match the recursive one, so we can eventually replace it, it might get a little warmer. :) I don't think this is where the performance penalty lies. Rather, I suspect it's the stack operations in the non-recursive parser itself. The speed test doesn't involve any partial token processing at all, and yet the non-recursive parser is significantly slower in that test. I think it'd be good for a v1.x of this feature to focus on simplification of the code, and hopefully consolidate and/or eliminate some of the duplicated parsing work so that the mental model isn't quite so big. I'm not sure how you think that can be done. I think we'd need to teach the lower levels of the lexer about incremental parsing too, so that we don't have two separate sources of truth about what ends a token. Bonus points if we could keep the parse state across chunks to the extent that we didn't need to restart at the beginning of the token every time. (Our current tools for this are kind of poor, like the restartable state machine in PQconnectPoll. While I'm dreaming, I'd like coroutines.) Now, whether the end result would be more or less maintainable is left as an exercise... I tried to disturb the main lexer processing as little as possible. We could possibly unify the two paths, but I have a strong suspicion that would result in a performance hit (the main part of the lexer doesn't copy any characters at all, it just keeps track of pointers into the input). And while the code might not undergo lots of change, the routine itself is quite performance critical. Anyway, I think that's all something for another day. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
meson vs windows perl
meson.build has this code
ldopts = run_command(perl, '-MExtUtils::Embed', '-e', 'ldopts',
check: true).stdout().strip() undesired =
run_command(perl_conf_cmd, 'ccdlflags', check:
true).stdout().split() undesired += run_command(perl_conf_cmd,
'ldflags', check: true).stdout().split() perl_ldopts = []
foreach ldopt : ldopts.split(' ') if ldopt == '' or ldopt in
undesired continue endif perl_ldopts +=
ldopt.strip('"') endforeach message('LDFLAGS recommended by
perl: "@0@"'.format(ldopts)) message('LDFLAGS for embedding
perl: "@0@"'.format(' '.join(perl_ldopts)))
This code is seriously broken if perl reports items including spaces,
when a) removing the quotes is quite wrong, and b) splitting on spaces
is also wrong.
Here's an example from one of my colleagues:
C:\Program Files\Microsoft Visual Studio\2022\Professional>perl.EXE
-MExtUtils::Embed -e ldopts
-nologo -nodefaultlib -debug -opt:ref,icf -ltcg
-libpath:"C:\edb\languagepack\v4\Perl-5.38\lib\CORE"
-machine:AMD64 -subsystem:console,"5.02"
"C:\edb\languagepack\v4\Perl-5.38\lib\CORE\perl538.lib"
"C:\Program Files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.38.33130\lib\x64\oldnames.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\kernel32.lib"
"C:\Program Files (x86)\Windows Kits\10\\lib\10.0.22621.0\\um\x64\user32.lib"
"C:\Program Files (x86)\Windows Kits\10\\lib\10.0.22621.0\\um\x64\gdi32.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\winspool.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\comdlg32.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\advapi32.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\shell32.lib"
"C:\Program Files (x86)\Windows Kits\10\\lib\10.0.22621.0\\um\x64\ole32.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\oleaut32.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\netapi32.lib"
"C:\Program Files (x86)\Windows Kits\10\\lib\10.0.22621.0\\um\x64\uuid.lib"
"C:\Program Files (x86)\Windows Kits\10\\lib\10.0.22621.0\\um\x64\ws2_32.lib"
"C:\Program Files (x86)\Windows Kits\10\\lib\10.0.22621.0\\um\x64\mpr.lib"
"C:\Program Files (x86)\Windows Kits\10\\lib\10.0.22621.0\\um\x64\winmm.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\version.lib"
"C:\Program Files (x86)\Windows Kits\10\\lib\10.0.22621.0\\um\x64\odbc32.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\odbccp32.lib"
"C:\Program Files (x86)\Windows
Kits\10\\lib\10.0.22621.0\\um\x64\comctl32.lib"
"C:\Program Files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.38.33130\lib\x64\msvcrt.lib"
"C:\Program Files\Microsoft Visual
Studio\2022\Professional\VC\Tools\MSVC\14.38.33130\lib\x64\vcruntime.lib"
"C:\Program Files (x86)\Windows Kits\10\lib\10.0.22621.0\ucrt\x64\ucrt.lib"
And with that we get errors like
cl : Command line warning D9024 : unrecognized source file type
'C:\Program', object file assumed
cl : Command line warning D9024 : unrecognized source file type
'Files\Microsoft', object file assumed
cl : Command line warning D9024 : unrecognized source file type 'Visual',
object file assumed
cl : Command line warning D9024 : unrecognized source file type
'C:\Program', object file assumed
cl : Command line warning D9024 : unrecognized source file type 'Files',
object file assumed
cl : Command line warning D9024 : unrecognized source file type
'(x86)\Windows', object file assumed
It looks like we need to get smarter about how we process the ldopts and strip
out the ccdlflags and ldflags
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
Re: Extension for PostgreSQL cast jsonb to hstore WIP
On 2024-04-02 Tu 07:07, ShadowGhost wrote: Hello all. Recently, when working with the hstore and json formats, I came across the fact that PostgreSQL has a cast of hstore to json, but there is no reverse cast. I thought it might make it more difficult to work with these formats. And I decided to make a cast json in the hstore. I used the built-in jsonb structure to create it and may have introduced methods to increase efficiency by 25% than converting the form jsonb->text->hstore. Which of course is a good fact. I also wrote regression tests to check the performance. I think this extension will improve the work with jsonb and hstore in PostgreSQL. If you've read this far, thank you for your interest, and I hope you enjoy this extension! One reason we don't have such a cast is that hstore has a flat structure, while json is tree structured, and it's not always an object / hash. Thus it's easy to reliably cast hstore to json but far less easy to cast json to hstore in the general case. What do you propose to do in the case or json consisting of scalars, or arrays, or with nested elements? cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Security lessons from liblzma
On 2024-03-31 Su 17:12, Andres Freund wrote: Hi, On 2024-03-31 12:18:29 +0200, Michael Banck wrote: If you ask where they are maintained, the answer is here: https://salsa.debian.org/postgresql/postgresql/-/tree/17/debian/patches?ref_type=heads the other major versions have their own branch. Luckily these are all quite small, leaving little space to hide stuff. I'd still like to get rid of at least some of them. I've previously proposed a patch to make pkglibdir configurable, I think we should just go for that. For the various defines, ISTM we should just make them #ifndef guarded, then they could be overridden by defining them at configure time. Some of them, like DEFAULT_PGSOCKET_DIR seem to be overridden by just about every distro. And others would be nice to easily override anyway, I e.g. dislike the default DEFAULT_PAGER value. +1 to this proposal. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Broken error detection in genbki.pl
On 2024-03-20 We 12:44, Tom Lane wrote:
David Wheeler complained over in [1] that genbki.pl fails to produce a
useful error message if there's anything wrong in a catalog data file.
He's right about that, but the band-aid patch he proposed doesn't
improve the situation much. The actual problem is that key error
messages in genbki.pl expect $bki_values->{line_number} to be set,
which it is not because we're invoking Catalog::ParseData with
$preserve_formatting = 0, and that runs a fast path that doesn't do
line-by-line processing and hence doesn't/can't fill that field.
I'm quite sure that those error cases worked as-intended when first
coded. I did not check the git history, but I suppose that somebody
added the non-preserve_formatting fast path later without any
consideration for the damage it was doing to error reporting ability.
I'm of the opinion that this change was penny-wise and pound-foolish.
On my machine, the time to re-make the bki files with the fast path
enabled is 0.597s, and with it disabled (measured with the attached
quick-hack patch) it's 0.612s. Is that savings worth future hackers
having to guess what they broke and where in a large .dat file?
As you can see from the quick-hack patch, it's kind of a mess to use
the $preserve_formatting = 1 case, because there are a lot of loops
that have to be taught to ignore comment lines, which we don't really
care about except in reformat_dat_file.pl. What I suggest doing, but
have not yet coded up, is to nuke the fast path in Catalog::ParseData
and reinterpret the $preserve_formatting argument as controlling
whether comments and whitespace are entered in the output data
structure, but not whether we parse it line-by-line. That should fix
this problem with zero change in the callers, and also buy back a
little bit of the cost compared to this quick hack.
Thoughts?
regards, tom lane
[1]
https://www.postgresql.org/message-id/60EF4E11-BC1C-4034-B37B-695662D28AD2%40justatheory.com
Makes sense
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-03-26 Tu 17:52, Jacob Champion wrote:
On Mon, Mar 25, 2024 at 3:14 PM Jacob Champion
wrote:
- add Assert calls in impossible error cases [2]
To expand on this one, I think these parts of the code (marked with
`<- here`) are impossible to reach:
switch (top)
{
case JSON_TOKEN_STRING:
if (next_prediction(pstack) == JSON_TOKEN_COLON)
ctx = JSON_PARSE_STRING;
else
ctx = JSON_PARSE_VALUE;<- here
break;
case JSON_TOKEN_NUMBER:<- here
case JSON_TOKEN_TRUE: <- here
case JSON_TOKEN_FALSE: <- here
case JSON_TOKEN_NULL: <- here
case JSON_TOKEN_ARRAY_START: <- here
case JSON_TOKEN_OBJECT_START: <- here
ctx = JSON_PARSE_VALUE;
break;
case JSON_TOKEN_ARRAY_END: <- here
ctx = JSON_PARSE_ARRAY_NEXT;
break;
case JSON_TOKEN_OBJECT_END:<- here
ctx = JSON_PARSE_OBJECT_NEXT;
break;
case JSON_TOKEN_COMMA: <- here
if (next_prediction(pstack) == JSON_TOKEN_STRING)
ctx = JSON_PARSE_OBJECT_NEXT;
else
ctx = JSON_PARSE_ARRAY_NEXT;
break;
Since none of these cases are non-terminals, the only way to get to
this part of the code is if (top != tok). But inspecting the
productions and transitions that can put these tokens on the stack, it
looks like the only way for them to be at the top of the stack to
begin with is if (tok == top). (Otherwise, we would have chosen a
different production, or else errored out on a non-terminal.)
This case is possible...
case JSON_TOKEN_STRING:
if (next_prediction(pstack) == JSON_TOKEN_COLON)
ctx = JSON_PARSE_STRING;
...if we're in the middle of JSON_PROD_[MORE_]KEY_PAIRS. But the
corresponding else case is not, because if we're in the middle of a
_KEY_PAIRS production, the next_prediction() _must_ be
JSON_TOKEN_COLON.
Do you agree, or am I missing a way to get there?
One good way of testing would be to add the Asserts, build with
-DFORCE_JSON_PSTACK, and run the standard regression suite, which has a
fairly comprehensive set of JSON errors. I'll play with that.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: Possibility to disable `ALTER SYSTEM`
> On Mar 27, 2024, at 3:53 AM, Tom Lane wrote: > > Bruce Momjian writes: >> I am thinking "enable_alter_system_command" is probably good because we >> already use "enable" so why not reuse that idea, and I think "command" >> is needed because we need to clarify we are talking about the command, >> and not generic altering of the system. We could use >> "enable_sql_alter_system" if people want something shorter. > > Robert already mentioned why not use "enable_": up to now that prefix > has only been applied to planner plan-type-enabling GUCs. I'd be okay > with "allow_alter_system_command", although I find it unnecessarily > verbose. Agree. I don’t think “_command” adds much clarity. Cheers Andrew
Re: WIP Incremental JSON Parser
On Mon, Mar 25, 2024 at 7:12 PM Jacob Champion < jacob.champ...@enterprisedb.com> wrote: > On Mon, Mar 25, 2024 at 4:02 PM Andrew Dunstan > wrote: > > Well, what's the alternative? The current parser doesn't check stack > depth in frontend code. Presumably it too will eventually just run out of > memory, possibly rather sooner as the stack frames could be more expensive > than the incremental parser stack extensions. > > Stack size should be pretty limited, at least on the platforms I'm > familiar with. So yeah, the recursive descent will segfault pretty > quickly, but it won't repalloc() an unbounded amount of heap space. > The alternative would just be to go back to a hardcoded limit in the > short term, I think. > > > OK, so we invent a new error code and have the parser return that if the stack depth gets too big? cheers andrew
Re: session username in default psql prompt?
On Mon, Mar 25, 2024 at 9:14 AM Jelte Fennema-Nio wrote: > On Mon, 25 Mar 2024 at 14:06, Robert Haas wrote: > > On Mon, Mar 25, 2024 at 4:30 AM Jelte Fennema-Nio > wrote: > > > That problem seems easy to address by adding a newline into the > > > default prompt. > > > > Ugh. Please, no! > > I guess it's partially a matter of taste, but personally I'm never > going back to a single line prompt. It's so nice for zoomed-in demos > that your SQL queries don't get broken up. > Very much a matter of taste. I knew when I saw your suggestion there would be some kickback. If horizontal space is at a premium vertical space is doubly so, I suspect. cheers andrew
Re: WIP Incremental JSON Parser
On Mon, Mar 25, 2024 at 6:15 PM Jacob Champion <
jacob.champ...@enterprisedb.com> wrote:
> On Wed, Mar 20, 2024 at 11:56 PM Andrew Dunstan
> wrote:
> > Thanks, included that and attended to the other issues we discussed. I
> think this is pretty close now.
>
> Okay, looking over the thread, there are the following open items:
> - extend the incremental test in order to exercise the semantic callbacks
> [1]
>
Yeah, I'm on a super long plane trip later this week, so I might get it
done then :-)
> - add Assert calls in impossible error cases [2]
>
ok, will do
> - error out if the non-incremental lex doesn't consume the entire token [2]
>
ok, will do
> - double-check that out of memory is an appropriate failure mode for
> the frontend [3]
>
Well, what's the alternative? The current parser doesn't check stack depth
in frontend code. Presumably it too will eventually just run out of memory,
possibly rather sooner as the stack frames could be more expensive than
the incremental parser stack extensions.
>
> Just as a general style nit:
>
> > + if (lex->incremental)
> > + {
> > + lex->input = lex->token_terminator = lex->line_start = json;
> > + lex->input_length = len;
> > + lex->inc_state->is_last_chunk = is_last;
> > + }
> > + else
> > + return JSON_INVALID_LEXER_TYPE;
>
> I think flipping this around would probably make it more readable;
> something like:
>
> if (!lex->incremental)
> return JSON_INVALID_LEXER_TYPE;
>
> lex->input = ...
>
>
>
Noted. will do, Thanks.
cheers
andrew
> [1]
> https://www.postgresql.org/message-id/CAOYmi%2BnHV55Uhz%2Bo-HKq0GNiWn2L5gMcuyRQEz_fqpGY%3DpFxKA%40mail.gmail.com
> [2]
> https://www.postgresql.org/message-id/CAD5tBcLi2ffZkktV2qrsKSBykE-N8CiYgrfbv0vZ-F7%3DxLFeqw%40mail.gmail.com
> [3]
> https://www.postgresql.org/message-id/CAOYmi%2BnY%3DrF6dJCzaOuA3d-3FbwXCcecOs_S1NutexFA3dRXAw%40mail.gmail.com
>
Re: pg_dump versus enum types, round N+1
On Sat, Mar 23, 2024 at 3:00 PM Tom Lane wrote:
> I have a patch in the queue [1] that among other things tries to
> reduce the number of XIDs consumed during pg_upgrade by making
> pg_restore group its commands into batches of a thousand or so
> per transaction. This had been passing tests, so I was quite
> surprised when the cfbot started to show it as falling over.
> Investigation showed that it is now failing because 6185c9737
> added these objects to the regression tests and didn't drop them:
>
> CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green', 'blue',
> 'purple');
> CREATE DOMAIN rgb AS rainbow CHECK (VALUE IN ('red', 'green', 'blue'));
>
> In binary-upgrade mode, pg_dump dumps the enum type like this:
>
> CREATE TYPE public.rainbow AS ENUM (
> );
> ALTER TYPE public.rainbow ADD VALUE 'red';
> ALTER TYPE public.rainbow ADD VALUE 'orange';
> ALTER TYPE public.rainbow ADD VALUE 'yellow';
> ALTER TYPE public.rainbow ADD VALUE 'green';
> ALTER TYPE public.rainbow ADD VALUE 'blue';
> ALTER TYPE public.rainbow ADD VALUE 'purple';
>
> and then, if we're within the same transaction, creation of the domain
> falls over with
>
> ERROR: unsafe use of new value "red" of enum type rainbow
> HINT: New enum values must be committed before they can be used.
>
> So I'm glad we found that sooner not later, but something needs
> to be done about it if [1] is to get committed. It doesn't seem
> particularly hard to fix though: we just have to track the enum
> type OIDs made in the current transaction, using largely the same
> approach as is already used in pg_enum.c to track enum value OIDs.
> enum.sql contains a comment opining that this is too expensive,
> but I don't see why it is as long as we don't bother to track
> commands that are nested within subtransactions. That would be a bit
> complicated to do correctly, but pg_dump/pg_restore doesn't need it.
>
> Hence, I propose the attached.
>
>
>
Makes sense, Nice clear comments.
cheers
andrew
Re: session username in default psql prompt?
On Fri, Mar 22, 2024 at 7:34 PM Tom Lane wrote: > > On the whole, I think we'd get more complaints about the default > prompt having more-or-less doubled in length than we'd get kudos > about this being a great usability improvement. Prompt space is > expensive and precious, at least for people who aren't in the > habit of working in very wide windows. > > > I'm not sure you're right, but in view of the opposition I won't press it. Thanks to Kori for the patch. cheers andrew
Re: session username in default psql prompt?
On Fri, Mar 22, 2024 at 4:04 PM Robert Haas wrote: > On Wed, Mar 13, 2024 at 4:56 AM Andrew Dunstan > wrote: > > Reposting as the archive mail processor doesn't seem to like the Apple > > mail attachment. > > I'm not really a fan of this. Right now my prompt looks like this: > > robert.haas=# > > If we did this, it would say: > > robert.h...@robert.haas=# > Hmm. Perhaps we should change the default to "%n@%~%R%x%# " Then when connected to your eponymous database you would see robert.haas@~=# Of course, people can put this in their .psqlrc, and I do. The suggestion came about because I had a couple of instances where people using the default prompt showed me stuff and the problem arose from confusion about which user they were connected as. > I have yet to meet anyone who doesn't think that one Robert Haas is > quite enough already, and perhaps too much by half. > perish the thought. cheers andrew
Re: automating RangeTblEntry node support
On Mon, Mar 11, 2024 at 5:29 AM Peter Eisentraut wrote: > On 20.02.24 08:57, Peter Eisentraut wrote: > > On 18.02.24 00:06, Matthias van de Meent wrote: > >> I'm not sure that the cleanup which is done when changing a RTE's > >> rtekind is also complete enough for this purpose. > >> Things like inline_cte_walker change the node->rtekind, which could > >> leave residual junk data in fields that are currently dropped during > >> serialization (as the rtekind specifically ignores those fields), but > >> which would add overhead when the default omission is expected to > >> handle these fields; as they could then contain junk. It looks like > >> there is some care about zeroing now unused fields, but I haven't > >> checked that it covers all cases and fields to the extent required so > >> that removing this specialized serializer would have zero impact on > >> size once the default omission patch is committed. > >> > >> An additional patch with a single function that for this purpose > >> clears junk fields from RTEs that changed kind would be appreciated: > >> it is often hand-coded at those locations the kind changes, but that's > >> more sensitive to programmer error. > > > > Yes, interesting idea. Or maybe an assert-like function that checks an > > existing structure for consistency. Or maybe both. I'll try this out. > > > > In the meantime, if there are no remaining concerns, I propose to commit > > the first two patches > > > > Remove custom Constraint node read/write implementations > > Remove custom _jumbleRangeTblEntry() > > After a few side quests, here is an updated patch set. (I had committed > the first of the two patches mentioned above, but not yet the second one.) > > v3-0001-Remove-obsolete-comment.patch > v3-0002-Improve-comment.patch > > These just update a few comments around the RangeTblEntry definition. > > v3-0003-Reformat-some-node-comments.patch > v3-0004-Remove-custom-_jumbleRangeTblEntry.patch > > This is pretty much the same patch as before. I have now split it up to > first reformat the comments to make room for the node annotations. This > patch is now also pgindent-proof. After some side quest discussions, > the set of fields to jumble seems correct now, so commit message > comments to the contrary have been dropped. > > v3-0005-Make-RangeTblEntry-dump-order-consistent.patch > > I separated that from the 0008 patch below. I think it useful even if > we don't go ahead with 0008 now, for example in dumps from the debugger, > and just in general to keep everything more consistent. > > v3-0006-WIP-AssertRangeTblEntryIsValid.patch > > This is in response to some of the discussions where there was some > doubt whether all fields are always filled and cleared correctly when > the RTE kind is changed. Seems correct as far as this goes. I didn't > know of a good way to hook this in, so I put it into the write/read > functions, which is obviously a bit weird if I'm proposing to remove > them later. Consider it a proof of concept. > > v3-0007-Simplify-range_table_mutator_impl-and-range_table.patch > v3-0008-WIP-Remove-custom-RangeTblEntry-node-read-write-i.patch > > At this point, I'm not too stressed about pressing forward with these > last two patches. We can look at them again perhaps if we make progress > on a more compact node output format. When I started this thread, I had > a lot of questions about various details about the RangeTblEntry struct, > and we have achieved many answers during the discussions, so I'm happy > with the progress. So for PG17, I'd like to just do patches 0001..0005. > Patches 1 thru 5 look good to me cheers andrew
Re: Trying to build x86 version on windows using meson
On Wed, Mar 20, 2024 at 6:21 PM Andres Freund wrote: > Hi, > > On 2024-03-21 11:02:27 +1300, David Rowley wrote: > > On Thu, 21 Mar 2024 at 11:00, Andres Freund wrote: > > > > > > On 2024-03-20 17:49:14 -0400, Dave Cramer wrote: > > > > First off this is on an ARM64 machine > > > > > > Uh, that's a fairly crucial bit - you're actually trying to cross > compile > > > then. I don't know much about cross compiling on windows, so it's > certainly > > > possible there's still some gaps there. > > > > How would initdb.exe / pg_regress.exe even run on the x86 build > > machine if it's compiled for ARM? > > I think this is building on an ARM64 host, targeting 32bit x86. > > Obviously tests can't run in that environment, but building should be > possible. I can e.g. build postgres for x86-64 windows on my linux machine, > but can't run the tests (in theory they could be run with wine, but wine > isn't > complete enough to run postgres). > Windows apparently has some magic built in for this: https://learn.microsoft.com/en-us/windows/arm/apps-on-arm-x86-emulation cheers andrew
Re: WIP Incremental JSON Parser
On Tue, Mar 19, 2024 at 6:07 PM Andrew Dunstan wrote: > > > > > It also removes the frontend exits I had. In the case of stack depth, we > follow the example of the RD parser and only check stack depth for backend > code. In the case of the check that the lexer is set up for incremental > parsing, the exit is replaced by an Assert. > > On second thoughts, I think it might be better if we invent a new error return code for a lexer mode mismatch. cheers andrew
Re: Possibility to disable `ALTER SYSTEM`
On Tue, Mar 19, 2024 at 2:28 PM Magnus Hagander wrote: > On Tue, Mar 19, 2024 at 3:52 PM Tom Lane wrote: > > > > Heikki Linnakangas writes: > > > Perhaps we could make that even better with a GUC though. I propose a > > > GUC called 'configuration_managed_externally = true / false". If you > set > > > it to true, we prevent ALTER SYSTEM and make the error message more > > > definitive: > > > > > postgres=# ALTER SYSTEM SET wal_level TO minimal; > > > ERROR: configuration is managed externally > > > > > As a bonus, if that GUC is set, we could even check at server startup > > > that all the configuration files are not writable by the postgres user, > > > and print a warning or refuse to start up if they are. > > > > I like this idea. The "bonus" is not optional though, because > > setting the files' ownership/permissions is the only way to be > > sure that the prohibition is even a little bit bulletproof. > > > > One small issue: how do we make that work on Windows? Have recent > > versions grown anything that looks like real file permissions? > > Windows has had full ACL support since 1993. The easiest way to do > what you're doing here is to just set a DENY permission on the > postgres operating system user. > > > > Yeah. See < https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/icacls> for example. cheers andrew
Re: documentation structure
On Mon, Mar 18, 2024 at 10:12 AM Robert Haas wrote: > I was looking at the documentation index this morning[1], and I can't > help feeling like there are some parts of it that are over-emphasized > and some parts that are under-emphasized. I'm not sure what we can do > about this exactly, but I thought it worth writing an email and seeing > what other people think. > > The two sections of the documentation that seem really > under-emphasized to me are the GUC documentation and the SQL > reference. The GUC documentation is all buried under "20. Server > Configuration" and the SQL command reference is under "I. SQL > commands". For reasons that I don't understand, all chapters except > for those in "VI. Reference" are numbered, but the chapters in that > section have Roman numerals instead. > > I don't know what other people's experience is, but for me, wanting to > know what a command does or what a setting does is extremely common. > Therefore, I think these chapters are disproportionately important and > should be emphasized more. In the case of the GUC reference, one idea > I have is to split up "III. Server Administration". My proposal is > that we divide it into three sections. The first would be called "III. > Server Installation" and would cover chapters 16 (installation from > binaries) through 19 (server setup and operation). The second would be > called "IV. Server Configuration" -- so every section that's currently > a subsection of "server configuration" would become a top-level > chapter. The third division would be "V. Server Administration," and > would cover the current chapters 21-33. This is probably far from > perfect, but it seems like a relatively simple change and better than > what we have now. > > I don't know what to do about "I. SQL commands". It's obviously > impractical to promote that to a top-level section, because it's got a > zillion sub-pages which I don't think we want in the top-level > documentation index. But having it as one of several unnumbered > chapters interposed between 51 and 52 doesn't seem great either. > > The stuff that I think is over-emphasized is as follows: (a) chapters > 1-3, the tutorial; (b) chapters 4-6, which are essentially a > continuation of the tutorial, and not at all similar to chapters 8-11 > which are chalk-full of detailed technical information; (c) chapters > 43-46, one per procedural language; perhaps these could just be > demoted to sub-sections of chapter 42 on procedural languages; (d) > chapters 47 (server programming interface), 50 (replication progress > tracking), and 51 (archive modules), all of which are important to > document but none of which seem important enough to put them in the > top-level documentation index; and (e) large parts of section "VII. > Internals," which again contain tons of stuff of very marginal > interest. The first ~4 chapters of the internals section seem like > they might be mainstream enough to justify the level of prominence > that we give them, but the rest has got to be of interest to a tiny > minority of readers. > > I think it might be possible to consolidate the internals section by > grouping a bunch of existing entries together by category. Basically, > after the first few chapters, you've got stuff that is of interest to > C programmers writing core or extension code; and you've got > explainers on things like GEQO and index op-classes and support > functions which might be of interest even to non-programmers. I think > for example that we don't need separate top-level chapters on writing > procedural language handlers, FDWs, tablesample methods, custom scan > providers, table access methods, index access methods, and WAL > resource managers. Some or all of those could be grouped under a > single chapter, perhaps, e.g. Using PostgreSQL Extensibility > Interfaces. > > Thoughts? I realize that this topic is HIGHLY prone to ENDLESS > bikeshedding, and it's inevitable that not everybody is going to > agree. But I hope we can agree that it's completely silly that it's > vastly easier to find the documentation about the backup manifest > format than it is to find the documentation on CREATE TABLE or > shared_buffers, and if we can agree on that, then perhaps we can agree > on some way to make things better. > > > +many for improving the index. My own pet docs peeve is a purely editorial one: func.sgml is a 30k line beast, and I think there's a good case for splitting out at least the larger chunks of it. cheers andrew
Re: Possibility to disable `ALTER SYSTEM`
On Tue, Mar 19, 2024 at 5:26 AM Heikki Linnakangas wrote: > I want to remind everyone of this from Gabriele's first message that > started this thread: > > > At the moment, a possible workaround is that `ALTER SYSTEM` can be > blocked > > by making the postgresql.auto.conf read only, but the returned message is > > misleading and that’s certainly bad user experience (which is very > > important in a cloud native environment): > > > > > > ``` > > postgres=# ALTER SYSTEM SET wal_level TO minimal; > > ERROR: could not open file "postgresql.auto.conf": Permission denied > > ``` > > I think making the config file read-only is a fine solution. If you > don't want postgres to mess with the config files, forbid it with the > permission system. > > Problems with pg_rewind, pg_basebackup were mentioned with that > approach. I think if you want the config files to be managed outside > PostgreSQL, by kubernetes, patroni or whatever, it would be good for > them to be read-only to the postgres user anyway, even if we had a > mechanism to disable ALTER SYSTEM. So it would be good to fix the > problems with those tools anyway. > > The error message is not great, I agree with that. Can we improve it? > Maybe just add a HINT like this: > > postgres=# ALTER SYSTEM SET wal_level TO minimal; > ERROR: could not open file "postgresql.auto.conf" for writing: > Permission denied > HINT: Configuration might be managed outside PostgreSQL > > > Perhaps we could make that even better with a GUC though. I propose a > GUC called 'configuration_managed_externally = true / false". If you set > it to true, we prevent ALTER SYSTEM and make the error message more > definitive: > > postgres=# ALTER SYSTEM SET wal_level TO minimal; > ERROR: configuration is managed externally > > As a bonus, if that GUC is set, we could even check at server startup > that all the configuration files are not writable by the postgres user, > and print a warning or refuse to start up if they are. > > (Another way to read this proposal is to rename the GUC that's been > discussed in this thread to 'configuration_managed_externally'. That > makes it look less like a security feature, and describes the intended > use case.) > > > I agree with pretty much all of this. cheers andrew
Re: What about Perl autodie?
On Mon, Mar 18, 2024 at 2:28 AM Peter Eisentraut wrote: > On 21.02.24 08:26, Peter Eisentraut wrote: > > On 14.02.24 17:52, Peter Eisentraut wrote: > >> A gentler way might be to start using some perlcritic policies like > >> InputOutput::RequireCheckedOpen or the more general > >> InputOutput::RequireCheckedSyscalls and add explicit error checking at > >> the sites it points out. > > > > Here is a start for that. I added the required stanza to perlcriticrc > > and started with an explicit list of functions to check: > > > > functions = chmod flock open read rename seek symlink system > > > > and fixed all the issues it pointed out. > > > > I picked those functions because most existing code already checked > > those, so the omissions are probably unintended, or in some cases also > > because I thought it would be important for test correctness (e.g., some > > tests using chmod). > > > > I didn't design any beautiful error messages, mostly just used "or die > > $!", which mostly matches existing code, and also this is > > developer-level code, so having the system error plus source code > > reference should be ok. > > > > In the second patch, I changed the perlcriticrc stanza to use an > > exclusion list instead of an explicit inclusion list. That way, you can > > see what we are currently *not* checking. I'm undecided which way > > around is better, and exactly what functions we should be checking. (Of > > course, in principle, all of them, but since this is test and build > > support code, not production code, there are probably some reasonable > > compromises to be made.) > > After some pondering, I figured the exclude list is better. So here is > a squashed patch, also with a complete commit message. > > Btw., do we check perlcritic in an automated way, like on the buildfarm? Yes. crake and koel do. cheers andrew
Re: Support json_errdetail in FRONTEND builds
> On Mar 16, 2024, at 8:53 AM, Daniel Gustafsson wrote: > > >> >> On 15 Mar 2024, at 21:56, Andrew Dunstan wrote: >> On Fri, Mar 15, 2024 at 10:15 AM Tom Lane > <mailto:t...@sss.pgh.pa.us>> wrote: >> Daniel Gustafsson mailto:dan...@yesql.se>> writes: >>> I can't see how refusing to free memory owned and controlled by someone >>> else, >>> and throwing an error if attempted, wouldn't be a sound defensive >>> programming >>> measure. >> >> I think the argument is about what "refusal" looks like. >> An Assert seems OK to me, but anything based on elog is >> likely to be problematic, because it'll involve exit() >> somewhere. >> >> Yeah, I agree an Assert seems safest here. >> >> I'd like to get this done ASAP so I can rebase my incremental parse >> patchset. Daniel, do you want to commit it? If not, I can. > > Sure, I can commit these patches. It won't be until tomorrow though since I > prefer to have ample time to monitor the buildfarm, if you are in a bigger > rush > than that then feel free to go ahead. > tomorrow will be fine, thanks Cheers Andrew
Re: Support json_errdetail in FRONTEND builds
On Fri, Mar 15, 2024 at 10:15 AM Tom Lane wrote: > Daniel Gustafsson writes: > > I can't see how refusing to free memory owned and controlled by someone > else, > > and throwing an error if attempted, wouldn't be a sound defensive > programming > > measure. > > I think the argument is about what "refusal" looks like. > An Assert seems OK to me, but anything based on elog is > likely to be problematic, because it'll involve exit() > somewhere. > > > Yeah, I agree an Assert seems safest here. I'd like to get this done ASAP so I can rebase my incremental parse patchset. Daniel, do you want to commit it? If not, I can. cheers andrew
Re: session username in default psql prompt?
On 2024-02-27 Tu 19:19, Kori Lane wrote: Here’s a patch for this. Reposting as the archive mail processor doesn't seem to like the Apple mail attachment. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index cc7d797159..7e82355425 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -4729,7 +4729,7 @@ testdb= \set PROMPT1 '%[%033[1;33;40m%]%n@%/%R%[%033[0m%]%# ' To insert a percent sign into your prompt, write %%. The default prompts are -'%/%R%x%# ' for prompts 1 and 2, and +'%n@%/%R%x%# ' for prompts 1 and 2, and ' ' for prompt 3. diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h index 505f99d8e4..8477f9 100644 --- a/src/bin/psql/settings.h +++ b/src/bin/psql/settings.h @@ -23,8 +23,8 @@ #define DEFAULT_EDITOR_LINENUMBER_ARG "+" #endif -#define DEFAULT_PROMPT1 "%/%R%x%# " -#define DEFAULT_PROMPT2 "%/%R%x%# " +#define DEFAULT_PROMPT1 "%n@%/%R%x%# " +#define DEFAULT_PROMPT2 "%n@%/%R%x%# " #define DEFAULT_PROMPT3 ">> " /*
Re: meson vs tarballs
On 2024-03-13 We 02:31, Andrew Dunstan wrote: On 2024-03-13 We 02:22, Peter Eisentraut wrote: On 13.03.24 07:11, Andrew Dunstan wrote: I and several colleagues have just been trying to build from a tarball with meson. That seems pretty awful and unfriendly and I didn't see anything about it in the docs. At https://www.postgresql.org/docs/16/install-requirements.html is says: """ Alternatively, PostgreSQL can be built using Meson. This is currently experimental and only works when building from a Git checkout (not from a distribution tarball). """ Ah!. Darn, I missed that. Thanks. Of course, when release 17 comes out this had better not be the case, since we have removed the custom Windows build system. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: meson vs tarballs
On 2024-03-13 We 02:22, Peter Eisentraut wrote: On 13.03.24 07:11, Andrew Dunstan wrote: I and several colleagues have just been trying to build from a tarball with meson. That seems pretty awful and unfriendly and I didn't see anything about it in the docs. At https://www.postgresql.org/docs/16/install-requirements.html is says: """ Alternatively, PostgreSQL can be built using Meson. This is currently experimental and only works when building from a Git checkout (not from a distribution tarball). """ Ah!. Darn, I missed that. Thanks. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
meson vs tarballs
I and several colleagues have just been trying to build from a tarball with meson. `meson setup build .` results in this: [...] Message: checking for file conflicts between source and build directory meson.build:2963:2: ERROR: Problem encountered: Non-clean source code directory detected. To build with meson the source tree may not have an in-place, ./configure style, build configured. You can have both meson and ./configure style builds for the same source tree by building out-of-source / VPATH with configure. Alternatively use a separate check out for meson based builds. Conflicting files in source directory: [huge list of files] The conflicting files need to be removed, either by removing the files listed above, or by running configure and then make maintainer-clean. That seems pretty awful and unfriendly and I didn't see anything about it in the docs. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: perl: unsafe empty pattern behavior
On 2024-03-12 Tu 18:59, Tom Lane wrote:
Jeff Davis writes:
On Tue, 2024-03-12 at 18:53 +0100, Alvaro Herrera wrote:
I also tried grepping (for things
like qr{}, qr[], qr||, qr!!) and didn't find anything beyond what you
have ... but I only looked for the "qr" literal, not other ways to
get regexes.
I think that's fine. qr// seems the most dangerous, because it seems to
behave differently in different versions of perl.
I wonder whether perlcritic has sufficiently deep understanding of
Perl code that it could find these hazards. I already checked,
and found that there's no built-in filter for this (at least not
in the perlcritic version I have), but maybe we could write one?
The rules seem to be plug-in modules, so you can make your own
in principle.
Yeah, that was my thought too. I'd start with ProhibitComplexRegexes.pm
as a template.
If nobody else does it I'll have a go, but it might take a while.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: Support json_errdetail in FRONTEND builds
On 2024-03-12 Tu 14:43, Jacob Champion wrote: Hello, Both the incremental JSON [1] and OAuth [2] patchsets would be improved by json_errdetail(), which was removed from FRONTEND builds in b44669b2ca: The routine providing a detailed error message based on the error code is made backend-only, the existing code being unsafe to use in the frontend as the error message may finish by being palloc'd or point to a static string, so there is no way to know if the memory of the message should be pfree'd or not. Attached is a patch to undo this, by attaching any necessary allocations to the JsonLexContext so the caller doesn't have to keep track of them. This is based on the first patch of the OAuth patchset, which additionally needs json_errdetail() to be safe to use from libpq itself. Alvaro pointed out offlist that we don't have to go that far to re-enable this function for the utilities, so this patch is a sort of middle ground between what we have now and what OAuth implements. (There is some additional minimization that could be done to this patch, but I'm hoping to keep the code structure consistent between the two, if the result is acceptable.) Seems reasonable. Two notes that I wanted to point out explicitly: - On the other thread, Daniel contributed a destroyStringInfo() counterpart for makeStringInfo(), which is optional but seemed useful to include here. yeah, although maybe worth a different patch. - After this patch, if a frontend client calls json_errdetail() without calling freeJsonLexContext(), it will leak memory. Not too concerned about this: 1. we tend to be a bit more relaxed about that in frontend programs, especially those not expected to run for long times and especially on error paths, where in many cases the program will just exit anyway. 2. the fix is simple where it's needed. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: [PROPOSAL] Skip test citext_utf8 on Windows
On 2024-03-11 Mo 22:50, Thomas Munro wrote:
On Tue, Mar 12, 2024 at 2:56 PM Andrew Dunstan wrote:
On 2024-03-11 Mo 04:21, Oleg Tselebrovskiy wrote:
Greetings, everyone!
While running "installchecks" on databases with UTF-8 encoding the test
citext_utf8 fails because of Turkish dotted I like this:
SELECT 'i'::citext = 'İ'::citext AS t;
t
---
- t
+ f
(1 row)
I tried to replicate the test's results by hand and with any collation
that I tried (including --locale="Turkish") this test failed
Also an interesing result of my tesing. If you initialize you DB
with -E utf-8 --locale="Turkish" and then run select LOWER('İ');
the output will be this:
lower
---
İ
(1 row)
Which I find strange since lower() uses collation that was passed
(default in this case but still)
Wouldn't we be better off finding a Windows fix for this, instead of
sweeping it under the rug?
Given the sorry state of our Windows locale support, I've started
wondering about deleting it and telling users to adopt our nascent
built-in support or ICU[1].
This other thread [2] says the sorting is intransitive so I don't
think it really meets our needs anyway.
[1]
https://www.postgresql.org/message-id/flat/CA%2BhUKGJhV__g_TJ0jVqPbnTuqT%2B%2BM6KFv2wj%2B9AV-cABNCXN6Q%40mail.gmail.com#bc35c0b88962ff8c24c27aecc1bca72e
[2]
https://www.postgresql.org/message-id/flat/1407a2c0-062b-4e4c-b728-438fdff5cb07%40manitou-mail.org
Makes more sense than just hacking the tests to avoid running them on
Windows. (I also didn't much like doing it by parsing the version
string, although I know there's at least one precedent for doing that.)
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: [PROPOSAL] Skip test citext_utf8 on Windows
On 2024-03-11 Mo 04:21, Oleg Tselebrovskiy wrote:
Greetings, everyone!
While running "installchecks" on databases with UTF-8 encoding the test
citext_utf8 fails because of Turkish dotted I like this:
SELECT 'i'::citext = 'İ'::citext AS t;
t
---
- t
+ f
(1 row)
I tried to replicate the test's results by hand and with any collation
that I tried (including --locale="Turkish") this test failed
Also an interesing result of my tesing. If you initialize you DB
with -E utf-8 --locale="Turkish" and then run select LOWER('İ');
the output will be this:
lower
---
İ
(1 row)
Which I find strange since lower() uses collation that was passed
(default in this case but still)
Wouldn't we be better off finding a Windows fix for this, instead of
sweeping it under the rug?
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-03-07 Th 10:28, Jacob Champion wrote: Some more observations as I make my way through the patch: In src/common/jsonapi.c, +#define JSON_NUM_NONTERMINALS 6 Should this be 5 now? Yep. + res = pg_parse_json_incremental(&(incstate->lex), &(incstate->sem), + chunk, size, is_last); + + expected = is_last ? JSON_SUCCESS : JSON_INCOMPLETE; + + if (res != expected) + json_manifest_parse_failure(context, "parsing failed"); This leads to error messages like pg_verifybackup: error: could not parse backup manifest: parsing failed which I would imagine is going to lead to confused support requests in the event that someone does manage to corrupt their manifest. Can we make use of json_errdetail() and print the line and column numbers? Patch 0001 over at [1] has one approach to making json_errdetail() workable in frontend code. Looks sound on a first look. Maybe we should get that pushed ASAP so we can take advantage of it. Top-level scalars like `false` or `12345` do not parse correctly if the chunk size is too small; instead json_errdetail() reports 'Token "" is invalid'. With small chunk sizes, json_errdetail() additionally segfaults on constructions like `[tru]` or `12zz`. Ugh. Will investigate. For my local testing, I'm carrying the following diff in 001_test_json_parser_incremental.pl: - ok($stdout =~ /SUCCESS/, "chunk size $size: test succeeds"); - ok(!$stderr, "chunk size $size: no error output"); + like($stdout, qr/SUCCESS/, "chunk size $size: test succeeds"); + is($stderr, "", "chunk size $size: no error output"); This is particularly helpful when a test fails spuriously due to code coverage spray on stderr. Makes sense, thanks. I'll have a fresh patch set soon which will also take care of the bitrot. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: speed up a logical replica setup
On 2024-02-27 Tu 05:02, Hayato Kuroda (Fujitsu) wrote: Dear Euler, Sorry, which pg_rewind option did you mention? I cannot find. IIUC, -l is an only option which can accept the path, but it is not related with us. Sorry, I found [1]. I was confused with pg_resetwal. But my opinion was not so changed. The reason why --config-file exists is that pg_rewind requires that target must be stopped, which is different from the current pg_createsubscriber. So I still do not like to add options. [1]: https://www.postgresql.org/docs/current/app-pgrewind.html#:~:text=%2D%2Dconfig%2Dfile%3Dfilename [2]: ``` The target server must be shut down cleanly before running pg_rewind ``` Even though that is a difference I'd still rather we did more or less the same thing more or less the same way across utilities, so I agree with Euler's suggestion. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: [PoC] Federated Authn/z with OAUTHBEARER
On 2024-02-28 We 09:05, Jacob Champion wrote: Daniel and I discussed trying a Python version of the test server, since the standard library there should give us more goodies to work with. A proof of concept is in 0009. I think the big question I have for it is, how would we communicate what we want the server to do for the test? (We could perhaps switch on magic values of the client ID?) In the end I'd like to be testing close to 100% of the failure modes, and that's likely to mean a lot of back-and-forth if the server implementation isn't in the Perl process. Can you give some more details about what this python gadget would buy us? I note that there are a couple of CPAN modules that provide OAuth2 servers, not sure if they would be of any use. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Logging parallel worker draught
On 2024-02-27 Tu 05:03, Benoit Lobréau wrote: On 2/25/24 23:32, Peter Smith wrote: Also, I don't understand how the word "draught" (aka "draft") makes sense here -- I assume the intended word was "drought" (???). yes, that was the intent, sorry about that. English is not my native langage and I was convinced the spelling was correct. Both are English words spelled correctly, but with very different meanings. (Drought is definitely the one you want here.) This reminds me of the Errata section of Sellars and Yeatman's classic "history" work "1066 And All That": "For 'pheasant' read 'peasant' throughout." cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: More new SQL/JSON item methods
On 2024-02-02 Fr 00:31, Jeevan Chalke wrote:
On Thu, Feb 1, 2024 at 11:25 AM Kyotaro Horiguchi
wrote:
At Thu, 1 Feb 2024 09:22:22 +0530, Jeevan Chalke
wrote in
> On Thu, Feb 1, 2024 at 7:24 AM Kyotaro Horiguchi
> wrote:
>
> > At Thu, 01 Feb 2024 10:49:57 +0900 (JST), Kyotaro Horiguchi <
> > horikyota@gmail.com> wrote in
> > > By the way, while playing with this feature, I noticed the
following
> > > error message:
> > >
> > > > select jsonb_path_query('1.1' , '$.boolean()');
> > > > ERROR: numeric argument of jsonpath item method
.boolean() is out of
> > range for type boolean
> > >
> > > The error message seems a bit off to me. For example,
"argument '1.1'
> > > is invalid for type [bB]oolean" seems more appropriate for this
> > > specific issue. (I'm not ceratin about our policy on the
spelling of
> > > Boolean..)
> >
> > Or, following our general convention, it would be spelled as:
> >
> > 'invalid argument for type Boolean: "1.1"'
> >
>
> jsonpath way:
Hmm. I see.
> ERROR: argument of jsonpath item method .boolean() is invalid
for type
> boolean
>
> or, if we add input value, then
>
> ERROR: argument "1.1" of jsonpath item method .boolean() is
invalid for
> type boolean
>
> And this should work for all the error types, like out of range,
not valid,
> invalid input, etc, etc. Also, we don't need separate error
messages for
> string input as well, which currently has the following form:
>
> "string argument of jsonpath item method .%s() is not a valid
> representation.."
Agreed.
Attached are patches based on the discussion.
Thanks, I combined these and pushed the result.
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-02-26 Mo 19:20, Andrew Dunstan wrote: On 2024-02-26 Mo 10:10, Jacob Champion wrote: On Mon, Feb 26, 2024 at 7:08 AM Jacob Champion wrote: As a brute force example of the latter, with the attached diff I get test failures at chunk sizes 1, 2, 3, 4, 6, and 12. But this time with the diff. Ouch. I'll check it out. Thanks! The good news is that the parser is doing fine - this issue was due to a thinko on my part in the test program that got triggered by the input file size being an exact multiple of the chunk size. I'll have a new patch set later this week. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-02-26 Mo 10:10, Jacob Champion wrote: On Mon, Feb 26, 2024 at 7:08 AM Jacob Champion wrote: As a brute force example of the latter, with the attached diff I get test failures at chunk sizes 1, 2, 3, 4, 6, and 12. But this time with the diff. Ouch. I'll check it out. Thanks! cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Can we include capturing logs of pgdata/pg_upgrade_output.d/*/log in buildfarm
On 2024-02-25 Su 11:18, vignesh C wrote: On Thu, 15 Feb 2024 at 08:36, vignesh C wrote: On Thu, 15 Feb 2024 at 07:24, Michael Paquier wrote: On Wed, Feb 14, 2024 at 03:51:08PM +0530, vignesh C wrote: First regex is the testname_clusterinstance_data, second regex is the timestamp used for pg_upgrade, third regex is for the text files generated by pg_upgrade and fourth regex is for the log files generated by pg_upgrade. Can we include these log files also in the buildfarm? [1] - https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo=2024-02-10%2007%3A03%3A10 [2] - https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo=2023-12-07%2003%3A56%3A20 Indeed, these lack some patterns. Why not sending a pull request around [1] to get more patterns covered? I have added a few more patterns to include the pg_upgrade generated files. The attached patch has the changes for the same. Adding Andrew also to get his thoughts on this. I have added the following commitfest entry for this: https://commitfest.postgresql.org/47/4850/ Buildfarm code patches do not belong in the Commitfest, I have marked the item as rejected. You can send me patches directly or add a PR to the buildfarm's github repo. In this case the issue on drongo was a typo, the fix for which I had forgotten to propagate back in December. Note that the buildfarm's TestUpgrade.pm module is only used for branches < 15. For branches >= 15 we run the standard TAP test and this module does nothing. More generally, the collection of logs etc. for pg_upgrade will improve with the next release, which will be soon after I return from a vacation in about 2 weeks - experience shows that making releases just before a vacation is not a good idea :-) cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-02-22 Th 15:29, Jacob Champion wrote: On Thu, Feb 22, 2024 at 1:38 AM Andrew Dunstan wrote: Patch 5 in this series fixes those issues and adjusts most of the tests to add some trailing junk to the pieces of json, so we can be sure that this is done right. This fixes the test failure for me, thanks! I've attached my current mesonification diff, which just adds test_json_parser to the suite. It relies on the PATH that's set up, which appears to include the build directory for both VPATH builds and Meson. OK, thanks, will add this in the next version. Are there plans to fill out the test suite more? Since we should be able to control all the initial conditions, it'd be good to get fairly comprehensive coverage of the new code. Well, it's tested (as we know) by the backup manifest tests. During development, I tested by making the regular parser use the non-recursive parser (see FORCE_JSON_PSTACK). That doesn't test the incremental piece of it, but it does check that the rest of it is doing the right thing. We could probably extend the incremental test by making it output a stream of tokens and making sure that was correct. As an aside, I find the behavior of need_escapes=false to be completely counterintuitive. I know the previous code did this, but it seems like the opposite of "provides unescaped strings" should be "provides raw strings", not "all strings are now NULL". Yes, we could possibly call it "need_strings" or something like that. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-02-20 Tu 19:53, Jacob Champion wrote:
On Tue, Feb 20, 2024 at 2:10 PM Andrew Dunstan wrote:
Well, that didn't help a lot, but meanwhile the CFBot seems to have
decided in the last few days that it's now happy, so full steam aead! ;-)
I haven't been able to track down the root cause yet, but I am able to
reproduce the failure consistently on my machine:
ERROR: could not parse backup manifest: file size is not an
integer: '16384, "Last-Modified": "2024-02-20 23:07:43 GMT",
"Checksum-Algorithm": "CRC32C", "Checksum": "66c829cd" },
{ "Path": "base/4/2606", "Size": 24576, "Last-Modified": "20
Full log is attached.
*sigh* That's weird. I wonder why you can reproduce it and I can't. Can
you give me details of the build? OS, compiler, path to source, build
setup etc.? Anything that might be remotely relevant.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: WIP Incremental JSON Parser
On 2024-01-26 Fr 12:15, Andrew Dunstan wrote: On 2024-01-24 We 13:08, Robert Haas wrote: Maybe you should adjust your patch to dump the manifests into the log file with note(). Then when cfbot runs on it you can see exactly what the raw file looks like. Although I wonder if it's possible that the manifest itself is OK, but somehow it gets garbled when uploaded to the server, either because the client code that sends it or the server code that receives it does something that isn't safe in 32-bit mode. If we hypothesize an off-by-one error or a buffer overrun, that could possibly explain how one field got garbled while the rest of the file is OK. Yeah, I thought earlier today I was on the track of an off by one error, but I was apparently mistaken, so here's the same patch set with an extra patch that logs a bunch of stuff, and might let us see what's upsetting the cfbot. Well, that didn't help a lot, but meanwhile the CFBot seems to have decided in the last few days that it's now happy, so full steam aead! ;-) cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: What about Perl autodie?
On 2024-02-14 We 11:52, Peter Eisentraut wrote: On 08.02.24 16:53, Tom Lane wrote: Daniel Gustafsson writes: On 8 Feb 2024, at 08:01, Peter Eisentraut wrote: I suppose we could start using it for completely new scripts. +1, it would be nice to eventually be able to move to it everywhere so starting now with new scripts may make the eventual transition smoother. I'm still concerned about people carelessly using autodie-reliant code in places where they shouldn't. I offer two safer ways forward: 1. Wait till v16 is the oldest supported branch, and then migrate both HEAD and back branches to using autodie. 2. Don't wait, migrate them all now. This would mean requiring Perl 5.10.1 or later to run the TAP tests, even in back branches. I think #2 might not be all that radical. We have nothing older than 5.14.0 in the buildfarm, so we don't really have much grounds for claiming that 5.8.3 will work today. And 5.10.1 came out in 2009, so how likely is it that anyone cares anymore? A gentler way might be to start using some perlcritic policies like InputOutput::RequireCheckedOpen or the more general InputOutput::RequireCheckedSyscalls and add explicit error checking at the sites it points out. And then if we start using autodie in the future, any inappropriate backpatching of calls lacking error checks would be caught. Yeah, that should work. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: [PATCH] Add native windows on arm64 support
On 2024-02-12 Mo 11:44, Dave Cramer wrote: Dave Cramer www.postgres.rocks On Mon, 12 Feb 2024 at 09:19, Andrew Dunstan wrote: On 2024-02-12 Mo 08:51, Dave Cramer wrote: On Sat, 10 Feb 2024 at 13:28, Andrew Dunstan wrote: On 2024-02-10 Sa 12:20, Dave Cramer wrote: On Sat, 10 Feb 2024 at 11:19, Andrew Dunstan wrote: On 2024-02-09 Fr 14:23, Dave Cramer wrote: Dave Cramer www.postgres.rocks <http://www.postgres.rocks> On Fri, 9 Feb 2024 at 07:18, Dave Cramer <mailto:davecramer@postgres.rocks> wrote: On Fri, 9 Feb 2024 at 00:26, Michael Paquier wrote: On Tue, Feb 06, 2024 at 07:01:49AM -0500, Dave Cramer wrote: > Thanks, this patch works and > testing with meson passes. Only with the version posted at [1]? Interesting, that's the same contents as v8 posted upthread, minus src/tools/ because we don't need to care about them anymore. Andrew, what's happening on the test side? It does not seem you've mentioned any details about what is going wrong, or I have just missed them. > I'll try the buildfarm next. [1]: https://www.postgresql.org/message-id/ea42654a-3dc4-98b0-335b-56b7ec5e5...@dunslane.net interestingly meson test does not produce any error The buildfarm produces the following error for me: -SELECT relname, attname, coltypes, get_columns_length(coltypes) - FROM check_columns - WHERE get_columns_length(coltypes) % 8 != 0 OR - 'name'::regtype::oid = ANY(coltypes); - relname | attname | coltypes | get_columns_length --+-+--+ -(0 rows) - +server closed the connection unexpectedly +This probably means the server terminated abnormally +before or while processing the request. +connection to server was lost Actually digging some more, here is the actual error 2024-02-09 13:31:11.008 -05 postmaster[10672] LOG: server process (PID 11204) was terminated by exception 0xC005 2024-02-09 13:31:11.008 -05 postmaster[10672] DETAIL: Failed process was running: VACUUM; 2024-02-09 13:31:11.008 -05 postmaster[10672] HINT: See C include file "ntstatus.h" for a description of the hexadecimal value. 2024-02-09 13:31:11.008 -05 postmaster[10672] LOG: terminating any other active server processes 2024-02-09 13:31:11.013 -05 postmaster[10672] LOG: all server processes terminated; reinitializing 2024-02-09 13:31:11.034 -05 startup[6152] LOG: database system was interrupted; last known up at 2024-02-09 13:31:01 -05 So how does one debug this ? Also if I `run meson test` I don't see this error. What does the buildfarm do differently? First it does this: meson test -C $pgsql --no-rebuild --suite setup Then it does this (jflag is for the number of jobs): meson test -t $meson_test_timeout $jflag -C $pgsql --logbase checklog --print-errorlogs --no-rebuild --suite regress --test-args=--no-locale running the above manually produces no errors ?? Not for me. I get the error I previously reported, It's an access violation error. OK, so I have managed to get a debugger attached to postgres.exe when it faults and the fault occurs at https://github.com/postgres/postgres/blob/09eb633e1baa3b7cd7929f3cc77f9c46f63c20b1/src/backend/utils/mmgr/dsa.c#L869 span is pointing to 0x0 Further data point. If I select a build type of 'debug' instead of 'debugoptimized' the error disappears. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: [PATCH] Add native windows on arm64 support
On 2024-02-12 Mo 08:51, Dave Cramer wrote: On Sat, 10 Feb 2024 at 13:28, Andrew Dunstan wrote: On 2024-02-10 Sa 12:20, Dave Cramer wrote: On Sat, 10 Feb 2024 at 11:19, Andrew Dunstan wrote: On 2024-02-09 Fr 14:23, Dave Cramer wrote: Dave Cramer www.postgres.rocks <http://www.postgres.rocks> On Fri, 9 Feb 2024 at 07:18, Dave Cramer <mailto:davecramer@postgres.rocks> wrote: On Fri, 9 Feb 2024 at 00:26, Michael Paquier wrote: On Tue, Feb 06, 2024 at 07:01:49AM -0500, Dave Cramer wrote: > Thanks, this patch works and > testing with meson passes. Only with the version posted at [1]? Interesting, that's the same contents as v8 posted upthread, minus src/tools/ because we don't need to care about them anymore. Andrew, what's happening on the test side? It does not seem you've mentioned any details about what is going wrong, or I have just missed them. > I'll try the buildfarm next. [1]: https://www.postgresql.org/message-id/ea42654a-3dc4-98b0-335b-56b7ec5e5...@dunslane.net interestingly meson test does not produce any error The buildfarm produces the following error for me: -SELECT relname, attname, coltypes, get_columns_length(coltypes) - FROM check_columns - WHERE get_columns_length(coltypes) % 8 != 0 OR - 'name'::regtype::oid = ANY(coltypes); - relname | attname | coltypes | get_columns_length --+-+--+ -(0 rows) - +server closed the connection unexpectedly +This probably means the server terminated abnormally +before or while processing the request. +connection to server was lost Actually digging some more, here is the actual error 2024-02-09 13:31:11.008 -05 postmaster[10672] LOG: server process (PID 11204) was terminated by exception 0xC005 2024-02-09 13:31:11.008 -05 postmaster[10672] DETAIL: Failed process was running: VACUUM; 2024-02-09 13:31:11.008 -05 postmaster[10672] HINT: See C include file "ntstatus.h" for a description of the hexadecimal value. 2024-02-09 13:31:11.008 -05 postmaster[10672] LOG: terminating any other active server processes 2024-02-09 13:31:11.013 -05 postmaster[10672] LOG: all server processes terminated; reinitializing 2024-02-09 13:31:11.034 -05 startup[6152] LOG: database system was interrupted; last known up at 2024-02-09 13:31:01 -05 So how does one debug this ? Also if I `run meson test` I don't see this error. What does the buildfarm do differently? First it does this: meson test -C $pgsql --no-rebuild --suite setup Then it does this (jflag is for the number of jobs): meson test -t $meson_test_timeout $jflag -C $pgsql --logbase checklog --print-errorlogs --no-rebuild --suite regress --test-args=--no-locale running the above manually produces no errors ?? Not for me. I get the error I previously reported, It's an access violation error. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: Feature request support MS Entra ID Authentication from On-premises PostreSQL server
On 2024-02-10 Sa 12:26, rs.tr...@gmail.com wrote: Hi all, Don’t know if I got this to the right group. Proposal Template For a New Feature One-line Summary: Feature request Natively integration support Azure Microsoft Entra ID for authentication from On-premises PostreSQL server. Business Use-case: Explain the problem that you are trying to solve with the proposal. Using new Authentciation method (entra ID) vs Ldap method for On-Premises PostgreSQL server databases. User impact with the change: Trying to stream line accounts so we only have one place for Users and accounts, for onboarding and offboarding and our Echo system is starting to move to Azure, but we still have On-premises PostgresSQL servers. Our Security groups want us to use new Authentication methods and have integration into MS Entra ID. I know that I can from the Azure PostgreSQL log in with Azure Entra ID with psql.exe and pgAdmin 4 and have this working for the Azure PostgreSQl database. But have not found a way to do this with our On-premises PostgreSQL server databases. There may be a method for already doing this but I have not found it, and I am very new to PostgreSQL. What is the difference between this and ActiveDirectory? AD is already usable as an authentication mechanism. See for example <https://www.crunchydata.com/blog/windows-active-directory-postgresql-gssapi-kerberos-authentication> cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: [PATCH] Add native windows on arm64 support
On 2024-02-10 Sa 12:20, Dave Cramer wrote: On Sat, 10 Feb 2024 at 11:19, Andrew Dunstan wrote: On 2024-02-09 Fr 14:23, Dave Cramer wrote: Dave Cramer www.postgres.rocks <http://www.postgres.rocks> On Fri, 9 Feb 2024 at 07:18, Dave Cramer <mailto:davecramer@postgres.rocks> wrote: On Fri, 9 Feb 2024 at 00:26, Michael Paquier wrote: On Tue, Feb 06, 2024 at 07:01:49AM -0500, Dave Cramer wrote: > Thanks, this patch works and > testing with meson passes. Only with the version posted at [1]? Interesting, that's the same contents as v8 posted upthread, minus src/tools/ because we don't need to care about them anymore. Andrew, what's happening on the test side? It does not seem you've mentioned any details about what is going wrong, or I have just missed them. > I'll try the buildfarm next. [1]: https://www.postgresql.org/message-id/ea42654a-3dc4-98b0-335b-56b7ec5e5...@dunslane.net interestingly meson test does not produce any error The buildfarm produces the following error for me: -SELECT relname, attname, coltypes, get_columns_length(coltypes) - FROM check_columns - WHERE get_columns_length(coltypes) % 8 != 0 OR - 'name'::regtype::oid = ANY(coltypes); - relname | attname | coltypes | get_columns_length --+-+--+ -(0 rows) - +server closed the connection unexpectedly +This probably means the server terminated abnormally +before or while processing the request. +connection to server was lost Actually digging some more, here is the actual error 2024-02-09 13:31:11.008 -05 postmaster[10672] LOG: server process (PID 11204) was terminated by exception 0xC005 2024-02-09 13:31:11.008 -05 postmaster[10672] DETAIL: Failed process was running: VACUUM; 2024-02-09 13:31:11.008 -05 postmaster[10672] HINT: See C include file "ntstatus.h" for a description of the hexadecimal value. 2024-02-09 13:31:11.008 -05 postmaster[10672] LOG: terminating any other active server processes 2024-02-09 13:31:11.013 -05 postmaster[10672] LOG: all server processes terminated; reinitializing 2024-02-09 13:31:11.034 -05 startup[6152] LOG: database system was interrupted; last known up at 2024-02-09 13:31:01 -05 So how does one debug this ? Also if I `run meson test` I don't see this error. What does the buildfarm do differently? First it does this: meson test -C $pgsql --no-rebuild --suite setup Then it does this (jflag is for the number of jobs): meson test -t $meson_test_timeout $jflag -C $pgsql --logbase checklog --print-errorlogs --no-rebuild --suite regress --test-args=--no-locale cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: recently added jsonpath method change jsonb_path_query, jsonb_path_query_first immutability
On 2024-02-08 Th 21:02, Jeevan Chalke wrote: On Thu, Feb 8, 2024 at 2:22 PM jian he wrote: On Thu, Feb 8, 2024 at 1:27 PM Jeevan Chalke wrote: > > > > On Wed, Feb 7, 2024 at 9:13 PM jian he wrote: >> >> On Wed, Feb 7, 2024 at 7:36 PM Jeevan Chalke >> wrote: >> > Added checkTimezoneIsUsedForCast() check where ever we are casting timezoned to non-timezoned types and vice-versa. >> >> https://www.postgresql.org/docs/devel/functions-json.html >> above Table 9.51. jsonpath Filter Expression Elements, the Note >> section, do we also need to rephrase it? > > > OK. Added a line for the same. > diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 6788ba8..37ae2d1 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -18240,7 +18240,11 @@ ERROR: jsonpath member accessor can only be applied to an object timestamptz, and time to timetz. However, all but the first of these conversions depend on the current setting, and thus can only be performed - within timezone-aware jsonpath functions. + within timezone-aware jsonpath functions. Similarly, other + date/time-related methods that convert string to the date/time types + also do the casting and may involve the current + . To preserve the immutability, those can + only be performed within timezone-aware jsonpath functions. my proposed minor changes: - within timezone-aware jsonpath functions. + within timezone-aware jsonpath functions. Similarly, other + date/time-related methods that convert string to the date/time types + also do the casting and may involve the current + setting. Those conversions can + only be performed within timezone-aware jsonpath functions. I don't have a strong opinion, though. That seems fine as well. Let's leave that to the committer. I edited slightly to my taste, and committed the patch. Thanks. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: [PATCH] Add native windows on arm64 support
On 2024-02-09 Fr 14:23, Dave Cramer wrote: Dave Cramer www.postgres.rocks On Fri, 9 Feb 2024 at 07:18, Dave Cramer wrote: On Fri, 9 Feb 2024 at 00:26, Michael Paquier wrote: On Tue, Feb 06, 2024 at 07:01:49AM -0500, Dave Cramer wrote: > Thanks, this patch works and > testing with meson passes. Only with the version posted at [1]? Interesting, that's the same contents as v8 posted upthread, minus src/tools/ because we don't need to care about them anymore. Andrew, what's happening on the test side? It does not seem you've mentioned any details about what is going wrong, or I have just missed them. > I'll try the buildfarm next. [1]: https://www.postgresql.org/message-id/ea42654a-3dc4-98b0-335b-56b7ec5e5...@dunslane.net interestingly meson test does not produce any error The buildfarm produces the following error for me: -SELECT relname, attname, coltypes, get_columns_length(coltypes) - FROM check_columns - WHERE get_columns_length(coltypes) % 8 != 0 OR - 'name'::regtype::oid = ANY(coltypes); - relname | attname | coltypes | get_columns_length --+-+--+ -(0 rows) - +server closed the connection unexpectedly +This probably means the server terminated abnormally +before or while processing the request. +connection to server was lost Actually digging some more, here is the actual error 2024-02-09 13:31:11.008 -05 postmaster[10672] LOG: server process (PID 11204) was terminated by exception 0xC005 2024-02-09 13:31:11.008 -05 postmaster[10672] DETAIL: Failed process was running: VACUUM; 2024-02-09 13:31:11.008 -05 postmaster[10672] HINT: See C include file "ntstatus.h" for a description of the hexadecimal value. 2024-02-09 13:31:11.008 -05 postmaster[10672] LOG: terminating any other active server processes 2024-02-09 13:31:11.013 -05 postmaster[10672] LOG: all server processes terminated; reinitializing 2024-02-09 13:31:11.034 -05 startup[6152] LOG: database system was interrupted; last known up at 2024-02-09 13:31:01 -05 Yes, this is pretty much what I saw. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: Possibility to disable `ALTER SYSTEM`
On 2024-02-07 We 05:37, Gabriele Bartolini wrote: Hi Joel, On Wed, 7 Feb 2024 at 10:00, Joel Jacobson wrote: On Fri, Sep 8, 2023, at 16:17, Gabriele Bartolini wrote: > ``` > postgres=# ALTER SYSTEM SET wal_level TO minimal; > ERROR: could not open file "postgresql.auto.conf": Permission denied > ``` +1 to simply mark postgresql.auto.conf file as not being writeable. To improve the UX experience, how about first checking if the file is not writeable, or catch EACCESS, and add a user-friendly hint? ``` postgres=# ALTER SYSTEM SET wal_level TO minimal; ERROR: could not open file "postgresql.auto.conf": Permission denied HINT: The ALTER SYSTEM command is effectively disabled as the configuration file is set to read-only. ``` This would do - provided we fix the issue with pg_rewind not handling read-only files in PGDATA. This seems like the simplest solution. And maybe we should be fixing pg_rewind regardless of this issue? cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: What about Perl autodie?
On 2024-02-08 Th 11:08, Daniel Gustafsson wrote: On 8 Feb 2024, at 16:53, Tom Lane wrote: 2. Don't wait, migrate them all now. This would mean requiring Perl 5.10.1 or later to run the TAP tests, even in back branches. I think #2 might not be all that radical. We have nothing older than 5.14.0 in the buildfarm, so we don't really have much grounds for claiming that 5.8.3 will work today. And 5.10.1 came out in 2009, so how likely is it that anyone cares anymore? I would vote for this option, if we don't run the trailing edge anywhere where breakage is visible to developers then it is like you say, far from guaranteed to work. +1 from me too. We kept 5.8 going for a while because it was what the Msys (v1) DTK perl was, but that doesn't matter any more I think. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: recently added jsonpath method change jsonb_path_query, jsonb_path_query_first immutability
On 2024-02-05 Mo 22:06, jian he wrote: Hi. this commit [0] changes immutability of jsonb_path_query, jsonb_path_query_first? If so, it may change other functions also. demo: begin; SET LOCAL TIME ZONE 10.5; with cte(s) as (select jsonb '"2023-08-15 12:34:56 +05:30"') select jsonb_path_query(s, '$.timestamp_tz()')::text,'+10.5'::text,'timestamp_tz'::text from cte union all select jsonb_path_query(s, '$.time()')::text,'+10.5'::text, 'time'::text from cte union all select jsonb_path_query(s, '$.timestamp()')::text,'+10.5'::text,'timestamp'::text from cte union all select jsonb_path_query(s, '$.date()')::text,'+10.5'::text, 'date'::text from cte union all select jsonb_path_query(s, '$.time_tz()')::text,'+10.5'::text, 'time_tz'::text from cte; SET LOCAL TIME ZONE -8; with cte(s) as (select jsonb '"2023-08-15 12:34:56 +05:30"') select jsonb_path_query(s, '$.timestamp_tz()')::text,'-8'::text,'timestamp_tz'::text from cte union all select jsonb_path_query(s, '$.time()')::text,'-8'::text, 'time'::text from cte union all select jsonb_path_query(s, '$.timestamp()')::text,'-8'::text,'timestamp'::text from cte union all select jsonb_path_query(s, '$.date()')::text,'-8'::text, 'date'::text from cte union all select jsonb_path_query(s, '$.time_tz()')::text,'-8'::text, 'time_tz'::text from cte; commit; [0] https://git.postgresql.org/cgit/postgresql.git/commit/?id=66ea94e8e606529bb334515f388c62314956739e ouch. Good catch. Clearly we need to filter these like we do for the .datetime() method. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: 003_extrafiles.pl test fails on Windows with the newer Perl versions
On 2024-02-01 Th 12:32, Tom Lane wrote: Seems like the back branches are still not quite there: I'm seeing Name "PostgreSQL::Test::Utils::windows_os" used only once: possible typo at t/003_extrafiles.pl line 74. in v12 and v13, while it's Name "PostgreSQL::Test::Utils::windows_os" used only once: possible typo at t/003_extrafiles.pl line 85. in v14. v15 and up are OK. *sigh* Have pushed a fix. Thanks for noticing. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: [PATCH] Add native windows on arm64 support
On 2024-01-31 We 10:34, Peter Eisentraut wrote: On 31.01.24 16:20, Andrew Dunstan wrote: - PostgreSQL will only build for the x64 architecture on 64-bit Windows. + PostgreSQL will only build for the x64 and ARM64 architecture on 64-bit Windows. Are there any other 64-bit architectures for Windows? Possibly, the original sentence was meant to communicate that ARM was not supported, in which case it could now be removed? x86? That is in fact the default setting for VS even on ARM64. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: [PATCH] Add native windows on arm64 support
On 2024-01-30 Tu 17:54, Dave Cramer wrote:
On Tue, Jan 30, 2024 at 4:56 PM Andrew Dunstan
wrote:
On 2024-01-30 Tu 09:50, Dave Cramer wrote:
On Tue, 30 Jan 2024 at 08:38, Andrew Dunstan
wrote:
On 2024-01-29 Mo 11:20, Dave Cramer wrote:
Dave Cramer
www.postgres.rocks <http://www.postgres.rocks>
On Mon, 29 Jan 2024 at 11:16, Andrew Dunstan
wrote:
On 2024-01-26 Fr 09:18, Dave Cramer wrote:
On Fri, 26 Jan 2024 at 07:36, Andrew Dunstan
wrote:
On 2024-01-25 Th 20:32, Michael Paquier wrote:
> On Thu, Jan 25, 2024 at 04:52:30PM -0500, Dave
Cramer wrote:
>> On Thu, 25 Jan 2024 at 16:32, Andrew Dunstan
wrote:
>>> On 2024-01-25 Th 16:17, Dave Cramer wrote:
>>> Yeah, I think the default Developer Command
Prompt for VS2022 is set up
>>> for x86 builds. AIUI you should start by
executing "vcvarsall x64_arm64".
>> Yup, now I'm in the same state you are
> Wait a minute here. Based on [1], x64_arm64 means
you can use a x64
> host and you'll be able to produce ARM64 builds,
still these will not
> be able to run on the host where they were built.
How much of the
> patch posted upthread is required to produce such
builds? Basically
> everything from it, I guess, so as build
dependencies can be
> satisfied?
>
> [1]:
https://learn.microsoft.com/en-us/cpp/build/building-on-the-command-line?view=msvc-170
If you look at the table here x86 and x64 are the
only supported host
architectures. But that's OK, the x64 binaries will
run on arm64 (W11
ARM64 has x64 emulation builtin). If that didn't
work Dave and I would
not have got as far as we have. But you want the
x64_arm64 argument to
vcvarsall so you will get ARM64 output.
I've rebuilt it using x64_arm64 and with the attached
(very naive patch) and I still get an x64 binary :(
With this patch I still get a build error, but it's
different :-)
[1406/2088] "link" @src/backend/postgres.exe.rsp
FAILED: src/backend/postgres.exe src/backend/postgres.pdb
"link" @src/backend/postgres.exe.rsp
Creating library src\backend\postgres.exe.lib
storage_lmgr_s_lock.c.obj : error LNK2019: unresolved
external symbol spin_delay referenced in function
perform_spin_delay
src\backend\postgres.exe : fatal error LNK1120: 1
unresolved externals
Did you add the latest lock.patch ?
I'm a bit confused about exactly what needs to be applied.
Can you supply a complete patch to be applied to a pristine
checkout that will let me build?
cheers
See attached.
No, that is what is giving me the error shown above (just tried
again to be certain). And it's not surprising, as patch 2 #ifdef's
out the definition of spin_delay().
If you can get a complete build with these patches then I suspect
you're not doing a proper ARM64 build.
Okay I will look when I get home in a week
I made some progress. The attached is mostly taken from
<https://postgr.es/m/dbee741f-b9b7-a0d5-1b1b-f9b532bb6...@linaro.org>
With it applied I was able to get a successful build using the buildfarm
client. However, there are access violations when running some tests, so
there is still some work to do, apparently.
cheers
andrew
--
Andrew Dunstan
EDB:https://www.enterprisedb.com
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index ed5b285a5e..d9b8649dab 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -4150,7 +4150,7 @@ make: *** [postgres] Error 1
Special Considerations for 64-Bit Windows
- PostgreSQL will only build for the x64 architecture on 64-bit Windows.
+ PostgreSQL will only build for the x64 and ARM64 architecture on 64-bit Windows.
Mixing 32- and 64-bit versions in the same build tree is not supported.
diff --git a/meson.build b/meson.build
index 8ed51b6aae..14aea924ec 100644
--- a/meson.build
+++ b/meson.build
@@ -2046,8 +2046,11 @@ int main(void)
elif host_cpu == 'arm' or host_cpu == 'aarch64'
prog = '''
+#ifdef _MSC_VER
+#include
+#else
#include
-
+#endif
int main(void)
{
unsigned int crc = 0;
Re: 003_extrafiles.pl test fails on Windows with the newer Perl versions
On 2024-01-30 Tu 18:06, Tom Lane wrote: Andrew Dunstan writes: Pushed to all live branches. Thanks for the patch. v12 and v13 branches aren't looking good: Global symbol "$test_primary_datadir" requires explicit package name (did you forget to declare "my $test_primary_datadir"?) at t/003_extrafiles.pl line 80. Execution of t/003_extrafiles.pl aborted due to compilation errors. # Looks like your test exited with 255 before it could output anything. [17:19:57] t/003_extrafiles.pl .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 5/5 subtests Will fix. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: 003_extrafiles.pl test fails on Windows with the newer Perl versions
On 2024-01-30 Tu 06:49, Andrew Dunstan wrote: On 2024-01-30 Tu 06:21, Nazir Bilal Yavuz wrote: Hi, I was trying to install newer Perl versions to Windows CI images and found that 003_extrafiles.pl test fails on Windows with: (0.183s) not ok 2 - file lists match (0.000s) # Failed test 'file lists match' # at C:/cirrus/src/bin/pg_rewind/t/003_extrafiles.pl line 81. (0.000s) # Structures begin differing at: # $got->[0] = 'C:/cirrus/build/testrun/pg_rewind/003_extrafiles/data/t_003_extrafiles_primary_local_data/pgdata/tst_both_dir' # $expected->[0] = 'C:\cirrus\build/testrun/pg_rewind/003_extrafiles\data/t_003_extrafiles_primary_local_data/pgdata/tst_both_dir' (0.263s) not ok 5 - file lists match (0.000s) # Failed test 'file lists match' # at C:/cirrus/src/bin/pg_rewind/t/003_extrafiles.pl line 81. (0.000s) # Structures begin differing at: # $got->[0] = 'C:/cirrus/build/testrun/pg_rewind/003_extrafiles/data/t_003_extrafiles_primary_remote_data/pgdata/tst_both_dir' # $expected->[0] = 'C:\cirrus\build/testrun/pg_rewind/003_extrafiles\data/t_003_extrafiles_primary_remote_data/pgdata/tst_both_dir' It looks like File::Find converts backslashes to slashes in the newer Perl versions. I tried to find the related commit and found this: https://github.com/Perl/perl5/commit/414f14df98cb1c9a20f92c5c54948b67c09f072d To solve this, I did the same conversion for Windows before comparing the paths. And to support all Perl versions, I decided to always convert them on Windows regardless of the Perl's version. The fix is attached. I looked at other File::Find appearances in the code but they do not compare the paths. So, I do not think there is any need to fix them. Any kind of feedback would be appreciated. Looks reasonable on the face of it. I'll see about pushing this today. Pushed to all live branches. Thanks for the patch. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: [PATCH] Add native windows on arm64 support
On 2024-01-30 Tu 09:50, Dave Cramer wrote: On Tue, 30 Jan 2024 at 08:38, Andrew Dunstan wrote: On 2024-01-29 Mo 11:20, Dave Cramer wrote: Dave Cramer www.postgres.rocks <http://www.postgres.rocks> On Mon, 29 Jan 2024 at 11:16, Andrew Dunstan wrote: On 2024-01-26 Fr 09:18, Dave Cramer wrote: On Fri, 26 Jan 2024 at 07:36, Andrew Dunstan wrote: On 2024-01-25 Th 20:32, Michael Paquier wrote: > On Thu, Jan 25, 2024 at 04:52:30PM -0500, Dave Cramer wrote: >> On Thu, 25 Jan 2024 at 16:32, Andrew Dunstan wrote: >>> On 2024-01-25 Th 16:17, Dave Cramer wrote: >>> Yeah, I think the default Developer Command Prompt for VS2022 is set up >>> for x86 builds. AIUI you should start by executing "vcvarsall x64_arm64". >> Yup, now I'm in the same state you are > Wait a minute here. Based on [1], x64_arm64 means you can use a x64 > host and you'll be able to produce ARM64 builds, still these will not > be able to run on the host where they were built. How much of the > patch posted upthread is required to produce such builds? Basically > everything from it, I guess, so as build dependencies can be > satisfied? > > [1]: https://learn.microsoft.com/en-us/cpp/build/building-on-the-command-line?view=msvc-170 If you look at the table here x86 and x64 are the only supported host architectures. But that's OK, the x64 binaries will run on arm64 (W11 ARM64 has x64 emulation builtin). If that didn't work Dave and I would not have got as far as we have. But you want the x64_arm64 argument to vcvarsall so you will get ARM64 output. I've rebuilt it using x64_arm64 and with the attached (very naive patch) and I still get an x64 binary :( With this patch I still get a build error, but it's different :-) [1406/2088] "link" @src/backend/postgres.exe.rsp FAILED: src/backend/postgres.exe src/backend/postgres.pdb "link" @src/backend/postgres.exe.rsp Creating library src\backend\postgres.exe.lib storage_lmgr_s_lock.c.obj : error LNK2019: unresolved external symbol spin_delay referenced in function perform_spin_delay src\backend\postgres.exe : fatal error LNK1120: 1 unresolved externals Did you add the latest lock.patch ? I'm a bit confused about exactly what needs to be applied. Can you supply a complete patch to be applied to a pristine checkout that will let me build? cheers See attached. No, that is what is giving me the error shown above (just tried again to be certain). And it's not surprising, as patch 2 #ifdef's out the definition of spin_delay(). If you can get a complete build with these patches then I suspect you're not doing a proper ARM64 build. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: [PATCH] Add native windows on arm64 support
On 2024-01-29 Mo 11:20, Dave Cramer wrote: Dave Cramer www.postgres.rocks On Mon, 29 Jan 2024 at 11:16, Andrew Dunstan wrote: On 2024-01-26 Fr 09:18, Dave Cramer wrote: On Fri, 26 Jan 2024 at 07:36, Andrew Dunstan wrote: On 2024-01-25 Th 20:32, Michael Paquier wrote: > On Thu, Jan 25, 2024 at 04:52:30PM -0500, Dave Cramer wrote: >> On Thu, 25 Jan 2024 at 16:32, Andrew Dunstan wrote: >>> On 2024-01-25 Th 16:17, Dave Cramer wrote: >>> Yeah, I think the default Developer Command Prompt for VS2022 is set up >>> for x86 builds. AIUI you should start by executing "vcvarsall x64_arm64". >> Yup, now I'm in the same state you are > Wait a minute here. Based on [1], x64_arm64 means you can use a x64 > host and you'll be able to produce ARM64 builds, still these will not > be able to run on the host where they were built. How much of the > patch posted upthread is required to produce such builds? Basically > everything from it, I guess, so as build dependencies can be > satisfied? > > [1]: https://learn.microsoft.com/en-us/cpp/build/building-on-the-command-line?view=msvc-170 If you look at the table here x86 and x64 are the only supported host architectures. But that's OK, the x64 binaries will run on arm64 (W11 ARM64 has x64 emulation builtin). If that didn't work Dave and I would not have got as far as we have. But you want the x64_arm64 argument to vcvarsall so you will get ARM64 output. I've rebuilt it using x64_arm64 and with the attached (very naive patch) and I still get an x64 binary :( With this patch I still get a build error, but it's different :-) [1406/2088] "link" @src/backend/postgres.exe.rsp FAILED: src/backend/postgres.exe src/backend/postgres.pdb "link" @src/backend/postgres.exe.rsp Creating library src\backend\postgres.exe.lib storage_lmgr_s_lock.c.obj : error LNK2019: unresolved external symbol spin_delay referenced in function perform_spin_delay src\backend\postgres.exe : fatal error LNK1120: 1 unresolved externals Did you add the latest lock.patch ? I'm a bit confused about exactly what needs to be applied. Can you supply a complete patch to be applied to a pristine checkout that will let me build? cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: 003_extrafiles.pl test fails on Windows with the newer Perl versions
On 2024-01-30 Tu 06:21, Nazir Bilal Yavuz wrote: Hi, I was trying to install newer Perl versions to Windows CI images and found that 003_extrafiles.pl test fails on Windows with: (0.183s) not ok 2 - file lists match (0.000s) # Failed test 'file lists match' # at C:/cirrus/src/bin/pg_rewind/t/003_extrafiles.pl line 81. (0.000s) # Structures begin differing at: # $got->[0] = 'C:/cirrus/build/testrun/pg_rewind/003_extrafiles/data/t_003_extrafiles_primary_local_data/pgdata/tst_both_dir' # $expected->[0] = 'C:\cirrus\build/testrun/pg_rewind/003_extrafiles\data/t_003_extrafiles_primary_local_data/pgdata/tst_both_dir' (0.263s) not ok 5 - file lists match (0.000s) # Failed test 'file lists match' # at C:/cirrus/src/bin/pg_rewind/t/003_extrafiles.pl line 81. (0.000s) # Structures begin differing at: # $got->[0] = 'C:/cirrus/build/testrun/pg_rewind/003_extrafiles/data/t_003_extrafiles_primary_remote_data/pgdata/tst_both_dir' # $expected->[0] = 'C:\cirrus\build/testrun/pg_rewind/003_extrafiles\data/t_003_extrafiles_primary_remote_data/pgdata/tst_both_dir' It looks like File::Find converts backslashes to slashes in the newer Perl versions. I tried to find the related commit and found this: https://github.com/Perl/perl5/commit/414f14df98cb1c9a20f92c5c54948b67c09f072d To solve this, I did the same conversion for Windows before comparing the paths. And to support all Perl versions, I decided to always convert them on Windows regardless of the Perl's version. The fix is attached. I looked at other File::Find appearances in the code but they do not compare the paths. So, I do not think there is any need to fix them. Any kind of feedback would be appreciated. Looks reasonable on the face of it. I'll see about pushing this today. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Should we remove -Wdeclaration-after-statement?
On 2024-01-29 Mo 14:58, Andres Freund wrote: Hi, On 2023-12-27 12:48:40 +0100, Jelte Fennema-Nio wrote: Postgres currently requires all variables to be declared at the top of the function, because it specifies -Wdeclaration-after-statement. One of the reasons that we had this warning was because C89 required this style of declaration. Requiring it everywhere made backporting easier, since some of our older supported PG versions needed to compile on C89. Now that we have dropped support for PG11 that reason goes away, since now all supported Postgres versions require C99. So, I think it's worth reconsidering if we want this warning to be enabled or not. +1 for allowing declarations to be intermixed with code, I'm about +0.5. Many Java, C++, Perl, and indeed C programmers might find it surprising that we're having this debate. On the more modern language front the same goes for Go and Rust. It seems clear that the language trend is mostly in this direction. But it's not something worth having a long and contentious debate over. We have plenty of better candidates for that :-) -infinity for changing existing code to do so. ditto. On that at least I think there's close to unanimous agreement. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: [PATCH] Add native windows on arm64 support
On 2024-01-26 Fr 09:18, Dave Cramer wrote: On Fri, 26 Jan 2024 at 07:36, Andrew Dunstan wrote: On 2024-01-25 Th 20:32, Michael Paquier wrote: > On Thu, Jan 25, 2024 at 04:52:30PM -0500, Dave Cramer wrote: >> On Thu, 25 Jan 2024 at 16:32, Andrew Dunstan wrote: >>> On 2024-01-25 Th 16:17, Dave Cramer wrote: >>> Yeah, I think the default Developer Command Prompt for VS2022 is set up >>> for x86 builds. AIUI you should start by executing "vcvarsall x64_arm64". >> Yup, now I'm in the same state you are > Wait a minute here. Based on [1], x64_arm64 means you can use a x64 > host and you'll be able to produce ARM64 builds, still these will not > be able to run on the host where they were built. How much of the > patch posted upthread is required to produce such builds? Basically > everything from it, I guess, so as build dependencies can be > satisfied? > > [1]: https://learn.microsoft.com/en-us/cpp/build/building-on-the-command-line?view=msvc-170 If you look at the table here x86 and x64 are the only supported host architectures. But that's OK, the x64 binaries will run on arm64 (W11 ARM64 has x64 emulation builtin). If that didn't work Dave and I would not have got as far as we have. But you want the x64_arm64 argument to vcvarsall so you will get ARM64 output. I've rebuilt it using x64_arm64 and with the attached (very naive patch) and I still get an x64 binary :( With this patch I still get a build error, but it's different :-) [1406/2088] "link" @src/backend/postgres.exe.rsp FAILED: src/backend/postgres.exe src/backend/postgres.pdb "link" @src/backend/postgres.exe.rsp Creating library src\backend\postgres.exe.lib storage_lmgr_s_lock.c.obj : error LNK2019: unresolved external symbol spin_delay referenced in function perform_spin_delay src\backend\postgres.exe : fatal error LNK1120: 1 unresolved externals cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: [PATCH] Add native windows on arm64 support
On 2024-01-26 Fr 09:18, Dave Cramer wrote: On Fri, 26 Jan 2024 at 07:36, Andrew Dunstan wrote: On 2024-01-25 Th 20:32, Michael Paquier wrote: > On Thu, Jan 25, 2024 at 04:52:30PM -0500, Dave Cramer wrote: >> On Thu, 25 Jan 2024 at 16:32, Andrew Dunstan wrote: >>> On 2024-01-25 Th 16:17, Dave Cramer wrote: >>> Yeah, I think the default Developer Command Prompt for VS2022 is set up >>> for x86 builds. AIUI you should start by executing "vcvarsall x64_arm64". >> Yup, now I'm in the same state you are > Wait a minute here. Based on [1], x64_arm64 means you can use a x64 > host and you'll be able to produce ARM64 builds, still these will not > be able to run on the host where they were built. How much of the > patch posted upthread is required to produce such builds? Basically > everything from it, I guess, so as build dependencies can be > satisfied? > > [1]: https://learn.microsoft.com/en-us/cpp/build/building-on-the-command-line?view=msvc-170 If you look at the table here x86 and x64 are the only supported host architectures. But that's OK, the x64 binaries will run on arm64 (W11 ARM64 has x64 emulation builtin). If that didn't work Dave and I would not have got as far as we have. But you want the x64_arm64 argument to vcvarsall so you will get ARM64 output. I've rebuilt it using x64_arm64 and with the attached (very naive patch) and I still get an x64 binary :( I am definitely getting ARM64 binaries (e.g. for initdb.exe the Windows on ARM compatibility setting is greyed out) I'll try your patch. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Re: psql: add \create_function command
On 2024-01-26 Fr 15:17, Tom Lane wrote:
Pavel Stehule writes:
I don't know, maybe I have a problem with the described use case. I cannot
imagine holding the body and head of PL routines in different places and I
don't understand the necessity to join it.
It seems a little weird to me too, and I would vote against accepting
\create_function as described because I think too few people would
want to use it. However, the idea of an easy way to pull in a file
and convert it to a SQL literal seems like it has many applications.
Yes, this proposal is far too narrow and would not cater for many use
cases I have had in the past.
I like your ideas upthread about \file_read and :{filename}
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
Re: PG versus libxml2 2.12.x
On 2024-01-27 Sa 14:04, Tom Lane wrote: Buildfarm member caiman has been failing build for a couple weeks now. The reason turns out to be that recent libxml2 has decided to throw a "const" into the signature required for custom error handlers. (API compatibility? What's that?) I don't mind adopting the "const" --- it's a good idea in isolation. The trouble is in fixing our code to work with both old and new libxml2 versions. We could thrash around with a configure test or something, but I think the most expedient answer is just to insert some explicit casts, as shown in the attached. It's possible though that some compilers will throw a cast-away-const warning. I'm not seeing any, but ... Also, I'm seeing a deprecation warning in contrib/xml2/xpath.c for xmlLoadExtDtdDefaultValue = 1; I'm not sure why that's still there, given that we disabled external DTD access ages ago. I propose we just remove it. In short, I suggest the attached. Looks reasonable. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
