Re: Better error reporting from extension scripts (Was: Extend ALTER OPERATOR)

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