Horton

Thread

Commits

  1. Integrate pg_bsd_indent into our build/test infrastructure.

  2. Sync pg_bsd_indent's copyright notices with Postgres practice.

  3. Import pg_bsd_indent sources.

  1. Importing pg_bsd_indent into our source tree

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-09T18:30:30Z

    As per the discussion at [1], here's a patchset to integrate
pg_bsd_indent into our main source tree, so that people don't
have to pull down a separate repo to get this tool.

0001 is a verbatim import of the current pg_bsd_indent repo contents.
I felt committing this separately is useful for traceability.

0002 adjusts the copyright notices to 3-clause BSD style,
so that we don't get complaints about our tree containing
copyrights inconsistent with the main Postgres license.

0003 is the first non-boring bit: it updates the Makefile
and some other things to account for now being an in-tree
build not out-of-tree.  Also, since 0002 already meant that
the README isn't exactly like upstream's, I got rid of the
separate README.pg_bsd_indent file and merged that info
into README.

0004 is the patch discussed at [2] to improve pgindent's
handling of multiline variable initializations.

0003 lacks meson support (anyone want to help with that?)
but otherwise these seem committable to me.  I'd anticipate
pushing 0001-0003 shortly but holding 0004 until we are ready
to do the post-March-CF pgindent run.  (Come to think of it,
0004 had probably better include a pg_bsd_indent version
bump too.)

			regards, tom lane

[1] https://www.postgresql.org/message-id/20230123001518.6hxyiczhn4kadvmf%40awork3.anarazel.de
[2] https://www.postgresql.org/message-id/flat/20230120013137.7ky7nl4e4zjorrfa%40awork3.anarazel.de

  2. Re: Importing pg_bsd_indent into our source tree

    Andres Freund <andres@anarazel.de> — 2023-02-09T21:02:02Z

    Hi,

On 2023-02-09 13:30:30 -0500, Tom Lane wrote:
> 0003 lacks meson support (anyone want to help with that?)

I'll give it a go, unless somebody else wants to.


Do we expect pg_bsd_indent to build / work on windows, right now? If it
doesn't, do we want to make that a hard requirement?

I'll have CI test that, once I added meson support.


> I'd anticipate pushing 0001-0003 shortly but holding 0004 until we are ready
> to do the post-March-CF pgindent run.  (Come to think of it, 0004 had
> probably better include a pg_bsd_indent version bump too.)

How large is the diff that creates? If it's not super-widespread, it might be
ok to do that earlier. I wouldn't mind not seeing that uglyness every time I
run pgindent on a patch... Although I guess post-March-CF isn't that far away
at this point :)

Greetings,

Andres Freund

  3. Re: Importing pg_bsd_indent into our source tree

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-09T21:42:08Z

    Andres Freund <andres@anarazel.de> writes:
> On 2023-02-09 13:30:30 -0500, Tom Lane wrote:
>> 0003 lacks meson support (anyone want to help with that?)

> I'll give it a go, unless somebody else wants to.

Thanks.

> Do we expect pg_bsd_indent to build / work on windows, right now?

It would be desirable, for sure.  I've not noticed anything remarkably
unportable in the code, so probably it's just a matter of getting the
build infrastructure to build it.  I suppose that we aren't going to
update the src/tools/msvc scripts anymore, so getting meson to handle
it should be enough??

>> I'd anticipate pushing 0001-0003 shortly but holding 0004 until we are ready
>> to do the post-March-CF pgindent run.  (Come to think of it, 0004 had
>> probably better include a pg_bsd_indent version bump too.)

> How large is the diff that creates? If it's not super-widespread, it might be
> ok to do that earlier.

It wasn't that big; I posted it in the thread discussing that change.

I think the real issue might just be that, assuming we bump the
pg_bsd_indent version number, that in itself would force interested
committers to update their copy Right Now.  I'd rather give a little
notice.

			regards, tom lane

  4. Re: Importing pg_bsd_indent into our source tree

    Andres Freund <andres@anarazel.de> — 2023-02-09T21:55:32Z

    Hi,

On 2023-02-09 13:02:02 -0800, Andres Freund wrote:
> On 2023-02-09 13:30:30 -0500, Tom Lane wrote:
> > 0003 lacks meson support (anyone want to help with that?)
>
> I'll give it a go, unless somebody else wants to.

