public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 2/3] +also rearrange the functions to their original order.. 34+ messages / 10 participants [nested] [flat]
* [PATCH 2/3] +also rearrange the functions to their original order.. @ 2023-02-28 19:34 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 34+ messages in thread From: Justin Pryzby @ 2023-02-28 19:34 UTC (permalink / raw) This allows comparing like: git diff --diff-algorithm=minimal -w e9960732a~:../src/bin/pg_dump/compress_io.c ../src/bin/pg_dump/compress_gzip.c --- src/bin/pg_dump/compress_gzip.c | 95 ++++++++++++++++++--------------- 1 file changed, 51 insertions(+), 44 deletions(-) diff --git a/src/bin/pg_dump/compress_gzip.c b/src/bin/pg_dump/compress_gzip.c index dd769750c8f..f3f5e87c9a8 100644 --- a/src/bin/pg_dump/compress_gzip.c +++ b/src/bin/pg_dump/compress_gzip.c @@ -24,22 +24,73 @@ * Compressor API *---------------------- */ typedef struct ZlibCompressorState { z_streamp zp; void *outbuf; size_t outsize; } ZlibCompressorState; +static void ReadDataFromArchiveZlib(ArchiveHandle *AH, CompressorState *cs); +static void WriteDataToArchiveZlib(ArchiveHandle *AH, CompressorState *cs, + const void *data, size_t dLen); +static void EndCompressorZlib(ArchiveHandle *AH, CompressorState *cs); +static void DeflateCompressorZlib(ArchiveHandle *AH, CompressorState *cs, + bool flush); + +/* Public routines that support zlib compressed data I/O */ +void +InitCompressorZlib(CompressorState *cs, + const pg_compress_specification compression_spec) +{ + ZlibCompressorState *gzipcs; + + cs->readData = ReadDataFromArchiveZlib; + cs->writeData = WriteDataToArchiveZlib; + cs->end = EndCompressorZlib; + + cs->compression_spec = compression_spec; + + gzipcs = (ZlibCompressorState *) pg_malloc0(sizeof(ZlibCompressorState)); + + cs->private_data = gzipcs; +} + +static void +EndCompressorZlib(ArchiveHandle *AH, CompressorState *cs) +{ + ZlibCompressorState *gzipcs = (ZlibCompressorState *) cs->private_data; + z_streamp zp; + + if (gzipcs->zp) + { + zp = gzipcs->zp; + zp->next_in = NULL; + zp->avail_in = 0; + + /* Flush any remaining data from zlib buffer */ + DeflateCompressorZlib(AH, cs, true); + + if (deflateEnd(zp) != Z_OK) + pg_fatal("could not close compression stream: %s", zp->msg); + + pg_free(gzipcs->outbuf); + pg_free(gzipcs->zp); + } + + pg_free(gzipcs); + cs->private_data = NULL; +} + /* Private routines that support zlib compressed data I/O */ static void DeflateCompressorZlib(ArchiveHandle *AH, CompressorState *cs, bool flush) { ZlibCompressorState *gzipcs = (ZlibCompressorState *) cs->private_data; z_streamp zp = gzipcs->zp; void *out = gzipcs->outbuf; int res = Z_OK; while (gzipcs->zp->avail_in != 0 || flush) { @@ -67,48 +118,22 @@ DeflateCompressorZlib(ArchiveHandle *AH, CompressorState *cs, bool flush) cs->writeF(AH, (char *) out, len); } zp->next_out = out; zp->avail_out = gzipcs->outsize; } if (res == Z_STREAM_END) break; } } -static void -EndCompressorZlib(ArchiveHandle *AH, CompressorState *cs) -{ - ZlibCompressorState *gzipcs = (ZlibCompressorState *) cs->private_data; - z_streamp zp; - - if (gzipcs->zp) - { - zp = gzipcs->zp; - zp->next_in = NULL; - zp->avail_in = 0; - - /* Flush any remaining data from zlib buffer */ - DeflateCompressorZlib(AH, cs, true); - - if (deflateEnd(zp) != Z_OK) - pg_fatal("could not close compression stream: %s", zp->msg); - - pg_free(gzipcs->outbuf); - pg_free(gzipcs->zp); - } - - pg_free(gzipcs); - cs->private_data = NULL; -} - static void WriteDataToArchiveZlib(ArchiveHandle *AH, CompressorState *cs, const void *data, size_t dLen) { ZlibCompressorState *gzipcs = (ZlibCompressorState *) cs->private_data; z_streamp zp; if (!gzipcs->zp) { zp = gzipcs->zp = (z_streamp) pg_malloc(sizeof(z_stream)); zp->zalloc = Z_NULL; @@ -193,40 +218,22 @@ ReadDataFromArchiveZlib(ArchiveHandle *AH, CompressorState *cs) ahwrite(out, 1, ZLIB_OUT_SIZE - zp->avail_out, AH); } if (inflateEnd(zp) != Z_OK) pg_fatal("could not close compression library: %s", zp->msg); free(buf); free(out); free(zp); } -/* Public routines that support zlib compressed data I/O */ -void -InitCompressorZlib(CompressorState *cs, - const pg_compress_specification compression_spec) -{ - ZlibCompressorState *gzipcs; - - cs->readData = ReadDataFromArchiveZlib; - cs->writeData = WriteDataToArchiveZlib; - cs->end = EndCompressorZlib; - - cs->compression_spec = compression_spec; - - gzipcs = (ZlibCompressorState *) pg_malloc0(sizeof(ZlibCompressorState)); - - cs->private_data = gzipcs; -} - /*---------------------- * Compress File API *---------------------- */ static size_t Gzip_read(void *ptr, size_t size, CompressFileHandle *CFH) { gzFile gzfp = (gzFile) CFH->private_data; size_t ret; -- 2.34.1 --eJnRUKwClWJh1Khz Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0003-pg_dump-call-deflateInit-in-Init-rather-than-in-Writ.patch" ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-23 14:27 Heikki Linnakangas <[email protected]> 0 siblings, 3 replies; 34+ messages in thread From: Heikki Linnakangas @ 2024-02-23 14:27 UTC (permalink / raw) To: Peter Smith <[email protected]>; Robert Haas <[email protected]>; +Cc: vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> Committed this. Thanks everyone! -- Heikki Linnakangas Neon (https://neon.tech) ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-23 15:12 Heikki Linnakangas <[email protected]> parent: Heikki Linnakangas <[email protected]> 2 siblings, 1 reply; 34+ messages in thread From: Heikki Linnakangas @ 2024-02-23 15:12 UTC (permalink / raw) To: pgsql-hackers; +Cc: vignesh C <[email protected]>; Andres Freund <[email protected]>; Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; Peter Smith <[email protected]> On 23/02/2024 16:27, Heikki Linnakangas wrote: > Committed this. Thanks everyone! Buildfarm animals 'sifaka' and 'longfin' are not happy, I will investigate.. -- Heikki Linnakangas Neon (https://neon.tech) ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-23 15:15 Tom Lane <[email protected]> parent: Heikki Linnakangas <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Tom Lane @ 2024-02-23 15:15 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: pgsql-hackers; vignesh C <[email protected]>; Andres Freund <[email protected]>; Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; Peter Smith <[email protected]> Heikki Linnakangas <[email protected]> writes: > Buildfarm animals 'sifaka' and 'longfin' are not happy, I will investigate.. Those are mine, let me know if you need local investigation. regards, tom lane ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-23 15:43 Heikki Linnakangas <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Heikki Linnakangas @ 2024-02-23 15:43 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: pgsql-hackers; vignesh C <[email protected]>; Andres Freund <[email protected]>; Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; Peter Smith <[email protected]> On 23/02/2024 17:15, Tom Lane wrote: > Heikki Linnakangas <[email protected]> writes: >> Buildfarm animals 'sifaka' and 'longfin' are not happy, I will investigate.. > > Those are mine, let me know if you need local investigation. Thanks, the error message was clear enough: > bulk_write.c:78:3: error: redefinition of typedef 'BulkWriteState' is a C11 feature [-Werror,-Wtypedef-redefinition] > } BulkWriteState; > ^ > ../../../../src/include/storage/bulk_write.h:20:31: note: previous definition is here > typedef struct BulkWriteState BulkWriteState; > ^ > 1 error generated. Fixed now, but I'm a bit surprised other buildfarm members nor cirrus CI caught that. I also tried to reproduce it locally by adding -Wtypedef-redefinition, but my version of clang didn't produce any warnings. Are there any extra compiler options on those animals or something? -- Heikki Linnakangas Neon (https://neon.tech) ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-23 16:06 Tom Lane <[email protected]> parent: Heikki Linnakangas <[email protected]> 0 siblings, 0 replies; 34+ messages in thread From: Tom Lane @ 2024-02-23 16:06 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: pgsql-hackers; vignesh C <[email protected]>; Andres Freund <[email protected]>; Melanie Plageman <[email protected]>; Robert Haas <[email protected]>; Peter Smith <[email protected]> Heikki Linnakangas <[email protected]> writes: > Thanks, the error message was clear enough: >> bulk_write.c:78:3: error: redefinition of typedef 'BulkWriteState' is a C11 feature [-Werror,-Wtypedef-redefinition] >> } BulkWriteState; > Fixed now, but I'm a bit surprised other buildfarm members nor cirrus CI > caught that. I also tried to reproduce it locally by adding > -Wtypedef-redefinition, but my version of clang didn't produce any > warnings. Are there any extra compiler options on those animals or > something? They use Apple's standard compiler (clang 15 or so), but 'CC' => 'ccache clang -std=gnu99', so maybe the -std has something to do with it. I installed that (or -std=gnu90 as appropriate to branch) on most of my build setups back when we started the C99 push. regards, tom lane ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-24 17:23 Noah Misch <[email protected]> parent: Heikki Linnakangas <[email protected]> 2 siblings, 1 reply; 34+ messages in thread From: Noah Misch @ 2024-02-24 17:23 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Fri, Feb 23, 2024 at 04:27:34PM +0200, Heikki Linnakangas wrote: > Committed this. Thanks everyone! https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mandrill&dt=2024-02-24%2015%3A13%3A14 got: TRAP: failed Assert("(uintptr_t) buffer == TYPEALIGN(PG_IO_ALIGN_SIZE, buffer)"), File: "md.c", Line: 472, PID: 43188608 with this stack trace: #5 0x10005cf0 in ExceptionalCondition (conditionName=0x1015d790 <XLogBeginInsert+80> "`", fileName=0x0, lineNumber=16780064) at assert.c:66 #6 0x102daba8 in mdextend (reln=0x1042628c <PageSetChecksumInplace+44>, forknum=812540744, blocknum=33, buffer=0x306e6000, skipFsync=812539904) at md.c:472 #7 0x102d6760 in smgrextend (reln=0x306e6670, forknum=812540744, blocknum=33, buffer=0x306e6000, skipFsync=812539904) at smgr.c:541 #8 0x104c8dac in smgr_bulk_flush (bulkstate=0x306e6000) at bulk_write.c:245 #9 0x107baf24 in _bt_blwritepage (wstate=0x100d0a14 <datum_image_eq@AF65_7+404>, buf=0x304f13b0, blkno=807631240) at nbtsort.c:638 #10 0x107bccd8 in _bt_buildadd (wstate=0x104c9384 <smgr_bulk_start_rel+132>, state=0x304eb190, itup=0xe10, truncextra=805686672) at nbtsort.c:984 #11 0x107bc86c in _bt_sort_dedup_finish_pending (wstate=0x3b6, state=0x19, dstate=0x3) at nbtsort.c:1036 #12 0x107bc188 in _bt_load (wstate=0x10, btspool=0x0, btspool2=0x0) at nbtsort.c:1331 #13 0x107bd4ec in _bt_leafbuild (btspool=0x101589fc <ProcessInvalidationMessages+188>, btspool2=0x0) at nbtsort.c:571 #14 0x107be028 in btbuild (heap=0x304d2a00, index=0x4e1f, indexInfo=0x3) at nbtsort.c:329 #15 0x1013538c in index_build (heapRelation=0x2, indexRelation=0x10bdc518 <getopt_long+2464664>, indexInfo=0x2, isreindex=10, parallel=false) at index.c:3047 #16 0x101389e0 in index_create (heapRelation=0x1001aac0 <palloc+192>, indexRelationName=0x20 <error: Cannot access memory at address 0x20>, indexRelationId=804393376, parentIndexRelid=805686672, parentConstraintId=268544704, relFileNumber=805309688, indexInfo=0x3009a328, indexColNames=0x30237a20, accessMethodId=403, tableSpaceId=0, collationIds=0x304d29d8, opclassIds=0x304d29f8, opclassOptions=0x304d2a18, coloptions=0x304d2a38, reloptions=0, flags=0, constr_flags=0, allow_system_table_mods=false, is_internal=false, constraintId=0x2ff211b4) at index.c:1260 #17 0x1050342c in DefineIndex (tableId=19994, stmt=0x2ff21370, indexRelationId=0, parentIndexId=0, parentConstraintId=0, total_parts=0, is_alter_table=false, check_rights=true, check_not_in_use=true, skip_build=false, quiet=false) at indexcmds.c:1204 #18 0x104b4474 in ProcessUtilitySlow (pstate=<error reading variable>, pstmt=0x3009a408, queryString=0x30099730 "CREATE INDEX dupindexcols_i ON dupindexcols (f1, id, f1 text_pattern_ops);", If there are other ways I should poke at it, let me know. ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-24 18:52 Thomas Munro <[email protected]> parent: Noah Misch <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Thomas Munro @ 2024-02-24 18:52 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Sun, Feb 25, 2024 at 6:24 AM Noah Misch <[email protected]> wrote: > On Fri, Feb 23, 2024 at 04:27:34PM +0200, Heikki Linnakangas wrote: > > Committed this. Thanks everyone! > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mandrill&dt=2024-02-24%2015%3A13%3A14 got: > TRAP: failed Assert("(uintptr_t) buffer == TYPEALIGN(PG_IO_ALIGN_SIZE, buffer)"), File: "md.c", Line: 472, PID: 43188608 > > with this stack trace: > #5 0x10005cf0 in ExceptionalCondition (conditionName=0x1015d790 <XLogBeginInsert+80> "`", fileName=0x0, lineNumber=16780064) at assert.c:66 > #6 0x102daba8 in mdextend (reln=0x1042628c <PageSetChecksumInplace+44>, forknum=812540744, blocknum=33, buffer=0x306e6000, skipFsync=812539904) at md.c:472 > #7 0x102d6760 in smgrextend (reln=0x306e6670, forknum=812540744, blocknum=33, buffer=0x306e6000, skipFsync=812539904) at smgr.c:541 > #8 0x104c8dac in smgr_bulk_flush (bulkstate=0x306e6000) at bulk_write.c:245 So that's: static const PGIOAlignedBlock zero_buffer = {{0}}; /* worth BLCKSZ */ ... smgrextend(bulkstate->smgr, bulkstate->forknum, bulkstate->pages_written++, &zero_buffer, true); ... where PGIOAlignedBlock is: typedef union PGIOAlignedBlock { #ifdef pg_attribute_aligned pg_attribute_aligned(PG_IO_ALIGN_SIZE) #endif char data[BLCKSZ]; ... We see this happen with both xlc and gcc (new enough to know how to do this). One idea would be that the AIX *linker* is unable to align it, as that is the common tool-chain component here (and unlike stack and heap objects, this scope is the linker's job). There is a pre-existing example of a zero-buffer that is at file scope like that: pg_prewarm.c. Perhaps it doesn't get tested? Hmm. ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-24 19:50 Noah Misch <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 2 replies; 34+ messages in thread From: Noah Misch @ 2024-02-24 19:50 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Sun, Feb 25, 2024 at 07:52:16AM +1300, Thomas Munro wrote: > On Sun, Feb 25, 2024 at 6:24 AM Noah Misch <[email protected]> wrote: > > On Fri, Feb 23, 2024 at 04:27:34PM +0200, Heikki Linnakangas wrote: > > > Committed this. Thanks everyone! > > > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mandrill&dt=2024-02-24%2015%3A13%3A14 got: > > TRAP: failed Assert("(uintptr_t) buffer == TYPEALIGN(PG_IO_ALIGN_SIZE, buffer)"), File: "md.c", Line: 472, PID: 43188608 > > > > with this stack trace: > > #5 0x10005cf0 in ExceptionalCondition (conditionName=0x1015d790 <XLogBeginInsert+80> "`", fileName=0x0, lineNumber=16780064) at assert.c:66 > > #6 0x102daba8 in mdextend (reln=0x1042628c <PageSetChecksumInplace+44>, forknum=812540744, blocknum=33, buffer=0x306e6000, skipFsync=812539904) at md.c:472 > > #7 0x102d6760 in smgrextend (reln=0x306e6670, forknum=812540744, blocknum=33, buffer=0x306e6000, skipFsync=812539904) at smgr.c:541 > > #8 0x104c8dac in smgr_bulk_flush (bulkstate=0x306e6000) at bulk_write.c:245 > > So that's: > > static const PGIOAlignedBlock zero_buffer = {{0}}; /* worth BLCKSZ */ > > ... > smgrextend(bulkstate->smgr, bulkstate->forknum, > bulkstate->pages_written++, > &zero_buffer, > true); > > ... where PGIOAlignedBlock is: > > typedef union PGIOAlignedBlock > { > #ifdef pg_attribute_aligned > pg_attribute_aligned(PG_IO_ALIGN_SIZE) > #endif > char data[BLCKSZ]; > ... > > We see this happen with both xlc and gcc (new enough to know how to do > this). One idea would be that the AIX *linker* is unable to align it, > as that is the common tool-chain component here (and unlike stack and > heap objects, this scope is the linker's job). There is a > pre-existing example of a zero-buffer that is at file scope like that: > pg_prewarm.c. Perhaps it doesn't get tested? > > Hmm. GCC docs do say "For some linkers, the maximum supported alignment may be very very small.", but AIX "man LD" says "data sections are aligned on a boundary so as to satisfy the alignment of all CSECTs in the sections". It also has -H and -K flags to force some particular higher alignment. On GNU/Linux x64, gcc correctly records alignment=2**12 for the associated section (.rodata for bulk_write.o zero_buffer, .bss for pg_prewarm.o blockbuffer). If I'm reading this right, neither AIX gcc nor xlc is marking the section with sufficient alignment, in bulk_write.o or pg_prewarm.o: $ /opt/cfarm/binutils-latest/bin/objdump --section-headers ~/farm/*/HEAD/pgsqlkeep.*/src/backend/storage/smgr/bulk_write.o /home/nm/farm/gcc64/HEAD/pgsqlkeep.2024-02-24_00-03-22/src/backend/storage/smgr/bulk_write.o: file format aix5coff64-rs6000 Sections: Idx Name Size VMA LMA File off Algn 0 .text 0000277c 0000000000000000 0000000000000000 000000f0 2**2 CONTENTS, ALLOC, LOAD, RELOC, CODE 1 .data 000000e4 000000000000277c 000000000000277c 0000286c 2**3 CONTENTS, ALLOC, LOAD, RELOC, DATA 2 .debug 0001f7ea 0000000000000000 0000000000000000 00002950 2**3 CONTENTS /home/nm/farm/xlc32/HEAD/pgsqlkeep.2024-02-24_15-12-23/src/backend/storage/smgr/bulk_write.o: file format aixcoff-rs6000 Sections: Idx Name Size VMA LMA File off Algn 0 .text 00000880 00000000 00000000 00000180 2**2 CONTENTS, ALLOC, LOAD, RELOC, CODE 1 .data 0000410c 00000880 00000880 00000a00 2**3 CONTENTS, ALLOC, LOAD, RELOC, DATA 2 .bss 00000000 0000498c 0000498c 00000000 2**3 ALLOC 3 .debug 00008448 00000000 00000000 00004b24 2**3 CONTENTS 4 .except 00000018 00000000 00000000 00004b0c 2**3 CONTENTS, LOAD $ /opt/cfarm/binutils-latest/bin/objdump --section-headers ~/farm/*/HEAD/pgsqlkeep.*/contrib/pg_prewarm/pg_prewarm.o /home/nm/farm/gcc32/HEAD/pgsqlkeep.2024-01-21_03-16-12/contrib/pg_prewarm/pg_prewarm.o: file format aixcoff-rs6000 Sections: Idx Name Size VMA LMA File off Algn 0 .text 00000a6c 00000000 00000000 000000b4 2**2 CONTENTS, ALLOC, LOAD, RELOC, CODE 1 .data 00000044 00000a6c 00000a6c 00000b20 2**3 CONTENTS, ALLOC, LOAD, RELOC, DATA 2 .bss 00002550 00000ab0 00000ab0 00000000 2**3 ALLOC 3 .debug 0001c50e 00000000 00000000 00000b64 2**3 CONTENTS /home/nm/farm/gcc64/HEAD/pgsqlkeep.2024-02-15_17-13-04/contrib/pg_prewarm/pg_prewarm.o: file format aix5coff64-rs6000 Sections: Idx Name Size VMA LMA File off Algn 0 .text 00000948 0000000000000000 0000000000000000 00000138 2**2 CONTENTS, ALLOC, LOAD, RELOC, CODE 1 .data 00000078 0000000000000948 0000000000000948 00000a80 2**3 CONTENTS, ALLOC, LOAD, RELOC, DATA 2 .bss 00002640 00000000000009c0 00000000000009c0 00000000 2**3 ALLOC 3 .debug 0001d887 0000000000000000 0000000000000000 00000af8 2**3 CONTENTS ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-24 20:12 Thomas Munro <[email protected]> parent: Noah Misch <[email protected]> 1 sibling, 1 reply; 34+ messages in thread From: Thomas Munro @ 2024-02-24 20:12 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Sun, Feb 25, 2024 at 8:50 AM Noah Misch <[email protected]> wrote: > On GNU/Linux x64, gcc correctly records alignment=2**12 for the associated > section (.rodata for bulk_write.o zero_buffer, .bss for pg_prewarm.o > blockbuffer). If I'm reading this right, neither AIX gcc nor xlc is marking > the section with sufficient alignment, in bulk_write.o or pg_prewarm.o: Ah, that is a bit of a hazard that we should probably document. I guess the ideas to fix this would be: use smgrzeroextend() instead of this coding, and/or perhaps look at the coding of pg_pwrite_zeros() (function-local static) for any other place that needs such a thing, if it would be satisfied by function-local scope? ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-24 20:13 Thomas Munro <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Thomas Munro @ 2024-02-24 20:13 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Sun, Feb 25, 2024 at 9:12 AM Thomas Munro <[email protected]> wrote: > On Sun, Feb 25, 2024 at 8:50 AM Noah Misch <[email protected]> wrote: > > On GNU/Linux x64, gcc correctly records alignment=2**12 for the associated > > section (.rodata for bulk_write.o zero_buffer, .bss for pg_prewarm.o > > blockbuffer). If I'm reading this right, neither AIX gcc nor xlc is marking > > the section with sufficient alignment, in bulk_write.o or pg_prewarm.o: > > Ah, that is a bit of a hazard that we should probably document. > > I guess the ideas to fix this would be: use smgrzeroextend() instead > of this coding, and/or perhaps look at the coding of pg_pwrite_zeros() > (function-local static) for any other place that needs such a thing, > if it would be satisfied by function-local scope? Erm, wait, how does that function-local static object work differently? ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-24 20:26 Noah Misch <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 0 replies; 34+ messages in thread From: Noah Misch @ 2024-02-24 20:26 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Sun, Feb 25, 2024 at 09:13:47AM +1300, Thomas Munro wrote: > On Sun, Feb 25, 2024 at 9:12 AM Thomas Munro <[email protected]> wrote: > > On Sun, Feb 25, 2024 at 8:50 AM Noah Misch <[email protected]> wrote: > > > On GNU/Linux x64, gcc correctly records alignment=2**12 for the associated > > > section (.rodata for bulk_write.o zero_buffer, .bss for pg_prewarm.o > > > blockbuffer). If I'm reading this right, neither AIX gcc nor xlc is marking > > > the section with sufficient alignment, in bulk_write.o or pg_prewarm.o: > > > > Ah, that is a bit of a hazard that we should probably document. > > > > I guess the ideas to fix this would be: use smgrzeroextend() instead > > of this coding, and/or perhaps look at the coding of pg_pwrite_zeros() > > (function-local static) for any other place that needs such a thing, > > if it would be satisfied by function-local scope? True. Alternatively, could arrange for "#define PG_O_DIRECT 0" on AIX, which disables the alignment assertions (and debug_io_direct). > Erm, wait, how does that function-local static object work differently? I don't know specifically, but I expect they're different parts of the gcc implementation. Aligning an xcoff section may entail some xcoff-specific gcc component. Aligning a function-local object just changes the early instructions of the function; it's independent of the object format. ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-24 21:29 Andres Freund <[email protected]> parent: Noah Misch <[email protected]> 1 sibling, 2 replies; 34+ messages in thread From: Andres Freund @ 2024-02-24 21:29 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> Hi, On 2024-02-24 11:50:24 -0800, Noah Misch wrote: > > We see this happen with both xlc and gcc (new enough to know how to do > > this). One idea would be that the AIX *linker* is unable to align it, > > as that is the common tool-chain component here (and unlike stack and > > heap objects, this scope is the linker's job). There is a > > pre-existing example of a zero-buffer that is at file scope like that: > > pg_prewarm.c. Perhaps it doesn't get tested? > > > > Hmm. > > GCC docs do say "For some linkers, the maximum supported alignment may be very > very small.", but AIX "man LD" says "data sections are aligned on a boundary > so as to satisfy the alignment of all CSECTs in the sections". It also has -H > and -K flags to force some particular higher alignment. Some xlc manual [1] states that n must be a positive power of 2, or NIL. NIL can be specified as either __attribute__((aligned())) or __attribute__((aligned)); this is the same as specifying the maximum system alignment (16 bytes on all UNIX platforms). Which does seems to suggest that this is a platform restriction. Let's just drop AIX. This isn't the only alignment issue we've found and the solution for those isn't so much a fix as forcing everyone to carefully only look into one direction and not notice the cliffs to either side. Greetings, Andres Freund [1] https://www.ibm.com/docs/en/SSGH2K_13.1.2/com.ibm.compilers.aix.doc/proguide.pdf ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-24 22:06 Heikki Linnakangas <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 34+ messages in thread From: Heikki Linnakangas @ 2024-02-24 22:06 UTC (permalink / raw) To: Andres Freund <[email protected]>; Noah Misch <[email protected]>; +Cc: Thomas Munro <[email protected]>; Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On 24 February 2024 23:29:36 EET, Andres Freund <[email protected]> wrote: >Hi, > >On 2024-02-24 11:50:24 -0800, Noah Misch wrote: >> > We see this happen with both xlc and gcc (new enough to know how to do >> > this). One idea would be that the AIX *linker* is unable to align it, >> > as that is the common tool-chain component here (and unlike stack and >> > heap objects, this scope is the linker's job). There is a >> > pre-existing example of a zero-buffer that is at file scope like that: >> > pg_prewarm.c. Perhaps it doesn't get tested? >> > >> > Hmm. >> >> GCC docs do say "For some linkers, the maximum supported alignment may be very >> very small.", but AIX "man LD" says "data sections are aligned on a boundary >> so as to satisfy the alignment of all CSECTs in the sections". It also has -H >> and -K flags to force some particular higher alignment. > >Some xlc manual [1] states that > > n must be a positive power of 2, or NIL. NIL can be specified as either > __attribute__((aligned())) or __attribute__((aligned)); this is the same as > specifying the maximum system alignment (16 bytes on all UNIX platforms). > >Which does seems to suggest that this is a platform restriction. My reading of that paragraph is that you can set it to any powet of two, and it should work. 16 bytes is just what you get if you set it to NIL. >Let's just drop AIX. This isn't the only alignment issue we've found and the >solution for those isn't so much a fix as forcing everyone to carefully only >look into one direction and not notice the cliffs to either side. I think the way that decision should go is that as long as someone is willing to step up and do the work keep AIX support going, we support it. To be clear, that someone is not me. Anyone willing to do it? Regarding the issue at hand, perhaps we should define PG_IO_ALIGN_SIZE as 16 on AIX, if that's the best the linker can do on that platform. We could also make the allocation 2*PG_IO_ALIGN_SIZE and round up the starting address ourselves to PG_IO_ALIGN_SIZE. Or allocate it in the heap. - Heikki ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-24 22:16 Thomas Munro <[email protected]> parent: Heikki Linnakangas <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Thomas Munro @ 2024-02-24 22:16 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Andres Freund <[email protected]>; Noah Misch <[email protected]>; Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Sun, Feb 25, 2024 at 11:06 AM Heikki Linnakangas <[email protected]> wrote: > Regarding the issue at hand, perhaps we should define PG_IO_ALIGN_SIZE as 16 on AIX, if that's the best the linker can do on that platform. You'll probably get either an error or silently fall back to buffered I/O, if direct I/O is enabled and you try to read/write a badly aligned buffer. That's documented (they offer finfo() to query it, but it's always 4KB for the same sort of reasons as it is on every other OS). ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-24 22:37 Thomas Munro <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Thomas Munro @ 2024-02-24 22:37 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Andres Freund <[email protected]>; Noah Misch <[email protected]>; Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Sun, Feb 25, 2024 at 11:16 AM Thomas Munro <[email protected]> wrote: > On Sun, Feb 25, 2024 at 11:06 AM Heikki Linnakangas <[email protected]> wrote: > > Regarding the issue at hand, perhaps we should define PG_IO_ALIGN_SIZE as 16 on AIX, if that's the best the linker can do on that platform. > > You'll probably get either an error or silently fall back to buffered > I/O, if direct I/O is enabled and you try to read/write a badly > aligned buffer. That's documented (they offer finfo() to query it, > but it's always 4KB for the same sort of reasons as it is on every > other OS). I guess it's the latter ("to work efficiently" sounds like it isn't going to reject the request): https://www.ibm.com/docs/en/aix/7.3?topic=tuning-direct-io If you make it < 4KB then all direct I/O would be affected, not just this one place, so then you might as well just not allow direct I/O on AIX at all, to avoid giving a false impression that it does something. (Note that if we think the platform lacks O_DIRECT we don't make those assertions about alignment). FWIW I'm aware of one other thing that is wrong with our direct I/O support on AIX: it should perhaps be using a different flag. I created a wiki page to defer thinking about any AIX issues until/unless at least one real, live user shows up, which hasn't happened yet: https://wiki.postgresql.org/wiki/AIX ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-25 14:34 Heikki Linnakangas <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Heikki Linnakangas @ 2024-02-25 14:34 UTC (permalink / raw) To: Thomas Munro <[email protected]>; Noah Misch <[email protected]>; +Cc: Andres Freund <[email protected]>; Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On 25/02/2024 00:37, Thomas Munro wrote: > On Sun, Feb 25, 2024 at 11:16 AM Thomas Munro <[email protected]> wrote: >> On Sun, Feb 25, 2024 at 11:06 AM Heikki Linnakangas <[email protected]> wrote: >>> Regarding the issue at hand, perhaps we should define PG_IO_ALIGN_SIZE as 16 on AIX, if that's the best the linker can do on that platform. >> >> You'll probably get either an error or silently fall back to buffered >> I/O, if direct I/O is enabled and you try to read/write a badly >> aligned buffer. That's documented (they offer finfo() to query it, >> but it's always 4KB for the same sort of reasons as it is on every >> other OS). > > I guess it's the latter ("to work efficiently" sounds like it isn't > going to reject the request): > > https://www.ibm.com/docs/en/aix/7.3?topic=tuning-direct-io > > If you make it < 4KB then all direct I/O would be affected, not just > this one place, so then you might as well just not allow direct I/O on > AIX at all, to avoid giving a false impression that it does something. > (Note that if we think the platform lacks O_DIRECT we don't make those > assertions about alignment). > > FWIW I'm aware of one other thing that is wrong with our direct I/O > support on AIX: it should perhaps be using a different flag. I > created a wiki page to defer thinking about any AIX issues > until/unless at least one real, live user shows up, which hasn't > happened yet: https://wiki.postgresql.org/wiki/AIX Here's a patch that effectively disables direct I/O on AIX. I'm inclined to commit this as a quick fix to make the buildfarm green again. I agree with Andres though, that unless someone raises their hand and volunteers to properly maintain the AIX support, we should drop it. The current AIX buildfarm members are running AIX 7.1, which has been out of support since May 2023 (https://www.ibm.com/support/pages/aix-support-lifecycle-information). See also older thread on this [0]. Noah, you're running the current AIX buildfarm animals. How much effort are you interested to put into AIX support? [0] https://www.postgresql.org/message-id/20220702183354.a6uhja35wta7agew%40alap3.anarazel.de -- Heikki Linnakangas Neon (https://neon.tech) Attachments: [text/x-patch] 0001-Disable-O_DIRECT-on-AIX.patch (3.3K, ../../[email protected]/2-0001-Disable-O_DIRECT-on-AIX.patch) download | inline diff: From 3e206b952b8011c4bab97c7c87ea693832137999 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas <[email protected]> Date: Sun, 25 Feb 2024 13:33:07 +0200 Subject: [PATCH 1/1] Disable O_DIRECT on AIX AIX apparently doesn't support the __attribute__((aligned(a))) attribute, when a > 16. That means that PGIOAlignedBlock is not correctly aligned. That showed up as an assertion failure when writing pages with the new bulk_write.c facility, because it uses "static const" of type PGIOAlignedBlock. pg_prewarm.c also uses a static variable of type PGIOAlignedBlock, and it's not clear why it hasn't tripped the assertion. There surely would be ways to force the alignment on AIX, but at least this should make the buildfarm green again, and it doesn't seem worth spending more effort on this right now. We might soon drop AIX support altogether unless someone steps up to the plate to maintain it properly and set up a more recent buildfarm animal for it. Discussion: https://www.postgresql.org/message-id/[email protected] --- src/include/c.h | 14 ++++++++++---- src/include/port/aix.h | 3 +++ src/include/storage/fd.h | 2 +- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/include/c.h b/src/include/c.h index 2e3ea206e1..9a464bd592 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -186,6 +186,12 @@ #define pg_attribute_noreturn() __attribute__((noreturn)) #define pg_attribute_packed() __attribute__((packed)) #define HAVE_PG_ATTRIBUTE_NORETURN 1 + +/* Some platforms (AIX) support the aligned attribute only for small values */ +#if !defined(PG_MAX_ATTRIBUTE_ALIGN) || PG_MAX_ATTRIBUTE_ALIGN >= PG_IO_ALIGN_SIZE +#define pg_attribute_io_aligned __attribute__((aligned(PG_IO_ALIGN_SIZE))) +#endif + #elif defined(_MSC_VER) /* * MSVC supports aligned. noreturn is also possible but in MSVC it is @@ -1123,8 +1129,8 @@ typedef union PGAlignedBlock */ typedef union PGIOAlignedBlock { -#ifdef pg_attribute_aligned - pg_attribute_aligned(PG_IO_ALIGN_SIZE) +#ifdef pg_attribute_io_aligned + pg_attribute_io_aligned #endif char data[BLCKSZ]; double force_align_d; @@ -1134,8 +1140,8 @@ typedef union PGIOAlignedBlock /* Same, but for an XLOG_BLCKSZ-sized buffer */ typedef union PGAlignedXLogBlock { -#ifdef pg_attribute_aligned - pg_attribute_aligned(PG_IO_ALIGN_SIZE) +#ifdef pg_attribute_io_aligned + pg_attribute_io_aligned #endif char data[XLOG_BLCKSZ]; double force_align_d; diff --git a/src/include/port/aix.h b/src/include/port/aix.h index 5b1159c578..405151746d 100644 --- a/src/include/port/aix.h +++ b/src/include/port/aix.h @@ -12,3 +12,6 @@ #if defined(__ILP32__) && defined(__IBMC__) #define PG_FORCE_DISABLE_INLINE #endif + +/* pg_attribute_aligned(n) works only up to 16 bytes on AIX. */ +#define PG_MAX_ATTRIBUTE_ALIGN 16 diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h index 60bba5c970..579cb3142b 100644 --- a/src/include/storage/fd.h +++ b/src/include/storage/fd.h @@ -88,7 +88,7 @@ extern PGDLLIMPORT int max_safe_fds; * idea on a Unix). We can only use it if the compiler will correctly align * PGIOAlignedBlock for us, though. */ -#if defined(O_DIRECT) && defined(pg_attribute_aligned) +#if defined(O_DIRECT) && defined(pg_attribute_io_aligned) #define PG_O_DIRECT O_DIRECT #elif defined(F_NOCACHE) #define PG_O_DIRECT 0x80000000 -- 2.39.2 ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-25 19:43 Noah Misch <[email protected]> parent: Heikki Linnakangas <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Noah Misch @ 2024-02-25 19:43 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Thomas Munro <[email protected]>; Andres Freund <[email protected]>; Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Sun, Feb 25, 2024 at 04:34:47PM +0200, Heikki Linnakangas wrote: > I agree with Andres though, that unless someone raises their hand and > volunteers to properly maintain the AIX support, we should drop it. There's no way forward in which AIX support stops doing net harm. Even if AIX enthusiasts intercepted would-be buildfarm failures and fixed them before buildfarm.postgresql.org could see them, the damage from the broader community seeing the AIX-specific code would outweigh the benefits of AIX support. I've now disabled the animals for v17+, though each may do one more run before picking up the disable. My upthread observation about xcoff section alignment was a red herring. gcc populates symbol-level alignment, and section-level alignment is unnecessary if symbol-level alignment is correct. The simplest workaround for $SUBJECT AIX failure would be to remove the "const", based on the results of the attached test program. The pg_prewarm.c var is like al4096_static in the outputs below, hence the lack of trouble there. The bulk_write.c var is like al4096_static_const_initialized. ==== gcc 8.3.0 al4096 4096 @ 0x11000c000 (mod 0) al4096_initialized 4096 @ 0x110000fd0 (mod 4048 - BUG) al4096_const 4096 @ 0x11000f000 (mod 0) al4096_const_initialized 4096 @ 0x10000cd00 (mod 3328 - BUG) al4096_static 4096 @ 0x110005000 (mod 0) al4096_static_initialized 4096 @ 0x110008000 (mod 0) al4096_static_const 4096 @ 0x100000c10 (mod 3088 - BUG) al4096_static_const_initialized 4096 @ 0x100003c10 (mod 3088 - BUG) ==== xlc 12.01.0000.0000 al4096 4096 @ 0x110008000 (mod 0) al4096_initialized 4096 @ 0x110004000 (mod 0) al4096_const 4096 @ 0x11000b000 (mod 0) al4096_const_initialized 4096 @ 0x100007000 (mod 0) al4096_static 4096 @ 0x11000e000 (mod 0) al4096_static_initialized 4096 @ 0x110001000 (mod 0) al4096_static_const 4096 @ 0x110011000 (mod 0) al4096_static_const_initialized 4096 @ 0x1000007d0 (mod 2000 - BUG) ==== ibm-clang 17.1.1.2 al4096 4096 @ 0x110001000 (mod 0) al4096_initialized 4096 @ 0x110004000 (mod 0) al4096_const 4096 @ 0x100001000 (mod 0) al4096_const_initialized 4096 @ 0x100005000 (mod 0) al4096_static 4096 @ 0x110008000 (mod 0) al4096_static_initialized 4096 @ 0x11000b000 (mod 0) al4096_static_const 4096 @ 0x100009000 (mod 0) al4096_static_const_initialized 4096 @ 0x10000d000 (mod 0) #include <stdio.h> #include <stdint.h> /* add a byte so the compiler has to go out of its way to achieve alignment for consecutive instances */ typedef union PGIOAlignedBlock { __attribute__((aligned(4096))) char data[1 + 8192]; double force_align_d; uint64_t force_align_i64; } PGIOAlignedBlock; char pad; /* with and without each of: static, const, initializer */ PGIOAlignedBlock al4096; PGIOAlignedBlock al4096_initialized = {{0}}; const PGIOAlignedBlock al4096_const; const PGIOAlignedBlock al4096_const_initialized = {{0}}; static PGIOAlignedBlock al4096_static; static PGIOAlignedBlock al4096_static_initialized = {{0}}; static const PGIOAlignedBlock al4096_static_const; static const PGIOAlignedBlock al4096_static_const_initialized = {{0}}; /* in case last position is special */ static const PGIOAlignedBlock last; static const PGIOAlignedBlock last_initialized = {{0}}; #define DUMP(want_align, var) dump_internal(#var, (want_align), &(var)) static void dump_internal(const char *ident, unsigned want_align, const void *addr) { unsigned mod = (uintptr_t) addr % want_align; printf("%-32s %4u @ 0x%p (mod %u%s)\n", ident, want_align, addr, mod, mod ? " - BUG" : ""); } int main(int argc, char **argv) { DUMP(4096, al4096); DUMP(4096, al4096_initialized); DUMP(4096, al4096_const); DUMP(4096, al4096_const_initialized); DUMP(4096, al4096_static); DUMP(4096, al4096_static_initialized); DUMP(4096, al4096_static_const); DUMP(4096, al4096_static_const_initialized); return 0; } Attachments: [text/plain] align.c (1.5K, ../../[email protected]/2-align.c) download | inline: #include <stdio.h> #include <stdint.h> /* add a byte so the compiler has to go out of its way to achieve alignment for consecutive instances */ typedef union PGIOAlignedBlock { __attribute__((aligned(4096))) char data[1 + 8192]; double force_align_d; uint64_t force_align_i64; } PGIOAlignedBlock; char pad; /* with and without each of: static, const, initializer */ PGIOAlignedBlock al4096; PGIOAlignedBlock al4096_initialized = {{0}}; const PGIOAlignedBlock al4096_const; const PGIOAlignedBlock al4096_const_initialized = {{0}}; static PGIOAlignedBlock al4096_static; static PGIOAlignedBlock al4096_static_initialized = {{0}}; static const PGIOAlignedBlock al4096_static_const; static const PGIOAlignedBlock al4096_static_const_initialized = {{0}}; /* in case last position is special */ static const PGIOAlignedBlock last; static const PGIOAlignedBlock last_initialized = {{0}}; #define DUMP(want_align, var) dump_internal(#var, (want_align), &(var)) static void dump_internal(const char *ident, unsigned want_align, const void *addr) { unsigned mod = (uintptr_t) addr % want_align; printf("%-32s %4u @ 0x%p (mod %u%s)\n", ident, want_align, addr, mod, mod ? " - BUG" : ""); } int main(int argc, char **argv) { DUMP(4096, al4096); DUMP(4096, al4096_initialized); DUMP(4096, al4096_const); DUMP(4096, al4096_const_initialized); DUMP(4096, al4096_static); DUMP(4096, al4096_static_initialized); DUMP(4096, al4096_static_const); DUMP(4096, al4096_static_const_initialized); return 0; } ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-25 19:51 Tom Lane <[email protected]> parent: Noah Misch <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Tom Lane @ 2024-02-25 19:51 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Thomas Munro <[email protected]>; Andres Freund <[email protected]>; Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> Noah Misch <[email protected]> writes: > On Sun, Feb 25, 2024 at 04:34:47PM +0200, Heikki Linnakangas wrote: >> I agree with Andres though, that unless someone raises their hand and >> volunteers to properly maintain the AIX support, we should drop it. > There's no way forward in which AIX support stops doing net harm. Even if AIX > enthusiasts intercepted would-be buildfarm failures and fixed them before > buildfarm.postgresql.org could see them, the damage from the broader community > seeing the AIX-specific code would outweigh the benefits of AIX support. I've > now disabled the animals for v17+, though each may do one more run before > picking up the disable. So, we now need to strip the remnants of AIX support from the code and docs? I don't see that much of it, but it's misleading to leave it there. (BTW, I still want to nuke the remaining snippets of HPPA support. I don't think it does anybody any good to make it look like that's still expected to work.) regards, tom lane ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-26 04:12 Robert Haas <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Robert Haas @ 2024-02-26 04:12 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Noah Misch <[email protected]>; Heikki Linnakangas <[email protected]>; Thomas Munro <[email protected]>; Andres Freund <[email protected]>; Peter Smith <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Mon, Feb 26, 2024 at 1:21 AM Tom Lane <[email protected]> wrote: > So, we now need to strip the remnants of AIX support from the code and > docs? I don't see that much of it, but it's misleading to leave it > there. > > (BTW, I still want to nuke the remaining snippets of HPPA support. > I don't think it does anybody any good to make it look like that's > still expected to work.) +1 for removing things that don't work (or that we think probably don't work). -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-26 04:18 Michael Paquier <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Michael Paquier @ 2024-02-26 04:18 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Noah Misch <[email protected]>; Heikki Linnakangas <[email protected]>; Thomas Munro <[email protected]>; Andres Freund <[email protected]>; Peter Smith <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Mon, Feb 26, 2024 at 09:42:03AM +0530, Robert Haas wrote: > On Mon, Feb 26, 2024 at 1:21 AM Tom Lane <[email protected]> wrote: >> So, we now need to strip the remnants of AIX support from the code and >> docs? I don't see that much of it, but it's misleading to leave it >> there. >> >> (BTW, I still want to nuke the remaining snippets of HPPA support. >> I don't think it does anybody any good to make it look like that's >> still expected to work.) > > +1 for removing things that don't work (or that we think probably don't work). Seeing this stuff eat developer time because of the debugging of weird issues while having a very limited impact for end-users is sad, so +1 for a cleanup of any remnants if this disappears. -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-27 20:24 Heikki Linnakangas <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 4 replies; 34+ messages in thread From: Heikki Linnakangas @ 2024-02-27 20:24 UTC (permalink / raw) To: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; Noah Misch <[email protected]>; Thomas Munro <[email protected]>; Andres Freund <[email protected]>; Peter Smith <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On 26/02/2024 06:18, Michael Paquier wrote: > On Mon, Feb 26, 2024 at 09:42:03AM +0530, Robert Haas wrote: >> On Mon, Feb 26, 2024 at 1:21 AM Tom Lane <[email protected]> wrote: >>> So, we now need to strip the remnants of AIX support from the code and >>> docs? I don't see that much of it, but it's misleading to leave it >>> there. >>> >>> (BTW, I still want to nuke the remaining snippets of HPPA support. >>> I don't think it does anybody any good to make it look like that's >>> still expected to work.) >> >> +1 for removing things that don't work (or that we think probably don't work). > > Seeing this stuff eat developer time because of the debugging of weird > issues while having a very limited impact for end-users is sad, so +1 > for a cleanup of any remnants if this disappears. Here's a patch to fully remove AIX support. One small issue that warrants some discussion (in sanity_check.sql): > --- When ALIGNOF_DOUBLE==4 (e.g. AIX), the C ABI may impose 8-byte alignment on > +-- When MAXIMUM_ALIGNOF==8 but ALIGNOF_DOUBLE==4, the C ABI may impose 8-byte alignment > -- some of the C types that correspond to TYPALIGN_DOUBLE SQL types. To ensure > -- catalog C struct layout matches catalog tuple layout, arrange for the tuple > -- offset of each fixed-width, attalign='d' catalog column to be divisible by 8 > -- unconditionally. Keep such columns before the first NameData column of the > -- catalog, since packagers can override NAMEDATALEN to an odd number. > +-- (XXX: I'm not sure if any of the supported platforms have MAXIMUM_ALIGNOF==8 and > +-- ALIGNOF_DOUBLE==4. Perhaps we should just require that > +-- ALIGNOF_DOUBLE==MAXIMUM_ALIGNOF) What do y'all think of adding a check for ALIGNOF_DOUBLE==MAXIMUM_ALIGNOF to configure.ac and meson.build? It's not a requirement today, but I believe AIX was the only platform where that was not true. With AIX gone, that combination won't be tested, and we will probably break it sooner or later. -- Heikki Linnakangas Neon (https://neon.tech) Attachments: [text/x-patch] 0001-Remove-AIX-support.patch (52.6K, ../../[email protected]/2-0001-Remove-AIX-support.patch) download | inline diff: From 59a66f507365ff9cb8c462d8206be285e3e2632d Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas <[email protected]> Date: Wed, 28 Feb 2024 00:22:23 +0400 Subject: [PATCH 1/1] Remove AIX support There isn't a lot of user demand for AIX support, no one has stepped up to the plate to properly maintain it, so it's best to remove it altogether. AIX is still supported for stable versions. The acute issue that triggered this decision was that after commit 8af2565248, the AIX buildfarm members have been hitting this assertion: TRAP: failed Assert("(uintptr_t) buffer == TYPEALIGN(PG_IO_ALIGN_SIZE, buffer)"), File: "md.c", Line: 472, PID: 2949728 Apperently the "pg_attribute_aligned(a)" attribute doesn't work on AIX (linker?) for values larger than PG_IO_ALIGN_SIZE. That could be worked around, but we decided to just drop the AIX support instead. Discussion: https://www.postgresql.org/message-id/20240224172345.32%40rfd.leadboat.com --- Makefile | 2 + config/c-compiler.m4 | 2 +- configure | 301 +----------------- configure.ac | 34 +- doc/src/sgml/dfunc.sgml | 19 -- doc/src/sgml/installation.sgml | 119 +------ doc/src/sgml/runtime.sgml | 23 -- meson.build | 27 +- src/Makefile.shlib | 30 -- src/backend/Makefile | 20 -- src/backend/meson.build | 15 - src/backend/port/aix/mkldexport.sh | 61 ---- src/backend/utils/error/elog.c | 2 - src/backend/utils/misc/ps_status.c | 4 +- src/bin/pg_basebackup/t/010_pg_basebackup.pl | 3 - src/bin/pg_verifybackup/t/008_untar.pl | 3 - src/bin/pg_verifybackup/t/010_client_untar.pl | 3 - src/include/c.h | 18 +- src/include/port/aix.h | 14 - src/include/port/atomics.h | 6 +- src/include/storage/s_lock.h | 31 +- src/interfaces/libpq/Makefile | 2 +- src/interfaces/libpq/meson.build | 5 +- src/makefiles/Makefile.aix | 39 --- src/port/README | 2 +- src/port/strerror.c | 2 - src/template/aix | 25 -- src/test/regress/Makefile | 5 - src/test/regress/expected/sanity_check.out | 5 +- src/test/regress/sql/sanity_check.sql | 5 +- src/tools/gen_export.pl | 11 +- src/tools/pginclude/cpluspluscheck | 1 - src/tools/pginclude/headerscheck | 1 - 33 files changed, 52 insertions(+), 788 deletions(-) delete mode 100755 src/backend/port/aix/mkldexport.sh delete mode 100644 src/include/port/aix.h delete mode 100644 src/makefiles/Makefile.aix delete mode 100644 src/template/aix diff --git a/Makefile b/Makefile index 9bc1a4ec17b..8a2ec9396b6 100644 --- a/Makefile +++ b/Makefile @@ -13,6 +13,8 @@ # AIX make defaults to building *every* target of the first rule. Start with # a single-target, empty rule to make the other targets non-default. +# (We don't support AIX anymore, but if someone tries to build on AIX anyway, +# at least they'll get the instructions to run 'configure' first.) all: all check install installdirs installcheck installcheck-parallel uninstall clean distclean maintainer-clean dist distcheck world check-world install-world installcheck-world: diff --git a/config/c-compiler.m4 b/config/c-compiler.m4 index 5db02b2ab75..3268a780bb0 100644 --- a/config/c-compiler.m4 +++ b/config/c-compiler.m4 @@ -137,7 +137,7 @@ if test x"$pgac_cv__128bit_int" = xyes ; then AC_CACHE_CHECK([for __int128 alignment bug], [pgac_cv__128bit_int_bug], [AC_RUN_IFELSE([AC_LANG_PROGRAM([ /* This must match the corresponding code in c.h: */ -#if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__) +#if defined(__GNUC__) || defined(__SUNPRO_C) #define pg_attribute_aligned(a) __attribute__((aligned(a))) #elif defined(_MSC_VER) #define pg_attribute_aligned(a) __declspec(align(a)) diff --git a/configure b/configure index 6b87e5c9a8c..4e2dba338be 100755 --- a/configure +++ b/configure @@ -2987,7 +2987,6 @@ else # --with-template not given case $host_os in - aix*) template=aix ;; cygwin*|msys*) template=cygwin ;; darwin*) template=darwin ;; dragonfly*) template=netbsd ;; @@ -3917,10 +3916,10 @@ fi -case $template in - aix) pgac_cc_list="gcc xlc"; pgac_cxx_list="g++ xlC";; - *) pgac_cc_list="gcc cc"; pgac_cxx_list="g++ c++";; -esac +# If you don't specify a list of compilers to test, the AC_PROG_CC and +# AC_PROG_CXX macros test for a long list of unsupported compilers. +pgac_cc_list="gcc cc" +pgac_cxx_list="g++ c++" ac_ext=c ac_cpp='$CPP $CPPFLAGS' @@ -6874,190 +6873,6 @@ if test x"$pgac_cv_prog_CXX_cxxflags__fno_strict_aliasing" = x"yes"; then fi -elif test "$PORTNAME" = "aix"; then - # AIX's xlc has to have strict aliasing turned off too - -{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -qnoansialias, for CFLAGS" >&5 -$as_echo_n "checking whether ${CC} supports -qnoansialias, for CFLAGS... " >&6; } -if ${pgac_cv_prog_CC_cflags__qnoansialias+:} false; then : - $as_echo_n "(cached) " >&6 -else - pgac_save_CFLAGS=$CFLAGS -pgac_save_CC=$CC -CC=${CC} -CFLAGS="${CFLAGS} -qnoansialias" -ac_save_c_werror_flag=$ac_c_werror_flag -ac_c_werror_flag=yes -cat confdefs.h - <<_ACEOF >conftest.$ac_ext -/* end confdefs.h. */ - -int -main () -{ - - ; - return 0; -} -_ACEOF -if ac_fn_c_try_compile "$LINENO"; then : - pgac_cv_prog_CC_cflags__qnoansialias=yes -else - pgac_cv_prog_CC_cflags__qnoansialias=no -fi -rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext -ac_c_werror_flag=$ac_save_c_werror_flag -CFLAGS="$pgac_save_CFLAGS" -CC="$pgac_save_CC" -fi -{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CC_cflags__qnoansialias" >&5 -$as_echo "$pgac_cv_prog_CC_cflags__qnoansialias" >&6; } -if test x"$pgac_cv_prog_CC_cflags__qnoansialias" = x"yes"; then - CFLAGS="${CFLAGS} -qnoansialias" -fi - - - { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CXX} supports -qnoansialias, for CXXFLAGS" >&5 -$as_echo_n "checking whether ${CXX} supports -qnoansialias, for CXXFLAGS... " >&6; } -if ${pgac_cv_prog_CXX_cxxflags__qnoansialias+:} false; then : - $as_echo_n "(cached) " >&6 -else - pgac_save_CXXFLAGS=$CXXFLAGS -pgac_save_CXX=$CXX -CXX=${CXX} -CXXFLAGS="${CXXFLAGS} -qnoansialias" -ac_save_cxx_werror_flag=$ac_cxx_werror_flag -ac_cxx_werror_flag=yes -ac_ext=cpp -ac_cpp='$CXXCPP $CPPFLAGS' -ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5' -ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' -ac_compiler_gnu=$ac_cv_cxx_compiler_gnu - -cat confdefs.h - <<_ACEOF >conftest.$ac_ext -/* end confdefs.h. */ - -int -main () -{ - - ; - return 0; -} -_ACEOF -if ac_fn_cxx_try_compile "$LINENO"; then : - pgac_cv_prog_CXX_cxxflags__qnoansialias=yes -else - pgac_cv_prog_CXX_cxxflags__qnoansialias=no -fi -rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext -ac_ext=c -ac_cpp='$CPP $CPPFLAGS' -ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5' -ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' -ac_compiler_gnu=$ac_cv_c_compiler_gnu - -ac_cxx_werror_flag=$ac_save_cxx_werror_flag -CXXFLAGS="$pgac_save_CXXFLAGS" -CXX="$pgac_save_CXX" -fi -{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CXX_cxxflags__qnoansialias" >&5 -$as_echo "$pgac_cv_prog_CXX_cxxflags__qnoansialias" >&6; } -if test x"$pgac_cv_prog_CXX_cxxflags__qnoansialias" = x"yes"; then - CXXFLAGS="${CXXFLAGS} -qnoansialias" -fi - - - -{ $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -qlonglong, for CFLAGS" >&5 -$as_echo_n "checking whether ${CC} supports -qlonglong, for CFLAGS... " >&6; } -if ${pgac_cv_prog_CC_cflags__qlonglong+:} false; then : - $as_echo_n "(cached) " >&6 -else - pgac_save_CFLAGS=$CFLAGS -pgac_save_CC=$CC -CC=${CC} -CFLAGS="${CFLAGS} -qlonglong" -ac_save_c_werror_flag=$ac_c_werror_flag -ac_c_werror_flag=yes -cat confdefs.h - <<_ACEOF >conftest.$ac_ext -/* end confdefs.h. */ - -int -main () -{ - - ; - return 0; -} -_ACEOF -if ac_fn_c_try_compile "$LINENO"; then : - pgac_cv_prog_CC_cflags__qlonglong=yes -else - pgac_cv_prog_CC_cflags__qlonglong=no -fi -rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext -ac_c_werror_flag=$ac_save_c_werror_flag -CFLAGS="$pgac_save_CFLAGS" -CC="$pgac_save_CC" -fi -{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CC_cflags__qlonglong" >&5 -$as_echo "$pgac_cv_prog_CC_cflags__qlonglong" >&6; } -if test x"$pgac_cv_prog_CC_cflags__qlonglong" = x"yes"; then - CFLAGS="${CFLAGS} -qlonglong" -fi - - - { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CXX} supports -qlonglong, for CXXFLAGS" >&5 -$as_echo_n "checking whether ${CXX} supports -qlonglong, for CXXFLAGS... " >&6; } -if ${pgac_cv_prog_CXX_cxxflags__qlonglong+:} false; then : - $as_echo_n "(cached) " >&6 -else - pgac_save_CXXFLAGS=$CXXFLAGS -pgac_save_CXX=$CXX -CXX=${CXX} -CXXFLAGS="${CXXFLAGS} -qlonglong" -ac_save_cxx_werror_flag=$ac_cxx_werror_flag -ac_cxx_werror_flag=yes -ac_ext=cpp -ac_cpp='$CXXCPP $CPPFLAGS' -ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5' -ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' -ac_compiler_gnu=$ac_cv_cxx_compiler_gnu - -cat confdefs.h - <<_ACEOF >conftest.$ac_ext -/* end confdefs.h. */ - -int -main () -{ - - ; - return 0; -} -_ACEOF -if ac_fn_cxx_try_compile "$LINENO"; then : - pgac_cv_prog_CXX_cxxflags__qlonglong=yes -else - pgac_cv_prog_CXX_cxxflags__qlonglong=no -fi -rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext -ac_ext=c -ac_cpp='$CPP $CPPFLAGS' -ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5' -ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' -ac_compiler_gnu=$ac_cv_c_compiler_gnu - -ac_cxx_werror_flag=$ac_save_cxx_werror_flag -CXXFLAGS="$pgac_save_CXXFLAGS" -CXX="$pgac_save_CXX" -fi -{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CXX_cxxflags__qlonglong" >&5 -$as_echo "$pgac_cv_prog_CXX_cxxflags__qlonglong" >&6; } -if test x"$pgac_cv_prog_CXX_cxxflags__qlonglong" = x"yes"; then - CXXFLAGS="${CXXFLAGS} -qlonglong" -fi - - fi # If the compiler knows how to hide symbols, add the switch needed for that to @@ -7212,103 +7027,6 @@ if test x"$pgac_cv_prog_CXX_cxxflags__fvisibility_inlines_hidden" = x"yes"; then fi have_visibility_attribute=$pgac_cv_prog_CC_cflags__fvisibility_hidden -elif test "$PORTNAME" = "aix"; then - # Note that xlc accepts -fvisibility=hidden as a file. - { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CC} supports -qvisibility=hidden, for CFLAGS_SL_MODULE" >&5 -$as_echo_n "checking whether ${CC} supports -qvisibility=hidden, for CFLAGS_SL_MODULE... " >&6; } -if ${pgac_cv_prog_CC_cflags__qvisibility_hidden+:} false; then : - $as_echo_n "(cached) " >&6 -else - pgac_save_CFLAGS=$CFLAGS -pgac_save_CC=$CC -CC=${CC} -CFLAGS="${CFLAGS_SL_MODULE} -qvisibility=hidden" -ac_save_c_werror_flag=$ac_c_werror_flag -ac_c_werror_flag=yes -cat confdefs.h - <<_ACEOF >conftest.$ac_ext -/* end confdefs.h. */ - -int -main () -{ - - ; - return 0; -} -_ACEOF -if ac_fn_c_try_compile "$LINENO"; then : - pgac_cv_prog_CC_cflags__qvisibility_hidden=yes -else - pgac_cv_prog_CC_cflags__qvisibility_hidden=no -fi -rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext -ac_c_werror_flag=$ac_save_c_werror_flag -CFLAGS="$pgac_save_CFLAGS" -CC="$pgac_save_CC" -fi -{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CC_cflags__qvisibility_hidden" >&5 -$as_echo "$pgac_cv_prog_CC_cflags__qvisibility_hidden" >&6; } -if test x"$pgac_cv_prog_CC_cflags__qvisibility_hidden" = x"yes"; then - CFLAGS_SL_MODULE="${CFLAGS_SL_MODULE} -qvisibility=hidden" -fi - - - { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether ${CXX} supports -qvisibility=hidden, for CXXFLAGS_SL_MODULE" >&5 -$as_echo_n "checking whether ${CXX} supports -qvisibility=hidden, for CXXFLAGS_SL_MODULE... " >&6; } -if ${pgac_cv_prog_CXX_cxxflags__qvisibility_hidden+:} false; then : - $as_echo_n "(cached) " >&6 -else - pgac_save_CXXFLAGS=$CXXFLAGS -pgac_save_CXX=$CXX -CXX=${CXX} -CXXFLAGS="${CXXFLAGS_SL_MODULE} -qvisibility=hidden" -ac_save_cxx_werror_flag=$ac_cxx_werror_flag -ac_cxx_werror_flag=yes -ac_ext=cpp -ac_cpp='$CXXCPP $CPPFLAGS' -ac_compile='$CXX -c $CXXFLAGS $CPPFLAGS conftest.$ac_ext >&5' -ac_link='$CXX -o conftest$ac_exeext $CXXFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' -ac_compiler_gnu=$ac_cv_cxx_compiler_gnu - -cat confdefs.h - <<_ACEOF >conftest.$ac_ext -/* end confdefs.h. */ - -int -main () -{ - - ; - return 0; -} -_ACEOF -if ac_fn_cxx_try_compile "$LINENO"; then : - pgac_cv_prog_CXX_cxxflags__qvisibility_hidden=yes -else - pgac_cv_prog_CXX_cxxflags__qvisibility_hidden=no -fi -rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext -ac_ext=c -ac_cpp='$CPP $CPPFLAGS' -ac_compile='$CC -c $CFLAGS $CPPFLAGS conftest.$ac_ext >&5' -ac_link='$CC -o conftest$ac_exeext $CFLAGS $CPPFLAGS $LDFLAGS conftest.$ac_ext $LIBS >&5' -ac_compiler_gnu=$ac_cv_c_compiler_gnu - -ac_cxx_werror_flag=$ac_save_cxx_werror_flag -CXXFLAGS="$pgac_save_CXXFLAGS" -CXX="$pgac_save_CXX" -fi -{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_prog_CXX_cxxflags__qvisibility_hidden" >&5 -$as_echo "$pgac_cv_prog_CXX_cxxflags__qvisibility_hidden" >&6; } -if test x"$pgac_cv_prog_CXX_cxxflags__qvisibility_hidden" = x"yes"; then - CXXFLAGS_SL_MODULE="${CXXFLAGS_SL_MODULE} -qvisibility=hidden" -fi - - have_visibility_attribute=$pgac_cv_prog_CC_cflags__qvisibility_hidden - # Old xlc versions (<13.1) don't have support for -qvisibility. Use expfull to force - # all extension module symbols to be exported. - if test "$pgac_cv_prog_CC_cflags__qvisibility_hidden" != "yes"; then - CFLAGS_SL_MODULE="$CFLAGS_SL_MODULE -Wl,-b,expfull" - fi fi if test "$have_visibility_attribute" = "yes"; then @@ -13166,8 +12884,7 @@ fi fi -# Note: We can test for libldap_r only after we know PTHREAD_LIBS; -# also, on AIX, we may need to have openssl in LIBS for this step. +# Note: We can test for libldap_r only after we know PTHREAD_LIBS if test "$with_ldap" = yes ; then _LIBS="$LIBS" if test "$PORTNAME" != "win32"; then @@ -15025,10 +14742,6 @@ fi # spelling it understands, because it conflicts with # __declspec(restrict). Therefore we define pg_restrict to the # appropriate definition, which presumably won't conflict. -# -# Allow platforms with buggy compilers to force restrict to not be -# used by setting $FORCE_DISABLE_RESTRICT=yes in the relevant -# template. { $as_echo "$as_me:${as_lineno-$LINENO}: checking for C/C++ restrict keyword" >&5 $as_echo_n "checking for C/C++ restrict keyword... " >&6; } if ${ac_cv_c_restrict+:} false; then : @@ -15075,7 +14788,7 @@ _ACEOF ;; esac -if test "$ac_cv_c_restrict" = "no" -o "x$FORCE_DISABLE_RESTRICT" = "xyes"; then +if test "$ac_cv_c_restrict" = "no"; then pg_restrict="" else pg_restrict="$ac_cv_c_restrict" @@ -17391,7 +17104,7 @@ else /* end confdefs.h. */ /* This must match the corresponding code in c.h: */ -#if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__) +#if defined(__GNUC__) || defined(__SUNPRO_C) #define pg_attribute_aligned(a) __attribute__((aligned(a))) #elif defined(_MSC_VER) #define pg_attribute_aligned(a) __declspec(align(a)) diff --git a/configure.ac b/configure.ac index 6e64ece11da..e8851daa798 100644 --- a/configure.ac +++ b/configure.ac @@ -62,7 +62,6 @@ PGAC_ARG_REQ(with, template, [NAME], [override operating system template], # --with-template not given case $host_os in - aix*) template=aix ;; cygwin*|msys*) template=cygwin ;; darwin*) template=darwin ;; dragonfly*) template=netbsd ;; @@ -374,10 +373,10 @@ AC_DEFINE_UNQUOTED([XLOG_BLCKSZ], ${XLOG_BLCKSZ}, [ # variable. PGAC_ARG_REQ(with, CC, [CMD], [set compiler (deprecated)], [CC=$with_CC]) -case $template in - aix) pgac_cc_list="gcc xlc"; pgac_cxx_list="g++ xlC";; - *) pgac_cc_list="gcc cc"; pgac_cxx_list="g++ c++";; -esac +# If you don't specify a list of compilers to test, the AC_PROG_CC and +# AC_PROG_CXX macros test for a long list of unsupported compilers. +pgac_cc_list="gcc cc" +pgac_cxx_list="g++ c++" AC_PROG_CC([$pgac_cc_list]) AC_PROG_CC_C99() @@ -594,12 +593,6 @@ elif test "$ICC" = yes; then # Make sure strict aliasing is off (though this is said to be the default) PGAC_PROG_CC_CFLAGS_OPT([-fno-strict-aliasing]) PGAC_PROG_CXX_CFLAGS_OPT([-fno-strict-aliasing]) -elif test "$PORTNAME" = "aix"; then - # AIX's xlc has to have strict aliasing turned off too - PGAC_PROG_CC_CFLAGS_OPT([-qnoansialias]) - PGAC_PROG_CXX_CFLAGS_OPT([-qnoansialias]) - PGAC_PROG_CC_CFLAGS_OPT([-qlonglong]) - PGAC_PROG_CXX_CFLAGS_OPT([-qlonglong]) fi # If the compiler knows how to hide symbols, add the switch needed for that to @@ -618,16 +611,6 @@ if test "$GCC" = yes -o "$SUN_STUDIO_CC" = yes ; then PGAC_PROG_VARCXX_VARFLAGS_OPT(CXX, CXXFLAGS_SL_MODULE, [-fvisibility=hidden]) PGAC_PROG_VARCXX_VARFLAGS_OPT(CXX, CXXFLAGS_SL_MODULE, [-fvisibility-inlines-hidden]) have_visibility_attribute=$pgac_cv_prog_CC_cflags__fvisibility_hidden -elif test "$PORTNAME" = "aix"; then - # Note that xlc accepts -fvisibility=hidden as a file. - PGAC_PROG_CC_VAR_OPT(CFLAGS_SL_MODULE, [-qvisibility=hidden]) - PGAC_PROG_VARCXX_VARFLAGS_OPT(CXX, CXXFLAGS_SL_MODULE, [-qvisibility=hidden]) - have_visibility_attribute=$pgac_cv_prog_CC_cflags__qvisibility_hidden - # Old xlc versions (<13.1) don't have support for -qvisibility. Use expfull to force - # all extension module symbols to be exported. - if test "$pgac_cv_prog_CC_cflags__qvisibility_hidden" != "yes"; then - CFLAGS_SL_MODULE="$CFLAGS_SL_MODULE -Wl,-b,expfull" - fi fi if test "$have_visibility_attribute" = "yes"; then @@ -1407,8 +1390,7 @@ if test "$with_zstd" = yes ; then AC_CHECK_LIB(zstd, ZSTD_compress, [], [AC_MSG_ERROR([library 'zstd' is required for ZSTD support])]) fi -# Note: We can test for libldap_r only after we know PTHREAD_LIBS; -# also, on AIX, we may need to have openssl in LIBS for this step. +# Note: We can test for libldap_r only after we know PTHREAD_LIBS if test "$with_ldap" = yes ; then _LIBS="$LIBS" if test "$PORTNAME" != "win32"; then @@ -1666,12 +1648,8 @@ PGAC_TYPE_LOCALE_T # spelling it understands, because it conflicts with # __declspec(restrict). Therefore we define pg_restrict to the # appropriate definition, which presumably won't conflict. -# -# Allow platforms with buggy compilers to force restrict to not be -# used by setting $FORCE_DISABLE_RESTRICT=yes in the relevant -# template. AC_C_RESTRICT -if test "$ac_cv_c_restrict" = "no" -o "x$FORCE_DISABLE_RESTRICT" = "xyes"; then +if test "$ac_cv_c_restrict" = "no"; then pg_restrict="" else pg_restrict="$ac_cv_c_restrict" diff --git a/doc/src/sgml/dfunc.sgml b/doc/src/sgml/dfunc.sgml index 554f9fac4ce..b94aefcd0ca 100644 --- a/doc/src/sgml/dfunc.sgml +++ b/doc/src/sgml/dfunc.sgml @@ -202,23 +202,4 @@ gcc -G -o foo.so foo.o server expects to find the shared library files. </para> -<!-- -Under AIX, object files are compiled normally but building the shared -library requires a couple of steps. First, create the object file: -.nf -cc <other flags> -c foo.c -.fi -You must then create a symbol \*(lqexports\*(rq file for the object -file: -.nf -mkldexport foo.o `pwd` > foo.exp -.fi -Finally, you can create the shared library: -.nf -ld <other flags> -H512 -T512 -o foo.so -e _nostart \e - -bI:.../lib/postgres.exp -bE:foo.exp foo.o \e - -lm -lc 2>/dev/null -.fi - --> - </sect2> diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml index ed5b285a5ee..d901576d98e 100644 --- a/doc/src/sgml/installation.sgml +++ b/doc/src/sgml/installation.sgml @@ -3401,7 +3401,7 @@ export MANPATH <para> <productname>PostgreSQL</productname> can be expected to work on current versions of these operating systems: Linux, Windows, - FreeBSD, OpenBSD, NetBSD, DragonFlyBSD, macOS, AIX, Solaris, and illumos. + FreeBSD, OpenBSD, NetBSD, DragonFlyBSD, macOS, Solaris, and illumos. Other Unix-like systems may also work but are not currently being tested. In most cases, all CPU architectures supported by a given operating system will work. Look in @@ -3445,123 +3445,6 @@ export MANPATH installation issues. </para> - <sect2 id="installation-notes-aix"> - <title>AIX</title> - - <indexterm zone="installation-notes-aix"> - <primary>AIX</primary> - <secondary>installation on</secondary> - </indexterm> - - <para> - You can use GCC or the native IBM compiler <command>xlc</command> - to build <productname>PostgreSQL</productname> - on <productname>AIX</productname>. - </para> - - <para> - <productname>AIX</productname> versions before 7.1 are no longer - tested nor supported by the <productname>PostgreSQL</productname> - community. - </para> - - <sect3 id="installation-notes-aix-mem-management"> - <title>Memory Management</title> - <!-- https://archives.postgresql.org/message-id/[email protected] --> - - <para> - AIX can be somewhat peculiar with regards to the way it does - memory management. You can have a server with many multiples of - gigabytes of RAM free, but still get out of memory or address - space errors when running applications. One example - is loading of extensions failing with unusual errors. - For example, running as the owner of the PostgreSQL installation: -<screen> -=# CREATE EXTENSION plperl; -ERROR: could not load library "/opt/dbs/pgsql/lib/plperl.so": A memory address is not in the address space for the process. -</screen> - Running as a non-owner in the group possessing the PostgreSQL - installation: -<screen> -=# CREATE EXTENSION plperl; -ERROR: could not load library "/opt/dbs/pgsql/lib/plperl.so": Bad address -</screen> - Another example is out of memory errors in the PostgreSQL server - logs, with every memory allocation near or greater than 256 MB - failing. - </para> - - <para> - The overall cause of all these problems is the default bittedness - and memory model used by the server process. By default, all - binaries built on AIX are 32-bit. This does not depend upon - hardware type or kernel in use. These 32-bit processes are - limited to 4 GB of memory laid out in 256 MB segments using one - of a few models. The default allows for less than 256 MB in the - heap as it shares a single segment with the stack. - </para> - - <para> - In the case of the <literal>plperl</literal> example, above, - check your umask and the permissions of the binaries in your - PostgreSQL installation. The binaries involved in that example - were 32-bit and installed as mode 750 instead of 755. Due to the - permissions being set in this fashion, only the owner or a member - of the possessing group can load the library. Since it isn't - world-readable, the loader places the object into the process' - heap instead of the shared library segments where it would - otherwise be placed. - </para> - - <para> - The <quote>ideal</quote> solution for this is to use a 64-bit - build of PostgreSQL, but that is not always practical, because - systems with 32-bit processors can build, but not run, 64-bit - binaries. - </para> - - <para> - If a 32-bit binary is desired, set <symbol>LDR_CNTRL</symbol> to - <literal>MAXDATA=0x<replaceable>n</replaceable>0000000</literal>, - where 1 <= n <= 8, before starting the PostgreSQL server, - and try different values and <filename>postgresql.conf</filename> - settings to find a configuration that works satisfactorily. This - use of <symbol>LDR_CNTRL</symbol> tells AIX that you want the - server to have <symbol>MAXDATA</symbol> bytes set aside for the - heap, allocated in 256 MB segments. When you find a workable - configuration, - <command>ldedit</command> can be used to modify the binaries so - that they default to using the desired heap size. PostgreSQL can - also be rebuilt, passing <literal>configure - LDFLAGS="-Wl,-bmaxdata:0x<replaceable>n</replaceable>0000000"</literal> - to achieve the same effect. - </para> - - <para> - For a 64-bit build, set <envar>OBJECT_MODE</envar> to 64 and - pass <literal>CC="gcc -maix64"</literal> - and <literal>LDFLAGS="-Wl,-bbigtoc"</literal> - to <command>configure</command>. (Options for - <command>xlc</command> might differ.) If you omit the export of - <envar>OBJECT_MODE</envar>, your build may fail with linker errors. When - <envar>OBJECT_MODE</envar> is set, it tells AIX's build utilities - such as <command>ar</command>, <command>as</command>, and <command>ld</command> what - type of objects to default to handling. - </para> - - <para> - By default, overcommit of paging space can happen. While we have - not seen this occur, AIX will kill processes when it runs out of - memory and the overcommit is accessed. The closest to this that - we have seen is fork failing because the system decided that - there was not enough memory for another process. Like many other - parts of AIX, the paging space allocation method and - out-of-memory kill is configurable on a system- or process-wide - basis if this becomes a problem. - </para> - </sect3> - </sect2> - <sect2 id="installation-notes-cygwin"> <title>Cygwin</title> diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml index 64753d9c014..6047b8171d4 100644 --- a/doc/src/sgml/runtime.sgml +++ b/doc/src/sgml/runtime.sgml @@ -891,29 +891,6 @@ psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: No such <variablelist> - <varlistentry> - <term><systemitem class="osname">AIX</systemitem> - <indexterm><primary>AIX</primary><secondary>IPC configuration</secondary></indexterm> - </term> - <listitem> - <para> - It should not be necessary to do - any special configuration for such parameters as - <varname>SHMMAX</varname>, as it appears this is configured to - allow all memory to be used as shared memory. That is the - sort of configuration commonly used for other databases such - as <application>DB/2</application>.</para> - - <para> It might, however, be necessary to modify the global - <command>ulimit</command> information in - <filename>/etc/security/limits</filename>, as the default hard - limits for file sizes (<varname>fsize</varname>) and numbers of - files (<varname>nofiles</varname>) might be too low. - </para> - </listitem> - </varlistentry> - - <varlistentry> <term><systemitem class="osname">FreeBSD</systemitem> <indexterm><primary>FreeBSD</primary><secondary>IPC configuration</secondary></indexterm> diff --git a/meson.build b/meson.build index 8ed51b6aae8..c637a26e275 100644 --- a/meson.build +++ b/meson.build @@ -196,26 +196,7 @@ endif # that purpose. portname = host_system -if host_system == 'aix' - library_path_var = 'LIBPATH' - - export_file_format = 'aix' - export_fmt = '-Wl,-bE:@0@' - mod_link_args_fmt = ['-Wl,-bI:@0@'] - mod_link_with_dir = 'libdir' - mod_link_with_name = '@[email protected]' - - # M:SRE sets a flag indicating that an object is a shared library. Seems to - # work in some circumstances without, but required in others. - ldflags_sl += '-Wl,-bM:SRE' - ldflags_be += '-Wl,-brtllib' - - # Native memset() is faster, tested on: - # - AIX 5.1 and 5.2, XLC 6.0 (IBM's cc) - # - AIX 5.3 ML3, gcc 4.0.1 - memset_loop_limit = 0 - -elif host_system == 'cygwin' +if host_system == 'cygwin' sema_kind = 'unnamed_posix' cppflags += '-D_GNU_SOURCE' dlsuffix = '.dll' @@ -1571,7 +1552,7 @@ if cc.links(''' if not meson.is_cross_build() r = cc.run(''' /* This must match the corresponding code in c.h: */ - #if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__) + #if defined(__GNUC__) || defined(__SUNPRO_C) #define pg_attribute_aligned(a) __attribute__((aligned(a))) #elif defined(_MSC_VER) #define pg_attribute_aligned(a) __declspec(align(a)) @@ -2371,10 +2352,6 @@ endif # conflict. # # We assume C99 support, so we don't need to make this conditional. -# -# XXX: Historically we allowed platforms to disable restrict in template -# files, but that was only added for AIX when building with XLC, which we -# don't support yet. cdata.set('pg_restrict', '__restrict') diff --git a/src/Makefile.shlib b/src/Makefile.shlib index 8ca51ca03f7..986c71b5d20 100644 --- a/src/Makefile.shlib +++ b/src/Makefile.shlib @@ -106,20 +106,6 @@ ifdef SO_MAJOR_VERSION override CPPFLAGS += -DSO_MAJOR_VERSION=$(SO_MAJOR_VERSION) endif -ifeq ($(PORTNAME), aix) - LINK.shared = $(COMPILER) - ifdef SO_MAJOR_VERSION - shlib = lib$(NAME)$(DLSUFFIX).$(SO_MAJOR_VERSION) - endif - haslibarule = yes - # $(exports_file) is also usable as an import file - exports_file = lib$(NAME).exp - BUILD.exports = ( echo '\#! $(shlib)'; $(AWK) '/^[^\#]/ {printf "%s\n",$$1}' $< ) > $@ - ifneq (,$(SHLIB_EXPORTS)) - LINK.shared += -Wl,-bE:$(exports_file) - endif -endif - ifeq ($(PORTNAME), darwin) ifdef soname # linkable library @@ -268,14 +254,6 @@ $(stlib): $(OBJS) | $(SHLIB_PREREQS) touch $@ endif #haslibarule -# AIX wraps shared libraries inside a static library, can be used both -# for static and shared linking -ifeq ($(PORTNAME), aix) -$(stlib): $(shlib) - rm -f $(stlib) - $(AR) $(AROPT) $(stlib) $(shlib) -endif # aix - ifeq (,$(filter cygwin win32,$(PORTNAME))) # Normal case @@ -289,11 +267,8 @@ ifneq ($(shlib), $(shlib_major)) endif # Make sure we have a link to a name without any version numbers ifneq ($(shlib), $(shlib_bare)) -# except on AIX, where that's not a thing -ifneq ($(PORTNAME), aix) rm -f $(shlib_bare) $(LN_S) $(shlib) $(shlib_bare) -endif # aix endif # shlib_bare endif # shlib_major @@ -401,10 +376,6 @@ install-lib-static: $(stlib) installdirs-lib install-lib-shared: $(shlib) installdirs-lib ifdef soname -# we don't install $(shlib) on AIX -# (see http://archives.postgresql.org/message-id/52EF20B2E3209443BC37736D00C3C1380A6E79FE@EXADV1.host.magwien.gv.at) -ifneq ($(PORTNAME), aix) - $(INSTALL_SHLIB) $< '$(DESTDIR)$(libdir)/$(shlib)' ifneq ($(PORTNAME), cygwin) ifneq ($(PORTNAME), win32) ifneq ($(shlib), $(shlib_major)) @@ -419,7 +390,6 @@ ifneq ($(shlib), $(shlib_bare)) endif endif # not win32 endif # not cygwin -endif # not aix ifneq (,$(findstring $(PORTNAME),win32 cygwin)) $(INSTALL_SHLIB) $< '$(DESTDIR)$(bindir)/$(shlib)' endif diff --git a/src/backend/Makefile b/src/backend/Makefile index 7d2ea7d54a6..d66e2a4b9fa 100644 --- a/src/backend/Makefile +++ b/src/backend/Makefile @@ -62,14 +62,12 @@ all: submake-libpgport submake-catalog-headers submake-utils-headers postgres $( ifneq ($(PORTNAME), cygwin) ifneq ($(PORTNAME), win32) -ifneq ($(PORTNAME), aix) postgres: $(OBJS) $(CC) $(CFLAGS) $(call expand_subsys,$^) $(LDFLAGS) $(LIBS) -o $@ endif endif -endif ifeq ($(PORTNAME), cygwin) @@ -96,24 +94,6 @@ libpostgres.a: postgres endif # win32 -ifeq ($(PORTNAME), aix) - -postgres: $(POSTGRES_IMP) - $(CC) $(CFLAGS) $(call expand_subsys,$(OBJS)) $(LDFLAGS) -Wl,-bE:$(top_builddir)/src/backend/$(POSTGRES_IMP) $(LIBS) -Wl,-brtllib -o $@ - -# Linking to a single .o with -r is a lot faster than building a .a or passing -# all objects to MKLDEXPORT. -# -# It looks alluring to use $(CC) -r instead of ld -r, but that doesn't -# trivially work with gcc, due to gcc specific static libraries linked in with -# -r. -$(POSTGRES_IMP): $(OBJS) - ld -r -o SUBSYS.o $(call expand_subsys,$^) - $(MKLDEXPORT) SUBSYS.o . > $@ - @rm -f SUBSYS.o - -endif # aix - $(top_builddir)/src/port/libpgport_srv.a: | submake-libpgport diff --git a/src/backend/meson.build b/src/backend/meson.build index 8767aaba678..436c04af080 100644 --- a/src/backend/meson.build +++ b/src/backend/meson.build @@ -91,21 +91,6 @@ if cc.get_id() == 'msvc' # be restricted to b_pch=true. backend_link_with += postgres_lib -elif host_system == 'aix' - # The '.' argument leads mkldexport.sh to emit "#! .", which refers to the - # main executable, allowing extension libraries to resolve their undefined - # symbols to symbols in the postgres binary. - postgres_imp = custom_target('postgres.imp', - command: [files('port/aix/mkldexport.sh'), '@INPUT@', '.'], - input: postgres_lib, - output: 'postgres.imp', - capture: true, - install: true, - install_dir: dir_lib, - build_by_default: false, - ) - backend_link_args += '-Wl,-bE:@0@'.format(postgres_imp.full_path()) - backend_link_depends += postgres_imp endif backend_input = [] diff --git a/src/backend/port/aix/mkldexport.sh b/src/backend/port/aix/mkldexport.sh deleted file mode 100755 index adf3793e868..00000000000 --- a/src/backend/port/aix/mkldexport.sh +++ /dev/null @@ -1,61 +0,0 @@ -#!/bin/sh -# -# mkldexport -# create an AIX exports file from an object file -# -# src/backend/port/aix/mkldexport.sh -# -# Usage: -# mkldexport objectfile [location] -# where -# objectfile is the current location of the object file. -# location is the eventual (installed) location of the -# object file (if different from the current -# working directory). -# -# [This file comes from the Postgres 4.2 distribution. - ay 7/95] -# -# Header: /usr/local/devel/postgres/src/tools/mkldexport/RCS/mkldexport.sh,v 1.2 1994/03/13 04:59:12 aoki Exp -# - -# setting this to nm -B might be better -# ... due to changes in AIX 4.x ... -# ... let us search in different directories - Gerhard Reithofer -if [ -x /usr/ucb/nm ] -then NM=/usr/ucb/nm -elif [ -x /usr/bin/nm ] -then NM=/usr/bin/nm -elif [ -x /usr/ccs/bin/nm ] -then NM=/usr/ccs/bin/nm -elif [ -x /usr/usg/bin/nm ] -then NM=/usr/usg/bin/nm -else echo "Fatal error: cannot find `nm' ... please check your installation." - exit 1 -fi - -CMDNAME=`basename $0` -if [ -z "$1" ]; then - echo "Usage: $CMDNAME object [location]" - exit 1 -fi -OBJNAME=`basename $1` -if [ "`basename $OBJNAME`" != "`basename $OBJNAME .o`" ]; then - OBJNAME=`basename $OBJNAME .o`.so -fi -if [ -z "$2" ]; then - echo '#!' -else - if [ "$2" = "." ]; then - # for the base executable (AIX 4.2 and up) - echo '#! .' - else - echo '#!' $2 - fi -fi -$NM -BCg $1 | \ - egrep ' [TDB] ' | \ - sed -e 's/.* //' | \ - egrep -v '\$' | \ - sed -e 's/^[.]//' | \ - sort | \ - uniq diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index bba00a0087f..c9719f358b6 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -911,9 +911,7 @@ errcode_for_file_access(void) /* Wrong object type or state */ case ENOTDIR: /* Not a directory */ case EISDIR: /* Is a directory */ -#if defined(ENOTEMPTY) && (ENOTEMPTY != EEXIST) /* same code on AIX */ case ENOTEMPTY: /* Directory not empty */ -#endif edata->sqlerrcode = ERRCODE_WRONG_OBJECT_TYPE; break; diff --git a/src/backend/utils/misc/ps_status.c b/src/backend/utils/misc/ps_status.c index 8f77f4b563a..ddb45a6bce8 100644 --- a/src/backend/utils/misc/ps_status.c +++ b/src/backend/utils/misc/ps_status.c @@ -52,7 +52,7 @@ bool update_process_title = DEFAULT_UPDATE_PROCESS_TITLE; #define PS_USE_SETPROCTITLE_FAST #elif defined(HAVE_SETPROCTITLE) #define PS_USE_SETPROCTITLE -#elif defined(__linux__) || defined(_AIX) || defined(__sun) || defined(__darwin__) +#elif defined(__linux__) || defined(__sun) || defined(__darwin__) #define PS_USE_CLOBBER_ARGV #elif defined(WIN32) #define PS_USE_WIN32 @@ -62,7 +62,7 @@ bool update_process_title = DEFAULT_UPDATE_PROCESS_TITLE; /* Different systems want the buffer padded differently */ -#if defined(_AIX) || defined(__linux__) || defined(__darwin__) +#if defined(__linux__) || defined(__darwin__) #define PS_PADDING '\0' #else #define PS_PADDING ' ' diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl index 86cc01a640b..fc6b00224f6 100644 --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl @@ -400,9 +400,6 @@ is(scalar(@tblspc_tars), 1, 'one tablespace tar was created'); SKIP: { my $tar = $ENV{TAR}; - # don't check for a working tar here, to accommodate various odd - # cases such as AIX. If tar doesn't work the init_from_backup below - # will fail. skip "no tar program available", 1 if (!defined $tar || $tar eq ''); diff --git a/src/bin/pg_verifybackup/t/008_untar.pl b/src/bin/pg_verifybackup/t/008_untar.pl index 30d9f3f7f0f..b4235541ab0 100644 --- a/src/bin/pg_verifybackup/t/008_untar.pl +++ b/src/bin/pg_verifybackup/t/008_untar.pl @@ -103,9 +103,6 @@ for my $tc (@test_configuration) SKIP: { my $tar = $ENV{TAR}; - # don't check for a working tar here, to accommodate various odd - # cases such as AIX. If tar doesn't work the init_from_backup below - # will fail. skip "no tar program available", 1 if (!defined $tar || $tar eq ''); diff --git a/src/bin/pg_verifybackup/t/010_client_untar.pl b/src/bin/pg_verifybackup/t/010_client_untar.pl index 45010d79ac8..e9c2bcd7aad 100644 --- a/src/bin/pg_verifybackup/t/010_client_untar.pl +++ b/src/bin/pg_verifybackup/t/010_client_untar.pl @@ -133,9 +133,6 @@ for my $tc (@test_configuration) SKIP: { my $tar = $ENV{TAR}; - # don't check for a working tar here, to accommodate various odd - # cases such as AIX. If tar doesn't work the init_from_backup below - # will fail. skip "no tar program available", 1 if (!defined $tar || $tar eq ''); diff --git a/src/include/c.h b/src/include/c.h index 2e3ea206e1c..cc53f154041 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -105,8 +105,6 @@ * GCC: https://gcc.gnu.org/onlinedocs/gcc/Type-Attributes.html * Clang: https://clang.llvm.org/docs/AttributeReference.html * Sunpro: https://docs.oracle.com/cd/E18659_01/html/821-1384/gjzke.html - * XLC: https://www.ibm.com/support/knowledgecenter/SSGH2K_13.1.2/com.ibm.xlc131.aix.doc/language_ref/function_attributes.html - * XLC: https://www.ibm.com/support/knowledgecenter/SSGH2K_13.1.2/com.ibm.xlc131.aix.doc/language_ref/type_attrib.html */ /* @@ -171,8 +169,8 @@ #define PG_USED_FOR_ASSERTS_ONLY pg_attribute_unused() #endif -/* GCC and XLC support format attributes */ -#if defined(__GNUC__) || defined(__IBMC__) +/* GCC support format attributes */ +#if defined(__GNUC__) #define pg_attribute_format_arg(a) __attribute__((format_arg(a))) #define pg_attribute_printf(f,a) __attribute__((format(PG_PRINTF_ATTRIBUTE, f, a))) #else @@ -180,8 +178,8 @@ #define pg_attribute_printf(f,a) #endif -/* GCC, Sunpro and XLC support aligned, packed and noreturn */ -#if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__) +/* GCC and Sunpro support aligned, packed and noreturn */ +#if defined(__GNUC__) || defined(__SUNPRO_C) #define pg_attribute_aligned(a) __attribute__((aligned(a))) #define pg_attribute_noreturn() __attribute__((noreturn)) #define pg_attribute_packed() __attribute__((packed)) @@ -212,8 +210,8 @@ * choose not to. But, if possible, don't force inlining in unoptimized * debug builds. */ -#if (defined(__GNUC__) && __GNUC__ > 3 && defined(__OPTIMIZE__)) || defined(__SUNPRO_C) || defined(__IBMC__) -/* GCC > 3, Sunpro and XLC support always_inline via __attribute__ */ +#if (defined(__GNUC__) && __GNUC__ > 3 && defined(__OPTIMIZE__)) || defined(__SUNPRO_C) +/* GCC > 3 and Sunpro support always_inline via __attribute__ */ #define pg_attribute_always_inline __attribute__((always_inline)) inline #elif defined(_MSC_VER) /* MSVC has a special keyword for this */ @@ -229,8 +227,8 @@ * for proper cost attribution. Note that unlike the pg_attribute_XXX macros * above, this should be placed before the function's return type and name. */ -/* GCC, Sunpro and XLC support noinline via __attribute__ */ -#if (defined(__GNUC__) && __GNUC__ > 2) || defined(__SUNPRO_C) || defined(__IBMC__) +/* GCC and Sunpro support noinline via __attribute__ */ +#if (defined(__GNUC__) && __GNUC__ > 2) || defined(__SUNPRO_C) #define pg_noinline __attribute__((noinline)) /* msvc via declspec */ #elif defined(_MSC_VER) diff --git a/src/include/port/aix.h b/src/include/port/aix.h deleted file mode 100644 index 5b1159c5785..00000000000 --- a/src/include/port/aix.h +++ /dev/null @@ -1,14 +0,0 @@ -/* - * src/include/port/aix.h - */ -#define CLASS_CONFLICT -#define DISABLE_XOPEN_NLS - -/* - * "IBM XL C/C++ for AIX, V12.1" miscompiles, for 32-bit, some inline - * expansions of ginCompareItemPointers() "long long" arithmetic. To take - * advantage of inlining, build a 64-bit PostgreSQL. - */ -#if defined(__ILP32__) && defined(__IBMC__) -#define PG_FORCE_DISABLE_INLINE -#endif diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h index bf151037f72..504349080d4 100644 --- a/src/include/port/atomics.h +++ b/src/include/port/atomics.h @@ -84,11 +84,9 @@ * using compiler intrinsics are a good idea. */ /* - * gcc or compatible, including clang and icc. Exclude xlc. The ppc64le "IBM - * XL C/C++ for Linux, V13.1.2" emulates gcc, but __sync_lock_test_and_set() - * of one-byte types elicits SIGSEGV. That bug was gone by V13.1.5 (2016-12). + * gcc or compatible, including clang and icc. */ -#if (defined(__GNUC__) || defined(__INTEL_COMPILER)) && !(defined(__IBMC__) || defined(__IBMCPP__)) +#if defined(__GNUC__) || defined(__INTEL_COMPILER) #include "port/atomics/generic-gcc.h" #elif defined(_MSC_VER) #include "port/atomics/generic-msvc.h" diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h index 69582f4ae71..29ac6cdcd92 100644 --- a/src/include/storage/s_lock.h +++ b/src/include/storage/s_lock.h @@ -414,12 +414,6 @@ typedef unsigned int slock_t; * an isync is a sufficient synchronization barrier after a lwarx/stwcx loop. * But if the spinlock is in ordinary memory, we can use lwsync instead for * better performance. - * - * Ordinarily, we'd code the branches here using GNU-style local symbols, that - * is "1f" referencing "1:" and so on. But some people run gcc on AIX with - * IBM's assembler as backend, and IBM's assembler doesn't do local symbols. - * So hand-code the branch offsets; fortunately, all PPC instructions are - * exactly 4 bytes each, so it's not too hard to count. */ static __inline__ int tas(volatile slock_t *lock) @@ -430,15 +424,17 @@ tas(volatile slock_t *lock) __asm__ __volatile__( " lwarx %0,0,%3,1 \n" " cmpwi %0,0 \n" -" bne $+16 \n" /* branch to li %1,1 */ +" bne 1f \n" " addi %0,%0,1 \n" " stwcx. %0,0,%3 \n" -" beq $+12 \n" /* branch to lwsync */ +" beq 2f \n" +"1: \n" " li %1,1 \n" -" b $+12 \n" /* branch to end of asm sequence */ +" b 3f \n" +"2: \n" " lwsync \n" " li %1,0 \n" - +"3: \n" : "=&b"(_t), "=r"(_res), "+m"(*lock) : "r"(lock) : "memory", "cc"); @@ -666,21 +662,6 @@ tas(volatile slock_t *lock) #if !defined(HAS_TEST_AND_SET) /* We didn't trigger above, let's try here */ -#if defined(_AIX) /* AIX */ -/* - * AIX (POWER) - */ -#define HAS_TEST_AND_SET - -#include <sys/atomic_op.h> - -typedef int slock_t; - -#define TAS(lock) _check_lock((slock_t *) (lock), 0, 1) -#define S_UNLOCK(lock) _clear_lock((slock_t *) (lock), 0) -#endif /* _AIX */ - - /* These are in sunstudio_(sparc|x86).s */ #if defined(__SUNPRO_C) && (defined(__i386) || defined(__x86_64__) || defined(__sparc__) || defined(__sparc)) diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile index 083ca6f4cce..fe2af575c5d 100644 --- a/src/interfaces/libpq/Makefile +++ b/src/interfaces/libpq/Makefile @@ -114,7 +114,7 @@ backend_src = $(top_srcdir)/src/backend # coding rule. libpq-refs-stamp: $(shlib) ifneq ($(enable_coverage), yes) -ifeq (,$(filter aix solaris,$(PORTNAME))) +ifeq (,$(filter solaris,$(PORTNAME))) @if nm -A -u $< 2>/dev/null | grep -v -e __cxa_atexit -e __tsan_func_exit | grep exit; then \ echo 'libpq must not be calling any function which invokes exit'; exit 1; \ fi diff --git a/src/interfaces/libpq/meson.build b/src/interfaces/libpq/meson.build index a47b6f425dd..be6fadaea23 100644 --- a/src/interfaces/libpq/meson.build +++ b/src/interfaces/libpq/meson.build @@ -54,9 +54,8 @@ libpq_c_args = ['-DSO_MAJOR_VERSION=5'] # libpq_st, and {pgport,common}_shlib for libpq_sh # # We could try to avoid building the source files twice, but it probably adds -# more complexity than its worth (AIX doesn't support link_whole yet, reusing -# object files requires also linking to the library on windows or breaks -# precompiled headers). +# more complexity than its worth (reusing object files requires also linking +# to the library on windows or breaks precompiled headers). libpq_st = static_library('libpq', libpq_sources, include_directories: [libpq_inc], diff --git a/src/makefiles/Makefile.aix b/src/makefiles/Makefile.aix deleted file mode 100644 index dd16a7a0378..00000000000 --- a/src/makefiles/Makefile.aix +++ /dev/null @@ -1,39 +0,0 @@ -# MAKE_EXPORTS is required for svr4 loaders that want a file of -# symbol names to tell them what to export/import. -MAKE_EXPORTS= true - -# -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 - -# 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 - -rpath = -Wl,-blibpath:'$(rpathdir)$(libpath)' - -LDFLAGS_SL += -Wl,-bnoentry -Wl,-H512 -Wl,-bM:SRE - -# 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 - -# env var name to use in place of LD_LIBRARY_PATH -ld_library_path_var = LIBPATH - - -POSTGRES_IMP= postgres.imp - -ifdef PGXS -BE_DLLLIBS= -Wl,-bI:$(pkglibdir)/$(POSTGRES_IMP) -else -BE_DLLLIBS= -Wl,-bI:$(top_builddir)/src/backend/$(POSTGRES_IMP) -endif - -MKLDEXPORT_DIR=src/backend/port/aix -MKLDEXPORT=$(top_srcdir)/$(MKLDEXPORT_DIR)/mkldexport.sh - -%$(DLSUFFIX): %.o - $(CC) $(CFLAGS) $*.o $(LDFLAGS) $(LDFLAGS_SL) -o $@ $(BE_DLLLIBS) diff --git a/src/port/README b/src/port/README index 97f18a62338..ed5c54a72fa 100644 --- a/src/port/README +++ b/src/port/README @@ -28,5 +28,5 @@ applications. from libpgport are linked first. This avoids having applications dependent on symbols that are _used_ by libpq, but not intended to be exported by libpq. libpq's libpgport usage changes over time, so such a -dependency is a problem. Windows, Linux, AIX, and macOS use an export +dependency is a problem. Windows, Linux, and macOS use an export list to control the symbols exported by libpq. diff --git a/src/port/strerror.c b/src/port/strerror.c index 1070a49802e..4918ba821c1 100644 --- a/src/port/strerror.c +++ b/src/port/strerror.c @@ -214,10 +214,8 @@ get_errno_symbol(int errnum) return "ENOTCONN"; case ENOTDIR: return "ENOTDIR"; -#if defined(ENOTEMPTY) && (ENOTEMPTY != EEXIST) /* same code on AIX */ case ENOTEMPTY: return "ENOTEMPTY"; -#endif case ENOTSOCK: return "ENOTSOCK"; #ifdef ENOTSUP diff --git a/src/template/aix b/src/template/aix deleted file mode 100644 index 47fa8990a7c..00000000000 --- a/src/template/aix +++ /dev/null @@ -1,25 +0,0 @@ -# src/template/aix - -# Set default options if using xlc. This formerly included -qsrcmsg, but that -# option elicits internal compiler errors from xlc v16.1.0. Note: configure -# will add -qnoansialias if the compiler accepts it, even if user specifies a -# non-default CFLAGS setting. -if test "$GCC" != yes ; then - case $host_os in - *) - CFLAGS="-O2 -qmaxmem=16384" - ;; - esac - - # Due to a compiler bug, see [email protected] for details, - # force restrict not to be used when compiling with xlc. - FORCE_DISABLE_RESTRICT=yes -fi - -# Extra CFLAGS for code that will go into a shared library -CFLAGS_SL="" - -# Native memset() is faster, tested on: -# AIX 5.1 and 5.2, XLC 6.0 (IBM's cc) -# AIX 5.3 ML3, gcc 4.0.1 -MEMSET_LOOP_LIMIT=0 diff --git a/src/test/regress/Makefile b/src/test/regress/Makefile index 7c665ff892d..6409a485e84 100644 --- a/src/test/regress/Makefile +++ b/src/test/regress/Makefile @@ -7,11 +7,6 @@ # GNU make uses a make file named "GNUmakefile" in preference to "Makefile" # if it exists. Postgres is shipped with a "GNUmakefile". - -# AIX make defaults to building *every* target of the first rule. Start with -# a single-target, empty rule to make the other targets non-default. -all: - all install clean check installcheck: @echo "You must use GNU make to use Postgres. It may be installed" @echo "on your system with the name 'gmake'." diff --git a/src/test/regress/expected/sanity_check.out b/src/test/regress/expected/sanity_check.out index c5c675b7508..82972b70e6f 100644 --- a/src/test/regress/expected/sanity_check.out +++ b/src/test/regress/expected/sanity_check.out @@ -26,12 +26,15 @@ SELECT relname, relkind (0 rows) -- --- When ALIGNOF_DOUBLE==4 (e.g. AIX), the C ABI may impose 8-byte alignment on +-- When MAXIMUM_ALIGNOF==8 but ALIGNOF_DOUBLE==4, the C ABI may impose 8-byte alignment -- some of the C types that correspond to TYPALIGN_DOUBLE SQL types. To ensure -- catalog C struct layout matches catalog tuple layout, arrange for the tuple -- offset of each fixed-width, attalign='d' catalog column to be divisible by 8 -- unconditionally. Keep such columns before the first NameData column of the -- catalog, since packagers can override NAMEDATALEN to an odd number. +-- (XXX: I'm not sure if any of the supported platforms have MAXIMUM_ALIGNOF==8 and +-- ALIGNOF_DOUBLE==4. Perhaps we should just require that +-- ALIGNOF_DOUBLE==MAXIMUM_ALIGNOF) -- WITH check_columns AS ( SELECT relname, attname, diff --git a/src/test/regress/sql/sanity_check.sql b/src/test/regress/sql/sanity_check.sql index 7f338d191c6..2e9d5ebef3f 100644 --- a/src/test/regress/sql/sanity_check.sql +++ b/src/test/regress/sql/sanity_check.sql @@ -21,12 +21,15 @@ SELECT relname, relkind AND relfilenode <> 0; -- --- When ALIGNOF_DOUBLE==4 (e.g. AIX), the C ABI may impose 8-byte alignment on +-- When MAXIMUM_ALIGNOF==8 but ALIGNOF_DOUBLE==4, the C ABI may impose 8-byte alignment -- some of the C types that correspond to TYPALIGN_DOUBLE SQL types. To ensure -- catalog C struct layout matches catalog tuple layout, arrange for the tuple -- offset of each fixed-width, attalign='d' catalog column to be divisible by 8 -- unconditionally. Keep such columns before the first NameData column of the -- catalog, since packagers can override NAMEDATALEN to an odd number. +-- (XXX: I'm not sure if any of the supported platforms have MAXIMUM_ALIGNOF==8 and +-- ALIGNOF_DOUBLE==4. Perhaps we should just require that +-- ALIGNOF_DOUBLE==MAXIMUM_ALIGNOF) -- WITH check_columns AS ( SELECT relname, attname, diff --git a/src/tools/gen_export.pl b/src/tools/gen_export.pl index 888c8a197a9..d9fdaaaf6d0 100644 --- a/src/tools/gen_export.pl +++ b/src/tools/gen_export.pl @@ -16,12 +16,11 @@ GetOptions( 'input:s' => \$input, 'output:s' => \$output) or die "wrong arguments"; -if (not( $format eq 'aix' - or $format eq 'darwin' +if (not( $format eq 'darwin' or $format eq 'gnu' or $format eq 'win')) { - die "$0: $format is not yet handled (only aix, darwin, gnu, win are)\n"; + die "$0: $format is not yet handled (only darwin, gnu, win are)\n"; } open(my $input_handle, '<', $input) @@ -56,11 +55,7 @@ while (<$input_handle>) } elsif (/^(\S+)\s+(\S+)/) { - if ($format eq 'aix') - { - print $output_handle "$1\n"; - } - elsif ($format eq 'darwin') + if ($format eq 'darwin') { print $output_handle "_$1\n"; } diff --git a/src/tools/pginclude/cpluspluscheck b/src/tools/pginclude/cpluspluscheck index 7edfc44b49a..a46ff52cc1b 100755 --- a/src/tools/pginclude/cpluspluscheck +++ b/src/tools/pginclude/cpluspluscheck @@ -61,7 +61,6 @@ do # These files are platform-specific, and c.h will include the # one that's relevant for our current platform anyway. - test "$f" = src/include/port/aix.h && continue test "$f" = src/include/port/cygwin.h && continue test "$f" = src/include/port/darwin.h && continue test "$f" = src/include/port/freebsd.h && continue diff --git a/src/tools/pginclude/headerscheck b/src/tools/pginclude/headerscheck index 84b892b5c51..0e2d7f537ef 100755 --- a/src/tools/pginclude/headerscheck +++ b/src/tools/pginclude/headerscheck @@ -57,7 +57,6 @@ do # These files are platform-specific, and c.h will include the # one that's relevant for our current platform anyway. - test "$f" = src/include/port/aix.h && continue test "$f" = src/include/port/cygwin.h && continue test "$f" = src/include/port/darwin.h && continue test "$f" = src/include/port/freebsd.h && continue -- 2.39.2 ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-27 20:45 Tom Lane <[email protected]> parent: Heikki Linnakangas <[email protected]> 3 siblings, 0 replies; 34+ messages in thread From: Tom Lane @ 2024-02-27 20:45 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Noah Misch <[email protected]>; Thomas Munro <[email protected]>; Andres Freund <[email protected]>; Peter Smith <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> Heikki Linnakangas <[email protected]> writes: > What do y'all think of adding a check for > ALIGNOF_DOUBLE==MAXIMUM_ALIGNOF to configure.ac and meson.build? It's > not a requirement today, but I believe AIX was the only platform where > that was not true. With AIX gone, that combination won't be tested, and > we will probably break it sooner or later. +1, and then probably revert the whole test addition of 79b716cfb7a. I did a quick scrape of the buildfarm, and identified these as the only animals reporting ALIGNOF_DOUBLE less than 8: $ grep 'alignment of double' alignments | grep -v ' 8$' hornet | 2024-02-22 16:26:16 | checking alignment of double... 4 lapwing | 2024-02-27 12:40:15 | checking alignment of double... (cached) 4 mandrill | 2024-02-19 01:03:47 | checking alignment of double... 4 sungazer | 2024-02-21 00:22:48 | checking alignment of double... 4 tern | 2024-02-22 13:25:12 | checking alignment of double... 4 With AIX out of the picture, lapwing will be the only remaining animal testing MAXALIGN less than 8. That seems like a single point of failure ... should we spin up another couple 32-bit animals? I had supposed that my faithful old PPC animal mamba was helping to check this, but I see that under NetBSD it's joined the ALIGNOF_DOUBLE==8 crowd. regards, tom lane ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-27 22:30 Thomas Munro <[email protected]> parent: Heikki Linnakangas <[email protected]> 3 siblings, 0 replies; 34+ messages in thread From: Thomas Munro @ 2024-02-27 22:30 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Andres Freund <[email protected]>; Peter Smith <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Wed, Feb 28, 2024 at 9:24 AM Heikki Linnakangas <[email protected]> wrote: > Here's a patch to fully remove AIX support. --- a/doc/src/sgml/installation.sgml +++ b/doc/src/sgml/installation.sgml @@ -3401,7 +3401,7 @@ export MANPATH <para> <productname>PostgreSQL</productname> can be expected to work on current versions of these operating systems: Linux, Windows, - FreeBSD, OpenBSD, NetBSD, DragonFlyBSD, macOS, AIX, Solaris, and illumos. + FreeBSD, OpenBSD, NetBSD, DragonFlyBSD, macOS, Solaris, and illumos. There is also a little roll-of-honour of operating systems we used to support, just a couple of paragraphs down, where AIX should appear. ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-28 03:52 Noah Misch <[email protected]> parent: Heikki Linnakangas <[email protected]> 3 siblings, 0 replies; 34+ messages in thread From: Noah Misch @ 2024-02-28 03:52 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Andres Freund <[email protected]>; Peter Smith <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Wed, Feb 28, 2024 at 12:24:01AM +0400, Heikki Linnakangas wrote: > Here's a patch to fully remove AIX support. > Subject: [PATCH 1/1] Remove AIX support > > There isn't a lot of user demand for AIX support, no one has stepped > up to the plate to properly maintain it, so it's best to remove it Regardless of how someone were to step up to maintain it, we'd be telling them such contributions have negative value and must stop. We're expelling AIX due to low demand, compiler bugs, its ABI, and its shlib symbol export needs. > altogether. AIX is still supported for stable versions. > > The acute issue that triggered this decision was that after commit > 8af2565248, the AIX buildfarm members have been hitting this > assertion: > > TRAP: failed Assert("(uintptr_t) buffer == TYPEALIGN(PG_IO_ALIGN_SIZE, buffer)"), File: "md.c", Line: 472, PID: 2949728 > > Apperently the "pg_attribute_aligned(a)" attribute doesn't work on AIX > (linker?) for values larger than PG_IO_ALIGN_SIZE. No; see https://postgr.es/m/20240225194322.a5%40rfd.leadboat.com ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-02-28 09:26 Andres Freund <[email protected]> parent: Heikki Linnakangas <[email protected]> 3 siblings, 0 replies; 34+ messages in thread From: Andres Freund @ 2024-02-28 09:26 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Michael Paquier <[email protected]>; Robert Haas <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Thomas Munro <[email protected]>; Peter Smith <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> Hi, On 2024-02-28 00:24:01 +0400, Heikki Linnakangas wrote: > Here's a patch to fully remove AIX support. Thomas mentioned to me that cfbot failed with this applied: https://cirrus-ci.com/task/6348635416297472 https://api.cirrus-ci.com/v1/artifact/task/6348635416297472/log/tmp_install/log/initdb-template.log initdb: error while loading shared libraries: libpq.so.5: cannot open shared object file: No such file or directory While I couldn't reproduce the failure, I did notice that locally with the patch applied, system libpq ended up getting used. Which isn't pre-installed in the CI environment, explaining the failure. The problem is due to this hunk: > @@ -401,10 +376,6 @@ install-lib-static: $(stlib) installdirs-lib > > install-lib-shared: $(shlib) installdirs-lib > ifdef soname > -# we don't install $(shlib) on AIX > -# (see http://archives.postgresql.org/message-id/[email protected]...) > -ifneq ($(PORTNAME), aix) > - $(INSTALL_SHLIB) $< '$(DESTDIR)$(libdir)/$(shlib)' > ifneq ($(PORTNAME), cygwin) > ifneq ($(PORTNAME), win32) > ifneq ($(shlib), $(shlib_major)) So the versioned name didn't end up getting installed anymore, leading to broken symlinks in the install directory. > diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl > index 86cc01a640b..fc6b00224f6 100644 > --- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl > +++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl > @@ -400,9 +400,6 @@ is(scalar(@tblspc_tars), 1, 'one tablespace tar was created'); > SKIP: > { > my $tar = $ENV{TAR}; > - # don't check for a working tar here, to accommodate various odd > - # cases such as AIX. If tar doesn't work the init_from_backup below > - # will fail. > skip "no tar program available", 1 > if (!defined $tar || $tar eq ''); Maybe better to not remove the whole comment, just the reference to AIX? > diff --git a/src/test/regress/sql/sanity_check.sql b/src/test/regress/sql/sanity_check.sql > index 7f338d191c6..2e9d5ebef3f 100644 > --- a/src/test/regress/sql/sanity_check.sql > +++ b/src/test/regress/sql/sanity_check.sql > @@ -21,12 +21,15 @@ SELECT relname, relkind > AND relfilenode <> 0; > > -- > --- When ALIGNOF_DOUBLE==4 (e.g. AIX), the C ABI may impose 8-byte alignment on > +-- When MAXIMUM_ALIGNOF==8 but ALIGNOF_DOUBLE==4, the C ABI may impose 8-byte alignment > -- some of the C types that correspond to TYPALIGN_DOUBLE SQL types. To ensure > -- catalog C struct layout matches catalog tuple layout, arrange for the tuple > -- offset of each fixed-width, attalign='d' catalog column to be divisible by 8 > -- unconditionally. Keep such columns before the first NameData column of the > -- catalog, since packagers can override NAMEDATALEN to an odd number. > +-- (XXX: I'm not sure if any of the supported platforms have MAXIMUM_ALIGNOF==8 and > +-- ALIGNOF_DOUBLE==4. Perhaps we should just require that > +-- ALIGNOF_DOUBLE==MAXIMUM_ALIGNOF) > -- > WITH check_columns AS ( > SELECT relname, attname, I agree, this should be an error, and we should then remove the test. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 34+ messages in thread
* Remove AIX Support (was: Re: Relation bulk write facility) @ 2024-02-29 08:13 Michael Banck <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 34+ messages in thread From: Michael Banck @ 2024-02-29 08:13 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Noah Misch <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> Hi, On Sat, Feb 24, 2024 at 01:29:36PM -0800, Andres Freund wrote: > Let's just drop AIX. This isn't the only alignment issue we've found and the > solution for those isn't so much a fix as forcing everyone to carefully only > look into one direction and not notice the cliffs to either side. While I am not against dropping AIX (and certainly won't step up to maintain it just for fun), I don't think burying this inside some "Relation bulk write facility" thread is helpful; I have changed the thread title as a first step. The commit message says there is not a lot of user demand and that might be right, but contrary to other fringe OSes that got removed like HPPA or Irix, I believe Postgres on AIX is still used in production and if so, probably in a mission-critical manner at some old-school institutions (in fact, one of our customers does just that) and not as a thought-experiment. It is probably well-known among Postgres hackers that AIX support is problematic/a burden, but the current users might not be aware of this. Not sure what to do about this (especially now that this has been committed), maybe there should have been be a public deprecation notice first for v17... On the other hand, that might not work if important features like direct-IO would have to be bumped from v17 just because of AIX. I posted about this on Twitter and Mastodon to see whether anybody complains and did not get a lot of feedback. In any case, users will have a couple of years to migrate as usual if they upgrade to v16. Michael ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Remove AIX Support (was: Re: Relation bulk write facility) @ 2024-02-29 08:40 Daniel Gustafsson <[email protected]> parent: Michael Banck <[email protected]> 0 siblings, 0 replies; 34+ messages in thread From: Daniel Gustafsson @ 2024-02-29 08:40 UTC (permalink / raw) To: Michael Banck <[email protected]>; +Cc: Andres Freund <[email protected]>; Noah Misch <[email protected]>; Thomas Munro <[email protected]>; Heikki Linnakangas <[email protected]>; Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> > On 29 Feb 2024, at 09:13, Michael Banck <[email protected]> wrote: > In any case, users will have a couple of years to migrate as usual if > they upgrade to v16. As you say, there are many years left of AIX being supported so there is plenty of runway for planning a migration. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-07-01 20:52 Noah Misch <[email protected]> parent: Heikki Linnakangas <[email protected]> 2 siblings, 1 reply; 34+ messages in thread From: Noah Misch @ 2024-07-01 20:52 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Fri, Feb 23, 2024 at 04:27:34PM +0200, Heikki Linnakangas wrote: > Committed this. Thanks everyone! Commit 8af2565 wrote: > --- /dev/null > +++ b/src/backend/storage/smgr/bulk_write.c > +/* > + * Finish bulk write operation. > + * > + * This WAL-logs and flushes any remaining pending writes to disk, and fsyncs > + * the relation if needed. > + */ > +void > +smgr_bulk_finish(BulkWriteState *bulkstate) > +{ > + /* WAL-log and flush any remaining pages */ > + smgr_bulk_flush(bulkstate); > + > + /* > + * When we wrote out the pages, we passed skipFsync=true to avoid the > + * overhead of registering all the writes with the checkpointer. Register > + * the whole relation now. > + * > + * There is one hole in that idea: If a checkpoint occurred while we were > + * writing the pages, it already missed fsyncing the pages we had written > + * before the checkpoint started. A crash later on would replay the WAL > + * starting from the checkpoint, therefore it wouldn't replay our earlier > + * WAL records. So if a checkpoint started after the bulk write, fsync > + * the files now. > + */ > + if (!SmgrIsTemp(bulkstate->smgr)) > + { Shouldn't this be "if (bulkstate->use_wal)"? The GetRedoRecPtr()-based decision is irrelevant to the !wal case. Either we don't need fsync at all (TEMP or UNLOGGED) or smgrDoPendingSyncs() will do it (wal_level=minimal). I don't see any functional problem, but this likely arranges for an unnecessary sync when a checkpoint starts between mdcreate() and here. (The mdcreate() sync may also be unnecessary, but that's longstanding.) > + /* > + * Prevent a checkpoint from starting between the GetRedoRecPtr() and > + * smgrregistersync() calls. > + */ > + Assert((MyProc->delayChkptFlags & DELAY_CHKPT_START) == 0); > + MyProc->delayChkptFlags |= DELAY_CHKPT_START; > + > + if (bulkstate->start_RedoRecPtr != GetRedoRecPtr()) > + { > + /* > + * A checkpoint occurred and it didn't know about our writes, so > + * fsync() the relation ourselves. > + */ > + MyProc->delayChkptFlags &= ~DELAY_CHKPT_START; > + smgrimmedsync(bulkstate->smgr, bulkstate->forknum); > + elog(DEBUG1, "flushed relation because a checkpoint occurred concurrently"); > + } > + else > + { > + smgrregistersync(bulkstate->smgr, bulkstate->forknum); > + MyProc->delayChkptFlags &= ~DELAY_CHKPT_START; > + } > + } > +} This is an elegant optimization. ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-07-01 21:53 Heikki Linnakangas <[email protected]> parent: Noah Misch <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Heikki Linnakangas @ 2024-07-01 21:53 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> Thanks for poking at this! On 01/07/2024 23:52, Noah Misch wrote: > Commit 8af2565 wrote: >> --- /dev/null >> +++ b/src/backend/storage/smgr/bulk_write.c > >> +/* >> + * Finish bulk write operation. >> + * >> + * This WAL-logs and flushes any remaining pending writes to disk, and fsyncs >> + * the relation if needed. >> + */ >> +void >> +smgr_bulk_finish(BulkWriteState *bulkstate) >> +{ >> + /* WAL-log and flush any remaining pages */ >> + smgr_bulk_flush(bulkstate); >> + >> + /* >> + * When we wrote out the pages, we passed skipFsync=true to avoid the >> + * overhead of registering all the writes with the checkpointer. Register >> + * the whole relation now. >> + * >> + * There is one hole in that idea: If a checkpoint occurred while we were >> + * writing the pages, it already missed fsyncing the pages we had written >> + * before the checkpoint started. A crash later on would replay the WAL >> + * starting from the checkpoint, therefore it wouldn't replay our earlier >> + * WAL records. So if a checkpoint started after the bulk write, fsync >> + * the files now. >> + */ >> + if (!SmgrIsTemp(bulkstate->smgr)) >> + { > > Shouldn't this be "if (bulkstate->use_wal)"? The GetRedoRecPtr()-based > decision is irrelevant to the !wal case. Either we don't need fsync at all > (TEMP or UNLOGGED) or smgrDoPendingSyncs() will do it (wal_level=minimal). The point of GetRedoRecPtr() is to detect if a checkpoint has started concurrently. It works for that purpose whether or not the bulk load is WAL-logged. It is not compared with the LSNs of WAL records written by the bulk load. Unlogged tables do need to be fsync'd. The scenario is: 1. Bulk load an unlogged table. 2. Shut down Postgres cleanly 3. Pull power plug from server, and restart. We talked about this earlier in the "Unlogged relation copy is not fsync'd" thread [1]. I had already forgotten about that; that bug actually still exists in back branches, and we should fix it.. [1] https://www.postgresql.org/message-id/flat/65e94fc8-ce1d-dd02-3be3-fda0fe8f2965%40iki.fi > I don't see any functional problem, but this likely arranges for an > unnecessary sync when a checkpoint starts between mdcreate() and > here. (The mdcreate() sync may also be unnecessary, but that's > longstanding.) Hmm, yes we might do two fsyncs() with wal_level=minimal, unnecessarily. It seems hard to eliminate the redundancy. smgr_bulk_finish() could skip the fsync, if it knew that smgrDoPendingSyncs() will do it later. However, smgrDoPendingSyncs() might also decide to WAL-log the relation instead of fsyncing it, and in that case we do still need the fsync. Fortunately, fsync() on a file that's already flushed to disk is pretty cheap. -- Heikki Linnakangas Neon (https://neon.tech) ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-07-01 23:24 Noah Misch <[email protected]> parent: Heikki Linnakangas <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Noah Misch @ 2024-07-01 23:24 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Tue, Jul 02, 2024 at 12:53:05AM +0300, Heikki Linnakangas wrote: > On 01/07/2024 23:52, Noah Misch wrote: > > Commit 8af2565 wrote: > > > --- /dev/null > > > +++ b/src/backend/storage/smgr/bulk_write.c > > > > > +/* > > > + * Finish bulk write operation. > > > + * > > > + * This WAL-logs and flushes any remaining pending writes to disk, and fsyncs > > > + * the relation if needed. > > > + */ > > > +void > > > +smgr_bulk_finish(BulkWriteState *bulkstate) > > > +{ > > > + /* WAL-log and flush any remaining pages */ > > > + smgr_bulk_flush(bulkstate); > > > + > > > + /* > > > + * When we wrote out the pages, we passed skipFsync=true to avoid the > > > + * overhead of registering all the writes with the checkpointer. Register > > > + * the whole relation now. > > > + * > > > + * There is one hole in that idea: If a checkpoint occurred while we were > > > + * writing the pages, it already missed fsyncing the pages we had written > > > + * before the checkpoint started. A crash later on would replay the WAL > > > + * starting from the checkpoint, therefore it wouldn't replay our earlier > > > + * WAL records. So if a checkpoint started after the bulk write, fsync > > > + * the files now. > > > + */ > > > + if (!SmgrIsTemp(bulkstate->smgr)) > > > + { > > > > Shouldn't this be "if (bulkstate->use_wal)"? The GetRedoRecPtr()-based > > decision is irrelevant to the !wal case. Either we don't need fsync at all > > (TEMP or UNLOGGED) or smgrDoPendingSyncs() will do it (wal_level=minimal). > > The point of GetRedoRecPtr() is to detect if a checkpoint has started > concurrently. It works for that purpose whether or not the bulk load is > WAL-logged. It is not compared with the LSNs of WAL records written by the > bulk load. I think the significance of start_RedoRecPtr is it preceding all records needed to recreate the bulk write. If start_RedoRecPtr==GetRedoRecPtr() and we crash after commit, we're indifferent to whether the rel gets synced at a checkpoint before that crash or rebuilt from WAL after that crash. If start_RedoRecPtr!=GetRedoRecPtr(), some WAL of the bulk write is already deleted, so only smgrimmedsync() suffices. Overall, while it is not compared with LSNs in WAL records, it's significant only to the extent that such a WAL record exists. What am I missing? > Unlogged tables do need to be fsync'd. The scenario is: > > 1. Bulk load an unlogged table. > 2. Shut down Postgres cleanly > 3. Pull power plug from server, and restart. > > We talked about this earlier in the "Unlogged relation copy is not fsync'd" > thread [1]. I had already forgotten about that; that bug actually still > exists in back branches, and we should fix it.. > > [1] https://www.postgresql.org/message-id/flat/65e94fc8-ce1d-dd02-3be3-fda0fe8f2965%40iki.fi Ah, that's right. I agree this code suffices for unlogged. As a further optimization, it would be valid to ignore GetRedoRecPtr() for unlogged and always call smgrregistersync(). (For any rel, smgrimmedsync() improves on smgrregistersync() only if we fail to reach the shutdown checkpoint. Without a shutdown checkpoint, unlogged rels get reset anyway.) > > I don't see any functional problem, but this likely arranges for an > > unnecessary sync when a checkpoint starts between mdcreate() and > > here. (The mdcreate() sync may also be unnecessary, but that's > > longstanding.) > Hmm, yes we might do two fsyncs() with wal_level=minimal, unnecessarily. It > seems hard to eliminate the redundancy. smgr_bulk_finish() could skip the > fsync, if it knew that smgrDoPendingSyncs() will do it later. However, > smgrDoPendingSyncs() might also decide to WAL-log the relation instead of > fsyncing it, and in that case we do still need the fsync. We do not need the fsync in the "WAL-log the relation instead" case; see https://postgr.es/m/[email protected] So maybe like this: if (use_wal) /* includes init forks */ current logic; else if (unlogged) smgrregistersync; /* else temp || (permanent && wal_level=minimal): nothing to do */ > Fortunately, fsync() on a file that's already flushed to disk is pretty > cheap. Yep. I'm more concerned about future readers wondering why the function is using LSNs to decide what to do about data that doesn't appear in WAL. A comment could be another way to fix that, though. ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-07-02 11:42 Heikki Linnakangas <[email protected]> parent: Noah Misch <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Heikki Linnakangas @ 2024-07-02 11:42 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On 02/07/2024 02:24, Noah Misch wrote: > On Tue, Jul 02, 2024 at 12:53:05AM +0300, Heikki Linnakangas wrote: >> On 01/07/2024 23:52, Noah Misch wrote: >>> Commit 8af2565 wrote: >>>> --- /dev/null >>>> +++ b/src/backend/storage/smgr/bulk_write.c >>> >>>> +/* >>>> + * Finish bulk write operation. >>>> + * >>>> + * This WAL-logs and flushes any remaining pending writes to disk, and fsyncs >>>> + * the relation if needed. >>>> + */ >>>> +void >>>> +smgr_bulk_finish(BulkWriteState *bulkstate) >>>> +{ >>>> + /* WAL-log and flush any remaining pages */ >>>> + smgr_bulk_flush(bulkstate); >>>> + >>>> + /* >>>> + * When we wrote out the pages, we passed skipFsync=true to avoid the >>>> + * overhead of registering all the writes with the checkpointer. Register >>>> + * the whole relation now. >>>> + * >>>> + * There is one hole in that idea: If a checkpoint occurred while we were >>>> + * writing the pages, it already missed fsyncing the pages we had written >>>> + * before the checkpoint started. A crash later on would replay the WAL >>>> + * starting from the checkpoint, therefore it wouldn't replay our earlier >>>> + * WAL records. So if a checkpoint started after the bulk write, fsync >>>> + * the files now. >>>> + */ >>>> + if (!SmgrIsTemp(bulkstate->smgr)) >>>> + { >>> >>> Shouldn't this be "if (bulkstate->use_wal)"? The GetRedoRecPtr()-based >>> decision is irrelevant to the !wal case. Either we don't need fsync at all >>> (TEMP or UNLOGGED) or smgrDoPendingSyncs() will do it (wal_level=minimal). >> >> The point of GetRedoRecPtr() is to detect if a checkpoint has started >> concurrently. It works for that purpose whether or not the bulk load is >> WAL-logged. It is not compared with the LSNs of WAL records written by the >> bulk load. > > I think the significance of start_RedoRecPtr is it preceding all records > needed to recreate the bulk write. If start_RedoRecPtr==GetRedoRecPtr() and > we crash after commit, we're indifferent to whether the rel gets synced at a > checkpoint before that crash or rebuilt from WAL after that crash. If > start_RedoRecPtr!=GetRedoRecPtr(), some WAL of the bulk write is already > deleted, so only smgrimmedsync() suffices. Overall, while it is not compared > with LSNs in WAL records, it's significant only to the extent that such a WAL > record exists. What am I missing? You're right. You pointed out below that we don't need to register or immediately fsync the relation if it was not WAL-logged, I missed that. In the alternative universe that we did need to fsync() even in !use_wal case, the point of the start_RedoRecPtr==GetRedoRecPtr() was to detect whether the last checkpoint "missed" fsyncing the files that we wrote. But the point is moot now. >> Unlogged tables do need to be fsync'd. The scenario is: >> >> 1. Bulk load an unlogged table. >> 2. Shut down Postgres cleanly >> 3. Pull power plug from server, and restart. >> >> We talked about this earlier in the "Unlogged relation copy is not fsync'd" >> thread [1]. I had already forgotten about that; that bug actually still >> exists in back branches, and we should fix it.. >> >> [1] https://www.postgresql.org/message-id/flat/65e94fc8-ce1d-dd02-3be3-fda0fe8f2965%40iki.fi > > Ah, that's right. I agree this code suffices for unlogged. As a further > optimization, it would be valid to ignore GetRedoRecPtr() for unlogged and > always call smgrregistersync(). (For any rel, smgrimmedsync() improves on > smgrregistersync() only if we fail to reach the shutdown checkpoint. Without > a shutdown checkpoint, unlogged rels get reset anyway.) > >>> I don't see any functional problem, but this likely arranges for an >>> unnecessary sync when a checkpoint starts between mdcreate() and >>> here. (The mdcreate() sync may also be unnecessary, but that's >>> longstanding.) >> Hmm, yes we might do two fsyncs() with wal_level=minimal, unnecessarily. It >> seems hard to eliminate the redundancy. smgr_bulk_finish() could skip the >> fsync, if it knew that smgrDoPendingSyncs() will do it later. However, >> smgrDoPendingSyncs() might also decide to WAL-log the relation instead of >> fsyncing it, and in that case we do still need the fsync. > > We do not need the fsync in the "WAL-log the relation instead" case; see > https://postgr.es/m/[email protected] Ah, true, I missed that log_newpage_range() loads the pages to the buffer cache and dirties them. That kinds of sucks actually, I wish it didn't need to dirty the buffers. > So maybe like this: > > if (use_wal) /* includes init forks */ > current logic; > else if (unlogged) > smgrregistersync; > /* else temp || (permanent && wal_level=minimal): nothing to do */ Makes sense, except that we cannot distinguish between unlogged relations and permanent relations with !use_wal here. It would be nice to have relpersistence flag in SMgrRelation. I remember wanting to have that before, although I don't remember what the context was exactly. >> Fortunately, fsync() on a file that's already flushed to disk is pretty >> cheap. > > Yep. I'm more concerned about future readers wondering why the function is > using LSNs to decide what to do about data that doesn't appear in WAL. A > comment could be another way to fix that, though. Agreed, this is all very subtle, and deserves a good comment. What do you think of the attached? -- Heikki Linnakangas Neon (https://neon.tech) Attachments: [text/x-patch] 0001-Relax-fsyncing-at-end-of-bulk-load-that-was-not-WAL-.patch (3.9K, ../../[email protected]/2-0001-Relax-fsyncing-at-end-of-bulk-load-that-was-not-WAL-.patch) download | inline diff: From 6a7a2f34b2134b055c629789aa18a4ad0c4b50a9 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas <[email protected]> Date: Tue, 2 Jul 2024 14:30:29 +0300 Subject: [PATCH 1/1] Relax fsyncing at end of bulk load that was not WAL-logged And improve the comments. --- src/backend/storage/smgr/bulk_write.c | 71 ++++++++++++++++++++++----- 1 file changed, 60 insertions(+), 11 deletions(-) diff --git a/src/backend/storage/smgr/bulk_write.c b/src/backend/storage/smgr/bulk_write.c index 4a10ece4c39..f66d718c7be 100644 --- a/src/backend/storage/smgr/bulk_write.c +++ b/src/backend/storage/smgr/bulk_write.c @@ -132,19 +132,68 @@ smgr_bulk_finish(BulkWriteState *bulkstate) smgr_bulk_flush(bulkstate); /* - * When we wrote out the pages, we passed skipFsync=true to avoid the - * overhead of registering all the writes with the checkpointer. Register - * the whole relation now. - * - * There is one hole in that idea: If a checkpoint occurred while we were - * writing the pages, it already missed fsyncing the pages we had written - * before the checkpoint started. A crash later on would replay the WAL - * starting from the checkpoint, therefore it wouldn't replay our earlier - * WAL records. So if a checkpoint started after the bulk write, fsync - * the files now. + * Fsync the relation, or ask the checkpoint to register it, if necessary. */ - if (!SmgrIsTemp(bulkstate->smgr)) + if (SmgrIsTemp(bulkstate->smgr)) { + /* Temporary relations don't need to be fsync'd, ever */ + } + else if (!bulkstate->use_wal) + { + /* + * This is either an unlogged relation, or a permanent relation but we + * skipped WAL-logging because wal_level=minimal: + * + * A) Unlogged relation + * + * Unlogged relations will go away on crash, but they need to be + * fsync'd on a clean shutdown. It's sufficient to call + * smgrregistersync(), that ensures that the checkpointer will + * flush it at the shutdown checkpoint. (It will flush it on the + * next online checkpoint too, which is not strictly necessary.) + * + * Note that the init-fork of an unlogged relation is not + * considered unlogged for our purposes. It's treated like a + * regular permanent relation. The callers will pass use_wal=true + * for the init fork. + * + * B) Permanent relation, WAL-logging skipped because wal_level=minimal + * + * This is a new relation, and we didn't WAL-log the pages as we + * wrote, but they need to be fsync'd before commit. + * + * We don't need to do that here, however. The fsync() is done at + * commit, by smgrDoPendingSyncs() (*). + * + * (*) smgrDoPendingSyncs() might decide to WAL-log the whole + * relation at commit instead of fsyncing it, if the relation was + * very small, but it's smgrDoPendingSyncs() responsibility in any + * case. + * + * We cannot distinguish the two here, so conservatively assume it's + * an unlogged relation. A permanent relation with wal_level=minimal + * would require no actions, see above. + */ + smgrregistersync(bulkstate->smgr, bulkstate->forknum); + } + else + { + /* + * Permanent relation, WAL-logged normally. + * + * We already WAL-logged all the pages, so they will be replayed from + * WAL on crash. However, when we wrote out the pages, we passed + * skipFsync=true to avoid the overhead of registering all the writes + * with the checkpointer. Register the whole relation now. + * + * There is one hole in that idea: If a checkpoint occurred while we + * were writing the pages, it already missed fsyncing the pages we had + * written before the checkpoint started. A crash later on would + * replay the WAL starting from the checkpoint, therefore it wouldn't + * replay our earlier WAL records. So if a checkpoint started after + * the bulk write, fsync the files now. + */ + /* * Prevent a checkpoint from starting between the GetRedoRecPtr() and * smgrregistersync() calls. -- 2.39.2 ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-07-03 03:41 Noah Misch <[email protected]> parent: Heikki Linnakangas <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Noah Misch @ 2024-07-03 03:41 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On Tue, Jul 02, 2024 at 02:42:50PM +0300, Heikki Linnakangas wrote: > On 02/07/2024 02:24, Noah Misch wrote: > > On Tue, Jul 02, 2024 at 12:53:05AM +0300, Heikki Linnakangas wrote: > log_newpage_range() loads the pages to the buffer > cache and dirties them. That kinds of sucks actually, I wish it didn't need > to dirty the buffers. Agreed. > > > Fortunately, fsync() on a file that's already flushed to disk is pretty > > > cheap. > > > > Yep. I'm more concerned about future readers wondering why the function is > > using LSNs to decide what to do about data that doesn't appear in WAL. A > > comment could be another way to fix that, though. > > Agreed, this is all very subtle, and deserves a good comment. What do you > think of the attached? Looks good. Thanks. pgindent doesn't preserve all your indentation, but it doesn't make things objectionable, either. ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Relation bulk write facility @ 2024-08-16 12:18 Heikki Linnakangas <[email protected]> parent: Noah Misch <[email protected]> 0 siblings, 0 replies; 34+ messages in thread From: Heikki Linnakangas @ 2024-08-16 12:18 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Peter Smith <[email protected]>; Robert Haas <[email protected]>; vignesh C <[email protected]>; Andres Freund <[email protected]>; pgsql-hackers; Melanie Plageman <[email protected]> On 03/07/2024 06:41, Noah Misch wrote: > On Tue, Jul 02, 2024 at 02:42:50PM +0300, Heikki Linnakangas wrote: >> On 02/07/2024 02:24, Noah Misch wrote: >>> On Tue, Jul 02, 2024 at 12:53:05AM +0300, Heikki Linnakangas wrote: >>>> Fortunately, fsync() on a file that's already flushed to disk is pretty >>>> cheap. >>> >>> Yep. I'm more concerned about future readers wondering why the function is >>> using LSNs to decide what to do about data that doesn't appear in WAL. A >>> comment could be another way to fix that, though. >> >> Agreed, this is all very subtle, and deserves a good comment. What do you >> think of the attached? > > Looks good. Thanks. pgindent doesn't preserve all your indentation, but it > doesn't make things objectionable, either. Committed, thanks! (Sorry for the delay, I had forgotten about this already and found it only now sedimented at the bottom of my inbox) -- Heikki Linnakangas Neon (https://neon.tech) ^ permalink raw reply [nested|flat] 34+ messages in thread
end of thread, other threads:[~2024-08-16 12:18 UTC | newest] Thread overview: 34+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-02-28 19:34 [PATCH 2/3] +also rearrange the functions to their original order.. Justin Pryzby <[email protected]> 2024-02-23 14:27 Re: Relation bulk write facility Heikki Linnakangas <[email protected]> 2024-02-23 15:12 ` Re: Relation bulk write facility Heikki Linnakangas <[email protected]> 2024-02-23 15:15 ` Re: Relation bulk write facility Tom Lane <[email protected]> 2024-02-23 15:43 ` Re: Relation bulk write facility Heikki Linnakangas <[email protected]> 2024-02-23 16:06 ` Re: Relation bulk write facility Tom Lane <[email protected]> 2024-02-24 17:23 ` Re: Relation bulk write facility Noah Misch <[email protected]> 2024-02-24 18:52 ` Re: Relation bulk write facility Thomas Munro <[email protected]> 2024-02-24 19:50 ` Re: Relation bulk write facility Noah Misch <[email protected]> 2024-02-24 20:12 ` Re: Relation bulk write facility Thomas Munro <[email protected]> 2024-02-24 20:13 ` Re: Relation bulk write facility Thomas Munro <[email protected]> 2024-02-24 20:26 ` Re: Relation bulk write facility Noah Misch <[email protected]> 2024-02-24 21:29 ` Re: Relation bulk write facility Andres Freund <[email protected]> 2024-02-24 22:06 ` Re: Relation bulk write facility Heikki Linnakangas <[email protected]> 2024-02-24 22:16 ` Re: Relation bulk write facility Thomas Munro <[email protected]> 2024-02-24 22:37 ` Re: Relation bulk write facility Thomas Munro <[email protected]> 2024-02-25 14:34 ` Re: Relation bulk write facility Heikki Linnakangas <[email protected]> 2024-02-25 19:43 ` Re: Relation bulk write facility Noah Misch <[email protected]> 2024-02-25 19:51 ` Re: Relation bulk write facility Tom Lane <[email protected]> 2024-02-26 04:12 ` Re: Relation bulk write facility Robert Haas <[email protected]> 2024-02-26 04:18 ` Re: Relation bulk write facility Michael Paquier <[email protected]> 2024-02-27 20:24 ` Re: Relation bulk write facility Heikki Linnakangas <[email protected]> 2024-02-27 20:45 ` Re: Relation bulk write facility Tom Lane <[email protected]> 2024-02-27 22:30 ` Re: Relation bulk write facility Thomas Munro <[email protected]> 2024-02-28 03:52 ` Re: Relation bulk write facility Noah Misch <[email protected]> 2024-02-28 09:26 ` Re: Relation bulk write facility Andres Freund <[email protected]> 2024-02-29 08:13 ` Remove AIX Support (was: Re: Relation bulk write facility) Michael Banck <[email protected]> 2024-02-29 08:40 ` Re: Remove AIX Support (was: Re: Relation bulk write facility) Daniel Gustafsson <[email protected]> 2024-07-01 20:52 ` Re: Relation bulk write facility Noah Misch <[email protected]> 2024-07-01 21:53 ` Re: Relation bulk write facility Heikki Linnakangas <[email protected]> 2024-07-01 23:24 ` Re: Relation bulk write facility Noah Misch <[email protected]> 2024-07-02 11:42 ` Re: Relation bulk write facility Heikki Linnakangas <[email protected]> 2024-07-03 03:41 ` Re: Relation bulk write facility Noah Misch <[email protected]> 2024-08-16 12:18 ` Re: Relation bulk write facility Heikki Linnakangas <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox