From: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
---|---|
To: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
Cc: | Christoph Berg <myon(at)debian(dot)org>, Michael Banck <mbanck(at)gmx(dot)net>, Tommy Pavlicek <tommypav122(at)gmail(dot)com>, Dagfinn Ilmari Mannsåker <ilmari(at)ilmari(dot)org>, pgsql-hackers(at)lists(dot)postgresql(dot)org, jian(dot)universality(at)gmail(dot)com |
Subject: | Re: Better error reporting from extension scripts (Was: Extend ALTER OPERATOR) |
Date: | 2024-10-24 15:47:52 |
Message-ID: | 2480333.1729784872@sss.pgh.pa.us |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
In the no-good-deed-goes-unpunished department: buildfarm member
hamerkop doesn't like this patch [1]. The diffs look like
@@ -77,7 +77,7 @@
ERROR: syntax error at or near "FUNCTIN"
LINE 1: CREATE FUNCTIN my_erroneous_func(int) RETURNS int LANGUAGE S...
^
-QUERY: CREATE FUNCTIN my_erroneous_func(int) RETURNS int LANGUAGE SQL
+QUERY: CREATE FUNCTIN my_erroneous_func(int) RETURNS int LANGUAGE SQL
AS $$ SELECT $1 + 1 $$;
CONTEXT: extension script file "test_ext7--2.0--2.1bad.sql", near line 10
alter extension test_ext7 update to '2.2bad';
It's hard to be totally sure from the web page, but I suppose what
is happening is that the newline within the quoted query fragment
is represented as "\r\n" not just "\n". (I wonder why the cfbot
failed to detect this; there must be more moving parts involved
than just "it's Windows".)
The reason why this is happening seems fairly clear: extension.c's
read_whole_file() opens the script file with PG_BINARY_R, preventing
Windows' libc from hiding DOS-style newlines from us, even though the
git checkout on that machine is evidently using DOS newlines.
That seems like a rather odd choice. Elsewhere in the same module,
parse_extension_control_file() opens control files with plain "r",
so that those act differently. I checked the git history and did
not learn much. The original extensions commit d9572c4e3 implemented
reading with a call to read_binary_file(), and we seem to have just
carried that behavioral decision forward through various refactorings.
I don't recall if there was an intentional choice to use binary read
or that was just a random decision to use an available function.
So what I'd like to do to fix this is to change
- if ((file = AllocateFile(filename, PG_BINARY_R)) == NULL)
+ if ((file = AllocateFile(filename, "r")) == NULL)
The argument against that is it creates a nonzero chance of breaking
somebody's extension script file on Windows. But there's a
counter-argument that it might *prevent* bugs on Windows, by making
script behavior more nearly identical to what it is on not-Windows.
So I think that's kind of a wash.
Other approaches we could take with perhaps-smaller blast radii
include making script_error_callback() trim \r's out of the quoted
text (ugly) or creating a variant expected-file (hard to maintain,
and I wonder if it'd survive git-related newline munging).
Thoughts?
regards, tom lane
[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hamerkop&dt=2024-10-23%2011%3A00%3A37
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2024-10-24 16:06:32 | Re: general purpose array_sort |
Previous Message | David G. Johnston | 2024-10-24 15:40:04 | Re: general purpose array_sort |