Did that in the attached.

I didn't convert the test though, due to the duplicating it'd create. Perhaps
we should just move it to a shell script? Or maybe it just doesn't matter
enough to bother with?


> Do we expect pg_bsd_indent to build / work on windows, right now? If it
> doesn't, do we want to make that a hard requirement?

> I'll have CI test that, once I added meson support.

It doesn't build as-is with msvc, but does build with mingw. Failure:
https://cirrus-ci.com/task/6290206869946368?logs=build#L1573

"cl" "-Isrc\tools/pg_bsd_indent\pg_bsd_indent.exe.p" "-Isrc\tools/pg_bsd_indent" "-I..\src\tools\pg_bsd_indent" "-Isrc\include" "-I..\src\include" "-Ic:\openssl\1.1\include" "-I..\src\include\port\win32" "-I..\src\include\port\win32_msvc" "/MDd" "/nologo" "/showIncludes" "/utf-8" "/W2" "/Od" "/Zi" "/DWIN32" "/DWINDOWS" "/D__WINDOWS__" "/D__WIN32__" "/D_CRT_SECURE_NO_DEPRECATE" "/D_CRT_NONSTDC_NO_DEPRECATE" "/wd4018" "/wd4244" "/wd4273" "/wd4101" "/wd4102" "/wd4090" "/wd4267" "/Fdsrc\tools/pg_bsd_indent\pg_bsd_indent.exe.p\args.c.pdb" /Fosrc/tools/pg_bsd_indent/pg_bsd_indent.exe.p/args.c.obj "/c" ../src/tools/pg_bsd_indent/args.c
../src/tools/pg_bsd_indent/args.c(179): error C2065: 'PATH_MAX': undeclared identifier
../src/tools/pg_bsd_indent/args.c(179): error C2057: expected constant expression
../src/tools/pg_bsd_indent/args.c(179): error C2466: cannot allocate an array of constant size 0
../src/tools/pg_bsd_indent/args.c(179): error C2133: 'fname': unknown size
../src/tools/pg_bsd_indent/args.c(183): warning C4034: sizeof returns 0
../src/tools/pg_bsd_indent/args.c(185): warning C4034: sizeof returns 0
[1557/2161] Compiling C object src/tools/pg_bsd_indent/pg_bsd_indent.exe.p/err.c.obj
[1558/2161] Precompiling header src/interfaces/ecpg/ecpglib/libecpg.dll.p/meson_pch-c.c
[1559/2161] Compiling C object src/tools/pg_bsd_indent/pg_bsd_indent.exe.p/indent.c.obj
FAILED: src/tools/pg_bsd_indent/pg_bsd_indent.exe.p/indent.c.obj
"cl" "-Isrc\tools/pg_bsd_indent\pg_bsd_indent.exe.p" "-Isrc\tools/pg_bsd_indent" "-I..\src\tools\pg_bsd_indent" "-Isrc\include" "-I..\src\include" "-Ic:\openssl\1.1\include" "-I..\src\include\port\win32" "-I..\src\include\port\win32_msvc" "/MDd" "/nologo" "/showIncludes" "/utf-8" "/W2" "/Od" "/Zi" "/DWIN32" "/DWINDOWS" "/D__WINDOWS__" "/D__WIN32__" "/D_CRT_SECURE_NO_DEPRECATE" "/D_CRT_NONSTDC_NO_DEPRECATE" "/wd4018" "/wd4244" "/wd4273" "/wd4101" "/wd4102" "/wd4090" "/wd4267" "/Fdsrc\tools/pg_bsd_indent\pg_bsd_indent.exe.p\indent.c.pdb" /Fosrc/tools/pg_bsd_indent/pg_bsd_indent.exe.p/indent.c.obj "/c" ../src/tools/pg_bsd_indent/indent.c
../src/tools/pg_bsd_indent/indent.c(63): error C2065: 'MAXPATHLEN': undeclared identifier
../src/tools/pg_bsd_indent/indent.c(63): error C2057: expected constant expression
../src/tools/pg_bsd_indent/indent.c(63): error C2466: cannot allocate an array of constant size 0

