From: Tom Lane <tgl@sss.pgh.pa.us>
To: Pavel Stehule <pavel.stehule@gmail.com>
cc: Christoph Berg <myon@debian.org>, Michael Banck <mbanck@gmx.net>,
        Tommy Pavlicek <tommypav122@gmail.com>,
        =?UTF-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org>,
        pgsql-hackers@lists.postgresql.org, jian.universality@gmail.com
Subject: Re: Better error reporting from extension scripts (Was: Extend ALTER
 OPERATOR)
In-reply-to: 
 <CAFj8pRCtFo2jyJAjXpzB8Rg=mtPXfS44bp2gM4Ea-iRD-wA9Xw@mail.gmail.com>
References: 
 <CAEhP-W_1bMTLan2LtzS+xr=b-Wep56gw+OoirABQ8b2aOw+5yg@mail.gmail.com>
 <ZTfL1G1fBPQHVPWV@msg.df7cb.de> <560086.1698160614@sss.pgh.pa.us>
 <ZTfjXzOKFI9AawJF@msg.df7cb.de> <570833.1698162135@sss.pgh.pa.us>
 <66f33840.050a0220.1f9e60.7fb9@mx.google.com> <ZvV1ClhnbJLCz7Sm@msg.df7cb.de>
 <325394.1727365907@sss.pgh.pa.us> <ZvaiS57G10dITjQm@msg.df7cb.de>
 <571930.1727454668@sss.pgh.pa.us> <Zvbr5zv78AfhB83S@msg.df7cb.de>
 <621972.1727459684@sss.pgh.pa.us> <1092841.1727552736@sss.pgh.pa.us>
 <2245576.1728418678@sss.pgh.pa.us>
 <CAFj8pRBGquZK3aTYvN559FFqa=bMXEuxZZBLc6kMd671u62hTQ@mail.gmail.com>
 <2864637.1728662904@sss.pgh.pa.us>
 <CAFj8pRCF2-ObuMHV5sxQbq9sZ+hCv5mV5SUToD8=9Qw8tZ7DgQ@mail.gmail.com>
 <CAFj8pRCw09RUROc31nJCZBPXrO5fxaDO-Lg885eXMUPwjBZNSw@mail.gmail.com>
 <1967263.1729611423@sss.pgh.pa.us>
 <CAFj8pRCtFo2jyJAjXpzB8Rg=mtPXfS44bp2gM4Ea-iRD-wA9Xw@mail.gmail.com>
Comments: In-reply-to Pavel Stehule <pavel.stehule@gmail.com>
	message dated "Tue, 22 Oct 2024 20:35:19 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2480332.1729784872.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 24 Oct 2024 11:47:52 -0400
Message-ID: <2480333.1729784872@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2480333.1729784872%40sss.pgh.pa.us>
Precedence: bulk

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 1=
0
 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 =3D AllocateFile(filename, PG_BINARY_R)) =3D=3D NULL)
+	if ((file =3D AllocateFile(filename, "r")) =3D=3D 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=3Dhamerkop&dt=3D=
2024-10-23%2011%3A00%3A37