RE: AIX support - Mailing list pgsql-hackers

Hi Heikki,

Please find the attached patch addressing the provided comments. Our responses are outlined below.

>> - src/backend/port/aix/mkldexport.sh>

> Oh, that's interesting. So if we do that, we don't need mkldepxort.sh
> anymore?

Here the better approach still would be to use the script to extract the

symbols build the corresponding shared libraries.  As we see there are

lot of extensions that are getting built using the symbols exported from

"postges" binary, we need to definitely  use the script to extract the symbols

and import them while building the library extensions.

>> - WRT to the MEMSET_LOOP_LIMIT flag, this is set to “0”, which would
>> internally use

> Yes, I understand what it does. But why? Whatever benchmarking was done
> back in 2006 by is no longer relevant.

We ran the program , mentioned in the below link and collected the

benchmark stats on our node (POWER_10).

    https://postgrespro.com/list/thread-id/1673194

The native AIX memset() seems to performs better. The benchmark seems to be still

relevant, so I think we should continue to use the existing optimization for

AIX. Below are the stats (64bit Object mode).

>> ./memset-aix

        sizeof(int)  = 4

        sizeof(long) = 8

        Loop by int (size=8) : 0.238024

        memset by int (size=8) : 0.280301

        Loop by long (size=8) : 0.202650

        Loop by int (size=16) : 0.389846

        memset by int (size=16) : 0.280979

        Loop by long (size=16) : 0.246879

        Loop by int (size=32) : 0.773478

        memset by int (size=32) : 0.331691

        Loop by long (size=32) : 0.422261

        Loop by int (size=64) : 1.945150

        memset by int (size=64) : 0.319117

        Loop by long (size=64) : 0.769770

        Loop by int (size=128) : 4.517186

        memset by int (size=128) : 0.398292

        Loop by long (size=128) : 1.982307

        Loop by int (size=256) : 12.486661

        memset by int (size=256) : 0.463115

        Loop by long (size=256) : 4.526272

        Loop by int (size=512) : 24.433075

        memset by int (size=512) : 0.681426

        Loop by long (size=512) : 12.564701

        Loop by int (size=1024) : 48.060486

        memset by int (size=1024) : 0.904048

        Loop by long (size=1024) : 24.149871

        Loop by int (size=2048) : 96.288018

        memset by int (size=2048) : 1.509465

        Loop by long (size=2048) : 48.216079

        Loop by int (size=4096) : 191.847177

        memset by int (size=4096) : 1.435403

        Loop by long (size=4096) : 96.433072

>>  12 files changed, 224 insertions(+), 20 deletions(-)
> I'm glad to see this patch shrinking :-)

Thanks for your suggestions, yes it further shrinked.

> > diff --git a/src/include/port/aix.h b/src/include/port/aix.h

> Useless.

I too initially felt this was not needed, This file is still required as the below file is

getting auto generated. I attempted to remove it; the below error is hit.

so this is still required.

     64 ../../src/include/c.h:59:10: fatal error: pg_config_os.h: No such file or directory

     65    59 | #include "pg_config_os.h"               /* config from include/port/PORTNAME.h */

     66       |          ^~~~~~~~~~~~~~~~

> diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h

> Why is this change needed?

> Yes, I know we've been over this many times already. I still don't
> understand why it's needed. The onus is on you to explain it adequately,
> in comments in the patch, so that I and others understand it. Or even
> better, remove it if it's not necessary.

If you recall, we previously considered replacing this assembly code with

__sync_lock_test_and_set(). However, as you mentioned earlier, this should be

handled in a separate patch. For now, I'll make a note and submit a separate

patch for this later, as originally planned. Below is the reference to older

discussion.

https://www.postgresql.org/message-id/7751f9c5-e2e6-4252-a9fa-14b3e78ddec9%40iki.fi

>> diff --git a/src/makefiles/Makefile.aix b/src/makefiles/Makefile.aix
>> +MAKE_EXPORTS= true

>Oh this is interesting. I think MAKE_EXPORTS is actually a leftover that
>I failed to remove when I removed the AIX support; it's not used on any
>currently-supported platform.