This specific issue at least should be easily fixable.


Freebsd emits a compiler warning:

[21:37:50.909] In file included from ../src/tools/pg_bsd_indent/indent.c:54:
[21:37:50.909] ../src/tools/pg_bsd_indent/indent.h:31:9: warning: 'nitems' macro redefined [-Wmacro-redefined]
[21:37:50.909] #define nitems(array) (sizeof (array) / sizeof (array[0]))
[21:37:50.909]         ^
[21:37:50.909] /usr/include/sys/param.h:306:9: note: previous definition is here
[21:37:50.909] #define nitems(x)       (sizeof((x)) / sizeof((x)[0]))
[21:37:50.909]         ^
[21:37:50.911] 1 warning generated.



To we really want to require users to install pg_bsd_indent into PATH? Seems
like we ought to have a build target to invoke pgindent with a path to
pg_bsd_indent or such? But I guess we can address that later.



Independent of this specific patch: You seem to be generating your patch
series by invoking git show and redirecting that to a file? How do you apply a
series of such patches, while maintaining the commit messages? When git
format-patch is used, I can just use git am, but that doesn't work with your
patch series.

Greetings,

Andres Freund

  5. Re: Importing pg_bsd_indent into our source tree

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-09T22:12:52Z

    Andres Freund <andres@anarazel.de> writes:
> Did that in the attached.

Thanks.

> I didn't convert the test though, due to the duplicating it'd create. Perhaps
> we should just move it to a shell script? Or maybe it just doesn't matter
> enough to bother with?

We could move it to a shell script perhaps, but that seems pretty
low-priority.

> It doesn't build as-is with msvc, but does build with mingw. Failure:
> https://cirrus-ci.com/task/6290206869946368?logs=build#L1573

Thanks, I'll take a look at these things.

> To we really want to require users to install pg_bsd_indent into PATH? Seems
> like we ought to have a build target to invoke pgindent with a path to
> pg_bsd_indent or such? But I guess we can address that later.

For the moment I was just interested in maintaining the current workflow.
I know people muttered about having some sort of build target that'd
indent the whole tree from scratch after building pg_bsd_indent, but it's
not very clear to me how that'd work with e.g. VPATH configurations.

