Horton

Thread

Commits

  1. Be more careful to avoid including system headers after perl.h

  2. Use SSE2 in is_valid_ascii() where available.

  1. windows cfbot failing: my_perl

    Justin Pryzby <pryzby@telsasoft.com> — 2022-08-26T11:55:46Z

    The last 20 some consecutive builds failed:
https://cirrus-ci.com/github/postgresql-cfbot/postgresql

like this:
[09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl': undeclared identifier (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj]
[09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl': undeclared identifier (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj]
[09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: left of '->IMem' must point to struct/union (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj]
[09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: left of '->IMem' must point to struct/union (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj]

I imagine it may be due to an error hit while rebuilding the ci's docker image.

-- 
Justin

  2. Re: windows cfbot failing: my_perl

    Andres Freund <andres@anarazel.de> — 2022-08-26T13:21:51Z

    Hi,

On 2022-08-26 06:55:46 -0500, Justin Pryzby wrote:
> The last 20 some consecutive builds failed:
> https://cirrus-ci.com/github/postgresql-cfbot/postgresql
> 
> like this:
> [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl': undeclared identifier (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj]
> [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl': undeclared identifier (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj]
> [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: left of '->IMem' must point to struct/union (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj]
> [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: left of '->IMem' must point to struct/union (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj]
> 
> I imagine it may be due to an error hit while rebuilding the ci's docker image.

I don't think it's CI specific, see
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hamerkop&dt=2022-08-26%2011%3A00%3A11

Looks like the failures might have started with
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=121d2d3d70ecdb2113b340c5f3b99a61341291af
based on
https://cirrus-ci.com/github/postgres/postgres/

Not immediately obvious why that would be.

Greetings,

Andres Freund

  3. Re: windows cfbot failing: my_perl

    Andres Freund <andres@anarazel.de> — 2022-08-26T13:40:47Z

    Hi,

On 2022-08-26 06:21:51 -0700, Andres Freund wrote:
> On 2022-08-26 06:55:46 -0500, Justin Pryzby wrote:
> > The last 20 some consecutive builds failed:
> > https://cirrus-ci.com/github/postgresql-cfbot/postgresql
> > 
> > like this:
> > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl': undeclared identifier (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj]
> > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl': undeclared identifier (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj]
> > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: left of '->IMem' must point to struct/union (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj]
> > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: left of '->IMem' must point to struct/union (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj]
> > 
> > I imagine it may be due to an error hit while rebuilding the ci's docker image.
> 
> I don't think it's CI specific, see
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hamerkop&dt=2022-08-26%2011%3A00%3A11
> 
> Looks like the failures might have started with
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=121d2d3d70ecdb2113b340c5f3b99a61341291af
> based on
> https://cirrus-ci.com/github/postgres/postgres/
> 
> Not immediately obvious why that would be.

Reproduces in a VM, it starts to fail with that commit. Looks like somehow
different macros are trampling on each other. Something in perl is interfering
with msvc's malloc.h, turning

            if (_Marker == _ALLOCA_S_HEAP_MARKER)
            {
                free(_Memory);
            }
into

            if (_Marker == 0xDDDD)
            {
                (*(my_perl->IMem)->pFree)((my_perl->IMem), (_Memory));
            }

after preprocessing. No idea how.

Greetings,

Andres Freund

  4. Re: windows cfbot failing: my_perl

    Andres Freund <andres@anarazel.de> — 2022-08-26T14:27:00Z

    Hi,

On 2022-08-26 06:40:47 -0700, Andres Freund wrote:
> On 2022-08-26 06:21:51 -0700, Andres Freund wrote:
> > On 2022-08-26 06:55:46 -0500, Justin Pryzby wrote:
> > > The last 20 some consecutive builds failed:
> > > https://cirrus-ci.com/github/postgresql-cfbot/postgresql
> > > 
> > > like this:
> > > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl': undeclared identifier (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj]
> > > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2065: 'my_perl': undeclared identifier (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj]
> > > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: left of '->IMem' must point to struct/union (compiling source file src/pl/plperl/SPI.c) [c:\cirrus\plperl.vcxproj]
> > > [09:29:27.711] C:\Program Files (x86)\Windows Kits\10\Include\10.0.20348.0\ucrt\malloc.h(159,17): error C2223: left of '->IMem' must point to struct/union (compiling source file src/pl/plperl/Util.c) [c:\cirrus\plperl.vcxproj]
> > > 
> > > I imagine it may be due to an error hit while rebuilding the ci's docker image.
> > 
> > I don't think it's CI specific, see
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hamerkop&dt=2022-08-26%2011%3A00%3A11
> > 
> > Looks like the failures might have started with
> > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=121d2d3d70ecdb2113b340c5f3b99a61341291af
> > based on
> > https://cirrus-ci.com/github/postgres/postgres/
> > 
> > Not immediately obvious why that would be.
> 
> Reproduces in a VM, it starts to fail with that commit. Looks like somehow
> different macros are trampling on each other. Something in perl is interfering
> with msvc's malloc.h, turning
> 
>             if (_Marker == _ALLOCA_S_HEAP_MARKER)
>             {
>                 free(_Memory);
>             }
> into
> 
>             if (_Marker == 0xDDDD)
>             {
>                 (*(my_perl->IMem)->pFree)((my_perl->IMem), (_Memory));
>             }
> 
> after preprocessing. No idea how.

Because perl, extremely unhelpfully, #defines free. Which, not surprisingly,
causes issues when including system headers referencing free as well.

I don't really see a good solution to this other than hoisting the
mb/pg_wchar.h include out to before we include all the perl stuff. That does
fix the issue.

Greetings,

Andres Freund

  5. Re: windows cfbot failing: my_perl

    John Naylor <john.naylor@enterprisedb.com> — 2022-08-26T14:39:05Z

    On Fri, Aug 26, 2022 at 9:27 PM Andres Freund <andres@anarazel.de> wrote:
>
> Because perl, extremely unhelpfully, #defines free. Which, not surprisingly,
> causes issues when including system headers referencing free as well.
>
> I don't really see a good solution to this other than hoisting the
> mb/pg_wchar.h include out to before we include all the perl stuff. That does
> fix the issue.

We could also move is_valid_ascii somewhere else. It's only
tangentially related to "wide chars" anyway.

-- 
John Naylor
EDB: http://www.enterprisedb.com

  6. Re: windows cfbot failing: my_perl

    Andres Freund <andres@anarazel.de> — 2022-08-26T14:47:40Z

    Hi,

On 2022-08-26 21:39:05 +0700, John Naylor wrote:
> On Fri, Aug 26, 2022 at 9:27 PM Andres Freund <andres@anarazel.de> wrote:
> >
> > Because perl, extremely unhelpfully, #defines free. Which, not surprisingly,
> > causes issues when including system headers referencing free as well.
> >
> > I don't really see a good solution to this other than hoisting the
> > mb/pg_wchar.h include out to before we include all the perl stuff. That does
> > fix the issue.
> 
> We could also move is_valid_ascii somewhere else. It's only
> tangentially related to "wide chars" anyway.

Given the crazy defines of stuff like free, it seems like a good idea to have
a rule that no headers should be included after plperl.h with
PG_NEED_PERL_XSUB_H defined.  It's not like there's not other chances of of
pulling in malloc.h from within pg_wchar.h somehow.

It's a bit ugly to have the mb/pg_wchar.h in plperl.h instead of
plperl_helpers.h, but ...

Greetings,

Andres Freund

  7. Re: windows cfbot failing: my_perl

    Andres Freund <andres@anarazel.de> — 2022-08-26T20:28:30Z

    Hi,

Tom, Ilmari, you seem to have hacked on this stuff most (not so) recently. Do
you have a better suggestion than moving the mb/pg_wchar.h include out of
plperl_helpers.h as I suggest below?

On 2022-08-26 07:47:40 -0700, Andres Freund wrote:
> On 2022-08-26 21:39:05 +0700, John Naylor wrote:
> > On Fri, Aug 26, 2022 at 9:27 PM Andres Freund <andres@anarazel.de> wrote:
> > >
> > > Because perl, extremely unhelpfully, #defines free. Which, not surprisingly,
> > > causes issues when including system headers referencing free as well.
> > >
> > > I don't really see a good solution to this other than hoisting the
> > > mb/pg_wchar.h include out to before we include all the perl stuff. That does
> > > fix the issue.
> >
> > We could also move is_valid_ascii somewhere else. It's only
> > tangentially related to "wide chars" anyway.
>
> Given the crazy defines of stuff like free, it seems like a good idea to have
> a rule that no headers should be included after plperl.h with
> PG_NEED_PERL_XSUB_H defined.  It's not like there's not other chances of of
> pulling in malloc.h from within pg_wchar.h somehow.
>
> It's a bit ugly to have the mb/pg_wchar.h in plperl.h instead of
> plperl_helpers.h, but ...

Greetings,

Andres Freund

  8. Re: windows cfbot failing: my_perl

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-26T20:32:15Z

    Andres Freund <andres@anarazel.de> writes:
> Tom, Ilmari, you seem to have hacked on this stuff most (not so) recently. Do
> you have a better suggestion than moving the mb/pg_wchar.h include out of
> plperl_helpers.h as I suggest below?

I agree with the conclusion that we'd better #include all our own
headers before any of Perl's.  No strong opinions about which
rearrangement is least ugly --- but let's add some comments about
that requirement.

			regards, tom lane

  9. Re: windows cfbot failing: my_perl

    Andrew Dunstan <andrew@dunslane.net> — 2022-08-26T21:05:52Z

    On 2022-08-26 Fr 10:47, Andres Freund wrote:
> Hi,
>
> On 2022-08-26 21:39:05 +0700, John Naylor wrote:
>> On Fri, Aug 26, 2022 at 9:27 PM Andres Freund <andres@anarazel.de> wrote:
>>> Because perl, extremely unhelpfully, #defines free. Which, not surprisingly,
>>> causes issues when including system headers referencing free as well.
>>>
>>> I don't really see a good solution to this other than hoisting the
>>> mb/pg_wchar.h include out to before we include all the perl stuff. That does
>>> fix the issue.
>> We could also move is_valid_ascii somewhere else. It's only
>> tangentially related to "wide chars" anyway.
> Given the crazy defines of stuff like free, it seems like a good idea to have
> a rule that no headers should be included after plperl.h with
> PG_NEED_PERL_XSUB_H defined.  It's not like there's not other chances of of
> pulling in malloc.h from within pg_wchar.h somehow.
>
> It's a bit ugly to have the mb/pg_wchar.h in plperl.h instead of
> plperl_helpers.h, but ...
>


It's already included directly in plperl.c, so couldn't we just lift it
directly into SPI.xs and Util.xs?


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  10. Re: windows cfbot failing: my_perl

    Andres Freund <andres@anarazel.de> — 2022-08-26T21:15:07Z

    Hi,

On 2022-08-26 17:05:52 -0400, Andrew Dunstan wrote:
> On 2022-08-26 Fr 10:47, Andres Freund wrote:
> > Given the crazy defines of stuff like free, it seems like a good idea to have
> > a rule that no headers should be included after plperl.h with
> > PG_NEED_PERL_XSUB_H defined.  It's not like there's not other chances of of
> > pulling in malloc.h from within pg_wchar.h somehow.
> >
> > It's a bit ugly to have the mb/pg_wchar.h in plperl.h instead of
> > plperl_helpers.h, but ...

> It's already included directly in plperl.c, so couldn't we just lift it
> directly into SPI.xs and Util.xs?

I think it'd also be needed in hstore_plperl.c, jsonb_plperl.c. Putting the
include in plperl.h would keep that aspect transparent, because plperl_utils.h
includes plperl.h.

I don't think manually including all dependencies, even if it's just one, in
each of the six files currently using plperl_utils.h is a good approach.

Greetings,

Andres Freund

  11. Re: windows cfbot failing: my_perl

    John Naylor <john.naylor@enterprisedb.com> — 2022-08-27T02:32:45Z

    On Sat, Aug 27, 2022 at 4:15 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2022-08-26 17:05:52 -0400, Andrew Dunstan wrote:
> > On 2022-08-26 Fr 10:47, Andres Freund wrote:
> > > Given the crazy defines of stuff like free, it seems like a good idea to have
> > > a rule that no headers should be included after plperl.h with
> > > PG_NEED_PERL_XSUB_H defined.  It's not like there's not other chances of of
> > > pulling in malloc.h from within pg_wchar.h somehow.
> > >
> > > It's a bit ugly to have the mb/pg_wchar.h in plperl.h instead of
> > > plperl_helpers.h, but ...
>
> > It's already included directly in plperl.c, so couldn't we just lift it
> > directly into SPI.xs and Util.xs?
>
> I think it'd also be needed in hstore_plperl.c, jsonb_plperl.c. Putting the
> include in plperl.h would keep that aspect transparent, because plperl_utils.h
> includes plperl.h.

Since plperl_helpers.h already includes plperl.h, I'm not sure why
both are included everywhere the former is. If .c/.xs files didn't
include plperl.h directly, we could keep pg_wchar.h in
plperl_helpers.h. Not sure if that's workable or any better...

-- 
John Naylor
EDB: http://www.enterprisedb.com

  12. Re: windows cfbot failing: my_perl

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-27T03:02:06Z

    John Naylor <john.naylor@enterprisedb.com> writes:
> On Sat, Aug 27, 2022 at 4:15 AM Andres Freund <andres@anarazel.de> wrote:
>> I think it'd also be needed in hstore_plperl.c, jsonb_plperl.c. Putting the
>> include in plperl.h would keep that aspect transparent, because plperl_utils.h
>> includes plperl.h.

> Since plperl_helpers.h already includes plperl.h, I'm not sure why
> both are included everywhere the former is. If .c/.xs files didn't
> include plperl.h directly, we could keep pg_wchar.h in
> plperl_helpers.h. Not sure if that's workable or any better...

Maybe we should flush the separate plperl_helpers.h header and just
put those static-inline functions in plperl.h.

			regards, tom lane

  13. Re: windows cfbot failing: my_perl

    John Naylor <john.naylor@enterprisedb.com> — 2022-08-27T04:20:29Z

    On Sat, Aug 27, 2022 at 10:02 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> John Naylor <john.naylor@enterprisedb.com> writes:
> > On Sat, Aug 27, 2022 at 4:15 AM Andres Freund <andres@anarazel.de> wrote:
> >> I think it'd also be needed in hstore_plperl.c, jsonb_plperl.c. Putting the
> >> include in plperl.h would keep that aspect transparent, because plperl_utils.h
> >> includes plperl.h.
>
> > Since plperl_helpers.h already includes plperl.h, I'm not sure why
> > both are included everywhere the former is. If .c/.xs files didn't
> > include plperl.h directly, we could keep pg_wchar.h in
> > plperl_helpers.h. Not sure if that's workable or any better...
>
> Maybe we should flush the separate plperl_helpers.h header and just
> put those static-inline functions in plperl.h.

Here's a patch with that idea, not tested on Windows yet.

-- 
John Naylor
EDB: http://www.enterprisedb.com

  14. Re: windows cfbot failing: my_perl

    John Naylor <john.naylor@enterprisedb.com> — 2022-08-27T05:53:24Z

    On Sat, Aug 27, 2022 at 11:20 AM John Naylor
<john.naylor@enterprisedb.com> wrote:
>
> Here's a patch with that idea, not tested on Windows yet.

Update: I tried taking the CI for a spin, but ran into IT issues with
Github when I tried to push my branch to remote.

-- 
John Naylor
EDB: http://www.enterprisedb.com

  15. Re: windows cfbot failing: my_perl

    Andres Freund <andres@anarazel.de> — 2022-08-27T07:11:13Z

    On 2022-08-26 23:02:06 -0400, Tom Lane wrote:
> John Naylor <john.naylor@enterprisedb.com> writes:
> > On Sat, Aug 27, 2022 at 4:15 AM Andres Freund <andres@anarazel.de> wrote:
> >> I think it'd also be needed in hstore_plperl.c, jsonb_plperl.c. Putting the
> >> include in plperl.h would keep that aspect transparent, because plperl_utils.h
> >> includes plperl.h.
> 
> > Since plperl_helpers.h already includes plperl.h, I'm not sure why
> > both are included everywhere the former is. If .c/.xs files didn't
> > include plperl.h directly, we could keep pg_wchar.h in
> > plperl_helpers.h. Not sure if that's workable or any better...
> 
> Maybe we should flush the separate plperl_helpers.h header and just
> put those static-inline functions in plperl.h.

+1

  16. Re: windows cfbot failing: my_perl

    Andres Freund <andres@anarazel.de> — 2022-08-27T07:23:44Z

    Hi,

On 2022-08-27 12:53:24 +0700, John Naylor wrote:
> On Sat, Aug 27, 2022 at 11:20 AM John Naylor
> <john.naylor@enterprisedb.com> wrote:
> >
> > Here's a patch with that idea, not tested on Windows yet.
> 
> Update: I tried taking the CI for a spin, but ran into IT issues with
> Github when I tried to push my branch to remote.

A github, not a CI issue? Just making sure...

As a workaround you can just open a CF entry, that'll run the patch soon.


But either way, I ran the patch "manually" in a windows VM that I had running
anyway. With the meson patchset, but I don't see how it could matter here.

1/5 postgresql:setup / tmp_install                          OK                1.30s
2/5 postgresql:jsonb_plperl / jsonb_plperl/regress          OK                8.30s
3/5 postgresql:bool_plperl / bool_plperl/regress            OK                8.30s
4/5 postgresql:hstore_plperl / hstore_plperl/regress        OK                8.64s
5/5 postgresql:plperl / plperl/regress                      OK               10.41s

Ok:                 5


I didn't test other platforms.


WRT the patch's commit message: The issue isn't that perl's free() is
redefined, it's that perl's #define free (which references perl globals!)
breaks windows' header...


Greetings,

Andres Freund

  17. Re: windows cfbot failing: my_perl

    John Naylor <john.naylor@enterprisedb.com> — 2022-08-27T07:36:09Z

    On Sat, Aug 27, 2022 at 2:23 PM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2022-08-27 12:53:24 +0700, John Naylor wrote:
> > Update: I tried taking the CI for a spin, but ran into IT issues with
> > Github when I tried to push my branch to remote.
>
> A github, not a CI issue? Just making sure...

Yeah, I forked PG from the Github page, cloned it locally, applied the
patch and tried to push to origin.

> As a workaround you can just open a CF entry, that'll run the patch soon.

Yeah, I did that after taking a break -- there are compiler warnings
for contrib/sepgsql/label.c where pfree's argument is cast to void *,
so seems unrelated.

> But either way, I ran the patch "manually" in a windows VM that I had running
> anyway. With the meson patchset, but I don't see how it could matter here.
>
> 1/5 postgresql:setup / tmp_install                          OK                1.30s
> 2/5 postgresql:jsonb_plperl / jsonb_plperl/regress          OK                8.30s
> 3/5 postgresql:bool_plperl / bool_plperl/regress            OK                8.30s
> 4/5 postgresql:hstore_plperl / hstore_plperl/regress        OK                8.64s
> 5/5 postgresql:plperl / plperl/regress                      OK               10.41s
>
> Ok:                 5
>
>
> I didn't test other platforms.
>
>
> WRT the patch's commit message: The issue isn't that perl's free() is
> redefined, it's that perl's #define free (which references perl globals!)
> breaks windows' header...

Ah, thanks for that detail and for testing, will push.

-- 
John Naylor
EDB: http://www.enterprisedb.com