Removed it and its working fine.

>> +# -blibpath must contain ALL directories where we should look for libraries
>> +libpath := $(shell echo $(subst -L,:,$(filter -L/%,$(LDFLAGS))) | sed -e's/ //g'):/usr/lib:/lib

> Is this still sensible on modern AIX systems? What happens if you leave
> it out?

This is required as it is looking for the possible non-default directories for the linker

at the runtime. This is used along with rpath. As suggested, I tested this by removing

the libpath, but at run time the linker is not able to find the dependent libraries path

as a result, the binaries are not getting loaded. After doing some research, AIX uses a stricter,

more explicit approach. The runtime linker expects to tell it exactly where to look using -blibpath.

>> +# when building with gcc, need to make sure that libgcc can be found
>> +ifeq ($(GCC), yes)
>> +libpath := $(libpath):$(dir $(shell gcc -print-libgcc-file-name))
>> +endif

> Same here. Still relevant? What happens if you leave it out?

Removed it and it is working fine.

>> +# gcc needs to know it's building a shared lib, otherwise it'll not emit
>> +# correct code / link to the right support libraries
>> +ifeq ($(GCC), yes)
>> +LDFLAGS_SL += -shared
>> +endif

>On other platforms, we have this in LINK.shared, see src/Makefile.shlib.
>Should we do the same on AIX; if not, why not?

I tried your suggestion to move the gcc "LDFLAGS_SL += -shared" flag to Makefile.shlib,

"LINK.shared       = $(COMPILER) -shared", but there are some extensions dependent

on this in "contrib" which are not building with “-shared” flag, as a result a testcase

is failing to load this library "contrib/spi/autoinc.so" . Below is the error.

not ok 78    + triggers                                 2443 ms

31 +ERROR:  incompatible library "/home/pgdev/pgdb/postgres/src/test/regress/autoinc.so": missing magic block

32 +HINT:  Extension libraries are required to use the PG_MODULE_MAGIC macro.

I noticed that the “-shared” flag was missing while building the contrib/spi/autoinc.so library.

I tried to add the “include $(top_builddir)/src/Makefile.shlib” in the

contrib/spi/Makefile, but still the “-shared” flag is not getting included while building

the library.

So we will continue to use the earlier change.

> Does AIX have LD_LIBRARY_PATH? This suggests that it does:
> https://www.ibm.com/support/pages/aix-libpath-recommendations
> What's the difference between LIBPATH and LD_LIBRARY_PATH? Why prefer
LIBPATH?

AIX uses only LIBPATH and  LD_LIBRARY_PATH is specific to Linux.

> Separately from the remaining issues with this patch, I still feel
> pretty reluctant with accepting this, because if I commit this patch,
> I'm on the hook to keep it working. I do regularly commit stuff that I'm
> not personally that interested in, but a new port is different:

> If something breaks on AIX, I have no means (or interest!) in debugging
> it. That means that I need to be pretty confident that there are others
> who are interested and invested in keeping working, take ownership, will
> help to debug problems in a timely fashion, and can submit high-quality
> fixes. I am not seeing that.

I completely understand your concerns about committing to a new port and

ensuring its long-term maintenance.

Our team would like to extend the support to maintain PostgreSQL on AIX.

We would like to assist by timely debugging and resolving any issues on AIX.

We’re also keen to actively contribute to this effort.

> I also notice that all the AIX systems we have in the buildfarm are still:

> - maintained by Noah, who - correct me if I'm wrong - is not
> particularly interested in AIX or keen on maintaining them
> - are on AIX 7.1.5, which I believe is already end-of-line
> - running on power7 hardware which is 15 years old.

> That's not very reassuring.

From a hardware perspective, we believe it should be feasible to provide

new nodes in the OSU lab. We've already set up a Power9 machine with the

supported AIX version for the community usage. Recently, there was a request from

the community (Mark Wong) for additional nodes, and we're happy to offer

any hardware support needed.

Warm regards,

Sriram.