(I think you can already tell pgindent to use a specific pg_bsd_indent,
if your gripe is just about wanting to use a prebuilt copy that you
don't want to keep in PATH for some reason.)

> Independent of this specific patch: You seem to be generating your patch
> series by invoking git show and redirecting that to a file?

Yeah, it's pretty low-tech.  I'm not in the habit of posting multi-patch
series very often, so I haven't really bothered to use format-patch.
(I gave up on "git am" long ago as being too fragile, and always
use good ol' "patch" to apply patches, so I don't think about things
like whether it'd automatically absorb commit messages.  I pretty much
never use anyone else's commit message verbatim anyway ...)

			regards, tom lane

  6. Re: Importing pg_bsd_indent into our source tree

    Andres Freund <andres@anarazel.de> — 2023-02-09T22:14:50Z

    Hi,

On 2023-02-09 13:55:32 -0800, Andres Freund wrote:
> ../src/tools/pg_bsd_indent/args.c(179): error C2065: 'PATH_MAX': undeclared identifier
> ../src/tools/pg_bsd_indent/args.c(179): error C2057: expected constant expression
> ../src/tools/pg_bsd_indent/args.c(179): error C2466: cannot allocate an array of constant size 0
> ../src/tools/pg_bsd_indent/args.c(179): error C2133: 'fname': unknown size
> ../src/tools/pg_bsd_indent/args.c(183): warning C4034: sizeof returns 0
> ../src/tools/pg_bsd_indent/args.c(185): warning C4034: sizeof returns 0
> [1557/2161] Compiling C object src/tools/pg_bsd_indent/pg_bsd_indent.exe.p/err.c.obj
> [1558/2161] Precompiling header src/interfaces/ecpg/ecpglib/libecpg.dll.p/meson_pch-c.c
> [1559/2161] Compiling C object src/tools/pg_bsd_indent/pg_bsd_indent.exe.p/indent.c.obj
> FAILED: src/tools/pg_bsd_indent/pg_bsd_indent.exe.p/indent.c.obj
> "cl" "-Isrc\tools/pg_bsd_indent\pg_bsd_indent.exe.p" "-Isrc\tools/pg_bsd_indent" "-I..\src\tools\pg_bsd_indent" "-Isrc\include" "-I..\src\include" "-Ic:\openssl\1.1\include" "-I..\src\include\port\win32" "-I..\src\include\port\win32_msvc" "/MDd" "/nologo" "/showIncludes" "/utf-8" "/W2" "/Od" "/Zi" "/DWIN32" "/DWINDOWS" "/D__WINDOWS__" "/D__WIN32__" "/D_CRT_SECURE_NO_DEPRECATE" "/D_CRT_NONSTDC_NO_DEPRECATE" "/wd4018" "/wd4244" "/wd4273" "/wd4101" "/wd4102" "/wd4090" "/wd4267" "/Fdsrc\tools/pg_bsd_indent\pg_bsd_indent.exe.p\indent.c.pdb" /Fosrc/tools/pg_bsd_indent/pg_bsd_indent.exe.p/indent.c.obj "/c" ../src/tools/pg_bsd_indent/indent.c
> ../src/tools/pg_bsd_indent/indent.c(63): error C2065: 'MAXPATHLEN': undeclared identifier
> ../src/tools/pg_bsd_indent/indent.c(63): error C2057: expected constant expression
> ../src/tools/pg_bsd_indent/indent.c(63): error C2466: cannot allocate an array of constant size 0
> 
> This specific issue at least should be easily fixable.

The trivial fix of using MAXPGPATH made it build, without warnings. That
doesn't say anything about actually working. So I guess porting the test would
make sense.

Opinions on whether it would make sense as a shell script?

Greetings,

Andres Freund

  7. Re: Importing pg_bsd_indent into our source tree

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-09T22:19:22Z

    Andres Freund <andres@anarazel.de> writes:
> The trivial fix of using MAXPGPATH made it build, without warnings. That
> doesn't say anything about actually working. So I guess porting the test would
> make sense.

> Opinions on whether it would make sense as a shell script?

Hmmm .. a shell script would be fine by me, but it won't help in
testing a Windows build.  Maybe we need to make it a Perl script?

BTW, the attachments to your previous message are identical to what
I previously posted --- did you attach the wrong set of diffs?

			regards, tom lane

  8. Re: Importing pg_bsd_indent into our source tree

    Andres Freund <andres@anarazel.de> — 2023-02-09T23:10:13Z

    Hi,

On 2023-02-09 17:19:22 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > The trivial fix of using MAXPGPATH made it build, without warnings. That
> > doesn't say anything about actually working. So I guess porting the test would
> > make sense.
> 
> > Opinions on whether it would make sense as a shell script?
> 
> Hmmm .. a shell script would be fine by me, but it won't help in
> testing a Windows build.  Maybe we need to make it a Perl script?

At least for casual testing a shell script actually mostly works, due to git
it's easy enough to have a sh.exe around... Not something I'd necessarily want
to make a hard dependency, but for something like this it might suffice.  Of
course perl would be more dependable...


> BTW, the attachments to your previous message are identical to what
> I previously posted --- did you attach the wrong set of diffs?

I attached an extra patch, in addition to yours. I also attached yours so that
cfbot could continue to work, if you registered this.

Greetings,

Andres Freund

  9. Re: Importing pg_bsd_indent into our source tree

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-09T23:19:06Z

    Andres Freund <andres@anarazel.de> writes:
> On 2023-02-09 17:19:22 -0500, Tom Lane wrote:
>> Hmmm .. a shell script would be fine by me, but it won't help in
>> testing a Windows build.  Maybe we need to make it a Perl script?

> At least for casual testing a shell script actually mostly works, due to git
> it's easy enough to have a sh.exe around... Not something I'd necessarily want
> to make a hard dependency, but for something like this it might suffice.  Of
> course perl would be more dependable...

Yeah, also less question about whether it works on Windows.
I'll see about moving that into Perl.  It's short enough.

>> BTW, the attachments to your previous message are identical to what
>> I previously posted --- did you attach the wrong set of diffs?

> I attached an extra patch, in addition to yours.

D'oh, I didn't notice that :-(

> I also attached yours so that
> cfbot could continue to work, if you registered this.

I thought about registering it, but that won't teach us anything unless
we make it built-by-default, which was not my intention.  I guess we
could temporarily include it in the build.

			regards, tom lane

  10. Re: Importing pg_bsd_indent into our source tree

    Andres Freund <andres@anarazel.de> — 2023-02-10T00:08:41Z

    Hi,

On 2023-02-09 18:19:06 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2023-02-09 17:19:22 -0500, Tom Lane wrote:
> >> Hmmm .. a shell script would be fine by me, but it won't help in
> >> testing a Windows build.  Maybe we need to make it a Perl script?
> 
> > At least for casual testing a shell script actually mostly works, due to git
> > it's easy enough to have a sh.exe around... Not something I'd necessarily want
> > to make a hard dependency, but for something like this it might suffice.  Of
> > course perl would be more dependable...
> 
> Yeah, also less question about whether it works on Windows.
> I'll see about moving that into Perl.  It's short enough.

Cool.


> > I also attached yours so that
> > cfbot could continue to work, if you registered this.
> 
> I thought about registering it, but that won't teach us anything unless
> we make it built-by-default, which was not my intention.  I guess we
> could temporarily include it in the build.

The meson patch I sent did build it by default, that's why I saw the windows
failure and the freebsd warnings. If we don't want that, we'd need to add
  build_by_default: false

I'm fine either way. It's barely noticeable compared to the rest of postgres.

Greetings,

Andres Freund

  11. Re: Importing pg_bsd_indent into our source tree

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-10T00:20:37Z

    Andres Freund <andres@anarazel.de> writes:
> On 2023-02-09 18:19:06 -0500, Tom Lane wrote:
>> I thought about registering it, but that won't teach us anything unless
>> we make it built-by-default, which was not my intention.  I guess we
>> could temporarily include it in the build.

> The meson patch I sent did build it by default, that's why I saw the windows
> failure and the freebsd warnings. If we don't want that, we'd need to add
>   build_by_default: false

> I'm fine either way. It's barely noticeable compared to the rest of postgres.

Yeah, build-by-default isn't really a big deal.  Install-by-default
is more of a problem...

			regards, tom lane

  12. Re: Importing pg_bsd_indent into our source tree

    Andres Freund <andres@anarazel.de> — 2023-02-10T00:35:55Z

    On 2023-02-09 19:20:37 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I'm fine either way. It's barely noticeable compared to the rest of postgres.
> 
> Yeah, build-by-default isn't really a big deal.  Install-by-default
> is more of a problem...

Perhaps we should install it, just not in bin/, but alongside pgxs/, similar
to pg_regress et al?

  13. Re: Importing pg_bsd_indent into our source tree

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-10T00:42:13Z

    Andres Freund <andres@anarazel.de> writes:
> Perhaps we should install it, just not in bin/, but alongside pgxs/, similar
> to pg_regress et al?

For my own purposes, I really don't want it anywhere in the --prefix
tree.  That's not necessarily present when I'm using the program.

(Hmm, clarify: it wouldn't matter if install sticks it under pgxs,
but I don't want to be forced to use it from there.)

			regards, tom lane

  14. Re: Importing pg_bsd_indent into our source tree

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-11T23:54:00Z

    Here's a v3 of this patchset, incorporating your meson fixes as
well as patches for the portability problems you noted.

I ended up converting the test infrastructure into a TAP test,
which kind of feels like overkill; but the Meson system doesn't
seem to provide any lower-overhead way to run a test.

I've not touched the issue of whether and where to install
pg_bsd_indent; for now, neither build system will do so.

Also, for now both build systems *will* run tests on it,
although I'm not sure if plugging it into "make check-world"
is enough to cause the cfbot to do so, and I'm pretty sure
that the buildfarm won't notice that.

I'll let the cfbot loose on this, and if it runs the tests
successfully I plan to go ahead and push.  We can resolve
the installation question later.  We might want to back off
testing too once we're satisfied about portability.

(I left out the 0004 declaration-formatting patch for now, btw.)

			regards, tom lane

  15. Re: Importing pg_bsd_indent into our source tree

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-12T00:33:15Z

    I wrote:
> I'll let the cfbot loose on this, and if it runs the tests
> successfully I plan to go ahead and push.

cfbot didn't like that ...

			regards, tom lane

  16. Re: Importing pg_bsd_indent into our source tree

    Andres Freund <andres@anarazel.de> — 2023-02-12T00:42:54Z

    Hi,

On 2023-02-11 18:54:00 -0500, Tom Lane wrote:
> I ended up converting the test infrastructure into a TAP test,
> which kind of feels like overkill; but the Meson system doesn't
> seem to provide any lower-overhead way to run a test.

FWIW, The default way to indicate failures in a test is the exit
code. Obviously that allows less detailed reporting, but other than that, it
works (that's how we test pg_regress today).


> Also, for now both build systems *will* run tests on it,
> although I'm not sure if plugging it into "make check-world"
> is enough to cause the cfbot to do so, and I'm pretty sure
> that the buildfarm won't notice that.

That's sufficient for cfbot, on the CI task still using autoconf. And for
meson it'll also suffice.

It actually already ran:
https://cirrus-ci.com/build/5984572702195712

The windows test failure is a transient issue independent of the patch
(something went wrong with image permissions). However the linux autoconf one
isn't:
https://api.cirrus-ci.com/v1/artifact/task/5482952532951040/log/src/tools/pg_bsd_indent/tmp_check/log/regress_log_001_pg_bsd_indent

# Running: pg_bsd_indent --version
Command 'pg_bsd_indent' not found in /tmp/cirrus-ci-build/tmp_install/usr/local/pgsql/bin, /tmp/cirrus-ci-build/src/tools/pg_bsd_indent, /usr/local/sbin, /usr/local/bin, /usr/sbin, /usr/bin, /sbin, /bin at /tmp/cirrus-ci-build/src/tools/pg_bsd_indent/../../../src/test/perl/PostgreSQL/Test/Utils.pm line 832.

I guess there might be a missing dependency? PATH looks sufficient.


> I'll let the cfbot loose on this, and if it runs the tests
> successfully I plan to go ahead and push.  We can resolve
> the installation question later.  We might want to back off
> testing too once we're satisfied about portability.

> (I left out the 0004 declaration-formatting patch for now, btw.)

Makes sense.

Greetings,

Andres Freund

  17. Re: Importing pg_bsd_indent into our source tree

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-12T01:04:11Z

    Andres Freund <andres@anarazel.de> writes:
> # Running: pg_bsd_indent --version
> Command 'pg_bsd_indent' not found in /tmp/cirrus-ci-build/tmp_install/usr/local/pgsql/bin, /tmp/cirrus-ci-build/src/tools/pg_bsd_indent, /usr/local/sbin, /usr/local/bin, /usr/sbin, /usr/bin, /sbin, /bin at /tmp/cirrus-ci-build/src/tools/pg_bsd_indent/../../../src/test/perl/PostgreSQL/Test/Utils.pm line 832.

> I guess there might be a missing dependency? PATH looks sufficient.

Yeah, I expected that "check" would have a dependency on "all",
but apparently it doesn't (and I'd missed this because I had
pg_bsd_indent installed elsewhere in my PATH :-().  New build
running now.

			regards, tom lane

  18. Re: Importing pg_bsd_indent into our source tree

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-12T01:32:30Z

    Hmmm ... ci autoconf build is now happy, but the Windows run complains
that none of the output files match.  I'm betting that this is a
Windows-newline problem, since I now see that indent.c opens both the
input and output files in default (text) mode.  I'm inclined to
change it to open the output file in binary mode while leaving the
input in text, which should have the effect of stripping \r if it's
present.

			regards, tom lane

  19. Re: Importing pg_bsd_indent into our source tree

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-02-12T01:43:52Z

    I wrote:
> Hmmm ... ci autoconf build is now happy, but the Windows run complains
> that none of the output files match.  I'm betting that this is a
> Windows-newline problem, since I now see that indent.c opens both the
> input and output files in default (text) mode.  I'm inclined to
> change it to open the output file in binary mode while leaving the
> input in text, which should have the effect of stripping \r if it's
> present.

So let's see if that theory is correct at all ...

			regards, tom lane

  20. Re: Importing pg_bsd_indent into our source tree

    Thomas Munro <thomas.munro@gmail.com> — 2023-02-12T02:14:37Z

    On Sun, Feb 12, 2023 at 2:44 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > Hmmm ... ci autoconf build is now happy, but the Windows run complains
> > that none of the output files match.  I'm betting that this is a
> > Windows-newline problem, since I now see that indent.c opens both the
> > input and output files in default (text) mode.  I'm inclined to
> > change it to open the output file in binary mode while leaving the
> > input in text, which should have the effect of stripping \r if it's
> > present.
>
> So let's see if that theory is correct at all ...

(Since I happened to be tinkering on cfbot while you posted these, I
noticed that cfbot took over 50 minutes to start processing the v4.
The problem was upstream: the time in the second-last column of
https://commitfest.postgresql.org/42/ didn't change for that whole
time, even though the archives had your new email.  Cf castles, sand;
I should probably get a better trigger mechanism :-)  I like that page
because it lets me poll one single end point once per minute to learn
about changes across all threads, but I am not sure what sort of
technology connects the archives to the CF app, and how it can fail.)

  21. Re: Importing pg_bsd_indent into our source tree

    Alexander Law <exclusion@gmail.com> — 2023-06-07T08:00:00Z

    Hello,

I've discovered that the pg_bsd_indent test added here makes an
ASAN-instrumented build fail on:
ASAN_OPTIONS=detect_leaks=0:strict_string_checks=1 make check-world
as follows:
# +++ tap check in src/tools/pg_bsd_indent +++
t/001_pg_bsd_indent.pl .. 1/?
#   Failed test 'pg_bsd_indent succeeds on binary'
#   at t/001_pg_bsd_indent.pl line 41.

#   Failed test 'pg_bsd_indent output matches for binary'
#   at t/001_pg_bsd_indent.pl line 50.

#   Failed test 'pg_bsd_indent succeeds on comments'
#   at t/001_pg_bsd_indent.pl line 41.

#   Failed test 'pg_bsd_indent output matches for comments'
#   at t/001_pg_bsd_indent.pl line 50.
t/001_pg_bsd_indent.pl .. 10/?
...
regress_log_001_pg_bsd_indent contains:
# Running: pg_bsd_indent .../src/tools/pg_bsd_indent/tests/binary.0 binary.out 
-P.../src/tools/pg_bsd_indent/tests/binary.pro
=================================================================
==2124067==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60600000005a at pc 0x560aa8972dae bp 
0x7ffebd080460 sp 0x7ffebd07fc08
READ of size 40 at 0x60600000005a thread T0
     #0 0x560aa8972dad in __interceptor_strspn.part.0 (.../src/tools/pg_bsd_indent/pg_bsd_indent+0x5edad)
     #1 0x560aa8a3f495 in lexi .../src/tools/pg_bsd_indent/lexi.c:258
     #2 0x560aa8a3451d in main .../src/tools/pg_bsd_indent/indent.c:269
     #3 0x7f6ffab1ad8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
     #4 0x7f6ffab1ae3f in __libc_start_main_impl ../csu/libc-start.c:392
     #5 0x560aa894b7d4 in _start (.../src/tools/pg_bsd_indent/pg_bsd_indent+0x377d4)

0x60600000005a is located 0 bytes to the right of 58-byte region [0x606000000020,0x60600000005a)
allocated by thread T0 here:
     #0 0x560aa89e4980 in __interceptor_realloc.part.0 (.../src/tools/pg_bsd_indent/pg_bsd_indent+0xd0980)
     #1 0x560aa8a3d24e in fill_buffer .../src/tools/pg_bsd_indent/io.c:365

SUMMARY: AddressSanitizer: heap-buffer-overflow (.../src/tools/pg_bsd_indent/pg_bsd_indent+0x5edad) in 
__interceptor_strspn.part.0
...

I understand that that code is almost as ancient as me and it works as
intended (fill_buffer() doesn't null-terminate a buffer), but may be this is
worth fixing in the Postgres source tree to keep the whole testing baseline
high (if someone finds strict_string_checks useful). If so, perhaps
something like the attached will do.

Best regards,
Alexander