public inbox for [email protected]help / color / mirror / Atom feed
Re: Windows default locale vs initdb 22+ messages / 7 participants [nested] [flat]
* Re: Windows default locale vs initdb @ 2023-12-13 19:58 Thomas Munro <[email protected]> 0 siblings, 1 reply; 22+ messages in thread From: Thomas Munro @ 2023-12-13 19:58 UTC (permalink / raw) To: Juan José Santamaría Flecha <[email protected]>; +Cc: Noah Misch <[email protected]>; pgsql-hackers Here is a thought that occurs to me, as I follow along with Jeff Davis's evolving proposals for built-in collations and ctypes: What would stop us from dropping support for the libc (sic) provider on Windows? That may sound radical and likely to cause extra work for people on upgrade, but how does that compare to the pain of keeping this barely maintained code in the tree? Suppose the idea in this thread goes ahead and we get people to transition to the modern locale names: there is non-zero transitional/upgrade pain there too. How delicious it would be to just nuke the whole thing from orbit, and keep only cross-platform code that is maintained with enthusiasm by active hackers. That's probably a little extreme, but it's the direction my thoughts start to go in when confronting the realisation that it's up to us [Unix hackers making drive-by changes], no one is coming to help us [from the Windows user community]. I've even heard others talk about dropping Windows completely, due to the maintenance imbalance. This would be somewhat more fine grained. (One could use a similar argument to drop non-NTFS filesystems and turn on POSIX-mode file links, to end that other locus of struggle.) ^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Fix search_path for all maintenance commands @ 2024-01-18 03:54 Shubham Khanna <[email protected]> 0 siblings, 1 reply; 22+ messages in thread From: Shubham Khanna @ 2024-01-18 03:54 UTC (permalink / raw) To: Jeff Davis <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Noah Misch <[email protected]>; David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]> On Thu, Jan 18, 2024 at 9:19 AM Jeff Davis <[email protected]> wrote: > > On Mon, 2023-07-17 at 12:16 -0700, Jeff Davis wrote: > > Based on feedback, I plan to commit soon. > > Attached is a new version. > > Changes: > > * Also switch the search_path during CREATE MATERIALIZED VIEW, so that > it's consistent with REFRESH. As a part of this change, I slightly > reordered things in ExecCreateTableAs() so that the skipData path > returns early without entering the SECURITY_RESTRICTED_OPERATION. I > don't think that's a problem because (a) that is one place where > SECURITY_RESTRICTED_OPERATION is not used for security, but rather for > consistency; and (b) that path doesn't go through rewriter, planner, or > executor anyway so I don't see why it would matter. > > * Use GUC_ACTION_SAVE rather than GUC_ACTION_SET. That was a problem > with the previous patch for index functions executed in parallel > workers, which can happen calling SQL functions from pg_amcheck. > > * I used a wrapper function RestrictSearchPath() rather than calling > set_config_option() directly. That provides a nice place in case we > need to add a compatibility GUC to disable it. > > Question: > > Why do we switch to the table owner and use > SECURITY_RESTRICTED_OPERATION in DefineIndex(), when we will switch in > index_build (etc.) anyway? Similarly, why do we switch in vacuum_rel(), > when it doesn't matter for lazy vacuum and we will switch in > cluster_rel() and do_analyze_rel() anyway? I tried to apply the patch but it is failing at the Head. It is giving the following error: Hunk #7 succeeded at 3772 (offset -12 lines). patching file src/backend/commands/matview.c patching file src/backend/commands/vacuum.c Hunk #2 succeeded at 2169 (offset -19 lines). patching file src/backend/utils/init/usercontext.c patching file src/bin/scripts/t/100_vacuumdb.pl Hunk #1 FAILED at 109. 1 out of 1 hunk FAILED -- saving rejects to file src/bin/scripts/t/100_vacuumdb.pl.rej patching file src/include/utils/usercontext.h patching file src/test/modules/test_oat_hooks/expected/test_oat_hooks.out patching file src/test/regress/expected/matview.out patching file src/test/regress/expected/privileges.out patching file src/test/regress/expected/vacuum.out patching file src/test/regress/sql/matview.sql patching file src/test/regress/sql/privileges.sql patching file src/test/regress/sql/vacuum.sql Please send the Re-base version of the patch. Thanks and Regards, Shubham Khanna. ^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Fix search_path for all maintenance commands @ 2024-01-18 05:11 Jeff Davis <[email protected]> parent: Shubham Khanna <[email protected]> 0 siblings, 0 replies; 22+ messages in thread From: Jeff Davis @ 2024-01-18 05:11 UTC (permalink / raw) To: Shubham Khanna <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Noah Misch <[email protected]>; David G. Johnston <[email protected]>; Gurjeet Singh <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Greg Stark <[email protected]>; Tom Lane <[email protected]> On Thu, 2024-01-18 at 09:24 +0530, Shubham Khanna wrote: > I tried to apply the patch but it is failing at the Head. It is > giving > the following error: I am withdrawing this patch from the CF until it's more clear that this is what we really want to do. Thank you for looking into it. Regards, Jeff Davis ^ permalink raw reply [nested|flat] 22+ messages in thread
* [PATCH] rework locking code in GetMultiXactIdMembers @ 2024-03-03 14:20 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 22+ messages in thread From: Alvaro Herrera @ 2024-03-03 14:20 UTC (permalink / raw) Per Coverity --- src/backend/access/transam/multixact.c | 52 ++++++++++---------------- 1 file changed, 20 insertions(+), 32 deletions(-) diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index 9b81506145..ec446949a9 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -1250,14 +1250,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members, MultiXactOffset offset; int length; int truelength; - int i; MultiXactId oldestMXact; MultiXactId nextMXact; MultiXactId tmpMXact; MultiXactOffset nextOffset; MultiXactMember *ptr; LWLock *lock; - LWLock *prevlock = NULL; debug_elog3(DEBUG2, "GetMembers: asked for %u", multi); @@ -1364,18 +1362,9 @@ retry: pageno = MultiXactIdToOffsetPage(multi); entryno = MultiXactIdToOffsetEntry(multi); - /* - * If this page falls under a different bank, release the old bank's lock - * and acquire the lock of the new bank. - */ + /* Acquire the bank lock for the page we need. */ lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); - if (lock != prevlock) - { - if (prevlock != NULL) - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; - } + LWLockAcquire(lock, LW_EXCLUSIVE); slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi); offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno]; @@ -1410,17 +1399,19 @@ retry: if (pageno != prev_pageno) { + LWLock *newlock; + /* * Since we're going to access a different SLRU page, if this page * falls under a different bank, release the old bank's lock and * acquire the lock of the new bank. */ - lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); - if (prevlock != lock) + newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); + if (newlock != lock) { - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; + LWLockRelease(lock); + LWLockAcquire(newlock, LW_EXCLUSIVE); + lock = newlock; } slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact); } @@ -1432,8 +1423,8 @@ retry: if (nextMXOffset == 0) { /* Corner case 2: next multixact is still being filled in */ - LWLockRelease(prevlock); - prevlock = NULL; + LWLockRelease(lock); + lock = NULL; CHECK_FOR_INTERRUPTS(); pg_usleep(1000L); goto retry; @@ -1442,14 +1433,11 @@ retry: length = nextMXOffset - offset; } - LWLockRelease(prevlock); - prevlock = NULL; - ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember)); truelength = 0; prev_pageno = -1; - for (i = 0; i < length; i++, offset++) + for (int i = 0; i < length; i++, offset++) { TransactionId *xactptr; uint32 *flagsptr; @@ -1462,18 +1450,19 @@ retry: if (pageno != prev_pageno) { + LWLock *newlock; + /* * Since we're going to access a different SLRU page, if this page * falls under a different bank, release the old bank's lock and * acquire the lock of the new bank. */ - lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno); - if (lock != prevlock) + newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno); + if (newlock != lock) { - if (prevlock) - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; + LWLockRelease(lock); + LWLockAcquire(newlock, LW_EXCLUSIVE); + lock = newlock; } slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi); @@ -1499,8 +1488,7 @@ retry: truelength++; } - if (prevlock) - LWLockRelease(prevlock); + LWLockRelease(lock); /* A multixid with zero members should not happen */ Assert(truelength > 0); -- 2.39.2 --rb5vb6debo3bx6lt-- ^ permalink raw reply [nested|flat] 22+ messages in thread
* [PATCH v2 1/3] rework locking code in GetMultiXactIdMembers @ 2024-03-03 14:20 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 22+ messages in thread From: Alvaro Herrera @ 2024-03-03 14:20 UTC (permalink / raw) Per Coverity --- src/backend/access/transam/multixact.c | 53 +++++++++++--------------- 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index cd476b94fa..83b578dced 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -1247,14 +1247,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members, MultiXactOffset offset; int length; int truelength; - int i; MultiXactId oldestMXact; MultiXactId nextMXact; MultiXactId tmpMXact; MultiXactOffset nextOffset; MultiXactMember *ptr; LWLock *lock; - LWLock *prevlock = NULL; debug_elog3(DEBUG2, "GetMembers: asked for %u", multi); @@ -1361,18 +1359,9 @@ retry: pageno = MultiXactIdToOffsetPage(multi); entryno = MultiXactIdToOffsetEntry(multi); - /* - * If this page falls under a different bank, release the old bank's lock - * and acquire the lock of the new bank. - */ + /* Acquire the bank lock for the page we need. */ lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); - if (lock != prevlock) - { - if (prevlock != NULL) - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; - } + LWLockAcquire(lock, LW_EXCLUSIVE); slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi); offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno]; @@ -1407,17 +1396,19 @@ retry: if (pageno != prev_pageno) { + LWLock *newlock; + /* * Since we're going to access a different SLRU page, if this page * falls under a different bank, release the old bank's lock and * acquire the lock of the new bank. */ - lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); - if (prevlock != lock) + newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); + if (newlock != lock) { - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; + LWLockRelease(lock); + LWLockAcquire(newlock, LW_EXCLUSIVE); + lock = newlock; } slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact); } @@ -1429,8 +1420,7 @@ retry: if (nextMXOffset == 0) { /* Corner case 2: next multixact is still being filled in */ - LWLockRelease(prevlock); - prevlock = NULL; + LWLockRelease(lock); CHECK_FOR_INTERRUPTS(); pg_usleep(1000L); goto retry; @@ -1439,14 +1429,14 @@ retry: length = nextMXOffset - offset; } - LWLockRelease(prevlock); - prevlock = NULL; + LWLockRelease(lock); + lock = NULL; ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember)); truelength = 0; prev_pageno = -1; - for (i = 0; i < length; i++, offset++) + for (int i = 0; i < length; i++, offset++) { TransactionId *xactptr; uint32 *flagsptr; @@ -1459,18 +1449,20 @@ retry: if (pageno != prev_pageno) { + LWLock *newlock; + /* * Since we're going to access a different SLRU page, if this page * falls under a different bank, release the old bank's lock and * acquire the lock of the new bank. */ - lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno); - if (lock != prevlock) + newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno); + if (newlock != lock) { - if (prevlock) - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; + if (lock) + LWLockRelease(lock); + LWLockAcquire(newlock, LW_EXCLUSIVE); + lock = newlock; } slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi); @@ -1496,8 +1488,7 @@ retry: truelength++; } - if (prevlock) - LWLockRelease(prevlock); + LWLockRelease(lock); /* A multixid with zero members should not happen */ Assert(truelength > 0); -- 2.39.2 --gflnge3qfnp343ni Content-Type: text/x-diff; charset=utf-8 Content-Disposition: attachment; filename="v2-0002-Rework-redundant-loop-in-subtrans.c.patch" ^ permalink raw reply [nested|flat] 22+ messages in thread
* [PATCH] rework locking code in GetMultiXactIdMembers @ 2024-03-03 14:20 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 22+ messages in thread From: Alvaro Herrera @ 2024-03-03 14:20 UTC (permalink / raw) Per Coverity --- src/backend/access/transam/multixact.c | 52 ++++++++++---------------- 1 file changed, 20 insertions(+), 32 deletions(-) diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index 9b81506145..ec446949a9 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -1250,14 +1250,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members, MultiXactOffset offset; int length; int truelength; - int i; MultiXactId oldestMXact; MultiXactId nextMXact; MultiXactId tmpMXact; MultiXactOffset nextOffset; MultiXactMember *ptr; LWLock *lock; - LWLock *prevlock = NULL; debug_elog3(DEBUG2, "GetMembers: asked for %u", multi); @@ -1364,18 +1362,9 @@ retry: pageno = MultiXactIdToOffsetPage(multi); entryno = MultiXactIdToOffsetEntry(multi); - /* - * If this page falls under a different bank, release the old bank's lock - * and acquire the lock of the new bank. - */ + /* Acquire the bank lock for the page we need. */ lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); - if (lock != prevlock) - { - if (prevlock != NULL) - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; - } + LWLockAcquire(lock, LW_EXCLUSIVE); slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi); offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno]; @@ -1410,17 +1399,19 @@ retry: if (pageno != prev_pageno) { + LWLock *newlock; + /* * Since we're going to access a different SLRU page, if this page * falls under a different bank, release the old bank's lock and * acquire the lock of the new bank. */ - lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); - if (prevlock != lock) + newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); + if (newlock != lock) { - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; + LWLockRelease(lock); + LWLockAcquire(newlock, LW_EXCLUSIVE); + lock = newlock; } slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact); } @@ -1432,8 +1423,8 @@ retry: if (nextMXOffset == 0) { /* Corner case 2: next multixact is still being filled in */ - LWLockRelease(prevlock); - prevlock = NULL; + LWLockRelease(lock); + lock = NULL; CHECK_FOR_INTERRUPTS(); pg_usleep(1000L); goto retry; @@ -1442,14 +1433,11 @@ retry: length = nextMXOffset - offset; } - LWLockRelease(prevlock); - prevlock = NULL; - ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember)); truelength = 0; prev_pageno = -1; - for (i = 0; i < length; i++, offset++) + for (int i = 0; i < length; i++, offset++) { TransactionId *xactptr; uint32 *flagsptr; @@ -1462,18 +1450,19 @@ retry: if (pageno != prev_pageno) { + LWLock *newlock; + /* * Since we're going to access a different SLRU page, if this page * falls under a different bank, release the old bank's lock and * acquire the lock of the new bank. */ - lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno); - if (lock != prevlock) + newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno); + if (newlock != lock) { - if (prevlock) - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; + LWLockRelease(lock); + LWLockAcquire(newlock, LW_EXCLUSIVE); + lock = newlock; } slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi); @@ -1499,8 +1488,7 @@ retry: truelength++; } - if (prevlock) - LWLockRelease(prevlock); + LWLockRelease(lock); /* A multixid with zero members should not happen */ Assert(truelength > 0); -- 2.39.2 --rb5vb6debo3bx6lt-- ^ permalink raw reply [nested|flat] 22+ messages in thread
* [PATCH] rework locking code in GetMultiXactIdMembers @ 2024-03-03 14:20 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 22+ messages in thread From: Alvaro Herrera @ 2024-03-03 14:20 UTC (permalink / raw) Per Coverity --- src/backend/access/transam/multixact.c | 52 ++++++++++---------------- 1 file changed, 20 insertions(+), 32 deletions(-) diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index 9b81506145..ec446949a9 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -1250,14 +1250,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members, MultiXactOffset offset; int length; int truelength; - int i; MultiXactId oldestMXact; MultiXactId nextMXact; MultiXactId tmpMXact; MultiXactOffset nextOffset; MultiXactMember *ptr; LWLock *lock; - LWLock *prevlock = NULL; debug_elog3(DEBUG2, "GetMembers: asked for %u", multi); @@ -1364,18 +1362,9 @@ retry: pageno = MultiXactIdToOffsetPage(multi); entryno = MultiXactIdToOffsetEntry(multi); - /* - * If this page falls under a different bank, release the old bank's lock - * and acquire the lock of the new bank. - */ + /* Acquire the bank lock for the page we need. */ lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); - if (lock != prevlock) - { - if (prevlock != NULL) - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; - } + LWLockAcquire(lock, LW_EXCLUSIVE); slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi); offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno]; @@ -1410,17 +1399,19 @@ retry: if (pageno != prev_pageno) { + LWLock *newlock; + /* * Since we're going to access a different SLRU page, if this page * falls under a different bank, release the old bank's lock and * acquire the lock of the new bank. */ - lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); - if (prevlock != lock) + newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); + if (newlock != lock) { - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; + LWLockRelease(lock); + LWLockAcquire(newlock, LW_EXCLUSIVE); + lock = newlock; } slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact); } @@ -1432,8 +1423,8 @@ retry: if (nextMXOffset == 0) { /* Corner case 2: next multixact is still being filled in */ - LWLockRelease(prevlock); - prevlock = NULL; + LWLockRelease(lock); + lock = NULL; CHECK_FOR_INTERRUPTS(); pg_usleep(1000L); goto retry; @@ -1442,14 +1433,11 @@ retry: length = nextMXOffset - offset; } - LWLockRelease(prevlock); - prevlock = NULL; - ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember)); truelength = 0; prev_pageno = -1; - for (i = 0; i < length; i++, offset++) + for (int i = 0; i < length; i++, offset++) { TransactionId *xactptr; uint32 *flagsptr; @@ -1462,18 +1450,19 @@ retry: if (pageno != prev_pageno) { + LWLock *newlock; + /* * Since we're going to access a different SLRU page, if this page * falls under a different bank, release the old bank's lock and * acquire the lock of the new bank. */ - lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno); - if (lock != prevlock) + newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno); + if (newlock != lock) { - if (prevlock) - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; + LWLockRelease(lock); + LWLockAcquire(newlock, LW_EXCLUSIVE); + lock = newlock; } slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi); @@ -1499,8 +1488,7 @@ retry: truelength++; } - if (prevlock) - LWLockRelease(prevlock); + LWLockRelease(lock); /* A multixid with zero members should not happen */ Assert(truelength > 0); -- 2.39.2 --rb5vb6debo3bx6lt-- ^ permalink raw reply [nested|flat] 22+ messages in thread
* [PATCH v2 1/3] rework locking code in GetMultiXactIdMembers @ 2024-03-03 14:20 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 22+ messages in thread From: Alvaro Herrera @ 2024-03-03 14:20 UTC (permalink / raw) Per Coverity --- src/backend/access/transam/multixact.c | 53 +++++++++++--------------- 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index cd476b94fa..83b578dced 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -1247,14 +1247,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members, MultiXactOffset offset; int length; int truelength; - int i; MultiXactId oldestMXact; MultiXactId nextMXact; MultiXactId tmpMXact; MultiXactOffset nextOffset; MultiXactMember *ptr; LWLock *lock; - LWLock *prevlock = NULL; debug_elog3(DEBUG2, "GetMembers: asked for %u", multi); @@ -1361,18 +1359,9 @@ retry: pageno = MultiXactIdToOffsetPage(multi); entryno = MultiXactIdToOffsetEntry(multi); - /* - * If this page falls under a different bank, release the old bank's lock - * and acquire the lock of the new bank. - */ + /* Acquire the bank lock for the page we need. */ lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); - if (lock != prevlock) - { - if (prevlock != NULL) - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; - } + LWLockAcquire(lock, LW_EXCLUSIVE); slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi); offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno]; @@ -1407,17 +1396,19 @@ retry: if (pageno != prev_pageno) { + LWLock *newlock; + /* * Since we're going to access a different SLRU page, if this page * falls under a different bank, release the old bank's lock and * acquire the lock of the new bank. */ - lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); - if (prevlock != lock) + newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); + if (newlock != lock) { - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; + LWLockRelease(lock); + LWLockAcquire(newlock, LW_EXCLUSIVE); + lock = newlock; } slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact); } @@ -1429,8 +1420,7 @@ retry: if (nextMXOffset == 0) { /* Corner case 2: next multixact is still being filled in */ - LWLockRelease(prevlock); - prevlock = NULL; + LWLockRelease(lock); CHECK_FOR_INTERRUPTS(); pg_usleep(1000L); goto retry; @@ -1439,14 +1429,14 @@ retry: length = nextMXOffset - offset; } - LWLockRelease(prevlock); - prevlock = NULL; + LWLockRelease(lock); + lock = NULL; ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember)); truelength = 0; prev_pageno = -1; - for (i = 0; i < length; i++, offset++) + for (int i = 0; i < length; i++, offset++) { TransactionId *xactptr; uint32 *flagsptr; @@ -1459,18 +1449,20 @@ retry: if (pageno != prev_pageno) { + LWLock *newlock; + /* * Since we're going to access a different SLRU page, if this page * falls under a different bank, release the old bank's lock and * acquire the lock of the new bank. */ - lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno); - if (lock != prevlock) + newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno); + if (newlock != lock) { - if (prevlock) - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; + if (lock) + LWLockRelease(lock); + LWLockAcquire(newlock, LW_EXCLUSIVE); + lock = newlock; } slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi); @@ -1496,8 +1488,7 @@ retry: truelength++; } - if (prevlock) - LWLockRelease(prevlock); + LWLockRelease(lock); /* A multixid with zero members should not happen */ Assert(truelength > 0); -- 2.39.2 --gflnge3qfnp343ni Content-Type: text/x-diff; charset=utf-8 Content-Disposition: attachment; filename="v2-0002-Rework-redundant-loop-in-subtrans.c.patch" ^ permalink raw reply [nested|flat] 22+ messages in thread
* [PATCH] rework locking code in GetMultiXactIdMembers @ 2024-03-03 14:20 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 22+ messages in thread From: Alvaro Herrera @ 2024-03-03 14:20 UTC (permalink / raw) Per Coverity --- src/backend/access/transam/multixact.c | 52 ++++++++++---------------- 1 file changed, 20 insertions(+), 32 deletions(-) diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index 9b81506145..ec446949a9 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -1250,14 +1250,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members, MultiXactOffset offset; int length; int truelength; - int i; MultiXactId oldestMXact; MultiXactId nextMXact; MultiXactId tmpMXact; MultiXactOffset nextOffset; MultiXactMember *ptr; LWLock *lock; - LWLock *prevlock = NULL; debug_elog3(DEBUG2, "GetMembers: asked for %u", multi); @@ -1364,18 +1362,9 @@ retry: pageno = MultiXactIdToOffsetPage(multi); entryno = MultiXactIdToOffsetEntry(multi); - /* - * If this page falls under a different bank, release the old bank's lock - * and acquire the lock of the new bank. - */ + /* Acquire the bank lock for the page we need. */ lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); - if (lock != prevlock) - { - if (prevlock != NULL) - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; - } + LWLockAcquire(lock, LW_EXCLUSIVE); slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi); offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno]; @@ -1410,17 +1399,19 @@ retry: if (pageno != prev_pageno) { + LWLock *newlock; + /* * Since we're going to access a different SLRU page, if this page * falls under a different bank, release the old bank's lock and * acquire the lock of the new bank. */ - lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); - if (prevlock != lock) + newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); + if (newlock != lock) { - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; + LWLockRelease(lock); + LWLockAcquire(newlock, LW_EXCLUSIVE); + lock = newlock; } slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact); } @@ -1432,8 +1423,8 @@ retry: if (nextMXOffset == 0) { /* Corner case 2: next multixact is still being filled in */ - LWLockRelease(prevlock); - prevlock = NULL; + LWLockRelease(lock); + lock = NULL; CHECK_FOR_INTERRUPTS(); pg_usleep(1000L); goto retry; @@ -1442,14 +1433,11 @@ retry: length = nextMXOffset - offset; } - LWLockRelease(prevlock); - prevlock = NULL; - ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember)); truelength = 0; prev_pageno = -1; - for (i = 0; i < length; i++, offset++) + for (int i = 0; i < length; i++, offset++) { TransactionId *xactptr; uint32 *flagsptr; @@ -1462,18 +1450,19 @@ retry: if (pageno != prev_pageno) { + LWLock *newlock; + /* * Since we're going to access a different SLRU page, if this page * falls under a different bank, release the old bank's lock and * acquire the lock of the new bank. */ - lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno); - if (lock != prevlock) + newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno); + if (newlock != lock) { - if (prevlock) - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; + LWLockRelease(lock); + LWLockAcquire(newlock, LW_EXCLUSIVE); + lock = newlock; } slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi); @@ -1499,8 +1488,7 @@ retry: truelength++; } - if (prevlock) - LWLockRelease(prevlock); + LWLockRelease(lock); /* A multixid with zero members should not happen */ Assert(truelength > 0); -- 2.39.2 --rb5vb6debo3bx6lt-- ^ permalink raw reply [nested|flat] 22+ messages in thread
* [PATCH v2 1/3] rework locking code in GetMultiXactIdMembers @ 2024-03-03 14:20 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 22+ messages in thread From: Alvaro Herrera @ 2024-03-03 14:20 UTC (permalink / raw) Per Coverity --- src/backend/access/transam/multixact.c | 53 +++++++++++--------------- 1 file changed, 22 insertions(+), 31 deletions(-) diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index cd476b94fa..83b578dced 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -1247,14 +1247,12 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members, MultiXactOffset offset; int length; int truelength; - int i; MultiXactId oldestMXact; MultiXactId nextMXact; MultiXactId tmpMXact; MultiXactOffset nextOffset; MultiXactMember *ptr; LWLock *lock; - LWLock *prevlock = NULL; debug_elog3(DEBUG2, "GetMembers: asked for %u", multi); @@ -1361,18 +1359,9 @@ retry: pageno = MultiXactIdToOffsetPage(multi); entryno = MultiXactIdToOffsetEntry(multi); - /* - * If this page falls under a different bank, release the old bank's lock - * and acquire the lock of the new bank. - */ + /* Acquire the bank lock for the page we need. */ lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); - if (lock != prevlock) - { - if (prevlock != NULL) - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; - } + LWLockAcquire(lock, LW_EXCLUSIVE); slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, multi); offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno]; @@ -1407,17 +1396,19 @@ retry: if (pageno != prev_pageno) { + LWLock *newlock; + /* * Since we're going to access a different SLRU page, if this page * falls under a different bank, release the old bank's lock and * acquire the lock of the new bank. */ - lock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); - if (prevlock != lock) + newlock = SimpleLruGetBankLock(MultiXactOffsetCtl, pageno); + if (newlock != lock) { - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; + LWLockRelease(lock); + LWLockAcquire(newlock, LW_EXCLUSIVE); + lock = newlock; } slotno = SimpleLruReadPage(MultiXactOffsetCtl, pageno, true, tmpMXact); } @@ -1429,8 +1420,7 @@ retry: if (nextMXOffset == 0) { /* Corner case 2: next multixact is still being filled in */ - LWLockRelease(prevlock); - prevlock = NULL; + LWLockRelease(lock); CHECK_FOR_INTERRUPTS(); pg_usleep(1000L); goto retry; @@ -1439,14 +1429,14 @@ retry: length = nextMXOffset - offset; } - LWLockRelease(prevlock); - prevlock = NULL; + LWLockRelease(lock); + lock = NULL; ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember)); truelength = 0; prev_pageno = -1; - for (i = 0; i < length; i++, offset++) + for (int i = 0; i < length; i++, offset++) { TransactionId *xactptr; uint32 *flagsptr; @@ -1459,18 +1449,20 @@ retry: if (pageno != prev_pageno) { + LWLock *newlock; + /* * Since we're going to access a different SLRU page, if this page * falls under a different bank, release the old bank's lock and * acquire the lock of the new bank. */ - lock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno); - if (lock != prevlock) + newlock = SimpleLruGetBankLock(MultiXactMemberCtl, pageno); + if (newlock != lock) { - if (prevlock) - LWLockRelease(prevlock); - LWLockAcquire(lock, LW_EXCLUSIVE); - prevlock = lock; + if (lock) + LWLockRelease(lock); + LWLockAcquire(newlock, LW_EXCLUSIVE); + lock = newlock; } slotno = SimpleLruReadPage(MultiXactMemberCtl, pageno, true, multi); @@ -1496,8 +1488,7 @@ retry: truelength++; } - if (prevlock) - LWLockRelease(prevlock); + LWLockRelease(lock); /* A multixid with zero members should not happen */ Assert(truelength > 0); -- 2.39.2 --gflnge3qfnp343ni Content-Type: text/x-diff; charset=utf-8 Content-Disposition: attachment; filename="v2-0002-Rework-redundant-loop-in-subtrans.c.patch" ^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Windows default locale vs initdb @ 2024-07-22 02:51 Thomas Munro <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 3 replies; 22+ messages in thread From: Thomas Munro @ 2024-07-22 02:51 UTC (permalink / raw) To: pgsql-hackers; +Cc: Noah Misch <[email protected]>; Juan José Santamaría Flecha <[email protected]>; Ertan Küçükoglu <[email protected]> Ertan Küçükoglu offered to try to review and test this, so here's a rebase. Some notes: * it turned out that the Turkish i/I test problem I mentioned earlier in this thread[1] was just always broken on Windows, we just didn't ever test with UTF-8 before Meson took over; it's skipped now, see commit cff4e5a3[2] * it seems that you can't actually put encodings like .1252 on the end (.UTF-8 must be a special case); I don't know if we should look into a better UTF-8 mode for modern Windows, but that'd be a separate project * this patch only benefits people who run initdb.exe without explicitly specifying a locale; probably a good number of real systems in the wild actually use EDB's graphical installer which initialises a cluster and has its own way of choosing the locale, as discussed in Ertan's thread[3] [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJZskvCh%3DQm75UkHrY6c1QZUuC92Po9rponj1BbLmcMEA%4... [2] https://github.com/postgres/postgres/commit/cff4e5a3 [3] https://www.postgresql.org/message-id/flat/CAH2i4ydECHZPxEBB7gtRG3vROv7a0d3tqAFXzcJWQ9hRsc1znQ%40mai... Attachments: [text/x-patch] v5-0001-MinGW-has-GetLocaleInfoEx.patch (1.4K, ../../CA+hUKGL5mBN3JQuebAPbX0yxDNtpui04J+KSy2F7KBbhLGaJig@mail.gmail.com/2-v5-0001-MinGW-has-GetLocaleInfoEx.patch) download | inline diff: From fb33b7eb5482bae31b70bb54dbe77325b543a89c Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Mon, 20 Nov 2023 14:24:35 +1300 Subject: [PATCH v5 1/2] MinGW has GetLocaleInfoEx(). To use BCP 47 locale names like "en-US" without a suffix ".encoding", we need to be able to call GetLocaleInfoEx() to look up the encoding. That was previously gated for MSVC only, but MinGW has had the function for many years. Remove that gating, because otherwise our MinGW build farm animals would fail when a later commit switches to using the new names by default. There are probably other places where _MSC_VER is being used as a proxy for detecting MinGW with an out-of-date idea about missing functions. Discussion: https://postgr.es/m/CA%2BhUKGLsV3vTjPp7bOZBr3JTKp3Brkr9V0Qfmc7UvpWcmAQL4A%40mail.gmail.com --- src/port/chklocale.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/port/chklocale.c b/src/port/chklocale.c index 8cb81c8640e..a15b0d5349b 100644 --- a/src/port/chklocale.c +++ b/src/port/chklocale.c @@ -204,7 +204,6 @@ win32_langinfo(const char *ctype) char *r = NULL; char *codepage; -#if defined(_MSC_VER) uint32 cp; WCHAR wctype[LOCALE_NAME_MAX_LENGTH]; @@ -229,7 +228,6 @@ win32_langinfo(const char *ctype) } } else -#endif { /* * Locale format on Win32 is <Language>_<Country>.<CodePage>. For -- 2.45.2 [text/x-patch] v5-0002-Default-to-IETF-BCP-47-locale-names-in-initdb-on-.patch (4.2K, ../../CA+hUKGL5mBN3JQuebAPbX0yxDNtpui04J+KSy2F7KBbhLGaJig@mail.gmail.com/3-v5-0002-Default-to-IETF-BCP-47-locale-names-in-initdb-on-.patch) download | inline diff: From dc726a61aace86bda62687e3aa1411753ba3f1a4 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Tue, 19 Jul 2022 06:31:17 +1200 Subject: [PATCH v5 2/2] Default to IETF BCP 47 locale names in initdb on Windows. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Avoid selecting traditional Windows locale names written with English words, because (1) they are unstable and explicitly not recommended for use in databases and (2) they may contain non-ASCII characters, which we can't put in our shared catalogs. Since setlocale() returns such names, on Windows use GetUserDefaultLocaleName() if the user didn't provide an explicit locale. It returns BCP 47 strings like "en-US". Also update the documentation to recommend BCP 47 over the traditional names when providing explicit values to initdb. Reviewed-by: Juan José Santamaría Flecha <[email protected]> Reviewed-by: Discussion: https://postgr.es/m/CA%2BhUKGJ%3DXThErgAQRoqfCy1bKPxXVuF0%3D2zDbB%2BSxDs59pv7Fw%40mail.gmail.com --- doc/src/sgml/charset.sgml | 13 +++++++++++-- src/bin/initdb/initdb.c | 31 +++++++++++++++++++++++++++++-- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/doc/src/sgml/charset.sgml b/doc/src/sgml/charset.sgml index 834cb30c85a..adb21eb0799 100644 --- a/doc/src/sgml/charset.sgml +++ b/doc/src/sgml/charset.sgml @@ -83,8 +83,17 @@ initdb --locale=sv_SE system under what names depends on what was provided by the operating system vendor and what was installed. On most Unix systems, the command <literal>locale -a</literal> will provide a list of available locales. - Windows uses more verbose locale names, such as <literal>German_Germany</literal> - or <literal>Swedish_Sweden.1252</literal>, but the principles are the same. + </para> + + <para> + Windows uses BCP 47 language tags, like ICU. + For example, <literal>sv-SE</literal> represents Swedish as spoken in Sweden. + Windows also supports more verbose locale names based on full names + such as <literal>German_Germany</literal> or <literal>Swedish_Sweden.1252</literal>, + but these are not recommended because they are not stable across operating + system updates due to changes in geographical names, and may contain + non-ASCII characters which are not supported in PostgreSQL's shared + catalogs. </para> <para> diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index f00718a0150..393232b6cec 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -64,6 +64,10 @@ #include "sys/mman.h" #endif +#ifdef WIN32 +#include <winnls.h> +#endif + #include "access/xlog_internal.h" #include "catalog/pg_authid_d.h" #include "catalog/pg_class_d.h" /* pgrminclude ignore */ @@ -2132,6 +2136,7 @@ locale_date_order(const char *locale) static void check_locale_name(int category, const char *locale, char **canonname) { + char *locale_copy; char *save; char *res; @@ -2147,10 +2152,30 @@ check_locale_name(int category, const char *locale, char **canonname) /* for setlocale() call */ if (!locale) - locale = ""; + { +#ifdef WIN32 + wchar_t wide_name[LOCALE_NAME_MAX_LENGTH]; + char name[LOCALE_NAME_MAX_LENGTH]; + + /* use Windows API to find the default in BCP47 format */ + if (GetUserDefaultLocaleName(wide_name, LOCALE_NAME_MAX_LENGTH) == 0) + pg_fatal("failed to get default locale name: error code %lu", + GetLastError()); + if (WideCharToMultiByte(CP_ACP, 0, wide_name, -1, name, + LOCALE_NAME_MAX_LENGTH, NULL, NULL) == 0) + pg_fatal("failed to convert locale name: error code %lu", + GetLastError()); + locale_copy = pg_strdup(name); +#else + /* use environment to find the default */ + locale_copy = pg_strdup(""); +#endif + } + else + locale_copy = pg_strdup(locale); /* set the locale with setlocale, to see if it accepts it. */ - res = setlocale(category, locale); + res = setlocale(category, locale_copy); /* save canonical name if requested. */ if (res && canonname) @@ -2183,6 +2208,8 @@ check_locale_name(int category, const char *locale, char **canonname) pg_fatal("invalid locale settings; check LANG and LC_* environment variables"); } } + + free(locale_copy); } /* -- 2.45.2 ^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Windows default locale vs initdb @ 2024-07-22 08:04 Ertan Küçükoglu <[email protected]> parent: Thomas Munro <[email protected]> 2 siblings, 1 reply; 22+ messages in thread From: Ertan Küçükoglu @ 2024-07-22 08:04 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: pgsql-hackers; Noah Misch <[email protected]>; Juan José Santamaría Flecha <[email protected]> Hi, I am a complete noob about PostgreSQL development. I don't know about the PostgreSQL CI system. I will be needing some help as to how to do the tests. I have access to different Windows OSes (v10, Server 2022 mainly). These systems can be set to English or Turkish locales if needed. I can also add new Windows versions if needed. I do not know how to use patch files. I am also not sure what tests I should do. Do I need to set up a Windows build system for PostgreSQL CI? Will I download some files (EXE, etc) ready for testing? Copy them over an existing installation for testing? Thanks for your help. Regards, Ertan Thomas Munro <[email protected]>, 22 Tem 2024 Pzt, 05:52 tarihinde şunu yazdı: > Ertan Küçükoglu offered to try to review and test this, so here's a rebase. > > Some notes: > > * it turned out that the Turkish i/I test problem I mentioned earlier > in this thread[1] was just always broken on Windows, we just didn't > ever test with UTF-8 before Meson took over; it's skipped now, see > commit cff4e5a3[2] > > * it seems that you can't actually put encodings like .1252 on the end > (.UTF-8 must be a special case); I don't know if we should look into a > better UTF-8 mode for modern Windows, but that'd be a separate project > > * this patch only benefits people who run initdb.exe without > explicitly specifying a locale; probably a good number of real systems > in the wild actually use EDB's graphical installer which initialises a > cluster and has its own way of choosing the locale, as discussed in > Ertan's thread[3] > > [1] > https://www.postgresql.org/message-id/flat/CA%2BhUKGJZskvCh%3DQm75UkHrY6c1QZUuC92Po9rponj1BbLmcMEA%4... > [2] https://github.com/postgres/postgres/commit/cff4e5a3 > [3] > https://www.postgresql.org/message-id/flat/CAH2i4ydECHZPxEBB7gtRG3vROv7a0d3tqAFXzcJWQ9hRsc1znQ%40mai... > ^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Windows default locale vs initdb @ 2024-07-22 08:37 Zaid Shabbir <[email protected]> parent: Thomas Munro <[email protected]> 2 siblings, 1 reply; 22+ messages in thread From: Zaid Shabbir @ 2024-07-22 08:37 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: pgsql-hackers; Noah Misch <[email protected]>; Juan José Santamaría Flecha <[email protected]>; Ertan Küçükoglu <[email protected]> Hello Thomas, Can you please list down some of the use cases for the patch ? Other than Turkish, does this patch have an impact on other locales too ? Regards, Zaid On Mon, Jul 22, 2024 at 7:52 AM Thomas Munro <[email protected]> wrote: > Ertan Küçükoglu offered to try to review and test this, so here's a rebase. > > Some notes: > > * it turned out that the Turkish i/I test problem I mentioned earlier > in this thread[1] was just always broken on Windows, we just didn't > ever test with UTF-8 before Meson took over; it's skipped now, see > commit cff4e5a3[2] > > * it seems that you can't actually put encodings like .1252 on the end > (.UTF-8 must be a special case); I don't know if we should look into a > better UTF-8 mode for modern Windows, but that'd be a separate project > > * this patch only benefits people who run initdb.exe without > explicitly specifying a locale; probably a good number of real systems > in the wild actually use EDB's graphical installer which initialises a > cluster and has its own way of choosing the locale, as discussed in > Ertan's thread[3] > > [1] > https://www.postgresql.org/message-id/flat/CA%2BhUKGJZskvCh%3DQm75UkHrY6c1QZUuC92Po9rponj1BbLmcMEA%4... > [2] https://github.com/postgres/postgres/commit/cff4e5a3 > [3] > https://www.postgresql.org/message-id/flat/CAH2i4ydECHZPxEBB7gtRG3vROv7a0d3tqAFXzcJWQ9hRsc1znQ%40mai... > ^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Windows default locale vs initdb @ 2024-07-22 10:12 Thomas Munro <[email protected]> parent: Zaid Shabbir <[email protected]> 0 siblings, 0 replies; 22+ messages in thread From: Thomas Munro @ 2024-07-22 10:12 UTC (permalink / raw) To: Zaid Shabbir <[email protected]>; +Cc: pgsql-hackers; Noah Misch <[email protected]>; Juan José Santamaría Flecha <[email protected]>; Ertan Küçükoglu <[email protected]> On Mon, Jul 22, 2024 at 8:38 PM Zaid Shabbir <[email protected]> wrote: > Can you please list down some of the use cases for the patch ? Other than Turkish, does this patch have an impact on other locales too ? Hi Zaid, Yes, initdb.exe would use BCP47 codes by default for all languages. Who knows which country will change its name next? From a quick search of other recent cases: Czech Republic -> Czechia, Swaziland -> Eswatini, Cape Verde -> Cabo Verde, and more, plus others that we have older records of in the mailing list that seemed to change in some minor technical way: Macau, Hong Hong, Norwegian etc. The Windows manual says: "We do not recommend this form for locale strings embedded in code or serialized to storage, because these strings are more likely to be changed by an operating system update than the locale name form." It's pretty bad for our users when it happens and the Windows locale name changes: a database cluster that suddenly can't start, and even after you've figured out why and adjusted the references in postgresql.conf, you still can't connect. There is also the problem that some of the old full names have non-ASCII characters (Türkiye, São Tomé and Príncipe, Curaçao, Côte d'Ivoire, Åland) which is bad at least in theory because we use the string in times and places when it it is not clear what the encoding the name itself has. I don't use Windows myself, I've just been watching this train wreck replaying in a loop for long enough. Clearly it's going to take some time to wean the user community off the unstable names, and it struck me that the default is probably the main source of them in new clusters, hence this patch. ^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Windows default locale vs initdb @ 2024-07-22 11:01 Thomas Munro <[email protected]> parent: Ertan Küçükoglu <[email protected]> 0 siblings, 1 reply; 22+ messages in thread From: Thomas Munro @ 2024-07-22 11:01 UTC (permalink / raw) To: Ertan Küçükoglu <[email protected]>; +Cc: pgsql-hackers; Noah Misch <[email protected]>; Juan José Santamaría Flecha <[email protected]> On Mon, Jul 22, 2024 at 8:04 PM Ertan Küçükoglu <[email protected]> wrote: > I am a complete noob about PostgreSQL development. > I don't know about the PostgreSQL CI system. > I will be needing some help as to how to do the tests. > I have access to different Windows OSes (v10, Server 2022 mainly). > These systems can be set to English or Turkish locales if needed. > I can also add new Windows versions if needed. > I do not know how to use patch files. I am also not sure what tests I should do. > Do I need to set up a Windows build system for PostgreSQL CI? > Will I download some files (EXE, etc) ready for testing? Copy them over an existing installation for testing? Sorry, I didn't mean to put you on the spot :-) Yeah you'd need to install a compiler, various libraries and tools to be able to build form source with a patch. Unfortunately I'm not the best person to explain how to do that on Windows as I don't use it. Honestly it might be a bit too much new stuff to figure out at once just to test this small patch. What I'd be hoping for is confirmation that there are no weird unintended consequences or problems I'm not seeing since I'm writing blind patches based on documentation only, but it's probably too much to ask to figure out the whole development environment and then go on an open ended expedition looking for unknown problems. ^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Windows default locale vs initdb @ 2024-07-22 12:52 Ertan Küçükoglu <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 22+ messages in thread From: Ertan Küçükoglu @ 2024-07-22 12:52 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: pgsql-hackers; Noah Misch <[email protected]>; Juan José Santamaría Flecha <[email protected]> Thomas Munro <[email protected]>, 22 Tem 2024 Pzt, 14:00 tarihinde şunu yazdı: > Sorry, I didn't mean to put you on the spot :-) Yeah you'd need to > install a compiler, various libraries and tools to be able to build > form source with a patch. Unfortunately I'm not the best person to > explain how to do that on Windows as I don't use it. Honestly it > might be a bit too much new stuff to figure out at once just to test > this small patch. What I'd be hoping for is confirmation that there > are no weird unintended consequences or problems I'm not seeing since > I'm writing blind patches based on documentation only, but it's > probably too much to ask to figure out the whole development > environment and then go on an open ended expedition looking for > unknown problems. > I already installed Visual Studio 2022 with C++ support as suggested in https://www.postgresql.org/docs/current/install-windows-full.html I cloned codes in the system. But, I cannot find any "src/tools/msvc" directory. It is missing. Document states I need everything in there "The tools for building using Visual C++ or Platform SDK are in the src\tools\msvc directory." It seems I will need help setting up the build environment. ^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Windows default locale vs initdb @ 2024-07-22 13:44 Andrew Dunstan <[email protected]> parent: Thomas Munro <[email protected]> 2 siblings, 2 replies; 22+ messages in thread From: Andrew Dunstan @ 2024-07-22 13:44 UTC (permalink / raw) To: Thomas Munro <[email protected]>; pgsql-hackers; +Cc: Noah Misch <[email protected]>; Juan José Santamaría Flecha <[email protected]>; Ertan Küçükoglu <[email protected]> On 2024-07-21 Su 10:51 PM, Thomas Munro wrote: > Ertan Küçükoglu offered to try to review and test this, so here's a rebase. > > Some notes: > > * it turned out that the Turkish i/I test problem I mentioned earlier > in this thread[1] was just always broken on Windows, we just didn't > ever test with UTF-8 before Meson took over; it's skipped now, see > commit cff4e5a3[2] > > * it seems that you can't actually put encodings like .1252 on the end > (.UTF-8 must be a special case); I don't know if we should look into a > better UTF-8 mode for modern Windows, but that'd be a separate project > > * this patch only benefits people who run initdb.exe without > explicitly specifying a locale; probably a good number of real systems > in the wild actually use EDB's graphical installer which initialises a > cluster and has its own way of choosing the locale, as discussed in > Ertan's thread[3] > > [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGJZskvCh%3DQm75UkHrY6c1QZUuC92Po9rponj1BbLmcMEA%4... > [2] https://github.com/postgres/postgres/commit/cff4e5a3 > [3] https://www.postgresql.org/message-id/flat/CAH2i4ydECHZPxEBB7gtRG3vROv7a0d3tqAFXzcJWQ9hRsc1znQ%40mai... I have an environment I can use for testing. But what exactly am I testing? :-) Install a few "problem" language/region settings, switch the system and ensure initdb runs ok? Other than Turkish, which locales should I install? cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Windows default locale vs initdb @ 2024-07-22 13:51 Ertan Küçükoglu <[email protected]> parent: Andrew Dunstan <[email protected]> 1 sibling, 0 replies; 22+ messages in thread From: Ertan Küçükoglu @ 2024-07-22 13:51 UTC (permalink / raw) To: Andrew Dunstan <[email protected]>; +Cc: Thomas Munro <[email protected]>; pgsql-hackers; Noah Misch <[email protected]>; Juan José Santamaría Flecha <[email protected]> Andrew Dunstan <[email protected]>, 22 Tem 2024 Pzt, 16:44 tarihinde şunu yazdı: > I have an environment I can use for testing. But what exactly am I > testing? :-) Install a few "problem" language/region settings, switch > the system and ensure initdb runs ok? > > Other than Turkish, which locales should I install? > Thomas earlier listed a few: "From a quick search of other recent cases: Czech Republic -> Czechia, Swaziland -> Eswatini, Cape Verde -> Cabo Verde, and more, plus others that we have older records of in the mailing list that seemed to change in some minor technical way: Macau, Hong Hong, Norwegian etc." I am not sure if all needs testing though. Thanks & Regards, Ertan ^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Windows default locale vs initdb @ 2024-07-22 23:19 Thomas Munro <[email protected]> parent: Andrew Dunstan <[email protected]> 1 sibling, 1 reply; 22+ messages in thread From: Thomas Munro @ 2024-07-22 23:19 UTC (permalink / raw) To: Andrew Dunstan <[email protected]>; +Cc: pgsql-hackers; Noah Misch <[email protected]>; Juan José Santamaría Flecha <[email protected]>; Ertan Küçükoglu <[email protected]> On Tue, Jul 23, 2024 at 1:44 AM Andrew Dunstan <[email protected]> wrote: > I have an environment I can use for testing. But what exactly am I > testing? :-) Install a few "problem" language/region settings, switch > the system and ensure initdb runs ok? I just want to know about any weird unexpected consequences of using BCP47 locale names, before we change the default in v18. The only concrete thing I found so far was that MinGW didn't like it, but I provided a fix for that. It'd still be possible to initialise a new cluster with the old style names if you really want to, but you'd have to pass it in explicitly; I was wondering if that could be necessary in some pg_upgrade scenario but I guess not, it just clobbers template0's pg_database row with values from the source database, and recreates everything else so I think it should be fine (?). I am a little uneasy about the new names not having .encoding but there doesn't seem to be an issue with that (such locales exist on Unix too), and the OS still knows which encoding they use in that case. ^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Windows default locale vs initdb @ 2024-08-07 04:15 Thomas Munro <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 0 replies; 22+ messages in thread From: Thomas Munro @ 2024-08-07 04:15 UTC (permalink / raw) To: Andrew Dunstan <[email protected]>; +Cc: pgsql-hackers; Noah Misch <[email protected]>; Juan José Santamaría Flecha <[email protected]>; Ertan Küçükoglu <[email protected]> On Tue, Jul 23, 2024 at 11:19 AM Thomas Munro <[email protected]> wrote: > On Tue, Jul 23, 2024 at 1:44 AM Andrew Dunstan <[email protected]> wrote: > > I have an environment I can use for testing. But what exactly am I > > testing? :-) Install a few "problem" language/region settings, switch > > the system and ensure initdb runs ok? I thought a bit more about what to do with the messy .UTF-8 situation on Windows, and I think I might see a way forward that harmonises the code and behaviour with Unix, and deletes a lot of special case code. But it's only theories + CI so far. 0001, 0002: As before, teach initdb.exe to choose eg "en-US" by default. 0003: Force people to choose locales that match the database encoding, as we do on Unix. That is, forbid contradictory combinations like --locale="English_United States.1252" --encoding=UTF8, which are currently allowed (and the world is full of such database clusters because that is how the EDB installer GUI makes them). The only allowed combinations for American English should now be: --locale="en-US" --encoding="WIN1252", and --locale="en-US.UTF-8" --encoding="UTF8". You can still use the old names if you like, by explicitly writing --locale="English_United States.1252", but the encoding then has to be WIN1252. It's crazy to mix them up, let's ban that. Obviously there is a pg_upgrade case to worry about there. We'd have to "fix" the now illegal combinations, and I don't know exactly how yet. 0004: Rip out the code that does extra wchar_t conversations for collations. If I've understood correctly, we don't need them: if you have a .UTF-8 locale then your encoding is UTF-8 and should be able to use strcoll_l() directly. Right? 0005: Something similar was being done for strftime(). And we might as well use strftime_l() instead while we're here (part of general movement to use _l functions and stop splattering setlocale() all over the place, for the multithreaded future). These patches pass on CI. Do they give the expected results when used on a real Windows system? There are a few more places where we do wchar_t conversions that could probably be stripped out too, if my assumptions are correct, and we could dig further if the basic idea can be validated and people think this is going in a good direction. Attachments: [text/x-patch] v6-0001-MinGW-has-GetLocaleInfoEx.patch (1.4K, ../../CA+hUKGJ=ca39Cg=y=S89EaCYvvCF8NrZRO=uog-cnz0VzC6Kfg@mail.gmail.com/2-v6-0001-MinGW-has-GetLocaleInfoEx.patch) download | inline diff: From 886815244ab43092562ae3118cd5588a2fad5bb2 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Mon, 20 Nov 2023 14:24:35 +1300 Subject: [PATCH v6 1/5] MinGW has GetLocaleInfoEx(). To use BCP 47 locale names like "en-US" without a suffix ".encoding", we need to be able to call GetLocaleInfoEx() to look up the encoding. That was previously gated for MSVC only, but MinGW has had the function for many years. Remove that gating, because otherwise our MinGW build farm animals would fail when a later commit switches to using the new names by default. There are probably other places where _MSC_VER is being used as a proxy for detecting MinGW with an out-of-date idea about missing functions. Discussion: https://postgr.es/m/CA%2BhUKGLsV3vTjPp7bOZBr3JTKp3Brkr9V0Qfmc7UvpWcmAQL4A%40mail.gmail.com --- src/port/chklocale.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/port/chklocale.c b/src/port/chklocale.c index 8cb81c8640..a15b0d5349 100644 --- a/src/port/chklocale.c +++ b/src/port/chklocale.c @@ -204,7 +204,6 @@ win32_langinfo(const char *ctype) char *r = NULL; char *codepage; -#if defined(_MSC_VER) uint32 cp; WCHAR wctype[LOCALE_NAME_MAX_LENGTH]; @@ -229,7 +228,6 @@ win32_langinfo(const char *ctype) } } else -#endif { /* * Locale format on Win32 is <Language>_<Country>.<CodePage>. For -- 2.39.2 [text/x-patch] v6-0002-Default-to-IETF-BCP-47-locale-names-in-initdb-on-.patch (4.2K, ../../CA+hUKGJ=ca39Cg=y=S89EaCYvvCF8NrZRO=uog-cnz0VzC6Kfg@mail.gmail.com/3-v6-0002-Default-to-IETF-BCP-47-locale-names-in-initdb-on-.patch) download | inline diff: From 357751c04cdd3dc7dea1ee9409356d818af70d5d Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Tue, 19 Jul 2022 06:31:17 +1200 Subject: [PATCH v6 2/5] Default to IETF BCP 47 locale names in initdb on Windows. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Avoid selecting traditional Windows locale names written with English words, because (1) they are unstable and explicitly not recommended for use in databases and (2) they may contain non-ASCII characters, which we can't put in our shared catalogs. Since setlocale() returns such names, on Windows use GetUserDefaultLocaleName() if the user didn't provide an explicit locale. It returns BCP 47 strings like "en-US". Also update the documentation to recommend BCP 47 over the traditional names when providing explicit values to initdb. Reviewed-by: Juan José Santamaría Flecha <[email protected]> Reviewed-by: Discussion: https://postgr.es/m/CA%2BhUKGJ%3DXThErgAQRoqfCy1bKPxXVuF0%3D2zDbB%2BSxDs59pv7Fw%40mail.gmail.com --- doc/src/sgml/charset.sgml | 13 +++++++++++-- src/bin/initdb/initdb.c | 31 +++++++++++++++++++++++++++++-- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/doc/src/sgml/charset.sgml b/doc/src/sgml/charset.sgml index 834cb30c85..adb21eb079 100644 --- a/doc/src/sgml/charset.sgml +++ b/doc/src/sgml/charset.sgml @@ -83,8 +83,17 @@ initdb --locale=sv_SE system under what names depends on what was provided by the operating system vendor and what was installed. On most Unix systems, the command <literal>locale -a</literal> will provide a list of available locales. - Windows uses more verbose locale names, such as <literal>German_Germany</literal> - or <literal>Swedish_Sweden.1252</literal>, but the principles are the same. + </para> + + <para> + Windows uses BCP 47 language tags, like ICU. + For example, <literal>sv-SE</literal> represents Swedish as spoken in Sweden. + Windows also supports more verbose locale names based on full names + such as <literal>German_Germany</literal> or <literal>Swedish_Sweden.1252</literal>, + but these are not recommended because they are not stable across operating + system updates due to changes in geographical names, and may contain + non-ASCII characters which are not supported in PostgreSQL's shared + catalogs. </para> <para> diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index f00718a015..393232b6ce 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -64,6 +64,10 @@ #include "sys/mman.h" #endif +#ifdef WIN32 +#include <winnls.h> +#endif + #include "access/xlog_internal.h" #include "catalog/pg_authid_d.h" #include "catalog/pg_class_d.h" /* pgrminclude ignore */ @@ -2132,6 +2136,7 @@ locale_date_order(const char *locale) static void check_locale_name(int category, const char *locale, char **canonname) { + char *locale_copy; char *save; char *res; @@ -2147,10 +2152,30 @@ check_locale_name(int category, const char *locale, char **canonname) /* for setlocale() call */ if (!locale) - locale = ""; + { +#ifdef WIN32 + wchar_t wide_name[LOCALE_NAME_MAX_LENGTH]; + char name[LOCALE_NAME_MAX_LENGTH]; + + /* use Windows API to find the default in BCP47 format */ + if (GetUserDefaultLocaleName(wide_name, LOCALE_NAME_MAX_LENGTH) == 0) + pg_fatal("failed to get default locale name: error code %lu", + GetLastError()); + if (WideCharToMultiByte(CP_ACP, 0, wide_name, -1, name, + LOCALE_NAME_MAX_LENGTH, NULL, NULL) == 0) + pg_fatal("failed to convert locale name: error code %lu", + GetLastError()); + locale_copy = pg_strdup(name); +#else + /* use environment to find the default */ + locale_copy = pg_strdup(""); +#endif + } + else + locale_copy = pg_strdup(locale); /* set the locale with setlocale, to see if it accepts it. */ - res = setlocale(category, locale); + res = setlocale(category, locale_copy); /* save canonical name if requested. */ if (res && canonname) @@ -2183,6 +2208,8 @@ check_locale_name(int category, const char *locale, char **canonname) pg_fatal("invalid locale settings; check LANG and LC_* environment variables"); } } + + free(locale_copy); } /* -- 2.39.2 [text/x-patch] v6-0003-Don-t-allow-UTF-8-with-non-UTF-8-locales-on-Windo.patch (4.5K, ../../CA+hUKGJ=ca39Cg=y=S89EaCYvvCF8NrZRO=uog-cnz0VzC6Kfg@mail.gmail.com/4-v6-0003-Don-t-allow-UTF-8-with-non-UTF-8-locales-on-Windo.patch) download | inline diff: From a4b0b0324900d12d487370a08b4ddba20552e230 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Wed, 7 Aug 2024 10:23:05 +1200 Subject: [PATCH v6 3/5] Don't allow UTF-8 with non-UTF-8 locales on Windows. Historically, we allowed contradictions such as: initdb.exe --locale="French_France.1252" --encoding="UTF-8" That's because Windows didn't support UTF-8 directly, and PostgreSQL had to perform UTF-8 char -> wchar_t conversions at various places on that OS, and still does. Therefore it was never actually passing UTF-8 text to operating system facilities. In preparation for removing those code paths, harmonizing the code and behavior with Unix builds, and adapting to modern Windows interfaces, ban such contradictions. Locale names should ideally be specified as BCP 47 tags. If UTF-8 is desired, the name should have ".UTF-8" on the end, but otherwise the traditional encoding of that language is implied. Now only the following are valid: initdb.exe --locale="fr-FR" --encoding="WIN1252" initdb.exe --locale="fr-FR.UTF-8" --encoding="UTF-8" initdb.exe --locale="French_France.1252" --encoding="WIN1252" (The last form is not recommended, but still accepted.) XXX This will cause problems for clusters upgraded with pg_upgrade from a system using locales with the wrong encoding. We'll need a way to translate to the correct modern locale names. Discussion: https://postgr.es/m/CA%2BhUKGJ%3DXThErgAQRoqfCy1bKPxXVuF0%3D2zDbB%2BSxDs59pv7Fw%40mail.gmail.com --- src/backend/commands/dbcommands.c | 12 +----------- src/bin/initdb/initdb.c | 11 ----------- 2 files changed, 1 insertion(+), 22 deletions(-) diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c index 7026352bc9..566085fecc 100644 --- a/src/backend/commands/dbcommands.c +++ b/src/backend/commands/dbcommands.c @@ -1555,11 +1555,7 @@ createdb(ParseState *pstate, const CreatedbStmt *stmt) * 2. locale encoding = -1, which means that we couldn't determine the * locale's encoding and have to trust the user to get it right. * - * 3. selected encoding is UTF8 and platform is win32. This is because - * UTF8 is a pseudo codepage that is supported in all locales since it's - * converted to UTF16 before being used. - * - * 4. selected encoding is SQL_ASCII, but only if you're a superuser. This + * 3. selected encoding is SQL_ASCII, but only if you're a superuser. This * is risky but we have historically allowed it --- notably, the * regression tests require it. * @@ -1574,9 +1570,6 @@ check_encoding_locale_matches(int encoding, const char *collate, const char *cty if (!(ctype_encoding == encoding || ctype_encoding == PG_SQL_ASCII || ctype_encoding == -1 || -#ifdef WIN32 - encoding == PG_UTF8 || -#endif (encoding == PG_SQL_ASCII && superuser()))) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), @@ -1589,9 +1582,6 @@ check_encoding_locale_matches(int encoding, const char *collate, const char *cty if (!(collate_encoding == encoding || collate_encoding == PG_SQL_ASCII || collate_encoding == -1 || -#ifdef WIN32 - encoding == PG_UTF8 || -#endif (encoding == PG_SQL_ASCII && superuser()))) ereport(ERROR, (errcode(ERRCODE_INVALID_PARAMETER_VALUE), diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index 393232b6ce..77bf815919 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -2228,9 +2228,6 @@ check_locale_encoding(const char *locale, int user_enc) if (!(locale_enc == user_enc || locale_enc == PG_SQL_ASCII || locale_enc == -1 || -#ifdef WIN32 - user_enc == PG_UTF8 || -#endif user_enc == PG_SQL_ASCII)) { pg_log_error("encoding mismatch"); @@ -2695,13 +2692,6 @@ setup_locale_encoding(void) * Windows, UTF-8 works with any locale, so we can fall back to * UTF-8. */ -#ifdef WIN32 - encodingid = PG_UTF8; - printf(_("Encoding \"%s\" implied by locale is not allowed as a server-side encoding.\n" - "The default database encoding will be set to \"%s\" instead.\n"), - pg_encoding_to_char(ctype_enc), - pg_encoding_to_char(encodingid)); -#else pg_log_error("locale \"%s\" requires unsupported encoding \"%s\"", lc_ctype, pg_encoding_to_char(ctype_enc)); pg_log_error_detail("Encoding \"%s\" is not allowed as a server-side encoding.", @@ -2709,7 +2699,6 @@ setup_locale_encoding(void) pg_log_error_hint("Rerun %s with a different locale selection.", progname); exit(1); -#endif } else { -- 2.39.2 [text/x-patch] v6-0004-Collate-UTF-8-without-wchar_t-conversion-in-Windo.patch (3.7K, ../../CA+hUKGJ=ca39Cg=y=S89EaCYvvCF8NrZRO=uog-cnz0VzC6Kfg@mail.gmail.com/5-v6-0004-Collate-UTF-8-without-wchar_t-conversion-in-Windo.patch) download | inline diff: From 5e8689b50db21fe5adfcee15f54524eefe64c492 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Wed, 7 Aug 2024 10:36:15 +1200 Subject: [PATCH v6 4/5] Collate UTF-8 without wchar_t conversion in Windows. Traditionally, Windows didn't support UTF-8 encoding in system interfaces, and we had to convert to UTF-16 and use wcscoll_l(). Windows 10+ has UTF-8 support, and an earlier commit banned the use of locales with encoding that doesn't match the database, so we can now harmonize with the Unix code paths and just call strcoll_l(). Discussion: https://postgr.es/m/CA%2BhUKGJ%3DXThErgAQRoqfCy1bKPxXVuF0%3D2zDbB%2BSxDs59pv7Fw%40mail.gmail.com --- src/backend/utils/adt/pg_locale.c | 90 +------------------------------ 1 file changed, 1 insertion(+), 89 deletions(-) diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c index cd3661e727..4d3c3e4e75 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -1804,78 +1804,6 @@ get_collation_actual_version(char collprovider, const char *collcollate) return collversion; } -/* - * pg_strncoll_libc_win32_utf8 - * - * Win32 does not have UTF-8. Convert UTF8 arguments to wide characters and - * invoke wcscoll_l(). - */ -#ifdef WIN32 -static int -pg_strncoll_libc_win32_utf8(const char *arg1, size_t len1, const char *arg2, - size_t len2, pg_locale_t locale) -{ - char sbuf[TEXTBUFLEN]; - char *buf = sbuf; - char *a1p, - *a2p; - int a1len = len1 * 2 + 2; - int a2len = len2 * 2 + 2; - int r; - int result; - - Assert(locale->provider == COLLPROVIDER_LIBC); - Assert(GetDatabaseEncoding() == PG_UTF8); -#ifndef WIN32 - Assert(false); -#endif - - if (a1len + a2len > TEXTBUFLEN) - buf = palloc(a1len + a2len); - - a1p = buf; - a2p = buf + a1len; - - /* API does not work for zero-length input */ - if (len1 == 0) - r = 0; - else - { - r = MultiByteToWideChar(CP_UTF8, 0, arg1, len1, - (LPWSTR) a1p, a1len / 2); - if (!r) - ereport(ERROR, - (errmsg("could not convert string to UTF-16: error code %lu", - GetLastError()))); - } - ((LPWSTR) a1p)[r] = 0; - - if (len2 == 0) - r = 0; - else - { - r = MultiByteToWideChar(CP_UTF8, 0, arg2, len2, - (LPWSTR) a2p, a2len / 2); - if (!r) - ereport(ERROR, - (errmsg("could not convert string to UTF-16: error code %lu", - GetLastError()))); - } - ((LPWSTR) a2p)[r] = 0; - - errno = 0; - result = wcscoll_l((LPWSTR) a1p, (LPWSTR) a2p, locale->info.lt); - if (result == 2147483647) /* _NLSCMPERROR; missing from mingw headers */ - ereport(ERROR, - (errmsg("could not compare Unicode strings: %m"))); - - if (buf != sbuf) - pfree(buf); - - return result; -} -#endif /* WIN32 */ - /* * pg_strcoll_libc * @@ -1891,17 +1819,7 @@ pg_strcoll_libc(const char *arg1, const char *arg2, pg_locale_t locale) int result; Assert(locale->provider == COLLPROVIDER_LIBC); -#ifdef WIN32 - if (GetDatabaseEncoding() == PG_UTF8) - { - size_t len1 = strlen(arg1); - size_t len2 = strlen(arg2); - - result = pg_strncoll_libc_win32_utf8(arg1, len1, arg2, len2, locale); - } - else -#endif /* WIN32 */ - result = strcoll_l(arg1, arg2, locale->info.lt); + result = strcoll_l(arg1, arg2, locale->info.lt); return result; } @@ -1925,12 +1843,6 @@ pg_strncoll_libc(const char *arg1, size_t len1, const char *arg2, size_t len2, Assert(locale->provider == COLLPROVIDER_LIBC); -#ifdef WIN32 - /* check for this case before doing the work for nul-termination */ - if (GetDatabaseEncoding() == PG_UTF8) - return pg_strncoll_libc_win32_utf8(arg1, len1, arg2, len2, locale); -#endif /* WIN32 */ - if (bufsize1 + bufsize2 > TEXTBUFLEN) buf = palloc(bufsize1 + bufsize2); -- 2.39.2 [text/x-patch] v6-0005-Format-times-without-wchar_t-conversion-in-Window.patch (9.0K, ../../CA+hUKGJ=ca39Cg=y=S89EaCYvvCF8NrZRO=uog-cnz0VzC6Kfg@mail.gmail.com/6-v6-0005-Format-times-without-wchar_t-conversion-in-Window.patch) download | inline diff: From 05a451df747f219192fbf79d833ac50285048dbf Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Wed, 7 Aug 2024 11:33:22 +1200 Subject: [PATCH v6 5/5] Format times without wchar_t conversion in Windows. Previously we allowed the locale to be set to something that used an encoding that didn't match the database. We have disallowed that now, so we can use strftime() directly. And if we're going to touch that code, we might as well use strftime_l() instead and skip some ugly save/restore of global state. strftime_l() is from POSIX 2008. All supported systems have it, thought Windows has a leading underscore. For the CI MinGW cross-build warning check to pass, add -lucrt because otherwise strftime_l() is not available. Discussion: https://postgr.es/m/CA%2BhUKGJ%3DXThErgAQRoqfCy1bKPxXVuF0%3D2zDbB%2BSxDs59pv7Fw%40mail.gmail.com --- .cirrus.tasks.yml | 1 + src/backend/utils/adt/pg_locale.c | 147 ++++-------------------------- src/include/port/win32_port.h | 1 + 3 files changed, 21 insertions(+), 128 deletions(-) diff --git a/.cirrus.tasks.yml b/.cirrus.tasks.yml index 1ce6c443a8..3bf81ed4af 100644 --- a/.cirrus.tasks.yml +++ b/.cirrus.tasks.yml @@ -753,6 +753,7 @@ task: --host=x86_64-w64-mingw32 \ --enable-cassert \ --without-icu \ + LDFLAGS="-lucrt" \ CC="ccache x86_64-w64-mingw32-gcc" \ CXX="ccache x86_64-w64-mingw32-g++" make -s -j${BUILD_JOBS} clean diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c index 4d3c3e4e75..5e64470b58 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -174,6 +174,8 @@ static void icu_set_collation_attributes(UCollator *collator, const char *loc, UErrorCode *status); #endif +static void report_newlocale_failure(const char *localename); + /* * POSIX doesn't define _l-variants of these functions, but several systems * have them. We provide our own replacements here. @@ -732,65 +734,6 @@ PGLC_localeconv(void) return &CurrentLocaleConv; } -#ifdef WIN32 -/* - * On Windows, strftime() returns its output in encoding CP_ACP (the default - * operating system codepage for the computer), which is likely different - * from SERVER_ENCODING. This is especially important in Japanese versions - * of Windows which will use SJIS encoding, which we don't support as a - * server encoding. - * - * So, instead of using strftime(), use wcsftime() to return the value in - * wide characters (internally UTF16) and then convert to UTF8, which we - * know how to handle directly. - * - * Note that this only affects the calls to strftime() in this file, which are - * used to get the locale-aware strings. Other parts of the backend use - * pg_strftime(), which isn't locale-aware and does not need to be replaced. - */ -static size_t -strftime_win32(char *dst, size_t dstlen, - const char *format, const struct tm *tm) -{ - size_t len; - wchar_t wformat[8]; /* formats used below need 3 chars */ - wchar_t wbuf[MAX_L10N_DATA]; - - /* - * Get a wchar_t version of the format string. We only actually use - * plain-ASCII formats in this file, so we can say that they're UTF8. - */ - len = MultiByteToWideChar(CP_UTF8, 0, format, -1, - wformat, lengthof(wformat)); - if (len == 0) - elog(ERROR, "could not convert format string from UTF-8: error code %lu", - GetLastError()); - - len = wcsftime(wbuf, MAX_L10N_DATA, wformat, tm); - if (len == 0) - { - /* - * wcsftime failed, possibly because the result would not fit in - * MAX_L10N_DATA. Return 0 with the contents of dst unspecified. - */ - return 0; - } - - len = WideCharToMultiByte(CP_UTF8, 0, wbuf, len, dst, dstlen - 1, - NULL, NULL); - if (len == 0) - elog(ERROR, "could not convert string to UTF-8: error code %lu", - GetLastError()); - - dst[len] = '\0'; - - return len; -} - -/* redefine strftime() */ -#define strftime(a,b,c,d) strftime_win32(a,b,c,d) -#endif /* WIN32 */ - /* * Subroutine for cache_locale_time(). * Convert the given string from encoding "encoding" to the database @@ -829,10 +772,7 @@ cache_locale_time(void) bool strftimefail = false; int encoding; int i; - char *save_lc_time; -#ifdef WIN32 - char *save_lc_ctype; -#endif + locale_t locale; /* did we do this already? */ if (CurrentLCTimeValid) @@ -840,50 +780,24 @@ cache_locale_time(void) elog(DEBUG3, "cache_locale_time() executed; locale: \"%s\"", locale_time); - /* - * As in PGLC_localeconv(), it's critical that we not throw error while - * libc's locale settings have nondefault values. Hence, we just call - * strftime() within the critical section, and then convert and save its - * results afterwards. - */ - - /* Save prevailing value of time locale */ - save_lc_time = setlocale(LC_TIME, NULL); - if (!save_lc_time) - elog(ERROR, "setlocale(NULL) failed"); - save_lc_time = pstrdup(save_lc_time); - #ifdef WIN32 - - /* - * On Windows, it appears that wcsftime() internally uses LC_CTYPE, so we - * must set it here. This code looks the same as what PGLC_localeconv() - * does, but the underlying reason is different: this does NOT determine - * the encoding we'll get back from strftime_win32(). - */ - - /* Save prevailing value of ctype locale */ - save_lc_ctype = setlocale(LC_CTYPE, NULL); - if (!save_lc_ctype) - elog(ERROR, "setlocale(NULL) failed"); - save_lc_ctype = pstrdup(save_lc_ctype); - - /* use lc_time to set the ctype */ - setlocale(LC_CTYPE, locale_time); + locale = _create_locale(LC_ALL, locale_time); +#else + locale = newlocale(LC_ALL, locale_time, NULL); #endif + if (!locale) + report_newlocale_failure(locale_time); - setlocale(LC_TIME, locale_time); - - /* We use times close to current time as data for strftime(). */ + /* We use times close to current time as data for strftime_l(). */ timenow = time(NULL); timeinfo = localtime(&timenow); - /* Store the strftime results in MAX_L10N_DATA-sized portions of buf[] */ + /* Store the strftime_l results in MAX_L10N_DATA-sized portions of buf[] */ bufptr = buf; /* * MAX_L10N_DATA is sufficient buffer space for every known locale, and - * POSIX defines no strftime() errors. (Buffer space exhaustion is not an + * POSIX defines no strftime_l() errors. (Buffer space exhaustion is not an * error.) An implementation might report errors (e.g. ENOMEM) by * returning 0 (or, less plausibly, a negative value) and setting errno. * Report errno just in case the implementation did that, but clear it in @@ -895,10 +809,10 @@ cache_locale_time(void) for (i = 0; i < 7; i++) { timeinfo->tm_wday = i; - if (strftime(bufptr, MAX_L10N_DATA, "%a", timeinfo) <= 0) + if (strftime_l(bufptr, MAX_L10N_DATA, "%a", timeinfo, locale) <= 0) strftimefail = true; bufptr += MAX_L10N_DATA; - if (strftime(bufptr, MAX_L10N_DATA, "%A", timeinfo) <= 0) + if (strftime_l(bufptr, MAX_L10N_DATA, "%A", timeinfo, locale) <= 0) strftimefail = true; bufptr += MAX_L10N_DATA; } @@ -908,39 +822,26 @@ cache_locale_time(void) { timeinfo->tm_mon = i; timeinfo->tm_mday = 1; /* make sure we don't have invalid date */ - if (strftime(bufptr, MAX_L10N_DATA, "%b", timeinfo) <= 0) + if (strftime_l(bufptr, MAX_L10N_DATA, "%b", timeinfo, locale) <= 0) strftimefail = true; bufptr += MAX_L10N_DATA; - if (strftime(bufptr, MAX_L10N_DATA, "%B", timeinfo) <= 0) + if (strftime_l(bufptr, MAX_L10N_DATA, "%B", timeinfo, locale) <= 0) strftimefail = true; bufptr += MAX_L10N_DATA; } - /* - * Restore the prevailing locale settings; as in PGLC_localeconv(), - * failure to do so is fatal. - */ #ifdef WIN32 - if (!setlocale(LC_CTYPE, save_lc_ctype)) - elog(FATAL, "failed to restore LC_CTYPE to \"%s\"", save_lc_ctype); + _free_locale(locale); +#else + freelocale(locale); #endif - if (!setlocale(LC_TIME, save_lc_time)) - elog(FATAL, "failed to restore LC_TIME to \"%s\"", save_lc_time); /* * At this point we've done our best to clean up, and can throw errors, or * call functions that might throw errors, with a clean conscience. */ if (strftimefail) - elog(ERROR, "strftime() failed: %m"); - - /* Release the pstrdup'd locale names */ - pfree(save_lc_time); -#ifdef WIN32 - pfree(save_lc_ctype); -#endif - -#ifndef WIN32 + elog(ERROR, "strftime_() failed: %m"); /* * As in PGLC_localeconv(), we must convert strftime()'s output from the @@ -951,16 +852,6 @@ cache_locale_time(void) if (encoding < 0) encoding = PG_SQL_ASCII; -#else - - /* - * On Windows, strftime_win32() always returns UTF8 data, so convert from - * that if necessary. - */ - encoding = PG_UTF8; - -#endif /* WIN32 */ - bufptr = buf; /* localized days */ diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h index 7ffe5891c6..87157a1095 100644 --- a/src/include/port/win32_port.h +++ b/src/include/port/win32_port.h @@ -450,6 +450,7 @@ extern int _pglstat64(const char *name, struct stat *buf); #define isspace_l _isspace_l #define iswspace_l _iswspace_l #define strcoll_l _strcoll_l +#define strftime_l _strftime_l #define strxfrm_l _strxfrm_l #define wcscoll_l _wcscoll_l -- 2.39.2 ^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Windows default locale vs initdb @ 2024-08-08 08:08 Ertan Küçükoglu <[email protected]> parent: Ertan Küçükoglu <[email protected]> 0 siblings, 1 reply; 22+ messages in thread From: Ertan Küçükoglu @ 2024-08-08 08:08 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: pgsql-hackers; Noah Misch <[email protected]>; Juan José Santamaría Flecha <[email protected]> > > I already installed Visual Studio 2022 with C++ support as suggested in > https://www.postgresql.org/docs/current/install-windows-full.html > I cloned codes in the system. > But, I cannot find any "src/tools/msvc" directory. It is missing. > Document states I need everything in there > "The tools for building using Visual C++ or Platform SDK are in the > src\tools\msvc directory." > It seems I will need help setting up the build environment. > I am willing to be a tester for Windows given I could get help setting up the build environment. It also feels documentation needs some update as I failed to find necessary files. Thanks & Regards, Ertan ^ permalink raw reply [nested|flat] 22+ messages in thread
* Re: Windows default locale vs initdb @ 2024-08-08 11:39 Andrew Dunstan <[email protected]> parent: Ertan Küçükoglu <[email protected]> 0 siblings, 0 replies; 22+ messages in thread From: Andrew Dunstan @ 2024-08-08 11:39 UTC (permalink / raw) To: Ertan Küçükoglu <[email protected]>; Thomas Munro <[email protected]>; +Cc: pgsql-hackers; Noah Misch <[email protected]>; Juan José Santamaría Flecha <[email protected]> On 2024-08-08 Th 4:08 AM, Ertan Küçükoglu wrote: > > I already installed Visual Studio 2022 with C++ support as > suggested in > https://www.postgresql.org/docs/current/install-windows-full.html > I cloned codes in the system. > But, I cannot find any "src/tools/msvc" directory. It is missing. > Document states I need everything in there > "The tools for building using Visual C++ or Platform SDK are in > the src\tools\msvc directory." > It seems I will need help setting up the build environment. > > > I am willing to be a tester for Windows given I could get help setting > up the build environment. > It also feels documentation needs some update as I failed to find > necessary files. If you're trying to build the master branch those documents no longer apply. You will need to build using meson, as documented here: <https://www.postgresql.org/docs/17/install-meson.html; cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 22+ messages in thread
end of thread, other threads:[~2024-08-08 11:39 UTC | newest] Thread overview: 22+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-12-13 19:58 Re: Windows default locale vs initdb Thomas Munro <[email protected]> 2024-07-22 02:51 ` Thomas Munro <[email protected]> 2024-07-22 08:04 ` Ertan Küçükoglu <[email protected]> 2024-07-22 11:01 ` Thomas Munro <[email protected]> 2024-07-22 12:52 ` Ertan Küçükoglu <[email protected]> 2024-08-08 08:08 ` Ertan Küçükoglu <[email protected]> 2024-08-08 11:39 ` Andrew Dunstan <[email protected]> 2024-07-22 08:37 ` Zaid Shabbir <[email protected]> 2024-07-22 10:12 ` Thomas Munro <[email protected]> 2024-07-22 13:44 ` Andrew Dunstan <[email protected]> 2024-07-22 13:51 ` Ertan Küçükoglu <[email protected]> 2024-07-22 23:19 ` Thomas Munro <[email protected]> 2024-08-07 04:15 ` Thomas Munro <[email protected]> 2024-01-18 03:54 Re: Fix search_path for all maintenance commands Shubham Khanna <[email protected]> 2024-01-18 05:11 ` Re: Fix search_path for all maintenance commands Jeff Davis <[email protected]> 2024-03-03 14:20 [PATCH] rework locking code in GetMultiXactIdMembers Alvaro Herrera <[email protected]> 2024-03-03 14:20 [PATCH v2 1/3] rework locking code in GetMultiXactIdMembers Alvaro Herrera <[email protected]> 2024-03-03 14:20 [PATCH v2 1/3] rework locking code in GetMultiXactIdMembers Alvaro Herrera <[email protected]> 2024-03-03 14:20 [PATCH] rework locking code in GetMultiXactIdMembers Alvaro Herrera <[email protected]> 2024-03-03 14:20 [PATCH v2 1/3] rework locking code in GetMultiXactIdMembers Alvaro Herrera <[email protected]> 2024-03-03 14:20 [PATCH] rework locking code in GetMultiXactIdMembers Alvaro Herrera <[email protected]> 2024-03-03 14:20 [PATCH] rework locking code in GetMultiXactIdMembers Alvaro Herrera <[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